Re: [PATCH v6 6/9] vfio/iommufd: Implement VFIOIOMMUClass::set_dirty_tracking support

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

On 23/07/2024 09:03, Eric Auger wrote:
> Hi Joao,
> 
> On 7/22/24 23:13, Joao Martins wrote:
>> ioctl(iommufd, IOMMU_HWPT_SET_DIRTY_TRACKING, arg) is the UAPI that
>> enables or disables dirty page tracking. The ioctl 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.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/sysemu/iommufd.h |  2 ++
>>  backends/iommufd.c       | 23 +++++++++++++++++++++++
>>  hw/vfio/iommufd.c        | 32 ++++++++++++++++++++++++++++++++
>>  backends/trace-events    |  1 +
>>  4 files changed, 58 insertions(+)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index e917e7591d05..6fb412f61144 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -55,6 +55,8 @@ 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 06b135111f30..b97883503884 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -238,6 +238,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 ? IOMMU_HWPT_DIRTY_TRACKING_ENABLE : 0,
>> +    };
>> +
>> +    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 7afea0b041ed..b882a3f59a6e 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -115,6 +115,37 @@ static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>>      return hwpt && 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 here I see you handle the case where we have hwpts with and without
> support for dirty tracking within the same container so I guess this
> answers my previous question. So do you want to tag a container as
> dirty_pages_supported = true as soon as one device is backed up with a
> dirty tracking hwpt within that container? I think I miss the high level
> view of when this case may happen and why the devices do not end up in
> different containers/ioas. But maybe I completely mix up things &
> objects. Eric

I think the only gap I have in this series that I don't catch exactly right in
all this logic, is when the IOMMU dirty tracking is not homogeneous, which
remains to be seen in pratice (from h/w perspective). That's where we currently
have a gap here. Other than that, either we do 'all VFs do dirty tracking' or
'all devices are backed by IOMMU'.

A container may have different hwpt with different capabilities e.g. systems
where IOMMU instances have different dirty tracking ability (again I don't know
if these exist). Which, on hwpt without dirty tracking, on which the devices
attached to it that lack VF dirty tracking should be added an LM blocker.

These checks were added from the previous cycles, but I was actually thinking in
making these g_assert() to make sure this is not exercised like that. But
considering I plan on improve a mixed usage of VF dirty tracking with IOMMU I
left them as simply checks.

>> +
>> +        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();
>> @@ -725,6 +756,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)"
>