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? > + * 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. > +#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. > + > +#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? > + for_each_rdt_resource(r) > + pr_info("Intel RDT %s allocation %s detected\n", r->name, > + r->cdp_capable ? " (with CDP)" : ""); This want's curly braces around the loop. Thanks, tglx