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

[Keqian Zhu]

Hi Kirti,

On 2021/1/28 5:03, Kirti Wankhede wrote:
> 
> 
> On 1/11/2021 1:04 PM, Keqian Zhu wrote:
>> For now the switch of vfio dirty page tracking is integrated into
>> the vfio_save_handler, it causes some problems [1].
>>
> 
> Sorry, I missed [1] mail, somehow it didn't landed in my inbox.
> 
>> The object of dirty tracking is guest memory, but the object of
>> the vfio_save_handler is device state. This mixed logic produces
>> unnecessary coupling and conflicts:
>>
>> 1. Coupling: Their saving granule is different (perVM vs perDevice).
>>     vfio will enable dirty_page_tracking for each devices, actually
>>     once is enough.
> 
> That's correct, enabling dirty page tracking once is enough. But log_start 
> and log_stop gets called on address space update transaction, region_add() or 
> region_del(), at this point migration may not be active. We don't want to 
> allocate bitmap memory in kernel for lifetime of VM, without knowing 
> migration will be happen or not. vfio_iommu_type1 module should allocate 
> bitmap memory only while migration is active.
> 
Yeah, we can use global start/stop callbacks as suggested by Paolo, which 
solves this problem.

> Paolo's suggestion here to use log_global_start and log_global_stop callbacks 
> seems correct here. But at this point vfio device state is not yet changed to 
> |_SAVING as you had identified it in [1]. May be we can start tracking bitmap 
> in iommu_type1 module while device is not yet _SAVING, but getting dirty 
> bitmap while device is yet not in _SAVING|_RUNNING state doesn't seem optimal 
> solution.
> 
> Pasting here your question from [1]
> 
>> Before start dirty tracking, we will check and ensure that the device
>>  is at _SAVING state and return error otherwise.  But the question is
>>  that what is the rationale?  Why does the VFIO_IOMMU_DIRTY_PAGES
>> ioctl have something to do with the device state?
> 
> Lets walk through the types of devices we are supporting:
> 1. mdev devices without IOMMU backed device
>     Vendor driver pins pages as and when required during runtime. We can say 
> that vendor driver is smart which identifies the pages to pin. We are good 
> here.
> 
> 2. mdev device with IOMMU backed device
>     This is similar to vfio-pci, direct assigned device, where all pages are 
> pinned at VM bootup. Vendor driver is not smart, so bitmap query will report 
> all pages dirty always. If --auto-converge is not set, VM stucks infinitely 
> in pre-copy phase. This is known to us.
> 
little question here ;-) . Why auto-converge (slow down vCPU) helps to ease the 
case of full dirty?

> 3. mdev device with IOMMU backed device with smart vendor driver
>     In this case as well all pages are pinned at VM bootup, but vendor driver 
> is smart to identify the pages and pin them explicitly.
> Pages can be pinned anytime, i.e. during normal VM runtime or on setting 
> _SAVING flag (entering pre-copy phase) or while in iterative pre-copy phase. 
> There is no restriction based on these phases for calling vfio_pin_pages(). 
> Vendor driver can start pinning pages based on its device state when _SAVING 
> flag is set. In that case, if dirty bitmap is queried before that then it 
> will report all sysmem as dirty with an unnecessary copy of sysmem.
> As an optimal solution, I think its better to query bitmap only after all 
> vfio devices are in pre-copy phase, i.e. after _SAVING flag is set.
OK, I get your idea. But Qemu assumes all pages are dirty initially, this seems 
not a problem.
Let's assume we have a device of type 3, and this device starts to pin pages on 
setting _SAVING flag.

Before this patch, the work flow is:
{
ram_save_setup()
    memory_global_dirty_log_start():  start dirty tracking excludes vfio part.
    migration_bitmap_sync_precopy():  try to sync dirty log from kvm, vhost 
etc, including vfio (as all device saving is not satisfied, fail to get log 
from vfio). The result is that bitmap of ramblock is all dirty.

vfio_save_setup() of this device
    vfio_migration_set_state(): Add SAVING state to this device, and vfio 
starts to log dirty page of this device.

first round (i.e. bulk stage) of ram saving: only handle dirty log which is 
collected from the first call of migration_bitmap_sync_precopy().

iterative stage of ram saving: when the remaining dirty log is less than 
threshold, call migration_bitmap_sync_precopy() again. At this stage, all 
device is saving, so success to get log from vfio.
}

With this patch, the work flow is:
{
ram_save_setup()
    memory_global_dirty_log_start():  start dirty tracking includes vfio part.
    migration_bitmap_sync_precopy():  try to sync dirty log from kvm, vhost 
etc, including vfio (as all device saving is not checked, success to get full 
dirty log from vfio). The result is that bitmap of ramblock is all dirty.
vfio_save_setup() of this device
        vfio_migration_set_state(): Add SAVING state to this device, and vfio 
starts to log dirty page of this device.

first round of ram saving: only handles dirty log which is collected from the 
first call of migration_bitmap_sync_precopy().

iterative stage of ram saving: when the remaining dirty log is less than a 
threshold, calls migration_bitmap_sync_precopy() again. At this stage, all 
device is saving, so success to get log from vfio.
}

We can see that there is no unnecessary copy of guest memory with this patch.

> 
>> 2. Conflicts: The ram_save_setup() traverses all memory_listeners
>>     to execute their log_start() and log_sync() hooks to get the
>>     first round dirty bitmap, which is used by the bulk stage of
>>     ram saving. However, it can't get dirty bitmap from vfio, as
>>     @savevm_ram_handlers is registered before @vfio_save_handler.
>>
> Right, but it can get dirty bitmap from vfio device in it's iterative callback
> ram_save_pending ->
>     migration_bitmap_sync_precopy() .. ->
>          vfio_listerner_log_sync
> 
Yeah, this work flow is OK. But we are not able to handle vfio dirty log at the 
first round of ram saving.

We plan to add a new interface named manual_log_clear in vfio[1]. If this 
interface is enabled, kernel vfio
doesn't automatically clear and repopulate dirty bitmap when report dirty log 
to Qemu. These actions will
be triggered by Qemu when it invokes manual_log_clear 
(memory_region_clear_dirty_bitmap()) before handles
the dirty log.

Under the following conditions:
(1)Qemu can handles vfio dirty log at the first round of ram saving.
(2)device is of type1.
(3)manual_log_clear is enabled.
(3)device driver unpins some scopes during the first round of ram saving 
(before Qemu actually handles these scopes).

Then these unpinned scope is not reported to Qemu at iterative stage 
(manual_log_clear clears them and re-population
does not contains them), which avoid unnecessary copy of guest memory.

Thanks,
Keqian

[1]https://lore.kernel.org/kvmarm/20210128151742.18840-1-zhukeqian1@huawei.com/


> Thanks,
> Kirti
> 
>> Move the switch of vfio dirty_page_tracking into vfio_memory_listener
>> can solve above problems. Besides, Do not require devices in SAVING
>> state for vfio_sync_dirty_bitmap().
>>
>> [1] https://www.spinics.net/lists/kvm/msg229967.html
>>
>> Reported-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>>   hw/vfio/common.c    | 53 +++++++++++++++++++++++++++++++++++++--------
>>   hw/vfio/migration.c | 35 ------------------------------
>>   2 files changed, 44 insertions(+), 44 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 6ff1daa763..9128cd7ee1 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -311,7 +311,7 @@ bool vfio_mig_active(void)
>>       return true;
>>   }
>>   -static bool vfio_devices_all_saving(VFIOContainer *container)
>> +static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>>   {
>>       VFIOGroup *group;
>>       VFIODevice *vbasedev;
>> @@ -329,13 +329,8 @@ static bool vfio_devices_all_saving(VFIOContainer 
>> *container)
>>                   return false;
>>               }
>>   -            if (migration->device_state & VFIO_DEVICE_STATE_SAVING) {
>> -                if ((vbasedev->pre_copy_dirty_page_tracking == 
>> ON_OFF_AUTO_OFF)
>> -                    && (migration->device_state & 
>> VFIO_DEVICE_STATE_RUNNING)) {
>> -                        return false;
>> -                }
>> -                continue;
>> -            } else {
>> +            if ((vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF)
>> +                && (migration->device_state & VFIO_DEVICE_STATE_RUNNING)) {
>>                   return false;
>>               }
>>           }
>> @@ -987,6 +982,44 @@ static void vfio_listener_region_del(MemoryListener 
>> *listener,
>>       }
>>   }
>>   +static void vfio_set_dirty_page_tracking(VFIOContainer *container, bool 
>> start)
>> +{
>> +    int ret;
>> +    struct vfio_iommu_type1_dirty_bitmap dirty = {
>> +        .argsz = sizeof(dirty),
>> +    };
>> +
>> +    if (start) {
>> +        dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
>> +    } else {
>> +        dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP;
>> +    }
>> +
>> +    ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>> +    if (ret) {
>> +        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>> +                     dirty.flags, errno);
>> +    }
>> +}
>> +
>> +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);
>> +}
>> +
>> +static void vfio_listener_log_stop(MemoryListener *listener,
>> +                                   MemoryRegionSection *section,
>> +                                   int old, int new)
>> +{
>> +    VFIOContainer *container = container_of(listener, VFIOContainer, 
>> listener);
>> +
>> +    vfio_set_dirty_page_tracking(container, false);
>> +}
>> +
>>   static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>>                                    uint64_t size, ram_addr_t ram_addr)
>>   {
>> @@ -1128,7 +1161,7 @@ static void vfio_listerner_log_sync(MemoryListener 
>> *listener,
>>           return;
>>       }
>>   -    if (vfio_devices_all_saving(container)) {
>> +    if (vfio_devices_all_dirty_tracking(container)) {
>>           vfio_sync_dirty_bitmap(container, section);
>>       }
>>   }
>> @@ -1136,6 +1169,8 @@ static void vfio_listerner_log_sync(MemoryListener 
>> *listener,
>>   static const MemoryListener vfio_memory_listener = {
>>       .region_add = vfio_listener_region_add,
>>       .region_del = vfio_listener_region_del,
>> +    .log_start = vfio_listener_log_start,
>> +    .log_stop = vfio_listener_log_stop,
>>       .log_sync = vfio_listerner_log_sync,
>>   };
>>   diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
>> index 00daa50ed8..c0f646823a 100644
>> --- a/hw/vfio/migration.c
>> +++ b/hw/vfio/migration.c
>> @@ -395,40 +395,10 @@ static int vfio_load_device_config_state(QEMUFile *f, 
>> void *opaque)
>>       return qemu_file_get_error(f);
>>   }
>>   -static int vfio_set_dirty_page_tracking(VFIODevice *vbasedev, bool start)
>> -{
>> -    int ret;
>> -    VFIOMigration *migration = vbasedev->migration;
>> -    VFIOContainer *container = vbasedev->group->container;
>> -    struct vfio_iommu_type1_dirty_bitmap dirty = {
>> -        .argsz = sizeof(dirty),
>> -    };
>> -
>> -    if (start) {
>> -        if (migration->device_state & VFIO_DEVICE_STATE_SAVING) {
>> -            dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
>> -        } else {
>> -            return -EINVAL;
>> -        }
>> -    } else {
>> -            dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP;
>> -    }
>> -
>> -    ret = ioctl(container->fd, VFIO_IOMMU_DIRTY_PAGES, &dirty);
>> -    if (ret) {
>> -        error_report("Failed to set dirty tracking flag 0x%x errno: %d",
>> -                     dirty.flags, errno);
>> -        return -errno;
>> -    }
>> -    return ret;
>> -}
>> -
>>   static void vfio_migration_cleanup(VFIODevice *vbasedev)
>>   {
>>       VFIOMigration *migration = vbasedev->migration;
>>   -    vfio_set_dirty_page_tracking(vbasedev, false);
>> -
>>       if (migration->region.mmaps) {
>>           vfio_region_unmap(&migration->region);
>>       }
>> @@ -469,11 +439,6 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
>>           return ret;
>>       }
>>   -    ret = vfio_set_dirty_page_tracking(vbasedev, true);
>> -    if (ret) {
>> -        return ret;
>> -    }
>> -
>>       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
>>         ret = qemu_file_get_error(f);
>>
> .
>