Re: [PATCH v1 15/17] xen/riscv: implement trap redirection to a guest

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

On 8/12/26 6:03 PM, Jan Beulich wrote:
> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>> Some traps taken by Xen on behalf of a guest can't or shouldn't be
>> handled by the hypervisor and must be forwarded to the guest's own
>> S-mode exception handler instead: e.g. when riscv_vcpu_unpriv_read()
>> faults while accessing guest memory, or when emulation hits a condition
>> only the guest kernel can resolve.
> 
> Is the plan to use riscv_vcpu_unpriv_read() also for reading hypercall
> buffers?

Yes, it could also be used to read hypercall buffers, but I don't think 
it's the best option, as hypercall buffers could be larger than 8 bytes 
(which is the size supported by the `hlv` instruction on the RV64 
platform). For that case, I think it would be better to map the Xen page 
corresponding to the GVA of the hypercall buffer and then use the usual 
memcpy(). So, basically, use copy_guest() on RISC-V for that purpose.


> In that case trap redirection shouldn't come into play.

It isn't mandatory to perform a redirection in the case of 
riscv_vcpu_unpriv_read(), so if trap redirection shouldn't happen for 
hypercall buffers, then the caller of riscv_vcpu_unpriv_read() needs to 
handle that properly by checking utrap.cause. Something like:

```
     *insn = riscv_vcpu_unpriv_read(true, regs->sepc, &utrap);
     if ( utrap.scause )
     {
         ...
         utrap.sepc = regs->sepc;
         utrap.stval = utrap.sepc;

         riscv_vcpu_trap_redirect(&utrap);

         return true;
     }
```

So, if this cannot happen in the case of a hypercall buffer, then we 
need to return -EFAULT in the if ( utrap.scause ) case.

I don't think I understand why redirection shouldn't come into play. Do 
you mean that the hypercall buffer will always be available, and that it 
is impossible for the hlv instruction to fail, so there is no point in 
handling redirection at all in this case?

> 
>> Introduce riscv_vcpu_trap_redirect() for that purpose. It makes the
>> trap appear to the guest as if it had been taken directly in VS-mode:
>> the trap information is transferred to the guest's virtual supervisor
>> CSRs and the vCPU is resumed at its exception vector in supervisor
>> mode, following the trap entry rules of the RISC-V privileged
>> specification.
>>
>> The implementation is based on kvm_riscv_vcpu_trap_redirect() from
>> Linux, with a few deviations:
>>   - The function reads and writes physical VS-mode CSRs, so it is only
>>     meaningful for the currently running vCPU. Instead of taking a
>>     struct vcpu argument, it always operates on current.
>>   - The MODE field of vstvec is masked off explicitly when computing the
>>     exception target PC (exceptions always vector to BASE), rather than
>>     relying on the hardwired zero bit of sepc to drop it on VM entry.
>>   - Assertions document the preconditions: the trap must have been taken
>>     from virtualized mode (hstatus.SPV set), and only synchronous
>>     exceptions may be redirected - interrupts must be injected via hvip
>>     instead, so that the hardware performs VS-mode trap entry itself,
>>     respecting vsstatus.SIE and vectored vstvec dispatch.
> 
> For this last bullet point - how is a reviewer supposed to validate the
> assertions added when no caller of the new function exists?

My bad (again). The caller appears in "[PATCH v1 16/17] xen/riscv: add 
guest load emulation for trapped MMIO accesses" (as you already know) so 
I have to re-order patches and put this patch after PATCH v1 16/17.

> 
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>> ---
>>   xen/arch/riscv/guestcopy.c                | 54 +++++++++++++++++++++++
>>   xen/arch/riscv/include/asm/guest_access.h |  2 +
>>   2 files changed, 56 insertions(+)
> 
> I don't understand this placement - trap redirection has nothing
> (directly) to do with accessing guest memory.

Agree, at some point. I put trap redirection there as the idea was to 
catch trap from hlv{x} instructions and redirect some of them to guest 
so I put it to guestcopy.h.

I will put inside traps.{c,h} instead.

> 
>> --- a/xen/arch/riscv/guestcopy.c
>> +++ b/xen/arch/riscv/guestcopy.c
>> @@ -205,3 +205,57 @@ unsigned long riscv_vcpu_unpriv_read(bool read_insn,
>>   
>>       return val;
>>   }
>> +
>> +/* Redirect trap to Guest. */
>> +void riscv_vcpu_trap_redirect(const struct trap_info *trap)
>> +{
>> +    struct cpu_user_regs *regs = vcpu_guest_cpu_user_regs(current);
>> +    unsigned long vsstatus = csr_read(CSR_VSSTATUS);
>> +
>> +    /*
>> +     * Redirecting a trap makes sense only if the trap was taken from
>> +     * virtualized mode, i.e. sret is going to return to VS-mode.
>> +     */
>> +    ASSERT(regs->hstatus & HSTATUS_SPV);
>> +
>> +    /*
>> +     * Only synchronous exceptions can be redirected. Interrupts must be
>> +     * injected via hvip instead, so that the hardware itself performs
>> +     * VS-mode trap entry, respecting vsstatus.SIE and the vectored
>> +     * dispatch (BASE + 4 * cause) if vstvec is configured so.
>> +     */
>> +    ASSERT(!(trap->scause & CAUSE_IRQ_FLAG));
>> +
>> +    /* Change Guest SSTATUS.SPP bit */
>> +    vsstatus &= ~SSTATUS_SPP;
>> +    if ( regs->sstatus & SSTATUS_SPP )
>> +        vsstatus |= SSTATUS_SPP;
>> +
>> +    /* Change Guest SSTATUS.SPIE bit */
>> +    vsstatus &= ~SSTATUS_SPIE;
>> +    if ( vsstatus & SSTATUS_SIE )
>> +        vsstatus |= SSTATUS_SPIE;
>> +
>> +    /* Clear Guest SSTATUS.SIE bit */
>> +    vsstatus &= ~SSTATUS_SIE;
>> +
>> +    /* Update Guest SSTATUS */
>> +    csr_write(CSR_VSSTATUS, vsstatus);
>> +
>> +    /* Update Guest SCAUSE, STVAL, and SEPC */
>> +    csr_write(CSR_VSCAUSE, trap->scause);
>> +    csr_write(CSR_VSTVAL, trap->stval);
>> +    csr_write(CSR_VSEPC, trap->sepc);
>> +
>> +    /*
>> +     * Set Guest PC to Guest exception vector.
>> +     *
>> +     * vstvec[1:0] is the vector MODE, not part of the address. Exceptions
>> +     * always target BASE regardless of MODE, so mask it off explicitly
>> +     * instead of relying on the hardwired zero bit of sepc to drop it.
>> +     */
>> +    regs->sepc = csr_read(CSR_VSTVEC) & ~0x3UL;
> 
> Can there be a proper constant please for this mask?

Sure, I will introduce one.

Thanks.

~ Oleksii