diff mbox series

[3/4] KVM: X86: Use smp_rmb() to pair with smp_wmb() in mmu_try_to_unsync_pages()

Message ID 20211019110154.4091-4-jiangshanlai@gmail.com (mailing list archive)
State New, archived
Headers show
Series KVM: X86: Improve guest TLB flushing | expand

Commit Message

Lai Jiangshan Oct. 19, 2021, 11:01 a.m. UTC 
From: Lai Jiangshan <laijs@linux.alibaba.com>

The commit 578e1c4db2213 ("kvm: x86: Avoid taking MMU lock in
kvm_mmu_sync_roots if no sync is needed") added smp_wmb() in
mmu_try_to_unsync_pages(), but the corresponding smp_load_acquire()
isn't used on the load of SPTE.W which is impossible since the load of
SPTE.W is performed in the CPU's pagetable walking.

This patch changes to use smp_rmb() instead.  This patch fixes nothing
but just comments since smp_rmb() is NOP and compiler barrier() is not
required since the load of SPTE.W is before VMEXIT.

Cc: Junaid Shahid <junaids@google.com>
Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kvm/mmu/mmu.c | 47 +++++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 14 deletions(-)

Comments

Lai Jiangshan Oct. 21, 2021, 2:32 a.m. UTC | #1 
Hello Junaid Shahid

Any comments on the patch?  I may have been misunderstanding many things and I
have been often.

thanks
Lai

On 2021/10/19 19:01, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> The commit 578e1c4db2213 ("kvm: x86: Avoid taking MMU lock in
> kvm_mmu_sync_roots if no sync is needed") added smp_wmb() in
> mmu_try_to_unsync_pages(), but the corresponding smp_load_acquire()
> isn't used on the load of SPTE.W which is impossible since the load of
> SPTE.W is performed in the CPU's pagetable walking.
> 
> This patch changes to use smp_rmb() instead.  This patch fixes nothing
> but just comments since smp_rmb() is NOP and compiler barrier() is not
> required since the load of SPTE.W is before VMEXIT.
> 
> Cc: Junaid Shahid <junaids@google.com>
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
Paolo Bonzini Oct. 21, 2021, 5:44 p.m. UTC | #2 
On 19/10/21 13:01, Lai Jiangshan wrote:
> From: Lai Jiangshan<laijs@linux.alibaba.com>
> 
> The commit 578e1c4db2213 ("kvm: x86: Avoid taking MMU lock in
> kvm_mmu_sync_roots if no sync is needed") added smp_wmb() in
> mmu_try_to_unsync_pages(), but the corresponding smp_load_acquire()
> isn't used on the load of SPTE.W which is impossible since the load of
> SPTE.W is performed in the CPU's pagetable walking.
> 
> This patch changes to use smp_rmb() instead.  This patch fixes nothing
> but just comments since smp_rmb() is NOP and compiler barrier() is not
> required since the load of SPTE.W is before VMEXIT.

I think that even implicit loads during pagetable walking obey read-read 
ordering on x86, but this is clearer and it is necessary for patch 4.

Paolo
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c6ddb042b281..900c7a157c99 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2665,8 +2665,9 @@  int mmu_try_to_unsync_pages(struct kvm_vcpu *vcpu, struct kvm_memory_slot *slot,
 	 *     (sp->unsync = true)
 	 *
 	 * The write barrier below ensures that 1.1 happens before 1.2 and thus
-	 * the situation in 2.4 does not arise. The implicit barrier in 2.2
-	 * pairs with this write barrier.
+	 * the situation in 2.4 does not arise.  The implicit read barrier
+	 * between 2.1's load of SPTE.W and 2.3 (as in is_unsync_root()) pairs
+	 * with this write barrier.
 	 */
 	smp_wmb();
 
@@ -3629,6 +3630,35 @@  static int mmu_alloc_special_roots(struct kvm_vcpu *vcpu)
 #endif
 }
 
+static bool is_unsync_root(hpa_t root)
+{
+	struct kvm_mmu_page *sp;
+
+	/*
+	 * Even if another CPU was marking the SP as unsync-ed simultaneously,
+	 * any guest page table changes are not guaranteed to be visible anyway
+	 * until this VCPU issues a TLB flush strictly after those changes are
+	 * made.  We only need to ensure that the other CPU sets these flags
+	 * before any actual changes to the page tables are made.  The comments
+	 * in mmu_try_to_unsync_pages() describe what could go wrong if this
+	 * requirement isn't satisfied.
+	 *
+	 * To pair with the smp_wmb() in mmu_try_to_unsync_pages() between the
+	 * write to sp->unsync[_children] and the write to SPTE.W, a read
+	 * barrier is needed after the CPU reads SPTE.W (or the read itself is
+	 * an acquire operation) while doing page table walk and before the
+	 * checks of sp->unsync[_children] here.  The CPU has already provided
+	 * the needed semantic, but an NOP smp_rmb() here can provide symmetric
+	 * pairing and richer information.
+	 */
+	smp_rmb();
+	sp = to_shadow_page(root);
+	if (sp->unsync || sp->unsync_children)
+		return true;
+
+	return false;
+}
+
 void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
 {
 	int i;
@@ -3646,18 +3676,7 @@  void kvm_mmu_sync_roots(struct kvm_vcpu *vcpu)
 		hpa_t root = vcpu->arch.mmu->root_hpa;
 		sp = to_shadow_page(root);
 
-		/*
-		 * Even if another CPU was marking the SP as unsync-ed
-		 * simultaneously, any guest page table changes are not
-		 * guaranteed to be visible anyway until this VCPU issues a TLB
-		 * flush strictly after those changes are made. We only need to
-		 * ensure that the other CPU sets these flags before any actual
-		 * changes to the page tables are made. The comments in
-		 * mmu_try_to_unsync_pages() describe what could go wrong if
-		 * this requirement isn't satisfied.
-		 */
-		if (!smp_load_acquire(&sp->unsync) &&
-		    !smp_load_acquire(&sp->unsync_children))
+		if (!is_unsync_root(root))
 			return;
 
 		write_lock(&vcpu->kvm->mmu_lock);