On Fri, 24 Jun 2022 11:12:37 -0300 Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > 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@xxxxxxxxxx> 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. So maybe we just need a comment in the definition of the vfio_migration_ops that vfio_device.mig_ops is a static property of the vfio_device which must be set prior to registering the vfio_device. Thanks, Alex
