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

[Michael S. Tsirkin]

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 <address@hidden>

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.


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