Re: [PATCH v5 12/13] vfio/migration: Don't block migration device dirty tracking is unsupported

[Cédric Le Goater]

On 7/19/24 19:26, Joao Martins wrote:
> On 19/07/2024 15:24, Joao Martins wrote:
> > On 19/07/2024 15:17, Cédric Le Goater wrote:
> > > On 7/19/24 14:05, Joao Martins wrote:
> > > > By default VFIO migration is set to auto, which will support live
> > > > migration if the migration capability is set *and* also dirty page
> > > > tracking is supported.
> > > >
> > > > For testing purposes one can force enable without dirty page tracking
> > > > via enable-migration=on, but that option is generally left for testing
> > > > purposes.
> > > >
> > > > So starting with IOMMU dirty tracking it can use to accomodate the lack of
> > > > VF dirty page tracking allowing us to minimize the VF requirements for
> > > > migration and thus enabling migration by default for those too.
> > > >
> > > > While at it change the error messages to mention IOMMU dirty tracking as
> > > > well.
> > > >
> > > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> > > > ---
> > > >    include/hw/vfio/vfio-common.h |  1 +
> > > >    hw/vfio/iommufd.c             |  2 +-
> > > >    hw/vfio/migration.c           | 11 ++++++-----
> > > >    3 files changed, 8 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > > > index 7e530c7869dc..00b9e933449e 100644
> > > > --- a/include/hw/vfio/vfio-common.h
> > > > +++ b/include/hw/vfio/vfio-common.h
> > > > @@ -299,6 +299,7 @@ int vfio_devices_query_dirty_bitmap(const
> > > > VFIOContainerBase *bcontainer,
> > > >                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
> > > >    int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
> > > >                              uint64_t size, ram_addr_t ram_addr, Error **errp);
> > > > +bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt);
> > > >      /* Returns 0 on success, or a negative errno. */
> > > >    bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp);
> > > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> > > > index 7dd5d43ce06a..a998e8578552 100644
> > > > --- a/hw/vfio/iommufd.c
> > > > +++ b/hw/vfio/iommufd.c
> > > > @@ -111,7 +111,7 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice
> > > > *vbasedev)
> > > >        iommufd_backend_disconnect(vbasedev->iommufd);
> > > >    }
> > > >    -static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
> > > > +bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
> > > >    {
> > > >        return hwpt && hwpt->hwpt_flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> > > >    }
> > > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > > > index 34d4be2ce1b1..63ffa46c9652 100644
> > > > --- a/hw/vfio/migration.c
> > > > +++ b/hw/vfio/migration.c
> > > > @@ -1036,16 +1036,17 @@ bool vfio_migration_realize(VFIODevice *vbasedev,
> > > > Error **errp)
> > > >            return !vfio_block_migration(vbasedev, err, errp);
> > > >        }
> > > >    -    if (!vbasedev->dirty_pages_supported) {
> > > > +    if (!vbasedev->dirty_pages_supported &&
> > > > +        !iommufd_hwpt_dirty_tracking(vbasedev->hwpt)) {
> > >
> > >
> > > Some platforms do not have IOMMUFD support and this call will need
> > > some kind of abstract wrapper to reflect dirty tracking support in
> > > the IOMMU backend.
> >
> > This was actually on purpose because only IOMMUFD presents a view of hardware
> > whereas type1 supporting dirty page tracking is not used as means to 'migration
> > is supported'.
> >
> > The hwpt is nil in type1 and the helper checks that, so it should return false.
>
> Oh wait, maybe you're talking about CONFIG_IOMMUFD=n which I totally didn't
> consider. Maybe this would be a elegant way to address it? Looks to pass my
> build with CONFIG_IOMMUFD=n
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 61dd48e79b71..422ad4a5bdd1 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -300,7 +300,14 @@ int vfio_devices_query_dirty_bitmap(const VFIOContainerBase
> *bcontainer,
>                   VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>   int vfio_get_dirty_bitmap(const VFIOContainerBase *bcontainer, uint64_t iova,
>                             uint64_t size, ram_addr_t ram_addr, Error **errp);
> +#ifdef CONFIG_IOMMUFD
>   bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt);
> +#else
> +static inline bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
> +{
> +    return false;
> +}
> +#endif
>
>   /* Returns 0 on success, or a negative errno. */
>   bool vfio_device_get_name(VFIODevice *vbasedev, Error **errp);

hmm, no. You will need to introduce a new Host IOMMU device capability,
something like :

   HOST_IOMMU_DEVICE_CAP_DIRTY_TRACKING,

Then, introduce an helper routine to check the capability  :

   return hiodc->get_cap( ... HOST_IOMMU_DEVICE_CAP_DIRTY_TRACKING...)
 
and replace the iommufd_hwpt_dirty_tracking call with it.

Yeah I know, it's cumbersome but it's cleaner !

That's not a major problem in the series. I can address it at the end
to avoid a resend. First, let's get a R-b on all other patches.

Thanks,

C.