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

[PATCH v2 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
  - Tested when CONFIG_AMD_MEM_ENCRYPT is not set

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/

Chanages from V1:
(https://lore.kernel.org/linux-iommu/20220613012502.109918-1-suravee.suthikulpanit@amd.com/T/#t )
  - Remove the newly introduced domain_type_supported() callback.
  - Patch 1: Modify existing check_feature_on_all_iommus() instead of
             introducing another helper function to do similar check.
  - Patch 3: Modify to use check_feature_on_all_iommus().
  - Patch 4: Add IOMMU init_state check before enabling SNP.
             Also move the function declaration to include/linux/amd-iommu.h 
  - Patch 6: Modify amd_iommu_domain_alloc() to fail when allocating identity
             domain and SNP is enabled.

Best Regards,
Suravee

Brijesh Singh (1):
  iommu/amd: Introduce function to check and enable SNP

Suravee Suthikulpanit (6):
  iommu/amd: Warn when found inconsistency EFR mask
  iommu/amd: Process all IVHDs before enabling IOMMU features
  iommu/amd: Introduce an iommu variable for tracking SNP support status
  iommu/amd: Set translation valid bit only when IO page tables are in
    use
  iommu/amd: Do not support IOMMU_DOMAIN_IDENTITY after SNP is enabled
  iommu/amd: Do not support IOMMUv2 APIs when SNP is enabled

 drivers/iommu/amd/amd_iommu_types.h |   5 ++
 drivers/iommu/amd/init.c            | 110 +++++++++++++++++++++++-----
 drivers/iommu/amd/iommu.c           |  28 ++++++-
 include/linux/amd-iommu.h           |   6 ++
 4 files changed, 127 insertions(+), 22 deletions(-)

-- 
2.32.0

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

[PATCH v2 1/7] iommu/amd: Warn when found inconsistency EFR mask

[Suravee Suthikulpanit via iommu]

The function check_feature_on_all_iommus() checks to ensure if an IOMMU
feature support bit is set on the Extended Feature Register (EFR).
Current logic iterates through all IOMMU, and returns false when it
found the first unset bit.

To provide more thorough checking, modify the logic to iterate through all
IOMMUs even when found that the bit is not set, and also throws a FW_BUG
warning if inconsistency is found.

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

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 3dd0f26039c7..b3e4551ce9dd 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -261,18 +261,29 @@ int amd_iommu_get_num_iommus(void)
 }
 
 #ifdef CONFIG_IRQ_REMAP
+/*
+ * Iterate through all the IOMMUs to verify if the specified
+ * EFR bitmask of IOMMU feature are set.
+ * Warn and return false if found inconsistency.
+ */
 static bool check_feature_on_all_iommus(u64 mask)
 {
 	bool ret = false;
 	struct amd_iommu *iommu;
 
 	for_each_iommu(iommu) {
-		ret = iommu_feature(iommu, mask);
-		if (!ret)
+		bool tmp = iommu_feature(iommu, mask);
+
+		if ((ret != tmp) &&
+		    !list_is_first(&iommu->list, &amd_iommu_list)) {
+			pr_err(FW_BUG "Found inconsistent EFR mask (%#llx) on iommu%d (%04x:%02x:%02x.%01x).\n",
+			       mask, iommu->index, iommu->pci_seg->id, PCI_BUS_NUM(iommu->devid),
+			       PCI_SLOT(iommu->devid), PCI_FUNC(iommu->devid));
 			return false;
+		}
+		ret = tmp;
 	}
-
-	return true;
+	return ret;
 }
 #endif
 
-- 
2.32.0

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

[PATCH v2 2/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 3 phases:

Phase1 : Processes information of the IVRS table for all IOMMU instances.
This allow all IVHDs to be processed prior to enabling features.

Phase2 : Early feature support check on all IOMMUs (using information in
IVHD blocks.

Phase3 : Iterates through all IOMMU instances and enabling features.

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

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index b3e4551ce9dd..5f86e357dbaa 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1692,7 +1692,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)
@@ -1773,6 +1772,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;
 
@@ -1794,10 +1800,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)
@@ -1808,7 +1810,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;
 }
@@ -1853,6 +1855,7 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	end += table->length;
 	p += IVRS_HEADER_LENGTH;
 
+	/* Phase 1: Process all IVHD blocks */
 	while (p < end) {
 		h = (struct ivhd_header *)p;
 		if (*p == amd_iommu_target_ivhd_type) {
@@ -1878,6 +1881,15 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	}
 	WARN_ON(p != end);
 
+	/* Phase 2 : Early feature support check */
+
+	/* Phase 3 : Enabling IOMMU features */
+	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 v2 3/7] iommu/amd: Introduce an iommu variable for tracking SNP support status

[Suravee Suthikulpanit via iommu]

EFR[SNPSup] needs to be checked early in the boot process, since it is
used to determine how IOMMU driver configures other IOMMU features
and data structures. This check can be done as soon as the IOMMU driver
finishes parsing IVHDs.

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

Also report IOMMU SNP support information for each IOMMU.

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

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 5f86e357dbaa..013c55e3c2f2 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -166,6 +166,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 */
@@ -260,7 +262,6 @@ int amd_iommu_get_num_iommus(void)
 	return amd_iommus_present;
 }
 
-#ifdef CONFIG_IRQ_REMAP
 /*
  * Iterate through all the IOMMUs to verify if the specified
  * EFR bitmask of IOMMU feature are set.
@@ -285,7 +286,6 @@ static bool check_feature_on_all_iommus(u64 mask)
 	}
 	return ret;
 }
-#endif
 
 /*
  * For IVHD type 0x11/0x40, EFR is also available via IVHD.
@@ -368,7 +368,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:
@@ -783,7 +783,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;
@@ -1882,6 +1882,7 @@ static int __init init_iommu_all(struct acpi_table_header *table)
 	WARN_ON(p != end);
 
 	/* Phase 2 : Early feature support check */
+	amd_iommu_snp_sup = check_feature_on_all_iommus(FEATURE_SNP);
 
 	/* Phase 3 : Enabling IOMMU features */
 	for_each_iommu(iommu) {
@@ -2118,6 +2119,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 v2 4/7] iommu/amd: Introduce function to check and enable SNP

[Suravee Suthikulpanit via iommu]

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

To support SNP, IOMMU needs to be enabled, and prohibits IOMMU
configurations where DTE[Mode]=0, which means it cannot be supported with
IOMMU passthrough domain (a.k.a IOMMU_DOMAIN_IDENTITY),
and when AMD IOMMU driver is configured to not use the IOMMU host (v1) page
table. Otherwise, RMP table initialization could cause the system to crash.

The request to enable SNP support in IOMMU must be done before PCI
initialization state of the IOMMU driver because enabling SNP affects
how IOMMU driver sets up IOMMU data structures (i.e. DTE).

Unlike other IOMMU features, SNP feature does not have an enable bit in
the IOMMU control register. Instead, the IOMMU driver introduces
an amd_iommu_snp_en variable to track enabling state of SNP.

Introduce amd_iommu_snp_enable() for other drivers to request enabling
the SNP support in IOMMU, which checks all prerequisites and determines
if the feature can be safely enabled.

Please see the IOMMU spec section 2.12 for further details.

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 |  5 ++++
 drivers/iommu/amd/init.c            | 45 +++++++++++++++++++++++++++--
 drivers/iommu/amd/iommu.c           |  4 +--
 include/linux/amd-iommu.h           |  6 ++++
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 73b729be7410..ce4db2835b36 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -463,6 +463,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) | \
@@ -1013,4 +1016,6 @@ extern struct amd_irte_ops irte_32_ops;
 extern struct amd_irte_ops irte_128_ops;
 #endif
 
+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 013c55e3c2f2..b5d3de327a5f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -95,8 +95,6 @@
  * 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.
@@ -168,6 +166,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 */
@@ -3549,3 +3550,43 @@ 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);
 }
+
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+int amd_iommu_snp_enable(void)
+{
+	/*
+	 * The SNP support requires that IOMMU must be enabled, and is
+	 * not configured in the passthrough mode.
+	 */
+	if (no_iommu || iommu_default_passthrough()) {
+		pr_err("SNP: IOMMU is either disabled or configured in passthrough mode.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Prevent enabling SNP after IOMMU_ENABLED state because this process
+	 * affect how IOMMU driver sets up data structures and configures
+	 * IOMMU hardware.
+	 */
+	if (init_state > IOMMU_ENABLED) {
+		pr_err("SNP: Too late to enable SNP for IOMMU.\n");
+		return -EINVAL;
+	}
+
+	amd_iommu_snp_en = amd_iommu_snp_sup;
+	if (!amd_iommu_snp_en)
+		return -EINVAL;
+
+	pr_info("SNP enabled\n");
+
+	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
+	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
+		pr_warn("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 0;
+}
+EXPORT_SYMBOL_GPL(amd_iommu_snp_enable);
+#endif
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 86045dc50a0f..0792cd618dba 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -71,7 +71,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;
@@ -2412,7 +2412,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/amd-iommu.h b/include/linux/amd-iommu.h
index 58e6c3806c09..d7bee7f09f92 100644
--- a/include/linux/amd-iommu.h
+++ b/include/linux/amd-iommu.h
@@ -206,4 +206,10 @@ int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn,
 		u64 *value);
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 
+#ifdef CONFIG_AMD_MEM_ENCRYPT
+int amd_iommu_snp_enable(void);
+#else
+static inline int amd_iommu_snp_enable(void) { return -ENODEV; }
+#endif
+
 #endif /* _ASM_X86_AMD_IOMMU_H */
-- 
2.32.0

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

Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP

[Suthikulpanit, Suravee via iommu]

Recap discussion on the other thread.

https://lore.kernel.org/linux-mm/CAMkAt6qorwbAXaPaCaSm0SC9o2uQ9ZQzB6s1kBkvAv2D4tkUug@mail.gmail.com/#t

On 6/16/2022 8:55 AM, Suravee Suthikulpanit wrote:
> +int amd_iommu_snp_enable(void)
> +{
> +	/*
> +	 * The SNP support requires that IOMMU must be enabled, and is
> +	 * not configured in the passthrough mode.
> +	 */
> +	if (no_iommu || iommu_default_passthrough()) {
> +		pr_err("SNP: IOMMU is either disabled or configured in passthrough mode.\n");
> +		return -EINVAL;
> +	}

Peter has suggested rewording to something more descriptive such as:

"SNP: IOMMU is either disabled or configured in passthrough mode, SNP cannot be supported".

Thank you,
Suravee

> +	/*
> +	 * Prevent enabling SNP after IOMMU_ENABLED state because this process
> +	 * affect how IOMMU driver sets up data structures and configures
> +	 * IOMMU hardware.
> +	 */
> +	if (init_state > IOMMU_ENABLED) {
> +		pr_err("SNP: Too late to enable SNP for IOMMU.\n");
> +		return -EINVAL;
> +	}
> +
> +	amd_iommu_snp_en = amd_iommu_snp_sup;
> +	if (!amd_iommu_snp_en)
> +		return -EINVAL;
> +
> +	pr_info("SNP enabled\n");
> +
> +	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
> +	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
> +		pr_warn("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 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_iommu_snp_enable);

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

Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP

[Robin Murphy]

On 2022-06-16 02:55, Suravee Suthikulpanit wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> To support SNP, IOMMU needs to be enabled, and prohibits IOMMU
> configurations where DTE[Mode]=0, which means it cannot be supported with
> IOMMU passthrough domain (a.k.a IOMMU_DOMAIN_IDENTITY),
> and when AMD IOMMU driver is configured to not use the IOMMU host (v1) page
> table. Otherwise, RMP table initialization could cause the system to crash.
> 
> The request to enable SNP support in IOMMU must be done before PCI
> initialization state of the IOMMU driver because enabling SNP affects
> how IOMMU driver sets up IOMMU data structures (i.e. DTE).
> 
> Unlike other IOMMU features, SNP feature does not have an enable bit in
> the IOMMU control register. Instead, the IOMMU driver introduces
> an amd_iommu_snp_en variable to track enabling state of SNP.
> 
> Introduce amd_iommu_snp_enable() for other drivers to request enabling
> the SNP support in IOMMU, which checks all prerequisites and determines
> if the feature can be safely enabled.
> 
> Please see the IOMMU spec section 2.12 for further details.
> 
> 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 |  5 ++++
>   drivers/iommu/amd/init.c            | 45 +++++++++++++++++++++++++++--
>   drivers/iommu/amd/iommu.c           |  4 +--
>   include/linux/amd-iommu.h           |  6 ++++
>   4 files changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
> index 73b729be7410..ce4db2835b36 100644
> --- a/drivers/iommu/amd/amd_iommu_types.h
> +++ b/drivers/iommu/amd/amd_iommu_types.h
> @@ -463,6 +463,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) | \
> @@ -1013,4 +1016,6 @@ extern struct amd_irte_ops irte_32_ops;
>   extern struct amd_irte_ops irte_128_ops;
>   #endif
>   
> +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 013c55e3c2f2..b5d3de327a5f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -95,8 +95,6 @@
>    * 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.
> @@ -168,6 +166,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 */
> @@ -3549,3 +3550,43 @@ 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);
>   }
> +
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +int amd_iommu_snp_enable(void)
> +{
> +	/*
> +	 * The SNP support requires that IOMMU must be enabled, and is
> +	 * not configured in the passthrough mode.
> +	 */
> +	if (no_iommu || iommu_default_passthrough()) {
> +		pr_err("SNP: IOMMU is either disabled or configured in passthrough mode.\n");

I agree that clarifying what the actual implication is would be a good idea.

> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Prevent enabling SNP after IOMMU_ENABLED state because this process
> +	 * affect how IOMMU driver sets up data structures and configures
> +	 * IOMMU hardware.
> +	 */
> +	if (init_state > IOMMU_ENABLED) {
> +		pr_err("SNP: Too late to enable SNP for IOMMU.\n");
> +		return -EINVAL;
> +	}
> +
> +	amd_iommu_snp_en = amd_iommu_snp_sup;
> +	if (!amd_iommu_snp_en)
> +		return -EINVAL;
> +
> +	pr_info("SNP enabled\n");
> +
> +	/* Enforce IOMMU v1 pagetable when SNP is enabled. */
> +	if (amd_iommu_pgtable != AMD_IOMMU_V1) {
> +		pr_warn("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 0;
> +}
> +EXPORT_SYMBOL_GPL(amd_iommu_snp_enable);

Once again this export seems dubious - surely the IOMMU is going to be 
enabled well before there's even a chance to load any modules?

> +#endif
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 86045dc50a0f..0792cd618dba 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -71,7 +71,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;
> @@ -2412,7 +2412,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/amd-iommu.h b/include/linux/amd-iommu.h
> index 58e6c3806c09..d7bee7f09f92 100644
> --- a/include/linux/amd-iommu.h
> +++ b/include/linux/amd-iommu.h
> @@ -206,4 +206,10 @@ int amd_iommu_pc_get_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn,
>   		u64 *value);
>   struct amd_iommu *get_amd_iommu(unsigned int idx);
>   
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +int amd_iommu_snp_enable(void);
> +#else
> +static inline int amd_iommu_snp_enable(void) { return -ENODEV; }
> +#endif

If this is only supposed to be called by other SEV code, it probably 
shouldn't need a stub.

Overall though, this is way nicer than v1, and it's definitely the right 
name in the right place now, thanks! FWIW, with those nits picked one 
way or another:

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Cheers,
Robin.

> +
>   #endif /* _ASM_X86_AMD_IOMMU_H */
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP

[Suthikulpanit, Suravee via iommu]

On 6/22/2022 3:35 PM, Robin Murphy wrote:
> 
> Overall though, this is way nicer than v1, and it's definitely the right name in the right place now, thanks! FWIW, with those nits picked one way or another:
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
> Cheers,
> Robin.

Thanks for your review. I'll send out v3 w/ your suggestions and reviewed-by tag.

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

[PATCH v2 5/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 b5d3de327a5f..bc008a82c12c 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2544,7 +2544,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 0792cd618dba..4f4571d3ff61 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1563,7 +1563,14 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 	    (domain->flags & PD_GIOV_MASK))
 		pte_root |= DTE_FLAG_GIOV;
 
-	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];
 
@@ -1625,7 +1632,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 v2 6/7] iommu/amd: Do not support IOMMU_DOMAIN_IDENTITY after SNP is enabled

[Suravee Suthikulpanit via iommu]

Once SNP is enabled (by executing SNP_INIT command), IOMMU can no longer
support the passthrough domain (i.e. IOMMU_DOMAIN_IDENTITY).

The SNP_INIT command is called early in the boot process, and would fail
if the kernel is configure to default to passthrough mode.

After the system is already booted, users can try to change IOMMU domain
type of a particular IOMMU group. In this case, the IOMMU driver needs to
check the SNP-enable status and return failure when requesting to change
domain type to identity.

Therefore, return failure when trying to allocate identity domain.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 4f4571d3ff61..d8a6df423b90 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2119,6 +2119,15 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
 {
 	struct protection_domain *domain;
 
+	/*
+	 * 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)) {
+		pr_warn("Cannot allocate identity domain due to SNP\n");
+		return NULL;
+	}
+
 	domain = protection_domain_alloc(type);
 	if (!domain)
 		return NULL;
-- 
2.32.0

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

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

[Robin Murphy]

On 2022-06-16 02:55, Suravee Suthikulpanit wrote:
> Once SNP is enabled (by executing SNP_INIT command), IOMMU can no longer
> support the passthrough domain (i.e. IOMMU_DOMAIN_IDENTITY).
> 
> The SNP_INIT command is called early in the boot process, and would fail
> if the kernel is configure to default to passthrough mode.
> 
> After the system is already booted, users can try to change IOMMU domain
> type of a particular IOMMU group. In this case, the IOMMU driver needs to
> check the SNP-enable status and return failure when requesting to change
> domain type to identity.
> 
> Therefore, return failure when trying to allocate identity domain.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/amd/iommu.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 4f4571d3ff61..d8a6df423b90 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2119,6 +2119,15 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned type)
>   {
>   	struct protection_domain *domain;
>   
> +	/*
> +	 * 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)) {
> +		pr_warn("Cannot allocate identity domain due to SNP\n");

Maybe pr_warn_once? Although on the other hand, perhaps anyone with the 
privilege to be messing with the sysfs interface at all could be trusted 
not to flood their own logs :/

Either way,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> +		return NULL;
> +	}
> +
>   	domain = protection_domain_alloc(type);
>   	if (!domain)
>   		return NULL;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 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 bc008a82c12c..780d6977a331 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3448,7 +3448,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