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

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

On 09/02/15 12:56, Jan Beulich wrote:
>>>> On 09.02.15 at 12:52, <andrew.cooper3@citrix.com> wrote:
>> 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@citrix.com>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Tim Deegan <tim@xen.org>
>>>>
>>>> ---
>>>>
>>>> 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.
> So wouldn't it be possible to get rid of nmi_crash() and have
> nmi() call *nmi_handler instead of don_nmi (and nmi_handler
> would really just become an alias of exception_table[2]?

nmi_crash() deliberately doesn't follow the handle_ist_exception path to
avoid possibly switching stack and possibly returning back to a guest. 
It has an emergency ud2 on the end to cover errors in do_nmi_crash().

Reusing exception_table[2] might be preferable to adding a new variable,
but it would involve moving exception_table[] from rodata to data, which
is rather less preferable overall.

~Andrew