[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

[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

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