Re: [PATCH v2 08/10] target/i386/kvm: reset AMD PMU registers during VM reset

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

Hi Dongli,

> >> +    /*
> >> +     * If KVM_CAP_PMU_CAPABILITY is not supported, there is no way to
> >> +     * disable the AMD pmu virtualization.
> >> +     *
> >> +     * If KVM_CAP_PMU_CAPABILITY is supported !cpu->enable_pmu
> >> +     * indicates the KVM has already disabled the PMU virtualization.
> >> +     */
> >> +    if (has_pmu_cap && !cpu->enable_pmu) {
> >> +        return;
> >> +    }
> > 
> > Could we only check "cpu->enable_pmu" at the beginning of this function?
> > then if pmu is already disabled, we don't need to initialize the pmu info.
> 
> I don't think so. There is a case:
> 
> - cpu->enable_pmu = false. (That is, "-cpu host,-pmu").
> - But for KVM prior v5.18 that KVM_CAP_PMU_CAPABILITY doesn't exist.
> 
> There is no way to disable vPMU. To determine based on only
> "!cpu->enable_pmu" doesn't work.

Ah, I didn't get your point here. When QEMU user has already disabled
PMU, why we still need to continue initialize PMU info and save/load PMU
MSRs? In this case, user won't expect vPMU could work.

> It works only when "!cpu->enable_pmu" and KVM_CAP_PMU_CAPABILITY exists.
> 
> 
> We may still need a static global variable here to indicate where
> "kvm.enable_pmu=N" (as discussed in PATCH 07).
>
> > 
> >> +    if (IS_INTEL_CPU(env)) {
> > 
> > Zhaoxin also supports architectural PerfMon in 0xa.
> > 
> > I'm not sure if this check should also involve Zhaoxin CPU, so cc
> > zhaoxin guys for double check.
> 
> Sure for both here and below 'ditto'. Thank you very much!

Per the Linux commit 3a4ac121c2cac, Zhaoxin mostly follows Intel
Architectural PerfMon-v2. Afterall, before this patch, these PMU things
didn't check any vendor, so I suppose vPMU may could work for Zhaoxin as
well. Therefore, its' better to consider Zhaoxin when you check Intel
CPU, which can help avoid introducing some regressions.

Thanks,
Zhao