Re: [PATCH V2 2/2] tools/perf/powerpc/util: Add support to handle compatible mode PVR for perf json events

[Madhavan Srinivasan <maddy@linux.ibm.com>]

On 3/21/25 4:04 PM, Christophe Leroy wrote:
> Hi,
> 
> Le 10/10/2024 à 16:51, Athira Rajeev a écrit :
>> perf list picks the events supported for specific platform
>> from pmu-events/arch/powerpc/<platform>. Example power10 events
>> are in pmu-events/arch/powerpc/power10, power9 events are part
>> of pmu-events/arch/powerpc/power9. The decision of which
>> platform to pick is determined based on PVR value in powerpc.
>> The PVR value is matched from pmu-events/arch/powerpc/mapfile.csv
>>
>> Example:
>>
>> Format:
>>     PVR,Version,JSON/file/pathname,Type
>>
>> 0x004[bcd][[:xdigit:]]{4},1,power8,core
>> 0x0066[[:xdigit:]]{4},1,power8,core
>> 0x004e[[:xdigit:]]{4},1,power9,core
>> 0x0080[[:xdigit:]]{4},1,power10,core
>> 0x0082[[:xdigit:]]{4},1,power10,core
>>
>> The code gets the PVR from system using get_cpuid_str function
>> in arch/powerpc/util/headers.c ( from SPRN_PVR ) and compares
>> with value from mapfile.csv
>> In case of compat mode, say when partition is booted in a power9
>> mode when the system is a power10, this picks incorrectly. Because
>> PVR will point to power10 where as it should pick events from power9
>> folder. To support generic events, add new folder
>> pmu-events/arch/powerpc/compat to contain the ISA architected events
>> which is supported in compat mode. Also return 0x00ffffff as pvr
>> when booted in compat mode. Based on this pvr value, json will
>> pick events from pmu-events/arch/powerpc/compat
>>
>> Suggested-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> 
> I see this patch was merged into mainline allthough it had CI failures and still has.
> 
> Could you please fix it ?

We already have a fix patch posted for this issue 

https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20250321100726.699956-1-likhitha@linux.ibm.com/

Maddy


> 
> arch/powerpc/util/header.c: In function 'is_compat_mode':
> Error: arch/powerpc/util/header.c:20:14: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    20 |  if (!strcmp((char *)platform, (char *)base_platform))
>       |              ^
> Error: arch/powerpc/util/header.c:20:32: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>    20 |  if (!strcmp((char *)platform, (char *)base_platform))
>       |                                ^
> cc1: all warnings being treated as errors
> make[6]: *** [/linux/tools/build/Makefile.build:86: /output/arch/powerpc/util/header.o] Error 1
> 
> 
> The following fix but is maybe not the right one as in reality getauxval() seems to return a long not a u64.
> 
> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
> index c7df534dbf8f..1b045d410f31 100644
> --- a/tools/perf/arch/powerpc/util/header.c
> +++ b/tools/perf/arch/powerpc/util/header.c
> @@ -17,7 +17,7 @@ static bool is_compat_mode(void)
>      u64 base_platform = getauxval(AT_BASE_PLATFORM);
>      u64 platform = getauxval(AT_PLATFORM);
> 
> -    if (!strcmp((char *)platform, (char *)base_platform))
> +    if (!strcmp((char *)(long)platform, (char *)(long)base_platform))
>          return false;
> 
>      return true;
> 
> 
> Thanks
> Christophe
> 
>> ---
>> changelog:
>> V1 -> V2:
>> Corrected commit message and subject line
>>
>>   tools/perf/arch/powerpc/util/header.c | 32 ++++++++++++++++++++++++++-
>>   1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/arch/powerpc/util/header.c b/tools/perf/arch/powerpc/util/header.c
>> index 6b00efd53638..adc82c479443 100644
>> --- a/tools/perf/arch/powerpc/util/header.c
>> +++ b/tools/perf/arch/powerpc/util/header.c
>> @@ -10,6 +10,18 @@
>>   #include "utils_header.h"
>>   #include "metricgroup.h"
>>   #include <api/fs/fs.h>
>> +#include <sys/auxv.h>
>> +
>> +static bool is_compat_mode(void)
>> +{
>> +    u64 base_platform = getauxval(AT_BASE_PLATFORM);
>> +    u64 platform = getauxval(AT_PLATFORM);
>> +
>> +    if (!strcmp((char *)platform, (char *)base_platform))
>> +        return false;
>> +
>> +    return true;
>> +}
>>     int
>>   get_cpuid(char *buffer, size_t sz)
>> @@ -33,8 +45,26 @@ char *
>>   get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
>>   {
>>       char *bufp;
>> +    unsigned long pvr;
>> +
>> +    /*
>> +     * IBM Power System supports compatible mode. That is
>> +     * Nth generation platform can support previous generation
>> +     * OS in a mode called compatibile mode. For ex. LPAR can be
>> +     * booted in a Power9 mode when the system is a Power10.
>> +     *
>> +     * In the compatible mode, care must be taken when generating
>> +     * PVR value. When read, PVR will be of the AT_BASE_PLATFORM
>> +     * To support generic events, return 0x00ffffff as pvr when
>> +     * booted in compat mode. Based on this pvr value, json will
>> +     * pick events from pmu-events/arch/powerpc/compat
>> +     */
>> +    if (!is_compat_mode())
>> +        pvr = mfspr(SPRN_PVR);
>> +    else
>> +        pvr = 0x00ffffff;
>>   -    if (asprintf(&bufp, "0x%.8lx", mfspr(SPRN_PVR)) < 0)
>> +    if (asprintf(&bufp, "0x%.8lx", pvr) < 0)
>>           bufp = NULL;
>>         return bufp;
>