Re: [PATCH 4/9] vfio/migration: Skip pre-copy if dirty page tracking is not supported

[Avihai Horon]

On 5/20/2022 1:58 PM, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 5/12/22 16:43, Avihai Horon wrote:
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 21e8f9d4d4..d4b6653026 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -863,10 +863,17 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error 
> > **errp)
> >       struct vfio_region_info *info = NULL;
> >       int ret = -ENOTSUP;
> >
> > -    if (!vbasedev->enable_migration || !container->dirty_pages_supported) {
> > +    if (!vbasedev->enable_migration) {
> >           goto add_blocker;
> >       }
> >
> > +    if (!container->dirty_pages_supported) {
> > +        warn_report(
> > +            "%s: IOMMU of the device's VFIO container doesn't support dirty page 
> > tracking, migration pre-copy phase will be skipped",
> > +            vbasedev->name);
> Maybe warn_report_once(...) given that the following N devices would observe the
> same thing in theory.

Yes, you are right. Will change.

> > +        migrate_get_current()->skip_precopy = true;
> > +    }
> > +
> Perhaps it might be easier to reuse the existing knob to disable pre-copy
> per device that Kirti added some time ago, rather than changing migration core 
> just
> yet (e.g. you might wanna bail of the migration preemptively because the CPU is 
> dirtying
> too many pages for example?):
>
> if (!container->dirty_pages_supported) {
>      warn_report_once(...)
>      pre_copy_dirty_page_tracking = ON_OFF_AUTO_OFF;
> }

But this doesn't prevent the VFIO device from dirtying RAM pages during 
pre-copy phase.
The VFIO device can dirty RAM pages during pre-copy and it won't have a 
way to report the dirtied pages to QEMU and migration will be faulty.

Thanks.

> You might need to tackle the fact you will get when dirty_pages start/stop 
> ioctls
> returns you error messages. The errors is just because log_start() and 
> log_stop() simply
> fail because the ioctl doesn't exist, but everything else is fine -- at least 
> that's what
> I observed at least. Should be noted that it's a problem with the existing
> `-device vfio-pci host=XX:YY.ZZ,x-pre-copy-dirty-page-tracking=true` regardless 
> of this patch:
>
> void vfio_listener_log_global_start()
> {
>          if (vfio_devices_all_dirty_tracking(container)) {
>                  vfio_set_dirty_page_tracking(container, true);
>          }
> }
>
> ... And same for vfio_listener_log_global_stop() -- maybe a worthwhile 
> predecessor patch.