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

Richard Henderson posted 12 patches 6 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v2 11/12] linux-user/aarch64: Reset btype for syscalls and signals
Posted by Richard Henderson 6 years, 8 months ago
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 <richard.henderson@linaro.org>
---
 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],
@@ -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;
             env->xregs[0] = do_arm_semihosting(env);
             break;
         case EXCP_YIELD:
-- 
2.17.2


Re: [Qemu-devel] [PATCH v2 11/12] linux-user/aarch64: Reset btype for syscalls and signals
Posted by Peter Maydell 6 years, 7 months ago
On Mon, 28 Jan 2019 at 22:31, Richard Henderson
<richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>
> ---
>  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

Re: [Qemu-devel] [PATCH v2 11/12] linux-user/aarch64: Reset btype for syscalls and signals
Posted by Richard Henderson 6 years, 7 months ago
On 2/4/19 12:02 PM, Peter Maydell wrote:
> On Mon, 28 Jan 2019 at 22:31, Richard Henderson
> <richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>
>> ---
>>  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 ?

Er.. yes.  I sort of forgot we had semi-hosting for aa64.


r~