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

[Gleb Natapov <gleb@qumranet.com>]

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

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

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

--
			Gleb.