[patch, take 3] PCI: use ACPI to verify extended config space on x86

[patch, take 3] PCI: use ACPI to verify extended config space on x86

[Chuck Ebbert]

From: Rajesh Shah <rajesh.shah@intel.com>

Extend the verification for PCI-X/PCI-Express extended config
space pointer. Checks whether the MCFG address range is listed
as a motherboard resource, per the PCI firmware spec.

The old check only looked in int 15 e820 memory map, causing
several systems to fail the verification and lose extended
config space.

Uses common code shared between i386 and x86_64.

Declaration of is_acpi_reserved() is now in <acpi/acpi_drivers.h>

Signed-off-by: Rajesh Shah <rajesh.shah@intel.com>
Signed-off-by: Chuck Ebbert <76306.1226@compuserve.com>

--- 2.6.18-rc1-64.orig/arch/i386/pci/acpi.c
+++ 2.6.18-rc1-64/arch/i386/pci/acpi.c
@@ -5,6 +5,107 @@
 #include <asm/numa.h>
 #include "pci.h"
 
+static int __init is_motherboard_resource(acpi_handle handle)
+{
+       acpi_status status;
+       struct acpi_device_info *info;
+       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
+       int i;
+
+       status = acpi_get_object_info(handle, &buffer);
+       if (ACPI_FAILURE(status))
+	       return 0;
+       info = buffer.pointer;
+       if ((info->valid & ACPI_VALID_HID) &&
+		       (!strcmp(ACPI_MB_HID1, info->hardware_id.value) ||
+			!strcmp(ACPI_MB_HID2, info->hardware_id.value))) {
+	       kfree(buffer.pointer);
+	       return 1;
+       }
+       if (info->valid & ACPI_VALID_CID) {
+	       for (i=0; i < info->compatibility_id.count; i++) {
+		       if (!strcmp(ACPI_MB_HID1,
+				info->compatibility_id.id[i].value) ||
+				!strcmp(ACPI_MB_HID2,
+					info->compatibility_id.id[i].value)) {
+                                        kfree(buffer.pointer);
+                                        return 1;
+			}
+		}
+	}
+	kfree(buffer.pointer);
+        return 0;
+}
+
+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;
+
+		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_status status = AE_OK;
+
+	/* Stop namespace scanning if we've already verified MMCONFIG */
+	if (mcfg_res->flags)
+		return AE_CTRL_TERMINATE;
+
+	if (is_motherboard_resource(handle)) {
+		status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				check_mcfg_resource, context);
+	}
+	return status;
+}
+
+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_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+			ACPI_UINT32_MAX, find_mboard_resource,
+			(void *)&mcfg_res, NULL);
+	return mcfg_res.flags;
+}
+
 struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_device *device, int domain, int busnum)
 {
 	struct pci_bus *bus;
--- 2.6.18-rc1-64.orig/arch/i386/pci/mmconfig.c
+++ 2.6.18-rc1-64/arch/i386/pci/mmconfig.c
@@ -201,10 +201,15 @@ void __init pci_mmcfg_init(void)
 	if (!e820_all_mapped(pci_mmcfg_config[0].base_address,
 			pci_mmcfg_config[0].base_address + MMCONFIG_APER_MIN,
 			E820_RESERVED)) {
-		printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %x is not E820-reserved\n",
+		if (!is_acpi_reserved(pci_mmcfg_config[0].base_address,
+					pci_mmcfg_config[0].base_address +
+					MMCONFIG_APER_MIN)) {
+			printk(KERN_ERR
+				"PCI: BIOS Bug: MCFG area at %x not reserved\n",
 				pci_mmcfg_config[0].base_address);
-		printk(KERN_ERR "PCI: Not using MMCONFIG.\n");
-		return;
+			printk(KERN_ERR "PCI: Not using MMCONFIG.\n");
+			return;
+		}
 	}
 
 	printk(KERN_INFO "PCI: Using MMCONFIG\n");
--- 2.6.18-rc1-64.orig/drivers/acpi/motherboard.c
+++ 2.6.18-rc1-64/drivers/acpi/motherboard.c
@@ -32,9 +32,6 @@
 #define _COMPONENT		ACPI_SYSTEM_COMPONENT
 ACPI_MODULE_NAME("acpi_motherboard")
 
-/* Dell use PNP0C01 instead of PNP0C02 */
-#define ACPI_MB_HID1			"PNP0C01"
-#define ACPI_MB_HID2			"PNP0C02"
 /**
  * Doesn't care about legacy IO ports, only IO ports beyond 0x1000 are reserved
  * Doesn't care about the failure of 'request_region', since other may reserve
--- 2.6.18-rc1-64.orig/include/acpi/aclocal.h
+++ 2.6.18-rc1-64/include/acpi/aclocal.h
@@ -696,6 +696,10 @@ struct acpi_parse_state {
 
 #define PCI_ROOT_HID_STRING             "PNP0A03"
 #define PCI_EXPRESS_ROOT_HID_STRING     "PNP0A08"
+/* Dell use PNP0C01 instead of PNP0C02 */
+#define ACPI_MB_HID1			"PNP0C01"
+#define ACPI_MB_HID2			"PNP0C02"
+
 
 struct acpi_bit_register_info {
 	u8 parent_register;
--- 2.6.18-rc1-64.orig/include/acpi/acpi_drivers.h
+++ 2.6.18-rc1-64/include/acpi/acpi_drivers.h
@@ -76,6 +76,10 @@ int acpi_pci_bind_root(struct acpi_devic
 struct pci_bus *pci_acpi_scan_root(struct acpi_device *device, int domain,
 				   int bus);
 
+/* Arch-defined function to check if region is acpi-reserved */
+
+extern int is_acpi_reserved(unsigned long start, unsigned long end);
+
 /* --------------------------------------------------------------------------
                                   Power Resource
    -------------------------------------------------------------------------- */
--- 2.6.18-rc1-64.orig/arch/x86_64/pci/mmconfig.c
+++ 2.6.18-rc1-64/arch/x86_64/pci/mmconfig.c
@@ -180,10 +180,15 @@ void __init pci_mmcfg_init(void)
 	if (!e820_all_mapped(pci_mmcfg_config[0].base_address,
 			pci_mmcfg_config[0].base_address + MMCONFIG_APER_MIN,
 			E820_RESERVED)) {
-		printk(KERN_ERR "PCI: BIOS Bug: MCFG area at %x is not E820-reserved\n",
+		if (!is_acpi_reserved(pci_mmcfg_config[0].base_address,
+					pci_mmcfg_config[0].base_address +
+					MMCONFIG_APER_MIN)) {
+			printk(KERN_ERR
+				"PCI: BIOS Bug: MCFG area at %x is not reserved\n",
 				pci_mmcfg_config[0].base_address);
-		printk(KERN_ERR "PCI: Not using MMCONFIG.\n");
-		return;
+			printk(KERN_ERR "PCI: Not using MMCONFIG.\n");
+			return;
+		}
 	}
 
 	/* RED-PEN i386 doesn't do _nocache right now */
-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: [patch, take 3] PCI: use ACPI to verify extended config space on x86

[Arjan van de Ven]

On Fri, 2006-07-14 at 03:34 -0400, Chuck Ebbert wrote:
> From: Rajesh Shah <rajesh.shah@intel.com>
> 
> Extend the verification for PCI-X/PCI-Express extended config
> space pointer. Checks whether the MCFG address range is listed
> as a motherboard resource, per the PCI firmware spec.

I'm still not quite happy about this; the entire point of the check is
that we CAN'T trust the ACPI implementation, and want a second opinion.
This patch basically asks the ACPI implementation if we can trust the
ACPI implementation. I'm not sure that's a good idea.
And I understood that most issues went away with the more relaxed check
that is in gregkh's tree already (if not in mainline, I should check
that). 

Re: [patch, take 3] PCI: use ACPI to verify extended config space on x86

[Chuck Ebbert]

In-Reply-To: <1152869988.3159.25.camel@laptopd505.fenrus.org>

On Fri, 14 Jul 2006 11:39:48 +0200, Arjan van de Ven wrote:

> > Extend the verification for PCI-X/PCI-Express extended config
> > space pointer. Checks whether the MCFG address range is listed
> > as a motherboard resource, per the PCI firmware spec.
> 
> I'm still not quite happy about this; the entire point of the check is
> that we CAN'T trust the ACPI implementation, and want a second opinion.
> This patch basically asks the ACPI implementation if we can trust the
> ACPI implementation. I'm not sure that's a good idea.
> And I understood that most issues went away with the more relaxed check
> that is in gregkh's tree already (if not in mainline, I should check
> that). 

The more-relaxed check is in mainline.  I wrote it, but it didn't even
fix the problem on my own machine. This did.

According to Rajesh, the spec doesn't require the MCFG space to be
e820-reserved, so that's not really a valid check.

-- 
Chuck
 "You can't read a newspaper if you can't read."  --George W. Bush

Re: [patch, take 3] PCI: use ACPI to verify extended config space on x86

[Andi Kleen]

On Friday 14 July 2006 15:57, Chuck Ebbert wrote:
> In-Reply-To: <1152869988.3159.25.camel@laptopd505.fenrus.org>
> 
> On Fri, 14 Jul 2006 11:39:48 +0200, Arjan van de Ven wrote:
> 
> > > Extend the verification for PCI-X/PCI-Express extended config
> > > space pointer. Checks whether the MCFG address range is listed
> > > as a motherboard resource, per the PCI firmware spec.
> > 
> > I'm still not quite happy about this; the entire point of the check is
> > that we CAN'T trust the ACPI implementation, and want a second opinion.
> > This patch basically asks the ACPI implementation if we can trust the
> > ACPI implementation. I'm not sure that's a good idea.
> > And I understood that most issues went away with the more relaxed check
> > that is in gregkh's tree already (if not in mainline, I should check
> > that). 
> 
> The more-relaxed check is in mainline.  I wrote it, but it didn't even
> fix the problem on my own machine. 

Why did you submit it then when it didn't work?

> This did. 
> 
> According to Rajesh, the spec doesn't require the MCFG space to be
> e820-reserved, so that's not really a valid check.

Anyways Rajesh's patch is probably the way to go. If the ACPI
implementatin is self consistent it can be probably trusted.

The e820 check was just a heuristic and it clearly wasn't a good one. 

-Andi