Re: [PATCH vfio 2/2] vfio: Split migration ops from main device ops

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jun 21, 2022 at 10:41:46AM -0600, Alex Williamson wrote:
> On Mon, 20 Jun 2022 00:49:09 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Sun, Jun 19, 2022 at 12:25:50PM +0300, Yishai Hadas wrote:
> > 
> > > Means, staying with a single device_ops but just inline a check whether
> > > migration is really supported inside the migration get/set state callbacks
> > > and let the core call it unconditionally.  
> > 
> > I find it much cleaner to have op == NULL means unsupported.
> > 
> > As soon as you start linking supported/unsupported to other flags it
> > can get very complicated fairly fast. I have this experiance from RDMA
> > where we've spent a long time now ripping out hundreds of flag tests
> > and replacing them with NULL op checks. Many bugs were fixed doing
> > this as drivers never fully understood what the flags mean and ended
> > up with flags set that their driver doesn't properly implement.
> > 
> > The mistake we made in RDMA was not splitting the ops, instead the ops
> > were left mutable so the driver could load the right combination based
> > on HW ability.
> 
> I don't really have an issue with splitting the ops, but what
> techniques have you learned from RDMA to make drivers setting ops less
> ad-hoc than proposed here?  Are drivers expected to set ops before a
> formally defined registration point?  

Yes, the flow is the same three step process as in VFIO:

 vfio_init_group_dev()
 [driver continues to prepare the vfio_device]
 vfio_register_group_dev()

I included the 'ops' as an argument to vfio_init_group_dev() as a code
reduction not a statement that the ops must be fully set before
vfio_init_group_dev() returns.

The entire point of the init step is to allow the core and driver to
co-operate and fully initialize the object before moving to register.

So I don't view it as ad-hoc that the object needs further setup after
vfio_init_group_dev().

> Is there an API for setting ops or is it open coded per driver?

RDMA uses a function ib_set_device_ops() but that is only because
there is alot of code in that function to do the copying of ops
pointers. Splitting avoids the copying so we don't really need a
function.

Jason