[PATCH 0/7] iommu/amd: Enforce IOMMU restrictions for SNP-enabled system

[PATCH 0/7] iommu/amd: Enforce IOMMU restrictions for SNP-enabled system

[Suravee Suthikulpanit via iommu]

SNP-enabled system requires IOMMU v1 page table to be configured with
non-zero DTE[Mode] for DMA-capable devices. This effects a number of
usecases such as IOMMU pass-through mode and AMD IOMMUv2 APIs for
binding/unbinding pasid.

The series introduce a global variable to check SNP-enabled state
during driver initialization, and use it to enforce the SNP restrictions
during runtime.

Also, for non-DMA-capable devices such as IOAPIC, the recommendation
is to set DTE[TV] and DTE[Mode] to zero on SNP-enabled system.
Therefore, additinal checks is added before setting DTE[TV].

Testing:
  - Tested booting and verify dmesg.
  - Tested booting with iommu=pt
  - Tested loading amd_iommu_v2 driver
  - Tested changing the iommu domain at runtime
  - Tested booting SEV/SNP-enabled guest

Pre-requisite:
  - [PATCH v3 00/35] iommu/amd: Add multiple PCI segments support
    https://lore.kernel.org/linux-iommu/20220511072141.15485-29-vasant.hegde@amd.com/T/

Note:
  - Previously discussed on here:
    [PATCH v2] iommu/amd: Set translation valid bit only when IO page tables are in used
    https://www.spinics.net/lists/kernel/msg4351005.html

Best Regards,
Suravee

Brijesh Singh (1):
  iommu/amd: Introduce function to check SEV-SNP support

Suravee Suthikulpanit (6):
  iommu/amd: Process all IVHDs before enabling IOMMU features
  iommu/amd: Introduce a global variable for tracking SNP enable status
  iommu/amd: Set translation valid bit only when IO page tables are in
    use
  iommu: Add domain_type_supported() callback in iommu_ops
  iommu/amd: Do not support IOMMU_DOMAIN_IDENTITY when SNP is enabled
  iommu/amd: Do not support IOMMUv2 APIs when SNP is enabled

 drivers/iommu/amd/amd_iommu_types.h |  11 +++
 drivers/iommu/amd/init.c            | 111 +++++++++++++++++++++++-----
 drivers/iommu/amd/iommu.c           |  31 +++++++-
 drivers/iommu/iommu.c               |  13 +++-
 include/linux/iommu.h               |  11 +++
 5 files changed, 153 insertions(+), 24 deletions(-)

-- 
2.32.0

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

[PATCH 1/7] iommu/amd: Process all IVHDs before enabling IOMMU features

[Suravee Suthikulpanit via iommu]

The ACPI IVRS table can contain multiple IVHD blocks. Each block contains
information used to initialize each IOMMU instance.

Currently, init_iommu_all sequentially process IVHD block and initialize
IOMMU instance one-by-one. However, certain features require all IOMMUs
to be configured in the same way system-wide. In case certain IVHD blocks
contain inconsistent information (most likely FW bugs), the driver needs
to go through and try to revert settings on IOMMUs that have already been
configured.

A solution is to split IOMMU initialization into 2 phases:

Phase 1 processes information of the IVRS table for all IOMMU instances.
This allow all IVHDs to be processed prior to enabling features.

Phase 2 iterates through all IOMMU instances and enabling each features.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 8877d2a20398..6a4a019f1e1d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1687,7 +1687,6 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 				 struct acpi_table_header *ivrs_base)
 {
 	struct amd_iommu_pci_seg *pci_seg;
-	int ret;
 
 	pci_seg = get_pci_segment(h->pci_seg, ivrs_base);
 	if (pci_seg == NULL)
@@ -1768,6 +1767,13 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 	if (!iommu->mmio_base)
 		return -ENOMEM;
 
+	return init_iommu_from_acpi(iommu, h);
+}
+
+static int __init init_iommu_one_late(struct amd_iommu *iommu)
+{
+	int ret;
+
 	if (alloc_cwwb_sem(iommu))
 		return -ENOMEM;
 
@@ -1789,10 +1795,6 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 	if (amd_iommu_pre_enabled)
 		amd_iommu_pre_enabled = translation_pre_enabled(iommu);
 
-	ret = init_iommu_from_acpi(iommu, h);
-	if (ret)
-		return ret;
-
 	if (amd_iommu_irq_remap) {
 		ret = amd_iommu_create_irq_domain(iommu);
 		if (ret)
@@ -1803,7 +1805,7 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h,
 	 * Make sure IOMMU is not considered to translate itself. The IVRS
 	 * table tells us so, but this is a lie!
 	 */
-	pci_seg->rlookup_table[iommu->devid] = NULL;
+	iommu->pci_seg->rlookup_table[iommu->devid] = NULL;
 
 	return 0;
 }
@@ -1873,6 +1875,12 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	}
 	WARN_ON(p != end);
 
+	for_each_iommu(iommu) {
+		ret = init_iommu_one_late(iommu);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.32.0

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

[PATCH 2/7] iommu/amd: Introduce a global variable for tracking SNP enable status

[Suravee Suthikulpanit via iommu]

IOMMU support for SNP feature is detected via the EFR[SNPSup] bit.
Also, it is required that EFR[SNPSup] are consistent across all IOMMU
instances.

This information is needed early in the boot process,
since it is used to determine how IOMMU driver configures several other
IOMMU features and data structures (e.g. as soon as the IOMMU driver
finishes parsing IVHDs).

Introduce a global variable for tracking the SNP support status, which is
initialized before enabling the rest of IOMMU features.

Also throw a warning if found inconsistency EFR[SNPSup] among IOMMU
instances.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 42 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 6a4a019f1e1d..3965bd3f4f67 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -164,6 +164,8 @@ static bool amd_iommu_disabled __initdata;
 static bool amd_iommu_force_enable __initdata;
 static int amd_iommu_target_ivhd_type;
 
+static bool amd_iommu_snp_sup;
+
 LIST_HEAD(amd_iommu_pci_seg_list);	/* list of all PCI segments */
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
@@ -355,7 +357,7 @@ static void iommu_set_cwwb_range(struct amd_iommu *iommu)
 	u64 start = iommu_virt_to_phys((void *)iommu->cmd_sem);
 	u64 entry = start & PM_ADDR_MASK;
 
-	if (!iommu_feature(iommu, FEATURE_SNP))
+	if (!amd_iommu_snp_sup)
 		return;
 
 	/* Note:
@@ -770,7 +772,7 @@ static void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
 	void *buf = (void *)__get_free_pages(gfp, order);
 
 	if (buf &&
-	    iommu_feature(iommu, FEATURE_SNP) &&
+	    amd_iommu_snp_sup &&
 	    set_memory_4k((unsigned long)buf, (1 << order))) {
 		free_pages((unsigned long)buf, order);
 		buf = NULL;
@@ -1836,6 +1838,37 @@ static u8 get_highest_supported_ivhd_type(struct acpi_table_header *ivrs)
 	return last_type;
 }
 
+/*
+ * SNP is enabled system-wide. So, iterate through all the IOMMUs to
+ * verify all EFR[SNPSup] bits are set, and use global variable to track
+ * whether the feature is supported.
+ */
+static void __init init_snp_global(void)
+{
+	struct amd_iommu *iommu;
+
+	for_each_iommu(iommu) {
+		if (iommu_feature(iommu, FEATURE_SNP)) {
+			amd_iommu_snp_sup = true;
+			continue;
+		}
+
+		/*
+		 * Warn and mark SNP as not supported if there is inconsistency
+		 * in any of the IOMMU.
+		 */
+		if (amd_iommu_snp_sup && !list_is_first(&iommu->list, &amd_iommu_list)) {
+			pr_err(FW_BUG "iommu%d (%04x:%02x:%02x.%01x): Found inconsistent EFR[SNPSup].\n",
+			       iommu->index, iommu->pci_seg->id, PCI_BUS_NUM(iommu->devid),
+			       PCI_SLOT(iommu->devid), PCI_FUNC(iommu->devid));
+			pr_err(FW_BUG "Disable SNP support\n");
+			amd_iommu_snp_sup = false;
+		}
+		return;
+	}
+	amd_iommu_snp_sup = true;
+}
+
 /*
  * Iterates over all IOMMU entries in the ACPI table, allocates the
  * IOMMU structure and initializes it with init_iommu_one()
@@ -1875,6 +1908,8 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	}
 	WARN_ON(p != end);
 
+	init_snp_global();
+
 	for_each_iommu(iommu) {
 		ret = init_iommu_one_late(iommu);
 		if (ret)
@@ -2095,6 +2130,9 @@ static void print_iommu_info(void)
 			if (iommu->features & FEATURE_GAM_VAPIC)
 				pr_cont(" GA_vAPIC");
 
+			if (iommu->features & FEATURE_SNP)
+				pr_cont(" SNP");
+
 			pr_cont("\n");
 		}
 	}
-- 
2.32.0

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

[PATCH 3/7] iommu/amd: Introduce function to check SEV-SNP support

[Suravee Suthikulpanit via iommu]

From: Brijesh Singh <brijesh.singh@amd.com>

The SEV-SNP support requires that IOMMU must to enabled. It also prohibits
IOMMU configurations where DTE[Mode]=0, which means the SEV-SNP feature is
not supported with IOMMU passthrough domain (a.k.a IOMMU_DOMAIN_IDENTITY),
or when AMD IOMMU driver is configured to not use the IOMMU host (v1) page
table.

Otherwise, the SNP_INIT command (used for initializing firmware) will fail.

Unlike other IOMMU features, SNP feature does not have an enable bit in
the IOMMU control register. Instead, the feature is considered enabled
when SNP_INIT command is executed, which is done by a separte driver.

Introduce iommu_sev_snp_supported() for checking if IOMMU supports
the SEV-SNP feature, and an amd_iommu_snp_en global variable to keep track
of SNP enable status.

Please see the IOMMU spec section 2.12 for further details.

Tested-by: Ashish Kalra <Vashish.kalra@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 11 ++++++++
 drivers/iommu/amd/init.c            | 39 ++++++++++++++++++++++-------
 drivers/iommu/amd/iommu.c           |  4 +--
 include/linux/iommu.h               |  9 +++++++
 4 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 328572cf6fa5..6552c0da8f32 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -450,6 +450,9 @@ extern bool amd_iommu_irq_remap;
 /* kmem_cache to get tables with 128 byte alignement */
 extern struct kmem_cache *amd_iommu_irq_cache;
 
+/* SNP is enabled on the system? */
+extern bool amd_iommu_snp_en;
+
 #define PCI_SBDF_TO_SEGID(sbdf)		(((sbdf) >> 16) & 0xffff)
 #define PCI_SBDF_TO_DEVID(sbdf)		((sbdf) & 0xffff)
 #define PCI_SEG_DEVID_TO_SBDF(seg, devid)	((((u32)(seg) & 0xffff) << 16) | \
@@ -999,4 +1002,12 @@ extern struct amd_irte_ops irte_32_ops;
 extern struct amd_irte_ops irte_128_ops;
 #endif
 
+/*
+ * ACPI table definitions
+ *
+ * These data structures are laid over the table to parse the important values
+ * out of it.
+ */
+extern struct iommu_ops amd_iommu_ops;
+
 #endif /* _ASM_X86_AMD_IOMMU_TYPES_H */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 3965bd3f4f67..da32e7bdd1fa 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -88,15 +88,6 @@
 #define IVRS_GET_SBDF_ID(seg, bus, dev, fd)	(((seg & 0xffff) << 16) | ((bus & 0xff) << 8) \
 						 | ((dev & 0x1f) << 3) | (fn & 0x7))
 
-/*
- * ACPI table definitions
- *
- * These data structures are laid over the table to parse the important values
- * out of it.
- */
-
-extern const struct iommu_ops amd_iommu_ops;
-
 /*
  * structure describing one IOMMU in the ACPI table. Typically followed by one
  * or more ivhd_entrys.
@@ -166,6 +157,9 @@ static int amd_iommu_target_ivhd_type;
 
 static bool amd_iommu_snp_sup;
 
+bool amd_iommu_snp_en;
+EXPORT_SYMBOL(amd_iommu_snp_en);
+
 LIST_HEAD(amd_iommu_pci_seg_list);	/* list of all PCI segments */
 LIST_HEAD(amd_iommu_list);		/* list of all AMD IOMMUs in the
 					   system */
@@ -3543,3 +3537,30 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64
 
 	return iommu_pc_get_set_reg(iommu, bank, cntr, fxn, value, true);
 }
+
+bool iommu_sev_snp_supported(void)
+{
+	/*
+	 * The SEV-SNP support requires that IOMMU must be enabled, and is
+	 * not configured in the passthrough mode.
+	 */
+	if (no_iommu || iommu_default_passthrough()) {
+		pr_err("SEV-SNP: IOMMU is either disabled or configured in passthrough mode.\n");
+		return false;
+	}
+
+	amd_iommu_snp_en = amd_iommu_snp_sup;
+	if (amd_iommu_snp_en)
+		pr_info("SNP enabled\n");
+
+	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
+	if ((amd_iommu_pgtable != AMD_IOMMU_V1) &&
+	     amd_iommu_snp_en) {
+		pr_info("Force to using AMD IOMMU v1 page table due to SNP\n");
+		amd_iommu_pgtable = AMD_IOMMU_V1;
+		amd_iommu_ops.pgsize_bitmap = AMD_IOMMU_PGSIZES;
+	}
+
+	return amd_iommu_snp_en;
+}
+EXPORT_SYMBOL_GPL(iommu_sev_snp_supported);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 3e1f0fa42ec3..b9dc0d4b6d77 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -70,7 +70,7 @@ LIST_HEAD(acpihid_map);
  * Domain for untranslated devices - only allocated
  * if iommu=pt passed on kernel cmd line.
  */
-const struct iommu_ops amd_iommu_ops;
+struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
 int amd_iommu_max_glx_val = -1;
@@ -2368,7 +2368,7 @@ static int amd_iommu_def_domain_type(struct device *dev)
 	return 0;
 }
 
-const struct iommu_ops amd_iommu_ops = {
+struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
 	.probe_device = amd_iommu_probe_device,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9208eca4b0d1..fecb72e1b11b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -675,6 +675,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
 void iommu_sva_unbind_device(struct iommu_sva *handle);
 u32 iommu_sva_get_pasid(struct iommu_sva *handle);
 
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+bool iommu_sev_snp_supported(void);
+#else
+static inline bool iommu_sev_snp_supported(void) { return false; }
+#endif
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -1031,6 +1037,9 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
 {
 	return NULL;
 }
+
+static inline bool iommu_sev_snp_supported(void) { return false; }
+
 #endif /* CONFIG_IOMMU_API */
 
 /**
-- 
2.32.0

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

Re: [PATCH 3/7] iommu/amd: Introduce function to check SEV-SNP support

[Suthikulpanit, Suravee via iommu]

On 6/13/2022 8:24 AM, Suravee Suthikulpanit wrote:
> @@ -3543,3 +3537,30 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64
>   
>   	return iommu_pc_get_set_reg(iommu, bank, cntr, fxn, value, true);
>   }
> +
> +bool iommu_sev_snp_supported(void)
> +{
> +	/*
> +	 * The SEV-SNP support requires that IOMMU must be enabled, and is
> +	 * not configured in the passthrough mode.
> +	 */
> +	if (no_iommu || iommu_default_passthrough()) {
> +		pr_err("SEV-SNP: IOMMU is either disabled or configured in passthrough mode.\n");
> +		return false;
> +	}
> +
> +	amd_iommu_snp_en = amd_iommu_snp_sup;
> +	if (amd_iommu_snp_en)
> +		pr_info("SNP enabled\n");
> +
> +	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
> +	if ((amd_iommu_pgtable != AMD_IOMMU_V1) &&
> +	     amd_iommu_snp_en) {
> +		pr_info("Force to using AMD IOMMU v1 page table due to SNP\n");
> +		amd_iommu_pgtable = AMD_IOMMU_V1;
> +		amd_iommu_ops.pgsize_bitmap = AMD_IOMMU_PGSIZES;
> +	}
> +
> +	return amd_iommu_snp_en;
> +}
> +EXPORT_SYMBOL_GPL(iommu_sev_snp_supported);

I need to guard this function w/ #ifdef CONFIG_AMD_MEM_ENCRYPT to prevent build error when CONFIG_AMD_MEM_ENCRYPT=n.
I will send out v2 to fix this.

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

[PATCH 4/7] iommu/amd: Set translation valid bit only when IO page tables are in use

[Suravee Suthikulpanit via iommu]

On AMD system with SNP enabled, IOMMU hardware checks the host translation
valid (TV) and guest translation valid (GV) bits in the device table entry
(DTE) before accessing the corresponded page tables.

However, current IOMMU driver sets the TV bit for all devices regardless
of whether the host page table is in use. This results in
ILLEGAL_DEV_TABLE_ENTRY event for devices, which do not the host page
table root pointer set up.

Thefore, when SNP is enabled, only set TV bit when DMA remapping is not
used, which is when domain ID in the AMD IOMMU device table entry (DTE)
is zero.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c  |  3 ++-
 drivers/iommu/amd/iommu.c | 15 +++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index da32e7bdd1fa..a9152d3f33bf 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2546,7 +2546,8 @@ static void init_device_table_dma(struct amd_iommu_pci_seg *pci_seg)
 
 	for (devid = 0; devid <= pci_seg->last_bdf; ++devid) {
 		__set_dev_entry_bit(dev_table, devid, DEV_ENTRY_VALID);
-		__set_dev_entry_bit(dev_table, devid, DEV_ENTRY_TRANSLATION);
+		if (!amd_iommu_snp_en)
+			__set_dev_entry_bit(dev_table, devid, DEV_ENTRY_TRANSLATION);
 	}
 }
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b9dc0d4b6d77..ca4647f04382 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1552,7 +1552,14 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 
 	pte_root |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
 		    << DEV_ENTRY_MODE_SHIFT;
-	pte_root |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V | DTE_FLAG_TV;
+	pte_root |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V;
+
+	/*
+	 * When SNP is enabled, Only set TV bit when IOMMU
+	 * page translation is in use.
+	 */
+	if (!amd_iommu_snp_en || (domain->id != 0))
+		pte_root |= DTE_FLAG_TV;
 
 	flags = dev_table[devid].data[1];
 
@@ -1612,7 +1619,11 @@ static void clear_dte_entry(struct amd_iommu *iommu, u16 devid)
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 
 	/* remove entry from the device table seen by the hardware */
-	dev_table[devid].data[0]  = DTE_FLAG_V | DTE_FLAG_TV;
+	dev_table[devid].data[0]  = DTE_FLAG_V;
+
+	if (!amd_iommu_snp_en)
+		dev_table[devid].data[0] |= DTE_FLAG_TV;
+
 	dev_table[devid].data[1] &= DTE_FLAG_MASK;
 
 	amd_iommu_apply_erratum_63(iommu, devid);
-- 
2.32.0

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

[PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Suravee Suthikulpanit via iommu]

When user requests to change IOMMU domain to a new type, IOMMU generic
layer checks the requested type against the default domain type returned
by vendor-specific IOMMU driver.

However, there is only one default domain type, and current mechanism
does not allow if the requested type does not match the default type.

Introducing check_domain_type_supported() callback in iommu_ops,
which allows IOMMU generic layer to check with vendor-specific IOMMU driver
whether the requested type is supported. This allows user to request
types other than the default type.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/iommu.c | 13 ++++++++++++-
 include/linux/iommu.h |  2 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..4afb956ce083 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1521,6 +1521,16 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
+static bool iommu_domain_type_supported(struct device *dev, int type)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	if (ops->domain_type_supported)
+		return ops->domain_type_supported(dev, type);
+
+	return true;
+}
+
 static int iommu_get_def_domain_type(struct device *dev)
 {
 	const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -2937,7 +2947,8 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
 		 * domain the device was booted with
 		 */
 		type = dev_def_dom ? : iommu_def_domain_type;
-	} else if (dev_def_dom && type != dev_def_dom) {
+	} else if (!iommu_domain_type_supported(dev, type) ||
+		   (dev_def_dom && type != dev_def_dom)) {
 		dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
 				    iommu_domain_type_str(type));
 		ret = -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fecb72e1b11b..40c47ab15005 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -214,6 +214,7 @@ struct iommu_iotlb_gather {
  *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
  *		- IOMMU_DOMAIN_DMA: must use a dma domain
  *		- 0: use the default setting
+ * @domain_type_supported: check if the specified domain type is supported
  * @default_domain_ops: the default ops for domains
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  * @owner: Driver module providing these ops
@@ -252,6 +253,7 @@ struct iommu_ops {
 			     struct iommu_page_response *msg);
 
 	int (*def_domain_type)(struct device *dev);
+	bool (*domain_type_supported)(struct device *dev, int type);
 
 	const struct iommu_domain_ops *default_domain_ops;
 	unsigned long pgsize_bitmap;
-- 
2.32.0

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

Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Robin Murphy]

On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
> When user requests to change IOMMU domain to a new type, IOMMU generic
> layer checks the requested type against the default domain type returned
> by vendor-specific IOMMU driver.
> 
> However, there is only one default domain type, and current mechanism
> does not allow if the requested type does not match the default type.

I don't really follow the reasoning here. If a driver's def_domain_type 
callback returns a specific type, it's saying that the device *has* to 
have that specific domain type for driver/platform-specific reasons. If 
that's not the case, then the driver shouldn't say so in the first place.

> Introducing check_domain_type_supported() callback in iommu_ops,
> which allows IOMMU generic layer to check with vendor-specific IOMMU driver
> whether the requested type is supported. This allows user to request
> types other than the default type.

Note also that you're only adding this in the sysfs path - what about 
the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?

AFAICS there shouldn't need to be any core-level changes to support 
this. We already have drivers which don't support passthrough at all, so 
conditionally not supporting it should be no big deal. What should 
happen currently is that def_domain_type returns 0 for "don't care", 
then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so 
iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.

Thanks,
Robin.

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/iommu.c | 13 ++++++++++++-
>   include/linux/iommu.h |  2 ++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2c45b85b9fc..4afb956ce083 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1521,6 +1521,16 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   
> +static bool iommu_domain_type_supported(struct device *dev, int type)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +
> +	if (ops->domain_type_supported)
> +		return ops->domain_type_supported(dev, type);
> +
> +	return true;
> +}
> +
>   static int iommu_get_def_domain_type(struct device *dev)
>   {
>   	const struct iommu_ops *ops = dev_iommu_ops(dev);
> @@ -2937,7 +2947,8 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>   		 * domain the device was booted with
>   		 */
>   		type = dev_def_dom ? : iommu_def_domain_type;
> -	} else if (dev_def_dom && type != dev_def_dom) {
> +	} else if (!iommu_domain_type_supported(dev, type) ||
> +		   (dev_def_dom && type != dev_def_dom)) {
>   		dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
>   				    iommu_domain_type_str(type));
>   		ret = -EINVAL;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fecb72e1b11b..40c47ab15005 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -214,6 +214,7 @@ struct iommu_iotlb_gather {
>    *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
>    *		- IOMMU_DOMAIN_DMA: must use a dma domain
>    *		- 0: use the default setting
> + * @domain_type_supported: check if the specified domain type is supported
>    * @default_domain_ops: the default ops for domains
>    * @pgsize_bitmap: bitmap of all possible supported page sizes
>    * @owner: Driver module providing these ops
> @@ -252,6 +253,7 @@ struct iommu_ops {
>   			     struct iommu_page_response *msg);
>   
>   	int (*def_domain_type)(struct device *dev);
> +	bool (*domain_type_supported)(struct device *dev, int type);
>   
>   	const struct iommu_domain_ops *default_domain_ops;
>   	unsigned long pgsize_bitmap;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Suthikulpanit, Suravee via iommu]

Robin,

On 6/13/2022 4:31 PM, Robin Murphy wrote:
> On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
>> When user requests to change IOMMU domain to a new type, IOMMU generic
>> layer checks the requested type against the default domain type returned
>> by vendor-specific IOMMU driver.
>>
>> However, there is only one default domain type, and current mechanism
>> does not allow if the requested type does not match the default type.
> 
> I don't really follow the reasoning here. If a driver's def_domain_type callback returns a specific type, it's saying that the device *has* to have that specific domain type for driver/platform-specific reasons. 

Agree, and I understand this part.

> If that's not the case, then the driver shouldn't say so in the first place.

Considering the case:
1. Boot w/ default domain = IOMMU_DOMAIN_DMA_FQ
2. User wants to change to IOMMU_DOMAIN_IDENTITY, which is not supported by IOMMU driver. In this case, IOMMU driver can return IOMMU_DOMAIN_DMA_FQ and prevent the mode change.
3. However, if user want to change to IOMMU_DOMAIN_DMA. The driver can support this. However, since the def_domain_type() returns IOMMU_DOMAIN_DMA_FQ, it ends up prevent the mode change.

IIUC, we should support step 3 above. Basically, with the newly proposed interface, it allows us to check with IOMMU driver if it can support certain domain types before trying
to allocate the domain.

> 
>> Introducing check_domain_type_supported() callback in iommu_ops,
>> which allows IOMMU generic layer to check with vendor-specific IOMMU driver
>> whether the requested type is supported. This allows user to request
>> types other than the default type.
> 
> Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?

For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y.
So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3).

Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode.
Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch
to SNP-enabled mode.

> AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.

Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care",
but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc().

Please let me know if I am missing something.

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

Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Robin Murphy]

On 2022-06-13 15:38, Suthikulpanit, Suravee wrote:
> Robin,
> 
> On 6/13/2022 4:31 PM, Robin Murphy wrote:
>> On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
>>> When user requests to change IOMMU domain to a new type, IOMMU generic
>>> layer checks the requested type against the default domain type returned
>>> by vendor-specific IOMMU driver.
>>>
>>> However, there is only one default domain type, and current mechanism
>>> does not allow if the requested type does not match the default type.
>>
>> I don't really follow the reasoning here. If a driver's 
>> def_domain_type callback returns a specific type, it's saying that the 
>> device *has* to have that specific domain type for 
>> driver/platform-specific reasons. 
> 
> Agree, and I understand this part.
> 
>> If 
>> that's not the case, then the driver shouldn't say so in the first place.
> 
> Considering the case:
> 1. Boot w/ default domain = IOMMU_DOMAIN_DMA_FQ
> 2. User wants to change to IOMMU_DOMAIN_IDENTITY, which is not supported 
> by IOMMU driver. In this case, IOMMU driver can return 
> IOMMU_DOMAIN_DMA_FQ and prevent the mode change.
> 3. However, if user want to change to IOMMU_DOMAIN_DMA. The driver can 
> support this. However, since the def_domain_type() returns 
> IOMMU_DOMAIN_DMA_FQ, it ends up prevent the mode change.

Why would a driver be forcing IOMMU_DOMAIN_DMA_FQ for a device though? 
Nobody's doing that today, and semantically it wouldn't really make 
sense - forcing translation to deny passthrough on a device-specific 
basis (beyond the common handling of untrusted devices) *might* be a 
thing, but the performance/strictness tradeoff of using a flush queue or 
not is surely a subjective user decision, not an objective platform one.

> IIUC, we should support step 3 above. Basically, with the newly proposed 
> interface, it allows us to check with IOMMU driver if it can support 
> certain domain types before trying
> to allocate the domain.

Indeed we could do that - as a much more comprehensive change to the 
internal domain_alloc interfaces - but do we really need to? If we 
succeed at allocating a domain then we know it's supported; if it fails 
then we can't give the user what they asked for, regardless of the exact 
reason why - what do we gain from doubling the number of potential 
failure paths that we have to handle?

>>> Introducing check_domain_type_supported() callback in iommu_ops,
>>> which allows IOMMU generic layer to check with vendor-specific IOMMU driver 
>>>
>>> whether the requested type is supported. This allows user to request
>>> types other than the default type.
>>
>> Note also that you're only adding this in the sysfs path - what about 
>> the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?
> 
> For SNP case, we cannot enable SNP if iommu=off or iommu=pt or 
> iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y.
> So, when another driver tries to enable SNP, the IOMMU driver prevents 
> it (see iommu_sev_snp_supported() in patch 3).

Ugh, I hadn't looked too closely at the other patches, but an interface 
that looks like a simple "is this feature supported?" check with a 
secret side-effect of changing global behaviour as well? Yuck :(

What external drivers are expected to have the authority to affect the 
entire system and call that? The fact that you're exporting it suggests 
they could be loaded from modules *after* v2 features have been enabled 
and/or the user has configured a non-default identity domain for a group 
via sysfs... Fun!

> Instead, if we boot with iommu.passhthrough=0, when another driver tries 
> to enable SNP, the IOMMU driver allows this and switch to SNP enable mode.
> Subsequently, if user tries to switch a domain (via sysfs) to 
> IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has 
> already switch
> to SNP-enabled mode.
> 
>> AFAICS there shouldn't need to be any core-level changes to support 
>> this. We already have drivers which don't support passthrough at all, 
>> so conditionally not supporting it should be no big deal. What should 
>> happen currently is that def_domain_type returns 0 for "don't care", 
>> then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, 
>> so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.
> 
> Technically, we can do it the way you suggest. But isn't this confusing? 
> At first, def_domain_type() returns 0 for "don't care",
> but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when 
> trying to call domain_alloc().

Yes, that's how it works; def_domain_type is responsible for quirking 
individual *devices* that need to have a specific domain type (in 
practice, devices which need identity mapping), while domain_alloc is 
responsible for saying which domain types the driver supports as a 
whole, by allocating them or not as appropriate.

We don't have a particularly neat way to achieve the negative of 
def_domain_type - i.e. saying that a specific device *can't* use a 
specific otherwise-supported domain type - other than subsequently 
failing in attach_dev, but so far we've not needed such a thing. And if 
SNP is expected to be mutually exclusive with identity domain support 
globally, then we still shouldn't need it.

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

Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Suthikulpanit, Suravee via iommu]

Robin,

On 6/14/2022 4:51 PM, Robin Murphy wrote:
> On 2022-06-13 15:38, Suthikulpanit, Suravee wrote:
>> Robin,
>>
>> On 6/13/2022 4:31 PM, Robin Murphy wrote:
>>>
>>>> Introducing check_domain_type_supported() callback in iommu_ops,
>>>> which allows IOMMU generic layer to check with vendor-specific IOMMU driver
>>>> whether the requested type is supported. This allows user to request
>>>> types other than the default type.
>>>
>>> Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?
>>
>> For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y.
>> So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3).
> 
> Ugh, I hadn't looked too closely at the other patches, but an interface that looks like a simple "is this feature supported?" check with a secret side-effect of changing global behaviour as well? Yuck :(
> 
> What external drivers are expected to have the authority to affect the entire system and call that? The fact that you're exporting it suggests they could be loaded from modules *after* v2 features have been enabled and/or the user has configured a non-default identity domain for a group via sysfs... Fun!

I see your point.

Currently, the function to enable SNP will be called from SEV driver when it tries to enable SNP support globally on the system.
This is done during fs_initcall(), which is early in the boot process. I can also add a guard code to make sure that this won't
be done after a certain phase.

>> Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode.
>> Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch
>> to SNP-enabled mode.
>>
>>> AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.
>>
>> Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care",
>> but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc().
> 
> Yes, that's how it works; def_domain_type is responsible for quirking individual *devices* that need to have a specific domain type (in practice, devices which need identity mapping), while domain_alloc is responsible for saying which domain types the driver supports as a whole, by allocating them or not as appropriate.
> 
> We don't have a particularly neat way to achieve the negative of def_domain_type - i.e. saying that a specific device *can't* use a specific otherwise-supported domain type - other than subsequently failing in attach_dev, but so far we've not needed such a thing. And if SNP is expected to be mutually exclusive with identity domain support globally, then we still shouldn't need it.

Thanks for your feedback.

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

[PATCH 6/7] iommu/amd: Do not support IOMMU_DOMAIN_IDENTITY when SNP is enabled

[Suravee Suthikulpanit via iommu]

Since DTE[Mode]=0 is prohibited on system, which enables SNP,
the passthrough domain (IOMMU_DOMAIN_IDENTITY) is not support.
Instead, only support IOMMU_DOMAIN_DMA[_FQ] domains.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/iommu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index ca4647f04382..ecde9e08102d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2379,6 +2379,17 @@ static int amd_iommu_def_domain_type(struct device *dev)
 	return 0;
 }
 
+static bool amd_iommu_domain_type_supported(struct device *dev, int type)
+{
+	/*
+	 * Since DTE[Mode]=0 is prohibited on SNP-enabled system,
+	 * default to use IOMMU_DOMAIN_DMA[_FQ].
+	 */
+	if (amd_iommu_snp_en && (type == IOMMU_DOMAIN_IDENTITY))
+		return false;
+	return true;
+}
+
 struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
@@ -2391,6 +2402,7 @@ struct iommu_ops amd_iommu_ops = {
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 	.def_domain_type = amd_iommu_def_domain_type,
+	.domain_type_supported = amd_iommu_domain_type_supported,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
 		.detach_dev	= amd_iommu_detach_device,
-- 
2.32.0

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

[PATCH 7/7] iommu/amd: Do not support IOMMUv2 APIs when SNP is enabled

[Suravee Suthikulpanit via iommu]

The IOMMUv2 APIs (for supporting shared virtual memory with PASID)
configures the domain with IOMMU v2 page table, and sets DTE[Mode]=0.
This configuration cannot be supported on SNP-enabled system.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/init.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index a9152d3f33bf..1565f0fb955a 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3435,7 +3435,12 @@ __setup("ivrs_acpihid",		parse_ivrs_acpihid);
 
 bool amd_iommu_v2_supported(void)
 {
-	return amd_iommu_v2_present;
+	/*
+	 * Since DTE[Mode]=0 is prohibited on SNP-enabled system
+	 * (i.e. EFR[SNPSup]=1), IOMMUv2 page table cannot be used without
+	 * setting up IOMMUv1 page table.
+	 */
+	return amd_iommu_v2_present && !amd_iommu_snp_en;
 }
 EXPORT_SYMBOL(amd_iommu_v2_supported);
 
-- 
2.32.0

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