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

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.