* Re: [PATCH v5 06/13] riscv: fpu: switch has_fpu() to riscv_has_extension_likely() [not found] ` <20230128172856.3814-7-jszhang@kernel.org> @ 2023-03-22 12:01 ` Jason A. Donenfeld 2023-03-22 12:09 ` [PATCH] riscv: require alternatives framework when selecting FPU support Jason A. Donenfeld 0 siblings, 1 reply; 11+ messages in thread From: Jason A. Donenfeld @ 2023-03-22 12:01 UTC (permalink / raw) To: Jisheng Zhang Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Andrew Jones, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions Hi, On Sun, Jan 29, 2023 at 01:28:49AM +0800, Jisheng Zhang wrote: > Switch has_fpu() from static branch to the new helper > riscv_has_extension_likely(). > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > --- > arch/riscv/include/asm/switch_to.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h > index 11463489fec6..60f8ca01d36e 100644 > --- a/arch/riscv/include/asm/switch_to.h > +++ b/arch/riscv/include/asm/switch_to.h > @@ -59,7 +59,8 @@ static inline void __switch_to_aux(struct task_struct *prev, > > static __always_inline bool has_fpu(void) > { > - return static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_FPU]); > + return riscv_has_extension_likely(RISCV_ISA_EXT_f) || > + riscv_has_extension_likely(RISCV_ISA_EXT_d); This causes programs to crash on kernels that are compiled with CONFIG_RISCV_ALTERNATIVE=n. Since CONFIG_RISCV_ALTERNATIVE isn't selectable, this is a problem. You can try this out for yourself using the WireGuard test suite: ARCH=riscv64 make -C tools/testing/selftests/wireguard/qemu -j$(nproc) And you'll see the crash: [ 2.172093] init.sh[45]: unhandled signal 4 code 0x1 at 0x00ffffff945a2170 in libc.so[ffffff94562000+8c000] [ 2.174306] CPU: 0 PID: 45 Comm: init.sh Not tainted 6.3.0-rc3+ #1 [ 2.174981] Hardware name: riscv-virtio,qemu (DT) [ 2.175639] epc : 00ffffff945a2170 ra : 00aaaaaae7332820 sp : 00fffffffd3e6c00 [ 2.176287] gp : 00aaaaaae73aff40 tp : 00ffffff945f1a50 t0 : 0000000000000000 [ 2.176858] t1 : 00aaaaaae7331f9c t2 : 0000000000000002 s0 : 00fffffffd3e6de0 [ 2.177427] s1 : 0000000000000002 a0 : 00aaaaaae73b7380 a1 : 00fffffffd3e6dc8 [ 2.177990] a2 : 00fffffffd3e6de0 a3 : 0000000000000000 a4 : 0000000000000000 [ 2.178524] a5 : 0000000000000002 a6 : 000000000000008b a7 : 0000000000000010 [ 2.179081] s2 : 00aaaaaae73327f0 s3 : 00ffffff945ef990 s4 : 00ffffff945f1988 [ 2.179796] s5 : 00ffffff945f1b48 s6 : 0000000000000000 s7 : 00000000000000e0 [ 2.180366] s8 : 00ffffff945f1d58 s9 : 00ffffff945ecb88 s10: 00ffffff945f17e0 [ 2.185464] s11: 0000000000000001 t3 : 00ffffff945a213c t4 : 0000000300000000 [ 2.186106] t5 : 0000000000000003 t6 : ffffffffffffffff [ 2.186520] status: 0000000200000020 badaddr: 000000000000b920 cause: 0000000000000002 I bisected it to this commit: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()") Thanks, Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 12:01 ` [PATCH v5 06/13] riscv: fpu: switch has_fpu() to riscv_has_extension_likely() Jason A. Donenfeld @ 2023-03-22 12:09 ` Jason A. Donenfeld 2023-03-22 12:46 ` Andrew Jones 0 siblings, 1 reply; 11+ messages in thread From: Jason A. Donenfeld @ 2023-03-22 12:09 UTC (permalink / raw) To: Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Andrew Jones, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions Cc: Jason A. Donenfeld When moving switch_to's has_fpu() over to using riscv_has_extension_ likely() rather than static branchs, the FPU code gained a dependency on the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is selected. Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()") Link: https://lore.kernel.org/all/ZBruFRwt3rUVngPu@zx2c4.com/ Cc: Jisheng Zhang <jszhang@kernel.org> Cc: Andrew Jones <ajones@ventanamicro.com> Cc: Heiko Stuebner <heiko@sntech.de> Cc: Conor Dooley <conor.dooley@microchip.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- arch/riscv/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index c5e42cc37604..0f59350c699d 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE config FPU bool "FPU support" default y + select RISCV_ALTERNATIVE help Say N here if you want to disable all floating-point related procedure in the kernel. -- 2.40.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 12:09 ` [PATCH] riscv: require alternatives framework when selecting FPU support Jason A. Donenfeld @ 2023-03-22 12:46 ` Andrew Jones 2023-03-22 15:17 ` Conor Dooley 0 siblings, 1 reply; 11+ messages in thread From: Andrew Jones @ 2023-03-22 12:46 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote: > When moving switch_to's has_fpu() over to using riscv_has_extension_ > likely() rather than static branchs, the FPU code gained a dependency on > the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected > when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not > work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is > selected. > > Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()") > Link: https://lore.kernel.org/all/ZBruFRwt3rUVngPu@zx2c4.com/ > Cc: Jisheng Zhang <jszhang@kernel.org> > Cc: Andrew Jones <ajones@ventanamicro.com> > Cc: Heiko Stuebner <heiko@sntech.de> > Cc: Conor Dooley <conor.dooley@microchip.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > arch/riscv/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index c5e42cc37604..0f59350c699d 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE > config FPU > bool "FPU support" > default y > + select RISCV_ALTERNATIVE > help > Say N here if you want to disable all floating-point related procedure > in the kernel. > -- > 2.40.0 > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> I took a look to see if we missed anything else and see that we should do the same patch for KVM. I'll send one. (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we can defer that wedding a bit longer.) Thanks, drew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 12:46 ` Andrew Jones @ 2023-03-22 15:17 ` Conor Dooley 2023-03-22 19:26 ` Andrew Jones 0 siblings, 1 reply; 11+ messages in thread From: Conor Dooley @ 2023-03-22 15:17 UTC (permalink / raw) To: Andrew Jones Cc: Jason A. Donenfeld, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions [-- Attachment #1: Type: text/plain, Size: 1932 bytes --] On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote: > > When moving switch_to's has_fpu() over to using riscv_has_extension_ > > likely() rather than static branchs, the FPU code gained a dependency on > > the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected > > when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not > > work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is > > selected. > > > > Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()") > > Link: https://lore.kernel.org/all/ZBruFRwt3rUVngPu@zx2c4.com/ > > Cc: Jisheng Zhang <jszhang@kernel.org> > > Cc: Andrew Jones <ajones@ventanamicro.com> > > Cc: Heiko Stuebner <heiko@sntech.de> > > Cc: Conor Dooley <conor.dooley@microchip.com> Thanks for fixing it! Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > --- > > arch/riscv/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index c5e42cc37604..0f59350c699d 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE > > config FPU > > bool "FPU support" > > default y > > + select RISCV_ALTERNATIVE > > help > > Say N here if you want to disable all floating-point related procedure > > in the kernel. > > -- > > 2.40.0 > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > I took a look to see if we missed anything else and see that we should > do the same patch for KVM. I'll send one. > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > can defer that wedding a bit longer.) At that point, the config option should just go away entirely, no? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 15:17 ` Conor Dooley @ 2023-03-22 19:26 ` Andrew Jones 2023-03-22 19:44 ` Conor Dooley 0 siblings, 1 reply; 11+ messages in thread From: Andrew Jones @ 2023-03-22 19:26 UTC (permalink / raw) To: Conor Dooley Cc: Jason A. Donenfeld, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote: > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > > On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote: > > > When moving switch_to's has_fpu() over to using riscv_has_extension_ > > > likely() rather than static branchs, the FPU code gained a dependency on > > > the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected > > > when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not > > > work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is > > > selected. > > > > > > Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()") > > > Link: https://lore.kernel.org/all/ZBruFRwt3rUVngPu@zx2c4.com/ > > > Cc: Jisheng Zhang <jszhang@kernel.org> > > > Cc: Andrew Jones <ajones@ventanamicro.com> > > > Cc: Heiko Stuebner <heiko@sntech.de> > > > Cc: Conor Dooley <conor.dooley@microchip.com> > > Thanks for fixing it! > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > > > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > > --- > > > arch/riscv/Kconfig | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > > index c5e42cc37604..0f59350c699d 100644 > > > --- a/arch/riscv/Kconfig > > > +++ b/arch/riscv/Kconfig > > > @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE > > > config FPU > > > bool "FPU support" > > > default y > > > + select RISCV_ALTERNATIVE > > > help > > > Say N here if you want to disable all floating-point related procedure > > > in the kernel. > > > -- > > > 2.40.0 > > > > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > > I took a look to see if we missed anything else and see that we should > > do the same patch for KVM. I'll send one. > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > > can defer that wedding a bit longer.) > > At that point, the config option should just go away entirely, no? Ah, yes, and that makes the idea even more attractive, as we could remove several ifdefs. Thanks, drew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 19:26 ` Andrew Jones @ 2023-03-22 19:44 ` Conor Dooley 2023-03-22 20:05 ` Conor Dooley 0 siblings, 1 reply; 11+ messages in thread From: Conor Dooley @ 2023-03-22 19:44 UTC (permalink / raw) To: Andrew Jones Cc: Jason A. Donenfeld, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions [-- Attachment #1: Type: text/plain, Size: 639 bytes --] On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote: > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote: > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > > > can defer that wedding a bit longer.) > > > > At that point, the config option should just go away entirely, no? > > Ah, yes, and that makes the idea even more attractive, as we could remove > several ifdefs. I went and did the cursory check, it's not compatible with XIP_KERNEL so dropping the option entirely probably isn't a possibility :/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 19:44 ` Conor Dooley @ 2023-03-22 20:05 ` Conor Dooley 2023-03-22 20:19 ` Jason A. Donenfeld 0 siblings, 1 reply; 11+ messages in thread From: Conor Dooley @ 2023-03-22 20:05 UTC (permalink / raw) To: Andrew Jones Cc: Jason A. Donenfeld, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions [-- Attachment #1: Type: text/plain, Size: 991 bytes --] On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote: > On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote: > > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote: > > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > > > > can defer that wedding a bit longer.) > > > > > > At that point, the config option should just go away entirely, no? > > > > Ah, yes, and that makes the idea even more attractive, as we could remove > > several ifdefs. > > I went and did the cursory check, it's not compatible with XIP_KERNEL so > dropping the option entirely probably isn't a possibility :/ What I said is only now sinking in. We're now going to be disabling FPU support on XIP kernels with this patch. Well, technically not this patch since it wouldn't have built without Jason's changes, but that doesn't seem like the right thing to do... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 20:05 ` Conor Dooley @ 2023-03-22 20:19 ` Jason A. Donenfeld 2023-03-23 14:49 ` Conor Dooley 0 siblings, 1 reply; 11+ messages in thread From: Jason A. Donenfeld @ 2023-03-22 20:19 UTC (permalink / raw) To: Conor Dooley Cc: Andrew Jones, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, Conor Dooley, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions On Wed, Mar 22, 2023 at 9:05 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote: > > On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote: > > > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote: > > > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > > > > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > > > > > can defer that wedding a bit longer.) > > > > > > > > At that point, the config option should just go away entirely, no? > > > > > > Ah, yes, and that makes the idea even more attractive, as we could remove > > > several ifdefs. > > > > I went and did the cursory check, it's not compatible with XIP_KERNEL so > > dropping the option entirely probably isn't a possibility :/ > > What I said is only now sinking in. We're now going to be disabling FPU > support on XIP kernels with this patch. > Well, technically not this patch since it wouldn't have built without > Jason's changes, but that doesn't seem like the right thing to do... I suppose you could have riscv_has_extension_*() fall back to something that doesn't use alternatives on XIP kernels. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-22 20:19 ` Jason A. Donenfeld @ 2023-03-23 14:49 ` Conor Dooley 2023-03-23 15:56 ` Jason A. Donenfeld 0 siblings, 1 reply; 11+ messages in thread From: Conor Dooley @ 2023-03-23 14:49 UTC (permalink / raw) To: Jason A. Donenfeld, Andrew Jones, Jisheng Zhang Cc: Conor Dooley, Andrew Jones, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions [-- Attachment #1: Type: text/plain, Size: 4029 bytes --] On Wed, Mar 22, 2023 at 09:19:50PM +0100, Jason A. Donenfeld wrote: > On Wed, Mar 22, 2023 at 9:05 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote: > > > On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote: > > > > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote: > > > > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote: > > > > > > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we > > > > > > can defer that wedding a bit longer.) > > > > > > > > > > At that point, the config option should just go away entirely, no? > > > > > > > > Ah, yes, and that makes the idea even more attractive, as we could remove > > > > several ifdefs. > > > > > > I went and did the cursory check, it's not compatible with XIP_KERNEL so > > > dropping the option entirely probably isn't a possibility :/ > > > > What I said is only now sinking in. We're now going to be disabling FPU > > support on XIP kernels with this patch. > > Well, technically not this patch since it wouldn't have built without > > Jason's changes, but that doesn't seem like the right thing to do... > > I suppose you could have riscv_has_extension_*() fall back to > something that doesn't use alternatives on XIP kernels. Yah, something like the below I guess? Probably overlooking something silly & it's lost the benefit of the static branch that it used to have, but with the infra that we have at the moment this seemed like the sanest thing to do? This would requiring picking up your patch Jason, but with an "if !XIP_KERNEL" added to the select. It's only had the lightest of build tests, but I can go make it a real patch if there's not something obviously amiss. diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h index e3021b2590de..6263a0de1c6a 100644 --- a/arch/riscv/include/asm/hwcap.h +++ b/arch/riscv/include/asm/hwcap.h @@ -57,18 +57,31 @@ struct riscv_isa_ext_data { unsigned int isa_ext_id; }; +unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); + +#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) + +bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); +#define riscv_isa_extension_available(isa_bitmap, ext) \ + __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) + static __always_inline bool riscv_has_extension_likely(const unsigned long ext) { compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX"); - asm_volatile_goto( - ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1) - : - : [ext] "i" (ext) - : - : l_no); + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + asm_volatile_goto( + ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1) + : + : [ext] "i" (ext) + : + : l_no); + } else { + if (!__riscv_isa_extension_available(NULL, ext)) + goto l_no; + } return true; l_no: @@ -81,26 +94,23 @@ riscv_has_extension_unlikely(const unsigned long ext) compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX"); - asm_volatile_goto( - ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1) - : - : [ext] "i" (ext) - : - : l_yes); + if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) { + asm_volatile_goto( + ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1) + : + : [ext] "i" (ext) + : + : l_yes); + } else { + if (__riscv_isa_extension_available(NULL, ext)) + goto l_yes; + } return false; l_yes: return true; } -unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap); - -#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext) - -bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit); -#define riscv_isa_extension_available(isa_bitmap, ext) \ - __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext) - #endif #endif /* _ASM_RISCV_HWCAP_H */ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-23 14:49 ` Conor Dooley @ 2023-03-23 15:56 ` Jason A. Donenfeld 2023-03-23 22:19 ` Conor Dooley 0 siblings, 1 reply; 11+ messages in thread From: Jason A. Donenfeld @ 2023-03-23 15:56 UTC (permalink / raw) To: Conor Dooley Cc: Andrew Jones, Jisheng Zhang, Conor Dooley, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions On Thu, Mar 23, 2023 at 02:49:34PM +0000, Conor Dooley wrote: > This would requiring picking up your patch Jason, but with an > "if !XIP_KERNEL" added to the select. So the risk of making this all work is that we wind up forgetting to add `select alternatives if !xip` to various places that need it (fpu, kvm, maybe others? future others?), because it appears to work, thanks to the code in your patch. But making it work is also probably a good thing, since we obviously want the fpu and maybe other things to work on xip kernels. So maybe we should get rid of the CONFIG_RISCV_ALTERNATIVES knob entirely, making it "always enabled", and then conditonalize the alternatives code to BUILD_BUG_ON when called with CONFIG_XIP_KERNEL=y. Then, this build bug will get hit immediately by riscv_has_extension_*(), which will then require your patch, which can run in a `if (IS_ENABLED(XIP_KERNEL))` block or similar. The result of that will be: - !xip kernels properly use the fast riscv_has_extension_*() code and any alternatives code needed, since it's always selected. - xip kernels get a BUILD_BUG_ON if they use any alternatives-based code that doesn't have a xip fallback yet. What do you think of that approach? A "lighter weight" version of that approach would be to just remove all of the `select RISCV_ALTERNATIVES` lines, and instead make RISCV_ALTERNATIVES specify `default !XIP_KERNEL`. That would more or less amount to the above too, though with weirder error cases. Jason ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] riscv: require alternatives framework when selecting FPU support 2023-03-23 15:56 ` Jason A. Donenfeld @ 2023-03-23 22:19 ` Conor Dooley 0 siblings, 0 replies; 11+ messages in thread From: Conor Dooley @ 2023-03-23 22:19 UTC (permalink / raw) To: Jason A. Donenfeld Cc: Conor Dooley, Andrew Jones, Jisheng Zhang, Palmer Dabbelt, Paul Walmsley, Albert Ou, Anup Patel, Atish Patra, Heiko Stuebner, linux-riscv, linux-kernel, kvm, kvm-riscv, regressions, regressions [-- Attachment #1: Type: text/plain, Size: 4202 bytes --] Hey Jason, I read this mail before I left work today & had a think about it on the bike home, and had a whole response thought out, got distracted and forgot it all.. Hopefully I've remembered everything I had to say! On Thu, Mar 23, 2023 at 04:56:24PM +0100, Jason A. Donenfeld wrote: > On Thu, Mar 23, 2023 at 02:49:34PM +0000, Conor Dooley wrote: > > This would requiring picking up your patch Jason, but with an > > "if !XIP_KERNEL" added to the select. > > So the risk of making this all work is that we wind up forgetting to add > `select alternatives if !xip` to various places that need it (fpu, kvm, > maybe others? future others?), because it appears to work, thanks to the > code in your patch. > > But making it work is also probably a good thing, since we obviously > want the fpu and maybe other things to work on xip kernels. I'm not super pushed about the "maybe other things", since the "maybe other things" that are in my head (errata and recently added extensions) have never worked on xip kernels, and losing them isn't a regression. Since XIP_KERNEL is deemed to be a "NONPORTABLE" option, we wouldn't need alternatives to enable it for them, but changes would be required for that to make the alternatives collapse to a build time thing. Can deal with that iff someone actually does come along wanting it. We do need to fix this so that situations like the one you hit can't happen, while not regressing the level of support for xip, so some level of "making it work" is needed, but I do agree that it needs to be done in a less footgun way. > So maybe we should get rid of the CONFIG_RISCV_ALTERNATIVES knob > entirely, making it "always enabled", and then conditonalize the > alternatives code to BUILD_BUG_ON when called with CONFIG_XIP_KERNEL=y. > Then, this build bug will get hit immediately by > riscv_has_extension_*(), which will then require your patch, which can > run in a `if (IS_ENABLED(XIP_KERNEL))` block or similar. > > The result of that will be: > - !xip kernels properly use the fast riscv_has_extension_*() code and > any alternatives code needed, since it's always selected. > - xip kernels get a BUILD_BUG_ON if they use any alternatives-based code > that doesn't have a xip fallback yet. > > What do you think of that approach? Initially I thought "great, lets always enable the alternatives framework" but I don't think we can do that. For the has_extension() stuff a fallback is fine, but I don't think that applies to using alternatives for either errata or enabling extensions at runtime. I just don't really want to go through and modify the alternative macros so that they're evaluated at build time for xip unless that is absolutely required down the line. (I'd rather not even do it at all.) Most of the things that are currently selecting RISCV_ALTERNATIVE do so to patch in support for extensions or enable errata, and I don't think we should expose those config options if the alternatives that they rely on cannot be used. I think that means something like... > A "lighter weight" version of that approach would be to just remove all of > the `select RISCV_ALTERNATIVES` lines, and instead make > RISCV_ALTERNATIVES specify `default !XIP_KERNEL`. That would more or > less amount to the above too, though with weirder error cases. ...adding a "select RISCV_ALTERNATIVE if !XIP_KERNEL" to the CONFIG_RISCV entry, and similarly to what you suggest here, swapping all of the instances of "select RISCV_ALTERNATIVE" for "depends on RISCV_ALTERNATIVE". That does still mean we can drop all of the "depends on !XIP_KERNEL" that are littered around the place whereever we are using alternatives & should only get the slow path for extension checking for xip kernels. That'd handle the issue that you pointed out, where if the select is missing, my suggested change makes it appear to work if alternatives are not enabled too. The BUILD_BUG_ON idea is good too, probably not fixes material, but might be worth having to prevent alternatives somehow being used when XIP_KERNEL is set. I'll try to whip something up tomorrow... Thanks Jason, Conor. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-03-23 22:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20230128172856.3814-1-jszhang@kernel.org>
[not found] ` <20230128172856.3814-7-jszhang@kernel.org>
2023-03-22 12:01 ` [PATCH v5 06/13] riscv: fpu: switch has_fpu() to riscv_has_extension_likely() Jason A. Donenfeld
2023-03-22 12:09 ` [PATCH] riscv: require alternatives framework when selecting FPU support Jason A. Donenfeld
2023-03-22 12:46 ` Andrew Jones
2023-03-22 15:17 ` Conor Dooley
2023-03-22 19:26 ` Andrew Jones
2023-03-22 19:44 ` Conor Dooley
2023-03-22 20:05 ` Conor Dooley
2023-03-22 20:19 ` Jason A. Donenfeld
2023-03-23 14:49 ` Conor Dooley
2023-03-23 15:56 ` Jason A. Donenfeld
2023-03-23 22:19 ` Conor Dooley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).