Received-SPF: pass (google.com: domain of linux-kernel+bounces-83826-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 27 Feb 2024 10:17:21 -0800
From: Charlie Jenkins <charlie@rivosinc.com>
To: Conor Dooley <conor.dooley@microchip.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	Jisheng Zhang <jszhang@kernel.org>, Evan Green <evan@rivosinc.com>,
	=?iso-8859-1?Q?Cl=E9ment_L=E9ger?= <cleger@rivosinc.com>,
	Eric Biggers <ebiggers@kernel.org>,
	Elliot Berman <quic_eberman@quicinc.com>,
	Charles Lohr <lohr85@gmail.com>, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] riscv: Set unalignment speed at compile time
Message-ID: <Zd4nMa+x28omuhiF@ghost>
References: <20240216-disable_misaligned_probe_config-v4-0-dc01e581c0ac@rivosinc.com>
 <20240216-disable_misaligned_probe_config-v4-2-dc01e581c0ac@rivosinc.com>
 <20240227-condone-impeach-9469dffc6b47@wendy>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240227-condone-impeach-9469dffc6b47@wendy>

On Tue, Feb 27, 2024 at 11:39:25AM +0000, Conor Dooley wrote:
> On Fri, Feb 16, 2024 at 12:33:19PM -0800, Charlie Jenkins wrote:
> > Introduce Kconfig options to set the kernel unaligned access support.
> > These options provide a non-portable alternative to the runtime
> > unaligned access probe.
> > 
> > To support this, the unaligned access probing code is moved into it's
> > own file and gated behind a new RISCV_PROBE_UNALIGNED_ACCESS_SUPPORT
> > option.
> > 
> > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
> > ---
> >  arch/riscv/Kconfig                          |  58 +++++-
> >  arch/riscv/include/asm/cpufeature.h         |  30 +++-
> >  arch/riscv/kernel/Makefile                  |   6 +-
> >  arch/riscv/kernel/cpufeature.c              | 255 --------------------------
> >  arch/riscv/kernel/misaligned_access_speed.c | 265 ++++++++++++++++++++++++++++
> >  arch/riscv/kernel/probe_emulated_access.c   |  64 +++++++
> >  arch/riscv/kernel/sys_hwprobe.c             |  25 +++
> >  arch/riscv/kernel/traps_misaligned.c        |  54 +-----
> >  8 files changed, 442 insertions(+), 315 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index bffbd869a068..3cf700adc43b 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -690,25 +690,71 @@ config THREAD_SIZE_ORDER
> >  config RISCV_MISALIGNED
> 
> 
> Why can we not make up our minds on what to call this? The majority of
> users are "unaligned" but the file you add and this config option are
> "misaligned."

We have both everywhere, maybe we (I?) should go in and standardize the
wording everywhere. I personally prefer "misaligned" which means
"incorrectly aligned" over "unaligned" which means "not aligned" because
a 7-bit alignment is still "aligned" along a 7-bit boundary, but it is
certainly incorrectly aligned.

> 
> >  	bool "Support misaligned load/store traps for kernel and userspace"
> >  	select SYSCTL_ARCH_UNALIGN_ALLOW
> > +	depends on RISCV_PROBE_UNALIGNED_ACCESS || RISCV_EMULATED_UNALIGNED_ACCESS
> >  	default y
> >  	help
> >  	  Say Y here if you want the kernel to embed support for misaligned
> >  	  load/store for both kernel and userspace. When disable, misaligned
> >  	  accesses will generate SIGBUS in userspace and panic in kernel.
> >  
> > +choice
> > +	prompt "Unaligned Accesses Support"
> > +	default RISCV_PROBE_UNALIGNED_ACCESS
> > +	help
> > +	  This selects the hardware support for unaligned accesses. This
> > +	  information is used by the kernel to perform optimizations. It is also
> > +	  exposed to user space via the hwprobe syscall. The hardware will be
> > +	  probed at boot by default.
> > +
> > +config RISCV_PROBE_UNALIGNED_ACCESS
> > +	bool "Probe for hardware unaligned access support"
> > +	help
> > +	  During boot, the kernel will run a series of tests to determine the
> > +	  speed of unaligned accesses. This is the only portable option. This
> > +	  probing will dynamically determine the speed of unaligned accesses on
> > +	  the boot hardware.
> > +
> > +config RISCV_EMULATED_UNALIGNED_ACCESS
> > +	bool "Assume the CPU expects emulated unaligned memory accesses"
> > +	depends on NONPORTABLE
> 
> This is portable too, right?

I guess so? I think I would prefer to have the probing being the only
portable option.

> 
> > +	select RISCV_MISALIGNED
> > +	help
> > +	  Assume that the CPU expects emulated unaligned memory accesses.
> > +	  When enabled, this option notifies the kernel and userspace that
> > +	  unaligned memory accesses will be emulated by the kernel.
> 
> > To enforce
> > +	  this expectation, RISCV_MISALIGNED is selected by this option.
> 
> Drop this IMO, let Kconfig handle displaying the dependencies.
> 

I was debating if Kconfig handling was enough, so I am glad it is, I
will remove this.

> > +
> > +config RISCV_SLOW_UNALIGNED_ACCESS
> > +	bool "Assume the CPU supports slow unaligned memory accesses"
> > +	depends on NONPORTABLE
> > +	help
> > +	  Assume that the CPU supports slow unaligned memory accesses. When
> > +	  enabled, this option improves the performance of the kernel on such
> > +	  CPUs.
> 
> Does it? Are you sure that generating unaligned accesses on systems
> where they are slow is a performance increase?
> That said, I don't really see this option actually doing anything other
> than setting the value for hwprobe, so I don't actually know what the
> effect of this option actually is on the kernel's performance.
> 
> Generally I would like to suggest a change from "CPU" to "system" here,
> since the slow cases that exist are mostly because the unaligned access
> is actually emulated in firmware.

It would be ideal if "emulated" was used for any case of emulated
accesses (firmware or in the kernel).  Doing emulated accesses will be
orders of magnitude slower than a processor that "slowly" handles the
accesses.

So even if the processor performs a "slow" access, it could still be
beneficial for the kernel to do the misaligned access rather than manual
do the alignment.

Currently there is no place that takes into account this "slow" option
but I wanted to leave it open for future optimizations.

> 
> > However, the kernel will run much more slowly, or will not be
> > +	  able to run at all, on CPUs that do not support unaligned memory
> > +	  accesses.
> > +
> >  config RISCV_EFFICIENT_UNALIGNED_ACCESS
> >  	bool "Assume the CPU supports fast unaligned memory accesses"
> >  	depends on NONPORTABLE
> >  	select DCACHE_WORD_ACCESS if MMU
> >  	select HAVE_EFFICIENT_UNALIGNED_ACCESS
> >  	help
> > -	  Say Y here if you want the kernel to assume that the CPU supports
> > -	  efficient unaligned memory accesses.  When enabled, this option
> > -	  improves the performance of the kernel on such CPUs.  However, the
> > -	  kernel will run much more slowly, or will not be able to run at all,
> > -	  on CPUs that do not support efficient unaligned memory accesses.
> > +	  Assume that the CPU supports fast unaligned memory accesses. When
> > +	  enabled, this option improves the performance of the kernel on such
> > +	  CPUs.  However, the kernel will run much more slowly, or will not be
> > +	  able to run at all, on CPUs that do not support efficient unaligned
> > +	  memory accesses.
> > +
> > +config RISCV_UNSUPPORTED_UNALIGNED_ACCESS
> 
> This option needs to be removed. The uabi states that unaligned access
> is supported in userspace, if the cpu or firmware does not implement
> unaligned access then the kernel must emulate it.

Should it removed from hwprobe as well then?

> 
> > +	bool "Assume the CPU doesn't support unaligned memory accesses"
> > +	depends on NONPORTABLE
> > +	help
> > +	  Assume that the CPU doesn't support unaligned memory accesses. Only
> > +	  select this option if there is no registered trap handler to emulate
> > +	  unaligned accesses.
> >  
> > -	  If unsure what to do here, say N.
> > +endchoice
> >  
> >  endmenu # "Platform type"
> 
> > diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> > index eb3ac304fc42..928ad3384406 100644
> > --- a/arch/riscv/include/asm/cpufeature.h
> > +++ b/arch/riscv/include/asm/cpufeature.h
> > @@ -33,10 +33,26 @@ extern struct riscv_isainfo hart_isa[NR_CPUS];
> >  
> >  void riscv_user_isa_enable(void);
> >  
> > -#ifdef CONFIG_RISCV_MISALIGNED
> > +#if defined(CONFIG_RISCV_MISALIGNED)
> > +#if defined(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)
> >  bool unaligned_ctl_available(void);
> >  bool check_unaligned_access_emulated(int cpu);
> >  void unaligned_emulation_finish(void);
> > +#elif defined(CONFIG_RISCV_EMULATED_UNALIGNED_ACCESS)
> > +static inline bool unaligned_ctl_available(void)
> > +{
> > +	return true;
> > +}
> > +
> > +static inline bool check_unaligned_access_emulated(int cpu)
> > +{
> > +	return true;
> > +}
> > +
> > +static inline void unaligned_emulation_finish(void) {}
> > +#else
> > +#error "CONFIG_RISCV_MISALIGNED is only supported if CONFIG_RISCV_PROBE_UNALIGNED_ACCESS or CONFIG_RISCV_EMULATED_UNALIGNED_ACCESS is selected."
> 
> Why is this an error? Enforce this in Kconfig (you already have) and drop
> the clause.

Awesome, I wasn't sure if the Kconfig was "enough".

- Charlie

> 
> Cheers,
> Conor.