From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 3/3] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode Date: Wed, 11 Feb 2015 12:13:07 +0000 Message-ID: <54DB4753.6010102@citrix.com> References: <1423588332-23819-1-git-send-email-andrew.cooper3@citrix.com> <1423588332-23819-4-git-send-email-andrew.cooper3@citrix.com> <54DB44EE020000780005EF90@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54DB44EE020000780005EF90@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , Xen-devel List-Id: xen-devel@lists.xenproject.org On 11/02/15 11:02, Jan Beulich wrote: >>>> On 10.02.15 at 18:12, 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 &. None of these casts can be dropped, because write_atomic() uses explicit uint??_t casts. Gcc objects because of -Werror=pointer-to-int-cast. ~Andrew