Re: [PATCH 1/3] linux-user/aarch64: Add ESR signal frame for SIGSEGV, SIGBUS

[Peter Maydell]

On Tue, 22 Aug 2023 at 18:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> These are all synchronous exceptions for which the kernel
> passes on ESR to the user signal handler.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> @@ -191,6 +199,14 @@ static void target_setup_end_record(struct 
> target_aarch64_ctx *end)
>      __put_user(0, &end->size);
>  }
>
> +static void target_setup_esr_record(struct target_esr_context *esr,
> +                                    CPUARMState *env)
> +{
> +    __put_user(TARGET_ESR_MAGIC, &esr->head.magic);
> +    __put_user(sizeof(struct target_esr_context), &esr->head.size);
> +    __put_user(env->exception.syndrome, &esr->esr);
> +}
> +
>  static void target_setup_sve_record(struct target_sve_context *sve,
>                                      CPUARMState *env, int size)
>  {
> @@ -443,6 +459,10 @@ static int target_restore_sigframe(CPUARMState *env,
>              fpsimd = (struct target_fpsimd_context *)ctx;
>              break;
>
> +        case TARGET_ESR_MAGIC:
> +            /* ignore */
> +            break;
> +
>          case TARGET_SVE_MAGIC:
>              if (sve || size < sizeof(struct target_sve_context)) {
>                  goto err;
> @@ -558,6 +578,23 @@ static int alloc_sigframe_space(int this_size, 
> target_sigframe_layout *l)
>      return this_loc;
>  }
>
> +static bool need_save_esr(target_siginfo_t *info, CPUARMState *env)
> +{
> +    int sig = info->si_signo;
> +    int type = info->si_code >> 16;
> +
> +    if (type != QEMU_SI_FAULT) {
> +        return false;
> +    }
> +
> +    /* See arch/arm64/mm/fault.c, set_thread_esr. */
> +    if (sig == TARGET_SIGSEGV || sig == TARGET_SIGBUS) {
> +        return true;
> +    }

It's possible to get here without env->exception.syndrome
being set correctly, I think, if we take a host
SIGSEGV or SIGBUS and host_signal_handler() calls either
cpu_loop_exit_sigsegv() or cpu_loop_exit_sigbus(). Can also
happen for other places that call one of those two functions,
like allocation_tag_mem(). At least, I can't see where we
would be setting syndrome in that code path.

> +
> +    return false;
> +}

Maybe we should do the "sanitize ESR for fault addresses in
the upper half of guest address space" logic that the kernel
set_thread_esr() does?

thanks
-- PMM