Re: [PATCH v8 4/4] riscv: Set unaligned access speed at compile time

[Conor Dooley <conor.dooley@microchip.com>]

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

On Fri, Mar 08, 2024 at 01:52:24AM -0800, Emil Renner Berthing wrote:
> Charlie Jenkins wrote:

> >  config RISCV_MISALIGNED
> > -	bool "Support misaligned load/store traps for kernel and userspace"
> > +	bool
> >  	select SYSCTL_ARCH_UNALIGN_ALLOW
> > -	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.
> > +	  Embed support for misaligned load/store for both kernel and userspace.
> > +	  When disabled, misaligned accesses will generate SIGBUS in userspace
> > +	  and panic in the kernel.
> 
> Hmm.. this is *may* generate SIGBUS in userspace and panic the kernel. The CPU
> could support unaligned access natively or there might be a handler in M-mode,
> right?

Correct. The last sentence could become "When disabled, and there is no
support in hardware or firmware, unsigned accesses will...". That said,
this option is no longer user visible, so we could really simplify the
hell out of this option to just mention that it controls building the
in-kernel emulator.

> > +choice
> > +	prompt "Unaligned Accesses Support"
> > +	default RISCV_PROBE_UNALIGNED_ACCESS
> > +	help
> > +	  This determines the level of 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"
> > +	select RISCV_MISALIGNED
> > +	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 underlying system. If unaligned
> > +	  memory accesses trap into the kernel as they are not supported by the
> > +	  system, the kernel will emulate the unaligned accesses to preserve the
> > +	  UABI.
> > +
> > +config RISCV_EMULATED_UNALIGNED_ACCESS
> > +	bool "Emulate unaligned access where system support is missing"
> > +	select RISCV_MISALIGNED
> > +	help
> > +	  If unaligned memory accesses trap into the kernel as they are not
> > +	  supported by the system, the kernel will emulate the unaligned
> > +	  accesses to preserve the UABI. When the underlying system does support
> > +	  unaligned accesses, the unaligned accesses are assumed to be slow.
> 
> It's still not quite clear to me when you'd want to choose this over probing
> above. Assuming the probe measures correctly this can only result in a kernel
> that behaves the same or slower than with the option above, right?

Aye, mostly the same - some people don't want the boot-time overhead
of actually running the profiling code, so this option is for them.
Maybe that's not such a big deal anymore with the improvements to do it
in parallel, but given how bad performance on some of the systems is
when firmware does the emulation, it is definitely still noticeable.
I know we definitely have customers that care about their boot time very
strongly, so I can imagine they'd be turning this off.

> > +
> > +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 and userspace programs may not be able to run at all on systems
> > +	  that do not support unaligned memory accesses.
> 
> Again you're just explicitly saying no to the optimizations the kernel might do
> if it detects fast unaligned access, only here you'll also crash if they're not
> handled by the CPU or M-mode. Why would you want that?
> 
> I'm probably missing something, but the only reason I can think of is if you
> want build a really small kernel and save the few bytes for the handler and
> probing code.

Aye, just to allow you to disable the in-kernel emulator. That's
currently a choice that is presented to people, so this option preserves
that. IMO this is by far the least useful option and is locked behind
NONPORTABLE anyway. Maybe we could delete it, and if someone really wants
it, it would not be all that much of a hassle to add back in the future?

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