[PATCH 0/2] irqbypass/kvm: Silence registration errors

[PATCH 0/2] irqbypass/kvm: Silence registration errors

[Alex Williamson]

Currently an AMD system using vfio-pci/kvm will issue a dev_info() for
every MSI/X vector registered because kvm_x86_ops does not support a
bypass mechanism except on Intel and the add_producer callback returns
an errno for that.  This is meaningless, unintended, and confuses
users.  We could simply have KVM's add_producer callback return 0 if
there's no bypass mechanism, but then why are we registering as an IRQ
bypass consumer in the first place?  We also don't necessarily want to
simply remove the dev_info/pr_info on registration failure because
then we have no warning if something does actually go wrong.  So
instead, let's conditionalize IRQ bypass registration on whether we
have any support for it, and to keep the de-registration path clean
and fill an unintended gap, let's ignore deregistration of NULL tokens
and prevent registration of the same.  NULL isn't a good, unique
cookie anyway.  Tested on AMD and non-PI Intel.  Thanks,

Alex

---

Alex Williamson (2):
      irqbypass: Disallow NULL token
      kvm: Conditionally register IRQ bypass consumer


 arch/x86/kvm/x86.c        |   19 ++++++++-----------
 include/linux/irqbypass.h |    4 ++--
 include/linux/kvm_host.h  |    1 +
 virt/kvm/eventfd.c        |   18 ++++++++++--------
 virt/lib/irqbypass.c      |   12 +++++++++++-
 5 files changed, 32 insertions(+), 22 deletions(-)

[PATCH 1/2] irqbypass: Disallow NULL token

[Alex Williamson]

A NULL token is meaningless and can only lead to unintended problems.
Error on registration with a NULL token, ignore de-registrations with
a NULL token.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 include/linux/irqbypass.h |    4 ++--
 virt/lib/irqbypass.c      |   12 +++++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/irqbypass.h b/include/linux/irqbypass.h
index 1551b5b..f0f5d26 100644
--- a/include/linux/irqbypass.h
+++ b/include/linux/irqbypass.h
@@ -34,7 +34,7 @@ struct irq_bypass_consumer;
 /**
  * struct irq_bypass_producer - IRQ bypass producer definition
  * @node: IRQ bypass manager private list management
- * @token: opaque token to match between producer and consumer
+ * @token: opaque token to match between producer and consumer (non-NULL)
  * @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)
@@ -60,7 +60,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
+ * @token: opaque token to match between producer and consumer (non-NULL)
  * @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)
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c
index 09a03b5..52abac4 100644
--- a/virt/lib/irqbypass.c
+++ b/virt/lib/irqbypass.c
@@ -89,6 +89,9 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer)
 	struct irq_bypass_producer *tmp;
 	struct irq_bypass_consumer *consumer;
 
+	if (!producer->token)
+		return -EINVAL;
+
 	might_sleep();
 
 	if (!try_module_get(THIS_MODULE))
@@ -136,6 +139,9 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer)
 	struct irq_bypass_producer *tmp;
 	struct irq_bypass_consumer *consumer;
 
+	if (!producer->token)
+		return;
+
 	might_sleep();
 
 	if (!try_module_get(THIS_MODULE))
@@ -177,7 +183,8 @@ int irq_bypass_register_consumer(struct irq_bypass_consumer *consumer)
 	struct irq_bypass_consumer *tmp;
 	struct irq_bypass_producer *producer;
 
-	if (!consumer->add_producer || !consumer->del_producer)
+	if (!consumer->token ||
+	    !consumer->add_producer || !consumer->del_producer)
 		return -EINVAL;
 
 	might_sleep();
@@ -227,6 +234,9 @@ void irq_bypass_unregister_consumer(struct irq_bypass_consumer *consumer)
 	struct irq_bypass_consumer *tmp;
 	struct irq_bypass_producer *producer;
 
+	if (!consumer->token)
+		return;
+
 	might_sleep();
 
 	if (!try_module_get(THIS_MODULE))

[PATCH 2/2] kvm: Conditionally register IRQ bypass consumer

[Alex Williamson]

If we don't support a mechanism for bypassing IRQs, don't register as
a consumer.  This eliminates meaningless dev_info()s when the connect
fails between producer and consumer, such as on AMD systems where
kvm_x86_ops->update_pi_irte is not implemented

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 arch/x86/kvm/x86.c       |   19 ++++++++-----------
 include/linux/kvm_host.h |    1 +
 virt/kvm/eventfd.c       |   18 ++++++++++--------
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b7798c..ac97a0e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8355,19 +8355,21 @@ bool kvm_arch_has_noncoherent_dma(struct kvm *kvm)
 }
 EXPORT_SYMBOL_GPL(kvm_arch_has_noncoherent_dma);
 
+bool kvm_arch_has_irq_bypass(void)
+{
+	return kvm_x86_ops->update_pi_irte != NULL;
+}
+
 int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *cons,
 				      struct irq_bypass_producer *prod)
 {
 	struct kvm_kernel_irqfd *irqfd =
 		container_of(cons, struct kvm_kernel_irqfd, consumer);
 
-	if (kvm_x86_ops->update_pi_irte) {
-		irqfd->producer = prod;
-		return kvm_x86_ops->update_pi_irte(irqfd->kvm,
-				prod->irq, irqfd->gsi, 1);
-	}
+	irqfd->producer = prod;
 
-	return -EINVAL;
+	return kvm_x86_ops->update_pi_irte(irqfd->kvm,
+					   prod->irq, irqfd->gsi, 1);
 }
 
 void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
@@ -8377,11 +8379,6 @@ void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *cons,
 	struct kvm_kernel_irqfd *irqfd =
 		container_of(cons, struct kvm_kernel_irqfd, consumer);
 
-	if (!kvm_x86_ops->update_pi_irte) {
-		WARN_ON(irqfd->producer != NULL);
-		return;
-	}
-
 	WARN_ON(irqfd->producer != prod);
 	irqfd->producer = NULL;
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5276fe0..3aa4aa6 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1169,6 +1169,7 @@ static inline void kvm_vcpu_set_dy_eligible(struct kvm_vcpu *vcpu, bool val)
 #endif /* CONFIG_HAVE_KVM_CPU_RELAX_INTERCEPT */
 
 #ifdef CONFIG_HAVE_KVM_IRQ_BYPASS
+bool kvm_arch_has_irq_bypass(void);
 int kvm_arch_irq_bypass_add_producer(struct irq_bypass_consumer *,
 			   struct irq_bypass_producer *);
 void kvm_arch_irq_bypass_del_producer(struct irq_bypass_consumer *,
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 46dbc0a..e469b60 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -408,15 +408,17 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
 	 */
 	fdput(f);
 #ifdef CONFIG_HAVE_KVM_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);
-	if (ret)
-		pr_info("irq bypass consumer (token %p) registration fails: %d\n",
+	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);
+		if (ret)
+			pr_info("irq bypass consumer (token %p) registration fails: %d\n",
 				irqfd->consumer.token, ret);
+	}
 #endif
 
 	return 0;

Re: [PATCH 0/2] irqbypass/kvm: Silence registration errors

[Paolo Bonzini]

On 05/05/2016 19:58, Alex Williamson wrote:
> Currently an AMD system using vfio-pci/kvm will issue a dev_info() for
> every MSI/X vector registered because kvm_x86_ops does not support a
> bypass mechanism except on Intel and the add_producer callback returns
> an errno for that.  This is meaningless, unintended, and confuses
> users.  We could simply have KVM's add_producer callback return 0 if
> there's no bypass mechanism, but then why are we registering as an IRQ
> bypass consumer in the first place?  We also don't necessarily want to
> simply remove the dev_info/pr_info on registration failure because
> then we have no warning if something does actually go wrong.  So
> instead, let's conditionalize IRQ bypass registration on whether we
> have any support for it, and to keep the de-registration path clean
> and fill an unintended gap, let's ignore deregistration of NULL tokens
> and prevent registration of the same.  NULL isn't a good, unique
> cookie anyway.  Tested on AMD and non-PI Intel.  Thanks,
> 
> Alex

Thanks, applying these patches to kvm/queue.

Paolo