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

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

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/kvm/irq.c |    4 ++++
 arch/x86/kvm/x86.c |    4 +++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 8c1b9c5..24e2667 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -124,11 +124,15 @@ 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));
 	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)
 {
+	/* Must be called with in-kernel IRQ chip, otherwise it's nonsense */
+	ASSERT(irqchip_in_kernel(kvm));
 	hlist_del(&kian->link);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 675fcc1..c5763d7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -176,7 +176,9 @@ 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);
+	if (irqchip_in_kernel(kvm))
+		kvm_unregister_irq_ack_notifier(kvm,
+				&assigned_dev->ack_notifier);
 
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take
-- 
1.5.4.5

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

[Sheng Yang]

Also remove unnecessary parameter of unregister irq ack notifier.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 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(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3833c48..41955ed 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -313,8 +313,7 @@ void kvm_set_irq(struct kvm *kvm, 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);
 
 #ifdef CONFIG_DMAR
 int kvm_iommu_map_pages(struct kvm *kvm, gfn_t base_gfn,
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index d0169f5..54b251d 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -50,11 +50,15 @@ 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);
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cf0ab8e..d2ae1c9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -145,7 +145,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);
 
 	if (cancel_work_sync(&assigned_dev->interrupt_work))
 		/* We had pending work. That means we will have to take
-- 
1.5.4.5

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

[Avi Kivity]

Sheng Yang wrote:
> Also remove unnecessary parameter of unregister irq ack notifier.
>
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index d0169f5..54b251d 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -50,11 +50,15 @@ 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);
>  }
>   

We don't want a BUG() here is the user specifies -no-kvm-irqchip; is
there a check on the irq assignment ioctls before calling this?


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Sheng Yang]

On Thursday 09 October 2008 16:34:47 Avi Kivity wrote:
> Sheng Yang wrote:
> > Also remove unnecessary parameter of unregister irq ack notifier.
> >
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index d0169f5..54b251d 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -50,11 +50,15 @@ 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);
> >  }
>
> We don't want a BUG() here is the user specifies -no-kvm-irqchip; is
> there a check on the irq assignment ioctls before calling this?

Yes. kvm_register_irq_ack_notifier should be called within irqchip_in_kernel() 
(on the other side, only if we have irqchip_in_kernel(), ack_notifier is 
useful, so we shouldn't call it without it), And I can't see if this would be 
useful with userspace irqchip, so add a ASSERT here. 

--
regards
Yang, Sheng

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

[Avi Kivity]

Sheng Yang wrote:
> On Thursday 09 October 2008 16:34:47 Avi Kivity wrote:
>   
>> Sheng Yang wrote:
>>     
>>> Also remove unnecessary parameter of unregister irq ack notifier.
>>>
>>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>>> index d0169f5..54b251d 100644
>>> --- a/virt/kvm/irq_comm.c
>>> +++ b/virt/kvm/irq_comm.c
>>> @@ -50,11 +50,15 @@ 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);
>>>  }
>>>       
>> We don't want a BUG() here is the user specifies -no-kvm-irqchip; is
>> there a check on the irq assignment ioctls before calling this?
>>     
>
> Yes. kvm_register_irq_ack_notifier should be called within irqchip_in_kernel() 
> (on the other side, only if we have irqchip_in_kernel(), ack_notifier is 
> useful, so we shouldn't call it without it), And I can't see if this would be 
> useful with userspace irqchip, so add a ASSERT here. 
>   

Yes.

The code changed quite a lot here due to ia64 gaining VT-d support.  Can 
you regenerate the patch?

-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

Also remove unnecessary parameter of unregister irq ack notifier.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 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(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bb92be2..3a0fb77 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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);
 
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 55ad76e..9fbbdea 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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);
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..4f43abe 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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))
-- 
1.5.4.5

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

[Avi Kivity]

Sheng Yang wrote:
> Also remove unnecessary parameter of unregister irq ack notifier.
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function

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

[Mark McLoughlin]

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:
... 
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 55ad76e..9fbbdea 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -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);

> +	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.

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

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

[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.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 9fbbdea..973df99 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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);
 }
 
-- 
1.5.4.3

[PATCH 2/4] KVM: make kvm_unregister_irq_ack_notifier() safe

[Mark McLoughlin]

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 <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 973df99..db75045 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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 */
-- 
1.5.4.3

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

[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".

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index db75045..869f5e8 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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;
-- 
1.5.4.3

[PATCH 4/4] KVM: split out kvm_free_assigned_irq()

[Mark McLoughlin]

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

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/kvm_main.c |   31 ++++++++++++++++++++++++-------
 1 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 54d25e6..bcdf715 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.5.4.3

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

[Avi Kivity]

Mark McLoughlin wrote:
> 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.

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

Why not replace 0 with the actual symbolic constant?


-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

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

>
> On Mon, 2008-10-20 at 16:07 +0800, Sheng Yang wrote:
> ...
>
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 55ad76e..9fbbdea 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -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);

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.

-- 
regards
Yang, Sheng

>
> > +	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.
>
> >  	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.

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

[Mark McLoughlin]

On Sun, 2008-11-30 at 12:28 +0200, Avi Kivity wrote:
> Mark McLoughlin wrote:
> > 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 ...

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

Okay, let's try with -1 then.

> > +	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.

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

[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.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 9fbbdea..973df99 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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);
 }
 
-- 
1.5.4.3

[PATCH 2/5] KVM: make kvm_unregister_irq_ack_notifier() safe

[Mark McLoughlin]

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 <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 973df99..db75045 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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 */
-- 
1.5.4.3

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

[Mark McLoughlin]

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 <markmc@redhat.com>
---
 virt/kvm/kvm_main.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8dab7ce..63fd882 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.5.4.3

[PATCH 4/5] KVM: add KVM_USERSPACE_IRQ_SOURCE_ID assertions

[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.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/irq_comm.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index db75045..aa5d1e5 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -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;
-- 
1.5.4.3

[PATCH 5/5] KVM: split out kvm_free_assigned_irq()

[Mark McLoughlin]

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

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/kvm_main.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 63fd882..e41d39d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
-- 
1.5.4.3

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

[Avi Kivity]

Mark McLoughlin wrote:
> 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.

-- 
error compiling committee.c: too many arguments to function

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

[Avi Kivity]

Mark McLoughlin wrote:
> 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.

-- 
error compiling committee.c: too many arguments to function