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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.02.15 at 18:12, <andrew.cooper3@xxxxxxxxxx> wrote:
> @@ -127,38 +128,26 @@ static void nmi_shootdown_cpus(void)
>  
>      cpumask_andnot(&waiting_to_crash, &cpu_online_map, cpumask_of(cpu));
>  
> -    /* 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.
> +    /*
> +     * Disable IST for MCEs to avoid stack corruption race conditions, and
> +     * change the NMI hanlder to a nop to avoid deviation from this codepath.

handler

>       */
> -    for ( i = 0; i < nr_cpu_ids; i++ )
> -    {
> -        if ( idt_tables[i] == NULL )
> -            continue;
> -
> -        if ( i == cpu )
> -        {
> -            /*
> -             * Disable the interrupt stack tables for this cpu's MCE and NMI 
> -             * handlers, and alter the NMI handler to have no operation.  
> -             * Disabling the stack tables prevents stack corruption race 
> -             * conditions, while changing the handler helps prevent 
> cascading 
> -             * faults; we are certainly going to crash by this point.
> -             *
> -             * 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_gate_lower(&idt_tables[i][TRAP_nmi],
> -                            SYS_DESC_irq_gate, 0, &trap_nop);
> -            set_ist(&idt_tables[i][TRAP_machine_check], IST_NONE);
> -        }
> -        else
> -        {
> -            /* Do not update stack table for other pcpus. */
> -            _update_gate_addr_lower(&idt_tables[i][TRAP_nmi], &nmi_crash);
> -        }
> -    }
> +    _set_gate_lower(&idt_tables[cpu][TRAP_nmi],
> +                    SYS_DESC_irq_gate, 0, &trap_nop);
> +    set_ist(&idt_tables[cpu][TRAP_machine_check], IST_NONE);
> +
> +    /*
> +     * Ideally would be:
> +     *   exception_table[TRAP_nmi] = &do_nmi_crash;
> +     *
> +     * but the exception_table is read only.  Borrow and unused fixmap entry

... an unused ...

> +     * to construct a writable mapping.
> +     */
> +    set_fixmap(FIX_TBOOT_MAP_ADDRESS, __pa(&exception_table[TRAP_nmi]));
> +    write_atomic((unsigned long *)
> +                 (fix_to_virt(FIX_TBOOT_MAP_ADDRESS) +
> +                  ((unsigned long)&exception_table[TRAP_nmi] & ~PAGE_MASK)),
> +                 (unsigned long)&do_nmi_crash);

By converting the first cast to (void **) or even
(typeof(do_nmi_crash) **) it would seem possible to drop the last
cast altogether. While at it you could also drop the unnecessary &.

Jan


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