Patchwork [16/16] Ranges unavailable for PCI BARs should be marked as reserved in the E820 memory map, in case the OS wants to change the BARs.

login
register
about
Submitter Tobias Diedrich
Date 2010-11-07 12:46:38
Message ID <20101107124729.095637758@yamamaya.is-a-geek.org>
Download mbox | patch
Permalink /patch/2271/
State New
Headers show

Comments

Tobias Diedrich - 2010-11-07 12:46:38
Linux also needs the MMCONF area to be reserved either in E820 or
as an ACPI motherboard resource or it will not enable MMCONFIG
and the extended pcie configuration area will be unaccessible:

This patch adds the IORESOURCE_RESERVE flag to the APIC and MMCONF
resource flags to do this.
I also added a new resource for the mapped bios rom area just below 4GB.
I'm not sure if the choice for the index parameter of new_resource()
is correct though.
Note that the bios rom decode is enabled in
src/southbridge/via/vt8237r/vt8237r_early_smbus.c
for the whole 4MB area (even though the comment says 1MB).

dmesg excerpt without patch:
|BIOS-provided physical RAM map:
| BIOS-e820: 0000000000000000 - 000000000009f000 (usable)
| BIOS-e820: 000000000009f000 - 00000000000a0000 (reserved)
| BIOS-e820: 00000000000f0000 - 0000000000100000 (reserved)
| BIOS-e820: 0000000000100000 - 00000000dffed000 (usable)
| BIOS-e820: 00000000dffed000 - 00000000e0000000 (reserved)
| BIOS-e820: 0000000100000000 - 0000000140000000 (usable)
[...]
|ACPI: bus type pci registered
|PCI: MMCONFIG for domain 0000 [bus 00-ff] at [mem 0xe0000000-0xefffffff] (base 0xe0000000)
|PCI: not using MMCONFIG
|PCI: PCI BIOS revision 2.10 entry at 0xffe77, last bus=7
|PCI: Using configuration type 1 for base access
|bio: create slab <bio-0> at 0
|ACPI: EC: Look up EC in DSDT
|ACPI: Interpreter enabled
|ACPI: (supports S0 S5)
|ACPI: Using IOAPIC for interrupt routing
|PCI: MMCONFIG for domain 0000 [bus 00-ff] at [mem 0xe0000000-0xefffffff] (base 0xe0000000)
|[Firmware Bug]: PCI: MMCONFIG at [mem 0xe0000000-0xefffffff] not reserved in ACPI motherboard resources
|PCI: not using MMCONFIG
|PCI: DMI: pci_use_crs=1 pci_probe=0000000b
|PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
|ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
|pci_root PNP0A03:00: host bridge window [io  0x0000-0x0cf7]
|pci_root PNP0A03:00: host bridge window [io  0x0d00-0xffff]
|pci_root PNP0A03:00: host bridge window [mem 0xe0000000-0xffffffff]

dmesg excerpt with patch:
|BIOS-provided physical RAM map:
| BIOS-e820: 0000000000000000 - 000000000009f000 (usable)
| BIOS-e820: 000000000009f000 - 00000000000a0000 (reserved)
| BIOS-e820: 00000000000f0000 - 0000000000100000 (reserved)
| BIOS-e820: 0000000000100000 - 00000000dffed000 (usable)
| BIOS-e820: 00000000dffed000 - 00000000f0000000 (reserved)
| BIOS-e820: 00000000fec00000 - 00000000fec00100 (reserved)
| BIOS-e820: 00000000fecc0000 - 00000000fecc0100 (reserved)
| BIOS-e820: 00000000ffc00000 - 0000000100000000 (reserved)
| BIOS-e820: 0000000100000000 - 0000000140000000 (usable)
[...]
|ACPI: bus type pci registered
|PCI: MMCONFIG for domain 0000 [bus 00-ff] at [mem 0xe0000000-0xefffffff] (base 0xe0000000)
|PCI: MMCONFIG at [mem 0xe0000000-0xefffffff] reserved in E820
|PCI: Using MMCONFIG for extended config space
|PCI: Using configuration type 1 for base access
|bio: create slab <bio-0> at 0
|ACPI: EC: Look up EC in DSDT
|ACPI: Interpreter enabled
|ACPI: (supports S0 S5)
|ACPI: Using IOAPIC for interrupt routing
|PCI: DMI: pci_use_crs=1 pci_probe=00000008
|PCI: Using host bridge windows from ACPI; if necessary, use "pci=nocrs" and report a bug
|ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
|pci_root PNP0A03:00: host bridge window [io  0x0000-0x0cf7]
|pci_root PNP0A03:00: host bridge window [io  0x0d00-0xffff]
|pci_root PNP0A03:00: host bridge window [mem 0xe0000000-0xffffffff]

Signed-off-by: Tobias Diedrich <ranma+coreboot@tdiedrich.de>

---
Scott - 2010-11-07 17:06:40
-----Original Message-----
From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Tobias Diedrich
Sent: Sunday, November 07, 2010 06:47 AM
To: coreboot@coreboot.org
Cc: Rudolf Marek; Tobias Diedrich
Subject: [coreboot] [patch 16/16] Ranges unavailable for PCI BARs should bemarked as reserved in the E820 memory map,in case the OS
wants to change the BARs.

]Linux also needs the MMCONF area to be reserved either in E820 or
]as an ACPI motherboard resource or it will not enable MMCONFIG
]and the extended pcie configuration area will be unaccessible:
]
]This patch adds the IORESOURCE_RESERVE flag to the APIC and MMCONF
]resource flags to do this.
]I also added a new resource for the mapped bios rom area just below 4GB.
]I'm not sure if the choice for the index parameter of new_resource()
]is correct though.
]Note that the bios rom decode is enabled in
]src/southbridge/via/vt8237r/vt8237r_early_smbus.c
]for the whole 4MB area (even though the comment says 1MB).

Thank you Tobias. To be even more conservative, the upper 5 MB of the
first 4GB can be reserved for flash memory. This is because many LPC
flash chips place the jedec ID register of the boot device at address
ffbc0000.

Thanks,
Scott
Tobias Diedrich - 2010-11-07 20:32:34
Scott Duplichan wrote:
> From: Tobias Diedrich
> ]Linux also needs the MMCONF area to be reserved either in E820 or
> ]as an ACPI motherboard resource or it will not enable MMCONFIG
> ]and the extended pcie configuration area will be unaccessible:
> ]
> ]This patch adds the IORESOURCE_RESERVE flag to the APIC and MMCONF
> ]resource flags to do this.
> ]I also added a new resource for the mapped bios rom area just below 4GB.
> ]I'm not sure if the choice for the index parameter of new_resource()
> ]is correct though.
> ]Note that the bios rom decode is enabled in
> ]src/southbridge/via/vt8237r/vt8237r_early_smbus.c
> ]for the whole 4MB area (even though the comment says 1MB).
> 
> Thank you Tobias. To be even more conservative, the upper 5 MB of the
> first 4GB can be reserved for flash memory. This is because many LPC
> flash chips place the jedec ID register of the boot device at address
> ffbc0000.

I think that probably doesn't apply here, since the LPC flash
shouldn't get chip-select outside the selected area.
However src/southbridge/via/vt8237r/bootblock.c (which I had missed
because I got my board to work without touching this file)
says its actually 8MB big for VT8237A and VT8237S.
Scott - 2010-11-07 20:49:17
-----Original Message-----
From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Tobias Diedrich
Sent: Sunday, November 07, 2010 02:33 PM
To: Scott Duplichan
Cc: 'Rudolf Marek'; coreboot@coreboot.org
Subject: Re: [coreboot] [patch 16/16] Ranges unavailable for PCI BARs shouldbemarked as reserved in the E820 memory map,in case the
OS wants to change the BARs.

]Scott Duplichan wrote:
]> From: Tobias Diedrich
]> ]Linux also needs the MMCONF area to be reserved either in E820 or
]> ]as an ACPI motherboard resource or it will not enable MMCONFIG
]> ]and the extended pcie configuration area will be unaccessible:
]> ]
]> ]This patch adds the IORESOURCE_RESERVE flag to the APIC and MMCONF
]> ]resource flags to do this.
]> ]I also added a new resource for the mapped bios rom area just below 4GB.
]> ]I'm not sure if the choice for the index parameter of new_resource()
]> ]is correct though.
]> ]Note that the bios rom decode is enabled in
]> ]src/southbridge/via/vt8237r/vt8237r_early_smbus.c
]> ]for the whole 4MB area (even though the comment says 1MB).
]> 
]> Thank you Tobias. To be even more conservative, the upper 5 MB of the
]> first 4GB can be reserved for flash memory. This is because many LPC
]> flash chips place the jedec ID register of the boot device at address
]> ffbc0000.
]
]I think that probably doesn't apply here, since the LPC flash
]shouldn't get chip-select outside the selected area.
]However src/southbridge/via/vt8237r/bootblock.c (which I had missed
]because I got my board to work without touching this file)
]says its actually 8MB big for VT8237A and VT8237S.

Hello Tobias,

Here is my concern,
1) Coreboot reserves only 4MB (ffc00000-ffffffff).
2) The OS then assigns a PCI memory range that ends at ffbfffff.
3) A bios flash update program is run from the OS. It expands the
   flash decode range if needed then tries to read the flash jedec
   ID at ffbc0000. Both the flash chip and PCI device are set to
   decode ffbc0000. I do not know which device wins. If the flash
   wins and overrides the PCI device, things will be OK unless the
   OS needs to access the PCI device before flashing is complete.
   If the PCI device wins and overrides the flash, then the flash
   update utility will not be able to read the jedec ID.

Thanks,
Scott

]-- 
]Tobias						PGP: http://8ef7ddba.uguu.de
Rudolf Marek - 2010-11-07 21:14:53
Hi,

I thought our resource allocator did not touch the high region because of 
lapic/hpet/apic stuff. Maybe it is no longer true. I got some issue with PCI 
sata  add on card (linux complaining about resource overlap on SB700)

Maybe this is heading to same direction?

Thanks,
Rudolf
Scott - 2010-11-07 21:51:57
-----Original Message-----
From: Rudolf Marek [mailto:r.marek@assembler.cz] 
Sent: Sunday, November 07, 2010 03:15 PM
To: Scott Duplichan
Cc: 'Tobias Diedrich'; coreboot@coreboot.org
Subject: Re: [coreboot] [patch 16/16] Ranges unavailable for PCI BARs shouldbemarked as reserved in the E820 memory map,in case the
OS wants to change the BARs.

]Hi,
]
]I thought our resource allocator did not touch the high region because of 
]lapic/hpet/apic stuff. Maybe it is no longer true. I got some issue with PCI 
]sata  add on card (linux complaining about resource overlap on SB700)

Hello Rudolf,

You are right about coreboot, at least for the present time. However,
the allocation scheme still needs some work. For example, fec00000
and above is never allocated to a pci device because of the I/O apic,
which is on the pci bus. However, mmconf is not protected from pci use,
because it falls under 'APIC_CLUSTER: 0', and not a pci bus:
http://www.coreboot.org/pipermail/coreboot/2010-October/061320.html

I think the immediate concern for reserving flash memory space is
because the OS is allowed to make PCI resource assignments on its own.
The OS could use any range not reserved in e820. However, windows,
and possibly linux, also restrict PCI use to address ranges that ACPI
_CRS show as belonging to the pci bus. Coreboot still needs work in
this area. So maybe my example is a little far-fetched. But a commercial
BIOS reserves all the memory decoded by the flash chip, so coreboot
should probably do the same unless there is a specific reason not to.

]Maybe this is heading to same direction?

Could be. I have two overlap situations with win7 to avoid. One is the
mmconf problem described above. The other is when two video cards are
present, they both claim the legacy vga memory and I/O ranges.

]Thanks,
]Rudolf
Tobias Diedrich - 2010-11-07 23:31:51
Scott Duplichan wrote:
> From: Tobias Diedrich
> ]Scott Duplichan wrote:
> ]> Thank you Tobias. To be even more conservative, the upper 5 MB of the
> ]> first 4GB can be reserved for flash memory. This is because many LPC
> ]> flash chips place the jedec ID register of the boot device at address
> ]> ffbc0000.
> ]
> ]I think that probably doesn't apply here, since the LPC flash
> ]shouldn't get chip-select outside the selected area.
> ]However src/southbridge/via/vt8237r/bootblock.c (which I had missed
> ]because I got my board to work without touching this file)
> ]says its actually 8MB big for VT8237A and VT8237S.
> 
> Hello Tobias,
> 
> Here is my concern,
> 1) Coreboot reserves only 4MB (ffc00000-ffffffff).
> 2) The OS then assigns a PCI memory range that ends at ffbfffff.
> 3) A bios flash update program is run from the OS. It expands the
>    flash decode range if needed then tries to read the flash jedec
>    ID at ffbc0000. Both the flash chip and PCI device are set to
>    decode ffbc0000. I do not know which device wins. If the flash
>    wins and overrides the PCI device, things will be OK unless the
>    OS needs to access the PCI device before flashing is complete.
>    If the PCI device wins and overrides the flash, then the flash
>    update utility will not be able to read the jedec ID.

When coreboot reserves the full decode range (so it can't be
expanded any further) it should be fine though.
Carl-Daniel Hailfinger - 2010-11-08 02:17:16
On 08.11.2010 00:31, Tobias Diedrich wrote:
> When coreboot reserves the full decode range (so it can't be
> expanded any further) it should be fine though.
>   

So far I have not seen flash decode ranges larger than 16 MB on any x86
chipset. Reserving less than 16 MB is very dangerous on all Intel/AMD
chipsets released in the last 6+ years because flashrom may maximize the
decode area to 16 MB after the machine has booted.

Regards,
Carl-Daniel

Patch

Index: src/southbridge/via/k8t890/k8t890_traf_ctrl.c
===================================================================
--- src/southbridge/via/k8t890/k8t890_traf_ctrl.c.orig	2010-11-07 01:19:19.000000000 +0100
+++ src/southbridge/via/k8t890/k8t890_traf_ctrl.c	2010-11-07 13:23:18.000000000 +0100
@@ -58,7 +58,7 @@ 
 	res->limit = res->base + res->size - 1;
 	res->align = 8;
 	res->gran = 8;
-	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
+	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_RESERVE |
 		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
 
 	/* Add an MMCONFIG resource. */
@@ -67,7 +67,7 @@ 
 	res->align = log2(res->size);
 	res->gran = log2(res->size);
 	res->limit = 0xffffffff;	/* 4G */
-	res->flags = IORESOURCE_MEM;
+	res->flags = IORESOURCE_MEM | IORESOURCE_RESERVE;
 }
 
 static void traf_ctrl_enable_generic(struct device *dev)
Index: src/southbridge/via/vt8237r/vt8237r_lpc.c
===================================================================
--- src/southbridge/via/vt8237r/vt8237r_lpc.c.orig	2010-11-07 13:00:14.000000000 +0100
+++ src/southbridge/via/vt8237r/vt8237r_lpc.c	2010-11-07 13:23:18.000000000 +0100
@@ -569,7 +569,15 @@ 
 	res->limit = 0xffffffffUL;
 	res->align = 8;
 	res->gran = 8;
-	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
+	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_RESERVE |
+		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
+
+	/* Fixed flashrom resource */
+	res = new_resource(dev, 4);
+	res->base = 0xffc00000UL;
+	res->size = 0x00400000UL; /* 4MB */
+	res->limit = 0xffffffffUL;
+	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED | IORESOURCE_RESERVE |
 		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
 
 	res = new_resource(dev, 1);