* [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
@ 2014-02-03 17:33 Oleksandr Tyshchenko
2014-02-03 17:51 ` Julien Grall
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Oleksandr Tyshchenko @ 2014-02-03 17:33 UTC (permalink / raw)
To: xen-devel
The possible deadlock scenario is explained below:
non interrupt context: interrupt contex interrupt context
(CPU0): (CPU1):
vgic_distr_mmio_write() do_trap_irq() do_softirq()
| | |
vgic_disable_irqs() ... ...
| | |
gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
| ... | |
| spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
| ... ... ...
| ... <----------------.---- spin_lock_irqsave(...) ...
| ... <----------------.-.---------------------------spin_lock_irqsave(...)
| ... . . Oops! The lock has already taken.
| spin_unlock(...) . .
| ... . .
gic_irq_disable() . .
... . .
spin_lock(...) . .
... . .
... <----------------. .
... <------------------.
...
spin_unlock(...)
Since the gic_remove_from_queues() and gic_irq_disable() called from
non interrupt context and they acquire the same lock as gic_set_guest_irq()
which called from interrupt context we must disable interrupts in these
functions to avoid possible deadlocks.
Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
---
xen/arch/arm/gic.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index c44a4d0..7d83b0c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
static void gic_irq_disable(struct irq_desc *desc)
{
int irq = desc->irq;
+ unsigned long flags;
- spin_lock(&desc->lock);
+ spin_lock_irqsave(&desc->lock, flags);
spin_lock(&gic.lock);
/* Disable routing */
GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
desc->status |= IRQ_DISABLED;
spin_unlock(&gic.lock);
- spin_unlock(&desc->lock);
+ spin_unlock_irqrestore(&desc->lock, flags);
}
static unsigned int gic_irq_startup(struct irq_desc *desc)
@@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
{
struct pending_irq *p = irq_to_pending(v, virtual_irq);
+ unsigned long flags;
- spin_lock(&gic.lock);
+ spin_lock_irqsave(&gic.lock, flags);
if ( !list_empty(&p->lr_queue) )
list_del_init(&p->lr_queue);
- spin_unlock(&gic.lock);
+ spin_unlock_irqrestore(&gic.lock, flags);
}
void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-03 17:33 [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq() Oleksandr Tyshchenko
@ 2014-02-03 17:51 ` Julien Grall
2014-02-03 17:59 ` Oleksandr Tyshchenko
2014-02-04 15:52 ` Julien Grall
2014-02-04 15:21 ` Stefano Stabellini
2014-02-06 13:07 ` Ian Campbell
2 siblings, 2 replies; 8+ messages in thread
From: Julien Grall @ 2014-02-03 17:51 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: Stefano Stabellini, Tim Deegan, Ian Campbell, xen-devel
(+ Xen ARM maintainers)
Hello Oleksandr,
Thanks for the patch. For next time, can you add the Xen ARM maintainers
in cc? With the amount of mail in the mailing list, your mail could be
lost easily. :)
On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
> The possible deadlock scenario is explained below:
>
> non interrupt context: interrupt contex interrupt context
> (CPU0): (CPU1):
> vgic_distr_mmio_write() do_trap_irq() do_softirq()
> | | |
> vgic_disable_irqs() ... ...
> | | |
> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
> | ... | |
> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
> | ... ... ...
> | ... <----------------.---- spin_lock_irqsave(...) ...
> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
> | ... . . Oops! The lock has already taken.
> | spin_unlock(...) . .
> | ... . .
> gic_irq_disable() . .
> ... . .
> spin_lock(...) . .
> ... . .
> ... <----------------. .
> ... <------------------.
> ...
> spin_unlock(...)
>
> Since the gic_remove_from_queues() and gic_irq_disable() called from
> non interrupt context and they acquire the same lock as gic_set_guest_irq()
> which called from interrupt context we must disable interrupts in these
> functions to avoid possible deadlocks.
>
> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
I think this patch should have a release exception for Xen 4.4. It's fix
a race condition in the interrupt management.
> ---
> xen/arch/arm/gic.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index c44a4d0..7d83b0c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
> static void gic_irq_disable(struct irq_desc *desc)
> {
> int irq = desc->irq;
> + unsigned long flags;
>
> - spin_lock(&desc->lock);
> + spin_lock_irqsave(&desc->lock, flags);
> spin_lock(&gic.lock);
> /* Disable routing */
> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
> desc->status |= IRQ_DISABLED;
> spin_unlock(&gic.lock);
> - spin_unlock(&desc->lock);
> + spin_unlock_irqrestore(&desc->lock, flags);
> }
>
> static unsigned int gic_irq_startup(struct irq_desc *desc)
> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
> {
> struct pending_irq *p = irq_to_pending(v, virtual_irq);
> + unsigned long flags;
>
> - spin_lock(&gic.lock);
> + spin_lock_irqsave(&gic.lock, flags);
> if ( !list_empty(&p->lr_queue) )
> list_del_init(&p->lr_queue);
> - spin_unlock(&gic.lock);
> + spin_unlock_irqrestore(&gic.lock, flags);
> }
>
> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-03 17:51 ` Julien Grall
@ 2014-02-03 17:59 ` Oleksandr Tyshchenko
2014-02-04 15:52 ` Julien Grall
1 sibling, 0 replies; 8+ messages in thread
From: Oleksandr Tyshchenko @ 2014-02-03 17:59 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, Tim Deegan, Ian Campbell, xen-devel
Hello, all.
On Mon, Feb 3, 2014 at 7:51 PM, Julien Grall <julien.grall@linaro.org> wrote:
> (+ Xen ARM maintainers)
>
> Hello Oleksandr,
>
> Thanks for the patch. For next time, can you add the Xen ARM maintainers
> in cc? With the amount of mail in the mailing list, your mail could be
> lost easily. :)
Yes, I can.
>
> On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
>> The possible deadlock scenario is explained below:
>>
>> non interrupt context: interrupt contex interrupt context
>> (CPU0): (CPU1):
>> vgic_distr_mmio_write() do_trap_irq() do_softirq()
>> | | |
>> vgic_disable_irqs() ... ...
>> | | |
>> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
>> | ... | |
>> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
>> | ... ... ...
>> | ... <----------------.---- spin_lock_irqsave(...) ...
>> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
>> | ... . . Oops! The lock has already taken.
>> | spin_unlock(...) . .
>> | ... . .
>> gic_irq_disable() . .
>> ... . .
>> spin_lock(...) . .
>> ... . .
>> ... <----------------. .
>> ... <------------------.
>> ...
>> spin_unlock(...)
>>
>> Since the gic_remove_from_queues() and gic_irq_disable() called from
>> non interrupt context and they acquire the same lock as gic_set_guest_irq()
>> which called from interrupt context we must disable interrupts in these
>> functions to avoid possible deadlocks.
>>
>> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> Acked-by: Julien Grall <julien.grall@linaro.org>
>
> I think this patch should have a release exception for Xen 4.4. It's fix
> a race condition in the interrupt management.
>
>> ---
>> xen/arch/arm/gic.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index c44a4d0..7d83b0c 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
>> static void gic_irq_disable(struct irq_desc *desc)
>> {
>> int irq = desc->irq;
>> + unsigned long flags;
>>
>> - spin_lock(&desc->lock);
>> + spin_lock_irqsave(&desc->lock, flags);
>> spin_lock(&gic.lock);
>> /* Disable routing */
>> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>> desc->status |= IRQ_DISABLED;
>> spin_unlock(&gic.lock);
>> - spin_unlock(&desc->lock);
>> + spin_unlock_irqrestore(&desc->lock, flags);
>> }
>>
>> static unsigned int gic_irq_startup(struct irq_desc *desc)
>> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
>> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>> {
>> struct pending_irq *p = irq_to_pending(v, virtual_irq);
>> + unsigned long flags;
>>
>> - spin_lock(&gic.lock);
>> + spin_lock_irqsave(&gic.lock, flags);
>> if ( !list_empty(&p->lr_queue) )
>> list_del_init(&p->lr_queue);
>> - spin_unlock(&gic.lock);
>> + spin_unlock_irqrestore(&gic.lock, flags);
>> }
>>
>> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>>
>
>
> --
> Julien Grall
Thank you.
--
Oleksandr Tyshchenko | Embedded Developer
GlobalLogic
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-03 17:51 ` Julien Grall
2014-02-03 17:59 ` Oleksandr Tyshchenko
@ 2014-02-04 15:52 ` Julien Grall
2014-02-05 14:38 ` George Dunlap
1 sibling, 1 reply; 8+ messages in thread
From: Julien Grall @ 2014-02-04 15:52 UTC (permalink / raw)
To: Oleksandr Tyshchenko
Cc: George Dunlap, Stefano Stabellini, Ian Campbell, xen-devel
Forget to add George.
On 02/03/2014 05:51 PM, Julien Grall wrote:
> (+ Xen ARM maintainers)
>
> Hello Oleksandr,
>
> Thanks for the patch. For next time, can you add the Xen ARM maintainers
> in cc? With the amount of mail in the mailing list, your mail could be
> lost easily. :)
>
> On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
>> The possible deadlock scenario is explained below:
>>
>> non interrupt context: interrupt contex interrupt context
>> (CPU0): (CPU1):
>> vgic_distr_mmio_write() do_trap_irq() do_softirq()
>> | | |
>> vgic_disable_irqs() ... ...
>> | | |
>> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
>> | ... | |
>> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
>> | ... ... ...
>> | ... <----------------.---- spin_lock_irqsave(...) ...
>> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
>> | ... . . Oops! The lock has already taken.
>> | spin_unlock(...) . .
>> | ... . .
>> gic_irq_disable() . .
>> ... . .
>> spin_lock(...) . .
>> ... . .
>> ... <----------------. .
>> ... <------------------.
>> ...
>> spin_unlock(...)
>>
>> Since the gic_remove_from_queues() and gic_irq_disable() called from
>> non interrupt context and they acquire the same lock as gic_set_guest_irq()
>> which called from interrupt context we must disable interrupts in these
>> functions to avoid possible deadlocks.
>>
>> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
> Acked-by: Julien Grall <julien.grall@linaro.org>
>
> I think this patch should have a release exception for Xen 4.4. It's fix
> a race condition in the interrupt management.
>
>> ---
>> xen/arch/arm/gic.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index c44a4d0..7d83b0c 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
>> static void gic_irq_disable(struct irq_desc *desc)
>> {
>> int irq = desc->irq;
>> + unsigned long flags;
>>
>> - spin_lock(&desc->lock);
>> + spin_lock_irqsave(&desc->lock, flags);
>> spin_lock(&gic.lock);
>> /* Disable routing */
>> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>> desc->status |= IRQ_DISABLED;
>> spin_unlock(&gic.lock);
>> - spin_unlock(&desc->lock);
>> + spin_unlock_irqrestore(&desc->lock, flags);
>> }
>>
>> static unsigned int gic_irq_startup(struct irq_desc *desc)
>> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
>> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>> {
>> struct pending_irq *p = irq_to_pending(v, virtual_irq);
>> + unsigned long flags;
>>
>> - spin_lock(&gic.lock);
>> + spin_lock_irqsave(&gic.lock, flags);
>> if ( !list_empty(&p->lr_queue) )
>> list_del_init(&p->lr_queue);
>> - spin_unlock(&gic.lock);
>> + spin_unlock_irqrestore(&gic.lock, flags);
>> }
>>
>> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>>
>
>
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-04 15:52 ` Julien Grall
@ 2014-02-05 14:38 ` George Dunlap
0 siblings, 0 replies; 8+ messages in thread
From: George Dunlap @ 2014-02-05 14:38 UTC (permalink / raw)
To: Julien Grall, Oleksandr Tyshchenko
Cc: Stefano Stabellini, Ian Campbell, xen-devel
On 02/04/2014 03:52 PM, Julien Grall wrote:
> Forget to add George.
>
> On 02/03/2014 05:51 PM, Julien Grall wrote:
>> (+ Xen ARM maintainers)
>>
>> Hello Oleksandr,
>>
>> Thanks for the patch. For next time, can you add the Xen ARM maintainers
>> in cc? With the amount of mail in the mailing list, your mail could be
>> lost easily. :)
>>
>> On 02/03/2014 05:33 PM, Oleksandr Tyshchenko wrote:
>>> The possible deadlock scenario is explained below:
>>>
>>> non interrupt context: interrupt contex interrupt context
>>> (CPU0): (CPU1):
>>> vgic_distr_mmio_write() do_trap_irq() do_softirq()
>>> | | |
>>> vgic_disable_irqs() ... ...
>>> | | |
>>> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
>>> | ... | |
>>> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
>>> | ... ... ...
>>> | ... <----------------.---- spin_lock_irqsave(...) ...
>>> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
>>> | ... . . Oops! The lock has already taken.
>>> | spin_unlock(...) . .
>>> | ... . .
>>> gic_irq_disable() . .
>>> ... . .
>>> spin_lock(...) . .
>>> ... . .
>>> ... <----------------. .
>>> ... <------------------.
>>> ...
>>> spin_unlock(...)
>>>
>>> Since the gic_remove_from_queues() and gic_irq_disable() called from
>>> non interrupt context and they acquire the same lock as gic_set_guest_irq()
>>> which called from interrupt context we must disable interrupts in these
>>> functions to avoid possible deadlocks.
>>>
>>> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>> Acked-by: Julien Grall <julien.grall@linaro.org>
>>
>> I think this patch should have a release exception for Xen 4.4. It's fix
>> a race condition in the interrupt management.
Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>>
>>> ---
>>> xen/arch/arm/gic.c | 10 ++++++----
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index c44a4d0..7d83b0c 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
>>> static void gic_irq_disable(struct irq_desc *desc)
>>> {
>>> int irq = desc->irq;
>>> + unsigned long flags;
>>>
>>> - spin_lock(&desc->lock);
>>> + spin_lock_irqsave(&desc->lock, flags);
>>> spin_lock(&gic.lock);
>>> /* Disable routing */
>>> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>>> desc->status |= IRQ_DISABLED;
>>> spin_unlock(&gic.lock);
>>> - spin_unlock(&desc->lock);
>>> + spin_unlock_irqrestore(&desc->lock, flags);
>>> }
>>>
>>> static unsigned int gic_irq_startup(struct irq_desc *desc)
>>> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
>>> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>>> {
>>> struct pending_irq *p = irq_to_pending(v, virtual_irq);
>>> + unsigned long flags;
>>>
>>> - spin_lock(&gic.lock);
>>> + spin_lock_irqsave(&gic.lock, flags);
>>> if ( !list_empty(&p->lr_queue) )
>>> list_del_init(&p->lr_queue);
>>> - spin_unlock(&gic.lock);
>>> + spin_unlock_irqrestore(&gic.lock, flags);
>>> }
>>>
>>> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-03 17:33 [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq() Oleksandr Tyshchenko
2014-02-03 17:51 ` Julien Grall
@ 2014-02-04 15:21 ` Stefano Stabellini
2014-02-04 15:36 ` Oleksandr Tyshchenko
2014-02-06 13:07 ` Ian Campbell
2 siblings, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2014-02-04 15:21 UTC (permalink / raw)
To: Oleksandr Tyshchenko; +Cc: xen-devel
On Mon, 3 Feb 2014, Oleksandr Tyshchenko wrote:
> The possible deadlock scenario is explained below:
>
> non interrupt context: interrupt contex interrupt context
> (CPU0): (CPU1):
> vgic_distr_mmio_write() do_trap_irq() do_softirq()
> | | |
> vgic_disable_irqs() ... ...
> | | |
> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
> | ... | |
> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
> | ... ... ...
> | ... <----------------.---- spin_lock_irqsave(...) ...
> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
> | ... . . Oops! The lock has already taken.
> | spin_unlock(...) . .
> | ... . .
> gic_irq_disable() . .
> ... . .
> spin_lock(...) . .
> ... . .
> ... <----------------. .
> ... <------------------.
> ...
> spin_unlock(...)
>
> Since the gic_remove_from_queues() and gic_irq_disable() called from
> non interrupt context and they acquire the same lock as gic_set_guest_irq()
> which called from interrupt context we must disable interrupts in these
> functions to avoid possible deadlocks.
>
> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
nice work Oleksandr!
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> xen/arch/arm/gic.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index c44a4d0..7d83b0c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
> static void gic_irq_disable(struct irq_desc *desc)
> {
> int irq = desc->irq;
> + unsigned long flags;
>
> - spin_lock(&desc->lock);
> + spin_lock_irqsave(&desc->lock, flags);
> spin_lock(&gic.lock);
> /* Disable routing */
> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
> desc->status |= IRQ_DISABLED;
> spin_unlock(&gic.lock);
> - spin_unlock(&desc->lock);
> + spin_unlock_irqrestore(&desc->lock, flags);
> }
>
> static unsigned int gic_irq_startup(struct irq_desc *desc)
> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
> {
> struct pending_irq *p = irq_to_pending(v, virtual_irq);
> + unsigned long flags;
>
> - spin_lock(&gic.lock);
> + spin_lock_irqsave(&gic.lock, flags);
> if ( !list_empty(&p->lr_queue) )
> list_del_init(&p->lr_queue);
> - spin_unlock(&gic.lock);
> + spin_unlock_irqrestore(&gic.lock, flags);
> }
>
> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> --
> 1.7.9.5
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-04 15:21 ` Stefano Stabellini
@ 2014-02-04 15:36 ` Oleksandr Tyshchenko
0 siblings, 0 replies; 8+ messages in thread
From: Oleksandr Tyshchenko @ 2014-02-04 15:36 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
Thanks.
Due to this thread "xen/arm: maintenance_interrupt SMP fix", I found
out a lot of interesting things.
On Tue, Feb 4, 2014 at 5:21 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 3 Feb 2014, Oleksandr Tyshchenko wrote:
>> The possible deadlock scenario is explained below:
>>
>> non interrupt context: interrupt contex interrupt context
>> (CPU0): (CPU1):
>> vgic_distr_mmio_write() do_trap_irq() do_softirq()
>> | | |
>> vgic_disable_irqs() ... ...
>> | | |
>> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
>> | ... | |
>> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
>> | ... ... ...
>> | ... <----------------.---- spin_lock_irqsave(...) ...
>> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
>> | ... . . Oops! The lock has already taken.
>> | spin_unlock(...) . .
>> | ... . .
>> gic_irq_disable() . .
>> ... . .
>> spin_lock(...) . .
>> ... . .
>> ... <----------------. .
>> ... <------------------.
>> ...
>> spin_unlock(...)
>>
>> Since the gic_remove_from_queues() and gic_irq_disable() called from
>> non interrupt context and they acquire the same lock as gic_set_guest_irq()
>> which called from interrupt context we must disable interrupts in these
>> functions to avoid possible deadlocks.
>>
>> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
>
> nice work Oleksandr!
>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
>
>> xen/arch/arm/gic.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index c44a4d0..7d83b0c 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -147,14 +147,15 @@ static void gic_irq_enable(struct irq_desc *desc)
>> static void gic_irq_disable(struct irq_desc *desc)
>> {
>> int irq = desc->irq;
>> + unsigned long flags;
>>
>> - spin_lock(&desc->lock);
>> + spin_lock_irqsave(&desc->lock, flags);
>> spin_lock(&gic.lock);
>> /* Disable routing */
>> GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
>> desc->status |= IRQ_DISABLED;
>> spin_unlock(&gic.lock);
>> - spin_unlock(&desc->lock);
>> + spin_unlock_irqrestore(&desc->lock, flags);
>> }
>>
>> static unsigned int gic_irq_startup(struct irq_desc *desc)
>> @@ -658,11 +659,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, unsigned int irq,
>> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
>> {
>> struct pending_irq *p = irq_to_pending(v, virtual_irq);
>> + unsigned long flags;
>>
>> - spin_lock(&gic.lock);
>> + spin_lock_irqsave(&gic.lock, flags);
>> if ( !list_empty(&p->lr_queue) )
>> list_del_init(&p->lr_queue);
>> - spin_unlock(&gic.lock);
>> + spin_unlock_irqrestore(&gic.lock, flags);
>> }
>>
>> void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>> --
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
--
Name | Title
GlobalLogic
P +x.xxx.xxx.xxxx M +x.xxx.xxx.xxxx S skype
www.globallogic.com
http://www.globallogic.com/email_disclaimer.txt
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq()
2014-02-03 17:33 [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq() Oleksandr Tyshchenko
2014-02-03 17:51 ` Julien Grall
2014-02-04 15:21 ` Stefano Stabellini
@ 2014-02-06 13:07 ` Ian Campbell
2 siblings, 0 replies; 8+ messages in thread
From: Ian Campbell @ 2014-02-06 13:07 UTC (permalink / raw)
To: Oleksandr Tyshchenko; +Cc: xen-devel
On Mon, 2014-02-03 at 19:33 +0200, Oleksandr Tyshchenko wrote:
> The possible deadlock scenario is explained below:
>
> non interrupt context: interrupt contex interrupt context
> (CPU0): (CPU1):
> vgic_distr_mmio_write() do_trap_irq() do_softirq()
> | | |
> vgic_disable_irqs() ... ...
> | | |
> gic_remove_from_queues() vgic_vcpu_inject_irq() vgic_vcpu_inject_irq()
> | ... | |
> | spin_lock(...) gic_set_guest_irq() gic_set_guest_irq()
> | ... ... ...
> | ... <----------------.---- spin_lock_irqsave(...) ...
> | ... <----------------.-.---------------------------spin_lock_irqsave(...)
> | ... . . Oops! The lock has already taken.
> | spin_unlock(...) . .
> | ... . .
> gic_irq_disable() . .
> ... . .
> spin_lock(...) . .
> ... . .
> ... <----------------. .
> ... <------------------.
> ...
> spin_unlock(...)
>
> Since the gic_remove_from_queues() and gic_irq_disable() called from
> non interrupt context and they acquire the same lock as gic_set_guest_irq()
> which called from interrupt context we must disable interrupts in these
> functions to avoid possible deadlocks.
>
> Change-Id: Ia354d87bb44418956e30cd7e49cc76616c359cc9
What is this?
> Signed-off-by: Oleksandr Tyshchenko <oleksandr.tyshchenko@globallogic.com>
Applied with acks from Stefano, Julien and myself.
Was there anything else arising from "xen/arm: maintenance_interrupt SMP
fix" which is a target for 4.4? I don't think so, the IPI priority
raising is targeted at 4.5.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-02-06 13:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-03 17:33 [PATCH v1] xen/arm: Fix deadlock in gic_set_guest_irq() Oleksandr Tyshchenko
2014-02-03 17:51 ` Julien Grall
2014-02-03 17:59 ` Oleksandr Tyshchenko
2014-02-04 15:52 ` Julien Grall
2014-02-05 14:38 ` George Dunlap
2014-02-04 15:21 ` Stefano Stabellini
2014-02-04 15:36 ` Oleksandr Tyshchenko
2014-02-06 13:07 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).