Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP
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 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
Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP
On 2022-06-16 02:55, Suravee Suthikulpanit wrote:
From: Brijesh Singh
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
Signed-off-by: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
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) & 0x)
#define PCI_SBDF_TO_DEVID(sbdf) ((sbdf) & 0x)
#define PCI_SEG_DEVID_TO_SBDF(seg, devid) u32)(seg) & 0x) << 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
Re: [PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP
Recap discussion on the other thread.
https://lore.kernel.org/linux-mm/camkat6qorwbaxapacasm0sc9o2uq9zqzb6s1kbkvav2d4tk...@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
[PATCH v2 4/7] iommu/amd: Introduce function to check and enable SNP
From: Brijesh Singh
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
Signed-off-by: Suravee Suthikulpanit
Signed-off-by: Brijesh Singh
---
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) & 0x)
#define PCI_SBDF_TO_DEVID(sbdf)((sbdf) & 0x)
#define PCI_SEG_DEVID_TO_SBDF(seg, devid) u32)(seg) & 0x) << 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)
