[PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Mark Brown]

Currently our SVE syscall ABI documentation does not reflect the actual
implemented ABI, it says that register state not shared with FPSIMD
becomes undefined on syscall when in reality we always clear it. Since
changing this would cause a change in the observed kernel behaviour
there is a substantial desire to avoid taking advantage of the
documented ABI so instead let's document what we actually do so it's
clear that it is in reality an ABI.

There has been some pushback on tightening the documentation in the past
but it is hard to see who that helps, it makes the implementation
decisions less clear and makes it harder for people to discover and make
use of the actual ABI. The main practical concern is that qemu's user
mode does not currently flush the registers.

v3:
 - Rebase onto v6.0-rc3.
v2:
 - Rebase onto v6.0-rc1.

Mark Brown (3):
  kselftest/arm64: Correct buffer allocation for SVE Z registers
  arm64/sve: Document our actual ABI for clearing registers on syscall
  kselftest/arm64: Enforce actual ABI for SVE syscalls

 Documentation/arm64/sve.rst                   |  2 +-
 .../testing/selftests/arm64/abi/syscall-abi.c | 61 ++++++++++++-------
 2 files changed, 41 insertions(+), 22 deletions(-)


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.30.2

[PATCH v3 1/3] kselftest/arm64: Correct buffer allocation for SVE Z registers

[Mark Brown]

The buffer used for verifying SVE Z registers allocated enough space for
16 maximally sized registers rather than 32 due to using the macro for the
number of P registers. In practice this didn't matter since for historical
reasons the maximum VQ defined in the ABI is greater the architectural
maximum so we will always allocate more space than is needed even with
emulated platforms implementing the architectural maximum. Still, we should
use the right define.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 tools/testing/selftests/arm64/abi/syscall-abi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/abi/syscall-abi.c b/tools/testing/selftests/arm64/abi/syscall-abi.c
index b632bfe9e022..95229fa73232 100644
--- a/tools/testing/selftests/arm64/abi/syscall-abi.c
+++ b/tools/testing/selftests/arm64/abi/syscall-abi.c
@@ -113,8 +113,8 @@ static int check_fpr(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 }
 
 static uint8_t z_zero[__SVE_ZREG_SIZE(SVE_VQ_MAX)];
-uint8_t z_in[SVE_NUM_PREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
-uint8_t z_out[SVE_NUM_PREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
+uint8_t z_in[SVE_NUM_ZREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
+uint8_t z_out[SVE_NUM_ZREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
 
 static void setup_z(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 		    uint64_t svcr)
-- 
2.30.2

[PATCH v3 2/3] arm64/sve: Document our actual ABI for clearing registers on syscall

[Mark Brown]

Currently our ABI documentation says that the state of the bits in the Z
registers not shared with the V registers becomes undefined on syscall but
our actual implementation unconditionally clears these bits. Taking
advantage of the flexibility of our documented ABI would be a change in the
observable ABI so there is concern around doing so, instead document the
actual behaviour so that it is more discoverable for userspace programmers
who might be able to take advantage of it and to record our decision about
not changing the kernel ABI.

This makes qemu's user mode implementation buggy since it does not clear
these bits.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 Documentation/arm64/sve.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/arm64/sve.rst b/Documentation/arm64/sve.rst
index 93c2c2990584..e39acf95d157 100644
--- a/Documentation/arm64/sve.rst
+++ b/Documentation/arm64/sve.rst
@@ -111,7 +111,7 @@ the SVE instruction set architecture.
 
 * On syscall, V0..V31 are preserved (as without SVE).  Thus, bits [127:0] of
   Z0..Z31 are preserved.  All other bits of Z0..Z31, and all of P0..P15 and FFR
-  become unspecified on return from a syscall.
+  become zero on return from a syscall.
 
 * The SVE registers are not used to pass arguments to or receive results from
   any syscall.
-- 
2.30.2

[PATCH v3 3/3] kselftest/arm64: Enforce actual ABI for SVE syscalls

[Mark Brown]

Currently syscall-abi permits the bits in Z registers not shared with the
V registers as well as all of the predicate registers to be preserved on
syscall but the actual implementation has always cleared them and our
documentation has now been updated to make that the documented ABI so
update the syscall-abi test to match.

Signed-off-by: Mark Brown <broonie@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 .../testing/selftests/arm64/abi/syscall-abi.c | 57 ++++++++++++-------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/arm64/abi/syscall-abi.c b/tools/testing/selftests/arm64/abi/syscall-abi.c
index 95229fa73232..dd7ebe536d05 100644
--- a/tools/testing/selftests/arm64/abi/syscall-abi.c
+++ b/tools/testing/selftests/arm64/abi/syscall-abi.c
@@ -112,6 +112,8 @@ static int check_fpr(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 	return errors;
 }
 
+#define SVE_Z_SHARED_BYTES (128 / 8)
+
 static uint8_t z_zero[__SVE_ZREG_SIZE(SVE_VQ_MAX)];
 uint8_t z_in[SVE_NUM_ZREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
 uint8_t z_out[SVE_NUM_ZREGS * __SVE_ZREG_SIZE(SVE_VQ_MAX)];
@@ -133,22 +135,39 @@ static int check_z(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 	if (!sve_vl)
 		return 0;
 
-	/*
-	 * After a syscall the low 128 bits of the Z registers should
-	 * be preserved and the rest be zeroed or preserved, except if
-	 * we were in streaming mode in which case the low 128 bits may
-	 * also be cleared by the transition out of streaming mode.
-	 */
 	for (i = 0; i < SVE_NUM_ZREGS; i++) {
-		void *in = &z_in[reg_size * i];
-		void *out = &z_out[reg_size * i];
-
-		if ((memcmp(in, out, SVE_VQ_BYTES) != 0) &&
-		    !((svcr & SVCR_SM_MASK) &&
-		      memcmp(z_zero, out, SVE_VQ_BYTES) == 0)) {
-			ksft_print_msg("%s SVE VL %d Z%d low 128 bits changed\n",
-				       cfg->name, sve_vl, i);
-			errors++;
+		uint8_t *in = &z_in[reg_size * i];
+		uint8_t *out = &z_out[reg_size * i];
+
+		if (svcr & SVCR_SM_MASK) {
+			/*
+			 * In streaming mode the whole register should
+			 * be cleared by the transition out of
+			 * streaming mode.
+			 */
+			if (memcmp(z_zero, out, reg_size) != 0) {
+				ksft_print_msg("%s SVE VL %d Z%d non-zero\n",
+					       cfg->name, sve_vl, i);
+				errors++;
+			}
+		} else {
+			/*
+			 * For standard SVE the low 128 bits should be
+			 * preserved and any additional bits cleared.
+			 */
+			if (memcmp(in, out, SVE_Z_SHARED_BYTES) != 0) {
+				ksft_print_msg("%s SVE VL %d Z%d low 128 bits changed\n",
+					       cfg->name, sve_vl, i);
+				errors++;
+			}
+
+			if (reg_size > SVE_Z_SHARED_BYTES &&
+			    (memcmp(z_zero, out + SVE_Z_SHARED_BYTES,
+				    reg_size - SVE_Z_SHARED_BYTES) != 0)) {
+				ksft_print_msg("%s SVE VL %d Z%d high bits non-zero\n",
+					       cfg->name, sve_vl, i);
+				errors++;
+			}
 		}
 	}
 
@@ -176,9 +195,9 @@ static int check_p(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 	if (!sve_vl)
 		return 0;
 
-	/* After a syscall the P registers should be preserved or zeroed */
+	/* After a syscall the P registers should be zeroed */
 	for (i = 0; i < SVE_NUM_PREGS * reg_size; i++)
-		if (p_out[i] && (p_in[i] != p_out[i]))
+		if (p_out[i])
 			errors++;
 	if (errors)
 		ksft_print_msg("%s SVE VL %d predicate registers non-zero\n",
@@ -226,9 +245,9 @@ static int check_ffr(struct syscall_cfg *cfg, int sve_vl, int sme_vl,
 	    !(getauxval(AT_HWCAP2) & HWCAP2_SME_FA64))
 		return 0;
 
-	/* After a syscall the P registers should be preserved or zeroed */
+	/* After a syscall FFR should be zeroed */
 	for (i = 0; i < reg_size; i++)
-		if (ffr_out[i] && (ffr_in[i] != ffr_out[i]))
+		if (ffr_out[i])
 			errors++;
 	if (errors)
 		ksft_print_msg("%s SVE VL %d FFR non-zero\n",
-- 
2.30.2

Re: [PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Catalin Marinas]

On Mon, Aug 29, 2022 at 05:24:59PM +0100, Mark Brown wrote:
> v3:
>  - Rebase onto v6.0-rc3.
> v2:
>  - Rebase onto v6.0-rc1.

Please don't rebase beyond -rc1 unless it no longer applies cleanly. I
came back from holiday and I have two or three versions of all of your
patches in my inbox. It just adds to the clutter.

Thanks.

-- 
Catalin

Re: [PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Mark Brown]

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

On Fri, Sep 02, 2022 at 07:22:20PM +0100, Catalin Marinas wrote:
> On Mon, Aug 29, 2022 at 05:24:59PM +0100, Mark Brown wrote:

> > v3:
> >  - Rebase onto v6.0-rc3.
> > v2:
> >  - Rebase onto v6.0-rc1.

> Please don't rebase beyond -rc1 unless it no longer applies cleanly. I
> came back from holiday and I have two or three versions of all of your
> patches in my inbox. It just adds to the clutter.

OK, sure.  It might help to advertise what you're looking for here - the
whole thing with wanting everything based off -rc3 has never been clear
to me, IIRC I figured it out from some off hand comment rather than
actually knowing what you and Will want.  It's neither base off latest
nor base off -rc1 which seem to be the more common policies.

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

Re: [PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Catalin Marinas]

On Fri, Sep 02, 2022 at 07:57:15PM +0100, Mark Brown wrote:
> On Fri, Sep 02, 2022 at 07:22:20PM +0100, Catalin Marinas wrote:
> > On Mon, Aug 29, 2022 at 05:24:59PM +0100, Mark Brown wrote:
> > > v3:
> > >  - Rebase onto v6.0-rc3.
> > > v2:
> > >  - Rebase onto v6.0-rc1.
> 
> > Please don't rebase beyond -rc1 unless it no longer applies cleanly. I
> > came back from holiday and I have two or three versions of all of your
> > patches in my inbox. It just adds to the clutter.
> 
> OK, sure.  It might help to advertise what you're looking for here - the
> whole thing with wanting everything based off -rc3 has never been clear
> to me, IIRC I figured it out from some off hand comment rather than
> actually knowing what you and Will want.  It's neither base off latest
> nor base off -rc1 which seem to be the more common policies.

Usually basing off -rc1 is sufficient unless the patches conflict with
something newer and we occasionally push for-next/core to even higher
-rcX. Of course, fixes for something in a late -rc should be based off
that version.

Both Will and I start queuing patches around -rc3 with the two weeks
before pretty much reviewing or waiting to see if there are any more
comments. I don't mind you basing your patches off -rc3 though in
general it's better for series aimed at the upcoming merging window to
be posted shortly after -rc1 to give them some more time on the list.
However, once posted, if there are no changes please don't repost them
for subsequent -rcX, it won't make any difference as we can just apply
the original series to whatever we base for-next/core off.

Thanks.

-- 
Catalin

Re: (subset) [PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Catalin Marinas]

On Mon, 29 Aug 2022 17:24:59 +0100, Mark Brown wrote:
> Currently our SVE syscall ABI documentation does not reflect the actual
> implemented ABI, it says that register state not shared with FPSIMD
> becomes undefined on syscall when in reality we always clear it. Since
> changing this would cause a change in the observed kernel behaviour
> there is a substantial desire to avoid taking advantage of the
> documented ABI so instead let's document what we actually do so it's
> clear that it is in reality an ABI.
> 
> [...]

Applied to arm64 (for-next/kselftest), thanks!

[1/3] kselftest/arm64: Correct buffer allocation for SVE Z registers
      https://git.kernel.org/arm64/c/27f3d9e70fd8
[3/3] kselftest/arm64: Enforce actual ABI for SVE syscalls
      https://git.kernel.org/arm64/c/9ccff5080758

-- 
Catalin

Re: (subset) [PATCH v3 0/3] arm64/sve: Document our actual SVE syscall ABI

[Catalin Marinas]

On Mon, 29 Aug 2022 17:24:59 +0100, Mark Brown wrote:
> Currently our SVE syscall ABI documentation does not reflect the actual
> implemented ABI, it says that register state not shared with FPSIMD
> becomes undefined on syscall when in reality we always clear it. Since
> changing this would cause a change in the observed kernel behaviour
> there is a substantial desire to avoid taking advantage of the
> documented ABI so instead let's document what we actually do so it's
> clear that it is in reality an ABI.
> 
> [...]

Applied to arm64 (for-next/doc), thanks!

[2/3] arm64/sve: Document our actual ABI for clearing registers on syscall
      https://git.kernel.org/arm64/c/d09ee410a3c3

-- 
Catalin