On Wed, Nov 28, 2012 at 10:29 AM, Auld, Will <will.a...@intel.com> 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:mtosa...@redhat.com] > > Sent: Tuesday, November 27, 2012 5:00 PM > > To: Auld, Will > > Cc: k...@vger.kernel.org; Dugger, Donald D; Liu, Jinsong; Zhang, > > Xiantao; a...@redhat.com; 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 <will.a...@intel.com> > > > --- > > > 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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
