[PATCH v1 2/3] riscv: errata: picoheart: Add workaround for cbo.clean errata
Yicong Yang
yang.yicong at picoheart.com
Thu Jul 16 05:20:56 PDT 2026
On 7/16/26 6:14 PM, Bo Gan wrote:
> Hi Yicong,
>
> On 7/15/26 01:47, Yicong Yang wrote:
>> cbo.clean may fail to write back the cacheline in a few very corner
>> cases on the affected Picoheart SoCs. Since cbo.flush is not affected,
>> the problem could be solved by promoting cbo.clean to cbo.flush to
>> keep the data coherent. So Implement and register Picoheart specific
>> riscv_nonstd_cache_ops for all the kernel space cache maintenance
>> operations. Implement riscv_nonstd_cache_ops->wback with cbo.clean
>> while other callbacks with corresponding CBO operations.
>>
>> This could solve the issue in the kernel space. For userspace, clear
>> the xenvcfg.CBCFE to trap the cbo.{clean, flush} and emulate
>> the cbo.clean with cbo.flush in the kernel space to avoid the issue.
>>
>> Signed-off-by: Yicong Yang <yang.yicong at picoheart.com>
>> ---
>> arch/riscv/Kconfig.errata | 12 +++
>> arch/riscv/errata/picoheart/errata.c | 144 +++++++++++++++++++++++++++
>> arch/riscv/include/asm/errata_list.h | 25 ++++-
>> arch/riscv/include/asm/insn.h | 4 +
>> arch/riscv/kernel/cpufeature.c | 3 +
>> arch/riscv/kernel/traps.c | 4 +
>> 6 files changed, 191 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
>> index 374d7fcf7936..8ce5c0314321 100644
>> --- a/arch/riscv/Kconfig.errata
>> +++ b/arch/riscv/Kconfig.errata
>> @@ -164,4 +164,16 @@ config ERRATA_PICOHEART
>> Otherwise, please say "N" here to avoid unnecessary overhead.
>> +config ERRATA_PICOHEART_CBO_CLEAN
>> + bool "Apply Picoheart cbo.clean errata"
>> + depends on ERRATA_PICOHEART
>> + select RISCV_NONSTANDARD_CACHE_OPS
>> + default y
>> + help
>> + This will apply the cbo.clean errata workaround on the affected
>> + Picoheart SoCs where cbo.clean instructions may fail to write back
>> + the cacheline in few corner cases.
>> +
>> + If you don't know what to do here, say "Y".
>> +
>> endmenu # "CPU errata selection"
>> diff --git a/arch/riscv/errata/picoheart/errata.c b/arch/riscv/errata/picoheart/errata.c
>> index 53b690878f78..9392c1634ece 100644
>> --- a/arch/riscv/errata/picoheart/errata.c
>> +++ b/arch/riscv/errata/picoheart/errata.c
>> @@ -5,20 +5,164 @@
>> * Author: Yicong Yang <yang.yicong at picoheart.com>
>> */
>> +#include <linux/align.h>
>> #include <linux/cacheflush.h>
>> #include <linux/cleanup.h>
>> +#include <linux/jump_label.h>
>> #include <linux/kernel.h>
>> #include <linux/memory.h>
>> +#include <linux/signal.h>
>> #include <linux/string.h>
>> +#include <asm/asm-extable.h>
>> #include <asm/alternative.h>
>> +#include <asm/cpufeature.h>
>> +#include <asm/dma-noncoherent.h>
>> #include <asm/errata_list.h>
>> +#include <asm/insn.h>
>> +#include <asm/insn-def.h>
>> #include <asm/text-patching.h>
>> +#include <asm/uaccess.h>
>> #include <asm/vendor_extensions.h>
>> #include <asm/vendorid_list.h>
>> +#ifdef CONFIG_ERRATA_PICOHEART_CBO_CLEAN
>> +
>> +static bool insn_is_cbo_clean_flush(u32 insn_buf)
>> +{
>> + u32 rd, opcode, funct3, funct12;
>> +
>> + /* CBO instructions don't have compressed variants */
>> + if (GET_INSN_LENGTH(insn_buf) != 4)
>> + return false;
>> +
>> + rd = RV_EXTRACT_RD_REG(insn_buf);
>> + opcode = insn_buf & __INSN_OPCODE_MASK;
>> + funct3 = insn_buf & RV_INSN_FUNCT3_MASK;
>> + funct12 = insn_buf & RV_INSN_FUNCT12_MASK;
>> +
>> + if (rd != 0 || opcode != 15 || funct3 != RV_ENCODE_FUNCT3(CBO) ||
>> + (funct12 != RV_ENCODE_FUNCT12(CBO_CLEAN) &&
>> + funct12 != RV_ENCODE_FUNCT12(CBO_FLUSH)))
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +bool riscv_picoheart_illegal_insn_handler(struct pt_regs *regs)
>> +{
>> + u32 __user *epc = (u32 __user *)regs->epc;
>> + u32 insn = (u32)regs->badaddr;
>> + void __user *line_addr;
>> + int ret = 0;
>> + u64 addr;
>> + u32 rs1;
>> +
>> + /* Do the emulation only for userspace on the sufferred platform */
>> + if (!user_mode(regs) || !riscv_cpu_has_zicbom_errata())
>> + return false;
>> +
>> + if (!insn) {
>> + if (__get_user(insn, epc))
>
> There's already the get_insn/__read_insn function in traps_misaligned.c.
> Why not use that instead?
>
it's local there and depends on CONFIG_RISCV_MISALIGNED, so I just follow
riscv_v_first_use_handler() to simply reimplement here. Currently there's
no wide use for emulation, otherwise we can extract them to a common file.
> 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)?
>> + 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.
>> +
>> + __enable_user_access();
>> + asm volatile("\n"
>> + "1: \n"
>> + CBO_FLUSH(%[addr])
>> + "2: \n"
>> + _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %[err])
>> + : [err] "+r" (ret)
>> + : [addr] "r" (untagged_addr(addr))
>> + : "memory");
>> + __disable_user_access();
>
> It's not very maintainable. It should probably reuse some macros in
> uaccess.h.
could you be more clear? the logic here is pretty simple, enable
the user access and do a cbo.flush on the target userspace
address. use kernel's fixup table mechemism to fetch errors
if target address has any access/permission issue. I cannot
find macros except __{enable, disable}_user_access() from
uaccess.h to use here.
>
>> +
>> + 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.
>
>> + return true;
>> +}
>> +
>> +#endif /* CONFIG_ERRATA_PICOHEART_CBO_CLEAN */
>> +
>> +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);
>> +}
>> +
>> +static void picoheart_errata_cache_inv(phys_addr_t paddr, size_t size)
>> +{
>> + void *vaddr = phys_to_virt(paddr);
>> +
>> + ALT_CMO_OP(INVAL, vaddr, size, riscv_cbom_block_size);
>> +}
>> +
>> +static void picoheart_errata_cache_wback_inv(phys_addr_t paddr, size_t size)
>> +{
>> + void *vaddr = phys_to_virt(paddr);
>> +
>> + ALT_CMO_OP(FLUSH, vaddr, size, riscv_cbom_block_size);
>> +}
>> +
>> +struct riscv_nonstd_cache_ops picoheart_errata_cmo_ops = {
>> + .wback = &picoheart_errata_cache_wback,
>> + .inv = &picoheart_errata_cache_inv,
>> + .wback_inv = &picoheart_errata_cache_wback_inv,
>> +};
>> +
>> +DEFINE_STATIC_KEY_FALSE(has_picoheart_cbo_clean_errata);
>> +
>> +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;
>> +
>> + if (archid != 0x804a555049544552 || impid != 0x100)
>> + return;
>> +
>> + riscv_noncoherent_supported();
>> + riscv_noncoherent_register_cache_ops(&picoheart_errata_cmo_ops);
>> + static_branch_enable(&has_picoheart_cbo_clean_errata);
>> +}
>> +
>> static u32 picoheart_errata_probe(unsigned int stage, unsigned long archid,
>> unsigned long impid)
>> {
>> + picoheart_errata_probe_cbo_clean(stage, archid, impid);
>> return 0;
>> }
>> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
>> index 6694b5ccdcf8..2b786decfa39 100644
>> --- a/arch/riscv/include/asm/errata_list.h
>> +++ b/arch/riscv/include/asm/errata_list.h
>> @@ -117,6 +117,29 @@ asm volatile(ALTERNATIVE( \
>> #define THEAD_C9XX_RV_IRQ_PMU 17
>> #define THEAD_C9XX_CSR_SCOUNTEROF 0x5c5
>> -#endif /* __ASSEMBLER__ */
>> +#ifdef CONFIG_ERRATA_PICOHEART_CBO_CLEAN
>> +
>> +#include <linux/jump_label.h>
>> +
>> +DECLARE_STATIC_KEY_FALSE(has_picoheart_cbo_clean_errata);
>> +
>> +static inline bool riscv_cpu_has_zicbom_errata(void)
>> +{
>> + return static_branch_unlikely(&has_picoheart_cbo_clean_errata);
>> +}
>> +
>> +bool riscv_picoheart_illegal_insn_handler(struct pt_regs *regs);
>> +
>> +#else /* !CONFIG_ERRATA_PICOHEART_CBO_CLEAN */
>> +
>> +static inline bool riscv_cpu_has_zicbom_errata(void) { return false; }
>> +static inline bool riscv_picoheart_illegal_insn_handler(struct pt_regs *regs)
>> +{
>> + return false;
>> +}
>> +
>> +#endif /* CONFIG_ERRATA_PICOHEART_CBO_CLEAN */
>> +
>> +#endif /* __ASSEMBLY__ */
>> #endif
>> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
>> index c3005573e8c9..5b7c0e6fabbd 100644
>> --- a/arch/riscv/include/asm/insn.h
>> +++ b/arch/riscv/include/asm/insn.h
>> @@ -12,6 +12,7 @@
>> #define RV_INSN_FUNCT3_OPOFF 12
>> #define RV_INSN_OPCODE_MASK GENMASK(6, 0)
>> #define RV_INSN_OPCODE_OPOFF 0
>> +#define RV_INSN_FUNCT12_MASK GENMASK(31, 20)
>> #define RV_INSN_FUNCT12_OPOFF 20
>> #define RV_ENCODE_FUNCT3(f_) (RVG_FUNCT3_##f_ << RV_INSN_FUNCT3_OPOFF)
>> @@ -171,6 +172,7 @@
>> #define RVG_FUNCT3_JALR 0x0
>> #define RVG_FUNCT3_BEQ 0x0
>> #define RVG_FUNCT3_BNE 0x1
>> +#define RVG_FUNCT3_CBO 0x2
>> #define RVG_FUNCT3_BLT 0x4
>> #define RVG_FUNCT3_BGE 0x5
>> #define RVG_FUNCT3_BLTU 0x6
>> @@ -186,6 +188,8 @@
>> #define RVC_FUNCT4_C_EBREAK 0x9
>> #define RVG_FUNCT12_EBREAK 0x1
>> +#define RVG_FUNCT12_CBO_CLEAN 0x1
>> +#define RVG_FUNCT12_CBO_FLUSH 0x2
>> #define RVG_FUNCT12_SRET 0x102
>> #define RVG_MATCH_AUIPC (RVG_OPCODE_AUIPC)
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 1fe647e03515..11ae0f00cc82 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -1208,6 +1208,9 @@ void __init riscv_user_isa_enable(void)
>> if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOP) &&
>> any_cpu_has_zicbop)
>> pr_warn("Zicbop disabled as it is unavailable on some harts\n");
>> +
>> + if (riscv_cpu_has_zicbom_errata())
>> + current->thread.envcfg &= ~ENVCFG_CBCFE;
>> }
>> #ifdef CONFIG_RISCV_ALTERNATIVE
>> 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).
>> 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? If it does not delegate the exception and do
the trap-n-emulation, then the kernel code won't run. but if it doesn't, the
kernel need to do the workaround to keep the function correct.
For this trap and emulation way, both kernel and openSBI are inefficient and
have no significant difference.
thanks.
More information about the linux-riscv
mailing list