qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC 0/4] linux-user: simplify safe signal handling
@ 2021-11-08  2:37 Warner Losh
  2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Warner Losh @ 2021-11-08  2:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Laurent Vivier, Warner Losh, Philippe Mathieu-Daude

This is a quick RFC to see if something like this is worth doing.

I've created a new interface host_signal_set_pc. This allows us to move all the
nearly identical copies of rewind_if_in_safe_syscall into signal.c.  This
reduces the amount of code that needs to be rewritten for bsd-user's adaptation
of both the safe signal handling and the sigsegv/sigbus changes that have
happened. Since BSD's mcontext_t differs in some cases, we wouldn't be able to
share this between platforms, but it reduces the number of nearly identical
routines I'd have to write.

In addition, the assembler glue for the safe system calls is almost identical
between linux and bsd-user's fork. The only difference is inverting the system
call return to comply with the -ERRNO convention *-user uses in the rest of the
code which is native to Linux, but differs for the BSDs and other traditional
unix targets.

I know the patches may not be sliced and diced in the typical desired
fashion. This is a RFC, and the changes are short enough to be easily digested
though since it's quite repetitive.

These were extracted from the 'blitz' branch we have in the bsd-user fork and
then that was adapted to use the common code. I've pushed a branch to gitlab
(viewable at https://gitlab.com/bsdimp/qemu/-/tree/blitz if you prefer that to
fetching) that shows how these will be used. I'm working on upstreaming
bsd-user/signal.c next which will take a little bit of time to work into a place
where it can be reviewed here. I wanted to get feedback because this is
one chunk I can cleave off and make landing that easier.

Warner Losh (4):
  linux-user: Add host_signal_set_pc to set pc in mcontext
  linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  linux-user/safe-syscall.inc.S: Move to common-user
  common-user: Allow return codes to be adjusted after sytsem call

 .../host/aarch64/safe-syscall.inc.S           |  1 +
 .../host/arm/safe-syscall.inc.S               |  1 +
 .../host/i386/safe-syscall.inc.S              |  1 +
 .../host/ppc64/safe-syscall.inc.S             |  1 +
 .../host/riscv/safe-syscall.inc.S             |  1 +
 .../host/s390x/safe-syscall.inc.S             |  1 +
 .../host/x86_64/safe-syscall.inc.S            |  1 +
 linux-user/host/aarch64/host-signal.h         |  5 +++++
 linux-user/host/aarch64/hostdep.h             | 20 -------------------
 linux-user/host/alpha/host-signal.h           |  5 +++++
 linux-user/host/arm/host-signal.h             |  5 +++++
 linux-user/host/arm/hostdep.h                 | 20 -------------------
 linux-user/host/i386/host-signal.h            |  5 +++++
 linux-user/host/i386/hostdep.h                | 20 -------------------
 linux-user/host/mips/host-signal.h            |  5 +++++
 linux-user/host/ppc/host-signal.h             |  5 +++++
 linux-user/host/ppc64/hostdep.h               | 20 -------------------
 linux-user/host/riscv/host-signal.h           |  5 +++++
 linux-user/host/riscv/hostdep.h               | 20 -------------------
 linux-user/host/s390/host-signal.h            |  5 +++++
 linux-user/host/s390x/hostdep.h               | 20 -------------------
 linux-user/host/sparc/host-signal.h           |  9 +++++++++
 linux-user/host/x86_64/host-signal.h          |  5 +++++
 linux-user/host/x86_64/hostdep.h              | 20 -------------------
 linux-user/safe-syscall.S                     |  1 +
 linux-user/signal.c                           | 18 ++++++++++++++++-
 meson.build                                   |  1 +
 27 files changed, 80 insertions(+), 141 deletions(-)
 rename {linux-user => common-user}/host/aarch64/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/arm/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/i386/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/ppc64/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/riscv/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/s390x/safe-syscall.inc.S (99%)
 rename {linux-user => common-user}/host/x86_64/safe-syscall.inc.S (99%)

-- 
2.33.0



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext
  2021-11-08  2:37 [RFC 0/4] linux-user: simplify safe signal handling Warner Losh
@ 2021-11-08  2:37 ` Warner Losh
  2021-11-08 15:03   ` Richard Henderson
  2021-11-10 15:43   ` Philippe Mathieu-Daudé
  2021-11-08  2:37 ` [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall Warner Losh
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 16+ messages in thread
From: Warner Losh @ 2021-11-08  2:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Laurent Vivier, Warner Losh, Philippe Mathieu-Daude

Add a new function host_signal_set_pc to set the next pc in an
mcontext. The caller should ensure this is a valid PC for execution.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 linux-user/host/aarch64/host-signal.h | 5 +++++
 linux-user/host/alpha/host-signal.h   | 5 +++++
 linux-user/host/arm/host-signal.h     | 5 +++++
 linux-user/host/i386/host-signal.h    | 5 +++++
 linux-user/host/mips/host-signal.h    | 5 +++++
 linux-user/host/ppc/host-signal.h     | 5 +++++
 linux-user/host/riscv/host-signal.h   | 5 +++++
 linux-user/host/s390/host-signal.h    | 5 +++++
 linux-user/host/sparc/host-signal.h   | 9 +++++++++
 linux-user/host/x86_64/host-signal.h  | 5 +++++
 10 files changed, 54 insertions(+)

diff --git a/linux-user/host/aarch64/host-signal.h b/linux-user/host/aarch64/host-signal.h
index 0c0b08383a..9770b36dc1 100644
--- a/linux-user/host/aarch64/host-signal.h
+++ b/linux-user/host/aarch64/host-signal.h
@@ -35,6 +35,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.pc;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.pc = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     struct _aarch64_ctx *hdr;
diff --git a/linux-user/host/alpha/host-signal.h b/linux-user/host/alpha/host-signal.h
index e080be412f..f4c942948a 100644
--- a/linux-user/host/alpha/host-signal.h
+++ b/linux-user/host/alpha/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.sc_pc;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.sc_pc = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint32_t *pc = (uint32_t *)host_signal_pc(uc);
diff --git a/linux-user/host/arm/host-signal.h b/linux-user/host/arm/host-signal.h
index efb165c0c5..6c095773c0 100644
--- a/linux-user/host/arm/host-signal.h
+++ b/linux-user/host/arm/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.arm_pc;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.arm_pc = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     /*
diff --git a/linux-user/host/i386/host-signal.h b/linux-user/host/i386/host-signal.h
index 4c8eef99ce..abe1ece5c9 100644
--- a/linux-user/host/i386/host-signal.h
+++ b/linux-user/host/i386/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.gregs[REG_EIP];
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.gregs[REG_EIP] = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.gregs[REG_TRAPNO] == 0xe
diff --git a/linux-user/host/mips/host-signal.h b/linux-user/host/mips/host-signal.h
index ef341f7c20..c666ed8c3f 100644
--- a/linux-user/host/mips/host-signal.h
+++ b/linux-user/host/mips/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.pc;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.pc = pc;
+}
+
 #if defined(__misp16) || defined(__mips_micromips)
 #error "Unsupported encoding"
 #endif
diff --git a/linux-user/host/ppc/host-signal.h b/linux-user/host/ppc/host-signal.h
index a491c413dc..1d8e658ff7 100644
--- a/linux-user/host/ppc/host-signal.h
+++ b/linux-user/host/ppc/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.regs->nip;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.regs->nip = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.regs->trap != 0x400
diff --git a/linux-user/host/riscv/host-signal.h b/linux-user/host/riscv/host-signal.h
index 3b168cb58b..a4f170efb0 100644
--- a/linux-user/host/riscv/host-signal.h
+++ b/linux-user/host/riscv/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.__gregs[REG_PC];
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.__gregs[REG_PC] = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     /*
diff --git a/linux-user/host/s390/host-signal.h b/linux-user/host/s390/host-signal.h
index 26990e4893..a524f2ab00 100644
--- a/linux-user/host/s390/host-signal.h
+++ b/linux-user/host/s390/host-signal.h
@@ -16,6 +16,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.psw.addr;
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.psw.addr = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint16_t *pinsn = (uint16_t *)host_signal_pc(uc);
diff --git a/linux-user/host/sparc/host-signal.h b/linux-user/host/sparc/host-signal.h
index 5e71d33f8e..e301c2c1ea 100644
--- a/linux-user/host/sparc/host-signal.h
+++ b/linux-user/host/sparc/host-signal.h
@@ -20,6 +20,15 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
 #endif
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+#ifdef __arch64__
+    uc->uc_mcontext.mc_gregs[MC_PC] = pc;
+#else
+    &uc->uc_mcontext.gregs[REG_PC] = pc;
+#endif
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint32_t insn = *(uint32_t *)host_signal_pc(uc);
diff --git a/linux-user/host/x86_64/host-signal.h b/linux-user/host/x86_64/host-signal.h
index 883d2fcf65..c71d597eb2 100644
--- a/linux-user/host/x86_64/host-signal.h
+++ b/linux-user/host/x86_64/host-signal.h
@@ -15,6 +15,11 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
     return uc->uc_mcontext.gregs[REG_RIP];
 }
 
+static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
+{
+    uc->uc_mcontext.gregs[REG_RIP] = pc;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.gregs[REG_TRAPNO] == 0xe
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-08  2:37 [RFC 0/4] linux-user: simplify safe signal handling Warner Losh
  2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
@ 2021-11-08  2:37 ` Warner Losh
  2021-11-08 15:07   ` Richard Henderson
  2021-11-10 15:44   ` Philippe Mathieu-Daudé
  2021-11-08  2:37 ` [RFC 3/4] linux-user/safe-syscall.inc.S: Move to common-user Warner Losh
  2021-11-08  2:37 ` [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call Warner Losh
  3 siblings, 2 replies; 16+ messages in thread
From: Warner Losh @ 2021-11-08  2:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Laurent Vivier, Warner Losh, Philippe Mathieu-Daude

All instances of rewind_if_in_safe_syscall are the same, differing only
in how the instruction point is fetched from the ucontext and the size
of the registers. Use host_signal_pc and new host_signal_set_pc
interfaces to fetch the pointer to the PC and adjust if needed. Delete
all the old copies of rewind_if_in_safe_syscall.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 linux-user/host/aarch64/hostdep.h | 20 --------------------
 linux-user/host/arm/hostdep.h     | 20 --------------------
 linux-user/host/i386/hostdep.h    | 20 --------------------
 linux-user/host/ppc64/hostdep.h   | 20 --------------------
 linux-user/host/riscv/hostdep.h   | 20 --------------------
 linux-user/host/s390x/hostdep.h   | 20 --------------------
 linux-user/host/x86_64/hostdep.h  | 20 --------------------
 linux-user/signal.c               | 18 +++++++++++++++++-
 8 files changed, 17 insertions(+), 141 deletions(-)

diff --git a/linux-user/host/aarch64/hostdep.h b/linux-user/host/aarch64/hostdep.h
index a8d41a21ad..39299d798a 100644
--- a/linux-user/host/aarch64/hostdep.h
+++ b/linux-user/host/aarch64/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    __u64 *pcreg = &uc->uc_mcontext.pc;
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/arm/hostdep.h b/linux-user/host/arm/hostdep.h
index 9276fe6ceb..86b137875a 100644
--- a/linux-user/host/arm/hostdep.h
+++ b/linux-user/host/arm/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    unsigned long *pcreg = &uc->uc_mcontext.arm_pc;
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/i386/hostdep.h b/linux-user/host/i386/hostdep.h
index 073be74d87..ce7136501f 100644
--- a/linux-user/host/i386/hostdep.h
+++ b/linux-user/host/i386/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    greg_t *pcreg = &uc->uc_mcontext.gregs[REG_EIP];
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/ppc64/hostdep.h b/linux-user/host/ppc64/hostdep.h
index 98979ad917..0c290dd904 100644
--- a/linux-user/host/ppc64/hostdep.h
+++ b/linux-user/host/ppc64/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    unsigned long *pcreg = &uc->uc_mcontext.gp_regs[PT_NIP];
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/riscv/hostdep.h b/linux-user/host/riscv/hostdep.h
index 2ba07456ae..7f67c22868 100644
--- a/linux-user/host/riscv/hostdep.h
+++ b/linux-user/host/riscv/hostdep.h
@@ -11,24 +11,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    unsigned long *pcreg = &uc->uc_mcontext.__gregs[REG_PC];
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/s390x/hostdep.h b/linux-user/host/s390x/hostdep.h
index 4f0171f36f..d801145854 100644
--- a/linux-user/host/s390x/hostdep.h
+++ b/linux-user/host/s390x/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    unsigned long *pcreg = &uc->uc_mcontext.psw.addr;
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/host/x86_64/hostdep.h b/linux-user/host/x86_64/hostdep.h
index a4fefb5114..9c62bd26bd 100644
--- a/linux-user/host/x86_64/hostdep.h
+++ b/linux-user/host/x86_64/hostdep.h
@@ -15,24 +15,4 @@
 /* We have a safe-syscall.inc.S */
 #define HAVE_SAFE_SYSCALL
 
-#ifndef __ASSEMBLER__
-
-/* These are defined by the safe-syscall.inc.S file */
-extern char safe_syscall_start[];
-extern char safe_syscall_end[];
-
-/* Adjust the signal context to rewind out of safe-syscall if we're in it */
-static inline void rewind_if_in_safe_syscall(void *puc)
-{
-    ucontext_t *uc = puc;
-    greg_t *pcreg = &uc->uc_mcontext.gregs[REG_RIP];
-
-    if (*pcreg > (uintptr_t)safe_syscall_start
-        && *pcreg < (uintptr_t)safe_syscall_end) {
-        *pcreg = (uintptr_t)safe_syscall_start;
-    }
-}
-
-#endif /* __ASSEMBLER__ */
-
 #endif
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 81c45bfce9..dafdf46b93 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -793,7 +793,23 @@ int queue_signal(CPUArchState *env, int sig, int si_type,
     return 1; /* indicates that the signal was queued */
 }
 
-#ifndef HAVE_SAFE_SYSCALL
+#ifdef HAVE_SAFE_SYSCALL
+/* These are defined by the safe-syscall.inc.S file */
+extern char safe_syscall_start[];
+extern char safe_syscall_end[];
+
+/* Adjust the signal context to rewind out of safe-syscall if we're in it */
+static inline void rewind_if_in_safe_syscall(void *puc)
+{
+    ucontext_t *uc = (ucontext_t *)puc;
+    uintptr_t pcreg = host_signal_pc(uc);
+
+    if (pcreg > (uintptr_t)safe_syscall_start
+        && pcreg < (uintptr_t)safe_syscall_end) {
+        host_signal_set_pc(uc, (uintptr_t)safe_syscall_start);
+    }
+}
+#else
 static inline void rewind_if_in_safe_syscall(void *puc)
 {
     /* Default version: never rewind */
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC 3/4] linux-user/safe-syscall.inc.S: Move to common-user
  2021-11-08  2:37 [RFC 0/4] linux-user: simplify safe signal handling Warner Losh
  2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
  2021-11-08  2:37 ` [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall Warner Losh
@ 2021-11-08  2:37 ` Warner Losh
  2021-11-08  2:37 ` [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call Warner Losh
  3 siblings, 0 replies; 16+ messages in thread
From: Warner Losh @ 2021-11-08  2:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Laurent Vivier, Warner Losh, Philippe Mathieu-Daude

Move all the safe_syscall.inc.S files to common-user. They are almost
identical between linux-user and bsd-user to re-use.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 {linux-user => common-user}/host/aarch64/safe-syscall.inc.S | 0
 {linux-user => common-user}/host/arm/safe-syscall.inc.S     | 0
 {linux-user => common-user}/host/i386/safe-syscall.inc.S    | 0
 {linux-user => common-user}/host/ppc64/safe-syscall.inc.S   | 0
 {linux-user => common-user}/host/riscv/safe-syscall.inc.S   | 0
 {linux-user => common-user}/host/s390x/safe-syscall.inc.S   | 0
 {linux-user => common-user}/host/x86_64/safe-syscall.inc.S  | 0
 meson.build                                                 | 1 +
 8 files changed, 1 insertion(+)
 rename {linux-user => common-user}/host/aarch64/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/arm/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/i386/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/ppc64/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/riscv/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/s390x/safe-syscall.inc.S (100%)
 rename {linux-user => common-user}/host/x86_64/safe-syscall.inc.S (100%)

diff --git a/linux-user/host/aarch64/safe-syscall.inc.S b/common-user/host/aarch64/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/aarch64/safe-syscall.inc.S
rename to common-user/host/aarch64/safe-syscall.inc.S
diff --git a/linux-user/host/arm/safe-syscall.inc.S b/common-user/host/arm/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/arm/safe-syscall.inc.S
rename to common-user/host/arm/safe-syscall.inc.S
diff --git a/linux-user/host/i386/safe-syscall.inc.S b/common-user/host/i386/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/i386/safe-syscall.inc.S
rename to common-user/host/i386/safe-syscall.inc.S
diff --git a/linux-user/host/ppc64/safe-syscall.inc.S b/common-user/host/ppc64/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/ppc64/safe-syscall.inc.S
rename to common-user/host/ppc64/safe-syscall.inc.S
diff --git a/linux-user/host/riscv/safe-syscall.inc.S b/common-user/host/riscv/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/riscv/safe-syscall.inc.S
rename to common-user/host/riscv/safe-syscall.inc.S
diff --git a/linux-user/host/s390x/safe-syscall.inc.S b/common-user/host/s390x/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/s390x/safe-syscall.inc.S
rename to common-user/host/s390x/safe-syscall.inc.S
diff --git a/linux-user/host/x86_64/safe-syscall.inc.S b/common-user/host/x86_64/safe-syscall.inc.S
similarity index 100%
rename from linux-user/host/x86_64/safe-syscall.inc.S
rename to common-user/host/x86_64/safe-syscall.inc.S
diff --git a/meson.build b/meson.build
index 26c58123e9..6f7acc8936 100644
--- a/meson.build
+++ b/meson.build
@@ -2883,6 +2883,7 @@ foreach target : target_dirs
     if 'CONFIG_LINUX_USER' in config_target
       base_dir = 'linux-user'
       target_inc += include_directories('linux-user/host/' / config_host['ARCH'])
+      target_inc += include_directories('common-user/host/' / config_host['ARCH'])
     endif
     if 'CONFIG_BSD_USER' in config_target
       base_dir = 'bsd-user'
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call
  2021-11-08  2:37 [RFC 0/4] linux-user: simplify safe signal handling Warner Losh
                   ` (2 preceding siblings ...)
  2021-11-08  2:37 ` [RFC 3/4] linux-user/safe-syscall.inc.S: Move to common-user Warner Losh
@ 2021-11-08  2:37 ` Warner Losh
  2021-11-08 15:10   ` Richard Henderson
  3 siblings, 1 reply; 16+ messages in thread
From: Warner Losh @ 2021-11-08  2:37 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Laurent Vivier, Warner Losh, Philippe Mathieu-Daude

All the *-users generally use the Linux style of negative return codes
for errno. However, other systems, like FreeBSD, have a different
convention. Allow those systems to insert code after the syscall that
adjusts the return value of the system call to match the native linux
format.

Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 common-user/host/aarch64/safe-syscall.inc.S | 1 +
 common-user/host/arm/safe-syscall.inc.S     | 1 +
 common-user/host/i386/safe-syscall.inc.S    | 1 +
 common-user/host/ppc64/safe-syscall.inc.S   | 1 +
 common-user/host/riscv/safe-syscall.inc.S   | 1 +
 common-user/host/s390x/safe-syscall.inc.S   | 1 +
 common-user/host/x86_64/safe-syscall.inc.S  | 1 +
 linux-user/safe-syscall.S                   | 1 +
 8 files changed, 8 insertions(+)

diff --git a/common-user/host/aarch64/safe-syscall.inc.S b/common-user/host/aarch64/safe-syscall.inc.S
index bc1f5a9792..81d83e8e79 100644
--- a/common-user/host/aarch64/safe-syscall.inc.S
+++ b/common-user/host/aarch64/safe-syscall.inc.S
@@ -64,6 +64,7 @@ safe_syscall_start:
 	svc	0x0
 safe_syscall_end:
 	/* code path for having successfully executed the syscall */
+	ADJUST_SYSCALL_RETCODE
 	ret
 
 0:
diff --git a/common-user/host/arm/safe-syscall.inc.S b/common-user/host/arm/safe-syscall.inc.S
index 88c4958504..40e9a5e28d 100644
--- a/common-user/host/arm/safe-syscall.inc.S
+++ b/common-user/host/arm/safe-syscall.inc.S
@@ -78,6 +78,7 @@ safe_syscall_start:
 	swi	0
 safe_syscall_end:
 	/* code path for having successfully executed the syscall */
+	ADJUST_SYSCALL_RETCODE
 	pop	{ r4, r5, r6, r7, r8, pc }
 
 1:
diff --git a/common-user/host/i386/safe-syscall.inc.S b/common-user/host/i386/safe-syscall.inc.S
index 9e58fc6504..eb6b43bd81 100644
--- a/common-user/host/i386/safe-syscall.inc.S
+++ b/common-user/host/i386/safe-syscall.inc.S
@@ -75,6 +75,7 @@ safe_syscall_start:
 	int	$0x80
 safe_syscall_end:
 	/* code path for having successfully executed the syscall */
+	ADJUST_SYSCALL_RETCODE
 	pop	%ebx
 	.cfi_remember_state
 	.cfi_adjust_cfa_offset -4
diff --git a/common-user/host/ppc64/safe-syscall.inc.S b/common-user/host/ppc64/safe-syscall.inc.S
index 875133173b..974bd03f8d 100644
--- a/common-user/host/ppc64/safe-syscall.inc.S
+++ b/common-user/host/ppc64/safe-syscall.inc.S
@@ -75,6 +75,7 @@ safe_syscall_start:
 	sc
 safe_syscall_end:
 	/* code path when we did execute the syscall */
+	ADJUST_SYSCALL_RETCODE
 	ld 14, 16(1) /* restore r14 to its original value */
 	bnslr+
 
diff --git a/common-user/host/riscv/safe-syscall.inc.S b/common-user/host/riscv/safe-syscall.inc.S
index 9ca3fbfd1e..a4bd5c5c72 100644
--- a/common-user/host/riscv/safe-syscall.inc.S
+++ b/common-user/host/riscv/safe-syscall.inc.S
@@ -66,6 +66,7 @@ safe_syscall_start:
 	scall
 safe_syscall_end:
 	/* code path for having successfully executed the syscall */
+	ADJUST_SYSCALL_RETCODE
 	ret
 
 0:
diff --git a/common-user/host/s390x/safe-syscall.inc.S b/common-user/host/s390x/safe-syscall.inc.S
index 414b44ad38..4ba60fbed0 100644
--- a/common-user/host/s390x/safe-syscall.inc.S
+++ b/common-user/host/s390x/safe-syscall.inc.S
@@ -76,6 +76,7 @@ safe_syscall_start:
 	jne	2f
 	svc	0
 safe_syscall_end:
+	ADJUST_SYSCALL_RETCODE
 
 1:	lg	%r15,0(%r15)		/* load back chain */
 	.cfi_remember_state
diff --git a/common-user/host/x86_64/safe-syscall.inc.S b/common-user/host/x86_64/safe-syscall.inc.S
index f36992daa3..e1ae6f83e6 100644
--- a/common-user/host/x86_64/safe-syscall.inc.S
+++ b/common-user/host/x86_64/safe-syscall.inc.S
@@ -72,6 +72,7 @@ safe_syscall_start:
         syscall
 safe_syscall_end:
         /* code path for having successfully executed the syscall */
+	ADJUST_SYSCALL_RETCODE
         pop     %rbp
         .cfi_remember_state
         .cfi_def_cfa_offset 8
diff --git a/linux-user/safe-syscall.S b/linux-user/safe-syscall.S
index 42ea7c40ba..0d6dd19398 100644
--- a/linux-user/safe-syscall.S
+++ b/linux-user/safe-syscall.S
@@ -17,6 +17,7 @@
  * so that this will pull in the right fragment for the architecture.
  */
 #ifdef HAVE_SAFE_SYSCALL
+#define ADJUST_SYSCALL_RETCODE		/* No adjustment for linux */
 #include "safe-syscall.inc.S"
 #endif
 
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext
  2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
@ 2021-11-08 15:03   ` Richard Henderson
  2021-11-10 15:43   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 16+ messages in thread
From: Richard Henderson @ 2021-11-08 15:03 UTC (permalink / raw)
  To: Warner Losh, qemu-devel; +Cc: Laurent Vivier, Philippe Mathieu-Daude

On 11/8/21 3:37 AM, Warner Losh wrote:
> Add a new function host_signal_set_pc to set the next pc in an
> mcontext. The caller should ensure this is a valid PC for execution.
> 
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   linux-user/host/aarch64/host-signal.h | 5 +++++
>   linux-user/host/alpha/host-signal.h   | 5 +++++
>   linux-user/host/arm/host-signal.h     | 5 +++++
>   linux-user/host/i386/host-signal.h    | 5 +++++
>   linux-user/host/mips/host-signal.h    | 5 +++++
>   linux-user/host/ppc/host-signal.h     | 5 +++++
>   linux-user/host/riscv/host-signal.h   | 5 +++++
>   linux-user/host/s390/host-signal.h    | 5 +++++
>   linux-user/host/sparc/host-signal.h   | 9 +++++++++
>   linux-user/host/x86_64/host-signal.h  | 5 +++++
>   10 files changed, 54 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-08  2:37 ` [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall Warner Losh
@ 2021-11-08 15:07   ` Richard Henderson
  2021-11-08 16:39     ` Warner Losh
  2021-11-10 15:44   ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 16+ messages in thread
From: Richard Henderson @ 2021-11-08 15:07 UTC (permalink / raw)
  To: Warner Losh, qemu-devel; +Cc: Laurent Vivier, Philippe Mathieu-Daude

On 11/8/21 3:37 AM, Warner Losh wrote:
> All instances of rewind_if_in_safe_syscall are the same, differing only
> in how the instruction point is fetched from the ucontext and the size
> of the registers. Use host_signal_pc and new host_signal_set_pc
> interfaces to fetch the pointer to the PC and adjust if needed. Delete
> all the old copies of rewind_if_in_safe_syscall.
> 
> Signed-off-by: Warner Losh<imp@bsdimp.com>
> ---
>   linux-user/host/aarch64/hostdep.h | 20 --------------------
>   linux-user/host/arm/hostdep.h     | 20 --------------------
>   linux-user/host/i386/hostdep.h    | 20 --------------------
>   linux-user/host/ppc64/hostdep.h   | 20 --------------------
>   linux-user/host/riscv/hostdep.h   | 20 --------------------
>   linux-user/host/s390x/hostdep.h   | 20 --------------------
>   linux-user/host/x86_64/hostdep.h  | 20 --------------------
>   linux-user/signal.c               | 18 +++++++++++++++++-
>   8 files changed, 17 insertions(+), 141 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Although I think we can fairly safely drop HAVE_SAFE_SYSCALL.  It is required for proper 
operation.  As with host-signal.h, really.


r~


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call
  2021-11-08  2:37 ` [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call Warner Losh
@ 2021-11-08 15:10   ` Richard Henderson
  2021-11-08 18:49     ` Warner Losh
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Henderson @ 2021-11-08 15:10 UTC (permalink / raw)
  To: Warner Losh, qemu-devel; +Cc: Laurent Vivier, Philippe Mathieu-Daude

On 11/8/21 3:37 AM, Warner Losh wrote:
> All the *-users generally use the Linux style of negative return codes
> for errno. However, other systems, like FreeBSD, have a different
> convention. Allow those systems to insert code after the syscall that
> adjusts the return value of the system call to match the native linux
> format.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>   common-user/host/aarch64/safe-syscall.inc.S | 1 +
>   common-user/host/arm/safe-syscall.inc.S     | 1 +
>   common-user/host/i386/safe-syscall.inc.S    | 1 +
>   common-user/host/ppc64/safe-syscall.inc.S   | 1 +
>   common-user/host/riscv/safe-syscall.inc.S   | 1 +
>   common-user/host/s390x/safe-syscall.inc.S   | 1 +
>   common-user/host/x86_64/safe-syscall.inc.S  | 1 +
>   linux-user/safe-syscall.S                   | 1 +
>   8 files changed, 8 insertions(+)
> 
> diff --git a/common-user/host/aarch64/safe-syscall.inc.S b/common-user/host/aarch64/safe-syscall.inc.S
> index bc1f5a9792..81d83e8e79 100644
> --- a/common-user/host/aarch64/safe-syscall.inc.S
> +++ b/common-user/host/aarch64/safe-syscall.inc.S
> @@ -64,6 +64,7 @@ safe_syscall_start:
>   	svc	0x0
>   safe_syscall_end:
>   	/* code path for having successfully executed the syscall */
> +	ADJUST_SYSCALL_RETCODE
>   	ret
>   
>   0:

Not sure about this, really.  Is it really that much cleaner to insert this than create 
separate 10-line files, with the adjustment included?


r~


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-08 15:07   ` Richard Henderson
@ 2021-11-08 16:39     ` Warner Losh
  2021-11-10 16:20       ` Warner Losh
  0 siblings, 1 reply; 16+ messages in thread
From: Warner Losh @ 2021-11-08 16:39 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Philippe Mathieu-Daude, QEMU Developers, Laurent Vivier

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

On Mon, Nov 8, 2021 at 8:07 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/8/21 3:37 AM, Warner Losh wrote:
> > All instances of rewind_if_in_safe_syscall are the same, differing only
> > in how the instruction point is fetched from the ucontext and the size
> > of the registers. Use host_signal_pc and new host_signal_set_pc
> > interfaces to fetch the pointer to the PC and adjust if needed. Delete
> > all the old copies of rewind_if_in_safe_syscall.
> >
> > Signed-off-by: Warner Losh<imp@bsdimp.com>
> > ---
> >   linux-user/host/aarch64/hostdep.h | 20 --------------------
> >   linux-user/host/arm/hostdep.h     | 20 --------------------
> >   linux-user/host/i386/hostdep.h    | 20 --------------------
> >   linux-user/host/ppc64/hostdep.h   | 20 --------------------
> >   linux-user/host/riscv/hostdep.h   | 20 --------------------
> >   linux-user/host/s390x/hostdep.h   | 20 --------------------
> >   linux-user/host/x86_64/hostdep.h  | 20 --------------------
> >   linux-user/signal.c               | 18 +++++++++++++++++-
> >   8 files changed, 17 insertions(+), 141 deletions(-)
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Although I think we can fairly safely drop HAVE_SAFE_SYSCALL.  It is
> required for proper
> operation.  As with host-signal.h, really.
>

Yes. The only possible use I can see for it is to allow people to bring up
new platforms more easily by deferring the signal-safe system call details
until later in that process.

Warner

[-- Attachment #2: Type: text/html, Size: 2166 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call
  2021-11-08 15:10   ` Richard Henderson
@ 2021-11-08 18:49     ` Warner Losh
  2021-11-09  8:11       ` Richard Henderson
  0 siblings, 1 reply; 16+ messages in thread
From: Warner Losh @ 2021-11-08 18:49 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Philippe Mathieu-Daude, QEMU Developers, Laurent Vivier

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Mon, Nov 8, 2021 at 8:10 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/8/21 3:37 AM, Warner Losh wrote:
> > All the *-users generally use the Linux style of negative return codes
> > for errno. However, other systems, like FreeBSD, have a different
> > convention. Allow those systems to insert code after the syscall that
> > adjusts the return value of the system call to match the native linux
> > format.
> >
> > Signed-off-by: Warner Losh <imp@bsdimp.com>
> > ---
> >   common-user/host/aarch64/safe-syscall.inc.S | 1 +
> >   common-user/host/arm/safe-syscall.inc.S     | 1 +
> >   common-user/host/i386/safe-syscall.inc.S    | 1 +
> >   common-user/host/ppc64/safe-syscall.inc.S   | 1 +
> >   common-user/host/riscv/safe-syscall.inc.S   | 1 +
> >   common-user/host/s390x/safe-syscall.inc.S   | 1 +
> >   common-user/host/x86_64/safe-syscall.inc.S  | 1 +
> >   linux-user/safe-syscall.S                   | 1 +
> >   8 files changed, 8 insertions(+)
> >
> > diff --git a/common-user/host/aarch64/safe-syscall.inc.S
> b/common-user/host/aarch64/safe-syscall.inc.S
> > index bc1f5a9792..81d83e8e79 100644
> > --- a/common-user/host/aarch64/safe-syscall.inc.S
> > +++ b/common-user/host/aarch64/safe-syscall.inc.S
> > @@ -64,6 +64,7 @@ safe_syscall_start:
> >       svc     0x0
> >   safe_syscall_end:
> >       /* code path for having successfully executed the syscall */
> > +     ADJUST_SYSCALL_RETCODE
> >       ret
> >
> >   0:
>
> Not sure about this, really.  Is it really that much cleaner to insert
> this than create
> separate 10-line files, with the adjustment included?
>

While the meat of these files is only around 10 lines, the files themselves
are more like 70 lines with a lot of extra marking to ensure proper
integration with toolchains. Such lines have a tendency, over time, to need
adjusting as new toolchains change the requirements slightly (clang
certainly has forced that in a number of places in FreeBSD's code base, and
every new version seems to require something). The adjustments have all
been 3 lines (gmail seems to hate my formatting):

+#define        ADJUST_SYSCALL_RETCODE \
+    jnb 2f;                    \
+    neg %rax;                  \
+    2:

which is significantly easier to maintain than having to monitor these
files for changes and copying over the changes that happen. Plus, as I'm
upstreaming the arch support, it's one more file I have to include in the
review, it's one more file that may get questions and advice I'll have to
answer with 'it's a verbatim copy of the linux version with these three
lines added, why do I need to make those stylistic changes'. All of which
takes extra time. The upstreaming is going much more slowly than I'd
anticipated (but on the up-side also finding more problems than I thought
were latent in the system), and we're still at about 30k lines (after
starting at about 36k lines, though some of that is due to deleting mips).
It's been running about a month per 1000 lines to get reviewed and
upstreamed. There's about 626 lines in these 6 files, so sharing them
seemed like it would save me a few weeks of upstreaming time as well
(though I'll be the first to admit that translating LoC metrics to time has
a dubious history).

The other alternative I considered was having a #ifdef __FreeBSD__ ..
#endif in all those files, but I thought that even more intrusive.

Warner

[-- Attachment #2: Type: text/html, Size: 4225 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call
  2021-11-08 18:49     ` Warner Losh
@ 2021-11-09  8:11       ` Richard Henderson
  2021-11-10  2:10         ` Warner Losh
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Henderson @ 2021-11-09  8:11 UTC (permalink / raw)
  To: Warner Losh
  Cc: Philippe Mathieu-Daude, Alex Bennée, QEMU Developers,
	Laurent Vivier

On 11/8/21 7:49 PM, Warner Losh wrote:
>      >       /* code path for having successfully executed the syscall */
>      > +     ADJUST_SYSCALL_RETCODE
>      >       ret
>      >
>      >   0:
> 
>     Not sure about this, really.  Is it really that much cleaner to insert this than create
>     separate 10-line files, with the adjustment included?
...
> The adjustments have all been 3 lines (gmail seems to hate my formatting):
> 
> +#define        ADJUST_SYSCALL_RETCODE \
> +    jnb 2f;                    \
> +    neg %rax;                  \
> +    2:
> 
> which is significantly easier to maintain than having to monitor these files for changes 
> and copying over the changes that happen.
...
> The other alternative I considered was having a #ifdef __FreeBSD__ .. #endif in all those 
> files, but I thought that even more intrusive.

Actually, the ifdef sounds surprisingly attractive to me.  Is it ENOCOFFEE?

What I find awkward about ADJUST_SYSCALL_RETCODE is that when you're looking at the 
definition, you have no reference to the context, and vice versa.  Not that it can't be 
worked out, but it seems like the same amount of code either way, and clearer when it's 
together.

We've already split the host cpu apart, which is the major point of ifdeffery, so it 
doesn't seem like we'll wind up with a large amount of ifdefs here; we're not likely to 
see mynewos-user wanting to share this code any time soon.

I feel sufficiently fuzzy on this to solicit other opinions though.


r~


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call
  2021-11-09  8:11       ` Richard Henderson
@ 2021-11-10  2:10         ` Warner Losh
  0 siblings, 0 replies; 16+ messages in thread
From: Warner Losh @ 2021-11-10  2:10 UTC (permalink / raw)
  To: Richard Henderson
  Cc: Philippe Mathieu-Daude, Alex Bennée, QEMU Developers,
	Laurent Vivier

[-- Attachment #1: Type: text/plain, Size: 2009 bytes --]

On Tue, Nov 9, 2021 at 1:11 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 11/8/21 7:49 PM, Warner Losh wrote:
> >      >       /* code path for having successfully executed the syscall */
> >      > +     ADJUST_SYSCALL_RETCODE
> >      >       ret
> >      >
> >      >   0:
> >
> >     Not sure about this, really.  Is it really that much cleaner to
> insert this than create
> >     separate 10-line files, with the adjustment included?
> ...
> > The adjustments have all been 3 lines (gmail seems to hate my
> formatting):
> >
> > +#define        ADJUST_SYSCALL_RETCODE \
> > +    jnb 2f;                    \
> > +    neg %rax;                  \
> > +    2:
> >
> > which is significantly easier to maintain than having to monitor these
> files for changes
> > and copying over the changes that happen.
> ...
> > The other alternative I considered was having a #ifdef __FreeBSD__ ..
> #endif in all those
> > files, but I thought that even more intrusive.
>
> Actually, the ifdef sounds surprisingly attractive to me.  Is it ENOCOFFEE?
>
> What I find awkward about ADJUST_SYSCALL_RETCODE is that when you're
> looking at the
> definition, you have no reference to the context, and vice versa.  Not
> that it can't be
> worked out, but it seems like the same amount of code either way, and
> clearer when it's
> together.
>
> We've already split the host cpu apart, which is the major point of
> ifdeffery, so it
> doesn't seem like we'll wind up with a large amount of ifdefs here; we're
> not likely to
> see mynewos-user wanting to share this code any time soon.
>
> I feel sufficiently fuzzy on this to solicit other opinions though.
>

I'm OK with #ifdef. The macro hides the ifdef in a general way... but
there's
always a problem generalizing from one example...

If nobody has a better idea, I'll respin with the #ifdefs in a day or two.
That would also
give good sharing and be a couple of steps down the road towards getting a
series of
signals patches queued up...

Warner

[-- Attachment #2: Type: text/html, Size: 2704 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext
  2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
  2021-11-08 15:03   ` Richard Henderson
@ 2021-11-10 15:43   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-10 15:43 UTC (permalink / raw)
  To: Warner Losh, qemu-devel; +Cc: Richard Henderson, Laurent Vivier

On 11/8/21 03:37, Warner Losh wrote:
> Add a new function host_signal_set_pc to set the next pc in an
> mcontext. The caller should ensure this is a valid PC for execution.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  linux-user/host/aarch64/host-signal.h | 5 +++++
>  linux-user/host/alpha/host-signal.h   | 5 +++++
>  linux-user/host/arm/host-signal.h     | 5 +++++
>  linux-user/host/i386/host-signal.h    | 5 +++++
>  linux-user/host/mips/host-signal.h    | 5 +++++
>  linux-user/host/ppc/host-signal.h     | 5 +++++
>  linux-user/host/riscv/host-signal.h   | 5 +++++
>  linux-user/host/s390/host-signal.h    | 5 +++++
>  linux-user/host/sparc/host-signal.h   | 9 +++++++++
>  linux-user/host/x86_64/host-signal.h  | 5 +++++
>  10 files changed, 54 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-08  2:37 ` [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall Warner Losh
  2021-11-08 15:07   ` Richard Henderson
@ 2021-11-10 15:44   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-11-10 15:44 UTC (permalink / raw)
  To: Warner Losh, qemu-devel; +Cc: Richard Henderson, Laurent Vivier

On 11/8/21 03:37, Warner Losh wrote:
> All instances of rewind_if_in_safe_syscall are the same, differing only
> in how the instruction point is fetched from the ucontext and the size
> of the registers. Use host_signal_pc and new host_signal_set_pc
> interfaces to fetch the pointer to the PC and adjust if needed. Delete
> all the old copies of rewind_if_in_safe_syscall.
> 
> Signed-off-by: Warner Losh <imp@bsdimp.com>
> ---
>  linux-user/host/aarch64/hostdep.h | 20 --------------------
>  linux-user/host/arm/hostdep.h     | 20 --------------------
>  linux-user/host/i386/hostdep.h    | 20 --------------------
>  linux-user/host/ppc64/hostdep.h   | 20 --------------------
>  linux-user/host/riscv/hostdep.h   | 20 --------------------
>  linux-user/host/s390x/hostdep.h   | 20 --------------------
>  linux-user/host/x86_64/hostdep.h  | 20 --------------------
>  linux-user/signal.c               | 18 +++++++++++++++++-
>  8 files changed, 17 insertions(+), 141 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-08 16:39     ` Warner Losh
@ 2021-11-10 16:20       ` Warner Losh
  2021-11-10 16:31         ` Richard Henderson
  0 siblings, 1 reply; 16+ messages in thread
From: Warner Losh @ 2021-11-10 16:20 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Philippe Mathieu-Daude, QEMU Developers, Laurent Vivier

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On Mon, Nov 8, 2021 at 9:39 AM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Mon, Nov 8, 2021 at 8:07 AM Richard Henderson <
> richard.henderson@linaro.org> wrote:
>
>> On 11/8/21 3:37 AM, Warner Losh wrote:
>> > All instances of rewind_if_in_safe_syscall are the same, differing only
>> > in how the instruction point is fetched from the ucontext and the size
>> > of the registers. Use host_signal_pc and new host_signal_set_pc
>> > interfaces to fetch the pointer to the PC and adjust if needed. Delete
>> > all the old copies of rewind_if_in_safe_syscall.
>> >
>> > Signed-off-by: Warner Losh<imp@bsdimp.com>
>> > ---
>> >   linux-user/host/aarch64/hostdep.h | 20 --------------------
>> >   linux-user/host/arm/hostdep.h     | 20 --------------------
>> >   linux-user/host/i386/hostdep.h    | 20 --------------------
>> >   linux-user/host/ppc64/hostdep.h   | 20 --------------------
>> >   linux-user/host/riscv/hostdep.h   | 20 --------------------
>> >   linux-user/host/s390x/hostdep.h   | 20 --------------------
>> >   linux-user/host/x86_64/hostdep.h  | 20 --------------------
>> >   linux-user/signal.c               | 18 +++++++++++++++++-
>> >   8 files changed, 17 insertions(+), 141 deletions(-)
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Although I think we can fairly safely drop HAVE_SAFE_SYSCALL.  It is
>> required for proper
>> operation.  As with host-signal.h, really.
>>
>
> Yes. The only possible use I can see for it is to allow people to bring up
> new platforms more easily by deferring the signal-safe system call details
> until later in that process.
>

If we do, we'd need to remove the linux-user on a mips host tests from the
CI pipeline. Those are the only ones left that don't use this that we test.

Warner

[-- Attachment #2: Type: text/html, Size: 2735 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall
  2021-11-10 16:20       ` Warner Losh
@ 2021-11-10 16:31         ` Richard Henderson
  0 siblings, 0 replies; 16+ messages in thread
From: Richard Henderson @ 2021-11-10 16:31 UTC (permalink / raw)
  To: Warner Losh; +Cc: Philippe Mathieu-Daude, QEMU Developers, Laurent Vivier

On 11/10/21 5:20 PM, Warner Losh wrote:
>         Although I think we can fairly safely drop HAVE_SAFE_SYSCALL.  It is required for
>         proper
>         operation.  As with host-signal.h, really.
> 
>     Yes. The only possible use I can see for it is to allow people to bring up new
>     platforms more easily by deferring the signal-safe system call details until later in
>     that process.
> 
> If we do, we'd need to remove the linux-user on a mips host tests from the CI pipeline. 
> Those are the only ones left that don't use this that we test.

Ack, thanks.  I'll take care of this next release.


r~


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-11-10 16:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08  2:37 [RFC 0/4] linux-user: simplify safe signal handling Warner Losh
2021-11-08  2:37 ` [RFC 1/4] linux-user: Add host_signal_set_pc to set pc in mcontext Warner Losh
2021-11-08 15:03   ` Richard Henderson
2021-11-10 15:43   ` Philippe Mathieu-Daudé
2021-11-08  2:37 ` [RFC 2/4] linux-user/signal.c: Create a common rewind_if_in_safe_syscall Warner Losh
2021-11-08 15:07   ` Richard Henderson
2021-11-08 16:39     ` Warner Losh
2021-11-10 16:20       ` Warner Losh
2021-11-10 16:31         ` Richard Henderson
2021-11-10 15:44   ` Philippe Mathieu-Daudé
2021-11-08  2:37 ` [RFC 3/4] linux-user/safe-syscall.inc.S: Move to common-user Warner Losh
2021-11-08  2:37 ` [RFC 4/4] common-user: Allow return codes to be adjusted after sytsem call Warner Losh
2021-11-08 15:10   ` Richard Henderson
2021-11-08 18:49     ` Warner Losh
2021-11-09  8:11       ` Richard Henderson
2021-11-10  2:10         ` Warner Losh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).