[PATCH v1 2/3] riscv: errata: picoheart: Add workaround for cbo.clean errata

[Yicong Yang]

On 7/17/26 4:06 PM, Vivian Wang wrote:
> On 7/17/26 13:58, Yicong Yang wrote:
> 
>> On 7/17/26 10:44 AM, Vivian Wang wrote:
>>> On 7/15/26 16:47, Yicong Yang wrote:
>>> [...]
>>>> +
>>>> +	/* Do the emulation only for userspace on the sufferred platform */
>>>> +	if (!user_mode(regs) || !riscv_cpu_has_zicbom_errata())
>>> user_mode(regs) is already checked below in the handler right? I think
>>> checking again here is unnecessary.
>> maybe not (or I miss somewhere)?
>>
>> but the check here is to make sure we can bail out early for non-errata
>> cases to avoid running unnecessary stuffs.
> 
> It's only called in do_trap_insn_illegal() right? In the
> if(user_mode(regs)) branch
> 
>     do_trap_insn_illegal()
>         if (user_mode(regs))
>             riscv_v_first_use_handler()
>             if (!handled) riscv_picoheart_illegal_insn_handler()
>             if (!handled) do_trap_error()
> 
> Did I miss something?

okay, you mean the caller then it's right.

you asked "below in the handler" so I thought you mean somewhere implicit
in this riscv_picoheart_illegal_insn_handler.

> 
>>> Also, I'd rather riscv_cpu_has_zicbom_errata() to have the word
>>> "picoheart" in it, because "clean doesn't work, please flush instead" is
>>> still rather specific. We could always rename this later.
>>>
>>>> [...]
>>>>
>>>> +static void picoheart_errata_cache_wback(phys_addr_t paddr, size_t size)
>>>> +{
>>>> +	void *vaddr = phys_to_virt(paddr);
>>>> +
>>>> +	ALT_CMO_OP(FLUSH, vaddr, size, riscv_cbom_block_size);
>>> Do we really need this? ...
>> yes, this one line macro will make the code clear and simple. we
>> only need to handling the logic of ourself (replace cbo.clean with
>> cbo.flush) without caring about the underlying assemble and configs...
>>
>>>> +}
>>>>
>>>> [...]
>>>>
>>>> +static void picoheart_errata_probe_cbo_clean(unsigned int stage,
>>>> +					     unsigned long archid,
>>>> +					     unsigned long impid)
>>>> +{
>>>> +	if (!IS_ENABLED(CONFIG_ERRATA_PICOHEART_CBO_CLEAN))
>>>> +		return;
>>>> +
>>>> +	if (stage != RISCV_ALTERNATIVES_BOOT)
>>>> +		return;
>>>> +
>>>> +	if (!riscv_isa_extension_available(NULL, ZICBOM))
>>>> +		return;
>>> ... and this? Are there Picoheart CPUs with this specific archid or
>>> impid that *doesn't* support Zicbom? Or systems with firmware that
>>> doesn't enable Zicbom?
>> it's sanity to have this check as this errata is for Zicbom, in
>> case a certain version of firmware doesn't report Zicbom in any
>> unknown reasons.
> 
> Okay, I get what you mean. You didn't write ALT_CMO_OP because you
> wanted to double check Zicbom support in the actual ops. You wrote that
> because that's just the macro with cbo.flush in it.
> 
> So you only need to have the wback function, and inv and wback can be
> left as zero initialized. In that case arch_dma_cache_*() in

benefit?

I don't have strong feelings for either way but I feel this implicit
assignment will make the code a bit less readable and not that
self-explained. Considering only ~10 more LOCs if implement all
these three callbacks...

> arch/riscv/mm/dma-noncoherent.c will just do the standard operation. See
> e.g.:
> 
>     static inline void arch_dma_cache_inv(phys_addr_t paddr, size_t size)
>     {
>     	void *vaddr = phys_to_virt(paddr);
> 
>     #ifdef CONFIG_RISCV_NONSTANDARD_CACHE_OPS
>     	if (unlikely(noncoherent_cache_ops.inv)) {         // <-- HERE
>     		noncoherent_cache_ops.inv(paddr, size);
>     		return;
>     	}
>     #endif
> 
>     	ALT_CMO_OP(INVAL, vaddr, size, riscv_cbom_block_size);
>     }
> 
> And after you've done that, consider this somewhat out of left field
> suggestion: Move the static inline void arch_dma_cache_*() functions to
> dma-noncoherent.h and here just define the nonstandard cache ops here as:
> 
>     struct riscv_nonstd_cache_ops picoheart_errata_cmo_ops = {
>     	.wback = arch_dma_cache_wback_inv
>     };
> 
> It's somewhat weirdly recursive but in a way that I think makes sense:
> It's saying "Instead of wback, do a wback_inv" (Inform 7, anyone? :P).
> Perf-wise this has one extra cache-hot load for the NULL wback_inv (hot
> since it's *right* next to the wback function pointer just loaded), and
> one predictably non-taken branch, and in exchange you get to just
> completely drop all of your own implementations.
> 

But I'll just implement the custom callback directly for the readability
here (as you also find it weirdly, and probably more person will have
doubts here).

thanks.