| Message ID | 20240524200017.150339-4-zide.chen@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve -overcommit cpu-pm=on|off | expand |
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.chen@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. > 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.chen@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. > + 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 > >
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.chen@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.chen@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 >> >>
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; + } + + 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,
It seems not a good idea to expand features in host_cpu_realizefn, which is for host CPU only. 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.chen@intel.com> --- target/i386/host-cpu.c | 12 ------------ target/i386/kvm/kvm-cpu.c | 11 +++++++++-- 2 files changed, 9 insertions(+), 14 deletions(-)