Re: [PATCH 06/13] perf/x86/amd: Support PERF_SAMPLE_PHY_ADDR using IBS_DC_PHYSADDR

[Ravi Bangoria <ravi.bangoria@amd.com>]

On 25-May-22 4:51 PM, Peter Zijlstra wrote:
> On Wed, May 25, 2022 at 03:09:31PM +0530, Ravi Bangoria wrote:
>> IBS_DC_PHYSADDR provides the physical data address for the tagged load/
>> store operation. Populate perf sample physical address using it.
>>
>> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
>> ---
>>  arch/x86/events/amd/ibs.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
>> index b57736357e25..c719020c0e83 100644
>> --- a/arch/x86/events/amd/ibs.c
>> +++ b/arch/x86/events/amd/ibs.c
>> @@ -986,13 +986,35 @@ static void perf_ibs_get_data_addr(struct perf_event *event,
>>  	data->addr = ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)];
>>  }
>>  
>> +static void perf_ibs_get_phy_addr(struct perf_event *event,
>> +				  struct perf_ibs_data *ibs_data,
>> +				  struct perf_sample_data *data)
>> +{
>> +	union perf_mem_data_src *data_src = &data->data_src;
>> +	u64 op_data3 = ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSOPDATA3)];
>> +	u64 phy_addr_valid = op_data3 & IBS_DC_PHY_ADDR_VALID_MASK;
>> +
>> +	if (!(event->attr.sample_type & PERF_SAMPLE_DATA_SRC))
>> +		perf_ibs_get_mem_op(op_data3, data);
>> +
>> +	if ((data_src->mem_op != PERF_MEM_OP_LOAD &&
>> +	    data_src->mem_op != PERF_MEM_OP_STORE) ||
>> +	    !phy_addr_valid) {
>> +		data->phys_addr = 0x0;
>> +		return;
>> +	}
>> +
>> +	data->phys_addr = ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSDCPHYSAD)];
>> +}
> 
> perf_prepare_sample() will unconditionally overwrite data->phys_addr.
> There is currently no facility to let the driver set this field.

Thanks for pointing it Peter. Would you mind if I add:

diff --git a/arch/x86/events/amd/ibs.c b/arch/x86/events/amd/ibs.c
index c719020c0e83..fbd1f4e94d47 100644
--- a/arch/x86/events/amd/ibs.c
+++ b/arch/x86/events/amd/ibs.c
@@ -986,6 +986,19 @@ static void perf_ibs_get_data_addr(struct perf_event *event,
        data->addr = ibs_data->regs[ibs_op_msr_idx(MSR_AMD64_IBSDCLINAD)];
 }

+/* data_src->mem_op should have been set by perf_ibs_get_phy_addr() */
+bool perf_arch_phys_addr_set(struct perf_event *event,
+                            struct perf_sample_data *data)
+{
+       union perf_mem_data_src *data_src = &data->data_src;
+
+       if (event->pmu != &perf_ibs_op.pmu)
+               return false;
+
+       return (data_src->mem_op == PERF_MEM_OP_LOAD ||
+               data_src->mem_op == PERF_MEM_OP_STORE);
+}
+
 static void perf_ibs_get_phy_addr(struct perf_event *event,
                                  struct perf_ibs_data *ibs_data,
                                  struct perf_sample_data *data)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index da759560eec5..67402af3b70f 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1477,6 +1477,8 @@ extern void perf_event_task_tick(void);
 extern int perf_event_account_interrupt(struct perf_event *event);
 extern int perf_event_period(struct perf_event *event, u64 value);
 extern u64 perf_event_pause(struct perf_event *event, bool reset);
+bool perf_arch_phys_addr_set(struct perf_event *event,
+                            struct perf_sample_data *data);
 #else /* !CONFIG_PERF_EVENTS: */
 static inline void *
 perf_aux_output_begin(struct perf_output_handle *handle,
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7699be46f3a1..9baeb2d21bc0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7283,6 +7283,12 @@ perf_callchain(struct perf_event *event, struct pt_regs *regs)
        return callchain ?: &__empty_callchain;
 }

+bool __weak perf_arch_phys_addr_set(struct perf_event *event,
+                                   struct perf_sample_data *data)
+{
+       return false;
+}
+
 void perf_prepare_sample(struct perf_event_header *header,
                         struct perf_sample_data *data,
                         struct perf_event *event,
@@ -7404,8 +7410,10 @@ void perf_prepare_sample(struct perf_event_header *header,
                header->size += size;
        }

-       if (sample_type & PERF_SAMPLE_PHYS_ADDR)
+       if (sample_type & PERF_SAMPLE_PHYS_ADDR &&
+           !perf_arch_phys_addr_set(event, data)) {
                data->phys_addr = perf_virt_to_phys(data->addr);
+       }

 #ifdef CONFIG_CGROUP_PERF
        if (sample_type & PERF_SAMPLE_CGROUP) {

Thanks,
Ravi