[PATCH v2 for-4.12] x86/vpmu: Improve documentation and parsing for vpmu=

* [PATCH v2 for-4.12] x86/vpmu: Improve documentation and parsing for vpmu=
@ 2019-02-20 13:29 Andrew Cooper
  2019-02-20 13:43 ` Jan Beulich
  0 siblings, 1 reply; 2+ messages in thread
From: Andrew Cooper @ 2019-02-20 13:29 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné

The behaviour of vpmu=<bool> being exclusive of vpmu=bts|ipc|arch is odd and
contrary to Xen's normal command line parsing behaviour.  Rewrite the parsing
to use the normal form, but retain the previous behaviour where the use of
bts/ipc/arch implies vpmu=true.

Parts of the documenation are stale, most notibly the HVM-only statement.
Update it for consistency and correctness.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Have vpmu=0 superceed previous bts/ipc/arch settings, for better
   consistency similar Xen options
 * Rephrase the documentation to better indicate the relationship between the
   sub-options.
---
 docs/misc/xen-command-line.pandoc | 44 +++++++++++++------------
 xen/arch/x86/cpu/vpmu.c           | 68 ++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 58 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index c8d1ced..a03c0b4 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2108,36 +2108,38 @@ Use Virtual Processor ID support if available.  This prevents the need for TLB
 flushes on VM entry and exit, increasing performance.
 
 ### vpmu (x86)
-> `= ( <boolean> | { bts | ipc | arch [, ...] } )`
+    = List of [ <bool>, bts, ipc, arch ]
 
-> Default: `off`
+    Applicability: x86.  Default: false
 
-Switch on the virtualized performance monitoring unit for HVM guests.
+Controls for Performance Monitoring Unit virtualisation.
 
-If the current cpu isn't supported a message like
-'VPMU: Initialization failed. ...'
-is printed on the hypervisor serial log.
+Performance monitoring facilities tend to be very hardware specific, and
+provide access to a wealth of low level processor information.
 
-For some Intel Nehalem processors a quirk handling exist for an unknown
-wrong behaviour (see `handle_pmc_quirk()`).
+*   An overall boolean can be used to enable or disable vPMU support.  vPMU is
+    disabled by default.
 
-If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
-feature is switched on on Intel processors supporting this feature.
+    When enabled, guests have full access to all performance counter settings,
+    including model specific functionality.  This is a superset of the
+    functionality offered by `ipc` and/or `arch`, but a subset of the
+    functionality offered by `bts`.
 
-vpmu=ipc enables performance monitoring, but restricts the counters to the
-most minimum set possible: instructions, cycles, and reference cycles. These
-can be used to calculate instructions per cycle (IPC).
+    Xen's watchdog functionality is implemented using performance counters.
+    As a result, use of the **watchdog** option will override and disable
+    vPMU.
 
-vpmu=arch enables performance monitoring, but restricts the counters to the
-pre-defined architectural events only. These are exposed by cpuid, and listed
-in the Pre-Defined Architectural Performance Events table from the Intel 64
-and IA-32 Architectures Software Developer's Manual, Volume 3B, System
-Programming Guide, Part 2.
+*   The `bts` option enables performance monitoring, and permits additional
+    access to the Branch Trace Store controls.  BTS is an Intel feature where
+    the processor can write data into a buffer whenever a branch occurs.
+    However, as this feature isn't virtualised, a misconfiguration by the
+    guest can lock the entire system up.
 
-If a boolean is not used, combinations of flags are allowed, comma separated.
-For example, vpmu=arch,bts.
+*   The `ipc` option allows access to the most minimal set of counters
+    possible: instructions, cycles, and reference cycles.  These can be used
+    to calculate instructions per cycle (IPC).
 
-Note that if **watchdog** option is also specified vpmu will be turned off.
+*   The `arch` option allows access to the pre-defined architectural events.
 
 *Warning:*
 As the virtualisation is not 100% safe, don't use the vpmu flag on
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 13da7d0..8324d62 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -42,19 +42,9 @@ CHECK_pmu_cntr_pair;
 CHECK_pmu_data;
 CHECK_pmu_params;
 
-/*
- * "vpmu" :     vpmu generally enabled (all counters)
- * "vpmu=off"  : vpmu generally disabled
- * "vpmu=bts"  : vpmu enabled and Intel BTS feature switched on.
- * "vpmu=ipc"  : vpmu enabled for IPC counters only (most restrictive)
- * "vpmu=arch" : vpmu enabled for predef arch counters only (restrictive)
- * flag combinations are allowed, eg, "vpmu=ipc,bts".
- */
 static unsigned int __read_mostly opt_vpmu_enabled;
 unsigned int __read_mostly vpmu_mode = XENPMU_MODE_OFF;
 unsigned int __read_mostly vpmu_features = 0;
-static int parse_vpmu_params(const char *s);
-custom_param("vpmu", parse_vpmu_params);
 
 static DEFINE_SPINLOCK(vpmu_lock);
 static unsigned vpmu_count;
@@ -64,37 +54,41 @@ static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
 static int __init parse_vpmu_params(const char *s)
 {
     const char *ss;
+    int rc = 0, val;
 
-    switch ( parse_bool(s, NULL) )
-    {
-    case 0:
-        break;
-    default:
-        do {
-            ss = strchr(s, ',');
-            if ( !ss )
-                ss = strchr(s, '\0');
-
-            if ( !cmdline_strcmp(s, "bts") )
-                vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
-            else if ( !cmdline_strcmp(s, "ipc") )
-                vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
-            else if ( !cmdline_strcmp(s, "arch") )
-                vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
-            else
-                return -EINVAL;
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( (val = parse_bool(s, ss)) >= 0 )
+        {
+            opt_vpmu_enabled = val;
+            if ( !val )
+                vpmu_features = 0;
+        }
+        else if ( !cmdline_strcmp(s, "bts") )
+            vpmu_features |= XENPMU_FEATURE_INTEL_BTS;
+        else if ( !cmdline_strcmp(s, "ipc") )
+            vpmu_features |= XENPMU_FEATURE_IPC_ONLY;
+        else if ( !cmdline_strcmp(s, "arch") )
+            vpmu_features |= XENPMU_FEATURE_ARCH_ONLY;
+        else
+            rc = -EINVAL;
+
+        s = ss + 1;
+    } while ( *ss );
 
-            s = ss + 1;
-        } while ( *ss );
-        /* fall through */
-    case 1:
-        /* Default VPMU mode */
+    /* Selecting bts/ipc/arch implies vpmu=1. */
+    if ( vpmu_features )
+        opt_vpmu_enabled = true;
+
+    if ( opt_vpmu_enabled )
         vpmu_mode = XENPMU_MODE_SELF;
-        opt_vpmu_enabled = 1;
-        break;
-    }
-    return 0;
+
+    return rc;
 }
+custom_param("vpmu", parse_vpmu_params);
 
 void vpmu_lvtpc_update(uint32_t val)
 {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 2+ messages in thread