Re: [PATCH] riscv: Add ERRATA_MIPS_P8700_WFI to replace WFI with mips.pause

[Paul Walmsley <pjw@kernel.org>]

Hi,

On Mon, 11 May 2026, Aleksa Paunovic via B4 Relay wrote:

> From: Djordje Todorovic <djordje.todorovic@htecgroup.com>
> 
> The MIPS P8700 has bugs with the WFI instruction. 

Has MIPS published an erratum for this?   If so, could you add a link to 
it in the patch description?  If not, could you please briefly describe 
the impact of the bug in the patch description, when WFI is used?

> This errata uses the RISC-V alternatives framework to patch all WFI 
> instructions with the MIPS P8700 pause opcode (0x00501013) at runtime 
> when running on P8700 hardware.
> 
> Two call sites are patched:
>  - arch/riscv/kernel/head.S: secondary hart parking loop
>  - arch/riscv/include/asm/processor.h: wait_for_interrupt()
> 
> Signed-off-by: Djordje Todorovic <djordje.todorovic@htecgroup.com>
> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> ---
> This patch was tested on QEMU configured with eight P8700 harts,
> as well as on the MIPS Boston board, configured with a single P8700 CPU.
> Errata application was tested by disassembling with GDB on QEMU
> and inserting an illegal instruction on the Boston board.
> Correctness was tested with a combination of kselftests
> and torture tests (rcu, locktorture), along with coremark testing.

This kind of test report is very helpful!  Thank you.

> ---
>  arch/riscv/Kconfig.errata                    | 11 +++++++++++
>  arch/riscv/errata/mips/errata.c              | 14 ++++++++++++++
>  arch/riscv/include/asm/errata_list.h         | 27 +++++++++++++++++++++++++++
>  arch/riscv/include/asm/errata_list_vendors.h |  5 +++--
>  arch/riscv/include/asm/processor.h           |  3 ++-
>  arch/riscv/kernel/head.S                     |  4 +++-
>  6 files changed, 60 insertions(+), 4 deletions(-)
> 

[ ... ]

> diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
> index 6694b5ccdcf85cfe7e767ea4de981b34f2b17b04..cbe90b19556203e1462cfb345b164c9061887e74 100644
> --- a/arch/riscv/include/asm/errata_list.h
> +++ b/arch/riscv/include/asm/errata_list.h
> @@ -25,6 +25,16 @@ ALTERNATIVE(__stringify(RISCV_PTR do_page_fault),			\
>  	    __stringify(RISCV_PTR sifive_cip_453_page_fault_trp),	\
>  	    SIFIVE_VENDOR_ID, ERRATA_SIFIVE_CIP_453,			\
>  	    CONFIG_ERRATA_SIFIVE_CIP_453)
> +
> +#ifdef CONFIG_ERRATA_MIPS_P8700_WFI
> +#define ALT_WFI								\
> +ALTERNATIVE("wfi; .rept 7; nop; .endr;",				\
> +	".rept 8; .insn 0x00501013; .endr;", MIPS_VENDOR_ID,		\
> +	ERRATA_MIPS_P8700_WFI, CONFIG_ERRATA_MIPS_P8700_WFI)

.insn only works on newer toolchains/binutils versions - this is why 
CONFIG_AS_HAS_INSN was added.  See for example commit 
dc20452e6caf962f04ede7f364267b0c37784ab4 ("riscv: Fix CONFIG_AS_HAS_INSN 
for new .insn usage").  Can you please fix this?

Please add some sort of code comment to describe why 8 MIPS PAUSE 
instructions are needed, as opposed to a single MIPS PAUSE instruction.  
An event that causes the first PAUSE to exit would then encounter the 
subsequent PAUSEs, at which point, there could be additional latency 
before the core can return to doing real work.  The P8700 Programmer's 
Guide mentions a maximum number of cycles per PAUSE instruction, but 
doesn't state what it is precisely.  Could you please add a comment here 
to describe what it is?

> +#else
> +#define ALT_WFI	wfi
> +#endif
> +
>  #else /* !__ASSEMBLER__ */
>  
>  #define ALT_SFENCE_VMA_ASID(asid)					\
> @@ -53,6 +63,23 @@ asm(ALTERNATIVE(	\
>  	: /* no inputs */	\
>  	: "memory")
>  
> +#ifdef CONFIG_ERRATA_MIPS_P8700_WFI
> +#define ALT_RISCV_WFI()										\
> +asm volatile(ALTERNATIVE(									\
> +		"wfi\n" /* Original RISC-V wfi insn */						\
> +		__nops(7),									\
> +		".rept 8;" MIPS_PAUSE ".endr;\n", /* Replacement: mips.pause for P8700 */	\
> +		MIPS_VENDOR_ID, /* Vendor ID to match */					\
> +		ERRATA_MIPS_P8700_WFI, /* patch_id */						\
> +		CONFIG_ERRATA_MIPS_P8700_WFI)							\
> +		: /* no outputs */								\
> +		: /* no inputs */								\
> +		: "memory")
> +#else
> +#define ALT_RISCV_WFI()		\
> +	__asm__ __volatile__ ("wfi")
> +#endif
> +
>  /*
>   * _val is marked as "will be overwritten", so need to set it to 0
>   * in the default case.


thanks,

- Paul

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv