| Message ID | 20240418-stable-railway-7cce07e1e440@spud (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] RISC-V: clarify what some RISCV_ISA* config options do | expand |
On 2024-04-18 9:21 AM, 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: Björn Töpel <bjorn@rivosinc.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > > Vector copy-paste-o fixed, correct spelling of optimisations kept. > > CC: Samuel Holland <samuel.holland@sifive.com> > CC: Pu Lehui <pulehui@huaweicloud.com> > CC: Björn Töpel <bjorn@kernel.org> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: linux-riscv@lists.infradead.org > CC: linux-kernel@vger.kernel.org > --- > arch/riscv/Kconfig | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) Reviewed-by: Samuel Holland <samuel.holland@sifive.com>
On Thu, Apr 18, 2024 at 03:21:01PM +0100, 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: Björn Töpel <bjorn@rivosinc.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > > Vector copy-paste-o fixed, correct spelling of optimisations kept. > > CC: Samuel Holland <samuel.holland@sifive.com> > CC: Pu Lehui <pulehui@huaweicloud.com> > CC: Björn Töpel <bjorn@kernel.org> > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: linux-riscv@lists.infradead.org > CC: linux-kernel@vger.kernel.org > --- > 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.
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.