From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Nadav Har'El" Subject: Re: [PATCH 2/3] Fix nested VMX TSC emulation Date: Wed, 3 Aug 2011 11:35:04 +0300 Message-ID: <20110803083504.GA2863@fermat.math.technion.ac.il> References: <1312289591-nyh@il.ibm.com> <201108021254.p72Csqn1002137@rice.haifa.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-8-i Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, "Roedel, Joerg" , Bandan Das , Marcelo Tosatti , avi@redhat.com To: Zachary Amsden Return-path: Received: from mailgw12.technion.ac.il ([132.68.225.12]:1128 "EHLO mailgw12.technion.ac.il" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751935Ab1HCIfT (ORCPT ); Wed, 3 Aug 2011 04:35:19 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: Hi, On Wed, Aug 03, 2011, Zachary Amsden wrote about "Re: [PATCH 2/3] Fix n= ested VMX TSC emulation": >... > > @@ -6529,8 +6537,11 @@ static void prepare_vmcs02(struct kvm_vc > > - =A0 =A0 =A0 vmcs_write64(TSC_OFFSET, > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 vmx->nested.vmcs01_tsc_offset + vmcs1= 2->tsc_offset); > > + =A0 =A0 =A0 if (vmcs12->cpu_based_vm_exec_control & CPU_BASED_USE= _TSC_OFFSETING) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 vmcs_write64(TSC_OFFSET, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vmx->nested.vmcs01_ts= c_offset + vmcs12->tsc_offset); > > + =A0 =A0 =A0 else > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 vmcs_write64(TSC_OFFSET, vmx->nested.= vmcs01_tsc_offset); >=20 > I need more context here... where do you apply the adjustment? >=20 > The offset should be added to the vmcs01_tsc_offset only (but also > written into the hardware VMCS, which should not be preserved when th= e > 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 fo= r 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 c= ase 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 exitin= g > 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 =3D 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 +=3D adjustment; } } Thanks, Nadav. --=20 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!"- Grou= cho Marx