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
