Re: [Qemu-devel] [PATCH v5 for-2.3 26/28] acpi: restrict the aml emission to PXB host bridges

[Marcel Apfelbaum <marcel@redhat.com>]

On 03/10/2015 05:41 PM, Michael S. Tsirkin wrote:
> On Tue, Mar 10, 2015 at 05:32:12PM +0200, Marcel Apfelbaum wrote:
>> Initial implementation assumed that the aml used for
>> any extra root buses would be generic, however this
>> is not always true. Restrict aml emission only to i440fx and
>> PXB because is the only supported combination for now.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> I was wondering about this too.
> Please split this up and squash into appropriate patches.
> I know it's more work but it's worth it.
Sure,

Thanks,
Marcel
>
>
>> ---
>>   hw/i386/acpi-build.c                | 56 ++++++++++++++++++++-----------------
>>   hw/pci-bridge/pci_expander_bridge.c |  1 -
>>   include/hw/pci/pci_host.h           |  2 ++
>>   3 files changed, 32 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index ff18a07..2bc8a80 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -887,7 +887,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>>       /* Reserve space for header */
>>       acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>>
>> -    {
>> +    if (find_i440fx()) {
>>           PciInfoList *info_list, *info;
>>           Error *err = NULL;
>>
>> @@ -901,37 +901,41 @@ build_ssdt(GArray *table_data, GArray *linker,
>>               PciInfo *bus_info = info->value;
>>               PCIHostState *host;
>>
>> -            if (bus_info->bus == 0) {
>> -                continue;
>> -            }
>> +            HOST_BRIDGE_FOREACH(host) {
>> +                int numa_node;
>>
>> -            if (bus_info->bus < root_bus_limit) {
>> -                root_bus_limit = bus_info->bus - 1;
>> -            }
>> +                if (!(pci_bus_num(host->bus) == bus_info->bus)) {
>> +                    continue;
>> +                }
>>
>> -            scope = aml_scope("\\_SB");
>> -            dev = aml_device("PC%.02X", (uint8_t)bus_info->bus);
>> -            aml_append(dev, aml_name_decl("_UID",
>> -                aml_string("PC%.02X", (uint8_t)bus_info->bus)));
>> -            aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03")));
>> -            aml_append(dev,
>> -                aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus)));
>> +                if (!object_dynamic_cast(OBJECT(host), TYPE_PXB_HOST)) {
>> +                    break;
>> +                }
>
> Or you can check the device/vendor id if that's easier.
>
>>
>> -            HOST_BRIDGE_FOREACH(host) {
>> -                if (pci_bus_num(host->bus) == bus_info->bus) {
>> -                    int numa_node = pci_bus_numa_node(host->bus);
>> -                    if (numa_node != NUMA_NODE_UNASSIGNED) {
>> -                        aml_append(dev,
>> +                if (bus_info->bus < root_bus_limit) {
>> +                    root_bus_limit = bus_info->bus - 1;
>> +                }
>> +
>> +                scope = aml_scope("\\_SB");
>> +                dev = aml_device("PC%.02X", (uint8_t)bus_info->bus);
>> +                aml_append(dev, aml_name_decl("_UID",
>> +                            aml_string("PC%.02X", (uint8_t)bus_info->bus)));
>> +                aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03")));
>> +                aml_append(dev,
>> +                        aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus)));
>> +
>> +                numa_node = pci_bus_numa_node(host->bus);
>> +                if (numa_node != NUMA_NODE_UNASSIGNED) {
>> +                    aml_append(dev,
>>                               aml_name_decl("_PXM", aml_int(numa_node)));
>> -                    }
>>                   }
>> -            }
>>
>> -            aml_append(dev, build_prt());
>> -            crs = build_crs(pci, bus_info, &io_ranges, &mem_ranges);
>> -            aml_append(dev, aml_name_decl("_CRS", crs));
>> -            aml_append(scope, dev);
>> -            aml_append(ssdt, scope);
>> +                aml_append(dev, build_prt());
>> +                crs = build_crs(pci, bus_info, &io_ranges, &mem_ranges);
>> +                aml_append(dev, aml_name_decl("_CRS", crs));
>> +                aml_append(scope, dev);
>> +                aml_append(ssdt, scope);
>> +            }
>>           }
>>
>>           qapi_free_PciInfoList(info_list);
>> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
>> index 9329aab..8e6740e 100644
>> --- a/hw/pci-bridge/pci_expander_bridge.c
>> +++ b/hw/pci-bridge/pci_expander_bridge.c
>> @@ -41,7 +41,6 @@ typedef struct PXBDev {
>>       uint16_t numa_node;
>>   } PXBDev;
>>
>> -#define TYPE_PXB_HOST "pxb-host"
>>
>>   static int pxb_bus_num(PCIBus *bus)
>>   {
>> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
>> index ba5272f..9a35389 100644
>> --- a/include/hw/pci/pci_host.h
>> +++ b/include/hw/pci/pci_host.h
>> @@ -30,6 +30,8 @@
>>
>>   #include "hw/sysbus.h"
>>
>> +#define TYPE_PXB_HOST "pxb-host"
>> +
>
> That's a wrong place for it I think.
>
>>   /**
>>    * Marker interface for classes whose instances can
>>    * be main host bridges. It is intended to be used
>> --
>> 2.1.0