[PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Mark Brown]

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
 	}
 };

---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240731-kvm-arm64-fix-s1pie-test-a40b6be58d7b

Best regards,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Joey Gouly]

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>
> 

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Marc Zyngier]

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.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list

[Oliver Upton]

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