Message-ID: <3da77593-bc22-46e6-9cff-dd7f8eb02cd9@rivosinc.com>
Date: Wed, 3 Jul 2024 17:47:56 -0400
From: Jesse Taube <jesse@...osinc.com>
To: Clément Léger <cleger@...osinc.com>,
 Charlie Jenkins <charlie@...osinc.com>
Cc: Conor Dooley <conor@...nel.org>, linux-riscv@...ts.infradead.org,
 Jonathan Corbet <corbet@....net>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Evan Green <evan@...osinc.com>, Andrew Jones <ajones@...tanamicro.com>,
 Xiao Wang <xiao.w.wang@...el.com>, Andy Chiu <andy.chiu@...ive.com>,
 Eric Biggers <ebiggers@...gle.com>, Greentime Hu <greentime.hu@...ive.com>,
 Björn Töpel <bjorn@...osinc.com>,
 Heiko Stuebner <heiko@...ech.de>, Costa Shulyupin <costa.shul@...hat.com>,
 Andrew Morton <akpm@...ux-foundation.org>, Baoquan He <bhe@...hat.com>,
 Anup Patel <apatel@...tanamicro.com>, Zong Li <zong.li@...ive.com>,
 Sami Tolvanen <samitolvanen@...gle.com>,
 Ben Dooks <ben.dooks@...ethink.co.uk>,
 Alexandre Ghiti <alexghiti@...osinc.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 Erick Archer <erick.archer@....com>, Joel Granados <j.granados@...sung.com>,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v3 4/8] RISC-V: Check Zicclsm to set unaligned access
 speed



On 7/3/24 03:13, Clément Léger wrote:
> 
> 
> On 03/07/2024 00:22, Charlie Jenkins wrote:
>> On Mon, Jul 01, 2024 at 04:20:15PM +0200, Clément Léger wrote:
>>>
>>>
>>> On 01/07/2024 15:58, Conor Dooley wrote:
>>>> On Mon, Jul 01, 2024 at 09:15:09AM +0200, Clément Léger wrote:
>>>>>
>>>>>
>>>>> On 27/06/2024 23:20, Charlie Jenkins wrote:
>>>>>> On Wed, Jun 26, 2024 at 03:39:14PM +0100, Conor Dooley wrote:
>>>>>>> On Mon, Jun 24, 2024 at 08:49:57PM -0400, Jesse Taube wrote:
>>>>>>>> Check for Zicclsm before checking for unaligned access speed. This will
>>>>>>>> greatly reduce the boot up time as finding the access speed is no longer
>>>>>>>> necessary.
>>>>>>>>
>>>>>>>> Signed-off-by: Jesse Taube <jesse@...osinc.com>
>>>>>>>> ---
>>>>>>>> V2 -> V3:
>>>>>>>>   - New patch split from previous patch
>>>>>>>> ---
>>>>>>>>   arch/riscv/kernel/unaligned_access_speed.c | 26 ++++++++++++++--------
>>>>>>>>   1 file changed, 17 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/kernel/unaligned_access_speed.c b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> index a9a6bcb02acf..329fd289b5c8 100644
>>>>>>>> --- a/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> +++ b/arch/riscv/kernel/unaligned_access_speed.c
>>>>>>>> @@ -259,23 +259,31 @@ static int check_unaligned_access_speed_all_cpus(void)
>>>>>>>>   	kfree(bufs);
>>>>>>>>   	return 0;
>>>>>>>>   }
>>>>>>>> +#else /* CONFIG_RISCV_PROBE_UNALIGNED_ACCESS */
>>>>>>>> +static int check_unaligned_access_speed_all_cpus(void)
>>>>>>>> +{
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +#endif
>>>>>>>>   
>>>>>>>>   static int check_unaligned_access_all_cpus(void)
>>>>>>>>   {
>>>>>>>> -	bool all_cpus_emulated = check_unaligned_access_emulated_all_cpus();
>>>>>>>> +	bool all_cpus_emulated;
>>>>>>>> +	int cpu;
>>>>>>>> +
>>>>>>>> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICCLSM)) {
>>>>>>>> +		for_each_online_cpu(cpu) {
>>>>>>>> +			per_cpu(misaligned_access_speed, cpu) = RISCV_HWPROBE_MISALIGNED_FAST;
>>>>>>>
>>>>>>> - const: zicclsm
>>>>>>>    description:
>>>>>>>      The standard Zicclsm extension for misaligned support for all regular
>>>>>>>      load and store instructions (including scalar and vector) but not AMOs
>>>>>>>      or other specialized forms of memory access. Defined in the
>>>>>>>      RISC-V RVA Profiles Specification.
>>>>>>>
>>>>>>> Doesn't, unfortunately, say anywhere there that they're actually fast :(
>>>>>>
>>>>>> Oh no! That is unfortunate that the ISA does not explicitly call that
>>>>>> out, but I think that acceptable.
>>>>>>
>>>>>> If a vendor puts Zicclsm in their isa string, they should expect
>>>>>> software to take advantage of misaligned accesses. FAST is our signal to
>>>>>> tell software that they should emit misaligned accesses.
>>>>>
>>>>> AFAIK, Zicclsm is not even an ISA extension, simply a profile
>>>>> specification which means that only the execution environment which
>>>>> provides the profile support misaligned accesses (cf
>>>>> https://lists.riscv.org/g/tech-profiles/message/56).
>>>>
>>>> I dunno, the specification status page used to describe it as an
>>>> extension:
>>>> https://wiki.riscv.org/display/HOME/Specification+Status+-+Historical
>>>> My understanding was that these could be considered extensions, just
>>>> like we are considering svade to be one.
>>>>
>>>>> . I don't think we
>>>>> can extrapolate that the misaligned accesses will be fast at all.
>>>>
>>>> That is my opinion on it too. If it doesn't say "fast" and give a
>>>> definition for what that means in the binding, then we can't assume that
>>>> it is fast. I'm also wary of extending definitions of extensions in the
>>>> binding, because a) I am 90% sure that people writing devicetrees don't
>>>> care and b) it'd be a potential difference between DT and ACPI without a
>>>> real justification (unlike the zkr or svade/svadu situations).
>>>
>>> BTW, the profile spec [1] has a note that states the following for Zicclsm:
>>>
>>> "Even though mandated, misaligned loads and stores might execute
>>> extremely slowly. Standard software distributions should assume their
>>> existence only for correctness, not for performance."
>>>
>>> Which was also quoted in patch 1, so I guess that settles it.
>>
>> The intention here was to allow vendors to configure an option to skip
>> the probing. This extension does not seem useful as it is written! A way
>> around this would be to add a kernel arg to set the access speed but
>> maybe it doesn't matter. For the sake of this patch, it looks like we
>> should get rid of this Zicclsm check.
> 
> I think a parameter could be appropriate for vendors that want to skip
> the probing and gain a bit of time on boot time. Other options already
> exists to force specific settings so, why not !

Sounds like a good solution thanks for the idea Charlie!
I will drop this patch and send a separate patch to implement two kernel 
parameters for skipping(and setting the speed of) vector and scalar 
speed tests.

Thanks,
Jesse Taube

> 
> Thanks,
> 
> Clément
> 
>>
>> - Charlie
>>
>>>
>>> Thanks,
>>>
>>> Clément
>>>
>>> Link:
>>> https://github.com/riscv/riscv-profiles/blob/main/src/profiles.adoc?plain=1#L524
>>> [1]
>>>
>>>>
>>>>>> This allows for a generic kernel, like the one a distro would compile, to
>>>>>> skip the probing when booting on a system that explicitly called out
>>>>>> that the hardware supports misaligned accesses.

Powered by blists - more mailing lists