Re: [PATCH v2 22/39] xen/riscv: add guest memory read helper

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <oleksii.kurochko@xxxxxxxxx>
>>
>> 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