Re: [PATCH V3] vmx/nmi: Do not use self_nmi() in VMEXIT handler

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

On 22/11/12 15:21, Jan Beulich wrote:
>>>> On 22.11.12 at 16:16, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 22/11/12 15:15, Jan Beulich wrote:
>>>>>> On 22.11.12 at 16:00, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>>> The self_nmi() code cause's an NMI to be triggered by sending an APIC
>>>> message to the local processor.  However, NMIs are blocked by the
>>>> VMEXIT, until the next iret or VMENTER.
>>>>
>>>> Volume 3 Chapter 27 Section 1 of the Intel SDM states:
>>>>
>>>> An NMI causes subsequent NMIs to be blocked, but only after the VM exit
>>>> completes.
>>>>
>>>> As a result, as soon as the VMENTER happens, an immediate VMEXIT
>>>> happens as a result of the queued NMI.  We have seen hundreds of
>>>> iterations of this VMEXIT/VMENTER loop before the HVM guest resumes
>>>> normal operation.
>>>>
>>>> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> --
>>>> Changes since v2
>>>>  * Switch from 'int $2' to do_nmi()
>>>>  * Reworked commit message to more clearly explain the problem
>>>>
>>>> diff -r 2489c2926698 -r d7ea938044ac xen/arch/x86/hvm/vmx/vmx.c
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -2269,6 +2269,14 @@ void vmx_vmexit_handler(struct cpu_user_
>>>>          vector = intr_info & INTR_INFO_VECTOR_MASK;
>>>>          if ( vector == TRAP_machine_check )
>>>>              do_machine_check(regs);
>>>> +        else if ( vector == TRAP_nmi &&
>>>> +                ( (intr_info & INTR_INFO_INTR_TYPE_MASK) ==
>>>> +                  (X86_EVENTTYPE_NMI << 8) ) )
>>>> +            /* Must be called before interrupts are enabled to ensure
>>>> +             * the NMI handler code is run before the first IRET. The
>>>> +             * IRET unblocks subsequent NMI's (Intel SDM Vol 3, 6.7.1)
>>>> +             */
>>>> +            do_nmi();
>>> But that's only half of it, at least as far as I recall the outcome of
>>> the discussion: You want an IRET (or VMRESUME) before possibly
>>> going into the scheduler (and hence not back to the current VM).
>>> And the same also on the PV exit path from an NMI.
>> When I read this codepath, there seemed to be no consideration for
>> re-scheduling.  I will double check, but I think the VMRESUME is
>> unconditional if the VMEXIT reason was a real NMI.
> Interesting - I see nothing NMI-related in vmx/entry.S, i.e. it is
> my understanding that just like in any other case you may end
> up calling do_softirq on the way out from handling the NMI.
>
> Jan
>

Ah yes - vmx_asm_do_vmentry does have the potential to process softirqs,
which we will want to do with NMIs enabled.  I missed that last time I
looked.

So we need to deliberately execute an iretd.  int $2 is unusable because
it adds an extra dimension to the fake NMI corruption issue if you get
the combination int $2->MCE->NMI before the int $2 has correctly saved
its exception frame.

A quick solution would be to execute a noop function with
run_in_exception_handler().  Alternatively, I can code enable_nmi() or
so which does an inline iret to itself.  Which of these would you prefer?

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com