[patch 0/2] Collecting host bridge resources

[patch 0/2] Collecting host bridge resources

[rajesh.shah]

ACPI hotplug code now uses the PCI core to allocate and manage
resources for hot-plug devices. To work correctly, this requires
all bridges to report resources they are decoding in their
pci_bus structure. We already do this for PCI-PCI bridges, but
not for host bridges. This patchset reads and stores host bridge
resources reported by ACPI BIOS for i386 and x86_64 systems.
Without this, we currently assume that all host bridges decode
all unused resources in iomem_resource and ioport_resource.
This patchset also adds a boot parameter (acpi=root_resources),
and the code to collect resources does not trigger unless it
is enabled via this boot option. This fixes hotplug failures
on some IBM (xseries) systems, and has been previously discussed
on the pci hotplug mailing list.

The current pci_bus structure can only store 4 resource
descriptors for a bus, but many host bridges decode more than 4
ranges. Storing incomplete host bridges resource ranges could
lead to resource-not-available errors for devices that were
otherwise properly configured by BIOS. Hence, this code triggers
only when it is explicitly enabled using the acpi=root_resources
boot option. For now, I expect that only systems that have PCI
hotplug slots directly under a host bridge will enable this option.

Rajesh

--

[patch 1/2] i386: collect host bridge resources

[rajesh.shah]

[-- Attachment #1: i386-host-bridge-resources.patch --]
[-- Type: text/plain, Size: 6507 bytes --]

This patch reads and stores host bridge resources reported by
ACPI BIOS for i386 systems.  This is needed since ACPI hotplug
code now uses the PCI core for resource management. This patch
also adds a new boot parameter (acpi=root_resources) to trigger
the code, and host bridge resources are not stored unless
this boot parameter is specified.

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

Index: linux-2.6.12-rc4-mm2/arch/i386/pci/acpi.c
===================================================================
--- linux-2.6.12-rc4-mm2.orig/arch/i386/pci/acpi.c
+++ linux-2.6.12-rc4-mm2/arch/i386/pci/acpi.c
@@ -5,14 +5,168 @@
 #include <asm/hw_irq.h>
 #include "pci.h"
 
+int acpi_read_root_resources;
+
+static void inline
+_set_resource(int idx, struct pci_bus *bus,
+		struct acpi_resource_address64 *addr, unsigned long flags)
+{
+	bus->resource[idx]->name = bus->name;
+	bus->resource[idx]->start = addr->min_address_range;
+	bus->resource[idx]->end = addr->max_address_range;
+	bus->resource[idx]->flags = flags;
+}
+
+static acpi_status
+add_resources(struct acpi_resource *acpi_res, void *context)
+{
+	struct acpi_resource_address64 address;
+	unsigned long smallest, old_size, new_size, flags = 0;
+	struct pci_bus *bus = context;
+	int i, idx = 1;
+
+	if (acpi_res->id != ACPI_RSTYPE_ADDRESS16 &&
+	    acpi_res->id != ACPI_RSTYPE_ADDRESS32 &&
+	    acpi_res->id != ACPI_RSTYPE_ADDRESS64)
+		return AE_OK;
+
+	if (ACPI_FAILURE(acpi_resource_to_address64(acpi_res, &address)))
+		return AE_OK;
+
+	/*
+	 * Per the ACPI spec, we should pick up only ACPI_PRODUCER type
+	 * resources. However, many BIOSs get this wrong and report
+	 * resources they pass down as ACPI_CONSUMER type resources. For now,
+	 * pick up all resources here.
+	 */
+	if (address.address_length <= 0)
+		return AE_OK;
+
+	switch (address.resource_type) {
+		/*
+		 * We (arbitrarily) reserve 1 resource descriptor for the
+		 * largest block of IO resources, and the remaining descriptors
+		 * for the largest blocks of memory resources.
+		 */
+		case ACPI_IO_RANGE:
+			flags = IORESOURCE_IO;
+			new_size = address.max_address_range -
+				address.min_address_range;
+			old_size = bus->resource[0]->end -
+				bus->resource[0]->start;
+			if (new_size > old_size) {
+				if (old_size)
+					printk(KERN_WARNING
+						"PCI: Ignoring IO range %lx-%lx\n",
+						bus->resource[0]->start,
+						bus->resource[0]->end);
+				_set_resource(0, bus, &address, flags);
+			}
+			break;
+
+		case ACPI_MEMORY_RANGE:
+			flags = IORESOURCE_MEM;
+			if (address.attribute.memory.cache_attribute ==
+					ACPI_PREFETCHABLE_MEMORY)
+				flags |= IORESOURCE_PREFETCH;
+			smallest = ~0;
+			new_size = address.max_address_range -
+				address.min_address_range;
+			for (i = 1; i < PCI_BUS_NUM_RESOURCES; i++) {
+				if (!bus->resource[i]->flags) {
+					_set_resource(i, bus, &address, flags);
+					return AE_OK;
+				}
+				old_size = bus->resource[i]->end -
+					bus->resource[i]->start;
+				if (old_size < smallest) {
+					smallest = old_size;
+					idx = i;
+				}
+			}
+			if (new_size > smallest) {
+				printk(KERN_WARNING
+					"PCI: Ignoring range %lx-%lx, flags %lx\n",
+					bus->resource[idx]->start,
+					bus->resource[idx]->end, flags);
+				_set_resource(idx, bus, &address, flags);
+			}
+			break;
+		default:
+			break;
+	}
+	return AE_OK;
+}
+
+static int __devinit
+verify_root_windows(struct pci_bus *bus)
+{
+	int i, num_io = 0, num_mem = 0;
+	int type_mask = IORESOURCE_IO | IORESOURCE_MEM;
+
+	for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++) {
+		switch (bus->resource[i]->flags & type_mask) {
+			case IORESOURCE_IO:
+				num_io++;
+				break;
+			case IORESOURCE_MEM:
+				num_mem++;
+				break;
+			default:
+				break;
+		}
+	}
+
+	if (num_io || num_mem)
+		return 1;
+	else
+		printk(KERN_WARNING
+			"PCI: BIOS reported bogus host bridge resources\n");
+	return 0;
+}
+
+static void __devinit
+pcibios_setup_root_windows(struct pci_bus *bus, acpi_handle handle)
+{
+	struct resource *res;
+	int i;
+	acpi_status status;
+
+	res = kmalloc((PCI_BUS_NUM_RESOURCES * sizeof(*res)),
+			GFP_KERNEL);
+	if (!res)
+		return;
+	memset(res, 0, (PCI_BUS_NUM_RESOURCES * sizeof(*res)));
+
+	for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++)
+		bus->resource[i] = res+i;
+
+	sprintf(bus->name, "PCI Bus #%02x", bus->number);
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+			add_resources, bus);
+	if (ACPI_FAILURE(status) || !verify_root_windows(bus)) {
+		printk(KERN_WARNING
+			"PCI: Falling back to default host bridge resources\n");
+		for (i = 0; i < PCI_BUS_NUM_RESOURCES; i++)
+			bus->resource[i] = NULL;
+		kfree(res);
+		bus->resource[0] = &ioport_resource;
+		bus->resource[1] = &iomem_resource;
+	}
+}
+
 struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_device *device, int domain, int busnum)
 {
+	struct pci_bus *bus;
 	if (domain != 0) {
 		printk(KERN_WARNING "PCI: Multiple domains not supported\n");
 		return NULL;
 	}
 
-	return pcibios_scan_root(busnum);
+	bus =  pcibios_scan_root(busnum);
+	if ((bus) && (acpi_read_root_resources))
+		pcibios_setup_root_windows(bus, device->handle);
+	return bus;
 }
 
 extern int pci_routeirq;
Index: linux-2.6.12-rc4-mm2/arch/i386/kernel/setup.c
===================================================================
--- linux-2.6.12-rc4-mm2.orig/arch/i386/kernel/setup.c
+++ linux-2.6.12-rc4-mm2/arch/i386/kernel/setup.c
@@ -791,6 +791,10 @@ static void __init parse_cmdline_early (
 		else if (!memcmp(from, "acpi=noirq", 10)) {
 			acpi_noirq_set();
 		}
+		/* Use ACPI to read host bridge resources */
+		else if (!memcmp(from, "acpi=root_resources", 19)) {
+			acpi_set_read_root_resources();
+		}
 
 		else if (!memcmp(from, "acpi_sci=edge", 13))
 			acpi_sci_flags.trigger =  1;
Index: linux-2.6.12-rc4-mm2/include/asm-i386/acpi.h
===================================================================
--- linux-2.6.12-rc4-mm2.orig/include/asm-i386/acpi.h
+++ linux-2.6.12-rc4-mm2/include/asm-i386/acpi.h
@@ -164,10 +164,16 @@ static inline void acpi_disable_pci(void
 	acpi_noirq_set();
 }
 extern int acpi_irq_balance_set(char *str);
+extern int acpi_read_root_resources;
+static inline void acpi_set_read_root_resources(void)
+{
+	acpi_read_root_resources = 1;
+}
 #else
 static inline void acpi_noirq_set(void) { }
 static inline void acpi_disable_pci(void) { }
 static inline int acpi_irq_balance_set(char *str) { return 0; }
+static inline void acpi_set_read_root_resources(void) { }
 #endif
 
 #ifdef CONFIG_ACPI_SLEEP

--

[patch 2/2] x86_64: Collect host bridge resources

[rajesh.shah]

[-- Attachment #1: x86_64-host-bridge-resources.patch --]
[-- Type: text/plain, Size: 1716 bytes --]

This patch reads and stores host bridge resources reported by
ACPI BIOS for x86_64 systems. This is needed since ACPI hotplug
code now uses the PCI core for resource management. This patch
simply adds the boot parameter (acpi=root_resources) to enable
the functionality that is implemented in arch/i386.

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

Index: linux-2.6.12-rc4-mm2/include/asm-x86_64/acpi.h
===================================================================
--- linux-2.6.12-rc4-mm2.orig/include/asm-x86_64/acpi.h
+++ linux-2.6.12-rc4-mm2/include/asm-x86_64/acpi.h
@@ -143,10 +143,16 @@ static inline void acpi_disable_pci(void
 	acpi_noirq_set();
 }
 extern int acpi_irq_balance_set(char *str);
+extern int acpi_read_root_resources;
+static inline void acpi_set_read_root_resources(void)
+{
+	acpi_read_root_resources = 1;
+}
 #else
 static inline void acpi_noirq_set(void) { }
 static inline void acpi_disable_pci(void) { }
 static inline int acpi_irq_balance_set(char *str) { return 0; }
+static inline void acpi_set_read_root_resources(void) { }
 #endif
 
 #ifdef CONFIG_ACPI_SLEEP
Index: linux-2.6.12-rc4-mm2/arch/x86_64/kernel/setup.c
===================================================================
--- linux-2.6.12-rc4-mm2.orig/arch/x86_64/kernel/setup.c
+++ linux-2.6.12-rc4-mm2/arch/x86_64/kernel/setup.c
@@ -319,6 +319,9 @@ static __init void parse_cmdline_early (
 			acpi_disable_pci();
 		else if (!memcmp(from, "acpi=noirq", 10))
 			acpi_noirq_set();
+		/* Use ACPI to read host bridge resources */
+		else if (!memcmp(from, "acpi=root_resources", 19))
+			acpi_set_read_root_resources();
 
 		else if (!memcmp(from, "acpi_sci=edge", 13))
 			acpi_sci_flags.trigger =  1;

--

Re: [patch 2/2] x86_64: Collect host bridge resources

[Andi Kleen]

On Fri, May 20, 2005 at 05:42:41PM -0700, rajesh.shah@intel.com wrote:
> This patch reads and stores host bridge resources reported by
> ACPI BIOS for x86_64 systems. This is needed since ACPI hotplug
> code now uses the PCI core for resource management. This patch
> simply adds the boot parameter (acpi=root_resources) to enable
> the functionality that is implemented in arch/i386.
> 

This means all hot plug users have to pass this strange parameter?
That does not sound very user friendly. Especially since you usually
only need pci hotplug in emergencies, and then you likely didnt pass it.

Cant you find a way to do this without parameters? Any reason
to not make it default?

-Andi

Re: [patch 2/2] x86_64: Collect host bridge resources

[Rajesh Shah]

On Mon, May 23, 2005 at 06:15:07PM +0200, Andi Kleen wrote:
> On Fri, May 20, 2005 at 05:42:41PM -0700, rajesh.shah@intel.com wrote:
> > This patch reads and stores host bridge resources reported by
> > ACPI BIOS for x86_64 systems. This is needed since ACPI hotplug
> > code now uses the PCI core for resource management. This patch
> > simply adds the boot parameter (acpi=root_resources) to enable
> > the functionality that is implemented in arch/i386.
> > 
> 
> This means all hot plug users have to pass this strange parameter?
> That does not sound very user friendly. Especially since you usually
> only need pci hotplug in emergencies, and then you likely didnt pass it.
> 
> Cant you find a way to do this without parameters? Any reason
> to not make it default?
> 
I found several systems in which the host bridge was decoding 6+
resource ranges. In the pci_bus structure, I only have room for 4,
so I'm forced to drop some ranges that are in fact being passed
down. For such cases, if I enable this by default, I see boot time
failures for devices that attempted to claim these dropped resources.
These devices were otherwise properly configured by BIOS, and work
fine with today's (incorrect) assumption that all host bridges
decode all unclaimed resources. I didn't want the patch to break
existing systems, hence the boot parameter.

Another option I'd thought of but never really pursued was to
implement this as a late_initcall. I'll look into that some more.
In that case, we'd continue to think that all host bridges decode
all unclaimed resources at boot time and depend on BIOS to program
resources for boot time devices correctly. Later, we'd collect the
more accurate host bridge resource picture to make hotplug work
correctly. Kind of hackish, but I can't think of another way to
avoid the boot parameter.

Rajesh

Re: [patch 2/2] x86_64: Collect host bridge resources

[Andi Kleen]

On Mon, May 23, 2005 at 05:57:08PM -0700, Rajesh Shah wrote:
> On Mon, May 23, 2005 at 06:15:07PM +0200, Andi Kleen wrote:
> > On Fri, May 20, 2005 at 05:42:41PM -0700, rajesh.shah@intel.com wrote:
> > > This patch reads and stores host bridge resources reported by
> > > ACPI BIOS for x86_64 systems. This is needed since ACPI hotplug
> > > code now uses the PCI core for resource management. This patch
> > > simply adds the boot parameter (acpi=root_resources) to enable
> > > the functionality that is implemented in arch/i386.
> > > 
> > 
> > This means all hot plug users have to pass this strange parameter?
> > That does not sound very user friendly. Especially since you usually
> > only need pci hotplug in emergencies, and then you likely didnt pass it.
> > 
> > Cant you find a way to do this without parameters? Any reason
> > to not make it default?
> > 
> I found several systems in which the host bridge was decoding 6+
> resource ranges. In the pci_bus structure, I only have room for 4,
> so I'm forced to drop some ranges that are in fact being passed

How about you allocate an extended structure with kmalloc in this case?
Or if it is only 6 ranges max (it is not, is it?) you could extend
the array.

I doubt this information will need *that* much memory, so it should
be reasonable to just teach the PCI subsystem about it.

> Another option I'd thought of but never really pursued was to
> implement this as a late_initcall. I'll look into that some more.
> In that case, we'd continue to think that all host bridges decode
> all unclaimed resources at boot time and depend on BIOS to program
> resources for boot time devices correctly. Later, we'd collect the
> more accurate host bridge resource picture to make hotplug work
> correctly. Kind of hackish, but I can't think of another way to
> avoid the boot parameter.

It sounds preferable to me to just give PCI the full picture from
the beginning instead of using such hacks which will likely
come back later to hurt us.

-Andi

Re: [patch 2/2] x86_64: Collect host bridge resources

[Ivan Kokshaysky]

On Tue, May 24, 2005 at 02:05:27PM +0200, Andi Kleen wrote:
> How about you allocate an extended structure with kmalloc in this case?

This would lead to quite a few changes in the PCI subsystem.
Looks good as a long-term solution though.

> Or if it is only 6 ranges max (it is not, is it?) you could extend
> the array.
> 
> I doubt this information will need *that* much memory, so it should
> be reasonable to just teach the PCI subsystem about it.

Agreed. As a bonus, extending the PCI_BUS_NUM_RESOURCES to 6 would
cleanly resolve problems with "transparent" PCI bridges - the bus
might have 3 "native" + 3 parent bus ranges in that case.

Ivan.

Re: [patch 2/2] x86_64: Collect host bridge resources

[Rajesh Shah]

On Tue, May 24, 2005 at 06:58:56PM +0400, Ivan Kokshaysky wrote:
> On Tue, May 24, 2005 at 02:05:27PM +0200, Andi Kleen wrote:
> > How about you allocate an extended structure with kmalloc in this case?
> 
> This would lead to quite a few changes in the PCI subsystem.
> Looks good as a long-term solution though.
> 
Yes, I did look at that and this would be a big change that would
affect almost all architectures. I was thinking something like
this would be more appropriate as part of the PCI rewrite that
Adam Belay had proposed.

> > Or if it is only 6 ranges max (it is not, is it?) you could extend
> > the array.
> > 
No, 6 is not guaranteed but would cover a larger percentage of
systems. 8 would probably cover all but a few special cases.

> > I doubt this information will need *that* much memory, so it should
> > be reasonable to just teach the PCI subsystem about it.
> 
> Agreed. As a bonus, extending the PCI_BUS_NUM_RESOURCES to 6 would
> cleanly resolve problems with "transparent" PCI bridges - the bus
> might have 3 "native" + 3 parent bus ranges in that case.
> 
The concern here isn't just increasing the size of pci_bus. The
resource pointers in pci_bus point to resource structures in the
corresponding pci_dev structure for p2p bridges. If we want to
maintain this scheme, we'd have to increase the number of resources
in the pci_dev structure too, which increases it for every single
pci device in the system. Probably ok for big server machines, but
would others (e.g. embedded folks) complain?

I just realized that I did not explicitly CC Greg in my original
post. Doing that now, to see what he thinks.

Rajesh

Re: [patch 2/2] x86_64: Collect host bridge resources

[Ivan Kokshaysky]

On Tue, May 24, 2005 at 08:45:36AM -0700, Rajesh Shah wrote:
> The concern here isn't just increasing the size of pci_bus. The
> resource pointers in pci_bus point to resource structures in the
> corresponding pci_dev structure for p2p bridges. If we want to
> maintain this scheme, we'd have to increase the number of resources
> in the pci_dev structure too, which increases it for every single
> pci device in the system.

No. The pci_bus resource pointers are just pointers to _some_ resources
and generally aren't tied to particular pci device. For example, the
root pci buses often don't even have corresponding bus->self structure,
and bus resources are pointers to global io[mem,port]_resource.
And definitely we must not touch resource layout in struct pci_dev -
it's defined by pci specs.

> Probably ok for big server machines, but
> would others (e.g. embedded folks) complain?

Increasing the size of pci_bus by 8 or 16 bytes shouldn't be a problem -
I don't think embedded machines have a lot of pci buses. :-)
Anyway, the default PCI_BUS_NUM_RESOURCES value can be overridden in 
arch specific headers.

Ivan.

Re: [patch 2/2] x86_64: Collect host bridge resources

[Rajesh Shah]

On Tue, May 24, 2005 at 08:58:55PM +0400, Ivan Kokshaysky wrote:
> On Tue, May 24, 2005 at 08:45:36AM -0700, Rajesh Shah wrote:
> > The concern here isn't just increasing the size of pci_bus. The
> > resource pointers in pci_bus point to resource structures in the
> > corresponding pci_dev structure for p2p bridges. If we want to
> > maintain this scheme, we'd have to increase the number of resources
> > in the pci_dev structure too, which increases it for every single
> > pci device in the system.
> 
> No. The pci_bus resource pointers are just pointers to _some_ resources
> and generally aren't tied to particular pci device. For example, the
> root pci buses often don't even have corresponding bus->self structure,
> and bus resources are pointers to global io[mem,port]_resource.

For the transparent p2p bridge problem you mentioned, wouldn't you
be dealing with p2p bridges, and therefore expect the pci_bus
resource pointers to point to the corresponding pci_dev resources?
Or are you proposing to decouple pci_bus resource pointers from 
pci_dev completely? From quick code inspection, that seems to be
not too much trouble to increase from 4 then.

Rajesh

Re: [patch 2/2] x86_64: Collect host bridge resources

[Ivan Kokshaysky]

On Tue, May 24, 2005 at 10:37:25AM -0700, Rajesh Shah wrote:
> For the transparent p2p bridge problem you mentioned, wouldn't you
> be dealing with p2p bridges, and therefore expect the pci_bus
> resource pointers to point to the corresponding pci_dev resources?

The problem is that for subtractive decode bridges we assume
full "transparency" and completely ignore standard p2p bridge
resources (i.e. windows) just using first 3 parent bus pointers
whatever they are.
This model does work in most cases, but there are potential problems
with peer-to-peer DMA behind such bridges, poor performance for MMIO
ranges outside bridge windows, prefetchable vs. non-prefetchable issues
and so on.

If we had 6 or more resource pointers in struct pci_bus, then the
appended patch would fix that.

> Or are you proposing to decouple pci_bus resource pointers from 
> pci_dev completely?

Actually no. Low-level bridge drivers (p2p, cardbus or particular
host bridge) certainly know about resource layout of the respective
device. But generic resource management code doesn't make any
assumptions about that and looks only at resource types.

> From quick code inspection, that seems to be
> not too much trouble to increase from 4 then.

No trouble at all, I guess.

Ivan.

--- linux/drivers/pci/probe.c.orig	Sat May  7 09:20:31 2005
+++ linux/drivers/pci/probe.c	Wed May 25 18:31:34 2005
@@ -239,9 +239,8 @@ void __devinit pci_read_bridge_bases(str
 
 	if (dev->transparent) {
 		printk(KERN_INFO "PCI: Transparent bridge - %s\n", pci_name(dev));
-		for(i = 0; i < PCI_BUS_NUM_RESOURCES; i++)
-			child->resource[i] = child->parent->resource[i];
-		return;
+		for(i = 3; i < PCI_BUS_NUM_RESOURCES; i++)
+			child->resource[i] = child->parent->resource[i - 3];
 	}
 
 	for(i=0; i<3; i++)