| Message ID | 20241209105237.10498-1-o451686892@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kselftest/arm64: abi: fix SVCR detection | expand |
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.
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
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.
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
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.
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.
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:
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(-)