KVM: Unregister IRQ ACK notifier with in-kernel irqchip
PATCH 1/1 KVM: Unregister IRQ ACK notifier with in-kernel irqchip
--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +3 -1
@@ -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
Signature · 1 line
1.5.4.5
Quoted 10 lines · * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
Signed-off-by Sheng Yang <[email protected]>--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +0
-0
arch/x86/kvm/x86.c static void kvm_free_assigned_device(struct kvm *kvm, · line 181
Quoted 7 lines
arch/x86/kvm/x86.c +3
-1@@ -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.
Quoted 14 lines · On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
Quoted 13 lines · * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
Signed-off-by Sheng Yang <[email protected]>--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +0
-0@@ -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);
Quoted 7 lines
arch/x86/kvm/x86.c static void kvm_free_assigned_device(struct kvm *kvm, · line 181
Quoted 4 lines
arch/x86/kvm/x86.c +3
-1-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
Quoted 23 lines · On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
Quoted 14 lines · On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
Quoted 13 lines · * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
Signed-off-by Sheng Yang <[email protected]>--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +0
-0@@ -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);Quoted 7 lines
arch/x86/kvm/x86.c static void kvm_free_assigned_device(struct kvm *kvm, · line 181
Quoted 4 lines
arch/x86/kvm/x86.c +3
-1-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.
Quoted 4 lines
Yeah, that's more reasonable. But now I just see,
kvm_register_irq_ack_notifier()go withirqchip_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
Quoted 24 lines · * On Wednesday 08 Oct 2008 14:34:05 Sheng Yang wrote:
Quoted 23 lines · On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
Quoted 21 lines · On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
Quoted 10 lines · * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
Signed-off-by Sheng Yang <[email protected]>--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +0
-0arch/x86/kvm/x86.c static void kvm_free_assigned_device(struct kvm · line 181
Quoted 7 lines
arch/x86/kvm/x86.c +3
-1@@ -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.
Quoted 3 lines
Quoted 3 lines
Yeah, that's more reasonable. But now I just see,
kvm_register_irq_ack_notifier()go withirqchip_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.
Quoted 8 lines
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 registerkvm_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 aBUG_ON()orASSERT()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
Quoted 44 lines · On Wednesday 08 October 2008 17:20:20 Amit Shah wrote:
Quoted 24 lines · * On Wednesday 08 Oct 2008 14:34:05 Sheng Yang wrote:
Quoted 23 lines · On Wednesday 08 October 2008 16:54:18 Sheng Yang wrote:
Quoted 21 lines · On Wednesday 08 October 2008 15:08:52 Amit Shah wrote:
Quoted 10 lines · * On Wednesday 08 Oct 2008 12:09:20 Sheng Yang wrote:
Signed-off-by Sheng Yang <[email protected]>--- arch/x86/kvm/x86.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)arch/x86/kvm/x86.c +0
-0arch/x86/kvm/x86.c static void kvm_free_assigned_device(struct kvm · line 181
Quoted 7 lines
arch/x86/kvm/x86.c +2
-1@@ -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.
Quoted 3 lines
Quoted 3 lines
Yeah, that's more reasonable. But now I just see,
kvm_register_irq_ack_notifier()go withirqchip_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.
Quoted 8 lines
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 registerkvm_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 aBUG_ON()orASSERT()to the function, rather let the function tell if itself need in-kernel irqchip.
Quoted 4 lines
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_ONor 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.
Quoted 5 lines
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
Quoted 2 lines
Amit
PATCH 1/1 KVM: IRQ ACK notifier should be used with in-kernel irqchip
Also remove unnecessary parameter of unregister irq ack notifier.
--- 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(-)
arch/x86/kvm/irq.c +6 -2
@@ -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); }
arch/x86/kvm/irq.h +1 -2
@@ -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);
arch/x86/kvm/x86.c +1 -1
@@ -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
Signature · 1 line
1.5.4.5
PATCH 1/1 KVM: IRQ ACK notifier should be used with in-kernel irqchip
In reply to Sheng Yang--- arch/x86/kvm/irq.c | 4 ++++ arch/x86/kvm/x86.c | 4 +++- 2 files changed, 7 insertions(+), 1 deletions(-)
arch/x86/kvm/irq.c +4 -0
@@ -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); }
arch/x86/kvm/x86.c +3 -1
@@ -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
Signature · 1 line
1.5.4.5