Re: [Qemu-devel] [PATCH v2 11/12] linux-user/aarch64: Reset btype for syscalls and signals

[Peter Maydell]

On Mon, 28 Jan 2019 at 22:31, Richard Henderson
<address@hidden> wrote:
>
> The value of btype for syscalls is CONSTRAINED UNPREDICTABLE,
> so we need to make sure that the value is 0 before clone,
> fork, or syscall return.
>
> The value of btype for signals is defined, but it does not make
> sense for a SIGILL handler to enter with the btype set as for
> the indirect branch that caused the SIGILL.
>
> Clearing the value early means that btype is zero within the pstate
> saved into the signal frame, and so is also zero on (normal) signal
> return, but also allows the signal handler to adjust the value as
> seen after the sigcontext restore.
>
> This last is a guess at a future kernel's user-space ABI.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/aarch64/cpu_loop.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/linux-user/aarch64/cpu_loop.c b/linux-user/aarch64/cpu_loop.c
> index 65d815f030..51ea9961ba 100644
> --- a/linux-user/aarch64/cpu_loop.c
> +++ b/linux-user/aarch64/cpu_loop.c
> @@ -83,8 +83,19 @@ void cpu_loop(CPUARMState *env)
>          cpu_exec_end(cs);
>          process_queued_cpu_work(cs);
>
> +        /*
> +         * The state of BTYPE on syscall and interrupt entry is CONSTRAINED
> +         * UNPREDICTABLE.  The real kernel will need to tidy this up as well.
> +         * Do this before syscalls and signals, so that the value is correct
> +         * both within signal handlers, and on return from syscall 
> (especially
> +         * clone & fork) and from signal handlers.
> +         *
> +         * The SIGILL signal handler, for BTITrap, can see the failing BTYPE
> +         * within the ESR value in the signal frame.
> +         */
>          switch (trapnr) {
>          case EXCP_SWI:
> +            env->btype = 0;
>              ret = do_syscall(env,
>                               env->xregs[8],
>                               env->xregs[0],

If the idea is to give a particular value on return from
the syscall and on entry to a signal handler, shouldn't we be
setting it after the do_syscall() call returns, and in the
signal handler entry path ?

> @@ -104,6 +115,7 @@ void cpu_loop(CPUARMState *env)
>              /* just indicate that signals should be handled asap */
>              break;
>          case EXCP_UDEF:
> +            env->btype = 0;
>              info.si_signo = TARGET_SIGILL;
>              info.si_errno = 0;
>              info.si_code = TARGET_ILL_ILLOPN;
> @@ -112,6 +124,7 @@ void cpu_loop(CPUARMState *env)
>              break;
>          case EXCP_PREFETCH_ABORT:
>          case EXCP_DATA_ABORT:
> +            env->btype = 0;
>              info.si_signo = TARGET_SIGSEGV;
>              info.si_errno = 0;
>              /* XXX: check env->error_code */

> @@ -121,12 +134,14 @@ void cpu_loop(CPUARMState *env)
>              break;
>          case EXCP_DEBUG:
>          case EXCP_BKPT:
> +            env->btype = 0;
>              info.si_signo = TARGET_SIGTRAP;
>              info.si_errno = 0;
>              info.si_code = TARGET_TRAP_BRKPT;
>              queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
>              break;
>          case EXCP_SEMIHOST:
> +            env->btype = 0;

Leaving btype alone rather than clearing it here would be
consistent with how we handle semihosting in system emulation,
right ?

>              env->xregs[0] = do_arm_semihosting(env);
>              break;
>          case EXCP_YIELD:
> --

thanks
-- PMM