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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/22/26 2:30 PM, Baptiste Le Duc wrote:
> On 2026-10-15 16:59:53+02:00, Oleksii Kurochko wrote:
> > On 9/30/26 6:23 PM, Oleksii Kurochko wrote:
> >
> > > Implement emulate_load() on top of the decoding interface introduced by
> > > the previous patch: fetch the trapped instruction, decode it, dispatch
> > > the access to a registered MMIO handler via do_mmio(), write the result
> > > back into the destination register and step over the instruction.
> > >
> > > Xen dispatches MMIO synchronously to an in-hypervisor handler, so unlike
> > > KVM RISC-V there is no userspace exit/return step and no equivalent of
> > > the kvm_io_bus_read() / KVM_EXIT_MMIO / kvm_riscv_vcpu_mmio_return()
> > > split; the result is consumed in place.
> > >
> > > Sign extension is done here rather than in the handlers: a signed load
> > > is normalized by a shift pair, so a handler need only report the value
> > > it read.
> > This part of commit message should be updated. Suggestion look below...
> >
> > > At the moment vINTC is the only backend registered with the MMIO
> > > dispatch, so in practice this only covers vINTC traps. An access which
> > > no handler claims currently crashes the domain; injecting an access
> > > fault into the guest instead is left for later.
> > >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > ---
> > [...]
> >
> > >    static int emulate_load(const struct guest_fault *gf)
> > >    {
> > > -    /* Transient, until the helpers gain their users. */
> > > -    (void)advance_pc;
> > > -    (void)guest_xlen;
> > > -    (void)guest_gpr;
> > > -    (void)insn_fetch_faulted;
> > > -    (void)decode_ldst_insn;
> > > +    struct cpu_user_regs *regs = gf->regs;
> > > +    mmio_info_t info = { .is_write = false };
> > > +    struct decoded_insn di = {};
> > > +    unsigned int shift = 0;
> As shift is always computed, I don't think it makes sense to init it.
Good point!
I think that originally I took that into account when tried to the 
suggestion but missed to sent here. I will double check!
> > > +    int rc;
> > >    
> > > -    return -EOPNOTSUPP;
> > > +    /* 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(info.data) - di.len);
> > > +
> > > +#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. */
> > > +    if ( di.reg )
> > > +        *guest_gpr(regs, &di) = (long)(info.data << shift) >> shift;
> > It would be more correct to have here:
> >
> >       shift = BITS_PER_BYTE * (sizeof(info.data) - di.len);
> >       ...
> >       if ( di.reg )
> >           *guest_gpr(regs, &di) = di.is_unsigned
> >                                   ? (info.data << shift) >> shift
> >                                   : (long)(info.data << shift) >> shift;
> >
> > and update the commit message part "Sign extension is done here…" to:
> >
> > Sign and zero extension are done here rather than in the handlers: a
> > shift pair drops whatever lies above the access width in the value read
> > and, for a signed load, sign-extends it, so a handler need only report
> > the value it read.
> this LGTM.
Thanks for checking that!

~ Oleksii