From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH 1/6] x86: detect and initialize Intel CAT feature Date: Mon, 16 Mar 2015 13:47:06 +0000 Message-ID: <5506ECEA020000780006A4CD@mail.emea.novell.com> References: <1426241605-4114-1-git-send-email-chao.p.peng@linux.intel.com> <1426241605-4114-2-git-send-email-chao.p.peng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426241605-4114-2-git-send-email-chao.p.peng@linux.intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Chao Peng Cc: wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, will.auld@intel.com, keir@xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org >>> On 13.03.15 at 11:13, 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