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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.10.2026 18:50, Baptiste Le Duc wrote:
>> Implement context_switch() and the helpers it needs: save/restore of
>> H/VS CSRs, virtual timer and P2M context, and __context_switch() in
>> assembly, which switches Xen's own callee-saved state (and thereby the
>> stack) from prev to next. The virtual interrupt controller state isn't
>> switched here.
>>
>> Add offsets of struct arch_vcpu's xen_saved_context to asm-offsets.c for
>> use by __context_switch().
>>
>> henvcfg, htimedelta and vsie are 64-bit on both RV32 and RV64, so store
>> them as uint64_t and access them with csr_{read,write}64(). For that
>> purpose introduce csr_read64().
>>
>> The VMID has to be claimed on the context switch rather than on guest
>> entry: once p2m_ctxt_switch_to() writes HGATP, speculation can populate
>> G-stage entries under whatever VMID is written there. This is done by
>> p2m_vmid_switch_to(), called right before the HGATP write:
>>
>>  - VMIDs are a per-hart resource, so the (generation, vmid) pair of a
>>    vCPU which last ran on another hart is invalidated before a VMID is
>>    claimed for it. The local flush for a wrapped generation moves along
>>    with the claim.
>>  - Nothing is switched in for the idle vCPU, so HGATP keeps pointing at
>>    the p2m of the domain which ran on the hart last. Track that domain
>>    per hart (hgatp_owner) and keep the hart in its dirty_cpumask until a
>>    vCPU of another domain is switched in: p2m_tlb_flush() then goes on
>>    reaching the hart, and a domain which idles between two runs on the
>>    same hart keeps its VMIDs.
>>  - When the owner changes, the hart leaves the old owner's dirty_cpumask
>>    while its TLB may still hold that domain's G-stage translations, so it
>>    is moved to a new VMID generation, which makes none of them reachable
>>    again. It joins the new owner's dirty_cpumask before HGATP is written;
>>    a full barrier there, paired with one in p2m_tlb_flush(), guarantees
>>    that a concurrent P2M change either reaches the hart with an
>>    HFENCE.GVMA or is visible to it before it walks the p2m.
>>
>> That leaves p2m_handle_vmenter() with nothing to do, so drop it together
>> with its call from check_for_pcpu_work(): a VMID is only ever invalidated
>> while its vCPU isn't running, and p2m_tlb_flush() drops stale entries
>> with a remote HFENCE.GVMA rather than by retiring VMIDs. Unlike
>> p2m_handle_vmenter(), HGATP is written unconditionally, as it holds the
>> G-stage root too, which on a context switch belongs to another domain.
>>
>> While at it, fix the inclusion order of headers in asm-offsets.c: Xen's
>> headers go first, then arch specific ones.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>> ---
>> Changes in v3:
>>  - Update the commit message.
>>  - Introduce csr_read64(), as vsie is 64-bit on RV32 too, i.e. has to be
>>    accessed as the vsie/vsieh pair there.
>>  - Make vsie declared as uint64_t instead of register_t.
>>  - Rename the parameter of save_csr_regs() to p and of restore_csr_regs()
>>    to n, to match their callers ctxt_switch_from()/ctxt_switch_to() and the
>>    rest of the context switch helpers (p2m_ctxt_switch_{from,to}(),
>>    vtimer_ctxt_switch_{from,to}()).
>>  - s/save_csr_regs/csr_regs_ctxt_switch_from/
>>  - s/restore_csr_regs/csr_regs_ctxt_switch_to/
>>  - Drop the forward declaration of struct vcpu in <asm/system.h>: the
>>    return type of __context_switch() already declares the tag at file
>>    scope.
>>  - Make the next parameter of __context_switch() pointer-to-const, as
>>    next's saved context is only read.
>>  - Extend the comment above __context_switch(): as ra is switched too, it
>>    doesn't return to its caller, but to where next last called it from
>>    (next's own context_switch(), or continue_new_vcpu() for a vCPU which
>>    has never run).
>>  - Call vtimer_ctxt_switch_to() after csr_regs_ctxt_switch_to(), so that
>>    ctxt_switch_to() restores state in the reverse order of
>>    ctxt_switch_from() saving it.
>>  - Rework p2m's VMID handling in the context switch: track the domain
>>    whose p2m HGATP points at in a per-CPU hgatp_owner, so a pass through
>>    idle no longer drops the hart from that domain's dirty_cpumask nor
>>    burns a VMID generation; move the dirty_cpumask update and the
>>    invalidation of a VMID brought from another hart from schedule_tail()
>>    and ctxt_switch_to() to p2m_ctxt_switch_to(), next to the VMID claim;
>>    add the barriers ordering the dirty_cpumask update against
>>    p2m_tlb_flush().
>>  - Initialise arch_vcpu.last_cpu to CPU_NONE instead of NR_CPUS.
>>  - Mark prev's dirty_cpu clean before ctxt_switch_to() and set current's
>>    after it.
>> ---
>> Changes in v2:
>>  - New patch.
>> ---
>> ---
>>  xen/arch/riscv/domain.c              | 127 ++++++++++++++++++++++
>>  xen/arch/riscv/entry.S               |  48 ++++++++
>>  xen/arch/riscv/include/asm/csr.h     |  13 +++
>>  xen/arch/riscv/include/asm/domain.h  |  15 ++-
>>  xen/arch/riscv/include/asm/p2m.h     |   1 -
>>  xen/arch/riscv/include/asm/system.h  |   2 +
>>  xen/arch/riscv/p2m.c                 | 157 ++++++++++++++++++---------
>>  xen/arch/riscv/riscv64/asm-offsets.c |  19 +++-
>>  xen/arch/riscv/stubs.c               |   5 -
>>  xen/arch/riscv/traps.c               |   2 -
>>  10 files changed, 330 insertions(+), 59 deletions(-)
>>
>> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
>> index fd62296356..f80643e2d8 100644
>> --- a/xen/arch/riscv/domain.c
>> +++ b/xen/arch/riscv/domain.c
>> @@ -11,6 +11,7 @@
>>  #include <asm/bitops.h>
>>  #include <asm/cpufeature.h>
>>  #include <asm/csr.h>
>> +#include <asm/current.h>
>>  #include <asm/intc.h>
>>  #include <asm/mmio.h>
>>  #include <asm/riscv_encoding.h>
>> @@ -152,6 +153,8 @@ int arch_vcpu_create(struct vcpu *v)
>>      if ( is_idle_vcpu(v) )
>>          return 0;
>>  
>> +    v->arch.last_cpu = CPU_NONE;
>> +
>>      vcpu_csr_init(v);
>>  
>>      if ( (rc = vcpu_vtimer_init(v)) )
>> @@ -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);
>> +     */
>> +
>> +    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);
>> +}
>> +
>> +static void ctxt_switch_from(struct vcpu *p)
>> +{
>> +    /*
>> +     * When the idle VCPU is running, Xen will always stay in hypervisor
>> +     * mode.
>> +     * Therefore we don't need to save the context of an idle VCPU.
>> +     */
>> +    if ( is_idle_vcpu(p) )
>> +        return;
>> +
>> +    p2m_ctxt_switch_from(p);
>> +
>> +    vtimer_ctxt_switch_from(p);
>> +
>> +    csr_regs_ctxt_switch_from(p);
>> +}
>> +
>> +static void ctxt_switch_to(struct vcpu *n)
>> +{
>> +    /*
>> +     * When the idle VCPU is running, Xen will always stay in hypervisor
>> +     * mode.
>> +     * Therefore we don't need to restore the context of an idle VCPU.
>> +     */
>> +    if ( is_idle_vcpu(n) )
>> +        return;
>> +
>> +    csr_regs_ctxt_switch_to(n);
>> +
>> +    vtimer_ctxt_switch_to(n);
>> +
>> +    p2m_ctxt_switch_to(n);
>> +}
>> +
>> +static void schedule_tail(struct vcpu *prev)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +
>> +    ASSERT(prev != current);
>> +
>> +    ctxt_switch_from(prev);
>> +
>> +    write_atomic(&prev->dirty_cpu, VCPU_CPU_CLEAN);
>> +
>> +    ctxt_switch_to(current);
>> +
>> +    write_atomic(&current->dirty_cpu, cpu);
>> +
>> +    current->arch.last_cpu = cpu;
>> +
>> +    /*
>> +     * sched_context_switched() internally uses a spinlock,
>> +     * which requires interrupts to be enabled.
>> +     */
>> +    local_irq_enable();
>> +
>> +    sched_context_switched(prev, current);
>> +}
>> +
>> +void context_switch(struct vcpu *prev, struct vcpu *next)
>> +{
>> +    ASSERT(local_irq_is_enabled());
>> +    ASSERT(prev != next);
>> +    ASSERT(!vcpu_cpu_dirty(next));
>> +
>> +    local_irq_disable();
>> +
>> +    set_current(next);
>> +
>> +    prev = __context_switch(prev, next);
>> +
>> +    schedule_tail(prev);
>> +}
>> +
>>  static void __init __maybe_unused build_assertions(void)
>>  {
>>      /*
>> diff --git a/xen/arch/riscv/entry.S b/xen/arch/riscv/entry.S
>> index 202a35fb03..017fbb0626 100644
>> --- a/xen/arch/riscv/entry.S
>> +++ b/xen/arch/riscv/entry.S
>> @@ -99,3 +99,51 @@ restore_registers:
>>  
>>          sret
>>  END(handle_trap)
>> +
>> +/*
>> + * struct vcpu *__context_switch(struct vcpu *prev, const struct vcpu *next)
>> + *
>> + * This is called on prev's stack, and returns on next's. As ra is
>> + * switched too, it doesn't return to its caller: it returns to where
>> + * next last called it from, i.e. into next's own context_switch(), or,
>> + * for a vCPU which has never run, to continue_new_vcpu() with an empty
>> + * stack.
>> + *
>> + * a0 - prev
>> + * a1 - next
>> + *
>> + * Returns prev in a0
>> + */
>> +FUNC(__context_switch)
>> +        REG_S   s0, VCPU_XEN_SAVED_CONTEXT_S0(a0)
>> +        REG_S   s1, VCPU_XEN_SAVED_CONTEXT_S1(a0)
>> +        REG_S   s2, VCPU_XEN_SAVED_CONTEXT_S2(a0)
>> +        REG_S   s3, VCPU_XEN_SAVED_CONTEXT_S3(a0)
>> +        REG_S   s4, VCPU_XEN_SAVED_CONTEXT_S4(a0)
>> +        REG_S   s5, VCPU_XEN_SAVED_CONTEXT_S5(a0)
>> +        REG_S   s6, VCPU_XEN_SAVED_CONTEXT_S6(a0)
>> +        REG_S   s7, VCPU_XEN_SAVED_CONTEXT_S7(a0)
>> +        REG_S   s8, VCPU_XEN_SAVED_CONTEXT_S8(a0)
>> +        REG_S   s9, VCPU_XEN_SAVED_CONTEXT_S9(a0)
>> +        REG_S   s10, VCPU_XEN_SAVED_CONTEXT_S10(a0)
>> +        REG_S   s11, VCPU_XEN_SAVED_CONTEXT_S11(a0)
>> +        REG_S   sp, VCPU_XEN_SAVED_CONTEXT_SP(a0)
>> +        REG_S   ra, VCPU_XEN_SAVED_CONTEXT_RA(a0)
>> +
>> +        REG_L   s0, VCPU_XEN_SAVED_CONTEXT_S0(a1)
>> +        REG_L   s1, VCPU_XEN_SAVED_CONTEXT_S1(a1)
>> +        REG_L   s2, VCPU_XEN_SAVED_CONTEXT_S2(a1)
>> +        REG_L   s3, VCPU_XEN_SAVED_CONTEXT_S3(a1)
>> +        REG_L   s4, VCPU_XEN_SAVED_CONTEXT_S4(a1)
>> +        REG_L   s5, VCPU_XEN_SAVED_CONTEXT_S5(a1)
>> +        REG_L   s6, VCPU_XEN_SAVED_CONTEXT_S6(a1)
>> +        REG_L   s7, VCPU_XEN_SAVED_CONTEXT_S7(a1)
>> +        REG_L   s8, VCPU_XEN_SAVED_CONTEXT_S8(a1)
>> +        REG_L   s9, VCPU_XEN_SAVED_CONTEXT_S9(a1)
>> +        REG_L   s10, VCPU_XEN_SAVED_CONTEXT_S10(a1)
>> +        REG_L   s11, VCPU_XEN_SAVED_CONTEXT_S11(a1)
>> +        REG_L   sp, VCPU_XEN_SAVED_CONTEXT_SP(a1)
>> +        REG_L   ra, VCPU_XEN_SAVED_CONTEXT_RA(a1)
>> +
>> +        ret
>> +END(__context_switch)

First: None of the reply context above is relevant in your reply. Why
did you keep it? As indicated before, this way you make every reader
scroll through many lines, until they would finally find the first
piece of the actual reply. (I've deliberately kept it all, to show
the percentage of the overall reply that this occupied.)

>> --- a/xen/arch/riscv/include/asm/csr.h
>> +++ b/xen/arch/riscv/include/asm/csr.h
>> @@ -40,6 +40,13 @@
>>      csr_write(csr ## H, v_ >> 32);  \
>>  })
>>  
>> +#define csr_read64(csr)                 \
>> +({                                      \
>> +    uint64_t v_ = csr_read(csr ## H);   \
> This violates Misra 20.12 rule (docs/misra/rules.rst) as we use macro
> parameter `csr` as an operand of ##, while it could be expanded as a
> macro (e.g. csr_read64(CSR_HENVCFG)).

Yes. But: You say nothing as to a possible (and plausible) different
way of coding this. Imo the best way to deal with this is by a
deviation (iirc we already have a few), yet I don't think we're going
to add deviations for RISC-V until we actually are about to routinely
scan the code.

Jan