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

[Vivian Wang]

On 7/15/26 16: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.
>
> [...]
>
> 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) ||

This opcode != 15 could be opcode != RVG_OPCODE_FENCE.

Well... I'm not sure how we should deal with the fact that it's called
"FENCE". I'm more inclined to rename it to RVG_OPCODE_MISC_MEM because
that's what it's supposed to be.

> +	    (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())

user_mode(regs) is already checked below in the handler right? I think
checking again here is unnecessary.

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

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

Vivian "dramforever" Wang