* [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 20:25 ` Alex Williamson
2021-08-24 14:46 ` [PATCH 02/14] vfio: factor out a vfio_iommu_driver_allowed helper Christoph Hellwig
` (12 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm, Jason Gunthorpe
From: Jason Gunthorpe <jgg@nvidia.com>
We don't need to hold a reference to the group in the driver as well as
obtain a reference to the same group as the first thing
vfio_register_group_dev() does.
Since the drivers never use the group move this all into the core code.
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 17 ++-----------
drivers/vfio/pci/vfio_pci.c | 15 ++----------
drivers/vfio/platform/vfio_platform_common.c | 13 +---------
drivers/vfio/vfio.c | 25 ++++++--------------
include/linux/vfio.h | 3 ---
5 files changed, 12 insertions(+), 61 deletions(-)
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 0ead91bfa83867..9e838fed560339 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -505,22 +505,13 @@ static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev)
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 = &mc_dev->dev;
int ret;
- group = vfio_iommu_group_get(dev);
- if (!group) {
- dev_err(dev, "VFIO_FSL_MC: No IOMMU group\n");
- return -EINVAL;
- }
-
vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
- if (!vdev) {
- ret = -ENOMEM;
- goto out_group_put;
- }
+ if (!vdev)
+ return -ENOMEM;
vfio_init_group_dev(&vdev->vdev, dev, &vfio_fsl_mc_ops);
vdev->mc_dev = mc_dev;
@@ -556,8 +547,6 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
out_uninit:
vfio_uninit_group_dev(&vdev->vdev);
kfree(vdev);
-out_group_put:
- vfio_iommu_group_put(group, dev);
return ret;
}
@@ -574,8 +563,6 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
vfio_uninit_group_dev(&vdev->vdev);
kfree(vdev);
- vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
-
return 0;
}
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index a4f44ea52fa324..b01c1c6cf1f5e6 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -1875,7 +1875,6 @@ static void vfio_pci_vga_uninit(struct vfio_pci_device *vdev)
static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct vfio_pci_device *vdev;
- struct iommu_group *group;
int ret;
if (vfio_pci_is_denylisted(pdev))
@@ -1897,15 +1896,9 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
return -EBUSY;
}
- group = vfio_iommu_group_get(&pdev->dev);
- if (!group)
- return -EINVAL;
-
vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
- if (!vdev) {
- ret = -ENOMEM;
- goto out_group_put;
- }
+ if (!vdev)
+ return -ENOMEM;
vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops);
vdev->pdev = pdev;
@@ -1971,8 +1964,6 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
vfio_uninit_group_dev(&vdev->vdev);
kfree(vdev->pm_save);
kfree(vdev);
-out_group_put:
- vfio_iommu_group_put(group, &pdev->dev);
return ret;
}
@@ -1988,8 +1979,6 @@ static void vfio_pci_remove(struct pci_dev *pdev)
vfio_uninit_group_dev(&vdev->vdev);
vfio_pci_vga_uninit(vdev);
- vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
-
if (!disable_idle_d3)
vfio_pci_set_power_state(vdev, PCI_D0);
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index 6af7ce7d619c25..256f55b84e70a0 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -642,7 +642,6 @@ static int vfio_platform_of_probe(struct vfio_platform_device *vdev,
int vfio_platform_probe_common(struct vfio_platform_device *vdev,
struct device *dev)
{
- struct iommu_group *group;
int ret;
vfio_init_group_dev(&vdev->vdev, dev, &vfio_platform_ops);
@@ -663,24 +662,15 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
goto out_uninit;
}
- group = vfio_iommu_group_get(dev);
- if (!group) {
- dev_err(dev, "No IOMMU group for device %s\n", vdev->name);
- ret = -EINVAL;
- goto put_reset;
- }
-
ret = vfio_register_group_dev(&vdev->vdev);
if (ret)
- goto put_iommu;
+ goto put_reset;
mutex_init(&vdev->igate);
pm_runtime_enable(dev);
return 0;
-put_iommu:
- vfio_iommu_group_put(group, dev);
put_reset:
vfio_platform_put_reset(vdev);
out_uninit:
@@ -696,7 +686,6 @@ void vfio_platform_remove_common(struct vfio_platform_device *vdev)
pm_runtime_disable(vdev->device);
vfio_platform_put_reset(vdev);
vfio_uninit_group_dev(&vdev->vdev);
- vfio_iommu_group_put(vdev->vdev.dev->iommu_group, vdev->vdev.dev);
}
EXPORT_SYMBOL_GPL(vfio_platform_remove_common);
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 3c034fe14ccb03..5bd520f0dc6107 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -169,15 +169,7 @@ static void vfio_release_device_set(struct vfio_device *device)
xa_unlock(&vfio_device_set_xa);
}
-/*
- * vfio_iommu_group_{get,put} are only intended for VFIO bus driver probe
- * and remove functions, any use cases other than acquiring the first
- * reference for the purpose of calling vfio_register_group_dev() or removing
- * that symmetric reference after vfio_unregister_group_dev() should use the raw
- * iommu_group_{get,put} functions. In particular, vfio_iommu_group_put()
- * removes the device from the dummy group and cannot be nested.
- */
-struct iommu_group *vfio_iommu_group_get(struct device *dev)
+static struct iommu_group *vfio_iommu_group_get(struct device *dev)
{
struct iommu_group *group;
int __maybe_unused ret;
@@ -220,9 +212,8 @@ struct iommu_group *vfio_iommu_group_get(struct device *dev)
return group;
}
-EXPORT_SYMBOL_GPL(vfio_iommu_group_get);
-void vfio_iommu_group_put(struct iommu_group *group, struct device *dev)
+static void vfio_iommu_group_put(struct iommu_group *group, struct device *dev)
{
#ifdef CONFIG_VFIO_NOIOMMU
if (iommu_group_get_iommudata(group) == &noiommu)
@@ -231,7 +222,6 @@ void vfio_iommu_group_put(struct iommu_group *group, struct device *dev)
iommu_group_put(group);
}
-EXPORT_SYMBOL_GPL(vfio_iommu_group_put);
#ifdef CONFIG_VFIO_NOIOMMU
static void *vfio_noiommu_open(unsigned long arg)
@@ -841,7 +831,7 @@ int vfio_register_group_dev(struct vfio_device *device)
if (!device->dev_set)
vfio_assign_device_set(device, device);
- iommu_group = iommu_group_get(device->dev);
+ iommu_group = vfio_iommu_group_get(device->dev);
if (!iommu_group)
return -EINVAL;
@@ -849,7 +839,7 @@ int vfio_register_group_dev(struct vfio_device *device)
if (!group) {
group = vfio_create_group(iommu_group);
if (IS_ERR(group)) {
- iommu_group_put(iommu_group);
+ vfio_iommu_group_put(iommu_group, device->dev);
return PTR_ERR(group);
}
} else {
@@ -857,7 +847,7 @@ int vfio_register_group_dev(struct vfio_device *device)
* A found vfio_group already holds a reference to the
* iommu_group. A created vfio_group keeps the reference.
*/
- iommu_group_put(iommu_group);
+ vfio_iommu_group_put(iommu_group, device->dev);
}
existing_device = vfio_group_get_device(group, device->dev);
@@ -865,7 +855,7 @@ int vfio_register_group_dev(struct vfio_device *device)
dev_WARN(device->dev, "Device already exists on group %d\n",
iommu_group_id(iommu_group));
vfio_device_put(existing_device);
- vfio_group_put(group);
+ vfio_iommu_group_put(iommu_group, device->dev);
return -EBUSY;
}
@@ -1010,8 +1000,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
if (list_empty(&group->device_list))
wait_event(group->container_q, !group->container);
- /* Matches the get in vfio_register_group_dev() */
- vfio_group_put(group);
+ vfio_iommu_group_put(group->iommu_group, device->dev);
}
EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index b53a9557884ada..f7083c2fd0d099 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -71,9 +71,6 @@ struct vfio_device_ops {
int (*match)(struct vfio_device *vdev, char *buf);
};
-extern struct iommu_group *vfio_iommu_group_get(struct device *dev);
-extern void vfio_iommu_group_put(struct iommu_group *group, struct device *dev);
-
void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
const struct vfio_device_ops *ops);
void vfio_uninit_group_dev(struct vfio_device *device);
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-24 14:46 ` [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev() Christoph Hellwig
@ 2021-08-24 20:25 ` Alex Williamson
2021-08-24 22:50 ` Jason Gunthorpe
2021-08-25 12:43 ` Christoph Hellwig
0 siblings, 2 replies; 58+ messages in thread
From: Alex Williamson @ 2021-08-24 20:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm, Jason Gunthorpe
On Tue, 24 Aug 2021 16:46:36 +0200
Christoph Hellwig <hch@lst.de> wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
>
> We don't need to hold a reference to the group in the driver as well as
> obtain a reference to the same group as the first thing
> vfio_register_group_dev() does.
>
> Since the drivers never use the group move this all into the core code.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 17 ++-----------
> drivers/vfio/pci/vfio_pci.c | 15 ++----------
> drivers/vfio/platform/vfio_platform_common.c | 13 +---------
> drivers/vfio/vfio.c | 25 ++++++--------------
> include/linux/vfio.h | 3 ---
> 5 files changed, 12 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index a4f44ea52fa324..b01c1c6cf1f5e6 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1875,7 +1875,6 @@ static void vfio_pci_vga_uninit(struct vfio_pci_device *vdev)
> static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> struct vfio_pci_device *vdev;
> - struct iommu_group *group;
> int ret;
>
> if (vfio_pci_is_denylisted(pdev))
> @@ -1897,15 +1896,9 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> return -EBUSY;
> }
>
> - group = vfio_iommu_group_get(&pdev->dev);
> - if (!group)
> - return -EINVAL;
> -
> vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> - if (!vdev) {
> - ret = -ENOMEM;
> - goto out_group_put;
> - }
> + if (!vdev)
> + return -ENOMEM;
>
> vfio_init_group_dev(&vdev->vdev, &pdev->dev, &vfio_pci_ops);
> vdev->pdev = pdev;
> @@ -1971,8 +1964,6 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> vfio_uninit_group_dev(&vdev->vdev);
> kfree(vdev->pm_save);
> kfree(vdev);
> -out_group_put:
> - vfio_iommu_group_put(group, &pdev->dev);
> return ret;
> }
>
> @@ -1988,8 +1979,6 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> vfio_uninit_group_dev(&vdev->vdev);
> vfio_pci_vga_uninit(vdev);
>
> - vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
> -
> if (!disable_idle_d3)
> vfio_pci_set_power_state(vdev, PCI_D0);
>
I think this turns into the patch below on top of Yishai's
vfio-pci-core series. Please verify. If you'd like something
different, please post an update. Thanks,
Alex
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index c67751948504..4134dceab3f7 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1807,7 +1807,6 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_uninit_device);
int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
{
struct pci_dev *pdev = vdev->pdev;
- struct iommu_group *group;
int ret;
if (pdev->hdr_type != PCI_HEADER_TYPE_NORMAL)
@@ -1826,10 +1825,6 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
return -EBUSY;
}
- group = vfio_iommu_group_get(&pdev->dev);
- if (!group)
- return -EINVAL;
-
if (pci_is_root_bus(pdev->bus)) {
ret = vfio_assign_device_set(&vdev->vdev, vdev);
} else if (!pci_probe_reset_slot(pdev->slot)) {
@@ -1843,10 +1838,10 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
}
if (ret)
- goto out_group_put;
+ return ret;
ret = vfio_pci_vf_init(vdev);
if (ret)
- goto out_group_put;
+ return ret;
ret = vfio_pci_vga_init(vdev);
if (ret)
goto out_vf;
@@ -1877,8 +1872,6 @@ int vfio_pci_core_register_device(struct vfio_pci_core_device *vdev)
vfio_pci_set_power_state(vdev, PCI_D0);
out_vf:
vfio_pci_vf_uninit(vdev);
-out_group_put:
- vfio_iommu_group_put(group, &pdev->dev);
return ret;
}
EXPORT_SYMBOL_GPL(vfio_pci_core_register_device);
@@ -1894,8 +1887,6 @@ void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
vfio_pci_vf_uninit(vdev);
vfio_pci_vga_uninit(vdev);
- vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
-
if (!disable_idle_d3)
vfio_pci_set_power_state(vdev, PCI_D0);
}
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-24 20:25 ` Alex Williamson
@ 2021-08-24 22:50 ` Jason Gunthorpe
2021-08-25 12:43 ` Christoph Hellwig
1 sibling, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 22:50 UTC (permalink / raw)
To: Alex Williamson
Cc: Christoph Hellwig, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 02:25:08PM -0600, Alex Williamson wrote:
> > @@ -1988,8 +1979,6 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> > vfio_uninit_group_dev(&vdev->vdev);
> > vfio_pci_vga_uninit(vdev);
> >
> > - vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
> > -
> > if (!disable_idle_d3)
> > vfio_pci_set_power_state(vdev, PCI_D0);
> >
>
>
> I think this turns into the patch below on top of Yishai's
> vfio-pci-core series. Please verify. If you'd like something
> different, please post an update. Thanks,
It appears correct to me, thanks
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-24 20:25 ` Alex Williamson
2021-08-24 22:50 ` Jason Gunthorpe
@ 2021-08-25 12:43 ` Christoph Hellwig
2021-08-25 15:48 ` Alex Williamson
1 sibling, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 12:43 UTC (permalink / raw)
To: Alex Williamson
Cc: Christoph Hellwig, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, Jason Gunthorpe, kvm, Jason Gunthorpe
On Tue, Aug 24, 2021 at 02:25:08PM -0600, Alex Williamson wrote:
> I think this turns into the patch below on top of Yishai's
> vfio-pci-core series. Please verify. If you'd like something
> different, please post an update. Thanks,
The change looks fine to me. Does that mean you want me to rebase
on top of the above series?
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-25 12:43 ` Christoph Hellwig
@ 2021-08-25 15:48 ` Alex Williamson
2021-08-25 16:08 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Alex Williamson @ 2021-08-25 15:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm, Jason Gunthorpe, Max Gurtovoy,
Yishai Hadas
On Wed, 25 Aug 2021 14:43:03 +0200
Christoph Hellwig <hch@lst.de> wrote:
> On Tue, Aug 24, 2021 at 02:25:08PM -0600, Alex Williamson wrote:
> > I think this turns into the patch below on top of Yishai's
> > vfio-pci-core series. Please verify. If you'd like something
> > different, please post an update. Thanks,
>
> The change looks fine to me. Does that mean you want me to rebase
> on top of the above series?
I wish the answer here was more obvious, but we're still waiting for
PCI and scripts/mod/ acks for that series. I note however that while
the functionality of other driver's having a userspace driver discovery
mechanism hinges on patches 9 & 10 of Yishai's series, those patches
can also be cleanly moved to the end of the series, or a follow-on
series if necessary, and current vfio-pci binding continues to work.
Should we go ahead and get final reviews for {1-8,11-13} of Yishai's
series to get them in my next branch so we have a consistent base to
work from? Thanks,
Alex
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev()
2021-08-25 15:48 ` Alex Williamson
@ 2021-08-25 16:08 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 16:08 UTC (permalink / raw)
To: Alex Williamson
Cc: Christoph Hellwig, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm, Max Gurtovoy, Yishai Hadas
On Wed, Aug 25, 2021 at 09:48:19AM -0600, Alex Williamson wrote:
> On Wed, 25 Aug 2021 14:43:03 +0200
> Christoph Hellwig <hch@lst.de> wrote:
>
> > On Tue, Aug 24, 2021 at 02:25:08PM -0600, Alex Williamson wrote:
> > > I think this turns into the patch below on top of Yishai's
> > > vfio-pci-core series. Please verify. If you'd like something
> > > different, please post an update. Thanks,
> >
> > The change looks fine to me. Does that mean you want me to rebase
> > on top of the above series?
>
> I wish the answer here was more obvious, but we're still waiting for
> PCI and scripts/mod/ acks for that series. I note however that while
> the functionality of other driver's having a userspace driver discovery
> mechanism hinges on patches 9 & 10 of Yishai's series, those patches
> can also be cleanly moved to the end of the series, or a follow-on
> series if necessary, and current vfio-pci binding continues to work.
>
> Should we go ahead and get final reviews for {1-8,11-13} of Yishai's
> series to get them in my next branch so we have a consistent base to
> work from? Thanks,
I see Bjorn has just now ack'd the PCI parts so I think it is also
fine to go ahead. A subsystem is ultimately responsible to define the
modalias format under its bus, the file2alias changes are supposed to
track it.
Also I'm looking at past changes to file2alias and I don't really see
many acks..
Thanks,
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 02/14] vfio: factor out a vfio_iommu_driver_allowed helper
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
2021-08-24 14:46 ` [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev() Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 23:24 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 03/14] vfio: remove the iommudata check in vfio_noiommu_attach_group Christoph Hellwig
` (11 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Factor out a little helper to make the checks for the noiommu driver less
ugly.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 5bd520f0dc6107..6705349ed93378 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -267,8 +267,23 @@ static const struct vfio_iommu_driver_ops vfio_noiommu_ops = {
.attach_group = vfio_noiommu_attach_group,
.detach_group = vfio_noiommu_detach_group,
};
-#endif
+/*
+ * Only noiommu containers can use vfio-noiommu and noiommu containers can only
+ * use vfio-noiommu.
+ */
+static inline bool vfio_iommu_driver_allowed(struct vfio_container *container,
+ const struct vfio_iommu_driver *driver)
+{
+ return container->noiommu == (driver->ops == &vfio_noiommu_ops);
+}
+#else
+static inline bool vfio_iommu_driver_allowed(struct vfio_container *container,
+ const struct vfio_iommu_driver *driver)
+{
+ return true;
+}
+#endif /* CONFIG_VFIO_NOIOMMU */
/**
* IOMMU driver registration
@@ -1031,13 +1046,10 @@ static long vfio_ioctl_check_extension(struct vfio_container *container,
list_for_each_entry(driver, &vfio.iommu_drivers_list,
vfio_next) {
-#ifdef CONFIG_VFIO_NOIOMMU
if (!list_empty(&container->group_list) &&
- (container->noiommu !=
- (driver->ops == &vfio_noiommu_ops)))
+ !vfio_iommu_driver_allowed(container,
+ driver))
continue;
-#endif
-
if (!try_module_get(driver->ops->owner))
continue;
@@ -1109,15 +1121,8 @@ static long vfio_ioctl_set_iommu(struct vfio_container *container,
list_for_each_entry(driver, &vfio.iommu_drivers_list, vfio_next) {
void *data;
-#ifdef CONFIG_VFIO_NOIOMMU
- /*
- * Only noiommu containers can use vfio-noiommu and noiommu
- * containers can only use vfio-noiommu.
- */
- if (container->noiommu != (driver->ops == &vfio_noiommu_ops))
+ if (!vfio_iommu_driver_allowed(container, driver))
continue;
-#endif
-
if (!try_module_get(driver->ops->owner))
continue;
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 02/14] vfio: factor out a vfio_iommu_driver_allowed helper
2021-08-24 14:46 ` [PATCH 02/14] vfio: factor out a vfio_iommu_driver_allowed helper Christoph Hellwig
@ 2021-08-24 23:24 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 23:24 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:37PM +0200, Christoph Hellwig wrote:
> Factor out a little helper to make the checks for the noiommu driver less
> ugly.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 03/14] vfio: remove the iommudata check in vfio_noiommu_attach_group
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
2021-08-24 14:46 ` [PATCH 01/14] vfio: Move vfio_iommu_group_get() to vfio_register_group_dev() Christoph Hellwig
2021-08-24 14:46 ` [PATCH 02/14] vfio: factor out a vfio_iommu_driver_allowed helper Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 23:31 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper Christoph Hellwig
` (10 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
vfio_noiommu_attach_group has two callers:
1) __vfio_container_attach_groups is called by vfio_ioctl_set_iommu,
which just called vfio_iommu_driver_allowed
2) vfio_group_set_container requires already checks ->noiommu on the
vfio_group, which is propagated from the iommudata in
vfio_create_group
so this check is entirely superflous and can be removed.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 6705349ed93378..00aeef5bb29abd 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -250,7 +250,7 @@ static long vfio_noiommu_ioctl(void *iommu_data,
static int vfio_noiommu_attach_group(void *iommu_data,
struct iommu_group *iommu_group)
{
- return iommu_group_get_iommudata(iommu_group) == &noiommu ? 0 : -EINVAL;
+ return 0;
}
static void vfio_noiommu_detach_group(void *iommu_data,
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 03/14] vfio: remove the iommudata check in vfio_noiommu_attach_group
2021-08-24 14:46 ` [PATCH 03/14] vfio: remove the iommudata check in vfio_noiommu_attach_group Christoph Hellwig
@ 2021-08-24 23:31 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 23:31 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:38PM +0200, Christoph Hellwig wrote:
> vfio_noiommu_attach_group has two callers:
>
> 1) __vfio_container_attach_groups is called by vfio_ioctl_set_iommu,
> which just called vfio_iommu_driver_allowed
> 2) vfio_group_set_container requires already checks ->noiommu on the
> vfio_group, which is propagated from the iommudata in
> vfio_create_group
>
> so this check is entirely superflous and can be removed.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (2 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 03/14] vfio: remove the iommudata check in vfio_noiommu_attach_group Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 23:40 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 05/14] vfio: refactor noiommu group creation Christoph Hellwig
` (9 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Factor out a helper to find or allocate the vfio_group to reduce the
spagetthi code in vfio_register_group_dev a little.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 49 ++++++++++++++++++++++++++-------------------
1 file changed, 28 insertions(+), 21 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 00aeef5bb29abd..9e97ad36a1c052 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -833,10 +833,32 @@ void vfio_uninit_group_dev(struct vfio_device *device)
}
EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
+struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
+{
+ struct iommu_group *iommu_group;
+ struct vfio_group *group;
+
+ iommu_group = vfio_iommu_group_get(dev);
+ if (!iommu_group)
+ return ERR_PTR(-EINVAL);
+
+ /*
+ * A found vfio_group already holds a reference to the iommu_group.
+ * A created vfio_group keeps the reference.
+ */
+ group = vfio_group_get_from_iommu(iommu_group);
+ if (!group) {
+ group = vfio_create_group(iommu_group);
+ if (!IS_ERR(group))
+ return group;
+ }
+ vfio_iommu_group_put(iommu_group, dev);
+ return group;
+}
+
int vfio_register_group_dev(struct vfio_device *device)
{
struct vfio_device *existing_device;
- struct iommu_group *iommu_group;
struct vfio_group *group;
/*
@@ -846,31 +868,16 @@ int vfio_register_group_dev(struct vfio_device *device)
if (!device->dev_set)
vfio_assign_device_set(device, device);
- iommu_group = vfio_iommu_group_get(device->dev);
- if (!iommu_group)
- return -EINVAL;
-
- group = vfio_group_get_from_iommu(iommu_group);
- if (!group) {
- group = vfio_create_group(iommu_group);
- if (IS_ERR(group)) {
- vfio_iommu_group_put(iommu_group, device->dev);
- return PTR_ERR(group);
- }
- } else {
- /*
- * A found vfio_group already holds a reference to the
- * iommu_group. A created vfio_group keeps the reference.
- */
- vfio_iommu_group_put(iommu_group, device->dev);
- }
+ group = vfio_group_find_or_alloc(device->dev);
+ if (IS_ERR(group))
+ return PTR_ERR(group);
existing_device = vfio_group_get_device(group, device->dev);
if (existing_device) {
dev_WARN(device->dev, "Device already exists on group %d\n",
- iommu_group_id(iommu_group));
+ iommu_group_id(group->iommu_group));
vfio_device_put(existing_device);
- vfio_iommu_group_put(iommu_group, device->dev);
+ vfio_iommu_group_put(group->iommu_group, device->dev);
return -EBUSY;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper
2021-08-24 14:46 ` [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper Christoph Hellwig
@ 2021-08-24 23:40 ` Jason Gunthorpe
2021-08-25 5:30 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 23:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:39PM +0200, Christoph Hellwig wrote:
> Factor out a helper to find or allocate the vfio_group to reduce the
> spagetthi code in vfio_register_group_dev a little.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> drivers/vfio/vfio.c | 49 ++++++++++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 00aeef5bb29abd..9e97ad36a1c052 100644
> +++ b/drivers/vfio/vfio.c
> @@ -833,10 +833,32 @@ void vfio_uninit_group_dev(struct vfio_device *device)
> }
> EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
>
> +struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
> +{
> + struct iommu_group *iommu_group;
> + struct vfio_group *group;
> +
> + iommu_group = vfio_iommu_group_get(dev);
> + if (!iommu_group)
> + return ERR_PTR(-EINVAL);
> +
> + /*
> + * A found vfio_group already holds a reference to the iommu_group.
> + * A created vfio_group keeps the reference.
> + */
> + group = vfio_group_get_from_iommu(iommu_group);
> + if (!group) {
> + group = vfio_create_group(iommu_group);
> + if (!IS_ERR(group))
> + return group;
> + }
> + vfio_iommu_group_put(iommu_group, dev);
> + return group;
I think the non-"success oriented flow" is less readable than what was
before. It is jarring to see, and I was about to say this is not
logically the same having missed the !...
How about:
/* If found group already holds the iommu_group reference */
group = vfio_group_get_from_iommu(iommu_group);
if (group)
goto out_put;
group = vfio_create_group(iommu_group);
if (IS_ERR(group))
goto out_put;
/* If created our iommu_group reference was moved to group, keep it */
return group;
out_put:
vfio_iommu_group_put(iommu_group, dev);
return group;
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper
2021-08-24 23:40 ` Jason Gunthorpe
@ 2021-08-25 5:30 ` Christoph Hellwig
0 siblings, 0 replies; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 5:30 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Tue, Aug 24, 2021 at 08:40:50PM -0300, Jason Gunthorpe wrote:
> I think the non-"success oriented flow" is less readable than what was
> before. It is jarring to see, and I was about to say this is not
> logically the same having missed the !...
>
> How about:
>
> /* If found group already holds the iommu_group reference */
> group = vfio_group_get_from_iommu(iommu_group);
> if (group)
> goto out_put;
>
> group = vfio_create_group(iommu_group);
> if (IS_ERR(group))
> goto out_put;
> /* If created our iommu_group reference was moved to group, keep it */
> return group;
>
> out_put:
> vfio_iommu_group_put(iommu_group, dev);
> return group;
Fine with me. I actually repeatedly switched between the two flows
because I couldn't decide on one.
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 05/14] vfio: refactor noiommu group creation
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (3 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 04/14] vfio: factor out a vfio_group_find_or_alloc helper Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 23:43 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 06/14] vfio: remove the iommudata hack for noiommu groups Christoph Hellwig
` (8 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Split the actual noiommu group creation from vfio_iommu_group_get into a
new helper, and open code the rest of vfio_iommu_group_get in its only
caller. This creates an antirely separate and clear code path for the
noiommu group creation.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 100 +++++++++++++++++++++++---------------------
1 file changed, 53 insertions(+), 47 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 9e97ad36a1c052..53e0d52f3b0209 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -169,50 +169,6 @@ static void vfio_release_device_set(struct vfio_device *device)
xa_unlock(&vfio_device_set_xa);
}
-static struct iommu_group *vfio_iommu_group_get(struct device *dev)
-{
- struct iommu_group *group;
- int __maybe_unused ret;
-
- group = iommu_group_get(dev);
-
-#ifdef CONFIG_VFIO_NOIOMMU
- /*
- * With noiommu enabled, an IOMMU group will be created for a device
- * that doesn't already have one and doesn't have an iommu_ops on their
- * bus. We set iommudata simply to be able to identify these groups
- * as special use and for reclamation later.
- */
- if (group || !noiommu || iommu_present(dev->bus))
- return group;
-
- group = iommu_group_alloc();
- if (IS_ERR(group))
- return NULL;
-
- iommu_group_set_name(group, "vfio-noiommu");
- iommu_group_set_iommudata(group, &noiommu, NULL);
- ret = iommu_group_add_device(group, dev);
- if (ret) {
- iommu_group_put(group);
- return NULL;
- }
-
- /*
- * Where to taint? At this point we've added an IOMMU group for a
- * device that is not backed by iommu_ops, therefore any iommu_
- * callback using iommu_ops can legitimately Oops. So, while we may
- * be about to give a DMA capable device to a user without IOMMU
- * protection, which is clearly taint-worthy, let's go ahead and do
- * it here.
- */
- add_taint(TAINT_USER, LOCKDEP_STILL_OK);
- dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n");
-#endif
-
- return group;
-}
-
static void vfio_iommu_group_put(struct iommu_group *group, struct device *dev)
{
#ifdef CONFIG_VFIO_NOIOMMU
@@ -833,12 +789,61 @@ void vfio_uninit_group_dev(struct vfio_device *device)
}
EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
-struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
+#ifdef CONFIG_VFIO_NOIOMMU
+static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
{
struct iommu_group *iommu_group;
struct vfio_group *group;
+ int ret;
+
+ iommu_group = iommu_group_alloc();
+ if (IS_ERR(iommu_group))
+ return ERR_CAST(iommu_group);
- iommu_group = vfio_iommu_group_get(dev);
+ iommu_group_set_name(iommu_group, "vfio-noiommu");
+ iommu_group_set_iommudata(iommu_group, &noiommu, NULL);
+ ret = iommu_group_add_device(iommu_group, dev);
+ if (ret)
+ goto out_put_group;
+
+ group = vfio_create_group(iommu_group);
+ if (IS_ERR(group)) {
+ ret = PTR_ERR(group);
+ goto out_remove_device;
+ }
+
+ return group;
+
+out_remove_device:
+ iommu_group_remove_device(dev);
+out_put_group:
+ iommu_group_put(iommu_group);
+ return ERR_PTR(ret);
+}
+#endif
+
+static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
+{
+ struct iommu_group *iommu_group;
+ struct vfio_group *group;
+
+ iommu_group = iommu_group_get(dev);
+#ifdef CONFIG_VFIO_NOIOMMU
+ if (!iommu_group && noiommu && !iommu_present(dev->bus)) {
+ /*
+ * With noiommu enabled, create an IOMMU group for devices that
+ * don't already have one and don't have an iommu_ops on their
+ * bus. Taint the kernel because we're about to give a DMA
+ * capable device to a user without IOMMU protection.
+ */
+ group = vfio_noiommu_group_alloc(dev);
+ if (group) {
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+ dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n");
+ }
+ return group;
+ }
+#endif
if (!iommu_group)
return ERR_PTR(-EINVAL);
@@ -852,7 +857,8 @@ struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
if (!IS_ERR(group))
return group;
}
- vfio_iommu_group_put(iommu_group, dev);
+
+ iommu_group_put(iommu_group);
return group;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 05/14] vfio: refactor noiommu group creation
2021-08-24 14:46 ` [PATCH 05/14] vfio: refactor noiommu group creation Christoph Hellwig
@ 2021-08-24 23:43 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 23:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:40PM +0200, Christoph Hellwig wrote:
> Split the actual noiommu group creation from vfio_iommu_group_get into a
> new helper, and open code the rest of vfio_iommu_group_get in its only
> caller. This creates an antirely separate and clear code path for the
> noiommu group creation.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 100 +++++++++++++++++++++++---------------------
> 1 file changed, 53 insertions(+), 47 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 06/14] vfio: remove the iommudata hack for noiommu groups
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (4 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 05/14] vfio: refactor noiommu group creation Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-24 23:45 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices Christoph Hellwig
` (7 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Just pass a noiommu argument to vfio_create_group and set up the
->noiommu flag directly, and remove the now superflous
vfio_iommu_group_put helper.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 53e0d52f3b0209..f44532fed5bb4f 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -169,16 +169,6 @@ static void vfio_release_device_set(struct vfio_device *device)
xa_unlock(&vfio_device_set_xa);
}
-static void vfio_iommu_group_put(struct iommu_group *group, struct device *dev)
-{
-#ifdef CONFIG_VFIO_NOIOMMU
- if (iommu_group_get_iommudata(group) == &noiommu)
- iommu_group_remove_device(dev);
-#endif
-
- iommu_group_put(group);
-}
-
#ifdef CONFIG_VFIO_NOIOMMU
static void *vfio_noiommu_open(unsigned long arg)
{
@@ -345,7 +335,8 @@ static void vfio_group_unlock_and_free(struct vfio_group *group)
/**
* Group objects - create, release, get, put, search
*/
-static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
+static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
+ bool noiommu)
{
struct vfio_group *group, *tmp;
struct device *dev;
@@ -364,9 +355,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
atomic_set(&group->opened, 0);
init_waitqueue_head(&group->container_q);
group->iommu_group = iommu_group;
-#ifdef CONFIG_VFIO_NOIOMMU
- group->noiommu = (iommu_group_get_iommudata(iommu_group) == &noiommu);
-#endif
+ group->noiommu = noiommu;
BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
group->nb.notifier_call = vfio_iommu_group_notifier;
@@ -801,12 +790,11 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
return ERR_CAST(iommu_group);
iommu_group_set_name(iommu_group, "vfio-noiommu");
- iommu_group_set_iommudata(iommu_group, &noiommu, NULL);
ret = iommu_group_add_device(iommu_group, dev);
if (ret)
goto out_put_group;
- group = vfio_create_group(iommu_group);
+ group = vfio_create_group(iommu_group, true);
if (IS_ERR(group)) {
ret = PTR_ERR(group);
goto out_remove_device;
@@ -853,7 +841,7 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
*/
group = vfio_group_get_from_iommu(iommu_group);
if (!group) {
- group = vfio_create_group(iommu_group);
+ group = vfio_create_group(iommu_group, false);
if (!IS_ERR(group))
return group;
}
@@ -883,7 +871,9 @@ int vfio_register_group_dev(struct vfio_device *device)
dev_WARN(device->dev, "Device already exists on group %d\n",
iommu_group_id(group->iommu_group));
vfio_device_put(existing_device);
- vfio_iommu_group_put(group->iommu_group, device->dev);
+ if (group->noiommu)
+ iommu_group_remove_device(device->dev);
+ iommu_group_put(group->iommu_group);
return -EBUSY;
}
@@ -1028,7 +1018,9 @@ void vfio_unregister_group_dev(struct vfio_device *device)
if (list_empty(&group->device_list))
wait_event(group->container_q, !group->container);
- vfio_iommu_group_put(group->iommu_group, device->dev);
+ if (group->noiommu)
+ iommu_group_remove_device(device->dev);
+ iommu_group_put(group->iommu_group);
}
EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 06/14] vfio: remove the iommudata hack for noiommu groups
2021-08-24 14:46 ` [PATCH 06/14] vfio: remove the iommudata hack for noiommu groups Christoph Hellwig
@ 2021-08-24 23:45 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-24 23:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:41PM +0200, Christoph Hellwig wrote:
> Just pass a noiommu argument to vfio_create_group and set up the
> ->noiommu flag directly, and remove the now superflous
> vfio_iommu_group_put helper.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 30 +++++++++++-------------------
> 1 file changed, 11 insertions(+), 19 deletions(-)
I like this a lot better
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (5 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 06/14] vfio: remove the iommudata hack for noiommu groups Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:19 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 08/14] vfio: remove unused method from vfio_iommu_driver_ops Christoph Hellwig
` (6 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Reuse the logic in vfio_noiommu_group_alloc to allocate a single-device
iommu group for mediated device. For this a new
vfio_register_mediated_dev is added that calls vfio_noiommu_group_alloc.
The noiommu boolean in struct vfio_group is replaced with a set of flags
to that mediated devices can be distinguished from noiommu devices.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/mdev/mdev_driver.c | 46 ++---------------------
drivers/vfio/mdev/vfio_mdev.c | 2 +-
drivers/vfio/vfio.c | 66 ++++++++++++++++++++-------------
include/linux/vfio.h | 1 +
samples/vfio-mdev/mbochs.c | 2 +-
samples/vfio-mdev/mdpy.c | 2 +-
samples/vfio-mdev/mtty.c | 2 +-
7 files changed, 49 insertions(+), 72 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
index c368ec824e2b5c..14b9ab17426838 100644
--- a/drivers/vfio/mdev/mdev_driver.c
+++ b/drivers/vfio/mdev/mdev_driver.c
@@ -13,61 +13,23 @@
#include "mdev_private.h"
-static int mdev_attach_iommu(struct mdev_device *mdev)
-{
- int ret;
- struct iommu_group *group;
-
- group = iommu_group_alloc();
- if (IS_ERR(group))
- return PTR_ERR(group);
-
- ret = iommu_group_add_device(group, &mdev->dev);
- if (!ret)
- dev_info(&mdev->dev, "MDEV: group_id = %d\n",
- iommu_group_id(group));
-
- iommu_group_put(group);
- return ret;
-}
-
-static void mdev_detach_iommu(struct mdev_device *mdev)
-{
- iommu_group_remove_device(&mdev->dev);
- dev_info(&mdev->dev, "MDEV: detaching iommu\n");
-}
-
static int mdev_probe(struct device *dev)
{
struct mdev_driver *drv =
container_of(dev->driver, struct mdev_driver, driver);
- struct mdev_device *mdev = to_mdev_device(dev);
- int ret;
-
- ret = mdev_attach_iommu(mdev);
- if (ret)
- return ret;
- if (drv->probe) {
- ret = drv->probe(mdev);
- if (ret)
- mdev_detach_iommu(mdev);
- }
-
- return ret;
+ if (!drv->probe)
+ return 0;
+ return drv->probe(to_mdev_device(dev));
}
static int mdev_remove(struct device *dev)
{
struct mdev_driver *drv =
container_of(dev->driver, struct mdev_driver, driver);
- struct mdev_device *mdev = to_mdev_device(dev);
if (drv->remove)
- drv->remove(mdev);
-
- mdev_detach_iommu(mdev);
-
+ drv->remove(to_mdev_device(dev));
return 0;
}
diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
index 7a9883048216e7..7bab075173145b 100644
--- a/drivers/vfio/mdev/vfio_mdev.c
+++ b/drivers/vfio/mdev/vfio_mdev.c
@@ -119,7 +119,7 @@ static int vfio_mdev_probe(struct mdev_device *mdev)
return -ENOMEM;
vfio_init_group_dev(vdev, &mdev->dev, &vfio_mdev_dev_ops);
- ret = vfio_register_group_dev(vdev);
+ ret = vfio_register_mediated_dev(vdev);
if (ret)
goto out_uninit;
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index f44532fed5bb4f..45987dc897179e 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -67,6 +67,9 @@ struct vfio_unbound_dev {
struct list_head unbound_next;
};
+#define VFIO_MEDIATED (1 << 0)
+#define VFIO_NOIOMMU (1 << 1)
+
struct vfio_group {
struct kref kref;
int minor;
@@ -83,7 +86,7 @@ struct vfio_group {
struct mutex unbound_lock;
atomic_t opened;
wait_queue_head_t container_q;
- bool noiommu;
+ unsigned int flags;
unsigned int dev_counter;
struct kvm *kvm;
struct blocking_notifier_head notifier;
@@ -336,7 +339,7 @@ static void vfio_group_unlock_and_free(struct vfio_group *group)
* Group objects - create, release, get, put, search
*/
static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
- bool noiommu)
+ unsigned int flags)
{
struct vfio_group *group, *tmp;
struct device *dev;
@@ -355,7 +358,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
atomic_set(&group->opened, 0);
init_waitqueue_head(&group->container_q);
group->iommu_group = iommu_group;
- group->noiommu = noiommu;
+ group->flags = flags;
BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
group->nb.notifier_call = vfio_iommu_group_notifier;
@@ -391,8 +394,8 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group,
}
dev = device_create(vfio.class, NULL,
- MKDEV(MAJOR(vfio.group_devt), minor),
- group, "%s%d", group->noiommu ? "noiommu-" : "",
+ MKDEV(MAJOR(vfio.group_devt), minor), group, "%s%d",
+ (group->flags & VFIO_NOIOMMU) ? "noiommu-" : "",
iommu_group_id(iommu_group));
if (IS_ERR(dev)) {
vfio_free_group_minor(minor);
@@ -778,8 +781,8 @@ void vfio_uninit_group_dev(struct vfio_device *device)
}
EXPORT_SYMBOL_GPL(vfio_uninit_group_dev);
-#ifdef CONFIG_VFIO_NOIOMMU
-static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
+static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
+ unsigned int flags)
{
struct iommu_group *iommu_group;
struct vfio_group *group;
@@ -794,7 +797,7 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
if (ret)
goto out_put_group;
- group = vfio_create_group(iommu_group, true);
+ group = vfio_create_group(iommu_group, flags);
if (IS_ERR(group)) {
ret = PTR_ERR(group);
goto out_remove_device;
@@ -808,7 +811,6 @@ static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev)
iommu_group_put(iommu_group);
return ERR_PTR(ret);
}
-#endif
static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
{
@@ -824,7 +826,7 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
* bus. Taint the kernel because we're about to give a DMA
* capable device to a user without IOMMU protection.
*/
- group = vfio_noiommu_group_alloc(dev);
+ group = vfio_noiommu_group_alloc(dev, VFIO_NOIOMMU);
if (group) {
add_taint(TAINT_USER, LOCKDEP_STILL_OK);
dev_warn(dev, "Adding kernel taint for vfio-noiommu group on device\n");
@@ -841,7 +843,7 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
*/
group = vfio_group_get_from_iommu(iommu_group);
if (!group) {
- group = vfio_create_group(iommu_group, false);
+ group = vfio_create_group(iommu_group, 0);
if (!IS_ERR(group))
return group;
}
@@ -850,10 +852,13 @@ static struct vfio_group *vfio_group_find_or_alloc(struct device *dev)
return group;
}
-int vfio_register_group_dev(struct vfio_device *device)
+static int __vfio_register_dev(struct vfio_device *device,
+ struct vfio_group *group)
{
struct vfio_device *existing_device;
- struct vfio_group *group;
+
+ if (IS_ERR(group))
+ return PTR_ERR(group);
/*
* If the driver doesn't specify a set then the device is added to a
@@ -862,16 +867,12 @@ int vfio_register_group_dev(struct vfio_device *device)
if (!device->dev_set)
vfio_assign_device_set(device, device);
- group = vfio_group_find_or_alloc(device->dev);
- if (IS_ERR(group))
- return PTR_ERR(group);
-
existing_device = vfio_group_get_device(group, device->dev);
if (existing_device) {
dev_WARN(device->dev, "Device already exists on group %d\n",
iommu_group_id(group->iommu_group));
vfio_device_put(existing_device);
- if (group->noiommu)
+ if (group->flags & (VFIO_NOIOMMU | VFIO_MEDIATED))
iommu_group_remove_device(device->dev);
iommu_group_put(group->iommu_group);
return -EBUSY;
@@ -890,8 +891,21 @@ int vfio_register_group_dev(struct vfio_device *device)
return 0;
}
+
+int vfio_register_group_dev(struct vfio_device *device)
+{
+ return __vfio_register_dev(device,
+ vfio_group_find_or_alloc(device->dev));
+}
EXPORT_SYMBOL_GPL(vfio_register_group_dev);
+int vfio_register_mediated_dev(struct vfio_device *device)
+{
+ return __vfio_register_dev(device,
+ vfio_noiommu_group_alloc(device->dev, VFIO_MEDIATED));
+}
+EXPORT_SYMBOL_GPL(vfio_register_mediated_dev);
+
/**
* Get a reference to the vfio_device for a device. Even if the
* caller thinks they own the device, they could be racing with a
@@ -1018,7 +1032,7 @@ void vfio_unregister_group_dev(struct vfio_device *device)
if (list_empty(&group->device_list))
wait_event(group->container_q, !group->container);
- if (group->noiommu)
+ if (group->flags & (VFIO_NOIOMMU | VFIO_MEDIATED))
iommu_group_remove_device(device->dev);
iommu_group_put(group->iommu_group);
}
@@ -1365,7 +1379,7 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
if (atomic_read(&group->container_users))
return -EINVAL;
- if (group->noiommu && !capable(CAP_SYS_RAWIO))
+ if ((group->flags & VFIO_NOIOMMU) && !capable(CAP_SYS_RAWIO))
return -EPERM;
f = fdget(container_fd);
@@ -1385,7 +1399,7 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
/* Real groups and fake groups cannot mix */
if (!list_empty(&container->group_list) &&
- container->noiommu != group->noiommu) {
+ container->noiommu != (group->flags & VFIO_NOIOMMU)) {
ret = -EPERM;
goto unlock_out;
}
@@ -1399,7 +1413,7 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
}
group->container = container;
- container->noiommu = group->noiommu;
+ container->noiommu = (group->flags & VFIO_NOIOMMU);
list_add(&group->container_next, &container->group_list);
/* Get a reference on the container and mark a user within the group */
@@ -1423,7 +1437,7 @@ static int vfio_group_add_container_user(struct vfio_group *group)
if (!atomic_inc_not_zero(&group->container_users))
return -EINVAL;
- if (group->noiommu) {
+ if (group->flags & VFIO_NOIOMMU) {
atomic_dec(&group->container_users);
return -EPERM;
}
@@ -1448,7 +1462,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
!group->container->iommu_driver || !vfio_group_viable(group))
return -EINVAL;
- if (group->noiommu && !capable(CAP_SYS_RAWIO))
+ if ((group->flags & VFIO_NOIOMMU) && !capable(CAP_SYS_RAWIO))
return -EPERM;
device = vfio_device_get_from_name(group, buf);
@@ -1495,7 +1509,7 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
fd_install(fdno, filep);
- if (group->noiommu)
+ if (group->flags & VFIO_NOIOMMU)
dev_warn(device->dev, "vfio-noiommu device opened by user "
"(%s:%d)\n", current->comm, task_pid_nr(current));
return fdno;
@@ -1591,7 +1605,7 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
if (!group)
return -ENODEV;
- if (group->noiommu && !capable(CAP_SYS_RAWIO)) {
+ if ((group->flags & VFIO_NOIOMMU) && !capable(CAP_SYS_RAWIO)) {
vfio_group_put(group);
return -EPERM;
}
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index f7083c2fd0d099..2ffbb2d02c9346 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -75,6 +75,7 @@ void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
const struct vfio_device_ops *ops);
void vfio_uninit_group_dev(struct vfio_device *device);
int vfio_register_group_dev(struct vfio_device *device);
+int vfio_register_mediated_dev(struct vfio_device *device);
void vfio_unregister_group_dev(struct vfio_device *device);
extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
extern void vfio_device_put(struct vfio_device *device);
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index c313ab4d1f4e4e..f59218e03dd1b4 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -553,7 +553,7 @@ static int mbochs_probe(struct mdev_device *mdev)
mbochs_create_config_space(mdev_state);
mbochs_reset(mdev_state);
- ret = vfio_register_group_dev(&mdev_state->vdev);
+ ret = vfio_register_mediated_dev(&mdev_state->vdev);
if (ret)
goto err_mem;
dev_set_drvdata(&mdev->dev, mdev_state);
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index 8d1a80a0722aa9..4df380014ad914 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -258,7 +258,7 @@ static int mdpy_probe(struct mdev_device *mdev)
mdpy_count++;
- ret = vfio_register_group_dev(&mdev_state->vdev);
+ ret = vfio_register_mediated_dev(&mdev_state->vdev);
if (ret)
goto err_mem;
dev_set_drvdata(&mdev->dev, mdev_state);
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 5983cdb16e3d1d..ef9f48bfe8d04f 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -741,7 +741,7 @@ static int mtty_probe(struct mdev_device *mdev)
mtty_create_config_space(mdev_state);
- ret = vfio_register_group_dev(&mdev_state->vdev);
+ ret = vfio_register_mediated_dev(&mdev_state->vdev);
if (ret)
goto err_vconfig;
dev_set_drvdata(&mdev->dev, mdev_state);
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-24 14:46 ` [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices Christoph Hellwig
@ 2021-08-25 0:19 ` Jason Gunthorpe
2021-08-25 5:32 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:42PM +0200, Christoph Hellwig wrote:
> +int vfio_register_group_dev(struct vfio_device *device)
> +{
> + return __vfio_register_dev(device,
> + vfio_group_find_or_alloc(device->dev));
> +}
> EXPORT_SYMBOL_GPL(vfio_register_group_dev);
>
> +int vfio_register_mediated_dev(struct vfio_device *device)
> +{
> + return __vfio_register_dev(device,
> + vfio_noiommu_group_alloc(device->dev, VFIO_MEDIATED));
> +}
> +EXPORT_SYMBOL_GPL(vfio_register_mediated_dev);
The mechanism looks fine, but I think the core code is much clearer if
the name is not 'mediated' but 'sw_iommu' or something that implies
the group is running with a software page table. mediated has become
so overloaded in this code.
Really what this flag is doing is putting the group into a state where
the vfio_*pin_pages APIs are available once the vfio_device is opened.
So it really becomes an API family where a driver will call
vfio_regsiter_sw_iommu_dev(), then use the vfio_pin/unpin_pages API
set.
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 0:19 ` Jason Gunthorpe
@ 2021-08-25 5:32 ` Christoph Hellwig
2021-08-25 12:21 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 5:32 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Tue, Aug 24, 2021 at 09:19:16PM -0300, Jason Gunthorpe wrote:
> The mechanism looks fine, but I think the core code is much clearer if
> the name is not 'mediated' but 'sw_iommu' or something that implies
> the group is running with a software page table. mediated has become
> so overloaded in this code.
I thought that was sort of the definition of mediated - there needs to
ben entify that "mediates" access so that a user of this interface
can't trigger undmediated DMA to arbitrary addresses. My other choice
that I used for a while was "virtual". sw_iommu sounds a little clumsy.
I guess either way we need some documentation describing it along the
above lines.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 5:32 ` Christoph Hellwig
@ 2021-08-25 12:21 ` Jason Gunthorpe
2021-08-25 12:24 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 12:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Wed, Aug 25, 2021 at 07:32:37AM +0200, Christoph Hellwig wrote:
> On Tue, Aug 24, 2021 at 09:19:16PM -0300, Jason Gunthorpe wrote:
> > The mechanism looks fine, but I think the core code is much clearer if
> > the name is not 'mediated' but 'sw_iommu' or something that implies
> > the group is running with a software page table. mediated has become
> > so overloaded in this code.
>
> I thought that was sort of the definition of mediated - there needs to
> ben entify that "mediates" access so that a user of this interface
> can't trigger undmediated DMA to arbitrary addresses. My other choice
> that I used for a while was "virtual". sw_iommu sounds a little clumsy.
vfio mediated is really some toolbox of different features that a
driver can use to build what it wants.
For instance Intel is looking at this concept of a mediated device
that uses PASID for the IOMMU. It would have a real IOMMU, use real
IOMMU page tables and in this language it would create some PASID
group, not a "sw_iommu" group.
This feature is about creating a device that is not connected to a HW
IO page table (at least by the VFIO iommu code) but the IO page table
is held in software and accessed by the VFIO driver through the pin
API.
virtual_iommu is somewhat overloaded with the idea of a vIOMMU created
by qemu and stuffed into a guest..
"domainless" might work but I also find it confusing that the iommu
code uses the word domain to refer to a HW IO page table :\
Maybe "sw io page table" ?
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:21 ` Jason Gunthorpe
@ 2021-08-25 12:24 ` Christoph Hellwig
2021-08-25 12:34 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 12:24 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Wed, Aug 25, 2021 at 09:21:44AM -0300, Jason Gunthorpe wrote:
> This feature is about creating a device that is not connected to a HW
> IO page table (at least by the VFIO iommu code) but the IO page table
> is held in software and accessed by the VFIO driver through the pin
> API.
>
> virtual_iommu is somewhat overloaded with the idea of a vIOMMU created
> by qemu and stuffed into a guest..
>
> "domainless" might work but I also find it confusing that the iommu
> code uses the word domain to refer to a HW IO page table :\
>
> Maybe "sw io page table" ?
Or simply emulated? At least looking at i915 there is very little
direct connection to the actual hardware, and while I don't understand
them fully the s390 driver look similar. And the samples are completely
faked up anyway.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:24 ` Christoph Hellwig
@ 2021-08-25 12:34 ` Jason Gunthorpe
2021-08-25 12:37 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 12:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Wed, Aug 25, 2021 at 02:24:00PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 25, 2021 at 09:21:44AM -0300, Jason Gunthorpe wrote:
> > This feature is about creating a device that is not connected to a HW
> > IO page table (at least by the VFIO iommu code) but the IO page table
> > is held in software and accessed by the VFIO driver through the pin
> > API.
> >
> > virtual_iommu is somewhat overloaded with the idea of a vIOMMU created
> > by qemu and stuffed into a guest..
> >
> > "domainless" might work but I also find it confusing that the iommu
> > code uses the word domain to refer to a HW IO page table :\
> >
> > Maybe "sw io page table" ?
>
> Or simply emulated? At least looking at i915 there is very little
> direct connection to the actual hardware, and while I don't understand
> them fully the s390 driver look similar. And the samples are completely
> faked up anyway.
Emulated IO page table works for me!
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:34 ` Jason Gunthorpe
@ 2021-08-25 12:37 ` Christoph Hellwig
2021-08-25 12:45 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 12:37 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Wed, Aug 25, 2021 at 09:34:54AM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 25, 2021 at 02:24:00PM +0200, Christoph Hellwig wrote:
> > On Wed, Aug 25, 2021 at 09:21:44AM -0300, Jason Gunthorpe wrote:
> > > This feature is about creating a device that is not connected to a HW
> > > IO page table (at least by the VFIO iommu code) but the IO page table
> > > is held in software and accessed by the VFIO driver through the pin
> > > API.
> > >
> > > virtual_iommu is somewhat overloaded with the idea of a vIOMMU created
> > > by qemu and stuffed into a guest..
> > >
> > > "domainless" might work but I also find it confusing that the iommu
> > > code uses the word domain to refer to a HW IO page table :\
> > >
> > > Maybe "sw io page table" ?
> >
> > Or simply emulated? At least looking at i915 there is very little
> > direct connection to the actual hardware, and while I don't understand
> > them fully the s390 driver look similar. And the samples are completely
> > faked up anyway.
>
> Emulated IO page table works for me!
Hmm, that is a full sentence for the comment that needs to be added.
Are you fine with just s/mediated/emulated/ for the symbol names or
do you want something more specific?
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:37 ` Christoph Hellwig
@ 2021-08-25 12:45 ` Jason Gunthorpe
2021-08-25 12:50 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 12:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Wed, Aug 25, 2021 at 02:37:42PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 25, 2021 at 09:34:54AM -0300, Jason Gunthorpe wrote:
> > On Wed, Aug 25, 2021 at 02:24:00PM +0200, Christoph Hellwig wrote:
> > > On Wed, Aug 25, 2021 at 09:21:44AM -0300, Jason Gunthorpe wrote:
> > > > This feature is about creating a device that is not connected to a HW
> > > > IO page table (at least by the VFIO iommu code) but the IO page table
> > > > is held in software and accessed by the VFIO driver through the pin
> > > > API.
> > > >
> > > > virtual_iommu is somewhat overloaded with the idea of a vIOMMU created
> > > > by qemu and stuffed into a guest..
> > > >
> > > > "domainless" might work but I also find it confusing that the iommu
> > > > code uses the word domain to refer to a HW IO page table :\
> > > >
> > > > Maybe "sw io page table" ?
> > >
> > > Or simply emulated? At least looking at i915 there is very little
> > > direct connection to the actual hardware, and while I don't understand
> > > them fully the s390 driver look similar. And the samples are completely
> > > faked up anyway.
> >
> > Emulated IO page table works for me!
>
> Hmm, that is a full sentence for the comment that needs to be added.
> Are you fine with just s/mediated/emulated/ for the symbol names or
> do you want something more specific?
I think so, in context of a group or iommu function it kind of makes
sense emulated would refer to the page table/dma process
The driver entry point can have the extra words
vfio_register_emulated_dma_dev()
Seems to read well?
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:45 ` Jason Gunthorpe
@ 2021-08-25 12:50 ` Christoph Hellwig
2021-08-25 12:50 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 12:50 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Wed, Aug 25, 2021 at 09:45:38AM -0300, Jason Gunthorpe wrote:
> I think so, in context of a group or iommu function it kind of makes
> sense emulated would refer to the page table/dma process
>
> The driver entry point can have the extra words
> vfio_register_emulated_dma_dev()
Not my preference, but I could live with it. Or maybe emulated_iommu?
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices
2021-08-25 12:50 ` Christoph Hellwig
@ 2021-08-25 12:50 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 12:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Wed, Aug 25, 2021 at 02:50:22PM +0200, Christoph Hellwig wrote:
> On Wed, Aug 25, 2021 at 09:45:38AM -0300, Jason Gunthorpe wrote:
> > I think so, in context of a group or iommu function it kind of makes
> > sense emulated would refer to the page table/dma process
> >
> > The driver entry point can have the extra words
> > vfio_register_emulated_dma_dev()
>
> Not my preference, but I could live with it. Or maybe emulated_iommu?
Sure
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 08/14] vfio: remove unused method from vfio_iommu_driver_ops
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (6 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 07/14] vfio: simplify iommu group allocation for mediated devices Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:21 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 09/14] vfio: move the vfio_iommu_driver_ops interface out of <linux/vfio.h> Christoph Hellwig
` (5 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
The read, write and mmap methods are never implemented, so remove them.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 50 --------------------------------------------
include/linux/vfio.h | 5 -----
2 files changed, 55 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 45987dc897179e..34621e0044e8b4 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1247,62 +1247,12 @@ static int vfio_fops_release(struct inode *inode, struct file *filep)
return 0;
}
-/*
- * Once an iommu driver is set, we optionally pass read/write/mmap
- * on to the driver, allowing management interfaces beyond ioctl.
- */
-static ssize_t vfio_fops_read(struct file *filep, char __user *buf,
- size_t count, loff_t *ppos)
-{
- struct vfio_container *container = filep->private_data;
- struct vfio_iommu_driver *driver;
- ssize_t ret = -EINVAL;
-
- driver = container->iommu_driver;
- if (likely(driver && driver->ops->read))
- ret = driver->ops->read(container->iommu_data,
- buf, count, ppos);
-
- return ret;
-}
-
-static ssize_t vfio_fops_write(struct file *filep, const char __user *buf,
- size_t count, loff_t *ppos)
-{
- struct vfio_container *container = filep->private_data;
- struct vfio_iommu_driver *driver;
- ssize_t ret = -EINVAL;
-
- driver = container->iommu_driver;
- if (likely(driver && driver->ops->write))
- ret = driver->ops->write(container->iommu_data,
- buf, count, ppos);
-
- return ret;
-}
-
-static int vfio_fops_mmap(struct file *filep, struct vm_area_struct *vma)
-{
- struct vfio_container *container = filep->private_data;
- struct vfio_iommu_driver *driver;
- int ret = -EINVAL;
-
- driver = container->iommu_driver;
- if (likely(driver && driver->ops->mmap))
- ret = driver->ops->mmap(container->iommu_data, vma);
-
- return ret;
-}
-
static const struct file_operations vfio_fops = {
.owner = THIS_MODULE,
.open = vfio_fops_open,
.release = vfio_fops_release,
- .read = vfio_fops_read,
- .write = vfio_fops_write,
.unlocked_ioctl = vfio_fops_unl_ioctl,
.compat_ioctl = compat_ptr_ioctl,
- .mmap = vfio_fops_mmap,
};
/**
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 2ffbb2d02c9346..82042690d7978d 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -95,13 +95,8 @@ struct vfio_iommu_driver_ops {
struct module *owner;
void *(*open)(unsigned long arg);
void (*release)(void *iommu_data);
- ssize_t (*read)(void *iommu_data, char __user *buf,
- size_t count, loff_t *ppos);
- ssize_t (*write)(void *iommu_data, const char __user *buf,
- size_t count, loff_t *size);
long (*ioctl)(void *iommu_data, unsigned int cmd,
unsigned long arg);
- int (*mmap)(void *iommu_data, struct vm_area_struct *vma);
int (*attach_group)(void *iommu_data,
struct iommu_group *group);
void (*detach_group)(void *iommu_data,
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 08/14] vfio: remove unused method from vfio_iommu_driver_ops
2021-08-24 14:46 ` [PATCH 08/14] vfio: remove unused method from vfio_iommu_driver_ops Christoph Hellwig
@ 2021-08-25 0:21 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:43PM +0200, Christoph Hellwig wrote:
> The read, write and mmap methods are never implemented, so remove them.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 50 --------------------------------------------
> include/linux/vfio.h | 5 -----
> 2 files changed, 55 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 09/14] vfio: move the vfio_iommu_driver_ops interface out of <linux/vfio.h>
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (7 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 08/14] vfio: remove unused method from vfio_iommu_driver_ops Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:25 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 10/14] vfio: remove the unused mdev iommu hook Christoph Hellwig
` (4 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Create a new private drivers/vfio/vfio.h header for the interface between
the VFIO core and the iommu drivers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 1 +
drivers/vfio/vfio.h | 47 +++++++++++++++++++++++++++++
drivers/vfio/vfio_iommu_spapr_tce.c | 1 +
drivers/vfio/vfio_iommu_type1.c | 1 +
include/linux/vfio.h | 44 ---------------------------
5 files changed, 50 insertions(+), 44 deletions(-)
create mode 100644 drivers/vfio/vfio.h
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 34621e0044e8b4..26663dfa19e70d 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -32,6 +32,7 @@
#include <linux/vfio.h>
#include <linux/wait.h>
#include <linux/sched/signal.h>
+#include "vfio.h"
#define DRIVER_VERSION "0.3"
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
new file mode 100644
index 00000000000000..a78de649eb2f16
--- /dev/null
+++ b/drivers/vfio/vfio.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
+ * Author: Alex Williamson <alex.williamson@redhat.com>
+ */
+
+/* events for the backend driver notify callback */
+enum vfio_iommu_notify_type {
+ VFIO_IOMMU_CONTAINER_CLOSE = 0,
+};
+
+/**
+ * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
+ */
+struct vfio_iommu_driver_ops {
+ char *name;
+ struct module *owner;
+ void *(*open)(unsigned long arg);
+ void (*release)(void *iommu_data);
+ long (*ioctl)(void *iommu_data, unsigned int cmd,
+ unsigned long arg);
+ int (*attach_group)(void *iommu_data,
+ struct iommu_group *group);
+ void (*detach_group)(void *iommu_data,
+ struct iommu_group *group);
+ int (*pin_pages)(void *iommu_data,
+ struct iommu_group *group,
+ unsigned long *user_pfn,
+ int npage, int prot,
+ unsigned long *phys_pfn);
+ int (*unpin_pages)(void *iommu_data,
+ unsigned long *user_pfn, int npage);
+ int (*register_notifier)(void *iommu_data,
+ unsigned long *events,
+ struct notifier_block *nb);
+ int (*unregister_notifier)(void *iommu_data,
+ struct notifier_block *nb);
+ int (*dma_rw)(void *iommu_data, dma_addr_t user_iova,
+ void *data, size_t count, bool write);
+ struct iommu_domain *(*group_iommu_domain)(void *iommu_data,
+ struct iommu_group *group);
+ void (*notify)(void *iommu_data,
+ enum vfio_iommu_notify_type event);
+};
+
+int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
+void vfio_unregister_iommu_driver(const struct vfio_iommu_driver_ops *ops);
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index fe888b5dcc0062..3efd09faeca4a8 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -20,6 +20,7 @@
#include <linux/sched/mm.h>
#include <linux/sched/signal.h>
#include <linux/mm.h>
+#include "vfio.h"
#include <asm/iommu.h>
#include <asm/tce.h>
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0b4f7c174c7a2b..92777797578e50 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -40,6 +40,7 @@
#include <linux/notifier.h>
#include <linux/dma-iommu.h>
#include <linux/irqdomain.h>
+#include "vfio.h"
#define DRIVER_VERSION "0.2"
#define DRIVER_AUTHOR "Alex Williamson <alex.williamson@redhat.com>"
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 82042690d7978d..5df56626751efd 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -82,50 +82,6 @@ extern void vfio_device_put(struct vfio_device *device);
int vfio_assign_device_set(struct vfio_device *device, void *set_id);
-/* events for the backend driver notify callback */
-enum vfio_iommu_notify_type {
- VFIO_IOMMU_CONTAINER_CLOSE = 0,
-};
-
-/**
- * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
- */
-struct vfio_iommu_driver_ops {
- char *name;
- struct module *owner;
- void *(*open)(unsigned long arg);
- void (*release)(void *iommu_data);
- long (*ioctl)(void *iommu_data, unsigned int cmd,
- unsigned long arg);
- int (*attach_group)(void *iommu_data,
- struct iommu_group *group);
- void (*detach_group)(void *iommu_data,
- struct iommu_group *group);
- int (*pin_pages)(void *iommu_data,
- struct iommu_group *group,
- unsigned long *user_pfn,
- int npage, int prot,
- unsigned long *phys_pfn);
- int (*unpin_pages)(void *iommu_data,
- unsigned long *user_pfn, int npage);
- int (*register_notifier)(void *iommu_data,
- unsigned long *events,
- struct notifier_block *nb);
- int (*unregister_notifier)(void *iommu_data,
- struct notifier_block *nb);
- int (*dma_rw)(void *iommu_data, dma_addr_t user_iova,
- void *data, size_t count, bool write);
- struct iommu_domain *(*group_iommu_domain)(void *iommu_data,
- struct iommu_group *group);
- void (*notify)(void *iommu_data,
- enum vfio_iommu_notify_type event);
-};
-
-extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
-
-extern void vfio_unregister_iommu_driver(
- const struct vfio_iommu_driver_ops *ops);
-
/*
* External user API
*/
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 09/14] vfio: move the vfio_iommu_driver_ops interface out of <linux/vfio.h>
2021-08-24 14:46 ` [PATCH 09/14] vfio: move the vfio_iommu_driver_ops interface out of <linux/vfio.h> Christoph Hellwig
@ 2021-08-25 0:25 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:44PM +0200, Christoph Hellwig wrote:
> Create a new private drivers/vfio/vfio.h header for the interface between
> the VFIO core and the iommu drivers.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 1 +
> drivers/vfio/vfio.h | 47 +++++++++++++++++++++++++++++
> drivers/vfio/vfio_iommu_spapr_tce.c | 1 +
> drivers/vfio/vfio_iommu_type1.c | 1 +
> include/linux/vfio.h | 44 ---------------------------
> 5 files changed, 50 insertions(+), 44 deletions(-)
> create mode 100644 drivers/vfio/vfio.h
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 10/14] vfio: remove the unused mdev iommu hook
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (8 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 09/14] vfio: move the vfio_iommu_driver_ops interface out of <linux/vfio.h> Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:25 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1 Christoph Hellwig
` (3 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
The iommu_device field in struct mdev_device has never been used
since it was added more than 2 years ago.
This is a manual revert of commit 7bd50f0cd2
("vfio/type1: Add domain at(de)taching group helpers").
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio_iommu_type1.c | 133 +++++++-------------------------
include/linux/mdev.h | 20 -----
2 files changed, 26 insertions(+), 127 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 92777797578e50..39e2706d0b3f34 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -114,7 +114,6 @@ struct vfio_batch {
struct vfio_iommu_group {
struct iommu_group *iommu_group;
struct list_head next;
- bool mdev_group; /* An mdev group */
bool pinned_page_dirty_scope;
};
@@ -1935,61 +1934,6 @@ static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
return ret;
}
-static int vfio_mdev_attach_domain(struct device *dev, void *data)
-{
- struct mdev_device *mdev = to_mdev_device(dev);
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = mdev_get_iommu_device(mdev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- return iommu_aux_attach_device(domain, iommu_device);
- else
- return iommu_attach_device(domain, iommu_device);
- }
-
- return -EINVAL;
-}
-
-static int vfio_mdev_detach_domain(struct device *dev, void *data)
-{
- struct mdev_device *mdev = to_mdev_device(dev);
- struct iommu_domain *domain = data;
- struct device *iommu_device;
-
- iommu_device = mdev_get_iommu_device(mdev);
- if (iommu_device) {
- if (iommu_dev_feature_enabled(iommu_device, IOMMU_DEV_FEAT_AUX))
- iommu_aux_detach_device(domain, iommu_device);
- else
- iommu_detach_device(domain, iommu_device);
- }
-
- return 0;
-}
-
-static int vfio_iommu_attach_group(struct vfio_domain *domain,
- struct vfio_iommu_group *group)
-{
- if (group->mdev_group)
- return iommu_group_for_each_dev(group->iommu_group,
- domain->domain,
- vfio_mdev_attach_domain);
- else
- return iommu_attach_group(domain->domain, group->iommu_group);
-}
-
-static void vfio_iommu_detach_group(struct vfio_domain *domain,
- struct vfio_iommu_group *group)
-{
- if (group->mdev_group)
- iommu_group_for_each_dev(group->iommu_group, domain->domain,
- vfio_mdev_detach_domain);
- else
- iommu_detach_group(domain->domain, group->iommu_group);
-}
-
static bool vfio_bus_is_mdev(struct bus_type *bus)
{
struct bus_type *mdev_bus;
@@ -2004,20 +1948,6 @@ static bool vfio_bus_is_mdev(struct bus_type *bus)
return ret;
}
-static int vfio_mdev_iommu_device(struct device *dev, void *data)
-{
- struct mdev_device *mdev = to_mdev_device(dev);
- struct device **old = data, *new;
-
- new = mdev_get_iommu_device(mdev);
- if (!new || (*old && *old != new))
- return -EINVAL;
-
- *old = new;
-
- return 0;
-}
-
/*
* This is a helper function to insert an address range to iova list.
* The list is initially created with a single entry corresponding to
@@ -2278,38 +2208,25 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
goto out_free;
if (vfio_bus_is_mdev(bus)) {
- struct device *iommu_device = NULL;
-
- group->mdev_group = true;
-
- /* Determine the isolation type */
- ret = iommu_group_for_each_dev(iommu_group, &iommu_device,
- vfio_mdev_iommu_device);
- if (ret || !iommu_device) {
- if (!iommu->external_domain) {
- INIT_LIST_HEAD(&domain->group_list);
- iommu->external_domain = domain;
- vfio_update_pgsize_bitmap(iommu);
- } else {
- kfree(domain);
- }
-
- list_add(&group->next,
- &iommu->external_domain->group_list);
- /*
- * Non-iommu backed group cannot dirty memory directly,
- * it can only use interfaces that provide dirty
- * tracking.
- * The iommu scope can only be promoted with the
- * addition of a dirty tracking group.
- */
- group->pinned_page_dirty_scope = true;
- mutex_unlock(&iommu->lock);
-
- return 0;
+ if (!iommu->external_domain) {
+ INIT_LIST_HEAD(&domain->group_list);
+ iommu->external_domain = domain;
+ vfio_update_pgsize_bitmap(iommu);
+ } else {
+ kfree(domain);
}
- bus = iommu_device->bus;
+ list_add(&group->next, &iommu->external_domain->group_list);
+ /*
+ * Non-iommu backed group cannot dirty memory directly, it can
+ * only use interfaces that provide dirty tracking.
+ * The iommu scope can only be promoted with the addition of a
+ * dirty tracking group.
+ */
+ group->pinned_page_dirty_scope = true;
+ mutex_unlock(&iommu->lock);
+
+ return 0;
}
domain->domain = iommu_domain_alloc(bus);
@@ -2324,7 +2241,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
goto out_domain;
}
- ret = vfio_iommu_attach_group(domain, group);
+ ret = iommu_attach_group(domain->domain, group->iommu_group);
if (ret)
goto out_domain;
@@ -2391,15 +2308,17 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
list_for_each_entry(d, &iommu->domain_list, next) {
if (d->domain->ops == domain->domain->ops &&
d->prot == domain->prot) {
- vfio_iommu_detach_group(domain, group);
- if (!vfio_iommu_attach_group(d, group)) {
+ iommu_detach_group(domain->domain, group->iommu_group);
+ if (!iommu_attach_group(d->domain,
+ group->iommu_group)) {
list_add(&group->next, &d->group_list);
iommu_domain_free(domain->domain);
kfree(domain);
goto done;
}
- ret = vfio_iommu_attach_group(domain, group);
+ ret = iommu_attach_group(domain->domain,
+ group->iommu_group);
if (ret)
goto out_domain;
}
@@ -2436,7 +2355,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
return 0;
out_detach:
- vfio_iommu_detach_group(domain, group);
+ iommu_detach_group(domain->domain, group->iommu_group);
out_domain:
iommu_domain_free(domain->domain);
vfio_iommu_iova_free(&iova_copy);
@@ -2601,7 +2520,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
if (!group)
continue;
- vfio_iommu_detach_group(domain, group);
+ iommu_detach_group(domain->domain, group->iommu_group);
update_dirty_scope = !group->pinned_page_dirty_scope;
list_del(&group->next);
kfree(group);
@@ -2689,7 +2608,7 @@ static void vfio_release_domain(struct vfio_domain *domain, bool external)
list_for_each_entry_safe(group, group_tmp,
&domain->group_list, next) {
if (!external)
- vfio_iommu_detach_group(domain, group);
+ iommu_detach_group(domain->domain, group->iommu_group);
list_del(&group->next);
kfree(group);
}
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 68427e8fadebd6..15d03f6532d073 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -18,7 +18,6 @@ struct mdev_device {
void *driver_data;
struct list_head next;
struct mdev_type *type;
- struct device *iommu_device;
bool active;
};
@@ -27,25 +26,6 @@ static inline struct mdev_device *to_mdev_device(struct device *dev)
return container_of(dev, struct mdev_device, dev);
}
-/*
- * Called by the parent device driver to set the device which represents
- * this mdev in iommu protection scope. By default, the iommu device is
- * NULL, that indicates using vendor defined isolation.
- *
- * @dev: the mediated device that iommu will isolate.
- * @iommu_device: a pci device which represents the iommu for @dev.
- */
-static inline void mdev_set_iommu_device(struct mdev_device *mdev,
- struct device *iommu_device)
-{
- mdev->iommu_device = iommu_device;
-}
-
-static inline struct device *mdev_get_iommu_device(struct mdev_device *mdev)
-{
- return mdev->iommu_device;
-}
-
unsigned int mdev_get_type_group_id(struct mdev_device *mdev);
unsigned int mtype_get_type_group_id(struct mdev_type *mtype);
struct device *mtype_get_parent_dev(struct mdev_type *mtype);
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 10/14] vfio: remove the unused mdev iommu hook
2021-08-24 14:46 ` [PATCH 10/14] vfio: remove the unused mdev iommu hook Christoph Hellwig
@ 2021-08-25 0:25 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:45PM +0200, Christoph Hellwig wrote:
> The iommu_device field in struct mdev_device has never been used
> since it was added more than 2 years ago.
>
> This is a manual revert of commit 7bd50f0cd2
> ("vfio/type1: Add domain at(de)taching group helpers").
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio_iommu_type1.c | 133 +++++++-------------------------
> include/linux/mdev.h | 20 -----
> 2 files changed, 26 insertions(+), 127 deletions(-)
Right, this was for the imagined PASID support that looks like it is
going in a different direction..
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (9 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 10/14] vfio: remove the unused mdev iommu hook Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:28 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 12/14] vfio/spapr_tce: reject mediated devices Christoph Hellwig
` (2 subsequent siblings)
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Pass the group flags to ->attach_group and remove the messy check for
the bus type.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio.c | 11 +++++------
drivers/vfio/vfio.h | 7 ++++++-
drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
drivers/vfio/vfio_iommu_type1.c | 19 ++-----------------
4 files changed, 14 insertions(+), 25 deletions(-)
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 26663dfa19e70d..a140ce3b15196d 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -68,9 +68,6 @@ struct vfio_unbound_dev {
struct list_head unbound_next;
};
-#define VFIO_MEDIATED (1 << 0)
-#define VFIO_NOIOMMU (1 << 1)
-
struct vfio_group {
struct kref kref;
int minor;
@@ -198,7 +195,7 @@ static long vfio_noiommu_ioctl(void *iommu_data,
}
static int vfio_noiommu_attach_group(void *iommu_data,
- struct iommu_group *iommu_group)
+ struct iommu_group *iommu_group, unsigned int flags)
{
return 0;
}
@@ -1100,7 +1097,8 @@ static int __vfio_container_attach_groups(struct vfio_container *container,
int ret = -ENODEV;
list_for_each_entry(group, &container->group_list, container_next) {
- ret = driver->ops->attach_group(data, group->iommu_group);
+ ret = driver->ops->attach_group(data, group->iommu_group,
+ group->flags);
if (ret)
goto unwind;
}
@@ -1358,7 +1356,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
driver = container->iommu_driver;
if (driver) {
ret = driver->ops->attach_group(container->iommu_data,
- group->iommu_group);
+ group->iommu_group,
+ group->flags);
if (ret)
goto unlock_out;
}
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a78de649eb2f16..0c1a88fa991545 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -9,6 +9,10 @@ enum vfio_iommu_notify_type {
VFIO_IOMMU_CONTAINER_CLOSE = 0,
};
+/* flags for group->flags and ->attach_group */
+#define VFIO_MEDIATED (1 << 0)
+#define VFIO_NOIOMMU (1 << 1)
+
/**
* struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
*/
@@ -20,7 +24,8 @@ struct vfio_iommu_driver_ops {
long (*ioctl)(void *iommu_data, unsigned int cmd,
unsigned long arg);
int (*attach_group)(void *iommu_data,
- struct iommu_group *group);
+ struct iommu_group *group,
+ unsigned int flags);
void (*detach_group)(void *iommu_data,
struct iommu_group *group);
int (*pin_pages)(void *iommu_data,
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 3efd09faeca4a8..7567328d347d25 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -1239,7 +1239,7 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
}
static int tce_iommu_attach_group(void *iommu_data,
- struct iommu_group *iommu_group)
+ struct iommu_group *iommu_group, unsigned int flags)
{
int ret = 0;
struct tce_container *container = iommu_data;
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 39e2706d0b3f34..44a3abdca580a0 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -36,7 +36,6 @@
#include <linux/uaccess.h>
#include <linux/vfio.h>
#include <linux/workqueue.h>
-#include <linux/mdev.h>
#include <linux/notifier.h>
#include <linux/dma-iommu.h>
#include <linux/irqdomain.h>
@@ -1934,20 +1933,6 @@ static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
return ret;
}
-static bool vfio_bus_is_mdev(struct bus_type *bus)
-{
- struct bus_type *mdev_bus;
- bool ret = false;
-
- mdev_bus = symbol_get(mdev_bus_type);
- if (mdev_bus) {
- ret = (bus == mdev_bus);
- symbol_put(mdev_bus_type);
- }
-
- return ret;
-}
-
/*
* This is a helper function to insert an address range to iova list.
* The list is initially created with a single entry corresponding to
@@ -2172,7 +2157,7 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu *iommu,
}
static int vfio_iommu_type1_attach_group(void *iommu_data,
- struct iommu_group *iommu_group)
+ struct iommu_group *iommu_group, unsigned int flags)
{
struct vfio_iommu *iommu = iommu_data;
struct vfio_iommu_group *group;
@@ -2207,7 +2192,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
if (ret)
goto out_free;
- if (vfio_bus_is_mdev(bus)) {
+ if (flags & VFIO_MEDIATED) {
if (!iommu->external_domain) {
INIT_LIST_HEAD(&domain->group_list);
iommu->external_domain = domain;
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1
2021-08-24 14:46 ` [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1 Christoph Hellwig
@ 2021-08-25 0:28 ` Jason Gunthorpe
2021-08-25 5:34 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:46PM +0200, Christoph Hellwig wrote:
> Pass the group flags to ->attach_group and remove the messy check for
> the bus type.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio.c | 11 +++++------
> drivers/vfio/vfio.h | 7 ++++++-
> drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
> drivers/vfio/vfio_iommu_type1.c | 19 ++-----------------
> 4 files changed, 14 insertions(+), 25 deletions(-)
Every caller is doing group->iommu_group, maybe change the signature
to
(*attach_group)(struct vfio_iommu *iommu, struct vfio_iommu_group *group)
?
Then the flags don't need to be another arg, just group->flags
Happy to see the symbol_get removed!
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1
2021-08-25 0:28 ` Jason Gunthorpe
@ 2021-08-25 5:34 ` Christoph Hellwig
2021-08-25 12:35 ` Jason Gunthorpe
0 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 5:34 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Tue, Aug 24, 2021 at 09:28:50PM -0300, Jason Gunthorpe wrote:
> On Tue, Aug 24, 2021 at 04:46:46PM +0200, Christoph Hellwig wrote:
> > Pass the group flags to ->attach_group and remove the messy check for
> > the bus type.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > drivers/vfio/vfio.c | 11 +++++------
> > drivers/vfio/vfio.h | 7 ++++++-
> > drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
> > drivers/vfio/vfio_iommu_type1.c | 19 ++-----------------
> > 4 files changed, 14 insertions(+), 25 deletions(-)
>
> Every caller is doing group->iommu_group, maybe change the signature
> to
>
> (*attach_group)(struct vfio_iommu *iommu, struct vfio_iommu_group *group)
>
> ?
s/vfio_iommu/vfio_container/, but yes. I actually have a series that
does this (also for a few other methods), but this requires exposing
vfio_container and vfio_iommu_group outside of vfio.c. Given that this
series is big enough I decided to skip it for now, but that plus a few
other changes will eventually simplify the group lookups in
vfio_iommu_type1.c.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1
2021-08-25 5:34 ` Christoph Hellwig
@ 2021-08-25 12:35 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 12:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Wed, Aug 25, 2021 at 07:34:27AM +0200, Christoph Hellwig wrote:
> On Tue, Aug 24, 2021 at 09:28:50PM -0300, Jason Gunthorpe wrote:
> > On Tue, Aug 24, 2021 at 04:46:46PM +0200, Christoph Hellwig wrote:
> > > Pass the group flags to ->attach_group and remove the messy check for
> > > the bus type.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > drivers/vfio/vfio.c | 11 +++++------
> > > drivers/vfio/vfio.h | 7 ++++++-
> > > drivers/vfio/vfio_iommu_spapr_tce.c | 2 +-
> > > drivers/vfio/vfio_iommu_type1.c | 19 ++-----------------
> > > 4 files changed, 14 insertions(+), 25 deletions(-)
> >
> > Every caller is doing group->iommu_group, maybe change the signature
> > to
> >
> > (*attach_group)(struct vfio_iommu *iommu, struct vfio_iommu_group *group)
> >
> > ?
>
> s/vfio_iommu/vfio_container/, but yes. I actually have a series that
> does this (also for a few other methods), but this requires exposing
> vfio_container and vfio_iommu_group outside of vfio.c. Given that this
> series is big enough I decided to skip it for now, but that plus a few
> other changes will eventually simplify the group lookups in
> vfio_iommu_type1.c.
Fair enough
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 12/14] vfio/spapr_tce: reject mediated devices
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (10 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 11/14] vfio: clean up the check for mediated device in vfio_iommu_type1 Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:29 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 13/14] vfio/iommu_type1: remove the "external" domain Christoph Hellwig
2021-08-24 14:46 ` [PATCH 14/14] vfio/iommu_type1: remove IS_IOMMU_CAP_DOMAIN_IN_CONTAINER Christoph Hellwig
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
Unlike the the type1 IOMMU backend, the SPAPR one does not contain any
support for the magic non-IOMMU backed iommu_group used by mediated
devices, so reject them in ->attach_group.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio_iommu_spapr_tce.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 7567328d347d25..779d4e985d825f 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -1246,6 +1246,9 @@ static int tce_iommu_attach_group(void *iommu_data,
struct iommu_table_group *table_group;
struct tce_iommu_group *tcegrp = NULL;
+ if (flags & VFIO_MEDIATED)
+ return -EINVAL;
+
mutex_lock(&container->lock);
/* pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 12/14] vfio/spapr_tce: reject mediated devices
2021-08-24 14:46 ` [PATCH 12/14] vfio/spapr_tce: reject mediated devices Christoph Hellwig
@ 2021-08-25 0:29 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:47PM +0200, Christoph Hellwig wrote:
> Unlike the the type1 IOMMU backend, the SPAPR one does not contain any
> support for the magic non-IOMMU backed iommu_group used by mediated
> devices, so reject them in ->attach_group.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio_iommu_spapr_tce.c | 3 +++
> 1 file changed, 3 insertions(+)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 13/14] vfio/iommu_type1: remove the "external" domain
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (11 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 12/14] vfio/spapr_tce: reject mediated devices Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:36 ` Jason Gunthorpe
2021-08-24 14:46 ` [PATCH 14/14] vfio/iommu_type1: remove IS_IOMMU_CAP_DOMAIN_IN_CONTAINER Christoph Hellwig
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
The external_domain concept rather misleading and not actually needed.
Replace it with a list of mediated groups in struct vfio_iommu and
document the purpose.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio_iommu_type1.c | 123 +++++++++++++++-----------------
1 file changed, 57 insertions(+), 66 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 44a3abdca580a0..205f13c05b236e 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -65,7 +65,6 @@ MODULE_PARM_DESC(dma_entry_limit,
struct vfio_iommu {
struct list_head domain_list;
struct list_head iova_list;
- struct vfio_domain *external_domain; /* domain for external user */
struct mutex lock;
struct rb_root dma_list;
struct blocking_notifier_head notifier;
@@ -78,6 +77,12 @@ struct vfio_iommu {
bool nesting;
bool dirty_page_tracking;
bool container_open;
+
+ /*
+ * Tracks the fake iommu groups created by vfio to support mediated
+ * devices. These are not backed by an actual IOMMU.
+ */
+ struct list_head mediated_groups;
};
struct vfio_domain {
@@ -1892,8 +1897,8 @@ static struct vfio_iommu_group*
vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
struct iommu_group *iommu_group)
{
+ struct vfio_iommu_group *group;
struct vfio_domain *domain;
- struct vfio_iommu_group *group = NULL;
list_for_each_entry(domain, &iommu->domain_list, next) {
group = find_iommu_group(domain, iommu_group);
@@ -1901,10 +1906,10 @@ vfio_iommu_find_iommu_group(struct vfio_iommu *iommu,
return group;
}
- if (iommu->external_domain)
- group = find_iommu_group(iommu->external_domain, iommu_group);
-
- return group;
+ list_for_each_entry(group, &iommu->mediated_groups, next)
+ if (group->iommu_group == iommu_group)
+ return group;
+ return NULL;
}
static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
@@ -2163,45 +2168,27 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
struct vfio_iommu_group *group;
struct vfio_domain *domain, *d;
struct bus_type *bus = NULL;
- int ret;
bool resv_msi, msi_remap;
phys_addr_t resv_msi_base = 0;
struct iommu_domain_geometry *geo;
LIST_HEAD(iova_copy);
LIST_HEAD(group_resv_regions);
+ int ret = -EINVAL;
mutex_lock(&iommu->lock);
/* Check for duplicates */
- if (vfio_iommu_find_iommu_group(iommu, iommu_group)) {
- mutex_unlock(&iommu->lock);
- return -EINVAL;
- }
+ if (vfio_iommu_find_iommu_group(iommu, iommu_group))
+ goto out_unlock;
+ ret = -ENOMEM;
group = kzalloc(sizeof(*group), GFP_KERNEL);
- domain = kzalloc(sizeof(*domain), GFP_KERNEL);
- if (!group || !domain) {
- ret = -ENOMEM;
- goto out_free;
- }
-
+ if (!group)
+ goto out_unlock;
group->iommu_group = iommu_group;
- /* Determine bus_type in order to allocate a domain */
- ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
- if (ret)
- goto out_free;
-
if (flags & VFIO_MEDIATED) {
- if (!iommu->external_domain) {
- INIT_LIST_HEAD(&domain->group_list);
- iommu->external_domain = domain;
- vfio_update_pgsize_bitmap(iommu);
- } else {
- kfree(domain);
- }
-
- list_add(&group->next, &iommu->external_domain->group_list);
+ list_add(&group->next, &iommu->mediated_groups);
/*
* Non-iommu backed group cannot dirty memory directly, it can
* only use interfaces that provide dirty tracking.
@@ -2209,16 +2196,24 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
* dirty tracking group.
*/
group->pinned_page_dirty_scope = true;
- mutex_unlock(&iommu->lock);
-
- return 0;
+ ret = 0;
+ goto out_unlock;
}
+ /* Determine bus_type in order to allocate a domain */
+ ret = iommu_group_for_each_dev(iommu_group, &bus, vfio_bus_type);
+ if (ret)
+ goto out_free_group;
+
+ ret = -ENOMEM;
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (!domain)
+ goto out_free_group;
+
+ ret = -EIO;
domain->domain = iommu_domain_alloc(bus);
- if (!domain->domain) {
- ret = -EIO;
- goto out_free;
- }
+ if (!domain->domain)
+ goto out_free_domain;
if (iommu->nesting) {
ret = iommu_enable_nesting(domain->domain);
@@ -2345,9 +2340,11 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
iommu_domain_free(domain->domain);
vfio_iommu_iova_free(&iova_copy);
vfio_iommu_resv_free(&group_resv_regions);
-out_free:
+out_free_domain:
kfree(domain);
+out_free_group:
kfree(group);
+out_unlock:
mutex_unlock(&iommu->lock);
return ret;
}
@@ -2472,25 +2469,19 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
LIST_HEAD(iova_copy);
mutex_lock(&iommu->lock);
+ list_for_each_entry(group, &iommu->mediated_groups, next) {
+ if (group->iommu_group != iommu_group)
+ continue;
+ update_dirty_scope = !group->pinned_page_dirty_scope;
+ list_del(&group->next);
+ kfree(group);
- if (iommu->external_domain) {
- group = find_iommu_group(iommu->external_domain, iommu_group);
- if (group) {
- update_dirty_scope = !group->pinned_page_dirty_scope;
- list_del(&group->next);
- kfree(group);
-
- if (list_empty(&iommu->external_domain->group_list)) {
- if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
- WARN_ON(iommu->notifier.head);
- vfio_iommu_unmap_unpin_all(iommu);
- }
-
- kfree(iommu->external_domain);
- iommu->external_domain = NULL;
- }
- goto detach_group_done;
+ if (list_empty(&iommu->mediated_groups) &&
+ !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
+ WARN_ON(iommu->notifier.head);
+ vfio_iommu_unmap_unpin_all(iommu);
}
+ goto detach_group_done;
}
/*
@@ -2518,7 +2509,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
*/
if (list_empty(&domain->group_list)) {
if (list_is_singular(&iommu->domain_list)) {
- if (!iommu->external_domain) {
+ if (list_empty(&iommu->mediated_groups)) {
WARN_ON(iommu->notifier.head);
vfio_iommu_unmap_unpin_all(iommu);
} else {
@@ -2582,41 +2573,41 @@ static void *vfio_iommu_type1_open(unsigned long arg)
mutex_init(&iommu->lock);
BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
init_waitqueue_head(&iommu->vaddr_wait);
+ INIT_LIST_HEAD(&iommu->mediated_groups);
return iommu;
}
-static void vfio_release_domain(struct vfio_domain *domain, bool external)
+static void vfio_release_domain(struct vfio_domain *domain)
{
struct vfio_iommu_group *group, *group_tmp;
list_for_each_entry_safe(group, group_tmp,
&domain->group_list, next) {
- if (!external)
- iommu_detach_group(domain->domain, group->iommu_group);
+ iommu_detach_group(domain->domain, group->iommu_group);
list_del(&group->next);
kfree(group);
}
- if (!external)
- iommu_domain_free(domain->domain);
+ iommu_domain_free(domain->domain);
}
static void vfio_iommu_type1_release(void *iommu_data)
{
struct vfio_iommu *iommu = iommu_data;
struct vfio_domain *domain, *domain_tmp;
+ struct vfio_iommu_group *group, *n;
- if (iommu->external_domain) {
- vfio_release_domain(iommu->external_domain, true);
- kfree(iommu->external_domain);
+ list_for_each_entry_safe(group, n, &iommu->mediated_groups, next) {
+ list_del(&group->next);
+ kfree(group);
}
vfio_iommu_unmap_unpin_all(iommu);
list_for_each_entry_safe(domain, domain_tmp,
&iommu->domain_list, next) {
- vfio_release_domain(domain, false);
+ vfio_release_domain(domain);
list_del(&domain->next);
kfree(domain);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 13/14] vfio/iommu_type1: remove the "external" domain
2021-08-24 14:46 ` [PATCH 13/14] vfio/iommu_type1: remove the "external" domain Christoph Hellwig
@ 2021-08-25 0:36 ` Jason Gunthorpe
2021-08-25 5:35 ` Christoph Hellwig
0 siblings, 1 reply; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:36 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:48PM +0200, Christoph Hellwig wrote:
> The external_domain concept rather misleading and not actually needed.
> Replace it with a list of mediated groups in struct vfio_iommu and
> document the purpose.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio_iommu_type1.c | 123 +++++++++++++++-----------------
> 1 file changed, 57 insertions(+), 66 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> + /*
> + * Tracks the fake iommu groups created by vfio to support mediated
> + * devices. These are not backed by an actual IOMMU.
> + */
> + struct list_head mediated_groups;
Though again I'd prefer another name to mediated here.. These are
domainless groups?
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH 13/14] vfio/iommu_type1: remove the "external" domain
2021-08-25 0:36 ` Jason Gunthorpe
@ 2021-08-25 5:35 ` Christoph Hellwig
0 siblings, 0 replies; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-25 5:35 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Christoph Hellwig, Alex Williamson, Diana Craciun, Cornelia Huck,
Kirti Wankhede, Eric Auger, kvm
On Tue, Aug 24, 2021 at 09:36:29PM -0300, Jason Gunthorpe wrote:
> > + /*
> > + * Tracks the fake iommu groups created by vfio to support mediated
> > + * devices. These are not backed by an actual IOMMU.
> > + */
> > + struct list_head mediated_groups;
>
> Though again I'd prefer another name to mediated here.. These are
> domainless groups?
I think they should use whatever named we use for/instead of the mediated
groups as these are the same concept. Using an unrelated name like
external did causes major confusion.
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH 14/14] vfio/iommu_type1: remove IS_IOMMU_CAP_DOMAIN_IN_CONTAINER
2021-08-24 14:46 cleanup vfio iommu_group creation v2 Christoph Hellwig
` (12 preceding siblings ...)
2021-08-24 14:46 ` [PATCH 13/14] vfio/iommu_type1: remove the "external" domain Christoph Hellwig
@ 2021-08-24 14:46 ` Christoph Hellwig
2021-08-25 0:38 ` Jason Gunthorpe
13 siblings, 1 reply; 58+ messages in thread
From: Christoph Hellwig @ 2021-08-24 14:46 UTC (permalink / raw)
To: Alex Williamson
Cc: Diana Craciun, Cornelia Huck, Kirti Wankhede, Eric Auger,
Jason Gunthorpe, kvm
IS_IOMMU_CAP_DOMAIN_IN_CONTAINER just obsfucated the checks being
performed, so open code it in the callers.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/vfio/vfio_iommu_type1.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 205f13c05b236e..42bd902243eca5 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -144,9 +144,6 @@ struct vfio_regions {
size_t len;
};
-#define IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu) \
- (!list_empty(&iommu->domain_list))
-
#define DIRTY_BITMAP_BYTES(n) (ALIGN(n, BITS_PER_TYPE(u64)) / BITS_PER_BYTE)
/*
@@ -884,7 +881,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
* already pinned and accounted. Accounting should be done if there is no
* iommu capable domain in the container.
*/
- do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
+ do_accounting = list_empty(&iommu->domain_list);
for (i = 0; i < npage; i++) {
struct vfio_pfn *vpfn;
@@ -973,7 +970,7 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
mutex_lock(&iommu->lock);
- do_accounting = !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu);
+ do_accounting = list_empty(&iommu->domain_list);
for (i = 0; i < npage; i++) {
struct vfio_dma *dma;
dma_addr_t iova;
@@ -1094,7 +1091,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
if (!dma->size)
return 0;
- if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
+ if (list_empty(&iommu->domain_list))
return 0;
/*
@@ -1671,7 +1668,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
vfio_link_dma(iommu, dma);
/* Don't pin and map if container doesn't contain IOMMU capable domain*/
- if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu))
+ if (list_empty(&iommu->domain_list))
dma->size = size;
else
ret = vfio_pin_map_dma(iommu, dma, size);
@@ -2477,7 +2474,7 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
kfree(group);
if (list_empty(&iommu->mediated_groups) &&
- !IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
+ list_empty(&iommu->domain_list)) {
WARN_ON(iommu->notifier.head);
vfio_iommu_unmap_unpin_all(iommu);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: [PATCH 14/14] vfio/iommu_type1: remove IS_IOMMU_CAP_DOMAIN_IN_CONTAINER
2021-08-24 14:46 ` [PATCH 14/14] vfio/iommu_type1: remove IS_IOMMU_CAP_DOMAIN_IN_CONTAINER Christoph Hellwig
@ 2021-08-25 0:38 ` Jason Gunthorpe
0 siblings, 0 replies; 58+ messages in thread
From: Jason Gunthorpe @ 2021-08-25 0:38 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alex Williamson, Diana Craciun, Cornelia Huck, Kirti Wankhede,
Eric Auger, kvm
On Tue, Aug 24, 2021 at 04:46:49PM +0200, Christoph Hellwig wrote:
> IS_IOMMU_CAP_DOMAIN_IN_CONTAINER just obsfucated the checks being
> performed, so open code it in the callers.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/vfio/vfio_iommu_type1.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 58+ messages in thread