diff mbox series

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

Message ID 29b4fbb1045bb7cb49facfe2bc3e470fd74234bf.1698261255.git.edwin.torok@cloud.com (mailing list archive)
State New, archived
Headers show
Series vPMU bugfixes and support for PMUv5 | expand

Commit Message

Edwin Torok Oct. 25, 2023, 7:29 p.m. UTC 
From: Edwin Török <edvin.torok@citrix.com>

These are available, but were hidden by CPUID previously.

There are IR (all guests), NB and L2I (dom0 only) performance counters too
that need to be implemented, add placeholder entries for them.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 xen/arch/x86/cpu-policy.c                   | 14 +++++++++++---
 xen/arch/x86/hvm/svm/svm.c                  |  1 +
 xen/arch/x86/pv/emul-priv-op.c              |  1 +
 xen/include/public/arch-x86/cpufeatureset.h |  4 ++++
 4 files changed, 17 insertions(+), 3 deletions(-)

Comments

Jan Beulich July 22, 2024, 1:43 p.m. UTC | #1 
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
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index e38b648f7d..4242a21e1d 100644
--- 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;
+     }
+ }
 
 void calculate_raw_cpu_policy(void)
 {
@@ -391,6 +398,7 @@  static void __init calculate_host_policy(void)
     if ( vpmu_mode == XENPMU_MODE_OFF ) {
         p->basic.raw[0xa] = EMPTY_LEAF;
         p->basic.pdcm = 0;
+        p->extd.perfctrextcore = 0;
     }
 
     if ( p->extd.svm )
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 45f8e1ffd1..ecb6184f51 100644
--- 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;
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 105485bb1e..8d802b5df0 100644
--- 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 &
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 0aa3251397..5faca0bf7a 100644
--- 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 */
+XEN_CPUFEATURE(PERFCTREXTNB,  3*32+24) /*   Extended Northbridge performance counters */
 XEN_CPUFEATURE(DBEXT,         3*32+26) /*A  data breakpoint extension */
+XEN_CPUFEATURE(PERFCTREXTL2I, 3*32+28) /*   Extended L2 cache performance counters */
 XEN_CPUFEATURE(MONITORX,      3*32+29) /*   MONITOR extension (MONITORX/MWAITX) */
 
 /* Intel-defined CPU features, CPUID level 0x0000000D:1.eax, word 4 */
@@ -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 */
 XEN_CPUFEATURE(RSTR_FP_ERR_PTRS, 8*32+ 2) /*A  (F)X{SAVE,RSTOR} always saves/restores FPU Error pointers */
 XEN_CPUFEATURE(WBNOINVD,      8*32+ 9) /*   WBNOINVD instruction */
 XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by AMD) */