[PATCH RFC 0/2] PCI, x86: update MMCFG information when hot-plugging PCI host bridges

[PATCH RFC 0/2] PCI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

This patchset enhance pci_root driver to update MMCFG information when
hot-plugging PCI root bridges. It applies to Yinghai's tree at
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug

The second patch is based on Taku Izumi work with some enhancements to
correctly handle PCI host bridges without _CBA method.

Jiang Liu (2):
  PCI,x86: introduce new MMCFG interfaces to support PCI host bridge
    hotplug
  PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host
    bridges

 arch/x86/include/asm/pci_x86.h |    4 +
 arch/x86/pci/acpi.c            |   58 +++++++++++++
 arch/x86/pci/mmconfig-shared.c |  186 +++++++++++++++++++++++++++++++---------
 arch/x86/pci/mmconfig_32.c     |   28 ++++++-
 arch/x86/pci/mmconfig_64.c     |   35 +++++++-
 drivers/acpi/pci_root.c        |   20 +++++
 include/acpi/acnames.h         |    1 +
 include/linux/pci-acpi.h       |    3 +
 8 files changed, 288 insertions(+), 47 deletions(-)

-- 
1.7.5.4

Re: [PATCH RFC 0/2] PCI, x86: update MMCFG information when hot-plugging PCI host bridges

[Bjorn Helgaas]

On Sun, Apr 8, 2012 at 11:12 AM, Jiang Liu <liuj97@gmail.com> wrote:
> This patchset enhance pci_root driver to update MMCFG information when
> hot-plugging PCI root bridges. It applies to Yinghai's tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-hotplug
>
> The second patch is based on Taku Izumi work with some enhancements to
> correctly handle PCI host bridges without _CBA method.
>
> Jiang Liu (2):
>  PCI,x86: introduce new MMCFG interfaces to support PCI host bridge
>    hotplug
>  PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host
>    bridges
>
>  arch/x86/include/asm/pci_x86.h |    4 +
>  arch/x86/pci/acpi.c            |   58 +++++++++++++
>  arch/x86/pci/mmconfig-shared.c |  186 +++++++++++++++++++++++++++++++---------
>  arch/x86/pci/mmconfig_32.c     |   28 ++++++-
>  arch/x86/pci/mmconfig_64.c     |   35 +++++++-
>  drivers/acpi/pci_root.c        |   20 +++++
>  include/acpi/acnames.h         |    1 +
>  include/linux/pci-acpi.h       |    3 +
>  8 files changed, 288 insertions(+), 47 deletions(-)

Jiang, I assume this series is superceded by "[PATCH V4 0/6] PCI, x86:
update MMCFG information when hot-plugging PCI host bridges".  Let me
know if that's incorrect.

Bjorn

[PATCH 1/2] PCI,x86: introduce new MMCFG interfaces to support PCI host bridge hotplug

[Jiang Liu]

This patch introduces two new interfaces, pci_mmconfig_insert() and
pci_mmconfig_delete(), to support PCI host bridge hotplug on x86 platforms.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/include/asm/pci_x86.h |    4 +
 arch/x86/pci/mmconfig-shared.c |  186 +++++++++++++++++++++++++++++++---------
 arch/x86/pci/mmconfig_32.c     |   28 ++++++-
 arch/x86/pci/mmconfig_64.c     |   35 +++++++-
 4 files changed, 206 insertions(+), 47 deletions(-)

diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index b3a5317..ba8570c 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -135,6 +135,10 @@ struct pci_mmcfg_region {
 
 extern int __init pci_mmcfg_arch_init(void);
 extern void __init pci_mmcfg_arch_free(void);
+extern int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
+extern void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
+extern int __devinit pci_mmconfig_insert(int seg, int start, int end, u64 addr);
+extern int __devinit pci_mmconfig_delete(int seg, int start, int end, u64 addr);
 extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
 
 extern struct list_head pci_mmcfg_list;
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 301e325..dda9470 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -17,6 +17,8 @@
 #include <linux/bitmap.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/rculist.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 #include <asm/acpi.h>
@@ -45,24 +47,25 @@ static __init void free_all_mmcfg(void)
 		pci_mmconfig_remove(cfg);
 }
 
-static __init void list_add_sorted(struct pci_mmcfg_region *new)
+static __devinit 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(cfg, &pci_mmcfg_list, list) {
+	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(&new->list, &cfg->list);
+			list_add_tail_rcu(&new->list, &cfg->list);
 			return;
 		}
 	}
-	list_add_tail(&new->list, &pci_mmcfg_list);
+	list_add_tail_rcu(&new->list, &pci_mmcfg_list);
 }
 
-static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
-							int end, u64 addr)
+static __devinit struct pci_mmcfg_region *pci_mmconfig_alloc(int segment,
+							     int start,
+							     int end, u64 addr)
 {
 	struct pci_mmcfg_region *new;
 	struct resource *res;
@@ -79,8 +82,6 @@ static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
 	new->start_bus = start;
 	new->end_bus = end;
 
-	list_add_sorted(new);
-
 	res = &new->res;
 	res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
 	res->end = addr + PCI_MMCFG_BUS_OFFSET(end + 1) - 1;
@@ -96,11 +97,23 @@ static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
 	return new;
 }
 
+static __init 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)
+		list_add_sorted(new);
+
+	return new;
+}
+
 struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
 {
 	struct pci_mmcfg_region *cfg;
 
-	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
 		if (cfg->segment == segment &&
 		    cfg->start_bus <= bus && bus <= cfg->end_bus)
 			return cfg;
@@ -364,8 +377,8 @@ static void __init pci_mmcfg_insert_resources(void)
 	pci_mmcfg_resources_inserted = 1;
 }
 
-static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
-					      void *data)
+static acpi_status __devinit check_mcfg_resource(struct acpi_resource *res,
+						 void *data)
 {
 	struct resource *mcfg_res = data;
 	struct acpi_resource_address64 address;
@@ -401,8 +414,8 @@ static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
 	return AE_OK;
 }
 
-static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
-		void *context, void **rv)
+static acpi_status __devinit find_mboard_resource(acpi_handle handle, u32 lvl,
+						  void *context, void **rv)
 {
 	struct resource *mcfg_res = context;
 
@@ -415,7 +428,7 @@ static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
 	return AE_OK;
 }
 
-static int __init is_acpi_reserved(u64 start, u64 end, unsigned not_used)
+static int __devinit is_acpi_reserved(u64 start, u64 end, unsigned not_used)
 {
 	struct resource mcfg_res;
 
@@ -434,8 +447,9 @@ static int __init is_acpi_reserved(u64 start, u64 end, unsigned not_used)
 
 typedef int (*check_reserved_t)(u64 start, u64 end, unsigned type);
 
-static int __init is_mmconf_reserved(check_reserved_t is_reserved,
-				    struct pci_mmcfg_region *cfg, int with_e820)
+static int __devinit is_mmconf_reserved(check_reserved_t is_reserved,
+					struct pci_mmcfg_region *cfg,
+					int with_e820)
 {
 	u64 addr = cfg->res.start;
 	u64 size = resource_size(&cfg->res);
@@ -474,39 +488,38 @@ static int __init is_mmconf_reserved(check_reserved_t is_reserved,
 	return valid;
 }
 
+static int __devinit pci_mmcfg_check_reserved(struct pci_mmcfg_region *cfg,
+					      int early)
+{
+	if (!early && !acpi_disabled) {
+		if (is_mmconf_reserved(is_acpi_reserved, cfg, 0))
+			return 1;
+		else
+			printk(KERN_ERR FW_BUG PREFIX
+			       "MMCONFIG at %pR not reserved in "
+			       "ACPI motherboard resources\n",
+			       &cfg->res);
+	}
+
+	/* Don't try to do this check unless configuration
+	   type 1 is available. how about type 2 ?*/
+	if (raw_pci_ops)
+		return is_mmconf_reserved(e820_all_mapped, cfg, 1);
+
+	return 0;
+}
+
 static void __init pci_mmcfg_reject_broken(int early)
 {
 	struct pci_mmcfg_region *cfg;
 
 	list_for_each_entry(cfg, &pci_mmcfg_list, list) {
-		int valid = 0;
-
-		if (!early && !acpi_disabled) {
-			valid = is_mmconf_reserved(is_acpi_reserved, cfg, 0);
-
-			if (valid)
-				continue;
-			else
-				printk(KERN_ERR FW_BUG PREFIX
-				       "MMCONFIG at %pR not reserved in "
-				       "ACPI motherboard resources\n",
-				       &cfg->res);
+		if (pci_mmcfg_check_reserved(cfg, early) == 0) {
+			printk(KERN_INFO PREFIX "not using MMCONFIG\n");
+			free_all_mmcfg();
+			return;
 		}
-
-		/* Don't try to do this check unless configuration
-		   type 1 is available. how about type 2 ?*/
-		if (raw_pci_ops)
-			valid = is_mmconf_reserved(e820_all_mapped, cfg, 1);
-
-		if (!valid)
-			goto reject;
 	}
-
-	return;
-
-reject:
-	printk(KERN_INFO PREFIX "not using MMCONFIG\n");
-	free_all_mmcfg();
 }
 
 static int __initdata known_bridge;
@@ -665,3 +678,92 @@ static int __init pci_mmcfg_late_insert_resources(void)
  * with other system resources.
  */
 late_initcall(pci_mmcfg_late_insert_resources);
+
+static DEFINE_MUTEX(pci_mmcfg_lock);
+
+/* Add MMCFG information for hot-added host bridges at runtime */
+int __devinit pci_mmconfig_insert(int segment, int start, int end, u64 addr)
+{
+	int rc;
+	struct pci_mmcfg_region *cfg = NULL;
+
+	if (addr == 0 || segment < 0 || segment > USHRT_MAX ||
+	    start < 0 || start > 255 || end < start || end > 255)
+		return -EINVAL;
+
+	mutex_lock(&pci_mmcfg_lock);
+	cfg = pci_mmconfig_lookup(segment, start);
+	if (cfg) {
+		if (cfg->start_bus == start && cfg->end_bus == end &&
+		    cfg->address == addr) {
+			rc = -EEXIST;
+		} else {
+			rc = -EINVAL;
+			printk(KERN_WARNING PREFIX
+			       "MMCONFIG for domain %04x [bus %02x-%02x] "
+			       "conflicts with domain %04x [bus %02x-%02x]\n",
+			       segment, start, end,
+			       cfg->segment, cfg->start_bus, cfg->end_bus);
+		}
+		goto out;
+	}
+
+	cfg = pci_mmconfig_alloc(segment, start, end, addr);
+	if (cfg == NULL) {
+		rc = -ENOMEM;
+	} else if (!pci_mmcfg_check_reserved(cfg, 0)) {
+		rc = -EINVAL;
+		printk(KERN_WARNING PREFIX
+		       "MMCONFIG for domain %04x [bus %02x-%02x] "
+		       "isn't reserved\n", segment, start, end);
+	} else if (insert_resource(&iomem_resource, &cfg->res)) {
+		rc = -EBUSY;
+		printk(KERN_WARNING PREFIX
+		       "failed to insert resource for domain "
+		       "%04x [bus %02x-%02x]\n", segment, start, end);
+	} else if (pci_mmcfg_arch_map(cfg)) {
+		rc = -EBUSY;
+		printk(KERN_WARNING PREFIX
+		       "failed to map resource for domain "
+		       "%04x [bus %02x-%02x]\n", segment, start, end);
+	} else {
+		list_add_sorted(cfg);
+		cfg = NULL;
+		rc = 0;
+	}
+
+	if (cfg) {
+		if (cfg->res.parent)
+			release_resource(&cfg->res);
+		kfree(cfg);
+	}
+
+out:
+	mutex_unlock(&pci_mmcfg_lock);
+
+	return rc;
+}
+
+/* Delete MMCFG information at runtime */
+int __devinit pci_mmconfig_delete(int segment, int start, int end, u64 addr)
+{
+	struct pci_mmcfg_region *cfg;
+
+	mutex_lock(&pci_mmcfg_lock);
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+		if (cfg->segment == segment && cfg->start_bus == start &&
+		    cfg->end_bus == end && cfg->address == addr) {
+			list_del_rcu(&cfg->list);
+			synchronize_rcu();
+			pci_mmcfg_arch_unmap(cfg);
+			if (cfg->res.parent)
+				release_resource(&cfg->res);
+			mutex_unlock(&pci_mmcfg_lock);
+			kfree(cfg);
+			return 0;
+		}
+	}
+	mutex_unlock(&pci_mmcfg_lock);
+
+	return -ENOENT;
+}
diff --git a/arch/x86/pci/mmconfig_32.c b/arch/x86/pci/mmconfig_32.c
index 5372e86..c2756de 100644
--- a/arch/x86/pci/mmconfig_32.c
+++ b/arch/x86/pci/mmconfig_32.c
@@ -11,6 +11,7 @@
 
 #include <linux/pci.h>
 #include <linux/init.h>
+#include <linux/rcupdate.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 #include <acpi/acpi.h>
@@ -60,9 +61,12 @@ err:		*value = -1;
 		return -EINVAL;
 	}
 
+	rcu_read_lock();
 	base = get_base_addr(seg, bus, devfn);
-	if (!base)
+	if (!base) {
+		rcu_read_unlock();
 		goto err;
+	}
 
 	raw_spin_lock_irqsave(&pci_config_lock, flags);
 
@@ -80,6 +84,7 @@ err:		*value = -1;
 		break;
 	}
 	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
+	rcu_read_unlock();
 
 	return 0;
 }
@@ -93,9 +98,12 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 	if ((bus > 255) || (devfn > 255) || (reg > 4095))
 		return -EINVAL;
 
+	rcu_read_lock();
 	base = get_base_addr(seg, bus, devfn);
-	if (!base)
+	if (!base) {
+		rcu_read_unlock();
 		return -EINVAL;
+	}
 
 	raw_spin_lock_irqsave(&pci_config_lock, flags);
 
@@ -113,6 +121,7 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 		break;
 	}
 	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
+	rcu_read_unlock();
 
 	return 0;
 }
@@ -132,3 +141,18 @@ int __init pci_mmcfg_arch_init(void)
 void __init pci_mmcfg_arch_free(void)
 {
 }
+
+int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
+{
+	return 0;
+}
+
+void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+{
+	unsigned long flags;
+
+	/* Invalidate the cached mmcfg map entry. */
+	raw_spin_lock_irqsave(&pci_config_lock, flags);
+	mmcfg_last_accessed_device = 0;
+	raw_spin_unlock_irqrestore(&pci_config_lock, flags);
+}
diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
index 915a493..6822dd6 100644
--- a/arch/x86/pci/mmconfig_64.c
+++ b/arch/x86/pci/mmconfig_64.c
@@ -9,6 +9,7 @@
 #include <linux/init.h>
 #include <linux/acpi.h>
 #include <linux/bitmap.h>
+#include <linux/rcupdate.h>
 #include <asm/e820.h>
 #include <asm/pci_x86.h>
 
@@ -34,9 +35,12 @@ err:		*value = -1;
 		return -EINVAL;
 	}
 
+	rcu_read_lock();
 	addr = pci_dev_base(seg, bus, devfn);
-	if (!addr)
+	if (!addr) {
+		rcu_read_unlock();
 		goto err;
+	}
 
 	switch (len) {
 	case 1:
@@ -49,6 +53,7 @@ err:		*value = -1;
 		*value = mmio_config_readl(addr + reg);
 		break;
 	}
+	rcu_read_unlock();
 
 	return 0;
 }
@@ -62,9 +67,12 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
 		return -EINVAL;
 
+	rcu_read_lock();
 	addr = pci_dev_base(seg, bus, devfn);
-	if (!addr)
+	if (!addr) {
+		rcu_read_unlock();
 		return -EINVAL;
+	}
 
 	switch (len) {
 	case 1:
@@ -77,6 +85,7 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
 		mmio_config_writel(addr + reg, value);
 		break;
 	}
+	rcu_read_unlock();
 
 	return 0;
 }
@@ -86,7 +95,7 @@ static const struct pci_raw_ops pci_mmcfg = {
 	.write =	pci_mmcfg_write,
 };
 
-static void __iomem * __init mcfg_ioremap(struct pci_mmcfg_region *cfg)
+static void __iomem * __devinit mcfg_ioremap(struct pci_mmcfg_region *cfg)
 {
 	void __iomem *addr;
 	u64 start, size;
@@ -129,3 +138,23 @@ void __init pci_mmcfg_arch_free(void)
 		}
 	}
 }
+
+int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
+{
+	cfg->virt = mcfg_ioremap(cfg);
+	if (!cfg->virt) {
+		printk(KERN_ERR PREFIX "can't map MMCONFIG at %pR\n",
+		       &cfg->res);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+{
+	if (cfg && cfg->virt) {
+		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
+		cfg->virt = NULL;
+	}
+}
-- 
1.7.5.4

Re: [PATCH 1/2] PCI,x86: introduce new MMCFG interfaces to support PCI host bridge hotplug

[Yinghai Lu]

On Sun, Apr 8, 2012 at 10:12 AM, Jiang Liu <liuj97@gmail.com> wrote:
> This patch introduces two new interfaces, pci_mmconfig_insert() and
> pci_mmconfig_delete(), to support PCI host bridge hotplug on x86 platforms.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  arch/x86/include/asm/pci_x86.h |    4 +
>  arch/x86/pci/mmconfig-shared.c |  186 +++++++++++++++++++++++++++++++---------
>  arch/x86/pci/mmconfig_32.c     |   28 ++++++-
>  arch/x86/pci/mmconfig_64.c     |   35 +++++++-
>  4 files changed, 206 insertions(+), 47 deletions(-)

this patch is way too big. at least could split it to five patches.

>
> diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
> index b3a5317..ba8570c 100644
> --- a/arch/x86/include/asm/pci_x86.h
> +++ b/arch/x86/include/asm/pci_x86.h
> @@ -135,6 +135,10 @@ struct pci_mmcfg_region {
>
>  extern int __init pci_mmcfg_arch_init(void);
>  extern void __init pci_mmcfg_arch_free(void);
> +extern int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
> +extern void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
> +extern int __devinit pci_mmconfig_insert(int seg, int start, int end, u64 addr);
> +extern int __devinit pci_mmconfig_delete(int seg, int start, int end, u64 addr);
>  extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
>
>  extern struct list_head pci_mmcfg_list;
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 301e325..dda9470 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -17,6 +17,8 @@
>  #include <linux/bitmap.h>
>  #include <linux/dmi.h>
>  #include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/rculist.h>
>  #include <asm/e820.h>
>  #include <asm/pci_x86.h>
>  #include <asm/acpi.h>
> @@ -45,24 +47,25 @@ static __init void free_all_mmcfg(void)
>                pci_mmconfig_remove(cfg);
>  }
>
> -static __init void list_add_sorted(struct pci_mmcfg_region *new)
> +static __devinit 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(cfg, &pci_mmcfg_list, list) {
> +       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(&new->list, &cfg->list);
> +                       list_add_tail_rcu(&new->list, &cfg->list);
>                        return;
>                }
>        }
> -       list_add_tail(&new->list, &pci_mmcfg_list);
> +       list_add_tail_rcu(&new->list, &pci_mmcfg_list);
>  }
>
> -static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
> -                                                       int end, u64 addr)
> +static __devinit struct pci_mmcfg_region *pci_mmconfig_alloc(int segment,
> +                                                            int start,
> +                                                            int end, u64 addr)
>  {
>        struct pci_mmcfg_region *new;
>        struct resource *res;
> @@ -79,8 +82,6 @@ static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
>        new->start_bus = start;
>        new->end_bus = end;
>
> -       list_add_sorted(new);
> -
>        res = &new->res;
>        res->start = addr + PCI_MMCFG_BUS_OFFSET(start);
>        res->end = addr + PCI_MMCFG_BUS_OFFSET(end + 1) - 1;
> @@ -96,11 +97,23 @@ static __init struct pci_mmcfg_region *pci_mmconfig_add(int segment, int start,
>        return new;
>  }
>
> +static __init 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)
> +               list_add_sorted(new);
> +
> +       return new;
> +}
> +

above two change could be in separated patch.

>  struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
>  {
>        struct pci_mmcfg_region *cfg;
>
> -       list_for_each_entry(cfg, &pci_mmcfg_list, list)
> +       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
>                if (cfg->segment == segment &&
>                    cfg->start_bus <= bus && bus <= cfg->end_bus)
>                        return cfg;
> @@ -364,8 +377,8 @@ static void __init pci_mmcfg_insert_resources(void)
>        pci_mmcfg_resources_inserted = 1;
>  }
>
> -static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
> -                                             void *data)
> +static acpi_status __devinit check_mcfg_resource(struct acpi_resource *res,
> +                                                void *data)
>  {
>        struct resource *mcfg_res = data;
>        struct acpi_resource_address64 address;
> @@ -401,8 +414,8 @@ static acpi_status __init check_mcfg_resource(struct acpi_resource *res,
>        return AE_OK;
>  }
>
> -static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
> -               void *context, void **rv)
> +static acpi_status __devinit find_mboard_resource(acpi_handle handle, u32 lvl,
> +                                                 void *context, void **rv)
>  {
>        struct resource *mcfg_res = context;
>
> @@ -415,7 +428,7 @@ static acpi_status __init find_mboard_resource(acpi_handle handle, u32 lvl,
>        return AE_OK;
>  }
>
> -static int __init is_acpi_reserved(u64 start, u64 end, unsigned not_used)
> +static int __devinit is_acpi_reserved(u64 start, u64 end, unsigned not_used)
>  {
>        struct resource mcfg_res;
>
> @@ -434,8 +447,9 @@ static int __init is_acpi_reserved(u64 start, u64 end, unsigned not_used)
>
>  typedef int (*check_reserved_t)(u64 start, u64 end, unsigned type);
>
> -static int __init is_mmconf_reserved(check_reserved_t is_reserved,
> -                                   struct pci_mmcfg_region *cfg, int with_e820)
> +static int __devinit is_mmconf_reserved(check_reserved_t is_reserved,
> +                                       struct pci_mmcfg_region *cfg,
> +                                       int with_e820)
>  {
>        u64 addr = cfg->res.start;
>        u64 size = resource_size(&cfg->res);
> @@ -474,39 +488,38 @@ static int __init is_mmconf_reserved(check_reserved_t is_reserved,
>        return valid;
>  }
>
> +static int __devinit pci_mmcfg_check_reserved(struct pci_mmcfg_region *cfg,
> +                                             int early)
> +{
> +       if (!early && !acpi_disabled) {
> +               if (is_mmconf_reserved(is_acpi_reserved, cfg, 0))
> +                       return 1;
> +               else
> +                       printk(KERN_ERR FW_BUG PREFIX
> +                              "MMCONFIG at %pR not reserved in "
> +                              "ACPI motherboard resources\n",
> +                              &cfg->res);
> +       }
> +
> +       /* Don't try to do this check unless configuration
> +          type 1 is available. how about type 2 ?*/
> +       if (raw_pci_ops)
> +               return is_mmconf_reserved(e820_all_mapped, cfg, 1);
> +
> +       return 0;
> +}
> +
>  static void __init pci_mmcfg_reject_broken(int early)
>  {
>        struct pci_mmcfg_region *cfg;
>
>        list_for_each_entry(cfg, &pci_mmcfg_list, list) {
> -               int valid = 0;
> -
> -               if (!early && !acpi_disabled) {
> -                       valid = is_mmconf_reserved(is_acpi_reserved, cfg, 0);
> -
> -                       if (valid)
> -                               continue;
> -                       else
> -                               printk(KERN_ERR FW_BUG PREFIX
> -                                      "MMCONFIG at %pR not reserved in "
> -                                      "ACPI motherboard resources\n",
> -                                      &cfg->res);
> +               if (pci_mmcfg_check_reserved(cfg, early) == 0) {
> +                       printk(KERN_INFO PREFIX "not using MMCONFIG\n");
> +                       free_all_mmcfg();
> +                       return;
>                }
> -
> -               /* Don't try to do this check unless configuration
> -                  type 1 is available. how about type 2 ?*/
> -               if (raw_pci_ops)
> -                       valid = is_mmconf_reserved(e820_all_mapped, cfg, 1);
> -
> -               if (!valid)
> -                       goto reject;
>        }
> -
> -       return;
> -
> -reject:
> -       printk(KERN_INFO PREFIX "not using MMCONFIG\n");
> -       free_all_mmcfg();
>  }
>
>  static int __initdata known_bridge;

above two changes could be in separated patch.

> @@ -665,3 +678,92 @@ static int __init pci_mmcfg_late_insert_resources(void)
>  * with other system resources.
>  */
>  late_initcall(pci_mmcfg_late_insert_resources);
> +
> +static DEFINE_MUTEX(pci_mmcfg_lock);
> +
> +/* Add MMCFG information for hot-added host bridges at runtime */
> +int __devinit pci_mmconfig_insert(int segment, int start, int end, u64 addr)
> +{
> +       int rc;
> +       struct pci_mmcfg_region *cfg = NULL;
> +
> +       if (addr == 0 || segment < 0 || segment > USHRT_MAX ||
> +           start < 0 || start > 255 || end < start || end > 255)
> +               return -EINVAL;
> +
> +       mutex_lock(&pci_mmcfg_lock);
> +       cfg = pci_mmconfig_lookup(segment, start);
> +       if (cfg) {
> +               if (cfg->start_bus == start && cfg->end_bus == end &&
> +                   cfg->address == addr) {
> +                       rc = -EEXIST;
> +               } else {
> +                       rc = -EINVAL;
> +                       printk(KERN_WARNING PREFIX
> +                              "MMCONFIG for domain %04x [bus %02x-%02x] "
> +                              "conflicts with domain %04x [bus %02x-%02x]\n",
> +                              segment, start, end,
> +                              cfg->segment, cfg->start_bus, cfg->end_bus);
> +               }
> +               goto out;
> +       }
> +
> +       cfg = pci_mmconfig_alloc(segment, start, end, addr);
> +       if (cfg == NULL) {
> +               rc = -ENOMEM;
> +       } else if (!pci_mmcfg_check_reserved(cfg, 0)) {
> +               rc = -EINVAL;
> +               printk(KERN_WARNING PREFIX
> +                      "MMCONFIG for domain %04x [bus %02x-%02x] "
> +                      "isn't reserved\n", segment, start, end);
> +       } else if (insert_resource(&iomem_resource, &cfg->res)) {
> +               rc = -EBUSY;
> +               printk(KERN_WARNING PREFIX
> +                      "failed to insert resource for domain "
> +                      "%04x [bus %02x-%02x]\n", segment, start, end);
> +       } else if (pci_mmcfg_arch_map(cfg)) {
> +               rc = -EBUSY;
> +               printk(KERN_WARNING PREFIX
> +                      "failed to map resource for domain "
> +                      "%04x [bus %02x-%02x]\n", segment, start, end);
> +       } else {
> +               list_add_sorted(cfg);
> +               cfg = NULL;
> +               rc = 0;
> +       }
> +
> +       if (cfg) {
> +               if (cfg->res.parent)
> +                       release_resource(&cfg->res);
> +               kfree(cfg);
> +       }
> +
> +out:
> +       mutex_unlock(&pci_mmcfg_lock);
> +
> +       return rc;
> +}
> +
> +/* Delete MMCFG information at runtime */
> +int __devinit pci_mmconfig_delete(int segment, int start, int end, u64 addr)
> +{
> +       struct pci_mmcfg_region *cfg;
> +
> +       mutex_lock(&pci_mmcfg_lock);
> +       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> +               if (cfg->segment == segment && cfg->start_bus == start &&
> +                   cfg->end_bus == end && cfg->address == addr) {
> +                       list_del_rcu(&cfg->list);
> +                       synchronize_rcu();
> +                       pci_mmcfg_arch_unmap(cfg);
> +                       if (cfg->res.parent)
> +                               release_resource(&cfg->res);
> +                       mutex_unlock(&pci_mmcfg_lock);
> +                       kfree(cfg);
> +                       return 0;
> +               }
> +       }
> +       mutex_unlock(&pci_mmcfg_lock);
> +
> +       return -ENOENT;
> +}

new added _insert/_remove could be in another patch.

> diff --git a/arch/x86/pci/mmconfig_32.c b/arch/x86/pci/mmconfig_32.c
> index 5372e86..c2756de 100644
> --- a/arch/x86/pci/mmconfig_32.c
> +++ b/arch/x86/pci/mmconfig_32.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/pci.h>
>  #include <linux/init.h>
> +#include <linux/rcupdate.h>
>  #include <asm/e820.h>
>  #include <asm/pci_x86.h>
>  #include <acpi/acpi.h>
> @@ -60,9 +61,12 @@ err:         *value = -1;
>                return -EINVAL;
>        }
>
> +       rcu_read_lock();
>        base = get_base_addr(seg, bus, devfn);
> -       if (!base)
> +       if (!base) {
> +               rcu_read_unlock();
>                goto err;
> +       }
>
>        raw_spin_lock_irqsave(&pci_config_lock, flags);
>
> @@ -80,6 +84,7 @@ err:          *value = -1;
>                break;
>        }
>        raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> +       rcu_read_unlock();
>
>        return 0;
>  }
> @@ -93,9 +98,12 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>        if ((bus > 255) || (devfn > 255) || (reg > 4095))
>                return -EINVAL;
>
> +       rcu_read_lock();
>        base = get_base_addr(seg, bus, devfn);
> -       if (!base)
> +       if (!base) {
> +               rcu_read_unlock();
>                return -EINVAL;
> +       }
>
>        raw_spin_lock_irqsave(&pci_config_lock, flags);
>
> @@ -113,6 +121,7 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>                break;
>        }
>        raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> +       rcu_read_unlock();
>
>        return 0;
>  }
> @@ -132,3 +141,18 @@ int __init pci_mmcfg_arch_init(void)
>  void __init pci_mmcfg_arch_free(void)
>  {
>  }
> +
> +int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
> +{
> +       return 0;
> +}
> +
> +void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
> +{
> +       unsigned long flags;
> +
> +       /* Invalidate the cached mmcfg map entry. */
> +       raw_spin_lock_irqsave(&pci_config_lock, flags);
> +       mmcfg_last_accessed_device = 0;
> +       raw_spin_unlock_irqrestore(&pci_config_lock, flags);
> +}
> diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
> index 915a493..6822dd6 100644
> --- a/arch/x86/pci/mmconfig_64.c
> +++ b/arch/x86/pci/mmconfig_64.c
> @@ -9,6 +9,7 @@
>  #include <linux/init.h>
>  #include <linux/acpi.h>
>  #include <linux/bitmap.h>
> +#include <linux/rcupdate.h>
>  #include <asm/e820.h>
>  #include <asm/pci_x86.h>
>
> @@ -34,9 +35,12 @@ err:         *value = -1;
>                return -EINVAL;
>        }
>
> +       rcu_read_lock();
>        addr = pci_dev_base(seg, bus, devfn);
> -       if (!addr)
> +       if (!addr) {
> +               rcu_read_unlock();
>                goto err;
> +       }
>
>        switch (len) {
>        case 1:
> @@ -49,6 +53,7 @@ err:          *value = -1;
>                *value = mmio_config_readl(addr + reg);
>                break;
>        }
> +       rcu_read_unlock();
>
>        return 0;
>  }
> @@ -62,9 +67,12 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>        if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
>                return -EINVAL;
>
> +       rcu_read_lock();
>        addr = pci_dev_base(seg, bus, devfn);
> -       if (!addr)
> +       if (!addr) {
> +               rcu_read_unlock();
>                return -EINVAL;
> +       }
>
>        switch (len) {
>        case 1:
> @@ -77,6 +85,7 @@ static int pci_mmcfg_write(unsigned int seg, unsigned int bus,
>                mmio_config_writel(addr + reg, value);
>                break;
>        }
> +       rcu_read_unlock();
>
>        return 0;
>  }
> @@ -86,7 +95,7 @@ static const struct pci_raw_ops pci_mmcfg = {
>        .write =        pci_mmcfg_write,
>  };
>
> -static void __iomem * __init mcfg_ioremap(struct pci_mmcfg_region *cfg)
> +static void __iomem * __devinit mcfg_ioremap(struct pci_mmcfg_region *cfg)
>  {
>        void __iomem *addr;
>        u64 start, size;
> @@ -129,3 +138,23 @@ void __init pci_mmcfg_arch_free(void)
>                }
>        }
>  }
> +
> +int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
> +{
> +       cfg->virt = mcfg_ioremap(cfg);
> +       if (!cfg->virt) {
> +               printk(KERN_ERR PREFIX "can't map MMCONFIG at %pR\n",
> +                      &cfg->res);
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
> +void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
> +{
> +       if (cfg && cfg->virt) {
> +               iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
> +               cfg->virt = NULL;
> +       }
> +}

another one.

> --
> 1.7.5.4
>

[PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

This patch enhances pci_root driver to update MMCFG information when
hot-plugging PCI root bridges on x86 platforms.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/pci_root.c  |   20 ++++++++++++++++
 include/acpi/acnames.h   |    1 +
 include/linux/pci-acpi.h |    3 ++
 4 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index da0149d..9184970 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
 	return bus;
 }
 
+int arch_acpi_pci_root_add(struct acpi_pci_root *root)
+{
+	int result = 0;
+	acpi_status status;
+	unsigned long long base_addr;
+	struct pci_mmcfg_region *cfg;
+
+	/*
+	 * Try to insert MMCFG information for host bridges with _CBA method
+	 */
+	status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
+				       NULL, &base_addr);
+	if (ACPI_SUCCESS(status)) {
+		result = pci_mmconfig_insert(root->segment,
+					     root->secondary.start,
+					     root->secondary.end,
+					     base_addr);
+		/*
+		 * MMCFG information for hot-pluggable host bridges may have
+		 * already been added by __pci_mmcfg_init();
+		 */
+		if (result == -EEXIST)
+			result = 0;
+	} else if (status == AE_NOT_FOUND) {
+		/*
+		 * Check whether MMCFG information has been added for
+		 * host bridges without _CBA method.
+		 */
+		rcu_read_lock();
+		cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
+		if (!cfg || cfg->end_bus < root->secondary.end)
+			result = -ENODEV;
+		rcu_read_unlock();
+	} else
+		result = -ENODEV;
+
+	return result;
+}
+
+int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
+{
+	acpi_status status;
+	unsigned long long base_addr;
+
+	/* Remove MMCFG information for host bridges with _CBA method */
+	status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
+				       NULL, &base_addr);
+	if (ACPI_SUCCESS(status))
+		return pci_mmconfig_delete(root->segment,
+					   root->secondary.start,
+					   root->secondary.end,
+					   base_addr);
+	else if (status != AE_NOT_FOUND)
+		return -ENODEV;
+
+	return 0;
+}
+
 int __init pci_acpi_init(void)
 {
 	struct pci_dev *dev = NULL;
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 4a7d575..a62bfa8 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -448,6 +448,16 @@ out:
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
+{
+	return 0;
+}
+
+int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
+{
+	return 0;
+}
+
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	unsigned long long segment, bus;
@@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
 	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
 	device->driver_data = root;
 
+	if (arch_acpi_pci_root_add(root)) {
+		printk(KERN_ERR PREFIX
+			"can't add MMCFG information for Bus %04x:%02x\n",
+			root->segment, (unsigned int)root->secondary.start);
+		result = -ENODEV;
+		goto out_free;
+	}
+
 	/*
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
@@ -629,6 +647,7 @@ out_del_root:
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
+	arch_acpi_pci_root_remove(root);
 out_free:
 	kfree(root);
 	return result;
@@ -679,6 +698,7 @@ out:
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
+	arch_acpi_pci_root_remove(root);
 	kfree(root);
 
 	return 0;
diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
index 38f5088..99bda75 100644
--- a/include/acpi/acnames.h
+++ b/include/acpi/acnames.h
@@ -62,6 +62,7 @@
 #define METHOD_NAME__AEI        "_AEI"
 #define METHOD_NAME__PRW        "_PRW"
 #define METHOD_NAME__SRS        "_SRS"
+#define METHOD_NAME__CBA	"_CBA"
 
 /* Method names - these methods must appear at the namespace root */
 
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index ac93634..816b971 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
 
 void acpi_pci_root_rescan(void);
 
+extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
+extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
+
 #else
 
 static inline void acpi_pci_root_rescan(void) { }
-- 
1.7.5.4

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Yinghai Lu]

On Sun, Apr 8, 2012 at 10:12 AM, Jiang Liu <liuj97@gmail.com> wrote:
> This patch enhances pci_root driver to update MMCFG information when
> hot-plugging PCI root bridges on x86 platforms.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/pci_root.c  |   20 ++++++++++++++++
>  include/acpi/acnames.h   |    1 +
>  include/linux/pci-acpi.h |    3 ++
>  4 files changed, 82 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index da0149d..9184970 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
>        return bus;
>  }
>
> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
> +{
> +       int result = 0;
> +       acpi_status status;
> +       unsigned long long base_addr;
> +       struct pci_mmcfg_region *cfg;
> +
> +       /*
> +        * Try to insert MMCFG information for host bridges with _CBA method
> +        */
> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
> +                                      NULL, &base_addr);
> +       if (ACPI_SUCCESS(status)) {
> +               result = pci_mmconfig_insert(root->segment,
> +                                            root->secondary.start,
> +                                            root->secondary.end,
> +                                            base_addr);
> +               /*
> +                * MMCFG information for hot-pluggable host bridges may have
> +                * already been added by __pci_mmcfg_init();
> +                */
> +               if (result == -EEXIST)
> +                       result = 0;
> +       } else if (status == AE_NOT_FOUND) {
> +               /*
> +                * Check whether MMCFG information has been added for
> +                * host bridges without _CBA method.
> +                */
> +               rcu_read_lock();
> +               cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +               if (!cfg || cfg->end_bus < root->secondary.end)
> +                       result = -ENODEV;
> +               rcu_read_unlock();
> +       } else
> +               result = -ENODEV;
> +
> +       return result;
> +}
> +
> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
> +{
> +       acpi_status status;
> +       unsigned long long base_addr;
> +
> +       /* Remove MMCFG information for host bridges with _CBA method */
> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
> +                                      NULL, &base_addr);

add one flag in acpi_pci_root about we added mmconf for it before?
like mmconf_added?

> +       if (ACPI_SUCCESS(status))
> +               return pci_mmconfig_delete(root->segment,
> +                                          root->secondary.start,
> +                                          root->secondary.end,
> +                                          base_addr);
> +       else if (status != AE_NOT_FOUND)
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
>  int __init pci_acpi_init(void)
>  {
>        struct pci_dev *dev = NULL;
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 4a7d575..a62bfa8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -448,6 +448,16 @@ out:
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>
> +int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
> +{
> +       return 0;
> +}
> +
> +int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
> +{
> +       return 0;
> +}
> +
>  static int __devinit acpi_pci_root_add(struct acpi_device *device)
>  {
>        unsigned long long segment, bus;
> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>        strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>        device->driver_data = root;
>
> +       if (arch_acpi_pci_root_add(root)) {
> +               printk(KERN_ERR PREFIX
> +                       "can't add MMCFG information for Bus %04x:%02x\n",
> +                       root->segment, (unsigned int)root->secondary.start);
> +               result = -ENODEV;
> +               goto out_free;
> +       }
> +
>        /*
>         * All supported architectures that use ACPI have support for
>         * PCI domains, so we indicate this in _OSC support capabilities.
> @@ -629,6 +647,7 @@ out_del_root:
>        list_del_rcu(&root->node);
>        mutex_unlock(&acpi_pci_root_lock);
>        synchronize_rcu();
> +       arch_acpi_pci_root_remove(root);
>  out_free:
>        kfree(root);
>        return result;
> @@ -679,6 +698,7 @@ out:
>        list_del_rcu(&root->node);
>        mutex_unlock(&acpi_pci_root_lock);
>        synchronize_rcu();
> +       arch_acpi_pci_root_remove(root);
>        kfree(root);
>
>        return 0;
> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
> index 38f5088..99bda75 100644
> --- a/include/acpi/acnames.h
> +++ b/include/acpi/acnames.h
> @@ -62,6 +62,7 @@
>  #define METHOD_NAME__AEI        "_AEI"
>  #define METHOD_NAME__PRW        "_PRW"
>  #define METHOD_NAME__SRS        "_SRS"
> +#define METHOD_NAME__CBA       "_CBA"
>
>  /* Method names - these methods must appear at the namespace root */
>
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index ac93634..816b971 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>
>  void acpi_pci_root_rescan(void);
>
> +extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
> +extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
> +

don't need extern here.

>  #else
>
>  static inline void acpi_pci_root_rescan(void) { }
> --
> 1.7.5.4
>

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

Hi Yinghai,
	Thanks for your comments and will send out another version
according to your suggestions.
On 2012-4-9 3:19, Yinghai Lu wrote:
> On Sun, Apr 8, 2012 at 10:12 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> This patch enhances pci_root driver to update MMCFG information when
>> hot-plugging PCI root bridges on x86 platforms.
>>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>>   arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/acpi/pci_root.c  |   20 ++++++++++++++++
>>   include/acpi/acnames.h   |    1 +
>>   include/linux/pci-acpi.h |    3 ++
>>   4 files changed, 82 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index da0149d..9184970 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
>>         return bus;
>>   }
>>
>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       int result = 0;
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +       struct pci_mmcfg_region *cfg;
>> +
>> +       /*
>> +        * Try to insert MMCFG information for host bridges with _CBA method
>> +        */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL,&base_addr);
>> +       if (ACPI_SUCCESS(status)) {
>> +               result = pci_mmconfig_insert(root->segment,
>> +                                            root->secondary.start,
>> +                                            root->secondary.end,
>> +                                            base_addr);
>> +               /*
>> +                * MMCFG information for hot-pluggable host bridges may have
>> +                * already been added by __pci_mmcfg_init();
>> +                */
>> +               if (result == -EEXIST)
>> +                       result = 0;
>> +       } else if (status == AE_NOT_FOUND) {
>> +               /*
>> +                * Check whether MMCFG information has been added for
>> +                * host bridges without _CBA method.
>> +                */
>> +               rcu_read_lock();
>> +               cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +               if (!cfg || cfg->end_bus<  root->secondary.end)
>> +                       result = -ENODEV;
>> +               rcu_read_unlock();
>> +       } else
>> +               result = -ENODEV;
>> +
>> +       return result;
>> +}
>> +
>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +
>> +       /* Remove MMCFG information for host bridges with _CBA method */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL,&base_addr);
>
> add one flag in acpi_pci_root about we added mmconf for it before?
> like mmconf_added?
>
>> +       if (ACPI_SUCCESS(status))
>> +               return pci_mmconfig_delete(root->segment,
>> +                                          root->secondary.start,
>> +                                          root->secondary.end,
>> +                                          base_addr);
>> +       else if (status != AE_NOT_FOUND)
>> +               return -ENODEV;
>> +
>> +       return 0;
>> +}
>> +
>>   int __init pci_acpi_init(void)
>>   {
>>         struct pci_dev *dev = NULL;
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 4a7d575..a62bfa8 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -448,6 +448,16 @@ out:
>>   }
>>   EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>
>> +int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>> +int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>>   static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>   {
>>         unsigned long long segment, bus;
>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>         strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>         device->driver_data = root;
>>
>> +       if (arch_acpi_pci_root_add(root)) {
>> +               printk(KERN_ERR PREFIX
>> +                       "can't add MMCFG information for Bus %04x:%02x\n",
>> +                       root->segment, (unsigned int)root->secondary.start);
>> +               result = -ENODEV;
>> +               goto out_free;
>> +       }
>> +
>>         /*
>>          * All supported architectures that use ACPI have support for
>>          * PCI domains, so we indicate this in _OSC support capabilities.
>> @@ -629,6 +647,7 @@ out_del_root:
>>         list_del_rcu(&root->node);
>>         mutex_unlock(&acpi_pci_root_lock);
>>         synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>   out_free:
>>         kfree(root);
>>         return result;
>> @@ -679,6 +698,7 @@ out:
>>         list_del_rcu(&root->node);
>>         mutex_unlock(&acpi_pci_root_lock);
>>         synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>         kfree(root);
>>
>>         return 0;
>> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
>> index 38f5088..99bda75 100644
>> --- a/include/acpi/acnames.h
>> +++ b/include/acpi/acnames.h
>> @@ -62,6 +62,7 @@
>>   #define METHOD_NAME__AEI        "_AEI"
>>   #define METHOD_NAME__PRW        "_PRW"
>>   #define METHOD_NAME__SRS        "_SRS"
>> +#define METHOD_NAME__CBA       "_CBA"
>>
>>   /* Method names - these methods must appear at the namespace root */
>>
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index ac93634..816b971 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>>
>>   void acpi_pci_root_rescan(void);
>>
>> +extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
>> +extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
>> +
>
> don't need extern here.
>
>>   #else
>>
>>   static inline void acpi_pci_root_rescan(void) { }
>> --
>> 1.7.5.4
>>
>
> .
>

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

Hi Yinghai,
	
>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +
>> +       /* Remove MMCFG information for host bridges with _CBA method */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL, &base_addr);
> 
> add one flag in acpi_pci_root about we added mmconf for it before?
> like mmconf_added?
> 

I feel adding a flag here doesn't help here. For any hot-pluggable PCI host bridge
with _CBA method available, arch_acpi_pci_root_remove() should delete the MMCFG 
information, no matter it's added by __pci_mmcfg_init() or arch_acpi_pci_root_add(). 
So check existence of _CBA seems simpler here.
Thanks!

On 04/09/2012 03:19 AM, Yinghai Lu wrote:
> On Sun, Apr 8, 2012 at 10:12 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> This patch enhances pci_root driver to update MMCFG information when
>> hot-plugging PCI root bridges on x86 platforms.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/pci_root.c  |   20 ++++++++++++++++
>>  include/acpi/acnames.h   |    1 +
>>  include/linux/pci-acpi.h |    3 ++
>>  4 files changed, 82 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index da0149d..9184970 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
>>        return bus;
>>  }
>>
>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       int result = 0;
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +       struct pci_mmcfg_region *cfg;
>> +
>> +       /*
>> +        * Try to insert MMCFG information for host bridges with _CBA method
>> +        */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL, &base_addr);
>> +       if (ACPI_SUCCESS(status)) {
>> +               result = pci_mmconfig_insert(root->segment,
>> +                                            root->secondary.start,
>> +                                            root->secondary.end,
>> +                                            base_addr);
>> +               /*
>> +                * MMCFG information for hot-pluggable host bridges may have
>> +                * already been added by __pci_mmcfg_init();
>> +                */
>> +               if (result == -EEXIST)
>> +                       result = 0;
>> +       } else if (status == AE_NOT_FOUND) {
>> +               /*
>> +                * Check whether MMCFG information has been added for
>> +                * host bridges without _CBA method.
>> +                */
>> +               rcu_read_lock();
>> +               cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +               if (!cfg || cfg->end_bus < root->secondary.end)
>> +                       result = -ENODEV;
>> +               rcu_read_unlock();
>> +       } else
>> +               result = -ENODEV;
>> +
>> +       return result;
>> +}
>> +
>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +
>> +       /* Remove MMCFG information for host bridges with _CBA method */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL, &base_addr);
> 
> add one flag in acpi_pci_root about we added mmconf for it before?
> like mmconf_added?
> 
>> +       if (ACPI_SUCCESS(status))
>> +               return pci_mmconfig_delete(root->segment,
>> +                                          root->secondary.start,
>> +                                          root->secondary.end,
>> +                                          base_addr);
>> +       else if (status != AE_NOT_FOUND)
>> +               return -ENODEV;
>> +
>> +       return 0;
>> +}
>> +
>>  int __init pci_acpi_init(void)
>>  {
>>        struct pci_dev *dev = NULL;
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 4a7d575..a62bfa8 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -448,6 +448,16 @@ out:
>>  }
>>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>
>> +int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>> +int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>>  static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>  {
>>        unsigned long long segment, bus;
>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>        strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>        device->driver_data = root;
>>
>> +       if (arch_acpi_pci_root_add(root)) {
>> +               printk(KERN_ERR PREFIX
>> +                       "can't add MMCFG information for Bus %04x:%02x\n",
>> +                       root->segment, (unsigned int)root->secondary.start);
>> +               result = -ENODEV;
>> +               goto out_free;
>> +       }
>> +
>>        /*
>>         * All supported architectures that use ACPI have support for
>>         * PCI domains, so we indicate this in _OSC support capabilities.
>> @@ -629,6 +647,7 @@ out_del_root:
>>        list_del_rcu(&root->node);
>>        mutex_unlock(&acpi_pci_root_lock);
>>        synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>  out_free:
>>        kfree(root);
>>        return result;
>> @@ -679,6 +698,7 @@ out:
>>        list_del_rcu(&root->node);
>>        mutex_unlock(&acpi_pci_root_lock);
>>        synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>        kfree(root);
>>
>>        return 0;
>> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
>> index 38f5088..99bda75 100644
>> --- a/include/acpi/acnames.h
>> +++ b/include/acpi/acnames.h
>> @@ -62,6 +62,7 @@
>>  #define METHOD_NAME__AEI        "_AEI"
>>  #define METHOD_NAME__PRW        "_PRW"
>>  #define METHOD_NAME__SRS        "_SRS"
>> +#define METHOD_NAME__CBA       "_CBA"
>>
>>  /* Method names - these methods must appear at the namespace root */
>>
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index ac93634..816b971 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>>
>>  void acpi_pci_root_rescan(void);
>>
>> +extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
>> +extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
>> +
> 
> don't need extern here.
> 
>>  #else
>>
>>  static inline void acpi_pci_root_rescan(void) { }
>> --
>> 1.7.5.4
>>

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Yinghai Lu]

On Mon, Apr 9, 2012 at 7:37 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Hi Yinghai,
>
>>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>>> +{
>>> +       acpi_status status;
>>> +       unsigned long long base_addr;
>>> +
>>> +       /* Remove MMCFG information for host bridges with _CBA method */
>>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>>> +                                      NULL, &base_addr);
>>
>> add one flag in acpi_pci_root about we added mmconf for it before?
>> like mmconf_added?
>>
>
> I feel adding a flag here doesn't help here. For any hot-pluggable PCI host bridge
> with _CBA method available, arch_acpi_pci_root_remove() should delete the MMCFG
> information, no matter it's added by __pci_mmcfg_init() or arch_acpi_pci_root_add().
> So check existence of _CBA seems simpler here.

_remove should not care about _CBA, it should only care about if it
gets added on _add path.

Also like Kenji said, We should let it go through even no MMCONF is
added at that time.
like somehow _CBA does not exis for some root bus or evaluating failing etc.

Yinghai

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Mon, Apr 9, 2012 at 7:37 AM, Jiang Liu <liuj97@gmail.com> wrote:
> Hi Yinghai,
>
>>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>>> +{
>>> +       acpi_status status;
>>> +       unsigned long long base_addr;
>>> +
>>> +       /* Remove MMCFG information for host bridges with _CBA method */
>>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>>> +                                      NULL, &base_addr);
>>
>> add one flag in acpi_pci_root about we added mmconf for it before?
>> like mmconf_added?
>>
>
> I feel adding a flag here doesn't help here. For any hot-pluggable PCI host bridge
> with _CBA method available, arch_acpi_pci_root_remove() should delete the MMCFG
> information, no matter it's added by __pci_mmcfg_init() or arch_acpi_pci_root_add().
> So check existence of _CBA seems simpler here.

please check updated patch that will use mmconf_added.
it will make sure only remove mmconf with _CBA.
other than that if BIOS have duplicated one in MCFG table, and
_CBA, will bail out early.

Line added will be 68 instead of 90.

also some changes could be merged to patch 1-5.

Thanks

Yinghai

[-- Attachment #2: jiang_mmconf_6_v3.patch --]
[-- Type: application/octet-stream, Size: 9448 bytes --]

From: Jiang Liu <liuj97@gmail.com>
To: Taku Izumi <izumi.taku@jp.fujitsu.com>,
	Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>,
	Yinghai Lu <yinghai@kernel.org>
Cc: Jiang Liu <jiang.liu@huawei.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Len Brown <lenb@kernel.org>,
	Jiang Liu <liuj97@gmail.com>,
	Keping Chen <chenkeping@huawei.com>,
	x86@vger.kernel.org,
	linux-pci@vger.kernel.org,
	linux-acpi@vger.kernel.org
Subject: [PATCH V2 6/6] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges
Date: Tue, 10 Apr 2012 00:22:48 +0800
Message-Id: <1333988568-11282-7-git-send-email-jiang.liu@huawei.com>

This patch enhances pci_root driver to update MMCFG information when
hot-plugging PCI root bridges on x86 platforms.

-v3: add mmconf_added to simply free path, also make pci_mmconfig_insert()
     to process extra exist case --- By Yinghai

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 arch/x86/include/asm/pci_x86.h |    4 +--
 arch/x86/pci/acpi.c            |   47 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/pci/mmconfig-shared.c |   17 ++++++--------
 arch/x86/pci/mmconfig_32.c     |    2 -
 arch/x86/pci/mmconfig_64.c     |    2 -
 drivers/acpi/pci_root.c        |   17 ++++++++++++++
 include/acpi/acnames.h         |    1 
 include/acpi/acpi_bus.h        |    1 
 include/linux/pci-acpi.h       |    3 ++
 9 files changed, 81 insertions(+), 13 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/acpi.c
+++ linux-2.6/arch/x86/pci/acpi.c
@@ -488,6 +488,53 @@ struct pci_bus * __devinit pci_acpi_scan
 	return bus;
 }
 
+int __devinit arch_acpi_pci_root_add(struct acpi_pci_root *root)
+{
+	int result = 0;
+	unsigned long long base_addr = 0;
+
+	/* MMCONFIG disabled */
+	if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
+		return 0;
+
+	/*
+	 * Try to insert MMCFG information for host bridges with _CBA method
+	 */
+	acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
+				       NULL, &base_addr);
+
+	result = pci_mmconfig_insert(root->segment,
+				     root->secondary.start,
+				     root->secondary.end,
+				     base_addr);
+	if (!result)
+		root->mmconf_added = true;
+
+	if (result == -EEXIST)
+		result = 0;
+
+	if (result)
+		printk(KERN_ERR
+			"can't add MMCFG information for Bus %04x:%02x\n",
+			root->segment, (unsigned int)root->secondary.start);
+
+	/* still could use conf1 with it */
+	if (!root->segment)
+		result = 0;
+
+	return result;
+}
+
+int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
+{
+	if (!root->mmconf_added)
+		return 0;
+
+	return pci_mmconfig_delete(root->segment,
+				   root->secondary.start,
+				   root->secondary.end);
+}
+
 int __init pci_acpi_init(void)
 {
 	struct pci_dev *dev = NULL;
Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -448,6 +448,16 @@ out:
 }
 EXPORT_SYMBOL(acpi_pci_osc_control_set);
 
+int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
+{
+	return 0;
+}
+
+int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
+{
+	return 0;
+}
+
 static int __devinit acpi_pci_root_add(struct acpi_device *device)
 {
 	unsigned long long segment, bus;
@@ -504,6 +514,11 @@ static int __devinit acpi_pci_root_add(s
 	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
 	device->driver_data = root;
 
+	if (arch_acpi_pci_root_add(root)) {
+		result = -ENODEV;
+		goto out_free;
+	}
+
 	/*
 	 * All supported architectures that use ACPI have support for
 	 * PCI domains, so we indicate this in _OSC support capabilities.
@@ -629,6 +644,7 @@ out_del_root:
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
+	arch_acpi_pci_root_remove(root);
 out_free:
 	kfree(root);
 	return result;
@@ -686,6 +702,7 @@ out:
 	list_del_rcu(&root->node);
 	mutex_unlock(&acpi_pci_root_lock);
 	synchronize_rcu();
+	arch_acpi_pci_root_remove(root);
 	kfree(root);
 
 	return 0;
Index: linux-2.6/include/acpi/acnames.h
===================================================================
--- linux-2.6.orig/include/acpi/acnames.h
+++ linux-2.6/include/acpi/acnames.h
@@ -62,6 +62,7 @@
 #define METHOD_NAME__AEI        "_AEI"
 #define METHOD_NAME__PRW        "_PRW"
 #define METHOD_NAME__SRS        "_SRS"
+#define METHOD_NAME__CBA	"_CBA"
 
 /* Method names - these methods must appear at the namespace root */
 
Index: linux-2.6/include/linux/pci-acpi.h
===================================================================
--- linux-2.6.orig/include/linux/pci-acpi.h
+++ linux-2.6/include/linux/pci-acpi.h
@@ -18,6 +18,9 @@ extern acpi_status pci_acpi_add_pm_notif
 					     struct pci_dev *pci_dev);
 extern acpi_status pci_acpi_remove_pm_notifier(struct acpi_device *dev);
 
+int arch_acpi_pci_root_add(struct acpi_pci_root *root);
+int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
+
 static inline acpi_handle acpi_find_root_bridge_handle(struct pci_dev *pdev)
 {
 	struct pci_bus *pbus = pdev->bus;
Index: linux-2.6/arch/x86/include/asm/pci_x86.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/pci_x86.h
+++ linux-2.6/arch/x86/include/asm/pci_x86.h
@@ -136,9 +136,9 @@ struct pci_mmcfg_region {
 extern int __init pci_mmcfg_arch_init(void);
 extern void __init pci_mmcfg_arch_free(void);
 extern int __devinit pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg);
-extern void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
+extern void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg);
 extern int __devinit pci_mmconfig_insert(int seg, int start, int end, u64 addr);
-extern int __devinit pci_mmconfig_delete(int seg, int start, int end, u64 addr);
+extern int pci_mmconfig_delete(int seg, int start, int end);
 extern struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus);
 
 extern struct list_head pci_mmcfg_list;
Index: linux-2.6/arch/x86/pci/mmconfig-shared.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig-shared.c
+++ linux-2.6/arch/x86/pci/mmconfig-shared.c
@@ -684,21 +684,19 @@ static DEFINE_MUTEX(pci_mmcfg_lock);
 /* Add MMCFG information for hot-added host bridges at runtime */
 int __devinit pci_mmconfig_insert(int segment, int start, int end, u64 addr)
 {
-	int rc;
+	int rc = -EINVAL;
 	struct pci_mmcfg_region *cfg = NULL;
 
-	if (addr == 0 || segment < 0 || segment > USHRT_MAX ||
+	if (segment < 0 || segment > USHRT_MAX ||
 	    start < 0 || start > 255 || end < start || end > 255)
-		return -EINVAL;
+		return rc;
 
 	mutex_lock(&pci_mmcfg_lock);
 	cfg = pci_mmconfig_lookup(segment, start);
 	if (cfg) {
-		if (cfg->start_bus == start && cfg->end_bus == end &&
-		    cfg->address == addr) {
+		if (cfg->start_bus <= start && cfg->end_bus >= end) {
 			rc = -EEXIST;
 		} else {
-			rc = -EINVAL;
 			printk(KERN_WARNING PREFIX
 			       "MMCONFIG for domain %04x [bus %02x-%02x] "
 			       "conflicts with domain %04x [bus %02x-%02x]\n",
@@ -707,12 +705,13 @@ int __devinit pci_mmconfig_insert(int se
 		}
 		goto out;
 	}
+	if (!addr)
+		goto out;
 
 	cfg = pci_mmconfig_alloc(segment, start, end, addr);
 	if (cfg == NULL) {
 		rc = -ENOMEM;
 	} else if (!pci_mmcfg_check_reserved(cfg, 0)) {
-		rc = -EINVAL;
 		printk(KERN_WARNING PREFIX
 		       "MMCONFIG for domain %04x [bus %02x-%02x] "
 		       "isn't reserved\n", segment, start, end);
@@ -745,14 +744,14 @@ out:
 }
 
 /* Delete MMCFG information at runtime */
-int __devinit pci_mmconfig_delete(int segment, int start, int end, u64 addr)
+int pci_mmconfig_delete(int segment, int start, int end)
 {
 	struct pci_mmcfg_region *cfg;
 
 	mutex_lock(&pci_mmcfg_lock);
 	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
 		if (cfg->segment == segment && cfg->start_bus == start &&
-		    cfg->end_bus == end && cfg->address == addr) {
+		    cfg->end_bus == end) {
 			list_del_rcu(&cfg->list);
 			synchronize_rcu();
 			pci_mmcfg_arch_unmap(cfg);
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -371,6 +371,7 @@ struct acpi_pci_root {
 	u32 osc_support_set;	/* _OSC state of support bits */
 	u32 osc_control_set;	/* _OSC state of control bits */
 	bool hot_added;
+	bool mmconf_added;
 };
 
 /* helper */
Index: linux-2.6/arch/x86/pci/mmconfig_64.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig_64.c
+++ linux-2.6/arch/x86/pci/mmconfig_64.c
@@ -151,7 +151,7 @@ int __devinit pci_mmcfg_arch_map(struct
 	return 0;
 }
 
-void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
 {
 	if (cfg && cfg->virt) {
 		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
Index: linux-2.6/arch/x86/pci/mmconfig_32.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/mmconfig_32.c
+++ linux-2.6/arch/x86/pci/mmconfig_32.c
@@ -147,7 +147,7 @@ int __devinit pci_mmcfg_arch_map(struct
 	return 0;
 }
 
-void __devinit pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
 {
 	unsigned long flags;
 

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Kenji Kaneshige]

Your patch looks good to me.

I have some comments.

(2012/04/09 2:12), Jiang Liu wrote:
> This patch enhances pci_root driver to update MMCFG information when
> hot-plugging PCI root bridges on x86 platforms.
>

Do you have the patch that can be applied to Bjorn's pci tree?

<snip.>

> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
> +{
> +	int result = 0;
> +	acpi_status status;
> +	unsigned long long base_addr;
> +	struct pci_mmcfg_region *cfg;
> +
> +	/*
> +	 * Try to insert MMCFG information for host bridges with _CBA method
> +	 */
> +	status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
> +				       NULL,&base_addr);
> +	if (ACPI_SUCCESS(status)) {
> +		result = pci_mmconfig_insert(root->segment,
> +					     root->secondary.start,
> +					     root->secondary.end,
> +					     base_addr);
> +		/*
> +		 * MMCFG information for hot-pluggable host bridges may have
> +		 * already been added by __pci_mmcfg_init();
> +		 */
> +		if (result == -EEXIST)
> +			result = 0;

Just for confirmation.
>From my interpretation of PCI firmware spec, MCFG doesn't have any entry
for hot-pluggable hostbridge. So I assume this is for the machine that
is not compliant to the spec. Is my understanding same as yours?

<snip.>

>   static int __devinit acpi_pci_root_add(struct acpi_device *device)
>   {
>   	unsigned long long segment, bus;
> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>   	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>   	device->driver_data = root;
> 
> +	if (arch_acpi_pci_root_add(root)) {
> +		printk(KERN_ERR PREFIX
> +			"can't add MMCFG information for Bus %04x:%02x\n",
> +			root->segment, (unsigned int)root->secondary.start);
> +		result = -ENODEV;
> +		goto out_free;
> +	}

Desn't this break the system that doesn't support MMCONFIG?

In my understanding, arch_acpi_pci_root_add() returns -ENODEV if
mmconfig information is found neither in MCFG table nor _CBA. And
pci root bridge initialization seems to fail arch_acpi_pci_root_add()
returns non-zero value.

Regards,
Kenji Kaneshige

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

Hi Kenji,
	Thanks for your careful review and comments.

On 04/09/2012 07:43 PM, Kenji Kaneshige wrote:
> Your patch looks good to me.
> 
> I have some comments.
> 
> (2012/04/09 2:12), Jiang Liu wrote:
>> This patch enhances pci_root driver to update MMCFG information when
>> hot-plugging PCI root bridges on x86 platforms.
>>
> 
> Do you have the patch that can be applied to Bjorn's pci tree?
> 
> <snip.>
Will try to generate a version against Bjorn's version. Could you please tell
me the exact git link for that? I haven't pull from Bjorn's tree yet.

> 
>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +	int result = 0;
>> +	acpi_status status;
>> +	unsigned long long base_addr;
>> +	struct pci_mmcfg_region *cfg;
>> +
>> +	/*
>> +	 * Try to insert MMCFG information for host bridges with _CBA method
>> +	 */
>> +	status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +				       NULL,&base_addr);
>> +	if (ACPI_SUCCESS(status)) {
>> +		result = pci_mmconfig_insert(root->segment,
>> +					     root->secondary.start,
>> +					     root->secondary.end,
>> +					     base_addr);
>> +		/*
>> +		 * MMCFG information for hot-pluggable host bridges may have
>> +		 * already been added by __pci_mmcfg_init();
>> +		 */
>> +		if (result == -EEXIST)
>> +			result = 0;
> 
> Just for confirmation.
> From my interpretation of PCI firmware spec, MCFG doesn't have any entry
> for hot-pluggable hostbridge. So I assume this is for the machine that
> is not compliant to the spec. Is my understanding same as yours?
> 
> <snip.>
You are right, it's defined to that way in PCI FW Spec 3.1.
Here I have some concerns about the PCI buses to host all Ubox components
on Intel NHM/WSM/SNB/IVB processors. BIOS people are prone to declare
MMCFG information for those host bridges by MCFG table instead of _CBA method,
though those host bridge will disappear after hot-removing a physical processor.

> 
>>   static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>   {
>>   	unsigned long long segment, bus;
>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>   	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>   	device->driver_data = root;
>>
>> +	if (arch_acpi_pci_root_add(root)) {
>> +		printk(KERN_ERR PREFIX
>> +			"can't add MMCFG information for Bus %04x:%02x\n",
>> +			root->segment, (unsigned int)root->secondary.start);
>> +		result = -ENODEV;
>> +		goto out_free;
>> +	}
> 
> Desn't this break the system that doesn't support MMCONFIG?
> 
> In my understanding, arch_acpi_pci_root_add() returns -ENODEV if
> mmconfig information is found neither in MCFG table nor _CBA. And
> pci root bridge initialization seems to fail arch_acpi_pci_root_add()
> returns non-zero value.
Good catch, will add following code into arch_acpi_pci_root_add() and 
arch_acpi_pci_root_remove() to solve this issue.
---
        /* MMCONFIG disabled */
        if ((pci_probe & PCI_PROBE_MMCONF) == 0)
                return 0;
---


> 
> Regards,
> Kenji Kaneshige

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Kenji Kaneshige]

(2012/04/10 1:02), Jiang Liu wrote:
> Hi Kenji,
> 	Thanks for your careful review and comments.
>
> On 04/09/2012 07:43 PM, Kenji Kaneshige wrote:
>> Your patch looks good to me.
>>
>> I have some comments.
>>
>> (2012/04/09 2:12), Jiang Liu wrote:
>>> This patch enhances pci_root driver to update MMCFG information when
>>> hot-plugging PCI root bridges on x86 platforms.
>>>
>>
>> Do you have the patch that can be applied to Bjorn's pci tree?
>>
>> <snip.>
> Will try to generate a version against Bjorn's version. Could you please tell
> me the exact git link for that? I haven't pull from Bjorn's tree yet.

As you may know, it was announced _today_ (sorry:).

http://www.spinics.net/lists/linux-pci/msg14626.html

>
>>
>>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>>> +{
>>> +	int result = 0;
>>> +	acpi_status status;
>>> +	unsigned long long base_addr;
>>> +	struct pci_mmcfg_region *cfg;
>>> +
>>> +	/*
>>> +	 * Try to insert MMCFG information for host bridges with _CBA method
>>> +	 */
>>> +	status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>>> +				       NULL,&base_addr);
>>> +	if (ACPI_SUCCESS(status)) {
>>> +		result = pci_mmconfig_insert(root->segment,
>>> +					     root->secondary.start,
>>> +					     root->secondary.end,
>>> +					     base_addr);
>>> +		/*
>>> +		 * MMCFG information for hot-pluggable host bridges may have
>>> +		 * already been added by __pci_mmcfg_init();
>>> +		 */
>>> +		if (result == -EEXIST)
>>> +			result = 0;
>>
>> Just for confirmation.
>>  From my interpretation of PCI firmware spec, MCFG doesn't have any entry
>> for hot-pluggable hostbridge. So I assume this is for the machine that
>> is not compliant to the spec. Is my understanding same as yours?
>>
>> <snip.>
> You are right, it's defined to that way in PCI FW Spec 3.1.
> Here I have some concerns about the PCI buses to host all Ubox components
> on Intel NHM/WSM/SNB/IVB processors. BIOS people are prone to declare
> MMCFG information for those host bridges by MCFG table instead of _CBA method,
> though those host bridge will disappear after hot-removing a physical processor.

Ok, thank you for clarification.

>
>>
>>>    static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>>    {
>>>    	unsigned long long segment, bus;
>>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>>    	strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>>    	device->driver_data = root;
>>>
>>> +	if (arch_acpi_pci_root_add(root)) {
>>> +		printk(KERN_ERR PREFIX
>>> +			"can't add MMCFG information for Bus %04x:%02x\n",
>>> +			root->segment, (unsigned int)root->secondary.start);

Additional comment.

This printk message looks strange because arch_acpi_pci_root_add()
is not a mmconfig specific function. So I think this message
should be moved to arch specific code (arch_acpi_pci_root_add()).

>>> +		result = -ENODEV;
>>> +		goto out_free;
>>> +	}
>>
>> Desn't this break the system that doesn't support MMCONFIG?
>>
>> In my understanding, arch_acpi_pci_root_add() returns -ENODEV if
>> mmconfig information is found neither in MCFG table nor _CBA. And
>> pci root bridge initialization seems to fail arch_acpi_pci_root_add()
>> returns non-zero value.
> Good catch, will add following code into arch_acpi_pci_root_add() and
> arch_acpi_pci_root_remove() to solve this issue.
> ---
>          /* MMCONFIG disabled */
>          if ((pci_probe&  PCI_PROBE_MMCONF) == 0)
>                  return 0;
> ---

My understanding is that PCI_PROBE_MMCONF is set even if the system
doesn't have MCFG table. So I don't think this solves the issue. I
guess this is what Yinghai pointed out on your V2 patch [6/6].

Additionally, I think there is a remaining issue even if we change
this check like below.

	if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
         	return 0;

I think this check has an assumption that system has at least one
MCFG table entry and it has been initialized before
acpi_pci_root_add() is called. I think this doesn't work on the
system that doesn't have MCFG and all the pci root bridge have
_CBA (that is, all host bridges are hot-pluggable and BIOS is
implemented in the way PCI FW spec defines). As a result, MMCONFIG
would never be enabled on such systems. Could you double check this?

Regards,
Kenji Kaneshige

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Yinghai Lu]

On Tue, Apr 10, 2012 at 3:32 AM, Kenji Kaneshige
<kaneshige.kenji@jp.fujitsu.com> wrote:
> Additionally, I think there is a remaining issue even if we change
> this check like below.
>
>
>        if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
>                return 0;
>
> I think this check has an assumption that system has at least one
> MCFG table entry and it has been initialized before
> acpi_pci_root_add() is called. I think this doesn't work on the
> system that doesn't have MCFG and all the pci root bridge have
> _CBA (that is, all host bridges are hot-pluggable and BIOS is
> implemented in the way PCI FW spec defines). As a result, MMCONFIG
> would never be enabled on such systems. Could you double check this?

You are right.

We can just remove that checking.

But wonder if current x86_64 system support that.
All peer root buses can be physically removed ?

Thanks

Yinghai

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

On 04/10/2012 11:47 PM, Yinghai Lu wrote:
> On Tue, Apr 10, 2012 at 3:32 AM, Kenji Kaneshige
> <kaneshige.kenji@jp.fujitsu.com> wrote:
>> Additionally, I think there is a remaining issue even if we change
>> this check like below.
>>
>>
>>        if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
>>                return 0;
>>
>> I think this check has an assumption that system has at least one
>> MCFG table entry and it has been initialized before
>> acpi_pci_root_add() is called. I think this doesn't work on the
>> system that doesn't have MCFG and all the pci root bridge have
>> _CBA (that is, all host bridges are hot-pluggable and BIOS is
>> implemented in the way PCI FW spec defines). As a result, MMCONFIG
>> would never be enabled on such systems. Could you double check this?
> 
> You are right.
> 
> We can just remove that checking.
> 
> But wonder if current x86_64 system support that.
> All peer root buses can be physically removed ?
It's theoretically possible that BIOS doesn't provide MMCFG or _CBA
for the legacy (non-removable bus 0) host bridge. 

> 
> Thanks
> 
> Yinghai

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

On 04/10/2012 06:32 PM, Kenji Kaneshige wrote:
> (2012/04/10 1:02), Jiang Liu wrote:
>> Hi Kenji,
>>     Thanks for your careful review and comments.
>>
>> On 04/09/2012 07:43 PM, Kenji Kaneshige wrote:
>>> Your patch looks good to me.
>>>
>>> I have some comments.
>>>
>>> (2012/04/09 2:12), Jiang Liu wrote:
>>>> This patch enhances pci_root driver to update MMCFG information when
>>>> hot-plugging PCI root bridges on x86 platforms.
>>>>
>>>
>>> Do you have the patch that can be applied to Bjorn's pci tree?
>>>
>>> <snip.>
>> Will try to generate a version against Bjorn's version. Could you please tell
>> me the exact git link for that? I haven't pull from Bjorn's tree yet.
> 
> As you may know, it was announced _today_ (sorry:).
> 
> http://www.spinics.net/lists/linux-pci/msg14626.html
> 
>>
>>>
>>>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>>>> +{
>>>> +    int result = 0;
>>>> +    acpi_status status;
>>>> +    unsigned long long base_addr;
>>>> +    struct pci_mmcfg_region *cfg;
>>>> +
>>>> +    /*
>>>> +     * Try to insert MMCFG information for host bridges with _CBA method
>>>> +     */
>>>> +    status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>>>> +                       NULL,&base_addr);
>>>> +    if (ACPI_SUCCESS(status)) {
>>>> +        result = pci_mmconfig_insert(root->segment,
>>>> +                         root->secondary.start,
>>>> +                         root->secondary.end,
>>>> +                         base_addr);
>>>> +        /*
>>>> +         * MMCFG information for hot-pluggable host bridges may have
>>>> +         * already been added by __pci_mmcfg_init();
>>>> +         */
>>>> +        if (result == -EEXIST)
>>>> +            result = 0;
>>>
>>> Just for confirmation.
>>>  From my interpretation of PCI firmware spec, MCFG doesn't have any entry
>>> for hot-pluggable hostbridge. So I assume this is for the machine that
>>> is not compliant to the spec. Is my understanding same as yours?
>>>
>>> <snip.>
>> You are right, it's defined to that way in PCI FW Spec 3.1.
>> Here I have some concerns about the PCI buses to host all Ubox components
>> on Intel NHM/WSM/SNB/IVB processors. BIOS people are prone to declare
>> MMCFG information for those host bridges by MCFG table instead of _CBA method,
>> though those host bridge will disappear after hot-removing a physical processor.
> 
> Ok, thank you for clarification.
> 
>>
>>>
>>>>    static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>>>    {
>>>>        unsigned long long segment, bus;
>>>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>>>        strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>>>        device->driver_data = root;
>>>>
>>>> +    if (arch_acpi_pci_root_add(root)) {
>>>> +        printk(KERN_ERR PREFIX
>>>> +            "can't add MMCFG information for Bus %04x:%02x\n",
>>>> +            root->segment, (unsigned int)root->secondary.start);
> 
> Additional comment.
> 
> This printk message looks strange because arch_acpi_pci_root_add()
> is not a mmconfig specific function. So I think this message
> should be moved to arch specific code (arch_acpi_pci_root_add()).
> 
>>>> +        result = -ENODEV;
>>>> +        goto out_free;
>>>> +    }
>>>
>>> Desn't this break the system that doesn't support MMCONFIG?
>>>
>>> In my understanding, arch_acpi_pci_root_add() returns -ENODEV if
>>> mmconfig information is found neither in MCFG table nor _CBA. And
>>> pci root bridge initialization seems to fail arch_acpi_pci_root_add()
>>> returns non-zero value.
>> Good catch, will add following code into arch_acpi_pci_root_add() and
>> arch_acpi_pci_root_remove() to solve this issue.
>> ---
>>          /* MMCONFIG disabled */
>>          if ((pci_probe&  PCI_PROBE_MMCONF) == 0)
>>                  return 0;
>> ---
> 
> My understanding is that PCI_PROBE_MMCONF is set even if the system
> doesn't have MCFG table. So I don't think this solves the issue. I
> guess this is what Yinghai pointed out on your V2 patch [6/6].
> 
> Additionally, I think there is a remaining issue even if we change
> this check like below.
> 
>     if (!!(pci_probe & PCI_PROBE_MASK & ~PCI_PROBE_MMCONF))
>             return 0;
> 
> I think this check has an assumption that system has at least one
> MCFG table entry and it has been initialized before
> acpi_pci_root_add() is called. I think this doesn't work on the
> system that doesn't have MCFG and all the pci root bridge have
> _CBA (that is, all host bridges are hot-pluggable and BIOS is
> implemented in the way PCI FW spec defines). As a result, MMCONFIG
> would never be enabled on such systems. Could you double check this?
Good points. We underestimated the complexity here.
I'm working on a new patch based on Yinghai's v3 patchset and will send
it out soon.
Thanks!

> 
> Regards,
> Kenji Kaneshige

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Bjorn Helgaas]

On Sun, Apr 8, 2012 at 11:12 AM, Jiang Liu <liuj97@gmail.com> wrote:
> This patch enhances pci_root driver to update MMCFG information when
> hot-plugging PCI root bridges on x86 platforms.
>
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/pci_root.c  |   20 ++++++++++++++++
>  include/acpi/acnames.h   |    1 +
>  include/linux/pci-acpi.h |    3 ++
>  4 files changed, 82 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> index da0149d..9184970 100644
> --- a/arch/x86/pci/acpi.c
> +++ b/arch/x86/pci/acpi.c
> @@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
>        return bus;
>  }
>
> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
> +{
> +       int result = 0;
> +       acpi_status status;
> +       unsigned long long base_addr;
> +       struct pci_mmcfg_region *cfg;
> +
> +       /*
> +        * Try to insert MMCFG information for host bridges with _CBA method
> +        */
> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
> +                                      NULL, &base_addr);

I would prefer to have _CBA evaluated in drivers/acpi/pci_root.c, not
in the arch code.  It's true that _CBA is probably only used by x86
today, but the spec says nothing about it being arch-dependent, and I
suspect it may be used on arm soon.

> +       if (ACPI_SUCCESS(status)) {
> +               result = pci_mmconfig_insert(root->segment,
> +                                            root->secondary.start,
> +                                            root->secondary.end,
> +                                            base_addr);
> +               /*
> +                * MMCFG information for hot-pluggable host bridges may have
> +                * already been added by __pci_mmcfg_init();
> +                */
> +               if (result == -EEXIST)
> +                       result = 0;
> +       } else if (status == AE_NOT_FOUND) {
> +               /*
> +                * Check whether MMCFG information has been added for
> +                * host bridges without _CBA method.
> +                */
> +               rcu_read_lock();
> +               cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
> +               if (!cfg || cfg->end_bus < root->secondary.end)
> +                       result = -ENODEV;
> +               rcu_read_unlock();
> +       } else
> +               result = -ENODEV;
> +
> +       return result;
> +}
> +
> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
> +{
> +       acpi_status status;
> +       unsigned long long base_addr;
> +
> +       /* Remove MMCFG information for host bridges with _CBA method */
> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
> +                                      NULL, &base_addr);

I think the arch-specific "map MMCONFIG space at this addr" should
return a pointer or something that we can save and use to unmap it on
remove.  That way we don't have to evaluate _CBA again.

> +       if (ACPI_SUCCESS(status))
> +               return pci_mmconfig_delete(root->segment,
> +                                          root->secondary.start,
> +                                          root->secondary.end,
> +                                          base_addr);
> +       else if (status != AE_NOT_FOUND)
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
>  int __init pci_acpi_init(void)
>  {
>        struct pci_dev *dev = NULL;
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 4a7d575..a62bfa8 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -448,6 +448,16 @@ out:
>  }
>  EXPORT_SYMBOL(acpi_pci_osc_control_set);
>
> +int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
> +{
> +       return 0;
> +}
> +
> +int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
> +{
> +       return 0;
> +}
> +
>  static int __devinit acpi_pci_root_add(struct acpi_device *device)
>  {
>        unsigned long long segment, bus;
> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>        strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>        device->driver_data = root;
>
> +       if (arch_acpi_pci_root_add(root)) {
> +               printk(KERN_ERR PREFIX
> +                       "can't add MMCFG information for Bus %04x:%02x\n",
> +                       root->segment, (unsigned int)root->secondary.start);
> +               result = -ENODEV;
> +               goto out_free;
> +       }
> +
>        /*
>         * All supported architectures that use ACPI have support for
>         * PCI domains, so we indicate this in _OSC support capabilities.
> @@ -629,6 +647,7 @@ out_del_root:
>        list_del_rcu(&root->node);
>        mutex_unlock(&acpi_pci_root_lock);
>        synchronize_rcu();
> +       arch_acpi_pci_root_remove(root);
>  out_free:
>        kfree(root);
>        return result;
> @@ -679,6 +698,7 @@ out:
>        list_del_rcu(&root->node);
>        mutex_unlock(&acpi_pci_root_lock);
>        synchronize_rcu();
> +       arch_acpi_pci_root_remove(root);
>        kfree(root);
>
>        return 0;
> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
> index 38f5088..99bda75 100644
> --- a/include/acpi/acnames.h
> +++ b/include/acpi/acnames.h
> @@ -62,6 +62,7 @@
>  #define METHOD_NAME__AEI        "_AEI"
>  #define METHOD_NAME__PRW        "_PRW"
>  #define METHOD_NAME__SRS        "_SRS"
> +#define METHOD_NAME__CBA       "_CBA"
>
>  /* Method names - these methods must appear at the namespace root */
>
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index ac93634..816b971 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>
>  void acpi_pci_root_rescan(void);
>
> +extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
> +extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
> +
>  #else
>
>  static inline void acpi_pci_root_rescan(void) { }

I don't know where to put this in the conversation, but I think we
should separate the question of whether we do blind probing from the
question of how we set up MMCONFIG space from MCFG.

We currently parse the entire MCFG in some sort of initcall, but I
think we *could* change the x86 blind probing routines to attempt to
set up MMCONFIG space for the buses they find, just like
acpi_pci_root_add() does it for the ACPI host bridges.

That way we can continue blind probing, but make MMCONFIG init more sensible.

Bjorn

Re: [PATCH 2/2] PCI, ACPI, x86: update MMCFG information when hot-plugging PCI host bridges

[Jiang Liu]

On 2012-5-3 7:55, Bjorn Helgaas wrote:
> On Sun, Apr 8, 2012 at 11:12 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> This patch enhances pci_root driver to update MMCFG information when
>> hot-plugging PCI root bridges on x86 platforms.
>>
>> Signed-off-by: Jiang Liu<jiang.liu@huawei.com>
>> ---
>>   arch/x86/pci/acpi.c      |   58 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/acpi/pci_root.c  |   20 ++++++++++++++++
>>   include/acpi/acnames.h   |    1 +
>>   include/linux/pci-acpi.h |    3 ++
>>   4 files changed, 82 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index da0149d..9184970 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -488,6 +488,64 @@ struct pci_bus * __devinit pci_acpi_scan_root(struct acpi_pci_root *root)
>>         return bus;
>>   }
>>
>> +int arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       int result = 0;
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +       struct pci_mmcfg_region *cfg;
>> +
>> +       /*
>> +        * Try to insert MMCFG information for host bridges with _CBA method
>> +        */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL,&base_addr);
>
> I would prefer to have _CBA evaluated in drivers/acpi/pci_root.c, not
> in the arch code.  It's true that _CBA is probably only used by x86
> today, but the spec says nothing about it being arch-dependent, and I
> suspect it may be used on arm soon.
Good point. I just noticed that IA64 doesn't need MMCFG, but haven't
realized there's still another candidate.

>
>> +       if (ACPI_SUCCESS(status)) {
>> +               result = pci_mmconfig_insert(root->segment,
>> +                                            root->secondary.start,
>> +                                            root->secondary.end,
>> +                                            base_addr);
>> +               /*
>> +                * MMCFG information for hot-pluggable host bridges may have
>> +                * already been added by __pci_mmcfg_init();
>> +                */
>> +               if (result == -EEXIST)
>> +                       result = 0;
>> +       } else if (status == AE_NOT_FOUND) {
>> +               /*
>> +                * Check whether MMCFG information has been added for
>> +                * host bridges without _CBA method.
>> +                */
>> +               rcu_read_lock();
>> +               cfg = pci_mmconfig_lookup(root->segment, root->secondary.start);
>> +               if (!cfg || cfg->end_bus<  root->secondary.end)
>> +                       result = -ENODEV;
>> +               rcu_read_unlock();
>> +       } else
>> +               result = -ENODEV;
>> +
>> +       return result;
>> +}
>> +
>> +int arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       acpi_status status;
>> +       unsigned long long base_addr;
>> +
>> +       /* Remove MMCFG information for host bridges with _CBA method */
>> +       status = acpi_evaluate_integer(root->device->handle, METHOD_NAME__CBA,
>> +                                      NULL,&base_addr);
>
> I think the arch-specific "map MMCONFIG space at this addr" should
> return a pointer or something that we can save and use to unmap it on
> remove.  That way we don't have to evaluate _CBA again.
OK, will change to follow that way.

>
>> +       if (ACPI_SUCCESS(status))
>> +               return pci_mmconfig_delete(root->segment,
>> +                                          root->secondary.start,
>> +                                          root->secondary.end,
>> +                                          base_addr);
>> +       else if (status != AE_NOT_FOUND)
>> +               return -ENODEV;
>> +
>> +       return 0;
>> +}
>> +
>>   int __init pci_acpi_init(void)
>>   {
>>         struct pci_dev *dev = NULL;
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index 4a7d575..a62bfa8 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -448,6 +448,16 @@ out:
>>   }
>>   EXPORT_SYMBOL(acpi_pci_osc_control_set);
>>
>> +int __weak arch_acpi_pci_root_add(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>> +int __weak arch_acpi_pci_root_remove(struct acpi_pci_root *root)
>> +{
>> +       return 0;
>> +}
>> +
>>   static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>   {
>>         unsigned long long segment, bus;
>> @@ -504,6 +514,14 @@ static int __devinit acpi_pci_root_add(struct acpi_device *device)
>>         strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
>>         device->driver_data = root;
>>
>> +       if (arch_acpi_pci_root_add(root)) {
>> +               printk(KERN_ERR PREFIX
>> +                       "can't add MMCFG information for Bus %04x:%02x\n",
>> +                       root->segment, (unsigned int)root->secondary.start);
>> +               result = -ENODEV;
>> +               goto out_free;
>> +       }
>> +
>>         /*
>>          * All supported architectures that use ACPI have support for
>>          * PCI domains, so we indicate this in _OSC support capabilities.
>> @@ -629,6 +647,7 @@ out_del_root:
>>         list_del_rcu(&root->node);
>>         mutex_unlock(&acpi_pci_root_lock);
>>         synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>   out_free:
>>         kfree(root);
>>         return result;
>> @@ -679,6 +698,7 @@ out:
>>         list_del_rcu(&root->node);
>>         mutex_unlock(&acpi_pci_root_lock);
>>         synchronize_rcu();
>> +       arch_acpi_pci_root_remove(root);
>>         kfree(root);
>>
>>         return 0;
>> diff --git a/include/acpi/acnames.h b/include/acpi/acnames.h
>> index 38f5088..99bda75 100644
>> --- a/include/acpi/acnames.h
>> +++ b/include/acpi/acnames.h
>> @@ -62,6 +62,7 @@
>>   #define METHOD_NAME__AEI        "_AEI"
>>   #define METHOD_NAME__PRW        "_PRW"
>>   #define METHOD_NAME__SRS        "_SRS"
>> +#define METHOD_NAME__CBA       "_CBA"
>>
>>   /* Method names - these methods must appear at the namespace root */
>>
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index ac93634..816b971 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -38,6 +38,9 @@ static inline acpi_handle acpi_pci_get_bridge_handle(struct pci_bus *pbus)
>>
>>   void acpi_pci_root_rescan(void);
>>
>> +extern int arch_acpi_pci_root_add(struct acpi_pci_root *root);
>> +extern int arch_acpi_pci_root_remove(struct acpi_pci_root *root);
>> +
>>   #else
>>
>>   static inline void acpi_pci_root_rescan(void) { }
>
> I don't know where to put this in the conversation, but I think we
> should separate the question of whether we do blind probing from the
> question of how we set up MMCONFIG space from MCFG.
>
> We currently parse the entire MCFG in some sort of initcall, but I
> think we *could* change the x86 blind probing routines to attempt to
> set up MMCONFIG space for the buses they find, just like
> acpi_pci_root_add() does it for the ACPI host bridges.
>
> That way we can continue blind probing, but make MMCONFIG init more sensible.
Good suggestion, will do more investigation about that.

>
> Bjorn
>
> .
>