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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 8/12/26 5:30 PM, Jan Beulich wrote:
> 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?

Unprivileged because HLV/HLVX reads guest memory as if it were accessed 
from a less-privileged (guest) context, rather than by the hypervisor in 
HS mode.

I think I am okay generally to drop "unpriv..." from the function name.

> The same functions would be use for reading Dom0's memory, wouldn't they?

Yes, I don't see any issue to let this function to read DomO's memory 
too. But Dom0 could be counted as "unprivileged" too as it is executed 
in VS-mode which is less-privileged then HS-mode.

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

Agree, it will be better. I will update prototype of the function.

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

Agree, the wording is incorrect what I meant it is that we don't want to 
corrupt hstatus which was saved during guest exit to hypervisor.

I will just put the following comment "As hstatus is going to be changed 
we don't want an interrupt to change it".

> 
>> +    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?

You're right, it doesn't. handle_trap() only saves hstatus into the trap 
frame on entry and restores it before sret; nothing in between installs 
a hypervisor-specific value. So the guest's hstatus (in particular SPVP, 
the only field HLV cares about here (HU only matters in U-mode) ) is 
still live when we get here. Traps taken from HS-mode update only SPV 
and GVA, neither of which affects HLV.

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

Correct, and I'll drop local_irq_save() too. In fact it is already a 
no-op at both call sites: do_trap() runs with interrupts disabled by the 
trap entry itself. And even with the hstatus write in place it would not 
have been needed: any nested trap goes through the same entry path, 
which saves and restores hstatus (an NMI path built the same way would 
be safe for the same reason, rather than relying on interrupts being 
masked).

What I will do instead is document and assert the actual precondition: 
the function may only be called on the trap-handling path of the current 
vCPU, before returning to the guest. That is where hstatus, vsatp and 
hgatp are guaranteed to still be that vCPU's. do_trap() reaches 
check_for_pcpu_work(), and hence any reschedule, only after handling is 
done.

The following check I will add instead csr_swap() and local_irq_save():
   ASSERT(vcpu_guest_cpu_user_regs(current)->hstatus & HSTATUS_SPV);

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

I will add the following comment above the function:

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

> (How they would do that is
> entirely unclear to me, as they can't simply invoke this function again
> passing guest_addr + 4.)

Then it will be needed to update the code of riscv_unpriv_read().

For now we could something like:

        /*
          * Only two halfwords are fetched, so an encoding wider than 32 
bits
          * would have been truncated. Report it as illegal with a zero 
stval:
          * a nonzero one would have to hold the actual faulting 
instruction,
          * whereas zero simply means the value isn't provided.
          */
         if ( !INSN_IS_16BIT(insn) && !INSN_IS_32BIT(insn) )
             return truly_illegal_insn(v, 0);

> 
>> +        : [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.

memory clobber tells the compiler that the assembly code performs memory 
reads or writes to items other than those listed in the input and output 
operands and so I don't tell here that *trap will be changed.

Why this understanding is wrong?

Alternative, I think, could be:
         : [val] "+r" (val), "+m" (*trap)
         : [addr] "r" (guest_addr), [ti] "r" (trap) );
And then memory clobber could be dropped.



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

I agree with '+' if we will initialize val with some value.

Regarding, '&' my understanding is that I have to use it always when

> 
>> +        /*
>> +         * 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.

Sure, I will do the following:

#if defined(CONFIG_RISCV_64)
             "hlv.d %[val], (%[addr])\n"
#elif defined(CONFIG_RISCV_32)
             "hlv.w %[val], (%[addr])\n"
#else
              #error "unsupported RISC-V variant: no hlv for a machine word"
#endif

> 
>> +            "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?
> 

For the fetch path there is an implicit constraint, but the architecture
guarantees it: guest_addr is always the guest's sepc, and IALIGN is 16
bits (32 without the C extension), so it cannot be odd, the guest would
have taken an instruction-address-misaligned exception before we ever 
saw this trap. hlvx.hu is then a naturally aligned halfword access, and
advancing by 2 preserves that.

For the data read there is deliberately no constraint: HLV behaves as 
the guest's own access would, so on a hart which handles misaligned 
accesses it simply works, and on one which doesn't it raises 
load-address-misaligned, which the exception table turns into 
trap->scause for the caller to redirect.

But then it will be need to:

--- a/xen/arch/riscv/traps.c
+++ b/xen/arch/riscv/traps.c
@@ -565,51 +565,73 @@ static void do_unexpected_trap(const struct 
cpu_user_regs *regs)
  void do_trap(struct cpu_user_regs *cpu_regs)
  {
      register_t pc = cpu_regs->sepc;
      unsigned long cause = csr_read(CSR_SCAUSE);

+    /*
+     * A synchronous trap taken while Xen itself was running may come 
from an
+     * access done on a vCPU's behalf, e.g. the hlv/hlvx sequences in
+     * riscv_vcpu_unpriv_read(). Those accesses are covered by 
exception table
+     * entries which record the fault details for the caller and resume
+     * execution past the faulting instruction.
+     *
+     * Interrupts must be excluded here: one taken at an address which 
happens
+     * to be listed in the exception table would otherwise be "fixed 
up" as if
+     * the access itself had faulted, silently skipping it.
+     *
+     * Returning early skips check_for_pcpu_work() below, which is correct:
+     * that only runs for traps taken from the guest.
+     */
+    if ( !(cause & CAUSE_IRQ_FLAG) && !(cpu_regs->hstatus & HSTATUS_SPV) &&
+         fixup_exception(cpu_regs) )
+        return;
+
      switch ( cause )
      {
      case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
          /* CAUSE_VIRTUAL_SUPERVISOR_ECALL should come from VS-mode */
          BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));

          vsbi_handle_ecall(cpu_regs);
          break;

      case CAUSE_LOAD_GUEST_PAGE_FAULT:
      case CAUSE_STORE_GUEST_PAGE_FAULT:
+        /*
+         * Anything not recovered by the exception table above must 
have come
+         * from the guest: a G-stage fault taken in Xen context, e.g. by an
+         * hlv/hlvx not covered by an entry, is a bug.
+         */
+        BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));
+
          handle_guest_page_fault(cause, cpu_regs);
          break;

      case CAUSE_VIRTUAL_INST_FAULT:
      {
          int ret;

          BUG_ON(!(cpu_regs->hstatus & HSTATUS_SPV));

          ret = handle_virt_instruction_fault(current);
          if ( ret < 0 )
              /* TODO: crash only domain instead of Xen? */
              /* domain_crash(current->domain); */
              panic("couldn't handle CAUSE_VIRTUAL_INST_FAULT: %d\n", ret);
          break;
      }

      case CAUSE_ILLEGAL_INSTRUCTION:
          if ( do_bug_frame(cpu_regs, pc) >= 0 )
          {
              if ( !(is_kernel_text(pc) || is_kernel_inittext(pc)) )
              {
                  printk("Something wrong with PC: %#lx\n", pc);
                  die();
              }

              cpu_regs->sepc += GET_INSN_LENGTH(*(uint16_t *)pc);

              break;
          }

-        if ( fixup_exception(cpu_regs) )
-            break;
-
          fallthrough;
      default:
          if ( cause & CAUSE_IRQ_FLAG )
          {
              /* Handle interrupt */

Does it make sense?

Thanks.

~ Oleksii