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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

> 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.
> +
> +    /*
> +     * 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);
Just to understand, what is the aim of this check? Is it to be sure this
function has been called during a guest fault and not a nested HS fault?

-- 
Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>