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

[Yicong Yang]

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

sounds reasonable to me. will rename and use it here.

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

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.

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

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.

thanks.