qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates
@ 2016-06-13 21:45 Richard Henderson
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
                   ` (6 more replies)
  0 siblings, 7 replies; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

We added support for x86_64 in 4d330cee37a2; this adds support
for 5 more hosts.  Also, tweak the signal_pending test for x86_64.


r~


Richard Henderson (6):
  linux-user: fix x86_64 safe_syscall
  linux-user: Provide safe_syscall for i386
  linux-user: Provide safe_syscall for arm
  linux-user: Provide safe_syscall for aarch64
  linux-user: Provide safe_syscall for s390x
  linux-user: Provide safe_syscall for ppc64

 linux-user/host/aarch64/hostdep.h          |  34 +++++++++
 linux-user/host/aarch64/safe-syscall.inc.S |  72 +++++++++++++++++++
 linux-user/host/arm/hostdep.h              |  34 +++++++++
 linux-user/host/arm/safe-syscall.inc.S     |  86 ++++++++++++++++++++++
 linux-user/host/i386/hostdep.h             |  34 +++++++++
 linux-user/host/i386/safe-syscall.inc.S    | 110 +++++++++++++++++++++++++++++
 linux-user/host/ppc64/hostdep.h            |  34 +++++++++
 linux-user/host/ppc64/safe-syscall.inc.S   |  87 +++++++++++++++++++++++
 linux-user/host/s390x/hostdep.h            |  34 +++++++++
 linux-user/host/s390x/safe-syscall.inc.S   |  87 +++++++++++++++++++++++
 linux-user/host/x86_64/safe-syscall.inc.S  |   6 +-
 11 files changed, 615 insertions(+), 3 deletions(-)
 create mode 100644 linux-user/host/aarch64/hostdep.h
 create mode 100644 linux-user/host/aarch64/safe-syscall.inc.S
 create mode 100644 linux-user/host/arm/hostdep.h
 create mode 100644 linux-user/host/arm/safe-syscall.inc.S
 create mode 100644 linux-user/host/i386/hostdep.h
 create mode 100644 linux-user/host/i386/safe-syscall.inc.S
 create mode 100644 linux-user/host/ppc64/hostdep.h
 create mode 100644 linux-user/host/ppc64/safe-syscall.inc.S
 create mode 100644 linux-user/host/s390x/hostdep.h
 create mode 100644 linux-user/host/s390x/safe-syscall.inc.S

-- 
2.5.5

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

* [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-14 11:58   ` Peter Maydell
  2016-06-21 19:26   ` Riku Voipio
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386 Richard Henderson
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Do what the comment says, test for signal_pending non-zero,
rather than the current coe which tests for bit 0 non-zero.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/x86_64/safe-syscall.inc.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/host/x86_64/safe-syscall.inc.S b/linux-user/host/x86_64/safe-syscall.inc.S
index e09368d..f36992d 100644
--- a/linux-user/host/x86_64/safe-syscall.inc.S
+++ b/linux-user/host/x86_64/safe-syscall.inc.S
@@ -67,8 +67,8 @@ safe_syscall_base:
          */
 safe_syscall_start:
         /* if signal_pending is non-zero, don't do the call */
-        testl   $1, (%rbp)
-        jnz     return_ERESTARTSYS
+        cmpl	$0, (%rbp)
+        jnz     1f
         syscall
 safe_syscall_end:
         /* code path for having successfully executed the syscall */
@@ -78,7 +78,7 @@ safe_syscall_end:
         .cfi_restore rbp
         ret
 
-return_ERESTARTSYS:
+1:
         /* code path when we didn't execute the syscall */
         .cfi_restore_state
         mov     $-TARGET_ERESTARTSYS, %rax
-- 
2.5.5

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

* [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-14 11:58   ` Peter Maydell
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm Richard Henderson
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/i386/hostdep.h          |  34 ++++++++++
 linux-user/host/i386/safe-syscall.inc.S | 110 ++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)
 create mode 100644 linux-user/host/i386/hostdep.h
 create mode 100644 linux-user/host/i386/safe-syscall.inc.S

diff --git a/linux-user/host/i386/hostdep.h b/linux-user/host/i386/hostdep.h
new file mode 100644
index 0000000..9e2b4d7
--- /dev/null
+++ b/linux-user/host/i386/hostdep.h
@@ -0,0 +1,34 @@
+/*
+ * hostdep.h : things which are dependent on the host architecture
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HOSTDEP_H
+#define QEMU_HOSTDEP_H
+
+/* 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)
+{
+    struct ucontext *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/i386/safe-syscall.inc.S b/linux-user/host/i386/safe-syscall.inc.S
new file mode 100644
index 0000000..f5f0c64
--- /dev/null
+++ b/linux-user/host/i386/safe-syscall.inc.S
@@ -0,0 +1,110 @@
+/*
+ * safe-syscall.inc.S : host-specific assembly fragment
+ * to handle signals occurring at the same time as system calls.
+ * This is intended to be included by linux-user/safe-syscall.S
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+        .global safe_syscall_base
+        .global safe_syscall_start
+        .global safe_syscall_end
+        .type   safe_syscall_base, @function
+
+        /* This is the entry point for making a system call. The calling
+         * convention here is that of a C varargs function with the
+         * first argument an 'int *' to the signal_pending flag, the
+         * second one the system call number (as a 'long'), and all further
+         * arguments being syscall arguments (also 'long').
+         * We return a long which is the syscall's return value, which
+         * may be negative-errno on failure. Conversion to the
+         * -1-and-errno-set convention is done by the calling wrapper.
+         */
+safe_syscall_base:
+        .cfi_startproc
+        push    %ebp
+        .cfi_adjust_cfa_offset 4
+        .cfi_rel_offset ebp, 0
+	push	%esi
+        .cfi_adjust_cfa_offset 4
+        .cfi_rel_offset esi, 0
+	push	%edi
+        .cfi_adjust_cfa_offset 4
+        .cfi_rel_offset edi, 0
+	push	%ebx
+        .cfi_adjust_cfa_offset 4
+        .cfi_rel_offset ebx, 0
+
+        /* The syscall calling convention isn't the same as the C one:
+         * we enter with 0(%esp) == return address
+         *               4(%esp) == *signal_pending
+         *               8(%esp) == syscall number
+         *               12(%esp) ... 32(%esp) == syscall arguments
+         *               and return the result in eax
+         * and the syscall instruction needs
+         *               eax == syscall number
+         *               ebx, ecx, edx, esi, edi, ebp == syscall arguments
+         *               and returns the result in eax
+         * Shuffle everything around appropriately.
+	 * Note the 16 bytes that we pushed to save registers.
+         */
+        mov     12+16(%esp), %ebx       /* the syscall arguments */
+        mov     16+16(%esp), %ecx
+        mov     20+16(%esp), %edx
+        mov     24+16(%esp), %esi
+        mov     28+16(%esp), %edi
+        mov     32+16(%esp), %ebp
+
+        /* This next sequence of code works in conjunction with the
+         * rewind_if_safe_syscall_function(). If a signal is taken
+         * and the interrupted PC is anywhere between 'safe_syscall_start'
+         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
+         * The code sequence must therefore be able to cope with this, and
+         * the syscall instruction must be the final one in the sequence.
+         */
+safe_syscall_start:
+        /* if signal_pending is non-zero, don't do the call */
+	mov	4+16(%esp), %eax	/* signal_pending */
+        cmp	$0, (%eax)
+        mov     8+16(%esp), %eax        /* syscall number */
+        jnz     1f
+        int	$0x80
+safe_syscall_end:
+        /* code path for having successfully executed the syscall */
+        pop     %ebx
+        .cfi_remember_state
+        .cfi_def_cfa_offset 4
+        .cfi_restore ebx
+	pop	%edi
+        .cfi_def_cfa_offset 4
+        .cfi_restore edi
+	pop	%esi
+        .cfi_def_cfa_offset 4
+        .cfi_restore esi
+	pop	%ebp
+        .cfi_def_cfa_offset 4
+        .cfi_restore ebp
+        ret
+
+1:
+        /* code path when we didn't execute the syscall */
+        .cfi_restore_state
+        mov     $-TARGET_ERESTARTSYS, %eax
+        pop     %ebx
+        .cfi_remember_state
+        .cfi_def_cfa_offset 4
+        .cfi_restore ebx
+	pop	%edi
+        .cfi_def_cfa_offset 4
+        .cfi_restore edi
+	pop	%esi
+        .cfi_def_cfa_offset 4
+        .cfi_restore esi
+	pop	%ebp
+        .cfi_def_cfa_offset 4
+        .cfi_restore ebp
+        ret
+        .cfi_endproc
+
+        .size   safe_syscall_base, .-safe_syscall_base
-- 
2.5.5

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

* [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386 Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-14 12:04   ` Peter Maydell
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64 Richard Henderson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/arm/hostdep.h          | 34 ++++++++++++++
 linux-user/host/arm/safe-syscall.inc.S | 86 ++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)
 create mode 100644 linux-user/host/arm/hostdep.h
 create mode 100644 linux-user/host/arm/safe-syscall.inc.S

diff --git a/linux-user/host/arm/hostdep.h b/linux-user/host/arm/hostdep.h
new file mode 100644
index 0000000..1426fb6
--- /dev/null
+++ b/linux-user/host/arm/hostdep.h
@@ -0,0 +1,34 @@
+/*
+ * hostdep.h : things which are dependent on the host architecture
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HOSTDEP_H
+#define QEMU_HOSTDEP_H
+
+/* 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)
+{
+    struct ucontext *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/arm/safe-syscall.inc.S b/linux-user/host/arm/safe-syscall.inc.S
new file mode 100644
index 0000000..52f8883
--- /dev/null
+++ b/linux-user/host/arm/safe-syscall.inc.S
@@ -0,0 +1,86 @@
+/*
+ * safe-syscall.inc.S : host-specific assembly fragment
+ * to handle signals occurring at the same time as system calls.
+ * This is intended to be included by linux-user/safe-syscall.S
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+	.global safe_syscall_base
+	.global safe_syscall_start
+	.global safe_syscall_end
+	.type   safe_syscall_base, %function
+
+	.cfi_sections	.debug_frame
+
+	.text
+	.syntax unified
+	.arm
+
+	/* This is the entry point for making a system call. The calling
+	 * convention here is that of a C varargs function with the
+	 * first argument an 'int *' to the signal_pending flag, the
+	 * second one the system call number (as a 'long'), and all further
+	 * arguments being syscall arguments (also 'long').
+	 * We return a long which is the syscall's return value, which
+	 * may be negative-errno on failure. Conversion to the
+	 * -1-and-errno-set convention is done by the calling wrapper.
+	 */
+safe_syscall_base:
+	.fnstart
+	.cfi_startproc
+	mov	ip, sp			/* save entry stack */
+	push	{ r4, r5, r6, r7, r8, lr }
+	.save	{ r4, r5, r6, r7, r8, lr }
+	.cfi_adjust_cfa_offset 24
+	.cfi_rel_offset r4, 0
+	.cfi_rel_offset r5, 4
+	.cfi_rel_offset r6, 8
+	.cfi_rel_offset r7, 12
+	.cfi_rel_offset r8, 16
+	.cfi_rel_offset lr, 20
+
+	/* The syscall calling convention isn't the same as the C one:
+	 * we enter with r0 == *signal_pending
+	 *               r1 == syscall number
+	 *               r2, r3, [sp+0] ... [sp+12] == syscall arguments
+	 *               and return the result in r0
+	 * and the syscall instruction needs
+	 *               r7 == syscall number
+	 *               r0 ... r6 == syscall arguments
+	 *               and returns the result in r0
+	 * Shuffle everything around appropriately.
+	 * Note the 16 bytes that we pushed to save registers.
+	 */
+	mov	r8, r0			/* copy signal_pending */
+	mov	r7, r1			/* syscall number */
+	mov	r0, r2			/* syscall args */
+	mov	r1, r3
+	ldm	ip, { r2, r3, r4, r5, r6 }
+
+	/* This next sequence of code works in conjunction with the
+	 * rewind_if_safe_syscall_function(). If a signal is taken
+	 * and the interrupted PC is anywhere between 'safe_syscall_start'
+	 * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
+	 * The code sequence must therefore be able to cope with this, and
+	 * the syscall instruction must be the final one in the sequence.
+	 */
+safe_syscall_start:
+	/* if signal_pending is non-zero, don't do the call */
+	ldr	ip, [r8]		/* signal_pending */
+	tst	ip, ip
+	bne	1f
+	swi	0
+safe_syscall_end:
+	/* code path for having successfully executed the syscall */
+	pop	{ r4, r5, r6, r7, r8, pc }
+
+1:
+	/* code path when we didn't execute the syscall */
+	ldr	r0, =-TARGET_ERESTARTSYS
+	pop	{ r4, r5, r6, r7, r8, pc }
+	.fnend
+	.cfi_endproc
+
+	.size   safe_syscall_base, .-safe_syscall_base
-- 
2.5.5

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

* [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
                   ` (2 preceding siblings ...)
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-13 22:04   ` Peter Maydell
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 5/6] linux-user: Provide safe_syscall for s390x Richard Henderson
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/aarch64/hostdep.h          | 34 ++++++++++++++
 linux-user/host/aarch64/safe-syscall.inc.S | 72 ++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 linux-user/host/aarch64/hostdep.h
 create mode 100644 linux-user/host/aarch64/safe-syscall.inc.S

diff --git a/linux-user/host/aarch64/hostdep.h b/linux-user/host/aarch64/hostdep.h
new file mode 100644
index 0000000..0ff7985
--- /dev/null
+++ b/linux-user/host/aarch64/hostdep.h
@@ -0,0 +1,34 @@
+/*
+ * hostdep.h : things which are dependent on the host architecture
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HOSTDEP_H
+#define QEMU_HOSTDEP_H
+
+/* 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)
+{
+    struct ucontext *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/aarch64/safe-syscall.inc.S b/linux-user/host/aarch64/safe-syscall.inc.S
new file mode 100644
index 0000000..5416b90
--- /dev/null
+++ b/linux-user/host/aarch64/safe-syscall.inc.S
@@ -0,0 +1,72 @@
+/*
+ * safe-syscall.inc.S : host-specific assembly fragment
+ * to handle signals occurring at the same time as system calls.
+ * This is intended to be included by linux-user/safe-syscall.S
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+        .global safe_syscall_base
+        .global safe_syscall_start
+        .global safe_syscall_end
+        .type   safe_syscall_base, #function
+        .type   safe_syscall_start, #function
+        .type   safe_syscall_end, #function
+
+        /* This is the entry point for making a system call. The calling
+         * convention here is that of a C varargs function with the
+         * first argument an 'int *' to the signal_pending flag, the
+         * second one the system call number (as a 'long'), and all further
+         * arguments being syscall arguments (also 'long').
+         * We return a long which is the syscall's return value, which
+         * may be negative-errno on failure. Conversion to the
+         * -1-and-errno-set convention is done by the calling wrapper.
+         */
+safe_syscall_base:
+        .cfi_startproc
+        /* The syscall calling convention isn't the same as the
+         * C one:
+         * we enter with x0 == *signal_pending
+         *               x1 == syscall number
+         *               x2 ... x7, (stack) == syscall arguments
+         *               and return the result in x0
+         * and the syscall instruction needs
+         *               x8 == syscall number
+         *               x0 ... x6 == syscall arguments
+         *               and returns the result in x0
+         * Shuffle everything around appropriately.
+         */
+	mov	x9, x0          /* signal_pending pointer */
+	mov	w8, w1          /* syscall number */
+	mov	x0, x2          /* syscall arguments */
+	mov	x1, x3
+	mov	x2, x4
+	mov	x3, x5
+	mov	x4, x6
+	mov	x6, x7
+	ldr	x7, [sp]
+
+        /* This next sequence of code works in conjunction with the
+         * rewind_if_safe_syscall_function(). If a signal is taken
+         * and the interrupted PC is anywhere between 'safe_syscall_start'
+         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
+         * The code sequence must therefore be able to cope with this, and
+         * the syscall instruction must be the final one in the sequence.
+         */
+safe_syscall_start:
+        /* if signal_pending is non-zero, don't do the call */
+	ldr	w10, [x9]
+	cbnz	w10, 0f 
+        svc	0x0
+safe_syscall_end:
+        /* code path for having successfully executed the syscall */
+        ret
+
+0:
+        /* code path when we didn't execute the syscall */
+        mov     x0, #-TARGET_ERESTARTSYS
+        ret
+        .cfi_endproc
+
+        .size   safe_syscall_base, .-safe_syscall_base
-- 
2.5.5

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

* [Qemu-devel] [PATCH 5/6] linux-user: Provide safe_syscall for s390x
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
                   ` (3 preceding siblings ...)
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64 Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64 Richard Henderson
  2016-06-13 21:53 ` [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Peter Maydell
  6 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/s390x/hostdep.h          | 34 +++++++++++++
 linux-user/host/s390x/safe-syscall.inc.S | 87 ++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 linux-user/host/s390x/hostdep.h
 create mode 100644 linux-user/host/s390x/safe-syscall.inc.S

diff --git a/linux-user/host/s390x/hostdep.h b/linux-user/host/s390x/hostdep.h
new file mode 100644
index 0000000..e94745f
--- /dev/null
+++ b/linux-user/host/s390x/hostdep.h
@@ -0,0 +1,34 @@
+/*
+ * hostdep.h : things which are dependent on the host architecture
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HOSTDEP_H
+#define QEMU_HOSTDEP_H
+
+/* 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)
+{
+    struct ucontext *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/s390x/safe-syscall.inc.S b/linux-user/host/s390x/safe-syscall.inc.S
new file mode 100644
index 0000000..8a0e420
--- /dev/null
+++ b/linux-user/host/s390x/safe-syscall.inc.S
@@ -0,0 +1,87 @@
+/*
+ * safe-syscall.inc.S : host-specific assembly fragment
+ * to handle signals occurring at the same time as system calls.
+ * This is intended to be included by linux-user/safe-syscall.S
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+        .global safe_syscall_base
+        .global safe_syscall_start
+        .global safe_syscall_end
+        .type   safe_syscall_base, @function
+
+        /* This is the entry point for making a system call. The calling
+         * convention here is that of a C varargs function with the
+         * first argument an 'int *' to the signal_pending flag, the
+         * second one the system call number (as a 'long'), and all further
+         * arguments being syscall arguments (also 'long').
+         * We return a long which is the syscall's return value, which
+         * may be negative-errno on failure. Conversion to the
+         * -1-and-errno-set convention is done by the calling wrapper.
+         */
+safe_syscall_base:
+        .cfi_startproc
+	stmg	%r6,%r15,48(%r15)	/* save all call-saved registers */
+	.cfi_offset %r15,-40
+	.cfi_offset %r14,-48
+	.cfi_offset %r13,-56
+	.cfi_offset %r12,-64
+	.cfi_offset %r11,-72
+	.cfi_offset %r10,-80
+	.cfi_offset %r9,-88
+	.cfi_offset %r8,-96
+	.cfi_offset %r7,-104
+	.cfi_offset %r6,-112
+	lgr	%r1,%r15
+	lg	%r0,8(%r15)		/* load eos */
+	aghi	%r15,-160
+	.cfi_adjust_cfa_offset 160
+	stg	%r1,0(%r15)		/* store back chain */
+	stg	%r0,8(%r15)		/* store eos */
+
+        /* The syscall calling convention isn't the same as the
+         * C one:
+         * we enter with r2 == *signal_pending
+         *               r3 == syscall number
+         *               r4, r5, r6, (stack) == syscall arguments
+         *               and return the result in r2
+         * and the syscall instruction needs
+         *               r1 == syscall number
+         *               r2 ... r7 == syscall arguments
+         *               and returns the result in r2
+         * Shuffle everything around appropriately.
+         */
+	lgr	%r8,%r2			/* signal_pending pointer */
+	lgr	%r1,%r3			/* syscall number */
+	lgr	%r2,%r4			/* syscall args */
+	lgr	%r3,%r5
+	lgr	%r4,%r6
+	lmg	%r5,%r7,320(%r15)
+
+        /* This next sequence of code works in conjunction with the
+         * rewind_if_safe_syscall_function(). If a signal is taken
+         * and the interrupted PC is anywhere between 'safe_syscall_start'
+         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
+         * The code sequence must therefore be able to cope with this, and
+         * the syscall instruction must be the final one in the sequence.
+         */
+safe_syscall_start:
+        /* if signal_pending is non-zero, don't do the call */
+	lt	%r0,0(%r8)
+	jne	2f
+	svc	0
+safe_syscall_end:
+
+1:	lg	%r15,0(%r15)		/* load back chain */
+	.cfi_remember_state
+	.cfi_adjust_cfa_offset -160
+	lmg	%r6,%r15,48(%r15)	/* load saved registers */
+	br	%r14
+	.cfi_restore_state
+2:	lghi	%r2, -TARGET_ERESTARTSYS
+	j	1b
+        .cfi_endproc
+
+        .size   safe_syscall_base, .-safe_syscall_base
-- 
2.5.5

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

* [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
                   ` (4 preceding siblings ...)
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 5/6] linux-user: Provide safe_syscall for s390x Richard Henderson
@ 2016-06-13 21:45 ` Richard Henderson
  2016-06-13 22:23   ` Peter Maydell
  2016-06-13 21:53 ` [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Peter Maydell
  6 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 21:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: riku.voipio, peter.maydell

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/host/ppc64/hostdep.h          | 34 +++++++++++++
 linux-user/host/ppc64/safe-syscall.inc.S | 87 ++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 linux-user/host/ppc64/hostdep.h
 create mode 100644 linux-user/host/ppc64/safe-syscall.inc.S

diff --git a/linux-user/host/ppc64/hostdep.h b/linux-user/host/ppc64/hostdep.h
new file mode 100644
index 0000000..81e3d55
--- /dev/null
+++ b/linux-user/host/ppc64/hostdep.h
@@ -0,0 +1,34 @@
+/*
+ * hostdep.h : things which are dependent on the host architecture
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_HOSTDEP_H
+#define QEMU_HOSTDEP_H
+
+/* 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)
+{
+    struct ucontext *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/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S
new file mode 100644
index 0000000..89e979c
--- /dev/null
+++ b/linux-user/host/ppc64/safe-syscall.inc.S
@@ -0,0 +1,87 @@
+/*
+ * safe-syscall.inc.S : host-specific assembly fragment
+ * to handle signals occurring at the same time as system calls.
+ * This is intended to be included by linux-user/safe-syscall.S
+ *
+ * Copyright (C) 2015 Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+        .global safe_syscall_base
+        .global safe_syscall_start
+        .global safe_syscall_end
+        .type   safe_syscall_base, @function
+
+	.text
+
+        /* This is the entry point for making a system call. The calling
+         * convention here is that of a C varargs function with the
+         * first argument an 'int *' to the signal_pending flag, the
+         * second one the system call number (as a 'long'), and all further
+         * arguments being syscall arguments (also 'long').
+         * We return a long which is the syscall's return value, which
+         * may be negative-errno on failure. Conversion to the
+         * -1-and-errno-set convention is done by the calling wrapper.
+         */
+#if _CALL_ELF == 2
+safe_syscall_base:
+        .cfi_startproc
+        .localentry safe_syscall_base,0
+#else
+	.section ".opd","aw"
+	.align	3
+safe_syscall_base:
+	.quad	.L.safe_syscall_base,.TOC.@tocbase,0
+	.previous
+.L.safe_syscall_base:
+        .cfi_startproc
+#endif
+        /* We enter with r3 == *signal_pending
+         *               r4 == syscall number
+         *               r5 ... r10 == syscall arguments
+         *               and return the result in r3
+         * and the syscall instruction needs
+         *               r0 == syscall number
+         *               r3 ... r8 == syscall arguments
+         *               and returns the result in r3
+         * Shuffle everything around appropriately.
+         */
+	mr	11, 3
+	mr	0, 4	/* syscall number */
+	mr	3, 5	/* syscall arguments */
+	mr	4, 6
+	mr	5, 7
+	mr	6, 8
+	mr	7, 9
+	mr	8, 10
+
+        /* This next sequence of code works in conjunction with the
+         * rewind_if_safe_syscall_function(). If a signal is taken
+         * and the interrupted PC is anywhere between 'safe_syscall_start'
+         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
+         * The code sequence must therefore be able to cope with this, and
+         * the syscall instruction must be the final one in the sequence.
+         */
+safe_syscall_start:
+        /* if signal_pending is non-zero, don't do the call */
+        lwz	12, 0(11)
+	cmpwi	0, 12, 0
+        bne-	0f
+        sc
+safe_syscall_end:
+        blr
+
+0:
+        /* code path when we didn't execute the syscall */
+	addi	3, 0, -TARGET_ERESTARTSYS
+	blr
+        .cfi_endproc
+
+#if _CALL_ELF == 2
+        .size   safe_syscall_base, .-safe_syscall_base
+#else
+	.size   safe_syscall_base, .-.L.safe_syscall_base
+        .size   .L.safe_syscall_base, .-.L.safe_syscall_base
+#endif
-- 
2.5.5

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

* Re: [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates
  2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
                   ` (5 preceding siblings ...)
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64 Richard Henderson
@ 2016-06-13 21:53 ` Peter Maydell
  2016-06-13 22:09   ` Peter Maydell
  6 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 21:53 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> We added support for x86_64 in 4d330cee37a2; this adds support
> for 5 more hosts.  Also, tweak the signal_pending test for x86_64.
>
>
> r~
>
>
> Richard Henderson (6):
>   linux-user: fix x86_64 safe_syscall
>   linux-user: Provide safe_syscall for i386
>   linux-user: Provide safe_syscall for arm
>   linux-user: Provide safe_syscall for aarch64
>   linux-user: Provide safe_syscall for s390x
>   linux-user: Provide safe_syscall for ppc64

I just spent today writing these for i386, arm and aarch64,
so we've duplicated work here :-(

-- PMM

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64 Richard Henderson
@ 2016-06-13 22:04   ` Peter Maydell
  2016-06-13 22:21     ` Richard Henderson
  0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:04 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/aarch64/hostdep.h          | 34 ++++++++++++++
>  linux-user/host/aarch64/safe-syscall.inc.S | 72 ++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
>  create mode 100644 linux-user/host/aarch64/hostdep.h
>  create mode 100644 linux-user/host/aarch64/safe-syscall.inc.S
>
> diff --git a/linux-user/host/aarch64/hostdep.h b/linux-user/host/aarch64/hostdep.h
> new file mode 100644
> index 0000000..0ff7985
> --- /dev/null
> +++ b/linux-user/host/aarch64/hostdep.h
> @@ -0,0 +1,34 @@
> +/*
> + * hostdep.h : things which are dependent on the host architecture
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_HOSTDEP_H
> +#define QEMU_HOSTDEP_H
> +
> +/* 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)
> +{
> +    struct ucontext *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/aarch64/safe-syscall.inc.S b/linux-user/host/aarch64/safe-syscall.inc.S
> new file mode 100644
> index 0000000..5416b90
> --- /dev/null
> +++ b/linux-user/host/aarch64/safe-syscall.inc.S
> @@ -0,0 +1,72 @@
> +/*
> + * safe-syscall.inc.S : host-specific assembly fragment
> + * to handle signals occurring at the same time as system calls.
> + * This is intended to be included by linux-user/safe-syscall.S
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +        .global safe_syscall_base
> +        .global safe_syscall_start
> +        .global safe_syscall_end
> +        .type   safe_syscall_base, #function
> +        .type   safe_syscall_start, #function
> +        .type   safe_syscall_end, #function

_start and _end aren't function entry points, so is it OK
to mark them as functions?

(The 'as' manual doesn't document what setting .type does in much
detail...)

> +
> +        /* This is the entry point for making a system call. The calling
> +         * convention here is that of a C varargs function with the
> +         * first argument an 'int *' to the signal_pending flag, the
> +         * second one the system call number (as a 'long'), and all further
> +         * arguments being syscall arguments (also 'long').
> +         * We return a long which is the syscall's return value, which
> +         * may be negative-errno on failure. Conversion to the
> +         * -1-and-errno-set convention is done by the calling wrapper.
> +         */
> +safe_syscall_base:
> +        .cfi_startproc
> +        /* The syscall calling convention isn't the same as the
> +         * C one:
> +         * we enter with x0 == *signal_pending
> +         *               x1 == syscall number
> +         *               x2 ... x7, (stack) == syscall arguments
> +         *               and return the result in x0
> +         * and the syscall instruction needs
> +         *               x8 == syscall number
> +         *               x0 ... x6 == syscall arguments
> +         *               and returns the result in x0
> +         * Shuffle everything around appropriately.
> +         */

http://man7.org/linux/man-pages/man2/syscall.2.html
doesn't mention a syscall argument in x7, just x0..x5.

> +       mov     x9, x0          /* signal_pending pointer */
> +       mov     w8, w1          /* syscall number */

Seems a bit odd to use a 32-bit move for this when our input
calling convention has it as 64 bits and the kernel's calling
convention has it as 64 bits.

> +       mov     x0, x2          /* syscall arguments */
> +       mov     x1, x3
> +       mov     x2, x4
> +       mov     x3, x5
> +       mov     x4, x6
> +       mov     x6, x7
> +       ldr     x7, [sp]
> +
> +        /* This next sequence of code works in conjunction with the
> +         * rewind_if_safe_syscall_function(). If a signal is taken
> +         * and the interrupted PC is anywhere between 'safe_syscall_start'
> +         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
> +         * The code sequence must therefore be able to cope with this, and
> +         * the syscall instruction must be the final one in the sequence.
> +         */
> +safe_syscall_start:
> +        /* if signal_pending is non-zero, don't do the call */
> +       ldr     w10, [x9]
> +       cbnz    w10, 0f
> +        svc    0x0
> +safe_syscall_end:
> +        /* code path for having successfully executed the syscall */
> +        ret
> +
> +0:
> +        /* code path when we didn't execute the syscall */
> +        mov     x0, #-TARGET_ERESTARTSYS
> +        ret
> +        .cfi_endproc
> +
> +        .size   safe_syscall_base, .-safe_syscall_base
> --
> 2.5.5
>

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates
  2016-06-13 21:53 ` [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Peter Maydell
@ 2016-06-13 22:09   ` Peter Maydell
  2016-06-21 19:08     ` Riku Voipio
  0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:09 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
>> Richard Henderson (6):
>>   linux-user: fix x86_64 safe_syscall
>>   linux-user: Provide safe_syscall for i386
>>   linux-user: Provide safe_syscall for arm
>>   linux-user: Provide safe_syscall for aarch64
>>   linux-user: Provide safe_syscall for s390x
>>   linux-user: Provide safe_syscall for ppc64
>
> I just spent today writing these for i386, arm and aarch64,
> so we've duplicated work here :-(

Also, I have a patchset which moves from the generic/hostdep.h
to having explicitly one hostdep.h per supported architecture.
That needs to go in before these, otherwise these break
compilation unless you do a 'make clean'.

I'll send that out tomorrow, working version in
 https://git.linaro.org/people/peter.maydell/qemu-arm.git/shortlog/refs/heads/sigrace-fixes

We also have a bug in the signal.c code which I noticed
trying to test my i386 safe_syscall: we call sigfillset()
on the uc_sigmask field of the ucontext_t* that the
kernel passes us as argument 3 of the signal handler.
This trashes a lot of stuff on the stack because the
libc headers say "sigset_t is 128 bytes" and the kernel
says "it's only 8 bytes", so the sigfillset() writes -1
to a lot of the stack that it shouldn't. (I don't know
why glibc exposes a struct that isn't actually what
the kernel provides here, but it's a very long standing
confusion :-( )

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 22:04   ` Peter Maydell
@ 2016-06-13 22:21     ` Richard Henderson
  2016-06-13 22:28       ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 22:21 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Riku Voipio

On 06/13/2016 03:04 PM, Peter Maydell wrote:
>> +        .global safe_syscall_base
>> +        .global safe_syscall_start
>> +        .global safe_syscall_end
>> +        .type   safe_syscall_base, #function
>> +        .type   safe_syscall_start, #function
>> +        .type   safe_syscall_end, #function
>
> _start and _end aren't function entry points, so is it OK
> to mark them as functions?

Yes.  Indeed, if you don't, the objdump will think these are data symbols and 
fail to disassemble the instructions that follow.


> (The 'as' manual doesn't document what setting .type does in much
> detail...)

It sets the Elf_Sym.st_type field.
But what that implies is very host specific.

> http://man7.org/linux/man-pages/man2/syscall.2.html
> doesn't mention a syscall argument in x7, just x0..x5.

I took glibc as definitive, since that's code that definitely works.

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/aarch64/syscall.S;h=98c1b42ee96fe0fb39955849773ca16d095803e0;hb=HEAD

I'll also point you at the kernel's __sys_trace, which does in fact save and 
restore all 8 registers around the tracing call-outs.

>> +       mov     x9, x0          /* signal_pending pointer */
>> +       mov     w8, w1          /* syscall number */
>
> Seems a bit odd to use a 32-bit move for this when our input
> calling convention has it as 64 bits and the kernel's calling
> convention has it as 64 bits.

I got that from glibc too.



r~

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

* Re: [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64 Richard Henderson
@ 2016-06-13 22:23   ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:23 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/ppc64/hostdep.h          | 34 +++++++++++++
>  linux-user/host/ppc64/safe-syscall.inc.S | 87 ++++++++++++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
>  create mode 100644 linux-user/host/ppc64/hostdep.h
>  create mode 100644 linux-user/host/ppc64/safe-syscall.inc.S
>
> diff --git a/linux-user/host/ppc64/hostdep.h b/linux-user/host/ppc64/hostdep.h
> new file mode 100644
> index 0000000..81e3d55
> --- /dev/null
> +++ b/linux-user/host/ppc64/hostdep.h
> @@ -0,0 +1,34 @@
> +/*
> + * hostdep.h : things which are dependent on the host architecture
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_HOSTDEP_H
> +#define QEMU_HOSTDEP_H
> +
> +/* 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)
> +{
> +    struct ucontext *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/ppc64/safe-syscall.inc.S b/linux-user/host/ppc64/safe-syscall.inc.S
> new file mode 100644
> index 0000000..89e979c
> --- /dev/null
> +++ b/linux-user/host/ppc64/safe-syscall.inc.S
> @@ -0,0 +1,87 @@
> +/*
> + * safe-syscall.inc.S : host-specific assembly fragment
> + * to handle signals occurring at the same time as system calls.
> + * This is intended to be included by linux-user/safe-syscall.S
> + *
> + * Copyright (C) 2015 Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>

I would say this ppc code was copyright you (or RedHat depending
which hat you're wearing), not Timothy.

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 22:21     ` Richard Henderson
@ 2016-06-13 22:28       ` Peter Maydell
  2016-06-13 22:31         ` Peter Maydell
  2016-06-13 22:38         ` Richard Henderson
  0 siblings, 2 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:28 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 23:21, Richard Henderson <rth@twiddle.net> wrote:
> On 06/13/2016 03:04 PM, Peter Maydell wrote:
>>>
>>> +        .global safe_syscall_base
>>> +        .global safe_syscall_start
>>> +        .global safe_syscall_end
>>> +        .type   safe_syscall_base, #function
>>> +        .type   safe_syscall_start, #function
>>> +        .type   safe_syscall_end, #function
>>
>>
>> _start and _end aren't function entry points, so is it OK
>> to mark them as functions?
>
>
> Yes.  Indeed, if you don't, the objdump will think these are data symbols
> and fail to disassemble the instructions that follow.

I was told this was an objdump bug... it certainly
seems like an objdump bug to me, because this works
on the other architectures.
https://bugs.linaro.org/show_bug.cgi?id=815

>> (The 'as' manual doesn't document what setting .type does in much
>> detail...)
>
>
> It sets the Elf_Sym.st_type field.
> But what that implies is very host specific.
>
>> http://man7.org/linux/man-pages/man2/syscall.2.html
>> doesn't mention a syscall argument in x7, just x0..x5.
>
> I took glibc as definitive, since that's code that definitely works.

If we never use the 7th syscall argument (and we can't,
since it doesn't exist on some hosts), then it's
unnecessary work, though I guess coming back to fix
all these host functions later might be tedious.

> https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/aarch64/syscall.S;h=98c1b42ee96fe0fb39955849773ca16d095803e0;hb=HEAD
>
> I'll also point you at the kernel's __sys_trace, which does in fact save and
> restore all 8 registers around the tracing call-outs.
>
>>> +       mov     x9, x0          /* signal_pending pointer */
>>> +       mov     w8, w1          /* syscall number */
>>
>>
>> Seems a bit odd to use a 32-bit move for this when our input
>> calling convention has it as 64 bits and the kernel's calling
>> convention has it as 64 bits.
>
>
> I got that from glibc too.

glibc's syscall() takes the system parameter as an int and
does a sign-extending move into x0 with an uxtw.
safe_syscall() takes a long, so it's already 64 bits.

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 22:28       ` Peter Maydell
@ 2016-06-13 22:31         ` Peter Maydell
  2016-06-13 22:38         ` Richard Henderson
  1 sibling, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:31 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 23:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> glibc's syscall() takes the system parameter as an int and
> does a sign-extending move into x0 with an uxtw.

...zero-extending...

-- PMM

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 22:28       ` Peter Maydell
  2016-06-13 22:31         ` Peter Maydell
@ 2016-06-13 22:38         ` Richard Henderson
  2016-06-13 22:40           ` Peter Maydell
  1 sibling, 1 reply; 24+ messages in thread
From: Richard Henderson @ 2016-06-13 22:38 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Riku Voipio

On 06/13/2016 03:28 PM, Peter Maydell wrote:
> On 13 June 2016 at 23:21, Richard Henderson <rth@twiddle.net> wrote:
>> On 06/13/2016 03:04 PM, Peter Maydell wrote:
>>>>
>>>> +        .global safe_syscall_base
>>>> +        .global safe_syscall_start
>>>> +        .global safe_syscall_end
>>>> +        .type   safe_syscall_base, #function
>>>> +        .type   safe_syscall_start, #function
>>>> +        .type   safe_syscall_end, #function
>>>
>>>
>>> _start and _end aren't function entry points, so is it OK
>>> to mark them as functions?
>>
>>
>> Yes.  Indeed, if you don't, the objdump will think these are data symbols
>> and fail to disassemble the instructions that follow.
>
> I was told this was an objdump bug... it certainly
> seems like an objdump bug to me, because this works
> on the other architectures.
> https://bugs.linaro.org/show_bug.cgi?id=815

This may be a hold-over from arm32, which as you recall has a rather 
complicated scheme for annotating data vs instructions.

>> I took glibc as definitive, since that's code that definitely works.
>
> If we never use the 7th syscall argument (and we can't,
> since it doesn't exist on some hosts), then it's
> unnecessary work, though I guess coming back to fix
> all these host functions later might be tedious.

You're probably right about the 7th syscall argument on a 64-bit host.

> glibc's syscall() takes the system parameter as an int and
> does a sign-extending move into x0 with an uxtw.
> safe_syscall() takes a long, so it's already 64 bits.

Well, uxtw is a zero-extending move.  So...


r~

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

* Re: [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64
  2016-06-13 22:38         ` Richard Henderson
@ 2016-06-13 22:40           ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-13 22:40 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 23:38, Richard Henderson <rth@twiddle.net> wrote:
> On 06/13/2016 03:28 PM, Peter Maydell wrote:
>> glibc's syscall() takes the system parameter as an int and
>> does a sign-extending move into x0 with an uxtw.
>> safe_syscall() takes a long, so it's already 64 bits.
>
>
> Well, uxtw is a zero-extending move.  So...

Yeah 'sign-extending' was a thinko. But the point is
that for syscall() the input is 32 bits and the
value it feeds to the kernel is 64 bits, hence the
extension. For safe_syscall() the input is 64 bits
and the value fed to the kernel is also 64 bits,
so the most 'natural' thing is just to move a
64 bit value (saves the reader looking up whether
mov wX, wY clears the high half or not, if nothing else).

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386 Richard Henderson
@ 2016-06-14 11:58   ` Peter Maydell
  2016-06-14 15:47     ` Richard Henderson
  0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-06-14 11:58 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/i386/hostdep.h          |  34 ++++++++++
>  linux-user/host/i386/safe-syscall.inc.S | 110 ++++++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+)
>  create mode 100644 linux-user/host/i386/hostdep.h
>  create mode 100644 linux-user/host/i386/safe-syscall.inc.S
>
> diff --git a/linux-user/host/i386/hostdep.h b/linux-user/host/i386/hostdep.h
> new file mode 100644
> index 0000000..9e2b4d7
> --- /dev/null
> +++ b/linux-user/host/i386/hostdep.h
> @@ -0,0 +1,34 @@
> +/*
> + * hostdep.h : things which are dependent on the host architecture
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_HOSTDEP_H
> +#define QEMU_HOSTDEP_H
> +
> +/* 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)
> +{
> +    struct ucontext *uc = puc;
> +    greg_t *pcreg = &uc->uc_mcontext.gregs[REG_EIP];

user-exec.c has
#ifndef REG_EIP
/* for glibc 2.1 */
#define REG_EIP    EIP
#endif

Do we still care about glibc 2.1 ? (Probably not, 2.2 was
released fifteen years ago now...)

> +
> +    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/safe-syscall.inc.S b/linux-user/host/i386/safe-syscall.inc.S
> new file mode 100644
> index 0000000..f5f0c64
> --- /dev/null
> +++ b/linux-user/host/i386/safe-syscall.inc.S
> @@ -0,0 +1,110 @@
> +/*
> + * safe-syscall.inc.S : host-specific assembly fragment
> + * to handle signals occurring at the same time as system calls.
> + * This is intended to be included by linux-user/safe-syscall.S

Missing copyright/written by attribution ?

> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +        .global safe_syscall_base
> +        .global safe_syscall_start
> +        .global safe_syscall_end
> +        .type   safe_syscall_base, @function

I guess 4-space indent would match the rest of QEMU...

> +
> +        /* This is the entry point for making a system call. The calling
> +         * convention here is that of a C varargs function with the
> +         * first argument an 'int *' to the signal_pending flag, the
> +         * second one the system call number (as a 'long'), and all further
> +         * arguments being syscall arguments (also 'long').
> +         * We return a long which is the syscall's return value, which
> +         * may be negative-errno on failure. Conversion to the
> +         * -1-and-errno-set convention is done by the calling wrapper.
> +         */
> +safe_syscall_base:
> +        .cfi_startproc
> +        push    %ebp
> +        .cfi_adjust_cfa_offset 4
> +        .cfi_rel_offset ebp, 0
> +       push    %esi
> +        .cfi_adjust_cfa_offset 4
> +        .cfi_rel_offset esi, 0
> +       push    %edi

Odd indentation here.

> +        .cfi_adjust_cfa_offset 4
> +        .cfi_rel_offset edi, 0
> +       push    %ebx
> +        .cfi_adjust_cfa_offset 4
> +        .cfi_rel_offset ebx, 0
> +
> +        /* The syscall calling convention isn't the same as the C one:
> +         * we enter with 0(%esp) == return address
> +         *               4(%esp) == *signal_pending
> +         *               8(%esp) == syscall number
> +         *               12(%esp) ... 32(%esp) == syscall arguments
> +         *               and return the result in eax
> +         * and the syscall instruction needs
> +         *               eax == syscall number
> +         *               ebx, ecx, edx, esi, edi, ebp == syscall arguments
> +         *               and returns the result in eax
> +         * Shuffle everything around appropriately.
> +        * Note the 16 bytes that we pushed to save registers.
> +         */
> +        mov     12+16(%esp), %ebx       /* the syscall arguments */
> +        mov     16+16(%esp), %ecx
> +        mov     20+16(%esp), %edx
> +        mov     24+16(%esp), %esi
> +        mov     28+16(%esp), %edi
> +        mov     32+16(%esp), %ebp
> +
> +        /* This next sequence of code works in conjunction with the
> +         * rewind_if_safe_syscall_function(). If a signal is taken
> +         * and the interrupted PC is anywhere between 'safe_syscall_start'
> +         * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
> +         * The code sequence must therefore be able to cope with this, and
> +         * the syscall instruction must be the final one in the sequence.
> +         */
> +safe_syscall_start:
> +        /* if signal_pending is non-zero, don't do the call */
> +       mov     4+16(%esp), %eax        /* signal_pending */
> +        cmp    $0, (%eax)
> +        mov     8+16(%esp), %eax        /* syscall number */
> +        jnz     1f

Any particular reason for doing the jump after the mov?

> +        int    $0x80
> +safe_syscall_end:
> +        /* code path for having successfully executed the syscall */
> +        pop     %ebx
> +        .cfi_remember_state
> +        .cfi_def_cfa_offset 4

Shouldn't these all be ".cfi_adjust_cfa_offset -4" ? That's what glibc
uses AFAICT.

> +        .cfi_restore ebx
> +       pop     %edi
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore edi
> +       pop     %esi
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore esi
> +       pop     %ebp
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore ebp
> +        ret
> +
> +1:
> +        /* code path when we didn't execute the syscall */
> +        .cfi_restore_state
> +        mov     $-TARGET_ERESTARTSYS, %eax
> +        pop     %ebx
> +        .cfi_remember_state

We don't need to remember state here I think.

> +        .cfi_def_cfa_offset 4
> +        .cfi_restore ebx
> +       pop     %edi
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore edi
> +       pop     %esi
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore esi
> +       pop     %ebp
> +        .cfi_def_cfa_offset 4
> +        .cfi_restore ebp
> +        ret
> +        .cfi_endproc
> +
> +        .size   safe_syscall_base, .-safe_syscall_base
> --
> 2.5.5

Other than some trivialities like order of register push/pops
this is virtually identical code to the version I had, so it
must be right :-)

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
@ 2016-06-14 11:58   ` Peter Maydell
  2016-06-21 19:26   ` Riku Voipio
  1 sibling, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-14 11:58 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> Do what the comment says, test for signal_pending non-zero,
> rather than the current coe which tests for bit 0 non-zero.

"code"

>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/x86_64/safe-syscall.inc.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/linux-user/host/x86_64/safe-syscall.inc.S b/linux-user/host/x86_64/safe-syscall.inc.S
> index e09368d..f36992d 100644
> --- a/linux-user/host/x86_64/safe-syscall.inc.S
> +++ b/linux-user/host/x86_64/safe-syscall.inc.S
> @@ -67,8 +67,8 @@ safe_syscall_base:
>           */
>  safe_syscall_start:
>          /* if signal_pending is non-zero, don't do the call */
> -        testl   $1, (%rbp)
> -        jnz     return_ERESTARTSYS
> +        cmpl   $0, (%rbp)
> +        jnz     1f
>          syscall
>  safe_syscall_end:
>          /* code path for having successfully executed the syscall */
> @@ -78,7 +78,7 @@ safe_syscall_end:
>          .cfi_restore rbp
>          ret
>
> -return_ERESTARTSYS:
> +1:
>          /* code path when we didn't execute the syscall */
>          .cfi_restore_state
>          mov     $-TARGET_ERESTARTSYS, %rax

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

* Re: [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm Richard Henderson
@ 2016-06-14 12:04   ` Peter Maydell
  2016-06-14 15:53     ` Richard Henderson
  0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2016-06-14 12:04 UTC (permalink / raw)
  To: Richard Henderson; +Cc: QEMU Developers, Riku Voipio

On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/arm/hostdep.h          | 34 ++++++++++++++
>  linux-user/host/arm/safe-syscall.inc.S | 86 ++++++++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+)
>  create mode 100644 linux-user/host/arm/hostdep.h
>  create mode 100644 linux-user/host/arm/safe-syscall.inc.S
>
> diff --git a/linux-user/host/arm/hostdep.h b/linux-user/host/arm/hostdep.h
> new file mode 100644
> index 0000000..1426fb6
> --- /dev/null
> +++ b/linux-user/host/arm/hostdep.h
> @@ -0,0 +1,34 @@
> +/*
> + * hostdep.h : things which are dependent on the host architecture
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef QEMU_HOSTDEP_H
> +#define QEMU_HOSTDEP_H
> +
> +/* 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)
> +{
> +    struct ucontext *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/arm/safe-syscall.inc.S b/linux-user/host/arm/safe-syscall.inc.S
> new file mode 100644
> index 0000000..52f8883
> --- /dev/null
> +++ b/linux-user/host/arm/safe-syscall.inc.S
> @@ -0,0 +1,86 @@
> +/*
> + * safe-syscall.inc.S : host-specific assembly fragment
> + * to handle signals occurring at the same time as system calls.
> + * This is intended to be included by linux-user/safe-syscall.S
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +       .global safe_syscall_base
> +       .global safe_syscall_start
> +       .global safe_syscall_end
> +       .type   safe_syscall_base, %function
> +
> +       .cfi_sections   .debug_frame
> +
> +       .text
> +       .syntax unified
> +       .arm

Do we need a ".align 2" here? glibc has one.

> +
> +       /* This is the entry point for making a system call. The calling
> +        * convention here is that of a C varargs function with the
> +        * first argument an 'int *' to the signal_pending flag, the
> +        * second one the system call number (as a 'long'), and all further
> +        * arguments being syscall arguments (also 'long').
> +        * We return a long which is the syscall's return value, which
> +        * may be negative-errno on failure. Conversion to the
> +        * -1-and-errno-set convention is done by the calling wrapper.
> +        */
> +safe_syscall_base:
> +       .fnstart
> +       .cfi_startproc
> +       mov     ip, sp                  /* save entry stack */

Personally I find the numbered registers like "r12" easier to read than
the named versions like "ip" (I always have to look the latter up
to find out which register they actually are, so it saves effort
to just write r12 in the first place IMHO.)

> +       push    { r4, r5, r6, r7, r8, lr }
> +       .save   { r4, r5, r6, r7, r8, lr }
> +       .cfi_adjust_cfa_offset 24
> +       .cfi_rel_offset r4, 0
> +       .cfi_rel_offset r5, 4
> +       .cfi_rel_offset r6, 8
> +       .cfi_rel_offset r7, 12
> +       .cfi_rel_offset r8, 16
> +       .cfi_rel_offset lr, 20
> +
> +       /* The syscall calling convention isn't the same as the C one:
> +        * we enter with r0 == *signal_pending
> +        *               r1 == syscall number
> +        *               r2, r3, [sp+0] ... [sp+12] == syscall arguments
> +        *               and return the result in r0
> +        * and the syscall instruction needs
> +        *               r7 == syscall number
> +        *               r0 ... r6 == syscall arguments
> +        *               and returns the result in r0
> +        * Shuffle everything around appropriately.
> +        * Note the 16 bytes that we pushed to save registers.
> +        */
> +       mov     r8, r0                  /* copy signal_pending */
> +       mov     r7, r1                  /* syscall number */
> +       mov     r0, r2                  /* syscall args */
> +       mov     r1, r3
> +       ldm     ip, { r2, r3, r4, r5, r6 }
> +
> +       /* This next sequence of code works in conjunction with the
> +        * rewind_if_safe_syscall_function(). If a signal is taken
> +        * and the interrupted PC is anywhere between 'safe_syscall_start'
> +        * and 'safe_syscall_end' then we rewind it to 'safe_syscall_start'.
> +        * The code sequence must therefore be able to cope with this, and
> +        * the syscall instruction must be the final one in the sequence.
> +        */
> +safe_syscall_start:
> +       /* if signal_pending is non-zero, don't do the call */
> +       ldr     ip, [r8]                /* signal_pending */
> +       tst     ip, ip
> +       bne     1f
> +       swi     0
> +safe_syscall_end:
> +       /* code path for having successfully executed the syscall */
> +       pop     { r4, r5, r6, r7, r8, pc }

Worth commenting here that we assume that we're not trying to do
old ARMv4T interworking ?

> +
> +1:
> +       /* code path when we didn't execute the syscall */
> +       ldr     r0, =-TARGET_ERESTARTSYS
> +       pop     { r4, r5, r6, r7, r8, pc }
> +       .fnend
> +       .cfi_endproc
> +
> +       .size   safe_syscall_base, .-safe_syscall_base
> --
> 2.5.5
>

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM



-- 
12345678901234567890123456789012345678901234567890123456789012345678901234567890
         1         2         3         4         5         6         7         8

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

* Re: [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386
  2016-06-14 11:58   ` Peter Maydell
@ 2016-06-14 15:47     ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2016-06-14 15:47 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Riku Voipio

On 06/14/2016 04:58 AM, Peter Maydell wrote:
>> +    greg_t *pcreg = &uc->uc_mcontext.gregs[REG_EIP];
> 
> user-exec.c has
> #ifndef REG_EIP
> /* for glibc 2.1 */
> #define REG_EIP    EIP
> #endif
> 
> Do we still care about glibc 2.1 ? (Probably not, 2.2 was
> released fifteen years ago now...)
> 

Heh.  I would say not.  We've got other much more recent requirements.

>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +        .global safe_syscall_base
>> +        .global safe_syscall_start
>> +        .global safe_syscall_end
>> +        .type   safe_syscall_base, @function
> 
> I guess 4-space indent would match the rest of QEMU...

This is assembler not C.  My brain is tied to a 1-tab indent.

>> +        .cfi_rel_offset esi, 0
>> +       push    %edi
> 
> Odd indentation here.

Yeah, that's mixing code from your x86_64 version which uses spaces not tabs.

>> +        cmp    $0, (%eax)
>> +        mov     8+16(%esp), %eax        /* syscall number */
>> +        jnz     1f
> 
> Any particular reason for doing the jump after the mov?

No.  Indeed, recent cpus will fuse the cmp+jnz so they're better off together.

>> +        .cfi_def_cfa_offset 4
> 
> Shouldn't these all be ".cfi_adjust_cfa_offset -4" ? That's what glibc
> uses AFAICT.

Typo.  Good catch.

>> +        .cfi_remember_state
> 
> We don't need to remember state here I think.

Correct.  Cut and paste.

> Other than some trivialities like order of register push/pops
> this is virtually identical code to the version I had, so it
> must be right :-)

Heh.


r~

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

* Re: [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm
  2016-06-14 12:04   ` Peter Maydell
@ 2016-06-14 15:53     ` Richard Henderson
  0 siblings, 0 replies; 24+ messages in thread
From: Richard Henderson @ 2016-06-14 15:53 UTC (permalink / raw)
  To: Peter Maydell; +Cc: QEMU Developers, Riku Voipio

On 06/14/2016 05:04 AM, Peter Maydell wrote:
> On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  linux-user/host/arm/hostdep.h          | 34 ++++++++++++++
>>  linux-user/host/arm/safe-syscall.inc.S | 86 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 120 insertions(+)
>>  create mode 100644 linux-user/host/arm/hostdep.h
>>  create mode 100644 linux-user/host/arm/safe-syscall.inc.S
>>
>> diff --git a/linux-user/host/arm/hostdep.h b/linux-user/host/arm/hostdep.h
>> new file mode 100644
>> index 0000000..1426fb6
>> --- /dev/null
>> +++ b/linux-user/host/arm/hostdep.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * hostdep.h : things which are dependent on the host architecture
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#ifndef QEMU_HOSTDEP_H
>> +#define QEMU_HOSTDEP_H
>> +
>> +/* 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)
>> +{
>> +    struct ucontext *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/arm/safe-syscall.inc.S b/linux-user/host/arm/safe-syscall.inc.S
>> new file mode 100644
>> index 0000000..52f8883
>> --- /dev/null
>> +++ b/linux-user/host/arm/safe-syscall.inc.S
>> @@ -0,0 +1,86 @@
>> +/*
>> + * safe-syscall.inc.S : host-specific assembly fragment
>> + * to handle signals occurring at the same time as system calls.
>> + * This is intended to be included by linux-user/safe-syscall.S
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +       .global safe_syscall_base
>> +       .global safe_syscall_start
>> +       .global safe_syscall_end
>> +       .type   safe_syscall_base, %function
>> +
>> +       .cfi_sections   .debug_frame
>> +
>> +       .text
>> +       .syntax unified
>> +       .arm
> 
> Do we need a ".align 2" here? glibc has one.

It's probably best to have one.

>> +       mov     ip, sp                  /* save entry stack */
> 
> Personally I find the numbered registers like "r12" easier to read than
> the named versions like "ip" (I always have to look the latter up
> to find out which register they actually are, so it saves effort
> to just write r12 in the first place IMHO.)

That's fine.

>> +       /* code path for having successfully executed the syscall */
>> +       pop     { r4, r5, r6, r7, r8, pc }
> 
> Worth commenting here that we assume that we're not trying to do
> old ARMv4T interworking ?

At one time weren't we talking about dropping host support for really old arm
(pre arm5t?).  I seem to recall making a note about some possible cleanups to
tcg/arm/.

If so, then we shouldn't be noting that specifically here, but somewhere else.
Perhaps README (although it forwards most everything to the web site),
or even a configure test.


r~

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

* Re: [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates
  2016-06-13 22:09   ` Peter Maydell
@ 2016-06-21 19:08     ` Riku Voipio
  2016-06-21 19:49       ` Peter Maydell
  0 siblings, 1 reply; 24+ messages in thread
From: Riku Voipio @ 2016-06-21 19:08 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Richard Henderson, QEMU Developers

On Mon, Jun 13, 2016 at 11:09:17PM +0100, Peter Maydell wrote:
> On 13 June 2016 at 22:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 13 June 2016 at 22:45, Richard Henderson <rth@twiddle.net> wrote:
> >> Richard Henderson (6):
> >>   linux-user: fix x86_64 safe_syscall
> >>   linux-user: Provide safe_syscall for i386
> >>   linux-user: Provide safe_syscall for arm
> >>   linux-user: Provide safe_syscall for aarch64
> >>   linux-user: Provide safe_syscall for s390x
> >>   linux-user: Provide safe_syscall for ppc64
> >
> > I just spent today writing these for i386, arm and aarch64,
> > so we've duplicated work here :-(

On the positive side, duplication makes it easier to give
a really throughout review :)

> Also, I have a patchset which moves from the generic/hostdep.h
> to having explicitly one hostdep.h per supported architecture.
> That needs to go in before these, otherwise these break
> compilation unless you do a 'make clean'.
> 
> I'll send that out tomorrow, working version in
>  https://git.linaro.org/people/peter.maydell/qemu-arm.git/shortlog/refs/heads/sigrace-fixes

I've merged all pending patches except the safe_syscall ones to:

https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream

Peter, Richard, do you have an agreement who's versions of
the safe_syscall patches should be included? 

Riku

> We also have a bug in the signal.c code which I noticed
> trying to test my i386 safe_syscall: we call sigfillset()
> on the uc_sigmask field of the ucontext_t* that the
> kernel passes us as argument 3 of the signal handler.
> This trashes a lot of stuff on the stack because the
> libc headers say "sigset_t is 128 bytes" and the kernel
> says "it's only 8 bytes", so the sigfillset() writes -1
> to a lot of the stack that it shouldn't. (I don't know
> why glibc exposes a struct that isn't actually what
> the kernel provides here, but it's a very long standing
> confusion :-( )
> 
> thanks
> -- PMM

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

* Re: [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall
  2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
  2016-06-14 11:58   ` Peter Maydell
@ 2016-06-21 19:26   ` Riku Voipio
  1 sibling, 0 replies; 24+ messages in thread
From: Riku Voipio @ 2016-06-21 19:26 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel, peter.maydell

On Mon, Jun 13, 2016 at 02:45:21PM -0700, Richard Henderson wrote:
> Do what the comment says, test for signal_pending non-zero,
> rather than the current coe which tests for bit 0 non-zero.

Applied to linux-user with the type fixed,
Thanks

> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  linux-user/host/x86_64/safe-syscall.inc.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/host/x86_64/safe-syscall.inc.S b/linux-user/host/x86_64/safe-syscall.inc.S
> index e09368d..f36992d 100644
> --- a/linux-user/host/x86_64/safe-syscall.inc.S
> +++ b/linux-user/host/x86_64/safe-syscall.inc.S
> @@ -67,8 +67,8 @@ safe_syscall_base:
>           */
>  safe_syscall_start:
>          /* if signal_pending is non-zero, don't do the call */
> -        testl   $1, (%rbp)
> -        jnz     return_ERESTARTSYS
> +        cmpl	$0, (%rbp)
> +        jnz     1f
>          syscall
>  safe_syscall_end:
>          /* code path for having successfully executed the syscall */
> @@ -78,7 +78,7 @@ safe_syscall_end:
>          .cfi_restore rbp
>          ret
>  
> -return_ERESTARTSYS:
> +1:
>          /* code path when we didn't execute the syscall */
>          .cfi_restore_state
>          mov     $-TARGET_ERESTARTSYS, %rax
> -- 
> 2.5.5
> 

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

* Re: [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates
  2016-06-21 19:08     ` Riku Voipio
@ 2016-06-21 19:49       ` Peter Maydell
  0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2016-06-21 19:49 UTC (permalink / raw)
  To: Riku Voipio; +Cc: Richard Henderson, QEMU Developers

On 21 June 2016 at 20:08, Riku Voipio <riku.voipio@iki.fi> wrote:
> I've merged all pending patches except the safe_syscall ones to:
>
> https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream
>
> Peter, Richard, do you have an agreement who's versions of
> the safe_syscall patches should be included?

I'm happy to go with Richard's patches (he caught a
few issues my versions missed), but
(a) I had code review comments on some of them
(b) they'll have some trivial textual conflicts when they
get rebased on top of the patches that provide a
hostdep.h for every arch

thanks
-- PMM

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

end of thread, other threads:[~2016-06-21 19:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-13 21:45 [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Richard Henderson
2016-06-13 21:45 ` [Qemu-devel] [PATCH 1/6] linux-user: fix x86_64 safe_syscall Richard Henderson
2016-06-14 11:58   ` Peter Maydell
2016-06-21 19:26   ` Riku Voipio
2016-06-13 21:45 ` [Qemu-devel] [PATCH 2/6] linux-user: Provide safe_syscall for i386 Richard Henderson
2016-06-14 11:58   ` Peter Maydell
2016-06-14 15:47     ` Richard Henderson
2016-06-13 21:45 ` [Qemu-devel] [PATCH 3/6] linux-user: Provide safe_syscall for arm Richard Henderson
2016-06-14 12:04   ` Peter Maydell
2016-06-14 15:53     ` Richard Henderson
2016-06-13 21:45 ` [Qemu-devel] [PATCH 4/6] linux-user: Provide safe_syscall for aarch64 Richard Henderson
2016-06-13 22:04   ` Peter Maydell
2016-06-13 22:21     ` Richard Henderson
2016-06-13 22:28       ` Peter Maydell
2016-06-13 22:31         ` Peter Maydell
2016-06-13 22:38         ` Richard Henderson
2016-06-13 22:40           ` Peter Maydell
2016-06-13 21:45 ` [Qemu-devel] [PATCH 5/6] linux-user: Provide safe_syscall for s390x Richard Henderson
2016-06-13 21:45 ` [Qemu-devel] [PATCH 6/6] linux-user: Provide safe_syscall for ppc64 Richard Henderson
2016-06-13 22:23   ` Peter Maydell
2016-06-13 21:53 ` [Qemu-devel] [PATCH 0/6] linux-user: safe_syscall updates Peter Maydell
2016-06-13 22:09   ` Peter Maydell
2016-06-21 19:08     ` Riku Voipio
2016-06-21 19:49       ` Peter Maydell

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).