Re: [PATCH 2/3] Fix nested VMX TSC emulation

["Nadav Har'El" <nyh@math.technion.ac.il>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=iso-8859-8-i, Size: 2603 bytes --]

Hi,

On Wed, Aug 03, 2011, Zachary Amsden wrote about "Re: [PATCH 2/3] Fix nested VMX TSC emulation":
>...
> > @@ -6529,8 +6537,11 @@ static void prepare_vmcs02(struct kvm_vc
> > -       vmcs_write64(TSC_OFFSET,
> > -               vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
> > +       if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE_TSC_OFFSETING)
> > +               vmcs_write64(TSC_OFFSET,
> > +                       vmx->nested.vmcs01_tsc_offset + vmcs12->tsc_offset);
> > +       else
> > +               vmcs_write64(TSC_OFFSET, vmx->nested.vmcs01_tsc_offset);
> 
> I need more context here... where do you apply the adjustment?
> 
> The offset should be added to the vmcs01_tsc_offset only (but also
> written into the hardware VMCS, which should not be preserved when the
> guest exits).

This code is part of prepare_vmcs02(), which prepares a VMCS to run L2 based
on vmcs12 (the VMCS that L1 prepared for L2) and vmcs01 (L0's wishes for L1).
We need to run L2 with the TSC offset being the sum of the offset that L0
asked for L1, and the offset that L1 then asked for L2. What I fixed in this
patch is that L1 actually has the option not to use TSC_OFFSETING, and if it
doesn't, no offset should be added to it. As I said, this is a corner case
that probably won't happen in practice (it certainly won't happen if L1 is
KVM).

> This is correct.  You should always restore the L1 offset when exiting
> if it might have changed.  This implies also that you must update
> vmx->nested.vmcs01_tsc_offset if you receive a call to
> vmx_adjust_tsc_offset while L2 is running, which is why I wanted to
> see more context above.

I think you mistook the patch hunk you mentioned above to be somehow
relevant to vmx_adjust_tsc_offset()? It isn't... vmx_adjust_tsc_offset()
is unmodified by these patches, and was already correct. It looked, and
still looks, like this:

static void vmx_adjust_tsc_offset(struct kvm_vcpu *vcpu, s64 adjustment)
{
	u64 offset = vmcs_read64(TSC_OFFSET);
	vmcs_write64(TSC_OFFSET, offset + adjustment);
	if (is_guest_mode(vcpu)) {
		/* Even when running L2, the adjustment needs to apply to L1 */
		to_vmx(vcpu)->nested.vmcs01_tsc_offset += adjustment;
	}
}


Thanks,
Nadav.

-- 
Nadav Har'El                        |        Wednesday, Aug  3 2011, 3 Av 5771
nyh@math.technion.ac.il             |-----------------------------------------
Phone +972-523-790466, ICQ 13349191 |"Mommy! The garbage man is here!" "Well,
http://nadav.harel.org.il           |tell him we don't want any!"- Groucho Marx