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

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

[Malcolm Crossley]

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);

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

[Ian Campbell]

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.

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

[Tim Deegan]

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.

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

[Jan Beulich]

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

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

[Keir Fraser]

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>