KVM: Dirty ring interface

From: Paolo Bonzini <[email protected]>

For ring-based dirty log tracking, it will be more efficient to account writes during schedule-out or schedule-in to the currently running VCPU. We would like to do it even if the write doesn't use the current VCPU's address space, as is the case for cached writes (see commit 4e335d9e7ddb, "Revert "KVM: Support vCPU-based gfn->hva cache"", 2017-05-02).

Therefore, add a mechanism to track the currently-loaded kvm_vcpu struct. There is already something similar in KVM/ARM; one important difference is that kvm_arch_vcpu_{load,put} have two callers in virt/kvm/kvm_main.c:
we have to update both the architecture-independent vcpu_{load,put} and the preempt notifiers.

Another change made in the process is to allow using kvm_get_running_vcpu() in preemptible code. This is allowed because preempt notifiers ensure that the value does not change even after the VCPU thread is migrated.

In case it was clear, I strongly dislike adding kvm_get_running_vcpu(). IMO, it's a unnecessary hack. The proper change to ensure a valid vCPU is seen by mark_page_dirty_in_ring() when there is a current vCPU is to plumb the vCPU down through the various call stacks. Looking up the call stacks for mark_page_dirty() and mark_page_dirty_in_slot(), they all originate with a vcpu->kvm within a few functions, except for the rare case where the write is coming from a non-vcpu ioctl(), in which case there is no current vCPU.

The proper change is obviously much bigger in scope and would require touching gobs of arch specific code, but IMO the end result would be worth the effort. E.g. there's a decent chance it would reduce the API between common KVM and arch specific code by eliminating the exports of variants that take "struct kvm *" instead of "struct kvm_vcpu *".

Quoted 9 lines · Sean Christopherson

In case it was clear, I strongly dislike adding kvm_get_running_vcpu(). IMO, it's a unnecessary hack. The proper change to ensure a valid vCPU is seen by mark_page_dirty_in_ring() when there is a current vCPU is to plumb the vCPU down through the various call stacks. Looking up the call stacks for mark_page_dirty() and mark_page_dirty_in_slot(), they all originate with a vcpu->kvm within a few functions, except for the rare case where the write is coming from a non-vcpu ioctl(), in which case there is no current vCPU.

Quoted 5 lines

The proper change is obviously much bigger in scope and would require touching gobs of arch specific code, but IMO the end result would be worth the effort. E.g. there's a decent chance it would reduce the API between common KVM and arch specific code by eliminating the exports of variants that take "struct kvm *" instead of "struct kvm_vcpu *".

It's not that simple. In some cases, the "struct kvm *" cannot be easily replaced with a "struct kvm_vcpu *" without making the API less intuitive; for example think of a function that takes a kvm_vcpu pointer but then calls gfn_to_hva(vcpu->kvm) instead of the expected kvm_vcpu_gfn_to_hva(vcpu).

That said, looking at the code again after a couple years I agree that the usage of kvm_get_running_vcpu() is ugly. But I don't think it's kvm_get_running_vcpu()'s fault, rather it's the vCPU argument in mark_page_dirty_in_slot and mark_page_dirty_in_ring that is confusing and we should not be adding.

kvm_get_running_vcpu() basically means "you can use the per-vCPU ring and avoid locking", nothing more. Right now we need the vCPU argument in mark_page_dirty_in_ring for kvm_arch_vcpu_memslots_id(vcpu), but that is unnecessary and is the real source of confusion (possibly bugs too) if it gets out of sync.

Instead, let's add an as_id field to struct kvm_memory_slot (which is trivial to initialize in __kvm_set_memory_region), and just do

	as_id = slot->as_id;
	vcpu = kvm_get_running_vcpu();

in mark_page_dirty_in_ring.

From: "Cao, Lei" <[email protected]>

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

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

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

---
 virt/kvm/kvm_main.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

virt/kvm/kvm_main.c +0 -0

index fac0760c870e..8f8940cc4b84 100644

On Fri, Nov 29, 2019 at 04:34:52PM -0500, Peter Xu wrote:

Why?

Quoted 18 lines

From: "Cao, Lei" <[email protected]>

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

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

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

---
 virt/kvm/kvm_main.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

virt/kvm/kvm_main.c +0 -0

index fac0760c870e..8f8940cc4b84 100644
@@ -145,7 +145,10 @@ static void hardware_disable_all(void);
 
 static void kvm_io_bus_destroy(struct kvm_io_bus *bus);
 

virt/kvm/kvm_main.c static void hardware_disable_all(void); · line 151

Quoted 5 lines

virt/kvm/kvm_main.c +4 -1

-static void mark_page_dirty_in_slot(struct kvm_memory_slot *memslot, gfn_t gfn);
+static void mark_page_dirty_in_slot(struct kvm *kvm,
+				    struct kvm_vcpu *vcpu,
+				    struct kvm_memory_slot *memslot,
+				    gfn_t gfn);

Why both? Passing @vcpu gets you @kvm.

It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields.

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

---
 virt/kvm/kvm_main.c | 2 ++
 1 file changed, 2 insertions(+)

virt/kvm/kvm_main.c +2 -0

index 8f8940cc4b84..681452d288cd 100644
@@ -352,6 +352,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	}
 	vcpu->run = page_address(page);
 
+	BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE);
+
 	kvm_vcpu_set_in_spin_loop(vcpu, false);
 	kvm_vcpu_set_dy_eligible(vcpu, false);
 	vcpu->preempted = false;

Signature · 1 line

2.21.0

Quoted 3 lines · Peter Xu

It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields.

Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's almost absurdly obvious when looking at the code, but a) the patch itself does not provide that context and b) the changelog should hold up on its own,

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

Quoted 22 lines

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

---
 virt/kvm/kvm_main.c | 2 ++
 1 file changed, 2 insertions(+)

virt/kvm/kvm_main.c +2 -0

index 8f8940cc4b84..681452d288cd 100644
@@ -352,6 +352,8 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	}
 	vcpu->run = page_address(page);
 
+	BUILD_BUG_ON(sizeof(struct kvm_run) > PAGE_SIZE);
+
 	kvm_vcpu_set_in_spin_loop(vcpu, false);
 	kvm_vcpu_set_dy_eligible(vcpu, false);
 	vcpu->preempted = false;

Signature · 1 line

2.21.0

Quoted 9 lines · Sean Christopherson

Quoted 3 lines · Peter Xu

It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields.

Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's almost absurdly obvious when looking at the code, but a) the patch itself does not provide that context and b) the changelog should hold up on its own,

Right, I'll enhance the commit message.

Quoted 2 lines

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

And that's why I added BUILD_BUG_ON right beneath that allocation. :)

It's just a helper for developers when adding new kvm_run fields, not a risk for anyone who wants to start allocating more pages for it.

Quoted 11 lines · Peter Xu

Quoted 9 lines · Sean Christopherson

Quoted 3 lines · Peter Xu

It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields.

Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's almost absurdly obvious when looking at the code, but a) the patch itself does not provide that context and b) the changelog should hold up on its own,

Quoted 6 lines

Right, I'll enhance the commit message.

Quoted 2 lines

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

And that's why I added BUILD_BUG_ON right beneath that allocation. :)

My point is that if the allocation were changed to no longer be a straightforward alloc_page() then someone reading the combined code would have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for this case because the specific constraints of vcpu->run make it highly unlikely to use anything else, but that's beside the point.

Quoted 2 lines

It's just a helper for developers when adding new kvm_run fields, not a risk for anyone who wants to start allocating more pages for it.

But by adding a BUILD_BUG_ON without explaining *why*, you're placing an extra burden on someone that wants to increase the size of kvm->run, e.g. it's not at all obvious from the changelog whether this patch is adding the BUILD_BUG_ON purely because the code allocates memory for vcpu->run via alloc_page(), or if there is some fundamental aspect of vcpu->run that requires it to never span multiple pages.

Quoted 28 lines · Sean Christopherson

Quoted 11 lines · Peter Xu

Quoted 9 lines · Sean Christopherson

Quoted 3 lines · Peter Xu

It's already going to reach 2400 Bytes (which is over half of page size on 4K page archs), so maybe it's good to have this build-time check in case it overflows when adding new fields.

Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's almost absurdly obvious when looking at the code, but a) the patch itself does not provide that context and b) the changelog should hold up on its own,

Quoted 6 lines

Right, I'll enhance the commit message.

Quoted 2 lines

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

And that's why I added BUILD_BUG_ON right beneath that allocation. :)

My point is that if the allocation were changed to no longer be a straightforward alloc_page() then someone reading the combined code would have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for this case because the specific constraints of vcpu->run make it highly unlikely to use anything else, but that's beside the point.

Quoted 2 lines

It's just a helper for developers when adding new kvm_run fields, not a risk for anyone who wants to start allocating more pages for it.

Quoted 6 lines

But by adding a BUILD_BUG_ON without explaining *why*, you're placing an extra burden on someone that wants to increase the size of kvm->run, e.g. it's not at all obvious from the changelog whether this patch is adding the BUILD_BUG_ON purely because the code allocates memory for vcpu->run via alloc_page(), or if there is some fundamental aspect of vcpu->run that requires it to never span multiple pages.

How about I add a comment above it?

/*
* Currently kvm_run only uses one physical page. Warn the develper
* if kvm_run accidentaly grows more than that.
*/
BUILD_BUG_ON(...);

Quoted 3 lines

Quoted 2 lines

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

And that's why I added BUILD_BUG_ON right beneath that allocation. :)

Quoted 3 lines · Sean Christopherson

Quoted 3 lines

Quoted 2 lines

e.g. in a mostly hypothetical case where the allocation of vcpu->run were changed to something else.

And that's why I added BUILD_BUG_ON right beneath that allocation. :)

It's not exactly beneath it (it's out of the patch context at least). I think a comment is not strictly necessary, but a better commit message is and, since you are at it, I would put the BUILD_BUG_ON *before* the allocation. That makes it more obvious that you are checking the invariant before allocating.

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.

We defined two new data structures:

struct kvm_dirty_ring;
struct kvm_dirty_ring_indexes;

Firstly, kvm_dirty_ring is defined to represent a ring of dirty pages. When dirty tracking is enabled, we can push dirty gfn onto the ring.

Secondly, kvm_dirty_ring_indexes is defined to represent the user/kernel interface of each ring. Currently it contains two
indexes: (1) avail_index represents where we should push our next PFN (written by kernel), while (2) fetch_index represents where the userspace should fetch the next dirty PFN (written by userspace).

One complete ring is composed by one kvm_dirty_ring plus its corresponding kvm_dirty_ring_indexes.

Currently, we have N+1 rings for each VM of N vcpus:

- for each vcpu, we have 1 per-vcpu dirty ring,
- for each vm, we have 1 per-vm dirty ring

Please refer to the documentation update in this patch for more details.

Note that this patch implements the core logic of dirty ring buffer. It's still disabled for all archs for now. Also, we'll address some of the other issues in follow up patches before it's firstly enabled on x86.

[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]>

---

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

new file mode 100644
index 000000000000..9264891f3c32
@@ -0,0 +1,156 @@
+#include <linux/kvm_host.h>
+#include <linux/kvm.h>
+#include <linux/vmalloc.h>

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

+	    kvm_dirty_ring_get_rsvd_entries();
+	ring->dirty_index = 0;
+	ring->reset_index = 0;
+	spin_lock_init(&ring->lock);
+
+	return 0;
+}
+

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

+		vfree(ring->dirty_gfns);
+		ring->dirty_gfns = NULL;
+	}
+}

virt/kvm/kvm_main.c +6 -0

index 681452d288cd..8642c977629b 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
 

@@ -149,6 +151,10 @@ static void mark_page_dirty_in_slot(struct kvm *kvm,
 				    struct kvm_vcpu *vcpu,
 				    struct kvm_memory_slot *memslot,
 				    gfn_t gfn);
+static void mark_page_dirty_in_ring(struct kvm *kvm,
+				    struct kvm_vcpu *vcpu,
+				    struct kvm_memory_slot *slot,
+				    gfn_t gfn);
 
 __visible bool kvm_rebooting;
 EXPORT_SYMBOL_GPL(kvm_rebooting);

virt/kvm/kvm_main.c +18 -1

+		}
+	}
+
 	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:

@@ -381,6 +398,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);
 

@@ -690,6 +709,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	struct kvm *kvm = kvm_arch_alloc_vm();
 	int r = -ENOMEM;
 	int i;
+	struct page *page;
 
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);

@@ -705,6 +725,14 @@ static struct kvm *kvm_create_vm(unsigned long type)
 
 	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
 
+	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+	if (!page) {
+		r = -ENOMEM;
+		goto out_err_alloc_page;
+	}
+	kvm->vm_run = page_address(page);
+	BUILD_BUG_ON(sizeof(struct kvm_vm_run) > PAGE_SIZE);
+
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))

virt/kvm/kvm_main.c +7 -1

+
+	if (kvm->vm_run) {
+		free_page((unsigned long)kvm->vm_run);
+		kvm->vm_run = NULL;
+	}
+
 	kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
 	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);

@@ -2301,7 +2341,7 @@ static void mark_page_dirty_in_slot(struct kvm *kvm,
 {
 	if (memslot && memslot->dirty_bitmap) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
-
+		mark_page_dirty_in_ring(kvm, vcpu, memslot, gfn);
 		set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }

@@ -2649,6 +2689,13 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_on_spin);

Quoted 41 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.

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.

We defined two new data structures:

struct kvm_dirty_ring;
struct kvm_dirty_ring_indexes;

Firstly, kvm_dirty_ring is defined to represent a ring of dirty pages. When dirty tracking is enabled, we can push dirty gfn onto the ring.

Secondly, kvm_dirty_ring_indexes is defined to represent the user/kernel interface of each ring. Currently it contains two
indexes: (1) avail_index represents where we should push our next PFN (written by kernel), while (2) fetch_index represents where the userspace should fetch the next dirty PFN (written by userspace).

One complete ring is composed by one kvm_dirty_ring plus its corresponding kvm_dirty_ring_indexes.

Quoted 4 lines

Currently, we have N+1 rings for each VM of N vcpus:

- for each vcpu, we have 1 per-vcpu dirty ring,
- for each vm, we have 1 per-vm dirty ring

Why? I assume the purpose of per-vcpu rings is to avoid contention between threads, but the motiviation needs to be explicitly stated. And why is a per-vm fallback ring needed?

If my assumption is correct, have other approaches been tried/profiled? E.g. using cmpxchg to reserve N number of entries in a shared ring.

IMO,
adding kvm_get_running_vcpu() is a hack that is just asking for future abuse and the vcpu/vm/as_id interactions in mark_page_dirty_in_ring() look extremely fragile.

I also dislike having two different mechanisms
for accessing the ring (lock for per-vm, something else for per-vcpu).

Quoted 15 lines

Please refer to the documentation update in this patch for more details.

Note that this patch implements the core logic of dirty ring buffer. It's still disabled for all archs for now. Also, we'll address some of the other issues in follow up patches before it's firstly enabled on x86.

[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]>

---

...

Quoted 15 lines

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

new file mode 100644
index 000000000000..9264891f3c32
@@ -0,0 +1,156 @@
+#include <linux/kvm_host.h>
+#include <linux/kvm.h>
+#include <linux/vmalloc.h>
+#include <linux/kvm_dirty_ring.h>
+
+u32 kvm_dirty_ring_get_rsvd_entries(void)
+{
+	return KVM_DIRTY_RING_RSVD_ENTRIES + kvm_cpu_dirty_log_size();
+}

virt/kvm/dirty_ring.c line 13

Quoted 4 lines

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

+
+int kvm_dirty_ring_alloc(struct kvm *kvm, struct kvm_dirty_ring *ring)
+{
+	u32 size = kvm->dirty_ring_size;

Just pass in @size, that way you don't need @kvm. And the callers will be less ugly, e.g. the initial allocation won't need to speculatively set kvm->dirty_ring_size.

virt/kvm/dirty_ring.c line 22

Quoted 10 lines

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

+
+	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 =
+	    (kvm->dirty_ring_size / sizeof(struct kvm_dirty_gfn)) -

And passing @size avoids issues like this where a local var is ignored.

Quoted 8 lines

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

+	    kvm_dirty_ring_get_rsvd_entries();
+	ring->dirty_index = 0;
+	ring->reset_index = 0;
+	spin_lock_init(&ring->lock);
+
+	return 0;
+}
+

...

virt/kvm/dirty_ring.c line 152

Quoted 3 lines

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

+void kvm_dirty_ring_free(struct kvm_dirty_ring *ring)
+{
+	if (ring->dirty_gfns) {

Why condition freeing the dirty ring on kvm->dirty_ring_size, this obviously protects itself. Not to mention vfree() also plays nice with a NULL input.

Quoted 33 lines

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

+		vfree(ring->dirty_gfns);
+		ring->dirty_gfns = NULL;
+	}
+}

virt/kvm/kvm_main.c +6 -0

index 681452d288cd..8642c977629b 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
 

@@ -149,6 +151,10 @@ static void mark_page_dirty_in_slot(struct kvm *kvm,
 				    struct kvm_vcpu *vcpu,
 				    struct kvm_memory_slot *memslot,
 				    gfn_t gfn);
+static void mark_page_dirty_in_ring(struct kvm *kvm,
+				    struct kvm_vcpu *vcpu,
+				    struct kvm_memory_slot *slot,
+				    gfn_t gfn);
 
 __visible bool kvm_rebooting;
 EXPORT_SYMBOL_GPL(kvm_rebooting);

@@ -359,11 +365,22 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->preempted = false;
 	vcpu->ready = false;
 

virt/kvm/kvm_main.c int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id) · line 372

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

+	if (kvm->dirty_ring_size) {
+		r = kvm_dirty_ring_alloc(vcpu->kvm, &vcpu->dirty_ring);
+		if (r) {
+			kvm->dirty_ring_size = 0;
+			goto fail_free_run;

This looks wrong, kvm->dirty_ring_size is used to free allocations, i.e. previous allocations will leak if a vcpu allocation fails.

Quoted 48 lines

virt/kvm/kvm_main.c +18 -1

+		}
+	}
+
 	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:

@@ -381,6 +398,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);
 

@@ -690,6 +709,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	struct kvm *kvm = kvm_arch_alloc_vm();
 	int r = -ENOMEM;
 	int i;
+	struct page *page;
 
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);

@@ -705,6 +725,14 @@ static struct kvm *kvm_create_vm(unsigned long type)
 
 	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
 
+	page = alloc_page(GFP_KERNEL | __GFP_ZERO);
+	if (!page) {
+		r = -ENOMEM;
+		goto out_err_alloc_page;
+	}
+	kvm->vm_run = page_address(page);
+	BUILD_BUG_ON(sizeof(struct kvm_vm_run) > PAGE_SIZE);
+
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))

virt/kvm/kvm_main.c static struct kvm *kvm_create_vm(unsigned long type) · line 807

Quoted 6 lines

virt/kvm/kvm_main.c +2 -0

@@ -775,6 +803,9 @@ static struct kvm *kvm_create_vm(unsigned long type)
 out_err_no_irq_srcu:
 	cleanup_srcu_struct(&kvm->srcu);
 out_err_no_srcu:
+	free_page((unsigned long)page);
+	kvm->vm_run = NULL;

No need to nullify vm_run.

Quoted 9 lines

virt/kvm/kvm_main.c +1 -0

+out_err_alloc_page:
 	kvm_arch_free_vm(kvm);
 	mmdrop(current->mm);
 	return ERR_PTR(r);

@@ -800,6 +831,15 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	int i;
 	struct mm_struct *mm = kvm->mm;
 

virt/kvm/kvm_main.c static void kvm_destroy_vm(struct kvm *kvm) · line 836

Quoted 3 lines

virt/kvm/kvm_main.c +3 -0

+	if (kvm->dirty_ring_size) {
+		kvm_dirty_ring_free(&kvm->vm_dirty_ring);
+	}

Unnecessary parantheses.

Quoted 22 lines

virt/kvm/kvm_main.c +7 -1

+
+	if (kvm->vm_run) {
+		free_page((unsigned long)kvm->vm_run);
+		kvm->vm_run = NULL;
+	}
+
 	kvm_uevent_notify_change(KVM_EVENT_DESTROY_VM, kvm);
 	kvm_destroy_vm_debugfs(kvm);
 	kvm_arch_sync_events(kvm);

@@ -2301,7 +2341,7 @@ static void mark_page_dirty_in_slot(struct kvm *kvm,
 {
 	if (memslot && memslot->dirty_bitmap) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
-
+		mark_page_dirty_in_ring(kvm, vcpu, memslot, gfn);
 		set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }

@@ -2649,6 +2689,13 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 }
 EXPORT_SYMBOL_GPL(kvm_vcpu_on_spin);

Quoted 9 lines · Sean Christopherson

Quoted 4 lines · Peter Xu

Currently, we have N+1 rings for each VM of N vcpus:

- for each vcpu, we have 1 per-vcpu dirty ring,
- for each vm, we have 1 per-vm dirty ring

Why? I assume the purpose of per-vcpu rings is to avoid contention between threads, but the motiviation needs to be explicitly stated. And why is a per-vm fallback ring needed?

Yes, as explained in previous reply, the problem is there could have guest memory writes without vcpu contexts.

Quoted 1 line

Quoted 2 lines

If my assumption is correct, have other approaches been tried/profiled? E.g. using cmpxchg to reserve N number of entries in a shared ring.

Not yet, but I'd be fine to try anything if there's better alternatives. Besides, could you help explain why sharing one ring and let each vcpu to reserve a region in the ring could be helpful in the pov of performance?

Quoted 4 lines

IMO,
adding kvm_get_running_vcpu() is a hack that is just asking for future abuse and the vcpu/vm/as_id interactions in mark_page_dirty_in_ring() look extremely fragile.

I agree. Another way is to put heavier traffic to the per-vm ring, but the downside could be that the per-vm ring could get full easier (but I haven't tested).

Quoted 2 lines

I also dislike having two different mechanisms
for accessing the ring (lock for per-vm, something else for per-vcpu).

Actually I proposed to drop the per-vm ring (actually I had a version that implemented this.. and I just changed it back to the per-vm ring later on, see below) and when there's no vcpu context I thought about:

(1) use vcpu0 ring

(2) or a better algo to pick up a per-vcpu ring (like, the less full ring, we can do many things here, e.g., we can easily maintain a structure track this so we can get O(1) search, I think)

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Quoted 15 lines · Peter Xu

Quoted 9 lines · Sean Christopherson

Quoted 4 lines · Peter Xu

Currently, we have N+1 rings for each VM of N vcpus:

- for each vcpu, we have 1 per-vcpu dirty ring,
- for each vm, we have 1 per-vm dirty ring

Why? I assume the purpose of per-vcpu rings is to avoid contention between threads, but the motiviation needs to be explicitly stated. And why is a per-vm fallback ring needed?

Yes, as explained in previous reply, the problem is there could have guest memory writes without vcpu contexts.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

If my assumption is correct, have other approaches been tried/profiled? E.g. using cmpxchg to reserve N number of entries in a shared ring.

Not yet, but I'd be fine to try anything if there's better alternatives. Besides, could you help explain why sharing one ring and let each vcpu to reserve a region in the ring could be helpful in the pov of performance?

The goal would be to avoid taking a lock, or at least to avoid holding a lock for an extended duration, e.g. some sort of multi-step process where entries in the ring are first reserved, then filled, and finally marked valid. That'd allow the "fill" action to be done in parallel.

In case it isn't clear, I haven't thought through an actual solution :-).

My point is that I think it's worth exploring and profiling other implementations because the dual per-vm and per-vcpu rings has a few warts that we'd be stuck with forever.

Quoted 6 lines

Quoted 5 lines

IMO,
adding kvm_get_running_vcpu() is a hack that is just asking for future abuse and the vcpu/vm/as_id interactions in mark_page_dirty_in_ring() look extremely fragile.

Quoted 3 lines

I agree. Another way is to put heavier traffic to the per-vm ring, but the downside could be that the per-vm ring could get full easier (but I haven't tested).

There's nothing that prevents increasing the size of the common ring each time a new vCPU is added. Alternatively, userspace could explicitly request or hint the desired ring size.

Quoted 14 lines

Quoted 3 lines

I also dislike having two different mechanisms
for accessing the ring (lock for per-vm, something else for per-vcpu).

Actually I proposed to drop the per-vm ring (actually I had a version that implemented this.. and I just changed it back to the per-vm ring later on, see below) and when there's no vcpu context I thought about:

(1) use vcpu0 ring

(2) or a better algo to pick up a per-vcpu ring (like, the less full ring, we can do many things here, e.g., we can easily maintain a structure track this so we can get O(1) search, I think)

Quoted 4 lines

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Ya, per-vm is definitely better than dumping on vcpu0. I'm hoping we can find a third option that provides comparable performance without using any per-vcpu rings.

Quoted 5 lines

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Ya, per-vm is definitely better than dumping on vcpu0. I'm hoping we can find a third option that provides comparable performance without using any per-vcpu rings.

Quoted 9 lines · Sean Christopherson

Quoted 5 lines

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Ya, per-vm is definitely better than dumping on vcpu0. I'm hoping we can find a third option that provides comparable performance without using any per-vcpu rings.

The advantage of per-vCPU rings is that it naturally: 1) parallelizes the processing of dirty pages; 2) makes userspace vCPU thread do more work on vCPUs that dirty more pages.

I agree that on the producer side we could reserve multiple entries in the case of PML (and without PML only one entry should be added at a time). But I'm afraid that things get ugly when the ring is full, because you'd have to wait for all vCPUs to finish publishing the entries they have reserved.

It's ugly that we _also_ need a per-VM ring, but unfortunately some operations do not really have a vCPU that they can refer to.

Quoted 15 lines · Paolo Bonzini

Quoted 9 lines · Sean Christopherson

Quoted 5 lines

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Ya, per-vm is definitely better than dumping on vcpu0. I'm hoping we can find a third option that provides comparable performance without using any per-vcpu rings.

The advantage of per-vCPU rings is that it naturally: 1) parallelizes the processing of dirty pages; 2) makes userspace vCPU thread do more work on vCPUs that dirty more pages.

Quoted 5 lines

I agree that on the producer side we could reserve multiple entries in the case of PML (and without PML only one entry should be added at a time). But I'm afraid that things get ugly when the ring is full, because you'd have to wait for all vCPUs to finish publishing the entries they have reserved.

Ah, I take it the intended model is that userspace will only start pulling entries off the ring when KVM explicitly signals that the ring is "full"?

Rather than reserve entries, what if vCPUs reserved an entire ring? Create a pool of N=nr_vcpus rings that are shared by all vCPUs. To mark pages dirty, a vCPU claims a ring, pushes the pages into the ring, and then returns the ring to the pool. If pushing pages hits the soft limit, a request is made to drain the ring and the ring is not returned to the pool until it is drained.

Except for acquiring a ring, which likely can be heavily optimized, that'd allow parallel processing (#1), and would provide a facsimile of #2 as pushing more pages onto a ring would naturally increase the likelihood of triggering a drain. And it might be interesting to see the effect of using different methods of ring selection, e.g. pure round robin, LRU, last used on the current vCPU, etc...

Quoted 2 lines

It's ugly that we _also_ need a per-VM ring, but unfortunately some operations do not really have a vCPU that they can refer to.

Quoted 7 lines · Sean Christopherson

Rather than reserve entries, what if vCPUs reserved an entire ring? Create a pool of N=nr_vcpus rings that are shared by all vCPUs. To mark pages dirty, a vCPU claims a ring, pushes the pages into the ring, and then returns the ring to the pool. If pushing pages hits the soft limit, a request is made to drain the ring and the ring is not returned to the pool until it is drained.

Quoted 6 lines

Except for acquiring a ring, which likely can be heavily optimized, that'd allow parallel processing (#1), and would provide a facsimile of #2 as pushing more pages onto a ring would naturally increase the likelihood of triggering a drain. And it might be interesting to see the effect of using different methods of ring selection, e.g. pure round robin, LRU, last used on the current vCPU, etc...

If you are creating nr_vcpus rings, and draining is done on the vCPU thread that has filled the ring, why not create nr_vcpus+1? The current code then is exactly the same as pre-claiming a ring per vCPU and never releasing it, and using a spinlock to claim the per-VM ring.

Quoted 15 lines · Paolo Bonzini

Quoted 13 lines · Sean Christopherson

Rather than reserve entries, what if vCPUs reserved an entire ring? Create a pool of N=nr_vcpus rings that are shared by all vCPUs. To mark pages dirty, a vCPU claims a ring, pushes the pages into the ring, and then returns the ring to the pool. If pushing pages hits the soft limit, a request is made to drain the ring and the ring is not returned to the pool until it is drained.

Except for acquiring a ring, which likely can be heavily optimized, that'd allow parallel processing (#1), and would provide a facsimile of #2 as pushing more pages onto a ring would naturally increase the likelihood of triggering a drain. And it might be interesting to see the effect of using different methods of ring selection, e.g. pure round robin, LRU, last used on the current vCPU, etc...

Quoted 4 lines

If you are creating nr_vcpus rings, and draining is done on the vCPU thread that has filled the ring, why not create nr_vcpus+1? The current code then is exactly the same as pre-claiming a ring per vCPU and never releasing it, and using a spinlock to claim the per-VM ring.

Because I really don't like kvm_get_running_vcpu() :-)

Binding the rings to vCPUs also makes for an inflexible API, e.g. the amount of memory required for the rings scales linearly with the number of vCPUs, or maybe there's a use case for having M:N vCPUs:rings.

That being said, I'm pretty clueless when it comes to implementing and tuning the userspace side of this type of stuff, so feel free to ignore my thoughts on the API.

Quoted 16 lines · Sean Christopherson

Quoted 15 lines · Paolo Bonzini

Quoted 9 lines · Sean Christopherson

Quoted 5 lines

I discussed this with Paolo, but I think Paolo preferred the per-vm ring because there's no good reason to choose vcpu0 as what (1) suggested. While if to choose (2) we probably need to lock even for per-cpu ring, so could be a bit slower.

Ya, per-vm is definitely better than dumping on vcpu0. I'm hoping we can find a third option that provides comparable performance without using any per-vcpu rings.

The advantage of per-vCPU rings is that it naturally: 1) parallelizes the processing of dirty pages; 2) makes userspace vCPU thread do more work on vCPUs that dirty more pages.

Quoted 8 lines

Quoted 5 lines

I agree that on the producer side we could reserve multiple entries in the case of PML (and without PML only one entry should be added at a time). But I'm afraid that things get ugly when the ring is full, because you'd have to wait for all vCPUs to finish publishing the entries they have reserved.

Ah, I take it the intended model is that userspace will only start pulling entries off the ring when KVM explicitly signals that the ring is "full"?

No, it's not. But perhaps in the asynchronous case you can delay pushing the reserved entries to the consumer until a moment where no CPUs have left empty slots in the ring buffer (somebody must have done multi-producer ring buffers before). In the ring-full case that is harder because it requires synchronization.

Quoted 7 lines

Rather than reserve entries, what if vCPUs reserved an entire ring? Create a pool of N=nr_vcpus rings that are shared by all vCPUs. To mark pages dirty, a vCPU claims a ring, pushes the pages into the ring, and then returns the ring to the pool. If pushing pages hits the soft limit, a request is made to drain the ring and the ring is not returned to the pool until it is drained.

Quoted 6 lines

Except for acquiring a ring, which likely can be heavily optimized, that'd allow parallel processing (#1), and would provide a facsimile of #2 as pushing more pages onto a ring would naturally increase the likelihood of triggering a drain. And it might be interesting to see the effect of using different methods of ring selection, e.g. pure round robin, LRU, last used on the current vCPU, etc...

If you are creating nr_vcpus rings, and draining is done on the vCPU thread that has filled the ring, why not create nr_vcpus+1? The current code then is exactly the same as pre-claiming a ring per vCPU and never releasing it, and using a spinlock to claim the per-VM ring.

However, we could build on top of my other suggestion to add slot->as_id, and wrap kvm_get_running_vcpu() with a nice API, mimicking exactly what you've suggested. Maybe even add a scary comment around kvm_get_running_vcpu() suggesting that users only do so to avoid locking and wrap it with a nice API. Similar to what get_cpu/put_cpu do with smp_processor_id.

1) Add a pointer from struct kvm_dirty_ring to struct kvm_dirty_ring_indexes:

vcpu->dirty_ring->data = &vcpu->run->vcpu_ring_indexes; kvm->vm_dirty_ring->data = *kvm->vm_run->vm_ring_indexes;

2) push the ring choice and locking to two new functions

struct kvm_ring *kvm_get_dirty_ring(struct kvm *kvm)

{
	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();

	if (vcpu && !WARN_ON_ONCE(vcpu->kvm != kvm)) {
		return &vcpu->dirty_ring;
	} else {
		/*
		 * Put onto per vm ring because no vcpu context.
		 * We'll kick vcpu0 if ring is full.
		 */
		spin_lock(&kvm->vm_dirty_ring->lock);
		return &kvm->vm_dirty_ring;
	}
}

void kvm_put_dirty_ring(struct kvm *kvm,
struct kvm_dirty_ring *ring)

{
	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
	bool full = kvm_dirty_ring_used(ring) >= ring->soft_limit;

	if (ring == &kvm->vm_dirty_ring) {
		if (vcpu == NULL)
			vcpu = kvm->vcpus[0];
		spin_unlock(&kvm->vm_dirty_ring->lock);
	}

	if (full)
		kvm_make_request(KVM_REQ_DIRTY_RING_FULL, vcpu);
}

3) simplify kvm_dirty_ring_push to

void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,

			 u32 slot, u64 offset)
{
	/* left as an exercise to the reader */
}

and mark_page_dirty_in_ring to

static void mark_page_dirty_in_ring(struct kvm *kvm,
struct kvm_memory_slot *slot, gfn_t gfn)

{
	struct kvm_dirty_ring *ring;

	if (!kvm->dirty_ring_size)
		return;

	ring = kvm_get_dirty_ring(kvm);
	kvm_dirty_ring_push(ring, (slot->as_id << 16) | slot->id,
			    gfn - slot->base_gfn);
	kvm_put_dirty_ring(kvm, ring);
}

Just until recently I noticed that actually kvm_get_running_vcpu() has a real benefit in that it gives a very solid result on whether we're with the vcpu context, even more accurate than when we pass vcpu pointers around (because sometimes we just passed the kvm pointer along the stack even if we're with a vcpu context, just like what we did with mark_page_dirty_in_slot).

I'm thinking whether I can start
to use this information in the next post on solving an issue I encountered with the waitqueue.

Current waitqueue is still problematic in that it could wait even with the mmu lock held when with vcpu context.

The issue is KVM_RESET_DIRTY_RINGS needs the mmu lock to manipulate the write bits, while it's the only interface to also wake up the dirty ring sleepers. They could dead lock like this:

main thread vcpu thread
=========== ===========
kvm page fault
mark_page_dirty_in_slot mmu lock taken mark dirty, ring full queue on waitqueue (with mmu lock)
KVM_RESET_DIRTY_RINGS
take mmu lock <------------ deadlock here
reset ring gfns
wakeup dirty ring sleepers

And if we see if the mark_page_dirty_in_slot() is not with a vcpu context (e.g. kvm_mmu_page_fault) but with an ioctl context (those cases we'll use per-vm dirty ring) then it's probably fine.

My planned solution:

- When kvm_get_running_vcpu() != NULL, we postpone the waitqueue waits until we finished handling this page fault, probably in somewhere around vcpu_enter_guest, so that we can do wait_event() after the mmu lock released

Quoted 6 lines

I'm thinking whether I can start
to use this information in the next post on solving an issue I encountered with the waitqueue.

Current waitqueue is still problematic in that it could wait even with the mmu lock held when with vcpu context.

I think the idea of the soft limit is that the waiting just cannot happen. That is, the number of dirtied pages _outside_ the guest (guest accesses are taken care of by PML, and are subtracted from the soft limit) cannot exceed hard_limit - (soft_limit + pml_size).

Quoted 22 lines

The issue is KVM_RESET_DIRTY_RINGS needs the mmu lock to manipulate the write bits, while it's the only interface to also wake up the dirty ring sleepers. They could dead lock like this:

main thread vcpu thread
=========== ===========
kvm page fault
mark_page_dirty_in_slot mmu lock taken mark dirty, ring full queue on waitqueue (with mmu lock)
KVM_RESET_DIRTY_RINGS
take mmu lock <------------ deadlock here
reset ring gfns
wakeup dirty ring sleepers

And if we see if the mark_page_dirty_in_slot() is not with a vcpu context (e.g. kvm_mmu_page_fault) but with an ioctl context (those cases we'll use per-vm dirty ring) then it's probably fine.

Quoted 6 lines

My planned solution:

- When kvm_get_running_vcpu() != NULL, we postpone the waitqueue waits until we finished handling this page fault, probably in somewhere around vcpu_enter_guest, so that we can do wait_event() after the mmu lock released

I think this can cause a race:

vCPU 1 vCPU 2 host
---------------------------------------------------------------
mark page dirty
write to page
treat page as not dirty
add page to ring

where vCPU 2 skips the clean-page slow path entirely.

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines

I'm thinking whether I can start
to use this information in the next post on solving an issue I encountered with the waitqueue.

Current waitqueue is still problematic in that it could wait even with the mmu lock held when with vcpu context.

Quoted 4 lines

I think the idea of the soft limit is that the waiting just cannot happen. That is, the number of dirtied pages _outside_ the guest (guest accesses are taken care of by PML, and are subtracted from the soft limit) cannot exceed hard_limit - (soft_limit + pml_size).

So the question go backs to, whether this is guaranteed somehow? Or do you prefer us to keep the warn_on_once until it triggers then we can analyze (which I doubt..)?

One thing to mention is that for with-vcpu cases, we probably can even stop KVM_RUN immediately as long as either the per-vm or per-vcpu ring reaches the softlimit, then for vcpu case it should be easier to guarantee that. What I want to know is the rest of cases like ioctls or even something not from the userspace (which I think I should read more later..).

Quoted 13 lines

Quoted 10 lines

And if we see if the mark_page_dirty_in_slot() is not with a vcpu context (e.g. kvm_mmu_page_fault) but with an ioctl context (those cases we'll use per-vm dirty ring) then it's probably fine.

My planned solution:

- When kvm_get_running_vcpu() != NULL, we postpone the waitqueue waits until we finished handling this page fault, probably in somewhere around vcpu_enter_guest, so that we can do wait_event() after the mmu lock released

I think this can cause a race:

Quoted 8 lines

vCPU 1 vCPU 2 host
---------------------------------------------------------------
mark page dirty
write to page
treat page as not dirty
add page to ring

where vCPU 2 skips the clean-page slow path entirely.

If we're still with the rule in userspace that we first do RESET then collect and send the pages (just like what we've discussed before), then IMHO it's fine to have vcpu2 to skip the slow path? Because RESET happens at "treat page as not dirty", then if we are sure that we only collect and send pages after that point, then the latest "write to page" data from vcpu2 won't be lost even if vcpu2 is not blocked by vcpu1's ring full?

Maybe we can also consider to let mark_page_dirty_in_slot() return a value, then the upper layer could have a chance to skip the spte update if mark_page_dirty_in_slot() fails to mark the dirty bit, so it can return directly with RET_PF_RETRY.

Quoted 8 lines · Peter Xu

Quoted 7 lines · Paolo Bonzini

Quoted 6 lines

I'm thinking whether I can start
to use this information in the next post on solving an issue I encountered with the waitqueue.

Current waitqueue is still problematic in that it could wait even with the mmu lock held when with vcpu context.

Quoted 8 lines

Quoted 4 lines

I think the idea of the soft limit is that the waiting just cannot happen. That is, the number of dirtied pages _outside_ the guest (guest accesses are taken care of by PML, and are subtracted from the soft limit) cannot exceed hard_limit - (soft_limit + pml_size).

So the question go backs to, whether this is guaranteed somehow? Or do you prefer us to keep the warn_on_once until it triggers then we can analyze (which I doubt..)?

Yes, I would like to keep the WARN_ON_ONCE just because you never know.

Of course it would be much better to audit the calls to kvm_write_guest and figure out how many could trigger (e.g. two from the operands of an emulated instruction, 5 from a nested EPT walk, 1 from a page walk, etc.).

Quoted 7 lines

One thing to mention is that for with-vcpu cases, we probably can even stop KVM_RUN immediately as long as either the per-vm or per-vcpu ring reaches the softlimit, then for vcpu case it should be easier to guarantee that. What I want to know is the rest of cases like ioctls or even something not from the userspace (which I think I should read more later..).

Which ioctls? Most ioctls shouldn't dirty memory at all.

Quoted 23 lines

Quoted 14 lines

Quoted 11 lines

And if we see if the mark_page_dirty_in_slot() is not with a vcpu context (e.g. kvm_mmu_page_fault) but with an ioctl context (those cases we'll use per-vm dirty ring) then it's probably fine.

My planned solution:

- When kvm_get_running_vcpu() != NULL, we postpone the waitqueue waits until we finished handling this page fault, probably in somewhere around vcpu_enter_guest, so that we can do wait_event() after the mmu lock released

I think this can cause a race:

Quoted 8 lines

vCPU 1 vCPU 2 host
---------------------------------------------------------------
mark page dirty
write to page
treat page as not dirty
add page to ring

where vCPU 2 skips the clean-page slow path entirely.

Quoted 7 lines

If we're still with the rule in userspace that we first do RESET then collect and send the pages (just like what we've discussed before), then IMHO it's fine to have vcpu2 to skip the slow path? Because RESET happens at "treat page as not dirty", then if we are sure that we only collect and send pages after that point, then the latest "write to page" data from vcpu2 won't be lost even if vcpu2 is not blocked by vcpu1's ring full?

Good point, the race would become

(quoted diff) +0 -0

 	vCPU 1			vCPU 2		host
 	---------------------------------------------------------------
 	mark page dirty
 				write to page

reset rings
wait for mmu lock
add page to ring
release mmu lock
...do reset... release mmu lock
page is now dirty

Quoted 5 lines

Maybe we can also consider to let mark_page_dirty_in_slot() return a value, then the upper layer could have a chance to skip the spte update if mark_page_dirty_in_slot() fails to mark the dirty bit, so it can return directly with RET_PF_RETRY.

I don't think that's possible, most writes won't come from a page fault path and cannot retry.

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]

We should only need to look at the leaves of the traces because they're where the dirty request starts. I'm marking all the leaves with below criteria then it'll be easier to focus:

Cases with [*]: should not matter much
[&]: actually with a per-vcpu context in the upper layer [?]: uncertain...

I'm a bit amazed after I took these notes, since I found that besides those that could probbaly be ignored (marked as [*]), most of the rest per-vm dirty requests are actually with a vcpu context.

Although now because we have kvm_get_running_vcpu() all cases for [&] should be fine without changing anything, but I tend to add another patch in the next post to convert all the [&] cases explicitly to pass vcpu pointer instead of kvm pointer to be clear if no one disagrees, then we verify that against kvm_get_running_vcpu().

init_rmode_tss or init_rmode_identity_map. But I've marked them as unimportant because they should only happen once at boot.

Quoted 14 lines

Quoted 7 lines

If we're still with the rule in userspace that we first do RESET then collect and send the pages (just like what we've discussed before), then IMHO it's fine to have vcpu2 to skip the slow path? Because RESET happens at "treat page as not dirty", then if we are sure that we only collect and send pages after that point, then the latest "write to page" data from vcpu2 won't be lost even if vcpu2 is not blocked by vcpu1's ring full?

Good point, the race would become

(quoted diff) +0 -0

 	vCPU 1			vCPU 2		host
 	---------------------------------------------------------------
 	mark page dirty
 				write to page

Quoted 7 lines

reset rings
wait for mmu lock
add page to ring
release mmu lock
...do reset... release mmu lock
page is now dirty

Hmm, the page will be dirty after the reset, but is that an issue?

Or, could you help me to identify what I've missed?

Quoted 2 lines

I don't think that's possible, most writes won't come from a page fault path and cannot retry.

Yep, maybe I should say it in the other way round: we only wait if kvm_get_running_vcpu() == NULL. Then in somewhere near vcpu_enter_guest(), we add a check to wait if per-vcpu ring is full. Would that work?

Quoted 5 lines

Although now because we have kvm_get_running_vcpu() all cases for [&] should be fine without changing anything, but I tend to add another patch in the next post to convert all the [&] cases explicitly to pass vcpu pointer instead of kvm pointer to be clear if no one disagrees, then we verify that against kvm_get_running_vcpu().

This is a good idea but remember not to convert those to kvm_vcpu_write_guest, because you _don't_ want these writes to touch SMRAM (most of the addresses are OS-controlled rather than firmware-controlled).

Quoted 3 lines

init_rmode_tss or init_rmode_identity_map. But I've marked them as unimportant because they should only happen once at boot.

We need to check if userspace can add an arbitrary number of entries by calling KVM_SET_TSS_ADDR repeatedly. I think it can; we'd have to forbid multiple calls to KVM_SET_TSS_ADDR which is not a problem in general.

Quoted 7 lines

Quoted 2 lines

I don't think that's possible, most writes won't come from a page fault path and cannot retry.

Yep, maybe I should say it in the other way round: we only wait if kvm_get_running_vcpu() == NULL. Then in somewhere near vcpu_enter_guest(), we add a check to wait if per-vcpu ring is full. Would that work?

Yes, that should work, especially if we know that kvmgt is the only case that can wait. And since:

1) kvmgt doesn't really need dirty page tracking (because VFIO devices generally don't track dirty pages, and because kvmgt shouldn't be using kvm_write_guest anyway)

2) the real mode TSS and identity map shouldn't even be tracked, as they are invisible to userspace

it seems to me that kvm_get_running_vcpu() lets us get rid of the per-VM ring altogether.

Quoted 6 lines · Paolo Bonzini

Quoted 5 lines

Although now because we have kvm_get_running_vcpu() all cases for [&] should be fine without changing anything, but I tend to add another patch in the next post to convert all the [&] cases explicitly to pass vcpu pointer instead of kvm pointer to be clear if no one disagrees, then we verify that against kvm_get_running_vcpu().

Quoted 4 lines

This is a good idea but remember not to convert those to kvm_vcpu_write_guest, because you _don't_ want these writes to touch SMRAM (most of the addresses are OS-controlled rather than firmware-controlled).

OK. I think I only need to pass in vcpu* instead of kvm* in kvm_write_guest_page() just like kvm_vcpu_write_guest(), however we still keep to only write to address space id==0 for that.

Quoted 6 lines

Quoted 2 lines

init_rmode_tss or init_rmode_identity_map. But I've marked them as unimportant because they should only happen once at boot.

We need to check if userspace can add an arbitrary number of entries by calling KVM_SET_TSS_ADDR repeatedly. I think it can; we'd have to forbid multiple calls to KVM_SET_TSS_ADDR which is not a problem in general.

Will do that altogether with the series. I can further change both of these calls to not track dirty at all, which shouldn't be hard, after all userspace didn't even know them, as you mentioned below.

Is there anything to explain what KVM_SET_TSS_ADDR is used for? This is the thing I found that is closest to useful (from api.txt):

So... has it really popped into existance somewhere? It would be good at least to know why it does not need to be migrated.

Quoted 12 lines · Peter Xu

Quoted 10 lines · Paolo Bonzini

Quoted 5 lines

Although now because we have kvm_get_running_vcpu() all cases for [&] should be fine without changing anything, but I tend to add another patch in the next post to convert all the [&] cases explicitly to pass vcpu pointer instead of kvm pointer to be clear if no one disagrees, then we verify that against kvm_get_running_vcpu().

This is a good idea but remember not to convert those to kvm_vcpu_write_guest, because you _don't_ want these writes to touch SMRAM (most of the addresses are OS-controlled rather than firmware-controlled).

Quoted 3 lines

OK. I think I only need to pass in vcpu* instead of kvm* in kvm_write_guest_page() just like kvm_vcpu_write_guest(), however we still keep to only write to address space id==0 for that.

No, please pass it all the way down to the [&] functions but not to kvm_write_guest_page. Those should keep using vcpu->kvm.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

init_rmode_tss or init_rmode_identity_map. But I've marked them as unimportant because they should only happen once at boot.

We need to check if userspace can add an arbitrary number of entries by calling KVM_SET_TSS_ADDR repeatedly. I think it can; we'd have to forbid multiple calls to KVM_SET_TSS_ADDR which is not a problem in general.

Quoted 6 lines

Will do that altogether with the series. I can further change both of these calls to not track dirty at all, which shouldn't be hard, after all userspace didn't even know them, as you mentioned below.

Is there anything to explain what KVM_SET_TSS_ADDR is used for? This is the thing I found that is closest to useful (from api.txt):

The best description is probably at https://lwn.net/Articles/658883/:

They are needed for unrestricted_guest=0. Remember that, in that case, the VM always runs in protected mode and with paging enabled. In order to emulate real mode you put the guest in a vm86 task, so you need some place for a TSS and for a page table, and they must be in guest RAM because the guest's TR and CR3 points to it. They are invisible to the guest, because the STR and MOV-from-CR instructions are invalid in vm86 mode, but it must be there.

If you don't call KVM_SET_TSS_ADDR you actually get a complaint in dmesg, and the TR stays at 0. I am not really sure what kind of bad things can happen with unrestricted_guest=0, probably you just get a VM Entry failure. The TSS takes 3 pages of memory. An interesting point is that you actually don't need to set the TR selector to a valid value (as you would do when running in "normal" vm86 mode), you can simply set the base and limit registers that are hidden in the processor, and generally inaccessible except through VMREAD/VMWRITE or system management mode. So KVM needs to set up a TSS but not a GDT.

For paging, instead, 1 page is enough because we have only 4GB of memory to address. KVM disables CR4.PAE (page address extensions, aka 8-byte entries in each page directory or page table) and enables CR4.PSE (page size extensions, aka 4MB huge pages support with 4-byte page directory entries). One page then fits 1024 4-byte page directory entries, each for a 4MB huge pages, totaling exactly 4GB. Here if you don't set it the page table is at address 0xFFFBC000. QEMU changes it to 0xFEFFC000 so that the BIOS can be up to 16MB in size (the default only allows 256k between 0xFFFC0000 and 0xFFFFFFFF).

The different handling, where only the page table has a default, is unfortunate, but so goes life...

Quoted 2 lines

So... has it really popped into existance somewhere? It would be good at least to know why it does not need to be migrated.

It does not need to be migrated just because the contents are constant.

Quoted 2 lines

No, please pass it all the way down to the [&] functions but not to kvm_write_guest_page. Those should keep using vcpu->kvm.

Actually I even wanted to refactor these helpers. I mean, we have two sets of helpers now, kvm_[vcpu]_{read|write}*(), so one set is per-vm, the other set is per-vcpu. IIUC the only difference of these two are whether we should consider ((vcpu)->arch.hflags & HF_SMM_MASK) or we just write to address space zero always.

Could we unify them into a
single set of helper (I'll just drop the *_vcpu_* helpers because it's longer when write) but we always pass in vcpu* as the first parameter? Then we add another parameter "vcpu_smm" to show whether we want to consider the HF_SMM_MASK flag.

Quoted 7 lines · Peter Xu

Quoted 2 lines

No, please pass it all the way down to the [&] functions but not to kvm_write_guest_page. Those should keep using vcpu->kvm.

Actually I even wanted to refactor these helpers. I mean, we have two sets of helpers now, kvm_[vcpu]_{read|write}*(), so one set is per-vm, the other set is per-vcpu. IIUC the only difference of these two are whether we should consider ((vcpu)->arch.hflags & HF_SMM_MASK) or we just write to address space zero always.

Right.

Quoted 5 lines

Could we unify them into a
single set of helper (I'll just drop the *_vcpu_* helpers because it's longer when write) but we always pass in vcpu* as the first parameter? Then we add another parameter "vcpu_smm" to show whether we want to consider the HF_SMM_MASK flag.

You'd have to check through all KVM implementations whether you always have the vCPU. Also non-x86 doesn't have address spaces, and by the time you add ", true" or ", false" it's longer than the "_vcpu_" you have removed. So, not a good idea in my opinion. :D

Quoted 17 lines · Paolo Bonzini

Quoted 7 lines · Peter Xu

Quoted 2 lines

No, please pass it all the way down to the [&] functions but not to kvm_write_guest_page. Those should keep using vcpu->kvm.

Actually I even wanted to refactor these helpers. I mean, we have two sets of helpers now, kvm_[vcpu]_{read|write}*(), so one set is per-vm, the other set is per-vcpu. IIUC the only difference of these two are whether we should consider ((vcpu)->arch.hflags & HF_SMM_MASK) or we just write to address space zero always.

Right.

Quoted 5 lines

Could we unify them into a
single set of helper (I'll just drop the *_vcpu_* helpers because it's longer when write) but we always pass in vcpu* as the first parameter? Then we add another parameter "vcpu_smm" to show whether we want to consider the HF_SMM_MASK flag.

Quoted 4 lines

You'd have to check through all KVM implementations whether you always have the vCPU. Also non-x86 doesn't have address spaces, and by the time you add ", true" or ", false" it's longer than the "_vcpu_" you have removed. So, not a good idea in my opinion. :D

Well, now I've changed my mind. :) (considering that we still have many places that will not have vcpu*...)

I can simply add that "vcpu_smm" parameter to kvm_vcpu_write_*() without removing the kvm_write_*() helpers. Then I'll be able to convert most of the kvm_write_*() (or its family) callers to kvm_vcpu_write*(..., vcpu_smm=false) calls where proper.

Would that be good?

I've lost track of the problem you're trying to solve, but if you do something like "vcpu_smm=false", explicitly pass an address space ID instead of hardcoding x86 specific SMM crud, e.g.

	kvm_vcpu_write*(..., as_id=0);

Quoted 4 lines · Sean Christopherson

I've lost track of the problem you're trying to solve, but if you do something like "vcpu_smm=false", explicitly pass an address space ID instead of hardcoding x86 specific SMM crud, e.g.

Quoted 1 line

	kvm_vcpu_write*(..., as_id=0);

And the point of having kvm_vcpu_* vs. kvm_write_* was exactly to not having to hardcode the address space ID. If anything you could add a __kvm_vcpu_write_* API that takes vcpu+as_id, but really I'd prefer to keep kvm_get_running_vcpu() for now and then it can be refactored later.
There are already way too many memory r/w APIs...

From: Paolo Bonzini
Sent: Monday, December 16, 2019 6:08 PM

[Alex and Kevin: there are doubts below regarding dirty page tracking from VFIO and mdev devices, which perhaps you can help with]

Quoted 5 lines · Peter Xu

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]

These don't matter because their content is not visible to userspace (the backing storage is mmap-ed by __x86_set_memory_region). In fact, d

Quoted 9 lines

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]

This then expands (partially) to

intel_gvt_hypervisor_write_gpa
emulate_csb_update
emulate_execlist_ctx_schedule_out
complete_execlist_workload
complete_current_workload
workload_thread
emulate_execlist_ctx_schedule_in
prepare_execlist_workload
prepare_workload
dispatch_workload
workload_thread

So KVMGT is always writing to GPAs instead of IOVAs and basically bypassing a guest IOMMU. So here it would be better if kvmgt was changed not use kvm_write_guest (also because I'd probably have nacked that if I had known :)).

As far as I know, there is some work on live migration with both VFIO and mdev, and that probably includes some dirty page tracking API. kvmgt could switch to that API, or there could be VFIO APIs similar to kvm_write_guest but taking IOVAs instead of GPAs. Advantage: this would fix the GPA/IOVA confusion. Disadvantage: userspace would lose the tracking of writes from mdev devices. Kevin, are these writes used in any way? Do the calls to intel_gvt_hypervisor_write_gpa covers all writes from kvmgt vGPUs, or can the hardware write to memory as well (which would be my guess if I didn't know anything about kvmgt, which I pretty much don't)?

Quoted 13 lines

We should only need to look at the leaves of the traces because they're where the dirty request starts. I'm marking all the leaves with below criteria then it'll be easier to focus:

Cases with [*]: should not matter much
[&]: actually with a per-vcpu context in the upper layer [?]: uncertain...

I'm a bit amazed after I took these notes, since I found that besides those that could probbaly be ignored (marked as [*]), most of the rest per-vm dirty requests are actually with a vcpu context.

Quoted 5 lines

Although now because we have kvm_get_running_vcpu() all cases for [&] should be fine without changing anything, but I tend to add another patch in the next post to convert all the [&] cases explicitly to pass vcpu pointer instead of kvm pointer to be clear if no one disagrees, then we verify that against kvm_get_running_vcpu().

This is a good idea but remember not to convert those to kvm_vcpu_write_guest, because you _don't_ want these writes to touch SMRAM (most of the addresses are OS-controlled rather than firmware-controlled).

Quoted 2 lines

init_rmode_tss or init_rmode_identity_map. But I've marked them as unimportant because they should only happen once at boot.

We need to check if userspace can add an arbitrary number of entries by calling KVM_SET_TSS_ADDR repeatedly. I think it can; we'd have to forbid multiple calls to KVM_SET_TSS_ADDR which is not a problem in general.

Quoted 22 lines

Quoted 14 lines

Quoted 7 lines

If we're still with the rule in userspace that we first do RESET then collect and send the pages (just like what we've discussed before), then IMHO it's fine to have vcpu2 to skip the slow path? Because RESET happens at "treat page as not dirty", then if we are sure that we only collect and send pages after that point, then the latest "write to page" data from vcpu2 won't be lost even if vcpu2 is not blocked by vcpu1's ring full?

Good point, the race would become

(quoted diff) +0 -0

 	vCPU 1			vCPU 2		host
 	---------------------------------------------------------------
 	mark page dirty
 				write to page

Quoted 7 lines

reset rings
wait for mmu lock
add page to ring
release mmu lock
...do reset... release mmu lock
page is now dirty

Quoted 3 lines

Hmm, the page will be dirty after the reset, but is that an issue?

Or, could you help me to identify what I've missed?

Nothing: the race is always solved in such a way that there's no issue.

Quoted 7 lines

Quoted 2 lines

I don't think that's possible, most writes won't come from a page fault path and cannot retry.

Yep, maybe I should say it in the other way round: we only wait if kvm_get_running_vcpu() == NULL. Then in somewhere near vcpu_enter_guest(), we add a check to wait if per-vcpu ring is full. Would that work?

Yes, that should work, especially if we know that kvmgt is the only case that can wait. And since:

1) kvmgt doesn't really need dirty page tracking (because VFIO devices generally don't track dirty pages, and because kvmgt shouldn't be using kvm_write_guest anyway)

2) the real mode TSS and identity map shouldn't even be tracked, as they are invisible to userspace

it seems to me that kvm_get_running_vcpu() lets us get rid of the per-VM ring altogether.

Paolo

Quoted 40 lines

From: Paolo Bonzini
Sent: Monday, December 16, 2019 6:08 PM

[Alex and Kevin: there are doubts below regarding dirty page tracking from VFIO and mdev devices, which perhaps you can help with]

Quoted 5 lines · Peter Xu

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]  

These don't matter because their content is not visible to userspace (the backing storage is mmap-ed by __x86_set_memory_region). In fact, d

Quoted 9 lines

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]  

This then expands (partially) to

intel_gvt_hypervisor_write_gpa
emulate_csb_update
emulate_execlist_ctx_schedule_out
complete_execlist_workload
complete_current_workload
workload_thread
emulate_execlist_ctx_schedule_in
prepare_execlist_workload
prepare_workload
dispatch_workload
workload_thread

Quoted 4 lines

So KVMGT is always writing to GPAs instead of IOVAs and basically bypassing a guest IOMMU. So here it would be better if kvmgt was changed not use kvm_write_guest (also because I'd probably have nacked that if I had known :)).

I agree.

Quoted 11 lines

As far as I know, there is some work on live migration with both VFIO and mdev, and that probably includes some dirty page tracking API. kvmgt could switch to that API, or there could be VFIO APIs similar to kvm_write_guest but taking IOVAs instead of GPAs. Advantage: this would fix the GPA/IOVA confusion. Disadvantage: userspace would lose the tracking of writes from mdev devices. Kevin, are these writes used in any way? Do the calls to intel_gvt_hypervisor_write_gpa covers all writes from kvmgt vGPUs, or can the hardware write to memory as well (which would be my guess if I didn't know anything about kvmgt, which I pretty much don't)?

intel_gvt_hypervisor_write_gpa covers all writes due to software mediation.

for hardware updates, it needs be mapped in IOMMU through vfio_pin_pages before any DMA happens. The ongoing dirty tracking effort in VFIO will take every pinned page through that API as dirtied.

However, currently VFIO doesn't implement any vfio_read/write_guest interface yet. and it doesn't make sense to use vfio_pin_pages for software dirtied pages, as pin is unnecessary and heavy involving iommu invalidation.

Alex, if you are OK we'll work on such interface and move kvmgt to use it. After it's accepted, we can also mark pages dirty through this new interface in Kirti's dirty page tracking series.

Quoted 4 lines

Alex, if you are OK we'll work on such interface and move kvmgt to use it. After it's accepted, we can also mark pages dirty through this new interface in Kirti's dirty page tracking series.

I'm not sure what you're asking for, is it an interface for the host CPU to read/write the memory backing of a mapped IOVA range without pinning pages? That seems like something like that would make sense for an emulation model where a page does not need to be pinned for physical DMA. If you're asking more for an interface that understands the userspace driver is a VM (ie. implied using a _guest postfix on the function name) and knows about GPA mappings beyond the windows directly mapped for device access, I'd not look fondly on such a request.

From: Paolo Bonzini <[email protected]>
Sent: Wednesday, December 18, 2019 12:31 AM

Quoted 3 lines · Alex Williamson

Quoted 3 lines

Alex, if you are OK we'll work on such interface and move kvmgt to use it. After it's accepted, we can also mark pages dirty through this new

interface

Quoted 8 lines

Quoted 1 line

in Kirti's dirty page tracking series.

I'm not sure what you're asking for, is it an interface for the host CPU to read/write the memory backing of a mapped IOVA range without pinning pages? That seems like something like that would make sense for an emulation model where a page does not need to be pinned for physical DMA. If you're asking more for an interface that understands the userspace driver is a VM (ie. implied using a _guest postfix on the function name) and knows about GPA mappings beyond the windows

directly

Quoted 1 line

mapped for device access, I'd not look fondly on such a request.

No, it would definitely be the former, using IOVAs to access guest memory---kvmgt is currently doing the latter by calling into KVM, and I'm not really fond of that either.

From: Tian, Kevin
Sent: Tuesday, December 17, 2019 10:29 AM

Quoted 40 lines

From: Paolo Bonzini
Sent: Monday, December 16, 2019 6:08 PM

[Alex and Kevin: there are doubts below regarding dirty page tracking from VFIO and mdev devices, which perhaps you can help with]

Quoted 5 lines · Peter Xu

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]

These don't matter because their content is not visible to userspace (the backing storage is mmap-ed by __x86_set_memory_region). In fact, d

Quoted 9 lines

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]

This then expands (partially) to

intel_gvt_hypervisor_write_gpa
emulate_csb_update
emulate_execlist_ctx_schedule_out
complete_execlist_workload
complete_current_workload
workload_thread
emulate_execlist_ctx_schedule_in
prepare_execlist_workload
prepare_workload
dispatch_workload
workload_thread

Quoted 4 lines

So KVMGT is always writing to GPAs instead of IOVAs and basically bypassing a guest IOMMU. So here it would be better if kvmgt was changed not use kvm_write_guest (also because I'd probably have nacked that if I had known :)).

I agree.

Quoted 11 lines

As far as I know, there is some work on live migration with both VFIO and mdev, and that probably includes some dirty page tracking API. kvmgt could switch to that API, or there could be VFIO APIs similar to kvm_write_guest but taking IOVAs instead of GPAs. Advantage: this would fix the GPA/IOVA confusion. Disadvantage: userspace would lose the tracking of writes from mdev devices. Kevin, are these writes used in any way? Do the calls to intel_gvt_hypervisor_write_gpa covers all writes from kvmgt vGPUs, or can the hardware write to memory as well (which would be my guess if I didn't know anything about kvmgt, which I pretty much don't)?

intel_gvt_hypervisor_write_gpa covers all writes due to software mediation.

for hardware updates, it needs be mapped in IOMMU through vfio_pin_pages
before any DMA happens. The ongoing dirty tracking effort in VFIO will take every pinned page through that API as dirtied.

However, currently VFIO doesn't implement any vfio_read/write_guest interface yet. and it doesn't make sense to use vfio_pin_pages for software dirtied pages, as pin is unnecessary and heavy involving iommu invalidation.

Alex, if you are OK we'll work on such interface and move kvmgt to use it. After it's accepted, we can also mark pages dirty through this new interface in Kirti's dirty page tracking series.

Quoted 69 lines

From: Tian, Kevin
Sent: Tuesday, December 17, 2019 10:29 AM

Quoted 40 lines

From: Paolo Bonzini
Sent: Monday, December 16, 2019 6:08 PM

[Alex and Kevin: there are doubts below regarding dirty page tracking from VFIO and mdev devices, which perhaps you can help with]

Quoted 5 lines · Peter Xu

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]

These don't matter because their content is not visible to userspace (the backing storage is mmap-ed by __x86_set_memory_region). In fact, d

Quoted 9 lines

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]

This then expands (partially) to

intel_gvt_hypervisor_write_gpa
emulate_csb_update
emulate_execlist_ctx_schedule_out
complete_execlist_workload
complete_current_workload
workload_thread
emulate_execlist_ctx_schedule_in
prepare_execlist_workload
prepare_workload
dispatch_workload
workload_thread

Quoted 4 lines

So KVMGT is always writing to GPAs instead of IOVAs and basically bypassing a guest IOMMU. So here it would be better if kvmgt was changed not use kvm_write_guest (also because I'd probably have nacked that if I had known :)).

I agree.

Quoted 11 lines

As far as I know, there is some work on live migration with both VFIO and mdev, and that probably includes some dirty page tracking API. kvmgt could switch to that API, or there could be VFIO APIs similar to kvm_write_guest but taking IOVAs instead of GPAs. Advantage: this would fix the GPA/IOVA confusion. Disadvantage: userspace would lose the tracking of writes from mdev devices. Kevin, are these writes used in any way? Do the calls to intel_gvt_hypervisor_write_gpa covers all writes from kvmgt vGPUs, or can the hardware write to memory as well (which would be my guess if I didn't know anything about kvmgt, which I pretty much don't)?

intel_gvt_hypervisor_write_gpa covers all writes due to software mediation.

for hardware updates, it needs be mapped in IOMMU through vfio_pin_pages
before any DMA happens. The ongoing dirty tracking effort in VFIO will take every pinned page through that API as dirtied.

Quoted 3 lines

However, currently VFIO doesn't implement any vfio_read/write_guest interface yet. and it doesn't make sense to use vfio_pin_pages for software dirtied pages, as pin is unnecessary and heavy involving iommu invalidation.

One correction. vfio_pin_pages doesn't involve iommu invalidation. I should just mean that pinning the page is not necessary. We just need a kvm-like interface based on hva to access.

Quoted 5 lines

Alex, if you are OK we'll work on such interface and move kvmgt to use it. After it's accepted, we can also mark pages dirty through this new interface in Kirti's dirty page tracking series.

Quoted 69 lines · Kevin

Quoted 69 lines

From: Tian, Kevin
Sent: Tuesday, December 17, 2019 10:29 AM

Quoted 40 lines

From: Paolo Bonzini
Sent: Monday, December 16, 2019 6:08 PM

[Alex and Kevin: there are doubts below regarding dirty page tracking from VFIO and mdev devices, which perhaps you can help with]

Quoted 5 lines · Peter Xu

                init_rmode_tss
                    vmx_set_tss_addr
                        kvm_vm_ioctl_set_tss_addr [*]
                init_rmode_identity_map
                    vmx_create_vcpu [*]  

These don't matter because their content is not visible to userspace (the backing storage is mmap-ed by __x86_set_memory_region). In fact, d

Quoted 9 lines

                vmx_write_pml_buffer
                    kvm_arch_write_log_dirty [&]
                kvm_write_guest
                    kvm_hv_setup_tsc_page
                        kvm_guest_time_update [&]
                    nested_flush_cached_shadow_vmcs12 [&]
                    kvm_write_wall_clock [&]
                    kvm_pv_clock_pairing [&]
                    kvmgt_rw_gpa [?]  

This then expands (partially) to

intel_gvt_hypervisor_write_gpa
emulate_csb_update
emulate_execlist_ctx_schedule_out
complete_execlist_workload
complete_current_workload
workload_thread
emulate_execlist_ctx_schedule_in
prepare_execlist_workload
prepare_workload
dispatch_workload
workload_thread

Quoted 4 lines

So KVMGT is always writing to GPAs instead of IOVAs and basically bypassing a guest IOMMU. So here it would be better if kvmgt was changed not use kvm_write_guest (also because I'd probably have nacked that if I had known :)).

I agree.

Quoted 11 lines

As far as I know, there is some work on live migration with both VFIO and mdev, and that probably includes some dirty page tracking API. kvmgt could switch to that API, or there could be VFIO APIs similar to kvm_write_guest but taking IOVAs instead of GPAs. Advantage: this would fix the GPA/IOVA confusion. Disadvantage: userspace would lose the tracking of writes from mdev devices. Kevin, are these writes used in any way? Do the calls to intel_gvt_hypervisor_write_gpa covers all writes from kvmgt vGPUs, or can the hardware write to memory as well (which would be my guess if I didn't know anything about kvmgt, which I pretty much don't)?

intel_gvt_hypervisor_write_gpa covers all writes due to software mediation.

for hardware updates, it needs be mapped in IOMMU through vfio_pin_pages
before any DMA happens. The ongoing dirty tracking effort in VFIO will take every pinned page through that API as dirtied.

Quoted 8 lines

Quoted 3 lines

However, currently VFIO doesn't implement any vfio_read/write_guest interface yet. and it doesn't make sense to use vfio_pin_pages for software dirtied pages, as pin is unnecessary and heavy involving iommu invalidation.

One correction. vfio_pin_pages doesn't involve iommu invalidation. I should just mean that pinning the page is not necessary. We just need a kvm-like interface based on hva to access.

And can we propose to differentiate read and write when calling vfio_pin_pages, e.g. vfio_pin_pages_read, vfio_pin_pages_write? Otherwise, calling to vfio_pin_pages will unnecessarily cause read pages to be dirty and sometimes reading guest pages is a way for device model to track dirty pages.

virt/kvm/kvm_main.c +5 -0

+	u64 offset;
+	int ret;
+	struct kvm_dirty_ring *ring;
+	struct kvm_dirty_ring_indexes *indexes;
+	bool is_vm_ring;

virt/kvm/kvm_main.c +20 -0

+		ring = &kvm->vm_dirty_ring;
+		indexes = &kvm->vm_run->vm_ring_indexes;
+		is_vm_ring = true;
+	}
+
+	ret = kvm_dirty_ring_push(ring, indexes,
+				  (as_id << 16)|slot->id, offset,
+				  is_vm_ring);
+	if (ret < 0) {
+		if (is_vm_ring)
+			pr_warn_once("vcpu %d dirty log overflow\n",
+				     vcpu->vcpu_id);
+		else
+			pr_warn_once("per-vm dirty log overflow\n");
+		return;
+	}
+
+	if (ret)
+		kvm_make_request(KVM_REQ_DIRTY_RING_FULL, vcpu);
+}

The setting of as_id is wrong, both with and without a vCPU. as_id should come from slot->as_id.

virt/kvm/kvm_main.c static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg) · line 3355

Quoted 5 lines

virt/kvm/kvm_main.c +5 -0

+		/*
+		 * Put onto per vm ring because no vcpu context.  Kick
+		 * vcpu0 if ring is full.
+		 */
+		vcpu = kvm->vcpus[0];

Is this a rare event?

Quoted 20 lines

virt/kvm/kvm_main.c +20 -0

+		ring = &kvm->vm_dirty_ring;
+		indexes = &kvm->vm_run->vm_ring_indexes;
+		is_vm_ring = true;
+	}
+
+	ret = kvm_dirty_ring_push(ring, indexes,
+				  (as_id << 16)|slot->id, offset,
+				  is_vm_ring);
+	if (ret < 0) {
+		if (is_vm_ring)
+			pr_warn_once("vcpu %d dirty log overflow\n",
+				     vcpu->vcpu_id);
+		else
+			pr_warn_once("per-vm dirty log overflow\n");
+		return;
+	}
+
+	if (ret)
+		kvm_make_request(KVM_REQ_DIRTY_RING_FULL, vcpu);
+}

Quoted 2 lines · Sean Christopherson

The setting of as_id is wrong, both with and without a vCPU. as_id should come from slot->as_id.

Which doesn't exist, but is an excellent suggestion nevertheless.

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

+int kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
+			struct kvm_dirty_ring_indexes *indexes,
+			u32 slot, u64 offset, bool lock)
+{
+	int ret;
+	struct kvm_dirty_gfn *entry;
+
+	if (lock)

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

+	smp_wmb();
+	ring->dirty_index++;
+	WRITE_ONCE(indexes->avail_index, ring->dirty_index);
+	ret = kvm_dirty_ring_used(ring) >= ring->soft_limit;
+	pr_info("%s: slot %u offset %llu used %u\n",
+		__func__, slot, offset, kvm_dirty_ring_used(ring));
+
+out:

Quoted 4 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

[1] https://lkml.org/lkml/2019/4/9/5

Quoted 9 lines · Jason Wang

Quoted 4 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 3 lines

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

Yeah, I don't think that would be better than mmap.

Quoted 11 lines · Paolo Bonzini

Quoted 7 lines · Jason Wang

Quoted 3 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 4 lines

Quoted 3 lines

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

Yeah, I don't think that would be better than mmap.

Yeah I agree, because I didn't see how copy_to/from_user() helped to do icache/dcache flushings...

Some context here: Jason raised this question offlist first on whether we should also need these flush_dcache_cache() helpers for operations like kvm dirty ring accesses. I feel like it should, however I've got two other questions, on:

- if we need to do flush_dcache_page() on kernel modified pages (assuming the same page has mapped to userspace), then why don't we need flush_cache_page() too on the page, where flush_cache_page() is defined not-a-nop on those archs?

- assuming an arch has not-a-nop impl for flush_[d]cache_page(), would atomic operations like cmpxchg really work for them (assuming that ISAs like cmpxchg should depend on cache consistency).

Sorry I think these are for sure a bit out of topic for kvm dirty ring patchset, but since we're at it, I'm raising the questions up in case there're answers..

Thanks,

Quoted 15 lines · Peter Xu

Quoted 14 lines · Paolo Bonzini

Quoted 7 lines · Jason Wang

Quoted 3 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 3 lines

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

Quoted 3 lines

Quoted 1 line

Yeah, I don't think that would be better than mmap.

Yeah I agree, because I didn't see how copy_to/from_user() helped to do icache/dcache flushings...

It looks to me one advantage is that exact the same VA is used by both userspace and kernel so there will be no alias.

Quoted 15 lines · Jason Wang

Quoted 14 lines · Peter Xu

Quoted 13 lines · Paolo Bonzini

Quoted 6 lines · Jason Wang

Quoted 3 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 3 lines

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Yeah, I don't think that would be better than mmap.

Yeah I agree, because I didn't see how copy_to/from_user() helped to do icache/dcache flushings...

It looks to me one advantage is that exact the same VA is used by both userspace and kernel so there will be no alias.

Hmm.. but what if the page is mapped more than once in user? Thanks,

Quoted 6 lines · Jason Wang

Quoted 4 lines

+    entry = &ring->dirty_gfns[ring->dirty_index & (ring->size - 1)]; +    entry->slot = slot;
+    entry->offset = offset;

Quoted 3 lines

Haven't gone through the whole series, sorry if it was a silly question but I wonder things like this will suffer from similar issue on virtually tagged archs as mentioned in [1].

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 3 lines

Is this better to allocate the ring from userspace and set to KVM instead? Then we can use copy_to/from_user() friends (a little bit slow on recent CPUs).

Yeah, I don't think that would be better than mmap.

Paolo

Quoted 1 line

[1] https://lkml.org/lkml/2019/4/9/5

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

If you did and it's not a good fit, this is something good to mention in the commit log.

I also wonder about performance numbers - any data here?

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

There are no used/available entries here, it's unidirectional (kernel->user).

Quoted 5 lines

If you did and it's not a good fit, this is something good to mention in the commit log.

I also wonder about performance numbers - any data here?

Yes some numbers would be useful. Note however that the improvement is asymptotical, O(#dirtied pages) vs O(#total pages) so it may differ depending on the workload.

Quoted 3 lines · Paolo Bonzini

Quoted 2 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Quoted 5 lines

Quoted 2 lines

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

There are no used/available entries here, it's unidirectional (kernel->user).

Agreed. Vring could be an overkill IMHO (the whole dirty_ring.c is 100+ LOC only).

Quoted 9 lines

Quoted 5 lines

If you did and it's not a good fit, this is something good to mention in the commit log.

I also wonder about performance numbers - any data here?

Yes some numbers would be useful. Note however that the improvement is asymptotical, O(#dirtied pages) vs O(#total pages) so it may differ depending on the workload.

Yes. I plan to give some numbers when start to work on the QEMU series (after this lands). However as Paolo said, those numbers would probably only be with some special case where I know the dirty ring could win. Frankly speaking I don't even know whether we should change the default logging mode when the QEMU work is done - I feel like the old logging interface is still good in many major cases (small vms, or high dirty rates). It could be that we just offer another option when the user could consider to solve specific problems.

Thanks,

Signature · 1 line

Peter Xu

Quoted 7 lines · Peter Xu

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

Quoted 5 lines

Quoted 2 lines

There are no used/available entries here, it's unidirectional (kernel->user).

Agreed. Vring could be an overkill IMHO (the whole dirty_ring.c is 100+ LOC only).

I guess you don't do polling/ event suppression and other tricks that virtio came up with for speed then?

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Peter Xu

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

There are no used/available entries here, it's unidirectional (kernel->user).

Agreed. Vring could be an overkill IMHO (the whole dirty_ring.c is 100+ LOC only).

I guess you don't do polling/ event suppression and other tricks that virtio came up with for speed then?

There are no interrupts either, so no need for event suppression. You have vmexits when the ring gets full (and that needs to be synchronous), but apart from that the migration thread will poll the rings once when it needs to send more pages.

Paolo

Quoted 9 lines · Paolo Bonzini

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Peter Xu

Quoted 6 lines · Paolo Bonzini

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

There are no used/available entries here, it's unidirectional (kernel->user).

Agreed. Vring could be an overkill IMHO (the whole dirty_ring.c is 100+ LOC only).

I guess you don't do polling/ event suppression and other tricks that virtio came up with for speed then?

I looked at the code finally, there's actually available, and fetched is exactly like used. Not saying existing code is a great fit for you as you have an extra slot parameter to pass and it's reversed as compared to vhost, with kernel being the driver and userspace the device (even though vringh might fit, yet needs to be updated to support packed rings though). But sticking to an existing format is a good idea IMHO, or if not I think it's not a bad idea to add some justification.

Quoted 4 lines · Michael S. Tsirkin

Quoted 2 lines

There is no new infrastructure to track the dirty pages---it's just a different way to pass them to userspace.

Did you guys consider using one of the virtio ring formats? Maybe reusing vhost code?

There are no used/available entries here, it's unidirectional (kernel->user).

Quoted 4 lines

If you did and it's not a good fit, this is something good to mention in the commit log.

I also wonder about performance numbers - any data here?

Yes some numbers would be useful. Note however that the improvement is asymptotical, O(#dirtied pages) vs O(#total pages) so it may differ depending on the workload.

Paolo

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.

We defined two new data structures:

struct kvm_dirty_ring;
struct kvm_dirty_ring_indexes;

Firstly, kvm_dirty_ring is defined to represent a ring of dirty pages. When dirty tracking is enabled, we can push dirty gfn onto the ring.

Secondly, kvm_dirty_ring_indexes is defined to represent the user/kernel interface of each ring. Currently it contains two
indexes: (1) avail_index represents where we should push our next PFN (written by kernel), while (2) fetch_index represents where the userspace should fetch the next dirty PFN (written by userspace).

One complete ring is composed by one kvm_dirty_ring plus its corresponding kvm_dirty_ring_indexes.

Currently, we have N+1 rings for each VM of N vcpus:

- for each vcpu, we have 1 per-vcpu dirty ring,
- for each vm, we have 1 per-vm dirty ring

Please refer to the documentation update in this patch for more details.

Note that this patch implements the core logic of dirty ring buffer. It's still disabled for all archs for now. Also, we'll address some of the other issues in follow up patches before it's firstly enabled on x86.

[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 | 109 +++++++++++++++
 arch/x86/kvm/Makefile          |   3 +-
 include/linux/kvm_dirty_ring.h |  67 +++++++++
 include/linux/kvm_host.h       |  33 +++++
 include/linux/kvm_types.h      |   1 +
 include/uapi/linux/kvm.h       |  36 +++++
 virt/kvm/dirty_ring.c          | 156 +++++++++++++++++++++
 virt/kvm/kvm_main.c            | 240 ++++++++++++++++++++++++++++++++-
 8 files changed, 642 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 +6 -0

index 49183add44e7..fa622c9a2eb8 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,

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

@@ -5358,6 +5371,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

@@ -5365,6 +5379,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

@@ -5378,3 +5393,97 @@ 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 and one global
+ring per vm.
+
+One dirty ring has the following two major structures:
+
+struct kvm_dirty_ring {
+	u16 dirty_index;

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

+
+The two indices in the ring buffer are free running counters.
+
+In pseudocode, processing the ring buffer looks like this:
+
+	idx = load-acquire(&ring->fetch_index);
+	while (idx != ring->avail_index) {
+		struct kvm_dirty_gfn *entry;
+		entry = &ring->dirty_gfns[idx & (size - 1)];

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

+
+After the capability is enabled, userspace can mmap the global ring
+buffer (kvm_dirty_gfn[], offset KVM_DIRTY_LOG_PAGE_OFFSET) and the
+indexes (kvm_dirty_ring_indexes, offset 0) from the VM file
+descriptor.  The per-vcpu dirty ring instead is mmapped when the vcpu
+is created, similar to the kvm_run struct (kvm_dirty_ring_indexes
+locates inside kvm_run, while kvm_dirty_gfn[] at offset
+KVM_DIRTY_LOG_PAGE_OFFSET).
+
+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

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 \

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

new file mode 100644
index 000000000000..8335635b7ff7
@@ -0,0 +1,67 @@
+#ifndef KVM_DIRTY_RING_H
+#define KVM_DIRTY_RING_H
+
+/*
+ * struct kvm_dirty_ring is defined in include/uapi/linux/kvm.h.
+ *
+ * dirty_ring:  shared with userspace via mmap. It is the compact list
+ *              that holds the dirty pages.
+ * 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
+ * lock:        protects dirty_ring, only in use if this is the global
+ *              ring
+ *
+ * The number of dirty pages in the ring is calculated by,
+ * dirty_index - reset_index
+ *
+ * kernel increments dirty_ring->indices.avail_index after dirty index
+ * is incremented. When userspace harvests the dirty pages, it increments
+ * dirty_ring->indices.fetch_index up to dirty_ring->indices.avail_index.
+ * When kernel reenables dirty traps for the dirty pages, it increments
+ * reset_index up to dirty_ring->indices.fetch_index.
+ *
+ */
+struct kvm_dirty_ring {
+	u32 dirty_index;
+	u32 reset_index;
+	u32 size;
+	u32 soft_limit;
+	spinlock_t lock;
+	struct kvm_dirty_gfn *dirty_gfns;
+};
+
+u32 kvm_dirty_ring_get_rsvd_entries(void);
+int kvm_dirty_ring_alloc(struct kvm *kvm, struct kvm_dirty_ring *ring);
+
+/*
+ * 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,
+			 struct kvm_dirty_ring_indexes *indexes);
+
+/*
+ * returns 0: successfully pushed
+ *         1: successfully pushed, soft limit reached,
+ *            vcpu should exit to userspace
+ *         -EBUSY: unable to push, dirty ring full.
+ */
+int kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
+			struct kvm_dirty_ring_indexes *indexes,
+			u32 slot, u64 offset, bool lock);
+
+/* for use in vm_operations_struct */
+struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 i);
+
+void kvm_dirty_ring_free(struct kvm_dirty_ring *ring);
+bool kvm_dirty_ring_full(struct kvm_dirty_ring *ring);
+
+#endif

include/linux/kvm_host.h +13 -0

index 498a39462ac1..7b747bc9ff3e 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

@@ -146,6 +147,7 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MMU_RELOAD        (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_PENDING_TIMER     2
 #define KVM_REQ_UNHALT            3
+#define KVM_REQ_DIRTY_RING_FULL   4
 #define KVM_REQUEST_ARCH_BASE     8
 
 #define KVM_ARCH_REQ_FLAGS(nr, flags) ({ \

@@ -321,6 +323,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)

@@ -501,6 +504,10 @@ struct kvm {
 	struct srcu_struct srcu;
 	struct srcu_struct irq_srcu;
 	pid_t userspace_pid;
+	/* Data structure to be exported by mmap(kvm->fd, 0) */
+	struct kvm_vm_run *vm_run;
+	u32 dirty_ring_size;
+	struct kvm_dirty_ring vm_dirty_ring;
 };
 
 #define kvm_err(fmt, ...) \

@@ -832,6 +839,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,

@@ -1411,4 +1420,28 @@ 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.

include/linux/kvm_host.h +1 -0

+
 #endif

include/linux/kvm_types.h +1 -0

index 1c88e69db3d9..d9d03eea145a 100644
@@ -11,6 +11,7 @@ struct kvm_irq_routing_table;
 struct kvm_memory_slot;
 struct kvm_one_reg;
 struct kvm_run;
+struct kvm_vm_run;
 struct kvm_userspace_memory_region;
 struct kvm_vcpu;
 struct kvm_vcpu_init;

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

index e6f17c8e2dba..0b88d76d6215 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,11 @@ struct kvm_hyperv_exit {
 /* Encounter unexpected vm-exit reason */
 #define KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON	4
 
+struct kvm_dirty_ring_indexes {
+	__u32 avail_index; /* set by kernel */
+	__u32 fetch_index; /* set by userspace */
+};
+
 /* for KVM_RUN, returned by mmap(vcpu_fd, offset=0) */
 struct kvm_run {
 	/* in */

@@ -421,6 +427,13 @@ struct kvm_run {
 		struct kvm_sync_regs regs;
 		char padding[SYNC_REGS_SIZE_BYTES];
 	} s;
+
+	struct kvm_dirty_ring_indexes vcpu_ring_indexes;
+};
+
+/* Returned by mmap(kvm->fd, offset=0) */
+struct kvm_vm_run {
+	struct kvm_dirty_ring_indexes vm_ring_indexes;
 };
 
 /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */

@@ -1009,6 +1022,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
 

@@ -1472,6 +1486,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 */

@@ -1622,4 +1639,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 +5 -0

new file mode 100644
index 000000000000..9264891f3c32
@@ -0,0 +1,156 @@
+#include <linux/kvm_host.h>
+#include <linux/kvm.h>
+#include <linux/vmalloc.h>
+#include <linux/kvm_dirty_ring.h>
+