diff mbox series

[v2,20/41] linux-user: Add raise_sigsegv

Message ID 20210918184527.408540-21-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show
Series linux-user: Streamline handling of SIGSEGV | expand

Commit Message

Richard Henderson Sept. 18, 2021, 6:45 p.m. UTC
This is a new interface to be provided by the os emulator
for raising SIGSEGV on fault.  Use the new record_sigsegv
target hook.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/exec-all.h | 15 +++++++++++++++
 accel/tcg/user-exec.c   | 33 ++++++++++++++++++---------------
 linux-user/signal.c     | 30 ++++++++++++++++++++++--------
 3 files changed, 55 insertions(+), 23 deletions(-)

Comments

Philippe Mathieu-Daudé Sept. 19, 2021, 6:26 p.m. UTC | #1
On 9/18/21 20:45, Richard Henderson wrote:
> This is a new interface to be provided by the os emulator
> for raising SIGSEGV on fault.  Use the new record_sigsegv
> target hook.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h | 15 +++++++++++++++
>  accel/tcg/user-exec.c   | 33 ++++++++++++++++++---------------
>  linux-user/signal.c     | 30 ++++++++++++++++++++++--------
>  3 files changed, 55 insertions(+), 23 deletions(-)

To the best of my knowledge:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Richard Henderson Sept. 19, 2021, 6:35 p.m. UTC | #2
On 9/18/21 11:45 AM, Richard Henderson wrote:
> +/**
> + * raise_sigsegv:
> + * @cpu: the cpu context
> + * @addr: the guest address of the fault
> + * @access_type: access was read/write/execute
> + * @maperr: true for invalid page, false for permission fault
> + * @ra: host pc for unwinding
> + *
> + * Use the TCGCPUOps hook to record cpu state, do guest operating system
> + * specific things to raise SIGSEGV, and jump to the main cpu loop.
> + */
> +void QEMU_NORETURN raise_sigsegv(CPUState *cpu, target_ulong addr,
> +                                 MMUAccessType access_type,
> +                                 bool maperr, uintptr_t ra);

FYI, something to bikeshed here is the name of the function.  Should it in fact be 
cpu_loop_exit_raise_sigsegv?

Because it can't be used outside of the running cpu context.  (E.g. there are a couple of 
instances where it's tempting to use this from within cpu_loop itself, processing 
pseudo-syscalls.)


r~
Philippe Mathieu-Daudé Sept. 19, 2021, 6:43 p.m. UTC | #3
On 9/19/21 20:35, Richard Henderson wrote:
> On 9/18/21 11:45 AM, Richard Henderson wrote:
>> +/**
>> + * raise_sigsegv:
>> + * @cpu: the cpu context
>> + * @addr: the guest address of the fault
>> + * @access_type: access was read/write/execute
>> + * @maperr: true for invalid page, false for permission fault
>> + * @ra: host pc for unwinding
>> + *
>> + * Use the TCGCPUOps hook to record cpu state, do guest operating system
>> + * specific things to raise SIGSEGV, and jump to the main cpu loop.
>> + */
>> +void QEMU_NORETURN raise_sigsegv(CPUState *cpu, target_ulong addr,
>> +                                 MMUAccessType access_type,
>> +                                 bool maperr, uintptr_t ra);
> 
> FYI, something to bikeshed here is the name of the function.  Should it
> in fact be cpu_loop_exit_raise_sigsegv?

That or cpu_loop_exit_segv() which is explicit enough IMO.

> Because it can't be used outside of the running cpu context.  (E.g.
> there are a couple of instances where it's tempting to use this from
> within cpu_loop itself, processing pseudo-syscalls.)
> 
> 
> r~
>
Warner Losh Sept. 19, 2021, 6:53 p.m. UTC | #4
> On Sep 19, 2021, at 12:43 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> On 9/19/21 20:35, Richard Henderson wrote:
>> On 9/18/21 11:45 AM, Richard Henderson wrote:
>>> +/**
>>> + * raise_sigsegv:
>>> + * @cpu: the cpu context
>>> + * @addr: the guest address of the fault
>>> + * @access_type: access was read/write/execute
>>> + * @maperr: true for invalid page, false for permission fault
>>> + * @ra: host pc for unwinding
>>> + *
>>> + * Use the TCGCPUOps hook to record cpu state, do guest operating system
>>> + * specific things to raise SIGSEGV, and jump to the main cpu loop.
>>> + */
>>> +void QEMU_NORETURN raise_sigsegv(CPUState *cpu, target_ulong addr,
>>> +                                 MMUAccessType access_type,
>>> +                                 bool maperr, uintptr_t ra);
>> 
>> FYI, something to bikeshed here is the name of the function.  Should it
>> in fact be cpu_loop_exit_raise_sigsegv?
> 
> That or cpu_loop_exit_segv() which is explicit enough IMO.

That name works for me…

Also, and this is a stretch so consider it maybe a bit weak…

Reviewed by: Warner Losh <imp@bsdimp.com>


> Because it can't be used outside of the running cpu context.  (E.g.
>> 
>> there are a couple of instances where it's tempting to use this from
>> within cpu_loop itself, processing pseudo-syscalls.)
>> 
>> 
>> r~
>> 
> 
>
diff mbox series

Patch

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5dd663c153..2091c1bf1a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -685,6 +685,21 @@  MMUAccessType adjust_signal_pc(uintptr_t *pc, bool is_write);
 bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t *old_set,
                                  uintptr_t host_pc, abi_ptr guest_addr);
 
+/**
+ * raise_sigsegv:
+ * @cpu: the cpu context
+ * @addr: the guest address of the fault
+ * @access_type: access was read/write/execute
+ * @maperr: true for invalid page, false for permission fault
+ * @ra: host pc for unwinding
+ *
+ * Use the TCGCPUOps hook to record cpu state, do guest operating system
+ * specific things to raise SIGSEGV, and jump to the main cpu loop.
+ */
+void QEMU_NORETURN raise_sigsegv(CPUState *cpu, target_ulong addr,
+                                 MMUAccessType access_type,
+                                 bool maperr, uintptr_t ra);
+
 #else
 static inline void mmap_lock(void) {}
 static inline void mmap_unlock(void) {}
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 01e7e69e7f..ab9cc6686d 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -143,35 +143,38 @@  static int probe_access_internal(CPUArchState *env, target_ulong addr,
                                  int fault_size, MMUAccessType access_type,
                                  bool nonfault, uintptr_t ra)
 {
-    int flags;
+    bool maperr = true;
+    int acc_flag;
 
     switch (access_type) {
     case MMU_DATA_STORE:
-        flags = PAGE_WRITE;
+        acc_flag = PAGE_WRITE_ORG;
         break;
     case MMU_DATA_LOAD:
-        flags = PAGE_READ;
+        acc_flag = PAGE_READ;
         break;
     case MMU_INST_FETCH:
-        flags = PAGE_EXEC;
+        acc_flag = PAGE_EXEC;
         break;
     default:
         g_assert_not_reached();
     }
 
-    if (!guest_addr_valid_untagged(addr) ||
-        page_check_range(addr, 1, flags) < 0) {
-        if (nonfault) {
-            return TLB_INVALID_MASK;
-        } else {
-            CPUState *cpu = env_cpu(env);
-            CPUClass *cc = CPU_GET_CLASS(cpu);
-            cc->tcg_ops->tlb_fill(cpu, addr, fault_size, access_type,
-                                  MMU_USER_IDX, false, ra);
-            g_assert_not_reached();
+    if (guest_addr_valid_untagged(addr)) {
+        int page_flags = page_get_flags(addr);
+        if (page_flags & acc_flag) {
+            return 0; /* success */
+        }
+        if (page_flags & PAGE_VALID) {
+            maperr = false;
         }
     }
-    return 0;
+
+    if (nonfault) {
+        return TLB_INVALID_MASK;
+    }
+
+    raise_sigsegv(env_cpu(env), addr, access_type, maperr, ra);
 }
 
 int probe_access_flags(CPUArchState *env, target_ulong addr,
diff --git a/linux-user/signal.c b/linux-user/signal.c
index e6531fdfa0..ae31b46be0 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -681,9 +681,27 @@  void force_sigsegv(int oldsig)
     }
     force_sig(TARGET_SIGSEGV);
 }
-
 #endif
 
+void raise_sigsegv(CPUState *cpu, target_ulong addr,
+                   MMUAccessType access_type, bool maperr, uintptr_t ra)
+{
+    const struct TCGCPUOps *tcg_ops = CPU_GET_CLASS(cpu)->tcg_ops;
+
+    if (tcg_ops->record_sigsegv) {
+        tcg_ops->record_sigsegv(cpu, addr, access_type, maperr, ra);
+    } else if (tcg_ops->tlb_fill) {
+        tcg_ops->tlb_fill(cpu, addr, 0, access_type, MMU_USER_IDX, false, ra);
+        g_assert_not_reached();
+    }
+
+    force_sig_fault(TARGET_SIGSEGV,
+                    maperr ? TARGET_SEGV_MAPERR : TARGET_SEGV_ACCERR,
+                    addr);
+    cpu->exception_index = EXCP_INTERRUPT;
+    cpu_loop_exit_restore(cpu, ra);
+}
+
 /* abort execution with signal */
 static void QEMU_NORETURN dump_core_and_abort(int target_sig)
 {
@@ -799,7 +817,7 @@  static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
         access_type = adjust_signal_pc(&pc, is_write);
 
         if (host_sig == SIGSEGV) {
-            const struct TCGCPUOps *tcg_ops;
+            bool maperr = true;
 
             if (info->si_code == SEGV_ACCERR && h2g_valid(host_addr)) {
                 /* If this was a write to a TB protected page, restart. */
@@ -814,18 +832,14 @@  static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
                  * which means that we may get ACCERR when we want MAPERR.
                  */
                 if (page_get_flags(guest_addr) & PAGE_VALID) {
-                    /* maperr = false; */
+                    maperr = false;
                 } else {
                     info->si_code = SEGV_MAPERR;
                 }
             }
 
             sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
-
-            tcg_ops = CPU_GET_CLASS(cpu)->tcg_ops;
-            tcg_ops->tlb_fill(cpu, guest_addr, 0, access_type,
-                              MMU_USER_IDX, false, pc);
-            g_assert_not_reached();
+            raise_sigsegv(cpu, guest_addr, access_type, maperr, pc);
         } else {
             sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
         }