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

[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 1/2] i386 PCI: improve extended config space verification

[rajesh.shah]

[-- Attachment #1: i386-fix-mcfg-check --]
[-- Type: text/plain, Size: 5617 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/mmconfig.c   |  113 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/acpi/motherboard.c |    3 -
 include/acpi/aclocal.h     |    2 
 3 files changed, 112 insertions(+), 6 deletions(-)

Index: linux-2.6.17-mm1/arch/i386/pci/mmconfig.c
===================================================================
--- linux-2.6.17-mm1.orig/arch/i386/pci/mmconfig.c
+++ linux-2.6.17-mm1/arch/i386/pci/mmconfig.c
@@ -187,6 +187,108 @@ 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)
 {
 	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
@@ -201,10 +303,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;
+		}
 	}
 
 	printk(KERN_INFO "PCI: Using MMCONFIG\n");
Index: linux-2.6.17-mm1/include/acpi/aclocal.h
===================================================================
--- linux-2.6.17-mm1.orig/include/acpi/aclocal.h
+++ linux-2.6.17-mm1/include/acpi/aclocal.h
@@ -697,6 +697,8 @@ struct acpi_parse_state {
 
 #define PCI_ROOT_HID_STRING             "PNP0A03"
 #define PCI_EXPRESS_ROOT_HID_STRING     "PNP0A08"
+#define ACPI_MB_HID1			"PNP0C01"
+#define ACPI_MB_HID2			"PNP0C02"
 
 struct acpi_bit_register_info {
 	u8 parent_register;
Index: linux-2.6.17-mm1/drivers/acpi/motherboard.c
===================================================================
--- linux-2.6.17-mm1.orig/drivers/acpi/motherboard.c
+++ linux-2.6.17-mm1/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

--

[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

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

[Jeff Garzik]

rajesh.shah@intel.com wrote:
> 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.

On a related note -- PCI segments potentially enumerated in ACPI -- I 
have a PCI segments patch available at:

'pciseg' branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git

GregKH also has a few fixes for this which I need to integrate, too.

	Jeff