[PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support

Joao Martins posted 12 patches 4 months, 2 weeks ago
Maintainers: Yi Liu <yi.l.liu@intel.com>, Eric Auger <eric.auger@redhat.com>, Zhenzhong Duan <zhenzhong.duan@intel.com>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
There is a newer version of this series
[PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Joao Martins 4 months, 2 weeks ago
ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
enables or disables dirty page tracking. It is used if the hwpt
has been created with dirty tracking supported domain (stored in
hwpt::flags) and it is called on the whole list of iommu domains
it is are tracking. On failure it rolls it back.

The checking of hwpt::flags is introduced here as a second user
and thus consolidate such check into a helper function
iommufd_hwpt_dirty_tracking().

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 include/sysemu/iommufd.h |  3 +++
 backends/iommufd.c       | 23 +++++++++++++++++++++++
 hw/vfio/iommufd.c        | 39 ++++++++++++++++++++++++++++++++++++++-
 backends/trace-events    |  1 +
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index e917e7591d05..7416d9219703 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -55,6 +55,9 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
                                 uint32_t data_type, uint32_t data_len,
                                 void *data_ptr, uint32_t *out_hwpt,
                                 Error **errp);
+bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
+                                        bool start, Error **errp);
 
 #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
+
 #endif
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 41a9dec3b2c5..239f0976e0ad 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -239,6 +239,29 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
     return true;
 }
 
+bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
+                                        uint32_t hwpt_id, bool start,
+                                        Error **errp)
+{
+    int ret;
+    struct iommu_hwpt_set_dirty_tracking set_dirty = {
+            .size = sizeof(set_dirty),
+            .hwpt_id = hwpt_id,
+            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
+    };
+
+    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
+    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno : 0);
+    if (ret) {
+        error_setg_errno(errp, errno,
+                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
+                         hwpt_id);
+        return false;
+    }
+
+    return true;
+}
+
 bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
                                      uint32_t *type, void *data, uint32_t len,
                                      uint64_t *caps, Error **errp)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index edc8f97d8f3d..da678315faeb 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -110,6 +110,42 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
     iommufd_backend_disconnect(vbasedev->iommufd);
 }
 
+static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
+{
+    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+}
+
+static int iommufd_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
+                                           bool start, Error **errp)
+{
+    const VFIOIOMMUFDContainer *container =
+        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
+    VFIOIOASHwpt *hwpt;
+
+    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
+        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
+            continue;
+        }
+
+        if (!iommufd_backend_set_dirty_tracking(container->be,
+                                                hwpt->hwpt_id, start, errp)) {
+            goto err;
+        }
+    }
+
+    return 0;
+
+err:
+    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
+        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
+            continue;
+        }
+        iommufd_backend_set_dirty_tracking(container->be,
+                                           hwpt->hwpt_id, !start, NULL);
+    }
+    return -EINVAL;
+}
+
 static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
 {
     ERRP_GUARD();
@@ -278,7 +314,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
     QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
     QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
     container->bcontainer.dirty_pages_supported |=
-                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
+                              iommufd_hwpt_dirty_tracking(hwpt);
     return true;
 }
 
@@ -717,6 +753,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
     vioc->attach_device = iommufd_cdev_attach;
     vioc->detach_device = iommufd_cdev_detach;
     vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
+    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
 };
 
 static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
diff --git a/backends/trace-events b/backends/trace-events
index 4d8ac02fe7d6..28aca3b859d4 100644
--- a/backends/trace-events
+++ b/backends/trace-events
@@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t si
 iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
 iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
 iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
+iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int ret) " iommufd=%d hwpt=%u enable=%d (%d)"
-- 
2.17.2
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Eric Auger 4 months, 1 week ago

On 7/12/24 13:47, Joao Martins wrote:
> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
> enables or disables dirty page tracking. It is used if the hwpt
> has been created with dirty tracking supported domain (stored in
> hwpt::flags) and it is called on the whole list of iommu domains
> it is are tracking. On failure it rolls it back.

it is are tracking ?
also please clearly state what is "it"

>
> The checking of hwpt::flags is introduced here as a second user
?? -> introduce iommufd_hwpt_dirty_tracking() helper to avoid code dup?
> and thus consolidate such check into a helper function
> iommufd_hwpt_dirty_tracking().
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  include/sysemu/iommufd.h |  3 +++
>  backends/iommufd.c       | 23 +++++++++++++++++++++++
>  hw/vfio/iommufd.c        | 39 ++++++++++++++++++++++++++++++++++++++-
>  backends/trace-events    |  1 +
>  4 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index e917e7591d05..7416d9219703 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -55,6 +55,9 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>                                  uint32_t data_type, uint32_t data_len,
>                                  void *data_ptr, uint32_t *out_hwpt,
>                                  Error **errp);
> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
> +                                        bool start, Error **errp);
>  
>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
spurious line change
>  #endif
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 41a9dec3b2c5..239f0976e0ad 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -239,6 +239,29 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>      return true;
>  }
>  
> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
> +                                        uint32_t hwpt_id, bool start,
> +                                        Error **errp)
> +{
> +    int ret;
> +    struct iommu_hwpt_set_dirty_tracking set_dirty = {
> +            .size = sizeof(set_dirty),
> +            .hwpt_id = hwpt_id,
> +            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
> +    };
> +
> +    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
> +    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno : 0);
> +    if (ret) {
> +        error_setg_errno(errp, errno,
> +                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
> +                         hwpt_id);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>                                       uint32_t *type, void *data, uint32_t len,
>                                       uint64_t *caps, Error **errp)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index edc8f97d8f3d..da678315faeb 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -110,6 +110,42 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>      iommufd_backend_disconnect(vbasedev->iommufd);
>  }
>  
> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
> +{
> +    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +}
> +
> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
> +                                           bool start, Error **errp)
> +{
> +    const VFIOIOMMUFDContainer *container =
> +        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
> +    VFIOIOASHwpt *hwpt;
> +
> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
> +            continue;
> +        }
> +
> +        if (!iommufd_backend_set_dirty_tracking(container->be,
> +                                                hwpt->hwpt_id, start, errp)) {
> +            goto err;
> +        }
> +    }
> +
> +    return 0;
> +
> +err:
> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
> +            continue;
> +        }
> +        iommufd_backend_set_dirty_tracking(container->be,
> +                                           hwpt->hwpt_id, !start, NULL);
> +    }
> +    return -EINVAL;
> +}
> +
>  static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>  {
>      ERRP_GUARD();
> @@ -278,7 +314,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>      QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>      QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>      container->bcontainer.dirty_pages_supported |=
> -                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
> +                              iommufd_hwpt_dirty_tracking(hwpt);
>      return true;
>  }
>  
> @@ -717,6 +753,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>      vioc->attach_device = iommufd_cdev_attach;
>      vioc->detach_device = iommufd_cdev_detach;
>      vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
> +    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
>  };
>  
>  static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
> diff --git a/backends/trace-events b/backends/trace-events
> index 4d8ac02fe7d6..28aca3b859d4 100644
> --- a/backends/trace-events
> +++ b/backends/trace-events
> @@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t si
>  iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>  iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
>  iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
> +iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int ret) " iommufd=%d hwpt=%u enable=%d (%d)"
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Joao Martins 4 months, 1 week ago
On 17/07/2024 13:36, Eric Auger wrote:
> 
> 
> On 7/12/24 13:47, Joao Martins wrote:
>> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>> enables or disables dirty page tracking. It is used if the hwpt
>> has been created with dirty tracking supported domain (stored in
>> hwpt::flags) and it is called on the whole list of iommu domains
>> it is are tracking. On failure it rolls it back.
> 
> it is are tracking ?

*it is*

> also please clearly state what is "it"
>

sure

>>
>> The checking of hwpt::flags is introduced here as a second user
> ?? -> introduce iommufd_hwpt_dirty_tracking() helper to avoid code dup?

Right I am doing that already. Not sure what the problem is with this sentence?

Or you meant to use yours as it's simpler/easier to understand.

>> and thus consolidate such check into a helper function
>> iommufd_hwpt_dirty_tracking().
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  include/sysemu/iommufd.h |  3 +++
>>  backends/iommufd.c       | 23 +++++++++++++++++++++++
>>  hw/vfio/iommufd.c        | 39 ++++++++++++++++++++++++++++++++++++++-
>>  backends/trace-events    |  1 +
>>  4 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index e917e7591d05..7416d9219703 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -55,6 +55,9 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>                                  uint32_t data_type, uint32_t data_len,
>>                                  void *data_ptr, uint32_t *out_hwpt,
>>                                  Error **errp);
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
>> +                                        bool start, Error **errp);
>>  
>>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>> +
> spurious line change

ok

>>  #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 41a9dec3b2c5..239f0976e0ad 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -239,6 +239,29 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>      return true;
>>  }
>>  
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>> +                                        uint32_t hwpt_id, bool start,
>> +                                        Error **errp)
>> +{
>> +    int ret;
>> +    struct iommu_hwpt_set_dirty_tracking set_dirty = {
>> +            .size = sizeof(set_dirty),
>> +            .hwpt_id = hwpt_id,
>> +            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
>> +    };
>> +
>> +    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
>> +    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno : 0);
>> +    if (ret) {
>> +        error_setg_errno(errp, errno,
>> +                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
>> +                         hwpt_id);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>  bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>>                                       uint32_t *type, void *data, uint32_t len,
>>                                       uint64_t *caps, Error **errp)
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index edc8f97d8f3d..da678315faeb 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -110,6 +110,42 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>>      iommufd_backend_disconnect(vbasedev->iommufd);
>>  }
>>  
>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>> +{
>> +    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>> +}
>> +
>> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>> +                                           bool start, Error **errp)
>> +{
>> +    const VFIOIOMMUFDContainer *container =
>> +        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>> +    VFIOIOASHwpt *hwpt;
>> +
>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>> +            continue;
>> +        }
>> +
>> +        if (!iommufd_backend_set_dirty_tracking(container->be,
>> +                                                hwpt->hwpt_id, start, errp)) {
>> +            goto err;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +
>> +err:
>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>> +            continue;
>> +        }
>> +        iommufd_backend_set_dirty_tracking(container->be,
>> +                                           hwpt->hwpt_id, !start, NULL);
>> +    }
>> +    return -EINVAL;
>> +}
>> +
>>  static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>>  {
>>      ERRP_GUARD();
>> @@ -278,7 +314,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>      QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>>      QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>>      container->bcontainer.dirty_pages_supported |=
>> -                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
>> +                              iommufd_hwpt_dirty_tracking(hwpt);
>>      return true;
>>  }
>>  
>> @@ -717,6 +753,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>>      vioc->attach_device = iommufd_cdev_attach;
>>      vioc->detach_device = iommufd_cdev_detach;
>>      vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
>> +    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
>>  };
>>  
>>  static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>> diff --git a/backends/trace-events b/backends/trace-events
>> index 4d8ac02fe7d6..28aca3b859d4 100644
>> --- a/backends/trace-events
>> +++ b/backends/trace-events
>> @@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t si
>>  iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>>  iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
>>  iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
>> +iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int ret) " iommufd=%d hwpt=%u enable=%d (%d)"
>
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Eric Auger 4 months, 1 week ago

On 7/17/24 14:41, Joao Martins wrote:
> On 17/07/2024 13:36, Eric Auger wrote:
>>
>> On 7/12/24 13:47, Joao Martins wrote:
>>> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>>> enables or disables dirty page tracking. It is used if the hwpt
>>> has been created with dirty tracking supported domain (stored in
>>> hwpt::flags) and it is called on the whole list of iommu domains
>>> it is are tracking. On failure it rolls it back.
>> it is are tracking ?
> *it is*
>
>> also please clearly state what is "it"
>>
> sure
>
>>> The checking of hwpt::flags is introduced here as a second user
>> ?? -> introduce iommufd_hwpt_dirty_tracking() helper to avoid code dup?
> Right I am doing that already. Not sure what the problem is with this sentence?

"The checking of hwpt::flags is introduced here as a second user" phrasing sounds weird to me.
I guess you just meant that you need to check 
hwpt::flags in another place so better off introducing an helper.

>
> Or you meant to use yours as it's simpler/easier to understand.
>
>>> and thus consolidate such check into a helper function
>>> iommufd_hwpt_dirty_tracking().
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>  include/sysemu/iommufd.h |  3 +++
>>>  backends/iommufd.c       | 23 +++++++++++++++++++++++
>>>  hw/vfio/iommufd.c        | 39 ++++++++++++++++++++++++++++++++++++++-
>>>  backends/trace-events    |  1 +
>>>  4 files changed, 65 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>>> index e917e7591d05..7416d9219703 100644
>>> --- a/include/sysemu/iommufd.h
>>> +++ b/include/sysemu/iommufd.h
>>> @@ -55,6 +55,9 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>>                                  uint32_t data_type, uint32_t data_len,
>>>                                  void *data_ptr, uint32_t *out_hwpt,
>>>                                  Error **errp);
>>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
>>> +                                        bool start, Error **errp);
>>>  
>>>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>> +
>> spurious line change
> ok
>
>>>  #endif
>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index 41a9dec3b2c5..239f0976e0ad 100644
>>> --- a/backends/iommufd.c
>>> +++ b/backends/iommufd.c
>>> @@ -239,6 +239,29 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>>      return true;
>>>  }
>>>  
>>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>>> +                                        uint32_t hwpt_id, bool start,
>>> +                                        Error **errp)
>>> +{
>>> +    int ret;
>>> +    struct iommu_hwpt_set_dirty_tracking set_dirty = {
>>> +            .size = sizeof(set_dirty),
>>> +            .hwpt_id = hwpt_id,
>>> +            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
>>> +    };
>>> +
>>> +    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
>>> +    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno : 0);
>>> +    if (ret) {
>>> +        error_setg_errno(errp, errno,
>>> +                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
>>> +                         hwpt_id);
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>>  bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>>>                                       uint32_t *type, void *data, uint32_t len,
>>>                                       uint64_t *caps, Error **errp)
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index edc8f97d8f3d..da678315faeb 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -110,6 +110,42 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>>>      iommufd_backend_disconnect(vbasedev->iommufd);
>>>  }
>>>  
>>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>>> +{
>>> +    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>>> +}
>>> +
>>> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
>>> +                                           bool start, Error **errp)
>>> +{
>>> +    const VFIOIOMMUFDContainer *container =
>>> +        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>>> +    VFIOIOASHwpt *hwpt;
>>> +
>>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>>> +            continue;
>>> +        }
>>> +
>>> +        if (!iommufd_backend_set_dirty_tracking(container->be,
>>> +                                                hwpt->hwpt_id, start, errp)) {
>>> +            goto err;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>>> +            continue;
>>> +        }
>>> +        iommufd_backend_set_dirty_tracking(container->be,
>>> +                                           hwpt->hwpt_id, !start, NULL);
>>> +    }
>>> +    return -EINVAL;
>>> +}
>>> +
>>>  static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>>>  {
>>>      ERRP_GUARD();
>>> @@ -278,7 +314,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>>      QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>>>      QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>>>      container->bcontainer.dirty_pages_supported |=
>>> -                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
>>> +                              iommufd_hwpt_dirty_tracking(hwpt);
>>>      return true;
>>>  }
>>>  
>>> @@ -717,6 +753,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>>>      vioc->attach_device = iommufd_cdev_attach;
>>>      vioc->detach_device = iommufd_cdev_detach;
>>>      vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
>>> +    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
>>>  };
>>>  
>>>  static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>> diff --git a/backends/trace-events b/backends/trace-events
>>> index 4d8ac02fe7d6..28aca3b859d4 100644
>>> --- a/backends/trace-events
>>> +++ b/backends/trace-events
>>> @@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t si
>>>  iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>>>  iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
>>>  iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
>>> +iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int ret) " iommufd=%d hwpt=%u enable=%d (%d)"
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Joao Martins 4 months, 1 week ago
On 17/07/2024 14:34, Eric Auger wrote:
> On 7/17/24 14:41, Joao Martins wrote:
>> On 17/07/2024 13:36, Eric Auger wrote:
>>> On 7/12/24 13:47, Joao Martins wrote:
>>>> The checking of hwpt::flags is introduced here as a second user
>>> ?? -> introduce iommufd_hwpt_dirty_tracking() helper to avoid code dup?
>> Right I am doing that already. Not sure what the problem is with this sentence?
> 
> "The checking of hwpt::flags is introduced here as a second user" phrasing sounds weird to me.
> I guess you just meant that you need to check 
> hwpt::flags in another place so better off introducing an helper.
> 

Exactly.
RE: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Duan, Zhenzhong 4 months, 1 week ago

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: [PATCH v4 09/12] vfio/iommufd: Implement
>VFIOIOMMUClass::set_dirty_tracking support
>
>ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>enables or disables dirty page tracking. It is used if the hwpt
>has been created with dirty tracking supported domain (stored in
>hwpt::flags) and it is called on the whole list of iommu domains
>it is are tracking. On failure it rolls it back.
>
>The checking of hwpt::flags is introduced here as a second user
>and thus consolidate such check into a helper function
>iommufd_hwpt_dirty_tracking().
>
>Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>---
> include/sysemu/iommufd.h |  3 +++
> backends/iommufd.c       | 23 +++++++++++++++++++++++
> hw/vfio/iommufd.c        | 39
>++++++++++++++++++++++++++++++++++++++-
> backends/trace-events    |  1 +
> 4 files changed, 65 insertions(+), 1 deletion(-)
>
>diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>index e917e7591d05..7416d9219703 100644
>--- a/include/sysemu/iommufd.h
>+++ b/include/sysemu/iommufd.h
>@@ -55,6 +55,9 @@ bool
>iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>                                 uint32_t data_type, uint32_t data_len,
>                                 void *data_ptr, uint32_t *out_hwpt,
>                                 Error **errp);
>+bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>uint32_t hwpt_id,
>+                                        bool start, Error **errp);
>
> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>TYPE_HOST_IOMMU_DEVICE "-iommufd"
>+
> #endif
>diff --git a/backends/iommufd.c b/backends/iommufd.c
>index 41a9dec3b2c5..239f0976e0ad 100644
>--- a/backends/iommufd.c
>+++ b/backends/iommufd.c
>@@ -239,6 +239,29 @@ bool
>iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>     return true;
> }
>
>+bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>+                                        uint32_t hwpt_id, bool start,
>+                                        Error **errp)
>+{
>+    int ret;
>+    struct iommu_hwpt_set_dirty_tracking set_dirty = {
>+            .size = sizeof(set_dirty),
>+            .hwpt_id = hwpt_id,
>+            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
>+    };
>+
>+    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
>+    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno :
>0);
>+    if (ret) {
>+        error_setg_errno(errp, errno,
>+                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
>+                         hwpt_id);
>+        return false;
>+    }
>+
>+    return true;
>+}
>+
> bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>devid,
>                                      uint32_t *type, void *data, uint32_t len,
>                                      uint64_t *caps, Error **errp)
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index edc8f97d8f3d..da678315faeb 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -110,6 +110,42 @@ static void
>iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>     iommufd_backend_disconnect(vbasedev->iommufd);
> }
>
>+static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>+{
>+    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>+}
>+
>+static int iommufd_set_dirty_page_tracking(const VFIOContainerBase
>*bcontainer,
>+                                           bool start, Error **errp)
>+{
>+    const VFIOIOMMUFDContainer *container =
>+        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>+    VFIOIOASHwpt *hwpt;
>+
>+    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>+        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>+            continue;
>+        }

So the devices under an hwpt that doesn't support dirty tracking are bypassed.
Then how to track dirty pages coming from those devices?

Thanks
Zhenzhong

>+
>+        if (!iommufd_backend_set_dirty_tracking(container->be,
>+                                                hwpt->hwpt_id, start, errp)) {
>+            goto err;
>+        }
>+    }
>+
>+    return 0;
>+
>+err:
>+    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>+        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>+            continue;
>+        }
>+        iommufd_backend_set_dirty_tracking(container->be,
>+                                           hwpt->hwpt_id, !start, NULL);
>+    }
>+    return -EINVAL;
>+}
>+
> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
> {
>     ERRP_GUARD();
>@@ -278,7 +314,7 @@ static bool
>iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>     QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>     QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>     container->bcontainer.dirty_pages_supported |=
>-                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
>+                              iommufd_hwpt_dirty_tracking(hwpt);
>     return true;
> }
>
>@@ -717,6 +753,7 @@ static void
>vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>     vioc->attach_device = iommufd_cdev_attach;
>     vioc->detach_device = iommufd_cdev_detach;
>     vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
>+    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
> };
>
> static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void
>*opaque,
>diff --git a/backends/trace-events b/backends/trace-events
>index 4d8ac02fe7d6..28aca3b859d4 100644
>--- a/backends/trace-events
>+++ b/backends/trace-events
>@@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd,
>uint32_t ioas, uint64_t iova, uint64_t si
> iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d
>ioas=%d"
> iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t
>pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr,
>uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u
>flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u
>(%d)"
> iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d
>id=%d (%d)"
>+iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start,
>int ret) " iommufd=%d hwpt=%u enable=%d (%d)"
>--
>2.17.2
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Joao Martins 4 months, 1 week ago
On 17/07/2024 03:24, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v4 09/12] vfio/iommufd: Implement
>> VFIOIOMMUClass::set_dirty_tracking support
>>
>> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>> enables or disables dirty page tracking. It is used if the hwpt
>> has been created with dirty tracking supported domain (stored in
>> hwpt::flags) and it is called on the whole list of iommu domains
>> it is are tracking. On failure it rolls it back.
>>
>> The checking of hwpt::flags is introduced here as a second user
>> and thus consolidate such check into a helper function
>> iommufd_hwpt_dirty_tracking().
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> include/sysemu/iommufd.h |  3 +++
>> backends/iommufd.c       | 23 +++++++++++++++++++++++
>> hw/vfio/iommufd.c        | 39
>> ++++++++++++++++++++++++++++++++++++++-
>> backends/trace-events    |  1 +
>> 4 files changed, 65 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index e917e7591d05..7416d9219703 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -55,6 +55,9 @@ bool
>> iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>                                 uint32_t data_type, uint32_t data_len,
>>                                 void *data_ptr, uint32_t *out_hwpt,
>>                                 Error **errp);
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>> uint32_t hwpt_id,
>> +                                        bool start, Error **errp);
>>
>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>> TYPE_HOST_IOMMU_DEVICE "-iommufd"
>> +
>> #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 41a9dec3b2c5..239f0976e0ad 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -239,6 +239,29 @@ bool
>> iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>>     return true;
>> }
>>
>> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
>> +                                        uint32_t hwpt_id, bool start,
>> +                                        Error **errp)
>> +{
>> +    int ret;
>> +    struct iommu_hwpt_set_dirty_tracking set_dirty = {
>> +            .size = sizeof(set_dirty),
>> +            .hwpt_id = hwpt_id,
>> +            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,
>> +    };
>> +
>> +    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
>> +    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno :
>> 0);
>> +    if (ret) {
>> +        error_setg_errno(errp, errno,
>> +                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
>> +                         hwpt_id);
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>> devid,
>>                                      uint32_t *type, void *data, uint32_t len,
>>                                      uint64_t *caps, Error **errp)
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index edc8f97d8f3d..da678315faeb 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -110,6 +110,42 @@ static void
>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>>     iommufd_backend_disconnect(vbasedev->iommufd);
>> }
>>
>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>> +{
>> +    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>> +}
>> +
>> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase
>> *bcontainer,
>> +                                           bool start, Error **errp)
>> +{
>> +    const VFIOIOMMUFDContainer *container =
>> +        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
>> +    VFIOIOASHwpt *hwpt;
>> +
>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>> +            continue;
>> +        }
> 
> So the devices under an hwpt that doesn't support dirty tracking are bypassed.
> Then how to track dirty pages coming from those devices?
> 

We don't support 'mixed mode' dirty tracking right now even before this series.
I plan on lifting that restriction as a follow up. So far I was thinking that to
make sure migration is blocked if neither VF nor IOMMU VF dirty tracking are
supported.

The reason is that the migration initialization of the VFIODevice needs to be
adjusted to be able to understand all the constraints that the IOMMU dirty
tracking is not requested when VF dirty tracking is in use, and vice-versa. Thus
making this check a lot more representative of the features it is using.
Re: [PATCH v4 09/12] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support
Posted by Cédric Le Goater 4 months, 1 week ago
On 7/12/24 13:47, Joao Martins wrote:
> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
> enables or disables dirty page tracking. It is used if the hwpt
> has been created with dirty tracking supported domain (stored in
> hwpt::flags) and it is called on the whole list of iommu domains
> it is are tracking. On failure it rolls it back.
> 
> The checking of hwpt::flags is introduced here as a second user
> and thus consolidate such check into a helper function
> iommufd_hwpt_dirty_tracking().
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   include/sysemu/iommufd.h |  3 +++
>   backends/iommufd.c       | 23 +++++++++++++++++++++++
>   hw/vfio/iommufd.c        | 39 ++++++++++++++++++++++++++++++++++++++-
>   backends/trace-events    |  1 +
>   4 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index e917e7591d05..7416d9219703 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -55,6 +55,9 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>                                   uint32_t data_type, uint32_t data_len,
>                                   void *data_ptr, uint32_t *out_hwpt,
>                                   Error **errp);
> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
> +                                        bool start, Error **errp);
>   
>   #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
>   #endif
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 41a9dec3b2c5..239f0976e0ad 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -239,6 +239,29 @@ bool iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t dev_id,
>       return true;
>   }
>   
> +bool iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be,
> +                                        uint32_t hwpt_id, bool start,
> +                                        Error **errp)
> +{
> +    int ret;
> +    struct iommu_hwpt_set_dirty_tracking set_dirty = {
> +            .size = sizeof(set_dirty),
> +            .hwpt_id = hwpt_id,
> +            .flags = !start ? 0 : IOMMU_HWPT_DIRTY_TRACKING_ENABLE,

How about :

             .flags = start ? IOMMU_HWPT_DIRTY_TRACKING_ENABLE : 0,

?


Thanks,

C.


> +    };
> +
> +    ret = ioctl(be->fd, IOMMU_HWPT_SET_DIRTY_TRACKING, &set_dirty);
> +    trace_iommufd_backend_set_dirty(be->fd, hwpt_id, start, ret ? errno : 0);
> +    if (ret) {
> +        error_setg_errno(errp, errno,
> +                         "IOMMU_HWPT_SET_DIRTY_TRACKING(hwpt_id %u) failed",
> +                         hwpt_id);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>   bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>                                        uint32_t *type, void *data, uint32_t len,
>                                        uint64_t *caps, Error **errp)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index edc8f97d8f3d..da678315faeb 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -110,6 +110,42 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>       iommufd_backend_disconnect(vbasedev->iommufd);
>   }
>   
> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
> +{
> +    return hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +}
> +
> +static int iommufd_set_dirty_page_tracking(const VFIOContainerBase *bcontainer,
> +                                           bool start, Error **errp)
> +{
> +    const VFIOIOMMUFDContainer *container =
> +        container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
> +    VFIOIOASHwpt *hwpt;
> +
> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
> +            continue;
> +        }
> +
> +        if (!iommufd_backend_set_dirty_tracking(container->be,
> +                                                hwpt->hwpt_id, start, errp)) {
> +            goto err;
> +        }
> +    }
> +
> +    return 0;
> +
> +err:
> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
> +            continue;
> +        }
> +        iommufd_backend_set_dirty_tracking(container->be,
> +                                           hwpt->hwpt_id, !start, NULL);
> +    }
> +    return -EINVAL;
> +}
> +
>   static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>   {
>       ERRP_GUARD();
> @@ -278,7 +314,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>       QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>       QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>       container->bcontainer.dirty_pages_supported |=
> -                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
> +                              iommufd_hwpt_dirty_tracking(hwpt);
>       return true;
>   }
>   
> @@ -717,6 +753,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
>       vioc->attach_device = iommufd_cdev_attach;
>       vioc->detach_device = iommufd_cdev_detach;
>       vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
> +    vioc->set_dirty_page_tracking = iommufd_set_dirty_page_tracking;
>   };
>   
>   static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
> diff --git a/backends/trace-events b/backends/trace-events
> index 4d8ac02fe7d6..28aca3b859d4 100644
> --- a/backends/trace-events
> +++ b/backends/trace-events
> @@ -16,3 +16,4 @@ iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t si
>   iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>   iommufd_backend_alloc_hwpt(int iommufd, uint32_t dev_id, uint32_t pt_id, uint32_t flags, uint32_t hwpt_type, uint32_t len, uint64_t data_ptr, uint32_t out_hwpt_id, int ret) " iommufd=%d dev_id=%u pt_id=%u flags=0x%x hwpt_type=%u len=%u data_ptr=0x%"PRIx64" out_hwpt=%u (%d)"
>   iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d (%d)"
> +iommufd_backend_set_dirty(int iommufd, uint32_t hwpt_id, bool start, int ret) " iommufd=%d hwpt=%u enable=%d (%d)"