From: Conor Dooley <[email protected]>
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: Björn Töpel <[email protected]>
Signed-off-by: Conor Dooley <[email protected]>
---
Vector copy-paste-o fixed, correct spelling of optimisations kept.
CC: Samuel Holland <[email protected]>
CC: Pu Lehui <[email protected]>
CC: Björn Töpel <[email protected]>
CC: Paul Walmsley <[email protected]>
CC: Palmer Dabbelt <[email protected]>
CC: [email protected]
CC: [email protected]
---
arch/riscv/Kconfig | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 6d64888134ba..c3a7793b0a7c 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -503,8 +503,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 when it is detected by
+ the kernel at boot.
The Svnapot extension is used to mark contiguous PTEs as a range
of contiguous virtual-to-physical translations for a naturally
@@ -522,9 +522,9 @@ 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) when it is detected by the kernel at
+ boot.
The memory type for a page contains a combination of attributes
that indicate the cacheability, idempotency, and ordering
@@ -543,14 +543,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.
+ in the kernel. Without this option enabled, neither the kernel nor
+ userspace may use vector.
If you don't know what to do here, say Y.
@@ -608,8 +609,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,
@@ -625,9 +626,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.
@@ -686,7 +687,8 @@ config FPU
default y
help
Say N here if you want to disable all floating-point related procedure
- in the kernel.
+ in the kernel. Without this option enabled, neither the kernel nor
+ userspace may use floating-point procedures.
If you don't know what to do here, say Y.
--
2.43.0
On 2024-04-18 9:21 AM, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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: Björn Töpel <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
>
> Vector copy-paste-o fixed, correct spelling of optimisations kept.
>
> CC: Samuel Holland <[email protected]>
> CC: Pu Lehui <[email protected]>
> CC: Björn Töpel <[email protected]>
> CC: Paul Walmsley <[email protected]>
> CC: Palmer Dabbelt <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> arch/riscv/Kconfig | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
Reviewed-by: Samuel Holland <[email protected]>
On Thu, Apr 18, 2024 at 03:21:01PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> 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: Bj?rn T?pel <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
>
> Vector copy-paste-o fixed, correct spelling of optimisations kept.
>
> CC: Samuel Holland <[email protected]>
> CC: Pu Lehui <[email protected]>
> CC: Bj?rn T?pel <[email protected]>
> CC: Paul Walmsley <[email protected]>
> CC: Palmer Dabbelt <[email protected]>
> CC: [email protected]
> CC: [email protected]
> ---
> arch/riscv/Kconfig | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 6d64888134ba..c3a7793b0a7c 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -503,8 +503,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 when it is detected by
> + the kernel at boot.
I'm not sure we need the 'by the kernel', since I guess that's implied by
being in a Kconfig help text, but either way is fine by me.
>
> The Svnapot extension is used to mark contiguous PTEs as a range
> of contiguous virtual-to-physical translations for a naturally
> @@ -522,9 +522,9 @@ 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) when it is detected by the kernel at
> + boot.
Same 'by the kernel' drop suggestion.
>
> The memory type for a page contains a combination of attributes
> that indicate the cacheability, idempotency, and ordering
> @@ -543,14 +543,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.
> + in the kernel. Without this option enabled, neither the kernel nor
> + userspace may use vector.
>
> If you don't know what to do here, say Y.
>
> @@ -608,8 +609,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,
> @@ -625,9 +626,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.
> @@ -686,7 +687,8 @@ config FPU
> default y
> help
> Say N here if you want to disable all floating-point related procedure
> - in the kernel.
> + in the kernel. Without this option enabled, neither the kernel nor
> + userspace may use floating-point procedures.
>
> If you don't know what to do here, say Y.
>
Zicboz could also use some clarification, right? Or is the fact that
RISCV_ISA_ZICBOZ enables the use in both the kernel and userspace the
reason "Enable the use of the Zicboz extension (cbo.zero instruction)
when available." looks sufficient? Maybe Zicboz should follow the
"Say N here if..." pattern of V and FPU?
Thanks,
drew
On Fri, Apr 19, 2024 at 01:01:52PM +0200, Andrew Jones wrote:
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 6d64888134ba..c3a7793b0a7c 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -503,8 +503,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 when it is detected by
> > + the kernel at boot.
>
> I'm not sure we need the 'by the kernel', since I guess that's implied by
> being in a Kconfig help text, but either way is fine by me.
I think we do, given some of the options are required for userspace to
use it and others are not. Distinguishing between them doesn't cos us
more than a few characters so I think it is worthwhile.
> > @@ -686,7 +687,8 @@ config FPU
> > default y
> > help
> > Say N here if you want to disable all floating-point related procedure
> > - in the kernel.
> > + in the kernel. Without this option enabled, neither the kernel nor
> > + userspace may use floating-point procedures.
> >
> > If you don't know what to do here, say Y.
> >
>
> Zicboz could also use some clarification, right? Or is the fact that
> RISCV_ISA_ZICBOZ enables the use in both the kernel and userspace the
> reason "Enable the use of the Zicboz extension (cbo.zero instruction)
> when available." looks sufficient? Maybe Zicboz should follow the
> "Say N here if..." pattern of V and FPU?
Yeah, I think I just overlooked Zicboz. If the kernel option is needed
for userspace to use it then yeah, it should follow the same wording as
V/FPU.
On Fri, Apr 19, 2024 at 12:06:59PM +0100, Conor Dooley wrote:
> On Fri, Apr 19, 2024 at 01:01:52PM +0200, Andrew Jones wrote:
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 6d64888134ba..c3a7793b0a7c 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -503,8 +503,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 when it is detected by
> > > + the kernel at boot.
> >
> > I'm not sure we need the 'by the kernel', since I guess that's implied by
> > being in a Kconfig help text, but either way is fine by me.
>
> I think we do, given some of the options are required for userspace to
> use it and others are not. Distinguishing between them doesn't cos us
> more than a few characters so I think it is worthwhile.
I agree we should ensure 'support in the kernel' type of text is present,
but here we're saying 'detected by the kernel' which I was thinking was
implied since this is kernel code. Maybe we should just add the 'the
kernel' text to where the support is rather than where the detection is?
I assumed it was left off of the 'Add support' because Svnapot is for
S-mode.
>
>
> > > @@ -686,7 +687,8 @@ config FPU
> > > default y
> > > help
> > > Say N here if you want to disable all floating-point related procedure
> > > - in the kernel.
> > > + in the kernel. Without this option enabled, neither the kernel nor
> > > + userspace may use floating-point procedures.
> > >
> > > If you don't know what to do here, say Y.
> > >
> >
> > Zicboz could also use some clarification, right? Or is the fact that
> > RISCV_ISA_ZICBOZ enables the use in both the kernel and userspace the
> > reason "Enable the use of the Zicboz extension (cbo.zero instruction)
> > when available." looks sufficient? Maybe Zicboz should follow the
> > "Say N here if..." pattern of V and FPU?
>
> Yeah, I think I just overlooked Zicboz. If the kernel option is needed
> for userspace to use it then yeah, it should follow the same wording as
> V/FPU.
Actually, never mind. I was thinking we only set the envcfg when this
config was selected, but that's not true. We'll set it whenever the
extension is present with or without this config. So I guess it can
follow Zicbom's pattern.
Thanks,
drew
On Fri, Apr 19, 2024 at 04:05:34PM +0200, Andrew Jones wrote:
> On Fri, Apr 19, 2024 at 12:06:59PM +0100, Conor Dooley wrote:
> > On Fri, Apr 19, 2024 at 01:01:52PM +0200, Andrew Jones wrote:
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index 6d64888134ba..c3a7793b0a7c 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -503,8 +503,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 when it is detected by
> > > > + the kernel at boot.
> > >
> > > I'm not sure we need the 'by the kernel', since I guess that's implied by
> > > being in a Kconfig help text, but either way is fine by me.
> >
> > I think we do, given some of the options are required for userspace to
> > use it and others are not. Distinguishing between them doesn't cos us
> > more than a few characters so I think it is worthwhile.
>
> I agree we should ensure 'support in the kernel' type of text is present,
> but here we're saying 'detected by the kernel' which I was thinking was
> implied since this is kernel code. Maybe we should just add the 'the
> kernel' text to where the support is rather than where the detection is?
Sure, that makes sense to me. We could go for "Say y here to add support
for the Foobar ISA extension for foobarisation in the kernel when it is
detected at boot" and in the cases where userspace depends on the option
too we could additionally say "When this option is disabled, neither the
kernel nor userspace may use Foobar". So Svnapot could become
Say y here to add support for the Svnapot (Naturally Aligned Power of
Two Pages) 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
aligned power-of-2 (NAPOT) granularity larger than the base 4KB page
size. When HUGETLBFS is also selected this option unconditionally
allocates some memory for each NAPOT page size supported by the kernel.
When optimizing for low memory consumption and for platforms without
the Svnapot extension, it may be better to say N here.
And vector would be
Say y here to 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.
If you don't know what to do here, say Y.
The other thing is the "Say y here" stuff. I find it to be a little
weird to be honest - these are all default enable I don't think "Say y"
makes sense, but writing inverted descriptions feels wrong. Maybe the
solution is just s/Say y here to a/A/, which many of these extensions
already do?
> I assumed it was left off of the 'Add support' because Svnapot is for
> S-mode.
So part of my rationale for being over-eager in re-wording is that I
know people just copy-paste these config options and it's easy to miss.
>
> >
> >
> > > > @@ -686,7 +687,8 @@ config FPU
> > > > default y
> > > > help
> > > > Say N here if you want to disable all floating-point related procedure
> > > > - in the kernel.
> > > > + in the kernel. Without this option enabled, neither the kernel nor
> > > > + userspace may use floating-point procedures.
> > > >
> > > > If you don't know what to do here, say Y.
> > > >
> > >
> > > Zicboz could also use some clarification, right? Or is the fact that
> > > RISCV_ISA_ZICBOZ enables the use in both the kernel and userspace the
> > > reason "Enable the use of the Zicboz extension (cbo.zero instruction)
> > > when available." looks sufficient? Maybe Zicboz should follow the
> > > "Say N here if..." pattern of V and FPU?
> >
> > Yeah, I think I just overlooked Zicboz. If the kernel option is needed
> > for userspace to use it then yeah, it should follow the same wording as
> > V/FPU.
>
> Actually, never mind. I was thinking we only set the envcfg when this
> config was selected, but that's not true. We'll set it whenever the
> extension is present with or without this config. So I guess it can
> follow Zicbom's pattern.
I'm regretting trying to change these options sorta minimially -
Zicbom's
Add support for the Zicbom extension (Cache Block Management
Operations) and enable its use in the kernel when it is detected
at boot.
is better worded than the Svnapot one purely cos of what it looked like
before the patch. I think for v2 I'll re-write them all to look pretty
similar in terms of their opening paragraph.