Re: [PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()
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 >> 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 >> --- >> 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 = >env; >> - >> -host_cpuid(5, 0, >mwait.eax, >mwait.ebx, >> - >mwait.ecx, >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 = >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, >mwait.eax, >mwait.ebx, >> + >mwait.ecx, >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 >> >>
Re: [PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()
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 > 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 > --- > 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 = >env; > - > -host_cpuid(5, 0, >mwait.eax, >mwait.ebx, > - >mwait.ecx, >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 = >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, >mwait.eax, >mwait.ebx, > + >mwait.ecx, >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 > >
[PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()
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 --- 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 = >env; - -host_cpuid(5, 0, >mwait.eax, >mwait.ebx, - >mwait.ecx, >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 = >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, >mwait.eax, >mwait.ebx, + >mwait.ecx, >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