Re: [PATCH v1 5/9] target/i386/kvm: Use AMD PMU MSR paths for Hygon

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

Hello Tina,

> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 8e2bfbbe1d..215e0c8f03 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2175,6 +2175,21 @@ static void kvm_init_pmu_info_amd(struct kvm_cpuid2 *cpuid, X86CPU *cpu)
>      }
>  }
>  
> +/*
> + * KVM exposes the AMD PMU CPUID/MSR layout for Hygon guests, so QEMU must
> + * use the AMD PMU setup and MSR state paths for Hygon too.
> + */
> +static bool kvm_pmu_uses_amd_msrs(const CPUX86State *env)
> +{
> +    return IS_AMD_CPU(env) || IS_HYGON_CPU(env);
> +}
> +
> +static bool host_cpu_uses_amd_pmu_msrs(const char *host_vendor)
> +{
> +    return g_str_equal(host_vendor, CPUID_VENDOR_AMD) ||
> +           g_str_equal(host_vendor, CPUID_VENDOR_HYGON);
> +}
>
>  static bool is_host_compat_vendor(CPUX86State *env)
>  {
>      char host_vendor[CPUID_VENDOR_SZ + 1];
> @@ -2191,8 +2206,8 @@ static bool is_host_compat_vendor(CPUX86State *env)
>          return true;
>      }
>
> -    return g_str_equal(host_vendor, CPUID_VENDOR_AMD) &&
> -           IS_AMD_CPU(env);
> +    return host_cpu_uses_amd_pmu_msrs(host_vendor) &&
> +           kvm_pmu_uses_amd_msrs(env);
>  }

It seems to be doing the same thing as the previous Intel & Zhaoxin
compatibility check.

To unify similar checks, how about we abstract them into the "PMU vendor
family"?

For example,

typedef enum {
    X86_PMU_VENDOR_UNKNOWN,
    X86_PMU_VENDOR_INTEL,
    X86_PMU_VENDOR_AMD,
} X86PMUVendor;

static X86PMUVendor x86_cpu_pmu_vendor(const CPUX86State *env)
{
    if (IS_INTEL_CPU(env) || IS_ZHAOXIN_CPU(env)) {
        return X86_PMU_VENDOR_INTEL;
    }
    if (IS_AMD_CPU(env) || IS_HYGON_CPU(env)) {
        return X86_PMU_VENDOR_AMD;
    }
    return X86_PMU_VENDOR_UNKNOWN;
}

static X86PMUVendor x86_host_pmu_vendor(void)
{
    char host_vendor[CPUID_VENDOR_SZ + 1];

    host_cpu_vendor_fms(host_vendor, NULL, NULL, NULL);

    if (g_str_equal(host_vendor, CPUID_VENDOR_INTEL) ||
        g_str_equal(host_vendor, CPUID_VENDOR_ZHAOXIN1) ||
        g_str_equal(host_vendor, CPUID_VENDOR_ZHAOXIN2)) {
        return X86_PMU_VENDOR_INTEL;
    }

    if (g_str_equal(host_vendor, CPUID_VENDOR_AMD) ||
        g_str_equal(host_vendor, CPUID_VENDOR_HYGON)) {
        return X86_PMU_VENDOR_AMD;
    }
    return X86_PMU_VENDOR_UNKNOWN;
}

/*
 * The guest vPMU can be virtualized only when the host and guest's PMU
 * architectures are compatible.
 */
static bool is_host_compat_vendor(CPUX86State *env)
{
    X86PMUVendor guest = x86_cpu_pmu_vendor(env);

    return guest != X86_PMU_VENDOR_UNKNOWN && guest == x86_host_pmu_vendor();
}

>  static void kvm_init_pmu_info(struct kvm_cpuid2 *cpuid, X86CPU *cpu)
> @@ -2222,7 +2237,7 @@ static void kvm_init_pmu_info(struct kvm_cpuid2 *cpuid, X86CPU *cpu)
>  
>      if (IS_INTEL_CPU(env) || IS_ZHAOXIN_CPU(env)) {
>          kvm_init_pmu_info_intel(cpuid);
> -    } else if (IS_AMD_CPU(env)) {
> +    } else if (kvm_pmu_uses_amd_msrs(env)) {
>          kvm_init_pmu_info_amd(cpuid, cpu);
>      }
>  }

Then such CPU check can be replaced with:

    switch (x86_cpu_pmu_vendor(env)) {
    case X86_PMU_VENDOR_INTEL:
        kvm_init_pmu_info_intel(cpuid);
        break;
    case X86_PMU_VENDOR_AMD:
        kvm_init_pmu_info_amd(cpuid, cpu);
        break;
    default:
        g_assert_not_reached();
    }

The latter can also be replaced with "x86_cpu_pmu_vendor(env) ==
X86_PMU_VENDOR_AMD" instead of the "kvm_pmu_uses_amd_msrs(env)" you're
currently using.

What do you think?

Regards,
Zhao