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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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. It is what usually is used in Xen in such cases. 
vcpu isn't the best one name in general as it should be v. But for these 
functions I will use n and p correspondignly.
Thanks for noticing that.

~ Oleksii