| Message ID | 569F9419.4000601@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2016 05:05 PM, Paolo Bonzini wrote: > > > On 19/01/2016 08:48, Denis V. Lunev wrote: >>> >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index 6877b4d7e..93c9e25 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -145,6 +145,7 @@ static inline bool is_error_page(struct page *page) >>> #define KVM_REQ_HV_RESET 29 >>> #define KVM_REQ_HV_EXIT 30 >>> #define KVM_REQ_HV_STIMER 31 >>> +#define KVM_REQ_HV_TSC_PAGE 32 >>> #define KVM_REQ_MAX 64 >>> >> ping > > Applied with this change: > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index d7716c6e2752..047c275717d3 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) > tsc_khz, tsc, tsc_scale, tsc_offset); > > tsc_ref.tsc_sequence++; > - if (tsc_ref.tsc_sequence == 0) > + if (tsc_ref.tsc_sequence == 0xFFFFFFFF tsc_ref.tsc_sequence == 0) "(tsc_ref.tsc_sequence == 0xFFFFFFFF || tsc_ref.tsc_sequence == 0)" ? > tsc_ref.tsc_sequence = 1; > > tsc_ref.tsc_scale = tsc_scale; > > and renumbering KVM_REQ_HV_TSC_PAGE from 32 to 31. > > Paolo >
On 01/20/2016 05:05 PM, Paolo Bonzini wrote: > > On 19/01/2016 08:48, Denis V. Lunev wrote: >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index 6877b4d7e..93c9e25 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -145,6 +145,7 @@ static inline bool is_error_page(struct page *page) >>> #define KVM_REQ_HV_RESET 29 >>> #define KVM_REQ_HV_EXIT 30 >>> #define KVM_REQ_HV_STIMER 31 >>> +#define KVM_REQ_HV_TSC_PAGE 32 >>> #define KVM_REQ_MAX 64 >>> >> ping > Applied with this change: > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index d7716c6e2752..047c275717d3 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) > tsc_khz, tsc, tsc_scale, tsc_offset); > > tsc_ref.tsc_sequence++; > - if (tsc_ref.tsc_sequence == 0) > + if (tsc_ref.tsc_sequence == 0xFFFFFFFF tsc_ref.tsc_sequence == 0) > tsc_ref.tsc_sequence = 1; > > tsc_ref.tsc_scale = tsc_scale; > > and renumbering KVM_REQ_HV_TSC_PAGE from 32 to 31. > > Paolo the latter does not seem to be correct to me. MS spec has bug, see this thread: https://lkml.org/lkml/2015/11/2/655 and the change was Acked-by: K.Y. Den
On Wed, Jan 20, 2016 at 05:44:49PM +0300, Denis V. Lunev wrote: > On 01/20/2016 05:05 PM, Paolo Bonzini wrote: > > > >On 19/01/2016 08:48, Denis V. Lunev wrote: > >>>diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > >>>index 6877b4d7e..93c9e25 100644 > >>>--- a/include/linux/kvm_host.h > >>>+++ b/include/linux/kvm_host.h > >>>@@ -145,6 +145,7 @@ static inline bool is_error_page(struct page *page) > >>> #define KVM_REQ_HV_RESET 29 > >>> #define KVM_REQ_HV_EXIT 30 > >>> #define KVM_REQ_HV_STIMER 31 > >>>+#define KVM_REQ_HV_TSC_PAGE 32 > >>> #define KVM_REQ_MAX 64 > >>ping > >Applied with this change: > > > >diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > >index d7716c6e2752..047c275717d3 100644 > >--- a/arch/x86/kvm/hyperv.c > >+++ b/arch/x86/kvm/hyperv.c > >@@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) > > tsc_khz, tsc, tsc_scale, tsc_offset); > > tsc_ref.tsc_sequence++; > >- if (tsc_ref.tsc_sequence == 0) > >+ if (tsc_ref.tsc_sequence == 0xFFFFFFFF tsc_ref.tsc_sequence == 0) > > tsc_ref.tsc_sequence = 1; > > tsc_ref.tsc_scale = tsc_scale; > > > >and renumbering KVM_REQ_HV_TSC_PAGE from 32 to 31. > > > >Paolo > the latter does not seem to be correct to me. > MS spec has bug, see this thread: > https://lkml.org/lkml/2015/11/2/655 > and the change was Acked-by: K.Y. What happens in the patched code is this: when the sequence counter wraps around and hits an invalid value, skip to the next valid one. I think Paolo wanted to be compatible not only with the de facto implementation in Windows Hyper-V guests, but also with the Linux driver which followed the buggy spec and thought 0xFFFFFFFF to be invalid. So the change is fine (as long as there's || rather than a space there ;) Thanks, Roman.
On 01/20/2016 05:52 PM, Roman Kagan wrote: > On Wed, Jan 20, 2016 at 05:44:49PM +0300, Denis V. Lunev wrote: >> On 01/20/2016 05:05 PM, Paolo Bonzini wrote: >>> On 19/01/2016 08:48, Denis V. Lunev wrote: >>>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>>>> index 6877b4d7e..93c9e25 100644 >>>>> --- a/include/linux/kvm_host.h >>>>> +++ b/include/linux/kvm_host.h >>>>> @@ -145,6 +145,7 @@ static inline bool is_error_page(struct page *page) >>>>> #define KVM_REQ_HV_RESET 29 >>>>> #define KVM_REQ_HV_EXIT 30 >>>>> #define KVM_REQ_HV_STIMER 31 >>>>> +#define KVM_REQ_HV_TSC_PAGE 32 >>>>> #define KVM_REQ_MAX 64 >>>> ping >>> Applied with this change: >>> >>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >>> index d7716c6e2752..047c275717d3 100644 >>> --- a/arch/x86/kvm/hyperv.c >>> +++ b/arch/x86/kvm/hyperv.c >>> @@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) >>> tsc_khz, tsc, tsc_scale, tsc_offset); >>> tsc_ref.tsc_sequence++; >>> - if (tsc_ref.tsc_sequence == 0) >>> + if (tsc_ref.tsc_sequence == 0xFFFFFFFF tsc_ref.tsc_sequence == 0) >>> tsc_ref.tsc_sequence = 1; >>> tsc_ref.tsc_scale = tsc_scale; >>> >>> and renumbering KVM_REQ_HV_TSC_PAGE from 32 to 31. >>> >>> Paolo >> the latter does not seem to be correct to me. >> MS spec has bug, see this thread: >> https://lkml.org/lkml/2015/11/2/655 >> and the change was Acked-by: K.Y. > What happens in the patched code is this: when the sequence counter > wraps around and hits an invalid value, skip to the next valid one. > > I think Paolo wanted to be compatible not only with the de facto > implementation in Windows Hyper-V guests, but also with the Linux driver > which followed the buggy spec and thought 0xFFFFFFFF to be invalid. > > So the change is fine (as long as there's || rather than a space there > ;) this seems reasonable, you are right :) Thank you for the explanation.
On 20/01/2016 15:52, Roman Kagan wrote: > I think Paolo wanted to be compatible not only with the de facto > implementation in Windows Hyper-V guests, but also with the Linux driver > which followed the buggy spec and thought 0xFFFFFFFF to be invalid. 0xFFFFFFFF is not invalid; it means it is valid but you have to use the MSR. With 0, you cannot use the MSR either and Windows will use the ACPI PM timer or similar. You can see the difference from the result of QueryPerformanceFrequency, IIRC. Paolo
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index d7716c6e2752..047c275717d3 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -842,7 +842,7 @@ int kvm_hv_setup_tsc_page(struct kvm_vcpu *vcpu) tsc_khz, tsc, tsc_scale, tsc_offset); tsc_ref.tsc_sequence++; - if (tsc_ref.tsc_sequence == 0) + if (tsc_ref.tsc_sequence == 0xFFFFFFFF tsc_ref.tsc_sequence == 0) tsc_ref.tsc_sequence = 1; tsc_ref.tsc_scale = tsc_scale;