| Message ID | 20250302220112.17653-6-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386/kvm/pmu: PMU Enhancement, Bugfix and Cleanup | expand |
On 3/3/2025 6:00 AM, Dongli Zhang wrote: > The initialization of 'has_architectural_pmu_version', > 'num_architectural_pmu_gp_counters', and > 'num_architectural_pmu_fixed_counters' is unrelated to the process of > building the CPUID. > > Extract them out of kvm_x86_build_cpuid(). > > No functional change. > > Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> > --- > Changed since v1: > - Still extract the code, but call them for all CPUs. > > target/i386/kvm/kvm.c | 66 +++++++++++++++++++++++++------------------ > 1 file changed, 39 insertions(+), 27 deletions(-) > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 5c8a852dbd..8f293ffd61 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1959,33 +1959,6 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, > } > } > > - if (limit >= 0x0a) { > - uint32_t eax, edx; > - > - cpu_x86_cpuid(env, 0x0a, 0, &eax, &unused, &unused, &edx); > - > - has_architectural_pmu_version = eax & 0xff; > - if (has_architectural_pmu_version > 0) { > - num_architectural_pmu_gp_counters = (eax & 0xff00) >> 8; > - > - /* Shouldn't be more than 32, since that's the number of bits > - * available in EBX to tell us _which_ counters are available. > - * Play it safe. > - */ > - if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { > - num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; > - } > - > - if (has_architectural_pmu_version > 1) { > - num_architectural_pmu_fixed_counters = edx & 0x1f; > - > - if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { > - num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; > - } > - } > - } > - } > - > cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused); > > for (i = 0x80000000; i <= limit; i++) { > @@ -2085,6 +2058,43 @@ int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp) > return 0; > } > > +static void kvm_init_pmu_info(CPUX86State *env) > +{ > + uint32_t eax, edx; > + uint32_t unused; > + uint32_t limit; > + > + cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); > + > + if (limit < 0x0a) { > + return; > + } > + > + cpu_x86_cpuid(env, 0x0a, 0, &eax, &unused, &unused, &edx); > + > + has_architectural_pmu_version = eax & 0xff; > + if (has_architectural_pmu_version > 0) { > + num_architectural_pmu_gp_counters = (eax & 0xff00) >> 8; > + > + /* > + * Shouldn't be more than 32, since that's the number of bits > + * available in EBX to tell us _which_ counters are available. > + * Play it safe. > + */ > + if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { > + num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; > + } > + > + if (has_architectural_pmu_version > 1) { > + num_architectural_pmu_fixed_counters = edx & 0x1f; > + > + if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { > + num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; > + } > + } > + } > +} > + > int kvm_arch_init_vcpu(CPUState *cs) > { > struct { > @@ -2267,6 +2277,8 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); > cpuid_data.cpuid.nent = cpuid_i; > > + kvm_init_pmu_info(env); > + > if (((env->cpuid_version >> 8)&0xF) >= 6 > && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == > (CPUID_MCE | CPUID_MCA)) { Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> +static void kvm_init_pmu_info(CPUX86State *env) > +{ > + uint32_t eax, edx; > + uint32_t unused; > + uint32_t limit; > + > + cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); At this stage, CPUID has already been filled and we should not use cpu_x86_cpuid() to get the "raw" CPUID info. Instead, after kvm_x86_build_cpuid(), the cpuid_find_entry() helper should be preferred. With cpuid_find_entry(), we don't even need to check the limit again. > + > + if (limit < 0x0a) { > + return; > + } ... > int kvm_arch_init_vcpu(CPUState *cs) > { > struct { > @@ -2267,6 +2277,8 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); > cpuid_data.cpuid.nent = cpuid_i; > > + kvm_init_pmu_info(env); > + Referring what has_msr_feature_control did, what about the following change? int kvm_arch_init_vcpu(CPUState *cs) { struct { @@ -2277,8 +2240,6 @@ int kvm_arch_init_vcpu(CPUState *cs) cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); cpuid_data.cpuid.nent = cpuid_i; - kvm_init_pmu_info(env); - if (((env->cpuid_version >> 8)&0xF) >= 6 && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == (CPUID_MCE | CPUID_MCA)) { @@ -2329,6 +2290,31 @@ int kvm_arch_init_vcpu(CPUState *cs) has_msr_feature_control = true; } + c = cpuid_find_entry(&cpuid_data.cpuid, 0xa, 0); + if (c) { + has_architectural_pmu_version = c->eax & 0xff; + if (has_architectural_pmu_version > 0) { + num_architectural_pmu_gp_counters = (c->eax & 0xff00) >> 8; + + /* + * Shouldn't be more than 32, since that's the number of bits + * available in EBX to tell us _which_ counters are available. + * Play it safe. + */ + if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { + num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; + } + + if (has_architectural_pmu_version > 1) { + num_architectural_pmu_fixed_counters = c->edx & 0x1f; + + if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { + num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; + } + } + } + } + if (env->mcg_cap & MCG_LMCE_P) { has_msr_mcg_ext_ctl = has_msr_feature_control = true; } --- The above codes check 0xa after 0x1 and 0x7, and uses the local variable `c`, so that it doesn't need to wrap another new function. Regards, Zhao
Hi Zhao, On 3/7/25 1:15 AM, Zhao Liu wrote: >> +static void kvm_init_pmu_info(CPUX86State *env) >> +{ >> + uint32_t eax, edx; >> + uint32_t unused; >> + uint32_t limit; >> + >> + cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); > > At this stage, CPUID has already been filled and we should not use > cpu_x86_cpuid() to get the "raw" CPUID info. > > Instead, after kvm_x86_build_cpuid(), the cpuid_find_entry() helper > should be preferred. > > With cpuid_find_entry(), we don't even need to check the limit again. > >> + >> + if (limit < 0x0a) { >> + return; >> + } > > ... > >> int kvm_arch_init_vcpu(CPUState *cs) >> { >> struct { >> @@ -2267,6 +2277,8 @@ int kvm_arch_init_vcpu(CPUState *cs) >> cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); >> cpuid_data.cpuid.nent = cpuid_i; >> >> + kvm_init_pmu_info(env); >> + > > Referring what has_msr_feature_control did, what about the following > change? > > int kvm_arch_init_vcpu(CPUState *cs) > { > struct { > @@ -2277,8 +2240,6 @@ int kvm_arch_init_vcpu(CPUState *cs) > cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); > cpuid_data.cpuid.nent = cpuid_i; > > - kvm_init_pmu_info(env); > - > if (((env->cpuid_version >> 8)&0xF) >= 6 > && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == > (CPUID_MCE | CPUID_MCA)) { > @@ -2329,6 +2290,31 @@ int kvm_arch_init_vcpu(CPUState *cs) > has_msr_feature_control = true; > } > > + c = cpuid_find_entry(&cpuid_data.cpuid, 0xa, 0); > + if (c) { > + has_architectural_pmu_version = c->eax & 0xff; > + if (has_architectural_pmu_version > 0) { > + num_architectural_pmu_gp_counters = (c->eax & 0xff00) >> 8; > + > + /* > + * Shouldn't be more than 32, since that's the number of bits > + * available in EBX to tell us _which_ counters are available. > + * Play it safe. > + */ > + if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { BTW, I may need this bound checking for the PerfMonV2 patch, where the number of counters is determined by cpuid(0x80000022). > + num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; > + } > + > + if (has_architectural_pmu_version > 1) { > + num_architectural_pmu_fixed_counters = c->edx & 0x1f; > + > + if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { > + num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; > + } > + } > + } > + } > + > if (env->mcg_cap & MCG_LMCE_P) { > has_msr_mcg_ext_ctl = has_msr_feature_control = true; > } > --- > > The above codes check 0xa after 0x1 and 0x7, and uses the local variable > `c`, so that it doesn't need to wrap another new function. > How about we still wrap in another new function with &cpuid_data.cpuid as an argument? 1. In current patch, we need cpuid(0xa) to query Intel PMU info. 2. In PATCH 08/10 (AMD), we need cpuid(0x80000001) to determine PERFCORE. https://lore.kernel.org/all/20250302220112.17653-9-dongli.zhang@oracle.com/ (Otherwise, we may use ((env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_PERFCORE), but I prefer something consistent) 3. In PATCH 09/10 (AMD PerfMonV2), we need cpuid(0x80000022) to query the PerfMonV2 support, and the number of PMU counters. https://lore.kernel.org/all/20250302220112.17653-10-dongli.zhang@oracle.com/ Thank you very much! Dongli Zhang
> How about we still wrap in another new function with &cpuid_data.cpuid as > an argument? > > 1. In current patch, we need cpuid(0xa) to query Intel PMU info. > > 2. In PATCH 08/10 (AMD), we need cpuid(0x80000001) to determine PERFCORE. > > https://lore.kernel.org/all/20250302220112.17653-9-dongli.zhang@oracle.com/ > > (Otherwise, we may use ((env->features[FEAT_8000_0001_ECX] & > CPUID_EXT3_PERFCORE), but I prefer something consistent) > > > 3. In PATCH 09/10 (AMD PerfMonV2), we need cpuid(0x80000022) to query the > PerfMonV2 support, and the number of PMU counters. > > https://lore.kernel.org/all/20250302220112.17653-10-dongli.zhang@oracle.com/ Thanks, I see. This new function makes sense for me. Regards, Zhao
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 5c8a852dbd..8f293ffd61 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1959,33 +1959,6 @@ static uint32_t kvm_x86_build_cpuid(CPUX86State *env, } } - if (limit >= 0x0a) { - uint32_t eax, edx; - - cpu_x86_cpuid(env, 0x0a, 0, &eax, &unused, &unused, &edx); - - has_architectural_pmu_version = eax & 0xff; - if (has_architectural_pmu_version > 0) { - num_architectural_pmu_gp_counters = (eax & 0xff00) >> 8; - - /* Shouldn't be more than 32, since that's the number of bits - * available in EBX to tell us _which_ counters are available. - * Play it safe. - */ - if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { - num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; - } - - if (has_architectural_pmu_version > 1) { - num_architectural_pmu_fixed_counters = edx & 0x1f; - - if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { - num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; - } - } - } - } - cpu_x86_cpuid(env, 0x80000000, 0, &limit, &unused, &unused, &unused); for (i = 0x80000000; i <= limit; i++) { @@ -2085,6 +2058,43 @@ int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp) return 0; } +static void kvm_init_pmu_info(CPUX86State *env) +{ + uint32_t eax, edx; + uint32_t unused; + uint32_t limit; + + cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused); + + if (limit < 0x0a) { + return; + } + + cpu_x86_cpuid(env, 0x0a, 0, &eax, &unused, &unused, &edx); + + has_architectural_pmu_version = eax & 0xff; + if (has_architectural_pmu_version > 0) { + num_architectural_pmu_gp_counters = (eax & 0xff00) >> 8; + + /* + * Shouldn't be more than 32, since that's the number of bits + * available in EBX to tell us _which_ counters are available. + * Play it safe. + */ + if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) { + num_architectural_pmu_gp_counters = MAX_GP_COUNTERS; + } + + if (has_architectural_pmu_version > 1) { + num_architectural_pmu_fixed_counters = edx & 0x1f; + + if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) { + num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS; + } + } + } +} + int kvm_arch_init_vcpu(CPUState *cs) { struct { @@ -2267,6 +2277,8 @@ int kvm_arch_init_vcpu(CPUState *cs) cpuid_i = kvm_x86_build_cpuid(env, cpuid_data.entries, cpuid_i); cpuid_data.cpuid.nent = cpuid_i; + kvm_init_pmu_info(env); + if (((env->cpuid_version >> 8)&0xF) >= 6 && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == (CPUID_MCE | CPUID_MCA)) {
The initialization of 'has_architectural_pmu_version', 'num_architectural_pmu_gp_counters', and 'num_architectural_pmu_fixed_counters' is unrelated to the process of building the CPUID. Extract them out of kvm_x86_build_cpuid(). No functional change. Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- Changed since v1: - Still extract the code, but call them for all CPUs. target/i386/kvm/kvm.c | 66 +++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 27 deletions(-)