Re: [PATCH v1 16/17] xen/riscv: add guest load emulation for trapped MMIO accesses

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/13/26 9:28 AM, Jan Beulich wrote:
> On 13.08.2026 09:15, Jan Beulich wrote:
> > On 29.07.2026 15:40, Oleksii Kurochko wrote:
> > >   static int emulate_load(unsigned long fault_addr, unsigned long htinst)
> > >   {
> > > -    return -EOPNOTSUPP;
> > > +    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
> > > +    mmio_info_t info = { .is_write = false };
> > > +    unsigned long insn;
> > > +    unsigned int shift = 0, len, insn_len;
> > > +    bool is_unsigned = false;
> > > +    int rc;
> > > +
> > > +    if ( decode_trapped_insn(htinst, &insn, &insn_len) )
> > > +        return 0;
> > > +
> > > +    /* Decode length of MMIO and whether it is a sign- or zero-extending load 
> > > */
> > > +    if ( (insn & INSN_MASK_LB) == INSN_MATCH_LB )
> > > +        len = 1;
> > > +    else if ( (insn & INSN_MASK_LBU) == INSN_MATCH_LBU )
> > > +    {
> > > +        len = 1;
> > > +        is_unsigned = true;
> > > +    }
> > > +    else if ( (insn & INSN_MASK_LH) == INSN_MATCH_LH )
> > > +        len = 2;
> > > +    else if ( (insn & INSN_MASK_LHU) == INSN_MATCH_LHU )
> > > +    {
> > > +        len = 2;
> > > +        is_unsigned = true;
> > > +    }
> > > +    else if ( (insn & INSN_MASK_LW) == INSN_MATCH_LW )
> > > +        len = 4;
> > Already up to here this demonstrates a weakness of the INSN_MASK_*
> > set of #define-s (which I similarly observe in binutils, and I expect it
> > all has the same questionable origin). All INSN_MASK_L* and INSN_MASK_FL*
> > (also INSN_MASK_S* and INSN_MASK_FS*) are identical, allowing for a nice
> > switch() to be used here in principle. That said, with access width
> > nicely encoded in FUNCT3, it's not even clear whether a switch() would
> > end up being needed / efficient.
> >
> > Otoh none of these masks cover the pseudoinsns that htinst may supply.
> >
> > Further, what about A-extension insns? Some (if not all) of them can
> > plausibly be used on MMIO, I think.
> Because of the further additions that are going to be needed, may I also
> suggest to consider putting emulation code in its own file (emulate.c
> perhaps), rather than directly in traps.c?
Good point. It really makes sense to move emulation now to emulate.c.

Thanks.

~ Oleksii