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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
>>>
>>> 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".

Jan