Re: [PATCH v3 17/39] xen/riscv: add guest page fault handling stub

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/20/26 4:59 PM, Baptiste Le Duc wrote:
> > Add a handler for guest page faults and hook it into the trap path,
> > providing the trap-side entry point which will later feed the MMIO
> > dispatch.
Based on your comments below I will update here to:

 Add a handler for guest page faults and hook it into the trap path,
-providing the trap-side entry point which will later feed the MMIO
-dispatch.
+providing the trap-side entry point for the MMIO dispatch.

> > This will be used, for example, to trap accesses to APLIC registers so
> > that a guest can initialize and drive an emulated interrupt controller.
> >
> > A G-stage (stage-2) fault has one of three causes, reported via scause:
> > - Fetch fault: guest tried to execute from a guest-physical address
> >    that is unmapped or that G-stage marks non-executable. Never
> >    emulatable (nothing to emulate a fetch into). On real hardware
> >    this raises an instruction access fault, so Xen reflects the same
> >    fault back to the guest. Decided here.
> >
> > - Load fault / Store fault: left undecided by this patch, this is
> >    where MMIO emulation will hook in later.
> Nit: should drop "in later" as it would be stale in the future. I'd
> suggest:
>    - Load fault / Store fault: this is where MMIO emulation will hook.
>      Not in scope of this patch.
I will re-use your option with small remark:

"this is where MMIO emulation *hooks in*. Not in scope of this patch."

'hooks in' instead of 'will hook'.

> > Any of these three faults can instead be reported via a pseudoinstruction
> > in htinst, when both:
> >
> > (a) the fault occurred on an implicit access Xen made to walk a
> >      VS-stage page table, and
> Xen doesn't access the VS-stage page tables here: the implicit access is
> done by the hardware page-table walker, while translating a guest access
> through the guest's VS-stage page tables. I'd suggest to use spec's
> wording. Moreover, "and" is not needed between both conditions as you said
> above "when both" which already indicates that both should be true.
>
> Here is a proposition:
> ```
>      Any of these three faults can instead be reported via a
>      pseudoinstruction in htinst, when both:
>          (a) the fault is caused by an implicit memory access for VS-stage
>              address translation.
>          (b) htval holds a nonzero value: the guest-physical address of that
>              VS-stage PTE, not of the guest's original access.
> ```
This option better. I will use it.

> > However, none of these paths consult the p2m first, and the future MMIO path
> > won't either: RISC-V has no populate-on-demand, no paging, and no
> > mem_access, so every guest mapping is established eagerly. A G-stage
> > fault therefore never indicates a mapping Xen could lazily resolve to
> > let the access complete.
> >
> > resolve_faulting_gpa() is a stub which always fails for now, so any guest
> > page fault other than one taken on an implicit access for VS-stage
> > translation currently crashes the domain. That one is reflected to the
> > guest through trap_redirect(), which is still a BUG_ON() placeholder
> > though, and so takes the host down. This is no worse than before this
> > patch, where these traps fell through to do_unexpected_trap() and die().
> >
> > Cache the "trap came from the guest" test in a local, as the guest page
> > fault case needs it too.
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > +    default:
> > +        domain_crash(current->domain, "Impossible cause (%#lx) in %s?\n",
> > +                     gf->cause, __func__);
> Nit: please drop __func__, the message is unique enough to find where it
> comes from.
Agree, not to much sense. It will be dropped.

> > +        return;
> > +    }
> > +
> > +    trap_redirect(&utrap);
> > +}
> > +
> > +void handle_guest_page_fault(struct cpu_user_regs *regs, unsigned long cause)
> > +{
> > +    struct guest_fault gf = {
> > +        .regs = regs,
> > +        .cause = cause,
> > +        .gpa = INVALID_PADDR,
> > +    };
> > +    int rc;
> > +
> > +    gf.htinst = csr_read(CSR_HTINST);
> > +    gf.htval = csr_read(CSR_HTVAL);
> > +    gf.stval = csr_read(CSR_STVAL);
> > +
> > +    /*
> > +     * 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)
> Nit: I think this paragraph could be re-written by using what you said
> in the commit message.
> ```
>      htinst reports one of the special pseudoinstruction values when both:
>        (a) the fault is caused by an implicit memory access for VS-stage
>            address translation
>        (b) htval holds a nonzero value: the guest physical address of that
>            VS-stage PTE, not of the guest's original access, which is
>            unknown as VS-stage translation didn't complete.
>
>      No access to emulate then: to match bare hardware behavior, inject an
>      access fault of the ORIGINAL access type (Instruction, Load, or
>      Store/AMO) that initiated the address translation.
> ```
I will use then your option.

> > +     *
> > +     * 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.
> > +     *
> > +     * 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.
> > +     */
> > +    if ( htinst_is_pseudo(gf.htinst) )
> > +    {
> > +        inject_access_fault(&gf);
> > +
> > +        return;
> > +    }
> > +
> > +    rc = resolve_faulting_gpa(&gf);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    switch ( cause )
> > +    {
> > +    case CAUSE_LOAD_GUEST_PAGE_FAULT:
> > +        rc = emulate_load(&gf);
> > +        break;
> > +
> > +    case CAUSE_STORE_GUEST_PAGE_FAULT:
> > +        rc = emulate_store(&gf);
> > +        break;
> > +
> > +    case CAUSE_FETCH_GUEST_PAGE_FAULT:
> > +        /*
> > +         * Guest is trying to reach unmapped/unpopulated, or G-stage PTE 
> > doesn't
> > +         * allow execution (X=0). Generate fetch fault in this case.
> > +         */
> > +        inject_access_fault(&gf);
> > +        return;
> > +
> > +    default:
> > +        ASSERT_UNREACHABLE();
> > +        rc = -EOPNOTSUPP;
> > +
> > +        break;
> > +    }
> > +
> > + out:
> > +    if ( rc )
> > +        domain_crash(current->domain,
> > +                     "%s: unable to handle guest page fault (cause=%#lx) at gpa 
> > %#"PRIpaddr"\n",
> > +                     __func__, cause, gf.gpa);
> Probably __func__ can be dropped as the message is unique enough.
Agree with dropping of __func__.

> > +}
> > diff --git a/xen/arch/riscv/traps.c b/xen/arch/riscv/traps.c
> > @@ -193,6 +194,7 @@ void do_trap(struct cpu_user_regs *cpu_regs)
> >   {
> >       register_t pc = cpu_regs->sepc;
> >       unsigned long cause = csr_read(CSR_SCAUSE);
> > +    bool from_guest = cpu_regs->hstatus & HSTATUS_SPV;
> > @@ -275,3 +289,9 @@ enum mc_disposition arch_do_multicall_call(struct mc_state 
> > *state)
> > +
> > +/* Redirect trap to Guest. */
> This duplicates the comment on the prototype in traps.h, so I'd drop it
> here.
Agree, it is enough to have the comment only in prototype.

Thanks!

~ Oleksii