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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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

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

Jan