Skip to contentSkip to conversation

PCI mmconfig without ACPI

RFC PATCH In review Ed Swierk 4 participants 9 messages Feb 4, 2009 – May 5, 2009

Subsystems (4)
x86 ACPI PCI ACPI component architecture (acpica)
lore
Share & tools
lore

Ed Swierk

author
#0 lore raw

RFC PATCH 1/1 PCI mmconfig without ACPI

Make it possible to use memory-mapped PCI configuration space on systems with a supported PCI host bridge without CONFIG_ACPI.

The acpi_mcfg_allocation struct serves double duty, as a template for parsing the ACPI MCFG table and also to store the mmconfig data, which doesn't necessarily come from ACPI. Should I leave the struct in acpi/actbl1.h for ACPI parsing, and create a new one for storing mmconfig data?

---

arch/x86/Kconfig +2 -2
Index: linux-2.6.27.4/arch/x86/Kconfig
===================================================================
@@ -1599,7 +1599,7 @@ config PCI_DIRECT
 
 config PCI_MMCONFIG
 	def_bool y
-	depends on X86_32 && PCI && ACPI && (PCI_GOMMCONFIG || PCI_GOANY)
+	depends on X86_32 && PCI && (PCI_GOMMCONFIG || PCI_GOANY)
 
 config PCI_OLPC
 	def_bool y

@@ -1611,7 +1611,7 @@ config PCI_DOMAINS
 
 config PCI_MMCONFIG
 	bool "Support mmconfig PCI config space access"
-	depends on X86_64 && PCI && ACPI
+	depends on X86_64 && PCI
 
 config DMAR
 	bool "Support for DMA Remapping Devices (EXPERIMENTAL)"
arch/x86/kernel/acpi/boot.c +0 -4
Index: linux-2.6.27.4/arch/x86/kernel/acpi/boot.c
===================================================================
@@ -156,10 +156,6 @@ char *__init __acpi_map_table(unsigned l
 }
 
 #ifdef CONFIG_PCI_MMCONFIG
-/* The physical address of the MMCONFIG aperture.  Set from ACPI tables. */
-struct acpi_mcfg_allocation *pci_mmcfg_config;
-int pci_mmcfg_config_num;
-
 static int __init acpi_mcfg_oem_check(struct acpi_table_mcfg *mcfg)
 {
 	if (!strcmp(mcfg->header.oem_id, "SGI"))
arch/x86/pci/mmconfig-shared.c +10 -0
Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c
===================================================================
@@ -18,6 +18,10 @@
 
 #include "pci.h"
 
+/* The physical address of the MMCONFIG aperture.  Set from ACPI tables. */
+struct acpi_mcfg_allocation *pci_mmcfg_config;
+int pci_mmcfg_config_num;
+
 /* aperture is up to 256MB but BIOS may reserve less */
 #define MMCONFIG_APER_MIN	(2 * 1024*1024)
 #define MMCONFIG_APER_MAX	(256 * 1024*1024)

@@ -264,6 +268,8 @@ static void __init pci_mmcfg_insert_reso
 	pci_mmcfg_resources_inserted = 1;
 }
 
+#ifdef CONFIG_ACPI
+
 static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
 					      void *data)
 {

@@ -425,6 +431,8 @@ reject:
 	pci_mmcfg_config_num = 0;
 }
 
+#endif
+
 static int __initdata known_bridge;
 
 static void __init __pci_mmcfg_init(int early)

@@ -446,10 +454,12 @@ static void __init __pci_mmcfg_init(int 
 			known_bridge = 1;
 	}
 
+#ifdef CONFIG_ACPI
 	if (!known_bridge) {
 		acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
 		pci_mmcfg_reject_broken(early);
 	}
+#endif
 
 	if ((pci_mmcfg_config_num == 0) ||
 	    (pci_mmcfg_config == NULL) ||
include/acpi/actbl1.h +0 -10
Index: linux-2.6.27.4/include/acpi/actbl1.h
===================================================================
@@ -1045,16 +1045,6 @@ struct acpi_table_mcfg {
 	u8 reserved[8];
 };
 
-/* Subtable */
-
-struct acpi_mcfg_allocation {
-	u64 address;		/* Base address, processor-relative */
-	u16 pci_segment;	/* PCI segment group number */
-	u8 start_bus_number;	/* Starting PCI Bus number */
-	u8 end_bus_number;	/* Final PCI Bus number */
-	u32 reserved;
-};
-
 /*******************************************************************************
  *
  * SBST - Smart Battery Specification Table
include/linux/acpi.h +0 -3
Index: linux-2.6.27.4/include/linux/acpi.h
===================================================================
@@ -118,9 +118,6 @@ int acpi_unregister_ioapic(acpi_handle h
 void acpi_irq_stats_init(void);
 extern u32 acpi_irq_handled;
 
-extern struct acpi_mcfg_allocation *pci_mmcfg_config;
-extern int pci_mmcfg_config_num;
-
 extern int sbf_port;
 extern unsigned long acpi_realmode_flags;
arch/x86/pci/pci.h +15 -0
Index: linux-2.6.27.4/arch/x86/pci/pci.h
===================================================================
@@ -159,3 +159,18 @@ static inline void mmio_config_writel(vo
 {
 	asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
 }
+
+#ifdef CONFIG_PCI_MMCONFIG
+
+struct acpi_mcfg_allocation {
+	u64 address;		/* Base address, processor-relative */
+	u16 pci_segment;	/* PCI segment group number */
+	u8 start_bus_number;	/* Starting PCI Bus number */
+	u8 end_bus_number;	/* Final PCI Bus number */
+	u32 reserved;
+};
+
+extern struct acpi_mcfg_allocation *pci_mmcfg_config;
+extern int pci_mmcfg_config_num;
+
+#endif
#1 lore raw
Quoted 8 lines · Ed Swierk

Make it possible to use memory-mapped PCI configuration space on systems with a supported PCI host bridge without CONFIG_ACPI.

The acpi_mcfg_allocation struct serves double duty, as a template for parsing the ACPI MCFG table and also to store the mmconfig data, which doesn't necessarily come from ACPI. Should I leave the struct in acpi/actbl1.h for ACPI parsing, and create a new one for storing mmconfig data?

ok, that's certainly a nice cleanup and restructuring of this code.

A few comments:

arch/x86/Kconfig config PCI_DIRECT · line 1602

Quoted 4 lines

config PCI_MMCONFIG

arch/x86/Kconfig +1 -1
 	def_bool y
-	depends on X86_32 && PCI && ACPI && (PCI_GOMMCONFIG || PCI_GOANY)
+	depends on X86_32 && PCI && (PCI_GOMMCONFIG || PCI_GOANY)

( nice - increasing a PCI feature's reach and decoupling it from hardware
enumeration methods such as ACPI is always good news! )

arch/x86/pci/mmconfig-shared.c static void __init pci_mmcfg_insert_reso · line 274

Quoted 4 lines
arch/x86/pci/mmconfig-shared.c +2 -0
+#ifdef CONFIG_ACPI
+
 static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
 					      void *data)

An even cleaner approach would be to create a new file:

arch/x86/pci/mmconfig-acpi.c, and move this block of 5 functions there - and add a obj-$(CONFIG_ACPI) rule to arch/x86/pci/Makefile to build it.

The interfacing to arch/x86/pci/mmconfig-shared.c could be simplified too, instead of this two-pass thing:

        if (!known_bridge) {
                acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
                pci_mmcfg_reject_broken(early);
        }

a single:

	if (!known_bridge)
		pci_detect_acpi_mmcfg(early);

interface could be used. In the !CONFIG_ACPI case this interface would be an inline do-nothing wrapper, in a pci x86 header file:

static inline void pci_detect_acpi_mmcfg(int early) { }

A few currently local symbols in the file would have to be made explicit and moved into the header - but it should be rather straightforward i think.

That way we avoid the ugly #ifdef and clean up the general code structure and modularization a bit.

this #ifdef would go away as well:

Quoted 7 lines
arch/x86/pci/pci.h +2 -0
+#ifdef CONFIG_ACPI
 	if (!known_bridge) {
 		acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg);
 		pci_mmcfg_reject_broken(early);
 	}
+#endif

arch/x86/pci/pci.h static inline void mmio_config_writel(vo · line 171

Quoted 10 lines
arch/x86/pci/pci.h +9 -0
 
+#ifdef CONFIG_PCI_MMCONFIG
+
+struct acpi_mcfg_allocation {
+	u64 address;		/* Base address, processor-relative */
+	u16 pci_segment;	/* PCI segment group number */
+	u8 start_bus_number;	/* Starting PCI Bus number */
+	u8 end_bus_number;	/* Final PCI Bus number */
+	u32 reserved;
+};

Please rename this to "struct pci_mcfg_allocation" - there's nothing ACPI about it anymore - mmcfg is a PCI feature and ACPI is an enumeration method.

Also, while touching it, please also use the opportunity to align structure fields vertically:

struct pci_mcfg_allocation {
u64 address; /* Base address, processor-relative */
u16 pci_segment; /* PCI segment group number */
u8 start_bus_number; /* Starting PCI Bus number */
u8 end_bus_number; /* Final PCI Bus number */
u32 __reserved;
};

The whole layout of this structure becomes easier to read and nicer to look at as well.

Another small detail: note how i renamed reserved to __reserved - that is a standard way to de-emphasise the signficance of a structure field.

The reserved field there is for future expansion and to pad the structure to 16 bytes - it doesnt really mean much and the underscores move it a bit out of the default line of sight.

With a 'reserved' field people end up wondering whether it's perhaps some _semantic_ 'reserved area' kind of thing (like for e820 maps, etc.) - so it's never bad to make that distinction explicit via the double underscores.

But again, nice patch and it would be nice to see this concept hit mainline.

Ingo

#2 lore raw
Quoted 2 lines · Ingo Molnar

But again, nice patch and it would be nice to see this concept hit mainline.

Yeah, concept seems fine, and the cleanups you suggested sound good (splitting the files would make things clearer, and some of that stuff probably belongs in the ACPI or PNP core anyway).

My only concern is that enabling mmconfig w/o a reservation (as might happen if ACPI was disabled) may cause problems since it could overlap with other space. That was one of the issues we ran into when enabling it in the first place... But we've worked around it so far so I'm happy to give it a spin for the next cycle.

Thanks,

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
author
#3 lore raw
In reply to Ingo Molnar

Here's another attempt at decoupling the setup of memory-mapped PCI configuration space from ACPI. I implemented your suggestions for moving the ACPI-specific code to a separate file. I left the definition of struct acpi_mcfg_allocation alone, and created a new struct pci_mcfg_allocation. The former is now used only for parsing the actual ACPI MCFG table, while the latter is used to store information about mmcfg regions regardless of where they came from.

(This is still an RFC; the code is pretty much untested.)

---

arch/x86/Kconfig +6 -2
Index: linux-2.6.27.4/arch/x86/Kconfig
===================================================================
@@ -1599,7 +1599,7 @@ config PCI_DIRECT
 
 config PCI_MMCONFIG
 	def_bool y
-	depends on X86_32 && PCI && ACPI && (PCI_GOMMCONFIG || PCI_GOANY)
+	depends on X86_32 && PCI && (PCI_GOMMCONFIG || PCI_GOANY)
 
 config PCI_OLPC
 	def_bool y

@@ -1611,7 +1611,11 @@ config PCI_DOMAINS
 
 config PCI_MMCONFIG
 	bool "Support mmconfig PCI config space access"
-	depends on X86_64 && PCI && ACPI
+	depends on X86_64 && PCI
+
+config ACPI_MMCONFIG
+	def_bool y
+	depends on PCI_MMCONFIG && ACPI
 
 config DMAR
 	bool "Support for DMA Remapping Devices (EXPERIMENTAL)"
arch/x86/kernel/acpi/boot.c +27 -26

90 lines not on this page · raw message

arch/x86/pci/mmconfig-shared.c +13 -169

232 lines not on this page · raw message

include/linux/acpi.h +0 -3
Index: linux-2.6.27.4/include/linux/acpi.h
===================================================================
@@ -118,9 +118,6 @@ int acpi_unregister_ioapic(acpi_handle h
 void acpi_irq_stats_init(void);
 extern u32 acpi_irq_handled;
 
-extern struct acpi_mcfg_allocation *pci_mmcfg_config;
-extern int pci_mmcfg_config_num;
-
 extern int sbf_port;
 extern unsigned long acpi_realmode_flags;
arch/x86/pci/pci.h +20 -0
Index: linux-2.6.27.4/arch/x86/pci/pci.h
===================================================================
@@ -159,3 +159,23 @@ static inline void mmio_config_writel(vo
 {
 	asm volatile("movl %%eax,(%1)" :: "a" (val), "r" (pos) : "memory");
 }
+
+#ifdef CONFIG_PCI_MMCONFIG
+
+struct pci_mcfg_allocation {
+	u64 address;		/* Base address, processor-relative */
+	u16 pci_segment;	/* PCI segment group number */
+	u8 start_bus_number;	/* Starting PCI Bus number */
+	u8 end_bus_number;	/* Final PCI Bus number */
+};
+
+extern struct pci_mcfg_allocation *pci_mmcfg_config;
+extern int pci_mmcfg_config_num;
+
+#endif
+
+#ifdef CONFIG_ACPI_MMCONFIG
+extern void __init pci_detect_acpi_mmcfg(int early);
+#else
+inline static void __init pci_detect_acpi_mmcfg(int early) { }
+#endif
arch/x86/pci/mmconfig_32.c +1 -1
Index: linux-2.6.27.4/arch/x86/pci/mmconfig_32.c
===================================================================
@@ -27,7 +27,7 @@ static int mmcfg_last_accessed_cpu;
  */
 static u32 get_base_addr(unsigned int seg, int bus, unsigned devfn)
 {
-	struct acpi_mcfg_allocation *cfg;
+	struct pci_mcfg_allocation *cfg;
 	int cfg_num;
 
 	for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {
arch/x86/pci/mmconfig_64.c +3 -4
Index: linux-2.6.27.4/arch/x86/pci/mmconfig_64.c
===================================================================
@@ -7,7 +7,6 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
-#include <linux/acpi.h>
 #include <linux/bitmap.h>
 #include <asm/e820.h>
 

@@ -15,14 +14,14 @@
 
 /* Static virtual mapping of the MMCONFIG aperture */
 struct mmcfg_virt {
-	struct acpi_mcfg_allocation *cfg;
+	struct pci_mcfg_allocation *cfg;
 	char __iomem *virt;
 };
 static struct mmcfg_virt *pci_mmcfg_virt;
 
 static char __iomem *get_virt(unsigned int seg, unsigned bus)
 {
-	struct acpi_mcfg_allocation *cfg;
+	struct pci_mcfg_allocation *cfg;
 	int cfg_num;
 
 	for (cfg_num = 0; cfg_num < pci_mmcfg_config_num; cfg_num++) {

@@ -110,7 +109,7 @@ static struct pci_raw_ops pci_mmcfg = {
 	.write =	pci_mmcfg_write,
 };
 
-static void __iomem * __init mcfg_ioremap(struct acpi_mcfg_allocation *cfg)
+static void __iomem * __init mcfg_ioremap(struct pci_mcfg_allocation *cfg)
 {
 	void __iomem *addr;
 	u64 start, size;
arch/x86/pci/mmconfig-acpi.c new file +173 -0

173 lines not on this page · raw message

#4 lore raw
Quoted 10 lines · On Mon, 16 Feb 2009 11:32:21 -0800 Ed Swierk <[email protected]> wrote:

Here's another attempt at decoupling the setup of memory-mapped PCI configuration space from ACPI. I implemented your suggestions for moving the ACPI-specific code to a separate file. I left the definition of struct acpi_mcfg_allocation alone, and created a new struct pci_mcfg_allocation. The former is now used only for parsing the actual ACPI MCFG table, while the latter is used to store information about mmcfg regions regardless of where they came from.

(This is still an RFC; the code is pretty much untested.)

I'm still a bit dubious about this; does it solve a real issue? Or just remove the ACPI dependency for its own sake?

Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
author
#5 lore raw
Quoted 2 lines · Jesse Barnes

I'm still a bit dubious about this; does it solve a real issue?  Or just remove the ACPI dependency for its own sake?

Our products run Linux with a non-ACPI BIOS (Coreboot), so this is not just a theoretical issue.

--Ed
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw
Quoted 7 lines · On Tue, 5 May 2009 13:09:30 -0700 Ed Swierk <[email protected]> wrote:
Quoted 2 lines · Jesse Barnes

I'm still a bit dubious about this; does it solve a real issue?  Or just remove the ACPI dependency for its own sake?

Our products run Linux with a non-ACPI BIOS (Coreboot), so this is not just a theoretical issue.

Ah of course, Coreboot. The patch will need a lot of testing though; care to send me an updated version so we can get it into linux-next and have some people try it out?

Thanks,

Signature · 5 lines
Jesse Barnes, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
reviewer
#7 lore raw
In reply to Ingo Molnar
Quoted 31 lines

arch/x86/pci/pci.h static inline void mmio_config_writel(vo · line 171

Quoted 10 lines
arch/x86/pci/pci.h +9 -0
+#ifdef CONFIG_PCI_MMCONFIG
+
+struct acpi_mcfg_allocation {
+	u64 address;		/* Base address, processor-relative */
+	u16 pci_segment;	/* PCI segment group number */
+	u8 start_bus_number;	/* Starting PCI Bus number */
+	u8 end_bus_number;	/* Final PCI Bus number */
+	u32 reserved;
+};

Please rename this to "struct pci_mcfg_allocation" - there's nothing ACPI about it anymore - mmcfg is a PCI feature and ACPI is an enumeration method.

Also, while touching it, please also use the opportunity to align structure fields vertically:

struct pci_mcfg_allocation {
u64 address; /* Base address, processor-relative */
u16 pci_segment; /* PCI segment group number */
u8 start_bus_number; /* Starting PCI Bus number */
u8 end_bus_number; /* Final PCI Bus number */
u32 __reserved;
};

The whole layout of this structure becomes easier to read and nicer to look at as well.

Another small detail: note how i renamed reserved to __reserved - that is a standard way to de-emphasise the signficance of a structure field.

Quoted 7 lines

The reserved field there is for future expansion and to pad the structure to 16 bytes - it doesnt really mean much and the underscores move it a bit out of the default line of sight.

With a 'reserved' field people end up wondering whether it's perhaps some _semantic_ 'reserved area' kind of thing (like for e820 maps, etc.) - so it's never bad to make that distinction explicit via the double underscores.

struct acpi_mcfg_allocation is the structure that maps onto the MCFG ACPI table as defined in the PCI firmware spec and provided by the ACPI BIOS.

I'd like it to stay in actbl1.h -- as that is part of ACPICA, which tracks the standard tables. (and I see you did this with your updated patch, thanks.)

FWIW, "reserved" here really does have a specific definition. On read-only tables, such as this one, reserved fields are defined to return 0 on reads for this version of the table, but may return non-zero on future revisions.

thanks,
-Len

#8 lore raw
Quoted 43 lines · Len Brown
Quoted 31 lines

arch/x86/pci/pci.h static inline void mmio_config_writel(vo · line 171

Quoted 10 lines
arch/x86/pci/pci.h +9 -0
+#ifdef CONFIG_PCI_MMCONFIG
+
+struct acpi_mcfg_allocation {
+	u64 address;		/* Base address, processor-relative */
+	u16 pci_segment;	/* PCI segment group number */
+	u8 start_bus_number;	/* Starting PCI Bus number */
+	u8 end_bus_number;	/* Final PCI Bus number */
+	u32 reserved;
+};

Please rename this to "struct pci_mcfg_allocation" - there's nothing ACPI about it anymore - mmcfg is a PCI feature and ACPI is an enumeration method.

Also, while touching it, please also use the opportunity to align structure fields vertically:

struct pci_mcfg_allocation {
u64 address; /* Base address, processor-relative */
u16 pci_segment; /* PCI segment group number */
u8 start_bus_number; /* Starting PCI Bus number */
u8 end_bus_number; /* Final PCI Bus number */
u32 __reserved;
};

The whole layout of this structure becomes easier to read and nicer to look at as well.

Another small detail: note how i renamed reserved to __reserved - that is a standard way to de-emphasise the signficance of a structure field.

Quoted 7 lines

The reserved field there is for future expansion and to pad the structure to 16 bytes - it doesnt really mean much and the underscores move it a bit out of the default line of sight.

With a 'reserved' field people end up wondering whether it's perhaps some _semantic_ 'reserved area' kind of thing (like for e820 maps, etc.) - so it's never bad to make that distinction explicit via the double underscores.

struct acpi_mcfg_allocation is the structure that maps onto the MCFG ACPI table as defined in the PCI firmware spec and provided by the ACPI BIOS.

Quoted 8 lines

I'd like it to stay in actbl1.h -- as that is part of ACPICA, which tracks the standard tables. (and I see you did this with your updated patch, thanks.)

FWIW, "reserved" here really does have a specific definition. On read-only tables, such as this one, reserved fields are defined to return 0 on reads for this version of the table, but may return non-zero on future revisions.

of course - i did not want to suggest anything else.

Anything that the hardware accesses/provides is special and reserved in that sense.

My suggestion to rename to __reserved was to document this fact better and to make sure there's no higher-level 'reserved' concept controlled here.

Ingo