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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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?

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?
Thanks!

~ Oleksii