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
