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:13 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>
> > > ---
> > [...]
> Sorry I don't understand, did you forget to put the suggestion?
Sorry for confusion, I put it here to drop the code which isn't 
necessary for my suggestion. and my suggestion was just at the end of my 
original reply (I will just copy it here):
>   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;
> +    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.

> +
> +    advance_pc(regs, di.insn_len);
> +
> +    return 0;
>   }
>

~ Oleksii