[PATCH] Cleanups & fixes for the MTRR driver

Andi Kleen (ak@muc.de)
Wed, 27 Nov 2002 18:54:42 +0100


To fix some bugs and cut down code duplication between i386 and x86-64
I decided to reuse (using symlinks) the i386 MTRR driver for x86-64.
To do that I did some cleanups and a few bug fixes in the i386 mtrr driver.

- Convert it to seq_file. This makes it a lot cleaner and gets rid
of some cruft.
- Some long -> int required on 64bit (nop on 32bit)
- Add some RED-PENs for unfixed bugs (e.g. it doesn't support
physical addresses > 4GB on 32bit hosts and the SMP locking is not
very strong)
- Remove bogus // in mtrr_write that made the function a nop
(= only ioctl was working for changing)
- Unify duplicated mtrr_strings define.
- Some other minor fixes and cleanups

Patch for 2.5.49.

Please consider applying,

-Andi

diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/generic.c linux/arch/i386/kernel/cpu/mtrr/generic.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/generic.c 2002-11-13 12:07:27.000000000 +0100
+++ linux/arch/i386/kernel/cpu/mtrr/generic.c 2002-11-27 12:13:18.000000000 +0100
@@ -1,3 +1,5 @@
+/* This only handles 32bit MTRR on 32bit hosts. This is strictly wrong
+ because MTRRs can span upto 40 bits (36bits on most modern x86) */
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/mm.h>
@@ -30,7 +32,7 @@ get_mtrr_var_range(unsigned int index, s
static void __init
get_fixed_ranges(mtrr_type * frs)
{
- unsigned long *p = (unsigned long *) frs;
+ unsigned int *p = (unsigned int *) frs;
int i;

rdmsr(MTRRfix64K_00000_MSR, p[0], p[1]);
@@ -46,7 +48,7 @@ void get_mtrr_state(void)
{
unsigned int i;
struct mtrr_var_range *vrs;
- unsigned long lo, dummy;
+ unsigned lo, dummy;

if (!mtrr_state.var_ranges) {
mtrr_state.var_ranges = kmalloc(num_var_ranges * sizeof (struct mtrr_var_range),
@@ -113,11 +115,10 @@ int generic_get_free_region(unsigned lon
return -ENOSPC;
}

-
void generic_get_mtrr(unsigned int reg, unsigned long *base,
unsigned long *size, mtrr_type * type)
{
- unsigned long mask_lo, mask_hi, base_lo, base_hi;
+ unsigned int mask_lo, mask_hi, base_lo, base_hi;

rdmsr(MTRRphysMask_MSR(reg), mask_lo, mask_hi);
if ((mask_lo & 0x800) == 0) {
@@ -143,10 +144,10 @@ void generic_get_mtrr(unsigned int reg,

static int __init set_fixed_ranges(mtrr_type * frs)
{
- unsigned long *p = (unsigned long *) frs;
+ unsigned int *p = (unsigned int *) frs;
int changed = FALSE;
int i;
- unsigned long lo, hi;
+ unsigned int lo, hi;

rdmsr(MTRRfix64K_00000_MSR, lo, hi);
if (p[0] != lo || p[1] != hi) {
@@ -228,13 +229,13 @@ static unsigned long set_mtrr_state(u32
}


-static u32 cr4 = 0;
+static unsigned long cr4 = 0;
static u32 deftype_lo, deftype_hi;
static spinlock_t set_atomicity_lock = SPIN_LOCK_UNLOCKED;

static void prepare_set(void)
{
- u32 cr0;
+ unsigned long cr0;

/* Note that this is not ideal, since the cache is only flushed/disabled
for this CPU while the MTRRs are changed, but changing this requires
diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/if.c linux/arch/i386/kernel/cpu/mtrr/if.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/if.c 2002-10-19 06:01:18.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/if.c 2002-11-27 12:33:08.000000000 +0100
@@ -3,27 +3,27 @@
#include <linux/devfs_fs_kernel.h>
#include <linux/ctype.h>
#include <linux/module.h>
+#include <linux/seq_file.h>
#include <asm/uaccess.h>

-/* What kind of fucking hack is this? */
-#define MTRR_NEED_STRINGS
+#define LINE_SIZE 80

#include <asm/mtrr.h>
#include "mtrr.h"

-static char *ascii_buffer;
-static unsigned int ascii_buf_bytes;
-
+/* RED-PEN: this is accessed without any locking */
extern unsigned int *usage_table;

-#define LINE_SIZE 80
+static int mtrr_seq_show(struct seq_file *seq, void *offset);
+
+#define FILE_FCOUNT(f) (((struct seq_file *)((f)->private_data))->private)

static int
mtrr_file_add(unsigned long base, unsigned long size,
unsigned int type, char increment, struct file *file, int page)
{
int reg, max;
- unsigned int *fcount = file->private_data;
+ unsigned int *fcount = FILE_FCOUNT(file);

max = num_var_ranges;
if (fcount == NULL) {
@@ -33,7 +33,7 @@ mtrr_file_add(unsigned long base, unsign
return -ENOMEM;
}
memset(fcount, 0, max * sizeof *fcount);
- file->private_data = fcount;
+ FILE_FCOUNT(file) = fcount;
}
if (!page) {
if ((base & (PAGE_SIZE - 1)) || (size & (PAGE_SIZE - 1))) {
@@ -79,19 +79,7 @@ mtrr_file_del(unsigned long base, unsign
return reg;
}

-static ssize_t
-mtrr_read(struct file *file, char *buf, size_t len, loff_t * ppos)
-{
- if (*ppos >= ascii_buf_bytes)
- return 0;
- if (*ppos + len > ascii_buf_bytes)
- len = ascii_buf_bytes - *ppos;
- if (copy_to_user(buf, ascii_buffer + *ppos, len))
- return -EFAULT;
- *ppos += len;
- return len;
-}
-
+/* RED-PEN: seq_file can seek now. this is ignored. */
static ssize_t
mtrr_write(struct file *file, const char *buf, size_t len, loff_t * ppos)
/* Format of control line:
@@ -149,7 +137,7 @@ mtrr_write(struct file *file, const char
ptr += 5;
for (; isspace(*ptr); ++ptr) ;
for (i = 0; i < MTRR_NUM_TYPES; ++i) {
-// if (strcmp(ptr, mtrr_strings[i]))
+ if (strcmp(ptr, mtrr_strings[i]))
continue;
base >>= PAGE_SHIFT;
size >>= PAGE_SHIFT;
@@ -173,6 +161,8 @@ mtrr_ioctl(struct inode *inode, struct f
struct mtrr_sentry sentry;
struct mtrr_gentry gentry;

+ printk("mtrr_ioctl %d\n", cmd);
+
switch (cmd) {
default:
return -ENOIOCTLCMD;
@@ -291,10 +281,9 @@ static int
mtrr_close(struct inode *ino, struct file *file)
{
int i, max;
- unsigned int *fcount = file->private_data;
+ unsigned int *fcount = FILE_FCOUNT(file);

- if (fcount == NULL)
- return 0;
+ if (fcount != NULL) {
max = num_var_ranges;
for (i = 0; i < max; ++i) {
while (fcount[i] > 0) {
@@ -304,13 +293,26 @@ mtrr_close(struct inode *ino, struct fil
}
}
kfree(fcount);
- file->private_data = NULL;
- return 0;
+ FILE_FCOUNT(file) = NULL;
+ }
+ return single_release(ino, file);
+}
+
+static int mtrr_open(struct inode *inode, struct file *file)
+{
+ if (!mtrr_if)
+ return -EIO;
+ if (!mtrr_if->get)
+ return -ENXIO;
+ return single_open(file, mtrr_seq_show, NULL);
}

static struct file_operations mtrr_fops = {
.owner = THIS_MODULE,
- .read = mtrr_read,
+ .open = mtrr_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
.write = mtrr_write,
.ioctl = mtrr_ioctl,
.release = mtrr_close,
@@ -329,14 +331,14 @@ char * attrib_to_str(int x)
return (x <= 6) ? mtrr_strings[x] : "?";
}

-void compute_ascii(void)
+static int mtrr_seq_show(struct seq_file *seq, void *offset)
{
char factor;
- int i, max;
+ int i, max, len;
mtrr_type type;
unsigned long base, size;

- ascii_buf_bytes = 0;
+ len = 0;
max = num_var_ranges;
for (i = 0; i < max; i++) {
mtrr_if->get(i, &base, &size, &type);
@@ -351,32 +353,19 @@ void compute_ascii(void)
factor = 'M';
size >>= 20 - PAGE_SHIFT;
}
- sprintf
- (ascii_buffer + ascii_buf_bytes,
+ /* RED-PEN: base can be > 32bit */
+ len += seq_printf(seq,
"reg%02i: base=0x%05lx000 (%4liMB), size=%4li%cB: %s, count=%d\n",
i, base, base >> (20 - PAGE_SHIFT), size, factor,
attrib_to_str(type), usage_table[i]);
- ascii_buf_bytes +=
- strlen(ascii_buffer + ascii_buf_bytes);
}
}
- devfs_set_file_size(devfs_handle, ascii_buf_bytes);
-# ifdef CONFIG_PROC_FS
- if (proc_root_mtrr)
- proc_root_mtrr->size = ascii_buf_bytes;
-# endif /* CONFIG_PROC_FS */
+ devfs_set_file_size(devfs_handle, len);
+ return 0;
}

static int __init mtrr_if_init(void)
{
- int max = num_var_ranges;
-
- if ((ascii_buffer = kmalloc(max * LINE_SIZE, GFP_KERNEL)) == NULL) {
- printk("mtrr: could not allocate\n");
- return -ENOMEM;
- }
- ascii_buf_bytes = 0;
- compute_ascii();
#ifdef CONFIG_PROC_FS
proc_root_mtrr =
create_proc_entry("mtrr", S_IWUSR | S_IRUGO, &proc_root);
diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/main.c linux/arch/i386/kernel/cpu/mtrr/main.c
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/main.c 2002-10-19 06:01:09.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/main.c 2002-11-27 11:43:26.000000000 +0100
@@ -36,7 +36,6 @@
#include <linux/pci.h>
#include <linux/smp.h>

-#define MTRR_NEED_STRINGS
#include <asm/mtrr.h>

#include <asm/uaccess.h>
@@ -54,6 +53,7 @@ static DECLARE_MUTEX(main_lock);
u32 size_or_mask, size_and_mask;

static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
+
struct mtrr_ops * mtrr_if = NULL;

__initdata char *mtrr_if_name[] = {
@@ -125,14 +125,6 @@ static void init_table(void)
}
for (i = 0; i < max; i++)
usage_table[i] = 1;
-#ifdef USERSPACE_INTERFACE
- if ((ascii_buffer = kmalloc(max * LINE_SIZE, GFP_KERNEL)) == NULL) {
- printk("mtrr: could not allocate\n");
- return;
- }
- ascii_buf_bytes = 0;
- compute_ascii();
-#endif
}

struct set_mtrr_data {
@@ -163,7 +155,7 @@ static void ipi_handler(void *info)
}

/* The master has cleared me to execute */
- if (data->smp_reg != ~0UL)
+ if (data->smp_reg != ~0U)
mtrr_if->set(data->smp_reg, data->smp_base,
data->smp_size, data->smp_type);
else
@@ -253,7 +245,7 @@ static void set_mtrr(unsigned int reg, u
* to replicate across all the APs.
* If we're doing that @reg is set to something special...
*/
- if (reg != ~0UL)
+ if (reg != ~0U)
mtrr_if->set(reg,base,size,type);

/* wait for the others */
@@ -361,7 +353,6 @@ int mtrr_add_page(unsigned long base, un
}
if (increment)
++usage_table[i];
- compute_ascii();
error = i;
goto out;
}
@@ -370,7 +361,6 @@ int mtrr_add_page(unsigned long base, un
if (i >= 0) {
set_mtrr(i, base, size, type);
usage_table[i] = 1;
- compute_ascii();
} else
printk("mtrr: no more MTRRs available\n");
error = i;
@@ -491,7 +481,6 @@ int mtrr_del_page(int reg, unsigned long
}
if (--usage_table[reg] < 1)
set_mtrr(reg, 0, 0, 0);
- compute_ascii();
error = reg;
out:
up(&main_lock);
@@ -547,7 +536,7 @@ static void init_other_cpus(void)
get_mtrr_state();

/* bring up the other processors */
- set_mtrr(~0UL,0,0,0);
+ set_mtrr(~0U,0,0,0);

if (use_intel()) {
finalize_mtrr_state();
@@ -583,7 +572,7 @@ static int __init mtrr_init(void)
query the width (in bits) of the physical
addressable memory on the Hammer family.
*/
- if (boot_cpu_data.x86 == 7
+ if (boot_cpu_data.x86 >= 7
&& (cpuid_eax(0x80000000) >= 0x80000008)) {
u32 phys_addr;
phys_addr = cpuid_eax(0x80000008) & 0xff;
@@ -643,5 +632,16 @@ static int __init mtrr_init(void)
return mtrr_if ? -ENXIO : 0;
}

+char *mtrr_strings[MTRR_NUM_TYPES] =
+{
+ "uncachable", /* 0 */
+ "write-combining", /* 1 */
+ "?", /* 2 */
+ "?", /* 3 */
+ "write-through", /* 4 */
+ "write-protect", /* 5 */
+ "write-back", /* 6 */
+};
+
core_initcall(mtrr_init);

diff -burpN -X ../KDIFX linux-vanilla/arch/i386/kernel/cpu/mtrr/mtrr.h linux/arch/i386/kernel/cpu/mtrr/mtrr.h
--- linux-vanilla/arch/i386/kernel/cpu/mtrr/mtrr.h 2002-10-19 06:01:18.000000000 +0200
+++ linux/arch/i386/kernel/cpu/mtrr/mtrr.h 2002-11-27 12:03:45.000000000 +0100
@@ -85,9 +85,6 @@ void get_mtrr_state(void);

extern void set_mtrr_ops(struct mtrr_ops * ops);

-/* Don't even ask... */
-extern void compute_ascii(void);
-
extern u32 size_or_mask, size_and_mask;
extern struct mtrr_ops * mtrr_if;

diff -burpN -X ../KDIFX linux-vanilla/include/asm-i386/mtrr.h linux/include/asm-i386/mtrr.h
--- linux-vanilla/include/asm-i386/mtrr.h 2002-10-19 06:01:52.000000000 +0200
+++ linux/include/asm-i386/mtrr.h 2002-11-27 12:41:20.000000000 +0100
@@ -65,21 +65,10 @@ struct mtrr_gentry
#define MTRR_TYPE_WRBACK 6
#define MTRR_NUM_TYPES 7

-#ifdef MTRR_NEED_STRINGS
-static char *mtrr_strings[MTRR_NUM_TYPES] =
-{
- "uncachable", /* 0 */
- "write-combining", /* 1 */
- "?", /* 2 */
- "?", /* 3 */
- "write-through", /* 4 */
- "write-protect", /* 5 */
- "write-back", /* 6 */
-};
-#endif
-
#ifdef __KERNEL__

+extern char *mtrr_strings[];
+
/* The following functions are for use by other drivers */
# ifdef CONFIG_MTRR
extern int mtrr_add (unsigned long base, unsigned long size,
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/