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

[Jiang Liu <liuj97@gmail.com>]

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