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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2026 18:06, Oleksii Kurochko wrote:
> On 8/13/26 9:15 AM, Jan Beulich wrote:
>> On 29.07.2026 15:40, Oleksii Kurochko wrote:
>>> @@ -210,9 +217,162 @@ static always_inline unsigned long 
>>> get_faulting_gpa(void)
>>>       return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
>>>   }
>>>   
>>> +/*
>>> + * Determine the trapped instruction which caused a guest MMIO trap.
>>> + *
>>> + * Returns true if the trap was redirected to the guest, in which case
>>> + * the caller must stop emulation and return success. Otherwise *insn
>>> + * and *insn_len are filled in and the caller should continue decoding.
>>> + */
>>> +static bool decode_trapped_insn(unsigned long htinst, unsigned long *insn,
>>> +                                unsigned int *insn_len)
>>> +{
>>> +    if ( htinst & 0x1 )
>>> +    {
>>> +        /*
>>> +         * Bit[0] == 1 implies trapped instruction value is
>>> +         * transformed instruction or custom instruction.
>>> +         */
>>> +        *insn = htinst | INSN_16BIT_MASK;
>>> +        *insn_len = (htinst & BIT(1, UL)) ? INSN_LEN(*insn) : 2;
>>
>> In the if() you don't use BIT(), while here you do. Please be consistent.
>>
>> Why the use of INSN_LEN(), when due to the earlier assignment it'll always
>> yield 4 here?
> 
> ld/sd instruction which we are trapping here at the moment here could be 
> 2 bit and 4 bit depends on C extension so we need to pass correct 
> instruction length to advance_pc() after it is emulated.

Well, fine, but how does that matter? I pointed you at the preceding
assignment, which sets bits 0 and 1. With that INSN_LEN() is guaranteed
to return (at least) 4 (and it's not presently capable of returning
values larger than 4).

>> Finally, how would the caller know whether it looks at a transformed insn
>> or (as fetched below) a "normal" one?
> 
> According to the spec ((part from htinst ... ):
> On a synchronous exception, if a nonzero value is written, one of the 
> following shall be true about the value:
> 
> • Bit 0 is 1, and replacing bit 1 with 1 makes the value into a valid 
> encoding of a standard instruction.
> In this case, the instruction that trapped is the same kind as indicated 
> by the register value, and the register value is the transformation of 
> the trapping instruction, as defined later. For example, if bits 1:0 are 
> binary 11 and the register value is the encoding of a standard LW (load 
> word) instruction, then the trapping instruction is LW, and the register 
> value is the transformation of the trapping LW instruction.
> 
> • Bit 0 is 1, and replacing bit 1 with 1 makes the value into an 
> instruction encoding that is explicitly designated for a custom 
> instruction (not an unused reserved encoding). This is a custom value. 
> The instruction that trapped is a non-standard instruction. The
> interpretation of a custom value is not otherwise specified by this 
> standard.
> 
> • The value is one of the special pseudoinstructions defined later, all 
> of which have bits 1:0 equal to 00.
> 
> So setting bit 0 to 1 we will guarantee that it is normal "normal" 
> instruction.

Right. Yet my question was how to distinguish the cases. Or are you trying
to tell me that distinguishing isn't going to be necessary, anywhere?

>>> +    }
>>> +    else
>>> +    {
>>> +        struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
>>
>> Pointer-to-const.
>>
>>> +        struct trap_info utrap = { 0 };
>>
>> Just {} please.
>>
>>> +        /*
>>> +         * Bit[0] == 0 implies trapped instruction value is
>>> +         * zero or special value.
>>> +         */
>>
>> How come you get away without dealing with pseudoinsns? The insn pointed at
>> by regs->sepc is of no interest for faults caused by implicit memory accesses
>> originating from VS-stage address translation.
> 
> It is really problem but I think it should be resolved much earlier in 
> handle_guest_page_fault(). I will add the following:
> 
> /*
>       * A guest page fault taken on an implicit memory access performed for
>       * VS-stage address translation (reading a PTE, or updating its A/D 
> bits)
>       * reports a pseudoinstruction in htinst rather than a transformed
>       * instruction. Such a fault can't be emulated: htval holds the guest
>       * physical address of a VS-stage PTE rather than of any access the 
> guest
>       * itself performed (and its two least significant bits are zero 
> instead
>       * of matching stval), while the instruction at sepc is unrelated 
> to the
>       * access which actually faulted.
>       *
>       * Report an access fault to the guest at the original virtual address,
>       * which is what stval already holds and what hardware would raise 
> for a
>       * page table walk hitting an inaccessible address.
>       */
>      if ( (htinst == INSN_PSEUDO_VS_LOAD) || (htinst == 
> INSN_PSEUDO_VS_STORE) )
>      {
>          struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
>          struct trap_info utrap = {
>              .scause = (htinst == INSN_PSEUDO_VS_LOAD) ? CAUSE_LOAD_ACCESS
>                                                        : CAUSE_STORE_ACCESS,
>              .sepc = regs->sepc,
>              .stval = csr_read(CSR_STVAL),
>          };
> 
>          riscv_trap_redirect(&utrap);
>          return;
>      }

That's not what would happen on bare hardware though, aiui. At least I don't
think I ever found it being spelled out anywhere what the supposed behavior
is when a page table resides in unpopulated space.

>>> +        *insn = riscv_vcpu_unpriv_read(true, regs->sepc, &utrap);
>>> +        if ( utrap.scause )
>>> +        {
>>> +            /*
>>> +             * A G-stage fault here would mean the P2M mapping of the page
>>> +             * containing the trapped instruction disappeared after it was
>>> +             * fetched.
>>
>> Does it? What about, again, faults from VS-stage address translation while
>> hardware was trying to fetch an insn? That is ...
>>
>>> Nothing removes P2M mappings of a running domain yet,
>>> +             * so this cannot happen.
>>
>> ... the necessary P2M mapping may never have been there.
> 
> If VS-stage failed then CAUSE_LOAD_PAGE_FAULT will happen so BUG_ON() 
> won't occur and it will be passed to guest to handle it.

Are you sure? So far it was my understanding that CAUSE_LOAD_PAGE_FAULT
would happen when VS-stage translation hits e.g. a non-present leaf
entry. But got an address translation failure while doing the VS-stage
page walk (i.e. failure to translate the address found in a VS-stage
PTE to a host address) would raise CAUSE_LOAD_GUEST_PAGE_FAULT.

> BUG_ON() here catches CAUSE_LOAD_GUEST_PAGE_FAULT (G-stage translation 
> failure).
> 
> Also, as I mentioned above I will change BUG_ON() too:
> 
>              /*
>               * If during getting of trapped instruction a fault happen in
>               * G-stage translation then CAUSE_LOAD_GUEST_PAGE_FAULT is
>               * generated. Such faults during this operation is 
> considered as
>               * bus
>               */

What is "bus" here (dym "bug"?), and why is the sentence unfinished?

>>> +             * TODO: Revisit once P2M mappings can be removed at runtime.
>>> +             */
>>> +            BUG_ON(is_load_guest_page_fault(utrap.scause));
>>> +
>>> +            utrap.sepc = regs->sepc;
>>> +            utrap.stval = utrap.sepc;
>>
>> How do you know the fault was at .sepc? A 32-bit insn crossing a page 
>> boundary
>> (implying the C extension is available) may well fault only on its higher 
>> half.
> 
> According to the spec, if stval is written with a nonzero value when an 
> instruction access-fault or page-fault exception occurs on a system with 
> variable-length instructions, then stval will contain the virtual 
> address of the portion of the instruction that caused the fault, while 
> sepc will point to the beginning of the instruction.
> 
> So here, we are trying to emulate what real hardware will do in this 
> case. In regs->sepc, we have the start of the instruction that we didn't 
> touch. sepc is filled according to the spec in this case.

Right, but utrap.stval is set to the same value, which is explicitly not
in line with what you say above ("will contain the virtual address of the
portion of the instruction that caused the fault").

> Regarding utrap.stval, we know that utrap.sepc points to the correct 
> part of the faulting address, as we are reading the instruction in 
> 16-bit chunks:
> 
> HLVX_HU(%[val], %[addr])        ; low 16 bits from sepc
> andi %[tmp], %[val], 3
> addi %[tmp], %[tmp], -3
> bne  %[tmp], zero, 2f           ; if not (insn & 3) == 3 -> 16-bit, end
> addi %[addr], %[addr], 2        ; <- addr is now sepc+2
> HLVX_HU(%[tmp], %[addr])        ; high 16 bits, possibly from another page
> 
> So, if a trap happens while reading the high 16 bits (which may be 
> located on another page), then utrap.sepc, if the read fails, will point 
> to the high part of the instruction, which is what the spec requires.
> 
> Does that make sense?

Not really, no. As said above - the code as written guarantees
utrap.stval == utrap.sepc, and that cannot always be correct.

>>> +/*
>>> + * Check alignment and dispatch a decoded MMIO access to a registered
>>> + * handler. On success (0), info->data holds the read value for loads.
>>> + */
>>> +static int do_mmio(mmio_info_t *info, unsigned long fault_addr,
>>> +                   unsigned int len)
>>> +{
>>> +    /* Fault address should be aligned to length of MMIO */
>>> +    if ( fault_addr & (len - 1) )
>>> +        return -EIO;
>>> +
>>> +    info->gpa = fault_addr;
>>> +    info->len = len;
>>> +
>>> +    switch ( try_handle_mmio(info) )
>>> +    {
>>> +    case IO_HANDLED:
>>> +        return 0;
>>> +    case IO_ABORT:
>>> +        return -EIO;
>>> +    default:
>>> +        return -EOPNOTSUPP;
>>> +    }
>>> +}
>>
>> And there's no indication of "retry needed", e.g. when something changed
>> between find_mmio_handler() and handle_{read,write}()?
> 
> I don't have any specific scenario where it is needed now so I don't 
> know what to say.
> And there is no race between find_mmio_handler() and 
> handle_{read,write}() as find_mmio_handler() returns copy of the 
> structure under read_lock():

Oh, right, but that's not visible here at all and requires going back to
patch 04 to realize.

>>>   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.
> 
> As I answered above we should handle that before this function will call 
> so here we won't deal with htinst at all. Of course, if what I wrote 
> above is correct. I will double check before applying that.
> 
>>
>> Further, what about A-extension insns? Some (if not all) of them can
>> plausibly be used on MMIO, I think.
> 
> I’m not really sure that the A-extension is actively used for MMIO.

Does the spec preclude their use? I'm unaware of such a restriction.

> At 
> least, Linux doesn’t do that for now, which is why we don’t handle 
> A-extension instructions here.

Focusing on what present Linux needs is okay, but then remaining gaps
should (as said on various other occasions before) be clearly marked.

> I think this is related to the fact that MMIO is usually (if not 
> always?) naturally aligned, and naturally aligned loads and stores are 
> guaranteed by RISC-V to execute atomically.

How does this matter, when a bit or field in MMIO may serve the purpose
of e.g. a semaphore?

>>> +    {
>>> +        len = 4;
>>> +        is_unsigned = true;
>>> +    }
>>> +#endif
>>> +    else if ( (insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW )
>>> +    {
>>> +        len = 4;
>>> +        insn = RVC_RS2S(insn) << SH_RD;
>>> +    }
>>> +    else if ( (insn & INSN_MASK_C_LWSP) == INSN_MATCH_C_LWSP &&
>>> +              RV_X(insn, SH_RD, 5) )
>>> +        len = 4;
>>> +#ifndef CONFIG_RISCV_32
>>> +    else if ( (insn & INSN_MASK_LD) == INSN_MATCH_LD )
>>> +        len = 8;
>>> +    else if ( (insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD )
>>> +    {
>>> +        len = 8;
>>> +        insn = RVC_RS2S(insn) << SH_RD;
>>> +    }
>>> +    else if ( (insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
>>> +              RV_X(insn, SH_RD, 5) )
>>> +        len = 8;
>>> +#endif
>>> +    else
>>> +        return -EOPNOTSUPP;
>>
>> Because you don't permit F/D/Q for guests (yet), FL* and FS* aren't
>> covered, I expect? 
> 
> At the moment, I wrote this function with handling of MMIO instruction 
> in mind, which are at the moment ld and sd.
> 
> Even if to permit F/D/Q then do we really need to trap that 
> instructions? Hypervisor could allow access to FPU to guest and then it 
> will be just a question of context switch to properly save and restore FPU.

And how would you know FPU loads/stores aren't used against MMIO? Later
on, once V support is added, even its loads/stores might be used that way.
Think of video frame buffer accesses, for example.

Jan