Re: [Qemu-devel] [PATCH] KVM: emulate lapic tsc deadline timer for hvm

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Aug 22, 2011 at 12:18:49PM +0300, Avi Kivity wrote:
> On 08/17/2011 07:19 AM, Liu, Jinsong wrote:
> > From a9670ddff84080c56183e2d678189e100f891174 Mon Sep 17 00:00:00 2001
> >From: Liu, Jinsong<jinsong.liu@intel.com>
> >Date: Wed, 17 Aug 2011 11:36:28 +0800
> >Subject: [PATCH] KVM: emulate lapic tsc deadline timer for hvm
> 
> kvm doesn't have hvm.
> 
> >This patch emulate lapic tsc deadline timer for hvm:
> >Enumerate tsc deadline timer capacibility by CPUID;
> >Enable tsc deadline timer mode by LAPIC MMIO;
> >Start tsc deadline timer by MSR;
> 
> >diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> >index 4258aac..28bcf48 100644
> >--- a/arch/x86/include/asm/cpufeature.h
> >+++ b/arch/x86/include/asm/cpufeature.h
> >@@ -120,6 +120,7 @@
> >  #define X86_FEATURE_X2APIC	(4*32+21) /* x2APIC */
> >  #define X86_FEATURE_MOVBE	(4*32+22) /* MOVBE instruction */
> >  #define X86_FEATURE_POPCNT      (4*32+23) /* POPCNT instruction */
> >+#define X86_FEATURE_TSC_DEADLINE_TIMER    (4*32+24) /* Tsc deadline timer */
> >  #define X86_FEATURE_AES		(4*32+25) /* AES instructions */
> >  #define X86_FEATURE_XSAVE	(4*32+26) /* XSAVE/XRSTOR/XSETBV/XGETBV */
> >  #define X86_FEATURE_OSXSAVE	(4*32+27) /* "" XSAVE enabled in the OS */
> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >index 307e3cf..28f7128 100644
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -635,6 +635,7 @@ struct kvm_x86_ops {
> >  	int (*check_intercept)(struct kvm_vcpu *vcpu,
> >  			       struct x86_instruction_info *info,
> >  			       enum x86_intercept_stage stage);
> >+	u64 (*guest_to_host_tsc)(u64 guest_tsc);
> >  };
> 
> Please put this near the other tsc functions.  Add a comment
> explaining what value is returned under nested virtualization.
> 
> Please add the svm callback implementation.
> 
> >
> >--- a/arch/x86/include/asm/msr-index.h
> >+++ b/arch/x86/include/asm/msr-index.h
> >@@ -229,6 +229,8 @@
> >  #define MSR_IA32_APICBASE_ENABLE	(1<<11)
> >  #define MSR_IA32_APICBASE_BASE		(0xfffff<<12)
> >
> >+#define MSR_IA32_TSCDEADLINE		0x000006e0
> >+
> >  #define MSR_IA32_UCODE_WRITE		0x00000079
> >  #define MSR_IA32_UCODE_REV		0x0000008b
> 
> Need to add to msrs_to_save so live migration works.

MSR must be explicitly listed in qemu, also.

> >+		if (!apic->lapic_timer.tscdeadline)
> >+			return;
> >+
> >+		tsc_target = kvm_x86_ops->
> >+			guest_to_host_tsc(apic->lapic_timer.tscdeadline);
> >+		rdtscll(tsc_now);
> >+		tsc_delta = tsc_target - tsc_now;
> 
> This only works if we have a constant tsc, that's not true for large
> multiboard machines.  Need to do this with irqs disabled as well
> (reading both 'now' and 'tsc_now' in the same critical section).

Should look like this:

local_irq_disable();
u64 guest_tsc = kvm_x86_ops->read_l1_tsc(vcpu);
if (guest_tsc <= tscdeadline)
        hrtimer_start(now);
else {
	ns = convert_to_ns(guest_tsc - tscdeadline);
	hrtimer_start(now + ns);
}
local_irq_enable();

Note the vcpus tsc can have different frequency than the hosts, so
vcpu_tsc_khz() should be used to convert to nanoseconds, not tsc_khz.

> >+		if (tsc_delta<  0)
> >+			tsc_delta = 0;
> >+
> >+		nsec = tsc_delta * 1000000L / tsc_khz;
> >+		hrtimer_start(&apic->lapic_timer.timer,
> >+			ktime_add_ns(now, nsec), HRTIMER_MODE_ABS);
> >+	}
> >  }
> >
> >@@ -883,6 +936,28 @@ void kvm_free_lapic(struct kvm_vcpu *vcpu)
> >   *----------------------------------------------------------------------
> >   */
> >
> >+u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu)
> >+{
> >+	struct kvm_lapic *apic = vcpu->arch.apic;
> >+
> >+	if (apic_lvtt_oneshot(apic) || apic_lvtt_period(apic))
> >+		return 0;
> 
> Why?

The hardware reset value of the IA32_TSC_DEADLINE MSR is 0. In other
timer modes (LVT bit 18 = 0), the IA32_TSC_DEADLINE MSR reads zero and
writes are ignored.

> 
> >+
> >+	return apic->lapic_timer.tscdeadline;
> >+}
> >+
> >+void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
> >+{
> >+	struct kvm_lapic *apic = vcpu->arch.apic;
> >+
> >+	if (apic_lvtt_oneshot(apic) || apic_lvtt_period(apic))
> >+		return;
> >+
> >+	hrtimer_cancel(&apic->lapic_timer.timer);
> >+	apic->lapic_timer.tscdeadline = data;
> >+	start_apic_timer(apic);
> 
> Shouldn't the msr value be updated even if we're outside tsc-deadline mode?