[PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Dave Martin
By inspection while debugging something else, I noticed that the byte
order of FPSIMD V-register stores and SVE Z-register stores is not the
same when running on big-endian.

This is not properly taken into account when moving between the FPSIMD
and SVE register views inside the kernel, resulting in the bytes of a
V-register getting spontaneously reversed in some situations, from
userspace's point of view.  The signal frame and ptrace interface are
also affected.  The KVM ABI forbids mixing the two views and so should
not be affected.

See patch 2 for details.

Patch 1 does some trivial preparatory refactoring.

gdb may or may not be affected by this, depending on how it uses the
NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
assess that.

Dave Martin (2):
  arm64/sve: Factor out FPSIMD to SVE state conversion
  arm64/sve: Fix missing SVE/FPSIMD endianness conversions

 Documentation/arm64/sve.txt              | 16 +++++++++++
 arch/arm64/include/uapi/asm/kvm.h        |  7 +++++
 arch/arm64/include/uapi/asm/ptrace.h     |  4 +++
 arch/arm64/include/uapi/asm/sigcontext.h | 14 +++++++++
 arch/arm64/kernel/fpsimd.c               | 49 ++++++++++++++++++++++++--------
 5 files changed, 78 insertions(+), 12 deletions(-)

--
2.1.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] arm64/sve: Factor out FPSIMD to SVE state conversion

Dave Martin
Currently we convert from FPSIMD to SVE register state in memory in
two places.

We are about to amend the way this works, so factor this operation
out so that subsequent changes only have to be made in one place.

Signed-off-by: Dave Martin <[hidden email]>
---
 arch/arm64/kernel/fpsimd.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index a38bf74..61ceeb9 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -352,6 +352,16 @@ static int __init sve_sysctl_init(void) { return 0; }
 #define ZREG(sve_state, vq, n) ((char *)(sve_state) + \
  (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
 
+static void __fpsimd_to_sve(void *sst, struct user_fpsimd_state const *fst,
+    unsigned int vq)
+{
+ unsigned int i;
+
+ for (i = 0; i < 32; ++i)
+ memcpy(ZREG(sst, vq, i), &fst->vregs[i],
+       sizeof(fst->vregs[i]));
+}
+
 /*
  * Transfer the FPSIMD state in task->thread.uw.fpsimd_state to
  * task->thread.sve_state.
@@ -368,15 +378,12 @@ static void fpsimd_to_sve(struct task_struct *task)
  unsigned int vq;
  void *sst = task->thread.sve_state;
  struct user_fpsimd_state const *fst = &task->thread.uw.fpsimd_state;
- unsigned int i;
 
  if (!system_supports_sve())
  return;
 
  vq = sve_vq_from_vl(task->thread.sve_vl);
- for (i = 0; i < 32; ++i)
- memcpy(ZREG(sst, vq, i), &fst->vregs[i],
-       sizeof(fst->vregs[i]));
+ __fpsimd_to_sve(sst, fst, vq);
 }
 
 /*
@@ -490,7 +497,6 @@ void sve_sync_from_fpsimd_zeropad(struct task_struct *task)
  unsigned int vq;
  void *sst = task->thread.sve_state;
  struct user_fpsimd_state const *fst = &task->thread.uw.fpsimd_state;
- unsigned int i;
 
  if (!test_tsk_thread_flag(task, TIF_SVE))
  return;
@@ -498,10 +504,7 @@ void sve_sync_from_fpsimd_zeropad(struct task_struct *task)
  vq = sve_vq_from_vl(task->thread.sve_vl);
 
  memset(sst, 0, SVE_SIG_REGS_SIZE(vq));
-
- for (i = 0; i < 32; ++i)
- memcpy(ZREG(sst, vq, i), &fst->vregs[i],
-       sizeof(fst->vregs[i]));
+ __fpsimd_to_sve(sst, fst, vq);
 }
 
 int sve_set_vector_length(struct task_struct *task,
--
2.1.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] arm64/sve: Fix missing SVE/FPSIMD endianness conversions

Dave Martin
In reply to this post by Dave Martin
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() and
sve_to_fpsimd() 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.

Fixes: bc0ee4760364 ("arm64/sve: Core task context handling")
Fixes: 8cd969d28fd2 ("arm64/sve: Signal handling support")
Fixes: 43d4da2c45b2 ("arm64/sve: ptrace and ELF coredump support")
Signed-off-by: Dave Martin <[hidden email]>

---

The ptrace change is theoretically an ABI break, but since the current
behaviour is obviously wrong, I consider this a fix.

Tested on the Arm Fast Model, using:

 * asm code that mixes SVE instructions and syscalls;

 * ptrace interactions that mix the SVE_PT_REGS_FPSIMD and
   SVE_PT_REGS_SVE views of NT_ARM_SVE.

Signal frame behaviour not directly tested (since the underlying
conversion functions are the same in all cases).

Demonstrator code, on a big-endian platform:

.arch_extension sve

        index z0.b, #0, #1
        str v0, [x0]
// x0 -> 0f 0e 0d 0c 0b 0a 09 08 07 06 05 04 03 02 01 00

        mov w8, #__NR_getpid
        svc #0 // any noop syscall, reverts to non-SVE regs
        str v0, [x0]
// x0 -> 0f 0e 0d 0c 0b 0a 09 08 07 06 05 04 03 02 01 00

        mov z0.d, z0.d
        // triggers an SVE trap and buggy FPSIMD->SVE conversion
        str v0, [x0]
// x0 -> 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f

While a "cool feature", this is not what was intended.
---
 Documentation/arm64/sve.txt              | 16 +++++++++++++++
 arch/arm64/include/uapi/asm/kvm.h        |  7 +++++++
 arch/arm64/include/uapi/asm/ptrace.h     |  4 ++++
 arch/arm64/include/uapi/asm/sigcontext.h | 14 +++++++++++++
 arch/arm64/kernel/fpsimd.c               | 34 ++++++++++++++++++++++++++------
 5 files changed, 69 insertions(+), 6 deletions(-)

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-
+ * invariant layout which differs from the layout used for the FPSIMD
+ * V-registers on big-endian systems: see sigcontext.h for more explanaion.
+ */
+
 #define KVM_ARM64_SVE_VQ_MIN __SVE_VQ_MIN
 #define KVM_ARM64_SVE_VQ_MAX __SVE_VQ_MAX
 
diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
index d78623a..018bf7d 100644
--- a/arch/arm64/include/uapi/asm/ptrace.h
+++ b/arch/arm64/include/uapi/asm/ptrace.h
@@ -176,6 +176,10 @@ struct user_sve_header {
  * FPCR uint32_t FPCR
  *
  * Additional data might be appended in the future.
+ *
+ * The Z-, P- and FFR registers and represented in memory in an endianness-
+ * invariant layout which differs from the layout used for the FPSIMD
+ * V-registers on big-endian systems: see sigcontext.h for more explanaion.
  */
 
 #define SVE_PT_SVE_ZREG_SIZE(vq) __SVE_ZREG_SIZE(vq)
diff --git a/arch/arm64/include/uapi/asm/sigcontext.h b/arch/arm64/include/uapi/asm/sigcontext.h
index 5f3c0ce..5231569 100644
--- a/arch/arm64/include/uapi/asm/sigcontext.h
+++ b/arch/arm64/include/uapi/asm/sigcontext.h
@@ -77,6 +77,15 @@ struct fpsimd_context {
  __uint128_t vregs[32];
 };
 
+/*
+ * Note: similarly to all other integer fields, each V-register is stored in an
+ * endianness-dependent format, with the byte at offset i from the start of the
+ * in-memory representation of the register value containing
+ *
+ *    bits [(7 + 8 * i) : (8 * i)] of the register on little-endian hosts; or
+ *    bits [(127 - 8 * i) : (120 - 8 * i)] on big-endian hosts.
+ */
+
 /* ESR_EL1 context */
 #define ESR_MAGIC 0x45535201
 
@@ -204,6 +213,11 @@ struct sve_context {
  * FFR uint16_t[vq] first-fault status register
  *
  * Additional data might be appended in the future.
+ *
+ * Unlike vregs[] in fpsimd_context, each SVE scalable register (Z-, P- or FFR)
+ * is encoded in memory an endianness-invariant format, with the byte at offset
+ * i from the start of the in-memory representation containing bits
+ * [(7 + 8 * i) : (8 * i)] of the register value.
  */
 
 #define SVE_SIG_ZREG_SIZE(vq) __SVE_ZREG_SIZE(vq)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 61ceeb9..d2f7544 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -39,6 +39,7 @@
 #include <linux/slab.h>
 #include <linux/stddef.h>
 #include <linux/sysctl.h>
+#include <linux/swab.h>
 
 #include <asm/esr.h>
 #include <asm/fpsimd.h>
@@ -352,14 +353,33 @@ static int __init sve_sysctl_init(void) { return 0; }
 #define ZREG(sve_state, vq, n) ((char *)(sve_state) + \
  (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
 
+#ifdef CONFIG_CPU_BIG_ENDIAN
+static __uint128_t arm64_cpu_to_le128(__uint128_t x)
+{
+ u64 a = swab64(x);
+ u64 b = swab64(x >> 64);
+
+ return ((__uint128_t)a << 64) | b;
+}
+#else
+static __uint128_t arm64_cpu_to_le128(__uint128_t x)
+{
+ return x;
+}
+#endif
+
+#define arm64_le128_to_cpu(x) arm64_cpu_to_le128(x)
+
 static void __fpsimd_to_sve(void *sst, struct user_fpsimd_state const *fst,
     unsigned int vq)
 {
  unsigned int i;
+ __uint128_t *p;
 
- for (i = 0; i < 32; ++i)
- memcpy(ZREG(sst, vq, i), &fst->vregs[i],
-       sizeof(fst->vregs[i]));
+ for (i = 0; i < 32; ++i) {
+ p = (__uint128_t *)ZREG(sst, vq, i);
+ *p = arm64_cpu_to_le128(fst->vregs[i]);
+ }
 }
 
 /*
@@ -402,14 +422,16 @@ static void sve_to_fpsimd(struct task_struct *task)
  void const *sst = task->thread.sve_state;
  struct user_fpsimd_state *fst = &task->thread.uw.fpsimd_state;
  unsigned int i;
+ __uint128_t const *p;
 
  if (!system_supports_sve())
  return;
 
  vq = sve_vq_from_vl(task->thread.sve_vl);
- for (i = 0; i < 32; ++i)
- memcpy(&fst->vregs[i], ZREG(sst, vq, i),
-       sizeof(fst->vregs[i]));
+ for (i = 0; i < 32; ++i) {
+ p = (__uint128_t const *)ZREG(sst, vq, i);
+ fst->vregs[i] = arm64_le128_to_cpu(*p);
+ }
 }
 
 #ifdef CONFIG_ARM64_SVE
--
2.1.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Will Deacon
In reply to this post by Dave Martin
On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:

> By inspection while debugging something else, I noticed that the byte
> order of FPSIMD V-register stores and SVE Z-register stores is not the
> same when running on big-endian.
>
> This is not properly taken into account when moving between the FPSIMD
> and SVE register views inside the kernel, resulting in the bytes of a
> V-register getting spontaneously reversed in some situations, from
> userspace's point of view.  The signal frame and ptrace interface are
> also affected.  The KVM ABI forbids mixing the two views and so should
> not be affected.
>
> See patch 2 for details.
>
> Patch 1 does some trivial preparatory refactoring.

Sorry to be a pain, but would you be able to flip this series round so that
the fix doesn't depend on the refactoring, please? That way we can put it
into stable without the dependency.

> gdb may or may not be affected by this, depending on how it uses the
> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
> assess that.

Wouldn't this be easy enough to test?

Will
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Dave Martin
On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:

> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
> > By inspection while debugging something else, I noticed that the byte
> > order of FPSIMD V-register stores and SVE Z-register stores is not the
> > same when running on big-endian.
> >
> > This is not properly taken into account when moving between the FPSIMD
> > and SVE register views inside the kernel, resulting in the bytes of a
> > V-register getting spontaneously reversed in some situations, from
> > userspace's point of view.  The signal frame and ptrace interface are
> > also affected.  The KVM ABI forbids mixing the two views and so should
> > not be affected.
> >
> > See patch 2 for details.
> >
> > Patch 1 does some trivial preparatory refactoring.
>
> Sorry to be a pain, but would you be able to flip this series round so that
> the fix doesn't depend on the refactoring, please? That way we can put it
> into stable without the dependency.
>
> > gdb may or may not be affected by this, depending on how it uses the
> > NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
> > assess that.
>
> Wouldn't this be easy enough to test?

So, gdb works OK on big-endian but weird stuff happening on both with
and without the fix.

There are places in the gdb code itself where it is likely missing
endianness conversions, but I need to follow up with the gdb folks to
clarify whether my patch is missing something...

Cheers
---Dave
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Alan Hayward


> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
>
> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
>>> By inspection while debugging something else, I noticed that the byte
>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
>>> same when running on big-endian.
>>>
>>> This is not properly taken into account when moving between the FPSIMD
>>> and SVE register views inside the kernel, resulting in the bytes of a
>>> V-register getting spontaneously reversed in some situations, from
>>> userspace's point of view.  The signal frame and ptrace interface are
>>> also affected.  The KVM ABI forbids mixing the two views and so should
>>> not be affected.
>>>
>>> See patch 2 for details.
>>>
>>> Patch 1 does some trivial preparatory refactoring.
>>
>> Sorry to be a pain, but would you be able to flip this series round so that
>> the fix doesn't depend on the refactoring, please? That way we can put it
>> into stable without the dependency.
>>
>>> gdb may or may not be affected by this, depending on how it uses the
>>> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
>>> assess that.
>>
>> Wouldn't this be easy enough to test?
>
> So, gdb works OK on big-endian but weird stuff happening on both with
> and without the fix.
>
> There are places in the gdb code itself where it is likely missing
> endianness conversions, but I need to follow up with the gdb folks to
> clarify whether my patch is missing something…

(I added the SVE support for GDB).

I’ve tried these changes out myself using GDB.
With your changes everything looks good, apart from:
* GDB gets it wrong when the ptrace sve structure contains a fpsimd.
* I need to do some testing around sigcontexts, but again I think GDB
  will need a slight change.
I’ll get some patches together for GDB.


> The ptrace change is theoretically an ABI break, but since the current
> behaviour is obviously wrong, I consider this a fix.

I’m happy with this change from GDB's side.


Thanks,
Alan.


>
> Cheers
> ---Dave
>
> _______________________________________________
> linux-arm-kernel mailing list
> [hidden email]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Dave Martin
On Tue, Jun 11, 2019 at 04:16:11PM +0000, Alan Hayward wrote:

>
>
> > On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
> >
> > On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
> >> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
> >>> By inspection while debugging something else, I noticed that the byte
> >>> order of FPSIMD V-register stores and SVE Z-register stores is not the
> >>> same when running on big-endian.
> >>>
> >>> This is not properly taken into account when moving between the FPSIMD
> >>> and SVE register views inside the kernel, resulting in the bytes of a
> >>> V-register getting spontaneously reversed in some situations, from
> >>> userspace's point of view.  The signal frame and ptrace interface are
> >>> also affected.  The KVM ABI forbids mixing the two views and so should
> >>> not be affected.
> >>>
> >>> See patch 2 for details.
> >>>
> >>> Patch 1 does some trivial preparatory refactoring.
> >>
> >> Sorry to be a pain, but would you be able to flip this series round so that
> >> the fix doesn't depend on the refactoring, please? That way we can put it
> >> into stable without the dependency.
> >>
> >>> gdb may or may not be affected by this, depending on how it uses the
> >>> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
> >>> assess that.
> >>
> >> Wouldn't this be easy enough to test?
> >
> > So, gdb works OK on big-endian but weird stuff happening on both with
> > and without the fix.
> >
> > There are places in the gdb code itself where it is likely missing
> > endianness conversions, but I need to follow up with the gdb folks to
> > clarify whether my patch is missing something…
>
> (I added the SVE support for GDB).
>
> I’ve tried these changes out myself using GDB.
> With your changes everything looks good, apart from:
> * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
> * I need to do some testing around sigcontexts, but again I think GDB
>   will need a slight change.
> I’ll get some patches together for GDB.
>
>
> > The ptrace change is theoretically an ABI break, but since the current
> > behaviour is obviously wrong, I consider this a fix.
>
> I’m happy with this change from GDB's side.

OK, thanks for confirming.

Cheers
---Dave
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Alex Bennée
In reply to this post by Alan Hayward

Alan Hayward <[hidden email]> writes:

>> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
>>
>> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
>>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
>>>> By inspection while debugging something else, I noticed that the byte
>>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
>>>> same when running on big-endian.
>>>>
>>>> This is not properly taken into account when moving between the FPSIMD
>>>> and SVE register views inside the kernel, resulting in the bytes of a
>>>> V-register getting spontaneously reversed in some situations, from
>>>> userspace's point of view.  The signal frame and ptrace interface are
>>>> also affected.  The KVM ABI forbids mixing the two views and so should
>>>> not be affected.
>>>>
>>>> See patch 2 for details.
>>>>
>>>> Patch 1 does some trivial preparatory refactoring.
>>>
>>> Sorry to be a pain, but would you be able to flip this series round so that
>>> the fix doesn't depend on the refactoring, please? That way we can put it
>>> into stable without the dependency.
>>>
>>>> gdb may or may not be affected by this, depending on how it uses the
>>>> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
>>>> assess that.
>>>
>>> Wouldn't this be easy enough to test?
>>
>> So, gdb works OK on big-endian but weird stuff happening on both with
>> and without the fix.
>>
>> There are places in the gdb code itself where it is likely missing
>> endianness conversions, but I need to follow up with the gdb folks to
>> clarify whether my patch is missing something…
>
> (I added the SVE support for GDB).
>
> I’ve tried these changes out myself using GDB.
> With your changes everything looks good, apart from:
> * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
> * I need to do some testing around sigcontexts, but again I think GDB
>   will need a slight change.
> I’ll get some patches together for GDB.

Where is the latest state of SVE support for GDB? I really should check
the QEMU gdbstub does the correct things for SVE registers but I was
waiting for upstream gdb support.

--
Alex Bennée
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Alan Hayward


> On 12 Jun 2019, at 11:40, Alex Bennée <[hidden email]> wrote:
>
>
> Alan Hayward <[hidden email]> writes:
>
>>> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
>>>
>>> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
>>>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
>>>>> By inspection while debugging something else, I noticed that the byte
>>>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
>>>>> same when running on big-endian.
>>>>>
>>>>> This is not properly taken into account when moving between the FPSIMD
>>>>> and SVE register views inside the kernel, resulting in the bytes of a
>>>>> V-register getting spontaneously reversed in some situations, from
>>>>> userspace's point of view.  The signal frame and ptrace interface are
>>>>> also affected.  The KVM ABI forbids mixing the two views and so should
>>>>> not be affected.
>>>>>
>>>>> See patch 2 for details.
>>>>>
>>>>> Patch 1 does some trivial preparatory refactoring.
>>>>
>>>> Sorry to be a pain, but would you be able to flip this series round so that
>>>> the fix doesn't depend on the refactoring, please? That way we can put it
>>>> into stable without the dependency.
>>>>
>>>>> gdb may or may not be affected by this, depending on how it uses the
>>>>> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
>>>>> assess that.
>>>>
>>>> Wouldn't this be easy enough to test?
>>>
>>> So, gdb works OK on big-endian but weird stuff happening on both with
>>> and without the fix.
>>>
>>> There are places in the gdb code itself where it is likely missing
>>> endianness conversions, but I need to follow up with the gdb folks to
>>> clarify whether my patch is missing something…
>>
>> (I added the SVE support for GDB).
>>
>> I’ve tried these changes out myself using GDB.
>> With your changes everything looks good, apart from:
>> * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
>> * I need to do some testing around sigcontexts, but again I think GDB
>>  will need a slight change.
>> I’ll get some patches together for GDB.
>
> Where is the latest state of SVE support for GDB? I really should check
> the QEMU gdbstub does the correct things for SVE registers but I was
> waiting for upstream gdb support.

SVE is supported in GDB 8.2.

If your program starts changing vector lengths whilst the process is running,
then you’ll run into issues. Fixed for GDB in HEAD, but not for remote stubs.
Let me know if that’s a problem for you - I’ve yet to find real uses cases
for needing it.


Alan.

>
> --
> Alex Bennée

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Dave Martin
In reply to this post by Alex Bennée
On Wed, Jun 12, 2019 at 11:40:11AM +0100, Alex Bennée wrote:

>
> Alan Hayward <[hidden email]> writes:
>
> >> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
> >>
> >> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
> >>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
> >>>> By inspection while debugging something else, I noticed that the byte
> >>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
> >>>> same when running on big-endian.
> >>>>
> >>>> This is not properly taken into account when moving between the FPSIMD
> >>>> and SVE register views inside the kernel, resulting in the bytes of a
> >>>> V-register getting spontaneously reversed in some situations, from
> >>>> userspace's point of view.  The signal frame and ptrace interface are
> >>>> also affected.  The KVM ABI forbids mixing the two views and so should
> >>>> not be affected.
> >>>>
> >>>> See patch 2 for details.
> >>>>
> >>>> Patch 1 does some trivial preparatory refactoring.
> >>>
> >>> Sorry to be a pain, but would you be able to flip this series round so that
> >>> the fix doesn't depend on the refactoring, please? That way we can put it
> >>> into stable without the dependency.
> >>>
> >>>> gdb may or may not be affected by this, depending on how it uses the
> >>>> NT_PRFPREG and NT_ARM_SVE regsets.  I'll leave it to the developers to
> >>>> assess that.
> >>>
> >>> Wouldn't this be easy enough to test?
> >>
> >> So, gdb works OK on big-endian but weird stuff happening on both with
> >> and without the fix.
> >>
> >> There are places in the gdb code itself where it is likely missing
> >> endianness conversions, but I need to follow up with the gdb folks to
> >> clarify whether my patch is missing something…
> >
> > (I added the SVE support for GDB).
> >
> > I’ve tried these changes out myself using GDB.
> > With your changes everything looks good, apart from:
> > * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
> > * I need to do some testing around sigcontexts, but again I think GDB
> >   will need a slight change.
> > I’ll get some patches together for GDB.
>
> Where is the latest state of SVE support for GDB? I really should check
> the QEMU gdbstub does the correct things for SVE registers but I was
> waiting for upstream gdb support.

Does this issue need looking at for the QEMU userspace emulation too?

Cheers
---Dave
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Alex Bennée

Dave Martin <[hidden email]> writes:

> On Wed, Jun 12, 2019 at 11:40:11AM +0100, Alex Bennée wrote:
>>
>> Alan Hayward <[hidden email]> writes:
>>
>> >> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
>> >>
>> >> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
>> >>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
>> >>>> By inspection while debugging something else, I noticed that the byte
>> >>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
>> >>>> same when running on big-endian.
>> >>>>
>> >>>> This is not properly taken into account when moving between the FPSIMD
>> >>>> and SVE register views inside the kernel, resulting in the bytes of a
>> >>>> V-register getting spontaneously reversed in some situations, from
>> >>>> userspace's point of view.  The signal frame and ptrace interface are
>> >>>> also affected.  The KVM ABI forbids mixing the two views and so should
>> >>>> not be affected.
<snip>

>> >>>
>> >>> Wouldn't this be easy enough to test?
>> >>
>> >> So, gdb works OK on big-endian but weird stuff happening on both with
>> >> and without the fix.
>> >>
>> >> There are places in the gdb code itself where it is likely missing
>> >> endianness conversions, but I need to follow up with the gdb folks to
>> >> clarify whether my patch is missing something…
>> >
>> > (I added the SVE support for GDB).
>> >
>> > I’ve tried these changes out myself using GDB.
>> > With your changes everything looks good, apart from:
>> > * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
>> > * I need to do some testing around sigcontexts, but again I think GDB
>> >   will need a slight change.
>> > I’ll get some patches together for GDB.
>>
>> Where is the latest state of SVE support for GDB? I really should check
>> the QEMU gdbstub does the correct things for SVE registers but I was
>> waiting for upstream gdb support.
>
> Does this issue need looking at for the QEMU userspace emulation too?

Hmm I think we are OK. For the SVE frame itself we explicitly store in
LE order:

  static void target_setup_sve_record(struct target_sve_context *sve,
                                      CPUARMState *env, int vq, int size)
  {
      int i, j;

      __put_user(TARGET_SVE_MAGIC, &sve->head.magic);
      __put_user(size, &sve->head.size);
      __put_user(vq * TARGET_SVE_VQ_BYTES, &sve->vl);

      /* Note that SVE regs are stored as a byte stream, with each byte element
       * at a subsequent address.  This corresponds to a little-endian store
       * of our 64-bit hunks.
       */
      for (i = 0; i < 32; ++i) {
          uint64_t *z = (void *)sve + TARGET_SVE_SIG_ZREG_OFFSET(vq, i);
          for (j = 0; j < vq * 2; ++j) {
              __put_user_e(env->vfp.zregs[i].d[j], z + j, le);
          }
      }
      for (i = 0; i <= 16; ++i) {
          uint16_t *p = (void *)sve + TARGET_SVE_SIG_PREG_OFFSET(vq, i);
          for (j = 0; j < vq; ++j) {
              uint64_t r = env->vfp.pregs[i].p[j >> 2];
              __put_user_e(r >> ((j & 3) * 16), p + j, le);
          }
      }
  }

For the aliased fpsimd registers we store in the target endian format:

  static void target_setup_fpsimd_record(struct target_fpsimd_context *fpsimd,
                                         CPUARMState *env)
  {
      int i;

      __put_user(TARGET_FPSIMD_MAGIC, &fpsimd->head.magic);
      __put_user(sizeof(struct target_fpsimd_context), &fpsimd->head.size);
      __put_user(vfp_get_fpsr(env), &fpsimd->fpsr);
      __put_user(vfp_get_fpcr(env), &fpsimd->fpcr);

      for (i = 0; i < 32; i++) {
          uint64_t *q = aa64_vfp_qreg(env, i);
  #ifdef TARGET_WORDS_BIGENDIAN
          __put_user(q[0], &fpsimd->vregs[i * 2 + 1]);
          __put_user(q[1], &fpsimd->vregs[i * 2]);
  #else
          __put_user(q[0], &fpsimd->vregs[i * 2]);
          __put_user(q[1], &fpsimd->vregs[i * 2 + 1]);
  #endif
      }
  }

Where our layout for the quads is always:

  Qn = regs[n].d[1]:regs[n].d[0]



--
Alex Bennée
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] arm64/sve: Fix mutating register endianness on big-endian

Dave Martin
On Wed, Jun 12, 2019 at 02:18:29PM +0100, Alex Bennée wrote:

>
> Dave Martin <[hidden email]> writes:
>
> > On Wed, Jun 12, 2019 at 11:40:11AM +0100, Alex Bennée wrote:
> >>
> >> Alan Hayward <[hidden email]> writes:
> >>
> >> >> On 7 Jun 2019, at 16:48, Dave Martin <[hidden email]> wrote:
> >> >>
> >> >> On Fri, Jun 07, 2019 at 10:38:58AM +0100, Will Deacon wrote:
> >> >>> On Thu, Jun 06, 2019 at 05:44:53PM +0100, Dave Martin wrote:
> >> >>>> By inspection while debugging something else, I noticed that the byte
> >> >>>> order of FPSIMD V-register stores and SVE Z-register stores is not the
> >> >>>> same when running on big-endian.
> >> >>>>
> >> >>>> This is not properly taken into account when moving between the FPSIMD
> >> >>>> and SVE register views inside the kernel, resulting in the bytes of a
> >> >>>> V-register getting spontaneously reversed in some situations, from
> >> >>>> userspace's point of view.  The signal frame and ptrace interface are
> >> >>>> also affected.  The KVM ABI forbids mixing the two views and so should
> >> >>>> not be affected.
> <snip>
> >> >>>
> >> >>> Wouldn't this be easy enough to test?
> >> >>
> >> >> So, gdb works OK on big-endian but weird stuff happening on both with
> >> >> and without the fix.
> >> >>
> >> >> There are places in the gdb code itself where it is likely missing
> >> >> endianness conversions, but I need to follow up with the gdb folks to
> >> >> clarify whether my patch is missing something…
> >> >
> >> > (I added the SVE support for GDB).
> >> >
> >> > I’ve tried these changes out myself using GDB.
> >> > With your changes everything looks good, apart from:
> >> > * GDB gets it wrong when the ptrace sve structure contains a fpsimd.
> >> > * I need to do some testing around sigcontexts, but again I think GDB
> >> >   will need a slight change.
> >> > I’ll get some patches together for GDB.
> >>
> >> Where is the latest state of SVE support for GDB? I really should check
> >> the QEMU gdbstub does the correct things for SVE registers but I was
> >> waiting for upstream gdb support.
> >
> > Does this issue need looking at for the QEMU userspace emulation too?
>
> Hmm I think we are OK. For the SVE frame itself we explicitly store in
> LE order:
>
>   static void target_setup_sve_record(struct target_sve_context *sve,
>                                       CPUARMState *env, int vq, int size)
>   {
>       int i, j;
>
>       __put_user(TARGET_SVE_MAGIC, &sve->head.magic);
>       __put_user(size, &sve->head.size);
>       __put_user(vq * TARGET_SVE_VQ_BYTES, &sve->vl);
>
>       /* Note that SVE regs are stored as a byte stream, with each byte element
>        * at a subsequent address.  This corresponds to a little-endian store
>        * of our 64-bit hunks.
>        */
>       for (i = 0; i < 32; ++i) {
>           uint64_t *z = (void *)sve + TARGET_SVE_SIG_ZREG_OFFSET(vq, i);
>           for (j = 0; j < vq * 2; ++j) {
>               __put_user_e(env->vfp.zregs[i].d[j], z + j, le);
>           }
>       }
>       for (i = 0; i <= 16; ++i) {
>           uint16_t *p = (void *)sve + TARGET_SVE_SIG_PREG_OFFSET(vq, i);
>           for (j = 0; j < vq; ++j) {
>               uint64_t r = env->vfp.pregs[i].p[j >> 2];
>               __put_user_e(r >> ((j & 3) * 16), p + j, le);
>           }
>       }
>   }
>
> For the aliased fpsimd registers we store in the target endian format:
>
>   static void target_setup_fpsimd_record(struct target_fpsimd_context *fpsimd,
>                                          CPUARMState *env)
>   {
>       int i;
>
>       __put_user(TARGET_FPSIMD_MAGIC, &fpsimd->head.magic);
>       __put_user(sizeof(struct target_fpsimd_context), &fpsimd->head.size);
>       __put_user(vfp_get_fpsr(env), &fpsimd->fpsr);
>       __put_user(vfp_get_fpcr(env), &fpsimd->fpcr);
>
>       for (i = 0; i < 32; i++) {
>           uint64_t *q = aa64_vfp_qreg(env, i);
>   #ifdef TARGET_WORDS_BIGENDIAN
>           __put_user(q[0], &fpsimd->vregs[i * 2 + 1]);
>           __put_user(q[1], &fpsimd->vregs[i * 2]);
>   #else
>           __put_user(q[0], &fpsimd->vregs[i * 2]);
>           __put_user(q[1], &fpsimd->vregs[i * 2 + 1]);
>   #endif
>       }
>   }

That's reassuring.

What about where you merge the fpsimd registers back into the SVE regs
on sigreturn?

> Where our layout for the quads is always:
>
>   Qn = regs[n].d[1]:regs[n].d[0]

You mean always on BE?  That would be wrong for LE.

Cheers
---Dave