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

On Wed, Jun 12, 2019 at 02:18:29PM +0100, Alex Bennée wrote:
> 
> Dave Martin <[email protected]> writes:
> 
> > On Wed, Jun 12, 2019 at 11:40:11AM +0100, Alex Bennée wrote:
> >>
> >> Alan Hayward <[email protected]> writes:
> >>
> >> >> On 7 Jun 2019, at 16:48, Dave Martin <[email protected]> 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