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

On 8/13/26 9:28 AM, Jan Beulich wrote:
> On 13.08.2026 09:15, Jan Beulich wrote:
>> On 29.07.2026 15:40, Oleksii Kurochko wrote:
>>>   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.
> 
> Because of the further additions that are going to be needed, may I also
> suggest to consider putting emulation code in its own file (emulate.c
> perhaps), rather than directly in traps.c?

Good point. It really makes sense to move emulation now to emulate.c.

Thanks.

~ Oleksii