Re: [PATCH V5] x86/kexec: Change NMI and MCE handling on kexec path

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/12/12 10:59, Tim Deegan wrote:
> At 11:35 +0000 on 12 Dec (1355312134), Andrew Cooper wrote:
>> diff -r ef8c1b607b10 -r 96b068439bc4 xen/arch/x86/crash.c
>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -32,41 +32,127 @@
>>
>>   static atomic_t waiting_for_crash_ipi;
>>   static unsigned int crashing_cpu;
>> +static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
>>
>> -static int crash_nmi_callback(struct cpu_user_regs *regs, int cpu)
>> +/* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. */
>> +void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
>>   {
>> -    /* Don't do anything if this handler is invoked on crashing cpu.
>> -     * Otherwise, system will completely hang. Crashing cpu can get
>> -     * an NMI if system was initially booted with nmi_watchdog parameter.
>> +    int cpu = smp_processor_id();
>> +
>> +    /* nmi_shootdown_cpus() should ensure that this assertion is correct. */
>> +    ASSERT( cpu != crashing_cpu );
> nmi_shootdown_cpus() has a go, but I think it would have to do an atomic
> compare-exchange on crashing_cpu to actually be sure that only one CPU
> is crashing at a time.  If two CPUs try to lead crashes at the same
> time, it will deadlock here, with NMIs disabled on all CPUs.
>
> The old behaviour was also not entirely race-free, but a little more
> likey to make progress, as in most cases at least one CPU would see
> cpu == crashing_cpu here and return.
>
> Using cmpxchg in nmi_shootdown_cpus() would have the drawback that if
> one CPU tried to kexec and wedged, another CPU couldn't then have a go.
> Not sure which of all these unlikely scenarios is worst. :)

kexec_common_shutdown() calls one_cpu_only() which gives the impression 
that nmi_shootdown_cpus() can only be gotten to once.  I think this is 
race free for different pcpus attempting to crash at the same time, but 
is certainly not safe for a cascade crash on the same pcpu.

As far as I can reason, this race can be worked around by doing a 
combined atomic set of the KEXEC_IN_PROGRESS flag, and a set of the 
crashing cpu id.  However, there are also other race conditions along 
the crash path which are addressed in subsequent patches which are still 
in progress.  As the choice here is between two differently nasty race 
conditions, I would prefer to defer fixing it to later and doing it 
properly.

>
>> diff -r ef8c1b607b10 -r 96b068439bc4 xen/arch/x86/x86_64/entry.S
>> --- a/xen/arch/x86/x86_64/entry.S
>> +++ b/xen/arch/x86/x86_64/entry.S
>> @@ -635,11 +635,45 @@ ENTRY(nmi)
>>           movl  $TRAP_nmi,4(%rsp)
>>           jmp   handle_ist_exception
>>
>> +ENTRY(nmi_crash)
>> +        cli
> Aren't interrupts disabled already because we came in through an
> interrupt gate?

I was taking the pragmatic approach that in this case, we really have no 
guarantees about Xen state.  Having said that, we will (hopefully) have 
entered here through a gate, which will be intacked, and the function 
itself is deliberately safe to interruption.  I shall remove it.

>
>> +        pushq $0
>> +        movl $TRAP_nmi,4(%rsp)
>> +        SAVE_ALL
>> +        movq %rsp,%rdi
> Why?  do_nmi_crash() doesn't actually use any of its arguments.
> AFAICT, we could pretty much use do_nmi_crash explicitly in the IDT
> entry.

I was going with the DECLARE_TRAP_HANDLER() way of doing things, for 
consistency with the other handlers.  I can change it if you wish, but 
as this is not a hotpath, I would err on the lazy side and leave it as 
it is.

>
>> +        callq do_nmi_crash /* Does not return */
>> +        ud2
>> +
>>   ENTRY(machine_check)
>>           pushq $0
>>           movl  $TRAP_machine_check,4(%rsp)
>>           jmp   handle_ist_exception
>>
>> +/* Enable NMIs.  No special register assumptions. Only %rax is not preserved. */
>> +ENTRY(enable_nmis)
>> +        movq  %rsp, %rax /* Grab RSP before pushing */
>> +
>> +        /* Set up stack frame */
>> +        pushq $0               /* SS */
>> +        pushq %rax             /* RSP */
>> +        pushfq                 /* RFLAGS */
>> +        pushq $__HYPERVISOR_CS /* CS */
>> +        leaq  1f(%rip),%rax
>> +        pushq %rax             /* RIP */
>> +
>> +        iretq /* Disable the hardware NMI latch */
>> +1:
>> +        retq
> This could be made a bit smaller by something like:
>
> 	popq  %rcx         /* Remember return address */
> 	movq  %rsp, %rax   /* Grab RSP before pushing */
> 	
> 	/* Set up stack frame */
> 	pushq $0               /* SS */
> 	pushq %rax             /* RSP */
> 	pushfq                 /* RFLAGS */
> 	pushq $__HYPERVISOR_CS /* CS */
> 	pushq %rcx             /* RIP */
> 	
> 	iretq /* Disable the hardware NMI latch and return */
>
> which also keeps the call/return counts balanced.  But tbh I'm not sure
> it's worth it.  And I suspect you'll tell me why it's wrong. :)

I cant see any problem with it.

>
>> +
>> +/* No op trap handler.  Required for kexec crash path.  This is not
>> + * declared with the ENTRY() macro to avoid wasted alignment space.
>> + */
>> +.globl trap_nop
>> +trap_nop:
>> +        iretq
> The commit message says this reuses the iretq in enable_nmis().
>
> Cheers,
>
> Tim.

Ok - I will fix the stale message.

~Andrew