| Message ID | 20200706164155.24696-32-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/53] tcg/svm: use host cr4 during NPT page table walk | expand |
On Mon, 2020-07-06 at 12:41 -0400, Paolo Bonzini wrote: > Currently, QEMU is overriding KVM_GET_SUPPORTED_CPUID's answer for > the WAITPKG bit depending on the "-overcommit cpu-pm" setting. This is a > bad idea because it does not even check if the host supports it, but it > can be done in x86_cpu_realizefn just like we do for the MONITOR bit. > > This patch moves it there, while making it conditional on host > support for the related UMWAIT MSR. > > Cc: qemu-stable@nongnu.org > Reported-by: Maxim Levitsky <mlevitsk@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/i386/cpu.c | 3 +++ > target/i386/kvm.c | 11 +++++------ > target/i386/kvm_i386.h | 1 + > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index c44cc510e1..dc9ba06f1f 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6536,6 +6536,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx, > &cpu->mwait.ecx, &cpu->mwait.edx); > env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR; > + if (kvm_enabled() && kvm_has_waitpkg()) { > + env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG; > + } > } > if (kvm_enabled() && cpu->ucode_rev == 0) { > cpu->ucode_rev = kvm_arch_get_supported_msr_feature(kvm_state, > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 2b6b7443d2..b8455c89ed 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -411,12 +411,6 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > if (host_tsx_blacklisted()) { > ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE); > } > - } else if (function == 7 && index == 0 && reg == R_ECX) { > - if (enable_cpu_pm) { > - ret |= CPUID_7_0_ECX_WAITPKG; > - } else { > - ret &= ~CPUID_7_0_ECX_WAITPKG; > - } > } else if (function == 7 && index == 0 && reg == R_EDX) { > /* > * Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM hosts. > @@ -4730,3 +4724,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) > { > abort(); > } > + > +bool kvm_has_waitpkg(void) > +{ > + return has_msr_umwait; Note that this depends on the fix in the kernel to report this msr only when UMWAIT is supported. I personally don't mind that. If we want to support older kernels that don't, then we have to run 'cpuid' ourselves and check the result. Otherwise looks good to me. Best regards, Maxim Levitsky > +} > diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h > index 00bde7acaf..064b8798a2 100644 > --- a/target/i386/kvm_i386.h > +++ b/target/i386/kvm_i386.h > @@ -44,6 +44,7 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value); > > bool kvm_enable_x2apic(void); > bool kvm_has_x2apic_api(void); > +bool kvm_has_waitpkg(void); > > bool kvm_hv_vpindex_settable(void); >
On 07/07/20 13:42, Maxim Levitsky wrote: >> +bool kvm_has_waitpkg(void) >> +{ >> + return has_msr_umwait; > Note that this depends on the fix in the kernel > to report this msr only when UMWAIT is supported. > I personally don't mind that. > > If we want to support older kernels that don't, > then we have to run 'cpuid' ourselves and check the result. > > Otherwise looks good to me. Yes, the original kernel version was completely busted but fortunately the fix will make it into stable kernels, probably faster than it can make into QEMU releases... Paolo
On 7/7/2020 12:41 AM, Paolo Bonzini wrote: > Currently, QEMU is overriding KVM_GET_SUPPORTED_CPUID's answer for > the WAITPKG bit depending on the "-overcommit cpu-pm" setting. This is a > bad idea because it does not even check if the host supports it, but it > can be done in x86_cpu_realizefn just like we do for the MONITOR bit. > > This patch moves it there, while making it conditional on host > support for the related UMWAIT MSR. > > Cc: qemu-stable@nongnu.org > Reported-by: Maxim Levitsky <mlevitsk@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > target/i386/cpu.c | 3 +++ > target/i386/kvm.c | 11 +++++------ > target/i386/kvm_i386.h | 1 + > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index c44cc510e1..dc9ba06f1f 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6536,6 +6536,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx, > &cpu->mwait.ecx, &cpu->mwait.edx); > env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR; > + if (kvm_enabled() && kvm_has_waitpkg()) { > + env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG; > + } Hi Paolo and Maxim, Sorry to find out this long-time-ago mail although the code has been refactor nowadays. Recently, when testing WAITPKG exposure, I found WAITPKG is no longer controlled by "-overcommit cpu-pm", i.e. if !enable_cpu_pm && KVM cap reports WAITPKG (KVM set the cap in vmx_set_cpu_caps), this feature can still be seen in guest. It is obviously due to QEMU doesn't clear CPUID_7_0_ECX_WAITPKG explicitly if !enable_cpu_pm. So, is this the expected behavior for this patch? Thanks Chenyi > } > if (kvm_enabled() && cpu->ucode_rev == 0) { > cpu->ucode_rev = kvm_arch_get_supported_msr_feature(kvm_state, > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 2b6b7443d2..b8455c89ed 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -411,12 +411,6 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > if (host_tsx_blacklisted()) { > ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE); > } > - } else if (function == 7 && index == 0 && reg == R_ECX) { > - if (enable_cpu_pm) { > - ret |= CPUID_7_0_ECX_WAITPKG; > - } else { > - ret &= ~CPUID_7_0_ECX_WAITPKG; > - } > } else if (function == 7 && index == 0 && reg == R_EDX) { > /* > * Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM hosts. > @@ -4730,3 +4724,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) > { > abort(); > } > + > +bool kvm_has_waitpkg(void) > +{ > + return has_msr_umwait; > +} > diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h > index 00bde7acaf..064b8798a2 100644 > --- a/target/i386/kvm_i386.h > +++ b/target/i386/kvm_i386.h > @@ -44,6 +44,7 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value); > > bool kvm_enable_x2apic(void); > bool kvm_has_x2apic_api(void); > +bool kvm_has_waitpkg(void); > > bool kvm_hv_vpindex_settable(void); > > -- > 2.26.2 > > >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index c44cc510e1..dc9ba06f1f 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6536,6 +6536,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx, &cpu->mwait.ecx, &cpu->mwait.edx); env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR; + if (kvm_enabled() && kvm_has_waitpkg()) { + env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG; + } } if (kvm_enabled() && cpu->ucode_rev == 0) { cpu->ucode_rev = kvm_arch_get_supported_msr_feature(kvm_state, diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 2b6b7443d2..b8455c89ed 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -411,12 +411,6 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, if (host_tsx_blacklisted()) { ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE); } - } else if (function == 7 && index == 0 && reg == R_ECX) { - if (enable_cpu_pm) { - ret |= CPUID_7_0_ECX_WAITPKG; - } else { - ret &= ~CPUID_7_0_ECX_WAITPKG; - } } else if (function == 7 && index == 0 && reg == R_EDX) { /* * Linux v4.17-v4.20 incorrectly return ARCH_CAPABILITIES on SVM hosts. @@ -4730,3 +4724,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data) { abort(); } + +bool kvm_has_waitpkg(void) +{ + return has_msr_umwait; +} diff --git a/target/i386/kvm_i386.h b/target/i386/kvm_i386.h index 00bde7acaf..064b8798a2 100644 --- a/target/i386/kvm_i386.h +++ b/target/i386/kvm_i386.h @@ -44,6 +44,7 @@ void kvm_put_apicbase(X86CPU *cpu, uint64_t value); bool kvm_enable_x2apic(void); bool kvm_has_x2apic_api(void); +bool kvm_has_waitpkg(void); bool kvm_hv_vpindex_settable(void);
Currently, QEMU is overriding KVM_GET_SUPPORTED_CPUID's answer for the WAITPKG bit depending on the "-overcommit cpu-pm" setting. This is a bad idea because it does not even check if the host supports it, but it can be done in x86_cpu_realizefn just like we do for the MONITOR bit. This patch moves it there, while making it conditional on host support for the related UMWAIT MSR. Cc: qemu-stable@nongnu.org Reported-by: Maxim Levitsky <mlevitsk@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- target/i386/cpu.c | 3 +++ target/i386/kvm.c | 11 +++++------ target/i386/kvm_i386.h | 1 + 3 files changed, 9 insertions(+), 6 deletions(-)