> -----Original Message----- > From: Jason Gunthorpe [mailto:jgg@xxxxxxxxxx] > Sent: 24 May 2022 00:28 > To: Alex Williamson <alex.williamson@xxxxxxxxxx> > Cc: Yishai Hadas <yishaih@xxxxxxxxxx>; kvm@xxxxxxxxxxxxxxx; > maorg@xxxxxxxxxx; cohuck@xxxxxxxxxx; kevin.tian@xxxxxxxxx; Shameerali > Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; liulongfang > <liulongfang@xxxxxxxxxx> > 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@xxxxxxxxxx> 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
