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

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

On 09/02/15 13:26, Jan Beulich wrote:
>>>> On 09.02.15 at 14:13, <andrew.cooper3@citrix.com> wrote:
>> On 09/02/15 12:56, Jan Beulich wrote:
>>> 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. 
> Is the stack switching really dangerous here? And if it is, wouldn't
> checking a suitable flag to skip it still be better than the mess of
> NMI entry points?

I believe the argument was more along the lines of "less to go wrong"
than anything specifically unsafe.  One thing is certainly does do is
make the stack traces crystal clear between the crashing cpu and the
shot-down cpus. (It has been a while since I did this work, and the
commit message for 77ad1faa6b doesn't jog my memory.)

>
>> It has an emergency ud2 on the end to cover errors in do_nmi_crash().
> That could as well be a BUG() at the end of do_nmi_crash() itself.

That, along with the noreturn, would catch any coding errors in this
area.  The set of non-coding errors which could fail to be caught by a
BUG() but are caught by the ud2 is very small.

>
>> 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.
> Considering the purpose you need the modification for, stealing an
> unused (at that time) fixmap entry and creating a writeable alias
> mapping would seem a reasonable alternative to me.

It looks safe to borrow FIX_TBOOT_MAP_ADDRESS which is otherwise only
used in tboot_copy_memory() which is __init.

~Andrew