Re: [Xen-devel] [PATCH v2] x86: Fixup IRQs when CPUs go down during shutdown

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.12.15 at 14:46, <ross.lagerwall@xxxxxxxxxx> wrote:
> Commit fc0c3fa2ad5c ("x86/IO-APIC: fix setup of Xen internally used IRQs
> (take 2)") introduced a regression on some hardware where Xen would hang
> during shutdown, repeating the following message:
> APIC error on CPU0: 08(08), Receive accept error
> 
> This appears to be because an interrupt (in this case from the serial
> console) destined for a CPU other than the boot CPU is left unhandled so
> an APIC error on CPU 0 is generated instead.
> 
> To fix this, before taking down the non-boot CPUs, call fixup_irqs()
> with a CPU mask of only the boot CPU to reset the IRQ affinities
> correctly.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---

Even though in this case interested people may know, missing info
on changes from previous version here.

> +/* CPU(s) have been removed from mask.  Re-set irq affinities. */
> +void fixup_irqs(const cpumask_t *mask, bool_t verbose)

The comment doesn't match reality. And I wonder whether it
wouldn't be reasonable to imply "verbose" (either from mask
equaling &cpu_online_map, or by introducing SYS_STATE_shutdown
and/or SYS_STATE_reboot).

> @@ -2385,16 +2382,27 @@ void fixup_irqs(void)
>  
>          spin_unlock(&desc->lock);
>  
> -        if ( break_affinity && set_affinity )
> -            printk("Broke affinity for irq %i\n", irq);
> -        else if ( !set_affinity )
> -            printk("Cannot set affinity for irq %i\n", irq);
> +        if ( verbose )
> +        {
> +            if ( break_affinity && set_affinity )
> +                printk("Broke affinity for irq %i\n", irq);
> +            else if ( !set_affinity )
> +                printk("Cannot set affinity for irq %i\n", irq);
> +        }

How about

        if ( !verbose )
           continue;

limiting churn on code?

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -286,6 +286,7 @@ void __stop_this_cpu(void)
>  
>  static void stop_this_cpu(void *dummy)
>  {
> +    fixup_eoi();
>      __stop_this_cpu();

Is this really needed during shutdown?

> @@ -298,6 +299,13 @@ static void stop_this_cpu(void *dummy)
>  void smp_send_stop(void)
>  {
>      int timeout = 10;
> +    cpumask_t online;
> +
> +    cpumask_clear(&online);
> +    cpumask_set_cpu(smp_processor_id(), &online);

That's what we have cpumask_of() for.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel