diff mbox series

[RFCv2,09/24] iommufd: Add IOMMU_HWPT_SET_DIRTY

Message ID 20230518204650.14541-10-joao.m.martins@oracle.com (mailing list archive)
State New, archived
Headers show
Series IOMMUFD Dirty Tracking | expand

Commit Message

Joao Martins May 18, 2023, 8:46 p.m. UTC
Every IOMMU driver should be able to implement the needed iommu domain ops
to control dirty tracking.

Connect a hw_pagetable to the IOMMU core dirty tracking ops, specifically
the ability to enable/disable dirty tracking on an IOMMU domain
(hw_pagetable id). To that end add an io_pagetable kernel API to toggle
dirty tracking:

* iopt_set_dirty_tracking(iopt, [domain], state)

The intended caller of this is via the hw_pagetable object that is created.

Internally we will ensure the leftover dirty state is cleared /right
before/ we start dirty tracking. This is also useful for iommu drivers
which may decide that dirty tracking is always-enabled without being
able to disable it via iommu domain op.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 drivers/iommu/iommufd/hw_pagetable.c    | 24 +++++++++++++++++
 drivers/iommu/iommufd/io_pagetable.c    | 36 +++++++++++++++++++++++++
 drivers/iommu/iommufd/iommufd_private.h | 11 ++++++++
 drivers/iommu/iommufd/main.c            |  3 +++
 include/uapi/linux/iommufd.h            | 27 +++++++++++++++++++
 5 files changed, 101 insertions(+)

Comments

Jason Gunthorpe May 19, 2023, 1:49 p.m. UTC | #1
On Thu, May 18, 2023 at 09:46:35PM +0100, Joao Martins wrote:
> +int iopt_set_dirty_tracking(struct io_pagetable *iopt,
> +			    struct iommu_domain *domain, bool enable)
> +{
> +	const struct iommu_domain_ops *ops = domain->ops;
> +	struct iommu_dirty_bitmap dirty;
> +	struct iommu_iotlb_gather gather;
> +	struct iopt_area *area;
> +	int ret = 0;
> +
> +	if (!ops->set_dirty_tracking)
> +		return -EOPNOTSUPP;
> +
> +	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
> +
> +	down_write(&iopt->iova_rwsem);
> +	for (area = iopt_area_iter_first(iopt, 0, ULONG_MAX);
> +	     area && enable;

That's a goofy way to write this.. put this in a function and don't
call it if enable is not set.

Why is this down_write() ?

You can see that this locking already prevents racing dirty read with
domain unmap.

This domain cannot be removed from the iopt eg through
iopt_table_remove_domain() because this is holding the object
reference on the hwpt

The area cannot be unmapped because this is holding the
&iopt->iova_rwsem

There is no other way to call unmap..

You do have to check that area->pages != NULL though

Jason
Joao Martins May 19, 2023, 2:21 p.m. UTC | #2
On 19/05/2023 14:49, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 09:46:35PM +0100, Joao Martins wrote:
>> +int iopt_set_dirty_tracking(struct io_pagetable *iopt,
>> +			    struct iommu_domain *domain, bool enable)
>> +{
>> +	const struct iommu_domain_ops *ops = domain->ops;
>> +	struct iommu_dirty_bitmap dirty;
>> +	struct iommu_iotlb_gather gather;
>> +	struct iopt_area *area;
>> +	int ret = 0;
>> +
>> +	if (!ops->set_dirty_tracking)
>> +		return -EOPNOTSUPP;
>> +
>> +	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
>> +
>> +	down_write(&iopt->iova_rwsem);
>> +	for (area = iopt_area_iter_first(iopt, 0, ULONG_MAX);
>> +	     area && enable;
> 
> That's a goofy way to write this.. put this in a function and don't
> call it if enable is not set.
> 
I'll move this into a iopt_clear_dirty_data()

There's also another less positive aspect here on the implicit assumption I made
that a dirty::bitmap == NULL means we do not care about recording dirty bitmap.
Which is OK. But I use that field being NULL as means to ignore the iommu driver
status control to just clear the remnant dirty bits that could have been done
until we disable dirty tracking. I'm thinking in making this into a flags value,
but keeping internally only, I am not sure this should be exposed to userspace.

> Why is this down_write() ?
> 
> You can see that this locking already prevents racing dirty read with
> domain unmap.
> 
> This domain cannot be removed from the iopt eg through
> iopt_table_remove_domain() because this is holding the object
> reference on the hwpt
> 
> The area cannot be unmapped because this is holding the
> &iopt->iova_rwsem
> 
> There is no other way to call unmap..

down_read(&iopt->iova_rwsem) is more approprite;
iopt_read_and_clear_dirty_data() does so already.

But I should iterating over areas there too, which I wrongly not doing.

> You do have to check that area->pages != NULL though

OK
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 4f0b72737ae2..7acbd88d05b7 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -200,3 +200,27 @@  int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
 	iommufd_put_object(&idev->obj);
 	return rc;
 }
+
+int iommufd_hwpt_set_dirty(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_hwpt_set_dirty *cmd = ucmd->cmd;
+	struct iommufd_hw_pagetable *hwpt;
+	struct iommufd_ioas *ioas;
+	int rc = -EOPNOTSUPP;
+	bool enable;
+
+	hwpt = iommufd_get_hwpt(ucmd, cmd->hwpt_id);
+	if (IS_ERR(hwpt))
+		return PTR_ERR(hwpt);
+
+	if (!hwpt->enforce_dirty)
+		return -EOPNOTSUPP;
+
+	ioas = hwpt->ioas;
+	enable = cmd->flags & IOMMU_DIRTY_TRACKING_ENABLED;
+
+	rc = iopt_set_dirty_tracking(&ioas->iopt, hwpt->domain, enable);
+
+	iommufd_put_object(&hwpt->obj);
+	return rc;
+}
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
index 187626e5f2bc..01adb0f7e4d0 100644
--- a/drivers/iommu/iommufd/io_pagetable.c
+++ b/drivers/iommu/iommufd/io_pagetable.c
@@ -479,6 +479,42 @@  int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
 	down_read(&iopt->iova_rwsem);
 	ret = iommu_read_and_clear_dirty(domain, flags, bitmap);
 	up_read(&iopt->iova_rwsem);
+
+	return ret;
+}
+
+int iopt_set_dirty_tracking(struct io_pagetable *iopt,
+			    struct iommu_domain *domain, bool enable)
+{
+	const struct iommu_domain_ops *ops = domain->ops;
+	struct iommu_dirty_bitmap dirty;
+	struct iommu_iotlb_gather gather;
+	struct iopt_area *area;
+	int ret = 0;
+
+	if (!ops->set_dirty_tracking)
+		return -EOPNOTSUPP;
+
+	iommu_dirty_bitmap_init(&dirty, NULL, &gather);
+
+	down_write(&iopt->iova_rwsem);
+	for (area = iopt_area_iter_first(iopt, 0, ULONG_MAX);
+	     area && enable;
+	     area = iopt_area_iter_next(area, 0, ULONG_MAX)) {
+		ret = ops->read_and_clear_dirty(domain,
+						iopt_area_iova(area),
+						iopt_area_length(area), 0,
+						&dirty);
+		if (ret)
+			goto out_unlock;
+	}
+
+	iommu_iotlb_sync(domain, &gather);
+
+	ret = ops->set_dirty_tracking(domain, enable);
+
+out_unlock:
+	up_write(&iopt->iova_rwsem);
 	return ret;
 }
 
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 2259b15340e4..e902197a6a42 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -10,6 +10,7 @@ 
 #include <linux/uaccess.h>
 #include <linux/iommu.h>
 #include <linux/iova_bitmap.h>
+#include <uapi/linux/iommufd.h>
 
 struct iommu_domain;
 struct iommu_group;
@@ -83,6 +84,8 @@  int iopt_read_and_clear_dirty_data(struct io_pagetable *iopt,
 				   struct iommu_domain *domain,
 				   unsigned long flags,
 				   struct iommufd_dirty_data *bitmap);
+int iopt_set_dirty_tracking(struct io_pagetable *iopt,
+			    struct iommu_domain *domain, bool enable);
 
 void iommufd_access_notify_unmap(struct io_pagetable *iopt, unsigned long iova,
 				 unsigned long length);
@@ -267,6 +270,14 @@  struct iommufd_hw_pagetable {
 	struct list_head hwpt_item;
 };
 
+static inline struct iommufd_hw_pagetable *iommufd_get_hwpt(
+					struct iommufd_ucmd *ucmd, u32 id)
+{
+	return container_of(iommufd_get_object(ucmd->ictx, id,
+					       IOMMUFD_OBJ_HW_PAGETABLE),
+			    struct iommufd_hw_pagetable, obj);
+}
+int iommufd_hwpt_set_dirty(struct iommufd_ucmd *ucmd);
 struct iommufd_hw_pagetable *
 iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			   struct iommufd_device *idev, bool immediate_attach,
diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c
index 3932fe26522b..8c4640df0547 100644
--- a/drivers/iommu/iommufd/main.c
+++ b/drivers/iommu/iommufd/main.c
@@ -277,6 +277,7 @@  union ucmd_buffer {
 	struct iommu_ioas_unmap unmap;
 	struct iommu_option option;
 	struct iommu_vfio_ioas vfio_ioas;
+	struct iommu_hwpt_set_dirty set_dirty;
 #ifdef CONFIG_IOMMUFD_TEST
 	struct iommu_test_cmd test;
 #endif
@@ -318,6 +319,8 @@  static const struct iommufd_ioctl_op iommufd_ioctl_ops[] = {
 		 val64),
 	IOCTL_OP(IOMMU_VFIO_IOAS, iommufd_vfio_ioas, struct iommu_vfio_ioas,
 		 __reserved),
+	IOCTL_OP(IOMMU_HWPT_SET_DIRTY, iommufd_hwpt_set_dirty,
+		 struct iommu_hwpt_set_dirty, __reserved),
 #ifdef CONFIG_IOMMUFD_TEST
 	IOCTL_OP(IOMMU_TEST_CMD, iommufd_test, struct iommu_test_cmd, last),
 #endif
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 1cd9c54d0f64..85498f14b3ae 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -46,6 +46,7 @@  enum {
 	IOMMUFD_CMD_OPTION,
 	IOMMUFD_CMD_VFIO_IOAS,
 	IOMMUFD_CMD_HWPT_ALLOC,
+	IOMMUFD_CMD_HWPT_SET_DIRTY,
 };
 
 /**
@@ -379,4 +380,30 @@  struct iommu_hwpt_alloc {
 	__u32 __reserved;
 };
 #define IOMMU_HWPT_ALLOC _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_ALLOC)
+
+/**
+ * enum iommufd_set_dirty_flags - Flags for steering dirty tracking
+ * @IOMMU_DIRTY_TRACKING_DISABLED: Disables dirty tracking
+ * @IOMMU_DIRTY_TRACKING_ENABLED: Enables dirty tracking
+ */
+enum iommufd_hwpt_set_dirty_flags {
+	IOMMU_DIRTY_TRACKING_DISABLED = 0,
+	IOMMU_DIRTY_TRACKING_ENABLED = 1,
+};
+
+/**
+ * struct iommu_hwpt_set_dirty - ioctl(IOMMU_HWPT_SET_DIRTY)
+ * @size: sizeof(struct iommu_hwpt_set_dirty)
+ * @flags: Flags to control dirty tracking status.
+ * @hwpt_id: HW pagetable ID that represents the IOMMU domain.
+ *
+ * Toggle dirty tracking on an HW pagetable.
+ */
+struct iommu_hwpt_set_dirty {
+	__u32 size;
+	__u32 flags;
+	__u32 hwpt_id;
+	__u32 __reserved;
+};
+#define IOMMU_HWPT_SET_DIRTY _IO(IOMMUFD_TYPE, IOMMUFD_CMD_HWPT_SET_DIRTY)
 #endif