Re: [RFC] fix VMX TSC synchronicity

[Marcelo Tosatti <marcelo-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org>]

On Mon, Jan 14, 2008 at 02:06:47PM -0200, Marcelo Tosatti wrote:
> Hi Avi,
> 
> On Sun, Jan 13, 2008 at 02:19:29PM +0200, Avi Kivity wrote:
> > Marcelo Tosatti wrote:
> > >The boot TSC sync check is failing on recent Linux SMP guests on TSC
> > >stable hosts.
> > >
> > >  
> > 
> > What about tsc unstable hosts?  If your patch convinces the guest its 
> > tsc is table, while the host tsc is not, then it may cause confusion 
> > later on.
> 
> The adjustment to zero won't fool the guest because it assumes that the
> TSC's are synchronized. It will simply set the guest TSC to zero on all
> VCPUs based on the time VCPU0 was initialized.
> 
> That is, setting -(vcpu[0].first_tsc) on all VCPU's will not correct any
> synchronization problem.
> 
> > >Following patch attempts to address the problems, which are:
> > >
> > >1) APIC_DM_STARTUP, which is only called for vcpu's other than vcpu0,
> > >will trigger ->vcpu_reset(), setting the TSC to 0. Fix that by moving
> > >the guest_write_tsc(0) to vcpu_load.
> > >
> > >2) vcpu's are initialized at different times by QEMU (vcpu0 init happens
> > >way earlier than the rest). Fix that by initializing the TSC of vcpu's >
> > >0 with reference to vcpu0 init tsc value. This way TSC synchronization
> > >is kept (apparently Xen does something similar).
> > >
> > >3) The code which adjusts the TSC of a VCPU on physical CPU switch is
> > >meant to guarantee that the guest sees a monotonically increasing value.
> > >However there is a large gap, in terms of clocks, between the time which
> > >the source CPU TSC is read and the time the destination CPU TSC is read.
> > >So move those two reads to happen at vcpu_clear.
> > >
> > >I believe that 3) is the reason for the -no-kvm-irqchip problems
> > >reported by Avi on RHEL5, with kernels < 2.6.21 (where vcpu->cpu pinning
> > >would fix the problem). Unfortunately I could reproduce that problem.
> > >
> > >4-way guest with constant tick at 250Hz now reliably sees the TSC's
> > >synchronized, and idle guest CPU consumption is reduced by 50% (3-4%
> > >instead of 8%, the latter with acpi_pm_good boot parameter).
> > >
> > >
> > >diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > >index 4741806..e1c8cf4 100644
> > >--- a/arch/x86/kvm/vmx.c
> > >+++ b/arch/x86/kvm/vmx.c
> > >@@ -47,6 +47,8 @@ struct vcpu_vmx {
> > > 	struct kvm_vcpu       vcpu;
> > > 	int                   launched;
> > > 	u8                    fail;
> > >+	u64		      first_tsc;
> > >+	u64		      tsc_this;
> > > 	u32                   idt_vectoring_info;
> > > 	struct kvm_msr_entry *guest_msrs;
> > > 	struct kvm_msr_entry *host_msrs;
> > >@@ -254,6 +256,7 @@ static void vcpu_clear(struct vcpu_vmx *vmx)
> > > 	if (vmx->vcpu.cpu == -1)
> > > 		return;
> > > 	smp_call_function_single(vmx->vcpu.cpu, __vcpu_clear, vmx, 0, 1);
> > >+	rdtscll(vmx->tsc_this);
> > > 	vmx->launched = 0;
> > > }
> > > 
> > >@@ -480,6 +483,8 @@ static void vmx_load_host_state(struct vcpu_vmx *vmx)
> > > 	reload_host_efer(vmx);
> > > }
> > > 
> > >+static void guest_write_tsc(u64 host_tsc, u64 guest_tsc);
> > >+
> > > /*
> > >  * Switches to specified vcpu, until a matching vcpu_put(), but assumes
> > >  * vcpu mutex is already taken.
> > >@@ -488,7 +493,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int 
> > >cpu)
> > > {
> > > 	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > 	u64 phys_addr = __pa(vmx->vmcs);
> > >-	u64 tsc_this, delta;
> > >+	u64 delta;
> > > 
> > > 	if (vcpu->cpu != cpu) {
> > > 		vcpu_clear(vmx);
> > >@@ -511,6 +516,19 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int 
> > >cpu)
> > > 		struct descriptor_table dt;
> > > 		unsigned long sysenter_esp;
> > > 
> > >+		if (unlikely(vcpu->cpu == -1)) {
> > >  
> > 
> > This can happen after migration, I believe.
> 
> Right now the destination host will do ->vcpu_reset() on all VCPU's on
> startup... so its already broken. This patch is not introducing any
> issues, just changing where it happens :)
> 
> Perhaps fixing migration should be the subject of a separate patch?
> 
> > >+			rdtscll(vcpu->arch.host_tsc);
> > >+			vmx->tsc_this = vcpu->arch.host_tsc;
> > >+			if (vcpu->vcpu_id == 0) {
> > >+				guest_write_tsc(vcpu->arch.host_tsc, 0);
> > >+				vmx->first_tsc = vcpu->arch.host_tsc;
> > >+			} else {
> > >+				struct vcpu_vmx *cpu0;
> > >+				cpu0 = to_vmx(vcpu->kvm->vcpus[0]);
> > >+				guest_write_tsc(cpu0->first_tsc, 0);
> > >+			}
> > >+		}
> > >+
> > >  
> > 
> > Depending on vcpu_id == 0 can cause problems (for example, if vcpu 0 is 
> > somehow not the first to run).
> 
> But QEMU will always run kvm_create() first (which does
> kvm_create_vcpu(0)), then start the other threads later. So I don't see
> how that can happen.
> 
> > We might initialize the tsc base on vm creation, and if the host tsc is 
> > synced, then the guest tsc should also be.
> > 
> > > 		vcpu->cpu = cpu;
> > > 		/*
> > > 		 * Linux uses per-cpu TSS and GDT, so set these when 
> > > 		 switching
> > >@@ -526,8 +544,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int 
> > >cpu)
> > > 		/*
> > > 		 * Make sure the time stamp counter is monotonous.
> > > 		 */
> > >-		rdtscll(tsc_this);
> > >-		delta = vcpu->arch.host_tsc - tsc_this;
> > >+		delta = vcpu->arch.host_tsc - vmx->tsc_this;
> > > 		vmcs_write64(TSC_OFFSET, vmcs_read64(TSC_OFFSET) + delta);
> > >  
> > 
> > This is a little roundabout, how about moving the delta calculation 
> > immediately after the call to vcpu_clear()?
> > 
> > I don't think this is the cause of the problem, it can't account for 
> > more than a few hundred cycles, compared to the much greater vmentry cost.
> 
> Actually it accounts for several thousand cycles warp by the time the
> kernel checks the TSC synchronization between CPU's, which happens very
> early on boot.
> 
> You saw the hang on userspace init, by then there could be many
> VCPU->CPU switchs.
> 
> > 
> > Anyway it should be in a separate patch.
> 
> How does the following look (minus proper changelog):

Actually setting the TSC_OFFSET at vcpu_clear() seems a bit early:

vmwrite error: reg 2010 value 82470677 (err -2109274505)
Pid: 5590, comm: qemu-system-x86 Not tainted 2.6.24-rc6-ge7706133 #26

Call Trace:
 [<ffffffff880a5983>] :kvm_intel:vmwrite_error+0x22/0x28
 [<ffffffff880a5a99>] :kvm_intel:vcpu_clear+0x54/0x60
 [<ffffffff880a5ad0>] :kvm_intel:vmx_vcpu_load+0x29/0x12b
 [<ffffffff8024d803>] get_futex_value_locked+0x1d/0x38
 [<ffffffff8808fcf1>] :kvm:kvm_arch_vcpu_ioctl_run+0x13/0x4b9
 [<ffffffff8808c3df>] :kvm:kvm_vcpu_ioctl+0xda/0x2dd

So the first patch seemed alright (taking into account the comments from
my previous email, that migration should probably be fixed in a separate
patch since its broken already and that its guaranteed that vcpu0 is the
first to hit vcpu_load).

Avi?

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace