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

kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/2] irqbypass/kvm: Silence registration errors
@ 2016-05-05 17:58 Alex Williamson
  2016-05-05 17:58 ` [PATCH 1/2] irqbypass: Disallow NULL token Alex Williamson
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Alex Williamson @ 2016-05-05 17:58 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, feng.wu, linux-kernel, eric.auger

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] irqbypass: Disallow NULL token
  2016-05-05 17:58 [PATCH 0/2] irqbypass/kvm: Silence registration errors Alex Williamson
@ 2016-05-05 17:58 ` Alex Williamson
  2016-05-05 17:58 ` [PATCH 2/2] kvm: Conditionally register IRQ bypass consumer Alex Williamson
  2016-05-11 14:49 ` [PATCH 0/2] irqbypass/kvm: Silence registration errors Paolo Bonzini
  2 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2016-05-05 17:58 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, feng.wu, linux-kernel, eric.auger

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] kvm: Conditionally register IRQ bypass consumer
  2016-05-05 17:58 [PATCH 0/2] irqbypass/kvm: Silence registration errors Alex Williamson
  2016-05-05 17:58 ` [PATCH 1/2] irqbypass: Disallow NULL token Alex Williamson
@ 2016-05-05 17:58 ` Alex Williamson
  2016-05-11 14:49 ` [PATCH 0/2] irqbypass/kvm: Silence registration errors Paolo Bonzini
  2 siblings, 0 replies; 4+ messages in thread
From: Alex Williamson @ 2016-05-05 17:58 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, feng.wu, linux-kernel, eric.auger

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;


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 0/2] irqbypass/kvm: Silence registration errors
  2016-05-05 17:58 [PATCH 0/2] irqbypass/kvm: Silence registration errors Alex Williamson
  2016-05-05 17:58 ` [PATCH 1/2] irqbypass: Disallow NULL token Alex Williamson
  2016-05-05 17:58 ` [PATCH 2/2] kvm: Conditionally register IRQ bypass consumer Alex Williamson
@ 2016-05-11 14:49 ` Paolo Bonzini
  2 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2016-05-11 14:49 UTC (permalink / raw)
  To: Alex Williamson, kvm; +Cc: feng.wu, linux-kernel, eric.auger



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-05-11 14:49 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-05 17:58 [PATCH 0/2] irqbypass/kvm: Silence registration errors Alex Williamson
2016-05-05 17:58 ` [PATCH 1/2] irqbypass: Disallow NULL token Alex Williamson
2016-05-05 17:58 ` [PATCH 2/2] kvm: Conditionally register IRQ bypass consumer Alex Williamson
2016-05-11 14:49 ` [PATCH 0/2] irqbypass/kvm: Silence registration errors Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).