Since commit fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev
calls") we can end up with duplicates in the list of devices attached to
a domain. This is inefficient and confusing since only one domain can
actually be in control of the IOMMU translations for a device. Fix this
by detaching the device from the previous domain, if any, on attach.
This also makes the restore behavior analogous between IOMMU and DMA API
control.

Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
 drivers/iommu/s390-iommu.c | 82 ++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index c898bcbbce11..de8f76775240 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -83,14 +83,41 @@ static void s390_domain_free(struct iommu_domain *domain)
 	kfree(s390_domain);
 }
 
+static bool __s390_iommu_detach_device(struct s390_domain *s390_domain,
+				     struct zpci_dev *zdev)
+{
+	struct s390_domain_device *domain_device, *tmp;
+	unsigned long flags;
+	bool found = false;
+
+	spin_lock_irqsave(&s390_domain->list_lock, flags);
+	list_for_each_entry_safe(domain_device, tmp, &s390_domain->devices,
+				 list) {
+		if (domain_device->zdev == zdev) {
+			list_del(&domain_device->list);
+			kfree(domain_device);
+			found = true;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
+
+	if (found) {
+		zdev->s390_domain = NULL;
+		zpci_unregister_ioat(zdev, 0);
+	}
+	return found;
+}
+
 static int s390_iommu_attach_device(struct iommu_domain *domain,
 				    struct device *dev)
 {
 	struct s390_domain *s390_domain = to_s390_domain(domain);
 	struct zpci_dev *zdev = to_zpci_dev(dev);
 	struct s390_domain_device *domain_device;
+	struct s390_domain *prev_domain = NULL;
 	unsigned long flags;
-	int cc, rc;
+	int cc, rc = 0;
 
 	if (!zdev)
 		return -ENODEV;
@@ -99,16 +126,16 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 	if (!domain_device)
 		return -ENOMEM;
 
-	if (zdev->dma_table && !zdev->s390_domain) {
-		cc = zpci_dma_exit_device(zdev);
-		if (cc) {
+	if (zdev->s390_domain) {
+		prev_domain = zdev->s390_domain;
+		if (!__s390_iommu_detach_device(zdev->s390_domain, zdev))
+			rc = -EIO;
+	} else if (zdev->dma_table) {
+		if (zpci_dma_exit_device(zdev))
 			rc = -EIO;
-			goto out_free;
-		}
 	}
-
-	if (zdev->s390_domain)
-		zpci_unregister_ioat(zdev, 0);
+	if (rc)
+		goto out_free;
 
 	zdev->dma_table = s390_domain->dma_table;
 	cc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
@@ -129,7 +156,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 		   domain->geometry.aperture_end != zdev->end_dma) {
 		rc = -EINVAL;
 		spin_unlock_irqrestore(&s390_domain->list_lock, flags);
-		goto out_restore;
+		goto out_unregister_restore;
 	}
 	domain_device->zdev = zdev;
 	zdev->s390_domain = s390_domain;
@@ -138,14 +165,15 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 
 	return 0;
 
+out_unregister_restore:
+	zpci_unregister_ioat(zdev, 0);
 out_restore:
-	if (!zdev->s390_domain) {
+	zdev->dma_table = NULL;
+	if (prev_domain)
+		s390_iommu_attach_device(&prev_domain->domain,
+					 dev);
+	else
 		zpci_dma_init_device(zdev);
-	} else {
-		zdev->dma_table = zdev->s390_domain->dma_table;
-		zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
-				   virt_to_phys(zdev->dma_table));
-	}
 out_free:
 	kfree(domain_device);
 
@@ -157,30 +185,14 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
 {
 	struct s390_domain *s390_domain = to_s390_domain(domain);
 	struct zpci_dev *zdev = to_zpci_dev(dev);
-	struct s390_domain_device *domain_device, *tmp;
-	unsigned long flags;
-	int found = 0;
+	bool detached;
 
 	if (!zdev)
 		return;
 
-	spin_lock_irqsave(&s390_domain->list_lock, flags);
-	list_for_each_entry_safe(domain_device, tmp, &s390_domain->devices,
-				 list) {
-		if (domain_device->zdev == zdev) {
-			list_del(&domain_device->list);
-			kfree(domain_device);
-			found = 1;
-			break;
-		}
-	}
-	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
-
-	if (found && (zdev->s390_domain == s390_domain)) {
-		zdev->s390_domain = NULL;
-		zpci_unregister_ioat(zdev, 0);
+	detached = __s390_iommu_detach_device(s390_domain, zdev);
+	if (detached)
 		zpci_dma_init_device(zdev);
-	}
 }
 
 static struct iommu_device *s390_iommu_probe_device(struct device *dev)
-- 
2.34.1

On Thu, Sep 15, 2022 at 05:14:00PM +0200, Niklas Schnelle wrote:
> Since commit fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev
> calls") we can end up with duplicates in the list of devices attached to
> a domain. This is inefficient and confusing since only one domain can
> actually be in control of the IOMMU translations for a device. Fix this
> by detaching the device from the previous domain, if any, on attach.
> This also makes the restore behavior analogous between IOMMU and DMA API
> control.
> 
> Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
>  drivers/iommu/s390-iommu.c | 82 ++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index c898bcbbce11..de8f76775240 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -83,14 +83,41 @@ static void s390_domain_free(struct iommu_domain *domain)
>  	kfree(s390_domain);
>  }
>  
> +static bool __s390_iommu_detach_device(struct s390_domain *s390_domain,
> +				     struct zpci_dev *zdev)
> +{
> +	struct s390_domain_device *domain_device, *tmp;
> +	unsigned long flags;
> +	bool found = false;
> +
> +	spin_lock_irqsave(&s390_domain->list_lock, flags);
> +	list_for_each_entry_safe(domain_device, tmp, &s390_domain->devices,
> +				 list) {
> +		if (domain_device->zdev == zdev) {

Why all this searching? The domain argument is only being provided to
help drivers find their data structures, in most cases I would expect
it to be mostly unused.

After patch 3 the struct is gone, so isn't this just

 spin_lock_irqsave(&s390_domain->list_lock, flags);
 list_del_init(&zdev->iommu_list)
 spin_unlock_irqsave(&s390_domain->list_lock, flags);

?

Jason

On Tue, 2022-09-20 at 11:21 -0300, Jason Gunthorpe wrote:
> On Thu, Sep 15, 2022 at 05:14:00PM +0200, Niklas Schnelle wrote:
> > Since commit fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev
> > calls") we can end up with duplicates in the list of devices attached to
> > a domain. This is inefficient and confusing since only one domain can
> > actually be in control of the IOMMU translations for a device. Fix this
> > by detaching the device from the previous domain, if any, on attach.
> > This also makes the restore behavior analogous between IOMMU and DMA API
> > control.
> > 
> > Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> >  drivers/iommu/s390-iommu.c | 82 ++++++++++++++++++++++----------------
> >  1 file changed, 47 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> > index c898bcbbce11..de8f76775240 100644
> > --- a/drivers/iommu/s390-iommu.c
> > +++ b/drivers/iommu/s390-iommu.c
> > @@ -83,14 +83,41 @@ static void s390_domain_free(struct iommu_domain *domain)
> >  	kfree(s390_domain);
> >  }
> >  
> > +static bool __s390_iommu_detach_device(struct s390_domain *s390_domain,
> > +				     struct zpci_dev *zdev)
> > +{
> > +	struct s390_domain_device *domain_device, *tmp;
> > +	unsigned long flags;
> > +	bool found = false;
> > +
> > +	spin_lock_irqsave(&s390_domain->list_lock, flags);
> > +	list_for_each_entry_safe(domain_device, tmp, &s390_domain->devices,
> > +				 list) {
> > +		if (domain_device->zdev == zdev) {
> 
> Why all this searching? The domain argument is only being provided to
> help drivers find their data structures, in most cases I would expect
> it to be mostly unused.

Before patch 3 we have no other way besides searching in the list to
get from the struct device to the struct s390_domain_device that we
need to kfree(). But yeah as shown by patch 3 this whole
s390_domain_device thing is not needed anyway.

> 
> After patch 3 the struct is gone, so isn't this just
> 
>  spin_lock_irqsave(&s390_domain->list_lock, flags);
>  list_del_init(&zdev->iommu_list)
>  spin_unlock_irqsave(&s390_domain->list_lock, flags);
> 
> ?

Yes with patch 3 I think you're right, the above should be enough to
get it removed from the list and there really shouldn't be a call to
detach from a domain if it wasn't attached to it, right? Just to be
safe we could also do nothing if (zdev->s390_domain != s390_domain) or
maybe better just BUG_ON()?

One thing that is still a bit of a mismatch is that architecturally
zpci_unregister_ioat() can fail but detach returns void. Now, one
reason for that is a hot unplug of the device has occurred in the
meantime in which case the device doesn't use the DMA translations
anymore anyway.

If anything else happens I don't know what we should do,
zpci_dma_exit_device() in our DMA API implementation in these cases
leaks the DMA tables such that any hardware still accessing them would
get valid tables but I don't think I've ever seen this occurring.