[PATCH V2] LoongArch: Provide kernel fpu functions

[PATCH V2] LoongArch: Provide kernel fpu functions

[Huacai Chen]

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

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
+ */
+
+#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(&current->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(&current->thread.fpu);
+
+	this_cpu_write(in_kernel_fpu, false);
+	preempt_enable();
+}
+EXPORT_SYMBOL(kernel_fpu_end);
-- 
2.39.1

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[Randy Dunlap]

Hi,


On 3/5/23 19:12, Huacai Chen wrote:
> +void kernel_fpu_begin(void)
> +{
> +	if(this_cpu_read(in_kernel_fpu))

	if (
> +		return;
> +
> +	preempt_disable();
> +	this_cpu_write(in_kernel_fpu, true);
> +
> +	if (!is_fpu_owner())
> +		enable_fpu();
> +	else
> +		_save_fp(&current->thread.fpu);
> +}
> +EXPORT_SYMBOL(kernel_fpu_begin);
> +
> +void kernel_fpu_end(void)
> +{
> +	if(!this_cpu_read(in_kernel_fpu))

	if (

i.e., add a space after "if".

> +		return;
> +
> +	if (!is_fpu_owner())
> +		disable_fpu();
> +	else
> +		_restore_fp(&current->thread.fpu);
> +
> +	this_cpu_write(in_kernel_fpu, false);
> +	preempt_enable();
> +}
> +EXPORT_SYMBOL(kernel_fpu_end);

-- 
~Randy

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[WANG Xuerui]

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(&current->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(&current->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>

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[Huacai Chen]

On Mon, Mar 6, 2023 at 11:15 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi,
>
>
> On 3/5/23 19:12, Huacai Chen wrote:
> > +void kernel_fpu_begin(void)
> > +{
> > +     if(this_cpu_read(in_kernel_fpu))
>
>         if (
> > +             return;
> > +
> > +     preempt_disable();
> > +     this_cpu_write(in_kernel_fpu, true);
> > +
> > +     if (!is_fpu_owner())
> > +             enable_fpu();
> > +     else
> > +             _save_fp(&current->thread.fpu);
> > +}
> > +EXPORT_SYMBOL(kernel_fpu_begin);
> > +
> > +void kernel_fpu_end(void)
> > +{
> > +     if(!this_cpu_read(in_kernel_fpu))
>
>         if (
>
> i.e., add a space after "if".
OK, thanks.

Huacai
>
> > +             return;
> > +
> > +     if (!is_fpu_owner())
> > +             disable_fpu();
> > +     else
> > +             _restore_fp(&current->thread.fpu);
> > +
> > +     this_cpu_write(in_kernel_fpu, false);
> > +     preempt_enable();
> > +}
> > +EXPORT_SYMBOL(kernel_fpu_end);
>
> --
> ~Randy

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[Christoph Hellwig]

NAK, this needs to be an EXPORT_SYMBOL_GPL.

Also no way we're going to merge this without an actual user.

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[Huacai Chen]

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.

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[WANG Xuerui]

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/

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

Re: [PATCH V2] LoongArch: Provide kernel fpu functions

[Christoph Hellwig]

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.