2009-04-18 08:44:21

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH 1/3] x86/pci: fix -1 calling to e820_all_mapped with mmconfig


e820_all_mapped need end is (addr + size) instead of (addr + size - 1)

[Impact: fix -1 offset calling]

Signed-off-by: Yinghai Lu <[email protected]>

---
arch/x86/pci/mmconfig-shared.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -375,7 +375,7 @@ static acpi_status __init check_mcfg_res
if (!fixmem32)
return AE_OK;
if ((mcfg_res->start >= fixmem32->address) &&
- (mcfg_res->end < (fixmem32->address +
+ (mcfg_res->end <= (fixmem32->address +
fixmem32->address_length))) {
mcfg_res->flags = 1;
return AE_CTRL_TERMINATE;
@@ -392,7 +392,7 @@ static acpi_status __init check_mcfg_res
return AE_OK;

if ((mcfg_res->start >= address.minimum) &&
- (mcfg_res->end < (address.minimum + address.address_length))) {
+ (mcfg_res->end <= (address.minimum + address.address_length))) {
mcfg_res->flags = 1;
return AE_CTRL_TERMINATE;
}
@@ -439,7 +439,7 @@ static int __init is_mmconf_reserved(che
u64 old_size = size;
int valid = 0;

- while (!is_reserved(addr, addr + size - 1, E820_RESERVED)) {
+ while (!is_reserved(addr, addr + size, E820_RESERVED)) {
size >>= 1;
if (size < (16UL<<20))
break;


2009-04-18 08:45:28

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH 2/3] pci: don't assume pref memio are 64bit -v2


one system with 4g installed ( there is 1g hole)

when 4G installed.
BIOS put ACPI etc need the hole
[ 0.000000] BIOS-provided physical RAM map:
[ 0.000000] BIOS-e820: 0000000000000000 - 000000000009bc00 (usable)
[ 0.000000] BIOS-e820: 000000000009bc00 - 00000000000a0000 (reserved)
[ 0.000000] BIOS-e820: 00000000000e3000 - 0000000000100000 (reserved)
[ 0.000000] BIOS-e820: 0000000000100000 - 00000000bffa0000 (usable)
[ 0.000000] BIOS-e820: 00000000bffa0000 - 00000000bffae000 (ACPI data)
[ 0.000000] BIOS-e820: 00000000bffae000 - 00000000bfff0000 (ACPI NVS)
[ 0.000000] BIOS-e820: 00000000bfff0000 - 00000000c0000000 (reserved)
[ 0.000000] BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
[ 0.000000] BIOS-e820: 00000000ffb00000 - 0000000100000000 (reserved)
[ 0.000000] BIOS-e820: 0000000100000000 - 0000000140000000 (usable)
so in kernel resource will be reserved for 0xbffa0000 - 0xbfff0000 for ACPI
0x100000 - 0xbffa0000 for RAM...

and BIOS set
[ 0.240007] pci 0000:00:01.0: bridge 64bit mmio pref: [0xbdf00000-0xddefffff]
[ 0.237102] pci 0000:01:00.0: reg 10 32bit mmio: [0xc0000000-0xcfffffff]
that is conflict with reserved res. so it can not be reserved Kernel.

then Kernel try to get range from 0x140000000 ( above the RAM, 5G and above 4g)
and set let the bridge to use it, and ATI cards to use it.

but the problem is that ATI only support 32bit ...

we should not assign 64bit range to pci device that only take 32bit pref

try to set PCI_PREF_RANGE_TYPE_64 in 64bit resource of pci_device (besides in pci_bridge),
and make the bus resource only have that bit set when all device under that do support
64bit pref mem
then use that flag to decide the max limit for find/request.

[Impact: do assign wrong range to device that doesn't support it]

v2: fix b_res->flags and logic and passing result.

Reported-and-tested-by: Yannick <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>

---
drivers/pci/bus.c | 8 +++++++-
drivers/pci/probe.c | 8 ++++++--
drivers/pci/setup-bus.c | 40 +++++++++++++++++++++++++++++++---------
3 files changed, 44 insertions(+), 12 deletions(-)

Index: linux-2.6/drivers/pci/bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/bus.c
+++ linux-2.6/drivers/pci/bus.c
@@ -41,9 +41,15 @@ pci_bus_alloc_resource(struct pci_bus *b
void *alignf_data)
{
int i, ret = -ENOMEM;
+ resource_size_t max = -1;

type_mask |= IORESOURCE_IO | IORESOURCE_MEM;

+ /* don't allocate too high if the pref mem doesn't support 64bit*/
+ if ((res->flags & (IORESOURCE_PREFETCH | PCI_PREF_RANGE_TYPE_64)) ==
+ IORESOURCE_PREFETCH)
+ max = 0xffffffff;
+
for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
struct resource *r = bus->resource[i];
if (!r)
@@ -62,7 +68,7 @@ pci_bus_alloc_resource(struct pci_bus *b
/* Ok, try it out.. */
ret = allocate_resource(r, res, size,
r->start ? : min,
- -1, align,
+ max, align,
alignf, alignf_data);
if (ret == 0)
break;
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -193,7 +193,7 @@ int __pci_read_base(struct pci_dev *dev,
res->flags |= pci_calc_resource_flags(l) | IORESOURCE_SIZEALIGN;
if (type == pci_bar_io) {
l &= PCI_BASE_ADDRESS_IO_MASK;
- mask = PCI_BASE_ADDRESS_IO_MASK & 0xffff;
+ mask = PCI_BASE_ADDRESS_IO_MASK & IO_SPACE_LIMIT;
} else {
l &= PCI_BASE_ADDRESS_MEM_MASK;
mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
@@ -237,6 +237,9 @@ int __pci_read_base(struct pci_dev *dev,
dev_printk(KERN_DEBUG, &dev->dev,
"reg %x 64bit mmio: %pR\n", pos, res);
}
+
+ if (res->flags & IORESOURCE_PREFETCH)
+ res->flags |= PCI_PREF_RANGE_TYPE_64;
} else {
sz = pci_size(l, sz, mask);

@@ -362,7 +365,8 @@ void __devinit pci_read_bridge_bases(str
}
}
if (base <= limit) {
- res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM | IORESOURCE_PREFETCH;
+ res->flags = (mem_base_lo & PCI_PREF_RANGE_TYPE_MASK) |
+ IORESOURCE_MEM | IORESOURCE_PREFETCH;
res->start = base;
res->end = limit + 0xfffff;
dev_printk(KERN_DEBUG, &dev->dev, "bridge %sbit mmio pref: %pR\n",
Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -143,6 +143,7 @@ static void pci_setup_bridge(struct pci_
struct pci_dev *bridge = bus->self;
struct pci_bus_region region;
u32 l, bu, lu, io_upper16;
+ int pref_mem64;

if (pci_is_enabled(bridge))
return;
@@ -198,16 +199,22 @@ static void pci_setup_bridge(struct pci_
pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, 0);

/* Set up PREF base/limit. */
+ pref_mem64 = 0;
bu = lu = 0;
pcibios_resource_to_bus(bridge, &region, bus->resource[2]);
if (bus->resource[2]->flags & IORESOURCE_PREFETCH) {
+ int width = 8;
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
- bu = upper_32_bits(region.start);
- lu = upper_32_bits(region.end);
- dev_info(&bridge->dev, " PREFETCH window: %#016llx-%#016llx\n",
- (unsigned long long)region.start,
- (unsigned long long)region.end);
+ if (bus->resource[2]->flags & PCI_PREF_RANGE_TYPE_64) {
+ pref_mem64 = 1;
+ bu = upper_32_bits(region.start);
+ lu = upper_32_bits(region.end);
+ width = 16;
+ }
+ dev_info(&bridge->dev, " PREFETCH window: %#0*llx-%#0*llx\n",
+ width, (unsigned long long)region.start,
+ width, (unsigned long long)region.end);
}
else {
l = 0x0000fff0;
@@ -215,9 +222,11 @@ static void pci_setup_bridge(struct pci_
}
pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, l);

- /* Set the upper 32 bits of PREF base & limit. */
- pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
- pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
+ if (pref_mem64) {
+ /* Set the upper 32 bits of PREF base & limit. */
+ pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
+ pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
+ }

pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, bus->bridge_ctl);
}
@@ -255,8 +264,11 @@ static void pci_bridge_check_ranges(stru
pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &pmem);
pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, 0x0);
}
- if (pmem)
+ if (pmem) {
b_res[2].flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH;
+ if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64)
+ b_res[2].flags |= PCI_PREF_RANGE_TYPE_64;
+ }
}

/* Helper function for sizing routines: find first available
@@ -336,6 +348,7 @@ static int pbus_size_mem(struct pci_bus
resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */
int order, max_order;
struct resource *b_res = find_free_bus_resource(bus, type);
+ unsigned int mem64_mask = 0;

if (!b_res)
return 0;
@@ -344,6 +357,11 @@ static int pbus_size_mem(struct pci_bus
max_order = 0;
size = 0;

+ if (type & IORESOURCE_PREFETCH) {
+ mem64_mask = b_res->flags & PCI_PREF_RANGE_TYPE_64;
+ b_res->flags &= ~PCI_PREF_RANGE_TYPE_64;
+ }
+
list_for_each_entry(dev, &bus->devices, bus_list) {
int i;

@@ -372,6 +390,8 @@ static int pbus_size_mem(struct pci_bus
aligns[order] += align;
if (order > max_order)
max_order = order;
+ if (r->flags & IORESOURCE_PREFETCH)
+ mem64_mask &= r->flags & PCI_PREF_RANGE_TYPE_64;
}
}

@@ -396,6 +416,8 @@ static int pbus_size_mem(struct pci_bus
b_res->start = min_align;
b_res->end = size + min_align - 1;
b_res->flags |= IORESOURCE_STARTALIGN;
+ if (type & IORESOURCE_PREFETCH)
+ b_res->flags |= mem64_mask;
return 1;
}

2009-04-18 08:46:49

by Yinghai Lu

[permalink] [raw]
Subject: [PATCH 3/3] pci: don't printout if the bus res size is 0


also print out if it is prefetechable mmio

[Impact: cleanup]

Signed-off-by: Yinghai Lu <[email protected]>

---
drivers/pci/setup-bus.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/setup-bus.c
===================================================================
--- linux-2.6.orig/drivers/pci/setup-bus.c
+++ linux-2.6/drivers/pci/setup-bus.c
@@ -558,11 +558,13 @@ static void pci_bus_dump_res(struct pci_

for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
struct resource *res = bus->resource[i];
- if (!res)
+ if (!res || !res->end)
continue;

dev_printk(KERN_DEBUG, &bus->dev, "resource %d %s %pR\n", i,
- (res->flags & IORESOURCE_IO) ? "io: " : "mem:", res);
+ (res->flags & IORESOURCE_IO) ? "io: " :
+ ((res->flags & IORESOURCE_PREFETCH)? "pref mem":"mem:"),
+ res);
}
}

2009-04-18 09:15:36

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/3] pci: don't assume pref memio are 64bit -v2


* Yinghai Lu <[email protected]> wrote:

> one system with 4g installed ( there is 1g hole)
>
> when 4G installed.
> BIOS put ACPI etc need the hole
> [ 0.000000] BIOS-provided physical RAM map:
> [ 0.000000] BIOS-e820: 0000000000000000 - 000000000009bc00 (usable)
> [ 0.000000] BIOS-e820: 000000000009bc00 - 00000000000a0000 (reserved)
> [ 0.000000] BIOS-e820: 00000000000e3000 - 0000000000100000 (reserved)
> [ 0.000000] BIOS-e820: 0000000000100000 - 00000000bffa0000 (usable)
> [ 0.000000] BIOS-e820: 00000000bffa0000 - 00000000bffae000 (ACPI data)
> [ 0.000000] BIOS-e820: 00000000bffae000 - 00000000bfff0000 (ACPI NVS)
> [ 0.000000] BIOS-e820: 00000000bfff0000 - 00000000c0000000 (reserved)
> [ 0.000000] BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
> [ 0.000000] BIOS-e820: 00000000ffb00000 - 0000000100000000 (reserved)
> [ 0.000000] BIOS-e820: 0000000100000000 - 0000000140000000 (usable)
> so in kernel resource will be reserved for 0xbffa0000 - 0xbfff0000 for ACPI
> 0x100000 - 0xbffa0000 for RAM...

btw., sidenote, it would be nice to enhance the e820 table printout
with size and hole info as well. I did this manually yesterday in
another mail:

BIOS-provided physical RAM map:
BIOS-e820: 0000000000000000 - 000000000009fc00 (usable) 0.639 MB RAM
BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved) 0.001 MB
[ hole ] 0.250 MB
BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved) 0.125 MB
BIOS-e820: 0000000000100000 - 000000003ed94000 (usable) 1004.5 MB RAM
BIOS-e820: 000000003ed94000 - 000000003ee4e000 (ACPI NVS) 0.7 MB
BIOS-e820: 000000003ee4e000 - 000000003fea2000 (usable) 16.3 MB RAM
BIOS-e820: 000000003fea2000 - 000000003fee9000 (ACPI NVS) 0.3 MB
BIOS-e820: 000000003fee9000 - 000000003feed000 (usable) 0.15 MB RAM
BIOS-e820: 000000003feed000 - 000000003feff000 (ACPI data 0.07 MB
BIOS-e820: 000000003feff000 - 000000003ff00000 (usable) 0.004 MB RAM
[ hole ] 1.0 MB
[ hole ] 3072.0 MB

it would be extremely useful to have that in the printout, to see
the physical address space layout at a glance. Small holes are easy
to miss, and e820 entry sizes are hard to judge at a glance.

To make this fit well, i'd suggest to drop the 'BIOS-e820: ' prefix
- it's redundant.

Ingo

2009-04-18 09:16:48

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 3/3] pci: don't printout if the bus res size is 0


* Yinghai Lu <[email protected]> wrote:

>
> also print out if it is prefetechable mmio
>
> [Impact: cleanup]
>
> Signed-off-by: Yinghai Lu <[email protected]>
>
> ---
> drivers/pci/setup-bus.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: linux-2.6/drivers/pci/setup-bus.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/setup-bus.c
> +++ linux-2.6/drivers/pci/setup-bus.c
> @@ -558,11 +558,13 @@ static void pci_bus_dump_res(struct pci_
>
> for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
> struct resource *res = bus->resource[i];
> - if (!res)
> + if (!res || !res->end)
> continue;
>
> dev_printk(KERN_DEBUG, &bus->dev, "resource %d %s %pR\n", i,
> - (res->flags & IORESOURCE_IO) ? "io: " : "mem:", res);
> + (res->flags & IORESOURCE_IO) ? "io: " :
> + ((res->flags & IORESOURCE_PREFETCH)? "pref mem":"mem:"),
> + res);

'pref mem' is easily mistaken for 'preferential memory' or something
similar. Would printing "prefetchable-mem" still be OK?

Ingo

2009-04-18 09:17:45

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 1/3] x86/pci: fix -1 calling to e820_all_mapped with mmconfig


* Yinghai Lu <[email protected]> wrote:

> e820_all_mapped need end is (addr + size) instead of (addr + size - 1)
>
> [Impact: fix -1 offset calling]
>
> Signed-off-by: Yinghai Lu <[email protected]>
>
> ---
> arch/x86/pci/mmconfig-shared.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
> ===================================================================
> --- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
> +++ linux-2.6/arch/x86/pci/mmconfig-shared.c
> @@ -375,7 +375,7 @@ static acpi_status __init check_mcfg_res
> if (!fixmem32)
> return AE_OK;
> if ((mcfg_res->start >= fixmem32->address) &&
> - (mcfg_res->end < (fixmem32->address +
> + (mcfg_res->end <= (fixmem32->address +
> fixmem32->address_length))) {
> mcfg_res->flags = 1;
> return AE_CTRL_TERMINATE;
> @@ -392,7 +392,7 @@ static acpi_status __init check_mcfg_res
> return AE_OK;
>
> if ((mcfg_res->start >= address.minimum) &&
> - (mcfg_res->end < (address.minimum + address.address_length))) {
> + (mcfg_res->end <= (address.minimum + address.address_length))) {
> mcfg_res->flags = 1;
> return AE_CTRL_TERMINATE;
> }
> @@ -439,7 +439,7 @@ static int __init is_mmconf_reserved(che
> u64 old_size = size;
> int valid = 0;
>
> - while (!is_reserved(addr, addr + size - 1, E820_RESERVED)) {
> + while (!is_reserved(addr, addr + size, E820_RESERVED)) {
> size >>= 1;
> if (size < (16UL<<20))
> break;

good catch. Probably also needed for -stable.

Acked-by: Ingo Molnar <[email protected]>

Ingo

2009-04-18 17:02:25

by Yinghai Lu

[permalink] [raw]
Subject: Re: [PATCH 2/3] pci: don't assume pref memio are 64bit -v2

Ingo Molnar wrote:
> * Yinghai Lu <[email protected]> wrote:
>
>> one system with 4g installed ( there is 1g hole)
>>
>> when 4G installed.
>> BIOS put ACPI etc need the hole
>> [ 0.000000] BIOS-provided physical RAM map:
>> [ 0.000000] BIOS-e820: 0000000000000000 - 000000000009bc00 (usable)
>> [ 0.000000] BIOS-e820: 000000000009bc00 - 00000000000a0000 (reserved)
>> [ 0.000000] BIOS-e820: 00000000000e3000 - 0000000000100000 (reserved)
>> [ 0.000000] BIOS-e820: 0000000000100000 - 00000000bffa0000 (usable)
>> [ 0.000000] BIOS-e820: 00000000bffa0000 - 00000000bffae000 (ACPI data)
>> [ 0.000000] BIOS-e820: 00000000bffae000 - 00000000bfff0000 (ACPI NVS)
>> [ 0.000000] BIOS-e820: 00000000bfff0000 - 00000000c0000000 (reserved)
>> [ 0.000000] BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
>> [ 0.000000] BIOS-e820: 00000000ffb00000 - 0000000100000000 (reserved)
>> [ 0.000000] BIOS-e820: 0000000100000000 - 0000000140000000 (usable)
>> so in kernel resource will be reserved for 0xbffa0000 - 0xbfff0000 for ACPI
>> 0x100000 - 0xbffa0000 for RAM...
>
> btw., sidenote, it would be nice to enhance the e820 table printout
> with size and hole info as well. I did this manually yesterday in
> another mail:
>
> BIOS-provided physical RAM map:
> BIOS-e820: 0000000000000000 - 000000000009fc00 (usable) 0.639 MB RAM
> BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved) 0.001 MB
> [ hole ] 0.250 MB
> BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved) 0.125 MB
> BIOS-e820: 0000000000100000 - 000000003ed94000 (usable) 1004.5 MB RAM
> BIOS-e820: 000000003ed94000 - 000000003ee4e000 (ACPI NVS) 0.7 MB
> BIOS-e820: 000000003ee4e000 - 000000003fea2000 (usable) 16.3 MB RAM
> BIOS-e820: 000000003fea2000 - 000000003fee9000 (ACPI NVS) 0.3 MB
> BIOS-e820: 000000003fee9000 - 000000003feed000 (usable) 0.15 MB RAM
> BIOS-e820: 000000003feed000 - 000000003feff000 (ACPI data 0.07 MB
> BIOS-e820: 000000003feff000 - 000000003ff00000 (usable) 0.004 MB RAM
> [ hole ] 1.0 MB
> [ hole ] 3072.0 MB
>
> it would be extremely useful to have that in the printout, to see
> the physical address space layout at a glance. Small holes are easy
> to miss, and e820 entry sizes are hard to judge at a glance.
>
> To make this fit well, i'd suggest to drop the 'BIOS-e820: ' prefix
> - it's redundant.

will look at it. and should be another patch for x86.

YH

2009-04-18 20:09:05

by Ivan Kokshaysky

[permalink] [raw]
Subject: Re: [PATCH 2/3] pci: don't assume pref memio are 64bit -v2

On Sat, Apr 18, 2009 at 01:44:51AM -0700, Yinghai Lu wrote:
> and BIOS set
> [ 0.240007] pci 0000:00:01.0: bridge 64bit mmio pref: [0xbdf00000-0xddefffff]

An obvious BIOS bug, the bridge base overlaps the physical low RAM
(0x00000000-0xc0000000). Technically speaking, this nonsense *happens*
to work on Intel hardware, so it seems to be quite common bug nowadays -
BIOS writers get lost in ACPI and other "useful" stuff contradicting
the PCI specs.

...

> + /* don't allocate too high if the pref mem doesn't support 64bit*/
> + if ((res->flags & (IORESOURCE_PREFETCH | PCI_PREF_RANGE_TYPE_64)) ==
> + IORESOURCE_PREFETCH)
> + max = 0xffffffff;

This effectively destroys non-x86 64-bit arches. You've been told about
that before, so I'm really surprised to see this "patch" once again.

Categorically NACKed.

P.S. I recall that I had a patch that addressed the issue, and Ingo
made some reasonable comments about it. Will post it tomorrow.

Ivan.

2009-04-18 20:26:26

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 2/3] pci: don't assume pref memio are 64bit -v2


* Ivan Kokshaysky <[email protected]> wrote:

> On Sat, Apr 18, 2009 at 01:44:51AM -0700, Yinghai Lu wrote:
> > and BIOS set
> > [ 0.240007] pci 0000:00:01.0: bridge 64bit mmio pref: [0xbdf00000-0xddefffff]
>
> An obvious BIOS bug, the bridge base overlaps the physical low RAM
> (0x00000000-0xc0000000). Technically speaking, this nonsense
> *happens* to work on Intel hardware, so it seems to be quite
> common bug nowadays - BIOS writers get lost in ACPI and other
> "useful" stuff contradicting the PCI specs.

it doesnt matter whether we call it a BIOS bug or not.

> ...
>
> > + /* don't allocate too high if the pref mem doesn't support 64bit*/
> > + if ((res->flags & (IORESOURCE_PREFETCH | PCI_PREF_RANGE_TYPE_64)) ==
> > + IORESOURCE_PREFETCH)
> > + max = 0xffffffff;
>
> This effectively destroys non-x86 64-bit arches. You've been told about
> that before, so I'm really surprised to see this "patch" once again.
>
> Categorically NACKed.

You can ridicule the patch and can NAK it (and rightfully so, it's
wrong), but you seem to miss the simple fact that this solves a very
real problem.

So consider this patch a documentation and analysis of a real
problem which made Linux work on hardware where it did not work
before. That's more valuable than 95% of our commits btw.

> P.S. I recall that I had a patch that addressed the issue, and
> Ingo made some reasonable comments about it. Will post it
> tomorrow.

That should have been pursued far more agressively.

Thanks,

Ingo

2009-04-22 22:05:35

by Jesse Barnes

[permalink] [raw]
Subject: Re: [PATCH 1/3] x86/pci: fix -1 calling to e820_all_mapped with mmconfig

On Sat, 18 Apr 2009 01:43:46 -0700
Yinghai Lu <[email protected]> wrote:

>
> e820_all_mapped need end is (addr + size) instead of (addr + size - 1)
>
> [Impact: fix -1 offset calling]
>
> Signed-off-by: Yinghai Lu <[email protected]>

Applied, thanks.

--
Jesse Barnes, Intel Open Source Technology Center