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
