On 5/30/2024 11:53 PM, Zhao Liu wrote:
> On Fri, May 24, 2024 at 01:00:17PM -0700, Zide Chen wrote:
>> Date: Fri, 24 May 2024 13:00:17 -0700
>> From: Zide Chen <zide.c...@intel.com>
>> Subject: [PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into
>> kvm_cpu_realizefn()
>> X-Mailer: git-send-email 2.34.1
>>
>> It seems not a good idea to expand features in host_cpu_realizefn,
>> which is for host CPU only.
>
> It is restricted by max_features and should be part of the "max" CPU,
> and the current target/i386/host-cpu.c should only serve the “host” CPU.
Yes, since this file only serves for "host" CPU, that's why I proposed
to move this code to kvm_cpu_realizefn().
>
>> Additionally, cpu-pm option is KVM
>> specific, and it's cleaner to put it in kvm_cpu_realizefn(), together
>> with the WAITPKG code.
>>
>> Fixes: f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using
>> AccelCPUClass")
>> Signed-off-by: Zide Chen <zide.c...@intel.com>
>> ---
>> target/i386/host-cpu.c | 12 ------------
>> target/i386/kvm/kvm-cpu.c | 11 +++++++++--
>> 2 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
>> index 280e427c017c..8b8bf5afeccf 100644
>> --- a/target/i386/host-cpu.c
>> +++ b/target/i386/host-cpu.c
>> @@ -42,15 +42,6 @@ static uint32_t host_cpu_phys_bits(void)
>> return host_phys_bits;
>> }
>>
>> -static void host_cpu_enable_cpu_pm(X86CPU *cpu)
>> -{
>> - CPUX86State *env = &cpu->env;
>> -
>> - host_cpuid(5, 0, &cpu->mwait.eax, &cpu->mwait.ebx,
>> - &cpu->mwait.ecx, &cpu->mwait.edx);
>> - env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
>> -}
>> -
>> static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
>> {
>> uint32_t host_phys_bits = host_cpu_phys_bits();
>> @@ -83,9 +74,6 @@ bool host_cpu_realizefn(CPUState *cs, Error **errp)
>> X86CPU *cpu = X86_CPU(cs);
>> CPUX86State *env = &cpu->env;
>>
>> - if (cpu->max_features && enable_cpu_pm) {
>> - host_cpu_enable_cpu_pm(cpu);
>> - }
>> if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
>> uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu);
>>
>> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
>> index 3adcedf0dbc3..197c892da89a 100644
>> --- a/target/i386/kvm/kvm-cpu.c
>> +++ b/target/i386/kvm/kvm-cpu.c
>> @@ -64,9 +64,16 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
>> * cpu_common_realizefn() (via xcc->parent_realize)
>> */
>> if (cpu->max_features) {
>> - if (enable_cpu_pm && kvm_has_waitpkg()) {
>> - env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
>> + if (enable_cpu_pm) {
>> + if (kvm_has_waitpkg()) {
>> + env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
>> + }
>
> If you agree with my comment in patch 2, here we need a mwait bit check.
I still think that take advantaged of x86_cpu_filter_features() to check
host availability is a better choice.
>
>> + 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 (cpu->ucode_rev == 0) {
>> cpu->ucode_rev =
>> kvm_arch_get_supported_msr_feature(kvm_state,
>> --
>> 2.34.1
>>
>>
