arm64: pmuv3: Support v8.1 PMUv3 extension

The PMUVer field can have a value 4 for PMUv3 which supports 16 bit evtCount field (this is documented in ARM Architecture Reference Manual Supplement ARMv8.1).

The current check for PMUVer to be equal to 1 fails on ThunderX2 which has value 4 in PMUVer field. Fix this.

Signed-off-by Jayachandran C <[email protected]>

---

This applies on top of the current arm64 tree and fixes a breakage due to the ACPI perf patches.

 arch/arm64/kernel/perf_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/perf_event.c +0 -0

index 98c7493..5388ed8 100644

Hi,

Quoted 4 lines · Jayachandran C

The PMUVer field can have a value 4 for PMUv3 which supports 16 bit evtCount field (this is documented in ARM Architecture Reference Manual Supplement ARMv8.1).

Quoted 8 lines

The current check for PMUVer to be equal to 1 fails on ThunderX2 which has value 4 in PMUVer field. Fix this.

Signed-off-by Jayachandran C <[email protected]>

---

This applies on top of the current arm64 tree and fixes a breakage due to the ACPI perf patches.

Sorry for the delay on this. I'm still awaiting an architectural clarification on how this field should be interpreted, as per my prior comments [1].

I realise that's not much consolation here, so I'm happy to take an intermediate fix. One comment on that below.

Quoted 8 lines

 arch/arm64/kernel/perf_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kernel/perf_event.c +0 -0

index 98c7493..5388ed8 100644

arch/arm64/kernel/perf_event.c static void __armv8pmu_probe_pmu(void *info) · line 976

Quoted 6 lines

arch/arm64/kernel/perf_event.c +1 -1

@@ -972,7 +972,7 @@ static void __armv8pmu_probe_pmu(void *info)
      dfr0 = read_sysreg(id_aa64dfr0_el1);
      pmuver = cpuid_feature_extract_unsigned_field(dfr0,
                      ID_AA64DFR0_PMUVER_SHIFT);
-     if (pmuver != 1)
+     if (pmuver != 1 && pmuver != 4)
              return;

Can we please make this:

        pmuver = cpuid_feature_extract_signed_field(dfr0,
                        ID_AA64DFR0_PMUVER_SHIFT);
        if (pmuver < 1)
                return;

With that, I'm happy to take this while we await further clarification.

The v8.1 supplement is quite clear on the field definition:

PMUVer, bits [11:8]
....
Defined values are:
0000 Performance Monitors extension System registers not implemented. 0001 Performance Monitors extension System registers implemented, PMUv3. 0100 Performance Monitors extension System registers implemented, PMUv3, with a 16-bit
evtCount field, and if EL2 is implemented, the addition of the MDCR_EL2.HPMD bit.
1111 IMPLEMENTATION DEFINED form of performance monitors supported, PMUv3 not
supported.
All other values are reserved.
In ARMv8-A the permitted values are 0b0000, 0b0001 and 0b1111.
In ARMv8.1 the permitted values are 0b0000, 0b0100 and 0b1111.

I changed the code to strictly do this. We have to exclude 0xf, since that is not PMUv3. And we cannot predict what the reserved values will represent, so it is best to skip them until they are defined to be PMUv3 compatible.

Quoted 15 lines · Jayachandran C

The v8.1 supplement is quite clear on the field definition:

PMUVer, bits [11:8]
....
Defined values are:
0000 Performance Monitors extension System registers not implemented. 0001 Performance Monitors extension System registers implemented, PMUv3. 0100 Performance Monitors extension System registers implemented, PMUv3, with a 16-bit
evtCount field, and if EL2 is implemented, the addition of the MDCR_EL2.HPMD bit.
1111 IMPLEMENTATION DEFINED form of performance monitors supported, PMUv3 not
supported.
All other values are reserved.
In ARMv8-A the permitted values are 0b0000, 0b0001 and 0b1111.
In ARMv8.1 the permitted values are 0b0000, 0b0100 and 0b1111.

Quoted 3 lines

I changed the code to strictly do this. We have to exclude 0xf, since that is not PMUv3. And we cannot predict what the reserved values will represent, so it is best to skip them until they are defined to be PMUv3 compatible.

My understanding is that ID_AA64DFR0.PMUVer is intended to be covered by the usual ID register principles, and thus at least 0x2-0x7 are reserved for architected backwards compatible extensions to PMUv3.

See ARM DDI 0487B.a, D7.1.4, "Principles of the ID scheme for fields in ID registers". It is explicitly stated that the scheme applies to ID_AA64DFR0.

Per those rules, we should check >= the minimum PMUv3 implemented value, i.e. val >= 1. Due to both 0x0 and 0xF meaning PMUv3 isn't implemented, it's not clear if the fields should be treated as if it were signed or unsigned, and I'm awaiting clarification on this.

Either way, I believe that 0x1-0x7 must all be compatible with baseline PMUv3 per the ID scheme principles.

Quoted 24 lines · Mark Rutland

Quoted 15 lines · Jayachandran C

The v8.1 supplement is quite clear on the field definition:

PMUVer, bits [11:8]
....
Defined values are:
0000 Performance Monitors extension System registers not implemented. 0001 Performance Monitors extension System registers implemented, PMUv3. 0100 Performance Monitors extension System registers implemented, PMUv3, with a 16-bit
evtCount field, and if EL2 is implemented, the addition of the MDCR_EL2.HPMD bit.
1111 IMPLEMENTATION DEFINED form of performance monitors supported, PMUv3 not
supported.
All other values are reserved.
In ARMv8-A the permitted values are 0b0000, 0b0001 and 0b1111.
In ARMv8.1 the permitted values are 0b0000, 0b0100 and 0b1111.

Quoted 3 lines

I changed the code to strictly do this. We have to exclude 0xf, since that is not PMUv3. And we cannot predict what the reserved values will represent, so it is best to skip them until they are defined to be PMUv3 compatible.

My understanding is that ID_AA64DFR0.PMUVer is intended to be covered by the usual ID register principles, and thus at least 0x2-0x7 are reserved for architected backwards compatible extensions to PMUv3.

Quoted 8 lines

See ARM DDI 0487B.a, D7.1.4, "Principles of the ID scheme for fields in ID registers". It is explicitly stated that the scheme applies to ID_AA64DFR0.

Per those rules, we should check >= the minimum PMUv3 implemented value, i.e. val >= 1. Due to both 0x0 and 0xF meaning PMUv3 isn't implemented, it's not clear if the fields should be treated as if it were signed or unsigned, and I'm awaiting clarification on this.

Thanks for the reference. Since 0xf means that there is a PMU (but it is not PMUv3), this looks like an unsigned ID with a special case.

Quoted 2 lines

Either way, I believe that 0x1-0x7 must all be compatible with baseline PMUv3 per the ID scheme principles.

In case you don't get authoritative information on this, can you please merge a version which does either 'if (pmuver < 1 || pmuver == 0xf)' or 'if (pmuver < 1 || pmuver > 7)' to the patchset?

Quoted 37 lines · Jayachandran C

Quoted 24 lines · Mark Rutland

Quoted 15 lines · Jayachandran C

The v8.1 supplement is quite clear on the field definition:

PMUVer, bits [11:8]
....
Defined values are:
0000 Performance Monitors extension System registers not implemented. 0001 Performance Monitors extension System registers implemented, PMUv3. 0100 Performance Monitors extension System registers implemented, PMUv3, with a 16-bit
evtCount field, and if EL2 is implemented, the addition of the MDCR_EL2.HPMD bit.
1111 IMPLEMENTATION DEFINED form of performance monitors supported, PMUv3 not
supported.
All other values are reserved.
In ARMv8-A the permitted values are 0b0000, 0b0001 and 0b1111.
In ARMv8.1 the permitted values are 0b0000, 0b0100 and 0b1111.

Quoted 3 lines

I changed the code to strictly do this. We have to exclude 0xf, since that is not PMUv3. And we cannot predict what the reserved values will represent, so it is best to skip them until they are defined to be PMUv3 compatible.

My understanding is that ID_AA64DFR0.PMUVer is intended to be covered by the usual ID register principles, and thus at least 0x2-0x7 are reserved for architected backwards compatible extensions to PMUv3.

Quoted 8 lines

See ARM DDI 0487B.a, D7.1.4, "Principles of the ID scheme for fields in ID registers". It is explicitly stated that the scheme applies to ID_AA64DFR0.

Per those rules, we should check >= the minimum PMUv3 implemented value, i.e. val >= 1. Due to both 0x0 and 0xF meaning PMUv3 isn't implemented, it's not clear if the fields should be treated as if it were signed or unsigned, and I'm awaiting clarification on this.

Thanks for the reference. Since 0xf means that there is a PMU (but it is not PMUv3), this looks like an unsigned ID with a special case.

Quoted 6 lines

Quoted 2 lines

Either way, I believe that 0x1-0x7 must all be compatible with baseline PMUv3 per the ID scheme principles.

In case you don't get authoritative information on this, can you please merge a version which does either 'if (pmuver < 1 || pmuver == 0xf)' or 'if (pmuver < 1 || pmuver > 7)' to the patchset?

Sure, that's what I proposed:

        pmuver = cpuid_feature_extract_signed_field(dfr0,
                        ID_AA64DFR0_PMUVER_SHIFT);
        if (pmuver < 1)
                return;

Note that it treats the value as signed, so it will accept 0x1-0x7, and return for 0x0 or 0x8-0xf.