Re: [PATCH v2] arm64/arm: perf: Don't discard upper 32 bits from PMCEID0/1 registers

[Ilkka Koskinen <ilkka@os.amperecomputing.com>]

Hi Mark,

On Fri, 27 Oct 2023, Mark Rutland wrote:
> On Thu, Oct 26, 2023 at 06:22:43PM -0700, Ilkka Koskinen wrote:
>> The upper 32 bits of PMCEID[n] registers are used to describe whether
>> architectural and microarchitectural events in range 0x4000-0x401f
>> exist. Due to discarding the bits, the driver made the events invisible,
>> even if they existed.
>>
>> Fixes: df29ddf4f04b ("arm64: perf: Abstract system register accesses away")
>
> Hmm... are we silently truncating anything else?
>
> I see that read_pmxevcntr() and write_pmxevcntr() treat PMXEVCNTR_EL0 as 32-bit
> despite it being 64-bit with PMUv3.5, but it doesn't matter as those functions
> are never used, and should just be deleted.
>
> Similarly PMCR_EL0 is 64-bit, with bit 32 bit FZS when FEAT_SPEv1p2 is
> implemented. That doesn't matter to us right now as we don't use FZS, but it's
> something that could catch us out in future if new bitss get added.
>
> Could we please go check all the accessors added in df29ddf4f04b, and fix those
> in one go?

I quickly look at the registers and unless I'm mistaken there's just one 
more register, PMCCFILTR_EL0, that needs to be taken care. It seems to 
have VS bits [57:56], if FEAT_PMUv3_SME is implemented.

I'll try to submit a new patch that handles all of them tomorrow.

Cheers, Ilkka

>
>> Reported-by: Carl Worth <carl@os.amperecomputing.com>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> ---
>>
>> Changes since v1:
>>
>>   * Took arm32 specific code from Marc Zyngier's review comment
>>   * Fixed a couple of typos in the commit message
>>
>> I have tested the patch on Arm64. However, what comes to Arm32 part, I have
>> only compared the code with Arm32 specification and cross compiled it.
>>
>> v1: https://lore.kernel.org/all/20231025200815.104017-1-ilkka@os.amperecomputing.com/
>>
>> ---
>>
>>  arch/arm/include/asm/arm_pmuv3.h   | 33 +++++++++++++++++++++---------
>>  arch/arm64/include/asm/arm_pmuv3.h |  4 ++--
>>  2 files changed, 25 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arm_pmuv3.h b/arch/arm/include/asm/arm_pmuv3.h
>> index 72529f5e2bed..90841cb7ce43 100644
>> --- a/arch/arm/include/asm/arm_pmuv3.h
>> +++ b/arch/arm/include/asm/arm_pmuv3.h
>> @@ -23,6 +23,8 @@
>>  #define PMUSERENR		__ACCESS_CP15(c9,  0, c14, 0)
>>  #define PMINTENSET		__ACCESS_CP15(c9,  0, c14, 1)
>>  #define PMINTENCLR		__ACCESS_CP15(c9,  0, c14, 2)
>> +#define PMCEID2			__ACCESS_CP15(c9,  0, c14, 4)
>> +#define PMCEID3			__ACCESS_CP15(c9,  0, c14, 5)
>>  #define PMMIR			__ACCESS_CP15(c9,  0, c14, 6)
>>  #define PMCCFILTR		__ACCESS_CP15(c14, 0, c15, 7)
>>
>> @@ -205,16 +207,6 @@ static inline void write_pmuserenr(u32 val)
>>  	write_sysreg(val, PMUSERENR);
>>  }
>>
>> -static inline u32 read_pmceid0(void)
>> -{
>> -	return read_sysreg(PMCEID0);
>> -}
>> -
>> -static inline u32 read_pmceid1(void)
>> -{
>> -	return read_sysreg(PMCEID1);
>> -}
>> -
>>  static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
>>  static inline void kvm_clr_pmu_events(u32 clr) {}
>>  static inline bool kvm_pmu_counter_deferred(struct perf_event_attr *attr)
>> @@ -231,6 +223,7 @@ static inline void kvm_vcpu_pmu_resync_el0(void) {}
>>
>>  /* PMU Version in DFR Register */
>>  #define ARMV8_PMU_DFR_VER_NI        0
>> +#define ARMV8_PMU_DFR_VER_V3P1      0x4
>>  #define ARMV8_PMU_DFR_VER_V3P4      0x5
>>  #define ARMV8_PMU_DFR_VER_V3P5      0x6
>>  #define ARMV8_PMU_DFR_VER_IMP_DEF   0xF
>> @@ -251,4 +244,24 @@ static inline bool is_pmuv3p5(int pmuver)
>>  	return pmuver >= ARMV8_PMU_DFR_VER_V3P5;
>>  }
>>
>> +static inline u64 read_pmceid0(void)
>> +{
>> +	u64 val = read_sysreg(PMCEID0);
>> +
>> +	if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1)
>> +		val |= (u64)read_sysreg(PMCEID2) << 32;
>> +
>> +	return val;
>> +}
>> +
>> +static inline u64 read_pmceid1(void)
>> +{
>> +	u64 val = read_sysreg(PMCEID1);
>> +
>> +	if (read_pmuver() >= ARMV8_PMU_DFR_VER_V3P1)
>> +		val |= (u64)read_sysreg(PMCEID3) << 32;
>> +
>> +	return val;
>> +}
>> +
>>  #endif
>> diff --git a/arch/arm64/include/asm/arm_pmuv3.h b/arch/arm64/include/asm/arm_pmuv3.h
>> index 18dc2fb3d7b7..3e92b7cb57a4 100644
>> --- a/arch/arm64/include/asm/arm_pmuv3.h
>> +++ b/arch/arm64/include/asm/arm_pmuv3.h
>> @@ -126,12 +126,12 @@ static inline void write_pmuserenr(u32 val)
>>  	write_sysreg(val, pmuserenr_el0);
>>  }
>>
>> -static inline u32 read_pmceid0(void)
>> +static inline u64 read_pmceid0(void)
>>  {
>>  	return read_sysreg(pmceid0_el0);
>>  }
>>
>> -static inline u32 read_pmceid1(void)
>> +static inline u64 read_pmceid1(void)
>>  {
>>  	return read_sysreg(pmceid1_el0);
>>  }
>> --
>> 2.41.0
>>
>