Messages in this thread | | | Date | Thu, 29 Feb 2024 12:26:37 +0000 | From | Conor Dooley <> | Subject | Re: [PATCH v5 2/2] riscv: Set unaligned access speed at compile time |
| |
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. [unhandled content-type:application/pgp-signature] | |