| Message ID | 20230921155522.283582-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] um: Enable preemption in UML | expand |
On 21/09/2023 16:55, anton.ivanov@cambridgegreys.com wrote: > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > Preemption requires saving/restoring FPU state. This patch > adds support for it using GCC intrinsics. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > --- > arch/um/Kconfig | 1 - > arch/um/include/asm/fpu/api.h | 9 ++- > arch/um/include/asm/processor-generic.h | 3 + > arch/um/kernel/Makefile | 4 ++ > arch/um/kernel/fpu.c | 77 +++++++++++++++++++++++++ > arch/um/kernel/irq.c | 2 + > 6 files changed, 92 insertions(+), 4 deletions(-) > create mode 100644 arch/um/kernel/fpu.c Cleanup of typos and stale comments from older versions. > > 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/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h > index 71bfd9ef3938..9e7680bf48f0 100644 > --- a/arch/um/include/asm/fpu/api.h > +++ b/arch/um/include/asm/fpu/api.h > @@ -4,12 +4,15 @@ > > /* Copyright (c) 2020 Cambridge Greys Ltd > * Copyright (c) 2020 Red Hat Inc. > - * A set of "dummy" defines to allow the direct inclusion > - * of x86 optimized copy, xor, etc routines into the > - * UML code tree. */ > + */ > > +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) > +extern void kernel_fpu_begin(void); > +extern void kernel_fpu_end(void); > +#else > #define kernel_fpu_begin() (void)0 > #define kernel_fpu_end() (void)0 > +#endif > > 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..c08e8c5b0249 100644 > --- a/arch/um/include/asm/processor-generic.h > +++ b/arch/um/include/asm/processor-generic.h > @@ -44,6 +44,9 @@ struct thread_struct { > } cb; > } u; > } request; > +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) > + u8 fpu[2048] __aligned(64); /* Intel docs require xsave/xrestore area to be aligned to 64 bytes */ > +#endif > }; > > #define INIT_THREAD \ > diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile > index 811188be954c..c616e884a488 100644 > --- a/arch/um/kernel/Makefile > +++ b/arch/um/kernel/Makefile > @@ -26,9 +26,13 @@ obj-$(CONFIG_OF) += dtb.o > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > obj-$(CONFIG_STACKTRACE) += stacktrace.o > obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o > +obj-$(CONFIG_PREEMPT) += fpu.o > +obj-$(CONFIG_PREEMPT_VOLUNTARY) += fpu.o > > USER_OBJS := config.o > > +CFLAGS_fpu.o += -mxsave -mxsaveopt > + > include $(srctree)/arch/um/scripts/Makefile.rules > > targets := config.c config.tmp capflags.c > diff --git a/arch/um/kernel/fpu.c b/arch/um/kernel/fpu.c > new file mode 100644 > index 000000000000..346c07236185 > --- /dev/null > +++ b/arch/um/kernel/fpu.c > @@ -0,0 +1,77 @@ > +// 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> > +#include <asm/cpufeature.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); > + > +/* UML and driver code it pulls out of the x86 tree knows about 387 features > + * up to and including AVX512. TILE, etc are not yet supported. > + */ > + > +#define KNOWN_387_FEATURES 0xFF > + > + > +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 > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) > + __builtin_ia32_xsaveopt64(¤t->thread.fpu, KNOWN_387_FEATURES); > + else { > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xsave64(¤t->thread.fpu, KNOWN_387_FEATURES); > + else > + __builtin_ia32_fxsave64(¤t->thread.fpu); > + } > +#else > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) > + __builtin_ia32_xsaveopt(¤t->thread.fpu, KNOWN_387_FEATURES); > + else { > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xsave(¤t->thread.fpu, KNOWN_387_FEATURES); > + else > + __builtin_ia32_fxsave(¤t->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 > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xrstor64(¤t->thread.fpu, KNOWN_387_FEATURES); > + else > + __builtin_ia32_fxrstor64(¤t->thread.fpu); > +#else > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xrstor(¤t->thread.fpu, KNOWN_387_FEATURES); > + else > + __builtin_ia32_fxrstor(¤t->thread.fpu); > +#endif > + this_cpu_write(in_kernel_fpu, false); > + > + preempt_enable(); > +} > +EXPORT_SYMBOL_GPL(kernel_fpu_end); > + > diff --git a/arch/um/kernel/irq.c b/arch/um/kernel/irq.c > index 635d44606bfe..c02525da45df 100644 > --- a/arch/um/kernel/irq.c > +++ b/arch/um/kernel/irq.c > @@ -195,7 +195,9 @@ static void _sigio_handler(struct uml_pt_regs *regs, > > void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) > { > + preempt_disable(); > _sigio_handler(regs, irqs_suspended); > + preempt_enable(); > } > > static struct irq_entry *get_irq_entry_by_fd(int fd)
On Thu, Sep 21, 2023 at 11:55, anton.ivanov@cambridgegreys.com wrote: > > From: Anton Ivanov anton.ivanov@cambridgegreys.com > > > Preemption requires saving/restoring FPU state. This patch > adds support for it using GCC intrinsics. > > Signed-off-by: Anton Ivanov anton.ivanov@cambridgegreys.com > > --- > arch/um/Kconfig | 1 - > arch/um/include/asm/fpu/api.h | 9 ++- > arch/um/include/asm/processor-generic.h | 3 + > arch/um/kernel/Makefile | 4 ++ > arch/um/kernel/fpu.c | 77 +++++++++++++++++++++++++ > arch/um/kernel/irq.c | 2 + > 6 files changed, 92 insertions(+), 4 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/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h > index 71bfd9ef3938..9e7680bf48f0 100644 > --- a/arch/um/include/asm/fpu/api.h > +++ b/arch/um/include/asm/fpu/api.h > @@ -4,12 +4,15 @@ > > /* Copyright (c) 2020 Cambridge Greys Ltd > * Copyright (c) 2020 Red Hat Inc. > - * A set of "dummy" defines to allow the direct inclusion > - * of x86 optimized copy, xor, etc routines into the > - * UML code tree. */ > + / > > +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) > +extern void kernel_fpu_begin(void); > +extern void kernel_fpu_end(void); > +#else > #define kernel_fpu_begin() (void)0 > #define kernel_fpu_end() (void)0 > +#endif > > 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..c08e8c5b0249 100644 > --- a/arch/um/include/asm/processor-generic.h > +++ b/arch/um/include/asm/processor-generic.h > @@ -44,6 +44,9 @@ struct thread_struct { > } cb; > } u; > } request; > +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) > + u8 fpu[2048] __aligned(64); / Intel docs require xsave/xrestore area to be aligned to 64 bytes / > +#endif > }; > > #define INIT_THREAD \ > diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile > index 811188be954c..c616e884a488 100644 > --- a/arch/um/kernel/Makefile > +++ b/arch/um/kernel/Makefile > @@ -26,9 +26,13 @@ obj-$(CONFIG_OF) += dtb.o > obj-$(CONFIG_EARLY_PRINTK) += early_printk.o > obj-$(CONFIG_STACKTRACE) += stacktrace.o > obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o > +obj-$(CONFIG_PREEMPT) += fpu.o > +obj-$(CONFIG_PREEMPT_VOLUNTARY) += fpu.o > > USER_OBJS := config.o > > +CFLAGS_fpu.o += -mxsave -mxsaveopt > + > include $(srctree)/arch/um/scripts/Makefile.rules > > targets := config.c config.tmp capflags.c > diff --git a/arch/um/kernel/fpu.c b/arch/um/kernel/fpu.c > new file mode 100644 > index 000000000000..346c07236185 > --- /dev/null > +++ b/arch/um/kernel/fpu.c > @@ -0,0 +1,77 @@ > +// 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> > > +#include <asm/cpufeature.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); > + > +/ UML and driver code it pulls out of the x86 tree knows about 387 features > + * up to and including AVX512. TILE, etc are not yet supported. > + */ Tiny nit, but you use two different styles of block comments here. You did the same with the copyright declarations. > + > +#define KNOWN_387_FEATURES 0xFF > + > + > +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 > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) > + __builtin_ia32_xsaveopt64(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else { > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xsave64(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else > + __builtin_ia32_fxsave64(¤t->thread.fpu); > > + } > +#else > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) > + __builtin_ia32_xsaveopt(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else { > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xsave(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else > + __builtin_ia32_fxsave(¤t->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 > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xrstor64(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else > + __builtin_ia32_fxrstor64(¤t->thread.fpu); > > +#else > + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) > + __builtin_ia32_xrstor(¤t->thread.fpu, KNOWN_387_FEATURES); > > + else > + __builtin_ia32_fxrstor(¤t->thread.fpu); > > +#endif > + this_cpu_write(in_kernel_fpu, false); > + > + preempt_enable(); > +} > +EXPORT_SYMBOL_GPL(kernel_fpu_end); > + > diff --git a/arch/um/kernel/irq.c b/arch/um/kernel/irq.c > index 635d44606bfe..c02525da45df 100644 > --- a/arch/um/kernel/irq.c > +++ b/arch/um/kernel/irq.c > @@ -195,7 +195,9 @@ static void _sigio_handler(struct uml_pt_regs *regs, > > void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) > { > + preempt_disable(); > _sigio_handler(regs, irqs_suspended); > + preempt_enable(); > } > > static struct irq_entry *get_irq_entry_by_fd(int fd) > -- > 2.30.2 I'm happy with this as it is. The issue with AVX registers being clobbered when xsave isn't supported is unlikely to matter in practice, and can be fixed with a patch afterwards. Reviewed-by: Peter Lafreniere <peter@n8pjl.ca> Cheers, Peter
----- Ursprüngliche Mail ----- > Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> > An: "linux-um" <linux-um@lists.infradead.org> > CC: "Johannes Berg" <johannes@sipsolutions.net>, "richard" <richard@nod.at>, "anton ivanov" > <anton.ivanov@cambridgegreys.com> > Gesendet: Donnerstag, 21. September 2023 17:55:22 > Betreff: [PATCH v4] um: Enable preemption in UML > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > Preemption requires saving/restoring FPU state. This patch > adds support for it using GCC intrinsics. This patch triggers here the following splat: ------------[ cut here ]------------ WARNING: CPU: 0 PID: 0 at init/main.c:992 start_kernel+0x541/0x6b6 Interrupts were enabled early Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc2-ga74d8f8592e5 #128 Stack: 60381b94 00000001 604d3d80 6002b6f6 60444bdd 00000001 604d3e20 6037ecfa 604d3db0 60389b2f 604d3da0 60381b94 Call Trace: [<60381b94>] ? _printk+0x0/0x94 [<600212b1>] show_stack+0x13c/0x14b [<60381b94>] ? _printk+0x0/0x94 [<6002b6f6>] ? um_set_signals+0x0/0x3f [<6037ecfa>] ? sprintf+0x0/0x95 [<60389b2f>] dump_stack_lvl+0x4d/0x5a [<60381b94>] ? _printk+0x0/0x94 [<60389b56>] dump_stack+0x1a/0x1c [<60033ebc>] __warn+0xd9/0x109 [<60033fbd>] warn_slowpath_fmt+0xd1/0xdf [<60033eec>] ? warn_slowpath_fmt+0x0/0xdf [<60381b94>] ? _printk+0x0/0x94 [<60050e2c>] ? __blocking_notifier_chain_register+0x33/0x78 [<60381b94>] ? _printk+0x0/0x94 [<60050e83>] ? blocking_notifier_chain_register+0x12/0x14 [<6005fa92>] ? register_pm_notifier+0x1d/0x1f [<600152c9>] ? random_init+0xc2/0x126 [<60381b94>] ? _printk+0x0/0x94 [<60001c40>] start_kernel+0x541/0x6b6 [<6004deb0>] ? parse_args+0x0/0x2b9 [<6002b609>] ? block_signals+0x0/0x16 [<60003b9d>] start_kernel_proc+0x49/0x4d [<60061863>] ? kmsg_dump_register+0x6d/0x75 [<6001ffdb>] new_thread_handler+0x81/0xb2 [<60003b52>] ? kmsg_dumper_stdout_init+0x1a/0x1c [<60022e52>] uml_finishsetup+0x54/0x59 ---[ end trace 0000000000000000 ]--- x86_64 defconfig + CONFIG_PREEMPT_VOLUNTARY=y Thanks, //richard
On 21/09/2023 22:02, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- >> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> >> An: "linux-um" <linux-um@lists.infradead.org> >> CC: "Johannes Berg" <johannes@sipsolutions.net>, "richard" <richard@nod.at>, "anton ivanov" >> <anton.ivanov@cambridgegreys.com> >> Gesendet: Donnerstag, 21. September 2023 17:55:22 >> Betreff: [PATCH v4] um: Enable preemption in UML >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> Preemption requires saving/restoring FPU state. This patch >> adds support for it using GCC intrinsics. > This patch triggers here the following splat: Ack. I will look into it tomorrow. There will be places where it will bomb out. F.E. I am chasing one is triggered by the NFS server (it may be the same as in this trace - it happens around the end of the initial setup). All in all it looks better than I expected. defconfig + vector + nfs client + CONFIG_PREEMPT - I had the kernel build without any issues from a source tree on NFS and with reasonable performance too. > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 0 at init/main.c:992 start_kernel+0x541/0x6b6 > Interrupts were enabled early > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 6.6.0-rc2-ga74d8f8592e5 #128 > Stack: > 60381b94 00000001 604d3d80 6002b6f6 > 60444bdd 00000001 604d3e20 6037ecfa > 604d3db0 60389b2f 604d3da0 60381b94 > Call Trace: > [<60381b94>] ? _printk+0x0/0x94 > [<600212b1>] show_stack+0x13c/0x14b > [<60381b94>] ? _printk+0x0/0x94 > [<6002b6f6>] ? um_set_signals+0x0/0x3f > [<6037ecfa>] ? sprintf+0x0/0x95 > [<60389b2f>] dump_stack_lvl+0x4d/0x5a > [<60381b94>] ? _printk+0x0/0x94 > [<60389b56>] dump_stack+0x1a/0x1c > [<60033ebc>] __warn+0xd9/0x109 > [<60033fbd>] warn_slowpath_fmt+0xd1/0xdf > [<60033eec>] ? warn_slowpath_fmt+0x0/0xdf > [<60381b94>] ? _printk+0x0/0x94 > [<60050e2c>] ? __blocking_notifier_chain_register+0x33/0x78 > [<60381b94>] ? _printk+0x0/0x94 > [<60050e83>] ? blocking_notifier_chain_register+0x12/0x14 > [<6005fa92>] ? register_pm_notifier+0x1d/0x1f > [<600152c9>] ? random_init+0xc2/0x126 > [<60381b94>] ? _printk+0x0/0x94 > [<60001c40>] start_kernel+0x541/0x6b6 > [<6004deb0>] ? parse_args+0x0/0x2b9 > [<6002b609>] ? block_signals+0x0/0x16 > [<60003b9d>] start_kernel_proc+0x49/0x4d > [<60061863>] ? kmsg_dump_register+0x6d/0x75 > [<6001ffdb>] new_thread_handler+0x81/0xb2 > [<60003b52>] ? kmsg_dumper_stdout_init+0x1a/0x1c > [<60022e52>] uml_finishsetup+0x54/0x59 > ---[ end trace 0000000000000000 ]--- > > x86_64 defconfig + CONFIG_PREEMPT_VOLUNTARY=y > > Thanks, > //richard >
----- Ursprüngliche Mail ----- > Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> > An: "richard" <richard@nod.at> > CC: "linux-um" <linux-um@lists.infradead.org>, "Johannes Berg" <johannes@sipsolutions.net> > Gesendet: Donnerstag, 21. September 2023 23:41:16 > Betreff: Re: [PATCH v4] um: Enable preemption in UML > On 21/09/2023 22:02, Richard Weinberger wrote: >> ----- Ursprüngliche Mail ----- >>> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> >>> An: "linux-um" <linux-um@lists.infradead.org> >>> CC: "Johannes Berg" <johannes@sipsolutions.net>, "richard" <richard@nod.at>, >>> "anton ivanov" >>> <anton.ivanov@cambridgegreys.com> >>> Gesendet: Donnerstag, 21. September 2023 17:55:22 >>> Betreff: [PATCH v4] um: Enable preemption in UML >>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> >>> Preemption requires saving/restoring FPU state. This patch >>> adds support for it using GCC intrinsics. >> This patch triggers here the following splat: > > Ack. I will look into it tomorrow. Thx! I had a brief look, UML's init_IRQ() seems to return now with interrupts enabled. Dunno why, I need some sleep now. > > There will be places where it will bomb out. F.E. I am chasing one is > triggered by the NFS server (it may be the same as in this trace - it > happens around the end of the initial setup). > > All in all it looks better than I expected. True that! :-) Thanks, //richard
On 21/09/2023 22:55, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- >> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> >> An: "richard" <richard@nod.at> >> CC: "linux-um" <linux-um@lists.infradead.org>, "Johannes Berg" <johannes@sipsolutions.net> >> Gesendet: Donnerstag, 21. September 2023 23:41:16 >> Betreff: Re: [PATCH v4] um: Enable preemption in UML >> On 21/09/2023 22:02, Richard Weinberger wrote: >>> ----- Ursprüngliche Mail ----- >>>> Von: "anton ivanov" <anton.ivanov@cambridgegreys.com> >>>> An: "linux-um" <linux-um@lists.infradead.org> >>>> CC: "Johannes Berg" <johannes@sipsolutions.net>, "richard" <richard@nod.at>, >>>> "anton ivanov" >>>> <anton.ivanov@cambridgegreys.com> >>>> Gesendet: Donnerstag, 21. September 2023 17:55:22 >>>> Betreff: [PATCH v4] um: Enable preemption in UML >>>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>>> >>>> Preemption requires saving/restoring FPU state. This patch >>>> adds support for it using GCC intrinsics. >>> This patch triggers here the following splat: >> Ack. I will look into it tomorrow. > Thx! > I had a brief look, UML's init_IRQ() seems to return now with interrupts enabled. > Dunno why, I need some sleep now. Tlb. Looking at other architectures (and specifically x86) it needs preempt_disable/enable around critical sections. From there you may end up in the wrong state across the board. > >> There will be places where it will bomb out. F.E. I am chasing one is >> triggered by the NFS server (it may be the same as in this trace - it >> happens around the end of the initial setup). >> >> All in all it looks better than I expected. > True that! :-) > > Thanks, > //richard > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
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/include/asm/fpu/api.h b/arch/um/include/asm/fpu/api.h index 71bfd9ef3938..9e7680bf48f0 100644 --- a/arch/um/include/asm/fpu/api.h +++ b/arch/um/include/asm/fpu/api.h @@ -4,12 +4,15 @@ /* Copyright (c) 2020 Cambridge Greys Ltd * Copyright (c) 2020 Red Hat Inc. - * A set of "dummy" defines to allow the direct inclusion - * of x86 optimized copy, xor, etc routines into the - * UML code tree. */ + */ +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) +extern void kernel_fpu_begin(void); +extern void kernel_fpu_end(void); +#else #define kernel_fpu_begin() (void)0 #define kernel_fpu_end() (void)0 +#endif 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..c08e8c5b0249 100644 --- a/arch/um/include/asm/processor-generic.h +++ b/arch/um/include/asm/processor-generic.h @@ -44,6 +44,9 @@ struct thread_struct { } cb; } u; } request; +#if defined(CONFIG_PREEMPT) || defined(CONFIG_PREEMPT_VOLUNTARY) + u8 fpu[2048] __aligned(64); /* Intel docs require xsave/xrestore area to be aligned to 64 bytes */ +#endif }; #define INIT_THREAD \ diff --git a/arch/um/kernel/Makefile b/arch/um/kernel/Makefile index 811188be954c..c616e884a488 100644 --- a/arch/um/kernel/Makefile +++ b/arch/um/kernel/Makefile @@ -26,9 +26,13 @@ obj-$(CONFIG_OF) += dtb.o obj-$(CONFIG_EARLY_PRINTK) += early_printk.o obj-$(CONFIG_STACKTRACE) += stacktrace.o obj-$(CONFIG_GENERIC_PCI_IOMAP) += ioport.o +obj-$(CONFIG_PREEMPT) += fpu.o +obj-$(CONFIG_PREEMPT_VOLUNTARY) += fpu.o USER_OBJS := config.o +CFLAGS_fpu.o += -mxsave -mxsaveopt + include $(srctree)/arch/um/scripts/Makefile.rules targets := config.c config.tmp capflags.c diff --git a/arch/um/kernel/fpu.c b/arch/um/kernel/fpu.c new file mode 100644 index 000000000000..346c07236185 --- /dev/null +++ b/arch/um/kernel/fpu.c @@ -0,0 +1,77 @@ +// 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> +#include <asm/cpufeature.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); + +/* UML and driver code it pulls out of the x86 tree knows about 387 features + * up to and including AVX512. TILE, etc are not yet supported. + */ + +#define KNOWN_387_FEATURES 0xFF + + +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 + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) + __builtin_ia32_xsaveopt64(¤t->thread.fpu, KNOWN_387_FEATURES); + else { + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) + __builtin_ia32_xsave64(¤t->thread.fpu, KNOWN_387_FEATURES); + else + __builtin_ia32_fxsave64(¤t->thread.fpu); + } +#else + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVEOPT))) + __builtin_ia32_xsaveopt(¤t->thread.fpu, KNOWN_387_FEATURES); + else { + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) + __builtin_ia32_xsave(¤t->thread.fpu, KNOWN_387_FEATURES); + else + __builtin_ia32_fxsave(¤t->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 + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) + __builtin_ia32_xrstor64(¤t->thread.fpu, KNOWN_387_FEATURES); + else + __builtin_ia32_fxrstor64(¤t->thread.fpu); +#else + if (likely(cpu_has(&boot_cpu_data, X86_FEATURE_XSAVE))) + __builtin_ia32_xrstor(¤t->thread.fpu, KNOWN_387_FEATURES); + else + __builtin_ia32_fxrstor(¤t->thread.fpu); +#endif + this_cpu_write(in_kernel_fpu, false); + + preempt_enable(); +} +EXPORT_SYMBOL_GPL(kernel_fpu_end); + diff --git a/arch/um/kernel/irq.c b/arch/um/kernel/irq.c index 635d44606bfe..c02525da45df 100644 --- a/arch/um/kernel/irq.c +++ b/arch/um/kernel/irq.c @@ -195,7 +195,9 @@ static void _sigio_handler(struct uml_pt_regs *regs, void sigio_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs) { + preempt_disable(); _sigio_handler(regs, irqs_suspended); + preempt_enable(); } static struct irq_entry *get_irq_entry_by_fd(int fd)