Re: [PATCH v2 05/10] target/i386/kvm: extract unrelated code out of kvm_x86_build_cpuid()

[Dongli Zhang <dongli.zhang@oracle.com>]

Hi Zhao,

On 3/7/25 1:15 AM, Zhao Liu wrote:
>> +static void kvm_init_pmu_info(CPUX86State *env)
>> +{
>> +    uint32_t eax, edx;
>> +    uint32_t unused;
>> +    uint32_t limit;
>> +
>> +    cpu_x86_cpuid(env, 0, 0, &limit, &unused, &unused, &unused);
> 
> At this stage, CPUID has already been filled and we should not use
> cpu_x86_cpuid() to get the "raw" CPUID info.
> 
> Instead, after kvm_x86_build_cpuid(), the cpuid_find_entry() helper
> should be preferred.
> 
> With cpuid_find_entry(), we don't even need to check the limit again.
> 
>> +
>> +    if (limit < 0x0a) {
>> +        return;
>> +    }
> 
> ...
> 
>>  int kvm_arch_init_vcpu(CPUState *cs)
>>  {
>>      struct {
>> @@ -2267,6 +2277,8 @@ 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;
>>  
>> +    kvm_init_pmu_info(env);
>> +
> 
> Referring what has_msr_feature_control did, what about the following
> change?
> 
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      struct {
> @@ -2277,8 +2240,6 @@ 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;
> 
> -    kvm_init_pmu_info(env);
> -
>      if (((env->cpuid_version >> 8)&0xF) >= 6
>          && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
>             (CPUID_MCE | CPUID_MCA)) {
> @@ -2329,6 +2290,31 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          has_msr_feature_control = true;
>      }
> 
> +    c = cpuid_find_entry(&cpuid_data.cpuid, 0xa, 0);
> +    if (c) {
> +        has_architectural_pmu_version = c->eax & 0xff;
> +        if (has_architectural_pmu_version > 0) {
> +            num_architectural_pmu_gp_counters = (c->eax & 0xff00) >> 8;
> +
> +            /*
> +             * Shouldn't be more than 32, since that's the number of bits
> +             * available in EBX to tell us _which_ counters are available.
> +             * Play it safe.
> +             */
> +            if (num_architectural_pmu_gp_counters > MAX_GP_COUNTERS) {

BTW, I may need this bound checking for the PerfMonV2 patch, where the
number of counters is determined by cpuid(0x80000022).

> +                num_architectural_pmu_gp_counters = MAX_GP_COUNTERS;
> +            }
> +
> +            if (has_architectural_pmu_version > 1) {
> +                num_architectural_pmu_fixed_counters = c->edx & 0x1f;
> +
> +                if (num_architectural_pmu_fixed_counters > MAX_FIXED_COUNTERS) {
> +                    num_architectural_pmu_fixed_counters = MAX_FIXED_COUNTERS;
> +                }
> +            }
> +        }
> +    }
> +
>      if (env->mcg_cap & MCG_LMCE_P) {
>          has_msr_mcg_ext_ctl = has_msr_feature_control = true;
>      }
> ---
> 
> The above codes check 0xa after 0x1 and 0x7, and uses the local variable
> `c`, so that it doesn't need to wrap another new function.
> 

How about we still wrap in another new function with &cpuid_data.cpuid as
an argument?

1. In current patch, we need cpuid(0xa) to query Intel PMU info.

2. In PATCH 08/10 (AMD), we need cpuid(0x80000001) to determine PERFCORE.

https://lore.kernel.org/all/20250302220112.17653-9-dongli.zhang@oracle.com/

(Otherwise, we may use ((env->features[FEAT_8000_0001_ECX] &
CPUID_EXT3_PERFCORE), but I prefer something consistent)


3. In PATCH 09/10 (AMD PerfMonV2), we need cpuid(0x80000022) to query the
PerfMonV2 support, and the number of PMU counters.

https://lore.kernel.org/all/20250302220112.17653-10-dongli.zhang@oracle.com/

Thank you very much!

Dongli Zhang