[PATCH] um: Enable preemption in UML

[Anton Ivanov]

On 20/09/2023 15:49, Peter Lafreniere wrote:
> On Wed, Sep 20, 2023 at 08:31, anton.ivanov at cambridgegreys.com wrote:
>> From: Anton Ivanov anton.ivanov at cambridgegreys.com
>>
>>
>> Preemption requires saving/restoring FPU state. This patch
>> adds support for it using GCC intrinsics.
>>
>> Signed-off-by: Anton Ivanov anton.ivanov at cambridgegreys.com
>>
>> ---
>> arch/um/Kconfig | 1 -
>> arch/um/Makefile | 3 +-
>> arch/um/include/asm/fpu/api.h | 4 +-
>> arch/um/include/asm/processor-generic.h | 1 +
>> arch/um/kernel/Makefile | 2 +-
>> arch/um/kernel/fpu.c | 49 +++++++++++++++++++++++++
>> 6 files changed, 55 insertions(+), 5 deletions(-)
>> create mode 100644 arch/um/kernel/fpu.c
>>
>> diff --git a/arch/um/Kconfig b/arch/um/Kconfig
>> index b5e179360534..603f5fd82293 100644
>> --- a/arch/um/Kconfig
>> +++ b/arch/um/Kconfig
>> @@ -11,7 +11,6 @@ config UML
>> select ARCH_HAS_KCOV
>> select ARCH_HAS_STRNCPY_FROM_USER
>> select ARCH_HAS_STRNLEN_USER
>> - select ARCH_NO_PREEMPT
>> select HAVE_ARCH_AUDITSYSCALL
>> select HAVE_ARCH_KASAN if X86_64
>> select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN
>> diff --git a/arch/um/Makefile b/arch/um/Makefile
>> index 82f05f250634..35a059baadeb 100644
>> --- a/arch/um/Makefile
>> +++ b/arch/um/Makefile
>> @@ -61,7 +61,8 @@ KBUILD_CFLAGS += $(CFLAGS) $(CFLAGS-y) -D__arch_um__ \
>> $(ARCH_INCLUDE) $(MODE_INCLUDE) -Dvmap=kernel_vmap \
>> -Dlongjmp=kernel_longjmp -Dsetjmp=kernel_setjmp \
>> -Din6addr_loopback=kernel_in6addr_loopback \
>> - -Din6addr_any=kernel_in6addr_any -Dstrrchr=kernel_strrchr
>> + -Din6addr_any=kernel_in6addr_any -Dstrrchr=kernel_strrchr \
>> + -mxsave
> Since we're now using a new isa extention, I would add a check in
> linux_main() with a warning message if xsave is not available.
>
> It also wouldn't hurt to only require xsave if CONFIG_PREEMPT is enabled.

Indeed. The full area ends up being quite large - half a page or thereabouts.

>
>> KBUILD_RUSTFLAGS += -Crelocation-model=pie
>>
>> diff --git a/arch/um/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h
>> index 71bfd9ef3938..0094624ae9b4 100644
>> --- a/arch/um/include/asm/fpu/api.h
>> +++ b/arch/um/include/asm/fpu/api.h
>> @@ -8,8 +8,8 @@
>> * of x86 optimized copy, xor, etc routines into the
>> * UML code tree. /
>>
>> -#define kernel_fpu_begin() (void)0
>> -#define kernel_fpu_end() (void)0
>> +void kernel_fpu_begin(void);
>> +void kernel_fpu_end(void);
>>
>> static inline bool irq_fpu_usable(void)
>> {
>> diff --git a/arch/um/include/asm/processor-generic.h b/arch/um/include/asm/processor-generic.h
>> index 7414154b8e9a..1c3287f1a444 100644
>> --- a/arch/um/include/asm/processor-generic.h
>> +++ b/arch/um/include/asm/processor-generic.h
>> @@ -44,6 +44,7 @@ struct thread_struct {
>> } cb;
>> } u;
>> } request;
>> + u8 fpu[512] __aligned(16); / Intel docs require xsave/xrestore area to be aligned to 16 bytes /
>> };
>>
>> #define INIT_THREAD \
>> diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile
>> index 811188be954c..5d9fbaa544be 100644
>> --- a/arch/um/kernel/Makefile
>> +++ b/arch/um/kernel/Makefile
>> @@ -16,7 +16,7 @@ extra-y := vmlinux.lds
>>
>> obj-y = config.o exec.o exitcode.o irq.o ksyms.o mem.o \
>> physmem.o process.o ptrace.o reboot.o sigio.o \
>> - signal.o sysrq.o time.o tlb.o trap.o \
>> + signal.o sysrq.o time.o tlb.o trap.o fpu.o\
>> um_arch.o umid.o maccess.o kmsg_dump.o capflags.o skas/
>> obj-y += load_file.o
>>
>> diff --git a/arch/um/kernel/fpu.c b/arch/um/kernel/fpu.c
>> new file mode 100644
>> index 000000000000..4142a08c474d
>> --- /dev/null
>> +++ b/arch/um/kernel/fpu.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/
>> + * Copyright (C) 2023 Cambridge Greys Ltd
>> + * Copyright (C) 2023 Red Hat Inc
>> + */
>> +
>> +#include <linux/cpu.h>
>>
>> +#include <linux/init.h>
>>
>> +#include <asm/fpu/api.h>
>>
>> +
>> +/*
>> + * The critical section between kernel_fpu_begin() and kernel_fpu_end()
>> + * is non-reentrant. It is the caller's responsibility to avoid reentrance.
>> + */
>> +
>> +static DEFINE_PER_CPU(bool, in_kernel_fpu);
>> +
>> +void kernel_fpu_begin(void)
>> +{
>> + preempt_disable();
>> +
>> + WARN_ON(this_cpu_read(in_kernel_fpu));
>> +
>> + this_cpu_write(in_kernel_fpu, true);
>> +
>> +#ifdef CONFIG_64BIT
>> + __builtin_ia32_fxsave64(&current->thread.fpu);
>>
>> +#else
>> + __builtin_ia32_fxsave(&current->thread.fpu);
>>
>> +#endif
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(kernel_fpu_begin);
>> +
>> +void kernel_fpu_end(void)
>> +{
>> + WARN_ON(!this_cpu_read(in_kernel_fpu));
>> +
>> +#ifdef CONFIG_64BIT
>> + __builtin_ia32_fxrstor64(&current->thread.fpu);
>>
>> +#else
>> + __builtin_ia32_fxrstor(&current->thread.fpu);
>>
>> +#endif
>> + this_cpu_write(in_kernel_fpu, false);
>> +
>> + preempt_enable();
>> +}
>> +EXPORT_SYMBOL_GPL(kernel_fpu_end);
>> +
> According to https://www.felixcloutier.com/x86/fxsave, the fxsave and fxrstor
> instructions do not save ymm, zmm, or mask registers. Please make that explicit
> in your comment, or replace intrinsics with inline assembler to support those
> registers.
>
> Several x86 crypto routines use these larger registers which could overwrite
> userspace registers. These modules aren't built by default, and may not be
> available on UML. Even so, this issue is worth a comment, at the least.
>
> On my system, lscpu within uml shows all the feature flags that are available
> on the host system, which includes AVX extentions.

That should be xsave, not fxsave upgrading to xsaveopt if available.

>
> Cheers,
> Peter Lafreniere
>
>
-- 
Anton R. Ivanov
Cambridgegreys Limited. Registered in England. Company Number 10273661
https://www.cambridgegreys.com/