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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

(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?

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:).

+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?

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)) {
+			"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?

Kenji Kaneshige
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at

[Home]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux