Re: [PATCH] vfio/migrate: Move switch of dirty tracking into vfio_memory_listener

[Keqian Zhu <zhukeqian1@huawei.com>]

On 2021/1/29 15:49, Paolo Bonzini wrote:
> On 28/01/21 21:02, Dr. David Alan Gilbert wrote:
>> * Paolo Bonzini (pbonzini@redhat.com) wrote:
>>> On 11/01/21 08:34, Keqian Zhu wrote:
>>>> +static void vfio_listener_log_start(MemoryListener *listener,
>>>> +                                    MemoryRegionSection *section,
>>>> +                                    int old, int new)
>>>> +{
>>>> +    VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>>>> +
>>>> +    vfio_set_dirty_page_tracking(container, true);
>>>> +}
>>>
>>> This would enable dirty page tracking also just for having a framebuffer
>>> (DIRTY_MEMORY_VGA).  Technically it would be correct, but it would also be
>>> more heavyweight than expected.
>>
>> Wouldn't that only happen on emulated video devices?
> 
> Yes, but still it's not impossible to have both an emulated VGA and an assigned GPU or vGPU.
> 
>>> In order to only cover live migration, you can use the log_global_start and
>>> log_global_stop callbacks instead.
>>>
>>> If you want to use log_start and log_stop, you need to add respectively
>>>
>>>      if (old != 0) {
>>>          return;
>>>      }
>>>
>>> and
>>>
>>>      if (new != 0) {
>>>          return;
>>>      }
>>
>> Why 0, wouldn't you be checking for DIRTY_LOG_MIGRATION somewhere?
> 
> Actually thinking more about it log_start/log_stop are just wrong, because they would be called many times, for each MemoryRegionSection.

Agree. This will be called for each MemoryRegionSection and each time when dirty_log_mask changed.

KVM uses log_start/log_stop, because it can start dirty tracking for every memslot individually, but vfio just has global start/stop semantics.

Anyway, use global start/stop is correct choice.

Thanks,
Keqian

> 
> Paolo
> 
> .
>