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

[Marc Zyngier <maz@kernel.org>]

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...