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

Avi Kivity
2007-03-03T08:47:00Z

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.