user-mode-linux-devel

[uml-devel] [PATCH 0/3] um: Add support for extended processor state

[Eli C. <eli...@gm...>]

This series adds support for the extended processor state (XSTATE) for
x86_64 UML, especially the YMM registers used by AVX/AVX2 instructions.

Modern userspace programs built with AVX can now run inside x86_64 UML
without YMM registers getting corrupted.

Eli Cooper (3):
  um: extend _fpstate to _xstate
  um: add extended processor state save/restore support
  um: fix ptrace PTRACE_GETFPREGS and PTRACE_SETFPREG support

 arch/um/include/shared/registers.h    |  2 ++
 arch/um/kernel/process.c              |  2 +-
 arch/x86/um/os-Linux/registers.c      | 49 +++++++++++++++++++++++++++++++++--
 arch/x86/um/ptrace_32.c               |  4 +--
 arch/x86/um/ptrace_64.c               | 14 +++++-----
 arch/x86/um/shared/sysdep/ptrace_64.h |  4 +--
 arch/x86/um/signal.c                  | 26 +++++++++----------
 arch/x86/um/user-offsets.c            |  2 +-
 8 files changed, 73 insertions(+), 30 deletions(-)

-- 
2.7.2

[uml-devel] [PATCH 2/3] um: add extended processor state save/restore support

[Eli C. <eli...@gm...>]

This patch extends save_fp_registers() and restore_fp_registers() to use
PTRACE_GETREGSET and PTRACE_SETREGSET with the XSTATE note type, adding
support for new processor state extensions.

Now these functions expect *fp_regs to have the space of an _xstate struct.

When the new ptrace requests are unavailable, it falls back to the old
PTRACE_GETFPREGS and PTRACE_SETFPREGS methods, which have been renamed to
save_i387_registers() and restore_i387_registers().

Signed-off-by: Eli Cooper <eli...@gm...>
---
 arch/um/include/shared/registers.h    |  2 ++
 arch/um/kernel/process.c              |  2 +-
 arch/x86/um/os-Linux/registers.c      | 49 +++++++++++++++++++++++++++++++++--
 arch/x86/um/shared/sysdep/ptrace_64.h |  4 +--
 4 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/arch/um/include/shared/registers.h b/arch/um/include/shared/registers.h
index f5b7635..a74449b 100644
--- a/arch/um/include/shared/registers.h
+++ b/arch/um/include/shared/registers.h
@@ -9,6 +9,8 @@
 #include <sysdep/ptrace.h>
 #include <sysdep/archsetjmp.h>
 
+extern int save_i387_registers(int pid, unsigned long *fp_regs);
+extern int restore_i387_registers(int pid, unsigned long *fp_regs);
 extern int save_fp_registers(int pid, unsigned long *fp_regs);
 extern int restore_fp_registers(int pid, unsigned long *fp_regs);
 extern int save_fpx_registers(int pid, unsigned long *fp_regs);
diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
index 48af59a..d55a473 100644
--- a/arch/um/kernel/process.c
+++ b/arch/um/kernel/process.c
@@ -402,6 +402,6 @@ int elf_core_copy_fpregs(struct task_struct *t, elf_fpregset_t *fpu)
 {
 	int cpu = current_thread_info()->cpu;
 
-	return save_fp_registers(userspace_pid[cpu], (unsigned long *) fpu);
+	return save_i387_registers(userspace_pid[cpu], (unsigned long *) fpu);
 }
 
diff --git a/arch/x86/um/os-Linux/registers.c b/arch/x86/um/os-Linux/registers.c
index 41bfe84..c7f12c9 100644
--- a/arch/x86/um/os-Linux/registers.c
+++ b/arch/x86/um/os-Linux/registers.c
@@ -11,21 +11,56 @@
 #endif
 #include <longjmp.h>
 #include <sysdep/ptrace_user.h>
+#include <sys/uio.h>
+#include <asm/sigcontext.h>
+#include <linux/elf.h>
 
-int save_fp_registers(int pid, unsigned long *fp_regs)
+int have_xstate_support;
+
+int save_i387_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_GETFPREGS, pid, 0, fp_regs) < 0)
 		return -errno;
 	return 0;
 }
 
-int restore_fp_registers(int pid, unsigned long *fp_regs)
+int save_fp_registers(int pid, unsigned long *fp_regs)
+{
+	struct iovec iov;
+
+	if (have_xstate_support) {
+		iov.iov_base = fp_regs;
+		iov.iov_len = sizeof(struct _xstate);
+		if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
+			return -errno;
+		return 0;
+	} else {
+		return save_i387_registers(pid, fp_regs);
+	}
+}
+
+int restore_i387_registers(int pid, unsigned long *fp_regs)
 {
 	if (ptrace(PTRACE_SETFPREGS, pid, 0, fp_regs) < 0)
 		return -errno;
 	return 0;
 }
 
+int restore_fp_registers(int pid, unsigned long *fp_regs)
+{
+	struct iovec iov;
+
+	if (have_xstate_support) {
+		iov.iov_base = fp_regs;
+		iov.iov_len = sizeof(struct _xstate);
+		if (ptrace(PTRACE_SETREGSET, pid, NT_X86_XSTATE, &iov) < 0)
+			return -errno;
+		return 0;
+	} else {
+		return restore_i387_registers(pid, fp_regs);
+	}
+}
+
 #ifdef __i386__
 int have_fpx_regs = 1;
 int save_fpx_registers(int pid, unsigned long *fp_regs)
@@ -85,6 +120,16 @@ int put_fp_registers(int pid, unsigned long *regs)
 	return restore_fp_registers(pid, regs);
 }
 
+void arch_init_registers(int pid)
+{
+	struct _xstate fp_regs;
+	struct iovec iov;
+
+	iov.iov_base = &fp_regs;
+	iov.iov_len = sizeof(struct _xstate);
+	if (ptrace(PTRACE_GETREGSET, pid, NT_X86_XSTATE, &iov) == 0)
+		have_xstate_support = 1;
+}
 #endif
 
 unsigned long get_thread_reg(int reg, jmp_buf *buf)
diff --git a/arch/x86/um/shared/sysdep/ptrace_64.h b/arch/x86/um/shared/sysdep/ptrace_64.h
index 919789f..0dc223a 100644
--- a/arch/x86/um/shared/sysdep/ptrace_64.h
+++ b/arch/x86/um/shared/sysdep/ptrace_64.h
@@ -57,8 +57,6 @@
 #define UPT_SYSCALL_ARG5(r) UPT_R8(r)
 #define UPT_SYSCALL_ARG6(r) UPT_R9(r)
 
-static inline void arch_init_registers(int pid)
-{
-}
+extern void arch_init_registers(int pid);
 
 #endif
-- 
2.7.2

[uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[Eli C. <eli...@gm...>]

Extends _fpstate to _xstate, in order to hold AVX/YMM registers.
Due to increased frame size, compilers might emit some warnings.

Signed-off-by: Eli Cooper <eli...@gm...>
---
 arch/x86/um/signal.c       | 26 +++++++++++++-------------
 arch/x86/um/user-offsets.c |  2 +-
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/um/signal.c b/arch/x86/um/signal.c
index 14fcd01..e66bb06 100644
--- a/arch/x86/um/signal.c
+++ b/arch/x86/um/signal.c
@@ -225,10 +225,10 @@ static int copy_sc_from_user(struct pt_regs *regs,
 	} else
 #endif
 	{
-		struct user_i387_struct fp;
+		struct _xstate fp;
 
 		err = copy_from_user(&fp, (void *)sc.fpstate,
-				     sizeof(struct user_i387_struct));
+				     sizeof(struct _xstate));
 		if (err)
 			return 1;
 
@@ -244,7 +244,7 @@ static int copy_sc_from_user(struct pt_regs *regs,
 }
 
 static int copy_sc_to_user(struct sigcontext __user *to,
-			   struct _fpstate __user *to_fp, struct pt_regs *regs,
+			   struct _xstate __user *to_fp, struct pt_regs *regs,
 			   unsigned long mask)
 {
 	struct sigcontext sc;
@@ -310,25 +310,25 @@ static int copy_sc_to_user(struct sigcontext __user *to,
 			return 1;
 		}
 
-		err = convert_fxsr_to_user(to_fp, &fpx);
+		err = convert_fxsr_to_user(&to_fp->fpstate, &fpx);
 		if (err)
 			return 1;
 
-		err |= __put_user(fpx.swd, &to_fp->status);
-		err |= __put_user(X86_FXSR_MAGIC, &to_fp->magic);
+		err |= __put_user(fpx.swd, &to_fp->fpstate.status);
+		err |= __put_user(X86_FXSR_MAGIC, &to_fp->fpstate.magic);
 		if (err)
 			return 1;
 
-		if (copy_to_user(&to_fp->_fxsr_env[0], &fpx,
+		if (copy_to_user(&to_fp->fpstate._fxsr_env[0], &fpx,
 				 sizeof(struct user_fxsr_struct)))
 			return 1;
 	} else
 #endif
 	{
-		struct user_i387_struct fp;
+		struct _xstate fp;
 
 		err = save_fp_registers(pid, (unsigned long *) &fp);
-		if (copy_to_user(to_fp, &fp, sizeof(struct user_i387_struct)))
+		if (copy_to_user(to_fp, &fp, sizeof(struct _xstate)))
 			return 1;
 	}
 
@@ -337,7 +337,7 @@ static int copy_sc_to_user(struct sigcontext __user *to,
 
 #ifdef CONFIG_X86_32
 static int copy_ucontext_to_user(struct ucontext __user *uc,
-				 struct _fpstate __user *fp, sigset_t *set,
+				 struct _xstate __user *fp, sigset_t *set,
 				 unsigned long sp)
 {
 	int err = 0;
@@ -353,7 +353,7 @@ struct sigframe
 	char __user *pretcode;
 	int sig;
 	struct sigcontext sc;
-	struct _fpstate fpstate;
+	struct _xstate fpstate;
 	unsigned long extramask[_NSIG_WORDS-1];
 	char retcode[8];
 };
@@ -366,7 +366,7 @@ struct rt_sigframe
 	void __user *puc;
 	struct siginfo info;
 	struct ucontext uc;
-	struct _fpstate fpstate;
+	struct _xstate fpstate;
 	char retcode[8];
 };
 
@@ -495,7 +495,7 @@ struct rt_sigframe
 	char __user *pretcode;
 	struct ucontext uc;
 	struct siginfo info;
-	struct _fpstate fpstate;
+	struct _xstate fpstate;
 };
 
 int setup_signal_stack_si(unsigned long stack_top, struct ksignal *ksig,
diff --git a/arch/x86/um/user-offsets.c b/arch/x86/um/user-offsets.c
index ce7e360..bab4362 100644
--- a/arch/x86/um/user-offsets.c
+++ b/arch/x86/um/user-offsets.c
@@ -52,7 +52,7 @@ void foo(void)
 	DEFINE(HOST_GS, GS);
 	DEFINE(HOST_ORIG_AX, ORIG_EAX);
 #else
-	DEFINE(HOST_FP_SIZE, sizeof(struct _fpstate) / sizeof(unsigned long));
+	DEFINE(HOST_FP_SIZE, sizeof(struct _xstate) / sizeof(unsigned long));
 	DEFINE_LONGS(HOST_BX, RBX);
 	DEFINE_LONGS(HOST_CX, RCX);
 	DEFINE_LONGS(HOST_DI, RDI);
-- 
2.7.2

[uml-devel] [PATCH 3/3] um: fix ptrace PTRACE_GETFPREGS and PTRACE_SETFPREG support

[Eli C. <eli...@gm...>]

This patch makes ptrace in UML responde to PTRACE_GETFPREGS/_SETFPREG
requests with a user_i387_struct (thus independent from HOST_FP_SIZE), and
by calling save_i387_registers() and restore_i387_registers() instead of
the extended save_fp_registers() and restore_fp_registers() functions.

Signed-off-by: Eli Cooper <eli...@gm...>
---
 arch/x86/um/ptrace_32.c |  4 ++--
 arch/x86/um/ptrace_64.c | 14 ++++++--------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/um/ptrace_32.c b/arch/x86/um/ptrace_32.c
index 47c78d5..2eeaf2c 100644
--- a/arch/x86/um/ptrace_32.c
+++ b/arch/x86/um/ptrace_32.c
@@ -194,7 +194,7 @@ static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
 	struct user_i387_struct fpregs;
 
-	err = save_fp_registers(userspace_pid[cpu], (unsigned long *) &fpregs);
+	err = save_i387_registers(userspace_pid[cpu],
+				  (unsigned long *) &fpregs);
 	if (err)
 		return err;
 
@@ -214,7 +214,7 @@ static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 	if (n > 0)
 		return -EFAULT;
 
-	return restore_fp_registers(userspace_pid[cpu],
+	return restore_i387_registers(userspace_pid[cpu],
 				    (unsigned long *) &fpregs);
 }
 
diff --git a/arch/x86/um/ptrace_64.c b/arch/x86/um/ptrace_64.c
index a629694..14c9ab1 100644
--- a/arch/x86/um/ptrace_64.c
+++ b/arch/x86/um/ptrace_64.c
@@ -222,14 +222,13 @@ int is_syscall(unsigned long addr)
 static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
 	int err, n, cpu = ((struct thread_info *) child->stack)->cpu;
-	long fpregs[HOST_FP_SIZE];
+	struct user_i387_struct fpregs;
 
-	BUG_ON(sizeof(*buf) != sizeof(fpregs));
-	err = save_fp_registers(userspace_pid[cpu], fpregs);
+	err = save_i387_registers(userspace_pid[cpu], &fpregs);
 	if (err)
 		return err;
 
-	n = copy_to_user(buf, fpregs, sizeof(fpregs));
+	n = copy_to_user(buf, &fpregs, sizeof(fpregs));
 	if (n > 0)
 		return -EFAULT;
 
@@ -239,14 +238,13 @@ static int get_fpregs(struct user_i387_struct __user *buf, struct task_struct *c
 static int set_fpregs(struct user_i387_struct __user *buf, struct task_struct *child)
 {
 	int n, cpu = ((struct thread_info *) child->stack)->cpu;
-	long fpregs[HOST_FP_SIZE];
+	struct user_i387_struct fpregs;
 
-	BUG_ON(sizeof(*buf) != sizeof(fpregs));
-	n = copy_from_user(fpregs, buf, sizeof(fpregs));
+	n = copy_from_user(&fpregs, buf, sizeof(fpregs));
 	if (n > 0)
 		return -EFAULT;
 
-	return restore_fp_registers(userspace_pid[cpu], fpregs);
+	return restore_i387_registers(userspace_pid[cpu], &fpregs);
 }
 
 long subarch_ptrace(struct task_struct *child, long request,
-- 
2.7.2

Re: [uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[Richard W. <ri...@no...>]

Hi!

Am 06.03.2016 um 15:36 schrieb Eli Cooper:
> Extends _fpstate to _xstate, in order to hold AVX/YMM registers.
> Due to increased frame size, compilers might emit some warnings.

Hmm, this needs rework. Having everything on the stack is not good.
Can you also create a selftest such that this bug cannot happen again?

Thanks,
//richard

Re: [uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[Eli C. <eli...@gm...>]

Hi Richard,

On 2016/3/10 4:44, Richard Weinberger wrote:
> Hmm, this needs rework. Having everything on the stack is not good.

Okay, I'll rework the functions whose stack size is greater than the
warning threshold by using kmalloc.

> Can you also create a selftest such that this bug cannot happen again?

It seems that instead of writing a self-test showing this problem cannot
happen again, I wrote a test that manifested another bug that is not
directly related to my patch.

Without applying my patch, the current UML should support XMM registers
because those are covered by _fpstate and PTRACE_GETFPREGS. But it
seemed that XMM registers are not restored after the signal handler returns.

In the following quick test, the main loop should run indefinitely
despite XMM registers are modified by the signal handler. But in UML,
the loop breaks randomly within a minute or two, showing that the
registers are corrupted. So far I haven't found the cause. Any hints?

Thanks,
Eli

---
/* test if signal handling preserves XMM registers */
#include <stdio.h>
#include <unistd.h>
#include <signal.h>

int count;

void sighandler(int signum)
{
        count++;

        /* alarm(1) without calling libc */
        asm("mov $0x1,%rdi");
        asm("mov $0x25,%rax");
        asm("syscall");

        asm("movq $0xdeadbeef,%r11");
        /* the following two instructions
         * modify xmm0 and xmm1 registers */
        asm("vmovq %r11,%xmm0");
        asm("vmovq %r11,%xmm1");
}

int main()
{
        struct sigaction act;
        double a = 3.14159, b = 2.71828;

        act.sa_handler = sighandler;
        act.sa_flags = 0;
        sigemptyset(&act.sa_mask);
        sigaction(SIGALRM, &act, NULL);

        alarm(1);

        /* this loop should run indefinitely */
        while (a + b == a + b) ;

        printf("count = %d\n", count);
        return 1;
}

Re: [uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[<st...@ni...>]

> /* test if signal handling preserves XMM registers */
> #include <stdio.h>
> #include <unistd.h>
> #include <signal.h>
>
> int count;
>
> void sighandler(int signum)
> {
>         count++;
>
>         /* alarm(1) without calling libc */
>         asm("mov $0x1,%rdi");
>         asm("mov $0x25,%rax");
>         asm("syscall");
>
>         asm("movq $0xdeadbeef,%r11");
>         /* the following two instructions
>          * modify xmm0 and xmm1 registers */
>         asm("vmovq %r11,%xmm0");
>         asm("vmovq %r11,%xmm1");
> }
>
> int main()
> {
>         struct sigaction act;
>         double a = 3.14159, b = 2.71828;
>
>         act.sa_handler = sighandler;
>         act.sa_flags = 0;
>         sigemptyset(&act.sa_mask);
>         sigaction(SIGALRM, &act, NULL);
>
>         alarm(1);
>
>         /* this loop should run indefinitely */
>         while (a + b == a + b) ;
>
>         printf("count = %d\n", count);

if you put a break here in gdb, and dump the FPU regs, what does it 
say?

Can be that either we a lacking or missplaced a fwait (to flush FPU)

>         return 1;
> }

Re: [uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[Richard W. <ri...@no...>]

Eli,

Am 12.03.2016 um 08:08 schrieb Eli Cooper:
> Hi Richard,
> 
> On 2016/3/10 4:44, Richard Weinberger wrote:
>> Hmm, this needs rework. Having everything on the stack is not good.
> 
> Okay, I'll rework the functions whose stack size is greater than the
> warning threshold by using kmalloc.

I fear it is not that easy. Having a kmalloc() per context switch would
be every expensive. Even for UML.

>> Can you also create a selftest such that this bug cannot happen again?
> 
> It seems that instead of writing a self-test showing this problem cannot
> happen again, I wrote a test that manifested another bug that is not
> directly related to my patch.
> 
> Without applying my patch, the current UML should support XMM registers
> because those are covered by _fpstate and PTRACE_GETFPREGS. But it
> seemed that XMM registers are not restored after the signal handler returns.
> 
> In the following quick test, the main loop should run indefinitely
> despite XMM registers are modified by the signal handler. But in UML,
> the loop breaks randomly within a minute or two, showing that the
> registers are corrupted. So far I haven't found the cause. Any hints?

Meh. :(
Can you figure out whether the issue depends on the host kernel? i.e. try something older
and Linus' tree.
UML is a heavy user of ptrace(), maybe the recent FPU cleanup on x86 broke something.

Thanks,
//richard

> Thanks,
> Eli
> 
> ---
> /* test if signal handling preserves XMM registers */
> #include <stdio.h>
> #include <unistd.h>
> #include <signal.h>
> 
> int count;
> 
> void sighandler(int signum)
> {
>         count++;
> 
>         /* alarm(1) without calling libc */
>         asm("mov $0x1,%rdi");
>         asm("mov $0x25,%rax");
>         asm("syscall");
> 
>         asm("movq $0xdeadbeef,%r11");
>         /* the following two instructions
>          * modify xmm0 and xmm1 registers */
>         asm("vmovq %r11,%xmm0");
>         asm("vmovq %r11,%xmm1");
> }
> 
> int main()
> {
>         struct sigaction act;
>         double a = 3.14159, b = 2.71828;
> 
>         act.sa_handler = sighandler;
>         act.sa_flags = 0;
>         sigemptyset(&act.sa_mask);
>         sigaction(SIGALRM, &act, NULL);
> 
>         alarm(1);
> 
>         /* this loop should run indefinitely */
>         while (a + b == a + b) ;
> 
>         printf("count = %d\n", count);
>         return 1;
> }
> 

Re: [uml-devel] [PATCH 1/3] um: extend _fpstate to _xstate

[Eli C. <eli...@gm...>]

Hi Richard,

On 2016/3/13 15:58, Richard Weinberger wrote:
> Eli,
>
> Am 12.03.2016 um 08:08 schrieb Eli Cooper:
>> > Hi Richard,
>> > 
>> > On 2016/3/10 4:44, Richard Weinberger wrote:
>>> >> Hmm, this needs rework. Having everything on the stack is not good.
>> > 
>> > Okay, I'll rework the functions whose stack size is greater than the
>> > warning threshold by using kmalloc.
> I fear it is not that easy. Having a kmalloc() per context switch would
> be every expensive. Even for UML.

Actually only two functions' stack frame size exceed kernel's default
warning threshold (1024 bytes) after the _xstate extension, i.e.,
copy_sc_from_user and copy_sc_to_user. That's because they have an
_xstate on stack as well as a sigcontext, which contains another
_xstate. Context switches due to signal handling are rare; thus I think
having a kmalloc() for signal handling is acceptable.
>>> >> Can you also create a selftest such that this bug cannot happen again?
>> > 
>> > It seems that instead of writing a self-test showing this problem cannot
>> > happen again, I wrote a test that manifested another bug that is not
>> > directly related to my patch.
>> > 
>> > Without applying my patch, the current UML should support XMM registers
>> > because those are covered by _fpstate and PTRACE_GETFPREGS. But it
>> > seemed that XMM registers are not restored after the signal handler returns.
>> > 
>> > In the following quick test, the main loop should run indefinitely
>> > despite XMM registers are modified by the signal handler. But in UML,
>> > the loop breaks randomly within a minute or two, showing that the
>> > registers are corrupted. So far I haven't found the cause. Any hints?
> Meh. :(
> Can you figure out whether the issue depends on the host kernel? i.e. try something older
> and Linus' tree.
> UML is a heavy user of ptrace(), maybe the recent FPU cleanup on x86 broke something.
No, it seems that this issue does not depend on the host kernel, UML
kernel, or CPU. I can reproduce this bug on a variety of combinations of
them, with the host kernel ranging from 2.6.32 to 3.19 to 4.5.

Thanks,
Eli
>
> Thanks,
> //richard
>