1/2: MMCONFIG: validate against ACPI motherboard resources

This path adds validation of the MMCONFIG table against the ACPI reserved motherboard resources. If the MMCONFIG table is found to be reserved in ACPI, we don't bother checking the E820 table. The PCI Express firmware spec apparently tells BIOS developers that reservation in ACPI is required and E820 reservation is optional, so checking against ACPI first makes sense. Many BIOSes don't reserve the MMCONFIG region in E820 even though it is perfectly functional, the existing check needlessly disables MMCONFIG in these cases.

In order to do this, MMCONFIG setup has been split into two phases. If PCI configuration type 1 is not available then MMCONFIG is enabled early as before. Otherwise, it is enabled later after the ACPI interpreter is enabled, since we need to be able to execute control methods in order to check the ACPI reserved resources. Presently this is just triggered off the end of ACPI interpreter initialization.

There are a few other behavioral changes here:

-Validate all MMCONFIG configurations provided, not just the first one.

-Validate the entire required length of each configuration according to the provided ending bus number is reserved, not just the minimum required allocation.

-Validate that the area is reserved even if we read it from the chipset directly and not from the MCFG table. This catches the case where the BIOS didn't set the location properly in the chipset and has mapped it over other things it shouldn't have.

diff -up linux-2.6.22-rc2-mm1/arch/i386/pci/init.c linux-2.6.22-rc2-mm1edit/arch/i386/pci/init.c
@@ -12,7 +12,7 @@ static __init int pci_access_init(void)

	type = pci_direct_probe();

#endif
#ifdef CONFIG_PCI_MMCONFIG
- pci_mmcfg_init(type);
+ pci_mmcfg_early_init(type);

#endif
	if (raw_pci_ops)
		return 0;

diff -up linux-2.6.22-rc2-mm1/arch/i386/pci/mmconfig-shared.c linux-2.6.22-rc2-mm1edit/arch/i386/pci/mmconfig-shared.c
@@ -206,9 +206,77 @@ static void __init pci_mmcfg_insert_reso

	pci_mmcfg_resources_inserted = 1;
}

-static void __init pci_mmcfg_reject_broken(int type)
+static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
+ void *data)
+{
+ struct resource *mcfg_res = data;
+ struct acpi_resource_address64 address;
+ acpi_status status;
+
+ if (res->type == ACPI_RESOURCE_TYPE_FIXED_MEMORY32) {
+ struct acpi_resource_fixed_memory32 *fixmem32 =
+ &res->data.fixed_memory32;
+ if (!fixmem32)
+ return AE_OK;
+ if ((mcfg_res->start >= fixmem32->address) &&
+ (mcfg_res->end <= (fixmem32->address +
+ fixmem32->address_length))) {
+ mcfg_res->flags = 1;
+ return AE_CTRL_TERMINATE;
+ }
+ }
+ if ((res->type != ACPI_RESOURCE_TYPE_ADDRESS32) &&
+ (res->type != ACPI_RESOURCE_TYPE_ADDRESS64))
+ return AE_OK;
+
+ status = acpi_resource_to_address64(res, &address);
+ if (ACPI_FAILURE(status) || (address.address_length <= 0) ||
+ (address.resource_type != ACPI_MEMORY_RANGE))
+ return AE_OK;
+
+ if ((mcfg_res->start >= address.minimum) &&
+ (mcfg_res->end <=
+ (address.minimum +address.address_length))) {
+ mcfg_res->flags = 1;
+ return AE_CTRL_TERMINATE;
+ }
+ return AE_OK;
+}
+
+static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
+ void *context, void **rv)
+{
+ struct resource *mcfg_res = context;
+
+ acpi_walk_resources(handle, METHOD_NAME__CRS,
+ check_mcfg_resource, context);
+
+ if (mcfg_res->flags)
+ return AE_CTRL_TERMINATE;
+
+ return AE_OK;
+}
+
+static int __init is_acpi_reserved(unsigned long start, unsigned long end) +{
+ struct resource mcfg_res;
+
+ mcfg_res.start = start;
+ mcfg_res.end = end;
+ mcfg_res.flags = 0;
+
+ acpi_get_devices("PNP0C01", find_mboard_resource, &mcfg_res, NULL);
+
+ if( !mcfg_res.flags )
+ acpi_get_devices("PNP0C02", find_mboard_resource, &mcfg_res, NULL);
+
+ return mcfg_res.flags;
+}
+
+static void __init pci_mmcfg_reject_broken(void)

{
	typeof(pci_mmcfg_config[0]) *cfg;

+ int i;

	if ((pci_mmcfg_config_num == 0) ||
	    (pci_mmcfg_config == NULL) ||
@@ -228,18 +296,36 @@ static void __init pci_mmcfg_reject_brok
		       "Rejected as broken MCFG.\n");
		goto reject;
	}

-
- /*
- * Only do this check when type 1 works. If it doesn't work
- * assume we run on a Mac and always use MCFG
- */
- if (type == 1 && !e820_all_mapped(cfg->address,
- cfg->address + MMCONFIG_APER_MIN,
- E820_RESERVED)) {
- printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %Lx is not"
- " E820-reserved\n", cfg->address);
- goto reject;
+
+ for(i=0; i < pci_mmcfg_config_num; i++) {
+ u32 size = (cfg->end_bus_number + 1) << 20;
+ cfg = &pci_mmcfg_config[i];
+ printk(KERN_NOTICE "PCI: MCFG configuration %d: base %p segment %hu buses %u - %u\n",
+ i, (void*)cfg->address, cfg->pci_segment,
+ (unsigned int)cfg->start_bus_number,
+ (unsigned int)cfg->end_bus_number);
+ if(is_acpi_reserved(cfg->address,
+ cfg->address + size - 1))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
+ "in ACPI motherboard resources\n",
+ cfg->address);
+ else {
+ printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %Lx is not "
+ "reserved in ACPI motherboard resources\n",
+ cfg->address);
+ /* Don't try to do this check unless configuration type 1 is
+ available. */
+ if((pci_probe & PCI_PROBE_CONF1) &&
+ e820_all_mapped(cfg->address,
+ cfg->address + size - 1,
+ E820_RESERVED))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved in E820\n",
+ cfg->address);
+ else
+ goto reject;
+ }

	}
+
	return;

reject:
@@ -249,20 +335,46 @@ reject:

	pci_mmcfg_config_num = 0;
}

-void __init pci_mmcfg_init(int type)
+void __init pci_mmcfg_early_init(int type)
+{
+ if ((pci_probe & PCI_PROBE_MMCONF) == 0)
+ return;
+
+ /* If type 1 access is available, no need to enable MMCONFIG yet, we can
+ defer until later when the ACPI interpreter is available to better
+ validate things. */
+ if( type == 1 )
+ return;
+
+ acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
+
+ if ((pci_mmcfg_config_num == 0) ||
+ (pci_mmcfg_config == NULL) ||
+ (pci_mmcfg_config[0].address == 0))
+ return;
+
+ if (pci_mmcfg_arch_init())
+ pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
+}
+
+void __init pci_mmcfg_late_init(void)

{
	int known_bridge = 0;

+ /* MMCONFIG disabled */

	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
		return;

+
+ /* MMCONFIG already enabled */
+ if (!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
+ return;

- if (type == 1 && pci_mmcfg_check_hostbridge())
+ if ((pci_probe & PCI_PROBE_CONF1) && pci_mmcfg_check_hostbridge())

		known_bridge = 1;
-
-	if (!known_bridge) {
+	else
		acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);

- pci_mmcfg_reject_broken(type);
- }
+
+ pci_mmcfg_reject_broken();

	if ((pci_mmcfg_config_num == 0) ||
	    (pci_mmcfg_config == NULL) ||
@@ -270,7 +382,7 @@ void __init pci_mmcfg_init(int type)
		return;

	if (pci_mmcfg_arch_init()) {
-		if (type == 1)
+		if (pci_probe & PCI_PROBE_CONF1)
			unreachable_devices();
		if (known_bridge)
			pci_mmcfg_insert_resources(IORESOURCE_BUSY);

diff -up linux-2.6.22-rc2-mm1/arch/i386/pci/pci.h linux-2.6.22-rc2-mm1edit/arch/i386/pci/pci.h
@@ -91,7 +91,8 @@ extern int pci_conf1_read(unsigned int s

extern int pci_direct_probe(void);
extern void pci_direct_init(int type);
extern void pci_pcbios_init(void);
-extern void pci_mmcfg_init(int type);
+extern void pci_mmcfg_early_init(int type);
+extern void pci_mmcfg_late_init(void);
extern void pcibios_sort(void);

/* pci-mmconfig.c */

@@ -42,6 +42,7 @@

ACPI_MODULE_NAME("bus");
#ifdef CONFIG_X86
extern void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger); +extern void __init pci_mmcfg_late_init(void);
#endif

struct acpi_device *acpi_root;
@@ -755,6 +756,9 @@ static int __init acpi_init(void)

	result = acpi_bus_init();

	if (!result) {
+#ifdef CONFIG_X86
+		pci_mmcfg_late_init();
+#endif		
#ifdef CONFIG_PM_LEGACY
		if (!PM_IS_ACTIVE())
			pm_active = 1;

Please fix the formatting of your code.

"for" and "if" are not functions, and they have a space before the parenthesis.

And pretty much every single conditional in this patch is spread out over two or more lines and has at least three different indentations. There's something wrong here. Code can't look this bad and still be fine. Some of this looks like random whitespace noise:

+ if(is_acpi_reserved(cfg->address,
+ cfg->address + size - 1))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
+ "in ACPI motherboard resources\n",
+ cfg->address);
+ else {

That's just horrid. Please try to make the code _look_ nicer.

For example, just making "is_acpi_reserved()" take a start/len thing instead, would allow you to at least do

	if (is_acpi_reserved(cfg->address, size)) {
		printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
			"in ACPI motherboard resources\n",
			cfg->address);
	} else {
		...

(and has the braces rigth too - don't pair an unbraced "if ()" with a braced "else" statement), and that together with making the body of the for-loop a separate function would possibly make that code read a lot better.

Same goes for this thing:

+ if((pci_probe & PCI_PROBE_CONF1) &&
+ e820_all_mapped(cfg->address,
+ cfg->address + size - 1,
+ E820_RESERVED))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved in E820\n",
+ cfg->address);
+ else
+ goto reject;

there really is *not* a highly coveted prize for having the most different indentation in the fewest possible lines of code!

Yeah, I realize that maybe this is nit-picking, but trying to read this patch really does make you go blind. It violates so many coding standards that it's almost impossible to read the code itself. It's made worse by the fact that you then also used Thunderbird to send the patch, and had it set for

Content-type: text/plain; charset=ISO-8859-1; format=flowed

where that "format=flowed" means that basically no mail-client will be able to read it sanely (because a lot of them will re-flow the text), and you have to save it to a file before you can even comment on it.

Gaah. See

http://mbligh.org/linuxdocs/Email/Clients/Thunderbird

where the most important sentence is "Thunderbird is written by aged whore monkeys stoned on crack". But it also talks about how to disable that idiotic format=flowed for patches, and how to make sure it's not wrapping.

But as mentioned, your patch itself had some whitespace issues even aside from the regular Thunderbird breakage.

Linus

[snip snip]

That looks like standard lisp style indenting (yes I realise that C != lisp). At a guess I would say that code was written in emacs (which can be fixed by the necessary changes in your ~/.emacs). Grand parent post should check out these resources, they may help...

http://www.bloomington.in.us/~brutt/emacs-c-dev.html
http://lug.umbc.edu/tutorials/adv-emacs.html

or if you are really gung ho the CC-mode documentation

http://www.chemie.fu-berlin.de/chemnet/use/info/cc-mode/cc-mode_6.html

Matt

I'll try and fix up the formatting and repost this patch. I suspect some of the issues are from the added code clashing with the way the existing code was formatted.

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

Thanks.

Well, I have to admit that I might not have reacted so much if it hadn't been for the Thunderbird thing, which made it look _really_ strange at first, so then I had to go outside my mail client to look closer. And once I looked closer, I just went "aiieee, it wasn't all the email client" ;)

Linus

Not long ago, Tejun pointed out the "External Editor" extension for Thunderbird, which turns out to be the only really sane way to submit patches with that client.

Download and install it, then add a button for it using View->Toolbars->Customize... and finally just click on it when in the Compose dialog.

A very useful tip. Thanks again to Tejun for pointing it out.

Yes, I've been using that one, as well as changing the word wrap length to 0 characters to switch that off. Apparently disabling format=flowed is needed as well, however :-)

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

I got fed up of telling people to reconfigure their MUAs a long time ago and ended up with this in my .procmailrc

:0fw
| /usr/bin/perl -pe 's/^(Content-Type: .*)format=flowed/\1format=flawed/'

It doesn't solve all the worlds problems, but it least makes that crap readable in _my_ MUA. Sadly, if it's a patch that I have to apply then chances are I'll have to get them to resend it with something else anyway, as it's inevitably buggered in some other way because thunderbird really is that dire.

I'm convinced there's some contest to see who can make the worst graphical mail client for Linux. I'm not sure what the prize is, or who's winning, but the entries so far are horrific.

Dave

http://www.codemonkey.org.uk

LOL.

Although in this case it was actually not a Linux MUA:

	User-Agent: Thunderbird 2.0.0.0 (Windows/20070326)

but I didn't want to embarrass Robert publicly before you brought up the issue of operating systems ;)

Your procmail filter looks somewhat broken, btw. It will trigger on things that aren't headers, and it will potentially _not_ trigger on headers (ie if they are line continuations). But I guess it works well enough in practice.

Linus

Oh well, there's no accounting for taste ;-)

Yeah, now that I look again, it actually missed on Robert's message because it isn't case-insensitive. It's far from perfect.

Dave

http://www.codemonkey.org.uk

Lotus Notes has no serious competition.

Andy's patch-checking script will (should) detect wordwrapping, tab-expansion and hopefully space-stuffing. When we get that sorted out, people who submit broken patches to one of the lists should get a robot reply within minutes telling them what they did wrong, so things will become largely self-correcting.

I am sooooo looking forward to that thing. <Sends note to Nobel prize committee>

This path adds validation of the MMCONFIG table against the ACPI reserved motherboard resources. If the MMCONFIG table is found to be reserved in ACPI, we don't bother checking the E820 table. The PCI Express firmware spec apparently tells BIOS developers that reservation in ACPI is required and E820 reservation is optional, so checking against ACPI first makes sense. Many BIOSes don't reserve the MMCONFIG region in E820 even though it is perfectly functional, the existing check needlessly disables MMCONFIG in these cases.

In order to do this, MMCONFIG setup has been split into two phases. If PCI configuration type 1 is not available then MMCONFIG is enabled early as before. Otherwise, it is enabled later after the ACPI interpreter is enabled, since we need to be able to execute control methods in order to check the ACPI reserved resources. Presently this is just triggered off the end of ACPI interpreter initialization.

There are a few other behavioral changes here:

-Validate all MMCONFIG configurations provided, not just the first one.

-Validate the entire required length of each configuration according to the provided ending bus number is reserved, not just the minimum required allocation.

-Validate that the area is reserved even if we read it from the chipset directly and not from the MCFG table. This catches the case where the BIOS didn't set the location properly in the chipset and has mapped it over other things it shouldn't have.

Based on an original patch by Rajesh Shah from Intel.

---

This should fix up some of the whitespace/formatting problems in the previous version. There were actually some bugs in the check_mcfg_resource function, there were some <= that should have been <. Also forgot the attribution for Rajesh Shah who wrote the original version of some of this code.

diff -rup --exclude-from=linux-2.6.22-rc2-mm1/Documentation/dontdiff linux-2.6.22-rc2-mm1/arch/i386/pci/init.c linux-2.6.22-rc2-mm1edit/arch/i386/pci/init.c
@@ -12,7 +12,7 @@ static __init int pci_access_init(void)
 	type = pci_direct_probe();
 #endif
 #ifdef CONFIG_PCI_MMCONFIG
-	pci_mmcfg_init(type);
+	pci_mmcfg_early_init(type);
 #endif
 	if (raw_pci_ops)
 		return 0;

diff -rup --exclude-from=linux-2.6.22-rc2-mm1/Documentation/dontdiff linux-2.6.22-rc2-mm1/arch/i386/pci/pci.h linux-2.6.22-rc2-mm1edit/arch/i386/pci/pci.h
@@ -91,7 +91,8 @@ extern int pci_conf1_read(unsigned int s
 extern int pci_direct_probe(void);
 extern void pci_direct_init(int type);
 extern void pci_pcbios_init(void);
-extern void pci_mmcfg_init(int type);
+extern void pci_mmcfg_early_init(int type);
+extern void pci_mmcfg_late_init(void);
 extern void pcibios_sort(void);
 
 /* pci-mmconfig.c */

diff -rup --exclude-from=linux-2.6.22-rc2-mm1/Documentation/dontdiff linux-2.6.22-rc2-mm1/drivers/acpi/bus.c linux-2.6.22-rc2-mm1edit/drivers/acpi/bus.c
@@ -42,6 +42,7 @@
 ACPI_MODULE_NAME("bus");
 #ifdef	CONFIG_X86
 extern void __init acpi_pic_sci_set_trigger(unsigned int irq, u16 trigger);
+extern void __init pci_mmcfg_late_init(void);
 #endif
 
 struct acpi_device *acpi_root;

@@ -755,6 +756,9 @@ static int __init acpi_init(void)
 	result = acpi_bus_init();
 
 	if (!result) {
+#ifdef CONFIG_X86
+		pci_mmcfg_late_init();
+#endif		
 #ifdef CONFIG_PM_LEGACY
 		if (!PM_IS_ACTIVE())
 			pm_active = 1;

Just for the record, I still fundamentally disagree with that part. You're not catching what you think you're catching, since the chipset tells you what it is going to decode as mmconfig, no matter what is connected to it.

OG.

But we don't - not in the case where it's overlapped some other area with the MMCONFIG region. We saw a case of this on some of the Intel boards, where the MMCONFIG is a 128MB area at 0xf0000000, and when sizing the BARs on a PCI Express video device with a 256MB region, it ended up being located momentarily at f0000000-ffffffff, which overlapped the MMCONFIG area. That caused MMCONFIG to stop working, so apparently on that chipset, PCI Express devices connected to the northbridge have a higher decode priority than the MMCONFIG area.

If the BIOS is so screwed up that this becomes an issue, I don't think we can sanely try to use the table, since we can't anticipate all the potential problems that might result.

Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

I'm not sure what you're objecting to? Are you suggesting that we use mmconfig space as defined by the bridge register even if it's not mentioned (or doesn't match) the MCFG table?

Jesse