[PATCH RESEND 00/10] iommu/amd: SVA Support (Part 4) - SVA and IOPF

[PATCH RESEND 00/10] iommu/amd: SVA Support (Part 4) - SVA and IOPF

[Vasant Hegde]

This is part 4 of the 4-part series to introduce Share Virtual Address
(SVA) support for devices, which can support PCI ATS, PASID and PRI
capabilities. These devices are referred to as SVA-capable devices in
this series.

It contains the following enhancements:

* Patch 1 - 2:
  Rename / add support to enable/disable features

* Patch 3 - 4:
  Introduce SVA support

* Patch 5 - 10:
  Add IOPF support

This patch series is based on top of SVA Part 3 [1].
  [1] https://lore.kernel.org/linux-iommu/20230816174031.634453-1-vasant.hegde@amd.com/

This is also available at github :
  https://github.com/AMDESE/linux/tree/iommu_sva_part4_v1_v6.5_rc1


Thank you,
Vasant / Suravee


Suravee Suthikulpanit (5):
  iommu/amd: Add support to enable/disable SVA feature
  iommu/amd: Move PPR-related functions into ppr.c
  iommu/amd: Define per-IOMMU iopf_queue
  iommu/amd: Add support for page response
  iommu/amd: Introduce logic to enable/disable IOPF

Vasant Hegde (2):
  iommu/amd: Rename amd_iommu_v2_supported() as amd_iommu_sva_supported()
  iommu/amd: Initial SVA support for AMD IOMMU

Wei Huang (3):
  iommu/amd: Add support for enabling/disabling IOMMU features
  iommu/amd: Add support for add/remove device for IOPF
  iommu/amd: Add IO page fault notifier handler

 drivers/iommu/amd/Kconfig           |   1 +
 drivers/iommu/amd/Makefile          |   2 +-
 drivers/iommu/amd/amd_iommu.h       |  41 ++-
 drivers/iommu/amd/amd_iommu_types.h |  22 ++
 drivers/iommu/amd/init.c            |  72 ++----
 drivers/iommu/amd/iommu.c           | 170 ++++++++-----
 drivers/iommu/amd/ppr.c             | 377 ++++++++++++++++++++++++++++
 drivers/iommu/amd/sva.c             | 328 ++++++++++++++++++++++++
 8 files changed, 894 insertions(+), 119 deletions(-)
 create mode 100644 drivers/iommu/amd/ppr.c
 create mode 100644 drivers/iommu/amd/sva.c

-- 
2.31.1

[PATCH RESEND 01/10] iommu/amd: Rename amd_iommu_v2_supported() as amd_iommu_sva_supported()

[Vasant Hegde]

To reflect its usage. No functional changes intended.

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

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 92fba9bda3d2..f236d540c917 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -41,7 +41,7 @@ extern int amd_iommu_guest_ir;
 extern enum io_pgtable_fmt amd_iommu_pgtable;
 extern int amd_iommu_gpt_level;
 
-bool amd_iommu_v2_supported(void);
+bool amd_iommu_sva_supported(void);
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
 bool amd_iommu_pc_supported(void);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 64bcf3df37ee..3820ae738088 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3652,7 +3652,7 @@ __setup("ivrs_ioapic",		parse_ivrs_ioapic);
 __setup("ivrs_hpet",		parse_ivrs_hpet);
 __setup("ivrs_acpihid",		parse_ivrs_acpihid);
 
-bool amd_iommu_v2_supported(void)
+bool amd_iommu_sva_supported(void)
 {
 	/* CPU page table size should match IOMMU guest page table size */
 	if (cpu_feature_enabled(X86_FEATURE_LA57) &&
-- 
2.31.1

[PATCH RESEND 02/10] iommu/amd: Add support for enabling/disabling IOMMU features

[Vasant Hegde]

From: Wei Huang <wei.huang2@amd.com>

Add support for struct iommu_ops.dev_{enable/disable}_feat. Please note
that the empty feature switches will be populated by subsequent patches.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 15fce04ac59b..8fd647c0d657 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2563,6 +2563,32 @@ static bool amd_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 	return true;
 }
 
+static int amd_iommu_dev_enable_feature(struct device *dev,
+					enum iommu_dev_features feat)
+{
+	int ret;
+
+	switch (feat) {
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int amd_iommu_dev_disable_feature(struct device *dev,
+					 enum iommu_dev_features feat)
+{
+	int ret;
+
+	switch (feat) {
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
@@ -2574,6 +2600,8 @@ const struct iommu_ops amd_iommu_ops = {
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.pgsize_bitmap	= AMD_IOMMU_PGSIZES,
 	.def_domain_type = amd_iommu_def_domain_type,
+	.dev_enable_feat = amd_iommu_dev_enable_feature,
+	.dev_disable_feat = amd_iommu_dev_disable_feature,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
 		.map_pages	= amd_iommu_map_pages,
-- 
2.31.1

[PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde]

This includes :
  - Add support to allocate SVA domain. It also sets 'set_dev_pasid'
    to bind PASID to device and make necessary updates to DTE.

  - Add iommu_ops.remove_dev_pasid support. It will unbind PASID from
    device and makes necessary update to DTE.

  - PASID management
    xarray to keep track the PASIDs
    struct amd_sva_pasid
      It will contain mm struct, mmu notifier, protection domain and list
      of devices within IOMMU group using this pasid.
    struct amd_sva_dev
      This contains device list and pointer to iommu_dev_data. So that we
      can track pasid to device binding.

  - mmu notifier for iotlb invalidation

  - Add IOMMU_SVA as dependency to AMD_IOMMU driver

Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/Kconfig           |   1 +
 drivers/iommu/amd/Makefile          |   2 +-
 drivers/iommu/amd/amd_iommu.h       |   5 +
 drivers/iommu/amd/amd_iommu_types.h |  10 +
 drivers/iommu/amd/iommu.c           |  10 +
 drivers/iommu/amd/sva.c             | 299 ++++++++++++++++++++++++++++
 6 files changed, 326 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/amd/sva.c

diff --git a/drivers/iommu/amd/Kconfig b/drivers/iommu/amd/Kconfig
index 75132ae861a2..cabf4ccde1ed 100644
--- a/drivers/iommu/amd/Kconfig
+++ b/drivers/iommu/amd/Kconfig
@@ -10,6 +10,7 @@ config AMD_IOMMU
 	select IOMMU_API
 	select IOMMU_IOVA
 	select IOMMU_IO_PGTABLE
+	select IOMMU_SVA
 	depends on X86_64 && PCI && ACPI && HAVE_CMPXCHG_DOUBLE
 	help
 	  With this option you can enable support for AMD IOMMU hardware in
diff --git a/drivers/iommu/amd/Makefile b/drivers/iommu/amd/Makefile
index f454fbb1569e..848c89fa5238 100644
--- a/drivers/iommu/amd/Makefile
+++ b/drivers/iommu/amd/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o io_pgtable_v2.o
+obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o io_pgtable_v2.o sva.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += debugfs.o
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index f236d540c917..6fdad7a54781 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -41,7 +41,12 @@ extern int amd_iommu_guest_ir;
 extern enum io_pgtable_fmt amd_iommu_pgtable;
 extern int amd_iommu_gpt_level;
 
+/* SVA */
 bool amd_iommu_sva_supported(void);
+int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
+			    struct device *dev, ioasid_t pasid);
+void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid);
+
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
 bool amd_iommu_pc_supported(void);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index b1a54d8c7506..88b9e41b6fbc 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -922,6 +922,16 @@ static inline int get_hpet_devid(int id)
 	return -EINVAL;
 }
 
+static inline struct protection_domain *amd_iommu_get_pdomain(struct device *dev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	if (!dev_data || !dev_data->domain)
+		return NULL;
+
+	return dev_data->domain;
+}
+
 enum amd_iommu_intr_mode_type {
 	AMD_IOMMU_GUEST_IR_LEGACY,
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8fd647c0d657..46bb1bfdc322 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -76,6 +76,7 @@ struct iommu_cmd {
 struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
+static void amd_iommu_domain_free(struct iommu_domain *dom);
 
 static void set_dte_entry(struct amd_iommu *iommu, u16 devid,
 			  struct gcr3_tbl_info *gcr3_info,
@@ -2187,6 +2188,11 @@ static int protection_domain_init_v2(struct protection_domain *pdom)
 	return 0;
 }
 
+static const struct iommu_domain_ops amd_svm_domain_ops = {
+	.set_dev_pasid = amd_iommu_set_dev_pasid,
+	.free	       = amd_iommu_domain_free
+};
+
 static struct protection_domain *protection_domain_alloc(unsigned int type)
 {
 	struct io_pgtable_ops *pgtbl_ops;
@@ -2221,6 +2227,9 @@ static struct protection_domain *protection_domain_alloc(unsigned int type)
 	case IOMMU_DOMAIN_UNMANAGED:
 		pgtable = AMD_IOMMU_V1;
 		break;
+	case IOMMU_DOMAIN_SVA:
+		domain->domain.ops = &amd_svm_domain_ops;
+		return domain;
 	default:
 		goto out_err;
 	}
@@ -2602,6 +2611,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.def_domain_type = amd_iommu_def_domain_type,
 	.dev_enable_feat = amd_iommu_dev_enable_feature,
 	.dev_disable_feat = amd_iommu_dev_disable_feature,
+	.remove_dev_pasid = amd_iommu_remove_dev_pasid,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
 		.map_pages	= amd_iommu_map_pages,
diff --git a/drivers/iommu/amd/sva.c b/drivers/iommu/amd/sva.c
new file mode 100644
index 000000000000..c7c7e7cb5414
--- /dev/null
+++ b/drivers/iommu/amd/sva.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ */
+
+#define pr_fmt(fmt)     "AMD-Vi: " fmt
+#define dev_fmt(fmt)    pr_fmt(fmt)
+
+#include <linux/iommu.h>
+#include <linux/mm_types.h>
+#include <linux/mmu_notifier.h>
+
+#include "amd_iommu.h"
+#include "../iommu-sva.h"
+
+struct amd_sva_pasid {
+	u32			pasid;	/* PASID index */
+	struct mm_struct	*mm;	/* mm_struct for the faults */
+	struct mmu_notifier	mn;	/* mmu_notifier handle */
+	struct list_head	dev_list; /* List of devices for this pasid */
+};
+
+struct amd_sva_dev {
+	struct device		*dev;
+	struct iommu_dev_data	*dev_data;
+	struct list_head	list;
+	struct rcu_head		rcu;
+};
+
+static DEFINE_MUTEX(pasid_mutex);
+static DEFINE_XARRAY_ALLOC(sva_pasid_array);
+
+
+static int sva_pasid_private_add(u32 pasid, void *priv)
+{
+	return xa_alloc(&sva_pasid_array, &pasid, priv,
+			XA_LIMIT(pasid, pasid), GFP_ATOMIC);
+}
+
+static void sva_pasid_private_remove(u32 pasid)
+{
+	xa_erase(&sva_pasid_array, pasid);
+}
+
+static void *sva_pasid_private_find(u32 pasid)
+{
+	return xa_load(&sva_pasid_array, pasid);
+}
+
+static struct amd_sva_dev *sva_dev_alloc(struct device *dev)
+{
+	struct amd_sva_dev *sva_dev;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	sva_dev = kzalloc(sizeof(*sva_dev), GFP_KERNEL);
+	if (!sva_dev)
+		return NULL;
+
+	sva_dev->dev = dev;
+	sva_dev->dev_data = dev_data;
+	init_rcu_head(&sva_dev->rcu);
+
+	return sva_dev;
+}
+
+static inline void sva_dev_free(struct amd_sva_dev *sva_dev)
+{
+	kfree_rcu(sva_dev, rcu);
+}
+
+static void sva_mn_invalidate_range(struct mmu_notifier *mn,
+				    struct mm_struct *mm,
+				    unsigned long start, unsigned long end)
+{
+	struct amd_sva_pasid *sva_pasid;
+	struct amd_sva_dev *sva_dev;
+
+	rcu_read_lock();
+
+	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
+	if (!sva_pasid) {
+		rcu_read_unlock();
+		return;
+	}
+
+	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
+		if ((start ^ (end - 1)) < PAGE_SIZE)
+			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
+		else
+			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
+	}
+
+	rcu_read_unlock();
+}
+
+static void sva_mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
+{
+	struct amd_sva_pasid *sva_pasid;
+	struct amd_sva_dev *sva_dev;
+
+	rcu_read_lock();
+
+	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
+	if (!sva_pasid) {
+		rcu_read_unlock();
+		return;
+	}
+
+	/* make it visible */
+	smp_wmb();
+
+	/*
+	 * This might end up being called from exit_mmap(), *before* unbinding
+	 * the PASID. In such cases we clear the PASID table and flush IOTLB.
+	 */
+	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list)
+		amd_iommu_clear_gcr3(sva_dev->dev_data, sva_pasid->pasid);
+
+	rcu_read_unlock();
+}
+
+static const struct mmu_notifier_ops sva_mn = {
+	.invalidate_range = sva_mn_invalidate_range,
+	.release = sva_mn_release,
+};
+
+static struct amd_sva_pasid *sva_pasid_alloc(struct mm_struct *mm)
+{
+	struct amd_sva_pasid *sva_pasid;
+
+	sva_pasid = kzalloc(sizeof(*sva_pasid), GFP_KERNEL);
+	if (!sva_pasid)
+		return NULL;
+
+	sva_pasid->pasid = mm->pasid;
+	sva_pasid->mm = mm;
+	sva_pasid->mn.ops = &sva_mn;
+	INIT_LIST_HEAD(&sva_pasid->dev_list);
+
+	return sva_pasid;
+}
+
+static inline void sva_pasid_free(struct amd_sva_pasid *sva_pasid)
+{
+	kfree(sva_pasid);
+}
+
+static struct amd_sva_dev *pasid_to_sva_dev(struct device *dev,
+					    struct amd_sva_pasid *sva_pasid)
+{
+	struct amd_sva_dev *sva_dev;
+
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
+		if (sva_dev->dev == dev) {
+			rcu_read_unlock();
+			return sva_dev;
+		}
+	}
+
+	rcu_read_unlock();
+	return NULL;
+}
+
+static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
+{
+	struct amd_sva_pasid *sva_pasid;
+	struct amd_sva_dev *sva_dev;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+	int ret = -EINVAL;
+
+	sva_dev = sva_dev_alloc(dev);
+	if (!sva_dev)
+		return ret;
+
+	sva_pasid = sva_pasid_private_find(mm->pasid);
+	if (!sva_pasid) {
+		sva_pasid = sva_pasid_alloc(mm);
+		if (!sva_pasid)
+			goto out_sva_dev;
+
+		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
+		if (ret)
+			goto out_sva_pasid;
+
+		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
+					 iommu_virt_to_phys(sva_pasid->mm->pgd));
+		if (ret)
+			goto out_pasid_remove;
+
+		ret = mmu_notifier_register(&sva_pasid->mn, mm);
+		if (ret)
+			goto out_clear_gcr3;
+	}
+
+	/* Add sva_dev into list */
+	list_add(&sva_dev->list, &sva_pasid->dev_list);
+	return 0;
+
+out_clear_gcr3:
+	amd_iommu_clear_gcr3(dev_data, sva_pasid->pasid);
+
+out_pasid_remove:
+	sva_pasid_private_remove(sva_pasid->pasid);
+
+out_sva_pasid:
+	sva_pasid_free(sva_pasid);
+
+out_sva_dev:
+	sva_dev_free(sva_dev);
+	return ret;
+}
+
+static void sva_unbind_mm(struct device *dev, u32 pasid)
+{
+	struct amd_sva_pasid *sva_pasid;
+	struct amd_sva_dev *sva_dev;
+
+	sva_pasid = sva_pasid_private_find(pasid);
+	if (!sva_pasid)
+		return;
+
+	sva_dev = pasid_to_sva_dev(dev, sva_pasid);
+	if (!sva_dev)
+		return;
+
+	/* make it visible */
+	smp_wmb();
+
+	/* Update GCR3 table and flush IOTLB */
+	amd_iommu_clear_gcr3(sva_dev->dev_data, sva_pasid->pasid);
+
+	list_del_rcu(&sva_dev->list);
+	sva_dev_free(sva_dev);
+
+	if (list_empty(&sva_pasid->dev_list)) {
+		if (sva_pasid->mn.ops)
+			mmu_notifier_unregister(&sva_pasid->mn, sva_pasid->mm);
+
+		sva_pasid_private_remove(pasid);
+		sva_pasid_free(sva_pasid);
+	}
+}
+
+int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
+			    struct device *dev, ioasid_t pasid)
+{
+	struct protection_domain *pdom;
+	struct mm_struct *mm = domain->mm;
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+	int ret = -EINVAL;
+
+	/* PASID zero is used for requests from the I/O device without PASID */
+	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
+		return ret;
+
+	/* Make sure SVA mode is enabled for device default domain */
+	pdom = amd_iommu_get_pdomain(dev);
+
+	if (!pdom || pdom->pd_mode != PD_MODE_V2 ||
+	    dev_data->gcr3_info.gcr3_tbl == NULL)
+		return ret;
+
+	/* Init sva_pasid and bind mm */
+	mutex_lock(&pasid_mutex);
+	ret = sva_bind_mm(dev, mm);
+	mutex_unlock(&pasid_mutex);
+
+	return ret;
+}
+
+void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
+{
+	struct iommu_domain *domain;
+
+	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
+		return;
+
+	/* Get SVA domain */
+	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
+	if (!domain)
+		return;
+
+	switch (domain->type) {
+	case IOMMU_DOMAIN_SVA:
+		/* Ensure that all queued faults have been processed */
+		iopf_queue_flush_dev(dev);
+
+		mutex_lock(&pasid_mutex);
+		sva_unbind_mm(dev, pasid);
+		mutex_unlock(&pasid_mutex);
+		break;
+	default:
+		/* Should never reach here */
+		WARN_ON(1);
+		break;
+	}
+}
-- 
2.31.1

[PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Vasant Hegde]

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

Add support to enable/disable SVA feature. It will setup v2
page table mode with PASID and enables SVA.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  4 +++
 drivers/iommu/amd/iommu.c     | 63 +++++++++++++++++++++++++++++++++++
 drivers/iommu/amd/sva.c       | 29 ++++++++++++++++
 3 files changed, 96 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6fdad7a54781..3f7ddf19ed72 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -46,6 +46,10 @@ bool amd_iommu_sva_supported(void);
 int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
 			    struct device *dev, ioasid_t pasid);
 void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid);
+int amd_iommu_sva_enable(struct device *dev);
+int amd_iommu_sva_disable(struct device *dev);
+int amd_iommu_sva_gcr3_init(struct iommu_dev_data *dev_data, int pasids);
+int amd_iommu_sva_gcr3_uninit(struct iommu_dev_data *dev_data);
 
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 46bb1bfdc322..4868eee4c4f9 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2162,6 +2162,63 @@ static void protection_domain_free(struct protection_domain *domain)
 	kfree(domain);
 }
 
+/*******************************
+ * SVA-related helper functions
+ */
+static inline bool pdom_is_pt_mode(struct protection_domain *pdom)
+{
+	return (pdom->domain.type == IOMMU_DOMAIN_IDENTITY);
+}
+
+static inline bool pdom_is_v2_pgtbl_mode(struct protection_domain *pdom)
+{
+	return (pdom->iop.pgd != NULL);
+}
+
+int amd_iommu_sva_gcr3_init(struct iommu_dev_data *dev_data, int pasids)
+{
+	struct protection_domain *pdom = dev_data->domain;
+	unsigned long flags;
+	int ret = 0;
+
+	/*
+	 * We cannot support SVA w/ existing v1 page table in the same domain
+	 * since it will be nested. However, existing domain w/ v2 pagetable
+	 * can be used for SVA.
+	 */
+	if (pdom->pd_mode == PD_MODE_V1)
+		return -EOPNOTSUPP;
+
+	spin_lock_irqsave(&dev_data->lock, flags);
+
+	/* Allocate GCR3 table */
+	if (pdom_is_pt_mode(dev_data->domain))
+		ret = setup_gcr3_table(dev_data, pasids);
+
+	spin_unlock_irqrestore(&dev_data->lock, flags);
+	return ret;
+}
+
+int amd_iommu_sva_gcr3_uninit(struct iommu_dev_data *dev_data)
+{
+	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev_data->lock, flags);
+
+	if (gcr3_info->pasid_cnt) {
+		spin_unlock_irqrestore(&dev_data->lock, flags);
+		return -EBUSY;
+	}
+
+	/* Free GCR3 table */
+	if (pdom_is_pt_mode(dev_data->domain))
+		free_gcr3_table(dev_data);
+
+	spin_unlock_irqrestore(&dev_data->lock, flags);
+	return 0;
+}
+
 static int protection_domain_init_v1(struct protection_domain *domain, int mode)
 {
 	u64 *pt_root = NULL;
@@ -2578,6 +2635,9 @@ static int amd_iommu_dev_enable_feature(struct device *dev,
 	int ret;
 
 	switch (feat) {
+	case IOMMU_DEV_FEAT_SVA:
+		ret = amd_iommu_sva_enable(dev);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2591,6 +2651,9 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
 	int ret;
 
 	switch (feat) {
+	case IOMMU_DEV_FEAT_SVA:
+		ret = amd_iommu_sva_disable(dev);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/iommu/amd/sva.c b/drivers/iommu/amd/sva.c
index c7c7e7cb5414..b4cc5b854753 100644
--- a/drivers/iommu/amd/sva.c
+++ b/drivers/iommu/amd/sva.c
@@ -297,3 +297,32 @@ void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 		break;
 	}
 }
+
+int amd_iommu_sva_enable(struct device *dev)
+{
+	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	if (!pdev || !iommu || !dev_data)
+		return -EINVAL;
+
+	if (!amd_iommu_sva_supported())
+		return -ENODEV;
+
+	if (!dev_data->pasid_enabled)
+		return -EINVAL;
+
+	return amd_iommu_sva_gcr3_init(dev_data, dev->iommu->max_pasids);
+}
+
+int amd_iommu_sva_disable(struct device *dev)
+{
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	if (!iommu || !dev_data)
+		return -EINVAL;
+
+	return amd_iommu_sva_gcr3_uninit(dev_data);
+}
-- 
2.31.1

[PATCH RESEND 05/10] iommu/amd: Move PPR-related functions into ppr.c

[Vasant Hegde]

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

In preparation to subsequent PPR-related patches, and also remove static
declaration for certain helper functions so that it can be reused in other
files.

Also rename below functions:
  alloc_ppr_log        -> amd_iommu_alloc_ppr_log
  iommu_enable_ppr_log -> amd_iommu_enable_ppr_log
  free_ppr_log         -> amd_iommu_free_ppr_log
  iommu_poll_ppr_log   -> amd_iommu_poll_ppr_log

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/Makefile    |   2 +-
 drivers/iommu/amd/amd_iommu.h |  17 +++++-
 drivers/iommu/amd/init.c      |  65 +++-----------------
 drivers/iommu/amd/iommu.c     |  55 +----------------
 drivers/iommu/amd/ppr.c       | 112 ++++++++++++++++++++++++++++++++++
 5 files changed, 137 insertions(+), 114 deletions(-)
 create mode 100644 drivers/iommu/amd/ppr.c

diff --git a/drivers/iommu/amd/Makefile b/drivers/iommu/amd/Makefile
index 848c89fa5238..a6601d885c5b 100644
--- a/drivers/iommu/amd/Makefile
+++ b/drivers/iommu/amd/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o io_pgtable_v2.o sva.o
+obj-$(CONFIG_AMD_IOMMU) += iommu.o init.o quirks.o io_pgtable.o io_pgtable_v2.o sva.o ppr.o
 obj-$(CONFIG_AMD_IOMMU_DEBUGFS) += debugfs.o
diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 3f7ddf19ed72..31e6cb3ba5d6 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -17,6 +17,9 @@ irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data);
 irqreturn_t amd_iommu_int_thread_galog(int irq, void *data);
 irqreturn_t amd_iommu_int_handler(int irq, void *data);
 void amd_iommu_apply_erratum_63(struct amd_iommu *iommu, u16 devid);
+void amd_iommu_restart_log(struct amd_iommu *iommu, const char *evt_type,
+			   u8 cntrl_intr, u8 cntrl_log,
+			   u32 status_run_mask, u32 status_overflow_mask);
 void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
 void amd_iommu_restart_ga_log(struct amd_iommu *iommu);
 void amd_iommu_restart_ppr_log(struct amd_iommu *iommu);
@@ -24,6 +27,9 @@ int amd_iommu_init_devices(void);
 void amd_iommu_uninit_devices(void);
 void amd_iommu_init_notifier(void);
 void amd_iommu_set_rlookup_table(struct amd_iommu *iommu, u16 devid);
+void iommu_feature_enable(struct amd_iommu *iommu, u8 bit);
+void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
+				  gfp_t gfp, size_t size);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
@@ -69,6 +75,14 @@ int amd_iommu_set_gcr3(struct iommu_dev_data *dev_data,
 		       u32 pasid, unsigned long gcr3);
 int amd_iommu_clear_gcr3(struct iommu_dev_data *dev_data, u32 pasid);
 
+/* PPR */
+int __init amd_iommu_alloc_ppr_log(struct amd_iommu *iommu);
+void __init amd_iommu_free_ppr_log(struct amd_iommu *iommu);
+void amd_iommu_enable_ppr_log(struct amd_iommu *iommu);
+void amd_iommu_poll_ppr_log(struct amd_iommu *iommu);
+int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
+			   int status, int tag);
+
 int amd_iommu_flush_page(struct protection_domain *domain, u32 pasid, u64 address);
 void amd_iommu_update_and_flush_device_table(struct protection_domain *domain);
 void amd_iommu_domain_update(struct protection_domain *domain);
@@ -89,9 +103,6 @@ static inline int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
 #define PPR_INVALID			0x1
 #define PPR_FAILURE			0xf
 
-int amd_iommu_complete_ppr(struct pci_dev *pdev, u32 pasid,
-			   int status, int tag);
-
 static inline bool is_rd890_iommu(struct pci_dev *pdev)
 {
 	return (pdev->vendor == PCI_VENDOR_ID_ATI) &&
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 3820ae738088..a20a7f998ecd 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -418,7 +418,7 @@ static void iommu_set_device_table(struct amd_iommu *iommu)
 }
 
 /* Generic functions to enable/disable certain features of the IOMMU. */
-static void iommu_feature_enable(struct amd_iommu *iommu, u8 bit)
+void iommu_feature_enable(struct amd_iommu *iommu, u8 bit)
 {
 	u64 ctrl;
 
@@ -745,9 +745,9 @@ static int __init alloc_command_buffer(struct amd_iommu *iommu)
  * Interrupt handler has processed all pending events and adjusted head
  * and tail pointer. Reset overflow mask and restart logging again.
  */
-static void amd_iommu_restart_log(struct amd_iommu *iommu, const char *evt_type,
-				  u8 cntrl_intr, u8 cntrl_log,
-				  u32 status_run_mask, u32 status_overflow_mask)
+void amd_iommu_restart_log(struct amd_iommu *iommu, const char *evt_type,
+			   u8 cntrl_intr, u8 cntrl_log,
+			   u32 status_run_mask, u32 status_overflow_mask)
 {
 	u32 status;
 
@@ -788,17 +788,6 @@ void amd_iommu_restart_ga_log(struct amd_iommu *iommu)
 			      MMIO_STATUS_GALOG_OVERFLOW_MASK);
 }
 
-/*
- * This function restarts ppr logging in case the IOMMU experienced
- * PPR log overflow.
- */
-void amd_iommu_restart_ppr_log(struct amd_iommu *iommu)
-{
-	amd_iommu_restart_log(iommu, "PPR", CONTROL_PPRINT_EN,
-			      CONTROL_PPRLOG_EN, MMIO_STATUS_PPR_RUN_MASK,
-			      MMIO_STATUS_PPR_OVERFLOW_MASK);
-}
-
 /*
  * This function resets the command buffer if the IOMMU stopped fetching
  * commands from it.
@@ -847,8 +836,8 @@ static void __init free_command_buffer(struct amd_iommu *iommu)
 	free_pages((unsigned long)iommu->cmd_buf, get_order(CMD_BUFFER_SIZE));
 }
 
-static void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
-					 gfp_t gfp, size_t size)
+void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu, gfp_t gfp,
+				  size_t size)
 {
 	int order = get_order(size);
 	void *buf = (void *)__get_free_pages(gfp, order);
@@ -903,42 +892,6 @@ static void __init free_event_buffer(struct amd_iommu *iommu)
 	free_pages((unsigned long)iommu->evt_buf, get_order(EVT_BUFFER_SIZE));
 }
 
-/* allocates the memory where the IOMMU will log its events to */
-static int __init alloc_ppr_log(struct amd_iommu *iommu)
-{
-	iommu->ppr_log = iommu_alloc_4k_pages(iommu, GFP_KERNEL | __GFP_ZERO,
-					      PPR_LOG_SIZE);
-
-	return iommu->ppr_log ? 0 : -ENOMEM;
-}
-
-static void iommu_enable_ppr_log(struct amd_iommu *iommu)
-{
-	u64 entry;
-
-	if (iommu->ppr_log == NULL)
-		return;
-
-	iommu_feature_enable(iommu, CONTROL_PPR_EN);
-
-	entry = iommu_virt_to_phys(iommu->ppr_log) | PPR_LOG_SIZE_512;
-
-	memcpy_toio(iommu->mmio_base + MMIO_PPR_LOG_OFFSET,
-		    &entry, sizeof(entry));
-
-	/* set head and tail to zero manually */
-	writel(0x00, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
-	writel(0x00, iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
-
-	iommu_feature_enable(iommu, CONTROL_PPRLOG_EN);
-	iommu_feature_enable(iommu, CONTROL_PPRINT_EN);
-}
-
-static void __init free_ppr_log(struct amd_iommu *iommu)
-{
-	free_pages((unsigned long)iommu->ppr_log, get_order(PPR_LOG_SIZE));
-}
-
 static void free_ga_log(struct amd_iommu *iommu)
 {
 #ifdef CONFIG_IRQ_REMAP
@@ -1682,7 +1635,7 @@ static void __init free_iommu_one(struct amd_iommu *iommu)
 	free_cwwb_sem(iommu);
 	free_command_buffer(iommu);
 	free_event_buffer(iommu);
-	free_ppr_log(iommu);
+	amd_iommu_free_ppr_log(iommu);
 	free_ga_log(iommu);
 	iommu_unmap_mmio_space(iommu);
 }
@@ -2095,7 +2048,7 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 			amd_iommu_max_glx_val = min(amd_iommu_max_glx_val, glxval);
 	}
 
-	if (check_feature(FEATURE_PPR) && alloc_ppr_log(iommu))
+	if (check_feature(FEATURE_PPR) && amd_iommu_alloc_ppr_log(iommu))
 		return -ENOMEM;
 
 	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
@@ -2839,7 +2792,7 @@ static void enable_iommus_v2(void)
 	struct amd_iommu *iommu;
 
 	for_each_iommu(iommu) {
-		iommu_enable_ppr_log(iommu);
+		amd_iommu_enable_ppr_log(iommu);
 		iommu_enable_gt(iommu);
 	}
 }
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 4868eee4c4f9..5234b1ee8f8b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -820,59 +820,6 @@ static void iommu_poll_events(struct amd_iommu *iommu)
 	writel(head, iommu->mmio_base + MMIO_EVT_HEAD_OFFSET);
 }
 
-static void iommu_poll_ppr_log(struct amd_iommu *iommu)
-{
-	u32 head, tail;
-
-	if (iommu->ppr_log == NULL)
-		return;
-
-	head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
-	tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
-
-	while (head != tail) {
-		volatile u64 *raw;
-		u64 entry[2];
-		int i;
-
-		raw = (u64 *)(iommu->ppr_log + head);
-
-		/*
-		 * Hardware bug: Interrupt may arrive before the entry is
-		 * written to memory. If this happens we need to wait for the
-		 * entry to arrive.
-		 */
-		for (i = 0; i < LOOP_TIMEOUT; ++i) {
-			if (PPR_REQ_TYPE(raw[0]) != 0)
-				break;
-			udelay(1);
-		}
-
-		/* Avoid memcpy function-call overhead */
-		entry[0] = raw[0];
-		entry[1] = raw[1];
-
-		/*
-		 * To detect the hardware errata 733 we need to clear the
-		 * entry back to zero. This issue does not exist on SNP
-		 * enabled system. Also this buffer is not writeable on
-		 * SNP enabled system.
-		 */
-		if (!amd_iommu_snp_en)
-			raw[0] = raw[1] = 0UL;
-
-		/* Update head pointer of hardware ring-buffer */
-		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
-		writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
-
-		/* TODO: PPR Handler will be added when we add IOPF support */
-
-		/* Refresh ring-buffer information */
-		head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
-		tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
-	}
-}
-
 #ifdef CONFIG_IRQ_REMAP
 static int (*iommu_ga_log_notifier)(u32);
 
@@ -993,7 +940,7 @@ irqreturn_t amd_iommu_int_thread_pprlog(int irq, void *data)
 {
 	amd_iommu_handle_irq(data, "PPR", MMIO_STATUS_PPR_INT_MASK,
 			     MMIO_STATUS_PPR_OVERFLOW_MASK,
-			     iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
+			     amd_iommu_poll_ppr_log, amd_iommu_restart_ppr_log);
 
 	return IRQ_HANDLED;
 }
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
new file mode 100644
index 000000000000..673fcc30f9dc
--- /dev/null
+++ b/drivers/iommu/amd/ppr.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ */
+
+#define pr_fmt(fmt)     "AMD-Vi: " fmt
+#define dev_fmt(fmt)    pr_fmt(fmt)
+
+#include <linux/amd-iommu.h>
+#include <linux/delay.h>
+#include <linux/mmu_notifier.h>
+
+#include "amd_iommu.h"
+#include "amd_iommu_types.h"
+
+int __init amd_iommu_alloc_ppr_log(struct amd_iommu *iommu)
+{
+	iommu->ppr_log = iommu_alloc_4k_pages(iommu, GFP_KERNEL | __GFP_ZERO,
+					      PPR_LOG_SIZE);
+	return iommu->ppr_log ? 0 : -ENOMEM;
+}
+
+void amd_iommu_enable_ppr_log(struct amd_iommu *iommu)
+{
+	u64 entry;
+
+	if (iommu->ppr_log == NULL)
+		return;
+
+	iommu_feature_enable(iommu, CONTROL_PPR_EN);
+
+	entry = iommu_virt_to_phys(iommu->ppr_log) | PPR_LOG_SIZE_512;
+
+	memcpy_toio(iommu->mmio_base + MMIO_PPR_LOG_OFFSET,
+		    &entry, sizeof(entry));
+
+	/* set head and tail to zero manually */
+	writel(0x00, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
+	writel(0x00, iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
+
+	iommu_feature_enable(iommu, CONTROL_PPRINT_EN);
+	iommu_feature_enable(iommu, CONTROL_PPRLOG_EN);
+}
+
+void __init amd_iommu_free_ppr_log(struct amd_iommu *iommu)
+{
+	free_pages((unsigned long)iommu->ppr_log, get_order(PPR_LOG_SIZE));
+}
+
+/*
+ * This function restarts ppr logging in case the IOMMU experienced
+ * PPR log overflow.
+ */
+void amd_iommu_restart_ppr_log(struct amd_iommu *iommu)
+{
+	amd_iommu_restart_log(iommu, "PPR", CONTROL_PPRINT_EN,
+			      CONTROL_PPRLOG_EN, MMIO_STATUS_PPR_RUN_MASK,
+			      MMIO_STATUS_PPR_OVERFLOW_MASK);
+}
+
+void amd_iommu_poll_ppr_log(struct amd_iommu *iommu)
+{
+	u32 head, tail;
+
+	if (iommu->ppr_log == NULL)
+		return;
+
+	head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
+	tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
+
+	while (head != tail) {
+		volatile u64 *raw;
+		u64 entry[2];
+		int i;
+
+		raw = (u64 *)(iommu->ppr_log + head);
+
+		/*
+		 * Hardware bug: Interrupt may arrive before the entry is
+		 * written to memory. If this happens we need to wait for the
+		 * entry to arrive.
+		 */
+		for (i = 0; i < LOOP_TIMEOUT; ++i) {
+			if (PPR_REQ_TYPE(raw[0]) != 0)
+				break;
+			udelay(1);
+		}
+
+		/* Avoid memcpy function-call overhead */
+		entry[0] = raw[0];
+		entry[1] = raw[1];
+
+		/*
+		 * To detect the hardware errata 733 we need to clear the
+		 * entry back to zero. This issue does not exist on SNP
+		 * enabled system. Also this buffer is not writeable on
+		 * SNP enabled system.
+		 */
+		if (!amd_iommu_snp_en)
+			raw[0] = raw[1] = 0UL;
+
+		/* Update head pointer of hardware ring-buffer */
+		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
+		writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
+
+		/* TODO: PPR Handler will be added when we add IOPF support */
+
+		/* Refresh ring-buffer information */
+		head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
+		tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
+	}
+}
-- 
2.31.1

[PATCH RESEND 06/10] iommu/amd: Define per-IOMMU iopf_queue

[Vasant Hegde]

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

AMD IOMMU hardware supports PCI Peripheral Paging Request (PPR) using
a PPR log, which is a circular buffer containing requests from downstream
end-point devices.

There is one PPR log per IOMMU instance. Therefore, allocate an iopf_queue
per IOMMU instance during driver initialization, and free the queue during
driver deinitialization.

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

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 31e6cb3ba5d6..5395d97486c8 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -57,6 +57,10 @@ int amd_iommu_sva_disable(struct device *dev);
 int amd_iommu_sva_gcr3_init(struct iommu_dev_data *dev_data, int pasids);
 int amd_iommu_sva_gcr3_uninit(struct iommu_dev_data *dev_data);
 
+/* IOPF */
+int amd_iommu_iopf_init(struct amd_iommu *iommu);
+void amd_iommu_iopf_uninit(struct amd_iommu *iommu);
+
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
 bool amd_iommu_pc_supported(void);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 88b9e41b6fbc..d09ce37cd3e9 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -758,6 +758,10 @@ struct amd_iommu {
 	/* DebugFS Info */
 	struct dentry *debugfs;
 #endif
+
+	/* IOPF support */
+	struct iopf_queue *iopf_queue;
+	unsigned char iopfq_name[32];
 };
 
 static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index a20a7f998ecd..302e964b1eff 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1638,6 +1638,7 @@ static void __init free_iommu_one(struct amd_iommu *iommu)
 	amd_iommu_free_ppr_log(iommu);
 	free_ga_log(iommu);
 	iommu_unmap_mmio_space(iommu);
+	amd_iommu_iopf_uninit(iommu);
 }
 
 static void __init free_iommu_all(void)
@@ -2791,9 +2792,13 @@ static void enable_iommus_v2(void)
 {
 	struct amd_iommu *iommu;
 
+	if (!amd_iommu_gt_ppr_supported())
+		return;
+
 	for_each_iommu(iommu) {
 		amd_iommu_enable_ppr_log(iommu);
 		iommu_enable_gt(iommu);
+		amd_iommu_iopf_init(iommu);
 	}
 }
 
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index 673fcc30f9dc..2e8d55657cb5 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -12,6 +12,7 @@
 
 #include "amd_iommu.h"
 #include "amd_iommu_types.h"
+#include "../iommu-sva.h"
 
 int __init amd_iommu_alloc_ppr_log(struct amd_iommu *iommu)
 {
@@ -110,3 +111,45 @@ void amd_iommu_poll_ppr_log(struct amd_iommu *iommu)
 		tail = readl(iommu->mmio_base + MMIO_PPR_TAIL_OFFSET);
 	}
 }
+
+/**************************************************************
+ *
+ * IOPF handling stuff
+ */
+
+/* Setup per-IOMMU IOPF queue if not exist. */
+int amd_iommu_iopf_init(struct amd_iommu *iommu)
+{
+	unsigned long flags;
+	int ret = 0;
+
+	raw_spin_lock_irqsave(&iommu->lock, flags);
+
+	if (iommu->iopf_queue)
+		goto out;
+
+	snprintf(iommu->iopfq_name, sizeof(iommu->iopfq_name),
+		 "amdiommu-%#x-iopfq",
+		 PCI_SEG_DEVID_TO_SBDF(iommu->pci_seg->id, iommu->devid));
+
+	iommu->iopf_queue = iopf_queue_alloc(iommu->iopfq_name);
+	if (!iommu->iopf_queue)
+		ret = -ENOMEM;
+
+out:
+	raw_spin_unlock_irqrestore(&iommu->lock, flags);
+	return ret;
+}
+
+/* Destroy per-IOMMU IOPF queue if no longer needed. */
+void amd_iommu_iopf_uninit(struct amd_iommu *iommu)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&iommu->lock, flags);
+
+	iopf_queue_free(iommu->iopf_queue);
+	iommu->iopf_queue = NULL;
+
+	raw_spin_unlock_irqrestore(&iommu->lock, flags);
+}
-- 
2.31.1

[PATCH RESEND 07/10] iommu/amd: Add support for page response

[Vasant Hegde]

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

This generates AMD IOMMU COMPLETE_PPR_REQUEST for the specified device
with the specified PRI Response Code.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Wei Huang <wei.huang2@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  3 +++
 drivers/iommu/amd/iommu.c     |  1 +
 drivers/iommu/amd/ppr.c       | 15 +++++++++++++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 5395d97486c8..167541bdcf1d 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -60,6 +60,9 @@ int amd_iommu_sva_gcr3_uninit(struct iommu_dev_data *dev_data);
 /* IOPF */
 int amd_iommu_iopf_init(struct amd_iommu *iommu);
 void amd_iommu_iopf_uninit(struct amd_iommu *iommu);
+int amd_iommu_page_response(struct device *dev,
+			    struct iommu_fault_event *event,
+			    struct iommu_page_response *resp);
 
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5234b1ee8f8b..948f9bf47e68 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2622,6 +2622,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.dev_enable_feat = amd_iommu_dev_enable_feature,
 	.dev_disable_feat = amd_iommu_dev_disable_feature,
 	.remove_dev_pasid = amd_iommu_remove_dev_pasid,
+	.page_response = amd_iommu_page_response,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
 		.map_pages	= amd_iommu_map_pages,
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index 2e8d55657cb5..8cbcdb9c1669 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -153,3 +153,18 @@ void amd_iommu_iopf_uninit(struct amd_iommu *iommu)
 
 	raw_spin_unlock_irqrestore(&iommu->lock, flags);
 }
+
+int amd_iommu_page_response(struct device *dev,
+			    struct iommu_fault_event *event,
+			    struct iommu_page_response *resp)
+{
+	struct pci_dev *pdev;
+
+	if (!dev || !dev_is_pci(dev))
+		return -ENODEV;
+
+	pdev = to_pci_dev(dev);
+
+	return amd_iommu_complete_ppr(pdev, resp->pasid, resp->code,
+				      resp->grpid);
+}
-- 
2.31.1

[PATCH RESEND 08/10] iommu/amd: Add support for add/remove device for IOPF

[Vasant Hegde]

From: Wei Huang <wei.huang2@amd.com>

Which adds/removes the device to the corresponding per-IOMMU iopf_queue.
It also registers device fault handler. These interfaces are called
from IOPF feature enable/disable path.

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  2 ++
 drivers/iommu/amd/ppr.c       | 50 +++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 167541bdcf1d..85b9ed9b350a 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -60,6 +60,8 @@ int amd_iommu_sva_gcr3_uninit(struct iommu_dev_data *dev_data);
 /* IOPF */
 int amd_iommu_iopf_init(struct amd_iommu *iommu);
 void amd_iommu_iopf_uninit(struct amd_iommu *iommu);
+int amd_iommu_iopf_add_device(struct amd_iommu *iommu, struct device *dev);
+int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev);
 int amd_iommu_page_response(struct device *dev,
 			    struct iommu_fault_event *event,
 			    struct iommu_page_response *resp);
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index 8cbcdb9c1669..2e5ca4847eda 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -168,3 +168,53 @@ int amd_iommu_page_response(struct device *dev,
 	return amd_iommu_complete_ppr(pdev, resp->pasid, resp->code,
 				      resp->grpid);
 }
+
+int amd_iommu_iopf_add_device(struct amd_iommu *iommu, struct device *dev)
+{
+	unsigned long flags;
+	int ret = -EINVAL;
+
+	raw_spin_lock_irqsave(&iommu->lock, flags);
+
+	if (!iommu->iopf_queue)
+		goto out;
+
+	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
+	if (ret)
+		goto out;
+
+	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
+	if (ret)
+		iopf_queue_remove_device(iommu->iopf_queue, dev);
+
+out:
+	raw_spin_unlock_irqrestore(&iommu->lock, flags);
+	return ret;
+}
+
+int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev)
+{
+	unsigned long flags;
+	int ret = -EINVAL;
+
+	raw_spin_lock_irqsave(&iommu->lock, flags);
+
+	if (!iommu->iopf_queue)
+		goto out;
+
+	ret = iommu_unregister_device_fault_handler(dev);
+	if (ret) {
+		pr_warn("Failed to unregister device (0x%x) fault handler\n",
+			dev->id);
+	}
+
+	ret = iopf_queue_remove_device(iommu->iopf_queue, dev);
+	if (ret) {
+		pr_warn("Failed to remove device (0x%x) from iopf queue\n",
+			dev->id);
+	}
+
+out:
+	raw_spin_unlock_irqrestore(&iommu->lock, flags);
+	return ret;
+}
-- 
2.31.1

[PATCH RESEND 09/10] iommu/amd: Add IO page fault notifier handler

[Vasant Hegde]

From: Wei Huang <wei.huang2@amd.com>

Whenever there is a page fault IOMMU logs entry to ppr log and sends
interrupt to host. We have to handle the page fault and respond to
IOMMU.

Currentlty we have page fault handler support for V2API mode. Let us add
support for SVA mode. We will use core IOMMU page fault handler. We just
need to validate the request and call core iommu fault handler interface.

Also rename search_dev_data() -> amd_iommu_search_dev_data().

Signed-off-by: Wei Huang <wei.huang2@amd.com>
Co-developed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h       |   2 +
 drivers/iommu/amd/amd_iommu_types.h |   8 +++
 drivers/iommu/amd/iommu.c           |   7 +-
 drivers/iommu/amd/ppr.c             | 103 +++++++++++++++++++++++++++-
 4 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 85b9ed9b350a..d2dfd7b6b6e2 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -30,6 +30,8 @@ void amd_iommu_set_rlookup_table(struct amd_iommu *iommu, u16 devid);
 void iommu_feature_enable(struct amd_iommu *iommu, u8 bit);
 void *__init iommu_alloc_4k_pages(struct amd_iommu *iommu,
 				  gfp_t gfp, size_t size);
+struct iommu_dev_data *amd_iommu_search_dev_data(struct amd_iommu *iommu,
+						 u16 devid);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index d09ce37cd3e9..8fd5798a5992 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -248,6 +248,14 @@
 #define PPR_ENTRY_SIZE		16
 #define PPR_LOG_SIZE		(PPR_ENTRY_SIZE * PPR_LOG_ENTRIES)
 
+/* PAGE_SERVICE_REQUEST PPR Log Buffer Entry flags */
+#define PPR_FLAG_EXEC		0x002	/* Execute permission requested */
+#define PPR_FLAG_READ		0x004	/* Read permission requested */
+#define PPR_FLAG_WRITE		0x020	/* Write permission requested */
+#define PPR_FLAG_US		0x040	/* 1: User, 0: Supervisor */
+#define PPR_FLAG_RVSD		0x080	/* Reserved bit not zero */
+#define PPR_FLAG_GN		0x100	/* GVA and PASID is valid */
+
 #define PPR_REQ_TYPE(x)		(((x) >> 60) & 0xfULL)
 #define PPR_FLAGS(x)		(((x) >> 48) & 0xfffULL)
 #define PPR_DEVID(x)		((x) & 0xffffULL)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 948f9bf47e68..7734e6bba624 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -201,7 +201,8 @@ static struct iommu_dev_data *alloc_dev_data(struct amd_iommu *iommu, u16 devid)
 	return dev_data;
 }
 
-static struct iommu_dev_data *search_dev_data(struct amd_iommu *iommu, u16 devid)
+struct iommu_dev_data *amd_iommu_search_dev_data(struct amd_iommu *iommu,
+						 u16 devid)
 {
 	struct iommu_dev_data *dev_data;
 	struct llist_node *node;
@@ -285,7 +286,7 @@ static struct iommu_dev_data *find_dev_data(struct amd_iommu *iommu, u16 devid)
 {
 	struct iommu_dev_data *dev_data;
 
-	dev_data = search_dev_data(iommu, devid);
+	dev_data = amd_iommu_search_dev_data(iommu, devid);
 
 	if (dev_data == NULL) {
 		dev_data = alloc_dev_data(iommu, devid);
@@ -3631,7 +3632,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info)
 	if (ir_data->iommu == NULL)
 		return -EINVAL;
 
-	dev_data = search_dev_data(ir_data->iommu, irte_info->devid);
+	dev_data = amd_iommu_search_dev_data(ir_data->iommu, irte_info->devid);
 
 	/* Note:
 	 * This device has never been set up for guest mode.
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index 2e5ca4847eda..db4fa4534cd6 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -59,6 +59,106 @@ void amd_iommu_restart_ppr_log(struct amd_iommu *iommu)
 			      MMIO_STATUS_PPR_OVERFLOW_MASK);
 }
 
+static inline u32 ppr_flag_to_fault_perm(u16 flag)
+{
+	int perm = 0;
+
+	if (flag & PPR_FLAG_READ)
+		perm |= IOMMU_FAULT_PERM_READ;
+	if (flag & PPR_FLAG_WRITE)
+		perm |= IOMMU_FAULT_PERM_WRITE;
+	if (flag & PPR_FLAG_EXEC)
+		perm |= IOMMU_FAULT_PERM_EXEC;
+	if (!(flag & PPR_FLAG_US))
+		perm |= IOMMU_FAULT_PERM_PRIV;
+
+	return perm;
+}
+
+static bool ppr_is_valid(struct amd_iommu *iommu, u64 *raw)
+{
+	struct device *dev = iommu->iommu.dev;
+	u16 devid = PPR_DEVID(raw[0]);
+
+	if (!(PPR_FLAGS(raw[0]) & PPR_FLAG_GN)) {
+		dev_warn(dev, "PPR logged [Request ignored due to GN=0 (device=%04x:%02x:%02x.%x "
+			 "pasid=0x%05llx address=0x%llx flags=0x%04llx tag=0x%03llx]\n",
+			 iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+			 PPR_PASID(raw[0]), raw[1], PPR_FLAGS(raw[0]), PPR_TAG(raw[0]));
+		return false;
+	}
+
+	if (PPR_FLAGS(raw[0]) & PPR_FLAG_RVSD) {
+		dev_warn(dev, "PPR logged [Invalid request format (device=%04x:%02x:%02x.%x "
+			 "pasid=0x%05llx address=0x%llx flags=0x%04llx tag=0x%03llx]\n",
+			 iommu->pci_seg->id, PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+			 PPR_PASID(raw[0]), raw[1], PPR_FLAGS(raw[0]), PPR_TAG(raw[0]));
+		return false;
+	}
+
+	return true;
+}
+
+static void iommu_call_iopf_notifier(struct amd_iommu *iommu, u64 *raw)
+{
+	struct iommu_fault_event event;
+	struct pci_dev *pdev;
+	int ret = -EINVAL;
+	u16 devid = PPR_DEVID(raw[0]);
+
+	if (PPR_REQ_TYPE(raw[0]) != PPR_REQ_FAULT) {
+		pr_err_ratelimited("Unknown PPR request received\n");
+		return;
+	}
+
+	if (!ppr_is_valid(iommu, raw))
+		goto out;
+
+	pdev = pci_get_domain_bus_and_slot(iommu->pci_seg->id, PCI_BUS_NUM(devid),
+					   devid & 0xff);
+	if (!pdev)
+		goto out;
+
+	memset(&event, 0, sizeof(struct iommu_fault_event));
+
+	event.fault.type = IOMMU_FAULT_PAGE_REQ;
+	event.fault.prm.perm = ppr_flag_to_fault_perm(PPR_FLAGS(raw[0]));
+	event.fault.prm.addr = (u64)(raw[1] & PAGE_MASK);
+	event.fault.prm.pasid = PPR_PASID(raw[0]);
+	event.fault.prm.grpid = PPR_TAG(raw[0]) & 0x1FF;
+
+	/*
+	 * PASID zero is used for requests from the I/O device without
+	 * a PASID
+	 */
+	if (event.fault.prm.pasid == 0 ||
+	    event.fault.prm.pasid >= pdev->dev.iommu->max_pasids) {
+		pr_info_ratelimited("Invalid PASID : 0x%x, device : 0x%x\n",
+				    event.fault.prm.pasid, pdev->dev.id);
+		goto out;
+	}
+
+
+	event.fault.prm.flags |= IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
+	event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+	if (PPR_TAG(raw[0]) & 0x200)
+		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
+
+	/* Submit event */
+	ret = iommu_report_device_fault(&pdev->dev, &event);
+
+out:
+	if (ret) {
+		/* Nobody cared, abort */
+		struct iommu_page_response resp = {
+			.pasid = PPR_PASID(raw[0]),
+			.grpid = PPR_TAG(raw[0]) & 0x1FF,
+			.code = IOMMU_PAGE_RESP_FAILURE,
+		};
+		amd_iommu_page_response(&pdev->dev, &event, &resp);
+	}
+}
+
 void amd_iommu_poll_ppr_log(struct amd_iommu *iommu)
 {
 	u32 head, tail;
@@ -104,7 +204,8 @@ void amd_iommu_poll_ppr_log(struct amd_iommu *iommu)
 		head = (head + PPR_ENTRY_SIZE) % PPR_LOG_SIZE;
 		writel(head, iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
 
-		/* TODO: PPR Handler will be added when we add IOPF support */
+		/* Handle PPR entry */
+		iommu_call_iopf_notifier(iommu, entry);
 
 		/* Refresh ring-buffer information */
 		head = readl(iommu->mmio_base + MMIO_PPR_HEAD_OFFSET);
-- 
2.31.1

[PATCH RESEND 10/10] iommu/amd: Introduce logic to enable/disable IOPF

[Vasant Hegde]

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

Add support to enable/disable IOPF feature. So that PRI capable device
can make use of page fault handler in SVA mode.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h |  2 ++
 drivers/iommu/amd/iommu.c     |  6 ++++
 drivers/iommu/amd/ppr.c       | 56 +++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index d2dfd7b6b6e2..7525abd64cf1 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -67,6 +67,8 @@ int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev);
 int amd_iommu_page_response(struct device *dev,
 			    struct iommu_fault_event *event,
 			    struct iommu_page_response *resp);
+int amd_iommu_iopf_enable(struct device *dev);
+int amd_iommu_iopf_disable(struct device *dev);
 
 struct amd_iommu *get_amd_iommu(unsigned int idx);
 u8 amd_iommu_pc_get_max_banks(unsigned int idx);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 7734e6bba624..aff47a5d3dd1 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2586,6 +2586,9 @@ static int amd_iommu_dev_enable_feature(struct device *dev,
 	case IOMMU_DEV_FEAT_SVA:
 		ret = amd_iommu_sva_enable(dev);
 		break;
+	case IOMMU_DEV_FEAT_IOPF:
+		ret = amd_iommu_iopf_enable(dev);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -2602,6 +2605,9 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
 	case IOMMU_DEV_FEAT_SVA:
 		ret = amd_iommu_sva_disable(dev);
 		break;
+	case IOMMU_DEV_FEAT_IOPF:
+		ret = amd_iommu_iopf_disable(dev);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
index db4fa4534cd6..8c197b768b4a 100644
--- a/drivers/iommu/amd/ppr.c
+++ b/drivers/iommu/amd/ppr.c
@@ -9,6 +9,7 @@
 #include <linux/amd-iommu.h>
 #include <linux/delay.h>
 #include <linux/mmu_notifier.h>
+#include <linux/pci-ats.h>
 
 #include "amd_iommu.h"
 #include "amd_iommu_types.h"
@@ -319,3 +320,58 @@ int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev)
 	raw_spin_unlock_irqrestore(&iommu->lock, flags);
 	return ret;
 }
+
+static int amd_iommu_iopf_update(struct device *dev, bool enable)
+{
+	unsigned long flags;
+	int ret;
+	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
+	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+	struct protection_domain *pdom = amd_iommu_get_pdomain(dev);
+
+	if (!pdev || !iommu || !dev_data)
+		return -EINVAL;
+
+	spin_lock_irqsave(&pdom->lock, flags);
+
+	if (enable) {
+		ret = amd_iommu_iopf_add_device(iommu, dev);
+		if (ret)
+			goto out;
+
+		dev_data->ppr = true;
+	} else {
+		ret = amd_iommu_iopf_remove_device(iommu, dev);
+		dev_data->ppr = false;
+	}
+
+	amd_iommu_domain_update(pdom);
+
+out:
+	spin_unlock_irqrestore(&pdom->lock, flags);
+	return ret;
+}
+
+int amd_iommu_iopf_enable(struct device *dev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	if (!(dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PRI_SUP))
+		return -ENODEV;
+
+	if (!dev_data->ats_enabled || !dev_data->pri_enabled)
+		return -EINVAL;
+
+	return amd_iommu_iopf_update(dev, true);
+}
+
+int amd_iommu_iopf_disable(struct device *dev)
+{
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
+
+	if (!dev_data->pri_enabled)
+		return -EINVAL;
+
+	return amd_iommu_iopf_update(dev, false);
+}
-- 
2.31.1

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe]

On Wed, Aug 23, 2023 at 02:04:08PM +0000, Vasant Hegde wrote:
> diff --git a/drivers/iommu/amd/sva.c b/drivers/iommu/amd/sva.c
> new file mode 100644
> index 000000000000..c7c7e7cb5414
> --- /dev/null
> +++ b/drivers/iommu/amd/sva.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + */
> +
> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
> +#define dev_fmt(fmt)    pr_fmt(fmt)
> +
> +#include <linux/iommu.h>
> +#include <linux/mm_types.h>
> +#include <linux/mmu_notifier.h>
> +
> +#include "amd_iommu.h"
> +#include "../iommu-sva.h"
> +
> +struct amd_sva_pasid {
> +	u32			pasid;	/* PASID index */
> +	struct mm_struct	*mm;	/* mm_struct for the faults */
> +	struct mmu_notifier	mn;	/* mmu_notifier handle */
> +	struct list_head	dev_list; /* List of devices for this pasid */
> +};
> +
> +struct amd_sva_dev {
> +	struct device		*dev;
> +	struct iommu_dev_data	*dev_data;
> +	struct list_head	list;
> +	struct rcu_head		rcu;
> +};
> +
> +static DEFINE_MUTEX(pasid_mutex);
> +static DEFINE_XARRAY_ALLOC(sva_pasid_array);
> +
> +
> +static int sva_pasid_private_add(u32 pasid, void *priv)
> +{
> +	return xa_alloc(&sva_pasid_array, &pasid, priv,
> +			XA_LIMIT(pasid, pasid), GFP_ATOMIC);
> +}
> +
> +static void sva_pasid_private_remove(u32 pasid)
> +{
> +	xa_erase(&sva_pasid_array, pasid);
> +}
> +
> +static void *sva_pasid_private_find(u32 pasid)
> +{
> +	return xa_load(&sva_pasid_array, pasid);
> +}

No PASID stuff in SVA code at all please, all of this is wrong.

The only PASID comes from here, and it should be the only place PASID
shows up:

 +int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
 +			    struct device *dev, ioasid_t pasid)
 +{

> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
> +{
> +	struct amd_sva_pasid *sva_pasid;
> +	struct amd_sva_dev *sva_dev;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	int ret = -EINVAL;
> +
> +	sva_dev = sva_dev_alloc(dev);
> +	if (!sva_dev)
> +		return ret;
> +
> +	sva_pasid = sva_pasid_private_find(mm->pasid);
> +	if (!sva_pasid) {
> +		sva_pasid = sva_pasid_alloc(mm);
> +		if (!sva_pasid)
> +			goto out_sva_dev;

No per-PASID struct. AMD enablement should go after this series:

 https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/

Put the mmu_notifier directly into the protection_domain. Assume you
have a single protection_domain per mm.

Also amd_sva_dev is not appropriate, the list of PASIDs (and masters)
a domain is associated with is part of the generic PASID support in
the protection_domain itself.

These details would be clearer if you start from enabling PASID
support for an UNMANAGED domain.

SVA should be a tiny incremental from that which simply calls the
same invalidation and manages the mmu notifier.

> +static struct amd_sva_dev *sva_dev_alloc(struct device *dev)
> +{
> +	struct amd_sva_dev *sva_dev;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	sva_dev = kzalloc(sizeof(*sva_dev), GFP_KERNEL);
> +	if (!sva_dev)
> +		return NULL;
> +
> +	sva_dev->dev = dev;
> +	sva_dev->dev_data = dev_data;
> +	init_rcu_head(&sva_dev->rcu);
> +
> +	return sva_dev;
> +}
> +
> +static inline void sva_dev_free(struct amd_sva_dev *sva_dev)
> +{
> +	kfree_rcu(sva_dev, rcu);
> +}
> +
> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
> +				    struct mm_struct *mm,
> +				    unsigned long start, unsigned long end)
> +{
> +	struct amd_sva_pasid *sva_pasid;
> +	struct amd_sva_dev *sva_dev;
> +
> +	rcu_read_lock();
> +
> +	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
> +	if (!sva_pasid) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
> +		if ((start ^ (end - 1)) < PAGE_SIZE)
> +			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
> +		else
> +			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
> +	}

SVA invalidation should be the same as normal PASID invalidation. You
need to track the list of PASIDs a protection_domain is associated
with in the protection domain itself, not in special SVA code. Don't
repeat the SMMU mistakes please.

Use container_of(mn) to get back to the SVA protection domain and then
you can access the protection domains list of PASIDs & masters.

> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
> +{
> +	struct amd_sva_pasid *sva_pasid;
> +	struct amd_sva_dev *sva_dev;
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	int ret = -EINVAL;
> +
> +	sva_dev = sva_dev_alloc(dev);
> +	if (!sva_dev)
> +		return ret;
> +
> +	sva_pasid = sva_pasid_private_find(mm->pasid);
> +	if (!sva_pasid) {
> +		sva_pasid = sva_pasid_alloc(mm);
> +		if (!sva_pasid)
> +			goto out_sva_dev;
> +
> +		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
> +		if (ret)
> +			goto out_sva_pasid;
> +
> +		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
> +					 iommu_virt_to_phys(sva_pasid->mm->pgd));
> +		if (ret)
> +			goto out_pasid_remove;
> +
> +		ret = mmu_notifier_register(&sva_pasid->mn, mm);
> +		if (ret)
> +			goto out_clear_gcr3;
> +	}

The mmu_notifier should be setup when the domain is allocated, not
during bind.

This is an issue we need to fix in the core code :(

> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
> +		return;

We should probably have the core code pass in the old domain to this
function, it is looking more like a mistake we didn't do that.

> +
> +	/* Get SVA domain */
> +	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
> +	if (!domain)
> +		return;
> +
> +	switch (domain->type) {
> +	case IOMMU_DOMAIN_SVA:
> +		/* Ensure that all queued faults have been processed */
> +		iopf_queue_flush_dev(dev);
> +
> +		mutex_lock(&pasid_mutex);
> +		sva_unbind_mm(dev, pasid);
> +		mutex_unlock(&pasid_mutex);

SVA should not be special for detach, this should all be generic code.

The mmu notifier is freed during SVA domain dealloc.

Jason

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Jason Gunthorpe]

On Wed, Aug 23, 2023 at 02:04:09PM +0000, Vasant Hegde wrote:

> +int amd_iommu_sva_enable(struct device *dev)
> +{
> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	if (!pdev || !iommu || !dev_data)
> +		return -EINVAL;
> +
> +	if (!amd_iommu_sva_supported())
> +		return -ENODEV;
> +
> +	if (!dev_data->pasid_enabled)
> +		return -EINVAL;
> +
> +	return amd_iommu_sva_gcr3_init(dev_data, dev->iommu->max_pasids);
> +}
> +
> +int amd_iommu_sva_disable(struct device *dev)
> +{
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	if (!iommu || !dev_data)
> +		return -EINVAL;
> +
> +	return amd_iommu_sva_gcr3_uninit(dev_data);
> +}

I think these features are a mistake, you need to implement them for
now but I wouldn't touch the gcr3 table, and disable should be a NOP.

gcr3 touching and enforcing should be part of generic PASID code and
be triggered naturally when the SVA domain is attached to a PASID.

Jason

Re: [PATCH RESEND 08/10] iommu/amd: Add support for add/remove device for IOPF

[Jason Gunthorpe]

On Wed, Aug 23, 2023 at 02:04:13PM +0000, Vasant Hegde wrote:

> diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
> index 8cbcdb9c1669..2e5ca4847eda 100644
> --- a/drivers/iommu/amd/ppr.c
> +++ b/drivers/iommu/amd/ppr.c
> @@ -168,3 +168,53 @@ int amd_iommu_page_response(struct device *dev,
>  	return amd_iommu_complete_ppr(pdev, resp->pasid, resp->code,
>  				      resp->grpid);
>  }
> +
> +int amd_iommu_iopf_add_device(struct amd_iommu *iommu, struct device *dev)
> +{
> +	unsigned long flags;
> +	int ret = -EINVAL;
> +
> +	raw_spin_lock_irqsave(&iommu->lock, flags);
> +
> +	if (!iommu->iopf_queue)
> +		goto out;
> +
> +	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
> +	if (ret)
> +		goto out;
> +
> +	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
> +	if (ret)
> +		iopf_queue_remove_device(iommu->iopf_queue, dev);
> +
> +out:
> +	raw_spin_unlock_irqrestore(&iommu->lock, flags);
> +	return ret;
> +}

Please assume the iopf rework will arrive before your series:

https://lore.kernel.org/linux-iommu/20230817234047.195194-1-baolu.lu@linux.intel.com/

In the new arrangement the work queue to is used by the SVA code to
move the blocking handle_mm_fault out of the global page request
processing.

I would expect there to be one (unbound) work_queue for SVA, shared by
all SVA domains.

Jason

Re: [PATCH RESEND 10/10] iommu/amd: Introduce logic to enable/disable IOPF

[Jason Gunthorpe]

On Wed, Aug 23, 2023 at 02:04:15PM +0000, Vasant Hegde wrote:
> diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
> index db4fa4534cd6..8c197b768b4a 100644
> --- a/drivers/iommu/amd/ppr.c
> +++ b/drivers/iommu/amd/ppr.c
> @@ -9,6 +9,7 @@
>  #include <linux/amd-iommu.h>
>  #include <linux/delay.h>
>  #include <linux/mmu_notifier.h>
> +#include <linux/pci-ats.h>
>  
>  #include "amd_iommu.h"
>  #include "amd_iommu_types.h"
> @@ -319,3 +320,58 @@ int amd_iommu_iopf_remove_device(struct amd_iommu *iommu, struct device *dev)
>  	raw_spin_unlock_irqrestore(&iommu->lock, flags);
>  	return ret;
>  }
> +
> +static int amd_iommu_iopf_update(struct device *dev, bool enable)
> +{
> +	unsigned long flags;
> +	int ret;
> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	struct protection_domain *pdom = amd_iommu_get_pdomain(dev);
> +
> +	if (!pdev || !iommu || !dev_data)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&pdom->lock, flags);
> +
> +	if (enable) {
> +		ret = amd_iommu_iopf_add_device(iommu, dev);
> +		if (ret)
> +			goto out;
> +
> +		dev_data->ppr = true;
> +	} else {
> +		ret = amd_iommu_iopf_remove_device(iommu, dev);
> +		dev_data->ppr = false;
> +	}
> +
> +	amd_iommu_domain_update(pdom);
> +
> +out:
> +	spin_unlock_irqrestore(&pdom->lock, flags);
> +	return ret;
> +}
> +
> +int amd_iommu_iopf_enable(struct device *dev)
> +{
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	if (!(dev_data->flags & AMD_IOMMU_DEVICE_FLAG_PRI_SUP))
> +		return -ENODEV;
> +
> +	if (!dev_data->ats_enabled || !dev_data->pri_enabled)
> +		return -EINVAL;
> +
> +	return amd_iommu_iopf_update(dev, true);
> +}
> +
> +int amd_iommu_iopf_disable(struct device *dev)
> +{
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +
> +	if (!dev_data->pri_enabled)
> +		return -EINVAL;
> +
> +	return amd_iommu_iopf_update(dev, false);
> +}

These features also look like a mistake..

PRI is enabled if the domain has an iopf handler, when that domain is
attached to the RID/PASID.

It remains enabled so long as any IOPF enabled domain is present on
the device.

So again, these should be checks but otherwise NOPs and any actual
working has to be pushed into the domain attach code.

Jason

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde]

Jason,


On 8/23/2023 7:58 PM, Jason Gunthorpe wrote:
> On Wed, Aug 23, 2023 at 02:04:08PM +0000, Vasant Hegde wrote:
>> diff --git a/drivers/iommu/amd/sva.c b/drivers/iommu/amd/sva.c
>> new file mode 100644
>> index 000000000000..c7c7e7cb5414
>> --- /dev/null
>> +++ b/drivers/iommu/amd/sva.c
>> @@ -0,0 +1,299 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
>> +#define dev_fmt(fmt)    pr_fmt(fmt)
>> +
>> +#include <linux/iommu.h>
>> +#include <linux/mm_types.h>
>> +#include <linux/mmu_notifier.h>
>> +
>> +#include "amd_iommu.h"
>> +#include "../iommu-sva.h"
>> +
>> +struct amd_sva_pasid {
>> +	u32			pasid;	/* PASID index */
>> +	struct mm_struct	*mm;	/* mm_struct for the faults */
>> +	struct mmu_notifier	mn;	/* mmu_notifier handle */
>> +	struct list_head	dev_list; /* List of devices for this pasid */
>> +};
>> +
>> +struct amd_sva_dev {
>> +	struct device		*dev;
>> +	struct iommu_dev_data	*dev_data;
>> +	struct list_head	list;
>> +	struct rcu_head		rcu;
>> +};
>> +
>> +static DEFINE_MUTEX(pasid_mutex);
>> +static DEFINE_XARRAY_ALLOC(sva_pasid_array);
>> +
>> +
>> +static int sva_pasid_private_add(u32 pasid, void *priv)
>> +{
>> +	return xa_alloc(&sva_pasid_array, &pasid, priv,
>> +			XA_LIMIT(pasid, pasid), GFP_ATOMIC);
>> +}
>> +
>> +static void sva_pasid_private_remove(u32 pasid)
>> +{
>> +	xa_erase(&sva_pasid_array, pasid);
>> +}
>> +
>> +static void *sva_pasid_private_find(u32 pasid)
>> +{
>> +	return xa_load(&sva_pasid_array, pasid);
>> +}
> 
> No PASID stuff in SVA code at all please, all of this is wrong.

This is based on upstream code!

> 
> The only PASID comes from here, and it should be the only place PASID
> shows up:
> 
>  +int amd_iommu_set_dev_pasid(struct iommu_domain *domain,
>  +			    struct device *dev, ioasid_t pasid)
>  +{
> 
>> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
>> +{
>> +	struct amd_sva_pasid *sva_pasid;
>> +	struct amd_sva_dev *sva_dev;
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +	int ret = -EINVAL;
>> +
>> +	sva_dev = sva_dev_alloc(dev);
>> +	if (!sva_dev)
>> +		return ret;
>> +
>> +	sva_pasid = sva_pasid_private_find(mm->pasid);
>> +	if (!sva_pasid) {
>> +		sva_pasid = sva_pasid_alloc(mm);
>> +		if (!sva_pasid)
>> +			goto out_sva_dev;
> 
> No per-PASID struct. AMD enablement should go after this series:
> 
>  https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/
> 
> Put the mmu_notifier directly into the protection_domain. Assume you
> have a single protection_domain per mm.

Actually we have protection domain for each IOMMU group (or multiple group in
case of VM). Our invalidation needs device protection domain ID (not the SVA
domain ID which is created during device/PASID binding).

Invalidation takes three parameter :
  Device Domain ID, PASID, Address

> 
> Also amd_sva_dev is not appropriate, the list of PASIDs (and masters)
> a domain is associated with is part of the generic PASID support in
> the protection_domain itself.

You mean to say, we maintain PASID list in device protection_domain and then in
invalidation path (somehow) we retrieve protection domain and use it for
invalidation? -OR- someway establish link between SVA domain to base device
protection domain?


> 
> These details would be clearer if you start from enabling PASID
> support for an UNMANAGED domain.

We don't support PASID with V1 page table.

> 
> SVA should be a tiny incremental from that which simply calls the
> same invalidation and manages the mmu notifier.

Yeah. If we can get a way to retrieve device protection domain ID (not SVA
protection domain) in notifier path then it makes life easy. But currently I
don't see a way to do that.


> 
>> +static struct amd_sva_dev *sva_dev_alloc(struct device *dev)
>> +{
>> +	struct amd_sva_dev *sva_dev;
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +
>> +	sva_dev = kzalloc(sizeof(*sva_dev), GFP_KERNEL);
>> +	if (!sva_dev)
>> +		return NULL;
>> +
>> +	sva_dev->dev = dev;
>> +	sva_dev->dev_data = dev_data;
>> +	init_rcu_head(&sva_dev->rcu);
>> +
>> +	return sva_dev;
>> +}
>> +
>> +static inline void sva_dev_free(struct amd_sva_dev *sva_dev)
>> +{
>> +	kfree_rcu(sva_dev, rcu);
>> +}
>> +
>> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
>> +				    struct mm_struct *mm,
>> +				    unsigned long start, unsigned long end)
>> +{
>> +	struct amd_sva_pasid *sva_pasid;
>> +	struct amd_sva_dev *sva_dev;
>> +
>> +	rcu_read_lock();
>> +
>> +	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
>> +	if (!sva_pasid) {
>> +		rcu_read_unlock();
>> +		return;
>> +	}
>> +
>> +	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
>> +		if ((start ^ (end - 1)) < PAGE_SIZE)
>> +			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
>> +		else
>> +			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
>> +	}
> 
> SVA invalidation should be the same as normal PASID invalidation. You

Its same (Currently we have two different functions based on PAGE_SIZE. We have
a separate series to improve our invalidation logic).

> need to track the list of PASIDs a protection_domain is associated
> with in the protection domain itself, not in special SVA code. Don't
> repeat the SMMU mistakes please.
> 
> Use container_of(mn) to get back to the SVA protection domain and then
> you can access the protection domains list of PASIDs & masters.

I don't think I understood this. Even with Tina's patch series, we can get the
SVA domain list. But we don't have a way to get device base protection domain.

> 
>> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
>> +{
>> +	struct amd_sva_pasid *sva_pasid;
>> +	struct amd_sva_dev *sva_dev;
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +	int ret = -EINVAL;
>> +
>> +	sva_dev = sva_dev_alloc(dev);
>> +	if (!sva_dev)
>> +		return ret;
>> +
>> +	sva_pasid = sva_pasid_private_find(mm->pasid);
>> +	if (!sva_pasid) {
>> +		sva_pasid = sva_pasid_alloc(mm);
>> +		if (!sva_pasid)
>> +			goto out_sva_dev;
>> +
>> +		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
>> +		if (ret)
>> +			goto out_sva_pasid;
>> +
>> +		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
>> +					 iommu_virt_to_phys(sva_pasid->mm->pgd));
>> +		if (ret)
>> +			goto out_pasid_remove;
>> +
>> +		ret = mmu_notifier_register(&sva_pasid->mn, mm);
>> +		if (ret)
>> +			goto out_clear_gcr3;
>> +	}
> 
> The mmu_notifier should be setup when the domain is allocated, not
> during bind.

We need to program (at least with AMD IOMMU) device PASID table before it can
handle the invalidation notifications. Not sure how it will work if we move mmu
notifier setup to domain allocation path.


> 
> This is an issue we need to fix in the core code :(
> 
>> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>> +{
>> +	struct iommu_domain *domain;
>> +
>> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
>> +		return;
> 
> We should probably have the core code pass in the old domain to this
> function, it is looking more like a mistake we didn't do that.

I don't think I understood this. Well, honestly I never understood why
remove_dev_pasid() is part of iommu_ops while set_dev_pasid is part of domain ops.

My understanding is in this path we necessary things like in AMD case updating
device table entry and finally remove mmu notifier.


-Vasant

> 
>> +
>> +	/* Get SVA domain */
>> +	domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
>> +	if (!domain)
>> +		return;
>> +
>> +	switch (domain->type) {
>> +	case IOMMU_DOMAIN_SVA:
>> +		/* Ensure that all queued faults have been processed */
>> +		iopf_queue_flush_dev(dev);
>> +
>> +		mutex_lock(&pasid_mutex);
>> +		sva_unbind_mm(dev, pasid);
>> +		mutex_unlock(&pasid_mutex);
> 
> SVA should not be special for detach, this should all be generic code.
> 
> The mmu notifier is freed during SVA domain dealloc.
> 
> Jason

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Vasant Hegde]

Jason,


On 8/23/2023 8:58 PM, Jason Gunthorpe wrote:
> On Wed, Aug 23, 2023 at 02:04:09PM +0000, Vasant Hegde wrote:
> 
>> +int amd_iommu_sva_enable(struct device *dev)
>> +{
>> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +
>> +	if (!pdev || !iommu || !dev_data)
>> +		return -EINVAL;
>> +
>> +	if (!amd_iommu_sva_supported())
>> +		return -ENODEV;
>> +
>> +	if (!dev_data->pasid_enabled)
>> +		return -EINVAL;
>> +
>> +	return amd_iommu_sva_gcr3_init(dev_data, dev->iommu->max_pasids);
>> +}
>> +
>> +int amd_iommu_sva_disable(struct device *dev)
>> +{
>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> +
>> +	if (!iommu || !dev_data)
>> +		return -EINVAL;
>> +
>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
>> +}
> 
> I think these features are a mistake, you need to implement them for
> now but I wouldn't touch the gcr3 table, and disable should be a NOP.

We need to configure Device Table Entry when we enable/disable SVA.
Also based on base domain type we do adjust the GCR3 table.

-Vasant

> 
> gcr3 touching and enforcing should be part of generic PASID code and
> be triggered naturally when the SVA domain is attached to a PASID.
> 
> Jason

Re: [PATCH RESEND 08/10] iommu/amd: Add support for add/remove device for IOPF

[Vasant Hegde]

Jason,

On 8/23/2023 9:03 PM, Jason Gunthorpe wrote:
> On Wed, Aug 23, 2023 at 02:04:13PM +0000, Vasant Hegde wrote:
> 
>> diff --git a/drivers/iommu/amd/ppr.c b/drivers/iommu/amd/ppr.c
>> index 8cbcdb9c1669..2e5ca4847eda 100644
>> --- a/drivers/iommu/amd/ppr.c
>> +++ b/drivers/iommu/amd/ppr.c
>> @@ -168,3 +168,53 @@ int amd_iommu_page_response(struct device *dev,
>>  	return amd_iommu_complete_ppr(pdev, resp->pasid, resp->code,
>>  				      resp->grpid);
>>  }
>> +
>> +int amd_iommu_iopf_add_device(struct amd_iommu *iommu, struct device *dev)
>> +{
>> +	unsigned long flags;
>> +	int ret = -EINVAL;
>> +
>> +	raw_spin_lock_irqsave(&iommu->lock, flags);
>> +
>> +	if (!iommu->iopf_queue)
>> +		goto out;
>> +
>> +	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
>> +	if (ret)
>> +		iopf_queue_remove_device(iommu->iopf_queue, dev);
>> +
>> +out:
>> +	raw_spin_unlock_irqrestore(&iommu->lock, flags);
>> +	return ret;
>> +}
> 
> Please assume the iopf rework will arrive before your series:
> 
> https://lore.kernel.org/linux-iommu/20230817234047.195194-1-baolu.lu@linux.intel.com/

Sure. I will rebase/rework patches on top of this series.

-Vasant

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe]

On Mon, Aug 28, 2023 at 04:09:16PM +0530, Vasant Hegde wrote:

> >> +static void *sva_pasid_private_find(u32 pasid)
> >> +{
> >> +	return xa_load(&sva_pasid_array, pasid);
> >> +}
> > 
> > No PASID stuff in SVA code at all please, all of this is wrong.
> 
> This is based on upstream code!

Yes, and we are changing all of this because how wrong the drivers
went with stuff like this. :(

> > No per-PASID struct. AMD enablement should go after this series:
> > 
> >  https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/
> > 
> > Put the mmu_notifier directly into the protection_domain. Assume you
> > have a single protection_domain per mm.
> 
> Actually we have protection domain for each IOMMU group (or multiple group in
> case of VM). 

Why would you do that for SVA? That is not the direction we are going.
One iommu_domain per mm, managed by the core code.

Driver assumes this and optimizes based on it.

If driver needs per device/pasid/whatever data then it keeps only that
data in a list hanging off the single iommu_domain.

It should be *exactly the same* data that an unmanaged domain needs to
manage its invalidations and ATC.

> > Also amd_sva_dev is not appropriate, the list of PASIDs (and masters)
> > a domain is associated with is part of the generic PASID support in
> > the protection_domain itself.
> 
> You mean to say, we maintain PASID list in device protection_domain
> and then in

yes, a linked list of devices RID and PASIDs that the domain is
attached to.

This is mandatory to issue any form of invalidation, an UNMANAGED
domains should use the same list, and same mechanism to invalidate
their PASID attachments as well.

> invalidation path (somehow) we retrieve protection domain and use it
> for

Yes, obtain the domain trivially via container_of(mmu_notifier). Store
the notifier in the SVA iommu_domain's driver struct
(protection_domain) to do this.

This is why the core code helps the driver by de-duplicating the SVA
domains, it can assume the iommu_domain is already minimal and it can
then safely place the notifier there. The drivers should not try to
de-duplicate the notifier with refcounting/etc.

> > These details would be clearer if you start from enabling PASID
> > support for an UNMANAGED domain.
> 
> We don't support PASID with V1 page table.

I didn't say V1 page table, I said enabling PASID support for
UNMANGED domains. This means you need a flavour of UNMANAGED domain
that is V2 page table. Just like ARM does. You already have this
support in the driver.

> >> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
> >> +				    struct mm_struct *mm,
> >> +				    unsigned long start, unsigned long end)
> >> +{
> >> +	struct amd_sva_pasid *sva_pasid;
> >> +	struct amd_sva_dev *sva_dev;
> >> +
> >> +	rcu_read_lock();
> >> +
> >> +	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
> >> +	if (!sva_pasid) {
> >> +		rcu_read_unlock();
> >> +		return;
> >> +	}
> >> +
> >> +	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
> >> +		if ((start ^ (end - 1)) < PAGE_SIZE)
> >> +			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
> >> +		else
> >> +			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
> >> +	}
> > 
> > SVA invalidation should be the same as normal PASID invalidation. You
> 
> Its same (Currently we have two different functions based on PAGE_SIZE. We have
> a separate series to improve our invalidation logic).

It is not the same, you have this weird sva_pasid thing in here. PASID
is NOT part of the SVA layer.

The API expects UNMANAGED domains will support PASID attach as well,
that is a significant use case.

> > Use container_of(mn) to get back to the SVA protection domain and then
> > you can access the protection domains list of PASIDs & masters.
> 
> I don't think I understood this. Even with Tina's patch series, we can get the
> SVA domain list. But we don't have a way to get device base protection domain.

Tina's patch series allows you to place the mmu_notifier struct
directly in the protection_domain struct.

When you get the invalidate() callback you go back to the
iommu_domain/protection domain that is affiliated with this MM.

From there you have all the information you can possibly need:

 - Global information in the protect_domain shared by all the devices
   (eg ARM uses this to put the shared cache tag)

 - Per attachment information (RID & PASID/etc) for each attachment.
   Iterate over this list to get the PASID/etc.

> >> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
> >> +{
> >> +	struct amd_sva_pasid *sva_pasid;
> >> +	struct amd_sva_dev *sva_dev;
> >> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >> +	int ret = -EINVAL;
> >> +
> >> +	sva_dev = sva_dev_alloc(dev);
> >> +	if (!sva_dev)
> >> +		return ret;
> >> +
> >> +	sva_pasid = sva_pasid_private_find(mm->pasid);
> >> +	if (!sva_pasid) {
> >> +		sva_pasid = sva_pasid_alloc(mm);
> >> +		if (!sva_pasid)
> >> +			goto out_sva_dev;
> >> +
> >> +		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
> >> +		if (ret)
> >> +			goto out_sva_pasid;
> >> +
> >> +		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
> >> +					 iommu_virt_to_phys(sva_pasid->mm->pgd));
> >> +		if (ret)
> >> +			goto out_pasid_remove;
> >> +
> >> +		ret = mmu_notifier_register(&sva_pasid->mn, mm);
> >> +		if (ret)
> >> +			goto out_clear_gcr3;
> >> +	}
> > 
> > The mmu_notifier should be setup when the domain is allocated, not
> > during bind.
> 
> We need to program (at least with AMD IOMMU) device PASID table before it can
> handle the invalidation notifications. Not sure how it will work if we move mmu
> notifier setup to domain allocation path.

The device list will be empty so there will be no PASIDs to
invalidate at this point. There will be seperate locking to protect
the device list, ensure that locking properly covers populating the
device list and setting up the HW to respond to the PASID.

> >> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >> +{
> >> +	struct iommu_domain *domain;
> >> +
> >> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
> >> +		return;
> > 
> > We should probably have the core code pass in the old domain to this
> > function, it is looking more like a mistake we didn't do that.
> 
> I don't think I understood this. Well, honestly I never understood why
> remove_dev_pasid() is part of iommu_ops while set_dev_pasid is part of domain ops.

Yeah, that is a bit weird, not sure there is a good reason

Regardless, the API should take in the current domain parmeter and
drivers shouldn't call the core to look it up in the xarray if that is
what drivers need to do.

> My understanding is in this path we necessary things like in AMD case updating
> device table entry and finally remove mmu notifier.

You should remove the RID&PASID from the device list and flush out the
HW caches under a lock.

mmu_notifier should be removed when the domain is deallocated.

Jason

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Jason Gunthorpe]

On Mon, Aug 28, 2023 at 04:15:45PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/23/2023 8:58 PM, Jason Gunthorpe wrote:
> > On Wed, Aug 23, 2023 at 02:04:09PM +0000, Vasant Hegde wrote:
> > 
> >> +int amd_iommu_sva_enable(struct device *dev)
> >> +{
> >> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
> >> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >> +
> >> +	if (!pdev || !iommu || !dev_data)
> >> +		return -EINVAL;
> >> +
> >> +	if (!amd_iommu_sva_supported())
> >> +		return -ENODEV;
> >> +
> >> +	if (!dev_data->pasid_enabled)
> >> +		return -EINVAL;
> >> +
> >> +	return amd_iommu_sva_gcr3_init(dev_data, dev->iommu->max_pasids);
> >> +}
> >> +
> >> +int amd_iommu_sva_disable(struct device *dev)
> >> +{
> >> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >> +
> >> +	if (!iommu || !dev_data)
> >> +		return -EINVAL;
> >> +
> >> +	return amd_iommu_sva_gcr3_uninit(dev_data);
> >> +}
> > 
> > I think these features are a mistake, you need to implement them for
> > now but I wouldn't touch the gcr3 table, and disable should be a NOP.
> 
> We need to configure Device Table Entry when we enable/disable SVA.

The DTE should only be changed by bind/unbind of RID/PASID - it has
nothing to do with these APIs.

Again, start with enabling native PASID support for UNAMANGED v2
domains and your SVA will make alot more sense and be alot cleaner.

Jason

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Vasant Hegde]

Jason,


On 8/30/2023 10:39 PM, Jason Gunthorpe wrote:
> On Mon, Aug 28, 2023 at 04:15:45PM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 8/23/2023 8:58 PM, Jason Gunthorpe wrote:
>>> On Wed, Aug 23, 2023 at 02:04:09PM +0000, Vasant Hegde wrote:
>>>
>>>> +int amd_iommu_sva_enable(struct device *dev)
>>>> +{
>>>> +	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
>>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>>> +
>>>> +	if (!pdev || !iommu || !dev_data)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (!amd_iommu_sva_supported())
>>>> +		return -ENODEV;
>>>> +
>>>> +	if (!dev_data->pasid_enabled)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return amd_iommu_sva_gcr3_init(dev_data, dev->iommu->max_pasids);
>>>> +}
>>>> +
>>>> +int amd_iommu_sva_disable(struct device *dev)
>>>> +{
>>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>>> +
>>>> +	if (!iommu || !dev_data)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
>>>> +}
>>>
>>> I think these features are a mistake, you need to implement them for
>>> now but I wouldn't touch the gcr3 table, and disable should be a NOP.
>>
>> We need to configure Device Table Entry when we enable/disable SVA.
> 
> The DTE should only be changed by bind/unbind of RID/PASID - it has
> nothing to do with these APIs.

Above functions are called during enable_feature(SVA) path. Not in PASID
bind/unbind path. In PASID bind/unbind path we just update PASID table.


> 
> Again, start with enabling native PASID support for UNAMANGED v2
> domains and your SVA will make alot more sense and be alot cleaner.

Currently, the IOMMU_DOMAIN_UNMANAGED is used by VFIO, which sets up the v1
table for GPA->SPA translation.

If we were to force the unmanaged domain to use the v2 table by default, we
would not be able to support nested translation, where it requires v1 table to
be managed by the host ( GPA->SPA) and v2 in the guest (GVA->GPA).

In this case, how would you handle this issue?

A solution might be to allow specifying which page table (v1 or v2) to use when
creating the unmanaged domain.


-Vasant

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Jason Gunthorpe]

On Thu, Aug 31, 2023 at 12:30:21AM +0530, Vasant Hegde wrote:
> >>>> +int amd_iommu_sva_disable(struct device *dev)
> >>>> +{
> >>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >>>> +
> >>>> +	if (!iommu || !dev_data)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
> >>>> +}
> >>>
> >>> I think these features are a mistake, you need to implement them for
> >>> now but I wouldn't touch the gcr3 table, and disable should be a NOP.
> >>
> >> We need to configure Device Table Entry when we enable/disable SVA.
> > 
> > The DTE should only be changed by bind/unbind of RID/PASID - it has
> > nothing to do with these APIs.
> 
> Above functions are called during enable_feature(SVA) path. Not in PASID
> bind/unbind path. In PASID bind/unbind path we just update PASID table.

I know, which is why they can't change anything about the IOMMU
HW. Only attach/bind/unbind should change HW state.

I intend to remove these APIs once ARM is fixed, so please don't start
to rely on them. The driver is doing something wrong if it is changing
HW setups outside of attach/bind/unbind.

These APIs are from an era before we had proper PASID API support and
no longer make sense.

> > Again, start with enabling native PASID support for UNAMANGED v2
> > domains and your SVA will make alot more sense and be alot cleaner.
> 
> Currently, the IOMMU_DOMAIN_UNMANAGED is used by VFIO, which sets up the v1
> table for GPA->SPA translation.

> If we were to force the unmanaged domain to use the v2 table by default, we
> would not be able to support nested translation, where it requires v1 table to
> be managed by the host ( GPA->SPA) and v2 in the guest (GVA->GPA).

I've said this so many times now

Upstream kernel does not support nested translation with VFIO. The
force v1 hack in the AMD driver *SHOULD NOT* be part of mainline.

I don't care about any unmerged out of tree patch sets that rely on it -
especially ones that ignored that the semi-upstream approved hack to do
this is VFIO_TYPE1_NESTING_IOMMU (which is also about to be removed),
not what you are talking about here.

iommufd has the upstream solution for this problem and it relies on
the iommufd user specifically asking for a nesting parent page table
which is the only signal that the AMD driver should use to force a v1
format on a PASID capable device.

Otherwise *all* requests for any kind of paging domain should use the
v2 if the device has PASID support.

Indeed after my other series is merged the AMD driver should be
converted to the alloc_domain_paging() interface that directly
prevents this abuse of the API.

Jason

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde]

Jason,


On 8/30/2023 10:37 PM, Jason Gunthorpe wrote:
> On Mon, Aug 28, 2023 at 04:09:16PM +0530, Vasant Hegde wrote:
> 
>>>> +static void *sva_pasid_private_find(u32 pasid)
>>>> +{
>>>> +	return xa_load(&sva_pasid_array, pasid);
>>>> +}
>>>
>>> No PASID stuff in SVA code at all please, all of this is wrong.
>>
>> This is based on upstream code!
> 
> Yes, and we are changing all of this because how wrong the drivers
> went with stuff like this. :(
> 
>>> No per-PASID struct. AMD enablement should go after this series:
>>>
>>>  https://lore.kernel.org/linux-iommu/20230808074944.7825-1-tina.zhang@intel.com/
>>>
>>> Put the mmu_notifier directly into the protection_domain. Assume you
>>> have a single protection_domain per mm.
>>
>> Actually we have protection domain for each IOMMU group (or multiple group in
>> case of VM). 
> 
> Why would you do that for SVA? That is not the direction we are going.
> One iommu_domain per mm, managed by the core code.
> 
> Driver assumes this and optimizes based on it.
> 
> If driver needs per device/pasid/whatever data then it keeps only that
> data in a list hanging off the single iommu_domain.
> 
> It should be *exactly the same* data that an unmanaged domain needs to
> manage its invalidations and ATC.
> 
>>> Also amd_sva_dev is not appropriate, the list of PASIDs (and masters)
>>> a domain is associated with is part of the generic PASID support in
>>> the protection_domain itself.
>>
>> You mean to say, we maintain PASID list in device protection_domain
>> and then in
> 
> yes, a linked list of devices RID and PASIDs that the domain is
> attached to.
> 
> This is mandatory to issue any form of invalidation, an UNMANAGED
> domains should use the same list, and same mechanism to invalidate
> their PASID attachments as well.
> 
>> invalidation path (somehow) we retrieve protection domain and use it
>> for
> 
> Yes, obtain the domain trivially via container_of(mmu_notifier). Store
> the notifier in the SVA iommu_domain's driver struct
> (protection_domain) to do this.

Reading through the discussion so far again and the other series, my
understanding is :
  - set_dev_pasid() will check the compatibility and bind device/pasid only if
its compatibility. In AMD case we will check against protection domain. Ex:
 If we have two devices (devA and devB) in two different protection domain then:
   set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
   set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
   Core will allocate new SVA domain (sva_domain_new)
   set_dev_pasid(sva_domain_new, devB, pasidX) - SUCCESS

  - We will track mmu notifier and other data required for invalidation in SVA
protection domain.

  - During invalidation, we will retrieve SVA protection domain using mmu
notifier. Use device protection domain which was tracked in this SVA domain for
invalidation.

Does above flow makes sense? I do have a code based on above flow. I will try to
post soon.



> 
> This is why the core code helps the driver by de-duplicating the SVA
> domains, it can assume the iommu_domain is already minimal and it can
> then safely place the notifier there. The drivers should not try to
> de-duplicate the notifier with refcounting/etc.
> 
>>> These details would be clearer if you start from enabling PASID
>>> support for an UNMANAGED domain.
>>
>> We don't support PASID with V1 page table.
> 
> I didn't say V1 page table, I said enabling PASID support for
> UNMANGED domains. This means you need a flavour of UNMANAGED domain
> that is V2 page table. Just like ARM does. You already have this
> support in the driver.
> 
>>>> +static void sva_mn_invalidate_range(struct mmu_notifier *mn,
>>>> +				    struct mm_struct *mm,
>>>> +				    unsigned long start, unsigned long end)
>>>> +{
>>>> +	struct amd_sva_pasid *sva_pasid;
>>>> +	struct amd_sva_dev *sva_dev;
>>>> +
>>>> +	rcu_read_lock();
>>>> +
>>>> +	sva_pasid = container_of(mn, struct amd_sva_pasid, mn);
>>>> +	if (!sva_pasid) {
>>>> +		rcu_read_unlock();
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	list_for_each_entry_rcu(sva_dev, &sva_pasid->dev_list, list) {
>>>> +		if ((start ^ (end - 1)) < PAGE_SIZE)
>>>> +			amd_iommu_flush_page(sva_dev->dev_data->domain, sva_pasid->pasid, start);
>>>> +		else
>>>> +			amd_iommu_flush_tlb(sva_dev->dev_data->domain, sva_pasid->pasid);
>>>> +	}
>>>
>>> SVA invalidation should be the same as normal PASID invalidation. You
>>
>> Its same (Currently we have two different functions based on PAGE_SIZE. We have
>> a separate series to improve our invalidation logic).
> 
> It is not the same, you have this weird sva_pasid thing in here. PASID
> is NOT part of the SVA layer.
> 
> The API expects UNMANAGED domains will support PASID attach as well,
> that is a significant use case.

Can you elaborate the use cases you are referring here?

We do have use cases for PASID and PASID+PRI. But I am not aware of any use case
for UNMANAGED domain.


> 
>>> Use container_of(mn) to get back to the SVA protection domain and then
>>> you can access the protection domains list of PASIDs & masters.
>>
>> I don't think I understood this. Even with Tina's patch series, we can get the
>> SVA domain list. But we don't have a way to get device base protection domain.
> 
> Tina's patch series allows you to place the mmu_notifier struct
> directly in the protection_domain struct.
> 
> When you get the invalidate() callback you go back to the
> iommu_domain/protection domain that is affiliated with this MM.
> 
> From there you have all the information you can possibly need:
> 
>  - Global information in the protect_domain shared by all the devices
>    (eg ARM uses this to put the shared cache tag)
> 
>  - Per attachment information (RID & PASID/etc) for each attachment.
>    Iterate over this list to get the PASID/etc.

Got it. Thanks.


> 
>>>> +static int sva_bind_mm(struct device *dev, struct mm_struct *mm)
>>>> +{
>>>> +	struct amd_sva_pasid *sva_pasid;
>>>> +	struct amd_sva_dev *sva_dev;
>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>>> +	int ret = -EINVAL;
>>>> +
>>>> +	sva_dev = sva_dev_alloc(dev);
>>>> +	if (!sva_dev)
>>>> +		return ret;
>>>> +
>>>> +	sva_pasid = sva_pasid_private_find(mm->pasid);
>>>> +	if (!sva_pasid) {
>>>> +		sva_pasid = sva_pasid_alloc(mm);
>>>> +		if (!sva_pasid)
>>>> +			goto out_sva_dev;
>>>> +
>>>> +		ret = sva_pasid_private_add(sva_pasid->pasid, sva_pasid);
>>>> +		if (ret)
>>>> +			goto out_sva_pasid;
>>>> +
>>>> +		ret = amd_iommu_set_gcr3(dev_data, sva_pasid->pasid,
>>>> +					 iommu_virt_to_phys(sva_pasid->mm->pgd));
>>>> +		if (ret)
>>>> +			goto out_pasid_remove;
>>>> +
>>>> +		ret = mmu_notifier_register(&sva_pasid->mn, mm);
>>>> +		if (ret)
>>>> +			goto out_clear_gcr3;
>>>> +	}
>>>
>>> The mmu_notifier should be setup when the domain is allocated, not
>>> during bind.
>>
>> We need to program (at least with AMD IOMMU) device PASID table before it can
>> handle the invalidation notifications. Not sure how it will work if we move mmu
>> notifier setup to domain allocation path.
> 
> The device list will be empty so there will be no PASIDs to
> invalidate at this point. There will be seperate locking to protect
> the device list, ensure that locking properly covers populating the
> device list and setting up the HW to respond to the PASID.
> 
>>>> +void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>>>> +{
>>>> +	struct iommu_domain *domain;
>>>> +
>>>> +	if (pasid == 0 || pasid >= dev->iommu->max_pasids)
>>>> +		return;
>>>
>>> We should probably have the core code pass in the old domain to this
>>> function, it is looking more like a mistake we didn't do that.
>>
>> I don't think I understood this. Well, honestly I never understood why
>> remove_dev_pasid() is part of iommu_ops while set_dev_pasid is part of domain ops.
> 
> Yeah, that is a bit weird, not sure there is a good reason
> 
> Regardless, the API should take in the current domain parmeter and
> drivers shouldn't call the core to look it up in the xarray if that is
> what drivers need to do.

Yeah. All I care is retrieving SVA domain. Using SVA domain I can get the
protection domain and do rest of the stuff.

With all new changes, it makes sense to pass sva domain to remove_dev_pasid.

-Vasant

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 11:48:31AM +0530, Vasant Hegde wrote:

> Reading through the discussion so far again and the other series, my
> understanding is :
>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
> its compatibility. In AMD case we will check against protection domain. Ex:
>  If we have two devices (devA and devB) in two different protection domain then:
>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
>    Core will allocate new SVA domain (sva_domain_new)
>    set_dev_pasid(sva_domain_new, devB, pasidX) - SUCCESS

I don't expect a compatability check to ever fail on an AMD driver -
what condition do you imagine where re-allocating a SVA domain will
make it work with a device?

>   - We will track mmu notifier and other data required for invalidation in SVA
> protection domain.

Yes
 
>   - During invalidation, we will retrieve SVA protection domain using mmu
> notifier. Use device protection domain which was tracked in this SVA domain for
> invalidation.

The iommu_domain/protection_domain must NOT be 1:1 with a device. The
driver must maintain a list of devices attached to the domain, and the
per-device-attachment parameters like PASID/cache tags/etc.

This is very important.

> > It is not the same, you have this weird sva_pasid thing in here. PASID
> > is NOT part of the SVA layer.
> > 
> > The API expects UNMANAGED domains will support PASID attach as well,
> > that is a significant use case.
> 
> Can you elaborate the use cases you are referring here?
> 
> We do have use cases for PASID and PASID+PRI. But I am not aware of any use case
> for UNMANAGED domain.

The iommu API is evolving so there are only a few domain types:
BLOCKED, IDENTITY, PAGING, SVA, OPAQUE

PAGING is what we today call UNMANAGED/DMA/DMA_FQ

PAGING domains need to support PASID+PRI, in today's language that
means UNMANAGED domains.

We have many use cases for PRI support with generic PAGING domains.

SVA is a special case of a PAGING domain where there is no map/unmap/invalidate
API and the IO page table comes from a mm_struct.

Start by making UNMANAGED (aka PAGING) domains work with PASID + PRI
and then SVA is a very small incremental step.

Jason

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde]

Jason,


On 9/5/2023 5:56 PM, Jason Gunthorpe wrote:
> On Tue, Sep 05, 2023 at 11:48:31AM +0530, Vasant Hegde wrote:
> 
>> Reading through the discussion so far again and the other series, my
>> understanding is :
>>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
>> its compatibility. In AMD case we will check against protection domain. Ex:
>>  If we have two devices (devA and devB) in two different protection domain then:
>>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
>>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail

Here I expect it to fails as devB is in different protection domain. From
invalidation point of view, devA and devB are not compatible. Hence I think it
should fail binding.


>>    Core will allocate new SVA domain (sva_domain_new)
>>    set_dev_pasid(sva_domain_new, devB, pasidX) - SUCCESS
> 
> I don't expect a compatability check to ever fail on an AMD driver -
> what condition do you imagine where re-allocating a SVA domain will
> make it work with a device?

In above scenario. Essentially we will have 1 x 1 mapping between SVA domain to
device protection domain (Here I am referring to the domain that we attached in
attach_device() path).



>>   - We will track mmu notifier and other data required for invalidation in SVA
>> protection domain.
> 
> Yes
>  
>>   - During invalidation, we will retrieve SVA protection domain using mmu
>> notifier. Use device protection domain which was tracked in this SVA domain for
>> invalidation.
> 
> The iommu_domain/protection_domain must NOT be 1:1 with a device. The
> driver must maintain a list of devices attached to the domain, and the
> per-device-attachment parameters like PASID/cache tags/etc.

We want to track SVA protection domain to device protection domain link. So that
invalidation becomes straight.
If we track dev/PASID then I am not sure how we can solve duplicate invalidation
issue that we have today (i. e. if we have two devices within same protection
domain and if we track dev/pasid combinatin, then will call invalidation twice).


> 
> This is very important.
> 
>>> It is not the same, you have this weird sva_pasid thing in here. PASID
>>> is NOT part of the SVA layer.
>>>
>>> The API expects UNMANAGED domains will support PASID attach as well,
>>> that is a significant use case.
>>
>> Can you elaborate the use cases you are referring here?
>>
>> We do have use cases for PASID and PASID+PRI. But I am not aware of any use case
>> for UNMANAGED domain.
> 
> The iommu API is evolving so there are only a few domain types:
> BLOCKED, IDENTITY, PAGING, SVA, OPAQUE
> 
> PAGING is what we today call UNMANAGED/DMA/DMA_FQ

Ok.


> 
> PAGING domains need to support PASID+PRI, in today's language that
> means UNMANAGED domains.
> 
> We have many use cases for PRI support with generic PAGING domains.

What is that usecase? That's what I am trying to understand it better.

> 
> SVA is a special case of a PAGING domain where there is no map/unmap/invalidate
> API and the IO page table comes from a mm_struct.
> 
> Start by making UNMANAGED (aka PAGING) domains work with PASID + PRI
> and then SVA is a very small incremental step.

If there is a real use case then why not? We can do that. But for now we want
the proper SVA to go first.

-Vasant

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 08:09:39PM +0530, Vasant Hegde wrote:
> >>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
> >> its compatibility. In AMD case we will check against protection domain. Ex:
> >>  If we have two devices (devA and devB) in two different protection domain then:
> >>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
> >>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
> 
> Here I expect it to fails as devB is in different protection domain. From
> invalidation point of view, devA and devB are not compatible. Hence I think it
> should fail binding.

No, that isn't the best thing to do - you could do this, but it will
be more inefficient.

The domain should represent the ioptes not the target of invalidation.

> >>   - During invalidation, we will retrieve SVA protection domain using mmu
> >> notifier. Use device protection domain which was tracked in this SVA domain for
> >> invalidation.
> > 
> > The iommu_domain/protection_domain must NOT be 1:1 with a device. The
> > driver must maintain a list of devices attached to the domain, and the
> > per-device-attachment parameters like PASID/cache tags/etc.
> 
> We want to track SVA protection domain to device protection domain link. So that
> invalidation becomes straight.
> If we track dev/PASID then I am not sure how we can solve duplicate invalidation
> issue that we have today (i. e. if we have two devices within same protection
> domain and if we track dev/pasid combinatin, then will call invalidation twice).

Look at what Michael is doing for SMMUv3, they have a per-smmu
instance cache tag and a per-device ATC invalidation they need to
issue. They keep a sorted list of device attachments and simply do one
invalidation per-smmu instance and one invalidation per entry.

It is pretty simple logic.

> > PAGING domains need to support PASID+PRI, in today's language that
> > means UNMANAGED domains.
> > 
> > We have many use cases for PRI support with generic PAGING domains.
> 
> What is that usecase? That's what I am trying to understand it better.

iommufd will require this for generic vSVA in all paths that don't use
nesting.

We have use cases to share a KVM page table with PRI.

Google apparently has some usecase since they are fixing it in ARM.

Besides that, it is the IOMMU API, drivers have to implement it, you
don't get to pick and choose.

> > SVA is a special case of a PAGING domain where there is no map/unmap/invalidate
> > API and the IO page table comes from a mm_struct.
> > 
> > Start by making UNMANAGED (aka PAGING) domains work with PASID + PRI
> > and then SVA is a very small incremental step.
> 
> If there is a real use case then why not? We can do that. But for now we want
> the proper SVA to go first.

You are not going to get SVA without also properly doing all
infrastructure to enable paging and unmanaged - I won't support
another shortcut hackjob for SVA that needs unwinding like ARM has.

PASID and PRI must not be tightly linked to SVA.
      
Jason

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Vasant Hegde]

Jason,


On 8/31/2023 5:16 AM, Jason Gunthorpe wrote:
> On Thu, Aug 31, 2023 at 12:30:21AM +0530, Vasant Hegde wrote:
>>>>>> +int amd_iommu_sva_disable(struct device *dev)
>>>>>> +{
>>>>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
>>>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>>>>> +
>>>>>> +	if (!iommu || !dev_data)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
>>>>>> +}
>>>>>
>>>>> I think these features are a mistake, you need to implement them for
>>>>> now but I wouldn't touch the gcr3 table, and disable should be a NOP.
>>>>
>>>> We need to configure Device Table Entry when we enable/disable SVA.
>>>
>>> The DTE should only be changed by bind/unbind of RID/PASID - it has
>>> nothing to do with these APIs.
>>
>> Above functions are called during enable_feature(SVA) path. Not in PASID
>> bind/unbind path. In PASID bind/unbind path we just update PASID table.
> 
> I know, which is why they can't change anything about the IOMMU
> HW. Only attach/bind/unbind should change HW state.
> 
> I intend to remove these APIs once ARM is fixed, so please don't start
> to rely on them. The driver is doing something wrong if it is changing
> HW setups outside of attach/bind/unbind.


If you intend to remove enable_feature(SVA) API completely, then you help us to
figure out how to handle below issues?

1) During attach_device() we don't know whether device is really going to use
SVA or not. So we want to start with minimal GCR3 table (that can handle just
PASID 0) and during enable_feature(SVA) we want to enable GCR3 table that can
handle max_pasids supported by a device. That way we can avoid performance
penalty if device is not using SVA (extra GCR3 table walk).

Note that to keep things simple current patchset configures max_pasids during
boot itself. But we will fix this once this series goes in.

2) If we boot with passthrough mode, then during attach_device() we are not
allocating GCR3 table. It will be allocated only when its needed. That way we
are not wasting memory.

Do you suggest that the driver setup GCR3 table at the first PASID binding to
the device?

-Vasant

Re: [PATCH RESEND 04/10] iommu/amd: Add support to enable/disable SVA feature

[Jason Gunthorpe]

On Thu, Sep 07, 2023 at 12:45:01PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/31/2023 5:16 AM, Jason Gunthorpe wrote:
> > On Thu, Aug 31, 2023 at 12:30:21AM +0530, Vasant Hegde wrote:
> >>>>>> +int amd_iommu_sva_disable(struct device *dev)
> >>>>>> +{
> >>>>>> +	struct amd_iommu *iommu = get_amd_iommu_from_dev(dev);
> >>>>>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> >>>>>> +
> >>>>>> +	if (!iommu || !dev_data)
> >>>>>> +		return -EINVAL;
> >>>>>> +
> >>>>>> +	return amd_iommu_sva_gcr3_uninit(dev_data);
> >>>>>> +}
> >>>>>
> >>>>> I think these features are a mistake, you need to implement them for
> >>>>> now but I wouldn't touch the gcr3 table, and disable should be a NOP.
> >>>>
> >>>> We need to configure Device Table Entry when we enable/disable SVA.
> >>>
> >>> The DTE should only be changed by bind/unbind of RID/PASID - it has
> >>> nothing to do with these APIs.
> >>
> >> Above functions are called during enable_feature(SVA) path. Not in PASID
> >> bind/unbind path. In PASID bind/unbind path we just update PASID table.
> > 
> > I know, which is why they can't change anything about the IOMMU
> > HW. Only attach/bind/unbind should change HW state.
> > 
> > I intend to remove these APIs once ARM is fixed, so please don't start
> > to rely on them. The driver is doing something wrong if it is changing
> > HW setups outside of attach/bind/unbind.
> 
> 
> If you intend to remove enable_feature(SVA) API completely, then you help us to
> figure out how to handle below issues?
> 
> 1) During attach_device() we don't know whether device is really going to use
> SVA or not. So we want to start with minimal GCR3 table (that can handle just
> PASID 0) and during enable_feature(SVA) we want to enable GCR3 table that can
> handle max_pasids supported by a device. That way we can avoid performance
> penalty if device is not using SVA (extra GCR3 table walk).

Resize the GCR3 table when the first pasid using attach happens.

We can discuss if/when it should be shrunk back to minimal.

> 2) If we boot with passthrough mode, then during attach_device() we are not
> allocating GCR3 table. It will be allocated only when its needed. That way we
> are not wasting memory.

> Do you suggest that the driver setup GCR3 table at the first PASID binding to
> the device?

Yes. PASID must work correctly stand alone.

Jason

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Vasant Hegde]

Jason,

On 9/5/2023 11:44 PM, Jason Gunthorpe wrote:
> On Tue, Sep 05, 2023 at 08:09:39PM +0530, Vasant Hegde wrote:
>>>>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
>>>> its compatibility. In AMD case we will check against protection domain. Ex:
>>>>  If we have two devices (devA and devB) in two different protection domain then:
>>>>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
>>>>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
>>
>> Here I expect it to fails as devB is in different protection domain. From
>> invalidation point of view, devA and devB are not compatible. Hence I think it
>> should fail binding.
> 
> No, that isn't the best thing to do - you could do this, but it will
> be more inefficient.

What inefficiency are you referring? Allocating extra SVA domain -OR- mmu
notifier getting called for each SVA domain -OR- something else?

> 
> The domain should represent the ioptes not the target of invalidation.

I agree with this and I have done SVA domain implementation for AMD driver in v2
[1].


Then I am wondering why not just create single SVA domain for a PASID and let
individual driver handle underneath requirement like target invalidation? Am I
missing here?


> 
>>>>   - During invalidation, we will retrieve SVA protection domain using mmu
>>>> notifier. Use device protection domain which was tracked in this SVA domain for
>>>> invalidation.
>>>
>>> The iommu_domain/protection_domain must NOT be 1:1 with a device. The
>>> driver must maintain a list of devices attached to the domain, and the
>>> per-device-attachment parameters like PASID/cache tags/etc.
>>
>> We want to track SVA protection domain to device protection domain link. So that
>> invalidation becomes straight.
>> If we track dev/PASID then I am not sure how we can solve duplicate invalidation
>> issue that we have today (i. e. if we have two devices within same protection
>> domain and if we track dev/pasid combinatin, then will call invalidation twice).
> 
> Look at what Michael is doing for SMMUv3, they have a per-smmu
> instance cache tag and a per-device ATC invalidation they need to
> issue. They keep a sorted list of device attachments and simply do one
> invalidation per-smmu instance and one invalidation per entry.
> 
> It is pretty simple logic.
> 
>>> PAGING domains need to support PASID+PRI, in today's language that
>>> means UNMANAGED domains.
>>>
>>> We have many use cases for PRI support with generic PAGING domains.
>>
>> What is that usecase? That's what I am trying to understand it better.
> 
> iommufd will require this for generic vSVA in all paths that don't use
> nesting.
> 
> We have use cases to share a KVM page table with PRI.
> 
> Google apparently has some usecase since they are fixing it in ARM.
> 
> Besides that, it is the IOMMU API, drivers have to implement it, you
> don't get to pick and choose.

Is there any documentation on the mentioned use case and the required IOMMU API
because this is new to me. I would like to understand things better before
making the change in AMD IOMMU driver.


> 
>>> SVA is a special case of a PAGING domain where there is no map/unmap/invalidate
>>> API and the IO page table comes from a mm_struct.
>>>
>>> Start by making UNMANAGED (aka PAGING) domains work with PASID + PRI
>>> and then SVA is a very small incremental step.
>>
>> If there is a real use case then why not? We can do that. But for now we want
>> the proper SVA to go first.
> 
> You are not going to get SVA without also properly doing all
> infrastructure to enable paging and unmanaged - I won't support
> another shortcut hackjob for SVA that needs unwinding like ARM has.

The goal of this series is to introduce the SVA support for AMD IOMMU driver.

So far we have been accommodating all the review comments.
  - Dropping iommu_v2 module before getting SVA
  - Reworking SVA top of Tina's series
  - Reworking IOPF on top of Baolu's IOPF enhancement

Adding PASID support for the unmanaged domain is a new requirement and not
relevant to this series. It will also require a considerable amount of changes
in AMD IOMMU driver that we need to further investigate to better understand.
For now, we would like to stay focus on getting SVA support in upstream before
moving on to add PASID support for UNMANAGED domain.


I just posted v2 [1]. Please take a look.

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


> 
> PASID and PRI must not be tightly linked to SVA.

Its not. Patchset already supports PASID and PASID+PRI combination.

-Vasant

Re: [PATCH RESEND 03/10] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe]

On Mon, Sep 11, 2023 at 05:46:39PM +0530, Vasant Hegde wrote:
> Jason,
> 
> On 9/5/2023 11:44 PM, Jason Gunthorpe wrote:
> > On Tue, Sep 05, 2023 at 08:09:39PM +0530, Vasant Hegde wrote:
> >>>>   - set_dev_pasid() will check the compatibility and bind device/pasid only if
> >>>> its compatibility. In AMD case we will check against protection domain. Ex:
> >>>>  If we have two devices (devA and devB) in two different protection domain then:
> >>>>    set_dev_pasid(sva_domain, devA, pasidX) - SUCCESS
> >>>>    set_dev_pasid(sva_domain, devB, pasidX) - Compatibility check fail
> >>
> >> Here I expect it to fails as devB is in different protection domain. From
> >> invalidation point of view, devA and devB are not compatible. Hence I think it
> >> should fail binding.
> > 
> > No, that isn't the best thing to do - you could do this, but it will
> > be more inefficient.
> 
> What inefficiency are you referring? Allocating extra SVA domain -OR- mmu
> notifier getting called for each SVA domain -OR- something else?

Primarily multiple mmu notifiers is not so great

> Then I am wondering why not just create single SVA domain for a PASID and let
> individual driver handle underneath requirement like target invalidation? Am I
> missing here?

domains are not connected to PASIDs - PASID is part of the attachment.

We want drivers to support one domain per IOPTE table and allow all
possible combinations of compatible attachments.

> > We have use cases to share a KVM page table with PRI.
> > 
> > Google apparently has some usecase since they are fixing it in ARM.
> > 
> > Besides that, it is the IOMMU API, drivers have to implement it, you
> > don't get to pick and choose.
> 
> Is there any documentation on the mentioned use case and the
> required IOMMU API because this is new to me. I would like to
> understand things better before making the change in AMD IOMMU
> driver.

Just the API definitions we have been working toward and the
dicussions on the list.

It is not a complex concept, UNMANAGED paging domains need to support
PRI and PASID.

> > You are not going to get SVA without also properly doing all
> > infrastructure to enable paging and unmanaged - I won't support
> > another shortcut hackjob for SVA that needs unwinding like ARM has.
> 
> The goal of this series is to introduce the SVA support for AMD IOMMU driver.
> 
> So far we have been accommodating all the review comments.
>   - Dropping iommu_v2 module before getting SVA
>   - Reworking SVA top of Tina's series
>   - Reworking IOPF on top of Baolu's IOPF enhancement

If you would prefer to have a more stable base to work on then wait a
few months until ARM and Intel drviers are fully fixed. Otherwise you
are going to have to keep pace with the advancing work.

> Adding PASID support for the unmanaged domain is a new requirement
> and not relevant to this series.

It is not. It is what the iommu API is designed to do, ARM got
grandfathered in with their half implementation because it already
existed. New drivers must implement the API properly and not take
shortcuts.

> It will also require a considerable amount of changes
> in AMD IOMMU driver that we need to further investigate to better
> understand.

Exactly. I do not want to see new drivers follow down the path of
ARM at exctly the same time we are trying to fix ARM to work properly!
This undermines all the progress we are making! The API has been
defined, please follow it!

On ARM unwinding the bad design without breaking SVA is proving very
hard after the fact. You even say it will be hard on AMD, so NO, do it
right to start with.

Every single thing you need to implement to support SVA is also needed
to support generic PRI+PASID. The request here is to layer the code
correctly so that the PASID and PRI bits are not linked to SVA.

You don't need to *test* PRI+PASID but you do need to layer the code
correctly so that it is trivial to implement.

Jason