On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:

From: Jayachandran C jchandra@broadcom.com

Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is to share the API and code with ARM64 later. The corresponding declarations are moved from asm/pci_x86.h to linux/pci-acpi.h

As a part of this we introduce three functions that can be implemented by the arch code: pci_mmconfig_map_resource() to map a mcfg entry, pci_mmconfig_unmap_resource to do the corresponding unmap and pci_mmconfig_enabled to see if the arch setup of mcfg entries was successful. We also provide weak implementations of these, which will be used from ARM64. On x86, we retain the old logic by providing platform specific implementation.

This patch is purely rearranging code, it should not have any impact on the logic of MCFG parsing or list handling.

Signed-off-by: Jayachandran C jchandra@broadcom.com [Xen parts:] Acked-by: David Vrabel david.vrabel@citrix.com

---

arch/x86/include/asm/pci_x86.h | 24 +--- arch/x86/pci/mmconfig-shared.c | 269 +++++------------------------------ arch/x86/pci/mmconfig_32.c | 1 + arch/x86/pci/mmconfig_64.c | 1 + arch/x86/pci/numachip.c | 1 + drivers/acpi/Makefile | 1 + drivers/acpi/pci_mcfg.c | 312 +++++++++++++++++++++++++++++++++++++++++ drivers/xen/pci.c | 5 +- include/linux/pci-acpi.h | 33 +++++ 9 files changed, 386 insertions(+), 261 deletions(-) create mode 100644 drivers/acpi/pci_mcfg.c

This patch makes perfect sense to me and manages to move MCFG handling to common code in a seamless manner, it is basically a code move with weak functions to cater for X86 specific legacy bits which are otherwise pretty complex to untangle, so (apart from a few nits below):

Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

[...]

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c new file mode 100644 index 0000000..ea84365 --- /dev/null +++ b/drivers/acpi/pci_mcfg.c @@ -0,0 +1,312 @@ +/*

• • pci_mcfg.c
• • Common code to maintain the MCFG areas and mappings
• • This has been extracted from arch/x86/pci/mmconfig-shared.c
• • and moved here so that other architectures can use this code.
• */

+#include <linux/pci.h> +#include <linux/init.h> +#include <linux/dmi.h> +#include <linux/pci-acpi.h> +#include <linux/sfi_acpi.h> +#include <linux/slab.h> +#include <linux/mutex.h> +#include <linux/rculist.h>

Nit: while at it order them alphabetically.

+#define PREFIX "ACPI: "

+static DEFINE_MUTEX(pci_mmcfg_lock); +LIST_HEAD(pci_mmcfg_list);

+static void list_add_sorted(struct pci_mmcfg_region *new) +{

• struct pci_mmcfg_region *cfg;
• /* keep list sorted by segment and starting bus number */
• list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {

• if (cfg->segment > new->segment ||
  

•     (cfg->segment == new->segment &&
  

•      cfg->start_bus >= new->start_bus)) {
  

• 	list_add_tail_rcu(&new->list, &cfg->list);
  

• 	return;
  

• }
  

• }
• list_add_tail_rcu(&new->list, &pci_mmcfg_list);

+}

+static struct pci_mmcfg_region *pci_mmconfig_alloc(int segment, int start,

• 				   int end, u64 addr)
  

+{

• struct pci_mmcfg_region *new;
• struct resource *res;
• if (addr == 0)

• return NULL;
  

• new = kzalloc(sizeof(*new), GFP_KERNEL);
• if (!new)

• return NULL;
  

• new->address = addr;
• new->segment = segment;
• new->start_bus = start;
• new->end_bus = end;
• res = &new->res;
• res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
• res->end = addr + PCI_MMCFG_BUS_OFFSET(end + 1) - 1;
• res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
• snprintf(new->name, PCI_MMCFG_RESOURCE_NAME_LEN,

•  "PCI MMCONFIG %04x [bus %02x-%02x]", segment, start, end);
  

• res->name = new->name;
• return new;

+}

+struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

• 					int end, u64 addr)
  

+{

• struct pci_mmcfg_region *new;
• new = pci_mmconfig_alloc(segment, start, end, addr);
• if (new) {

• mutex_lock(&pci_mmcfg_lock);
  

• list_add_sorted(new);
  

• mutex_unlock(&pci_mmcfg_lock);
  

• pr_info(PREFIX
  

•        "MMCONFIG for domain %04x [bus %02x-%02x] at %pR "
  

•        "(base %#lx)\n",
  

•        segment, start, end, &new->res, (unsigned long)addr);
  

• }
• return new;

+}

+struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus) +{

• struct pci_mmcfg_region *cfg;
• list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)

• if (cfg->segment == segment &&
  

•     cfg->start_bus <= bus && bus <= cfg->end_bus)
  

• 	return cfg;
  

• return NULL;

+}

+/*

• • Map a pci_mmcfg_region, can be overrriden by arch

s/overrriden/overridden/

[...]

+static int __init acpi_mcfg_check_entry(struct acpi_table_mcfg *mcfg,

• 			struct acpi_mcfg_allocation *cfg)
  

+{

• int year;
• if (!config_enabled(CONFIG_X86))

• return 0;
  

This check in generic code may ruffle someone's feathers, I even think we can run this function safely on ARM64 but to prevent surprises we'd better keep the X86 check, alternatives like adding a weak function just for a quirk do not make much sense to me.

Lorenzo

• if (cfg->address < 0xFFFFFFFF)

• return 0;
  

• if (!strncmp(mcfg->header.oem_id, "SGI", 3))

• return 0;
  

• if (mcfg->header.revision >= 1) {

• if (dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL) &&
  

•     year >= 2010)
  

• 	return 0;
  

• }
• pr_err(PREFIX "MCFG region for %04x [bus %02x-%02x] at %#llx "

•       "is above 4GB, ignored\n", cfg->pci_segment,
  

•       cfg->start_bus_number, cfg->end_bus_number, cfg->address);
  

• return -EINVAL;

+}

+static int __init pci_parse_mcfg(struct acpi_table_header *header) +{

• struct acpi_table_mcfg *mcfg;
• struct acpi_mcfg_allocation *cfg_table, *cfg;
• unsigned long i;
• int entries;
• if (!header)

• return -EINVAL;
  

• mcfg = (struct acpi_table_mcfg *)header;
• /* how many config structures do we have */
• entries = 0;
• i = header->length - sizeof(struct acpi_table_mcfg);
• while (i >= sizeof(struct acpi_mcfg_allocation)) {

• entries++;
  

• i -= sizeof(struct acpi_mcfg_allocation);
  

• }
• if (entries == 0) {

• pr_err(PREFIX "MMCONFIG has no entries\n");
  

• return -ENODEV;
  

• }
• cfg_table = (struct acpi_mcfg_allocation *) &mcfg[1];
• for (i = 0; i < entries; i++) {

• cfg = &cfg_table[i];
  

• if (acpi_mcfg_check_entry(mcfg, cfg))
  

• 	return -ENODEV;
  

• if (pci_mmconfig_add(cfg->pci_segment, cfg->start_bus_number,
  

• 		   cfg->end_bus_number, cfg->address) == NULL) {
  

• 	pr_warn(PREFIX "no memory for MCFG entries\n");
  

• 	return -ENOMEM;
  

• }
  

• }
• return 0;

+}

+int __init pci_mmconfig_parse_table(void) +{

• return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);

+}

+void __weak __init pci_mmcfg_late_init(void) +{

• int err, n = 0;
• struct pci_mmcfg_region *cfg;
• err = pci_mmconfig_parse_table();
• if (err) {

• pr_err(PREFIX " Failed to parse MCFG (%d)\n", err);
  

• return;
  

• }
• list_for_each_entry(cfg, &pci_mmcfg_list, list) {

• pci_mmconfig_map_resource(NULL, cfg);
  

• n++;
  

• }
• pr_info(PREFIX " MCFG table loaded %d entries\n", n);

+} diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c index 7494dbe..97aa9d3 100644 --- a/drivers/xen/pci.c +++ b/drivers/xen/pci.c @@ -27,9 +27,6 @@ #include <asm/xen/hypervisor.h> #include <asm/xen/hypercall.h> #include "../pci/pci.h" -#ifdef CONFIG_PCI_MMCONFIG -#include <asm/pci_x86.h> -#endif static bool __read_mostly pci_seg_supported = true; @@ -221,7 +218,7 @@ static int __init xen_mcfg_late(void) if (!xen_initial_domain()) return 0;

• if ((pci_probe & PCI_PROBE_MMCONF) == 0)

• if (!pci_mmconfig_enabled()) return 0;

if (list_empty(&pci_mmcfg_list)) diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..e9450ef 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; #define RESET_DELAY_DSM 0x08 #define FUNCTION_DELAY_DSM 0x09 +/* common API to maintain list of MCFG regions */ +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)

+struct pci_mmcfg_region {

• struct list_head list;
• struct resource res;
• u64 address;
• char __iomem *virt;
• u16 segment;
• u8 start_bus;
• u8 end_bus;
• char name[PCI_MMCFG_RESOURCE_NAME_LEN];

+};

+extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,

• 	       phys_addr_t addr);
  

+extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);

+extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus); +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

• 					int end, u64 addr);
  

+extern int pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg);

+extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region *mcfg); +extern int pci_mmconfig_enabled(void); +extern int __init pci_mmconfig_parse_table(void);

+extern struct list_head pci_mmcfg_list;

+#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12)

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { } -- 1.9.1

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas helgaas@kernel.org wrote:

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is to share the API and code with ARM64 later. The corresponding declarations are moved from asm/pci_x86.h to linux/pci-acpi.h

As a part of this we introduce three functions that can be implemented by the arch code: pci_mmconfig_map_resource() to map a mcfg entry, pci_mmconfig_unmap_resource to do the corresponding unmap and pci_mmconfig_enabled to see if the arch setup of mcfg entries was successful. We also provide weak implementations of these, which will be used from ARM64. On x86, we retain the old logic by providing platform specific implementation.

This patch is purely rearranging code, it should not have any impact on the logic of MCFG parsing or list handling.

I definitely want to figure out how to make this work well on ARM64. I need to ponder this some more, so these are just some initial thoughts.

My first impression is that (a) the x86 MCFG code is an unmitigated disaster, and (b) we're trying a little too hard to make that mess generic. I think we might be better served if we came up with some cleaner, more generic code that we can use for ARM64 today, and migrate x86 toward that over time.

My concern is that if we elevate the current x86 code to be "arch-independent", we will be perpetuating some interfaces and designs that shouldn't be allowed to escape arch/x86.

I think the major decision is whether to maintain the pci_mmcfg_list for all architectures or not. My initial plan was not to do this because of the mess (basically the ECAM region info should be attached to the pci root and not maintained in a separate list that needs locking), The patch I posted initially https://patchwork.ozlabs.org/patch/553464/ had a much simpler way of handling the MCFG table without using the list.

In x86 case it is not feasible to remove using the pci_mmcfg_list. The only use of it outside is in xen that can be fixed up.

Some of the code that moved to drivers/acpi/pci_mcfg.c is not really ACPI-specific, and could potentially be used for non-ACPI bridges that support ECAM. I'd like to see that sort of code moved to a new file like drivers/pci/ecam.c.

There's a little bit of overlap here with the ECAM code in pci-host-generic.c. I'm not sure whether or how to include that, but it's a very good example of how simple this *should* be: probe the host bridge, discover the ECAM region, request the region, ioremap it, done.

I had a similar approach in my initial patchset, please see the patch above. The resource for ECAM is mapped similar to the the way pci-host-generic.c handled it. An additional step I could do was to move the common code (ioremap and mapbus) into a common file and share the code with pci-host-generic.c

...

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c new file mode 100644 index 0000000..ea84365 --- /dev/null +++ b/drivers/acpi/pci_mcfg.c ... +int __weak pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg)
  

+{

• struct resource *tmp;
  

• void __iomem *vaddr;
  

• tmp = insert_resource_conflict(&iomem_resource, &mcfg->res);
  

• if (tmp) {
  

•         dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
  

•                 &mcfg->res, tmp->name, tmp);
  

•         return -EBUSY;
  

• }
  

I think this is a mistake in the x86 MCFG support that we should not carry over to a generic implementation. We should not use the MCFG table for resource reservation because MCFG is not defined by the ACPI spec and an OS need not include support for it. The platform must indicate in some other, more generic way, that ECAM space is reserved. This probably means ECAM space should be declared in a PNP0C02 _CRS method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).

We might need some kind of x86-specific quirk that does this, or a pcibios hook or something here; I just don't think it should be generic.

...

+int __init pci_mmconfig_parse_table(void) +{

• return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
  

+}

I don't like the fact that we parse the entire MCFG table at once here. I think we should look for the information we need when we are claiming a PCI host bridge, e.g., in acpi_pci_root_add(). This might not be a great fit for the way ACPI table management works, but I think it's better to do things on-demand rather than just-in-case.

There is an overhead of looking up this table, and the information available there is very limited (i.e, segment, start_bus, end_bus and address). My approach in the above patch is to save this info into an array at boot time and avoid multiple lookups.

...

diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..e9450ef 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; #define RESET_DELAY_DSM 0x08 #define FUNCTION_DELAY_DSM 0x09

+/* common API to maintain list of MCFG regions */ +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)

+struct pci_mmcfg_region {

• struct list_head list;
  

• struct resource res;
  

• u64 address;
  

• char __iomem *virt;
  

• u16 segment;
  

• u8 start_bus;
  

• u8 end_bus;
  

• char name[PCI_MMCFG_RESOURCE_NAME_LEN];
  

+};

+extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,

•                        phys_addr_t addr);
  

+extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);

+extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus); +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

•                                                 int end, u64 addr);
  

+extern int pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg);
  

+extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region *mcfg); +extern int pci_mmconfig_enabled(void); +extern int __init pci_mmconfig_parse_table(void);

+extern struct list_head pci_mmcfg_list;

+#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12)

With the exception of pci_mmconfig_parse_table(), nothing here is ACPI-specific. I'd like to see the PCI ECAM-related interfaces (hopefully not these exact ones, but a more rational set) put in something like include/linux/pci-ecam.h.

...

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I can update this patch to - drop the pci_mmcfg_list handling from generic case - move common ECAM code so that it can be shared with pci-host-generic.c if that is what you are looking for. The code will end up looking much simpler.

Thanks, JC.

Hi Bjorn,

Thanks for your pointers! See my comments inline. Aslo, can you please have a look at my previous patch set v4 and check how many of your comments are already addressed there. We may want to back to it then.

https://lkml.org/lkml/2016/2/4/646 Especially patches [0-6] which handle MMCONFIG refactoring.

On 05.03.2016 05:14, Bjorn Helgaas wrote:

On Fri, Mar 04, 2016 at 02:05:56PM +0530, Jayachandran Chandrashekaran Nair wrote:

...

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas helgaas@kernel.org wrote:

...

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is to share the API and code with ARM64 later. The corresponding declarations are moved from asm/pci_x86.h to linux/pci-acpi.h

As a part of this we introduce three functions that can be implemented by the arch code: pci_mmconfig_map_resource() to map a mcfg entry, pci_mmconfig_unmap_resource to do the corresponding unmap and pci_mmconfig_enabled to see if the arch setup of mcfg entries was successful. We also provide weak implementations of these, which will be used from ARM64. On x86, we retain the old logic by providing platform specific implementation.

This patch is purely rearranging code, it should not have any impact on the logic of MCFG parsing or list handling.

I definitely want to figure out how to make this work well on ARM64. I need to ponder this some more, so these are just some initial thoughts.

My first impression is that (a) the x86 MCFG code is an unmitigated disaster, and (b) we're trying a little too hard to make that mess generic. I think we might be better served if we came up with some cleaner, more generic code that we can use for ARM64 today, and migrate x86 toward that over time.

My concern is that if we elevate the current x86 code to be "arch-independent", we will be perpetuating some interfaces and designs that shouldn't be allowed to escape arch/x86.

I think the major decision is whether to maintain the pci_mmcfg_list for all architectures or not. My initial plan was not to do this because of the mess (basically the ECAM region info should be attached to the pci root and not maintained in a separate list that needs locking), The patch I posted initially https://patchwork.ozlabs.org/patch/553464/ had a much simpler way of handling the MCFG table without using the list.

I agree that ECAM info should be attached to the PCI host controller. That should simplify locking and hot-add and hot-removal of host controllers.

I think pci_mmcfg_list is an implementation detail that may not need to be generic. I certainly don't think it needs to be part of the interface.

...

In x86 case it is not feasible to remove using the pci_mmcfg_list. The only use of it outside is in xen that can be fixed up.

...

Some of the code that moved to drivers/acpi/pci_mcfg.c is not really ACPI-specific, and could potentially be used for non-ACPI bridges that support ECAM. I'd like to see that sort of code moved to a new file like drivers/pci/ecam.c.

There's a little bit of overlap here with the ECAM code in pci-host-generic.c. I'm not sure whether or how to include that, but it's a very good example of how simple this *should* be: probe the host bridge, discover the ECAM region, request the region, ioremap it, done.

I had a similar approach in my initial patchset, please see the patch above. The resource for ECAM is mapped similar to the the way pci-host-generic.c handled it. An additional step I could do was to move the common code (ioremap and mapbus) into a common file and share the code with pci-host-generic.c

...

...

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c new file mode 100644 index 0000000..ea84365 --- /dev/null +++ b/drivers/acpi/pci_mcfg.c ... +int __weak pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg)
  

+{

• struct resource *tmp;
  

• void __iomem *vaddr;
  

• tmp = insert_resource_conflict(&iomem_resource, &mcfg->res);
  

• if (tmp) {
  

•         dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
  

•                 &mcfg->res, tmp->name, tmp);
  

•         return -EBUSY;
  

• }
  

I think this is a mistake in the x86 MCFG support that we should not carry over to a generic implementation. We should not use the MCFG table for resource reservation because MCFG is not defined by the ACPI spec and an OS need not include support for it. The platform must indicate in some other, more generic way, that ECAM space is reserved. This probably means ECAM space should be declared in a PNP0C02 _CRS method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).

We might need some kind of x86-specific quirk that does this, or a pcibios hook or something here; I just don't think it should be generic.

...

+int __init pci_mmconfig_parse_table(void) +{

• return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
  

+}

I don't like the fact that we parse the entire MCFG table at once here. I think we should look for the information we need when we are claiming a PCI host bridge, e.g., in acpi_pci_root_add(). This might not be a great fit for the way ACPI table management works, but I think it's better to do things on-demand rather than just-in-case.

There is an overhead of looking up this table, and the information available there is very limited (i.e, segment, start_bus, end_bus and address). My approach in the above patch is to save this info into an array at boot time and avoid multiple lookups.

We need to look up MCFG info once per host bridge, so I don't think there's any performance issue here. But we do use acpi_table_parse(), which is __init, and *that* is a reason why we might need to parse the entire MCFG at boot-time. But this is the least of our worries in any case.

...

...

...

diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..e9450ef 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; #define RESET_DELAY_DSM 0x08 #define FUNCTION_DELAY_DSM 0x09

+/* common API to maintain list of MCFG regions */ +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)

+struct pci_mmcfg_region {

• struct list_head list;
  

• struct resource res;
  

• u64 address;
  

• char __iomem *virt;
  

• u16 segment;
  

• u8 start_bus;
  

• u8 end_bus;
  

• char name[PCI_MMCFG_RESOURCE_NAME_LEN];
  

+};

+extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,

•                        phys_addr_t addr);
  

+extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);

+extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus); +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

•                                                 int end, u64 addr);
  

+extern int pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg);
  

+extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region *mcfg); +extern int pci_mmconfig_enabled(void); +extern int __init pci_mmconfig_parse_table(void);

+extern struct list_head pci_mmcfg_list;

+#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12)

With the exception of pci_mmconfig_parse_table(), nothing here is ACPI-specific. I'd like to see the PCI ECAM-related interfaces (hopefully not these exact ones, but a more rational set) put in something like include/linux/pci-ecam.h.

...

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I can update this patch to

• drop the pci_mmcfg_list handling from generic case
• move common ECAM code so that it can be shared with pci-host-generic.c

if that is what you are looking for. The code will end up looking much simpler.

I think we should ignore x86 mmconfig for now. It is absurdly complicated and I'm not sure it's fixable. I *do* want to keep drivers/acpi/pci_root.c for all ACPI host bridges, including x86, ia64, and arm64.

So I think we should write generic MCFG and ECAM support from scratch for arm64. Something like this:

• Add an acpi_mcfg_init(), maybe in drivers/acpi/pci_mcfg.c, to be called from acpi_init() to copy MCFG info to something we can access after __init. This would not reserve resources, but probably does have to ioremap() the regions to support raw_pci_read().

As said, ECAM and ACPI specific code was isolated in previous patch. There, I tried to leave x86 complication in arch/x86/ and extract generic functionalities to driver/pci/ecam.c as the library.

• Implement raw_pci_read(), which is "special" because ACPI needs it for PCI config access from AML. It's supposed to be "always accessible" and we don't have a struct pci_bus *, so this probably has to use the MCFG copy and the ioremap done above. Maybe it should go in the same file. This is completely independent of the PCI core and PCI data structures.

We were looking for the answer which would justify RAW PCI config accessors being for ARM64 world. Unfortunately, nobody was able to show real use case for ARM64. Do you see the reason we need this? Our conclusion was to leave it empty for ARM64 which in turn makes code simpler. I am not ASWG member while that was under discussion so I will ask Lorenzo to elaborate more on this.

• Implement arm64 pci_acpi_scan_root() that calls acpi_pci_root_create() with an .init_info() function that calls acpi_pci_root_get_mcfg_addr() to read _CBA, and if that fails, looks up the bus range in the MCFG copy from above. It should call request_mem_region(). For a region from _CBA, it should call ioremap(). For regions from MCFG it can probably use the ioremap done by acpi_mcfg_init().

Yes, Expanding .init_info() to check for _CBA is good point.

 I know acpi_pci_root_add() calls acpi_pci_root_get_mcfg_addr()
 before calling pci_acpi_scan_root(), but I think that's wrong
 because (a) some arches, e.g., ia64, don't use ECAM and (b) _CBA
 and MCFG should be handled in the same place.

 I know calling request_mem_region() here will probably be an
 ordering problem because the PNP0C02 driver hasn't reserved
 resources yet.  But the host bridge driver is using the region and
 it should reserve it.

• If we store the ECAM mapped base address in the sysdata or struct pci_host_bridge, the normal config accessors can use pci_generic_config_read() with a new generic .map_bus() function.

pci_generic_config_{read|write}() is what we want to use, actually we do now, but ECAM region and sysdata association will remove ECAM region lookup step (see patch 09/15 of this series).

 On x86, the normal config access path is:

   pci_read(struct pci_bus *, ...)
     raw_pci_read(seg, bus#, ...)
 raw_pci_ext_ops->read(seg, bus#, ...)
   pci_mmcfg_read(seg, bus#, ...)
     pci_dev_base
       pci_mmconfig_lookup(seg, bus#)

 I think this is somewhat backwards because we start with a pci_bus
 pointer, so we *could* have a nice simple bus-specific accessor,
 but we throw that pointer away, so pci_mmcfg_read() has to start
 over and look up the ECAM offset from scratch, which makes it all
 unnecessarily complicated.

As you pointed out raw_pci_{read|write} make things complicated, so IMO we should either say they are absolutely necessary (and then think how to simplify it) or just use simple bus-specific accessor (patch 02/15) e.g. for ARM64.

Any comments appreciated.

Thanks, Tomasz

Hi Tomasz,

On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki tn@semihalf.com wrote:

Hi Bjorn,

Thanks for your pointers! See my comments inline. Aslo, can you please have a look at my previous patch set v4 and check how many of your comments are already addressed there. We may want to back to it then.

https://lkml.org/lkml/2016/2/4/646 Especially patches [0-6] which handle MMCONFIG refactoring.

On 05.03.2016 05:14, Bjorn Helgaas wrote:

...

On Fri, Mar 04, 2016 at 02:05:56PM +0530, Jayachandran Chandrashekaran Nair wrote:

...

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas helgaas@kernel.org wrote:

...

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is to share the API and code with ARM64 later. The corresponding declarations are moved from asm/pci_x86.h to linux/pci-acpi.h

As a part of this we introduce three functions that can be implemented by the arch code: pci_mmconfig_map_resource() to map a mcfg entry, pci_mmconfig_unmap_resource to do the corresponding unmap and pci_mmconfig_enabled to see if the arch setup of mcfg entries was successful. We also provide weak implementations of these, which will be used from ARM64. On x86, we retain the old logic by providing platform specific implementation.

This patch is purely rearranging code, it should not have any impact on the logic of MCFG parsing or list handling.

I definitely want to figure out how to make this work well on ARM64. I need to ponder this some more, so these are just some initial thoughts.

My first impression is that (a) the x86 MCFG code is an unmitigated disaster, and (b) we're trying a little too hard to make that mess generic. I think we might be better served if we came up with some cleaner, more generic code that we can use for ARM64 today, and migrate x86 toward that over time.

My concern is that if we elevate the current x86 code to be "arch-independent", we will be perpetuating some interfaces and designs that shouldn't be allowed to escape arch/x86.

I think the major decision is whether to maintain the pci_mmcfg_list for all architectures or not. My initial plan was not to do this because of the mess (basically the ECAM region info should be attached to the pci root and not maintained in a separate list that needs locking), The patch I posted initially https://patchwork.ozlabs.org/patch/553464/ had a much simpler way of handling the MCFG table without using the list.

I agree that ECAM info should be attached to the PCI host controller. That should simplify locking and hot-add and hot-removal of host controllers.

I think pci_mmcfg_list is an implementation detail that may not need to be generic. I certainly don't think it needs to be part of the interface.

...

In x86 case it is not feasible to remove using the pci_mmcfg_list. The only use of it outside is in xen that can be fixed up.

...

Some of the code that moved to drivers/acpi/pci_mcfg.c is not really ACPI-specific, and could potentially be used for non-ACPI bridges that support ECAM. I'd like to see that sort of code moved to a new file like drivers/pci/ecam.c.

There's a little bit of overlap here with the ECAM code in pci-host-generic.c. I'm not sure whether or how to include that, but it's a very good example of how simple this *should* be: probe the host bridge, discover the ECAM region, request the region, ioremap it, done.

I had a similar approach in my initial patchset, please see the patch above. The resource for ECAM is mapped similar to the the way pci-host-generic.c handled it. An additional step I could do was to move the common code (ioremap and mapbus) into a common file and share the code with pci-host-generic.c

...

...

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c new file mode 100644 index 0000000..ea84365 --- /dev/null +++ b/drivers/acpi/pci_mcfg.c ... +int __weak pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg)
  

+{

• struct resource *tmp;
  

• void __iomem *vaddr;
  

• tmp = insert_resource_conflict(&iomem_resource, &mcfg->res);
  

• if (tmp) {
  

•         dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
  

•                 &mcfg->res, tmp->name, tmp);
  

•         return -EBUSY;
  

• }
  

I think this is a mistake in the x86 MCFG support that we should not carry over to a generic implementation. We should not use the MCFG table for resource reservation because MCFG is not defined by the ACPI spec and an OS need not include support for it. The platform must indicate in some other, more generic way, that ECAM space is reserved. This probably means ECAM space should be declared in a PNP0C02 _CRS method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).

We might need some kind of x86-specific quirk that does this, or a pcibios hook or something here; I just don't think it should be generic.

...

+int __init pci_mmconfig_parse_table(void) +{

• return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
  

+}

I don't like the fact that we parse the entire MCFG table at once here. I think we should look for the information we need when we are claiming a PCI host bridge, e.g., in acpi_pci_root_add(). This might not be a great fit for the way ACPI table management works, but I think it's better to do things on-demand rather than just-in-case.

There is an overhead of looking up this table, and the information available there is very limited (i.e, segment, start_bus, end_bus and address). My approach in the above patch is to save this info into an array at boot time and avoid multiple lookups.

We need to look up MCFG info once per host bridge, so I don't think there's any performance issue here. But we do use acpi_table_parse(), which is __init, and *that* is a reason why we might need to parse the entire MCFG at boot-time. But this is the least of our worries in any case.

...

...

...

diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..e9450ef 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; #define RESET_DELAY_DSM 0x08 #define FUNCTION_DELAY_DSM 0x09

+/* common API to maintain list of MCFG regions */ +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)

+struct pci_mmcfg_region {

• struct list_head list;
  

• struct resource res;
  

• u64 address;
  

• char __iomem *virt;
  

• u16 segment;
  

• u8 start_bus;
  

• u8 end_bus;
  

• char name[PCI_MMCFG_RESOURCE_NAME_LEN];
  

+};

+extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,

•                        phys_addr_t addr);
  

+extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);

+extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus); +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

•                                                 int end, u64
  

addr); +extern int pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg);
  

+extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region *mcfg); +extern int pci_mmconfig_enabled(void); +extern int __init pci_mmconfig_parse_table(void);

+extern struct list_head pci_mmcfg_list;

+#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12)

With the exception of pci_mmconfig_parse_table(), nothing here is ACPI-specific. I'd like to see the PCI ECAM-related interfaces (hopefully not these exact ones, but a more rational set) put in something like include/linux/pci-ecam.h.

...

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I can update this patch to

• drop the pci_mmcfg_list handling from generic case
• move common ECAM code so that it can be shared with pci-host-generic.c

if that is what you are looking for. The code will end up looking much simpler.

I think we should ignore x86 mmconfig for now. It is absurdly complicated and I'm not sure it's fixable. I *do* want to keep drivers/acpi/pci_root.c for all ACPI host bridges, including x86, ia64, and arm64.

So I think we should write generic MCFG and ECAM support from scratch for arm64. Something like this:

• Add an acpi_mcfg_init(), maybe in drivers/acpi/pci_mcfg.c, to be called from acpi_init() to copy MCFG info to something we can access after __init. This would not reserve resources, but probably does have to ioremap() the regions to support raw_pci_read().

As said, ECAM and ACPI specific code was isolated in previous patch. There, I tried to leave x86 complication in arch/x86/ and extract generic functionalities to driver/pci/ecam.c as the library.

...

• Implement raw_pci_read(), which is "special" because ACPI needs it for PCI config access from AML. It's supposed to be "always accessible" and we don't have a struct pci_bus *, so this probably has to use the MCFG copy and the ioremap done above. Maybe it should go in the same file. This is completely independent of the PCI core and PCI data structures.

We were looking for the answer which would justify RAW PCI config accessors being for ARM64 world. Unfortunately, nobody was able to show real use case for ARM64. Do you see the reason we need this? Our conclusion was to leave it empty for ARM64 which in turn makes code simpler. I am not ASWG member while that was under discussion so I will ask Lorenzo to elaborate more on this.

...

• Implement arm64 pci_acpi_scan_root() that calls acpi_pci_root_create() with an .init_info() function that calls acpi_pci_root_get_mcfg_addr() to read _CBA, and if that fails, looks up the bus range in the MCFG copy from above. It should call request_mem_region(). For a region from _CBA, it should call ioremap(). For regions from MCFG it can probably use the ioremap done by acpi_mcfg_init().

Yes, Expanding .init_info() to check for _CBA is good point.

...

 I know acpi_pci_root_add() calls acpi_pci_root_get_mcfg_addr()
 before calling pci_acpi_scan_root(), but I think that's wrong
 because (a) some arches, e.g., ia64, don't use ECAM and (b) _CBA
 and MCFG should be handled in the same place.

 I know calling request_mem_region() here will probably be an
 ordering problem because the PNP0C02 driver hasn't reserved
 resources yet.  But the host bridge driver is using the region and
 it should reserve it.

• If we store the ECAM mapped base address in the sysdata or struct pci_host_bridge, the normal config accessors can use pci_generic_config_read() with a new generic .map_bus() function.

pci_generic_config_{read|write}() is what we want to use, actually we do now, but ECAM region and sysdata association will remove ECAM region lookup step (see patch 09/15 of this series).

...

 On x86, the normal config access path is:

   pci_read(struct pci_bus *, ...)
     raw_pci_read(seg, bus#, ...)
      raw_pci_ext_ops->read(seg, bus#, ...)
        pci_mmcfg_read(seg, bus#, ...)
          pci_dev_base
            pci_mmconfig_lookup(seg, bus#)

 I think this is somewhat backwards because we start with a pci_bus
 pointer, so we *could* have a nice simple bus-specific accessor,
 but we throw that pointer away, so pci_mmcfg_read() has to start
 over and look up the ECAM offset from scratch, which makes it all
 unnecessarily complicated.

As you pointed out raw_pci_{read|write} make things complicated, so IMO we should either say they are absolutely necessary (and then think how to simplify it) or just use simple bus-specific accessor (patch 02/15) e.g. for ARM64.

Both raw_pci_read/write and a host controller with pci_generic_read/write can be done without much trouble, please see the patch I had at: https://patchwork.ozlabs.org/patch/575526/

I have been looking at Bjorn's suggestions and trying to see if I can update my MCFG patch taking care of them. I will post an updated patchset soon, unless you want to take this up.

JC.

Hi Tomasz,

On Wed, Mar 9, 2016 at 4:20 PM, Tomasz Nowicki tn@semihalf.com wrote:

Hi Jayachandran,

On 09.03.2016 11:10, Jayachandran Chandrashekaran Nair wrote:

...

Hi Tomasz,

On Wed, Mar 9, 2016 at 2:43 PM, Tomasz Nowicki tn@semihalf.com wrote:

...

Hi Bjorn,

Thanks for your pointers! See my comments inline. Aslo, can you please have a look at my previous patch set v4 and check how many of your comments are already addressed there. We may want to back to it then.

https://lkml.org/lkml/2016/2/4/646 Especially patches [0-6] which handle MMCONFIG refactoring.

On 05.03.2016 05:14, Bjorn Helgaas wrote:

...

On Fri, Mar 04, 2016 at 02:05:56PM +0530, Jayachandran Chandrashekaran Nair wrote:

...

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas helgaas@kernel.org wrote:

...

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote: > > > From: Jayachandran C jchandra@broadcom.com > > Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is > to share the API and code with ARM64 later. The corresponding > declarations are moved from asm/pci_x86.h to linux/pci-acpi.h > > As a part of this we introduce three functions that can be > implemented by the arch code: pci_mmconfig_map_resource() to map a > mcfg entry, pci_mmconfig_unmap_resource to do the corresponding > unmap and pci_mmconfig_enabled to see if the arch setup of > mcfg entries was successful. We also provide weak implementations > of these, which will be used from ARM64. On x86, we retain the > old logic by providing platform specific implementation. > > This patch is purely rearranging code, it should not have any > impact on the logic of MCFG parsing or list handling.

I definitely want to figure out how to make this work well on ARM64. I need to ponder this some more, so these are just some initial thoughts.

My first impression is that (a) the x86 MCFG code is an unmitigated disaster, and (b) we're trying a little too hard to make that mess generic. I think we might be better served if we came up with some cleaner, more generic code that we can use for ARM64 today, and migrate x86 toward that over time.

My concern is that if we elevate the current x86 code to be "arch-independent", we will be perpetuating some interfaces and designs that shouldn't be allowed to escape arch/x86.

I think the major decision is whether to maintain the pci_mmcfg_list for all architectures or not. My initial plan was not to do this because of the mess (basically the ECAM region info should be attached to the pci root and not maintained in a separate list that needs locking), The patch I posted initially https://patchwork.ozlabs.org/patch/553464/ had a much simpler way of handling the MCFG table without using the list.

I agree that ECAM info should be attached to the PCI host controller. That should simplify locking and hot-add and hot-removal of host controllers.

I think pci_mmcfg_list is an implementation detail that may not need to be generic. I certainly don't think it needs to be part of the interface.

...

In x86 case it is not feasible to remove using the pci_mmcfg_list. The only use of it outside is in xen that can be fixed up.

...

Some of the code that moved to drivers/acpi/pci_mcfg.c is not really ACPI-specific, and could potentially be used for non-ACPI bridges that support ECAM. I'd like to see that sort of code moved to a new file like drivers/pci/ecam.c.

There's a little bit of overlap here with the ECAM code in pci-host-generic.c. I'm not sure whether or how to include that, but it's a very good example of how simple this *should* be: probe the host bridge, discover the ECAM region, request the region, ioremap it, done.

I had a similar approach in my initial patchset, please see the patch above. The resource for ECAM is mapped similar to the the way pci-host-generic.c handled it. An additional step I could do was to move the common code (ioremap and mapbus) into a common file and share the code with pci-host-generic.c

...

> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > new file mode 100644 > index 0000000..ea84365 > --- /dev/null > +++ b/drivers/acpi/pci_mcfg.c > ... > +int __weak pci_mmconfig_map_resource(struct device *dev, > + struct pci_mmcfg_region *mcfg) > +{ > + struct resource *tmp; > + void __iomem *vaddr; > + > + tmp = insert_resource_conflict(&iomem_resource, &mcfg->res); > + if (tmp) { > + dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n", > + &mcfg->res, tmp->name, tmp); > + return -EBUSY; > + }

I think this is a mistake in the x86 MCFG support that we should not carry over to a generic implementation. We should not use the MCFG table for resource reservation because MCFG is not defined by the ACPI spec and an OS need not include support for it. The platform must indicate in some other, more generic way, that ECAM space is reserved. This probably means ECAM space should be declared in a PNP0C02 _CRS method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).

We might need some kind of x86-specific quirk that does this, or a pcibios hook or something here; I just don't think it should be generic.

> +int __init pci_mmconfig_parse_table(void) > +{ > + return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg); > +}

I don't like the fact that we parse the entire MCFG table at once here. I think we should look for the information we need when we are claiming a PCI host bridge, e.g., in acpi_pci_root_add(). This might not be a great fit for the way ACPI table management works, but I think it's better to do things on-demand rather than just-in-case.

There is an overhead of looking up this table, and the information available there is very limited (i.e, segment, start_bus, end_bus and address). My approach in the above patch is to save this info into an array at boot time and avoid multiple lookups.

We need to look up MCFG info once per host bridge, so I don't think there's any performance issue here. But we do use acpi_table_parse(), which is __init, and *that* is a reason why we might need to parse the entire MCFG at boot-time. But this is the least of our worries in any case.

...

...

> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > index 89ab057..e9450ef 100644 > --- a/include/linux/pci-acpi.h > +++ b/include/linux/pci-acpi.h > @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; > #define RESET_DELAY_DSM 0x08 > #define FUNCTION_DELAY_DSM 0x09 > > +/* common API to maintain list of MCFG regions */ > +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ > +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2) > + > +struct pci_mmcfg_region { > + struct list_head list; > + struct resource res; > + u64 address; > + char __iomem *virt; > + u16 segment; > + u8 start_bus; > + u8 end_bus; > + char name[PCI_MMCFG_RESOURCE_NAME_LEN]; > +}; > + > +extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 > start, > u8 end, > + phys_addr_t addr); > +extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end); > + > +extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int > bus); > +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int > start, > + int end, u64 > addr); > +extern int pci_mmconfig_map_resource(struct device *dev, > + struct pci_mmcfg_region *mcfg); > +extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region > *mcfg); > +extern int pci_mmconfig_enabled(void); > +extern int __init pci_mmconfig_parse_table(void); > + > +extern struct list_head pci_mmcfg_list; > + > +#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) > +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12) > +

With the exception of pci_mmconfig_parse_table(), nothing here is ACPI-specific. I'd like to see the PCI ECAM-related interfaces (hopefully not these exact ones, but a more rational set) put in something like include/linux/pci-ecam.h.

> #else /* CONFIG_ACPI */ > static inline void acpi_pci_add_bus(struct pci_bus *bus) { } > static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I can update this patch to

• drop the pci_mmcfg_list handling from generic case
• move common ECAM code so that it can be shared with pci-host-generic.c

if that is what you are looking for. The code will end up looking much simpler.

I think we should ignore x86 mmconfig for now. It is absurdly complicated and I'm not sure it's fixable. I *do* want to keep drivers/acpi/pci_root.c for all ACPI host bridges, including x86, ia64, and arm64.

So I think we should write generic MCFG and ECAM support from scratch for arm64. Something like this:

- Add an acpi_mcfg_init(), maybe in drivers/acpi/pci_mcfg.c, to be
  called from acpi_init() to copy MCFG info to something we can
  access after __init.  This would not reserve resources, but
  probably does have to ioremap() the regions to support
  raw_pci_read().

As said, ECAM and ACPI specific code was isolated in previous patch. There, I tried to leave x86 complication in arch/x86/ and extract generic functionalities to driver/pci/ecam.c as the library.

...

- Implement raw_pci_read(), which is "special" because ACPI needs it
  for PCI config access from AML.  It's supposed to be "always
  accessible" and we don't have a struct pci_bus *, so this probably
  has to use the MCFG copy and the ioremap done above.  Maybe it
  should go in the same file.  This is completely independent of
  the PCI core and PCI data structures.

We were looking for the answer which would justify RAW PCI config accessors being for ARM64 world. Unfortunately, nobody was able to show real use case for ARM64. Do you see the reason we need this? Our conclusion was to leave it empty for ARM64 which in turn makes code simpler. I am not ASWG member while that was under discussion so I will ask Lorenzo to elaborate more on this.

...

- Implement arm64 pci_acpi_scan_root() that calls
  acpi_pci_root_create() with an .init_info() function that calls
  acpi_pci_root_get_mcfg_addr() to read _CBA, and if that fails,
  looks up the bus range in the MCFG copy from above.  It should
  call request_mem_region().  For a region from _CBA, it should call
  ioremap().  For regions from MCFG it can probably use the ioremap
  done by acpi_mcfg_init().

Yes, Expanding .init_info() to check for _CBA is good point.

...

  I know acpi_pci_root_add() calls acpi_pci_root_get_mcfg_addr()
  before calling pci_acpi_scan_root(), but I think that's wrong
  because (a) some arches, e.g., ia64, don't use ECAM and (b) _CBA
  and MCFG should be handled in the same place.

  I know calling request_mem_region() here will probably be an
  ordering problem because the PNP0C02 driver hasn't reserved
  resources yet.  But the host bridge driver is using the region and
  it should reserve it.

- If we store the ECAM mapped base address in the sysdata or struct
  pci_host_bridge, the normal config accessors can use
  pci_generic_config_read() with a new generic .map_bus() function.

pci_generic_config_{read|write}() is what we want to use, actually we do now, but ECAM region and sysdata association will remove ECAM region lookup step (see patch 09/15 of this series).

...

  On x86, the normal config access path is:

    pci_read(struct pci_bus *, ...)
      raw_pci_read(seg, bus#, ...)
       raw_pci_ext_ops->read(seg, bus#, ...)
         pci_mmcfg_read(seg, bus#, ...)
           pci_dev_base
             pci_mmconfig_lookup(seg, bus#)

  I think this is somewhat backwards because we start with a pci_bus
  pointer, so we *could* have a nice simple bus-specific accessor,
  but we throw that pointer away, so pci_mmcfg_read() has to start
  over and look up the ECAM offset from scratch, which makes it all
  unnecessarily complicated.

As you pointed out raw_pci_{read|write} make things complicated, so IMO we should either say they are absolutely necessary (and then think how to simplify it) or just use simple bus-specific accessor (patch 02/15) e.g. for ARM64.

Both raw_pci_read/write and a host controller with pci_generic_read/write can be done without much trouble, please see the patch I had at: https://patchwork.ozlabs.org/patch/575526/

Yes it is doable, I implemented it in the same way in one of my initial patch series, about year ago. I'm questioning raw accessors presence on per-arch basis. If it is really needed for all archs, then we definitely should implement it. If ARM64 does not care for it, there is no point to complicate it. Especially, I mean all kind of PCI config space quirk we will need to handle right after this patch got merged, see: [PATCH V5 13/15] pci, acpi: Match PCI config space accessors against platfrom specific quirks.

and

https://lkml.org/lkml/2016/2/9/627 https://lkml.org/lkml/2016/2/8/967

Giving these quirks, raw accessors are not so easy.

The whole quirk handling infrastructure seems to be an overkill to me. I will leave it to maintainers to comment further.

...

I have been looking at Bjorn's suggestions and trying to see if I can update my MCFG patch taking care of them. I will post an updated patch set soon, unless you want to take this up.

Yes, I want to post next version and keep this patch set together, if you and Bjorn are okay. I am feeling that my previous patch set is close to what Bjorn suggested, modulo the way we keep MCFG regions. Lets discuss it here, then I will post it as next version. I am looking forward to hear Bjorn's comment on my previous patch set.

I have been looking thru the code, and I have a reasonable implementation which updates this one patch. This pulls in common code from pci-host-generic.c as well. I will post it by next week and you can decide whether to use it to update your patchset.

Thanks, JC.

Add config option PCI_GENERIC_ECAM and file drivers/pci/ecam.c to provide generic functions to access memory mapped PCI config space.

The API defines 'struct pci_config_window' to hold the mappings. The function pci_generic_map_config() is provided to allocate the struct, request the memory region, and do the ioremap() calls needed to setup the mapping. pci_generic_unmap_config() is provided to delete a mapping.

A helper function pci_generic_map_bus() is also provided to be used to implement pci_ops map_bus method.

Signed-off-by: Jayachandran C jchandra@broadcom.com --- drivers/pci/Kconfig | 3 ++ drivers/pci/Makefile | 2 + drivers/pci/ecam.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 10 ++++ 4 files changed, 142 insertions(+) create mode 100644 drivers/pci/ecam.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 209292e..e930d62 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -83,6 +83,9 @@ config HT_IRQ config PCI_ATS bool

+config PCI_GENERIC_ECAM + bool + config PCI_IOV bool "PCI IOV support" depends on PCI diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index 2154092..810aec8 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -55,6 +55,8 @@ obj-$(CONFIG_PCI_SYSCALL) += syscall.o

obj-$(CONFIG_PCI_STUB) += pci-stub.o

+obj-$(CONFIG_PCI_GENERIC_ECAM) += ecam.o + obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o

obj-$(CONFIG_OF) += of.o diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c new file mode 100644 index 0000000..6a63901 --- /dev/null +++ b/drivers/pci/ecam.c @@ -0,0 +1,127 @@ +/* + * Copyright 2016 Broadcom + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation (the "GPL"). + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License version 2 (GPLv2) for more details. + * + * You should have received a copy of the GNU General Public License + * version 2 (GPLv2) along with this source code. + */ + +#include <linux/device.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/slab.h> + +/* + * On 64 bit systems, we do a single ioremap for the whole config space + * since we have enough virtual address range available. On 32 bit, do an + * ioremap per bus. + */ +static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT); + +/* + * struct to hold the mappings of a config space window. This + * will be allocated with enough entries in win[] to hold all + * the mappings for the bus range. + */ +struct pci_config_window { + phys_addr_t cfgaddr; + u8 bus_start; + u8 bus_end; + u8 bus_shift; + u8 devfn_shift; + void __iomem *win[0]; +}; + +/* + * helper function provided to implement the pci_ops ->map_bus method + */ +void __iomem *pci_generic_map_bus(struct pci_config_window *cfg, + unsigned int busn, unsigned int devfn, int where) +{ + void __iomem *base; + + if (busn < cfg->bus_start || busn > cfg->bus_end) + return NULL; + + busn -= cfg->bus_start; + if (per_bus_mapping) + base = cfg->win[busn]; + else + base = cfg->win[0] + (busn << cfg->bus_shift); + return base + (devfn << cfg->devfn_shift) + where; +} + +/* + * Create a PCI config space window + * - reserve mem region + * - alloc struct pci_config_window with space for all mappings + * - ioremap the config space + */ +struct pci_config_window *pci_generic_map_config(phys_addr_t addr, + u8 bus_start, u8 bus_end, u8 bus_shift, u8 devfn_shift) +{ + struct pci_config_window *cfg; + unsigned int bus_range, bsz, mapsz; + int i, nidx; + + if (bus_end < bus_start) + return ERR_PTR(-EINVAL); + + bus_range = bus_end - bus_start + 1; + bsz = 1 << bus_shift; + nidx = per_bus_mapping ? bus_range : 1; + mapsz = per_bus_mapping ? bsz : bus_range * bsz; + cfg = kzalloc(sizeof(*cfg) + nidx * sizeof(cfg->win[0]), GFP_KERNEL); + if (!cfg) + return ERR_PTR(-ENOMEM); + + cfg->bus_start = bus_start; + cfg->bus_end = bus_end; + cfg->bus_shift = bus_shift; + cfg->devfn_shift = devfn_shift; + + if (!request_mem_region(addr, bus_range * bsz, "Configuration Space")) + goto err_exit; + + /* cfgaddr has to be set after request_mem_region */ + cfg->cfgaddr = addr; + + for (i = 0; i < nidx; i++) { + cfg->win[i] = ioremap(addr + i * mapsz, mapsz); + if (!cfg->win[i]) + goto err_exit; + } + return cfg; + +err_exit: + pci_generic_unmap_config(cfg); + return ERR_PTR(-ENOMEM); +} + +/* + * Free a config space mapping + */ +void pci_generic_unmap_config(struct pci_config_window *cfg) +{ + unsigned int bus_range; + int i, nidx; + + bus_range = cfg->bus_end - cfg->bus_start + 1; + nidx = per_bus_mapping ? bus_range : 1; + for (i = 0; i < nidx; i++) + if (cfg->win[i]) + iounmap(cfg->win[i]); + if (cfg->cfgaddr) + release_mem_region(cfg->cfgaddr, bus_range << cfg->bus_shift); + kfree(cfg); +} diff --git a/include/linux/pci.h b/include/linux/pci.h index 3df9e37..33c46bc 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2013,6 +2013,16 @@ static inline bool pci_ari_enabled(struct pci_bus *bus) return bus->self && bus->self->ari_enabled; }

+/* Generic ECAM mapping API */ +#ifdef CONFIG_PCI_GENERIC_ECAM +struct pci_config_window; +void __iomem *pci_generic_map_bus(struct pci_config_window *cfg, + unsigned int busn, unsigned int devfn, int where); +struct pci_config_window *pci_generic_map_config(phys_addr_t addr, + u8 bus_start, u8 bus_end, u8 bus_shift, u8 devfn_shift); +void pci_generic_unmap_config(struct pci_config_window *cfg); +#endif + /* provide the legacy pci_dma_* API */ #include <linux/pci-dma-compat.h>

Add a generic ACPI based PCI host controller, and provide config option ACPI_PCI_HOST_GENERIC to enable it.

The implementation selects PCI_MMCONFIG and implements function pci_mmcfg_late_init() to parse MCFG table and save its entries. PCI_GENERIC_ECAM is also selected and pci/ecam.c functions are used to map the config space of the PCI controller.

acpi_pci_root_ops is setup so that init_info looks up the saved MCFG entries and sets up the config space mapping. release_info deletes the mapping and frees the memory. Generic PCI functions are used for accessing config space.

Signed-off-by: Jayachandran C jchandra@broadcom.com --- drivers/acpi/Kconfig | 9 ++ drivers/acpi/Makefile | 1 + drivers/acpi/pci_gen_host.c | 247 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 257 insertions(+) create mode 100644 drivers/acpi/pci_gen_host.c

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 82b96ee..f178f2e 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -343,6 +343,15 @@ config ACPI_PCI_SLOT i.e., segment/bus/device/function tuples, with physical slots in the system. If you are unsure, say N.

+config ACPI_PCI_HOST_GENERIC + bool "Generic ACPI based PCI controller" + depends on ARM64 + select PCI_MMCONFIG + select PCI_GENERIC_ECAM + help + Say Y if you want the generic ACPI based PCI controller + implementation. + config X86_PM_TIMER bool "Power Management Timer Support" if EXPERT depends on X86 diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index cb648a4..ceab5b0 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -65,6 +65,7 @@ obj-$(CONFIG_ACPI_BUTTON) += button.o obj-$(CONFIG_ACPI_FAN) += fan.o obj-$(CONFIG_ACPI_VIDEO) += video.o obj-$(CONFIG_ACPI_PCI_SLOT) += pci_slot.o +obj-$(CONFIG_ACPI_PCI_HOST_GENERIC) += pci_gen_host.o obj-$(CONFIG_ACPI_PROCESSOR) += processor.o obj-$(CONFIG_ACPI) += container.o obj-$(CONFIG_ACPI_THERMAL) += thermal.o diff --git a/drivers/acpi/pci_gen_host.c b/drivers/acpi/pci_gen_host.c new file mode 100644 index 0000000..a43f2cee --- /dev/null +++ b/drivers/acpi/pci_gen_host.c @@ -0,0 +1,247 @@ +/* + * Copyright 2016 Broadcom + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, as + * published by the Free Software Foundation (the "GPL"). + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License version 2 (GPLv2) for more details. + * + * You should have received a copy of the GNU General Public License + * version 2 (GPLv2) along with this source code. + */ +#include <linux/kernel.h> +#include <linux/pci.h> +#include <linux/pci-acpi.h> +#include <linux/sfi_acpi.h> +#include <linux/slab.h> + +#define PREFIX "ACPI: " + +/* + * Structure to hold entries from the MCFG table, these need to be + * mapped until a pci host bridge claims them for raw_pci_read/write + * to work + */ +struct mcfg_entry { + phys_addr_t addr; + u16 segment; + u8 bus_start; + u8 bus_end; +}; + +/* + * Global to save mcfg entries + */ +static struct { + struct mcfg_entry *cfg; + int size; +} mcfgsav; + +/* List of all ACPI PCI roots, needed for raw operations */ +static struct list_head gen_acpi_pci_roots; + +/* lock for global table AND list above */ +static struct mutex gen_acpi_pci_lock; + +/* ACPI info for generic ACPI PCI controller */ +struct acpi_pci_generic_root_info { + struct acpi_pci_root_info common; + struct pci_config_window *cfg; /* config space mapping */ + struct list_head node; /* node in acpi_pci_roots */ +}; + +/* Call generic map_bus after getting cfg pointer */ +static void __iomem *gen_acpi_map_cfg_bus(struct pci_bus *bus, + unsigned int devfn, int where) +{ + struct acpi_pci_generic_root_info *ri = bus->sysdata; + + return pci_generic_map_bus(ri->cfg, bus->number, devfn, where); +} + +static struct pci_ops acpi_pci_ops = { + .map_bus = gen_acpi_map_cfg_bus, + .read = pci_generic_config_read, + .write = pci_generic_config_write, +}; + +/* find the entry in mcfgsav.cfg which contains range bus_start..bus_end */ +static int mcfg_lookup(u16 seg, u8 bus_start, u8 bus_end) +{ + struct mcfg_entry *e; + int i; + + for (i = 0, e = mcfgsav.cfg; i < mcfgsav.size; i++, e++) { + if (seg != e->segment) + continue; + if (bus_start >= e->bus_start && bus_start <= e->bus_end) + return (bus_end <= e->bus_end) ? i : -EINVAL; + else if (bus_end >= e->bus_start && bus_end <= e->bus_end) + return -EINVAL; + } + return -ENOENT; +} + +/* + * init_info - lookup the bus range for the domain in MCFG, and set up + * config space mapping. + */ +static int pci_acpi_generic_init_info(struct acpi_pci_root_info *ci) +{ + struct acpi_pci_generic_root_info *ri; + struct acpi_pci_root *root = ci->root; + u16 seg = root->segment; + u8 bus_start = root->secondary.start; + u8 bus_end = root->secondary.end; + phys_addr_t addr = root->mcfg_addr; + struct mcfg_entry *e; + int ret; + + ri = container_of(ci, struct acpi_pci_generic_root_info, common); + + mutex_lock(&gen_acpi_pci_lock); + ret = mcfg_lookup(seg, bus_start, bus_end); + switch (ret) { + case -ENOENT: + if (addr != 0) /* use address from _CBA */ + break; + pr_err("%04x:%02x-%02x mcfg lookup failed\n", seg, + bus_start, bus_end); + goto err_out; + case -EINVAL: + pr_err("%04x:%02x-%02x bus range error\n", seg, bus_start, + bus_end); + goto err_out; + default: + e = &mcfgsav.cfg[ret]; + if (addr == 0) + addr = e->addr; + if (bus_start != e->bus_start) { + pr_err("%04x:%02x-%02x bus range mismatch %02x\n", + seg, bus_start, bus_end, e->bus_start); + goto err_out; + } + if (addr != e->addr) { + pr_warn("%04x:%02x-%02x addr mismatch, ignoring MCFG\n", + seg, bus_start, bus_end); + } else if (bus_end != e->bus_end) { + pr_warn("%04x:%02x-%02x bus end mismatch %02x\n", + seg, bus_start, bus_end, e->bus_end); + bus_end = min(bus_end, e->bus_end); + } + break; + } + + ri->cfg = pci_generic_map_config(addr, bus_start, bus_end, 20, 12); + if (IS_ERR(ri->cfg)) { + ret = PTR_ERR(ri->cfg); + pr_err("%04x:%02x-%02x error %d mapping CFG\n", seg, bus_start, + bus_end, ret); + goto err_out; + } + list_add_tail(&ri->node, &gen_acpi_pci_roots); +err_out: + mutex_unlock(&gen_acpi_pci_lock); + return ret; +} + +/* release_info: free resrouces allocated by init_info */ +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci) +{ + struct acpi_pci_generic_root_info *ri; + + ri = container_of(ci, struct acpi_pci_generic_root_info, common); + + mutex_lock(&gen_acpi_pci_lock); + list_del(&ri->node); + pci_generic_unmap_config(ri->cfg); + mutex_unlock(&gen_acpi_pci_lock); + + kfree(ri); +} + +static struct acpi_pci_root_ops acpi_pci_root_ops = { + .pci_ops = &acpi_pci_ops, + .init_info = pci_acpi_generic_init_info, + .release_info = pci_acpi_generic_release_info, +}; + +/* Interface called from ACPI code to setup PCI host controller */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{ + int node = acpi_get_node(root->device->handle); + struct acpi_pci_generic_root_info *ri; + struct pci_bus *bus, *child; + + ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node); + if (!ri) + return NULL; + + bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common, ri); + if (!bus) + return NULL; + + pci_bus_size_bridges(bus); + pci_bus_assign_resources(bus); + + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); + + return bus; +} + +/* handle MCFG table entries */ +static __init int handle_mcfg(struct acpi_table_header *header) +{ + struct acpi_table_mcfg *mcfg; + struct acpi_mcfg_allocation *mptr; + struct mcfg_entry *e, *arr; + int i, n; + + if (!header) + return -EINVAL; + + mcfg = (struct acpi_table_mcfg *)header; + mptr = (struct acpi_mcfg_allocation *) &mcfg[1]; + n = (header->length - sizeof(*mcfg)) / sizeof(*mptr); + if (n <= 0 || n > 255) { + pr_err(PREFIX " MCFG has incorrect entries (%d).\n", n); + return -EINVAL; + } + + arr = kcalloc(n, sizeof(*arr), GFP_KERNEL); + if (!arr) + return -ENOMEM; + + for (i = 0, e = arr; i < n; i++, mptr++, e++) { + e->segment = mptr->pci_segment; + e->addr = mptr->address; + e->bus_start = mptr->start_bus_number; + e->bus_end = mptr->end_bus_number; + } + + mcfgsav.cfg = arr; + mcfgsav.size = n; + return 0; +} + +/* Interface called by ACPI - parse and save MCFG table */ +void __init pci_mmcfg_late_init(void) +{ + int err; + + mutex_init(&gen_acpi_pci_lock); + INIT_LIST_HEAD(&gen_acpi_pci_roots); + err = acpi_sfi_table_parse(ACPI_SIG_MCFG, handle_mcfg); + if (err) { + pr_err(PREFIX " Failed to parse MCFG (%d)\n", err); + mcfgsav.size = 0; + } else { + pr_info(PREFIX " MCFG table at %p, %d entries.\n", + mcfgsav.cfg, mcfgsav.size); + } +}

On 09.03.2016 10:13, Tomasz Nowicki wrote:

Hi Bjorn,

Thanks for your pointers! See my comments inline. Aslo, can you please have a look at my previous patch set v4 and check how many of your comments are already addressed there. We may want to back to it then.

https://lkml.org/lkml/2016/2/4/646 Especially patches [0-6] which handle MMCONFIG refactoring.

On 05.03.2016 05:14, Bjorn Helgaas wrote:

...

On Fri, Mar 04, 2016 at 02:05:56PM +0530, Jayachandran Chandrashekaran Nair wrote:

...

On Fri, Mar 4, 2016 at 4:21 AM, Bjorn Helgaas helgaas@kernel.org wrote:

...

Hi Tomasz, Jayachandran, et al,

On Tue, Feb 16, 2016 at 02:53:31PM +0100, Tomasz Nowicki wrote:

...

From: Jayachandran C jchandra@broadcom.com

Move pci_mmcfg_list handling to a drivers/acpi/pci_mcfg.c. This is to share the API and code with ARM64 later. The corresponding declarations are moved from asm/pci_x86.h to linux/pci-acpi.h

As a part of this we introduce three functions that can be implemented by the arch code: pci_mmconfig_map_resource() to map a mcfg entry, pci_mmconfig_unmap_resource to do the corresponding unmap and pci_mmconfig_enabled to see if the arch setup of mcfg entries was successful. We also provide weak implementations of these, which will be used from ARM64. On x86, we retain the old logic by providing platform specific implementation.

This patch is purely rearranging code, it should not have any impact on the logic of MCFG parsing or list handling.

I definitely want to figure out how to make this work well on ARM64. I need to ponder this some more, so these are just some initial thoughts.

My first impression is that (a) the x86 MCFG code is an unmitigated disaster, and (b) we're trying a little too hard to make that mess generic. I think we might be better served if we came up with some cleaner, more generic code that we can use for ARM64 today, and migrate x86 toward that over time.

My concern is that if we elevate the current x86 code to be "arch-independent", we will be perpetuating some interfaces and designs that shouldn't be allowed to escape arch/x86.

I think the major decision is whether to maintain the pci_mmcfg_list for all architectures or not. My initial plan was not to do this because of the mess (basically the ECAM region info should be attached to the pci root and not maintained in a separate list that needs locking), The patch I posted initially https://patchwork.ozlabs.org/patch/553464/ had a much simpler way of handling the MCFG table without using the list.

I agree that ECAM info should be attached to the PCI host controller. That should simplify locking and hot-add and hot-removal of host controllers.

I think pci_mmcfg_list is an implementation detail that may not need to be generic. I certainly don't think it needs to be part of the interface.

...

In x86 case it is not feasible to remove using the pci_mmcfg_list. The only use of it outside is in xen that can be fixed up.

...

Some of the code that moved to drivers/acpi/pci_mcfg.c is not really ACPI-specific, and could potentially be used for non-ACPI bridges that support ECAM. I'd like to see that sort of code moved to a new file like drivers/pci/ecam.c.

There's a little bit of overlap here with the ECAM code in pci-host-generic.c. I'm not sure whether or how to include that, but it's a very good example of how simple this *should* be: probe the host bridge, discover the ECAM region, request the region, ioremap it, done.

I had a similar approach in my initial patchset, please see the patch above. The resource for ECAM is mapped similar to the the way pci-host-generic.c handled it. An additional step I could do was to move the common code (ioremap and mapbus) into a common file and share the code with pci-host-generic.c

...

...

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c new file mode 100644 index 0000000..ea84365 --- /dev/null +++ b/drivers/acpi/pci_mcfg.c ... +int __weak pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg)
  

+{

• struct resource *tmp;
  

• void __iomem *vaddr;
  

• tmp = insert_resource_conflict(&iomem_resource, &mcfg->res);
  

• if (tmp) {
  

•         dev_warn(dev, "MMCONFIG %pR conflicts with %s %pR\n",
  

•                 &mcfg->res, tmp->name, tmp);
  

•         return -EBUSY;
  

• }
  

I think this is a mistake in the x86 MCFG support that we should not carry over to a generic implementation. We should not use the MCFG table for resource reservation because MCFG is not defined by the ACPI spec and an OS need not include support for it. The platform must indicate in some other, more generic way, that ECAM space is reserved. This probably means ECAM space should be declared in a PNP0C02 _CRS method (see the PCI Firmware Spec r3.0, sec 4.1.2, note 2).

We might need some kind of x86-specific quirk that does this, or a pcibios hook or something here; I just don't think it should be generic.

...

+int __init pci_mmconfig_parse_table(void) +{

• return acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
  

+}

I don't like the fact that we parse the entire MCFG table at once here. I think we should look for the information we need when we are claiming a PCI host bridge, e.g., in acpi_pci_root_add(). This might not be a great fit for the way ACPI table management works, but I think it's better to do things on-demand rather than just-in-case.

There is an overhead of looking up this table, and the information available there is very limited (i.e, segment, start_bus, end_bus and address). My approach in the above patch is to save this info into an array at boot time and avoid multiple lookups.

We need to look up MCFG info once per host bridge, so I don't think there's any performance issue here. But we do use acpi_table_parse(), which is __init, and *that* is a reason why we might need to parse the entire MCFG at boot-time. But this is the least of our worries in any case.

...

...

...

diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index 89ab057..e9450ef 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -106,6 +106,39 @@ extern const u8 pci_acpi_dsm_uuid[]; #define RESET_DELAY_DSM 0x08 #define FUNCTION_DELAY_DSM 0x09

+/* common API to maintain list of MCFG regions */ +/* "PCI MMCONFIG %04x [bus %02x-%02x]" */ +#define PCI_MMCFG_RESOURCE_NAME_LEN (22 + 4 + 2 + 2)

+struct pci_mmcfg_region {

• struct list_head list;
  

• struct resource res;
  

• u64 address;
  

• char __iomem *virt;
  

• u16 segment;
  

• u8 start_bus;
  

• u8 end_bus;
  

• char name[PCI_MMCFG_RESOURCE_NAME_LEN];
  

+};

+extern int pci_mmconfig_insert(struct device *dev, u16 seg, u8 start, u8 end,

•                        phys_addr_t addr);
  

+extern int pci_mmconfig_delete(u16 seg, u8 start, u8 end);

+extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus); +extern struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,

•                                                 int end, u64
  

addr); +extern int pci_mmconfig_map_resource(struct device *dev,

• struct pci_mmcfg_region *mcfg);
  

+extern void pci_mmconfig_unmap_resource(struct pci_mmcfg_region *mcfg); +extern int pci_mmconfig_enabled(void); +extern int __init pci_mmconfig_parse_table(void);

+extern struct list_head pci_mmcfg_list;

+#define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) +#define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12)

With the exception of pci_mmconfig_parse_table(), nothing here is ACPI-specific. I'd like to see the PCI ECAM-related interfaces (hopefully not these exact ones, but a more rational set) put in something like include/linux/pci-ecam.h.

...

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I can update this patch to

• drop the pci_mmcfg_list handling from generic case
• move common ECAM code so that it can be shared with pci-host-generic.c

if that is what you are looking for. The code will end up looking much simpler.

I think we should ignore x86 mmconfig for now. It is absurdly complicated and I'm not sure it's fixable. I *do* want to keep drivers/acpi/pci_root.c for all ACPI host bridges, including x86, ia64, and arm64.

So I think we should write generic MCFG and ECAM support from scratch for arm64. Something like this:

• Add an acpi_mcfg_init(), maybe in drivers/acpi/pci_mcfg.c, to be called from acpi_init() to copy MCFG info to something we can access after __init. This would not reserve resources, but probably does have to ioremap() the regions to support raw_pci_read().

As said, ECAM and ACPI specific code was isolated in previous patch. There, I tried to leave x86 complication in arch/x86/ and extract generic functionalities to driver/pci/ecam.c as the library.

...

• Implement raw_pci_read(), which is "special" because ACPI needs it for PCI config access from AML. It's supposed to be "always accessible" and we don't have a struct pci_bus *, so this probably has to use the MCFG copy and the ioremap done above. Maybe it should go in the same file. This is completely independent of the PCI core and PCI data structures.

We were looking for the answer which would justify RAW PCI config accessors being for ARM64 world. Unfortunately, nobody was able to show real use case for ARM64. Do you see the reason we need this? Our conclusion was to leave it empty for ARM64 which in turn makes code simpler. I am not ASWG member while that was under discussion so I will ask Lorenzo to elaborate more on this.

...

• Implement arm64 pci_acpi_scan_root() that calls acpi_pci_root_create() with an .init_info() function that calls acpi_pci_root_get_mcfg_addr() to read _CBA, and if that fails, looks up the bus range in the MCFG copy from above. It should call request_mem_region(). For a region from _CBA, it should call ioremap(). For regions from MCFG it can probably use the ioremap done by acpi_mcfg_init().

Yes, Expanding .init_info() to check for _CBA is good point.

...

 I know acpi_pci_root_add() calls acpi_pci_root_get_mcfg_addr()
 before calling pci_acpi_scan_root(), but I think that's wrong
 because (a) some arches, e.g., ia64, don't use ECAM and (b) _CBA
 and MCFG should be handled in the same place.

 I know calling request_mem_region() here will probably be an
 ordering problem because the PNP0C02 driver hasn't reserved
 resources yet.  But the host bridge driver is using the region and
 it should reserve it.

• If we store the ECAM mapped base address in the sysdata or struct pci_host_bridge, the normal config accessors can use pci_generic_config_read() with a new generic .map_bus() function.

pci_generic_config_{read|write}() is what we want to use, actually we do now, but ECAM region and sysdata association will remove ECAM region lookup step (see patch 09/15 of this series).

...

 On x86, the normal config access path is:

   pci_read(struct pci_bus *, ...)
     raw_pci_read(seg, bus#, ...)
  raw_pci_ext_ops->read(seg, bus#, ...)
    pci_mmcfg_read(seg, bus#, ...)
      pci_dev_base
        pci_mmconfig_lookup(seg, bus#)

 I think this is somewhat backwards because we start with a pci_bus
 pointer, so we *could* have a nice simple bus-specific accessor,
 but we throw that pointer away, so pci_mmcfg_read() has to start
 over and look up the ECAM offset from scratch, which makes it all
 unnecessarily complicated.

As you pointed out raw_pci_{read|write} make things complicated, so IMO we should either say they are absolutely necessary (and then think how to simplify it) or just use simple bus-specific accessor (patch 02/15) e.g. for ARM64.

Any comments appreciated.

Hi Bjorn,

Kindly reminder. I would like to move on with this patch set. Can you please comments on it so that we could decide which way to go.

Regards, Tomasz

On Tue, Feb 16, 2016 at 02:53:32PM +0100, Tomasz Nowicki wrote:

Lets keep RAW ACPI PCI config space accessors empty by default, since we are note sure if they are necessary accross all archs. Once we sort this out, we can provide generic version or let architectures to overwrite, like now x86.

"ACPICA code requires raw PCI bus accessors in order to give AML access to PCI_Config regions in platforms where they are actually used. The raw PCI bus accessors implementation is arch-dependent, therefore this patch adds a weak generic implementation (for now empty but can be generalized if common functionality is found among arches) allowing arches where PCI_Config regions are currently required to override it (eg x86) as needed and providing at the same time default stubs for arches that do not require them".

?

Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Signed-off-by: Tomasz Nowicki tn@semihalf.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Jeremy Linton jeremy.linton@arm.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index ea84365..0467b00 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -21,6 +21,26 @@ static DEFINE_MUTEX(pci_mmcfg_lock); LIST_HEAD(pci_mmcfg_list); +/*

• • raw_pci_read/write - raw ACPI PCI config space accessors.
• • By defauly (__weak) these accessors are empty and should be overwritten

s/defauly/default

• • by architectures which support operations on ACPI PCI_Config regions,
• • see osl.c file.

Add the path or remove the file reference.

• */

+int __weak raw_pci_read(unsigned int domain, unsigned int bus,

• 	unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return PCIBIOS_DEVICE_NOT_FOUND;

+}

+int __weak raw_pci_write(unsigned int domain, unsigned int bus,

• 	 unsigned int devfn, int reg, int len, u32 val)
  

+{

• return PCIBIOS_DEVICE_NOT_FOUND;

+}

static void list_add_sorted(struct pci_mmcfg_region *new) { struct pci_mmcfg_region *cfg;

Note: this patch is not strictly required, but it is nice because it removes the raw/dumb/empty accessors from ARM64 code (where they do not belong), so:

Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

On Tue, Feb 16, 2016 at 02:53:39PM +0100, Tomasz Nowicki wrote:

We use generic accessors from access.c by default. However, we already know platforms that need special handling while accessing to PCI config space. These platforms will need different accessors set matched against platform ID, domain, bus touple. Therefore we are going to add (in future) DECLARE_ACPI_MCFG_FIXUP which will register platform specific custom accessors. For now, we let pci_mcfg_get_ops to take acpi_pci_root structure as an arguments and left some space for quirk matching algorithm.

You should not describe the future (because you do not know if/when that will be implemented), you should describe what the patch does in its current form.

"This patch implements MCFG based PCI bus operations through MCFG map function and generic PCI accessors".

Signed-off-by: Tomasz Nowicki tn@semihalf.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_mcfg.c | 30 ++++++++++++++++++++++++++++++ include/linux/pci-acpi.h | 12 ++++++++++++ 2 files changed, 42 insertions(+)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 3282f2a..0062257 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -41,6 +41,36 @@ int __weak raw_pci_write(unsigned int domain, unsigned int bus, return PCIBIOS_DEVICE_NOT_FOUND; } +void __iomem * +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset) +{

• struct pci_mmcfg_region *cfg;
• cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
• if (cfg && cfg->virt)

• return cfg->virt +
  

• 	(PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
  

• 	offset;
  

• return NULL;

+}

+/* Default generic PCI config accessors */ +static struct pci_ops default_pci_mcfg_ops = {

• .map_bus = pci_mcfg_dev_base,
• .read = pci_generic_config_read,
• .write = pci_generic_config_write,

+};

Nit: s/default_pci_mcfg_ops/pci_mcfg_ops

+struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root) +{

• /*

• * TODO: Match against platform specific quirks and return
  

• * corresponding PCI config space accessor set.
  

• */
  

Remove this comment, see above.

• return &default_pci_mcfg_ops;

See above.

+}

static void list_add_sorted(struct pci_mmcfg_region *new) { struct pci_mmcfg_region *cfg; diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h index b4f87ba9..3dc6a8c 100644 --- a/include/linux/pci-acpi.h +++ b/include/linux/pci-acpi.h @@ -141,6 +141,18 @@ extern struct list_head pci_mmcfg_list; #define PCI_MMCFG_BUS_OFFSET(bus) ((bus) << 20) #define PCI_MMCFG_OFFSET(bus, devfn) ((bus) << 20 | (devfn) << 12) +#ifdef CONFIG_PCI_MMCONFIG +extern struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root); +extern void __iomem *pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn,

• 		       int offset);
  

+#else +static inline struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root) +{ return NULL; } +static inline void __iomem *pci_mcfg_dev_base(struct pci_bus *bus,

• 			      unsigned int devfn, int offset)
  

+{ return NULL; } +#endif

#else /* CONFIG_ACPI */ static inline void acpi_pci_add_bus(struct pci_bus *bus) { } static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }

I think it can even be squashed, anyway:

Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

On 18.03.2016 16:49, Mark Salter wrote:

On Tue, 2016-02-16 at 14:53 +0100, Tomasz Nowicki wrote:

...

Some platforms may not be fully compliant with generic set of PCI config accessors. For these cases we implement the way to overwrite accessors set prior to PCI buses enumeration. Algorithm traverses available quirk list, matches against <platform ID (DMI), domain, bus number> tuple and returns corresponding accessors. All quirks can be defined using: DECLARE_ACPI_MCFG_FIXUP() and kept self contained. Example,

static const struct dmi_system_id foo_dmi[] = { { .ident = "<Platform ident string>", .callback = <handler>, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "<system vendor>"), DMI_MATCH(DMI_PRODUCT_NAME, "<product name>"), DMI_MATCH(DMI_PRODUCT_VERSION, "product version"), }, }, { } };

static struct pci_ops foo_ecam_pci_ops = { .map_bus = pci_mcfg_dev_base, .read = foo_ecam_config_read, .write = foo_ecam_config_write, }; DECLARE_ACPI_MCFG_FIXUP(foo_dmi, NULL, &foo_ecam_pci_ops, <domain_nr>, <bus_nr>);

More custom (non-DMI) matching can be done via an extra call. Note that there is possibility to assign quirk related private data to root->sysdata which will be available along read/wriate accessor, example:

static int boo_match(struct pci_mcfg_fixup *fixup, struct acpi_pci_root *root) { return [condition] ? 1 : 0; }

int boo_ecam_config_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val) { struct acpi_pci_root *root = bus->sysdata; struct boo_priv_data *boo_data = root->sysdata;

     [..]

}

static struct pci_ops boo_ecam_pci_ops = { .map_bus = pci_mcfg_dev_base, .read = boo_ecam_config_read, .write = boo_ecam_config_write, }; DECLARE_ACPI_MCFG_FIXUP(NULL, boo_match, &boo_ecam_pci_ops, <domain_nr>, <bus_nr>);

Signed-off-by: Tomasz Nowicki tn@semihalf.com Tested-by: Duc Dang dhdang@apm.com Tested-by: Dongdong Liu liudongdong3@huawei.com Tested-by: Hanjun Guo hanjun.guo@linaro.org Tested-by: Graeme Gregory graeme.gregory@linaro.org Tested-by: Sinan Kaya okaya@codeaurora.org

---

drivers/acpi/pci_mcfg.c | 32 ++++++++++++++++++++++++++++++-- include/acpi/acpi_bus.h | 1 + include/asm-generic/vmlinux.lds.h | 7 +++++++ include/linux/pci-acpi.h | 18 ++++++++++++++++++ 4 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 0062257..b343547 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -41,6 +41,29 @@ int __weak raw_pci_write(unsigned int domain, unsigned int bus, return PCIBIOS_DEVICE_NOT_FOUND; }

+extern struct pci_mcfg_fixup __start_acpi_mcfg_fixups[]; +extern struct pci_mcfg_fixup __end_acpi_mcfg_fixups[];

+static struct pci_ops *pci_mcfg_check_quirks(struct acpi_pci_root *root) +{

• struct pci_mcfg_fixup *f;
• int bus_num = root->secondary.start;
• int domain = root->segment;
• /*

• * First match against PCI topology <domain:bus> then use DMI or
  

• * custom match handler.
  

• */
  

• for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {

• if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
  

•     (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
  

•     (f->system ? dmi_check_system(f->system) : 1 &&
  

•      f->match ? f->match(f, root) : 1))
  

The parens are not quite right here ^^^^ If dmi_check_system() returns true, f->match won't get called.

This should be: (f->system ? dmi_check_system(f->system) : 1) && f->match ? f->match(f, root) : 1)

Well spotted. Thanks!

Tomasz

[+ Duc, this needs testing on DT PCI hosts that do not call pci_fixup_irqs()]

On Tue, Feb 16, 2016 at 02:53:44PM +0100, Tomasz Nowicki wrote:

Subject is wrong, leftover from previous posting (ie you do not allocate at device enable anymore).

This is the last step before enabling generic ACPI PCI host controller for ARM64. We need to take care of legacy IRQ mapping for non-MSI(X)

You do not check MSIs anymore.

PCI devices. pcibios_alloc_irq() evaluation is not sensitive to ACPI device enumeration order, so it is the best place to assign device's IRQs for ACPI boot method. Also, it does not hurt DT to be initialized form the same place.

NOTE: *This is going to be temporary solution*. There is ongoing work which aims for cleaning legacy IRQ allocation from arch specific code. We can consider this patch as the necessary evil which will be removed once cleanup series lands in mailnline in the near future.

"To enable PCI legacy IRQs on platforms booting with ACPI, arch code should include ACPI specific callbacks that parse and set-up the device IRQ number, equivalent to the DT boot path. Owing to the current ACPI core scan handlers implementation, ACPI PCI legacy IRQs bindings cannot be parsed at device add time, since that would trigger ACPI scan handlers ordering issues depending on how the ACPI tables are defined.

To solve this problem and consolidate FW PCI legacy IRQs parsing in one single pcibios callback (pending final removal), this patch moves DT PCI IRQ parsing to the pcibios_alloc_irq() callback (called by PCI core code at device probe time) and adds ACPI PCI legacy IRQs parsing to the same callback too, so that FW PCI legacy IRQs parsing is confined in one single arch callback that can be easily removed when code parsing PCI legacy IRQs is consolidated and moved to core PCI code".

?

Signed-off-by: Tomasz Nowicki tn@semihalf.com Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

---

arch/arm64/kernel/pci.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index 023b983..6e77e1b 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -52,11 +52,16 @@ int pcibios_enable_device(struct pci_dev *dev, int mask) } /*

• • Try to assign the IRQ number from DT when adding a new device

• • Try to assign the IRQ number when probing a new device
  */

-int pcibios_add_device(struct pci_dev *dev) +int pcibios_alloc_irq(struct pci_dev *dev) {

• dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);

• if (acpi_disabled)

• dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
  

+#ifdef CONFIG_ACPI

• else

• return acpi_pci_irq_enable(dev);
  

+#endif return 0; }

It is good this code is now in one single function, it will be removed more quickly :D

So pending APM X-gene DT testing:

Reviewed-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com