diff mbox series

[v3,1/2] RISC-V: clarify what some RISCV_ISA* config options do

Message ID 20240528-varnish-status-9c22973093a0@spud (mailing list archive)
State Changes Requested
Headers show
Series RISC-V: clarify what some RISCV_ISA* config options do & redo Zbb toolchain dependency | expand

Checks

Context Check Description
conchuod/vmtest-for-next-PR fail PR summary
conchuod/patch-1-test-1 success .github/scripts/patches/tests/build_rv32_defconfig.sh
conchuod/patch-1-test-2 fail .github/scripts/patches/tests/build_rv64_clang_allmodconfig.sh
conchuod/patch-1-test-3 fail .github/scripts/patches/tests/build_rv64_gcc_allmodconfig.sh
conchuod/patch-1-test-4 success .github/scripts/patches/tests/build_rv64_nommu_k210_defconfig.sh
conchuod/patch-1-test-5 success .github/scripts/patches/tests/build_rv64_nommu_virt_defconfig.sh
conchuod/patch-1-test-6 success .github/scripts/patches/tests/checkpatch.sh
conchuod/patch-1-test-7 success .github/scripts/patches/tests/dtb_warn_rv64.sh
conchuod/patch-1-test-8 success .github/scripts/patches/tests/header_inline.sh
conchuod/patch-1-test-9 success .github/scripts/patches/tests/kdoc.sh
conchuod/patch-1-test-10 success .github/scripts/patches/tests/module_param.sh
conchuod/patch-1-test-11 success .github/scripts/patches/tests/verify_fixes.sh
conchuod/patch-1-test-12 success .github/scripts/patches/tests/verify_signedoff.sh

Commit Message

Conor Dooley May 28, 2024, 11:11 a.m. UTC
From: Conor Dooley <conor.dooley@microchip.com>

During some discussion on IRC yesterday and on Pu's bpf patch [1]
I noticed that these RISCV_ISA* Kconfig options are not really clear
about their implications. Many of these options have no impact on what
userspace is allowed to do, for example an application can use Zbb
regardless of whether or not the kernel does. Change the help text to
try and clarify whether or not an option affects just the kernel, or
also userspace. None of these options actually control whether or not an
extension is detected dynamically as that's done regardless of Kconfig
options, so drop any text that implies the option is required for
dynamic detection, rewording them as "do x when y is detected".

Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/Kconfig | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

Comments

Alexandre Ghiti May 29, 2024, 8:47 a.m. UTC | #1
Hi Conor,

On 28/05/2024 13:11, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
>
> During some discussion on IRC yesterday and on Pu's bpf patch [1]
> I noticed that these RISCV_ISA* Kconfig options are not really clear
> about their implications. Many of these options have no impact on what
> userspace is allowed to do, for example an application can use Zbb
> regardless of whether or not the kernel does. Change the help text to
> try and clarify whether or not an option affects just the kernel, or
> also userspace. None of these options actually control whether or not an
> extension is detected dynamically as that's done regardless of Kconfig
> options, so drop any text that implies the option is required for
> dynamic detection, rewording them as "do x when y is detected".
>
> Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>   arch/riscv/Kconfig | 36 +++++++++++++++++++-----------------
>   1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b94176e25be1..3b702e6cc051 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -501,7 +501,8 @@ config RISCV_ISA_C
>   	help
>   	  Adds "C" to the ISA subsets that the toolchain is allowed to emit
>   	  when building Linux, which results in compressed instructions in the
> -	  Linux binary.
> +	  Linux binary. This option produces a kernel that will not run on
> +	  systems that do not support compressed instructions.
>   
>   	  If you don't know what to do here, say Y.
>   
> @@ -511,8 +512,8 @@ config RISCV_ISA_SVNAPOT
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
> -	  time and enable its usage.
> +	  Add support for the Svnapot ISA-extension in the kernel when it
> +	  is detected at boot.


To me, the new version makes things even more confusing: svnapot 
mappings will indeed be handled by the kernel (since only the kernel 
sets up the page tables) but it will only be used (for now) for HugeTLB 
mappings in userspace.


>   
>   	  The Svnapot extension is used to mark contiguous PTEs as a range
>   	  of contiguous virtual-to-physical translations for a naturally
> @@ -530,9 +531,8 @@ config RISCV_ISA_SVPBMT
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	   Adds support to dynamically detect the presence of the Svpbmt
> -	   ISA-extension (Supervisor-mode: page-based memory types) and
> -	   enable its usage.
> +	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
> +	   page-based memory types) in the kernel when it is detected at boot.
>   
>   	   The memory type for a page contains a combination of attributes
>   	   that indicate the cacheability, idempotency, and ordering
> @@ -551,14 +551,15 @@ config TOOLCHAIN_HAS_V
>   	depends on AS_HAS_OPTION_ARCH
>   
>   config RISCV_ISA_V
> -	bool "VECTOR extension support"
> +	bool "Vector extension support"
>   	depends on TOOLCHAIN_HAS_V
>   	depends on FPU
>   	select DYNAMIC_SIGFRAME
>   	default y
>   	help
> -	  Say N here if you want to disable all vector related procedure
> -	  in the kernel.
> +	  Add support for the Vector extension when it is detected at boot.
> +	  When this option is disabled, neither the kernel nor userspace may
> +	  use vector procedures.
>   
>   	  If you don't know what to do here, say Y.
>   
> @@ -616,8 +617,8 @@ config RISCV_ISA_ZBB
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	help
> -	   Adds support to dynamically detect the presence of the ZBB
> -	   extension (basic bit manipulation) and enable its usage.
> +	   Add support for enabling optimisations in the kernel when the
> +	   Zbb extension is detected at boot.
>   
>   	   The Zbb extension provides instructions to accelerate a number
>   	   of bit-specific operations (count bit population, sign extending,
> @@ -633,9 +634,9 @@ config RISCV_ISA_ZICBOM
>   	select RISCV_DMA_NONCOHERENT
>   	select DMA_DIRECT_REMAP
>   	help
> -	   Adds support to dynamically detect the presence of the ZICBOM
> -	   extension (Cache Block Management Operations) and enable its
> -	   usage.
> +	   Add support for the Zicbom extension (Cache Block Management
> +	   Operations) and enable its use in the kernel when it is detected
> +	   at boot.
>   
>   	   The Zicbom extension can be used to handle for example
>   	   non-coherent DMA support on devices that need it.
> @@ -648,7 +649,7 @@ config RISCV_ISA_ZICBOZ
>   	default y
>   	help
>   	   Enable the use of the Zicboz extension (cbo.zero instruction)
> -	   when available.
> +	   in the kernel when it is detected at boot.
>   
>   	   The Zicboz extension is used for faster zeroing of memory.
>   
> @@ -693,8 +694,9 @@ config FPU
>   	bool "FPU support"
>   	default y
>   	help
> -	  Say N here if you want to disable all floating-point related procedure
> -	  in the kernel.
> +	  Add support for floating point operations when an FPU is detected at
> +	  boot. When this option is disabled, neither the kernel nor userspace
> +	  may use the floating point unit.
>   
>   	  If you don't know what to do here, say Y.
>
Conor Dooley May 29, 2024, 8:54 a.m. UTC | #2
On Wed, May 29, 2024 at 10:47:40AM +0200, Alexandre Ghiti wrote:
> Hi Conor,
> 
> On 28/05/2024 13:11, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > During some discussion on IRC yesterday and on Pu's bpf patch [1]
> > I noticed that these RISCV_ISA* Kconfig options are not really clear
> > about their implications. Many of these options have no impact on what
> > userspace is allowed to do, for example an application can use Zbb
> > regardless of whether or not the kernel does. Change the help text to
> > try and clarify whether or not an option affects just the kernel, or
> > also userspace. None of these options actually control whether or not an
> > extension is detected dynamically as that's done regardless of Kconfig
> > options, so drop any text that implies the option is required for
> > dynamic detection, rewording them as "do x when y is detected".
> > 
> > Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >   arch/riscv/Kconfig | 36 +++++++++++++++++++-----------------
> >   1 file changed, 19 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index b94176e25be1..3b702e6cc051 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -501,7 +501,8 @@ config RISCV_ISA_C
> >   	help
> >   	  Adds "C" to the ISA subsets that the toolchain is allowed to emit
> >   	  when building Linux, which results in compressed instructions in the
> > -	  Linux binary.
> > +	  Linux binary. This option produces a kernel that will not run on
> > +	  systems that do not support compressed instructions.
> >   	  If you don't know what to do here, say Y.
> > @@ -511,8 +512,8 @@ config RISCV_ISA_SVNAPOT
> >   	depends on RISCV_ALTERNATIVE
> >   	default y
> >   	help
> > -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
> > -	  time and enable its usage.
> > +	  Add support for the Svnapot ISA-extension in the kernel when it
> > +	  is detected at boot.
> 
> 
> To me, the new version makes things even more confusing: svnapot mappings
> will indeed be handled by the kernel (since only the kernel sets up the page
> tables) but it will only be used (for now) for HugeTLB mappings in
> userspace.

How would you suggest that I word it? "Enable the use of the Svnapot
ISA-extension when it is detected at boot"? The current text implies that
these options control detection of extensions (which they do not) and
that is what I am looking to remove as it has caused confusion.
Alexandre Ghiti May 29, 2024, 9:08 a.m. UTC | #3
On 29/05/2024 10:54, Conor Dooley wrote:
> On Wed, May 29, 2024 at 10:47:40AM +0200, Alexandre Ghiti wrote:
>> Hi Conor,
>>
>> On 28/05/2024 13:11, Conor Dooley wrote:
>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>
>>> During some discussion on IRC yesterday and on Pu's bpf patch [1]
>>> I noticed that these RISCV_ISA* Kconfig options are not really clear
>>> about their implications. Many of these options have no impact on what
>>> userspace is allowed to do, for example an application can use Zbb
>>> regardless of whether or not the kernel does. Change the help text to
>>> try and clarify whether or not an option affects just the kernel, or
>>> also userspace. None of these options actually control whether or not an
>>> extension is detected dynamically as that's done regardless of Kconfig
>>> options, so drop any text that implies the option is required for
>>> dynamic detection, rewording them as "do x when y is detected".
>>>
>>> Link: https://lore.kernel.org/linux-riscv/20240328-ferocity-repose-c554f75a676c@spud/ [1]
>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>>    arch/riscv/Kconfig | 36 +++++++++++++++++++-----------------
>>>    1 file changed, 19 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index b94176e25be1..3b702e6cc051 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -501,7 +501,8 @@ config RISCV_ISA_C
>>>    	help
>>>    	  Adds "C" to the ISA subsets that the toolchain is allowed to emit
>>>    	  when building Linux, which results in compressed instructions in the
>>> -	  Linux binary.
>>> +	  Linux binary. This option produces a kernel that will not run on
>>> +	  systems that do not support compressed instructions.
>>>    	  If you don't know what to do here, say Y.
>>> @@ -511,8 +512,8 @@ config RISCV_ISA_SVNAPOT
>>>    	depends on RISCV_ALTERNATIVE
>>>    	default y
>>>    	help
>>> -	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
>>> -	  time and enable its usage.
>>> +	  Add support for the Svnapot ISA-extension in the kernel when it
>>> +	  is detected at boot.
>>
>> To me, the new version makes things even more confusing: svnapot mappings
>> will indeed be handled by the kernel (since only the kernel sets up the page
>> tables) but it will only be used (for now) for HugeTLB mappings in
>> userspace.
> How would you suggest that I word it? "Enable the use of the Svnapot
> ISA-extension when it is detected at boot"? The current text implies that
> these options control detection of extensions (which they do not) and
> that is what I am looking to remove as it has caused confusion.


Ok, I see what you mean. Your above suggestion looks great then :)

Thanks,

Alex


>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index b94176e25be1..3b702e6cc051 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -501,7 +501,8 @@  config RISCV_ISA_C
 	help
 	  Adds "C" to the ISA subsets that the toolchain is allowed to emit
 	  when building Linux, which results in compressed instructions in the
-	  Linux binary.
+	  Linux binary. This option produces a kernel that will not run on
+	  systems that do not support compressed instructions.
 
 	  If you don't know what to do here, say Y.
 
@@ -511,8 +512,8 @@  config RISCV_ISA_SVNAPOT
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	  Allow kernel to detect the Svnapot ISA-extension dynamically at boot
-	  time and enable its usage.
+	  Add support for the Svnapot ISA-extension in the kernel when it
+	  is detected at boot.
 
 	  The Svnapot extension is used to mark contiguous PTEs as a range
 	  of contiguous virtual-to-physical translations for a naturally
@@ -530,9 +531,8 @@  config RISCV_ISA_SVPBMT
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	   Adds support to dynamically detect the presence of the Svpbmt
-	   ISA-extension (Supervisor-mode: page-based memory types) and
-	   enable its usage.
+	   Add support for the Svpbmt ISA-extension (Supervisor-mode:
+	   page-based memory types) in the kernel when it is detected at boot.
 
 	   The memory type for a page contains a combination of attributes
 	   that indicate the cacheability, idempotency, and ordering
@@ -551,14 +551,15 @@  config TOOLCHAIN_HAS_V
 	depends on AS_HAS_OPTION_ARCH
 
 config RISCV_ISA_V
-	bool "VECTOR extension support"
+	bool "Vector extension support"
 	depends on TOOLCHAIN_HAS_V
 	depends on FPU
 	select DYNAMIC_SIGFRAME
 	default y
 	help
-	  Say N here if you want to disable all vector related procedure
-	  in the kernel.
+	  Add support for the Vector extension when it is detected at boot.
+	  When this option is disabled, neither the kernel nor userspace may
+	  use vector procedures.
 
 	  If you don't know what to do here, say Y.
 
@@ -616,8 +617,8 @@  config RISCV_ISA_ZBB
 	depends on RISCV_ALTERNATIVE
 	default y
 	help
-	   Adds support to dynamically detect the presence of the ZBB
-	   extension (basic bit manipulation) and enable its usage.
+	   Add support for enabling optimisations in the kernel when the
+	   Zbb extension is detected at boot.
 
 	   The Zbb extension provides instructions to accelerate a number
 	   of bit-specific operations (count bit population, sign extending,
@@ -633,9 +634,9 @@  config RISCV_ISA_ZICBOM
 	select RISCV_DMA_NONCOHERENT
 	select DMA_DIRECT_REMAP
 	help
-	   Adds support to dynamically detect the presence of the ZICBOM
-	   extension (Cache Block Management Operations) and enable its
-	   usage.
+	   Add support for the Zicbom extension (Cache Block Management
+	   Operations) and enable its use in the kernel when it is detected
+	   at boot.
 
 	   The Zicbom extension can be used to handle for example
 	   non-coherent DMA support on devices that need it.
@@ -648,7 +649,7 @@  config RISCV_ISA_ZICBOZ
 	default y
 	help
 	   Enable the use of the Zicboz extension (cbo.zero instruction)
-	   when available.
+	   in the kernel when it is detected at boot.
 
 	   The Zicboz extension is used for faster zeroing of memory.
 
@@ -693,8 +694,9 @@  config FPU
 	bool "FPU support"
 	default y
 	help
-	  Say N here if you want to disable all floating-point related procedure
-	  in the kernel.
+	  Add support for floating point operations when an FPU is detected at
+	  boot. When this option is disabled, neither the kernel nor userspace
+	  may use the floating point unit.
 
 	  If you don't know what to do here, say Y.