From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933809AbcJQPEH (ORCPT ); Mon, 17 Oct 2016 11:04:07 -0400 Received: from mga02.intel.com ([134.134.136.20]:22057 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932489AbcJQPD6 (ORCPT ); Mon, 17 Oct 2016 11:03:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,357,1473145200"; d="scan'208";a="20393494" Date: Mon, 17 Oct 2016 11:06:55 -0700 From: Fenghua Yu To: Thomas Gleixner Cc: Fenghua Yu , "H. Peter Anvin" , Ingo Molnar , Tony Luck , Peter Zijlstra , Stephane Eranian , Borislav Petkov , Dave Hansen , Nilay Vaish , Shaohua Li , David Carrillo-Cisneros , Ravi V Shankar , Sai Prakhya , Vikas Shivappa , linux-kernel , x86 Subject: Re: [PATCH v4 08/18] x86/intel_rdt: Pick up L3/L2 RDT parameters from CPUID Message-ID: <20161017180655.GB8999@linux.intel.com> References: <1476497548-11169-1-git-send-email-fenghua.yu@intel.com> <1476497548-11169-9-git-send-email-fenghua.yu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 17, 2016 at 03:45:32PM +0200, Thomas Gleixner wrote: > On Fri, 14 Oct 2016, Fenghua Yu wrote: > > +/** > > + * struct rdt_resource - attributes of an RDT resource > > + * @enabled: Is this feature enabled on this machine > > + * @name: Name to use in "schemata" file > > + * @max_closid: Maximum number of CLOSIDs supported > > + * @num_closid: Current number of CLOSIDs available > > + * @max_cbm: Largest Cache Bit Mask allowed > > + * @min_cbm_bits: Minimum number of bits to be set in a cache > > That should be 'number of consecutive bits', right? Change to "Minimum number of consecutive bits to be set in a cache", is that ok? It's 2 on Haswell. It's 1 in other cases i.e. L3 on Broadwell and Skylake servers etc. > > > + * bit mask > > + * @domains: All domains for this resource > > + * @num_domains: Number of domains active > > + * @msr_base: Base MSR address for CBMs > > + * @cdp_capable: Code/Data Prioritization available > > + * @cdp_enabled: Code/Data Prioritization enabled > > I wonder whether this is the proper abstraction level. We might as well do > the following: > > rdtresources[] = { > { > .name = "L3", > }, > { > .name = "L3Data", > }, > { > .name = "L3Code", > }, > > and enable either L3 or L3Data+L3Code. Not sure if that makes things > simpler, but it's definitely worth a thought or two. This way will be better than having cdp_enabled/capable for L3 and not for L2. And this doesn't change current userinterface design either, I think. > > > +#define for_each_rdt_resource(r) \ > > + for (r = rdt_resources_all; r->name; r++) \ > > + if (r->enabled) > > So the resource array must be NULL terminated, right? You might as well use > > r < rdt_resources_all + ARRAY_SIZE(rdt_resources_all) > > as the loop condition. So you avoid the NULL termination. Will do. > > > + > > +#define IA32_L3_CBM_BASE 0xc90 > > +extern struct rdt_resource rdt_resources_all[]; > > Please visually split this. CBM_BASE has nothing to do with the resource > array. > > > +#define domain_init(name) LIST_HEAD_INIT(rdt_resources_all[name].domains) > > name is really misleading here. Please use id and make this an inline > function. > > > +struct rdt_resource rdt_resources_all[] = { > > > static inline bool get_rdt_resources(void) > > { > > + struct rdt_resource *r; > > bool ret = false; > > > > if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && > > @@ -74,20 +105,53 @@ static inline bool get_rdt_resources(void) > > > > if (!boot_cpu_has(X86_FEATURE_RDT_A)) > > return false; > > - if (boot_cpu_has(X86_FEATURE_CAT_L3)) > > + if (boot_cpu_has(X86_FEATURE_CAT_L3)) { > > + union cpuid_0x10_1_eax eax; > > + union cpuid_0x10_1_edx edx; > > + u32 ebx, ecx; > > + > > + r = &rdt_resources_all[RDT_RESOURCE_L3]; > > + cpuid_count(0x00000010, 1, &eax.full, &ebx, &ecx, &edx.full); > > + r->max_closid = edx.split.cos_max + 1; > > + r->num_closid = r->max_closid; > > + r->cbm_len = eax.split.cbm_len + 1; > > + r->max_cbm = BIT_MASK(eax.split.cbm_len + 1) - 1; > > + if (boot_cpu_has(X86_FEATURE_CDP_L3)) > > + r->cdp_capable = true; > > + r->enabled = true; > > + > > ret = true; > > + } > > + if (boot_cpu_has(X86_FEATURE_CAT_L2)) { > > + union cpuid_0x10_1_eax eax; > > + union cpuid_0x10_1_edx edx; > > + u32 ebx, ecx; > > + > > + /* CPUID 0x10.2 fields are same format at 0x10.1 */ > > + r = &rdt_resources_all[RDT_RESOURCE_L2]; > > + cpuid_count(0x00000010, 2, &eax.full, &ebx, &ecx, &edx.full); > > + r->max_closid = edx.split.cos_max + 1; > > + r->num_closid = r->max_closid; > > + r->cbm_len = eax.split.cbm_len + 1; > > + r->max_cbm = BIT_MASK(eax.split.cbm_len + 1) - 1; > > + r->enabled = true; > > Copy and paste is a wonderful thing, right? > > static void rdt_get_config(int idx, struct rdt_resource *r) > { > union cpuid_0x10_1_eax eax; > union cpuid_0x10_1_edx edx; > u32 ebx, ecx; > > cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full); > r->max_closid = edx.split.cos_max + 1; > r->num_closid = r->max_closid; > r->cbm_len = eax.split.cbm_len + 1; > r->max_cbm = BIT_MASK(eax.split.cbm_len + 1) - 1; > r->enabled = true; > } > > and and the call site: > > if (boot_cpu_has(X86_FEATURE_CAT_L3)) { > rdt_get_config(1, &rdt_resources_all[RDT_RESOURCE_L3]); > if (boot_cpu_has(X86_FEATURE_CDP_L3)) > r->cdp_capable = true; > ret = true; > } > > if (boot_cpu_has(X86_FEATURE_CAT_L2)) { > rdt_get_config(2, &rdt_resources_all[RDT_RESOURCE_L2]); > ret = true; > } > > Hmm? Sure. Thanks. -Fenghua