Re: [PATCH v5 3/7] KVM: x86: Allow to use bitmap in ring-based dirty page tracking

[Gavin Shan <gshan@redhat.com>]

Hi Peter/Oliver,

On 10/11/22 11:56 AM, Gavin Shan wrote:
> On 10/11/22 9:12 AM, Oliver Upton wrote:
>> On Mon, Oct 10, 2022 at 08:20:29PM -0400, Peter Xu wrote:
>>> On Mon, Oct 10, 2022 at 11:58:22PM +0000, Oliver Upton wrote:
>>>> I think this further drives the point home -- there's zero need for the
>>>> bitmap with dirty ring on x86, so why even support it? The proposal of
>>>> ALLOW_BITMAP && DIRTY_RING should be arm64-specific. Any other arch that
>>>> needs to dirty memory outside of a vCPU context can opt-in to the
>>>> behavior.
>>>
>>> Yeah that sounds working too, but it'll be slightly hackish as then the
>>> user app will need some "#ifdef ARM64" blocks for e.g. sync dirty bitmap.
>>> With the new cap the user app can implement the whole ring with generic
>>> code.
>>
>> Isn't the current route of exposing ALLOW_BITMAP on other arches for no
>> reason headed in exactly that direction? Userspace would need to know if
>> it _really_ needs the dirty bitmap in addition to the dirty ring, which
>> could take the form of architecture ifdeffery.
>>
>> OTOH, if the cap is only exposed when it is absolutely necessary, an
>> arch-generic live migration implementation could enable the cap whenever
>> it is advertized and scan the bitmap accordingly.
>>
>> The VMM must know something about the architecture it is running on, as
>> it calls KVM_DEV_ARM_ITS_SAVE_TABLES after all...
>>
> 
> It looks good to me by using CONFIG_HAVE_KVM_DIRTY_RING_USE_BITMAP to
> opt-in KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP. The most important point is
> to ensure 'kvm->dirty_ring_with_bitmap == true' when dirty ring capability
> is enabled. In this way, we can fail early when KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP
> isn't enabled on attempt to enable dirty ring capability.
> 
> If both of you agree, I will integrate the suggested code changes in
> next respin, with necessary tweak.
> 
> - In kvm_vm_ioctl_enable_cap_generic(), 'kvm->dirty_ring_with_bitmap' is
>    updated to 'true' unconditionally.
> 
>    static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
>                                               struct kvm_enable_cap *cap)
>    {
>        :
>        case KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP:
>             kvm->dirty_ring_with_bitmap = true;
>             return 0;
>    }
> 
> - In mark_page_dirty_in_slot(), we need comprehensive checks like below.
> 
>    void mark_page_dirty_in_slot(struct kvm *kvm,
>                                 const struct kvm_memory_slot *memslot,
>                                 gfn_t gfn)
>    {
> #ifdef CONFIG_HAVE_KVM_DIRTY_RING
>        if (WARN_ON_ONCE(vcpu && vcpu->kvm != kvm))
>            return;
> 
> #ifndef CONFIG_HAVE_KVM_DIRTY_RING_WITH_BITMAP
>        if (WARN_ON_ONCE(!vcpu))
>            return;
> #endif
> #endif
> 
>    }
> 
> - Use kvm_dirty_ring_exclusive(), which was suggested by Peter before.
>    The function is used in various spots to allow the dirty bitmap is
>    created and accessed.
> 
>    bool kvm_dirty_ring_exclusive(struct kvm *kvm)
>    {
>        return kvm->dirty_ring_size && !kvm->dirty_ring_with_bitmap;
>    }
> 
> 

I've included Oliver's suggested changes into v6, which was just posted:

https://lore.kernel.org/kvmarm/3123a04f-a674-782b-9e9b-0baf3db49ebc@redhat.com/

Please find your time to review v6 directly, thanks!

>>> Also more flexible to expose it as generic cap? E.g., one day x86 can
>>> enable this too for whatever reason (even though I don't think so..).
>>
>> I had imagined something like this patch where the arch opts-in to some
>> generic construct if it *requires* the use of both the ring and bitmap
>> (very rough sketch).
>>

Thanks,
Gavin