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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2026 17:21, Oleksii Kurochko wrote:
> @@ -419,7 +416,41 @@ static __maybe_unused bool decode_ldst_insn(struct 
> decoded_insn *di,
>  
>  static int emulate_load(const struct guest_fault *gf)
>  {
> -    return -EOPNOTSUPP;
> +    struct cpu_user_regs *regs = gf->regs;
> +    mmio_info_t info = { .is_write = false };
> +    struct decoded_insn di;
> +    unsigned int shift = 0;
> +    int rc;
> +
> +    /* 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(unsigned long) - di.len);

This is one of the cases where sizeof(<type>) is not only unclear to
read, but actively risky: Which variable(s) of that type does this
refer to? What if those variable(s)' type(s) change? Aha, ...

> +#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: writing regs->zero would break the
> +     * invariant that it reads as zero when x0 is a source operand elsewhere.
> +     */
> +    if ( di.reg )
> +        *guest_gpr(regs, di.reg) = (long)(info.data << shift) >> shift;

... you apparently mean sizeof(info.data) there.

The comment is (nit) also too long for my taste. Everything from the
colon onwards is imo redundant.

Jan