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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com>
>> +    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