[patch 0/2] PCI: improve extended config space verification - take #2

[patch 0/2] PCI: improve extended config space verification - take #2

[rajesh.shah]

ACPI defines an MCFG table that gives us the pointer to where the
extended PCI-X/PCI-Express configuration space exists. We validate
this region today by making sure that the reported range is marked
as reserved in the int 15 E820 memory map. However, the PCI firmware
spec states this is optional and BIOS should be reporting the MCFG
range as a motherboard resources. Several of my systems failed the
existing check and ended up without extended PCI-Express config
space. This patch extends the verification to also look for the
MCFG range as a motherboard resource in ACPI. This solves the
problem on my i386 as well as x86_64 test systems.

The difference from the first version of this patchset is that
I moved the bulk of the code to a file shared between i386 and
x86_64, to get code reuse.

Rajesh

--

[patch 1/2] i386 PCI: improve extended config space verification

[rajesh.shah]

[-- Attachment #1: i386-fix-mcfg-check --]
[-- Type: text/plain, Size: 5937 bytes --]

Extend the verification for PCI-X/PCI-Express extended config
space pointer. This patch 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.

Signed-off-by: Rajesh Shah <rajesh.shah@intel.com>

 arch/i386/pci/acpi.c       |  101 +++++++++++++++++++++++++++++++++++++++++++++
 arch/i386/pci/mmconfig.c   |   11 +++-
 drivers/acpi/motherboard.c |    3 -
 include/acpi/aclocal.h     |    4 +
 4 files changed, 113 insertions(+), 6 deletions(-)

Index: linux-2.6.17-mm2/arch/i386/pci/acpi.c
===================================================================
--- linux-2.6.17-mm2.orig/arch/i386/pci/acpi.c
+++ linux-2.6.17-mm2/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;
Index: linux-2.6.17-mm2/arch/i386/pci/mmconfig.c
===================================================================
--- linux-2.6.17-mm2.orig/arch/i386/pci/mmconfig.c
+++ linux-2.6.17-mm2/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");
Index: linux-2.6.17-mm2/drivers/acpi/motherboard.c
===================================================================
--- linux-2.6.17-mm2.orig/drivers/acpi/motherboard.c
+++ linux-2.6.17-mm2/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
Index: linux-2.6.17-mm2/include/acpi/aclocal.h
===================================================================
--- linux-2.6.17-mm2.orig/include/acpi/aclocal.h
+++ linux-2.6.17-mm2/include/acpi/aclocal.h
@@ -697,6 +697,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;

--

[patch 2/2] x86_64 PCI: improve extended config space verification

[rajesh.shah]

[-- Attachment #1: x86_64-fix-mcfg-check --]
[-- Type: text/plain, Size: 1792 bytes --]

Extend the verification for PCI-X/PCI-Express extended config
space pointer. This patch 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. x86_64 picks up the verification code from the i386
directory.

Signed-off-by: Rajesh Shah <rajesh.shah@intel.com>

 arch/x86_64/pci/mmconfig.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6.17-mm2/arch/x86_64/pci/mmconfig.c
===================================================================
--- linux-2.6.17-mm2.orig/arch/x86_64/pci/mmconfig.c
+++ linux-2.6.17-mm2/arch/x86_64/pci/mmconfig.c
@@ -16,6 +16,7 @@
 /* aperture is up to 256MB but BIOS may reserve less */
 #define MMCONFIG_APER_MIN	(2 * 1024*1024)
 #define MMCONFIG_APER_MAX	(256 * 1024*1024)
+extern int is_acpi_reserved(unsigned long start, unsigned long end);
 
 /* Verify the first 16 busses. We assume that systems with more busses
    get MCFG right. */
@@ -179,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 */

--

Re: [patch 2/2] x86_64 PCI: improve extended config space verification

[Andi Kleen]

> Index: linux-2.6.17-mm2/arch/x86_64/pci/mmconfig.c
> ===================================================================
> --- linux-2.6.17-mm2.orig/arch/x86_64/pci/mmconfig.c
> +++ linux-2.6.17-mm2/arch/x86_64/pci/mmconfig.c
> @@ -16,6 +16,7 @@
>  /* aperture is up to 256MB but BIOS may reserve less */
>  #define MMCONFIG_APER_MIN	(2 * 1024*1024)
>  #define MMCONFIG_APER_MAX	(256 * 1024*1024)
> +extern int is_acpi_reserved(unsigned long start, unsigned long end);

The prototype belongs into some shared header.

-Andi

[patch 0/2] PCI: improve extended config space verification

[rajesh.shah]

ACPI defines an MCFG table that gives us the pointer to where the
extended PCI-X/PCI-Express configuration space exists. We validate
this region today by making sure that the reported range is marked
as reserved in the int 15 E820 memory map. However, the PCI firmware
spec states this is optional and BIOS should be reporting the MCFG
range as a motherboard resources. Several of my systems failed the
existing check and ended up without extended PCI-Express config
space. This patch extends the verification to also look for the
MCFG range as a motherboard resource in ACPI. This solves the
problem on my i386 as well as x86_64 test systems.

Rajesh
--

[patch 2/2] x86_64 PCI: improve extended config space verification

[rajesh.shah]

[-- Attachment #1: x86_64-fix-mcfg-check --]
[-- Type: text/plain, Size: 4439 bytes --]

Extend the verification for PCI-X/PCI-Express extended config
space pointer. This patch 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.

Signed-off-by: Rajesh Shah <rajesh.shah@intel.com>

 arch/x86_64/pci/mmconfig.c |  112 +++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 109 insertions(+), 3 deletions(-)

Index: linux-2.6.17-mm1/arch/x86_64/pci/mmconfig.c
===================================================================
--- linux-2.6.17-mm1.orig/arch/x86_64/pci/mmconfig.c
+++ linux-2.6.17-mm1/arch/x86_64/pci/mmconfig.c
@@ -164,6 +164,107 @@ static __init void unreachable_devices(v
 	}
 }
 
+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;
+}
+
 void __init pci_mmcfg_init(void)
 {
 	int i;
@@ -180,10 +281,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 */

--

Re: [patch 2/2] x86_64 PCI: improve extended config space verification

[Andi Kleen]

On Friday 23 June 2006 22:09, rajesh.shah@intel.com wrote:
> Extend the verification for PCI-X/PCI-Express extended config
> space pointer. This patch 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.

By adding so much code to it you volunteered to factor the 
sanity check into a common i386/x86-64 file first.

-Andi

Re: [patch 2/2] x86_64 PCI: improve extended config space verification

[Greg KH]

On Fri, Jun 23, 2006 at 10:30:36PM +0200, Andi Kleen wrote:
> On Friday 23 June 2006 22:09, rajesh.shah@intel.com wrote:
> > Extend the verification for PCI-X/PCI-Express extended config
> > space pointer. This patch 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.
> 
> By adding so much code to it you volunteered to factor the 
> sanity check into a common i386/x86-64 file first.

I agree :)

Also, have you looked at the current -git tree?  It also modified some
stuff in this area?  Look at commit
ead2bfeb7f739d2ad6e09dc1343f0da51feb7f51 for details.

thanks,

greg k-h

Re: [patch 2/2] x86_64 PCI: improve extended config space verification

[Rajesh Shah]

On Fri, Jun 23, 2006 at 03:43:18PM -0700, Greg KH wrote:
> On Fri, Jun 23, 2006 at 10:30:36PM +0200, Andi Kleen wrote:
> > On Friday 23 June 2006 22:09, rajesh.shah@intel.com wrote:
> > > Extend the verification for PCI-X/PCI-Express extended config
> > > space pointer. This patch 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.
> > 
> > By adding so much code to it you volunteered to factor the 
> > sanity check into a common i386/x86-64 file first.
> 
> I agree :)
> 
> Also, have you looked at the current -git tree?  It also modified some
> stuff in this area?  Look at commit
> ead2bfeb7f739d2ad6e09dc1343f0da51feb7f51 for details.
> 
OK, I just reposted after moving the code to a common file. I did
check that it compiled against the current -git tree, though the
testing was done with -mm2. This did give me back my extended
config space, but it would also be nice if it made a difference
to the other folks who had reported this problem (CC'd).

thanks,
Rajesh