I've updated the part of handling of VMID for p2m during context switch
as some things were still missed. This one implementation looks more
correct to me.
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 0ad851ee0f5f..c05d6f8abaaa 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -404,16 +404,6 @@ static void ctxt_switch_to(struct vcpu *n)
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);
-
vtimer_ctxt_switch_to(n);
restore_csr_regs(n);
@@ -421,6 +411,15 @@ static void ctxt_switch_to(struct vcpu *n)
p2m_ctxt_switch_to(n);
}
+/*
+ * Domain whose p2m this hart's HGATP points at. ctxt_switch_to() bails out
+ * early for the idle vCPU, so HGATP survives a pass through idle and keeps
+ * pointing at the domain which ran here last. That domain, rather than the
+ * one the scheduler switched away from, is what owns this hart's G-stage
+ * translations.
+ */
+static DEFINE_PER_CPU(struct domain *, hgatp_owner);
+
static void schedule_tail(struct vcpu *prev)
{
unsigned int cpu = smp_processor_id();
@@ -429,40 +428,88 @@ static void schedule_tail(struct vcpu *prev)
ctxt_switch_from(prev);
+ write_atomic(&prev->dirty_cpu, VCPU_CPU_CLEAN);
+
/*
- * 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.
+ * Switching to the idle vCPU leaves HGATP alone, so this hart
keeps both
+ * the G-stage translations of its owner and its place in that domain's
+ * dirty_cpumask: p2m_tlb_flush() goes on reaching it, and a domain
which
+ * idles between two runs on the same hart keeps its VMIDs.
*/
- if ( prev->domain != current->domain )
+ if ( !is_idle_vcpu(current) )
+ {
+ struct domain *owner = this_cpu(hgatp_owner);
+
+ if ( owner != current->domain )
+ {
+ /*
+ * Once this hart drops out of the owner's dirty_cpumask it
stops
+ * being a target of p2m_tlb_flush(), while its TLB may
still hold
+ * G-stage translations of that domain: none of the vCPUs
of that
+ * domain which ran here has had its VMID invalidated. Move the
+ * hart to a new VMID generation so that none of them can be
+ * reached again.
+ */
+ if ( owner )
+ {
+ vmid_flush_hart();
+
+ cpumask_clear_cpu(cpu, owner->dirty_cpumask);
+ }
+
+ /*
+ * Mark this hart in the incoming domain's dirty_cpumask before
+ * ctxt_switch_to() points HGATP at its p2m. This avoids a
race on
+ * things like p2m flushing, which is synchronised on that
+ * function.
+ */
+ cpumask_set_cpu(cpu, current->domain->dirty_cpumask);
+
+ /*
+ * Pairs with the barrier in p2m_tlb_flush().
cpumask_set_cpu() is
+ * an unordered AMO on RISC-V, so without this a concurrent
flusher
+ * could read the mask without this hart in it while this
hart is
+ * already walking the p2m it is about to be pointed at.
+ */
+ smp_mb();
+
+ this_cpu(hgatp_owner) = current->domain;
+ }
+ }
+
+ if ( !is_idle_vcpu(current) )
{
- cpumask_set_cpu(cpu, current->domain->dirty_cpumask);
+ bool need_flush;
+
+ /*
+ * A VMID is meaningful only on the hart whose pool issued it:
+ * generations are per-hart counters which all start at 1 and
advance
+ * independently, so the pair a vCPU brings from another hart
may match
+ * this hart's generation by coincidence, leaving the vCPU
under a VMID
+ * which is live here for someone else.
+ */
+ if ( current->arch.last_cpu != cpu )
+ vmid_flush_vcpu(current);
/*
- * 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.
+ * Claim the VMID here rather than leaving it to the next guest
entry:
+ * ctxt_switch_to() makes HGATP live below, and a stale VMID there
+ * pairs this domain's G-stage root with a tag which may
already have
+ * been re-issued to a vCPU of another domain.
*/
- if ( !is_idle_vcpu(prev) )
- vmid_flush_hart();
+ need_flush = vmid_handle_vmenter(¤t->arch.vmid);
- cpumask_clear_cpu(cpu, prev->domain->dirty_cpumask);
+ /*
+ * A VMID isn't re-used until the generation it was issued in
wraps, so
+ * a G-stage flush is needed only when vmid_handle_vmenter()
says so.
+ */
+ if ( unlikely(need_flush) )
+ local_hfence_gvma_all();
}
- write_atomic(¤t->dirty_cpu, cpu);
ctxt_switch_to(current);
- write_atomic(&prev->dirty_cpu, VCPU_CPU_CLEAN);
+ write_atomic(¤t->dirty_cpu, cpu);
current->arch.last_cpu = cpu;
diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
index 1f7a6907525d..98c2d6de6933 100644
--- a/xen/arch/riscv/p2m.c
+++ b/xen/arch/riscv/p2m.c
@@ -243,6 +243,15 @@ static void p2m_tlb_flush(struct p2m_domain *p2m)
p2m->need_flush = false;
+ /*
+ * Order the p2m updates above against the read of dirty_cpumask below,
+ * pairing with the barrier in schedule_tail(). Either that hart is
seen
+ * here and gets an HFENCE.GVMA, or it adds itself to the mask
afterwards,
+ * in which case it starts walking this p2m only once the updates are
+ * visible to it.
+ */
+ smp_mb();
+
sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
}
@@ -1523,22 +1532,12 @@ void p2m_ctxt_switch_from(struct vcpu *p)
void p2m_ctxt_switch_to(struct vcpu *n)
{
struct p2m_domain *p2m = p2m_get_hostp2m(n->domain);
- bool need_flush;
if ( is_idle_vcpu(n) )
return;
- need_flush = vmid_handle_vmenter(&n->arch.vmid);
-
csr_write(CSR_HGATP, construct_hgatp(p2m, n->arch.vmid.vmid));
- /*
- * A VMID isn't re-used until the generation it was issued in wraps, so
- * a G-stage flush is needed only when vmid_handle_vmenter() says so.
- */
- if ( unlikely(need_flush) )
- local_hfence_gvma_all();
-
csr_write(CSR_VSATP, n->arch.vsatp);
/*
Any concerns about this implementation?
Thanks in advance.
~ Oleksii
