KVM: IRQ ACK notifier should be used with in-kernel irqchip

PATCH Applied Sheng Yang 3 participants 17 messages Oct 20, 2008 – Dec 2, 2008 KVM

lore

Share & tools

lore

Sheng Yang

author

2008-10-20 08:08 UTC #0 lore raw

PATCH 1/1 KVM: IRQ ACK notifier should be used with in-kernel irqchip

Also remove unnecessary parameter of unregister irq ack notifier.

Signed-off-by Sheng Yang <[email protected]>

---
 include/linux/kvm_host.h |    3 +--
 virt/kvm/irq_comm.c      |    8 ++++++--
 virt/kvm/kvm_main.c      |    2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

include/linux/kvm_host.h +1 -2

index bb92be2..3a0fb77 100644
@@ -316,8 +316,7 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
-void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
-				     struct kvm_irq_ack_notifier *kian);
+void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian);
 int kvm_request_irq_source_id(struct kvm *kvm);
 void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);

virt/kvm/irq_comm.c +6 -2

index 55ad76e..9fbbdea 100644
@@ -58,12 +58,16 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian)
 {
+	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
+	ASSERT(irqchip_in_kernel(kvm));
+	ASSERT(kian);
 	hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
 }
 
-void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
-				     struct kvm_irq_ack_notifier *kian)
+void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
 {
+	if (!kian)
+		return;
 	hlist_del(&kian->link);
 }

virt/kvm/kvm_main.c +1 -1

index a87f45e..4f43abe 100644
@@ -143,7 +143,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested)
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 
-	kvm_unregister_irq_ack_notifier(kvm, &assigned_dev->ack_notifier);
+	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
 	kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
 
 	if (cancel_work_sync(&assigned_dev->interrupt_work))

Signature · 1 line

1.5.4.5

Avi Kivity

Applied

2008-10-22 10:26 UTC #1 lore raw

Quoted 2 lines · Sheng Yang

Also remove unnecessary parameter of unregister irq ack notifier.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Mark McLoughlin

2008-11-28 10:27 UTC #2 lore raw

In reply to Sheng Yang

Hi,

I just got an oops (with 2.6.28-rc6) when running "qemu-kvm -S -pcidevice ..." and immediately quitting rather than starting the guest.

The issue is that at this point ASSIGN_PCI_DEVICE has been called, but not ASSIGN_IRQ, so kvm_unregister_irq_ack_notifier() oops when we try and remove a notifier which hasn't already been added.

The fix is simple - use hlist_del_init() rather than hlist_del() - but I also came across this patch in Avi's tree ...

On Mon, 2008-10-20 at 16:07 +0800, Sheng Yang wrote:
...

virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi) · line 62

Quoted 10 lines

virt/kvm/irq_comm.c +2 -0

index 55ad76e..9fbbdea 100644
@@ -58,12 +58,16 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian)
 {
+	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
+	ASSERT(irqchip_in_kernel(kvm));

This is a seriously ugly assertion - there is no reason for the IRQ ACK notifier abstraction to know anything about when it is called, and it's easy to verify that kvm_register_irq_ack_notifier() is only called with the in-kernel irqchip ... it's only called in one place:

                if (irqchip_in_kernel(kvm)) {
                        /* Register ack nofitier */
                        match->ack_notifier.gsi = -1;
                        match->ack_notifier.irq_acked =
                                        kvm_assigned_dev_ack_irq;
                        kvm_register_irq_ack_notifier(kvm,
                                        &match->ack_notifier);

virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi) · line 63

Quoted 1 line

virt/kvm/irq_comm.c +1 -0
+	ASSERT(kian);

This is bogus; the ack notifier structure is embedded in assigned device structure, so we can never pass NULL here - it's not like it's a dynamically allocated structure.

virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi) · line 71

Quoted 10 lines

virt/kvm/irq_comm.c +3 -2

 	hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
 }
 
-void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
-				     struct kvm_irq_ack_notifier *kian)
+void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
 {
+	if (!kian)
+		return;
 	hlist_del(&kian->link);

This is where I think you were trying to fix the issue I saw ... but again, it's bogus. We will never pass a NULL ack notifier struct, but we may well pass one which hasn't been previously registered.

I'm going to follow up with a number of patches to clean some of this up.

Cheers,
Mark.

Mark McLoughlin

2008-11-28 10:27 UTC #3 lore raw

PATCH 1/4 KVM: remove the IRQ ACK notifier assertions

We will obviously never pass a NULL struct kvm_irq_ack_notifier* to this functions. They are always embedded in the assigned device structure, so the assertion add nothing.

The irqchip_in_kernel() assertion is very out of place - clearly this little abstraction needs to know nothing about the upper layer details.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

virt/kvm/irq_comm.c +0 -3

index 9fbbdea..973df99 100644
@@ -58,9 +58,6 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian)
 {
-	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
-	ASSERT(irqchip_in_kernel(kvm));
-	ASSERT(kian);
 	hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
 }

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-11-28 10:27 UTC #4 lore raw

PATCH 2/4 KVM: make kvm_unregister_irq_ack_notifier() safe

We never pass a NULL notifier pointer here, but we may well pass a notifier struct which hasn't previously been registered.

Guard against this by using hlist_del_init() which will not do anything if the node hasn't been added to the list and, when removing the node, will ensure that a subsequent call to hlist_del_init() will be fine too.

Fixes an oops seen when an assigned device is freed before and IRQ is assigned to it.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

virt/kvm/irq_comm.c +1 -3

index 973df99..db75045 100644
@@ -63,9 +63,7 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 
 void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
 {
-	if (!kian)
-		return;
-	hlist_del(&kian->link);
+	hlist_del_init(&kian->link);
 }
 
 /* The caller must hold kvm->lock mutex */

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-11-28 10:27 UTC #5 lore raw

PATCH 3/4 KVM: gracefully handle zero in kvm_free_irq_source_id()

Allow kvm_free_irq_source_id() to be called with a zero ID.

Zero is reserved for KVM_USERSPACE_IRQ_SOURCE_ID, so we can guarantee that kvm_request_irq_source_id() will never return zero and use zero to indicate "no source ID allocated".

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

virt/kvm/irq_comm.c +10 -4

index db75045..869f5e8 100644
@@ -72,11 +72,15 @@ int kvm_request_irq_source_id(struct kvm *kvm)
 	unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
 	int irq_source_id = find_first_zero_bit(bitmap,
 				sizeof(kvm->arch.irq_sources_bitmap));
+
 	if (irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
 		printk(KERN_WARNING "kvm: exhaust allocatable IRQ sources!\n");
-		irq_source_id = -EFAULT;
-	} else
-		set_bit(irq_source_id, bitmap);
+		return -EFAULT;
+	}
+
+	ASSERT(irq_source_id != 0); /* KVM_USERSPACE_IRQ_SOURCE_ID reserved */
+	set_bit(irq_source_id, bitmap);
+
 	return irq_source_id;
 }

@@ -84,7 +88,9 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 {
 	int i;
 
-	if (irq_source_id <= 0 ||
+	if (!irq_source_id)
+		return;
+	if (irq_source_id < 0 ||
 	    irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
 		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
 		return;

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-11-28 10:27 UTC #6 lore raw

PATCH 4/4 KVM: split out kvm_free_assigned_irq()

Split out the logic corresponding to undoing assign_irq() and clean it up a bit.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/kvm_main.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

virt/kvm/kvm_main.c +24 -7

index 54d25e6..bcdf715 100644
@@ -200,17 +200,19 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	enable_irq(dev->host_irq);
 }
 
-static void kvm_free_assigned_device(struct kvm *kvm,
-				     struct kvm_assigned_dev_kernel
-				     *assigned_dev)
+static void kvm_free_assigned_irq(struct kvm *kvm,
+				  struct kvm_assigned_dev_kernel *assigned_dev)
 {
-	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested_type)
-		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
-	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
-		pci_disable_msi(assigned_dev->dev);
+	if (!irqchip_in_kernel(kvm))
+		return;
 
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
+
 	kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
+	assigned_dev->irq_source_id = 0;
+
+	if (!assigned_dev->irq_requested_type)
+		return;
 
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take

@@ -218,6 +220,21 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 		 */
 		kvm_put_kvm(kvm);
 
+	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
+		pci_disable_msi(assigned_dev->dev);
+
+	assigned_dev->irq_requested_type = 0;
+}
+
+
+static void kvm_free_assigned_device(struct kvm *kvm,
+				     struct kvm_assigned_dev_kernel
+				     *assigned_dev)
+{
+	kvm_free_assigned_irq(kvm, assigned_dev);
+
 	pci_reset_function(assigned_dev->dev);
 
 	pci_release_regions(assigned_dev->dev);

Signature · 1 line

1.5.4.3

Avi Kivity

2008-11-30 10:28 UTC #7 lore raw

Re: [PATCH 3/4] KVM: gracefully handle zero in kvm_free_irq_source_id()

In reply to Mark McLoughlin

Quoted 7 lines · Mark McLoughlin

Allow kvm_free_irq_source_id() to be called with a zero ID.
Zero is reserved for KVM_USERSPACE_IRQ_SOURCE_ID, so we can guarantee that kvm_request_irq_source_id() will never return zero and use zero to indicate "no source ID allocated".

Zero is a legal value for irq source ids, overloading it as something else is confusing.

Things should continue to work if we #define it to 17.

virt/kvm/irq_comm.c int kvm_request_irq_source_id(struct kvm *kvm) · line 81

Quoted 4 lines

virt/kvm/irq_comm.c +3 -0

+	}
+
+	ASSERT(irq_source_id != 0); /* KVM_USERSPACE_IRQ_SOURCE_ID reserved */

Why not replace 0 with the actual symbolic constant?

Signature · 1 line

error compiling committee.c: too many arguments to function

Mark McLoughlin

2008-12-01 13:58 UTC #8 lore raw

Re: [PATCH 3/4] KVM: gracefully handle zero in kvm_free_irq_source_id()

Quoted 11 lines · Avi Kivity

Quoted 7 lines · Mark McLoughlin
Allow kvm_free_irq_source_id() to be called with a zero ID.
Zero is reserved for KVM_USERSPACE_IRQ_SOURCE_ID, so we can guarantee that kvm_request_irq_source_id() will never return zero and use zero to indicate "no source ID allocated".
Zero is a legal value for irq source ids, overloading it as something else is confusing.

Fair enough; I choose zero because it's naturally initialised to that by the kzalloc(). But I prefer explicit initialisation anyway, so ...

Quoted 1 line

Things should continue to work if we #define it to 17.

Okay, let's try with -1 then.

Quoted 4 lines

virt/kvm/irq_comm.c int kvm_request_irq_source_id(struct kvm *kvm) · line 81
Quoted 2 lines
virt/kvm/irq_comm.c +1 -0
+	ASSERT(irq_source_id != 0); /* KVM_USERSPACE_IRQ_SOURCE_ID reserved */
Why not replace 0 with the actual symbolic constant?

Because I was giving 0 two meanings :-)

Respin of the patches coming up.

Cheers,
Mark.

Mark McLoughlin

2008-12-01 13:59 UTC #9 lore raw

PATCH 1/5 KVM: remove the IRQ ACK notifier assertions

We will obviously never pass a NULL struct kvm_irq_ack_notifier* to this functions. They are always embedded in the assigned device structure, so the assertion add nothing.

The irqchip_in_kernel() assertion is very out of place - clearly this little abstraction needs to know nothing about the upper layer details.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

virt/kvm/irq_comm.c +0 -3

index 9fbbdea..973df99 100644
@@ -58,9 +58,6 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian)
 {
-	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
-	ASSERT(irqchip_in_kernel(kvm));
-	ASSERT(kian);
 	hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
 }

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-12-01 13:59 UTC #10 lore raw

PATCH 2/5 KVM: make kvm_unregister_irq_ack_notifier() safe

We never pass a NULL notifier pointer here, but we may well pass a notifier struct which hasn't previously been registered.

Fixes an oops seen when an assigned device is freed before and IRQ is assigned to it.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

virt/kvm/irq_comm.c +1 -3

index 973df99..db75045 100644
@@ -63,9 +63,7 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 
 void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
 {
-	if (!kian)
-		return;
-	hlist_del(&kian->link);
+	hlist_del_init(&kian->link);
 }
 
 /* The caller must hold kvm->lock mutex */

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-12-01 13:59 UTC #11 lore raw

PATCH 3/5 KVM: don't fee an unallocated irq source id

Set assigned_dev->irq_source_id to -1 so that we can avoid freeing a source ID which we never allocated.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/kvm_main.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

virt/kvm/kvm_main.c +5 -2

index 8dab7ce..63fd882 100644
@@ -210,7 +210,10 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 		pci_disable_msi(assigned_dev->dev);
 
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
-	kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
+
+	if (assigned_dev->irq_source_id != -1)
+		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
+	assigned_dev->irq_source_id = -1;
 
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take

@@ -466,7 +469,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	match->host_busnr = assigned_dev->busnr;
 	match->host_devfn = assigned_dev->devfn;
 	match->dev = dev;
-
+	match->irq_source_id = -1;
 	match->kvm = kvm;
 
 	list_add(&match->list, &kvm->arch.assigned_dev_head);

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-12-01 13:59 UTC #12 lore raw

PATCH 4/5 KVM: add KVM_USERSPACE_IRQ_SOURCE_ID assertions

Make sure kvm_request_irq_source_id() never returns KVM_USERSPACE_IRQ_SOURCE_ID.

Likewise, check that kvm_free_irq_source_id() never accepts KVM_USERSPACE_IRQ_SOURCE_ID.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/irq_comm.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

virt/kvm/irq_comm.c +10 -4

index db75045..aa5d1e5 100644
@@ -72,11 +72,15 @@ int kvm_request_irq_source_id(struct kvm *kvm)
 	unsigned long *bitmap = &kvm->arch.irq_sources_bitmap;
 	int irq_source_id = find_first_zero_bit(bitmap,
 				sizeof(kvm->arch.irq_sources_bitmap));
+
 	if (irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
 		printk(KERN_WARNING "kvm: exhaust allocatable IRQ sources!\n");
-		irq_source_id = -EFAULT;
-	} else
-		set_bit(irq_source_id, bitmap);
+		return -EFAULT;
+	}
+
+	ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
+	set_bit(irq_source_id, bitmap);
+
 	return irq_source_id;
 }

@@ -84,7 +88,9 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 {
 	int i;
 
-	if (irq_source_id <= 0 ||
+	ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
+
+	if (irq_source_id < 0 ||
 	    irq_source_id >= sizeof(kvm->arch.irq_sources_bitmap)) {
 		printk(KERN_ERR "kvm: IRQ source ID out of range!\n");
 		return;

Signature · 1 line

1.5.4.3

Mark McLoughlin

2008-12-01 13:59 UTC #13 lore raw

PATCH 5/5 KVM: split out kvm_free_assigned_irq()

Split out the logic corresponding to undoing assign_irq() and clean it up a bit.

Signed-off-by Mark McLoughlin <[email protected]>

---
 virt/kvm/kvm_main.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

virt/kvm/kvm_main.c +22 -7

index 63fd882..e41d39d 100644
@@ -200,14 +200,11 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 	enable_irq(dev->host_irq);
 }
 
-static void kvm_free_assigned_device(struct kvm *kvm,
-				     struct kvm_assigned_dev_kernel
-				     *assigned_dev)
+static void kvm_free_assigned_irq(struct kvm *kvm,
+				  struct kvm_assigned_dev_kernel *assigned_dev)
 {
-	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested_type)
-		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
-	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
-		pci_disable_msi(assigned_dev->dev);
+	if (!irqchip_in_kernel(kvm))
+		return;
 
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);

@@ -215,12 +212,30 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
 	assigned_dev->irq_source_id = -1;
 
+	if (!assigned_dev->irq_requested_type)
+		return;
+
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take
 		 * care of kvm_put_kvm.
 		 */
 		kvm_put_kvm(kvm);
 
+	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
+		pci_disable_msi(assigned_dev->dev);
+
+	assigned_dev->irq_requested_type = 0;
+}
+
+
+static void kvm_free_assigned_device(struct kvm *kvm,
+				     struct kvm_assigned_dev_kernel
+				     *assigned_dev)
+{
+	kvm_free_assigned_irq(kvm, assigned_dev);
+
 	pci_reset_function(assigned_dev->dev);
 
 	pci_release_regions(assigned_dev->dev);

Signature · 1 line

1.5.4.3

Avi Kivity

2008-12-02 12:41 UTC #14 lore raw

Re: [PATCH 4/5] KVM: add KVM_USERSPACE_IRQ_SOURCE_ID assertions

In reply to Mark McLoughlin

Quoted 6 lines · Mark McLoughlin

Make sure kvm_request_irq_source_id() never returns KVM_USERSPACE_IRQ_SOURCE_ID.
Likewise, check that kvm_free_irq_source_id() never accepts KVM_USERSPACE_IRQ_SOURCE_ID.

An alternative way to do this is to drop the distinction KVM_USERSPACE_IRQ_SOURCE_ID has, and simply allocate it via the normal irq source id allocation API (and store it in struct kvm). That's not worth the churn though.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

Applied

2008-12-02 12:39 UTC #15 lore raw

Re: [PATCH 1/5] KVM: remove the IRQ ACK notifier assertions

In reply to Mark McLoughlin

Quoted 8 lines · Mark McLoughlin

We will obviously never pass a NULL struct kvm_irq_ack_notifier* to this functions. They are always embedded in the assigned device structure, so the assertion add nothing.
The irqchip_in_kernel() assertion is very out of place - clearly this little abstraction needs to know nothing about the upper layer details.

Applied all, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function

Sheng Yang

author

2008-12-01 02:36 UTC #16 lore raw

In reply to Mark McLoughlin

Quoted 11 lines · On Friday 28 November 2008 18:25:51 Mark McLoughlin wrote:

Hi,
I just got an oops (with 2.6.28-rc6) when running "qemu-kvm -S -pcidevice ..." and immediately quitting rather than starting the guest.
The issue is that at this point ASSIGN_PCI_DEVICE has been called, but not ASSIGN_IRQ, so kvm_unregister_irq_ack_notifier() oops when we try and remove a notifier which hasn't already been added.
The fix is simple - use hlist_del_init() rather than hlist_del() - but I also came across this patch in Avi's tree ...

Yes, that's what I meant to fix. Thanks for point out the bug. It's indeed a buggy fix for (!kian).

Quoted 20 lines · Sheng Yang

...
virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned · line 62
Quoted 10 lines
virt/kvm/irq_comm.c +2 -0
index 55ad76e..9fbbdea 100644
@@ -58,12 +58,16 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned
 gsi) void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian)
 {
+	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
+	ASSERT(irqchip_in_kernel(kvm));
This is a seriously ugly assertion - there is no reason for the IRQ ACK notifier abstraction to know anything about when it is called, and it's easy to verify that kvm_register_irq_ack_notifier() is only called with the in-kernel irqchip ... it's only called in one place:

Quoted 7 lines

                if (irqchip_in_kernel(kvm)) {
                        /* Register ack nofitier */
                        match->ack_notifier.gsi = -1;
                        match->ack_notifier.irq_acked =
                                        kvm_assigned_dev_ack_irq;
                        kvm_register_irq_ack_notifier(kvm,
                                        &match->ack_notifier);

Should be two. Another one is PIT. Of course PIT should also be used with in- kernel-irqchip. My feeling here this one is not that unnecessary...

Anyway, I think your patches are OK for now.

Signature · 2 lines

regards
Yang, Sheng

Quoted 27 lines

virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi) · line 63
Quoted 2 lines
virt/kvm/irq_comm.c +1 -0
+	ASSERT(kian);
This is bogus; the ack notifier structure is embedded in assigned device structure, so we can never pass NULL here - it's not like it's a dynamically allocated structure.
virt/kvm/irq_comm.c void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi) · line 71
Quoted 10 lines
virt/kvm/irq_comm.c +3 -2
 	hlist_add_head(&kian->link, &kvm->arch.irq_ack_notifier_list);
 }
 
-void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
-				     struct kvm_irq_ack_notifier *kian)
+void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian)
 {
+	if (!kian)
+		return;
 	hlist_del(&kian->link);
This is where I think you were trying to fix the issue I saw ... but again, it's bogus. We will never pass a NULL ack notifier struct, but we may well pass one which hasn't been previously registered.
I'm going to follow up with a number of patches to clean some of this up.
Cheers,
Mark.