Re: [PATCH v2 15/15] linux-user/s390x: Handle vector regs in signal stack

From: David Hildenbrand <david@redhat.com>
To: Richard Henderson <richard.henderson@linaro.org>, qemu-devel@nongnu.org
Cc: thuth@redhat.com, qemu-s390x@nongnu.org, cohuck@redhat.com,
	laurent@vivier.eu
Subject: Re: [PATCH v2 15/15] linux-user/s390x: Handle vector regs in signal stack
Date: Thu, 29 Apr 2021 09:32:58 +0200	[thread overview]
Message-ID: <0b8ad978-124a-6eff-f405-a7af3b378761@redhat.com> (raw)
In-Reply-To: <20210428193408.233706-16-richard.henderson@linaro.org>

On 28.04.21 21:34, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/s390x/signal.c | 62 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/s390x/signal.c b/linux-user/s390x/signal.c
> index 9d470e4ca0..b537646e60 100644
> --- a/linux-user/s390x/signal.c
> +++ b/linux-user/s390x/signal.c
> @@ -50,6 +50,12 @@ typedef struct {
>       target_s390_fp_regs     fpregs;
>   } target_sigregs;
>   
> +typedef struct {
> +    uint64_t vxrs_low[16];
> +    uint64_t vxrs_high[16][2];
> +    uint8_t reserved[128];
> +} target_sigregs_ext;
> +
>   typedef struct {
>       abi_ulong oldmask[_SIGCONTEXT_NSIG_WORDS];
>       abi_ulong sregs;
> @@ -60,15 +66,20 @@ typedef struct {
>       target_sigcontext sc;
>       target_sigregs sregs;
>       int signo;
> +    target_sigregs_ext sregs_ext;
>       uint16_t retcode;
>   } sigframe;
>   
> +#define TARGET_UC_VXRS 2
> +
>   struct target_ucontext {
>       abi_ulong tuc_flags;
>       abi_ulong tuc_link;
>       target_stack_t tuc_stack;
>       target_sigregs tuc_mcontext;
> -    target_sigset_t tuc_sigmask;   /* mask last for extensibility */
> +    target_sigset_t tuc_sigmask;
> +    uint8_t reserved[128 - sizeof(target_sigset_t)];

Guess I'd have used an unnamed union here

union {
	target_sigset_t tuc_sigmask;
	uint8_t reserved[128];
};

> +    target_sigregs_ext tuc_mcontext_ext;
>   };
>   
>   typedef struct {
> @@ -128,6 +139,24 @@ static void save_sigregs(CPUS390XState *env, target_sigregs *sregs)
>       }
>   }
>   
> +static void save_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
> +{
> +    int i;
> +
> +    /*
> +     * if (MACHINE_HAS_VX) ...
> +     * That said, we always allocate the stack storage and the
> +     * space is always available in env.
> +     */
> +    for (i = 0; i < 16; ++i) {
> +       __put_user(env->vregs[i][1], &ext->vxrs_low[i]);
> +    }
> +    for (i = 0; i < 16; ++i) {
> +       __put_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
> +       __put_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
> +    }
> +}
> +
>   void setup_frame(int sig, struct target_sigaction *ka,
>                    target_sigset_t *set, CPUS390XState *env)
>   {
> @@ -161,6 +190,9 @@ void setup_frame(int sig, struct target_sigaction *ka,
>        */
>       __put_user(sig, &frame->signo);
>   
> +    /* Create sigregs_ext on the signal stack. */
> +    save_sigregs_ext(env, &frame->sregs_ext);
> +
>       /*
>        * Set up to return from userspace.
>        * If provided, use a stub already in userspace.
> @@ -202,6 +234,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       rt_sigframe *frame;
>       abi_ulong frame_addr;
>       abi_ulong restorer;
> +    abi_ulong uc_flags;
>   
>       frame_addr = get_sigframe(ka, env, sizeof *frame);
>       trace_user_setup_rt_frame(env, frame_addr);
> @@ -229,10 +262,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       tswap_siginfo(&frame->info, info);
>   
>       /* Create ucontext on the signal stack. */
> -    __put_user(0, &frame->uc.tuc_flags);
> +    uc_flags = 0;
> +    if (s390_has_feat(S390_FEAT_VECTOR)) {
> +        uc_flags |= TARGET_UC_VXRS;
> +    }
> +    __put_user(uc_flags, &frame->uc.tuc_flags);
>       __put_user(0, &frame->uc.tuc_link);
>       target_save_altstack(&frame->uc.tuc_stack, env);
>       save_sigregs(env, &frame->uc.tuc_mcontext);
> +    save_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
>       tswap_sigset(&frame->uc.tuc_sigmask, set);
>   
>       /* Set up registers for signal handler */
> @@ -271,6 +309,24 @@ static void restore_sigregs(CPUS390XState *env, target_sigregs *sc)
>       }
>   }
>   
> +static void restore_sigregs_ext(CPUS390XState *env, target_sigregs_ext *ext)
> +{
> +    int i;
> +
> +    /*
> +     * if (MACHINE_HAS_VX) ...
> +     * That said, we always allocate the stack storage and the
> +     * space is always available in env.
> +     */
> +    for (i = 0; i < 16; ++i) {
> +       __get_user(env->vregs[i][1], &ext->vxrs_low[i]);
> +    }
> +    for (i = 0; i < 16; ++i) {
> +       __get_user(env->vregs[i + 16][0], &ext->vxrs_high[i][0]);
> +       __get_user(env->vregs[i + 16][1], &ext->vxrs_high[i][1]);
> +    }
> +}
> +
>   long do_sigreturn(CPUS390XState *env)
>   {
>       sigframe *frame;
> @@ -292,6 +348,7 @@ long do_sigreturn(CPUS390XState *env)
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
>       restore_sigregs(env, &frame->sregs);
> +    restore_sigregs_ext(env, &frame->sregs_ext);
>   
>       unlock_user_struct(frame, frame_addr, 0);
>       return -TARGET_QEMU_ESIGRETURN;
> @@ -313,6 +370,7 @@ long do_rt_sigreturn(CPUS390XState *env)
>       set_sigmask(&set); /* ~_BLOCKABLE? */
>   
>       restore_sigregs(env, &frame->uc.tuc_mcontext);
> +    restore_sigregs_ext(env, &frame->uc.tuc_mcontext_ext);
>   
>       target_restore_altstack(&frame->uc.tuc_stack, env);
>   
> 

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb