Re: [PATCHv1] xen/events: don't bind non-percpu VIRQs with percpu chip

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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,