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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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