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

[Richard Henderson]

On 2/4/19 12:02 PM, Peter Maydell wrote:
> 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 ?

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


r~