* [PATCH] kvm: fix race with level interrupts
@ 2012-07-18 21:52 Michael S. Tsirkin
2012-07-18 22:16 ` Alex Williamson
2012-07-18 22:26 ` Alex Williamson
0 siblings, 2 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-18 21:52 UTC (permalink / raw)
To: Avi Kivity, Marcelo Tosatti, Alex Williamson, gleb, kvm, linux-kernel
When more than 1 source id is in use for the same GSI, we have the
following race related to handling irq_states:
CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
calls kvm_ioapic_set_irq(0).
Now ioapic thinks the level is 0 but irq_state is not 0.
Note that above is valid behaviour if CPU0 and CPU1 are using different
source ids.
Fix by performing all irq_states bitmap handling under pic/ioapic lock.
This also removes the need for atomics with irq_states handling.
Reported-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This survives stress testing for me (though I only tried virtio, not
device assignment).
Avi, Marcelo, though we have not observed this in the field,
it's a bugfix so probably 3.5 material?
I assume yes so the patch is against 3.5-rc7.
Also stable? It's a very old bug.
arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
arch/x86/kvm/i8259.c | 14 ++++++++++++--
virt/kvm/ioapic.c | 13 ++++++++++++-
virt/kvm/ioapic.h | 4 +++-
virt/kvm/irq_comm.c | 31 ++++---------------------------
5 files changed, 45 insertions(+), 32 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index db7c1f2..fe6e9f1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
-int kvm_pic_set_irq(void *opaque, int irq, int level);
+static inline int kvm_irq_line_state(unsigned long *irq_state,
+ int irq_source_id, int level)
+{
+ /* Logical OR for level trig interrupt */
+ if (level)
+ __set_bit(irq_source_id, irq_state);
+ else
+ __clear_bit(irq_source_id, irq_state);
+
+ return !!(*irq_state);
+}
+
+int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
+void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
void kvm_inject_nmi(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 81cf4fa..95b504b 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
pic_unlock(s);
}
-int kvm_pic_set_irq(void *opaque, int irq, int level)
+int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
{
- struct kvm_pic *s = opaque;
int ret = -1;
+ int level;
pic_lock(s);
if (irq >= 0 && irq < PIC_NUM_PINS) {
+ level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
pic_update_irq(s);
trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
@@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
return ret;
}
+void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
+{
+ int i;
+ pic_lock(s);
+ for (i = 0; i < PIC_NUM_PINS; i++)
+ __clear_bit(src_id, &s->irq_states[i]);
+ pic_unlock(s);
+}
+
/*
* acknowledge interrupt 'irq'
*/
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 26fd54d..268b332 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
}
-int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
{
u32 old_irr;
u32 mask = 1 << irq;
union kvm_ioapic_redirect_entry entry;
int ret = 1;
+ int level;
spin_lock(&ioapic->lock);
old_irr = ioapic->irr;
if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
entry = ioapic->redirtbl[irq];
+ level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
level ^= entry.fields.polarity;
if (!level)
ioapic->irr &= ~mask;
@@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
return ret;
}
+void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
+{
+ int i;
+ spin_lock(&ioapic->lock);
+ for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
+ __clear_bit(src_id, &ioapic->irq_states[i]);
+ spin_unlock(&ioapic->lock);
+}
+
static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
int trigger_mode)
{
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 32872a0..a30abfe 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
int kvm_ioapic_init(struct kvm *kvm);
void kvm_ioapic_destroy(struct kvm *kvm);
-int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
+ int level);
+void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
struct kvm_lapic_irq *irq);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 5afb431..83402d7 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -33,26 +33,12 @@
#include "ioapic.h"
-static inline int kvm_irq_line_state(unsigned long *irq_state,
- int irq_source_id, int level)
-{
- /* Logical OR for level trig interrupt */
- if (level)
- set_bit(irq_source_id, irq_state);
- else
- clear_bit(irq_source_id, irq_state);
-
- return !!(*irq_state);
-}
-
static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
struct kvm *kvm, int irq_source_id, int level)
{
#ifdef CONFIG_X86
struct kvm_pic *pic = pic_irqchip(kvm);
- level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
- irq_source_id, level);
- return kvm_pic_set_irq(pic, e->irqchip.pin, level);
+ return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
#else
return -1;
#endif
@@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
struct kvm *kvm, int irq_source_id, int level)
{
struct kvm_ioapic *ioapic = kvm->arch.vioapic;
- level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
- irq_source_id, level);
-
- return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
+ return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
}
inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
@@ -249,8 +232,6 @@ unlock:
void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
{
- int i;
-
ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
mutex_lock(&kvm->irq_lock);
@@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
if (!irqchip_in_kernel(kvm))
goto unlock;
- for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
- clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
- if (i >= 16)
- continue;
+ kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
#ifdef CONFIG_X86
- clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
+ kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
#endif
- }
unlock:
mutex_unlock(&kvm->irq_lock);
}
--
MST
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 21:52 [PATCH] kvm: fix race with level interrupts Michael S. Tsirkin
@ 2012-07-18 22:16 ` Alex Williamson
2012-07-18 22:44 ` Michael S. Tsirkin
2012-07-18 22:26 ` Alex Williamson
1 sibling, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2012-07-18 22:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> When more than 1 source id is in use for the same GSI, we have the
> following race related to handling irq_states:
>
> CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> calls kvm_ioapic_set_irq(0).
>
> Now ioapic thinks the level is 0 but irq_state is not 0.
>
> Note that above is valid behaviour if CPU0 and CPU1 are using different
> source ids.
>
> Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> This also removes the need for atomics with irq_states handling.
>
> Reported-by: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This survives stress testing for me (though I only tried virtio, not
> device assignment).
> Avi, Marcelo, though we have not observed this in the field,
> it's a bugfix so probably 3.5 material?
> I assume yes so the patch is against 3.5-rc7.
> Also stable? It's a very old bug.
>
>
> arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> arch/x86/kvm/i8259.c | 14 ++++++++++++--
> virt/kvm/ioapic.c | 13 ++++++++++++-
> virt/kvm/ioapic.h | 4 +++-
> virt/kvm/irq_comm.c | 31 ++++---------------------------
> 5 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index db7c1f2..fe6e9f1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
>
> -int kvm_pic_set_irq(void *opaque, int irq, int level);
> +static inline int kvm_irq_line_state(unsigned long *irq_state,
> + int irq_source_id, int level)
> +{
> + /* Logical OR for level trig interrupt */
> + if (level)
> + __set_bit(irq_source_id, irq_state);
> + else
> + __clear_bit(irq_source_id, irq_state);
> +
> + return !!(*irq_state);
> +}
> +
> +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
>
> void kvm_inject_nmi(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 81cf4fa..95b504b 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> pic_unlock(s);
> }
>
> -int kvm_pic_set_irq(void *opaque, int irq, int level)
> +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
Please use "irq_source_id" and keep "level". I hate reviewing code
where I have to differentiate 'l' vs '1'. Same below, mixing src_id and
irq_source_id in various places.
> {
> - struct kvm_pic *s = opaque;
> int ret = -1;
> + int level;
And you'd avoid this variable
>
> pic_lock(s);
> if (irq >= 0 && irq < PIC_NUM_PINS) {
> + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> pic_update_irq(s);
> trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> return ret;
> }
>
> +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> +{
> + int i;
> + pic_lock(s);
> + for (i = 0; i < PIC_NUM_PINS; i++)
> + __clear_bit(src_id, &s->irq_states[i]);
> + pic_unlock(s);
> +}
> +
> /*
> * acknowledge interrupt 'irq'
> */
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 26fd54d..268b332 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> }
>
> -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> {
> u32 old_irr;
> u32 mask = 1 << irq;
> union kvm_ioapic_redirect_entry entry;
> int ret = 1;
> + int level;
>
> spin_lock(&ioapic->lock);
> old_irr = ioapic->irr;
> if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> entry = ioapic->redirtbl[irq];
> + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> level ^= entry.fields.polarity;
> if (!level)
> ioapic->irr &= ~mask;
> @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> return ret;
> }
>
> +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> +{
> + int i;
> + spin_lock(&ioapic->lock);
> + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> + __clear_bit(src_id, &ioapic->irq_states[i]);
> + spin_unlock(&ioapic->lock);
> +}
> +
> static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> int trigger_mode)
> {
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 32872a0..a30abfe 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> int kvm_ioapic_init(struct kvm *kvm);
> void kvm_ioapic_destroy(struct kvm *kvm);
> -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> + int level);
> +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> struct kvm_lapic_irq *irq);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 5afb431..83402d7 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -33,26 +33,12 @@
>
> #include "ioapic.h"
>
> -static inline int kvm_irq_line_state(unsigned long *irq_state,
> - int irq_source_id, int level)
> -{
> - /* Logical OR for level trig interrupt */
> - if (level)
> - set_bit(irq_source_id, irq_state);
> - else
> - clear_bit(irq_source_id, irq_state);
> -
> - return !!(*irq_state);
> -}
> -
> static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> struct kvm *kvm, int irq_source_id, int level)
> {
> #ifdef CONFIG_X86
> struct kvm_pic *pic = pic_irqchip(kvm);
> - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> - irq_source_id, level);
> - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> #else
> return -1;
> #endif
> @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> struct kvm *kvm, int irq_source_id, int level)
> {
> struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> - irq_source_id, level);
> -
> - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> }
>
> inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> @@ -249,8 +232,6 @@ unlock:
>
> void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> {
> - int i;
> -
> ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
>
> mutex_lock(&kvm->irq_lock);
> @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> if (!irqchip_in_kernel(kvm))
> goto unlock;
>
> - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> - if (i >= 16)
> - continue;
> + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> #ifdef CONFIG_X86
> - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> #endif
> - }
> unlock:
> mutex_unlock(&kvm->irq_lock);
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 21:52 [PATCH] kvm: fix race with level interrupts Michael S. Tsirkin
2012-07-18 22:16 ` Alex Williamson
@ 2012-07-18 22:26 ` Alex Williamson
2012-07-18 22:49 ` Michael S. Tsirkin
1 sibling, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2012-07-18 22:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> When more than 1 source id is in use for the same GSI, we have the
> following race related to handling irq_states:
>
> CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> calls kvm_ioapic_set_irq(0).
>
> Now ioapic thinks the level is 0 but irq_state is not 0.
>
> Note that above is valid behaviour if CPU0 and CPU1 are using different
> source ids.
>
> Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> This also removes the need for atomics with irq_states handling.
>
> Reported-by: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This survives stress testing for me (though I only tried virtio, not
> device assignment).
> Avi, Marcelo, though we have not observed this in the field,
> it's a bugfix so probably 3.5 material?
> I assume yes so the patch is against 3.5-rc7.
> Also stable? It's a very old bug.
>
>
> arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> arch/x86/kvm/i8259.c | 14 ++++++++++++--
> virt/kvm/ioapic.c | 13 ++++++++++++-
> virt/kvm/ioapic.h | 4 +++-
> virt/kvm/irq_comm.c | 31 ++++---------------------------
> 5 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index db7c1f2..fe6e9f1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
>
> -int kvm_pic_set_irq(void *opaque, int irq, int level);
> +static inline int kvm_irq_line_state(unsigned long *irq_state,
> + int irq_source_id, int level)
This should probably be __kvm_irq_line_state given the calling
restrictions.
> +{
> + /* Logical OR for level trig interrupt */
> + if (level)
> + __set_bit(irq_source_id, irq_state);
> + else
> + __clear_bit(irq_source_id, irq_state);
> +
> + return !!(*irq_state);
> +}
> +
> +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
>
> void kvm_inject_nmi(struct kvm_vcpu *vcpu);
>
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 81cf4fa..95b504b 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> pic_unlock(s);
> }
>
> -int kvm_pic_set_irq(void *opaque, int irq, int level)
> +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> {
> - struct kvm_pic *s = opaque;
> int ret = -1;
> + int level;
>
> pic_lock(s);
> if (irq >= 0 && irq < PIC_NUM_PINS) {
> + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> pic_update_irq(s);
> trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> return ret;
> }
>
> +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> +{
> + int i;
> + pic_lock(s);
> + for (i = 0; i < PIC_NUM_PINS; i++)
> + __clear_bit(src_id, &s->irq_states[i]);
> + pic_unlock(s);
> +}
> +
> /*
> * acknowledge interrupt 'irq'
> */
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 26fd54d..268b332 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> }
>
> -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> {
> u32 old_irr;
> u32 mask = 1 << irq;
> union kvm_ioapic_redirect_entry entry;
> int ret = 1;
> + int level;
>
> spin_lock(&ioapic->lock);
> old_irr = ioapic->irr;
> if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> entry = ioapic->redirtbl[irq];
> + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> level ^= entry.fields.polarity;
> if (!level)
> ioapic->irr &= ~mask;
> @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> return ret;
> }
>
> +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> +{
> + int i;
> + spin_lock(&ioapic->lock);
> + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> + __clear_bit(src_id, &ioapic->irq_states[i]);
> + spin_unlock(&ioapic->lock);
> +}
> +
> static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> int trigger_mode)
> {
> diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> index 32872a0..a30abfe 100644
> --- a/virt/kvm/ioapic.h
> +++ b/virt/kvm/ioapic.h
> @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> int kvm_ioapic_init(struct kvm *kvm);
> void kvm_ioapic_destroy(struct kvm *kvm);
> -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> + int level);
> +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> struct kvm_lapic_irq *irq);
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 5afb431..83402d7 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -33,26 +33,12 @@
>
> #include "ioapic.h"
>
> -static inline int kvm_irq_line_state(unsigned long *irq_state,
> - int irq_source_id, int level)
> -{
> - /* Logical OR for level trig interrupt */
> - if (level)
> - set_bit(irq_source_id, irq_state);
> - else
> - clear_bit(irq_source_id, irq_state);
> -
> - return !!(*irq_state);
> -}
> -
> static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> struct kvm *kvm, int irq_source_id, int level)
> {
> #ifdef CONFIG_X86
> struct kvm_pic *pic = pic_irqchip(kvm);
> - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> - irq_source_id, level);
> - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> #else
> return -1;
> #endif
> @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> struct kvm *kvm, int irq_source_id, int level)
> {
> struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> - irq_source_id, level);
> -
> - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> }
>
> inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> @@ -249,8 +232,6 @@ unlock:
>
> void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> {
> - int i;
> -
> ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
>
> mutex_lock(&kvm->irq_lock);
> @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> if (!irqchip_in_kernel(kvm))
> goto unlock;
>
> - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> - if (i >= 16)
> - continue;
> + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> #ifdef CONFIG_X86
> - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> #endif
> - }
> unlock:
> mutex_unlock(&kvm->irq_lock);
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 22:16 ` Alex Williamson
@ 2012-07-18 22:44 ` Michael S. Tsirkin
2012-07-18 23:22 ` Alex Williamson
2012-07-19 7:51 ` Gleb Natapov
0 siblings, 2 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-18 22:44 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Wed, Jul 18, 2012 at 04:16:14PM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > When more than 1 source id is in use for the same GSI, we have the
> > following race related to handling irq_states:
> >
> > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > calls kvm_ioapic_set_irq(0).
> >
> > Now ioapic thinks the level is 0 but irq_state is not 0.
> >
> > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > source ids.
> >
> > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > This also removes the need for atomics with irq_states handling.
> >
> > Reported-by: Gleb Natapov <gleb@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > This survives stress testing for me (though I only tried virtio, not
> > device assignment).
> > Avi, Marcelo, though we have not observed this in the field,
> > it's a bugfix so probably 3.5 material?
> > I assume yes so the patch is against 3.5-rc7.
> > Also stable? It's a very old bug.
> >
> >
> > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > virt/kvm/ioapic.c | 13 ++++++++++++-
> > virt/kvm/ioapic.h | 4 +++-
> > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > 5 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index db7c1f2..fe6e9f1 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> >
> > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > + int irq_source_id, int level)
> > +{
> > + /* Logical OR for level trig interrupt */
> > + if (level)
> > + __set_bit(irq_source_id, irq_state);
> > + else
> > + __clear_bit(irq_source_id, irq_state);
> > +
> > + return !!(*irq_state);
> > +}
> > +
> > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> >
> > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> >
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 81cf4fa..95b504b 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > pic_unlock(s);
> > }
> >
> > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
>
> Please use "irq_source_id" and keep "level". I hate reviewing code
> where I have to differentiate 'l' vs '1'.
l is an illegal variable name? Switch to a different font.
> Same below, mixing src_id and
> irq_source_id in various places.
irq_source_id is too long, coding style says variable names
should be short:
LOCAL variable names should be short, and to the point.
> > {
> > - struct kvm_pic *s = opaque;
> > int ret = -1;
> > + int level;
>
> And you'd avoid this variable
by giving a different meaning to the same variable in
different parts of a function? Now *that* is a bad idea.
l is level for a specific source, level is the resulting irq value.
> >
> > pic_lock(s);
> > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > pic_update_irq(s);
> > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > return ret;
> > }
> >
> > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > +{
> > + int i;
> > + pic_lock(s);
> > + for (i = 0; i < PIC_NUM_PINS; i++)
> > + __clear_bit(src_id, &s->irq_states[i]);
> > + pic_unlock(s);
> > +}
> > +
> > /*
> > * acknowledge interrupt 'irq'
> > */
> > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > index 26fd54d..268b332 100644
> > --- a/virt/kvm/ioapic.c
> > +++ b/virt/kvm/ioapic.c
> > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > }
> >
> > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > {
> > u32 old_irr;
> > u32 mask = 1 << irq;
> > union kvm_ioapic_redirect_entry entry;
> > int ret = 1;
> > + int level;
> >
> > spin_lock(&ioapic->lock);
> > old_irr = ioapic->irr;
> > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > entry = ioapic->redirtbl[irq];
> > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > level ^= entry.fields.polarity;
> > if (!level)
> > ioapic->irr &= ~mask;
> > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > return ret;
> > }
> >
> > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > +{
> > + int i;
> > + spin_lock(&ioapic->lock);
> > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > + spin_unlock(&ioapic->lock);
> > +}
> > +
> > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > int trigger_mode)
> > {
> > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > index 32872a0..a30abfe 100644
> > --- a/virt/kvm/ioapic.h
> > +++ b/virt/kvm/ioapic.h
> > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > int kvm_ioapic_init(struct kvm *kvm);
> > void kvm_ioapic_destroy(struct kvm *kvm);
> > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > + int level);
> > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > struct kvm_lapic_irq *irq);
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 5afb431..83402d7 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -33,26 +33,12 @@
> >
> > #include "ioapic.h"
> >
> > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > - int irq_source_id, int level)
> > -{
> > - /* Logical OR for level trig interrupt */
> > - if (level)
> > - set_bit(irq_source_id, irq_state);
> > - else
> > - clear_bit(irq_source_id, irq_state);
> > -
> > - return !!(*irq_state);
> > -}
> > -
> > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > struct kvm *kvm, int irq_source_id, int level)
> > {
> > #ifdef CONFIG_X86
> > struct kvm_pic *pic = pic_irqchip(kvm);
> > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > - irq_source_id, level);
> > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > #else
> > return -1;
> > #endif
> > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > struct kvm *kvm, int irq_source_id, int level)
> > {
> > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > - irq_source_id, level);
> > -
> > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > }
> >
> > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > @@ -249,8 +232,6 @@ unlock:
> >
> > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > {
> > - int i;
> > -
> > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> >
> > mutex_lock(&kvm->irq_lock);
> > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > if (!irqchip_in_kernel(kvm))
> > goto unlock;
> >
> > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > - if (i >= 16)
> > - continue;
> > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > #ifdef CONFIG_X86
> > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > #endif
> > - }
> > unlock:
> > mutex_unlock(&kvm->irq_lock);
> > }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 22:26 ` Alex Williamson
@ 2012-07-18 22:49 ` Michael S. Tsirkin
2012-07-18 23:20 ` Alex Williamson
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-18 22:49 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > When more than 1 source id is in use for the same GSI, we have the
> > following race related to handling irq_states:
> >
> > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > calls kvm_ioapic_set_irq(0).
> >
> > Now ioapic thinks the level is 0 but irq_state is not 0.
> >
> > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > source ids.
> >
> > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > This also removes the need for atomics with irq_states handling.
> >
> > Reported-by: Gleb Natapov <gleb@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > This survives stress testing for me (though I only tried virtio, not
> > device assignment).
> > Avi, Marcelo, though we have not observed this in the field,
> > it's a bugfix so probably 3.5 material?
> > I assume yes so the patch is against 3.5-rc7.
> > Also stable? It's a very old bug.
> >
> >
> > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > virt/kvm/ioapic.c | 13 ++++++++++++-
> > virt/kvm/ioapic.h | 4 +++-
> > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > 5 files changed, 45 insertions(+), 32 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index db7c1f2..fe6e9f1 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> >
> > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > + int irq_source_id, int level)
>
> This should probably be __kvm_irq_line_state given the calling
> restrictions.
It's such a trivial helper, do we need to split hairs about this?
Look it's not a good time for minor coding style nits.
3.5 is imminent, it's about 1am here and I really don't have time to retest
today, so we'll release another kernel with a bug.
Could you focus on reviewing the functionality and correctness, and
leave ideas for better variable naming aside until 3.6?
> > +{
> > + /* Logical OR for level trig interrupt */
> > + if (level)
> > + __set_bit(irq_source_id, irq_state);
> > + else
> > + __clear_bit(irq_source_id, irq_state);
> > +
> > + return !!(*irq_state);
> > +}
> > +
> > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> >
> > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> >
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 81cf4fa..95b504b 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > pic_unlock(s);
> > }
> >
> > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> > {
> > - struct kvm_pic *s = opaque;
> > int ret = -1;
> > + int level;
> >
> > pic_lock(s);
> > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > pic_update_irq(s);
> > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > return ret;
> > }
> >
> > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > +{
> > + int i;
> > + pic_lock(s);
> > + for (i = 0; i < PIC_NUM_PINS; i++)
> > + __clear_bit(src_id, &s->irq_states[i]);
> > + pic_unlock(s);
> > +}
> > +
> > /*
> > * acknowledge interrupt 'irq'
> > */
> > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > index 26fd54d..268b332 100644
> > --- a/virt/kvm/ioapic.c
> > +++ b/virt/kvm/ioapic.c
> > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > }
> >
> > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > {
> > u32 old_irr;
> > u32 mask = 1 << irq;
> > union kvm_ioapic_redirect_entry entry;
> > int ret = 1;
> > + int level;
> >
> > spin_lock(&ioapic->lock);
> > old_irr = ioapic->irr;
> > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > entry = ioapic->redirtbl[irq];
> > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > level ^= entry.fields.polarity;
> > if (!level)
> > ioapic->irr &= ~mask;
> > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > return ret;
> > }
> >
> > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > +{
> > + int i;
> > + spin_lock(&ioapic->lock);
> > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > + spin_unlock(&ioapic->lock);
> > +}
> > +
> > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > int trigger_mode)
> > {
> > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > index 32872a0..a30abfe 100644
> > --- a/virt/kvm/ioapic.h
> > +++ b/virt/kvm/ioapic.h
> > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > int kvm_ioapic_init(struct kvm *kvm);
> > void kvm_ioapic_destroy(struct kvm *kvm);
> > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > + int level);
> > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > struct kvm_lapic_irq *irq);
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 5afb431..83402d7 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -33,26 +33,12 @@
> >
> > #include "ioapic.h"
> >
> > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > - int irq_source_id, int level)
> > -{
> > - /* Logical OR for level trig interrupt */
> > - if (level)
> > - set_bit(irq_source_id, irq_state);
> > - else
> > - clear_bit(irq_source_id, irq_state);
> > -
> > - return !!(*irq_state);
> > -}
> > -
> > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > struct kvm *kvm, int irq_source_id, int level)
> > {
> > #ifdef CONFIG_X86
> > struct kvm_pic *pic = pic_irqchip(kvm);
> > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > - irq_source_id, level);
> > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > #else
> > return -1;
> > #endif
> > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > struct kvm *kvm, int irq_source_id, int level)
> > {
> > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > - irq_source_id, level);
> > -
> > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > }
> >
> > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > @@ -249,8 +232,6 @@ unlock:
> >
> > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > {
> > - int i;
> > -
> > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> >
> > mutex_lock(&kvm->irq_lock);
> > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > if (!irqchip_in_kernel(kvm))
> > goto unlock;
> >
> > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > - if (i >= 16)
> > - continue;
> > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > #ifdef CONFIG_X86
> > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > #endif
> > - }
> > unlock:
> > mutex_unlock(&kvm->irq_lock);
> > }
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 22:49 ` Michael S. Tsirkin
@ 2012-07-18 23:20 ` Alex Williamson
2012-07-19 7:47 ` Gleb Natapov
2012-07-19 9:07 ` Michael S. Tsirkin
0 siblings, 2 replies; 15+ messages in thread
From: Alex Williamson @ 2012-07-18 23:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 01:49 +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > When more than 1 source id is in use for the same GSI, we have the
> > > following race related to handling irq_states:
> > >
> > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > calls kvm_ioapic_set_irq(0).
> > >
> > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > >
> > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > source ids.
> > >
> > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > This also removes the need for atomics with irq_states handling.
> > >
> > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > This survives stress testing for me (though I only tried virtio, not
> > > device assignment).
> > > Avi, Marcelo, though we have not observed this in the field,
> > > it's a bugfix so probably 3.5 material?
> > > I assume yes so the patch is against 3.5-rc7.
> > > Also stable? It's a very old bug.
> > >
> > >
> > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > virt/kvm/ioapic.h | 4 +++-
> > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index db7c1f2..fe6e9f1 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > + int irq_source_id, int level)
> >
> > This should probably be __kvm_irq_line_state given the calling
> > restrictions.
>
> It's such a trivial helper, do we need to split hairs about this?
>
> Look it's not a good time for minor coding style nits.
> 3.5 is imminent, it's about 1am here and I really don't have time to retest
> today, so we'll release another kernel with a bug.
>
> Could you focus on reviewing the functionality and correctness, and
> leave ideas for better variable naming aside until 3.6?
Please get off your high horse, this bug has existed for a long time and
nobody has noticed. __ indicates locking and makes people think twice
about arbitrarily calling it. Correct naming prevents future bugs,
which is something you've been riding me on in other areas...
> > > +{
> > > + /* Logical OR for level trig interrupt */
> > > + if (level)
> > > + __set_bit(irq_source_id, irq_state);
> > > + else
> > > + __clear_bit(irq_source_id, irq_state);
> > > +
> > > + return !!(*irq_state);
> > > +}
> > > +
> > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > >
> > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > >
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 81cf4fa..95b504b 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > pic_unlock(s);
> > > }
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> > > {
> > > - struct kvm_pic *s = opaque;
> > > int ret = -1;
> > > + int level;
> > >
> > > pic_lock(s);
> > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > pic_update_irq(s);
> > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > +{
> > > + int i;
> > > + pic_lock(s);
> > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &s->irq_states[i]);
> > > + pic_unlock(s);
> > > +}
> > > +
> > > /*
> > > * acknowledge interrupt 'irq'
> > > */
> > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > index 26fd54d..268b332 100644
> > > --- a/virt/kvm/ioapic.c
> > > +++ b/virt/kvm/ioapic.c
> > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > }
> > >
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > {
> > > u32 old_irr;
> > > u32 mask = 1 << irq;
> > > union kvm_ioapic_redirect_entry entry;
> > > int ret = 1;
> > > + int level;
> > >
> > > spin_lock(&ioapic->lock);
> > > old_irr = ioapic->irr;
> > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > entry = ioapic->redirtbl[irq];
> > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > level ^= entry.fields.polarity;
> > > if (!level)
> > > ioapic->irr &= ~mask;
> > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > +{
> > > + int i;
> > > + spin_lock(&ioapic->lock);
> > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > + spin_unlock(&ioapic->lock);
> > > +}
> > > +
> > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > int trigger_mode)
> > > {
> > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > index 32872a0..a30abfe 100644
> > > --- a/virt/kvm/ioapic.h
> > > +++ b/virt/kvm/ioapic.h
> > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > int kvm_ioapic_init(struct kvm *kvm);
> > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > + int level);
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > struct kvm_lapic_irq *irq);
> > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > index 5afb431..83402d7 100644
> > > --- a/virt/kvm/irq_comm.c
> > > +++ b/virt/kvm/irq_comm.c
> > > @@ -33,26 +33,12 @@
> > >
> > > #include "ioapic.h"
> > >
> > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > - int irq_source_id, int level)
> > > -{
> > > - /* Logical OR for level trig interrupt */
> > > - if (level)
> > > - set_bit(irq_source_id, irq_state);
> > > - else
> > > - clear_bit(irq_source_id, irq_state);
> > > -
> > > - return !!(*irq_state);
> > > -}
> > > -
> > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > #ifdef CONFIG_X86
> > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > #else
> > > return -1;
> > > #endif
> > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > -
> > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > }
> > >
> > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > @@ -249,8 +232,6 @@ unlock:
> > >
> > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > {
> > > - int i;
> > > -
> > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > >
> > > mutex_lock(&kvm->irq_lock);
> > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > if (!irqchip_in_kernel(kvm))
> > > goto unlock;
> > >
> > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > - if (i >= 16)
> > > - continue;
> > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > #ifdef CONFIG_X86
> > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > #endif
> > > - }
> > > unlock:
> > > mutex_unlock(&kvm->irq_lock);
> > > }
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 22:44 ` Michael S. Tsirkin
@ 2012-07-18 23:22 ` Alex Williamson
2012-07-19 9:15 ` Michael S. Tsirkin
2012-07-19 7:51 ` Gleb Natapov
1 sibling, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2012-07-18 23:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 01:44 +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 04:16:14PM -0600, Alex Williamson wrote:
> > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > When more than 1 source id is in use for the same GSI, we have the
> > > following race related to handling irq_states:
> > >
> > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > calls kvm_ioapic_set_irq(0).
> > >
> > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > >
> > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > source ids.
> > >
> > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > This also removes the need for atomics with irq_states handling.
> > >
> > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > This survives stress testing for me (though I only tried virtio, not
> > > device assignment).
> > > Avi, Marcelo, though we have not observed this in the field,
> > > it's a bugfix so probably 3.5 material?
> > > I assume yes so the patch is against 3.5-rc7.
> > > Also stable? It's a very old bug.
> > >
> > >
> > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > virt/kvm/ioapic.h | 4 +++-
> > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index db7c1f2..fe6e9f1 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > + int irq_source_id, int level)
> > > +{
> > > + /* Logical OR for level trig interrupt */
> > > + if (level)
> > > + __set_bit(irq_source_id, irq_state);
> > > + else
> > > + __clear_bit(irq_source_id, irq_state);
> > > +
> > > + return !!(*irq_state);
> > > +}
> > > +
> > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > >
> > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > >
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 81cf4fa..95b504b 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > pic_unlock(s);
> > > }
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> >
> > Please use "irq_source_id" and keep "level". I hate reviewing code
> > where I have to differentiate 'l' vs '1'.
>
> l is an illegal variable name? Switch to a different font.
WTF
> > Same below, mixing src_id and
> > irq_source_id in various places.
>
> irq_source_id is too long, coding style says variable names
> should be short:
> LOCAL variable names should be short, and to the point.
irq_source_id is used all over the code, so either convert them all of
be consistent.
> > > {
> > > - struct kvm_pic *s = opaque;
> > > int ret = -1;
> > > + int level;
> >
> > And you'd avoid this variable
>
> by giving a different meaning to the same variable in
> different parts of a function? Now *that* is a bad idea.
>
> l is level for a specific source, level is the resulting irq value.
Then maybe 'l' isn't a meaningful variable name.
> > >
> > > pic_lock(s);
> > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > pic_update_irq(s);
> > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > +{
> > > + int i;
> > > + pic_lock(s);
> > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &s->irq_states[i]);
> > > + pic_unlock(s);
> > > +}
> > > +
> > > /*
> > > * acknowledge interrupt 'irq'
> > > */
> > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > index 26fd54d..268b332 100644
> > > --- a/virt/kvm/ioapic.c
> > > +++ b/virt/kvm/ioapic.c
> > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > }
> > >
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > {
> > > u32 old_irr;
> > > u32 mask = 1 << irq;
> > > union kvm_ioapic_redirect_entry entry;
> > > int ret = 1;
> > > + int level;
> > >
> > > spin_lock(&ioapic->lock);
> > > old_irr = ioapic->irr;
> > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > entry = ioapic->redirtbl[irq];
> > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > level ^= entry.fields.polarity;
> > > if (!level)
> > > ioapic->irr &= ~mask;
> > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > +{
> > > + int i;
> > > + spin_lock(&ioapic->lock);
> > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > + spin_unlock(&ioapic->lock);
> > > +}
> > > +
> > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > int trigger_mode)
> > > {
> > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > index 32872a0..a30abfe 100644
> > > --- a/virt/kvm/ioapic.h
> > > +++ b/virt/kvm/ioapic.h
> > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > int kvm_ioapic_init(struct kvm *kvm);
> > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > + int level);
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > struct kvm_lapic_irq *irq);
> > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > index 5afb431..83402d7 100644
> > > --- a/virt/kvm/irq_comm.c
> > > +++ b/virt/kvm/irq_comm.c
> > > @@ -33,26 +33,12 @@
> > >
> > > #include "ioapic.h"
> > >
> > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > - int irq_source_id, int level)
> > > -{
> > > - /* Logical OR for level trig interrupt */
> > > - if (level)
> > > - set_bit(irq_source_id, irq_state);
> > > - else
> > > - clear_bit(irq_source_id, irq_state);
> > > -
> > > - return !!(*irq_state);
> > > -}
> > > -
> > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > #ifdef CONFIG_X86
> > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > #else
> > > return -1;
> > > #endif
> > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > -
> > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > }
> > >
> > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > @@ -249,8 +232,6 @@ unlock:
> > >
> > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > {
> > > - int i;
> > > -
> > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > >
> > > mutex_lock(&kvm->irq_lock);
> > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > if (!irqchip_in_kernel(kvm))
> > > goto unlock;
> > >
> > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > - if (i >= 16)
> > > - continue;
> > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > #ifdef CONFIG_X86
> > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > #endif
> > > - }
> > > unlock:
> > > mutex_unlock(&kvm->irq_lock);
> > > }
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 23:20 ` Alex Williamson
@ 2012-07-19 7:47 ` Gleb Natapov
2012-07-19 9:07 ` Michael S. Tsirkin
1 sibling, 0 replies; 15+ messages in thread
From: Gleb Natapov @ 2012-07-19 7:47 UTC (permalink / raw)
To: Alex Williamson
Cc: Michael S. Tsirkin, Avi Kivity, Marcelo Tosatti, kvm, linux-kernel
On Wed, Jul 18, 2012 at 05:20:40PM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 01:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> > > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > > When more than 1 source id is in use for the same GSI, we have the
> > > > following race related to handling irq_states:
> > > >
> > > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > > calls kvm_ioapic_set_irq(0).
> > > >
> > > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > > >
> > > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > > source ids.
> > > >
> > > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > > This also removes the need for atomics with irq_states handling.
> > > >
> > > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >
> > > > This survives stress testing for me (though I only tried virtio, not
> > > > device assignment).
> > > > Avi, Marcelo, though we have not observed this in the field,
> > > > it's a bugfix so probably 3.5 material?
> > > > I assume yes so the patch is against 3.5-rc7.
> > > > Also stable? It's a very old bug.
> > > >
> > > >
> > > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > > virt/kvm/ioapic.h | 4 +++-
> > > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index db7c1f2..fe6e9f1 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > > >
> > > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > + int irq_source_id, int level)
> > >
> > > This should probably be __kvm_irq_line_state given the calling
> > > restrictions.
> >
> > It's such a trivial helper, do we need to split hairs about this?
> >
> > Look it's not a good time for minor coding style nits.
> > 3.5 is imminent, it's about 1am here and I really don't have time to retest
> > today, so we'll release another kernel with a bug.
> >
> > Could you focus on reviewing the functionality and correctness, and
> > leave ideas for better variable naming aside until 3.6?
>
> Please get off your high horse, this bug has existed for a long time and
> nobody has noticed. __ indicates locking and makes people think twice
> about arbitrarily calling it. Correct naming prevents future bugs,
> which is something you've been riding me on in other areas...
>
Agree, this is not a recent regression and in fact the bug cannot be
triggered without device assignment that uses level interrupt (is this
works at all?).
--
Gleb.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 22:44 ` Michael S. Tsirkin
2012-07-18 23:22 ` Alex Williamson
@ 2012-07-19 7:51 ` Gleb Natapov
1 sibling, 0 replies; 15+ messages in thread
From: Gleb Natapov @ 2012-07-19 7:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alex Williamson, Avi Kivity, Marcelo Tosatti, kvm, linux-kernel
On Thu, Jul 19, 2012 at 01:44:59AM +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 04:16:14PM -0600, Alex Williamson wrote:
> > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > When more than 1 source id is in use for the same GSI, we have the
> > > following race related to handling irq_states:
> > >
> > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > calls kvm_ioapic_set_irq(0).
> > >
> > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > >
> > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > source ids.
> > >
> > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > This also removes the need for atomics with irq_states handling.
> > >
> > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >
> > > This survives stress testing for me (though I only tried virtio, not
> > > device assignment).
> > > Avi, Marcelo, though we have not observed this in the field,
> > > it's a bugfix so probably 3.5 material?
> > > I assume yes so the patch is against 3.5-rc7.
> > > Also stable? It's a very old bug.
> > >
> > >
> > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > virt/kvm/ioapic.h | 4 +++-
> > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index db7c1f2..fe6e9f1 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > + int irq_source_id, int level)
> > > +{
> > > + /* Logical OR for level trig interrupt */
> > > + if (level)
> > > + __set_bit(irq_source_id, irq_state);
> > > + else
> > > + __clear_bit(irq_source_id, irq_state);
> > > +
> > > + return !!(*irq_state);
> > > +}
> > > +
> > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > >
> > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > >
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 81cf4fa..95b504b 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > pic_unlock(s);
> > > }
> > >
> > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> >
> > Please use "irq_source_id" and keep "level". I hate reviewing code
> > where I have to differentiate 'l' vs '1'.
>
> l is an illegal variable name? Switch to a different font.
>
No, but it does not make it good variable name. I agree with Alex here.
> > Same below, mixing src_id and
> > irq_source_id in various places.
>
> irq_source_id is too long, coding style says variable names
> should be short:
> LOCAL variable names should be short, and to the point.
Again I agree with Alex about consistency.
>
> > > {
> > > - struct kvm_pic *s = opaque;
> > > int ret = -1;
> > > + int level;
> >
> > And you'd avoid this variable
>
> by giving a different meaning to the same variable in
> different parts of a function? Now *that* is a bad idea.
>
> l is level for a specific source, level is the resulting irq value.
>
You can call it shared_level if you wish, but what's wrong with doing:
ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7,
kvm_irq_line_state(&s->irq_states[irq], src_id, l));
> > >
> > > pic_lock(s);
> > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > pic_update_irq(s);
> > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > +{
> > > + int i;
> > > + pic_lock(s);
> > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &s->irq_states[i]);
> > > + pic_unlock(s);
> > > +}
> > > +
> > > /*
> > > * acknowledge interrupt 'irq'
> > > */
> > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > index 26fd54d..268b332 100644
> > > --- a/virt/kvm/ioapic.c
> > > +++ b/virt/kvm/ioapic.c
> > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > }
> > >
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > {
> > > u32 old_irr;
> > > u32 mask = 1 << irq;
> > > union kvm_ioapic_redirect_entry entry;
> > > int ret = 1;
> > > + int level;
> > >
> > > spin_lock(&ioapic->lock);
> > > old_irr = ioapic->irr;
> > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > entry = ioapic->redirtbl[irq];
> > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > level ^= entry.fields.polarity;
> > > if (!level)
> > > ioapic->irr &= ~mask;
> > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > return ret;
> > > }
> > >
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > +{
> > > + int i;
> > > + spin_lock(&ioapic->lock);
> > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > + spin_unlock(&ioapic->lock);
> > > +}
> > > +
> > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > int trigger_mode)
> > > {
> > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > index 32872a0..a30abfe 100644
> > > --- a/virt/kvm/ioapic.h
> > > +++ b/virt/kvm/ioapic.h
> > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > int kvm_ioapic_init(struct kvm *kvm);
> > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > + int level);
> > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > struct kvm_lapic_irq *irq);
> > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > index 5afb431..83402d7 100644
> > > --- a/virt/kvm/irq_comm.c
> > > +++ b/virt/kvm/irq_comm.c
> > > @@ -33,26 +33,12 @@
> > >
> > > #include "ioapic.h"
> > >
> > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > - int irq_source_id, int level)
> > > -{
> > > - /* Logical OR for level trig interrupt */
> > > - if (level)
> > > - set_bit(irq_source_id, irq_state);
> > > - else
> > > - clear_bit(irq_source_id, irq_state);
> > > -
> > > - return !!(*irq_state);
> > > -}
> > > -
> > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > #ifdef CONFIG_X86
> > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > #else
> > > return -1;
> > > #endif
> > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > struct kvm *kvm, int irq_source_id, int level)
> > > {
> > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > - irq_source_id, level);
> > > -
> > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > }
> > >
> > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > @@ -249,8 +232,6 @@ unlock:
> > >
> > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > {
> > > - int i;
> > > -
> > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > >
> > > mutex_lock(&kvm->irq_lock);
> > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > if (!irqchip_in_kernel(kvm))
> > > goto unlock;
> > >
> > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > - if (i >= 16)
> > > - continue;
> > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > #ifdef CONFIG_X86
> > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > #endif
> > > - }
> > > unlock:
> > > mutex_unlock(&kvm->irq_lock);
> > > }
> >
> >
--
Gleb.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 23:20 ` Alex Williamson
2012-07-19 7:47 ` Gleb Natapov
@ 2012-07-19 9:07 ` Michael S. Tsirkin
2012-07-19 12:11 ` Alex Williamson
1 sibling, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-19 9:07 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Wed, Jul 18, 2012 at 05:20:40PM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 01:49 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> > > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > > When more than 1 source id is in use for the same GSI, we have the
> > > > following race related to handling irq_states:
> > > >
> > > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > > calls kvm_ioapic_set_irq(0).
> > > >
> > > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > > >
> > > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > > source ids.
> > > >
> > > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > > This also removes the need for atomics with irq_states handling.
> > > >
> > > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >
> > > > This survives stress testing for me (though I only tried virtio, not
> > > > device assignment).
> > > > Avi, Marcelo, though we have not observed this in the field,
> > > > it's a bugfix so probably 3.5 material?
> > > > I assume yes so the patch is against 3.5-rc7.
> > > > Also stable? It's a very old bug.
> > > >
> > > >
> > > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > > virt/kvm/ioapic.h | 4 +++-
> > > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index db7c1f2..fe6e9f1 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > > >
> > > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > + int irq_source_id, int level)
> > >
> > > This should probably be __kvm_irq_line_state given the calling
> > > restrictions.
> >
> > It's such a trivial helper, do we need to split hairs about this?
> >
> > Look it's not a good time for minor coding style nits.
> > 3.5 is imminent, it's about 1am here and I really don't have time to retest
> > today, so we'll release another kernel with a bug.
> >
> > Could you focus on reviewing the functionality and correctness, and
> > leave ideas for better variable naming aside until 3.6?
>
> Please get off your high horse, this bug has existed for a long time and
> nobody has noticed. __ indicates locking and makes people think twice
> about arbitrarily calling it. Correct naming prevents future bugs,
> which is something you've been riding me on in other areas...
It's all fine but you already sent two messages about improving coding
style in the same patch. What I really hoped for is a thorough review
that looks at all issues at once, including the functional side as well.
Or maybe style comments is all there is ... we'll see.
> > > > +{
> > > > + /* Logical OR for level trig interrupt */
> > > > + if (level)
> > > > + __set_bit(irq_source_id, irq_state);
> > > > + else
> > > > + __clear_bit(irq_source_id, irq_state);
> > > > +
> > > > + return !!(*irq_state);
> > > > +}
> > > > +
> > > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > > >
> > > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > > >
> > > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > > index 81cf4fa..95b504b 100644
> > > > --- a/arch/x86/kvm/i8259.c
> > > > +++ b/arch/x86/kvm/i8259.c
> > > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > > pic_unlock(s);
> > > > }
> > > >
> > > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> > > > {
> > > > - struct kvm_pic *s = opaque;
> > > > int ret = -1;
> > > > + int level;
> > > >
> > > > pic_lock(s);
> > > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > > pic_update_irq(s);
> > > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > return ret;
> > > > }
> > > >
> > > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > > +{
> > > > + int i;
> > > > + pic_lock(s);
> > > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > > + __clear_bit(src_id, &s->irq_states[i]);
> > > > + pic_unlock(s);
> > > > +}
> > > > +
> > > > /*
> > > > * acknowledge interrupt 'irq'
> > > > */
> > > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > > index 26fd54d..268b332 100644
> > > > --- a/virt/kvm/ioapic.c
> > > > +++ b/virt/kvm/ioapic.c
> > > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > > }
> > > >
> > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > > {
> > > > u32 old_irr;
> > > > u32 mask = 1 << irq;
> > > > union kvm_ioapic_redirect_entry entry;
> > > > int ret = 1;
> > > > + int level;
> > > >
> > > > spin_lock(&ioapic->lock);
> > > > old_irr = ioapic->irr;
> > > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > > entry = ioapic->redirtbl[irq];
> > > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > > level ^= entry.fields.polarity;
> > > > if (!level)
> > > > ioapic->irr &= ~mask;
> > > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > return ret;
> > > > }
> > > >
> > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > > +{
> > > > + int i;
> > > > + spin_lock(&ioapic->lock);
> > > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > > + spin_unlock(&ioapic->lock);
> > > > +}
> > > > +
> > > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > > int trigger_mode)
> > > > {
> > > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > > index 32872a0..a30abfe 100644
> > > > --- a/virt/kvm/ioapic.h
> > > > +++ b/virt/kvm/ioapic.h
> > > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > > int kvm_ioapic_init(struct kvm *kvm);
> > > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > > + int level);
> > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > > struct kvm_lapic_irq *irq);
> > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > index 5afb431..83402d7 100644
> > > > --- a/virt/kvm/irq_comm.c
> > > > +++ b/virt/kvm/irq_comm.c
> > > > @@ -33,26 +33,12 @@
> > > >
> > > > #include "ioapic.h"
> > > >
> > > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > - int irq_source_id, int level)
> > > > -{
> > > > - /* Logical OR for level trig interrupt */
> > > > - if (level)
> > > > - set_bit(irq_source_id, irq_state);
> > > > - else
> > > > - clear_bit(irq_source_id, irq_state);
> > > > -
> > > > - return !!(*irq_state);
> > > > -}
> > > > -
> > > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > struct kvm *kvm, int irq_source_id, int level)
> > > > {
> > > > #ifdef CONFIG_X86
> > > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > > - irq_source_id, level);
> > > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > > #else
> > > > return -1;
> > > > #endif
> > > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > struct kvm *kvm, int irq_source_id, int level)
> > > > {
> > > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > > - irq_source_id, level);
> > > > -
> > > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > > }
> > > >
> > > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > > @@ -249,8 +232,6 @@ unlock:
> > > >
> > > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > {
> > > > - int i;
> > > > -
> > > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > > >
> > > > mutex_lock(&kvm->irq_lock);
> > > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > if (!irqchip_in_kernel(kvm))
> > > > goto unlock;
> > > >
> > > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > > - if (i >= 16)
> > > > - continue;
> > > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > > #ifdef CONFIG_X86
> > > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > > #endif
> > > > - }
> > > > unlock:
> > > > mutex_unlock(&kvm->irq_lock);
> > > > }
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-18 23:22 ` Alex Williamson
@ 2012-07-19 9:15 ` Michael S. Tsirkin
2012-07-19 12:06 ` Alex Williamson
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-19 9:15 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Wed, Jul 18, 2012 at 05:22:42PM -0600, Alex Williamson wrote:
> I hate reviewing code
> > > where I have to differentiate 'l' vs '1'.
> >
> > l is an illegal variable name? Switch to a different font.
>
> WTF
Really, you should use a font where these differ. I currently use
Monospace but there are many other good fonts. If you don't bugs sneak
in.
int x = 11;
and
int x = 1l;
Should look very different.
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-19 9:15 ` Michael S. Tsirkin
@ 2012-07-19 12:06 ` Alex Williamson
2012-07-19 12:10 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2012-07-19 12:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 12:15 +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 05:22:42PM -0600, Alex Williamson wrote:
> > I hate reviewing code
> > > > where I have to differentiate 'l' vs '1'.
> > >
> > > l is an illegal variable name? Switch to a different font.
> >
> > WTF
>
> Really, you should use a font where these differ. I currently use
> Monospace but there are many other good fonts. If you don't bugs sneak
> in.
>
> int x = 11;
> and
> int x = 1l;
>
> Should look very different.
Don't be an asshole, my dog could tell the difference between those
side-by-side. That's not what you posted. I use monospace as well and
find it offensive that you suggest someone change fonts to review your
code. Clearly, any time spent trying to identify a single character in
isolation is not well spent.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-19 12:06 ` Alex Williamson
@ 2012-07-19 12:10 ` Michael S. Tsirkin
0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-19 12:10 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, Jul 19, 2012 at 06:06:31AM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 12:15 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 18, 2012 at 05:22:42PM -0600, Alex Williamson wrote:
> > > I hate reviewing code
> > > > > where I have to differentiate 'l' vs '1'.
> > > >
> > > > l is an illegal variable name? Switch to a different font.
> > >
> > > WTF
> >
> > Really, you should use a font where these differ. I currently use
> > Monospace but there are many other good fonts. If you don't bugs sneak
> > in.
> >
> > int x = 11;
> > and
> > int x = 1l;
> >
> > Should look very different.
>
> Don't be an asshole, my dog could tell the difference between those
> side-by-side. That's not what you posted. I use monospace as well and
> find it offensive that you suggest someone change fonts to review your
> code. Clearly, any time spent trying to identify a single character in
> isolation is not well spent.
>
I apologize for offending you, this was not my intent.
I sent v2 to address your comments, thanks.
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-19 9:07 ` Michael S. Tsirkin
@ 2012-07-19 12:11 ` Alex Williamson
2012-07-19 12:24 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: Alex Williamson @ 2012-07-19 12:11 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, 2012-07-19 at 12:07 +0300, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2012 at 05:20:40PM -0600, Alex Williamson wrote:
> > On Thu, 2012-07-19 at 01:49 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> > > > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > > > When more than 1 source id is in use for the same GSI, we have the
> > > > > following race related to handling irq_states:
> > > > >
> > > > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > > > calls kvm_ioapic_set_irq(0).
> > > > >
> > > > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > > > >
> > > > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > > > source ids.
> > > > >
> > > > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > > > This also removes the need for atomics with irq_states handling.
> > > > >
> > > > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >
> > > > > This survives stress testing for me (though I only tried virtio, not
> > > > > device assignment).
> > > > > Avi, Marcelo, though we have not observed this in the field,
> > > > > it's a bugfix so probably 3.5 material?
> > > > > I assume yes so the patch is against 3.5-rc7.
> > > > > Also stable? It's a very old bug.
> > > > >
> > > > >
> > > > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > > > virt/kvm/ioapic.h | 4 +++-
> > > > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > > > >
> > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > > index db7c1f2..fe6e9f1 100644
> > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > > > >
> > > > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > > + int irq_source_id, int level)
> > > >
> > > > This should probably be __kvm_irq_line_state given the calling
> > > > restrictions.
> > >
> > > It's such a trivial helper, do we need to split hairs about this?
> > >
> > > Look it's not a good time for minor coding style nits.
> > > 3.5 is imminent, it's about 1am here and I really don't have time to retest
> > > today, so we'll release another kernel with a bug.
> > >
> > > Could you focus on reviewing the functionality and correctness, and
> > > leave ideas for better variable naming aside until 3.6?
> >
> > Please get off your high horse, this bug has existed for a long time and
> > nobody has noticed. __ indicates locking and makes people think twice
> > about arbitrarily calling it. Correct naming prevents future bugs,
> > which is something you've been riding me on in other areas...
>
> It's all fine but you already sent two messages about improving coding
> style in the same patch. What I really hoped for is a thorough review
> that looks at all issues at once, including the functional side as well.
> Or maybe style comments is all there is ... we'll see.
So you're accusing me of an superfluous review because I noted coding
style errors? Very professional. My comments on your rfc should
validate the review was thorough.
> > > > > +{
> > > > > + /* Logical OR for level trig interrupt */
> > > > > + if (level)
> > > > > + __set_bit(irq_source_id, irq_state);
> > > > > + else
> > > > > + __clear_bit(irq_source_id, irq_state);
> > > > > +
> > > > > + return !!(*irq_state);
> > > > > +}
> > > > > +
> > > > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > > > >
> > > > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > > > >
> > > > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > > > index 81cf4fa..95b504b 100644
> > > > > --- a/arch/x86/kvm/i8259.c
> > > > > +++ b/arch/x86/kvm/i8259.c
> > > > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > > > pic_unlock(s);
> > > > > }
> > > > >
> > > > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> > > > > {
> > > > > - struct kvm_pic *s = opaque;
> > > > > int ret = -1;
> > > > > + int level;
> > > > >
> > > > > pic_lock(s);
> > > > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > > > pic_update_irq(s);
> > > > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > > > +{
> > > > > + int i;
> > > > > + pic_lock(s);
> > > > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > > > + __clear_bit(src_id, &s->irq_states[i]);
> > > > > + pic_unlock(s);
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * acknowledge interrupt 'irq'
> > > > > */
> > > > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > > > index 26fd54d..268b332 100644
> > > > > --- a/virt/kvm/ioapic.c
> > > > > +++ b/virt/kvm/ioapic.c
> > > > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > > > }
> > > > >
> > > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > > > {
> > > > > u32 old_irr;
> > > > > u32 mask = 1 << irq;
> > > > > union kvm_ioapic_redirect_entry entry;
> > > > > int ret = 1;
> > > > > + int level;
> > > > >
> > > > > spin_lock(&ioapic->lock);
> > > > > old_irr = ioapic->irr;
> > > > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > > > entry = ioapic->redirtbl[irq];
> > > > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > > > level ^= entry.fields.polarity;
> > > > > if (!level)
> > > > > ioapic->irr &= ~mask;
> > > > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > > > +{
> > > > > + int i;
> > > > > + spin_lock(&ioapic->lock);
> > > > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > > > + spin_unlock(&ioapic->lock);
> > > > > +}
> > > > > +
> > > > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > > > int trigger_mode)
> > > > > {
> > > > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > > > index 32872a0..a30abfe 100644
> > > > > --- a/virt/kvm/ioapic.h
> > > > > +++ b/virt/kvm/ioapic.h
> > > > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > > > int kvm_ioapic_init(struct kvm *kvm);
> > > > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > > > + int level);
> > > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > > > struct kvm_lapic_irq *irq);
> > > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > > index 5afb431..83402d7 100644
> > > > > --- a/virt/kvm/irq_comm.c
> > > > > +++ b/virt/kvm/irq_comm.c
> > > > > @@ -33,26 +33,12 @@
> > > > >
> > > > > #include "ioapic.h"
> > > > >
> > > > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > > - int irq_source_id, int level)
> > > > > -{
> > > > > - /* Logical OR for level trig interrupt */
> > > > > - if (level)
> > > > > - set_bit(irq_source_id, irq_state);
> > > > > - else
> > > > > - clear_bit(irq_source_id, irq_state);
> > > > > -
> > > > > - return !!(*irq_state);
> > > > > -}
> > > > > -
> > > > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > > struct kvm *kvm, int irq_source_id, int level)
> > > > > {
> > > > > #ifdef CONFIG_X86
> > > > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > > > - irq_source_id, level);
> > > > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > > > #else
> > > > > return -1;
> > > > > #endif
> > > > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > > struct kvm *kvm, int irq_source_id, int level)
> > > > > {
> > > > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > > > - irq_source_id, level);
> > > > > -
> > > > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > > > }
> > > > >
> > > > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > > > @@ -249,8 +232,6 @@ unlock:
> > > > >
> > > > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > > {
> > > > > - int i;
> > > > > -
> > > > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > > > >
> > > > > mutex_lock(&kvm->irq_lock);
> > > > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > > if (!irqchip_in_kernel(kvm))
> > > > > goto unlock;
> > > > >
> > > > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > > > - if (i >= 16)
> > > > > - continue;
> > > > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > > > #ifdef CONFIG_X86
> > > > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > > > #endif
> > > > > - }
> > > > > unlock:
> > > > > mutex_unlock(&kvm->irq_lock);
> > > > > }
> > > >
> > > >
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] kvm: fix race with level interrupts
2012-07-19 12:11 ` Alex Williamson
@ 2012-07-19 12:24 ` Michael S. Tsirkin
0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2012-07-19 12:24 UTC (permalink / raw)
To: Alex Williamson; +Cc: Avi Kivity, Marcelo Tosatti, gleb, kvm, linux-kernel
On Thu, Jul 19, 2012 at 06:11:15AM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 12:07 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jul 18, 2012 at 05:20:40PM -0600, Alex Williamson wrote:
> > > On Thu, 2012-07-19 at 01:49 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jul 18, 2012 at 04:26:41PM -0600, Alex Williamson wrote:
> > > > > On Thu, 2012-07-19 at 00:52 +0300, Michael S. Tsirkin wrote:
> > > > > > When more than 1 source id is in use for the same GSI, we have the
> > > > > > following race related to handling irq_states:
> > > > > >
> > > > > > CPU 0 clears bit in irq_states. CPU 0 reads irq_state as 0.
> > > > > > CPU 1 sets bit in irq_states. CPU 1 calls kvm_ioapic_set_irq(1). CPU 0
> > > > > > calls kvm_ioapic_set_irq(0).
> > > > > >
> > > > > > Now ioapic thinks the level is 0 but irq_state is not 0.
> > > > > >
> > > > > > Note that above is valid behaviour if CPU0 and CPU1 are using different
> > > > > > source ids.
> > > > > >
> > > > > > Fix by performing all irq_states bitmap handling under pic/ioapic lock.
> > > > > > This also removes the need for atomics with irq_states handling.
> > > > > >
> > > > > > Reported-by: Gleb Natapov <gleb@redhat.com>
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > This survives stress testing for me (though I only tried virtio, not
> > > > > > device assignment).
> > > > > > Avi, Marcelo, though we have not observed this in the field,
> > > > > > it's a bugfix so probably 3.5 material?
> > > > > > I assume yes so the patch is against 3.5-rc7.
> > > > > > Also stable? It's a very old bug.
> > > > > >
> > > > > >
> > > > > > arch/x86/include/asm/kvm_host.h | 15 ++++++++++++++-
> > > > > > arch/x86/kvm/i8259.c | 14 ++++++++++++--
> > > > > > virt/kvm/ioapic.c | 13 ++++++++++++-
> > > > > > virt/kvm/ioapic.h | 4 +++-
> > > > > > virt/kvm/irq_comm.c | 31 ++++---------------------------
> > > > > > 5 files changed, 45 insertions(+), 32 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > > > index db7c1f2..fe6e9f1 100644
> > > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > > @@ -802,7 +802,20 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > > void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
> > > > > > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> > > > > >
> > > > > > -int kvm_pic_set_irq(void *opaque, int irq, int level);
> > > > > > +static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > > > + int irq_source_id, int level)
> > > > >
> > > > > This should probably be __kvm_irq_line_state given the calling
> > > > > restrictions.
> > > >
> > > > It's such a trivial helper, do we need to split hairs about this?
> > > >
> > > > Look it's not a good time for minor coding style nits.
> > > > 3.5 is imminent, it's about 1am here and I really don't have time to retest
> > > > today, so we'll release another kernel with a bug.
> > > >
> > > > Could you focus on reviewing the functionality and correctness, and
> > > > leave ideas for better variable naming aside until 3.6?
> > >
> > > Please get off your high horse, this bug has existed for a long time and
> > > nobody has noticed. __ indicates locking and makes people think twice
> > > about arbitrarily calling it. Correct naming prevents future bugs,
> > > which is something you've been riding me on in other areas...
> >
> > It's all fine but you already sent two messages about improving coding
> > style in the same patch. What I really hoped for is a thorough review
> > that looks at all issues at once, including the functional side as well.
> > Or maybe style comments is all there is ... we'll see.
>
> So you're accusing me of an superfluous review because I noted coding
> style errors?
No, what I mean is that you sent multiple messages on different parts of
the same patch which is confusing. It seems reasonable to ask for a single
mail that looks at all issues at once with a small patch like this.
> Very professional. My comments on your rfc should
> validate the review was thorough.
But you gave me no indication whether you have completed the review or
only looked at style and more comments are forthcoming.
>
> > > > > > +{
> > > > > > + /* Logical OR for level trig interrupt */
> > > > > > + if (level)
> > > > > > + __set_bit(irq_source_id, irq_state);
> > > > > > + else
> > > > > > + __clear_bit(irq_source_id, irq_state);
> > > > > > +
> > > > > > + return !!(*irq_state);
> > > > > > +}
> > > > > > +
> > > > > > +int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);
> > > > > > +void kvm_pic_clear_all(struct kvm_pic *pic, int irq_source_id);
> > > > > >
> > > > > > void kvm_inject_nmi(struct kvm_vcpu *vcpu);
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > > > > index 81cf4fa..95b504b 100644
> > > > > > --- a/arch/x86/kvm/i8259.c
> > > > > > +++ b/arch/x86/kvm/i8259.c
> > > > > > @@ -188,13 +188,14 @@ void kvm_pic_update_irq(struct kvm_pic *s)
> > > > > > pic_unlock(s);
> > > > > > }
> > > > > >
> > > > > > -int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > > > +int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
> > > > > > {
> > > > > > - struct kvm_pic *s = opaque;
> > > > > > int ret = -1;
> > > > > > + int level;
> > > > > >
> > > > > > pic_lock(s);
> > > > > > if (irq >= 0 && irq < PIC_NUM_PINS) {
> > > > > > + level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
> > > > > > ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
> > > > > > pic_update_irq(s);
> > > > > > trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,
> > > > > > @@ -205,6 +206,15 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +void kvm_pic_clear_all(struct kvm_pic *s, int src_id)
> > > > > > +{
> > > > > > + int i;
> > > > > > + pic_lock(s);
> > > > > > + for (i = 0; i < PIC_NUM_PINS; i++)
> > > > > > + __clear_bit(src_id, &s->irq_states[i]);
> > > > > > + pic_unlock(s);
> > > > > > +}
> > > > > > +
> > > > > > /*
> > > > > > * acknowledge interrupt 'irq'
> > > > > > */
> > > > > > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > > > > > index 26fd54d..268b332 100644
> > > > > > --- a/virt/kvm/ioapic.c
> > > > > > +++ b/virt/kvm/ioapic.c
> > > > > > @@ -191,17 +191,19 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
> > > > > > return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
> > > > > > }
> > > > > >
> > > > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
> > > > > > {
> > > > > > u32 old_irr;
> > > > > > u32 mask = 1 << irq;
> > > > > > union kvm_ioapic_redirect_entry entry;
> > > > > > int ret = 1;
> > > > > > + int level;
> > > > > >
> > > > > > spin_lock(&ioapic->lock);
> > > > > > old_irr = ioapic->irr;
> > > > > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > > > > > entry = ioapic->redirtbl[irq];
> > > > > > + level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
> > > > > > level ^= entry.fields.polarity;
> > > > > > if (!level)
> > > > > > ioapic->irr &= ~mask;
> > > > > > @@ -221,6 +223,15 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > > > > > return ret;
> > > > > > }
> > > > > >
> > > > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int src_id)
> > > > > > +{
> > > > > > + int i;
> > > > > > + spin_lock(&ioapic->lock);
> > > > > > + for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++)
> > > > > > + __clear_bit(src_id, &ioapic->irq_states[i]);
> > > > > > + spin_unlock(&ioapic->lock);
> > > > > > +}
> > > > > > +
> > > > > > static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
> > > > > > int trigger_mode)
> > > > > > {
> > > > > > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
> > > > > > index 32872a0..a30abfe 100644
> > > > > > --- a/virt/kvm/ioapic.h
> > > > > > +++ b/virt/kvm/ioapic.h
> > > > > > @@ -74,7 +74,9 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
> > > > > > bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
> > > > > > int kvm_ioapic_init(struct kvm *kvm);
> > > > > > void kvm_ioapic_destroy(struct kvm *kvm);
> > > > > > -int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
> > > > > > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
> > > > > > + int level);
> > > > > > +void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
> > > > > > void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
> > > > > > int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
> > > > > > struct kvm_lapic_irq *irq);
> > > > > > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > > > > > index 5afb431..83402d7 100644
> > > > > > --- a/virt/kvm/irq_comm.c
> > > > > > +++ b/virt/kvm/irq_comm.c
> > > > > > @@ -33,26 +33,12 @@
> > > > > >
> > > > > > #include "ioapic.h"
> > > > > >
> > > > > > -static inline int kvm_irq_line_state(unsigned long *irq_state,
> > > > > > - int irq_source_id, int level)
> > > > > > -{
> > > > > > - /* Logical OR for level trig interrupt */
> > > > > > - if (level)
> > > > > > - set_bit(irq_source_id, irq_state);
> > > > > > - else
> > > > > > - clear_bit(irq_source_id, irq_state);
> > > > > > -
> > > > > > - return !!(*irq_state);
> > > > > > -}
> > > > > > -
> > > > > > static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > > > struct kvm *kvm, int irq_source_id, int level)
> > > > > > {
> > > > > > #ifdef CONFIG_X86
> > > > > > struct kvm_pic *pic = pic_irqchip(kvm);
> > > > > > - level = kvm_irq_line_state(&pic->irq_states[e->irqchip.pin],
> > > > > > - irq_source_id, level);
> > > > > > - return kvm_pic_set_irq(pic, e->irqchip.pin, level);
> > > > > > + return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
> > > > > > #else
> > > > > > return -1;
> > > > > > #endif
> > > > > > @@ -62,10 +48,7 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
> > > > > > struct kvm *kvm, int irq_source_id, int level)
> > > > > > {
> > > > > > struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> > > > > > - level = kvm_irq_line_state(&ioapic->irq_states[e->irqchip.pin],
> > > > > > - irq_source_id, level);
> > > > > > -
> > > > > > - return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, level);
> > > > > > + return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
> > > > > > }
> > > > > >
> > > > > > inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
> > > > > > @@ -249,8 +232,6 @@ unlock:
> > > > > >
> > > > > > void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > > > {
> > > > > > - int i;
> > > > > > -
> > > > > > ASSERT(irq_source_id != KVM_USERSPACE_IRQ_SOURCE_ID);
> > > > > >
> > > > > > mutex_lock(&kvm->irq_lock);
> > > > > > @@ -263,14 +244,10 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
> > > > > > if (!irqchip_in_kernel(kvm))
> > > > > > goto unlock;
> > > > > >
> > > > > > - for (i = 0; i < KVM_IOAPIC_NUM_PINS; i++) {
> > > > > > - clear_bit(irq_source_id, &kvm->arch.vioapic->irq_states[i]);
> > > > > > - if (i >= 16)
> > > > > > - continue;
> > > > > > + kvm_ioapic_clear_all(kvm->arch.vioapic, irq_source_id);
> > > > > > #ifdef CONFIG_X86
> > > > > > - clear_bit(irq_source_id, &pic_irqchip(kvm)->irq_states[i]);
> > > > > > + kvm_pic_clear_all(pic_irqchip(kvm), irq_source_id);
> > > > > > #endif
> > > > > > - }
> > > > > > unlock:
> > > > > > mutex_unlock(&kvm->irq_lock);
> > > > > > }
> > > > >
> > > > >
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-07-19 12:24 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-18 21:52 [PATCH] kvm: fix race with level interrupts Michael S. Tsirkin
2012-07-18 22:16 ` Alex Williamson
2012-07-18 22:44 ` Michael S. Tsirkin
2012-07-18 23:22 ` Alex Williamson
2012-07-19 9:15 ` Michael S. Tsirkin
2012-07-19 12:06 ` Alex Williamson
2012-07-19 12:10 ` Michael S. Tsirkin
2012-07-19 7:51 ` Gleb Natapov
2012-07-18 22:26 ` Alex Williamson
2012-07-18 22:49 ` Michael S. Tsirkin
2012-07-18 23:20 ` Alex Williamson
2012-07-19 7:47 ` Gleb Natapov
2012-07-19 9:07 ` Michael S. Tsirkin
2012-07-19 12:11 ` Alex Williamson
2012-07-19 12:24 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.