Re: [PATCH] powerpc/hv-gpci: Fix hv_gpci event list

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

On 10/18/22 1:35 PM, Michael Ellerman wrote:
> Kajol Jain <kjain@linux.ibm.com> writes:
>> Based on getPerfCountInfo v1.018 documentation, some of the
>> hv_gpci events got deprecated for platforms firmware that
>> supports counter_info_version 0x8 or above.
>>
>> Patch fixes the hv_gpci event list by adding a new attribute
>> group called "hv_gpci_event_attrs_v6" and a "EVENT_ENABLE"
>> macro to enable these events for platform firmware
>> that supports counter_info_version 0x6 or below.
> Does this handle CPUs booted in compat mode?
Nice catch. Sorry I missed that part completely during internal review. 
my bad.
> ie. where the firmware is newer but the kernel is told to behave as if
> the CPU is an older version - so cpu_has_feature() doesn't necessarily
> match the underlying hardware.
>
> Is there some reason the event list is populated based on the CPU
> features rather than by calling the hypervisor and asking what version
> is supported?
I will review the hcall doc again for that option.

maddy
>
>> Fixes: 97bf2640184f4 ("powerpc/perf/hv-gpci: add the remaining gpci
>> requests")
> Please don't wrap the fixes tag.
>
> cheers
>
>> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
>> ---
>>   arch/powerpc/perf/hv-gpci-requests.h |  4 ++++
>>   arch/powerpc/perf/hv-gpci.c          |  9 +++++++--
>>   arch/powerpc/perf/hv-gpci.h          |  1 +
>>   arch/powerpc/perf/req-gen/perf.h     | 17 +++++++++++++++++
>>   4 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/perf/hv-gpci-requests.h b/arch/powerpc/perf/hv-gpci-requests.h
>> index 8965b4463d43..baef3d082de9 100644
>> --- a/arch/powerpc/perf/hv-gpci-requests.h
>> +++ b/arch/powerpc/perf/hv-gpci-requests.h
>> @@ -79,6 +79,7 @@ REQUEST(__field(0,	8,	partition_id)
>>   )
>>   #include I(REQUEST_END)
>>   
>> +#ifdef EVENT_ENABLE
>>   /*
>>    * Not available for counter_info_version >= 0x8, use
>>    * run_instruction_cycles_by_partition(0x100) instead.
>> @@ -92,6 +93,7 @@ REQUEST(__field(0,	8,	partition_id)
>>   	__count(0x10,	8,	cycles)
>>   )
>>   #include I(REQUEST_END)
>> +#endif
>>   
>>   #define REQUEST_NAME system_performance_capabilities
>>   #define REQUEST_NUM 0x40
>> @@ -103,6 +105,7 @@ REQUEST(__field(0,	1,	perf_collect_privileged)
>>   )
>>   #include I(REQUEST_END)
>>   
>> +#ifdef EVENT_ENABLE
>>   #define REQUEST_NAME processor_bus_utilization_abc_links
>>   #define REQUEST_NUM 0x50
>>   #define REQUEST_IDX_KIND "hw_chip_id=?"
>> @@ -194,6 +197,7 @@ REQUEST(__field(0,	4,	phys_processor_idx)
>>   	__count(0x28,	8,	instructions_completed)
>>   )
>>   #include I(REQUEST_END)
>> +#endif
>>   
>>   /* Processor_core_power_mode (0x95) skipped, no counters */
>>   /* Affinity_domain_information_by_virtual_processor (0xA0) skipped,
>> diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c
>> index 5eb60ed5b5e8..065a01812b3e 100644
>> --- a/arch/powerpc/perf/hv-gpci.c
>> +++ b/arch/powerpc/perf/hv-gpci.c
>> @@ -70,9 +70,9 @@ static const struct attribute_group format_group = {
>>   	.attrs = format_attrs,
>>   };
>>   
>> -static const struct attribute_group event_group = {
>> +static struct attribute_group event_group = {
>>   	.name  = "events",
>> -	.attrs = hv_gpci_event_attrs,
>> +	/* .attrs is set in init */
>>   };
>>   
>>   #define HV_CAPS_ATTR(_name, _format)				\
>> @@ -353,6 +353,11 @@ static int hv_gpci_init(void)
>>   	/* sampling not supported */
>>   	h_gpci_pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
>>   
>> +	if (cpu_has_feature(CPU_FTR_ARCH_207S))
>> +		event_group.attrs = hv_gpci_event_attrs;
>> +	else
>> +		event_group.attrs = hv_gpci_event_attrs_v6;
>> +
>>   	r = perf_pmu_register(&h_gpci_pmu, h_gpci_pmu.name, -1);
>>   	if (r)
>>   		return r;
>> diff --git a/arch/powerpc/perf/hv-gpci.h b/arch/powerpc/perf/hv-gpci.h
>> index 4d108262bed7..866172c1651c 100644
>> --- a/arch/powerpc/perf/hv-gpci.h
>> +++ b/arch/powerpc/perf/hv-gpci.h
>> @@ -26,6 +26,7 @@ enum {
>>   #define REQUEST_FILE "../hv-gpci-requests.h"
>>   #define NAME_LOWER hv_gpci
>>   #define NAME_UPPER HV_GPCI
>> +#define EVENT_ENABLE	1
>>   #include "req-gen/perf.h"
>>   #undef REQUEST_FILE
>>   #undef NAME_LOWER
>> diff --git a/arch/powerpc/perf/req-gen/perf.h b/arch/powerpc/perf/req-gen/perf.h
>> index fa9bc804e67a..78d407e3fcc6 100644
>> --- a/arch/powerpc/perf/req-gen/perf.h
>> +++ b/arch/powerpc/perf/req-gen/perf.h
>> @@ -139,6 +139,23 @@ PMU_EVENT_ATTR_STRING(							\
>>   #define REQUEST_(r_name, r_value, r_idx_1, r_fields)			\
>>   	r_fields
>>   
>> +/* Generate event list for platforms with counter_info_version 0x6 or below */
>> +static __maybe_unused struct attribute *hv_gpci_event_attrs_v6[] = {
>> +#include REQUEST_FILE
>> +	NULL
>> +};
>> +
>> +/*
>> + * Based on getPerfCountInfo v1.018 documentation, some of the hv-gpci
>> + * events got deprecated for platforms firmware that supports
>> + * counter_info_version 0x8 or above.
>> + * Undefining macro EVENT_ENABLE, to disable the addition of deprecated
>> + * events in "hv_gpci_event_attrs" attribute group, for platforms that
>> + * supports counter_info_version 0x8 or above.
>> + */
>> +#undef EVENT_ENABLE
>> +
>> +/* Generate event list for platforms with counter_info_version 0x8 or above*/
>>   static __maybe_unused struct attribute *hv_gpci_event_attrs[] = {
>>   #include REQUEST_FILE
>>   	NULL
>> -- 
>> 2.31.1