Re: [PATCH v5 2/2] riscv: Set unaligned access speed at compile time

Conor Dooley
2024-02-29T12:26:53Z

[-- Attachment #1.1: Type: text/plain, Size: 5782 bytes --]

On Tue, Feb 27, 2024 at 03:13:14PM -0800, Charlie Jenkins wrote:

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index bffbd869a068..ad0a9c1f8802 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -690,25 +690,58 @@ config THREAD_SIZE_ORDER
> config RISCV_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 probing will dynamically determine
> + the speed of unaligned accesses on the boot hardware.
> +
> +config RISCV_EMULATED_UNALIGNED_ACCESS
> + bool "Assume the system expects emulated unaligned memory accesses"
> + help
> + Assume that the system expects emulated unaligned memory accesses.
> + When enabled, this option notifies the kernel and userspace that
> + unaligned memory accesses will be emulated by either the kernel or
> + firmware.
> +
> +config RISCV_SLOW_UNALIGNED_ACCESS
> + bool "Assume the system supports slow unaligned memory accesses"
> + depends on NONPORTABLE
> + help
> + Assume that the system supports slow unaligned memory accesses. The
> + kernel may not be able to run at all on systems that do not support
> + unaligned memory accesses.
> +
> config RISCV_EFFICIENT_UNALIGNED_ACCESS
> - bool "Assume the CPU supports fast unaligned memory accesses"
> + bool "Assume the system 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 system supports fast unaligned memory accesses. When
> + enabled, this option improves the performance of the kernel on such
> + systems. However, the kernel will run much more slowly, or will not
> + be able to run at all, on systems that do not support efficient
> + unaligned memory accesses.
>
> - If unsure what to do here, say N.
> +endchoice

Thinking about this some more, you've got 6 different options here:

1 probed with no emulation available (choice set to probe + RISCV_MISALIGNED=n)
2 probe with in-kernel emulation available (choice set to probe + RISCV_MISALIGNED=y)
3 in-kernel emulation only (choice set to emulated + RISCV_MISALIGNED=y)
4 no in-kernel emulation but report emulated (choice set to emulated + RISCV_MISALIGNED=n)
5 slow unaligned (choice set to slow)
6 fast unaligned (choice set to fast)

Out of these, only 2 and 3 are portable options, since 1, 4 and 5 will
cause uabi issues if the CPUs or firmware does not support unaligned
access & 6 will not run in the same circumstances.

My first thought here was about the motivation for the patch and what it
has resulted in. Being able to support HAVE_EFFICIENT_ALIGNED_ACCESS is
pretty nice, but it then seems like beyond that we are introducing
configuration for configurations sake, without looking at what the
resultant kernels will be useful for. Having 6 different options for how
the kernel can be configured in this way seems excessive and I don't
really get why some of them are even useful.

Take for example situation 4. Unless I have misunderstood the Kconfig
options above, if you configure a kernel in that way, it will always
report as emulated, but there is no emulation provided. This just seems
like a option that's only purpose is setting a hwprobe value, which is
a dog wagging the tail situation to me.

The other thing is about what options are actually marked as
NONPORTABLE, given it is done in the choice option - but whether or not
something is actually non-portable actually depends on whether or not
the in-kernel emulator exists.

I think what I would do here is simplify this quite a bit, starting by
making RISCV_MISALIGNED an internal option that users cannot enable but
is selected by the PORTABLE choice options. I would then re-work the
choice options a bit. My 4 would be:

1 probe: probe at boot time, falling back to emulated if not performant
2 emulated: always emulate it in the kernel
3 slow: don't probe or emulate in the kernel
4 fast: Your current fast option

1 & 2 select RISCV_UNALIGNED and are portable because they contain the
emulated support and thus satisfy the UABI constaints.
3 & 4 are marked NONPORTABLE. I think 3 will run on systems that don't
support unaligned accesses but it will have UABI problems. 4 for the
reason mentioned in the Kconfig option above.

I think that that gives you 4 meaningful options, what do you think?

Cheers,
Conor.