Re: [Qemu-devel] [PATCH v2 0/6] user-exec: cpu_resume_from_signal() cleanups

[Peter Maydell <peter.maydell@linaro.org>]

On 6 June 2016 at 15:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping!

Thanks for the review, Sergey. Unless anybody else wants to review
or wants to take it through their tree (Riku?), I propose to apply
this to master sometime later this week.

thanks
-- PMM

> On 17 May 2016 at 15:18, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I was trying to reason about user-mode's handling of signal masks,
>> and I found our current code a bit confusing, so I cleaned it up.
>>
>> At the moment for user-only mode cpu_resume_from_signal() takes a
>> usercontext pointer; if this is non-NULL then it has some awkward
>> OS and CPU specific code to set the signal mask from something
>> inside the usercontext before doing the same kind of siglongjmp()
>> that the softmmu cpu_resume_from_signal() does.
>>
>> In fact the two use cases are completely separate:
>>  * almost all calls to cpu_resume_from_signal() pass a NULL puc
>>    argument (and most of those are softmmu-only anyway)
>>  * only the code path handle_cpu_signal -> page_unprotect ->
>>    tb_invalidate_phys_page -> cpu_resume_from_signal will pass
>>    a non-NULL puc.
>>
>> The cleanups are:
>>  * pull the call to cpu_resume_from_signal() up through the
>>    callstack so we do the signal mask manipulation in
>>    handle_cpu_signal()
>>  * drop the OS/CPU spceific code to get a signal mask out of
>>    a usercontext, because in the specific case of handle_cpu_signal()
>>    we already have the signal mask value and can just use it
>>  * rename cpu_resume_from_signal() to cpu_loop_exit_noexc(),
>>    since all the remaining callsites are not in fact signal handlers
>>    or even called from signal handlers
>>  * get rid of an ugly TARGET_I386 ifdef in user-exec.c by moving
>>    the i386-specific code into its handle_mmu_fault hook.
>>
>> Changes v1->v2:
>>  * patches 1-4 are the same and already reviewed
>>  * patch 5 is new, and just adds a clarifying comment to
>>    do_interrupt_user()
>>  * patch 6 is the old patch 5, and now sets env->exception_next_eip
>>    to -1 as a clear indication that the value is not going to be used
>>    (as noted in the comment in the new patch 5)
>>
>> thanks
>> -- PMM
>>
>>
>> Peter Maydell (6):
>>   translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page()
>>   user-exec: Push resume-from-signal code out to handle_cpu_signal()
>>   cpu-exec: Rename cpu_resume_from_signal() to cpu_loop_exit_noexc()
>>   user-exec: Don't reextract sigmask from usercontext pointer
>>   target-i386: Add comment about do_interrupt_user() next_eip argument
>>   target-i386: Move user-mode exception actions out of user-exec.c
>>
>>  cpu-exec-common.c        |  8 ++---
>>  exec.c                   |  2 +-
>>  hw/i386/kvmvapic.c       |  2 +-
>>  include/exec/exec-all.h  |  2 +-
>>  target-i386/bpt_helper.c |  2 +-
>>  target-i386/helper.c     |  2 ++
>>  target-i386/seg_helper.c |  6 +++-
>>  target-lm32/helper.c     |  2 +-
>>  target-s390x/helper.c    |  2 +-
>>  target-xtensa/helper.c   |  2 +-
>>  translate-all.c          | 40 ++++++++++++---------
>>  translate-all.h          |  2 +-
>>  user-exec.c              | 93 +++++++++++++++++++++---------------------------
>>  13 files changed, 82 insertions(+), 83 deletions(-)
>>
>> --
>> 1.9.1