[PATCH 7/9] riscv: Prepare for unaligned access type table lookups

[Charlie Jenkins]

On Mon, Feb 10, 2025 at 03:20:34PM +0100, Clément Léger wrote:
> 
> 
> On 10/02/2025 15:06, Andrew Jones wrote:
> > On Mon, Feb 10, 2025 at 12:07:40PM +0100, Clément Léger wrote:
> >>
> >>
> >> On 10/02/2025 11:16, Anup Patel wrote:
> >>> On Sat, Feb 8, 2025 at 6:53 AM Charlie Jenkins <charlie at rivosinc.com> wrote:
> >>>>
> >>>> On Fri, Feb 07, 2025 at 05:19:47PM +0100, Andrew Jones wrote:
> >>>>> Probing unaligned accesses on boot is time consuming. Provide a
> >>>>> function which will be used to look up the access type in a table
> >>>>> by id registers. Vendors which provide table entries can then skip
> >>>>> the probing.
> >>>>
> >>>> The access checker in my experience is only time consuming on slow
> >>>> hardware. Hardware that supports fast unaligned accesses isn't really
> >>>> impacted by this? Avoiding a list of hardware that has slow/fast
> >>>> unaligned accesses in the kernel was the main reason for dynamically
> >>>> checking. We did introduce the config option to compile the kernel with
> >>>> assumed slow/fast accesses, which of course has the downside of
> >>>> recompiling the kernel and I assume that you already considered that.
> >>>
> >>> The kconfig option does not align with the vision of running the same
> >>> kernel image across platforms.
> >>
> >> I'd would be advocating to remove compile time options as well and use
> >> another way to skip the probe (see below).
> >>
> >>>
> >>>>
> >>>> Instead of having a table in the kernel, something that would be more
> >>>> platform agnostic would be to have an extension that signals this
> >>>> information. That seems like it would accomplish the same goal and
> >>>> leverage the existing infrastructure in the kernel, albeit with the need
> >>>> to make a new extension.
> >>>>
> >>>
> >>> IMO, expecting an ISA extension to be defined for all possible
> >>> microarchitectural choices is not going to scale so it is better
> >>> to have infrastructure in kernel itself to infer microarchitectural
> >>> choices based on RISC-V implementation ID.
> >>
> >> Since adding an extension seems quite unlikely, and that a device-tree
> >> property is likely DT centric and not applicable to ACPI as well, was a
> >> command line argument considered ?
> >>
> > 
> > I did consider adding a command line option in addition to the table,
> > allowing platforms which neither have a table entry [yet] nor want to do
> > the speed test, to set whatever they like. In the end, I dropped it, since
> > I don't have a use case at this time. However, if we really don't want a
> > table, then I can look into the command line option instead.
> 
> Sorry if I wasn't clear, I wasn't considering this as a replacement for
> your table but rather as a replacement to Charlie's compile time define
> to skip misaligned speed probing since it is like "lpj=<x>". You can
> specify it on command line if you want to skip the loop time detection
> of loops per jiffies and have faster boot.

Jesse sent out a patch for a kernel parameter to set the access speed to
whatever is desired [1].

- Charlie

> -}
> -#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
> -static void __init check_unaligned_access_speed_all_cpus(void)
> -{
> -}
> -#endif
> -
>  #ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>  static void check_vector_unaligned_access(struct work_struct *work __always_unused)
>  {
> @@ -370,6 +380,11 @@ static int __init vec_check_unaligned_access_speed_all_cpus(void *unused __alway
>  }
>  #endif
>  
> +static bool check_vector_unaligned_access_table(void)
> +{
> +	return false;
> +}
> +
>  static int riscv_online_cpu_vec(unsigned int cpu)
>  {
>  	if (!has_vector()) {
> @@ -377,6 +392,9 @@ static int riscv_online_cpu_vec(unsigned int cpu)
>  		return 0;
>  	}
>  
> +	if (check_vector_unaligned_access_table())
> +		return 0;
> +
>  #ifdef CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS
>  	if (per_cpu(vector_misaligned_access, cpu) != RISCV_HWPROBE_MISALIGNED_VECTOR_UNKNOWN)
>  		return 0;
> @@ -392,13 +410,15 @@ static int __init check_unaligned_access_all_cpus(void)
>  {
>  	int cpu;
>  
> -	if (!check_unaligned_access_emulated_all_cpus())
> +	if (!check_unaligned_access_table() &&
> +	    !check_unaligned_access_emulated_all_cpus())
>  		check_unaligned_access_speed_all_cpus();
>  
>  	if (!has_vector()) {
>  		for_each_online_cpu(cpu)
>  			per_cpu(vector_misaligned_access, cpu) = RISCV_HWPROBE_MISALIGNED_VECTOR_UNSUPPORTED;
> -	} else if (!check_vector_unaligned_access_emulated_all_cpus() &&
> +	} else if (!check_vector_unaligned_access_table() &&
> +		   !check_vector_unaligned_access_emulated_all_cpus() &&
>  		   IS_ENABLED(CONFIG_RISCV_PROBE_VECTOR_UNALIGNED_ACCESS)) {
>  		kthread_run(vec_check_unaligned_access_speed_all_cpus,
>  			    NULL, "vec_check_unaligned_access_speed_all_cpus");

> 
> Regarding your table, it feels like a bit going back to old hardcoded
> platform description ;). I think some kind of auto-detection of speed
> (not builtin the kernel) for platforms could be good as well to skip
> probing.
> 
> A DT property also seems ok to me since the goal is to describe
> hardware. Would a common DT/ACPI property be appropriate ? The
> device_property API unified both so if we used some common property to
> describe the misaligned access speed (both in DT cpu node/ ACPI CPU
> device package), we could keep a single parsing method. But I'm no ACPI
> expert so I don't know if that really make sense.
> 
> Thanks,
> 
> Clément
> 
> > 
> > Thanks,
> > drew
>