Re: [PATCH 4/5] xen/riscv: make Zihintpause no longer a required extension

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 <baptiste.le-duc@xxxxxxxxxx>
> ---
>   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