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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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.
> > +    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.

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>