| Message ID | 20190620084620.17974-3-tao3.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Enable user wait instructions | expand |
On Thu, 2019-06-20 at 16:46 +0800, Tao Xu wrote: > UMWAIT and TPAUSE instructions use IA32_UMWAIT_CONTROL at MSR index E1H > to determines the maximum time in TSC-quanta that the processor can reside > in either C0.1 or C0.2. > > This patch emulates MSR IA32_UMWAIT_CONTROL in guest and differentiate > IA32_UMWAIT_CONTROL between host and guest. The variable > mwait_control_cached in arch/x86/power/umwait.c caches the MSR value, so > this patch uses it to avoid frequently rdmsr of IA32_UMWAIT_CONTROL. > > Co-developed-by: Jingqi Liu <jingqi.liu@intel.com> > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > --- > > Changes in v5: > remove vmx_waitpkg_supported() to fix guest can rdmsr or wrmsr > when the feature is off (Xiaoyao) > remove the atomic_switch_ia32_umwait_control() and move the > codes into vmx_set_msr() > rebase the patch because the kernel dependcy patch updated to > v5: https://lkml.org/lkml/2019/6/19/972 > --- > arch/x86/kernel/cpu/umwait.c | 3 ++- > arch/x86/kvm/vmx/vmx.c | 24 ++++++++++++++++++++++++ > arch/x86/kvm/vmx/vmx.h | 3 +++ > arch/x86/kvm/x86.c | 1 + > 4 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c > index 4b2aff7b2d4d..db5c193ef136 100644 > --- a/arch/x86/kernel/cpu/umwait.c > +++ b/arch/x86/kernel/cpu/umwait.c > @@ -15,7 +15,8 @@ > * MSR value. By default, umwait max time is 100000 in TSC-quanta and C0.2 > * is enabled > */ > -static u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, > UMWAIT_C02_ENABLED); > +u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, UMWAIT_C02_ENABLED); > +EXPORT_SYMBOL_GPL(umwait_control_cached); > > /* > * Serialize access to umwait_control_cached and IA32_UMWAIT_CONTROL MSR > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index b35bfac30a34..0fb55c8426e2 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1679,6 +1679,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct > msr_data *msr_info) > #endif > case MSR_EFER: > return kvm_get_msr_common(vcpu, msr_info); > + case MSR_IA32_UMWAIT_CONTROL: > + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) > + return 1; > + > + msr_info->data = vmx->msr_ia32_umwait_control; > + break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > @@ -1841,6 +1847,22 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct > msr_data *msr_info) > return 1; > vmcs_write64(GUEST_BNDCFGS, data); > break; > + case MSR_IA32_UMWAIT_CONTROL: > + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) > + return 1; > + > + /* The reserved bit IA32_UMWAIT_CONTROL[1] should be zero */ > + if (data & BIT_ULL(1)) > + return 1; > + > + vmx->msr_ia32_umwait_control = data; > + if (vmx->msr_ia32_umwait_control != umwait_control_cached) > + add_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL, > + vmx->msr_ia32_umwait_control, > + umwait_control_cached, false); > + else > + clear_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL); You cannot put the atomic switch here. What if umwait_control_cached is changed at runtime? Host kernel patch exposed a sysfs interface to let it happen. > + break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > @@ -4126,6 +4148,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool > init_event) > vmx->rmode.vm86_active = 0; > vmx->spec_ctrl = 0; > > + vmx->msr_ia32_umwait_control = 0; > + > vcpu->arch.microcode_version = 0x100000000ULL; > vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > kvm_set_cr8(vcpu, 0); > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 61128b48c503..8485bec7c38a 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -14,6 +14,8 @@ > extern const u32 vmx_msr_index[]; > extern u64 host_efer; > > +extern u32 umwait_control_cached; > + > #define MSR_TYPE_R 1 > #define MSR_TYPE_W 2 > #define MSR_TYPE_RW 3 > @@ -194,6 +196,7 @@ struct vcpu_vmx { > #endif > > u64 spec_ctrl; > + u64 msr_ia32_umwait_control; > > u32 vm_entry_controls_shadow; > u32 vm_exit_controls_shadow; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 83aefd759846..4480de459bf4 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = { > MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B, > MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B, > MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B, > + MSR_IA32_UMWAIT_CONTROL, > }; > > static unsigned num_msrs_to_save;
On Thu, 2019-06-20 at 16:46 +0800, Tao Xu wrote: > UMWAIT and TPAUSE instructions use IA32_UMWAIT_CONTROL at MSR index E1H > to determines the maximum time in TSC-quanta that the processor can reside > in either C0.1 or C0.2. > > This patch emulates MSR IA32_UMWAIT_CONTROL in guest and differentiate > IA32_UMWAIT_CONTROL between host and guest. The variable > mwait_control_cached in arch/x86/power/umwait.c caches the MSR value, so > this patch uses it to avoid frequently rdmsr of IA32_UMWAIT_CONTROL. > > Co-developed-by: Jingqi Liu <jingqi.liu@intel.com> > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com> > Signed-off-by: Tao Xu <tao3.xu@intel.com> > --- > > Changes in v5: > remove vmx_waitpkg_supported() to fix guest can rdmsr or wrmsr > when the feature is off (Xiaoyao) > remove the atomic_switch_ia32_umwait_control() and move the > codes into vmx_set_msr() > rebase the patch because the kernel dependcy patch updated to > v5: https://lkml.org/lkml/2019/6/19/972 > --- > arch/x86/kernel/cpu/umwait.c | 3 ++- > arch/x86/kvm/vmx/vmx.c | 24 ++++++++++++++++++++++++ > arch/x86/kvm/vmx/vmx.h | 3 +++ > arch/x86/kvm/x86.c | 1 + > 4 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c > index 4b2aff7b2d4d..db5c193ef136 100644 > --- a/arch/x86/kernel/cpu/umwait.c > +++ b/arch/x86/kernel/cpu/umwait.c > @@ -15,7 +15,8 @@ > * MSR value. By default, umwait max time is 100000 in TSC-quanta and C0.2 > * is enabled > */ > -static u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, > UMWAIT_C02_ENABLED); > +u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, UMWAIT_C02_ENABLED); > +EXPORT_SYMBOL_GPL(umwait_control_cached); > > /* > * Serialize access to umwait_control_cached and IA32_UMWAIT_CONTROL MSR > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index b35bfac30a34..0fb55c8426e2 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -1679,6 +1679,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct > msr_data *msr_info) > #endif > case MSR_EFER: > return kvm_get_msr_common(vcpu, msr_info); > + case MSR_IA32_UMWAIT_CONTROL: > + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) Need to check (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) otherwise, userspace, like qemu, cannot access this msr. Also, same for vmx_set_msr. > + return 1; > + > + msr_info->data = vmx->msr_ia32_umwait_control; > + break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > @@ -1841,6 +1847,22 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct > msr_data *msr_info) > return 1; > vmcs_write64(GUEST_BNDCFGS, data); > break; > + case MSR_IA32_UMWAIT_CONTROL: > + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) > + return 1; > + > + /* The reserved bit IA32_UMWAIT_CONTROL[1] should be zero */ > + if (data & BIT_ULL(1)) > + return 1; > + > + vmx->msr_ia32_umwait_control = data; > + if (vmx->msr_ia32_umwait_control != umwait_control_cached) > + add_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL, > + vmx->msr_ia32_umwait_control, > + umwait_control_cached, false); > + else > + clear_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL); > + break; > case MSR_IA32_SPEC_CTRL: > if (!msr_info->host_initiated && > !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) > @@ -4126,6 +4148,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool > init_event) > vmx->rmode.vm86_active = 0; > vmx->spec_ctrl = 0; > > + vmx->msr_ia32_umwait_control = 0; > + > vcpu->arch.microcode_version = 0x100000000ULL; > vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); > kvm_set_cr8(vcpu, 0); > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h > index 61128b48c503..8485bec7c38a 100644 > --- a/arch/x86/kvm/vmx/vmx.h > +++ b/arch/x86/kvm/vmx/vmx.h > @@ -14,6 +14,8 @@ > extern const u32 vmx_msr_index[]; > extern u64 host_efer; > > +extern u32 umwait_control_cached; > + > #define MSR_TYPE_R 1 > #define MSR_TYPE_W 2 > #define MSR_TYPE_RW 3 > @@ -194,6 +196,7 @@ struct vcpu_vmx { > #endif > > u64 spec_ctrl; > + u64 msr_ia32_umwait_control; > > u32 vm_entry_controls_shadow; > u32 vm_exit_controls_shadow; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 83aefd759846..4480de459bf4 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = { > MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B, > MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B, > MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B, > + MSR_IA32_UMWAIT_CONTROL, > }; > > static unsigned num_msrs_to_save;
On 20/06/19 11:46, Xiaoyao Li wrote: > You cannot put the atomic switch here. What if umwait_control_cached is changed > at runtime? Host kernel patch exposed a sysfs interface to let it happen. Thanks for the review, Xiaoyao. I agree with both of your remarks. Paolo >> + break; >> case MSR_IA32_SPEC_CTRL: >> if (!msr_info->host_initiated && >> !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) >> @@ -4126,6 +4148,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool >> init_event) >> vmx->rmode.vm86_active = 0; >> vmx->spec_ctrl = 0; >> >> + vmx->msr_ia32_umwait_control = 0; >> + >> vcpu->arch.microcode_version = 0x100000000ULL; >> vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); >> kvm_set_cr8(vcpu, 0); >> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h >> index 61128b48c503..8485bec7c38a 100644 >> --- a/arch/x86/kvm/vmx/vmx.h >> +++ b/arch/x86/kvm/vmx/vmx.h >> @@ -14,6 +14,8 @@ >> extern const u32 vmx_msr_index[]; >> extern u64 host_efer; >> >> +extern u32 umwait_control_cached; >> + >> #define MSR_TYPE_R 1 >> #define MSR_TYPE_W 2 >> #define MSR_TYPE_RW 3 >> @@ -194,6 +196,7 @@ struct vcpu_vmx { >> #endif >> >> u64 spec_ctrl; >> + u64 msr_ia32_umwait_control; >> >> u32 vm_entry_controls_shadow; >> u32 vm_exit_controls_shadow; >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 83aefd759846..4480de459bf4 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = { >> MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B, >> MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B, >> MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B, >> + MSR_IA32_UMWAIT_CONTROL, >> }; >> >> static unsigned num_msrs_to_save; >
On 7/3/2019 12:37 AM, Paolo Bonzini wrote: > On 20/06/19 11:46, Xiaoyao Li wrote: >> You cannot put the atomic switch here. What if umwait_control_cached is changed >> at runtime? Host kernel patch exposed a sysfs interface to let it happen. > > Thanks for the review, Xiaoyao. I agree with both of your remarks. > > Paolo > Hi paolo, The issues have been solved in v6 patches, could you help to review v6 patches? Thanks Tao >>> + break; >>> case MSR_IA32_SPEC_CTRL: >>> if (!msr_info->host_initiated && >>> !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) >>> @@ -4126,6 +4148,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool >>> init_event) >>> vmx->rmode.vm86_active = 0; >>> vmx->spec_ctrl = 0; >>> >>> + vmx->msr_ia32_umwait_control = 0; >>> + >>> vcpu->arch.microcode_version = 0x100000000ULL; >>> vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); >>> kvm_set_cr8(vcpu, 0); >>> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h >>> index 61128b48c503..8485bec7c38a 100644 >>> --- a/arch/x86/kvm/vmx/vmx.h >>> +++ b/arch/x86/kvm/vmx/vmx.h >>> @@ -14,6 +14,8 @@ >>> extern const u32 vmx_msr_index[]; >>> extern u64 host_efer; >>> >>> +extern u32 umwait_control_cached; >>> + >>> #define MSR_TYPE_R 1 >>> #define MSR_TYPE_W 2 >>> #define MSR_TYPE_RW 3 >>> @@ -194,6 +196,7 @@ struct vcpu_vmx { >>> #endif >>> >>> u64 spec_ctrl; >>> + u64 msr_ia32_umwait_control; >>> >>> u32 vm_entry_controls_shadow; >>> u32 vm_exit_controls_shadow; >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 83aefd759846..4480de459bf4 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = { >>> MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B, >>> MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B, >>> MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B, >>> + MSR_IA32_UMWAIT_CONTROL, >>> }; >>> >>> static unsigned num_msrs_to_save; >> >
diff --git a/arch/x86/kernel/cpu/umwait.c b/arch/x86/kernel/cpu/umwait.c index 4b2aff7b2d4d..db5c193ef136 100644 --- a/arch/x86/kernel/cpu/umwait.c +++ b/arch/x86/kernel/cpu/umwait.c @@ -15,7 +15,8 @@ * MSR value. By default, umwait max time is 100000 in TSC-quanta and C0.2 * is enabled */ -static u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, UMWAIT_C02_ENABLED); +u32 umwait_control_cached = UMWAIT_CTRL_VAL(100000, UMWAIT_C02_ENABLED); +EXPORT_SYMBOL_GPL(umwait_control_cached); /* * Serialize access to umwait_control_cached and IA32_UMWAIT_CONTROL MSR diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index b35bfac30a34..0fb55c8426e2 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -1679,6 +1679,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) #endif case MSR_EFER: return kvm_get_msr_common(vcpu, msr_info); + case MSR_IA32_UMWAIT_CONTROL: + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) + return 1; + + msr_info->data = vmx->msr_ia32_umwait_control; + break; case MSR_IA32_SPEC_CTRL: if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) @@ -1841,6 +1847,22 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) return 1; vmcs_write64(GUEST_BNDCFGS, data); break; + case MSR_IA32_UMWAIT_CONTROL: + if (!guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)) + return 1; + + /* The reserved bit IA32_UMWAIT_CONTROL[1] should be zero */ + if (data & BIT_ULL(1)) + return 1; + + vmx->msr_ia32_umwait_control = data; + if (vmx->msr_ia32_umwait_control != umwait_control_cached) + add_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL, + vmx->msr_ia32_umwait_control, + umwait_control_cached, false); + else + clear_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL); + break; case MSR_IA32_SPEC_CTRL: if (!msr_info->host_initiated && !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL)) @@ -4126,6 +4148,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) vmx->rmode.vm86_active = 0; vmx->spec_ctrl = 0; + vmx->msr_ia32_umwait_control = 0; + vcpu->arch.microcode_version = 0x100000000ULL; vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val(); kvm_set_cr8(vcpu, 0); diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h index 61128b48c503..8485bec7c38a 100644 --- a/arch/x86/kvm/vmx/vmx.h +++ b/arch/x86/kvm/vmx/vmx.h @@ -14,6 +14,8 @@ extern const u32 vmx_msr_index[]; extern u64 host_efer; +extern u32 umwait_control_cached; + #define MSR_TYPE_R 1 #define MSR_TYPE_W 2 #define MSR_TYPE_RW 3 @@ -194,6 +196,7 @@ struct vcpu_vmx { #endif u64 spec_ctrl; + u64 msr_ia32_umwait_control; u32 vm_entry_controls_shadow; u32 vm_exit_controls_shadow; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 83aefd759846..4480de459bf4 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = { MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B, MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B, MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B, + MSR_IA32_UMWAIT_CONTROL, }; static unsigned num_msrs_to_save;