KVM: Dirty ring interface

arch/x86/kvm/x86.c +13 -2

+ *   -errno:        on error
+ *
+ * The caller should always use IS_ERR() to check the return value
+ * before use.  Note, the KVM internal memory slots are guaranteed to
+ * remain valid and unchanged until the VM is destroyed, i.e., the
+ * GPA->HVA translation will not change.  However, the HVA is a user
+ * address, i.e. its accessibility is not guaranteed, and must be
+ * accessed via __copy_{to,from}_user().
+ */
+void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
+				      u32 size)
 {
 	int i, r;
 	unsigned long hva, uninitialized_var(old_npages);

@@ -9800,12 +9825,12 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 
 	/* Called with kvm->slots_lock held.  */
 	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
-		return -EINVAL;
+		return ERR_PTR_USR(-EINVAL);
 
 	slot = id_to_memslot(slots, id);
 	if (size) {
 		if (slot && slot->npages)
-			return -EEXIST;
+			return ERR_PTR_USR(-EEXIST);
 
 		/*
 		 * MAP_SHARED to prevent internal slot pages from being moved

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

 		/*
 		 * Stuff a non-canonical value to catch use-after-delete.  This

@@ -9838,13 +9863,13 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 		m.memory_size = size;
 		r = __kvm_set_memory_region(kvm, &m);
 		if (r < 0)
-			return r;
+			return ERR_PTR_USR(r);
 	}
 
 	if (!size)
 		vm_munmap(hva, old_npages * PAGE_SIZE);
 
-	return 0;
+	return (void __user *)hva;
 }
 EXPORT_SYMBOL_GPL(__x86_set_memory_region);
 

Signature · 1 line

2.24.1

Quoted 8 lines · Peter Xu

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

index 1b6d9ac9533c..faa702c4d37b 100644
@@ -9791,7 +9791,32 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 

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

Quoted 2 lines

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

-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+#define  ERR_PTR_USR(e)  ((void __user *)ERR_PTR(e))

Heh, my first thought when reading the below code was "cool, I didn't know there was ERR_PTR_USR!". This probably should be in include/linux/err.h, or maybe a new arch specific implementation if it's not universally safe.

An alternative, which looks enticing given that proper user variants will be a bit of an explosion, would be to do:

static void *____x86_set_memory_region(...)
{
<actual function>
}

void __user *__x86_set_memory_region(...)

  {
	return (void __user *)____x86_set_memory_region(...);

}

A second alternative would be to return an "unsigned long", i.e. force the one function that actually accesses the hva to do the cast. I think I like this option the best as it would minimize the churn in __x86_set_memory_region(). Callers can use IS_ERR_VALUE() to detect failure.

Quoted 4 lines

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

+/**
+ * __x86_set_memory_region: Setup KVM internal memory slot
+ *

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

Quoted 10 lines

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

+ * @kvm: the kvm pointer to the VM.
+ * @id: the slot ID to setup.
+ * @gpa: the GPA to install the slot (unused when @size == 0).
+ * @size: the size of the slot. Set to zero to uninstall a slot.
+ *
+ * This function helps to setup a KVM internal memory slot.  Specify
+ * @size > 0 to install a new slot, while @size == 0 to uninstall a
+ * slot.  The return code can be one of the following:
+ *
+ *   HVA:           on success (uninstall will return a bogus HVA)

I think it's important to call out that it returns '0' on uninstall, e.g. otherwise it's not clear how a caller can detect failure.

Quoted 28 lines

arch/x86/kvm/x86.c +13 -2

+ *   -errno:        on error
+ *
+ * The caller should always use IS_ERR() to check the return value
+ * before use.  Note, the KVM internal memory slots are guaranteed to
+ * remain valid and unchanged until the VM is destroyed, i.e., the
+ * GPA->HVA translation will not change.  However, the HVA is a user
+ * address, i.e. its accessibility is not guaranteed, and must be
+ * accessed via __copy_{to,from}_user().
+ */
+void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
+				      u32 size)
 {
 	int i, r;
 	unsigned long hva, uninitialized_var(old_npages);

@@ -9800,12 +9825,12 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 
 	/* Called with kvm->slots_lock held.  */
 	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
-		return -EINVAL;
+		return ERR_PTR_USR(-EINVAL);
 
 	slot = id_to_memslot(slots, id);
 	if (size) {
 		if (slot && slot->npages)
-			return -EEXIST;
+			return ERR_PTR_USR(-EEXIST);
 

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9841

Quoted 6 lines

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

 		/*
 		 * MAP_SHARED to prevent internal slot pages from being moved

@@ -9814,10 +9839,10 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 		hva = vm_mmap(NULL, 0, size, PROT_READ | PROT_WRITE,
 			      MAP_SHARED | MAP_ANONYMOUS, 0);
 		if (IS_ERR((void *)hva))

IS_ERR_VALUE() can be used to avoid the double cast.

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9842

Quoted 3 lines

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

-			return PTR_ERR((void *)hva);
+			return (void __user *)hva;

If we still want to go down the route of ERR_PTR_USR, then an ERR_CAST_USR seems in order.

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9845

Quoted 5 lines

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

 	} else {
 		if (!slot || !slot->npages)
-			return 0;
+			return ERR_PTR_USR(0);

"return ERR_PTR_USR(NULL)" or "return NULL" would be more intuitive. Moot point if the return is changed to "unsigned long".

Quoted 9 lines · Sean Christopherson

Quoted 8 lines · Peter Xu

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

index 1b6d9ac9533c..faa702c4d37b 100644
@@ -9791,7 +9791,32 @@ void kvm_arch_sync_events(struct kvm *kvm)
 	kvm_free_pit(kvm);
 }
 

Quoted 6 lines

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

Quoted 2 lines

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

-int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
+#define  ERR_PTR_USR(e)  ((void __user *)ERR_PTR(e))

Heh, my first thought when reading the below code was "cool, I didn't know there was ERR_PTR_USR!". This probably should be in include/linux/err.h, or maybe a new arch specific implementation if it's not universally safe.

Yeah, I just wanted to avoid introducing things in common headers before I'm sure it'll be used in the rest of the world.. We can always replace them with a global definition when it comes.

Quoted 13 lines

An alternative, which looks enticing given that proper user variants will be a bit of an explosion, would be to do:

static void *____x86_set_memory_region(...)
{
<actual function>
}

void __user *__x86_set_memory_region(...)

  {
	return (void __user *)____x86_set_memory_region(...);

}

Quoted 4 lines

A second alternative would be to return an "unsigned long", i.e. force the one function that actually accesses the hva to do the cast. I think I like this option the best as it would minimize the churn in __x86_set_memory_region(). Callers can use IS_ERR_VALUE() to detect failure.

If you won't mind, I would prefer a 2nd opinion (maybe Paolo?) so we can consolidate the idea before I change them... (I would for sure still prefer the current approach for simplicity since after all I don't have strong opionion..)

Quoted 14 lines

Quoted 13 lines

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

+/**
+ * __x86_set_memory_region: Setup KVM internal memory slot
+ *
+ * @kvm: the kvm pointer to the VM.
+ * @id: the slot ID to setup.
+ * @gpa: the GPA to install the slot (unused when @size == 0).
+ * @size: the size of the slot. Set to zero to uninstall a slot.
+ *
+ * This function helps to setup a KVM internal memory slot.  Specify
+ * @size > 0 to install a new slot, while @size == 0 to uninstall a
+ * slot.  The return code can be one of the following:
+ *
+ *   HVA:           on success (uninstall will return a bogus HVA)

Quoted 2 lines

I think it's important to call out that it returns '0' on uninstall, e.g. otherwise it's not clear how a caller can detect failure.

It will "return (0xdeadull << 48)" as you proposed in abbed4fa94f6? :-)

Frankly speaking I always preferred zero but that's just not true any more after above change. This also reminded me that maybe we should also return the same thing at [1] below.

Quoted 27 lines

Quoted 27 lines

arch/x86/kvm/x86.c +13 -2

+ *   -errno:        on error
+ *
+ * The caller should always use IS_ERR() to check the return value
+ * before use.  Note, the KVM internal memory slots are guaranteed to
+ * remain valid and unchanged until the VM is destroyed, i.e., the
+ * GPA->HVA translation will not change.  However, the HVA is a user
+ * address, i.e. its accessibility is not guaranteed, and must be
+ * accessed via __copy_{to,from}_user().
+ */
+void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa,
+				      u32 size)
 {
 	int i, r;
 	unsigned long hva, uninitialized_var(old_npages);

@@ -9800,12 +9825,12 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 
 	/* Called with kvm->slots_lock held.  */
 	if (WARN_ON(id >= KVM_MEM_SLOTS_NUM))
-		return -EINVAL;
+		return ERR_PTR_USR(-EINVAL);
 
 	slot = id_to_memslot(slots, id);
 	if (size) {
 		if (slot && slot->npages)
-			return -EEXIST;
+			return ERR_PTR_USR(-EEXIST);
 

Quoted 8 lines

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9841

Quoted 6 lines

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

 		/*
 		 * MAP_SHARED to prevent internal slot pages from being moved

@@ -9814,10 +9839,10 @@ int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size)
 		hva = vm_mmap(NULL, 0, size, PROT_READ | PROT_WRITE,
 			      MAP_SHARED | MAP_ANONYMOUS, 0);
 		if (IS_ERR((void *)hva))

IS_ERR_VALUE() can be used to avoid the double cast.

Agreed. But it's a context cleanup, so I normally will keep it as is (or use a standalone patch).

Quoted 6 lines

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9842

Quoted 3 lines

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

-			return PTR_ERR((void *)hva);
+			return (void __user *)hva;

If we still want to go down the route of ERR_PTR_USR, then an ERR_CAST_USR seems in order.

Sure. But I'll still keep it kvm-only if you won't mind...

Quoted 5 lines

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9845

Quoted 5 lines

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

 	} else {
 		if (!slot || !slot->npages)
-			return 0;
+			return ERR_PTR_USR(0);

[1]

Quoted 3 lines

"return ERR_PTR_USR(NULL)" or "return NULL" would be more intuitive. Moot point if the return is changed to "unsigned long".

ERR_PTR_USR() takes a "long". I can use ERR_CAST_USR(NULL) if you prefer me to explicitly use NULL.

Thanks,

Signature · 1 line

Peter Xu

Quoted 5 lines

It will "return (0xdeadull << 48)" as you proposed in abbed4fa94f6? :-)

Frankly speaking I always preferred zero but that's just not true any more after above change. This also reminded me that maybe we should also return the same thing at [1] below.

Ah, I was looking at this code:

	if (!slot || !slot->npages)
		return 0;

That means deletion returns different success values for "deletion was a nop" and "deletion was successful". The nop path should probably return (or fill in) "(unsigned long)(0xdeadull << 48)" as well.

Quoted 7 lines

Quoted 5 lines

arch/x86/kvm/x86.c int __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size) · line 9845

Quoted 5 lines

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

 	} else {
 		if (!slot || !slot->npages)
-			return 0;
+			return ERR_PTR_USR(0);

[1]

We don't clear the dirty bitmap before because KVM_GET_DIRTY_LOG will clear it for us before copying the dirty log onto it. However we'd still better to clear it explicitly instead of assuming the kernel will always do it for us.

More importantly, in the upcoming dirty ring tests we'll start to fetch dirty pages from a ring buffer, so no one is going to clear the dirty bitmap for us.

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

---
 tools/testing/selftests/kvm/dirty_log_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/testing/selftests/kvm/dirty_log_test.c +1 -1

index 752ec158ac59..6a8275a22861 100644
@@ -195,7 +195,7 @@ static void vm_dirty_log_verify(enum vm_guest_mode mode, unsigned long *bmap)
 				    page);
 		}
 
-		if (test_bit_le(page, bmap)) {
+		if (test_and_clear_bit_le(page, bmap)) {
 			host_dirty_count++;
 			/*
 			 * If the bit is set, the value written onto

Signature · 1 line

2.24.1

Provide a hook for the checks after vcpu_run() completes. Preparation for the dirty ring test because we'll need to take care of another exit reason.

Since at it, drop the pages_count because after all we have a better summary right now with statistics, and clean it up a bit.

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

---
 tools/testing/selftests/kvm/dirty_log_test.c | 36 +++++++++++++-------
 1 file changed, 24 insertions(+), 12 deletions(-)

tools/testing/selftests/kvm/dirty_log_test.c +24 -12

index 139ccb550618..a2160946bcf5 100644
@@ -178,6 +178,15 @@ static void clear_log_collect_dirty_pages(struct kvm_vm *vm, int slot,
 	kvm_vm_clear_dirty_log(vm, slot, bitmap, 0, num_pages);
 }
 
+static void default_after_vcpu_run(struct kvm_vm *vm)
+{
+	struct kvm_run *run = vcpu_state(vm, VCPU_ID);
+
+	TEST_ASSERT(get_ucall(vm, VCPU_ID, NULL) == UCALL_SYNC,
+		    "Invalid guest sync status: exit_reason=%s\n",
+		    exit_reason_str(run->exit_reason));
+}
+
 struct log_mode {
 	const char *name;
 	/* Return true if this mode is supported, otherwise false */

@@ -187,16 +196,20 @@ struct log_mode {
 	/* Hook to collect the dirty pages into the bitmap provided */
 	void (*collect_dirty_pages) (struct kvm_vm *vm, int slot,
 				     void *bitmap, uint32_t num_pages);
+	/* Hook to call when after each vcpu run */
+	void (*after_vcpu_run)(struct kvm_vm *vm);
 } log_modes[LOG_MODE_NUM] = {
 	{
 		.name = "dirty-log",
 		.collect_dirty_pages = dirty_log_collect_dirty_pages,
+		.after_vcpu_run = default_after_vcpu_run,
 	},
 	{
 		.name = "clear-log",
 		.supported = clear_log_supported,
 		.create_vm_done = clear_log_create_vm_done,
 		.collect_dirty_pages = clear_log_collect_dirty_pages,
+		.after_vcpu_run = default_after_vcpu_run,
 	},
 };
 

@@ -247,6 +260,14 @@ static void log_mode_collect_dirty_pages(struct kvm_vm *vm, int slot,
 	mode->collect_dirty_pages(vm, slot, bitmap, num_pages);
 }
 
+static void log_mode_after_vcpu_run(struct kvm_vm *vm)
+{
+	struct log_mode *mode = &log_modes[host_log_mode];
+
+	if (mode->after_vcpu_run)
+		mode->after_vcpu_run(vm);
+}
+
 static void generate_random_array(uint64_t *guest_array, uint64_t size)
 {
 	uint64_t i;

@@ -261,25 +282,16 @@ static void *vcpu_worker(void *data)
 	struct kvm_vm *vm = data;
 	uint64_t *guest_array;
 	uint64_t pages_count = 0;
-	struct kvm_run *run;
-
-	run = vcpu_state(vm, VCPU_ID);
 
 	guest_array = addr_gva2hva(vm, (vm_vaddr_t)random_array);
-	generate_random_array(guest_array, TEST_PAGES_PER_LOOP);
 
 	while (!READ_ONCE(host_quit)) {
+		generate_random_array(guest_array, TEST_PAGES_PER_LOOP);
+		pages_count += TEST_PAGES_PER_LOOP;
 		/* Let the guest dirty the random pages */
 		ret = _vcpu_run(vm, VCPU_ID);
 		TEST_ASSERT(ret == 0, "vcpu_run failed: %d\n", ret);
-		if (get_ucall(vm, VCPU_ID, NULL) == UCALL_SYNC) {
-			pages_count += TEST_PAGES_PER_LOOP;
-			generate_random_array(guest_array, TEST_PAGES_PER_LOOP);
-		} else {
-			TEST_FAIL("Invalid guest sync status: "
-				  "exit_reason=%s\n",
-				  exit_reason_str(run->exit_reason));
-		}
+		log_mode_after_vcpu_run(vm);
 	}
 
 	pr_info("Dirtied %"PRIu64" pages\n", pages_count);

Signature · 1 line

2.24.1

Quoted 4 lines · Peter Xu

Provide a hook for the checks after vcpu_run() completes. Preparation for the dirty ring test because we'll need to take care of another exit reason.

Quoted 2 lines

Since at it, drop the pages_count because after all we have a better summary right now with statistics, and clean it up a bit.

I don't see what you mean by "drop the pages_count", because it's still there. But otherwise

Reviewed-by Andrew Jones <[email protected]>

Previously the dirty ring test was working in synchronous way, because only with a vmexit (with that it was the ring full event) we'll know the hardware dirty bits will be flushed to the dirty ring.

With this patch we first introduced the vcpu kick mechanism by using SIGUSR1, meanwhile we can have a guarantee of vmexit and also the flushing of hardware dirty bits. With all these, we can keep the vcpu dirty work asynchronous of the whole collection procedure now. Still, we need to be very careful that we can only do it async if the vcpu is not reaching soft limit (no KVM_EXIT_DIRTY_RING_FULL). Otherwise we must collect the dirty bits before continuing the vcpu.

Further increase the dirty ring size to current maximum to make sure we torture more on the no-ring-full case, which should be the major scenario when the hypervisors like QEMU would like to use this feature.

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

---
 tools/testing/selftests/kvm/dirty_log_test.c  | 126 +++++++++++++-----
 .../testing/selftests/kvm/include/kvm_util.h  |   1 +
 tools/testing/selftests/kvm/lib/kvm_util.c    |   9 ++
 3 files changed, 106 insertions(+), 30 deletions(-)

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead of (slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind kvm_dirty_gfn with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM memslots is a restriction that needs synchronization to memslot changes, which further needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty bits correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even if we fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively) important scenario to think about for dirty logging comparing to memory additions & movings. Because memory addition will always have no initial dirty page, and
we don't really move RAM a lot (or do we ever?!) for a general VM use case.

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

- When the memslot is going to be removed but quickly added back with
changed
size, then we need to keep those dirty bits because it's just a commmon
way
to e.g. punch an MMIO hole in an existing RAM region (here I'd confess I feel like using "slot_id" to identify memslot is really unfriendly syscall design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a common guest for either system reboots or device hotplugs, etc.).

The real scenario we want to cover for dirty tracking is the 2nd one.

If we can track dirty using raw GPA, the 2nd scenario is solved itself. Because we know we'll add those memslots back (though it might be with a different slot ID), then the GPA value will still make sense, which means we should be able to avoid any kind of synchronization for things like memory removals, as long as the userspace is aware of that.

With that, when we fetch the dirty bits, we lookup the memslot dynamically, drop bits if the memslot does not exist on that address (e.g., permanent removals), and use whatever memslot is there for that guest physical address.
Though we for sure still need to handle memory move, that the userspace needs
to still take care of dirty bit flushing and sync for a memory move, however that's merely not happening so nothing to take care about either.

Does this makes sense? Comments greatly welcomed..

Thanks,

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08361.html

--
Peter Xu

Quoted 40 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead of (slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind kvm_dirty_gfn with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM memslots is a restriction that needs synchronization to memslot changes, which further needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty bits correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even if we fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively) important scenario to think about for dirty logging comparing to memory additions & movings. Because memory addition will always have no initial dirty page, and
we don't really move RAM a lot (or do we ever?!) for a general VM use case.

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

Quoted 9 lines

- When the memslot is going to be removed but quickly added back with
changed
size, then we need to keep those dirty bits because it's just a commmon
way
to e.g. punch an MMIO hole in an existing RAM region (here I'd confess I feel like using "slot_id" to identify memslot is really unfriendly syscall design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a common guest for either system reboots or device hotplugs, etc.).

why would device hotplug punch a hole in an existing RAM region?

Quoted 9 lines

The real scenario we want to cover for dirty tracking is the 2nd one.

If we can track dirty using raw GPA, the 2nd scenario is solved itself. Because we know we'll add those memslots back (though it might be with a different slot ID), then the GPA value will still make sense, which means we should be able to avoid any kind of synchronization for things like memory removals, as long as the userspace is aware of that.

A curious question. What about the backing storage of the affected GPA is changed after adding back? Is recorded dirty info for previous backing storage still making sense for the newer one?

Thanks
Kevin

Quoted 18 lines

With that, when we fetch the dirty bits, we lookup the memslot dynamically, drop bits if the memslot does not exist on that address (e.g., permanent removals), and use whatever memslot is there for that guest physical address.
Though we for sure still need to handle memory move, that the userspace needs
to still take care of dirty bit flushing and sync for a memory move, however that's merely not happening so nothing to take care about either.

Does this makes sense? Comments greatly welcomed..

Thanks,

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08361.html

--
Peter Xu

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 11:23 PM

Quoted 8 lines · Kevin

Quoted 8 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead of (slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind

kvm_dirty_gfn

Quoted 5 lines

Quoted 5 lines

with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM memslots is

a

Quoted 9 lines

Quoted 9 lines

restriction that needs synchronization to memslot changes, which further needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty bits correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even if

we

Quoted 3 lines

Quoted 3 lines

fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively)

important

Quoted 1 line

Quoted 1 line

scenario to think about for dirty logging comparing to memory additions

&

Quoted 3 lines

Quoted 3 lines

movings. Because memory addition will always have no initial dirty page, and
we don't really move RAM a lot (or do we ever?!) for a general VM use

case.

Quoted 11 lines

Quoted 11 lines

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

Quoted 5 lines

Quoted 5 lines

- When the memslot is going to be removed but quickly added back with
changed
size, then we need to keep those dirty bits because it's just a commmon
way
to e.g. punch an MMIO hole in an existing RAM region (here I'd confess

I

Quoted 3 lines

Quoted 3 lines

feel like using "slot_id" to identify memslot is really unfriendly syscall design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a

common

Quoted 3 lines

Quoted 1 line

guest for either system reboots or device hotplugs, etc.).

why would device hotplug punch a hole in an existing RAM region?

I thought it could happen because I used to trace the KVM ioctls and see the memslot changes during driver loading. But later when I tried to hotplug a

device I do see that it won't... The new MMIO regions are added only into 0xfe000000 for a virtio-net:

00000000fe000000-00000000fe000fff (prio 0, i/o): virtio-pci-common 00000000fe001000-00000000fe001fff (prio 0, i/o): virtio-pci-isr 00000000fe002000-00000000fe002fff (prio 0, i/o): virtio-pci-device 00000000fe003000-00000000fe003fff (prio 0, i/o): virtio-pci-notify 00000000fe840000-00000000fe84002f (prio 0, i/o): msix-table 00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba

Does it mean that device plugging is guaranteed to not trigger RAM changes?

I
am really curious about what cases we need to consider in which we need to keep
the dirty bits for a memory removal, and if system reset is the only case, then it could be even easier (because we might be able to avoid the sync in memory
removal but do that once in a sys reset hook)...

Quoted 7 lines

Quoted 7 lines

The real scenario we want to cover for dirty tracking is the 2nd one.

If we can track dirty using raw GPA, the 2nd scenario is solved itself. Because we know we'll add those memslots back (though it might be with

a

Quoted 1 line

Quoted 1 line

different slot ID), then the GPA value will still make sense, which means

we

Quoted 1 line

Quoted 1 line

should be able to avoid any kind of synchronization for things like

memory

Quoted 5 lines

Quoted 1 line

removals, as long as the userspace is aware of that.

A curious question. What about the backing storage of the affected GPA is changed after adding back? Is recorded dirty info for previous backing storage still making sense for the newer one?

It's the case of a permanent removal, plus another addition iiuc. Then the worst case is we get some extra dirty bits set on that new memory region, but
IMHO that's benigh (we'll migrate some extra pages even they could be zero pages).

Thanks,

Quoted 6 lines

Thanks
Kevin

Quoted 2 lines

With that, when we fetch the dirty bits, we lookup the memslot

dynamically,

Quoted 4 lines

Quoted 4 lines

drop bits if the memslot does not exist on that address (e.g., permanent removals), and use whatever memslot is there for that guest physical address.
Though we for sure still need to handle memory move, that the

userspace

Quoted 2 lines

Quoted 2 lines

needs
to still take care of dirty bit flushing and sync for a memory move,

however

Quoted 7 lines

Quoted 7 lines

that's merely not happening so nothing to take care about either.

Does this makes sense? Comments greatly welcomed..

Thanks,

[1] https://lists.gnu.org/archive/html/qemu-devel/2020-

03/msg08361.html

Quoted 4 lines

Quoted 3 lines

--
Peter Xu

--
Peter Xu

Quoted 60 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 11:23 PM

Quoted 8 lines · Kevin

Quoted 8 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead of (slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind

kvm_dirty_gfn

Quoted 5 lines

Quoted 5 lines

with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM memslots is

a

Quoted 9 lines

Quoted 9 lines

restriction that needs synchronization to memslot changes, which further needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty bits correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even if

we

Quoted 3 lines

Quoted 3 lines

fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively)

important

Quoted 1 line

Quoted 1 line

scenario to think about for dirty logging comparing to memory additions

&

Quoted 3 lines

Quoted 3 lines

movings. Because memory addition will always have no initial dirty page, and
we don't really move RAM a lot (or do we ever?!) for a general VM use

case.

Quoted 11 lines

Quoted 11 lines

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

Quoted 5 lines

Quoted 5 lines

- When the memslot is going to be removed but quickly added back with
changed
size, then we need to keep those dirty bits because it's just a commmon
way
to e.g. punch an MMIO hole in an existing RAM region (here I'd confess

I

Quoted 3 lines

Quoted 3 lines

feel like using "slot_id" to identify memslot is really unfriendly syscall design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a

common

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

guest for either system reboots or device hotplugs, etc.).

why would device hotplug punch a hole in an existing RAM region?

I thought it could happen because I used to trace the KVM ioctls and see the memslot changes during driver loading. But later when I tried to hotplug a

Is there more detail why driver loading may lead to memslot changes?

Quoted 11 lines

device I do see that it won't... The new MMIO regions are added only into 0xfe000000 for a virtio-net:

00000000fe000000-00000000fe000fff (prio 0, i/o): virtio-pci-common 00000000fe001000-00000000fe001fff (prio 0, i/o): virtio-pci-isr 00000000fe002000-00000000fe002fff (prio 0, i/o): virtio-pci-device 00000000fe003000-00000000fe003fff (prio 0, i/o): virtio-pci-notify 00000000fe840000-00000000fe84002f (prio 0, i/o): msix-table 00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba

Quoted 1 line

Does it mean that device plugging is guaranteed to not trigger RAM changes?

I'd think so. Otherwise from guest p.o.v any device hotplug implies doing a memory hot-unplug first then it's a bad design.

Quoted 8 lines

I
am really curious about what cases we need to consider in which we need to keep
the dirty bits for a memory removal, and if system reset is the only case, then it could be even easier (because we might be able to avoid the sync in memory
removal but do that once in a sys reset hook)...

Possibly memory hot-unplug, as allowed by recent virtio-mem?

btw VFIO faces a similar problem when unmapping a DMA range (e.g. when vIOMMU is enabled) in dirty log phase. There could be some dirty bits which are not retrieved when unmapping happens. VFIO chooses to return the dirty bits in a buffer passed in the unmapping parameters. Can memslot interface do similar thing by allowing the userspace to specify a buffer pointer to hold whatever dirty pages recorded for the slot that is being removed?

From: Peter Xu
Sent: Friday, April 24, 2020 10:20 PM

Quoted 10 lines · Kevin

Quoted 10 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 11:23 PM

Quoted 6 lines · Kevin

Quoted 6 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead

of

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

(slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind

kvm_dirty_gfn

Quoted 5 lines

Quoted 5 lines

with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM

memslots is

Quoted 2 lines

Quoted 2 lines

a

Quoted 1 line

Quoted 1 line

restriction that needs synchronization to memslot changes, which

further

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty

bits

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even

if

Quoted 6 lines

Quoted 6 lines

we

Quoted 3 lines

Quoted 3 lines

fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively)

important

Quoted 1 line

Quoted 1 line

scenario to think about for dirty logging comparing to memory

additions

Quoted 2 lines

Quoted 2 lines

&

Quoted 1 line

Quoted 1 line

movings. Because memory addition will always have no initial dirty

page,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

and
we don't really move RAM a lot (or do we ever?!) for a general VM

use

Quoted 6 lines

Quoted 6 lines

case.

Quoted 5 lines

Quoted 5 lines

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

- When the memslot is going to be removed but quickly added back

with

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

changed
size, then we need to keep those dirty bits because it's just a

commmon

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

way
to e.g. punch an MMIO hole in an existing RAM region (here I'd

confess

Quoted 2 lines

Quoted 2 lines

I

Quoted 1 line

Quoted 1 line

feel like using "slot_id" to identify memslot is really unfriendly

syscall

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a

common

Quoted 3 lines

Quoted 1 line

guest for either system reboots or device hotplugs, etc.).

why would device hotplug punch a hole in an existing RAM region?

I thought it could happen because I used to trace the KVM ioctls and see

the

Quoted 1 line

Quoted 1 line

memslot changes during driver loading. But later when I tried to hotplug

a

Quoted 2 lines

Is there more detail why driver loading may lead to memslot changes?

E.g., I can observe these after Linux loads and before the prompt, which is a simplest VM with default devices on:

[email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#9 flags=0x1 gpa=0xa0000 size=0x10000 ua=0x7fadf6800000 ret=0

After a careful look, I noticed it's only the VGA device mostly turning slot 3 off & on. Frankly speaking I don't know why it happens to do so.

Quoted 2 lines

Quoted 2 lines

device I do see that it won't... The new MMIO regions are added only

into

Quoted 10 lines

Quoted 10 lines

0xfe000000 for a virtio-net:

00000000fe000000-00000000fe000fff (prio 0, i/o): virtio-pci-common 00000000fe001000-00000000fe001fff (prio 0, i/o): virtio-pci-isr 00000000fe002000-00000000fe002fff (prio 0, i/o): virtio-pci-device 00000000fe003000-00000000fe003fff (prio 0, i/o): virtio-pci-notify 00000000fe840000-00000000fe84002f (prio 0, i/o): msix-table 00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba

Does it mean that device plugging is guaranteed to not trigger RAM

changes?

Quoted 3 lines

I'd think so. Otherwise from guest p.o.v any device hotplug implies doing a memory hot-unplug first then it's a bad design.

Right that's what I was confused about. Then maybe you're right. :)

Quoted 3 lines

Quoted 3 lines

I
am really curious about what cases we need to consider in which we

need to

Quoted 2 lines

Quoted 2 lines

keep
the dirty bits for a memory removal, and if system reset is the only case,

then

Quoted 4 lines

Quoted 3 lines

it could be even easier (because we might be able to avoid the sync in memory
removal but do that once in a sys reset hook)...

Quoted 1 line

Possibly memory hot-unplug, as allowed by recent virtio-mem?

That should belong to the case where dirty bits do not matter at all after the removal, right? I would be mostly curious about when we (1) remove a memory
slot, and at the meantime (2) we still care about the dirty bits of that slot.

I'll see whether I can remove the dirty bit sync in kvm_set_phys_mem(), which I
think is really nasty.

Quoted 2 lines

btw VFIO faces a similar problem when unmapping a DMA range (e.g.

when

Quoted 1 line

vIOMMU is enabled) in dirty log phase. There could be some dirty bits

which are

Quoted 2 lines

not retrieved when unmapping happens. VFIO chooses to return the dirty bits in a buffer passed in the unmapping parameters. Can memslot

interface

Quoted 1 line

do similar thing by allowing the userspace to specify a buffer pointer to

hold

Quoted 1 line

whatever dirty pages recorded for the slot that is being removed?

Yes I think we can, but may not be necessary. Actually IMHO CPU access to pages are slightly different to device DMAs in that we can do these sequence to
collect the dirty bits of a slot safely:

- mark slot as READONLY
- KVM_GET_DIRTY_LOG on the slot
- finally remove the slot

I guess VFIO cannot do that because there's no way to really "mark the region
as read-only" for a device because DMA could happen and DMA would fail then
when writting to a readonly slot.

On the KVM/CPU side, after we mark the slot as READONLY then the CPU writes
will page fault and fallback to the QEMU userspace, then QEMU will take care of
the writes (so those writes could be slow but still working) then even if we mark it READONLY it won't fail the writes but just fallback to QEMU.

Btw, since we're discussing the VFIO dirty logging across memory removal... the
unmapping DMA range you're talking about needs to be added back later, right?

Then what if the device does DMA after the removal but before it's added back?

I think this is a more general question not for dirty logging but also for when dirty logging is not enabled - I never understand how this could be fixed with existing facilities.

Quoted 183 lines

From: Peter Xu
Sent: Friday, April 24, 2020 10:20 PM

Quoted 10 lines · Kevin

Quoted 10 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 11:23 PM

Quoted 6 lines · Kevin

Quoted 6 lines

From: Peter Xu <[email protected]>
Sent: Thursday, April 23, 2020 2:52 AM

Hi,

TL;DR: I'm thinking whether we should record pure GPA/GFN instead

of

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

(slot_id,
slot_offset) tuple for dirty pages in kvm dirty ring to unbind

kvm_dirty_gfn

Quoted 5 lines

Quoted 5 lines

with memslots.

(A slightly longer version starts...)

The problem is that binding dirty tracking operations to KVM

memslots is

Quoted 2 lines

Quoted 2 lines

a

Quoted 1 line

Quoted 1 line

restriction that needs synchronization to memslot changes, which

further

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

needs
synchronization across all the vcpus because they're the consumers of memslots.
E.g., when we remove a memory slot, we need to flush all the dirty

bits

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

correctly before we do the removal of the memslot. That's actually an known
defect for QEMU/KVM [1] (I bet it could be a defect for many other hypervisors...) right now with current dirty logging. Meanwhile, even

if

Quoted 6 lines

Quoted 6 lines

we

Quoted 3 lines

Quoted 3 lines

fix it, that procedure is not scale at all, and error prone to dead locks.

Here memory removal is really an (still corner-cased but relatively)

important

Quoted 1 line

Quoted 1 line

scenario to think about for dirty logging comparing to memory

additions

Quoted 2 lines

Quoted 2 lines

&

Quoted 1 line

Quoted 1 line

movings. Because memory addition will always have no initial dirty

page,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

and
we don't really move RAM a lot (or do we ever?!) for a general VM

use

Quoted 6 lines

Quoted 6 lines

case.

Quoted 5 lines

Quoted 5 lines

Then I went a step back to think about why we need these dirty bit information
after all if the memslot is going to be removed?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

There're two cases:

- When the memslot is going to be removed forever, then the dirty
information
is indeed meaningless and can be dropped, and,

- When the memslot is going to be removed but quickly added back

with

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

changed
size, then we need to keep those dirty bits because it's just a

commmon

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

way
to e.g. punch an MMIO hole in an existing RAM region (here I'd

confess

Quoted 2 lines

Quoted 2 lines

I

Quoted 1 line

Quoted 1 line

feel like using "slot_id" to identify memslot is really unfriendly

syscall

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

design for things like "hole punchings" in the RAM address space... However such "punch hold" operation is really needed even for a

common

Quoted 3 lines

Quoted 1 line

guest for either system reboots or device hotplugs, etc.).

why would device hotplug punch a hole in an existing RAM region?

I thought it could happen because I used to trace the KVM ioctls and see

the

Quoted 1 line

Quoted 1 line

memslot changes during driver loading. But later when I tried to hotplug

a

Quoted 2 lines

Is there more detail why driver loading may lead to memslot changes?

E.g., I can observe these after Linux loads and before the prompt, which is a simplest VM with default devices on:

[email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x0 gpa=0xfd000000 size=0x0 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#3 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#65539 flags=0x1 gpa=0xfd000000 size=0x1000000 ua=0x7fadf6800000 ret=0 [email protected]:kvm_set_user_memory Slot#9 flags=0x1 gpa=0xa0000 size=0x10000 ua=0x7fadf6800000 ret=0

After a careful look, I noticed it's only the VGA device mostly turning slot 3 off & on. Frankly speaking I don't know why it happens to do so.

Quoted 2 lines

Quoted 2 lines

device I do see that it won't... The new MMIO regions are added only

into

Quoted 10 lines

Quoted 10 lines

0xfe000000 for a virtio-net:

00000000fe000000-00000000fe000fff (prio 0, i/o): virtio-pci-common 00000000fe001000-00000000fe001fff (prio 0, i/o): virtio-pci-isr 00000000fe002000-00000000fe002fff (prio 0, i/o): virtio-pci-device 00000000fe003000-00000000fe003fff (prio 0, i/o): virtio-pci-notify 00000000fe840000-00000000fe84002f (prio 0, i/o): msix-table 00000000fe840800-00000000fe840807 (prio 0, i/o): msix-pba

Does it mean that device plugging is guaranteed to not trigger RAM

changes?

Quoted 3 lines

I'd think so. Otherwise from guest p.o.v any device hotplug implies doing a memory hot-unplug first then it's a bad design.

Right that's what I was confused about. Then maybe you're right. :)

Quoted 3 lines

Quoted 3 lines

I
am really curious about what cases we need to consider in which we

need to

Quoted 2 lines

Quoted 2 lines

keep
the dirty bits for a memory removal, and if system reset is the only case,

then

Quoted 4 lines

Quoted 3 lines

it could be even easier (because we might be able to avoid the sync in memory
removal but do that once in a sys reset hook)...

Quoted 6 lines

Quoted 1 line

Possibly memory hot-unplug, as allowed by recent virtio-mem?

That should belong to the case where dirty bits do not matter at all after the removal, right? I would be mostly curious about when we (1) remove a memory
slot, and at the meantime (2) we still care about the dirty bits of that slot.

I remember one case. PCIe spec defines a resizable BAR capability, allowing hardware to communicate supported resource sizes and software to set an optimal size back to hardware. If such a capability is presented to guest and the BAR is backed by memory, it's possible to observe a removal-and- add-back scenario. However in such case, the spec requires the software to clear memory space enable bit in command register before changing the BAR size. I suppose such thing should happen at boot phase where dirty bits related to old BAR size don't matter.

Quoted 33 lines

I'll see whether I can remove the dirty bit sync in kvm_set_phys_mem(), which I
think is really nasty.

Quoted 2 lines

btw VFIO faces a similar problem when unmapping a DMA range (e.g.

when

Quoted 1 line

vIOMMU is enabled) in dirty log phase. There could be some dirty bits

which are

Quoted 2 lines

not retrieved when unmapping happens. VFIO chooses to return the dirty bits in a buffer passed in the unmapping parameters. Can memslot

interface

Quoted 1 line

do similar thing by allowing the userspace to specify a buffer pointer to

hold

Quoted 1 line

whatever dirty pages recorded for the slot that is being removed?

Yes I think we can, but may not be necessary. Actually IMHO CPU access to pages are slightly different to device DMAs in that we can do these sequence to
collect the dirty bits of a slot safely:

- mark slot as READONLY
- KVM_GET_DIRTY_LOG on the slot
- finally remove the slot

I guess VFIO cannot do that because there's no way to really "mark the region
as read-only" for a device because DMA could happen and DMA would fail then
when writting to a readonly slot.

Quoted 6 lines

On the KVM/CPU side, after we mark the slot as READONLY then the CPU writes
will page fault and fallback to the QEMU userspace, then QEMU will take care of
the writes (so those writes could be slow but still working) then even if we mark it READONLY it won't fail the writes but just fallback to QEMU.

Yes, you are right.

Quoted 1 line

Quoted 4 lines

Btw, since we're discussing the VFIO dirty logging across memory removal... the
unmapping DMA range you're talking about needs to be added back later, right?

pure memory removal doesn't need add-back. Or are you specifically referring to removal-and-add-back case?

Quoted 3 lines

Then what if the device does DMA after the removal but before it's added back?

then just got IOMMU page fault, but I guess that I didn't get your real question...

Quoted 4 lines

I think this is a more general question not for dirty logging but also for when dirty logging is not enabled - I never understand how this could be fixed with existing facilities.

Thanks,
Kevin