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

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

On 9/8/26 4:10 PM, Jan Beulich wrote:
> 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.

I think it can't be pointer-to-const as emulate_load/store functions 
wants to change PC register after MMIO access emulation is finished to 
not trap again.

Regarding the comment itself I agree that it isn't fully true right now 
because there is no trap from guest and so no guest registers 
save/restore but it will for sure be and we can't not to save/restore 
guest registers when do_trap() happens.

I will re-word it to:

/* The guest register state */

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

I will write it simpler	 then:

/*
  * Is @htinst one of the special pseudoinstruction values, reported for 
a guest
  * page fault taken on an implicit memory access done for VS-stage address
  * translation?
  *
  * It is enough to check only bits[1:0] as according to the spec:
  *
  * The value is one of the special pseudoinstructions defined later, all of
  * which have bits 1:0 equal to 00.
  */
static bool htinst_is_pseudo(unsigned long htinst)
{
     return htinst && ((htinst & 3) == 0);
}

> 
>> +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?

It could be initializers. I will use 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.

I will do the following then:

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);

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

I think it is okay for now and if it will a real use case then an update 
of this code will be needed.

> 
>> +    resolve_faulting_gpa(&gf);
> 
> Since the function is only a stub right now - how is one to tell whether
> this indeed can never fail?

It can't be tell. But what is wrong if it could fail? (Actually with 
current implementation introduced in later patches you can find it can 
fail if a necessary extension or software page walk isn't introduced).

If we can't resolve faulting GPA address then we can't continue to work 
and so at least domain should be crashed.

> 
>> +    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?

I will add one.

> 
>> +        inject_access_fault(&gf);
>> +        rc = 0;
>> +        break;
> 
> Simply "return" instead of the latter two statements?

It makes sense. I will do just return.

Thanks.

~ Oleksii