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

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
      }
  }

Where our layout for the quads is always:

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



--
Alex Bennée