Xen devel - Nov 2012 - [PATCH] xen: vmx: Use an INT 2 call to process real NMI's instead of self_nmi() in VMEXIT handler

The self_nmi() code cause''s an NMI to be triggered by sending an APIC
message
to the local processor. Unfortunately there is a delay in the delivery of the
APIC message and we may already have re-entered the HVM guest by the time the
NMI is taken. This results in the VMEXIT code calling the self_nmi() function
again. We have seen hundreds of iterations of this VMEXIT/VMENTER loop before
the HVM guest resumes normal operation.

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.

So we believe it is safe to directly invoke the INT call as NMI''s
should
already be blocked.

The INT 2 call will perform an IRET which will unblock later calls to the NMI
handler, according to Intel SDM Volume 3 Chapter 6.7.1.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

diff -r 62885b3c34c8 -r e1fbee58b25c xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2442,7 +2442,7 @@ 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. */
+            asm("int $2"); /* Real NMI, vector 2: normal processing.
*/
             break;
         case TRAP_machine_check:
             HVMTRACE_0D(MCE);

On Tue, 2012-11-13 at 13:21 +0000, Malcolm Crossley
wrote:
> The self_nmi() code cause''s an NMI to be triggered by sending an
APIC message
> to the local processor. Unfortunately there is a delay in the delivery of
the
> APIC message and we may already have re-entered the HVM guest by the time
the
> NMI is taken. This results in the VMEXIT code calling the self_nmi()
function
> again. We have seen hundreds of iterations of this VMEXIT/VMENTER loop
before
> the HVM guest resumes normal operation.
> 
> 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.
> 
> So we believe it is safe to directly invoke the INT call as NMI''s
should
> already be blocked.
> 
> The INT 2 call will perform an IRET which will unblock later calls to the
NMI
> handler, according to Intel SDM Volume 3 Chapter 6.7.1.
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Acked-by: Tim Deegan <tim@xen.org>
> 
> diff -r 62885b3c34c8 -r e1fbee58b25c xen/arch/x86/hvm/vmx/vmx.c
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2442,7 +2442,7 @@ 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. */
> +            asm("int $2"); /* Real NMI, vector 2: normal
processing. */
               asm volatile("...")

I think? Otherwise this could potentially get hoisted up

Do we need any clobbers? Specifically I''m thinking of memory since
taking an int2 ought to preserve register state.

Ian.

At 13:29 +0000 on 13 Nov (1352813350), Ian Campbell
wrote:
> On Tue, 2012-11-13 at 13:21 +0000, Malcolm Crossley wrote:
> > The self_nmi() code cause''s an NMI to be triggered by sending
an APIC message
> > to the local processor. Unfortunately there is a delay in the delivery
of the
> > APIC message and we may already have re-entered the HVM guest by the
time the
> > NMI is taken. This results in the VMEXIT code calling the self_nmi()
function
> > again. We have seen hundreds of iterations of this VMEXIT/VMENTER loop
before
> > the HVM guest resumes normal operation.
> > 
> > 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.
> > 
> > So we believe it is safe to directly invoke the INT call as
NMI''s should
> > already be blocked.
> > 
> > The INT 2 call will perform an IRET which will unblock later calls to
the NMI
> > handler, according to Intel SDM Volume 3 Chapter 6.7.1.
> > 
> > Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> > Acked-by: Tim Deegan <tim@xen.org>
> > 
> > diff -r 62885b3c34c8 -r e1fbee58b25c xen/arch/x86/hvm/vmx/vmx.c
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -2442,7 +2442,7 @@ 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. */
> > +            asm("int $2"); /* Real NMI, vector 2: normal
processing. */
> 
>                asm volatile("...")
> 
> I think? Otherwise this could potentially get hoisted up
Good catch.  Hoisted would be fine, but it could also be entirely
discarded. :)
> Do we need any clobbers? Specifically I''m thinking of memory since
> taking an int2 ought to preserve register state.
I don''t think so - nothing on this path depends on the actual behaviour
of the NMI handler.

Tim.

>>> On 13.11.12 at 14:39, Tim Deegan <tim@xen.org> wrote:
> At 13:29 +0000 on 13 Nov (1352813350), Ian Campbell wrote:
>> On Tue, 2012-11-13 at 13:21 +0000, Malcolm Crossley wrote:
>> > --- a/xen/arch/x86/hvm/vmx/vmx.c
>> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> > @@ -2442,7 +2442,7 @@ 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.
*/
>> > +            asm("int $2"); /* Real NMI, vector 2:
normal processing. */
>> 
>>                asm volatile("...")
>> 
>> I think? Otherwise this could potentially get hoisted up
> 
> Good catch.  Hoisted would be fine, but it could also be entirely
> discarded. :)
> 
>> Do we need any clobbers? Specifically I''m thinking of memory
since
>> taking an int2 ought to preserve register state.
> 
> I don''t think so - nothing on this path depends on the actual
behaviour
> of the NMI handler.
We should still add it, as the NMI handler does modify global
memory. Even if you can''t spot any dependency now, it would
be a rather hard to debug problem if there was, or if there ever
gets something added to that effect.

Jan

On 13/11/2012 13:39, "Tim Deegan" <tim@xen.org> wrote:
>>> diff -r 62885b3c34c8 -r e1fbee58b25c xen/arch/x86/hvm/vmx/vmx.c
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -2442,7 +2442,7 @@ 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.
*/
>>> +            asm("int $2"); /* Real NMI, vector 2: normal
processing. */
>> 
>>                asm volatile("...")
>> 
>> I think? Otherwise this could potentially get hoisted up
> 
> Good catch.  Hoisted would be fine, but it could also be entirely
> discarded. :)
Parameter-less asm blocks are a special case that will never be considered
side-effect free I believe. Still ''asm volatile'' would be our
stylistic
choice in this case anyway.

And with that:
Acked-by: Keir Fraser <keir@xen.org>