Re: [PATCH v2 18/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.
> 
> Two of the situations handled here are already decided, as neither can
> ever be turned into an emulated access:
> 
>  - A fault reported with a pseudoinstruction in htinst was taken on an
>    implicit access made for VS-stage address translation, so htval holds
>    the address of a VS-stage PTE rather than of anything the guest asked
>    for, and the guest physical address behind the original access is not
>    known. This is orthogonal to the cause and can accompany any of the
>    three, which is why it is checked first. scause keeps reporting the
>    type of the original access, and on bare hardware a PTE which cannot
>    be read raises an access fault of exactly that type, so reflect one
>    back to the guest.
> 
>  - A fetch fault means the guest tried to execute from a guest physical
>    address which is unmapped or which G-stage does not allow to be
>    executed. On bare hardware a fetch from physical memory which does
>    not exist, or which may not be executed, raises an instruction access
>    fault, so reflect one back too.
> 
> Explicit loads and stores are where MMIO emulation will hook in.
> 
> Neither of the two paths above consults the p2m first, and neither will
> the MMIO one: RISC-V has no populate-on-demand, no paging and no
> mem_access, so every guest mapping is established eagerly and a G-stage
> fault never denotes a mapping Xen could install to let the faulting
> access complete.
> 
> Both of the helpers this leans on, resolve_faulting_gpa() and
> trap_redirect(), are BUG_ON() placeholders for now, so each of the three
> causes currently takes the host down rather than the domain. That is no
> worse than before this patch, where the same causes fell through to
> do_unexpected_trap() and die(). Implementing the helpers is left to
> later patches.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>

I think the commit message it not very clear as it conflates two
different countable things (the 3 fault causes reported via scause vs.
the pseudoinstruction condition, which is orthogonal and can accompany
any of them), which makes "two situations decided" or "any of the three"
hard to follow on first read. 

Here is a proposition with a split to distinguish fetch-fault and
pseudoinstruction cases into explicit bullets with their outcome stated
("Decided here"), and added spec-mentioned conditions about the
pseudoinstruction's existence conditions:
```
    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.

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

    Both helpers this handler relies on, resolve_faulting_gpa() and
    trap_redirect(), are BUG_ON() placeholders for now, so all three causes
    currently take the host down instead of just the guest. This is no worse
    than before this patch, where these traps fell through to
    do_unexpected_trap() and die().
```

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