Re: [PATCH v1] RISC-V: clarify what some RISCV_ISA* config options do

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4805 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]