[PATCH 0/3] iommu/amd: I/O VA address limits

[PATCH 0/3] iommu/amd: I/O VA address limits

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for I/O virtual
addresses that can be handled by the IOMMUs in the system. Parse that
data from the IVRS header to provide aperture information for DMA
mappings and users of the iommu API.

Sebastian Ott (3):
  iommu/amd: Parse supported address sizes from IVRS
  iommu/amd: Restrict aperture for domains to conform with IVRS
  iommu/amd: Actually enforce geometry aperture

 drivers/iommu/amd_iommu.c       | 16 +++++++++++-----
 drivers/iommu/amd_iommu_init.c  | 26 ++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_types.h |  3 +++
 3 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/3] iommu/amd: Parse supported address sizes from IVRS

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for I/O virtual
addresses that can be handled by the IOMMUs in the system. Parse that
data from the IVRS header so that it can be considered in limiting the
I/O aperture.

Based on prior work by Marius Hillenbrand.

Link: https://www.amd.com/system/files/TechDocs/48882_IOMMU_3.05_PUB.pdf

Signed-off-by: Sebastian Ott <sebott@amazon.de>
---
 drivers/iommu/amd_iommu.c       |  1 +
 drivers/iommu/amd_iommu_init.c  | 26 ++++++++++++++++++++++++++
 drivers/iommu/amd_iommu_types.h |  3 +++
 3 files changed, 30 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2883ac389abb..fb4a44550c4a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -88,6 +88,7 @@ const struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
 int amd_iommu_max_glx_val = -1;
+u8 amd_iommu_ivrs_va_size;
 
 /*
  * general struct to manage commands send to an IOMMU
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 5b81fd16f5fa..3b07218e7000 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -11,6 +11,7 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/list.h>
+#include <linux/bitfield.h>
 #include <linux/bitmap.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
@@ -41,6 +42,7 @@
  * definitions for the ACPI scanning code
  */
 #define IVRS_HEADER_LENGTH 48
+#define IVRS_HEADER_IVINFO_OFFSET 36
 
 #define ACPI_IVHD_TYPE_MAX_SUPPORTED	0x40
 #define ACPI_IVMD_TYPE_ALL              0x20
@@ -2492,6 +2494,27 @@ static void __init free_dma_resources(void)
 	free_unity_maps();
 }
 
+static void __init get_ivrs_ivinfo(struct acpi_table_header *ivrs)
+{
+	u32 *ivinfo = (u32 *)((u8 *)ivrs + IVRS_HEADER_IVINFO_OFFSET);
+	u8 va_size = FIELD_GET(GENMASK(21, 15), *ivinfo);
+	u8 valid_va_sizes[] = {32, 40, 48, 64};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(valid_va_sizes); i++) {
+		if (va_size == valid_va_sizes[i]) {
+			amd_iommu_ivrs_va_size = va_size;
+			break;
+		}
+	}
+
+	if (!amd_iommu_ivrs_va_size) {
+		pr_warn("Invalid virtual address size %u in IVRS header, use most restrictive %u\n",
+			va_size, valid_va_sizes[0]);
+		amd_iommu_ivrs_va_size = valid_va_sizes[0];
+	}
+}
+
 /*
  * This is the hardware init function for AMD IOMMU in the system.
  * This function is called either from amd_iommu_init or from the interrupt
@@ -2546,6 +2569,9 @@ static int __init early_amd_iommu_init(void)
 	if (ret)
 		goto out;
 
+	get_ivrs_ivinfo(ivrs_base);
+	DUMP_printk("IVRS vasize=%d\n", amd_iommu_ivrs_va_size);
+
 	amd_iommu_target_ivhd_type = get_highest_supported_ivhd_type(ivrs_base);
 	DUMP_printk("Using IVHD type %#x\n", amd_iommu_target_ivhd_type);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 7a8fdec138bd..8141b2874170 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -755,6 +755,9 @@ extern bool amd_iommu_force_isolation;
 /* Max levels of glxval supported */
 extern int amd_iommu_max_glx_val;
 
+/* Maximum virtual address size supported */
+extern u8 amd_iommu_ivrs_va_size;
+
 /*
  * This function flushes all internal caches of
  * the IOMMU used by this driver.
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/3] iommu/amd: Restrict aperture for domains to conform with IVRS

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for I/O virtual
addresses. When allocating new protection domains that perform
translation, propagate these limits as the domain's geometry / aperture.

Based on prior work by Marius Hillenbrand.

Signed-off-by: Sebastian Ott <sebott@amazon.de>
---
 drivers/iommu/amd_iommu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index fb4a44550c4a..d2e79e27778e 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2460,6 +2460,7 @@ static struct protection_domain *protection_domain_alloc(void)
 
 static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 {
+	dma_addr_t max_va = DMA_BIT_MASK(amd_iommu_ivrs_va_size);
 	struct protection_domain *pdomain;
 	u64 *pt_root, root;
 
@@ -2477,11 +2478,6 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 
 		root = amd_iommu_domain_encode_pgtable(pt_root, PAGE_MODE_3_LEVEL);
 		atomic64_set(&pdomain->pt_root, root);
-
-		pdomain->domain.geometry.aperture_start = 0;
-		pdomain->domain.geometry.aperture_end   = ~0ULL;
-		pdomain->domain.geometry.force_aperture = true;
-
 		break;
 	case IOMMU_DOMAIN_DMA:
 		pdomain = dma_ops_domain_alloc();
@@ -2501,6 +2497,10 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 		return NULL;
 	}
 
+	pdomain->domain.geometry.aperture_start = 0;
+	pdomain->domain.geometry.aperture_end   = max_va;
+	pdomain->domain.geometry.force_aperture = true;
+
 	return &pdomain->domain;
 }
 
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/3] iommu/amd: Actually enforce geometry aperture

[Sebastian Ott via iommu]

Add a check to enforce that I/O virtual addresses picked by iommu API
users stay within the domains geometry aperture.

Signed-off-by: Sebastian Ott <sebott@amazon.de>
---
 drivers/iommu/amd_iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index d2e79e27778e..6485e2081706 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2618,6 +2618,11 @@ static int amd_iommu_map(struct iommu_domain *dom, unsigned long iova,
 	if (pgtable.mode == PAGE_MODE_NONE)
 		return -EINVAL;
 
+	if (dom->geometry.force_aperture &&
+	    (iova < dom->geometry.aperture_start ||
+	     iova + page_size - 1 > dom->geometry.aperture_end))
+		return -EINVAL;
+
 	if (iommu_prot & IOMMU_READ)
 		prot |= IOMMU_PROT_IR;
 	if (iommu_prot & IOMMU_WRITE)
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/3] iommu/amd: I/O VA address limits

[Sebastian Ott via iommu]

Hello Joerg,

On 2020-06-05 16:56, Sebastian Ott wrote:
> The IVRS ACPI table specifies maximum address sizes for I/O virtual
> addresses that can be handled by the IOMMUs in the system. Parse that
> data from the IVRS header to provide aperture information for DMA
> mappings and users of the iommu API.
>
> Sebastian Ott (3):
>    iommu/amd: Parse supported address sizes from IVRS
>    iommu/amd: Restrict aperture for domains to conform with IVRS
>    iommu/amd: Actually enforce geometry aperture
>
>   drivers/iommu/amd_iommu.c       | 16 +++++++++++-----
>   drivers/iommu/amd_iommu_init.c  | 26 ++++++++++++++++++++++++++
>   drivers/iommu/amd_iommu_types.h |  3 +++
>   3 files changed, 40 insertions(+), 5 deletions(-)
>
Would you consider taking these patches for the next release?

Regards,
Sebastian




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/3] iommu/amd: Actually enforce geometry aperture

[Joerg Roedel]

Hi Sebastian,

On Fri, Jun 05, 2020 at 04:56:55PM +0200, Sebastian Ott wrote:
> Add a check to enforce that I/O virtual addresses picked by iommu API
> users stay within the domains geometry aperture.
> 
> Signed-off-by: Sebastian Ott <sebott@amazon.de>
> ---
>  drivers/iommu/amd_iommu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index d2e79e27778e..6485e2081706 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -2618,6 +2618,11 @@ static int amd_iommu_map(struct iommu_domain *dom, unsigned long iova,
>  	if (pgtable.mode == PAGE_MODE_NONE)
>  		return -EINVAL;
>  
> +	if (dom->geometry.force_aperture &&
> +	    (iova < dom->geometry.aperture_start ||
> +	     iova + page_size - 1 > dom->geometry.aperture_end))
> +		return -EINVAL;
> +
>  	if (iommu_prot & IOMMU_READ)
>  		prot |= IOMMU_PROT_IR;
>  	if (iommu_prot & IOMMU_WRITE)

This patch need also make iommu_setup_dma_ops() aware of the aperture
limits.

Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 0/3] iommu/amd: I/O VA address limits

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for I/O virtual
addresses that can be handled by the IOMMUs in the system. Parse that
data from the IVRS header to provide aperture information for DMA
mappings and users of the iommu API.

Changes for V2:
 - use limits in iommu_setup_dma_ops()
 - rebased to current upstream

Sebastian Ott (3):
  iommu/amd: Parse supported address sizes from IVRS
  iommu/amd: Restrict aperture for domains to conform with IVRS
  iommu/amd: Actually enforce geometry aperture

 drivers/iommu/amd/amd_iommu_types.h |  3 +++
 drivers/iommu/amd/init.c            | 26 ++++++++++++++++++++++++++
 drivers/iommu/amd/iommu.c           | 12 ++++++++++--
 3 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 1/3] iommu/amd: Parse supported address sizes from IVRS

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for i/o virtual
addresses that can be handled by the IOMMUs in the system.  Parse that
data from the IVRS header so that it can be considered in limiting the
IO aperture (in subsequent patches).

Based on prior work by Marius Hillenbrand.

Link: https://www.amd.com/system/files/TechDocs/48882_IOMMU_3.05_PUB.pdf

Signed-off-by: Sebastian Ott <sebott@amazon.de>
Cc: Benjamin Serebrin <serebrin@amazon.com>
Cc: Filippo Sironi <sironi@amazon.de>

CR: https://code.amazon.com/reviews/CR-26408321
---
 drivers/iommu/amd/amd_iommu_types.h |  3 +++
 drivers/iommu/amd/init.c            | 26 ++++++++++++++++++++++++++
 drivers/iommu/amd/iommu.c           |  1 +
 3 files changed, 30 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 30a5d412255a..0946638306d6 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -754,6 +754,9 @@ extern bool amd_iommu_force_isolation;
 /* Max levels of glxval supported */
 extern int amd_iommu_max_glx_val;
 
+/* Maximum virtual address supported */
+extern u64 amd_iommu_max_va;
+
 /*
  * This function flushes all internal caches of
  * the IOMMU used by this driver.
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 6ebd4825e320..ab9d226b4215 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -11,6 +11,7 @@
 #include <linux/pci.h>
 #include <linux/acpi.h>
 #include <linux/list.h>
+#include <linux/bitfield.h>
 #include <linux/bitmap.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
@@ -39,6 +40,7 @@
  * definitions for the ACPI scanning code
  */
 #define IVRS_HEADER_LENGTH 48
+#define IVRS_HEADER_IVINFO_OFFSET 36
 
 #define ACPI_IVHD_TYPE_MAX_SUPPORTED	0x40
 #define ACPI_IVMD_TYPE_ALL              0x20
@@ -2490,6 +2492,27 @@ static void __init free_dma_resources(void)
 	free_unity_maps();
 }
 
+static void __init get_ivrs_ivinfo(struct acpi_table_header *ivrs)
+{
+	u32 *ivinfo = (u32 *)((u8 *)ivrs + IVRS_HEADER_IVINFO_OFFSET);
+	u8 va_size = FIELD_GET(GENMASK(21, 15), *ivinfo);
+	u8 valid_va_sizes[] = {32, 40, 48, 64};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(valid_va_sizes); i++) {
+		if (va_size == valid_va_sizes[i]) {
+			amd_iommu_max_va = DMA_BIT_MASK(va_size);
+			break;
+		}
+	}
+
+	if (!amd_iommu_max_va) {
+		pr_warn("Invalid virtual address size %u in IVRS header, use most restrictive %u\n",
+			va_size, valid_va_sizes[0]);
+		amd_iommu_max_va = DMA_BIT_MASK(valid_va_sizes[0]);
+	}
+}
+
 /*
  * This is the hardware init function for AMD IOMMU in the system.
  * This function is called either from amd_iommu_init or from the interrupt
@@ -2544,6 +2567,9 @@ static int __init early_amd_iommu_init(void)
 	if (ret)
 		goto out;
 
+	get_ivrs_ivinfo(ivrs_base);
+	DUMP_printk("IVRS vasize=%llx\n", amd_iommu_max_va);
+
 	amd_iommu_target_ivhd_type = get_highest_supported_ivhd_type(ivrs_base);
 	DUMP_printk("Using IVHD type %#x\n", amd_iommu_target_ivhd_type);
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 74cca1757172..acab35220d98 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -88,6 +88,7 @@ const struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
 int amd_iommu_max_glx_val = -1;
+u64 amd_iommu_max_va;
 
 /*
  * general struct to manage commands send to an IOMMU
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/3] iommu/amd: Restrict aperture for domains to conform with IVRS

[Sebastian Ott via iommu]

The IVRS ACPI table specifies maximum address sizes for I/O virtual
addresses. When allocating new protection domains that perform
translation, propagate these limits as the domain's geometry / aperture.

Based on prior work by Marius Hillenbrand.

Signed-off-by: Sebastian Ott <sebott@amazon.de>
Cc: Benjamin Serebrin <serebrin@amazon.com>
Cc: Filippo Sironi <sironi@amazon.de>

CR: https://code.amazon.com/reviews/CR-26408353
---
 drivers/iommu/amd/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index acab35220d98..b3f79820fd6d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2382,7 +2382,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 		return NULL;
 
 	domain->domain.geometry.aperture_start = 0;
-	domain->domain.geometry.aperture_end   = ~0ULL;
+	domain->domain.geometry.aperture_end   = amd_iommu_max_va;
 	domain->domain.geometry.force_aperture = true;
 
 	if (type == IOMMU_DOMAIN_DMA &&
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/3] iommu/amd: Actually enforce geometry aperture

[Sebastian Ott via iommu]

Add a check to enforce that I/O virtual addresses picked by iommu API
users stay within the domains geometry aperture.

Signed-off-by: Sebastian Ott <sebott@amazon.de>
Cc: Benjamin Serebrin <serebrin@amazon.com>
Cc: Filippo Sironi <sironi@amazon.de>

CR: https://code.amazon.com/reviews/CR-26408388
---
 drivers/iommu/amd/iommu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b3f79820fd6d..bfa9c4a1fcf8 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2159,11 +2159,13 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 static void amd_iommu_probe_finalize(struct device *dev)
 {
 	struct iommu_domain *domain;
+	u64 base = IOVA_START_PFN << PAGE_SHIFT;
+	u64 size = amd_iommu_max_va - base;
 
 	/* 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, IOVA_START_PFN << PAGE_SHIFT, 0);
+		iommu_setup_dma_ops(dev, base, size);
 }
 
 static void amd_iommu_release_device(struct device *dev)
@@ -2500,6 +2502,11 @@ static int amd_iommu_map(struct iommu_domain *dom, unsigned long iova,
 	if (pgtable.mode == PAGE_MODE_NONE)
 		return -EINVAL;
 
+	if (dom->geometry.force_aperture &&
+	    (iova < dom->geometry.aperture_start ||
+	     iova + page_size - 1 > dom->geometry.aperture_end))
+		return -EINVAL;
+
 	if (iommu_prot & IOMMU_READ)
 		prot |= IOMMU_PROT_IR;
 	if (iommu_prot & IOMMU_WRITE)
-- 
2.17.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Joerg Roedel]

Hi Sebastian,

On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
> The IVRS ACPI table specifies maximum address sizes for I/O virtual
> addresses that can be handled by the IOMMUs in the system. Parse that
> data from the IVRS header to provide aperture information for DMA
> mappings and users of the iommu API.
> 
> Changes for V2:
>  - use limits in iommu_setup_dma_ops()
>  - rebased to current upstream
> 
> Sebastian Ott (3):
>   iommu/amd: Parse supported address sizes from IVRS
>   iommu/amd: Restrict aperture for domains to conform with IVRS
>   iommu/amd: Actually enforce geometry aperture

Thanks for the changes. May I ask what the reason for those changes are?
AFAIK all AMD IOMMU implementations (in hardware) support full 64bit
address spaces, and the IVRS table might actually be wrong, limiting the
address space in the worst case to only 32 bit.

Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Sebastian Ott via iommu]

Hello Joerg,

On 2020-07-10 14:31, Joerg Roedel wrote:
> On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
>> The IVRS ACPI table specifies maximum address sizes for I/O virtual
>> addresses that can be handled by the IOMMUs in the system. Parse that
>> data from the IVRS header to provide aperture information for DMA
>> mappings and users of the iommu API.
>>
>> Changes for V2:
>>   - use limits in iommu_setup_dma_ops()
>>   - rebased to current upstream
>>
>> Sebastian Ott (3):
>>    iommu/amd: Parse supported address sizes from IVRS
>>    iommu/amd: Restrict aperture for domains to conform with IVRS
>>    iommu/amd: Actually enforce geometry aperture
> Thanks for the changes. May I ask what the reason for those changes are?
> AFAIK all AMD IOMMU implementations (in hardware) support full 64bit
> address spaces, and the IVRS table might actually be wrong, limiting the
> address space in the worst case to only 32 bit.

It's not the IOMMU, but we've encountered devices that are capable of 
more than
32- but less than 64- bit IOVA, and there's no way to express that to 
the IOVA
allocator in the PCIe spec. Our solution was to have our platforms 
express an
IVRS entry that says the IOMMU is capable of 48-bit, which these devices 
can generate.
48 bits is plenty of address space in this generation for the 
application we have.

Sebastian




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Robin Murphy]

On 2020-07-17 10:20, Sebastian Ott via iommu wrote:
> Hello Joerg,
> 
> On 2020-07-10 14:31, Joerg Roedel wrote:
>> On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
>>> The IVRS ACPI table specifies maximum address sizes for I/O virtual
>>> addresses that can be handled by the IOMMUs in the system. Parse that
>>> data from the IVRS header to provide aperture information for DMA
>>> mappings and users of the iommu API.
>>>
>>> Changes for V2:
>>>   - use limits in iommu_setup_dma_ops()
>>>   - rebased to current upstream
>>>
>>> Sebastian Ott (3):
>>>    iommu/amd: Parse supported address sizes from IVRS
>>>    iommu/amd: Restrict aperture for domains to conform with IVRS
>>>    iommu/amd: Actually enforce geometry aperture
>> Thanks for the changes. May I ask what the reason for those changes are?
>> AFAIK all AMD IOMMU implementations (in hardware) support full 64bit
>> address spaces, and the IVRS table might actually be wrong, limiting the
>> address space in the worst case to only 32 bit.
> 
> It's not the IOMMU, but we've encountered devices that are capable of 
> more than
> 32- but less than 64- bit IOVA, and there's no way to express that to 
> the IOVA
> allocator in the PCIe spec. Our solution was to have our platforms 
> express an
> IVRS entry that says the IOMMU is capable of 48-bit, which these devices 
> can generate.
> 48 bits is plenty of address space in this generation for the 
> application we have.

Hmm, for constraints of individual devices, it should really be their 
drivers' job to call dma_set_mask*() with the appropriate value in the 
first place rather than just assuming that 64 means anything >32. If 
it's a case where the device itself technically is 64-bit capable, but 
an upstream bridge is constraining it, then that limit can also be 
described either by dedicated firmware properties (e.g. ACPI _DMA) or 
with a quirk like via_no_dac().

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Sironi, Filippo via iommu]

On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote:
> 
> On 2020-07-17 10:20, Sebastian Ott via iommu wrote:
> > Hello Joerg,
> > 
> > On 2020-07-10 14:31, Joerg Roedel wrote:
> > > On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
> > > > The IVRS ACPI table specifies maximum address sizes for I/O
> > > > virtual
> > > > addresses that can be handled by the IOMMUs in the system. Parse
> > > > that
> > > > data from the IVRS header to provide aperture information for
> > > > DMA
> > > > mappings and users of the iommu API.
> > > > 
> > > > Changes for V2:
> > > >   - use limits in iommu_setup_dma_ops()
> > > >   - rebased to current upstream
> > > > 
> > > > Sebastian Ott (3):
> > > >    iommu/amd: Parse supported address sizes from IVRS
> > > >    iommu/amd: Restrict aperture for domains to conform with IVRS
> > > >    iommu/amd: Actually enforce geometry aperture
> > > 
> > > Thanks for the changes. May I ask what the reason for those
> > > changes are?
> > > AFAIK all AMD IOMMU implementations (in hardware) support full
> > > 64bit
> > > address spaces, and the IVRS table might actually be wrong,
> > > limiting the
> > > address space in the worst case to only 32 bit.
> > 
> > It's not the IOMMU, but we've encountered devices that are capable
> > of
> > more than
> > 32- but less than 64- bit IOVA, and there's no way to express that
> > to
> > the IOVA
> > allocator in the PCIe spec. Our solution was to have our platforms
> > express an
> > IVRS entry that says the IOMMU is capable of 48-bit, which these
> > devices
> > can generate.
> > 48 bits is plenty of address space in this generation for the
> > application we have.
> 
> Hmm, for constraints of individual devices, it should really be their
> drivers' job to call dma_set_mask*() with the appropriate value in the
> first place rather than just assuming that 64 means anything >32. If
> it's a case where the device itself technically is 64-bit capable, but
> an upstream bridge is constraining it, then that limit can also be
> described either by dedicated firmware properties (e.g. ACPI _DMA) or
> with a quirk like via_no_dac().
> 
> Robin.

You cannot rely on the device driver only because the device driver
attach might be a generic one like vfio-pci, for instance, that doesn't
have any device specific knowledge.

Filippo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Robin Murphy]

On 2020-07-17 14:22, Sironi, Filippo wrote:
> On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote:
>>
>> On 2020-07-17 10:20, Sebastian Ott via iommu wrote:
>>> Hello Joerg,
>>>
>>> On 2020-07-10 14:31, Joerg Roedel wrote:
>>>> On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
>>>>> The IVRS ACPI table specifies maximum address sizes for I/O
>>>>> virtual
>>>>> addresses that can be handled by the IOMMUs in the system. Parse
>>>>> that
>>>>> data from the IVRS header to provide aperture information for
>>>>> DMA
>>>>> mappings and users of the iommu API.
>>>>>
>>>>> Changes for V2:
>>>>>    - use limits in iommu_setup_dma_ops()
>>>>>    - rebased to current upstream
>>>>>
>>>>> Sebastian Ott (3):
>>>>>     iommu/amd: Parse supported address sizes from IVRS
>>>>>     iommu/amd: Restrict aperture for domains to conform with IVRS
>>>>>     iommu/amd: Actually enforce geometry aperture
>>>>
>>>> Thanks for the changes. May I ask what the reason for those
>>>> changes are?
>>>> AFAIK all AMD IOMMU implementations (in hardware) support full
>>>> 64bit
>>>> address spaces, and the IVRS table might actually be wrong,
>>>> limiting the
>>>> address space in the worst case to only 32 bit.
>>>
>>> It's not the IOMMU, but we've encountered devices that are capable
>>> of
>>> more than
>>> 32- but less than 64- bit IOVA, and there's no way to express that
>>> to
>>> the IOVA
>>> allocator in the PCIe spec. Our solution was to have our platforms
>>> express an
>>> IVRS entry that says the IOMMU is capable of 48-bit, which these
>>> devices
>>> can generate.
>>> 48 bits is plenty of address space in this generation for the
>>> application we have.
>>
>> Hmm, for constraints of individual devices, it should really be their
>> drivers' job to call dma_set_mask*() with the appropriate value in the
>> first place rather than just assuming that 64 means anything >32. If
>> it's a case where the device itself technically is 64-bit capable, but
>> an upstream bridge is constraining it, then that limit can also be
>> described either by dedicated firmware properties (e.g. ACPI _DMA) or
>> with a quirk like via_no_dac().
>>
>> Robin.
> 
> You cannot rely on the device driver only because the device driver
> attach might be a generic one like vfio-pci, for instance, that doesn't
> have any device specific knowledge.

Indeed, but on the other hand a generic driver that doesn't know the 
device is highly unlikely to set up any DMA transactions by itself 
either. In the case of VFIO, it would then be the guest/userspace 
driver's responsibility to take the equivalent action to avoid 
allocating addresses the hardware can't actually use.

I'm mostly just wary that trying to fake up a per-device restriction as 
a global one is a bit crude, and has the inherent problem that whatever 
you think the lowest common denominator is, there's the potential for 
some device to be hotplugged in later and break the assumption you've 
already had to commit to.

And of course I am taking a bit of a DMA-API-centric viewpoint here - I 
think exposing per-device properties like bus_dma_limit that aren't 
easily identifiable for VFIO users to take into account is still rather 
an open problem.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Sironi, Filippo via iommu]

On Fri, 2020-07-17 at 15:36 +0100, Robin Murphy wrote:
> On 2020-07-17 14:22, Sironi, Filippo wrote:
> > On Fri, 2020-07-17 at 10:47 +0100, Robin Murphy wrote:
> > > 
> > > On 2020-07-17 10:20, Sebastian Ott via iommu wrote:
> > > > Hello Joerg,
> > > > 
> > > > On 2020-07-10 14:31, Joerg Roedel wrote:
> > > > > On Wed, Jul 01, 2020 at 12:46:31AM +0200, Sebastian Ott wrote:
> > > > > > The IVRS ACPI table specifies maximum address sizes for I/O
> > > > > > virtual
> > > > > > addresses that can be handled by the IOMMUs in the system.
> > > > > > Parse
> > > > > > that
> > > > > > data from the IVRS header to provide aperture information
> > > > > > for
> > > > > > DMA
> > > > > > mappings and users of the iommu API.
> > > > > > 
> > > > > > Changes for V2:
> > > > > >    - use limits in iommu_setup_dma_ops()
> > > > > >    - rebased to current upstream
> > > > > > 
> > > > > > Sebastian Ott (3):
> > > > > >     iommu/amd: Parse supported address sizes from IVRS
> > > > > >     iommu/amd: Restrict aperture for domains to conform with
> > > > > > IVRS
> > > > > >     iommu/amd: Actually enforce geometry aperture
> > > > > 
> > > > > Thanks for the changes. May I ask what the reason for those
> > > > > changes are?
> > > > > AFAIK all AMD IOMMU implementations (in hardware) support full
> > > > > 64bit
> > > > > address spaces, and the IVRS table might actually be wrong,
> > > > > limiting the
> > > > > address space in the worst case to only 32 bit.
> > > > 
> > > > It's not the IOMMU, but we've encountered devices that are
> > > > capable
> > > > of
> > > > more than
> > > > 32- but less than 64- bit IOVA, and there's no way to express
> > > > that
> > > > to
> > > > the IOVA
> > > > allocator in the PCIe spec. Our solution was to have our
> > > > platforms
> > > > express an
> > > > IVRS entry that says the IOMMU is capable of 48-bit, which these
> > > > devices
> > > > can generate.
> > > > 48 bits is plenty of address space in this generation for the
> > > > application we have.
> > > 
> > > Hmm, for constraints of individual devices, it should really be
> > > their
> > > drivers' job to call dma_set_mask*() with the appropriate value in
> > > the
> > > first place rather than just assuming that 64 means anything >32.
> > > If
> > > it's a case where the device itself technically is 64-bit capable,
> > > but
> > > an upstream bridge is constraining it, then that limit can also be
> > > described either by dedicated firmware properties (e.g. ACPI _DMA)
> > > or
> > > with a quirk like via_no_dac().
> > > 
> > > Robin.
> > 
> > You cannot rely on the device driver only because the device driver
> > attach might be a generic one like vfio-pci, for instance, that
> > doesn't
> > have any device specific knowledge.
> 
> Indeed, but on the other hand a generic driver that doesn't know the
> device is highly unlikely to set up any DMA transactions by itself
> either. In the case of VFIO, it would then be the guest/userspace
> driver's responsibility to take the equivalent action to avoid
> allocating addresses the hardware can't actually use.

I don't believe that we want to trust a userspace driver here, this may
result in hosts becoming unstable because devices are asked to do things
they aren't meant to do (e.g., DMA beyond 48 bits).

> I'm mostly just wary that trying to fake up a per-device restriction
> as
> a global one is a bit crude, and has the inherent problem that
> whatever
> you think the lowest common denominator is, there's the potential for
> some device to be hotplugged in later and break the assumption you've
> already had to commit to.

I agree, if the BIOS sets up an IVRS table with aperture of 48 bits and
all of a sudden we hotplug a device that only support 36 bits we're in a
bad place.

> And of course I am taking a bit of a DMA-API-centric viewpoint here -
> I
> think exposing per-device properties like bus_dma_limit that aren't
> easily identifiable for VFIO users to take into account is still
> rather
> an open problem.
> 
> Robin.

The use of ACPI _DMA that you suggest looks interesting and would allow
the kernel to prevent a dumb userspace driver using VFIO to make damage,
I think.

It doesn't look like there's much support for ACPI _DMA though. Are you
aware of existing efforts on this front?

Filippo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[joro]

On Fri, Jul 17, 2020 at 03:15:43PM +0000, Sironi, Filippo wrote:
> I don't believe that we want to trust a userspace driver here, this may
> result in hosts becoming unstable because devices are asked to do things
> they aren't meant to do (e.g., DMA beyond 48 bits).

How is the hosts stability affected when a device is programmed with
handles outside of its DMA mask?

Anyway, someone needs to know how to use the device in the end, and this
entity also needs to know the DMA mask of the device and pass it down to
path to the dma-iommu code.

Regards,

	Joerg

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[Sironi, Filippo via iommu]

On Wed, 2020-07-22 at 14:19 +0200, joro@8bytes.org wrote:
> 
> On Fri, Jul 17, 2020 at 03:15:43PM +0000, Sironi, Filippo wrote:
> > I don't believe that we want to trust a userspace driver here, this
> > may
> > result in hosts becoming unstable because devices are asked to do
> > things
> > they aren't meant to do (e.g., DMA beyond 48 bits).
> 
> How is the hosts stability affected when a device is programmed with
> handles outside of its DMA mask?

I wouldn't be surprised if a PCIe device raises a PCIe SERR if it is
asked to do DMA beyond its abilities.

> Anyway, someone needs to know how to use the device in the end, and
> this
> entity also needs to know the DMA mask of the device and pass it down
> to
> path to the dma-iommu code.
> 
> Regards,
> 
>         Joerg

I agree, device drivers should do the right thing and if we have generic
device drivers they may need "quirks" based on VID:DID to do the right
thing.

Still, I believe that the VFIO case is special because the device driver
is effectively in userspace I really think that trusting userspace isn't
quite correct (and we can keep discussing on this front).

I think that this discussion is orthogonal wrt the spirit of the
patches. They are actually adding a few bits to the AMD IOMMU driver
(and propagating them to the right places) to implement a portion of the
specification that wasn't implemented, i.e., relying on the IVRS table.
These patches are valuable independently on the content of the IVRS
table, be it 32, 48, or 64 bits.

Filippo





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 0/3] iommu/amd: I/O VA address limits

[joro]

On Wed, Jul 22, 2020 at 12:34:57PM +0000, Sironi, Filippo wrote:
> On Wed, 2020-07-22 at 14:19 +0200, joro@8bytes.org wrote:

> I wouldn't be surprised if a PCIe device raises a PCIe SERR if it is
> asked to do DMA beyond its abilities.

Yeah, but that would also make it impossible to safely assign the device
to any untrusted entity, like a guest of user-space driver.

> I think that this discussion is orthogonal wrt the spirit of the
> patches. They are actually adding a few bits to the AMD IOMMU driver
> (and propagating them to the right places) to implement a portion of the
> specification that wasn't implemented, i.e., relying on the IVRS table.
> These patches are valuable independently on the content of the IVRS
> table, be it 32, 48, or 64 bits.

You are right from a technical point of view, and the patches are as
well. The problem I see is that there are a lot of systems out there
with an AMD IOMMU and possibly broken ACPI tables. And if the driver
starts checking this field now it is possible that it breaks formerly
working setups.

So doing this needs a strong reason, like upcoming hardware that has
lower limits in the supported address space size than before. The
use-case you have described is not a strong enough reason to take the
risk.

Regards,

	Joerg
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu