[PATCH] um: always use the internal copy of the FP registers

[Tiwei Bie]

On 2024/9/13 16:22, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg at intel.com>
> 
> When switching from userspace to the kernel, all registers including the
> FP registers are copied into the kernel and restored later on. As such,
> the true source for the FP register state is actually already in the
> kernel and they should never be grabbed from the userspace process.
> 
> Change the various places to simply copy the data from the internal FP
> register storage area.

Cool! I had similar thoughts, but I haven't tried to make it happen.

> Note that on i386 the format of PTRACE_GETFPREGS
> and PTRACE_GETFPXREGS is different enough that conversion would be
> needed. With this patch, -EINVAL is returned if the non-native format is
> requested.
> 
> The upside is, that this patchset fixes setting registers via ptrace
> (which simply did not work before) as well as fixing setting floating
> point registers using the mcontext on signal return on i386.
> 
> Signed-off-by: Benjamin Berg <benjamin.berg at intel.com>
> 
> ---
> 
> I think that ideally more changs should be done in this space. For a
> start, one can remove support for hosts without PTRACE_GETFPXREGS. But,
> more importantly, UML should probably implement the regset API, which
> would considerably improve both ptrace and coredump generation and
> should also make it easy to fix the cases where this patch returns an
> error.
> ---
>  arch/um/include/shared/registers.h |  6 ---
>  arch/um/kernel/process.c           | 11 ++++-
>  arch/x86/um/os-Linux/registers.c   | 12 +++---
>  arch/x86/um/ptrace_32.c            | 50 ++++++++++++-----------
>  arch/x86/um/ptrace_64.c            | 20 ++++-----
>  arch/x86/um/signal.c               | 65 ++++++++++++++----------------
>  6 files changed, 80 insertions(+), 84 deletions(-)
> 
> diff --git a/arch/um/include/shared/registers.h b/arch/um/include/shared/registers.h
> index a0450326521c..7d81b2339a48 100644
> --- a/arch/um/include/shared/registers.h
> +++ b/arch/um/include/shared/registers.h
> @@ -8,12 +8,6 @@
>  
>  #include <sysdep/ptrace.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);
> -extern int restore_fpx_registers(int pid, unsigned long *fp_regs);
>  extern int init_pid_registers(int pid);
>  extern void get_safe_registers(unsigned long *regs, unsigned long *fp_regs);
>  extern int get_fp_registers(int pid, unsigned long *regs);
> diff --git a/arch/um/kernel/process.c b/arch/um/kernel/process.c
> index be2856af6d4c..ad798d40f8a4 100644
> --- a/arch/um/kernel/process.c
> +++ b/arch/um/kernel/process.c
> @@ -290,8 +290,15 @@ unsigned long __get_wchan(struct task_struct *p)
>  
>  int elf_core_copy_task_fpregs(struct task_struct *t, elf_fpregset_t *fpu)
>  {
> -	int cpu = current_thread_info()->cpu;
> +#ifdef CONFIG_X86_32
> +	extern int have_fpx_regs;
>  
> -	return save_i387_registers(userspace_pid[cpu], (unsigned long *) fpu);
> +	/* FIXME: A plain copy does not work on i386 with have_fpx_regs */
> +	if (have_fpx_regs)
> +		return -1;
> +#endif
> +	memcpy(fpu, &t->thread.regs.regs.fp, sizeof(*fpu));
> +
> +	return 0;
>  }

This function is expected to return a boolean value which should be
true on success.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_elf.c?h=v6.11-rc7#n1816

Regards,
Tiwei