Re: [PATCH v2] riscv: Use Zalrsc extension to implement atomic functions

[Jessica Clarke]

On 3 Feb 2025, at 19:50, Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> 
> On Mon, Feb 03, 2025 at 01:30:48PM -0600, Samuel Holland wrote:
>> Hi Charlie,
>> 
>> On 2025-02-03 1:12 PM, Charlie Jenkins wrote:
>>> On Sun, Feb 02, 2025 at 08:08:50PM +0000, Conor Dooley wrote:
>>>> On Sat, Feb 01, 2025 at 01:04:25PM +0100, Aleksandar Rikalo wrote:
>>>>> On Fri, Jan 10, 2025 at 4:23 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
>>>>> 
>>>>>>> From: Chao-ying Fu <cfu@xxxxxxxx>
>>>>>>> 
>>>>>>> Use only LR/SC instructions to implement atomic functions.
>>>>>> 
>>>>>> In the previous patch you mention that this is to support MIPS P8700. Can
>>>>>> you expand on why this change is required? The datasheet at [1] says:
>>>>>> 
>>>>>> "The P8700 core is configured to support the RV64GCZba_Zbb (G = IMAFD)
>>>>>> Standard ISA. It includes the RV64I base ISA, Multiply (M), Atomic (A),
>>>>>> Single-Precision Floating Point (F), Double (D), Compressed (C) RISC-V
>>>>>> extensions, as well as the as well as the bit-manipulation extensions
>>>>>> (Zba) and (Zbb)"
>>>>>> 
>>>>>> The "A" extension is a part of "G" which is mostly assumed to exist in
>>>>>> the kernel. Additionally, having this be a compilation flag will cause
>>>>>> traps on generic kernels. We generally try to push everything we can
>>>>>> into runtime feature detection since there are so many possible variants
>>>>>> of riscv.
>>>>>> 
>>>>>> Expressing not being able to perform a feature like this is normally
>>>>>> better expressed as an errata. Then generic kernels will be able to
>>>>>> include this, and anybody who doesn't want to have the extra nops
>>>>>> introduced can disable the errata. A similar approach to what I pointed
>>>>>> out last time should work here too (but with more places to replace)
>>>>>> [2].
>>>>>> 
>>>>>> [1] https://mips.com/wp-content/uploads/2024/11/P8700_Data_Sheet.pdf
>>>>>> [2] https://lore.kernel.org/lkml/Z2-UNfwcAQYZqVBU@ghost/T/
>>>>> 
>>>>> So far we haven't found a way to do this using errata.
>>>> 
>>>> You mean using alternatives? Not implementing A, but instead
>>>> implementing Zalrsc, is not an erratum. It's a design decision.
>>> 
>>> We could do the same thing we do with misaligned access detection and
>>> run some instructions to determine if these instructions are being
>>> emulated.  If they are being emulated, patch all of the places to use
>>> zalrsc.
>> 
>> Is the implication here that the riscv,isa-extensions list passed to the kernel
>> will contain "a", even though the hardware does not support it, because AMOs are
>> emulated in M-mode?
>> 
>> If that is not the case, there is no need for runtime detection. The alternative
>> entry can check RISCV_ISA_EXT_ZAAMO (which would be implied by RISCV_ISA_EXT_a)
>> in the ISA bitmap like normal.
> 
> That would be much better! I was under the assumption that the usecase
> for this patch was that they were passing in "a" and wanting to only get
> zalrsc. We should be able to check
> RISCV_ISA_EXT_ZAAMO/RISCV_ISA_EXT_ZALRSC to get the information without
> runtime detection.

In LLVM at least -mcpu=mips-p8700 enables the full A extension...

Jess