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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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(&current->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(&current->dirty_cpu, cpu);

     ctxt_switch_to(current);

-    write_atomic(&prev->dirty_cpu, VCPU_CPU_CLEAN);
+    write_atomic(&current->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