[PATCH 0/5] iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3

[PATCH 0/5] iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3

[Shameer Kolothum]

Hi,

This is revisiting the earlier attempts [1, 2] to use SMMUv3 HTTU feature
for dirty page tracking. The Intel/AMD support is already mainline.

The code is now based on the new IOMMUFD APIs and is rebased on
Jason's SMMUv3 driver refactor series[3].

One major change is in patch #3 where the IO PTE walker code is modified
to take care of any iova size instead of iommu pgsize,

This is tested using a Qemu branch based on "vfio: Adopt iommufd" v4 series[4].

Basic sanity tests are done using an emulation setup and on a test hardware
setup. Block page split/merge(BBML) is not part of this series. I am planning
to send it separately.

Complete git branches are here,

https://github.com/hisilicon/kernel-dev/tree/smmuv3_newapi-dbm-arm
https://github.com/hisilicon/qemu/tree/iommufd_cdev_v4-dbm-arm

Please take a look and let me know your feedback.

Thanks,
Shameer
1. https://lore.kernel.org/lkml/20210413085457.25400-1-zhukeqian1@huawei.com/
2. https://lore.kernel.org/linux-iommu/20230518204650.14541-1-joao.m.martins@oracle.com/
3. https://lore.kernel.org/all/0-v2-de8b10590bf5+400-smmuv3_newapi_p1_jgg@nvidia.com/
4. https://lore.kernel.org/qemu-devel/20231102071302.1818071-1-zhenzhong.duan@intel.com/

Jean-Philippe Brucker (1):
  iommu/arm-smmu-v3: Add feature detection for HTTU

Joao Martins (2):
  iommu/arm-smmu-v3: Add set_dirty_tracking() support
  iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

Keqian Zhu (1):
  iommu/arm-smmu-v3: Add read_and_clear_dirty() support

Kunkun Jiang (1):
  iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 115 +++++++++++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   8 ++
 drivers/iommu/io-pgtable-arm.c              | 125 +++++++++++++++++++-
 include/linux/io-pgtable.h                  |   4 +
 4 files changed, 249 insertions(+), 3 deletions(-)

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] iommu/arm-smmu-v3: Add feature detection for HTTU

[Shameer Kolothum]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

If the SMMU supports it and the kernel was built with HTTU support,
Probe support for Hardware Translation Table Update (HTTU) which is
essentially to enable hardware update of access and dirty flags.

Probe and set the smmu::features for Hardware Dirty and Hardware Access
bits. This is in preparation, to enable it on the context descriptors of
stage 1 format.

Link: https://lore.kernel.org/lkml/20210413085457.25400-5-zhukeqian1@huawei.com/
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
[joaomart: Change commit message to reflect the underlying changes,
 the Link points to the original version this was based]
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 +++++++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  5 ++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 0075fbab1d2e..ce3559514af3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4072,6 +4072,28 @@ static void arm_smmu_device_iidr_probe(struct arm_smmu_device *smmu)
 	}
 }
 
+static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg)
+{
+	u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | ARM_SMMU_FEAT_HD);
+	u32 features = 0;
+
+	switch (FIELD_GET(IDR0_HTTU, reg)) {
+	case IDR0_HTTU_ACCESS_DIRTY:
+		features |= ARM_SMMU_FEAT_HD;
+		fallthrough;
+	case IDR0_HTTU_ACCESS:
+		features |= ARM_SMMU_FEAT_HA;
+	}
+
+	if (smmu->dev->of_node)
+		smmu->features |= features;
+	else if (features != fw_features)
+		/* ACPI IORT sets the HTTU bits */
+		dev_warn(smmu->dev,
+			 "IDR0.HTTU overridden by FW configuration (0x%x)\n",
+			 fw_features);
+}
+
 static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 {
 	u32 reg;
@@ -4132,6 +4154,8 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
 			smmu->features |= ARM_SMMU_FEAT_E2H;
 	}
 
+	arm_smmu_get_httu(smmu, reg);
+
 	/*
 	 * The coherency feature as set by FW is used in preference to the ID
 	 * register, but warn on mismatch.
@@ -4324,6 +4348,14 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
 	if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE)
 		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
 
+	switch (FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags)) {
+	case IDR0_HTTU_ACCESS_DIRTY:
+		smmu->features |= ARM_SMMU_FEAT_HD;
+		fallthrough;
+	case IDR0_HTTU_ACCESS:
+		smmu->features |= ARM_SMMU_FEAT_HA;
+	}
+
 	return 0;
 }
 #else
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 38cd65b79ac8..d49db234d13b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -33,6 +33,9 @@
 #define IDR0_ASID16			(1 << 12)
 #define IDR0_ATS			(1 << 10)
 #define IDR0_HYP			(1 << 9)
+#define IDR0_HTTU			GENMASK(7, 6)
+#define IDR0_HTTU_ACCESS		1
+#define IDR0_HTTU_ACCESS_DIRTY		2
 #define IDR0_COHACC			(1 << 4)
 #define IDR0_TTF			GENMASK(3, 2)
 #define IDR0_TTF_AARCH64		2
@@ -662,6 +665,8 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_SVA		(1 << 17)
 #define ARM_SMMU_FEAT_E2H		(1 << 18)
 #define ARM_SMMU_FEAT_NESTING		(1 << 19)
+#define ARM_SMMU_FEAT_HA		(1 << 20)
+#define ARM_SMMU_FEAT_HD		(1 << 21)
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

[Shameer Kolothum]

From: Kunkun Jiang <jiangkunkun@huawei.com>

As nested mode is not upstreamed now, we just aim to support dirty
log tracking for stage1 with io-pgtable mapping (means not support
SVA mapping). If HTTU is supported, we enable HA/HD bits in the SMMU
CD and transfer ARM_HD quirk to io-pgtable.

We additionally don't set HD|HA if not supportted.

Link: https://lore.kernel.org/lkml/20210413085457.25400-6-zhukeqian1@huawei.com/
Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
[joaomart:Convey HD|HA bits over to the context descriptor
 and update commit message; original in Link, where this is based on]
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +++
 drivers/iommu/io-pgtable-arm.c              | 11 +++++++++--
 include/linux/io-pgtable.h                  |  4 ++++
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index ce3559514af3..de4d07c4cc7f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1240,6 +1240,11 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
 		CTXDESC_CD_0_ASET |
 		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
 		);
+
+	if (pgtbl_cfg->quirks & IO_PGTABLE_QUIRK_ARM_HD)
+		target->data[0] |= cpu_to_le64(CTXDESC_CD_0_TCR_HA |
+					       CTXDESC_CD_0_TCR_HD);
+
 	target->data[1] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
 				      CTXDESC_CD_1_TTB0_MASK);
 	target->data[3] = cpu_to_le64(pgtbl_cfg->arm_lpae_s1_cfg.mair);
@@ -2219,6 +2224,13 @@ static const struct iommu_flush_ops arm_smmu_flush_ops = {
 	.tlb_add_page	= arm_smmu_tlb_inv_page_nosync,
 };
 
+static bool arm_smmu_dbm_capable(struct arm_smmu_device *smmu)
+{
+	u32 flags = (ARM_SMMU_FEAT_HD | ARM_SMMU_FEAT_COHERENCY);
+
+	return (smmu->features & flags) == flags;
+}
+
 /* IOMMU API */
 static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 {
@@ -2401,6 +2413,10 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 		.iommu_dev	= smmu->dev,
 	};
 
+	if (arm_smmu_dbm_capable(smmu) &&
+	    smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
+
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index d49db234d13b..21b9748c3d2c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -316,6 +316,9 @@ struct arm_smmu_cd {
 #define CTXDESC_CD_0_TCR_IPS		GENMASK_ULL(34, 32)
 #define CTXDESC_CD_0_TCR_TBI0		(1ULL << 38)
 
+#define CTXDESC_CD_0_TCR_HA            (1UL << 43)
+#define CTXDESC_CD_0_TCR_HD            (1UL << 42)
+
 #define CTXDESC_CD_0_AA64		(1UL << 41)
 #define CTXDESC_CD_0_S			(1UL << 44)
 #define CTXDESC_CD_0_R			(1UL << 45)
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 72dcdd468cf3..b2f470529459 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -75,6 +75,7 @@
 
 #define ARM_LPAE_PTE_NSTABLE		(((arm_lpae_iopte)1) << 63)
 #define ARM_LPAE_PTE_XN			(((arm_lpae_iopte)3) << 53)
+#define ARM_LPAE_PTE_DBM		(((arm_lpae_iopte)1) << 51)
 #define ARM_LPAE_PTE_AF			(((arm_lpae_iopte)1) << 10)
 #define ARM_LPAE_PTE_SH_NS		(((arm_lpae_iopte)0) << 8)
 #define ARM_LPAE_PTE_SH_OS		(((arm_lpae_iopte)2) << 8)
@@ -84,7 +85,7 @@
 
 #define ARM_LPAE_PTE_ATTR_LO_MASK	(((arm_lpae_iopte)0x3ff) << 2)
 /* Ignore the contiguous bit for block splitting */
-#define ARM_LPAE_PTE_ATTR_HI_MASK	(((arm_lpae_iopte)6) << 52)
+#define ARM_LPAE_PTE_ATTR_HI_MASK	(((arm_lpae_iopte)13) << 51)
 #define ARM_LPAE_PTE_ATTR_MASK		(ARM_LPAE_PTE_ATTR_LO_MASK |	\
 					 ARM_LPAE_PTE_ATTR_HI_MASK)
 /* Software bit for solving coherency races */
@@ -93,6 +94,9 @@
 /* Stage-1 PTE */
 #define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
 #define ARM_LPAE_PTE_AP_RDONLY		(((arm_lpae_iopte)2) << 6)
+#define ARM_LPAE_PTE_AP_RDONLY_BIT	7
+#define ARM_LPAE_PTE_AP_WRITABLE	(ARM_LPAE_PTE_AP_RDONLY | \
+					 ARM_LPAE_PTE_DBM)
 #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
 #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
 
@@ -407,6 +411,8 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		pte = ARM_LPAE_PTE_nG;
 		if (!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
 			pte |= ARM_LPAE_PTE_AP_RDONLY;
+		else if (data->iop.cfg.quirks & IO_PGTABLE_QUIRK_ARM_HD)
+			pte |= ARM_LPAE_PTE_AP_WRITABLE;
 		if (!(prot & IOMMU_PRIV))
 			pte |= ARM_LPAE_PTE_AP_UNPRIV;
 	} else {
@@ -804,7 +810,8 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, void *cookie)
 
 	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
 			    IO_PGTABLE_QUIRK_ARM_TTBR1 |
-			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA))
+			    IO_PGTABLE_QUIRK_ARM_OUTER_WBWA |
+			    IO_PGTABLE_QUIRK_ARM_HD))
 		return NULL;
 
 	data = arm_lpae_alloc_pgtable(cfg);
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 25142a0e2fc2..40e479766475 100644
--- a/include/linux/io-pgtable.h
+++ b/include/linux/io-pgtable.h
@@ -85,6 +85,8 @@ struct io_pgtable_cfg {
 	 *
 	 * IO_PGTABLE_QUIRK_ARM_OUTER_WBWA: Override the outer-cacheability
 	 *	attributes set in the TCR for a non-coherent page-table walker.
+	 *
+	 * IO_PGTABLE_QUIRK_ARM_HD: Enables dirty tracking in stage 1 pagetable.
 	 */
 	#define IO_PGTABLE_QUIRK_ARM_NS			BIT(0)
 	#define IO_PGTABLE_QUIRK_NO_PERMS		BIT(1)
@@ -92,6 +94,8 @@ struct io_pgtable_cfg {
 	#define IO_PGTABLE_QUIRK_ARM_MTK_TTBR_EXT	BIT(4)
 	#define IO_PGTABLE_QUIRK_ARM_TTBR1		BIT(5)
 	#define IO_PGTABLE_QUIRK_ARM_OUTER_WBWA		BIT(6)
+	#define IO_PGTABLE_QUIRK_ARM_HD			BIT(7)
+
 	unsigned long			quirks;
 	unsigned long			pgsize_bitmap;
 	unsigned int			ias;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] iommu/arm-smmu-v3: Add read_and_clear_dirty() support

[Shameer Kolothum]

From: Keqian Zhu <zhukeqian1@huawei.com>

.read_and_clear_dirty() IOMMU domain op takes care of reading the dirty
bits (i.e. PTE has both DBM and AP[2] set) and marshalling into a bitmap of
a given page size.

While reading the dirty bits we also clear the PTE AP[2] bit to mark it as
writable-clean depending on read_and_clear_dirty() flags.

Structure it in a way that the IOPTE walker is generic, and so we pass a
function pointer over what to do on a per-PTE basis.

[Link below points to the original version that was based on]

Link: https://lore.kernel.org/lkml/20210413085457.25400-11-zhukeqian1@huawei.com/
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> 
Co-developed-by: Kunkun Jiang <jiangkunkun@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
[joaomart: Massage commit message and code to the new IOMMU op and logic]
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
[Shameer: Changes to IOPTE walker function to handle size != iommu pgsize
 blk sizes]
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  27 +++++
 drivers/iommu/io-pgtable-arm.c              | 114 ++++++++++++++++++++
 2 files changed, 141 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index de4d07c4cc7f..8331a2c70a0c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -43,6 +43,7 @@ MODULE_PARM_DESC(disable_msipolling,
 	"Disable MSI-based polling for CMD_SYNC completion.");
 
 static struct iommu_ops arm_smmu_ops;
+static struct iommu_dirty_ops arm_smmu_dirty_ops;
 
 enum arm_smmu_msi_index {
 	EVTQ_MSI_INDEX,
@@ -3379,6 +3380,28 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
 	return group;
 }
 
+static int arm_smmu_read_and_clear_dirty(struct iommu_domain *domain,
+					 unsigned long iova, size_t size,
+					 unsigned long flags,
+					 struct iommu_dirty_bitmap *dirty)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+	int ret;
+
+	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
+		return -EINVAL;
+
+	if (!ops || !ops->read_and_clear_dirty) {
+		pr_err_once("io-pgtable don't support dirty tracking\n");
+		return -ENODEV;
+	}
+
+	ret = ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
+
+	return ret;
+}
+
 static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
 	return iommu_fwspec_add_ids(dev, args->args, 1);
@@ -3504,6 +3527,10 @@ static struct iommu_ops arm_smmu_ops = {
 	}
 };
 
+static struct iommu_dirty_ops arm_smmu_dirty_ops = {
+	.read_and_clear_dirty	= arm_smmu_read_and_clear_dirty,
+};
+
 /* Probing and initialisation functions */
 static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 				   struct arm_smmu_queue *q,
diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index b2f470529459..66b04c8ec693 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -717,6 +717,119 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
 	return iopte_to_paddr(pte, data) | iova;
 }
 
+struct arm_lpae_iopte_read_dirty {
+	unsigned long flags;
+	struct iommu_dirty_bitmap *dirty;
+};
+
+static int __arm_lpae_read_and_clear_dirty(unsigned long iova, size_t size,
+					   arm_lpae_iopte *ptep, void *opaque)
+{
+	struct arm_lpae_iopte_read_dirty *arg = opaque;
+	struct iommu_dirty_bitmap *dirty = arg->dirty;
+	arm_lpae_iopte pte;
+
+	pte = READ_ONCE(*ptep);
+	if (WARN_ON(!pte))
+		return -EINVAL;
+
+	if ((pte & ARM_LPAE_PTE_AP_WRITABLE) == ARM_LPAE_PTE_AP_WRITABLE)
+		return 0;
+
+	iommu_dirty_bitmap_record(dirty, iova, size);
+	if (!(arg->flags & IOMMU_DIRTY_NO_CLEAR))
+		set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT, (unsigned long *)ptep);
+	return 0;
+}
+
+static int __arm_lpae_iopte_walk(struct arm_lpae_io_pgtable *data,
+				 unsigned long iova, size_t size,
+				 int lvl, arm_lpae_iopte *ptep,
+				 int (*fn)(unsigned long iova, size_t size,
+					   arm_lpae_iopte *pte, void *opaque),
+				 void *opaque)
+{
+	struct io_pgtable *iop = &data->iop;
+	arm_lpae_iopte *base_ptep = ptep;
+	arm_lpae_iopte pte;
+	size_t base, next_size;
+	int ret;
+
+	if (WARN_ON_ONCE(!fn))
+		return -EINVAL;
+
+	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
+		return -EINVAL;
+
+	ptep += ARM_LPAE_LVL_IDX(iova, lvl, data);
+	pte = READ_ONCE(*ptep);
+	if (WARN_ON(!pte))
+		return -EINVAL;
+
+	if (size == ARM_LPAE_BLOCK_SIZE(lvl, data)) {
+		if (iopte_leaf(pte, lvl, iop->fmt))
+			return fn(iova, size, ptep, opaque);
+
+		/* Current level is table, traverse next level */
+		next_size = ARM_LPAE_BLOCK_SIZE(lvl + 1, data);
+		ptep = iopte_deref(pte, data);
+		for (base = 0; base < size; base += next_size) {
+			ret = __arm_lpae_iopte_walk(data, iova + base,
+						    next_size, lvl + 1, ptep,
+						    fn, opaque);
+			if (ret)
+				return ret;
+		}
+		return 0;
+	} else if (iopte_leaf(pte, lvl, iop->fmt)) {
+		size_t blk_size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+
+		for (base = 0; base < size; base += blk_size) {
+			ret = __arm_lpae_iopte_walk(data, iova + base,
+						    blk_size, lvl, base_ptep,
+						    fn, opaque);
+			if (ret)
+				return ret;
+		}
+		return 0;
+	}
+
+	/* Keep on walkin */
+	ptep = iopte_deref(pte, data);
+	return __arm_lpae_iopte_walk(data, iova, size, lvl + 1, ptep,
+				     fn, opaque);
+}
+
+static int arm_lpae_read_and_clear_dirty(struct io_pgtable_ops *ops,
+					 unsigned long iova, size_t size,
+					 unsigned long flags,
+					 struct iommu_dirty_bitmap *dirty)
+{
+	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
+	struct io_pgtable_cfg *cfg = &data->iop.cfg;
+	struct arm_lpae_iopte_read_dirty arg = {
+		.flags = flags, .dirty = dirty,
+	};
+	arm_lpae_iopte *ptep = data->pgd;
+	int lvl = data->start_level;
+	long iaext = (s64)iova >> cfg->ias;
+
+	if (WARN_ON(!size))
+		return -EINVAL;
+
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_TTBR1)
+		iaext = ~iaext;
+	if (WARN_ON(iaext))
+		return -EINVAL;
+
+	if (data->iop.fmt != ARM_64_LPAE_S1 &&
+	    data->iop.fmt != ARM_32_LPAE_S1)
+		return -EINVAL;
+
+	return __arm_lpae_iopte_walk(data, iova, size, lvl, ptep,
+				     __arm_lpae_read_and_clear_dirty, &arg);
+}
+
 static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
 {
 	unsigned long granule, page_sizes;
@@ -795,6 +908,7 @@ arm_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg)
 		.map_pages	= arm_lpae_map_pages,
 		.unmap_pages	= arm_lpae_unmap_pages,
 		.iova_to_phys	= arm_lpae_iova_to_phys,
+		.read_and_clear_dirty = arm_lpae_read_and_clear_dirty,
 	};
 
 	return data;
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Shameer Kolothum]

From: Joao Martins <joao.m.martins@oracle.com>

Dirty tracking will always be enabled with DBM=1 modifier enabled
by default when HD is supported.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 8331a2c70a0c..c5eabdc29ba5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3402,6 +3402,27 @@ static int arm_smmu_read_and_clear_dirty(struct iommu_domain *domain,
 	return ret;
 }
 
+static int arm_smmu_set_dirty_tracking(struct iommu_domain *domain,
+				       bool enabled)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
+
+	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
+		return -EINVAL;
+
+	if (!ops) {
+		pr_err_once("io-pgtable don't support dirty tracking\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Always enabled and the dirty bitmap is cleared prior to
+	 * set_dirty_tracking().
+	 */
+	return 0;
+}
+
 static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
 	return iommu_fwspec_add_ids(dev, args->args, 1);
@@ -3529,6 +3550,7 @@ static struct iommu_ops arm_smmu_ops = {
 
 static struct iommu_dirty_ops arm_smmu_dirty_ops = {
 	.read_and_clear_dirty	= arm_smmu_read_and_clear_dirty,
+	.set_dirty_tracking     = arm_smmu_set_dirty_tracking,
 };
 
 /* Probing and initialisation functions */
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

[Shameer Kolothum]

From: Joao Martins <joao.m.martins@oracle.com>

SMMUv3 implements all requirements of revoking device
attachment if smmu does not support dirty tracking.

Finally handle the IOMMU_CAP_DIRTY in iommu_capable for
IOMMUFD_DEVICE_GET_HW_INFO.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index c5eabdc29ba5..2c100f2136bc 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2244,6 +2244,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 	case IOMMU_CAP_NOEXEC:
 	case IOMMU_CAP_DEFERRED_FLUSH:
 		return true;
+	case IOMMU_CAP_DIRTY_TRACKING:
+		return arm_smmu_dbm_capable(master->smmu);
 	default:
 		return false;
 	}
@@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	master = dev_iommu_priv_get(dev);
 	smmu = master->smmu;
 
+	if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
+		return -EINVAL;
+
 	mutex_lock(&smmu_domain->init_mutex);
 
 	if (!smmu_domain->smmu) {
@@ -3077,7 +3082,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 			   const struct iommu_user_data *user_data)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-	const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT;
+	const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
+				 IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+	bool enforce_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	struct arm_smmu_domain *smmu_domain;
 	int ret;
 
@@ -3090,6 +3097,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 	if (user_data)
 		return ERR_PTR(-EINVAL);
 
+	if (enforce_dirty &&
+	    !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
+		return ERR_PTR(-EOPNOTSUPP);
+
 	smmu_domain = arm_smmu_domain_alloc();
 	if (!smmu_domain)
 		return ERR_PTR(-ENOMEM);
@@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 
 	smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
 	smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
+	if (enforce_dirty)
+		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;
+
 	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
 	if (ret)
 		goto err_free;
@@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg)
 
 	if (smmu->dev->of_node)
 		smmu->features |= features;
-	else if (features != fw_features)
+	else if (features != fw_features) {
 		/* ACPI IORT sets the HTTU bits */
 		dev_warn(smmu->dev,
-			 "IDR0.HTTU overridden by FW configuration (0x%x)\n",
+			 "IDR0.HTTU not overridden by FW configuration (0x%x)\n",
 			 fw_features);
+		smmu->features |= features;
+	}
 }
 
 static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

[Jason Gunthorpe]

On Tue, Nov 28, 2023 at 09:49:37AM +0000, Shameer Kolothum wrote:

>  /* IOMMU API */
>  static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
>  {
> @@ -2401,6 +2413,10 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  		.iommu_dev	= smmu->dev,
>  	};
>  
> +	if (arm_smmu_dbm_capable(smmu) &&
> +	    smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
> +		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
> +

This flow has become a bit wonky, the switch statement right above is
already checking S1 and partially initializing pgtbl_cfg.

I suggest moving the pgtbl_cfg init to above the switch and making the
switch store directly into pgtbl_cfg values and remove the stack ios
and oas values

Regards,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] iommu/arm-smmu-v3: Add read_and_clear_dirty() support

[Jason Gunthorpe]

On Tue, Nov 28, 2023 at 09:49:38AM +0000, Shameer Kolothum wrote:

> +static int arm_smmu_read_and_clear_dirty(struct iommu_domain *domain,
> +					 unsigned long iova, size_t size,
> +					 unsigned long flags,
> +					 struct iommu_dirty_bitmap *dirty)
> +{
> +	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> +	int ret;
> +
> +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> +		return -EINVAL;
> +
> +	if (!ops || !ops->read_and_clear_dirty) {
> +		pr_err_once("io-pgtable don't support dirty tracking\n");
> +		return -ENODEV;

All these prints concern me, either these can never happen due to
constraints on the caller, eg iommufd, in which case use WARN_ON_ONCe

Or iommufd does allow these improper things and thus it should
silently fail.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Jason Gunthorpe]

On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
> 
> Dirty tracking will always be enabled with DBM=1 modifier enabled
> by default when HD is supported.

Is this trying to say that ARM doesn't have a per-table global enable
for dirty tracking but instead pre-sets the DBM bit to avoid the cost?

So on smmuv3 to enable we have to clear everything and disable
continues to pay a penalty since we don't go and mark all things as
dirty again?

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

[Jason Gunthorpe]

On Tue, Nov 28, 2023 at 09:49:40AM +0000, Shameer Kolothum wrote:
> @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	master = dev_iommu_priv_get(dev);
>  	smmu = master->smmu;
>  
> +	if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
> +		return -EINVAL;
> +

This is not necessary, a domain can be attached to a single smmu and
finalize was run on that smmu already. So dirty ops should only be set
if this is a S1 domain finalized ona smmu that was dbm capable.

Otherwise none of this makes any sense.

> @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  
>  	smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
>  	smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
> +	if (enforce_dirty)
> +		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;

Ah, this seems in the wrong place, perhaps that is the confusion
everywhere?

If the finalize actually enables dirty tracking in the pgtbl_ops then
it should set the diryty_ops, they should not be set in alloc_user.

Specifically, a S2 domain should never have dirty_ops set.

IOW if domain.dirty_ops != NULL then pgtbl_ops != NULL && pgtbl_ops->read_and_clear_dirty

Thus no need to have all the other prints/etc then.

So I'd move this into finalize.

> @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg)
>  
>  	if (smmu->dev->of_node)
>  		smmu->features |= features;
> -	else if (features != fw_features)
> +	else if (features != fw_features) {
>  		/* ACPI IORT sets the HTTU bits */
>  		dev_warn(smmu->dev,
> -			 "IDR0.HTTU overridden by FW configuration (0x%x)\n",
> +			 "IDR0.HTTU not overridden by FW configuration (0x%x)\n",
>  			 fw_features);
> +		smmu->features |= features;
> +	}
>  }

Is this hunk misplaced?

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] iommu/arm-smmu-v3: Add feature detection for HTTU

[Jason Gunthorpe]

On Tue, Nov 28, 2023 at 09:49:36AM +0000, Shameer Kolothum wrote:
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> 
> If the SMMU supports it and the kernel was built with HTTU support,
> Probe support for Hardware Translation Table Update (HTTU) which is
> essentially to enable hardware update of access and dirty flags.
> 
> Probe and set the smmu::features for Hardware Dirty and Hardware Access
> bits. This is in preparation, to enable it on the context descriptors of
> stage 1 format.
> 
> Link: https://lore.kernel.org/lkml/20210413085457.25400-5-zhukeqian1@huawei.com/
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> [joaomart: Change commit message to reflect the underlying changes,
>  the Link points to the original version this was based]
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 +++++++++++++++++++++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  5 ++++
>  2 files changed, 37 insertions(+)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, November 29, 2023 7:43 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking()
> support
> 
> On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote:
> > From: Joao Martins <joao.m.martins@oracle.com>
> >
> > Dirty tracking will always be enabled with DBM=1 modifier enabled
> > by default when HD is supported.
> 
> Is this trying to say that ARM doesn't have a per-table global enable
> for dirty tracking but instead pre-sets the DBM bit to avoid the cost?
>

Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had
it walking the PTEs and setting the DBM on set_dirty_tracking(). But it
complicates things if we have to incorporates updating entries for any
new mappings.  And it was suggested to keep it enabled here,

https://lore.kernel.org/kvm/2d369e58-8ac0-f263-7b94-fe73917782e1@linux.intel.com/T/#m3be8a185668810fb24a030617f5eaf79a9a32748

> So on smmuv3 to enable we have to clear everything and disable
> continues to pay a penalty since we don't go and mark all things as
> dirty again?

Yes we clear everything on enable. Sorry I didn't get the second part.
We don't mark dirty on disable. How is that different from Intel/AMD?

Thanks,
Shameer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, November 29, 2023 7:49 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain
> attach/alloc
> 
> On Tue, Nov 28, 2023 at 09:49:40AM +0000, Shameer Kolothum wrote:
> > @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct
> iommu_domain *domain, struct device *dev)
> >  	master = dev_iommu_priv_get(dev);
> >  	smmu = master->smmu;
> >
> > +	if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
> > +		return -EINVAL;
> > +
> 
> This is not necessary, a domain can be attached to a single smmu and
> finalize was run on that smmu already. So dirty ops should only be set
> if this is a S1 domain finalized ona smmu that was dbm capable.
> 
> Otherwise none of this makes any sense.
> 
> > @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev,
> u32 flags,
> >
> >  	smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
> >  	smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
> > +	if (enforce_dirty)
> > +		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;
> 
> Ah, this seems in the wrong place, perhaps that is the confusion
> everywhere?
> 
> If the finalize actually enables dirty tracking in the pgtbl_ops then
> it should set the diryty_ops, they should not be set in alloc_user.
> 
> Specifically, a S2 domain should never have dirty_ops set.
> 
> IOW if domain.dirty_ops != NULL then pgtbl_ops != NULL && pgtbl_ops-
> >read_and_clear_dirty
> 
> Thus no need to have all the other prints/etc then.
> 
> So I'd move this into finalize.

Ok. Make sense. I will move it.

> 
> > @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct
> arm_smmu_device *smmu, u32 reg)
> >
> >  	if (smmu->dev->of_node)
> >  		smmu->features |= features;
> > -	else if (features != fw_features)
> > +	else if (features != fw_features) {
> >  		/* ACPI IORT sets the HTTU bits */
> >  		dev_warn(smmu->dev,
> > -			 "IDR0.HTTU overridden by FW configuration (0x%x)\n",
> > +			 "IDR0.HTTU not overridden by FW configuration
> (0x%x)\n",
> >  			 fw_features);
> > +		smmu->features |= features;
> > +	}
> >  }
> 
> Is this hunk misplaced?

Oops... My bad. Added for testing. 

Thanks,
Shameer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/5] iommu/arm-smmu-v3: Add read_and_clear_dirty() support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, November 29, 2023 7:35 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 3/5] iommu/arm-smmu-v3: Add read_and_clear_dirty()
> support
> 
> On Tue, Nov 28, 2023 at 09:49:38AM +0000, Shameer Kolothum wrote:
> 
> > +static int arm_smmu_read_and_clear_dirty(struct iommu_domain *domain,
> > +					 unsigned long iova, size_t size,
> > +					 unsigned long flags,
> > +					 struct iommu_dirty_bitmap *dirty)
> > +{
> > +	struct arm_smmu_domain *smmu_domain =
> to_smmu_domain(domain);
> > +	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
> > +	int ret;
> > +
> > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > +		return -EINVAL;
> > +
> > +	if (!ops || !ops->read_and_clear_dirty) {
> > +		pr_err_once("io-pgtable don't support dirty tracking\n");
> > +		return -ENODEV;
> 
> All these prints concern me, either these can never happen due to
> constraints on the caller, eg iommufd, in which case use WARN_ON_ONCe
> 
> Or iommufd does allow these improper things and thus it should
> silently fail.
> 
Ok. Will revisit these for next revision.

Thanks,
Shameer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/5] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-pgtable mapping

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, November 29, 2023 7:31 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 2/5] iommu/arm-smmu-v3: Enable HTTU for stage1 with io-
> pgtable mapping
> 
> On Tue, Nov 28, 2023 at 09:49:37AM +0000, Shameer Kolothum wrote:
> 
> >  /* IOMMU API */
> >  static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
> >  {
> > @@ -2401,6 +2413,10 @@ static int arm_smmu_domain_finalise(struct
> arm_smmu_domain *smmu_domain,
> >  		.iommu_dev	= smmu->dev,
> >  	};
> >
> > +	if (arm_smmu_dbm_capable(smmu) &&
> > +	    smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
> > +		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
> > +
> 
> This flow has become a bit wonky, the switch statement right above is
> already checking S1 and partially initializing pgtbl_cfg.
> 
> I suggest moving the pgtbl_cfg init to above the switch and making the
> switch store directly into pgtbl_cfg values and remove the stack ios
> and oas values
> 

Got it. Will do.

Thanks,
Shameer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Jason Gunthorpe]

On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi wrote:
> > On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote:
> > > From: Joao Martins <joao.m.martins@oracle.com>
> > >
> > > Dirty tracking will always be enabled with DBM=1 modifier enabled
> > > by default when HD is supported.
> > 
> > Is this trying to say that ARM doesn't have a per-table global enable
> > for dirty tracking but instead pre-sets the DBM bit to avoid the cost?
> >
> 
> Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had
> it walking the PTEs and setting the DBM on set_dirty_tracking().

set_dirty_tracking doesn't have access to the necessary locking to
touch the PTEs.

> > So on smmuv3 to enable we have to clear everything and disable
> > continues to pay a penalty since we don't go and mark all things as
> > dirty again?
> 
> Yes we clear everything on enable. Sorry I didn't get the second part.
> We don't mark dirty on disable. How is that different from Intel/AMD?

Intel/AMD have a global switch so they just turn off the tracking and
stop paying the cost.

This approach on ARM means once the tracking is logically turned off
the HW will continue to generate memory traffic to set dirty bits on
DMAs. There is no way to back to the at-start state where their is 0
memory traffic on DMAs. Not sure that it totally matters, but it is
worth noting someplace.

If we do want to solve this then ARM would need iommufd to make a pass
over the page table to set for disable similar to how we have to clear
for enable.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, November 30, 2023 12:54 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking()
> support
> 
> On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi
> wrote:
> > > On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote:
> > > > From: Joao Martins <joao.m.martins@oracle.com>
> > > >
> > > > Dirty tracking will always be enabled with DBM=1 modifier enabled
> > > > by default when HD is supported.
> > >
> > > Is this trying to say that ARM doesn't have a per-table global enable
> > > for dirty tracking but instead pre-sets the DBM bit to avoid the cost?
> > >
> >
> > Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had
> > it walking the PTEs and setting the DBM on set_dirty_tracking().
> 
> set_dirty_tracking doesn't have access to the necessary locking to
> touch the PTEs.
> 
> > > So on smmuv3 to enable we have to clear everything and disable
> > > continues to pay a penalty since we don't go and mark all things as
> > > dirty again?
> >
> > Yes we clear everything on enable. Sorry I didn't get the second part.
> > We don't mark dirty on disable. How is that different from Intel/AMD?
> 
> Intel/AMD have a global switch so they just turn off the tracking and
> stop paying the cost.
> 
> This approach on ARM means once the tracking is logically turned off
> the HW will continue to generate memory traffic to set dirty bits on
> DMAs. There is no way to back to the at-start state where their is 0
> memory traffic on DMAs. Not sure that it totally matters, but it is
> worth noting someplace.

Ah..you meant the HW penalty of keeping it on always.

> If we do want to solve this then ARM would need iommufd to make a pass
> over the page table to set for disable similar to how we have to clear
> for enable.

I will try to see if there is any overhead in keeping it on. Also we could limit
turning it on for DOMAIN_NESTED ?

Thanks,
Shameer


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iommu/arm-smmu-v3: Add set_dirty_tracking() support

[Joao Martins]

On 30/11/2023 12:54, Jason Gunthorpe wrote:
> On Thu, Nov 30, 2023 at 08:56:32AM +0000, Shameerali Kolothum Thodi wrote:
>>> On Tue, Nov 28, 2023 at 09:49:39AM +0000, Shameer Kolothum wrote:
>>>> From: Joao Martins <joao.m.martins@oracle.com>
>>>>
>>>> Dirty tracking will always be enabled with DBM=1 modifier enabled
>>>> by default when HD is supported.
>>>
>>> Is this trying to say that ARM doesn't have a per-table global enable
>>> for dirty tracking but instead pre-sets the DBM bit to avoid the cost?
>>>
>>
>> Yes. SMMUv3 has per-PTE DBM control and I think the initial RFC had
>> it walking the PTEs and setting the DBM on set_dirty_tracking().
> 
> set_dirty_tracking doesn't have access to the necessary locking to
> touch the PTEs.
> 
This is done for free by iopt_clear_dirty_data().

set_dirty_tracking() is mostly a nop, under the assumption that dirty
tracking is always enabled.

>>> So on smmuv3 to enable we have to clear everything and disable
>>> continues to pay a penalty since we don't go and mark all things as
>>> dirty again?
>>
>> Yes we clear everything on enable. Sorry I didn't get the second part.
>> We don't mark dirty on disable. How is that different from Intel/AMD?
> 
> Intel/AMD have a global switch so they just turn off the tracking and
> stop paying the cost.
> 
> This approach on ARM means once the tracking is logically turned off
> the HW will continue to generate memory traffic to set dirty bits on
> DMAs. There is no way to back to the at-start state where their is 0
> memory traffic on DMAs. Not sure that it totally matters, but it is
> worth noting someplace.
> 
> If we do want to solve this then ARM would need iommufd to make a pass
> over the page table to set for disable similar to how we have to clear
> for enable.

The firsts attempt at this had to be dynamic[0].

Like set_dirty_tracking_range would pass over the io pagetable and set the DBM
bit to enable dirty tracking. It was suggested that we switch to an always-on
mode to simplify initial bringup of the feature, and if the always-on was
somehow affecting DMA performance, we would re-attempt at this dynamic mode
post-mortem.

With the current code structure, perhaps having set_dirty_tracking() do the
DBM-enable pass we would need to move iorw_sem section to include the call to
set_dirty_tracking() and smmu op set_dirty_tracking() op would walk the whole
pt, without relying on iopt areas. Or we go back to iterating areas similar to
[0] with a new op.

[0] https://lore.kernel.org/kvm/20220428210933.3583-16-joao.m.martins@oracle.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

[Joao Martins]

On 28/11/2023 09:49, Shameer Kolothum wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
> 
> SMMUv3 implements all requirements of revoking device
> attachment if smmu does not support dirty tracking.
> 
> Finally handle the IOMMU_CAP_DIRTY in iommu_capable for
> IOMMUFD_DEVICE_GET_HW_INFO.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index c5eabdc29ba5..2c100f2136bc 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2244,6 +2244,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
>  	case IOMMU_CAP_NOEXEC:
>  	case IOMMU_CAP_DEFERRED_FLUSH:
>  		return true;
> +	case IOMMU_CAP_DIRTY_TRACKING:
> +		return arm_smmu_dbm_capable(master->smmu);
>  	default:
>  		return false;
>  	}
> @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	master = dev_iommu_priv_get(dev);
>  	smmu = master->smmu;
>  
> +	if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
> +		return -EINVAL;
> +
>  	mutex_lock(&smmu_domain->init_mutex);
>  
>  	if (!smmu_domain->smmu) {
> @@ -3077,7 +3082,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  			   const struct iommu_user_data *user_data)
>  {
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> -	const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT;
> +	const u32 paging_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
> +				 IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +	bool enforce_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>  	struct arm_smmu_domain *smmu_domain;
>  	int ret;
>  
> @@ -3090,6 +3097,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  	if (user_data)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (enforce_dirty &&
> +	    !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
> +		return ERR_PTR(-EOPNOTSUPP);
> +
>  	smmu_domain = arm_smmu_domain_alloc();
>  	if (!smmu_domain)
>  		return ERR_PTR(-ENOMEM);
> @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  
>  	smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
>  	smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
> +	if (enforce_dirty)
> +		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;
> +
>  	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
>  	if (ret)
>  		goto err_free;

Perhaps it would be better to limit the blast radius of the always-on mode, to
just enable it in the context descriptor depending on passing the
HWPT_ALLOC_DIRTY_TRACKING flag instead of enabling for everybody.
Something like this in arm_smmu_domain_finalise():

	if (arm_smmu_dbm_capable(smmu) && smmu_domain->domain.dirty_ops &&
	    smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
		pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;


For bisectability perhaps the second patch of this series should be the last,
while bringing the implementation of arm_smmu_dbm_capable() to this patch.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel