Re: [PATCH 22/31] nVMX: Deciding if L0 or L1 should handle an L2 exit

From: "Nadav Har'El" <nyh@math.technion.ac.il>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"gleb@redhat.com" <gleb@redhat.com>,
	"avi@redhat.com" <avi@redhat.com>
Subject: Re: [PATCH 22/31] nVMX: Deciding if L0 or L1 should handle an L2 exit
Date: Wed, 25 May 2011 16:45:07 +0300	[thread overview]
Message-ID: <20110525134507.GF16418@fermat.math.technion.ac.il> (raw)
In-Reply-To: <625BA99ED14B2D499DC4E29D8138F1505C9BFA39E8@shsmsx502.ccr.corp.intel.com>

On Wed, May 25, 2011, Tian, Kevin wrote about "RE: [PATCH 22/31] nVMX: Deciding if L0 or L1 should handle an L2 exit":
> > +static inline bool nested_cpu_has_virtual_nmis(struct kvm_vcpu *vcpu)
> > +{
> > +	return is_guest_mode(vcpu) &&
> > +		(get_vmcs12(vcpu)->pin_based_vm_exec_control &
> > +			PIN_BASED_VIRTUAL_NMIS);
> > +}
> 
> any reason to add guest mode check here? I didn't see such check in your
> earlier nested_cpu_has_xxx. It would be clearer to use existing nested_cpu_has_xxx
> along with is_guest_mode explicitly which makes such usage consistent.

The nested_cpu_has function is for procbased controls, not pinbased controls...
But you're right, it's strange that only this function has is_guest_mode()
in it. Moving it outside. The call site is now really ugly ;-)

> > +	case EXIT_REASON_INVLPG:
> > +		return vmcs12->cpu_based_vm_exec_control &
> > +				CPU_BASED_INVLPG_EXITING;
> 
> use nested_cpu_has.

Right ;-)

> > +	if (exit_reason == EXIT_REASON_VMLAUNCH ||
> > +	    exit_reason == EXIT_REASON_VMRESUME)
> > +		vmx->nested.nested_run_pending = 1;
> > +	else
> > +		vmx->nested.nested_run_pending = 0;
> 
> what about VMLAUNCH invoked from L2? In such case I think you expect
> L1 to run instead of L2.

Wow, a good catch!

According to the theory (see our paper :-)), our implementations should work
with any number of nesting levels, not just two. But in practice, we've always
had a bug in running L3, and we never had the time to figure out why.
This is a good lead - I'll have to investigate. But not now.

> On the other hand, isn't just guest mode check enough to differentiate
> pending nested run? When L1 invokes VMLAUNCH/VMRESUME, guest mode
> hasn't been set yet, and below check will fail. All other operations will then
> be checked by nested_vmx_exit_handled...
> 
> Do I miss anything here?

As we discussed in another thread, nested_run_pending is important later,
at the injection path.

> > -	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked)) {
> > +	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked &&
> > +			!nested_cpu_has_virtual_nmis(vcpu))) {
> 
> Would L0 want to control vNMI for L2 guest? Otherwise we just use is_guest_mode
> here for the condition check?

I don't remember the details here, but this if() used to be different, until
Avi Kivity asked me to change it to this state.

Nadav.

-- 
Nadav Har'El                        |    Wednesday, May 25 2011, 21 Iyyar 5771
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |The person who knows how to laugh at
http://nadav.harel.org.il           |himself will never cease to be amused.