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 <[email protected]>
Signed-off-by: Huacai Chen <[email protected]>
---
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(¤t->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(¤t->thread.fpu);
+
+ this_cpu_write(in_kernel_fpu, false);
+ preempt_enable();
+}
+EXPORT_SYMBOL_GPL(kernel_fpu_end);
--
2.39.1
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 <[email protected]>
Thanks, but I prefer my name spelled in the native word order ;-)
> Signed-off-by: Huacai Chen <[email protected]>
> ---
> 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(¤t->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(¤t->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/
Hi, Xuerui,
On Sun, Mar 5, 2023 at 1:53 PM WANG Xuerui <[email protected]> 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 <[email protected]>
> Thanks, but I prefer my name spelled in the native word order ;-)
OK, I will correct it.
> > Signed-off-by: Huacai Chen <[email protected]>
> > ---
> > 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(¤t->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(¤t->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/
>
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 <[email protected]>
School of Aerospace Science and Technology, Xidian University
On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <[email protected]> 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 <[email protected]>
> School of Aerospace Science and Technology, Xidian University
On 2023/3/6 09:55, Huacai Chen wrote:
> On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <[email protected]> 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/
...
> 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)
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/
On Mon, Mar 06, 2023 at 09:55:27AM +0800, Huacai Chen wrote:
> On Sun, Mar 5, 2023 at 9:28 PM Xi Ruoyao <[email protected]> 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.