LinuxLists.cc - [PATCH v2 00/24] iommu: Refactor DMA domain strictness

[permalink] [raw]

Subject: [PATCH v2 01/24] iommu: Pull IOVA cookie management into the core

Now that everyone has converged on iommu-dma for IOMMU_DOMAIN_DMA
support, we can abandon the notion of drivers being responsible for the
cookie type, and consolidate all the management into the core code.

CC: Marek Szyprowski <[email protected]>
CC: Yoshihiro Shimoda <[email protected]>
CC: Geert Uytterhoeven <[email protected]>
CC: Yong Wu <[email protected]>
CC: Heiko Stuebner <[email protected]>
CC: Chunyan Zhang <[email protected]>
CC: Chunyan Zhang <[email protected]>
CC: Maxime Ripard <[email protected]>
CC: Jean-Philippe Brucker <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommu.c | 7 +++++++
include/linux/iommu.h | 3 ++-
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2cda9950bd5..ea5a9ea8d431 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -7,6 +7,7 @@
#define pr_fmt(fmt) "iommu: " fmt

#include <linux/device.h>
+#include <linux/dma-iommu.h>
#include <linux/kernel.h>
#include <linux/bits.h>
#include <linux/bug.h>
@@ -1946,6 +1947,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
/* Assume all sizes by default; the driver may override this later */
domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;

+ /* Temporarily ignore -EEXIST while drivers still get their own cookies */
+ if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain) == -ENOMEM) {
+ iommu_domain_free(domain);
+ domain = NULL;
+ }
return domain;
}

@@ -1957,6 +1963,7 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);

void iommu_domain_free(struct iommu_domain *domain)
{
+ iommu_put_dma_cookie(domain);
domain->ops->domain_free(domain);
}
EXPORT_SYMBOL_GPL(iommu_domain_free);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4997c78e2670..141779d76035 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -40,6 +40,7 @@ struct iommu_domain;
struct notifier_block;
struct iommu_sva;
struct iommu_fault_event;
+struct iommu_dma_cookie;

/* iommu fault flags */
#define IOMMU_FAULT_READ 0x0
@@ -86,7 +87,7 @@ struct iommu_domain {
iommu_fault_handler_t handler;
void *handler_token;
struct iommu_domain_geometry geometry;
- void *iova_cookie;
+ struct iommu_dma_cookie *iova_cookie;
};

enum iommu_cap {
--
2.25.1

2021-07-28 16:00:54

[permalink] [raw]

Subject: [PATCH v2 08/24] iommu/rockchip: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Heiko Stuebner <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/rockchip-iommu.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9febfb7f3025..c24561f54f32 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1074,10 +1074,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
if (!rk_domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&rk_domain->domain))
- goto err_free_domain;
-
/*
* rk32xx iommus use a 2 level pagetable.
* Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
@@ -1085,7 +1081,7 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)
*/
rk_domain->dt = (u32 *)get_zeroed_page(GFP_KERNEL | GFP_DMA32);
if (!rk_domain->dt)
- goto err_put_cookie;
+ goto err_free_domain;

rk_domain->dt_dma = dma_map_single(dma_dev, rk_domain->dt,
SPAGE_SIZE, DMA_TO_DEVICE);
@@ -1106,9 +1102,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type)

err_free_dt:
free_page((unsigned long)rk_domain->dt);
-err_put_cookie:
- if (type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&rk_domain->domain);
err_free_domain:
kfree(rk_domain);

@@ -1137,8 +1130,6 @@ static void rk_iommu_domain_free(struct iommu_domain *domain)
SPAGE_SIZE, DMA_TO_DEVICE);
free_page((unsigned long)rk_domain->dt);

- if (domain->type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&rk_domain->domain);
kfree(rk_domain);
}

--
2.25.1

2021-07-28 16:01:07

[permalink] [raw]

Subject: [PATCH v2 11/24] iommu/virtio: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Jean-Philippe Brucker <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/virtio-iommu.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 6abdcab7273b..80930ce04a16 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -598,12 +598,6 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type)
spin_lock_init(&vdomain->mappings_lock);
vdomain->mappings = RB_ROOT_CACHED;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&vdomain->domain)) {
- kfree(vdomain);
- return NULL;
- }
-
return &vdomain->domain;
}

@@ -643,8 +637,6 @@ static void viommu_domain_free(struct iommu_domain *domain)
{
struct viommu_domain *vdomain = to_viommu_domain(domain);

- iommu_put_dma_cookie(domain);
-
/* Free all remaining mappings (size 2^64) */
viommu_del_mappings(vdomain, 0, 0);

--
2.25.1

2021-07-28 16:01:07

[permalink] [raw]

Subject: [PATCH v2 04/24] iommu/vt-d: Drop IOVA cookie management

The core code bakes its own cookies now.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/intel/iommu.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index c12cc955389a..7e168634c433 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1979,10 +1979,6 @@ static void domain_exit(struct dmar_domain *domain)
/* Remove associated devices and clear attached or cached domains */
domain_remove_dev_info(domain);

- /* destroy iovas */
- if (domain->domain.type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&domain->domain);
-
if (domain->pgd) {
struct page *freelist;

@@ -4544,10 +4540,6 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
return NULL;
}

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&dmar_domain->domain))
- return NULL;
-
domain = &dmar_domain->domain;
domain->geometry.aperture_start = 0;
domain->geometry.aperture_end =
--
2.25.1

2021-07-28 16:01:09

[permalink] [raw]

Subject: [PATCH v2 03/24] iommu/arm-smmu: Drop IOVA cookie management

The core code bakes its own cookies now.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 -------
drivers/iommu/arm/arm-smmu/arm-smmu.c | 15 ++++-----------
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --------
3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6346f21726f4..4c648da447bf 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1984,12 +1984,6 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
if (!smmu_domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&smmu_domain->domain)) {
- kfree(smmu_domain);
- return NULL;
- }
-
mutex_init(&smmu_domain->init_mutex);
INIT_LIST_HEAD(&smmu_domain->devices);
spin_lock_init(&smmu_domain->devices_lock);
@@ -2021,7 +2015,6 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
struct arm_smmu_device *smmu = smmu_domain->smmu;

- iommu_put_dma_cookie(domain);
free_io_pgtable_ops(smmu_domain->pgtbl_ops);

/* Free the CD and ASID, if we allocated them */
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index ac21170fa208..970d9e4dcd69 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -868,10 +868,10 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;

- if (type != IOMMU_DOMAIN_UNMANAGED &&
- type != IOMMU_DOMAIN_DMA &&
- type != IOMMU_DOMAIN_IDENTITY)
- return NULL;
+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
+ if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
+ return NULL;
+ }
/*
* Allocate the domain and initialise some of its data structures.
* We can't really do anything meaningful until we've added a
@@ -881,12 +881,6 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
if (!smmu_domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA && (using_legacy_binding ||
- iommu_get_dma_cookie(&smmu_domain->domain))) {
- kfree(smmu_domain);
- return NULL;
- }
-
mutex_init(&smmu_domain->init_mutex);
spin_lock_init(&smmu_domain->cb_lock);

@@ -901,7 +895,6 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
* Free the domain resources. We assume that all devices have
* already been detached.
*/
- iommu_put_dma_cookie(domain);
arm_smmu_destroy_domain_context(domain);
kfree(smmu_domain);
}
diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 021cf8f65ffc..4b7eca5f5148 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -335,12 +335,6 @@ static struct iommu_domain *qcom_iommu_domain_alloc(unsigned type)
if (!qcom_domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&qcom_domain->domain)) {
- kfree(qcom_domain);
- return NULL;
- }
-
mutex_init(&qcom_domain->init_mutex);
spin_lock_init(&qcom_domain->pgtbl_lock);

@@ -351,8 +345,6 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)
{
struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);

- iommu_put_dma_cookie(domain);
-
if (qcom_domain->iommu) {
/*
* NOTE: unmap can be called after client device is powered
--
2.25.1

2021-07-28 16:01:10

[permalink] [raw]

Subject: [PATCH v2 05/24] iommu/exynos: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Marek Szyprowski <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/exynos-iommu.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index d0fbf1d10e18..34085d069cda 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -735,20 +735,16 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
/* Check if correct PTE offsets are initialized */
BUG_ON(PG_ENT_SHIFT < 0 || !dma_dev);

+ if (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_UNMANAGED)
+ return NULL;
+
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA) {
- if (iommu_get_dma_cookie(&domain->domain) != 0)
- goto err_pgtable;
- } else if (type != IOMMU_DOMAIN_UNMANAGED) {
- goto err_pgtable;
- }
-
domain->pgtable = (sysmmu_pte_t *)__get_free_pages(GFP_KERNEL, 2);
if (!domain->pgtable)
- goto err_dma_cookie;
+ goto err_pgtable;

domain->lv2entcnt = (short *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 1);
if (!domain->lv2entcnt)
@@ -779,9 +775,6 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
free_pages((unsigned long)domain->lv2entcnt, 1);
err_counter:
free_pages((unsigned long)domain->pgtable, 2);
-err_dma_cookie:
- if (type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&domain->domain);
err_pgtable:
kfree(domain);
return NULL;
@@ -809,9 +802,6 @@ static void exynos_iommu_domain_free(struct iommu_domain *iommu_domain)

spin_unlock_irqrestore(&domain->lock, flags);

- if (iommu_domain->type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(iommu_domain);
-
dma_unmap_single(dma_dev, virt_to_phys(domain->pgtable), LV1TABLE_SIZE,
DMA_TO_DEVICE);

--
2.25.1

2021-07-28 16:01:11

[permalink] [raw]

Subject: [PATCH v2 02/24] iommu/amd: Drop IOVA cookie management

The core code bakes its own cookies now.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/amd/iommu.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 52fe2326042a..0fd98d35d73b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1918,16 +1918,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
domain->domain.geometry.aperture_end = ~0ULL;
domain->domain.geometry.force_aperture = true;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&domain->domain) == -ENOMEM)
- goto free_domain;
-
return &domain->domain;
-
-free_domain:
- protection_domain_free(domain);
-
- return NULL;
}

static void amd_iommu_domain_free(struct iommu_domain *dom)
@@ -1944,9 +1935,6 @@ static void amd_iommu_domain_free(struct iommu_domain *dom)
if (!dom)
return;

- if (dom->type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&domain->domain);
-
if (domain->flags & PD_IOMMUV2_MASK)
free_gcr3_table(domain);

--
2.25.1

2021-07-28 16:01:12

[permalink] [raw]

Subject: [PATCH v2 06/24] iommu/ipmmu-vmsa: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Yoshihiro Shimoda <[email protected]>
CC: Geert Uytterhoeven <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/ipmmu-vmsa.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 51ea6f00db2f..31252268f0d0 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -564,10 +564,13 @@ static irqreturn_t ipmmu_irq(int irq, void *dev)
* IOMMU Operations
*/

-static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
+static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
{
struct ipmmu_vmsa_domain *domain;

+ if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+ return NULL;
+
domain = kzalloc(sizeof(*domain), GFP_KERNEL);
if (!domain)
return NULL;
@@ -577,27 +580,6 @@ static struct iommu_domain *__ipmmu_domain_alloc(unsigned type)
return &domain->io_domain;
}

-static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
-{
- struct iommu_domain *io_domain = NULL;
-
- switch (type) {
- case IOMMU_DOMAIN_UNMANAGED:
- io_domain = __ipmmu_domain_alloc(type);
- break;
-
- case IOMMU_DOMAIN_DMA:
- io_domain = __ipmmu_domain_alloc(type);
- if (io_domain && iommu_get_dma_cookie(io_domain)) {
- kfree(io_domain);
- io_domain = NULL;
- }
- break;
- }
-
- return io_domain;
-}
-
static void ipmmu_domain_free(struct iommu_domain *io_domain)
{
struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
@@ -606,7 +588,6 @@ static void ipmmu_domain_free(struct iommu_domain *io_domain)
* Free the domain resources. We assume that all devices have already
* been detached.
*/
- iommu_put_dma_cookie(io_domain);
ipmmu_domain_destroy_context(domain);
free_io_pgtable_ops(domain->iop);
kfree(domain);
--
2.25.1

2021-07-28 16:01:21

[permalink] [raw]

Subject: [PATCH v2 15/24] iommu/amd: Prepare for multiple DMA domain types

The DMA ops reset/setup can simply be unconditional, since
iommu-dma already knows only to touch DMA domains.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/amd/iommu.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 0fd98d35d73b..02f9b4fffe90 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1707,14 +1707,9 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)

static void amd_iommu_probe_finalize(struct device *dev)
{
- struct iommu_domain *domain;
-
/* Domains are initialized for this device - have a look what we ended up with */
- domain = iommu_get_domain_for_dev(dev);
- if (domain->type == IOMMU_DOMAIN_DMA)
- iommu_setup_dma_ops(dev, 0, U64_MAX);
- else
- set_dma_ops(dev, NULL);
+ set_dma_ops(dev, NULL);
+ iommu_setup_dma_ops(dev, 0, U64_MAX);
}

static void amd_iommu_release_device(struct device *dev)
--
2.25.1

2021-07-28 16:01:23

[permalink] [raw]

Subject: [PATCH v2 07/24] iommu/mtk: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Yong Wu <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/mtk_iommu.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 6f7c69688ce2..e39a6d1da28d 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -441,17 +441,11 @@ static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
if (!dom)
return NULL;

- if (iommu_get_dma_cookie(&dom->domain)) {
- kfree(dom);
- return NULL;
- }
-
return &dom->domain;
}

static void mtk_iommu_domain_free(struct iommu_domain *domain)
{
- iommu_put_dma_cookie(domain);
kfree(to_mtk_domain(domain));
}

--
2.25.1

2021-07-28 16:01:40

[permalink] [raw]

Subject: [PATCH v2 09/24] iommu/sprd: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Chunyan Zhang <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/sprd-iommu.c | 6 ------
1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 73dfd9946312..2bc1de6e823d 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -144,11 +144,6 @@ static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
if (!dom)
return NULL;

- if (iommu_get_dma_cookie(&dom->domain)) {
- kfree(dom);
- return NULL;
- }
-
spin_lock_init(&dom->pgtlock);

dom->domain.geometry.aperture_start = 0;
@@ -161,7 +156,6 @@ static void sprd_iommu_domain_free(struct iommu_domain *domain)
{
struct sprd_iommu_domain *dom = to_sprd_domain(domain);

- iommu_put_dma_cookie(domain);
kfree(dom);
}

--
2.25.1

2021-07-28 16:01:48

[permalink] [raw]

Subject: [PATCH v2 10/24] iommu/sun50i: Drop IOVA cookie management

The core code bakes its own cookies now.

CC: Maxime Ripard <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/sun50i-iommu.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 181bb1c3437c..c349a95ec7bd 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -610,14 +610,10 @@ static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)
if (!sun50i_domain)
return NULL;

- if (type == IOMMU_DOMAIN_DMA &&
- iommu_get_dma_cookie(&sun50i_domain->domain))
- goto err_free_domain;
-
sun50i_domain->dt = (u32 *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
get_order(DT_SIZE));
if (!sun50i_domain->dt)
- goto err_put_cookie;
+ goto err_free_domain;

refcount_set(&sun50i_domain->refcnt, 1);

@@ -627,10 +623,6 @@ static struct iommu_domain *sun50i_iommu_domain_alloc(unsigned type)

return &sun50i_domain->domain;

-err_put_cookie:
- if (type == IOMMU_DOMAIN_DMA)
- iommu_put_dma_cookie(&sun50i_domain->domain);
-
err_free_domain:
kfree(sun50i_domain);

@@ -644,8 +636,6 @@ static void sun50i_iommu_domain_free(struct iommu_domain *domain)
free_pages((unsigned long)sun50i_domain->dt, get_order(DT_SIZE));
sun50i_domain->dt = NULL;

- iommu_put_dma_cookie(domain);
-
kfree(sun50i_domain);
}

--
2.25.1

2021-07-28 16:01:56

[permalink] [raw]

Subject: [PATCH v2 12/24] iommu/dma: Unexport IOVA cookie management

IOVA cookies are now got and put by core code, so we no longer need to
export these to modular drivers. The export for getting MSI cookies
stays, since VFIO can still be a module, but it was already relying on
someone else putting them, so that aspect is unaffected.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/dma-iommu.c | 7 -------
drivers/iommu/iommu.c | 3 +--
2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 98ba927aee1a..10067fbc4309 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -98,9 +98,6 @@ static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
/**
* iommu_get_dma_cookie - Acquire DMA-API resources for a domain
* @domain: IOMMU domain to prepare for DMA-API usage
- *
- * IOMMU drivers should normally call this from their domain_alloc
- * callback when domain->type == IOMMU_DOMAIN_DMA.
*/
int iommu_get_dma_cookie(struct iommu_domain *domain)
{
@@ -113,7 +110,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)

return 0;
}
-EXPORT_SYMBOL(iommu_get_dma_cookie);

/**
* iommu_get_msi_cookie - Acquire just MSI remapping resources
@@ -151,8 +147,6 @@ EXPORT_SYMBOL(iommu_get_msi_cookie);
* iommu_put_dma_cookie - Release a domain's DMA mapping resources
* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() or
* iommu_get_msi_cookie()
- *
- * IOMMU drivers should normally call this from their domain_free callback.
*/
void iommu_put_dma_cookie(struct iommu_domain *domain)
{
@@ -172,7 +166,6 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
kfree(cookie);
domain->iova_cookie = NULL;
}
-EXPORT_SYMBOL(iommu_put_dma_cookie);

/**
* iommu_dma_get_resv_regions - Reserved region driver helper
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ea5a9ea8d431..fa8109369f74 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1947,8 +1947,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
/* Assume all sizes by default; the driver may override this later */
domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;

- /* Temporarily ignore -EEXIST while drivers still get their own cookies */
- if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain) == -ENOMEM) {
+ if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
iommu_domain_free(domain);
domain = NULL;
}
--
2.25.1

2021-07-28 16:02:37

[permalink] [raw]

Subject: [PATCH v2 13/24] iommu/dma: Remove redundant "!dev" checks

iommu_dma_init_domain() is now only called from iommu_setup_dma_ops(),
which has already assumed dev to be non-NULL.

Reviewed-by: John Garry <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/dma-iommu.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 10067fbc4309..e28396cea6eb 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -363,7 +363,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,

init_iova_domain(iovad, 1UL << order, base_pfn);

- if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) &&
+ if (!cookie->fq_domain && !dev_is_untrusted(dev) &&
domain->ops->flush_iotlb_all && !iommu_get_dma_strict(domain)) {
if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
iommu_dma_entry_dtor))
@@ -372,9 +372,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
cookie->fq_domain = domain;
}

- if (!dev)
- return 0;
-
return iova_reserve_iommu_regions(dev, domain);
}

--
2.25.1

2021-07-28 16:02:45

[permalink] [raw]

Subject: [PATCH v2 14/24] iommu: Introduce explicit type for non-strict DMA domains

Promote the difference between strict and non-strict DMA domains from an
internal detail to a distinct domain feature and type, to pave the road
for exposing it through the sysfs default domain interface.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/dma-iommu.c | 2 +-
drivers/iommu/iommu.c | 8 ++++++--
include/linux/iommu.h | 11 +++++++++++
3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index e28396cea6eb..8b3545c01077 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1311,7 +1311,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
* The IOMMU core code allocates the default DMA domain, which the
* underlying IOMMU driver needs to support via the dma-iommu layer.
*/
- if (domain->type == IOMMU_DOMAIN_DMA) {
+ if (iommu_is_dma_domain(domain)) {
if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
goto out_err;
dev->dma_ops = &iommu_dma_ops;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index fa8109369f74..982545234cf3 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -115,6 +115,7 @@ static const char *iommu_domain_type_str(unsigned int t)
case IOMMU_DOMAIN_UNMANAGED:
return "Unmanaged";
case IOMMU_DOMAIN_DMA:
+ case IOMMU_DOMAIN_DMA_FQ:
return "Translated";
default:
return "Unknown";
@@ -552,6 +553,9 @@ static ssize_t iommu_group_show_type(struct iommu_group *group,
case IOMMU_DOMAIN_DMA:
type = "DMA\n";
break;
+ case IOMMU_DOMAIN_DMA_FQ:
+ type = "DMA-FQ\n";
+ break;
}
}
mutex_unlock(&group->mutex);
@@ -765,7 +769,7 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
unsigned long pg_size;
int ret = 0;

- if (!domain || domain->type != IOMMU_DOMAIN_DMA)
+ if (!domain || !iommu_is_dma_domain(domain))
return 0;

BUG_ON(!domain->pgsize_bitmap);
@@ -1947,7 +1951,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
/* Assume all sizes by default; the driver may override this later */
domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;

- if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
+ if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
iommu_domain_free(domain);
domain = NULL;
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 141779d76035..046ba4d54cd2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -61,6 +61,7 @@ struct iommu_domain_geometry {
#define __IOMMU_DOMAIN_DMA_API (1U << 1) /* Domain for use in DMA-API
implementation */
#define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
+#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */

/*
* This are the possible domain-types
@@ -73,12 +74,17 @@ struct iommu_domain_geometry {
* IOMMU_DOMAIN_DMA - Internally used for DMA-API implementations.
* This flag allows IOMMU drivers to implement
* certain optimizations for these domains
+ * IOMMU_DOMAIN_DMA_FQ - As above, but definitely using batched TLB
+ * invalidation.
*/
#define IOMMU_DOMAIN_BLOCKED (0U)
#define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT)
#define IOMMU_DOMAIN_UNMANAGED (__IOMMU_DOMAIN_PAGING)
#define IOMMU_DOMAIN_DMA (__IOMMU_DOMAIN_PAGING | \
__IOMMU_DOMAIN_DMA_API)
+#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
+ __IOMMU_DOMAIN_DMA_API | \
+ __IOMMU_DOMAIN_DMA_FQ)

struct iommu_domain {
unsigned type;
@@ -90,6 +96,11 @@ struct iommu_domain {
struct iommu_dma_cookie *iova_cookie;
};

+static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
+{
+ return domain->type & __IOMMU_DOMAIN_DMA_API;
+}
+
enum iommu_cap {
IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
transactions */
--
2.25.1

2021-07-28 16:03:02

[permalink] [raw]

Subject: [PATCH v2 18/24] iommu: Express DMA strictness via the domain type

Eliminate the iommu_get_dma_strict() indirection and pipe the
information through the domain type from the beginning. Besides
the flow simplification this also has several nice side-effects:

- Automatically implies strict mode for untrusted devices by
virtue of their IOMMU_DOMAIN_DMA override.
- Ensures that we only end up using flush queues for drivers
which are aware of them and can actually benefit.
- Allows us to handle flush queue init failure by falling back
to strict mode instead of leaving it to possibly blow up later.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +-
drivers/iommu/dma-iommu.c | 9 +++++----
drivers/iommu/iommu.c | 12 +++---------
include/linux/iommu.h | 1 -
5 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index a1f0d83d1eb5..19400826eba7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2175,7 +2175,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
.iommu_dev = smmu->dev,
};

- if (!iommu_get_dma_strict(domain))
+ if (domain->type == IOMMU_DOMAIN_DMA_FQ)
pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;

pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 936c5e9d5e82..109e4723f9f5 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -765,7 +765,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
.iommu_dev = smmu->dev,
};

- if (!iommu_get_dma_strict(domain))
+ if (domain->type == IOMMU_DOMAIN_DMA_FQ)
pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;

if (smmu->impl && smmu->impl->init_context) {
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 8b3545c01077..7f3968865387 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -363,13 +363,14 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,

init_iova_domain(iovad, 1UL << order, base_pfn);

- if (!cookie->fq_domain && !dev_is_untrusted(dev) &&
- domain->ops->flush_iotlb_all && !iommu_get_dma_strict(domain)) {
+ if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
- iommu_dma_entry_dtor))
+ iommu_dma_entry_dtor)) {
pr_warn("iova flush queue initialization failed\n");
- else
+ domain->type = IOMMU_DOMAIN_DMA;
+ } else {
cookie->fq_domain = domain;
+ }
}

return iova_reserve_iommu_regions(dev, domain);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 982545234cf3..eecb5657de69 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void)
}
}

+ if (!iommu_default_passthrough() && !iommu_dma_strict)
+ iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;
+
pr_info("Default domain type: %s %s\n",
iommu_domain_type_str(iommu_def_domain_type),
(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
@@ -357,15 +360,6 @@ void iommu_set_dma_strict(void)
iommu_dma_strict = true;
}

-bool iommu_get_dma_strict(struct iommu_domain *domain)
-{
- /* only allow lazy flushing for DMA domains */
- if (domain->type == IOMMU_DOMAIN_DMA)
- return iommu_dma_strict;
- return true;
-}
-EXPORT_SYMBOL_GPL(iommu_get_dma_strict);
-
static ssize_t iommu_group_attr_show(struct kobject *kobj,
struct attribute *__attr, char *buf)
{
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 046ba4d54cd2..edfe2fdb8368 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -498,7 +498,6 @@ int iommu_set_pgtable_quirks(struct iommu_domain *domain,
unsigned long quirks);

void iommu_set_dma_strict(void);
-bool iommu_get_dma_strict(struct iommu_domain *domain);

extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev,
unsigned long iova, int flags);
--
2.25.1

2021-07-28 16:03:26

[permalink] [raw]

Subject: [PATCH v2 19/24] iommu: Expose DMA domain strictness via sysfs

The sysfs interface for default domain types exists primarily so users
can choose the performance/security tradeoff relevant to their own
workload. As such, the choice between the policies for DMA domains fits
perfectly as an additional point on that scale - downgrading a
particular device from a strict default to non-strict may be enough to
let it reach the desired level of performance, while still retaining
more peace of mind than with a wide-open identity domain. Now that we've
abstracted non-strict mode as a distinct type of DMA domain, allow it to
be chosen through the user interface as well.

Signed-off-by: Robin Murphy <[email protected]>
---
Documentation/ABI/testing/sysfs-kernel-iommu_groups | 2 ++
drivers/iommu/iommu.c | 2 ++
2 files changed, 4 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
index eae2f1c1e11e..43ba764ba5b7 100644
--- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
+++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
@@ -42,6 +42,8 @@ Description: /sys/kernel/iommu_groups/<grp_id>/type shows the type of default
======== ======================================================
DMA All the DMA transactions from the device in this group
are translated by the iommu.
+ DMA-FQ As above, but using batched invalidation to lazily
+ remove translations after use.
identity All the DMA transactions from the device in this group
are not translated by the iommu.
auto Change to the type the device was booted with.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index eecb5657de69..5a08e0806cbb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3265,6 +3265,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
req_type = IOMMU_DOMAIN_IDENTITY;
else if (sysfs_streq(buf, "DMA"))
req_type = IOMMU_DOMAIN_DMA;
+ else if (sysfs_streq(buf, "DMA-FQ"))
+ req_type = IOMMU_DOMAIN_DMA_FQ;
else if (sysfs_streq(buf, "auto"))
req_type = 0;
else
--
2.25.1

2021-07-28 16:03:26

[permalink] [raw]

Subject: [PATCH v2 21/24] iommu/dma: Factor out flush queue init

Factor out flush queue setup from the initial domain init so that we
can potentially trigger it from sysfs later on in a domain's lifetime.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/dma-iommu.c | 30 ++++++++++++++++++++----------
include/linux/dma-iommu.h | 9 ++++++---
2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7f3968865387..304a3ec71223 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
}

+int iommu_dma_init_fq(struct iommu_domain *domain)
+{
+ struct iommu_dma_cookie *cookie = domain->iova_cookie;
+
+ if (domain->type != IOMMU_DOMAIN_DMA_FQ)
+ return -EINVAL;
+ if (cookie->fq_domain)
+ return 0;
+
+ if (init_iova_flush_queue(&cookie->iovad, iommu_dma_flush_iotlb_all,
+ iommu_dma_entry_dtor)) {
+ pr_warn("iova flush queue initialization failed\n");
+ domain->type = IOMMU_DOMAIN_DMA;
+ return -ENODEV;
+ }
+ cookie->fq_domain = domain;
+ return 0;
+}
+
/**
* iommu_dma_init_domain - Initialise a DMA mapping domain
* @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -362,16 +381,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
}

init_iova_domain(iovad, 1UL << order, base_pfn);
-
- if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
- if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
- iommu_dma_entry_dtor)) {
- pr_warn("iova flush queue initialization failed\n");
- domain->type = IOMMU_DOMAIN_DMA;
- } else {
- cookie->fq_domain = domain;
- }
- }
+ iommu_dma_init_fq(domain);

return iova_reserve_iommu_regions(dev, domain);
}
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 758ca4694257..81ab647f1618 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -20,6 +20,7 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);

/* Setup call for arch DMA mapping code */
void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
+int iommu_dma_init_fq(struct iommu_domain *domain);

/* The DMA API isn't _quite_ the whole story, though... */
/*
@@ -37,9 +38,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,

void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);

-void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
- struct iommu_domain *domain);
-
extern bool iommu_dma_forcedac;

#else /* CONFIG_IOMMU_DMA */
@@ -54,6 +52,11 @@ static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base,
{
}

+static inline int iommu_dma_init_fq(struct iommu_domain *domain)
+{
+ return -EINVAL;
+}
+
static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
{
return -ENODEV;
--
2.25.1

2021-07-28 16:03:40

[permalink] [raw]

Subject: [PATCH v2 23/24] iommu/arm-smmu: Allow non-strict in pgtable_quirks interface

To make io-pgtable aware of a flush queue being dynamically enabled,
allow IO_PGTABLE_QUIRK_NON_STRICT to be set even after a domain has been
attached to, and hook up the final piece of the puzzle in iommu-dma.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 +++++++++++
drivers/iommu/dma-iommu.c | 3 +++
3 files changed, 29 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 19400826eba7..40fa9cb382c3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2711,6 +2711,20 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
return ret;
}

+static int arm_smmu_set_pgtable_quirks(struct iommu_domain *domain,
+ unsigned long quirks)
+{
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+
+ if (quirks == IO_PGTABLE_QUIRK_NON_STRICT && smmu_domain->pgtbl_ops) {
+ struct io_pgtable *iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
+
+ iop->cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+ return 0;
+ }
+ return -EINVAL;
+}
+
static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
{
return iommu_fwspec_add_ids(dev, args->args, 1);
@@ -2825,6 +2839,7 @@ static struct iommu_ops arm_smmu_ops = {
.release_device = arm_smmu_release_device,
.device_group = arm_smmu_device_group,
.enable_nesting = arm_smmu_enable_nesting,
+ .set_pgtable_quirks = arm_smmu_set_pgtable_quirks,
.of_xlate = arm_smmu_of_xlate,
.get_resv_regions = arm_smmu_get_resv_regions,
.put_resv_regions = generic_iommu_put_resv_regions,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 109e4723f9f5..f18684f308b9 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1518,6 +1518,17 @@ static int arm_smmu_set_pgtable_quirks(struct iommu_domain *domain,
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
int ret = 0;

+ if (quirks == IO_PGTABLE_QUIRK_NON_STRICT) {
+ struct io_pgtable *iop;
+
+ if (!smmu_domain->pgtbl_ops)
+ return -EINVAL;
+
+ iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
+ iop->cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
+ return 0;
+ }
+
mutex_lock(&smmu_domain->init_mutex);
if (smmu_domain->smmu)
ret = -EPERM;
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 304a3ec71223..6e3eca778267 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -16,6 +16,7 @@
#include <linux/huge_mm.h>
#include <linux/iommu.h>
#include <linux/iova.h>
+#include <linux/io-pgtable.h>
#include <linux/irq.h>
#include <linux/mm.h>
#include <linux/mutex.h>
@@ -326,6 +327,8 @@ int iommu_dma_init_fq(struct iommu_domain *domain)
return -ENODEV;
}
cookie->fq_domain = domain;
+ if (domain->ops->set_pgtable_quirks)
+ domain->ops->set_pgtable_quirks(domain, IO_PGTABLE_QUIRK_NON_STRICT);
return 0;
}

--
2.25.1

2021-07-28 16:04:03

[permalink] [raw]

Subject: [PATCH v2 17/24] iommu/vt-d: Prepare for multiple DMA domain types

In preparation for the strict vs. non-strict decision for DMA domains
to be expressed in the domain type, make sure we expose our flush queue
awareness by accepting the new domain type, and test the specific
feature flag where we want to identify DMA domains in general. The DMA
ops reset/setup can simply be made unconditional, since iommu-dma
already knows only to touch DMA domains.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/intel/iommu.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7e168634c433..8fc46c9d6b96 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -582,7 +582,7 @@ struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
int iommu_id;

/* si_domain and vm domain should not get here. */
- if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
+ if (WARN_ON(!iommu_is_dma_domain(&domain->domain)))
return NULL;

for_each_domain_iommu(iommu_id, domain)
@@ -1034,7 +1034,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
pteval = ((uint64_t)virt_to_dma_pfn(tmp_page) << VTD_PAGE_SHIFT) | DMA_PTE_READ | DMA_PTE_WRITE;
if (domain_use_first_level(domain)) {
pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
- if (domain->domain.type == IOMMU_DOMAIN_DMA)
+ if (iommu_is_dma_domain(&domain->domain))
pteval |= DMA_FL_PTE_ACCESS;
}
if (cmpxchg64(&pte->val, 0ULL, pteval))
@@ -2345,7 +2345,7 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
if (domain_use_first_level(domain)) {
attr |= DMA_FL_PTE_XD | DMA_FL_PTE_US;

- if (domain->domain.type == IOMMU_DOMAIN_DMA) {
+ if (iommu_is_dma_domain(&domain->domain)) {
attr |= DMA_FL_PTE_ACCESS;
if (prot & DMA_PTE_WRITE)
attr |= DMA_FL_PTE_DIRTY;
@@ -4528,6 +4528,7 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)

switch (type) {
case IOMMU_DOMAIN_DMA:
+ case IOMMU_DOMAIN_DMA_FQ:
case IOMMU_DOMAIN_UNMANAGED:
dmar_domain = alloc_domain(0);
if (!dmar_domain) {
@@ -5197,12 +5198,8 @@ static void intel_iommu_release_device(struct device *dev)

static void intel_iommu_probe_finalize(struct device *dev)
{
- struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-
- if (domain && domain->type == IOMMU_DOMAIN_DMA)
- iommu_setup_dma_ops(dev, 0, U64_MAX);
- else
- set_dma_ops(dev, NULL);
+ set_dma_ops(dev, NULL);
+ iommu_setup_dma_ops(dev, 0, U64_MAX);
}

static void intel_iommu_get_resv_regions(struct device *device,
--
2.25.1

2021-07-28 16:04:19

[permalink] [raw]

Subject: [PATCH v2 20/24] iommu: Merge strictness and domain type configs

To parallel the sysfs behaviour, merge the new build-time option
for DMA domain strictness into the default domain type choice.

Suggested-by: Joerg Roedel <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/Kconfig | 80 +++++++++++++++++++++----------------------
drivers/iommu/iommu.c | 2 +-
2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index c84da8205be7..6e06f876d75a 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -79,55 +79,55 @@ config IOMMU_DEBUGFS
debug/iommu directory, and then populate a subdirectory with
entries as required.

-config IOMMU_DEFAULT_PASSTHROUGH
- bool "IOMMU passthrough by default"
- depends on IOMMU_API
- help
- Enable passthrough by default, removing the need to pass in
- iommu.passthrough=on or iommu=pt through command line. If this
- is enabled, you can still disable with iommu.passthrough=off
- or iommu=nopt depending on the architecture.
-
- If unsure, say N here.
-
choice
- prompt "IOMMU default DMA IOTLB invalidation mode"
- depends on IOMMU_DMA
-
- default IOMMU_DEFAULT_LAZY if (AMD_IOMMU || INTEL_IOMMU)
- default IOMMU_DEFAULT_STRICT
+ prompt "IOMMU default domain type"
+ depends on IOMMU_API
+ default IOMMU_DEFAULT_DMA_LAZY if AMD_IOMMU || INTEL_IOMMU
+ default IOMMU_DEFAULT_DMA_STRICT
help
- This option allows an IOMMU DMA IOTLB invalidation mode to be
- chosen at build time, to override the default mode of each ARCH,
- removing the need to pass in kernel parameters through command line.
- It is still possible to provide common boot params to override this
- config.
+ Choose the type of IOMMU domain used to manage DMA API usage by
+ device drivers. The options here typically represent different
+ levels of tradeoff between robustness/security and performance,
+ depending on the IOMMU driver. Not all IOMMUs support all options.
+ This choice can be overridden at boot via the command line, and for
+ some devices also at runtime via sysfs.

If unsure, keep the default.

-config IOMMU_DEFAULT_STRICT
- bool "strict"
+config IOMMU_DEFAULT_DMA_STRICT
+ bool "Translated - Strict"
help
- For every IOMMU DMA unmap operation, the flush operation of IOTLB and
- the free operation of IOVA are guaranteed to be done in the unmap
- function.
+ Trusted devices use translation to restrict their access to only
+ DMA-mapped pages, with strict TLB invalidation on unmap. Equivalent
+ to passing "iommu.passthrough=0 iommu.strict=1" on the command line.

-config IOMMU_DEFAULT_LAZY
- bool "lazy"
+ Untrusted devices always use this mode, with an additional layer of
+ bounce-buffering such that they cannot gain access to any unrelated
+ data within a mapped page.
+
+config IOMMU_DEFAULT_DMA_LAZY
+ bool "Translated - Lazy"
help
- Support lazy mode, where for every IOMMU DMA unmap operation, the
- flush operation of IOTLB and the free operation of IOVA are deferred.
- They are only guaranteed to be done before the related IOVA will be
- reused.
+ Trusted devices use translation to restrict their access to only
+ DMA-mapped pages, but with "lazy" batched TLB invalidation. This
+ mode allows higher performance with some IOMMUs due to reduced TLB
+ flushing, but at the cost of reduced isolation since devices may be
+ able to access memory for some time after it has been unmapped.
+ Equivalent to passing "iommu.passthrough=0 iommu.strict=0" on the
+ command line.

- The isolation provided in this mode is not as secure as STRICT mode,
- such that a vulnerable time window may be created between the DMA
- unmap and the mappings cached in the IOMMU IOTLB or device TLB
- finally being invalidated, where the device could still access the
- memory which has already been unmapped by the device driver.
- However this mode may provide better performance in high throughput
- scenarios, and is still considerably more secure than passthrough
- mode or no IOMMU.
+ If this mode is not supported by the IOMMU driver, the effective
+ runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.
+
+config IOMMU_DEFAULT_PASSTHROUGH
+ bool "Passthrough"
+ help
+ Trusted devices are identity-mapped, giving them unrestricted access
+ to memory with minimal performance overhead. Equivalent to passing
+ "iommu.passthrough=1" (historically "iommu=pt") on the command line.
+
+ If this mode is not supported by the IOMMU driver, the effective
+ runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.

endchoice

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5a08e0806cbb..25c1adc1ec67 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -31,7 +31,7 @@ static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);

static unsigned int iommu_def_domain_type __read_mostly;
-static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_STRICT);
+static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
static u32 iommu_cmd_line __read_mostly;

struct iommu_group {
--
2.25.1

2021-07-28 16:08:19

[permalink] [raw]

Subject: [PATCH v2 16/24] iommu/arm-smmu: Prepare for multiple DMA domain types

In preparation for the strict vs. non-strict decision for DMA domains to
be expressed in the domain type, make sure we expose our flush queue
awareness by accepting the new domain type.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
drivers/iommu/arm/arm-smmu/arm-smmu.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 4c648da447bf..a1f0d83d1eb5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1972,6 +1972,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)

if (type != IOMMU_DOMAIN_UNMANAGED &&
type != IOMMU_DOMAIN_DMA &&
+ type != IOMMU_DOMAIN_DMA_FQ &&
type != IOMMU_DOMAIN_IDENTITY)
return NULL;

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 970d9e4dcd69..936c5e9d5e82 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -869,7 +869,8 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
struct arm_smmu_domain *smmu_domain;

if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
- if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
+ if (using_legacy_binding ||
+ (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_DMA_FQ))
return NULL;
}
/*
--
2.25.1

2021-07-28 16:09:12

[permalink] [raw]

Subject: [PATCH v2 24/24] iommu: Only log strictness for DMA domains

When passthrough is enabled, the default strictness policy becomes
irrelevant, since any subsequent runtime override to a DMA domain type
now embodies an explicit choice of strictness as well. Save on noise by
only logging the default policy when it is meaningfully in effect.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommu.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index be399d630953..87d7b299436e 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -144,10 +144,11 @@ static int __init iommu_subsys_init(void)
(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
"(set via kernel command line)" : "");

- pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
- iommu_dma_strict ? "strict" : "lazy",
- (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
- "(set via kernel command line)" : "");
+ if (!iommu_default_passthrough())
+ pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
+ iommu_dma_strict ? "strict" : "lazy",
+ (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
+ "(set via kernel command line)" : "");

return 0;
}
--
2.25.1

2021-07-28 16:09:12

[permalink] [raw]

Subject: [PATCH v2 22/24] iommu: Allow enabling non-strict mode dynamically

Allocating and enabling a flush queue is in fact something we can
reasonably do while a DMA domain is active, without having to rebuild it
from scratch. Thus we can allow a strict -> non-strict transition from
sysfs without requiring to unbind the device's driver, which is of
particular interest to users who want to make selective relaxations to
critical devices like the one serving their root filesystem.

Disabling and draining a queue also seems technically possible to
achieve without rebuilding the whole domain, but would certainly be more
involved. Furthermore there's not such a clear use-case for tightening
up security *after* the device may already have done whatever it is that
you don't trust it not to do, so we only consider the relaxation case.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommu.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 25c1adc1ec67..be399d630953 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3200,6 +3200,13 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
goto out;
}

+ /* We can bring up a flush queue without tearing down the domain */
+ if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
+ prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
+ ret = iommu_dma_init_fq(prev_dom);
+ goto out;
+ }
+
/* Sets group->default_domain to the newly allocated domain */
ret = iommu_group_alloc_default_domain(dev->bus, group, type);
if (ret)
@@ -3240,9 +3247,9 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
}

/*
- * Changing the default domain through sysfs requires the users to ubind the
- * drivers from the devices in the iommu group. Return failure if this doesn't
- * meet.
+ * Changing the default domain through sysfs requires the users to unbind the
+ * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
+ * transition. Return failure if this isn't met.
*
* We need to consider the race between this and the device release path.
* device_lock(dev) is used here to guarantee that the device release path
@@ -3318,7 +3325,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,

/* Check if the device in the group still has a driver bound to it */
device_lock(dev);
- if (device_is_bound(dev)) {
+ if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
+ group->default_domain->type == IOMMU_DOMAIN_DMA)) {
pr_err_ratelimited("Device is still bound to driver\n");
ret = -EBUSY;
goto out;
--
2.25.1

2021-07-29 02:57:07

by chenxiang (M)

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

Hi Robin,

在 2021/7/28 23:58, Robin Murphy 写道:
> Hi all,
>
> Here's v2 where things start to look more realistic, hence the expanded
> CC list. The patches are now based on the current iommu/core branch to
> take John's iommu_set_dma_strict() cleanup into account.
>
> The series remiains in two (or possibly 3) logical parts - for people
> CC'd on cookie cleanup patches, the later parts should not affect you
> since your drivers don't implement non-strict mode anyway; the cleanup
> is all pretty straightforward, but please do yell at me if I've managed
> to let a silly mistake slip through and broken your driver.
>
> This time I have also build-tested x86 as well as arm64 :)

I have tested those patchset on ARM64 with SMMUV3, and the testcases are
as follows:
- Boot with iommu.strict=0, running fio and it works well;
- Boot with iommu.strict=1, running fio and it works well;
- Change strict mode to lazy mode when building, the change takes effect;
- Boot without iommu.strict(default strict mode), change the sysfs
interface type from DMA to DMA-FQ dynamically during running fio, and it
works well;
- Boot without iommu.strict(default strict mode), change the sysfs
interface type from DMA-FQ to DMA dynamically, and it is not allowed and
print "Device or resource busy"
(i know it is qualified, and we can change no-strict mode to strict by
unbind the driver -> change the sysfs interface (type)->bind the driver
(tested this and it works well),
but i have a small question: is it also possible to change from DMA-FQ
to DMA dynamically? )

Anyway, please feel free to add :
Tested-by: Xiang Chen <[email protected]>

>
> Changes in v2:
>
> - Add iommu_is_dma_domain() helper to abstract flag check (and help
> avoid silly typos like the one in v1).
> - Tweak a few commit messages for spelling and (hopefully) clarity.
> - Move the iommu_create_device_direct_mappings() update to patch #14
> where it should have been.
> - Rewrite patch #20 as a conversion of the now-existing option.
> - Clean up the ops->flush_iotlb_all check which is also made redundant
> by the new domain type
> - Add patch #24, which is arguably tangential, but it was something I
> spotted during the rebase, so...
>
> Once again, the whole lot is available on a branch here:
>
> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>
> Thanks,
> Robin.
>
>
> CC: Marek Szyprowski <[email protected]>
> CC: Yoshihiro Shimoda <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: Yong Wu <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Maxime Ripard <[email protected]>
> CC: Jean-Philippe Brucker <[email protected]>
>
> Robin Murphy (24):
> iommu: Pull IOVA cookie management into the core
> iommu/amd: Drop IOVA cookie management
> iommu/arm-smmu: Drop IOVA cookie management
> iommu/vt-d: Drop IOVA cookie management
> iommu/exynos: Drop IOVA cookie management
> iommu/ipmmu-vmsa: Drop IOVA cookie management
> iommu/mtk: Drop IOVA cookie management
> iommu/rockchip: Drop IOVA cookie management
> iommu/sprd: Drop IOVA cookie management
> iommu/sun50i: Drop IOVA cookie management
> iommu/virtio: Drop IOVA cookie management
> iommu/dma: Unexport IOVA cookie management
> iommu/dma: Remove redundant "!dev" checks
> iommu: Introduce explicit type for non-strict DMA domains
> iommu/amd: Prepare for multiple DMA domain types
> iommu/arm-smmu: Prepare for multiple DMA domain types
> iommu/vt-d: Prepare for multiple DMA domain types
> iommu: Express DMA strictness via the domain type
> iommu: Expose DMA domain strictness via sysfs
> iommu: Merge strictness and domain type configs
> iommu/dma: Factor out flush queue init
> iommu: Allow enabling non-strict mode dynamically
> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
> iommu: Only log strictness for DMA domains
>
> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
> drivers/iommu/Kconfig | 80 +++++++++----------
> drivers/iommu/amd/iommu.c | 21 +----
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
> drivers/iommu/dma-iommu.c | 44 +++++-----
> drivers/iommu/exynos-iommu.c | 18 +----
> drivers/iommu/intel/iommu.c | 23 ++----
> drivers/iommu/iommu.c | 53 +++++++-----
> drivers/iommu/ipmmu-vmsa.c | 27 +------
> drivers/iommu/mtk_iommu.c | 6 --
> drivers/iommu/rockchip-iommu.c | 11 +--
> drivers/iommu/sprd-iommu.c | 6 --
> drivers/iommu/sun50i-iommu.c | 12 +--
> drivers/iommu/virtio-iommu.c | 8 --
> include/linux/dma-iommu.h | 9 ++-
> include/linux/iommu.h | 15 +++-
> 18 files changed, 171 insertions(+), 226 deletions(-)
>

2021-07-29 07:19:28

[permalink] [raw]

Subject: Re: [PATCH v2 18/24] iommu: Express DMA strictness via the domain type

Hi Robin,

On 7/28/21 11:58 PM, Robin Murphy wrote:
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 982545234cf3..eecb5657de69 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void)
> }
> }
>
> + if (!iommu_default_passthrough() && !iommu_dma_strict)
> + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;

iommu_dma_strict could be changed later by the vendor iommu driver via
iommu_set_dma_strict(). This seems not to be the right place to set
iommu_def_domain_type.

> +
> pr_info("Default domain type: %s %s\n",
> iommu_domain_type_str(iommu_def_domain_type),
> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?

Best regards,
baolu

2021-07-29 09:06:23

[permalink] [raw]

Subject: Re: [PATCH v2 24/24] iommu: Only log strictness for DMA domains

On 28/07/2021 16:58, Robin Murphy wrote:
> When passthrough is enabled, the default strictness policy becomes
> irrelevant, since any subsequent runtime override to a DMA domain type
> now embodies an explicit choice of strictness as well. Save on noise by
> only logging the default policy when it is meaningfully in effect.
>
> Signed-off-by: Robin Murphy<[email protected]>

FWIW, small comment below,

Reviewed-by: John Garry <[email protected]>

> ---
> drivers/iommu/iommu.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index be399d630953..87d7b299436e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -144,10 +144,11 @@ static int __init iommu_subsys_init(void)
> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
> "(set via kernel command line)" : "");
>
> - pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
> - iommu_dma_strict ? "strict" : "lazy",
> - (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> - "(set via kernel command line)" : "");
> + if (!iommu_default_passthrough())

I suppose that you could also do an early return to save indenting...

> + pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
> + iommu_dma_strict ? "strict" : "lazy",
> + (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> + "(set via kernel command line)" : "");
>
> return 0;
> }

2021-07-29 09:38:29

[permalink] [raw]

Subject: Re: [PATCH v2 18/24] iommu: Express DMA strictness via the domain type

On 2021-07-29 08:13, Lu Baolu wrote:
> Hi Robin,
>
> On 7/28/21 11:58 PM, Robin Murphy wrote:
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 982545234cf3..eecb5657de69 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void)
>>           }
>>       }
>> +    if (!iommu_default_passthrough() && !iommu_dma_strict)
>> +        iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;
>
> iommu_dma_strict could be changed later by the vendor iommu driver via
> iommu_set_dma_strict(). This seems not to be the right place to set
> iommu_def_domain_type.

Ah yes, good catch once again, thanks!

I think this *is* the right place to initially set it to honour the
command-line option, since that matches what we do for passthrough.
However also like passthrough we'll need to keep things in sync if it's
updated later, like this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 87d7b299436e..593d4555bc57 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup);
void iommu_set_dma_strict(void)
{
iommu_dma_strict = true;
+ if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ)
+ iommu_def_domain_type = IOMMU_DOMAIN_DMA;
}

static ssize_t iommu_group_attr_show(struct kobject *kobj,

Does that seem reasonable? I'm not sure there's any cleaner way to do it
since we don't want to inadvertently clobber the default type if the
user has given us something funky like "intel_iommu=strict
iommu.passthrough=1".

Cheers,
Robin.

>
>> +
>>       pr_info("Default domain type: %s %s\n",
>>           iommu_domain_type_str(iommu_def_domain_type),
>>           (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
>
> Best regards,
> baolu

2021-07-29 11:00:06

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

On 2021-07-29 03:55, chenxiang (M) wrote:
> Hi Robin,
>
>
> 在 2021/7/28 23:58, Robin Murphy 写道:
>> Hi all,
>>
>> Here's v2 where things start to look more realistic, hence the expanded
>> CC list. The patches are now based on the current iommu/core branch to
>> take John's iommu_set_dma_strict() cleanup into account.
>>
>> The series remiains in two (or possibly 3) logical parts - for people
>> CC'd on cookie cleanup patches, the later parts should not affect you
>> since your drivers don't implement non-strict mode anyway; the cleanup
>> is all pretty straightforward, but please do yell at me if I've managed
>> to let a silly mistake slip through and broken your driver.
>>
>> This time I have also build-tested x86 as well as arm64 :)
>
> I have tested those patchset on ARM64 with SMMUV3, and the testcases are
> as follows:
> - Boot with iommu.strict=0, running fio and it works well;
> - Boot with iommu.strict=1, running fio and it works well;
> - Change strict mode to lazy mode when building, the change takes effect;
> - Boot without iommu.strict(default strict mode), change the sysfs
> interface type from DMA to DMA-FQ dynamically during running fio, and it
> works well;
> - Boot without iommu.strict(default strict mode), change the sysfs
> interface type from DMA-FQ to DMA dynamically, and it is not allowed and
> print "Device or resource busy"
> (i know it is qualified, and we can change no-strict mode to strict by
> unbind the driver -> change the sysfs interface (type)->bind the driver
> (tested this and it works well),
> but i have a small question: is it also possible to change from DMA-FQ
> to DMA dynamically? )

As patch #22 mentions, I think it's possible in principle, but it's
certainly trickier. When enabling a flush queue, it doesn't matter if it
takes a while for other threads to notice that cookie->fq_domain is now
set and stop doing synchronous invalidations (and in the SMMU case it
seems like there are probably enough dependencies to additionally
prevent the io_pgtable quirk being observable before that). However when
disabling, we'd need to be absolutely sure that the driver *has* started
invalidating strictly before we stop queueing freed IOVAs, plus we need
to be absolutely sure that we've stopped queueing freed IOVAs before we
attempt to tear down the flush queue itself. I'm not sure off-hand how
feasible it would be to put all that synchronisation in the right places
without it also impacting normal operation.

Furthermore, as also noted, there doesn't seem to be a good reason for
ever actually needing to do that. If a device isn't trusted, it should
be given a strict domain *before* any driver has a chance to start doing
anything, or your trust model is broken and pretty useless. I can
imagine some niche debugging/benchmarking cases where it might help save
a bit of effort, but nothing with a strong enough justification to be
worth supporting in mainline.

> Anyway, please feel free to add :
> Tested-by: Xiang Chen <[email protected]>

That's great, thanks!

Robin.

>> Changes in v2:
>>
>> - Add iommu_is_dma_domain() helper to abstract flag check (and help
>>    avoid silly typos like the one in v1).
>> - Tweak a few commit messages for spelling and (hopefully) clarity.
>> - Move the iommu_create_device_direct_mappings() update to patch #14
>>    where it should have been.
>> - Rewrite patch #20 as a conversion of the now-existing option.
>> - Clean up the ops->flush_iotlb_all check which is also made redundant
>>    by the new domain type
>> - Add patch #24, which is arguably tangential, but it was something I
>>    spotted during the rebase, so...
>>
>> Once again, the whole lot is available on a branch here:
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>>
>> Thanks,
>> Robin.
>>
>>
>> CC: Marek Szyprowski <[email protected]>
>> CC: Yoshihiro Shimoda <[email protected]>
>> CC: Geert Uytterhoeven <[email protected]>
>> CC: Yong Wu <[email protected]>
>> CC: Heiko Stuebner <[email protected]>
>> CC: Chunyan Zhang <[email protected]>
>> CC: Chunyan Zhang <[email protected]>
>> CC: Maxime Ripard <[email protected]>
>> CC: Jean-Philippe Brucker <[email protected]>
>>
>> Robin Murphy (24):
>>    iommu: Pull IOVA cookie management into the core
>>    iommu/amd: Drop IOVA cookie management
>>    iommu/arm-smmu: Drop IOVA cookie management
>>    iommu/vt-d: Drop IOVA cookie management
>>    iommu/exynos: Drop IOVA cookie management
>>    iommu/ipmmu-vmsa: Drop IOVA cookie management
>>    iommu/mtk: Drop IOVA cookie management
>>    iommu/rockchip: Drop IOVA cookie management
>>    iommu/sprd: Drop IOVA cookie management
>>    iommu/sun50i: Drop IOVA cookie management
>>    iommu/virtio: Drop IOVA cookie management
>>    iommu/dma: Unexport IOVA cookie management
>>    iommu/dma: Remove redundant "!dev" checks
>>    iommu: Introduce explicit type for non-strict DMA domains
>>    iommu/amd: Prepare for multiple DMA domain types
>>    iommu/arm-smmu: Prepare for multiple DMA domain types
>>    iommu/vt-d: Prepare for multiple DMA domain types
>>    iommu: Express DMA strictness via the domain type
>>    iommu: Expose DMA domain strictness via sysfs
>>    iommu: Merge strictness and domain type configs
>>    iommu/dma: Factor out flush queue init
>>    iommu: Allow enabling non-strict mode dynamically
>>    iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
>>    iommu: Only log strictness for DMA domains
>>
>> .../ABI/testing/sysfs-kernel-iommu_groups     | 2 +
>> drivers/iommu/Kconfig                         | 80 +++++++++----------
>> drivers/iommu/amd/iommu.c                     | 21 +----
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 25 ++++--
>> drivers/iommu/arm/arm-smmu/arm-smmu.c         | 29 ++++---
>> drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 8 --
>> drivers/iommu/dma-iommu.c                     | 44 +++++-----
>> drivers/iommu/exynos-iommu.c                  | 18 +----
>> drivers/iommu/intel/iommu.c                   | 23 ++----
>> drivers/iommu/iommu.c                         | 53 +++++++-----
>> drivers/iommu/ipmmu-vmsa.c                    | 27 +------
>> drivers/iommu/mtk_iommu.c                     | 6 --
>> drivers/iommu/rockchip-iommu.c                | 11 +--
>> drivers/iommu/sprd-iommu.c                    | 6 --
>> drivers/iommu/sun50i-iommu.c                  | 12 +--
>> drivers/iommu/virtio-iommu.c                  | 8 --
>> include/linux/dma-iommu.h                     | 9 ++-
>> include/linux/iommu.h                         | 15 +++-
>> 18 files changed, 171 insertions(+), 226 deletions(-)
>>
>
>

2021-07-29 12:43:24

[permalink] [raw]

Subject: Re: [PATCH v2 18/24] iommu: Express DMA strictness via the domain type

On 2021/7/29 17:36, Robin Murphy wrote:
> On 2021-07-29 08:13, Lu Baolu wrote:
>> Hi Robin,
>>
>> On 7/28/21 11:58 PM, Robin Murphy wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 982545234cf3..eecb5657de69 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void)
>>>           }
>>>       }
>>> +    if (!iommu_default_passthrough() && !iommu_dma_strict)
>>> +        iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;
>>
>> iommu_dma_strict could be changed later by the vendor iommu driver via
>> iommu_set_dma_strict(). This seems not to be the right place to set
>> iommu_def_domain_type.
>
> Ah yes, good catch once again, thanks!
>
> I think this *is* the right place to initially set it to honour the
> command-line option, since that matches what we do for passthrough.
> However also like passthrough we'll need to keep things in sync if it's
> updated later, like this:
>
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 87d7b299436e..593d4555bc57 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup);
> void iommu_set_dma_strict(void)
> {
>         iommu_dma_strict = true;
> +       if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ)
> +               iommu_def_domain_type = IOMMU_DOMAIN_DMA;
> }
>
> static ssize_t iommu_group_attr_show(struct kobject *kobj,
>
>
> Does that seem reasonable? I'm not sure there's any cleaner way to do it
> since we don't want to inadvertently clobber the default type if the
> user has given us something funky like "intel_iommu=strict
> iommu.passthrough=1".

Yeah! It's reasonable as far as I can see.

Best regards,
baolu

>
> Cheers,
> Robin.
>
>>
>>> +
>>>       pr_info("Default domain type: %s %s\n",
>>>           iommu_domain_type_str(iommu_def_domain_type),
>>>           (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
>>
>> Best regards,
>> baolu

2021-07-29 15:07:11

by Heiko Stuebner

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

Hi Robin,

Am Mittwoch, 28. Juli 2021, 17:58:21 CEST schrieb Robin Murphy:
> Hi all,
>
> Here's v2 where things start to look more realistic, hence the expanded
> CC list. The patches are now based on the current iommu/core branch to
> take John's iommu_set_dma_strict() cleanup into account.
>
> The series remiains in two (or possibly 3) logical parts - for people
> CC'd on cookie cleanup patches, the later parts should not affect you
> since your drivers don't implement non-strict mode anyway; the cleanup
> is all pretty straightforward, but please do yell at me if I've managed
> to let a silly mistake slip through and broken your driver.
>
> This time I have also build-tested x86 as well as arm64 :)

TL;DR: arm64 yay, arm32 nay ;-)

testcase:
5.14-rc3
+ iommu/next
+ patches 1+8 (the ones you cc'd me on)
iommu: Pull IOVA cookie management into the core
iommu/rockchip: Drop IOVA cookie management

rk3399+hdmi (puma): boots with graphics
rk3399+edp (kevin): boots with graphics
px30+dsi (minievb): boots with graphics

rk3288 (arm32, veyron-pinky): hangs when trying to start the rockchip-drm
at some points the rest of the system recovers and fills the log with

[ 47.193776] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
[ 47.193867] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:31:plane-0] commit wait timed out
[ 57.433743] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
[ 57.433828] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:40:plane-4] commit wait timed out

spews

testcase 2:
5.14-rc3
+ iommu/next

all works fine on both arm32+arm64

That whole iommu voodoo is a bit over my head right now, so I'm not sure
what to poke to diagnose this.

Heiko

> Changes in v2:
>
> - Add iommu_is_dma_domain() helper to abstract flag check (and help
> avoid silly typos like the one in v1).
> - Tweak a few commit messages for spelling and (hopefully) clarity.
> - Move the iommu_create_device_direct_mappings() update to patch #14
> where it should have been.
> - Rewrite patch #20 as a conversion of the now-existing option.
> - Clean up the ops->flush_iotlb_all check which is also made redundant
> by the new domain type
> - Add patch #24, which is arguably tangential, but it was something I
> spotted during the rebase, so...
>
> Once again, the whole lot is available on a branch here:
>
> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>
> Thanks,
> Robin.
>
>
> CC: Marek Szyprowski <[email protected]>
> CC: Yoshihiro Shimoda <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: Yong Wu <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Maxime Ripard <[email protected]>
> CC: Jean-Philippe Brucker <[email protected]>
>
> Robin Murphy (24):
> iommu: Pull IOVA cookie management into the core
> iommu/amd: Drop IOVA cookie management
> iommu/arm-smmu: Drop IOVA cookie management
> iommu/vt-d: Drop IOVA cookie management
> iommu/exynos: Drop IOVA cookie management
> iommu/ipmmu-vmsa: Drop IOVA cookie management
> iommu/mtk: Drop IOVA cookie management
> iommu/rockchip: Drop IOVA cookie management
> iommu/sprd: Drop IOVA cookie management
> iommu/sun50i: Drop IOVA cookie management
> iommu/virtio: Drop IOVA cookie management
> iommu/dma: Unexport IOVA cookie management
> iommu/dma: Remove redundant "!dev" checks
> iommu: Introduce explicit type for non-strict DMA domains
> iommu/amd: Prepare for multiple DMA domain types
> iommu/arm-smmu: Prepare for multiple DMA domain types
> iommu/vt-d: Prepare for multiple DMA domain types
> iommu: Express DMA strictness via the domain type
> iommu: Expose DMA domain strictness via sysfs
> iommu: Merge strictness and domain type configs
> iommu/dma: Factor out flush queue init
> iommu: Allow enabling non-strict mode dynamically
> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
> iommu: Only log strictness for DMA domains
>
> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
> drivers/iommu/Kconfig | 80 +++++++++----------
> drivers/iommu/amd/iommu.c | 21 +----
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
> drivers/iommu/dma-iommu.c | 44 +++++-----
> drivers/iommu/exynos-iommu.c | 18 +----
> drivers/iommu/intel/iommu.c | 23 ++----
> drivers/iommu/iommu.c | 53 +++++++-----
> drivers/iommu/ipmmu-vmsa.c | 27 +------
> drivers/iommu/mtk_iommu.c | 6 --
> drivers/iommu/rockchip-iommu.c | 11 +--
> drivers/iommu/sprd-iommu.c | 6 --
> drivers/iommu/sun50i-iommu.c | 12 +--
> drivers/iommu/virtio-iommu.c | 8 --
> include/linux/dma-iommu.h | 9 ++-
> include/linux/iommu.h | 15 +++-
> 18 files changed, 171 insertions(+), 226 deletions(-)
>
>

2021-07-29 15:46:27

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

On 2021-07-29 16:04, Heiko Stübner wrote:
> Hi Robin,
>
> Am Mittwoch, 28. Juli 2021, 17:58:21 CEST schrieb Robin Murphy:
>> Hi all,
>>
>> Here's v2 where things start to look more realistic, hence the expanded
>> CC list. The patches are now based on the current iommu/core branch to
>> take John's iommu_set_dma_strict() cleanup into account.
>>
>> The series remiains in two (or possibly 3) logical parts - for people
>> CC'd on cookie cleanup patches, the later parts should not affect you
>> since your drivers don't implement non-strict mode anyway; the cleanup
>> is all pretty straightforward, but please do yell at me if I've managed
>> to let a silly mistake slip through and broken your driver.
>>
>> This time I have also build-tested x86 as well as arm64 :)
>
> TL;DR: arm64 yay, arm32 nay ;-)

Cheers Heiko!

> testcase:
> 5.14-rc3
> + iommu/next
> + patches 1+8 (the ones you cc'd me on)
> iommu: Pull IOVA cookie management into the core
> iommu/rockchip: Drop IOVA cookie management
>
> rk3399+hdmi (puma): boots with graphics
> rk3399+edp (kevin): boots with graphics
> px30+dsi (minievb): boots with graphics
>
> rk3288 (arm32, veyron-pinky): hangs when trying to start the rockchip-drm
> at some points the rest of the system recovers and fills the log with
>
> [ 47.193776] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [ 47.193867] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:31:plane-0] commit wait timed out
> [ 57.433743] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [ 57.433828] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:40:plane-4] commit wait timed out
>
> spews
>
> testcase 2:
> 5.14-rc3
> + iommu/next
>
> all works fine on both arm32+arm64
>
>
> That whole iommu voodoo is a bit over my head right now, so I'm not sure
> what to poke to diagnose this.

Dang, this wasn't supposed to affect 32-bit Arm at all, since that
doesn't touch any of the default domain stuff either way. I have both my
RK3288 box (which IIRC doesn't currently boot) and an Odroid-U3 in the
"desk pile" right in front of me, so at worst I'll try bringing one of
those to life to see what silly thing I have indeed done to break 32-bit.

I have a vague idea forming already, which suggests that it might get
better again once patch #12 is applied, but even if so there's no excuse
not to be bisectable, so I need to dig in and fix it - many thanks for
yelling as requested :D

Robin.

>
>
> Heiko
>
>
>> Changes in v2:
>>
>> - Add iommu_is_dma_domain() helper to abstract flag check (and help
>> avoid silly typos like the one in v1).
>> - Tweak a few commit messages for spelling and (hopefully) clarity.
>> - Move the iommu_create_device_direct_mappings() update to patch #14
>> where it should have been.
>> - Rewrite patch #20 as a conversion of the now-existing option.
>> - Clean up the ops->flush_iotlb_all check which is also made redundant
>> by the new domain type
>> - Add patch #24, which is arguably tangential, but it was something I
>> spotted during the rebase, so...
>>
>> Once again, the whole lot is available on a branch here:
>>
>> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>>
>> Thanks,
>> Robin.
>>
>>
>> CC: Marek Szyprowski <[email protected]>
>> CC: Yoshihiro Shimoda <[email protected]>
>> CC: Geert Uytterhoeven <[email protected]>
>> CC: Yong Wu <[email protected]>
>> CC: Heiko Stuebner <[email protected]>
>> CC: Chunyan Zhang <[email protected]>
>> CC: Chunyan Zhang <[email protected]>
>> CC: Maxime Ripard <[email protected]>
>> CC: Jean-Philippe Brucker <[email protected]>
>>
>> Robin Murphy (24):
>> iommu: Pull IOVA cookie management into the core
>> iommu/amd: Drop IOVA cookie management
>> iommu/arm-smmu: Drop IOVA cookie management
>> iommu/vt-d: Drop IOVA cookie management
>> iommu/exynos: Drop IOVA cookie management
>> iommu/ipmmu-vmsa: Drop IOVA cookie management
>> iommu/mtk: Drop IOVA cookie management
>> iommu/rockchip: Drop IOVA cookie management
>> iommu/sprd: Drop IOVA cookie management
>> iommu/sun50i: Drop IOVA cookie management
>> iommu/virtio: Drop IOVA cookie management
>> iommu/dma: Unexport IOVA cookie management
>> iommu/dma: Remove redundant "!dev" checks
>> iommu: Introduce explicit type for non-strict DMA domains
>> iommu/amd: Prepare for multiple DMA domain types
>> iommu/arm-smmu: Prepare for multiple DMA domain types
>> iommu/vt-d: Prepare for multiple DMA domain types
>> iommu: Express DMA strictness via the domain type
>> iommu: Expose DMA domain strictness via sysfs
>> iommu: Merge strictness and domain type configs
>> iommu/dma: Factor out flush queue init
>> iommu: Allow enabling non-strict mode dynamically
>> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
>> iommu: Only log strictness for DMA domains
>>
>> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
>> drivers/iommu/Kconfig | 80 +++++++++----------
>> drivers/iommu/amd/iommu.c | 21 +----
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
>> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
>> drivers/iommu/dma-iommu.c | 44 +++++-----
>> drivers/iommu/exynos-iommu.c | 18 +----
>> drivers/iommu/intel/iommu.c | 23 ++----
>> drivers/iommu/iommu.c | 53 +++++++-----
>> drivers/iommu/ipmmu-vmsa.c | 27 +------
>> drivers/iommu/mtk_iommu.c | 6 --
>> drivers/iommu/rockchip-iommu.c | 11 +--
>> drivers/iommu/sprd-iommu.c | 6 --
>> drivers/iommu/sun50i-iommu.c | 12 +--
>> drivers/iommu/virtio-iommu.c | 8 --
>> include/linux/dma-iommu.h | 9 ++-
>> include/linux/iommu.h | 15 +++-
>> 18 files changed, 171 insertions(+), 226 deletions(-)
>>
>>
>
>
>
>

2021-07-29 15:55:27

by Heiko Stuebner

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

Am Donnerstag, 29. Juli 2021, 17:43:07 CEST schrieb Robin Murphy:
> On 2021-07-29 16:04, Heiko St?bner wrote:
> > Hi Robin,
> >
> > Am Mittwoch, 28. Juli 2021, 17:58:21 CEST schrieb Robin Murphy:
> >> Hi all,
> >>
> >> Here's v2 where things start to look more realistic, hence the expanded
> >> CC list. The patches are now based on the current iommu/core branch to
> >> take John's iommu_set_dma_strict() cleanup into account.
> >>
> >> The series remiains in two (or possibly 3) logical parts - for people
> >> CC'd on cookie cleanup patches, the later parts should not affect you
> >> since your drivers don't implement non-strict mode anyway; the cleanup
> >> is all pretty straightforward, but please do yell at me if I've managed
> >> to let a silly mistake slip through and broken your driver.
> >>
> >> This time I have also build-tested x86 as well as arm64 :)
> >
> > TL;DR: arm64 yay, arm32 nay ;-)
>
> Cheers Heiko!
>
> > testcase:
> > 5.14-rc3
> > + iommu/next
> > + patches 1+8 (the ones you cc'd me on)
> > iommu: Pull IOVA cookie management into the core
> > iommu/rockchip: Drop IOVA cookie management
> >
> > rk3399+hdmi (puma): boots with graphics
> > rk3399+edp (kevin): boots with graphics
> > px30+dsi (minievb): boots with graphics
> >
> > rk3288 (arm32, veyron-pinky): hangs when trying to start the rockchip-drm
> > at some points the rest of the system recovers and fills the log with
> >
> > [ 47.193776] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> > [ 47.193867] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:31:plane-0] commit wait timed out
> > [ 57.433743] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> > [ 57.433828] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:40:plane-4] commit wait timed out
> >
> > spews
> >
> > testcase 2:
> > 5.14-rc3
> > + iommu/next
> >
> > all works fine on both arm32+arm64
> >
> >
> > That whole iommu voodoo is a bit over my head right now, so I'm not sure
> > what to poke to diagnose this.
>
> Dang, this wasn't supposed to affect 32-bit Arm at all, since that
> doesn't touch any of the default domain stuff either way. I have both my
> RK3288 box (which IIRC doesn't currently boot) and an Odroid-U3 in the
> "desk pile" right in front of me, so at worst I'll try bringing one of
> those to life to see what silly thing I have indeed done to break 32-bit.
>
> I have a vague idea forming already, which suggests that it might get
> better again once patch #12 is applied, but even if so there's no excuse
> not to be bisectable, so I need to dig in and fix it - many thanks for
> yelling as requested :D

That vague idea was actually quite correct, applying
iommu/dma: Unexport IOVA cookie management
on top of the the two patches makes my rk3288 boot correctly again
and the display also works again.

Heiko

>
> Robin.
>
> >
> >
> > Heiko
> >
> >
> >> Changes in v2:
> >>
> >> - Add iommu_is_dma_domain() helper to abstract flag check (and help
> >> avoid silly typos like the one in v1).
> >> - Tweak a few commit messages for spelling and (hopefully) clarity.
> >> - Move the iommu_create_device_direct_mappings() update to patch #14
> >> where it should have been.
> >> - Rewrite patch #20 as a conversion of the now-existing option.
> >> - Clean up the ops->flush_iotlb_all check which is also made redundant
> >> by the new domain type
> >> - Add patch #24, which is arguably tangential, but it was something I
> >> spotted during the rebase, so...
> >>
> >> Once again, the whole lot is available on a branch here:
> >>
> >> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
> >>
> >> Thanks,
> >> Robin.
> >>
> >>
> >> CC: Marek Szyprowski <[email protected]>
> >> CC: Yoshihiro Shimoda <[email protected]>
> >> CC: Geert Uytterhoeven <[email protected]>
> >> CC: Yong Wu <[email protected]>
> >> CC: Heiko Stuebner <[email protected]>
> >> CC: Chunyan Zhang <[email protected]>
> >> CC: Chunyan Zhang <[email protected]>
> >> CC: Maxime Ripard <[email protected]>
> >> CC: Jean-Philippe Brucker <[email protected]>
> >>
> >> Robin Murphy (24):
> >> iommu: Pull IOVA cookie management into the core
> >> iommu/amd: Drop IOVA cookie management
> >> iommu/arm-smmu: Drop IOVA cookie management
> >> iommu/vt-d: Drop IOVA cookie management
> >> iommu/exynos: Drop IOVA cookie management
> >> iommu/ipmmu-vmsa: Drop IOVA cookie management
> >> iommu/mtk: Drop IOVA cookie management
> >> iommu/rockchip: Drop IOVA cookie management
> >> iommu/sprd: Drop IOVA cookie management
> >> iommu/sun50i: Drop IOVA cookie management
> >> iommu/virtio: Drop IOVA cookie management
> >> iommu/dma: Unexport IOVA cookie management
> >> iommu/dma: Remove redundant "!dev" checks
> >> iommu: Introduce explicit type for non-strict DMA domains
> >> iommu/amd: Prepare for multiple DMA domain types
> >> iommu/arm-smmu: Prepare for multiple DMA domain types
> >> iommu/vt-d: Prepare for multiple DMA domain types
> >> iommu: Express DMA strictness via the domain type
> >> iommu: Expose DMA domain strictness via sysfs
> >> iommu: Merge strictness and domain type configs
> >> iommu/dma: Factor out flush queue init
> >> iommu: Allow enabling non-strict mode dynamically
> >> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
> >> iommu: Only log strictness for DMA domains
> >>
> >> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
> >> drivers/iommu/Kconfig | 80 +++++++++----------
> >> drivers/iommu/amd/iommu.c | 21 +----
> >> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
> >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
> >> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
> >> drivers/iommu/dma-iommu.c | 44 +++++-----
> >> drivers/iommu/exynos-iommu.c | 18 +----
> >> drivers/iommu/intel/iommu.c | 23 ++----
> >> drivers/iommu/iommu.c | 53 +++++++-----
> >> drivers/iommu/ipmmu-vmsa.c | 27 +------
> >> drivers/iommu/mtk_iommu.c | 6 --
> >> drivers/iommu/rockchip-iommu.c | 11 +--
> >> drivers/iommu/sprd-iommu.c | 6 --
> >> drivers/iommu/sun50i-iommu.c | 12 +--
> >> drivers/iommu/virtio-iommu.c | 8 --
> >> include/linux/dma-iommu.h | 9 ++-
> >> include/linux/iommu.h | 15 +++-
> >> 18 files changed, 171 insertions(+), 226 deletions(-)
> >>
> >>
> >
> >
> >
> >
>

2021-07-29 16:31:30

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

On 2021-07-29 16:53, Heiko Stübner wrote:
> Am Donnerstag, 29. Juli 2021, 17:43:07 CEST schrieb Robin Murphy:
>> On 2021-07-29 16:04, Heiko Stübner wrote:
>>> Hi Robin,
>>>
>>> Am Mittwoch, 28. Juli 2021, 17:58:21 CEST schrieb Robin Murphy:
>>>> Hi all,
>>>>
>>>> Here's v2 where things start to look more realistic, hence the expanded
>>>> CC list. The patches are now based on the current iommu/core branch to
>>>> take John's iommu_set_dma_strict() cleanup into account.
>>>>
>>>> The series remiains in two (or possibly 3) logical parts - for people
>>>> CC'd on cookie cleanup patches, the later parts should not affect you
>>>> since your drivers don't implement non-strict mode anyway; the cleanup
>>>> is all pretty straightforward, but please do yell at me if I've managed
>>>> to let a silly mistake slip through and broken your driver.
>>>>
>>>> This time I have also build-tested x86 as well as arm64 :)
>>>
>>> TL;DR: arm64 yay, arm32 nay ;-)
>>
>> Cheers Heiko!
>>
>>> testcase:
>>> 5.14-rc3
>>> + iommu/next
>>> + patches 1+8 (the ones you cc'd me on)
>>> iommu: Pull IOVA cookie management into the core
>>> iommu/rockchip: Drop IOVA cookie management
>>>
>>> rk3399+hdmi (puma): boots with graphics
>>> rk3399+edp (kevin): boots with graphics
>>> px30+dsi (minievb): boots with graphics
>>>
>>> rk3288 (arm32, veyron-pinky): hangs when trying to start the rockchip-drm
>>> at some points the rest of the system recovers and fills the log with
>>>
>>> [ 47.193776] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
>>> [ 47.193867] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:31:plane-0] commit wait timed out
>>> [ 57.433743] [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
>>> [ 57.433828] [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [PLANE:40:plane-4] commit wait timed out
>>>
>>> spews
>>>
>>> testcase 2:
>>> 5.14-rc3
>>> + iommu/next
>>>
>>> all works fine on both arm32+arm64
>>>
>>>
>>> That whole iommu voodoo is a bit over my head right now, so I'm not sure
>>> what to poke to diagnose this.
>>
>> Dang, this wasn't supposed to affect 32-bit Arm at all, since that
>> doesn't touch any of the default domain stuff either way. I have both my
>> RK3288 box (which IIRC doesn't currently boot) and an Odroid-U3 in the
>> "desk pile" right in front of me, so at worst I'll try bringing one of
>> those to life to see what silly thing I have indeed done to break 32-bit.
>>
>> I have a vague idea forming already, which suggests that it might get
>> better again once patch #12 is applied, but even if so there's no excuse
>> not to be bisectable, so I need to dig in and fix it - many thanks for
>> yelling as requested :D
>
> That vague idea was actually quite correct, applying
> iommu/dma: Unexport IOVA cookie management
> on top of the the two patches makes my rk3288 boot correctly again
> and the display also works again.

Yup, since the !CONFIG_IOMMU_DMA stub for iommu_get_dma_cookie() returns
-ENODEV, rather than the -ENOMEM that the temporary special case is
expecting from the real function, it will inadvertently allow the
default domain to be created (when it wasn't before). I still have no
idea why that causes a problem though, since arm_iommu_attach_device()
should end up kicking a default domain out of the way even if one does
exist... :/

Either way I'll fix my bug - indeed it was an oversight that I hadn't
considered which exact error code the stub "fails" with - to avoid the
temporary change in behaviour, but I'll have to keep digging into the
arch/arm code and rockchip-iommu to see if something's also off there.

Cheers,
Robin.

2021-07-29 22:35:18

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

Hi,

On Wed, Jul 28, 2021 at 8:59 AM Robin Murphy <[email protected]> wrote:
>
> Hi all,
>
> Here's v2 where things start to look more realistic, hence the expanded
> CC list. The patches are now based on the current iommu/core branch to
> take John's iommu_set_dma_strict() cleanup into account.
>
> The series remiains in two (or possibly 3) logical parts - for people
> CC'd on cookie cleanup patches, the later parts should not affect you
> since your drivers don't implement non-strict mode anyway; the cleanup
> is all pretty straightforward, but please do yell at me if I've managed
> to let a silly mistake slip through and broken your driver.
>
> This time I have also build-tested x86 as well as arm64 :)
>
> Changes in v2:
>
> - Add iommu_is_dma_domain() helper to abstract flag check (and help
> avoid silly typos like the one in v1).
> - Tweak a few commit messages for spelling and (hopefully) clarity.
> - Move the iommu_create_device_direct_mappings() update to patch #14
> where it should have been.
> - Rewrite patch #20 as a conversion of the now-existing option.
> - Clean up the ops->flush_iotlb_all check which is also made redundant
> by the new domain type
> - Add patch #24, which is arguably tangential, but it was something I
> spotted during the rebase, so...
>
> Once again, the whole lot is available on a branch here:
>
> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>
> Thanks,
> Robin.
>
>
> CC: Marek Szyprowski <[email protected]>
> CC: Yoshihiro Shimoda <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: Yong Wu <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Maxime Ripard <[email protected]>
> CC: Jean-Philippe Brucker <[email protected]>
>
> Robin Murphy (24):
> iommu: Pull IOVA cookie management into the core
> iommu/amd: Drop IOVA cookie management
> iommu/arm-smmu: Drop IOVA cookie management
> iommu/vt-d: Drop IOVA cookie management
> iommu/exynos: Drop IOVA cookie management
> iommu/ipmmu-vmsa: Drop IOVA cookie management
> iommu/mtk: Drop IOVA cookie management
> iommu/rockchip: Drop IOVA cookie management
> iommu/sprd: Drop IOVA cookie management
> iommu/sun50i: Drop IOVA cookie management
> iommu/virtio: Drop IOVA cookie management
> iommu/dma: Unexport IOVA cookie management
> iommu/dma: Remove redundant "!dev" checks
> iommu: Introduce explicit type for non-strict DMA domains
> iommu/amd: Prepare for multiple DMA domain types
> iommu/arm-smmu: Prepare for multiple DMA domain types
> iommu/vt-d: Prepare for multiple DMA domain types
> iommu: Express DMA strictness via the domain type
> iommu: Expose DMA domain strictness via sysfs
> iommu: Merge strictness and domain type configs
> iommu/dma: Factor out flush queue init
> iommu: Allow enabling non-strict mode dynamically
> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
> iommu: Only log strictness for DMA domains
>
> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
> drivers/iommu/Kconfig | 80 +++++++++----------
> drivers/iommu/amd/iommu.c | 21 +----
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
> drivers/iommu/dma-iommu.c | 44 +++++-----
> drivers/iommu/exynos-iommu.c | 18 +----
> drivers/iommu/intel/iommu.c | 23 ++----
> drivers/iommu/iommu.c | 53 +++++++-----
> drivers/iommu/ipmmu-vmsa.c | 27 +------
> drivers/iommu/mtk_iommu.c | 6 --
> drivers/iommu/rockchip-iommu.c | 11 +--
> drivers/iommu/sprd-iommu.c | 6 --
> drivers/iommu/sun50i-iommu.c | 12 +--
> drivers/iommu/virtio-iommu.c | 8 --
> include/linux/dma-iommu.h | 9 ++-
> include/linux/iommu.h | 15 +++-
> 18 files changed, 171 insertions(+), 226 deletions(-)

I ran with:

a) mainline Linux (at commit 4010a528219e)
b) pulled iommu/next (at commit 9be9f5580ab6)
c) picked from patchwork your series

...and I ran on sc7180-trogdor-lazor.

Things worked OK and I could transition my eMMC to non-strict mode with:

echo DMA-FQ > /sys/devices/platform/soc@0/7c4000.sdhci/iommu_group/type

I was definitely getting some inconsistencies in my tests where the
eMMC speeds were getting into a bad state, but I don't believe it's
related to your patch series. I could transition myself back to strict
DMA with this (only got one unrelated warn splat about
dev_pm_opp_put_clkname when unbinding) because I was booted up from
USB for testing:

cd /sys/bus/mmc/drivers/mmcblk
echo mmc1:0001 > unbind
cd /sys/bus/platform/drivers/sdhci_msm/
echo 7c4000.sdhci > unbind
echo DMA > /sys/devices/platform/soc@0/7c4000.sdhci/iommu_group/type
echo 7c4000.sdhci > bind

...and it was consistently faster with non-strict than with strict so
whatever bad state I sometimes managed to get in it affected both
modes. ;-)

So I guess that's a long-winded way to say this:

Tested-by: Douglas Anderson <[email protected]>

2021-07-30 00:08:59

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

Hi,

On Thu, Jul 29, 2021 at 3:33 PM Doug Anderson <[email protected]> wrote:
>
> I was definitely getting some inconsistencies in my tests where the
> eMMC speeds were getting into a bad state, but I don't believe it's
> related to your patch series.

I think this was just me being an idiot. I forgot that I'd been
running with KASAN, so that explains why my speeds were so much slower
than usual and probably also explains how it could get in a bad state
(I guess it also explains why sugov was eating up 30% of my CPU time
since that went away too!). No mystery here aside from why it took me
this long to realize it.

I'm now getting ~213 MB/s without forcing it to lazy and ~261 MB/s
with forcing it to lazy through sysfs (and without any other cpufreq
hacks).

-Doug

2021-07-30 01:22:26

by chenxiang (M)

[permalink] [raw]

Subject: Re: [PATCH v2 00/24] iommu: Refactor DMA domain strictness

在 2021/7/29 18:59, Robin Murphy 写道:
> On 2021-07-29 03:55, chenxiang (M) wrote:
>> Hi Robin,
>>
>>
>> 在 2021/7/28 23:58, Robin Murphy 写道:
>>> Hi all,
>>>
>>> Here's v2 where things start to look more realistic, hence the expanded
>>> CC list. The patches are now based on the current iommu/core branch to
>>> take John's iommu_set_dma_strict() cleanup into account.
>>>
>>> The series remiains in two (or possibly 3) logical parts - for people
>>> CC'd on cookie cleanup patches, the later parts should not affect you
>>> since your drivers don't implement non-strict mode anyway; the cleanup
>>> is all pretty straightforward, but please do yell at me if I've managed
>>> to let a silly mistake slip through and broken your driver.
>>>
>>> This time I have also build-tested x86 as well as arm64 :)
>>
>> I have tested those patchset on ARM64 with SMMUV3, and the testcases
>> are as follows:
>> - Boot with iommu.strict=0, running fio and it works well;
>> - Boot with iommu.strict=1, running fio and it works well;
>> - Change strict mode to lazy mode when building, the change takes
>> effect;
>> - Boot without iommu.strict(default strict mode), change the sysfs
>> interface type from DMA to DMA-FQ dynamically during running fio, and
>> it works well;
>> - Boot without iommu.strict(default strict mode), change the sysfs
>> interface type from DMA-FQ to DMA dynamically, and it is not allowed
>> and print "Device or resource busy"
>> (i know it is qualified, and we can change no-strict mode to strict
>> by unbind the driver -> change the sysfs interface (type)->bind the
>> driver (tested this and it works well),
>> but i have a small question: is it also possible to change from
>> DMA-FQ to DMA dynamically? )
>
> As patch #22 mentions, I think it's possible in principle, but it's
> certainly trickier. When enabling a flush queue, it doesn't matter if
> it takes a while for other threads to notice that cookie->fq_domain is
> now set and stop doing synchronous invalidations (and in the SMMU case
> it seems like there are probably enough dependencies to additionally
> prevent the io_pgtable quirk being observable before that). However
> when disabling, we'd need to be absolutely sure that the driver *has*
> started invalidating strictly before we stop queueing freed IOVAs,
> plus we need to be absolutely sure that we've stopped queueing freed
> IOVAs before we attempt to tear down the flush queue itself. I'm not
> sure off-hand how feasible it would be to put all that synchronisation
> in the right places without it also impacting normal operation.
>
> Furthermore, as also noted, there doesn't seem to be a good reason for
> ever actually needing to do that. If a device isn't trusted, it should
> be given a strict domain *before* any driver has a chance to start
> doing anything, or your trust model is broken and pretty useless. I
> can imagine some niche debugging/benchmarking cases where it might
> help save a bit of effort, but nothing with a strong enough
> justification to be worth supporting in mainline.

Ok, thanks.

>
>> Anyway, please feel free to add :
>> Tested-by: Xiang Chen <[email protected]>
>
> That's great, thanks!
>
> Robin.
>
>>> Changes in v2:
>>>
>>> - Add iommu_is_dma_domain() helper to abstract flag check (and help
>>> avoid silly typos like the one in v1).
>>> - Tweak a few commit messages for spelling and (hopefully) clarity.
>>> - Move the iommu_create_device_direct_mappings() update to patch #14
>>> where it should have been.
>>> - Rewrite patch #20 as a conversion of the now-existing option.
>>> - Clean up the ops->flush_iotlb_all check which is also made redundant
>>> by the new domain type
>>> - Add patch #24, which is arguably tangential, but it was something I
>>> spotted during the rebase, so...
>>>
>>> Once again, the whole lot is available on a branch here:
>>>
>>> https://gitlab.arm.com/linux-arm/linux-rm/-/tree/iommu/fq
>>>
>>> Thanks,
>>> Robin.
>>>
>>>
>>> CC: Marek Szyprowski <[email protected]>
>>> CC: Yoshihiro Shimoda <[email protected]>
>>> CC: Geert Uytterhoeven <[email protected]>
>>> CC: Yong Wu <[email protected]>
>>> CC: Heiko Stuebner <[email protected]>
>>> CC: Chunyan Zhang <[email protected]>
>>> CC: Chunyan Zhang <[email protected]>
>>> CC: Maxime Ripard <[email protected]>
>>> CC: Jean-Philippe Brucker <[email protected]>
>>>
>>> Robin Murphy (24):
>>> iommu: Pull IOVA cookie management into the core
>>> iommu/amd: Drop IOVA cookie management
>>> iommu/arm-smmu: Drop IOVA cookie management
>>> iommu/vt-d: Drop IOVA cookie management
>>> iommu/exynos: Drop IOVA cookie management
>>> iommu/ipmmu-vmsa: Drop IOVA cookie management
>>> iommu/mtk: Drop IOVA cookie management
>>> iommu/rockchip: Drop IOVA cookie management
>>> iommu/sprd: Drop IOVA cookie management
>>> iommu/sun50i: Drop IOVA cookie management
>>> iommu/virtio: Drop IOVA cookie management
>>> iommu/dma: Unexport IOVA cookie management
>>> iommu/dma: Remove redundant "!dev" checks
>>> iommu: Introduce explicit type for non-strict DMA domains
>>> iommu/amd: Prepare for multiple DMA domain types
>>> iommu/arm-smmu: Prepare for multiple DMA domain types
>>> iommu/vt-d: Prepare for multiple DMA domain types
>>> iommu: Express DMA strictness via the domain type
>>> iommu: Expose DMA domain strictness via sysfs
>>> iommu: Merge strictness and domain type configs
>>> iommu/dma: Factor out flush queue init
>>> iommu: Allow enabling non-strict mode dynamically
>>> iommu/arm-smmu: Allow non-strict in pgtable_quirks interface
>>> iommu: Only log strictness for DMA domains
>>>
>>> .../ABI/testing/sysfs-kernel-iommu_groups | 2 +
>>> drivers/iommu/Kconfig | 80
>>> +++++++++----------
>>> drivers/iommu/amd/iommu.c | 21 +----
>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 25 ++++--
>>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 29 ++++---
>>> drivers/iommu/arm/arm-smmu/qcom_iommu.c | 8 --
>>> drivers/iommu/dma-iommu.c | 44 +++++-----
>>> drivers/iommu/exynos-iommu.c | 18 +----
>>> drivers/iommu/intel/iommu.c | 23 ++----
>>> drivers/iommu/iommu.c | 53 +++++++-----
>>> drivers/iommu/ipmmu-vmsa.c | 27 +------
>>> drivers/iommu/mtk_iommu.c | 6 --
>>> drivers/iommu/rockchip-iommu.c | 11 +--
>>> drivers/iommu/sprd-iommu.c | 6 --
>>> drivers/iommu/sun50i-iommu.c | 12 +--
>>> drivers/iommu/virtio-iommu.c | 8 --
>>> include/linux/dma-iommu.h | 9 ++-
>>> include/linux/iommu.h | 15 +++-
>>> 18 files changed, 171 insertions(+), 226 deletions(-)
>>>
>>
>>
>
> .
>

2021-07-30 06:10:02

[permalink] [raw]

Subject: Re: [PATCH v2 01/24] iommu: Pull IOVA cookie management into the core

On 7/28/21 11:58 PM, Robin Murphy wrote:
> Now that everyone has converged on iommu-dma for IOMMU_DOMAIN_DMA
> support, we can abandon the notion of drivers being responsible for the
> cookie type, and consolidate all the management into the core code.
>
> CC: Marek Szyprowski <[email protected]>
> CC: Yoshihiro Shimoda <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: Yong Wu <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Maxime Ripard <[email protected]>
> CC: Jean-Philippe Brucker <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommu.c | 7 +++++++
> include/linux/iommu.h | 3 ++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2cda9950bd5..ea5a9ea8d431 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -7,6 +7,7 @@
> #define pr_fmt(fmt) "iommu: " fmt
>
> #include <linux/device.h>
> +#include <linux/dma-iommu.h>
> #include <linux/kernel.h>
> #include <linux/bits.h>
> #include <linux/bug.h>
> @@ -1946,6 +1947,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> /* Assume all sizes by default; the driver may override this later */
> domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>
> + /* Temporarily ignore -EEXIST while drivers still get their own cookies */
> + if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain) == -ENOMEM) {
> + iommu_domain_free(domain);
> + domain = NULL;
> + }
> return domain;
> }
>
> @@ -1957,6 +1963,7 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>
> void iommu_domain_free(struct iommu_domain *domain)
> {
> + iommu_put_dma_cookie(domain);
> domain->ops->domain_free(domain);
> }
> EXPORT_SYMBOL_GPL(iommu_domain_free);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 4997c78e2670..141779d76035 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -40,6 +40,7 @@ struct iommu_domain;
> struct notifier_block;
> struct iommu_sva;
> struct iommu_fault_event;
> +struct iommu_dma_cookie;
>
> /* iommu fault flags */
> #define IOMMU_FAULT_READ 0x0
> @@ -86,7 +87,7 @@ struct iommu_domain {
> iommu_fault_handler_t handler;
> void *handler_token;
> struct iommu_domain_geometry geometry;
> - void *iova_cookie;
> + struct iommu_dma_cookie *iova_cookie;
> };
>
> enum iommu_cap {
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:10:59

[permalink] [raw]

Subject: Re: [PATCH v2 12/24] iommu/dma: Unexport IOVA cookie management

On 7/28/21 11:58 PM, Robin Murphy wrote:
> IOVA cookies are now got and put by core code, so we no longer need to
> export these to modular drivers. The export for getting MSI cookies
> stays, since VFIO can still be a module, but it was already relying on
> someone else putting them, so that aspect is unaffected.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 7 -------
> drivers/iommu/iommu.c | 3 +--
> 2 files changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 98ba927aee1a..10067fbc4309 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -98,9 +98,6 @@ static struct iommu_dma_cookie *cookie_alloc(enum iommu_dma_cookie_type type)
> /**
> * iommu_get_dma_cookie - Acquire DMA-API resources for a domain
> * @domain: IOMMU domain to prepare for DMA-API usage
> - *
> - * IOMMU drivers should normally call this from their domain_alloc
> - * callback when domain->type == IOMMU_DOMAIN_DMA.
> */
> int iommu_get_dma_cookie(struct iommu_domain *domain)
> {
> @@ -113,7 +110,6 @@ int iommu_get_dma_cookie(struct iommu_domain *domain)
>
> return 0;
> }
> -EXPORT_SYMBOL(iommu_get_dma_cookie);
>
> /**
> * iommu_get_msi_cookie - Acquire just MSI remapping resources
> @@ -151,8 +147,6 @@ EXPORT_SYMBOL(iommu_get_msi_cookie);
> * iommu_put_dma_cookie - Release a domain's DMA mapping resources
> * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie() or
> * iommu_get_msi_cookie()
> - *
> - * IOMMU drivers should normally call this from their domain_free callback.
> */
> void iommu_put_dma_cookie(struct iommu_domain *domain)
> {
> @@ -172,7 +166,6 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
> kfree(cookie);
> domain->iova_cookie = NULL;
> }
> -EXPORT_SYMBOL(iommu_put_dma_cookie);
>
> /**
> * iommu_dma_get_resv_regions - Reserved region driver helper
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ea5a9ea8d431..fa8109369f74 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1947,8 +1947,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> /* Assume all sizes by default; the driver may override this later */
> domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>
> - /* Temporarily ignore -EEXIST while drivers still get their own cookies */
> - if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain) == -ENOMEM) {
> + if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
> iommu_domain_free(domain);
> domain = NULL;
> }
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:11:29

[permalink] [raw]

Subject: Re: [PATCH v2 04/24] iommu/vt-d: Drop IOVA cookie management

On 7/28/21 11:58 PM, Robin Murphy wrote:
> The core code bakes its own cookies now.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index c12cc955389a..7e168634c433 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1979,10 +1979,6 @@ static void domain_exit(struct dmar_domain *domain)
> /* Remove associated devices and clear attached or cached domains */
> domain_remove_dev_info(domain);
>
> - /* destroy iovas */
> - if (domain->domain.type == IOMMU_DOMAIN_DMA)
> - iommu_put_dma_cookie(&domain->domain);
> -
> if (domain->pgd) {
> struct page *freelist;
>
> @@ -4544,10 +4540,6 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
> return NULL;
> }
>
> - if (type == IOMMU_DOMAIN_DMA &&
> - iommu_get_dma_cookie(&dmar_domain->domain))
> - return NULL;
> -
> domain = &dmar_domain->domain;
> domain->geometry.aperture_start = 0;
> domain->geometry.aperture_end =
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:11:40

[permalink] [raw]

Subject: Re: [PATCH v2 13/24] iommu/dma: Remove redundant "!dev" checks

On 7/28/21 11:58 PM, Robin Murphy wrote:
> iommu_dma_init_domain() is now only called from iommu_setup_dma_ops(),
> which has already assumed dev to be non-NULL.
>
> Reviewed-by: John Garry <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 10067fbc4309..e28396cea6eb 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -363,7 +363,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>
> init_iova_domain(iovad, 1UL << order, base_pfn);
>
> - if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) &&
> + if (!cookie->fq_domain && !dev_is_untrusted(dev) &&
> domain->ops->flush_iotlb_all && !iommu_get_dma_strict(domain)) {
> if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> iommu_dma_entry_dtor))
> @@ -372,9 +372,6 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
> cookie->fq_domain = domain;
> }
>
> - if (!dev)
> - return 0;
> -
> return iova_reserve_iommu_regions(dev, domain);
> }
>
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:11:58

[permalink] [raw]

Subject: Re: [PATCH v2 14/24] iommu: Introduce explicit type for non-strict DMA domains

On 7/28/21 11:58 PM, Robin Murphy wrote:
> Promote the difference between strict and non-strict DMA domains from an
> internal detail to a distinct domain feature and type, to pave the road
> for exposing it through the sysfs default domain interface.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 2 +-
> drivers/iommu/iommu.c | 8 ++++++--
> include/linux/iommu.h | 11 +++++++++++
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index e28396cea6eb..8b3545c01077 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1311,7 +1311,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
> * The IOMMU core code allocates the default DMA domain, which the
> * underlying IOMMU driver needs to support via the dma-iommu layer.
> */
> - if (domain->type == IOMMU_DOMAIN_DMA) {
> + if (iommu_is_dma_domain(domain)) {
> if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
> goto out_err;
> dev->dma_ops = &iommu_dma_ops;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index fa8109369f74..982545234cf3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -115,6 +115,7 @@ static const char *iommu_domain_type_str(unsigned int t)
> case IOMMU_DOMAIN_UNMANAGED:
> return "Unmanaged";
> case IOMMU_DOMAIN_DMA:
> + case IOMMU_DOMAIN_DMA_FQ:
> return "Translated";
> default:
> return "Unknown";
> @@ -552,6 +553,9 @@ static ssize_t iommu_group_show_type(struct iommu_group *group,
> case IOMMU_DOMAIN_DMA:
> type = "DMA\n";
> break;
> + case IOMMU_DOMAIN_DMA_FQ:
> + type = "DMA-FQ\n";
> + break;
> }
> }
> mutex_unlock(&group->mutex);
> @@ -765,7 +769,7 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
> unsigned long pg_size;
> int ret = 0;
>
> - if (!domain || domain->type != IOMMU_DOMAIN_DMA)
> + if (!domain || !iommu_is_dma_domain(domain))
> return 0;
>
> BUG_ON(!domain->pgsize_bitmap);
> @@ -1947,7 +1951,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> /* Assume all sizes by default; the driver may override this later */
> domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>
> - if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
> + if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
> iommu_domain_free(domain);
> domain = NULL;
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 141779d76035..046ba4d54cd2 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -61,6 +61,7 @@ struct iommu_domain_geometry {
> #define __IOMMU_DOMAIN_DMA_API (1U << 1) /* Domain for use in DMA-API
> implementation */
> #define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
> +#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
>
> /*
> * This are the possible domain-types
> @@ -73,12 +74,17 @@ struct iommu_domain_geometry {
> * IOMMU_DOMAIN_DMA - Internally used for DMA-API implementations.
> * This flag allows IOMMU drivers to implement
> * certain optimizations for these domains
> + * IOMMU_DOMAIN_DMA_FQ - As above, but definitely using batched TLB
> + * invalidation.
> */
> #define IOMMU_DOMAIN_BLOCKED (0U)
> #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT)
> #define IOMMU_DOMAIN_UNMANAGED (__IOMMU_DOMAIN_PAGING)
> #define IOMMU_DOMAIN_DMA (__IOMMU_DOMAIN_PAGING | \
> __IOMMU_DOMAIN_DMA_API)
> +#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
> + __IOMMU_DOMAIN_DMA_API | \
> + __IOMMU_DOMAIN_DMA_FQ)
>
> struct iommu_domain {
> unsigned type;
> @@ -90,6 +96,11 @@ struct iommu_domain {
> struct iommu_dma_cookie *iova_cookie;
> };
>
> +static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> +{
> + return domain->type & __IOMMU_DOMAIN_DMA_API;
> +}
> +
> enum iommu_cap {
> IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
> transactions */
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:13:28

[permalink] [raw]

Subject: Re: [PATCH v2 17/24] iommu/vt-d: Prepare for multiple DMA domain types

On 7/28/21 11:58 PM, Robin Murphy wrote:
> In preparation for the strict vs. non-strict decision for DMA domains
> to be expressed in the domain type, make sure we expose our flush queue
> awareness by accepting the new domain type, and test the specific
> feature flag where we want to identify DMA domains in general. The DMA
> ops reset/setup can simply be made unconditional, since iommu-dma
> already knows only to touch DMA domains.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 7e168634c433..8fc46c9d6b96 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -582,7 +582,7 @@ struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
> int iommu_id;
>
> /* si_domain and vm domain should not get here. */
> - if (WARN_ON(domain->domain.type != IOMMU_DOMAIN_DMA))
> + if (WARN_ON(!iommu_is_dma_domain(&domain->domain)))
> return NULL;
>
> for_each_domain_iommu(iommu_id, domain)
> @@ -1034,7 +1034,7 @@ static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
> pteval = ((uint64_t)virt_to_dma_pfn(tmp_page) << VTD_PAGE_SHIFT) | DMA_PTE_READ | DMA_PTE_WRITE;
> if (domain_use_first_level(domain)) {
> pteval |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
> - if (domain->domain.type == IOMMU_DOMAIN_DMA)
> + if (iommu_is_dma_domain(&domain->domain))
> pteval |= DMA_FL_PTE_ACCESS;
> }
> if (cmpxchg64(&pte->val, 0ULL, pteval))
> @@ -2345,7 +2345,7 @@ __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
> if (domain_use_first_level(domain)) {
> attr |= DMA_FL_PTE_XD | DMA_FL_PTE_US;
>
> - if (domain->domain.type == IOMMU_DOMAIN_DMA) {
> + if (iommu_is_dma_domain(&domain->domain)) {
> attr |= DMA_FL_PTE_ACCESS;
> if (prot & DMA_PTE_WRITE)
> attr |= DMA_FL_PTE_DIRTY;
> @@ -4528,6 +4528,7 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
>
> switch (type) {
> case IOMMU_DOMAIN_DMA:
> + case IOMMU_DOMAIN_DMA_FQ:
> case IOMMU_DOMAIN_UNMANAGED:
> dmar_domain = alloc_domain(0);
> if (!dmar_domain) {
> @@ -5197,12 +5198,8 @@ static void intel_iommu_release_device(struct device *dev)
>
> static void intel_iommu_probe_finalize(struct device *dev)
> {
> - struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> -
> - if (domain && domain->type == IOMMU_DOMAIN_DMA)
> - iommu_setup_dma_ops(dev, 0, U64_MAX);
> - else
> - set_dma_ops(dev, NULL);
> + set_dma_ops(dev, NULL);
> + iommu_setup_dma_ops(dev, 0, U64_MAX);
> }
>
> static void intel_iommu_get_resv_regions(struct device *device,
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:13:35

[permalink] [raw]

Subject: Re: [PATCH v2 18/24] iommu: Express DMA strictness via the domain type

On 7/29/21 5:36 PM, Robin Murphy wrote:
> On 2021-07-29 08:13, Lu Baolu wrote:
>> Hi Robin,
>>
>> On 7/28/21 11:58 PM, Robin Murphy wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 982545234cf3..eecb5657de69 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void)
>>>           }
>>>       }
>>> +    if (!iommu_default_passthrough() && !iommu_dma_strict)
>>> +        iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ;
>>
>> iommu_dma_strict could be changed later by the vendor iommu driver via
>> iommu_set_dma_strict(). This seems not to be the right place to set
>> iommu_def_domain_type.
>
> Ah yes, good catch once again, thanks!
>
> I think this *is* the right place to initially set it to honour the
> command-line option, since that matches what we do for passthrough.
> However also like passthrough we'll need to keep things in sync if it's
> updated later, like this:
>
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 87d7b299436e..593d4555bc57 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup);
> void iommu_set_dma_strict(void)
> {
>         iommu_dma_strict = true;
> +       if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ)
> +               iommu_def_domain_type = IOMMU_DOMAIN_DMA;
> }
>
> static ssize_t iommu_group_attr_show(struct kobject *kobj,
>
>
> Does that seem reasonable? I'm not sure there's any cleaner way to do it
> since we don't want to inadvertently clobber the default type if the
> user has given us something funky like "intel_iommu=strict
> iommu.passthrough=1".
>
> Cheers,
> Robin.
>
>>
>>> +
>>>       pr_info("Default domain type: %s %s\n",
>>>           iommu_domain_type_str(iommu_def_domain_type),
>>>           (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
>>
>> Best regards,
>> baolu

With above fixed,

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:13:53

[permalink] [raw]

Subject: Re: [PATCH v2 19/24] iommu: Expose DMA domain strictness via sysfs

On 7/28/21 11:58 PM, Robin Murphy wrote:
> The sysfs interface for default domain types exists primarily so users
> can choose the performance/security tradeoff relevant to their own
> workload. As such, the choice between the policies for DMA domains fits
> perfectly as an additional point on that scale - downgrading a
> particular device from a strict default to non-strict may be enough to
> let it reach the desired level of performance, while still retaining
> more peace of mind than with a wide-open identity domain. Now that we've
> abstracted non-strict mode as a distinct type of DMA domain, allow it to
> be chosen through the user interface as well.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-kernel-iommu_groups | 2 ++
> drivers/iommu/iommu.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> index eae2f1c1e11e..43ba764ba5b7 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> +++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> @@ -42,6 +42,8 @@ Description: /sys/kernel/iommu_groups/<grp_id>/type shows the type of default
> ======== ======================================================
> DMA All the DMA transactions from the device in this group
> are translated by the iommu.
> + DMA-FQ As above, but using batched invalidation to lazily
> + remove translations after use.
> identity All the DMA transactions from the device in this group
> are not translated by the iommu.
> auto Change to the type the device was booted with.
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index eecb5657de69..5a08e0806cbb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3265,6 +3265,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> req_type = IOMMU_DOMAIN_IDENTITY;
> else if (sysfs_streq(buf, "DMA"))
> req_type = IOMMU_DOMAIN_DMA;
> + else if (sysfs_streq(buf, "DMA-FQ"))
> + req_type = IOMMU_DOMAIN_DMA_FQ;
> else if (sysfs_streq(buf, "auto"))
> req_type = 0;
> else
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:14:21

[permalink] [raw]

Subject: Re: [PATCH v2 20/24] iommu: Merge strictness and domain type configs

On 7/28/21 11:58 PM, Robin Murphy wrote:
> To parallel the sysfs behaviour, merge the new build-time option
> for DMA domain strictness into the default domain type choice.
>
> Suggested-by: Joerg Roedel <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/Kconfig | 80 +++++++++++++++++++++----------------------
> drivers/iommu/iommu.c | 2 +-
> 2 files changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index c84da8205be7..6e06f876d75a 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -79,55 +79,55 @@ config IOMMU_DEBUGFS
> debug/iommu directory, and then populate a subdirectory with
> entries as required.
>
> -config IOMMU_DEFAULT_PASSTHROUGH
> - bool "IOMMU passthrough by default"
> - depends on IOMMU_API
> - help
> - Enable passthrough by default, removing the need to pass in
> - iommu.passthrough=on or iommu=pt through command line. If this
> - is enabled, you can still disable with iommu.passthrough=off
> - or iommu=nopt depending on the architecture.
> -
> - If unsure, say N here.
> -
> choice
> - prompt "IOMMU default DMA IOTLB invalidation mode"
> - depends on IOMMU_DMA
> -
> - default IOMMU_DEFAULT_LAZY if (AMD_IOMMU || INTEL_IOMMU)
> - default IOMMU_DEFAULT_STRICT
> + prompt "IOMMU default domain type"
> + depends on IOMMU_API
> + default IOMMU_DEFAULT_DMA_LAZY if AMD_IOMMU || INTEL_IOMMU
> + default IOMMU_DEFAULT_DMA_STRICT
> help
> - This option allows an IOMMU DMA IOTLB invalidation mode to be
> - chosen at build time, to override the default mode of each ARCH,
> - removing the need to pass in kernel parameters through command line.
> - It is still possible to provide common boot params to override this
> - config.
> + Choose the type of IOMMU domain used to manage DMA API usage by
> + device drivers. The options here typically represent different
> + levels of tradeoff between robustness/security and performance,
> + depending on the IOMMU driver. Not all IOMMUs support all options.
> + This choice can be overridden at boot via the command line, and for
> + some devices also at runtime via sysfs.
>
> If unsure, keep the default.
>
> -config IOMMU_DEFAULT_STRICT
> - bool "strict"
> +config IOMMU_DEFAULT_DMA_STRICT
> + bool "Translated - Strict"
> help
> - For every IOMMU DMA unmap operation, the flush operation of IOTLB and
> - the free operation of IOVA are guaranteed to be done in the unmap
> - function.
> + Trusted devices use translation to restrict their access to only
> + DMA-mapped pages, with strict TLB invalidation on unmap. Equivalent
> + to passing "iommu.passthrough=0 iommu.strict=1" on the command line.
>
> -config IOMMU_DEFAULT_LAZY
> - bool "lazy"
> + Untrusted devices always use this mode, with an additional layer of
> + bounce-buffering such that they cannot gain access to any unrelated
> + data within a mapped page.
> +
> +config IOMMU_DEFAULT_DMA_LAZY
> + bool "Translated - Lazy"
> help
> - Support lazy mode, where for every IOMMU DMA unmap operation, the
> - flush operation of IOTLB and the free operation of IOVA are deferred.
> - They are only guaranteed to be done before the related IOVA will be
> - reused.
> + Trusted devices use translation to restrict their access to only
> + DMA-mapped pages, but with "lazy" batched TLB invalidation. This
> + mode allows higher performance with some IOMMUs due to reduced TLB
> + flushing, but at the cost of reduced isolation since devices may be
> + able to access memory for some time after it has been unmapped.
> + Equivalent to passing "iommu.passthrough=0 iommu.strict=0" on the
> + command line.
>
> - The isolation provided in this mode is not as secure as STRICT mode,
> - such that a vulnerable time window may be created between the DMA
> - unmap and the mappings cached in the IOMMU IOTLB or device TLB
> - finally being invalidated, where the device could still access the
> - memory which has already been unmapped by the device driver.
> - However this mode may provide better performance in high throughput
> - scenarios, and is still considerably more secure than passthrough
> - mode or no IOMMU.
> + If this mode is not supported by the IOMMU driver, the effective
> + runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.
> +
> +config IOMMU_DEFAULT_PASSTHROUGH
> + bool "Passthrough"
> + help
> + Trusted devices are identity-mapped, giving them unrestricted access
> + to memory with minimal performance overhead. Equivalent to passing
> + "iommu.passthrough=1" (historically "iommu=pt") on the command line.
> +
> + If this mode is not supported by the IOMMU driver, the effective
> + runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.
>
> endchoice
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 5a08e0806cbb..25c1adc1ec67 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -31,7 +31,7 @@ static struct kset *iommu_group_kset;
> static DEFINE_IDA(iommu_group_ida);
>
> static unsigned int iommu_def_domain_type __read_mostly;
> -static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_STRICT);
> +static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
> static u32 iommu_cmd_line __read_mostly;
>
> struct iommu_group {
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:15:21

[permalink] [raw]

Subject: Re: [PATCH v2 22/24] iommu: Allow enabling non-strict mode dynamically

On 7/28/21 11:58 PM, Robin Murphy wrote:
> Allocating and enabling a flush queue is in fact something we can
> reasonably do while a DMA domain is active, without having to rebuild it
> from scratch. Thus we can allow a strict -> non-strict transition from
> sysfs without requiring to unbind the device's driver, which is of
> particular interest to users who want to make selective relaxations to
> critical devices like the one serving their root filesystem.
>
> Disabling and draining a queue also seems technically possible to
> achieve without rebuilding the whole domain, but would certainly be more
> involved. Furthermore there's not such a clear use-case for tightening
> up security *after* the device may already have done whatever it is that
> you don't trust it not to do, so we only consider the relaxation case.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommu.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 25c1adc1ec67..be399d630953 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3200,6 +3200,13 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
> goto out;
> }
>
> + /* We can bring up a flush queue without tearing down the domain */
> + if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
> + prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
> + ret = iommu_dma_init_fq(prev_dom);
> + goto out;
> + }
> +
> /* Sets group->default_domain to the newly allocated domain */
> ret = iommu_group_alloc_default_domain(dev->bus, group, type);
> if (ret)
> @@ -3240,9 +3247,9 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
> }
>
> /*
> - * Changing the default domain through sysfs requires the users to ubind the
> - * drivers from the devices in the iommu group. Return failure if this doesn't
> - * meet.
> + * Changing the default domain through sysfs requires the users to unbind the
> + * drivers from the devices in the iommu group, except for a DMA -> DMA-FQ
> + * transition. Return failure if this isn't met.
> *
> * We need to consider the race between this and the device release path.
> * device_lock(dev) is used here to guarantee that the device release path
> @@ -3318,7 +3325,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
>
> /* Check if the device in the group still has a driver bound to it */
> device_lock(dev);
> - if (device_is_bound(dev)) {
> + if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
> + group->default_domain->type == IOMMU_DOMAIN_DMA)) {
> pr_err_ratelimited("Device is still bound to driver\n");
> ret = -EBUSY;
> goto out;
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:15:41

[permalink] [raw]

Subject: Re: [PATCH v2 21/24] iommu/dma: Factor out flush queue init

On 7/28/21 11:58 PM, Robin Murphy wrote:
> Factor out flush queue setup from the initial domain init so that we
> can potentially trigger it from sysfs later on in a domain's lifetime.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/dma-iommu.c | 30 ++++++++++++++++++++----------
> include/linux/dma-iommu.h | 9 ++++++---
> 2 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 7f3968865387..304a3ec71223 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
> return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
> }
>
> +int iommu_dma_init_fq(struct iommu_domain *domain)
> +{
> + struct iommu_dma_cookie *cookie = domain->iova_cookie;
> +
> + if (domain->type != IOMMU_DOMAIN_DMA_FQ)
> + return -EINVAL;
> + if (cookie->fq_domain)
> + return 0;
> +
> + if (init_iova_flush_queue(&cookie->iovad, iommu_dma_flush_iotlb_all,
> + iommu_dma_entry_dtor)) {
> + pr_warn("iova flush queue initialization failed\n");
> + domain->type = IOMMU_DOMAIN_DMA;
> + return -ENODEV;
> + }
> + cookie->fq_domain = domain;
> + return 0;
> +}
> +
> /**
> * iommu_dma_init_domain - Initialise a DMA mapping domain
> * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
> @@ -362,16 +381,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
> }
>
> init_iova_domain(iovad, 1UL << order, base_pfn);
> -
> - if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) {
> - if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all,
> - iommu_dma_entry_dtor)) {
> - pr_warn("iova flush queue initialization failed\n");
> - domain->type = IOMMU_DOMAIN_DMA;
> - } else {
> - cookie->fq_domain = domain;
> - }
> - }
> + iommu_dma_init_fq(domain);
>
> return iova_reserve_iommu_regions(dev, domain);
> }
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 758ca4694257..81ab647f1618 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -20,6 +20,7 @@ void iommu_put_dma_cookie(struct iommu_domain *domain);
>
> /* Setup call for arch DMA mapping code */
> void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
> +int iommu_dma_init_fq(struct iommu_domain *domain);
>
> /* The DMA API isn't _quite_ the whole story, though... */
> /*
> @@ -37,9 +38,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc,
>
> void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list);
>
> -void iommu_dma_free_cpu_cached_iovas(unsigned int cpu,
> - struct iommu_domain *domain);
> -
> extern bool iommu_dma_forcedac;
>
> #else /* CONFIG_IOMMU_DMA */
> @@ -54,6 +52,11 @@ static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base,
> {
> }
>
> +static inline int iommu_dma_init_fq(struct iommu_domain *domain)
> +{
> + return -EINVAL;
> +}
> +
> static inline int iommu_get_dma_cookie(struct iommu_domain *domain)
> {
> return -ENODEV;
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 06:15:45

[permalink] [raw]

Subject: Re: [PATCH v2 24/24] iommu: Only log strictness for DMA domains

On 7/28/21 11:58 PM, Robin Murphy wrote:
> When passthrough is enabled, the default strictness policy becomes
> irrelevant, since any subsequent runtime override to a DMA domain type
> now embodies an explicit choice of strictness as well. Save on noise by
> only logging the default policy when it is meaningfully in effect.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommu.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index be399d630953..87d7b299436e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -144,10 +144,11 @@ static int __init iommu_subsys_init(void)
> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ?
> "(set via kernel command line)" : "");
>
> - pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
> - iommu_dma_strict ? "strict" : "lazy",
> - (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> - "(set via kernel command line)" : "");
> + if (!iommu_default_passthrough())
> + pr_info("DMA domain TLB invalidation policy: %s mode %s\n",
> + iommu_dma_strict ? "strict" : "lazy",
> + (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> + "(set via kernel command line)" : "");
>
> return 0;
> }
>

Reviewed-by: Lu Baolu <[email protected]>

Best regards,
baolu

2021-07-30 09:24:14

[permalink] [raw]

Subject: Re: [PATCH v2 12/24] iommu/dma: Unexport IOVA cookie management

On Wed, Jul 28, 2021 at 04:58:33PM +0100, Robin Murphy wrote:
> IOVA cookies are now got and put by core code, so we no longer need to
> export these to modular drivers. The export for getting MSI cookies
> stays, since VFIO can still be a module, but it was already relying on
> someone else putting them, so that aspect is unaffected.
>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: Jean-Philippe Brucker <[email protected]>

2021-07-30 09:24:54

[permalink] [raw]

Subject: Re: [PATCH v2 11/24] iommu/virtio: Drop IOVA cookie management

On Wed, Jul 28, 2021 at 04:58:32PM +0100, Robin Murphy wrote:
> The core code bakes its own cookies now.
>
> CC: Jean-Philippe Brucker <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: Jean-Philippe Brucker <[email protected]>

Also tested on the Seattle (vsmmuv3/viommu with lazy/strict, still need to
test the runtime switch).

> ---
> drivers/iommu/virtio-iommu.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 6abdcab7273b..80930ce04a16 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -598,12 +598,6 @@ static struct iommu_domain *viommu_domain_alloc(unsigned type)
> spin_lock_init(&vdomain->mappings_lock);
> vdomain->mappings = RB_ROOT_CACHED;
>
> - if (type == IOMMU_DOMAIN_DMA &&
> - iommu_get_dma_cookie(&vdomain->domain)) {
> - kfree(vdomain);
> - return NULL;
> - }
> -
> return &vdomain->domain;
> }
>
> @@ -643,8 +637,6 @@ static void viommu_domain_free(struct iommu_domain *domain)
> {
> struct viommu_domain *vdomain = to_viommu_domain(domain);
>
> - iommu_put_dma_cookie(domain);
> -
> /* Free all remaining mappings (size 2^64) */
> viommu_del_mappings(vdomain, 0, 0);
>
> --
> 2.25.1
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

2021-07-30 09:25:32

[permalink] [raw]

Subject: Re: [PATCH v2 21/24] iommu/dma: Factor out flush queue init

On 28/07/2021 16:58, Robin Murphy wrote:
> Factor out flush queue setup from the initial domain init so that we
> can potentially trigger it from sysfs later on in a domain's lifetime.
>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: John Garry <[email protected]>

2021-07-30 09:26:13

[permalink] [raw]

Subject: Re: [PATCH v2 14/24] iommu: Introduce explicit type for non-strict DMA domains

On Wed, Jul 28, 2021 at 04:58:35PM +0100, Robin Murphy wrote:
> Promote the difference between strict and non-strict DMA domains from an
> internal detail to a distinct domain feature and type, to pave the road
> for exposing it through the sysfs default domain interface.
>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: Jean-Philippe Brucker <[email protected]>

> ---
> drivers/iommu/dma-iommu.c | 2 +-
> drivers/iommu/iommu.c | 8 ++++++--
> include/linux/iommu.h | 11 +++++++++++
> 3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index e28396cea6eb..8b3545c01077 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1311,7 +1311,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
> * The IOMMU core code allocates the default DMA domain, which the
> * underlying IOMMU driver needs to support via the dma-iommu layer.
> */
> - if (domain->type == IOMMU_DOMAIN_DMA) {
> + if (iommu_is_dma_domain(domain)) {
> if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
> goto out_err;
> dev->dma_ops = &iommu_dma_ops;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index fa8109369f74..982545234cf3 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -115,6 +115,7 @@ static const char *iommu_domain_type_str(unsigned int t)
> case IOMMU_DOMAIN_UNMANAGED:
> return "Unmanaged";
> case IOMMU_DOMAIN_DMA:
> + case IOMMU_DOMAIN_DMA_FQ:
> return "Translated";
> default:
> return "Unknown";
> @@ -552,6 +553,9 @@ static ssize_t iommu_group_show_type(struct iommu_group *group,
> case IOMMU_DOMAIN_DMA:
> type = "DMA\n";
> break;
> + case IOMMU_DOMAIN_DMA_FQ:
> + type = "DMA-FQ\n";
> + break;
> }
> }
> mutex_unlock(&group->mutex);
> @@ -765,7 +769,7 @@ static int iommu_create_device_direct_mappings(struct iommu_group *group,
> unsigned long pg_size;
> int ret = 0;
>
> - if (!domain || domain->type != IOMMU_DOMAIN_DMA)
> + if (!domain || !iommu_is_dma_domain(domain))
> return 0;
>
> BUG_ON(!domain->pgsize_bitmap);
> @@ -1947,7 +1951,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> /* Assume all sizes by default; the driver may override this later */
> domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>
> - if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain)) {
> + if (iommu_is_dma_domain(domain) && iommu_get_dma_cookie(domain)) {
> iommu_domain_free(domain);
> domain = NULL;
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 141779d76035..046ba4d54cd2 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -61,6 +61,7 @@ struct iommu_domain_geometry {
> #define __IOMMU_DOMAIN_DMA_API (1U << 1) /* Domain for use in DMA-API
> implementation */
> #define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
> +#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
>
> /*
> * This are the possible domain-types
> @@ -73,12 +74,17 @@ struct iommu_domain_geometry {
> * IOMMU_DOMAIN_DMA - Internally used for DMA-API implementations.
> * This flag allows IOMMU drivers to implement
> * certain optimizations for these domains
> + * IOMMU_DOMAIN_DMA_FQ - As above, but definitely using batched TLB
> + * invalidation.
> */
> #define IOMMU_DOMAIN_BLOCKED (0U)
> #define IOMMU_DOMAIN_IDENTITY (__IOMMU_DOMAIN_PT)
> #define IOMMU_DOMAIN_UNMANAGED (__IOMMU_DOMAIN_PAGING)
> #define IOMMU_DOMAIN_DMA (__IOMMU_DOMAIN_PAGING | \
> __IOMMU_DOMAIN_DMA_API)
> +#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
> + __IOMMU_DOMAIN_DMA_API | \
> + __IOMMU_DOMAIN_DMA_FQ)
>
> struct iommu_domain {
> unsigned type;
> @@ -90,6 +96,11 @@ struct iommu_domain {
> struct iommu_dma_cookie *iova_cookie;
> };
>
> +static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
> +{
> + return domain->type & __IOMMU_DOMAIN_DMA_API;
> +}
> +
> enum iommu_cap {
> IOMMU_CAP_CACHE_COHERENCY, /* IOMMU can enforce cache coherent DMA
> transactions */
> --
> 2.25.1
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

2021-07-30 09:28:47

[permalink] [raw]

Subject: Re: [PATCH v2 22/24] iommu: Allow enabling non-strict mode dynamically

On 28/07/2021 16:58, Robin Murphy wrote:
> Allocating and enabling a flush queue is in fact something we can
> reasonably do while a DMA domain is active, without having to rebuild it
> from scratch. Thus we can allow a strict -> non-strict transition from
> sysfs without requiring to unbind the device's driver, which is of
> particular interest to users who want to make selective relaxations to
> critical devices like the one serving their root filesystem.
>
> Disabling and draining a queue also seems technically possible to
> achieve without rebuilding the whole domain, but would certainly be more
> involved. Furthermore there's not such a clear use-case for tightening
> up security*after* the device may already have done whatever it is that
> you don't trust it not to do, so we only consider the relaxation case.
>
> Signed-off-by: Robin Murphy<[email protected]>

Reviewed-by: John Garry <[email protected]>

2021-07-30 09:31:58

[permalink] [raw]

Subject: Re: [PATCH v2 20/24] iommu: Merge strictness and domain type configs

On Wed, Jul 28, 2021 at 04:58:41PM +0100, Robin Murphy wrote:
> To parallel the sysfs behaviour, merge the new build-time option
> for DMA domain strictness into the default domain type choice.
>
> Suggested-by: Joerg Roedel <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>

kernel-parameters.txt still refers to the old config options

Otherwise
Reviewed-by: Jean-Philippe Brucker <[email protected]>

> ---
> drivers/iommu/Kconfig | 80 +++++++++++++++++++++----------------------
> drivers/iommu/iommu.c | 2 +-
> 2 files changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index c84da8205be7..6e06f876d75a 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -79,55 +79,55 @@ config IOMMU_DEBUGFS
> debug/iommu directory, and then populate a subdirectory with
> entries as required.
>
> -config IOMMU_DEFAULT_PASSTHROUGH
> - bool "IOMMU passthrough by default"
> - depends on IOMMU_API
> - help
> - Enable passthrough by default, removing the need to pass in
> - iommu.passthrough=on or iommu=pt through command line. If this
> - is enabled, you can still disable with iommu.passthrough=off
> - or iommu=nopt depending on the architecture.
> -
> - If unsure, say N here.
> -
> choice
> - prompt "IOMMU default DMA IOTLB invalidation mode"
> - depends on IOMMU_DMA
> -
> - default IOMMU_DEFAULT_LAZY if (AMD_IOMMU || INTEL_IOMMU)
> - default IOMMU_DEFAULT_STRICT
> + prompt "IOMMU default domain type"
> + depends on IOMMU_API
> + default IOMMU_DEFAULT_DMA_LAZY if AMD_IOMMU || INTEL_IOMMU
> + default IOMMU_DEFAULT_DMA_STRICT
> help
> - This option allows an IOMMU DMA IOTLB invalidation mode to be
> - chosen at build time, to override the default mode of each ARCH,
> - removing the need to pass in kernel parameters through command line.
> - It is still possible to provide common boot params to override this
> - config.
> + Choose the type of IOMMU domain used to manage DMA API usage by
> + device drivers. The options here typically represent different
> + levels of tradeoff between robustness/security and performance,
> + depending on the IOMMU driver. Not all IOMMUs support all options.
> + This choice can be overridden at boot via the command line, and for
> + some devices also at runtime via sysfs.
>
> If unsure, keep the default.
>
> -config IOMMU_DEFAULT_STRICT
> - bool "strict"
> +config IOMMU_DEFAULT_DMA_STRICT
> + bool "Translated - Strict"
> help
> - For every IOMMU DMA unmap operation, the flush operation of IOTLB and
> - the free operation of IOVA are guaranteed to be done in the unmap
> - function.
> + Trusted devices use translation to restrict their access to only
> + DMA-mapped pages, with strict TLB invalidation on unmap. Equivalent
> + to passing "iommu.passthrough=0 iommu.strict=1" on the command line.
>
> -config IOMMU_DEFAULT_LAZY
> - bool "lazy"
> + Untrusted devices always use this mode, with an additional layer of
> + bounce-buffering such that they cannot gain access to any unrelated
> + data within a mapped page.
> +
> +config IOMMU_DEFAULT_DMA_LAZY
> + bool "Translated - Lazy"
> help
> - Support lazy mode, where for every IOMMU DMA unmap operation, the
> - flush operation of IOTLB and the free operation of IOVA are deferred.
> - They are only guaranteed to be done before the related IOVA will be
> - reused.
> + Trusted devices use translation to restrict their access to only
> + DMA-mapped pages, but with "lazy" batched TLB invalidation. This
> + mode allows higher performance with some IOMMUs due to reduced TLB
> + flushing, but at the cost of reduced isolation since devices may be
> + able to access memory for some time after it has been unmapped.
> + Equivalent to passing "iommu.passthrough=0 iommu.strict=0" on the
> + command line.
>
> - The isolation provided in this mode is not as secure as STRICT mode,
> - such that a vulnerable time window may be created between the DMA
> - unmap and the mappings cached in the IOMMU IOTLB or device TLB
> - finally being invalidated, where the device could still access the
> - memory which has already been unmapped by the device driver.
> - However this mode may provide better performance in high throughput
> - scenarios, and is still considerably more secure than passthrough
> - mode or no IOMMU.
> + If this mode is not supported by the IOMMU driver, the effective
> + runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.
> +
> +config IOMMU_DEFAULT_PASSTHROUGH
> + bool "Passthrough"
> + help
> + Trusted devices are identity-mapped, giving them unrestricted access
> + to memory with minimal performance overhead. Equivalent to passing
> + "iommu.passthrough=1" (historically "iommu=pt") on the command line.
> +
> + If this mode is not supported by the IOMMU driver, the effective
> + runtime default will fall back to IOMMU_DEFAULT_DMA_STRICT.
>
> endchoice
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 5a08e0806cbb..25c1adc1ec67 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -31,7 +31,7 @@ static struct kset *iommu_group_kset;
> static DEFINE_IDA(iommu_group_ida);
>
> static unsigned int iommu_def_domain_type __read_mostly;
> -static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_STRICT);
> +static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
> static u32 iommu_cmd_line __read_mostly;
>
> struct iommu_group {
> --
> 2.25.1
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

2021-07-30 09:32:44

[permalink] [raw]

Subject: Re: [PATCH v2 19/24] iommu: Expose DMA domain strictness via sysfs

On Wed, Jul 28, 2021 at 04:58:40PM +0100, Robin Murphy wrote:
> The sysfs interface for default domain types exists primarily so users
> can choose the performance/security tradeoff relevant to their own
> workload. As such, the choice between the policies for DMA domains fits
> perfectly as an additional point on that scale - downgrading a
> particular device from a strict default to non-strict may be enough to
> let it reach the desired level of performance, while still retaining
> more peace of mind than with a wide-open identity domain. Now that we've
> abstracted non-strict mode as a distinct type of DMA domain, allow it to
> be chosen through the user interface as well.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-kernel-iommu_groups | 2 ++
> drivers/iommu/iommu.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-kernel-iommu_groups b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> index eae2f1c1e11e..43ba764ba5b7 100644
> --- a/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> +++ b/Documentation/ABI/testing/sysfs-kernel-iommu_groups
> @@ -42,6 +42,8 @@ Description: /sys/kernel/iommu_groups/<grp_id>/type shows the type of default
> ======== ======================================================
> DMA All the DMA transactions from the device in this group
> are translated by the iommu.
> + DMA-FQ As above, but using batched invalidation to lazily
> + remove translations after use.

It might be useful to desribe the security/performance tradeoff here as
well. I guess a normal user is more likely to look at the doc for this
sysfs knob than the kernel config or parameters.

Thanks,
Jean

> identity All the DMA transactions from the device in this group
> are not translated by the iommu.
> auto Change to the type the device was booted with.
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index eecb5657de69..5a08e0806cbb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3265,6 +3265,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> req_type = IOMMU_DOMAIN_IDENTITY;
> else if (sysfs_streq(buf, "DMA"))
> req_type = IOMMU_DOMAIN_DMA;
> + else if (sysfs_streq(buf, "DMA-FQ"))
> + req_type = IOMMU_DOMAIN_DMA_FQ;
> else if (sysfs_streq(buf, "auto"))
> req_type = 0;
> else
> --
> 2.25.1
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

2021-07-30 09:34:44

[permalink] [raw]

Subject: Re: [PATCH v2 01/24] iommu: Pull IOVA cookie management into the core

On Wed, Jul 28, 2021 at 04:58:22PM +0100, Robin Murphy wrote:
> Now that everyone has converged on iommu-dma for IOMMU_DOMAIN_DMA
> support, we can abandon the notion of drivers being responsible for the
> cookie type, and consolidate all the management into the core code.
>
> CC: Marek Szyprowski <[email protected]>
> CC: Yoshihiro Shimoda <[email protected]>
> CC: Geert Uytterhoeven <[email protected]>
> CC: Yong Wu <[email protected]>
> CC: Heiko Stuebner <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Chunyan Zhang <[email protected]>
> CC: Maxime Ripard <[email protected]>
> CC: Jean-Philippe Brucker <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: Jean-Philippe Brucker <[email protected]>

> ---
> drivers/iommu/iommu.c | 7 +++++++
> include/linux/iommu.h | 3 ++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2cda9950bd5..ea5a9ea8d431 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -7,6 +7,7 @@
> #define pr_fmt(fmt) "iommu: " fmt
>
> #include <linux/device.h>
> +#include <linux/dma-iommu.h>
> #include <linux/kernel.h>
> #include <linux/bits.h>
> #include <linux/bug.h>
> @@ -1946,6 +1947,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> /* Assume all sizes by default; the driver may override this later */
> domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>
> + /* Temporarily ignore -EEXIST while drivers still get their own cookies */
> + if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(domain) == -ENOMEM) {
> + iommu_domain_free(domain);
> + domain = NULL;
> + }
> return domain;
> }
>
> @@ -1957,6 +1963,7 @@ EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>
> void iommu_domain_free(struct iommu_domain *domain)
> {
> + iommu_put_dma_cookie(domain);
> domain->ops->domain_free(domain);
> }
> EXPORT_SYMBOL_GPL(iommu_domain_free);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 4997c78e2670..141779d76035 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -40,6 +40,7 @@ struct iommu_domain;
> struct notifier_block;
> struct iommu_sva;
> struct iommu_fault_event;
> +struct iommu_dma_cookie;
>
> /* iommu fault flags */
> #define IOMMU_FAULT_READ 0x0
> @@ -86,7 +87,7 @@ struct iommu_domain {
> iommu_fault_handler_t handler;
> void *handler_token;
> struct iommu_domain_geometry geometry;
> - void *iova_cookie;
> + struct iommu_dma_cookie *iova_cookie;
> };
>
> enum iommu_cap {
> --
> 2.25.1
>
> _______________________________________________
> iommu mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

2021-07-30 09:36:15

[permalink] [raw]

Subject: Re: [PATCH v2 20/24] iommu: Merge strictness and domain type configs

On 28/07/2021 16:58, Robin Murphy wrote:
>
> -config IOMMU_DEFAULT_LAZY
> - bool "lazy"
> + Untrusted devices always use this mode, with an additional layer of
> + bounce-buffering such that they cannot gain access to any unrelated
> + data within a mapped page.
> +
> +config IOMMU_DEFAULT_DMA_LAZY
> + bool "Translated - Lazy"
> help

Since these are being renamed, can you update the kernel-parameters.txt:

iommu.strict= [ARM64, X86] Configure TLB invalidation behaviour

....
DMA unmap operations invalidate IOMMU hardware TLBs
synchronously.
unset - Use value of CONFIG_IOMMU_DEFAULT_{LAZY,STRICT}.

^^^^^

Thanks

2021-07-30 10:22:12

[permalink] [raw]

Subject: Re: [PATCH v2 19/24] iommu: Expose DMA domain strictness via sysfs

On 28/07/2021 16:58, Robin Murphy wrote:
> The sysfs interface for default domain types exists primarily so users
> can choose the performance/security tradeoff relevant to their own
> workload. As such, the choice between the policies for DMA domains fits
> perfectly as an additional point on that scale - downgrading a
> particular device from a strict default to non-strict may be enough to
> let it reach the desired level of performance, while still retaining
> more peace of mind than with a wide-open identity domain. Now that we've
> abstracted non-strict mode as a distinct type of DMA domain, allow it to
> be chosen through the user interface as well.
>
> Signed-off-by: Robin Murphy<[email protected]>
> ---

Reviewed-by: John Garry <[email protected]>

2021-08-02 13:06:25

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v2 23/24] iommu/arm-smmu: Allow non-strict in pgtable_quirks interface

On Wed, Jul 28, 2021 at 04:58:44PM +0100, Robin Murphy wrote:
> To make io-pgtable aware of a flush queue being dynamically enabled,
> allow IO_PGTABLE_QUIRK_NON_STRICT to be set even after a domain has been
> attached to, and hook up the final piece of the puzzle in iommu-dma.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 +++++++++++
> drivers/iommu/dma-iommu.c | 3 +++
> 3 files changed, 29 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 19400826eba7..40fa9cb382c3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2711,6 +2711,20 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
> return ret;
> }
>
> +static int arm_smmu_set_pgtable_quirks(struct iommu_domain *domain,
> + unsigned long quirks)
> +{
> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +
> + if (quirks == IO_PGTABLE_QUIRK_NON_STRICT && smmu_domain->pgtbl_ops) {
> + struct io_pgtable *iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> +
> + iop->cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> + return 0;
> + }
> + return -EINVAL;
> +}

I don't see anything serialising this against a concurrent iommu_unmap(), so
the ordering and atomicity looks quite suspicious to me here. I don't think
it's just the page-table quirks either, but also setting cookie->fq_domain.

Will

2021-08-02 14:18:23

[permalink] [raw]

Subject: Re: [PATCH v2 23/24] iommu/arm-smmu: Allow non-strict in pgtable_quirks interface

On 2021-08-02 14:04, Will Deacon wrote:
> On Wed, Jul 28, 2021 at 04:58:44PM +0100, Robin Murphy wrote:
>> To make io-pgtable aware of a flush queue being dynamically enabled,
>> allow IO_PGTABLE_QUIRK_NON_STRICT to be set even after a domain has been
>> attached to, and hook up the final piece of the puzzle in iommu-dma.
>>
>> Signed-off-by: Robin Murphy <[email protected]>
>> ---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 +++++++++++
>> drivers/iommu/dma-iommu.c | 3 +++
>> 3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 19400826eba7..40fa9cb382c3 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2711,6 +2711,20 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
>> return ret;
>> }
>>
>> +static int arm_smmu_set_pgtable_quirks(struct iommu_domain *domain,
>> + unsigned long quirks)
>> +{
>> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>> +
>> + if (quirks == IO_PGTABLE_QUIRK_NON_STRICT && smmu_domain->pgtbl_ops) {
>> + struct io_pgtable *iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
>> +
>> + iop->cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
>> + return 0;
>> + }
>> + return -EINVAL;
>> +}
>
> I don't see anything serialising this against a concurrent iommu_unmap(), so
> the ordering and atomicity looks quite suspicious to me here. I don't think
> it's just the page-table quirks either, but also setting cookie->fq_domain.

Heh, I confess to very much taking the cheeky "let's say nothing and see
what Will thinks about concurrency" approach here :)

The beauty of only allowing relaxation in the strict->non-strict
direction is that it shouldn't need serialising as such - it doesn't
matter if the update to cookie->fq_domain is observed between
iommu_unmap() and iommu_dma_free_iova(), since there's no correctness
impact to queueing IOVAs which may already have been invalidated and may
or may not have been synced. AFAICS the only condition which matters is
that the setting of the io-pgtable quirk must observe fq_domain being
set. It feels like there must be enough dependencies on the read side,
but we might need an smp_wmb() between the two in iommu_dma_init_fq()?

I've also flip-flopped a bit on whether fq_domain needs to be accessed
with READ_ONCE/WRITE_ONCE - by the time of posting I'd convinced myself
that it was probably OK, but looking again now I suppose this wacky
reordering is theoretically possible:

iommu_dma_unmap() {
bool free_fq = cookie->fq_domain; // == false

iommu_unmap();

if (!cookie->fq_domain) // observes new non-NULL value
iommu_tlb_sync(); // skipped

iommu_dma_free_iova { // inlined
if (free_fq) // false
queue_iova();
else
free_iova_fast(); // Uh-oh!
}
}

so although I still can't see atomicity being a problem I guess we do
need it for the sake of reordering at least.

Cheers,
Robin.

2021-08-03 10:37:35

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH v2 23/24] iommu/arm-smmu: Allow non-strict in pgtable_quirks interface

On Mon, Aug 02, 2021 at 03:15:50PM +0100, Robin Murphy wrote:
> On 2021-08-02 14:04, Will Deacon wrote:
> > On Wed, Jul 28, 2021 at 04:58:44PM +0100, Robin Murphy wrote:
> > > To make io-pgtable aware of a flush queue being dynamically enabled,
> > > allow IO_PGTABLE_QUIRK_NON_STRICT to be set even after a domain has been
> > > attached to, and hook up the final piece of the puzzle in iommu-dma.
> > >
> > > Signed-off-by: Robin Murphy <[email protected]>
> > > ---
> > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 +++++++++++
> > > drivers/iommu/dma-iommu.c | 3 +++
> > > 3 files changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > index 19400826eba7..40fa9cb382c3 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -2711,6 +2711,20 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
> > > return ret;
> > > }
> > > +static int arm_smmu_set_pgtable_quirks(struct iommu_domain *domain,
> > > + unsigned long quirks)
> > > +{
> > > + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > > +
> > > + if (quirks == IO_PGTABLE_QUIRK_NON_STRICT && smmu_domain->pgtbl_ops) {
> > > + struct io_pgtable *iop = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
> > > +
> > > + iop->cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT;
> > > + return 0;
> > > + }
> > > + return -EINVAL;
> > > +}
> >
> > I don't see anything serialising this against a concurrent iommu_unmap(), so
> > the ordering and atomicity looks quite suspicious to me here. I don't think
> > it's just the page-table quirks either, but also setting cookie->fq_domain.
>
> Heh, I confess to very much taking the cheeky "let's say nothing and see
> what Will thinks about concurrency" approach here :)

Damnit, I fell for that didn't I?

Overall, I'm really nervous about the concurrency here and think we'd be
better off requiring the unbind as we have for the other domain changes.

> The beauty of only allowing relaxation in the strict->non-strict direction
> is that it shouldn't need serialising as such - it doesn't matter if the
> update to cookie->fq_domain is observed between iommu_unmap() and
> iommu_dma_free_iova(), since there's no correctness impact to queueing IOVAs
> which may already have been invalidated and may or may not have been synced.
> AFAICS the only condition which matters is that the setting of the
> io-pgtable quirk must observe fq_domain being set. It feels like there must
> be enough dependencies on the read side, but we might need an smp_wmb()
> between the two in iommu_dma_init_fq()?
>
> I've also flip-flopped a bit on whether fq_domain needs to be accessed with
> READ_ONCE/WRITE_ONCE - by the time of posting I'd convinced myself that it
> was probably OK, but looking again now I suppose this wacky reordering is
> theoretically possible:
>
>
> iommu_dma_unmap() {
> bool free_fq = cookie->fq_domain; // == false
>
> iommu_unmap();
>
> if (!cookie->fq_domain) // observes new non-NULL value
> iommu_tlb_sync(); // skipped
>
> iommu_dma_free_iova { // inlined
> if (free_fq) // false
> queue_iova();
> else
> free_iova_fast(); // Uh-oh!
> }
> }
>
> so although I still can't see atomicity being a problem I guess we do need
> it for the sake of reordering at least.

With your changes, I think quite a few things can go wrong.

* cookie->fq_domain may be observed but iovad->fq could be NULL
* We can miss the smp_wmb() in the pgtable code but end up deferring the
IOVA reclaim
* iommu_change_dev_def_domain() only holds the group mutex afaict, so can
possibly run concurrently with itself on the same domain?
* iommu_dma_init_fq() can flip the domain type back from
IOMMU_DOMAIN_DMA_FQ to IOMMU_DOMAIN_DMA on the error path
* set_pgtable_quirks() isn't atomic, which probably is ok for now, but the
moment we use it anywhere else it's dangerous

and I suspect there are more :/

Will

2021-08-03 12:14:59