* [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip @ 2008-10-08 6:39 Sheng Yang 2008-10-08 7:08 ` Amit Shah 0 siblings, 1 reply; 8+ messages in thread From: Sheng Yang @ 2008-10-08 6:39 UTC (permalink / raw) To: Avi Kivity; +Cc: kvm, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip 2008-10-08 6:39 [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip Sheng Yang @ 2008-10-08 7:08 ` Amit Shah 2008-10-08 8:54 ` Sheng Yang 0 siblings, 1 reply; 8+ messages in thread From: Amit Shah @ 2008-10-08 7:08 UTC (permalink / raw) To: Sheng Yang; +Cc: Avi Kivity, kvm * 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip 2008-10-08 7:08 ` Amit Shah @ 2008-10-08 8:54 ` Sheng Yang 2008-10-08 9:04 ` Sheng Yang 0 siblings, 1 reply; 8+ messages in thread From: Sheng Yang @ 2008-10-08 8:54 UTC (permalink / raw) To: Amit Shah; +Cc: Avi Kivity, kvm 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip 2008-10-08 8:54 ` Sheng Yang @ 2008-10-08 9:04 ` Sheng Yang 2008-10-08 9:20 ` Amit Shah 2008-10-08 9:28 ` [PATCH 1/1] " Sheng Yang 0 siblings, 2 replies; 8+ messages in thread From: Sheng Yang @ 2008-10-08 9:04 UTC (permalink / raw) To: kvm; +Cc: Amit Shah, Avi Kivity 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip 2008-10-08 9:04 ` Sheng Yang @ 2008-10-08 9:20 ` Amit Shah 2008-10-08 9:32 ` Sheng Yang 2008-10-08 9:28 ` [PATCH 1/1] " Sheng Yang 1 sibling, 1 reply; 8+ messages in thread From: Amit Shah @ 2008-10-08 9:20 UTC (permalink / raw) To: Sheng Yang; +Cc: kvm, Avi Kivity * 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip 2008-10-08 9:20 ` Amit Shah @ 2008-10-08 9:32 ` Sheng Yang 2008-10-08 9:46 ` [PATCH] KVM: IRQ ACK notifier should be used " Sheng Yang 0 siblings, 1 reply; 8+ messages in thread From: Sheng Yang @ 2008-10-08 9:32 UTC (permalink / raw) To: Amit Shah; +Cc: kvm, Avi Kivity 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] KVM: IRQ ACK notifier should be used with in-kernel irqchip 2008-10-08 9:32 ` Sheng Yang @ 2008-10-08 9:46 ` Sheng Yang 0 siblings, 0 replies; 8+ messages in thread From: Sheng Yang @ 2008-10-08 9:46 UTC (permalink / raw) To: Avi Kivity; +Cc: Amit Shah, kvm, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 1/1] KVM: IRQ ACK notifier should be used with in-kernel irqchip 2008-10-08 9:04 ` Sheng Yang 2008-10-08 9:20 ` Amit Shah @ 2008-10-08 9:28 ` Sheng Yang 1 sibling, 0 replies; 8+ messages in thread From: Sheng Yang @ 2008-10-08 9:28 UTC (permalink / raw) To: Avi Kivity; +Cc: Amit Shah, kvm, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2008-10-08 9:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-10-08 6:39 [PATCH] KVM: Unregister IRQ ACK notifier with in-kernel irqchip Sheng Yang 2008-10-08 7:08 ` Amit Shah 2008-10-08 8:54 ` Sheng Yang 2008-10-08 9:04 ` Sheng Yang 2008-10-08 9:20 ` Amit Shah 2008-10-08 9:32 ` Sheng Yang 2008-10-08 9:46 ` [PATCH] KVM: IRQ ACK notifier should be used " Sheng Yang 2008-10-08 9:28 ` [PATCH 1/1] " Sheng Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).