Re: [PATCH v2 07/13] vfio/ccw: Convert to use vfio_register_group_dev()

From: Jason Gunthorpe <jgg@nvidia.com>
To: Cornelia Huck <cohuck@redhat.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
	Eric Farman <farman@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	kvm@vger.kernel.org, linux-s390@vger.kernel.org,
	Peter Oberparleiter <oberpar@linux.ibm.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Vineeth Vijayan <vneethv@linux.ibm.com>,
	"Raj, Ashok" <ashok.raj@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Daniel Vetter <daniel@ffwll.ch>, Christoph Hellwig <hch@lst.de>,
	Leon Romanovsky <leonro@nvidia.com>,
	Max Gurtovoy <mgurtovoy@nvidia.com>,
	Tarun Gupta <targupta@nvidia.com>
Subject: Re: [PATCH v2 07/13] vfio/ccw: Convert to use vfio_register_group_dev()
Date: Wed, 28 Apr 2021 14:20:08 -0300	[thread overview]
Message-ID: <20210428172008.GV1370958@nvidia.com> (raw)
In-Reply-To: <20210428190949.4360afb7.cohuck@redhat.com>

On Wed, Apr 28, 2021 at 07:09:49PM +0200, Cornelia Huck wrote:
> On Mon, 26 Apr 2021 17:00:09 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > This is more complicated because vfio_ccw is sharing the vfio_device
> > between both the mdev_device and its vfio_device and the css_driver.
> > 
> > The mdev is a singleton, and the reason for this sharing appears to be to
> > allow the extra css_driver function callbacks to be delivered to the
> > vfio_device.
> > 
> > This keeps things as they were, with the css_driver allocating the
> > singleton, not the mdev_driver, this is pretty confusing. I'm also
> > uncertain how the lifetime model for the mdev works in the css_driver
> > callbacks.
> > 
> > At this point embed the vfio_device in the vfio_ccw_private and
> > instantiate it as a vfio_device when the mdev probes. The drvdata of both
> > the css_device and the mdev_device point at the private, and container_of
> > is used to get it back from the vfio_device.
> 
> I've been staring at this for some time, and I'm not sure whether this
> is a good approach.
> 
> We allow at most one mdev per subchannel (slicing it up does not make
> sense), so we can be sure that there's a 1:1 relationship between mdev
> and parent device, and we can track it via a single pointer.

This seems like one of these cases where using the mdev GUID API was not a
great fit. The ccs_driver should have just directly created a
vfio_device and not gone into the mdev guid lifecycle world.

> The vfio_ccw_private driver data is allocated during probe (same as for
> other css_drivers.) Embedding a vfio_device here means that we have a
> structure tied into it that is operating with different lifetime rules.
> 
> What about creating a second structure instead that can embed the
> vfio_device, is allocated during mdev probing, and is linked up with
> the vfio_ccw_private structure? That would follow the pattern of other
> drivers more closely.

IIRC we still end up with pointers crossing between the two
structs. If you can't convince yourself that is correct (and I could
not) then it is already buggy today.

It is as I said to Eric, either there is no concurrency when there is
no mdev and everything is correct today, or there is concurrency and
it seems buggy today too.

The right answer it to move the allocations out of the css_driver
probe and put them only in the mdev driver probe because they can only
make sense when the mdev driver is instantiated. Then everything is
clear and very understandable how it should work.

I almost did this, but couldn't figure out how the lifetime of the
ccs_driver callbacks are working relative to the lifetime of the mdev
device since they also reach into these structs. Maybe they can't be
called for some css related reason?

Jason