Re: [PATCH v2 18/39] xen/riscv: add guest page fault handling stub

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/10/26 8:38 AM, Jan Beulich wrote:
> On 09.09.2026 17:09, Oleksii Kurochko wrote:
> > On 9/8/26 4:10 PM, Jan Beulich wrote:
> > > On 27.08.2026 17:21, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/emulate.c
> > > > @@ -0,0 +1,179 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > > +
> > > > +/*
> > > > + * RISC-V instruction emulation for trapped guest accesses
> > > > + */
> > > > +
> > > > +#include <xen/bug.h>
> > > > +#include <xen/errno.h>
> > > > +#include <xen/sched.h>
> > > > +#include <xen/types.h>
> > > > +
> > > > +#include <asm/csr.h>
> > > > +#include <asm/current.h>
> > > > +#include <asm/emulate.h>
> > > > +#include <asm/riscv_encoding.h>
> > > > +#include <asm/traps.h>
> > > > +
> > > > +/*
> > > > + * The hardware-reported details of a guest page fault, gathered once by
> > > > + * handle_guest_page_fault() and passed down to the emulation of the faulted
> > > > + * access.
> > > > + */
> > > > +struct guest_fault {
> > > > +    /* The guest register state as saved on entry to do_trap(). */
> > > > +    struct cpu_user_regs *regs;
> > > If the comment was true, this could be pointer-to-const.
> > I think it can't be pointer-to-const as emulate_load/store functions
> > wants to change PC register after MMIO access emulation is finished to
> > not trap again.
> Of course, hence how I started the sentence.
>
> > > > +    /* scause: a fetch, a load or a store/AMO guest page fault. */
> > > > +    unsigned long cause;
> > > > +    /*
> > > > +     * htinst: the trapped instruction in its transformed form, or one of the
> > > > +     * special values (zero, or a pseudoinstruction).
> > > > +     */
> > > > +    unsigned long htinst;
> > > > +    /* htval: as written by hardware; see resolve_faulting_gpa(). */
> > > > +    unsigned long htval;
> > > > +    /* stval: the guest virtual address of the faulting access. */
> > > > +    unsigned long stval;
> > > > +    /* The faulting guest physical address, filled by resolve_faulting_gpa(). 
> > > > */
> > > > +    paddr_t gpa;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Is @htinst one of the pseudoinstructions reported for a guest page fault
> > > > + * taken on an implicit memory access done for VS-stage address translation?
> > > > + *
> > > > + * All four values are recognized regardless of the hypervisor's XLEN: the
> > > > + * width they encode is that of a VS-stage PTE, i.e. it follows the guest's
> > > > + * paging mode (4 bytes for Sv32, 8 otherwise). On RV32 the 64-bit forms
> > > > + * simply never occur.
> > > > + */
> > > > +static bool htinst_is_pseudo(unsigned long htinst)
> > > > +{
> > > > +    switch ( htinst )
> > > > +    {
> > > > +    case INSN_PSEUDO_VS_LOAD32:
> > > > +    case INSN_PSEUDO_VS_STORE32:
> > > > +    case INSN_PSEUDO_VS_LOAD64:
> > > > +    case INSN_PSEUDO_VS_STORE64:
> > > > +        return true;
> > > > +
> > > > +    default:
> > > > +        return false;
> > > > +    }
> > > > +}
> > > This feels fragile. New pseudo-insns can appear at any time. If the value as
> > > a whole is non-zero, aiui the low two bits being zero indicate a pseudo-insn.
> > > In which case enumerating pseudo-insns we are currently aware of isn't
> > > necessary.
> > I will write it simpler  then:
> >
> > /*
> >    * Is @htinst one of the special pseudoinstruction values, reported for
> > a guest
> >    * page fault taken on an implicit memory access done for VS-stage address
> >    * translation?
> >    *
> >    * It is enough to check only bits[1:0] as according to the spec:
> >    *
> >    * The value is one of the special pseudoinstructions defined later, all of
> >    * which have bits 1:0 equal to 00.
> >    */
> > static bool htinst_is_pseudo(unsigned long htinst)
> > {
> >       return htinst && ((htinst & 3) == 0);
> > }
> And preferably
>
>       return htinst && !(htinst & 3);
>
> to be self-consistent.
Good point. I will apply your suggestion.

> > > > +    /*
> > > > +     * A guest-page fault may arise due to an implicit memory access during
> > > > +     * first-stage (VS-stage) address translation, in which case a guest
> > > > +     * physical address written to htval is that of the implicit memory
> > > > +     * access that faulted - for example, the address of a VS-level page
> > > > +     * table entry that could not be read. (The guest physical address
> > > > +     * corresponding to the original virtual address is unknown when
> > > > +     * VS-stage translation fails to complete)
> > > > +     *
> > > > +     * In such cases htinst reports one of the pseudoinstructions recognized
> > > > +     * by htinst_is_pseudo(), and the fault requires separate handling (since
> > > > +     * G-stage translation failed on an unpopulated/unmapped guest physical
> > > > +     * address during a hardware page-table walk). To match bare hardware
> > > > +     * behavior, we must inject an access fault of the ORIGINAL access type
> > > > +     * (Instruction, Load, or Store/AMO) that initiated the address
> > > > +     * translation.
> > > > +     */
> > > > +    if ( htinst_is_pseudo(gf.htinst) )
> > > > +    {
> > > > +        inject_access_fault(&gf);
> > > > +
> > > > +        return;
> > > > +    }
> > > I.e. you imply that guests won't put their page tables in MMIO? That's
> > > fragile imo; I have seen OSes to use video frame buffers for all kinds
> > > of (transient) purposes, for example.
> > I think it is okay for now and if it will a real use case then an update
> > of this code will be needed.
> May I then ask that you leave a remark (maybe even fixme) to this effect?
Sure, I will then add the following to the comment above if ():

     * FIXME: This assumes that guest page tables never reside in an 
emulated
     * MMIO region, i.e. that an implicit access faulting at G-stage always
     * targets an unpopulated GPA. Guests may (even transiently) place page
     * tables in MMIO-backed memory, e.g. a video frame buffer. Supporting
     * that would require walking the VS-stage page tables in software,
     * accessing the PTEs (including A/D updates) through the MMIO 
handlers,
     * and then emulating the original access, instead of injecting a 
fault.
> > > > +    resolve_faulting_gpa(&gf);
> > > Since the function is only a stub right now - how is one to tell whether
> > > this indeed can never fail?
> > It can't be tell. But what is wrong if it could fail? (Actually with
> > current implementation introduced in later patches you can find it can
> > fail if a necessary extension or software page walk isn't introduced).
> Well, quite obviously if it can fail, its return value would need checking
> here.
That what I thought about after I sent my e-mail as a possible option.

I will update the prototype to:

+static int resolve_faulting_gpa(struct guest_fault *gf)
 {
-    BUG_ON("unimplemented");
+    return -EOPNOTSUPP;
 }

And handle an error code in the following way:

@@ -136,7 +144,8 @@ void handle_guest_page_fault(struct cpu_user_regs 
*regs, unsigned long cause
)
         return;
     }

-    resolve_faulting_gpa(&gf);
+    if ( rc = resolve_faulting_gpa(&gf) )
+        goto out;

     switch ( cause )
     {
@@ -163,6 +172,7 @@ void handle_guest_page_fault(struct cpu_user_regs 
*regs, unsigned long cause
)
         break;
     }

+ out:
     if ( rc )
         domain_crash(current->domain,

and then in the next patch "[PATCH v2 21/39] xen/riscv: resolve the 
faulting guest physical address" I will do "return -EOPNOTSUPP" instead 
of panic():
 -        panic("Shtvala isn't supported by h/w; s/w VS-stage walk 
required\n");
++    {
++        printk_once(XENLOG_WARNING
++                    "Shtvala isn't supported by h/w; s/w VS-stage walk 
required\n");
++        return -EOPNOTSUPP;
++    }

~ Oleksii