| Message ID | 20241104094119.4131-4-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386/kvm/pmu: Enhancement, Bugfix and Cleanup | expand |
Hi Dongli, > int kvm_arch_init_vcpu(CPUState *cs) > { > struct { > @@ -2237,6 +2247,13 @@ 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; > > + /* > + * Initialize PMU information only once for the first vCPU. > + */ > + if (cs == first_cpu) { > + kvm_init_pmu_info(env); > + } > + Thank you for the optimization. However, I think it’s not necessary because: * This is not a hot path and not a performance bottleneck. * Many CPUID leaves are consistent across CPUs, and 0xA is just one of them. * And encoding them all in kvm_x86_build_cpuid() is a common pattern. Separating out 0xa disrupts code readability and fragments the CPUID encoding. Therefore, code maintainability and correctness might be more important here, than performance concern. > if (((env->cpuid_version >> 8)&0xF) >= 6 > && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == > (CPUID_MCE | CPUID_MCA)) { > -- > 2.39.3 >
Hi Zhao, On 11/10/24 7:29 AM, Zhao Liu wrote: > Hi Dongli, > >> int kvm_arch_init_vcpu(CPUState *cs) >> { >> struct { >> @@ -2237,6 +2247,13 @@ 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; >> >> + /* >> + * Initialize PMU information only once for the first vCPU. >> + */ >> + if (cs == first_cpu) { >> + kvm_init_pmu_info(env); >> + } >> + > > Thank you for the optimization. However, I think it’s not necessary > because: > > * This is not a hot path and not a performance bottleneck. > * Many CPUID leaves are consistent across CPUs, and 0xA is just one of them. > * And encoding them all in kvm_x86_build_cpuid() is a common pattern. > Separating out 0xa disrupts code readability and fragments the CPUID encoding. > > Therefore, code maintainability and correctness might be more important here, > than performance concern. I am going to remove this patch in v2. Just a reminder, we may have more code in this function by other patches, including the initialization of both Intel and AMD PMU infortmation (PerfMonV2). Thank you very much! Dongli Zhang > >> if (((env->cpuid_version >> 8)&0xF) >= 6 >> && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == >> (CPUID_MCE | CPUID_MCA)) { >> -- >> 2.39.3 >>
On Tue, Nov 12, 2024 at 05:50:26PM -0800, dongli.zhang@oracle.com wrote: > Date: Tue, 12 Nov 2024 17:50:26 -0800 > From: dongli.zhang@oracle.com > Subject: Re: [PATCH 3/7] target/i386/kvm: init PMU information only once > > Hi Zhao, > > On 11/10/24 7:29 AM, Zhao Liu wrote: > > Hi Dongli, > > > >> int kvm_arch_init_vcpu(CPUState *cs) > >> { > >> struct { > >> @@ -2237,6 +2247,13 @@ 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; > >> > >> + /* > >> + * Initialize PMU information only once for the first vCPU. > >> + */ > >> + if (cs == first_cpu) { > >> + kvm_init_pmu_info(env); > >> + } > >> + > > > > Thank you for the optimization. However, I think it’s not necessary > > because: > > > > * This is not a hot path and not a performance bottleneck. > > * Many CPUID leaves are consistent across CPUs, and 0xA is just one of them. > > * And encoding them all in kvm_x86_build_cpuid() is a common pattern. > > Separating out 0xa disrupts code readability and fragments the CPUID encoding. > > > > Therefore, code maintainability and correctness might be more important here, > > than performance concern. > > I am going to remove this patch in v2. > > Just a reminder, we may have more code in this function by other patches, > including the initialization of both Intel and AMD PMU infortmation (PerfMonV2). Yes, I mean we can just remove "first_cpu" check and move this function into kvm_x86_build_cpuid() again. Your function wrapping is fine for me. > Dongli Zhang > > > > >> if (((env->cpuid_version >> 8)&0xF) >= 6 > >> && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) == > >> (CPUID_MCE | CPUID_MCA)) { > >> -- > >> 2.39.3 > >> >
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index ed73b1e7e0..5c0276f889 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1961,33 +1961,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++) { @@ -2055,6 +2028,43 @@ full: abort(); } +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 { @@ -2237,6 +2247,13 @@ 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; + /* + * Initialize PMU information only once for the first vCPU. + */ + if (cs == first_cpu) { + 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)) {
Currently, the 'has_architectural_pmu_version', 'num_architectural_pmu_gp_counters', and 'num_architectural_pmu_fixed_counters' are shared globally across all vCPUs and are initialized during the setup of each vCPU. To optimize, initialize PMU information only once for the first vCPU. Additionally, the code extracted from kvm_x86_build_cpuid() is unrelated to the process of building the CPUID. Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- target/i386/kvm/kvm.c | 71 +++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 27 deletions(-)