[PATCH] kselftest/arm64: abi: fix SVCR detection

[PATCH] kselftest/arm64: abi: fix SVCR detection

[Weizhao Ouyang]

When using svcr_in to check ZA and Streaming Mode, we should make sure
that the value in x2 is correct, otherwise it may trigger an Illegal
instruction if FEAT_SVE and !FEAT_SME.

Fixes: 43e3f85523e4 ("kselftest/arm64: Add SME support to syscall ABI test")
Signed-off-by: Weizhao Ouyang <o451686892@gmail.com>
---
 tools/testing/selftests/arm64/abi/syscall-abi-asm.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/arm64/abi/syscall-abi-asm.S b/tools/testing/selftests/arm64/abi/syscall-abi-asm.S
index df3230fdac39..98cde4f37abf 100644
--- a/tools/testing/selftests/arm64/abi/syscall-abi-asm.S
+++ b/tools/testing/selftests/arm64/abi/syscall-abi-asm.S
@@ -81,9 +81,9 @@ do_syscall:
 	stp	x27, x28, [sp, #96]
 
 	// Set SVCR if we're doing SME
-	cbz	x1, 1f
 	adrp	x2, svcr_in
 	ldr	x2, [x2, :lo12:svcr_in]
+	cbz	x1, 1f
 	msr	S3_3_C4_C2_2, x2
 1:
 
-- 
2.45.2

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Mark Brown]

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

On Mon, Dec 09, 2024 at 06:52:37PM +0800, Weizhao Ouyang wrote:

> When using svcr_in to check ZA and Streaming Mode, we should make sure
> that the value in x2 is correct, otherwise it may trigger an Illegal
> instruction if FEAT_SVE and !FEAT_SME.

>  	// Set SVCR if we're doing SME
> -	cbz	x1, 1f
>  	adrp	x2, svcr_in
>  	ldr	x2, [x2, :lo12:svcr_in]
> +	cbz	x1, 1f
>  	msr	S3_3_C4_C2_2, x2

This is against an older verison of the code so wouldn't apply now.
It's not also checking the value of SVCR, this is checking the SME flag
passed in via x1.  You can see that the SVCR value is loaded into x2 but
the check is against x1.

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

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Weizhao Ouyang]

On Mon, Dec 9, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 06:52:37PM +0800, Weizhao Ouyang wrote:
>
> > When using svcr_in to check ZA and Streaming Mode, we should make sure
> > that the value in x2 is correct, otherwise it may trigger an Illegal
> > instruction if FEAT_SVE and !FEAT_SME.
>
> >       // Set SVCR if we're doing SME
> > -     cbz     x1, 1f
> >       adrp    x2, svcr_in
> >       ldr     x2, [x2, :lo12:svcr_in]
> > +     cbz     x1, 1f
> >       msr     S3_3_C4_C2_2, x2
>
> This is against an older verison of the code so wouldn't apply now.
> It's not also checking the value of SVCR, this is checking the SME flag
> passed in via x1.  You can see that the SVCR value is loaded into x2 but
> the check is against x1.

Hi Mark,

This patch aims to fix the second check (SVCR_ZA_SHIFT) instead of
the first one (the x1 SME flag you're referring to):

// Load ZA and ZT0 if enabled - uses x12 as scratch due to SME LDR
tbz x2, #SVCR_ZA_SHIFT, 1f
mov w12, #0
ldr x2, =za_in
2: _ldr_za 12, 2

If SME disabled, x2 will not have an expected value.

BR,
Weizhao

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Mark Brown]

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

On Mon, Dec 09, 2024 at 08:51:28PM +0800, Weizhao Ouyang wrote:
> On Mon, Dec 9, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:

> > >       // Set SVCR if we're doing SME
> > > -     cbz     x1, 1f
> > >       adrp    x2, svcr_in
> > >       ldr     x2, [x2, :lo12:svcr_in]
> > > +     cbz     x1, 1f
> > >       msr     S3_3_C4_C2_2, x2

> > This is against an older verison of the code so wouldn't apply now.
> > It's not also checking the value of SVCR, this is checking the SME flag
> > the check is against x1.

> This patch aims to fix the second check (SVCR_ZA_SHIFT) instead of
> the first one (the x1 SME flag you're referring to):

If we don't have SME we should be skipping over all the SME code and
never even looking at the value of SVCR.  Looking at the current version
of the code it does that, it branches to check_sve_in if SME is not
enabled.

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

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Weizhao Ouyang]

On Mon, Dec 9, 2024 at 9:12 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 08:51:28PM +0800, Weizhao Ouyang wrote:
> > On Mon, Dec 9, 2024 at 8:36 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > >       // Set SVCR if we're doing SME
> > > > -     cbz     x1, 1f
> > > >       adrp    x2, svcr_in
> > > >       ldr     x2, [x2, :lo12:svcr_in]
> > > > +     cbz     x1, 1f
> > > >       msr     S3_3_C4_C2_2, x2
>
> > > This is against an older verison of the code so wouldn't apply now.
> > > It's not also checking the value of SVCR, this is checking the SME flag
> > > the check is against x1.
>
> > This patch aims to fix the second check (SVCR_ZA_SHIFT) instead of
> > the first one (the x1 SME flag you're referring to):
>
> If we don't have SME we should be skipping over all the SME code and
> never even looking at the value of SVCR.  Looking at the current version
> of the code it does that, it branches to check_sve_in if SME is not
> enabled.

Hi Mark,

Yes we should skip it, this is just a minor tweak based on the
current implementation, after all, we manually passed its value by
svcr_in.

Which latest code version are you referring to? I think check_sve_in
is in fp testcase, not in the abi testcase. (checked the -next tree)

BR,
Weizhao

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Mark Brown]

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

On Mon, Dec 09, 2024 at 09:26:01PM +0800, Weizhao Ouyang wrote:

> > If we don't have SME we should be skipping over all the SME code and
> > never even looking at the value of SVCR.  Looking at the current version
> > of the code it does that, it branches to check_sve_in if SME is not
> > enabled.

> Yes we should skip it, this is just a minor tweak based on the
> current implementation, after all, we manually passed its value by
> svcr_in.

It's a fairly trivial tweak to make...  in any case, it looks like we
also need the same change in the save path.

> Which latest code version are you referring to? I think check_sve_in
> is in fp testcase, not in the abi testcase. (checked the -next tree)

Ah, yes sorry.

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

Re: [PATCH] kselftest/arm64: abi: fix SVCR detection

[Weizhao Ouyang]

On Mon, Dec 9, 2024 at 9:51 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 09:26:01PM +0800, Weizhao Ouyang wrote:
>
> > > If we don't have SME we should be skipping over all the SME code and
> > > never even looking at the value of SVCR.  Looking at the current version
> > > of the code it does that, it branches to check_sve_in if SME is not
> > > enabled.
>
> > Yes we should skip it, this is just a minor tweak based on the
> > current implementation, after all, we manually passed its value by
> > svcr_in.
>
> It's a fairly trivial tweak to make...  in any case, it looks like we
> also need the same change in the save path.

Yeah. I have encountered this issue with both QEMU and FVP. I can
help to add a check_sve_in similar to the fp testcase for the abi
testcase.

BR,
Weizhao

>
> > Which latest code version are you referring to? I think check_sve_in
> > is in fp testcase, not in the abi testcase. (checked the -next tree)
>
> Ah, yes sorry.