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

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

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