iommufd: Add vIOMMU infrastructure (Part-3: vEVENTQ)

There is no need to keep them in the header. The vEVENTQ version of these two functions will turn out to be a different implementation and will not share with this fault version. Thus, move them out of the header.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h | 25 -------------------------
 drivers/iommu/iommufd/fault.c           | 25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 25 deletions(-)

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

There is no need to keep them in the header. The vEVENTQ version of these two functions will turn out to be a different implementation and will not share with this fault version. Thus, move them out of the header.

Signed-off-by Nicolin Chen <[email protected]>

The fault object was designed exclusively for hwpt's IO page faults (PRI). But its queue implementation can be reused for other purposes too, such as hardware IRQ and event injections to user space.

Meanwhile, a fault object holds a list of faults. So it's more accurate to call it a "fault queue". Combining the reusing idea above, abstract a new iommufd_eventq as a common structure embedded into struct iommufd_fault, similar to hwpt_paging holding a common hwpt.

Add a common iommufd_eventq_ops and iommufd_eventq_init to prepare for an IOMMUFD_OBJ_VEVENTQ (vIOMMU Event Queue).

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/iommufd/iommufd_private.h |  28 ++++--
 drivers/iommu/iommufd/fault.c           | 111 +++++++++++++-----------
 drivers/iommu/iommufd/hw_pagetable.c    |   6 +-
 3 files changed, 82 insertions(+), 63 deletions(-)

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

drivers/iommu/iommufd/iommufd_private.h +3 -0

+
+/*
+ * An iommufd_veventq object represents an interface to deliver vIOMMU

events to
+ * the user space. It is created/destroyed by the user space and associated with
+ * vIOMMU object(s) during the allocations.

+static inline void iommufd_vevent_handler(struct iommufd_veventq *veventq,

drivers/iommu/iommufd/iommufd_private.h +6 -0

+					  struct iommufd_vevent *vevent)
+{
+	struct iommufd_eventq *eventq = &veventq->common;
+
+	/*
+	 * Remove the overflow node and add the new node at the same

time. Note

(quoted diff) +1 -0

+	 * it is possible that vevent == &veventq->overflow for sequence

update

include/uapi/linux/iommufd.h +3 -0

+
+/**
+ * struct iommufd_vevent_header - Virtual Event Header for a vEVENTQ

Status
+ * @flags: Combination of enum iommu_veventq_flag
+ * @sequence: The sequence index of a vEVENT in the vEVENTQ, with a range of
+ * [0, INT_MAX] where the following index of INT_MAX is 0
+ * @__reserved: Must be 0
+ *
+ * Each iommufd_vevent_header reports a sequence index of the following vEVENT:
+ * ---------------------------------------------------------------------------
+ * || header0 {sequence=0} | data0 | header1 {sequence=1} | data1 |...| dataN ||
+ * ---------------------------------------------------------------------------
+ * And this sequence index is expected to be monotonic to the sequence index of
+ * the previous vEVENT. If two adjacent sequence indexes has a delta larger than
+ * 1, it indicates that an overflow occurred to the vEVENTQ and that delta - 1
+ * number of vEVENTs lost due to the overflow (e.g. two lost vEVENTs):
+ * ---------------------------------------------------------------------------
+ * || ... | header3 {sequence=3} | data3 | header6 {sequence=6} | data6 | ...

||
+ *  ---------------------------------------------------------------------------

+ * If an overflow occurred to the tail of the vEVENTQ and there is no following
+ * vEVENT providing the next sequence index, a special overflow header would be
+ * added to the tail of the vEVENTQ, where there would be no more type- specific
+ * data following the vEVENTQ:
+ * ---------------------------------------------------------------------------
+ * ||...| header3 {sequence=3} | data4 | header4 {flags=OVERFLOW, sequence=4} ||
+ * ---------------------------------------------------------------------------
+ */

Quoted 9 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

drivers/iommu/iommufd/iommufd_private.h +3 -0

+
+/*
+ * An iommufd_veventq object represents an interface to deliver vIOMMU

events to
+ * the user space. It is created/destroyed by the user space and associated with
+ * vIOMMU object(s) during the allocations.

s/object(s)/object/, given the eventq cannot be shared between vIOMMUs.

Quoted 11 lines

+static inline void iommufd_vevent_handler(struct iommufd_veventq *veventq,

drivers/iommu/iommufd/iommufd_private.h +6 -0

+					  struct iommufd_vevent *vevent)
+{
+	struct iommufd_eventq *eventq = &veventq->common;
+
+	/*
+	 * Remove the overflow node and add the new node at the same

time. Note

(quoted diff) +1 -0

+	 * it is possible that vevent == &veventq->overflow for sequence

update

drivers/iommu/iommufd/iommufd_private.h static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev, · line 583

Quoted 6 lines

drivers/iommu/iommufd/iommufd_private.h +6 -0

+	 */
+	spin_lock(&eventq->lock);
+	if (veventq->overflow.on_list) {
+		list_del(&veventq->overflow.node);
+		veventq->overflow.on_list = false;
+	}

We can save one field 'on_list' in every entry by:

	if (list_is_last(&veventq->overflow.node, &eventq->deliver))
		list_del(&veventq->overflow.node);

include/uapi/linux/iommufd.h struct iommu_ioas_change_process { · line 1056

Quoted 5 lines

include/uapi/linux/iommufd.h +5 -0

+struct iommufd_vevent_header {
+	__aligned_u64 flags;
+	__u32 sequence;
+	__u32 __reserved;
+};

Is there a reason that flags must be u64? At a glance all flags fields (except the one in iommu_hwpt_vtd_s1) in iommufd uAPIs are u32 which can cut the size of the header by half...

Quoted 4 lines

drivers/iommu/iommufd/eventq.c +3 -0

+void iommufd_veventq_abort(struct iommufd_object *obj)
+{
+	struct iommufd_eventq *eventq =

drivers/iommu/iommufd/eventq.c static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b · line 399

Quoted 10 lines

drivers/iommu/iommufd/eventq.c +10 -0

+		container_of(obj, struct iommufd_eventq, obj);
+	struct iommufd_veventq *veventq = eventq_to_veventq(eventq);
+	struct iommufd_viommu *viommu = veventq->viommu;
+	struct iommufd_vevent *cur, *next;
+
+	lockdep_assert_held_write(&viommu->veventqs_rwsem);
+
+	list_for_each_entry_safe(cur, next, &eventq->deliver, node) {
+		list_del(&cur->node);
+		kfree(cur);

kfree() doesn't apply to the overflow node.

otherwise it looks good to me:

Reviewed-by Kevin Tian <[email protected]>

drivers/iommu/iommufd/eventq.c +9 -0

+static struct iommufd_vevent *
+iommufd_veventq_deliver_fetch(struct iommufd_veventq *veventq)
+{
+	struct iommufd_eventq *eventq = &veventq->common;
+	struct list_head *list = &eventq->deliver;
+	struct iommufd_vevent *vevent = NULL;
+
+	spin_lock(&eventq->lock);
+	if (!list_empty(list)) {

drivers/iommu/iommufd/iommufd_private.h static inline int iommufd_hwpt_replace_device(struct iommufd_device *idev, · line 591

Quoted 8 lines · Nicolin Chen

drivers/iommu/iommufd/iommufd_private.h +8 -0

+	list_add_tail(&vevent->node, &eventq->deliver);
+	vevent->on_list = true;
+	vevent->header.sequence = atomic_read(&veventq->sequence);
+	if (atomic_read(&veventq->sequence) == INT_MAX)
+		atomic_set(&veventq->sequence, 0);
+	else
+		atomic_inc(&veventq->sequence);
+	spin_unlock(&eventq->lock);

This is all locked, we don't need veventq->sequence to be an atomic?

The bounding can be done with some simple math:

  veventq->sequence = (veventq->sequence + 1) & INT_MAX;

Quoted 15 lines

drivers/iommu/iommufd/eventq.c +15 -0

+static struct iommufd_vevent *
+iommufd_veventq_deliver_fetch(struct iommufd_veventq *veventq)
+{
+	struct iommufd_eventq *eventq = &veventq->common;
+	struct list_head *list = &eventq->deliver;
+	struct iommufd_vevent *vevent = NULL;
+
+	spin_lock(&eventq->lock);
+	if (!list_empty(list)) {
+		vevent = list_first_entry(list, struct iommufd_vevent, node);
+		list_del(&vevent->node);
+		vevent->on_list = false;
+	}
+	/* Make a copy of the overflow node for copy_to_user */
+	if (vevent == &veventq->overflow) {

drivers/iommu/iommufd/eventq.c static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b · line 434

Quoted 4 lines

drivers/iommu/iommufd/eventq.c +4 -0

+		vevent = kzalloc(sizeof(*vevent), GFP_ATOMIC);
+		if (vevent)
+			memcpy(vevent, &veventq->overflow, sizeof(*vevent));
+	}

This error handling is wonky, if we can't allocate then we shouldn't have done the list_del. Just return NULL which will cause iommufd_veventq_fops_read() to exist and userspace will try again.

drivers/iommu/iommufd/eventq.c static int iommufd_eventq_fops_release(struct inode *inode, struct file *filep) · line 537

Quoted 8 lines

drivers/iommu/iommufd/eventq.c +4 -0

@@ -403,6 +531,10 @@ static int iommufd_eventq_fops_release(struct inode *inode, struct file *filep)
 {
 	struct iommufd_eventq *eventq = filep->private_data;
 
+	if (eventq->obj.type == IOMMUFD_OBJ_VEVENTQ) {
+		atomic_set(&eventq_to_veventq(eventq)->sequence, 0);
+		atomic_set(&eventq_to_veventq(eventq)->num_events, 0);
+	}

Why? We are about to free the memory?

Quoted 6 lines

drivers/iommu/iommufd/eventq.c +6 -0

+int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_veventq_alloc *cmd = ucmd->cmd;
+	struct iommufd_veventq *veventq;
+	struct iommufd_viommu *viommu;
+	int fdno;

drivers/iommu/iommufd/eventq.c int iommufd_fault_iopf_handler(struct iopf_group *group) · line 658

Quoted 6 lines

drivers/iommu/iommufd/eventq.c +6 -0

+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VEVENTQ_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->veventq_depth)
+		return -EINVAL;

Check __reserved for 0 too

I think we can do:

	if (!list_empty(list)) {
		struct iommufd_vevent *next;

		next = list_first_entry(list, struct iommufd_vevent, node);
		if (next == &veventq->overflow) {
			/* Make a copy of the overflow node for copy_to_user */
			vevent = kzalloc(sizeof(*vevent), GFP_ATOMIC);
			if (!vevent)
				goto out_unlock;
		}
		list_del(&next->node);
		if (vevent)
			memcpy(vevent, next, sizeof(*vevent));
		else
			vevent = next;
	}

Quoted 6 lines

Quoted 6 lines

drivers/iommu/iommufd/eventq.c +6 -0

+int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_veventq_alloc *cmd = ucmd->cmd;
+	struct iommufd_veventq *veventq;
+	struct iommufd_viommu *viommu;
+	int fdno;

Quoted 8 lines

drivers/iommu/iommufd/eventq.c int iommufd_fault_iopf_handler(struct iopf_group *group) · line 658

Quoted 6 lines

drivers/iommu/iommufd/eventq.c +6 -0

+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VEVENTQ_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->veventq_depth)
+		return -EINVAL;

Check __reserved for 0 too

Kevin is suggesting a 32-bit flag field, so I think we can drop the __reserved in that case.

Quoted 15 lines · Nicolin Chen

Quoted 14 lines

drivers/iommu/iommufd/eventq.c int iommufd_fault_iopf_handler(struct iopf_group *group) · line 658

Quoted 12 lines

drivers/iommu/iommufd/eventq.c +12 -0

+int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
+{
+	struct iommu_veventq_alloc *cmd = ucmd->cmd;
+	struct iommufd_veventq *veventq;
+	struct iommufd_viommu *viommu;
+	int fdno;
+	int rc;
+
+	if (cmd->flags || cmd->type == IOMMU_VEVENTQ_TYPE_DEFAULT)
+		return -EOPNOTSUPP;
+	if (!cmd->veventq_depth)
+		return -EINVAL;

Check __reserved for 0 too

Quoted 2 lines

Kevin is suggesting a 32-bit flag field, so I think we can drop the __reserved in that case.

Those are different structs?

This is a reverse search v.s. iommufd_viommu_find_dev, as drivers may want to convert a struct device pointer (physical) to its virtual device ID for an event injection to the user space VM.

Again, this avoids exposing more core structures to the drivers, than the iommufd_viommu alone.

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 include/linux/iommufd.h        |  9 +++++++++
 drivers/iommu/iommufd/driver.c | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Quoted 7 lines · Nicolin Chen

This is a reverse search v.s. iommufd_viommu_find_dev, as drivers may want to convert a struct device pointer (physical) to its virtual device ID for an event injection to the user space VM.

Again, this avoids exposing more core structures to the drivers, than the iommufd_viommu alone.

Quoted 7 lines

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 include/linux/iommufd.h        |  9 +++++++++
 drivers/iommu/iommufd/driver.c | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Reviewed-by Jason Gunthorpe <[email protected]>

drivers/iommu/iommufd/driver.c struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu, · line 66

Quoted 4 lines

drivers/iommu/iommufd/driver.c +4 -0

+	xa_lock(&viommu->vdevs);
+	xa_for_each(&viommu->vdevs, index, vdev) {
+		if (vdev->dev == dev) {
+			*vdev_id = (unsigned long)vdev->id;

I don't think we need this cast

Quoted 16 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

This is a reverse search v.s. iommufd_viommu_find_dev, as drivers may want to convert a struct device pointer (physical) to its virtual device ID for an event injection to the user space VM.

Again, this avoids exposing more core structures to the drivers, than the iommufd_viommu alone.

Quoted 7 lines

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 include/linux/iommufd.h        |  9 +++++++++
 drivers/iommu/iommufd/driver.c | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Quoted 8 lines

Reviewed-by Jason Gunthorpe <[email protected]>

drivers/iommu/iommufd/driver.c struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu, · line 66

Quoted 4 lines

drivers/iommu/iommufd/driver.c +4 -0

+	xa_lock(&viommu->vdevs);
+	xa_for_each(&viommu->vdevs, index, vdev) {
+		if (vdev->dev == dev) {
+			*vdev_id = (unsigned long)vdev->id;

I don't think we need this cast

The left side is ulong for xarray index, while the right side is __aligned_u64 for uAPI. Could there be a gcc warning when somebody builds the kernel having a BITS_PER_LONG=32?

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

Similar to iommu_report_device_fault, this allows IOMMU drivers to report vIOMMU events from threaded IRQ handlers to user space hypervisors.

Reviewed-by Lu Baolu <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

drivers/iommu/iommufd/driver.c +18 -0

+int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
+				enum iommu_veventq_type type, void *event_data,
+				size_t data_len)
+{
+	struct iommufd_veventq *veventq;
+	struct iommufd_vevent *vevent;
+	int rc = 0;
+
+	if (WARN_ON_ONCE(!data_len || !event_data))
+		return -EINVAL;
+
+	down_read(&viommu->veventqs_rwsem);
+
+	veventq = iommufd_viommu_find_veventq(viommu, type);
+	if (!veventq) {
+		rc = -EOPNOTSUPP;
+		goto out_unlock_veventqs;
+	}

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 18, 2025 11:36 PM

Review

Quoted 2 lines · Nicolin Chen

(quoted diff) +2 -0

+int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
+				enum iommu_veventq_type type, void

*event_data,

Quoted 13 lines

drivers/iommu/iommufd/driver.c +13 -0

+				size_t data_len)
+{
+	struct iommufd_veventq *veventq;
+	struct iommufd_vevent *vevent;
+	int rc = 0;
+
+	if (WARN_ON_ONCE(!data_len || !event_data))
+		return -EINVAL;
+
+	down_read(&viommu->veventqs_rwsem);
+
+	veventq = iommufd_viommu_find_veventq(viommu, type);
+	if (!veventq) {

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,

Quoted 10 lines

drivers/iommu/iommufd/driver.c +10 -0

+		rc = -EOPNOTSUPP;
+		goto out_unlock_veventqs;
+	}
+
+	if (atomic_read(&veventq->num_events) == veventq->depth) {
+		vevent = &veventq->overflow;
+		goto out_set_header;
+	}
+
+	vevent = kmalloc(struct_size(vevent, event_data, data_len),

GFP_KERNEL);

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 107

Quoted 3 lines

drivers/iommu/iommufd/driver.c +3 -0

+	if (!vevent) {
+		rc = -ENOMEM;
+		goto out_unlock_veventqs;

This should record an overflow too

Quoted 31 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 18, 2025 11:36 PM

Review

Quoted 2 lines · Nicolin Chen

(quoted diff) +2 -0

+int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
+				enum iommu_veventq_type type, void

*event_data,

Quoted 13 lines

drivers/iommu/iommufd/driver.c +13 -0

+				size_t data_len)
+{
+	struct iommufd_veventq *veventq;
+	struct iommufd_vevent *vevent;
+	int rc = 0;
+
+	if (WARN_ON_ONCE(!data_len || !event_data))
+		return -EINVAL;
+
+	down_read(&viommu->veventqs_rwsem);
+
+	veventq = iommufd_viommu_find_veventq(viommu, type);
+	if (!veventq) {

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,

Quoted 10 lines

drivers/iommu/iommufd/driver.c +10 -0

+		rc = -EOPNOTSUPP;
+		goto out_unlock_veventqs;
+	}
+
+	if (atomic_read(&veventq->num_events) == veventq->depth) {
+		vevent = &veventq->overflow;
+		goto out_set_header;
+	}
+
+	vevent = kmalloc(struct_size(vevent, event_data, data_len),

GFP_KERNEL);

Quoted 6 lines

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 107

Quoted 3 lines

drivers/iommu/iommufd/driver.c +3 -0

+	if (!vevent) {
+		rc = -ENOMEM;
+		goto out_unlock_veventqs;

This should record an overflow too

In that case probably we want to rename 'overflow' to 'lost_event' which counts lost events for whatever reasons (overflow, oom, etc.)

From: Nicolin Chen <[email protected]>
Sent: Friday, February 21, 2025 5:16 AM

Quoted 5 lines · Kevin

Quoted 5 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, February 18, 2025 11:36 PM

Quoted 1 line · Nicolin Chen

+int iommufd_viommu_report_event(struct iommufd_viommu

*viommu,

Quoted 33 lines

Quoted 26 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+				enum iommu_veventq_type type, void

*event_data,

Quoted 13 lines

drivers/iommu/iommufd/driver.c +13 -0

+				size_t data_len)
+{
+	struct iommufd_veventq *veventq;
+	struct iommufd_vevent *vevent;
+	int rc = 0;
+
+	if (WARN_ON_ONCE(!data_len || !event_data))
+		return -EINVAL;
+
+	down_read(&viommu->veventqs_rwsem);
+
+	veventq = iommufd_viommu_find_veventq(viommu, type);
+	if (!veventq) {

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,

Quoted 10 lines

drivers/iommu/iommufd/driver.c +10 -0

+		rc = -EOPNOTSUPP;
+		goto out_unlock_veventqs;
+	}
+
+	if (atomic_read(&veventq->num_events) == veventq->depth) {
+		vevent = &veventq->overflow;
+		goto out_set_header;
+	}
+
+	vevent = kmalloc(struct_size(vevent, event_data, data_len),

GFP_KERNEL);

Quoted 6 lines

drivers/iommu/iommufd/driver.c int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 107

Quoted 3 lines

drivers/iommu/iommufd/driver.c +3 -0

+	if (!vevent) {
+		rc = -ENOMEM;
+		goto out_unlock_veventqs;

This should record an overflow too

Quoted 2 lines

In that case probably we want to rename 'overflow' to 'lost_event' which counts lost events for whatever reasons (overflow, oom, etc.)

Naming-wise, I think it may sound better to call the overflow node just an 'exception' that concludes with lost eventsfor different reasons.

A complication is that this 'exception' would give userspace a very implicit reason as the node just report the number of lost events v.s. providing the reason to each lost event.

With this, maybe the IOMMU_VEVENTQ_FLAG_OVERFLOW in the uAPI should
be just IOMMU_VEVENTQ_FLAG_EXCEPTION? Any better idea?

With that in mind we don't need provide reason for the static node, and IOMMU_EVENTQ_FLAG_LOST_EVENTS sounds closer to the real intention.

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

When attaching a device to a vIOMMU-based nested domain, vdev_id must be
present. Add a piece of code hard-requesting it, preparing for a vEVENTQ support in the following patch. Then, update the TEST_F.

A HWPT-based nested domain will return a NULL new_viommu, thus no such a
vDEVICE requirement.

Signed-off-by Nicolin Chen <[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

The handler will get vDEVICE object from the given mdev and convert it to its per-vIOMMU virtual ID to mimic a real IOMMU driver.

Signed-off-by Nicolin Chen <[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

Trigger vEVENTs by feeding an idev ID and validating the returned output virt_ids whether they equal to the value that was set to the vDEVICE.

Signed-off-by Nicolin Chen <[email protected]>

Documentation/userspace-api/iommufd.rst +15 -0

index 70289d6815d2..b0df15865dec 100644
@@ -63,6 +63,13 @@ Following IOMMUFD objects are exposed to userspace:
   space usually has mappings from guest-level I/O virtual addresses to guest-
   level physical addresses.
 
+- IOMMUFD_FAULT, representing a software queue for an HWPT reporting IO page
+  faults using the IOMMU HW's PRI (Page Request Interface). This queue object
+  provides user space an FD to poll the page fault events and also to respond
+  to those events. A FAULT object must be created first to get a fault_id that
+  could be then used to allocate a fault-enabled HWPT via the IOMMU_HWPT_ALLOC
+  command by setting the IOMMU_HWPT_FAULT_ID_VALID bit in its flags field.
+
 - IOMMUFD_OBJ_VIOMMU, representing a slice of the physical IOMMU instance,
   passed to or shared with a VM. It may be some HW-accelerated virtualization
   features and some SW resources used by the VM. For examples:

@@ -109,6 +116,14 @@ Following IOMMUFD objects are exposed to userspace:
   vIOMMU, which is a separate ioctl call from attaching the same device to an
   HWPT_PAGING that the vIOMMU holds.
 
+- IOMMUFD_OBJ_VEVENTQ, representing a software queue for a vIOMMU to report its
+  events such as translation faults occurred to a nested stage-1 (excluding I/O
+  page faults that should go through IOMMUFD_OBJ_FAULT) and HW-specific events.
+  This queue object provides user space an FD to poll/read the vIOMMU events. A
+  vIOMMU object must be created first to get its viommu_id, which could be then
+  used to allocate a vEVENTQ. Each vIOMMU can support multiple types of vEVENTS,
+  but is confined to one vEVENTQ per vEVENTQ type.
+
 All user-visible objects are destroyed via the IOMMU_DESTROY uAPI.
 
 The diagrams below show relationships between user-visible objects and kernel

With the introduction of the new objects, update the doc to reflect that.

Reviewed-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Signed-off-by Nicolin Chen <[email protected]>

---
 Documentation/userspace-api/iommufd.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_attach_dev_ste(struct iommu_domain *domain, · line 3181

Quoted 11 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9 -0

+	int ret;
 	struct arm_smmu_ste ste;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_attach_state state = {
+		.master = master,
+	};
+
+	ret = arm_smmu_attach_prepare_vmaster(&state, domain);
+	if (ret)
+		return ret;
+	arm_smmu_attach_commit_vmaster(&state);

I think you should make this into just a arm_smmu_clear_vmaster() with less complication..

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

Aside from the IOPF framework, iommufd provides an additional pathway to report hardware events, via the vEVENTQ of vIOMMU infrastructure.

Define an iommu_vevent_arm_smmuv3 uAPI structure, and report stage-1 events
in the threaded IRQ handler.

Signed-off-by Nicolin Chen <[email protected]>

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

 	mutex_lock(&smmu->streams_mutex);

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_handle_event(struct arm_smmu_device *smmu, · line 1832

Quoted 3 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

@@ -1831,31 +1831,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu,
 		return -EOPNOTSUPP;
 	}

There is still the filter at the top:

	switch (event->id) {
	case EVT_ID_TRANSLATION_FAULT:
	case EVT_ID_ADDR_SIZE_FAULT:
	case EVT_ID_ACCESS_FAULT:
	case EVT_ID_PERMISSION_FAULT:
		break;
	default:
		return -EOPNOTSUPP;
	}

Is that right here or should more event types be forwarded to the guest?

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

 	mutex_lock(&smmu->streams_mutex);

[..]

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_handle_event(struct arm_smmu_device *smmu, · line 1870

Quoted 5 lines

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4 -1

-	ret = iommu_report_device_fault(master->dev, &fault_evt);
+	if (event->stall) {
+		ret = iommu_report_device_fault(master->dev, &fault_evt);
+	} else {
+		down_read(&master->vmaster_rwsem);

This already holds the streams_mutex across all of this, do you think we should get rid of the vmaster_rwsem and hold the streams_mutex on write instead?

Quoted 18 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_handle_event(struct arm_smmu_device *smmu, · line 1832

Quoted 3 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

@@ -1831,31 +1831,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu,
 		return -EOPNOTSUPP;
 	}

There is still the filter at the top:

	switch (event->id) {
	case EVT_ID_TRANSLATION_FAULT:
	case EVT_ID_ADDR_SIZE_FAULT:
	case EVT_ID_ACCESS_FAULT:
	case EVT_ID_PERMISSION_FAULT:
		break;
	default:
		return -EOPNOTSUPP;
	}

Quoted 2 lines

Is that right here or should more event types be forwarded to the guest?

That doesn't seem to be right. Something like EVT_ID_BAD_CD_CONFIG should be forwarded too. I will go through the list.

Quoted 3 lines

This already holds the streams_mutex across all of this, do you think we should get rid of the vmaster_rwsem and hold the streams_mutex on write instead?

They are per master v.s. per smmu. The latter one would make master commits/attaches exclusive, which feels unnecessary to me, although it would make the code here slightly cleaner..

Quoted 19 lines · Nicolin Chen

Quoted 18 lines · Jason Gunthorpe

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_handle_event(struct arm_smmu_device *smmu, · line 1832

Quoted 3 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0 -0

@@ -1831,31 +1831,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu,
 		return -EOPNOTSUPP;
 	}

There is still the filter at the top:

	switch (event->id) {
	case EVT_ID_TRANSLATION_FAULT:
	case EVT_ID_ADDR_SIZE_FAULT:
	case EVT_ID_ACCESS_FAULT:
	case EVT_ID_PERMISSION_FAULT:
		break;
	default:
		return -EOPNOTSUPP;
	}

Quoted 5 lines

Quoted 2 lines

Is that right here or should more event types be forwarded to the guest?

That doesn't seem to be right. Something like EVT_ID_BAD_CD_CONFIG should be forwarded too. I will go through the list.

I think the above should decode into a 'faultable' path because they all decode to something with an IOVA

The rest should decode to things that include a SID and the SID decode should always be forwarded to the VM. Maybe there are small exclusions, but generally that is how I would see it..

Quoted 7 lines

Quoted 3 lines

This already holds the streams_mutex across all of this, do you think we should get rid of the vmaster_rwsem and hold the streams_mutex on write instead?

They are per master v.s. per smmu. The latter one would make master commits/attaches exclusive, which feels unnecessary to me, although it would make the code here slightly cleaner..

I'd pay the cost on the attach side to have a single lock on the fault side..

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

This already holds the streams_mutex across all of this, do you think we should get rid of the vmaster_rwsem and hold the streams_mutex on write instead?

Quoted 6 lines

Quoted 3 lines

They are per master v.s. per smmu. The latter one would make master commits/attaches exclusive, which feels unnecessary to me, although it would make the code here slightly cleaner..

I'd pay the cost on the attach side to have a single lock on the fault side..

OK. Maybe a small patch to turn the streams_mutex to streams_rwsem?

Quoted 8 lines · Nicolin Chen

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

This already holds the streams_mutex across all of this, do you think we should get rid of the vmaster_rwsem and hold the streams_mutex on write instead?

They are per master v.s. per smmu. The latter one would make master commits/attaches exclusive, which feels unnecessary to me, although it would make the code here slightly cleaner..

Quoted 4 lines

Quoted 2 lines

I'd pay the cost on the attach side to have a single lock on the fault side..

OK. Maybe a small patch to turn the streams_mutex to streams_rwsem?

I don't think the interrupt path is multithreaded, is it? So only 1 reader anyhow?

Quoted 19 lines · Nicolin Chen

Quoted 18 lines · Jason Gunthorpe

Review static int arm_smmu_handle_event(struct arm_smmu_device *smmu, · line 1832

Quoted 3 lines · Nicolin Chen

(quoted diff) +0 -0

@@ -1831,31 +1831,30 @@ static int arm_smmu_handle_event(struct arm_smmu_device *smmu,
 		return -EOPNOTSUPP;
 	}

There is still the filter at the top:

	switch (event->id) {
	case EVT_ID_TRANSLATION_FAULT:
	case EVT_ID_ADDR_SIZE_FAULT:
	case EVT_ID_ACCESS_FAULT:
	case EVT_ID_PERMISSION_FAULT:
		break;
	default:
		return -EOPNOTSUPP;
	}

Quoted 5 lines

Quoted 2 lines

Is that right here or should more event types be forwarded to the guest?

That doesn't seem to be right. Something like EVT_ID_BAD_CD_CONFIG should be forwarded too. I will go through the list.

I think the above should decode into a 'faultable' path because they all decode to something with an IOVA

The rest should decode to things that include a SID and the SID decode should always be forwarded to the VM. Maybe there are small exclusions, but generally that is how I would see it..

But F_CD_FETCH and F_STE_FETCH seem to be complicated here, as both

report PA in their FetchAddr fields, although the spec does mention both might be injected to a guest VM:
- "Note: This event might be injected into a guest VM, as though from a virtual SMMU, when a hypervisor receives a stage 2 Translation-related fault indicating CD fetch as a cause (with CLASS == CD)."

For F_CD_FETCH, at least the CD table pointer in the nested STE is an IPA, and all the entries in the CD table that can be 2-level are IPAs as well. So, we need some kinda reverse translation from a PA to IPA using its stage-2 mapping. I am not sure what's the best way to do that...

Otherwise, perhaps not-supporting them in this series might be a safer bet?

include/uapi/linux/iommufd.h enum iommu_veventq_type { · line 1071

Quoted 12 lines · Nicolin Chen

------------------------------------------------------------

include/uapi/linux/iommufd.h +2 -1

index fd2f13a63f27..be9746ecdc65 100644
@@ -1067,7 +1067,16 @@ enum iommu_veventq_type {
  * struct iommu_vevent_arm_smmuv3 - ARM SMMUv3 Virtual Event
  *                                  (IOMMU_VEVENTQ_TYPE_ARM_SMMUV3)
  * @evt: 256-bit ARM SMMUv3 Event record, little-endian.
- *       (Refer to "7.3 Event records" in SMMUv3 HW Spec)
+ *       Reported event records: (Refer to "7.3 Event records" in SMMUv3 HW Spec)
+ *       - 0x02 C_BAD_STREAMID

This is documented as 'Transaction StreamID out of range.' so it would by a hypervisor kernel bug to hit it

include/uapi/linux/iommufd.h enum iommu_veventq_type { · line 1072

Quoted 1 line

include/uapi/linux/iommufd.h +1 -0

+ *       - 0x04 C_BAD_STE

I'm not sure we do enough validation to reject all bad STE fragments so it makes sense this could happen.

include/uapi/linux/iommufd.h enum iommu_veventq_type { · line 1073

Quoted 1 line

include/uapi/linux/iommufd.h +1 -0

+ *       - 0x06 F_STREAM_DISABLED

This looked guest triggerable to me.. so it makes sense

include/uapi/linux/iommufd.h enum iommu_veventq_type { · line 1075

Quoted 2 lines

include/uapi/linux/iommufd.h +2 -0

+ *       - 0x08 C_BAD_SUBSTREAMID
+ *       - 0x0a C_BAD_STE

Typo, this is C_BAD_CD

Quoted 1 line

But F_CD_FETCH and F_STE_FETCH seem to be complicated here, as both

F_STE_FETCH would indicate a hypervisor failure managing the stream table so no need to forward it.

Quoted 6 lines

report PA in their FetchAddr fields, although the spec does mention both might be injected to a guest VM:
- "Note: This event might be injected into a guest VM, as though from a virtual SMMU, when a hypervisor receives a stage 2 Translation-related fault indicating CD fetch as a cause (with CLASS == CD)."

That sounds like the VMM should be catching the
F_TRANSLATION and convert it for the CLASS=CD

Quoted 5 lines

For F_CD_FETCH, at least the CD table pointer in the nested STE is an IPA, and all the entries in the CD table that can be 2-level are IPAs as well. So, we need some kinda reverse translation from a PA to IPA using its stage-2 mapping. I am not sure what's the best way to do that...

And if the F_TRANSLATION covers the case then maybe this just stays in the hypervisor?

Quoted 2 lines

Otherwise, perhaps not-supporting them in this series might be a safer bet?

Yeah, I would consider skipping F_CD_FETCH. May also just try it out and see what events come out on a CD fetch failure..

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

There is a DoS concern on the shared hardware event queue among devices passed through to VMs, that too many translation failures that belong to VMs could overflow the shared hardware event queue if those VMs or their VMMs don't handle/recover the devices properly.

The MEV bit in the STE allows to configure the SMMU HW to merge similar event records, though there is no guarantee. Set it in a nested STE for DoS mitigations.

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

There is a DoS concern on the shared hardware event queue among devices passed through to VMs, that too many translation failures that belong to VMs could overflow the shared hardware event queue if those VMs or their VMMs don't handle/recover the devices properly.

This statement is not specific to the nested configuration.

Quoted 1 line

Quoted 3 lines

The MEV bit in the STE allows to configure the SMMU HW to merge similar event records, though there is no guarantee. Set it in a nested STE for DoS mitigations.

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

Quoted 2 lines

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

I think the arm-smmu-v3's iommufd implementation only supports nested which could be the reason.

Quoted 3 lines · Pranjal Shrivastava

Quoted 2 lines

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

Quoted 2 lines

I think the arm-smmu-v3's iommufd implementation only supports nested which could be the reason.

I think starting with MEV in this limited case is reasonable.

I agree it makes sense to always turn it on from a production perspective..

Quoted 11 lines · Kevin

Quoted 7 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 25, 2025 8:31 AM

There is a DoS concern on the shared hardware event queue among devices passed through to VMs, that too many translation failures that belong to VMs could overflow the shared hardware event queue if those VMs or their VMMs don't handle/recover the devices properly.

This statement is not specific to the nested configuration.

Quoted 1 line

Quoted 6 lines

Quoted 3 lines

The MEV bit in the STE allows to configure the SMMU HW to merge similar event records, though there is no guarantee. Set it in a nested STE for DoS mitigations.

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

MEV is available at all times (if an implemented by the HW) and doesn't depend on the nested mode. As per the Arm SMMUv3 spec (section 3.5.5):

Events can be merged where all of the following conditions are upheld:
- The event types and all fields are identical, except fields explicitly
indicated in section 7.3 Event records.

- If present, the Stall field is 0. Stall fault records are not merged.

I'm not sure to what extent, but I think *trying* to merge similar event should reduce some chances of overflowing the hw eventq.

Quoted 2 lines

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

I think the arm-smmu-v3's iommufd implementation only supports nested which could be the reason.

Quoted 5 lines

Quoted 2 lines

Is MEV available only in nested mode? Otherwise it perhaps makes sense to turn it on in all configurations in IOMMUFD paths...

I think the arm-smmu-v3's iommufd implementation only supports nested which could be the reason.

I guess what Kevin says is that non-nested STE should set the MEV as well, e.g. BYPASS and ABORT, and perhaps stage-1-only case too where the attaching domain = UNMANAGED.

There is a DoS concern on the shared hardware event queue among devices passed through to VMs, that too many translation failures that belong to VMs could overflow the shared hardware event queue if those VMs or their VMMs don't handle/recover the devices properly.

The MEV bit in the STE allows to configure the SMMU HW to merge similar event records, though there is no guarantee. Set it in a nested STE for DoS mitigations.

Signed-off-by Nicolin Chen <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h         | 1 +
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 2 ++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c         | 4 ++--
 3 files changed, 5 insertions(+), 2 deletions(-)

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits) · line 1055

Quoted 9 lines · Nicolin Chen

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1 -1

@@ -1051,7 +1051,7 @@ void arm_smmu_get_ste_used(const __le64 *ent, __le64 *used_bits)
 			cpu_to_le64(STRTAB_STE_1_S1DSS | STRTAB_STE_1_S1CIR |
 				    STRTAB_STE_1_S1COR | STRTAB_STE_1_S1CSH |
 				    STRTAB_STE_1_S1STALLD | STRTAB_STE_1_STRW |
-				    STRTAB_STE_1_EATS);
+				    STRTAB_STE_1_EATS | STRTAB_STE_1_MEV);
 		used_bits[2] |= cpu_to_le64(STRTAB_STE_2_S2VMID);

You also ran the test suite?

This is on Github:
https://github.com/nicolinc/iommufd/commits/iommufd_veventq-v6

Testing with RMR patches for MSI:
https://github.com/nicolinc/iommufd/commits/iommufd_veventq-v6-with-rmr
Paring QEMU branch for testing:
https://github.com/nicolinc/qemu/commits/wip/for_iommufd_veventq-v6

Changelog
v6
* Drop supports_veventq viommu op
* Split bug/cosmetics fixes out of the series
* Drop the blocking mutex around copy_to_user()
* Add veventq_depth in uAPI to limit vEVENTQ size
* Revise the documentation for a clear description
* Fix sparse warnings in arm_vmaster_report_event()
* Rework iommufd_viommu_get_vdev_id() to return -ENOENT v.s. 0
* Allow Abort/Bypass STEs to allocate vEVENTQ and set STE.MEV for DoS mitigations