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

[Vivian Wang]

On 7/16/26 20:20, Yicong Yang wrote:
> On 7/16/26 6:14 PM, Bo Gan wrote:
>>> [...]
>>>
>> BTW, I just found that __get_user is not correct for fetching insns from
>> user. It doesn't set the MXR bit, and it'll fail if the userspace maps
>> the text page executable, but not readable.
>>
> good point. this maybe a fair reason for a commmon get_insn() for handling
> the insn read. but any case that the user would map the text as exec-only
> (considering we didn't find this from existing emulation case)?
>
The fact that userspace *can* do this is enough of a use case - it makes
it a correctness issue. The fact that it is not used in normal software
is not really a point against fixing a correctness issue.

But yes, this otherwise is an existing issue with this code pattern.

>>> +            return false;
>>> +    }
>>> +
>>> +    if (!insn_is_cbo_clean_flush(insn))
>>> +        return false;
>>> +
>>> +    rs1 = RV_EXTRACT_RS1_REG(insn);
>>> +    addr = rs1 ? ((unsigned long *)regs)[rs1] : 0;
>>> +    line_addr = (void __user *)ALIGN_DOWN(addr, riscv_cbom_block_size);
>>> +
>>> +    if (!access_ok(line_addr, riscv_cbom_block_size)) {
>>> +        current->thread.bad_cause = EXC_STORE_PAGE_FAULT;
>>> +        goto err_map;
>>> +    }
>> I feel this needs to be reworked if we really want to handle insn fault
>> this way. It should not touch current->thread.bad_cause or directly call
>> do_trap. Rather, follow the pattern of riscv_v_first_use_handler.
>>
> see the comment under err_map: below. for invalid address we need
> to emulate it as page/access fault (SIGSEV) rather than illegal
> instruction (SIGILL) that lead us here, thu need to update
> the ->bad_cause to match the SIGSEV.
>
> it's not comparable to riscv_v_first_use_handler which can simply
> keep the illegal exception on failure.

force_sig_fault(SIGSEGV, ...)

This handler only applies to user mode, so you can get away with stuff
like this.

> [...]
>
>>> +
>>> +    if (ret)
>>> +        goto err_map;
>>> +
>>> +    regs->epc += GET_INSN_LENGTH(insn);
>>> +    return true;
>>> +
>>> +err_map:
>>> +    /*
>>> +     * We'll reach here if the target address is invalid.
>>> +     * cbo.{flush ,clean} on invalid address will cause
>>> +     * store page fault, update the cause and badaddr.
>>> +     */
>>> +    regs->badaddr = addr;
>>> +    regs->cause = current->thread.bad_cause;
>>> +    do_trap(regs, SIGSEGV, SEGV_MAPERR, addr);
>> See comment above.
>>
>>> [...]
>>>
>>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>>> index 8c62c771a656..9cd21073babc 100644
>>> --- a/arch/riscv/kernel/traps.c
>>> +++ b/arch/riscv/kernel/traps.c
>>> @@ -177,6 +177,10 @@ asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *re
>>>           local_irq_enable();
>>>             handled = riscv_v_first_use_handler(regs);
>>> +
>>> +        if (!handled)
>>> +            handled = riscv_picoheart_illegal_insn_handler(regs);
>>> +
>> I don't think you should hack the common insn fault handler below and call
>> your handler function. At least you must ensure this is really a no-op on
>> other arch/platforms.
>>
> this is a nop if !CONFIG_ERRATA_PICOHEART_CBO_CLEAN and on unaffected platforms
> (toggled by static key has_picoheart_cbo_clean_errata).

I think what Bo might be going for is if (!handled &&
riscv_cpu_has_zicbom_errata()) to minimize the overhead on unaffected
platforms, especially since riscv_picoheart_illegal_insn_handler() is
and should be compiled out-of-line.

Aside from potentially having this run faster, done this way it's also
more obvious that this does nothing on non-affected platforms.

Honestly I'm not *too* concerned about performance in what's destined to
be the slow path.

>>>           if (!handled)
>>>               do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc,
>>>                         "Oops - illegal instruction");
>> Have you considered putting this trap-n-emulate logic in the SBI firmware
>> such as OpenSBI, which I'd assume you are using? In OpenSBI, it's already
>> handling illegal instruction fault, and forwarding to OS if not known. Why
>> not handle it there? It'll probably be more efficient and, better, you can
>> support unpatched Linux and userspace, and even in Guest VM. Still, your
>> picoheart_errata_cmo_ops is required, to avoid the emulation overhead for
>> kernel cbo.clean as much as possible.
> we cannot assume that the underlying openSBI doesn't delegate the illegal
> instruction exception, right? [...]

In general, this is not how Linux does things... Linux sees all the
firmware and hardware below as an entire thing. It's what RISC-V
describes as the "Supervisor Execution Environment" ("SEE"). So, *in
general* we should absolutely assume that the underlying SEE implements
things correctly if at all makes sense.

Otherwise, this never ends - why can we assume that the underlying
firmware is not just buggy? Why can we assume that the hardware runs the
add instruction correctly? It's the Pandora's box of things we "can't
assume". There's no need to open that box - we can just patch up what's
leaking out where it happens.

*However*, there's a problem for emulating cbo.clean in software in
M-mode firmware: There's no separate menvcfg.CBCE controlling cbo.clean
and not cbo.flush. There's only a menvcfg.CBCFE. So having M-mode FW
clear menvcfg.CBCFE actually *precludes* S-mode having efficient cbo.clean.

For maximum correctness, one might have M-mode firmware clear
menvcfg.CBCFE by default, emulate both as cbo.flush, and have a FWFT
vendor feature for S-mode software to declare "I know to only use
cbo.flush and not cbo.clean. Please give me the raw speed of cbo.flush."
but I'm worried that that's *way* overcomplicating things.

* Bo: Thoughts on this? What approach do you think would make sense,
given that you can't actually control cbo.flush and cbo.clean
separately? Or maybe I'm just mistaken about how this works?

Vivian "dramforever" Wang