Re: [PATCH v16 QEMU 13/16] vfio: Add function to start and stop dirty pages tracking

[Kirti Wankhede]

On 3/27/2020 12:40 AM, Alex Williamson wrote:
> On Wed, 25 Mar 2020 02:39:11 +0530
> Kirti Wankhede <address@hidden> wrote:
>
> > Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking
> > for VFIO devices.
> >
> > Signed-off-by: Kirti Wankhede <address@hidden>
> > ---
> >   hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index ab295d25620e..1827b7cfb316 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -9,6 +9,7 @@
> >   
> >   #include "qemu/osdep.h"
> >   #include "qemu/main-loop.h"
> > +#include <sys/ioctl.h>
> >   #include <linux/vfio.h>
> >   
> >   #include "sysemu/runstate.h"
> > @@ -296,6 +297,32 @@ static int vfio_load_device_config_state(QEMUFile *f, void 
> > *opaque)
> >       return qemu_file_get_error(f);
> >   }
> >   
> > +static int vfio_start_dirty_page_tracking(VFIODevice *vbasedev, bool start)
> > +{
> > +    int ret;
> > +    VFIOContainer *container = vbasedev->group->container;
> > +    struct vfio_iommu_type1_dirty_bitmap dirty = {
> > +        .argsz = sizeof(dirty),
> > +    };
> > +
> > +    if (start) {
> > +        if (vbasedev->device_state & VFIO_DEVICE_STATE_SAVING) {
> > +            dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_START;
> > +        } else {
> > +            return 0;
> > +        }
> > +    } else {
> > +            dirty.flags = VFIO_IOMMU_DIRTY_PAGES_FLAG_STOP;
> > +    }
>
> Dirty logging and device saving are logically separate, why do we link
> them here?

Dirty logging is associated with migration state and in vfio case we get 
to know that migration state for per device. We don't know which device 
is first or last. So start dirty page logging .save_setup. But this 
function can be called from other places also, so for sanity check start 
dirty pages tracking only when VFIO_DEVICE_STATE_SAVING flag is set.

> Why do we return success when we want to start logging if we haven't
> started logging?

It should be -EINVAL since dirty page tracking shouldn't start if 
VFIO_DEVICE_STATE_SAVING flag is not set, i.e. devices are not in SAVING 
state.

> > +
> > +    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 ret;
> > +}
> > +
> >   /* ---------------------------------------------------------------------- */
> >   
> >   static int vfio_save_setup(QEMUFile *f, void *opaque)
> > @@ -330,6 +357,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque)
> >        */
> >       qemu_put_be64(f, migration->region.size);
> >   
> > +    ret = vfio_start_dirty_page_tracking(vbasedev, true);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
>
> Haven't we corrupted the migration stream by exiting here?  Maybe this
> implies the entire migration fails, therefore we don't need to add the
> end marker?  Thanks,

If returned error here means migration fails.

Thanks,
Kirti

> Alex
>
> >       qemu_put_be64(f, VFIO_MIG_FLAG_END_OF_STATE);
> >   
> >       ret = qemu_file_get_error(f);
> > @@ -346,6 +378,8 @@ static void vfio_save_cleanup(void *opaque)
> >       VFIODevice *vbasedev = opaque;
> >       VFIOMigration *migration = vbasedev->migration;
> >   
> > +    vfio_start_dirty_page_tracking(vbasedev, false);
> > +
> >       if (migration->region.mmaps) {
> >           vfio_region_unmap(&migration->region);
> >       }
> > @@ -669,6 +703,8 @@ static void vfio_migration_state_notifier(Notifier 
> > *notifier, void *data)
> >           if (ret) {
> >               error_report("%s: Failed to set state RUNNING", vbasedev->name);
> >           }
> > +
> > +        vfio_start_dirty_page_tracking(vbasedev, false);
> >       }
> >   }
> >