qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V5 2/2] Enabling IA32_TSC_ADJUST for KVM guest V


From: Amos Kong
Subject: Re: [Qemu-devel] [PATCH V5 2/2] Enabling IA32_TSC_ADJUST for KVM guest VM support
Date: Wed, 28 Nov 2012 10:39:16 +0800

On Wed, Nov 28, 2012 at 10:29 AM, Auld, Will <address@hidden> wrote:
Thanks Marcelo,

I'll address these items.

>- Please rebase against queue branch on kvm.git.

I am not sure how to do this. The repository I have been working against is:

git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

I assume that I need to change this in some way but I am not sure what I need to do. Will you explain or give me the needed path?


http://www.linux-kvm.org/page/Code

You should use this tree: git://git.kernel.org/pub/scm/virt/kvm/kvm.git

 

Thanks,

Will

> -----Original Message-----
> From: Marcelo Tosatti [mailto:address@hidden]
> Sent: Tuesday, November 27, 2012 5:00 PM
> To: Auld, Will
> Cc: address@hidden; Dugger, Donald D; Liu, Jinsong; Zhang,
> Xiantao; address@hidden; qemu-devel; Gleb
> Subject: Re: [PATCH V5 2/2] Enabling IA32_TSC_ADJUST for KVM guest VM
> support
>
>
> Hi Will,
>
> On Tue, Nov 27, 2012 at 11:09:00AM -0800, Will Auld wrote:
> > CPUID.7.0.EBX[1]=1 indicates IA32_TSC_ADJUST MSR 0x3b is supported
> >
> > Basic design is to emulate the MSR by allowing reads and writes to a
> > guest vcpu specific location to store the value of the emulated MSR
> > while adding the value to the vmcs tsc_offset. In this way the
> > IA32_TSC_ADJUST value will be included in all reads to the TSC MSR
> > whether through rdmsr or rdtsc. This is of course as long as the "use
> > TSC counter offsetting" VM-execution control is enabled as well as
> the IA32_TSC_ADJUST control.
> >
> > However, because hardware will only return the TSC + IA32_TSC_ADJUST
> +
> > vmsc tsc_offset for a guest process when it does and rdtsc (with the
> > correct
> > settings) the value of our virtualized IA32_TSC_ADJUST must be stored
> > in one of these three locations. The argument against storing it in
> > the actual MSR is performance. This is likely to be seldom used while
> > the save/restore is required on every transition. IA32_TSC_ADJUST was
> > created as a way to solve some issues with writing TSC itself so that
> is not an option either.
> > The remaining option, defined above as our solution has the problem
> of
> > returning incorrect vmcs tsc_offset values (unless we intercept and
> > fix, not done here) as mentioned above. However, more problematic is
> > that storing the data in vmcs tsc_offset will have a different
> > semantic effect on the system than does using the actual MSR. This is
> illustrated in the following example:
> > The hypervisor set the IA32_TSC_ADJUST, then the guest sets it and a
> > guest process performs a rdtsc. In this case the guest process will
> > get TSC + IA32_TSC_ADJUST_hyperviser + vmsc tsc_offset including
> IA32_TSC_ADJUST_guest.
> > While the total system semantics changed the semantics as seen by the
> > guest do not and hence this will not cause a problem.
> >
> > Signed-off-by: Will Auld <address@hidden>
> > ---
> >  arch/x86/include/asm/cpufeature.h |  1 +
> >  arch/x86/include/asm/kvm_host.h   |  3 +++
> >  arch/x86/include/asm/msr-index.h  |  1 +
> >  arch/x86/kvm/cpuid.c              |  2 ++
> >  arch/x86/kvm/cpuid.h              |  8 ++++++++
> >  arch/x86/kvm/svm.c                |  7 +++++++
> >  arch/x86/kvm/vmx.c                |  9 +++++++++
> >  arch/x86/kvm/x86.c                | 22 ++++++++++++++++++++++
> >  8 files changed, 53 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/cpufeature.h
> > b/arch/x86/include/asm/cpufeature.h
> > index 6b7ee5f..e574d81 100644
> > --- a/arch/x86/include/asm/cpufeature.h
> > +++ b/arch/x86/include/asm/cpufeature.h
> > @@ -199,6 +199,7 @@
> >
> >  /* Intel-defined CPU features, CPUID level 0x00000007:0 (ebx), word
> 9 */
> >  #define X86_FEATURE_FSGSBASE       (9*32+ 0) /* {RD/WR}{FS/GS}BASE
> instructions*/
> > +#define X86_FEATURE_TSC_ADJUST  (9*32+ 1) /* TSC adjustment MSR 0x3b
> > +*/
> >  #define X86_FEATURE_BMI1   (9*32+ 3) /* 1st group bit manipulation
> extensions */
> >  #define X86_FEATURE_HLE            (9*32+ 4) /* Hardware Lock Elision
> */
> >  #define X86_FEATURE_AVX2   (9*32+ 5) /* AVX2 instructions */
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h index da34027..cf8c7e0 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -442,6 +442,8 @@ struct kvm_vcpu_arch {
> >     u32 virtual_tsc_mult;
> >     u32 virtual_tsc_khz;
> >
> > +   s64 ia32_tsc_adjust_msr;
> > +
> >     atomic_t nmi_queued;  /* unprocessed asynchronous NMIs */
> >     unsigned nmi_pending; /* NMI queued after currently running
> handler */
> >     bool nmi_injected;    /* Trying to inject an NMI this entry */
> > @@ -690,6 +692,7 @@ struct kvm_x86_ops {
> >     bool (*has_wbinvd_exit)(void);
> >
> >     void (*set_tsc_khz)(struct kvm_vcpu *vcpu, u32 user_tsc_khz, bool
> > scale);
> > +   u64 (*read_tsc_offset)(struct kvm_vcpu *vcpu);
> >     void (*write_tsc_offset)(struct kvm_vcpu *vcpu, u64 offset);
> >
> >     u64 (*compute_tsc_offset)(struct kvm_vcpu *vcpu, u64 target_tsc);
> > diff --git a/arch/x86/include/asm/msr-index.h
> > b/arch/x86/include/asm/msr-index.h
> > index 957ec87..6486569 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -231,6 +231,7 @@
> >  #define MSR_IA32_EBL_CR_POWERON            0x0000002a
> >  #define MSR_EBC_FREQUENCY_ID               0x0000002c
> >  #define MSR_IA32_FEATURE_CONTROL        0x0000003a
> > +#define MSR_IA32_TSC_ADJUST             0x0000003b
> >
> >  #define FEATURE_CONTROL_LOCKED                             (1<<0)
> >  #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX   (1<<1)
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index
> > 0595f13..e817bac 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -320,6 +320,8 @@ static int do_cpuid_ent(struct kvm_cpuid_entry2
> *entry, u32 function,
> >             if (index == 0) {
> >                     entry->ebx &= kvm_supported_word9_x86_features;
> >                     cpuid_mask(&entry->ebx, 9);
> > +                   // TSC_ADJUST is emulated
> > +                   entry->ebx |= F(TSC_ADJUST);
> >             } else
> >                     entry->ebx = 0;
> >             entry->eax = 0;
> > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index
> > a10e460..3a8b504 100644
> > --- a/arch/x86/kvm/cpuid.h
> > +++ b/arch/x86/kvm/cpuid.h
> > @@ -28,6 +28,14 @@ static inline bool guest_cpuid_has_xsave(struct
> kvm_vcpu *vcpu)
> >     return best && (best->ecx & bit(X86_FEATURE_XSAVE));  }
> >
> > +static inline bool guest_cpuid_has_tsc_adjust(struct kvm_vcpu *vcpu)
> > +{
> > +   struct kvm_cpuid_entry2 *best;
> > +
> > +   best = kvm_find_cpuid_entry(vcpu, 7, 0);
> > +   return best && (best->ebx & bit(X86_FEATURE_TSC_ADJUST)); }
> > +
> >  static inline bool guest_cpuid_has_smep(struct kvm_vcpu *vcpu)  {
> >     struct kvm_cpuid_entry2 *best;
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index
> > 5ac11f0..7f5e6eb 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -1012,6 +1012,12 @@ static void svm_set_tsc_khz(struct kvm_vcpu
> *vcpu, u32 user_tsc_khz, bool scale)
> >     svm->tsc_ratio             = ratio;
> >  }
> >
> > +static u64 svm_read_tsc_offset(struct kvm_vcpu *vcpu) {
> > +   struct vcpu_svm *svm = to_svm(vcpu);
> > +   return svm->vmcb->control.tsc_offset; }
> > +
> >  static void svm_write_tsc_offset(struct kvm_vcpu *vcpu, u64 offset)
> > {
> >     struct vcpu_svm *svm = to_svm(vcpu); @@ -4333,6 +4339,7 @@ static
> > struct kvm_x86_ops svm_x86_ops = {
> >     .has_wbinvd_exit = svm_has_wbinvd_exit,
> >
> >     .set_tsc_khz = svm_set_tsc_khz,
> > +   .read_tsc_offset = svm_read_tsc_offset,
> >     .write_tsc_offset = svm_write_tsc_offset,
> >     .adjust_tsc_offset = svm_adjust_tsc_offset,
> >     .compute_tsc_offset = svm_compute_tsc_offset, diff --git
> > a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 819970f..0a73e72
> > 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -1861,6 +1861,11 @@ static void vmx_set_tsc_khz(struct kvm_vcpu
> *vcpu, u32 user_tsc_khz, bool scale)
> >             WARN(1, "user requested TSC rate below hardware speed\n");
> }
> >
> > +static u64 vmx_read_tsc_offset(struct kvm_vcpu *vcpu) {
> > +   return vmcs_read64(TSC_OFFSET);
> > +}
> > +
> >  /*
> >   * writes 'offset' into guest's timestamp counter offset register
> >   */
> > @@ -2243,6 +2248,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
> >             }
> >             ret = kvm_set_msr_common(vcpu, msr_info);
> > +   case MSR_IA32_TSC_ADJUST:
> > +           ret = kvm_set_msr_common(vcpu, msr_info);
> > +           break;
> >     case MSR_TSC_AUX:
> >             if (!vmx->rdtscp_enabled)
> >                     return 1;
> > @@ -7338,6 +7346,7 @@ static struct kvm_x86_ops vmx_x86_ops = {
> >     .has_wbinvd_exit = cpu_has_vmx_wbinvd_exit,
> >
> >     .set_tsc_khz = vmx_set_tsc_khz,
> > +   .read_tsc_offset = vmx_read_tsc_offset,
> >     .write_tsc_offset = vmx_write_tsc_offset,
> >     .adjust_tsc_offset = vmx_adjust_tsc_offset,
> >     .compute_tsc_offset = vmx_compute_tsc_offset, diff --git
> > a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index edafa29..7c2990b
> > 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -824,6 +824,7 @@ static u32 msrs_to_save[] = {  static unsigned
> > num_msrs_to_save;
> >
> >  static u32 emulated_msrs[] = {
> > +   MSR_IA32_TSC_ADJUST,
> >     MSR_IA32_TSCDEADLINE,
> >     MSR_IA32_MISC_ENABLE,
> >     MSR_IA32_MCG_STATUS,
> > @@ -1047,6 +1048,12 @@ static u64 compute_guest_tsc(struct kvm_vcpu
> *vcpu, s64 kernel_ns)
> >     return tsc;
> >  }
> >
> > +static void update_ia32_tsc_adjust_msr(struct kvm_vcpu *vcpu, s64
> > +offset) {
> > +   u64 curr_offset =  kvm_x86_ops->read_tsc_offset(vcpu);
>
> Extra space.
>
> Other than that, looks fine. Remaining comments:
>
> - Basic functional test must be improved (testing that wrmsr(tsc, val)
> reflects on tsc_adjust and wrmsr(tsc_adjust_msr, val) reflects on tsc
> is enough.
>
> - Behaviour on reset: what is the behaviour on RESET?
>
> - Please rebase against queue branch on kvm.git.
>
> Thanks
>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to address@hidden
More majordomo info at  http://vger.kernel.org/majordomo-info.html


reply via email to

[Prev in Thread] Current Thread [Next in Thread]