Hi Zhao,
On 3/10/25 12:47 AM, Zhao Liu wrote:
> (+EwanHai for zhaoxin case...)
>
> ...
>
[snip]
>> +
>> + /*
>> + * Performance-monitoring supported from K7 and later.
>> + */
>> + if (family < 6) {
>> + return;
>> + }
>
> I understand we can get family by object_property_get_int() helper:
Thank you very much for suggestion!
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 4902694129f9..ff08c7bfee6c 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2106,27 +2106,22 @@ static void kvm_init_pmu_info_intel(CPUX86State *env)
> }
> }
>
[snip]
>
> @@ -2197,7 +2192,7 @@ static void kvm_init_pmu_info(CPUState *cs)
> if (IS_INTEL_CPU(env)) {
> kvm_init_pmu_info_intel(env);
> } else if (IS_AMD_CPU(env)) {
> - kvm_init_pmu_info_amd(env);
> + kvm_init_pmu_info_amd(cpu);
> }
> }
>
> ---
> Then for consistency, kvm_init_pmu_info_intel() could also accept
> "X86CPU *cpu" as the argument.
Sure. Will do.
>
>> + has_pmu_version = 1;
>> +
>> + cpu_x86_cpuid(env, 0x80000001, 0, &unused, &unused, &ecx, &unused);
>> +
>> + if (!(ecx & CPUID_EXT3_PERFCORE)) {
>> + num_pmu_gp_counters = AMD64_NUM_COUNTERS;
>> + return;
>> + }
>> +
>> + num_pmu_gp_counters = AMD64_NUM_COUNTERS_CORE;
>> +}
>
> ...
>
>> +static void kvm_init_pmu_info(CPUState *cs)
>> +{
>> + X86CPU *cpu = X86_CPU(cs);
>> + CPUX86State *env = &cpu->env;
>> +
>> + /*
>> + * The PMU virtualization is disabled by kvm.enable_pmu=N.
>> + */
>> + if (kvm_pmu_disabled) {
>> + return;
>> + }
>
> As I said in patch 7, we could return an error instead.
Sure.
In addition, as we have discussed, we are going to pass cpuid_data.cpuid as
argument, so that we don't need cpu_x86_cpuid() any longer.
>
>> + /*
>> + * It is not supported to virtualize AMD PMU registers on Intel
>> + * processors, nor to virtualize Intel PMU registers on AMD processors.
>> + */
>> + if (!is_same_vendor(env)) {
>
> Here it deserves a warning like:
>
> error_report("host doesn't support requested feature: vPMU\n");
Sure. Will do.
>
>> + return;
>> + }
>>
>> + /*
>> + * 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.
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!
Dongli Zhang
