| Message ID | 1368445517-5496-2-git-send-email-vrozenfe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 13, 2013 at 4:45 AM, Vadim Rozenfeld <vrozenfe@redhat.com> wrote: > Signed-off: Peter Lieven <pl@dlh.net> > Signed-off: Gleb Natapov <gleb@redhat.com> > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > The following patch allows to activate Hyper-V > reference time counter > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 3741c65..f0fee35 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -575,6 +575,8 @@ struct kvm_arch { > /* fields used by HYPER-V emulation */ > u64 hv_guest_os_id; > u64 hv_hypercall; > + u64 hv_ref_count; > + u64 hv_tsc_page; > > #ifdef CONFIG_KVM_MMU_AUDIT > int audit_point; > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index b80420b..9711819 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -136,6 +136,9 @@ > /* MSR used to read the per-partition time reference counter */ > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > +/* A partition's reference time stamp counter (TSC) page */ > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > + > /* Define the virtual APIC registers */ > #define HV_X64_MSR_EOI 0x40000070 > #define HV_X64_MSR_ICR 0x40000071 > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 094b5d9..1a4036d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > static u32 msrs_to_save[] = { > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > MSR_KVM_PV_EOI_EN, > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > switch (msr) { > case HV_X64_MSR_GUEST_OS_ID: > case HV_X64_MSR_HYPERCALL: > + case HV_X64_MSR_REFERENCE_TSC: > + case HV_X64_MSR_TIME_REF_COUNT: > r = true; > break; > } > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > if (__copy_to_user((void __user *)addr, instructions, 4)) > return 1; > kvm->arch.hv_hypercall = data; > + kvm->arch.hv_ref_count = get_kernel_ns(); > + break; > + } > + case HV_X64_MSR_REFERENCE_TSC: { > + u64 gfn; > + unsigned long addr; > + u32 tsc_ref; > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > + addr = gfn_to_hva(kvm, gfn); > + if (kvm_is_error_hva(addr)) > + return 1; > + tsc_ref = 0; > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) Does this do the right thing when we're migrating? How does usermode learn that the guest page has been dirtied? > + return 1; > + kvm->arch.hv_tsc_page = data; > break; > } > default: > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > case HV_X64_MSR_HYPERCALL: > data = kvm->arch.hv_hypercall; > break; > + case HV_X64_MSR_TIME_REF_COUNT: > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > + break; > + case HV_X64_MSR_REFERENCE_TSC: > + data = kvm->arch.hv_tsc_page; > + break; > default: > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > return 1; > -- > 1.8.1.2 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-13 at 16:30 -0700, Eric Northup wrote: > On Mon, May 13, 2013 at 4:45 AM, Vadim Rozenfeld <vrozenfe@redhat.com> wrote: > > Signed-off: Peter Lieven <pl@dlh.net> > > Signed-off: Gleb Natapov <gleb@redhat.com> > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > The following patch allows to activate Hyper-V > > reference time counter > > --- > > arch/x86/include/asm/kvm_host.h | 2 ++ > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 3741c65..f0fee35 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -575,6 +575,8 @@ struct kvm_arch { > > /* fields used by HYPER-V emulation */ > > u64 hv_guest_os_id; > > u64 hv_hypercall; > > + u64 hv_ref_count; > > + u64 hv_tsc_page; > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > int audit_point; > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > index b80420b..9711819 100644 > > --- a/arch/x86/include/uapi/asm/hyperv.h > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > @@ -136,6 +136,9 @@ > > /* MSR used to read the per-partition time reference counter */ > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > +/* A partition's reference time stamp counter (TSC) page */ > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > + > > /* Define the virtual APIC registers */ > > #define HV_X64_MSR_EOI 0x40000070 > > #define HV_X64_MSR_ICR 0x40000071 > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 094b5d9..1a4036d 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > static u32 msrs_to_save[] = { > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > MSR_KVM_PV_EOI_EN, > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > switch (msr) { > > case HV_X64_MSR_GUEST_OS_ID: > > case HV_X64_MSR_HYPERCALL: > > + case HV_X64_MSR_REFERENCE_TSC: > > + case HV_X64_MSR_TIME_REF_COUNT: > > r = true; > > break; > > } > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > return 1; > > kvm->arch.hv_hypercall = data; > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > + break; > > + } > > + case HV_X64_MSR_REFERENCE_TSC: { > > + u64 gfn; > > + unsigned long addr; > > + u32 tsc_ref; > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > + addr = gfn_to_hva(kvm, gfn); > > + if (kvm_is_error_hva(addr)) > > + return 1; > > + tsc_ref = 0; > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > Does this do the right thing when we're migrating? How does usermode > learn that the guest page has been dirtied? > No, it shouldn't be a problem for this patch. Guest allocates a page from nonpaged physical memory, maps it to the system address space, gets physical address and sends it to KVM. KVM sets the first DWORD (TscSequence) to zero, which means that guest will use reference time counter as a timestamp source even after migration. > > + return 1; > > + kvm->arch.hv_tsc_page = data; > > break; > > } > > default: > > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > case HV_X64_MSR_HYPERCALL: > > data = kvm->arch.hv_hypercall; > > break; > > + case HV_X64_MSR_TIME_REF_COUNT: > > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > > + break; > > + case HV_X64_MSR_REFERENCE_TSC: > > + data = kvm->arch.hv_tsc_page; > > + break; > > default: > > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > > return 1; > > -- > > 1.8.1.2 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13.05.2013 13:45, Vadim Rozenfeld wrote: > Signed-off: Peter Lieven <pl@dlh.net> > Signed-off: Gleb Natapov <gleb@redhat.com> > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > The following patch allows to activate Hyper-V > reference time counter > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 3741c65..f0fee35 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -575,6 +575,8 @@ struct kvm_arch { > /* fields used by HYPER-V emulation */ > u64 hv_guest_os_id; > u64 hv_hypercall; > + u64 hv_ref_count; > + u64 hv_tsc_page; > > #ifdef CONFIG_KVM_MMU_AUDIT > int audit_point; > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index b80420b..9711819 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -136,6 +136,9 @@ > /* MSR used to read the per-partition time reference counter */ > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > +/* A partition's reference time stamp counter (TSC) page */ > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > + > /* Define the virtual APIC registers */ > #define HV_X64_MSR_EOI 0x40000070 > #define HV_X64_MSR_ICR 0x40000071 > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 094b5d9..1a4036d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > static u32 msrs_to_save[] = { > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > MSR_KVM_PV_EOI_EN, > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > switch (msr) { > case HV_X64_MSR_GUEST_OS_ID: > case HV_X64_MSR_HYPERCALL: > + case HV_X64_MSR_REFERENCE_TSC: > + case HV_X64_MSR_TIME_REF_COUNT: > r = true; > break; > } > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > if (__copy_to_user((void __user *)addr, instructions, 4)) > return 1; > kvm->arch.hv_hypercall = data; > + kvm->arch.hv_ref_count = get_kernel_ns(); > + break; > + } > + case HV_X64_MSR_REFERENCE_TSC: { > + u64 gfn; > + unsigned long addr; > + u32 tsc_ref; > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > + addr = gfn_to_hva(kvm, gfn); > + if (kvm_is_error_hva(addr)) > + return 1; > + tsc_ref = 0; > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > + return 1; > + kvm->arch.hv_tsc_page = data; > break; > } > default: > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > case HV_X64_MSR_HYPERCALL: > data = kvm->arch.hv_hypercall; > break; > + case HV_X64_MSR_TIME_REF_COUNT: > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > + break; in an earlier version of this patch I have the following: + case HV_X64_MSR_TIME_REF_COUNT: { + u64 now_ns; + local_irq_disable(); + now_ns = get_kernel_ns(); + data = div_u64(now_ns + kvm->arch.kvmclock_offset - kvm->arch.hv_ref_count,100); + local_irq_enable(); + break; + } I do not know if this is right, but I can report that this one is working without any flaws since approx. 1.5 years. Peter -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-05-14 at 16:14 +0200, Peter Lieven wrote: > On 13.05.2013 13:45, Vadim Rozenfeld wrote: > > Signed-off: Peter Lieven <pl@dlh.net> > > Signed-off: Gleb Natapov <gleb@redhat.com> > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > The following patch allows to activate Hyper-V > > reference time counter > > --- > > arch/x86/include/asm/kvm_host.h | 2 ++ > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 3741c65..f0fee35 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -575,6 +575,8 @@ struct kvm_arch { > > /* fields used by HYPER-V emulation */ > > u64 hv_guest_os_id; > > u64 hv_hypercall; > > + u64 hv_ref_count; > > + u64 hv_tsc_page; > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > int audit_point; > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > index b80420b..9711819 100644 > > --- a/arch/x86/include/uapi/asm/hyperv.h > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > @@ -136,6 +136,9 @@ > > /* MSR used to read the per-partition time reference counter */ > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > +/* A partition's reference time stamp counter (TSC) page */ > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > + > > /* Define the virtual APIC registers */ > > #define HV_X64_MSR_EOI 0x40000070 > > #define HV_X64_MSR_ICR 0x40000071 > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 094b5d9..1a4036d 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > static u32 msrs_to_save[] = { > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > MSR_KVM_PV_EOI_EN, > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > switch (msr) { > > case HV_X64_MSR_GUEST_OS_ID: > > case HV_X64_MSR_HYPERCALL: > > + case HV_X64_MSR_REFERENCE_TSC: > > + case HV_X64_MSR_TIME_REF_COUNT: > > r = true; > > break; > > } > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > return 1; > > kvm->arch.hv_hypercall = data; > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > + break; > > + } > > + case HV_X64_MSR_REFERENCE_TSC: { > > + u64 gfn; > > + unsigned long addr; > > + u32 tsc_ref; > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > + addr = gfn_to_hva(kvm, gfn); > > + if (kvm_is_error_hva(addr)) > > + return 1; > > + tsc_ref = 0; > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > + return 1; > > + kvm->arch.hv_tsc_page = data; > > break; > > } > > default: > > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > case HV_X64_MSR_HYPERCALL: > > data = kvm->arch.hv_hypercall; > > break; > > + case HV_X64_MSR_TIME_REF_COUNT: > > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > > + break; > > > in an earlier version of this patch I have the following: > > + case HV_X64_MSR_TIME_REF_COUNT: { > + u64 now_ns; > + local_irq_disable(); > + now_ns = get_kernel_ns(); > + data = div_u64(now_ns + kvm->arch.kvmclock_offset - kvm->arch.hv_ref_count,100); > + local_irq_enable(); > + break; > + } > > I do not know if this is right, but I can report that this one is working without any flaws > since approx. 1.5 years. Hi Peter, I created this patch based on the original code posted in thread http://marc.info/?l=kvm&m=133278705514826 But please feel free to send your version, if you see any problem in the current code. Best regards, Vadim. > Peter -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 14, 2013 at 04:14:11PM +0200, Peter Lieven wrote: > On 13.05.2013 13:45, Vadim Rozenfeld wrote: > >Signed-off: Peter Lieven <pl@dlh.net> > >Signed-off: Gleb Natapov <gleb@redhat.com> > >Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > >The following patch allows to activate Hyper-V > >reference time counter > >--- > > arch/x86/include/asm/kvm_host.h | 2 ++ > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > >index 3741c65..f0fee35 100644 > >--- a/arch/x86/include/asm/kvm_host.h > >+++ b/arch/x86/include/asm/kvm_host.h > >@@ -575,6 +575,8 @@ struct kvm_arch { > > /* fields used by HYPER-V emulation */ > > u64 hv_guest_os_id; > > u64 hv_hypercall; > >+ u64 hv_ref_count; > >+ u64 hv_tsc_page; > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > int audit_point; > >diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > >index b80420b..9711819 100644 > >--- a/arch/x86/include/uapi/asm/hyperv.h > >+++ b/arch/x86/include/uapi/asm/hyperv.h > >@@ -136,6 +136,9 @@ > > /* MSR used to read the per-partition time reference counter */ > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > >+/* A partition's reference time stamp counter (TSC) page */ > >+#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > >+ > > /* Define the virtual APIC registers */ > > #define HV_X64_MSR_EOI 0x40000070 > > #define HV_X64_MSR_ICR 0x40000071 > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >index 094b5d9..1a4036d 100644 > >--- a/arch/x86/kvm/x86.c > >+++ b/arch/x86/kvm/x86.c > >@@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > static u32 msrs_to_save[] = { > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > >- HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > >+ HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > MSR_KVM_PV_EOI_EN, > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > >@@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > switch (msr) { > > case HV_X64_MSR_GUEST_OS_ID: > > case HV_X64_MSR_HYPERCALL: > >+ case HV_X64_MSR_REFERENCE_TSC: > >+ case HV_X64_MSR_TIME_REF_COUNT: > > r = true; > > break; > > } > >@@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > return 1; > > kvm->arch.hv_hypercall = data; > >+ kvm->arch.hv_ref_count = get_kernel_ns(); > >+ break; > >+ } > >+ case HV_X64_MSR_REFERENCE_TSC: { > >+ u64 gfn; > >+ unsigned long addr; > >+ u32 tsc_ref; > >+ gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > >+ addr = gfn_to_hva(kvm, gfn); > >+ if (kvm_is_error_hva(addr)) > >+ return 1; > >+ tsc_ref = 0; > >+ if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > >+ return 1; > >+ kvm->arch.hv_tsc_page = data; > > break; > > } > > default: > >@@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > case HV_X64_MSR_HYPERCALL: > > data = kvm->arch.hv_hypercall; > > break; > >+ case HV_X64_MSR_TIME_REF_COUNT: > >+ data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > >+ break; > > > in an earlier version of this patch I have the following: > > + case HV_X64_MSR_TIME_REF_COUNT: { > + u64 now_ns; > + local_irq_disable(); > + now_ns = get_kernel_ns(); > + data = div_u64(now_ns + kvm->arch.kvmclock_offset - kvm->arch.hv_ref_count,100); > + local_irq_enable(); > + break; > + } > > I do not know if this is right, but I can report that this one is working without any flaws > since approx. 1.5 years. > get_kernel_ns() should be called at least with preemption disabled and all other callers disable irq around it too, also without kvmclock_offset the counter will be incorrect after migration. Marcelo, is kvmclock_offset guaranties migration correctness or more is required? -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 14, 2013 at 07:46:36PM +1000, Vadim Rozenfeld wrote: > On Mon, 2013-05-13 at 16:30 -0700, Eric Northup wrote: > > On Mon, May 13, 2013 at 4:45 AM, Vadim Rozenfeld <vrozenfe@redhat.com> wrote: > > > Signed-off: Peter Lieven <pl@dlh.net> > > > Signed-off: Gleb Natapov <gleb@redhat.com> > > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > > > The following patch allows to activate Hyper-V > > > reference time counter > > > --- > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index 3741c65..f0fee35 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -575,6 +575,8 @@ struct kvm_arch { > > > /* fields used by HYPER-V emulation */ > > > u64 hv_guest_os_id; > > > u64 hv_hypercall; > > > + u64 hv_ref_count; > > > + u64 hv_tsc_page; > > > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > > int audit_point; > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > > index b80420b..9711819 100644 > > > --- a/arch/x86/include/uapi/asm/hyperv.h > > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > > @@ -136,6 +136,9 @@ > > > /* MSR used to read the per-partition time reference counter */ > > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > > > +/* A partition's reference time stamp counter (TSC) page */ > > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > > + > > > /* Define the virtual APIC registers */ > > > #define HV_X64_MSR_EOI 0x40000070 > > > #define HV_X64_MSR_ICR 0x40000071 > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 094b5d9..1a4036d 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > > static u32 msrs_to_save[] = { > > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > > MSR_KVM_PV_EOI_EN, > > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > > switch (msr) { > > > case HV_X64_MSR_GUEST_OS_ID: > > > case HV_X64_MSR_HYPERCALL: > > > + case HV_X64_MSR_REFERENCE_TSC: > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > r = true; > > > break; > > > } > > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > > return 1; > > > kvm->arch.hv_hypercall = data; > > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > > + break; > > > + } > > > + case HV_X64_MSR_REFERENCE_TSC: { > > > + u64 gfn; > > > + unsigned long addr; > > > + u32 tsc_ref; > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > > + addr = gfn_to_hva(kvm, gfn); > > > + if (kvm_is_error_hva(addr)) > > > + return 1; > > > + tsc_ref = 0; > > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > > > Does this do the right thing when we're migrating? How does usermode > > learn that the guest page has been dirtied? > > > > No, it shouldn't be a problem for this patch. Guest allocates a page > from nonpaged physical memory, maps it to the system address space, gets > physical address and sends it to KVM. KVM sets the first DWORD > (TscSequence) to zero, which means that guest will use reference time > counter as a timestamp source even after migration. > Eric is right, we need mark_page_dirty() here and in HV_X64_MSR_HYPERCALL too. Without it QEMU will not know that content of the page has changed and will not migrate it. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: > Signed-off: Peter Lieven <pl@dlh.net> > Signed-off: Gleb Natapov <gleb@redhat.com> > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > The following patch allows to activate Hyper-V > reference time counter > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 3741c65..f0fee35 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -575,6 +575,8 @@ struct kvm_arch { > /* fields used by HYPER-V emulation */ > u64 hv_guest_os_id; > u64 hv_hypercall; > + u64 hv_ref_count; > + u64 hv_tsc_page; > > #ifdef CONFIG_KVM_MMU_AUDIT > int audit_point; > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index b80420b..9711819 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -136,6 +136,9 @@ > /* MSR used to read the per-partition time reference counter */ > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > +/* A partition's reference time stamp counter (TSC) page */ > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > + > /* Define the virtual APIC registers */ > #define HV_X64_MSR_EOI 0x40000070 > #define HV_X64_MSR_ICR 0x40000071 > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 094b5d9..1a4036d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > static u32 msrs_to_save[] = { > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > MSR_KVM_PV_EOI_EN, > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > switch (msr) { > case HV_X64_MSR_GUEST_OS_ID: > case HV_X64_MSR_HYPERCALL: > + case HV_X64_MSR_REFERENCE_TSC: > + case HV_X64_MSR_TIME_REF_COUNT: > r = true; > break; > } > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > if (__copy_to_user((void __user *)addr, instructions, 4)) > return 1; > kvm->arch.hv_hypercall = data; > + kvm->arch.hv_ref_count = get_kernel_ns(); > + break; > + } > + case HV_X64_MSR_REFERENCE_TSC: { > + u64 gfn; > + unsigned long addr; > + u32 tsc_ref; > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? > + addr = gfn_to_hva(kvm, gfn); > + if (kvm_is_error_hva(addr)) > + return 1; > + tsc_ref = 0; > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > + return 1; > + kvm->arch.hv_tsc_page = data; > break; > } > default: > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > case HV_X64_MSR_HYPERCALL: > data = kvm->arch.hv_hypercall; > break; > + case HV_X64_MSR_TIME_REF_COUNT: > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > + break; > + case HV_X64_MSR_REFERENCE_TSC: > + data = kvm->arch.hv_tsc_page; > + break; > default: > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > return 1; > -- > 1.8.1.2 -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 11:18 +0300, Gleb Natapov wrote: > On Tue, May 14, 2013 at 07:46:36PM +1000, Vadim Rozenfeld wrote: > > On Mon, 2013-05-13 at 16:30 -0700, Eric Northup wrote: > > > On Mon, May 13, 2013 at 4:45 AM, Vadim Rozenfeld <vrozenfe@redhat.com> wrote: > > > > Signed-off: Peter Lieven <pl@dlh.net> > > > > Signed-off: Gleb Natapov <gleb@redhat.com> > > > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > > > > > The following patch allows to activate Hyper-V > > > > reference time counter > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index 3741c65..f0fee35 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -575,6 +575,8 @@ struct kvm_arch { > > > > /* fields used by HYPER-V emulation */ > > > > u64 hv_guest_os_id; > > > > u64 hv_hypercall; > > > > + u64 hv_ref_count; > > > > + u64 hv_tsc_page; > > > > > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > > > int audit_point; > > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > > > index b80420b..9711819 100644 > > > > --- a/arch/x86/include/uapi/asm/hyperv.h > > > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > > > @@ -136,6 +136,9 @@ > > > > /* MSR used to read the per-partition time reference counter */ > > > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > > > > > +/* A partition's reference time stamp counter (TSC) page */ > > > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > > > + > > > > /* Define the virtual APIC registers */ > > > > #define HV_X64_MSR_EOI 0x40000070 > > > > #define HV_X64_MSR_ICR 0x40000071 > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index 094b5d9..1a4036d 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > > > static u32 msrs_to_save[] = { > > > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > > > MSR_KVM_PV_EOI_EN, > > > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > > > switch (msr) { > > > > case HV_X64_MSR_GUEST_OS_ID: > > > > case HV_X64_MSR_HYPERCALL: > > > > + case HV_X64_MSR_REFERENCE_TSC: > > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > > r = true; > > > > break; > > > > } > > > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > > > return 1; > > > > kvm->arch.hv_hypercall = data; > > > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > > > + break; > > > > + } > > > > + case HV_X64_MSR_REFERENCE_TSC: { > > > > + u64 gfn; > > > > + unsigned long addr; > > > > + u32 tsc_ref; > > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > > > + addr = gfn_to_hva(kvm, gfn); > > > > + if (kvm_is_error_hva(addr)) > > > > + return 1; > > > > + tsc_ref = 0; > > > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > > > > > Does this do the right thing when we're migrating? How does usermode > > > learn that the guest page has been dirtied? > > > > > > > No, it shouldn't be a problem for this patch. Guest allocates a page > > from nonpaged physical memory, maps it to the system address space, gets > > physical address and sends it to KVM. KVM sets the first DWORD > > (TscSequence) to zero, which means that guest will use reference time > > counter as a timestamp source even after migration. > > > Eric is right, we need mark_page_dirty() here and in HV_X64_MSR_HYPERCALL > too. Without it QEMU will not know that content of the page has changed > and will not migrate it. OK. Will fix it. > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 11:34 +0300, Gleb Natapov wrote: > On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: > > Signed-off: Peter Lieven <pl@dlh.net> > > Signed-off: Gleb Natapov <gleb@redhat.com> > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > The following patch allows to activate Hyper-V > > reference time counter > > --- > > arch/x86/include/asm/kvm_host.h | 2 ++ > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > index 3741c65..f0fee35 100644 > > --- a/arch/x86/include/asm/kvm_host.h > > +++ b/arch/x86/include/asm/kvm_host.h > > @@ -575,6 +575,8 @@ struct kvm_arch { > > /* fields used by HYPER-V emulation */ > > u64 hv_guest_os_id; > > u64 hv_hypercall; > > + u64 hv_ref_count; > > + u64 hv_tsc_page; > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > int audit_point; > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > index b80420b..9711819 100644 > > --- a/arch/x86/include/uapi/asm/hyperv.h > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > @@ -136,6 +136,9 @@ > > /* MSR used to read the per-partition time reference counter */ > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > +/* A partition's reference time stamp counter (TSC) page */ > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > + > > /* Define the virtual APIC registers */ > > #define HV_X64_MSR_EOI 0x40000070 > > #define HV_X64_MSR_ICR 0x40000071 > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 094b5d9..1a4036d 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > static u32 msrs_to_save[] = { > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > MSR_KVM_PV_EOI_EN, > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > switch (msr) { > > case HV_X64_MSR_GUEST_OS_ID: > > case HV_X64_MSR_HYPERCALL: > > + case HV_X64_MSR_REFERENCE_TSC: > > + case HV_X64_MSR_TIME_REF_COUNT: > > r = true; > > break; > > } > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > return 1; > > kvm->arch.hv_hypercall = data; > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > + break; > > + } > > + case HV_X64_MSR_REFERENCE_TSC: { > > + u64 gfn; > > + unsigned long addr; > > + u32 tsc_ref; > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? Yes, I have this check added in the second patch. > > > + addr = gfn_to_hva(kvm, gfn); > > + if (kvm_is_error_hva(addr)) > > + return 1; > > + tsc_ref = 0; > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > + return 1; > > + kvm->arch.hv_tsc_page = data; > > break; > > } > > default: > > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > case HV_X64_MSR_HYPERCALL: > > data = kvm->arch.hv_hypercall; > > break; > > + case HV_X64_MSR_TIME_REF_COUNT: > > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > > + break; > > + case HV_X64_MSR_REFERENCE_TSC: > > + data = kvm->arch.hv_tsc_page; > > + break; > > default: > > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > > return 1; > > -- > > 1.8.1.2 > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 16, 2013 at 07:13:41PM +1000, Vadim Rozenfeld wrote: > On Thu, 2013-05-16 at 11:34 +0300, Gleb Natapov wrote: > > On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: > > > Signed-off: Peter Lieven <pl@dlh.net> > > > Signed-off: Gleb Natapov <gleb@redhat.com> > > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > > > The following patch allows to activate Hyper-V > > > reference time counter > > > --- > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > index 3741c65..f0fee35 100644 > > > --- a/arch/x86/include/asm/kvm_host.h > > > +++ b/arch/x86/include/asm/kvm_host.h > > > @@ -575,6 +575,8 @@ struct kvm_arch { > > > /* fields used by HYPER-V emulation */ > > > u64 hv_guest_os_id; > > > u64 hv_hypercall; > > > + u64 hv_ref_count; > > > + u64 hv_tsc_page; > > > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > > int audit_point; > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > > index b80420b..9711819 100644 > > > --- a/arch/x86/include/uapi/asm/hyperv.h > > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > > @@ -136,6 +136,9 @@ > > > /* MSR used to read the per-partition time reference counter */ > > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > > > +/* A partition's reference time stamp counter (TSC) page */ > > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > > + > > > /* Define the virtual APIC registers */ > > > #define HV_X64_MSR_EOI 0x40000070 > > > #define HV_X64_MSR_ICR 0x40000071 > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 094b5d9..1a4036d 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > > static u32 msrs_to_save[] = { > > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > > MSR_KVM_PV_EOI_EN, > > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > > switch (msr) { > > > case HV_X64_MSR_GUEST_OS_ID: > > > case HV_X64_MSR_HYPERCALL: > > > + case HV_X64_MSR_REFERENCE_TSC: > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > r = true; > > > break; > > > } > > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > > return 1; > > > kvm->arch.hv_hypercall = data; > > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > > + break; > > > + } > > > + case HV_X64_MSR_REFERENCE_TSC: { > > > + u64 gfn; > > > + unsigned long addr; > > > + u32 tsc_ref; > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? > > Yes, I have this check added in the second patch. > Move it here please. > > > > > + addr = gfn_to_hva(kvm, gfn); > > > + if (kvm_is_error_hva(addr)) > > > + return 1; > > > + tsc_ref = 0; > > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > > + return 1; > > > + kvm->arch.hv_tsc_page = data; > > > break; > > > } > > > default: > > > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > > case HV_X64_MSR_HYPERCALL: > > > data = kvm->arch.hv_hypercall; > > > break; > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > > > + break; > > > + case HV_X64_MSR_REFERENCE_TSC: > > > + data = kvm->arch.hv_tsc_page; > > > + break; > > > default: > > > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > > > return 1; > > > -- > > > 1.8.1.2 > > > > -- > > Gleb. > -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 12:21 +0300, Gleb Natapov wrote: > On Thu, May 16, 2013 at 07:13:41PM +1000, Vadim Rozenfeld wrote: > > On Thu, 2013-05-16 at 11:34 +0300, Gleb Natapov wrote: > > > On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: > > > > Signed-off: Peter Lieven <pl@dlh.net> > > > > Signed-off: Gleb Natapov <gleb@redhat.com> > > > > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > > > > > > > The following patch allows to activate Hyper-V > > > > reference time counter > > > > --- > > > > arch/x86/include/asm/kvm_host.h | 2 ++ > > > > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > > > > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > > > > 3 files changed, 29 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > > > > index 3741c65..f0fee35 100644 > > > > --- a/arch/x86/include/asm/kvm_host.h > > > > +++ b/arch/x86/include/asm/kvm_host.h > > > > @@ -575,6 +575,8 @@ struct kvm_arch { > > > > /* fields used by HYPER-V emulation */ > > > > u64 hv_guest_os_id; > > > > u64 hv_hypercall; > > > > + u64 hv_ref_count; > > > > + u64 hv_tsc_page; > > > > > > > > #ifdef CONFIG_KVM_MMU_AUDIT > > > > int audit_point; > > > > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > > > > index b80420b..9711819 100644 > > > > --- a/arch/x86/include/uapi/asm/hyperv.h > > > > +++ b/arch/x86/include/uapi/asm/hyperv.h > > > > @@ -136,6 +136,9 @@ > > > > /* MSR used to read the per-partition time reference counter */ > > > > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > > > > > > > +/* A partition's reference time stamp counter (TSC) page */ > > > > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > > > > + > > > > /* Define the virtual APIC registers */ > > > > #define HV_X64_MSR_EOI 0x40000070 > > > > #define HV_X64_MSR_ICR 0x40000071 > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > > index 094b5d9..1a4036d 100644 > > > > --- a/arch/x86/kvm/x86.c > > > > +++ b/arch/x86/kvm/x86.c > > > > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > > > > static u32 msrs_to_save[] = { > > > > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > > > > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > > > > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > > > > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > > > > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > > > > MSR_KVM_PV_EOI_EN, > > > > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > > > > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > > > > switch (msr) { > > > > case HV_X64_MSR_GUEST_OS_ID: > > > > case HV_X64_MSR_HYPERCALL: > > > > + case HV_X64_MSR_REFERENCE_TSC: > > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > > r = true; > > > > break; > > > > } > > > > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > > > if (__copy_to_user((void __user *)addr, instructions, 4)) > > > > return 1; > > > > kvm->arch.hv_hypercall = data; > > > > + kvm->arch.hv_ref_count = get_kernel_ns(); > > > > + break; > > > > + } > > > > + case HV_X64_MSR_REFERENCE_TSC: { > > > > + u64 gfn; > > > > + unsigned long addr; > > > > + u32 tsc_ref; > > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > > > Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? > > > > Yes, I have this check added in the second patch. > > > Move it here please. OK, will do it. > > > > > > > > + addr = gfn_to_hva(kvm, gfn); > > > > + if (kvm_is_error_hva(addr)) > > > > + return 1; > > > > + tsc_ref = 0; > > > > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > > > > + return 1; > > > > + kvm->arch.hv_tsc_page = data; > > > > break; > > > > } > > > > default: > > > > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > > > > case HV_X64_MSR_HYPERCALL: > > > > data = kvm->arch.hv_hypercall; > > > > break; > > > > + case HV_X64_MSR_TIME_REF_COUNT: > > > > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > > > > + break; > > > > + case HV_X64_MSR_REFERENCE_TSC: > > > > + data = kvm->arch.hv_tsc_page; > > > > + break; > > > > default: > > > > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > > > > return 1; > > > > -- > > > > 1.8.1.2 > > > > > > -- > > > Gleb. > > > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/05/2013 11:28, Vadim Rozenfeld ha scritto: >>>>> > > > > + case HV_X64_MSR_REFERENCE_TSC: { >>>>> > > > > + u64 gfn; >>>>> > > > > + unsigned long addr; >>>>> > > > > + u32 tsc_ref; >>>>> > > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; >>>> > > > Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? >>> > > >>> > > Yes, I have this check added in the second patch. >>> > > >> > Move it here please. > OK, will do it. >> > >>>> > > > >>>>> > > > > + addr = gfn_to_hva(kvm, gfn); >>>>> > > > > + if (kvm_is_error_hva(addr)) >>>>> > > > > + return 1; >>>>> > > > > + tsc_ref = 0; This should write 0xFFFFFFFF. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/05/2013 11:28, Vadim Rozenfeld ha scritto: > On Thu, 2013-05-16 at 12:21 +0300, Gleb Natapov wrote: >> On Thu, May 16, 2013 at 07:13:41PM +1000, Vadim Rozenfeld wrote: >>> On Thu, 2013-05-16 at 11:34 +0300, Gleb Natapov wrote: >>>> On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: >>>>> Signed-off: Peter Lieven <pl@dlh.net> >>>>> Signed-off: Gleb Natapov <gleb@redhat.com> >>>>> Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> >>>>> >>>>> The following patch allows to activate Hyper-V >>>>> reference time counter >>>>> --- >>>>> arch/x86/include/asm/kvm_host.h | 2 ++ >>>>> arch/x86/include/uapi/asm/hyperv.h | 3 +++ >>>>> arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- >>>>> 3 files changed, 29 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >>>>> index 3741c65..f0fee35 100644 >>>>> --- a/arch/x86/include/asm/kvm_host.h >>>>> +++ b/arch/x86/include/asm/kvm_host.h >>>>> @@ -575,6 +575,8 @@ struct kvm_arch { >>>>> /* fields used by HYPER-V emulation */ >>>>> u64 hv_guest_os_id; >>>>> u64 hv_hypercall; >>>>> + u64 hv_ref_count; >>>>> + u64 hv_tsc_page; >>>>> >>>>> #ifdef CONFIG_KVM_MMU_AUDIT >>>>> int audit_point; >>>>> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h >>>>> index b80420b..9711819 100644 >>>>> --- a/arch/x86/include/uapi/asm/hyperv.h >>>>> +++ b/arch/x86/include/uapi/asm/hyperv.h >>>>> @@ -136,6 +136,9 @@ >>>>> /* MSR used to read the per-partition time reference counter */ >>>>> #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 >>>>> >>>>> +/* A partition's reference time stamp counter (TSC) page */ >>>>> +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 >>>>> + >>>>> /* Define the virtual APIC registers */ >>>>> #define HV_X64_MSR_EOI 0x40000070 >>>>> #define HV_X64_MSR_ICR 0x40000071 >>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>> index 094b5d9..1a4036d 100644 >>>>> --- a/arch/x86/kvm/x86.c >>>>> +++ b/arch/x86/kvm/x86.c >>>>> @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); >>>>> static u32 msrs_to_save[] = { >>>>> MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, >>>>> MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, >>>>> - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, >>>>> + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, >>>>> HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, >>>>> MSR_KVM_PV_EOI_EN, >>>>> MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, >>>>> @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) >>>>> switch (msr) { >>>>> case HV_X64_MSR_GUEST_OS_ID: >>>>> case HV_X64_MSR_HYPERCALL: >>>>> + case HV_X64_MSR_REFERENCE_TSC: >>>>> + case HV_X64_MSR_TIME_REF_COUNT: >>>>> r = true; >>>>> break; >>>>> } >>>>> @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) >>>>> if (__copy_to_user((void __user *)addr, instructions, 4)) >>>>> return 1; >>>>> kvm->arch.hv_hypercall = data; >>>>> + kvm->arch.hv_ref_count = get_kernel_ns(); Please rename to kvm->arch.hv_ref_count_base. >>>>> + break; >>>>> + } >>>>> + case HV_X64_MSR_REFERENCE_TSC: { >>>>> + u64 gfn; >>>>> + unsigned long addr; >>>>> + u32 tsc_ref; >>>>> + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; >>>> Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? >>> >>> Yes, I have this check added in the second patch. >>> >> Move it here please. > OK, will do it. Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this patch, and leave it all to the second. Paolo >>>> >>>>> + addr = gfn_to_hva(kvm, gfn); >>>>> + if (kvm_is_error_hva(addr)) >>>>> + return 1; >>>>> + tsc_ref = 0; >>>>> + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) >>>>> + return 1; >>>>> + kvm->arch.hv_tsc_page = data; >>>>> break; >>>>> } >>>>> default: >>>>> @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) >>>>> case HV_X64_MSR_HYPERCALL: >>>>> data = kvm->arch.hv_hypercall; >>>>> break; >>>>> + case HV_X64_MSR_TIME_REF_COUNT: >>>>> + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); >>>>> + break; >>>>> + case HV_X64_MSR_REFERENCE_TSC: >>>>> + data = kvm->arch.hv_tsc_page; >>>>> + break; >>>>> default: >>>>> vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); >>>>> return 1; >>>>> -- >>>>> 1.8.1.2 >>>> >>>> -- >>>> Gleb. >>> >> >> -- >> Gleb. > > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 15:37 +0200, Paolo Bonzini wrote: > Il 16/05/2013 11:28, Vadim Rozenfeld ha scritto: > >>>>> > > > > + case HV_X64_MSR_REFERENCE_TSC: { > >>>>> > > > > + u64 gfn; > >>>>> > > > > + unsigned long addr; > >>>>> > > > > + u32 tsc_ref; > >>>>> > > > > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > >>>> > > > Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? > >>> > > > >>> > > Yes, I have this check added in the second patch. > >>> > > > >> > Move it here please. > > OK, will do it. > >> > > >>>> > > > > >>>>> > > > > + addr = gfn_to_hva(kvm, gfn); > >>>>> > > > > + if (kvm_is_error_hva(addr)) > >>>>> > > > > + return 1; > >>>>> > > > > + tsc_ref = 0; > > This should write 0xFFFFFFFF. This should write 0 Vadim. > > Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 15:44 +0200, Paolo Bonzini wrote: > Il 16/05/2013 11:28, Vadim Rozenfeld ha scritto: > > On Thu, 2013-05-16 at 12:21 +0300, Gleb Natapov wrote: > >> On Thu, May 16, 2013 at 07:13:41PM +1000, Vadim Rozenfeld wrote: > >>> On Thu, 2013-05-16 at 11:34 +0300, Gleb Natapov wrote: > >>>> On Mon, May 13, 2013 at 09:45:16PM +1000, Vadim Rozenfeld wrote: > >>>>> Signed-off: Peter Lieven <pl@dlh.net> > >>>>> Signed-off: Gleb Natapov <gleb@redhat.com> > >>>>> Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > >>>>> > >>>>> The following patch allows to activate Hyper-V > >>>>> reference time counter > >>>>> --- > >>>>> arch/x86/include/asm/kvm_host.h | 2 ++ > >>>>> arch/x86/include/uapi/asm/hyperv.h | 3 +++ > >>>>> arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > >>>>> 3 files changed, 29 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > >>>>> index 3741c65..f0fee35 100644 > >>>>> --- a/arch/x86/include/asm/kvm_host.h > >>>>> +++ b/arch/x86/include/asm/kvm_host.h > >>>>> @@ -575,6 +575,8 @@ struct kvm_arch { > >>>>> /* fields used by HYPER-V emulation */ > >>>>> u64 hv_guest_os_id; > >>>>> u64 hv_hypercall; > >>>>> + u64 hv_ref_count; > >>>>> + u64 hv_tsc_page; > >>>>> > >>>>> #ifdef CONFIG_KVM_MMU_AUDIT > >>>>> int audit_point; > >>>>> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > >>>>> index b80420b..9711819 100644 > >>>>> --- a/arch/x86/include/uapi/asm/hyperv.h > >>>>> +++ b/arch/x86/include/uapi/asm/hyperv.h > >>>>> @@ -136,6 +136,9 @@ > >>>>> /* MSR used to read the per-partition time reference counter */ > >>>>> #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > >>>>> > >>>>> +/* A partition's reference time stamp counter (TSC) page */ > >>>>> +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > >>>>> + > >>>>> /* Define the virtual APIC registers */ > >>>>> #define HV_X64_MSR_EOI 0x40000070 > >>>>> #define HV_X64_MSR_ICR 0x40000071 > >>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >>>>> index 094b5d9..1a4036d 100644 > >>>>> --- a/arch/x86/kvm/x86.c > >>>>> +++ b/arch/x86/kvm/x86.c > >>>>> @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > >>>>> static u32 msrs_to_save[] = { > >>>>> MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > >>>>> MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > >>>>> - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > >>>>> + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > >>>>> HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > >>>>> MSR_KVM_PV_EOI_EN, > >>>>> MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > >>>>> @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > >>>>> switch (msr) { > >>>>> case HV_X64_MSR_GUEST_OS_ID: > >>>>> case HV_X64_MSR_HYPERCALL: > >>>>> + case HV_X64_MSR_REFERENCE_TSC: > >>>>> + case HV_X64_MSR_TIME_REF_COUNT: > >>>>> r = true; > >>>>> break; > >>>>> } > >>>>> @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > >>>>> if (__copy_to_user((void __user *)addr, instructions, 4)) > >>>>> return 1; > >>>>> kvm->arch.hv_hypercall = data; > >>>>> + kvm->arch.hv_ref_count = get_kernel_ns(); > > Please rename to kvm->arch.hv_ref_count_base. > > >>>>> + break; > >>>>> + } > >>>>> + case HV_X64_MSR_REFERENCE_TSC: { > >>>>> + u64 gfn; > >>>>> + unsigned long addr; > >>>>> + u32 tsc_ref; > >>>>> + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > >>>> Shouldn't you check HV_X64_MSR_TSC_REFERENCE_ENABLE here? > >>> > >>> Yes, I have this check added in the second patch. > >>> > >> Move it here please. > > OK, will do it. > > Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > patch, and leave it all to the second. > What for? Could you please elaborate? Vadim. > Paolo > > >>>> > >>>>> + addr = gfn_to_hva(kvm, gfn); > >>>>> + if (kvm_is_error_hva(addr)) > >>>>> + return 1; > >>>>> + tsc_ref = 0; > >>>>> + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > >>>>> + return 1; > >>>>> + kvm->arch.hv_tsc_page = data; > >>>>> break; > >>>>> } > >>>>> default: > >>>>> @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > >>>>> case HV_X64_MSR_HYPERCALL: > >>>>> data = kvm->arch.hv_hypercall; > >>>>> break; > >>>>> + case HV_X64_MSR_TIME_REF_COUNT: > >>>>> + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > >>>>> + break; > >>>>> + case HV_X64_MSR_REFERENCE_TSC: > >>>>> + data = kvm->arch.hv_tsc_page; > >>>>> + break; > >>>>> default: > >>>>> vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > >>>>> return 1; > >>>>> -- > >>>>> 1.8.1.2 > >>>> > >>>> -- > >>>> Gleb. > >>> > >> > >> -- > >> Gleb. > > > > > > -- > > To unsubscribe from this list: send the line "unsubscribe kvm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 13/05/2013 13:45, Vadim Rozenfeld ha scritto: > Signed-off: Peter Lieven <pl@dlh.net> > Signed-off: Gleb Natapov <gleb@redhat.com> > Signed-off: Vadim Rozenfeld <vrozenfe@redhat.com> > > The following patch allows to activate Hyper-V > reference time counter > --- > arch/x86/include/asm/kvm_host.h | 2 ++ > arch/x86/include/uapi/asm/hyperv.h | 3 +++ > arch/x86/kvm/x86.c | 25 ++++++++++++++++++++++++- > 3 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 3741c65..f0fee35 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -575,6 +575,8 @@ struct kvm_arch { > /* fields used by HYPER-V emulation */ > u64 hv_guest_os_id; > u64 hv_hypercall; > + u64 hv_ref_count; > + u64 hv_tsc_page; > > #ifdef CONFIG_KVM_MMU_AUDIT > int audit_point; > diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h > index b80420b..9711819 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -136,6 +136,9 @@ > /* MSR used to read the per-partition time reference counter */ > #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 > > +/* A partition's reference time stamp counter (TSC) page */ > +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 > + > /* Define the virtual APIC registers */ > #define HV_X64_MSR_EOI 0x40000070 > #define HV_X64_MSR_ICR 0x40000071 > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 094b5d9..1a4036d 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); > static u32 msrs_to_save[] = { > MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, > MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, > - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, > + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, > HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, > MSR_KVM_PV_EOI_EN, > MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, > @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) > switch (msr) { > case HV_X64_MSR_GUEST_OS_ID: > case HV_X64_MSR_HYPERCALL: > + case HV_X64_MSR_REFERENCE_TSC: > + case HV_X64_MSR_TIME_REF_COUNT: > r = true; > break; > } > @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) > if (__copy_to_user((void __user *)addr, instructions, 4)) > return 1; > kvm->arch.hv_hypercall = data; > + kvm->arch.hv_ref_count = get_kernel_ns(); > + break; > + } > + case HV_X64_MSR_REFERENCE_TSC: { > + u64 gfn; > + unsigned long addr; > + u32 tsc_ref; > + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; > + addr = gfn_to_hva(kvm, gfn); > + if (kvm_is_error_hva(addr)) > + return 1; > + tsc_ref = 0; > + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) > + return 1; > + kvm->arch.hv_tsc_page = data; > break; > } > default: > @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) > case HV_X64_MSR_HYPERCALL: > data = kvm->arch.hv_hypercall; > break; > + case HV_X64_MSR_TIME_REF_COUNT: > + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); > + break; For migration, this should probably be writable from userspace (msr_info->host_initiated in kvm_set_msr_common). However, the spec says that the reference counter must count up from 0 _when the partition is created_, not when you initialize the enlightenments. Perhaps we can just piggy back on the kvmclock ioctls and remove hv_ref_count altogether. Then userspace doesn't need to write to HV_X64_MSR_TIME_REF_COUNT. Paolo > + case HV_X64_MSR_REFERENCE_TSC: > + data = kvm->arch.hv_tsc_page; > + break; > default: > vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); > return 1; > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: >>>>> > >>> >>>>> > >>> Yes, I have this check added in the second patch. >>>>> > >>> >>>> > >> Move it here please. >>> > > OK, will do it. >> > >> > Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this >> > patch, and leave it all to the second. >> > > What for? Could you please elaborate? To make code reviewable. Add one MSR here, the other in the second patch. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 16/05/2013 16:22, Vadim Rozenfeld ha scritto: >>>>>>>>>>> > >>>>> > > > > + addr = gfn_to_hva(kvm, gfn); >>>>>>>>>>> > >>>>> > > > > + if (kvm_is_error_hva(addr)) >>>>>>>>>>> > >>>>> > > > > + return 1; >>>>>>>>>>> > >>>>> > > > > + tsc_ref = 0; >> > >> > This should write 0xFFFFFFFF. > This should write 0 No. You have not writing sensible values to TscScale/TscOffset, hence the VM should fall back to another time source. Microsoft docs say "A special value of 0xFFFFFFFF is used to indicate that this facility is no longer a reliable source of reference time and the virtual machine must fall back to a different source (for example, the virtual PM timer)". So you need to write 0xFFFFFFFF here (though my suggestion would rather be to remove this part of the patch, and do everything in the next patch). The next patch should pick 0 if constant TSC is available, 0xFFFFFFFF if it is not. If the docs are wrong, I stand corrected but then you need a comment explaining that. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>> > >>> > >>>>> > >>> Yes, I have this check added in the second patch. > >>>>> > >>> > >>>> > >> Move it here please. > >>> > > OK, will do it. > >> > > >> > Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >> > patch, and leave it all to the second. > >> > > > What for? Could you please elaborate? > > To make code reviewable. Add one MSR here, the other in the second patch. removing HV_X64_MSR_REFERENCE_TSC will make this particular patch completely non-functional. > > Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: >> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: >>>>>>>>>>> >>>>>>>>>>> Yes, I have this check added in the second patch. >>>>>>>>>>> >>>>>>>>> Move it here please. >>>>>>> OK, will do it. >>>>> >>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this >>>>> patch, and leave it all to the second. >>>>> >>> What for? Could you please elaborate? > > To make code reviewable. Add one MSR here, the other in the second patch. > removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > completely non-functional. Do you mean Windows guest will BSOD or just that they won't use the reference TSC? If the latter, it's not a problem. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: > Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > > On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > >> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>>>>>>>> > >>>>>>>>>>> Yes, I have this check added in the second patch. > >>>>>>>>>>> > >>>>>>>>> Move it here please. > >>>>>>> OK, will do it. > >>>>> > >>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >>>>> patch, and leave it all to the second. > >>>>> > >>> What for? Could you please elaborate? > > > > To make code reviewable. Add one MSR here, the other in the second patch. > > removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > > completely non-functional. > > Do you mean Windows guest will BSOD or just that they won't use the > reference TSC? If the latter, it's not a problem. > I think it is. If reference counter works without TSC we have a bisect point for the case when something will going wrong with TSC. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 20/05/2013 10:36, Gleb Natapov ha scritto: > On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: >> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: >>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: >>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, I have this check added in the second patch. >>>>>>>>>>>>> >>>>>>>>>>> Move it here please. >>>>>>>>> OK, will do it. >>>>>>> >>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this >>>>>>> patch, and leave it all to the second. >>>>>>> >>>>> What for? Could you please elaborate? >>> >>> To make code reviewable. Add one MSR here, the other in the second patch. >>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch >>> completely non-functional. >> >> Do you mean Windows guest will BSOD or just that they won't use the >> reference TSC? If the latter, it's not a problem. >> > I think it is. If reference counter works without TSC we have a bisect > point for the case when something will going wrong with TSC. Isn't that exactly what might happen with this patch only? Windows will not use the TSC because it finds invalid values in the TSC page. If it still uses the reference counter, we have the situation you describe. refcount TSC page Y Y <= after patch 2 Y N <= after patch 1 N Y <= impossible N N <= removing TSC page from this patch? Of course if the guest BSODs, it's not possible to split the patches that way. Perhaps in that case it's simply better to do a single patch. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 10:42:52AM +0200, Paolo Bonzini wrote: > Il 20/05/2013 10:36, Gleb Natapov ha scritto: > > On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: > >> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > >>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > >>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>>>>>>>>>> > >>>>>>>>>>>>> Yes, I have this check added in the second patch. > >>>>>>>>>>>>> > >>>>>>>>>>> Move it here please. > >>>>>>>>> OK, will do it. > >>>>>>> > >>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >>>>>>> patch, and leave it all to the second. > >>>>>>> > >>>>> What for? Could you please elaborate? > >>> > >>> To make code reviewable. Add one MSR here, the other in the second patch. > >>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > >>> completely non-functional. > >> > >> Do you mean Windows guest will BSOD or just that they won't use the > >> reference TSC? If the latter, it's not a problem. > >> > > I think it is. If reference counter works without TSC we have a bisect > > point for the case when something will going wrong with TSC. > > Isn't that exactly what might happen with this patch only? Windows will > not use the TSC because it finds invalid values in the TSC page. Yes, it will use reference counter instead. Exactly what we want for a bisect point. > If it > still uses the reference counter, we have the situation you describe. > > refcount TSC page > Y Y <= after patch 2 > Y N <= after patch 1 > N Y <= impossible > N N <= removing TSC page from this patch? > > Of course if the guest BSODs, it's not possible to split the patches > that way. Perhaps in that case it's simply better to do a single patch. > I am not sure what you are trying to say. Your option list above shows that there is a value to split patches like they are split now. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 20/05/2013 10:49, Gleb Natapov ha scritto: > On Mon, May 20, 2013 at 10:42:52AM +0200, Paolo Bonzini wrote: >> Il 20/05/2013 10:36, Gleb Natapov ha scritto: >>> On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: >>>> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: >>>>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: >>>>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Yes, I have this check added in the second patch. >>>>>>>>>>>>>>> >>>>>>>>>>>>> Move it here please. >>>>>>>>>>> OK, will do it. >>>>>>>>> >>>>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this >>>>>>>>> patch, and leave it all to the second. >>>>>>>>> >>>>>>> What for? Could you please elaborate? >>>>> >>>>> To make code reviewable. Add one MSR here, the other in the second patch. >>>>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch >>>>> completely non-functional. >>>> >>>> Do you mean Windows guest will BSOD or just that they won't use the >>>> reference TSC? If the latter, it's not a problem. >>>> >>> I think it is. If reference counter works without TSC we have a bisect >>> point for the case when something will going wrong with TSC. >> >> Isn't that exactly what might happen with this patch only? Windows will >> not use the TSC because it finds invalid values in the TSC page. > > Yes, it will use reference counter instead. Exactly what we want for a bisect point. > >> If it >> still uses the reference counter, we have the situation you describe. >> >> refcount TSC page >> Y Y <= after patch 2 >> Y N <= after patch 1 >> N Y <= impossible >> N N <= removing TSC page from this patch? >> >> Of course if the guest BSODs, it's not possible to split the patches >> that way. Perhaps in that case it's simply better to do a single patch. >> > I am not sure what you are trying to say. Your option list above shows > that there is a value to split patches like they are split now. Hmm, we're talking past each other. :) I put the "?" because that's what Vadim implied ("it would make this particular patch non-functional"), but I don't see why it should be like this. To me, the obvious way of getting the desired bisect point is implementing one MSR per patch. So, moving the REFERENCE_TSC handling entirely to patch 2 would still be in the "refcount=Y, TSC page=N" case. In any case, this patch needs more comments and a better commit message. Microsoft docs are decent, but there are several non-obvious points in how the patches were done, and they need to be documented. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-20 at 10:05 +0200, Paolo Bonzini wrote: > Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > > On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > >> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>>>>>>>> > >>>>>>>>>>> Yes, I have this check added in the second patch. > >>>>>>>>>>> > >>>>>>>>> Move it here please. > >>>>>>> OK, will do it. > >>>>> > >>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >>>>> patch, and leave it all to the second. > >>>>> > >>> What for? Could you please elaborate? > > > > To make code reviewable. Add one MSR here, the other in the second patch. > > removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > > completely non-functional. > > Do you mean Windows guest will BSOD or just that they won't use the > reference TSC? If the latter, it's not a problem. Unfortunately, it will crash. > > Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 10:56:22AM +0200, Paolo Bonzini wrote: > Il 20/05/2013 10:49, Gleb Natapov ha scritto: > > On Mon, May 20, 2013 at 10:42:52AM +0200, Paolo Bonzini wrote: > >> Il 20/05/2013 10:36, Gleb Natapov ha scritto: > >>> On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: > >>>> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > >>>>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > >>>>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Yes, I have this check added in the second patch. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>> Move it here please. > >>>>>>>>>>> OK, will do it. > >>>>>>>>> > >>>>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >>>>>>>>> patch, and leave it all to the second. > >>>>>>>>> > >>>>>>> What for? Could you please elaborate? > >>>>> > >>>>> To make code reviewable. Add one MSR here, the other in the second patch. > >>>>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > >>>>> completely non-functional. > >>>> > >>>> Do you mean Windows guest will BSOD or just that they won't use the > >>>> reference TSC? If the latter, it's not a problem. > >>>> > >>> I think it is. If reference counter works without TSC we have a bisect > >>> point for the case when something will going wrong with TSC. > >> > >> Isn't that exactly what might happen with this patch only? Windows will > >> not use the TSC because it finds invalid values in the TSC page. > > > > Yes, it will use reference counter instead. Exactly what we want for a bisect point. > > > >> If it > >> still uses the reference counter, we have the situation you describe. > >> > >> refcount TSC page > >> Y Y <= after patch 2 > >> Y N <= after patch 1 > >> N Y <= impossible > >> N N <= removing TSC page from this patch? > >> > >> Of course if the guest BSODs, it's not possible to split the patches > >> that way. Perhaps in that case it's simply better to do a single patch. > >> > > I am not sure what you are trying to say. Your option list above shows > > that there is a value to split patches like they are split now. > > Hmm, we're talking past each other. :) > > I put the "?" because that's what Vadim implied ("it would make this > particular patch non-functional"), but I don't see why it should be like > this. To me, the obvious way of getting the desired bisect point is > implementing one MSR per patch. So, moving the REFERENCE_TSC handling > entirely to patch 2 would still be in the "refcount=Y, TSC page=N" case. > This is not the case as far as I understand from Vadim. Actually, from past discussion I remember win8 had some problem with incomplete implementation, but that was likely pre release versions. > In any case, this patch needs more comments and a better commit message. > Microsoft docs are decent, but there are several non-obvious points in > how the patches were done, and they need to be documented. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 10:56:22AM +0200, Paolo Bonzini wrote: > In any case, this patch needs more comments and a better commit message. > Microsoft docs are decent, but there are several non-obvious points in > how the patches were done, and they need to be documented. I wish you were right about Microsoft docs :) So in Hyper-V spec they say: Special value of 0xFFFFFFFF is used to indicate that this facility is no longer a reliable source of reference time and the virtual machine must fall back to a different source (for example, the virtual PM timer). May be they really mean "virtual PM timer" here and reference counter is not considered as a fall back source, but this is not what we want. On the other hand in API specification [1] they have: #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) which is not even documented in hyper-v spec. Actually 0 is specified as valid value there. Go figure. [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Il 20/05/2013 11:25, Gleb Natapov ha scritto: > So in Hyper-V spec they > say: > > Special value of 0xFFFFFFFF is used to indicate that this facility is no > longer a reliable source of reference time and the virtual machine must > fall back to a different source (for example, the virtual PM timer). > > May be they really mean "virtual PM timer" here and reference counter is > not considered as a fall back source, but this is not what we want. > > On the other hand in API specification [1] they have: > > #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) > > which is not even documented in hyper-v spec. Actually 0 is specified as > valid value there. Go figure. > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx Ok, if the API document is right then we should use HV_REFERENCE_TSC_SEQUENCE_INVALID instead of 0, with a comment explaining why we use 0 and not 0xFFFFFFFF. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 11:32:27AM +0200, Paolo Bonzini wrote: > Il 20/05/2013 11:25, Gleb Natapov ha scritto: > > So in Hyper-V spec they > > say: > > > > Special value of 0xFFFFFFFF is used to indicate that this facility is no > > longer a reliable source of reference time and the virtual machine must > > fall back to a different source (for example, the virtual PM timer). > > > > May be they really mean "virtual PM timer" here and reference counter is > > not considered as a fall back source, but this is not what we want. > > > > On the other hand in API specification [1] they have: > > > > #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) > > > > which is not even documented in hyper-v spec. Actually 0 is specified as > > valid value there. Go figure. > > > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx > > Ok, if the API document is right then we should use > HV_REFERENCE_TSC_SEQUENCE_INVALID instead of 0, with a comment > explaining why we use 0 and not 0xFFFFFFFF. > Using define is always a good idea no matter if API doc is right or hyper-v spec is, it's just the "decent documentation" part that got me :) Definitely better than nothing and thanks them for that. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-20 at 10:56 +0200, Paolo Bonzini wrote: > Il 20/05/2013 10:49, Gleb Natapov ha scritto: > > On Mon, May 20, 2013 at 10:42:52AM +0200, Paolo Bonzini wrote: > >> Il 20/05/2013 10:36, Gleb Natapov ha scritto: > >>> On Mon, May 20, 2013 at 10:05:38AM +0200, Paolo Bonzini wrote: > >>>> Il 19/05/2013 08:37, Vadim Rozenfeld ha scritto: > >>>>> On Thu, 2013-05-16 at 16:45 +0200, Paolo Bonzini wrote: > >>>>>> Il 16/05/2013 16:26, Vadim Rozenfeld ha scritto: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Yes, I have this check added in the second patch. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>> Move it here please. > >>>>>>>>>>> OK, will do it. > >>>>>>>>> > >>>>>>>>> Or better, remove all the handling of HV_X64_MSR_REFERENCE_TSC from this > >>>>>>>>> patch, and leave it all to the second. > >>>>>>>>> > >>>>>>> What for? Could you please elaborate? > >>>>> > >>>>> To make code reviewable. Add one MSR here, the other in the second patch. > >>>>> removing HV_X64_MSR_REFERENCE_TSC will make this particular patch > >>>>> completely non-functional. > >>>> > >>>> Do you mean Windows guest will BSOD or just that they won't use the > >>>> reference TSC? If the latter, it's not a problem. > >>>> > >>> I think it is. If reference counter works without TSC we have a bisect > >>> point for the case when something will going wrong with TSC. > >> > >> Isn't that exactly what might happen with this patch only? Windows will > >> not use the TSC because it finds invalid values in the TSC page. > > > > Yes, it will use reference counter instead. Exactly what we want for a bisect point. > > > >> If it > >> still uses the reference counter, we have the situation you describe. > >> > >> refcount TSC page > >> Y Y <= after patch 2 > >> Y N <= after patch 1 > >> N Y <= impossible > >> N N <= removing TSC page from this patch? > >> > >> Of course if the guest BSODs, it's not possible to split the patches > >> that way. Perhaps in that case it's simply better to do a single patch. > >> > > I am not sure what you are trying to say. Your option list above shows > > that there is a value to split patches like they are split now. > > Hmm, we're talking past each other. :) > > I put the "?" because that's what Vadim implied ("it would make this > particular patch non-functional"), but I don't see why it should be like > this. To me, the obvious way of getting the desired bisect point is > implementing one MSR per patch. So, moving the REFERENCE_TSC handling > entirely to patch 2 would still be in the "refcount=Y, TSC page=N" case. > > In any case, this patch needs more comments and a better commit message. > Microsoft docs are decent, but there are several non-obvious points in > how the patches were done, and they need to be documented. We need specify two partition privileges to activate reference time enlightenment in HYPERV_CPUID_FEATURES (0x40000003) AccessPartitionReferenceCounter and AccessPartitionReferenceTsc otherwise VM will use HPET or PMTimer as a timestamp source. If we specify AccessPartitionReferenceTsc but don't handle write request to HV_X64_MSR_REFERENCE_TSC - the system will fail with 0x78 (PHASE0_EXCEPTION) bugcheck code. If we provide HV_X64_MSR_REFERENCE_TSC handler but don't initialize sequence to 0 - guest will probably newer start or will be extremely slow, because in this case scale should also be initialized. Sequence 0 is a special case, it means use reference counter, but not TSC, as a time source. It is also a fallback solution in a case when a VM, which using TSC has been migrated to a host, which is not equipped with invariant TSC. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-20 at 12:25 +0300, Gleb Natapov wrote: > On Mon, May 20, 2013 at 10:56:22AM +0200, Paolo Bonzini wrote: > > In any case, this patch needs more comments and a better commit message. > > Microsoft docs are decent, but there are several non-obvious points in > > how the patches were done, and they need to be documented. > I wish you were right about Microsoft docs :) So in Hyper-V spec they > say: > > Special value of 0xFFFFFFFF is used to indicate that this facility is no > longer a reliable source of reference time and the virtual machine must > fall back to a different source (for example, the virtual PM timer). > > May be they really mean "virtual PM timer" here and reference counter is > not considered as a fall back source, but this is not what we want. As far as I know, you cannot fall back from iTSC to PMTimer or HPET, but you can fallback to reference counters. > > On the other hand in API specification [1] they have: > > #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) > > which is not even documented in hyper-v spec. Actually 0 is specified as > valid value there. Go figure. > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 20, 2013 at 08:25:11PM +1000, Vadim Rozenfeld wrote: > On Mon, 2013-05-20 at 12:25 +0300, Gleb Natapov wrote: > > On Mon, May 20, 2013 at 10:56:22AM +0200, Paolo Bonzini wrote: > > > In any case, this patch needs more comments and a better commit message. > > > Microsoft docs are decent, but there are several non-obvious points in > > > how the patches were done, and they need to be documented. > > I wish you were right about Microsoft docs :) So in Hyper-V spec they > > say: > > > > Special value of 0xFFFFFFFF is used to indicate that this facility is no > > longer a reliable source of reference time and the virtual machine must > > fall back to a different source (for example, the virtual PM timer). > > > > May be they really mean "virtual PM timer" here and reference counter is > > not considered as a fall back source, but this is not what we want. > > As far as I know, you cannot fall back from iTSC to PMTimer or HPET, > but you can fallback to reference counters. > What if you put 0xFFFFFFFF as a sequence? Or is this another case where the spec is wrong. > > > > On the other hand in API specification [1] they have: > > > > #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) > > > > which is not even documented in hyper-v spec. Actually 0 is specified as > > valid value there. Go figure. > > > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx > > > > -- > > Gleb. > -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-05-20 at 13:27 +0300, Gleb Natapov wrote: > On Mon, May 20, 2013 at 08:25:11PM +1000, Vadim Rozenfeld wrote: > > On Mon, 2013-05-20 at 12:25 +0300, Gleb Natapov wrote: > > > On Mon, May 20, 2013 at 10:56:22AM +0200, Paolo Bonzini wrote: > > > > In any case, this patch needs more comments and a better commit message. > > > > Microsoft docs are decent, but there are several non-obvious points in > > > > how the patches were done, and they need to be documented. > > > I wish you were right about Microsoft docs :) So in Hyper-V spec they > > > say: > > > > > > Special value of 0xFFFFFFFF is used to indicate that this facility is no > > > longer a reliable source of reference time and the virtual machine must > > > fall back to a different source (for example, the virtual PM timer). > > > > > > May be they really mean "virtual PM timer" here and reference counter is > > > not considered as a fall back source, but this is not what we want. > > > > As far as I know, you cannot fall back from iTSC to PMTimer or HPET, > > but you can fallback to reference counters. > > > What if you put 0xFFFFFFFF as a sequence? Or is this another case where > the spec is wrong. > it will use PMTimer (maybe HPET if you have it) if you specify it on VM's start up. But I'm not sure if it will work if you migrate from TSC or reference counter to 0xFFFFFFFF > > > > > > On the other hand in API specification [1] they have: > > > > > > #define HV_REFERENCE_TSC_SEQUENCE_INVALID (0x00000000) > > > > > > which is not even documented in hyper-v spec. Actually 0 is specified as > > > valid value there. Go figure. > > > > > > [1] http://msdn.microsoft.com/en-us/library/windows/hardware/ff540244%28v=vs.85%29.aspx > > > > > > -- > > > Gleb. > > > > -- > Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 3741c65..f0fee35 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -575,6 +575,8 @@ struct kvm_arch { /* fields used by HYPER-V emulation */ u64 hv_guest_os_id; u64 hv_hypercall; + u64 hv_ref_count; + u64 hv_tsc_page; #ifdef CONFIG_KVM_MMU_AUDIT int audit_point; diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h index b80420b..9711819 100644 --- a/arch/x86/include/uapi/asm/hyperv.h +++ b/arch/x86/include/uapi/asm/hyperv.h @@ -136,6 +136,9 @@ /* MSR used to read the per-partition time reference counter */ #define HV_X64_MSR_TIME_REF_COUNT 0x40000020 +/* A partition's reference time stamp counter (TSC) page */ +#define HV_X64_MSR_REFERENCE_TSC 0x40000021 + /* Define the virtual APIC registers */ #define HV_X64_MSR_EOI 0x40000070 #define HV_X64_MSR_ICR 0x40000071 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 094b5d9..1a4036d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -843,7 +843,7 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc); static u32 msrs_to_save[] = { MSR_KVM_SYSTEM_TIME, MSR_KVM_WALL_CLOCK, MSR_KVM_SYSTEM_TIME_NEW, MSR_KVM_WALL_CLOCK_NEW, - HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL, + HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL,HV_X64_MSR_TIME_REF_COUNT, HV_X64_MSR_APIC_ASSIST_PAGE, MSR_KVM_ASYNC_PF_EN, MSR_KVM_STEAL_TIME, MSR_KVM_PV_EOI_EN, MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP, MSR_IA32_SYSENTER_EIP, @@ -1764,6 +1764,8 @@ static bool kvm_hv_msr_partition_wide(u32 msr) switch (msr) { case HV_X64_MSR_GUEST_OS_ID: case HV_X64_MSR_HYPERCALL: + case HV_X64_MSR_REFERENCE_TSC: + case HV_X64_MSR_TIME_REF_COUNT: r = true; break; } @@ -1803,6 +1805,21 @@ static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data) if (__copy_to_user((void __user *)addr, instructions, 4)) return 1; kvm->arch.hv_hypercall = data; + kvm->arch.hv_ref_count = get_kernel_ns(); + break; + } + case HV_X64_MSR_REFERENCE_TSC: { + u64 gfn; + unsigned long addr; + u32 tsc_ref; + gfn = data >> HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT; + addr = gfn_to_hva(kvm, gfn); + if (kvm_is_error_hva(addr)) + return 1; + tsc_ref = 0; + if(__copy_to_user((void __user *)addr, &tsc_ref, sizeof(tsc_ref))) + return 1; + kvm->arch.hv_tsc_page = data; break; } default: @@ -2229,6 +2246,12 @@ static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata) case HV_X64_MSR_HYPERCALL: data = kvm->arch.hv_hypercall; break; + case HV_X64_MSR_TIME_REF_COUNT: + data = div_u64(get_kernel_ns() - kvm->arch.hv_ref_count,100); + break; + case HV_X64_MSR_REFERENCE_TSC: + data = kvm->arch.hv_tsc_page; + break; default: vcpu_unimpl(vcpu, "Hyper-V unhandled rdmsr: 0x%x\n", msr); return 1;