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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/11/26 4:00 PM, Jan Beulich wrote:
> 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.
Agree, struct cpu_user_regs * will be required.

But I am looking at how read_guest() is used and it shouldn't be used 
when nested HS trap happen never and it seems like it is too much to 
pass struct cpu_user_regs * just to check that.
I think it is enough just to have:

    case CAUSE_FETCH_GUEST_PAGE_FAULT:
    case CAUSE_LOAD_GUEST_PAGE_FAULT:
    case CAUSE_STORE_GUEST_PAGE_FAULT:
        /*
         * A guest page fault taken in Xen context comes from an hlv/hlvx
         * access made on a vCPU's behalf and is dealt with by the
         * fixup_exception() above, so only a guest can get here.
         */
        BUG_ON(!from_guest);

where we already checked that when riscv_read_guest() is used then it 
isn't nested HS trap.
So it looks like we could just drop the ASSERT() and probably update the 
comment above function and mention that riscv_read_guest() shouldn't be 
called in nested HS trap.
~ Oleksii