Re: [PATCH v3 11/39] xen/riscv: implement vCPU context switching

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 11/10/26 4:52 PM, Jan Beulich wrote:
> On 30.09.2026 18:23, Oleksii Kurochko wrote:
> > @@ -323,6 +326,130 @@ int arch_domain_create(struct domain *d,
> >       return rc;
> >   }
> >   
> > +static void csr_regs_ctxt_switch_from(struct vcpu *p)
> > +{
> > +    /*
> > +     * There is no need to save these CSRs as only hypervisor writes them in
> > +     * csr_regs_ctxt_switch_to() and guest can't access them so they shouldn't
> > +     * be stored here. Keep them commented here just for symmetry with the
> > +     * restore CSRs register part.
> > +     *
> > +     * p->arch.hedeleg = csr_read(CSR_HEDELEG);
> > +     * p->arch.hideleg = csr_read(CSR_HIDELEG);
> > +     * p->arch.henvcfg = csr_read64(CSR_HENVCFG);
> > +     * p->arch.hcounteren = csr_read(CSR_HCOUNTEREN);
> > +     * p->arch.htimedelta = csr_read64(CSR_HTIMEDELTA);
> > +     *
> > +     * if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> > +     *     p->arch.hstateen0 = csr_read(CSR_HSTATEEN0);
> > +     */
> I don't understand this comment. Whether the guest can access a CSR doesn't
> matter very much. If Xen modifies it on the guest's behalf, it may still
> need saving/restoring. The question for each individual one is: Can its
> value (or the guest-affecting fields of it) be reconstructed from other
> state? Or is what it holds global state? Or are p->arch.* fields reliably
> kept in sync with what is stored in the hw register?
The value of none of them can change behind Xen's back: they are HS-mode
CSRs, which the guest can't access, and Xen writes them only in
csr_regs_ctxt_switch_to(), from v->arch.* fields which don't change
after vcpu_csr_init(). So the h/w registers always hold what v->arch.*
does, and reading them back would be a no-op. The one exception is hie,
whose VS-level bits alias the vsie ones the guest can change; these are
covered by saving and restoring vsie (hie being written first).

I'll replace the commented-out code with a comment saying this:

static void csr_regs_ctxt_switch_from(struct vcpu *p)
{
    /*
     * Not saved: hedeleg, hideleg, hie, henvcfg, hcounteren, 
htimedelta and
     * hstateen0 only ever hold what csr_regs_ctxt_switch_to() writes there
     * from v->arch.*, which vcpu_csr_init() sets up once. Neither Xen nor
     * the guest changes them otherwise, but for the VS-level bits of hie,
     * aliases of the vsie bits, which the guest changes through vsie: they
     * are saved and restored as part of vsie.
     */

    p->arch.hvip = csr_read(CSR_HVIP);
...


> > +    p->arch.hvip = csr_read(CSR_HVIP);
> > +
> > +    p->arch.vsstatus = csr_read(CSR_VSSTATUS);
> > +    p->arch.vsie = csr_read64(CSR_VSIE);
> > +    p->arch.vstvec = csr_read(CSR_VSTVEC);
> > +    p->arch.vsscratch = csr_read(CSR_VSSCRATCH);
> > +    p->arch.vscause = csr_read(CSR_VSCAUSE);
> > +    p->arch.vstval = csr_read(CSR_VSTVAL);
> > +    p->arch.vsepc = csr_read(CSR_VSEPC);
> > +}
> > +
> > +static void csr_regs_ctxt_switch_to(struct vcpu *n)
> > +{
> > +    csr_write(CSR_HEDELEG, n->arch.hedeleg);
> > +    csr_write(CSR_HIDELEG, n->arch.hideleg);
> > +    csr_write(CSR_HVIP, n->arch.hvip);
> > +    csr_write64(CSR_HENVCFG, n->arch.henvcfg);
> > +    csr_write(CSR_HCOUNTEREN, n->arch.hcounteren);
> > +    csr_write64(CSR_HTIMEDELTA, n->arch.htimedelta);
> > +
> > +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
> > +        csr_write(CSR_HSTATEEN0, n->arch.hstateen0);
> > +
> > +    csr_write(CSR_VSSTATUS, n->arch.vsstatus);
> > +    csr_write64(CSR_VSIE, n->arch.vsie);
> > +    csr_write(CSR_VSTVEC, n->arch.vstvec);
> > +    csr_write(CSR_VSSCRATCH, n->arch.vsscratch);
> > +    csr_write(CSR_VSCAUSE, n->arch.vscause);
> > +    csr_write(CSR_VSTVAL, n->arch.vstval);
> > +    csr_write(CSR_VSEPC, n->arch.vsepc);
> > +}
> Btw, for both function names possibly s/csr_regs/csrs/ ? Or even just singular?
Good point. Use csrs_ctxt_switch_*() sounds good for me.

> > @@ -72,6 +79,12 @@
> >       (void)csr ## H;                 \
> >       csr_read(csr);                  \
> >   })
> > +
> > +#define csr_read64(csr)             \
> > +({                                  \
> > +    (void)csr ## H;                 \
> I continue to not really understand what use this is.
It will be dropped in the v4.

> > --- a/xen/arch/riscv/include/asm/system.h
> > +++ b/xen/arch/riscv/include/asm/system.h
> > @@ -76,6 +76,8 @@ static inline bool local_irq_is_enabled(void)
> >   
> >   #define arch_fetch_and_add(x, v) __sync_fetch_and_add(x, v)
> >   
> > +struct vcpu *__context_switch(struct vcpu *prev, const struct vcpu *next);
> asmlinkage (at least for Misra's sake)?
I will add.

Thanks.

~ Oleksii