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

On 8/20/26 9:34 AM, Jan Beulich wrote:
> 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).

Oh, right. But considering that htinst can handle only max 31 bits so it 
looks like it can't fit more then 32 bits instructions. But I think it 
is needed ifdef around INSN_LEN to not miss add support for longer 
instructions or just write now more generic macros (or static inline 
function).

> 
>>> 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?

Yes, I don't see for now why such distinguish is necessary. But I will 
re-check that point.
> 
>>>> +    }
>>>> +    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.

What do you mean here by "unpopulated space"? IIUC, it means that to 
have things working we should have GVA -> GPA mapping, if there is no 
such mapping then correspondent access bits aren't set and so a 
page-fault exception corresponding to the original access type. So not 
access fault should be here but just correspondent page fault.

If GPA itself is incorrect (it isn't mapped in G-stage) then it looks to 
me that access fault should be generated. But in this case I think we 
won't be here (in handle_guest_page_fault() at all) as just a page fault 
will be generated (so G-stage fault), not guest page fault (VS-fault 
what is the case in the code above but as I told in prev paragraph 
access fault is too much in that case and just page fault will be enough 
and it looks like it is correspond to hardware behavior).
> 
>>>> +        *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.

Sorry, you are right. Then what I suggested before instead of BUG_ON() 
will be enough:
             if ( is_load_guest_page_fault(utrap.scause) )
                 utrap.scause = CAUSE_FETCH_ACCESS;
as if we don't have mapping in G-stage then it looks like guest is 
trying to reach something wrong.
> 
>> 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.

Oh, right, utrap.stval = utrap.sepc; should be just dropped utrap.stval 
already has a correct value (from ex_handler_trap_info()) which should 
be passed to guest.

> 
>>>> +/*
>>>> + * 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.

I will update the comment above function:

/*
  * Check alignment and dispatch a decoded MMIO access to a registered
  * handler. On success (0), info->data holds the read value for loads.
  *
  * There is no "retry" outcome to handle: find_mmio_handler() returns a
  * copy of the matching handler taken under vmmio->lock and the ops
  * structures are never freed, so the lookup result cannot go stale
  * between finding the handler and invoking it.
  */


> 
>>>>    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.

Definitely no. In this case hypervisor will tell that we can't emulate 
this instruction and then extra handling should be added.

> 
>> 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?

Then yes it will be an issue and such instruction should be emulated 
(when such use cases will come into play)

> 
>>>> +    {
>>>> +        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.

We will get 'return -EOPNOTSUPP' and so guest will be crashed because we 
don't support such work with MMIO.
And then yes it should be likely to be added in parallel with adding 
F/D/Q support for guest. At the moment, KVM supports, for example, F/D/Q 
but doesn't emulate FPU load/store but I agree that with your example it 
could happen.

~ Oleksii