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: <2a56d7bc-3beb-51fe-fc65-f2f3bd192e34@linux.ibm.com>
Date:   Tue, 4 Oct 2022 12:18:05 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH v4 1/5] iommu/s390: Fix duplicate domain attachments
Content-Language: en-US
To:     Niklas Schnelle <schnelle@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev
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, jgg@nvidia.com, linux-kernel@vger.kernel.org
References: <20221004120706.2957492-1-schnelle@linux.ibm.com>
 <20221004120706.2957492-2-schnelle@linux.ibm.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <20221004120706.2957492-2-schnelle@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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?


>  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,