Re: [PATCH 7/7] perf arm-spe: Don't hard code config attribute

[James Clark <james.clark@linaro.org>]

On 02/12/2025 12:42 pm, Leo Yan wrote:
> On Tue, Dec 02, 2025 at 12:28:35PM +0000, Coresight ML wrote:
>> On Mon, Dec 01, 2025 at 04:41:10PM +0000, Coresight ML wrote:
>>> Use the config attribute that's published by the driver instead of
>>> hard coding "attr.config".
>>>
>>> Signed-off-by: James Clark <james.clark@linaro.org>
>>> ---
>>>   tools/perf/arch/arm64/util/arm-spe.c | 15 ++++++++-------
>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/perf/arch/arm64/util/arm-spe.c b/tools/perf/arch/arm64/util/arm-spe.c
>>> index d5ec1408d0ae..6c3dc97fde30 100644
>>> --- a/tools/perf/arch/arm64/util/arm-spe.c
>>> +++ b/tools/perf/arch/arm64/util/arm-spe.c
>>> @@ -256,7 +256,7 @@ static __u64 arm_spe_pmu__sample_period(const struct perf_pmu *arm_spe_pmu)
>>>   
>>>   static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>>>   {
>>> -	u64 bit;
>>> +	u64 pa_enable_bit;
>>>   
>>>   	evsel->core.attr.freq = 0;
>>>   	evsel->core.attr.sample_period = arm_spe_pmu__sample_period(evsel->pmu);
>>> @@ -288,9 +288,10 @@ static void arm_spe_setup_evsel(struct evsel *evsel, struct perf_cpu_map *cpus)
>>>   	 * inform that the resulting output's SPE samples contain physical addresses
>>>   	 * where applicable.
>>>   	 */
>>> -	bit = perf_pmu__format_bits(evsel->pmu, "pa_enable");
>>> -	if (evsel->core.attr.config & bit)
>>> -		evsel__set_sample_bit(evsel, PHYS_ADDR);
>>> +
>>> +	if (!evsel__get_config_val(evsel->pmu, evsel, "pa_enable", &pa_enable_bit))
>>> +		if (pa_enable_bit)
>>> +			evsel__set_sample_bit(evsel, PHYS_ADDR);
>>
>> Hmm... I am a bit concerned for the evsel__get_config_val() usage
>> throughout the series.
>>
>> evsel__get_config_val() returns a whole config value rather than the
>> bit field specified by the format name.  If other bits (but not the
>> "pa_enable" bit) are set in the same config set, would it wrongly set
>> the PHYS_ADDR sample bit?
> 
> I saw you used FIELD_GET() to read specific bit field in
> evsel__get_config_val().  This is not concerned anymore.
> 
> Sorry for noise.
> 
>> Seems to me, for reading specific format, perf_pmu__format_bits() is
>> more suitable than evsel__get_config_val().
> 
> For me, this comment is still valid, given perf_pmu__format_bits()
> will always exist.
> 

perf_pmu__format_bits() only returns the mask for the format attribute 
that's published by the kernel. It never changes depending on what 
options the user provides and doesn't even say which config field that 
mask applies to. It's fairly useless on its own and should probably be 
made private to pmu.c if we manage to refactor its uses out of 
intel-pt.c. Although they're a bit more involved there than here so I 
won't touch it now.

evsel__get_config_val() returns the value that the user provided for a 
named attribute which is what we need to do here.

> Thanks,
> Leo