Re: [PATCH v2 12/39] xen/riscv: implement vCPU context switching

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/10/26 3:29 PM, Jan Beulich wrote:
> On 27.08.2026 17:20, Oleksii Kurochko wrote:
> > +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);
> > +
> > +    save_csr_regs(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;
> > +
> > +    /*
> > +     * If this vCPU last ran on a different pCPU, invalidate its VMID so
> > +     * vmid_handle_vmenter() assigns a fresh one from the current pCPU's pool.
> > +     * Without this, two pCPUs could independently assign the same
> > +     * (generation, vmid) pair, generation counters start at the same value
> > +     * on all pCPUs and increment independently, causing TLB contamination.
> > +     */
> > +    if ( n->arch.last_cpu != smp_processor_id() )
> > +        vmid_flush_vcpu(n);
> I wonder why you need this, when we don't have anything similar in x86/HVM
> (and at the first glance Arm doesn't have anything similar either).
x86 does have the equivalent: vmx_do_resume() calls 
hvm_asid_flush_vcpu() in the active_cpu != smp_processor_id() branch, 
and svm_do_resume() does the same when launch_core != smp_processor_id() 
("Migrating to another ASID domain. Request a new ASID."). The RISC-V 
VMID allocator follows the x86 ASID scheme: VMIDs are a per-pCPU 
resource with a per-pCPU generation, so a (generation, vmid) pair 
obtained on one pCPU means nothing on another. Arm doesn't need this 
because it allocates a single VMID per domain from a global bitmap.
Also, note that I've update a little bit how VMIDs are flushed here [1]
but this check still present IIURC.

[1] 
https://lore.kernel.org/xen-devel/cover.1787838835.git.oleksii.kurochko@xxxxxxxxx/T/#m2c06e58c03a09022af112388be3585bf3ae6e4dc
> > +    vtimer_ctxt_switch_to(n);
> > +
> > +    restore_csr_regs(n);
> > +
> > +    p2m_ctxt_switch_to(n);
> > +}
> In the absenmce of a comment towards the need for this specific order I'd
> expect these three calls to be ordered the opposite of their counterparts
> in ctxt_switch_from().
I will put restore_csr_regs(n) (and rename it to 
csr_regs_ctxt_switch_to(n)) before vtimer_ctxt_switch_to(). There is no 
any specific requirement to be ordered in the way it is now.
> > +static void schedule_tail(struct vcpu *prev)
> > +{
> > +    unsigned int cpu = smp_processor_id();
> > +
> > +    ASSERT(prev != current);
> > +
> > +    ctxt_switch_from(prev);
> > +
> > +    /*
> > +     * Mark this CPU in next domain's dirty cpumasks before calling
> > +     * ctxt_switch_to(). This avoids a race on things like p2m flushing,
> > +     * which is synchronised on that function.
> > +     */
> > +    if ( prev->domain != current->domain )
> > +    {
> > +        cpumask_set_cpu(cpu, current->domain->dirty_cpumask);
> > +
> > +        /*
> > +         * Once this hart drops out of prev's dirty_cpumask it stops being a
> > +         * target of p2m_tlb_flush(), while its TLB may still hold G-stage
> > +         * translations of prev's domain: neither the vCPU which just ran nor
> > +         * any other vCPU of that domain which ran here earlier has had its
> > +         * VMID invalidated. Move the hart to a new VMID generation so that
> > +         * none of them can be reached again.
> > +         *
> > +         * Switching away from the idle vCPU needs no bump: the idle domain
> > +         * has no p2m of its own, and whatever G-stage entries this hart may
> > +         * still hold (or speculatively create while HGATP keeps pointing at
> > +         * the last guest's p2m) are tagged with a VMID which was already made
> > +         * stale when that guest was switched out. Skipping the bump here also
> > +         * avoids burning a generation on every pass through idle.
> > +         */
> > +        if ( !is_idle_vcpu(prev) )
> > +            vmid_flush_hart();
> > +
> > +        cpumask_clear_cpu(cpu, prev->domain->dirty_cpumask);
> > +    }
> > +    write_atomic(&current->dirty_cpu, cpu);
> > +
> > +    ctxt_switch_to(current);
> > +
> > +    write_atomic(&prev->dirty_cpu, VCPU_CPU_CLEAN);
> > +
> > +    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);
> > +}
> __context_switch() switches stacks, which can easily collide with code the
> compiler has emitted. For example, the call to schedule_tail() may not be
> a tail call, and context_switch()'s return address may have been spilled
> to the stack (or into one of the s<N> registers). There's a reason Arm and
> x86 have reset_stack_and_jump().
RISC-V will have reset_stack_and_jump() that too but just introduced 
later and will be used for different use case (in continue_new_vcpu() 
introduced later in this patch series). But as the Arm RISC-V doesn't 
use reset_stack_and_jump() in context_switch().
This follows the Arm model: every vCPU has its own Xen stack, and from 
the incoming vCPU's point of view __context_switch() is ABI-conforming. 
It restores exactly the sp/ra/s0-s11 that vCPU had when it itself called 
__context_switch() from context_switch(). So after the return we are in 
next's own context_switch() frame, and anything the compiler spilled 
there (ra included) belongs to next. The only exception is a vCPU which 
has never run: its ra points at continue_new_vcpu() on an empty stack, 
and that's where reset_stack_and_jump() is needed, as on Arm. I'll make 
continue_new_vcpu() noreturn accordingly. x86 differs because its stacks 
are per-pCPU (IIUC), hence its context_switch() can't return.
Here is some diagram for better understanding:
vCPU A (stack A)                vCPU B (stack B, switched out earlier)
schedule()                      schedule()
 `- context_switch(A, B)         `- context_switch(B, X)
     [A's frame: ra, s-regs]         [B's frame: ra, s-regs]
     `- __context_switch() --------> "returns" here
        (save A, load B)             schedule_tail(prev = A)
                                     ld ra <- B's frame (B's own ra)
                                     ret -> B's sched_context_switch()
                                         -> ... -> back into B


> > --- a/xen/arch/riscv/entry.S
> > +++ b/xen/arch/riscv/entry.S
> > @@ -99,3 +99,47 @@ restore_registers:
> >   
> >           sret
> >   END(handle_trap)
> > +
> > +/*
> > + * struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next)
> > + *
> > + * This is called on prev's stack, and returns on next's.
> With ra being switched it may also return to other than the caller. If
> that's really intended, I think it also needs calling out here.
Yes, it's intended. Normally it returns into next's own context_switch()
(where next itself last called __context_switch()), and for a vCPU
which has never run it returns to continue_new_vcpu(). I'll update the
comment to:

 * 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)
> What about gp and tp?
tp points to this hart's pcpu_info (set up once per hart by
setup_tp()), i.e. it's per-pCPU rather than per-vCPU state.
__context_switch() starts and ends on the same hart, so tp has to be
left alone.

gp isn't used by Xen at all: there's no __global_pointer$ in the
linker script, so no gp-relative relaxation happens, and the compiler
never allocates gp.

Neither of them is callee-saved per the psABI, so there's nothing to
preserve across the call. The guest's gp/tp are part of the guest
state and are going to be saved/restored via cpu_user_regs by the
trap entry/exit path.


> > --- a/xen/arch/riscv/include/asm/system.h
> > +++ b/xen/arch/riscv/include/asm/system.h
> > @@ -76,6 +76,10 @@ static inline bool local_irq_is_enabled(void)
> >   
> >   #define arch_fetch_and_add(x, v) __sync_fetch_and_add(x, v)
> >   
> > +struct vcpu;
> I don't think this is needed, as ...
>
> > +struct vcpu *__context_switch(struct vcpu *prev, struct vcpu *next);
> ... parsing of the return type will make the struct known (before
> parameters are parsed).
Make sense to me. I will drop forward declaration.

> Also - can't next be pointer-to-const?
It could be. I will add  const.

Thanks!

~ Oleksii