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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/14/26 2:01 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/emulate.c
> > +++ b/xen/arch/riscv/emulate.c
> > @@ -453,9 +453,28 @@ static int emulate_load(const struct guest_fault *gf)
> >       return 0;
> >   }
> >   
> > -static int emulate_store(struct guest_fault *gf)
> > +static int emulate_store(const struct guest_fault *gf)
> >   {
> > -    return -EOPNOTSUPP;
> > +    struct cpu_user_regs *regs = gf->regs;
> > +    mmio_info_t info = { .is_write = true };
> > +    struct decoded_insn di;
> > +    int rc;
> > +
> > +    if ( insn_fetch_faulted(gf, &di) )
> > +        return 0;
> > +
> > +    if ( !decode_ldst_insn(&di, guest_xlen(regs)) || !di.is_write )
> > +        return -EOPNOTSUPP;
> > +
> > +    info.data = *guest_gpr(regs, di.reg);
> This came to mind only here, but applies to the earlier patch as well:
> There's no checking of di.len, not even by an assertion. The above is
> fragile as to extensions like Zilsd. Zilsd itself may still be okay as
> the overrun of the register field will hit the correct one, but the
> general concern remains (plus of course that moving across fields is
> UB).
Nothing can produce di.len > sizeof(register_t) today, as the 8-byte 
cases are all gated on xlen == 64, so I'd add the assertion at the point 
where the lengths are assigned, covering both emulate_load() (where the 
shift calculation would underflow) and emulate_store() at once:
  ASSERT(di->len <= sizeof(register_t));
right before decode_ldst_insn()'s final "return true".

Probably it makes sense to have just "if (di->len >= sizeof(register_t) 
)" with the comment and then return false in deocde_lst_insn():
+    /*
+     * An access wider than a register could not be carried through: the
+     * register operand guest_gpr() hands out is register_t-wide, as is the
+     * value an emulated access moves. None of the encodings above yields
+     * such an access, the 8-byte ones all being gated on XLEN=64, but a
+     * future extension might (Zilsd, say, whose 8-byte accesses exist 
for a
+     * 32-bit guest).
+     */
+    if ( di->len > sizeof(register_t) )
+        return false;

(i think that the same could be also true for D and Zdinx but I will 
mention only Zilsd as an example)
Also, I think it make sense to add the comment to guest_gpr() than 
di->len is checked in decode_ldst_insn(). Alternative will be to update 
the proto of guest_gpr() and pass `di` and then have extra ASSERT() in 
guest_gpr() for the case if someone will try to use guest_gpr() without 
using insn_fetch_faulted() & decode_ldst_insn() before guest_gpr().
I will apply this alternative way, it looks to me better for now and 
then will add the following ASSERT:
    /*
     * decode_ldst_insn() is what fills @di in, and it rejects an access
     * wider than a register.
     */
    ASSERT(di->len <= sizeof(register_t));

Thanks.

~ Oleksii