Re: [PATCH 8/9] VMX: work around lacking VNMI support

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]

Jan Kiszka wrote:
> Gleb Natapov wrote:
>> Hi Jan,
>>
>> On Fri, Sep 19, 2008 at 02:04:37PM +0200, Jan Kiszka wrote:
>>>  static void vmx_inject_irq(struct kvm_vcpu *vcpu, int irq)
>>>  {
>>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>> @@ -2356,6 +2384,29 @@ static void vmx_inject_nmi(struct kvm_vc
>>>  {
>>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>>>  
>>> +	if (!cpu_has_virtual_nmis()) {
>>> +		int desc_size = is_long_mode(vcpu) ? 16 : 8;
>>> +		struct descriptor_table dt;
>>> +		gpa_t gpa;
>>> +		u64 desc;
>>> +
>>> +		/*
>>> +		 * Deny delivery if the NMI will not be handled by an
>>> +		 * interrupt gate (workaround depends on IRQ masking).
>>> +		 */
>>> +		vmx_get_idt(vcpu, &dt);
>>> +		if (!vcpu->arch.rmode.active && dt.limit
>>> +		    >= desc_size * (NMI_VECTOR + 1) - 1) {
>>> +			gpa = vcpu->arch.mmu.gva_to_gpa(vcpu,
>>> +					dt.base + desc_size * NMI_VECTOR);
>>> +			if (kvm_read_guest(vcpu->kvm, gpa, &desc, 8) == 0
>>> +			    && ((desc >> 40) & 0x7) != 0x6)
>>> +				return;
>>> +		}
>> Windows2003 sets NMI entry in IDT as a task gate (0x5) during hibernation and this check
>> prevents it from shutting down itself. It hangs in "It is save to turn
>> your computer now" screen.
> 
> Grmbl, what a weird guest...
> 
> Is this a regression of this patch because NMIs were considered broken
> by Windows on that host CPU so far?
> 
>> If I replace this part by:
>>     if(vmx->soft_vnmi_blocked)
>> 	  return;
>> It shut itself down properly.
> 
> OK, but that almost always evaluates to false here.
> 
> I guess (hope) Windows will switch to an NMI task which has IRQs
> disabled (!(TSS.EFLAGS & IF)), so this may become another check for
> those weird task users. Waiting on the next IRQ window for NMI injection
> should work equally well with tasks, given they disable IRQs properly.

I thought about it again, fortunately before implementing this horribly
complex additional check: These safety belts make no real sense. Even if
we double-check that the guest is starting to handle NMIs with IRQs
disabled, it may still fiddle with its IRQ mask before the originally
NMI-enabling iret, confusing our soft_vmi_blocked maintenance.

So, instead of adding more paranoid checks, let's drop them altogether,
relying on reasonably designed guests that have IRQs disabled as long as
NMIs are handled. If they come with such weirdness, all we may cause is
virtual NMI nesting in fairly rare cases for such unusual guests (if
they exist at all).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]