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

[Mats Petersson <mats.petersson@citrix.com>]

On 15/11/12 17:44, Tim Deegan wrote:
> At 17:33 +0000 on 15 Nov (1353000782), Mats Petersson wrote:
>> On 15/11/12 17:15, Tim Deegan wrote:
>>> At 17:03 +0000 on 15 Nov (1352998993), Mats Petersson wrote:
>>>>> On an AMD CPU we _don't_ have dedicated stacks for NMI or MCE when we're
>>>>> running a HVM guest, so the stack issue doesn't apply (but nested NMIs
>>>>> are still bad).
>>>>>
>>>>> On an Intel CPU, we _do_ use dedicated stacks for NMI and MCE in HVM
>>>>> guests.  We don't really have to but it saves time in the context switch
>>>>> not to update the IDT.  Using do_nmi() here means that the first NMI is
>>>>> handled on the normal stack instead.  It's also consistent with the way
>>>>> we call do_machine_check() for the MCE case.  But it needs an explicit
>>>>> IRET after the call to do_nmi() to make sure that NMIs get re-enabled.
>>>> Both AMD and Intel has an identical setup with regard to stacks and
>>>> general "what happens when we taken one of these interrupts".
>>> My reading of svm_ctxt_switch_{to,from} makes me disagree with this.
>>> AFAICT, on SVM we're not using dedicated stacks at all.
>> In SVM, the VMRUN returns to whatever stack you had before the VMRUN.
>> This is not what I'm talking about, however. The stack used for the NMI
>> and MCE comes from the interrupt descriptor entry for those respective
>> vectors.
> This is the code I was referring to:
>
>      /*
>       * Cannot use ISTs for NMI/#MC/#DF while we are running with the guest TR.
>       * But this doesn't matter: the IST is only req'd to handle SYSCALL/SYSRET.
>       */
>      idt_tables[cpu][TRAP_double_fault].a  &= ~(7UL << 32);
>      idt_tables[cpu][TRAP_nmi].a           &= ~(7UL << 32);
>      idt_tables[cpu][TRAP_machine_check].a &= ~(7UL << 32);
>
> Am I misreading it?

No, you are reading it perfectly right, I'm wrong...

--
Mats
>
>> So in conclusion, the do_mce_exception() call probably should be a
>> __asm__ __volatile__("int $X"), where X is the relevant vector.
> This handles MCEs that were raised in guest context.  If we've managed
> to get this far into the exit handler, the hypervisor stack is probably
> OK. :)
>
> I'd be happy to invoke the MCE handler though the IDT here, just for
> symmetry with the other cases, but I don't think it makes much
> difference.
>
> Tim.
>
>