KVM: Dirty ring interface

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9593

Quoted 8 lines · Peter Xu

arch/x86/kvm/x86.c +7 -1

-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.
+ */

In practice the address is still protected by the refcount, isn't it? Only destroying the VM could invalidate it.

Originally, we have three code paths that can dirty a page without vcpu context for X86:

- init_rmode_identity_map
- init_rmode_tss
- kvmgt_rw_gpa

init_rmode_identity_map and init_rmode_tss will be setup on destination VM no matter what (and the guest cannot even see them), so it does not make sense to track them at all.

To do this, allow __x86_set_memory_region() to return the userspace address that just allocated to the caller. Then in both of the functions we directly write to the userspace address instead of calling kvm_write_*() APIs. We need to make sure that we have the slots_lock held when accessing the userspace address.

Another trivial change is that we don't need to explicitly clear the identity page table root in init_rmode_identity_map() because no matter what we'll write to the whole page with 4M huge page entries.

Suggested-by Paolo Bonzini <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  3 +-
 arch/x86/kvm/svm.c              |  3 +-
 arch/x86/kvm/vmx/vmx.c          | 68 ++++++++++++++++-----------------
 arch/x86/kvm/x86.c              | 18 +++++++--
 4 files changed, 51 insertions(+), 41 deletions(-)

arch/x86/include/asm/kvm_host.h +0 -0

index eb6673c7d2e3..f536d139b3d2 100644

arch/x86/include/asm/kvm_host.h +0 -0

 bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
 bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
 

arch/x86/kvm/svm.c +2 -1

index 8f1b715dfde8..03a344ce7b66 100644
@@ -1698,7 +1698,8 @@ static int avic_init_access_page(struct kvm_vcpu *vcpu)
 	ret = __x86_set_memory_region(kvm,
 				      APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
 				      APIC_DEFAULT_PHYS_BASE,
-				      PAGE_SIZE);
+				      PAGE_SIZE,
+				      NULL);
 	if (ret)
 		goto out;
 

arch/x86/kvm/vmx/vmx.c +0 -0

index 7e3d370209e0..62175a246bcc 100644

arch/x86/kvm/vmx/vmx.c +12 -16

 {
-	gfn_t fn;
+	const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
 	u16 data = 0;
 	int idx, r;
 
-	idx = srcu_read_lock(&kvm->srcu);
-	fn = to_kvm_vmx(kvm)->tss_addr >> PAGE_SHIFT;
-	r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
+	for (idx = 0; idx < 3; idx++) {
+		r = __copy_to_user((void __user *)uaddr + PAGE_SIZE * idx,
+				   zero_page, PAGE_SIZE);
+		if (r)
+			return -EFAULT;
+	}
+
 	data = TSS_BASE_SIZE + TSS_REDIRECTION_SIZE;
-	r = kvm_write_guest_page(kvm, fn++, &data,
-			TSS_IOPB_BASE_OFFSET, sizeof(u16));
-	if (r < 0)
-		goto out;
-	r = kvm_clear_guest_page(kvm, fn++, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
-	r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
+	r = __copy_to_user((void __user *)uaddr + TSS_IOPB_BASE_OFFSET,
+			   &data, sizeof(data));
+	if (r)
+		return -EFAULT;

arch/x86/kvm/vmx/vmx.c +9 -11

 	/* Protect kvm_vmx->ept_identity_pagetable_done. */
 	mutex_lock(&kvm->slots_lock);

@@ -3490,21 +3485,21 @@ static int init_rmode_identity_map(struct kvm *kvm)
 	identity_map_pfn = kvm_vmx->ept_identity_map_addr >> PAGE_SHIFT;
 
 	r = __x86_set_memory_region(kvm, IDENTITY_PAGETABLE_PRIVATE_MEMSLOT,
-				    kvm_vmx->ept_identity_map_addr, PAGE_SIZE);
+				    kvm_vmx->ept_identity_map_addr, PAGE_SIZE,
+				    uaddr);
 	if (r < 0)
 		goto out;
 
-	r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
 	/* Set up identity-mapping pagetable for EPT in real mode */
 	for (i = 0; i < PT32_ENT_PER_PAGE; i++) {
 		tmp = (i << 22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
 			_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
-		r = kvm_write_guest_page(kvm, identity_map_pfn,
-				&tmp, i * sizeof(tmp), sizeof(tmp));
-		if (r < 0)
+		r = __copy_to_user((void __user *)uaddr + i * sizeof(tmp),
+				   &tmp, sizeof(tmp));
+		if (r) {
+			r = -EFAULT;
 			goto out;
+		}
 	}
 	kvm_vmx->ept_identity_pagetable_done = true;
 

@@ -3537,7 +3532,7 @@ static int alloc_apic_access_page(struct kvm *kvm)
 	if (kvm->arch.apic_access_page_done)
 		goto out;
 	r = __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
-				    APIC_DEFAULT_PHYS_BASE, PAGE_SIZE);
+				    APIC_DEFAULT_PHYS_BASE, PAGE_SIZE, NULL);
 	if (r)
 		goto out;
 

@@ -4478,19 +4473,22 @@ static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)
 static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr)
 {
 	int ret;
+	unsigned long *uaddr = NULL;
 
 	if (enable_unrestricted_guest)
 		return 0;
 
 	mutex_lock(&kvm->slots_lock);
 	ret = __x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, addr,
-				      PAGE_SIZE * 3);
-	mutex_unlock(&kvm->slots_lock);
-

arch/x86/kvm/vmx/vmx.c +1 -0

+	return ret;
 }
 
 static int vmx_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c +3 -3

@@ -9651,10 +9661,10 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 		 */
 		mutex_lock(&kvm->slots_lock);
 		__x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
-					0, 0);
+					0, 0, NULL);
 		__x86_set_memory_region(kvm, IDENTITY_PAGETABLE_PRIVATE_MEMSLOT,
-					0, 0);
-		__x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, 0, 0);
+					0, 0, NULL);
+		__x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, 0, 0, NULL);
 		mutex_unlock(&kvm->slots_lock);
 	}
 	if (kvm_x86_ops->vm_destroy)

Signature · 1 line

2.24.1

This code also needs to be tested by doing unrestricted_guest=0 when loading kvm_intel, because it's obviously broken.

__x86_set_memory_region()
takes an "unsigned long *", interpreted as a "pointer to a usersepace address", i.e. a "void __user **". But the callers are treating the param as a "unsigned long in userpace", e.g. init_rmode_identity_map() declares uaddr as an "unsigned long *", when really it should be declaring a straight "unsigned long" and passing "&uaddr". The only thing that saves KVM from dereferencing a bad pointer in __x86_set_memory_region() is that uaddr is initialized to NULL

Quoted 35 lines · Peter Xu

Originally, we have three code paths that can dirty a page without vcpu context for X86:

- init_rmode_identity_map
- init_rmode_tss
- kvmgt_rw_gpa

init_rmode_identity_map and init_rmode_tss will be setup on destination VM no matter what (and the guest cannot even see them), so it does not make sense to track them at all.

To do this, allow __x86_set_memory_region() to return the userspace address that just allocated to the caller. Then in both of the functions we directly write to the userspace address instead of calling kvm_write_*() APIs. We need to make sure that we have the slots_lock held when accessing the userspace address.

Another trivial change is that we don't need to explicitly clear the identity page table root in init_rmode_identity_map() because no matter what we'll write to the whole page with 4M huge page entries.

Suggested-by Paolo Bonzini <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 arch/x86/include/asm/kvm_host.h |  3 +-
 arch/x86/kvm/svm.c              |  3 +-
 arch/x86/kvm/vmx/vmx.c          | 68 ++++++++++++++++-----------------
 arch/x86/kvm/x86.c              | 18 +++++++--
 4 files changed, 51 insertions(+), 41 deletions(-)

arch/x86/include/asm/kvm_host.h +0 -0

index eb6673c7d2e3..f536d139b3d2 100644

arch/x86/include/asm/kvm_host.h void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu); · line 1622

Quoted 6 lines

arch/x86/include/asm/kvm_host.h +2 -1

@@ -1618,7 +1618,8 @@ void __kvm_request_immediate_exit(struct kvm_vcpu *vcpu);
 
 int kvm_is_in_guest(void);
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size);
+int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size,
+			    unsigned long *uaddr);

No need for a new param, just return a "void __user *" (or "void *" if the __user part requires lots of casting) and use ERR_PTR() to encode errors in the return value. I.e. return the userspace address.

The refactoring to return the address should be done in a separate patch as prep work for the move to __copy_to_user().

Quoted 23 lines

arch/x86/include/asm/kvm_host.h +0 -0

 bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
 bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
 

arch/x86/kvm/svm.c +2 -1

index 8f1b715dfde8..03a344ce7b66 100644
@@ -1698,7 +1698,8 @@ static int avic_init_access_page(struct kvm_vcpu *vcpu)
 	ret = __x86_set_memory_region(kvm,
 				      APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
 				      APIC_DEFAULT_PHYS_BASE,
-				      PAGE_SIZE);
+				      PAGE_SIZE,
+				      NULL);
 	if (ret)
 		goto out;
 

arch/x86/kvm/vmx/vmx.c +0 -0

index 7e3d370209e0..62175a246bcc 100644

arch/x86/kvm/vmx/vmx.c static bool guest_state_valid(struct kvm_vcpu *vcpu) · line 3444

Quoted 5 lines

arch/x86/kvm/vmx/vmx.c +1 -1

@@ -3441,34 +3441,28 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu)
 	return true;
 }
 
-static int init_rmode_tss(struct kvm *kvm)
+static int init_rmode_tss(struct kvm *kvm, unsigned long *uaddr)

uaddr is not a pointer to an unsigned long, it's a pointer to a TSS. Given that it's dereferenced as a "void __user *", it's probably best passed as exactly that.

This code also needs to be tested by doing unrestricted_guest=0 when loading kvm_intel, because it's obviously broken. __x86_set_memory_region() takes an "unsigned long *", interpreted as a "pointer to a usersepace address", i.e. a "void __user **". But the callers are treating the param as a "unsigned long in userpace", e.g. init_rmode_identity_map() declares uaddr as an "unsigned long *", when really it should be declaring a straight "unsigned long" and passing "&uaddr". The only thing that saves KVM from dereferencing a bad pointer in __x86_set_memory_region() is that uaddr is initialized to NULL

Quoted 38 lines

arch/x86/kvm/vmx/vmx.c +13 -18

 {
-	gfn_t fn;
+	const void *zero_page = (const void *) __va(page_to_phys(ZERO_PAGE(0)));
 	u16 data = 0;
 	int idx, r;
 
-	idx = srcu_read_lock(&kvm->srcu);
-	fn = to_kvm_vmx(kvm)->tss_addr >> PAGE_SHIFT;
-	r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
+	for (idx = 0; idx < 3; idx++) {
+		r = __copy_to_user((void __user *)uaddr + PAGE_SIZE * idx,
+				   zero_page, PAGE_SIZE);
+		if (r)
+			return -EFAULT;
+	}
+
 	data = TSS_BASE_SIZE + TSS_REDIRECTION_SIZE;
-	r = kvm_write_guest_page(kvm, fn++, &data,
-			TSS_IOPB_BASE_OFFSET, sizeof(u16));
-	if (r < 0)
-		goto out;
-	r = kvm_clear_guest_page(kvm, fn++, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
-	r = kvm_clear_guest_page(kvm, fn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
+	r = __copy_to_user((void __user *)uaddr + TSS_IOPB_BASE_OFFSET,
+			   &data, sizeof(data));
+	if (r)
+		return -EFAULT;
+
 	data = ~0;
-	r = kvm_write_guest_page(kvm, fn, &data,
-				 RMODE_TSS_SIZE - 2 * PAGE_SIZE - 1,

arch/x86/kvm/vmx/vmx.c static bool guest_state_valid(struct kvm_vcpu *vcpu) · line 3466

Quoted 6 lines

arch/x86/kvm/vmx/vmx.c +2 -3

-				 sizeof(u8));
-out:
-	srcu_read_unlock(&kvm->srcu, idx);
+	r = __copy_to_user((void __user *)uaddr - 1, &data, sizeof(data));
+
 	return r;

Why not "return __copy_to_user();"?

arch/x86/kvm/vmx/vmx.c static int init_rmode_identity_map(struct kvm *kvm) · line 3475

Quoted 8 lines

arch/x86/kvm/vmx/vmx.c +1 -0

 }
 

@@ -3478,6 +3472,7 @@ static int init_rmode_identity_map(struct kvm *kvm)
 	int i, r = 0;
 	kvm_pfn_t identity_map_pfn;
 	u32 tmp;
+	unsigned long *uaddr = NULL;

Again, not a pointer to an unsigned long.

Quoted 58 lines

arch/x86/kvm/vmx/vmx.c +11 -12

 	/* Protect kvm_vmx->ept_identity_pagetable_done. */
 	mutex_lock(&kvm->slots_lock);

@@ -3490,21 +3485,21 @@ static int init_rmode_identity_map(struct kvm *kvm)
 	identity_map_pfn = kvm_vmx->ept_identity_map_addr >> PAGE_SHIFT;
 
 	r = __x86_set_memory_region(kvm, IDENTITY_PAGETABLE_PRIVATE_MEMSLOT,
-				    kvm_vmx->ept_identity_map_addr, PAGE_SIZE);
+				    kvm_vmx->ept_identity_map_addr, PAGE_SIZE,
+				    uaddr);
 	if (r < 0)
 		goto out;
 
-	r = kvm_clear_guest_page(kvm, identity_map_pfn, 0, PAGE_SIZE);
-	if (r < 0)
-		goto out;
 	/* Set up identity-mapping pagetable for EPT in real mode */
 	for (i = 0; i < PT32_ENT_PER_PAGE; i++) {
 		tmp = (i << 22) + (_PAGE_PRESENT | _PAGE_RW | _PAGE_USER |
 			_PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_PSE);
-		r = kvm_write_guest_page(kvm, identity_map_pfn,
-				&tmp, i * sizeof(tmp), sizeof(tmp));
-		if (r < 0)
+		r = __copy_to_user((void __user *)uaddr + i * sizeof(tmp),
+				   &tmp, sizeof(tmp));
+		if (r) {
+			r = -EFAULT;
 			goto out;
+		}
 	}
 	kvm_vmx->ept_identity_pagetable_done = true;
 

@@ -3537,7 +3532,7 @@ static int alloc_apic_access_page(struct kvm *kvm)
 	if (kvm->arch.apic_access_page_done)
 		goto out;
 	r = __x86_set_memory_region(kvm, APIC_ACCESS_PAGE_PRIVATE_MEMSLOT,
-				    APIC_DEFAULT_PHYS_BASE, PAGE_SIZE);
+				    APIC_DEFAULT_PHYS_BASE, PAGE_SIZE, NULL);
 	if (r)
 		goto out;
 

@@ -4478,19 +4473,22 @@ static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu)
 static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr)
 {
 	int ret;
+	unsigned long *uaddr = NULL;
 
 	if (enable_unrestricted_guest)
 		return 0;
 
 	mutex_lock(&kvm->slots_lock);
 	ret = __x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, addr,
-				      PAGE_SIZE * 3);
-	mutex_unlock(&kvm->slots_lock);
-
+				      PAGE_SIZE * 3, uaddr);
 	if (ret)
-		return ret;
+		goto out;

arch/x86/kvm/vmx/vmx.c static int vmx_interrupt_allowed(struct kvm_vcpu *vcpu) · line 4490

Quoted 6 lines

arch/x86/kvm/vmx/vmx.c +4 -1

+
 	to_kvm_vmx(kvm)->tss_addr = addr;
-	return init_rmode_tss(kvm);
+	ret = init_rmode_tss(kvm, uaddr);
+out:
+	mutex_unlock(&kvm->slots_lock);

Unnecessary, see below.

Quoted 10 lines

arch/x86/kvm/vmx/vmx.c +1 -0

+	return ret;
 }
 
 static int vmx_set_identity_map_addr(struct kvm *kvm, u64 ident_addr)

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9592

Quoted 10 lines

arch/x86/kvm/x86.c +6 -1

@@ -9584,7 +9584,15 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.

This is all incorrect. Neither of those locks has any bearing on the validity of the hva. slots_lock does as the name suggests and prevents concurrent writes to the memslots. The SRCU lock ensures the implicit memslots lookup in kvm_clear_guest_page() won't result in a use-after-free due to derefencing old memslots.

Neither of those has anything to do with the userspace address, they're both fully tied to KVM's gfn->hva lookup. As Paolo pointed out, KVM's mapping is instead tied to the lifecycle of the VM. Note, even *that* has no bearing on the validity of the mapping or address as KVM only increments mm_count, not mm_users, i.e. guarantees the mm struct itself won't be freed but doesn't ensure the vmas or associated pages tables are valid.

Which is the entire point of using __copy_{to,from}_user(), as they gracefully handle the scenario where the process has not valid mapping and/or translation for the address.

Quoted 29 lines · Sean Christopherson

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9592

Quoted 10 lines

arch/x86/kvm/x86.c +6 -1

@@ -9584,7 +9584,15 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.

This is all incorrect. Neither of those locks has any bearing on the validity of the hva. slots_lock does as the name suggests and prevents concurrent writes to the memslots. The SRCU lock ensures the implicit memslots lookup in kvm_clear_guest_page() won't result in a use-after-free due to derefencing old memslots.

Neither of those has anything to do with the userspace address, they're both fully tied to KVM's gfn->hva lookup. As Paolo pointed out, KVM's mapping is instead tied to the lifecycle of the VM. Note, even *that* has no bearing on the validity of the mapping or address as KVM only increments mm_count, not mm_users, i.e. guarantees the mm struct itself won't be freed but doesn't ensure the vmas or associated pages tables are valid.

Quoted 3 lines

Which is the entire point of using __copy_{to,from}_user(), as they gracefully handle the scenario where the process has not valid mapping and/or translation for the address.

Sorry I don't understand.

I do think either the slots_lock or SRCU would protect at least the existing kvm.memslots, and if so at least the previous vm_mmap() return value should still be valid.

I agree that __copy_to_user() will protect us from many cases from process mm pov (which allows page faults inside), but again if the kvm.memslots is changed underneath us then it's another story, IMHO, and that's why we need either the lock or SRCU.

Or are you assuming that (1) __x86_set_memory_region() is only for the 3 private kvm memslots,

and (2) currently the kvm private memory slots will never change after VM is created and before VM is destroyed?

If so, I agree with you. However I don't see why we need to restrict __x86_set_memory_region() with that assumption, after all taking a lock is not expensive in this slow path.

Even if so, we'd better comment above __x86_set_memory_region() about this, so we know that we should not use __x86_set_memory_region() for future kvm internal memslots that are prone to change during VM's lifecycle (while currently it seems to be a very general interface).

Quoted 34 lines · Peter Xu

Quoted 32 lines · Sean Christopherson

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9592

Quoted 10 lines

arch/x86/kvm/x86.c +6 -1

@@ -9584,7 +9584,15 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.

This is all incorrect. Neither of those locks has any bearing on the validity of the hva. slots_lock does as the name suggests and prevents concurrent writes to the memslots. The SRCU lock ensures the implicit memslots lookup in kvm_clear_guest_page() won't result in a use-after-free due to derefencing old memslots.

Neither of those has anything to do with the userspace address, they're both fully tied to KVM's gfn->hva lookup. As Paolo pointed out, KVM's mapping is instead tied to the lifecycle of the VM. Note, even *that* has no bearing on the validity of the mapping or address as KVM only increments mm_count, not mm_users, i.e. guarantees the mm struct itself won't be freed but doesn't ensure the vmas or associated pages tables are valid.

Which is the entire point of using __copy_{to,from}_user(), as they gracefully handle the scenario where the process has not valid mapping and/or translation for the address.

Quoted 5 lines

Sorry I don't understand.

I do think either the slots_lock or SRCU would protect at least the existing kvm.memslots, and if so at least the previous vm_mmap() return value should still be valid.

Nope. kvm->slots_lock only protects gfn->hva lookups, e.g. userspace can munmap() the range at any time.

Quoted 5 lines

I agree that __copy_to_user() will protect us from many cases from process mm pov (which allows page faults inside), but again if the kvm.memslots is changed underneath us then it's another story, IMHO, and that's why we need either the lock or SRCU.

No, again, slots_lock and SRCU only protect gfn->hva lookups.

Quoted 3 lines

Or are you assuming that (1) __x86_set_memory_region() is only for the 3 private kvm memslots,

It's not an assumption, the entire purpose of __x86_set_memory_region() is to provide support for private KVM memslots.

Quoted 2 lines

and (2) currently the kvm private memory slots will never change after VM is created and before VM is destroyed?

No, I'm not assuming the private memslots are constant, e.g. the flow in question, vmx_set_tss_addr() is directly tied to an unprotected ioctl().

KVM's sole responsible for vmx_set_tss_addr() is to not crash the kernel. Userspace is responsible for ensuring it doesn't break its guests, e.g. that multiple calls to KVM_SET_TSS_ADDR are properly serialized.

In the existing code, KVM ensures it doesn't crash by holding the SRCU lock for the duration of init_rmode_tss() so that the gfn->hva lookups in kvm_clear_guest_page() don't dereference a stale memslots array.

In no way
does that ensure the validity of the resulting hva,

e.g. multiple calls to
KVM_SET_TSS_ADDR would race to set vmx->tss_addr and so init_rmode_tss() could be operating on a stale gpa.

Quoted 35 lines · Sean Christopherson

Quoted 34 lines · Peter Xu

Quoted 32 lines · Sean Christopherson

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9592

Quoted 10 lines

arch/x86/kvm/x86.c +6 -1

@@ -9584,7 +9584,15 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.

This is all incorrect. Neither of those locks has any bearing on the validity of the hva. slots_lock does as the name suggests and prevents concurrent writes to the memslots. The SRCU lock ensures the implicit memslots lookup in kvm_clear_guest_page() won't result in a use-after-free due to derefencing old memslots.

Neither of those has anything to do with the userspace address, they're both fully tied to KVM's gfn->hva lookup. As Paolo pointed out, KVM's mapping is instead tied to the lifecycle of the VM. Note, even *that* has no bearing on the validity of the mapping or address as KVM only increments mm_count, not mm_users, i.e. guarantees the mm struct itself won't be freed but doesn't ensure the vmas or associated pages tables are valid.

Which is the entire point of using __copy_{to,from}_user(), as they gracefully handle the scenario where the process has not valid mapping and/or translation for the address.

Quoted 8 lines

Quoted 5 lines

Sorry I don't understand.

I do think either the slots_lock or SRCU would protect at least the existing kvm.memslots, and if so at least the previous vm_mmap() return value should still be valid.

Nope. kvm->slots_lock only protects gfn->hva lookups, e.g. userspace can munmap() the range at any time.

Do we need to consider that? If the userspace did this then it'll corrupt itself, and imho private memory slot is not anything special here comparing to the user memory slots. For example, the userspace can unmap any region after KVM_SET_USER_MEMORY_REGION ioctl even if the region is filled into some of the userspace_addr of kvm_userspace_memory_region, so the cached userspace_addr can be invalid, then kvm_write_guest_page() can fail too with the same reason. IMHO kvm only need to make sure it handles the failure path then it's perfectly fine.

Quoted 6 lines

Quoted 4 lines

I agree that __copy_to_user() will protect us from many cases from process mm pov (which allows page faults inside), but again if the kvm.memslots is changed underneath us then it's another story, IMHO, and that's why we need either the lock or SRCU.

No, again, slots_lock and SRCU only protect gfn->hva lookups.

Yes, then could you further explain why do you think we don't need the slot lock?

Quoted 6 lines

Quoted 2 lines

Or are you assuming that (1) __x86_set_memory_region() is only for the 3 private kvm memslots,

It's not an assumption, the entire purpose of __x86_set_memory_region() is to provide support for private KVM memslots.

Quoted 5 lines

Quoted 2 lines

and (2) currently the kvm private memory slots will never change after VM is created and before VM is destroyed?

No, I'm not assuming the private memslots are constant, e.g. the flow in question, vmx_set_tss_addr() is directly tied to an unprotected ioctl().

Why it's unprotected?

Now vmx_set_tss_add() is protected by the slots lock so concurrent operation is safe, also it'll return -EEXIST if called for more than once.

[1]

Quoted 5 lines

KVM's sole responsible for vmx_set_tss_addr() is to not crash the kernel. Userspace is responsible for ensuring it doesn't break its guests, e.g. that multiple calls to KVM_SET_TSS_ADDR are properly serialized.

Quoted 3 lines

In the existing code, KVM ensures it doesn't crash by holding the SRCU lock for the duration of init_rmode_tss() so that the gfn->hva lookups in kvm_clear_guest_page() don't dereference a stale memslots array.

Here in the current master branch we have both the RCU lock and the slot lock held, that's why I think we can safely remove the RCU lock as long as we're still holding the slots lock. We can't do the reverse because otherwise multiple KVM_SET_TSS_ADDR could race.

Quoted 2 lines

In no way
does that ensure the validity of the resulting hva,

Yes, but as I mentioned, I don't think it's an issue to be considered by KVM, otherwise we should have the same issue all over the places when we fetch the cached userspace_addr from any user slots.

Quoted 3 lines

e.g. multiple calls to
KVM_SET_TSS_ADDR would race to set vmx->tss_addr and so init_rmode_tss() could be operating on a stale gpa.

Please refer to [1].

I just want to double-confirm on what we're discussing now. Are you sure you're suggesting that we should remove the slot lock in init_rmode_tss()? Asked because you discussed quite a bit on how the slot lock should protect GPA->HVA, about concurrency and so on, then I'm even more comfused...

Quoted 45 lines · Peter Xu

Quoted 35 lines · Sean Christopherson

Quoted 34 lines · Peter Xu

Quoted 32 lines · Sean Christopherson

Quoted 5 lines

arch/x86/kvm/x86.c +0 -0

index c4d3972dcd14..ff97782b3919 100644

arch/x86/kvm/x86.c void kvm_arch_sync_events(struct kvm *kvm) · line 9592

Quoted 10 lines

arch/x86/kvm/x86.c +6 -1

@@ -9584,7 +9584,15 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 
-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+/*
+ * If `uaddr' is specified, `*uaddr' will be returned with the
+ * userspace address that was just allocated.  `uaddr' is only
+ * meaningful if the function returns zero, and `uaddr' will only be
+ * valid when with either the slots_lock or with the SRCU read lock
+ * held.  After we release the lock, the returned `uaddr' will be invalid.

This is all incorrect. Neither of those locks has any bearing on the validity of the hva. slots_lock does as the name suggests and prevents concurrent writes to the memslots. The SRCU lock ensures the implicit memslots lookup in kvm_clear_guest_page() won't result in a use-after-free due to derefencing old memslots.

Neither of those has anything to do with the userspace address, they're both fully tied to KVM's gfn->hva lookup. As Paolo pointed out, KVM's mapping is instead tied to the lifecycle of the VM. Note, even *that* has no bearing on the validity of the mapping or address as KVM only increments mm_count, not mm_users, i.e. guarantees the mm struct itself won't be freed but doesn't ensure the vmas or associated pages tables are valid.

Which is the entire point of using __copy_{to,from}_user(), as they gracefully handle the scenario where the process has not valid mapping and/or translation for the address.

Quoted 8 lines

Quoted 5 lines

Sorry I don't understand.

I do think either the slots_lock or SRCU would protect at least the existing kvm.memslots, and if so at least the previous vm_mmap() return value should still be valid.

Nope. kvm->slots_lock only protects gfn->hva lookups, e.g. userspace can munmap() the range at any time.

Quoted 9 lines

Do we need to consider that? If the userspace did this then it'll corrupt itself, and imho private memory slot is not anything special here comparing to the user memory slots. For example, the userspace can unmap any region after KVM_SET_USER_MEMORY_REGION ioctl even if the region is filled into some of the userspace_addr of kvm_userspace_memory_region, so the cached userspace_addr can be invalid, then kvm_write_guest_page() can fail too with the same reason. IMHO kvm only need to make sure it handles the failure path then it's perfectly fine.

Yes? No? My point is that your original comment's assertion that "'uaddr' will only be valid when with either the slots_lock or with the SRCU read lock held." is wrong and misleading.

Quoted 10 lines

Quoted 7 lines

Quoted 5 lines

I agree that __copy_to_user() will protect us from many cases from process mm pov (which allows page faults inside), but again if the kvm.memslots is changed underneath us then it's another story, IMHO, and that's why we need either the lock or SRCU.

No, again, slots_lock and SRCU only protect gfn->hva lookups.

Yes, then could you further explain why do you think we don't need the slot lock?

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Or are you assuming that (1) __x86_set_memory_region() is only for the 3 private kvm memslots,

It's not an assumption, the entire purpose of __x86_set_memory_region() is to provide support for private KVM memslots.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

and (2) currently the kvm private memory slots will never change after VM is created and before VM is destroyed?

No, I'm not assuming the private memslots are constant, e.g. the flow in question, vmx_set_tss_addr() is directly tied to an unprotected ioctl().

Why it's unprotected?

Because it doesn't need to be protected.

Quoted 2 lines

Now vmx_set_tss_add() is protected by the slots lock so concurrent operation is safe, also it'll return -EEXIST if called for more than once.

Returning -EEXIST is an ABI change, e.g. userspace can currently call KVM_SET_TSS_ADDR any number of times, it just needs to ensure proper serialization between calls.

If you want to change the ABI, then submit a patch to do exactly that. But don't bury an ABI change under the pretense that it's a bug fix.

Quoted 12 lines

[1]

Quoted 8 lines

KVM's sole responsible for vmx_set_tss_addr() is to not crash the kernel. Userspace is responsible for ensuring it doesn't break its guests, e.g. that multiple calls to KVM_SET_TSS_ADDR are properly serialized.

In the existing code, KVM ensures it doesn't crash by holding the SRCU lock for the duration of init_rmode_tss() so that the gfn->hva lookups in kvm_clear_guest_page() don't dereference a stale memslots array.

Quoted 4 lines

Here in the current master branch we have both the RCU lock and the slot lock held, that's why I think we can safely remove the RCU lock as long as we're still holding the slots lock. We can't do the reverse because otherwise multiple KVM_SET_TSS_ADDR could race.

Your wording is all messed up. "we have both the RCU lock and the slot lock held" is wrong.

KVM holds slot_lock around __x86_set_memory_region(),
because changing the memslots must be mutually exclusive. It then *drops* slots_lock because it's done writing the memslots and grabs the SRCU lock in order to protect the gfn->hva lookups done by init_rmode_tss(). It *intentionally* drops slots_lock because writing init_rmode_tss() does not need to be a mutually exclusive operation, per KVM's existing ABI.

If KVM held both slots_lock and SRCU then __x86_set_memory_region() would deadlock on synchronize_srcu().

Quoted 3 lines

Quoted 2 lines

In no way
does that ensure the validity of the resulting hva,

Quoted 3 lines

Yes, but as I mentioned, I don't think it's an issue to be considered by KVM, otherwise we should have the same issue all over the places when we fetch the cached userspace_addr from any user slots.

Huh? Of course it's an issue that needs to be considered by KVM, e.g. kvm_{read,write}_guest_cached() aren't using __copy_{to,}from_user() for giggles.

Quoted 5 lines

Quoted 4 lines

e.g. multiple calls to
KVM_SET_TSS_ADDR would race to set vmx->tss_addr and so init_rmode_tss() could be operating on a stale gpa.

Quoted 7 lines

Please refer to [1].

I just want to double-confirm on what we're discussing now. Are you sure you're suggesting that we should remove the slot lock in init_rmode_tss()? Asked because you discussed quite a bit on how the slot lock should protect GPA->HVA, about concurrency and so on, then I'm even more comfused...

Yes, if init_rmode_tss() is provided the hva then it does not need to grab srcu_read_lock(&kvm->srcu) because it can directly call __copy_{to,from}_user() instead of bouncing through the KVM helpers that translate a gfn to hva.

The code can look like this. That being said, I've completely lost track of why __x86_set_memory_region() needs to provide the hva, i.e. have no idea if we *should* do this, or it would be better to keep the current code, which would be slower, but less custom.

static int init_rmode_tss(void __user *hva)

{
	const void *zero_page = (const void *)__va(page_to_phys(ZERO_PAGE(0)));
	u16 data = TSS_BASE_SIZE + TSS_REDIRECTION_SIZE;
	int r;

	r = __copy_to_user(hva, zero_page, PAGE_SIZE);
	if (r)
		return -EFAULT;

	r = __copy_to_user(hva + TSS_IOPB_BASE_OFFSET, &data, sizeof(u16))
	if (r)
		return -EFAULT;

	hva += PAGE_SIZE;
	r = __copy_to_user(hva + PAGE_SIZE, zero_page, PAGE_SIZE);
	if (r)
		return -EFAULT;

	hva += PAGE_SIZE;
	r = __copy_to_user(hva + PAGE_SIZE, zero_page, PAGE_SIZE);
	if (r)
		return -EFAULT;

	data = ~0;
	hva += RMODE_TSS_SIZE - 2 * PAGE_SIZE - 1;
	r = __copy_to_user(hva, &data, sizeof(u16))
	if (r)
		return -EFAULT;
}

static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr)

{
	void __user *hva;

	if (enable_unrestricted_guest)
		return 0;

	mutex_lock(&kvm->slots_lock);
	hva = __x86_set_memory_region(kvm, TSS_PRIVATE_MEMSLOT, addr,
				      PAGE_SIZE * 3);
	mutex_unlock(&kvm->slots_lock);

	if (IS_ERR(hva))
		return PTR_ERR(hva);

	to_kvm_vmx(kvm)->tss_addr = addr;
	return init_rmode_tss(hva);
}

Yes, userspace can corrupt its VM by invoking KVM_SET_TSS_ADDR multiple times without serializing the calls, but that's already true today.

Quoted 3 lines · Sean Christopherson

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

Quoted 4 lines

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

OK so you're suggesting to unlock the lock earlier to not cover init_rmode_tss() rather than dropping the whole lock... Yes it looks good to me. I think that's the major confusion I got.

Quoted 6 lines

Returning -EEXIST is an ABI change, e.g. userspace can currently call KVM_SET_TSS_ADDR any number of times, it just needs to ensure proper serialization between calls.

If you want to change the ABI, then submit a patch to do exactly that. But don't bury an ABI change under the pretense that it's a bug fix.

Could you explain what do you mean by "ABI change"?

I was talking about the original code, not after applying the patchset. To be explicit, I mean [a] below:

int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size,
unsigned long *uaddr)

{
	int i, r;
	unsigned long hva;
	struct kvm_memslots *slots = kvm_memslots(kvm);
	struct kvm_memory_slot *slot, old;

	/* Called with kvm->slots_lock held.  */
	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
		return -EINVAL;

	slot = id_to_memslot(slots, id);
	if (size) {
		if (slot->npages)
			return -EEXIST;  <------------------------ [a]
        }
        ...
}

Quoted 7 lines

Quoted 3 lines

Yes, but as I mentioned, I don't think it's an issue to be considered by KVM, otherwise we should have the same issue all over the places when we fetch the cached userspace_addr from any user slots.

Huh? Of course it's an issue that needs to be considered by KVM, e.g. kvm_{read,write}_guest_cached() aren't using __copy_{to,}from_user() for giggles.

The cache is for the GPA->HVA translation (struct gfn_to_hva_cache), we still use __copy_{to,}from_user() upon the HVAs, no?

Quoted 9 lines · Peter Xu

Quoted 7 lines · Sean Christopherson

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

Quoted 3 lines

OK so you're suggesting to unlock the lock earlier to not cover init_rmode_tss() rather than dropping the whole lock... Yes it looks good to me. I think that's the major confusion I got.

Ya. And I missed where the -EEXIST was coming from. I think we're on the same page.

Quoted 25 lines

Quoted 7 lines

Returning -EEXIST is an ABI change, e.g. userspace can currently call KVM_SET_TSS_ADDR any number of times, it just needs to ensure proper serialization between calls.

If you want to change the ABI, then submit a patch to do exactly that. But don't bury an ABI change under the pretense that it's a bug fix.

Could you explain what do you mean by "ABI change"?

I was talking about the original code, not after applying the patchset. To be explicit, I mean [a] below:

int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size,
unsigned long *uaddr)

{
	int i, r;
	unsigned long hva;
	struct kvm_memslots *slots = kvm_memslots(kvm);
	struct kvm_memory_slot *slot, old;

	/* Called with kvm->slots_lock held.  */
	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
		return -EINVAL;

Quoted 7 lines

	slot = id_to_memslot(slots, id);
	if (size) {
		if (slot->npages)
			return -EEXIST;  <------------------------ [a]
        }
        ...
}

Doh, I completely forgot that the second __x86_set_memory_region() would fail. Sorry :-(

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Yes, but as I mentioned, I don't think it's an issue to be considered by KVM, otherwise we should have the same issue all over the places when we fetch the cached userspace_addr from any user slots.

Huh? Of course it's an issue that needs to be considered by KVM, e.g. kvm_{read,write}_guest_cached() aren't using __copy_{to,}from_user() for giggles.

The cache is for the GPA->HVA translation (struct gfn_to_hva_cache), we still use __copy_{to,}from_user() upon the HVAs, no?

I'm still lost on this one. I'm pretty sure I'm incorrectly interpreting:

I don't think it's an issue to be considered by KVM, otherwise we should have the same issue all over the places when we fetch the cached userspace_addr from any user slots.

What is the issue to which you are referring?

Quoted 14 lines · Sean Christopherson

Quoted 12 lines · Peter Xu

Quoted 7 lines · Sean Christopherson

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

OK so you're suggesting to unlock the lock earlier to not cover init_rmode_tss() rather than dropping the whole lock... Yes it looks good to me. I think that's the major confusion I got.

Quoted 2 lines

Ya. And I missed where the -EEXIST was coming from. I think we're on the same page.

Good to know. Btw, for me I would still prefer to keep the lock be after the __copy_to_user()s because "HVA is valid without lock" is only true for these private memslots.

After all this is super slow path so I wouldn't mind to take the lock for some time longer.

Or otherwise if you really like the unlock() to
be earlier I can comment above the unlock:

/*
* We can unlock before using the HVA only because this KVM private
* memory slot will never change until the end of VM lifecycle. */

Quoted 15 lines · Peter Xu

Quoted 14 lines · Sean Christopherson

Quoted 12 lines · Peter Xu

Quoted 7 lines · Sean Christopherson

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

OK so you're suggesting to unlock the lock earlier to not cover init_rmode_tss() rather than dropping the whole lock... Yes it looks good to me. I think that's the major confusion I got.

Quoted 6 lines

Quoted 2 lines

Ya. And I missed where the -EEXIST was coming from. I think we're on the same page.

Good to know. Btw, for me I would still prefer to keep the lock be after the __copy_to_user()s because "HVA is valid without lock" is only true for these private memslots.

No. From KVM's perspective, the HVA is *never* valid. Even if you rewrote this statement to say "the gfn->hva translation is valid without lock" it would still be incorrect.

KVM is *always* using HVAs without holding lock, e.g. every time it enters the guest it is deferencing a memslot because the translations stored in the TLB are effectively gfn->hva->hpa. Obviously KVM ensures that it won't dereference a memslot that has been deleted/moved, but it's a lot more subtle than simply holding a lock.

Quoted 2 lines

After all this is super slow path so I wouldn't mind to take the lock for some time longer.

Holding the lock doesn't affect this super slow vmx_set_tss_addr(), it affects everything else that wants slots_lock. Now, admittedly it's extremely unlikely userspace is going to do KVM_SET_USER_MEMORY_REGION in parallel, but that's not the point and it's not why I'm objecting to holding the lock.

Holding the lock implies protection that is *not* provided. You and I know it's not needed for copy_{to,from}_user(), but look how long it's taken us to get on the same page. A future KVM developer comes along, sees this code, and thinks "oh, I need to hold slots_lock to dereference a gfn", and propagates the unnecessary locking to some other code.

Quoted 8 lines

Or otherwise if you really like the unlock() to
be earlier I can comment above the unlock:

/*
* We can unlock before using the HVA only because this KVM private
* memory slot will never change until the end of VM lifecycle. */

How about:

/*
* No need to hold slots_lock while filling the TSS, the TSS private
* memslot is guaranteed to be valid until the VM is destroyed, i.e.
* there is no danger of corrupting guest memory by consuming a stale
* gfn->hva lookup.
*/

Quoted 42 lines · Sean Christopherson

Quoted 15 lines · Peter Xu

Quoted 14 lines · Sean Christopherson

Quoted 12 lines · Peter Xu

Quoted 7 lines · Sean Christopherson

For the same reason we don't take mmap_sem, it gains us nothing, i.e. KVM still has to use copy_{to,from}_user().

In the proposed __x86_set_memory_region() refactor, vmx_set_tss_addr() would be provided the hva of the memory region. Since slots_lock and SRCU only protect gfn->hva, why would KVM take slots_lock since it already has the hva?

OK so you're suggesting to unlock the lock earlier to not cover init_rmode_tss() rather than dropping the whole lock... Yes it looks good to me. I think that's the major confusion I got.

Quoted 6 lines

Quoted 2 lines

Ya. And I missed where the -EEXIST was coming from. I think we're on the same page.

Good to know. Btw, for me I would still prefer to keep the lock be after the __copy_to_user()s because "HVA is valid without lock" is only true for these private memslots.

No. From KVM's perspective, the HVA is *never* valid. Even if you rewrote this statement to say "the gfn->hva translation is valid without lock" it would still be incorrect.

KVM is *always* using HVAs without holding lock, e.g. every time it enters the guest it is deferencing a memslot because the translations stored in the TLB are effectively gfn->hva->hpa. Obviously KVM ensures that it won't dereference a memslot that has been deleted/moved, but it's a lot more subtle than simply holding a lock.

Quoted 2 lines

After all this is super slow path so I wouldn't mind to take the lock for some time longer.

Holding the lock doesn't affect this super slow vmx_set_tss_addr(), it affects everything else that wants slots_lock. Now, admittedly it's extremely unlikely userspace is going to do KVM_SET_USER_MEMORY_REGION in parallel, but that's not the point and it's not why I'm objecting to holding the lock.

Quoted 5 lines

Holding the lock implies protection that is *not* provided. You and I know it's not needed for copy_{to,from}_user(), but look how long it's taken us to get on the same page. A future KVM developer comes along, sees this code, and thinks "oh, I need to hold slots_lock to dereference a gfn", and propagates the unnecessary locking to some other code.

At least for a user memory slot, we "need to hold slots_lock to dereference a gfn" (or srcu), right?

You know I'm suffering from a jetlag today, I thought I was still fine, now I start to doubt it. :-)

This patch is heavily based on previous work from Lei Cao <[email protected]> and Paolo Bonzini <[email protected]>. [1]

KVM currently uses large bitmaps to track dirty memory. These bitmaps are copied to userspace when userspace queries KVM for its dirty page information. The use of bitmaps is mostly sufficient for live migration, as large parts of memory are be dirtied from one log-dirty pass to another. However, in a checkpointing system, the number of dirty pages is small and in fact it is often bounded---the VM is paused when it has dirtied a pre-defined number of pages. Traversing a large, sparsely populated bitmap to find set bits is time-consuming, as is copying the bitmap to user-space.

A similar issue will be there for live migration when the guest memory is huge while the page dirty procedure is trivial. In that case for each dirty sync we need to pull the whole dirty bitmap to userspace and analyse every bit even if it's mostly zeros.

The preferred data structure for above scenarios is a dense list of guest frame numbers (GFN).

This patch series stores the dirty list in
kernel memory that can be memory mapped into userspace to allow speedy harvesting.

This patch enables dirty ring for X86 only. However it should be easily extended to other archs as well.

[1] https://patchwork.kernel.org/patch/10471409/

Signed-off-by Lei Cao <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 Documentation/virt/kvm/api.txt  |  89 ++++++++++++++++++
 arch/x86/include/asm/kvm_host.h |   3 +
 arch/x86/include/uapi/asm/kvm.h |   1 +
 arch/x86/kvm/Makefile           |   3 +-
 arch/x86/kvm/mmu/mmu.c          |   6 ++
 arch/x86/kvm/vmx/vmx.c          |   7 ++
 arch/x86/kvm/x86.c              |   9 ++
 include/linux/kvm_dirty_ring.h  |  55 +++++++++++
 include/linux/kvm_host.h        |  26 +++++
 include/trace/events/kvm.h      |  78 +++++++++++++++
 include/uapi/linux/kvm.h        |  33 +++++++
 virt/kvm/dirty_ring.c           | 162 ++++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c             | 137 ++++++++++++++++++++++++++-
 13 files changed, 606 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/kvm_dirty_ring.h
 create mode 100644 virt/kvm/dirty_ring.c

Documentation/virt/kvm/api.txt +31 -0

index ebb37b34dcfc..708c3e0f7eae 100644
@@ -231,6 +231,7 @@ Based on their initialization different VMs may have different capabilities.
 It is thus encouraged to use the vm ioctl to query for capabilities (available
 with KVM_CAP_CHECK_EXTENSION_VM on the vm fd)
 
+
 4.5 KVM_GET_VCPU_MMAP_SIZE
 
 Capability: basic

@@ -243,6 +244,18 @@ The KVM_RUN ioctl (cf.) communicates with userspace via a shared
 memory region.  This ioctl returns the size of that region.  See the
 KVM_RUN documentation for details.
 
+Besides the size of the KVM_RUN communication region, other areas of
+the VCPU file descriptor can be mmap-ed, including:
+
+- if KVM_CAP_COALESCED_MMIO is available, a page at
+  KVM_COALESCED_MMIO_PAGE_OFFSET * PAGE_SIZE; for historical reasons,
+  this page is included in the result of KVM_GET_VCPU_MMAP_SIZE.
+  KVM_CAP_COALESCED_MMIO is not documented yet.
+
+- if KVM_CAP_DIRTY_LOG_RING is available, a number of pages at
+  KVM_DIRTY_LOG_PAGE_OFFSET * PAGE_SIZE.  For more information on
+  KVM_CAP_DIRTY_LOG_RING, see section 8.3.
+
 
 4.6 KVM_SET_MEMORY_REGION
 

@@ -5376,6 +5389,7 @@ CPU when the exception is taken. If this virtual SError is taken to EL1 using
 AArch64, this value will be reported in the ISS field of ESR_ELx.
 
 See KVM_CAP_VCPU_EVENTS for more details.
+
 8.20 KVM_CAP_HYPERV_SEND_IPI
 
 Architectures: x86

@@ -5383,6 +5397,7 @@ Architectures: x86
 This capability indicates that KVM supports paravirtualized Hyper-V IPI send
 hypercalls:
 HvCallSendSyntheticClusterIpi, HvCallSendSyntheticClusterIpiEx.
+
 8.21 KVM_CAP_HYPERV_DIRECT_TLBFLUSH
 
 Architecture: x86

@@ -5396,3 +5411,77 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_DIRTY_LOG_RING
+
+Architectures: x86
+Parameters: args[0] - size of the dirty log ring
+
+KVM is capable of tracking dirty memory using ring buffers that are
+mmaped into userspace; there is one dirty ring per vcpu.
+
+One dirty ring is defined as below internally:
+
+struct kvm_dirty_ring {
+	u32 dirty_index;
+	u32 reset_index;
+	u32 size;
+	u32 soft_limit;

Documentation/virt/kvm/api.txt +8 -0

+        __u32 slot; /* as_id | slot_id */
+        __u64 offset;
+};
+
+Most of the ring structure is used by KVM internally, while only the
+indices are exposed to userspace:
+
+struct kvm_dirty_ring_indices {

Documentation/virt/kvm/api.txt +26 -0

+Just like for dirty page bitmaps, the buffer tracks writes to
+all user memory regions for which the KVM_MEM_LOG_DIRTY_PAGES flag was
+set in KVM_SET_USER_MEMORY_REGION.  Once a memory region is registered
+with the flag set, userspace can start harvesting dirty pages from the
+ring buffer.
+
+To harvest the dirty pages, userspace accesses the mmaped ring buffer
+to read the dirty GFNs up to avail_index, and sets the fetch_index
+accordingly.  This can be done when the guest is running or paused,
+and dirty pages need not be collected all at once.  After processing
+one or more entries in the ring buffer, userspace calls the VM ioctl
+KVM_RESET_DIRTY_RINGS to notify the kernel that it has updated
+fetch_index and to mark those pages clean.  Therefore, the ioctl
+must be called *before* reading the content of the dirty pages.
+
+However, there is a major difference comparing to the
+KVM_GET_DIRTY_LOG interface in that when reading the dirty ring from
+userspace it's still possible that the kernel has not yet flushed the
+hardware dirty buffers into the kernel buffer (which was previously
+done by the KVM_GET_DIRTY_LOG ioctl).  To achieve that, one needs to
+kick the vcpu out for a hardware buffer flush (vmexit) to make sure
+all the existing dirty gfns are flushed to the dirty rings.
+
+If one of the ring buffers is full, the guest will exit to userspace
+with the exit reason set to KVM_EXIT_DIRTY_LOG_FULL, and the KVM_RUN
+ioctl will return to userspace with zero.

arch/x86/include/asm/kvm_host.h +3 -0

index f536d139b3d2..3fe18402e6a3 100644
@@ -1181,6 +1181,7 @@ struct kvm_x86_ops {
 					   struct kvm_memory_slot *slot,
 					   gfn_t offset, unsigned long mask);
 	int (*write_log_dirty)(struct kvm_vcpu *vcpu);
+	int (*cpu_dirty_log_size)(void);
 
 	/* pmu operations of sub-arch */
 	const struct kvm_pmu_ops *pmu_ops;

@@ -1666,4 +1667,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
 #define GET_SMSTATE(type, buf, offset)		\
 	(*(type *)((buf) + (offset) - 0x7e00))
 
+int kvm_cpu_dirty_log_size(void);
+
 #endif /* _ASM_X86_KVM_HOST_H */

arch/x86/include/uapi/asm/kvm.h +1 -0

index 503d3f42da16..b59bf356c478 100644
@@ -12,6 +12,7 @@
 
 #define KVM_PIO_PAGE_OFFSET 1
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 2
+#define KVM_DIRTY_LOG_PAGE_OFFSET 64
 
 #define DE_VECTOR 0
 #define DB_VECTOR 1

arch/x86/kvm/Makefile +2 -1

index b19ef421084d..0acee817adfb 100644
@@ -5,7 +5,8 @@ ccflags-y += -Iarch/x86/kvm
 KVM := ../../../virt/kvm
 
 kvm-y			+= $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o \
-				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o
+				$(KVM)/eventfd.o $(KVM)/irqchip.o $(KVM)/vfio.o \
+				$(KVM)/dirty_ring.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 
 kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \

arch/x86/kvm/mmu/mmu.c +6 -0

index 7269130ea5e2..621b842a9b7b 100644
@@ -1832,7 +1832,13 @@ int kvm_arch_write_log_dirty(struct kvm_vcpu *vcpu)
 {
 	if (kvm_x86_ops->write_log_dirty)
 		return kvm_x86_ops->write_log_dirty(vcpu);
+	return 0;
+}
 
+int kvm_cpu_dirty_log_size(void)
+{
+	if (kvm_x86_ops->cpu_dirty_log_size)
+		return kvm_x86_ops->cpu_dirty_log_size();
 	return 0;
 }
 

arch/x86/kvm/vmx/vmx.c +7 -0

index 62175a246bcc..2151de89456d 100644
@@ -7689,6 +7689,7 @@ static __init int hardware_setup(void)
 		kvm_x86_ops->slot_disable_log_dirty = NULL;
 		kvm_x86_ops->flush_log_dirty = NULL;
 		kvm_x86_ops->enable_log_dirty_pt_masked = NULL;
+		kvm_x86_ops->cpu_dirty_log_size = NULL;
 	}
 
 	if (!cpu_has_vmx_preemption_timer())

@@ -7753,6 +7754,11 @@ static __exit void hardware_unsetup(void)
 	free_kvm_area();
 }
 
+static int vmx_cpu_dirty_log_size(void)
+{
+	return enable_pml ? PML_ENTITY_NUM : 0;
+}
+
 static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 	.cpu_has_kvm_support = cpu_has_kvm_support,
 	.disabled_by_bios = vmx_disabled_by_bios,

@@ -7875,6 +7881,7 @@ static struct kvm_x86_ops vmx_x86_ops __ro_after_init = {
 	.flush_log_dirty = vmx_flush_log_dirty,
 	.enable_log_dirty_pt_masked = vmx_enable_log_dirty_pt_masked,
 	.write_log_dirty = vmx_write_pml_buffer,
+	.cpu_dirty_log_size = vmx_cpu_dirty_log_size,
 
 	.pre_block = vmx_pre_block,
 	.post_block = vmx_post_block,

arch/x86/kvm/x86.c +9 -0

index ff97782b3919..9c3673592826 100644
@@ -7998,6 +7998,15 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	bool req_immediate_exit = false;
 
+	/* Forbid vmenter if vcpu dirty ring is soft-full */
+	if (unlikely(vcpu->kvm->dirty_ring_size &&
+		     kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
+		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
+		trace_kvm_dirty_ring_exit(vcpu);
+		r = 0;
+		goto out;
+	}
+
 	if (kvm_request_pending(vcpu)) {
 		if (kvm_check_request(KVM_REQ_GET_VMCS12_PAGES, vcpu)) {
 			if (unlikely(!kvm_x86_ops->get_vmcs12_pages(vcpu))) {

include/linux/kvm_dirty_ring.h new file +55 -0

new file mode 100644
index 000000000000..d6fe9e1b7617
@@ -0,0 +1,55 @@
+#ifndef KVM_DIRTY_RING_H
+#define KVM_DIRTY_RING_H
+
+/**
+ * kvm_dirty_ring: KVM internal dirty ring structure
+ *
+ * @dirty_index: free running counter that points to the next slot in
+ *               dirty_ring->dirty_gfns, where a new dirty page should go
+ * @reset_index: free running counter that points to the next dirty page
+ *               in dirty_ring->dirty_gfns for which dirty trap needs to
+ *               be reenabled
+ * @size:        size of the compact list, dirty_ring->dirty_gfns
+ * @soft_limit:  when the number of dirty pages in the list reaches this
+ *               limit, vcpu that owns this ring should exit to userspace
+ *               to allow userspace to harvest all the dirty pages
+ * @dirty_gfns:  the array to keep the dirty gfns
+ * @indices:     the pointer to the @kvm_dirty_ring_indices structure
+ *               of this specific ring
+ * @index:       index of this dirty ring
+ */
+struct kvm_dirty_ring {
+	u32 dirty_index;
+	u32 reset_index;
+	u32 size;
+	u32 soft_limit;
+	struct kvm_dirty_gfn *dirty_gfns;
+	struct kvm_dirty_ring_indices *indices;
+	int index;
+};
+
+u32 kvm_dirty_ring_get_rsvd_entries(void);
+int kvm_dirty_ring_alloc(struct kvm_dirty_ring *ring,
+			 struct kvm_dirty_ring_indices *indices,
+			 int index, u32 size);
+struct kvm_dirty_ring *kvm_dirty_ring_get(struct kvm *kvm);
+
+/*
+ * called with kvm->slots_lock held, returns the number of
+ * processed pages.
+ */
+int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring);
+
+/*
+ * returns =0: successfully pushed
+ *         <0: unable to push, need to wait
+ */
+void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset);
+
+/* for use in vm_operations_struct */
+struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset);
+
+void kvm_dirty_ring_free(struct kvm_dirty_ring *ring);
+bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring);
+
+#endif

include/linux/kvm_host.h +26 -0

index cbd633ece959..c96161c6a0c9 100644
@@ -34,6 +34,7 @@
 #include <linux/kvm_types.h>
 
 #include <asm/kvm_host.h>
+#include <linux/kvm_dirty_ring.h>
 
 #ifndef KVM_MAX_VCPU_ID
 #define KVM_MAX_VCPU_ID KVM_MAX_VCPUS

@@ -321,6 +322,7 @@ struct kvm_vcpu {
 	bool ready;
 	struct kvm_vcpu_arch arch;
 	struct dentry *debugfs_dentry;
+	struct kvm_dirty_ring dirty_ring;
 };
 
 static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)

@@ -502,6 +504,7 @@ struct kvm {
 	struct srcu_struct srcu;
 	struct srcu_struct irq_srcu;
 	pid_t userspace_pid;
+	u32 dirty_ring_size;
 };
 
 #define kvm_err(fmt, ...) \

@@ -831,6 +834,8 @@ void kvm_arch_mmu_enable_log_dirty_pt_masked(struct kvm *kvm,
 					gfn_t gfn_offset,
 					unsigned long mask);
 
+void kvm_reset_dirty_gfn(struct kvm *kvm, u32 slot, u64 offset, u64 mask);
+
 int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 				struct kvm_dirty_log *log);
 int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm,

@@ -1409,4 +1414,25 @@ int kvm_vm_create_worker_thread(struct kvm *kvm, kvm_vm_thread_fn_t thread_fn,
 				uintptr_t data, const char *name,
 				struct task_struct **thread_ptr);
 
+/*
+ * This defines how many reserved entries we want to keep before we
+ * kick the vcpu to the userspace to avoid dirty ring full.  This
+ * value can be tuned to higher if e.g. PML is enabled on the host.
+ */
+#define  KVM_DIRTY_RING_RSVD_ENTRIES  64
+
+/* Max number of entries allowed for each kvm dirty ring */
+#define  KVM_DIRTY_RING_MAX_ENTRIES  65536
+
+/*
+ * Arch needs to define these macro after implementing the dirty ring
+ * feature.  KVM_DIRTY_LOG_PAGE_OFFSET should be defined as the
+ * starting page offset of the dirty ring structures, while
+ * KVM_DIRTY_RING_VERSION should be defined as >=1.  By default, this
+ * feature is off on all archs.
+ */
+#ifndef KVM_DIRTY_LOG_PAGE_OFFSET
+#define KVM_DIRTY_LOG_PAGE_OFFSET 0
+#endif
+
 #endif

include/trace/events/kvm.h +78 -0

index 2c735a3e6613..3d850997940c 100644
@@ -399,6 +399,84 @@ TRACE_EVENT(kvm_halt_poll_ns,
 #define trace_kvm_halt_poll_ns_shrink(vcpu_id, new, old) \
 	trace_kvm_halt_poll_ns(false, vcpu_id, new, old)
 
+TRACE_EVENT(kvm_dirty_ring_push,
+	TP_PROTO(struct kvm_dirty_ring *ring, u32 slot, u64 offset),
+	TP_ARGS(ring, slot, offset),
+
+	TP_STRUCT__entry(
+		__field(int, index)
+		__field(u32, dirty_index)
+		__field(u32, reset_index)
+		__field(u32, slot)
+		__field(u64, offset)
+	),
+
+	TP_fast_assign(
+		__entry->index          = ring->index;
+		__entry->dirty_index    = ring->dirty_index;
+		__entry->reset_index    = ring->reset_index;
+		__entry->slot           = slot;
+		__entry->offset         = offset;
+	),
+
+	TP_printk("ring %d: dirty 0x%x reset 0x%x "
+		  "slot %u offset 0x%llx (used %u)",
+		  __entry->index, __entry->dirty_index,
+		  __entry->reset_index,  __entry->slot, __entry->offset,
+		  __entry->dirty_index - __entry->reset_index)
+);
+
+TRACE_EVENT(kvm_dirty_ring_reset,
+	TP_PROTO(struct kvm_dirty_ring *ring),
+	TP_ARGS(ring),
+
+	TP_STRUCT__entry(
+		__field(int, index)
+		__field(u32, dirty_index)
+		__field(u32, reset_index)
+	),
+
+	TP_fast_assign(
+		__entry->index          = ring->index;
+		__entry->dirty_index    = ring->dirty_index;
+		__entry->reset_index    = ring->reset_index;
+	),
+
+	TP_printk("ring %d: dirty 0x%x reset 0x%x (used %u)",
+		  __entry->index, __entry->dirty_index, __entry->reset_index,
+		  __entry->dirty_index - __entry->reset_index)
+);
+
+TRACE_EVENT(kvm_dirty_ring_waitqueue,
+	TP_PROTO(bool enter),
+	TP_ARGS(enter),
+
+	TP_STRUCT__entry(
+	    __field(bool, enter)
+	),
+
+	TP_fast_assign(
+	    __entry->enter = enter;
+	),
+
+	TP_printk("%s", __entry->enter ? "wait" : "awake")
+);
+
+TRACE_EVENT(kvm_dirty_ring_exit,
+	TP_PROTO(struct kvm_vcpu *vcpu),
+	TP_ARGS(vcpu),
+
+	TP_STRUCT__entry(
+	    __field(int, vcpu_id)
+	),
+
+	TP_fast_assign(
+	    __entry->vcpu_id = vcpu->vcpu_id;
+	),
+
+	TP_printk("vcpu %d", __entry->vcpu_id)
+);
+
 #endif /* _TRACE_KVM_MAIN_H */
 
 /* This part must be outside protection */

include/uapi/linux/kvm.h +33 -0

index f0a16b4adbbd..df4a1700ff1e 100644
@@ -236,6 +236,7 @@ struct kvm_hyperv_exit {
 #define KVM_EXIT_IOAPIC_EOI       26
 #define KVM_EXIT_HYPERV           27
 #define KVM_EXIT_ARM_NISV         28
+#define KVM_EXIT_DIRTY_RING_FULL  29
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */

@@ -247,6 +248,13 @@ struct kvm_hyperv_exit {
 /* Encounter unexpected vm-exit reason */
 #define KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON	4
 
+struct kvm_dirty_ring_indices {
+	__u32 avail_index; /* set by kernel */
+	__u32 padding1;
+	__u32 fetch_index; /* set by userspace */
+	__u32 padding2;
+};
+
 /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
 struct kvm_run {
 	/* in */

@@ -421,6 +429,8 @@ struct kvm_run {
 		struct kvm_sync_regs regs;
 		char padding[SYNC_REGS_SIZE_BYTES];
 	} s;
+
+	struct kvm_dirty_ring_indices vcpu_ring_indices;
 };
 
 /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */

@@ -1009,6 +1019,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_GUEST_DEBUG_SSTEP 176
 #define KVM_CAP_ARM_NISV_TO_USER 177
 #define KVM_CAP_ARM_INJECT_EXT_DABT 178
+#define KVM_CAP_DIRTY_LOG_RING 179
 
 #ifdef KVM_CAP_IRQ_ROUTING
 

@@ -1473,6 +1484,9 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+/* Available with KVM_CAP_DIRTY_LOG_RING */
+#define KVM_RESET_DIRTY_RINGS     _IO(KVMIO, 0xc3)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */

@@ -1623,4 +1637,23 @@ struct kvm_hyperv_eventfd {
 #define KVM_HYPERV_CONN_ID_MASK		0x00ffffff
 #define KVM_HYPERV_EVENTFD_DEASSIGN	(1 << 0)
 
+/*
+ * The following are the requirements for supporting dirty log ring
+ * (by enabling KVM_DIRTY_LOG_PAGE_OFFSET).
+ *
+ * 1. Memory accesses by KVM should call kvm_vcpu_write_* instead
+ *    of kvm_write_* so that the global dirty ring is not filled up
+ *    too quickly.
+ * 2. kvm_arch_mmu_enable_log_dirty_pt_masked should be defined for
+ *    enabling dirty logging.
+ * 3. There should not be a separate step to synchronize hardware
+ *    dirty bitmap with KVM's.
+ */
+
+struct kvm_dirty_gfn {
+	__u32 pad;
+	__u32 slot;
+	__u64 offset;
+};
+
 #endif /* __LINUX_KVM_H */

virt/kvm/dirty_ring.c new file +67 -0

new file mode 100644
index 000000000000..67ec5bbc21c0
@@ -0,0 +1,162 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * KVM dirty ring implementation
+ *
+ * Copyright 2019 Red Hat, Inc.
+ */
+#include <linux/kvm_host.h>
+#include <linux/kvm.h>
+#include <linux/vmalloc.h>
+#include <linux/kvm_dirty_ring.h>
+#include <trace/events/kvm.h>
+
+int __weak kvm_cpu_dirty_log_size(void)
+{
+	return 0;
+}
+
+u32 kvm_dirty_ring_get_rsvd_entries(void)
+{
+	return KVM_DIRTY_RING_RSVD_ENTRIES + kvm_cpu_dirty_log_size();
+}
+
+static u32 kvm_dirty_ring_used(struct kvm_dirty_ring *ring)
+{
+	return READ_ONCE(ring->dirty_index) - READ_ONCE(ring->reset_index);
+}
+
+bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring)
+{
+	return kvm_dirty_ring_used(ring) >= ring->soft_limit;
+}
+
+bool kvm_dirty_ring_full(struct kvm_dirty_ring *ring)
+{
+	return kvm_dirty_ring_used(ring) >= ring->size;
+}
+
+struct kvm_dirty_ring *kvm_dirty_ring_get(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
+
+	WARN_ON_ONCE(vcpu->kvm != kvm);
+
+	return &vcpu->dirty_ring;
+}
+
+int kvm_dirty_ring_alloc(struct kvm_dirty_ring *ring,
+			 struct kvm_dirty_ring_indices *indices,
+			 int index, u32 size)
+{
+	ring->dirty_gfns = vmalloc(size);
+	if (!ring->dirty_gfns)
+		return -ENOMEM;
+	memset(ring->dirty_gfns, 0, size);
+
+	ring->size = size / sizeof(struct kvm_dirty_gfn);
+	ring->soft_limit = ring->size - kvm_dirty_ring_get_rsvd_entries();
+	ring->dirty_index = 0;
+	ring->reset_index = 0;
+	ring->index = index;
+	ring->indices = indices;
+
+	return 0;
+}
+
+int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
+{

virt/kvm/dirty_ring.c new file +8 -0

+
+	/*
+	 * Note that fetch_index is written by the userspace, which
+	 * should not be trusted.  If this happens, then it's probably
+	 * that the userspace has written a wrong fetch_index.
+	 */
+	if (fetch - ring->reset_index > ring->size)
+		return -EINVAL;

virt/kvm/dirty_ring.c new file +12 -0

+		ring->reset_index++;
+		count++;
+		/*
+		 * Try to coalesce the reset operations when the guest is
+		 * scanning pages in the same slot.
+		 */
+		if (!first_round && next_slot == cur_slot) {
+			s64 delta = next_offset - cur_offset;
+
+			if (delta >= 0 && delta < BITS_PER_LONG) {
+				mask |= 1ull << delta;
+				continue;

virt/kvm/dirty_ring.c new file +20 -0

+		kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
+		cur_slot = next_slot;
+		cur_offset = next_offset;
+		mask = 1;
+		first_round = false;
+	}
+	kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
+
+	trace_kvm_dirty_ring_reset(ring);
+
+	return count;
+}
+
+void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset)
+{
+	struct kvm_dirty_gfn *entry;
+	struct kvm_dirty_ring_indices *indices = ring->indices;
+
+	/* It should never get full */
+	WARN_ON_ONCE(kvm_dirty_ring_full(ring));

virt/kvm/dirty_ring.c new file +15 -0

+	WRITE_ONCE(indices->avail_index, ring->dirty_index);
+
+	trace_kvm_dirty_ring_push(ring, slot, offset);
+}
+
+struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset)
+{
+	return vmalloc_to_page((void *)ring->dirty_gfns + offset * PAGE_SIZE);
+}
+
+void kvm_dirty_ring_free(struct kvm_dirty_ring *ring)
+{
+	vfree(ring->dirty_gfns);
+	ring->dirty_gfns = NULL;
+}

virt/kvm/kvm_main.c +135 -2

index 5bbd8b8730fa..5e36792e15ae 100644
@@ -64,6 +64,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/kvm.h>
 
+#include <linux/kvm_dirty_ring.h>
+
 /* Worst case buffer size needed for holding an integer. */
 #define ITOA_MAX_LEN 12
 

@@ -357,11 +359,22 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->preempted = false;
 	vcpu->ready = false;
 
+	if (kvm->dirty_ring_size) {
+		r = kvm_dirty_ring_alloc(&vcpu->dirty_ring,
+					 &vcpu->run->vcpu_ring_indices,
+					 id, kvm->dirty_ring_size);
+		if (r)
+			goto fail_free_run;
+	}
+
 	r = kvm_arch_vcpu_init(vcpu);
 	if (r < 0)
-		goto fail_free_run;
+		goto fail_free_ring;
 	return 0;
 
+fail_free_ring:
+	if (kvm->dirty_ring_size)
+		kvm_dirty_ring_free(&vcpu->dirty_ring);
 fail_free_run:
 	free_page((unsigned long)vcpu->run);
 fail:

@@ -379,6 +392,8 @@ void kvm_vcpu_uninit(struct kvm_vcpu *vcpu)
 	put_pid(rcu_dereference_protected(vcpu->pid, 1));
 	kvm_arch_vcpu_uninit(vcpu);
 	free_page((unsigned long)vcpu->run);
+	if (vcpu->kvm->dirty_ring_size)
+		kvm_dirty_ring_free(&vcpu->dirty_ring);
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_uninit);
 

@@ -2284,8 +2299,13 @@ static void mark_page_dirty_in_slot(struct kvm *kvm,
 {
 	if (memslot && memslot->dirty_bitmap) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
+		u32 slot = (memslot->as_id << 16) | memslot->id;
 
-		set_bit_le(rel_gfn, memslot->dirty_bitmap);
+		if (kvm->dirty_ring_size)
+			kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
+					    slot, rel_gfn);
+		else
+			set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }
 

@@ -2632,6 +2652,16 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_on_spin);
 
+static bool kvm_page_in_dirty_ring(struct kvm *kvm, unsigned long pgoff)
+{
+	if (!KVM_DIRTY_LOG_PAGE_OFFSET)
+		return false;
+
+	return (pgoff >= KVM_DIRTY_LOG_PAGE_OFFSET) &&
+	    (pgoff < KVM_DIRTY_LOG_PAGE_OFFSET +
+	     kvm->dirty_ring_size / PAGE_SIZE);
+}
+
 static vm_fault_t kvm_vcpu_fault(struct vm_fault *vmf)
 {
 	struct kvm_vcpu *vcpu = vmf->vma->vm_file->private_data;

@@ -2647,6 +2677,10 @@ static vm_fault_t kvm_vcpu_fault(struct vm_fault *vmf)
 	else if (vmf->pgoff == KVM_COALESCED_MMIO_PAGE_OFFSET)
 		page = virt_to_page(vcpu->kvm->coalesced_mmio_ring);
 #endif
+	else if (kvm_page_in_dirty_ring(vcpu->kvm, vmf->pgoff))
+		page = kvm_dirty_ring_get_page(
+		    &vcpu->dirty_ring,
+		    vmf->pgoff - KVM_DIRTY_LOG_PAGE_OFFSET);
 	else
 		return kvm_arch_vcpu_fault(vcpu, vmf);
 	get_page(page);

@@ -2660,6 +2694,15 @@ static const struct vm_operations_struct kvm_vcpu_vm_ops = {
 
 static int kvm_vcpu_mmap(struct file *file, struct vm_area_struct *vma)
 {
+	struct kvm_vcpu *vcpu = file->private_data;
+	unsigned long pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+
+	/* If to map any writable page within dirty ring, fail it */
+	if ((kvm_page_in_dirty_ring(vcpu->kvm, vma->vm_pgoff) ||
+	     kvm_page_in_dirty_ring(vcpu->kvm, vma->vm_pgoff + pages - 1)) &&
+	    vma->vm_flags & VM_WRITE)
+		return -EINVAL;
+
 	vma->vm_ops = &kvm_vcpu_vm_ops;
 	return 0;
 }

@@ -3242,12 +3285,97 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
 #endif
 	case KVM_CAP_NR_MEMSLOTS:
 		return KVM_USER_MEM_SLOTS;
+	case KVM_CAP_DIRTY_LOG_RING:
+#ifdef CONFIG_X86
+		return KVM_DIRTY_RING_MAX_ENTRIES;
+#else
+		return 0;
+#endif
 	default:
 		break;
 	}
 	return kvm_vm_ioctl_check_extension(kvm, arg);
 }
 
+void kvm_reset_dirty_gfn(struct kvm *kvm, u32 slot, u64 offset, u64 mask)
+{
+	struct kvm_memory_slot *memslot;
+	int as_id, id;
+
+	as_id = slot >> 16;
+	id = (u16)slot;
+	if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_USER_MEM_SLOTS)
+		return;
+
+	memslot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
+	if (offset >= memslot->npages)
+		return;
+
+	spin_lock(&kvm->mmu_lock);
+	kvm_arch_mmu_enable_log_dirty_pt_masked(kvm, memslot, offset, mask);
+	spin_unlock(&kvm->mmu_lock);
+}
+
+static int kvm_vm_ioctl_enable_dirty_log_ring(struct kvm *kvm, u32 size)
+{
+	int r;
+
+	if (!KVM_DIRTY_LOG_PAGE_OFFSET)
+		return -EINVAL;
+
+	/* the size should be power of 2 */
+	if (!size || (size & (size - 1)))
+		return -EINVAL;
+
+	/* Should be bigger to keep the reserved entries, or a page */
+	if (size < kvm_dirty_ring_get_rsvd_entries() *
+	    sizeof(struct kvm_dirty_gfn) || size < PAGE_SIZE)
+		return -EINVAL;
+
+	if (size > KVM_DIRTY_RING_MAX_ENTRIES *
+	    sizeof(struct kvm_dirty_gfn))
+		return -E2BIG;
+
+	/* We only allow it to set once */
+	if (kvm->dirty_ring_size)
+		return -EINVAL;
+
+	mutex_lock(&kvm->lock);
+
+	if (kvm->created_vcpus) {
+		/* We don't allow to change this value after vcpu created */
+		r = -EINVAL;
+	} else {
+		kvm->dirty_ring_size = size;
+		r = 0;
+	}
+
+	mutex_unlock(&kvm->lock);
+	return r;
+}
+
+static int kvm_vm_ioctl_reset_dirty_pages(struct kvm *kvm)
+{
+	int i;
+	struct kvm_vcpu *vcpu;
+	int cleared = 0;
+
+	if (!kvm->dirty_ring_size)
+		return -EINVAL;
+
+	mutex_lock(&kvm->slots_lock);
+
+	kvm_for_each_vcpu(i, vcpu, kvm)
+		cleared += kvm_dirty_ring_reset(vcpu->kvm, &vcpu->dirty_ring);
+
+	mutex_unlock(&kvm->slots_lock);
+
+	if (cleared)
+		kvm_flush_remote_tlbs(kvm);
+
+	return cleared;
+}
+
 int __attribute__((weak)) kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 						  struct kvm_enable_cap *cap)
 {

@@ -3265,6 +3393,8 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
 		kvm->manual_dirty_log_protect = cap->args[0];
 		return 0;
 #endif
+	case KVM_CAP_DIRTY_LOG_RING:
+		return kvm_vm_ioctl_enable_dirty_log_ring(kvm, cap->args[0]);
 	default:
 		return kvm_vm_ioctl_enable_cap(kvm, cap);
 	}

@@ -3452,6 +3582,9 @@ static long kvm_vm_ioctl(struct file *filp,
 	case KVM_CHECK_EXTENSION:
 		r = kvm_vm_ioctl_check_extension_generic(kvm, arg);
 		break;
+	case KVM_RESET_DIRTY_RINGS:
+		r = kvm_vm_ioctl_reset_dirty_pages(kvm);
+		break;
 	default:
 		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
 	}

Signature · 1 line

2.24.1

Quoted 13 lines · Peter Xu

This patch is heavily based on previous work from Lei Cao <[email protected]> and Paolo Bonzini <[email protected]>. [1]

KVM currently uses large bitmaps to track dirty memory. These bitmaps are copied to userspace when userspace queries KVM for its dirty page information. The use of bitmaps is mostly sufficient for live migration, as large parts of memory are be dirtied from one log-dirty pass to another. However, in a checkpointing system, the number of dirty pages is small and in fact it is often bounded---the VM is paused when it has dirtied a pre-defined number of pages. Traversing a large, sparsely populated bitmap to find set bits is time-consuming, as is copying the bitmap to user-space.

Quoted 7 lines

A similar issue will be there for live migration when the guest memory is huge while the page dirty procedure is trivial. In that case for each dirty sync we need to pull the whole dirty bitmap to userspace and analyse every bit even if it's mostly zeros.

The preferred data structure for above scenarios is a dense list of guest frame numbers (GFN).

No longer, this uses an array of structs.

Quoted 102 lines

This patch series stores the dirty list in
kernel memory that can be memory mapped into userspace to allow speedy harvesting.

This patch enables dirty ring for X86 only. However it should be easily extended to other archs as well.

[1] https://patchwork.kernel.org/patch/10471409/

Signed-off-by Lei Cao <[email protected]>

Signed-off-by Paolo Bonzini <[email protected]>

Signed-off-by Peter Xu <[email protected]>

---
 Documentation/virt/kvm/api.txt  |  89 ++++++++++++++++++
 arch/x86/include/asm/kvm_host.h |   3 +
 arch/x86/include/uapi/asm/kvm.h |   1 +
 arch/x86/kvm/Makefile           |   3 +-
 arch/x86/kvm/mmu/mmu.c          |   6 ++
 arch/x86/kvm/vmx/vmx.c          |   7 ++
 arch/x86/kvm/x86.c              |   9 ++
 include/linux/kvm_dirty_ring.h  |  55 +++++++++++
 include/linux/kvm_host.h        |  26 +++++
 include/trace/events/kvm.h      |  78 +++++++++++++++
 include/uapi/linux/kvm.h        |  33 +++++++
 virt/kvm/dirty_ring.c           | 162 ++++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c             | 137 ++++++++++++++++++++++++++-
 13 files changed, 606 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/kvm_dirty_ring.h
 create mode 100644 virt/kvm/dirty_ring.c

Documentation/virt/kvm/api.txt +36 -0

index ebb37b34dcfc..708c3e0f7eae 100644
@@ -231,6 +231,7 @@ Based on their initialization different VMs may have different capabilities.
 It is thus encouraged to use the vm ioctl to query for capabilities (available
 with KVM_CAP_CHECK_EXTENSION_VM on the vm fd)
 
+
 4.5 KVM_GET_VCPU_MMAP_SIZE
 
 Capability: basic

@@ -243,6 +244,18 @@ The KVM_RUN ioctl (cf.) communicates with userspace via a shared
 memory region.  This ioctl returns the size of that region.  See the
 KVM_RUN documentation for details.
 
+Besides the size of the KVM_RUN communication region, other areas of
+the VCPU file descriptor can be mmap-ed, including:
+
+- if KVM_CAP_COALESCED_MMIO is available, a page at
+  KVM_COALESCED_MMIO_PAGE_OFFSET * PAGE_SIZE; for historical reasons,
+  this page is included in the result of KVM_GET_VCPU_MMAP_SIZE.
+  KVM_CAP_COALESCED_MMIO is not documented yet.
+
+- if KVM_CAP_DIRTY_LOG_RING is available, a number of pages at
+  KVM_DIRTY_LOG_PAGE_OFFSET * PAGE_SIZE.  For more information on
+  KVM_CAP_DIRTY_LOG_RING, see section 8.3.
+
 
 4.6 KVM_SET_MEMORY_REGION
 

@@ -5376,6 +5389,7 @@ CPU when the exception is taken. If this virtual SError is taken to EL1 using
 AArch64, this value will be reported in the ISS field of ESR_ELx.
 
 See KVM_CAP_VCPU_EVENTS for more details.
+
 8.20 KVM_CAP_HYPERV_SEND_IPI
 
 Architectures: x86

@@ -5383,6 +5397,7 @@ Architectures: x86
 This capability indicates that KVM supports paravirtualized Hyper-V IPI send
 hypercalls:
 HvCallSendSyntheticClusterIpi, HvCallSendSyntheticClusterIpiEx.
+
 8.21 KVM_CAP_HYPERV_DIRECT_TLBFLUSH
 
 Architecture: x86

@@ -5396,3 +5411,77 @@ handling by KVM (as some KVM hypercall may be mistakenly treated as TLB
 flush hypercalls by Hyper-V) so userspace should disable KVM identification
 in CPUID and only exposes Hyper-V identification. In this case, guest
 thinks it's running on Hyper-V and only use Hyper-V hypercalls.
+
+8.22 KVM_CAP_DIRTY_LOG_RING
+
+Architectures: x86
+Parameters: args[0] - size of the dirty log ring
+
+KVM is capable of tracking dirty memory using ring buffers that are
+mmaped into userspace; there is one dirty ring per vcpu.
+
+One dirty ring is defined as below internally:
+
+struct kvm_dirty_ring {
+	u32 dirty_index;
+	u32 reset_index;
+	u32 size;
+	u32 soft_limit;
+	struct kvm_dirty_gfn *dirty_gfns;
+	struct kvm_dirty_ring_indices *indices;
+	int index;
+};
+

Documentation/virt/kvm/api.txt handling by KVM (as some KVM hypercall may be mistakenly treated as TLB · line 5439

Quoted 5 lines

Documentation/virt/kvm/api.txt +5 -0

+Dirty GFNs (Guest Frame Numbers) are stored in the dirty_gfns array.
+For each of the dirty entry it's defined as:
+
+struct kvm_dirty_gfn {
+        __u32 pad;  

How about sticking a length here?
This way huge pages can be dirtied in one go.

Quoted 8 lines

Documentation/virt/kvm/api.txt handling by KVM (as some KVM hypercall may be mistakenly treated as TLB · line 5439

Quoted 5 lines

Documentation/virt/kvm/api.txt +5 -0

+Dirty GFNs (Guest Frame Numbers) are stored in the dirty_gfns array.
+For each of the dirty entry it's defined as:
+
+struct kvm_dirty_gfn {
+        __u32 pad;  

How about sticking a length here?
This way huge pages can be dirtied in one go.

Not just huge pages, but any contiguous range of dirty pages could be reported far more concisely. Thanks,