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

[Jiang Liu <liuj97@gmail.com>]

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