[PATCH v10 0/7] KVM: arm64: Enable ring-based dirty memory tracking

[PATCH v10 0/7] 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").

For PATCH[v9 3/7], Peter's ack-by is kept since the recent changes
don't fundamentally break what he agreed. However, it would be nice
for Peter to double confirm.

v9: https://lore.kernel.org/kvmarm/20221108041039.111145-1-gshan@redhat.com/T/
v8: https://lore.kernel.org/kvmarm/e7196fb5-1e65-3a5b-ba88-1d98264110e3@redhat.com/T/#t
v7: https://lore.kernel.org/kvmarm/20221031003621.164306-1-gshan@redhat.com/
v6: https://lore.kernel.org/kvmarm/20221011061447.131531-1-gshan@redhat.com/
v5: https://lore.kernel.org/all/20221005004154.83502-1-gshan@redhat.com/
v4: https://lore.kernel.org/kvmarm/20220927005439.21130-1-gshan@redhat.com/
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 (partially)

Changelog
=========
v10:
  * Replace CONFIG_HAVE_DIRTY_RING_WITH_BITMAP with
    CONFIG_NEED_DIRTY_RING_WITH_BITMAP, improve the comments
    about the option, and reserve cap->flags for future
    needs to disable KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP in
    PATCH[v10 3/7]                                              (Sean)
v9:
  * Improved documentation in PATCH[v9 3/7]                     (Marc/Peter)
  * Add helper to check if we have any memslot and lockdep
    assert is added to kvm_use_dirty_bitmap() in PATCH[v9 3/7]  (Sean/Oliver)
  * Drop the helper to return dist->save_its_tables_in_progress
    and move kvm_arch_allow_write_without_running_vcpu() to
    vgic-its.c in PATCH[v9 4/7]                                 (Marc)
v8:
  * Pick review-by and ack-by                                   (Peter/Sean)
  * Drop chunk of code to clear KVM_REQ_DIRTY_RING_SOFT_FULL
    in kvm_dirty_ring_reset(). Add comments to say the event
    will be cleared by the VCPU thread next time when it enters
    the guest. All other changes related to kvm_dirty_ring_reset()
    are dropped in PATCH[v8 1/7].                               (Sean/Peter/Marc)
  * Drop PATCH[v7 3/7] since it has been merged                 (Marc/Oliver)
  * Document the order of DIRTY_RING_{ACQ_REL, WITH_BITMAP},
    add check to ensure no memslots are created when
    DIRTY_RING_WITH_BITMAP is enabled, and add weak function
    kvm_arch_allow_write_without_running_vcpu() in PATCH[v8 3/7] (Oliver)
  * Only keep ourself out of non-running-vcpu radar when vgic/its
    tables are being saved in PATCH[v8 4/7]                      (Marc/Sean)
v7:
  * Cut down #ifdef, avoid using 'container_of()', move the
    dirty-ring check after KVM_REQ_VM_DEAD, add comments
    for kvm_dirty_ring_check_request(), use tab character
    for KVM event definitions in kvm_host.h in PATCH[v7 01]    (Sean)
  * Add PATCH[v7 03] to recheck if the capability has
    been advertised prior to enable RING/RING_ACEL_REL         (Sean)
  * Improve the description about capability RING_WITH_BITMAP,
    rename kvm_dirty_ring_exclusive() to kvm_use_dirty_bitmap()
    in PATCH[v7 04/09]                                         (Peter/Oliver/Sean)
  * Add PATCH[v7 05/09] to improve no-running-vcpu report      (Marc/Sean)
  * Improve commit messages                                    (Sean/Oliver)
v6:
  * Add CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP, for arm64
    to advertise KVM_CAP_DIRTY_RING_WITH_BITMAP in
    PATCH[v6 3/8]                                              (Oliver/Peter)
  * Add helper kvm_dirty_ring_exclusive() to check if
    traditional bitmap-based dirty log tracking is
    exclusive to dirty-ring in PATCH[v6 3/8]                   (Peter)
  * Enable KVM_CAP_DIRTY_RING_WITH_BITMAP in PATCH[v6 5/8]     (Gavin)
v5:
  * Drop empty stub kvm_dirty_ring_check_request()             (Marc/Peter)
  * Add PATCH[v5 3/7] to allow using bitmap, indicated by
    KVM_CAP_DIRTY_LOG_RING_ALLOW_BITMAP                        (Marc/Peter)
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 (7):
  KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL
  KVM: Move declaration of kvm_cpu_dirty_log_size() to kvm_dirty_ring.h
  KVM: Support dirty ring in conjunction with bitmap
  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                | 36 ++++++++---
 .../virt/kvm/devices/arm-vgic-its.rst         |  5 +-
 arch/arm64/include/uapi/asm/kvm.h             |  1 +
 arch/arm64/kvm/Kconfig                        |  2 +
 arch/arm64/kvm/arm.c                          |  3 +
 arch/arm64/kvm/vgic/vgic-its.c                | 20 ++++++
 arch/x86/include/asm/kvm_host.h               |  2 -
 arch/x86/kvm/x86.c                            | 15 ++---
 include/kvm/arm_vgic.h                        |  1 +
 include/linux/kvm_dirty_ring.h                | 20 +++---
 include/linux/kvm_host.h                      | 10 +--
 include/uapi/linux/kvm.h                      |  1 +
 tools/testing/selftests/kvm/dirty_log_test.c  | 53 +++++++++++----
 tools/testing/selftests/kvm/lib/kvm_util.c    |  2 +-
 virt/kvm/Kconfig                              |  6 ++
 virt/kvm/dirty_ring.c                         | 46 ++++++++++++-
 virt/kvm/kvm_main.c                           | 64 +++++++++++++++----
 17 files changed, 227 insertions(+), 60 deletions(-)

-- 
2.23.0

[PATCH v10 1/7] KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL

[Gavin Shan]

The VCPU isn't expected to be runnable when the dirty ring becomes soft
full, until the dirty pages are harvested and the dirty ring is reset
from userspace. So there is a check in each guest's entrace to see if
the dirty ring is soft full or not. The VCPU is stopped from running if
its dirty ring has been soft full. The similar check will be needed when
the feature is going to be supported on ARM64. As Marc Zyngier suggested,
a new event will avoid pointless overhead to check the size of the dirty
ring ('vcpu->kvm->dirty_ring_size') in each guest's entrance.

Add KVM_REQ_DIRTY_RING_SOFT_FULL. The event is raised when the dirty ring
becomes soft full in kvm_dirty_ring_push(). The event is only cleared in
the check, done in the newly added helper kvm_dirty_ring_check_request().
Since the VCPU is not runnable when the dirty ring becomes soft full, the
KVM_REQ_DIRTY_RING_SOFT_FULL event is always set to prevent the VCPU from
running until the dirty pages are harvested and the dirty ring is reset by
userspace.

kvm_dirty_ring_soft_full() becomes a private function with the newly added
helper kvm_dirty_ring_check_request(). The alignment for the various event
definitions in kvm_host.h is changed to tab character by the way. In order
to avoid using 'container_of()', the argument @ring is replaced by @vcpu
in kvm_dirty_ring_push().

Link: https://lore.kernel.org/kvmarm/87lerkwtm5.wl-maz@kernel.org
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c             | 15 ++++++---------
 include/linux/kvm_dirty_ring.h | 12 ++++--------
 include/linux/kvm_host.h       |  9 +++++----
 virt/kvm/dirty_ring.c          | 32 ++++++++++++++++++++++++++++++--
 virt/kvm/kvm_main.c            |  3 +--
 5 files changed, 46 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5f5eb577d583..a228a1b872bb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10515,20 +10515,17 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	bool req_immediate_exit = false;
 
-	/* Forbid vmenter if vcpu dirty ring is soft-full */
-	if (unlikely(vcpu->kvm->dirty_ring_size &&
-		     kvm_dirty_ring_soft_full(&vcpu->dirty_ring))) {
-		vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL;
-		trace_kvm_dirty_ring_exit(vcpu);
-		r = 0;
-		goto out;
-	}
-
 	if (kvm_request_pending(vcpu)) {
 		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
 			r = -EIO;
 			goto out;
 		}
+
+		if (kvm_dirty_ring_check_request(vcpu)) {
+			r = 0;
+			goto out;
+		}
+
 		if (kvm_check_request(KVM_REQ_GET_NESTED_STATE_PAGES, vcpu)) {
 			if (unlikely(!kvm_x86_ops.nested_ops->get_nested_state_pages(vcpu))) {
 				r = 0;
diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
index 906f899813dc..9c13c4c3d30c 100644
--- a/include/linux/kvm_dirty_ring.h
+++ b/include/linux/kvm_dirty_ring.h
@@ -49,7 +49,7 @@ static inline int kvm_dirty_ring_reset(struct kvm *kvm,
 	return 0;
 }
 
-static inline void kvm_dirty_ring_push(struct kvm_dirty_ring *ring,
+static inline void kvm_dirty_ring_push(struct kvm_vcpu *vcpu,
 				       u32 slot, u64 offset)
 {
 }
@@ -64,11 +64,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);
@@ -84,13 +79,14 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring);
  * returns =0: successfully pushed
  *         <0: unable to push, need to wait
  */
-void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset);
+void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, 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 18592bdf4c1b..6fab55e58111 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -153,10 +153,11 @@ static inline bool is_error_page(struct page *page)
  * Architecture-independent vcpu->requests bit members
  * Bits 3-7 are reserved for more arch-independent bits.
  */
-#define KVM_REQ_TLB_FLUSH         (0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_VM_DEAD           (1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
-#define KVM_REQ_UNBLOCK           2
-#define KVM_REQUEST_ARCH_BASE     8
+#define KVM_REQ_TLB_FLUSH		(0 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_VM_DEAD			(1 | KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
+#define KVM_REQ_UNBLOCK			2
+#define KVM_REQ_DIRTY_RING_SOFT_FULL	3
+#define KVM_REQUEST_ARCH_BASE		8
 
 /*
  * KVM_REQ_OUTSIDE_GUEST_MODE exists is purely as way to force the vCPU to
diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
index d6fabf238032..fecbb7d75ad2 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;
 }
@@ -142,13 +142,19 @@ int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring)
 
 	kvm_reset_dirty_gfn(kvm, cur_slot, cur_offset, mask);
 
+	/*
+	 * The request KVM_REQ_DIRTY_RING_SOFT_FULL will be cleared
+	 * by the VCPU thread next time when it enters the guest.
+	 */
+
 	trace_kvm_dirty_ring_reset(ring);
 
 	return count;
 }
 
-void kvm_dirty_ring_push(struct kvm_dirty_ring *ring, u32 slot, u64 offset)
+void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, u32 slot, u64 offset)
 {
+	struct kvm_dirty_ring *ring = &vcpu->dirty_ring;
 	struct kvm_dirty_gfn *entry;
 
 	/* It should never get full */
@@ -166,6 +172,28 @@ 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_DIRTY_RING_SOFT_FULL, vcpu);
+}
+
+bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * The VCPU isn't runnable when the dirty ring becomes soft full.
+	 * The KVM_REQ_DIRTY_RING_SOFT_FULL event is always set to prevent
+	 * the VCPU from running until the dirty pages are harvested and
+	 * the dirty ring is reset by userspace.
+	 */
+	if (kvm_check_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu) &&
+	    kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) {
+		kvm_make_request(KVM_REQ_DIRTY_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)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 25d7872b29c1..c865d7d82685 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3314,8 +3314,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
 		u32 slot = (memslot->as_id << 16) | memslot->id;
 
 		if (kvm->dirty_ring_size)
-			kvm_dirty_ring_push(&vcpu->dirty_ring,
-					    slot, rel_gfn);
+			kvm_dirty_ring_push(vcpu, slot, rel_gfn);
 		else
 			set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
-- 
2.23.0

[PATCH v10 2/7] KVM: 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>
Reviewed-by: Peter Xu <peterx@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 7551b6f9c31c..b4dbde7d9eb1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2090,8 +2090,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 9c13c4c3d30c..199ead37b104 100644
--- a/include/linux/kvm_dirty_ring.h
+++ b/include/linux/kvm_dirty_ring.h
@@ -66,6 +66,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

[PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan]

ARM64 needs to dirty memory outside of a VCPU context when VGIC/ITS is
enabled. It's conflicting with that ring-based dirty page tracking always
requires a running VCPU context.

Introduce a new flavor of dirty ring that requires the use of both VCPU
dirty rings and a dirty bitmap. The expectation is that for non-VCPU
sources of dirty memory (such as the VGIC/ITS on arm64), KVM writes to
the dirty bitmap. Userspace should scan the dirty bitmap before migrating
the VM to the target.

Use an additional capability to advertise this behavior. The newly added
capability (KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP) can't be enabled before
KVM_CAP_DIRTY_LOG_RING_ACQ_REL on ARM64. In this way, the newly added
capability is treated as an extension of KVM_CAP_DIRTY_LOG_RING_ACQ_REL.

Suggested-by: Marc Zyngier <maz@kernel.org>
Suggested-by: Peter Xu <peterx@redhat.com>
Co-developed-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Gavin Shan <gshan@redhat.com>
Acked-by: Peter Xu <peterx@redhat.com>
---
 Documentation/virt/kvm/api.rst                | 34 ++++++++---
 .../virt/kvm/devices/arm-vgic-its.rst         |  5 +-
 include/linux/kvm_dirty_ring.h                |  7 +++
 include/linux/kvm_host.h                      |  1 +
 include/uapi/linux/kvm.h                      |  1 +
 virt/kvm/Kconfig                              |  6 ++
 virt/kvm/dirty_ring.c                         | 14 +++++
 virt/kvm/kvm_main.c                           | 61 ++++++++++++++++---
 8 files changed, 112 insertions(+), 17 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index eee9f857a986..1f1b09aa6db4 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -8003,13 +8003,6 @@ flushing is done by the KVM_GET_DIRTY_LOG ioctl).  To achieve that, one
 needs to kick the vcpu out of KVM_RUN using a signal.  The resulting
 vmexit ensures that all dirty GFNs are flushed to the dirty rings.
 
-NOTE: the capability KVM_CAP_DIRTY_LOG_RING and the corresponding
-ioctl KVM_RESET_DIRTY_RINGS are mutual exclusive to the existing ioctls
-KVM_GET_DIRTY_LOG and KVM_CLEAR_DIRTY_LOG.  After enabling
-KVM_CAP_DIRTY_LOG_RING with an acceptable dirty ring size, the virtual
-machine will switch to ring-buffer dirty page tracking and further
-KVM_GET_DIRTY_LOG or KVM_CLEAR_DIRTY_LOG ioctls will fail.
-
 NOTE: KVM_CAP_DIRTY_LOG_RING_ACQ_REL is the only capability that
 should be exposed by weakly ordered architecture, in order to indicate
 the additional memory ordering requirements imposed on userspace when
@@ -8018,6 +8011,33 @@ Architecture with TSO-like ordering (such as x86) are allowed to
 expose both KVM_CAP_DIRTY_LOG_RING and KVM_CAP_DIRTY_LOG_RING_ACQ_REL
 to userspace.
 
+After enabling the dirty rings, the userspace needs to detect the
+capability of KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP to see whether the
+ring structures can be backed by per-slot bitmaps. With this capability
+advertised, it means the architecture can dirty guest pages without
+vcpu/ring context, so that some of the dirty information will still be
+maintained in the bitmap structure. KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
+can't be enabled if the capability of KVM_CAP_DIRTY_LOG_RING_ACQ_REL
+hasn't been enabled, or any memslot has been existing.
+
+Note that the bitmap here is only a backup of the ring structure. The
+use of the ring and bitmap combination is only beneficial if there is
+only a very small amount of memory that is dirtied out of vcpu/ring
+context. Otherwise, the stand-alone per-slot bitmap mechanism needs to
+be considered.
+
+To collect dirty bits in the backup bitmap, userspace can use the same
+KVM_GET_DIRTY_LOG ioctl. KVM_CLEAR_DIRTY_LOG isn't needed as long as all
+the generation of the dirty bits is done in a single pass. Collecting
+the dirty bitmap should be the very last thing that the VMM does before
+considering the state as complete. VMM needs to ensure that the dirty
+state is final and avoid missing dirty pages from another ioctl ordered
+after the bitmap collection.
+
+NOTE: One example of using the backup bitmap is saving arm64 vgic/its
+tables through KVM_DEV_ARM_{VGIC_GRP_CTRL, ITS_SAVE_TABLES} command on
+KVM device "kvm-arm-vgic-its" when dirty ring is enabled.
+
 8.30 KVM_CAP_XEN_HVM
 --------------------
 
diff --git a/Documentation/virt/kvm/devices/arm-vgic-its.rst b/Documentation/virt/kvm/devices/arm-vgic-its.rst
index d257eddbae29..e053124f77c4 100644
--- a/Documentation/virt/kvm/devices/arm-vgic-its.rst
+++ b/Documentation/virt/kvm/devices/arm-vgic-its.rst
@@ -52,7 +52,10 @@ KVM_DEV_ARM_VGIC_GRP_CTRL
 
     KVM_DEV_ARM_ITS_SAVE_TABLES
       save the ITS table data into guest RAM, at the location provisioned
-      by the guest in corresponding registers/table entries.
+      by the guest in corresponding registers/table entries. Should userspace
+      require a form of dirty tracking to identify which pages are modified
+      by the saving process, it should use a bitmap even if using another
+      mechanism to track the memory dirtied by the vCPUs.
 
       The layout of the tables in guest memory defines an ABI. The entries
       are laid out in little endian format as described in the last paragraph.
diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h
index 199ead37b104..4862c98d80d3 100644
--- a/include/linux/kvm_dirty_ring.h
+++ b/include/linux/kvm_dirty_ring.h
@@ -37,6 +37,11 @@ static inline u32 kvm_dirty_ring_get_rsvd_entries(void)
 	return 0;
 }
 
+static inline bool kvm_use_dirty_bitmap(struct kvm *kvm)
+{
+	return true;
+}
+
 static inline int kvm_dirty_ring_alloc(struct kvm_dirty_ring *ring,
 				       int index, u32 size)
 {
@@ -67,6 +72,8 @@ 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);
+bool kvm_use_dirty_bitmap(struct kvm *kvm);
+bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm);
 u32 kvm_dirty_ring_get_rsvd_entries(void);
 int kvm_dirty_ring_alloc(struct kvm_dirty_ring *ring, int index, u32 size);
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 6fab55e58111..f51eb9419bfc 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -779,6 +779,7 @@ struct kvm {
 	pid_t userspace_pid;
 	unsigned int max_halt_poll_ns;
 	u32 dirty_ring_size;
+	bool dirty_ring_with_bitmap;
 	bool vm_bugged;
 	bool vm_dead;
 
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 0d5d4419139a..c87b5882d7ae 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1178,6 +1178,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_S390_ZPCI_OP 221
 #define KVM_CAP_S390_CPU_TOPOLOGY 222
 #define KVM_CAP_DIRTY_LOG_RING_ACQ_REL 223
+#define KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP 224
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 800f9470e36b..9fb1ff6f19e5 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -33,6 +33,12 @@ config HAVE_KVM_DIRTY_RING_ACQ_REL
        bool
        select HAVE_KVM_DIRTY_RING
 
+# Allow enabling both the dirty bitmap and dirty ring. Only architectures
+# that need to dirty memory outside of a vCPU context should select this.
+config NEED_KVM_DIRTY_RING_WITH_BITMAP
+	bool
+	depends on HAVE_KVM_DIRTY_RING
+
 config HAVE_KVM_EVENTFD
        bool
        select EVENTFD
diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c
index fecbb7d75ad2..c1cd7dfe4a90 100644
--- a/virt/kvm/dirty_ring.c
+++ b/virt/kvm/dirty_ring.c
@@ -21,6 +21,20 @@ u32 kvm_dirty_ring_get_rsvd_entries(void)
 	return KVM_DIRTY_RING_RSVD_ENTRIES + kvm_cpu_dirty_log_size();
 }
 
+bool kvm_use_dirty_bitmap(struct kvm *kvm)
+{
+	lockdep_assert_held(&kvm->slots_lock);
+
+	return !kvm->dirty_ring_size || kvm->dirty_ring_with_bitmap;
+}
+
+#ifndef CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP
+bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
+{
+	return false;
+}
+#endif
+
 static u32 kvm_dirty_ring_used(struct kvm_dirty_ring *ring)
 {
 	return READ_ONCE(ring->dirty_index) - READ_ONCE(ring->reset_index);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c865d7d82685..2719e10dd37d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1617,7 +1617,7 @@ static int kvm_prepare_memory_region(struct kvm *kvm,
 			new->dirty_bitmap = NULL;
 		else if (old && old->dirty_bitmap)
 			new->dirty_bitmap = old->dirty_bitmap;
-		else if (!kvm->dirty_ring_size) {
+		else if (kvm_use_dirty_bitmap(kvm)) {
 			r = kvm_alloc_dirty_bitmap(new);
 			if (r)
 				return r;
@@ -2060,8 +2060,8 @@ int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log,
 	unsigned long n;
 	unsigned long any = 0;
 
-	/* Dirty ring tracking is exclusive to dirty log tracking */
-	if (kvm->dirty_ring_size)
+	/* Dirty ring tracking may be exclusive to dirty log tracking */
+	if (!kvm_use_dirty_bitmap(kvm))
 		return -ENXIO;
 
 	*memslot = NULL;
@@ -2125,8 +2125,8 @@ static int kvm_get_dirty_log_protect(struct kvm *kvm, struct kvm_dirty_log *log)
 	unsigned long *dirty_bitmap_buffer;
 	bool flush;
 
-	/* Dirty ring tracking is exclusive to dirty log tracking */
-	if (kvm->dirty_ring_size)
+	/* Dirty ring tracking may be exclusive to dirty log tracking */
+	if (!kvm_use_dirty_bitmap(kvm))
 		return -ENXIO;
 
 	as_id = log->slot >> 16;
@@ -2237,8 +2237,8 @@ static int kvm_clear_dirty_log_protect(struct kvm *kvm,
 	unsigned long *dirty_bitmap_buffer;
 	bool flush;
 
-	/* Dirty ring tracking is exclusive to dirty log tracking */
-	if (kvm->dirty_ring_size)
+	/* Dirty ring tracking may be exclusive to dirty log tracking */
+	if (!kvm_use_dirty_bitmap(kvm))
 		return -ENXIO;
 
 	as_id = log->slot >> 16;
@@ -3305,7 +3305,10 @@ 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;
+
+	if (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) && !vcpu))
 		return;
 #endif
 
@@ -3313,7 +3316,7 @@ 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 (kvm->dirty_ring_size && vcpu)
 			kvm_dirty_ring_push(vcpu, slot, rel_gfn);
 		else
 			set_bit_le(rel_gfn, memslot->dirty_bitmap);
@@ -4482,6 +4485,9 @@ static long kvm_vm_ioctl_check_extension_generic(struct kvm *kvm, long arg)
 		return KVM_DIRTY_RING_MAX_ENTRIES * sizeof(struct kvm_dirty_gfn);
 #else
 		return 0;
+#endif
+#ifdef CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP
+	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP:
 #endif
 	case KVM_CAP_BINARY_STATS_FD:
 	case KVM_CAP_SYSTEM_EVENT_DATA:
@@ -4558,6 +4564,20 @@ int __attribute__((weak)) kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 	return -EINVAL;
 }
 
+static bool kvm_are_all_memslots_empty(struct kvm *kvm)
+{
+	int i;
+
+	lockdep_assert_held(&kvm->slots_lock);
+
+	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
+		if (!kvm_memslots_empty(__kvm_memslots(kvm, i)))
+			return false;
+	}
+
+	return true;
+}
+
 static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
 					   struct kvm_enable_cap *cap)
 {
@@ -4588,6 +4608,29 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
 			return -EINVAL;
 
 		return kvm_vm_ioctl_enable_dirty_log_ring(kvm, cap->args[0]);
+	case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP: {
+		int r = -EINVAL;
+
+		if (!IS_ENABLED(CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP) ||
+		    !kvm->dirty_ring_size || cap->flags)
+			return r;
+
+		mutex_lock(&kvm->slots_lock);
+
+		/*
+		 * For simplicity, allow enabling ring+bitmap if and only if
+		 * there are no memslots, e.g. to ensure all memslots allocate
+		 * a bitmap after the capability is enabled.
+		 */
+		if (kvm_are_all_memslots_empty(kvm)) {
+			kvm->dirty_ring_with_bitmap = true;
+			r = 0;
+		}
+
+		mutex_unlock(&kvm->slots_lock);
+
+		return r;
+	}
 	default:
 		return kvm_vm_ioctl_enable_cap(kvm, cap);
 	}
-- 
2.23.0

[PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Enable ring-based dirty memory tracking on ARM64:

  - Enable CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL.

  - Enable CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP.

  - Set KVM_DIRTY_LOG_PAGE_OFFSET for the ring buffer's physical page
    offset.

  - Add ARM64 specific kvm_arch_allow_write_without_running_vcpu() to
    keep the site of saving vgic/its tables out of the no-running-vcpu
    radar.

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            |  2 ++
 arch/arm64/kvm/arm.c              |  3 +++
 arch/arm64/kvm/vgic/vgic-its.c    | 20 ++++++++++++++++++++
 include/kvm/arm_vgic.h            |  1 +
 6 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 1f1b09aa6db4..773e4b202f47 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7921,7 +7921,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..05da3c8f7e88 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -32,6 +32,8 @@ menuconfig KVM
 	select KVM_VFIO
 	select HAVE_KVM_EVENTFD
 	select HAVE_KVM_IRQFD
+	select HAVE_KVM_DIRTY_RING_ACQ_REL
+	select NEED_KVM_DIRTY_RING_WITH_BITMAP
 	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 94d33e296e10..6b097605e38c 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -746,6 +746,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;
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 733b53055f97..94a666dd1443 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2743,6 +2743,7 @@ static int vgic_its_has_attr(struct kvm_device *dev,
 static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	struct vgic_dist *dist = &kvm->arch.vgic;
 	int ret = 0;
 
 	if (attr == KVM_DEV_ARM_VGIC_CTRL_INIT) /* Nothing to do */
@@ -2762,7 +2763,9 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
 		vgic_its_reset(kvm, its);
 		break;
 	case KVM_DEV_ARM_ITS_SAVE_TABLES:
+		dist->save_its_tables_in_progress = true;
 		ret = abi->save_tables(its);
+		dist->save_its_tables_in_progress = false;
 		break;
 	case KVM_DEV_ARM_ITS_RESTORE_TABLES:
 		ret = abi->restore_tables(its);
@@ -2775,6 +2778,23 @@ static int vgic_its_ctrl(struct kvm *kvm, struct vgic_its *its, u64 attr)
 	return ret;
 }
 
+/*
+ * kvm_arch_allow_write_without_running_vcpu - allow writing guest memory
+ * without the running VCPU when dirty ring is enabled.
+ *
+ * The running VCPU is required to track dirty guest pages when dirty ring
+ * is enabled. Otherwise, the backup bitmap should be used to track the
+ * dirty guest pages. When vgic/its tables are being saved, the backup
+ * bitmap is used to track the dirty guest pages due to the missed running
+ * VCPU in the period.
+ */
+bool kvm_arch_allow_write_without_running_vcpu(struct kvm *kvm)
+{
+	struct vgic_dist *dist = &kvm->arch.vgic;
+
+	return dist->save_its_tables_in_progress;
+}
+
 static int vgic_its_set_attr(struct kvm_device *dev,
 			     struct kvm_device_attr *attr)
 {
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 4df9e73a8bb5..9270cd87da3f 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -263,6 +263,7 @@ struct vgic_dist {
 	struct vgic_io_device	dist_iodev;
 
 	bool			has_its;
+	bool			save_its_tables_in_progress;
 
 	/*
 	 * Contains the attributes and gpa of the LPI configuration table.
-- 
2.23.0

[PATCH v10 5/7] 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 f1cb1627161f..89a1a420ebd5 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -1506,7 +1506,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

[PATCH v10 6/7] 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

[PATCH v10 7/7] 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

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

[Marc Zyngier]

On Thu, 10 Nov 2022 18:49:07 +0800, Gavin Shan 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").
> 
> For PATCH[v9 3/7], Peter's ack-by is kept since the recent changes
> don't fundamentally break what he agreed. However, it would be nice
> for Peter to double confirm.
> 
> [...]

Applied to next, thanks!

[1/7] KVM: x86: Introduce KVM_REQ_DIRTY_RING_SOFT_FULL
      commit: cf87ac739e488055a6046a410caa8f4da108948f
[2/7] KVM: Move declaration of kvm_cpu_dirty_log_size() to kvm_dirty_ring.h
      commit: e8a18565e59303ac12c626a161d72bd890bd2062
[3/7] KVM: Support dirty ring in conjunction with bitmap
      commit: 86bdf3ebcfe1ded055282536fecce13001874740
[4/7] KVM: arm64: Enable ring-based dirty memory tracking
      commit: 9cb1096f8590bc590326087bea65db932b53c3b5
[5/7] KVM: selftests: Use host page size to map ring buffer in dirty_log_test
      commit: a737f5ffb1e883e580730122be11c9eb832a7749
[6/7] KVM: selftests: Clear dirty ring states between two modes in dirty_log_test
      commit: 7167190ddb863bd061c0c6b61f4cec94184b40da
[7/7] KVM: selftests: Automate choosing dirty ring size in dirty_log_test
      commit: dc6df7d4d0633e65850d5372ae9f1234bcc6e26e

Cheers,

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

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Sean Christopherson]

On Thu, Nov 10, 2022, Gavin Shan wrote:
> @@ -3305,7 +3305,10 @@ 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;
> +
> +	if (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) && !vcpu))

Nit, the !vcpu check should come first.  99.9% of the time @vcpu will be non-NULL,
in which case the call to kvm_arch_allow_write_without_running_vcpu() can be
avoided.  And checking for !vcpu should also be cheaper than said call.

Since the below code is gaining a check on "vcpu" when using the dirty ring, and
needs to gain a check on memslot->dirty_bitmap, I think it makes sense to let KVM
continue on instead of bailing.  I.e. fill the dirty bitmap if possible instead
of dropping the dirty info and potentiall corrupting guest memory.

The "vcpu->kvm != kvm" check is different; if that fails, KVM would potentially
log the dirty page into the wrong VM's context, which could be fatal to one or
both VMs.

If it's not too late to rewrite history, this could even be done as a prep patch,
e.g. with a changelog explaning that KVM should try to log to the dirty bitmap
even if KVM has a bug where a write to guest memory was triggered without a running
vCPU.

---
 virt/kvm/kvm_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 43bbe4fde078..b1115bbd6038 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3313,18 +3313,20 @@ 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;
+
+	WARN_ON_ONCE(!vcpu);
 #endif
 
 	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
 		u32 slot = (memslot->as_id << 16) | memslot->id;
 
-		if (kvm->dirty_ring_size)
+		if (kvm->dirty_ring_size && vcpu)
 			kvm_dirty_ring_push(&vcpu->dirty_ring,
 					    slot, rel_gfn);
-		else
+		else if (memslot->dirty_bitmap)
 			set_bit_le(rel_gfn, memslot->dirty_bitmap);
 	}
 }

base-commit: 01b4689ee519329ce5f50ae02692e8acdaba0beb
-- 



>  		return;
>  #endif
>  
> @@ -3313,7 +3316,7 @@ 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 (kvm->dirty_ring_size && vcpu)
>  			kvm_dirty_ring_push(vcpu, slot, rel_gfn);
>  		else
>  			set_bit_le(rel_gfn, memslot->dirty_bitmap);

Not checking memslot->dirty_bitmap will allow a malicious userspace to induce a
NULL pointer dereference, e.g. enable dirty ring without the bitmap and save the
ITS tables.  The KVM_DEV_ARM_ITS_SAVE_TABLES path in patch 4 doesn't check that
kvm_use_dirty_bitmap() is true.

If requiring kvm_use_dirty_bitmap() to do KVM_DEV_ARM_ITS_SAVE_TABLES is deemed
to prescriptive, the best this code can do is:

		if (kvm->dirty_ring_size && vcpu)
			kvm_dirty_ring_push(&vcpu->dirty_ring,
					    slot, rel_gfn);
		else if (memslot->dirty_bitmap)
			set_bit_le(rel_gfn, memslot->dirty_bitmap);

If ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES, then this can be:

		if (kvm->dirty_ring_size && vcpu)
			kvm_dirty_ring_push(&vcpu->dirty_ring,
					    slot, rel_gfn);
		else if (!WARN_ON_ONCE(!memslot->dirty_bitmap))
			set_bit_le(rel_gfn, memslot->dirty_bitmap);

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan]

Hi Sean,

On 11/11/22 12:46 AM, Sean Christopherson wrote:
> On Thu, Nov 10, 2022, Gavin Shan wrote:
>> @@ -3305,7 +3305,10 @@ 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;
>> +
>> +	if (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) && !vcpu))
> 
> Nit, the !vcpu check should come first.  99.9% of the time @vcpu will be non-NULL,
> in which case the call to kvm_arch_allow_write_without_running_vcpu() can be
> avoided.  And checking for !vcpu should also be cheaper than said call.
> 
> Since the below code is gaining a check on "vcpu" when using the dirty ring, and
> needs to gain a check on memslot->dirty_bitmap, I think it makes sense to let KVM
> continue on instead of bailing.  I.e. fill the dirty bitmap if possible instead
> of dropping the dirty info and potentiall corrupting guest memory.
> 
> The "vcpu->kvm != kvm" check is different; if that fails, KVM would potentially
> log the dirty page into the wrong VM's context, which could be fatal to one or
> both VMs.
> 
> If it's not too late to rewrite history, this could even be done as a prep patch,
> e.g. with a changelog explaning that KVM should try to log to the dirty bitmap
> even if KVM has a bug where a write to guest memory was triggered without a running
> vCPU.
> 

The series has been merged to Marc's 'next' branch. I will post another
patch on the top to cover your comments since it's an additional improvement.

About these two checks kvm_arch_allow_write_without_running_vcpu() and !vcpu,
the first one is literally the precondition to the later one. It was my
thought. However, you're right that we have vcpu context for 99.9% time.
So it sounds good to me to swap their positions, to check !vcpu first.

Assume we run into !kvm_arch_allow_write_without_running_vcpu() and !vcpu,
it's a unknown (new) case where no running vcpu exists. Note the only known
case is vgic/its at present. The userspace is going to enable the backup bitmap
for the new reported case. Right, it'd better to keep it going instead of
bailing early. In this way, we don't have backwards compatible issue at
least, meaning new application and old kernel are still working.
'memslot->dirty_bitmap' needs to be checked if we don't bail early.

> ---
>   virt/kvm/kvm_main.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 43bbe4fde078..b1115bbd6038 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3313,18 +3313,20 @@ 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;
> +
> +	WARN_ON_ONCE(!vcpu);
>   #endif
>   
>   	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>   		unsigned long rel_gfn = gfn - memslot->base_gfn;
>   		u32 slot = (memslot->as_id << 16) | memslot->id;
>   
> -		if (kvm->dirty_ring_size)
> +		if (kvm->dirty_ring_size && vcpu)
>   			kvm_dirty_ring_push(&vcpu->dirty_ring,
>   					    slot, rel_gfn);
> -		else
> +		else if (memslot->dirty_bitmap)
>   			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>   	}
>   }
> 
> base-commit: 01b4689ee519329ce5f50ae02692e8acdaba0beb
>> Not checking memslot->dirty_bitmap will allow a malicious userspace to induce a
> NULL pointer dereference, e.g. enable dirty ring without the bitmap and save the
> ITS tables.  The KVM_DEV_ARM_ITS_SAVE_TABLES path in patch 4 doesn't check that
> kvm_use_dirty_bitmap() is true.

> If requiring kvm_use_dirty_bitmap() to do KVM_DEV_ARM_ITS_SAVE_TABLES is deemed
> to prescriptive, the best this code can do is:
>
>		if (kvm->dirty_ring_size && vcpu)
>			kvm_dirty_ring_push(&vcpu->dirty_ring,
>					    slot, rel_gfn);
>		else if (memslot->dirty_bitmap)
>			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>
> If ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES, then this can be:
>
>		if (kvm->dirty_ring_size && vcpu)
>			kvm_dirty_ring_push(&vcpu->dirty_ring,
>					    slot, rel_gfn);
>		else if (!WARN_ON_ONCE(!memslot->dirty_bitmap))
>			set_bit_le(rel_gfn, memslot->dirty_bitmap);
>

The first hunk of changes looks good to me as the back bitmap is required to
save vgic/its tables. For unknown (new) case where we don't have running vcpu,
we have WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm)) to
catch it.

---

commit b05377ecbe003f12c8b79846fa3a300401dcab68 (HEAD -> kvm/arm64_dirtyring)
Author: Gavin Shan <gshan@redhat.com>
Date:   Fri Nov 11 07:13:12 2022 +0800

     KVM: Push dirty information unconditionally to backup bitmap
     
     In mark_page_dirty_in_slot(), we bail out when no running vcpu exists and
     a running vcpu context is strictly required by architecture. It may cause
     backwards compatible issue. Currently, saving vgic/its tables is the only
     case where no running vcpu context is required. We may have other unknown
     cases where no running vcpu context exists and it's reported by the warning
     message. For this, the application is going to enable the backup bitmap for
     the unknown cases. However, the dirty information can't be pushed to the
     backup bitmap even though the backup bitmap has been enabled, until the
     unknown cases are added to the allowed list of non-running vcpu context
     with extra code changes to the host kernel.
     
     In order to make the new application, where the backup bitmap has been
     enabled, to work with the unchanged host, we continue to push the dirty
     information to the backup bitmap instead of bailing out early.
     
     Suggested-by: Sean Christopherson <seanjc@google.com>
     Signed-off-by: Gavin Shan <gshan@redhat.com>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2719e10dd37d..03e6a38094c1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3308,8 +3308,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
         if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
                 return;
  
-       if (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) && !vcpu))
-               return;
+       WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));
  #endif
  
         if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
@@ -3318,7 +3317,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
  
                 if (kvm->dirty_ring_size && vcpu)
                         kvm_dirty_ring_push(vcpu, slot, rel_gfn);
-               else
+               else if (memslot->dirty_bitmap)
                         set_bit_le(rel_gfn, memslot->dirty_bitmap);
         }
  }


Thanks,
Gavin

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Marc Zyngier]

On Thu, 10 Nov 2022 23:47:41 +0000,
Gavin Shan <gshan@redhat.com> wrote:
> 
> commit b05377ecbe003f12c8b79846fa3a300401dcab68 (HEAD -> kvm/arm64_dirtyring)
> Author: Gavin Shan <gshan@redhat.com>
> Date:   Fri Nov 11 07:13:12 2022 +0800
> 
>     KVM: Push dirty information unconditionally to backup bitmap
>         In mark_page_dirty_in_slot(), we bail out when no running vcpu
> exists and
>     a running vcpu context is strictly required by architecture. It may cause
>     backwards compatible issue. Currently, saving vgic/its tables is the only
>     case where no running vcpu context is required. We may have other unknown
>     cases where no running vcpu context exists and it's reported by the warning
>     message. For this, the application is going to enable the backup bitmap for
>     the unknown cases. However, the dirty information can't be pushed to the
>     backup bitmap even though the backup bitmap has been enabled, until the
>     unknown cases are added to the allowed list of non-running vcpu context
>     with extra code changes to the host kernel.
>         In order to make the new application, where the backup bitmap
> has been
>     enabled, to work with the unchanged host, we continue to push the dirty
>     information to the backup bitmap instead of bailing out early.
>         Suggested-by: Sean Christopherson <seanjc@google.com>
>     Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2719e10dd37d..03e6a38094c1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3308,8 +3308,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>         if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>                 return;
>  -       if
> (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) &&
> !vcpu))
> -               return;
> +       WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));

I'm happy with this.

>  #endif
>          if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
> @@ -3318,7 +3317,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>                  if (kvm->dirty_ring_size && vcpu)
>                         kvm_dirty_ring_push(vcpu, slot, rel_gfn);
> -               else
> +               else if (memslot->dirty_bitmap)
>                         set_bit_le(rel_gfn, memslot->dirty_bitmap);

But that I don't get. Or rather, I don't get the commit message that
matches this hunk. Do we want to catch the case where all of the
following are true:

- we don't have a vcpu,
- we're allowed to log non-vcpu dirtying
- we *only* have the ring?

If so, can we please capture that in the commit message?

Thanks,

	M.

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

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan]

Hi Marc,

On 11/11/22 11:19 PM, Marc Zyngier wrote:
> On Thu, 10 Nov 2022 23:47:41 +0000,
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> commit b05377ecbe003f12c8b79846fa3a300401dcab68 (HEAD -> kvm/arm64_dirtyring)
>> Author: Gavin Shan <gshan@redhat.com>
>> Date:   Fri Nov 11 07:13:12 2022 +0800
>>
>>      KVM: Push dirty information unconditionally to backup bitmap
>>          In mark_page_dirty_in_slot(), we bail out when no running vcpu
>> exists and
>>      a running vcpu context is strictly required by architecture. It may cause
>>      backwards compatible issue. Currently, saving vgic/its tables is the only
>>      case where no running vcpu context is required. We may have other unknown
>>      cases where no running vcpu context exists and it's reported by the warning
>>      message. For this, the application is going to enable the backup bitmap for
>>      the unknown cases. However, the dirty information can't be pushed to the
>>      backup bitmap even though the backup bitmap has been enabled, until the
>>      unknown cases are added to the allowed list of non-running vcpu context
>>      with extra code changes to the host kernel.
>>          In order to make the new application, where the backup bitmap
>> has been
>>      enabled, to work with the unchanged host, we continue to push the dirty
>>      information to the backup bitmap instead of bailing out early.
>>          Suggested-by: Sean Christopherson <seanjc@google.com>
>>      Signed-off-by: Gavin Shan <gshan@redhat.com>
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 2719e10dd37d..03e6a38094c1 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3308,8 +3308,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>          if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>                  return;
>>   -       if
>> (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) &&
>> !vcpu))
>> -               return;
>> +       WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));
> 
> I'm happy with this.
> 

Thanks, it's the primary change in this patch.

>>   #endif
>>           if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>> @@ -3318,7 +3317,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>                   if (kvm->dirty_ring_size && vcpu)
>>                          kvm_dirty_ring_push(vcpu, slot, rel_gfn);
>> -               else
>> +               else if (memslot->dirty_bitmap)
>>                          set_bit_le(rel_gfn, memslot->dirty_bitmap);
> 
> But that I don't get. Or rather, I don't get the commit message that
> matches this hunk. Do we want to catch the case where all of the
> following are true:
> 
> - we don't have a vcpu,
> - we're allowed to log non-vcpu dirtying
> - we *only* have the ring?
> 
> If so, can we please capture that in the commit message?
> 

Nice catch! This particular case needs to be warned explicitly. Without
the patch, kernel crash is triggered. With this patch applied, the error
or warning is dropped silently. We either check memslot->dirty_bitmap
in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
I personally the later one. Let me post a formal patch on top of your
'next' branch where the commit log will be improved accordingly.

Thanks,
Gavin

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Sean Christopherson]

On Sat, Nov 12, 2022, Gavin Shan wrote:
> Hi Marc,
> 
> On 11/11/22 11:19 PM, Marc Zyngier wrote:
> > On Thu, 10 Nov 2022 23:47:41 +0000,
> > Gavin Shan <gshan@redhat.com> wrote:
> > But that I don't get. Or rather, I don't get the commit message that
> > matches this hunk. Do we want to catch the case where all of the
> > following are true:
> > 
> > - we don't have a vcpu,
> > - we're allowed to log non-vcpu dirtying
> > - we *only* have the ring?

As written, no, because the resulting WARN will be user-triggerable.  As mentioned
earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
logging is enabled with a bitmap, then this code can WARN.

> > If so, can we please capture that in the commit message?
> > 
> 
> Nice catch! This particular case needs to be warned explicitly. Without
> the patch, kernel crash is triggered. With this patch applied, the error
> or warning is dropped silently. We either check memslot->dirty_bitmap
> in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
> I personally the later one. Let me post a formal patch on top of your
> 'next' branch where the commit log will be improved accordingly.

As above, a full WARN is not a viable option unless ARM commits to rejecting
KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario.  IMO, either reject the ITS save
or silently ignore the goof.  Adding a pr_warn_ratelimited() to alert the user
that they shot themselves in the foot after the fact seems rather pointless if 
KVM could have prevented the self-inflicted wound in the first place.

[*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Marc Zyngier]

On 2022-11-11 22:19, Gavin Shan wrote:
> Hi Marc,
> 
> On 11/11/22 11:19 PM, Marc Zyngier wrote:
>> On Thu, 10 Nov 2022 23:47:41 +0000,
>> Gavin Shan <gshan@redhat.com> wrote:
>>> 
>>> commit b05377ecbe003f12c8b79846fa3a300401dcab68 (HEAD -> 
>>> kvm/arm64_dirtyring)
>>> Author: Gavin Shan <gshan@redhat.com>
>>> Date:   Fri Nov 11 07:13:12 2022 +0800
>>> 
>>>      KVM: Push dirty information unconditionally to backup bitmap
>>>          In mark_page_dirty_in_slot(), we bail out when no running 
>>> vcpu
>>> exists and
>>>      a running vcpu context is strictly required by architecture. It 
>>> may cause
>>>      backwards compatible issue. Currently, saving vgic/its tables is 
>>> the only
>>>      case where no running vcpu context is required. We may have 
>>> other unknown
>>>      cases where no running vcpu context exists and it's reported by 
>>> the warning
>>>      message. For this, the application is going to enable the backup 
>>> bitmap for
>>>      the unknown cases. However, the dirty information can't be 
>>> pushed to the
>>>      backup bitmap even though the backup bitmap has been enabled, 
>>> until the
>>>      unknown cases are added to the allowed list of non-running vcpu 
>>> context
>>>      with extra code changes to the host kernel.
>>>          In order to make the new application, where the backup 
>>> bitmap
>>> has been
>>>      enabled, to work with the unchanged host, we continue to push 
>>> the dirty
>>>      information to the backup bitmap instead of bailing out early.
>>>          Suggested-by: Sean Christopherson <seanjc@google.com>
>>>      Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> 
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 2719e10dd37d..03e6a38094c1 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -3308,8 +3308,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>          if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>>>                  return;
>>>   -       if
>>> (WARN_ON_ONCE(!kvm_arch_allow_write_without_running_vcpu(kvm) &&
>>> !vcpu))
>>> -               return;
>>> +       WARN_ON_ONCE(!vcpu && 
>>> !kvm_arch_allow_write_without_running_vcpu(kvm));
>> 
>> I'm happy with this.
>> 
> 
> Thanks, it's the primary change in this patch.
> 
>>>   #endif
>>>           if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>>> @@ -3318,7 +3317,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>                   if (kvm->dirty_ring_size && vcpu)
>>>                          kvm_dirty_ring_push(vcpu, slot, rel_gfn);
>>> -               else
>>> +               else if (memslot->dirty_bitmap)
>>>                          set_bit_le(rel_gfn, memslot->dirty_bitmap);
>> 
>> But that I don't get. Or rather, I don't get the commit message that
>> matches this hunk. Do we want to catch the case where all of the
>> following are true:
>> 
>> - we don't have a vcpu,
>> - we're allowed to log non-vcpu dirtying
>> - we *only* have the ring?
>> 
>> If so, can we please capture that in the commit message?
>> 
> 
> Nice catch! This particular case needs to be warned explicitly. Without
> the patch, kernel crash is triggered. With this patch applied, the 
> error
> or warning is dropped silently. We either check memslot->dirty_bitmap
> in mark_page_dirty_in_slot(), or check it in
> kvm_arch_allow_write_without_running_vcpu().
> I personally the later one. Let me post a formal patch on top of your
> 'next' branch where the commit log will be improved accordingly.

I personally prefer this memslot->dirty_bitmap, as this is
a completely legal case (the VMM may not want to track the
ITS dirtying).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan]

Hi Sean,

On 11/12/22 7:00 AM, Sean Christopherson wrote:
> On Sat, Nov 12, 2022, Gavin Shan wrote:
>> On 11/11/22 11:19 PM, Marc Zyngier wrote:
>>> On Thu, 10 Nov 2022 23:47:41 +0000,
>>> Gavin Shan <gshan@redhat.com> wrote:
>>> But that I don't get. Or rather, I don't get the commit message that
>>> matches this hunk. Do we want to catch the case where all of the
>>> following are true:
>>>
>>> - we don't have a vcpu,
>>> - we're allowed to log non-vcpu dirtying
>>> - we *only* have the ring?
> 
> As written, no, because the resulting WARN will be user-triggerable.  As mentioned
> earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
> logging is enabled with a bitmap, then this code can WARN.
> 

I assume you're saying to reject the command when dirty ring is enabled __without__
a bitmap. vgic/its is the upper layer of dirty dirty. To me, it's a bad idea for the
upper layer needs to worry too much about the lower layer.

>>> If so, can we please capture that in the commit message?
>>>
>>
>> Nice catch! This particular case needs to be warned explicitly. Without
>> the patch, kernel crash is triggered. With this patch applied, the error
>> or warning is dropped silently. We either check memslot->dirty_bitmap
>> in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
>> I personally the later one. Let me post a formal patch on top of your
>> 'next' branch where the commit log will be improved accordingly.
> 
> As above, a full WARN is not a viable option unless ARM commits to rejecting
> KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario.  IMO, either reject the ITS save
> or silently ignore the goof.  Adding a pr_warn_ratelimited() to alert the user
> that they shot themselves in the foot after the fact seems rather pointless if
> KVM could have prevented the self-inflicted wound in the first place.
> 
> [*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
> 

Without a message printed by WARN, kernel crash or pr_warn_ratelimited(), it
will be hard for userspace to know what's going on, because the dirty bits
have been dropped silently. I think we still survive since we have WARN
message for other known cases where no running vcpu context exists.

So if I'm correct, what we need to do is to improve the commit message to
address Marc's concerns here? :)

Thanks,
Gavin

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Sean Christopherson]

On Sat, Nov 12, 2022, Gavin Shan wrote:
> Hi Sean,
> 
> On 11/12/22 7:00 AM, Sean Christopherson wrote:
> > On Sat, Nov 12, 2022, Gavin Shan wrote:
> > > On 11/11/22 11:19 PM, Marc Zyngier wrote:
> > > > On Thu, 10 Nov 2022 23:47:41 +0000,
> > > > Gavin Shan <gshan@redhat.com> wrote:
> > > > But that I don't get. Or rather, I don't get the commit message that
> > > > matches this hunk. Do we want to catch the case where all of the
> > > > following are true:
> > > > 
> > > > - we don't have a vcpu,
> > > > - we're allowed to log non-vcpu dirtying
> > > > - we *only* have the ring?
> > 
> > As written, no, because the resulting WARN will be user-triggerable.  As mentioned
> > earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
> > logging is enabled with a bitmap, then this code can WARN.
> > 
> 
> I assume you're saying to reject the command when dirty ring is enabled
> __without__ a bitmap. vgic/its is the upper layer of dirty dirty.

I was stating that that is an option.  I was not opining anything, I truly don't
care whether or not KVM_DEV_ARM_ITS_SAVE_TABLES is rejected.

> To me, it's a bad idea for the upper layer needs to worry too much about the
> lower layer.

That ship sailed when we added kvm_arch_allow_write_without_running_vcpu().
Arguably, it sailed when the dirty ring was added, which solidified the requirement
that writing guest memory "must" be done with a running vCPU.

> > > > If so, can we please capture that in the commit message?
> > > > 
> > > 
> > > Nice catch! This particular case needs to be warned explicitly. Without
> > > the patch, kernel crash is triggered. With this patch applied, the error
> > > or warning is dropped silently. We either check memslot->dirty_bitmap
> > > in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
> > > I personally the later one. Let me post a formal patch on top of your
> > > 'next' branch where the commit log will be improved accordingly.
> > 
> > As above, a full WARN is not a viable option unless ARM commits to rejecting
> > KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario.  IMO, either reject the ITS save
> > or silently ignore the goof.  Adding a pr_warn_ratelimited() to alert the user
> > that they shot themselves in the foot after the fact seems rather pointless if
> > KVM could have prevented the self-inflicted wound in the first place.
> > 
> > [*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
> > 
> 
> Without a message printed by WARN, kernel crash or pr_warn_ratelimited(), it
> will be hard for userspace to know what's going on, because the dirty bits
> have been dropped silently.I think we still survive since we have WARN
> message for other known cases where no running vcpu context exists.

That WARN is to catch KVM bugs.  No KVM bugs, no WARN.  WARNs must not be user
triggerable in the absence of kernel bugs.  This is a kernel rule, not a KVM thing,
e.g. see panic_on_warn.

printk() is useless for running at any kind of scale as userspace can't take action
on "failure", e.g. unless userspace has a priori knowledge of the _exact_ error
message then human intervention is required (there are other issues as well).

A ratelimited printk() makes things even worse because then a failing VM may not
get its "failure" logged, i.e. the printk() is even less actionable.

And user triggerable printks() need to be ratelimited to prevent a malicious or
broken userspace from flooding the kernel log.  Thus, this "failure" would need
to be ratelimited, making it all but useless for anyone but developers.

> So if I'm correct, what we need to do is to improve the commit message to
> address Marc's concerns here? :)

Yes, Marc is saying that it's not strictly wrong for userspace to not dirty log
the ITS save, so rejecting KVM_DEV_ARM_ITS_SAVE_TABLES is a bad option. 

Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Gavin Shan]

Hi Sean,

On 11/12/22 8:18 AM, Sean Christopherson wrote:
> On Sat, Nov 12, 2022, Gavin Shan wrote:
>> On 11/12/22 7:00 AM, Sean Christopherson wrote:
>>> On Sat, Nov 12, 2022, Gavin Shan wrote:
>>>> On 11/11/22 11:19 PM, Marc Zyngier wrote:
>>>>> On Thu, 10 Nov 2022 23:47:41 +0000,
>>>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>> But that I don't get. Or rather, I don't get the commit message that
>>>>> matches this hunk. Do we want to catch the case where all of the
>>>>> following are true:
>>>>>
>>>>> - we don't have a vcpu,
>>>>> - we're allowed to log non-vcpu dirtying
>>>>> - we *only* have the ring?
>>>
>>> As written, no, because the resulting WARN will be user-triggerable.  As mentioned
>>> earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
>>> logging is enabled with a bitmap, then this code can WARN.
>>>
>>
>> I assume you're saying to reject the command when dirty ring is enabled
>> __without__ a bitmap. vgic/its is the upper layer of dirty dirty.
> 
> I was stating that that is an option.  I was not opining anything, I truly don't
> care whether or not KVM_DEV_ARM_ITS_SAVE_TABLES is rejected.
> 

Ok.

>> To me, it's a bad idea for the upper layer needs to worry too much about the
>> lower layer.
> 
> That ship sailed when we added kvm_arch_allow_write_without_running_vcpu().
> Arguably, it sailed when the dirty ring was added, which solidified the requirement
> that writing guest memory "must" be done with a running vCPU.
> 

Well, sort of. I don't expect large mount of dirty pages will be tracked by
the backup bitmap :)

>>>>> If so, can we please capture that in the commit message?
>>>>>
>>>>
>>>> Nice catch! This particular case needs to be warned explicitly. Without
>>>> the patch, kernel crash is triggered. With this patch applied, the error
>>>> or warning is dropped silently. We either check memslot->dirty_bitmap
>>>> in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
>>>> I personally the later one. Let me post a formal patch on top of your
>>>> 'next' branch where the commit log will be improved accordingly.
>>>
>>> As above, a full WARN is not a viable option unless ARM commits to rejecting
>>> KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario.  IMO, either reject the ITS save
>>> or silently ignore the goof.  Adding a pr_warn_ratelimited() to alert the user
>>> that they shot themselves in the foot after the fact seems rather pointless if
>>> KVM could have prevented the self-inflicted wound in the first place.
>>>
>>> [*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
>>>
>>
>> Without a message printed by WARN, kernel crash or pr_warn_ratelimited(), it
>> will be hard for userspace to know what's going on, because the dirty bits
>> have been dropped silently.I think we still survive since we have WARN
>> message for other known cases where no running vcpu context exists.
> 
> That WARN is to catch KVM bugs.  No KVM bugs, no WARN.  WARNs must not be user
> triggerable in the absence of kernel bugs.  This is a kernel rule, not a KVM thing,
> e.g. see panic_on_warn.
> 
> printk() is useless for running at any kind of scale as userspace can't take action
> on "failure", e.g. unless userspace has a priori knowledge of the _exact_ error
> message then human intervention is required (there are other issues as well).
> 
> A ratelimited printk() makes things even worse because then a failing VM may not
> get its "failure" logged, i.e. the printk() is even less actionable.
> 
> And user triggerable printks() need to be ratelimited to prevent a malicious or
> broken userspace from flooding the kernel log.  Thus, this "failure" would need
> to be ratelimited, making it all but useless for anyone but developers.
> 

Thanks for the details to explain. Right, It's true for WARN to catch KVM bugs
instead of user-triggerable bugs. I agree that none of WARN/printk/ratelimited_printk()
is applicable in this case.

>> So if I'm correct, what we need to do is to improve the commit message to
>> address Marc's concerns here? :)
> 
> Yes, Marc is saying that it's not strictly wrong for userspace to not dirty log
> the ITS save, so rejecting KVM_DEV_ARM_ITS_SAVE_TABLES is a bad option.
> 

Ok. I've posted formal patch for 'next' branch, with the improved commit message.
Please take a look when you get a chance.

https://lore.kernel.org/kvmarm/20221112094322.21911-1-gshan@redhat.com/T/#u

Thanks,
Gavin

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Zenghui Yu]

Hi Gavin,

On 2022/11/10 18:49, Gavin Shan wrote:
> Enable ring-based dirty memory tracking on ARM64:
> 
>   - Enable CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL.
> 
>   - Enable CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP.
> 
>   - Set KVM_DIRTY_LOG_PAGE_OFFSET for the ring buffer's physical page
>     offset.
> 
>   - Add ARM64 specific kvm_arch_allow_write_without_running_vcpu() to
>     keep the site of saving vgic/its tables out of the no-running-vcpu
>     radar.

And we have KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.. On receiving it, the
emulated VGIC will write all pending bits (if any) into pending tables
(which reside in guest memory) and doesn't require a running vcpu
context.

The no-running-vcpu WARN can be triggered with the
kvm-unit-tests/its-pending-migration case. I run it using QEMU, which
has nothing to do with the dirty ring atm.

Or are there already discussions about it that I haven't noticed?

|void mark_page_dirty_in_slot(struct kvm *kvm,
|			     const struct kvm_memory_slot *memslot,
|			     gfn_t gfn)
|{
|	WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Zenghui,

On 1/15/23 10:20 PM, Zenghui Yu wrote:
> On 2022/11/10 18:49, Gavin Shan wrote:
>> Enable ring-based dirty memory tracking on ARM64:
>>
>>   - Enable CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL.
>>
>>   - Enable CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP.
>>
>>   - Set KVM_DIRTY_LOG_PAGE_OFFSET for the ring buffer's physical page
>>     offset.
>>
>>   - Add ARM64 specific kvm_arch_allow_write_without_running_vcpu() to
>>     keep the site of saving vgic/its tables out of the no-running-vcpu
>>     radar.
> 
> And we have KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.. On receiving it, the
> emulated VGIC will write all pending bits (if any) into pending tables
> (which reside in guest memory) and doesn't require a running vcpu
> context.
> 
> The no-running-vcpu WARN can be triggered with the
> kvm-unit-tests/its-pending-migration case. I run it using QEMU, which
> has nothing to do with the dirty ring atm.
> 
> Or are there already discussions about it that I haven't noticed?
> 
> |void mark_page_dirty_in_slot(struct kvm *kvm,
> |                 const struct kvm_memory_slot *memslot,
> |                 gfn_t gfn)
> |{
> |    WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));
> 

It's a new case we never noticed. Could you please share the QEMU command lines
to start the guest? I need to reproduce the issue on my side firstly.

The fix would be simply to extending kvm->arch.vgic.save_its_tables_in_progress
from 'bool' to a bit map (e.g. kvm->arch.vgic.dirty_guest_memory_flags) and introduce
two separate flags for ITS table and VGIC3 pending bits separately. Alternatively,
we can also introduce another 'bool kvm->arch.vgic.save_vgic_v3_tables_in_progress'
to cover the new case.


Thanks,
Gavin

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Zenghui,

On 1/15/23 10:56 PM, Gavin Shan wrote:
> On 1/15/23 10:20 PM, Zenghui Yu wrote:
>> On 2022/11/10 18:49, Gavin Shan wrote:
>>> Enable ring-based dirty memory tracking on ARM64:
>>>
>>>   - Enable CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL.
>>>
>>>   - Enable CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP.
>>>
>>>   - Set KVM_DIRTY_LOG_PAGE_OFFSET for the ring buffer's physical page
>>>     offset.
>>>
>>>   - Add ARM64 specific kvm_arch_allow_write_without_running_vcpu() to
>>>     keep the site of saving vgic/its tables out of the no-running-vcpu
>>>     radar.
>>
>> And we have KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.. On receiving it, the
>> emulated VGIC will write all pending bits (if any) into pending tables
>> (which reside in guest memory) and doesn't require a running vcpu
>> context.
>>
>> The no-running-vcpu WARN can be triggered with the
>> kvm-unit-tests/its-pending-migration case. I run it using QEMU, which
>> has nothing to do with the dirty ring atm.
>>
>> Or are there already discussions about it that I haven't noticed?
>>
>> |void mark_page_dirty_in_slot(struct kvm *kvm,
>> |                 const struct kvm_memory_slot *memslot,
>> |                 gfn_t gfn)
>> |{
>> |    WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));
>>
> 
> It's a new case we never noticed. Could you please share the QEMU command lines
> to start the guest? I need to reproduce the issue on my side firstly.
> 
> The fix would be simply to extending kvm->arch.vgic.save_its_tables_in_progress
> from 'bool' to a bit map (e.g. kvm->arch.vgic.dirty_guest_memory_flags) and introduce
> two separate flags for ITS table and VGIC3 pending bits separately. Alternatively,
> we can also introduce another 'bool kvm->arch.vgic.save_vgic_v3_tables_in_progress'
> to cover the new case.
> 

Thanks for reporting the issue. I can reproduce the issue on my side. I will post
fixes soon.

Thanks,
Gavin

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Gavin Shan]

Hi Zenghui,

On 1/15/23 10:56 PM, Gavin Shan wrote:
> On 1/15/23 10:20 PM, Zenghui Yu wrote:
>> On 2022/11/10 18:49, Gavin Shan wrote:
>>> Enable ring-based dirty memory tracking on ARM64:
>>>
>>>   - Enable CONFIG_HAVE_KVM_DIRTY_RING_ACQ_REL.
>>>
>>>   - Enable CONFIG_NEED_KVM_DIRTY_RING_WITH_BITMAP.
>>>
>>>   - Set KVM_DIRTY_LOG_PAGE_OFFSET for the ring buffer's physical page
>>>     offset.
>>>
>>>   - Add ARM64 specific kvm_arch_allow_write_without_running_vcpu() to
>>>     keep the site of saving vgic/its tables out of the no-running-vcpu
>>>     radar.
>>
>> And we have KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.. On receiving it, the
>> emulated VGIC will write all pending bits (if any) into pending tables
>> (which reside in guest memory) and doesn't require a running vcpu
>> context.
>>
>> The no-running-vcpu WARN can be triggered with the
>> kvm-unit-tests/its-pending-migration case. I run it using QEMU, which
>> has nothing to do with the dirty ring atm.
>>
>> Or are there already discussions about it that I haven't noticed?
>>
>> |void mark_page_dirty_in_slot(struct kvm *kvm,
>> |                 const struct kvm_memory_slot *memslot,
>> |                 gfn_t gfn)
>> |{
>> |    WARN_ON_ONCE(!vcpu && !kvm_arch_allow_write_without_running_vcpu(kvm));
>>
> 
> It's a new case we never noticed. Could you please share the QEMU command lines
> to start the guest? I need to reproduce the issue on my side firstly.
> 
> The fix would be simply to extending kvm->arch.vgic.save_its_tables_in_progress
> from 'bool' to a bit map (e.g. kvm->arch.vgic.dirty_guest_memory_flags) and introduce
> two separate flags for ITS table and VGIC3 pending bits separately. Alternatively,
> we can also introduce another 'bool kvm->arch.vgic.save_vgic_v3_tables_in_progress'
> to cover the new case.
> 

Please try the following series to see if the unexpected warning message disappears
or not.

https://lore.kernel.org/kvmarm/20230116040405.260935-1-gshan@redhat.com/T/#t

Thanks,
Gavin

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Zenghui Yu]

On 2023/1/15 19:56, Gavin Shan wrote:
> It's a new case we never noticed. Could you please share the QEMU 
> command lines
> to start the guest? I need to reproduce the issue on my side firstly.

I execute it under the kvm-unit-tests directory:

QEMU=/path/to/qemu-system-aarch64 ACCEL=kvm arch=arm64 ./run_tests.sh

and the QEMU command and the verbose messages are recorded in
logs/its-pending-migration.log. The key to reproduce is there should be
at least one pending LPI when we handle
KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES (which can be achieved by the
its-pending-migration test).

Re: [PATCH v10 4/7] KVM: arm64: Enable ring-based dirty memory tracking

[Zenghui Yu]

On 2023/1/16 12:09, Gavin Shan wrote:
> Please try the following series to see if the unexpected warning message 
> disappears
> or not.
> 
> https://lore.kernel.org/kvmarm/20230116040405.260935-1-gshan@redhat.com/T/#t

Ah, I will have a look. Thanks Gavin for the quick fixes!