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

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

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.
>
> Jan

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.

~Andrew

>
>>          break;
>>      case EXIT_REASON_MCE_DURING_VMENTRY:
>>          do_machine_check(regs);
>> @@ -2442,7 +2450,6 @@ void vmx_vmexit_handler(struct cpu_user_
>>                   (X86_EVENTTYPE_NMI << 8) )
>>                  goto exit_and_crash;
>>              HVMTRACE_0D(NMI);
>> -            self_nmi(); /* Real NMI, vector 2: normal processing. */
>>              break;
>>          case TRAP_machine_check:
>>              HVMTRACE_0D(MCE);
>
>

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