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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/02/2019 13:53, Jan Beulich wrote:
>>>> 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`)?

Oops yes.  Fixed.

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

Oh - that wasn't intentional.

An alternative, now I think about it, is to just have the <bool>=false
case clear vpmu_features.  This is new behaviour, but it is more
consistent with how other options work, and it wasn't expressable before.

~Andrew

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