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

From: Ravi Bangoria <ravi.bangoria@amd.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: acme@kernel.org, jolsa@kernel.org, namhyung@kernel.org,
	eranian@google.com, irogers@google.com, jmario@redhat.com,
	leo.yan@linaro.org, alisaidi@amazon.com, ak@linux.intel.com,
	kan.liang@linux.intel.com, dave.hansen@linux.intel.com,
	hpa@zytor.com, mingo@redhat.com, mark.rutland@arm.com,
	alexander.shishkin@linux.intel.com, tglx@linutronix.de,
	bp@alien8.de, x86@kernel.org, linux-perf-users@vger.kernel.org,
	linux-kernel@vger.kernel.org, sandipan.das@amd.com,
	ananth.narayan@amd.com, kim.phillips@amd.com,
	santosh.shukla@amd.com, Ravi Bangoria <ravi.bangoria@amd.com>
Subject: Re: [PATCH 06/13] perf/x86/amd: Support PERF_SAMPLE_PHY_ADDR using IBS_DC_PHYSADDR
Date: Thu, 26 May 2022 14:16:28 +0530	[thread overview]
Message-ID: <e10c3d1b-90a9-2f14-987b-9c0d2471ee53@amd.com> (raw)
In-Reply-To: <Yo4RSY2L80N5muJ9@hirez.programming.kicks-ass.net>

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
