[Qemu-devel] [PATCH 0/5] user-exec: cpu_resume_from_signal() cleanups

[Qemu-devel] [PATCH 0/5] user-exec: cpu_resume_from_signal() cleanups

[Peter Maydell]

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.

Peter Maydell (5):
  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: 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-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 +++++++++++++++++++++---------------------------
 12 files changed, 77 insertions(+), 82 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page()

[Peter Maydell]

The user-mode-only function tb_invalidate_phys_page() is only
called from two places:
 * page_unprotect(), which passes in a non-zero pc, a puc pointer
   and the value 'true' for the locked argument
 * page_set_flags(), which passes in a zero pc, a NULL puc pointer
   and a 'false' locked argument

If the pc is non-zero then we may call cpu_resume_from_signal(),
which does a longjmp out of the calling code (and out of the
signal handler); this is to cover the case of a target CPU with
"precise self-modifying code" (currently only x86) executing
a store instruction which modifies code in the same TB as the
store itself. Rather than doing the longjump directly here,
return a flag to the caller which indicates whether the current
TB was modified, and move the longjump to page_unprotect.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 translate-all.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/translate-all.c b/translate-all.c
index b54f472..4820d2e 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1438,10 +1438,13 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
     }
 }
 #else
-/* Called with mmap_lock held.  */
-static void tb_invalidate_phys_page(tb_page_addr_t addr,
-                                    uintptr_t pc, void *puc,
-                                    bool locked)
+/* Called with mmap_lock held. If pc is not 0 then it indicates the
+ * host PC of the faulting store instruction that caused this invalidate.
+ * Returns true if the caller needs to abort execution of the current
+ * TB (because it was modified by this store and the guest CPU has
+ * precise-SMC semantics).
+ */
+static bool tb_invalidate_phys_page(tb_page_addr_t addr, uintptr_t pc)
 {
     TranslationBlock *tb;
     PageDesc *p;
@@ -1459,7 +1462,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
     addr &= TARGET_PAGE_MASK;
     p = page_find(addr >> TARGET_PAGE_BITS);
     if (!p) {
-        return;
+        return false;
     }
     tb = p->first_tb;
 #ifdef TARGET_HAS_PRECISE_SMC
@@ -1498,12 +1501,10 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
            modifying the memory. It will ensure that it cannot modify
            itself */
         tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
-        if (locked) {
-            mmap_unlock();
-        }
-        cpu_resume_from_signal(cpu, puc);
+        return true;
     }
 #endif
+    return false;
 }
 #endif
 
@@ -1902,7 +1903,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
         if (!(p->flags & PAGE_WRITE) &&
             (flags & PAGE_WRITE) &&
             p->first_tb) {
-            tb_invalidate_phys_page(addr, 0, NULL, false);
+            tb_invalidate_phys_page(addr, 0);
         }
         p->flags = flags;
     }
@@ -1996,7 +1997,10 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
 
             /* and since the content will be modified, we must invalidate
                the corresponding translated code. */
-            tb_invalidate_phys_page(addr, pc, puc, true);
+            if (tb_invalidate_phys_page(addr, pc)) {
+                mmap_unlock();
+                cpu_resume_from_signal(current_cpu, puc);
+            }
 #ifdef DEBUG_TB_CHECK
             tb_invalidate_check(addr);
 #endif
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/5] translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page()

[Sergey Fedorov]

On 16/05/16 19:09, Peter Maydell wrote:
> The user-mode-only function tb_invalidate_phys_page() is only
> called from two places:
>  * page_unprotect(), which passes in a non-zero pc, a puc pointer
>    and the value 'true' for the locked argument
>  * page_set_flags(), which passes in a zero pc, a NULL puc pointer
>    and a 'false' locked argument
>
> If the pc is non-zero then we may call cpu_resume_from_signal(),
> which does a longjmp out of the calling code (and out of the
> signal handler); this is to cover the case of a target CPU with
> "precise self-modifying code" (currently only x86) executing
> a store instruction which modifies code in the same TB as the
> store itself. Rather than doing the longjump directly here,
> return a flag to the caller which indicates whether the current
> TB was modified, and move the longjump to page_unprotect.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  translate-all.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/translate-all.c b/translate-all.c
> index b54f472..4820d2e 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1438,10 +1438,13 @@ void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len)
>      }
>  }
>  #else
> -/* Called with mmap_lock held.  */
> -static void tb_invalidate_phys_page(tb_page_addr_t addr,
> -                                    uintptr_t pc, void *puc,
> -                                    bool locked)
> +/* Called with mmap_lock held. If pc is not 0 then it indicates the
> + * host PC of the faulting store instruction that caused this invalidate.
> + * Returns true if the caller needs to abort execution of the current
> + * TB (because it was modified by this store and the guest CPU has
> + * precise-SMC semantics).
> + */
> +static bool tb_invalidate_phys_page(tb_page_addr_t addr, uintptr_t pc)
>  {
>      TranslationBlock *tb;
>      PageDesc *p;
> @@ -1459,7 +1462,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>      addr &= TARGET_PAGE_MASK;
>      p = page_find(addr >> TARGET_PAGE_BITS);
>      if (!p) {
> -        return;
> +        return false;
>      }
>      tb = p->first_tb;
>  #ifdef TARGET_HAS_PRECISE_SMC
> @@ -1498,12 +1501,10 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>             modifying the memory. It will ensure that it cannot modify
>             itself */
>          tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
> -        if (locked) {
> -            mmap_unlock();
> -        }
> -        cpu_resume_from_signal(cpu, puc);
> +        return true;
>      }
>  #endif
> +    return false;
>  }
>  #endif
>  
> @@ -1902,7 +1903,7 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
>          if (!(p->flags & PAGE_WRITE) &&
>              (flags & PAGE_WRITE) &&
>              p->first_tb) {
> -            tb_invalidate_phys_page(addr, 0, NULL, false);
> +            tb_invalidate_phys_page(addr, 0);
>          }
>          p->flags = flags;
>      }
> @@ -1996,7 +1997,10 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
>  
>              /* and since the content will be modified, we must invalidate
>                 the corresponding translated code. */
> -            tb_invalidate_phys_page(addr, pc, puc, true);
> +            if (tb_invalidate_phys_page(addr, pc)) {
> +                mmap_unlock();
> +                cpu_resume_from_signal(current_cpu, puc);
> +            }
>  #ifdef DEBUG_TB_CHECK
>              tb_invalidate_check(addr);
>  #endif

Just my 2 cents: we could allow that cpu_resume_from_signal() call and
add mmap_lock_reset() similar to tb_lock_reset() to handle resetting
mmap_lock after a long jump.

Kind regards,
Sergey

Re: [Qemu-devel] [PATCH 1/5] translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page()

[Peter Maydell]

On 16 May 2016 at 18:13, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 16/05/16 19:09, Peter Maydell wrote:
>> @@ -1996,7 +1997,10 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
>>
>>              /* and since the content will be modified, we must invalidate
>>                 the corresponding translated code. */
>> -            tb_invalidate_phys_page(addr, pc, puc, true);
>> +            if (tb_invalidate_phys_page(addr, pc)) {
>> +                mmap_unlock();
>> +                cpu_resume_from_signal(current_cpu, puc);
>> +            }
>>  #ifdef DEBUG_TB_CHECK
>>              tb_invalidate_check(addr);
>>  #endif
>
> Just my 2 cents: we could allow that cpu_resume_from_signal() call and
> add mmap_lock_reset() similar to tb_lock_reset() to handle resetting
> mmap_lock after a long jump.

There's no need -- if you look at the rest of the patchset, that
call goes away from this function entirely and ends up in the
caller, at which point this function's handling of the mmap
lock is the straightforward "lock on entry, unlock before return".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/5] translate-all.c: Don't pass puc, locked to tb_invalidate_phys_page()

[Sergey Fedorov]

On 16/05/16 20:15, Peter Maydell wrote:
> On 16 May 2016 at 18:13, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 16/05/16 19:09, Peter Maydell wrote:
>>> @@ -1996,7 +1997,10 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
>>>
>>>              /* and since the content will be modified, we must invalidate
>>>                 the corresponding translated code. */
>>> -            tb_invalidate_phys_page(addr, pc, puc, true);
>>> +            if (tb_invalidate_phys_page(addr, pc)) {
>>> +                mmap_unlock();
>>> +                cpu_resume_from_signal(current_cpu, puc);
>>> +            }
>>>  #ifdef DEBUG_TB_CHECK
>>>              tb_invalidate_check(addr);
>>>  #endif
>> Just my 2 cents: we could allow that cpu_resume_from_signal() call and
>> add mmap_lock_reset() similar to tb_lock_reset() to handle resetting
>> mmap_lock after a long jump.
> There's no need -- if you look at the rest of the patchset, that
> call goes away from this function entirely and ends up in the
> caller, at which point this function's handling of the mmap
> lock is the straightforward "lock on entry, unlock before return".

Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Thanks,
Sergey

[Qemu-devel] [PATCH 2/5] user-exec: Push resume-from-signal code out to handle_cpu_signal()

[Peter Maydell]

Since the only caller of page_unprotect() which might cause it to
need to call cpu_resume_from_signal() is handle_cpu_signal() in
the user-mode code, push the longjump handling out to that function.

Since this is the only caller of cpu_resume_from_signal() which
passes a non-NULL puc argument, split the non-NULL handling into
a new cpu_exit_tb_from_sighandler() function. This allows us
to merge the softmmu and usermode implementations of the
cpu_resume_from_signal() function, which are now identical.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-exec-common.c |  2 +-
 translate-all.c   | 12 ++++++++----
 translate-all.h   |  2 +-
 user-exec.c       | 41 +++++++++++++++++++++++++++++------------
 4 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index 6bdda6b..62f5d6b 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -28,7 +28,6 @@ CPUState *tcg_current_cpu;
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
-#if defined(CONFIG_SOFTMMU)
 void cpu_resume_from_signal(CPUState *cpu, void *puc)
 {
     /* XXX: restore cpu registers saved in host registers */
@@ -37,6 +36,7 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
     siglongjmp(cpu->jmp_env, 1);
 }
 
+#if defined(CONFIG_SOFTMMU)
 void cpu_reloading_memory_map(void)
 {
     if (qemu_in_vcpu_thread()) {
diff --git a/translate-all.c b/translate-all.c
index 4820d2e..52a571e 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1955,7 +1955,7 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
             /* unprotect the page if it was put read-only because it
                contains translated code */
             if (!(p->flags & PAGE_WRITE)) {
-                if (!page_unprotect(addr, 0, NULL)) {
+                if (!page_unprotect(addr, 0)) {
                     return -1;
                 }
             }
@@ -1965,8 +1965,12 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
 }
 
 /* called from signal handler: invalidate the code and unprotect the
-   page. Return TRUE if the fault was successfully handled. */
-int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
+ * page. Return 0 if the fault was not handled, 1 if it was handled,
+ * and 2 if it was handled but the caller must cause the TB to be
+ * immediately exited. (We can only return 2 if the 'pc' argument is
+ * non-zero.)
+ */
+int page_unprotect(target_ulong address, uintptr_t pc)
 {
     unsigned int prot;
     PageDesc *p;
@@ -1999,7 +2003,7 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
                the corresponding translated code. */
             if (tb_invalidate_phys_page(addr, pc)) {
                 mmap_unlock();
-                cpu_resume_from_signal(current_cpu, puc);
+                return 2;
             }
 #ifdef DEBUG_TB_CHECK
             tb_invalidate_check(addr);
diff --git a/translate-all.h b/translate-all.h
index 0384640..ce6071b 100644
--- a/translate-all.h
+++ b/translate-all.h
@@ -27,7 +27,7 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end);
 void tb_check_watchpoint(CPUState *cpu);
 
 #ifdef CONFIG_USER_ONLY
-int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
+int page_unprotect(target_ulong address, uintptr_t pc);
 #endif
 
 #endif /* TRANSLATE_ALL_H */
diff --git a/user-exec.c b/user-exec.c
index d8d597b..1d02e24 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -54,7 +54,7 @@ static void exception_action(CPUState *cpu)
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
-void cpu_resume_from_signal(CPUState *cpu, void *puc)
+static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
 {
 #ifdef __linux__
     struct ucontext *uc = puc;
@@ -62,20 +62,18 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
     struct sigcontext *uc = puc;
 #endif
 
-    if (puc) {
-        /* XXX: use siglongjmp ? */
+    /* XXX: use siglongjmp ? */
 #ifdef __linux__
 #ifdef __ia64
-        sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
+    sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
 #else
-        sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
+    sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
 #endif
 #elif defined(__OpenBSD__)
-        sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
+    sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
 #endif
-    }
-    cpu->exception_index = -1;
-    siglongjmp(cpu->jmp_env, 1);
+
+    cpu_resume_from_signal(cpu, NULL);
 }
 
 /* 'pc' is the host PC at which the exception was raised. 'address' is
@@ -95,9 +93,28 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
            pc, address, is_write, *(unsigned long *)old_set);
 #endif
     /* XXX: locking issue */
-    if (is_write && h2g_valid(address)
-        && page_unprotect(h2g(address), pc, puc)) {
-        return 1;
+    if (is_write && h2g_valid(address)) {
+        switch (page_unprotect(h2g(address), pc)) {
+        case 0:
+            /* Fault not caused by a page marked unwritable to protect
+             * cached translations, must be the guest binary's problem
+             */
+            break;
+        case 1:
+            /* Fault caused by protection of cached translation; TBs
+             * invalidated, so resume execution
+             */
+            return 1;
+        case 2:
+            /* Fault caused by protection of cached translation, and the
+             * currently executing TB was modified and must be exited
+             * immediately.
+             */
+            cpu_exit_tb_from_sighandler(current_cpu, puc);
+            g_assert_not_reached();
+        default:
+            g_assert_not_reached();
+        }
     }
 
     /* Convert forcefully to guest address space, invalid addresses
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/5] user-exec: Push resume-from-signal code out to handle_cpu_signal()

[Sergey Fedorov]

On 16/05/16 19:09, Peter Maydell wrote:
> Since the only caller of page_unprotect() which might cause it to
> need to call cpu_resume_from_signal() is handle_cpu_signal() in
> the user-mode code, push the longjump handling out to that function.
>
> Since this is the only caller of cpu_resume_from_signal() which
> passes a non-NULL puc argument, split the non-NULL handling into
> a new cpu_exit_tb_from_sighandler() function. This allows us
> to merge the softmmu and usermode implementations of the
> cpu_resume_from_signal() function, which are now identical.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>

> ---
>  cpu-exec-common.c |  2 +-
>  translate-all.c   | 12 ++++++++----
>  translate-all.h   |  2 +-
>  user-exec.c       | 41 +++++++++++++++++++++++++++++------------
>  4 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/cpu-exec-common.c b/cpu-exec-common.c
> index 6bdda6b..62f5d6b 100644
> --- a/cpu-exec-common.c
> +++ b/cpu-exec-common.c
> @@ -28,7 +28,6 @@ CPUState *tcg_current_cpu;
>  /* exit the current TB from a signal handler. The host registers are
>     restored in a state compatible with the CPU emulator
>   */
> -#if defined(CONFIG_SOFTMMU)
>  void cpu_resume_from_signal(CPUState *cpu, void *puc)
>  {
>      /* XXX: restore cpu registers saved in host registers */
> @@ -37,6 +36,7 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
>      siglongjmp(cpu->jmp_env, 1);
>  }
>  
> +#if defined(CONFIG_SOFTMMU)
>  void cpu_reloading_memory_map(void)
>  {
>      if (qemu_in_vcpu_thread()) {
> diff --git a/translate-all.c b/translate-all.c
> index 4820d2e..52a571e 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1955,7 +1955,7 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
>              /* unprotect the page if it was put read-only because it
>                 contains translated code */
>              if (!(p->flags & PAGE_WRITE)) {
> -                if (!page_unprotect(addr, 0, NULL)) {
> +                if (!page_unprotect(addr, 0)) {
>                      return -1;
>                  }
>              }
> @@ -1965,8 +1965,12 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
>  }
>  
>  /* called from signal handler: invalidate the code and unprotect the
> -   page. Return TRUE if the fault was successfully handled. */
> -int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
> + * page. Return 0 if the fault was not handled, 1 if it was handled,
> + * and 2 if it was handled but the caller must cause the TB to be
> + * immediately exited. (We can only return 2 if the 'pc' argument is
> + * non-zero.)
> + */
> +int page_unprotect(target_ulong address, uintptr_t pc)
>  {
>      unsigned int prot;
>      PageDesc *p;
> @@ -1999,7 +2003,7 @@ int page_unprotect(target_ulong address, uintptr_t pc, void *puc)
>                 the corresponding translated code. */
>              if (tb_invalidate_phys_page(addr, pc)) {
>                  mmap_unlock();
> -                cpu_resume_from_signal(current_cpu, puc);
> +                return 2;
>              }
>  #ifdef DEBUG_TB_CHECK
>              tb_invalidate_check(addr);
> diff --git a/translate-all.h b/translate-all.h
> index 0384640..ce6071b 100644
> --- a/translate-all.h
> +++ b/translate-all.h
> @@ -27,7 +27,7 @@ void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end);
>  void tb_check_watchpoint(CPUState *cpu);
>  
>  #ifdef CONFIG_USER_ONLY
> -int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
> +int page_unprotect(target_ulong address, uintptr_t pc);
>  #endif
>  
>  #endif /* TRANSLATE_ALL_H */
> diff --git a/user-exec.c b/user-exec.c
> index d8d597b..1d02e24 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -54,7 +54,7 @@ static void exception_action(CPUState *cpu)
>  /* exit the current TB from a signal handler. The host registers are
>     restored in a state compatible with the CPU emulator
>   */
> -void cpu_resume_from_signal(CPUState *cpu, void *puc)
> +static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
>  {
>  #ifdef __linux__
>      struct ucontext *uc = puc;
> @@ -62,20 +62,18 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
>      struct sigcontext *uc = puc;
>  #endif
>  
> -    if (puc) {
> -        /* XXX: use siglongjmp ? */
> +    /* XXX: use siglongjmp ? */
>  #ifdef __linux__
>  #ifdef __ia64
> -        sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
> +    sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
>  #else
> -        sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
> +    sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
>  #endif
>  #elif defined(__OpenBSD__)
> -        sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
> +    sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
>  #endif
> -    }
> -    cpu->exception_index = -1;
> -    siglongjmp(cpu->jmp_env, 1);
> +
> +    cpu_resume_from_signal(cpu, NULL);
>  }
>  
>  /* 'pc' is the host PC at which the exception was raised. 'address' is
> @@ -95,9 +93,28 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>             pc, address, is_write, *(unsigned long *)old_set);
>  #endif
>      /* XXX: locking issue */
> -    if (is_write && h2g_valid(address)
> -        && page_unprotect(h2g(address), pc, puc)) {
> -        return 1;
> +    if (is_write && h2g_valid(address)) {
> +        switch (page_unprotect(h2g(address), pc)) {
> +        case 0:
> +            /* Fault not caused by a page marked unwritable to protect
> +             * cached translations, must be the guest binary's problem
> +             */
> +            break;
> +        case 1:
> +            /* Fault caused by protection of cached translation; TBs
> +             * invalidated, so resume execution
> +             */
> +            return 1;
> +        case 2:
> +            /* Fault caused by protection of cached translation, and the
> +             * currently executing TB was modified and must be exited
> +             * immediately.
> +             */
> +            cpu_exit_tb_from_sighandler(current_cpu, puc);
> +            g_assert_not_reached();
> +        default:
> +            g_assert_not_reached();
> +        }
>      }
>  
>      /* Convert forcefully to guest address space, invalid addresses

[Qemu-devel] [PATCH 3/5] cpu-exec: Rename cpu_resume_from_signal() to cpu_loop_exit_noexc()

[Peter Maydell]

The function cpu_resume_from_signal() is now always called with a
NULL puc argument, and is rather misnamed since it is never called
from a signal handler. It is essentially forcing an exit to the
top level cpu loop but without raising any exception, so rename
it to cpu_loop_exit_noexc() and drop the useless unused argument.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-exec-common.c        | 6 ++----
 exec.c                   | 2 +-
 hw/i386/kvmvapic.c       | 2 +-
 include/exec/exec-all.h  | 2 +-
 target-i386/bpt_helper.c | 2 +-
 target-lm32/helper.c     | 2 +-
 target-s390x/helper.c    | 2 +-
 target-xtensa/helper.c   | 2 +-
 translate-all.c          | 4 ++--
 user-exec.c              | 2 +-
 10 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index 62f5d6b..fac3aa7 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -25,10 +25,8 @@
 bool exit_request;
 CPUState *tcg_current_cpu;
 
-/* exit the current TB from a signal handler. The host registers are
-   restored in a state compatible with the CPU emulator
- */
-void cpu_resume_from_signal(CPUState *cpu, void *puc)
+/* exit the current TB, but without causing any exception to be raised */
+void cpu_loop_exit_noexc(CPUState *cpu)
 {
     /* XXX: restore cpu registers saved in host registers */
 
diff --git a/exec.c b/exec.c
index ee45472..c5c97ac 100644
--- a/exec.c
+++ b/exec.c
@@ -2121,7 +2121,7 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
                 } else {
                     cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
                     tb_gen_code(cpu, pc, cs_base, cpu_flags, 1);
-                    cpu_resume_from_signal(cpu, NULL);
+                    cpu_loop_exit_noexc(cpu);
                 }
             }
         } else {
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index f14445d..ee2f3fa 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -447,7 +447,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
 
     if (!kvm_enabled()) {
         tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
-        cpu_resume_from_signal(cs, NULL);
+        cpu_loop_exit_noexc(cs);
     }
 }
 
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 85528f9..1359f14 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -73,7 +73,7 @@ void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
 void cpu_gen_init(void);
 bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc);
 
-void QEMU_NORETURN cpu_resume_from_signal(CPUState *cpu, void *puc);
+void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
 void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
diff --git a/target-i386/bpt_helper.c b/target-i386/bpt_helper.c
index f47df19..458170e 100644
--- a/target-i386/bpt_helper.c
+++ b/target-i386/bpt_helper.c
@@ -217,7 +217,7 @@ void breakpoint_handler(CPUState *cs)
             if (check_hw_breakpoints(env, false)) {
                 raise_exception(env, EXCP01_DB);
             } else {
-                cpu_resume_from_signal(cs, NULL);
+                cpu_loop_exit_noexc(cs);
             }
         }
     } else {
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index 655248f..accfa7c 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -140,7 +140,7 @@ void lm32_debug_excp_handler(CPUState *cs)
             if (check_watchpoints(env)) {
                 raise_exception(env, EXCP_WATCHPOINT);
             } else {
-                cpu_resume_from_signal(cs, NULL);
+                cpu_loop_exit_noexc(cs);
             }
         }
     } else {
diff --git a/target-s390x/helper.c b/target-s390x/helper.c
index 92abe7e..f1e0a43 100644
--- a/target-s390x/helper.c
+++ b/target-s390x/helper.c
@@ -686,7 +686,7 @@ void s390x_cpu_debug_excp_handler(CPUState *cs)
            will be triggered, it will call load_psw which will recompute
            the watchpoints.  */
         cpu_watchpoint_remove_all(cs, BP_CPU);
-        cpu_resume_from_signal(cs, NULL);
+        cpu_loop_exit_noexc(cs);
     }
 }
 #endif /* CONFIG_USER_ONLY */
diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
index 839f4a7..768b32c 100644
--- a/target-xtensa/helper.c
+++ b/target-xtensa/helper.c
@@ -108,7 +108,7 @@ void xtensa_breakpoint_handler(CPUState *cs)
             if (cause) {
                 debug_exception_env(env, cause);
             }
-            cpu_resume_from_signal(cs, NULL);
+            cpu_loop_exit_noexc(cs);
         }
     }
 }
diff --git a/translate-all.c b/translate-all.c
index 52a571e..3a0d10f 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1394,7 +1394,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
            modifying the memory. It will ensure that it cannot modify
            itself */
         tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
-        cpu_resume_from_signal(cpu, NULL);
+        cpu_loop_exit_noexc(cpu);
     }
 #endif
 }
@@ -1654,7 +1654,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
        repeating the fault, which is horribly inefficient.
        Better would be to execute just this insn uncached, or generate a
        second new TB.  */
-    cpu_resume_from_signal(cpu, NULL);
+    cpu_loop_exit_noexc(cpu);
 }
 
 void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr)
diff --git a/user-exec.c b/user-exec.c
index 1d02e24..40b5e7c 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -73,7 +73,7 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
     sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
 #endif
 
-    cpu_resume_from_signal(cpu, NULL);
+    cpu_loop_exit_noexc(cpu);
 }
 
 /* 'pc' is the host PC at which the exception was raised. 'address' is
-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/5] cpu-exec: Rename cpu_resume_from_signal() to cpu_loop_exit_noexc()

[Sergey Fedorov]

On 16/05/16 19:09, Peter Maydell wrote:
> The function cpu_resume_from_signal() is now always called with a
> NULL puc argument, and is rather misnamed since it is never called
> from a signal handler. It is essentially forcing an exit to the
> top level cpu loop but without raising any exception, so rename
> it to cpu_loop_exit_noexc() and drop the useless unused argument.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>

> ---
>  cpu-exec-common.c        | 6 ++----
>  exec.c                   | 2 +-
>  hw/i386/kvmvapic.c       | 2 +-
>  include/exec/exec-all.h  | 2 +-
>  target-i386/bpt_helper.c | 2 +-
>  target-lm32/helper.c     | 2 +-
>  target-s390x/helper.c    | 2 +-
>  target-xtensa/helper.c   | 2 +-
>  translate-all.c          | 4 ++--
>  user-exec.c              | 2 +-
>  10 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/cpu-exec-common.c b/cpu-exec-common.c
> index 62f5d6b..fac3aa7 100644
> --- a/cpu-exec-common.c
> +++ b/cpu-exec-common.c
> @@ -25,10 +25,8 @@
>  bool exit_request;
>  CPUState *tcg_current_cpu;
>  
> -/* exit the current TB from a signal handler. The host registers are
> -   restored in a state compatible with the CPU emulator
> - */
> -void cpu_resume_from_signal(CPUState *cpu, void *puc)
> +/* exit the current TB, but without causing any exception to be raised */
> +void cpu_loop_exit_noexc(CPUState *cpu)
>  {
>      /* XXX: restore cpu registers saved in host registers */
>  
> diff --git a/exec.c b/exec.c
> index ee45472..c5c97ac 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2121,7 +2121,7 @@ static void check_watchpoint(int offset, int len, MemTxAttrs attrs, int flags)
>                  } else {
>                      cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
>                      tb_gen_code(cpu, pc, cs_base, cpu_flags, 1);
> -                    cpu_resume_from_signal(cpu, NULL);
> +                    cpu_loop_exit_noexc(cpu);
>                  }
>              }
>          } else {
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index f14445d..ee2f3fa 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -447,7 +447,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>  
>      if (!kvm_enabled()) {
>          tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
> -        cpu_resume_from_signal(cs, NULL);
> +        cpu_loop_exit_noexc(cs);
>      }
>  }
>  
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 85528f9..1359f14 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -73,7 +73,7 @@ void restore_state_to_opc(CPUArchState *env, struct TranslationBlock *tb,
>  void cpu_gen_init(void);
>  bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc);
>  
> -void QEMU_NORETURN cpu_resume_from_signal(CPUState *cpu, void *puc);
> +void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
>  void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
>  TranslationBlock *tb_gen_code(CPUState *cpu,
>                                target_ulong pc, target_ulong cs_base,
> diff --git a/target-i386/bpt_helper.c b/target-i386/bpt_helper.c
> index f47df19..458170e 100644
> --- a/target-i386/bpt_helper.c
> +++ b/target-i386/bpt_helper.c
> @@ -217,7 +217,7 @@ void breakpoint_handler(CPUState *cs)
>              if (check_hw_breakpoints(env, false)) {
>                  raise_exception(env, EXCP01_DB);
>              } else {
> -                cpu_resume_from_signal(cs, NULL);
> +                cpu_loop_exit_noexc(cs);
>              }
>          }
>      } else {
> diff --git a/target-lm32/helper.c b/target-lm32/helper.c
> index 655248f..accfa7c 100644
> --- a/target-lm32/helper.c
> +++ b/target-lm32/helper.c
> @@ -140,7 +140,7 @@ void lm32_debug_excp_handler(CPUState *cs)
>              if (check_watchpoints(env)) {
>                  raise_exception(env, EXCP_WATCHPOINT);
>              } else {
> -                cpu_resume_from_signal(cs, NULL);
> +                cpu_loop_exit_noexc(cs);
>              }
>          }
>      } else {
> diff --git a/target-s390x/helper.c b/target-s390x/helper.c
> index 92abe7e..f1e0a43 100644
> --- a/target-s390x/helper.c
> +++ b/target-s390x/helper.c
> @@ -686,7 +686,7 @@ void s390x_cpu_debug_excp_handler(CPUState *cs)
>             will be triggered, it will call load_psw which will recompute
>             the watchpoints.  */
>          cpu_watchpoint_remove_all(cs, BP_CPU);
> -        cpu_resume_from_signal(cs, NULL);
> +        cpu_loop_exit_noexc(cs);
>      }
>  }
>  #endif /* CONFIG_USER_ONLY */
> diff --git a/target-xtensa/helper.c b/target-xtensa/helper.c
> index 839f4a7..768b32c 100644
> --- a/target-xtensa/helper.c
> +++ b/target-xtensa/helper.c
> @@ -108,7 +108,7 @@ void xtensa_breakpoint_handler(CPUState *cs)
>              if (cause) {
>                  debug_exception_env(env, cause);
>              }
> -            cpu_resume_from_signal(cs, NULL);
> +            cpu_loop_exit_noexc(cs);
>          }
>      }
>  }
> diff --git a/translate-all.c b/translate-all.c
> index 52a571e..3a0d10f 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1394,7 +1394,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>             modifying the memory. It will ensure that it cannot modify
>             itself */
>          tb_gen_code(cpu, current_pc, current_cs_base, current_flags, 1);
> -        cpu_resume_from_signal(cpu, NULL);
> +        cpu_loop_exit_noexc(cpu);
>      }
>  #endif
>  }
> @@ -1654,7 +1654,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
>         repeating the fault, which is horribly inefficient.
>         Better would be to execute just this insn uncached, or generate a
>         second new TB.  */
> -    cpu_resume_from_signal(cpu, NULL);
> +    cpu_loop_exit_noexc(cpu);
>  }
>  
>  void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr)
> diff --git a/user-exec.c b/user-exec.c
> index 1d02e24..40b5e7c 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -73,7 +73,7 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
>      sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
>  #endif
>  
> -    cpu_resume_from_signal(cpu, NULL);
> +    cpu_loop_exit_noexc(cpu);
>  }
>  
>  /* 'pc' is the host PC at which the exception was raised. 'address' is

[Qemu-devel] [PATCH 4/5] user-exec: Don't reextract sigmask from usercontext pointer

[Peter Maydell]

Extracting the old signal mask from the usercontext pointer passed to
a signal handler is a pain because it is OS and CPU dependent.
Since we've already done it once and passed it to handle_cpu_signal(),
there's no need to do it again in cpu_exit_tb_from_sighandler().
This then means we don't need to pass a usercontext pointer in to
handle_cpu_signal() at all.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 user-exec.c | 48 ++++++++++++++++--------------------------------
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index 40b5e7c..ad669f4 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -54,25 +54,10 @@ static void exception_action(CPUState *cpu)
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
-static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
+static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set)
 {
-#ifdef __linux__
-    struct ucontext *uc = puc;
-#elif defined(__OpenBSD__)
-    struct sigcontext *uc = puc;
-#endif
-
     /* XXX: use siglongjmp ? */
-#ifdef __linux__
-#ifdef __ia64
-    sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
-#else
-    sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
-#endif
-#elif defined(__OpenBSD__)
-    sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
-#endif
-
+    sigprocmask(SIG_SETMASK, old_set, NULL);
     cpu_loop_exit_noexc(cpu);
 }
 
@@ -81,8 +66,7 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
    write caused the exception and otherwise 0'. 'old_set' is the
    signal set which should be restored */
 static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
-                                    int is_write, sigset_t *old_set,
-                                    void *puc)
+                                    int is_write, sigset_t *old_set)
 {
     CPUState *cpu;
     CPUClass *cc;
@@ -110,7 +94,7 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
              * currently executing TB was modified and must be exited
              * immediately.
              */
-            cpu_exit_tb_from_sighandler(current_cpu, puc);
+            cpu_exit_tb_from_sighandler(current_cpu, old_set);
             g_assert_not_reached();
         default:
             g_assert_not_reached();
@@ -204,7 +188,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
                              trapno == 0xe ?
                              (ERROR_sig(uc) >> 1) & 1 : 0,
-                             &MASK_sig(uc), puc);
+                             &MASK_sig(uc));
 }
 
 #elif defined(__x86_64__)
@@ -250,7 +234,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
                              TRAP_sig(uc) == 0xe ?
                              (ERROR_sig(uc) >> 1) & 1 : 0,
-                             &MASK_sig(uc), puc);
+                             &MASK_sig(uc));
 }
 
 #elif defined(_ARCH_PPC)
@@ -366,7 +350,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     }
 #endif
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__alpha__)
@@ -397,7 +381,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     }
 
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 #elif defined(__sparc__)
 
@@ -457,7 +441,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
         }
     }
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, sigmask, NULL);
+                             is_write, sigmask);
 }
 
 #elif defined(__arm__)
@@ -492,7 +476,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     is_write = extract32(uc->uc_mcontext.error_code, 11, 1);
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
                              is_write,
-                             &uc->uc_sigmask, puc);
+                             &uc->uc_sigmask);
 }
 
 #elif defined(__aarch64__)
@@ -520,7 +504,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
                 || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */
 
     return handle_cpu_signal(pc, (uintptr_t)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__mc68000)
@@ -538,7 +522,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     is_write = 0;
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
                              is_write,
-                             &uc->uc_sigmask, puc);
+                             &uc->uc_sigmask);
 }
 
 #elif defined(__ia64)
@@ -573,7 +557,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
     }
     return handle_cpu_signal(ip, (unsigned long)info->si_addr,
                              is_write,
-                             (sigset_t *)&uc->uc_sigmask, puc);
+                             (sigset_t *)&uc->uc_sigmask);
 }
 
 #elif defined(__s390__)
@@ -626,7 +610,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
         break;
     }
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__mips__)
@@ -642,7 +626,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     /* XXX: compute is_write */
     is_write = 0;
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 
 #elif defined(__hppa__)
@@ -684,7 +668,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
     }
 
     return handle_cpu_signal(pc, (unsigned long)info->si_addr,
-                             is_write, &uc->uc_sigmask, puc);
+                             is_write, &uc->uc_sigmask);
 }
 
 #else
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/5] user-exec: Don't reextract sigmask from usercontext pointer

[Sergey Fedorov]

On 16/05/16 19:09, Peter Maydell wrote:
> Extracting the old signal mask from the usercontext pointer passed to
> a signal handler is a pain because it is OS and CPU dependent.
> Since we've already done it once and passed it to handle_cpu_signal(),
> there's no need to do it again in cpu_exit_tb_from_sighandler().
> This then means we don't need to pass a usercontext pointer in to
> handle_cpu_signal() at all.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <sergey.fedorov@linaro.org>

> ---
>  user-exec.c | 48 ++++++++++++++++--------------------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/user-exec.c b/user-exec.c
> index 40b5e7c..ad669f4 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -54,25 +54,10 @@ static void exception_action(CPUState *cpu)
>  /* exit the current TB from a signal handler. The host registers are
>     restored in a state compatible with the CPU emulator
>   */
> -static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
> +static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set)
>  {
> -#ifdef __linux__
> -    struct ucontext *uc = puc;
> -#elif defined(__OpenBSD__)
> -    struct sigcontext *uc = puc;
> -#endif
> -
>      /* XXX: use siglongjmp ? */
> -#ifdef __linux__
> -#ifdef __ia64
> -    sigprocmask(SIG_SETMASK, (sigset_t *)&uc->uc_sigmask, NULL);
> -#else
> -    sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
> -#endif
> -#elif defined(__OpenBSD__)
> -    sigprocmask(SIG_SETMASK, &uc->sc_mask, NULL);
> -#endif
> -
> +    sigprocmask(SIG_SETMASK, old_set, NULL);
>      cpu_loop_exit_noexc(cpu);
>  }
>  
> @@ -81,8 +66,7 @@ static void cpu_exit_tb_from_sighandler(CPUState *cpu, void *puc)
>     write caused the exception and otherwise 0'. 'old_set' is the
>     signal set which should be restored */
>  static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
> -                                    int is_write, sigset_t *old_set,
> -                                    void *puc)
> +                                    int is_write, sigset_t *old_set)
>  {
>      CPUState *cpu;
>      CPUClass *cc;
> @@ -110,7 +94,7 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>               * currently executing TB was modified and must be exited
>               * immediately.
>               */
> -            cpu_exit_tb_from_sighandler(current_cpu, puc);
> +            cpu_exit_tb_from_sighandler(current_cpu, old_set);
>              g_assert_not_reached();
>          default:
>              g_assert_not_reached();
> @@ -204,7 +188,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
>                               trapno == 0xe ?
>                               (ERROR_sig(uc) >> 1) & 1 : 0,
> -                             &MASK_sig(uc), puc);
> +                             &MASK_sig(uc));
>  }
>  
>  #elif defined(__x86_64__)
> @@ -250,7 +234,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
>                               TRAP_sig(uc) == 0xe ?
>                               (ERROR_sig(uc) >> 1) & 1 : 0,
> -                             &MASK_sig(uc), puc);
> +                             &MASK_sig(uc));
>  }
>  
>  #elif defined(_ARCH_PPC)
> @@ -366,7 +350,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      }
>  #endif
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  
>  #elif defined(__alpha__)
> @@ -397,7 +381,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      }
>  
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  #elif defined(__sparc__)
>  
> @@ -457,7 +441,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>          }
>      }
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, sigmask, NULL);
> +                             is_write, sigmask);
>  }
>  
>  #elif defined(__arm__)
> @@ -492,7 +476,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      is_write = extract32(uc->uc_mcontext.error_code, 11, 1);
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
>                               is_write,
> -                             &uc->uc_sigmask, puc);
> +                             &uc->uc_sigmask);
>  }
>  
>  #elif defined(__aarch64__)
> @@ -520,7 +504,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
>                  || (insn & 0x3a400000) == 0x28000000); /* C3.3.7,14-16 */
>  
>      return handle_cpu_signal(pc, (uintptr_t)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  
>  #elif defined(__mc68000)
> @@ -538,7 +522,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      is_write = 0;
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
>                               is_write,
> -                             &uc->uc_sigmask, puc);
> +                             &uc->uc_sigmask);
>  }
>  
>  #elif defined(__ia64)
> @@ -573,7 +557,7 @@ int cpu_signal_handler(int host_signum, void *pinfo, void *puc)
>      }
>      return handle_cpu_signal(ip, (unsigned long)info->si_addr,
>                               is_write,
> -                             (sigset_t *)&uc->uc_sigmask, puc);
> +                             (sigset_t *)&uc->uc_sigmask);
>  }
>  
>  #elif defined(__s390__)
> @@ -626,7 +610,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>          break;
>      }
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  
>  #elif defined(__mips__)
> @@ -642,7 +626,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      /* XXX: compute is_write */
>      is_write = 0;
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  
>  #elif defined(__hppa__)
> @@ -684,7 +668,7 @@ int cpu_signal_handler(int host_signum, void *pinfo,
>      }
>  
>      return handle_cpu_signal(pc, (unsigned long)info->si_addr,
> -                             is_write, &uc->uc_sigmask, puc);
> +                             is_write, &uc->uc_sigmask);
>  }
>  
>  #else

[Qemu-devel] [PATCH 5/5] target-i386: Move user-mode exception actions out of user-exec.c

[Peter Maydell]

The exception_action() function in user-exec.c is just a call to
cpu_loop_exit() for every target CPU except i386.  Since this
function is only called if the target's handle_mmu_fault() hook has
indicated an MMU fault, and that hook is only called from the
handle_cpu_signal() code path, we can simply move the x86-specific
setup into that hook, which allows us to remove the TARGET_I386
ifdef from user-exec.c.

Of the actions that were done by the call to raise_interrupt_err():
 * cpu_svm_check_intercept_param() is a no-op in user mode
 * check_exception() is a no-op since double faults are impossible
   for user-mode
 * assignments to cs->exception_index and env->error_code are no-ops
 * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since
   pc is 0
which leaves just setting env_>exception_is_int and
env->exception_next_eip as actions that need to be added to
x86_cpu_handle_mmu_fault().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-i386/helper.c |  2 ++
 user-exec.c          | 16 +---------------
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf3e762..e1dde46 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
     env->error_code = (is_write << PG_ERROR_W_BIT);
     env->error_code |= PG_ERROR_U_MASK;
     cs->exception_index = EXCP0E_PAGE;
+    env->exception_is_int = 0;
+    env->exception_next_eip = env->eip;
     return 1;
 }
 
diff --git a/user-exec.c b/user-exec.c
index ad669f4..439bb37 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -39,18 +39,6 @@
 
 //#define DEBUG_SIGNAL
 
-static void exception_action(CPUState *cpu)
-{
-#if defined(TARGET_I386)
-    X86CPU *x86_cpu = X86_CPU(cpu);
-    CPUX86State *env1 = &x86_cpu->env;
-
-    raise_exception_err(env1, cpu->exception_index, env1->error_code);
-#else
-    cpu_loop_exit(cpu);
-#endif
-}
-
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
@@ -119,10 +107,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
     /* now we have a real cpu fault */
     cpu_restore_state(cpu, pc);
 
-    /* we restore the process signal mask as the sigreturn should
-       do it (XXX: use sigsetjmp) */
     sigprocmask(SIG_SETMASK, old_set, NULL);
-    exception_action(cpu);
+    cpu_loop_exit(cpu);
 
     /* never comes here */
     return 1;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/5] target-i386: Move user-mode exception actions out of user-exec.c

[Sergey Fedorov]

On 16/05/16 19:09, Peter Maydell wrote:
> The exception_action() function in user-exec.c is just a call to
> cpu_loop_exit() for every target CPU except i386.  Since this
> function is only called if the target's handle_mmu_fault() hook has
> indicated an MMU fault, and that hook is only called from the
> handle_cpu_signal() code path, we can simply move the x86-specific
> setup into that hook, which allows us to remove the TARGET_I386
> ifdef from user-exec.c.
>
> Of the actions that were done by the call to raise_interrupt_err():
>  * cpu_svm_check_intercept_param() is a no-op in user mode
>  * check_exception() is a no-op since double faults are impossible
>    for user-mode
>  * assignments to cs->exception_index and env->error_code are no-ops
>  * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since
>    pc is 0
> which leaves just setting env_>exception_is_int and
> env->exception_next_eip as actions that need to be added to
> x86_cpu_handle_mmu_fault().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-i386/helper.c |  2 ++
>  user-exec.c          | 16 +---------------
>  2 files changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf3e762..e1dde46 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
>      env->error_code = (is_write << PG_ERROR_W_BIT);
>      env->error_code |= PG_ERROR_U_MASK;
>      cs->exception_index = EXCP0E_PAGE;
> +    env->exception_is_int = 0;
> +    env->exception_next_eip = env->eip;

'env->eip' was updated by restore_state_to_opc() from
cpu_restore_state_from_tb() from cpu_restore_state() from
handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_
calling exception_action().

Kind regards,
Sergey

>      return 1;
>  }
>  
> diff --git a/user-exec.c b/user-exec.c
> index ad669f4..439bb37 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -39,18 +39,6 @@
>  
>  //#define DEBUG_SIGNAL
>  
> -static void exception_action(CPUState *cpu)
> -{
> -#if defined(TARGET_I386)
> -    X86CPU *x86_cpu = X86_CPU(cpu);
> -    CPUX86State *env1 = &x86_cpu->env;
> -
> -    raise_exception_err(env1, cpu->exception_index, env1->error_code);
> -#else
> -    cpu_loop_exit(cpu);
> -#endif
> -}
> -
>  /* exit the current TB from a signal handler. The host registers are
>     restored in a state compatible with the CPU emulator
>   */
> @@ -119,10 +107,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>      /* now we have a real cpu fault */
>      cpu_restore_state(cpu, pc);
>  
> -    /* we restore the process signal mask as the sigreturn should
> -       do it (XXX: use sigsetjmp) */
>      sigprocmask(SIG_SETMASK, old_set, NULL);
> -    exception_action(cpu);
> +    cpu_loop_exit(cpu);
>  
>      /* never comes here */
>      return 1;

Re: [Qemu-devel] [PATCH 5/5] target-i386: Move user-mode exception actions out of user-exec.c

[Peter Maydell]

On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 16/05/16 19:09, Peter Maydell wrote:
>> The exception_action() function in user-exec.c is just a call to
>> cpu_loop_exit() for every target CPU except i386.  Since this
>> function is only called if the target's handle_mmu_fault() hook has
>> indicated an MMU fault, and that hook is only called from the
>> handle_cpu_signal() code path, we can simply move the x86-specific
>> setup into that hook, which allows us to remove the TARGET_I386
>> ifdef from user-exec.c.
>>
>> Of the actions that were done by the call to raise_interrupt_err():
>>  * cpu_svm_check_intercept_param() is a no-op in user mode
>>  * check_exception() is a no-op since double faults are impossible
>>    for user-mode
>>  * assignments to cs->exception_index and env->error_code are no-ops
>>  * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since
>>    pc is 0
>> which leaves just setting env_>exception_is_int and
>> env->exception_next_eip as actions that need to be added to
>> x86_cpu_handle_mmu_fault().
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-i386/helper.c |  2 ++
>>  user-exec.c          | 16 +---------------
>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index bf3e762..e1dde46 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
>>      env->error_code = (is_write << PG_ERROR_W_BIT);
>>      env->error_code |= PG_ERROR_U_MASK;
>>      cs->exception_index = EXCP0E_PAGE;
>> +    env->exception_is_int = 0;
>> +    env->exception_next_eip = env->eip;
>
> 'env->eip' was updated by restore_state_to_opc() from
> cpu_restore_state_from_tb() from cpu_restore_state() from
> handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_
> calling exception_action().

Oops, nice catch. (I wonder if any of the other target architectures
are not correctly doing things in their handle_mmu_fault function
because the cpu_restore_state() call happens later?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/5] target-i386: Move user-mode exception actions out of user-exec.c

[Peter Maydell]

On 16 May 2016 at 19:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> 'env->eip' was updated by restore_state_to_opc() from
>> cpu_restore_state_from_tb() from cpu_restore_state() from
>> handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_
>> calling exception_action().
>
> Oops, nice catch. (I wonder if any of the other target architectures
> are not correctly doing things in their handle_mmu_fault function
> because the cpu_restore_state() call happens later?)

Looking at the other target architectures they're OK because
they don't do very much in the handle_mmu_fault function.
Since every single handle_mmu_fault function always returns 1
(ignoring one or two clearly softmmu-only versions) we could
in theory call cpu_restore_state() before the handle_mmu_fault
hook. However since in the softmmu case the equivalent code
is also called in a pre-restore-state setup it seems more
consistent to keep the user-exec.c code the order it is now.
So the target-i386 code needs rearranging a bit I guess
(perhaps to save the offset rather than the actual next eip?)

I think patches 1..4 are still worthwhile even if we drop
this one for now, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/5] target-i386: Move user-mode exception actions out of user-exec.c

[Peter Maydell]

On 16 May 2016 at 18:54, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 16/05/16 19:09, Peter Maydell wrote:
>> The exception_action() function in user-exec.c is just a call to
>> cpu_loop_exit() for every target CPU except i386.  Since this
>> function is only called if the target's handle_mmu_fault() hook has
>> indicated an MMU fault, and that hook is only called from the
>> handle_cpu_signal() code path, we can simply move the x86-specific
>> setup into that hook, which allows us to remove the TARGET_I386
>> ifdef from user-exec.c.
>>
>> Of the actions that were done by the call to raise_interrupt_err():
>>  * cpu_svm_check_intercept_param() is a no-op in user mode
>>  * check_exception() is a no-op since double faults are impossible
>>    for user-mode
>>  * assignments to cs->exception_index and env->error_code are no-ops
>>  * cpu_loop_exit_restore() is equivalent to cpu_loop_exit() since
>>    pc is 0
>> which leaves just setting env_>exception_is_int and
>> env->exception_next_eip as actions that need to be added to
>> x86_cpu_handle_mmu_fault().
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target-i386/helper.c |  2 ++
>>  user-exec.c          | 16 +---------------
>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index bf3e762..e1dde46 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -700,6 +700,8 @@ int x86_cpu_handle_mmu_fault(CPUState *cs, vaddr addr,
>>      env->error_code = (is_write << PG_ERROR_W_BIT);
>>      env->error_code |= PG_ERROR_U_MASK;
>>      cs->exception_index = EXCP0E_PAGE;
>> +    env->exception_is_int = 0;
>> +    env->exception_next_eip = env->eip;
>
> 'env->eip' was updated by restore_state_to_opc() from
> cpu_restore_state_from_tb() from cpu_restore_state() from
> handle_cpu_signal() _after_ calling 'handle_mmu_fault' hook but _before_
> calling exception_action().

I looked a bit closer at this, and I think that we're actually
OK to just not set exception_next_eip (or to set it wrongly,
according to taste). In the softmmu equivalent code path,
tlb_fill() will call raise_exception_err_ra() for a page
fault. This code path also ends up doing
  env->exception_next_eip = env->eip
before it has done a cpu_restore_state() [in this case the
restore-state happens in cpu_loop_exit_restore()]. But this
is OK because we only use env->exception_next_eip as the
value to pass into do_interrupt_all(), which states that
next_eip is only valid if is_int is true (ie we got here from
an int instruction or syscall).

For do_interrupt_user() it is also true that next_eip only
matters if is_int (or if into is EXCP_SYSCALL), so we can
safely just not update exception_next_eip here in
x86_cpu_handle_mmu_fault().

So I think the best approach is:
 (1) in this patch, set exception_next_eip to -1 (clearly
indicating that it is not supposed to be valid)
 (2) add a comment to do_interrupt_user() like the one for
do_interrupt_all() noting that next_eip is only valid in
some cases.

thanks
-- PMM