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

["Clément Léger" <cleger@rivosinc.com>]

On 10/02/2025 21:53, Charlie Jenkins wrote:
> On Mon, Feb 10, 2025 at 09:42:25PM +0100, Clément Léger wrote:
>>
>>
>> On 10/02/2025 18:20, Charlie Jenkins wrote:
>>> 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@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].
>>
>> Hey Charlie,
>>
>> Thanks but it seems you forgot to add the link ?
> 
> Oops, I frequently do that...
> 
> https://lore.kernel.org/linux-riscv/20240805173816.3722002-1-jesse@rivosinc.com/
> 
>>
>> Having configuration option + command line option seems like something
>> particularly heavy for such feature. The ifdefery/config options
>> involved in the misaligned probing code is already quite complicated. If
>> another mean to specify the misaligned speed access is added, I think
>> all configuration options to set the speed of accesses can then be
>> removed and just keep the command line. That will certainly simplify the
>> ifdef/config options.
> 
> Yeah that's why it didn't get merged because it felt like overkill. I
> responded on the thread to Anup as why I would prefer config options. It
> just comes down to config options being required to enable compiler
> features. The kernel is only built with rv64gc and usage of all other
> extensions requires hand written assembly. There are easy performance
> gains when compiling the kernel with rv64gc_zba_zbb_zbkb etc.
> Performance focused kernels will need to be recompiled anyway so I am of
> the opinion that grouping in other performance features as config
> options like this is the easiest thing to do and reduces the amount of
> code in the kernel.

As answered on the other thread, totally agree, except for the
misaligned accesses probing config options ;). Ultimately, we need
profiles configuration, either via defconfigs that enables a bunch of
optimization via ISA extension or configuration options that groups
these config options.

Clément

> 
> - Charlie
> 
>>
>> Clément
>>
>>>
>>> - 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
>>>>
>>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv