Re: [PATCH v1 02/14] vfio: Update vfio_add_group_dev() API

2021-03-11 Thread Christoph Hellwig 
On Wed, Mar 10, 2021 at 08:28:05AM -0700, Alex Williamson wrote:
> > Yes, that series puts vfio_device everywhere so APIs like Alex needs
> > to build here become trivial.
> > 
> > The fact we both converged on this same requirement is good
> 
> You're ahead of me in catching up with reviews Christoph, but
> considering stable backports and the motivations for each series, I'd
> expect to initially make the minimal API change and build from there.

Given how many exploitable bugs the work from Jason has found/fixed
I'd rather do it properly.  As most of this will have to be backported
for anyone who cares.

Re: [PATCH v1 02/14] vfio: Update vfio_add_group_dev() API

2021-03-10 Thread Alex Williamson 
On Wed, 10 Mar 2021 08:19:13 -0400
Jason Gunthorpe  wrote:

> On Wed, Mar 10, 2021 at 07:48:38AM +, Christoph Hellwig wrote:
> > On Mon, Mar 08, 2021 at 02:47:40PM -0700, Alex Williamson wrote:  
> > > Rather than an errno, return a pointer to the opaque vfio_device
> > > to allow the bus driver to call into vfio-core without additional
> > > lookups and references.  Note that bus drivers are still required
> > > to use vfio_del_group_dev() to teardown the vfio_device.
> > > 
> > > Signed-off-by: Alex Williamson   
> > 
> > This looks like it is superseded by the
> > 
> >   vfio: Split creation of a vfio_device into init and register ops  
> 
> Yes, that series puts vfio_device everywhere so APIs like Alex needs
> to build here become trivial.
> 
> The fact we both converged on this same requirement is good

You're ahead of me in catching up with reviews Christoph, but
considering stable backports and the motivations for each series, I'd
expect to initially make the minimal API change and build from there.
Thanks,

Alex

Re: [PATCH v1 02/14] vfio: Update vfio_add_group_dev() API

2021-03-10 Thread Jason Gunthorpe 
On Wed, Mar 10, 2021 at 07:48:38AM +, Christoph Hellwig wrote:
> On Mon, Mar 08, 2021 at 02:47:40PM -0700, Alex Williamson wrote:
> > Rather than an errno, return a pointer to the opaque vfio_device
> > to allow the bus driver to call into vfio-core without additional
> > lookups and references.  Note that bus drivers are still required
> > to use vfio_del_group_dev() to teardown the vfio_device.
> > 
> > Signed-off-by: Alex Williamson 
> 
> This looks like it is superseded by the
> 
>   vfio: Split creation of a vfio_device into init and register ops

Yes, that series puts vfio_device everywhere so APIs like Alex needs
to build here become trivial.

The fact we both converged on this same requirement is good

Jason

Re: [PATCH v1 02/14] vfio: Update vfio_add_group_dev() API

2021-03-09 Thread Christoph Hellwig 
On Mon, Mar 08, 2021 at 02:47:40PM -0700, Alex Williamson wrote:
> Rather than an errno, return a pointer to the opaque vfio_device
> to allow the bus driver to call into vfio-core without additional
> lookups and references.  Note that bus drivers are still required
> to use vfio_del_group_dev() to teardown the vfio_device.
> 
> Signed-off-by: Alex Williamson 

This looks like it is superseded by the

  vfio: Split creation of a vfio_device into init and register ops

patch from Jason, which provides a much nicer API.

[PATCH v1 02/14] vfio: Update vfio_add_group_dev() API

2021-03-08 Thread Alex Williamson 
Rather than an errno, return a pointer to the opaque vfio_device
to allow the bus driver to call into vfio-core without additional
lookups and references.  Note that bus drivers are still required
to use vfio_del_group_dev() to teardown the vfio_device.

Signed-off-by: Alex Williamson 
---
 Documentation/driver-api/vfio.rst|6 +++---
 drivers/vfio/fsl-mc/vfio_fsl_mc.c|6 --
 drivers/vfio/mdev/vfio_mdev.c|5 -
 drivers/vfio/pci/vfio_pci.c  |7 +--
 drivers/vfio/platform/vfio_platform_common.c |7 +--
 drivers/vfio/vfio.c  |   23 ++-
 include/linux/vfio.h |6 +++---
 7 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/Documentation/driver-api/vfio.rst 
b/Documentation/driver-api/vfio.rst
index f1a4d3c3ba0b..03e978eb8ec7 100644
--- a/Documentation/driver-api/vfio.rst
+++ b/Documentation/driver-api/vfio.rst
@@ -252,9 +252,9 @@ into VFIO core.  When devices are bound and unbound to the 
driver,
 the driver should call vfio_add_group_dev() and vfio_del_group_dev()
 respectively::
 
-   extern int vfio_add_group_dev(struct device *dev,
- const struct vfio_device_ops *ops,
- void *device_data);
+   extern struct vfio_device *vfio_add_group_dev(struct device *dev,
+   const struct vfio_device_ops *ops,
+   void *device_data);
 
extern void *vfio_del_group_dev(struct device *dev);
 
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c 
b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index f27e25112c40..a4c2d0b9cd51 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -592,6 +592,7 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
struct iommu_group *group;
struct vfio_fsl_mc_device *vdev;
struct device *dev = _dev->dev;
+   struct vfio_device *device;
int ret;
 
group = vfio_iommu_group_get(dev);
@@ -608,8 +609,9 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
 
vdev->mc_dev = mc_dev;
 
-   ret = vfio_add_group_dev(dev, _fsl_mc_ops, vdev);
-   if (ret) {
+   device = vfio_add_group_dev(dev, _fsl_mc_ops, vdev);
+   if (IS_ERR(device)) {
+   ret = PTR_ERR(device);
dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
goto out_group_put;
}
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index b52eea128549..ebae3871b155 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -124,8 +124,11 @@ static const struct vfio_device_ops vfio_mdev_dev_ops = {
 static int vfio_mdev_probe(struct device *dev)
 {
struct mdev_device *mdev = to_mdev_device(dev);
+   struct vfio_device *device;
 
-   return vfio_add_group_dev(dev, _mdev_dev_ops, mdev);
+   device = vfio_add_group_dev(dev, _mdev_dev_ops, mdev);
+
+   return PTR_ERR_OR_ZERO(device);
 }
 
 static void vfio_mdev_remove(struct device *dev)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 65e7e6b44578..f0a1d05f0137 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1926,6 +1926,7 @@ static int vfio_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *id)
 {
struct vfio_pci_device *vdev;
struct iommu_group *group;
+   struct vfio_device *device;
int ret;
 
if (vfio_pci_is_denylisted(pdev))
@@ -1968,9 +1969,11 @@ static int vfio_pci_probe(struct pci_dev *pdev, const 
struct pci_device_id *id)
INIT_LIST_HEAD(>vma_list);
init_rwsem(>memory_lock);
 
-   ret = vfio_add_group_dev(>dev, _pci_ops, vdev);
-   if (ret)
+   device = vfio_add_group_dev(>dev, _pci_ops, vdev);
+   if (IS_ERR(device)) {
+   ret = PTR_ERR(device);
goto out_free;
+   }
 
ret = vfio_pci_reflck_attach(vdev);
if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c 
b/drivers/vfio/platform/vfio_platform_common.c
index fb4b385191f2..ff41fe0b758e 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -657,6 +657,7 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
   struct device *dev)
 {
struct iommu_group *group;
+   struct vfio_device *device;
int ret;
 
if (!vdev)
@@ -685,9 +686,11 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
goto put_reset;
}
 
-   ret = vfio_add_group_dev(dev, _platform_ops, vdev);
-   if (ret)
+   device = vfio_add_group_dev(dev, _platform_ops, vdev);
+   if (IS_ERR(device)) {
+   ret = PTR_ERR(device);
goto