RE: [PATCH 20/31] nVMX: Exiting from L2 to L1

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Nadav Har'El [mailto:nyh@math.technion.ac.il]
> Sent: Wednesday, May 25, 2011 4:06 PM
> 
> On Wed, May 25, 2011, Tian, Kevin wrote about "RE: [PATCH 20/31] nVMX:
> Exiting from L2 to L1":
> > IOW, I disagree that if L0/L1 set same bit in cr0_guest_host_mask, then
> > the bit is identical in vmcs02.guest_cr0 and vmcs12.guest_cr0 because L1
> > has no permission to set its bit effectively in this case.
> >...
> > this is the problem:
> >
> > 	(vmcs12->guest_cr0 & vmcs12->cr0_guest_host_mask) !=
> > 	(vmcs02->guest_cr0 & vmcs12->cr0_guest_host_mask)
> 
> Sorry for arguing previously, this is a very good, and correct, point, which
> I missed. When both L0 and L1 are KVM, this didn't cause problems because
> the
> only problematic bit has been the TS bit, and when KVM wants to override this
> bit it always does it to 1.
> 
> So I've rewritten this function, based on my new understanding following your
> insights. I believe it now implements your formula *exactly*. Please take a
> look at the comments and the code, and see if you now agree with them:
> 
> /*
>  * On a nested exit from L2 to L1, vmcs12.guest_cr0 might not be up-to-date
>  * because L2 may have changed some cr0 bits directly
> (CRO_GUEST_HOST_MASK).
>  * This function returns the new value we should put in vmcs12.guest_cr0.
>  * It's not enough to just return the vmcs02 GUEST_CR0. Rather,
>  *  1. Bits that neither L0 nor L1 trapped, were set directly by L2 and are now
>  *     available in vmcs02 GUEST_CR0. (Note: It's enough to check that L0
>  *     didn't trap the bit, because if L1 did, so would L0).
>  *  2. Bits that L1 asked to trap (and therefore L0 also did) could not have
>  *     been modified by L2, and L1 knows it. So just leave the old value of
>  *     the bit from vmcs12.guest_cr0. Note that the bit from vmcs02
> GUEST_CR0
>  *     isn't relevant, because if L0 traps this bit it can set it to anything.
>  *  3. Bits that L1 didn't trap, but L0 did. L1 believes the guest could have
>  *     changed these bits, and therefore they need to be updated, but L0
>  *     didn't necessarily allow them to be changed in GUEST_CR0 - and
> rather
>  *     put them in vmcs02 CR0_READ_SHADOW. So take these bits from
> there.
>  */
> static inline unsigned long
> vmcs12_guest_cr0(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> {
> 	return
> 	/*1*/	(vmcs_readl(GUEST_CR0) & vcpu->arch.cr0_guest_owned_bits)
> |
> 	/*2*/	(vmcs12->guest_cr0 & vmcs12->cr0_guest_host_mask) |
> 	/*3*/	(vmcs_readl(CR0_READ_SHADOW) &
> ~(vmcs12->cr0_guest_host_mask |
> 			vcpu->arch.cr0_guest_owned_bits));
> }
> 
> 

This looks good to me. :-)

Thanks
Kevin