Re: [PATCH v1 16/17] xen/riscv: add guest load emulation for trapped MMIO accesses

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.07.2026 15:40, Oleksii Kurochko wrote:
> Introduce emulate_load() to decode and emulate guest load instructions
> that fault due to MMIO accesses. This provides the basic infrastructure
> required for MMIO emulation on RISC-V.
> 
> The instruction decode (decode_trapped_insn() and the mask/match chain
> for standard and compressed load encodings) is adapted from Linux's KVM
> RISC-V implementation. The completion path differs from KVM's,
> since Xen dispatches MMIO synchronously to an in-hypervisor handler via
> try_handle_mmio() and has no userspace exit/return step equivalent to
> KVM's kvm_io_bus_read() / KVM_EXIT_MMIO / kvm_riscv_vcpu_mmio_return()
> split.
> 
> A fault taken while re-reading the trapped instruction is handled
> depending on the faulting translation stage:
>  - A VS-stage fault is the guest's own fault (e.g. it modified its page
>    tables from another vCPU) and, as in KVM, is redirected to the
>    guest's trap vector, with the cause remapped to
>    CAUSE_FETCH_PAGE_FAULT since HLVX reports execute-permission failures
>    as load faults.
>  - A G-stage fault would mean the P2M mapping of the instruction page
>    disappeared after the instruction was fetched. KVM must handle this
>    by resuming the guest and retrying, as Linux MM can invalidate
>    G-stage mappings at any time. Xen does not remove P2M mappings of a
>    running domain at the moment, so this case is asserted unreachable with
>    BUG_ON(); it will need to be revisited once such removal is implemented.

I don't see why this cannot be implemented correctly right away. The behavior
should be that of an access to unpopulated space on bare hardware, whatever
that behavior is on RISC-V.

> @@ -13,6 +14,11 @@ struct trap_info {
>      register_t stval;
>  };
>  
> +static inline bool is_load_guest_page_fault(unsigned long scause)
> +{
> +    return (scause == CAUSE_LOAD_GUEST_PAGE_FAULT);
> +}

Is something like this really a useful wrapper to have? It doesn't really
shorten anything, nor does (imo) it aid readability.

> @@ -191,6 +193,11 @@ static void timer_interrupt(void)
>      raise_softirq(TIMER_SOFTIRQ);
>  }
>  
> +static always_inline void advance_pc(struct cpu_user_regs *regs, int step)

See my earlier remark regarding always_inline. Also - why plain int? Are
there (going to be) cases where PC is moved backwards (in which case
"advance" isn't suitable naming)?

> +{
> +    regs->sepc += step;
> +}
> +
>  static always_inline unsigned long get_faulting_gpa(void)
>  {
>      /*
> @@ -210,9 +217,162 @@ static always_inline unsigned long 
> get_faulting_gpa(void)
>      return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
>  }
>  
> +/*
> + * Determine the trapped instruction which caused a guest MMIO trap.
> + *
> + * Returns true if the trap was redirected to the guest, in which case
> + * the caller must stop emulation and return success. Otherwise *insn
> + * and *insn_len are filled in and the caller should continue decoding.
> + */
> +static bool decode_trapped_insn(unsigned long htinst, unsigned long *insn,
> +                                unsigned int *insn_len)
> +{
> +    if ( htinst & 0x1 )
> +    {
> +        /*
> +         * Bit[0] == 1 implies trapped instruction value is
> +         * transformed instruction or custom instruction.
> +         */
> +        *insn = htinst | INSN_16BIT_MASK;
> +        *insn_len = (htinst & BIT(1, UL)) ? INSN_LEN(*insn) : 2;

In the if() you don't use BIT(), while here you do. Please be consistent.

Why the use of INSN_LEN(), when due to the earlier assignment it'll always
yield 4 here?

Finally, how would the caller know whether it looks at a transformed insn
or (as fetched below) a "normal" one?

> +    }
> +    else
> +    {
> +        struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);

Pointer-to-const.

> +        struct trap_info utrap = { 0 };

Just {} please.

> +        /*
> +         * Bit[0] == 0 implies trapped instruction value is
> +         * zero or special value.
> +         */

How come you get away without dealing with pseudoinsns? The insn pointed at
by regs->sepc is of no interest for faults caused by implicit memory accesses
originating from VS-stage address translation.

> +        *insn = riscv_vcpu_unpriv_read(true, regs->sepc, &utrap);
> +        if ( utrap.scause )
> +        {
> +            /*
> +             * A G-stage fault here would mean the P2M mapping of the page
> +             * containing the trapped instruction disappeared after it was
> +             * fetched.

Does it? What about, again, faults from VS-stage address translation while
hardware was trying to fetch an insn? That is ...

> Nothing removes P2M mappings of a running domain yet,
> +             * so this cannot happen.

... the necessary P2M mapping may never have been there.

> +             * TODO: Revisit once P2M mappings can be removed at runtime.
> +             */
> +            BUG_ON(is_load_guest_page_fault(utrap.scause));
> +
> +            utrap.sepc = regs->sepc;
> +            utrap.stval = utrap.sepc;

How do you know the fault was at .sepc? A 32-bit insn crossing a page boundary
(implying the C extension is available) may well fault only on its higher half.

> +            riscv_vcpu_trap_redirect(&utrap);
> +
> +            return true;
> +        }
> +
> +        *insn_len = INSN_LEN(*insn);
> +    }
> +
> +    return false;
> +}
> +
> +/*
> + * Check alignment and dispatch a decoded MMIO access to a registered
> + * handler. On success (0), info->data holds the read value for loads.
> + */
> +static int do_mmio(mmio_info_t *info, unsigned long fault_addr,
> +                   unsigned int len)
> +{
> +    /* Fault address should be aligned to length of MMIO */
> +    if ( fault_addr & (len - 1) )
> +        return -EIO;
> +
> +    info->gpa = fault_addr;
> +    info->len = len;
> +
> +    switch ( try_handle_mmio(info) )
> +    {
> +    case IO_HANDLED:
> +        return 0;
> +    case IO_ABORT:
> +        return -EIO;
> +    default:
> +        return -EOPNOTSUPP;
> +    }
> +}

And there's no indication of "retry needed", e.g. when something changed
between find_mmio_handler() and handle_{read,write}()?

Also, nit: Blank lines please between non-fall-through case blocks.

>  static int emulate_load(unsigned long fault_addr, unsigned long htinst)
>  {
> -    return -EOPNOTSUPP;
> +    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
> +    mmio_info_t info = { .is_write = false };
> +    unsigned long insn;
> +    unsigned int shift = 0, len, insn_len;
> +    bool is_unsigned = false;
> +    int rc;
> +
> +    if ( decode_trapped_insn(htinst, &insn, &insn_len) )
> +        return 0;
> +
> +    /* Decode length of MMIO and whether it is a sign- or zero-extending 
> load */
> +    if ( (insn & INSN_MASK_LB) == INSN_MATCH_LB )
> +        len = 1;
> +    else if ( (insn & INSN_MASK_LBU) == INSN_MATCH_LBU )
> +    {
> +        len = 1;
> +        is_unsigned = true;
> +    }
> +    else if ( (insn & INSN_MASK_LH) == INSN_MATCH_LH )
> +        len = 2;
> +    else if ( (insn & INSN_MASK_LHU) == INSN_MATCH_LHU )
> +    {
> +        len = 2;
> +        is_unsigned = true;
> +    }
> +    else if ( (insn & INSN_MASK_LW) == INSN_MATCH_LW )
> +        len = 4;

Already up to here this demonstrates a weakness of the INSN_MASK_*
set of #define-s (which I similarly observe in binutils, and I expect it
all has the same questionable origin). All INSN_MASK_L* and INSN_MASK_FL*
(also INSN_MASK_S* and INSN_MASK_FS*) are identical, allowing for a nice
switch() to be used here in principle. That said, with access width
nicely encoded in FUNCT3, it's not even clear whether a switch() would
end up being needed / efficient.

Otoh none of these masks cover the pseudoinsns that htinst may supply.

Further, what about A-extension insns? Some (if not all) of them can
plausibly be used on MMIO, I think.

> +#ifndef CONFIG_RISCV_32
> +    else if ( (insn & INSN_MASK_LWU) == INSN_MATCH_LWU )

First: Better use IS_ENABLED() in favor of #if{,n}def, whenever possible.
And then this depends not only on CONFIG_RISCV_32, but also on guest
bitness.

> +    {
> +        len = 4;
> +        is_unsigned = true;
> +    }
> +#endif
> +    else if ( (insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW )
> +    {
> +        len = 4;
> +        insn = RVC_RS2S(insn) << SH_RD;
> +    }
> +    else if ( (insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
> +              RV_X(insn, SH_RD, 5) )
> +        len = 4;
> +#ifndef CONFIG_RISCV_32
> +    else if ( (insn & INSN_MASK_LD) == INSN_MATCH_LD )
> +        len = 8;
> +    else if ( (insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD )
> +    {
> +        len = 8;
> +        insn = RVC_RS2S(insn) << SH_RD;
> +    }
> +    else if ( (insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
> +              RV_X(insn, SH_RD, 5) )
> +        len = 8;
> +#endif
> +    else
> +        return -EOPNOTSUPP;

Because you don't permit F/D/Q for guests (yet), FL* and FS* aren't
covered, I expect? I wonder how easy it is going to be to spot the places
needing adjustment once support is to be added. Same perhaps for Zilsd in
RV32 guests.

Jan