[PATCH 00/04] PCI: add proper MCFG support to let AMD boxes support MMCONFIG

[PATCH 00/04] PCI: add proper MCFG support to let AMD boxes support MMCONFIG

[Greg KH]

Here's an update of the patch that I sent out yesterday adding MCFG
table parsing to the PCI and ACPI code.  It is now 4 patches long and
should cover parsing and using the MCFG tables properly for MMCONFIG PCI
accesses for x86_64 and i386 boxes.

I've tested this series on my i386 boxes that have MCFG tables, and
cross-compiled it for x86_64, but not run it on that arch yet (will do
so tomorrow.)  Any testing that people might be able to do on AMD boxes
that have not had MMCONFIG PCI support yet would be greatly appreciated.

Comments appreciated.  These patches are now in my pci patch set and
should show up in the next -mm release.

thanks,

greg k-h

Re: [PATCH 01/04] PCI: add proper MCFG table parsing to ACPI core.

[Greg KH]

This patch is the first step in properly handling the MCFG PCI table.
It defines the structures properly, and saves off the table so that the
pci mmconfig code can access it.  It moves the parsing of the table a
little later in the boot process, but still before the information is
needed.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 arch/i386/kernel/acpi/boot.c |   41 +++++++++++++++++++++++++++++++++--------
 arch/i386/pci/mmconfig.c     |   12 +++++++-----
 arch/x86_64/pci/mmconfig.c   |   16 +++++++++-------
 include/linux/acpi.h         |   16 +++++++++++++---
 4 files changed, 62 insertions(+), 23 deletions(-)

--- gregkh-2.6.orig/arch/i386/kernel/acpi/boot.c	2005-06-14 21:14:31.000000000 -0700
+++ gregkh-2.6/arch/i386/kernel/acpi/boot.c	2005-06-14 21:16:46.000000000 -0700
@@ -158,9 +158,15 @@
 #endif
 
 #ifdef CONFIG_PCI_MMCONFIG
-static int __init acpi_parse_mcfg(unsigned long phys_addr, unsigned long size)
+/* The physical address of the MMCONFIG aperture.  Set from ACPI tables. */
+struct acpi_table_mcfg_config *pci_mmcfg_config;
+int pci_mmcfg_config_num;
+
+int __init acpi_parse_mcfg(unsigned long phys_addr, unsigned long size)
 {
 	struct acpi_table_mcfg *mcfg;
+	unsigned long i;
+	int config_size;
 
 	if (!phys_addr || !size)
 		return -EINVAL;
@@ -171,18 +177,38 @@
 		return -ENODEV;
 	}
 
-	if (mcfg->base_reserved) {
-		printk(KERN_ERR PREFIX "MMCONFIG not in low 4GB of memory\n");
+	/* how many config structures do we have */
+	pci_mmcfg_config_num = 0;
+	i = size - sizeof(struct acpi_table_mcfg);
+	while (i >= sizeof(struct acpi_table_mcfg_config)) {
+		++pci_mmcfg_config_num;
+		i -= sizeof(struct acpi_table_mcfg_config);
+	};
+	if (pci_mmcfg_config_num == 0) {
+		printk(KERN_ERR PREFIX "MMCONFIG has no entries\n");
 		return -ENODEV;
 	}
 
-	pci_mmcfg_base_addr = mcfg->base_address;
+	config_size = pci_mmcfg_config_num * sizeof(*pci_mmcfg_config);
+	pci_mmcfg_config = kmalloc(config_size, GFP_KERNEL);
+	if (!pci_mmcfg_config) {
+		printk(KERN_WARNING PREFIX
+		       "No memory for MCFG config tables\n");
+		return -ENOMEM;
+	}
+
+	memcpy(pci_mmcfg_config, &mcfg->config, config_size);
+	for (i = 0; i < pci_mmcfg_config_num; ++i) {
+		if (mcfg->config[i].base_reserved) {
+			printk(KERN_ERR PREFIX
+			       "MMCONFIG not in low 4GB of memory\n");
+			return -ENODEV;
+		}
+	}
 
 	return 0;
 }
-#else
-#define	acpi_parse_mcfg NULL
-#endif /* !CONFIG_PCI_MMCONFIG */
+#endif /* CONFIG_PCI_MMCONFIG */
 
 #ifdef CONFIG_X86_LOCAL_APIC
 static int __init
@@ -923,7 +949,6 @@
 	acpi_process_madt();
 
 	acpi_table_parse(ACPI_HPET, acpi_parse_hpet);
-	acpi_table_parse(ACPI_MCFG, acpi_parse_mcfg);
 
 	return 0;
 }
--- gregkh-2.6.orig/include/linux/acpi.h	2005-06-14 21:14:31.000000000 -0700
+++ gregkh-2.6/include/linux/acpi.h	2005-06-14 21:16:46.000000000 -0700
@@ -342,11 +342,19 @@
 
 /* PCI MMCONFIG */
 
+/* Defined in PCI Firmware Specification 3.0 */
+struct acpi_table_mcfg_config {
+	u32				base_address;
+	u32				base_reserved;
+	u16				pci_segment_group_number;
+	u8				start_bus_number;
+	u8				end_bus_number;
+	u8				reserved[4];
+} __attribute__ ((packed));
 struct acpi_table_mcfg {
 	struct acpi_table_header	header;
 	u8				reserved[8];
-	u32				base_address;
-	u32				base_reserved;
+	struct acpi_table_mcfg_config	config[0];
 } __attribute__ ((packed));
 
 /* Table Handlers */
@@ -391,6 +399,7 @@
 int acpi_get_table_header_early (enum acpi_table_id id, struct acpi_table_header **header);
 int acpi_table_parse_madt (enum acpi_madt_entry_id id, acpi_madt_entry_handler handler, unsigned int max_entries);
 int acpi_table_parse_srat (enum acpi_srat_entry_id id, acpi_madt_entry_handler handler, unsigned int max_entries);
+int acpi_parse_mcfg (unsigned long phys_addr, unsigned long size);
 void acpi_table_print (struct acpi_table_header *header, unsigned long phys_addr);
 void acpi_table_print_madt_entry (acpi_table_entry_header *madt);
 void acpi_table_print_srat_entry (acpi_table_entry_header *srat);
@@ -412,7 +421,8 @@
 
 extern int acpi_mp_config;
 
-extern u32 pci_mmcfg_base_addr;
+extern struct acpi_table_mcfg_config *pci_mmcfg_config;
+extern int pci_mmcfg_config_num;
 
 extern int sbf_port ;
 
--- gregkh-2.6.orig/arch/i386/pci/mmconfig.c	2005-06-14 21:14:31.000000000 -0700
+++ gregkh-2.6/arch/i386/pci/mmconfig.c	2005-06-14 21:53:11.000000000 -0700
@@ -11,11 +11,9 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/acpi.h>
 #include "pci.h"
 
-/* The physical address of the MMCONFIG aperture.  Set from ACPI tables. */
-u32 pci_mmcfg_base_addr;
-
 #define mmcfg_virt_addr ((void __iomem *) fix_to_virt(FIX_PCIE_MCFG))
 
 /* The base address of the last MMCONFIG device accessed */
@@ -27,7 +25,7 @@
 
 static inline void pci_exp_set_dev_base(int bus, int devfn)
 {
-	u32 dev_base = pci_mmcfg_base_addr | (bus << 20) | (devfn << 12);
+	u32 dev_base = pci_mmcfg_config[0].base_address | (bus << 20) | (devfn << 12);
 	if (dev_base != mmcfg_last_accessed_device) {
 		mmcfg_last_accessed_device = dev_base;
 		set_fixmap_nocache(FIX_PCIE_MCFG, dev_base);
@@ -101,7 +99,11 @@
 {
 	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
 		goto out;
-	if (!pci_mmcfg_base_addr)
+
+	acpi_table_parse(ACPI_MCFG, acpi_parse_mcfg);
+	if ((pci_mmcfg_config_num == 0) ||
+	    (pci_mmcfg_config == NULL) ||
+	    (pci_mmcfg_config[0].base_address == 0))
 		goto out;
 
 	/* Kludge for now. Don't use mmconfig on AMD systems because
--- gregkh-2.6.orig/arch/x86_64/pci/mmconfig.c	2005-06-14 21:14:31.000000000 -0700
+++ gregkh-2.6/arch/x86_64/pci/mmconfig.c	2005-06-14 21:53:30.000000000 -0700
@@ -7,15 +7,13 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/acpi.h>
 #include "pci.h"
 
 #define MMCONFIG_APER_SIZE (256*1024*1024)
 
-/* The physical address of the MMCONFIG aperture.  Set from ACPI tables. */
-u32 pci_mmcfg_base_addr;
-
 /* Static virtual mapping of the MMCONFIG aperture */
-char *pci_mmcfg_virt;
+static char *pci_mmcfg_virt;
 
 static inline char *pci_dev_base(unsigned int bus, unsigned int devfn)
 {
@@ -77,7 +75,11 @@
 {
 	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
 		return 0;
-	if (!pci_mmcfg_base_addr)
+
+	acpi_table_parse(ACPI_MCFG, acpi_parse_mcfg);
+	if ((pci_mmcfg_config_num == 0) ||
+	    (pci_mmcfg_config == NULL) ||
+	    (pci_mmcfg_config[0].base_address == 0))
 		return 0;
 
 	/* Kludge for now. Don't use mmconfig on AMD systems because
@@ -88,13 +90,13 @@
 		return 0; 
 
 	/* RED-PEN i386 doesn't do _nocache right now */
-	pci_mmcfg_virt = ioremap_nocache(pci_mmcfg_base_addr, MMCONFIG_APER_SIZE);
+	pci_mmcfg_virt = ioremap_nocache(pci_mmcfg_config[0].base_address, MMCONFIG_APER_SIZE);
 	if (!pci_mmcfg_virt) { 
 		printk("PCI: Cannot map mmconfig aperture\n");
 		return 0;
 	}	
 
-	printk(KERN_INFO "PCI: Using MMCONFIG at %x\n", pci_mmcfg_base_addr);
+	printk(KERN_INFO "PCI: Using MMCONFIG at %x\n", pci_mmcfg_config[0].base_address);
 	raw_pci_ops = &pci_mmcfg;
 	pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
 

[PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Greg KH]

Now that we have access to the whole MCFG table, let's properly use it
for all pci device accesses (as that's what it is there for, some boxes
don't put all the busses into one entry.)

If, for some reason, the table is incorrect, we fallback to the "old
style" of mmconfig accesses, namely, we just assume the first entry in
the table is the one for us, and blindly use it.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 arch/i386/pci/mmconfig.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

--- gregkh-2.6.orig/arch/i386/pci/mmconfig.c	2005-06-14 21:53:11.000000000 -0700
+++ gregkh-2.6/arch/i386/pci/mmconfig.c	2005-06-14 22:07:52.000000000 -0700
@@ -22,10 +22,31 @@
 /*
  * Functions for accessing PCI configuration space with MMCONFIG accesses
  */
+static u32 get_base_addr(unsigned int seg, int bus)
+{
+	int cfg_num = -1;
+	struct acpi_table_mcfg_config *cfg;
+
+	while (1) {
+		++cfg_num;
+		if (cfg_num >= pci_mmcfg_config_num) {
+			/* something bad is going on, no cfg table is found. */
+			/* so we fall back to the old way we used to do this */
+			/* and just rely on the first entry to be correct. */
+			return pci_mmcfg_config[0].base_address;
+		}
+		cfg = &pci_mmcfg_config[cfg_num];
+		if (cfg->pci_segment_group_number != seg)
+			continue;
+		if ((cfg->start_bus_number <= bus) &&
+		    (cfg->end_bus_number >= bus))
+			return cfg->base_address;
+	}
+}
 
-static inline void pci_exp_set_dev_base(int bus, int devfn)
+static inline void pci_exp_set_dev_base(unsigned int seg, int bus, int devfn)
 {
-	u32 dev_base = pci_mmcfg_config[0].base_address | (bus << 20) | (devfn << 12);
+	u32 dev_base = get_base_addr(seg, bus) | (bus << 20) | (devfn << 12);
 	if (dev_base != mmcfg_last_accessed_device) {
 		mmcfg_last_accessed_device = dev_base;
 		set_fixmap_nocache(FIX_PCIE_MCFG, dev_base);
@@ -42,7 +63,7 @@
 
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	pci_exp_set_dev_base(bus, devfn);
+	pci_exp_set_dev_base(seg, bus, devfn);
 
 	switch (len) {
 	case 1:
@@ -71,7 +92,7 @@
 
 	spin_lock_irqsave(&pci_config_lock, flags);
 
-	pci_exp_set_dev_base(bus, devfn);
+	pci_exp_set_dev_base(seg, bus, devfn);
 
 	switch (len) {
 	case 1:

[PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Greg KH]

Now that we have access to the whole MCFG table, let's properly use it
for all pci device accesses (as that's what it is there for, some boxes
don't put all the busses into one entry.)

If, for some reason, the table is incorrect, we fallback to the "old
style" of mmconfig accesses, namely, we just assume the first entry in
the table is the one for us, and blindly use it.

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 arch/x86_64/pci/mmconfig.c |   58 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 48 insertions(+), 10 deletions(-)

--- gregkh-2.6.orig/arch/x86_64/pci/mmconfig.c	2005-06-14 22:06:17.000000000 -0700
+++ gregkh-2.6/arch/x86_64/pci/mmconfig.c	2005-06-14 22:08:44.000000000 -0700
@@ -13,17 +13,44 @@
 #define MMCONFIG_APER_SIZE (256*1024*1024)
 
 /* Static virtual mapping of the MMCONFIG aperture */
-static char *pci_mmcfg_virt;
+struct mmcfg_virt {
+	struct acpi_table_mcfg_config *cfg;
+	char *virt;
+};
+static struct mmcfg_virt *pci_mmcfg_virt;
+
+static char *get_virt(unsigned int seg, int bus)
+{
+	int cfg_num = -1;
+	struct acpi_table_mcfg_config *cfg;
+
+	while (1) {
+		++cfg_num;
+		if (cfg_num >= pci_mmcfg_config_num) {
+			/* something bad is going on, no cfg table is found. */
+			/* so we fall back to the old way we used to do this */
+			/* and just rely on the first entry to be correct. */
+			return pci_mmcfg_virt[0].virt;
+		}
+		cfg = pci_mmcfg_virt[cfg_num].cfg;
+		if (cfg->pci_segment_group_number != seg)
+			continue;
+		if ((cfg->start_bus_number <= bus) &&
+		    (cfg->end_bus_number >= bus))
+			return pci_mmcfg_virt[cfg_num].virt;
+	}
+}
 
-static inline char *pci_dev_base(unsigned int bus, unsigned int devfn)
+static inline char *pci_dev_base(unsigned int seg, unsigned int bus, unsigned int devfn)
 {
-	return pci_mmcfg_virt + ((bus << 20) | (devfn << 12));
+
+	return get_virt(seg, bus) + ((bus << 20) | (devfn << 12));
 }
 
 static int pci_mmcfg_read(unsigned int seg, unsigned int bus,
 			  unsigned int devfn, int reg, int len, u32 *value)
 {
-	char *addr = pci_dev_base(bus, devfn); 
+	char *addr = pci_dev_base(seg, bus, devfn);
 
 	if (unlikely(!value || (bus > 255) || (devfn > 255) || (reg > 4095)))
 		return -EINVAL;
@@ -46,7 +73,7 @@
 static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 			   unsigned int devfn, int reg, int len, u32 value)
 {
-	char *addr = pci_dev_base(bus,devfn);
+	char *addr = pci_dev_base(seg, bus, devfn);
 
 	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
 		return -EINVAL;
@@ -73,6 +100,8 @@
 
 static int __init pci_mmcfg_init(void)
 {
+	int i;
+
 	if ((pci_probe & PCI_PROBE_MMCONF) == 0)
 		return 0;
 
@@ -90,13 +119,22 @@
 		return 0; 
 
 	/* RED-PEN i386 doesn't do _nocache right now */
-	pci_mmcfg_virt = ioremap_nocache(pci_mmcfg_config[0].base_address, MMCONFIG_APER_SIZE);
-	if (!pci_mmcfg_virt) { 
-		printk("PCI: Cannot map mmconfig aperture\n");
+	pci_mmcfg_virt = kmalloc(sizeof(*pci_mmcfg_virt) * pci_mmcfg_config_num, GFP_KERNEL);
+	if (pci_mmcfg_virt == NULL) {
+		printk("PCI: Can not allocate memory for mmconfig structures\n");
 		return 0;
-	}	
+	}
+	for (i = 0; i < pci_mmcfg_config_num; ++i) {
+		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
+		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);
+		if (!pci_mmcfg_virt[i].virt) {
+			printk("PCI: Cannot map mmconfig aperture for segment %d\n",
+			       pci_mmcfg_config[i].pci_segment_group_number);
+			return 0;
+		}
+		printk(KERN_INFO "PCI: Using MMCONFIG at %x\n", pci_mmcfg_config[i].base_address);
+	}
 
-	printk(KERN_INFO "PCI: Using MMCONFIG at %x\n", pci_mmcfg_config[0].base_address);
 	raw_pci_ops = &pci_mmcfg;
 	pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
 

[PATCH 04/04] PCI: let AMD boxes use MMCONFIG

[Greg KH]

Now that we parse and handle the MCFG table properly, we can hopefully
remove the AMD check and let those boxes use MMCONFIG.

Note, this needs _lots_ of testing on lots of boxes before going to
mainline...

Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


---
 arch/i386/pci/mmconfig.c   |    7 -------
 arch/x86_64/pci/mmconfig.c |    7 -------
 2 files changed, 14 deletions(-)

--- gregkh-2.6.orig/arch/i386/pci/mmconfig.c	2005-06-14 22:07:52.000000000 -0700
+++ gregkh-2.6/arch/i386/pci/mmconfig.c	2005-06-14 22:09:16.000000000 -0700
@@ -127,13 +127,6 @@
 	    (pci_mmcfg_config[0].base_address == 0))
 		goto out;
 
-	/* Kludge for now. Don't use mmconfig on AMD systems because
-	   those have some busses where mmconfig doesn't work,
-	   and we don't parse ACPI MCFG well enough to handle that. 
-	   Remove when proper handling is added. */
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
-		goto out; 
-
 	printk(KERN_INFO "PCI: Using MMCONFIG\n");
 	raw_pci_ops = &pci_mmcfg;
 	pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
--- gregkh-2.6.orig/arch/x86_64/pci/mmconfig.c	2005-06-14 22:08:44.000000000 -0700
+++ gregkh-2.6/arch/x86_64/pci/mmconfig.c	2005-06-14 22:09:11.000000000 -0700
@@ -111,13 +111,6 @@
 	    (pci_mmcfg_config[0].base_address == 0))
 		return 0;
 
-	/* Kludge for now. Don't use mmconfig on AMD systems because
-	   those have some busses where mmconfig doesn't work,
-	   and we don't parse ACPI MCFG well enough to handle that. 
-	   Remove when proper handling is added. */
-	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
-		return 0; 
-
 	/* RED-PEN i386 doesn't do _nocache right now */
 	pci_mmcfg_virt = kmalloc(sizeof(*pci_mmcfg_virt) * pci_mmcfg_config_num, GFP_KERNEL);
 	if (pci_mmcfg_virt == NULL) {

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Andi Kleen]

On Tue, Jun 14, 2005 at 10:31:20PM -0700, Greg KH wrote:
> Now that we have access to the whole MCFG table, let's properly use it
> for all pci device accesses (as that's what it is there for, some boxes
> don't put all the busses into one entry.)
> 
> If, for some reason, the table is incorrect, we fallback to the "old
> style" of mmconfig accesses, namely, we just assume the first entry in
> the table is the one for us, and blindly use it.

I think it would be better to set different bus->ops at probe
time, not walk the table at runtime.

-Andi

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Jeff Garzik]

Andi Kleen wrote:
> On Tue, Jun 14, 2005 at 10:31:20PM -0700, Greg KH wrote:
> 
>>Now that we have access to the whole MCFG table, let's properly use it
>>for all pci device accesses (as that's what it is there for, some boxes
>>don't put all the busses into one entry.)
>>
>>If, for some reason, the table is incorrect, we fallback to the "old
>>style" of mmconfig accesses, namely, we just assume the first entry in
>>the table is the one for us, and blindly use it.
> 
> 
> I think it would be better to set different bus->ops at probe
> time, not walk the table at runtime.

Makes sense to me...

	Jeff

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Greg KH]

On Wed, Jun 15, 2005 at 11:48:33AM +0200, Andi Kleen wrote:
> On Tue, Jun 14, 2005 at 10:31:20PM -0700, Greg KH wrote:
> > Now that we have access to the whole MCFG table, let's properly use it
> > for all pci device accesses (as that's what it is there for, some boxes
> > don't put all the busses into one entry.)
> > 
> > If, for some reason, the table is incorrect, we fallback to the "old
> > style" of mmconfig accesses, namely, we just assume the first entry in
> > the table is the one for us, and blindly use it.
> 
> I think it would be better to set different bus->ops at probe
> time, not walk the table at runtime.

Yeah, I thought of that, but it's the same ops pointers that we want to
have called for the different devices.  The only thing different is the
base address of the bus.

In sleeping on it, I thought about just using the void * we have
availble for the bus to use to hold this base address, that way we only
have to look it up at bus creation time, not for every device access.

Of course to do that I might need another callback in the ops structure,
but hey, what's one more pointer :)

Sound a bit more reasonable?  I'll try to prototype this later tonight.

thanks,

greg k-h

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Andi Kleen]

On Wed, Jun 15, 2005 at 10:54:47AM -0700, Greg KH wrote:
> On Wed, Jun 15, 2005 at 11:48:33AM +0200, Andi Kleen wrote:
> > On Tue, Jun 14, 2005 at 10:31:20PM -0700, Greg KH wrote:
> > > Now that we have access to the whole MCFG table, let's properly use it
> > > for all pci device accesses (as that's what it is there for, some boxes
> > > don't put all the busses into one entry.)
> > > 
> > > If, for some reason, the table is incorrect, we fallback to the "old
> > > style" of mmconfig accesses, namely, we just assume the first entry in
> > > the table is the one for us, and blindly use it.
> > 
> > I think it would be better to set different bus->ops at probe
> > time, not walk the table at runtime.
> 
> Yeah, I thought of that, but it's the same ops pointers that we want to
> have called for the different devices.  The only thing different is the
> base address of the bus.

There can be bus segments that don't support it at all and still
need the old port based access method
(e.g. on a AMD K8 box the busses of the internal northbridge need this) 

For those you would have bus->ops pointing to the old port code,
for the others to the mmconfig code.

That's the whole point of the patch btw - to support mmconfig even
on these machines ;-) 

> 
> In sleeping on it, I thought about just using the void * we have
> availble for the bus to use to hold this base address, that way we only
> have to look it up at bus creation time, not for every device access.
> 
> Of course to do that I might need another callback in the ops structure,
> but hey, what's one more pointer :)
> 
> Sound a bit more reasonable?  I'll try to prototype this later tonight.

Yes, caching the base address would be a good idea too.

-Andi

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Greg KH]

On Wed, Jun 15, 2005 at 08:23:47PM +0200, Andi Kleen wrote:
> On Wed, Jun 15, 2005 at 10:54:47AM -0700, Greg KH wrote:
> > On Wed, Jun 15, 2005 at 11:48:33AM +0200, Andi Kleen wrote:
> > > On Tue, Jun 14, 2005 at 10:31:20PM -0700, Greg KH wrote:
> > > > Now that we have access to the whole MCFG table, let's properly use it
> > > > for all pci device accesses (as that's what it is there for, some boxes
> > > > don't put all the busses into one entry.)
> > > > 
> > > > If, for some reason, the table is incorrect, we fallback to the "old
> > > > style" of mmconfig accesses, namely, we just assume the first entry in
> > > > the table is the one for us, and blindly use it.
> > > 
> > > I think it would be better to set different bus->ops at probe
> > > time, not walk the table at runtime.
> > 
> > Yeah, I thought of that, but it's the same ops pointers that we want to
> > have called for the different devices.  The only thing different is the
> > base address of the bus.
> 
> There can be bus segments that don't support it at all and still
> need the old port based access method
> (e.g. on a AMD K8 box the busses of the internal northbridge need this) 

Ugh, I was afraid of that :(

> For those you would have bus->ops pointing to the old port code,
> for the others to the mmconfig code.

Well, for that, I'll have to set the bus ops when they are discovered.
So the same callback I mentioned can be used for that (due to the need
to check the ranges in the MCFG table).  I'll work on that too.

Does the K8 box just not have MCFG entries for the northbridge busses?

> That's the whole point of the patch btw - to support mmconfig even
> on these machines ;-) 

Ah, I thought it was to also actually support the MCFG table for boxes
that supported the different mmconfig addresses.

> > In sleeping on it, I thought about just using the void * we have
> > availble for the bus to use to hold this base address, that way we only
> > have to look it up at bus creation time, not for every device access.
> > 
> > Of course to do that I might need another callback in the ops structure,
> > but hey, what's one more pointer :)
> > 
> > Sound a bit more reasonable?  I'll try to prototype this later tonight.
> 
> Yes, caching the base address would be a good idea too.

Ok, I'll work on this and the above mentioned stuff too.

thanks,

greg k-h

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Andi Kleen]

> > For those you would have bus->ops pointing to the old port code,
> > for the others to the mmconfig code.
> 
> Well, for that, I'll have to set the bus ops when they are discovered.
> So the same callback I mentioned can be used for that (due to the need
> to check the ranges in the MCFG table).  I'll work on that too.
> 
> Does the K8 box just not have MCFG entries for the northbridge busses?

I believe so. 

-Andi

Re: [PATCH 02/04] PCI: use the MCFG table to properly access pci devices (i386)

[Greg KH]

On Wed, Jun 15, 2005 at 09:03:38PM +0200, Andi Kleen wrote:
> > > For those you would have bus->ops pointing to the old port code,
> > > for the others to the mmconfig code.
> > 
> > Well, for that, I'll have to set the bus ops when they are discovered.
> > So the same callback I mentioned can be used for that (due to the need
> > to check the ranges in the MCFG table).  I'll work on that too.
> > 
> > Does the K8 box just not have MCFG entries for the northbridge busses?
> 
> I believe so. 

Ok, that give us a fighting chance to get this to work...

thanks,

greg k-h

Re: [PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Rajesh Shah]

On Tue, Jun 14, 2005 at 10:32:14PM -0700, Greg KH wrote:
> 
> +	for (i = 0; i < pci_mmcfg_config_num; ++i) {
> +		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
> +		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);

This will map 256MB for each mmcfg aperture, probably better
to restrict it based on bus number range for this aperture.

Since you are reworking the patches, I will wait for the
next version and try that out on a couple of Intel x86_64
boxes here.

Rajesh

Re: [PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Greg KH]

On Thu, Jun 16, 2005 at 03:34:06PM -0700, Rajesh Shah wrote:
> On Tue, Jun 14, 2005 at 10:32:14PM -0700, Greg KH wrote:
> > 
> > +	for (i = 0; i < pci_mmcfg_config_num; ++i) {
> > +		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
> > +		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);
> 
> This will map 256MB for each mmcfg aperture, probably better
> to restrict it based on bus number range for this aperture.

It should be 1MB per bus number, right?

thanks,

greg k-h

Re: [PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Andi Kleen]

On Thu, Jun 16, 2005 at 03:42:23PM -0700, Greg KH wrote:
> On Thu, Jun 16, 2005 at 03:34:06PM -0700, Rajesh Shah wrote:
> > On Tue, Jun 14, 2005 at 10:32:14PM -0700, Greg KH wrote:
> > > 
> > > +	for (i = 0; i < pci_mmcfg_config_num; ++i) {
> > > +		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
> > > +		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);
> > 
> > This will map 256MB for each mmcfg aperture, probably better
> > to restrict it based on bus number range for this aperture.
> 
> It should be 1MB per bus number, right?

It shouldn't make much difference anyways - we have plenty of vmalloc
space on x86-64

The only advantage of using the exact number would be maybe to detect overruns.

-Andi

Re: [PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Brian Gerst]

Andi Kleen wrote:
> On Thu, Jun 16, 2005 at 03:42:23PM -0700, Greg KH wrote:
> 
>>On Thu, Jun 16, 2005 at 03:34:06PM -0700, Rajesh Shah wrote:
>>
>>>On Tue, Jun 14, 2005 at 10:32:14PM -0700, Greg KH wrote:
>>>
>>>>+	for (i = 0; i < pci_mmcfg_config_num; ++i) {
>>>>+		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
>>>>+		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);
>>>
>>>This will map 256MB for each mmcfg aperture, probably better
>>>to restrict it based on bus number range for this aperture.
>>
>>It should be 1MB per bus number, right?
> 
> 
> It shouldn't make much difference anyways - we have plenty of vmalloc
> space on x86-64

What about excess page table usage?

--
				Brian Gerst

Re: [PATCH 03/04] PCI: use the MCFG table to properly access pci devices (x86-64)

[Rajesh Shah]

On Fri, Jun 17, 2005 at 01:00:20AM +0200, Andi Kleen wrote:
> On Thu, Jun 16, 2005 at 03:42:23PM -0700, Greg KH wrote:
> > On Thu, Jun 16, 2005 at 03:34:06PM -0700, Rajesh Shah wrote:
> > > On Tue, Jun 14, 2005 at 10:32:14PM -0700, Greg KH wrote:
> > > > 
> > > > +	for (i = 0; i < pci_mmcfg_config_num; ++i) {
> > > > +		pci_mmcfg_virt[i].cfg = &pci_mmcfg_config[i];
> > > > +		pci_mmcfg_virt[i].virt = ioremap_nocache(pci_mmcfg_config[i].base_address, MMCONFIG_APER_SIZE);
> > > 
> > > This will map 256MB for each mmcfg aperture, probably better
> > > to restrict it based on bus number range for this aperture.
> > 
> > It should be 1MB per bus number, right?

Yes.

> 
> It shouldn't make much difference anyways - we have plenty of vmalloc
> space on x86-64
> 
Depends on how much you trust firmware :-). In the worst case, it
could create one such entry for each bus.

I noticed this because I worked on picking up resources for root
bridges from firmware recently, and noticed more than one system
in which firmware created ~10 entries for resources that were
adjacent to each other and could be coalesced into a single
resource range. Not illegal, just not optimal.

Rajesh