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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
CC: Juergen Gross <jgross@xxxxxxxx>
---
 docs/misc/xen-command-line.pandoc | 44 +++++++++++++--------------
 xen/arch/x86/cpu/vpmu.c           | 64 +++++++++++++++++----------------------
 2 files changed, 49 insertions(+), 59 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index 6a33775..a557353 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2088,36 +2088,36 @@ 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.  Specifying any of `bts`, `ipc` or `arch` implies
+    `vpmu=true`.
 
-If 'vpmu=bts' is specified the virtualisation of the Branch Trace Store (BTS)
-feature is switched on on Intel processors supporting this feature.
+    Xen's watchdog functionality is implemented using performance counters.
+    As a result, use of the **watchdog** option will override and disable
+    vPMU.
 
-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).
+*   The `bts` option enabled performance monitoring, and permits 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.
 
-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 `ipc` option enabled 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).
 
-If a boolean is not used, combinations of flags are allowed, comma separated.
-For example, vpmu=arch,bts.
-
-Note that if **watchdog** option is also specified vpmu will be turned off.
+*   The `arch` option enables performance monitoring, but restricts the
+    counters to the pre-defined architectural events only.
 
 *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..dadccf5 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,37 @@ static DEFINE_PER_CPU(struct vcpu *, last_vcpu);
 static int __init parse_vpmu_params(const char *s)
 {
     const char *ss;
+    int rc = 0, val;
+
+    do {
+        ss = strchr(s, ',');
+        if ( !ss )
+            ss = strchr(s, '\0');
+
+        if ( (val = parse_bool(s, ss)) >= 0 )
+            opt_vpmu_enabled = val;
+        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;
 
-    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;
+        s = ss + 1;
+    } while ( *ss );
+
+    /* Selecting bts/ipc/arch forces vpmu to enabled. */
+    if ( vpmu_features )
+        opt_vpmu_enabled = true;
 
-            s = ss + 1;
-        } while ( *ss );
-        /* fall through */
-    case 1:
-        /* Default VPMU mode */
+    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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel