* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension @ 2017-04-24 11:31 Jayachandran C 2017-04-24 12:57 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: Jayachandran C @ 2017-04-24 11:31 UTC (permalink / raw) To: linux-arm-kernel 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(-) 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; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 11:31 [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension Jayachandran C @ 2017-04-24 12:57 ` Mark Rutland 2017-04-24 13:39 ` Jayachandran C 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2017-04-24 12:57 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 12:57 ` Mark Rutland @ 2017-04-24 13:39 ` Jayachandran C 2017-04-24 14:03 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: Jayachandran C @ 2017-04-24 13:39 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 13:39 ` Jayachandran C @ 2017-04-24 14:03 ` Mark Rutland 2017-04-24 16:40 ` Jayachandran C 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2017-04-24 14:03 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 14:03 ` Mark Rutland @ 2017-04-24 16:40 ` Jayachandran C 2017-04-24 16:45 ` Mark Rutland 0 siblings, 1 reply; 7+ messages in thread From: Jayachandran C @ 2017-04-24 16:40 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 16:40 ` Jayachandran C @ 2017-04-24 16:45 ` Mark Rutland 2017-04-24 17:45 ` Jayachandran C 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2017-04-24 16:45 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension 2017-04-24 16:45 ` Mark Rutland @ 2017-04-24 17:45 ` Jayachandran C 0 siblings, 0 replies; 7+ messages in thread From: Jayachandran C @ 2017-04-24 17:45 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-24 17:45 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-24 11:31 [PATCH] arm64: pmuv3: Support v8.1 PMUv3 extension Jayachandran C 2017-04-24 12:57 ` Mark Rutland 2017-04-24 13:39 ` Jayachandran C 2017-04-24 14:03 ` Mark Rutland 2017-04-24 16:40 ` Jayachandran C 2017-04-24 16:45 ` Mark Rutland 2017-04-24 17:45 ` Jayachandran C
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).