Re: [PATCH v2 1/3] arm64/sve: Fix missing SVE/FPSIMD endianness conversions

[Dave Martin <Dave dot Martin at arm dot com>]

On Wed, Jun 12, 2019 at 06:46:04PM +0100, Julien Grall wrote:
> Hi Dave,
> 
> On 12/06/2019 17:00, Dave Martin wrote:
> >The in-memory representation of SVE and FPSIMD registers is
> >different: the FPSIMD V-registers are stored as single 128-bit
> >host-endian values, whereas SVE registers are stored in an
> >endianness-invariant byte order.
> >
> >This means that the two representations differ when running on a
> >big-endian host.  But we blindly copy data from one representation
> >to another when converting between the two, resulting in the
> >register contents being unintentionally byteswapped in certain
> >situations.  Currently this can be triggered by the first SVE
> >instruction after a syscall, for example (though the potential
> >trigger points may vary in future).
> >
> >So, fix the conversion functions fpsimd_to_sve(), sve_to_fpsimd()
> >and sve_sync_from_fpsimd_zeropad() to swab where appropriate.
> >
> >There is no common swahl128() or swab128() that we could use here.
> >Maybe it would be worth making this generic, but for now add a
> >simple local hack.
> >
> >Since the byte order differences are exposed in ABI, also clarify
> >the docuentation.
> 
> NIT: s/docuentation/documentation/
> 
> Although, it is probably too late to fix this one as Will already took the patch.
> 
> [...]
> 
> >diff --git a/Documentation/arm64/sve.txt b/Documentation/arm64/sve.txt
> >index 9940e92..6c0bed3 100644
> >--- a/Documentation/arm64/sve.txt
> >+++ b/Documentation/arm64/sve.txt
> >@@ -56,6 +56,18 @@ model features for SVE is included in Appendix A.
> >    is to connect to a target process first and then attempt a
> >    ptrace(PTRACE_GETREGSET, pid, NT_ARM_SVE, &iov).
> >+* Whenever SVE scalable register values (Zn, Pn, FFR) are exchanged in memory
> >+  between userspace and the kernel, the register value is encoded in memory in
> >+  an endianness-invariant layout, with bits [(8 * i + 7) : (8 * i)] encoded at
> >+  byte offset i in from the start of the memory representation.  This affects
> >+  for example the signal frame (struct sve_context) and ptrace interface
> >+  (struct user_sve_header) and associated data.
> >+
> >+  Beware that on big-endian systems this results in a different byte order than
> >+  for the FPSIMD V-registers, which are stored as single host-endian 128-bit
> >+  values, with bits [(127 - 8 * i) : (120 - 8 * i)] of the register encoded at
> >+  byte offset i.  (struct fpsimd_context, struct user_fpsimd_state).
> >+
> >  2.  Vector length terminology
> >  -----------------------------
> >@@ -124,6 +136,10 @@ the SVE instruction set architecture.
> >    size and layout.  Macros SVE_SIG_* are defined [1] to facilitate access to
> >    the members.
> >+* Each scalable register (Zn, Pn, FFR) is stored in an endianness-invariant
> >+  layout, with bits [(8 * i + 7) : (8 * i)] stored at byte offset i from the
> >+  start of the register's representation in memory.
> >+
> >  * If the SVE context is too big to fit in sigcontext.__reserved[], then extra
> >    space is allocated on the stack, an extra_context record is written in
> >    __reserved[] referencing this space.  sve_context is then written in the
> >diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >index 7b7ac0f..072ea1e 100644
> >--- a/arch/arm64/include/uapi/asm/kvm.h
> >+++ b/arch/arm64/include/uapi/asm/kvm.h
> >@@ -260,6 +260,13 @@ struct kvm_vcpu_events {
> >  	 KVM_REG_SIZE_U256 |						\
> >  	 ((i) & (KVM_ARM64_SVE_MAX_SLICES - 1)))
> >+/*
> >+ * Register values for KVM_REG_ARM64_SVE_ZREG(), KVM_REG_ARM64_SVE_PREG() and
> >+ * KVM_REG_ARM64_SVE_FFR() and represented in memory in an endianness-
> 
> NIT: s/and represented/are represented/ I think.
> 
> >+ * invariant layout which differs from the layout used for the FPSIMD
> >+ * V-registers on big-endian systems: see sigcontext.h for more explanaion.
> 
> NIT: s/explanaion/explanation/

Dang, the first of these two is quite confusing.

I might send a fix for that, but I guess it's not urgent.  Thanks for
spotting it.

Cheers
---Dave