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

[Alexandre Ghiti <alex@ghiti.fr>]

Hi Djordje,

On 19/03/2025 13:28, Djordje Todorovic wrote:
> HTEC Public
>
> Hi everyone,
>
> Thank you a lot for your comments.
> I am wondering how we should proceed from here.


I read the previous comments and everyone seems to agree that an 
implementation using an alternative is the way to go, so can you come 
with a new version doing that?

Thanks,

Alex


>
> Best,
> Djordje
>
> ________________________________________
> From: Djordje Todorovic <Djordje.Todorovic@htecgroup.com>
> Sent: Wednesday, February 12, 2025 11:22 AM
> To: Palmer Dabbelt <palmer@dabbelt.com>; jrtc27@jrtc27.com <jrtc27@jrtc27.com>
> Cc: Charlie Jenkins <charlie@rivosinc.com>; samuel.holland@sifive.com <samuel.holland@sifive.com>; Conor Dooley <conor@kernel.org>; arikalo@gmail.com <arikalo@gmail.com>; linux-riscv@lists.infradead.org <linux-riscv@lists.infradead.org>; Paul Walmsley <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu <aou@eecs.berkeley.edu>; Will Deacon <will@kernel.org>; peterz@infradead.org <peterz@infradead.org>; boqun.feng@gmail.com <boqun.feng@gmail.com>; Mark Rutland <mark.rutland@arm.com>; yury.norov@gmail.com <yury.norov@gmail.com>; linux@rasmusvillemoes.dk <linux@rasmusvillemoes.dk>; parri.andrea@gmail.com <parri.andrea@gmail.com>; leobras@redhat.com <leobras@redhat.com>; guoren@kernel.org <guoren@kernel.org>; ericchancf@google.com <ericchancf@google.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH v2] riscv: Use Zalrsc extension to implement atomic functions
>
> Hi,
>
> The official product page is located here:
> https://mips.com/products/hardware/p8700
>
> The mechanism we use to handle AMO is described in:
> "7.3.10 MIPS Configuration 6 Register (mipsconfig6)"
>
> Best,
> Djordje
>
> ________________________________________
> From: Palmer Dabbelt <palmer@dabbelt.com>
> Sent: Wednesday, February 5, 2025 4:49 PM
> To: jrtc27@jrtc27.com <jrtc27@jrtc27.com>
> Cc: Charlie Jenkins <charlie@rivosinc.com>; samuel.holland@sifive.com <samuel.holland@sifive.com>; Conor Dooley <conor@kernel.org>; arikalo@gmail.com <arikalo@gmail.com>; linux-riscv@lists.infradead.org <linux-riscv@lists.infradead.org>; Paul Walmsley <paul.walmsley@sifive.com>; aou@eecs.berkeley.edu <aou@eecs.berkeley.edu>; Will Deacon <will@kernel.org>; peterz@infradead.org <peterz@infradead.org>; boqun.feng@gmail.com <boqun.feng@gmail.com>; Mark Rutland <mark.rutland@arm.com>; yury.norov@gmail.com <yury.norov@gmail.com>; linux@rasmusvillemoes.dk <linux@rasmusvillemoes.dk>; parri.andrea@gmail.com <parri.andrea@gmail.com>; leobras@redhat.com <leobras@redhat.com>; guoren@kernel.org <guoren@kernel.org>; ericchancf@google.com <ericchancf@google.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Djordje Todorovic <djordje.todorovic@htecgroup.com>
> Subject: Re: [PATCH v2] riscv: Use Zalrsc extension to implement atomic functions
>
> [You don't often get email from palmer@dabbelt.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On Mon, 03 Feb 2025 13:34:48 PST (-0800), jrtc27@jrtc27.com wrote:
>> On 3 Feb 2025, at 19:50, Charlie Jenkins <charlie@rivosinc.com> 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@rivosinc.com> wrote:
>>>>>>>
>>>>>>>>> From: Chao-ying Fu <cfu@mips.com>
>>>>>>>>>
>>>>>>>>> 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...
> So then I think we need some help from the HW vendor here.
> Specifically: do these systems implement A via M-mode traps (ie, with a
> performance penalty) or is there something functional erratum in the A
> implementation.
>
> If it's just a performance thing then we need some benchmark justifying
> the extra work, which means some hardware we can point at to run the
> benchmark.  Probably also best to shim this in via alternatives, so we
> can keep multi-vendor kernels working.
>
> If it's a funtional issue then we need to know what's actually broken.
>
> Either way this should be exposed to userspace.
>
>> Jess
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv