Re: [Xen-devel] [PATCH 2 of 2 V3] x86/kexec: Change NMI and MCE handling on kexec path

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.12.12 at 22:18, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/12/2012 11:52, Jan Beulich wrote:
>>>>> On 06.12.12 at 22:42, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +
>>> +    /* Change NMI trap handlers.  Non-crashing pcpus get nmi_crash which
>>> +     * invokes do_nmi_crash (above), which cause them to write state and
>>> +     * fall into a loop.  The crashing pcpu gets the nop handler to
>>> +     * cause it to return to this function ASAP.
>>> +     */
>>> +    for ( i = 0; i < nr_cpu_ids; ++i )
>>> +        if ( idt_tables[i] )
>>> +        {
>>> +
>>> +            if ( i == cpu )
>>> +            {
>>> +                /* Disable the interrupt stack tables for this MCE and
>>> +                 * NMI handler (shortly to become a nop) as there is a 1
>>> +                 * instruction race window where NMIs could be
>>> +                 * re-enabled and corrupt the exception frame, leaving
>>> +                 * us unable to continue on this crash path (which half
>>> +                 * defeats the point of using the nop handler in the
>>> +                 * first place).
>>> +                 *
>>> +                 * This update is safe from a security point of view, as
>>> +                 * this pcpu is never going to try to sysret back to a
>>> +                 * PV vcpu.
>>> +                 */
>>> +                set_ist(&idt_tables[i][TRAP_nmi],           IST_NONE);
>>> +                set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE);
>>> +
>>> +                _set_gate(&idt_tables[i][TRAP_nmi], 14, 0, &trap_nop);
>> This makes the first set_ist() above pointless, doesn't it?
> 
> No.  The set_ist() is to remove the possibility of stack corruption from
> reentrant NMIs, while the trap_nop handler is so we don't get diverted
> into the regular NMI handler.  There is nothing the NMI handler would do
> which could alter the outcome, and there are many cases where the
> regular NMI handler would try to panic, starting us reentrantly on the
> kexec path again (where we would deadlock on the one_cpu_only() check).

I think you didn't get the point of the question: _set_gate() clears
the IST field of the descriptor anyway, so why clear it separately
first, and then overwrite it again?

>>> +            }
>>> +            else
>>> +                _set_gate(&idt_tables[i][TRAP_nmi], 14, 0, &nmi_crash);
>>> +        }
>>> +
>>>      /* Ensure the new callback function is set before sending out the NMI. 
> */
>>>      wmb();
>>>  ...
>>> +/* Enable NMIs.  No special register assumptions, and all preserved. */
>>> +ENTRY(enable_nmis)
>>> +        pushq %rax
>> What's the point of saving %rax here, btw?
>>
>> Jan
> 
> Because at the moment I believe I might need to call it from asm
> context, when doing some of the later fixes.  I figured that it was
> better to make it safe now, rather than patch it up later.

I don't think that's good practice - if you end up not calling the
thing from assembly code, the question on the purpose of saving
%rax will re-surface sooner or later. Plus the patch by itself
wouldn't really explain either why this is so (which might be
of interest in the context of backporting it to older trees).

Jan


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