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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com>
>>>>
>>>> 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