Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <dd92817a13601a4a449096b7ae94caa66a113853.camel@linux.ibm.com>
Subject: Re: [PATCH v4 1/5] iommu/s390: Fix duplicate domain attachments
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Matthew Rosato <mjrosato@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev,
        Jason Gunthorpe <jgg@nvidia.com>
Cc:     linux-s390@vger.kernel.org, borntraeger@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, joro@8bytes.org, will@kernel.org,
        robin.murphy@arm.com, linux-kernel@vger.kernel.org
Date:   Wed, 05 Oct 2022 09:58:58 +0200
In-Reply-To: <2a56d7bc-3beb-51fe-fc65-f2f3bd192e34@linux.ibm.com>
References: <20221004120706.2957492-1-schnelle@linux.ibm.com>
         <20221004120706.2957492-2-schnelle@linux.ibm.com>
         <2a56d7bc-3beb-51fe-fc65-f2f3bd192e34@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-10-04 at 12:18 -0400, Matthew Rosato wrote:
> On 10/4/22 8:07 AM, 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.
> > Add a WARN_ON() in case we still have attached devices on freeing the
> > domain. While here remove the re-attach on failure dance as it was
> > determined to be unlikely to help and may confuse debug and recovery.
> > 
> > Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> I've been testing this in isolation and it looks good to me, but one question...
> 
> > ---
> > v3 -> v4:
> > - Drop s390_domain from __s390_iommu_detach_device() (Jason)
> > - WARN_ON() mismatched domain in s390_iommu_detach_device() (Jason)
> > - Use __s390_iommu_detach_device() in s390_iommu_release_device() (Jason)
> > 
> >  drivers/iommu/s390-iommu.c | 97 +++++++++++++++-----------------------
> >  1 file changed, 39 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> > index c898bcbbce11..0f58e897bc95 100644
> > --- a/drivers/iommu/s390-iommu.c
> > +++ b/drivers/iommu/s390-iommu.c
> > @@ -79,10 +79,36 @@ static void s390_domain_free(struct iommu_domain *domain)
> >  {
> >  	struct s390_domain *s390_domain = to_s390_domain(domain);
> >  
> > +	WARN_ON(!list_empty(&s390_domain->devices));
> >  	dma_cleanup_tables(s390_domain->dma_table);
> >  	kfree(s390_domain);
> >  }
> >  
> > +static void __s390_iommu_detach_device(struct zpci_dev *zdev)
> > +{
> > +	struct s390_domain *s390_domain = zdev->s390_domain;
> > +	struct s390_domain_device *domain_device, *tmp;
> > +	unsigned long flags;
> > +
> > +	if (!s390_domain)
> > +		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);
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&s390_domain->list_lock, flags);
> > +
> > +	zpci_unregister_ioat(zdev, 0);
> > +	zdev->s390_domain = NULL;
> > +	zdev->dma_table = NULL;
> > +}
> > +
> >  static int s390_iommu_attach_device(struct iommu_domain *domain,
> >  				    struct device *dev)
> >  {
> > @@ -90,7 +116,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
> >  	struct zpci_dev *zdev = to_zpci_dev(dev);
> >  	struct s390_domain_device *domain_device;
> >  	unsigned long flags;
> > -	int cc, rc;
> > +	int cc, rc = 0;
> >  
> >  	if (!zdev)
> >  		return -ENODEV;
> > @@ -99,23 +125,17 @@ 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) {
> > -			rc = -EIO;
> > -			goto out_free;
> > -		}
> > -	}
> > -
> >  	if (zdev->s390_domain)
> > -		zpci_unregister_ioat(zdev, 0);
> > +		__s390_iommu_detach_device(zdev);
> > +	else if (zdev->dma_table)
> > +		zpci_dma_exit_device(zdev);
> >  
> >  	zdev->dma_table = s390_domain->dma_table;
> >  	cc = zpci_register_ioat(zdev, 0, zdev->start_dma, zdev->end_dma,
> >  				virt_to_phys(zdev->dma_table));
> >  	if (cc) {
> >  		rc = -EIO;
> > -		goto out_restore;
> > +		goto out_free;
> >  	}
> >  
> >  	spin_lock_irqsave(&s390_domain->list_lock, flags);
> > @@ -129,7 +149,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_free;
> >  	}
> >  	domain_device->zdev = zdev;
> >  	zdev->s390_domain = s390_domain;
> > @@ -138,14 +158,6 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
> >  
> >  	return 0;
> >  
> > -out_restore:
> > -	if (!zdev->s390_domain) {
> > -		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));
> > -	}
> 
> ^ I see you removed this awkward backout scenario (and replace the aperture check later) and I generally agree, but I'm looking at just this patch in isolation since its a fix...
> If we leave due to a failed register_ioat or aperture mismatch, what do we expect to happen moving forward?  In one case (aperture mismatch -- how?) something is left registered with firmware and another (register_ioat fails) we have nothing registered with firmware (as we've discussed for, then the device is probably in an error state).  Is the expectation that the device is just broken for now and, more importantly, will device recovery clean both of these scenarios up?

A failed aperture test leaving the IOAT registered would indeed be bad.
I guess I focused too much on the failure scenarios at the state after
these patches where this can't happen. I think this would leave us in a
bad state because zpci_register_ioat() succeeded with the domain's DMA
table but we won't have attached leading to the wrong decisions in
recovery paths (see below). 

I think we should do a zpci_unregister_ioat() and zdev->dma_table =
NULL in this case just to be safe. It's certainly still much less
fragile than the full rollback and even if the zpci_unregister_ioat()
fails it prevents recovery from restoring the wrong DMA translation
tables. I don't think we can really get into this situation though as
the aperture should match what firmware accepts but it's still a valid
code path.

@Jason would you be okay with that?

Recovery (via zpci_hot_reset_device()) should then be able to deal with
these situations as long as zdev->dma_table matches the IOAT
registration state.

1. If zdev->dma_table != NULL we re-register the previous DMA table
2. If zdev->dma_table == NULL we do zpci_dma_init_device()

> 
> 
> >  out_free:
> >  	kfree(domain_device);
> >  
> > @@ -155,32 +167,12 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
> >  static void s390_iommu_detach_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, *tmp;
> > -	unsigned long flags;
> > -	int found = 0;
> >  
> > -	if (!zdev)
> > -		return;
> > +	WARN_ON(zdev->s390_domain != to_s390_domain(domain));
> >  
> > -	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);
> > -		zpci_dma_init_device(zdev);
> > -	}
> > +	__s390_iommu_detach_device(zdev);
> > +	zpci_dma_init_device(zdev);
> >  }
> >  
> >  static struct iommu_device *s390_iommu_probe_device(struct device *dev)
> > @@ -193,24 +185,13 @@ static struct iommu_device *s390_iommu_probe_device(struct device *dev)
> >  static void s390_iommu_release_device(struct device *dev)
> >  {
> >  	struct zpci_dev *zdev = to_zpci_dev(dev);
> > -	struct iommu_domain *domain;
> >  
> >  	/*
> > -	 * This is a workaround for a scenario where the IOMMU API common code
> > -	 * "forgets" to call the detach_dev callback: After binding a device
> > -	 * to vfio-pci and completing the VFIO_SET_IOMMU ioctl (which triggers
> > -	 * the attach_dev), removing the device via
> > -	 * "echo 1 > /sys/bus/pci/devices/.../remove" won't trigger detach_dev,
> > -	 * only release_device will be called via the BUS_NOTIFY_REMOVED_DEVICE
> > -	 * notifier.
> > -	 *
> > -	 * So let's call detach_dev from here if it hasn't been called before.
> > +	 * release_device is expected to detach any domain currently attached
> > +	 * to the device, but keep it attached to other devices in the group.
> >  	 */
> > -	if (zdev && zdev->s390_domain) {
> > -		domain = iommu_get_domain_for_dev(dev);
> > -		if (domain)
> > -			s390_iommu_detach_device(domain, dev);
> > -	}
> > +	if (zdev)
> > +		__s390_iommu_detach_device(zdev);
> >  }
> >  
> >  static int s390_iommu_update_trans(struct s390_domain *s390_domain,