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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.02.19 at 12:41, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- 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

DYM "enables" here (and also below for `ipc`)?

> --- 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;

If you want to retain original behavior, the condition here would need
to be "!rc && vpmu_features". It's not clear whether your modification
in this regard is intentional.

Jan


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