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

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/

Cheers,

-- 
Julien Grall