Hi All,
This is v8 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. 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 applies on top of v6.1-rc2. It is also available as branch
iommu_fixes_v8 with the GPG signed tag s390_iommu_fixes_v8 on my niks/linux.git
on git.kernel.org[1].
Best regards,
Niklas
Changes since v7:
- Rebased on v6.1-rc2 adding gfp flags in iommu_alloc_resv_region()
- Added Jason's R-bs
Changes since v6:
- Rebased on top of v6.1-rc1 which now includes all prerequisites
- Added Matt's R-b
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 aperture 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/[email protected]/
[1] 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
While s390-iommu currently implements the map_page()/unmap_page()
operations which only map/unmap a single page at a time the internal
s390_iommu_update_trans() API already supports mapping/unmapping a range
of pages at once. Take advantage of this by implementing the
map_pages()/unmap_pages() operations instead thus allowing users of the
IOMMU drivers to map multiple pages in a single call followed by
a single I/O TLB flush if needed.
Reviewed-by: Matthew Rosato <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Niklas Schnelle <[email protected]>
---
v7->v8:
- Added Jason's R-b
drivers/iommu/s390-iommu.c | 48 +++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 104dfbec1037..7fb512bece9a 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -196,20 +196,15 @@ static void s390_iommu_release_device(struct device *dev)
static int s390_iommu_update_trans(struct s390_domain *s390_domain,
phys_addr_t pa, dma_addr_t dma_addr,
- size_t size, int flags)
+ unsigned long nr_pages, int flags)
{
phys_addr_t page_addr = pa & PAGE_MASK;
dma_addr_t start_dma_addr = dma_addr;
- unsigned long irq_flags, nr_pages, i;
+ unsigned long irq_flags, i;
struct zpci_dev *zdev;
unsigned long *entry;
int rc = 0;
- if (dma_addr < s390_domain->domain.geometry.aperture_start ||
- (dma_addr + size - 1) > s390_domain->domain.geometry.aperture_end)
- return -EINVAL;
-
- nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
if (!nr_pages)
return 0;
@@ -252,11 +247,24 @@ static int s390_iommu_update_trans(struct s390_domain *s390_domain,
return rc;
}
-static int s390_iommu_map(struct iommu_domain *domain, unsigned long iova,
- phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+static int s390_iommu_map_pages(struct iommu_domain *domain,
+ unsigned long iova, phys_addr_t paddr,
+ size_t pgsize, size_t pgcount,
+ int prot, gfp_t gfp, size_t *mapped)
{
struct s390_domain *s390_domain = to_s390_domain(domain);
int flags = ZPCI_PTE_VALID, rc = 0;
+ size_t size = pgcount << __ffs(pgsize);
+
+ if (pgsize != SZ_4K)
+ return -EINVAL;
+
+ if (iova < s390_domain->domain.geometry.aperture_start ||
+ (iova + size - 1) > s390_domain->domain.geometry.aperture_end)
+ return -EINVAL;
+
+ if (!IS_ALIGNED(iova | paddr, pgsize))
+ return -EINVAL;
if (!(prot & IOMMU_READ))
return -EINVAL;
@@ -265,7 +273,9 @@ static int s390_iommu_map(struct iommu_domain *domain, unsigned long iova,
flags |= ZPCI_TABLE_PROTECTED;
rc = s390_iommu_update_trans(s390_domain, paddr, iova,
- size, flags);
+ pgcount, flags);
+ if (!rc)
+ *mapped = size;
return rc;
}
@@ -301,21 +311,27 @@ static phys_addr_t s390_iommu_iova_to_phys(struct iommu_domain *domain,
return phys;
}
-static size_t s390_iommu_unmap(struct iommu_domain *domain,
- unsigned long iova, size_t size,
- struct iommu_iotlb_gather *gather)
+static size_t s390_iommu_unmap_pages(struct iommu_domain *domain,
+ unsigned long iova,
+ size_t pgsize, size_t pgcount,
+ struct iommu_iotlb_gather *gather)
{
struct s390_domain *s390_domain = to_s390_domain(domain);
+ size_t size = pgcount << __ffs(pgsize);
int flags = ZPCI_PTE_INVALID;
phys_addr_t paddr;
int rc;
+ if (WARN_ON(iova < s390_domain->domain.geometry.aperture_start ||
+ (iova + size - 1) > s390_domain->domain.geometry.aperture_end))
+ return 0;
+
paddr = s390_iommu_iova_to_phys(domain, iova);
if (!paddr)
return 0;
rc = s390_iommu_update_trans(s390_domain, paddr, iova,
- size, flags);
+ pgcount, flags);
if (rc)
return 0;
@@ -361,8 +377,8 @@ static const struct iommu_ops s390_iommu_ops = {
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = s390_iommu_attach_device,
.detach_dev = s390_iommu_detach_device,
- .map = s390_iommu_map,
- .unmap = s390_iommu_unmap,
+ .map_pages = s390_iommu_map_pages,
+ .unmap_pages = s390_iommu_unmap_pages,
.iova_to_phys = s390_iommu_iova_to_phys,
.free = s390_domain_free,
}
--
2.34.1
The .pgsize_bitmap property of struct iommu_ops is not a page mask but
rather has a bit set for each size of pages the IOMMU supports. As the
comment correctly pointed out at this moment the code only support 4K
pages so simply use SZ_4K here.
Reviewed-by: Matthew Rosato <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Niklas Schnelle <[email protected]>
---
drivers/iommu/s390-iommu.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 3e601ca6ee0f..104dfbec1037 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -12,13 +12,6 @@
#include <linux/sizes.h>
#include <asm/pci_dma.h>
-/*
- * Physically contiguous memory regions can be mapped with 4 KiB alignment,
- * we allow all page sizes that are an order of 4KiB (no special large page
- * support so far).
- */
-#define S390_IOMMU_PGSIZES (~0xFFFUL)
-
static const struct iommu_ops s390_iommu_ops;
struct s390_domain {
@@ -363,7 +356,7 @@ static const struct iommu_ops s390_iommu_ops = {
.probe_device = s390_iommu_probe_device,
.release_device = s390_iommu_release_device,
.device_group = generic_device_group,
- .pgsize_bitmap = S390_IOMMU_PGSIZES,
+ .pgsize_bitmap = SZ_4K,
.get_resv_regions = s390_iommu_get_resv_regions,
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = s390_iommu_attach_device,
--
2.34.1
On Tue, Oct 25, 2022 at 01:56:51PM +0200, Niklas Schnelle wrote:
> 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()
Applied, thanks.