[PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[Vasant Hegde via iommu]

This series introduces a new usage model for the v2 page table, where it
can be used to implement support for DMA-API by adopting the generic
IO page table framework.

One of the target usecases is to support nested IO page tables
where the guest uses the guest IO page table (v2) for translating
GVA to GPA, and the hypervisor uses the host I/O page table (v1) for
translating GPA to SPA. This is a pre-requisite for supporting the new
HW-assisted vIOMMU presented at the KVM Forum 2020.

  https://static.sched.com/hosted_files/kvmforum2020/26/vIOMMU%20KVM%20Forum%202020.pdf

The following components are introduced in this series:

- Part 1 (patch 1-4 and 6)
  Refactor the current IOMMU page table code to adopt the generic IO page
  table framework, and add AMD IOMMU Guest (v2) page table management code.

- Part 2 (patch 5)
  Add support for the AMD IOMMU Guest IO Protection feature (GIOV)
  where requests from the I/O device without a PASID are treated as
  if they have PASID of 0.

- Part 3 (patch 7)
  Introduce new "amd_iommu_pgtable" command-line to allow users
  to select the mode of operation (v1 or v2).

See AMD I/O Virtualization Technology Specification for more detail.

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

Note:
  This patchset is based on top of "iommu/amd: Add multiple PCI segments support" patchset [1].

[1] https://lore.kernel.org/linux-iommu/20220511072141.15485-1-vasant.hegde@amd.com/T/#t

Thanks,
Vasant


Changes from RFC -> v1:
  - Addressed review comments from Joerg
  - Reimplemented v2 page table

RFC patchset : https://lore.kernel.org/linux-iommu/20210312090411.6030-1-suravee.suthikulpanit@amd.com/T/#t

Suravee Suthikulpanit (5):
  iommu/amd: Refactor amd_iommu_domain_enable_v2
  iommu/amd: Update sanity check when enable PRI/ATS
  iommu/amd: Add support for Guest IO protection
  iommu/amd: Add support for using AMD IOMMU v2 page table for DMA-API
  iommu/amd: Introduce amd_iommu_pgtable command-line option

Vasant Hegde (2):
  iommu/amd: Fix sparse warning
  iommu/amd: Initial support for AMD IOMMU v2 page table

 .../admin-guide/kernel-parameters.txt         |   6 +
 drivers/iommu/amd/Makefile                    |   2 +-
 drivers/iommu/amd/amd_iommu_types.h           |   8 +-
 drivers/iommu/amd/init.c                      |  27 +-
 drivers/iommu/amd/io_pgtable_v2.c             | 407 ++++++++++++++++++
 drivers/iommu/amd/iommu.c                     |  90 ++--
 drivers/iommu/io-pgtable.c                    |   1 +
 include/linux/io-pgtable.h                    |   2 +
 8 files changed, 516 insertions(+), 27 deletions(-)
 create mode 100644 drivers/iommu/amd/io_pgtable_v2.c

-- 
2.27.0

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

[PATCH v1 1/7] iommu/amd: Refactor amd_iommu_domain_enable_v2

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The current function to enable IOMMU v2 also lock the domain.
In order to reuse the same code in different code path, in which
the domain has already been locked, refactor the function to separate
the locking from the enabling logic.

Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/iommu.c | 46 +++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index c95c09c56b37..059e699c43d1 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -85,6 +85,7 @@ struct iommu_cmd {
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
+static int domain_enable_v2(struct protection_domain *domain, int pasids, bool has_ppr);
 
 /****************************************************************************
  *
@@ -2427,11 +2428,10 @@ void amd_iommu_domain_direct_map(struct iommu_domain *dom)
 }
 EXPORT_SYMBOL(amd_iommu_domain_direct_map);
 
-int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
+/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
+static int domain_enable_v2(struct protection_domain *domain, int pasids, bool has_ppr)
 {
-	struct protection_domain *domain = to_pdomain(dom);
-	unsigned long flags;
-	int levels, ret;
+	int levels;
 
 	/* Number of GCR3 table levels required */
 	for (levels = 0; (pasids - 1) & ~0x1ff; pasids >>= 9)
@@ -2440,7 +2440,25 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
 	if (levels > amd_iommu_max_glx_val)
 		return -EINVAL;
 
-	spin_lock_irqsave(&domain->lock, flags);
+	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
+	if (domain->gcr3_tbl == NULL)
+		return -ENOMEM;
+
+	domain->glx      = levels;
+	domain->flags   |= PD_IOMMUV2_MASK;
+
+	amd_iommu_domain_update(domain);
+
+	return 0;
+}
+
+int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
+{
+	struct protection_domain *pdom = to_pdomain(dom);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&pdom->lock, flags);
 
 	/*
 	 * Save us all sanity checks whether devices already in the
@@ -2448,24 +2466,14 @@ int amd_iommu_domain_enable_v2(struct iommu_domain *dom, int pasids)
 	 * devices attached when it is switched into IOMMUv2 mode.
 	 */
 	ret = -EBUSY;
-	if (domain->dev_cnt > 0 || domain->flags & PD_IOMMUV2_MASK)
-		goto out;
-
-	ret = -ENOMEM;
-	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);
-	if (domain->gcr3_tbl == NULL)
+	if (pdom->dev_cnt > 0 || pdom->flags & PD_IOMMUV2_MASK)
 		goto out;
 
-	domain->glx      = levels;
-	domain->flags   |= PD_IOMMUV2_MASK;
-
-	amd_iommu_domain_update(domain);
-
-	ret = 0;
+	if (!pdom->gcr3_tbl)
+		ret = domain_enable_v2(pdom, pasids, true);
 
 out:
-	spin_unlock_irqrestore(&domain->lock, flags);
-
+	spin_unlock_irqrestore(&pdom->lock, flags);
 	return ret;
 }
 EXPORT_SYMBOL(amd_iommu_domain_enable_v2);
-- 
2.27.0

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

[PATCH v1 2/7] iommu/amd: Update sanity check when enable PRI/ATS

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Currently, PPR/ATS can be enabled only if the domain is type
identity mapping. However, when we allow the IOMMU v2 page table
to be used for DMA-API, the sanity check needs to be updated to
only apply for the case when using AMD_IOMMU_V1 page table mode.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 059e699c43d1..b558e8c30613 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1682,7 +1682,7 @@ static void pdev_iommuv2_disable(struct pci_dev *pdev)
 	pci_disable_pasid(pdev);
 }
 
-static int pdev_iommuv2_enable(struct pci_dev *pdev)
+static int pdev_pri_ats_enable(struct pci_dev *pdev)
 {
 	int ret;
 
@@ -1745,11 +1745,19 @@ static int attach_device(struct device *dev,
 		struct iommu_domain *def_domain = iommu_get_dma_domain(dev);
 
 		ret = -EINVAL;
-		if (def_domain->type != IOMMU_DOMAIN_IDENTITY)
+
+		/*
+		 * In case of using AMD_IOMMU_V1 page table mode and the device
+		 * is enabling for PPR/ATS support (using v2 table),
+		 * we need to make sure that the domain type is identity map.
+		 */
+		if ((amd_iommu_pgtable == AMD_IOMMU_V1) &&
+		    def_domain->type != IOMMU_DOMAIN_IDENTITY) {
 			goto out;
+		}
 
 		if (dev_data->iommu_v2) {
-			if (pdev_iommuv2_enable(pdev) != 0)
+			if (pdev_pri_ats_enable(pdev) != 0)
 				goto out;
 
 			dev_data->ats.enabled = true;
-- 
2.27.0

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

[PATCH v1 3/7] iommu/amd: Fix sparse warning

[Vasant Hegde via iommu]

Fix below sparse warning:
  CHECK   drivers/iommu/amd/iommu.c
  drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?

Also we are going to introduce v2 page table which has different
pgsize_bitmaps. Hence remove 'const' qualifier.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c  | 2 +-
 drivers/iommu/amd/iommu.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 8483d98a1775..453afce7d478 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -96,7 +96,7 @@
  * out of it.
  */
 
-extern const struct iommu_ops amd_iommu_ops;
+extern struct iommu_ops amd_iommu_ops;
 
 /*
  * structure describing one IOMMU in the ACPI table. Typically followed by one
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b558e8c30613..deb546266d42 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;
@@ -2374,7 +2374,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,
-- 
2.27.0

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

[PATCH v1 4/7] iommu/amd: Initial support for AMD IOMMU v2 page table

[Vasant Hegde via iommu]

Introduce IO page table framework support for AMD IOMMU v2 page table.
This patch implements 4 level page table within iommu amd driver and
supports 4K/2M/1G page sizes.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/Makefile          |   2 +-
 drivers/iommu/amd/amd_iommu_types.h |   5 +-
 drivers/iommu/amd/io_pgtable_v2.c   | 407 ++++++++++++++++++++++++++++
 drivers/iommu/io-pgtable.c          |   1 +
 include/linux/io-pgtable.h          |   2 +
 5 files changed, 415 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/amd/io_pgtable_v2.c

diff --git a/drivers/iommu/amd/Makefile b/drivers/iommu/amd/Makefile
index a935f8f4b974..773d8aa00283 100644
--- a/drivers/iommu/amd/Makefile
+++ b/drivers/iommu/amd/Makefile
@@ -1,4 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o
+obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o io_pgtable_v2.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += debugfs.o
 obj-$(CONFIG_AMD_IOMMU_V2) += iommu_v2.o
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 328572cf6fa5..4062313a2407 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -268,6 +268,8 @@
  * 512GB Pages are not supported due to a hardware bug
  */
 #define AMD_IOMMU_PGSIZES	((~0xFFFUL) & ~(2ULL << 38))
+/* 4K, 2MB, 1G page sizes are supported */
+#define AMD_IOMMU_PGSIZES_V2	(PAGE_SIZE | (1ULL << 21) | (1ULL << 30))
 
 /* Bit value definition for dte irq remapping fields*/
 #define DTE_IRQ_PHYS_ADDR_MASK	(((1ULL << 45)-1) << 6)
@@ -514,7 +516,8 @@ struct amd_io_pgtable {
 	struct io_pgtable	iop;
 	int			mode;
 	u64			*root;
-	atomic64_t		pt_root;    /* pgtable root and pgtable mode */
+	atomic64_t		pt_root;	/* pgtable root and pgtable mode */
+	u64			*pgd;		/* v2 pgtable pgd pointer */
 };
 
 /*
diff --git a/drivers/iommu/amd/io_pgtable_v2.c b/drivers/iommu/amd/io_pgtable_v2.c
new file mode 100644
index 000000000000..4248a182a780
--- /dev/null
+++ b/drivers/iommu/amd/io_pgtable_v2.c
@@ -0,0 +1,407 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * CPU-agnostic AMD IO page table v2 allocator.
+ *
+ * Copyright (C) 2022 Advanced Micro Devices, Inc.
+ * Author: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
+ * Author: Vasant Hegde <vasant.hegde@amd.com>
+ */
+
+#define pr_fmt(fmt)	"AMD-Vi: " fmt
+#define dev_fmt(fmt)	pr_fmt(fmt)
+
+#include <linux/bitops.h>
+#include <linux/io-pgtable.h>
+#include <linux/kernel.h>
+
+#include <asm/barrier.h>
+
+#include "amd_iommu_types.h"
+#include "amd_iommu.h"
+
+#define IOMMU_PAGE_PRESENT	BIT_ULL(0)	/* Is present */
+#define IOMMU_PAGE_RW		BIT_ULL(1)	/* Writeable */
+#define IOMMU_PAGE_USER		BIT_ULL(2)	/* Userspace addressable */
+#define IOMMU_PAGE_PWT		BIT_ULL(3)	/* Page write through */
+#define IOMMU_PAGE_PCD		BIT_ULL(4)	/* Page cache disabled */
+#define IOMMU_PAGE_ACCESS	BIT_ULL(5)	/* Was accessed (updated by IOMMU) */
+#define IOMMU_PAGE_DIRTY	BIT_ULL(6)	/* Was written to (updated by IOMMU) */
+#define IOMMU_PAGE_PSE		BIT_ULL(7)	/* Page Size Extensions */
+#define IOMMU_PAGE_NX		BIT_ULL(63)	/* No execute */
+
+#define MAX_PTRS_PER_PAGE	512
+
+#define IOMMU_PAGE_SIZE_2M	BIT_ULL(21)
+#define IOMMU_PAGE_SIZE_1G	BIT_ULL(30)
+
+
+static inline int get_pgtable_level(void)
+{
+	/* 5 level page table is not supported */
+	return PAGE_MODE_4_LEVEL;
+}
+
+static inline bool is_large_pte(u64 pte)
+{
+	return (pte & IOMMU_PAGE_PSE);
+}
+
+static inline void *alloc_pgtable_page(void)
+{
+	return (void *)get_zeroed_page(GFP_KERNEL);
+}
+
+static inline u64 set_pgtable_attr(u64 *page)
+{
+	u64 prot;
+
+	prot = IOMMU_PAGE_PRESENT | IOMMU_PAGE_RW | IOMMU_PAGE_USER;
+	prot |= IOMMU_PAGE_ACCESS | IOMMU_PAGE_DIRTY;
+
+	return (iommu_virt_to_phys(page) | prot);
+}
+
+static inline void *get_pgtable_pte(u64 pte)
+{
+	return iommu_phys_to_virt(pte & PM_ADDR_MASK);
+}
+
+static u64 set_pte_attr(u64 paddr, u64 pg_size, int prot)
+{
+	u64 pte;
+
+	pte = __sme_set(paddr & PM_ADDR_MASK);
+	pte |= IOMMU_PAGE_PRESENT | IOMMU_PAGE_USER;
+	pte |= IOMMU_PAGE_ACCESS | IOMMU_PAGE_DIRTY;
+
+	if (prot & IOMMU_PROT_IW)
+		pte |= IOMMU_PAGE_RW;
+
+	/* Large page */
+	if (pg_size == IOMMU_PAGE_SIZE_1G || pg_size == IOMMU_PAGE_SIZE_2M)
+		pte |= IOMMU_PAGE_PSE;
+
+	return pte;
+}
+
+static inline u64 get_alloc_page_size(u64 size)
+{
+	if (size >= IOMMU_PAGE_SIZE_1G)
+		return IOMMU_PAGE_SIZE_1G;
+
+	if (size >= IOMMU_PAGE_SIZE_2M)
+		return IOMMU_PAGE_SIZE_2M;
+
+	return PAGE_SIZE;
+}
+
+static inline int page_size_to_level(u64 pg_size)
+{
+	if (pg_size == IOMMU_PAGE_SIZE_1G)
+		return PAGE_MODE_3_LEVEL;
+	if (pg_size == IOMMU_PAGE_SIZE_2M)
+		return PAGE_MODE_2_LEVEL;
+
+	return PAGE_MODE_1_LEVEL;
+}
+
+static inline void free_pgtable_page(u64 *pt)
+{
+	free_page((unsigned long)pt);
+}
+
+static void free_pgtable(u64 *pt, int level)
+{
+	u64 *p;
+	int i;
+
+	for (i = 0; i < MAX_PTRS_PER_PAGE; i++) {
+		/* PTE present? */
+		if (!IOMMU_PTE_PRESENT(pt[i]))
+			continue;
+
+		if (is_large_pte(pt[i]))
+			continue;
+
+		/*
+		 * Free the next level. No need to look at l1 tables here since
+		 * they can only contain leaf PTEs; just free them directly.
+		 */
+		p = get_pgtable_pte(pt[i]);
+		if (level > 2)
+			free_pgtable(p, level - 1);
+		else
+			free_pgtable_page(p);
+	}
+
+	free_pgtable_page(pt);
+}
+
+/* Allocate page table */
+static u64 *v2_alloc_pte(u64 *pgd, unsigned long iova,
+			 unsigned long pg_size, bool *updated)
+{
+	u64 *pte, *page;
+	int level, end_level;
+
+	BUG_ON(!is_power_of_2(pg_size));
+
+	level = get_pgtable_level() - 1;
+	end_level = page_size_to_level(pg_size);
+	pte = &pgd[PM_LEVEL_INDEX(level, iova)];
+	iova = PAGE_SIZE_ALIGN(iova, PAGE_SIZE);
+
+	while (level >= end_level) {
+		u64 __pte, __npte;
+
+		__pte = *pte;
+
+		if (IOMMU_PTE_PRESENT(__pte) && is_large_pte(__pte)) {
+			/* Unmap large pte */
+			cmpxchg64(pte, *pte, 0ULL);
+			*updated = true;
+			continue;
+		}
+
+		if (!IOMMU_PTE_PRESENT(__pte)) {
+			page = alloc_pgtable_page();
+			if (!page)
+				return NULL;
+
+			__npte = set_pgtable_attr(page);
+			/* pte could have been changed somewhere. */
+			if (cmpxchg64(pte, __pte, __npte) != __pte)
+				free_pgtable_page(page);
+			else if (IOMMU_PTE_PRESENT(__pte))
+				*updated = true;
+
+			continue;
+		}
+
+		level -= 1;
+		pte = get_pgtable_pte(__pte);
+		pte = &pte[PM_LEVEL_INDEX(level, iova)];
+	}
+
+	/* Tear down existing pte entries */
+	if (IOMMU_PTE_PRESENT(*pte)) {
+		u64 *__pte;
+
+		*updated = true;
+		__pte = get_pgtable_pte(*pte);
+		cmpxchg64(pte, *pte, 0ULL);
+		if (pg_size == IOMMU_PAGE_SIZE_1G)
+			free_pgtable(__pte, end_level - 1);
+		else if (pg_size == IOMMU_PAGE_SIZE_2M)
+			free_pgtable_page(__pte);
+	}
+
+	return pte;
+}
+
+/*
+ * This function checks if there is a PTE for a given dma address.
+ * If there is one, it returns the pointer to it.
+ */
+static u64 *fetch_pte(struct amd_io_pgtable *pgtable,
+		      unsigned long iova, unsigned long *page_size)
+{
+	u64 *pte;
+	int level;
+
+	level = get_pgtable_level() - 1;
+	pte = &pgtable->pgd[PM_LEVEL_INDEX(level, iova)];
+	/* Default page size is 4K */
+	*page_size = PAGE_SIZE;
+
+	while (level) {
+		/* Not present */
+		if (!IOMMU_PTE_PRESENT(*pte))
+			return NULL;
+
+		/* Walk to the next level */
+		pte = get_pgtable_pte(*pte);
+		pte = &pte[PM_LEVEL_INDEX(level - 1, iova)];
+
+		/* Large page */
+		if (is_large_pte(*pte)) {
+			if (level == PAGE_MODE_3_LEVEL)
+				*page_size = IOMMU_PAGE_SIZE_1G;
+			else if (level == PAGE_MODE_2_LEVEL)
+				*page_size = IOMMU_PAGE_SIZE_2M;
+			else
+				return NULL;	/* Wrongly set PSE bit in PTE */
+
+			break;
+		}
+
+		level -= 1;
+	}
+
+	return pte;
+}
+
+static int iommu_v2_map_page(struct io_pgtable_ops *ops, unsigned long iova,
+			     phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+{
+	struct protection_domain *pdom = io_pgtable_ops_to_domain(ops);
+	u64 *pte;
+	unsigned long map_size;
+	unsigned long mapped_size = 0;
+	unsigned long o_iova = iova;
+	int count = 0;
+	int ret = 0;
+	bool updated = false;
+
+	BUG_ON(!IS_ALIGNED(iova, size) || !IS_ALIGNED(paddr, size));
+
+	if (!(prot & IOMMU_PROT_MASK))
+		return -EINVAL;
+
+	while (mapped_size < size) {
+		map_size = get_alloc_page_size(size - mapped_size);
+		pte = v2_alloc_pte(pdom->iop.pgd, iova, map_size, &updated);
+		if (!pte) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		*pte = set_pte_attr(paddr, map_size, prot);
+
+		count++;
+		iova += map_size;
+		paddr += map_size;
+		mapped_size += map_size;
+	}
+
+out:
+	if (updated) {
+		if (count > 1)
+			amd_iommu_flush_tlb(&pdom->domain, 0);
+		else
+			amd_iommu_flush_page(&pdom->domain, 0, o_iova);
+	}
+
+	return ret;
+}
+
+static unsigned long iommu_v2_unmap_page(struct io_pgtable_ops *ops,
+					 unsigned long iova, size_t size,
+					 struct iommu_iotlb_gather *gather)
+{
+	struct amd_io_pgtable *pgtable = io_pgtable_ops_to_data(ops);
+	unsigned long unmap_size;
+	unsigned long unmapped = 0;
+	u64 *pte;
+
+	BUG_ON(!is_power_of_2(size));
+
+	while (unmapped < size) {
+		pte = fetch_pte(pgtable, iova, &unmap_size);
+		if (pte) {
+			*pte = 0ULL;
+		}
+
+		iova = (iova & ~(unmap_size - 1)) + unmap_size;
+		unmapped += unmap_size;
+	}
+
+	BUG_ON(unmapped && !is_power_of_2(unmapped));
+
+	return unmapped;
+}
+
+static phys_addr_t iommu_v2_iova_to_phys(struct io_pgtable_ops *ops, unsigned long iova)
+{
+	struct amd_io_pgtable *pgtable = io_pgtable_ops_to_data(ops);
+	unsigned long offset_mask, pte_pgsize;
+	u64 *pte, __pte;
+
+	pte = fetch_pte(pgtable, iova, &pte_pgsize);
+	if (!pte || !IOMMU_PTE_PRESENT(*pte))
+		return 0;
+
+	offset_mask = pte_pgsize - 1;
+	__pte = __sme_clr(*pte & PM_ADDR_MASK);
+
+	return (__pte & ~offset_mask) | (iova & offset_mask);
+}
+
+/*
+ * ----------------------------------------------------
+ */
+static void v2_tlb_flush_all(void *cookie)
+{
+}
+
+static void v2_tlb_flush_walk(unsigned long iova, size_t size,
+			      size_t granule, void *cookie)
+{
+}
+
+static void v2_tlb_add_page(struct iommu_iotlb_gather *gather,
+			    unsigned long iova, size_t granule,
+			    void *cookie)
+{
+}
+
+static const struct iommu_flush_ops v2_flush_ops = {
+	.tlb_flush_all	= v2_tlb_flush_all,
+	.tlb_flush_walk = v2_tlb_flush_walk,
+	.tlb_add_page	= v2_tlb_add_page,
+};
+
+static void v2_free_pgtable(struct io_pgtable *iop)
+{
+	struct protection_domain *pdom;
+	struct amd_io_pgtable *pgtable = container_of(iop, struct amd_io_pgtable, iop);
+
+	pdom = container_of(pgtable, struct protection_domain, iop);
+	if (!(pdom->flags & PD_IOMMUV2_MASK))
+		return;
+
+	/*
+	 * Make changes visible to IOMMUs. No need to clear gcr3 entry
+	 * as gcr3 table is already freed.
+	 */
+	amd_iommu_domain_update(pdom);
+
+	/* Free page table */
+	free_pgtable(pgtable->pgd, get_pgtable_level());
+}
+
+static struct io_pgtable *v2_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
+{
+	struct amd_io_pgtable *pgtable = io_pgtable_cfg_to_data(cfg);
+	struct protection_domain *pdom = (struct protection_domain *)cookie;
+	int ret;
+
+	pgtable->pgd = alloc_pgtable_page();
+	if (!pgtable->pgd)
+		return NULL;
+
+	ret = amd_iommu_domain_set_gcr3(&pdom->domain, 0, iommu_virt_to_phys(pgtable->pgd));
+	if (ret)
+		goto err_free_pgd;
+
+	pgtable->iop.ops.map          = iommu_v2_map_page;
+	pgtable->iop.ops.unmap        = iommu_v2_unmap_page;
+	pgtable->iop.ops.iova_to_phys = iommu_v2_iova_to_phys;
+
+	cfg->pgsize_bitmap = AMD_IOMMU_PGSIZES_V2,
+	cfg->ias           = IOMMU_IN_ADDR_BIT_SIZE,
+	cfg->oas           = IOMMU_OUT_ADDR_BIT_SIZE,
+	cfg->tlb           = &v2_flush_ops;
+
+	return &pgtable->iop;
+
+err_free_pgd:
+	free_pgtable_page(pgtable->pgd);
+
+	return NULL;
+}
+
+struct io_pgtable_init_fns io_pgtable_amd_iommu_v2_init_fns = {
+	.alloc	= v2_alloc_pgtable,
+	.free	= v2_free_pgtable,
+};
diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
index f4bfcef98297..ac02a45ed798 100644
--- a/drivers/iommu/io-pgtable.c
+++ b/drivers/iommu/io-pgtable.c
@@ -27,6 +27,7 @@ io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
 #endif
 #ifdef CONFIG_AMD_IOMMU
 	[AMD_IOMMU_V1] = &io_pgtable_amd_iommu_v1_init_fns,
+	[AMD_IOMMU_V2] = &io_pgtable_amd_iommu_v2_init_fns,
 #endif
 };
 
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 86af6f0a00a2..dc0f50bbdfa9 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -16,6 +16,7 @@ enum io_pgtable_fmt {
 	ARM_V7S,
 	ARM_MALI_LPAE,
 	AMD_IOMMU_V1,
+	AMD_IOMMU_V2,
 	APPLE_DART,
 	IO_PGTABLE_NUM_FMTS,
 };
@@ -255,6 +256,7 @@ extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_amd_iommu_v1_init_fns;
+extern struct io_pgtable_init_fns io_pgtable_amd_iommu_v2_init_fns;
 extern struct io_pgtable_init_fns io_pgtable_apple_dart_init_fns;
 
 #endif /* __IO_PGTABLE_H */
-- 
2.27.0

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

[PATCH v1 5/7] iommu/amd: Add support for Guest IO protection

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

AMD IOMMU introduces support for Guest I/O protection where the request
from the I/O device without a PASID are treated as if they have PASID 0.

Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 drivers/iommu/amd/amd_iommu_types.h | 3 +++
 drivers/iommu/amd/init.c            | 8 ++++++++
 drivers/iommu/amd/iommu.c           | 5 +++++
 3 files changed, 16 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 4062313a2407..a25c24188104 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -93,6 +93,7 @@
 #define FEATURE_HE		(1ULL<<8)
 #define FEATURE_PC		(1ULL<<9)
 #define FEATURE_GAM_VAPIC	(1ULL<<21)
+#define FEATURE_GIOSUP		(1ULL<<48)
 #define FEATURE_EPHSUP		(1ULL<<50)
 #define FEATURE_SNP		(1ULL<<63)
 
@@ -370,6 +371,7 @@
 #define DTE_FLAG_IW (1ULL << 62)
 
 #define DTE_FLAG_IOTLB	(1ULL << 32)
+#define DTE_FLAG_GIOV	(1ULL << 54)
 #define DTE_FLAG_GV	(1ULL << 55)
 #define DTE_FLAG_MASK	(0x3ffULL << 32)
 #define DTE_GLX_SHIFT	(56)
@@ -427,6 +429,7 @@
 #define PD_PASSTHROUGH_MASK	(1UL << 2) /* domain has no page
 					      translation */
 #define PD_IOMMUV2_MASK		(1UL << 3) /* domain has gcr3 table */
+#define PD_GIOV_MASK		(1UL << 4) /* domain enable GIOV support */
 
 extern bool amd_iommu_dump;
 #define DUMP_printk(format, arg...)				\
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 453afce7d478..d4d9c812305d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2024,6 +2024,12 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 
 	init_iommu_perf_ctr(iommu);
 
+	if (amd_iommu_pgtable == AMD_IOMMU_V2 &&
+	    !iommu_feature(iommu, FEATURE_GIOSUP)) {
+		pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
+		amd_iommu_pgtable = AMD_IOMMU_V1;
+	}
+
 	if (is_rd890_iommu(iommu->dev)) {
 		int i, j;
 
@@ -2098,6 +2104,8 @@ static void print_iommu_info(void)
 		if (amd_iommu_xt_mode == IRQ_REMAP_X2APIC_MODE)
 			pr_info("X2APIC enabled\n");
 	}
+	if (amd_iommu_pgtable == AMD_IOMMU_V2)
+		pr_info("V2 page table enabled\n");
 }
 
 static int __init amd_iommu_init_pci(void)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index deb546266d42..f2d939b7cc4d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1553,6 +1553,11 @@ static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 
 	pte_root |= (domain->iop.mode & DEV_ENTRY_MODE_MASK)
 		    << DEV_ENTRY_MODE_SHIFT;
+
+	if ((domain->flags & PD_IOMMUV2_MASK) &&
+	    (domain->flags & PD_GIOV_MASK))
+		pte_root |= DTE_FLAG_GIOV;
+
 	pte_root |= DTE_FLAG_IR | DTE_FLAG_IW | DTE_FLAG_V | DTE_FLAG_TV;
 
 	flags = dev_table[devid].data[1];
-- 
2.27.0

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

[PATCH v1 6/7] iommu/amd: Add support for using AMD IOMMU v2 page table for DMA-API

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Introduce init function for setting up DMA domain for DMA-API with
the IOMMU v2 page table.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index f2d939b7cc4d..e5aef845f01c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2029,6 +2029,24 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 	return 0;
 }
 
+static int protection_domain_init_v2(struct protection_domain *domain)
+{
+	spin_lock_init(&domain->lock);
+	domain->id = domain_id_alloc();
+	if (!domain->id)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&domain->dev_list);
+
+	domain->flags |= PD_GIOV_MASK;
+
+	if (domain_enable_v2(domain, 1, false)) {
+		domain_id_free(domain->id);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static struct protection_domain *protection_domain_alloc(unsigned int type)
 {
 	struct io_pgtable_ops *pgtbl_ops;
@@ -2056,6 +2074,9 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 	case AMD_IOMMU_V1:
 		ret = protection_domain_init_v1(domain, mode);
 		break;
+	case AMD_IOMMU_V2:
+		ret = protection_domain_init_v2(domain);
+		break;
 	default:
 		ret = -EINVAL;
 	}
-- 
2.27.0

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

[PATCH v1 7/7] iommu/amd: Introduce amd_iommu_pgtable command-line option

[Vasant Hegde via iommu]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

To allow specification whether to use v1 or v2 IOMMU pagetable for
DMA remapping when calling kernel DMA-API.

Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  6 ++++++
 drivers/iommu/amd/init.c                        | 17 +++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index cc8f0c82ff55..d912c4c8b610 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -337,6 +337,12 @@
 			             This mode requires kvm-amd.avic=1.
 			             (Default when IOMMU HW support is present.)
 
+	amd_iommu_pgtable= [HW,X86-64]
+			Specifies one of the following AMD IOMMU page table to
+			be used for DMA remapping for DMA-API:
+			v1         - Use v1 page table (Default)
+			v2         - Use v2 page table
+
 	amijoy.map=	[HW,JOY] Amiga joystick support
 			Map of devices attached to JOY0DAT and JOY1DAT
 			Format: <a>,<b>
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index d4d9c812305d..3fae018f62d8 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3264,6 +3264,22 @@ static int __init parse_amd_iommu_dump(char *str)
 	return 1;
 }
 
+static int __init parse_amd_iommu_pgtable(char *str)
+{
+	for (; *str; ++str) {
+		if (strncmp(str, "v1", 2) == 0) {
+			amd_iommu_pgtable = AMD_IOMMU_V1;
+			amd_iommu_ops.pgsize_bitmap = AMD_IOMMU_PGSIZES;
+			break;
+		} else if (strncmp(str, "v2", 2) == 0) {
+			amd_iommu_pgtable = AMD_IOMMU_V2;
+			amd_iommu_ops.pgsize_bitmap = AMD_IOMMU_PGSIZES_V2;
+			break;
+		}
+	}
+	return 1;
+}
+
 static int __init parse_amd_iommu_intr(char *str)
 {
 	for (; *str; ++str) {
@@ -3397,6 +3413,7 @@ static int __init parse_ivrs_acpihid(char *str)
 
 __setup("amd_iommu_dump",	parse_amd_iommu_dump);
 __setup("amd_iommu=",		parse_amd_iommu_options);
+__setup("amd_iommu_pgtable=",	parse_amd_iommu_pgtable);
 __setup("amd_iommu_intr=",	parse_amd_iommu_intr);
 __setup("ivrs_ioapic",		parse_ivrs_ioapic);
 __setup("ivrs_hpet",		parse_ivrs_hpet);
-- 
2.27.0

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

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Joerg Roedel]

On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
> Fix below sparse warning:
>   CHECK   drivers/iommu/amd/iommu.c
>   drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?
> 
> Also we are going to introduce v2 page table which has different
> pgsize_bitmaps. Hence remove 'const' qualifier.

I am not a fan of removing the consts. Please use separate ops
structures for v2 page-tables and make then const as well. This probably
also has some optimization potential in the future when we can make the
ops call-back functions page-table specific.

Regards,

	Joerg

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

Re: [PATCH v1 4/7] iommu/amd: Initial support for AMD IOMMU v2 page table

[Joerg Roedel]

On Fri, Jun 03, 2022 at 04:51:04PM +0530, Vasant Hegde wrote:
> +/* Allocate page table */
> +static u64 *v2_alloc_pte(u64 *pgd, unsigned long iova,
> +			 unsigned long pg_size, bool *updated)
> +{
> +	u64 *pte, *page;
> +	int level, end_level;
> +
> +	BUG_ON(!is_power_of_2(pg_size));
> +
> +	level = get_pgtable_level() - 1;
> +	end_level = page_size_to_level(pg_size);
> +	pte = &pgd[PM_LEVEL_INDEX(level, iova)];
> +	iova = PAGE_SIZE_ALIGN(iova, PAGE_SIZE);
> +
> +	while (level >= end_level) {
> +		u64 __pte, __npte;
> +
> +		__pte = *pte;
> +
> +		if (IOMMU_PTE_PRESENT(__pte) && is_large_pte(__pte)) {
> +			/* Unmap large pte */
> +			cmpxchg64(pte, *pte, 0ULL);
> +			*updated = true;
> +			continue;
> +		}
> +
> +		if (!IOMMU_PTE_PRESENT(__pte)) {
> +			page = alloc_pgtable_page();
> +			if (!page)
> +				return NULL;
> +
> +			__npte = set_pgtable_attr(page);
> +			/* pte could have been changed somewhere. */
> +			if (cmpxchg64(pte, __pte, __npte) != __pte)
> +				free_pgtable_page(page);
> +			else if (IOMMU_PTE_PRESENT(__pte))
> +				*updated = true;
> +
> +			continue;
> +		}
> +
> +		level -= 1;
> +		pte = get_pgtable_pte(__pte);
> +		pte = &pte[PM_LEVEL_INDEX(level, iova)];
> +	}

I know that the V1 page-table code also uses loops for the allocation
path, but the main reason there is the variable amount of page-table
levels. The v2 page-tables have a fixed amount levels, so it is better
to unroll this loop here (and other loops iterating over page-table
levels). This makes the code more clear.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 7/7] iommu/amd: Introduce amd_iommu_pgtable command-line option

[Joerg Roedel]

On Fri, Jun 03, 2022 at 04:51:07PM +0530, Vasant Hegde wrote:
> +	amd_iommu_pgtable= [HW,X86-64]
> +			Specifies one of the following AMD IOMMU page table to
> +			be used for DMA remapping for DMA-API:
> +			v1         - Use v1 page table (Default)
> +			v2         - Use v2 page table
> +

Can we handle this somehow in the amd_iommu= option? Something like
amd_iommu=pgtbl_v2|pgtbl_v1?

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

Re: [PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[Joerg Roedel]

On Fri, Jun 03, 2022 at 04:51:00PM +0530, Vasant Hegde wrote:
> - Part 1 (patch 1-4 and 6)
>   Refactor the current IOMMU page table code to adopt the generic IO page
>   table framework, and add AMD IOMMU Guest (v2) page table management code.
> 
> - Part 2 (patch 5)
>   Add support for the AMD IOMMU Guest IO Protection feature (GIOV)
>   where requests from the I/O device without a PASID are treated as
>   if they have PASID of 0.
> 
> - Part 3 (patch 7)
>   Introduce new "amd_iommu_pgtable" command-line to allow users
>   to select the mode of operation (v1 or v2).

Something I didn't get entirely from the review is support level of the
amd_iommu_v2 driver. I think it will continue to work and the
requirement that the device identity maps DMA requests without PASID is
removed, right?

Regards,

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

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Robin Murphy]

On 2022-06-23 09:03, Joerg Roedel wrote:
> On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
>> Fix below sparse warning:
>>    CHECK   drivers/iommu/amd/iommu.c
>>    drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?
>>
>> Also we are going to introduce v2 page table which has different
>> pgsize_bitmaps. Hence remove 'const' qualifier.
> 
> I am not a fan of removing the consts. Please use separate ops
> structures for v2 page-tables and make then const as well. This probably
> also has some optimization potential in the future when we can make the
> ops call-back functions page-table specific.

TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the 
very least it would be logical to move it to iommu_domain_ops now, but 
maybe we could skip ahead and just rely on drivers initialising 
domain->pgsize_bitmap directly in their .domain_alloc?

(and FWIW I'm leaning towards the same for the domain->ops as well; the 
more I look at ops->default_domain_ops, the more it starts looking like 
a stupid mess... my stupid mess... sorry about that)

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

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Joerg Roedel]

On Thu, Jun 23, 2022 at 10:42:52AM +0100, Robin Murphy wrote:
> TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the
> very least it would be logical to move it to iommu_domain_ops now, but maybe
> we could skip ahead and just rely on drivers initialising
> domain->pgsize_bitmap directly in their .domain_alloc?
> 
> (and FWIW I'm leaning towards the same for the domain->ops as well; the more
> I look at ops->default_domain_ops, the more it starts looking like a stupid
> mess... my stupid mess... sorry about that)

Good idea, that would be even better.

Thanks,

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

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Vasant Hegde via iommu]

On 6/23/2022 3:12 PM, Robin Murphy wrote:
> On 2022-06-23 09:03, Joerg Roedel wrote:
>> On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
>>> Fix below sparse warning:
>>>    CHECK   drivers/iommu/amd/iommu.c
>>>    drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?
>>>
>>> Also we are going to introduce v2 page table which has different
>>> pgsize_bitmaps. Hence remove 'const' qualifier.
>>
>> I am not a fan of removing the consts. Please use separate ops
>> structures for v2 page-tables and make then const as well. This probably
>> also has some optimization potential in the future when we can make the
>> ops call-back functions page-table specific.
> 
> TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the very least it would be logical to move it to iommu_domain_ops now, but maybe we could skip ahead and just rely on drivers initialising domain->pgsize_bitmap directly in their .domain_alloc?
> 

Robin,

Something like below? If yes, I will cleanup and get proper fix.


-Vasant


diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 840831d5d2ad..32dd84a7c1da 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1916,6 +1916,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 			return -ENOMEM;
 	}
 
+	domain->domain.pgsize_bitmap	= AMD_IOMMU_PGSIZES;
 	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
 
 	return 0;
@@ -2282,7 +2283,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.get_resv_regions = amd_iommu_get_resv_regions,
 	.put_resv_regions = generic_iommu_put_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
-	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 	.def_domain_type = amd_iommu_def_domain_type,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..73cfba6a6728 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1915,8 +1915,6 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 		return NULL;
 
 	domain->type = type;
-	/* Assume all sizes by default; the driver may override this later */
-	domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
 	if (!domain->ops)
 		domain->ops = bus->iommu_ops->default_domain_ops;
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5e1afe169549..0c028aa71b96 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -255,7 +255,6 @@ struct iommu_ops {
 	int (*def_domain_type)(struct device *dev);
 
 	const struct iommu_domain_ops *default_domain_ops;
-	unsigned long pgsize_bitmap;
 	struct module *owner;
 };
 


> (and FWIW I'm leaning towards the same for the domain->ops as well; the more I look at ops->default_domain_ops, the more it starts looking like a stupid mess... my stupid mess... sorry about that)
> 
> Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v1 7/7] iommu/amd: Introduce amd_iommu_pgtable command-line option

[Vasant Hegde via iommu]

Hi Joerg,


On 6/23/2022 1:42 PM, Joerg Roedel wrote:
> On Fri, Jun 03, 2022 at 04:51:07PM +0530, Vasant Hegde wrote:
>> +	amd_iommu_pgtable= [HW,X86-64]
>> +			Specifies one of the following AMD IOMMU page table to
>> +			be used for DMA remapping for DMA-API:
>> +			v1         - Use v1 page table (Default)
>> +			v2         - Use v2 page table
>> +
> 
> Can we handle this somehow in the amd_iommu= option? Something like
> amd_iommu=pgtbl_v2|pgtbl_v1?
> 

I think it will complicate the parsing logic. We do have `amd_iommu=off` option.
How are we going to handle `amd_iommu=off,[pgtable_v1/v2]` ? 

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

Re: [PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[Vasant Hegde via iommu]

Hi Joerg,

On 6/23/2022 1:45 PM, Joerg Roedel wrote:
> On Fri, Jun 03, 2022 at 04:51:00PM +0530, Vasant Hegde wrote:
>> - Part 1 (patch 1-4 and 6)
>>   Refactor the current IOMMU page table code to adopt the generic IO page
>>   table framework, and add AMD IOMMU Guest (v2) page table management code.
>>
>> - Part 2 (patch 5)
>>   Add support for the AMD IOMMU Guest IO Protection feature (GIOV)
>>   where requests from the I/O device without a PASID are treated as
>>   if they have PASID of 0.
>>
>> - Part 3 (patch 7)
>>   Introduce new "amd_iommu_pgtable" command-line to allow users
>>   to select the mode of operation (v1 or v2).
> 
> Something I didn't get entirely from the review is support level of the
> amd_iommu_v2 driver. I think it will continue to work and the
> requirement that the device identity maps DMA requests without PASID is
> removed, right?

Sorry. I didn't get last statement ("device identity maps DMA requests without PASID").
Can you please elaborate?

-Vasant

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

Re: [PATCH v1 7/7] iommu/amd: Introduce amd_iommu_pgtable command-line option

[Joerg Roedel]

On Tue, Jun 28, 2022 at 01:23:52PM +0530, Vasant Hegde wrote:
> I think it will complicate the parsing logic. We do have `amd_iommu=off` option.
> How are we going to handle `amd_iommu=off,[pgtable_v1/v2]` ? 

In that case everything except 'off' will be ignored. The driver might
set its internal variables, but this has no effect as the driver never
initializes.

Regards,

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

Re: [PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[Joerg Roedel]

On Tue, Jun 28, 2022 at 02:35:51PM +0530, Vasant Hegde wrote:
> Sorry. I didn't get last statement ("device identity maps DMA requests without PASID").
> Can you please elaborate?

When using v1 page-tables, each device supporting ATS/PRI/PASID needs to
be direct-mapped, because the v1 page-tables basically act as a stage-2
page table for the PASID ones.

But when the non-pasid case moves to the pasid==0 page-table, then there
is not stage-2 anymore and a device can be used with ATS/PRI/PASID while
non-PASID requests are translated too, no?

I didn't get how this is handled in the current patch-set.

Regards,

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

Re: [PATCH v1 0/7] iommu/amd: Add Generic IO Page Table Framework Support for v2 Page Table

[Suthikulpanit, Suravee]

(RESEND)

Joerg,

The goal of this series is to allow DMA-API to use IOMMUv2 page table with PASID=0 (instead of v1 only).
The main use-case is to support nested IO page table for vIOMMU support (work in progress).

On 7/6/2022 4:14 PM, Joerg Roedel wrote:
 > On Tue, Jun 28, 2022 at 02:35:51PM +0530, Vasant Hegde wrote:
 >> Sorry. I didn't get last statement ("device identity maps DMA requests without PASID").
 >> Can you please elaborate?
 >
 > When using v1 page-tables, each device supporting ATS/PRI/PASID needs to
 > be direct-mapped, because the v1 page-tables basically act as a stage-2
 > page table for the PASID ones.

For ATS/PRI capable devices, it is currently handled by the existing AMD IOMMU v2 APIs,
In this case, the v1 table (stage2) is setup with DTE[mode]=0, and we set up v2 table (stage1)
to bind the process address space to the gCR3 table with the specified PASID.

 > But when the non-pasid case moves to the pasid==0 page-table, then there
 > is not stage-2 anymore and a device can be used with ATS/PRI/PASID while
 > non-PASID requests are translated too, no?

When we setup IOMMU v2 table w/ PASID=0, we also set the DTE[mode]=0 to setup
v1 table (stage 2) as identity-map (see set_dte_entry()).

In this case, if the device doesn't support ATS and PRI capability, we don't set
DTE[PPR] bit and/or DTE[I] (IOTLB enable for ATS)

Also, in case of booting w/ iommu=pt (i.e. identity mapping), we fallback to using v1 page table (setting DTE[Mode]=0).

Regards,
Suravee

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Vasant Hegde]

Hi Robin,

On 6/27/2022 3:59 PM, Vasant Hegde wrote:
> 
> 
> On 6/23/2022 3:12 PM, Robin Murphy wrote:
>> On 2022-06-23 09:03, Joerg Roedel wrote:
>>> On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
>>>> Fix below sparse warning:
>>>>    CHECK   drivers/iommu/amd/iommu.c
>>>>    drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?
>>>>
>>>> Also we are going to introduce v2 page table which has different
>>>> pgsize_bitmaps. Hence remove 'const' qualifier.
>>>
>>> I am not a fan of removing the consts. Please use separate ops
>>> structures for v2 page-tables and make then const as well. This probably
>>> also has some optimization potential in the future when we can make the
>>> ops call-back functions page-table specific.
>>
>> TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the very least it would be logical to move it to iommu_domain_ops now, but maybe we could skip ahead and just rely on drivers initialising domain->pgsize_bitmap directly in their .domain_alloc?
>>
> 
> Robin,
> 
> Something like below? If yes, I will cleanup and get proper fix.

Ping.


Will you be cleaning up pgsize_bitmap -OR- do you want me to cleanup (I can do that as separate
series)?

-Vasant

> 
> 
> -Vasant
> 
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 840831d5d2ad..32dd84a7c1da 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1916,6 +1916,7 @@ static int protection_domain_init_v1(struct protection_domain *domain, int mode)
>  			return -ENOMEM;
>  	}
>  
> +	domain->domain.pgsize_bitmap	= AMD_IOMMU_PGSIZES;
>  	amd_iommu_domain_set_pgtable(domain, pt_root, mode);
>  
>  	return 0;
> @@ -2282,7 +2283,6 @@ const struct iommu_ops amd_iommu_ops = {
>  	.get_resv_regions = amd_iommu_get_resv_regions,
>  	.put_resv_regions = generic_iommu_put_resv_regions,
>  	.is_attach_deferred = amd_iommu_is_attach_deferred,
> -	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
>  	.def_domain_type = amd_iommu_def_domain_type,
>  	.default_domain_ops = &(const struct iommu_domain_ops) {
>  		.attach_dev	= amd_iommu_attach_device,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 847ad47a2dfd..73cfba6a6728 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/ iommu/iommu.c
> @@ -1915,8 +1915,6 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>  		return NULL;
>  
>  	domain->type = type;
> -	/* Assume all sizes by default; the driver may override this later */
> -	domain->pgsize_bitmap = bus->iommu_ops->pgsize_bitmap;
>  	if (!domain->ops)
>  		domain->ops = bus->iommu_ops->default_domain_ops;
>  
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 5e1afe169549..0c028aa71b96 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -255,7 +255,6 @@ struct iommu_ops {
>  	int (*def_domain_type)(struct device *dev);
>  
>  	const struct iommu_domain_ops *default_domain_ops;
> -	unsigned long pgsize_bitmap;
>  	struct module *owner;
>  };
>  
> 
> 
>> (and FWIW I'm leaning towards the same for the domain->ops as well; the more I look at ops->default_domain_ops, the more it starts looking like a stupid mess... my stupid mess... sorry about that)
>>
>> Robin.

Re: [PATCH v1 3/7] iommu/amd: Fix sparse warning

[Robin Murphy]

On 2022-07-13 05:07, Vasant Hegde wrote:
> 
> Hi Robin,
> 
> On 6/27/2022 3:59 PM, Vasant Hegde wrote:
>>
>>
>> On 6/23/2022 3:12 PM, Robin Murphy wrote:
>>> On 2022-06-23 09:03, Joerg Roedel wrote:
>>>> On Fri, Jun 03, 2022 at 04:51:03PM +0530, Vasant Hegde wrote:
>>>>> Fix below sparse warning:
>>>>>     CHECK   drivers/iommu/amd/iommu.c
>>>>>     drivers/iommu/amd/iommu.c:73:24: warning: symbol 'amd_iommu_ops' was not declared. Should it be static?
>>>>>
>>>>> Also we are going to introduce v2 page table which has different
>>>>> pgsize_bitmaps. Hence remove 'const' qualifier.
>>>>
>>>> I am not a fan of removing the consts. Please use separate ops
>>>> structures for v2 page-tables and make then const as well. This probably
>>>> also has some optimization potential in the future when we can make the
>>>> ops call-back functions page-table specific.
>>>
>>> TBH it's probably time to retire iommu_ops->pgsize_bitmap anyway. At the very least it would be logical to move it to iommu_domain_ops now, but maybe we could skip ahead and just rely on drivers initialising domain->pgsize_bitmap directly in their .domain_alloc?
>>>
>>
>> Robin,
>>
>> Something like below? If yes, I will cleanup and get proper fix.
> 
> Ping.

Sorry for the delay :)

> Will you be cleaning up pgsize_bitmap -OR- do you want me to cleanup (I can do that as separate
> series)?

I started prototyping it too, and at least for the Arm SMMU drivers it 
turns out the change is a little bit fiddly, then the idea started 
steadily turned into moving *all* the data out of iommu_ops. That then 
ties directly into what I'm already working on, so I'm happy to pick it 
up. I think it should fit in quite neatly right after the 
bus_set_iommu() series, so I'll aim to get something ready for next cycle.

Thanks,
Robin.