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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/16/26 7:15 AM, Jan Beulich wrote:
> 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.

Oh, you are right, it should be guest's xlen.


Thanks.

~ Oleksii