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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> +    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().

> +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().

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

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

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

Also - can't next be pointer-to-const?

Jan