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

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

Reorder the existing OBJ/IOCTL lists.

Also run clang-format for the same coding style at line wrappings.

No functional change.

Reviewed-by Lu Baolu <[email protected]>

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

Reorder the existing OBJ/IOCTL lists.

Also run clang-format for the same coding style at line wrappings.

No functional change.

Reviewed-by Lu Baolu <[email protected]>

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

---
 drivers/iommu/iommufd/main.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

Reorder the existing OBJ/IOCTL lists.

Also run clang-format for the same coding style at line wrappings.

No functional change.

Reviewed-by Lu Baolu <[email protected]>

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

---
 drivers/iommu/iommufd/main.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

The infrastructure of a fault object will be shared with a new vEVENTQ object in a following change. Add a helper for a vEVENTQ allocator to call it too.

Reorder the iommufd_ctx_get and refcount_inc, to keep them symmetrical with the iommufd_fault_fops_release().

Since the new vEVENTQ doesn't need "response", leave the xa_init_flags in its original location.

Reviewed-by Kevin Tian <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

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

---
 drivers/iommu/iommufd/fault.c | 48 ++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)

The new vEVENTQ object will need a similar function for drivers to report the vIOMMU related events. Split the common part out to a smaller helper, and place it in the header so that CONFIG_IOMMUFD_DRIVER_CORE can include that in the driver.c file for drivers to use.

Then keep iommufd_fault_iopf_handler() in the header too, since it's quite simple after all.

Reviewed-by Kevin Tian <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

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

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

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

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).

Also, add missing xa_destroy and mutex_destroy in iommufd_fault_destroy().

Reviewed-by Lu Baolu <[email protected]>

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

(quoted diff) +0 -2

@@ -433,32 +434,35 @@ void iopt_remove_access(struct io_pagetable *iopt,
 			u32 iopt_access_list_id);
 void iommufd_access_destroy_object(struct iommufd_object *obj);
 
-/*
- * An iommufd_fault object represents an interface to deliver I/O page faults

Rename the file, aligning with the new eventq object.

Reviewed-by Kevin Tian <[email protected]>

Reviewed-by Lu Baolu <[email protected]>

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

---
 drivers/iommu/iommufd/Makefile              | 2 +-
 drivers/iommu/iommufd/{fault.c => eventq.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/iommu/iommufd/{fault.c => eventq.c} (100%)

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

drivers/iommu/iommufd/eventq.c +10 -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)

Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

drivers/iommu/iommufd/eventq.c +10 -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)

drivers/iommu/iommufd/eventq.c int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) · line 562

Quoted 10 lines

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

+		return -EOPNOTSUPP;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->supports_veventq ||
+	    !viommu->ops->supports_veventq(cmd->type))
+		return -EOPNOTSUPP;
+

I'm not sure about the necessity of above check. The event queue is just a software struct with a user-specified format for the iommu driver to report viommu event. The struct itself is not constrained by the hardware capability, though I'm not sure a real usage in which a smmu driver wants to report a vtd event. But legitimately an user can create any type of event queues which might just be never used.

It sounds clearer to do the check when IOPF cap is actually enabled on a device contained in the viommu. At that point check whether a required type eventqueue has been created. If not then fail the iopf enabling.

Then it reveals probably another todo in this series. Seems you still let the smmu driver statically enable iopf when probing the device. Sounds like iommufd_viommu_alloc_hwpt_nested() may accept IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and later dynamically enable/disable iopf when attaching a device to the hwpt and check the event queue type there. Just like how the fault object is handled.

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 11, 2025 5:29 AM

Quoted 23 lines · Kevin

Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

drivers/iommu/iommufd/eventq.c +10 -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)

drivers/iommu/iommufd/eventq.c int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) · line 562

Quoted 10 lines

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

+		return -EOPNOTSUPP;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->supports_veventq ||
+	    !viommu->ops->supports_veventq(cmd->type))
+		return -EOPNOTSUPP;
+

Quoted 7 lines

I'm not sure about the necessity of above check. The event queue is just a software struct with a user-specified format for the iommu driver to report viommu event. The struct itself is not constrained by the hardware capability, though I'm not sure a real usage in which a smmu driver wants to report a vtd event. But legitimately an user can create any type of event queues which might just be never used.

Allowing a random type that a driver will never use for reporting doesn't sound to make a lot of sense to me...

That being said, yea..I guess we could drop the limit here, since it isn't going to break anything?

Quoted 4 lines

It sounds clearer to do the check when IOPF cap is actually enabled on a device contained in the viommu. At that point check whether a required type eventqueue has been created. If not then fail the iopf enabling.

Hmm, isn't IOPF a different channel?

And a vEVENTQ is per vIOMMU, not necessarily per vDEVICE/device..

Quoted 8 lines

Then it reveals probably another todo in this series. Seems you still let the smmu driver statically enable iopf when probing the device. Sounds like iommufd_viommu_alloc_hwpt_nested() may accept IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and later dynamically enable/disable iopf when attaching a device to the hwpt and check the event queue type there. Just like how the fault object is handled.

You've lost me here :-/

Quoted 41 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 11, 2025 5:29 AM

Quoted 23 lines · Kevin

Quoted 12 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

drivers/iommu/iommufd/eventq.c +10 -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)

drivers/iommu/iommufd/eventq.c int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) · line 562

Quoted 10 lines

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

+		return -EOPNOTSUPP;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->supports_veventq ||
+	    !viommu->ops->supports_veventq(cmd->type))
+		return -EOPNOTSUPP;
+

Quoted 7 lines

I'm not sure about the necessity of above check. The event queue is just a software struct with a user-specified format for the iommu driver to report viommu event. The struct itself is not constrained by the hardware capability, though I'm not sure a real usage in which a smmu driver wants to report a vtd event. But legitimately an user can create any type of event queues which might just be never used.

Allowing a random type that a driver will never use for reporting doesn't sound to make a lot of sense to me...

That being said, yea..I guess we could drop the limit here, since it isn't going to break anything?

Quoted 6 lines

Quoted 4 lines

It sounds clearer to do the check when IOPF cap is actually enabled on a device contained in the viommu. At that point check whether a required type eventqueue has been created. If not then fail the iopf enabling.

Hmm, isn't IOPF a different channel?

We have a fault queue for delivering IOPF on hwpt, when vIOMMU is not involved

Now with vIOMMU my understanding was that all events including IOPF are delivered via the event queue in the vIOMMU. Just echoed by the documentation patch:

+- IOMMUFD_OBJ_VEVENTQ, representing a software queue for a vIOMMU to report its

Quoted 2 lines

And a vEVENTQ is per vIOMMU, not necessarily per vDEVICE/device..

Yes. My point was to verify whether the vEVENTQ type is compatible when a nested faultable hwpt is created with vIOMMU as the parent. then when attaching a device to the nested hwpt we dynamically turn on PRI on the device just like how it's handled in the fault queue path.

Quoted 8 lines

Quoted 7 lines

Then it reveals probably another todo in this series. Seems you still let the smmu driver statically enable iopf when probing the device. Sounds like iommufd_viommu_alloc_hwpt_nested() may accept IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and later dynamically enable/disable iopf when attaching a device to the hwpt and check the event queue type there. Just like how the fault object is handled.

Quoted 2 lines

You've lost me here :-/

Hope above explanation makes my point clearer. Then for a nested hwpt created within a vIOMMU there is an open whether we want a per-hwpt option to mark whether it allows fault, or assume that every nested hwpt (and the devices attached to it) must be faultable once any vEVENTQ is created in the vIOMMU.

From: Nicolin Chen <[email protected]>
Sent: Monday, January 13, 2025 12:51 PM

Quoted 4 lines · Kevin

Quoted 4 lines

From: Nicolin Chen <[email protected]>
Sent: Saturday, January 11, 2025 5:29 AM

On Fri, Jan 10, 2025 at 07:06:49AM +0000, Tian, Kevin wrote:

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

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

+
+int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)

Quoted 8 lines

Quoted 8 lines

drivers/iommu/iommufd/eventq.c int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)

Quoted 8 lines

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

+{
+	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)

Quoted 32 lines

Quoted 25 lines

Quoted 11 lines

drivers/iommu/iommufd/eventq.c int iommufd_fault_alloc(struct iommufd_ucmd *ucmd) · line 562

Quoted 10 lines

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

+		return -EOPNOTSUPP;
+
+	viommu = iommufd_get_viommu(ucmd, cmd->viommu_id);
+	if (IS_ERR(viommu))
+		return PTR_ERR(viommu);
+
+	if (!viommu->ops || !viommu->ops->supports_veventq ||
+	    !viommu->ops->supports_veventq(cmd->type))
+		return -EOPNOTSUPP;
+

Quoted 7 lines

I'm not sure about the necessity of above check. The event queue is just a software struct with a user-specified format for the iommu driver to report viommu event. The struct itself is not constrained by the hardware capability, though I'm not sure a real usage in which a smmu driver wants to report a vtd event. But legitimately an user can create any type of event queues which might just be never used.

Allowing a random type that a driver will never use for reporting doesn't sound to make a lot of sense to me...

That being said, yea..I guess we could drop the limit here, since it isn't going to break anything?

Quoted 6 lines

Quoted 4 lines

It sounds clearer to do the check when IOPF cap is actually enabled on a device contained in the viommu. At that point check whether a required type eventqueue has been created. If not then fail the iopf enabling.

Hmm, isn't IOPF a different channel?

Quoted 8 lines

We have a fault queue for delivering IOPF on hwpt, when vIOMMU is not involved

Now with vIOMMU my understanding was that all events including IOPF are delivered via the event queue in the vIOMMU. Just echoed by the documentation patch:

+- IOMMUFD_OBJ_VEVENTQ, representing a software queue for a

vIOMMU to report its

Quoted 1 line

+ events such as translation faults occurred to a nested stage-1 and HW-

specific

Quoted 1 line

+ events.

Oh, looks like that line misguided you.. It should be non-PRI type of fault, e.g. a stage-1 DMA translation error should be forwarded to the guest. I can make it clearer.

Quoted 7 lines

Quoted 2 lines

And a vEVENTQ is per vIOMMU, not necessarily per vDEVICE/device..

Yes. My point was to verify whether the vEVENTQ type is compatible when a nested faultable hwpt is created with vIOMMU as the parent. then when attaching a device to the nested hwpt we dynamically turn on PRI on the device just like how it's handled in the fault queue path.

We will still have the fault queue:
if (error is handled by PRI)
report via fault queue; // need response
else (error is handled by vEVENTQ)
report via vEVENTQ; // no need of response
else
dump unhandled faults;

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Then it reveals probably another todo in this series. Seems you still let the smmu driver statically enable iopf when probing the device. Sounds like iommufd_viommu_alloc_hwpt_nested() may accept IOMMU_HWPT_FAULT_ID_VALID to refer to a event queue and later dynamically enable/disable iopf when attaching a device to the hwpt and check the event queue type there. Just like how the fault object is handled.

You've lost me here :-/

Quoted 5 lines

Hope above explanation makes my point clearer. Then for a nested hwpt created within a vIOMMU there is an open whether we want a per-hwpt option to mark whether it allows fault, or assume that every nested hwpt (and the devices attached to it) must be faultable once any vEVENTQ is created in the vIOMMU.

A vIOMMU-based nested HWPT should still enable IOPF via the flag IOMMU_HWPT_FAULT_ID_VALID.

We will still have the fault queue:
if (error is handled by PRI)
report via fault queue; // need response

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

+static ssize_t iommufd_veventq_fops_read(struct iommufd_eventq *eventq,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	size_t done = 0;
+	int rc = 0;
+
+	if (*ppos)
+		return -ESPIPE;
+
+	mutex_lock(&eventq->mutex);
+	while (!list_empty(&eventq->deliver) && count > done) {

Quoted 12 lines · Nicolin Chen

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

+static ssize_t iommufd_veventq_fops_read(struct iommufd_eventq *eventq,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	size_t done = 0;
+	int rc = 0;
+
+	if (*ppos)
+		return -ESPIPE;
+
+	mutex_lock(&eventq->mutex);
+	while (!list_empty(&eventq->deliver) && count > done) {

drivers/iommu/iommufd/eventq.c static const struct iommufd_eventq_ops iommufd_fault_ops = { · line 402

Quoted 10 lines

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

+		struct iommufd_vevent *cur = list_first_entry(
+			&eventq->deliver, struct iommufd_vevent, node);
+
+		if (cur->data_len > count - done)
+			break;
+
+		if (copy_to_user(buf + done, cur->event_data, cur->data_len)) {
+			rc = -EFAULT;
+			break;
+		}

Now that I look at this more closely, the fault path this is copied from is not great.

This copy_to_user() can block while waiting on a page fault, possibily for a long time. While blocked the mutex is held and we can't add more entries to the list.

That will cause the shared IRQ handler in the iommu driver to back up, which would cause a global DOS.

This probably wants to be organized to look more like

while (itm = eventq_get_next_item(eventq)) {
   if (..) {
       eventq_restore_failed_item(eventq);
       return -1;
   }
}

Where the next_item would just be a simple spinlock across the linked list manipulation.

Quoted 44 lines · Jason Gunthorpe

Quoted 12 lines · Nicolin Chen

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

+static ssize_t iommufd_veventq_fops_read(struct iommufd_eventq *eventq,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	size_t done = 0;
+	int rc = 0;
+
+	if (*ppos)
+		return -ESPIPE;
+
+	mutex_lock(&eventq->mutex);
+	while (!list_empty(&eventq->deliver) && count > done) {

drivers/iommu/iommufd/eventq.c static const struct iommufd_eventq_ops iommufd_fault_ops = { · line 402

Quoted 10 lines

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

+		struct iommufd_vevent *cur = list_first_entry(
+			&eventq->deliver, struct iommufd_vevent, node);
+
+		if (cur->data_len > count - done)
+			break;
+
+		if (copy_to_user(buf + done, cur->event_data, cur->data_len)) {
+			rc = -EFAULT;
+			break;
+		}

Now that I look at this more closely, the fault path this is copied from is not great.

This copy_to_user() can block while waiting on a page fault, possibily for a long time. While blocked the mutex is held and we can't add more entries to the list.

That will cause the shared IRQ handler in the iommu driver to back up, which would cause a global DOS.

This probably wants to be organized to look more like

while (itm = eventq_get_next_item(eventq)) {
   if (..) {
       eventq_restore_failed_item(eventq);
       return -1;
   }
}

Quoted 2 lines

Where the next_item would just be a simple spinlock across the linked list manipulation.

Would it be simpler by just limiting one node per read(), i.e. no "while (!list_empty)" and no block?

The report() adds one node at a time, and wakes up the poll() each time of adding a node. And user space could read one event at a time too?

Quoted 51 lines · Nicolin Chen

Quoted 44 lines · Jason Gunthorpe

Quoted 12 lines · Nicolin Chen

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

+static ssize_t iommufd_veventq_fops_read(struct iommufd_eventq *eventq,
+					 char __user *buf, size_t count,
+					 loff_t *ppos)
+{
+	size_t done = 0;
+	int rc = 0;
+
+	if (*ppos)
+		return -ESPIPE;
+
+	mutex_lock(&eventq->mutex);
+	while (!list_empty(&eventq->deliver) && count > done) {

drivers/iommu/iommufd/eventq.c static const struct iommufd_eventq_ops iommufd_fault_ops = { · line 402

Quoted 10 lines

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

+		struct iommufd_vevent *cur = list_first_entry(
+			&eventq->deliver, struct iommufd_vevent, node);
+
+		if (cur->data_len > count - done)
+			break;
+
+		if (copy_to_user(buf + done, cur->event_data, cur->data_len)) {
+			rc = -EFAULT;
+			break;
+		}

Now that I look at this more closely, the fault path this is copied from is not great.

This copy_to_user() can block while waiting on a page fault, possibily for a long time. While blocked the mutex is held and we can't add more entries to the list.

That will cause the shared IRQ handler in the iommu driver to back up, which would cause a global DOS.

This probably wants to be organized to look more like

while (itm = eventq_get_next_item(eventq)) {
   if (..) {
       eventq_restore_failed_item(eventq);
       return -1;
   }
}

Quoted 2 lines

Where the next_item would just be a simple spinlock across the linked list manipulation.

Would it be simpler by just limiting one node per read(), i.e. no "while (!list_empty)" and no block?

Quoted 3 lines

The report() adds one node at a time, and wakes up the poll() each time of adding a node. And user space could read one event at a time too?

That doesn't really help, the issue is it holds the lock over the copy_to_user() which it is doing because it doesn't want pull the item off the list and then try to handle the failure and put it back.

Hmm, it seems that I haven't got your first narrative straight..

Would you mind elaborate "copy_to_user() can block while waiting on a page fault"? When would this happen?

Quoted 4 lines · Nicolin Chen

Hmm, it seems that I haven't got your first narrative straight..

Would you mind elaborate "copy_to_user() can block while waiting on a page fault"? When would this happen?

copy_to_user() is a sleeping function that sleeps if the user memory is non-present. So userspace can cause copy_to_user to copy to anything, including memory that is non-present and will take along time to page fault in. Eg perhaps by abusing userfaultfd.

We should not allow userspace to globally DOS the iommu driver this way.

So do not hold locks that are also held by the HW event path across copy_to_user().

Quoted 4 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

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

+
+	xa_lock(&viommu->vdevs);

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

Quoted 6 lines

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

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

Nit. No need to check vdev pointer as commented in last review.

Reviewed-by Kevin Tian <[email protected]>

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

+/*
+ * Typically called in driver's threaded IRQ handler.
+ * The @type and @event_data must be defined in include/uapi/linux/iommufd.h
+ */

(quoted diff) +2 -0

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

*event_data,

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

+				size_t data_len)
+{
+	struct iommufd_veventq *veventq;
+	struct iommufd_vevent *vevent;
+	int rc = 0;
+
+	if (!viommu)
+		return -ENODEV;
+	if (WARN_ON_ONCE(!viommu->ops || !viommu->ops-

Quoted 1 line

supports_veventq ||

drivers/iommu/iommufd/driver.c unsigned long iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 88

Quoted 6 lines

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

+	if (!viommu)
+		return -ENODEV;
+	if (WARN_ON_ONCE(!viommu->ops || !viommu->ops-

Quoted 1 line

supports_veventq ||

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

+			 !viommu->ops->supports_veventq(type)))
+		return -EOPNOTSUPP;

Hmm the driver knows which type is supported by itself before calling this helper. Why bother having the helper calling into the driver again to verify?

Quoted 10 lines · Kevin

drivers/iommu/iommufd/driver.c unsigned long iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 88

Quoted 6 lines

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

+	if (!viommu)
+		return -ENODEV;
+	if (WARN_ON_ONCE(!viommu->ops || !viommu->ops-

Quoted 1 line

supports_veventq ||

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

+			 !viommu->ops->supports_veventq(type)))
+		return -EOPNOTSUPP;

Hmm the driver knows which type is supported by itself before calling this helper. Why bother having the helper calling into the driver again to verify?

Indeed, it might make sense to protect this with

if (IS_ENABLED(CONFIG_IOMMUFD_TEST))

As a compiled out assertion

Or drop it

We shouldn't have unnecessary argument validation on fast paths, !viommu should go too.

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

+/*
+ * Typically called in driver's threaded IRQ handler.
+ * The @type and @event_data must be defined in include/uapi/linux/iommufd.h
+ */
+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 (!viommu)
+		return -ENODEV;
+	if (WARN_ON_ONCE(!viommu->ops || !viommu->ops->supports_veventq ||
+			 !viommu->ops->supports_veventq(type)))
+		return -EOPNOTSUPP;
+	if (WARN_ON_ONCE(!data_len || !event_data))
+		return -EINVAL;
+
+	down_read(&viommu->veventqs_rwsem);
+
+	veventq = iommufd_viommu_find_veventq(viommu, type);
+	if (!veventq) {

Quoted 24 lines · Nicolin Chen

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

+/*
+ * Typically called in driver's threaded IRQ handler.
+ * The @type and @event_data must be defined in include/uapi/linux/iommufd.h
+ */
+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 (!viommu)
+		return -ENODEV;
+	if (WARN_ON_ONCE(!viommu->ops || !viommu->ops->supports_veventq ||
+			 !viommu->ops->supports_veventq(type)))
+		return -EOPNOTSUPP;
+	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 unsigned long iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu, · line 105

Quoted 10 lines

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

+		rc = -EOPNOTSUPP;
+		goto out_unlock_veventqs;
+	}
+
+	vevent = kmalloc(struct_size(vevent, event_data, data_len), GFP_KERNEL);
+	if (!vevent) {
+		rc = -ENOMEM;
+		goto out_unlock_veventqs;
+	}
+	memcpy(vevent->event_data, event_data, data_len);

The page fault path is self limited because end point devices are only able to issue a certain number of PRI's before they have to stop.

But the async events generated by something like the SMMU are not self limiting and we can have a huge barrage of them. I think you need to add some kind of limiting here otherwise we will OOM the kernel and crash, eg if the VM spams protection errors.

The virtual event queue should behave the same as if the physical event queue overflows, and that logic should be in the smmu driver - this should return some Exxx to indicate the queue is filled.

I supposed we will need a way to indicate lost events to userspace on top of this?

Presumably userspace should specify the max queue size.

Quoted 3 lines

The virtual event queue should behave the same as if the physical event queue overflows, and that logic should be in the smmu driver - this should return some Exxx to indicate the queue is filled.

Hmm, the driver only screams...

static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) {
[...]

		/*
		 * Not much we can do on overflow, so scream and pretend we're
		 * trying harder.
		 */
		if (queue_sync_prod_in(q) == -EOVERFLOW)
			dev_err(smmu->dev, "EVTQ overflow detected -- events lost\n");

Quoted 2 lines

I supposed we will need a way to indicate lost events to userspace on top of this?

Perhaps another u32 flag in the arm_smmuv3_vevent struct to report an overflow. That said, what userspace/VMM will need to do with it?

Quoted 9 lines · Nicolin Chen

Quoted 3 lines

The virtual event queue should behave the same as if the physical event queue overflows, and that logic should be in the smmu driver - this should return some Exxx to indicate the queue is filled.

Hmm, the driver only screams...

static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) {
[...]

Quoted 6 lines

		/*
		 * Not much we can do on overflow, so scream and pretend we're
		 * trying harder.
		 */
		if (queue_sync_prod_in(q) == -EOVERFLOW)
			dev_err(smmu->dev, "EVTQ overflow detected -- events lost\n");

Well it must know from the HW somehow that the overflow has happened??

Quoted 5 lines

Quoted 2 lines

I supposed we will need a way to indicate lost events to userspace on top of this?

Perhaps another u32 flag in the arm_smmuv3_vevent struct to report an overflow. That said, what userspace/VMM will need to do with it?

Trigger the above code in the VM somehow?

Quoted 9 lines · Nicolin Chen

Quoted 3 lines

The virtual event queue should behave the same as if the physical event queue overflows, and that logic should be in the smmu driver - this should return some Exxx to indicate the queue is filled.

Hmm, the driver only screams...

static irqreturn_t arm_smmu_evtq_thread(int irq, void *dev) {
[...]

Quoted 6 lines

		/*
		 * Not much we can do on overflow, so scream and pretend we're
		 * trying harder.
		 */
		if (queue_sync_prod_in(q) == -EOVERFLOW)
			dev_err(smmu->dev, "EVTQ overflow detected -- events lost\n");

Well it must know from the HW somehow that the overflow has happened??

Quoted 5 lines

Quoted 2 lines

I supposed we will need a way to indicate lost events to userspace on top of this?

Perhaps another u32 flag in the arm_smmuv3_vevent struct to report an overflow. That said, what userspace/VMM will need to do with it?

Trigger the above code in the VM somehow?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I supposed we will need a way to indicate lost events to userspace on top of this?

Perhaps another u32 flag in the arm_smmuv3_vevent struct to report an overflow. That said, what userspace/VMM will need to do with it?

Quoted 1 line

Trigger the above code in the VM somehow?

I found two ways of forwarding an overflow flag:

1. Return -EOVERFLOW to read(). But it cannot return the read bytes any more:

Quoted 10 lines · Nicolin Chen

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I supposed we will need a way to indicate lost events to userspace on top of this?

Perhaps another u32 flag in the arm_smmuv3_vevent struct to report an overflow. That said, what userspace/VMM will need to do with it?

Trigger the above code in the VM somehow?

I found two ways of forwarding an overflow flag:

Quoted 2 lines

1. Return -EOVERFLOW to read(). But it cannot return the read bytes any more:

You could not return any bytes, it would have to be 0 bytes read, ie immediately return EOVERFLOW and do nothing else.

Returning EOVERFLOW from read would have to also clear the overflow indicator.

The other approach would be to add a sequence number to each event and let userspace detect the non-montonicity. It would require adding a header to the native ARM evt.

Review static __poll_t iommufd_eventq_fops_poll(struct file *filep, · line 424

Quoted 8 lines

2. Return EPOLLERR via pollfd.revents. But it cannot use POLLERR for other errors any more:
--------------------------------------------------

(quoted diff) +2 -0

@@ -420,2 +421,4 @@ static __poll_t iommufd_eventq_fops_poll(struct file *filep,
        poll_wait(filep, &eventq->wait_queue, wait);
+       if (test_bit(IOMMUFD_VEVENTQ_ERROR_OVERFLOW, veventq->errors))
+               return EPOLLERR;
        mutex_lock(&eventq->mutex);

But then how do you clear the error? I've only seen POLLERR used for fatal conditions so there is no recovery, it is permanent.

Quoted 5 lines

You could not return any bytes, it would have to be 0 bytes read, ie immediately return EOVERFLOW and do nothing else.

Returning EOVERFLOW from read would have to also clear the overflow indicator.

OK. That means user space should read again for actual events in the queue, after getting the first EOVERFLOW.

One concern is, if the report() keeps producing events to the queue, it will always set the EOVERFLOW flag, then user space won't have a chance to read the events out until the last report(). Wondering if this would make sense, as I see SMMU driver's arm_smmu_evtq_thread() reporting an OVERFLOW while allowing SW to continue reading the evtq.

Quoted 3 lines

The other approach would be to add a sequence number to each event and let userspace detect the non-montonicity. It would require adding a header to the native ARM evt.

Yea, I thought about that. The tricky thing is that the header will be a core-level header pairing with a driver-level vEVENTQ type and can never change its length, though we can define a 64-bit flag that can reserve the other 63 bits for future use?

Quoted 4 lines · Nicolin Chen

Quoted 3 lines

The other approach would be to add a sequence number to each event and let userspace detect the non-montonicity. It would require adding a header to the native ARM evt.

Quoted 4 lines

Yea, I thought about that. The tricky thing is that the header will be a core-level header pairing with a driver-level vEVENTQ type and can never change its length, though we can define a 64-bit flag that can reserve the other 63 bits for future use?

The header format could be revised by changing the driver specific format tag.

You'd want to push a special event when the first overflow happens and probably also report a counter so userspace can know how many events got lost.

This seems most robust and simplest to implement..

I think I'd implement it by having a static overflow list entry so no memory allocation is needed and just keep moving that entry to the back of the list every time an event is lost. This way it will cover lost events due to memory outages too

For old formats like the fault queue you could return EOVERFLOW whenever the sequence number becomes discontiguous or it sees the overflow event..

Quoted 3 lines

You'd want to push a special event when the first overflow happens and probably also report a counter so userspace can know how many events got lost.

How about this:

enum iommufd_veventq_header_type {
IOMMU_VEVENTQ_HEADER_TYPE_V1,
};

enum iommu_hwpt_pgfault_flags {
	IOMMU_VEVENT_HEADER_FLAGS_OVERFLOW = (1 << 0),
};

struct iommufd_vevent_header_v1 {
	__u64 flags;
	__u32 num_events;
	__u32 num_overflows; // valid if flag_overflow is set
};

Quoted 6 lines

This seems most robust and simplest to implement..

I think I'd implement it by having a static overflow list entry so no memory allocation is needed and just keep moving that entry to the back of the list every time an event is lost. This way it will cover lost events due to memory outages too

Could double-adding the same static node to the list happen and corrupt the list?

Quoted 3 lines

For old formats like the fault queue you could return EOVERFLOW whenever the sequence number becomes discontiguous or it sees the overflow event..

So, IOMMU_OBJ_FAULT is safe to return EOVERFLOW via read(), as you mentioned that it is self-limited right?

Quoted 8 lines · Nicolin Chen

	IOMMU_VEVENT_HEADER_FLAGS_OVERFLOW = (1 << 0),
};

struct iommufd_vevent_header_v1 {
	__u64 flags;
	__u32 num_events;
	__u32 num_overflows; // valid if flag_overflow is set
};

num_overflows is hard, I'd just keep a counter.

+/**
+ * struct iommufd_vevent_header - Virtual Event Header for a vEVENTQ Status
+ * @state: One of enum iommu_veventq_state

+ * @counter: A counter reflecting the state of the vEVENTQ

+ * ----------------------------------------------------------------------------
+ * | @state | @counter |
+ * ----------------------------------------------------------------------------

+ * | IOMMU_VEVENTQ_STATE_OK       | number of readable vEVENTs in the vEVENTQ |
+ * | IOMMU_VEVENTQ_STATE_OVERFLOW | number of missed vEVENTs since overflow   |

+ * ----------------------------------------------------------------------------

Quoted 3 lines · Nicolin Chen

+/**
+ * struct iommufd_vevent_header - Virtual Event Header for a vEVENTQ Status
+ * @state: One of enum iommu_veventq_state

I'd probably just make this a flag with overflow as the only current flag?

Quoted 8 lines

+ * @counter: A counter reflecting the state of the vEVENTQ

+ * ----------------------------------------------------------------------------
+ * | @state | @counter |
+ * ----------------------------------------------------------------------------

+ * | IOMMU_VEVENTQ_STATE_OK       | number of readable vEVENTs in the vEVENTQ |
+ * | IOMMU_VEVENTQ_STATE_OVERFLOW | number of missed vEVENTs since overflow   |

+ * ----------------------------------------------------------------------------

When I said counter I literally ment a counter of the number of events that were sent into the queue. So if events are dropped there is a trivial gap in the count. Very easy to implement

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

If events are lost in the middle of the queue then flags will remain 0 but counter will become non-montonic. A counter delta > 1 indicates that many events have been lost.

The counter of the number of events in the vEVENTQ could decrease when userspace reads the queue. But you were saying "the number of events that were sent into the queue", which is like a PROD index that would keep growing but reset to 0 after UINT_MAX?

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Quoted 3 lines

If events are lost in the middle of the queue then flags will remain 0 but counter will become non-montonic. A counter delta > 1 indicates that many events have been lost.

I don't quite get the "no subsequent events" v.s. "in the middle of the queue"..

The producer is the driver calling iommufd_viommu_report_event that only produces a single vEVENT at a time. When the number of vEVENTs in the vEVENTQ hits the @veventq_depth, it won't insert new vEVENTs but add an overflow (or exception) node to the head of deliver list and increase the producer index so the next vEVENT that can find an empty space in the queue will have an index with a gap (delta >= 1)?

Quoted 4 lines · Nicolin Chen

The counter of the number of events in the vEVENTQ could decrease when userspace reads the queue. But you were saying "the number of events that were sent into the queue", which is like a PROD index that would keep growing but reset to 0 after UINT_MAX?

yes

Quoted 5 lines

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

Quoted 3 lines

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Yes, but there won't be a next header. overflow would always be the last thing in a read() response. If there is another event then overflow is indicated by non-monotonic count.

Quoted 6 lines

Quoted 3 lines

If events are lost in the middle of the queue then flags will remain 0 but counter will become non-montonic. A counter delta > 1 indicates that many events have been lost.

I don't quite get the "no subsequent events" v.s. "in the middle of the queue"..

I mean to supress specific overflow events to userspace if the counter already fully indicates overflow.

The purpose of the overflow event is specifically, and only, to indicate immediately that an overflow occured at the end of the queue, and no additional events have been pushed since the overflow.

Without this we could loose an event and userspace may not realize it for a long time.

Quoted 6 lines

The producer is the driver calling iommufd_viommu_report_event that only produces a single vEVENT at a time. When the number of vEVENTs in the vEVENTQ hits the @veventq_depth, it won't insert new vEVENTs but add an overflow (or exception) node to the head of deliver list and increase the producer index so the next vEVENT that can find an empty space in the queue will have an index with a gap (delta >= 1)?

Yes, but each new overflow should move the single preallocated overflow node back to the end of the list, and the read side should skip the overflow node if it is not the last entry in the list

Ack. Then I think we should name it "index", beside a "counter" indicating the number of events in the queue. Or perhaps a pair of consumer and producer indexes that wrap at end of a limit.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Quoted 3 lines

Yes, but there won't be a next header. overflow would always be the last thing in a read() response. If there is another event then overflow is indicated by non-monotonic count.

I am not 100% sure why "overflow would always be the last thing in a read() response". I thought that kernel should immediately report an overflow to user space when the vEVENTQ is overflowed.

Yet, thinking about this once again: user space actually has its own queue. There's probably no point in letting it know about an overflow immediately when the kernel vEVENTQ overflows until its own user queue overflows after it reads the entire vEVENTQ so it can trigger a vHW event/irq to the VM?

Quoted 2 lines

Without this we could loose an event and userspace may not realize it for a long time.

I see. Because there is no further new event, there would be no new index to indicate a gap. Thus, we need an overflow node.

If the number of events in the queue is below @veventq_depth as userspace consumed the events from the queue, I think a new iommufd_viommu_report_event call should delete the overflow node from the end of the list, right?

Quoted 9 lines · Nicolin Chen

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Quoted 7 lines

Quoted 3 lines

Yes, but there won't be a next header. overflow would always be the last thing in a read() response. If there is another event then overflow is indicated by non-monotonic count.

I am not 100% sure why "overflow would always be the last thing in a read() response". I thought that kernel should immediately report an overflow to user space when the vEVENTQ is overflowed.

As below, if you observe overflow then it was at the end of the kernel queue and there is no further events after it. So it should always end up last.

Perhaps we could enforce this directly in the kernel's read by making it the only, first and last, response to read.

Quoted 5 lines

Yet, thinking about this once again: user space actually has its own queue. There's probably no point in letting it know about an overflow immediately when the kernel vEVENTQ overflows until its own user queue overflows after it reads the entire vEVENTQ so it can trigger a vHW event/irq to the VM?

The kernel has no idea what userspace is doing, the kernel's job should be to ensure timely delivery of all events, if an event is lost it should ensure timely delivery of the lost event notification. There is little else it can do.

Quoted 22 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Quoted 7 lines

Quoted 3 lines

Yes, but there won't be a next header. overflow would always be the last thing in a read() response. If there is another event then overflow is indicated by non-monotonic count.

I am not 100% sure why "overflow would always be the last thing in a read() response". I thought that kernel should immediately report an overflow to user space when the vEVENTQ is overflowed.

As below, if you observe overflow then it was at the end of the kernel queue and there is no further events after it. So it should always end up last.

Quoted 2 lines

Perhaps we could enforce this directly in the kernel's read by making it the only, first and last, response to read.

Hmm, since the overflow node is the last node in the list, isn't it already an enforcement it's the only/first/last node to read? (Assuming we choose to delete the overflow node from the list if
new event can be inserted.)

Quoted 6 lines

Quoted 5 lines

Yet, thinking about this once again: user space actually has its own queue. There's probably no point in letting it know about an overflow immediately when the kernel vEVENTQ overflows until its own user queue overflows after it reads the entire vEVENTQ so it can trigger a vHW event/irq to the VM?

Quoted 4 lines

The kernel has no idea what userspace is doing, the kernel's job should be to ensure timely delivery of all events, if an event is lost it should ensure timely delivery of the lost event notification. There is little else it can do.

Yet, "timely" means still having an entire-queue-size-long delay since the overflow node is at the end of the queue, right?

Quoted 26 lines · Nicolin Chen

Quoted 24 lines · Jason Gunthorpe

Quoted 9 lines · Nicolin Chen

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

IOMMU_VEVENTQ_STATE_OVERFLOW with a 0 length event is seen if events have been lost and no subsequent events are present. It exists to ensure timely delivery of the overflow event to userspace. counter will be the sequence number of the next successful event.

So userspace should first read the header to decide whether or not to read a vEVENT. If header is overflows, it should skip the vEVENT struct and read the next header?

Quoted 7 lines

Quoted 3 lines

Yes, but there won't be a next header. overflow would always be the last thing in a read() response. If there is another event then overflow is indicated by non-monotonic count.

I am not 100% sure why "overflow would always be the last thing in a read() response". I thought that kernel should immediately report an overflow to user space when the vEVENTQ is overflowed.

As below, if you observe overflow then it was at the end of the kernel queue and there is no further events after it. So it should always end up last.

Perhaps we could enforce this directly in the kernel's read by making it the only, first and last, response to read.

Quoted 4 lines

Hmm, since the overflow node is the last node in the list, isn't it already an enforcement it's the only/first/last node to read? (Assuming we choose to delete the overflow node from the list if
new event can be inserted.)

Since we don't hold the spinlock the whole time there is a race where we could pull the overflow off and then another entry could be pushed while we do the copy_to_user.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Yet, thinking about this once again: user space actually has its own queue. There's probably no point in letting it know about an overflow immediately when the kernel vEVENTQ overflows until its own user queue overflows after it reads the entire vEVENTQ so it can trigger a vHW event/irq to the VM?

Quoted 7 lines

Quoted 4 lines

The kernel has no idea what userspace is doing, the kernel's job should be to ensure timely delivery of all events, if an event is lost it should ensure timely delivery of the lost event notification. There is little else it can do.

Yet, "timely" means still having an entire-queue-size-long delay since the overflow node is at the end of the queue, right?

Yes, but also in this case the vIOMMU isn't experiancing an overflow so it doesn't need to know about it.

The main point here is if we somehow loose an event the vIOMMU driver may need to do something to recover from the lost event. It doesn't become relavent until the lost event is present in the virtual HW queue.

There is also the minor detail of what happens if the hypervisor HW queue overflows - I don't know the answer here. It is security concerning since the VM can spam DMA errors at high rate. :|

Quoted 3 lines

There is also the minor detail of what happens if the hypervisor HW queue overflows - I don't know the answer here. It is security concerning since the VM can spam DMA errors at high rate. :|

In my view, the hypervisor queue is the vHW queue for the VM, so it should act like a HW, which means it's up to the guest kernel driver that handles the high rate DMA errors..

Quoted 7 lines · Nicolin Chen

Quoted 3 lines

There is also the minor detail of what happens if the hypervisor HW queue overflows - I don't know the answer here. It is security concerning since the VM can spam DMA errors at high rate. :|

In my view, the hypervisor queue is the vHW queue for the VM, so it should act like a HW, which means it's up to the guest kernel driver that handles the high rate DMA errors..

I'm mainly wondering what happens if the single physical kernel event queue overflows because it is DOS'd by a VM and the hypervisor cannot drain it fast enough?

I haven't looked closely but is there some kind of rate limiting or otherwise to mitigate DOS attacks on the shared event queue from VMs?

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 22, 2025 3:16 PM

Quoted 13 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

Quoted 3 lines

There is also the minor detail of what happens if the hypervisor HW queue overflows - I don't know the answer here. It is security concerning since the VM can spam DMA errors at high rate. :|

In my view, the hypervisor queue is the vHW queue for the VM, so it should act like a HW, which means it's up to the guest kernel driver that handles the high rate DMA errors..

I'm mainly wondering what happens if the single physical kernel event queue overflows because it is DOS'd by a VM and the hypervisor cannot drain it fast enough?

Quoted 2 lines

I haven't looked closely but is there some kind of rate limiting or otherwise to mitigate DOS attacks on the shared event queue from VMs?

SMMUv3 reads the event out of the physical kernel event queue, and adds that to faultq or veventq or prints it out. So, it'd not overflow because of DOS? And all other drivers should do the same?

Quoted 17 lines

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 22, 2025 3:16 PM

Quoted 13 lines · Jason Gunthorpe

Quoted 7 lines · Nicolin Chen

Quoted 3 lines

There is also the minor detail of what happens if the hypervisor HW queue overflows - I don't know the answer here. It is security concerning since the VM can spam DMA errors at high rate. :|

In my view, the hypervisor queue is the vHW queue for the VM, so it should act like a HW, which means it's up to the guest kernel driver that handles the high rate DMA errors..

I'm mainly wondering what happens if the single physical kernel event queue overflows because it is DOS'd by a VM and the hypervisor cannot drain it fast enough?

Quoted 8 lines

Quoted 2 lines

I haven't looked closely but is there some kind of rate limiting or otherwise to mitigate DOS attacks on the shared event queue from VMs?

SMMUv3 reads the event out of the physical kernel event queue, and adds that to faultq or veventq or prints it out. So, it'd not overflow because of DOS? And all other drivers should do the same?

"add that to faultq or eventq" could take time or the irqthread could be preempted for various reasons then there is always an window within which an overflow condition could occur due to the smmu driver incapable of fetching pending events timely.

On VT-d the driver could disable reporting non-recoverable fault for a given device via a control bit in the PASID entry, but I didn't see a similar knob for PRQ.

ARM seems to also have an interesting event merging feature:
STE.MEV, bit [83]
Merge Events arising from terminated transactions from this stream.
0b0 Do not merge similar fault records
0b1 Permit similar fault records to be merged
The SMMU might be able to reduce the usage of the Event queue by coalescing fault records that share the same page granule of address, access type and SubstreamID.
Setting MEV == 1 does not guarantee that faults will be coalesced. Setting MEV == 0 causes a physical SMMU to prevent coalescing of fault records, however, a hypervisor might not honour this setting if it deems a guest to be too verbose.
Note: Software must expect, and be able to deal with, coalesced fault records even when MEV == 0.

Yet, the driver doesn't seem to care setting it at this moment.

Quoted 3 lines · Jason Gunthorpe

Since we don't hold the spinlock the whole time there is a race where we could pull the overflow off and then another entry could be pushed while we do the copy_to_user.

I see. I'll be careful around that. I can imagine that one tricky thing can be to restore the overflow node back to the list when a copy_to_user fails..

Quoted 8 lines · Nicolin Chen

Quoted 3 lines · Jason Gunthorpe

Since we don't hold the spinlock the whole time there is a race where we could pull the overflow off and then another entry could be pushed while we do the copy_to_user.

I see. I'll be careful around that. I can imagine that one tricky thing can be to restore the overflow node back to the list when a copy_to_user fails..

Hmm, it gets trickier because the overflow node is a preallocated single node per vEVENTQ. We must not only check list_empty for its restore, but also protect the overflow->header.sequence from races between atomic_inc and copy_to_user. However, we can't use a mutex because copy_to_user might DOS...

A simple solution could be to just duplicate overflow nodes, each of which contains a different sequence, like a regular vEVENT node. This certainly changes the uAPI for read(). Though the duplication of overflow nodes doesn't sound optimal, it's acceptable since the duplicated nodes would have been regular vEVENT nodes if there was no overflow (i.e. no extra overhead)?

Quoted 16 lines · Nicolin Chen

Quoted 8 lines · Nicolin Chen

Quoted 3 lines · Jason Gunthorpe

Since we don't hold the spinlock the whole time there is a race where we could pull the overflow off and then another entry could be pushed while we do the copy_to_user.

I see. I'll be careful around that. I can imagine that one tricky thing can be to restore the overflow node back to the list when a copy_to_user fails..

Hmm, it gets trickier because the overflow node is a preallocated single node per vEVENTQ. We must not only check list_empty for its restore, but also protect the overflow->header.sequence from races between atomic_inc and copy_to_user. However, we can't use a mutex because copy_to_user might DOS...

Quoted 6 lines

A simple solution could be to just duplicate overflow nodes, each of which contains a different sequence, like a regular vEVENT node. This certainly changes the uAPI for read(). Though the duplication of overflow nodes doesn't sound optimal, it's acceptable since the duplicated nodes would have been regular vEVENT nodes if there was no overflow (i.e. no extra overhead)?

Ah, didn't think clearly last night.. We can't simply add overflow nodes either for rate/memory limit reason that you concerned about in the other email. On the other hand, though certainly not being ideal, indulging the race at the sequence index might not be that harmful, compared to the extreme of the other case..

I'll give another thought today if there's some other way out.

I made a change to duplicate the overflow node in the fetch() that is protected by the spinlock, which is used for copy_to_user. Then the other routine for vEVENT reporting, protected by the spinlock, can race-freely update the preallocated overflow node.

From: Nicolin Chen <[email protected]>
Sent: Wednesday, January 8, 2025 1:10 AM

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

Reviewed-by Lu Baolu <[email protected]>

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

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

+int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
+				    struct iommu_domain *domain)
+{
+	struct arm_smmu_nested_domain *nested_domain;
+	struct arm_smmu_vmaster *vmaster;
+	unsigned long vsid;
+	unsigned int cfg;
+

Quoted 8 lines · Nicolin Chen

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

+int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
+				    struct iommu_domain *domain)
+{
+	struct arm_smmu_nested_domain *nested_domain;
+	struct arm_smmu_vmaster *vmaster;
+	unsigned long vsid;
+	unsigned int cfg;
+

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c static void arm_smmu_make_nested_domain_ste( · line 105

Quoted 10 lines

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

+	iommu_group_mutex_assert(state->master->dev);
+
+	if (domain->type != IOMMU_DOMAIN_NESTED)
+		return 0;
+	nested_domain = to_smmu_nested_domain(domain);
+
+	/* Skip ABORT/BYPASS or invalid vSTE */
+	cfg = FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(nested_domain->ste[0]));
+	if (cfg == STRTAB_STE_0_CFG_ABORT || cfg == STRTAB_STE_0_CFG_BYPASS)
+		return 0;

Why? If the VM sets an ABORT vSTE then I would expect that any protection violation events the VM triggers are captured and forwarded as well?

Basically any time a vSTE is in place we should capture events that are affiliated with the SID?

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c static void arm_smmu_make_nested_domain_ste( · line 113

Quoted 8 lines

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

+	if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
+		return 0;
+
+	vsid = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
+					  state->master->dev);
+	/* Fail the attach if vSID is not correct set by the user space */
+	if (!vsid)
+		return -ENOENT;

Is it really OK that 0 is being used as invalid here?

sparse warnings: (new ones prefixed by >>)

Quoted 1 line

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:461:21: sparse: sparse: invalid assignment: &=

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:461:21: sparse: left side has type restricted __le64
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:461:21: sparse: right side has type unsigned long long
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:462:21: sparse: sparse: invalid assignment: |=
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:462:21: sparse: left side has type restricted __le64
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:462:21: sparse: right side has type unsigned long long

Quoted 1 line

Quoted 1 line

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:464:23: sparse: sparse: cast from restricted __le64

drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c:465:23: sparse: sparse: cast from restricted __le64