[PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Sheng Yang]

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

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

Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Amit Shah]

* On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/kvm/x86.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> 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);

The unregister API should perform the check whether the said notifier exists 
so this shouldn't be necessary.

Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Sheng Yang]

On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
> * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  arch/x86/kvm/x86.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > 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);
>
> The unregister API should perform the check whether the said notifier
> exists so this shouldn't be necessary.

Yeah, that's more reasonable. But now I just see, 
kvm_register_irq_ack_notifier() go with irqchip_in_kernel() and unregister 
didn't. :)

--
regards
Yang, Sheng

Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Sheng Yang]

On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
> On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
> > * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  arch/x86/kvm/x86.c |    4 +++-
> > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > >
> > > 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);
> >
> > The unregister API should perform the check whether the said notifier
> > exists so this shouldn't be necessary.
>
> Yeah, that's more reasonable. But now I just see,
> kvm_register_irq_ack_notifier() go with irqchip_in_kernel() and unregister
> didn't. :)
>
Um... After consider a little more, I think keep it unwrapped by 
irqchip_in_kernel() may be a little more reasonable. The reason is we just 
register kvm_register_irq_ack_notifier() when we have in kernel irqchip. If 
we don't have in kernel irqchip, we shouldn't call them and try to perform 
this action. Call the function without in-kernel irqchip is wrong, ensure the 
exist of in-kernel irqchip is the caller's responsibility. What we can do is 
add a BUG_ON() or ASSERT() to the function, rather let the function tell if 
itself need in-kernel irqchip. 

I will update the patch to say we need in kernel irqchip when call the 
function explicitly. 

Thanks!
--
regards
Yang, Sheng

Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Amit Shah]

* On Wednesday 08 Oct 2008 14:34:05 Sheng Yang wrote:
> On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
> > On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
> > > * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
> > > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > > ---
> > > >  arch/x86/kvm/x86.c |    4 +++-
> > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > >
> > > > 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);
> > >
> > > The unregister API should perform the check whether the said notifier
> > > exists so this shouldn't be necessary.
> >
> > Yeah, that's more reasonable. But now I just see,
> > kvm_register_irq_ack_notifier() go with irqchip_in_kernel() and
> > unregister didn't. :)

Yes, because if we don't use the irqchip in the kernel, we don't need it at 
all.

However, there's no need to special-case the unregister path as you note 
below.

> Um... After consider a little more, I think keep it unwrapped by
> irqchip_in_kernel() may be a little more reasonable. The reason is we just
> register kvm_register_irq_ack_notifier() when we have in kernel irqchip. If
> we don't have in kernel irqchip, we shouldn't call them and try to perform
> this action. Call the function without in-kernel irqchip is wrong, ensure
> the exist of in-kernel irqchip is the caller's responsibility. What we can
> do is add a BUG_ON() or ASSERT() to the function, rather let the function
> tell if itself need in-kernel irqchip.

We ensure our notifier is registered only in the in-kernel irqchip case. 
Calling the unregister function in all the cases, though, should be fine I 
think. BUG_ON or ASSERT are conditions used to trap hardware bugs or 
unexpected paths in software. This doesn't qualify for that.

To make the code symmetrical, it might make sense to have the unregister path 
in the kernel-irqchip case, but I really don't think it's necessary and 
doesn't hurt readability as well. Also, the lesser the conditionals we have 
the better.

Amit

Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip

[Sheng Yang]

On Wednesday 08 October 2008 17:20:20 Amit Shah wrote:
> * On Wednesday 08 Oct 2008 14:34:05 Sheng Yang wrote:
> > On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
> > > On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
> > > > * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
> > > > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > > > ---
> > > > >  arch/x86/kvm/x86.c |    4 +++-
> > > > >  1 files changed, 3 insertions(+), 1 deletions(-)
> > > > >
> > > > > 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);
> > > >
> > > > The unregister API should perform the check whether the said notifier
> > > > exists so this shouldn't be necessary.
> > >
> > > Yeah, that's more reasonable. But now I just see,
> > > kvm_register_irq_ack_notifier() go with irqchip_in_kernel() and
> > > unregister didn't. :)
>
> Yes, because if we don't use the irqchip in the kernel, we don't need it at
> all.
>
> However, there's no need to special-case the unregister path as you note
> below.
>
> > Um... After consider a little more, I think keep it unwrapped by
> > irqchip_in_kernel() may be a little more reasonable. The reason is we
> > just register kvm_register_irq_ack_notifier() when we have in kernel
> > irqchip. If we don't have in kernel irqchip, we shouldn't call them and
> > try to perform this action. Call the function without in-kernel irqchip
> > is wrong, ensure the exist of in-kernel irqchip is the caller's
> > responsibility. What we can do is add a BUG_ON() or ASSERT() to the
> > function, rather let the function tell if itself need in-kernel irqchip.
>
> We ensure our notifier is registered only in the in-kernel irqchip case.
> Calling the unregister function in all the cases, though, should be fine I
> think. BUG_ON or ASSERT are conditions used to trap hardware bugs or
> unexpected paths in software. This doesn't qualify for that.

Yeah, so register should got some ASSERT. 
>
> To make the code symmetrical, it might make sense to have the unregister
> path in the kernel-irqchip case, but I really don't think it's necessary
> and doesn't hurt readability as well. Also, the lesser the conditionals we
> have the better.

OK. Seems we need a NULL judgement of unregister to keep it safe.

--
regards
Yang, Sheng
>
> Amit

[PATCH] 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>
---
 arch/x86/kvm/irq.c |    8 ++++++--
 arch/x86/kvm/irq.h |    3 +--
 arch/x86/kvm/x86.c |    2 +-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 8c1b9c5..714bcc1 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));
+	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/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 9f157c9..0c47117 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -89,8 +89,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);
 
 void kvm_timer_intr_post(struct kvm_vcpu *vcpu, int vec);
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 675fcc1..8a9a029 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -176,7 +176,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

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