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

[Vivian Wang]

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?

>> 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
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.

It's not as much a lines-of-code reduction as I can hope because there's
still the emulation stuff, of course... For that, I'm also looking
forward to Bo Gan's thoughts on that in the other thread.

Vivian "dramforever" Wang