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

[Tim Deegan <tim@xxxxxxx>]

At 14:21 +0000 on 13 Nov (1352816476), Jan Beulich wrote:
> >>> On 13.11.12 at 13:39, Tim Deegan <tim@xxxxxxx> wrote:
> > At 12:17 +0000 on 13 Nov (1352809049), Ian Campbell wrote:
> >> On Tue, 2012-11-13 at 12:05 +0000, Tim Deegan wrote:
> >> > At 11:47 +0000 on 13 Nov (1352807234), Tim Deegan wrote:
> >> > > At 11:38 +0000 on 13 Nov (1352806689), Jan Beulich wrote:
> >> > > > > diff -r 62885b3c34c8 -r ea756059a8da 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. */
> >> > > > 
> >> > > > In any case - why can't you call do_nmi() directly from here?
> >> > > 
> >> > > ... this is my doing.  There used to be a call to do_nmi() here, but
> >> > > do_nmi() doesn't block NMIs, so you can't just call it here in case you
> >> > > get _another_ NMI while you're in the NMI handler.
> >> > 
> >> > Oh wait, I see -- you're saying that this (20059:76a65bf2aa4d) is wrong
> >> > because NMIs are indeed blocked, and have been since the VMEXIT.
> >> > 
> >> > In that case, I agree that we should just run the NMI handler, but first
> >> > I would really like to know what _unblocks_ NMIs in this case.  Any of
> >> > the things I can think of (the next vmenter, the next iret, ??) will
> >> > need some handling to make sure they actually happen before, say, we
> >> > take this CPU into the idle loop...
> >> 
> >> What about a little stub-asm return_from_nmi / reenable_nmis with
> >> something like:
> >>    pushf
> >>         pushq $__HYPERVISOR_CS
> >>    pushq 1f
> >>    iret
> >> 1: ...
> >> 
> >> That should re-enable NMIs at whichever point we think is appropriate?
> > 
> > If an iret is what's needed then replacing self_nmi() with 'int $2'
> > (i.e. the proposed patch) seems like a neater fix.  And section 6.7.1 of
> > the manual suggests that iret is indeed what we want, so:
> 
> An IRET just cannot be the only thing that ends the NMI masked
> window.  At least some forms of #VMENTER should have that
> effect too, as otherwise NMIs would remain masked for arbitrary
> periods of time.
> 
> Further, under the assumption that the self_nmi() worked at least
> in some cases (since you likely tested it), there would also be room
> for the NMI not being masked when getting here. Which would get
> us into the stack trouble that Andrew mentioned in his reply.

I got the impression that Andrew and Malcolm were seeing a long loop of
exit/self_nmi()/enter/NMI/exit/..., eventually broken by a real
interrupt or an IPI causing (by its iret) the NMI to get delivered in
root mode.  So the NMI does get handled, just not immediately.

The fact that there was a loop and not just a delay in the NMI handling
suggests that VMENTER does indeed re-enable NMIs (or at least
NMI-exiting) but I couldn't find that in the manual.  In any case, I
think the int $2 version is correcter than the direct call as it also
disables normal interrupts.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel