RE: [PATCH] vfio: Split migration ops from main device ops

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgg@nvidia.com]
> Sent: 24 May 2022 00:28
> To: Alex Williamson <alex.williamson@redhat.com>
> Cc: Yishai Hadas <yishaih@nvidia.com>; kvm@vger.kernel.org;
> maorg@nvidia.com; cohuck@redhat.com; kevin.tian@intel.com; Shameerali
> Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; liulongfang
> <liulongfang@huawei.com>
> Subject: Re: [PATCH] vfio: Split migration ops from main device ops
> 
> On Mon, May 23, 2022 at 11:25:00AM -0600, Alex Williamson wrote:
> > On Sun, 22 May 2022 12:47:56 +0300
> > Yishai Hadas <yishaih@nvidia.com> wrote:
> >
> > > vfio core checks whether the driver sets some migration op (e.g.
> > > set_state/get_state) and accordingly calls its op.
> > >
> > > However, currently mlx5 driver sets the above ops without regards to
> > > its migration caps.
> > >
> > > This might lead to unexpected usage/Oops if user space may call to
> > > the above ops even if the driver doesn't support migration. As for
> > > example, the migration state_mutex is not initialized in that case.
> > >
> > > The cleanest way to manage that seems to split the migration ops
> > > from the main device ops, this will let the driver setting them
> > > separately from the main ops when it's applicable.
> > >
> > > As part of that, cleaned-up HISI driver to match this scheme.
> > >
> > > This scheme may enable down the road to come with some extra group
> > > of ops (e.g. DMA log) that can be set without regards to the other
> > > options based on driver caps.
> >
> > It seems like the hisi-acc driver already manages this by registering
> > different structs based on the device migration capabilities, why is
> > that not the default solution here?  Or of course the mlx5 driver
> > could test the migration capabilities before running into the weeds.
> > We also have vfio_device.migration_flags which could factor in here as well.
> 
> It starts to hit combinatoral explosion when the next patches add ops for dirty
> logging that may be optional too. This is simpler and simpifies the hisi driver to
> remove the 2nd ops too.

Hmm..but for hisi driver we still need to have those mmap/ioctl/read/write override 
functions to restrict the BAR region exposure to Guest in case of migration support. So I
am not sure we can get rid of two ops easily there(Though, we could add an explicit
check for the migration support in those callbacks).

Thanks,
Shameer