| Message ID | 1233766786.16414.26.camel@localhost.localdomain (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
* Ed Swierk <eswierk@aristanetworks.com> wrote: > 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: > 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) ( nice - increasing a PCI feature's reach and decoupling it from hardware enumeration methods such as ACPI is always good news! ) > +#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: > +#ifdef CONFIG_ACPI > if (!known_bridge) { > acpi_table_parse(ACPI_SIG_MCFG, acpi_parse_mcfg); > pci_mmcfg_reject_broken(early); > } > +#endif > +#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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 4, 2009 10:17 am Ingo Molnar wrote: > 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,
> > +#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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Len Brown <lenb@kernel.org> wrote: > > > > +#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. > > 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-2.6.27.4/arch/x86/Kconfig =================================================================== --- linux-2.6.27.4.orig/arch/x86/Kconfig +++ 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)" Index: linux-2.6.27.4/arch/x86/kernel/acpi/boot.c =================================================================== --- linux-2.6.27.4.orig/arch/x86/kernel/acpi/boot.c +++ 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")) Index: linux-2.6.27.4/arch/x86/pci/mmconfig-shared.c =================================================================== --- linux-2.6.27.4.orig/arch/x86/pci/mmconfig-shared.c +++ 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) || Index: linux-2.6.27.4/include/acpi/actbl1.h =================================================================== --- linux-2.6.27.4.orig/include/acpi/actbl1.h +++ 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 Index: linux-2.6.27.4/include/linux/acpi.h =================================================================== --- linux-2.6.27.4.orig/include/linux/acpi.h +++ 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; Index: linux-2.6.27.4/arch/x86/pci/pci.h =================================================================== --- linux-2.6.27.4.orig/arch/x86/pci/pci.h +++ 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