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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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.

Jan