Re: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty tracking capability

[Cédric Le Goater]

On 7/23/24 08:13, Joao Martins wrote:
> On 23/07/2024 06:11, Duan, Zhenzhong wrote:
> > > -----Original Message-----
> > > From: Joao Martins <joao.m.martins@oracle.com>
> > > Subject: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty
> > > tracking capability
> > >
> > > In preparation to using the dirty tracking UAPI, probe whether the IOMMU
> > > supports dirty tracking. This is done via the data stored in
> > > hiod::caps::hw_caps initialized from GET_HW_INFO.
> > >
> > > Qemu doesn't know if VF dirty tracking is supported when allocating
> > > hardware pagetable in iommufd_cdev_autodomains_get(). This is because
> > > VFIODevice migration state hasn't been initialized *yet* hence it can't pick
> > > between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports
> > > dirty tracking it always creates HWPTs with
> > > IOMMU_HWPT_ALLOC_DIRTY_TRACKING
> > > even if later on VFIOMigration decides to use VF dirty tracking instead.
> > >
> > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > ---
> > > include/hw/vfio/vfio-common.h |  2 ++
> > > hw/vfio/iommufd.c             | 20 ++++++++++++++++++++
> > > 2 files changed, 22 insertions(+)
> > >
> > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
> > > common.h
> > > index 4e44b26d3c45..1e02c98b09ba 100644
> > > --- a/include/hw/vfio/vfio-common.h
> > > +++ b/include/hw/vfio/vfio-common.h
> > > @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend;
> > >
> > > typedef struct VFIOIOASHwpt {
> > >      uint32_t hwpt_id;
> > > +    uint32_t hwpt_flags;
> > >      QLIST_HEAD(, VFIODevice) device_list;
> > >      QLIST_ENTRY(VFIOIOASHwpt) next;
> > > } VFIOIOASHwpt;
> > > @@ -139,6 +140,7 @@ typedef struct VFIODevice {
> > >      OnOffAuto pre_copy_dirty_page_tracking;
> > >      bool dirty_pages_supported;
> > >      bool dirty_tracking;
> > > +    bool iommu_dirty_tracking;
> > >      HostIOMMUDevice *hiod;
> > >      int devid;
> > >      IOMMUFDBackend *iommufd;
> > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> > > index 2324bf892c56..7afea0b041ed 100644
> > > --- a/hw/vfio/iommufd.c
> > > +++ b/hw/vfio/iommufd.c
> > > @@ -110,6 +110,11 @@ 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->hwpt_flags &
> > > IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> > > +}
> > > +
> > > static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
> > > {
> > >      ERRP_GUARD();
> > > @@ -246,6 +251,17 @@ static bool
> > > iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
> > >          }
> > >      }
> > >
> > > +    /*
> > > +     * This is quite early and VFIO Migration state isn't yet fully
> > > +     * initialized, thus rely only on IOMMU hardware capabilities as to
> > > +     * whether IOMMU dirty tracking is going to be requested. Later
> > > +     * vfio_migration_realize() may decide to use VF dirty tracking
> > > +     * instead.
> > > +     */
> > > +    if (vbasedev->hiod->caps.hw_caps &
> > > IOMMU_HW_CAP_DIRTY_TRACKING) {
> > > +        flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> > > +    }
> > > +
> > >      if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid,
> > >                                      container->ioas_id, flags,
> > >                                      IOMMU_HWPT_DATA_NONE, 0, NULL,
> > > @@ -255,6 +271,7 @@ static bool
> > > iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
> > >
> > >      hwpt = g_malloc0(sizeof(*hwpt));
> > >      hwpt->hwpt_id = hwpt_id;
> > > +    hwpt->hwpt_flags = flags;
> > >      QLIST_INIT(&hwpt->device_list);
> > >
> > >      ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp);
> > > @@ -265,8 +282,11 @@ static bool
> > > iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
> > >      }
> > >
> > >      vbasedev->hwpt = hwpt;
> > > +    vbasedev->iommu_dirty_tracking =
> > > iommufd_hwpt_dirty_tracking(hwpt);
> >
> > Don't we need to do same if attach to existing hwpt?
>
> Nice catch!
>
> Yes, we do need it e.g. we will need this fix up fo this patch


Fixed on vfio-9.1.

Thanks,

C.



> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 92b976464283..833a7400486c 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -305,6 +305,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice 
> *vbasedev,
>           } else {
>               vbasedev->hwpt = hwpt;
>               QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
> +            vbasedev->iommu_dirty_tracking = iommufd_hwpt_dirty_tracking(hwpt);
>               return true;
>           }
>       }