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

On 2026-08-28 10:59 +0200, Oleksii Kurochko wrote:
> 
> 
> 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.
> ```
I agree with this suggestion, thanks.
> 
> 
> > 
> > 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.
Good catch, thanks for that. I agree with what you said to not forget
platforms that use their own pause mechanism. I will add what you
suggested in v2.
> 
> ~ Oleksii
> 
> 
Thanks.
> 
>