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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #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

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