Re: [Xen-devel] [PATCH v2 3/7] x86: detect and initialize Intel CAT feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.15 at 11:41, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1090,9 +1090,9 @@ This option can be specified more than once (up to 8 
> times at present).
>  > `= <integer>`
>  
>  ### psr (Intel)
> -> `= List of ( cmt:<boolean> | rmid_max:<integer> )`
> +> `= List of ( cmt:<boolean> | rmid_max:<integer> | cat:<boolean> | 
> num_sockets:<interger> )`
>  
> -> Default: `psr=cmt:0,rmid_max:255`
> +> Default: `psr=cmt:0,rmid_max:255,cat:0,num_sockets:0(Detect at boot time)`

This "Detect at boot time" clearly doesn't belong on the "Default:" line.

> @@ -1112,6 +1117,16 @@ The following resources are available:
>    total/local memory bandwidth. Follow the same options with Cache Monitoring
>    Technology.
>  
> +* Cache Alllocation Technology (Broadwell and later).  Information regarding
> +  the cache allocation.
> +  * `cat` instructs Xen to enable/disable Cache Allocation Technology.
> +  * `num_sockets` indicates the number of available sockets for CAT feature
> +    detection. All the sockets up to num_sockets will be checked for CAT
> +    feature. The value is normally detected at boot time automatically if not
> +    specified(0). While the value may need to be specified manually for CPU
> +    hot-plug scenario in which case the automatic sockets number detection 
> may
> +    be not correct.

As mentioned before, the hotplug case should be taken care of as
much as possible to avoid someone having to use the option. I.e.
this ...

> +static unsigned int get_max_socket(void)
> +{
> +    unsigned int cpu, socket = 0;
> +
> +    for_each_present_cpu(cpu)
> +        if ( socket < cpu_to_socket(cpu) )
> +            socket = cpu_to_socket(cpu);
> +
> +    return socket;
> +}

... is too simplistic.

> @@ -204,9 +233,66 @@ void psr_ctxt_switch_to(struct domain *d)
>      psr_assoc_reg_write(psra, reg);
>  }
>  
> +static void cat_cpu_init(unsigned int cpu)
> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    struct psr_cat_socket_info *info;
> +    unsigned int socket;
> +    const struct cpuinfo_x86 *c;
> +
> +    socket = cpu_to_socket(cpu);
> +    if ( socket >= opt_num_sockets )
> +    {
> +        printk(XENLOG_WARNING "CAT: disabled on socket %d as num_sockets is 
> %d\n",
> +               socket, opt_num_sockets);
> +        return;
> +    }
> +
> +    info = cat_socket_info + socket;
> +
> +    /* Avoid initializing more than one times for the same socket. */
> +    if ( test_and_set_bool(info->initialized) )
> +        return;
> +
> +    c = cpu_data + cpu;
> +    if ( !cpu_has(c, X86_FEATURE_CAT) )
> +        return;
> +
> +    cpuid_count(0x10, 0, &eax, &ebx, &ecx, &edx);
> +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        cpuid_count(0x10, 1, &eax, &ebx, &ecx, &edx);

If numbers like the 0x10 here repeat, please give them names so one
can associate all uses with one another.

>  static void psr_cpu_init(unsigned int cpu)
>  {
> -    psr_assoc_init();
> +    if ( cat_socket_info )
> +        cat_cpu_init(cpu);
> +
> +    if (  psr_cmt_enabled() )
> +        psr_assoc_init();

So the previous patch put the same conditional inside psr_assoc_init().
Please don't add such obviously redundant checks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel