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

[Yicong Yang]

On 7/17/26 11:13 AM, Vivian Wang wrote:
> 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.

it's not enough, we also need to dump the context if exception-trace
is enabled. that's exactly the **only** two things do_trap() wrapped:
- dump the context if exception-trace enabled
- force_sig_fault()

> 
>> [...]
>>
>>>> +
>>>> +    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.
> 

it's okay for me if you mean to, it looked a bit fragment for me.
actually there's only a jalr difference as the riscv_cpu_has_zicbom_errata()
is checked once entering the riscv_picoheart_illegal_insn_handler().

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

I feel the same. the illegal trap handling should never on any performance
critical 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.

it's not about the correctness but the policy of the firmware. the kernel
cannot know whether the exception is delegated or handled by the firmware,
it's just transparent to the kernel...

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

...so the solution is clear:
- if the firmware decide to handle the illegal instruction, it's responsible
  for handling the emulation (for the errata). otherwise it's a bug of the
  firmware.
- or if the exception is delegated, the kernel will handle it.

so a handler in the kernel is always needed. it won't run in the former
policy but necessary in the latter one.

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

above you mentioned is another policy of the firmware that is transparent
to the kernel. if menvcfg.CBCFE is cleared, all the trap-n-emulate stuffs
are handled by the firmware, but it'll have performance issue as you also
mentioned. So our policy is trap-n-emulate the userspace instruction (either
in kernel or firmware according to the delegation policy) and execute
the instruction directly in the kernel space, as mentioned in the commit.

thanks.