* [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip
@ 2015-05-19 17:50 David Vrabel
2015-05-19 18:43 ` Boris Ostrovsky
2015-05-19 18:43 ` Boris Ostrovsky
0 siblings, 2 replies; 4+ messages in thread
From: David Vrabel @ 2015-05-19 17:50 UTC (permalink / raw)
To: xen-devel; +Cc: David Vrabel, Konrad Rzeszutek Wilk, Boris Ostrovsky, stable
A non-percpu VIRQ (e.g., VIRQ_CONSOLE) may be freed on a different
VCPU than it is bound to. This can result in a race between
handle_percpu_irq() and removing the action in __free_irq() because
handle_percpu_irq() does not take desc->lock. The interrupt handler
sees a NULL action and oopses.
Only use the percpu chip/handler for per-CPU VIRQs (like VIRQ_TIMER).
# cat /proc/interrupts | grep virq
40: 87246 0 xen-percpu-virq timer0
44: 0 0 xen-percpu-virq debug0
47: 0 20995 xen-percpu-virq timer1
51: 0 0 xen-percpu-virq debug1
69: 0 0 xen-dyn-virq xen-pcpu
74: 0 0 xen-dyn-virq mce
75: 29 0 xen-dyn-virq hvc_console
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: <stable@vger.kernel.org>
---
drivers/tty/hvc/hvc_xen.c | 2 +-
drivers/xen/events/events_base.c | 12 ++++++++----
include/xen/events.h | 2 +-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 5bab1c6..7a3d146 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -289,7 +289,7 @@ static int xen_initial_domain_console_init(void)
return -ENOMEM;
}
- info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0);
+ info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
info->vtermno = HVC_COOKIE;
spin_lock(&xencons_lock);
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2b8553b..3838795 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -957,7 +957,7 @@ unsigned xen_evtchn_nr_channels(void)
}
EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
-int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
+int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
{
struct evtchn_bind_virq bind_virq;
int evtchn, irq, ret;
@@ -971,8 +971,12 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
if (irq < 0)
goto out;
- irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
- handle_percpu_irq, "virq");
+ if (percpu)
+ irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
+ handle_percpu_irq, "virq");
+ else
+ irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
+ handle_edge_irq, "virq");
bind_virq.virq = virq;
bind_virq.vcpu = cpu;
@@ -1062,7 +1066,7 @@ int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
{
int irq, retval;
- irq = bind_virq_to_irq(virq, cpu);
+ irq = bind_virq_to_irq(virq, cpu, irqflags & IRQF_PERCPU);
if (irq < 0)
return irq;
retval = request_irq(irq, handler, irqflags, devname, dev_id);
diff --git a/include/xen/events.h b/include/xen/events.h
index 5321cd9..7d95fdf 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -17,7 +17,7 @@ int bind_evtchn_to_irqhandler(unsigned int evtchn,
irq_handler_t handler,
unsigned long irqflags, const char *devname,
void *dev_id);
-int bind_virq_to_irq(unsigned int virq, unsigned int cpu);
+int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu);
int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
irq_handler_t handler,
unsigned long irqflags, const char *devname,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip
2015-05-19 17:50 [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip David Vrabel
2015-05-19 18:43 ` Boris Ostrovsky
@ 2015-05-19 18:43 ` Boris Ostrovsky
1 sibling, 0 replies; 4+ messages in thread
From: Boris Ostrovsky @ 2015-05-19 18:43 UTC (permalink / raw)
To: David Vrabel, xen-devel; +Cc: Konrad Rzeszutek Wilk, stable
On 05/19/2015 01:50 PM, David Vrabel wrote:
> A non-percpu VIRQ (e.g., VIRQ_CONSOLE) may be freed on a different
> VCPU than it is bound to. This can result in a race between
> handle_percpu_irq() and removing the action in __free_irq() because
> handle_percpu_irq() does not take desc->lock. The interrupt handler
> sees a NULL action and oopses.
>
> Only use the percpu chip/handler for per-CPU VIRQs (like VIRQ_TIMER).
>
> # cat /proc/interrupts | grep virq
> 40: 87246 0 xen-percpu-virq timer0
> 44: 0 0 xen-percpu-virq debug0
> 47: 0 20995 xen-percpu-virq timer1
> 51: 0 0 xen-percpu-virq debug1
> 69: 0 0 xen-dyn-virq xen-pcpu
> 74: 0 0 xen-dyn-virq mce
> 75: 29 0 xen-dyn-virq hvc_console
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: <stable@vger.kernel.org>
Since we also use percpu handler when binding for IPIs should we also
set explicitly set IRQF_PERCPU in bind_ipi_to_irqhandler() (and drop it
from call sites)?
Regardless of that,
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> drivers/tty/hvc/hvc_xen.c | 2 +-
> drivers/xen/events/events_base.c | 12 ++++++++----
> include/xen/events.h | 2 +-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 5bab1c6..7a3d146 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -289,7 +289,7 @@ static int xen_initial_domain_console_init(void)
> return -ENOMEM;
> }
>
> - info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0);
> + info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
> info->vtermno = HVC_COOKIE;
>
> spin_lock(&xencons_lock);
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 2b8553b..3838795 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -957,7 +957,7 @@ unsigned xen_evtchn_nr_channels(void)
> }
> EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
>
> -int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> +int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
> {
> struct evtchn_bind_virq bind_virq;
> int evtchn, irq, ret;
> @@ -971,8 +971,12 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> if (irq < 0)
> goto out;
>
> - irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
> - handle_percpu_irq, "virq");
> + if (percpu)
> + irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
> + handle_percpu_irq, "virq");
> + else
> + irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
> + handle_edge_irq, "virq");
>
> bind_virq.virq = virq;
> bind_virq.vcpu = cpu;
> @@ -1062,7 +1066,7 @@ int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
> {
> int irq, retval;
>
> - irq = bind_virq_to_irq(virq, cpu);
> + irq = bind_virq_to_irq(virq, cpu, irqflags & IRQF_PERCPU);
> if (irq < 0)
> return irq;
> retval = request_irq(irq, handler, irqflags, devname, dev_id);
> diff --git a/include/xen/events.h b/include/xen/events.h
> index 5321cd9..7d95fdf 100644
> --- a/include/xen/events.h
> +++ b/include/xen/events.h
> @@ -17,7 +17,7 @@ int bind_evtchn_to_irqhandler(unsigned int evtchn,
> irq_handler_t handler,
> unsigned long irqflags, const char *devname,
> void *dev_id);
> -int bind_virq_to_irq(unsigned int virq, unsigned int cpu);
> +int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu);
> int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
> irq_handler_t handler,
> unsigned long irqflags, const char *devname,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip
2015-05-19 17:50 [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip David Vrabel
@ 2015-05-19 18:43 ` Boris Ostrovsky
2015-05-19 18:43 ` Boris Ostrovsky
1 sibling, 0 replies; 4+ messages in thread
From: Boris Ostrovsky @ 2015-05-19 18:43 UTC (permalink / raw)
To: David Vrabel, xen-devel; +Cc: stable
On 05/19/2015 01:50 PM, David Vrabel wrote:
> A non-percpu VIRQ (e.g., VIRQ_CONSOLE) may be freed on a different
> VCPU than it is bound to. This can result in a race between
> handle_percpu_irq() and removing the action in __free_irq() because
> handle_percpu_irq() does not take desc->lock. The interrupt handler
> sees a NULL action and oopses.
>
> Only use the percpu chip/handler for per-CPU VIRQs (like VIRQ_TIMER).
>
> # cat /proc/interrupts | grep virq
> 40: 87246 0 xen-percpu-virq timer0
> 44: 0 0 xen-percpu-virq debug0
> 47: 0 20995 xen-percpu-virq timer1
> 51: 0 0 xen-percpu-virq debug1
> 69: 0 0 xen-dyn-virq xen-pcpu
> 74: 0 0 xen-dyn-virq mce
> 75: 29 0 xen-dyn-virq hvc_console
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: <stable@vger.kernel.org>
Since we also use percpu handler when binding for IPIs should we also
set explicitly set IRQF_PERCPU in bind_ipi_to_irqhandler() (and drop it
from call sites)?
Regardless of that,
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> drivers/tty/hvc/hvc_xen.c | 2 +-
> drivers/xen/events/events_base.c | 12 ++++++++----
> include/xen/events.h | 2 +-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> index 5bab1c6..7a3d146 100644
> --- a/drivers/tty/hvc/hvc_xen.c
> +++ b/drivers/tty/hvc/hvc_xen.c
> @@ -289,7 +289,7 @@ static int xen_initial_domain_console_init(void)
> return -ENOMEM;
> }
>
> - info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0);
> + info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
> info->vtermno = HVC_COOKIE;
>
> spin_lock(&xencons_lock);
> diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
> index 2b8553b..3838795 100644
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -957,7 +957,7 @@ unsigned xen_evtchn_nr_channels(void)
> }
> EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
>
> -int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> +int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
> {
> struct evtchn_bind_virq bind_virq;
> int evtchn, irq, ret;
> @@ -971,8 +971,12 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
> if (irq < 0)
> goto out;
>
> - irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
> - handle_percpu_irq, "virq");
> + if (percpu)
> + irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
> + handle_percpu_irq, "virq");
> + else
> + irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
> + handle_edge_irq, "virq");
>
> bind_virq.virq = virq;
> bind_virq.vcpu = cpu;
> @@ -1062,7 +1066,7 @@ int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
> {
> int irq, retval;
>
> - irq = bind_virq_to_irq(virq, cpu);
> + irq = bind_virq_to_irq(virq, cpu, irqflags & IRQF_PERCPU);
> if (irq < 0)
> return irq;
> retval = request_irq(irq, handler, irqflags, devname, dev_id);
> diff --git a/include/xen/events.h b/include/xen/events.h
> index 5321cd9..7d95fdf 100644
> --- a/include/xen/events.h
> +++ b/include/xen/events.h
> @@ -17,7 +17,7 @@ int bind_evtchn_to_irqhandler(unsigned int evtchn,
> irq_handler_t handler,
> unsigned long irqflags, const char *devname,
> void *dev_id);
> -int bind_virq_to_irq(unsigned int virq, unsigned int cpu);
> +int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu);
> int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
> irq_handler_t handler,
> unsigned long irqflags, const char *devname,
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip
@ 2015-05-19 17:50 David Vrabel
0 siblings, 0 replies; 4+ messages in thread
From: David Vrabel @ 2015-05-19 17:50 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, David Vrabel, stable
A non-percpu VIRQ (e.g., VIRQ_CONSOLE) may be freed on a different
VCPU than it is bound to. This can result in a race between
handle_percpu_irq() and removing the action in __free_irq() because
handle_percpu_irq() does not take desc->lock. The interrupt handler
sees a NULL action and oopses.
Only use the percpu chip/handler for per-CPU VIRQs (like VIRQ_TIMER).
# cat /proc/interrupts | grep virq
40: 87246 0 xen-percpu-virq timer0
44: 0 0 xen-percpu-virq debug0
47: 0 20995 xen-percpu-virq timer1
51: 0 0 xen-percpu-virq debug1
69: 0 0 xen-dyn-virq xen-pcpu
74: 0 0 xen-dyn-virq mce
75: 29 0 xen-dyn-virq hvc_console
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: <stable@vger.kernel.org>
---
drivers/tty/hvc/hvc_xen.c | 2 +-
drivers/xen/events/events_base.c | 12 ++++++++----
include/xen/events.h | 2 +-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index 5bab1c6..7a3d146 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -289,7 +289,7 @@ static int xen_initial_domain_console_init(void)
return -ENOMEM;
}
- info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0);
+ info->irq = bind_virq_to_irq(VIRQ_CONSOLE, 0, false);
info->vtermno = HVC_COOKIE;
spin_lock(&xencons_lock);
diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2b8553b..3838795 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -957,7 +957,7 @@ unsigned xen_evtchn_nr_channels(void)
}
EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
-int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
+int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
{
struct evtchn_bind_virq bind_virq;
int evtchn, irq, ret;
@@ -971,8 +971,12 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu)
if (irq < 0)
goto out;
- irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
- handle_percpu_irq, "virq");
+ if (percpu)
+ irq_set_chip_and_handler_name(irq, &xen_percpu_chip,
+ handle_percpu_irq, "virq");
+ else
+ irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
+ handle_edge_irq, "virq");
bind_virq.virq = virq;
bind_virq.vcpu = cpu;
@@ -1062,7 +1066,7 @@ int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
{
int irq, retval;
- irq = bind_virq_to_irq(virq, cpu);
+ irq = bind_virq_to_irq(virq, cpu, irqflags & IRQF_PERCPU);
if (irq < 0)
return irq;
retval = request_irq(irq, handler, irqflags, devname, dev_id);
diff --git a/include/xen/events.h b/include/xen/events.h
index 5321cd9..7d95fdf 100644
--- a/include/xen/events.h
+++ b/include/xen/events.h
@@ -17,7 +17,7 @@ int bind_evtchn_to_irqhandler(unsigned int evtchn,
irq_handler_t handler,
unsigned long irqflags, const char *devname,
void *dev_id);
-int bind_virq_to_irq(unsigned int virq, unsigned int cpu);
+int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu);
int bind_virq_to_irqhandler(unsigned int virq, unsigned int cpu,
irq_handler_t handler,
unsigned long irqflags, const char *devname,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-05-19 18:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-19 17:50 [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip David Vrabel
2015-05-19 18:43 ` Boris Ostrovsky
2015-05-19 18:43 ` Boris Ostrovsky
2015-05-19 17:50 David Vrabel
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.