Add ABI Documentation entry and fix some /sys directory formating
issues with the L3 Cache Index Disable feature for future AMD
processors. Add a check to disable it for family 0x10 models
that do not support it properly. Correct the disable algorithm
to reflect erratum 388.
Signed-off-by: Mark Langsdorf <[email protected]>
diff --git a/Documentation/ABI/testing/sysfs-devices-cache_disable b/Documentation/ABI/testing/sysfs-devices-cache_disable
new file mode 100644
index 0000000..c7d9174
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-cache_disable
@@ -0,0 +1,18 @@
+What: /sys/devices/system/cpu/cpu*/cache/index*/cache_disable_X
+Date: August 2008
+KernelVersion: 2.6.27
+Contact: [email protected]
+Description: These files exist in every cpu's cache index directories.
+ There are currently 2 cache_disable_# files in each
+ directory. Reading from these files on a supported
+ processor will return that cache disable index value
+ for that processor and node. Writing to one of these
+ files will cause the specificed cache index to be disabled.
+
+ Currently, only AMD Family 10h Processors support cache index
+ disable, and only for their L3 caches. See the BIOS and
+ Kernel Developer's Guide at
+ http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/31116-Public-GH-BKDG_3.20_2-4-09.pdf
+ for formatting information and other details on the
+ cache index disable.
+Users: [email protected]
diff --git a/arch/x86/include/asm/k8.h b/arch/x86/include/asm/k8.h
index 54c8cc5..0d619c3 100644
--- a/arch/x86/include/asm/k8.h
+++ b/arch/x86/include/asm/k8.h
@@ -6,7 +6,11 @@
extern struct pci_device_id k8_nb_ids[];
extern int early_is_k8_nb(u32 value);
+#ifdef CONFIG_K8_NB
extern struct pci_dev **k8_northbridges;
+#else
+struct pci_dev **k8_northbridges;
+#endif
extern int num_k8_northbridges;
extern int cache_k8_northbridges(void);
extern void k8_flush_garts(void);
diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 483eda9..9f41501 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -18,6 +18,9 @@
#include <asm/processor.h>
#include <asm/smp.h>
+#include <linux/pci.h>
+#include <asm/k8.h>
+
#define LVL_1_INST 1
#define LVL_1_DATA 2
#define LVL_2 3
@@ -159,14 +162,6 @@ struct _cpuid4_info_regs {
unsigned long can_disable;
};
-#if defined(CONFIG_PCI) && defined(CONFIG_SYSFS)
-static struct pci_device_id k8_nb_id[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x1103) },
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x1203) },
- {}
-};
-#endif
-
unsigned short num_cache_leaves;
/* AMD doesn't have CPUID4. Emulate it here to report the same
@@ -291,6 +286,12 @@ amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
{
if (index < 3)
return;
+ if (boot_cpu_data.x86 == 0x11)
+ return;
+
+ if ((boot_cpu_data.x86 == 0x10) && (boot_cpu_data.x86_model < 0x8))
+ return;
+
this_leaf->can_disable = 1;
}
@@ -639,6 +640,68 @@ static ssize_t show_##file_name \
return sprintf (buf, "%lu\n", (unsigned long)this_leaf->object + val); \
}
+static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
+ unsigned int index)
+{
+ int node = cpu_to_node(first_cpu(this_leaf->shared_cpu_map));
+ struct pci_dev *dev = k8_northbridges[node];
+ unsigned int reg = 0;
+
+ if (!this_leaf->can_disable)
+ return -EINVAL;
+
+ pci_read_config_dword(dev, 0x1BC + index * 4, ®);
+ return sprintf(buf, "%x\n", reg);
+}
+
+#define SHOW_CACHE_DISABLE(index) \
+static ssize_t \
+show_cache_disable_##index(struct _cpuid4_info *this_leaf, char *buf) \
+{ \
+ return show_cache_disable(this_leaf, buf, index); \
+}
+
+static ssize_t
+store_cache_disable(struct _cpuid4_info *this_leaf, const char *buf,
+ size_t count, unsigned int index)
+{
+ int node = cpu_to_node(first_cpu(this_leaf->shared_cpu_map));
+ struct pci_dev *dev = k8_northbridges[node];
+ unsigned long val = 0;
+ unsigned int scrubber = 0;
+
+ if (!this_leaf->can_disable)
+ return -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (strict_strtoul(buf, 10, &val) < 0)
+ return -EINVAL;
+
+ val |= 0xc0000000;
+ pci_read_config_dword(dev, 0x58, &scrubber);
+ scrubber &= ~0x0f800000;
+ pci_write_config_dword(dev, 0x58, scrubber);
+ pci_write_config_dword(dev, 0x1BC + index * 4, val & ~0x40000000);
+ wbinvd();
+ pci_write_config_dword(dev, 0x1BC + index * 4, val);
+ return count;
+}
+
+#define STORE_CACHE_DISABLE(index) \
+static ssize_t \
+store_cache_disable_##index(struct _cpuid4_info *this_leaf, \
+ const char *buf, size_t count) \
+{ \
+ return store_cache_disable(this_leaf, buf, count, index); \
+}
+
+SHOW_CACHE_DISABLE(0)
+STORE_CACHE_DISABLE(0)
+SHOW_CACHE_DISABLE(1)
+STORE_CACHE_DISABLE(1)
+
show_one_plus(level, eax.split.level, 0);
show_one_plus(coherency_line_size, ebx.split.coherency_line_size, 1);
show_one_plus(physical_line_partition, ebx.split.physical_line_partition, 1);
@@ -696,98 +759,6 @@ static ssize_t show_type(struct _cpuid4_info *this_leaf, char *buf)
#define to_object(k) container_of(k, struct _index_kobject, kobj)
#define to_attr(a) container_of(a, struct _cache_attr, attr)
-#ifdef CONFIG_PCI
-static struct pci_dev *get_k8_northbridge(int node)
-{
- struct pci_dev *dev = NULL;
- int i;
-
- for (i = 0; i <= node; i++) {
- do {
- dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
- if (!dev)
- break;
- } while (!pci_match_id(&k8_nb_id[0], dev));
- if (!dev)
- break;
- }
- return dev;
-}
-#else
-static struct pci_dev *get_k8_northbridge(int node)
-{
- return NULL;
-}
-#endif
-
-static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf)
-{
- const struct cpumask *mask = to_cpumask(this_leaf->shared_cpu_map);
- int node = cpu_to_node(cpumask_first(mask));
- struct pci_dev *dev = NULL;
- ssize_t ret = 0;
- int i;
-
- if (!this_leaf->can_disable)
- return sprintf(buf, "Feature not enabled\n");
-
- dev = get_k8_northbridge(node);
- if (!dev) {
- printk(KERN_ERR "Attempting AMD northbridge operation on a system with no northbridge\n");
- return -EINVAL;
- }
-
- for (i = 0; i < 2; i++) {
- unsigned int reg;
-
- pci_read_config_dword(dev, 0x1BC + i * 4, ®);
-
- ret += sprintf(buf, "%sEntry: %d\n", buf, i);
- ret += sprintf(buf, "%sReads: %s\tNew Entries: %s\n",
- buf,
- reg & 0x80000000 ? "Disabled" : "Allowed",
- reg & 0x40000000 ? "Disabled" : "Allowed");
- ret += sprintf(buf, "%sSubCache: %x\tIndex: %x\n",
- buf, (reg & 0x30000) >> 16, reg & 0xfff);
- }
- return ret;
-}
-
-static ssize_t
-store_cache_disable(struct _cpuid4_info *this_leaf, const char *buf,
- size_t count)
-{
- const struct cpumask *mask = to_cpumask(this_leaf->shared_cpu_map);
- int node = cpu_to_node(cpumask_first(mask));
- struct pci_dev *dev = NULL;
- unsigned int ret, index, val;
-
- if (!this_leaf->can_disable)
- return 0;
-
- if (strlen(buf) > 15)
- return -EINVAL;
-
- ret = sscanf(buf, "%x %x", &index, &val);
- if (ret != 2)
- return -EINVAL;
- if (index > 1)
- return -EINVAL;
-
- val |= 0xc0000000;
- dev = get_k8_northbridge(node);
- if (!dev) {
- printk(KERN_ERR "Attempting AMD northbridge operation on a system with no northbridge\n");
- return -EINVAL;
- }
-
- pci_write_config_dword(dev, 0x1BC + index * 4, val & ~0x40000000);
- wbinvd();
- pci_write_config_dword(dev, 0x1BC + index * 4, val);
-
- return 1;
-}
-
struct _cache_attr {
struct attribute attr;
ssize_t (*show)(struct _cpuid4_info *, char *);
@@ -808,7 +779,11 @@ define_one_ro(size);
define_one_ro(shared_cpu_map);
define_one_ro(shared_cpu_list);
-static struct _cache_attr cache_disable = __ATTR(cache_disable, 0644, show_cache_disable, store_cache_disable);
+static struct _cache_attr cache_disable_0 = __ATTR(cache_disable_0, 0644,
+ show_cache_disable_0, store_cache_disable_0);
+static struct _cache_attr cache_disable_1 = __ATTR(cache_disable_1, 0644,
+ show_cache_disable_1, store_cache_disable_1);
+
static struct attribute * default_attrs[] = {
&type.attr,
@@ -820,7 +795,8 @@ static struct attribute * default_attrs[] = {
&size.attr,
&shared_cpu_map.attr,
&shared_cpu_list.attr,
- &cache_disable.attr,
+ &cache_disable_0.attr,
+ &cache_disable_1.attr,
NULL
};
* Mark Langsdorf <[email protected]> wrote:
> Add ABI Documentation entry and fix some /sys directory formating
> issues with the L3 Cache Index Disable feature for future AMD
> processors. Add a check to disable it for family 0x10 models
> that do not support it properly. Correct the disable algorithm
> to reflect erratum 388.
>
> Signed-off-by: Mark Langsdorf <[email protected]>
i'm still getting this build failure:
arch/x86/mm/built-in.o:(.bss+0x3640): multiple definition of `k8_northbridges'
i've reported this build failure before. I've excluded this commit
from tip:master for now.
Ingo
commit eb40831ca29a89f056f8c6128c8b36d3691f6698
Author: Mark Langsdorf <[email protected]>
Date: Wed Apr 8 15:48:45 2009 -0500
Add ABI Documentation entry and fix some /sys directory formating
issues with the L3 Cache Index Disable feature for future AMD
processors. Add a check to disable it for family 0x10 models
that do not support it yet.
Signed-off-by: Mark Langsdorf <[email protected]>
diff --git a/Documentation/ABI/testing/sysfs-devices-cache_disable b/Documentation/ABI/testing/sysfs-devices-cache_disable
new file mode 100644
index 0000000..175bb4f
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-cache_disable
@@ -0,0 +1,18 @@
+What: /sys/devices/system/cpu/cpu*/cache/index*/cache_disable_X
+Date: August 2008
+KernelVersion: 2.6.27
+Contact: [email protected]
+Description: These files exist in every cpu's cache index directories.
+ There are currently 2 cache_disable_# files in each
+ directory. Reading from these files on a supported
+ processor will return that cache disable index value
+ for that processor and node. Writing to one of these
+ files will cause the specificed cache index to be disabled.
+
+ Currently, only AMD Family 10h Processors support cache index
+ disable, and only for their L3 caches. See the BIOS and
+ Kernel Developer's Guide at
+ http://www.amd.com/us-en/assets/content_type/white_papers_and_tech_docs/31116-Public-GH-BKDG_3.20_2-4-09.pdf
+ for formatting information and other details on the
+ cache index disable.
+Users: [email protected]
diff --git a/arch/x86/include/asm/k8.h b/arch/x86/include/asm/k8.h
index 54c8cc5..ebd9390 100644
--- a/arch/x86/include/asm/k8.h
+++ b/arch/x86/include/asm/k8.h
@@ -12,4 +12,13 @@ extern int cache_k8_northbridges(void);
extern void k8_flush_garts(void);
extern int k8_scan_nodes(unsigned long start, unsigned long end);
+static inline struct pci_dev *get_k8_northbridge(int node)
+{
+#ifdef CONFIG_K8_NB
+ return k8_northbridges[node];
+#else
+ return NULL;
+#endif
+}
+
#endif /* _ASM_X86_K8_H */
diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
index 483eda9..453a6e3 100644
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -18,6 +18,9 @@
#include <asm/processor.h>
#include <asm/smp.h>
+#include <linux/pci.h>
+#include <asm/k8.h>
+
#define LVL_1_INST 1
#define LVL_1_DATA 2
#define LVL_2 3
@@ -159,14 +162,6 @@ struct _cpuid4_info_regs {
unsigned long can_disable;
};
-#if defined(CONFIG_PCI) && defined(CONFIG_SYSFS)
-static struct pci_device_id k8_nb_id[] = {
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x1103) },
- { PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x1203) },
- {}
-};
-#endif
-
unsigned short num_cache_leaves;
/* AMD doesn't have CPUID4. Emulate it here to report the same
@@ -291,6 +286,12 @@ amd_check_l3_disable(int index, struct _cpuid4_info_regs *this_leaf)
{
if (index < 3)
return;
+ if (boot_cpu_data.x86 == 0x11)
+ return;
+
+ if ((boot_cpu_data.x86 == 0x10) && (boot_cpu_data.x86_model < 0x8))
+ return;
+
this_leaf->can_disable = 1;
}
@@ -639,6 +640,70 @@ static ssize_t show_##file_name \
return sprintf (buf, "%lu\n", (unsigned long)this_leaf->object + val); \
}
+static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
+ unsigned int index)
+{
+ int cpu = cpumask_first(to_cpumask(this_leaf->shared_cpu_map));
+ int node = cpu_to_node(cpu);
+ struct pci_dev *dev = get_k8_northbridge(node);
+ unsigned int reg = 0;
+
+ if (!this_leaf->can_disable)
+ return -EINVAL;
+
+ pci_read_config_dword(dev, 0x1BC + index * 4, ®);
+ return sprintf(buf, "%x\n", reg);
+}
+
+#define SHOW_CACHE_DISABLE(index) \
+static ssize_t \
+show_cache_disable_##index(struct _cpuid4_info *this_leaf, char *buf) \
+{ \
+ return show_cache_disable(this_leaf, buf, index); \
+}
+
+static ssize_t
+store_cache_disable(struct _cpuid4_info *this_leaf, const char *buf,
+ size_t count, unsigned int index)
+{
+ int cpu = cpumask_first(to_cpumask(this_leaf->shared_cpu_map));
+ int node = cpu_to_node(cpu);
+ struct pci_dev *dev = get_k8_northbridge(node);
+ unsigned long val = 0;
+ unsigned int scrubber = 0;
+
+ if (!this_leaf->can_disable)
+ return -EINVAL;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ if (strict_strtoul(buf, 10, &val) < 0)
+ return -EINVAL;
+
+ val |= 0xc0000000;
+ pci_read_config_dword(dev, 0x58, &scrubber);
+ scrubber &= ~0x0f800000;
+ pci_write_config_dword(dev, 0x58, scrubber);
+ pci_write_config_dword(dev, 0x1BC + index * 4, val & ~0x40000000);
+ wbinvd();
+ pci_write_config_dword(dev, 0x1BC + index * 4, val);
+ return count;
+}
+
+#define STORE_CACHE_DISABLE(index) \
+static ssize_t \
+store_cache_disable_##index(struct _cpuid4_info *this_leaf, \
+ const char *buf, size_t count) \
+{ \
+ return store_cache_disable(this_leaf, buf, count, index); \
+}
+
+SHOW_CACHE_DISABLE(0)
+STORE_CACHE_DISABLE(0)
+SHOW_CACHE_DISABLE(1)
+STORE_CACHE_DISABLE(1)
+
show_one_plus(level, eax.split.level, 0);
show_one_plus(coherency_line_size, ebx.split.coherency_line_size, 1);
show_one_plus(physical_line_partition, ebx.split.physical_line_partition, 1);
@@ -696,98 +761,6 @@ static ssize_t show_type(struct _cpuid4_info *this_leaf, char *buf)
#define to_object(k) container_of(k, struct _index_kobject, kobj)
#define to_attr(a) container_of(a, struct _cache_attr, attr)
-#ifdef CONFIG_PCI
-static struct pci_dev *get_k8_northbridge(int node)
-{
- struct pci_dev *dev = NULL;
- int i;
-
- for (i = 0; i <= node; i++) {
- do {
- dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev);
- if (!dev)
- break;
- } while (!pci_match_id(&k8_nb_id[0], dev));
- if (!dev)
- break;
- }
- return dev;
-}
-#else
-static struct pci_dev *get_k8_northbridge(int node)
-{
- return NULL;
-}
-#endif
-
-static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf)
-{
- const struct cpumask *mask = to_cpumask(this_leaf->shared_cpu_map);
- int node = cpu_to_node(cpumask_first(mask));
- struct pci_dev *dev = NULL;
- ssize_t ret = 0;
- int i;
-
- if (!this_leaf->can_disable)
- return sprintf(buf, "Feature not enabled\n");
-
- dev = get_k8_northbridge(node);
- if (!dev) {
- printk(KERN_ERR "Attempting AMD northbridge operation on a system with no northbridge\n");
- return -EINVAL;
- }
-
- for (i = 0; i < 2; i++) {
- unsigned int reg;
-
- pci_read_config_dword(dev, 0x1BC + i * 4, ®);
-
- ret += sprintf(buf, "%sEntry: %d\n", buf, i);
- ret += sprintf(buf, "%sReads: %s\tNew Entries: %s\n",
- buf,
- reg & 0x80000000 ? "Disabled" : "Allowed",
- reg & 0x40000000 ? "Disabled" : "Allowed");
- ret += sprintf(buf, "%sSubCache: %x\tIndex: %x\n",
- buf, (reg & 0x30000) >> 16, reg & 0xfff);
- }
- return ret;
-}
-
-static ssize_t
-store_cache_disable(struct _cpuid4_info *this_leaf, const char *buf,
- size_t count)
-{
- const struct cpumask *mask = to_cpumask(this_leaf->shared_cpu_map);
- int node = cpu_to_node(cpumask_first(mask));
- struct pci_dev *dev = NULL;
- unsigned int ret, index, val;
-
- if (!this_leaf->can_disable)
- return 0;
-
- if (strlen(buf) > 15)
- return -EINVAL;
-
- ret = sscanf(buf, "%x %x", &index, &val);
- if (ret != 2)
- return -EINVAL;
- if (index > 1)
- return -EINVAL;
-
- val |= 0xc0000000;
- dev = get_k8_northbridge(node);
- if (!dev) {
- printk(KERN_ERR "Attempting AMD northbridge operation on a system with no northbridge\n");
- return -EINVAL;
- }
-
- pci_write_config_dword(dev, 0x1BC + index * 4, val & ~0x40000000);
- wbinvd();
- pci_write_config_dword(dev, 0x1BC + index * 4, val);
-
- return 1;
-}
-
struct _cache_attr {
struct attribute attr;
ssize_t (*show)(struct _cpuid4_info *, char *);
@@ -808,7 +781,11 @@ define_one_ro(size);
define_one_ro(shared_cpu_map);
define_one_ro(shared_cpu_list);
-static struct _cache_attr cache_disable = __ATTR(cache_disable, 0644, show_cache_disable, store_cache_disable);
+static struct _cache_attr cache_disable_0 = __ATTR(cache_disable_0, 0644,
+ show_cache_disable_0, store_cache_disable_0);
+static struct _cache_attr cache_disable_1 = __ATTR(cache_disable_1, 0644,
+ show_cache_disable_1, store_cache_disable_1);
+
static struct attribute * default_attrs[] = {
&type.attr,
@@ -820,7 +797,8 @@ static struct attribute * default_attrs[] = {
&size.attr,
&shared_cpu_map.attr,
&shared_cpu_list.attr,
- &cache_disable.attr,
+ &cache_disable_0.attr,
+ &cache_disable_1.attr,
NULL
};
The current code to set up the GART as an IOMMU enables GART
translations before it removes the aperture from the kernel
memory map, sets the GART PTEs to UC, sets up the guard and
scratch pages, or does a wbinvd(). This leaves the possibility
of cache aliasing open and can cause system crashes.
Re-order the code and add tlbflush so as to enable the
GART translations only after all safeguards are in place and
the tlb has been flushed.
AMD has tested this patch and seen no adverse effects.
Signed-off-by: Mark Langsdorf <[email protected]>
diff -r 0d1744c7acc7 arch/x86/kernel/pci-gart_64.c
--- a/arch/x86/kernel/pci-gart_64.c Fri Mar 27 16:47:28 2009 -0500
+++ b/arch/x86/kernel/pci-gart_64.c Mon Mar 30 15:05:47 2009 -0500
@@ -38,6 +38,7 @@
#include <asm/swiotlb.h>
#include <asm/dma.h>
#include <asm/k8.h>
+#include <asm/tlbflush.h>
static unsigned long iommu_bus_base; /* GART remapping area (physical) */
static unsigned long iommu_size; /* size of remapping area bytes */
@@ -682,9 +683,9 @@
if (set_memory_uc((unsigned long)gatt, gatt_size >> PAGE_SHIFT))
panic("Could not set GART PTEs to uncacheable pages");
+ wbinvd();
+
agp_gatt_table = gatt;
-
- enable_gart_translations();
error = sysdev_class_register(&gart_sysdev_class);
if (!error)
@@ -855,6 +856,11 @@
for (i = EMERGENCY_PAGES; i < iommu_pages; i++)
iommu_gatt_base[i] = gart_unmapped_entry;
+ wbinvd();
+ flush_tlb_all();
+
+ enable_gart_translations();
+
flush_gart();
dma_ops = &gart_dma_ops;
}
* Mark Langsdorf <[email protected]> wrote:
> commit eb40831ca29a89f056f8c6128c8b36d3691f6698
> Author: Mark Langsdorf <[email protected]>
> Date: Wed Apr 8 15:48:45 2009 -0500
>
> Add ABI Documentation entry and fix some /sys directory formating
> issues with the L3 Cache Index Disable feature for future AMD
> processors. Add a check to disable it for family 0x10 models
> that do not support it yet.
>
> Signed-off-by: Mark Langsdorf <[email protected]>
Could you please send a delta patch against the tip:x86/cpu branch,
which currently contains an earlier (broken) patch of yours:
45ca863: x86, cpu: conform L3 Cache Index Disable to Linux standards
You can get that branch by doing:
git checkout tip/x86/cpu
After having picked up -tip as a remote repository as per:
http://people.redhat.com/mingo/tip.git/README
Thanks,
Ingo
On Wed, Apr 8, 2009 at 2:08 PM, Mark Langsdorf <[email protected]> wrote:
> The current code to set up the GART as an IOMMU enables GART
> translations before it removes the aperture from the kernel
> memory map, sets the GART PTEs to UC, sets up the guard and
> scratch pages, or does a wbinvd(). ?This leaves the possibility
> of cache aliasing open and can cause system crashes.
>
> Re-order the code and add tlbflush so as to enable the
> GART translations only after all safeguards are in place and
> the tlb has been flushed.
>
> AMD has tested this patch and seen no adverse effects.
how about system with AGP card and more than 4g system RAM?
also it seems flush_gart() in init_k8_gatt() and gart_iommu_init()
could be removed?
YH
On Wed, Apr 08, 2009 at 04:02:34PM -0500, Mark Langsdorf wrote:
> commit eb40831ca29a89f056f8c6128c8b36d3691f6698
> Author: Mark Langsdorf <[email protected]>
> Date: Wed Apr 8 15:48:45 2009 -0500
>
> Add ABI Documentation entry and fix some /sys directory formating
> issues with the L3 Cache Index Disable feature for future AMD
> processors. Add a check to disable it for family 0x10 models
> that do not support it yet.
>
> Signed-off-by: Mark Langsdorf <[email protected]>
I feel uncomfortable with this patch (and the previous ones).
The patch addresses issues with the current Linux code but introduces
new ones.
I suggest to revert last two patches on tip/x86/cpu
and to split Mark's last patch into several pieces.
(BTW, i'll do this in any case asap).
This would really help to understand the code changes
and to ease review and further adaptions of this code.
> diff --git a/arch/x86/include/asm/k8.h b/arch/x86/include/asm/k8.h
> index 54c8cc5..ebd9390 100644
> --- a/arch/x86/include/asm/k8.h
> +++ b/arch/x86/include/asm/k8.h
> @@ -12,4 +12,13 @@ extern int cache_k8_northbridges(void);
> extern void k8_flush_garts(void);
> extern int k8_scan_nodes(unsigned long start, unsigned long end);
>
> +static inline struct pci_dev *get_k8_northbridge(int node)
> +{
> +#ifdef CONFIG_K8_NB
> + return k8_northbridges[node];
> +#else
> + return NULL;
> +#endif
> +}
IMHO a macro would suffice to return either k8_northbridges[node]
or NULL. And num_k8_northbridges should be checked, no?
> #endif /* _ASM_X86_K8_H */
> diff --git a/arch/x86/kernel/cpu/intel_cacheinfo.c b/arch/x86/kernel/cpu/intel_cacheinfo.c
> index 483eda9..453a6e3 100644
> --- a/arch/x86/kernel/cpu/intel_cacheinfo.c
> +++ b/arch/x86/kernel/cpu/intel_cacheinfo.c
> @@ -639,6 +640,70 @@ static ssize_t show_##file_name \
> return sprintf (buf, "%lu\n", (unsigned long)this_leaf->object + val); \
> }
>
> +static ssize_t show_cache_disable(struct _cpuid4_info *this_leaf, char *buf,
> + unsigned int index)
> +{
> + int cpu = cpumask_first(to_cpumask(this_leaf->shared_cpu_map));
> + int node = cpu_to_node(cpu);
> + struct pci_dev *dev = get_k8_northbridge(node);
> + unsigned int reg = 0;
> +
> + if (!this_leaf->can_disable)
> + return -EINVAL;
> +
> + pci_read_config_dword(dev, 0x1BC + index * 4, ®);
Do I miss something, or why is there no check for dev==NULL? IMHO
this will cause an Oops when dereferencing dev->bus in
pci_read_config_dword in case of "ifndef CONFIG_K8_NB".
> + return sprintf(buf, "%x\n", reg);
> +}
> +
> +#define SHOW_CACHE_DISABLE(index) \
> +static ssize_t \
> +show_cache_disable_##index(struct _cpuid4_info *this_leaf, char *buf) \
> +{ \
> + return show_cache_disable(this_leaf, buf, index); \
> +}
> +
> +static ssize_t
> +store_cache_disable(struct _cpuid4_info *this_leaf, const char *buf,
> + size_t count, unsigned int index)
> +{
> + int cpu = cpumask_first(to_cpumask(this_leaf->shared_cpu_map));
> + int node = cpu_to_node(cpu);
> + struct pci_dev *dev = get_k8_northbridge(node);
> + unsigned long val = 0;
> + unsigned int scrubber = 0;
> +
> + if (!this_leaf->can_disable)
> + return -EINVAL;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + if (strict_strtoul(buf, 10, &val) < 0)
> + return -EINVAL;
> +
> + val |= 0xc0000000;
> + pci_read_config_dword(dev, 0x58, &scrubber);
Missing check for dev==NULL and potential Oops, no?
> + scrubber &= ~0x0f800000;
> + pci_write_config_dword(dev, 0x58, scrubber);
This adapts L3 scrub rate but accesses reserved bits.
Perhaps you meant to zero out bits 24-28 which would
mean to use
scrubber &= ~0x1f000000;
(... but maybe I need some more coffee to do the bit fiddling ... ;-)
Regards,
Andreas
On Wed, Apr 08, 2009 at 04:08:55PM -0500, Mark Langsdorf wrote:
> The current code to set up the GART as an IOMMU enables GART
> translations before it removes the aperture from the kernel
> memory map, sets the GART PTEs to UC, sets up the guard and
> scratch pages, or does a wbinvd(). This leaves the possibility
> of cache aliasing open and can cause system crashes.
This patch looks broken. I don't think that there are any aliasing
problems in the existing code. Can you elaborate were do you see these
problems? Is there any existing bug report for this?
> Re-order the code and add tlbflush so as to enable the
> GART translations only after all safeguards are in place and
> the tlb has been flushed.
>
> AMD has tested this patch and seen no adverse effects.
>
> Signed-off-by: Mark Langsdorf <[email protected]>
>
> diff -r 0d1744c7acc7 arch/x86/kernel/pci-gart_64.c
> --- a/arch/x86/kernel/pci-gart_64.c Fri Mar 27 16:47:28 2009 -0500
> +++ b/arch/x86/kernel/pci-gart_64.c Mon Mar 30 15:05:47 2009 -0500
> @@ -38,6 +38,7 @@
> #include <asm/swiotlb.h>
> #include <asm/dma.h>
> #include <asm/k8.h>
> +#include <asm/tlbflush.h>
>
> static unsigned long iommu_bus_base; /* GART remapping area (physical) */
> static unsigned long iommu_size; /* size of remapping area bytes */
> @@ -682,9 +683,9 @@
> if (set_memory_uc((unsigned long)gatt, gatt_size >> PAGE_SHIFT))
> panic("Could not set GART PTEs to uncacheable pages");
>
> + wbinvd();
> +
> agp_gatt_table = gatt;
> -
> - enable_gart_translations();
To enable the GART here has the advantage that all other CPUs are not
yet up. Since the GATT has only non-present entries there is no cache
aliasing problem here. If you enable it later you need to flush the
cache on _all_ CPUs and not just the CPU where the GART initialization
code runs.
>
> error = sysdev_class_register(&gart_sysdev_class);
> if (!error)
> @@ -855,6 +856,11 @@
> for (i = EMERGENCY_PAGES; i < iommu_pages; i++)
> iommu_gatt_base[i] = gart_unmapped_entry;
>
> + wbinvd();
Why a wbinvd() here? A few lines above is already one.
> + flush_tlb_all();
Same with TLB flush. The set_memory_np() function does that for us.
> +
> + enable_gart_translations();
> +
If we change the enablement logic like done in this patch you need to
invalidate the caches on _all_ cpus. I don't think this is necessary if
we just enable all GARTs when only the boot cpu is up.
> flush_gart();
> dma_ops = &gart_dma_ops;
> }
> > AMD has tested this patch and seen no adverse effects.
>
> how about system with AGP card and more than 4g system RAM?
Tested on an Iwill DK8X 2P system with 8 GB of DRAM.
> also it seems flush_gart() in init_k8_gatt() and gart_iommu_init()
> could be removed?
Possibly, but I'd rather be cautious than not.
-Mark Langsdorf
Operating System Research Center
AMD