Hi All,

This is v5 of a follow up to Matt's recent series[0] where he tackled
a race that turned out to be outside of the s390 IOMMU driver itself as
well as duplicate device attachments. After an internal discussion we came
up with what I believe is a cleaner fix. Instead of actively checking for
duplicates we instead detach from any previous domain on attach. From my
cursory reading of the code this seems to be what the Intel IOMMU driver is
doing as well.

Moreover we drop the attempt to re-attach the device to its previous IOMMU
domain on failure. This was fragile, unlikely to help and unexpected for
calling code. Thanks Jason for the suggestion.

We can also get rid of struct s390_domain_device entirely if we instead
thread the list through the attached struct zpci_devs. This saves us from
having to allocate during attach and gets rid of one level of indirection
during IOMMU operations.

Additionally 3 more fixes have been added in v3 that weren't in v2 of this
series. One is for a potential situation where the aperture of a domain
could shrink and leave invalid translations. The next one fixes an off by
one in checking validity of an IOVA and the last one fixes a wrong value
for pgsize_bitmap.

In v4 we also add a patch changing to the map_pages()/unmap_pages()
interface in order to prevent a performance regression due to the
pgsize_bitmap change.

*Note*:
This series is against the s390 features branch[1] which already contains
the bus_next field removal that was part of v2.

It is also available as branch iommu_fixes_v6 with the GPG signed tag
s390_iommu_fixes_v5 on my niks/linux.git on git.kernel.org[2].

*Open Question*:
Which tree should this go via?

Best regards,
Niklas

Changes since v5:
- Only set zdev->dma_table once zpci_register_ioat() has succeeded like
  we correctly did in v4 (Matt)
- In patch 3 WARN_ON() aperture violation in .unmap_pages (Matt)
- In patch 3 return after WARN_ON() check for apterute in attach
Changes since v4:
- Add patch to change to the map_pages()/unmap_pages() API to prevent
  a performance regression from the pgsize_bitmap change (Robin)
- In patch 1 unregister IOAT on error (Matt)
- Turn the aperture check in attach into a WARN_ON() in patch 3 (Jason)

Changes since v3:
- 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)
- Make aperture check resistant against overflow (Jason)

Changes since v2:
- The patch removing the unused bus_next field has been spun out and
  already made it into the s390 feature branch on git.kernel.org
- Make __s390_iommu_detach_device() return void (Jason)
- Remove the re-attach on failure dance as it is unlikely to help
  and complicates debug and recovery (Jason)
- Ignore attempts to detach from domain that is not the active one
- Add patch to fix potential shrinking of the aperture and use
  reserved ranges per device instead of the aperture to respect
  IOVA range restrictions (Jason)
- Add a fix for an off by one error on checking an IOVA against
  the aperture
- Add a fix for wrong pgsize_bitmap

Changes since v1:
- After patch 3 we don't have to search in the devices list on detach as
  we alreadz have hold of the zpci_dev (Jason)
- Add a WARN_ON() if somehow ended up detaching a device from a domain that
  isn't the device's current domain.
- Removed the iteration and list delete from s390_domain_free() instead
  just WARN_ON() when we're freeing without having detached
- The last two points should help catching sequencing errors much more
  quickly in the future.

[0] https://lore.kernel.org/linux-iommu/20220831201236.77595-1-mjrosato@linux.ibm.com/
[1] https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git

Niklas Schnelle (6):
  iommu/s390: Fix duplicate domain attachments
  iommu/s390: Get rid of s390_domain_device
  iommu/s390: Fix potential s390_domain aperture shrinking
  iommu/s390: Fix incorrect aperture check
  iommu/s390: Fix incorrect pgsize_bitmap
  iommu/s390: Implement map_pages()/unmap_pages() instead of
    map()/unmap()

 arch/s390/include/asm/pci.h |   1 +
 drivers/iommu/s390-iommu.c  | 223 +++++++++++++++++-------------------
 2 files changed, 109 insertions(+), 115 deletions(-)

-- 
2.34.1

On Fri, 2022-10-07 at 11:49 +0200, Niklas Schnelle wrote:
> Hi All,
> 
> This is v5 of a follow up to Matt's recent series[0] where he tackled
> a race that turned out to be outside of the s390 IOMMU driver itself as
> well as duplicate device attachments. After an internal discussion we came
> up with what I believe is a cleaner fix. Instead of actively checking for
> duplicates we instead detach from any previous domain on attach. From my
> cursory reading of the code this seems to be what the Intel IOMMU driver is
> doing as well.
> 
> Moreover we drop the attempt to re-attach the device to its previous IOMMU
> domain on failure. This was fragile, unlikely to help and unexpected for
> calling code. Thanks Jason for the suggestion.
> 
> We can also get rid of struct s390_domain_device entirely if we instead
> thread the list through the attached struct zpci_devs. This saves us from
> having to allocate during attach and gets rid of one level of indirection
> during IOMMU operations.
> 
> Additionally 3 more fixes have been added in v3 that weren't in v2 of this
> series. One is for a potential situation where the aperture of a domain
> could shrink and leave invalid translations. The next one fixes an off by
> one in checking validity of an IOVA and the last one fixes a wrong value
> for pgsize_bitmap.
> 
> In v4 we also add a patch changing to the map_pages()/unmap_pages()
> interface in order to prevent a performance regression due to the
> pgsize_bitmap change.
> 
> *Note*:
> This series is against the s390 features branch[1] which already contains
> the bus_next field removal that was part of v2.
> 
> It is also available as branch iommu_fixes_v6 with the GPG signed tag
> s390_iommu_fixes_v5 on my niks/linux.git on git.kernel.org[2].
> 
> *Open Question*:
> Which tree should this go via?

The conflicting commit that removed the bus_next field from struct
zpci_dev has now made it into Linus' tree via the s390 pull. So this
series now applies cleanly on mainline master. Still not sure though
which tree this would best go into.

> 
> Best regards,
> Niklas
> 
> 
---8<---

On Mon, Oct 10, 2022 at 04:54:07PM +0200, Niklas Schnelle wrote:
> On Fri, 2022-10-07 at 11:49 +0200, Niklas Schnelle wrote:
> > Hi All,
> > 
> > This is v5 of a follow up to Matt's recent series[0] where he tackled
> > a race that turned out to be outside of the s390 IOMMU driver itself as
> > well as duplicate device attachments. After an internal discussion we came
> > up with what I believe is a cleaner fix. Instead of actively checking for
> > duplicates we instead detach from any previous domain on attach. From my
> > cursory reading of the code this seems to be what the Intel IOMMU driver is
> > doing as well.
> > 
> > Moreover we drop the attempt to re-attach the device to its previous IOMMU
> > domain on failure. This was fragile, unlikely to help and unexpected for
> > calling code. Thanks Jason for the suggestion.
> > 
> > We can also get rid of struct s390_domain_device entirely if we instead
> > thread the list through the attached struct zpci_devs. This saves us from
> > having to allocate during attach and gets rid of one level of indirection
> > during IOMMU operations.
> > 
> > Additionally 3 more fixes have been added in v3 that weren't in v2 of this
> > series. One is for a potential situation where the aperture of a domain
> > could shrink and leave invalid translations. The next one fixes an off by
> > one in checking validity of an IOVA and the last one fixes a wrong value
> > for pgsize_bitmap.
> > 
> > In v4 we also add a patch changing to the map_pages()/unmap_pages()
> > interface in order to prevent a performance regression due to the
> > pgsize_bitmap change.
> > 
> > *Note*:
> > This series is against the s390 features branch[1] which already contains
> > the bus_next field removal that was part of v2.
> > 
> > It is also available as branch iommu_fixes_v6 with the GPG signed tag
> > s390_iommu_fixes_v5 on my niks/linux.git on git.kernel.org[2].
> > 
> > *Open Question*:
> > Which tree should this go via?
> 
> The conflicting commit that removed the bus_next field from struct
> zpci_dev has now made it into Linus' tree via the s390 pull. So this
> series now applies cleanly on mainline master. Still not sure though
> which tree this would best go into.

Arguably it should go through Joerg's iommu tree since it is only in
the iommu driver..

If you need it on a branch to share with the s390 tree then send Joerg
a PR.

Jason

On Mon, 2022-10-10 at 15:45 -0300, Jason Gunthorpe wrote:
> On Mon, Oct 10, 2022 at 04:54:07PM +0200, Niklas Schnelle wrote:
> > On Fri, 2022-10-07 at 11:49 +0200, Niklas Schnelle wrote:
> > > Hi All,
> > > 
> > > This is v5 of a follow up to Matt's recent series[0] where he tackled
> > > a race that turned out to be outside of the s390 IOMMU driver itself as
> > > well as duplicate device attachments. After an internal discussion we came
> > > up with what I believe is a cleaner fix. Instead of actively checking for
> > > duplicates we instead detach from any previous domain on attach. From my
> > > cursory reading of the code this seems to be what the Intel IOMMU driver is
> > > doing as well.
> > > 
> > > Moreover we drop the attempt to re-attach the device to its previous IOMMU
> > > domain on failure. This was fragile, unlikely to help and unexpected for
> > > calling code. Thanks Jason for the suggestion.
> > > 
> > > We can also get rid of struct s390_domain_device entirely if we instead
> > > thread the list through the attached struct zpci_devs. This saves us from
> > > having to allocate during attach and gets rid of one level of indirection
> > > during IOMMU operations.
> > > 
> > > Additionally 3 more fixes have been added in v3 that weren't in v2 of this
> > > series. One is for a potential situation where the aperture of a domain
> > > could shrink and leave invalid translations. The next one fixes an off by
> > > one in checking validity of an IOVA and the last one fixes a wrong value
> > > for pgsize_bitmap.
> > > 
> > > In v4 we also add a patch changing to the map_pages()/unmap_pages()
> > > interface in order to prevent a performance regression due to the
> > > pgsize_bitmap change.
> > > 
> > > *Note*:
> > > This series is against the s390 features branch[1] which already contains
> > > the bus_next field removal that was part of v2.
> > > 
> > > It is also available as branch iommu_fixes_v6 with the GPG signed tag
> > > s390_iommu_fixes_v5 on my niks/linux.git on git.kernel.org[2].
> > > 
> > > *Open Question*:
> > > Which tree should this go via?
> > 
> > The conflicting commit that removed the bus_next field from struct
> > zpci_dev has now made it into Linus' tree via the s390 pull. So this
> > series now applies cleanly on mainline master. Still not sure though
> > which tree this would best go into.
> 
> Arguably it should go through Joerg's iommu tree since it is only in
> the iommu driver..
> 
> If you need it on a branch to share with the s390 tree then send Joerg
> a PR.
> 
> Jason

Ok makes sense, I don't think I need it on an extra branch and whatever
is easiest for Joerg is fine. I hope that since all but patch 6 are
fixes and that one is quite simple that this could maybe still go into
v6.1.

Not sure if Joerg is still waiting on some Acks or R-bs though. I did
remove yours on patches 1, 2,  and 3 as there were some changes since
you gave it. I don't think you gave one for patch 4 and patch 6 is new.

I plan on sending further IOMMU improvements and the DMA conversion
based on this but will just reference it and provide private branches
on git.kernel.org. I think those will target the next merge window at
the earliest so that should be fine.

Thanks,
Niklas

On Tue, Oct 11, 2022 at 01:03:27PM +0200, Niklas Schnelle wrote:
> Ok makes sense, I don't think I need it on an extra branch and whatever
> is easiest for Joerg is fine. I hope that since all but patch 6 are
> fixes and that one is quite simple that this could maybe still go into
> v6.1.

Oh there is no way for 6.1 at this point.

You will have to respin it for 6.1-rc1 and Joerg usually waits until
rc3 before taking any patches.

But I would post and origanize all the things you want for 6.1 next
week.

Jason