Skip to contentSkip to conversation

enhance x86 MTRR handling

PATCH In review Jan Beulich 8 participants 18 messages May 12, 2005 – May 19, 2005 Memory management

lore
Share & tools
lore

Jan Beulich

author
#0 lore raw

PATCH 1/1 enhance x86 MTRR handling

(Note: Patch also attached because the inline version is certain to get line wrapped.)

Until not so long ago, there were system log messages pointing to inconsistent MTRR setup of the video frame buffer caused by the way vesafb and X worked. While vesafb was fixed meanwhile, I believe fixing it there only hides a shortcoming in the MTRR code itself, in that that code is not symmetric with respect to the ordering of attempts to set up two (or more) regions where one contains the other. In the current shape, it permits only setting up sub-regions of pre-exisiting ones. The patch below makes this symmetric.

While working on that I noticed a few more inconsistencies in that code, namely
- use of 'unsigned int' for sizes in many, but not all places (the patch is converting this to use 'unsigned long' everywhere, which specifically might be necessary for x86-64 once a processor supporting more than 44 physical address bits would become available)
- the code to correct inconsistent settings during secondary processor startup tried (if necessary) to correct, among other things, the value in IA32_MTRR_DEF_TYPE, however the newly compute value would never get used to be stored in the respective MSR
- the generic range validation code checked that the end of the to-be-added range would be above 1MB; the value checked should have been the start of the range
- when contained regions are detected, previously this was allowed only when the old region was uncacheable; this can be symmetric (i.e. the new region can also be uncacheable) and even further as per Intel's documentation write-trough and write-back for either region is also compatible with the respective opposite in the other

Signed-off-by Jan Beulich <[email protected]>
arch/i386/kernel/cpu/mtrr/amd.c +1 -1
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/amd.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/amd.c
@@ -7,7 +7,7 @@
 
 static void
 amd_get_mtrr(unsigned int reg, unsigned long *base,
-	     unsigned int *size, mtrr_type * type)
+	     unsigned long *size, mtrr_type * type)
 {
 	unsigned long low, high;
arch/i386/kernel/cpu/mtrr/centaur.c +5 -4
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/centaur.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/centaur.c
@@ -17,7 +17,7 @@ static u8 centaur_mcr_type;	/* 0 for win
  */
 
 static int
-centaur_get_free_region(unsigned long base, unsigned long size)
+centaur_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -26,10 +26,11 @@ centaur_get_free_region(unsigned long ba
 {
 	int i, max;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned int lsize;
+	unsigned long lbase, lsize;
 
 	max = num_var_ranges;
+	if (replace_reg >= 0 && replace_reg < max)
+		return replace_reg;
 	for (i = 0; i < max; ++i) {
 		if (centaur_mcr_reserved & (1 << i))
 			continue;

@@ -49,7 +50,7 @@ mtrr_centaur_report_mcr(int mcr, u32 lo,
 
 static void
 centaur_get_mcr(unsigned int reg, unsigned long *base,
-		unsigned int *size, mtrr_type * type)
+		unsigned long *size, mtrr_type * type)
 {
 	*base = centaur_mcr[reg].high >> PAGE_SHIFT;
 	*size = -(centaur_mcr[reg].low & 0xfffff000) >> PAGE_SHIFT;
arch/i386/kernel/cpu/mtrr/cyrix.c +20 -5
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/cyrix.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/cyrix.c
@@ -9,7 +9,7 @@ int arr3_protected;
 
 static void
 cyrix_get_arr(unsigned int reg, unsigned long *base,
-	      unsigned int *size, mtrr_type * type)
+	      unsigned long *size, mtrr_type * type)
 {
 	unsigned long flags;
 	unsigned char arr, ccr3, rcr, shift;

@@ -77,7 +77,7 @@ cyrix_get_arr(unsigned int reg, unsigned
 }
 
 static int
-cyrix_get_free_region(unsigned long base, unsigned long size)
+cyrix_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free ARR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -86,9 +86,24 @@ cyrix_get_free_region(unsigned long base
 {
 	int i;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned int  lsize;
+	unsigned long lbase, lsize;
 
+	switch (replace_reg) {
+	case 7:
+		if (size < 0x40)
+			break;
+	case 6:
+	case 5:
+	case 4:
+		return replace_reg;
+	case 3:
+		if (arr3_protected)
+			break;
+	case 2:
+	case 1:
+	case 0:
+		return replace_reg;
+	}
 	/* If we are to set up a region >32M then look at ARR7 immediately */
 	if (size > 0x2000) {
 		cyrix_get_arr(7, &lbase, &lsize, &ltype);

@@ -214,7 +229,7 @@ static void cyrix_set_arr(unsigned int r
 
 typedef struct {
 	unsigned long base;
-	unsigned int size;
+	unsigned long size;
 	mtrr_type type;
 } arr_state_t;
arch/i386/kernel/cpu/mtrr/generic.c +12 -10
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/generic.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/generic.c
@@ -103,7 +103,7 @@ void mtrr_wrmsr(unsigned msr, unsigned a
 			smp_processor_id(), msr, a, b);
 }
 
-int generic_get_free_region(unsigned long base, unsigned long size)
+int generic_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -112,10 +112,11 @@ int generic_get_free_region(unsigned lon
 {
 	int i, max;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned lsize;
+	unsigned long lbase, lsize;
 
 	max = num_var_ranges;
+	if (replace_reg >= 0 && replace_reg < max)
+		return replace_reg;
 	for (i = 0; i < max; ++i) {
 		mtrr_if->get(i, &lbase, &lsize, &ltype);
 		if (lsize == 0)

@@ -125,7 +126,7 @@ int generic_get_free_region(unsigned lon
 }
 
 static void generic_get_mtrr(unsigned int reg, unsigned long *base,
-			     unsigned int *size, mtrr_type * type)
+			     unsigned long *size, mtrr_type *type)
 {
 	unsigned int mask_lo, mask_hi, base_lo, base_hi;
 

@@ -210,7 +211,9 @@ static int set_mtrr_var_ranges(unsigned 
 	return changed;
 }
 
-static unsigned long set_mtrr_state(u32 deftype_lo, u32 deftype_hi)
+static u32 deftype_lo, deftype_hi;
+
+static unsigned long set_mtrr_state(void)
 /*  [SUMMARY] Set the MTRR state for this CPU.
     <state> The MTRR state information to read.
     <ctxt> Some relevant CPU context.

@@ -232,7 +235,7 @@ static unsigned long set_mtrr_state(u32 
 	   so to set it we fiddle with the saved value  */
 	if ((deftype_lo & 0xff) != mtrr_state.def_type
 	    || ((deftype_lo & 0xc00) >> 10) != mtrr_state.enabled) {
-		deftype_lo |= (mtrr_state.def_type | mtrr_state.enabled << 10);
+		deftype_lo = (deftype_lo & ~0xcff) | mtrr_state.def_type | (mtrr_state.enabled << 10);
 		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
 	}
 

@@ -241,7 +244,6 @@ static unsigned long set_mtrr_state(u32 
 
 
 static unsigned long cr4 = 0;
-static u32 deftype_lo, deftype_hi;
 static DEFINE_SPINLOCK(set_atomicity_lock);
 
 /*

@@ -279,7 +281,7 @@ static void prepare_set(void)
 	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
 
 	/*  Disable MTRRs, and set the default type to uncached  */
-	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo & 0xf300UL, deftype_hi);
+	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo & ~0xcff, deftype_hi);
 }
 
 static void post_set(void)

@@ -308,7 +310,7 @@ static void generic_set_all(void)
 	prepare_set();
 
 	/* Actually set the state */
-	mask = set_mtrr_state(deftype_lo,deftype_hi);
+	mask = set_mtrr_state();
 
 	post_set();
 	local_irq_restore(flags);

@@ -375,7 +377,7 @@ int generic_validate_add_page(unsigned l
 		}
 	}
 
-	if (base + size < 0x100) {
+	if (base < 0x100) {
 		printk(KERN_WARNING "mtrr: cannot set region below 1 MiB (0x%lx000,0x%lx000)\n",
 		       base, size);
 		return -EINVAL;
arch/i386/kernel/cpu/mtrr/if.c +15 -9
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/if.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/if.c
@@ -155,6 +155,7 @@ mtrr_ioctl(struct inode *inode, struct f
 {
 	int err;
 	mtrr_type type;
+	unsigned long size;
 	struct mtrr_sentry sentry;
 	struct mtrr_gentry gentry;
 	void __user *arg = (void __user *) __arg;

@@ -205,15 +206,15 @@ mtrr_ioctl(struct inode *inode, struct f
 			return -EFAULT;
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
+		mtrr_if->get(gentry.regnum, &gentry.base, &size, &type);
 
 		/* Hide entries that go above 4GB */
-		if (gentry.base + gentry.size > 0x100000
-		    || gentry.size == 0x100000)
+		if (gentry.base + size - 1 >= (1UL << (8 * sizeof(gentry.size) - PAGE_SHIFT))
+		    || size >= (1UL << (8 * sizeof(gentry.size) - PAGE_SHIFT)))
 			gentry.base = gentry.size = gentry.type = 0;
 		else {
 			gentry.base <<= PAGE_SHIFT;
-			gentry.size <<= PAGE_SHIFT;
+			gentry.size = size << PAGE_SHIFT;
 			gentry.type = type;
 		}
 

@@ -263,8 +264,14 @@ mtrr_ioctl(struct inode *inode, struct f
 			return -EFAULT;
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
-		gentry.type = type;
+		mtrr_if->get(gentry.regnum, &gentry.base, &size, &type);
+		/* Hide entries that would overflow */
+		if (size != (__typeof__(gentry.size))size)
+			gentry.base = gentry.size = gentry.type = 0;
+		else {
+			gentry.size = size;
+			gentry.type = type;
+		}
 
 		if (copy_to_user(arg, &gentry, sizeof gentry))
 			return -EFAULT;

@@ -323,8 +330,7 @@ static int mtrr_seq_show(struct seq_file
 	char factor;
 	int i, max, len;
 	mtrr_type type;
-	unsigned long base;
-	unsigned int size;
+	unsigned long base, size;
 
 	len = 0;
 	max = num_var_ranges;

@@ -343,7 +349,7 @@ static int mtrr_seq_show(struct seq_file
 			}
 			/* RED-PEN: base can be > 32bit */ 
 			len += seq_printf(seq, 
-				   "reg%02i: base=0x%05lx000 (%4liMB), size=%4i%cB: %s, count=%d\n",
+				   "reg%02i: base=0x%05lx000 (%4luMB), size=%4lu%cB: %s, count=%d\n",
 			     i, base, base >> (20 - PAGE_SHIFT), size, factor,
 			     mtrr_attrib_to_str(type), usage_table[i]);
 		}
arch/i386/kernel/cpu/mtrr/main.c +40 -15

114 lines not on this page · raw message

arch/i386/kernel/cpu/mtrr/mtrr.h +5 -4
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/mtrr.h linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/mtrr.h
@@ -43,15 +43,16 @@ struct mtrr_ops {
 	void	(*set_all)(void);
 
 	void	(*get)(unsigned int reg, unsigned long *base,
-		       unsigned int *size, mtrr_type * type);
-	int	(*get_free_region) (unsigned long base, unsigned long size);
-
+		       unsigned long *size, mtrr_type * type);
+	int	(*get_free_region)(unsigned long base, unsigned long size,
+				   int replace_reg);
 	int	(*validate_add_page)(unsigned long base, unsigned long size,
 				     unsigned int type);
 	int	(*have_wrcomb)(void);
 };
 
-extern int generic_get_free_region(unsigned long base, unsigned long size);
+extern int generic_get_free_region(unsigned long base, unsigned long size,
+				   int replace_reg);
 extern int generic_validate_add_page(unsigned long base, unsigned long size,
 				     unsigned int type);

(Note: Patch also attached because the inline version is certain to get line wrapped.)

Until not so long ago, there were system log messages pointing to inconsistent MTRR setup of the video frame buffer caused by the way vesafb and X worked. While vesafb was fixed meanwhile, I believe fixing it there only hides a shortcoming in the MTRR code itself, in that that code is not symmetric with respect to the ordering of attempts to set up two (or more) regions where one contains the other. In the current shape, it permits only setting up sub-regions of pre-exisiting ones. The patch below makes this symmetric.

While working on that I noticed a few more inconsistencies in that code, namely
- use of 'unsigned int' for sizes in many, but not all places (the patch is converting this to use 'unsigned long' everywhere, which specifically might be necessary for x86-64 once a processor supporting more than 44 physical address bits would become available)
- the code to correct inconsistent settings during secondary processor startup tried (if necessary) to correct, among other things, the value in IA32_MTRR_DEF_TYPE, however the newly compute value would never get used to be stored in the respective MSR
- the generic range validation code checked that the end of the to-be-added range would be above 1MB; the value checked should have been the start of the range
- when contained regions are detected, previously this was allowed only when the old region was uncacheable; this can be symmetric (i.e. the new region can also be uncacheable) and even further as per Intel's documentation write-trough and write-back for either region is also compatible with the respective opposite in the other

Signed-off-by Jan Beulich <[email protected]>
arch/i386/kernel/cpu/mtrr/amd.c +1 -1
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/amd.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/amd.c
@@ -7,7 +7,7 @@
 
 static void
 amd_get_mtrr(unsigned int reg, unsigned long *base,
-	     unsigned int *size, mtrr_type * type)
+	     unsigned long *size, mtrr_type * type)
 {
 	unsigned long low, high;
arch/i386/kernel/cpu/mtrr/centaur.c +5 -4
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/centaur.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/centaur.c
@@ -17,7 +17,7 @@ static u8 centaur_mcr_type;	/* 0 for win
  */
 
 static int
-centaur_get_free_region(unsigned long base, unsigned long size)
+centaur_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -26,10 +26,11 @@ centaur_get_free_region(unsigned long ba
 {
 	int i, max;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned int lsize;
+	unsigned long lbase, lsize;
 
 	max = num_var_ranges;
+	if (replace_reg >= 0 && replace_reg < max)
+		return replace_reg;
 	for (i = 0; i < max; ++i) {
 		if (centaur_mcr_reserved & (1 << i))
 			continue;

@@ -49,7 +50,7 @@ mtrr_centaur_report_mcr(int mcr, u32 lo,
 
 static void
 centaur_get_mcr(unsigned int reg, unsigned long *base,
-		unsigned int *size, mtrr_type * type)
+		unsigned long *size, mtrr_type * type)
 {
 	*base = centaur_mcr[reg].high >> PAGE_SHIFT;
 	*size = -(centaur_mcr[reg].low & 0xfffff000) >> PAGE_SHIFT;
arch/i386/kernel/cpu/mtrr/cyrix.c +20 -5
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/cyrix.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/cyrix.c
@@ -9,7 +9,7 @@ int arr3_protected;
 
 static void
 cyrix_get_arr(unsigned int reg, unsigned long *base,
-	      unsigned int *size, mtrr_type * type)
+	      unsigned long *size, mtrr_type * type)
 {
 	unsigned long flags;
 	unsigned char arr, ccr3, rcr, shift;

@@ -77,7 +77,7 @@ cyrix_get_arr(unsigned int reg, unsigned
 }
 
 static int
-cyrix_get_free_region(unsigned long base, unsigned long size)
+cyrix_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free ARR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -86,9 +86,24 @@ cyrix_get_free_region(unsigned long base
 {
 	int i;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned int  lsize;
+	unsigned long lbase, lsize;
 
+	switch (replace_reg) {
+	case 7:
+		if (size < 0x40)
+			break;
+	case 6:
+	case 5:
+	case 4:
+		return replace_reg;
+	case 3:
+		if (arr3_protected)
+			break;
+	case 2:
+	case 1:
+	case 0:
+		return replace_reg;
+	}
 	/* If we are to set up a region >32M then look at ARR7 immediately */
 	if (size > 0x2000) {
 		cyrix_get_arr(7, &lbase, &lsize, &ltype);

@@ -214,7 +229,7 @@ static void cyrix_set_arr(unsigned int r
 
 typedef struct {
 	unsigned long base;
-	unsigned int size;
+	unsigned long size;
 	mtrr_type type;
 } arr_state_t;
arch/i386/kernel/cpu/mtrr/generic.c +12 -10
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/generic.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/generic.c
@@ -103,7 +103,7 @@ void mtrr_wrmsr(unsigned msr, unsigned a
 			smp_processor_id(), msr, a, b);
 }
 
-int generic_get_free_region(unsigned long base, unsigned long size)
+int generic_get_free_region(unsigned long base, unsigned long size, int replace_reg)
 /*  [SUMMARY] Get a free MTRR.
     <base> The starting (base) address of the region.
     <size> The size (in bytes) of the region.

@@ -112,10 +112,11 @@ int generic_get_free_region(unsigned lon
 {
 	int i, max;
 	mtrr_type ltype;
-	unsigned long lbase;
-	unsigned lsize;
+	unsigned long lbase, lsize;
 
 	max = num_var_ranges;
+	if (replace_reg >= 0 && replace_reg < max)
+		return replace_reg;
 	for (i = 0; i < max; ++i) {
 		mtrr_if->get(i, &lbase, &lsize, &ltype);
 		if (lsize == 0)

@@ -125,7 +126,7 @@ int generic_get_free_region(unsigned lon
 }
 
 static void generic_get_mtrr(unsigned int reg, unsigned long *base,
-			     unsigned int *size, mtrr_type * type)
+			     unsigned long *size, mtrr_type *type)
 {
 	unsigned int mask_lo, mask_hi, base_lo, base_hi;
 

@@ -210,7 +211,9 @@ static int set_mtrr_var_ranges(unsigned 
 	return changed;
 }
 
-static unsigned long set_mtrr_state(u32 deftype_lo, u32 deftype_hi)
+static u32 deftype_lo, deftype_hi;
+
+static unsigned long set_mtrr_state(void)
 /*  [SUMMARY] Set the MTRR state for this CPU.
     <state> The MTRR state information to read.
     <ctxt> Some relevant CPU context.

@@ -232,7 +235,7 @@ static unsigned long set_mtrr_state(u32 
 	   so to set it we fiddle with the saved value  */
 	if ((deftype_lo & 0xff) != mtrr_state.def_type
 	    || ((deftype_lo & 0xc00) >> 10) != mtrr_state.enabled) {
-		deftype_lo |= (mtrr_state.def_type | mtrr_state.enabled << 10);
+		deftype_lo = (deftype_lo & ~0xcff) | mtrr_state.def_type | (mtrr_state.enabled << 10);
 		change_mask |= MTRR_CHANGE_MASK_DEFTYPE;
 	}
 

@@ -241,7 +244,6 @@ static unsigned long set_mtrr_state(u32 
 
 
 static unsigned long cr4 = 0;
-static u32 deftype_lo, deftype_hi;
 static DEFINE_SPINLOCK(set_atomicity_lock);
 
 /*

@@ -279,7 +281,7 @@ static void prepare_set(void)
 	rdmsr(MTRRdefType_MSR, deftype_lo, deftype_hi);
 
 	/*  Disable MTRRs, and set the default type to uncached  */
-	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo & 0xf300UL, deftype_hi);
+	mtrr_wrmsr(MTRRdefType_MSR, deftype_lo & ~0xcff, deftype_hi);
 }
 
 static void post_set(void)

@@ -308,7 +310,7 @@ static void generic_set_all(void)
 	prepare_set();
 
 	/* Actually set the state */
-	mask = set_mtrr_state(deftype_lo,deftype_hi);
+	mask = set_mtrr_state();
 
 	post_set();
 	local_irq_restore(flags);

@@ -375,7 +377,7 @@ int generic_validate_add_page(unsigned l
 		}
 	}
 
-	if (base + size < 0x100) {
+	if (base < 0x100) {
 		printk(KERN_WARNING "mtrr: cannot set region below 1 MiB (0x%lx000,0x%lx000)\n",
 		       base, size);
 		return -EINVAL;
arch/i386/kernel/cpu/mtrr/if.c +15 -9
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/if.c linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/if.c
@@ -155,6 +155,7 @@ mtrr_ioctl(struct inode *inode, struct f
 {
 	int err;
 	mtrr_type type;
+	unsigned long size;
 	struct mtrr_sentry sentry;
 	struct mtrr_gentry gentry;
 	void __user *arg = (void __user *) __arg;

@@ -205,15 +206,15 @@ mtrr_ioctl(struct inode *inode, struct f
 			return -EFAULT;
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
+		mtrr_if->get(gentry.regnum, &gentry.base, &size, &type);
 
 		/* Hide entries that go above 4GB */
-		if (gentry.base + gentry.size > 0x100000
-		    || gentry.size == 0x100000)
+		if (gentry.base + size - 1 >= (1UL << (8 * sizeof(gentry.size) - PAGE_SHIFT))
+		    || size >= (1UL << (8 * sizeof(gentry.size) - PAGE_SHIFT)))
 			gentry.base = gentry.size = gentry.type = 0;
 		else {
 			gentry.base <<= PAGE_SHIFT;
-			gentry.size <<= PAGE_SHIFT;
+			gentry.size = size << PAGE_SHIFT;
 			gentry.type = type;
 		}
 

@@ -263,8 +264,14 @@ mtrr_ioctl(struct inode *inode, struct f
 			return -EFAULT;
 		if (gentry.regnum >= num_var_ranges)
 			return -EINVAL;
-		mtrr_if->get(gentry.regnum, &gentry.base, &gentry.size, &type);
-		gentry.type = type;
+		mtrr_if->get(gentry.regnum, &gentry.base, &size, &type);
+		/* Hide entries that would overflow */
+		if (size != (__typeof__(gentry.size))size)
+			gentry.base = gentry.size = gentry.type = 0;
+		else {
+			gentry.size = size;
+			gentry.type = type;
+		}
 
 		if (copy_to_user(arg, &gentry, sizeof gentry))
 			return -EFAULT;

@@ -323,8 +330,7 @@ static int mtrr_seq_show(struct seq_file
 	char factor;
 	int i, max, len;
 	mtrr_type type;
-	unsigned long base;
-	unsigned int size;
+	unsigned long base, size;
 
 	len = 0;
 	max = num_var_ranges;

@@ -343,7 +349,7 @@ static int mtrr_seq_show(struct seq_file
 			}
 			/* RED-PEN: base can be > 32bit */ 
 			len += seq_printf(seq, 
-				   "reg%02i: base=0x%05lx000 (%4liMB), size=%4i%cB: %s, count=%d\n",
+				   "reg%02i: base=0x%05lx000 (%4luMB), size=%4lu%cB: %s, count=%d\n",
 			     i, base, base >> (20 - PAGE_SHIFT), size, factor,
 			     mtrr_attrib_to_str(type), usage_table[i]);
 		}
arch/i386/kernel/cpu/mtrr/main.c +40 -15

114 lines not on this page · raw message

arch/i386/kernel/cpu/mtrr/mtrr.h +5 -4
diff -Npru linux-2.6.12-rc4.base/arch/i386/kernel/cpu/mtrr/mtrr.h linux-2.6.12-rc4/arch/i386/kernel/cpu/mtrr/mtrr.h
@@ -43,15 +43,16 @@ struct mtrr_ops {
 	void	(*set_all)(void);
 
 	void	(*get)(unsigned int reg, unsigned long *base,
-		       unsigned int *size, mtrr_type * type);
-	int	(*get_free_region) (unsigned long base, unsigned long size);
-
+		       unsigned long *size, mtrr_type * type);
+	int	(*get_free_region)(unsigned long base, unsigned long size,
+				   int replace_reg);
 	int	(*validate_add_page)(unsigned long base, unsigned long size,
 				     unsigned int type);
 	int	(*have_wrcomb)(void);
 };
 
-extern int generic_get_free_region(unsigned long base, unsigned long size);
+extern int generic_get_free_region(unsigned long base, unsigned long size,
+				   int replace_reg);
 extern int generic_validate_add_page(unsigned long base, unsigned long size,
 				     unsigned int type);
#1 lore raw
Quoted 4 lines · Jan Beulich

- the code to correct inconsistent settings during secondary processor startup tried (if necessary) to correct, among other things, the value in IA32_MTRR_DEF_TYPE, however the newly compute value would never get used to be stored in the respective MSR

Quoted 8 lines

- the generic range validation code checked that the end of the to-be-added range would be above 1MB; the value checked should have been the start of the range
- when contained regions are detected, previously this was allowed only when the old region was uncacheable; this can be symmetric (i.e. the new region can also be uncacheable) and even further as per Intel's documentation write-trough and write-back for either region is also compatible with the respective opposite in the other

Whilst your changes may have merit, I'd much rather see effort spent on getting PAT into shape than further massaging MTRR. Its well past its smell-by-date, and theres been no activity whatsoever afaik on getting Terrence Ripperda's cachemap stuff beaten into shape.

I'll dust off the last version he sent and diff against latest-mm later so it can get some more commentary. It seems everyone is in violent agreement that we want PAT support, but nothing seems to happen.

Dave

#2 lore raw
Quoted 10 lines · Dave Jones

Whilst your changes may have merit, I'd much rather see effort spent on getting PAT into shape than further massaging MTRR. Its well past its smell-by-date, and theres been no activity whatsoever afaik on getting Terrence Ripperda's cachemap stuff beaten into shape.

I'll dust off the last version he sent and diff against latest-mm later so it can get some more commentary. It seems everyone is in violent agreement that we want PAT support, but nothing seems to happen.

Yes we tried to contact the cachemap author but just found dead email addresses, so in the end we switched to using PAT.
But rather that make kernel changes, we simply wrote a startup utility to program the MSR registers using /dev/cpu/%d/msr
Our device driver then reads the IA32CR_PAT MSR and looks for a write-combining entry (value 0x01) and uses the slot # to program the correct PTE bits.

But having a default write-combining MSR slot and a defined call to enable it would be good; I believe Hugo Kohmann from Dolphin has a patch to do this for x86_64 at least.

Cheers
Addy.

#3 lore raw

RFC 1/1 Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

In reply to Dave Jones
Quoted 8 lines · Dave Jones

Whilst your changes may have merit, I'd much rather see effort spent on getting PAT into shape than further massaging MTRR. Its well past its smell-by-date, and theres been no activity whatsoever afaik on getting Terrence Ripperda's cachemap stuff beaten into shape.

I'll dust off the last version he sent and diff against latest-mm later so it can get some more commentary. It seems everyone is in violent agreement that we want PAT support, but nothing seems to happen.

I rediffed the last version of Terrence's patch that I could find (from 2.6.4), and fixed up some API changes (PCI changes), and some obvious codingstyle nits. (no doubt I've missed some, but lets concentrate on the more important things first eh?)

It compiles on i386, but I've done no runtime testing yet.

Obvious things that jump out at me.

: x86-64 will need updating to also take advantage of this.
It may be able to just copy the i386 includes as-is, I've not looked closely at the PAT related changes on x86-64 yet. Andi?

: The list manipulation macros in mm/cachemap.c are a little fugly.

Anything else ?

Dave

arch/i386/kernel/cpu/common.c +80 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/arch/i386/kernel/cpu/common.c 2.6-mm-cachemap/arch/i386/kernel/cpu/common.c
@@ -10,6 +10,9 @@
 #include <asm/msr.h>
 #include <asm/io.h>
 #include <asm/mmu_context.h>
+#include <asm/pat.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
 #ifdef CONFIG_X86_LOCAL_APIC
 #include <asm/mpspec.h>
 #include <asm/apic.h>

@@ -315,6 +318,48 @@ static int __init x86_serial_nr_setup(ch
 }
 __setup("serialnumber", x86_serial_nr_setup);
 
+static atomic_t pat_cpus_enabled = ATOMIC_INIT(0);
+
+static void __init setup_pat(void)
+{
+	unsigned long flags = 0, cr4val = 0, cr0val = 0;
+
+	if (!cpu_has_pat)
+		return;
+
+	local_save_flags(flags);
+	local_irq_disable();
+	if (cpu_has_pge) {
+		cr4val = read_cr4();
+		write_cr4(cr4val & (unsigned char) ~(1 << 7));
+	}
+	cr0val = read_cr0() | (1 << 30);
+	wbinvd();
+	write_cr0(cr0val);
+	wbinvd();
+	/* Okay everything is ready */
+
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_AMD:
+		wrmsr(IA32_CR_PAT, AMD_PAT_31_0, AMD_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	case X86_VENDOR_INTEL:
+		wrmsr(IA32_CR_PAT, INTEL_PAT_31_0, INTEL_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	default:
+		printk("Unknown vendor in setup_pat()\n");
+	}
+
+	/* Reenable the caches */
+	wbinvd();
+	write_cr0(read_cr0() & ~(1 << 30));
+	if (cpu_has_pge)
+		write_cr4(cr4val);
+
+	local_irq_restore(flags);
+}
 
 
 /*

@@ -431,6 +476,8 @@ void __devinit identify_cpu(struct cpuin
 	if (c == &boot_cpu_data)
 		sysenter_setup();
 	enable_sep_cpu();
+
+	setup_pat();
 }
 
 #ifdef CONFIG_X86_HT

@@ -659,3 +706,36 @@ void __devinit cpu_uninit(void)
 	per_cpu(cpu_tlbstate, cpu).active_mm = &init_mm;
 }
 #endif
+
+int cpu_supports_wrcomb(void)
+{
+	struct pci_dev *dev;
+
+	/* checks copied from arch/i386/kernel/cpu/mtrr/main.c */
+	/* do these only apply to mtrrs or pat as well? */
+	dev = pci_get_device(PCI_VENDOR_ID_SERVERWORKS,
+					PCI_DEVICE_ID_SERVERWORKS_LE, NULL);
+	if (dev) {
+		printk(KERN_INFO
+			"mtrr: Serverworks LE detected. Write-combining disabled.\n");
+			return 0;
+	}
+	/* Intel 450NX errata # 23. Non ascending cachline evictions to
+	write combining memory may resulting in data corruption */
+	dev = pci_get_device(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82451NX, NULL);
+	if (dev) {
+		printk(KERN_INFO
+			"mtrr: Intel 450NX MMC detected. Write-combining disabled.\n");
+		return 0;
+	}
+
+	if (cpu_has_pat) {
+		if (atomic_read(&pat_cpus_enabled) != num_online_cpus()) {
+			printk("not all cpus have pat enabled?\n");
+			BUG();
+		}
+		return 1;
+	}
+	return 0;
+}
+
arch/i386/kernel/cpu/mtrr/generic.c +15 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/arch/i386/kernel/cpu/mtrr/generic.c 2.6-mm-cachemap/arch/i386/kernel/cpu/mtrr/generic.c
@@ -3,6 +3,7 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/mm.h>
+#include <linux/cachemap.h>
 #include <asm/io.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>

@@ -27,6 +28,20 @@ get_mtrr_var_range(unsigned int index, s
 {
 	rdmsr(MTRRphysBase_MSR(index), vr->base_lo, vr->base_hi);
 	rdmsr(MTRRphysMask_MSR(index), vr->mask_lo, vr->mask_hi);
+
+	if (vr->mask_lo & 0x800) {
+		unsigned long start, size;
+
+		size =
+		    -(size_or_mask | vr->mask_hi << (32 - PAGE_SHIFT) | vr->
+		      mask_lo >> PAGE_SHIFT);
+		start =
+		    vr->base_hi << (32 -
+				    PAGE_SHIFT) | vr->base_lo >> PAGE_SHIFT;
+
+		cmap_report_range(start << PAGE_SHIFT, size << PAGE_SHIFT,
+			cmap_convert_mtrr_type(vr->base_lo & 0xff));
+	}
 }
 
 static void __init
arch/i386/kernel/cpu/mtrr/main.c +6 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/arch/i386/kernel/cpu/mtrr/main.c 2.6-mm-cachemap/arch/i386/kernel/cpu/mtrr/main.c
@@ -36,6 +36,7 @@
 #include <linux/pci.h>
 #include <linux/smp.h>
 #include <linux/cpu.h>
+#include <linux/cachemap.h>
 
 #include <asm/mtrr.h>
 

@@ -365,6 +366,8 @@ int mtrr_add_page(unsigned long base, un
 	/*  Search for an empty MTRR  */
 	i = mtrr_if->get_free_region(base, size);
 	if (i >= 0) {
+		cmap_report_range(base<<PAGE_SHIFT, size<<PAGE_SHIFT,
+			cmap_convert_mtrr_type(type));
 		set_mtrr(i, base, size, type);
 		usage_table[i] = 1;
 	} else

@@ -497,7 +500,10 @@ int mtrr_del_page(int reg, unsigned long
 		goto out;
 	}
 	if (--usage_table[reg] < 1)
+	{
+		cmap_release_range(lbase<<PAGE_SHIFT, lsize<<PAGE_SHIFT);
 		set_mtrr(reg, 0, 0, 0);
+	}
 	error = reg;
  out:
 	up(&main_lock);
arch/i386/mm/ioremap.c +13 -1
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/arch/i386/mm/ioremap.c 2.6-mm-cachemap/arch/i386/mm/ioremap.c
@@ -11,6 +11,7 @@
 #include <linux/vmalloc.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/cachemap.h>
 #include <asm/io.h>
 #include <asm/fixmap.h>
 #include <asm/cacheflush.h>

@@ -150,17 +151,23 @@ void __iomem * __ioremap(unsigned long p
 	phys_addr &= PAGE_MASK;
 	size = PAGE_ALIGN(last_addr+1) - phys_addr;
 
+	if (cmap_request_range(phys_addr, size, cmap_convert_flags(flags)))
+		return NULL;
+
 	/*
 	 * Ok, go for it..
 	 */
 	area = get_vm_area(size, VM_IOREMAP | (flags << 20));
-	if (!area)
+	if (!area) {
+		cmap_release_range(phys_addr, size);
 		return NULL;
+	}
 	area->phys_addr = phys_addr;
 	addr = (void __iomem *) area->addr;
 	if (ioremap_page_range((unsigned long) addr,
 			(unsigned long) addr + size, phys_addr, flags)) {
 		vunmap((void __force *) addr);
+		cmap_release_range(phys_addr, size);
 		return NULL;
 	}
 	return (void __iomem *) (offset + (char __iomem *)addr);

@@ -252,6 +259,11 @@ void iounmap(volatile void __iomem *addr
 				 PAGE_KERNEL);
 		global_flush_tlb();
 	} 
+
+	/* should this be if (p->flags) ?? */
+	/* vm_find_area pads by a page size */
+	cmap_release_range(p->phys_addr, p->size - PAGE_SIZE);
+
 	kfree(p); 
 }
include/asm-i386/cachemap.h +76 -0

76 lines not on this page · raw message

include/asm-i386/cpufeature.h +1 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/include/asm-i386/cpufeature.h 2.6-mm-cachemap/include/asm-i386/cpufeature.h
@@ -100,6 +100,7 @@
 #define cpu_has_tsc		boot_cpu_has(X86_FEATURE_TSC)
 #define cpu_has_pae		boot_cpu_has(X86_FEATURE_PAE)
 #define cpu_has_pge		boot_cpu_has(X86_FEATURE_PGE)
+#define cpu_has_pat		boot_cpu_has(X86_FEATURE_PAT)
 #define cpu_has_apic		boot_cpu_has(X86_FEATURE_APIC)
 #define cpu_has_sep		boot_cpu_has(X86_FEATURE_SEP)
 #define cpu_has_mtrr		boot_cpu_has(X86_FEATURE_MTRR)
include/asm-i386/pat.h +77 -0

77 lines not on this page · raw message

include/asm-i386/pgtable.h +6 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/include/asm-i386/pgtable.h 2.6-mm-cachemap/include/asm-i386/pgtable.h
@@ -119,6 +119,9 @@ void paging_init(void);
 #define _PAGE_UNUSED2	0x400
 #define _PAGE_UNUSED3	0x800
 
+/* Added define for write combining page, only valid if pat enabled. */
+#define _PAGE_WRCOMB   _PAGE_PWT
+
 #define _PAGE_FILE	0x040	/* set:pagecache unset:swap */
 #define _PAGE_PROTNONE	0x080	/* If not present */
 #ifdef CONFIG_X86_PAE

@@ -157,6 +160,7 @@ void paging_init(void);
 extern unsigned long long __PAGE_KERNEL, __PAGE_KERNEL_EXEC;
 #define __PAGE_KERNEL_RO		(__PAGE_KERNEL & ~_PAGE_RW)
 #define __PAGE_KERNEL_NOCACHE		(__PAGE_KERNEL | _PAGE_PCD)
+#define __PAGE_KERNEL_WRCOMB		(__PAGE_KERNEL | _PAGE_WRCOMB)
 #define __PAGE_KERNEL_LARGE		(__PAGE_KERNEL | _PAGE_PSE)
 #define __PAGE_KERNEL_LARGE_EXEC	(__PAGE_KERNEL_EXEC | _PAGE_PSE)
 

@@ -166,6 +170,8 @@ extern unsigned long long __PAGE_KERNEL,
 #define PAGE_KERNEL_NOCACHE	__pgprot(__PAGE_KERNEL_NOCACHE)
 #define PAGE_KERNEL_LARGE	__pgprot(__PAGE_KERNEL_LARGE)
 #define PAGE_KERNEL_LARGE_EXEC	__pgprot(__PAGE_KERNEL_LARGE_EXEC)
+#define PAGE_KERNEL_WRCOMB	__pgprot(__PAGE_KERNEL_WRCOMB)
+
 
 /*
  * The i386 can't do page protection for execute, and considers that
include/asm-i386/processor.h +2 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/include/asm-i386/processor.h 2.6-mm-cachemap/include/asm-i386/processor.h
@@ -696,4 +696,6 @@ extern unsigned long boot_option_idle_ov
 extern void enable_sep_cpu(void);
 extern int sysenter_setup(void);
 
+extern int cpu_supports_wrcomb(void);
+
 #endif /* __ASM_I386_PROCESSOR_H */
include/linux/cachemap.h +29 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/include/linux/cachemap.h 2.6-mm-cachemap/include/linux/cachemap.h
@@ -0,0 +1,29 @@
+
+#ifndef __CACHEMAP_H
+#define __CACHEMAP_H
+
+typedef struct { unsigned long cmap_prot; } cmap_prot_t;
+
+#define __cmap_prot(x)       ((cmap_prot_t) { (x) } )
+#define cmap_prot_val(x)     ((x).cmap_prot)
+
+#define _CMAP_PAGE_CACHED    (1<<0)
+#define _CMAP_PAGE_NOCACHED  (1<<1)
+#define _CMAP_PAGE_WRCOMB    (1<<2)
+#define _CMAP_PAGE_RO        (1<<3)
+
+#define CMAP_PAGE_CACHED     __cmap_prot(_CMAP_PAGE_CACHED)
+#define CMAP_PAGE_NOCACHED   __cmap_prot(_CMAP_PAGE_NOCACHED)
+#define CMAP_PAGE_WRCOMB     __cmap_prot(_CMAP_PAGE_WRCOMB)
+#define CMAP_PAGE_RO         __cmap_prot(_CMAP_PAGE_RO)
+
+#include <asm/cachemap.h>
+
+int cmap_init(void);
+int cmap_request_range(unsigned long start, unsigned long size,
+		       cmap_prot_t caching);
+int cmap_report_range(unsigned long start, unsigned long size,
+		      cmap_prot_t caching);
+int cmap_release_range(unsigned long start, unsigned long size);
+
+#endif				/* __CACHEMAP_H */
init/main.c +4 -0
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/init/main.c 2.6-mm-cachemap/init/main.c
@@ -43,6 +43,7 @@
 #include <linux/cpu.h>
 #include <linux/cpuset.h>
 #include <linux/efi.h>
+#include <linux/cachemap.h>
 #include <linux/unistd.h>
 #include <linux/rmap.h>
 #include <linux/mempolicy.h>

@@ -523,6 +524,9 @@ asmlinkage void __init start_kernel(void
 
 	acpi_early_init(); /* before LAPIC and SMP init */
 
+	/* initializing here catches most of the early ioremaps */
+	cmap_init();
+
 	/* Do the rest non-__init'ed, we're now alive */
 	rest_init();
 }
mm/cachemap.c +867 -0

867 lines not on this page · raw message

mm/Makefile +1 -1
diff -urpN --exclude-from=/home/davej/.exclude 2.6-mm/mm/Makefile 2.6-mm-cachemap/mm/Makefile
@@ -10,7 +10,7 @@ mmu-$(CONFIG_MMU)	:= fremap.o highmem.o 
 obj-y			:= bootmem.o filemap.o mempool.o oom_kill.o fadvise.o \
 			   page_alloc.o page-writeback.o pdflush.o \
 			   readahead.o slab.o swap.o truncate.o vmscan.o \
-			   prio_tree.o $(mmu-y)
+			   prio_tree.o cachemap.o $(mmu-y)
 
 obj-$(CONFIG_SWAP)	+= page_io.o swap_state.o swapfile.o thrash.o
 obj-$(CONFIG_HUGETLBFS)	+= hugetlb.o
#4 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 7 lines

: x86-64 will need updating to also take advantage of this.
It may be able to just copy the i386 includes as-is, I've not looked closely at the PAT related changes on x86-64 yet. Andi?

: The list manipulation macros in mm/cachemap.c are a little fugly.

Anything else ?

For memory (pfn_valid == 1) it would be more memory efficient to use a few bits in struct page->flags

In general because there are lots of uses of "range lists" it would be better to put it as a library into lib.

Coding style needs some work.

Too many printks.

I am not sure yet the cmaps don't need reference counting. For some cases (user space support) they probably will.

Need user space support, e.g. using the existing ioctls for /proc/bus/pci/* (they are currently not implemented on i386/x86-64 but should with this) Then someone would need to change the X server to use this.

Needs testing on a lot of systems.

Need to figure out if CMAP_ALLOW_OVERLAPPING should be set or not.

Probably need to go over the combining rule etc. with a fine comb

-Andi

#5 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 10 lines · Andi Kleen
Quoted 7 lines

: x86-64 will need updating to also take advantage of this.
It may be able to just copy the i386 includes as-is, I've not looked closely at the PAT related changes on x86-64 yet. Andi?

: The list manipulation macros in mm/cachemap.c are a little fugly.

Anything else ?

For memory (pfn_valid == 1) it would be more memory efficient to use a few bits in struct page->flags

I think page->flags use should be limited to things that are relatively performance-sensitive and arch-independent, mostly because we're running out of them on 32-bit platforms, fast.

Each incremental use of page flags doesn't have any immediate storage cost, but it's a serious pain when we run out, and having to bloat it to a 64-bit value on 32-bit platforms wouldn't be very memory efficient, either. :)

Quoted 2 lines

In general because there are lots of uses of "range lists" it would be better to put it as a library into lib.

Either that, or something like "Dynamically allocated pageflags" would be nice.

	http://lwn.net/Articles/124332/

-- Dave

#6 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 7 lines · Dave Hansen
Quoted 6 lines · Andi Kleen
Quoted 6 lines

: x86-64 will need updating to also take advantage of this.
It may be able to just copy the i386 includes as-is, I've not looked closely at the PAT related changes on x86-64 yet. Andi?

: The list manipulation macros in mm/cachemap.c are a little fugly.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

Anything else ?

For memory (pfn_valid == 1) it would be more memory efficient to use a few bits in struct page->flags

I think page->flags use should be limited to things that are relatively performance-sensitive and arch-independent, mostly because we're running out of them on 32-bit platforms, fast.

That is mostly because we encode the zone number in there, which never seemed like a particularly useful optimization to me (because zone is not that expensive to recompute using a small hash table) If the zone number was removed there would be plenty of bits.

There are 12 bits left then, which is plenty.

In fact one bit is already reserved for PAT like this (PG_uncached), PAT would probably only need another one so that write combining can be expressed too. That could be even done without getting rid of the zone number.

Quoted 5 lines

Each incremental use of page flags doesn't have any immediate storage cost, but it's a serious pain when we run out, and having to bloat it to a 64-bit value on 32-bit platforms wouldn't be very memory efficient, either. :)

Quoted 7 lines
Quoted 2 lines

In general because there are lots of uses of "range lists" it would be better to put it as a library into lib.

Either that, or something like "Dynamically allocated pageflags" would be nice.

	http://lwn.net/Articles/124332/

Looks quite ugly and overkill.

-Andi

#7 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

In reply to Andi Kleen

Hi Andi,

Quoted 2 lines · Andi Kleen

For memory (pfn_valid == 1) it would be more memory efficient to use a few bits in struct page->flags

Maybe.

Quoted 2 lines

In general because there are lots of uses of "range lists" it would be better to put it as a library into lib.

Ditto.

Quoted 1 line

Coding style needs some work.

Yep.

Quoted 1 line

Too many printks.

At least whilst this is getting polished, its worth keeping these around. Once its stable, I agree, they can go (or at least be
demoted to dprintk's). It seems that theres some problems with
the current code, so they're definitly useful..

for eg..

CMAP: cmap_request_range: 0xf8000000 - 0xf8100fff (1)
CMAP:     cachings mismatch (4 != 1)
CMAP: cmap_request_range: 0xf8101000 - 0xf8101fff (1)
CMAP:     cachings mismatch (4 != 1)
CMAP: cmap_request_range: 0xf8102000 - 0xf8301fff (1)
CMAP:     cachings mismatch (4 != 1)
[drm:radeon_do_init_cp] *ERROR* could not find ioremap agp regions!
CMAP: cmap_release_range: 0xff8f0000 - 0xff96efff
CMAP:    last user, freeing
CMAP:    last user, freeing
CMAP:    release_range successful!!

I'm not sure where that's coming from yet. There's also a few failures to release regions which need to be double checked.

Quoted 2 lines

I am not sure yet the cmaps don't need reference counting. For some cases (user space support) they probably will.

Asides from cmap_entry->count ?
Hmm, there doesn't seem to be anything guarding concurrent accesses to that.

Quoted 3 lines

Need user space support, e.g. using the existing ioctls for /proc/bus/pci/* (they are currently not implemented on i386/x86-64 but should with this) Then someone would need to change the X server to use this.

By hooking into ioremap(), we're getting this done automatically.

01:00.0 VGA compatible controller: ATI Technologies Inc Radeon RV200 QW [Radeon 7500] (prog-if 00 [VGA])
Subsystem: PC Partner Limited RV200 QW [Radeon 7500 LE]
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping+ SERR+ FastB2B-
Status: Cap+ 66Mhz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 32 (2000ns min), Cache Line Size 08
Interrupt: pin A routed to IRQ 225
Region 0: Memory at e8000000 (32-bit, prefetchable) [size=128M]
Region 1: I/O ports at c800 [size=256]
Region 2: Memory at ff8f0000 (32-bit, non-prefetchable) [size=64K]
Expansion ROM at ff8c0000 [disabled] [size=128K]
Capabilities: <available only to root>

(10:39:17:davej@dhcp83-2:~)$ grep e8000000 /proc/cachemap
 0xe8000000 - 0xebffffff: 0x0004 1

Though I agree a userspace interface could be useful.

Quoted 1 line

Need to figure out if CMAP_ALLOW_OVERLAPPING should be set or not.

*nod*, and if it should, lose the ifdef completely.

Quoted 1 line

Probably need to go over the combining rule etc. with a fine comb

agreed. There's a bunch of errata on older CPUs that should probably be checked too.

Thanks for the comments. I'm not working on this full-time, but I'll continue to poke at it occasionally, especially if theres interest from anyone else.

Dave

#8 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

sorry I fell behind on this and never got back to it. I just got back from vacation and am swamped with stacked-up work, but I'll try to get back involved with this by this weekend.

I have no doubts that this isn't in ready condition yet. at the last time I was working on this, I remember I was comparing how it behaved on various systems at my disposal and there were some glaring problems I was trying to take notes on. I think they were cachemap api problems, but I don't recall the details. I'll try to review the current code and old email to remember.

right now I think there were a lot of excessive printouts for debugging purposes. I also have no doubts that there are coding style differences that need to be cleaned up (feel free to tell me when my code sucks or isn't up to style).

on a side note, last I was working on this I still had to keep an extra tree around and manually diff things, which is a burden and easy to goof things up. is there an easier way to do this? it looks like you guys are experimenting with git, is there an faq on how to get started with that?

Thanks,
Terence

Quoted 86 lines · On Fri, May 13, 2005 at 11:52:41AM -0400, [email protected] wrote:

Hi Andi,

Quoted 2 lines · Andi Kleen

For memory (pfn_valid == 1) it would be more memory efficient to use a few bits in struct page->flags

Maybe.

Quoted 2 lines

In general because there are lots of uses of "range lists" it would be better to put it as a library into lib.

Ditto.

Quoted 1 line

Coding style needs some work.

Yep.

Quoted 1 line

Too many printks.

At least whilst this is getting polished, its worth keeping these around. Once its stable, I agree, they can go (or at least be
demoted to dprintk's). It seems that theres some problems with
the current code, so they're definitly useful..

for eg..

CMAP: cmap_request_range: 0xf8000000 - 0xf8100fff (1)
CMAP:     cachings mismatch (4 != 1)
CMAP: cmap_request_range: 0xf8101000 - 0xf8101fff (1)
CMAP:     cachings mismatch (4 != 1)
CMAP: cmap_request_range: 0xf8102000 - 0xf8301fff (1)
CMAP:     cachings mismatch (4 != 1)
[drm:radeon_do_init_cp] *ERROR* could not find ioremap agp regions!
CMAP: cmap_release_range: 0xff8f0000 - 0xff96efff
CMAP:    last user, freeing
CMAP:    last user, freeing
CMAP:    release_range successful!!

I'm not sure where that's coming from yet. There's also a few failures to release regions which need to be double checked.

Quoted 2 lines

I am not sure yet the cmaps don't need reference counting. For some cases (user space support) they probably will.

Asides from cmap_entry->count ?
Hmm, there doesn't seem to be anything guarding concurrent accesses to that.

Quoted 3 lines

Need user space support, e.g. using the existing ioctls for /proc/bus/pci/* (they are currently not implemented on i386/x86-64 but should with this) Then someone would need to change the X server to use this.

By hooking into ioremap(), we're getting this done automatically.

01:00.0 VGA compatible controller: ATI Technologies Inc Radeon RV200 QW [Radeon 7500] (prog-if 00 [VGA])
Subsystem: PC Partner Limited RV200 QW [Radeon 7500 LE]
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping+ SERR+ FastB2B-
Status: Cap+ 66Mhz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR-
Latency: 32 (2000ns min), Cache Line Size 08
Interrupt: pin A routed to IRQ 225
Region 0: Memory at e8000000 (32-bit, prefetchable) [size=128M]
Region 1: I/O ports at c800 [size=256]
Region 2: Memory at ff8f0000 (32-bit, non-prefetchable) [size=64K]
Expansion ROM at ff8c0000 [disabled] [size=128K]
Capabilities: <available only to root>

(10:39:17:davej@dhcp83-2:~)$ grep e8000000 /proc/cachemap
 0xe8000000 - 0xebffffff: 0x0004 1

Though I agree a userspace interface could be useful.

Quoted 1 line

Need to figure out if CMAP_ALLOW_OVERLAPPING should be set or not.

*nod*, and if it should, lose the ifdef completely.

Quoted 1 line

Probably need to go over the combining rule etc. with a fine comb

agreed. There's a bunch of errata on older CPUs that should probably be checked too.

Thanks for the comments. I'm not working on this full-time, but I'll continue to poke at it occasionally, especially if theres interest from anyone else.

Dave

#9 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 6 lines

I have no doubts that this isn't in ready condition yet. at the last time I was working on this, I remember I was comparing how it behaved on various systems at my disposal and there were some glaring problems I was trying to take notes on. I think they were cachemap api problems, but I don't recall the details. I'll try to review the current code and old email to remember.

I plan to do more work in this area in the future too.

Quoted 5 lines

right now I think there were a lot of excessive printouts for debugging purposes. I also have no doubts that there are coding style differences that need to be cleaned up (feel free to tell me when my code sucks or isn't up to style).

Perhaps should concentrate on the basic design first.

Quoted 6 lines

on a side note, last I was working on this I still had to keep an extra tree around and manually diff things, which is a burden and easy to goof things up. is there an easier way to do this? it looks like you guys are experimenting with git, is there an faq on how to get started with that?

I use quilt (http://quilt.sourceforge.net) for keeping patchkits. Works well. It is not a real SCM, but can be combined with one.

-Andi

#10 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 6 lines · On Thu, May 19, 2005 at 12:03:56AM +0200, [email protected] wrote:
Quoted 4 lines

right now I think there were a lot of excessive printouts for debugging purposes. I also have no doubts that there are coding style differences that need to be cleaned up (feel free to tell me when my code sucks or isn't up to style).

Perhaps should concentrate on the basic design first.

sure, feel free to tell me if that sucks as well :)

Quoted 8 lines
Quoted 5 lines

on a side note, last I was working on this I still had to keep an extra tree around and manually diff things, which is a burden and easy to goof things up. is there an easier way to do this? it looks like you guys are experimenting with git, is there an faq on how to get started with that?

I use quilt (http://quilt.sourceforge.net) for keeping patchkits. Works well. It is not a real SCM, but can be combined with one.

thanks, I'll take a look at that.

Thanks,
Terence

#11 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 9 lines · Terence Ripperda
Quoted 6 lines · On Thu, May 19, 2005 at 12:03:56AM +0200, [email protected] wrote:
Quoted 4 lines

right now I think there were a lot of excessive printouts for debugging purposes. I also have no doubts that there are coding style differences that need to be cleaned up (feel free to tell me when my code sucks or isn't up to style).

Perhaps should concentrate on the basic design first.

sure, feel free to tell me if that sucks as well :)

The main thing I am not sure about in the basic design was the overlapping handling. I suspect that needs to be discussed again, perhaps even with input from some CPU vendor people.

My feeling is that overlapping is not good and should be avoided.

Then we need a user interface for the X server too, which also needs to be designed. /proc/bus/pci/*/* has some ioctls for this on other platforms that might be adapted (common interface would be best of course), but I am
not sure the code is powerful enough for everything PAT offers. Probably needs some enhancements (do we really want to support the obscure PAT settings like write protect though?)

Do we need to handle the fixed range legacy MTRRs too?

It would be better to move the "range list using rbtree" code somewhere into lib. It certainly seems to be needed often (I did my own version for the NUMA policies ;-) and it doesn't make
much sense to duplicate all the time. iirc there is at least another patch pending with a similar datastructure.

I suppose you saw the list I sent to Dave. These are the things that came to mind while reading it. I haven't gone over everything in detail though.

I hope to have some time soon to go over all of this more closely.

-Andi

#12 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

In reply to Andi Kleen
Quoted 19 lines · Andi Kleen
Quoted 6 lines

I have no doubts that this isn't in ready condition yet. at the last time I was working on this, I remember I was comparing how it behaved on various systems at my disposal and there were some glaring problems I was trying to take notes on. I think they were cachemap api problems, but I don't recall the details. I'll try to review the current code and old email to remember.

I plan to do more work in this area in the future too.

Quoted 6 lines

right now I think there were a lot of excessive printouts for debugging purposes. I also have no doubts that there are coding style differences that need to be cleaned up (feel free to tell me when my code
sucks or isn't up to style).

Perhaps should concentrate on the basic design first.

Quoted 1 line
Quoted 8 lines
Quoted 5 lines

on a side note, last I was working on this I still had to keep an extra tree around and manually diff things, which is a burden and easy to goof things up. is there an easier way to do this? it looks like you guys are experimenting with git, is there an faq on how to get started with that?

I use quilt (http://quilt.sourceforge.net) for keeping patchkits. Works well. It is not a real SCM, but can be combined with one.

make that
http://savannah.nongnu.org/projects/quilt

the sf.net 'quilt' says:
Quilt is a Java software development tool that measures coverage....

Signature · 1 line
~Randy
#13 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

In reply to Dave Jones

arch/i386/kernel/cpu/common.c static int __init x86_serial_nr_setup(ch · line 353

Quoted 12 lines · Dave Jones
arch/i386/kernel/cpu/common.c +12 -0
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_AMD:
+		wrmsr(IA32_CR_PAT, AMD_PAT_31_0, AMD_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	case X86_VENDOR_INTEL:
+		wrmsr(IA32_CR_PAT, INTEL_PAT_31_0, INTEL_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	default:
+		printk("Unknown vendor in setup_pat()\n");
+	}

Drop the vendor check; PAT is a generic x86 feature. If AMD is not compatible (see below), then use X86_VENDOR_AMD: and default:.

arch/i386/kernel/cpu/common.c void __devinit cpu_uninit(void) · line 715

Quoted 3 lines
arch/i386/kernel/cpu/common.c +3 -0
+
+	/* checks copied from arch/i386/kernel/cpu/mtrr/main.c */
+	/* do these only apply to mtrrs or pat as well? */

It would apply to both; the chipset wouldn't even know how it got invoked.

Quoted 9 lines

+/* Here is the PAT's default layout on ia32 cpus when we are done.
+ * PAT0: Write Back
+ * PAT1: Write Combine
+ * PAT2: Uncached
+ * PAT3: Uncacheable
+ * PAT4: Write Through
+ * PAT5: Write Protect
+ * PAT6: Uncached
+ * PAT7: Uncacheable

Bad move. Some (Intel) processors drop the top bit, so it's much better to pick the protection methods one cares about (usually WB, WC, UC) and stick them in the first four, then duplicate the whole thing in the second half.

Unless you actually expect someone to use WT or WP, no need to tickle this particular bug.

Quoted 2 lines

+ * Note: On Athlon cpus PAT2/PAT3 & PAT6/PAT7 are both Uncacheable since
+ * there is no uncached type.

If one sets the PAT to "uncached", does one get the same function as "uncachable"?

-hpa

#14 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 17 lines · H. Peter Anvin

arch/i386/kernel/cpu/common.c static int __init x86_serial_nr_setup(ch · line 353

Quoted 12 lines · Dave Jones
arch/i386/kernel/cpu/common.c +12 -0
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_AMD:
+		wrmsr(IA32_CR_PAT, AMD_PAT_31_0, AMD_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	case X86_VENDOR_INTEL:
+		wrmsr(IA32_CR_PAT, INTEL_PAT_31_0, INTEL_PAT_63_32);
+		atomic_inc(&pat_cpus_enabled);
+		break;
+	default:
+		printk("Unknown vendor in setup_pat()\n");
+	}

Drop the vendor check; PAT is a generic x86 feature. If AMD is not compatible (see below), then use X86_VENDOR_AMD: and default:.

Done. Does transmeta have PAT btw ? I know newer VIA has it, but I haven't looked through the docs to double check its implementation yet.

Quoted 4 lines

arch/i386/kernel/cpu/common.c void __devinit cpu_uninit(void) · line 715

Quoted 2 lines
arch/i386/kernel/cpu/common.c +2 -0
+	/* checks copied from arch/i386/kernel/cpu/mtrr/main.c */
+	/* do these only apply to mtrrs or pat as well? */

It would apply to both; the chipset wouldn't even know how it got invoked.

ACK, Comment dropped.

Quoted 10 lines
Quoted 9 lines

+/* Here is the PAT's default layout on ia32 cpus when we are done.
+ * PAT0: Write Back
+ * PAT1: Write Combine
+ * PAT2: Uncached
+ * PAT3: Uncacheable
+ * PAT4: Write Through
+ * PAT5: Write Protect
+ * PAT6: Uncached
+ * PAT7: Uncacheable

Quoted 4 lines

Bad move. Some (Intel) processors drop the top bit, so it's much better to pick the protection methods one cares about (usually WB, WC, UC) and stick them in the first four, then duplicate the whole thing in the second half.

Noted.

Quoted 4 lines
Quoted 2 lines

+ * Note: On Athlon cpus PAT2/PAT3 & PAT6/PAT7 are both Uncacheable since
+ * there is no uncached type.

If one sets the PAT to "uncached", does one get the same function as "uncachable"?

AIUI, only as long as we don't have an MTRR covering the same range marked WC. It seems to be the only thing I could find documenting the differences between 'uncached' and 'uncacheable' in this context.
Though I've only looked through the Intel & AMD K8 docs, I don't have the K7 ones to hand.

Dave

#15 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 8 lines · Dave Jones
Quoted 3 lines

Drop the vendor check; PAT is a generic x86 feature. If AMD is not compatible (see below), then use X86_VENDOR_AMD: and default:.

Done. Does transmeta have PAT btw ? I know newer VIA has it, but I haven't looked through the docs to double check its implementation yet.

The Efficeon (TM8xxx) series does have PAT.

Quoted 12 lines
Quoted 5 lines
Quoted 3 lines

+ * Note: On Athlon cpus PAT2/PAT3 & PAT6/PAT7 are both Uncacheable since
+ * there is no uncached type.

If one sets the PAT to "uncached", does one get the same function as "uncachable"?

AIUI, only as long as we don't have an MTRR covering the same range marked WC. It seems to be the only thing I could find documenting the differences between 'uncached' and 'uncacheable' in this context.
Though I've only looked through the Intel & AMD K8 docs, I don't have the K7 ones to hand.

I mean, on the Athlon series, is it really necessary to use a different value?

-hpa

#16 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 1 line · H. Peter Anvin

The Efficeon (TM8xxx) series does have PAT.

1:1 with the Intel implementation I assume based on your earlier comments?

Quoted 13 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line

+ * Note: On Athlon cpus PAT2/PAT3 & PAT6/PAT7 are both Uncacheable

since > >+ * there is no uncached type.

Quoted 2 lines

If one sets the PAT to "uncached", does one get the same function as "uncachable"?

Quoted 7 lines

AIUI, only as long as we don't have an MTRR covering the same range marked WC.
It seems to be the only thing I could find documenting the differences between 'uncached' and 'uncacheable' in this context.
Though I've only looked through the Intel & AMD K8 docs, I don't have the K7 ones to hand.

Quoted 2 lines

I mean, on the Athlon series, is it really necessary to use a different value?

I'd have to dig out the docs and check.

Dave

#17 lore raw

Re: [RFC] Cachemap for 2.6.12rc4-mm1. Was Re: [PATCH] enhance x86 MTRR handling

Quoted 5 lines · Dave Jones
Quoted 1 line · H. Peter Anvin

The Efficeon (TM8xxx) series does have PAT.

1:1 with the Intel implementation I assume based on your earlier comments?

More or less. They don't implement WT and WP (defaulting to WB with full cache coherency and something else).

-hpa