[Qemu-devel] [PATCH v3 0/5] target/arm: linux-user changes for sve

[Qemu-devel] [PATCH v3 0/5] target/arm: linux-user changes for sve

[Richard Henderson]

Changes since v2:
  * 5 patches merged,
  * The PR_SVE_SET/GET_VL patch is more specifically user-only.
  * Split the signal frame patch into 4 parts.


r~


Richard Henderson (5):
  linux-user: Implement aarch64 PR_SVE_SET/GET_VL
  aarch64-linux-user: Split out helpers for guest signal handling
  aarch64-linux-user: Remove struct target_aux_context
  aarch64-linux-user: Add support for EXTRA signal frame records
  aarch64-linux-user: Add support for SVE signal frame records

 linux-user/aarch64/target_syscall.h |   3 +
 target/arm/cpu.h                    |   1 +
 linux-user/signal.c                 | 367 +++++++++++++++++++++++++++++-------
 linux-user/syscall.c                |  27 +++
 target/arm/cpu64.c                  |  41 ++++
 5 files changed, 372 insertions(+), 67 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v3 1/5] linux-user: Implement aarch64 PR_SVE_SET/GET_VL

[Richard Henderson]

As an implementation choice, widening VL has zeroed the
previously inaccessible portion of the sve registers.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/target_syscall.h |  3 +++
 target/arm/cpu.h                    |  1 +
 linux-user/syscall.c                | 27 ++++++++++++++++++++++++
 target/arm/cpu64.c                  | 41 +++++++++++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+)

diff --git a/linux-user/aarch64/target_syscall.h b/linux-user/aarch64/target_syscall.h
index 604ab99b14..205265e619 100644
--- a/linux-user/aarch64/target_syscall.h
+++ b/linux-user/aarch64/target_syscall.h
@@ -19,4 +19,7 @@ struct target_pt_regs {
 #define TARGET_MLOCKALL_MCL_CURRENT 1
 #define TARGET_MLOCKALL_MCL_FUTURE  2
 
+#define TARGET_PR_SVE_SET_VL  50
+#define TARGET_PR_SVE_GET_VL  51
+
 #endif /* AARCH64_TARGET_SYSCALL_H */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index de62df091c..f5fbb9b450 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -846,6 +846,7 @@ int arm_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
 #ifdef TARGET_AARCH64
 int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int aarch64_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq);
 #endif
 
 target_ulong do_arm_semihosting(CPUARMState *env);
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 82b35a6bdf..cf00ce10f1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -10659,6 +10659,33 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             break;
         }
 #endif
+#ifdef TARGET_AARCH64
+        case TARGET_PR_SVE_SET_VL:
+            /* We cannot support either PR_SVE_SET_VL_ONEXEC
+               or PR_SVE_VL_INHERIT.  Therefore, anything above
+               ARM_MAX_VQ results in EINVAL.  */
+            ret = -TARGET_EINVAL;
+            if (arm_feature(cpu_env, ARM_FEATURE_SVE)
+                && arg2 >= 0 && arg2 <= ARM_MAX_VQ * 16 && !(arg2 & 15)) {
+                CPUARMState *env = cpu_env;
+                int old_vq = (env->vfp.zcr_el[1] & 0xf) + 1;
+                int vq = MAX(arg2 / 16, 1);
+
+                if (vq < old_vq) {
+                    aarch64_sve_narrow_vq(env, vq);
+                }
+                env->vfp.zcr_el[1] = vq - 1;
+                ret = vq * 16;
+            }
+            break;
+        case TARGET_PR_SVE_GET_VL:
+            ret = -TARGET_EINVAL;
+            if (arm_feature(cpu_env, ARM_FEATURE_SVE)) {
+                CPUARMState *env = cpu_env;
+                ret = ((env->vfp.zcr_el[1] & 0xf) + 1) * 16;
+            }
+            break;
+#endif /* AARCH64 */
         case PR_GET_SECCOMP:
         case PR_SET_SECCOMP:
             /* Disable seccomp to prevent the target disabling syscalls we
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 1c330adc28..a0a81014b2 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -363,3 +363,44 @@ static void aarch64_cpu_register_types(void)
 }
 
 type_init(aarch64_cpu_register_types)
+
+/* The manual says that when SVE is enabled and VQ is widened the
+ * implementation is allowed to zero the previously inaccessible
+ * portion of the registers.  The corollary to that is that when
+ * SVE is enabled and VQ is narrowed we are also allowed to zero
+ * the now inaccessible portion of the registers.
+ *
+ * The intent of this is that no predicate bit beyond VQ is ever set.
+ * Which means that some operations on predicate registers themselves
+ * may operate on full uint64_t or even unrolled across the maximum
+ * uint64_t[4].  Performing 4 bits of host arithmetic unconditionally
+ * may well be cheaper than conditionals to restrict the operation
+ * to the relevant portion of a uint16_t[16].
+ *
+ * TODO: Need to call this for changes to the real system registers
+ * and EL state changes.
+ */
+void aarch64_sve_narrow_vq(CPUARMState *env, unsigned vq)
+{
+    int i, j;
+    uint64_t pmask;
+
+    assert(vq >= 1 && vq <= ARM_MAX_VQ);
+
+    /* Zap the high bits of the zregs.  */
+    for (i = 0; i < 32; i++) {
+        memset(&env->vfp.zregs[i].d[2 * vq], 0, 16 * (ARM_MAX_VQ - vq));
+    }
+
+    /* Zap the high bits of the pregs and ffr.  */
+    pmask = 0;
+    if (vq & 3) {
+        pmask = ~(-1ULL << (16 * (vq & 3)));
+    }
+    for (j = vq / 4; j < ARM_MAX_VQ / 4; j++) {
+        for (i = 0; i < 17; ++i) {
+            env->vfp.pregs[i].p[j] &= pmask;
+        }
+        pmask = 0;
+    }
+}
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 1/5] linux-user: Implement aarch64 PR_SVE_SET/GET_VL

[Peter Maydell]

On 16 February 2018 at 21:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> As an implementation choice, widening VL has zeroed the
> previously inaccessible portion of the sve registers.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/target_syscall.h |  3 +++
>  target/arm/cpu.h                    |  1 +
>  linux-user/syscall.c                | 27 ++++++++++++++++++++++++
>  target/arm/cpu64.c                  | 41 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 2/5] aarch64-linux-user: Split out helpers for guest signal handling

[Richard Henderson]

Split out helpers from target_setup_frame and target_restore_sigframe
for dealing with general registers, fpsimd registers, and the end record.

When we add support for sve registers, the relative positions of
these will change.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 120 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 69 insertions(+), 51 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 9a380b9e31..25c9743aed 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1462,16 +1462,17 @@ struct target_rt_sigframe {
     uint32_t tramp[2];
 };
 
-static int target_setup_sigframe(struct target_rt_sigframe *sf,
-                                 CPUARMState *env, target_sigset_t *set)
+static void target_setup_general_frame(struct target_rt_sigframe *sf,
+                                       CPUARMState *env, target_sigset_t *set)
 {
     int i;
-    struct target_aux_context *aux =
-        (struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
 
-    /* set up the stack frame for unwinding */
-    __put_user(env->xregs[29], &sf->fp);
-    __put_user(env->xregs[30], &sf->lr);
+    __put_user(0, &sf->uc.tuc_flags);
+    __put_user(0, &sf->uc.tuc_link);
+
+    __put_user(target_sigaltstack_used.ss_sp, &sf->uc.tuc_stack.ss_sp);
+    __put_user(sas_ss_flags(env->xregs[31]), &sf->uc.tuc_stack.ss_flags);
+    __put_user(target_sigaltstack_used.ss_size, &sf->uc.tuc_stack.ss_size);
 
     for (i = 0; i < 31; i++) {
         __put_user(env->xregs[i], &sf->uc.tuc_mcontext.regs[i]);
@@ -1485,39 +1486,42 @@ static int target_setup_sigframe(struct target_rt_sigframe *sf,
     for (i = 0; i < TARGET_NSIG_WORDS; i++) {
         __put_user(set->sig[i], &sf->uc.tuc_sigmask.sig[i]);
     }
+}
+
+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], &aux->fpsimd.vregs[i * 2 + 1]);
-        __put_user(q[1], &aux->fpsimd.vregs[i * 2]);
+        __put_user(q[0], &fpsimd->vregs[i * 2 + 1]);
+        __put_user(q[1], &fpsimd->vregs[i * 2]);
 #else
-        __put_user(q[0], &aux->fpsimd.vregs[i * 2]);
-        __put_user(q[1], &aux->fpsimd.vregs[i * 2 + 1]);
+        __put_user(q[0], &fpsimd->vregs[i * 2]);
+        __put_user(q[1], &fpsimd->vregs[i * 2 + 1]);
 #endif
     }
-    __put_user(vfp_get_fpsr(env), &aux->fpsimd.fpsr);
-    __put_user(vfp_get_fpcr(env), &aux->fpsimd.fpcr);
-    __put_user(TARGET_FPSIMD_MAGIC, &aux->fpsimd.head.magic);
-    __put_user(sizeof(struct target_fpsimd_context),
-            &aux->fpsimd.head.size);
-
-    /* set the "end" magic */
-    __put_user(0, &aux->end.magic);
-    __put_user(0, &aux->end.size);
-
-    return 0;
 }
 
-static int target_restore_sigframe(CPUARMState *env,
-                                   struct target_rt_sigframe *sf)
+static void target_setup_end_record(struct target_aarch64_ctx *end)
+{
+    __put_user(0, &end->magic);
+    __put_user(0, &end->size);
+}
+
+static void target_restore_general_frame(CPUARMState *env,
+                                         struct target_rt_sigframe *sf)
 {
     sigset_t set;
-    int i;
-    struct target_aux_context *aux =
-        (struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
-    uint32_t magic, size, fpsr, fpcr;
     uint64_t pstate;
+    int i;
 
     target_to_host_sigset(&set, &sf->uc.tuc_sigmask);
     set_sigmask(&set);
@@ -1530,30 +1534,48 @@ static int target_restore_sigframe(CPUARMState *env,
     __get_user(env->pc, &sf->uc.tuc_mcontext.pc);
     __get_user(pstate, &sf->uc.tuc_mcontext.pstate);
     pstate_write(env, pstate);
+}
 
-    __get_user(magic, &aux->fpsimd.head.magic);
-    __get_user(size, &aux->fpsimd.head.size);
+static void target_restore_fpsimd_record(CPUARMState *env,
+                                         struct target_fpsimd_context *fpsimd)
+{
+    uint32_t fpsr, fpcr;
+    int i;
 
-    if (magic != TARGET_FPSIMD_MAGIC
-        || size != sizeof(struct target_fpsimd_context)) {
-        return 1;
-    }
+    __get_user(fpsr, &fpsimd->fpsr);
+    vfp_set_fpsr(env, fpsr);
+    __get_user(fpcr, &fpsimd->fpcr);
+    vfp_set_fpcr(env, fpcr);
 
     for (i = 0; i < 32; i++) {
         uint64_t *q = aa64_vfp_qreg(env, i);
 #ifdef TARGET_WORDS_BIGENDIAN
-        __get_user(q[0], &aux->fpsimd.vregs[i * 2 + 1]);
-        __get_user(q[1], &aux->fpsimd.vregs[i * 2]);
+        __get_user(q[0], &fpsimd->vregs[i * 2 + 1]);
+        __get_user(q[1], &fpsimd->vregs[i * 2]);
 #else
-        __get_user(q[0], &aux->fpsimd.vregs[i * 2]);
-        __get_user(q[1], &aux->fpsimd.vregs[i * 2 + 1]);
+        __get_user(q[0], &fpsimd->vregs[i * 2]);
+        __get_user(q[1], &fpsimd->vregs[i * 2 + 1]);
 #endif
     }
-    __get_user(fpsr, &aux->fpsimd.fpsr);
-    vfp_set_fpsr(env, fpsr);
-    __get_user(fpcr, &aux->fpsimd.fpcr);
-    vfp_set_fpcr(env, fpcr);
+}
 
+static int target_restore_sigframe(CPUARMState *env,
+                                   struct target_rt_sigframe *sf)
+{
+    struct target_aux_context *aux
+        = (struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
+    uint32_t magic, size;
+
+    target_restore_general_frame(env, sf);
+
+    __get_user(magic, &aux->fpsimd.head.magic);
+    __get_user(size, &aux->fpsimd.head.size);
+    if (magic == TARGET_FPSIMD_MAGIC
+        && size == sizeof(struct target_fpsimd_context)) {
+        target_restore_fpsimd_record(env, &aux->fpsimd);
+    } else {
+        return 1;
+    }
     return 0;
 }
 
@@ -1580,6 +1602,7 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                CPUARMState *env)
 {
     struct target_rt_sigframe *frame;
+    struct target_aux_context *aux;
     abi_ulong frame_addr, return_addr;
 
     frame_addr = get_sigframe(ka, env);
@@ -1587,17 +1610,12 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
         goto give_sigsegv;
     }
+    aux = (struct target_aux_context *)frame->uc.tuc_mcontext.__reserved;
 
-    __put_user(0, &frame->uc.tuc_flags);
-    __put_user(0, &frame->uc.tuc_link);
+    target_setup_general_frame(frame, env, set);
+    target_setup_fpsimd_record(&aux->fpsimd, env);
+    target_setup_end_record(&aux->end);
 
-    __put_user(target_sigaltstack_used.ss_sp,
-                      &frame->uc.tuc_stack.ss_sp);
-    __put_user(sas_ss_flags(env->xregs[31]),
-                      &frame->uc.tuc_stack.ss_flags);
-    __put_user(target_sigaltstack_used.ss_size,
-                      &frame->uc.tuc_stack.ss_size);
-    target_setup_sigframe(frame, env, set);
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         return_addr = ka->sa_restorer;
     } else {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 2/5] aarch64-linux-user: Split out helpers for guest signal handling

[Peter Maydell]

On 16 February 2018 at 21:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Split out helpers from target_setup_frame and target_restore_sigframe
> for dealing with general registers, fpsimd registers, and the end record.
>
> When we add support for sve registers, the relative positions of
> these will change.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 3/5] aarch64-linux-user: Remove struct target_aux_context

[Richard Henderson]

This changes the qemu signal frame layout to be more like the kernel's,
in that the various records are dynamically allocated rather than fixed
in place by a structure.

For now, all of the allocation is out of uc.tuc_mcontext.__reserved,
so the allocation is actually trivial.  That will change with SVE support.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 89 ++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 28 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 25c9743aed..f9eef3d753 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1443,20 +1443,12 @@ struct target_fpsimd_context {
     uint64_t vregs[32 * 2]; /* really uint128_t vregs[32] */
 };
 
-/*
- * Auxiliary context saved in the sigcontext.__reserved array. Not exported to
- * user space as it will change with the addition of new context. User space
- * should check the magic/size information.
- */
-struct target_aux_context {
-    struct target_fpsimd_context fpsimd;
-    /* additional context to be added before "end" */
-    struct target_aarch64_ctx end;
-};
-
 struct target_rt_sigframe {
     struct target_siginfo info;
     struct target_ucontext uc;
+};
+
+struct target_rt_frame_record {
     uint64_t fp;
     uint64_t lr;
     uint32_t tramp[2];
@@ -1562,20 +1554,47 @@ static void target_restore_fpsimd_record(CPUARMState *env,
 static int target_restore_sigframe(CPUARMState *env,
                                    struct target_rt_sigframe *sf)
 {
-    struct target_aux_context *aux
-        = (struct target_aux_context *)sf->uc.tuc_mcontext.__reserved;
-    uint32_t magic, size;
+    struct target_aarch64_ctx *ctx;
+    struct target_fpsimd_context *fpsimd = NULL;
 
     target_restore_general_frame(env, sf);
 
-    __get_user(magic, &aux->fpsimd.head.magic);
-    __get_user(size, &aux->fpsimd.head.size);
-    if (magic == TARGET_FPSIMD_MAGIC
-        && size == sizeof(struct target_fpsimd_context)) {
-        target_restore_fpsimd_record(env, &aux->fpsimd);
-    } else {
+    ctx = (struct target_aarch64_ctx *)sf->uc.tuc_mcontext.__reserved;
+    while (ctx) {
+        uint32_t magic, size;
+
+        __get_user(magic, &ctx->magic);
+        __get_user(size, &ctx->size);
+        switch (magic) {
+        case 0:
+            if (size != 0) {
+                return 1;
+            }
+            ctx = NULL;
+            continue;
+
+        case TARGET_FPSIMD_MAGIC:
+            if (fpsimd || size != sizeof(struct target_fpsimd_context)) {
+                return 1;
+            }
+            fpsimd = (struct target_fpsimd_context *)ctx;
+            break;
+
+        default:
+            /* Unknown record -- we certainly didn't generate it.
+             * Did we in fact get out of sync?
+             */
+            return 1;
+        }
+        ctx = (void *)ctx + size;
+    }
+
+    /* Require FPSIMD always.  */
+    if (!fpsimd) {
         return 1;
     }
+    target_restore_fpsimd_record(env, fpsimd);
+
     return 0;
 }
 
@@ -1601,20 +1620,33 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                target_siginfo_t *info, target_sigset_t *set,
                                CPUARMState *env)
 {
+    int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
+    int fpsimd_ofs, end1_ofs, fr_ofs;
     struct target_rt_sigframe *frame;
-    struct target_aux_context *aux;
+    struct target_rt_frame_record *fr;
     abi_ulong frame_addr, return_addr;
 
+    fpsimd_ofs = size;
+    size += sizeof(struct target_fpsimd_context);
+    end1_ofs = size;
+    size += sizeof(struct target_aarch64_ctx);
+    fr_ofs = size;
+    size += sizeof(struct target_rt_frame_record);
+
     frame_addr = get_sigframe(ka, env);
     trace_user_setup_frame(env, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
         goto give_sigsegv;
     }
-    aux = (struct target_aux_context *)frame->uc.tuc_mcontext.__reserved;
 
     target_setup_general_frame(frame, env, set);
-    target_setup_fpsimd_record(&aux->fpsimd, env);
-    target_setup_end_record(&aux->end);
+    target_setup_fpsimd_record((void *)frame + fpsimd_ofs, env);
+    target_setup_end_record((void *)frame + end1_ofs);
+
+    /* Set up the stack frame for unwinding.  */
+    fr = (void *)frame + fr_ofs;
+    __put_user(env->xregs[29], &fr->fp);
+    __put_user(env->xregs[30], &fr->lr);
 
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         return_addr = ka->sa_restorer;
@@ -1624,13 +1656,14 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
          * Since these are instructions they need to be put as little-endian
          * regardless of target default or current CPU endianness.
          */
-        __put_user_e(0xd2801168, &frame->tramp[0], le);
-        __put_user_e(0xd4000001, &frame->tramp[1], le);
-        return_addr = frame_addr + offsetof(struct target_rt_sigframe, tramp);
+        __put_user_e(0xd2801168, &fr->tramp[0], le);
+        __put_user_e(0xd4000001, &fr->tramp[1], le);
+        return_addr = frame_addr + fr_ofs
+            + offsetof(struct target_rt_frame_record, tramp);
     }
     env->xregs[0] = usig;
     env->xregs[31] = frame_addr;
-    env->xregs[29] = env->xregs[31] + offsetof(struct target_rt_sigframe, fp);
+    env->xregs[29] = frame_addr + fr_ofs;
     env->pc = ka->_sa_handler;
     env->xregs[30] = return_addr;
     if (info) {
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 3/5] aarch64-linux-user: Remove struct target_aux_context

[Peter Maydell]

On 16 February 2018 at 21:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> This changes the qemu signal frame layout to be more like the kernel's,
> in that the various records are dynamically allocated rather than fixed
> in place by a structure.
>
> For now, all of the allocation is out of uc.tuc_mcontext.__reserved,
> so the allocation is actually trivial.  That will change with SVE support.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 4/5] aarch64-linux-user: Add support for EXTRA signal frame records

[Richard Henderson]

The EXTRA record allows for additional space to be allocated
beyon what is currently reserved.  Add code to emit and read
this record type.

Nothing uses extra space yet.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index f9eef3d753..ca0ba28c98 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1443,6 +1443,15 @@ struct target_fpsimd_context {
     uint64_t vregs[32 * 2]; /* really uint128_t vregs[32] */
 };
 
+#define TARGET_EXTRA_MAGIC  0x45585401
+
+struct target_extra_context {
+    struct target_aarch64_ctx head;
+    uint64_t datap; /* 16-byte aligned pointer to extra space cast to __u64 */
+    uint32_t size; /* size in bytes of the extra space */
+    uint32_t reserved[3];
+};
+
 struct target_rt_sigframe {
     struct target_siginfo info;
     struct target_ucontext uc;
@@ -1502,6 +1511,15 @@ static void target_setup_fpsimd_record(struct target_fpsimd_context *fpsimd,
     }
 }
 
+static void target_setup_extra_record(struct target_extra_context *extra,
+                                      uint64_t datap, uint32_t extra_size)
+{
+    __put_user(TARGET_EXTRA_MAGIC, &extra->head.magic);
+    __put_user(sizeof(struct target_extra_context), &extra->head.size);
+    __put_user(datap, &extra->datap);
+    __put_user(extra_size, &extra->size);
+}
+
 static void target_setup_end_record(struct target_aarch64_ctx *end)
 {
     __put_user(0, &end->magic);
@@ -1554,14 +1572,16 @@ static void target_restore_fpsimd_record(CPUARMState *env,
 static int target_restore_sigframe(CPUARMState *env,
                                    struct target_rt_sigframe *sf)
 {
-    struct target_aarch64_ctx *ctx;
+    struct target_aarch64_ctx *ctx, *extra = NULL;
     struct target_fpsimd_context *fpsimd = NULL;
+    uint64_t extra_datap = 0;
+    bool used_extra = false;
 
     target_restore_general_frame(env, sf);
 
     ctx = (struct target_aarch64_ctx *)sf->uc.tuc_mcontext.__reserved;
     while (ctx) {
-        uint32_t magic, size;
+        uint32_t magic, size, extra_size;
 
         __get_user(magic, &ctx->magic);
         __get_user(size, &ctx->size);
@@ -1570,7 +1590,12 @@ static int target_restore_sigframe(CPUARMState *env,
             if (size != 0) {
                 return 1;
             }
-            ctx = NULL;
+            if (used_extra) {
+                ctx = NULL;
+            } else {
+                ctx = extra;
+                used_extra = true;
+            }
             continue;
 
         case TARGET_FPSIMD_MAGIC:
@@ -1580,6 +1605,17 @@ static int target_restore_sigframe(CPUARMState *env,
             fpsimd = (struct target_fpsimd_context *)ctx;
             break;
 
+        case TARGET_EXTRA_MAGIC:
+            if (extra || size != sizeof(struct target_extra_context)) {
+                return 1;
+            }
+            __get_user(extra_datap,
+                       &((struct target_extra_context *)ctx)->datap);
+            __get_user(extra_size,
+                       &((struct target_extra_context *)ctx)->size);
+            extra = lock_user(VERIFY_READ, extra_datap, extra_size, 0);
+            break;
+
         default:
             /* Unknown record -- we certainly didn't generate it.
              * Did we in fact get out of sync?
@@ -1595,6 +1631,9 @@ static int target_restore_sigframe(CPUARMState *env,
     }
     target_restore_fpsimd_record(env, fpsimd);
 
+    if (extra) {
+        unlock_user(extra, extra_datap, 0);
+    }
     return 0;
 }
 
@@ -1621,7 +1660,8 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                CPUARMState *env)
 {
     int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
-    int fpsimd_ofs, end1_ofs, fr_ofs;
+    int fpsimd_ofs, end1_ofs, fr_ofs, end2_ofs = 0;
+    int extra_ofs = 0, extra_base = 0, extra_size = 0;
     struct target_rt_sigframe *frame;
     struct target_rt_frame_record *fr;
     abi_ulong frame_addr, return_addr;
@@ -1641,7 +1681,14 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
 
     target_setup_general_frame(frame, env, set);
     target_setup_fpsimd_record((void *)frame + fpsimd_ofs, env);
+    if (extra_ofs) {
+        target_setup_extra_record((void *)frame + extra_ofs,
+                                  frame_addr + extra_base, extra_size);
+    }
     target_setup_end_record((void *)frame + end1_ofs);
+    if (end2_ofs) {
+        target_setup_end_record((void *)frame + end2_ofs);
+    }
 
     /* Set up the stack frame for unwinding.  */
     fr = (void *)frame + fr_ofs;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 4/5] aarch64-linux-user: Add support for EXTRA signal frame records

[Peter Maydell]

On 16 February 2018 at 21:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> The EXTRA record allows for additional space to be allocated
> beyon what is currently reserved.  Add code to emit and read
> this record type.
>
> Nothing uses extra space yet.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

> @@ -1554,14 +1572,16 @@ static void target_restore_fpsimd_record(CPUARMState *env,
>  static int target_restore_sigframe(CPUARMState *env,
>                                     struct target_rt_sigframe *sf)
>  {
> -    struct target_aarch64_ctx *ctx;
> +    struct target_aarch64_ctx *ctx, *extra = NULL;
>      struct target_fpsimd_context *fpsimd = NULL;
> +    uint64_t extra_datap = 0;
> +    bool used_extra = false;
>
>      target_restore_general_frame(env, sf);
>
>      ctx = (struct target_aarch64_ctx *)sf->uc.tuc_mcontext.__reserved;
>      while (ctx) {
> -        uint32_t magic, size;
> +        uint32_t magic, size, extra_size;
>
>          __get_user(magic, &ctx->magic);
>          __get_user(size, &ctx->size);
> @@ -1570,7 +1590,12 @@ static int target_restore_sigframe(CPUARMState *env,
>              if (size != 0) {
>                  return 1;
>              }
> -            ctx = NULL;
> +            if (used_extra) {
> +                ctx = NULL;
> +            } else {
> +                ctx = extra;
> +                used_extra = true;
> +            }
>              continue;
>
>          case TARGET_FPSIMD_MAGIC:
> @@ -1580,6 +1605,17 @@ static int target_restore_sigframe(CPUARMState *env,
>              fpsimd = (struct target_fpsimd_context *)ctx;
>              break;
>
> +        case TARGET_EXTRA_MAGIC:
> +            if (extra || size != sizeof(struct target_extra_context)) {
> +                return 1;
> +            }
> +            __get_user(extra_datap,
> +                       &((struct target_extra_context *)ctx)->datap);
> +            __get_user(extra_size,
> +                       &((struct target_extra_context *)ctx)->size);
> +            extra = lock_user(VERIFY_READ, extra_datap, extra_size, 0);
> +            break;
> +
>          default:
>              /* Unknown record -- we certainly didn't generate it.
>               * Did we in fact get out of sync?
> @@ -1595,6 +1631,9 @@ static int target_restore_sigframe(CPUARMState *env,
>      }
>      target_restore_fpsimd_record(env, fpsimd);
>
> +    if (extra) {
> +        unlock_user(extra, extra_datap, 0);
> +    }

This will fail to call unlock_user if the function returns early
(eg because of failed magic-number checks or the FPSIMD record
not being present).

You don't need the "if (extra)" check -- unlock_user() is
specified to do nothing if passed a NULL host_ptr.

Otherwise looks good.

thanks
-- PMM

[Qemu-devel] [PATCH v3 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Richard Henderson]

Depending on the currently selected size of the SVE vector registers,
we can either store the data within the "standard" allocation, or we
may beedn to allocate additional space with an EXTRA record.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/signal.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 138 insertions(+), 3 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index ca0ba28c98..4c9fef4bb2 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1452,6 +1452,30 @@ struct target_extra_context {
     uint32_t reserved[3];
 };
 
+#define TARGET_SVE_MAGIC    0x53564501
+
+struct target_sve_context {
+    struct target_aarch64_ctx head;
+    uint16_t vl;
+    uint16_t reserved[3];
+};
+
+#define TARGET_SVE_VQ_BYTES  16
+
+#define TARGET_SVE_SIG_ZREG_SIZE(VQ)  ((VQ) * TARGET_SVE_VQ_BYTES)
+#define TARGET_SVE_SIG_PREG_SIZE(VQ)  ((VQ) * (TARGET_SVE_VQ_BYTES / 8))
+
+#define TARGET_SVE_SIG_REGS_OFFSET \
+    QEMU_ALIGN_UP(sizeof(struct target_sve_context), TARGET_SVE_VQ_BYTES)
+#define TARGET_SVE_SIG_ZREG_OFFSET(VQ, N) \
+    (TARGET_SVE_SIG_REGS_OFFSET + TARGET_SVE_SIG_ZREG_SIZE(VQ) * (N))
+#define TARGET_SVE_SIG_PREG_OFFSET(VQ, N) \
+    (TARGET_SVE_SIG_ZREG_OFFSET(VQ, 32) + TARGET_SVE_SIG_PREG_SIZE(VQ) * (N))
+#define TARGET_SVE_SIG_FFR_OFFSET(VQ) \
+    (TARGET_SVE_SIG_PREG_OFFSET(VQ, 16))
+#define TARGET_SVE_SIG_CONTEXT_SIZE(VQ) \
+    (TARGET_SVE_SIG_PREG_OFFSET(VQ, 17))
+
 struct target_rt_sigframe {
     struct target_siginfo info;
     struct target_ucontext uc;
@@ -1526,6 +1550,34 @@ static void target_setup_end_record(struct target_aarch64_ctx *end)
     __put_user(0, &end->size);
 }
 
+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);
+        }
+    }
+}
+
 static void target_restore_general_frame(CPUARMState *env,
                                          struct target_rt_sigframe *sf)
 {
@@ -1569,13 +1621,44 @@ static void target_restore_fpsimd_record(CPUARMState *env,
     }
 }
 
+static void target_restore_sve_record(CPUARMState *env,
+                                      struct target_sve_context *sve, int vq)
+{
+    int i, j;
+
+    /* 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) {
+            __get_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) {
+            uint16_t r;
+            __get_user_e(r, p + j, le);
+            if (j & 3) {
+                env->vfp.pregs[i].p[j >> 2] |= (uint64_t)r << ((j & 3) * 16);
+            } else {
+                env->vfp.pregs[i].p[j >> 2] = r;
+            }
+        }
+    }
+}
+
 static int target_restore_sigframe(CPUARMState *env,
                                    struct target_rt_sigframe *sf)
 {
     struct target_aarch64_ctx *ctx, *extra = NULL;
     struct target_fpsimd_context *fpsimd = NULL;
+    struct target_sve_context *sve = NULL;
     uint64_t extra_datap = 0;
     bool used_extra = false;
+    int vq = 0, sve_size = 0;
 
     target_restore_general_frame(env, sf);
 
@@ -1605,6 +1688,17 @@ static int target_restore_sigframe(CPUARMState *env,
             fpsimd = (struct target_fpsimd_context *)ctx;
             break;
 
+        case TARGET_SVE_MAGIC:
+            if (arm_feature(env, ARM_FEATURE_SVE)) {
+                vq = (env->vfp.zcr_el[1] & 0xf) + 1;
+                sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
+                if (!sve && size == sve_size) {
+                    sve = (struct target_sve_context *)ctx;
+                    break;
+                }
+            }
+            return 1;
+
         case TARGET_EXTRA_MAGIC:
             if (extra || size != sizeof(struct target_extra_context)) {
                 return 1;
@@ -1631,13 +1725,19 @@ static int target_restore_sigframe(CPUARMState *env,
     }
     target_restore_fpsimd_record(env, fpsimd);
 
+    /* SVE data, if present, overwrites FPSIMD data.  */
+    if (sve) {
+        target_restore_sve_record(env, sve, vq);
+    }
+
     if (extra) {
         unlock_user(extra, extra_datap, 0);
     }
     return 0;
 }
 
-static abi_ulong get_sigframe(struct target_sigaction *ka, CPUARMState *env)
+static abi_ulong get_sigframe(struct target_sigaction *ka,
+                              CPUARMState *env, int size)
 {
     abi_ulong sp;
 
@@ -1650,7 +1750,7 @@ static abi_ulong get_sigframe(struct target_sigaction *ka, CPUARMState *env)
         sp = target_sigaltstack_used.ss_sp + target_sigaltstack_used.ss_size;
     }
 
-    sp = (sp - sizeof(struct target_rt_sigframe)) & ~15;
+    sp = (sp - size) & ~15;
 
     return sp;
 }
@@ -1659,21 +1759,53 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                target_siginfo_t *info, target_sigset_t *set,
                                CPUARMState *env)
 {
+    int std_size = sizeof(struct target_rt_sigframe);
     int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
     int fpsimd_ofs, end1_ofs, fr_ofs, end2_ofs = 0;
     int extra_ofs = 0, extra_base = 0, extra_size = 0;
+    int sve_ofs = 0, vq = 0, sve_size = 0;
     struct target_rt_sigframe *frame;
     struct target_rt_frame_record *fr;
     abi_ulong frame_addr, return_addr;
 
+    /* Reserve space for standard exit marker.  */
+    std_size -= sizeof(struct target_aarch64_ctx);
+
+    /* FPSIMD record is always in the standard space.  */
     fpsimd_ofs = size;
     size += sizeof(struct target_fpsimd_context);
     end1_ofs = size;
+
+    /* SVE state needs saving only if it exists.  */
+    if (arm_feature(env, ARM_FEATURE_SVE)) {
+        vq = (env->vfp.zcr_el[1] & 0xf) + 1;
+        sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
+
+        /* For VQ <= 6, there is room in the standard space.  */
+        if (sve_size <= std_size) {
+            sve_ofs = size;
+            size += sve_size;
+            end1_ofs = size;
+        } else {
+            /* Otherwise we need to allocate extra space.  */
+            extra_ofs = size;
+            size += sizeof(struct target_extra_context);
+            end1_ofs = size;
+            size += QEMU_ALIGN_UP(sizeof(struct target_aarch64_ctx), 16);
+            extra_base = size;
+            extra_size = sve_size + sizeof(struct target_aarch64_ctx);
+
+            sve_ofs = size;
+            size += sve_size;
+            end2_ofs = size;
+        }
+    }
     size += sizeof(struct target_aarch64_ctx);
+
     fr_ofs = size;
     size += sizeof(struct target_rt_frame_record);
 
-    frame_addr = get_sigframe(ka, env);
+    frame_addr = get_sigframe(ka, env, size);
     trace_user_setup_frame(env, frame_addr);
     if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
         goto give_sigsegv;
@@ -1686,6 +1818,9 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
                                   frame_addr + extra_base, extra_size);
     }
     target_setup_end_record((void *)frame + end1_ofs);
+    if (sve_ofs) {
+        target_setup_sve_record((void *)frame + sve_ofs, env, vq, sve_size);
+    }
     if (end2_ofs) {
         target_setup_end_record((void *)frame + end2_ofs);
     }
-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Peter Maydell]

On 16 February 2018 at 21:56, Richard Henderson
<richard.henderson@linaro.org> wrote:
> Depending on the currently selected size of the SVE vector registers,
> we can either store the data within the "standard" allocation, or we
> may beedn to allocate additional space with an EXTRA record.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/signal.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 138 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index ca0ba28c98..4c9fef4bb2 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1452,6 +1452,30 @@ struct target_extra_context {
>      uint32_t reserved[3];
>  };
>
> +#define TARGET_SVE_MAGIC    0x53564501
> +
> +struct target_sve_context {
> +    struct target_aarch64_ctx head;
> +    uint16_t vl;
> +    uint16_t reserved[3];

Worth commenting that actual SVE register data will directly follow the struct.


> +static void target_restore_sve_record(CPUARMState *env,
> +                                      struct target_sve_context *sve, int vq)
> +{
> +    int i, j;
> +
> +    /* 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.

We're doing loads in this function, not stores.

> +     */
> +    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) {
> +            __get_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) {
> +            uint16_t r;
> +            __get_user_e(r, p + j, le);
> +            if (j & 3) {
> +                env->vfp.pregs[i].p[j >> 2] |= (uint64_t)r << ((j & 3) * 16);
> +            } else {
> +                env->vfp.pregs[i].p[j >> 2] = r;
> +            }
> +        }
> +    }
> +}
> +

> @@ -1659,21 +1759,53 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
>                                 target_siginfo_t *info, target_sigset_t *set,
>                                 CPUARMState *env)
>  {
> +    int std_size = sizeof(struct target_rt_sigframe);
>      int size = offsetof(struct target_rt_sigframe, uc.tuc_mcontext.__reserved);
>      int fpsimd_ofs, end1_ofs, fr_ofs, end2_ofs = 0;
>      int extra_ofs = 0, extra_base = 0, extra_size = 0;
> +    int sve_ofs = 0, vq = 0, sve_size = 0;
>      struct target_rt_sigframe *frame;
>      struct target_rt_frame_record *fr;
>      abi_ulong frame_addr, return_addr;
>
> +    /* Reserve space for standard exit marker.  */
> +    std_size -= sizeof(struct target_aarch64_ctx);
> +
> +    /* FPSIMD record is always in the standard space.  */
>      fpsimd_ofs = size;
>      size += sizeof(struct target_fpsimd_context);
>      end1_ofs = size;
> +
> +    /* SVE state needs saving only if it exists.  */
> +    if (arm_feature(env, ARM_FEATURE_SVE)) {
> +        vq = (env->vfp.zcr_el[1] & 0xf) + 1;
> +        sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
> +
> +        /* For VQ <= 6, there is room in the standard space.  */

The kernel header arch/arm64/include/uapi/asm/sigcontext.h
claims the record is in the standard space if "vl <= 64", which
doesn't seem to match up with "VQ <= 6" ?

> +        if (sve_size <= std_size) {
> +            sve_ofs = size;
> +            size += sve_size;
> +            end1_ofs = size;
> +        } else {
> +            /* Otherwise we need to allocate extra space.  */
> +            extra_ofs = size;
> +            size += sizeof(struct target_extra_context);
> +            end1_ofs = size;
> +            size += QEMU_ALIGN_UP(sizeof(struct target_aarch64_ctx), 16);

Why do we add the size of target_aarch64_ctx to size here?
We already account for the size of the end record later, so
what is this one?

> +            extra_base = size;
> +            extra_size = sve_size + sizeof(struct target_aarch64_ctx);

If we ever get two different kinds of optional record that need to
live in the extra space, this is going to need refactoring,
because at the moment it assumes that the SVE record is the first
and only thing that might live there. I guess that's OK for now, though.

> +
> +            sve_ofs = size;
> +            size += sve_size;
> +            end2_ofs = size;
> +        }
> +    }
>      size += sizeof(struct target_aarch64_ctx);
> +
>      fr_ofs = size;
>      size += sizeof(struct target_rt_frame_record);
>
> -    frame_addr = get_sigframe(ka, env);
> +    frame_addr = get_sigframe(ka, env, size);
>      trace_user_setup_frame(env, frame_addr);
>      if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0)) {
>          goto give_sigsegv;
> @@ -1686,6 +1818,9 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
>                                    frame_addr + extra_base, extra_size);
>      }
>      target_setup_end_record((void *)frame + end1_ofs);
> +    if (sve_ofs) {
> +        target_setup_sve_record((void *)frame + sve_ofs, env, vq, sve_size);
> +    }
>      if (end2_ofs) {
>          target_setup_end_record((void *)frame + end2_ofs);
>      }
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Richard Henderson]

On 02/22/2018 08:41 AM, Peter Maydell wrote:
> On 16 February 2018 at 21:56, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> Depending on the currently selected size of the SVE vector registers,
>> we can either store the data within the "standard" allocation, or we
>> may beedn to allocate additional space with an EXTRA record.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  linux-user/signal.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 138 insertions(+), 3 deletions(-)
>>
>> diff --git a/linux-user/signal.c b/linux-user/signal.c
>> index ca0ba28c98..4c9fef4bb2 100644
>> --- a/linux-user/signal.c
>> +++ b/linux-user/signal.c
>> @@ -1452,6 +1452,30 @@ struct target_extra_context {
>>      uint32_t reserved[3];
>>  };
>>
>> +#define TARGET_SVE_MAGIC    0x53564501
>> +
>> +struct target_sve_context {
>> +    struct target_aarch64_ctx head;
>> +    uint16_t vl;
>> +    uint16_t reserved[3];
> 
> Worth commenting that actual SVE register data will directly follow the struct.

Sure.

>> +static void target_restore_sve_record(CPUARMState *env,
>> +                                      struct target_sve_context *sve, int vq)
>> +{
>> +    int i, j;
>> +
>> +    /* 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.
> 
> We're doing loads in this function, not stores.

Oops.

>> +    /* SVE state needs saving only if it exists.  */
>> +    if (arm_feature(env, ARM_FEATURE_SVE)) {
>> +        vq = (env->vfp.zcr_el[1] & 0xf) + 1;
>> +        sve_size = QEMU_ALIGN_UP(TARGET_SVE_SIG_CONTEXT_SIZE(vq), 16);
>> +
>> +        /* For VQ <= 6, there is room in the standard space.  */
> 
> The kernel header arch/arm64/include/uapi/asm/sigcontext.h
> claims the record is in the standard space if "vl <= 64", which
> doesn't seem to match up with "VQ <= 6" ?

*shrug* I suppose the kernel guys only considered sizes that are powers of two,
even though other sizes are clearly allowed by the implementation?

The 4096 reserved bytes - sizeof(fpsimd) - sizeof(end) = 3560 bytes.
Values for TARGET_SVE_SIG_CONTEXT_SIZE(vq) are:
1	562
2	1108
3	1654
4	2200
5	2746
6	3292
7	3838
8	4384

So there's definitely room for VQ=6, with 268 bytes left over.

> 
>> +        if (sve_size <= std_size) {
>> +            sve_ofs = size;
>> +            size += sve_size;
>> +            end1_ofs = size;
>> +        } else {
>> +            /* Otherwise we need to allocate extra space.  */
>> +            extra_ofs = size;
>> +            size += sizeof(struct target_extra_context);
>> +            end1_ofs = size;
>> +            size += QEMU_ALIGN_UP(sizeof(struct target_aarch64_ctx), 16);
> 
> Why do we add the size of target_aarch64_ctx to size here?
> We already account for the size of the end record later, so
> what is this one?

This is for the end record within the extra space, as opposed to the end record
within the standard space which is what we accounted for before.  A comment
would help, I supposed.

> 
>> +            extra_base = size;
>> +            extra_size = sve_size + sizeof(struct target_aarch64_ctx);
> 
> If we ever get two different kinds of optional record that need to
> live in the extra space, this is going to need refactoring,
> because at the moment it assumes that the SVE record is the first
> and only thing that might live there. I guess that's OK for now, though.

I'm not quite sure how to generalize this; let's just let the next thing make
that change, once we know what's needed?


r~

Re: [Qemu-devel] [PATCH v3 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Peter Maydell]

On 22 February 2018 at 20:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 02/22/2018 08:41 AM, Peter Maydell wrote:
>> On 16 February 2018 at 21:56, Richard Henderson
>> <richard.henderson@linaro.org> wrote:

>>> +        if (sve_size <= std_size) {
>>> +            sve_ofs = size;
>>> +            size += sve_size;
>>> +            end1_ofs = size;
>>> +        } else {
>>> +            /* Otherwise we need to allocate extra space.  */
>>> +            extra_ofs = size;
>>> +            size += sizeof(struct target_extra_context);
>>> +            end1_ofs = size;
>>> +            size += QEMU_ALIGN_UP(sizeof(struct target_aarch64_ctx), 16);
>>
>> Why do we add the size of target_aarch64_ctx to size here?
>> We already account for the size of the end record later, so
>> what is this one?
>
> This is for the end record within the extra space, as opposed to the end record
> within the standard space which is what we accounted for before.  A comment
> would help, I supposed.

Oh, so 'size' is accounting for both the standard space used
and the extra space? I had thought that 'size' was just counting
up the standard space used, and the extra space count was in
extra_size.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 5/5] aarch64-linux-user: Add support for SVE signal frame records

[Richard Henderson]

On 02/23/2018 01:59 AM, Peter Maydell wrote:
> On 22 February 2018 at 20:14, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 02/22/2018 08:41 AM, Peter Maydell wrote:
>>> On 16 February 2018 at 21:56, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
> 
>>>> +        if (sve_size <= std_size) {
>>>> +            sve_ofs = size;
>>>> +            size += sve_size;
>>>> +            end1_ofs = size;
>>>> +        } else {
>>>> +            /* Otherwise we need to allocate extra space.  */
>>>> +            extra_ofs = size;
>>>> +            size += sizeof(struct target_extra_context);
>>>> +            end1_ofs = size;
>>>> +            size += QEMU_ALIGN_UP(sizeof(struct target_aarch64_ctx), 16);
>>>
>>> Why do we add the size of target_aarch64_ctx to size here?
>>> We already account for the size of the end record later, so
>>> what is this one?
>>
>> This is for the end record within the extra space, as opposed to the end record
>> within the standard space which is what we accounted for before.  A comment
>> would help, I supposed.
> 
> Oh, so 'size' is accounting for both the standard space used
> and the extra space? I had thought that 'size' was just counting
> up the standard space used, and the extra space count was in
> extra_size.

Yes.  The revised code uses a different name, total_size, that hopefully makes
this more clear.


r~