Re: [PATCH 3/4] linux-user/riscv: Add vector state to signal context

[Richard Henderson <richard.henderson@linaro.org>]

On 9/3/25 06:25, Nicholas Piggin wrote:
> This enables vector state to be saved and restored across signals.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   linux-user/riscv/signal.c | 130 ++++++++++++++++++++++++++++++++++++--
>   1 file changed, 126 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
> index 4ef55d0848..4acbabcbc9 100644
> --- a/linux-user/riscv/signal.c
> +++ b/linux-user/riscv/signal.c
> @@ -41,7 +41,17 @@ struct target_fp_state {
>       uint32_t fcsr;
>   };
>   
> +struct target_v_ext_state {
> +    target_ulong vstart;
> +    target_ulong vl;
> +    target_ulong vtype;
> +    target_ulong vcsr;
> +    target_ulong vlenb;

All abi_ulong.


> +    target_ulong datap;

abi_ptr.

> +} __attribute__((aligned(16)));

Where does this come from?

As it happens, sizeof(struct target_v_ext_state) will be a multiple of 16 for riscv64. 
however...

> +
>   /* The Magic number for signal context frame header. */
> +#define RISCV_V_MAGIC   0x53465457
>   #define END_MAGIC       0x0
>   
>   /* The size of END signal context header. */
> @@ -106,6 +116,88 @@ static abi_ulong get_sigframe(struct target_sigaction *ka,
>       return sp;
>   }
>   
> +static unsigned int get_v_state_size(CPURISCVState *env)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +
> +    return sizeof(struct target_ctx_hdr) +
> +           sizeof(struct target_v_ext_state) +
> +           cpu->cfg.vlenb * 32;
> +}
> +
> +static struct target_ctx_hdr *save_v_state(CPURISCVState *env,
> +                                           struct target_ctx_hdr *hdr)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +    target_ulong vlenb = cpu->cfg.vlenb;
> +    uint32_t riscv_v_sc_size = get_v_state_size(env);
> +    struct target_v_ext_state *vs;
> +    uint64_t *vdatap;
> +    int i;
> +
> +    __put_user(RISCV_V_MAGIC, &hdr->magic);
> +    __put_user(riscv_v_sc_size, &hdr->size);
> +
> +    vs = (struct target_v_ext_state *)(hdr + 1);
> +    vdatap = (uint64_t *)(vs + 1);

... if you wanted to ensure 16-byte alignment of the data for riscv32, you'd do it here. 
I don't believe there's anything about RVV that would require 16-byte aligned data though.

> +
> +    __put_user(env->vstart, &vs->vstart);
> +    __put_user(env->vl, &vs->vl);
> +    __put_user(env->vtype, &vs->vtype);
> +    target_ulong vcsr = riscv_csr_read(env, CSR_VCSR);
> +    __put_user(vcsr, &vs->vcsr);
> +    __put_user(vlenb, &vs->vlenb);
> +    __put_user((target_ulong)vdatap, &vs->datap);

h2g(vdatap)

> +static void restore_v_state(CPURISCVState *env,
> +                            struct target_ctx_hdr *hdr)
> +{
> +    RISCVCPU *cpu = env_archcpu(env);
> +    target_ulong vlenb = cpu->cfg.vlenb;
> +    struct target_v_ext_state *vs;
> +    uint64_t *vdatap;
> +    int i;
> +
> +    uint32_t size;
> +    __get_user(size, &hdr->size);
> +    if (size != get_v_state_size(env)) {
> +        g_assert_not_reached();
> +        /* XXX: warn, bail */
> +    }

Do not assert.  The kernel simply fails the restore,

     if (!(has_vector() || has_xtheadvector()) ||
         !riscv_v_vstate_query(regs) ||
         size != riscv_v_sc_size)
         return -EINVAL;


leading to SIGSEGV.

> +
> +    vs = (struct target_v_ext_state *)(hdr + 1);
> +
> +    __get_user(env->vstart, &vs->vstart);
> +    __get_user(env->vl, &vs->vl);
> +    __get_user(env->vtype, &vs->vtype);

You're missing a step here.  The unsanitized values from vs should be processed as if 
"vsetvl x0, vtype, vl".  In particular, if either input is garbage, vill will be set.  You 
probably need a small wrapper around helper_vsetvl().

Similarly vstart should be written with riscv_csr_write because it gets masked.

> +    target_ulong vcsr;
> +    __get_user(vcsr, &vs->vcsr);

Do not intersperse declarations.  As much as I like them, qemu has denied them in 
docs/devel/style.rst.

> +    riscv_csr_write(env, CSR_VCSR, vcsr);
> +    __get_user(vlenb, &vs->vlenb);
> +    target_ulong __vdatap;

No __, ever.  It's a reserved namespace in user-land.

> +    __get_user(__vdatap, &vs->datap);
> +    vdatap = (uint64_t *)__vdatap;

Bad cast.  Since vdatap may be discontiguous, you need

   host_vdatap = lock_user(VERIFY_READ, guest_vdatap, env->vlenb * 32);
   if (!host_vdatap) {
     goto badaddr;
   }

> +
> +    for (i = 0; i < 32; i++) {
> +        int j;
> +        for (j = 0; j < vlenb; j += 8) {

Feel free to use

     for (int i = 0;
         for (int j = 0;

etc.

> @@ -207,9 +308,30 @@ static void restore_sigcontext(CPURISCVState *env, struct target_sigcontext *sc)
>   
>       uint32_t magic;
>       __get_user(magic, &hdr->magic);
> -    if (magic != END_MAGIC) {
> -        qemu_log_mask(LOG_UNIMP, "signal: unknown extended context header: "
> -                                 "0x%08x, ignoring", magic);
> +    while (magic != END_MAGIC) {
> +        if (magic == RISCV_V_MAGIC) {
> +            if (riscv_has_ext(env, RVV)) {
> +                restore_v_state(env, hdr);
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR, "signal: sigcontext has V state "
> +                                               "but CPU does not.");
> +            }
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "signal: unknown extended state in "
> +                                           "sigcontext magic=0x%08x", magic);
> +        }
> +
> +        if (hdr->size == 0) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "signal: extended state in sigcontext "
> +                                           "has size 0");
> +        }
> +        hdr = (void *)hdr + hdr->size;
> +        __get_user(magic, &hdr->magic);
> +    }
> +
> +    if (hdr->size != END_HDR_SIZE) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "signal: extended state end header has "
> +                                       "size=%u (should be 0)", hdr->size);

All of these GUEST_ERROR should goto badaddr.  We haven't generally logged the reason for 
this, though I don't have any particular objection to doing so.


r~