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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.09.2026 15:57, Oleksii Kurochko wrote:
> 
> 
> On 9/10/26 5:28 PM, Jan Beulich wrote:
>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>> @@ -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);
>>[...]
>> 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.

It is not, afaict: vcpu_guest_cpu_user_regs(current) will give you the guest
frame no matter what context you're in. For what you want, you'd need to
pass struct cpu_user_regs * into here.

Jan

> Would you prefer to keep this defensive ASSERT (with 
> the updated comment), or drop it as redundant?
> 
> Thanks.
> 
> ~ Oleksii
>