[PATCH 0/3] x86, cacheinfo, amd: L3 Cache Index Disable fixes

[PATCH 0/3] x86, cacheinfo, amd: L3 Cache Index Disable fixes

[Borislav Petkov]

Hi,

these are a bunch of fixes which correct the disabling of L3 cache
indexes. This is needed, for example, in the case when excessive
correctable L3 error rates are observed. In a later step, this
functionality will be connected to the mcheck mechanism and used to
evaluate occurring L3 tag and data array errors and then disable the
respective failing L3 cache regions in order to defer system failure
before a sysop can intervene.

Those patches are also good -stable candidates.

Please apply,
thanks.

[PATCH 1/3] x86, cacheinfo: Fix disabling of L3 cache indexes

[Borislav Petkov]

* Correct the masks used for writing the cache index disable indexes.
* Do not turn off L3 scrubber - it is not necessary.
* Make sure wbinvd is executed on the same node where the L3 is.
* Check for out-of-bounds values written to the registers.
* Make show_cache_disable hex values unambiguous
* Check for Erratum #388

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |   34 ++++++++++++++++++++------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index fc6c8ef..66932f1 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -299,8 +299,10 @@ amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
 	if (boot_cpu_data.x86 == 0x11)
 		return;
 
-	/* see erratum #382 */
-	if ((boot_cpu_data.x86 == 0x10) && (boot_cpu_data.x86_model < 0x8))
+	/* see errata #382 and #388 */
+	if ((boot_cpu_data.x86 == 0x10) &&
+	    ((boot_cpu_data.x86_model < 0x8) ||
+	     (boot_cpu_data.x86_mask  < 0x1)))
 		return;
 
 	this_leaf->can_disable = 1;
@@ -726,18 +728,23 @@ static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
 		return -EINVAL;
 
 	pci_read_config_dword(dev, 0x1BC + index * 4, &reg);
-	return sprintf(buf, "%x\n", reg);
+	return sprintf(buf, "0x%08x\n", reg);
 }
 
 #define SHOW_CACHE_DISABLE(index)					\
 static ssize_t								\
-show_cache_disable_##index(struct _cpuid4_info *this_leaf, char *buf)  	\
+show_cache_disable_##index(struct _cpuid4_info *this_leaf, char *buf)	\
 {									\
 	return show_cache_disable(this_leaf, buf, index);		\
 }
 SHOW_CACHE_DISABLE(0)
 SHOW_CACHE_DISABLE(1)
 
+static void __wbinvd(void *dummy)
+{
+	asm volatile("wbinvd" : : : "memory");
+}
+
 static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	const char *buf, size_t count, unsigned int index)
 {
@@ -745,7 +752,9 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	int node = cpu_to_node(cpu);
 	struct pci_dev *dev = node_to_k8_nb_misc(node);
 	unsigned long val = 0;
-	unsigned int scrubber = 0;
+
+#define SUBCACHE_MASK	(3UL << 20)
+#define SUBCACHE_INDEX	0xfff
 
 	if (!this_leaf->can_disable)
 		return -EINVAL;
@@ -759,21 +768,20 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 	if (strict_strtoul(buf, 10, &val) < 0)
 		return -EINVAL;
 
-	val |= 0xc0000000;
-
-	pci_read_config_dword(dev, 0x58, &scrubber);
-	scrubber &= ~0x1f000000;
-	pci_write_config_dword(dev, 0x58, scrubber);
+	/* do not allow writes outside of allowed bits */
+	if (val & ~(SUBCACHE_MASK | SUBCACHE_INDEX))
+		return -EINVAL;
 
-	pci_write_config_dword(dev, 0x1BC + index * 4, val & ~0x40000000);
-	wbinvd();
+	val |= BIT(30);
 	pci_write_config_dword(dev, 0x1BC + index * 4, val);
+	smp_call_function_single(cpu, __wbinvd, NULL, 1);
+	pci_write_config_dword(dev, 0x1BC + index * 4, val | BIT(31));
 	return count;
 }
 
 #define STORE_CACHE_DISABLE(index)					\
 static ssize_t								\
-store_cache_disable_##index(struct _cpuid4_info *this_leaf,	     	\
+store_cache_disable_##index(struct _cpuid4_info *this_leaf,		\
 			    const char *buf, size_t count)		\
 {									\
 	return store_cache_disable(this_leaf, buf, count, index);	\
-- 
1.6.6

Re: [PATCH 1/3] x86, cacheinfo: Fix disabling of L3 cache indexes

[H. Peter Anvin]

On 01/19/2010 03:07 AM, Borislav Petkov wrote:
>  
> +static void __wbinvd(void *dummy)
> +{
> +	asm volatile("wbinvd" : : : "memory");
> +}
> +

[...]

> +	smp_call_function_single(cpu, __wbinvd, NULL, 1);

I really don't like this combination.

First of all, it's an asm version of an instruction we already have
macros for.  This should probably just be wbinvd(), or *possibly*
native_wbinvd(), although that would have to be justified -- especially
since the preexisting code used wbinvd().

Second, it's pretty obvious that the only reason for this function at
all is to provide a wrapper that can be passed to smp_call_function*().
 It would be a lot cleaner to have a small function wbinvd_on_cpu(cpu)
as a wrapper for the higher-order functionality.

	-hpa

Re: [PATCH 1/3] x86, cacheinfo: Fix disabling of L3 cache indexes

[Borislav Petkov]

On Wed, Jan 20, 2010 at 03:04:46PM -0800, H. Peter Anvin wrote:
> On 01/19/2010 03:07 AM, Borislav Petkov wrote:
> >  
> > +static void __wbinvd(void *dummy)
> > +{
> > +	asm volatile("wbinvd" : : : "memory");
> > +}
> > +
> 
> [...]
> 
> > +	smp_call_function_single(cpu, __wbinvd, NULL, 1);
> 
> I really don't like this combination.
> 
> First of all, it's an asm version of an instruction we already have
> macros for.  This should probably just be wbinvd(), or *possibly*
> native_wbinvd(), although that would have to be justified -- especially
> since the preexisting code used wbinvd().

The preexisting code using wbinvd is a bug since wbinvd has to happen on
a core which contains the L3 cache whose indices(!) we disable.

But yeah, a wbinvd smp version is a bit problematic since
smp_call_function_* expects a function pointer which has a void *
argument but native_wbinvd() has no args. By the way, there's a similar
thing in <drivers/char/agp/intel-agp.c::do_wbinvd()>

> Second, it's pretty obvious that the only reason for this function at
> all is to provide a wrapper that can be passed to smp_call_function*().
>  It would be a lot cleaner to have a small function wbinvd_on_cpu(cpu)
> as a wrapper for the higher-order functionality.

Done, see my next patch series.

-- 
Regards/Gruss,
Boris.

-
Advanced Micro Devices, Inc.
Operating Systems Research Center

Re: [PATCH 1/3] x86, cacheinfo: Fix disabling of L3 cache indexes

[H. Peter Anvin]

On 01/21/2010 08:30 AM, Borislav Petkov wrote:
> 
> The preexisting code using wbinvd is a bug since wbinvd has to happen on
> a core which contains the L3 cache whose indices(!) we disable.
> 

OK, please make a note in the code as to why it is (if it is) correct
not to use the PV version here... I'm not sure the above explains why it
should invoke native_wbinvd() and not wbinvd() -- the fact that is has
to be done on the right CPU is another matter, of course.

> But yeah, a wbinvd smp version is a bit problematic since
> smp_call_function_* expects a function pointer which has a void *
> argument but native_wbinvd() has no args. By the way, there's a similar
> thing in <drivers/char/agp/intel-agp.c::do_wbinvd()>

Saw that, thank you for handling that.

>> Second, it's pretty obvious that the only reason for this function at
>> all is to provide a wrapper that can be passed to smp_call_function*().
>>  It would be a lot cleaner to have a small function wbinvd_on_cpu(cpu)
>> as a wrapper for the higher-order functionality.
> 
> Done, see my next patch series.
> 

I wrote some comments on that before I saw this reply.  I think it has
some code cleanliness issues (noted in comments), but otherwise it's a
lot better.

	-hpa

[PATCH 2/3] x86, cacheinfo: Add cache index disable sysfs attrs only to L3 caches

[Borislav Petkov]

The cache_disable_[01] attribute in

/sys/devices/system/cpu/cpu?/cache/index[0-3]/

is enabled on all cache levels although only L3 supports it. Add it only
to the cache level that actually supports it.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |   35 ++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 66932f1..53e28e4 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -814,16 +814,24 @@ static struct _cache_attr cache_disable_0 = __ATTR(cache_disable_0, 0644,
 static struct _cache_attr cache_disable_1 = __ATTR(cache_disable_1, 0644,
 		show_cache_disable_1, store_cache_disable_1);
 
+#define DEFAULT_SYSFS_CACHE_ATTRS	\
+	&type.attr,			\
+	&level.attr,			\
+	&coherency_line_size.attr,	\
+	&physical_line_partition.attr,	\
+	&ways_of_associativity.attr,	\
+	&number_of_sets.attr,		\
+	&size.attr,			\
+	&shared_cpu_map.attr,		\
+	&shared_cpu_list.attr
+
 static struct attribute *default_attrs[] = {
-	&type.attr,
-	&level.attr,
-	&coherency_line_size.attr,
-	&physical_line_partition.attr,
-	&ways_of_associativity.attr,
-	&number_of_sets.attr,
-	&size.attr,
-	&shared_cpu_map.attr,
-	&shared_cpu_list.attr,
+	DEFAULT_SYSFS_CACHE_ATTRS,
+	NULL
+};
+
+static struct attribute *default_l3_attrs[] = {
+	DEFAULT_SYSFS_CACHE_ATTRS,
 	&cache_disable_0.attr,
 	&cache_disable_1.attr,
 	NULL
@@ -916,6 +924,7 @@ static int __cpuinit cache_add_dev(struct sys_device * sys_dev)
 	unsigned int cpu = sys_dev->id;
 	unsigned long i, j;
 	struct _index_kobject *this_object;
+	struct _cpuid4_info   *this_leaf;
 	int retval;
 
 	retval = cpuid4_cache_sysfs_init(cpu);
@@ -934,6 +943,14 @@ static int __cpuinit cache_add_dev(struct sys_device * sys_dev)
 		this_object = INDEX_KOBJECT_PTR(cpu, i);
 		this_object->cpu = cpu;
 		this_object->index = i;
+
+		this_leaf = CPUID4_INFO_IDX(cpu, i);
+
+		if (this_leaf->can_disable)
+			ktype_cache.default_attrs = default_l3_attrs;
+		else
+			ktype_cache.default_attrs = default_attrs;
+
 		retval = kobject_init_and_add(&(this_object->kobj),
 					      &ktype_cache,
 					      per_cpu(ici_cache_kobject, cpu),
-- 
1.6.6

[PATCH 3/3] x86, cacheinfo: Calculate L3 indexes

[Borislav Petkov]

We need to know the valid L3 indexes interval when disabling them over
/sysfs. Do that when the core is brought online and add boundary checks
to the sysfs .store attribute.

Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 arch/x86/kernel/cpu/intel_cacheinfo.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 53e28e4..fcb29c8 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -151,6 +151,7 @@ struct _cpuid4_info {
 	union _cpuid4_leaf_ecx ecx;
 	unsigned long size;
 	unsigned long can_disable;
+	unsigned int l3_indexes;
 	DECLARE_BITMAP(shared_cpu_map, NR_CPUS);
 };
 
@@ -161,6 +162,7 @@ struct _cpuid4_info_regs {
 	union _cpuid4_leaf_ecx ecx;
 	unsigned long size;
 	unsigned long can_disable;
+	unsigned int l3_indexes;
 };
 
 unsigned short			num_cache_leaves;
@@ -290,6 +292,29 @@ amd_cpuid4(int leaf, union _cpuid4_leaf_eax *eax,
 		(ebx->split.ways_of_associativity + 1) - 1;
 }
 
+static unsigned int __cpuinit amd_calc_l3_indexes(void)
+{
+	/*
+	 * We're called over smp_call_function_single() and therefore
+	 * are on the correct cpu.
+	 */
+	int cpu = smp_processor_id();
+	int node = cpu_to_node(cpu);
+	struct pci_dev *dev = node_to_k8_nb_misc(node);
+	unsigned int sc0, sc1, sc2, sc3;
+	u32 val;
+
+	pci_read_config_dword(dev, 0x1C4, &val);
+
+	/* calculate subcache sizes */
+	sc0 = !(val & BIT(0));
+	sc1 = !(val & BIT(4));
+	sc2 = !(val & BIT(8))  + !(val & BIT(9));
+	sc3 = !(val & BIT(12)) + !(val & BIT(13));
+
+	return (max(max(max(sc0, sc1), sc2), sc3) << 10) - 1;
+}
+
 static void __cpuinit
 amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
 {
@@ -306,6 +331,7 @@ amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
 		return;
 
 	this_leaf->can_disable = 1;
+	this_leaf->l3_indexes  = amd_calc_l3_indexes();
 }
 
 static int
@@ -769,7 +795,8 @@ static ssize_t store_cache_disable(struct _cpuid4_info *this_leaf,
 		return -EINVAL;
 
 	/* do not allow writes outside of allowed bits */
-	if (val & ~(SUBCACHE_MASK | SUBCACHE_INDEX))
+	if ((val & ~(SUBCACHE_MASK | SUBCACHE_INDEX)) ||
+	    ((val & SUBCACHE_INDEX) > this_leaf->l3_indexes))
 		return -EINVAL;
 
 	val |= BIT(30);
-- 
1.6.6

Re: [PATCH 3/3] x86, cacheinfo: Calculate L3 indexes

[H. Peter Anvin]

On 01/19/2010 03:07 AM, Borislav Petkov wrote:
> We need to know the valid L3 indexes interval when disabling them over
> /sysfs. Do that when the core is brought online and add boundary checks
> to the sysfs .store attribute.
> 
> Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> @@ -161,6 +162,7 @@ struct _cpuid4_info_regs {
>  	union _cpuid4_leaf_ecx ecx;
>  	unsigned long size;
>  	unsigned long can_disable;
> +	unsigned int l3_indexes;
>  };
>  

Hmmm... 32, 64, 64, 32 bits... we could move up the l3_indexes variable
here.   However, more likely is that "size" and "can_disable" have no
business being unsigned long in the first place -- especially the latter
seems to be actually used as a boolean, and really should be "bool".

Second, the preferred plural of "index" is "indices" (although both are
correct and present in the kernel source.)

	-hpa

Re: [PATCH 3/3] x86, cacheinfo: Calculate L3 indexes

[Borislav Petkov]

On Wed, Jan 20, 2010 at 03:15:31PM -0800, H. Peter Anvin wrote:
> On 01/19/2010 03:07 AM, Borislav Petkov wrote:
> > We need to know the valid L3 indexes interval when disabling them over
> > /sysfs. Do that when the core is brought online and add boundary checks
> > to the sysfs .store attribute.
> > 
> > Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
> > @@ -161,6 +162,7 @@ struct _cpuid4_info_regs {
> >  	union _cpuid4_leaf_ecx ecx;
> >  	unsigned long size;
> >  	unsigned long can_disable;
> > +	unsigned int l3_indexes;
> >  };
> >  
> 
> Hmmm... 32, 64, 64, 32 bits... we could move up the l3_indexes variable
> here.   However, more likely is that "size" and "can_disable" have no
> business being unsigned long in the first place -- especially the latter
> seems to be actually used as a boolean, and really should be "bool".

good point.

> Second, the preferred plural of "index" is "indices" (although both are
> correct and present in the kernel source.)

done.

-- 
Regards/Gruss,
Boris.

-
Advanced Micro Devices, Inc.
Operating Systems Research Center