| Message ID | 20240731-kvm-arm64-fix-s1pie-test-v1-1-a9253f3b7db4@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ad518452fd263766946346324810f14bd8bb8b34 |
| Headers | show |
| Series | KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list | expand |
On Wed, Jul 31, 2024 at 05:21:13PM +0100, Mark Brown wrote: > The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but > get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 > instead. > > Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") > Signed-off-by: Mark Brown <broonie@kernel.org> Argh! Thanks for spotting that. Reviewed-by: Joey Gouly <joey.gouly@arm.com> > --- > tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c > index 709d7d721760..4abebde78187 100644 > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c > @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { > { > ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ > ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ > - 4, > + 8, > 1 > }, > { > ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ > ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ > - 4, > + 8, > 1 > } > }; > > --- > base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b > change-id: 20240731-kvm-arm64-fix-s1pie-test-a40b6be58d7b > > Best regards, > -- > Mark Brown <broonie@kernel.org> >
On Wed, 31 Jul 2024 17:21:13 +0100, Mark Brown <broonie@kernel.org> wrote: > > The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but > get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 > instead. > > Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c > index 709d7d721760..4abebde78187 100644 > --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c > +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c > @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { > { > ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ > ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ > - 4, > + 8, > 1 > }, > { > ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ > ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ > - 4, > + 8, > 1 > } > }; Thanks for spotting this. However, we are fixing it in a very backward way. Can we please switch all this stuff to symbolic naming instead of magic numbers? Given how much effort is going into the "automated generation" thing, it is mind-boggling that the tests still rely on handcrafted numbers. We just end-up with two different sets of bugs. At the moment, the level of confidence I have in this stuff is sub-zero. M.
On Thu, Aug 01, 2024 at 05:45:49PM +0100, Marc Zyngier wrote: > Can we please switch all this stuff to symbolic naming instead of > magic numbers? Given how much effort is going into the "automated > generation" thing, it is mind-boggling that the tests still rely on > handcrafted numbers. We just end-up with two different sets of bugs. > At the moment, the level of confidence I have in this stuff is > sub-zero. Yeah, I was wondering why this wasn't using the generated values especially given that the generated headers are available to tools - I wasn't sure if this was a deliberate decision to cross check the data entry or something. I'd certainly be happy to convert, though that does seem a bit invasive for a fix.
On Thu, 01 Aug 2024 20:14:38 +0100, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Thu, Aug 01, 2024 at 05:45:49PM +0100, Marc Zyngier wrote: > > > Can we please switch all this stuff to symbolic naming instead of > > magic numbers? Given how much effort is going into the "automated > > generation" thing, it is mind-boggling that the tests still rely on > > handcrafted numbers. We just end-up with two different sets of bugs. > > > At the moment, the level of confidence I have in this stuff is > > sub-zero. > > Yeah, I was wondering why this wasn't using the generated values > especially given that the generated headers are available to tools - I > wasn't sure if this was a deliberate decision to cross check the data > entry or something. We've lost that battle a long time ago, given the numbers of bugs the sysreg file has had. The real reason is that the ABI reports the encoding, and that it is rather easy to just dump stuff back into the test using the script described in the very first commit for the test. Also, the test predates the generated stuff by some margin. > I'd certainly be happy to convert, though that does > seem a bit invasive for a fix. Not for a point fix, for sure. And if you do, make sure it is entirely scripted. Thanks, M.
On Fri, Aug 02, 2024 at 10:00:28AM +0100, Marc Zyngier wrote: > Also, the test predates the generated stuff by some margin. Yeah, there were still defines in the main kernel source that were being retyped rather than shared previously which made me wonder. > Mark Brown <broonie@kernel.org> wrote: > > I'd certainly be happy to convert, though that does > > seem a bit invasive for a fix. > Not for a point fix, for sure. And if you do, make sure it is entirely > scripted. When you say "entirely scripted" here I take it you're referring to the list of registers as well, and I guess also to the information about what is enumerated by which ID register values? I'd already been thinking about looking at the latter bit, and possibly also tracking wiring things up to traps (though that's only relevant inside the kernel). I agree that seems sensible, but I do think we can usefully do things in stages - even just replacing the magic numbers with use of the defines would be less error prone. It would be great if we just automatically covered every sysreg we know about in this test without any manual steps.
On Fri, 02 Aug 2024 13:43:03 +0100, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Fri, Aug 02, 2024 at 10:00:28AM +0100, Marc Zyngier wrote: > > > Also, the test predates the generated stuff by some margin. > > Yeah, there were still defines in the main kernel source that were being > retyped rather than shared previously which made me wonder. Definitions in the kernel are likely to exist for a long time though, as the tool is still pretty primitive and doesn't handle anything that changes layout (such as any register affected by E2H). > > > Mark Brown <broonie@kernel.org> wrote: > > > > I'd certainly be happy to convert, though that does > > > seem a bit invasive for a fix. > > > Not for a point fix, for sure. And if you do, make sure it is entirely > > scripted. > > When you say "entirely scripted" here I take it you're referring to the > list of registers as well, and I guess also to the information about > what is enumerated by which ID register values? The register list is indeed the #1 offender, and that should just be a script that goes over all the occurrences of ARM64_SYS_REG() and replace the encoding with something that uses the symbolic name. For the rest (shifts and stuff), we can probably do that by hand (there are only a few occurrences). M.
On Fri, Aug 02, 2024 at 02:36:14PM +0100, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > > Not for a point fix, for sure. And if you do, make sure it is entirely > > > scripted. > > When you say "entirely scripted" here I take it you're referring to the > > list of registers as well, and I guess also to the information about > > what is enumerated by which ID register values? > The register list is indeed the #1 offender, and that should just be a > script that goes over all the occurrences of ARM64_SYS_REG() and > replace the encoding with something that uses the symbolic name. Oh, I see - the scripting of an in place update. It sounded like you wanted the tables generating at build time (which I do think should be the eventual goal).
On Wed, 31 Jul 2024 17:21:13 +0100, Mark Brown wrote: > The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but > get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 > instead. > > Applied to kvmarm/fixes, thanks! [1/1] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list https://git.kernel.org/kvmarm/kvmarm/c/ad518452fd26 -- Best, Oliver
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..4abebde78187 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { { ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, + 8, 1 }, { ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, + 8, 1 } };
The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 instead. Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240731-kvm-arm64-fix-s1pie-test-a40b6be58d7b Best regards,