On Tue, Nov 12, 2024 at 05:50:26PM -0800, dongli.zhang@xxxxxxxxxx wrote:
> Date: Tue, 12 Nov 2024 17:50:26 -0800
> From: dongli.zhang@xxxxxxxxxx
> 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
> >>
>
