[RFC,V2,14/18] vfio/pci: Add core IMS support

Message ID 0f8fb7122814c5c5083799e935d5bd3d27d38aef.1696609476.git.reinette.chatre@intel.com
State New
Headers
Series vfio/pci: Back guest interrupts from Interrupt Message Store (IMS) |

Commit Message

Reinette Chatre Oct. 6, 2023, 4:41 p.m. UTC
  Add a new interrupt management backend enabling a guest MSI-X
interrupt to be backed by an IMS interrupt on the host.

An IMS interrupt is allocated via pci_ims_alloc_irq() and requires
an implementation specific cookie that is opaque to the IMS backend.
This can be a PASID, queue ID, pointer etc. During initialization
the IMS backend learns which PCI device to operate on (and thus which
interrupt domain to allocate from) and what the default cookie should
be for any new interrupt allocation.
IMS can associate a unique cookie with each vector (more support in
later patches) and to maintain this association the backend maintains
interrupt contexts for the virtual device's lifetime.

A virtual device driver starts by initializing the backend
using new vfio_pci_ims_init_intr_ctx(), cleanup using new
vfio_pci_ims_release_intr_ctx(). Once initialized the virtual
device driver can call vfio_pci_set_irqs_ioctl() to handle the
VFIO_DEVICE_SET_IRQS ioctl() after it has validated the parameters
to be appropriate for the particular device.

To support the IMS backend the core utilities need to be aware
which interrupt context it interacts with. New ims_backed_irq
enables this and is false for the PCI passthrough backend and
true for the IMS backend.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/vfio/pci/vfio_pci_intrs.c | 275 ++++++++++++++++++++++++++++++
 include/linux/vfio_pci_core.h     |   6 +
 2 files changed, 281 insertions(+)
  

Comments

Tian, Kevin Oct. 13, 2023, 8:10 a.m. UTC | #1
> From: Chatre, Reinette <reinette.chatre@intel.com>
> Sent: Saturday, October 7, 2023 12:41 AM
> 
> A virtual device driver starts by initializing the backend
> using new vfio_pci_ims_init_intr_ctx(), cleanup using new
> vfio_pci_ims_release_intr_ctx(). Once initialized the virtual
> device driver can call vfio_pci_set_irqs_ioctl() to handle the
> VFIO_DEVICE_SET_IRQS ioctl() after it has validated the parameters
> to be appropriate for the particular device.

I wonder whether the code sharing can go deeper from
vfio_pci_set_irqs_ioctl() all the way down to set_vector_signal()
with proper abstraction. Then handle emulated interrupt in the
common code instead of ims specific path. intel gvt also uses
emulated interrupt, which could be converted to use this library
too.

There is some subtle difference between pci/ims backends
regarding to how set_vector_signal() is coded in this series. But
it is not intuitive to me whether such a difference is conceptual
or simply from a coding preference.

Would you mind doing an exercise whether that is achievable?

Thanks
Kevin
  
Reinette Chatre Oct. 16, 2023, 5:48 p.m. UTC | #2
Hi Kevin,

On 10/13/2023 1:10 AM, Tian, Kevin wrote:
>> From: Chatre, Reinette <reinette.chatre@intel.com>
>> Sent: Saturday, October 7, 2023 12:41 AM
>>
>> A virtual device driver starts by initializing the backend
>> using new vfio_pci_ims_init_intr_ctx(), cleanup using new
>> vfio_pci_ims_release_intr_ctx(). Once initialized the virtual
>> device driver can call vfio_pci_set_irqs_ioctl() to handle the
>> VFIO_DEVICE_SET_IRQS ioctl() after it has validated the parameters
>> to be appropriate for the particular device.
> 
> I wonder whether the code sharing can go deeper from
> vfio_pci_set_irqs_ioctl() all the way down to set_vector_signal()
> with proper abstraction. 

There is a foundational difference in the MSI and IMS interrupt
management that is handled by the separate set_vector_signal()
implementations.

For MSI interrupts the interrupts stay allocated but the individual
interrupt context is always freed and re-allocated.

For IMS the interrupts are always freed and re-allocated (to ensure that
any new cookie is taken into account) while the individual interrupt
context stays allocated (to not lose the cookie value associated
with the individual interrupt).

It may indeed be possible to accommodate this difference with further
abstraction. I will study the code more to explore how this
can be done.

> Then handle emulated interrupt in the
> common code instead of ims specific path. intel gvt also uses
> emulated interrupt, which could be converted to use this library
> too.

Thank you for pointing me to intel gvt. 

> There is some subtle difference between pci/ims backends
> regarding to how set_vector_signal() is coded in this series. But
> it is not intuitive to me whether such a difference is conceptual
> or simply from a coding preference.
> 
> Would you mind doing an exercise whether that is achievable?

I do not mind at all. Will do.

Thank you very much for taking a look and sharing your guidance.

Reinette
  

Patch

diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index d04a4477c201..7880fd4077a6 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -22,6 +22,21 @@ 
 
 #include "vfio_pci_priv.h"
 
+/*
+ * Interrupt Message Store (IMS) private interrupt context data
+ * @vdev:		Virtual device. Used for name of device in
+ *			request_irq().
+ * @pdev:		PCI device owning the IMS domain from where
+ *			interrupts are allocated.
+ * @default_cookie:	Default cookie used for IMS interrupts without unique
+ *			cookie.
+ */
+struct vfio_pci_ims {
+	struct vfio_device		*vdev;
+	struct pci_dev			*pdev;
+	union msi_instance_cookie	default_cookie;
+};
+
 struct vfio_pci_irq_ctx {
 	struct eventfd_ctx	*trigger;
 	struct virqfd		*unmask;
@@ -29,6 +44,9 @@  struct vfio_pci_irq_ctx {
 	char			*name;
 	bool			masked;
 	struct irq_bypass_producer	producer;
+	int			ims_id;
+	int			virq;
+	union msi_instance_cookie	icookie;
 };
 
 static bool irq_is(struct vfio_pci_core_device *vdev, int type)
@@ -72,6 +90,12 @@  vfio_irq_ctx_alloc(struct vfio_pci_intr_ctx *intr_ctx, unsigned long index)
 	if (!ctx)
 		return NULL;
 
+	if (intr_ctx->ims_backed_irq)  {
+		struct vfio_pci_ims *ims = intr_ctx->priv;
+
+		ctx->icookie = ims->default_cookie;
+	}
+
 	ret = xa_insert(&intr_ctx->ctx, index, ctx, GFP_KERNEL_ACCOUNT);
 	if (ret) {
 		kfree(ctx);
@@ -822,6 +846,7 @@  void vfio_pci_init_intr_ctx(struct vfio_pci_core_device *vdev,
 	_vfio_pci_init_intr_ctx(intr_ctx);
 	intr_ctx->ops = &vfio_pci_intr_ops;
 	intr_ctx->priv = vdev;
+	intr_ctx->ims_backed_irq = false;
 }
 EXPORT_SYMBOL_GPL(vfio_pci_init_intr_ctx);
 
@@ -831,6 +856,256 @@  void vfio_pci_release_intr_ctx(struct vfio_pci_intr_ctx *intr_ctx)
 }
 EXPORT_SYMBOL_GPL(vfio_pci_release_intr_ctx);
 
+/* Guest MSI-X interrupts backed by IMS host interrupts */
+
+/*
+ * Free the IMS interrupt associated with @ctx.
+ *
+ * For an IMS interrupt the interrupt is freed from the underlying
+ * PCI device's IMS domain.
+ */
+static void vfio_pci_ims_irq_free(struct vfio_pci_intr_ctx *intr_ctx,
+				  struct vfio_pci_irq_ctx *ctx)
+{
+	struct vfio_pci_ims *ims = intr_ctx->priv;
+	struct msi_map irq_map = {};
+
+	irq_map.index = ctx->ims_id;
+	irq_map.virq = ctx->virq;
+	pci_ims_free_irq(ims->pdev, irq_map);
+	ctx->ims_id = -EINVAL;
+	ctx->virq = 0;
+}
+
+/*
+ * Allocate a host IMS interrupt for @ctx.
+ *
+ * For an IMS interrupt the interrupt is allocated from the underlying
+ * PCI device's IMS domain.
+ */
+static int vfio_pci_ims_irq_alloc(struct vfio_pci_intr_ctx *intr_ctx,
+				  struct vfio_pci_irq_ctx *ctx)
+{
+	struct vfio_pci_ims *ims = intr_ctx->priv;
+	struct msi_map irq_map = {};
+
+	irq_map = pci_ims_alloc_irq(ims->pdev, &ctx->icookie, NULL);
+	if (irq_map.index < 0)
+		return irq_map.index;
+
+	ctx->ims_id = irq_map.index;
+	ctx->virq = irq_map.virq;
+
+	return 0;
+}
+
+static int vfio_pci_ims_set_vector_signal(struct vfio_pci_intr_ctx *intr_ctx,
+					  unsigned int vector, int fd)
+{
+	struct vfio_pci_ims *ims = intr_ctx->priv;
+	struct device *dev = &ims->vdev->device;
+	struct vfio_pci_irq_ctx *ctx;
+	struct eventfd_ctx *trigger;
+	int ret;
+
+	ctx = vfio_irq_ctx_get(intr_ctx, vector);
+
+	if (ctx && ctx->trigger) {
+		irq_bypass_unregister_producer(&ctx->producer);
+		free_irq(ctx->virq, ctx->trigger);
+		vfio_pci_ims_irq_free(intr_ctx, ctx);
+		kfree(ctx->name);
+		ctx->name = NULL;
+		eventfd_ctx_put(ctx->trigger);
+		ctx->trigger = NULL;
+	}
+
+	if (fd < 0)
+		return 0;
+
+	/* Interrupt contexts remain allocated until shutdown. */
+	if (!ctx) {
+		ctx = vfio_irq_ctx_alloc(intr_ctx, vector);
+		if (!ctx)
+			return -ENOMEM;
+	}
+
+	ctx->name = kasprintf(GFP_KERNEL, "vfio-ims[%d](%s)", vector,
+			      dev_name(dev));
+	if (!ctx->name)
+		return -ENOMEM;
+
+	trigger = eventfd_ctx_fdget(fd);
+	if (IS_ERR(trigger)) {
+		ret = PTR_ERR(trigger);
+		goto out_free_name;
+	}
+
+	ctx->trigger = trigger;
+
+	ret = vfio_pci_ims_irq_alloc(intr_ctx, ctx);
+	if (ret < 0)
+		goto out_put_eventfd_ctx;
+
+	ret = request_irq(ctx->virq, vfio_msihandler, 0, ctx->name,
+			  ctx->trigger);
+	if (ret < 0)
+		goto out_free_irq;
+
+	ctx->producer.token = ctx->trigger;
+	ctx->producer.irq = ctx->virq;
+	ret = irq_bypass_register_producer(&ctx->producer);
+	if (unlikely(ret)) {
+		dev_info(&ims->vdev->device,
+			 "irq bypass producer (token %p) registration fails: %d\n",
+			 &ctx->producer.token, ret);
+		ctx->producer.token = NULL;
+	}
+
+	return 0;
+
+out_free_irq:
+	vfio_pci_ims_irq_free(intr_ctx, ctx);
+out_put_eventfd_ctx:
+	eventfd_ctx_put(ctx->trigger);
+	ctx->trigger = NULL;
+out_free_name:
+	kfree(ctx->name);
+	ctx->name = NULL;
+	return ret;
+}
+
+static int vfio_pci_ims_set_block(struct vfio_pci_intr_ctx *intr_ctx,
+				  unsigned int start, unsigned int count,
+				  int *fds)
+{
+	unsigned int i, j;
+	int ret = 0;
+
+	for (i = 0, j = start; i < count && !ret; i++, j++) {
+		int fd = fds ? fds[i] : -1;
+
+		ret = vfio_pci_ims_set_vector_signal(intr_ctx, j, fd);
+	}
+
+	if (ret) {
+		for (i = start; i < j; i++)
+			vfio_pci_ims_set_vector_signal(intr_ctx, i, -1);
+	}
+
+	return ret;
+}
+
+/*
+ * Manage Interrupt Message Store (IMS) or emulated interrupts on the
+ * host that are backing guest MSI-X vectors.
+ *
+ * @intr_ctx:	 Interrupt context
+ * @index:	 Type of guest vectors to set up.  Must be
+ *		 VFIO_PCI_MSIX_IRQ_INDEX.
+ * @start:	 First vector index.
+ * @count:	 Number of vectors.
+ * @flags:	 Type of data provided in @data.
+ * @data:	 Data as specified by @flags.
+ *
+ * Caller is required to validate provided range for @vdev.
+ *
+ * Context: Interrupt context must be initialized via
+ *	    vfio_pci_ims_init_intr_ctx()  before any interrupts can be allocated.
+ *
+ * Return: Error code on failure or 0 on success.
+ */
+static int vfio_pci_set_ims_trigger(struct vfio_pci_intr_ctx *intr_ctx,
+				    unsigned int index, unsigned int start,
+				    unsigned int count, u32 flags,
+				    void *data)
+{
+	struct vfio_pci_irq_ctx *ctx;
+	unsigned long i;
+
+	if (index != VFIO_PCI_MSIX_IRQ_INDEX)
+		return -EINVAL;
+
+	if (!count && (flags & VFIO_IRQ_SET_DATA_NONE)) {
+		xa_for_each(&intr_ctx->ctx, i, ctx)
+			vfio_pci_ims_set_vector_signal(intr_ctx, i, -1);
+		return 0;
+	}
+
+	if (flags & VFIO_IRQ_SET_DATA_EVENTFD)
+		return vfio_pci_ims_set_block(intr_ctx, start, count, (int *)data);
+
+	for (i = start; i < start + count; i++) {
+		ctx = vfio_irq_ctx_get(intr_ctx, i);
+		if (!ctx || !ctx->trigger)
+			continue;
+		if (flags & VFIO_IRQ_SET_DATA_NONE) {
+			eventfd_signal(ctx->trigger, 1);
+		} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
+			uint8_t *bools = data;
+
+			if (bools[i - start])
+				eventfd_signal(ctx->trigger, 1);
+		}
+	}
+
+	return 0;
+}
+
+struct vfio_pci_intr_ops vfio_pci_ims_intr_ops = {
+	.set_msix_trigger = vfio_pci_set_ims_trigger,
+	.set_req_trigger = vfio_pci_set_req_trigger,
+};
+
+int vfio_pci_ims_init_intr_ctx(struct vfio_device *vdev,
+			       struct vfio_pci_intr_ctx *intr_ctx,
+			       struct pci_dev *pdev,
+			       union msi_instance_cookie *default_cookie)
+{
+	struct vfio_pci_ims *ims;
+
+	ims = kzalloc(sizeof(*ims), GFP_KERNEL_ACCOUNT);
+	if (!ims)
+		return -ENOMEM;
+
+	ims->pdev = pdev;
+	ims->default_cookie = *default_cookie;
+	ims->vdev = vdev;
+
+	_vfio_pci_init_intr_ctx(intr_ctx);
+
+	intr_ctx->ops = &vfio_pci_ims_intr_ops;
+	intr_ctx->priv = ims;
+	intr_ctx->ims_backed_irq = true;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_pci_ims_init_intr_ctx);
+
+void vfio_pci_ims_release_intr_ctx(struct vfio_pci_intr_ctx *intr_ctx)
+{
+	struct vfio_pci_ims *ims = intr_ctx->priv;
+	struct vfio_pci_irq_ctx *ctx;
+	unsigned long i;
+
+	/*
+	 * IMS backed MSI-X keeps interrupt context allocated after
+	 * interrupt is freed. Interrupt contexts need to be freed
+	 * separately.
+	 */
+	mutex_lock(&intr_ctx->igate);
+	xa_for_each(&intr_ctx->ctx, i, ctx) {
+		WARN_ON_ONCE(ctx->trigger);
+		WARN_ON_ONCE(ctx->name);
+		xa_erase(&intr_ctx->ctx, i);
+		kfree(ctx);
+	}
+	mutex_unlock(&intr_ctx->igate);
+	kfree(ims);
+	_vfio_pci_release_intr_ctx(intr_ctx);
+}
+EXPORT_SYMBOL_GPL(vfio_pci_ims_release_intr_ctx);
+
 int vfio_pci_set_irqs_ioctl(struct vfio_pci_intr_ctx *intr_ctx, uint32_t flags,
 			    unsigned int index, unsigned int start,
 			    unsigned int count, void *data)
diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 1dd55d98dce9..13b807848286 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -67,6 +67,7 @@  struct vfio_pci_intr_ctx {
 	struct eventfd_ctx		*req_trigger;
 	struct xarray			ctx;
 	int				irq_type;
+	bool				ims_backed_irq:1;
 };
 
 struct vfio_pci_intr_ops {
@@ -161,6 +162,11 @@  void vfio_pci_release_intr_ctx(struct vfio_pci_intr_ctx *intr_ctx);
 int vfio_pci_set_irqs_ioctl(struct vfio_pci_intr_ctx *intr_ctx, uint32_t flags,
 			    unsigned int index, unsigned int start,
 			    unsigned int count, void *data);
+int vfio_pci_ims_init_intr_ctx(struct vfio_device *vdev,
+			       struct vfio_pci_intr_ctx *intr_ctx,
+			       struct pci_dev *pdev,
+			       union msi_instance_cookie *default_cookie);
+void vfio_pci_ims_release_intr_ctx(struct vfio_pci_intr_ctx *intr_ctx);
 int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
 				void __user *arg, size_t argsz);
 ssize_t vfio_pci_core_read(struct vfio_device *core_vdev, char __user *buf,