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

Conor Dooley
2024-02-28T10:43:36Z

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

On Tue, Feb 27, 2024 at 03:13:14PM -0800, Charlie Jenkins wrote:
> diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
> index a7c56b41efd2..69df8eaad9e7 100644
> --- a/arch/riscv/kernel/sys_hwprobe.c
> +++ b/arch/riscv/kernel/sys_hwprobe.c
> @@ -147,8 +147,10 @@ static bool hwprobe_ext0_has(const struct cpumask *cpus, unsigned long ext)
> return (pair.value & ext);
> }
>
> +#if defined(CONFIG_RISCV_PROBE_UNALIGNED_ACCESS)
> static u64 hwprobe_misaligned(const struct cpumask *cpus)
> {

> + return RISCV_HWPROBE_MISALIGNED_FAST;

This looks like a left over testing hack.

> int cpu;
> u64 perf = -1ULL;
>
> @@ -169,6 +171,27 @@ static u64 hwprobe_misaligned(const struct cpumask *cpus)
>
> return perf;
> }
> +#elif defined(CONFIG_RISCV_EMULATED_UNALIGNED_ACCESS)
> +static u64 hwprobe_misaligned(const struct cpumask *cpus)
> +{
> + return RISCV_HWPROBE_MISALIGNED_EMULATED;
> +}
> +#elif defined(CONFIG_RISCV_SLOW_UNALIGNED_ACCESS)
> +static u64 hwprobe_misaligned(const struct cpumask *cpus)
> +{
> + return RISCV_HWPROBE_MISALIGNED_SLOW;
> +}
> +#elif defined(CONFIG_RISCV_EFFICIENT_UNALIGNED_ACCESS)
> +static u64 hwprobe_misaligned(const struct cpumask *cpus)
> +{
> + return RISCV_HWPROBE_MISALIGNED_FAST;
> +}
> +#elif defined(CONFIG_RISCV_UNSUPPORTED_UNALIGNED_ACCESS)
> +static u64 hwprobe_misaligned(const struct cpumask *cpus)
> +{
> + return RISCV_HWPROBE_MISALIGNED_UNSUPPORTED;
> +}
> +#endif

I'm curious why we need multiple definitions of this here. My first
question is why we cannot have
if (IS_ENABLED(foo))
return bar;
inside the existing function for these cases.

My second question would be why fiddle with this in hwprobe at all?
I was specifically wondering why the kernel would not track this
information in the per-cpu variable misaligned_access_speed and the
syscall code could be left in the dark about how this is implemented.

>
> static void hwprobe_one_pair(struct riscv_hwprobe *pair,
> const struct cpumask *cpus)
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 8ded225e8c5b..ba6763dd9895 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -398,8 +398,6 @@ union reg_data {
> u64 data_u64;
> };
>
> -static bool unaligned_ctl __read_mostly;
> -
> /* sysctl hooks */
> int unaligned_enabled __read_mostly = 1; /* Enabled by default */
>
> @@ -413,7 +411,9 @@ int handle_misaligned_load(struct pt_regs *regs)
>
> perf_sw_event(PERF_COUNT_SW_ALIGNMENT_FAULTS, 1, regs, addr);
>
> +#ifdef CONFIG_RISCV_PROBE_UNALIGNED_ACCESS

if (IS_ENABLED()), no?
But perhaps more interestingly - why is this being set here at all?
My understanding of the emulated stuff was that if the in-kernel
emulation was enabled, it would be used unless the CPU itself supported
fast accesses. I would expect this to be advertised to userspace once
the system has booted, not set the first time that a cpu emulates an
access,
I feel like I need to take a look at how this has been implemented, I
never really looked too closely at it and how it is enabled does not
match my expectations.

Cheers,
Conor.

> *this_cpu_ptr(&misaligned_access_speed) = RISCV_HWPROBE_MISALIGNED_EMULATED;
> +#endif
>
> if (!unaligned_enabled)
> return -1;
> @@ -595,53 +595,3 @@ int handle_misaligned_store(struct pt_regs *regs)
>
> return 0;
> }
> -
> -bool check_unaligned_access_emulated(int cpu)
> -{
> - long *mas_ptr = per_cpu_ptr(&misaligned_access_speed, cpu);
> - unsigned long tmp_var, tmp_val;
> - bool misaligned_emu_detected;
> -
> - *mas_ptr = RISCV_HWPROBE_MISALIGNED_UNKNOWN;
> -
> - __asm__ __volatile__ (
> - " "REG_L" %[tmp], 1(%[ptr])\n"
> - : [tmp] "=r" (tmp_val) : [ptr] "r" (&tmp_var) : "memory");
> -
> - misaligned_emu_detected = (*mas_ptr == RISCV_HWPROBE_MISALIGNED_EMULATED);
> - /*
> - * If unaligned_ctl is already set, this means that we detected that all
> - * CPUS uses emulated misaligned access at boot time. If that changed
> - * when hotplugging the new cpu, this is something we don't handle.
> - */
> - if (unlikely(unaligned_ctl && !misaligned_emu_detected)) {
> - pr_crit("CPU misaligned accesses non homogeneous (expected all emulated)\n");
> - while (true)
> - cpu_relax();
> - }
> -
> - return misaligned_emu_detected;
> -}
> -
> -void unaligned_emulation_finish(void)
> -{
> - int cpu;
> -
> - /*
> - * We can only support PR_UNALIGN controls if all CPUs have misaligned
> - * accesses emulated since tasks requesting such control can run on any
> - * CPU.
> - */
> - for_each_present_cpu(cpu) {
> - if (per_cpu(misaligned_access_speed, cpu) !=
> - RISCV_HWPROBE_MISALIGNED_EMULATED) {
> - return;
> - }
> - }
> - unaligned_ctl = true;
> -}
> -
> -bool unaligned_ctl_available(void)
> -{
> - return unaligned_ctl;
> -}
>
> --
> 2.43.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv