Re: [PATCH v3 23/39] xen/riscv: add helpers for decoding a trapped load or store

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

> emulate_load() and emulate_store() will both need to obtain the
> instruction which caused a guest MMIO trap, decode it, and locate the
> register operand it names. Add what the two share, ahead of either of
> them being implemented: struct decoded_insn, insn_fetch_faulted(),
> decode_ldst_insn(), guest_xlen(), guest_gpr() and advance_pc(), together
> with the XLEN_FIELD_*, INSN_OPCODE_* and RV_RD()/RV_RS2() definitions
> they need.
> 
> Nothing calls any of this yet, so reference the functions from the
> emulate_load() stub to keep the build going; the references go away once
> emulate_load() gains its body later.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> How these helpers are used can be seen in the patches adding emulate_load()
> and emulate_store().
Appreciate this precision, thanks!
> [ ... 116 lines skipped ... ]
> +/*
> + * The effective XLEN of the guest at the point of the trap: hstatus.VSXL 
> for a
> + * trap taken from VS-mode, vsstatus.UXL for one taken from VU-mode.
> + *
> + * VSXL is consulted whichever mode the trap came from, as it also gives the
> + * width of vsstatus itself: where VSXL says 32, that register has no UXL 
> field
> + * to consult and VU-mode is 32-bit as well, there being nothing to 
> configure.
Reading "it also gives the width of vsstatus itself", one might
understand that VSXL equals UXL in all cases, while, as you mentioned,
this only holds when VSXL says 32. I would slightly reword it, e.g.:
```
    VSXL is consulted whichever mode the trap came from: if VSXL says
    32, vsstatus has no UXL field, so VU-mode is 32-bit as well and
    there is nothing to configure.
```
>
> + *
> + * It is needed to decode a trapped instruction: the encodings which exist 
> only
> + * for XLEN=64 must not be recognized for a 32-bit guest. Besides those 
> simply
> + * being reserved there, the compressed ones are ambiguous: C.LD and C.FLW
> + * share the encoding 0x6000 (mask 0xe003), and likewise C.SD/C.FSW,
> + * C.LDSP/C.FLWSP and C.SDSP/C.FSWSP.
> + *
> + * IS_ENABLED() can't be used here as HSTATUS_VSXL is defined for
> + * __riscv_xlen == 64 only, the field not existing on RV32 in the first 
> place.
> + */
> +static unsigned int guest_xlen(const struct cpu_user_regs *regs)
> +{
> +#ifdef CONFIG_RISCV_32
> +    return 32;
> +#else
> +    unsigned long xl = MASK_EXTR(regs->hstatus, HSTATUS_VSXL);
xl only ever holds a 2-bit field, so unsigned int would be better here.
> [ ... 49 lines skipped ... ]
> +static unsigned long *guest_gpr(struct cpu_user_regs *regs,
> +                                const struct decoded_insn *di)
> +{
> +    /*
> +     * decode_ldst_insn() is what fills @di in, and it rejects an access
> +     * wider than a guest register.
> +     */
> +    ASSERT(di->len <= (guest_xlen(regs) / BITS_PER_BYTE));
Nit: the parentheses around the division aren't needed.
> [ ... 14 lines skipped ... ]
> +static bool insn_fetch_faulted(const struct guest_fault *gf,
> +                               struct decoded_insn *di)
> +{
> +    unsigned long htinst = gf->htinst;
> +
> +    /*
> +     * A pseudoinstruction says nothing about the instruction the guest was
> +     * executing, and comes with a guest physical address which isn't the one
> +     * that instruction accessed. handle_guest_page_fault() deals with such a
> +     * fault on its own, so no emulation can ever start for one.
> +     */
> +    ASSERT(!htinst_is_pseudo(htinst));
> +
> +    /*
> +     * Bit[0] == 1 implies trapped instruction value is transformed 
> instruction
> +     * or custom value. A transformed instruction is a 32-bit encoding, so a
> +     * value with any bit above bit[31] set is a custom one, which is of no 
> use
> +     * for decoding: fetch the instruction from guest memory then, as for 
> zero.
> +     */
> +    if ( (htinst & BIT(0, UL)) && (htinst == (uint32_t)htinst) )
> +    {
> +        /*
> +         * The transformation always yields the 32-bit format, with bits[1:0]
> +         * holding a marker instead of the original opcode bits: bit[0] set 
> to
> +         * flag the transformation, bit[1] clear if the trapped instruction
> +         * was a compressed one. Restoring the opcode bits makes the value 
> the
> +         * valid 32-bit encoding decode_ldst_insn() decodes. Its handling of
> +         * compressed encodings exists for the branch below, where a
> +         * compressed instruction is read from guest memory as is: a trapped
> +         * one arrives here already expanded to its 32-bit equivalent, and 
> the
> +         * opcode bits just restored have it decoded as such.
Reading this, I first understood that something had to be done to recover
the original compressed opcode. "Restoring the opcode bits" suggests going
back to the trapped encoding, while it is only setting bits[1:0] to 0b11 of
an already expanded instruction. Maybe say that first, e.g.:

```
    /*
     * A compressed instruction is reported already expanded to its
     * 32-bit equivalent by the hardware, so htinst always holds a
     * 32-bit encoding, apart from bits[1:0] which is holding a marker:
     * bit[0] is set to flag the transformation and bit[1] is clear if
     * the trapped instruction was compressed. Setting both bits gives
     * the original 32-bit encoding for decode_ldst_insn().
     */
```
> +         *
> +         * The length then cannot come from the value anymore, only from
> +         * bit[1]. And only a 16- or a 32-bit instruction is ever reported
> +         * this way: the standard load and store instructions the hardware
> +         * transforms are all of one of these two lengths, anything else 
> comes
> +         * as the zero special value handled below.
> +         */
> +        di->insn = htinst | 3;
> +        di->insn_len = (htinst & BIT(1, UL)) ? 4 : 2;
> +    }
> +    else
> +    {
> +        const struct cpu_user_regs *regs = gf->regs;
> +        struct trap_info utrap = {};
> +
> +        /*
> +         * Bit[0] == 0 implies trapped instruction value is zero or special
> +         * value. With the pseudoinstructions ruled out above, only zero (or 
> a
> +         * custom value, see above) is left: the instruction has to be read 
> from
> +         * guest memory.
Nit: as the assert would disappears in release build, this comment is
partially true. I would suggest something like:
```
         * Bit[0] == 0 implies trapped instruction value is zero or
         * special value. With the pseudoinstructions ruled out (see
         * comment above) only zero (or a custom value, see above) is
         * left: the instruction has to be read from guest memory.
```

> +         */
> +
> +        di->insn = read_guest(regs->sepc, true, &utrap);
> +        if ( utrap.scause )
> +        {
> +            /*
> +             * If during getting of trapped instruction a fault happen in
> +             * G-stage translation then CAUSE_LOAD_GUEST_PAGE_FAULT is
> +             * generated. Such faults during this operation is considered as
> +             * bus error.
Typo: "Such faults ... are considered" (plural subject). Also "a fault
happen" above should be "a fault happens", and "as bus error" -> "a
bus error". Maybe:

    /*
    * If a G-stage translation fault happens while fetching the
    * trapped instruction, CAUSE_LOAD_GUEST_PAGE_FAULT is generated.
    * Such faults are treated as an access fault.
    */
I used access fault instead of bus error because the code below sets
CAUSE_FETCH_ACCESS which is more like an access fault.

> [ ... 54 lines skipped ... ]
> +/*
> + * Decode the load or store instruction fetched into @di, filling in the
> + * remaining fields of it (@di->insn and @di->insn_len are filled by
> + * insn_fetch_faulted()). Fields which don't apply to the instruction are 
> left
> + * alone, so @di is expected to start out zeroed.
> + *
> + * @xlen is the effective XLEN of the guest, needed as
> + * the encodings which exist for XLEN=64 only must not be recognized for a
> + * 32-bit guest.
Nit: odd wrapping, "the encodings" fits on the line above.

> + *
> + * Returns false if the instruction is not a load or store which can be
> + * emulated here.
>
Nit: maybe reword for clarity:

 * Returns false if the instruction is not a load or store, or cannot be
 * emulated here.

> + */
> +static bool decode_ldst_insn(struct decoded_insn *di, unsigned int xlen)
> +{
> +    uint32_t insn = di->insn;
> +    unsigned int funct3, width_log2;
> +
> +    if ( INSN_IS_16BIT(insn) )
> +    {
> +        /*
> +         * C.LW, C.LD, C.SW and C.SD (bits[1:0] == 00), and their sp-relative
> +         * C.*SP forms (bits[1:0] == 10), have bits[15:13] of the form x1y:
> +         * x is set for a store, and y selects a width of 4 or 8 bytes.
Very clear thanks.
> +         */
> +        funct3 = RV_X(insn, 13, 3);
> +
> +        if ( (insn & 1) || !(funct3 & 2) )
> +            return false;
> +
> +        di->is_write = funct3 & 4;
> +        width_log2 = 2 + (funct3 & 1);
> +
> +        if ( !(insn & 2) )
> +            /* Quadrant 0: bits[4:2] encode rd' (load) or rs2' (store) */
> +            di->reg = RVC_RS2S(insn);
> +        else if ( di->is_write )
> +            /* Quadrant 2 (CSS): bits[6:2] encode rs2 for C.SWSP / C.SDSP */
> +            di->reg = RVC_RS2(insn);
> +        else
> +        {
> +            /* Quadrant 2 (CI): bits[11:7] encode rd for C.LWSP / C.LDSP */
> +            di->reg = RV_RD(insn);
> +
> +            /* C.LWSP and C.LDSP are reserved with rd being x0. */
> +            if ( !di->reg )
> +                return false;
> +        }
> +    }
> +    else
> +    {
> +        /*
> +         * funct3[1:0] is log2 of the width in bytes, and funct3[2] selects
> +         * zero-extension for a load, while being reserved for a store.
> +         */
> +        funct3 = RV_X(insn, 12, 3);

Nit: the RVC branch above says where funct3 lives (bits[15:13]), this one
doesn't. Maybe mention bits[14:12] as well, so the reader doesn't wonder why
12 here vs 13 above:

```
        /*
         * LOAD and STORE (I/S-type) have funct3 in bits[14:12]: funct3[1:0]
         * is log2 of the width in bytes, and funct3[2] selects zero-extension
         * for a load, while being reserved for a store.
         */
```

> [ ... 51 lines skipped ... ]
> diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h 
> b/xen/arch/riscv/include/asm/riscv_encoding.h
> index b01da95f7e..1af309aa4a 100644
> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> @@ -65,6 +65,14 @@
>  #define SSTATUS64_UXL                        MSTATUS_UXL
>  #define SSTATUS64_SD                 MSTATUS64_SD
>  
> +/*
> + * Width encoded by the MXL, SXL, UXL and VSXL fields, all of which share one
> + * encoding. 0 is reserved.
> + */
> +#define XLEN_FIELD_32                        _UL(1)
> +#define XLEN_FIELD_64                        _UL(2)
> +#define XLEN_FIELD_128                       _UL(3)
Do we really need long here? int wouldn't be enough?

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