Re: [RFC PATCH 21/22] x86/AMD: fix CPUID for PerfCtr{4,5}

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.10.2023 21:29, Edwin Török wrote:
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -340,9 +340,16 @@ static void recalculate_misc(struct cpu_policy *p)
>          p->extd.raw[0x1e] = EMPTY_LEAF; /* TopoExt APIC ID/Core/Node */
>          p->extd.raw[0x1f] = EMPTY_LEAF; /* SEV */
>          p->extd.raw[0x20] = EMPTY_LEAF; /* Platform QoS */
> -        break;
> -    }
> -}
> +
> +        /* These are not implemented yet, hide from CPUID.
> +         * When they become implemented, make them available when full vpmu 
> is on */
> +        p->extd.irperf = 0;
> +        p->extd.perfctrextnb = 0;
> +        p->extd.perfctrextl2i = 0;
> +
> +         break;
> +     }
> + }

Part of this is unwanted churn: You shouldn't (wrongly) re-indent existing
code. The new comment also wants correcting for style.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1905,6 +1905,7 @@ static int cf_check svm_msr_read_intercept(
>      case MSR_AMD_FAM15H_EVNTSEL3:
>      case MSR_AMD_FAM15H_EVNTSEL4:
>      case MSR_AMD_FAM15H_EVNTSEL5:
> +    /* TODO: IRPerfCnt, L2I_* and NB_* support */
>          if ( vpmu_do_rdmsr(msr, msr_content) )
>              goto gpf;
>          break;

Imo such a comment wants indenting as it it was a statement, not a case label.

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -1156,6 +1156,7 @@ static int cf_check write_msr(
>          if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
>          {
>              vpmu_msr = true;
> +            /* fall-through */
>      case MSR_AMD_FAM15H_EVNTSEL0 ... MSR_AMD_FAM15H_PERFCTR5:
>      case MSR_K7_EVNTSEL0 ... MSR_K7_PERFCTR3:
>              if ( vpmu_msr || (boot_cpu_data.x86_vendor &

Unrelated change? And if one is to be made here, perhaps better to use the
pseudo-keyword?

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -166,7 +166,10 @@ XEN_CPUFEATURE(FMA4,          3*32+16) /*A  4 operands 
> MAC instructions */
>  XEN_CPUFEATURE(NODEID_MSR,    3*32+19) /*   NodeId MSR */
>  XEN_CPUFEATURE(TBM,           3*32+21) /*A  trailing bit manipulations */
>  XEN_CPUFEATURE(TOPOEXT,       3*32+22) /*   topology extensions CPUID leafs 
> */
> +XEN_CPUFEATURE(PERFCTREXTCORE, 3*32+23) /*A! Extended core performance 
> event-select registers */

I don't see a need for the exclamation mark.

> @@ -238,6 +241,7 @@ XEN_CPUFEATURE(EFRO,          7*32+10) /*   APERF/MPERF 
> Read Only interface */
>  
>  /* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
>  XEN_CPUFEATURE(CLZERO,        8*32+ 0) /*A  CLZERO instruction */
> +XEN_CPUFEATURE(IRPERF,        8*32+ 1) /* Instruction Retired Performance 
> Counter */

Please add two more padding blanks in the comment. I wonder anyway if the
three additions that you then only hide in calculate_host_policy() really
need adding here. They're definitely standing in the way of possibly
considering this for backport.

Arguably there may also be something missing here: If the feature was
disabled for a guest, shouldn't accesses to these MSRs also be refused?

Jan