Re: [RFC PATCH 3/7] accel/tcg: Split out handle_sigsegv_accerr_write

2021-09-13 Thread Philippe Mathieu-Daudé
On 9/14/21 12:05 AM, Richard Henderson wrote:
> This is the major portion of handle_cpu_signal which is specific
> to tcg, handling the page protections for the translations.
> Most of the rest will migrate to linux-user/ shortly.
> 
> Signed-off-by: Richard Henderson 
> ---
>  include/exec/exec-all.h | 12 ++
>  accel/tcg/user-exec.c   | 96 +
>  2 files changed, 72 insertions(+), 36 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 



[RFC PATCH 3/7] accel/tcg: Split out handle_sigsegv_accerr_write

2021-09-13 Thread Richard Henderson
This is the major portion of handle_cpu_signal which is specific
to tcg, handling the page protections for the translations.
Most of the rest will migrate to linux-user/ shortly.

Signed-off-by: Richard Henderson 
---
 include/exec/exec-all.h | 12 ++
 accel/tcg/user-exec.c   | 96 +
 2 files changed, 72 insertions(+), 36 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 7207912306..f582d3e688 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -671,6 +671,18 @@ static inline tb_page_addr_t 
get_page_addr_code_hostp(CPUArchState *env,
  */
 uintptr_t adjust_signal_pc(uintptr_t pc);
 
+/**
+ * handle_sigsegv_accerr_write:
+ * @cpu: the cpu context
+ * @old_set: the sigset_t from the signal ucontext_t
+ * @host_pc: the host pc, adjusted for the signal
+ * @host_addr: the host address of the fault
+ *
+ * Return true if the write fault has been handled, and should be re-tried.
+ */
+bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t *old_set,
+ uintptr_t host_pc, uintptr_t host_addr);
+
 /**
  * cpu_signal_handler
  * @signum: host signal number
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 1f7b7a3692..daef34a426 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -112,6 +112,60 @@ uintptr_t adjust_signal_pc(uintptr_t pc)
 }
 }
 
+/**
+ * handle_sigsegv_accerr_write:
+ * @cpu: the cpu context
+ * @old_set: the sigset_t from the signal ucontext_t
+ * @host_pc: the host pc, adjusted for the signal
+ * @host_addr: the host address of the fault
+ *
+ * Return true if the write fault has been handled, and should be re-tried.
+ *
+ * Note that it is important that we don't call page_unprotect() unless
+ * this is really a "write to nonwriteable page" fault, because
+ * page_unprotect() assumes that if it is called for an access to
+ * a page that's writeable this means we had two threads racing and
+ * another thread got there first and already made the page writeable;
+ * so we will retry the access. If we were to call page_unprotect()
+ * for some other kind of fault that should really be passed to the
+ * guest, we'd end up in an infinite loop of retrying the faulting access.
+ */
+bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t *old_set,
+ uintptr_t host_pc, uintptr_t host_addr)
+{
+if (!h2g_valid(host_addr)) {
+return false;
+}
+
+switch (page_unprotect(h2g(host_addr), host_pc)) {
+case 0:
+/*
+ * Fault not caused by a page marked unwritable to protect
+ * cached translations, must be the guest binary's problem.
+ */
+return false;
+case 1:
+/*
+ * Fault caused by protection of cached translation; TBs
+ * invalidated, so resume execution.  Retain helper_retaddr
+ * for a possible second fault.
+ */
+return true;
+case 2:
+/*
+ * Fault caused by protection of cached translation, and the
+ * currently executing TB was modified and must be exited
+ * immediately.  Clear helper_retaddr for next execution.
+ */
+clear_helper_retaddr();
+cpu_exit_tb_from_sighandler(cpu, old_set);
+/* NORETURN */
+
+default:
+g_assert_not_reached();
+}
+}
+
 /* 'pc' is the host PC at which the exception was raised. 'address' is
the effective address of the memory exception. 'is_write' is 1 if a
write caused the exception and otherwise 0'. 'old_set' is the
@@ -150,43 +204,13 @@ static inline int handle_cpu_signal(uintptr_t pc, 
siginfo_t *info,
 printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
pc, address, is_write, *(unsigned long *)old_set);
 #endif
-/* XXX: locking issue */
-/* Note that it is important that we don't call page_unprotect() unless
- * this is really a "write to nonwriteable page" fault, because
- * page_unprotect() assumes that if it is called for an access to
- * a page that's writeable this means we had two threads racing and
- * another thread got there first and already made the page writeable;
- * so we will retry the access. If we were to call page_unprotect()
- * for some other kind of fault that should really be passed to the
- * guest, we'd end up in an infinite loop of retrying the faulting
- * access.
- */
-if (is_write && info->si_signo == SIGSEGV && info->si_code == SEGV_ACCERR 
&&
-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.  Retain helper_re