Re: [PATCH v2 17/39] xen/riscv: decouple INSN_PSEUDO_VS_* from the hypervisor's XLEN

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

On 2026-09-08 11:34 +0200, Oleksii Kurochko wrote:
> 
> 
> On 9/7/26 5:57 PM, Baptiste Le Duc wrote:
> >> htinst reports a pseudoinstruction when a guest page fault is taken on an
> >> implicit memory access done for VS-stage address translation. Four such
> >> values are defined, differing in the access type (read or write) and in the
> >> access width: 4 bytes (0x2000/0x2020) or 8 bytes (0x3000/0x3020).
> >>
> >> That width is the width of a VS-stage PTE, i.e. it follows the guest's
> >> paging mode (4 bytes for Sv32, 8 bytes for Sv39 and wider) and has nothing
> >> to do with the XLEN Xen itself is built for. Selecting just one pair with
> >> where a guest running with VSXL=32 and Sv32 in vsatp produces the 4-byte
> > Sentence is broken. Guess you mean "Selecting just one pair based on Xen's 
> > XLEN misses the case where
> > …".
> 
> Likely it is becuase of my low level English but it seems that original 
> version wast okay and what you added is just a part from prev. sentence 
> but I will add your suggestion for better clearness. Thanks for noticing 
> that!
> 
> > 
> >> forms. Such an htinst would not be recognized as a pseudoinstruction and 
> >> the
> >> fault would be mistaken for an ordinary MMIO trap: Xen would fetch and
> >> decode whatever instruction sepc happens to point at (unrelated to the
> >> access which faulted) and emulate it against a guest physical address
> >> derived from htval, which for an implicit access holds the address of a
> >> VS-stage PTE rather than of any access the guest performed.
> >>
> >> Define all four values unconditionally instead, named after the access 
> >> width
> >> they encode rather than after the build's XLEN. On RV32 the 8-byte forms
> >> simply never occur, so recognizing them costs nothing.
> >>
> >> Dropping the ladder loses no build-time coverage: a build for an XLEN other
> >> than 32 or 64 already fails on the equivalent ladders in asm/asm.h and
> >> asm/config.h, so no replacement #error is needed here. Adding one keyed on
> >> CONFIG_RISCV_* would in any case re-introduce exactly the conflation this
> >> patch removes.
> >>
> >> This diverges from the imported version of riscv_encoding.h.
> >>
> >> No functional change: the values have no user yet.
> >>
> >> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >>
> >> diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h 
> >> b/xen/arch/riscv/include/asm/riscv_encoding.h
> >> index c63e5e3046..2d2e7e11b3 100644
> >> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> >> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> >> @@ -839,25 +839,17 @@
> >>   #define INSN_MASK_FENCE_TSO              0xffffffff
> >>   #define INSN_MATCH_FENCE_TSO             0x8330000f
> >>   
> >> -#if __riscv_xlen == 64
> >> -
> >>   /* 64-bit read for VS-stage address translation (RV64) */
> >> -#define INSN_PSEUDO_VS_LOAD               0x00003000
> >> +#define INSN_PSEUDO_VS_LOAD64             0x00003000
> >>   
> >>   /* 64-bit write for VS-stage address translation (RV64) */
> >> -#define INSN_PSEUDO_VS_STORE      0x00003020
> >> -
> >> -#elif __riscv_xlen == 32
> >> +#define INSN_PSEUDO_VS_STORE64            0x00003020
> >>   
> >>   /* 32-bit read for VS-stage address translation (RV32) */
> >> -#define INSN_PSEUDO_VS_LOAD               0x00002000
> >> +#define INSN_PSEUDO_VS_LOAD32             0x00002000
> >>   
> >>   /* 32-bit write for VS-stage address translation (RV32) */
> >> -#define INSN_PSEUDO_VS_STORE      0x00002020
> >> -
> > Whole point of patch is these no longer depend on build XLEN, yet
> > comments still say "(RV64)"/"(RV32)" which could be confusing. Maybe it
> > should be better to indicate, as the spec does, that RV32 values are
> > used when VSXLEN=32 (only sv32 paging mode) and RV64 values when
> > VSXLEN=64 (sv39+ paging modes).
> > 
> 
> Could you please clarify to me what in the spec it is?

It is the same table as you indicated below (but mine is Table 56, so it
seems we don't have the same spec version, I'm using the version 20260120).

When I found unclear is that "RV32" or "RV64" alone is ambiguous,
since host and guest can differ (e.g. RV64 host running an RV32 guest).
Saying it depends on VSXLEN instead makes clear which one is meant.

> 
> This comments are just copy from the spec:
> 
> Table 39. Special pseudoinstruction values for guest-page faults. The 
> RV32 values are used when VSXLEN=32, and the RV64 values when VSXLEN=64.
> Value           Meaning
> 0x00002000      32-bit read for VS-stage address translation (RV32)
> 0x00002020      32-bit write for VS-stage address translation (RV32)
> 
> Value           Meaning
> 0x00003000      64-bit read for VS-stage address translation (RV64)
> 0x00003020      64-bit write for VS-stage address translation (RV64)
> 
> So the comments are just copy of "Meaning" column from the spec.
> 
> And I think that Meaning column is fine here as we could have a case of 
> when hypervisor has XLEN=64 but guests could be on it RV32 and RV64 and 
> if a guest is RV32 (what means VSXLEN=32) then the comment above 
> defintion mean that we hav 32-bit read/write VS-stage address 
> translation for (RV32) guest and the similar is for RV64.
> 
> Do I miss something? Is a comments make more sense now or I have to 
> still update them in some way?
>
I think it would be more clear to modify the comment by that:
/* 32-bit read for VS-stage address translation (VSXLEN=32) */

But now with your explanation, your comment is more clear as it specify
VS-stage address, so the guest. Feel free to adopt my version or not.
> 
> Thanks!
> 
> ~ Oleksii
> 
> 
> 
>