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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

> +static void inject_access_fault(const struct guest_fault *gf)
> +{
> +    struct trap_info utrap = {};
> +
> +    switch ( gf->cause )
> +    {
> +    case CAUSE_FETCH_GUEST_PAGE_FAULT:
> +        utrap.scause = CAUSE_FETCH_ACCESS;
> +        break;
> +
> +    case CAUSE_LOAD_GUEST_PAGE_FAULT:
> +        utrap.scause = CAUSE_LOAD_ACCESS;
> +        break;
> +
> +    case CAUSE_STORE_GUEST_PAGE_FAULT:
> +        utrap.scause = CAUSE_STORE_ACCESS;
> +        break;
> +
> +    default:
> +        domain_crash(current->domain, "Impossible cause (%#lx) in %s?\n",
> +                     gf->cause, __func__);
> +        return;
> +    }
> +
> +    utrap.sepc = gf->regs->sepc;
> +    utrap.stval = gf->stval;

Would there be anything wrong with putting these in utrap's initializer?

> +    trap_redirect(&utrap);
> +}
> +
> +void handle_guest_page_fault(struct cpu_user_regs *regs, unsigned long cause)
> +{
> +    struct guest_fault gf = {
> +        .regs = regs,
> +        .cause = cause,
> +        .htinst = csr_read(CSR_HTINST),
> +        .htval = csr_read(CSR_HTVAL),
> +        .stval = csr_read(CSR_STVAL),
> +        .gpa = INVALID_PADDR,
> +    };

At some point RISC-V code will (very likely) also be scanned for Misra 
violations.
The csr_read()s here violate rule 13.1 ("Initializer lists shall not contain
persistent side effects"), and I think it would be better if such was avoided 
from
the start.

> +    int rc;
> +
> +    /*
> +     * 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.

> +    resolve_faulting_gpa(&gf);

Since the function is only a stub right now - how is one to tell whether
this indeed can never fail?

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

Is there perhaps a comma missing before "or", to help parsing the sentence?

> +        inject_access_fault(&gf);
> +        rc = 0;
> +        break;

Simply "return" instead of the latter two statements?

> +    default:
> +        rc = -EOPNOTSUPP;
> +        ASSERT_UNREACHABLE();

To fit a common pattern, these two lines want to be the other way around.

> +        break;
> +    }
> +
> +    if ( rc )
> +        domain_crash(current->domain,
> +                     "%s: unable to handle guest page fault (cause=%#lx) at "
> +                     "gpa %#"PRIpaddr"\n",

Please avoid wrapping of format strings across lines.

Jan