[PATCH] LoongArch: Provide kernel fpu functions

[PATCH] LoongArch: Provide kernel fpu functions

[Huacai Chen]

Provide kernel_fpu_begin()/kernel_fpu_end() to let the kernel use fpu
itself. They can be used by AMDGPU graphic driver for DCN.

Reported-by: Xuerui Wang <kernel@xen0n.name>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 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_GPL(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_GPL(kernel_fpu_end);
-- 
2.39.1

Re: [PATCH] LoongArch: Provide kernel fpu functions

[WANG Xuerui]

On 3/5/23 13:28, Huacai Chen wrote:
> Provide kernel_fpu_begin()/kernel_fpu_end() to let the kernel use fpu
> itself. They can be used by AMDGPU graphic driver for DCN.

Grammar nit: "itself" is wrongly placed. "allow the kernel itself to use 
FPU" could be better.

Also the expected usage is way broader than a single driver's single 
component. It's useful for a wide array of operations that will benefit 
from SIMD acceleration support that'll hopefully appear later. For now 
I'd suggest at least adding a single "e.g." after "used by" to signify 
this, if you're not rewording the sentence.

>
> Reported-by: Xuerui Wang <kernel@xen0n.name>
Thanks, but I prefer my name spelled in the native word order ;-)
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>   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;
Could be a conditional WARN_ON_ONCE like in arch/x86?
> +
> +	preempt_disable();
> +	this_cpu_write(in_kernel_fpu, true);
> +
> +	if (!is_fpu_owner())
> +		enable_fpu();
> +	else
> +		_save_fp(&current->thread.fpu);
> +}
> +EXPORT_SYMBOL_GPL(kernel_fpu_begin);

Might be good to provide some explanation in the commit message as to 
why the pair of helpers should be GPL-only. Do they touch state buried 
deep enough to make any downstream user a "derivative work"? Or are the 
annotation inspired by arch/x86?

I think this kinda needs more thought, because similar operations like 
arm's kernel_neon_{begin,end}, powerpc's enable_kernel_{fp,vsx,altivec} 
or s390's __kernel_fpu_{begin,end} are not made GPL-only. Making these 
helpers GPL-only precludes any non-GPL module to make use of SIMD on 
LoongArch, which may or may not be what you want. This can have 
commercial consequences so I can only leave the decision to you. 
(Although IMO the semantics are encapsulated and high-level enough to 
not warrant GPL-only marks, but it may well be the case that you have 
thought of something else but didn't mention here.)

> +
> +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_GPL(kernel_fpu_end);

-- 
WANG "xen0n" Xuerui

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

Re: [PATCH] LoongArch: Provide kernel fpu functions

[Huacai Chen]

Hi, Xuerui,

On Sun, Mar 5, 2023 at 1:53 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> On 3/5/23 13:28, Huacai Chen wrote:
> > Provide kernel_fpu_begin()/kernel_fpu_end() to let the kernel use fpu
> > itself. They can be used by AMDGPU graphic driver for DCN.
>
> Grammar nit: "itself" is wrongly placed. "allow the kernel itself to use
> FPU" could be better.
>
> Also the expected usage is way broader than a single driver's single
> component. It's useful for a wide array of operations that will benefit
> from SIMD acceleration support that'll hopefully appear later. For now
> I'd suggest at least adding a single "e.g." after "used by" to signify
> this, if you're not rewording the sentence.
OK, I will update it.

>
> >
> > Reported-by: Xuerui Wang <kernel@xen0n.name>
> Thanks, but I prefer my name spelled in the native word order ;-)
OK, I will correct it.

> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >   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;
> Could be a conditional WARN_ON_ONCE like in arch/x86?
> > +
> > +     preempt_disable();
> > +     this_cpu_write(in_kernel_fpu, true);
> > +
> > +     if (!is_fpu_owner())
> > +             enable_fpu();
> > +     else
> > +             _save_fp(&current->thread.fpu);
> > +}
> > +EXPORT_SYMBOL_GPL(kernel_fpu_begin);
>
> Might be good to provide some explanation in the commit message as to
> why the pair of helpers should be GPL-only. Do they touch state buried
> deep enough to make any downstream user a "derivative work"? Or are the
> annotation inspired by arch/x86?
Yes, just inspired by arch/x86, and I don't think these symbols should
be used by non-GPL modules.

Huacai
>
> I think this kinda needs more thought, because similar operations like
> arm's kernel_neon_{begin,end}, powerpc's enable_kernel_{fp,vsx,altivec}
> or s390's __kernel_fpu_{begin,end} are not made GPL-only. Making these
> helpers GPL-only precludes any non-GPL module to make use of SIMD on
> LoongArch, which may or may not be what you want. This can have
> commercial consequences so I can only leave the decision to you.
> (Although IMO the semantics are encapsulated and high-level enough to
> not warrant GPL-only marks, but it may well be the case that you have
> thought of something else but didn't mention here.)
>
> > +
> > +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_GPL(kernel_fpu_end);
>
> --
> WANG "xen0n" Xuerui
>
> Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
>

Re: [PATCH] LoongArch: Provide kernel fpu functions

[Xi Ruoyao]

On Sun, 2023-03-05 at 20:18 +0800, Huacai Chen wrote:
> > Might be good to provide some explanation in the commit message as to
> > why the pair of helpers should be GPL-only. Do they touch state buried
> > deep enough to make any downstream user a "derivative work"? Or are the
> > annotation inspired by arch/x86?
> Yes, just inspired by arch/x86, and I don't think these symbols should
> be used by non-GPL modules.

Hmm, what if one of your partners wish to provide a proprietary GPU
driver using the FPU like this way?  As a FLOSS developer I'd say "don't
do that, make your driver GPL".  But for Loongson there may be a
commercial issue.
-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH] LoongArch: Provide kernel fpu functions

[Huacai Chen]

On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Sun, 2023-03-05 at 20:18 +0800, Huacai Chen wrote:
> > > Might be good to provide some explanation in the commit message as to
> > > why the pair of helpers should be GPL-only. Do they touch state buried
> > > deep enough to make any downstream user a "derivative work"? Or are the
> > > annotation inspired by arch/x86?
> > Yes, just inspired by arch/x86, and I don't think these symbols should
> > be used by non-GPL modules.
>
> Hmm, what if one of your partners wish to provide a proprietary GPU
> driver using the FPU like this way?  As a FLOSS developer I'd say "don't
> do that, make your driver GPL".  But for Loongson there may be a
> commercial issue.
So use EXPORT_SYMBOL can make life easier?

Huacai
> --
> Xi Ruoyao <xry111@xry111.site>
> School of Aerospace Science and Technology, Xidian University

Re: [PATCH] LoongArch: Provide kernel fpu functions

[WANG Xuerui]

On 2023/3/6 09:55, Huacai Chen wrote:
> On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <xry111@xry111.site> wrote:
>>
>> On Sun, 2023-03-05 at 20:18 +0800, Huacai Chen wrote:
>>>> Might be good to provide some explanation in the commit message as to
>>>> why the pair of helpers should be GPL-only. Do they touch state buried
>>>> deep enough to make any downstream user a "derivative work"? Or are the
>>>> annotation inspired by arch/x86?
>>> Yes, just inspired by arch/x86, and I don't think these symbols should
>>> be used by non-GPL modules.
>>
>> Hmm, what if one of your partners wish to provide a proprietary GPU
>> driver using the FPU like this way?  As a FLOSS developer I'd say "don't
>> do that, make your driver GPL".  But for Loongson there may be a
>> commercial issue.
> So use EXPORT_SYMBOL can make life easier?

As I've detailed in my first reply, every arch other than x86 exposes 
this functionality without the GPL-only restriction. Although IMO 
non-GPL driver developers wouldn't grieve over this particular feature 
simply because pretty much everyone would have to build on x86 and that 
arch wouldn't have it exposed, I do think it will be more demanded on 
loongarch, where HW performance is in general lower than x86/arm64 
offerings at similar cost levels, so perhaps people would have to resort 
to FP/SIMD tricks to reach performance on par with others.

Also, if the old world is taken into consideration (which we normally 
have the luxury of not having to do so), consider Ruoyao's case where a 
commercial partner of Loongson wants to do this with the vendor kernel, 
but the symbols are exported GPL -- in this case I doubt the GPL marking 
will remain, thus creating inconsistency between upstream and vendor 
kernels, and community distros are going to complain loudly about the 
need to patch things. It's probably best to avoid all of this upfront.

Note that this is all suggestion though, it's down to you and your team 
to decide after all. And I've not done the archaeology about the other 
arches' choice yet, which may or may not reveal more reasoning behind 
their status quo.

-- 
WANG "xen0n" Xuerui

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

RE: [PATCH] LoongArch: Provide kernel fpu functions

[David Laight]

...
> Also, if the old world is taken into consideration (which we normally
> have the luxury of not having to do so), consider Ruoyao's case where a
> commercial partner of Loongson wants to do this with the vendor kernel,
> but the symbols are exported GPL -- in this case I doubt the GPL marking
> will remain, thus creating inconsistency between upstream and vendor
> kernels, and community distros are going to complain loudly about the
> need to patch things. It's probably best to avoid all of this upfront.

It is pretty easy to load a non-GPL module into a distro-built
kernel and call GPL-only functions.
(And without doing horrid things with kallsyms.)
As soon as you actually need to do one, adding others isn't a problem.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] LoongArch: Provide kernel fpu functions

[WANG Xuerui]

On 2023/3/6 20:53, David Laight wrote:
> ...
>> Also, if the old world is taken into consideration (which we normally
>> have the luxury of not having to do so), consider Ruoyao's case where a
>> commercial partner of Loongson wants to do this with the vendor kernel,
>> but the symbols are exported GPL -- in this case I doubt the GPL marking
>> will remain, thus creating inconsistency between upstream and vendor
>> kernels, and community distros are going to complain loudly about the
>> need to patch things. It's probably best to avoid all of this upfront.
> 
> It is pretty easy to load a non-GPL module into a distro-built
> kernel and call GPL-only functions.
> (And without doing horrid things with kallsyms.)
> As soon as you actually need to do one, adding others isn't a problem.

Hmm, do you mean patching the kernel downstream to remove the license 
checks, or something like that? I remember the so-called "GPL condom" 
trick was banned some time earlier, in commit 262e6ae7081df ("modules: 
inherit TAINT_PROPRIETARY_MODULE"). For now I can't think of a way that 
would allow such reference...

-- 
WANG "xen0n" Xuerui

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

Re: [PATCH] LoongArch: Provide kernel fpu functions

[Christoph Hellwig]

On Mon, Mar 06, 2023 at 09:55:27AM +0800, Huacai Chen wrote:
> On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <xry111@xry111.site> wrote:
> >
> > On Sun, 2023-03-05 at 20:18 +0800, Huacai Chen wrote:
> > > > Might be good to provide some explanation in the commit message as to
> > > > why the pair of helpers should be GPL-only. Do they touch state buried
> > > > deep enough to make any downstream user a "derivative work"? Or are the
> > > > annotation inspired by arch/x86?
> > > Yes, just inspired by arch/x86, and I don't think these symbols should
> > > be used by non-GPL modules.
> >
> > Hmm, what if one of your partners wish to provide a proprietary GPU
> > driver using the FPU like this way?  As a FLOSS developer I'd say "don't
> > do that, make your driver GPL".  But for Loongson there may be a
> > commercial issue.
> So use EXPORT_SYMBOL can make life easier?

No.  All in-kernel FPU helpers must be EXPORT_SYMBOL_GPL.