Re: [PATCH v1 13/17] xen/riscv: add unprivileged guest memory read helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> Introduce riscv_vcpu_unpriv_read() to allow Xen to safely read guest memory
> using HLV/HLVX instructions while reliably capturing trap context.

Both for the title and the function name: How does "unprivileged" matter here?
The same functions would be use for reading Dom0's memory, wouldn't they?

> @@ -114,3 +115,93 @@ 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
> + *
> + * @read_insn: Flag representing whether we are reading instruction
> + * @guest_addr: Guest address to read
> + * @trap: Output pointer to trap details
> + *
> + * 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.
> + */
> +unsigned long riscv_vcpu_unpriv_read(bool read_insn,
> +                                     unsigned long guest_addr,

Personally for such a function I'd expect the address to be the main (first)
parameter.

> +                                     struct trap_info *trap)
> +{
> +    unsigned long val, tmp;
> +    unsigned long flags, old_hstatus;
> +
> +    /*
> +     * As hstatus is going to be changed we don't want an interrupt to occur
> +     * with guest's hstatus register.
> +     */

I don't think "guest's hstatus register" is something real. hstatus is
entirely the hypervisor's register, controlling the guest.

> +    local_irq_save(flags);
> +
> +    /*
> +     * The hypervisor virtual-machine load and store instructions are valid
> +     * only in M-mode or HS-mode, or in U-mode when hstatus.HU=1. Each
> +     * instruction performs an explicit memory access as though V=1; i.e.,
> +     * with the address translation and protection, and the endianness,
> +     * that apply to memory accesses in either VS-mode or VU-mode.
> +     * Field SPVP of hstatus controls the privilege level of the access.
> +     * The explicit memory access is done as though in VU-mode when SPVP=0,
> +     * and as though in VS-mode when SPVP=1.
> +     *
> +     * So it is necessary to restore vCPU's hstatus before execution of
> +     * hlv* instruction.
> +     */
> +    old_hstatus = csr_swap(CSR_HSTATUS,
> +                           vcpu_guest_cpu_user_regs(current)->hstatus);

As you're limiting use of the function to the current vCPU, why would hstatus
need fiddling with? The fields of interest aren't being altered between exit
from guest and making it here, are they?

Without that IRQs also wouldn't need turning off (what about NMIs, btw, once
supported on Xen?), which would help real-time use cases (latency here can
otherwise be affected by guests, by wait of forcing exceptions to be raised).

> +    if ( read_insn )
> +    {
> +        asm volatile ( "\n"
> +            "1:\n"
> +            "   hlvx.hu %[val], (%[addr])\n"
> +            ASM_EXTABLE_TRAP_INFO(1b, 3f, %[ti])

Imo labels used for extable entries would better live on the same line as
the insn they mark.

> +            "   andi %[tmp], %[val], 3\n"
> +            "   addi %[tmp], %[tmp], -3\n"
> +            "   bne %[tmp], zero, 3f\n"

Use BNEZ?

> +            "   addi %[addr], %[addr], 2\n"
> +            "\n"
> +            "2:\n"
> +            "   hlvx.hu %[tmp], (%[addr])\n"
> +            ASM_EXTABLE_TRAP_INFO(2b, 3f, %[ti])
> +            "   sll %[tmp], %[tmp], 16\n"
> +            "   add %[val], %[val], %[tmp]\n"

May I suggest OR instead of ADD?

> +            "3:\n"

If this is an insn wider than 32 bits, you won't have fetched all of it.
I think you want to at least add a comment here indicating that e.g. it's
the callers responsibility to deal with that. (How they would do that is
entirely unclear to me, as they can't simply invoke this function again
passing guest_addr + 4.)

> +        : [val] "=&r" (val), [tmp] "=&r" (tmp), [addr] "+&r" (guest_addr)
> +        : [ti] "r" (trap) : "memory" );

You want to tell the compiler that *trap is written. Instead I don't see
why a memory clobber would be needed: You access a different address space,
i.e. nothing the compiler can make any assumptions about.

You also need to take precautions for not returning an uninitialized "val".
I think the variable wants initializing (perhaps to ~0) and "+r" wants
using as constraint. (Afaik & isn't necessary to use together with +.)

> +        /*
> +         * Although HLVX instructions' explicit memory accesses require 
> execute
> +         * permissions, they still raise the same exceptions as other load
> +         * instructions, rather than raising fetch exceptions instead.
> +         */
> +        if ( trap->scause == CAUSE_LOAD_PAGE_FAULT )
> +            trap->scause = CAUSE_FETCH_PAGE_FAULT;
> +    }
> +    else
> +    {
> +        asm volatile ( "\n"
> +            "1:\n"
> +#ifdef CONFIG_RISCV_64
> +            "hlv.d %[val], (%[addr])\n"
> +#else
> +            "hlv.w %[val], (%[addr])\n"
> +#endif

Once again please use enough care that RV128 would at least obviously fail to
build, rather than building something which then doesn't work.

> +            "2:\n"
> +            ASM_EXTABLE_TRAP_INFO(1b, 2b, %[ti])
> +        : [val] "=&r" (val)
> +        : [addr] "r" (guest_addr), [ti] "r" (trap) : "memory" );
> +    }
> +
> +    csr_write(CSR_HSTATUS, old_hstatus);
> +
> +    local_irq_restore(flags);
> +
> +    return val;
> +}
For both reads and fetches - are there no alignment constraints at all on the
incoming guest_addr?

Jan