[PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper

Nicolin Chen posted 14 patches 1 year ago
There is a newer version of this series
[PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Nicolin Chen 1 year ago
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 <baolu.lu@linux.intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 include/linux/iommufd.h        | 11 +++++++++
 drivers/iommu/iommufd/driver.c | 45 ++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
index 05cb393aff0a..60eff9272551 100644
--- a/include/linux/iommufd.h
+++ b/include/linux/iommufd.h
@@ -11,6 +11,7 @@
 #include <linux/refcount.h>
 #include <linux/types.h>
 #include <linux/xarray.h>
+#include <uapi/linux/iommufd.h>
 
 struct device;
 struct file;
@@ -192,6 +193,9 @@ struct device *iommufd_viommu_find_dev(struct iommufd_viommu *viommu,
 				       unsigned long vdev_id);
 int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
 			       struct device *dev, unsigned long *vdev_id);
+int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
+				enum iommu_veventq_type type, void *event_data,
+				size_t data_len);
 #else /* !CONFIG_IOMMUFD_DRIVER_CORE */
 static inline struct iommufd_object *
 _iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size,
@@ -212,6 +216,13 @@ static inline int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
 {
 	return -ENOENT;
 }
+
+static inline int iommufd_viommu_report_event(struct iommufd_viommu *viommu,
+					      enum iommu_veventq_type type,
+					      void *event_data, size_t data_len)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_IOMMUFD_DRIVER_CORE */
 
 /*
diff --git a/drivers/iommu/iommufd/driver.c b/drivers/iommu/iommufd/driver.c
index 185c4fde8987..9e52ce66204c 100644
--- a/drivers/iommu/iommufd/driver.c
+++ b/drivers/iommu/iommufd/driver.c
@@ -73,5 +73,50 @@ int iommufd_viommu_get_vdev_id(struct iommufd_viommu *viommu,
 }
 EXPORT_SYMBOL_NS_GPL(iommufd_viommu_get_vdev_id, "IOMMUFD");
 
+/*
+ * 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 (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;
+	}
+
+	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);
+	if (!vevent) {
+		rc = -ENOMEM;
+		goto out_unlock_veventqs;
+	}
+	memcpy(vevent->event_data, event_data, data_len);
+	vevent->data_len = data_len;
+	atomic_inc(&veventq->num_events);
+
+out_set_header:
+	iommufd_vevent_handler(veventq, vevent);
+out_unlock_veventqs:
+	up_read(&viommu->veventqs_rwsem);
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(iommufd_viommu_report_event, "IOMMUFD");
+
 MODULE_DESCRIPTION("iommufd code shared with builtin modules");
 MODULE_LICENSE("GPL");
-- 
2.43.0
Re: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Jason Gunthorpe 11 months, 3 weeks ago
On Fri, Jan 24, 2025 at 04:30:36PM -0800, Nicolin Chen wrote:
> +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;
> +	}
> +
> +	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);
> +	if (!vevent) {
> +		rc = -ENOMEM;
> +		goto out_unlock_veventqs;

This should record an overflow too

Jason
RE: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Tian, Kevin 11 months, 3 weeks ago
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, February 18, 2025 11:36 PM
> 
> On Fri, Jan 24, 2025 at 04:30:36PM -0800, Nicolin Chen wrote:
> > +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;
> > +	}
> > +
> > +	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);
> > +	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.)
Re: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Nicolin Chen 11 months, 3 weeks ago
On Wed, Feb 19, 2025 at 06:58:16AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, February 18, 2025 11:36 PM
> > 
> > On Fri, Jan 24, 2025 at 04:30:36PM -0800, Nicolin Chen wrote:
> > > +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;
> > > +	}
> > > +
> > > +	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);
> > > +	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.)

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?

Thanks
Nicolin
RE: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Tian, Kevin 11 months, 3 weeks ago
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, February 21, 2025 5:16 AM
> 
> On Wed, Feb 19, 2025 at 06:58:16AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > Sent: Tuesday, February 18, 2025 11:36 PM
> > >
> > > On Fri, Jan 24, 2025 at 04:30:36PM -0800, Nicolin Chen wrote:
> > > > +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;
> > > > +	}
> > > > +
> > > > +	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);
> > > > +	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.)
> 
> 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?
> 

'Exception' is broader compared to lost events.

We may view it in this way. When the overflow node is not used, the
user detects the sequence gap between two event entries. In that
case the gap literally reveals about the number of lost events. No
reason for why those events are lost.

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.

Given it's a renaming, let's have consensus from Jason before you
make a change.

Thanks
Kevin
Re: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Jason Gunthorpe 11 months, 3 weeks ago
On Fri, Feb 21, 2025 at 04:27:32AM +0000, Tian, Kevin wrote:
> 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.

I also like lost events - it is simple and to the point

Jason
RE: [PATCH v6 07/14] iommufd/viommu: Add iommufd_viommu_report_event helper
Posted by Tian, Kevin 11 months, 3 weeks ago
> From: Nicolin Chen <nicolinc@nvidia.com>
> 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 <baolu.lu@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

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