[PATCH] iommu/virtio: Detach domain on endpoint release

[PATCH] iommu/virtio: Detach domain on endpoint release

[Jean-Philippe Brucker]

When an endpoint is released, for example a PCIe VF is disabled or a
function hot-unplugged, it should be detached from its domain. Send a
DETACH request.

Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Link: https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 drivers/iommu/virtio-iommu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 5b8fe9bfa9a5..3d3d4462359e 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -788,6 +788,28 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return 0;
 }
 
+static void viommu_detach_dev(struct viommu_endpoint *vdev)
+{
+	int i;
+	struct virtio_iommu_req_detach req;
+	struct viommu_domain *vdomain = vdev->vdomain;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(vdev->dev);
+
+	if (!vdomain)
+		return;
+
+	req = (struct virtio_iommu_req_detach) {
+		.head.type	= VIRTIO_IOMMU_T_DETACH,
+		.domain		= cpu_to_le32(vdomain->id),
+	};
+
+	for (i = 0; i < fwspec->num_ids; i++) {
+		req.endpoint = cpu_to_le32(fwspec->ids[i]);
+		WARN_ON(viommu_send_req_sync(vdev->viommu, &req, sizeof(req)));
+	}
+	vdev->vdomain = NULL;
+}
+
 static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			    int prot, gfp_t gfp, size_t *mapped)
@@ -990,6 +1012,7 @@ static void viommu_release_device(struct device *dev)
 {
 	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
 
+	viommu_detach_dev(vdev);
 	iommu_put_resv_regions(dev, &vdev->resv_regions);
 	kfree(vdev);
 }
-- 
2.40.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] iommu/virtio: Detach domain on endpoint release

[Jean-Philippe Brucker]

Hi Joerg,

On Fri, Apr 14, 2023 at 04:07:45PM +0100, Jean-Philippe Brucker wrote:
> When an endpoint is released, for example a PCIe VF is disabled or a
> function hot-unplugged, it should be detached from its domain. Send a
> DETACH request.
> 
> Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
> Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Link: https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

This patch fixes device unregistration in the virtio-iommu driver, could
you please pick it up for the next batch of fixes?  It applies cleanly on
v6.4-rc1

Thanks,
Jean

> ---
>  drivers/iommu/virtio-iommu.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 5b8fe9bfa9a5..3d3d4462359e 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -788,6 +788,28 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	return 0;
>  }
>  
> +static void viommu_detach_dev(struct viommu_endpoint *vdev)
> +{
> +	int i;
> +	struct virtio_iommu_req_detach req;
> +	struct viommu_domain *vdomain = vdev->vdomain;
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(vdev->dev);
> +
> +	if (!vdomain)
> +		return;
> +
> +	req = (struct virtio_iommu_req_detach) {
> +		.head.type	= VIRTIO_IOMMU_T_DETACH,
> +		.domain		= cpu_to_le32(vdomain->id),
> +	};
> +
> +	for (i = 0; i < fwspec->num_ids; i++) {
> +		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> +		WARN_ON(viommu_send_req_sync(vdev->viommu, &req, sizeof(req)));
> +	}
> +	vdev->vdomain = NULL;
> +}
> +
>  static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
>  			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
>  			    int prot, gfp_t gfp, size_t *mapped)
> @@ -990,6 +1012,7 @@ static void viommu_release_device(struct device *dev)
>  {
>  	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
>  
> +	viommu_detach_dev(vdev);
>  	iommu_put_resv_regions(dev, &vdev->resv_regions);
>  	kfree(vdev);
>  }
> -- 
> 2.40.0
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] iommu/virtio: Detach domain on endpoint release

[Eric Auger]

Hi Jean,

On 4/14/23 17:07, Jean-Philippe Brucker wrote:
> When an endpoint is released, for example a PCIe VF is disabled or a
> function hot-unplugged, it should be detached from its domain. Send a
> DETACH request.
>
> Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
> Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Link: https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  drivers/iommu/virtio-iommu.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 5b8fe9bfa9a5..3d3d4462359e 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -788,6 +788,28 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	return 0;
>  }
>  
> +static void viommu_detach_dev(struct viommu_endpoint *vdev)
> +{
> +	int i;
> +	struct virtio_iommu_req_detach req;
> +	struct viommu_domain *vdomain = vdev->vdomain;
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(vdev->dev);
> +
> +	if (!vdomain)
> +		return;
> +
> +	req = (struct virtio_iommu_req_detach) {
> +		.head.type	= VIRTIO_IOMMU_T_DETACH,
> +		.domain		= cpu_to_le32(vdomain->id),
> +	};
> +
> +	for (i = 0; i < fwspec->num_ids; i++) {
> +		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> +		WARN_ON(viommu_send_req_sync(vdev->viommu, &req, sizeof(req)));
> +	}
just a late question: don't you need to decrement vdomain's nr_endpoints?

Thanks

Eric
> +	vdev->vdomain = NULL;
> +}
> +
>  static int viommu_map_pages(struct iommu_domain *domain, unsigned long iova,
>  			    phys_addr_t paddr, size_t pgsize, size_t pgcount,
>  			    int prot, gfp_t gfp, size_t *mapped)
> @@ -990,6 +1012,7 @@ static void viommu_release_device(struct device *dev)
>  {
>  	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
>  
> +	viommu_detach_dev(vdev);
>  	iommu_put_resv_regions(dev, &vdev->resv_regions);
>  	kfree(vdev);
>  }

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] iommu/virtio: Detach domain on endpoint release

[Jean-Philippe Brucker]

On Wed, May 10, 2023 at 05:37:22PM +0200, Eric Auger wrote:
> Hi Jean,
> 
> On 4/14/23 17:07, Jean-Philippe Brucker wrote:
> > When an endpoint is released, for example a PCIe VF is disabled or a
> > function hot-unplugged, it should be detached from its domain. Send a
> > DETACH request.
> >
> > Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
> > Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Link: https://lore.kernel.org/all/15bf1b00-3aa0-973a-3a86-3fa5c4d41d2c@daynix.com/
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  drivers/iommu/virtio-iommu.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 5b8fe9bfa9a5..3d3d4462359e 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -788,6 +788,28 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  	return 0;
> >  }
> >  
> > +static void viommu_detach_dev(struct viommu_endpoint *vdev)
> > +{
> > +	int i;
> > +	struct virtio_iommu_req_detach req;
> > +	struct viommu_domain *vdomain = vdev->vdomain;
> > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(vdev->dev);
> > +
> > +	if (!vdomain)
> > +		return;
> > +
> > +	req = (struct virtio_iommu_req_detach) {
> > +		.head.type	= VIRTIO_IOMMU_T_DETACH,
> > +		.domain		= cpu_to_le32(vdomain->id),
> > +	};
> > +
> > +	for (i = 0; i < fwspec->num_ids; i++) {
> > +		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> > +		WARN_ON(viommu_send_req_sync(vdev->viommu, &req, sizeof(req)));
> > +	}
> just a late question: don't you need to decrement vdomain's nr_endpoints?
> 

Ah yes, I'll fix that, thank you. I think attach() could use some cleanup
as well: if the request fails then we should keep the nr_endpoints
reference on the previous domain. But that's less urgent.

Thanks,
Jean

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] iommu/virtio: Detach domain on endpoint release

[Jean-Philippe Brucker]

On Wed, May 17, 2023 at 01:20:51PM -0300, Jason Gunthorpe wrote:
> On Wed, May 10, 2023 at 09:11:57AM +0100, Jean-Philippe Brucker wrote:
> 
> > > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > > index 5b8fe9bfa9a5..3d3d4462359e 100644
> > > --- a/drivers/iommu/virtio-iommu.c
> > > +++ b/drivers/iommu/virtio-iommu.c
> > > @@ -788,6 +788,28 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > >  	return 0;
> > >  }
> > >  
> > > +static void viommu_detach_dev(struct viommu_endpoint *vdev)
> > > +{
> > > +	int i;
> > > +	struct virtio_iommu_req_detach req;
> > > +	struct viommu_domain *vdomain = vdev->vdomain;
> > > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(vdev->dev);
> > > +
> > > +	if (!vdomain)
> > > +		return;
> > > +
> > > +	req = (struct virtio_iommu_req_detach) {
> > > +		.head.type	= VIRTIO_IOMMU_T_DETACH,
> > > +		.domain		= cpu_to_le32(vdomain->id),
> > > +	};
> > > +
> > > +	for (i = 0; i < fwspec->num_ids; i++) {
> > > +		req.endpoint = cpu_to_le32(fwspec->ids[i]);
> > > +		WARN_ON(viommu_send_req_sync(vdev->viommu, &req, sizeof(req)));
> > > +	}
> > > +	vdev->vdomain = NULL;
> 
> Not for this patch, but something to work on..
> 
> I assume detach disconnects the container on the VFIO side and puts it
> into a BLOCKED state?

At the moment that depends on the VMM boot-bypass policy: if virtio-iommu
is booted with global-bypass set, then detaching will go back to an
identity-mapped container, not a BLOCKED state. If the global-bypass bit
is clear, then it does go back to a BLOCKED state. However QEMU has a
default policy of boot-bypass (because that allows booting a firmware or
OS that doesn't have the IOMMU drivers).

The driver can clear the global-bypass bit to change this behavior, but at
the moment we just follow the VMM boot policy.

> 
> Can you wrapper this into a BLOCKED domain like we are moving drivers
> toward, and then attach the blocked domain instead of introducing this
> special case?

Yes, I think the way the virtio-iommu driver should implement BLOCKED
domains is initially clearing the global-bypass bit, and then issuing
DETACH requests when the core asks to attach a BLOCKED domain. This has
the same effect as issuing an ATTACH request with an empty domain, but
requires fewer resources in the VMM.

Thanks,
Jean

> 
> I've been thinking about having some core code support to do fairly
> common pattern of 'parking' the iommu at some well defined translation
> mode, BLOCKED in this case.
> 
> Thanks,
> Jason
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] iommu/virtio: Detach domain on endpoint release

[Jean-Philippe Brucker]

On Thu, May 18, 2023 at 10:59:12AM -0300, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 02:56:38PM +0100, Jean-Philippe Brucker wrote:
> > > Can you wrapper this into a BLOCKED domain like we are moving drivers
> > > toward, and then attach the blocked domain instead of introducing this
> > > special case?
> > 
> > Yes, I think the way the virtio-iommu driver should implement BLOCKED
> > domains is initially clearing the global-bypass bit, and then issuing
> > DETACH requests when the core asks to attach a BLOCKED domain. This has
> > the same effect as issuing an ATTACH request with an empty domain, but
> > requires fewer resources in the VMM.
> 
> Does that exclude identity though?

No, identity attaches a domain with the ATTACH_F_BYPASS flag (or an
identity-mapped domain if the feature is missing), it doesn't rely on
global-bypass.

> 
> It seems like the protocol should not have an implicit operation like
> this, the desired translation mode should always be made
> explicit.

I probably misunderstood your plan for BLOCKED. This particular patch is
about removing devices from the machine, for example PCIe hot-unplug. So I
thought you were suggesting the core will at some point attach a BLOCKED
domain to a device being removed, in order to block translation while the
device is being removed and while a new one is being plugged in with the
same RID. For that case I think DETACH, rather than ATTACH an empty
domain, makes more sense. Otherwise with the same reasoning we'd need to
attach all 4 billion endpoint IDs to an empty domain at boot which isn't
feasible. In addition I don't think we'll want to force the VMMs to keep
endpoint ID state internally after destroying devices, though that does
need to be specified one way or another.

If BLOCKED is only for transient states, for example while a struct device
is not bound to a driver, then attaching an empty domain works and is
simpler to implement. Probably the best is to implement BLOCKED this way
and still call DETACH in the release_device() op.

In any case, it shouldn't make a difference to the core. I'll see which
one is better for the VMMs.

Thanks,
Jean

> Having a boot time default makes sense, but there is little
> reason for an OS to go back to the boot time default...
> 
> Jason
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization