| Message ID | 1592684486-18511-13-git-send-email-kwankhede@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add migration support for VFIO devices | expand |
On Sun, 21 Jun 2020 01:51:21 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking > for VFIO devices. > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index faacea5327cb..e0fbb3a01855 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -11,6 +11,7 @@ > #include "qemu/main-loop.h" > #include "qemu/cutils.h" > #include <linux/vfio.h> > +#include <sys/ioctl.h> > > #include "sysemu/runstate.h" > #include "hw/vfio/vfio-common.h" > @@ -329,6 +330,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) I find 'start' functions that may also stop something a bit confusing. Maybe vfio_toggle_dirty_page_tracking()? > +{ > + 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 -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 ret; > +} > + > /* ---------------------------------------------------------------------- */ > > static int vfio_save_setup(QEMUFile *f, void *opaque) > @@ -360,6 +387,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > return ret; > } > > + ret = vfio_start_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); > @@ -375,6 +407,8 @@ static void vfio_save_cleanup(void *opaque) > VFIODevice *vbasedev = opaque; > VFIOMigration *migration = vbasedev->migration; > > + vfio_start_dirty_page_tracking(vbasedev, false); I suppose we can't do anything useful if stopping dirty page tracking fails? > + > if (migration->region.mmaps) { > vfio_region_unmap(&migration->region); > } > @@ -706,6 +740,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); > } > } >
* Cornelia Huck (cohuck@redhat.com) wrote: > On Sun, 21 Jun 2020 01:51:21 +0530 > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking > > for VFIO devices. > > > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > > index faacea5327cb..e0fbb3a01855 100644 > > --- a/hw/vfio/migration.c > > +++ b/hw/vfio/migration.c > > @@ -11,6 +11,7 @@ > > #include "qemu/main-loop.h" > > #include "qemu/cutils.h" > > #include <linux/vfio.h> > > +#include <sys/ioctl.h> > > > > #include "sysemu/runstate.h" > > #include "hw/vfio/vfio-common.h" > > @@ -329,6 +330,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) > > I find 'start' functions that may also stop something a bit confusing. > Maybe vfio_toggle_dirty_page_tracking()? I don't think toggle is any better; I always think of toggle as flipping the state to the other state. vfio_set_dirty_page_tracking maybe? Dave > > +{ > > + 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 -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 ret; > > +} > > + > > /* ---------------------------------------------------------------------- */ > > > > static int vfio_save_setup(QEMUFile *f, void *opaque) > > @@ -360,6 +387,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > > return ret; > > } > > > > + ret = vfio_start_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); > > @@ -375,6 +407,8 @@ static void vfio_save_cleanup(void *opaque) > > VFIODevice *vbasedev = opaque; > > VFIOMigration *migration = vbasedev->migration; > > > > + vfio_start_dirty_page_tracking(vbasedev, false); > > I suppose we can't do anything useful if stopping dirty page tracking > fails? > > > + > > if (migration->region.mmaps) { > > vfio_region_unmap(&migration->region); > > } > > @@ -706,6 +740,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); > > } > > } > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, 23 Jun 2020 12:01:25 +0100 "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote: > * Cornelia Huck (cohuck@redhat.com) wrote: > > On Sun, 21 Jun 2020 01:51:21 +0530 > > Kirti Wankhede <kwankhede@nvidia.com> wrote: > > > > > Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking > > > for VFIO devices. > > > > > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > --- > > > hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > > > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > > > index faacea5327cb..e0fbb3a01855 100644 > > > --- a/hw/vfio/migration.c > > > +++ b/hw/vfio/migration.c > > > @@ -11,6 +11,7 @@ > > > #include "qemu/main-loop.h" > > > #include "qemu/cutils.h" > > > #include <linux/vfio.h> > > > +#include <sys/ioctl.h> > > > > > > #include "sysemu/runstate.h" > > > #include "hw/vfio/vfio-common.h" > > > @@ -329,6 +330,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) > > > > I find 'start' functions that may also stop something a bit confusing. > > Maybe vfio_toggle_dirty_page_tracking()? > > I don't think toggle is any better; I always think of toggle as flipping > the state to the other state. > vfio_set_dirty_page_tracking maybe? Sounds good to me.
On Sun, 21 Jun 2020 01:51:21 +0530 Kirti Wankhede <kwankhede@nvidia.com> wrote: > Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking > for VFIO devices. > > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index faacea5327cb..e0fbb3a01855 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -11,6 +11,7 @@ > #include "qemu/main-loop.h" > #include "qemu/cutils.h" > #include <linux/vfio.h> > +#include <sys/ioctl.h> > > #include "sysemu/runstate.h" > #include "hw/vfio/vfio-common.h" > @@ -329,6 +330,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 -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 ret; > +} What happens when we have a device that supports a migration region paired with an IOMMU that doesn't report dirty page tracking, shouldn't we have tested container->dirty_pages_supported somewhere in the process of determining if the device is migratable? Thanks, Alex > + > /* ---------------------------------------------------------------------- */ > > static int vfio_save_setup(QEMUFile *f, void *opaque) > @@ -360,6 +387,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > return ret; > } > > + ret = vfio_start_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); > @@ -375,6 +407,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); > } > @@ -706,6 +740,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); > } > } >
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index faacea5327cb..e0fbb3a01855 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -11,6 +11,7 @@ #include "qemu/main-loop.h" #include "qemu/cutils.h" #include <linux/vfio.h> +#include <sys/ioctl.h> #include "sysemu/runstate.h" #include "hw/vfio/vfio-common.h" @@ -329,6 +330,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 -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 ret; +} + /* ---------------------------------------------------------------------- */ static int vfio_save_setup(QEMUFile *f, void *opaque) @@ -360,6 +387,11 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) return ret; } + ret = vfio_start_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); @@ -375,6 +407,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); } @@ -706,6 +740,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); } }