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

[Jan Beulich <jbeulich@suse.com>]

On 14.09.2026 17:57, Oleksii Kurochko wrote:
> On 9/14/26 1:03 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> @@ -13,9 +14,29 @@
>>>   #include <asm/csr.h>
>>>   #include <asm/current.h>
>>>   #include <asm/emulate.h>
>>> +#include <asm/guest_access.h>
>>> +#include <asm/processor.h>
>>>   #include <asm/riscv_encoding.h>
>>>   #include <asm/traps.h>
>>>   
>>> +/*
>>> + * Determine the trapped load or store instruction which caused a guest MMIO
>>> + * trap.
>>> + */
>>> +struct decoded_insn {
>>> +    /* The instruction itself, and its length in bytes. */
>>> +    unsigned long insn;
>>> +    unsigned int insn_len;
>>> +    /* Width of the memory access, in bytes. */
>>> +    unsigned int len;
>>> +    /* Number of the register operand: rd for a load, rs2 for a store. */
>>> +    unsigned int reg;
>>> +    /* The access is a store rather than a load. */
>>> +    bool is_write;
>>> +    /* The load zero-extends its result rather than sign-extending it. */
>>> +    bool is_unsigned;
>>> +};
>>
>> I wonder how efficient this is. With use of bitfield the size of this struct
>> can likely be more than halved. With suitable choice of widths this may not
>> even cause significantly worse generated code.
>>
> 
> We could compress the structure into 8 bytes:
> 
> struct decoded_insn {
>      /*
>       * The instruction itself: no ratified extension defines one wider than
>       * 32 bits, and insn_fetch_faulted() rejects anything longer.
>       */
>      uint32_t insn;
>      /* Length of the instruction in bytes: 2 or 4. */
>      unsigned int insn_len:3;
>      /* Width of the memory access, in bytes: 1, 2, 4 or 8. */
>      unsigned int len:4;
>      /* Number of the register operand: rd for a load, rs2 for a store. */
>      unsigned int reg:5;
>      /* The access is a store rather than a load. */
>      bool is_write:1;
>      /* The load zero-extends its result rather than sign-extending it. */
>      bool is_unsigned:1;
> };

Likely this is going a little too far: The non-bool fields may want to
be 8 bits wide, for better code gen.

>>> +    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;
>>> +    }
>>
>> These insns encode the access width uniformly, i.e. doing things the
>> way done above is rather inefficient.
> 
> I think that I don't know how to do that better at the moment.
> 
> It could be less of if/else if to do in this way:
> 
> 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);
> 
>          if ( !(insn & 2) )
>              di->reg = RVC_RS2S(insn);
>          else if ( di->is_write )
>              di->reg = RVC_RS2(insn);
>          else
>          {
>              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;
> 
> 
>      return true;
> }
> 
> But I am not sure this is what you meant.

Yes, this goes along the lines of what I was thinking of.

>>> +    /* 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;
>>
>> Careful with insns not part of the base ISA: Between the trap and you
>> getting to fetch and decode, the in-memory insn may have changed. You
>> posibly set yourself up for vulnerabilities if you permit C encodings
>> for guests not having C exposed to them.
> 
> I think then it will be better to reject it duing instruction fetch in 
> insn_fetch_faulted():
> 
>          di->insn_len = INSN_LEN(di->insn);
> 
>          /*
>           * read_guest() fetches at most two halfwords, so a wider 
> encoding has
>           * been read in part only and cannot be decoded here.
>           *
>           * Report an illegal instruction: none of the extensions exposed to
>           * guests has instructions wider than 32 bits, so such an 
> encoding is
>           * not a valid instruction for the guest in the first place. 
> The same
>           * goes for a compressed encoding where C isn't exposed to the 
> guest:
>           * the instruction in memory may have been changed since the 
> trap, so
>           * what is read back must not be taken to be what trapped.
>           */
>          if ( !di->insn_len ||
>               (di->insn_len == 2 &&
>                !riscv_isa_extension_available(current->domain->arch.isa,
>                                               RISCV_ISA_EXT_c)) )
>          {
>              ...
> 
> Would it be better?

It's an option. Where exactly the check is best placed I can't easily say.

Jan