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? Thanks, Will > -----Original Message----- > From: Marcelo Tosatti [mailto:mtosatti@xxxxxxxxxx] > Sent: Tuesday, November 27, 2012 5:00 PM > To: Auld, Will > Cc: kvm@xxxxxxxxxxxxxxx; Dugger, Donald D; Liu, Jinsong; Zhang, > Xiantao; avi@xxxxxxxxxx; 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.auld@xxxxxxxxx> > > --- > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
