Re: [Xen-devel] [PATCH 1/6] x86: detect and initialize Intel CAT feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.15 at 11:13, <chao.p.peng@xxxxxxxxxxxxxxx> wrote:
> @@ -1112,6 +1117,12 @@ 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.
> +  * `socket_num` indicates socket number. Detecte automatically at boot time
> +    if not specified(0). Useful for CPU hot-plug case.

While saying something, at least for me what is being said doesn't at
all make clear what "socket number" here is: The number of a specific
socket? The number of sockets? Yet something else? Without knowing
what it means by _just_ reading this description, people won't know
what to use the command line option for.

> @@ -58,15 +69,31 @@ static void __init parse_psr_param(char *s)
>                                      val_str);
>              }
>          }
> +        else if ( !strcmp(s, "cat") )
> +        {
> +            if ( !val_str )
> +                opt_psr |= PSR_CAT;
> +            else
> +            {
> +                int val_int = parse_bool(val_str);
> +                if ( val_int == 1 )

Blank line between declarations and statements please.

> -static void __init init_psr_cmt(unsigned int rmid_max)
> +static void __init psr_cmt_init(unsigned int rmid_max)

Is this renaming really an integral part of this patch?

> +static void do_cat_cpu_init(void* data)

* and blank are reversed here.

> +{
> +    unsigned int eax, ebx, ecx, edx;
> +    struct psr_cat_socket_info *info;
> +
> +    cpuid_count(0x10, 0, &eax, &ebx, &ecx, &edx);
> +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> +    {
> +        info = data;
> +
> +        cpuid_count(0x10, 1, &eax, &ebx, &ecx, &edx);
> +        info->cbm_len = (eax & 0x1f) + 1;
> +        info->cos_max = (edx & 0xffff);
> +
> +        info->enabled = 1;
> +        printk(XENLOG_INFO "CAT: enabled on socket %d, cos_max:%d, 
> cbm_len:%d\n",
> +               (int)(info - cat_socket_info), info->cos_max, info->cbm_len);

Bogus cast. Also the format specifier to output "unsigned int" is %u.

> +static void cat_cpu_init(unsigned int cpu)
> +{
> +    struct psr_cat_socket_info *info;
> +    unsigned int socket;
> +    const struct cpuinfo_x86 *c;
> +
> +    socket = cpu_to_socket(cpu);
> +    if ( socket >= opt_socket_num )
> +    {
> +        printk(XENLOG_WARNING "CAT: disabled on socket %d because socket_num 
> is %d\n",
> +               socket, opt_socket_num);
> +        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;
> +
> +    if ( cpu == smp_processor_id() )
> +        do_cat_cpu_init(info);
> +    else
> +        on_selected_cpus(cpumask_of(cpu), do_cat_cpu_init, info, 0);

Hmm, using an IPI here seems odd. Is there a reason to can't hook
this onto CPU_STARTING instead of CPU_ONLINE?

> +static int cpu_callback(
> +    struct notifier_block *nfb, unsigned long action, void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +
> +    switch ( action )
> +    {
> +    case CPU_ONLINE:
> +        (void)cat_cpu_init(cpu);

Bogus cast.

> +        break;
> +    default:
> +        break;

Pointless default case.

> +static unsigned int get_max_socket(void)
> +{
> +    unsigned int cpu, max_apicid = boot_cpu_physical_apicid;
> +
> +    for_each_present_cpu(cpu)
> +        if (max_apicid < x86_cpu_to_apicid[cpu])

Coding style.

> +            max_apicid = x86_cpu_to_apicid[cpu];
> +
> +    return apicid_to_socket(max_apicid);

Since when is the socket with the highest numbered APIC ID
the highest numbered socket? I think the whole function needs
to act on socket numbers only.

> +static void __init psr_cat_init(void)
> +{
> +    if ( opt_socket_num == 0 )
> +        opt_socket_num = get_max_socket() + 1;
> +    else if ( opt_socket_num > NR_CPUS )
> +         printk(XENLOG_WARNING "socket_num > NR_CPUS(%d), set to NR_CPUS.\n",
> +                NR_CPUS);
> +
> +    BUG_ON(opt_socket_num == 0);

Is that really a useful check? It triggering would imply
get_max_sockets() returned -1...

Jan

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