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

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

[Shameer Kolothum]

Hi

v2 --> v3

 -Rebased on top of the latest of Jason's refactor series git[3].
 -Addressed comments from Ryan and Jason(patch 2 & 3, Thanks!)
 -Added R-by tags  to 1 & 4.

Please take a look and let me know your feedback.
Thanks,
Shameer

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

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.

v1 --> v2:
https://lore.kernel.org/linux-iommu/20231128094940.1344-1-shameerali.kolothum.thodi@huawei.com/

Addressed review comments from Jason and Joao(Thanks)
   -Moved dirty_ops setting to domain finalise(patch #3)
   -Only enable DBM for stage 1 if domain_alloc_user() requests it.
   -Changed IO page table walker(patch #2) and tested with 4KB/16KB/64KB
    with l1/l2/l3 traversal.(The earlier one had a bug where it fails to
    walk L3 level).
   -Rearranged patches a bit to improve bi-sectability.
   -Rebased on top of Jason's v5 of SMMUv3 new API series git.

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://github.com/jgunthorpe/linux/commits/smmuv3_newapi

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

Joao Martins (1):
  iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

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

Shameer Kolothum (1):
  iommu/io-pgtable-arm: Add read_and_clear_dirty() support

 drivers/iommu/Kconfig                       |   1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 129 ++++++++++++++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |   8 ++
 drivers/iommu/io-pgtable-arm.c              | 110 ++++++++++++++++-
 include/linux/io-pgtable.h                  |   4 +
 5 files changed, 226 insertions(+), 26 deletions(-)

-- 
2.34.1

[PATCH v3 1/4] 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.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Ryan Roberts <ryan.roberts@arm.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 5250e1d89fb8..bed0183ba809 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -4197,6 +4197,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;
@@ -4257,6 +4279,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.
@@ -4452,6 +4476,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 d888441c5ec1..fd60052a9ec6 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
@@ -665,6 +668,8 @@ struct arm_smmu_device {
 #define ARM_SMMU_FEAT_E2H		(1 << 18)
 #define ARM_SMMU_FEAT_NESTING		(1 << 19)
 #define ARM_SMMU_FEAT_ATTR_TYPES_OVR	(1 << 20)
+#define ARM_SMMU_FEAT_HA		(1 << 21)
+#define ARM_SMMU_FEAT_HD		(1 << 22)
 	u32				features;
 
 #define ARM_SMMU_OPT_SKIP_PREFETCH	(1 << 0)
-- 
2.34.1

[PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Shameer Kolothum]

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

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

PTE states with respect to DBM bit:

                       DBM bit        AP[2]("RDONLY" bit)
1. writable_clean        1                 1
2. writable_dirty        1                 0
3. read-only             0                 1

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/io-pgtable-arm.c | 105 ++++++++++++++++++++++++++++++++-
 1 file changed, 103 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index f7828a7aad41..da6cc52859ba 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_PTE_XN | ARM_LPAE_PTE_DBM)
 #define ARM_LPAE_PTE_ATTR_MASK		(ARM_LPAE_PTE_ATTR_LO_MASK |	\
 					 ARM_LPAE_PTE_ATTR_HI_MASK)
 /* Software bit for solving coherency races */
@@ -92,7 +93,11 @@
 
 /* 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_RDONLY		(((arm_lpae_iopte)1) << \
+					   ARM_LPAE_PTE_AP_RDONLY_BIT)
+#define ARM_LPAE_PTE_AP_WRITABLE_CLEAN	(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)
 
@@ -138,6 +143,9 @@
 
 #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
 
+#define iopte_hw_dirty(pte)	(((pte) & ARM_LPAE_PTE_AP_WRITABLE_CLEAN) == \
+				   ARM_LPAE_PTE_DBM)
+
 struct arm_lpae_io_pgtable {
 	struct io_pgtable	iop;
 
@@ -729,6 +737,98 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
 	return iopte_to_paddr(pte, data) | iova;
 }
 
+struct io_pgtable_walk_data {
+	struct iommu_dirty_bitmap	*dirty;
+	unsigned long			flags;
+	u64				addr;
+	const u64			end;
+};
+
+static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
+				       struct io_pgtable_walk_data *walk_data,
+				       arm_lpae_iopte *ptep,
+				       int lvl);
+
+static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
+				  struct io_pgtable_walk_data *walk_data,
+				  arm_lpae_iopte *ptep, int lvl)
+{
+	struct io_pgtable *iop = &data->iop;
+	arm_lpae_iopte pte = READ_ONCE(*ptep);
+
+	if (WARN_ON(!pte))
+		return -EINVAL;
+
+	if (iopte_leaf(pte, lvl, iop->fmt)) {
+		size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
+
+		if (iopte_hw_dirty(pte)) {
+			iommu_dirty_bitmap_record(walk_data->dirty,
+						  walk_data->addr, size);
+			if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
+				set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
+					(unsigned long *)ptep);
+		}
+		walk_data->addr += size;
+		return 0;
+	}
+
+	ptep = iopte_deref(pte, data);
+	return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
+}
+
+static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
+				       struct io_pgtable_walk_data *walk_data,
+				       arm_lpae_iopte *ptep,
+				       int lvl)
+{
+	u32 idx;
+	int max_entries, ret;
+
+	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
+		return -EINVAL;
+
+	if (lvl == data->start_level)
+		max_entries = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);
+	else
+		max_entries = ARM_LPAE_PTES_PER_TABLE(data);
+
+	for (idx = ARM_LPAE_LVL_IDX(walk_data->addr, lvl, data);
+	     (idx < max_entries) && (walk_data->addr < walk_data->end); ++idx) {
+		ret = io_pgtable_visit_dirty(data, walk_data, ptep + idx, lvl);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+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 io_pgtable_walk_data walk_data = {
+		.dirty = dirty,
+		.flags = flags,
+		.addr = iova,
+		.end = iova + size,
+	};
+	arm_lpae_iopte *ptep = data->pgd;
+	int lvl = data->start_level;
+
+	if (WARN_ON(!size))
+		return -EINVAL;
+	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
+		return -EINVAL;
+	if (data->iop.fmt != ARM_64_LPAE_S1)
+		return -EINVAL;
+
+	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
+}
+
 static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
 {
 	unsigned long granule, page_sizes;
@@ -807,6 +907,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

[PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Shameer Kolothum]

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

This provides all the infrastructure to enable dirty tracking if the
hardware has the capability and domain alloc request for it.

Please note, we still report no support for IOMMU_CAP_DIRTY_TRACKING
as it will finally be enabled in a subsequent patch.

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 | 82 +++++++++++++++------
 include/linux/io-pgtable.h                  |  4 +
 2 files changed, 63 insertions(+), 23 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 bed0183ba809..ad18436c5f7f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -38,6 +38,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,
@@ -80,7 +81,8 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
 };
 
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
-				    struct arm_smmu_device *smmu);
+				    struct arm_smmu_device *smmu,
+				    u32 flags);
 static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
 static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain *smmu_domain);
 
@@ -2335,7 +2337,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 		struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 		int ret;
 
-		ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
+		ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, 0);
 		if (ret) {
 			kfree(smmu_domain);
 			return ERR_PTR(ret);
@@ -2408,13 +2410,14 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
 }
 
 static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
-				    struct arm_smmu_device *smmu)
+				    struct arm_smmu_device *smmu,
+				    u32 flags)
 {
 	int ret;
-	unsigned long ias, oas;
 	enum io_pgtable_fmt fmt;
 	struct io_pgtable_cfg pgtbl_cfg;
 	struct io_pgtable_ops *pgtbl_ops;
+	bool enable_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 
 	/* Restrict the stage to what we can actually support */
 	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
@@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
 		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
 
+	pgtbl_cfg = (struct io_pgtable_cfg) {
+		.pgsize_bitmap	= smmu->pgsize_bitmap,
+		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
+		.tlb		= &arm_smmu_flush_ops,
+		.iommu_dev	= smmu->dev,
+	};
+
 	switch (smmu_domain->stage) {
 	case ARM_SMMU_DOMAIN_S1:
-		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
-		ias = min_t(unsigned long, ias, VA_BITS);
-		oas = smmu->ias;
+		unsigned long ias = (smmu->features &
+				     ARM_SMMU_FEAT_VAX) ? 52 : 48;
+		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
+		pgtbl_cfg.oas = smmu->ias;
+		if (enable_dirty)
+			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
 		fmt = ARM_64_LPAE_S1;
 		break;
 	case ARM_SMMU_DOMAIN_S2:
-		ias = smmu->ias;
-		oas = smmu->oas;
+		pgtbl_cfg.ias = smmu->ias;
+		pgtbl_cfg.oas = smmu->oas;
 		fmt = ARM_64_LPAE_S2;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	pgtbl_cfg = (struct io_pgtable_cfg) {
-		.pgsize_bitmap	= smmu->pgsize_bitmap,
-		.ias		= ias,
-		.oas		= oas,
-		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
-		.tlb		= &arm_smmu_flush_ops,
-		.iommu_dev	= smmu->dev,
-	};
-
 	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
 	if (!pgtbl_ops)
 		return -ENOMEM;
@@ -2454,7 +2458,8 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
 	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
 	smmu_domain->domain.geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
 	smmu_domain->domain.geometry.force_aperture = true;
-
+	if (enable_dirty && smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
+		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;
 	ret = arm_smmu_domain_alloc_id(smmu, smmu_domain);
 	if (ret < 0) {
 		free_io_pgtable_ops(pgtbl_ops);
@@ -2777,7 +2782,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	mutex_lock(&smmu_domain->init_mutex);
 
 	if (!smmu_domain->smmu) {
-		ret = arm_smmu_domain_finalise(smmu_domain, smmu);
+		ret = arm_smmu_domain_finalise(smmu_domain, smmu, 0);
 	} else if (smmu_domain->smmu != smmu)
 		ret = -EINVAL;
 
@@ -2842,7 +2847,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
 
 	mutex_lock(&smmu_domain->init_mutex);
 	if (!smmu_domain->smmu)
-		ret = arm_smmu_domain_finalise(smmu_domain, smmu);
+		ret = arm_smmu_domain_finalise(smmu_domain, smmu, 0);
 	else if (smmu_domain->smmu != smmu)
 		ret = -EINVAL;
 	mutex_unlock(&smmu_domain->init_mutex);
@@ -3175,7 +3180,8 @@ 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;
 	struct arm_smmu_domain *smmu_domain;
 	int ret;
 
@@ -3188,6 +3194,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 	if (user_data)
 		return ERR_PTR(-EINVAL);
 
+	if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
+	    !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);
@@ -3203,7 +3213,7 @@ 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;
-	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
+	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);
 	if (ret)
 		goto err_free;
 	return &smmu_domain->domain;
@@ -3479,6 +3489,27 @@ static void arm_smmu_release_device(struct device *dev)
 	kfree(master);
 }
 
+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;
+
+	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
+}
+
+static int arm_smmu_set_dirty_tracking(struct iommu_domain *domain,
+				       bool enabled)
+{
+	/*
+	 * Always enabled and the dirty bitmap is cleared prior to
+	 * set_dirty_tracking().
+	 */
+	return 0;
+}
+
 static struct iommu_group *arm_smmu_device_group(struct device *dev)
 {
 	struct iommu_group *group;
@@ -3622,6 +3653,11 @@ 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 */
 static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
 				   struct arm_smmu_queue *q,
diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
index 86cf1f7ae389..8e75f944f07a 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

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

[Shameer Kolothum]

From: Kunkun Jiang <jiangkunkun@huawei.com>

If io-pgtable quirk flag indicates support for hardware update of
dirty state, enable HA/HD bits in the SMMU CD and also set the DBM
bit in the page descriptor.

Now report the dirty page tracking capability of SMMUv3 and
select IOMMUFD_DRIVER for ARM_SMMU_V3 if IOMMUFD is enabled.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/iommu/Kconfig                       |  1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 +++
 drivers/iommu/io-pgtable-arm.c              |  5 ++++-
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f872aeccd820..912cffc9f001 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -390,6 +390,7 @@ config ARM_SMMU_V3
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select GENERIC_MSI_IRQ
+	select IOMMUFD_DRIVER if IOMMUFD
 	help
 	  Support for implementations of the ARM System MMU architecture
 	  version 3 providing translation support to a PCIe root complex.
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 ad18436c5f7f..e3143be3cfc6 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1333,6 +1333,12 @@ void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
 		CTXDESC_CD_0_ASET |
 		FIELD_PREP(CTXDESC_CD_0_ASID, smmu_domain->asid)
 		);
+
+	/* To enable dirty flag update, set both Access flag and dirty state update */
+	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);
@@ -2264,6 +2270,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 features = (ARM_SMMU_FEAT_HD | ARM_SMMU_FEAT_COHERENCY);
+
+	return (smmu->features & features) == features;
+}
+
 /* IOMMU API */
 static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 {
@@ -2276,6 +2289,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;
 	}
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 fd60052a9ec6..f684676d0bb5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -312,6 +312,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 da6cc52859ba..20ac0e833c7b 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -433,6 +433,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_CLEAN;
 		if (!(prot & IOMMU_PRIV))
 			pte |= ARM_LPAE_PTE_AP_UNPRIV;
 	} else {
@@ -923,7 +925,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);
-- 
2.34.1

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Ryan Roberts]

On 30/04/2024 14:43, Shameer Kolothum wrote:
> .read_and_clear_dirty() IOMMU domain op takes care of reading the dirty
> bits (i.e. PTE has DBM set and AP[2] clear) and marshalling into a
> bitmap of a given page size.
> 
> While reading the dirty bits we also set the PTE AP[2] bit to mark it
> as writable-clean depending on read_and_clear_dirty() flags.
> 
> PTE states with respect to DBM bit:
> 
>                        DBM bit        AP[2]("RDONLY" bit)
> 1. writable_clean        1                 1
> 2. writable_dirty        1                 0
> 3. read-only             0                 1
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Couple of nits/comments, but regardless:

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

> ---
>  drivers/iommu/io-pgtable-arm.c | 105 ++++++++++++++++++++++++++++++++-
>  1 file changed, 103 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index f7828a7aad41..da6cc52859ba 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_PTE_XN | ARM_LPAE_PTE_DBM)
>  #define ARM_LPAE_PTE_ATTR_MASK		(ARM_LPAE_PTE_ATTR_LO_MASK |	\
>  					 ARM_LPAE_PTE_ATTR_HI_MASK)
>  /* Software bit for solving coherency races */
> @@ -92,7 +93,11 @@
>  
>  /* 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_RDONLY		(((arm_lpae_iopte)1) << \
> +					   ARM_LPAE_PTE_AP_RDONLY_BIT)
> +#define ARM_LPAE_PTE_AP_WRITABLE_CLEAN	(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)
>  
> @@ -138,6 +143,9 @@
>  
>  #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
>  
> +#define iopte_hw_dirty(pte)	(((pte) & ARM_LPAE_PTE_AP_WRITABLE_CLEAN) == \
> +				   ARM_LPAE_PTE_DBM)

Could this simply be called "iopte_dirty"? The hw and sw dirty concept only come
about in the arch code because we keep an additional sw bit to track dirty in
the pte, which you are not doing here.

> +
>  struct arm_lpae_io_pgtable {
>  	struct io_pgtable	iop;
>  
> @@ -729,6 +737,98 @@ static phys_addr_t arm_lpae_iova_to_phys(struct io_pgtable_ops *ops,
>  	return iopte_to_paddr(pte, data) | iova;
>  }
>  
> +struct io_pgtable_walk_data {
> +	struct iommu_dirty_bitmap	*dirty;
> +	unsigned long			flags;
> +	u64				addr;
> +	const u64			end;
> +};
> +
> +static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
> +				       struct io_pgtable_walk_data *walk_data,
> +				       arm_lpae_iopte *ptep,
> +				       int lvl);
> +
> +static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
> +				  struct io_pgtable_walk_data *walk_data,
> +				  arm_lpae_iopte *ptep, int lvl)
> +{
> +	struct io_pgtable *iop = &data->iop;
> +	arm_lpae_iopte pte = READ_ONCE(*ptep);
> +
> +	if (WARN_ON(!pte))

Wouldn't it actually be better to check that the pte is valid (i.e. bit 0 is
set)? And do we really need the warn here? I'm guessing you never expect to be
walking over a hole? - if it is legitimately possible to walk over a hole, then
the WARN is definitely incorrect and you would want to continue walking rather
than return.

> +		return -EINVAL;
> +
> +	if (iopte_leaf(pte, lvl, iop->fmt)) {
> +		size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +
> +		if (iopte_hw_dirty(pte)) {
> +			iommu_dirty_bitmap_record(walk_data->dirty,
> +						  walk_data->addr, size);
> +			if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
> +				set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
> +					(unsigned long *)ptep);

Just checking, set_bit() is atomic right? So safe against racing updates with
the HW. Pretty sure that's the case...

> +		}
> +		walk_data->addr += size;
> +		return 0;
> +	}
> +
> +	ptep = iopte_deref(pte, data);
> +	return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
> +}
> +
> +static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
> +				       struct io_pgtable_walk_data *walk_data,
> +				       arm_lpae_iopte *ptep,
> +				       int lvl)
> +{
> +	u32 idx;
> +	int max_entries, ret;
> +
> +	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
> +		return -EINVAL;
> +
> +	if (lvl == data->start_level)
> +		max_entries = ARM_LPAE_PGD_SIZE(data) / sizeof(arm_lpae_iopte);
> +	else
> +		max_entries = ARM_LPAE_PTES_PER_TABLE(data);
> +
> +	for (idx = ARM_LPAE_LVL_IDX(walk_data->addr, lvl, data);
> +	     (idx < max_entries) && (walk_data->addr < walk_data->end); ++idx) {
> +		ret = io_pgtable_visit_dirty(data, walk_data, ptep + idx, lvl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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 io_pgtable_walk_data walk_data = {
> +		.dirty = dirty,
> +		.flags = flags,
> +		.addr = iova,
> +		.end = iova + size,
> +	};
> +	arm_lpae_iopte *ptep = data->pgd;
> +	int lvl = data->start_level;
> +
> +	if (WARN_ON(!size))
> +		return -EINVAL;
> +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
> +		return -EINVAL;
> +	if (data->iop.fmt != ARM_64_LPAE_S1)
> +		return -EINVAL;
> +
> +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
> +}
> +
>  static void arm_lpae_restrict_pgsizes(struct io_pgtable_cfg *cfg)
>  {
>  	unsigned long granule, page_sizes;
> @@ -807,6 +907,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;

Re: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Ryan Roberts]

On 30/04/2024 14:43, Shameer Kolothum wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
> 
> This provides all the infrastructure to enable dirty tracking if the
> hardware has the capability and domain alloc request for it.
> 
> Please note, we still report no support for IOMMU_CAP_DIRTY_TRACKING
> as it will finally be enabled in a subsequent patch.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 82 +++++++++++++++------
>  include/linux/io-pgtable.h                  |  4 +
>  2 files changed, 63 insertions(+), 23 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 bed0183ba809..ad18436c5f7f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -38,6 +38,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,
> @@ -80,7 +81,8 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>  };
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> -				    struct arm_smmu_device *smmu);
> +				    struct arm_smmu_device *smmu,
> +				    u32 flags);
>  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>  static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain *smmu_domain);
>  
> @@ -2335,7 +2337,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>  		struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>  		int ret;
>  
> -		ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
> +		ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, 0);
>  		if (ret) {
>  			kfree(smmu_domain);
>  			return ERR_PTR(ret);
> @@ -2408,13 +2410,14 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
>  }
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> -				    struct arm_smmu_device *smmu)
> +				    struct arm_smmu_device *smmu,
> +				    u32 flags)
>  {
>  	int ret;
> -	unsigned long ias, oas;
>  	enum io_pgtable_fmt fmt;
>  	struct io_pgtable_cfg pgtbl_cfg;
>  	struct io_pgtable_ops *pgtbl_ops;
> +	bool enable_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>  
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>  
> +	pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +
>  	switch (smmu_domain->stage) {
>  	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
> +		unsigned long ias = (smmu->features &
> +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;
> +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
> +		pgtbl_cfg.oas = smmu->ias;
> +		if (enable_dirty)
> +			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
>  		fmt = ARM_64_LPAE_S1;
>  		break;
>  	case ARM_SMMU_DOMAIN_S2:
> -		ias = smmu->ias;
> -		oas = smmu->oas;
> +		pgtbl_cfg.ias = smmu->ias;
> +		pgtbl_cfg.oas = smmu->oas;
>  		fmt = ARM_64_LPAE_S2;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	pgtbl_cfg = (struct io_pgtable_cfg) {
> -		.pgsize_bitmap	= smmu->pgsize_bitmap,
> -		.ias		= ias,
> -		.oas		= oas,
> -		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> -		.tlb		= &arm_smmu_flush_ops,
> -		.iommu_dev	= smmu->dev,
> -	};
> -
>  	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
>  	if (!pgtbl_ops)
>  		return -ENOMEM;
> @@ -2454,7 +2458,8 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
>  	smmu_domain->domain.geometry.aperture_end = (1UL << pgtbl_cfg.ias) - 1;
>  	smmu_domain->domain.geometry.force_aperture = true;
> -
> +	if (enable_dirty && smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
> +		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;
>  	ret = arm_smmu_domain_alloc_id(smmu, smmu_domain);
>  	if (ret < 0) {
>  		free_io_pgtable_ops(pgtbl_ops);
> @@ -2777,7 +2782,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	mutex_lock(&smmu_domain->init_mutex);
>  
>  	if (!smmu_domain->smmu) {
> -		ret = arm_smmu_domain_finalise(smmu_domain, smmu);
> +		ret = arm_smmu_domain_finalise(smmu_domain, smmu, 0);
>  	} else if (smmu_domain->smmu != smmu)
>  		ret = -EINVAL;
>  
> @@ -2842,7 +2847,7 @@ static int arm_smmu_s1_set_dev_pasid(struct iommu_domain *domain,
>  
>  	mutex_lock(&smmu_domain->init_mutex);
>  	if (!smmu_domain->smmu)
> -		ret = arm_smmu_domain_finalise(smmu_domain, smmu);
> +		ret = arm_smmu_domain_finalise(smmu_domain, smmu, 0);
>  	else if (smmu_domain->smmu != smmu)
>  		ret = -EINVAL;
>  	mutex_unlock(&smmu_domain->init_mutex);
> @@ -3175,7 +3180,8 @@ 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;
>  	struct arm_smmu_domain *smmu_domain;
>  	int ret;
>  
> @@ -3188,6 +3194,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  	if (user_data)
>  		return ERR_PTR(-EINVAL);
>  
> +	if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
> +	    !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);
> @@ -3203,7 +3213,7 @@ 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;
> -	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu);
> +	ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);
>  	if (ret)
>  		goto err_free;
>  	return &smmu_domain->domain;
> @@ -3479,6 +3489,27 @@ static void arm_smmu_release_device(struct device *dev)
>  	kfree(master);
>  }
>  
> +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;
> +
> +	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
> +}
> +
> +static int arm_smmu_set_dirty_tracking(struct iommu_domain *domain,
> +				       bool enabled)
> +{
> +	/*
> +	 * Always enabled and the dirty bitmap is cleared prior to
> +	 * set_dirty_tracking().
> +	 */
> +	return 0;
> +}
> +
>  static struct iommu_group *arm_smmu_device_group(struct device *dev)
>  {
>  	struct iommu_group *group;
> @@ -3622,6 +3653,11 @@ 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 */
>  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  				   struct arm_smmu_queue *q,
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index 86cf1f7ae389..8e75f944f07a 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;

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

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:

> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index da6cc52859ba..20ac0e833c7b 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -433,6 +433,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_CLEAN;

This seems a bit suboptimal, it means the HTTU will be generating
dirty's before the tracking is turned on. As I understand it if the
SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
so this would be a drag on baseline performance?

Should this start out as dirty and let the enable flow clean it to
turn it on?

Looks fine otherwise:

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

Jason

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 02:43:06PM +0100, Shameer Kolothum wrote:
> +static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
> +				  struct io_pgtable_walk_data *walk_data,
> +				  arm_lpae_iopte *ptep, int lvl)
> +{
> +	struct io_pgtable *iop = &data->iop;
> +	arm_lpae_iopte pte = READ_ONCE(*ptep);
> +
> +	if (WARN_ON(!pte))
> +		return -EINVAL;

This seems poorly placed, why would pte ever be zero?

> +	if (iopte_leaf(pte, lvl, iop->fmt)) {
> +		size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +
> +		if (iopte_hw_dirty(pte)) {
> +			iommu_dirty_bitmap_record(walk_data->dirty,
> +						  walk_data->addr, size);
> +			if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
> +				set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
> +					(unsigned long *)ptep);
> +		}
> +		walk_data->addr += size;
> +		return 0;
> +	}
> +
> +	ptep = iopte_deref(pte, data);

This would be a better spot, if the pte doesn't indicate a next level
table then something has gone wrong, otherwise the returned pointer
has to be valid.

Something like this maybe?

static inline bool iopte_table(arm_lpae_iopte pte, int lvl,
			      enum io_pgtable_fmt fmt)
{
	if (lvl == (ARM_LPAE_MAX_LEVELS - 1))
		return false;
	return iopte_type(pte) == ARM_LPAE_PTE_TYPE_TABLE;
}


> +	return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
> +}
> +
> +static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
> +				       struct io_pgtable_walk_data *walk_data,
> +				       arm_lpae_iopte *ptep,
> +				       int lvl)
> +{
> +	u32 idx;
> +	int max_entries, ret;
> +
> +	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
> +		return -EINVAL;

And if the above ipte_deref() could check for a valid next level (with
no valid level at MAX_LEVELS) then this can never happen either and
can be removed.

Jason

Re: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 02:43:07PM +0100, Shameer Kolothum wrote:
> @@ -2408,13 +2410,14 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
>  }
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> -				    struct arm_smmu_device *smmu)
> +				    struct arm_smmu_device *smmu,
> +				    u32 flags)
>  {
>  	int ret;
> -	unsigned long ias, oas;
>  	enum io_pgtable_fmt fmt;
>  	struct io_pgtable_cfg pgtbl_cfg;
>  	struct io_pgtable_ops *pgtbl_ops;
> +	bool enable_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>  
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>  
> +	pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +
>  	switch (smmu_domain->stage) {
>  	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
> +		unsigned long ias = (smmu->features &
> +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;

It is a good idea to wrap this in a {} scope starting at the case when
declaring varaibles

> @@ -3188,6 +3194,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  	if (user_data)
>  		return ERR_PTR(-EINVAL);
>  
> +	if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
> +	    !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
> +		return ERR_PTR(-EOPNOTSUPP);

Let's put this device_iommu_capable hunk in the iommufd core code
instead of in drivers

> +
>  	smmu_domain = arm_smmu_domain_alloc();
>  	if (!smmu_domain)
>  		return ERR_PTR(-ENOMEM);

This needs the alloc_user function... Can you cherry pick it from my
series? Just remove all the nesting parts. I'd like to see this be
self-contained on top of v6.10-rc1. I think it is in good shape, lets
see it go early enough in that cycle

The arm_smmu_domain_alloc() will need to be picked as well.

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

Thanks,
Jason

Re: [PATCH v3 0/4] iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3

[Jason Gunthorpe]

On Tue, Apr 30, 2024 at 02:43:04PM +0100, Shameer Kolothum wrote:
> This is revisiting the earlier attempts [1, 2] to use SMMUv3 HTTU feature
> for dirty page tracking. The Intel/AMD support is already mainline.
> 
> 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.

This looks good to me, lets have it go after the merge window. As I
mentioned please post a version rebased on top of v6.10-rc1. (ie
based on the patches only in Joerg's current tree) in two weeks.

Thanks,
Jason

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

[Tian, Kevin]

> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Tuesday, April 30, 2024 9:43 PM
> 
> +
> +	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);
> +}

it's probably good to print out the IDR0.HTTU value.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Tian, Kevin]

> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Tuesday, April 30, 2024 9:43 PM
> 
> @@ -92,7 +93,11 @@
> 
>  /* 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_RDONLY		(((arm_lpae_iopte)1) << \
> +					   ARM_LPAE_PTE_AP_RDONLY_BIT)
> +#define ARM_LPAE_PTE_AP_WRITABLE_CLEAN
> 	(ARM_LPAE_PTE_AP_RDONLY | \
> +					 ARM_LPAE_PTE_DBM)

based on the usage is it clearer to be xxx_WRITEABLE_MASK?

>  #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
>  #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
> 
> @@ -138,6 +143,9 @@
> 
>  #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
> 
> +#define iopte_hw_dirty(pte)	(((pte) &
> ARM_LPAE_PTE_AP_WRITABLE_CLEAN) == \
> +				   ARM_LPAE_PTE_DBM)
> +

iopte_is_writeable_dirty()?

and the following "set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
(unsigned long *)ptep);" could be wrapped as:

	iopte_set_writable_clean(ptep);

> +
> +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 io_pgtable_walk_data walk_data = {
> +		.dirty = dirty,
> +		.flags = flags,
> +		.addr = iova,
> +		.end = iova + size,
> +	};
> +	arm_lpae_iopte *ptep = data->pgd;
> +	int lvl = data->start_level;
> +
> +	if (WARN_ON(!size))
> +		return -EINVAL;
> +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
> +		return -EINVAL;
> +	if (data->iop.fmt != ARM_64_LPAE_S1)
> +		return -EINVAL;
> +
> +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);

Intel/AMD drivers also checks:

        if (!dmar_domain->dirty_tracking && dirty->bitmap)
                return -EINVAL;

RE: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Tian, Kevin]

> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Sent: Tuesday, April 30, 2024 9:43 PM
> 
> @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct
> arm_smmu_domain *smmu_domain,
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> 
> +	pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.coherent_walk	= smmu->features &
> ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +
>  	switch (smmu_domain->stage) {
>  	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
> +		unsigned long ias = (smmu->features &
> +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;
> +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
> +		pgtbl_cfg.oas = smmu->ias;
> +		if (enable_dirty)
> +			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;

why is dirty tracking considered as a quirk?

>  		fmt = ARM_64_LPAE_S1;
>  		break;
>  	case ARM_SMMU_DOMAIN_S2:
> -		ias = smmu->ias;
> -		oas = smmu->oas;
> +		pgtbl_cfg.ias = smmu->ias;
> +		pgtbl_cfg.oas = smmu->oas;
>  		fmt = ARM_64_LPAE_S2;
>  		break;

so dirty-tracking is not supported by s2? what about nesting?

if this is desired then attempting to set dirty_tracking on a s2 domain
should be rejected with an error.

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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Sunday, May 12, 2024 8:09 PM
> 
> On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:
> 
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-
> arm.c
> > index da6cc52859ba..20ac0e833c7b 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -433,6 +433,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_CLEAN;
> 
> This seems a bit suboptimal, it means the HTTU will be generating
> dirty's before the tracking is turned on. As I understand it if the
> SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
> so this would be a drag on baseline performance?
> 
> Should this start out as dirty and let the enable flow clean it to
> turn it on?
> 

this appears to be good for other vendors too?

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Jason Gunthorpe]

On Wed, May 22, 2024 at 07:12:27AM +0000, Tian, Kevin wrote:
> > +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 io_pgtable_walk_data walk_data = {
> > +		.dirty = dirty,
> > +		.flags = flags,
> > +		.addr = iova,
> > +		.end = iova + size,
> > +	};
> > +	arm_lpae_iopte *ptep = data->pgd;
> > +	int lvl = data->start_level;
> > +
> > +	if (WARN_ON(!size))
> > +		return -EINVAL;
> > +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
> > +		return -EINVAL;
> > +	if (data->iop.fmt != ARM_64_LPAE_S1)
> > +		return -EINVAL;
> > +
> > +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
> 
> Intel/AMD drivers also checks:
> 
>         if (!dmar_domain->dirty_tracking && dirty->bitmap)
>                 return -EINVAL;

Those both seem redundant checks on a fast path?

Jason

Re: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Jason Gunthorpe]

On Wed, May 22, 2024 at 07:16:27AM +0000, Tian, Kevin wrote:

> >  	case ARM_SMMU_DOMAIN_S2:
> > -		ias = smmu->ias;
> > -		oas = smmu->oas;
> > +		pgtbl_cfg.ias = smmu->ias;
> > +		pgtbl_cfg.oas = smmu->oas;
> >  		fmt = ARM_64_LPAE_S2;
> >  		break;
> 
> so dirty-tracking is not supported by s2? what about nesting?

To be done later IIRC

> if this is desired then attempting to set dirty_tracking on a s2 domain
> should be rejected with an error.

Yes

Jason

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

[Jason Gunthorpe]

On Wed, May 22, 2024 at 07:19:05AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Sunday, May 12, 2024 8:09 PM
> > 
> > On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:
> > 
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-
> > arm.c
> > > index da6cc52859ba..20ac0e833c7b 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -433,6 +433,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_CLEAN;
> > 
> > This seems a bit suboptimal, it means the HTTU will be generating
> > dirty's before the tracking is turned on. As I understand it if the
> > SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
> > so this would be a drag on baseline performance?
> > 
> > Should this start out as dirty and let the enable flow clean it to
> > turn it on?
> > 
> 
> this appears to be good for other vendors too?

I thought Intel and AMD both had a per-table flag to turn on tracking
and without that bit the dirties are not written back?

Jason

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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Sunday, May 12, 2024 1:09 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;
> ryan.roberts@arm.com; 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 v3 4/4] iommu/arm-smmu-v3: Enable HTTU for stage1
> with io-pgtable mapping
> 
> On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:
> 
> > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-
> arm.c
> > index da6cc52859ba..20ac0e833c7b 100644
> > --- a/drivers/iommu/io-pgtable-arm.c
> > +++ b/drivers/iommu/io-pgtable-arm.c
> > @@ -433,6 +433,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_CLEAN;
> 
> This seems a bit suboptimal, it means the HTTU will be generating
> dirty's before the tracking is turned on. As I understand it if the
> SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
> so this would be a drag on baseline performance?

Our initial tests has not shown any difference so far. But it is a possibility.
 
> Should this start out as dirty and let the enable flow clean it to
> turn it on?

Ok. Just to be clear, so the suggestion is just set the DBM bit here and let the
read_and_clear_dirty() set the  AP_RDONLY_BIT through the iopt_set_dirty_tracking()
path.

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

Thanks,
Shameer

RE: [PATCH v3 0/4] iommu/smmuv3: Add IOMMUFD dirty tracking support for SMMUv3

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Sunday, May 12, 2024 1:59 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;
> ryan.roberts@arm.com; 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 v3 0/4] iommu/smmuv3: Add IOMMUFD dirty tracking
> support for SMMUv3
> 
> On Tue, Apr 30, 2024 at 02:43:04PM +0100, Shameer Kolothum wrote:
> > This is revisiting the earlier attempts [1, 2] to use SMMUv3 HTTU feature
> > for dirty page tracking. The Intel/AMD support is already mainline.
> >
> > 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.
> 
> This looks good to me, lets have it go after the merge window. As I
> mentioned please post a version rebased on top of v6.10-rc1. (ie
> based on the patches only in Joerg's current tree) in two weeks.

Ok. I will cherry pick the ones required to make this independent and
post it soon after addressing all the pending comments.

Thanks,
Shameer

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

[Jason Gunthorpe]

On Wed, May 22, 2024 at 01:26:20PM +0000, Shameerali Kolothum Thodi wrote:

> > On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:
> > 
> > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-
> > arm.c
> > > index da6cc52859ba..20ac0e833c7b 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -433,6 +433,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_CLEAN;
> > 
> > This seems a bit suboptimal, it means the HTTU will be generating
> > dirty's before the tracking is turned on. As I understand it if the
> > SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
> > so this would be a drag on baseline performance?
> 
> Our initial tests has not shown any difference so far. But it is a possibility.

That is good news, it means the setting is quite low overhead

> > Should this start out as dirty and let the enable flow clean it to
> > turn it on?
> 
> Ok. Just to be clear, so the suggestion is just set the DBM bit here and let the
> read_and_clear_dirty() set the  AP_RDONLY_BIT through the iopt_set_dirty_tracking()
> path.

Yes, but it would be good to check with someone who knows what the IP
does to be sure. My guess is that should avoid memory writes.

Jason

RE: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, May 22, 2024 8:12 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
> Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> jgg@nvidia.com; ryan.roberts@arm.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 v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty()
> support
> 
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, April 30, 2024 9:43 PM
> >
> > @@ -92,7 +93,11 @@
> >
> >  /* 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_RDONLY		(((arm_lpae_iopte)1) << \
> > +					   ARM_LPAE_PTE_AP_RDONLY_BIT)
> > +#define ARM_LPAE_PTE_AP_WRITABLE_CLEAN
> > 	(ARM_LPAE_PTE_AP_RDONLY | \
> > +					 ARM_LPAE_PTE_DBM)
> 
> based on the usage is it clearer to be xxx_WRITEABLE_MASK?

I think in patch 4 we use this to set the PTE Writeable Clean. Anyway I will
revisit this following Jason's comment on just setting the DBM bit there.

> 
> >  #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
> >  #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
> >
> > @@ -138,6 +143,9 @@
> >
> >  #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
> >
> > +#define iopte_hw_dirty(pte)	(((pte) &
> > ARM_LPAE_PTE_AP_WRITABLE_CLEAN) == \
> > +				   ARM_LPAE_PTE_DBM)
> > +
> 
> iopte_is_writeable_dirty()?
> 
> and the following "set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
> (unsigned long *)ptep);" could be wrapped as:
> 
> 	iopte_set_writable_clean(ptep);

Ok. Will consider this during respin.
 
> > +
> > +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 io_pgtable_walk_data walk_data = {
> > +		.dirty = dirty,
> > +		.flags = flags,
> > +		.addr = iova,
> > +		.end = iova + size,
> > +	};
> > +	arm_lpae_iopte *ptep = data->pgd;
> > +	int lvl = data->start_level;
> > +
> > +	if (WARN_ON(!size))
> > +		return -EINVAL;
> > +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
> > +		return -EINVAL;
> > +	if (data->iop.fmt != ARM_64_LPAE_S1)
> > +		return -EINVAL;
> > +
> > +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
> 
> Intel/AMD drivers also checks:
> 
>         if (!dmar_domain->dirty_tracking && dirty->bitmap)
>                 return -EINVAL;

Is that really required? Is the concern here is user may issue
GET_DIRTY_BITMAP IOCTL without any validation and that may
result in unnecessary scanning of page tables? May be we should
handle it in core code then I think.

Thanks,
Shameer

RE: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, May 22, 2024 8:16 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
> Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> jgg@nvidia.com; ryan.roberts@arm.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 v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking
> in domain alloc
> 
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, April 30, 2024 9:43 PM
> >
> > @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct
> > arm_smmu_domain *smmu_domain,
> >  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
> >  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> >
> > +	pgtbl_cfg = (struct io_pgtable_cfg) {
> > +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> > +		.coherent_walk	= smmu->features &
> > ARM_SMMU_FEAT_COHERENCY,
> > +		.tlb		= &arm_smmu_flush_ops,
> > +		.iommu_dev	= smmu->dev,
> > +	};
> > +
> >  	switch (smmu_domain->stage) {
> >  	case ARM_SMMU_DOMAIN_S1:
> > -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> > -		ias = min_t(unsigned long, ias, VA_BITS);
> > -		oas = smmu->ias;
> > +		unsigned long ias = (smmu->features &
> > +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;
> > +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
> > +		pgtbl_cfg.oas = smmu->ias;
> > +		if (enable_dirty)
> > +			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
> 
> why is dirty tracking considered as a quirk?

Yes, that is a bit unconventional. But this was discussed earlier and in SMMUv3 
driver the word "quirk" is considered in the broadest sense and there are
precedent for this in the driver already.

From Robin:

"Indeed these features aren't decorative grooves on a piece of furniture, 
but in the case of io-pgtable we're merely using "quirk" in its broadest 
sense to imply something that differs from the baseline default 
behaviour - ARM_MTK_EXT, ARM_TTBR1 and ARM_OUTER_WBWA (or whatever it's 
called this week) are all just indicating extra hardware features 
entirely comparable to HTTU;..."

https://lore.kernel.org/linux-iommu/5ada4a8b-8852-f83c-040a-9ef5dac51de2@arm.com/

> 
> >  		fmt = ARM_64_LPAE_S1;
> >  		break;
> >  	case ARM_SMMU_DOMAIN_S2:
> > -		ias = smmu->ias;
> > -		oas = smmu->oas;
> > +		pgtbl_cfg.ias = smmu->ias;
> > +		pgtbl_cfg.oas = smmu->oas;
> >  		fmt = ARM_64_LPAE_S2;
> >  		break;
> 
> so dirty-tracking is not supported by s2? what about nesting?

It will be added later when we have the nested support.
 
> if this is desired then attempting to set dirty_tracking on a s2 domain
> should be rejected with an error.

Ok. I will add that.

Thanks,
Shameer

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 15:03, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Tian, Kevin <kevin.tian@intel.com>
>> Sent: Wednesday, May 22, 2024 8:12 AM
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
>> Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
>> jgg@nvidia.com; ryan.roberts@arm.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 v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty()
>> support
>>
>>> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> Sent: Tuesday, April 30, 2024 9:43 PM
>>> +
>>> +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 io_pgtable_walk_data walk_data = {
>>> +		.dirty = dirty,
>>> +		.flags = flags,
>>> +		.addr = iova,
>>> +		.end = iova + size,
>>> +	};
>>> +	arm_lpae_iopte *ptep = data->pgd;
>>> +	int lvl = data->start_level;
>>> +
>>> +	if (WARN_ON(!size))
>>> +		return -EINVAL;
>>> +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
>>> +		return -EINVAL;
>>> +	if (data->iop.fmt != ARM_64_LPAE_S1)
>>> +		return -EINVAL;
>>> +
>>> +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
>>
>> Intel/AMD drivers also checks:
>>
>>         if (!dmar_domain->dirty_tracking && dirty->bitmap)
>>                 return -EINVAL;
> 
> Is that really required? Is the concern here is user may issue
> GET_DIRTY_BITMAP IOCTL without any validation and that may
> result in unnecessary scanning of page tables? May be we should
> handle it in core code then I think.

This is just to catch the case where IOMMUFD can call into read_and_clear()
without dirty tracking enabled and without a bitmap structure to clear dirty
bits -- in order to ensure a clean PTE data snapshot after start(). And thus it
errors out on others.

Right now we don't track in iommufd that dirty tracking is active in the domain,
vendor does so in its own way. I mean we could move that to core, but we end up
duplicating what the iommu driver is already doing.

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Jason Gunthorpe]

On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
 
> This is just to catch the case where IOMMUFD can call into read_and_clear()
> without dirty tracking enabled and without a bitmap structure to clear dirty
> bits -- in order to ensure a clean PTE data snapshot after start(). 

Is that broken then?

iopt_clear_dirty_data() sets the NULL:

	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
		ret = ops->read_and_clear_dirty(domain, iopt_area_iova(area),
						iopt_area_length(area), 0,
						&dirty);

But AMD, for instance, does nothing:

	spin_lock_irqsave(&pdomain->lock, lflags);
	if (!pdomain->dirty_tracking && dirty->bitmap) {
		spin_unlock_irqrestore(&pdomain->lock, lflags);
		return -EINVAL;
	}
	spin_unlock_irqrestore(&pdomain->lock, lflags);

	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);

It certainly didn't clear the IOPTEs.

AFAIK the NULL should be ignored here:

static inline void iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
					     unsigned long iova,
					     unsigned long length)
{
	if (dirty->bitmap)
		iova_bitmap_set(dirty->bitmap, iova, length);

Not above. That looks like a bug. Yes?

Thanks,
Jason

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 17:56, Jason Gunthorpe wrote:
> On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
>  
>> This is just to catch the case where IOMMUFD can call into read_and_clear()
>> without dirty tracking enabled and without a bitmap structure to clear dirty
>> bits -- in order to ensure a clean PTE data snapshot after start(). 
> 
> Is that broken then?
> 

It's not: The check errors out the caller ends up calling read-and-clear with a
bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
passes a null bitmap, it goes through and it walks and clears the IOPTEs
*without* recording them in the bitmap.

> iopt_clear_dirty_data() sets the NULL:
> 
> 	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
> 		ret = ops->read_and_clear_dirty(domain, iopt_area_iova(area),
> 						iopt_area_length(area), 0,
> 						&dirty);
> 
Right

> But AMD, for instance, does nothing:
> 
> 	spin_lock_irqsave(&pdomain->lock, lflags);
> 	if (!pdomain->dirty_tracking && dirty->bitmap) {
> 		spin_unlock_irqrestore(&pdomain->lock, lflags);
> 		return -EINVAL;
> 	}
> 	spin_unlock_irqrestore(&pdomain->lock, lflags);
> 
> 	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
> 
Same for Intel (except it has a comment explaining the why).

But it's on purpose.

> It certainly didn't clear the IOPTEs.
>
But notice that this gets called *without* dirty::bitmap set so it goes through
and walk the page tables and clear the dirty bits. It just doesn't record in the
bitmap.

> AFAIK the NULL should be ignored here:
> 
> static inline void iommu_dirty_bitmap_record(struct iommu_dirty_bitmap *dirty,
> 					     unsigned long iova,
> 					     unsigned long length)
> {
> 	if (dirty->bitmap)
> 		iova_bitmap_set(dirty->bitmap, iova, length);
>

Right, yes. As intended. When we pass NULL bitmap we mean that we don't care
about dirty info. But you are still clearing the IOPTEs in the driver.

> Not above. That looks like a bug. Yes?
> 
It's not a bug.

It might be too subtle or maybe strange -- I recall raising this as not being
too obvious IIRC.

All this doing was to allow read_and_clear iommu driver to clear the bits
without recording bitmap info -- which is what it is doing.

In any case I am happy to clean this up if there's something more elegant in
people's mind.

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Jason Gunthorpe]

On Wed, May 22, 2024 at 06:10:59PM +0100, Joao Martins wrote:
> On 22/05/2024 17:56, Jason Gunthorpe wrote:
> > On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
> >  
> >> This is just to catch the case where IOMMUFD can call into read_and_clear()
> >> without dirty tracking enabled and without a bitmap structure to clear dirty
> >> bits -- in order to ensure a clean PTE data snapshot after start(). 
> > 
> > Is that broken then?
> > 
> 
> It's not: The check errors out the caller ends up calling read-and-clear with a
> bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
> passes a null bitmap, it goes through and it walks and clears the IOPTEs
> *without* recording them in the bitmap.

It is not "without recording them in the bitmap", saying that is the
confusing thing. The purpose of that 'if' is to return -EINVAL if
dirty tracking is not turned on and we query the bitmap.

More like this puts it in the common code and writes it in a more
straightforward way with better locking:

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index d35c1b8c8e65ce..b2cb557d3ea427 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2645,13 +2645,6 @@ static int amd_iommu_read_and_clear_dirty(struct iommu_domain *domain,
 	if (!ops || !ops->read_and_clear_dirty)
 		return -EOPNOTSUPP;
 
-	spin_lock_irqsave(&pdomain->lock, lflags);
-	if (!pdomain->dirty_tracking && dirty->bitmap) {
-		spin_unlock_irqrestore(&pdomain->lock, lflags);
-		return -EINVAL;
-	}
-	spin_unlock_irqrestore(&pdomain->lock, lflags);
-
 	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
 }
 
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 50eb9aed47cc58..844f2cf061911f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4797,15 +4797,6 @@ static int intel_iommu_read_and_clear_dirty(struct iommu_domain *domain,
 	unsigned long end = iova + size - 1;
 	unsigned long pgsize;
 
-	/*
-	 * IOMMUFD core calls into a dirty tracking disabled domain without an
-	 * IOVA bitmap set in order to clean dirty bits in all PTEs that might
-	 * have occurred when we stopped dirty tracking. This ensures that we
-	 * never inherit dirtied bits from a previous cycle.
-	 */
-	if (!dmar_domain->dirty_tracking && dirty->bitmap)
-		return -EINVAL;
-
 	do {
 		struct dma_pte *pte;
 		int lvl = 0;
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 05fd9d3abf1b80..d116179809042d 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -536,7 +536,10 @@ int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
 		return ret;
 
 	down_read(&iopt->iova_rwsem);
-	ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
+	if (!iopt->dirty_tracking_enabled)
+		ret = -EINVAL;
+	else
+		ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
 	up_read(&iopt->iova_rwsem);
 
 	return ret;
@@ -580,7 +583,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
 	if (!ops)
 		return -EOPNOTSUPP;
 
-	down_read(&iopt->iova_rwsem);
+	down_write(&iopt->iova_rwsem);
+	if (iopt->dirty_tracking_enabled == enable) {
+		ret = 0;
+		goto out_unlock;
+	}
 
 	/* Clear dirty bits from PTEs to ensure a clean snapshot */
 	if (enable) {
@@ -590,9 +597,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
 	}
 
 	ret = ops->set_dirty_tracking(domain, enable);
-
+	if (ret)
+		goto out_unlock;
+	iopt->dirty_tracking_enabled = enable;
 out_unlock:
-	up_read(&iopt->iova_rwsem);
+	up_write(&iopt->iova_rwsem);
 	return ret;
 }
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 991f864d1f9bc1..de3761e15cab54 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -52,6 +52,7 @@ struct io_pagetable {
 	/* IOVA that cannot be allocated, struct iopt_reserved */
 	struct rb_root_cached reserved_itree;
 	u8 disable_large_pages;
+	u8 dirty_tracking_enabled;
 	unsigned long iova_alignment;
 };
 

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 18:50, Jason Gunthorpe wrote:
> On Wed, May 22, 2024 at 06:10:59PM +0100, Joao Martins wrote:
>> On 22/05/2024 17:56, Jason Gunthorpe wrote:
>>> On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
>>>  
>>>> This is just to catch the case where IOMMUFD can call into read_and_clear()
>>>> without dirty tracking enabled and without a bitmap structure to clear dirty
>>>> bits -- in order to ensure a clean PTE data snapshot after start(). 
>>>
>>> Is that broken then?
>>>
>>
>> It's not: The check errors out the caller ends up calling read-and-clear with a
>> bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
>> passes a null bitmap, it goes through and it walks and clears the IOPTEs
>> *without* recording them in the bitmap.
> 
> It is not "without recording them in the bitmap", saying that is the
> confusing thing. The purpose of that 'if' is to return -EINVAL if
> dirty tracking is not turned on and we query the bitmap.
> 
Right.

> More like this puts it in the common code and writes it in a more
> straightforward way with better locking:
> 
Yes, This snip you pasted would be the equivalent to the current way indeed.
Looks good

I think I was trying too hard not to duplicate 'state of dirty tracking' between
iommu driver and iommufd core that I unintendedly ended up convoluting with this
check in the driver :/

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index d35c1b8c8e65ce..b2cb557d3ea427 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2645,13 +2645,6 @@ static int amd_iommu_read_and_clear_dirty(struct iommu_domain *domain,
>  	if (!ops || !ops->read_and_clear_dirty)
>  		return -EOPNOTSUPP;
>  
> -	spin_lock_irqsave(&pdomain->lock, lflags);
> -	if (!pdomain->dirty_tracking && dirty->bitmap) {
> -		spin_unlock_irqrestore(&pdomain->lock, lflags);
> -		return -EINVAL;
> -	}
> -	spin_unlock_irqrestore(&pdomain->lock, lflags);
> -
>  	return ops->read_and_clear_dirty(ops, iova, size, flags, dirty);
>  }
>  
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 50eb9aed47cc58..844f2cf061911f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4797,15 +4797,6 @@ static int intel_iommu_read_and_clear_dirty(struct iommu_domain *domain,
>  	unsigned long end = iova + size - 1;
>  	unsigned long pgsize;
>  
> -	/*
> -	 * IOMMUFD core calls into a dirty tracking disabled domain without an
> -	 * IOVA bitmap set in order to clean dirty bits in all PTEs that might
> -	 * have occurred when we stopped dirty tracking. This ensures that we
> -	 * never inherit dirtied bits from a previous cycle.
> -	 */
> -	if (!dmar_domain->dirty_tracking && dirty->bitmap)
> -		return -EINVAL;
> -
>  	do {
>  		struct dma_pte *pte;
>  		int lvl = 0;
> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
> index 05fd9d3abf1b80..d116179809042d 100644
> --- a/drivers/iommu/iommufd/io_pagetable.c
> +++ b/drivers/iommu/iommufd/io_pagetable.c
> @@ -536,7 +536,10 @@ int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
>  		return ret;
>  
>  	down_read(&iopt->iova_rwsem);
> -	ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
> +	if (!iopt->dirty_tracking_enabled)
> +		ret = -EINVAL;
> +	else
> +		ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
>  	up_read(&iopt->iova_rwsem);
>  
>  	return ret;
> @@ -580,7 +583,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>  	if (!ops)
>  		return -EOPNOTSUPP;
>  
> -	down_read(&iopt->iova_rwsem);
> +	down_write(&iopt->iova_rwsem);
> +	if (iopt->dirty_tracking_enabled == enable) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
>  
>  	/* Clear dirty bits from PTEs to ensure a clean snapshot */
>  	if (enable) {
> @@ -590,9 +597,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>  	}
>  
>  	ret = ops->set_dirty_tracking(domain, enable);
> -
> +	if (ret)
> +		goto out_unlock;
> +	iopt->dirty_tracking_enabled = enable;
>  out_unlock:
> -	up_read(&iopt->iova_rwsem);
> +	up_write(&iopt->iova_rwsem);
>  	return ret;
>  }
>  
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 991f864d1f9bc1..de3761e15cab54 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -52,6 +52,7 @@ struct io_pagetable {
>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>  	struct rb_root_cached reserved_itree;
>  	u8 disable_large_pages;
> +	u8 dirty_tracking_enabled;
>  	unsigned long iova_alignment;
>  };
>  

Re: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Joao Martins]

On 22/05/2024 19:15, Joao Martins wrote:
> On 22/05/2024 18:50, Jason Gunthorpe wrote:
>> On Wed, May 22, 2024 at 06:10:59PM +0100, Joao Martins wrote:
>>> On 22/05/2024 17:56, Jason Gunthorpe wrote:
>>>> On Wed, May 22, 2024 at 03:37:57PM +0100, Joao Martins wrote:
>>>>  
>>>>> This is just to catch the case where IOMMUFD can call into read_and_clear()
>>>>> without dirty tracking enabled and without a bitmap structure to clear dirty
>>>>> bits -- in order to ensure a clean PTE data snapshot after start(). 
>>>>
>>>> Is that broken then?
>>>>
>>>
>>> It's not: The check errors out the caller ends up calling read-and-clear with a
>>> bitmap but without having started dirty tracking. the iopt_clear_dirty_data()
>>> passes a null bitmap, it goes through and it walks and clears the IOPTEs
>>> *without* recording them in the bitmap.
>>
>> It is not "without recording them in the bitmap", saying that is the
>> confusing thing. The purpose of that 'if' is to return -EINVAL if
>> dirty tracking is not turned on and we query the bitmap.
>>
> Right.
> 
>> More like this puts it in the common code and writes it in a more
>> straightforward way with better locking:
>>
> Yes, This snip you pasted would be the equivalent to the current way indeed.
> Looks good
> 
> I think I was trying too hard not to duplicate 'state of dirty tracking' between
> iommu driver and iommufd core that I unintendedly ended up convoluting with this
> check in the driver :/
> 

To this point above (...)

>> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
>> index 05fd9d3abf1b80..d116179809042d 100644
>> --- a/drivers/iommu/iommufd/io_pagetable.c
>> +++ b/drivers/iommu/iommufd/io_pagetable.c
>> @@ -536,7 +536,10 @@ int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
>>  		return ret;
>>  
>>  	down_read(&iopt->iova_rwsem);
>> -	ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
>> +	if (!iopt->dirty_tracking_enabled)
>> +		ret = -EINVAL;
>> +	else
>> +		ret = iommu_read_and_clear_dirty(domain, iopt, flags, bitmap);
>>  	up_read(&iopt->iova_rwsem);
>>  
>>  	return ret;
>> @@ -580,7 +583,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>>  	if (!ops)
>>  		return -EOPNOTSUPP;
>>  
>> -	down_read(&iopt->iova_rwsem);
>> +	down_write(&iopt->iova_rwsem);
>> +	if (iopt->dirty_tracking_enabled == enable) {
>> +		ret = 0;
>> +		goto out_unlock;
>> +	}
>>  

We have a similar check in set_dirty_tracking iommu op of intel/amd iommu
drivers that could be removed. Though I am not sure it's right to remove it even
as we move this up the stack

>>  	/* Clear dirty bits from PTEs to ensure a clean snapshot */
>>  	if (enable) {
>> @@ -590,9 +597,11 @@ int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>>  	}
>>  
>>  	ret = ops->set_dirty_tracking(domain, enable);
>> -
>> +	if (ret)
>> +		goto out_unlock;
>> +	iopt->dirty_tracking_enabled = enable;
>>  out_unlock:
>> -	up_read(&iopt->iova_rwsem);
>> +	up_write(&iopt->iova_rwsem);
>>  	return ret;
>>  }
>>  
>> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
>> index 991f864d1f9bc1..de3761e15cab54 100644
>> --- a/drivers/iommu/iommufd/iommufd_private.h
>> +++ b/drivers/iommu/iommufd/iommufd_private.h
>> @@ -52,6 +52,7 @@ struct io_pagetable {
>>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>>  	struct rb_root_cached reserved_itree;
>>  	u8 disable_large_pages;
>> +	u8 dirty_tracking_enabled;
>>  	unsigned long iova_alignment;
>>  };
>>  
> 

RE: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

[Tian, Kevin]

> From: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> 
> > -----Original Message-----
> > From: Tian, Kevin <kevin.tian@intel.com>
> > Sent: Wednesday, May 22, 2024 8:16 AM
> > To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>;
> > iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
> > Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> > jgg@nvidia.com; ryan.roberts@arm.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 v3 3/4] iommu/arm-smmu-v3: Add support for dirty
> tracking
> > in domain alloc
> >
> > > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > Sent: Tuesday, April 30, 2024 9:43 PM
> > >
> > > @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct
> > > arm_smmu_domain *smmu_domain,
> > >  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
> > >  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> > >
> > > +	pgtbl_cfg = (struct io_pgtable_cfg) {
> > > +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> > > +		.coherent_walk	= smmu->features &
> > > ARM_SMMU_FEAT_COHERENCY,
> > > +		.tlb		= &arm_smmu_flush_ops,
> > > +		.iommu_dev	= smmu->dev,
> > > +	};
> > > +
> > >  	switch (smmu_domain->stage) {
> > >  	case ARM_SMMU_DOMAIN_S1:
> > > -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> > > -		ias = min_t(unsigned long, ias, VA_BITS);
> > > -		oas = smmu->ias;
> > > +		unsigned long ias = (smmu->features &
> > > +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;
> > > +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
> > > +		pgtbl_cfg.oas = smmu->ias;
> > > +		if (enable_dirty)
> > > +			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
> >
> > why is dirty tracking considered as a quirk?
> 
> Yes, that is a bit unconventional. But this was discussed earlier and in
> SMMUv3
> driver the word "quirk" is considered in the broadest sense and there are
> precedent for this in the driver already.
> 
> From Robin:
> 
> "Indeed these features aren't decorative grooves on a piece of furniture,
> but in the case of io-pgtable we're merely using "quirk" in its broadest
> sense to imply something that differs from the baseline default
> behaviour - ARM_MTK_EXT, ARM_TTBR1 and ARM_OUTER_WBWA (or
> whatever it's
> called this week) are all just indicating extra hardware features
> entirely comparable to HTTU;..."
> 
> https://lore.kernel.org/linux-iommu/5ada4a8b-8852-f83c-040a-
> 9ef5dac51de2@arm.com/
> 

I see.

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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, May 22, 2024 8:39 PM
> 
> On Wed, May 22, 2024 at 07:19:05AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Sunday, May 12, 2024 8:09 PM
> > >
> > > On Tue, Apr 30, 2024 at 02:43:08PM +0100, Shameer Kolothum wrote:
> > >
> > > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-
> pgtable-
> > > arm.c
> > > > index da6cc52859ba..20ac0e833c7b 100644
> > > > --- a/drivers/iommu/io-pgtable-arm.c
> > > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > > @@ -433,6 +433,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_CLEAN;
> > >
> > > This seems a bit suboptimal, it means the HTTU will be generating
> > > dirty's before the tracking is turned on. As I understand it if the
> > > SMMU wants to write a dirty bit it has to do an atomic RMW to memory,
> > > so this would be a drag on baseline performance?
> > >
> > > Should this start out as dirty and let the enable flow clean it to
> > > turn it on?
> > >
> >
> > this appears to be good for other vendors too?
> 
> I thought Intel and AMD both had a per-table flag to turn on tracking
> and without that bit the dirties are not written back?
> 

yes, I misunderstood the original context. 

RE: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, May 23, 2024 1:51 AM
> 
> diff --git a/drivers/iommu/iommufd/iommufd_private.h
> b/drivers/iommu/iommufd/iommufd_private.h
> index 991f864d1f9bc1..de3761e15cab54 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -52,6 +52,7 @@ struct io_pagetable {
>  	/* IOVA that cannot be allocated, struct iopt_reserved */
>  	struct rb_root_cached reserved_itree;
>  	u8 disable_large_pages;
> +	u8 dirty_tracking_enabled;
>  	unsigned long iova_alignment;
>  };
> 

should it be a hwpt flag instead?