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

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

On 9/10/26 5:28 PM, Jan Beulich wrote:
> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>> --- 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)
>> +{
>> +    /*
>> +     * 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;
>> +
>> +    /*
>> +     * hlv/hlvx use hstatus.SPVP for the privilege of the access, and the
>> +     * live vsatp/hgatp for the translation. Xen never installs a value of
>> +     * its own in hstatus (it is only saved on trap entry and restored
>> +     * before sret) and it doesn't reschedule before returning to the
>> +     * guest, so all three still belong to the vCPU which trapped.
>> +     *
>> +     * Check the saved copy rather than the live CSR: a nested trap taken
>> +     * from HS-mode clears hstatus.SPV in the CSR (but leaves SPVP alone).
>> +     */
>> +    ASSERT(vcpu_guest_cpu_user_regs(current)->hstatus & HSTATUS_SPV);
> 
> The first paragraph talks of just hstatus.SPVP. The second paragraph then
> starting "Check ..." means that still refers to hstatus.SPVP, when - aiui -
> hstatus.SPV is meant.
> 
> Furthermore, instead of special casing nested faults here, but otherwise
> saying "Xen doesn't modify", wouldn't it be better to word things such
> that they remain correct if Xen ends up having a need to touch some other
> part of hstatus (including, potentially, SPV)? IOW - I think it is natural
> that the original guest value is checked.

You are right:
1. The transition between the two paragraphs was confusing regarding 
SPVP vs SPV. I will update the comment to clearly distinguish that 
hlv/hlvx rely on SPVP, whereas the ASSERT verifies SPV.

2. Re-phrasing this around the invariant that vcpu_guest_cpu_user_regs 
represents the guest state at trap entry makes much more sense and is 
future-proof against any potential changes to live hstatus manipulation 
in Xen.

3. Regarding the value of the ASSERT: yes, for any valid guest trap 
frame SPV must be set. The ASSERT serves as a sanity check to ensure 
riscv_read_guest() is never accidentally invoked outside a valid guest 
vCPU trap context.I will update the comment as follows in v3:

/*
      * hlv/hlvx instructions use the live hstatus.SPVP for access 
privilege,
      * and live vsatp/hgatp for translation. Since Xen does not reschedule
      * before returning to the guest, these CSRs still belong to current.
      *
      * Check SPV in the saved guest registers rather than the live CSR:
      * the saved copy reflects the guest virtualization mode (V=1) at 
the time
      * of trap entry, which remains invariant even if nested traps or 
HS-mode
      * execution modify the live CSR.
      */
     ASSERT(vcpu_guest_cpu_user_regs(current)->hstatus & HSTATUS_SPV);

> Question being of how much value
> that checking is: vcpu_guest_cpu_user_regs(current)->hstatus can't possibly
> have SPV clear, can it? Only nested exception frames could.

Given that vcpu_guest_cpu_user_regs(current)->hstatus will always have 
SPV set for any valid guest trap frame, the ASSERT is purely a defensive 
sanity check to ensure riscv_read_guest() is never called outside a 
guest trap context. Would you prefer to keep this defensive ASSERT (with 
the updated comment), or drop it as redundant?

Thanks.

~ Oleksii