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

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

> 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
…".

> 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).

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