On 09.09.2026 14:04, Baptiste Le Duc wrote: >> Introduce riscv_read_guest() to allow Xen to safely read guest memory >> using HLV/HLVX instructions while reliably capturing trap context. > >> This is required for instruction fetch emulation and MMIO decoding, where >> Xen must inspect guest memory that may not be directly accessible and may >> fault. >> >> The implementation is based on kvm_riscv_vcpu_unpriv_read() from Linux, >> with one deviation: the hlv/hlvx instructions translate the guest address >> through the live vsatp/hgatp CSRs, i.e. through the address space of the >> currently running vCPU, so the function can only be called safely for >> current. Instead of taking a struct vcpu argument, it always operates on >> current directly. >> >> Signed-off-by: Oleksii Kurochko <[email protected]> >> >> diff --git a/xen/arch/riscv/guestcopy.c b/xen/arch/riscv/guestcopy.c >> index 8a89212e0b..b2327822ac 100644 >> --- a/xen/arch/riscv/guestcopy.c >> +++ b/xen/arch/riscv/guestcopy.c >> @@ -6,6 +6,7 @@ >> #include <xen/string.h> >> >> #include <asm/guest_access.h> >> +#include <asm/traps.h> >> >> #define COPY_from_guest 0U >> #define COPY_to_guest BIT(0, U) >> @@ -114,3 +115,89 @@ unsigned long copy_to_guest_phys(struct domain *d, >> paddr_t gpa, void *buf, >> return copy_guest(buf, gpa, len, GPA_INFO(d), >> COPY_to_guest | COPY_gpa); >> } >> + >> +/* >> + * Read machine word from guest memory >> + * >> + * @guest_addr: Guest address to read >> + * @read_insn: Flag representing whether we are reading instruction >> + * @trap: Output pointer to trap details if something went wrong during read >> + * >> + * The hlv/hlvx instructions translate guest_addr through the live >> + * vsatp/hgatp CSRs, so the read is only meaningful for the address >> + * space of the currently running vCPU. >> + * >> + * At most two halfwords are fetched when @read_insn is true, i.e. encodings >> + * wider than 32 bits are not supported. Such an encoding cannot be >> completed >> + * by calling this function again at @guest_addr + 4: the length check is >> + * applied to the first halfword read, which would then be a continuation of >> + * the instruction rather than its opcode. It is up to the caller to reject >> + * anything that is neither a 16- nor a 32-bit encoding. >> + */ >> +unsigned long riscv_read_guest(unsigned long guest_addr, bool read_insn, >> + struct trap_info *trap) > Nit: every other function in this file / declared in this header > (raw_copy_from_guest, copy_to_guest_phys, ...) has no riscv_ prefix. Why > does this one get it? >> +{ >> + /* >> + * Poison the result: if the very first access faults, the fixup skips >> + * over the loads without writing it. Callers must check trap->scause. >> + */ >> + unsigned long val = ~0UL, tmp; >> > The actual "did a trap happen" contract callers rely on is > trap->scause == 0. If the very first access faults, fixup_exception() will > write scause accordingly to a non-zero value, but nothing in this function > clears trap->scause on the success path.
So perhaps the assumption is that callers pass in a zero-filled struct? Would want (need) spelling out, though. Jan
