Re: [PATCH v2 19/39] xen/riscv: implement trap redirection to a guest

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/8/26 12:01 PM, Oleksii Kurochko wrote:
> On 9/7/26 5:57 PM, Baptiste Le Duc wrote:
> > > Some traps taken by Xen on behalf of a guest can't or shouldn't be 
> > > handled
> > > by the hypervisor and have to be reflected to the guest's own S-mode 
> > > trap
> > > handler instead: the access faults which handle_guest_page_fault() 
> > > injects
> > > for a fault that can never become an emulated access, and, later on, a
> > > fault taken by the hlv/hlvx sequences of riscv_read_guest() while
> > > accessing guest memory on a vCPU's behalf.
> > Access faults from handle_guest_page_fault() aren't "taken by Xen on
> > behalf of a guest" as those are guest-page faults taken directly from the
> > guest's own execution. Xen just decides they can't be emulated and
> > reflects them back as access faults. Only the hlv/hlvx case is Xen
> > trapping on the guest's behalf (Xen itself executes the faulting 
> > access).> Conflating the two under one description makes the paragraph 
> > confusing.
> > Suggest splitting into two:
> >
> >      Two kinds of traps can't or shouldn't be handled by the 
> > hypervisor and
> >      have to be reflected to the guest's own S-mode trap handler instead:
> >
> >      - Traps Xen takes on the guest's behalf: the hlv/hlvx sequences
> >      riscv_read_guest() uses to access guest memory.
> >      - Access faults handle_guest_page_fault() injects for a guest-page
> >      fault that can never become an emulated access.
> Thanks, I'll update original paragraph with what you suggested.>
> > > Implement trap_redirect(), until now a BUG_ON() placeholder, 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.
> > >
> > > Add the STVEC_* definitions needed to tell the BASE and MODE fields of
> > > vstvec apart.
> > >
> > > 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.
> > >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > >
> > > diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h b/xen/arch/ 
> > > riscv/include/asm/riscv_encoding.h
> > > index 2d2e7e11b3..b2071f4758 100644
> > > --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> > > +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> > > @@ -109,6 +109,12 @@
> > >   #define SIP_SSIP            MIP_SSIP
> > >   #define SIP_STIP            MIP_STIP
> > > +/* stvec/vstvec: MODE is bits [1:0], BASE is bits [XLEN-1:2] */
> > > +#define STVEC_MODE_MASK            _UL(0x3)
> > > +#define STVEC_MODE_DIRECT        _UL(0x0)
> > > +#define STVEC_MODE_VECTORED        _UL(0x1)
> > > +#define STVEC_BASE_MASK            (~STVEC_MODE_MASK)
> > Nit: STVEC_MODE_DIRECT and STVEC_MODE_VECTORED aren't used anywhere in
> > this patch (only STVEC_BASE_MASK is). Either use them where you decide
> > exceptions always target BASE regardless of MODE, or drop them until a
> > patch that needs them.
> IMO it is fine to introduce *_DIRECT/VECORED here as they are used 
> implicitly through STVEC_BASE_MASK and thereby it will be better to 
> introduce them here now instead of open-code them and then just update 
> STVEC_BASE_MASK again when *_DIRECT/VECORED will be re-introduced.
Oh, sorry, you are right. STVEC_MODE_DIRECT and STVEC_MODE_VECTORED are 
really not used here (in this implementation). I planned to do:
#define STVEC_MODE_MASK (STVEC_MODE_DIRECT | STVEC_MODE_VECTORED)

But I missed to do in that way.

I will update the defintion of STVEC_MODE_MASK in suggested above way.

~ Oleksii