2013-06-13 20:17:23

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH v3 0/2] 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


2013-06-13 20:17:22

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH v3 2/2] 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.

Reported-by: Joshua Covington <[email protected]>
Tested-by: Joshua Covington <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
Cc: <[email protected]> v3.9

---
kernel/range.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)

Index: linux-2.6/kernel/range.c
===================================================================
--- linux-2.6.orig/kernel/range.c
+++ 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: */

2013-06-13 20:17:45

by Yinghai Lu

[permalink] [raw]
Subject: [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.

Reported-by: Joshua Covington <[email protected]>
Tested-by: Joshua Covington <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
Cc: <[email protected]> v3.9

---
arch/x86/kernel/cpu/mtrr/cleanup.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6/arch/x86/kernel/cpu/mtrr/cleanup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ 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",

2013-06-18 16:25:37

by Josh Boyer

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

On Thu, Jun 13, 2013 at 4:17 PM, Yinghai Lu <[email protected]> wrote:
> 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.
>
> Reported-by: Joshua Covington <[email protected]>
> Tested-by: Joshua Covington <[email protected]>
> Signed-off-by: Yinghai Lu <[email protected]>
> Cc: <[email protected]> v3.9

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

2013-06-18 16:30:38

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 0/2] x86, mtrr: fixs for mtrr_cleanup

On 06/13/2013 03:17 PM, Yinghai Lu wrote:
> 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.
>

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

-hpa

Subject: [tip:x86/urgent] 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.

Reported-by: Joshua Covington <[email protected]>
Tested-by: Joshua Covington <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Cc: <[email protected]> v3.9
Signed-off-by: H. Peter Anvin <[email protected]>
---
kernel/range.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/range.c b/kernel/range.c
index eb911db..322ea8e 100644
--- a/kernel/range.c
+++ b/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 *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: */

Subject: [tip:x86/urgent] 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.

Reported-by: Joshua Covington <[email protected]>
Tested-by: Joshua Covington <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Cc: <[email protected]> v3.9
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/kernel/cpu/mtrr/cleanup.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c b/arch/x86/kernel/cpu/mtrr/cleanup.c
index 35ffda5..5f90b85 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -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",

2013-06-18 17:28:25

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH v3 0/2] x86, mtrr: fixs for mtrr_cleanup

On Tue, Jun 18, 2013 at 9:29 AM, H. Peter Anvin <[email protected]> wrote:
>
> 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

2013-06-18 17:33:34

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 0/2] x86, mtrr: fixs for mtrr_cleanup

On 06/18/2013 12:28 PM, Yinghai Lu wrote:
> On Tue, Jun 18, 2013 at 9:29 AM, H. Peter Anvin <[email protected]> wrote:
>>
>> 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...

> We need to make them to use PAT instead.

Yes.

-hpa

2013-06-18 18:24:23

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH v3 0/2] x86, mtrr: fixs for mtrr_cleanup

On Tue, Jun 18, 2013 at 10:33 AM, H. Peter Anvin <[email protected]> wrote:
> On 06/18/2013 12:28 PM, Yinghai Lu wrote:
>> On Tue, Jun 18, 2013 at 9:29 AM, H. Peter Anvin <[email protected]> wrote:
>>>
>>> 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...
>
>> 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);