Message-ID: <alpine.DEB.2.10.1502241609590.3507@vshiva-Udesk>
Date:	Tue, 24 Feb 2015 16:42:10 -0800 (PST)
From:	Vikas Shivappa <vikas.shivappa@...el.com>
To:	Borislav Petkov <bp@...en8.de>
cc:	Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
	linux-kernel@...r.kernel.org, vikas.shivappa@...el.com,
	matt.fleming@...el.com, hpa@...or.com, tglx@...utronix.de,
	mingo@...nel.org, tj@...nel.org, peterz@...radead.org,
	will.auld@...el.com, dave.hansen@...el.com, andi.kleen@...el.com,
	tony.luck@...el.com, kanaka.d.juvva@...el.com
Subject: Re: [PATCH 1/7] x86/intel_rdt: Intel Cache Allocation Technology
 detection



On Tue, 24 Feb 2015, Borislav Petkov wrote:

> On Tue, Feb 24, 2015 at 03:16:38PM -0800, Vikas Shivappa wrote:
>> -#define NCAPINTS	13	/* N 32-bit words worth of info */
>> +#define NCAPINTS	14	/* N 32-bit words worth of info */
>>  #define NBUGINTS	1	/* N 32-bit bug flags */
>>
>>  /*
>> @@ -227,6 +227,7 @@
>>  #define X86_FEATURE_RTM		( 9*32+11) /* Restricted Transactional Memory */
>>  #define X86_FEATURE_CQM		( 9*32+12) /* Cache QoS Monitoring */
>>  #define X86_FEATURE_MPX		( 9*32+14) /* Memory Protection Extension */
>> +#define X86_FEATURE_RDT		( 9*32+15) /* Resource Allocation */
>>  #define X86_FEATURE_AVX512F	( 9*32+16) /* AVX-512 Foundation */
>>  #define X86_FEATURE_RDSEED	( 9*32+18) /* The RDSEED instruction */
>>  #define X86_FEATURE_ADX		( 9*32+19) /* The ADCX and ADOX instructions */
>> @@ -248,6 +249,9 @@
>>  /* Intel-defined CPU QoS Sub-leaf, CPUID level 0x0000000F:1 (edx), word 12 */
>>  #define X86_FEATURE_CQM_OCCUP_LLC (12*32+ 0) /* LLC occupancy monitoring if 1 */
>>
>> +/* Intel-defined CPU features, CPUID level 0x00000010:0 (ebx), word 13 */
>> +#define X86_FEATURE_CAT_L3	(13*32 + 1) /*Cache QOS Enforcement L3*/
> 					    ^^^^
> Spaces between comment markers and text please.

Will fix.

>
>> +
>>  /*
>>   * BUG word(s)
>>   */
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 242ceed..81d95ac 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -114,6 +114,9 @@ struct cpuinfo_x86 {
>>  	int			x86_cache_occ_scale;	/* scale to bytes */
>>  	int			x86_power;
>>  	unsigned long		loops_per_jiffy;
>> +	/* Cache Allocation Technology values */
>> +	int			x86_cat_cbmlength;
>> +	int			x86_cat_closs;
>
> Do I see it correctly, those two can be u16 each?

Yes , this can be u16 as the cbmlength and the number of clos are 4 and 16 bits 
only. Will make the change

>
>>  	/* cpuid returned max cores value: */
>>  	u16			 x86_max_cores;
>>  	u16			apicid;
>> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
>> index 6c1ca13..6c91e39 100644
>> --- a/arch/x86/kernel/cpu/Makefile
>> +++ b/arch/x86/kernel/cpu/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= perf_event_intel_uncore.o \
>>  					   perf_event_intel_uncore_nhmex.o
>>  endif
>>
>> +obj-$(CONFIG_CGROUP_RDT) 		+=intel_rdt.o
>>
>>  obj-$(CONFIG_X86_MCE)			+= mcheck/
>>  obj-$(CONFIG_MTRR)			+= mtrr/
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 9b0fb70..c5ea1dd 100644
>> --- a/arch/x86/kernel/cpu/common.c
>> +++ b/arch/x86/kernel/cpu/common.c
>> @@ -668,6 +668,21 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>>  		}
>>  	}
>>
>> +	/* Additional Intel-defined flags: level 0x00000010 */
>> +	if (c->cpuid_level >= 0x00000010) {
>> +		u32 eax, ebx, ecx, edx;
>> +
>> +		cpuid_count(0x00000010, 0, &eax, &ebx, &ecx, &edx);
>> +		c->x86_capability[13] = ebx;
>> +
>> +		if (cpu_has(c, X86_FEATURE_CAT_L3)) {
>> +
>> +			cpuid_count(0x00000010, 1, &eax, &ebx, &ecx, &edx);
>> +			c->x86_cat_closs = (edx & 0xffff) + 1;
>> +			c->x86_cat_cbmlength = (eax & 0xf) + 1;
>> +		}
>> +	}
>> +
>>  	/* AMD-defined flags: level 0x80000001 */
>>  	xlvl = cpuid_eax(0x80000000);
>>  	c->extended_cpuid_level = xlvl;
>> diff --git a/arch/x86/kernel/cpu/intel_rdt.c b/arch/x86/kernel/cpu/intel_rdt.c
>> new file mode 100644
>> index 0000000..46ce449
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/intel_rdt.c
>> @@ -0,0 +1,51 @@
>> +/*
>> + * Resource Director Technology(RDT) code
>> + *
>> + * Copyright (C) 2014 Intel Corporation
>> + *
>> + * 2014-09-10 Written by Vikas Shivappa
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * More information about RDT be found in the Intel (R) x86 Architecture
>> + * Software Developer Manual, section 17.15.
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +#include <linux/spinlock.h>
>> +
>> +static inline bool rdt_supported(struct cpuinfo_x86 *c)
>> +{
>> +	if (cpu_has(c, X86_FEATURE_RDT))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static int __init rdt_late_init(void)
>> +{
>> +	struct cpuinfo_x86 *c = &boot_cpu_data;
>> +	int maxid, cbm_len;
>> +
>> +	if (!rdt_supported(c))
>
> you can do cpu_has() directly here instead of the custom wrapper and
> drop that rdt_supported() thing.
>
>> +		return -ENODEV;
>> +
>> +	maxid = c->x86_cat_closs;
>> +	cbm_len = c->x86_cat_cbmlength;
>
> No need for those local variables, just use c->...

The above two are due to my bad habit of splitting the patches from the full 
implentation :) - these functions and variables come to use in the next patches 
in the series. But this can be changed if needed..

>
>> +
>> +	pr_info("cbmlength:%u,Closs: %u\n", cbm_len, maxid);
>
> This text message needs to be much more user-friendly if it is going out
> to the console unconditionally.
>

bit mask lengh:  number of CLOSids: ? . it should print with the module name as 
well which should help understand what it is for.

>> +
>> +	return 0;
>> +}
>> +
>> +late_initcall(rdt_late_init);
>
> Btw, this could all fit nicely in arch/x86/kernel/cpu/intel.c AFAICT
> instead of adding a separate file and you probably don't even need the
> late_initcall() even...

RDT would be the common shared resource control support in intel architecture 
and currently only has cache resource supported but can be easily extended to 
include more resources as documented in the SDM. We have a cgroup subsystem for 
rdt and donot want this to be enabled by default.

lateinit call is to ensure that this is called after the cgroup init and hence 
can be disabled if the rdt h/w support is not present - This is related to 
another caveat that cgroup_init cant handle failure gracefully , so we have to 
return success during the first css_alloc and then fail the late_initcall ..

>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 9afb971..c5004b3 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -961,6 +961,17 @@ config CPUSETS
>>
>>  	  Say N if unsure.
>>
>> +config CGROUP_RDT
>> +	bool "Resource Director Technology cgroup subsystem"
>> +	depends on X86_64
>
> depends on X86_64 && CPU_SUP_INTEL
>
> Also, this should probably also depend on CGROUP-something or so
> AFAICT...

This is with in the if CGROUPS

>
>> +	help
>> +	  This option provides framework to allocate resources like
>> +	  cache lines when applications fill cache.
>> +	  This can be used by users to configure how much cache
>> +	  that can be allocated to different applications.
>
> This help text doesn't really help me if I'm Joe User.
>

ok,Will try to make it more readable.

Regards,
Vikas

> -- 
> Regards/Gruss,
>    Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists