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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +            }
> +            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

> +        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 */
> +
> +/* No op trap handler.  Required for kexec crash path.
> + * It is not used in performance critical code, and saves having a single
> + * lone aligned iret. It does not use ENTRY to declare the symbol to avoid 
> the
> + * explicit alignment. */
> +.globl trap_nop;
> +trap_nop:
> +
> +        iretq /* Disable the hardware NMI latch */
> +1:
> +        popq %rax
> +        retq
> +
>  .section .rodata, "a", @progbits
>  
>  ENTRY(exception_table)



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