Re: [PATCH v4 40/45] linux-user/aarch64: Implement SME signal handling

[Peter Maydell]

On Tue, 28 Jun 2022 at 05:53, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Set the SM bit in the SVE record on signal delivery, create the ZA record.
> Restore SM and ZA state according to the records present on return.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/signal.c | 162 +++++++++++++++++++++++++++++++++---
>  1 file changed, 151 insertions(+), 11 deletions(-)
>
> diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
> index 22d0b8b4ec..1ad125d3d9 100644
> --- a/linux-user/aarch64/signal.c
> +++ b/linux-user/aarch64/signal.c
> @@ -104,6 +104,22 @@ struct target_sve_context {
>
>  #define TARGET_SVE_SIG_FLAG_SM  1
>
> +#define TARGET_ZA_MAGIC        0x54366345
> +
> +struct target_za_context {
> +    struct target_aarch64_ctx head;
> +    uint16_t vl;
> +    uint16_t reserved[3];
> +    /* The actual ZA data immediately follows. */
> +};
> +
> +#define TARGET_ZA_SIG_REGS_OFFSET \
> +    QEMU_ALIGN_UP(sizeof(struct target_za_context), TARGET_SVE_VQ_BYTES)
> +#define TARGET_ZA_SIG_ZAV_OFFSET(VQ, N) \
> +    (TARGET_ZA_SIG_REGS_OFFSET + (VQ) * TARGET_SVE_VQ_BYTES * (N))
> +#define TARGET_ZA_SIG_CONTEXT_SIZE(VQ) \
> +    TARGET_ZA_SIG_ZAV_OFFSET(VQ, VQ * TARGET_SVE_VQ_BYTES)
> +
>  struct target_rt_sigframe {
>      struct target_siginfo info;
>      struct target_ucontext uc;
> @@ -176,9 +192,9 @@ static void target_setup_end_record(struct 
> target_aarch64_ctx *end)
>  }
>
>  static void target_setup_sve_record(struct target_sve_context *sve,
> -                                    CPUARMState *env, int vq, int size)
> +                                    CPUARMState *env, int size)
>  {
> -    int i, j;
> +    int i, j, vq = sme_vq(env);


Shouldn't this be sve_vq() ?

>
>      memset(sve, 0, sizeof(*sve));
>      __put_user(TARGET_SVE_MAGIC, &sve->head.magic);
> @@ -207,6 +223,34 @@ static void target_setup_sve_record(struct 
> target_sve_context *sve,
>      }
>  }
>
> +static void target_setup_za_record(struct target_za_context *za,
> +                                   CPUARMState *env, int size)
> +{
> +    int vq = sme_vq(env);
> +    int vl = vq * TARGET_SVE_VQ_BYTES;
> +    int i, j;
> +
> +    memset(za, 0, sizeof(*za));
> +    __put_user(TARGET_ZA_MAGIC, &za->head.magic);
> +    __put_user(size, &za->head.size);
> +    __put_user(vl, &za->vl);
> +
> +    if (size == TARGET_ZA_SIG_CONTEXT_SIZE(0)) {
> +        return;
> +    }

I know we always set size in code we control, but it feels
a bit fragile to assume that if the size isn't the "for zero
data" case then it definitely has enough space to put all
the ZA data in it. Maybe we could assert() that the code
below isn't going to overrun size ?


> +
> +    /*
> +     * Note that ZA vectors are stored as a byte stream,
> +     * with each byte element at a subsequent address.
> +     */
> +    for (i = 0; i < vl; ++i) {
> +        uint64_t *z = (void *)za + TARGET_ZA_SIG_ZAV_OFFSET(vq, i);
> +        for (j = 0; j < vq * 2; ++j) {
> +            __put_user_e(env->zarray[i].d[j], z + j, le);
> +        }
> +    }
> +}
> +
>  static void target_restore_general_frame(CPUARMState *env,
>                                           struct target_rt_sigframe *sf)
>  {
> @@ -252,16 +296,28 @@ static void target_restore_fpsimd_record(CPUARMState 
> *env,
>
>  static bool target_restore_sve_record(CPUARMState *env,
>                                        struct target_sve_context *sve,
> -                                      int size)
> +                                      int size, int *svcr)
>  {
> -    int i, j, vl, vq;
> +    int i, j, vl, vq, flags;
> +    bool sm;
>
> +    /* ??? Kernel tests SVE && (!sm || SME); suggest (sm ? SME : SVE). */

This is fixed upstream (hasn't made it into Linus' git tree yet
but will at some point):
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=df07443f477a

so we might as well follow suit.

>      if (!cpu_isar_feature(aa64_sve, env_archcpu(env))) {
>          return false;
>      }
>
>      __get_user(vl, &sve->vl);
> -    vq = sve_vq(env);
> +    __get_user(flags, &sve->flags);
> +
> +    sm = flags & TARGET_SVE_SIG_FLAG_SM;
> +    if (sm) {
> +        if (!cpu_isar_feature(aa64_sme, env_archcpu(env))) {
> +            return false;
> +        }
> +        vq = sme_vq(env);
> +    } else {
> +        vq = sve_vq(env);
> +    }
>
>      /* Reject mismatched VL. */
>      if (vl != vq * TARGET_SVE_VQ_BYTES) {

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

thanks
-- PMM