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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Marc,

On 11/11/22 11:19 PM, Marc Zyngier wrote:
On Thu, 10 Nov 2022 23:47:41 +0000,
Gavin Shan <gshan@xxxxxxxxxx> wrote:

commit b05377ecbe003f12c8b79846fa3a300401dcab68 (HEAD -> kvm/arm64_dirtyring)
Author: Gavin Shan <gshan@xxxxxxxxxx>
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@xxxxxxxxxx>
     Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>

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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux