[PATCH v4 0/6] KVM: arm64: Enable ring-based dirty memory tracking

[PATCH v4 0/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

This series enables the ring-based dirty memory tracking for ARM64.
The feature has been available and enabled on x86 for a while. It
is beneficial when the number of dirty pages is small in a checkpointing
system or live migration scenario. More details can be found from
fb04a1eddb1a ("KVM: X86: Implement ring-based dirty memory tracking").

This series is applied on top of Marc's v2 series [0], fixing dirty-ring
ordering issue.

[0] https://lore.kernel.org/kvmarm/20220926145120.27974-1-maz@kernel.org

v3: https://lore.kernel.org/r/20220922003214.276736-1-gshan@redhat.com
v2: https://lore.kernel.org/lkml/YyiV%2Fl7O23aw5aaO@xz-m1.local/T/
v1: https://lore.kernel.org/lkml/20220819005601.198436-1-gshan@redhat.com

Testing
=======
(1) kvm/selftests/dirty_log_test
(2) Live migration by QEMU

Changelog
=========
v4:
  * Commit log improvement                                     (Marc)
  * Add helper kvm_dirty_ring_check_request()                  (Marc)
  * Drop ifdef for kvm_cpu_dirty_log_size()                    (Marc)
v3:
  * Check KVM_REQ_RING_SOFT_RULL inside kvm_request_pending()  (Peter)
  * Move declaration of kvm_cpu_dirty_log_size()               (test-robot)
v2:
  * Introduce KVM_REQ_RING_SOFT_FULL                           (Marc)
  * Changelog improvement                                      (Marc)
  * Fix dirty_log_test without knowing host page size          (Drew)

Gavin Shan (6):
  KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL
  KVM: x86: Move declaration of kvm_cpu_dirty_log_size() to
    kvm_dirty_ring.h
  KVM: arm64: Enable ring-based dirty memory tracking
  KVM: selftests: Use host page size to map ring buffer in
    dirty_log_test
  KVM: selftests: Clear dirty ring states between two modes in
    dirty_log_test
  KVM: selftests: Automate choosing dirty ring size in dirty_log_test

 Documentation/virt/kvm/api.rst               |  2 +-
 arch/arm64/include/uapi/asm/kvm.h            |  1 +
 arch/arm64/kvm/Kconfig                       |  1 +
 arch/arm64/kvm/arm.c                         |  3 ++
 arch/x86/include/asm/kvm_host.h              |  2 -
 arch/x86/kvm/x86.c                           | 15 +++---
 include/linux/kvm_dirty_ring.h               | 14 +++---
 include/linux/kvm_host.h                     |  1 +
 tools/testing/selftests/kvm/dirty_log_test.c | 53 ++++++++++++++------
 tools/testing/selftests/kvm/lib/kvm_util.c   |  2 +-
 virt/kvm/dirty_ring.c                        | 19 ++++++-
 11 files changed, 79 insertions(+), 34 deletions(-)

-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 1/6] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Gavin Shan]

This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty
ring of the specific VCPU becomes softly full in kvm_dirty_ring_push().
The VCPU is enforced to exit when the request is raised and its
dirty ring is softly full on its entrance.

The event is checked and handled in the newly introduced helper
kvm_dirty_ring_check_request(). With this, kvm_dirty_ring_soft_full()
becomes a private function.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/x86/kvm/x86.c             | 15 ++++++---------
 include/linux/kvm_dirty_ring.h | 13 +++++++------
 include/linux/kvm_host.h       |  1 +
 virt/kvm/dirty_ring.c          | 19 ++++++++++++++++++-
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b0c47b41c264..0dd0d32073e7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10260,16 +10260,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	bool req_immediate_exit = false;
 
-	/* Forbid vmenter if vcpu dirty ring is soft-full */
-	if (unlikely(vcpu->kvm->dirty_ring_size &&
-		     kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
-		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
-		trace_kvm_dirty_ring_exit(vcpu);
-		r = 0;
-		goto out;
-	}
-
 	if (kvm_request_pending(vcpu)) {
+		/* Forbid vmenter if vcpu dirty ring is soft-full */
+		if (kvm_dirty_ring_check_request(vcpu)) {
+			r = 0;
+			goto out;
+		}
+
 		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
 			r = -EIO;
 			goto out;
diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
index 906f899813dc..b188bfcf3a09 100644
--- a/include/linux/kvm_dirty_ring.h
+++ b/include/linux/kvm_dirty_ring.h
@@ -54,6 +54,11 @@ static inline void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
 {
 }
 
+static inline bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
+{
+	return false;
+}
+
 static inline struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring,
 						   u32 offset)
 {
@@ -64,11 +69,6 @@ static inline void kvm_dirty_ring_free(struct kvm_dirty_ring *ring)
 {
 }
 
-static inline bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring)
-{
-	return true;
-}
-
 #else /* CONFIG_HAVE_KVM_DIRTY_RING */
 
 u32 kvm_dirty_ring_get_rsvd_entries(void);
@@ -86,11 +86,12 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring);
  */
 void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset);
 
+bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu);
+
 /* for use in vm_operations_struct */
 struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset);
 
 void kvm_dirty_ring_free(struct kvm_dirty_ring *ring);
-bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring);
 
 #endif /* CONFIG_HAVE_KVM_DIRTY_RING */
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f4519d3689e1..53fa3134fee0 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -157,6 +157,7 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_VM_DEAD           (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_UNBLOCK           2
 #define KVM_REQ_UNHALT            3
+#define KVM_REQ_RING_SOFT_FULL    4
 #define KVM_REQUEST_ARCH_BASE     8
 
 /*
diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
index d6fabf238032..f68d75026bc0 100644
--- a/virt/kvm/dirty_ring.c
+++ b/virt/kvm/dirty_ring.c
@@ -26,7 +26,7 @@ static u32 kvm_dirty_ring_used(struct kvm_dirty_ring *ring)
 	return READ_ONCE(ring->dirty_index) - READ_ONCE(ring->reset_index);
 }
 
-bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring)
+static bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring)
 {
 	return kvm_dirty_ring_used(ring) >= ring->soft_limit;
 }
@@ -149,6 +149,7 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
 
 void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset)
 {
+	struct kvm_vcpu *vcpu = container_of(ring, struct kvm_vcpu, dirty_ring);
 	struct kvm_dirty_gfn *entry;
 
 	/* It should never get full */
@@ -166,6 +167,22 @@ void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset)
 	kvm_dirty_gfn_set_dirtied(entry);
 	ring->dirty_index++;
 	trace_kvm_dirty_ring_push(ring, slot, offset);
+
+	if (kvm_dirty_ring_soft_full(ring))
+		kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);
+}
+
+bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
+{
+	if (kvm_check_request(KVM_REQ_RING_SOFT_FULL, vcpu) &&
+		kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {
+		kvm_make_request(KVM_REQ_RING_SOFT_FULL, vcpu);
+		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
+		trace_kvm_dirty_ring_exit(vcpu);
+		return true;
+	}
+
+	return false;
 }
 
 struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset)
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 2/6] KVM: x86: Move declaration of kvm_cpu_dirty_log_size() to kvm_dirty_ring.h

[Gavin Shan]

Not all architectures like ARM64 need to override the function. Move
its declaration to kvm_dirty_ring.h to avoid the following compiling
warning on ARM64 when the feature is enabled.

  arch/arm64/kvm/../../../virt/kvm/dirty_ring.c:14:12:        \
  warning: no previous prototype for 'kvm_cpu_dirty_log_size' \
  [-Wmissing-prototypes]                                      \
  int __weak kvm_cpu_dirty_log_size(void)

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 2 --
 include/linux/kvm_dirty_ring.h  | 1 +
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index aa381ab69a19..f11b6a9388b5 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2083,8 +2083,6 @@ static inline int kvm_cpu_get_apicid(int mps_cpu)
 #define GET_SMSTATE(type, buf, offset)		\
 	(*(type *)((buf) + (offset) - 0x7e00))
 
-int kvm_cpu_dirty_log_size(void);
-
 int memslot_rmap_alloc(struct kvm_memory_slot *slot, unsigned long npages);
 
 #define KVM_CLOCK_VALID_FLAGS						\
diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
index b188bfcf3a09..7926c5566c0a 100644
--- a/include/linux/kvm_dirty_ring.h
+++ b/include/linux/kvm_dirty_ring.h
@@ -71,6 +71,7 @@ static inline void kvm_dirty_ring_free(struct kvm_dirty_ring *ring)
 
 #else /* CONFIG_HAVE_KVM_DIRTY_RING */
 
+int kvm_cpu_dirty_log_size(void);
 u32 kvm_dirty_ring_get_rsvd_entries(void);
 int kvm_dirty_ring_alloc(struct kvm_dirty_ring *ring, int index, u32 size);
 
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Enable ring-based dirty memory tracking on arm64 by selecting
CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL and providing the ring buffer's
physical page offset (KVM_DIRTY_LOG_PAGE_OFFSET).

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 Documentation/virt/kvm/api.rst    | 2 +-
 arch/arm64/include/uapi/asm/kvm.h | 1 +
 arch/arm64/kvm/Kconfig            | 1 +
 arch/arm64/kvm/arm.c              | 3 +++
 4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 32427ea160df..bad78a974ccf 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -8022,7 +8022,7 @@ regardless of what has actually been exposed through the CPUID leaf.
 8.29 KVM_CAP_DIRTY_LOG_RING/KVM_CAP_DIRTY_LOG_RING_ACQ_REL
 ----------------------------------------------------------
 
-:Architectures: x86
+:Architectures: x86, arm64
 :Parameters: args[0] - size of the dirty log ring
 
 KVM is capable of tracking dirty memory using ring buffers that are
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 316917b98707..a7a857f1784d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -43,6 +43,7 @@
 #define __KVM_HAVE_VCPU_EVENTS
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
+#define KVM_DIRTY_LOG_PAGE_OFFSET 64
 
 #define KVM_REG_SIZE(id)						\
 	(1U << (((id) & KVM_REG_SIZE_MASK) >> KVM_REG_SIZE_SHIFT))
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 815cc118c675..095c714b559e 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -32,6 +32,7 @@ menuconfig KVM
 	select KVM_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
+	select HAVE_KVM_DIRTY_RING_ACQ_REL
 	select HAVE_KVM_MSI
 	select HAVE_KVM_IRQCHIP
 	select HAVE_KVM_IRQ_ROUTING
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 917086be5c6b..53c963a159bc 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -747,6 +747,9 @@ static int check_vcpu_requests(struct kvm_vcpu *vcpu)
 
 		if (kvm_check_request(KVM_REQ_SUSPEND, vcpu))
 			return kvm_vcpu_suspend(vcpu);
+
+		if (kvm_dirty_ring_check_request(vcpu))
+			return 0;
 	}
 
 	return 1;
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 4/6] KVM: selftests: Use host page size to map ring buffer in dirty_log_test

[Gavin Shan]

In vcpu_map_dirty_ring(), the guest's page size is used to figure out
the offset in the virtual area. It works fine when we have same page
sizes on host and guest. However, it fails when the page sizes on host
and guest are different on arm64, like below error messages indicates.

  # ./dirty_log_test -M dirty-ring -m 7
  Setting log mode to: 'dirty-ring'
  Test iterations: 32, interval: 10 (ms)
  Testing guest mode: PA-bits:40,  VA-bits:48, 64K pages
  guest physical test memory offset: 0xffbffc0000
  vcpu stops because vcpu is kicked out...
  Notifying vcpu to continue
  vcpu continues now.
  ==== Test Assertion Failure ====
  lib/kvm_util.c:1477: addr == MAP_FAILED
  pid=9000 tid=9000 errno=0 - Success
  1  0x0000000000405f5b: vcpu_map_dirty_ring at kvm_util.c:1477
  2  0x0000000000402ebb: dirty_ring_collect_dirty_pages at dirty_log_test.c:349
  3  0x00000000004029b3: log_mode_collect_dirty_pages at dirty_log_test.c:478
  4  (inlined by) run_test at dirty_log_test.c:778
  5  (inlined by) run_test at dirty_log_test.c:691
  6  0x0000000000403a57: for_each_guest_mode at guest_modes.c:105
  7  0x0000000000401ccf: main at dirty_log_test.c:921
  8  0x0000ffffb06ec79b: ?? ??:0
  9  0x0000ffffb06ec86b: ?? ??:0
  10 0x0000000000401def: _start at ??:?
  Dirty ring mapped private

Fix the issue by using host's page size to map the ring buffer.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 411a4c0bc81c..c339c8b694e0 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1467,7 +1467,7 @@ struct kvm_reg_list *vcpu_get_reg_list(struct kvm_vcpu *vcpu)
 
 void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu)
 {
-	uint32_t page_size = vcpu->vm->page_size;
+	uint32_t page_size = getpagesize();
 	uint32_t size = vcpu->vm->dirty_ring_size;
 
 	TEST_ASSERT(size > 0, "Should enable dirty ring first");
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 5/6] KVM: selftests: Clear dirty ring states between two modes in dirty_log_test

[Gavin Shan]

There are two states, which need to be cleared before next mode
is executed. Otherwise, we will hit failure as the following messages
indicate.

- The variable 'dirty_ring_vcpu_ring_full' shared by main and vcpu
  thread. It's indicating if the vcpu exit due to full ring buffer.
  The value can be carried from previous mode (VM_MODE_P40V48_4K) to
  current one (VM_MODE_P40V48_64K) when VM_MODE_P40V48_16K isn't
  supported.

- The current ring buffer index needs to be reset before next mode
  (VM_MODE_P40V48_64K) is executed. Otherwise, the stale value is
  carried from previous mode (VM_MODE_P40V48_4K).

  # ./dirty_log_test -M dirty-ring
  Setting log mode to: 'dirty-ring'
  Test iterations: 32, interval: 10 (ms)
  Testing guest mode: PA-bits:40,  VA-bits:48,  4K pages
  guest physical test memory offset: 0xffbfffc000
    :
  Dirtied 995328 pages
  Total bits checked: dirty (1012434), clear (7114123), track_next (966700)
  Testing guest mode: PA-bits:40,  VA-bits:48, 64K pages
  guest physical test memory offset: 0xffbffc0000
  vcpu stops because vcpu is kicked out...
  vcpu continues now.
  Notifying vcpu to continue
  Iteration 1 collected 0 pages
  vcpu stops because dirty ring is full...
  vcpu continues now.
  vcpu stops because dirty ring is full...
  vcpu continues now.
  vcpu stops because dirty ring is full...
  ==== Test Assertion Failure ====
  dirty_log_test.c:369: cleared == count
  pid=10541 tid=10541 errno=22 - Invalid argument
     1	0x0000000000403087: dirty_ring_collect_dirty_pages at dirty_log_test.c:369
     2	0x0000000000402a0b: log_mode_collect_dirty_pages at dirty_log_test.c:492
     3	 (inlined by) run_test at dirty_log_test.c:795
     4	 (inlined by) run_test at dirty_log_test.c:705
     5	0x0000000000403a37: for_each_guest_mode at guest_modes.c:100
     6	0x0000000000401ccf: main at dirty_log_test.c:938
     7	0x0000ffff9ecd279b: ?? ??:0
     8	0x0000ffff9ecd286b: ?? ??:0
     9	0x0000000000401def: _start at ??:?
  Reset dirty pages (0) mismatch with collected (35566)

Fix the issues by clearing 'dirty_ring_vcpu_ring_full' and the ring
buffer index before next new mode is to be executed.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c | 27 ++++++++++++--------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index b5234d6efbe1..8758c10ec850 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -226,13 +226,15 @@ static void clear_log_create_vm_done(struct kvm_vm *vm)
 }
 
 static void dirty_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
-					  void *bitmap, uint32_t num_pages)
+					  void *bitmap, uint32_t num_pages,
+					  uint32_t *unused)
 {
 	kvm_vm_get_dirty_log(vcpu->vm, slot, bitmap);
 }
 
 static void clear_log_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
-					  void *bitmap, uint32_t num_pages)
+					  void *bitmap, uint32_t num_pages,
+					  uint32_t *unused)
 {
 	kvm_vm_get_dirty_log(vcpu->vm, slot, bitmap);
 	kvm_vm_clear_dirty_log(vcpu->vm, slot, bitmap, 0, num_pages);
@@ -329,10 +331,9 @@ static void dirty_ring_continue_vcpu(void)
 }
 
 static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
-					   void *bitmap, uint32_t num_pages)
+					   void *bitmap, uint32_t num_pages,
+					   uint32_t *ring_buf_idx)
 {
-	/* We only have one vcpu */
-	static uint32_t fetch_index = 0;
 	uint32_t count = 0, cleared;
 	bool continued_vcpu = false;
 
@@ -349,7 +350,8 @@ static void dirty_ring_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
 
 	/* Only have one vcpu */
 	count = dirty_ring_collect_one(vcpu_map_dirty_ring(vcpu),
-				       slot, bitmap, num_pages, &fetch_index);
+				       slot, bitmap, num_pages,
+				       ring_buf_idx);
 
 	cleared = kvm_vm_reset_dirty_ring(vcpu->vm);
 
@@ -406,7 +408,8 @@ struct log_mode {
 	void (*create_vm_done)(struct kvm_vm *vm);
 	/* Hook to collect the dirty pages into the bitmap provided */
 	void (*collect_dirty_pages) (struct kvm_vcpu *vcpu, int slot,
-				     void *bitmap, uint32_t num_pages);
+				     void *bitmap, uint32_t num_pages,
+				     uint32_t *ring_buf_idx);
 	/* Hook to call when after each vcpu run */
 	void (*after_vcpu_run)(struct kvm_vcpu *vcpu, int ret, int err);
 	void (*before_vcpu_join) (void);
@@ -471,13 +474,14 @@ static void log_mode_create_vm_done(struct kvm_vm *vm)
 }
 
 static void log_mode_collect_dirty_pages(struct kvm_vcpu *vcpu, int slot,
-					 void *bitmap, uint32_t num_pages)
+					 void *bitmap, uint32_t num_pages,
+					 uint32_t *ring_buf_idx)
 {
 	struct log_mode *mode = &log_modes[host_log_mode];
 
 	TEST_ASSERT(mode->collect_dirty_pages != NULL,
 		    "collect_dirty_pages() is required for any log mode!");
-	mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages);
+	mode->collect_dirty_pages(vcpu, slot, bitmap, num_pages, ring_buf_idx);
 }
 
 static void log_mode_after_vcpu_run(struct kvm_vcpu *vcpu, int ret, int err)
@@ -696,6 +700,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
 	unsigned long *bmap;
+	uint32_t ring_buf_idx = 0;
 
 	if (!log_mode_supported()) {
 		print_skip("Log mode '%s' not supported",
@@ -771,6 +776,7 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 	host_dirty_count = 0;
 	host_clear_count = 0;
 	host_track_next_count = 0;
+	WRITE_ONCE(dirty_ring_vcpu_ring_full, false);
 
 	pthread_create(&vcpu_thread, NULL, vcpu_worker, vcpu);
 
@@ -778,7 +784,8 @@ static void run_test(enum vm_guest_mode mode, void *arg)
 		/* Give the vcpu thread some time to dirty some pages */
 		usleep(p->interval * 1000);
 		log_mode_collect_dirty_pages(vcpu, TEST_MEM_SLOT_INDEX,
-					     bmap, host_num_pages);
+					     bmap, host_num_pages,
+					     &ring_buf_idx);
 
 		/*
 		 * See vcpu_sync_stop_requested definition for details on why
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v4 6/6] KVM: selftests: Automate choosing dirty ring size in dirty_log_test

[Gavin Shan]

In the dirty ring case, we rely on vcpu exit due to full dirty ring
state. On ARM64 system, there are 4096 host pages when the host
page size is 64KB. In this case, the vcpu never exits due to the
full dirty ring state. The similar case is 4KB page size on host
and 64KB page size on guest. The vcpu corrupts same set of host
pages, but the dirty page information isn't collected in the main
thread. This leads to infinite loop as the following log shows.

  # ./dirty_log_test -M dirty-ring -c 65536 -m 5
  Setting log mode to: 'dirty-ring'
  Test iterations: 32, interval: 10 (ms)
  Testing guest mode: PA-bits:40,  VA-bits:48,  4K pages
  guest physical test memory offset: 0xffbffe0000
  vcpu stops because vcpu is kicked out...
  Notifying vcpu to continue
  vcpu continues now.
  Iteration 1 collected 576 pages
  <No more output afterwards>

Fix the issue by automatically choosing the best dirty ring size,
to ensure vcpu exit due to full dirty ring state. The option '-c'
becomes a hint to the dirty ring count, instead of the value of it.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 tools/testing/selftests/kvm/dirty_log_test.c | 26 +++++++++++++++++---
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/dirty_log_test.c b/tools/testing/selftests/kvm/dirty_log_test.c
index 8758c10ec850..a87e5f78ebf1 100644
--- a/tools/testing/selftests/kvm/dirty_log_test.c
+++ b/tools/testing/selftests/kvm/dirty_log_test.c
@@ -24,6 +24,9 @@
 #include "guest_modes.h"
 #include "processor.h"
 
+#define DIRTY_MEM_BITS 30 /* 1G */
+#define PAGE_SHIFT_4K  12
+
 /* The memory slot index to track dirty pages */
 #define TEST_MEM_SLOT_INDEX		1
 
@@ -273,6 +276,24 @@ static bool dirty_ring_supported(void)
 
 static void dirty_ring_create_vm_done(struct kvm_vm *vm)
 {
+	uint64_t pages;
+	uint32_t limit;
+
+	/*
+	 * We rely on vcpu exit due to full dirty ring state. Adjust
+	 * the ring buffer size to ensure we're able to reach the
+	 * full dirty ring state.
+	 */
+	pages = (1ul << (DIRTY_MEM_BITS - vm->page_shift)) + 3;
+	pages = vm_adjust_num_guest_pages(vm->mode, pages);
+	if (vm->page_size < getpagesize())
+		pages = vm_num_host_pages(vm->mode, pages);
+
+	limit = 1 << (31 - __builtin_clz(pages));
+	test_dirty_ring_count = 1 << (31 - __builtin_clz(test_dirty_ring_count));
+	test_dirty_ring_count = min(limit, test_dirty_ring_count);
+	pr_info("dirty ring count: 0x%x\n", test_dirty_ring_count);
+
 	/*
 	 * Switch to dirty ring mode after VM creation but before any
 	 * of the vcpu creation.
@@ -685,9 +706,6 @@ static struct kvm_vm *create_vm(enum vm_guest_mode mode, struct kvm_vcpu **vcpu,
 	return vm;
 }
 
-#define DIRTY_MEM_BITS 30 /* 1G */
-#define PAGE_SHIFT_4K  12
-
 struct test_params {
 	unsigned long iterations;
 	unsigned long interval;
@@ -830,7 +848,7 @@ static void help(char *name)
 	printf("usage: %s [-h] [-i iterations] [-I interval] "
 	       "[-p offset] [-m mode]\n", name);
 	puts("");
-	printf(" -c: specify dirty ring size, in number of entries\n");
+	printf(" -c: hint to dirty ring size, in number of entries\n");
 	printf("     (only useful for dirty-ring test; default: %"PRIu32")\n",
 	       TEST_DIRTY_RING_COUNT);
 	printf(" -i: specify iteration counts (default: %"PRIu64")\n",
-- 
2.23.0

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 1/6] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Marc Zyngier]

On Mon, 26 Sep 2022 20:54:34 -0400,
Gavin Shan <gshan@redhat.com> wrote:
> 
> This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty
> ring of the specific VCPU becomes softly full in kvm_dirty_ring_push().
> The VCPU is enforced to exit when the request is raised and its
> dirty ring is softly full on its entrance.
> 
> The event is checked and handled in the newly introduced helper
> kvm_dirty_ring_check_request(). With this, kvm_dirty_ring_soft_full()
> becomes a private function.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  arch/x86/kvm/x86.c             | 15 ++++++---------
>  include/linux/kvm_dirty_ring.h | 13 +++++++------
>  include/linux/kvm_host.h       |  1 +
>  virt/kvm/dirty_ring.c          | 19 ++++++++++++++++++-
>  4 files changed, 32 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b0c47b41c264..0dd0d32073e7 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10260,16 +10260,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  
>  	bool req_immediate_exit = false;
>  
> -	/* Forbid vmenter if vcpu dirty ring is soft-full */
> -	if (unlikely(vcpu->kvm->dirty_ring_size &&
> -		     kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
> -		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
> -		trace_kvm_dirty_ring_exit(vcpu);
> -		r = 0;
> -		goto out;
> -	}
> -
>  	if (kvm_request_pending(vcpu)) {
> +		/* Forbid vmenter if vcpu dirty ring is soft-full */
> +		if (kvm_dirty_ring_check_request(vcpu)) {
> +			r = 0;
> +			goto out;
> +		}
> +
>  		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
>  			r = -EIO;
>  			goto out;
> diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
> index 906f899813dc..b188bfcf3a09 100644
> --- a/include/linux/kvm_dirty_ring.h
> +++ b/include/linux/kvm_dirty_ring.h
> @@ -54,6 +54,11 @@ static inline void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
>  {
>  }
>  
> +static inline bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
> +{
> +	return false;
> +}
> +

nit: I don't think this is needed at all. The dirty ring feature is
not user-selectable, and this is always called from arch code that is
fully aware of that option.

This can be fixed when applying the patch though, no need to resend
for this.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 0/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

+ Sean

On Mon, 26 Sep 2022 20:54:33 -0400,
Gavin Shan <gshan@redhat.com> wrote:
> 
> This series enables the ring-based dirty memory tracking for ARM64.
> The feature has been available and enabled on x86 for a while. It
> is beneficial when the number of dirty pages is small in a checkpointing
> system or live migration scenario. More details can be found from
> fb04a1eddb1a ("KVM: X86: Implement ring-based dirty memory tracking").
> 
> This series is applied on top of Marc's v2 series [0], fixing dirty-ring
> ordering issue.

This looks good to me as it stands. If someone on the x86 side of
things is willing to ack the x86 changes (both here and in my
series[0]), I'm happy to queue the whole thing.

Thanks,

	M.

[0] https://lore.kernel.org/kvmarm/20220926145120.27974-1-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 1/6] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Gavin Shan]

Hi Marc,

On 9/27/22 8:26 PM, Marc Zyngier wrote:
> On Mon, 26 Sep 2022 20:54:34 -0400,
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> This adds KVM_REQ_RING_SOFT_FULL, which is raised when the dirty
>> ring of the specific VCPU becomes softly full in kvm_dirty_ring_push().
>> The VCPU is enforced to exit when the request is raised and its
>> dirty ring is softly full on its entrance.
>>
>> The event is checked and handled in the newly introduced helper
>> kvm_dirty_ring_check_request(). With this, kvm_dirty_ring_soft_full()
>> becomes a private function.
>>
>> Suggested-by: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   arch/x86/kvm/x86.c             | 15 ++++++---------
>>   include/linux/kvm_dirty_ring.h | 13 +++++++------
>>   include/linux/kvm_host.h       |  1 +
>>   virt/kvm/dirty_ring.c          | 19 ++++++++++++++++++-
>>   4 files changed, 32 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index b0c47b41c264..0dd0d32073e7 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10260,16 +10260,13 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   
>>   	bool req_immediate_exit = false;
>>   
>> -	/* Forbid vmenter if vcpu dirty ring is soft-full */
>> -	if (unlikely(vcpu->kvm->dirty_ring_size &&
>> -		     kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
>> -		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
>> -		trace_kvm_dirty_ring_exit(vcpu);
>> -		r = 0;
>> -		goto out;
>> -	}
>> -
>>   	if (kvm_request_pending(vcpu)) {
>> +		/* Forbid vmenter if vcpu dirty ring is soft-full */
>> +		if (kvm_dirty_ring_check_request(vcpu)) {
>> +			r = 0;
>> +			goto out;
>> +		}
>> +
>>   		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
>>   			r = -EIO;
>>   			goto out;
>> diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
>> index 906f899813dc..b188bfcf3a09 100644
>> --- a/include/linux/kvm_dirty_ring.h
>> +++ b/include/linux/kvm_dirty_ring.h
>> @@ -54,6 +54,11 @@ static inline void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
>>   {
>>   }
>>   
>> +static inline bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
>> +{
>> +	return false;
>> +}
>> +
> 
> nit: I don't think this is needed at all. The dirty ring feature is
> not user-selectable, and this is always called from arch code that is
> fully aware of that option.
> 
> This can be fixed when applying the patch though, no need to resend
> for this.
> 

I had the assumption that the corresponding kernel config options are
dropped from arch/x86/kvm/Kconfig or arch/arm64/kvm/Kconfig by developers.
I think it's fine to drop it because the developers need to add the
stub in this particular and rare case.

Please help to drop it if you're going to queue the series. I will drop
it if another respin is needed.

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 1/6] KVM: x86: Introduce KVM_REQ_RING_SOFT_FULL

[Peter Xu]

On Tue, Sep 27, 2022 at 06:26:41AM -0400, Marc Zyngier wrote:
> > +static inline bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
> > +{
> > +	return false;
> > +}
> > +
> 
> nit: I don't think this is needed at all. The dirty ring feature is
> not user-selectable, and this is always called from arch code that is
> fully aware of that option.

Agreed.  With the change:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 2/6] KVM: x86: Move declaration of kvm_cpu_dirty_log_size() to kvm_dirty_ring.h

[Peter Xu]

On Tue, Sep 27, 2022 at 08:54:35AM +0800, Gavin Shan wrote:
> Not all architectures like ARM64 need to override the function. Move
> its declaration to kvm_dirty_ring.h to avoid the following compiling
> warning on ARM64 when the feature is enabled.
> 
>   arch/arm64/kvm/../../../virt/kvm/dirty_ring.c:14:12:        \
>   warning: no previous prototype for 'kvm_cpu_dirty_log_size' \
>   [-Wmissing-prototypes]                                      \
>   int __weak kvm_cpu_dirty_log_size(void)
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Tue, Sep 27, 2022 at 08:54:36AM +0800, Gavin Shan wrote:
> Enable ring-based dirty memory tracking on arm64 by selecting
> CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL and providing the ring buffer's
> physical page offset (KVM_DIRTY_LOG_PAGE_OFFSET).
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>

Gavin,

Any decision made on how to tackle with the GIC status dirty bits?

Thanks,

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Tue, 27 Sep 2022 12:02:52 -0400,
Peter Xu <peterx@redhat.com> wrote:
> 
> On Tue, Sep 27, 2022 at 08:54:36AM +0800, Gavin Shan wrote:
> > Enable ring-based dirty memory tracking on arm64 by selecting
> > CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL and providing the ring buffer's
> > physical page offset (KVM_DIRTY_LOG_PAGE_OFFSET).
> > 
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> Gavin,
> 
> Any decision made on how to tackle with the GIC status dirty bits?

Which dirty bits? Are you talking of the per-RD pending bits?

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Tue, Sep 27, 2022 at 01:32:07PM -0400, Marc Zyngier wrote:
> On Tue, 27 Sep 2022 12:02:52 -0400,
> Peter Xu <peterx@redhat.com> wrote:
> > 
> > On Tue, Sep 27, 2022 at 08:54:36AM +0800, Gavin Shan wrote:
> > > Enable ring-based dirty memory tracking on arm64 by selecting
> > > CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL and providing the ring buffer's
> > > physical page offset (KVM_DIRTY_LOG_PAGE_OFFSET).
> > > 
> > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > 
> > Gavin,
> > 
> > Any decision made on how to tackle with the GIC status dirty bits?
> 
> Which dirty bits? Are you talking of the per-RD pending bits?

Gavin found that some dirty pfn path may not have vcpu context for aarch64
offlist.

Borrowing Gavin's trace dump:

  el0t_64_sync
  el0t_64_sync_handler
  el0_svc
  do_el0_svc
  __arm64_sys_ioctl
  kvm_device_ioctl
  vgic_its_set_attr
  vgic_its_save_tables_v0
  kvm_write_guest
  __kvm_write_guest_page
  mark_page_dirty_in_slot

With current code it'll trigger the warning in mark_page_dirty_in_slot.

An userspace approach is doable by setting these pages as always dirty in
userspace (QEMU), but even if so IIUC we'll need to drop the warning
message in mark_page_dirty_in_slot() then we take no-vcpu dirty as no-op
and expected.

I'll leave the details to Gavin.

Thanks,

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Peter and Marc,

On 9/28/22 4:21 AM, Peter Xu wrote:
> On Tue, Sep 27, 2022 at 01:32:07PM -0400, Marc Zyngier wrote:
>> On Tue, 27 Sep 2022 12:02:52 -0400,
>> Peter Xu <peterx@redhat.com> wrote:

[...]

>>>
>>> Any decision made on how to tackle with the GIC status dirty bits?
>>
>> Which dirty bits? Are you talking of the per-RD pending bits?
> 
> Gavin found that some dirty pfn path may not have vcpu context for aarch64
> offlist.
> 
> Borrowing Gavin's trace dump:
> 
>    el0t_64_sync
>    el0t_64_sync_handler
>    el0_svc
>    do_el0_svc
>    __arm64_sys_ioctl
>    kvm_device_ioctl
>    vgic_its_set_attr
>    vgic_its_save_tables_v0
>    kvm_write_guest
>    __kvm_write_guest_page
>    mark_page_dirty_in_slot
> 
> With current code it'll trigger the warning in mark_page_dirty_in_slot.
> 
> An userspace approach is doable by setting these pages as always dirty in
> userspace (QEMU), but even if so IIUC we'll need to drop the warning
> message in mark_page_dirty_in_slot() then we take no-vcpu dirty as no-op
> and expected.
> 
> I'll leave the details to Gavin.
> 

Thanks to Peter for bringing the issue to here for further discussion. I'm
slowly approaching the idea/design to resolve the issue. If Marc agrees, It
wouldn't stop us to merge the series since the newly introduced capability
is only used by kvm/selftests/dirty_log_test. It means more changes are needed
by QEMU in order to enable the feature there through KVM_CAP_DIRTY_LOG_RING_ACQ_REL.
I can post follow-up patches to fix the VGIC/ITS issue.

We had some internal discussion about the mentioned issue. It's all about the
various VGIC/ITS tables, listed as below. In QEMU, those tables are requested
to be saved to memory for migration or shutdown. Unfortunately, there is no
running vcpu in this particular path and the corresponding dirty bits are
dropped with warning messages in mark_page_dirty_in_slot().

    ---> Command to save VGIC/ITS tables
         group: KVM_DEV_ARM_VGIC_GRP_CTRL
         attr : KVM_DEV_ARM_ITS_SAVE_TABLES

    ---> VGIC/ITS tables to be saved
         Device Table                     // REG_GITS_BASER_0
         Interrupt Translation Tables     // GITS_CMD_MAPD
         Collection Table                 // REG_GITS_BASER_1

    ---> QEMU's backtraces, triggering the issue

         main                                thread_start
         qemu_main                           start_thread
         qemu_cleanup                        qemu_thread_start
         vm_shutdown                         migration_thread
         do_vm_stop                          migration_iteration_run
         vm_state_notify                     migration_completion
         vm_change_state_handler             vm_stop_force_state
                                             do_vm_stop
                                             vm_state_notify
                                             vm_change_state_handler          // In hw/intc/arm_gicv3_its_kvm.c

I have rough idea as below. It's appreciated if you can comment before I'm
going a head for the prototype. The overall idea is to introduce another
dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
to dirty ring for vcpu (vcpu-dirty-ring).

    - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
      entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
      those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.

    - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
      mmap(kvm->fd). However, there won't have similar reset interface. It means
      'struct kvm_dirty_gfn::flags' won't track any information as we do for
      vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
      advertise 'always-dirty' pages from host to userspace.
      
    - For QEMU, shutdown/suspend/resume cases won't be concerning us any more. The
      only concerned case is migration. When the migration is about to complete,
      kvm-dirty-ring entries are fetched and the dirty bits are updated to global
      dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
      the code to find the best spot to do it.

Thanks,
Gavin



_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

Hi Gavin,

On Wed, 28 Sep 2022 00:47:43 +0100,
Gavin Shan <gshan@redhat.com> wrote:

> I have rough idea as below. It's appreciated if you can comment before I'm
> going a head for the prototype. The overall idea is to introduce another
> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
> to dirty ring for vcpu (vcpu-dirty-ring).
> 
>    - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
>      entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
>      those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
> 
>    - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
>      mmap(kvm->fd). However, there won't have similar reset interface. It means
>      'struct kvm_dirty_gfn::flags' won't track any information as we do for
>      vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
>      advertise 'always-dirty' pages from host to userspace.
>         - For QEMU, shutdown/suspend/resume cases won't be concerning
> us any more. The
>      only concerned case is migration. When the migration is about to complete,
>      kvm-dirty-ring entries are fetched and the dirty bits are updated to global
>      dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
>      the code to find the best spot to do it.

I think it makes a lot of sense to have a way to log writes that are
not generated by a vpcu, such as the GIC and maybe other things in the
future, such as DMA traffic (some SMMUs are able to track dirty pages
as well).

However, I don't really see the point in inventing a new mechanism for
that. Why don't we simply allow non-vpcu dirty pages to be tracked in
the dirty *bitmap*?

From a kernel perspective, this is dead easy:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5b064dbadaf4..ae9138f29d51 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
 	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
 
 #ifdef CONFIG_HAVE_KVM_DIRTY_RING
-	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
+	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
 		return;
 #endif
 
@@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 		u32 slot = (memslot->as_id << 16) | memslot->id;
 
-		if (kvm->dirty_ring_size)
+		if (vpcu && kvm->dirty_ring_size)
 			kvm_dirty_ring_push(&vcpu->dirty_ring,
 					    slot, rel_gfn);
-		else
+		/* non-vpcu dirtying ends up in the global bitmap */
+		if (!vcpu && memslot->dirty_bitmap)
 			set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }

though I'm sure there is a few more things to it.

To me, this is just a relaxation of an arbitrary limitation, as the
current assumption that only vcpus can dirty memory doesn't hold at
all.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
> Hi Gavin,
> 
> On Wed, 28 Sep 2022 00:47:43 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
> 
> > I have rough idea as below. It's appreciated if you can comment before I'm
> > going a head for the prototype. The overall idea is to introduce another
> > dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
> > to dirty ring for vcpu (vcpu-dirty-ring).
> > 
> >    - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
> >      entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
> >      those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
> > 
> >    - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
> >      mmap(kvm->fd). However, there won't have similar reset interface. It means
> >      'struct kvm_dirty_gfn::flags' won't track any information as we do for
> >      vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
> >      advertise 'always-dirty' pages from host to userspace.
> >         - For QEMU, shutdown/suspend/resume cases won't be concerning
> > us any more. The
> >      only concerned case is migration. When the migration is about to complete,
> >      kvm-dirty-ring entries are fetched and the dirty bits are updated to global
> >      dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
> >      the code to find the best spot to do it.
> 
> I think it makes a lot of sense to have a way to log writes that are
> not generated by a vpcu, such as the GIC and maybe other things in the
> future, such as DMA traffic (some SMMUs are able to track dirty pages
> as well).
> 
> However, I don't really see the point in inventing a new mechanism for
> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
> the dirty *bitmap*?
> 
> From a kernel perspective, this is dead easy:
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5b064dbadaf4..ae9138f29d51 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>  	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>  
>  #ifdef CONFIG_HAVE_KVM_DIRTY_RING
> -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
> +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>  		return;
>  #endif
>  
> @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>  		unsigned long rel_gfn = gfn - memslot->base_gfn;
>  		u32 slot = (memslot->as_id << 16) | memslot->id;
>  
> -		if (kvm->dirty_ring_size)
> +		if (vpcu && kvm->dirty_ring_size)
>  			kvm_dirty_ring_push(&vcpu->dirty_ring,
>  					    slot, rel_gfn);
> -		else
> +		/* non-vpcu dirtying ends up in the global bitmap */
> +		if (!vcpu && memslot->dirty_bitmap)
>  			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>  	}
>  }
> 
> though I'm sure there is a few more things to it.

Yes, currently the bitmaps are not created when rings are enabled.
kvm_prepare_memory_region() has:

		else if (!kvm->dirty_ring_size) {
			r = kvm_alloc_dirty_bitmap(new);

But I think maybe that's a solution worth considering.  Using the rings
have a major challenge on the limitation of ring size, so that for e.g. an
ioctl we need to make sure the pages to dirty within an ioctl procedure
will not be more than the ring can take.  Using dirty bitmap for a last
phase sync of constant (but still very small amount of) dirty pages does
sound reasonable and can avoid that complexity.  The payoff is we'll need
to allocate both the rings and the bitmaps.

> 
> To me, this is just a relaxation of an arbitrary limitation, as the
> current assumption that only vcpus can dirty memory doesn't hold at
> all.

The initial dirty ring proposal has a per-vm ring, but after we
investigated x86 we found that all legal dirty paths are with a vcpu
context (except one outlier on kvmgt which fixed within itself), so we
dropped the per-vm ring.

One thing to mention is that DMAs should not count in this case because
that's from device perspective, IOW either IOMMU or SMMU dirty tracking
should be reported to the device driver that interacts with the userspace
not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
includes emulated DMA like vhost (VHOST_SET_LOG_BASE).

Thanks,

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Marc and Peter,

On 9/29/22 12:52 AM, Peter Xu wrote:
> On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
>> On Wed, 28 Sep 2022 00:47:43 +0100,
>> Gavin Shan <gshan@redhat.com> wrote:
>>
>>> I have rough idea as below. It's appreciated if you can comment before I'm
>>> going a head for the prototype. The overall idea is to introduce another
>>> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
>>> to dirty ring for vcpu (vcpu-dirty-ring).
>>>
>>>     - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
>>>       entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
>>>       those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
>>>
>>>     - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
>>>       mmap(kvm->fd). However, there won't have similar reset interface. It means
>>>       'struct kvm_dirty_gfn::flags' won't track any information as we do for
>>>       vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
>>>       advertise 'always-dirty' pages from host to userspace.
>>>          - For QEMU, shutdown/suspend/resume cases won't be concerning
>>> us any more. The
>>>       only concerned case is migration. When the migration is about to complete,
>>>       kvm-dirty-ring entries are fetched and the dirty bits are updated to global
>>>       dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
>>>       the code to find the best spot to do it.
>>
>> I think it makes a lot of sense to have a way to log writes that are
>> not generated by a vpcu, such as the GIC and maybe other things in the
>> future, such as DMA traffic (some SMMUs are able to track dirty pages
>> as well).
>>
>> However, I don't really see the point in inventing a new mechanism for
>> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
>> the dirty *bitmap*?
>>
>>  From a kernel perspective, this is dead easy:
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 5b064dbadaf4..ae9138f29d51 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>   	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>>   
>>   #ifdef CONFIG_HAVE_KVM_DIRTY_RING
>> -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>> +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>   		return;
>>   #endif
>>   
>> @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>   		unsigned long rel_gfn = gfn - memslot->base_gfn;
>>   		u32 slot = (memslot->as_id << 16) | memslot->id;
>>   
>> -		if (kvm->dirty_ring_size)
>> +		if (vpcu && kvm->dirty_ring_size)
>>   			kvm_dirty_ring_push(&vcpu->dirty_ring,
>>   					    slot, rel_gfn);
>> -		else
>> +		/* non-vpcu dirtying ends up in the global bitmap */
>> +		if (!vcpu && memslot->dirty_bitmap)
>>   			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>>   	}
>>   }
>>
>> though I'm sure there is a few more things to it.
> 
> Yes, currently the bitmaps are not created when rings are enabled.
> kvm_prepare_memory_region() has:
> 
> 		else if (!kvm->dirty_ring_size) {
> 			r = kvm_alloc_dirty_bitmap(new);
> 
> But I think maybe that's a solution worth considering.  Using the rings
> have a major challenge on the limitation of ring size, so that for e.g. an
> ioctl we need to make sure the pages to dirty within an ioctl procedure
> will not be more than the ring can take.  Using dirty bitmap for a last
> phase sync of constant (but still very small amount of) dirty pages does
> sound reasonable and can avoid that complexity.  The payoff is we'll need
> to allocate both the rings and the bitmaps.
> 

Ok. I was thinking of using the bitmap to convey the dirty pages for
this particular case, where we don't have running vcpu. The concern I had
is the natural difference between a ring and bitmap. The ring-buffer is
discrete, comparing to bitmap. Besides, it sounds a little strange to
have two different sets of meta-data to track the data (dirty pages).

However, bitmap is easier way than per-vm ring. The constrains with
per-vm ring is just as Peter pointed. So lets reuse the bitmap to
convey the dirty pages for this particular case. I think the payoff,
extra bitmap, is acceptable. For this, we need another capability
(KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
bitmap in the last phase of migration.

If all of us agree on this, I can send another kernel patch to address
this. QEMU still need more patches so that the feature can be supported.

>>
>> To me, this is just a relaxation of an arbitrary limitation, as the
>> current assumption that only vcpus can dirty memory doesn't hold at
>> all.
> 
> The initial dirty ring proposal has a per-vm ring, but after we
> investigated x86 we found that all legal dirty paths are with a vcpu
> context (except one outlier on kvmgt which fixed within itself), so we
> dropped the per-vm ring.
> 
> One thing to mention is that DMAs should not count in this case because
> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> should be reported to the device driver that interacts with the userspace
> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
> 

Thanks to Peter for mentioning the per-vm ring's history. As I said above,
lets use bitmap instead if all of us agree.

If I'm correct, Marc may be talking about SMMU, which is emulated in host
instead of QEMU. In this case, the DMA target pages are similar to those
pages for vgic/its tables. Both sets of pages are invisible from QEMU.

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Peter and Marc,

On 9/29/22 7:50 PM, Gavin Shan wrote:
> On 9/29/22 12:52 AM, Peter Xu wrote:
>> On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
>>> On Wed, 28 Sep 2022 00:47:43 +0100,
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>>> I have rough idea as below. It's appreciated if you can comment before I'm
>>>> going a head for the prototype. The overall idea is to introduce another
>>>> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
>>>> to dirty ring for vcpu (vcpu-dirty-ring).
>>>>
>>>>     - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
>>>>       entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
>>>>       those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
>>>>
>>>>     - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
>>>>       mmap(kvm->fd). However, there won't have similar reset interface. It means
>>>>       'struct kvm_dirty_gfn::flags' won't track any information as we do for
>>>>       vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
>>>>       advertise 'always-dirty' pages from host to userspace.
>>>>          - For QEMU, shutdown/suspend/resume cases won't be concerning
>>>> us any more. The
>>>>       only concerned case is migration. When the migration is about to complete,
>>>>       kvm-dirty-ring entries are fetched and the dirty bits are updated to global
>>>>       dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
>>>>       the code to find the best spot to do it.
>>>
>>> I think it makes a lot of sense to have a way to log writes that are
>>> not generated by a vpcu, such as the GIC and maybe other things in the
>>> future, such as DMA traffic (some SMMUs are able to track dirty pages
>>> as well).
>>>
>>> However, I don't really see the point in inventing a new mechanism for
>>> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
>>> the dirty *bitmap*?
>>>
>>>  From a kernel perspective, this is dead easy:
>>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 5b064dbadaf4..ae9138f29d51 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>       struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>>>   #ifdef CONFIG_HAVE_KVM_DIRTY_RING
>>> -    if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>>> +    if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>>           return;
>>>   #endif
>>> @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>           unsigned long rel_gfn = gfn - memslot->base_gfn;
>>>           u32 slot = (memslot->as_id << 16) | memslot->id;
>>> -        if (kvm->dirty_ring_size)
>>> +        if (vpcu && kvm->dirty_ring_size)
>>>               kvm_dirty_ring_push(&vcpu->dirty_ring,
>>>                           slot, rel_gfn);
>>> -        else
>>> +        /* non-vpcu dirtying ends up in the global bitmap */
>>> +        if (!vcpu && memslot->dirty_bitmap)
>>>               set_bit_le(rel_gfn, memslot->dirty_bitmap);
>>>       }
>>>   }
>>>
>>> though I'm sure there is a few more things to it.
>>
>> Yes, currently the bitmaps are not created when rings are enabled.
>> kvm_prepare_memory_region() has:
>>
>>         else if (!kvm->dirty_ring_size) {
>>             r = kvm_alloc_dirty_bitmap(new);
>>
>> But I think maybe that's a solution worth considering.  Using the rings
>> have a major challenge on the limitation of ring size, so that for e.g. an
>> ioctl we need to make sure the pages to dirty within an ioctl procedure
>> will not be more than the ring can take.  Using dirty bitmap for a last
>> phase sync of constant (but still very small amount of) dirty pages does
>> sound reasonable and can avoid that complexity.  The payoff is we'll need
>> to allocate both the rings and the bitmaps.
>>
> 
> Ok. I was thinking of using the bitmap to convey the dirty pages for
> this particular case, where we don't have running vcpu. The concern I had
> is the natural difference between a ring and bitmap. The ring-buffer is
> discrete, comparing to bitmap. Besides, it sounds a little strange to
> have two different sets of meta-data to track the data (dirty pages).
> 
> However, bitmap is easier way than per-vm ring. The constrains with
> per-vm ring is just as Peter pointed. So lets reuse the bitmap to
> convey the dirty pages for this particular case. I think the payoff,
> extra bitmap, is acceptable. For this, we need another capability
> (KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
> bitmap in the last phase of migration.
> 
> If all of us agree on this, I can send another kernel patch to address
> this. QEMU still need more patches so that the feature can be supported.
> 

I've had the following PATCH[v5 3/7] to reuse bitmap for these particular
cases. KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG ioctls are used to visit
the bitmap. The new capability is advertised by KVM_CAP_DIRTY_LOG_RING_BITMAP.
Note those two ioctls are disabled when dirty-ring is enabled, we need to
enable them accordingly.

    PATCH[v5 3/7] KVM: x86: Use bitmap in ring-based dirty page tracking

I would like to post v5 after someone reviews or acks kvm/selftests part
of this series.

[...]

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Thu, Sep 29, 2022 at 07:50:12PM +1000, Gavin Shan wrote:
> Hi Marc and Peter,
> 
> On 9/29/22 12:52 AM, Peter Xu wrote:
> > On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
> > > On Wed, 28 Sep 2022 00:47:43 +0100,
> > > Gavin Shan <gshan@redhat.com> wrote:
> > > 
> > > > I have rough idea as below. It's appreciated if you can comment before I'm
> > > > going a head for the prototype. The overall idea is to introduce another
> > > > dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
> > > > to dirty ring for vcpu (vcpu-dirty-ring).
> > > > 
> > > >     - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
> > > >       entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
> > > >       those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
> > > > 
> > > >     - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
> > > >       mmap(kvm->fd). However, there won't have similar reset interface. It means
> > > >       'struct kvm_dirty_gfn::flags' won't track any information as we do for
> > > >       vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
> > > >       advertise 'always-dirty' pages from host to userspace.
> > > >          - For QEMU, shutdown/suspend/resume cases won't be concerning
> > > > us any more. The
> > > >       only concerned case is migration. When the migration is about to complete,
> > > >       kvm-dirty-ring entries are fetched and the dirty bits are updated to global
> > > >       dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
> > > >       the code to find the best spot to do it.
> > > 
> > > I think it makes a lot of sense to have a way to log writes that are
> > > not generated by a vpcu, such as the GIC and maybe other things in the
> > > future, such as DMA traffic (some SMMUs are able to track dirty pages
> > > as well).
> > > 
> > > However, I don't really see the point in inventing a new mechanism for
> > > that. Why don't we simply allow non-vpcu dirty pages to be tracked in
> > > the dirty *bitmap*?
> > > 
> > >  From a kernel perspective, this is dead easy:
> > > 
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 5b064dbadaf4..ae9138f29d51 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > >   	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> > >   #ifdef CONFIG_HAVE_KVM_DIRTY_RING
> > > -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
> > > +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
> > >   		return;
> > >   #endif
> > > @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> > >   		unsigned long rel_gfn = gfn - memslot->base_gfn;
> > >   		u32 slot = (memslot->as_id << 16) | memslot->id;
> > > -		if (kvm->dirty_ring_size)
> > > +		if (vpcu && kvm->dirty_ring_size)
> > >   			kvm_dirty_ring_push(&vcpu->dirty_ring,
> > >   					    slot, rel_gfn);
> > > -		else
> > > +		/* non-vpcu dirtying ends up in the global bitmap */
> > > +		if (!vcpu && memslot->dirty_bitmap)
> > >   			set_bit_le(rel_gfn, memslot->dirty_bitmap);
> > >   	}
> > >   }
> > > 
> > > though I'm sure there is a few more things to it.
> > 
> > Yes, currently the bitmaps are not created when rings are enabled.
> > kvm_prepare_memory_region() has:
> > 
> > 		else if (!kvm->dirty_ring_size) {
> > 			r = kvm_alloc_dirty_bitmap(new);
> > 
> > But I think maybe that's a solution worth considering.  Using the rings
> > have a major challenge on the limitation of ring size, so that for e.g. an
> > ioctl we need to make sure the pages to dirty within an ioctl procedure
> > will not be more than the ring can take.  Using dirty bitmap for a last
> > phase sync of constant (but still very small amount of) dirty pages does
> > sound reasonable and can avoid that complexity.  The payoff is we'll need
> > to allocate both the rings and the bitmaps.
> > 
> 
> Ok. I was thinking of using the bitmap to convey the dirty pages for
> this particular case, where we don't have running vcpu. The concern I had
> is the natural difference between a ring and bitmap. The ring-buffer is
> discrete, comparing to bitmap. Besides, it sounds a little strange to
> have two different sets of meta-data to track the data (dirty pages).

IMHO it's still important to investigate all potential users of the dirty
bitmap, like the GIC case.  Because we still want to know how many pages
can be dirtied in maximum that is to be dirtied (even without being able to
know which pages are dirtied from QEMU - or we don't need the bitmap but we
can set them in QEMU dirty bitmaps directly).

The thing is if we will only sync the dirty bitmap during the downtime, it
means there cannot have a large chunk of pages or the downtime is
unpredictable - QEMU will have its own way to calculate the downtime and
predict the best timing to stop the VM on src, however that's happened
before generating these dirty pages on the bitmap at least for GIC case..
So they need to be in small amount.

> 
> However, bitmap is easier way than per-vm ring. The constrains with
> per-vm ring is just as Peter pointed. So lets reuse the bitmap to
> convey the dirty pages for this particular case. I think the payoff,
> extra bitmap, is acceptable. For this, we need another capability
> (KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
> bitmap in the last phase of migration.

Sounds reasonable.  We need to make sure that cap:

  - Be enabled before any vcpu creation (just like the ring cap)
  - Can be optional for ring, so e.g. x86 can skip both the last phase sync
    and also creation of the dirty bmap in kernel

> 
> If all of us agree on this, I can send another kernel patch to address
> this. QEMU still need more patches so that the feature can be supported.

Sure.  We can also wait for a few days to see whether Paolo has any input.

> 
> > > 
> > > To me, this is just a relaxation of an arbitrary limitation, as the
> > > current assumption that only vcpus can dirty memory doesn't hold at
> > > all.
> > 
> > The initial dirty ring proposal has a per-vm ring, but after we
> > investigated x86 we found that all legal dirty paths are with a vcpu
> > context (except one outlier on kvmgt which fixed within itself), so we
> > dropped the per-vm ring.
> > 
> > One thing to mention is that DMAs should not count in this case because
> > that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> > should be reported to the device driver that interacts with the userspace
> > not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> > includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
> > 
> 
> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
> lets use bitmap instead if all of us agree.
> 
> If I'm correct, Marc may be talking about SMMU, which is emulated in host
> instead of QEMU. In this case, the DMA target pages are similar to those
> pages for vgic/its tables. Both sets of pages are invisible from QEMU.

OK, I'm not aware the SMMU can also be emulated in the host kernel, thanks
Gavin.  If so then we really need to think as I mentioned above since we
will only sync this when switchover, and if the DMA range can cover a large
portion of guest mem (assuming the physical pages to DMA can be randomly
allocated by guest device drivers) then it may be another problem to cause
drastic downtime.

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Wed, 28 Sep 2022 15:52:00 +0100,
Peter Xu <peterx@redhat.com> wrote:
> 
> On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
> > Hi Gavin,
> > 
> > On Wed, 28 Sep 2022 00:47:43 +0100,
> > Gavin Shan <gshan@redhat.com> wrote:
> > 
> > > I have rough idea as below. It's appreciated if you can comment before I'm
> > > going a head for the prototype. The overall idea is to introduce another
> > > dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
> > > to dirty ring for vcpu (vcpu-dirty-ring).
> > > 
> > >    - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
> > >      entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
> > >      those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
> > > 
> > >    - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
> > >      mmap(kvm->fd). However, there won't have similar reset interface. It means
> > >      'struct kvm_dirty_gfn::flags' won't track any information as we do for
> > >      vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
> > >      advertise 'always-dirty' pages from host to userspace.
> > >         - For QEMU, shutdown/suspend/resume cases won't be concerning
> > > us any more. The
> > >      only concerned case is migration. When the migration is about to complete,
> > >      kvm-dirty-ring entries are fetched and the dirty bits are updated to global
> > >      dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
> > >      the code to find the best spot to do it.
> > 
> > I think it makes a lot of sense to have a way to log writes that are
> > not generated by a vpcu, such as the GIC and maybe other things in the
> > future, such as DMA traffic (some SMMUs are able to track dirty pages
> > as well).
> > 
> > However, I don't really see the point in inventing a new mechanism for
> > that. Why don't we simply allow non-vpcu dirty pages to be tracked in
> > the dirty *bitmap*?
> > 
> > From a kernel perspective, this is dead easy:
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 5b064dbadaf4..ae9138f29d51 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> >  	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> >  
> >  #ifdef CONFIG_HAVE_KVM_DIRTY_RING
> > -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
> > +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
> >  		return;
> >  #endif
> >  
> > @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> >  		unsigned long rel_gfn = gfn - memslot->base_gfn;
> >  		u32 slot = (memslot->as_id << 16) | memslot->id;
> >  
> > -		if (kvm->dirty_ring_size)
> > +		if (vpcu && kvm->dirty_ring_size)
> >  			kvm_dirty_ring_push(&vcpu->dirty_ring,
> >  					    slot, rel_gfn);
> > -		else
> > +		/* non-vpcu dirtying ends up in the global bitmap */
> > +		if (!vcpu && memslot->dirty_bitmap)
> >  			set_bit_le(rel_gfn, memslot->dirty_bitmap);
> >  	}
> >  }
> > 
> > though I'm sure there is a few more things to it.
> 
> Yes, currently the bitmaps are not created when rings are enabled.
> kvm_prepare_memory_region() has:
> 
> 		else if (!kvm->dirty_ring_size) {
> 			r = kvm_alloc_dirty_bitmap(new);
> 
> But I think maybe that's a solution worth considering.  Using the rings
> have a major challenge on the limitation of ring size, so that for e.g. an
> ioctl we need to make sure the pages to dirty within an ioctl procedure
> will not be more than the ring can take.  Using dirty bitmap for a last
> phase sync of constant (but still very small amount of) dirty pages does
> sound reasonable and can avoid that complexity.  The payoff is we'll need
> to allocate both the rings and the bitmaps.

I think that's a reasonable price to pay.

> 
> > 
> > To me, this is just a relaxation of an arbitrary limitation, as the
> > current assumption that only vcpus can dirty memory doesn't hold at
> > all.
> 
> The initial dirty ring proposal has a per-vm ring, but after we
> investigated x86 we found that all legal dirty paths are with a vcpu
> context (except one outlier on kvmgt which fixed within itself), so we
> dropped the per-vm ring.
> 
> One thing to mention is that DMAs should not count in this case because
> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> should be reported to the device driver that interacts with the userspace
> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).

I beg to disagree. How do you make this work once we share the CPU
page tables with the SMMU, meaning that there is only one true source
of dirty bits? You can't separate the two.

Which is another reason why the dirty-ring approach is pretty limited
on ARM: it is an artificial construct that doesn't really work once we
get to this point.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Thu, 29 Sep 2022 10:50:12 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> Hi Marc and Peter,
> 
> On 9/29/22 12:52 AM, Peter Xu wrote:
> > On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
> >> On Wed, 28 Sep 2022 00:47:43 +0100,
> >> Gavin Shan <gshan@redhat.com> wrote:
> >> 
> >>> I have rough idea as below. It's appreciated if you can comment before I'm
> >>> going a head for the prototype. The overall idea is to introduce another
> >>> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
> >>> to dirty ring for vcpu (vcpu-dirty-ring).
> >>> 
> >>>     - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
> >>>       entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
> >>>       those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
> >>> 
> >>>     - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
> >>>       mmap(kvm->fd). However, there won't have similar reset interface. It means
> >>>       'struct kvm_dirty_gfn::flags' won't track any information as we do for
> >>>       vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
> >>>       advertise 'always-dirty' pages from host to userspace.
> >>>          - For QEMU, shutdown/suspend/resume cases won't be concerning
> >>> us any more. The
> >>>       only concerned case is migration. When the migration is about to complete,
> >>>       kvm-dirty-ring entries are fetched and the dirty bits are updated to global
> >>>       dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
> >>>       the code to find the best spot to do it.
> >> 
> >> I think it makes a lot of sense to have a way to log writes that are
> >> not generated by a vpcu, such as the GIC and maybe other things in the
> >> future, such as DMA traffic (some SMMUs are able to track dirty pages
> >> as well).
> >> 
> >> However, I don't really see the point in inventing a new mechanism for
> >> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
> >> the dirty *bitmap*?
> >> 
> >>  From a kernel perspective, this is dead easy:
> >> 
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 5b064dbadaf4..ae9138f29d51 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
> >>   	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> >>     #ifdef CONFIG_HAVE_KVM_DIRTY_RING
> >> -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
> >> +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
> >>   		return;
> >>   #endif
> >>   @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm
> >> *kvm,
> >>   		unsigned long rel_gfn = gfn - memslot->base_gfn;
> >>   		u32 slot = (memslot->as_id << 16) | memslot->id;
> >>   -		if (kvm->dirty_ring_size)
> >> +		if (vpcu && kvm->dirty_ring_size)
> >>   			kvm_dirty_ring_push(&vcpu->dirty_ring,
> >>   					    slot, rel_gfn);
> >> -		else
> >> +		/* non-vpcu dirtying ends up in the global bitmap */
> >> +		if (!vcpu && memslot->dirty_bitmap)
> >>   			set_bit_le(rel_gfn, memslot->dirty_bitmap);
> >>   	}
> >>   }
> >> 
> >> though I'm sure there is a few more things to it.
> > 
> > Yes, currently the bitmaps are not created when rings are enabled.
> > kvm_prepare_memory_region() has:
> > 
> > 		else if (!kvm->dirty_ring_size) {
> > 			r = kvm_alloc_dirty_bitmap(new);
> > 
> > But I think maybe that's a solution worth considering.  Using the rings
> > have a major challenge on the limitation of ring size, so that for e.g. an
> > ioctl we need to make sure the pages to dirty within an ioctl procedure
> > will not be more than the ring can take.  Using dirty bitmap for a last
> > phase sync of constant (but still very small amount of) dirty pages does
> > sound reasonable and can avoid that complexity.  The payoff is we'll need
> > to allocate both the rings and the bitmaps.
> > 
> 
> Ok. I was thinking of using the bitmap to convey the dirty pages for
> this particular case, where we don't have running vcpu. The concern I had
> is the natural difference between a ring and bitmap. The ring-buffer is
> discrete, comparing to bitmap. Besides, it sounds a little strange to
> have two different sets of meta-data to track the data (dirty pages).

The problem is that the dirty ring mechanism is a bit blinkered, and
cannot consider a source of dirty pages other than from the vcpus.

> However, bitmap is easier way than per-vm ring. The constrains with
> per-vm ring is just as Peter pointed. So lets reuse the bitmap to
> convey the dirty pages for this particular case. I think the payoff,
> extra bitmap, is acceptable. For this, we need another capability
> (KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
> bitmap in the last phase of migration.

Why another capability? Just allowing dirty logging to be enabled
before we saving the GIC state should be enough, shouldn't it?

> If all of us agree on this, I can send another kernel patch to address
> this. QEMU still need more patches so that the feature can be supported.

Yes, this will also need some work.

> >> 
> >> To me, this is just a relaxation of an arbitrary limitation, as the
> >> current assumption that only vcpus can dirty memory doesn't hold at
> >> all.
> > 
> > The initial dirty ring proposal has a per-vm ring, but after we
> > investigated x86 we found that all legal dirty paths are with a vcpu
> > context (except one outlier on kvmgt which fixed within itself), so we
> > dropped the per-vm ring.
> > 
> > One thing to mention is that DMAs should not count in this case because
> > that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> > should be reported to the device driver that interacts with the userspace
> > not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> > includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
> > 
> 
> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
> lets use bitmap instead if all of us agree.
> 
> If I'm correct, Marc may be talking about SMMU, which is emulated in host
> instead of QEMU. In this case, the DMA target pages are similar to those
> pages for vgic/its tables. Both sets of pages are invisible from QEMU.

No, I'm talking about an actual HW SMMU using the HTTU feature that
set the Dirty bit in the PTEs. And people have been working on sharing
SMMU and CPU PTs for some time, which would give us the one true
source of dirty page.

In this configuration, the dirty ring mechanism will be pretty useless.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Thu, 29 Sep 2022 12:31:34 +0100,
Gavin Shan <gshan@redhat.com> wrote:
> 
> I've had the following PATCH[v5 3/7] to reuse bitmap for these particular
> cases. KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG ioctls are used to visit
> the bitmap. The new capability is advertised by KVM_CAP_DIRTY_LOG_RING_BITMAP.
> Note those two ioctls are disabled when dirty-ring is enabled, we need to
> enable them accordingly.
> 
>    PATCH[v5 3/7] KVM: x86: Use bitmap in ring-based dirty page tracking
> 
> I would like to post v5 after someone reviews or acks kvm/selftests part
> of this series.

Feel free to post the series. This is too late for 6.1 anyway (I'll
probably send the PR to Paolo tomorrow), so this gives us plenty of
time to sort this out.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Thu, 29 Sep 2022 15:32:03 +0100,
Peter Xu <peterx@redhat.com> wrote:
> 
> > If I'm correct, Marc may be talking about SMMU, which is emulated in host
> > instead of QEMU. In this case, the DMA target pages are similar to those
> > pages for vgic/its tables. Both sets of pages are invisible from QEMU.
> 
> OK, I'm not aware the SMMU can also be emulated in the host kernel, thanks
> Gavin.  If so then we really need to think as I mentioned above since we
> will only sync this when switchover, and if the DMA range can cover a large
> portion of guest mem (assuming the physical pages to DMA can be randomly
> allocated by guest device drivers) then it may be another problem to cause
> drastic downtime.

I don't know where this is coming from. The kernel has no SMMU
emulation at all.

	  M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Marc,

On 9/29/22 10:42 PM, Marc Zyngier wrote:
> On Thu, 29 Sep 2022 10:50:12 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
>> On 9/29/22 12:52 AM, Peter Xu wrote:
>>> On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
>>>> On Wed, 28 Sep 2022 00:47:43 +0100,
>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>
>>>>> I have rough idea as below. It's appreciated if you can comment before I'm
>>>>> going a head for the prototype. The overall idea is to introduce another
>>>>> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
>>>>> to dirty ring for vcpu (vcpu-dirty-ring).
>>>>>
>>>>>      - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
>>>>>        entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
>>>>>        those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
>>>>>
>>>>>      - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
>>>>>        mmap(kvm->fd). However, there won't have similar reset interface. It means
>>>>>        'struct kvm_dirty_gfn::flags' won't track any information as we do for
>>>>>        vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
>>>>>        advertise 'always-dirty' pages from host to userspace.
>>>>>           - For QEMU, shutdown/suspend/resume cases won't be concerning
>>>>> us any more. The
>>>>>        only concerned case is migration. When the migration is about to complete,
>>>>>        kvm-dirty-ring entries are fetched and the dirty bits are updated to global
>>>>>        dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
>>>>>        the code to find the best spot to do it.
>>>>
>>>> I think it makes a lot of sense to have a way to log writes that are
>>>> not generated by a vpcu, such as the GIC and maybe other things in the
>>>> future, such as DMA traffic (some SMMUs are able to track dirty pages
>>>> as well).
>>>>
>>>> However, I don't really see the point in inventing a new mechanism for
>>>> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
>>>> the dirty *bitmap*?
>>>>
>>>>   From a kernel perspective, this is dead easy:
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 5b064dbadaf4..ae9138f29d51 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>>    	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>>>>      #ifdef CONFIG_HAVE_KVM_DIRTY_RING
>>>> -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>>>> +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>>>    		return;
>>>>    #endif
>>>>    @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm
>>>> *kvm,
>>>>    		unsigned long rel_gfn = gfn - memslot->base_gfn;
>>>>    		u32 slot = (memslot->as_id << 16) | memslot->id;
>>>>    -		if (kvm->dirty_ring_size)
>>>> +		if (vpcu && kvm->dirty_ring_size)
>>>>    			kvm_dirty_ring_push(&vcpu->dirty_ring,
>>>>    					    slot, rel_gfn);
>>>> -		else
>>>> +		/* non-vpcu dirtying ends up in the global bitmap */
>>>> +		if (!vcpu && memslot->dirty_bitmap)
>>>>    			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>>>>    	}
>>>>    }
>>>>
>>>> though I'm sure there is a few more things to it.
>>>
>>> Yes, currently the bitmaps are not created when rings are enabled.
>>> kvm_prepare_memory_region() has:
>>>
>>> 		else if (!kvm->dirty_ring_size) {
>>> 			r = kvm_alloc_dirty_bitmap(new);
>>>
>>> But I think maybe that's a solution worth considering.  Using the rings
>>> have a major challenge on the limitation of ring size, so that for e.g. an
>>> ioctl we need to make sure the pages to dirty within an ioctl procedure
>>> will not be more than the ring can take.  Using dirty bitmap for a last
>>> phase sync of constant (but still very small amount of) dirty pages does
>>> sound reasonable and can avoid that complexity.  The payoff is we'll need
>>> to allocate both the rings and the bitmaps.
>>>
>>
>> Ok. I was thinking of using the bitmap to convey the dirty pages for
>> this particular case, where we don't have running vcpu. The concern I had
>> is the natural difference between a ring and bitmap. The ring-buffer is
>> discrete, comparing to bitmap. Besides, it sounds a little strange to
>> have two different sets of meta-data to track the data (dirty pages).
> 
> The problem is that the dirty ring mechanism is a bit blinkered, and
> cannot consider a source of dirty pages other than from the vcpus.
> 

Ok.

>> However, bitmap is easier way than per-vm ring. The constrains with
>> per-vm ring is just as Peter pointed. So lets reuse the bitmap to
>> convey the dirty pages for this particular case. I think the payoff,
>> extra bitmap, is acceptable. For this, we need another capability
>> (KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
>> bitmap in the last phase of migration.
> 
> Why another capability? Just allowing dirty logging to be enabled
> before we saving the GIC state should be enough, shouldn't it?
> 

The GIC state would be just one case where no vcpu can be used to push
dirty page information. As you mentioned, SMMMU HTTU feature could possibly
be another case to ARM64. It's uncertain about other architectures where
dirty-ring will be supported. In QEMU, the dirty (bitmap) logging is enabled
at the beginning of migration and the bitmap is synchronized to global
dirty bitmap and RAMBlock's dirty bitmap gradually, as the following
backtrace shows. What we need to do for QEMU is probably retrieve the
bitmap at point (A).

Without the new capability, we will have to rely on the return value
from ioctls KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG to detect the
capability. For example, -ENXIO is returned on old kernels.

    migration_thread
      qemu_savevm_state_setup
        ram_save_setup
          ram_init_all
            ram_init_bitmaps
              memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION)   // dirty logging enabled
              migration_bitmap_sync_precopy(rs)
        :
      migration_iteration_run                                         // iteration 0
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
      migration_iteration_run                                        // iteration 1
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
      migration_iteration_run                                        // iteration 2
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
        :
      migration_iteration_run                                       // iteration N
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        migration_completion
          qemu_savevm_state_complete_precopy
            qemu_savevm_state_complete_precopy_iterable
              ram_save_complete
                migration_bitmap_sync_precopy                      // A
                <send all dirty pages>

Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
       in the last synchronization, right after the VM is stopped.

>> If all of us agree on this, I can send another kernel patch to address
>> this. QEMU still need more patches so that the feature can be supported.
> 
> Yes, this will also need some work.
> 
>>>>
>>>> To me, this is just a relaxation of an arbitrary limitation, as the
>>>> current assumption that only vcpus can dirty memory doesn't hold at
>>>> all.
>>>
>>> The initial dirty ring proposal has a per-vm ring, but after we
>>> investigated x86 we found that all legal dirty paths are with a vcpu
>>> context (except one outlier on kvmgt which fixed within itself), so we
>>> dropped the per-vm ring.
>>>
>>> One thing to mention is that DMAs should not count in this case because
>>> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
>>> should be reported to the device driver that interacts with the userspace
>>> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
>>> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
>>>
>>
>> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
>> lets use bitmap instead if all of us agree.
>>
>> If I'm correct, Marc may be talking about SMMU, which is emulated in host
>> instead of QEMU. In this case, the DMA target pages are similar to those
>> pages for vgic/its tables. Both sets of pages are invisible from QEMU.
> 
> No, I'm talking about an actual HW SMMU using the HTTU feature that
> set the Dirty bit in the PTEs. And people have been working on sharing
> SMMU and CPU PTs for some time, which would give us the one true
> source of dirty page.
> 
> In this configuration, the dirty ring mechanism will be pretty useless.
> 

Ok. I don't know the details. Marc, the dirty bitmap is helpful in this case?

Thanks,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Marc,

On 9/29/22 10:42 PM, Marc Zyngier wrote:
> On Thu, 29 Sep 2022 10:50:12 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
>> On 9/29/22 12:52 AM, Peter Xu wrote:
>>> On Wed, Sep 28, 2022 at 09:25:34AM +0100, Marc Zyngier wrote:
>>>> On Wed, 28 Sep 2022 00:47:43 +0100,
>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>
>>>>> I have rough idea as below. It's appreciated if you can comment before I'm
>>>>> going a head for the prototype. The overall idea is to introduce another
>>>>> dirty ring for KVM (kvm-dirty-ring). It's updated and visited separately
>>>>> to dirty ring for vcpu (vcpu-dirty-ring).
>>>>>
>>>>>      - When the various VGIC/ITS table base addresses are specified, kvm-dirty-ring
>>>>>        entries are added to mark those pages as 'always-dirty'. In mark_page_dirty_in_slot(),
>>>>>        those 'always-dirty' pages will be skipped, no entries pushed to vcpu-dirty-ring.
>>>>>
>>>>>      - Similar to vcpu-dirty-ring, kvm-dirty-ring is accessed from userspace through
>>>>>        mmap(kvm->fd). However, there won't have similar reset interface. It means
>>>>>        'struct kvm_dirty_gfn::flags' won't track any information as we do for
>>>>>        vcpu-dirty-ring. In this regard, kvm-dirty-ring is purely shared buffer to
>>>>>        advertise 'always-dirty' pages from host to userspace.
>>>>>           - For QEMU, shutdown/suspend/resume cases won't be concerning
>>>>> us any more. The
>>>>>        only concerned case is migration. When the migration is about to complete,
>>>>>        kvm-dirty-ring entries are fetched and the dirty bits are updated to global
>>>>>        dirty page bitmap and RAMBlock's dirty page bitmap. For this, I'm still reading
>>>>>        the code to find the best spot to do it.
>>>>
>>>> I think it makes a lot of sense to have a way to log writes that are
>>>> not generated by a vpcu, such as the GIC and maybe other things in the
>>>> future, such as DMA traffic (some SMMUs are able to track dirty pages
>>>> as well).
>>>>
>>>> However, I don't really see the point in inventing a new mechanism for
>>>> that. Why don't we simply allow non-vpcu dirty pages to be tracked in
>>>> the dirty *bitmap*?
>>>>
>>>>   From a kernel perspective, this is dead easy:
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 5b064dbadaf4..ae9138f29d51 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -3305,7 +3305,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>>    	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>>>>      #ifdef CONFIG_HAVE_KVM_DIRTY_RING
>>>> -	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>>>> +	if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>>>    		return;
>>>>    #endif
>>>>    @@ -3313,10 +3313,11 @@ void mark_page_dirty_in_slot(struct kvm
>>>> *kvm,
>>>>    		unsigned long rel_gfn = gfn - memslot->base_gfn;
>>>>    		u32 slot = (memslot->as_id << 16) | memslot->id;
>>>>    -		if (kvm->dirty_ring_size)
>>>> +		if (vpcu && kvm->dirty_ring_size)
>>>>    			kvm_dirty_ring_push(&vcpu->dirty_ring,
>>>>    					    slot, rel_gfn);
>>>> -		else
>>>> +		/* non-vpcu dirtying ends up in the global bitmap */
>>>> +		if (!vcpu && memslot->dirty_bitmap)
>>>>    			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>>>>    	}
>>>>    }
>>>>
>>>> though I'm sure there is a few more things to it.
>>>
>>> Yes, currently the bitmaps are not created when rings are enabled.
>>> kvm_prepare_memory_region() has:
>>>
>>> 		else if (!kvm->dirty_ring_size) {
>>> 			r = kvm_alloc_dirty_bitmap(new);
>>>
>>> But I think maybe that's a solution worth considering.  Using the rings
>>> have a major challenge on the limitation of ring size, so that for e.g. an
>>> ioctl we need to make sure the pages to dirty within an ioctl procedure
>>> will not be more than the ring can take.  Using dirty bitmap for a last
>>> phase sync of constant (but still very small amount of) dirty pages does
>>> sound reasonable and can avoid that complexity.  The payoff is we'll need
>>> to allocate both the rings and the bitmaps.
>>>
>>
>> Ok. I was thinking of using the bitmap to convey the dirty pages for
>> this particular case, where we don't have running vcpu. The concern I had
>> is the natural difference between a ring and bitmap. The ring-buffer is
>> discrete, comparing to bitmap. Besides, it sounds a little strange to
>> have two different sets of meta-data to track the data (dirty pages).
> 
> The problem is that the dirty ring mechanism is a bit blinkered, and
> cannot consider a source of dirty pages other than from the vcpus.
> 

Ok.

>> However, bitmap is easier way than per-vm ring. The constrains with
>> per-vm ring is just as Peter pointed. So lets reuse the bitmap to
>> convey the dirty pages for this particular case. I think the payoff,
>> extra bitmap, is acceptable. For this, we need another capability
>> (KVM_CAP_DIRTY_LOG_RING_BITMAP?) so that QEMU can collects the dirty
>> bitmap in the last phase of migration.
> 
> Why another capability? Just allowing dirty logging to be enabled
> before we saving the GIC state should be enough, shouldn't it?
> 

The GIC state would be just one case where no vcpu can be used to push
dirty page information. As you mentioned, SMMMU HTTU feature could possibly
be another case to ARM64. It's uncertain about other architectures where
dirty-ring will be supported. In QEMU, the dirty (bitmap) logging is enabled
at the beginning of migration and the bitmap is synchronized to global
dirty bitmap and RAMBlock's dirty bitmap gradually, as the following
backtrace shows. What we need to do for QEMU is probably retrieve the
bitmap at point (A).

Without the new capability, we will have to rely on the return value
from ioctls KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG to detect the
capability. For example, -ENXIO is returned on old kernels.

    migration_thread
      qemu_savevm_state_setup
        ram_save_setup
          ram_init_all
            ram_init_bitmaps
              memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION)   // dirty logging enabled
              migration_bitmap_sync_precopy(rs)
        :
      migration_iteration_run                                         // iteration 0
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
      migration_iteration_run                                        // iteration 1
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
      migration_iteration_run                                        // iteration 2
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        qemu_savevm_state_iterate
          ram_save_iterate
        :
      migration_iteration_run                                       // iteration N
        qemu_savevm_state_pending
          migration_bitmap_sync_precopy
        migration_completion
          qemu_savevm_state_complete_precopy
            qemu_savevm_state_complete_precopy_iterable
              ram_save_complete
                migration_bitmap_sync_precopy                      // A
                <send all dirty pages>

Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
       in the last synchronization, right after the VM is stopped.

>> If all of us agree on this, I can send another kernel patch to address
>> this. QEMU still need more patches so that the feature can be supported.
> 
> Yes, this will also need some work.
> 
>>>>
>>>> To me, this is just a relaxation of an arbitrary limitation, as the
>>>> current assumption that only vcpus can dirty memory doesn't hold at
>>>> all.
>>>
>>> The initial dirty ring proposal has a per-vm ring, but after we
>>> investigated x86 we found that all legal dirty paths are with a vcpu
>>> context (except one outlier on kvmgt which fixed within itself), so we
>>> dropped the per-vm ring.
>>>
>>> One thing to mention is that DMAs should not count in this case because
>>> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
>>> should be reported to the device driver that interacts with the userspace
>>> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
>>> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
>>>
>>
>> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
>> lets use bitmap instead if all of us agree.
>>
>> If I'm correct, Marc may be talking about SMMU, which is emulated in host
>> instead of QEMU. In this case, the DMA target pages are similar to those
>> pages for vgic/its tables. Both sets of pages are invisible from QEMU.
> 
> No, I'm talking about an actual HW SMMU using the HTTU feature that
> set the Dirty bit in the PTEs. And people have been working on sharing
> SMMU and CPU PTs for some time, which would give us the one true
> source of dirty page.
> 
> In this configuration, the dirty ring mechanism will be pretty useless.
> 

Ok. I don't know the details. Marc, the dirty bitmap is helpful in this case?

Thanks,
Gavin

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Tue, Oct 04, 2022 at 12:26:23PM +0800, Gavin Shan wrote:
> Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
>       in the last synchronization, right after the VM is stopped.

Agreed on postcopy.  Note that snapshot doesn't use kvm dirty logging
because it requires synchronous events (userfaultfd), so that's not
affected by this work.  Thanks,

-- 
Peter Xu

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Peter Xu]

On Tue, Oct 04, 2022 at 12:26:23PM +0800, Gavin Shan wrote:
> Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
>       in the last synchronization, right after the VM is stopped.

Agreed on postcopy.  Note that snapshot doesn't use kvm dirty logging
because it requires synchronous events (userfaultfd), so that's not
affected by this work.  Thanks,

-- 
Peter Xu

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Tue, 04 Oct 2022 05:26:23 +0100,
Gavin Shan <gshan@redhat.com> wrote:

[...]

> > Why another capability? Just allowing dirty logging to be enabled
> > before we saving the GIC state should be enough, shouldn't it?
> > 
> 
> The GIC state would be just one case where no vcpu can be used to push
> dirty page information. As you mentioned, SMMMU HTTU feature could possibly
> be another case to ARM64. It's uncertain about other architectures where
> dirty-ring will be supported. In QEMU, the dirty (bitmap) logging is enabled
> at the beginning of migration and the bitmap is synchronized to global
> dirty bitmap and RAMBlock's dirty bitmap gradually, as the following
> backtrace shows. What we need to do for QEMU is probably retrieve the
> bitmap at point (A).
> 
> Without the new capability, we will have to rely on the return value
> from ioctls KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG to detect the
> capability. For example, -ENXIO is returned on old kernels.

Huh. Fair enough.

KVM_CAP_ALLOW_DIRTY_LOG_AND_DIRTY_RING_TOGETHER_UNTIL_THE_NEXT_TIME...


> 
>    migration_thread
>      qemu_savevm_state_setup
>        ram_save_setup
>          ram_init_all
>            ram_init_bitmaps
>              memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION)   // dirty logging enabled
>              migration_bitmap_sync_precopy(rs)
>        :
>      migration_iteration_run                                         // iteration 0
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>      migration_iteration_run                                        // iteration 1
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>      migration_iteration_run                                        // iteration 2
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>        :
>      migration_iteration_run                                       // iteration N
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        migration_completion
>          qemu_savevm_state_complete_precopy
>            qemu_savevm_state_complete_precopy_iterable
>              ram_save_complete
>                migration_bitmap_sync_precopy                      // A
>                <send all dirty pages>
> 
> Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
>       in the last synchronization, right after the VM is stopped.

Not only the VM stopped, but also the devices made quiescent.

> >> If all of us agree on this, I can send another kernel patch to address
> >> this. QEMU still need more patches so that the feature can be supported.
> > 
> > Yes, this will also need some work.
> > 
> >>>> 
> >>>> To me, this is just a relaxation of an arbitrary limitation, as the
> >>>> current assumption that only vcpus can dirty memory doesn't hold at
> >>>> all.
> >>> 
> >>> The initial dirty ring proposal has a per-vm ring, but after we
> >>> investigated x86 we found that all legal dirty paths are with a vcpu
> >>> context (except one outlier on kvmgt which fixed within itself), so we
> >>> dropped the per-vm ring.
> >>> 
> >>> One thing to mention is that DMAs should not count in this case because
> >>> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> >>> should be reported to the device driver that interacts with the userspace
> >>> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> >>> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
> >>> 
> >> 
> >> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
> >> lets use bitmap instead if all of us agree.
> >> 
> >> If I'm correct, Marc may be talking about SMMU, which is emulated in host
> >> instead of QEMU. In this case, the DMA target pages are similar to those
> >> pages for vgic/its tables. Both sets of pages are invisible from QEMU.
> > 
> > No, I'm talking about an actual HW SMMU using the HTTU feature that
> > set the Dirty bit in the PTEs. And people have been working on sharing
> > SMMU and CPU PTs for some time, which would give us the one true
> > source of dirty page.
> > 
> > In this configuration, the dirty ring mechanism will be pretty useless.
> > 
> 
> Ok. I don't know the details. Marc, the dirty bitmap is helpful in this case?

Yes, the dirty bitmap is useful if the source of dirty bits is
obtained from the page tables. The cost of collecting/resetting the
bits is pretty high though.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 3/6] KVM: arm64: Enable ring-based dirty memory tracking

[Marc Zyngier]

On Tue, 04 Oct 2022 05:26:23 +0100,
Gavin Shan <gshan@redhat.com> wrote:

[...]

> > Why another capability? Just allowing dirty logging to be enabled
> > before we saving the GIC state should be enough, shouldn't it?
> > 
> 
> The GIC state would be just one case where no vcpu can be used to push
> dirty page information. As you mentioned, SMMMU HTTU feature could possibly
> be another case to ARM64. It's uncertain about other architectures where
> dirty-ring will be supported. In QEMU, the dirty (bitmap) logging is enabled
> at the beginning of migration and the bitmap is synchronized to global
> dirty bitmap and RAMBlock's dirty bitmap gradually, as the following
> backtrace shows. What we need to do for QEMU is probably retrieve the
> bitmap at point (A).
> 
> Without the new capability, we will have to rely on the return value
> from ioctls KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG to detect the
> capability. For example, -ENXIO is returned on old kernels.

Huh. Fair enough.

KVM_CAP_ALLOW_DIRTY_LOG_AND_DIRTY_RING_TOGETHER_UNTIL_THE_NEXT_TIME...


> 
>    migration_thread
>      qemu_savevm_state_setup
>        ram_save_setup
>          ram_init_all
>            ram_init_bitmaps
>              memory_global_dirty_log_start(GLOBAL_DIRTY_MIGRATION)   // dirty logging enabled
>              migration_bitmap_sync_precopy(rs)
>        :
>      migration_iteration_run                                         // iteration 0
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>      migration_iteration_run                                        // iteration 1
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>      migration_iteration_run                                        // iteration 2
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        qemu_savevm_state_iterate
>          ram_save_iterate
>        :
>      migration_iteration_run                                       // iteration N
>        qemu_savevm_state_pending
>          migration_bitmap_sync_precopy
>        migration_completion
>          qemu_savevm_state_complete_precopy
>            qemu_savevm_state_complete_precopy_iterable
>              ram_save_complete
>                migration_bitmap_sync_precopy                      // A
>                <send all dirty pages>
> 
> Note: for post-copy and snapshot, I assume we need to save the dirty bitmap
>       in the last synchronization, right after the VM is stopped.

Not only the VM stopped, but also the devices made quiescent.

> >> If all of us agree on this, I can send another kernel patch to address
> >> this. QEMU still need more patches so that the feature can be supported.
> > 
> > Yes, this will also need some work.
> > 
> >>>> 
> >>>> To me, this is just a relaxation of an arbitrary limitation, as the
> >>>> current assumption that only vcpus can dirty memory doesn't hold at
> >>>> all.
> >>> 
> >>> The initial dirty ring proposal has a per-vm ring, but after we
> >>> investigated x86 we found that all legal dirty paths are with a vcpu
> >>> context (except one outlier on kvmgt which fixed within itself), so we
> >>> dropped the per-vm ring.
> >>> 
> >>> One thing to mention is that DMAs should not count in this case because
> >>> that's from device perspective, IOW either IOMMU or SMMU dirty tracking
> >>> should be reported to the device driver that interacts with the userspace
> >>> not from KVM interfaces (e.g. vfio with VFIO_IOMMU_DIRTY_PAGES).  That even
> >>> includes emulated DMA like vhost (VHOST_SET_LOG_BASE).
> >>> 
> >> 
> >> Thanks to Peter for mentioning the per-vm ring's history. As I said above,
> >> lets use bitmap instead if all of us agree.
> >> 
> >> If I'm correct, Marc may be talking about SMMU, which is emulated in host
> >> instead of QEMU. In this case, the DMA target pages are similar to those
> >> pages for vgic/its tables. Both sets of pages are invisible from QEMU.
> > 
> > No, I'm talking about an actual HW SMMU using the HTTU feature that
> > set the Dirty bit in the PTEs. And people have been working on sharing
> > SMMU and CPU PTs for some time, which would give us the one true
> > source of dirty page.
> > 
> > In this configuration, the dirty ring mechanism will be pretty useless.
> > 
> 
> Ok. I don't know the details. Marc, the dirty bitmap is helpful in this case?

Yes, the dirty bitmap is useful if the source of dirty bits is
obtained from the page tables. The cost of collecting/resetting the
bits is pretty high though.

	M.

-- 
Without deviation from the norm, progress is not possible.