| Message ID | 1585084154-29461-14-git-send-email-kwankhede@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add migration support for VFIO devices | expand |
On Wed, 25 Mar 2020 02:39:11 +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> > --- > 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? Why do we return success when we want to start logging if we haven't started logging? > + > + 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, 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); > } > } >
* 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> > --- > 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; > + } > + > + 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; > + } > + > 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); Shouldn't you check the return value? > + > 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); > } > } > > -- > 2.7.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 3/27/2020 12:40 AM, Alex Williamson wrote: > On Wed, 25 Mar 2020 02:39:11 +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> >> --- >> 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); >> } >> } >> >
On 4/2/2020 12:33 AM, Dr. David Alan Gilbert wrote: > * 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> >> --- >> 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; >> + } >> + >> + 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; >> + } >> + >> 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); > > Shouldn't you check the return value? > Even if return value is checked, it will be ignored and this function returns void. Thanks, Kirti >> + >> 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); >> } >> } >> >> -- >> 2.7.0 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
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; + } + + 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; + } + 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); } }
Call VFIO_IOMMU_DIRTY_PAGES ioctl to start and stop dirty pages tracking for VFIO devices. Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com> --- hw/vfio/migration.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)