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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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

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

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

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

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

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>