Re: [Xen-devel] [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/02/15 11:43, Tim Deegan wrote:
> Hi,
>
> At 11:25 +0000 on 09 Feb (1423477508), Andrew Cooper wrote:
>> In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the IDT
>> of each processor, such that the next NMI it receives will force it into the
>> crash path.
>>
>> c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
>> inadvertently introduced another.  The original use of self_nmi() would 
>> follow
>> vector #2, but a direct call to do_nmi() does not.
>>
>> Introduce a function pointer which should be used in preference to direct
>> do_nmi() calls, which is updated on the crash path to point at do_nmi_crash()
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Tim Deegan <tim@xxxxxxx>
>>
>> ---
>>
>> This patch very certainly functions correctly (it is in active use now in a
>> customer escalation), but I was wondering how paranoid we should be about
>> interleaved reads/writes and whether an atomic write would be better?
>> Performance is not a issue at all but in a crash senario we don't want to be
>> taking any chances with correctness.
> Yes, atomic updates sound like a good idea.  Would it make sense to
> add a _get_gate() or similar so the vmx path can read the actual IDT
> rather than adding a _third_ place where we set what to do on NMI?

A _get_gate() would return nmi() or nmi_crash() rather than do_nmi() or
do_nmi_crash().  The latter pair is needed as we are already executing
in C context rather than coming straight in from an interrupt.

>
> If not...
>
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -522,9 +522,10 @@ DECLARE_TRAP_HANDLER(alignment_check);
>>  #undef DECLARE_TRAP_HANDLER_CONST
>>  #undef DECLARE_TRAP_HANDLER
>>  
>> +extern void (*nmi_handler)(const struct cpu_user_regs *regs);
> ...please add a comment here describing what's going on - in
> particular that changing this variable doesn't change the handler for
> all NMI paths (and that for most purposes set_nmi_callback() is
> more suitable).

Good point.  I will respin with some more atomicity and comments.

~Andrew


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