Re: [PATCH 3/7] target/i386/kvm: init PMU information only once

[Zhao Liu <zhao1.liu@intel.com>]

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
> >>
>