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

[Marc Zyngier <maz@kernel.org>]

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.