[V2] LoongArch: Provide kernel fpu functions
Commit Message
Provide kernel_fpu_begin()/kernel_fpu_end() to allow the kernel itself
to use fpu. They can be used by some other kernel components, e.g., the
AMDGPU graphic driver for DCN.
Reported-by: WANG Xuerui<kernel@xen0n.name>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
V2: Use non-GPL exports and update commit messages.
arch/loongarch/include/asm/fpu.h | 3 +++
arch/loongarch/kernel/Makefile | 2 +-
arch/loongarch/kernel/kfpu.c | 41 ++++++++++++++++++++++++++++++++
3 files changed, 45 insertions(+), 1 deletion(-)
create mode 100644 arch/loongarch/kernel/kfpu.c
Comments
On 2023/3/6 11:12, Huacai Chen wrote:
> Provide kernel_fpu_begin()/kernel_fpu_end() to allow the kernel itself
> to use fpu. They can be used by some other kernel components, e.g., the
> AMDGPU graphic driver for DCN.
IMO my previous explanation works better, but the current wording kinda
works for me. </grumble>
>
> Reported-by: WANG Xuerui<kernel@xen0n.name>
One space before the opening angle bracket.
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
> V2: Use non-GPL exports and update commit messages.
>
> arch/loongarch/include/asm/fpu.h | 3 +++
> arch/loongarch/kernel/Makefile | 2 +-
> arch/loongarch/kernel/kfpu.c | 41 ++++++++++++++++++++++++++++++++
> 3 files changed, 45 insertions(+), 1 deletion(-)
> create mode 100644 arch/loongarch/kernel/kfpu.c
>
> diff --git a/arch/loongarch/include/asm/fpu.h b/arch/loongarch/include/asm/fpu.h
> index 358b254d9c1d..192f8e35d912 100644
> --- a/arch/loongarch/include/asm/fpu.h
> +++ b/arch/loongarch/include/asm/fpu.h
> @@ -21,6 +21,9 @@
>
> struct sigcontext;
>
> +extern void kernel_fpu_begin(void);
> +extern void kernel_fpu_end(void);
> +
> extern void _init_fpu(unsigned int);
> extern void _save_fp(struct loongarch_fpu *);
> extern void _restore_fp(struct loongarch_fpu *);
> diff --git a/arch/loongarch/kernel/Makefile b/arch/loongarch/kernel/Makefile
> index 78d4e3384305..9a72d91cd104 100644
> --- a/arch/loongarch/kernel/Makefile
> +++ b/arch/loongarch/kernel/Makefile
> @@ -13,7 +13,7 @@ obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
> obj-$(CONFIG_ACPI) += acpi.o
> obj-$(CONFIG_EFI) += efi.o
>
> -obj-$(CONFIG_CPU_HAS_FPU) += fpu.o
> +obj-$(CONFIG_CPU_HAS_FPU) += fpu.o kfpu.o
>
> obj-$(CONFIG_ARCH_STRICT_ALIGN) += unaligned.o
>
> diff --git a/arch/loongarch/kernel/kfpu.c b/arch/loongarch/kernel/kfpu.c
> new file mode 100644
> index 000000000000..cd2a18fecdcc
> --- /dev/null
> +++ b/arch/loongarch/kernel/kfpu.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020-2023 Loongson Technology Corporation Limited
2020?
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/init.h>
> +#include <asm/fpu.h>
> +#include <asm/smp.h>
> +
> +static DEFINE_PER_CPU(bool, in_kernel_fpu);
> +
> +void kernel_fpu_begin(void)
> +{
> + if(this_cpu_read(in_kernel_fpu))
> + return;
You've ignored my WARN suggestion, but I could add it later anyway so
I'd allow this to go in as-is for now.
> +
> + preempt_disable();
> + this_cpu_write(in_kernel_fpu, true);
> +
> + if (!is_fpu_owner())
> + enable_fpu();
> + else
> + _save_fp(¤t->thread.fpu);
> +}
> +EXPORT_SYMBOL(kernel_fpu_begin);
> +
> +void kernel_fpu_end(void)
> +{
> + if(!this_cpu_read(in_kernel_fpu))
> + return;
> +
> + if (!is_fpu_owner())
> + disable_fpu();
> + else
> + _restore_fp(¤t->thread.fpu);
> +
> + this_cpu_write(in_kernel_fpu, false);
> + preempt_enable();
> +}
> +EXPORT_SYMBOL(kernel_fpu_end);
I've tested this along with the amdgpu DCN enablement, so:
Tested-by: WANG Xuerui <git@xen0n.name>
NAK, this needs to be an EXPORT_SYMBOL_GPL.
Also no way we're going to merge this without an actual user.
On Fri, Mar 10, 2023 at 12:45 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> NAK, this needs to be an EXPORT_SYMBOL_GPL.
OK, let's make it GPL again.
Huacai
>
> Also no way we're going to merge this without an actual user.
On 3/10/23 00:45, Christoph Hellwig wrote:
> NAK, this needs to be an EXPORT_SYMBOL_GPL.
>
> Also no way we're going to merge this without an actual user.
Okay so I sat down for some Saturday afternoon archaeology, and this is
indeed much hairier than I originally thought...
Basically the same conversation has happened back in 2019 [1][2][3],
mainly over the breakage it caused for zfs (obviously non-GPL and
out-of-tree, that apparently made people automatically ignore it), while
the introducing commit d63e79b114c02 ("x86/fpu: Uninline
kernel_fpu_begin()/end()") went in unnoticed at that time [4]. It's
clear that my opinion fell under the same camp as Andy and Paolo
("exporting as GPL" means "any usage of this symbol implies the module
is in fact derived work"), but the others disagreed ("in practice we
don't care if it has no in-kernel users, and even if it does it would
have to be GPL anyway", IIUC).
For now I've only been tinkering with Navi GPUs on loongarch, and
haven't got to try openzfs yet, but I surely don't want to start the
debate all over again, and making the loongarch FPU helpers GPL-only
works for me. However there's probably another question that Ruoyao
pointed out: do we want to mark the neon/altivec/s390x helpers GPL-only
right away? IMO this particular feature is not inherently arch-specific
(the same would have to happen for every arch with optionally enabled
extra state and instructions, not limited to FPU actually) so
availability of such feature should preferably be made symmetric over
arches.
Given the topic's sensitive nature I'd want to hear from more people
before deciding to (not) write the patches; thanks in advance.
[1]:
https://lore.kernel.org/all/761345df6285930339aced868ebf8ec459091383.1556807897.git.luto@kernel.org/
[2]: https://lore.kernel.org/all/20190522044204.24207-1-cyphar@cyphar.com/
[3]:
https://lore.kernel.org/all/CAB9dFdsZb-sZixeOzrt8F50h1pnUK2W2Cxx8+xjhgd0=6xs7iw@mail.gmail.com/T/#u
[4]:
https://lore.kernel.org/all/1430848300-27877-51-git-send-email-mingo@kernel.org/
On Sat, Mar 11, 2023 at 04:10:44PM +0800, WANG Xuerui wrote:
> again, and making the loongarch FPU helpers GPL-only works for me. However
> there's probably another question that Ruoyao pointed out: do we want to
> mark the neon/altivec/s390x helpers GPL-only right away? IMO this particular
> feature is not inherently arch-specific (the same would have to happen for
> every arch with optionally enabled extra state and instructions, not limited
> to FPU actually) so availability of such feature should preferably be made
> symmetric over arches.
In general we should, and all new code most. For existing code please
contact the relevant maintainers first.
@@ -21,6 +21,9 @@
struct sigcontext;
+extern void kernel_fpu_begin(void);
+extern void kernel_fpu_end(void);
+
extern void _init_fpu(unsigned int);
extern void _save_fp(struct loongarch_fpu *);
extern void _restore_fp(struct loongarch_fpu *);
@@ -13,7 +13,7 @@ obj-y += head.o cpu-probe.o cacheinfo.o env.o setup.o entry.o genex.o \
obj-$(CONFIG_ACPI) += acpi.o
obj-$(CONFIG_EFI) += efi.o
-obj-$(CONFIG_CPU_HAS_FPU) += fpu.o
+obj-$(CONFIG_CPU_HAS_FPU) += fpu.o kfpu.o
obj-$(CONFIG_ARCH_STRICT_ALIGN) += unaligned.o
new file mode 100644
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2020-2023 Loongson Technology Corporation Limited
+ */
+
+#include <linux/cpu.h>
+#include <linux/init.h>
+#include <asm/fpu.h>
+#include <asm/smp.h>
+
+static DEFINE_PER_CPU(bool, in_kernel_fpu);
+
+void kernel_fpu_begin(void)
+{
+ if(this_cpu_read(in_kernel_fpu))
+ return;
+
+ preempt_disable();
+ this_cpu_write(in_kernel_fpu, true);
+
+ if (!is_fpu_owner())
+ enable_fpu();
+ else
+ _save_fp(¤t->thread.fpu);
+}
+EXPORT_SYMBOL(kernel_fpu_begin);
+
+void kernel_fpu_end(void)
+{
+ if(!this_cpu_read(in_kernel_fpu))
+ return;
+
+ if (!is_fpu_owner())
+ disable_fpu();
+ else
+ _restore_fp(¤t->thread.fpu);
+
+ this_cpu_write(in_kernel_fpu, false);
+ preempt_enable();
+}
+EXPORT_SYMBOL(kernel_fpu_end);