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

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

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

Gleb Natapov wrote:
> On Mon, Sep 22, 2008 at 09:19:27AM +0200, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Sun, Sep 21, 2008 at 06:57:32PM +0200, 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...
>>> This is a known trick that some OSes use.
>> OK. Out of curiosity: What is that trick precisely?
>>
> As far as I understand it this way it can be guaranties that the handler
> will be executed with a known state. Here is the attempt to make linux
> do the same:
> http://marc.info/?l=linux-kernel&m=121638440618671&w=4

I see, dedicated stacks for intra-privilege level switching on good old
32-bit - makes sense.

> 
>>>> Is this a regression of this patch because NMIs were considered broken
>>>> by Windows on that host CPU so far?
>>>>
>>> Nope. This is not a regression. Hibernation hangs in the same place with
>>> the current git on this machine. It works on newer CPUs.
>>>
>>>
>>>>> 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.
>>> Without this check guest BSODs. CPU 0 send two NMI in a row to CPU 1 and
>>> if second one is accepted something goes wrong.
>> That should have been caught at the caller site of vmx_inject_nmi
>> already, having to catch it here is an indication of a deeper problem.
> I understand that. I am trying to find out why we are getting there with
> vmx->soft_vnmi_blocked == 1 at all.

Much appreciated.

> 
>> Are you sure the NMIs are sent by CPU (i.e. kvm_inject_nmi is called
>> twice)?
> I am sure. I have a printk there :)
> 
>>          Maybe it is a bug I fixed meanwhile, an updated series goes out
>> later this morning.
> Will try it. 

/me is preparing the posting now.

Jan


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