Re: [PATCH 1/3] Sync guest viewable TSC when vcpu migrated

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Thanks for picking this up!  Some comments below.

Leonard Norrgard wrote:
> Index: linux-2.6/drivers/kvm/svm.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/svm.c	2007-03-02 17:29:57.000000000 +0200
> +++ linux-2.6/drivers/kvm/svm.c	2007-03-02 17:31:15.000000000 +0200
> @@ -598,9 +598,45 @@
>  	kfree(vcpu->svm);
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
> +/*
> + * Switches to specified vcpu, until a matching vcpu_put(), but assumes
> + * vcpu mutex is already taken.
> + */
>  static struct kvm_vcpu *svm_vcpu_load(struct kvm_vcpu *vcpu)
>  {
> -	get_cpu();
> +	int cpu;
> +
> +	cpu = get_cpu();
> +
> +#ifdef CONFIG_SMP
>   

Please avoid such #ifdefs.  Non-SMP virtualization capable processors 
are rare, and smp_call_function_single() is defined for the !CONFIG_SMP 
case.

> +	if (vcpu->cpu != cpu) {
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			vcpu->svm->vmcb->control.tsc_offset +=
> +						tsc_previous - tsc_this;
>   

>  
> Index: linux-2.6/drivers/kvm/vmx.c
> ===================================================================
> --- linux-2.6.orig/drivers/kvm/vmx.c	2007-03-02 17:30:01.000000000 +0200
> +++ linux-2.6/drivers/kvm/vmx.c	2007-03-02 17:30:17.000000000 +0200
> @@ -200,6 +200,14 @@
>  #endif
>  }
>  
> +#ifdef CONFIG_SMP
> +static void ipi_rdtscll(void *arg)
> +{
> +	u64 *tsc = arg;
> +	rdtscll(*tsc);
> +}
> +#endif
> +
>  /*
>   * Switches to specified vcpu, until a matching vcpu_put(), but assumes
>   * vcpu mutex is already taken.
> @@ -230,6 +238,25 @@
>  		struct descriptor_table dt;
>  		unsigned long sysenter_esp;
>  
> +#ifdef CONFIG_SMP
> +		if (vcpu->cpu != -1) {
> +			u64 tsc_this, tsc_previous, guest_tsc_offset;
> +
> +			/* Get TSC value for this and the previous cpu. */
> +			rdtscll(tsc_this);
> +			smp_call_function_single(vcpu->cpu, ipi_rdtscll,
> +						 &tsc_previous, 0, 1);
> +
> +			/*
> +			 * Make sure that the guest sees a monotonically
> +			 * increasing TSC.
> +			 */
> +			guest_tsc_offset = vmcs_read64(TSC_OFFSET);
> +			vmcs_write64(TSC_OFFSET, guest_tsc_offset +
> +				     		tsc_previous - tsc_this);
> +		}
> +#endif
> +
>   

The vmx code already has an IPI (in vcpu_clear), so the two should be 
merged. IPIs are very expensive.


Perhaps call a vmx_vcpu_migrate IPI, which calls __vcpu_clear() and 
loads the tsc.  You can add a tsc_previous to struct kvm_vcpu so you 
have somewhere to store it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV