Re: [PATCH v3 18/21] linux-user/aarch64: Signal SEGV_MTEAERR for async tag check error

[Richard Henderson <richard.henderson@linaro.org>]

On 1/22/21 3:59 AM, Peter Maydell wrote:
> On Fri, 15 Jan 2021 at 22:47, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> So when does the real kernel report async MTE exceptions to userspace?
> The commit message would be a good place to briefly describe the
> kernel's strategy and where QEMU differs from it (if anywhere)...

I can add that, sure.

>>          case EXCP_INTERRUPT:
>> -            /* just indicate that signals should be handled asap */
>> +            /* Just indicate that signals should be handled asap. */
>> +            check_mte_async_fault(env, &info);
>>              break;
>>          case EXCP_UDEF:
>>              info.si_signo = TARGET_SIGILL;
> 
> So this doesn't guarantee to check the async-fault status on
> every exit from cpu_exec(), which means we might miss things.
> For instance I think this slightly contrived example would not
> ever take the SEGV:
>     STR x0, [x1]   # with a bad tag
>     YIELD
> l:  B l
> 
> because the STR and YIELD go into the same TB, the YIELD causes us
> to leave the TB with EXCP_YIELD, we don't check for an async fault
> in that code path, and then we'll go into the infinite loop and
> have nothing to prompt us to come out and look at the async fault flags.

I'll add that test case to the smoke test.

> Does it work if we just always queue the SEGV on exit from cpu_exec()
> and let the signal handling machinery prioritize if we also pend
> some other signal because this was an EXCP_UDEF or whatever?
> It would be neater if we could keep the fault-check outside the
> switch (trapnr) somehow.

I would think so.  I'll try that.

>> +#ifdef CONFIG_USER_ONLY
>> +        /*
>> +         * Stand in for a timer irq, setting _TIF_MTE_ASYNC_FAULT,
>> +         * which then sends a SIGSEGV when the thread is next scheduled.
>> +         * This cpu will return to the main loop at the end of the TB,
>> +         * which is rather sooner than "normal".  But the alternative
>> +         * is waiting until the next syscall.
>> +         */
>> +        qemu_cpu_kick(env_cpu(env));
>> +#endif
>>          break;
> 
> This does the right thing, but qemu_cpu_kick() is one of those
> functions that's in a category of "not used much at all in the
> codebase" and which always make me wonder if there's a reason.
> (In particular there's exactly one use in the whole of target/
> right now.) I suppose the case of "helper function wants to cause
> us to leave the TB loop but not to abort the current insn" is
> an unusual one...

Exactly.

Usually something in target/ calls (via mmio or whatnot) something in hw/ which
raises an interrupt, which does the kick.


r~