Re: [PATCH v3 07/10] vfio/iommufd: Implement VFIOIOMMUClass::query_dirty_bitmap support

[Joao Martins <joao.m.martins@oracle.com>]

On 09/07/2024 10:13, Joao Martins wrote:
> On 09/07/2024 08:05, Cédric Le Goater wrote:
>> On 7/8/24 4:34 PM, Joao Martins wrote:
>>> ioctl(iommufd, IOMMU_HWPT_GET_DIRTY_BITMAP, arg) is the UAPI
>>> that fetches the bitmap that tells what was dirty in an IOVA
>>> range.
>>>
>>> A single bitmap is allocated and used across all the hwpts
>>> sharing an IOAS which is then used in log_sync() to set Qemu
>>> global bitmaps.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>   include/sysemu/iommufd.h |  3 +++
>>>   backends/iommufd.c       | 26 ++++++++++++++++++++++++++
>>>   hw/vfio/iommufd.c        | 34 ++++++++++++++++++++++++++++++++++
>>>   backends/trace-events    |  1 +
>>>   4 files changed, 64 insertions(+)
>>>
>>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>>> index 1470377f55ba..223f1ea14e84 100644
>>> --- a/include/sysemu/iommufd.h
>>> +++ b/include/sysemu/iommufd.h
>>> @@ -56,6 +56,9 @@ int iommufd_backend_alloc_hwpt(IOMMUFDBackend *be, uint32_t
>>> dev_id,
>>>                                  void *data_ptr, uint32_t *out_hwpt);
>>>   int iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t hwpt_id,
>>>                                          bool start);
>>> +int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>>> +                                     uint64_t iova, ram_addr_t size,
>>> +                                     uint64_t page_size, uint64_t *data);
>>>     #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>>   diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index 69daabc27473..b2d3bbd7c31b 100644
>>> --- a/backends/iommufd.c
>>> +++ b/backends/iommufd.c
>>> @@ -257,6 +257,32 @@ int iommufd_backend_set_dirty_tracking(IOMMUFDBackend
>>> *be, uint32_t hwpt_id,
>>>       return ret;
>>>   }
>>>   +int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>>> +                                     uint64_t iova, ram_addr_t size,
>>> +                                     uint64_t page_size, uint64_t *data)
>>> +{
>>> +    int ret;
>>> +    struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap = {
>>> +        .size = sizeof(get_dirty_bitmap),
>>> +        .hwpt_id = hwpt_id,
>>> +        .iova = iova,
>>> +        .length = size,
>>> +        .page_size = page_size,
>>> +        .data = (uintptr_t)data,
>>> +    };
>>> +
>>> +    ret = ioctl(be->fd, IOMMU_HWPT_GET_DIRTY_BITMAP, &get_dirty_bitmap);
>>> +    trace_iommufd_backend_get_dirty_bitmap(be->fd, hwpt_id, iova, size,
>>> +                                           page_size, ret);
>>> +    if (ret) {
>>> +        error_report("IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"PRIx64
>>> +                     " size: 0x%"PRIx64") failed: %s", iova,
>>> +                     size, strerror(errno));
>>> +    }
>>> +
>>> +    return !ret ? 0 : -errno;
>>> +}
>>> +
>>>   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 158a98cb3b12..9fad47baed9e 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -25,6 +25,7 @@
>>>   #include "qemu/cutils.h"
>>>   #include "qemu/chardev_open.h"
>>>   #include "pci.h"
>>> +#include "exec/ram_addr.h"
>>>     static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova,
>>>                               ram_addr_t size, void *vaddr, bool readonly)
>>> @@ -152,6 +153,38 @@ err:
>>>       return ret;
>>>   }
>>>   +static int iommufd_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>>> +                                      VFIOBitmap *vbmap, hwaddr iova,
>>> +                                      hwaddr size, Error **errp)
>>> +{
>>> +    VFIOIOMMUFDContainer *container = container_of(bcontainer,
>>> +                                                   VFIOIOMMUFDContainer,
>>> +                                                   bcontainer);
>>> +    int ret;
>>> +    VFIOIOASHwpt *hwpt;
>>> +    unsigned long page_size;
>>> +
>>> +    page_size = qemu_real_host_page_size();
>>> +    QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>> +        if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>>> +            continue;
>>> +        }
>>> +
>>> +        ret = iommufd_backend_get_dirty_bitmap(container->be, hwpt->hwpt_id,
>>> +                                               iova, size, page_size,
>>> +                                               vbmap->bitmap);
>>> +        if (ret) {
>>> +            error_setg_errno(errp, ret,
>>> +                             "Failed to get dirty bitmap report, hwpt_id %u,
>>> iova: "
>>> +                             "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
>>> +                             hwpt->hwpt_id, iova, size);
>>
>> This error looks redundant with the one printed out in
>> iommufd_backend_get_dirty_bitmap(). Couldn't we add an 'Error **'
>> parameter and simply return a bool ?
>>
> 
> I'll add it.
> 
> Just as a sidebar: This is a odd pattern which seems somewhat spread, that
> somehow we only care about @errno as something to put on a message, rather then
> letting the user know what exact error code it had returned programmatically.
> Here in this series it is only important for the device attach so likely doesn't
> justify a Error structure enhancement, hence the rest of functions I introduced
> here can just adopt bool+errp.
> 
> 
Looks like this. A bit odd as the container ops (and legacy backend) expect an
errno. But I am assuming that you intended that way.

-int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
-                                     uint64_t iova, ram_addr_t size,
-                                     uint64_t page_size, uint64_t *data)
+bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
+                                      uint32_t hwpt_id,
+                                      uint64_t iova, ram_addr_t size,
+                                      uint64_t page_size, uint64_t *data,
+                                     Error **errp)
 {
     int ret;
     struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap = {
@@ -273,14 +278,15 @@ int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
uint32_t hwpt_id,

     ret = ioctl(be->fd, IOMMU_HWPT_GET_DIRTY_BITMAP, &get_dirty_bitmap);
     trace_iommufd_backend_get_dirty_bitmap(be->fd, hwpt_id, iova, size,
-                                           page_size, ret);
+                                           page_size, ret ? errno : 0);
     if (ret) {
-        error_report("IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"PRIx64
-                     " size: 0x%"PRIx64") failed: %s", iova,
-                     size, strerror(errno));
+        error_setg_errno(errp, errno,
+                         "IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"HWADDR_PRIx
+                         " size: 0x%"HWADDR_PRIx") failed", iova, size);
+        return false;
     }

-    return !ret ? 0 : -errno;
+    return true;
 }

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 35cf8da22ef7..4369df34a6ac 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -160,25 +154,18 @@ static int iommufd_query_dirty_bitmap(const
VFIOContainerBase *bcontainer,
     VFIOIOMMUFDContainer *container = container_of(bcontainer,
                                                    VFIOIOMMUFDContainer,
                                                    bcontainer);
-    int ret;
+    unsigned long page_size = qemu_real_host_page_size();
     VFIOIOASHwpt *hwpt;
-    unsigned long page_size;

-    page_size = qemu_real_host_page_size();
     QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
         if (!iommufd_hwpt_dirty_tracking(hwpt)) {
             continue;
         }

-        ret = iommufd_backend_get_dirty_bitmap(container->be, hwpt->hwpt_id,
-                                               iova, size, page_size,
-                                               vbmap->bitmap);
-        if (ret) {
-            error_setg_errno(errp, ret,
-                             "Failed to get dirty bitmap report, hwpt_id %u,
iova: "
-                             "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
-                             hwpt->hwpt_id, iova, size);
-            return ret;
+        if (!iommufd_backend_get_dirty_bitmap(container->be, hwpt->hwpt_id,
+                                              iova, size, page_size,
+                                              vbmap->bitmap)) {
+            return -EINVAL;
         }
     }

@@ -287,24 +274,11 @@ static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice
*vbasedev, Error **errp)
     return true;
 }
(...)