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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2026 06:53, Oleksii Kurochko wrote:
> 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)

Since you don't support floating point extensions so far, that's probably
best. I don't quite understand the mentioning of Zdinx, though: That
extension (by itself) doesn't add any memory access insns.

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

Here and above using register_t won't help with Zilsd. The type is tied
to Xen's xlen, but you mean to check against the guest's here.

Jan