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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: Yishai Hadas <yishaih@nvidia.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"maorg@nvidia.com" <maorg@nvidia.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	liulongfang <liulongfang@huawei.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	Shameerali Kolothum Thodi  <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH vfio 2/2] vfio: Split migration ops from main device ops
Date: Fri, 24 Jun 2022 11:12:37 -0300	[thread overview]
Message-ID: <20220624141237.GQ4147@nvidia.com> (raw)
In-Reply-To: <20220621104146.368b429a.alex.williamson@redhat.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