Move ownership of IRQ bypass token tracking into irqbypass.ko, and
explicitly require callers to pass an eventfd_ctx structure instead of a
completely opaque token. Relying on producers and consumers to set the
token appropriately is error prone, and hiding the fact that the token must
be an eventfd_ctx pointer (for all intents and purposes) unnecessarily
obfuscates the code and makes it more brittle.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
drivers/vfio/pci/vfio_pci_intrs.c | 5 +----
drivers/vhost/vdpa.c | 4 +---
include/linux/irqbypass.h | 31 +++++++++++++++++--------------
virt/kvm/eventfd.c | 3 +--
virt/lib/irqbypass.c | 30 +++++++++++++++++++++---------
5 files changed, 41 insertions(+), 32 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 8382c5834335..c852643d8359 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -505,15 +505,12 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev,
if (ret)
goto out_put_eventfd_ctx;
- ctx->producer.token = trigger;
ctx->producer.irq = irq;
- ret = irq_bypass_register_producer(&ctx->producer);
+ ret = irq_bypass_register_producer(&ctx->producer, trigger);
if (unlikely(ret)) {
dev_info(&pdev->dev,
"irq bypass producer (token %p) registration fails: %d\n",
ctx->producer.token, ret);
-
- ctx->producer.token = NULL;
}
ctx->trigger = trigger;
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 5a49b5a6d496..2749290f892d 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -213,7 +213,7 @@ static void vhost_vdpa_setup_vq_irq(struct vhost_vdpa *v, u16 qid)
return;
vq->call_ctx.producer.irq = irq;
- ret = irq_bypass_register_producer(&vq->call_ctx.producer);
+ ret = irq_bypass_register_producer(&vq->call_ctx.producer, vq->call_ctx.ctx);
if (unlikely(ret))
dev_info(&v->dev, "vq %u, irq bypass producer (token %p) registration fails, ret = %d\n",
qid, vq->call_ctx.producer.token, ret);
@@ -712,7 +712,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
if (ops->get_status(vdpa) &
VIRTIO_CONFIG_S_DRIVER_OK)
vhost_vdpa_unsetup_vq_irq(v, idx);
- vq->call_ctx.producer.token = NULL;
}
break;
}
@@ -753,7 +752,6 @@ static long vhost_vdpa_vring_ioctl(struct vhost_vdpa *v, unsigned int cmd,
cb.callback = vhost_vdpa_virtqueue_cb;
cb.private = vq;
cb.trigger = vq->call_ctx.ctx;
- vq->call_ctx.producer.token = vq->call_ctx.ctx;
if (ops->get_status(vdpa) &
VIRTIO_CONFIG_S_DRIVER_OK)
vhost_vdpa_setup_vq_irq(v, idx);
diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h
index 9bdb2a781841..379725b9a003 100644
--- a/include/linux/irqbypass.h
+++ b/include/linux/irqbypass.h
@@ -10,6 +10,7 @@
#include <linux/list.h>
+struct eventfd_ctx;
struct irq_bypass_consumer;
/*
@@ -18,20 +19,20 @@ struct irq_bypass_consumer;
* The IRQ bypass manager is a simple set of lists and callbacks that allows
* IRQ producers (ex. physical interrupt sources) to be matched to IRQ
* consumers (ex. virtualization hardware that allows IRQ bypass or offload)
- * via a shared token (ex. eventfd_ctx). Producers and consumers register
- * independently. When a token match is found, the optional @stop callback
- * will be called for each participant. The pair will then be connected via
- * the @add_* callbacks, and finally the optional @start callback will allow
- * any final coordination. When either participant is unregistered, the
- * process is repeated using the @del_* callbacks in place of the @add_*
- * callbacks. Match tokens must be unique per producer/consumer, 1:N pairings
- * are not supported.
+ * via a shared eventfd_ctx). Producers and consumers register independently.
+ * When a producer and consumer are paired, i.e. a token match is found, the
+ * optional @stop callback will be called for each participant. The pair will
+ * then be connected via the @add_* callbacks, and finally the optional @start
+ * callback will allow any final coordination. When either participant is
+ * unregistered, the process is repeated using the @del_* callbacks in place of
+ * the @add_* callbacks. Match tokens must be unique per producer/consumer,
+ * 1:N pairings are not supported.
*/
/**
* struct irq_bypass_producer - IRQ bypass producer definition
* @node: IRQ bypass manager private list management
- * @token: opaque token to match between producer and consumer (non-NULL)
+ * @token: IRQ bypass manage private token to match producers and consumers
* @irq: Linux IRQ number for the producer device
* @add_consumer: Connect the IRQ producer to an IRQ consumer (optional)
* @del_consumer: Disconnect the IRQ producer from an IRQ consumer (optional)
@@ -57,7 +58,7 @@ struct irq_bypass_producer {
/**
* struct irq_bypass_consumer - IRQ bypass consumer definition
* @node: IRQ bypass manager private list management
- * @token: opaque token to match between producer and consumer (non-NULL)
+ * @token: IRQ bypass manage private token to match producers and consumers
* @add_producer: Connect the IRQ consumer to an IRQ producer
* @del_producer: Disconnect the IRQ consumer from an IRQ producer
* @stop: Perform any quiesce operations necessary prior to add/del (optional)
@@ -79,9 +80,11 @@ struct irq_bypass_consumer {
void (*start)(struct irq_bypass_consumer *);
};
-int irq_bypass_register_producer(struct irq_bypass_producer *);
-void irq_bypass_unregister_producer(struct irq_bypass_producer *);
-int irq_bypass_register_consumer(struct irq_bypass_consumer *);
-void irq_bypass_unregister_consumer(struct irq_bypass_consumer *);
+int irq_bypass_register_producer(struct irq_bypass_producer *producer,
+ struct eventfd_ctx *eventfd);
+void irq_bypass_unregister_producer(struct irq_bypass_producer *producer);
+int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer,
+ struct eventfd_ctx *eventfd);
+void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer);
#endif /* IRQBYPASS_H */
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 249ba5b72e9b..afdbac0d0b9f 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -426,12 +426,11 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
#ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
if (kvm_arch_has_irq_bypass()) {
- irqfd->consumer.token = (void *)irqfd->eventfd;
irqfd->consumer.add_producer = kvm_arch_irq_bypass_add_producer;
irqfd->consumer.del_producer = kvm_arch_irq_bypass_del_producer;
irqfd->consumer.stop = kvm_arch_irq_bypass_stop;
irqfd->consumer.start = kvm_arch_irq_bypass_start;
- ret = irq_bypass_register_consumer(&irqfd->consumer);
+ ret = irq_bypass_register_consumer(&irqfd->consumer, irqfd->eventfd);
if (ret)
pr_info("irq bypass consumer (token %p) registration fails: %d\n",
irqfd->consumer.token, ret);
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index 28a4d933569a..98bf76d03078 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -77,30 +77,32 @@ static void __disconnect(struct irq_bypass_producer *prod,
/**
* irq_bypass_register_producer - register IRQ bypass producer
* @producer: pointer to producer structure
+ * @eventfd: pointer to the eventfd context associated with the producer
*
* Add the provided IRQ producer to the list of producers and connect
* with any matching token found on the IRQ consumers list.
*/
-int irq_bypass_register_producer(struct irq_bypass_producer *producer)
+int irq_bypass_register_producer(struct irq_bypass_producer *producer,
+ struct eventfd_ctx *eventfd)
{
struct irq_bypass_producer *tmp;
struct irq_bypass_consumer *consumer;
int ret;
- if (!producer->token)
+ if (WARN_ON_ONCE(producer->token))
return -EINVAL;
mutex_lock(&lock);
list_for_each_entry(tmp, &producers, node) {
- if (tmp->token == producer->token) {
+ if (tmp->token == eventfd) {
ret = -EBUSY;
goto out_err;
}
}
list_for_each_entry(consumer, &consumers, node) {
- if (consumer->token == producer->token) {
+ if (consumer->token == eventfd) {
ret = __connect(producer, consumer);
if (ret)
goto out_err;
@@ -108,6 +110,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer)
}
}
+ producer->token = eventfd;
list_add(&producer->node, &producers);
mutex_unlock(&lock);
@@ -147,10 +150,12 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer)
}
}
+ producer->token = NULL;
list_del(&producer->node);
break;
}
+ WARN_ON_ONCE(producer->token);
mutex_unlock(&lock);
}
EXPORT_SYMBOL_GPL(irq_bypass_unregister_producer);
@@ -158,31 +163,35 @@ EXPORT_SYMBOL_GPL(irq_bypass_unregister_producer);
/**
* irq_bypass_register_consumer - register IRQ bypass consumer
* @consumer: pointer to consumer structure
+ * @eventfd: pointer to the eventfd context associated with the consumer
*
* Add the provided IRQ consumer to the list of consumers and connect
* with any matching token found on the IRQ producer list.
*/
-int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer)
+int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer,
+ struct eventfd_ctx *eventfd)
{
struct irq_bypass_consumer *tmp;
struct irq_bypass_producer *producer;
int ret;
- if (!consumer->token ||
- !consumer->add_producer || !consumer->del_producer)
+ if (WARN_ON_ONCE(consumer->token))
+ return -EINVAL;
+
+ if (!consumer->add_producer || !consumer->del_producer)
return -EINVAL;
mutex_lock(&lock);
list_for_each_entry(tmp, &consumers, node) {
- if (tmp->token == consumer->token || tmp == consumer) {
+ if (tmp->token == eventfd || tmp == consumer) {
ret = -EBUSY;
goto out_err;
}
}
list_for_each_entry(producer, &producers, node) {
- if (producer->token == consumer->token) {
+ if (producer->token == eventfd) {
ret = __connect(producer, consumer);
if (ret)
goto out_err;
@@ -190,6 +199,7 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer)
}
}
+ consumer->token = eventfd;
list_add(&consumer->node, &consumers);
mutex_unlock(&lock);
@@ -229,10 +239,12 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer)
}
}
+ consumer->token = NULL;
list_del(&consumer->node);
break;
}
+ WARN_ON_ONCE(consumer->token);
mutex_unlock(&lock);
}
EXPORT_SYMBOL_GPL(irq_bypass_unregister_consumer);
--
2.49.0.504.g3bcea36a83-googOn Fri, 4 Apr 2025 14:14:45 -0700 Sean Christopherson <seanjc@google.com> wrote: > diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h > index 9bdb2a781841..379725b9a003 100644 > --- a/include/linux/irqbypass.h > +++ b/include/linux/irqbypass.h > @@ -10,6 +10,7 @@ > > #include <linux/list.h> > > +struct eventfd_ctx; > struct irq_bypass_consumer; > > /* > @@ -18,20 +19,20 @@ struct irq_bypass_consumer; > * The IRQ bypass manager is a simple set of lists and callbacks that allows > * IRQ producers (ex. physical interrupt sources) to be matched to IRQ > * consumers (ex. virtualization hardware that allows IRQ bypass or offload) > - * via a shared token (ex. eventfd_ctx). Producers and consumers register > - * independently. When a token match is found, the optional @stop callback > - * will be called for each participant. The pair will then be connected via > - * the @add_* callbacks, and finally the optional @start callback will allow > - * any final coordination. When either participant is unregistered, the > - * process is repeated using the @del_* callbacks in place of the @add_* > - * callbacks. Match tokens must be unique per producer/consumer, 1:N pairings > - * are not supported. > + * via a shared eventfd_ctx). Producers and consumers register independently. > + * When a producer and consumer are paired, i.e. a token match is found, the > + * optional @stop callback will be called for each participant. The pair will > + * then be connected via the @add_* callbacks, and finally the optional @start > + * callback will allow any final coordination. When either participant is > + * unregistered, the process is repeated using the @del_* callbacks in place of > + * the @add_* callbacks. Match tokens must be unique per producer/consumer, > + * 1:N pairings are not supported. > */ > > /** > * struct irq_bypass_producer - IRQ bypass producer definition > * @node: IRQ bypass manager private list management > - * @token: opaque token to match between producer and consumer (non-NULL) > + * @token: IRQ bypass manage private token to match producers and consumers The "token" terminology seems a little out of place after all is said and done in this series. Should it just be an "index" in anticipation of the usage with xarray and changed to an unsigned long? Or at least s/token/eventfd/ and changed to an eventfd_ctx pointer? Thanks, Alex
On Thu, Apr 10, 2025, Alex Williamson wrote: > On Fri, 4 Apr 2025 14:14:45 -0700 > Sean Christopherson <seanjc@google.com> wrote: > > diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h > > index 9bdb2a781841..379725b9a003 100644 > > --- a/include/linux/irqbypass.h > > +++ b/include/linux/irqbypass.h > > @@ -10,6 +10,7 @@ > > > > #include <linux/list.h> > > > > +struct eventfd_ctx; > > struct irq_bypass_consumer; > > > > /* > > @@ -18,20 +19,20 @@ struct irq_bypass_consumer; > > * The IRQ bypass manager is a simple set of lists and callbacks that allows > > * IRQ producers (ex. physical interrupt sources) to be matched to IRQ > > * consumers (ex. virtualization hardware that allows IRQ bypass or offload) > > - * via a shared token (ex. eventfd_ctx). Producers and consumers register > > - * independently. When a token match is found, the optional @stop callback > > - * will be called for each participant. The pair will then be connected via > > - * the @add_* callbacks, and finally the optional @start callback will allow > > - * any final coordination. When either participant is unregistered, the > > - * process is repeated using the @del_* callbacks in place of the @add_* > > - * callbacks. Match tokens must be unique per producer/consumer, 1:N pairings > > - * are not supported. > > + * via a shared eventfd_ctx). Producers and consumers register independently. > > + * When a producer and consumer are paired, i.e. a token match is found, the > > + * optional @stop callback will be called for each participant. The pair will > > + * then be connected via the @add_* callbacks, and finally the optional @start > > + * callback will allow any final coordination. When either participant is > > + * unregistered, the process is repeated using the @del_* callbacks in place of > > + * the @add_* callbacks. Match tokens must be unique per producer/consumer, > > + * 1:N pairings are not supported. > > */ > > > > /** > > * struct irq_bypass_producer - IRQ bypass producer definition > > * @node: IRQ bypass manager private list management > > - * @token: opaque token to match between producer and consumer (non-NULL) > > + * @token: IRQ bypass manage private token to match producers and consumers > > The "token" terminology seems a little out of place after all is said > and done in this series. Ugh, yeah, good point. I don't know why I left it as "token". > Should it just be an "index" in anticipation of the usage with xarray and > changed to an unsigned long? Or at least s/token/eventfd/ and changed to an > eventfd_ctx pointer? My strong vote is for "struct eventfd_ctx *eventfd;"
On Thu, 10 Apr 2025 15:04:35 -0700 Sean Christopherson <seanjc@google.com> wrote: > On Thu, Apr 10, 2025, Alex Williamson wrote: > > The "token" terminology seems a little out of place after all is said > > and done in this series. > > Ugh, yeah, good point. I don't know why I left it as "token". > > > Should it just be an "index" in anticipation of the usage with xarray and > > changed to an unsigned long? Or at least s/token/eventfd/ and changed to an > > eventfd_ctx pointer? > > My strong vote is for "struct eventfd_ctx *eventfd;" WFM, thanks, Alex
> From: Sean Christopherson <seanjc@google.com>
> Sent: Saturday, April 5, 2025 5:15 AM
>
> @@ -505,15 +505,12 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_core_device *vdev,
> if (ret)
> goto out_put_eventfd_ctx;
>
> - ctx->producer.token = trigger;
> ctx->producer.irq = irq;
> - ret = irq_bypass_register_producer(&ctx->producer);
> + ret = irq_bypass_register_producer(&ctx->producer, trigger);
> if (unlikely(ret)) {
> dev_info(&pdev->dev,
> "irq bypass producer (token %p) registration fails: %d\n",
> ctx->producer.token, ret);
Use 'trigger' as ctx->producer.token is NULL if registration fails.
> @@ -18,20 +19,20 @@ struct irq_bypass_consumer;
> * The IRQ bypass manager is a simple set of lists and callbacks that allows
> * IRQ producers (ex. physical interrupt sources) to be matched to IRQ
> * consumers (ex. virtualization hardware that allows IRQ bypass or offload)
> - * via a shared token (ex. eventfd_ctx). Producers and consumers register
> - * independently. When a token match is found, the optional @stop
> callback
> - * will be called for each participant. The pair will then be connected via
> - * the @add_* callbacks, and finally the optional @start callback will allow
> - * any final coordination. When either participant is unregistered, the
> - * process is repeated using the @del_* callbacks in place of the @add_*
> - * callbacks. Match tokens must be unique per producer/consumer, 1:N
> pairings
> - * are not supported.
> + * via a shared eventfd_ctx). Producers and consumers register
> independently.
s/eventfd_ctx)/eventfd_ctx/
> +int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer,
> + struct eventfd_ctx *eventfd)
> {
> struct irq_bypass_consumer *tmp;
> struct irq_bypass_producer *producer;
> int ret;
>
> - if (!consumer->token ||
> - !consumer->add_producer || !consumer->del_producer)
> + if (WARN_ON_ONCE(consumer->token))
> + return -EINVAL;
> +
> + if (!consumer->add_producer || !consumer->del_producer)
> return -EINVAL;
>
> mutex_lock(&lock);
>
> list_for_each_entry(tmp, &consumers, node) {
> - if (tmp->token == consumer->token || tmp == consumer) {
> + if (tmp->token == eventfd || tmp == consumer) {
> ret = -EBUSY;
> goto out_err;
> }
> }
the 2nd check 'tmp == consumer' is redundant. If they are equal
consumer->token is not NULL then the earlier WARN_ON will be
triggered already.
otherwise,
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Thu, Apr 10, 2025, Kevin Tian wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > +int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer,
> > + struct eventfd_ctx *eventfd)
> > {
> > struct irq_bypass_consumer *tmp;
> > struct irq_bypass_producer *producer;
> > int ret;
> >
> > - if (!consumer->token ||
> > - !consumer->add_producer || !consumer->del_producer)
> > + if (WARN_ON_ONCE(consumer->token))
> > + return -EINVAL;
> > +
> > + if (!consumer->add_producer || !consumer->del_producer)
> > return -EINVAL;
> >
> > mutex_lock(&lock);
> >
> > list_for_each_entry(tmp, &consumers, node) {
> > - if (tmp->token == consumer->token || tmp == consumer) {
> > + if (tmp->token == eventfd || tmp == consumer) {
> > ret = -EBUSY;
> > goto out_err;
> > }
> > }
>
> the 2nd check 'tmp == consumer' is redundant. If they are equal
> consumer->token is not NULL then the earlier WARN_ON will be
> triggered already.
Oh, nice. Good catch! That check subtly gets dropped on the conversion to
xarray, so it definitely makes sense to remove it in this patch.
© 2016 - 2026 Red Hat, Inc.