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

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

On 9/7/26 10:17 AM, Jan Beulich wrote:
> On 04.09.2026 16:55, Oleksii Kurochko wrote:
>>
>>
>> On 9/4/26 10:26 AM, Baptiste Le Duc wrote:
>>> As I understand it, a generation wrap doesn't retire a single VMID, it
>>> resets next_vmid to 1, which makes every VMID in 1..max_vmid reusable
>>> again in the new generation. We do a full (local) flush at that point to
>>> avoid two different vCPUs ending up with the same VMID valid at once,
>>> across generations.
>>>
>>> If this is correct, doing a full flush there also throws away entries
>>> for the current vCPU that a local HFENCE.GVMA(vmid) per retired VMID
>>
>> We are doing flushed for the pCPU on which a vCPU is ran.
>>
>>> could have preserved. A local-flush-per-VMID approach could also
>>> reduce how often we need a full flush at all.
>>>
>>> Is there a reason we don't do local flushing instead? I see x86 and KVM
>>> use the same flush-all design on wrap, so I assume there's a reason I'm
>>> missing, I'd like to understand it.
>>
>> What do you mean here by "local flushing instead"? We are doing local flush:
>>
>>       if ( unlikely(need_flush) )
>>           local_hfence_gvma_all();
>>
>> Do you mean why we don't do hfence_gvma only for specific VMID?
>>
>> A vCPU's VMID is valid only while vmid->generation == data->generation
>> (vmid.c:141). Bumping the generation invalidates every vCPU's VMID on
>> this hart simultaneously, so every G-stage entry in the TLB (whatever
>> number it is tagged with) belongs to a (vcpu, vmid) binding that can
>> never be consulted again. Each of those vCPUs will be handed a fresh
>> number on its next vmenter before it can run.
>>
>> That includes the current vCPU, which is the case you're worried about.
>> At the wrap it is being assigned VMID 1, not its previous number, so its
>> old entries are unreachable regardless of whether we flush them.
>> hfence.gvma per retired VMID would preserve them physically but not
>> usefully [A concrete example. vCPU A is running on the hart with
>> VMID=100 in generation G; the TLB holds G-stage entries tagged VMID=100.
>> A wrap occurs: the generation becomes G+1, next_vmid is reset to 1, and
>> A is assigned VMID=1 (vmid.c:154). From that moment on, the hardware
>> looks up translations for A under the tag VMID=1. The entries tagged 100
>> will no longer match anything: A isn't 100 any more, and no one else
>> will be handed 100 until the next wrap.
>> So A loses its warm entries not because we did an hfence.gvma, but
>> because it was renumbered. The flush has nothing to do with it. It
>> merely discards what has already become unreachable.]; they'd just
>> occupy TLB capacity until natural eviction. Preserving them would
>> require a different allocator that keeps a vCPU's number stable across a
>> rollover (Linux/KVM-arm64 style, with an active/reserved set pinning
>> live ASIDs), not a different flush granularity.
>>
>> So x86's hvm_asid_handle_vmenter() and KVM's equivalent aren't doing
>> this out of inertia — with a round-robin generation allocator, the full
>> flush is free of useful collateral damage and strictly cheaper than the
>> alternative. Preserving entries across a rollover is a real
>> optimisation, but it's an allocator change, and IMO worth doing only if
>> profiling shows the wrap flush matters.
>>
>>>
>>>> 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. Virtual interrupt controller context switch will be
>>>> introduced later.
>>>>
>>>> Add offsets of struct arch_vcpu's xen_saved_context to asm-offsets.c for
>>>> use by __context_switch().
>>>>
>>>> henvcfg and htimedelta are 64-bit on both RV32 and RV64, so store them as
>>>> uint64_t and use csr_{read,write}64() instead of open-coding accesses to
>>>> the high halves.
>>>>
>>>> A hart which drops out of a domain's dirty_cpumask stops being a target
>>>> of p2m_tlb_flush() while its TLB may still hold G-stage translations of
>>>> that domain, and neither the vCPU which just ran nor any other vCPU of
>>>> that domain which ran there earlier has had its VMID invalidated. Move
>>>> the hart to a new VMID generation at that point: a VMID number is never
>>>> re-used until a full local flush has happened, hence none of those
>>>> translations can be reached again.
>>>>
>>>> Claim the VMID in p2m_ctxt_switch_to() rather than at the next guest
>>>> entry. VMIDs are a per-hart resource, so the (generation, vmid) pair a
>>>> migrating vCPU brings from another hart is meaningless here and may even
>>>> match this hart's current generation, leaving the vCPU under a VMID owned
>>>> by another domain. ctxt_switch_to() invalidates that pair, but claiming a
>>>> replacement only on guest entry is too late: p2m_ctxt_switch_to() has by
>>>> then already made HGATP live, and speculation can populate G-stage entries
>>>> of the incoming domain under the stale VMID. The local flush for a wrapped
>>>> generation moves along with the claim.
>>>>
>>>> That leaves p2m_handle_vmenter() with nothing to do, so drop it together
>>>> with its call from check_for_pcpu_work(). A VMID can only be invalidated
>>>> while its vCPU isn't running: vmid_flush_vcpu() is called for the vCPU
>>>> being switched in, and vmid_flush_hart() runs either from schedule_tail(),
>>>> ahead of ctxt_switch_to(), or from the wrap path of vmid_handle_vmenter()
>>>> itself. A P2M change on another hart doesn't invalidate it either, as
>>>> p2m_tlb_flush() drops the stale entries directly with
>>>> sbi_remote_hfence_gvma() instead of retiring the VMIDs which tag them. A
>>>> guest therefore always runs under the VMID claimed on its way in, and
>>>> there is nothing left for a guest entry hook to notice.
>>>>
>>>> p2m_handle_vmenter() also skipped the HGATP write when the VMID it claimed
>>>> was unchanged. That isn't carried over: HGATP holds the G-stage root as
>>>> well, and skipping the write is only correct where that root is already
>>>> the incoming domain's. On the guest entry path it is, on the context
>>>> switch path it is not.
>>>>
>>>> While at it, fix the inclusion order of headers in asm-offsets.c: Xen's
>>>> headers go first, then arch specific ones.
>>> This could have a dedicated patch no?
>>
>> It could but considering that it is pretty small fix I think it could be
>> part of this patch. If you are insisting on moving that to separate
>> patch I will happy to do that.
>>
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>>>
>>>> diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
>>>> index ec327a5e8a..91a46d630f 100644
>>>> --- a/xen/arch/riscv/domain.c
>>>> +++ b/xen/arch/riscv/domain.c
>>>> @@ -11,9 +11,11 @@
>>>>    #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>
>>>> +#include <asm/vmid.h>
>>>>    #include <asm/vtimer.h>
>>>>    
>>>>    struct csr_masks {
>>>> @@ -158,6 +160,8 @@ int arch_vcpu_create(struct vcpu *v)
>>>>        if ( is_idle_vcpu(v) )
>>>>            return 0;
>>>>    
>>>> +    v->arch.last_cpu = NR_CPUS;
>>>> +
>>>>        vcpu_csr_init(v);
>>>>    
>>>>        if ( (rc = vcpu_vtimer_init(v)) )
>>>> @@ -329,6 +333,169 @@ int arch_domain_create(struct domain *d,
>>>>        return rc;
>>>>    }
>>>>    
>>>> +static void save_csr_regs(struct vcpu *vcpu)
>>>> +{
>>>> +    /*
>>>> +     * There is no need to save these CSRs as only hypervisor writes them in
>>>> +     * restore_csr_regs() 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.
>>>> +     *
>>>> +     * vcpu->arch.hedeleg = csr_read(CSR_HEDELEG);
>>>> +     * vcpu->arch.hideleg = csr_read(CSR_HIDELEG);
>>>> +     * vcpu->arch.henvcfg = csr_read64(CSR_HENVCFG);
>>>> +     * vcpu->arch.hcounteren = csr_read(CSR_HCOUNTEREN);
>>>> +     * vcpu->arch.htimedelta = csr_read64(CSR_HTIMEDELTA);
>>>> +     *
>>>> +     * if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
>>>> +     *     vcpu->arch.hstateen0 = csr_read(CSR_HSTATEEN0);
>>>> +     */
>>>> +
>>>> +    vcpu->arch.hvip = csr_read(CSR_HVIP);
>>>> +
>>>> +    vcpu->arch.vsstatus = csr_read(CSR_VSSTATUS);
>>>> +    vcpu->arch.vsie = csr_read(CSR_VSIE);
>>>
>>>
>>>> +    vcpu->arch.vstvec = csr_read(CSR_VSTVEC);
>>>> +    vcpu->arch.vsscratch = csr_read(CSR_VSSCRATCH);
>>>> +    vcpu->arch.vscause = csr_read(CSR_VSCAUSE);
>>>> +    vcpu->arch.vstval = csr_read(CSR_VSTVAL);
>>>> +    vcpu->arch.vsepc = csr_read(CSR_VSEPC);
>>>> +}
>>>> +
>>>> +static void restore_csr_regs(struct vcpu *vcpu)
>>>> +{
>>>> +    csr_write(CSR_HEDELEG, vcpu->arch.hedeleg);
>>>> +    csr_write(CSR_HIDELEG, vcpu->arch.hideleg);
>>>> +    csr_write(CSR_HVIP, vcpu->arch.hvip);
>>>> +    csr_write64(CSR_HENVCFG, vcpu->arch.henvcfg);
>>>> +    csr_write(CSR_HCOUNTEREN, vcpu->arch.hcounteren);
>>>> +    csr_write64(CSR_HTIMEDELTA, vcpu->arch.htimedelta);
>>>> +
>>>> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_smstateen) )
>>>> +        csr_write(CSR_HSTATEEN0, vcpu->arch.hstateen0);
>>>> +
>>>> +    csr_write(CSR_VSSTATUS, vcpu->arch.vsstatus);
>>>> +    csr_write(CSR_VSIE, vcpu->arch.vsie);
>>>
>>>
>>>> +    csr_write(CSR_VSTVEC, vcpu->arch.vstvec);
>>>> +    csr_write(CSR_VSSCRATCH, vcpu->arch.vsscratch);
>>>> +    csr_write(CSR_VSCAUSE, vcpu->arch.vscause);
>>>> +    csr_write(CSR_VSTVAL, vcpu->arch.vstval);
>>>> +    csr_write(CSR_VSEPC, vcpu->arch.vsepc);
>>>> +}
>>>> +
>>>> +static void ctxt_switch_from(struct vcpu *p)
>>> Is it expected to have diverse names for the vcpu arg? Above it's vcpu,
>>> here it's p (I assume it's for `previous` but I think the _from alone is
>>> enough to understand) and below it's n. Shouldn't be better to keep the same name?
>>
>> Above should be used n.
> 
> Why would that be? n in such contexts stands for "next", while here
> it can only be "previous".

Sorry for confusion.

I meant for restore_csr_regs() and correpondingly p for save_csr_regs().

I will also renaim the function to cxt_switch_to/from style.

~ Oleksii