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

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 12/02/2015 02:02 PM, Jan Beulich wrote:
>>>> On 02.12.15 at 14:46, <ross.lagerwall@citrix.com> 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@citrix.com>
>> ---
>
> 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).

I considered introducing a new SYS_STATE but decided that a function 
parameter was clearer rather than implying it from some other global state.

>
>> @@ -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?

OK.

>
>> --- 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?

Possibly not, but I think it's cleaner to do the same as what is used 
for CPU down.

>
>> @@ -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.
>

OK.

-- 
Ross Lagerwall