Re: [PATCH v2 19/41] hw/core: Add TCGCPUOps.record_sigsegv

[Richard Henderson]

On 9/19/21 11:24 AM, Philippe Mathieu-Daudé wrote:
> On 9/19/21 20:22, Philippe Mathieu-Daudé wrote:
> > On 9/18/21 20:45, Richard Henderson wrote:
> > > Add a new user-only interface for updating cpu state before
> > > raising a signal.  This will replace tlb_fill for user-only
> > > and should result in less boilerplate for each guest.
> > >
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> > >   include/hw/core/tcg-cpu-ops.h | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > >
> > > diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
> > > index 4a4c4053e3..e229a40772 100644
> > > --- a/include/hw/core/tcg-cpu-ops.h
> > > +++ b/include/hw/core/tcg-cpu-ops.h
> > > @@ -114,6 +114,32 @@ struct TCGCPUOps {
> > >        */
> > >       bool (*io_recompile_replay_branch)(CPUState *cpu,
> > >                                          const TranslationBlock *tb);
> > > +#else
> > > +    /**
> > > +     * record_sigsegv:
> > > +     * @cpu: cpu context
> > > +     * @addr: faulting guest address
> > > +     * @access_type: access was read/write/execute
> > > +     * @maperr: true for invalid page, false for permission fault
> > > +     * @ra: host pc for unwinding
> > > +     *
> > > +     * We are about to raise SIGSEGV with si_code set for @maperr,
> > > +     * and si_addr set for @addr.  Record anything further needed
> > > +     * for the signal ucontext_t.
> > > +     *
> > > +     * If the emulated kernel does not provide anything to the signal
> > > +     * handler with anything besides the user context registers, and
> > > +     * the siginfo_t, then this hook need do nothing and may be omitted.
> > > +     * Otherwise, record the data and return; the caller will raise
> > > +     * the signal, unwind the cpu state, and return to the main loop.
> > > +     *
> > > +     * If it is simpler to re-use the sysemu tlb_fill code, @ra is provided
> > > +     * so that a "normal" cpu exception can be raised.  In this case,
> > > +     * the signal must be raised by the architecture cpu_loop.
> > > +     */
> >
> > Shouldn't it have the QEMU_NORETURN attribute?
>
> Eh now I saw the next patch and understood raise_sigsegv() is
> where QEMU_NORETURN belong :)

Well, I had intended the hook to be able to return, so that the hook can merely set some 
env->fields, and return to raise the signal.  But in the end, all 4 targets that use the 
hook raise the exception themselves -- sometimes because env->exception_index is part of 
the data to be passed to the signal handler.


r~