Re: [PATCH v7 8/9] vfio/pci: Extend VFIO_DEVICE_GET_PCI_HOT_RESET_INFO for vfio device cdev

From: Jason Gunthorpe <jgg@nvidia.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: "Liu, Yi L" <yi.l.liu@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	"eric.auger@redhat.com" <eric.auger@redhat.com>,
	"nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
	"chao.p.peng@linux.intel.com" <chao.p.peng@linux.intel.com>,
	"yi.y.sun@linux.intel.com" <yi.y.sun@linux.intel.com>,
	"peterx@redhat.com" <peterx@redhat.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"shameerali.kolothum.thodi@huawei.com" 
	<shameerali.kolothum.thodi@huawei.com>,
	"lulu@redhat.com" <lulu@redhat.com>,
	"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
	"intel-gvt-dev@lists.freedesktop.org" 
	<intel-gvt-dev@lists.freedesktop.org>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"Hao, Xudong" <xudong.hao@intel.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>,
	"Xu, Terrence" <terrence.xu@intel.com>,
	"Jiang, Yanting" <yanting.jiang@intel.com>,
	"Duan, Zhenzhong" <zhenzhong.duan@intel.com>,
	"clegoate@redhat.com" <clegoate@redhat.com>
Subject: Re: [PATCH v7 8/9] vfio/pci: Extend VFIO_DEVICE_GET_PCI_HOT_RESET_INFO for vfio device cdev
Date: Tue, 13 Jun 2023 14:40:10 -0300	[thread overview]
Message-ID: <ZIip+nGKr8+jZb1z@nvidia.com> (raw)
In-Reply-To: <20230613083229.44a3f3ec.alex.williamson@redhat.com>

On Tue, Jun 13, 2023 at 08:32:29AM -0600, Alex Williamson wrote:
> On Tue, 13 Jun 2023 12:50:43 +0000
> "Liu, Yi L" <yi.l.liu@intel.com> wrote:
> 
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, June 13, 2023 7:47 PM
> > > 
> > > On Fri, Jun 02, 2023 at 05:15:14AM -0700, Yi Liu wrote:  
> > > > +/*
> > > > + * Return devid for a device which is affected by hot-reset.
> > > > + * - valid devid > 0 for the device that is bound to the input
> > > > + *   iommufd_ctx.
> > > > + * - devid == VFIO_PCI_DEVID_OWNED for the device that has not
> > > > + *   been bound to any iommufd_ctx but other device within its
> > > > + *   group has been bound to the input iommufd_ctx.
> > > > + * - devid == VFIO_PCI_DEVID_NOT_OWNED for others. e.g. device
> > > > + *   is bound to other iommufd_ctx etc.
> > > > + */
> > > > +int vfio_iommufd_device_hot_reset_devid(struct vfio_device *vdev,
> > > > +					struct iommufd_ctx *ictx)
> > > > +{
> > > > +	struct iommu_group *group;
> > > > +	int devid;
> > > > +
> > > > +	if (vfio_iommufd_device_ictx(vdev) == ictx)
> > > > +		return vfio_iommufd_device_id(vdev);
> > > > +
> > > > +	group = iommu_group_get(vdev->dev);
> > > > +	if (!group)
> > > > +		return VFIO_PCI_DEVID_NOT_OWNED;
> > > > +
> > > > +	if (iommufd_ctx_has_group(ictx, group))
> > > > +		devid = VFIO_PCI_DEVID_OWNED;
> > > > +	else
> > > > +		devid = VFIO_PCI_DEVID_NOT_OWNED;
> > > > +
> > > > +	iommu_group_put(group);
> > > > +
> > > > +	return devid;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(vfio_iommufd_device_hot_reset_devid);  
> > > 
> > > This function really should not be in the core iommufd.c file - it is
> > > a purely vfio-pci function - why did you have to place it here?  
> > 
> > Put it here can avoid calling iommufd_ctx_has_group() in vfio-pci,
> > which requires to import IOMMUFD_NS. If this reason is not so
> > strong I can move it back to vfio-pci code.
> 
> The PCI-isms here are the name of the function and the return value,
> otherwise this is simply a "give me the devid for this device in this
> context".  The function name is trivial to change and we can define the
> internal errno usage such that the vfio-pci-core code can interpret the
> correct uAPI value.  For example, -EXDEV ("Cross-device link") could
> maybe be interpreted as owned and any other errno is not-owned, -ENODEV
> maybe being the default.

Yeah, this approach seems logical

If the function is called

  vfio_iommufd_get_dev_id(struct vfio_device *vdev, struct iommufd_ctx *ictx)

Then maybe 
  ENOENT = device is owned but there is no Id
  ENODEV = device is not owned

EXDEV is good too, nice symmetry with ENODEV - it doesn't really
matter since there is only one caller and there is no embedded errno
propogation.

Jason