Skip to contentSkip to conversation

x86, mtrr: fixs for mtrr_cleanup

PATCH In review Yinghai Lu 4 participants 10 messages Jun 13, 2013 – Jun 18, 2013

Subsystems (2)
lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

v3 cover x86, mtrr: fixs for mtrr_cleanup

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

*BAD*gran_size: 64K chunk_size: 16M num_reg: 6 lose cover RAM: -0G

https://bugzilla.kernel.org/show_bug.cgi?id=59491

Fix it from two places, so late mtrr cleanup side and range add side.

x86, mtrr: Fix original mtrr range get for mtrr_cleanup
range: Do not add new blank slot with add_range_with_merge

Please apply them for v3.10.

Thanks

Yinghai

author
#1 lore raw

PATCH v3 1/2 x86, mtrr: Fix original mtrr range get for mtrr_cleanup

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

*BAD*gran_size: 64K chunk_size: 16M num_reg: 6 lose cover RAM: -0G

https://bugzilla.kernel.org/show_bug.cgi?id=59491

So it rejects new var mtrr layout.

It turns out we have some problem with initial mtrr range retrievel. current sequence is:

	x86_get_mtrr_mem_range
		==> bunchs of add_range_with_merge
		==> bunchs of subract_range
		==> clean_sort_range
	add_range_with_merge for [0,1M)
	sort_range()

add_range_with_merge could have blank slots, so we can not just sort only, that will have final result have extra blank slot in head.

So move that calling add_range_with_merge for [0,1M), with that we could avoid extra clean_sort_range calling.

---
 arch/x86/kernel/cpu/mtrr/cleanup.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/mtrr/cleanup.c +4 -4
Index: linux-2.6/arch/x86/kernel/cpu/mtrr/cleanup.c
===================================================================
@@ -714,15 +714,15 @@ int __init mtrr_cleanup(unsigned address
 	if (mtrr_tom2)
 		x_remove_size = (mtrr_tom2 >> PAGE_SHIFT) - x_remove_base;
 
-	nr_range = x86_get_mtrr_mem_range(range, 0, x_remove_base, x_remove_size);
 	/*
 	 * [0, 1M) should always be covered by var mtrr with WB
 	 * and fixed mtrrs should take effect before var mtrr for it:
 	 */
-	nr_range = add_range_with_merge(range, RANGE_NUM, nr_range, 0,
+	nr_range = add_range_with_merge(range, RANGE_NUM, 0, 0,
 					1ULL<<(20 - PAGE_SHIFT));
-	/* Sort the ranges: */
-	sort_range(range, nr_range);
+	/* add from var mtrr at last */
+	nr_range = x86_get_mtrr_mem_range(range, nr_range,
+					  x_remove_base, x_remove_size);
 
 	range_sums = sum_ranges(range, nr_range);
 	printk(KERN_INFO "total RAM covered: %ldM\n",
#2 lore raw

Re: [PATCH v3 1/2] x86, mtrr: Fix original mtrr range get for mtrr_cleanup

Quoted 22 lines · Yinghai Lu

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

*BAD*gran_size: 64K chunk_size: 16M num_reg: 6 lose cover RAM: -0G

https://bugzilla.kernel.org/show_bug.cgi?id=59491

So it rejects new var mtrr layout.

It turns out we have some problem with initial mtrr range retrievel. current sequence is:

        x86_get_mtrr_mem_range
                ==> bunchs of add_range_with_merge
                ==> bunchs of subract_range
                ==> clean_sort_range
        add_range_with_merge for [0,1M)
        sort_range()

add_range_with_merge could have blank slots, so we can not just sort only, that will have final result have extra blank slot in head.

Quoted 7 lines

So move that calling add_range_with_merge for [0,1M), with that we could avoid extra clean_sort_range calling.

We're at -rc6 now and this and it's companion aren't in Linus' tree, which means the broken 3.9.y stable series doesn't have them either. It's been posted for a while now, and v3 was really a minor cleanup. Is there some reason this is still not merged? We've been carrying v2 in Fedora for a bit now and it's cleared up the issue for a number of people.

josh

#3 lore raw

[tip:x86/urgent] x86, mtrr: Fix original mtrr range get for mtrr_cleanup

In reply to Yinghai Lu
x86, mtrr: Fix original mtrr range get for mtrr_cleanup
Commit-ID:  d8d386c10630d8f7837700f4c466443d49e12cc0
Gitweb:     http://git.kernel.org/tip/d8d386c10630d8f7837700f4c466443d49e12cc0
Author:     Yinghai Lu <[email protected]>
AuthorDate: Thu, 13 Jun 2013 13:17:01 -0700
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Tue, 18 Jun 2013 11:32:02 -0500

x86, mtrr: Fix original mtrr range get for mtrr_cleanup

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

*BAD*gran_size: 64K chunk_size: 16M num_reg: 6 lose cover RAM: -0G

https://bugzilla.kernel.org/show_bug.cgi?id=59491

So it rejects new var mtrr layout.

It turns out we have some problem with initial mtrr range retrieval. The current sequence is:

	x86_get_mtrr_mem_range
		==> bunchs of add_range_with_merge
		==> bunchs of subract_range
		==> clean_sort_range
	add_range_with_merge for [0,1M)
	sort_range()

add_range_with_merge could have blank slots, so we can not just sort only, that will have final result have extra blank slot in head.

So move that calling add_range_with_merge for [0,1M), with that we could avoid extra clean_sort_range calling.

---
 arch/x86/kernel/cpu/mtrr/cleanup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/mtrr/cleanup.c +4 -4
index 35ffda5..5f90b85 100644
@@ -714,15 +714,15 @@ int __init mtrr_cleanup(unsigned address_bits)
 	if (mtrr_tom2)
 		x_remove_size = (mtrr_tom2 >> PAGE_SHIFT) - x_remove_base;
 
-	nr_range = x86_get_mtrr_mem_range(range, 0, x_remove_base, x_remove_size);
 	/*
 	 * [0, 1M) should always be covered by var mtrr with WB
 	 * and fixed mtrrs should take effect before var mtrr for it:
 	 */
-	nr_range = add_range_with_merge(range, RANGE_NUM, nr_range, 0,
+	nr_range = add_range_with_merge(range, RANGE_NUM, 0, 0,
 					1ULL<<(20 - PAGE_SHIFT));
-	/* Sort the ranges: */
-	sort_range(range, nr_range);
+	/* add from var mtrr at last */
+	nr_range = x86_get_mtrr_mem_range(range, nr_range,
+					  x_remove_base, x_remove_size);
 
 	range_sums = sum_ranges(range, nr_range);
 	printk(KERN_INFO "total RAM covered: %ldM\n",
author
#4 lore raw

PATCH v3 2/2 range: Do not add new blank slot with add_range_with_merge

In reply to Yinghai Lu

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

The reason is add_range_with_merge could generate blank spot.

We could avoid that by searching new expanded start/end, that new range should include all connected ranges in range array. At last add the new expanded start/end to the range array. Also move up left array so do not add new blank slot in the range array.

-v2: move left array to avoid enhance add_range() -v3: include fix from Joshua about memmove declaring when
DYN_DEBUG is used.

---
 kernel/range.c |   23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
kernel/range.c +12 -11
Index: linux-2.6/kernel/range.c
===================================================================
@@ -4,7 +4,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/sort.h>
-
+#include <linux/string.h>
 #include <linux/range.h>
 
 int add_range(struct range *range, int az, int nr_range, u64 start, u64 end)
@@ -32,9 +32,8 @@ int add_range_with_merge(struct range *r
 	if (start >= end)
 		return nr_range;
 
-	/* Try to merge it with old one: */
+	/* get new start/end: */
 	for (i = 0; i < nr_range; i++) {
-		u64 final_start, final_end;
 		u64 common_start, common_end;
 
 		if (!range[i].end)
@@ -45,14 +44,16 @@ int add_range_with_merge(struct range *r
 		if (common_start > common_end)
 			continue;
 
-		final_start = min(range[i].start, start);
-		final_end = max(range[i].end, end);
-
-		/* clear it and add it back for further merge */
-		range[i].start = 0;
-		range[i].end =  0;
-		return add_range_with_merge(range, az, nr_range,
-			final_start, final_end);
+		/* new start/end, will add it back at last */
+		start = min(range[i].start, start);
+		end = max(range[i].end, end);
+
+		memmove(&range[i], &range[i + 1],
+			(nr_range - (i + 1)) * sizeof(range[i]));
+		range[nr_range - 1].start = 0;
+		range[nr_range - 1].end   = 0;
+		nr_range--;
+		i--;
 	}
 
 	/* Need to add it: */
#5 lore raw

[tip:x86/urgent] range: Do not add new blank slot with add_range_with_merge

range: Do not add new blank slot with add_range_with_merge
Commit-ID:  0541881502a1276149889fe468662ff6a8fc8f6d
Gitweb:     http://git.kernel.org/tip/0541881502a1276149889fe468662ff6a8fc8f6d
Author:     Yinghai Lu <[email protected]>
AuthorDate: Thu, 13 Jun 2013 13:17:02 -0700
Committer:  H. Peter Anvin <[email protected]>
CommitDate: Tue, 18 Jun 2013 11:32:10 -0500

range: Do not add new blank slot with add_range_with_merge

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

The reason is add_range_with_merge could generate blank spot.

We could avoid that by searching new expanded start/end, that new range should include all connected ranges in range array. At last add the new expanded start/end to the range array. Also move up left array so do not add new blank slot in the range array.

-v2: move left array to avoid enhance add_range() -v3: include fix from Joshua about memmove declaring when
DYN_DEBUG is used.

---
 kernel/range.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
kernel/range.c +11 -10
index eb911db..322ea8e 100644
@@ -4,7 +4,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/sort.h>
-
+#include <linux/string.h>
 #include <linux/range.h>
 
 int add_range(struct range *range, int az, int nr_range, u64 start, u64 end)
@@ -32,9 +32,8 @@ int add_range_with_merge(struct range *range, int az, int nr_range,
 	if (start >= end)
 		return nr_range;
 
-	/* Try to merge it with old one: */
+	/* get new start/end: */
 	for (i = 0; i < nr_range; i++) {
-		u64 final_start, final_end;
 		u64 common_start, common_end;
 
 		if (!range[i].end)
@@ -45,14 +44,16 @@ int add_range_with_merge(struct range *range, int az, int nr_range,
 		if (common_start > common_end)
 			continue;
 
-		final_start = min(range[i].start, start);
-		final_end = max(range[i].end, end);
+		/* new start/end, will add it back at last */
+		start = min(range[i].start, start);
+		end = max(range[i].end, end);
 
-		/* clear it and add it back for further merge */
-		range[i].start = 0;
-		range[i].end =  0;
-		return add_range_with_merge(range, az, nr_range,
-			final_start, final_end);
+		memmove(&range[i], &range[i + 1],
+			(nr_range - (i + 1)) * sizeof(range[i]));
+		range[nr_range - 1].start = 0;
+		range[nr_range - 1].end   = 0;
+		nr_range--;
+		i--;
 	}
 
 	/* Need to add it: */
#6 lore raw
In reply to Yinghai Lu
Quoted 6 lines · Yinghai Lu

Joshua reported: Commit cd7b304dfaf1 (x86, range: fix missing merge during add range) broke mtrr cleanup on his setup in 3.9.5. corresponding commit in upstream is fbe06b7bae7c.

*BAD*gran_size: 64K chunk_size: 16M num_reg: 6 lose cover RAM: -0G

Quoted 9 lines

https://bugzilla.kernel.org/show_bug.cgi?id=59491

Fix it from two places, so late mtrr cleanup side and range add side.

x86, mtrr: Fix original mtrr range get for mtrr_cleanup
range: Do not add new blank slot with add_range_with_merge

Please apply them for v3.10.

On this subject: what do we need to happen to be able to dump the MTRR cleanup stuff?

-hpa

author
#7 lore raw
Quoted 3 lines · H. Peter Anvin

On this subject: what do we need to happen to be able to dump the MTRR cleanup stuff?

the old x server like to add Write-combining entry in MTRR. looks like drm for mga200 is adding that too ?

We need to make them to use PAT instead.

Yinghai

#8 lore raw
Quoted 8 lines · Yinghai Lu
Quoted 3 lines · H. Peter Anvin

On this subject: what do we need to happen to be able to dump the MTRR cleanup stuff?

the old x server like to add Write-combining entry in MTRR. looks like drm for mga200 is adding that too ?

"Old X server" here has been a long time now. I need to dig into the MGA200 I guess...

Quoted 1 line

We need to make them to use PAT instead.

Yes.

-hpa

author
#9 lore raw
Quoted 6 lines · H. Peter Anvin
Quoted 5 lines · Yinghai Lu
Quoted 3 lines · H. Peter Anvin

On this subject: what do we need to happen to be able to dump the MTRR cleanup stuff?

Quoted 8 lines
Quoted 3 lines

the old x server like to add Write-combining entry in MTRR. looks like drm for mga200 is adding that too ?

"Old X server" here has been a long time now. I need to dig into the MGA200 I guess...

Quoted 1 line

We need to make them to use PAT instead.

looks that we have more mtrr_add calling:

drivers/gpu/drm/ast/ast_ttm.c: ast->fb_mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 0),
drivers/gpu/drm/cirrus/cirrus_ttm.c: cirrus->fb_mtrr =
drm_mtrr_add(pci_resource_start(dev->pdev, 0),
drivers/gpu/drm/drm_bufs.c: map->mtrr =
mtrr_add(map->offset, map->size, drivers/gpu/drm/drm_pci.c:
mtrr_add(dev->agp->agp_info.aper_base,
drivers/gpu/drm/i915/i915_dma.c: dev_priv->mm.gtt_mtrr =
mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1); drivers/gpu/drm/mgag200/mgag200_ttm.c: mdev->fb_mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 0), drivers/gpu/drm/nouveau/nouveau_ttm.c: drm->ttm.mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 1),
drivers/gpu/drm/radeon/radeon_object.c: rdev->mc.vram_mtrr =
mtrr_add(rdev->mc.aper_base, rdev->mc.aper_size, drivers/gpu/drm/savage/savage_bci.c:
drm_mtrr_add(dev_priv->mtrr[0].base, drivers/gpu/drm/savage/savage_bci.c:
drm_mtrr_add(dev_priv->mtrr[1].base, drivers/gpu/drm/savage/savage_bci.c:
drm_mtrr_add(dev_priv->mtrr[2].base, drivers/gpu/drm/savage/savage_bci.c:
drm_mtrr_add(dev_priv->mtrr[0].base,
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c: dev_priv->mmio_mtrr =
drm_mtrr_add(dev_priv->mmio_start,
drivers/infiniband/hw/ipath/ipath_wc_x86_64.c: cookie =
mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 0); drivers/infiniband/hw/ipath/ipath_wc_x86_64.c:
"mtrr_add() WC for PIO bufs "
drivers/infiniband/hw/qib/qib_wc_x86_64.c: cookie =
mtrr_add(pioaddr, piolen, MTRR_TYPE_WRCOMB, 0); drivers/infiniband/hw/qib/qib_wc_x86_64.c:
"mtrr_add() WC for PIO bufs failed (%d)\n",
drivers/media/pci/ivtv/ivtvfb.c: if
(mtrr_add(oi->fb_start_aligned_physaddr,
drivers/message/fusion/mptbase.c: ioc->mtrr_reg =
mtrr_add(ioc->req_frames_dma,
drivers/net/ethernet/myricom/myri10ge/myri10ge.c: mgp->mtrr =
mtrr_add(mgp->iomem_base, mgp->board_span,

will be in drm:

drivers/staging/xgifb/XGI_main_26.c: xgifb_info->mtrr =
mtrr_add(xgifb_info->video_base,

video:

drivers/video/arkfb.c: par->mtrr_reg =
mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);
drivers/video/aty/aty128fb.c: par->mtrr.vram =
mtrr_add(info->fix.smem_start,
drivers/video/aty/atyfb_base.c: par->mtrr_aper =
mtrr_add(par->res_start, par->res_size,
drivers/video/aty/atyfb_base.c: par->mtrr_reg =
mtrr_add(par->res_start + 0x800000 -
drivers/video/aty/radeon_base.c: rinfo->mtrr_hdl = nomtrr ? -1
: mtrr_add(rinfo->fb_base_phys,
drivers/video/gbefb.c: mtrr_add(gbe_mem_phys, gbe_mem_size, MTRR_TYPE_WRCOMB, 1);

drivers/video/i740fb.c:         par->mtrr_reg = mtrr_add(info->fix.smem_start,
drivers/video/i810/i810_main.h: par->mtrr_reg = mtrr_add((u32)

par->aperture.physical,
drivers/video/intelfb/intelfbdrv.c: dinfo->mtrr_reg =
mtrr_add(dinfo->aperture.physical,
drivers/video/kyro/fbdev.c: mtrr_add(kyro_fix.smem_start,
drivers/video/matrox/matroxfb_base.c: minfo->mtrr.vram =
mtrr_add(video_base_phys, minfo->video.len, MTRR_TYPE_WRCOMB, 1);
drivers/video/neofb.c: mtrr_add(info->fix.smem_start,
pci_resource_len(dev, 0),
drivers/video/nvidia/nvidia.c: par->mtrr.vram =
mtrr_add(nvidiafb_fix.smem_start,

drivers/video/pm2fb.c:                  mtrr_add(pm2fb_fix.smem_start,
drivers/video/pm3fb.c:          par->mtrr_handle =

mtrr_add(pm3fb_fix.smem_start,
drivers/video/riva/fbdev.c: default_par->mtrr.vram =
mtrr_add(rivafb_fix.smem_start,
drivers/video/s3fb.c: par->mtrr_reg =
mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);
drivers/video/savage/savagefb_driver.c: par->video.mtrr = mtrr_add(par->video.pbase, video_len,
drivers/video/sgivwfb.c: mtrr_add(sgivwfb_mem_phys,
sgivwfb_mem_size, MTRR_TYPE_WRCOMB, 1);
drivers/video/sis/sis_main.c: ivideo->mtrr =
mtrr_add(ivideo->video_base, ivideo->video_size, drivers/video/tdfxfb.c:static inline int mtrr_add(unsigned long base, unsigned long size,
drivers/video/tdfxfb.c: mtrr_add(info->fix.smem_start,
info->fix.smem_len,
drivers/video/uvesafb.c: rc =
mtrr_add(info->fix.smem_start,
drivers/video/vesafb.c: rc =
mtrr_add(vesafb_fix.smem_start, temp_size,
drivers/video/vt8623fb.c: par->mtrr_reg =
mtrr_add(info->fix.smem_start, info->fix.smem_len, MTRR_TYPE_WRCOMB, 1);