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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/18/26 10:44 AM, Baptiste Le Duc wrote:
> > 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().
> >
> > The mask/match chain is adapted from Linux's KVM RISC-V implementation.
> Nit: maybe you could add the Origin: trailer as mentioned in the
> sending-patches.adoc.
I think that I will drop that sentense as the code in v3 is changed 
pretty significantly so it doesn't too much sense to mention that it was 
derived from Linux's KVM RISC-V.
Basically even if I will put Origin: now here then it will be still 
pretty hard to undestand what was used or not as I mentioned above there 
are a lot of changes done in comparison with original.
I will take it in mind for my future patches.

[...]
> > +
> > +/*
> > + * 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()).
> > + *
> > + * @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.
> > + *
> > + * Returns false if the instruction is not a load or store which can be
> > + * emulated here.
> > + */
> > +static __maybe_unused bool decode_ldst_insn(struct decoded_insn *di,
> > +                                            unsigned int xlen)
> > +{
> > +    unsigned long insn = di->insn;
> > +    /* Register fields of the uncompressed forms ... */
> > +    unsigned int rd = RV_RD(insn);
> > +    unsigned int rs2 = RV_RS2(insn);
> > +    /*
> > +     * ... and of the compressed ones, where the 3-bit field selects one of
> > +     * x8..x15, while the stack-pointer-relative forms have a full-width one.
> > +     */
> > +    unsigned int rs2s = RVC_RS2S(insn);
> > +    unsigned int rs2c = RVC_RS2(insn);
> > +
> This naming are confusing because above you described di->reg to be rd
> for load and rs2 for store but here ...
> > +    di->is_write = false;
> > +    di->is_unsigned = false;
>
> > +    di->reg = rd;
> > +
> > +    if ( (insn & INSN_MASK_LB) == INSN_MATCH_LB )
> > +        di->len = 1;
> > +    else if ( (insn & INSN_MASK_LBU) == INSN_MATCH_LBU )
> > +    {
> > +        di->len = 1;
> > +        di->is_unsigned = true;
> > +    }
> > +    else if ( (insn & INSN_MASK_LH) == INSN_MATCH_LH )
> > +        di->len = 2;
> > +    else if ( (insn & INSN_MASK_LHU) == INSN_MATCH_LHU )
> > +    {
> > +        di->len = 2;
> > +        di->is_unsigned = true;
> > +    }
> > +    else if ( (insn & INSN_MASK_LW) == INSN_MATCH_LW )
> > +        di->len = 4;
> > +    else if ( xlen == 64 && (insn & INSN_MASK_LWU) == INSN_MATCH_LWU )
> > +    {
> > +        di->len = 4;
> > +        di->is_unsigned = true;
> > +    }
>
> > +    else if ( (insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW )
> > +    {
> > +        di->len = 4;
>
> > +        di->reg = rs2s;
> ... you assigned rs2s for a load. According to the spec, it should be rd'.
>
> I would suggest something generic to load and store. Maybe rxs with a comment 
> to explain it concerns rs2' for store and rd' for load.
>
>      /*
>       * ... and of the compressed ones, where the 3-bit field selects one of
>       * x8..x15, while the stack-pointer-relative forms have a full-width one.
>       * rxs is named after that field's spec mnemonic, rd'/rs2': rd' for
>       * compressed loads, rs2' for compressed stores.
>       */
>      unsigned int rxs = RVC_RS2S(insn);
Good point. It isn't partly applied to what I suggested in one of the 
reply to Jan B. but I will try to re-use part of your suggestion there.
It will look like:

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.
         */
        funct3 = RV_X(insn, 13, 3);

        if ( (insn & 1) || !(funct3 & 2) )
            return false;

        di->is_write = funct3 & 4;
        width_log2 = 2 + (funct3 & 1);

        /*
         * The register operand is rd' of a load or rs2' of a store for the
         * register-relative forms, both being bits[4:2], and rs2 of a 
store
         * or rd of a load for the sp-relative ones.
         */
        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);
        width_log2 = funct3 & 3;

        switch ( insn & INSN_OPCODE_MASK )
        {
        case INSN_OPCODE_LOAD:
            di->is_unsigned = funct3 & 4;
            di->reg = RV_RD(insn);
            break;

        case INSN_OPCODE_STORE:
            if ( funct3 & 4 )
                return false;
            di->is_write = true;
            di->reg = RV_RS2(insn);
            break;

        default:
            return false;
        }
    }

    di->len = 1U << width_log2;

    /*
     * No access is wider than XLEN, and one as wide as XLEN exists only in
     * its sign-extending form: this rules out the encodings which 
exist for
     * XLEN=64 only on a 32-bit guest, including C.FLW for C.LD (and 
alike).
     */
    if ( (di->len * BITS_PER_BYTE > xlen) ||
         (di->is_unsigned && di->len * BITS_PER_BYTE == xlen) )
        return false;

    return true;
}

> > +    }
> > +    /* c.lwsp and c.ldsp are reserved with rd being x0. */
> > +    else if ( (insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP && rd )
> > +        di->len = 4;
> > +    else if ( xlen == 64 && (insn & INSN_MASK_LD) == INSN_MATCH_LD )
> > +        di->len = 8;
> > +    else if ( xlen == 64 && (insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD )
> > +    {
> > +        di->len = 8;
> > +        di->reg = rs2s;
> > +    }
> > +    else if ( xlen == 64 && (insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
> > +              rd )
> > +        di->len = 8;
> > +    else if ( (insn & INSN_MASK_SB) == INSN_MATCH_SB )
> > +    {
> > +        di->len = 1;
> > +        di->is_write = true;
> > +        di->reg = rs2;
> > +    }
> > +    else if ( (insn & INSN_MASK_SH) == INSN_MATCH_SH )
> > +    {
> > +        di->len = 2;
> > +        di->is_write = true;
> > +        di->reg = rs2;
> > +    }
> > +    else if ( (insn & INSN_MASK_SW) == INSN_MATCH_SW )
> > +    {
> > +        di->len = 4;
> > +        di->is_write = true;
> > +        di->reg = rs2;
> > +    }
> > +    else if ( (insn & INSN_MASK_C_SW) == INSN_MATCH_C_SW )
> > +    {
> > +        di->len = 4;
> > +        di->is_write = true;
> > +        di->reg = rs2s;
> > +    }
> > +    else if ( (insn & INSN_MASK_C_SWSP) == INSN_MATCH_C_SWSP )
> > +    {
> > +        di->len = 4;
> > +        di->is_write = true;
> > +        di->reg = rs2c;
> > +    }
> > +    else if ( xlen == 64 && (insn & INSN_MASK_SD) == INSN_MATCH_SD )
> > +    {
> > +        di->len = 8;
> > +        di->is_write = true;
> > +        di->reg = rs2;
> > +    }
> > +    else if ( xlen == 64 && (insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD )
> > +    {
> > +        di->len = 8;
> > +        di->is_write = true;
> > +        di->reg = rs2s;
> > +    }
> > +    else if ( xlen == 64 && (insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP )
> > +    {
> > +        di->len = 8;
> > +        di->is_write = true;
> > +        di->reg = rs2c;
> > +    }
> > +    else
> > +        return false;
> > +
> > +    return true;
> > +}
> > +
[...]

> > +/*
> > + * 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)
> > +
> >   #if __riscv_xlen == 64
> >   #define HSTATUS_VSXL                  _UL(0x300000000)
> >   #define HSTATUS_VSXL_SHIFT            32
> > @@ -896,6 +904,8 @@
> >                                          (RV_X(x, 7, 2) << 6))
> >   #define RVC_SDSP_IMM(x)                       ((RV_X(x, 10, 3) << 3) | \
> >                                          (RV_X(x, 7, 3) << 6))
> > +#define RV_RD(insn)                    RV_X(insn, SH_RD, 5)
> > +#define RV_RS2(insn)                   RV_X(insn, SH_RS2, 5)
> Nit: format
I think it looks like that because tabs are used there instead of spaces 
and tabs are there because it was orignally taken from the project which 
uses tabs.
Thanks!

~ Oleksii