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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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