Re: [PATCH v2 25/39] xen/riscv: add guest load emulation for trapped MMIO accesses

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 1:48 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > @@ -419,7 +416,41 @@ static __maybe_unused bool decode_ldst_insn(struct 
> > decoded_insn *di,
> >   
> >   static int emulate_load(const struct guest_fault *gf)
> >   {
> > -    return -EOPNOTSUPP;
> > +    struct cpu_user_regs *regs = gf->regs;
> > +    mmio_info_t info = { .is_write = false };
> > +    struct decoded_insn di;
> > +    unsigned int shift = 0;
> > +    int rc;
> > +
> > +    /* A fault taken re-reading the instruction is redirected to the guest. */
> > +    if ( insn_fetch_faulted(gf, &di) )
> > +        return 0;
> > +
> > +    if ( !decode_ldst_insn(&di, guest_xlen(regs)) || di.is_write )
> > +        return -EOPNOTSUPP;
> > +
> > +    if ( !di.is_unsigned )
> > +        shift = BITS_PER_BYTE * (sizeof(unsigned long) - di.len);
> This is one of the cases where sizeof(<type>) is not only unclear to
> read, but actively risky: Which variable(s) of that type does this
> refer to? What if those variable(s)' type(s) change? Aha, ...
>
> > +#ifdef EMULATE_LOAD_DEBUG
> > +    gdprintk(XENLOG_DEBUG, "pc=%#lx, addr=%#"PRIpaddr", len=%u, shift=%u\n",
> > +             regs->sepc, gf->gpa, di.len, shift);
> > +#endif
> > +
> > +    rc = do_mmio(&info, gf->gpa, di.len);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    /*
> > +     * A load into x0 discards its result: writing regs->zero would break the
> > +     * invariant that it reads as zero when x0 is a source operand elsewhere.
> > +     */
> > +    if ( di.reg )
> > +        *guest_gpr(regs, di.reg) = (long)(info.data << shift) >> shift;
> ... you apparently mean sizeof(info.data) there.
Good point. I will try to follow such approach in future and use the 
variable name instead of a type.
> The comment is (nit) also too long for my taste. Everything from the
> colon onwards is imo redundant.
Probably you are right. It is a little bit obvious just from the 
defintion of x0 that it should be always zero. I will drop that part of 
the comment after the colon.
THanks.

~ Oleksii