On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
required_extensions[] panics at boot if Zihintpause is missing, but Xen
never actually depends on it: cpu_relax() only emits the "pause" when the
extension is implemented and otherwise falls back to the, which is a legal
no-op on any hart regardless of Zihintpause
support.
You raise a very valid point. Strictly speaking, stating that it "falls
back to a legal no-op" can be slightly misleading because it implies the
instruction is decoded as a literal NOP (addi x0, x0, 0).
In reality, the fallback is a fully valid FENCE instruction
(specifically encoded as `0x0100000F`, which represents `FENCE W, 0`).
Here is why this distinction matters and why it is safe:
1. Since the FENCE instruction is a mandatory part of the RISC-V Base
Integer Instruction Set (RV32I/RV64I), it is guaranteed to be present on
any compliant hart. Thus, it will never trigger an "illegal instruction"
trap.
2. When the Zihintpause extension is not implemented, the hart decodes
and executes this instruction as a standard FENCE with a predecessor set
of 'W' (writes) and an empty (null) successor set of '0'.
3. Because the successor set is empty, it imposes zero memory-ordering
constraints on subsequent instructions.
Thereby I think this part of commit message will be better to re-word in
the following way:
```
The fallback encoding `0x0100000F` is a legally valid FENCE instruction
(`FENCE W, 0`) rather than a native NOP. Since FENCE is guaranteed by
the RISC-V Base ISA, it will never raise an illegal instruction fault.
In the absence of Zihintpause, it executes with an empty successor set,
enforcing zero memory-ordering constraints and thus architecturally
behaving as a NOP.
```
Drop it from required_extensions so hardware without Zihintpause
still boots.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Baptiste Le Duc <[email protected]>
---
xen/arch/riscv/cpufeature.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
index 900cb9d772..661babc0a6 100644
--- a/xen/arch/riscv/cpufeature.c
+++ b/xen/arch/riscv/cpufeature.c
@@ -155,7 +155,6 @@ static const struct riscv_isa_ext_data __initconst
required_extensions[] = {
RISCV_ISA_EXT_DATA(h),
RISCV_ISA_EXT_DATA(zicsr),
RISCV_ISA_EXT_DATA(zifencei),
- RISCV_ISA_EXT_DATA(zihintpause),
RISCV_ISA_EXT_DATA(zbb),
};
It is also needed then to update docs/misc/riscv/booting.txt.
Generally, I agree that zihintpause should be dropped from
required_extensions[]. One thing I would like to point out is that, once
we do that, cpu_relax() may no longer provide a pause hint on hardware
that doesn't implement zihintpause, even if the hardware provides its
own pause instruction with different semantics from a fence which does
nothing.
For example, the MIPS P8700 provides its own pause instruction with a
different encoding from:
__asm__ __volatile__ ( ".insn r MISC_MEM, 0, 0, x0, x0, x16" );
Using fence in this case would not be power-efficient, as it behaves as
a no-op.
For the MIPS P8700, for example:
#define MIPS_PAUSE ASM_INSN_I("0x00501013\n\t")
#define MIPS_EHB ASM_INSN_I("0x00301013\n\t")
#define MIPS_IHB ASM_INSN_I("0x00101013\n\t")
I believe there are other implementations that don't use zihintpause but
provide their own pause instruction as well.
Therefore, I suggest adding the following to riscv_fill_hw_cap():
/*
* Zihintpause isn't mandatory: the encoding used by cpu_relax() is a
* HINT which executes as a no-op on hardware without the extension.
* Report it, as a platform may provide its own way to hint a spin-wait
* loop, which then has to be wired up in cpu_relax().
*/
if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_zihintpause) )
printk(XENLOG_WARNING
"Zihintpause unavailable: cpu_relax() gives the CPU no hint; "
"wire up this platform's pause equivalent in cpu_relax()\n");
Without such a check, we could easily miss updating cpu_relax() for
platforms with their own pause mechanism. While having zihintpause as a
required extension implicitly forces us to consider this, once it is no
longer required, I think we should keep an explicit indication that the
platform-specific pause mechanism may need to be wired up.
Thanks.
~ Oleksii
