Evaluates the _CRS object under PCI0 looking for producer resources.

Then searches the e820 memory space for a gap within these producer resources.
Allows us to find a gap for the unclaimed pci resources or MMIO resources

for hotplug devices within the BIOS allowed pci regions.
v1->v2: Some changes in the print strings.

	Also note the prototype for e820_search_gap has changes in this

	iteration, but the return value is ignored while calling it over here.
Signed-off-by: Alok N Kataria <akataria@vmware.com>
Index: linux-x86-tree.git/arch/x86/pci/acpi.c

===================================================================

--- linux-x86-tree.git.orig/arch/x86/pci/acpi.c	2008-06-23 17:24:07.000000000 -0700

+++ linux-x86-tree.git/arch/x86/pci/acpi.c	2008-06-24 10:57:43.000000000 -0700

@@ -4,6 +4,7 @@

 #include <linux/irq.h>

 #include <linux/dmi.h>

 #include <asm/numa.h>

+#include <asm/e820.h>

 #include "pci.h"
 struct pci_root_info {

@@ -14,6 +15,11 @@

 	int busnum;

 };
+struct gap_info {

+	unsigned long gapstart;

+	unsigned long gapsize;

+};

+

 static acpi_status

 resource_to_addr(struct acpi_resource *resource,

 			struct acpi_resource_address64 *addr)

@@ -110,6 +116,70 @@

 	}

 }
+static acpi_status search_gap(struct acpi_resource *resource, void *data)

+{

+	struct acpi_resource_address64 addr;

+	acpi_status status;

+	struct gap_info *gap = data;

+

+	status = resource_to_addr(resource, &addr);

+	if (ACPI_SUCCESS(status) &&

+	    addr.resource_type == ACPI_MEMORY_RANGE &&

+	    addr.address_length > gap->gapsize) {

+		e820_search_gap(&gap->gapstart, &gap->gapsize,

+				(addr.minimum + addr.translation_offset));

+	}

+

+	return AE_OK;

+}

+

+/*

+ * Search for a hole in the 32 bit address space for PCI to assign MMIO

+ * resources for hotplug or unconfigured resources.

+ * We query the CRS object of the PCI root device to look for possible producer

+ * resources in the tree and cons...
[ message continues ]

It seems reasonable.

But if the resource obtained from the PCI0 _CRS method is incorrect, we

will get the incorrect pci_mem_start.
At the same time after the patch is applied, pci_mem_start will be

parsed in two different ways. If the result is different, maybe the

incorrect pci_mem_start will be used. 
Best regards.
--

Hi Yakui,
What do you mean by the PCI0 _CRS being incorrect ? Why would the BIOS

give a incorrect _CRS object ?

Also we don't just take the value given from the _CRS method, we still

read the e820_map to search for an unallocated resource. So even if

(by chance) the _CRS method returns incorrect value we would still
Yes pci_mem_start would be initialized in 2 different ways but we

still have to parse the e820_map the old way because there could be
Yeah, The result is different in my case. Though my BIOS reserves

hotpluggable memory region, kernel doesn't respect that right now and

just parses the e820_map to calculate the gap and pci_mem_start value.

I have explained the problem in this mail.
http://marc.info/?l=linux-acpi&m=121391675711763&w=2
Maybe nobody has seen this problem yet, because there are no systems

out there with less than 4GB memory to start with and which allow

memory hotplug.
But still i don't understand what do you mean by, we can get incorrect

pci_mem_start, in which case ?
Thanks,

--

In the patch the address obtained from the _CRS object will be passed

into the function of e820_search_gap. In such case maybe we will get the
In the function of setup_arch the pci_mem_start will be parsed by

searching the e820 table.  After the patch is applied, we will parse the

pci_mem_start again in the function of pci_acpi_scan_init and it will

override the value parsed in the function of setup_arch. If the

pci_mem_start is incorrect in the second case, maybe it will have side

effect.
Thanks.
--

Hi Yakui,
True..the whole idea behind doing this patch was to get a correct

(different) value for pci_mem_start.

We read the _CRS object over here to make sure that we assign the

pci_mem_start from the address range which is reserved by the BIOS for

PCI devices.
Also this reading of _CRS object would be done before we start

initializing the pci devices, i.e. before we start using the value of

pci_mem_start, so the original value assigned by pci_setup_gap is just

overwritten by this function. So that should be fine IMHO.

Also we would still want the call for e820_setup_gap because there can
Yes it will override. But how can the value be incorrect in the second

case. As explained in my previous mail we still parse the e820_map to

check if we have unclaimed resources between start_address (that read

from  _CRS) to 2^32.  So even if this start_address is wrong we would

catch that during parsing e820_map. But again why would the _CRS

return incorrect values, are you talking about errors in BIOS ?
Thanks,

Alok

--

The pci_mem_start is still gotten by parsing the E820 table.But the

input parameter start_addr will be used in the function of

e820_search_gap.

  If the following is the resource start address returned by the PCI0

_CRS object , maybe the different pci_mem_start will be gotten.

    0xE0000000

    0xE4000000
  At the same time if several start address is returned by the _CRS

object, the e820 table will be parsed several times. 
Thanks.
--

Yes we will parse e820 several times, but we don't initialize

pci_mem_start in every pass.

It will be initialized only twice once via the e820_setup_gap code path

and once via pci_setup_gap.

And i think you agree that both of these are required ?
During the gap calculation the previous code or the code now in

e820_setup_gap does this...

	calculates a gap in e820_map from 0 to 2^32.

	Initializes pci_mem_start.
And now with this patch, the code in pci_setup_gap

does this...

	for each _CRS under PCI0

		search gap from start_addr of _CRS to 2^32 *[1].

	Initialize pci_mem_start with the biggest gap that we could

	find.
Essentially, what we are doing is just limiting the gap calculation to a

smaller address space depending on the ACPI information we get.
Now then, what problem do you see with this approach ?
*[1]

While writing this mail i figured out that, instead of searching from

start_addr of _CRS to 2^32 we should just search till *end_addr* of _CRS resource.

I will send a patch to that effect. 
Thanks,
--

Yes. But maybe the pci_mem_start obtained in pci_setup_gap will be

different with that obtained in e820_setup_gap on some bogus BIOS.

If the pci_mem_start obtained in pci_setup_gap can meet the requirement,
--

Ok, so the whole problem crops up from what if the BIOS itself is bogus.

Ok in that case too we take proper care that pci_mem_start is within the

expected limits, i.e. we make sure that

1. pci_mem_start is not initialized to an already allocated resource

address and..

2. pci_mem_start is within the lower 32bit of address space. 
I assume you also agree that all the checks are in place.

Also it would be great if you could ACK both the patches as well.
Thanks for the review.
--

Ingo, Please also apply this patch to the tip/out-of-tree

--
Incremental patch . This patch passes the pci producer resources

end_addr (got from _CRS) to the e820_search_gap function.

Without this patch the code will always search from the producer regions

start_address to 2^32, even though the producer region may be smaller. 
Now , we limit the search only within the producer region's address

space.

Also we put a condition to check if the resource lies in the 32bit

address range. 
Signed-off-by: Alok N Kataria <akataria@vmware.com>
Index: linux-x86-tree.git/arch/x86/pci/acpi.c

===================================================================

--- linux-x86-tree.git.orig/arch/x86/pci/acpi.c	2008-06-25 10:11:35.000000000 -0700

+++ linux-x86-tree.git/arch/x86/pci/acpi.c	2008-06-25 11:17:31.000000000 -0700

@@ -121,13 +121,21 @@

 	struct acpi_resource_address64 addr;

 	acpi_status status;

 	struct gap_info *gap = data;

+	unsigned long long start_addr, end_addr;
 	status = resource_to_addr(resource, &addr);

 	if (ACPI_SUCCESS(status) &&

 	    addr.resource_type == ACPI_MEMORY_RANGE &&

 	    addr.address_length > gap->gapsize) {

-		e820_search_gap(&gap->gapstart, &gap->gapsize,

-				(addr.minimum + addr.translation_offset));

+		start_addr = addr.minimum + addr.translation_offset;

+		/*

+		 * We want space only in the 32bit address range

+		 */

+		if (start_addr < UINT_MAX) {

+			end_addr = start_addr + addr.address_length;

+			e820_search_gap(&gap->gapstart, &gap->gapsize,

+					start_addr, end_addr);

+		}

 	}
 	return AE_OK;
--