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: <92e00af653b6c84bfbccd60651d106a73e54887b.camel@linux.ibm.com>
Subject: Re: [PATCH 1/3] iommu/s390: Fix duplicate domain attachments
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     Matthew Rosato <mjrosato@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev,
        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:   Tue, 20 Sep 2022 17:22:52 +0200
In-Reply-To: <YynMUNCrGCzaFPjI@nvidia.com>
References: <20220915151402.121032-1-schnelle@linux.ibm.com>
         <20220915151402.121032-2-schnelle@linux.ibm.com>
         <YynMUNCrGCzaFPjI@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.