From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:23385 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750822AbbESSq0 (ORCPT ); Tue, 19 May 2015 14:46:26 -0400 Message-ID: <555B846F.8000900@oracle.com> Date: Tue, 19 May 2015 14:43:59 -0400 From: Boris Ostrovsky MIME-Version: 1.0 To: David Vrabel , xen-devel@lists.xenproject.org CC: Konrad Rzeszutek Wilk , stable@vger.kernel.org Subject: Re: [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip References: <1432057857-15290-1-git-send-email-david.vrabel@citrix.com> In-Reply-To: <1432057857-15290-1-git-send-email-david.vrabel@citrix.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 > Cc: 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 > --- > 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,