[PATCH 0/4] arm64/fp: Documentation cleanups and clarifications

[PATCH 0/4] arm64/fp: Documentation cleanups and clarifications

[Mark Brown]

Edwin noticed some issues with the SVE and SME documentation which are
corrected by this series.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (4):
      arm64/sve: Remove bitrotted comment about syscall behaviour
      arm64/sme: Fix cut'n'paste in ABI document
      arm64/fp: Clarify effect of setting an unsupported system VL
      arm64/sme: Remove spurious 'is' in SME documentation

 Documentation/arch/arm64/sme.rst | 11 +++++------
 Documentation/arch/arm64/sve.rst | 10 ++--------
 2 files changed, 7 insertions(+), 14 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240110-arm64-sve-sme-doc-bc62ec1a2005

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

[PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Mark Brown]

When we documented that we always clear state not shared with FPSIMD we
didn't catch all of the places that mentioned that state might not be
cleared, remove a lingering reference.

Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/arch/arm64/sve.rst | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/arch/arm64/sve.rst b/Documentation/arch/arm64/sve.rst
index 0d9a426e9f85..b45a2da19bf1 100644
--- a/Documentation/arch/arm64/sve.rst
+++ b/Documentation/arch/arm64/sve.rst
@@ -117,11 +117,6 @@ the SVE instruction set architecture.
 * The SVE registers are not used to pass arguments to or receive results from
   any syscall.
 
-* In practice the affected registers/bits will be preserved or will be replaced
-  with zeros on return from a syscall, but userspace should not make
-  assumptions about this.  The kernel behaviour may vary on a case-by-case
-  basis.
-
 * All other SVE state of a thread, including the currently configured vector
   length, the state of the PR_SVE_VL_INHERIT flag, and the deferred vector
   length (if any), is preserved across all syscalls, subject to the specific

-- 
2.30.2

Re: [PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Dave Martin]

On Mon, Jan 22, 2024 at 08:41:51PM +0000, Mark Brown wrote:
> When we documented that we always clear state not shared with FPSIMD we

Where / when?

> didn't catch all of the places that mentioned that state might not be
> cleared, remove a lingering reference.
> 
> Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  Documentation/arch/arm64/sve.rst | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/Documentation/arch/arm64/sve.rst b/Documentation/arch/arm64/sve.rst
> index 0d9a426e9f85..b45a2da19bf1 100644
> --- a/Documentation/arch/arm64/sve.rst
> +++ b/Documentation/arch/arm64/sve.rst
> @@ -117,11 +117,6 @@ the SVE instruction set architecture.
>  * The SVE registers are not used to pass arguments to or receive results from
>    any syscall.
>  
> -* In practice the affected registers/bits will be preserved or will be replaced
> -  with zeros on return from a syscall, but userspace should not make
> -  assumptions about this.  The kernel behaviour may vary on a case-by-case
> -  basis.
> -

This was originally an intentionally conservative statement, to allow
the kernel the flexibility to relax the register zeroing behaviour in
the future.  It would have permitted not always disabling a task's SVE
across a syscall, for example.  There were some concerns about security
and testability that meant that we didn't use this flexibility to begin
with.

If we are making an irrevocable commitment not to use this flexibility
ever, then this comment can go, but if we're not totally sure then I
think it would be harmless to keep it (?)

(Feel free to point me to the relevant past discussion that I may have
missed.)

[...]

Cheers
---Dave

Re: [PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Mark Brown]

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

On Tue, Jan 23, 2024 at 03:44:23PM +0000, Dave Martin wrote:
> On Mon, Jan 22, 2024 at 08:41:51PM +0000, Mark Brown wrote:
> > When we documented that we always clear state not shared with FPSIMD we

> Where / when?

In the document that is being modified when it was written.

> > -* In practice the affected registers/bits will be preserved or will be replaced
> > -  with zeros on return from a syscall, but userspace should not make
> > -  assumptions about this.  The kernel behaviour may vary on a case-by-case
> > -  basis.

> This was originally an intentionally conservative statement, to allow
> the kernel the flexibility to relax the register zeroing behaviour in
> the future.  It would have permitted not always disabling a task's SVE
> across a syscall, for example.  There were some concerns about security
> and testability that meant that we didn't use this flexibility to begin
> with.

> If we are making an irrevocable commitment not to use this flexibility
> ever, then this comment can go, but if we're not totally sure then I
> think it would be harmless to keep it (?)

I think everyone except for Catalin had felt that the original
discussion had concluded that there was a commitment to always clear the
non-shared bits and was disappointed to learn that the documentation
said otherwise.  When I tried to take advantage of this as part of
optimising the system call overhead for SVE there were eventually
complaints.

> (Feel free to point me to the relevant past discussion that I may have
> missed.)

See the discussion on my syscall optimisation series:

    https://lore.kernel.org/all/20220620124158.482039-8-broonie@kernel.org/

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

Re: [PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Dave Martin]

On Tue, Jan 23, 2024 at 05:31:58PM +0000, Mark Brown wrote:
> On Tue, Jan 23, 2024 at 03:44:23PM +0000, Dave Martin wrote:
> > On Mon, Jan 22, 2024 at 08:41:51PM +0000, Mark Brown wrote:
> > > When we documented that we always clear state not shared with FPSIMD we
> 
> > Where / when?
> 
> In the document that is being modified when it was written.

Ah, right, I see this:

d09ee410a3c3 ("arm64/sve: Document our actual ABI for clearing registers on syscall")

where the zeroing is made explicit.

> 
> > > -* In practice the affected registers/bits will be preserved or will be replaced
> > > -  with zeros on return from a syscall, but userspace should not make
> > > -  assumptions about this.  The kernel behaviour may vary on a case-by-case
> > > -  basis.
> 
> > This was originally an intentionally conservative statement, to allow
> > the kernel the flexibility to relax the register zeroing behaviour in
> > the future.  It would have permitted not always disabling a task's SVE
> > across a syscall, for example.  There were some concerns about security
> > and testability that meant that we didn't use this flexibility to begin
> > with.
> 
> > If we are making an irrevocable commitment not to use this flexibility
> > ever, then this comment can go, but if we're not totally sure then I
> > think it would be harmless to keep it (?)
> 
> I think everyone except for Catalin had felt that the original
> discussion had concluded that there was a commitment to always clear the
> non-shared bits and was disappointed to learn that the documentation
> said otherwise.  When I tried to take advantage of this as part of
> optimising the system call overhead for SVE there were eventually
> complaints.
> 
> > (Feel free to point me to the relevant past discussion that I may have
> > missed.)
> 
> See the discussion on my syscall optimisation series:
> 
>     https://lore.kernel.org/all/20220620124158.482039-8-broonie@kernel.org/


I think my excuse would be that this was consciously left unresolved
when SVE originally went upstream: the kernel played safe by always
zeroing the bits, while userspace was told not to rely on this always
happening in future.

If the decision has effectively now been made to close the door
permanently those optimisations, then I guess it makes sense to clean
up the documentation to be as consistent as possible.


I still feel that it is iffy practice for userspace to rely on the
extra bits being zeroed -- I think the architecture hides this
guarantee anyway whenever you go through a function call confirming to
the regular procedure call standard (including the syscall wrappers).
But there may not be a lot of point trying to put people off if we
can't force them not to rely on it.

Cheers
---Dave

Re: [PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Mark Brown]

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

On Tue, Jan 23, 2024 at 05:54:17PM +0000, Dave Martin wrote:

> I still feel that it is iffy practice for userspace to rely on the
> extra bits being zeroed -- I think the architecture hides this
> guarantee anyway whenever you go through a function call confirming to
> the regular procedure call standard (including the syscall wrappers).
> But there may not be a lot of point trying to put people off if we
> can't force them not to rely on it.

I do tend to agree that the requirement to zero is excessively zealous
and that the risk from relaxing it is minor (it's stricter than the
function call ABI), I did leave a sysctl as a mechanism for restoring
compatibility in the case where we did run into issues in my original
series but I didn't expect to need it.  If you convince everyone else
I'd be happy to relax things but I don't super care either way.

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

Re: [PATCH 1/4] arm64/sve: Remove bitrotted comment about syscall behaviour

[Dave Martin]

On Tue, Jan 23, 2024 at 06:11:52PM +0000, Mark Brown wrote:
> On Tue, Jan 23, 2024 at 05:54:17PM +0000, Dave Martin wrote:
> 
> > I still feel that it is iffy practice for userspace to rely on the
> > extra bits being zeroed -- I think the architecture hides this
> > guarantee anyway whenever you go through a function call confirming to
> > the regular procedure call standard (including the syscall wrappers).
> > But there may not be a lot of point trying to put people off if we
> > can't force them not to rely on it.
> 
> I do tend to agree that the requirement to zero is excessively zealous
> and that the risk from relaxing it is minor (it's stricter than the
> function call ABI), I did leave a sysctl as a mechanism for restoring
> compatibility in the case where we did run into issues in my original
> series but I didn't expect to need it.  If you convince everyone else
> I'd be happy to relax things but I don't super care either way.

[...]

I don't feel that strongly about it.

Ideally we'd have gone for the fully relaxed approach from the start,
but it's hard to test whether "unspecified" registers aren't leaking
data from somewhere they shouldn't.

Given that the decision has been made anyway, the documentation should
not send mixed messages, so:

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

[PATCH 2/4] arm64/sme: Fix cut'n'paste in ABI document

[Mark Brown]

The ABI for SME is very like that for SVE so bits of the ABI were copied
but not adequately search and replaced, fix that.

Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/arch/arm64/sme.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
index 3d0e53ecac4f..3133d0e91b48 100644
--- a/Documentation/arch/arm64/sme.rst
+++ b/Documentation/arch/arm64/sme.rst
@@ -238,12 +238,12 @@ prctl(PR_SME_SET_VL, unsigned long arg)
       bits of Z0..Z31 except for Z0 bits [127:0] .. Z31 bits [127:0] to become
       unspecified, including both streaming and non-streaming SVE state.
       Calling PR_SME_SET_VL with vl equal to the thread's current vector
-      length, or calling PR_SME_SET_VL with the PR_SVE_SET_VL_ONEXEC flag,
+      length, or calling PR_SME_SET_VL with the PR_SME_SET_VL_ONEXEC flag,
       does not constitute a change to the vector length for this purpose.
 
     * Changing the vector length causes PSTATE.ZA and PSTATE.SM to be cleared.
       Calling PR_SME_SET_VL with vl equal to the thread's current vector
-      length, or calling PR_SME_SET_VL with the PR_SVE_SET_VL_ONEXEC flag,
+      length, or calling PR_SME_SET_VL with the PR_SME_SET_VL_ONEXEC flag,
       does not constitute a change to the vector length for this purpose.
 
 

-- 
2.30.2

[PATCH 3/4] arm64/fp: Clarify effect of setting an unsupported system VL

[Mark Brown]

The documentation for system vector length configuration does not cover all
cases where unsupported values are written, tighten it up.

Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/arch/arm64/sme.rst | 5 ++---
 Documentation/arch/arm64/sve.rst | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
index 3133d0e91b48..ba0a5e5b2523 100644
--- a/Documentation/arch/arm64/sme.rst
+++ b/Documentation/arch/arm64/sme.rst
@@ -379,9 +379,8 @@ The regset data starts with struct user_za_header, containing:
 /proc/sys/abi/sme_default_vector_length
 
     Writing the text representation of an integer to this file sets the system
-    default vector length to the specified value, unless the value is greater
-    than the maximum vector length supported by the system in which case the
-    default vector length is set to that maximum.
+    default vector length to the specified value rounded to a supported value
+    using the same rules as for setting vector length via prctl().
 
     The result can be determined by reopening the file and reading its
     contents.
diff --git a/Documentation/arch/arm64/sve.rst b/Documentation/arch/arm64/sve.rst
index b45a2da19bf1..b923727ff4b9 100644
--- a/Documentation/arch/arm64/sve.rst
+++ b/Documentation/arch/arm64/sve.rst
@@ -423,9 +423,8 @@ The regset data starts with struct user_sve_header, containing:
 /proc/sys/abi/sve_default_vector_length
 
     Writing the text representation of an integer to this file sets the system
-    default vector length to the specified value, unless the value is greater
-    than the maximum vector length supported by the system in which case the
-    default vector length is set to that maximum.
+    default vector length to the specified value rounded to a supported value
+    using the same rules as for setting vector length via prctl().
 
     The result can be determined by reopening the file and reading its
     contents.

-- 
2.30.2

Re: [PATCH 3/4] arm64/fp: Clarify effect of setting an unsupported system VL

[Dave Martin]

On Mon, Jan 22, 2024 at 08:41:53PM +0000, Mark Brown wrote:
> The documentation for system vector length configuration does not cover all
> cases where unsupported values are written, tighten it up.
> 
> Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  Documentation/arch/arm64/sme.rst | 5 ++---
>  Documentation/arch/arm64/sve.rst | 5 ++---
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
> index 3133d0e91b48..ba0a5e5b2523 100644
> --- a/Documentation/arch/arm64/sme.rst
> +++ b/Documentation/arch/arm64/sme.rst
> @@ -379,9 +379,8 @@ The regset data starts with struct user_za_header, containing:
>  /proc/sys/abi/sme_default_vector_length
>  
>      Writing the text representation of an integer to this file sets the system
> -    default vector length to the specified value, unless the value is greater
> -    than the maximum vector length supported by the system in which case the
> -    default vector length is set to that maximum.
> +    default vector length to the specified value rounded to a supported value
> +    using the same rules as for setting vector length via prctl().

Do parallel changes need to be made in sve.rst?

(There seems to be so much duplication and copy-paste between these
files that I wonder whether it would make sense to merge them...  but
that's probably a separate discussion.)

Nit: is it better to name the prctl here than just to say prctl()?
That would be easier for the reader to cross-reference.

>  
>      The result can be determined by reopening the file and reading its
>      contents.
> diff --git a/Documentation/arch/arm64/sve.rst b/Documentation/arch/arm64/sve.rst
> index b45a2da19bf1..b923727ff4b9 100644
> --- a/Documentation/arch/arm64/sve.rst
> +++ b/Documentation/arch/arm64/sve.rst
> @@ -423,9 +423,8 @@ The regset data starts with struct user_sve_header, containing:
>  /proc/sys/abi/sve_default_vector_length
>  
>      Writing the text representation of an integer to this file sets the system
> -    default vector length to the specified value, unless the value is greater
> -    than the maximum vector length supported by the system in which case the
> -    default vector length is set to that maximum.
> +    default vector length to the specified value rounded to a supported value
> +    using the same rules as for setting vector length via prctl().

Ditto.

[...]

Cheers
---Dave

Re: [PATCH 3/4] arm64/fp: Clarify effect of setting an unsupported system VL

[Mark Brown]

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

On Tue, Jan 23, 2024 at 03:49:27PM +0000, Dave Martin wrote:
> On Mon, Jan 22, 2024 at 08:41:53PM +0000, Mark Brown wrote:

> >  /proc/sys/abi/sme_default_vector_length
> >  
> >      Writing the text representation of an integer to this file sets the system
> > -    default vector length to the specified value, unless the value is greater
> > -    than the maximum vector length supported by the system in which case the
> > -    default vector length is set to that maximum.
> > +    default vector length to the specified value rounded to a supported value
> > +    using the same rules as for setting vector length via prctl().

> Do parallel changes need to be made in sve.rst?

They are, in this very patch?

> (There seems to be so much duplication and copy-paste between these
> files that I wonder whether it would make sense to merge them...  but
> that's probably a separate discussion.)

Indeed, thanks for volunteering.  Note that there are differences
resulting from specification differences.

> Nit: is it better to name the prctl here than just to say prctl()?
> That would be easier for the reader to cross-reference.

I guess, though it doesn't seem entirely idiomatic.

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

Re: [PATCH 3/4] arm64/fp: Clarify effect of setting an unsupported system VL

[Dave Martin]

On Tue, Jan 23, 2024 at 06:42:03PM +0000, Mark Brown wrote:
> On Tue, Jan 23, 2024 at 03:49:27PM +0000, Dave Martin wrote:
> > On Mon, Jan 22, 2024 at 08:41:53PM +0000, Mark Brown wrote:
> 
> > >  /proc/sys/abi/sme_default_vector_length
> > >  
> > >      Writing the text representation of an integer to this file sets the system
> > > -    default vector length to the specified value, unless the value is greater
> > > -    than the maximum vector length supported by the system in which case the
> > > -    default vector length is set to that maximum.
> > > +    default vector length to the specified value rounded to a supported value
> > > +    using the same rules as for setting vector length via prctl().
> 
> > Do parallel changes need to be made in sve.rst?
> 
> They are, in this very patch?

Duh, yes.  My brain seems to have auto-ignored the second hunk, since it
was clearly a duplicate :P

> > (There seems to be so much duplication and copy-paste between these
> > files that I wonder whether it would make sense to merge them...  but
> > that's probably a separate discussion.)
> 
> Indeed, thanks for volunteering.  Note that there are differences
> resulting from specification differences.

Thanks for agreeing to an unspecfied deadline ;)

I might have a go at some point though, just to familiarise myself with
the differences...

> > Nit: is it better to name the prctl here than just to say prctl()?
> > That would be easier for the reader to cross-reference.
> 
> I guess, though it doesn't seem entirely idiomatic.

I expect counterexamples can be found, but I guess the reader can figure
it out either way.

Cheers
---Dave

[PATCH 4/4] arm64/sme: Remove spurious 'is' in SME documentation

[Mark Brown]

Just a typographical error.

Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 Documentation/arch/arm64/sme.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
index ba0a5e5b2523..bc9855e599c8 100644
--- a/Documentation/arch/arm64/sme.rst
+++ b/Documentation/arch/arm64/sme.rst
@@ -75,7 +75,7 @@ model features for SME is included in Appendix A.
 2.  Vector lengths
 ------------------
 
-SME defines a second vector length similar to the SVE vector length which is
+SME defines a second vector length similar to the SVE vector length which
 controls the size of the streaming mode SVE vectors and the ZA matrix array.
 The ZA matrix is square with each side having as many bytes as a streaming
 mode SVE vector.

-- 
2.30.2

Re: [PATCH 4/4] arm64/sme: Remove spurious 'is' in SME documentation

[Dave Martin]

On Mon, Jan 22, 2024 at 08:41:54PM +0000, Mark Brown wrote:
> Just a typographical error.
> 
> Reported-by: Edmund Grimley-Evans <edmund.grimley-evans@arm.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>

FWIW (just to break my duck),

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

> ---
>  Documentation/arch/arm64/sme.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/arch/arm64/sme.rst b/Documentation/arch/arm64/sme.rst
> index ba0a5e5b2523..bc9855e599c8 100644
> --- a/Documentation/arch/arm64/sme.rst
> +++ b/Documentation/arch/arm64/sme.rst
> @@ -75,7 +75,7 @@ model features for SME is included in Appendix A.
>  2.  Vector lengths
>  ------------------
>  
> -SME defines a second vector length similar to the SVE vector length which is
> +SME defines a second vector length similar to the SVE vector length which
>  controls the size of the streaming mode SVE vectors and the ZA matrix array.
>  The ZA matrix is square with each side having as many bytes as a streaming
>  mode SVE vector.
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel