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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/8/26 6:04 PM, Baptiste Le Duc wrote:
> 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.
Agree your suggestion here will be more clear. I will apply it.

Thanks.

~ Oleksii