[v3,08/13] vfio/common: Add device dirty page tracking start/stop

Add device dirty page tracking start/stop functionality. This uses the
device DMA logging uAPI to start and stop dirty page tracking by device.

Device dirty page tracking is used only if all devices within a
container support device dirty page tracking.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
 hw/vfio/trace-events          |   1 +
 include/hw/vfio/vfio-common.h |   2 +
 3 files changed, 166 insertions(+), 3 deletions(-)

On 3/4/23 02:43, Joao Martins wrote:
> Add device dirty page tracking start/stop functionality. This uses the
> device DMA logging uAPI to start and stop dirty page tracking by device.
> 
> Device dirty page tracking is used only if all devices within a
> container support device dirty page tracking.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

The device attribute 'dirty_pages_supported' is only set in patch 12,
when all checks are done. That was a bit confusing when I started
looking. It should be fine anyhow.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
>   hw/vfio/trace-events          |   1 +
>   include/hw/vfio/vfio-common.h |   2 +
>   3 files changed, 166 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index d84e5fd86bb4..aa0df0604704 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -453,6 +453,22 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>       return true;
>   }
>   
> +static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (!vbasedev->dirty_pages_supported) {
> +                return false;
> +            }
> +        }
> +    }
> +
> +    return true;
> +}
> +
>   /*
>    * Check if all VFIO devices are running and migration is active, which is
>    * essentially equivalent to the migration being in pre-copy phase.
> @@ -1395,15 +1411,152 @@ static void vfio_dirty_tracking_init(VFIOContainer *container)
>       qemu_mutex_destroy(&container->tracking_mutex);
>   }
>   
> +static int vfio_devices_dma_logging_set(VFIOContainer *container,
> +                                        struct vfio_device_feature *feature)
> +{
> +    bool status = (feature->flags & VFIO_DEVICE_FEATURE_MASK) ==
> +                  VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> +    VFIODevice *vbasedev;
> +    VFIOGroup *group;
> +    int ret = 0;
> +
> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (vbasedev->dirty_tracking == status) {
> +                continue;
> +            }
> +
> +            ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> +            if (ret) {
> +                ret = -errno;
> +                error_report("%s: Failed to set DMA logging %s, err %d (%s)",
> +                             vbasedev->name, status ? "start" : "stop", ret,
> +                             strerror(errno));
> +                goto out;
> +            }
> +            vbasedev->dirty_tracking = status;
> +        }
> +    }
> +
> +out:
> +    return ret;
> +}
> +
> +static int vfio_devices_dma_logging_stop(VFIOContainer *container)
> +{
> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
> +                              sizeof(uint64_t))] = {};
> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
> +
> +    feature->argsz = sizeof(buf);
> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
> +
> +    return vfio_devices_dma_logging_set(container, feature);
> +}
> +
> +static struct vfio_device_feature *
> +vfio_device_feature_dma_logging_start_create(VFIOContainer *container)
> +{
> +    struct vfio_device_feature *feature;
> +    size_t feature_size;
> +    struct vfio_device_feature_dma_logging_control *control;
> +    struct vfio_device_feature_dma_logging_range *ranges;
> +    VFIODirtyTrackingRange *tracking = &container->tracking_range;
> +
> +    feature_size = sizeof(struct vfio_device_feature) +
> +                   sizeof(struct vfio_device_feature_dma_logging_control);
> +    feature = g_try_malloc0(feature_size);
> +    if (!feature) {
> +        errno = ENOMEM;
> +        return NULL;
> +    }
> +    feature->argsz = feature_size;
> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> +
> +    control = (struct vfio_device_feature_dma_logging_control *)feature->data;
> +    control->page_size = qemu_real_host_page_size();
> +
> +    /*
> +     * DMA logging uAPI guarantees to support at least a number of ranges that
> +     * fits into a single host kernel base page.
> +     */
> +    control->num_ranges = !!tracking->max32 + !!tracking->max64;
> +    ranges = g_try_new0(struct vfio_device_feature_dma_logging_range,
> +                        control->num_ranges);
> +    if (!ranges) {
> +        g_free(feature);
> +        errno = ENOMEM;
> +
> +        return NULL;
> +    }
> +
> +    control->ranges = (__aligned_u64)ranges;
> +    if (tracking->max32) {
> +        ranges->iova = tracking->min32;
> +        ranges->length = (tracking->max32 - tracking->min32) + 1;
> +        ranges++;
> +    }
> +    if (tracking->max64) {
> +        ranges->iova = tracking->min64;
> +        ranges->length = (tracking->max64 - tracking->min64) + 1;
> +    }
> +
> +    trace_vfio_device_dirty_tracking_start(control->num_ranges,
> +                                           tracking->min32, tracking->max32,
> +                                           tracking->min64, tracking->max64);
> +
> +    return feature;
> +}
> +
> +static void vfio_device_feature_dma_logging_start_destroy(
> +    struct vfio_device_feature *feature)
> +{
> +    struct vfio_device_feature_dma_logging_control *control =
> +        (struct vfio_device_feature_dma_logging_control *)feature->data;
> +    struct vfio_device_feature_dma_logging_range *ranges =
> +        (struct vfio_device_feature_dma_logging_range *)control->ranges;
> +
> +    g_free(ranges);
> +    g_free(feature);
> +}
> +
> +static int vfio_devices_dma_logging_start(VFIOContainer *container)
> +{
> +    struct vfio_device_feature *feature;
> +    int ret = 0;
> +
> +    vfio_dirty_tracking_init(container);
> +    feature = vfio_device_feature_dma_logging_start_create(container);
> +    if (!feature) {
> +        return -errno;
> +    }
> +
> +    ret = vfio_devices_dma_logging_set(container, feature);
> +    if (ret) {
> +        vfio_devices_dma_logging_stop(container);
> +    }
> +
> +    vfio_device_feature_dma_logging_start_destroy(feature);
> +
> +    return ret;
> +}
> +
>   static void vfio_listener_log_global_start(MemoryListener *listener)
>   {
>       VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>       int ret;
>   
> -    vfio_dirty_tracking_init(container);
> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> +        ret = vfio_devices_dma_logging_start(container);
> +    } else {
> +        ret = vfio_set_dirty_page_tracking(container, true);
> +    }
>   
> -    ret = vfio_set_dirty_page_tracking(container, true);
>       if (ret) {
> +        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> +                     ret, strerror(-ret));
>           vfio_set_migration_error(ret);
>       }
>   }
> @@ -1413,8 +1566,15 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>       VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>       int ret;
>   
> -    ret = vfio_set_dirty_page_tracking(container, false);
> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> +        ret = vfio_devices_dma_logging_stop(container);
> +    } else {
> +        ret = vfio_set_dirty_page_tracking(container, false);
> +    }
> +
>       if (ret) {
> +        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> +                     ret, strerror(-ret));
>           vfio_set_migration_error(ret);
>       }
>   }
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index d97a6de17921..7a7e0cfe5b23 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -105,6 +105,7 @@ vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t si
>   vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>   vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
>   vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64" - 0x%"PRIx64"]"
> +vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t max32, uint64_t min64, uint64_t max64) "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" - 0x%"PRIx64"]"
>   vfio_disconnect_container(int fd) "close container->fd=%d"
>   vfio_put_group(int fd) "close group->fd=%d"
>   vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 96791add2719..1cbbccd91e11 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -154,6 +154,8 @@ typedef struct VFIODevice {
>       VFIOMigration *migration;
>       Error *migration_blocker;
>       OnOffAuto pre_copy_dirty_page_tracking;
> +    bool dirty_pages_supported;
> +    bool dirty_tracking;
>   } VFIODevice;
>   
>   struct VFIODeviceOps {

On Sat,  4 Mar 2023 01:43:38 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> Add device dirty page tracking start/stop functionality. This uses the
> device DMA logging uAPI to start and stop dirty page tracking by device.
> 
> Device dirty page tracking is used only if all devices within a
> container support device dirty page tracking.
> 
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
>  hw/vfio/trace-events          |   1 +
>  include/hw/vfio/vfio-common.h |   2 +
>  3 files changed, 166 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index d84e5fd86bb4..aa0df0604704 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -453,6 +453,22 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>      return true;
>  }
>  
> +static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +
> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (!vbasedev->dirty_pages_supported) {
> +                return false;
> +            }
> +        }
> +    }
> +
> +    return true;
> +}
> +
>  /*
>   * Check if all VFIO devices are running and migration is active, which is
>   * essentially equivalent to the migration being in pre-copy phase.
> @@ -1395,15 +1411,152 @@ static void vfio_dirty_tracking_init(VFIOContainer *container)
>      qemu_mutex_destroy(&container->tracking_mutex);
>  }
>  
> +static int vfio_devices_dma_logging_set(VFIOContainer *container,
> +                                        struct vfio_device_feature *feature)
> +{
> +    bool status = (feature->flags & VFIO_DEVICE_FEATURE_MASK) ==
> +                  VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> +    VFIODevice *vbasedev;
> +    VFIOGroup *group;
> +    int ret = 0;
> +
> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            if (vbasedev->dirty_tracking == status) {
> +                continue;
> +            }
> +
> +            ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> +            if (ret) {
> +                ret = -errno;
> +                error_report("%s: Failed to set DMA logging %s, err %d (%s)",
> +                             vbasedev->name, status ? "start" : "stop", ret,
> +                             strerror(errno));
> +                goto out;
> +            }

Exiting and returning an error on the first failure when starting dirty
tracking makes sense.  Does that behavior still make sense for the stop
path?  Maybe since we only support a single device it doesn't really
matter, but this needs to be revisited for multiple devices.  Thanks,

Alex

> +            vbasedev->dirty_tracking = status;
> +        }
> +    }
> +
> +out:
> +    return ret;
> +}
> +
> +static int vfio_devices_dma_logging_stop(VFIOContainer *container)
> +{
> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
> +                              sizeof(uint64_t))] = {};
> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
> +
> +    feature->argsz = sizeof(buf);
> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
> +
> +    return vfio_devices_dma_logging_set(container, feature);
> +}
> +
> +static struct vfio_device_feature *
> +vfio_device_feature_dma_logging_start_create(VFIOContainer *container)
> +{
> +    struct vfio_device_feature *feature;
> +    size_t feature_size;
> +    struct vfio_device_feature_dma_logging_control *control;
> +    struct vfio_device_feature_dma_logging_range *ranges;
> +    VFIODirtyTrackingRange *tracking = &container->tracking_range;
> +
> +    feature_size = sizeof(struct vfio_device_feature) +
> +                   sizeof(struct vfio_device_feature_dma_logging_control);
> +    feature = g_try_malloc0(feature_size);
> +    if (!feature) {
> +        errno = ENOMEM;
> +        return NULL;
> +    }
> +    feature->argsz = feature_size;
> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> +
> +    control = (struct vfio_device_feature_dma_logging_control *)feature->data;
> +    control->page_size = qemu_real_host_page_size();
> +
> +    /*
> +     * DMA logging uAPI guarantees to support at least a number of ranges that
> +     * fits into a single host kernel base page.
> +     */
> +    control->num_ranges = !!tracking->max32 + !!tracking->max64;
> +    ranges = g_try_new0(struct vfio_device_feature_dma_logging_range,
> +                        control->num_ranges);
> +    if (!ranges) {
> +        g_free(feature);
> +        errno = ENOMEM;
> +
> +        return NULL;
> +    }
> +
> +    control->ranges = (__aligned_u64)ranges;
> +    if (tracking->max32) {
> +        ranges->iova = tracking->min32;
> +        ranges->length = (tracking->max32 - tracking->min32) + 1;
> +        ranges++;
> +    }
> +    if (tracking->max64) {
> +        ranges->iova = tracking->min64;
> +        ranges->length = (tracking->max64 - tracking->min64) + 1;
> +    }
> +
> +    trace_vfio_device_dirty_tracking_start(control->num_ranges,
> +                                           tracking->min32, tracking->max32,
> +                                           tracking->min64, tracking->max64);
> +
> +    return feature;
> +}
> +
> +static void vfio_device_feature_dma_logging_start_destroy(
> +    struct vfio_device_feature *feature)
> +{
> +    struct vfio_device_feature_dma_logging_control *control =
> +        (struct vfio_device_feature_dma_logging_control *)feature->data;
> +    struct vfio_device_feature_dma_logging_range *ranges =
> +        (struct vfio_device_feature_dma_logging_range *)control->ranges;
> +
> +    g_free(ranges);
> +    g_free(feature);
> +}
> +
> +static int vfio_devices_dma_logging_start(VFIOContainer *container)
> +{
> +    struct vfio_device_feature *feature;
> +    int ret = 0;
> +
> +    vfio_dirty_tracking_init(container);
> +    feature = vfio_device_feature_dma_logging_start_create(container);
> +    if (!feature) {
> +        return -errno;
> +    }
> +
> +    ret = vfio_devices_dma_logging_set(container, feature);
> +    if (ret) {
> +        vfio_devices_dma_logging_stop(container);
> +    }
> +
> +    vfio_device_feature_dma_logging_start_destroy(feature);
> +
> +    return ret;
> +}
> +
>  static void vfio_listener_log_global_start(MemoryListener *listener)
>  {
>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>      int ret;
>  
> -    vfio_dirty_tracking_init(container);
> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> +        ret = vfio_devices_dma_logging_start(container);
> +    } else {
> +        ret = vfio_set_dirty_page_tracking(container, true);
> +    }
>  
> -    ret = vfio_set_dirty_page_tracking(container, true);
>      if (ret) {
> +        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> +                     ret, strerror(-ret));
>          vfio_set_migration_error(ret);
>      }
>  }
> @@ -1413,8 +1566,15 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>      int ret;
>  
> -    ret = vfio_set_dirty_page_tracking(container, false);
> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> +        ret = vfio_devices_dma_logging_stop(container);
> +    } else {
> +        ret = vfio_set_dirty_page_tracking(container, false);
> +    }
> +
>      if (ret) {
> +        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> +                     ret, strerror(-ret));
>          vfio_set_migration_error(ret);
>      }
>  }
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index d97a6de17921..7a7e0cfe5b23 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -105,6 +105,7 @@ vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t si
>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
>  vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64" - 0x%"PRIx64"]"
> +vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t max32, uint64_t min64, uint64_t max64) "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" - 0x%"PRIx64"]"
>  vfio_disconnect_container(int fd) "close container->fd=%d"
>  vfio_put_group(int fd) "close group->fd=%d"
>  vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 96791add2719..1cbbccd91e11 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -154,6 +154,8 @@ typedef struct VFIODevice {
>      VFIOMigration *migration;
>      Error *migration_blocker;
>      OnOffAuto pre_copy_dirty_page_tracking;
> +    bool dirty_pages_supported;
> +    bool dirty_tracking;
>  } VFIODevice;
>  
>  struct VFIODeviceOps {

On 06/03/2023 18:42, Alex Williamson wrote:
> On Sat,  4 Mar 2023 01:43:38 +0000
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> Add device dirty page tracking start/stop functionality. This uses the
>> device DMA logging uAPI to start and stop dirty page tracking by device.
>>
>> Device dirty page tracking is used only if all devices within a
>> container support device dirty page tracking.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
>>  hw/vfio/trace-events          |   1 +
>>  include/hw/vfio/vfio-common.h |   2 +
>>  3 files changed, 166 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index d84e5fd86bb4..aa0df0604704 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -453,6 +453,22 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>>      return true;
>>  }
>>  
>> +static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
>> +{
>> +    VFIOGroup *group;
>> +    VFIODevice *vbasedev;
>> +
>> +    QLIST_FOREACH(group, &container->group_list, container_next) {
>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +            if (!vbasedev->dirty_pages_supported) {
>> +                return false;
>> +            }
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>  /*
>>   * Check if all VFIO devices are running and migration is active, which is
>>   * essentially equivalent to the migration being in pre-copy phase.
>> @@ -1395,15 +1411,152 @@ static void vfio_dirty_tracking_init(VFIOContainer *container)
>>      qemu_mutex_destroy(&container->tracking_mutex);
>>  }
>>  
>> +static int vfio_devices_dma_logging_set(VFIOContainer *container,
>> +                                        struct vfio_device_feature *feature)
>> +{
>> +    bool status = (feature->flags & VFIO_DEVICE_FEATURE_MASK) ==
>> +                  VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
>> +    VFIODevice *vbasedev;
>> +    VFIOGroup *group;
>> +    int ret = 0;
>> +
>> +    QLIST_FOREACH(group, &container->group_list, container_next) {
>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +            if (vbasedev->dirty_tracking == status) {
>> +                continue;
>> +            }
>> +
>> +            ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
>> +            if (ret) {
>> +                ret = -errno;
>> +                error_report("%s: Failed to set DMA logging %s, err %d (%s)",
>> +                             vbasedev->name, status ? "start" : "stop", ret,
>> +                             strerror(errno));
>> +                goto out;
>> +            }
> 
> Exiting and returning an error on the first failure when starting dirty
> tracking makes sense.  Does that behavior still make sense for the stop
> path?  Maybe since we only support a single device it doesn't really
> matter, but this needs to be revisited for multiple devices.  Thanks,
> 

How about I test for @status and exit earlier based on that?
(maybe variable should be renamed too) e.g.

if (ret) {
  ret = -errno;
  error_report("%s: Failed to set DMA logging %s, err %d (%s)",
               vbasedev->name, status ? "start" : "stop", ret,
               strerror(errno))
  if (status) {
      goto out;
  }
}

> Alex
> 
>> +            vbasedev->dirty_tracking = status;
>> +        }
>> +    }
>> +
>> +out:
>> +    return ret;
>> +}
>> +
>> +static int vfio_devices_dma_logging_stop(VFIOContainer *container)
>> +{
>> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>> +                              sizeof(uint64_t))] = {};
>> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
>> +
>> +    feature->argsz = sizeof(buf);
>> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
>> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
>> +
>> +    return vfio_devices_dma_logging_set(container, feature);
>> +}
>> +
>> +static struct vfio_device_feature *
>> +vfio_device_feature_dma_logging_start_create(VFIOContainer *container)
>> +{
>> +    struct vfio_device_feature *feature;
>> +    size_t feature_size;
>> +    struct vfio_device_feature_dma_logging_control *control;
>> +    struct vfio_device_feature_dma_logging_range *ranges;
>> +    VFIODirtyTrackingRange *tracking = &container->tracking_range;
>> +
>> +    feature_size = sizeof(struct vfio_device_feature) +
>> +                   sizeof(struct vfio_device_feature_dma_logging_control);
>> +    feature = g_try_malloc0(feature_size);
>> +    if (!feature) {
>> +        errno = ENOMEM;
>> +        return NULL;
>> +    }
>> +    feature->argsz = feature_size;
>> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
>> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
>> +
>> +    control = (struct vfio_device_feature_dma_logging_control *)feature->data;
>> +    control->page_size = qemu_real_host_page_size();
>> +
>> +    /*
>> +     * DMA logging uAPI guarantees to support at least a number of ranges that
>> +     * fits into a single host kernel base page.
>> +     */
>> +    control->num_ranges = !!tracking->max32 + !!tracking->max64;
>> +    ranges = g_try_new0(struct vfio_device_feature_dma_logging_range,
>> +                        control->num_ranges);
>> +    if (!ranges) {
>> +        g_free(feature);
>> +        errno = ENOMEM;
>> +
>> +        return NULL;
>> +    }
>> +
>> +    control->ranges = (__aligned_u64)ranges;
>> +    if (tracking->max32) {
>> +        ranges->iova = tracking->min32;
>> +        ranges->length = (tracking->max32 - tracking->min32) + 1;
>> +        ranges++;
>> +    }
>> +    if (tracking->max64) {
>> +        ranges->iova = tracking->min64;
>> +        ranges->length = (tracking->max64 - tracking->min64) + 1;
>> +    }
>> +
>> +    trace_vfio_device_dirty_tracking_start(control->num_ranges,
>> +                                           tracking->min32, tracking->max32,
>> +                                           tracking->min64, tracking->max64);
>> +
>> +    return feature;
>> +}
>> +
>> +static void vfio_device_feature_dma_logging_start_destroy(
>> +    struct vfio_device_feature *feature)
>> +{
>> +    struct vfio_device_feature_dma_logging_control *control =
>> +        (struct vfio_device_feature_dma_logging_control *)feature->data;
>> +    struct vfio_device_feature_dma_logging_range *ranges =
>> +        (struct vfio_device_feature_dma_logging_range *)control->ranges;
>> +
>> +    g_free(ranges);
>> +    g_free(feature);
>> +}
>> +
>> +static int vfio_devices_dma_logging_start(VFIOContainer *container)
>> +{
>> +    struct vfio_device_feature *feature;
>> +    int ret = 0;
>> +
>> +    vfio_dirty_tracking_init(container);
>> +    feature = vfio_device_feature_dma_logging_start_create(container);
>> +    if (!feature) {
>> +        return -errno;
>> +    }
>> +
>> +    ret = vfio_devices_dma_logging_set(container, feature);
>> +    if (ret) {
>> +        vfio_devices_dma_logging_stop(container);
>> +    }
>> +
>> +    vfio_device_feature_dma_logging_start_destroy(feature);
>> +
>> +    return ret;
>> +}
>> +
>>  static void vfio_listener_log_global_start(MemoryListener *listener)
>>  {
>>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>>      int ret;
>>  
>> -    vfio_dirty_tracking_init(container);
>> +    if (vfio_devices_all_device_dirty_tracking(container)) {
>> +        ret = vfio_devices_dma_logging_start(container);
>> +    } else {
>> +        ret = vfio_set_dirty_page_tracking(container, true);
>> +    }
>>  
>> -    ret = vfio_set_dirty_page_tracking(container, true);
>>      if (ret) {
>> +        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
>> +                     ret, strerror(-ret));
>>          vfio_set_migration_error(ret);
>>      }
>>  }
>> @@ -1413,8 +1566,15 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>>      int ret;
>>  
>> -    ret = vfio_set_dirty_page_tracking(container, false);
>> +    if (vfio_devices_all_device_dirty_tracking(container)) {
>> +        ret = vfio_devices_dma_logging_stop(container);
>> +    } else {
>> +        ret = vfio_set_dirty_page_tracking(container, false);
>> +    }
>> +
>>      if (ret) {
>> +        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
>> +                     ret, strerror(-ret));
>>          vfio_set_migration_error(ret);
>>      }
>>  }
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index d97a6de17921..7a7e0cfe5b23 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -105,6 +105,7 @@ vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t si
>>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
>>  vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64" - 0x%"PRIx64"]"
>> +vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t max32, uint64_t min64, uint64_t max64) "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" - 0x%"PRIx64"]"
>>  vfio_disconnect_container(int fd) "close container->fd=%d"
>>  vfio_put_group(int fd) "close group->fd=%d"
>>  vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 96791add2719..1cbbccd91e11 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -154,6 +154,8 @@ typedef struct VFIODevice {
>>      VFIOMigration *migration;
>>      Error *migration_blocker;
>>      OnOffAuto pre_copy_dirty_page_tracking;
>> +    bool dirty_pages_supported;
>> +    bool dirty_tracking;
>>  } VFIODevice;
>>  
>>  struct VFIODeviceOps {
>

On Mon, 6 Mar 2023 19:39:15 +0000
Joao Martins <joao.m.martins@oracle.com> wrote:

> On 06/03/2023 18:42, Alex Williamson wrote:
> > On Sat,  4 Mar 2023 01:43:38 +0000
> > Joao Martins <joao.m.martins@oracle.com> wrote:
> >   
> >> Add device dirty page tracking start/stop functionality. This uses the
> >> device DMA logging uAPI to start and stop dirty page tracking by device.
> >>
> >> Device dirty page tracking is used only if all devices within a
> >> container support device dirty page tracking.
> >>
> >> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >> ---
> >>  hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
> >>  hw/vfio/trace-events          |   1 +
> >>  include/hw/vfio/vfio-common.h |   2 +
> >>  3 files changed, 166 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> >> index d84e5fd86bb4..aa0df0604704 100644
> >> --- a/hw/vfio/common.c
> >> +++ b/hw/vfio/common.c
> >> @@ -453,6 +453,22 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
> >>      return true;
> >>  }
> >>  
> >> +static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
> >> +{
> >> +    VFIOGroup *group;
> >> +    VFIODevice *vbasedev;
> >> +
> >> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> >> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> >> +            if (!vbasedev->dirty_pages_supported) {
> >> +                return false;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    return true;
> >> +}
> >> +
> >>  /*
> >>   * Check if all VFIO devices are running and migration is active, which is
> >>   * essentially equivalent to the migration being in pre-copy phase.
> >> @@ -1395,15 +1411,152 @@ static void vfio_dirty_tracking_init(VFIOContainer *container)
> >>      qemu_mutex_destroy(&container->tracking_mutex);
> >>  }
> >>  
> >> +static int vfio_devices_dma_logging_set(VFIOContainer *container,
> >> +                                        struct vfio_device_feature *feature)
> >> +{
> >> +    bool status = (feature->flags & VFIO_DEVICE_FEATURE_MASK) ==
> >> +                  VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> >> +    VFIODevice *vbasedev;
> >> +    VFIOGroup *group;
> >> +    int ret = 0;
> >> +
> >> +    QLIST_FOREACH(group, &container->group_list, container_next) {
> >> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> >> +            if (vbasedev->dirty_tracking == status) {
> >> +                continue;
> >> +            }
> >> +
> >> +            ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
> >> +            if (ret) {
> >> +                ret = -errno;
> >> +                error_report("%s: Failed to set DMA logging %s, err %d (%s)",
> >> +                             vbasedev->name, status ? "start" : "stop", ret,
> >> +                             strerror(errno));
> >> +                goto out;
> >> +            }  
> > 
> > Exiting and returning an error on the first failure when starting dirty
> > tracking makes sense.  Does that behavior still make sense for the stop
> > path?  Maybe since we only support a single device it doesn't really
> > matter, but this needs to be revisited for multiple devices.  Thanks,
> >   
> 
> How about I test for @status and exit earlier based on that?
> (maybe variable should be renamed too) e.g.
> 
> if (ret) {
>   ret = -errno;
>   error_report("%s: Failed to set DMA logging %s, err %d (%s)",
>                vbasedev->name, status ? "start" : "stop", ret,
>                strerror(errno))
>   if (status) {
>       goto out;
>   }
> }

Yep, exit on first error enabling, continue on disabling makes more
sense, but then we need to look at what return code makes sense for the
teardown.  TBH, a teardown function would typically return void, so
it's possible we'd be better off not using this for both.  Thanks,

Alex

PS - no further original comments on v3 from me.

> >> +            vbasedev->dirty_tracking = status;
> >> +        }
> >> +    }
> >> +
> >> +out:
> >> +    return ret;
> >> +}
> >> +
> >> +static int vfio_devices_dma_logging_stop(VFIOContainer *container)
> >> +{
> >> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
> >> +                              sizeof(uint64_t))] = {};
> >> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
> >> +
> >> +    feature->argsz = sizeof(buf);
> >> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> >> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
> >> +
> >> +    return vfio_devices_dma_logging_set(container, feature);
> >> +}
> >> +
> >> +static struct vfio_device_feature *
> >> +vfio_device_feature_dma_logging_start_create(VFIOContainer *container)
> >> +{
> >> +    struct vfio_device_feature *feature;
> >> +    size_t feature_size;
> >> +    struct vfio_device_feature_dma_logging_control *control;
> >> +    struct vfio_device_feature_dma_logging_range *ranges;
> >> +    VFIODirtyTrackingRange *tracking = &container->tracking_range;
> >> +
> >> +    feature_size = sizeof(struct vfio_device_feature) +
> >> +                   sizeof(struct vfio_device_feature_dma_logging_control);
> >> +    feature = g_try_malloc0(feature_size);
> >> +    if (!feature) {
> >> +        errno = ENOMEM;
> >> +        return NULL;
> >> +    }
> >> +    feature->argsz = feature_size;
> >> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
> >> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
> >> +
> >> +    control = (struct vfio_device_feature_dma_logging_control *)feature->data;
> >> +    control->page_size = qemu_real_host_page_size();
> >> +
> >> +    /*
> >> +     * DMA logging uAPI guarantees to support at least a number of ranges that
> >> +     * fits into a single host kernel base page.
> >> +     */
> >> +    control->num_ranges = !!tracking->max32 + !!tracking->max64;
> >> +    ranges = g_try_new0(struct vfio_device_feature_dma_logging_range,
> >> +                        control->num_ranges);
> >> +    if (!ranges) {
> >> +        g_free(feature);
> >> +        errno = ENOMEM;
> >> +
> >> +        return NULL;
> >> +    }
> >> +
> >> +    control->ranges = (__aligned_u64)ranges;
> >> +    if (tracking->max32) {
> >> +        ranges->iova = tracking->min32;
> >> +        ranges->length = (tracking->max32 - tracking->min32) + 1;
> >> +        ranges++;
> >> +    }
> >> +    if (tracking->max64) {
> >> +        ranges->iova = tracking->min64;
> >> +        ranges->length = (tracking->max64 - tracking->min64) + 1;
> >> +    }
> >> +
> >> +    trace_vfio_device_dirty_tracking_start(control->num_ranges,
> >> +                                           tracking->min32, tracking->max32,
> >> +                                           tracking->min64, tracking->max64);
> >> +
> >> +    return feature;
> >> +}
> >> +
> >> +static void vfio_device_feature_dma_logging_start_destroy(
> >> +    struct vfio_device_feature *feature)
> >> +{
> >> +    struct vfio_device_feature_dma_logging_control *control =
> >> +        (struct vfio_device_feature_dma_logging_control *)feature->data;
> >> +    struct vfio_device_feature_dma_logging_range *ranges =
> >> +        (struct vfio_device_feature_dma_logging_range *)control->ranges;
> >> +
> >> +    g_free(ranges);
> >> +    g_free(feature);
> >> +}
> >> +
> >> +static int vfio_devices_dma_logging_start(VFIOContainer *container)
> >> +{
> >> +    struct vfio_device_feature *feature;
> >> +    int ret = 0;
> >> +
> >> +    vfio_dirty_tracking_init(container);
> >> +    feature = vfio_device_feature_dma_logging_start_create(container);
> >> +    if (!feature) {
> >> +        return -errno;
> >> +    }
> >> +
> >> +    ret = vfio_devices_dma_logging_set(container, feature);
> >> +    if (ret) {
> >> +        vfio_devices_dma_logging_stop(container);
> >> +    }
> >> +
> >> +    vfio_device_feature_dma_logging_start_destroy(feature);
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>  static void vfio_listener_log_global_start(MemoryListener *listener)
> >>  {
> >>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> >>      int ret;
> >>  
> >> -    vfio_dirty_tracking_init(container);
> >> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> >> +        ret = vfio_devices_dma_logging_start(container);
> >> +    } else {
> >> +        ret = vfio_set_dirty_page_tracking(container, true);
> >> +    }
> >>  
> >> -    ret = vfio_set_dirty_page_tracking(container, true);
> >>      if (ret) {
> >> +        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
> >> +                     ret, strerror(-ret));
> >>          vfio_set_migration_error(ret);
> >>      }
> >>  }
> >> @@ -1413,8 +1566,15 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
> >>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> >>      int ret;
> >>  
> >> -    ret = vfio_set_dirty_page_tracking(container, false);
> >> +    if (vfio_devices_all_device_dirty_tracking(container)) {
> >> +        ret = vfio_devices_dma_logging_stop(container);
> >> +    } else {
> >> +        ret = vfio_set_dirty_page_tracking(container, false);
> >> +    }
> >> +
> >>      if (ret) {
> >> +        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
> >> +                     ret, strerror(-ret));
> >>          vfio_set_migration_error(ret);
> >>      }
> >>  }
> >> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> >> index d97a6de17921..7a7e0cfe5b23 100644
> >> --- a/hw/vfio/trace-events
> >> +++ b/hw/vfio/trace-events
> >> @@ -105,6 +105,7 @@ vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t si
> >>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
> >>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
> >>  vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64" - 0x%"PRIx64"]"
> >> +vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t max32, uint64_t min64, uint64_t max64) "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" - 0x%"PRIx64"]"
> >>  vfio_disconnect_container(int fd) "close container->fd=%d"
> >>  vfio_put_group(int fd) "close group->fd=%d"
> >>  vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
> >> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> >> index 96791add2719..1cbbccd91e11 100644
> >> --- a/include/hw/vfio/vfio-common.h
> >> +++ b/include/hw/vfio/vfio-common.h
> >> @@ -154,6 +154,8 @@ typedef struct VFIODevice {
> >>      VFIOMigration *migration;
> >>      Error *migration_blocker;
> >>      OnOffAuto pre_copy_dirty_page_tracking;
> >> +    bool dirty_pages_supported;
> >> +    bool dirty_tracking;
> >>  } VFIODevice;
> >>  
> >>  struct VFIODeviceOps {  
> >   
>

On 06/03/2023 20:00, Alex Williamson wrote:
> On Mon, 6 Mar 2023 19:39:15 +0000
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> On 06/03/2023 18:42, Alex Williamson wrote:
>>> On Sat,  4 Mar 2023 01:43:38 +0000
>>> Joao Martins <joao.m.martins@oracle.com> wrote:
>>>   
>>>> Add device dirty page tracking start/stop functionality. This uses the
>>>> device DMA logging uAPI to start and stop dirty page tracking by device.
>>>>
>>>> Device dirty page tracking is used only if all devices within a
>>>> container support device dirty page tracking.
>>>>
>>>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>  hw/vfio/common.c              | 166 +++++++++++++++++++++++++++++++++-
>>>>  hw/vfio/trace-events          |   1 +
>>>>  include/hw/vfio/vfio-common.h |   2 +
>>>>  3 files changed, 166 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>>> index d84e5fd86bb4..aa0df0604704 100644
>>>> --- a/hw/vfio/common.c
>>>> +++ b/hw/vfio/common.c
>>>> @@ -453,6 +453,22 @@ static bool vfio_devices_all_dirty_tracking(VFIOContainer *container)
>>>>      return true;
>>>>  }
>>>>  
>>>> +static bool vfio_devices_all_device_dirty_tracking(VFIOContainer *container)
>>>> +{
>>>> +    VFIOGroup *group;
>>>> +    VFIODevice *vbasedev;
>>>> +
>>>> +    QLIST_FOREACH(group, &container->group_list, container_next) {
>>>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>>> +            if (!vbasedev->dirty_pages_supported) {
>>>> +                return false;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return true;
>>>> +}
>>>> +
>>>>  /*
>>>>   * Check if all VFIO devices are running and migration is active, which is
>>>>   * essentially equivalent to the migration being in pre-copy phase.
>>>> @@ -1395,15 +1411,152 @@ static void vfio_dirty_tracking_init(VFIOContainer *container)
>>>>      qemu_mutex_destroy(&container->tracking_mutex);
>>>>  }
>>>>  
>>>> +static int vfio_devices_dma_logging_set(VFIOContainer *container,
>>>> +                                        struct vfio_device_feature *feature)
>>>> +{
>>>> +    bool status = (feature->flags & VFIO_DEVICE_FEATURE_MASK) ==
>>>> +                  VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
>>>> +    VFIODevice *vbasedev;
>>>> +    VFIOGroup *group;
>>>> +    int ret = 0;
>>>> +
>>>> +    QLIST_FOREACH(group, &container->group_list, container_next) {
>>>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>>> +            if (vbasedev->dirty_tracking == status) {
>>>> +                continue;
>>>> +            }
>>>> +
>>>> +            ret = ioctl(vbasedev->fd, VFIO_DEVICE_FEATURE, feature);
>>>> +            if (ret) {
>>>> +                ret = -errno;
>>>> +                error_report("%s: Failed to set DMA logging %s, err %d (%s)",
>>>> +                             vbasedev->name, status ? "start" : "stop", ret,
>>>> +                             strerror(errno));
>>>> +                goto out;
>>>> +            }  
>>>
>>> Exiting and returning an error on the first failure when starting dirty
>>> tracking makes sense.  Does that behavior still make sense for the stop
>>> path?  Maybe since we only support a single device it doesn't really
>>> matter, but this needs to be revisited for multiple devices.  Thanks,
>>>   
>>
>> How about I test for @status and exit earlier based on that?
>> (maybe variable should be renamed too) e.g.
>>
>> if (ret) {
>>   ret = -errno;
>>   error_report("%s: Failed to set DMA logging %s, err %d (%s)",
>>                vbasedev->name, status ? "start" : "stop", ret,
>>                strerror(errno))
>>   if (status) {
>>       goto out;
>>   }
>> }
> 
> Yep, exit on first error enabling, continue on disabling makes more
> sense, but then we need to look at what return code makes sense for the
> teardown.  TBH, a teardown function would typically return void, so
> it's possible we'd be better off not using this for both.  Thanks,
> 
Agreed. I can unroll set into _start and _stop and turn _stop into a void function.

> Alex
> 
> PS - no further original comments on v3 from me.
> 
OK

>>>> +            vbasedev->dirty_tracking = status;
>>>> +        }
>>>> +    }
>>>> +
>>>> +out:
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int vfio_devices_dma_logging_stop(VFIOContainer *container)
>>>> +{
>>>> +    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
>>>> +                              sizeof(uint64_t))] = {};
>>>> +    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
>>>> +
>>>> +    feature->argsz = sizeof(buf);
>>>> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
>>>> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP;
>>>> +
>>>> +    return vfio_devices_dma_logging_set(container, feature);
>>>> +}
>>>> +
>>>> +static struct vfio_device_feature *
>>>> +vfio_device_feature_dma_logging_start_create(VFIOContainer *container)
>>>> +{
>>>> +    struct vfio_device_feature *feature;
>>>> +    size_t feature_size;
>>>> +    struct vfio_device_feature_dma_logging_control *control;
>>>> +    struct vfio_device_feature_dma_logging_range *ranges;
>>>> +    VFIODirtyTrackingRange *tracking = &container->tracking_range;
>>>> +
>>>> +    feature_size = sizeof(struct vfio_device_feature) +
>>>> +                   sizeof(struct vfio_device_feature_dma_logging_control);
>>>> +    feature = g_try_malloc0(feature_size);
>>>> +    if (!feature) {
>>>> +        errno = ENOMEM;
>>>> +        return NULL;
>>>> +    }
>>>> +    feature->argsz = feature_size;
>>>> +    feature->flags = VFIO_DEVICE_FEATURE_SET;
>>>> +    feature->flags |= VFIO_DEVICE_FEATURE_DMA_LOGGING_START;
>>>> +
>>>> +    control = (struct vfio_device_feature_dma_logging_control *)feature->data;
>>>> +    control->page_size = qemu_real_host_page_size();
>>>> +
>>>> +    /*
>>>> +     * DMA logging uAPI guarantees to support at least a number of ranges that
>>>> +     * fits into a single host kernel base page.
>>>> +     */
>>>> +    control->num_ranges = !!tracking->max32 + !!tracking->max64;
>>>> +    ranges = g_try_new0(struct vfio_device_feature_dma_logging_range,
>>>> +                        control->num_ranges);
>>>> +    if (!ranges) {
>>>> +        g_free(feature);
>>>> +        errno = ENOMEM;
>>>> +
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    control->ranges = (__aligned_u64)ranges;
>>>> +    if (tracking->max32) {
>>>> +        ranges->iova = tracking->min32;
>>>> +        ranges->length = (tracking->max32 - tracking->min32) + 1;
>>>> +        ranges++;
>>>> +    }
>>>> +    if (tracking->max64) {
>>>> +        ranges->iova = tracking->min64;
>>>> +        ranges->length = (tracking->max64 - tracking->min64) + 1;
>>>> +    }
>>>> +
>>>> +    trace_vfio_device_dirty_tracking_start(control->num_ranges,
>>>> +                                           tracking->min32, tracking->max32,
>>>> +                                           tracking->min64, tracking->max64);
>>>> +
>>>> +    return feature;
>>>> +}
>>>> +
>>>> +static void vfio_device_feature_dma_logging_start_destroy(
>>>> +    struct vfio_device_feature *feature)
>>>> +{
>>>> +    struct vfio_device_feature_dma_logging_control *control =
>>>> +        (struct vfio_device_feature_dma_logging_control *)feature->data;
>>>> +    struct vfio_device_feature_dma_logging_range *ranges =
>>>> +        (struct vfio_device_feature_dma_logging_range *)control->ranges;
>>>> +
>>>> +    g_free(ranges);
>>>> +    g_free(feature);
>>>> +}
>>>> +
>>>> +static int vfio_devices_dma_logging_start(VFIOContainer *container)
>>>> +{
>>>> +    struct vfio_device_feature *feature;
>>>> +    int ret = 0;
>>>> +
>>>> +    vfio_dirty_tracking_init(container);
>>>> +    feature = vfio_device_feature_dma_logging_start_create(container);
>>>> +    if (!feature) {
>>>> +        return -errno;
>>>> +    }
>>>> +
>>>> +    ret = vfio_devices_dma_logging_set(container, feature);
>>>> +    if (ret) {
>>>> +        vfio_devices_dma_logging_stop(container);
>>>> +    }
>>>> +
>>>> +    vfio_device_feature_dma_logging_start_destroy(feature);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>  static void vfio_listener_log_global_start(MemoryListener *listener)
>>>>  {
>>>>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>>>>      int ret;
>>>>  
>>>> -    vfio_dirty_tracking_init(container);
>>>> +    if (vfio_devices_all_device_dirty_tracking(container)) {
>>>> +        ret = vfio_devices_dma_logging_start(container);
>>>> +    } else {
>>>> +        ret = vfio_set_dirty_page_tracking(container, true);
>>>> +    }
>>>>  
>>>> -    ret = vfio_set_dirty_page_tracking(container, true);
>>>>      if (ret) {
>>>> +        error_report("vfio: Could not start dirty page tracking, err: %d (%s)",
>>>> +                     ret, strerror(-ret));
>>>>          vfio_set_migration_error(ret);
>>>>      }
>>>>  }
>>>> @@ -1413,8 +1566,15 @@ static void vfio_listener_log_global_stop(MemoryListener *listener)
>>>>      VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>>>>      int ret;
>>>>  
>>>> -    ret = vfio_set_dirty_page_tracking(container, false);
>>>> +    if (vfio_devices_all_device_dirty_tracking(container)) {
>>>> +        ret = vfio_devices_dma_logging_stop(container);
>>>> +    } else {
>>>> +        ret = vfio_set_dirty_page_tracking(container, false);
>>>> +    }
>>>> +
>>>>      if (ret) {
>>>> +        error_report("vfio: Could not stop dirty page tracking, err: %d (%s)",
>>>> +                     ret, strerror(-ret));
>>>>          vfio_set_migration_error(ret);
>>>>      }
>>>>  }
>>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>>>> index d97a6de17921..7a7e0cfe5b23 100644
>>>> --- a/hw/vfio/trace-events
>>>> +++ b/hw/vfio/trace-events
>>>> @@ -105,6 +105,7 @@ vfio_listener_region_add_no_dma_map(const char *name, uint64_t iova, uint64_t si
>>>>  vfio_listener_region_del_skip(uint64_t start, uint64_t end) "SKIPPING region_del 0x%"PRIx64" - 0x%"PRIx64
>>>>  vfio_listener_region_del(uint64_t start, uint64_t end) "region_del 0x%"PRIx64" - 0x%"PRIx64
>>>>  vfio_device_dirty_tracking_update(uint64_t start, uint64_t end, uint64_t min, uint64_t max) "section 0x%"PRIx64" - 0x%"PRIx64" -> update [0x%"PRIx64" - 0x%"PRIx64"]"
>>>> +vfio_device_dirty_tracking_start(int nr_ranges, uint64_t min32, uint64_t max32, uint64_t min64, uint64_t max64) "nr_ranges %d 32:[0x%"PRIx64" - 0x%"PRIx64"], 64:[0x%"PRIx64" - 0x%"PRIx64"]"
>>>>  vfio_disconnect_container(int fd) "close container->fd=%d"
>>>>  vfio_put_group(int fd) "close group->fd=%d"
>>>>  vfio_get_device(const char * name, unsigned int flags, unsigned int num_regions, unsigned int num_irqs) "Device %s flags: %u, regions: %u, irqs: %u"
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>>> index 96791add2719..1cbbccd91e11 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -154,6 +154,8 @@ typedef struct VFIODevice {
>>>>      VFIOMigration *migration;
>>>>      Error *migration_blocker;
>>>>      OnOffAuto pre_copy_dirty_page_tracking;
>>>> +    bool dirty_pages_supported;
>>>> +    bool dirty_tracking;
>>>>  } VFIODevice;
>>>>  
>>>>  struct VFIODeviceOps {  
>>>   
>>
>