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

From: Cornelia Huck <cohuck@redhat.com>
To: Jason Gunthorpe <jgg@nvidia.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 19:09:49 +0200	[thread overview]
Message-ID: <20210428190949.4360afb7.cohuck@redhat.com> (raw)
In-Reply-To: <7-v2-7667f42c9bad+935-vfio3_jgg@nvidia.com>

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.

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.

> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     |  21 +++--
>  drivers/s390/cio/vfio_ccw_ops.c     | 135 +++++++++++++++-------------
>  drivers/s390/cio/vfio_ccw_private.h |   5 ++
>  3 files changed, 94 insertions(+), 67 deletions(-)