| Message ID | 1493033503-4712-1-git-send-email-jnair@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Apr 24, 2017 at 11:31:43AM +0000, Jayachandran C wrote: > 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 <jnair@caviumnetworks.com> > --- > > 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. > arch/arm64/kernel/perf_event.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index 98c7493..5388ed8 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -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. Thanks, Mark. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/501305.html IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, Apr 24, 2017 at 01:57:47PM +0100, Mark Rutland wrote: > Hi, > > On Mon, Apr 24, 2017 at 11:31:43AM +0000, Jayachandran C wrote: > > 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 <jnair@caviumnetworks.com> > > --- > > > > 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. I thought I would sent this out since it was getting very late in the cycle. I don't mind waiting if you are planning to fix this, feel free to drop this patch and integrate your version. 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. > > arch/arm64/kernel/perf_event.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > > index 98c7493..5388ed8 100644 > > --- a/arch/arm64/kernel/perf_event.c > > +++ b/arch/arm64/kernel/perf_event.c > > @@ -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. Please see above, I don't really think this is better. JC.
On Mon, Apr 24, 2017 at 01:39:30PM +0000, Jayachandran C wrote: > 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. 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. Thanks, Mark. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
On Mon, Apr 24, 2017 at 03:03:48PM +0100, Mark Rutland wrote: > On Mon, Apr 24, 2017 at 01:39:30PM +0000, Jayachandran C wrote: > > 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. > > 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. 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. > 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? Thanks, JC.
On Mon, Apr 24, 2017 at 04:40:09PM +0000, Jayachandran C wrote: > On Mon, Apr 24, 2017 at 03:03:48PM +0100, Mark Rutland wrote: > > On Mon, Apr 24, 2017 at 01:39:30PM +0000, Jayachandran C wrote: > > > 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. > > > > 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. > > 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. > > > 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. Thanks, Mark.
On Mon, Apr 24, 2017 at 05:45:08PM +0100, Mark Rutland wrote: > On Mon, Apr 24, 2017 at 04:40:09PM +0000, Jayachandran C wrote: > > On Mon, Apr 24, 2017 at 03:03:48PM +0100, Mark Rutland wrote: > > > On Mon, Apr 24, 2017 at 01:39:30PM +0000, Jayachandran C wrote: > > > > 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. > > > > > > 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. > > > > 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. > > > > > 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. Ok, we will need to change the type of pmuver to int as well. But even then, I think the reasonable interpretation of the current spec is that the field is unsigned. Your original 'if (pmuver < 1 || pmuver == 0xf)' is the least surprising way of writing it. JC.
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c index 98c7493..5388ed8 100644 --- a/arch/arm64/kernel/perf_event.c +++ b/arch/arm64/kernel/perf_event.c @@ -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; probe->present = true;
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 <jnair@caviumnetworks.com> --- 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(-)