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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

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@gmail.com/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