Message-ID: <20240227-condone-impeach-9469dffc6b47@wendy>
Date: Tue, 27 Feb 2024 11:39:25 +0000
From: Conor Dooley <conor.dooley@...rochip.com>
To: Charlie Jenkins <charlie@...osinc.com>
CC: Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
	<palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Jisheng Zhang
	<jszhang@...nel.org>, Evan Green <evan@...osinc.com>,
	Clément Léger <cleger@...osinc.com>, Eric Biggers
	<ebiggers@...nel.org>, Elliot Berman <quic_eberman@...cinc.com>, Charles Lohr
	<lohr85@...il.com>, <linux-riscv@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] riscv: Set unalignment speed at compile time

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@...osinc.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."

>  	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?

> +	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.

> +
> +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.

> 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.

> +	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.

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists