Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:08PM +0000, Igor Mammedov wrote:
> it replaces PCI tree structure in SSDT with a set of scopes
> describing each PCI bus as a separate scope with a child devices.
> It makes code easier to follow and a little bit smaller.
> 
> In addition it makes simplier to convert current template
> patching approach to completely dynamically generated SSDT
> without dependency on IASL, which would allow to drop
> template binary blobs from source tree.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
>  hw/i386/acpi-build.c | 362 
> +++++++++++++++++++++++----------------------------
>  1 file changed, 165 insertions(+), 197 deletions(-)

I like it that your patch makes code smaller and simpler,
but I think we do need to generate hierarchical AML.

I think this can still be done with some modifications to
the logic you have here.

Basically current code does all work in build_pci_bus_end.
It follows that you can do the same by simply walking
the list in reverse order.





> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 97ff245..7606a73 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -641,196 +641,186 @@ static void acpi_set_pci_info(void)
>      }
>  }
>  
> -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> -                                     AcpiBuildPciBusHotplugState *parent,
> -                                     bool pcihp_bridge_en)
> +static char *pci_bus_get_scope_name(PCIBus *bus)
>  {
> -    state->parent = parent;
> -    state->device_table = build_alloc_array();
> -    state->notify_table = build_alloc_array();
> -    state->pcihp_bridge_en = pcihp_bridge_en;
> -}
> +    char *name = NULL;
> +    char *last = name;
>  
> -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> -{
> -    build_free_array(state->device_table);
> -    build_free_array(state->notify_table);
> -}
> +    while (bus->parent_dev) {
> +        last = name;
> +        name = g_strdup_printf("%s.S%.02X_",
> +                               name ? name : "", bus->parent_dev->devfn);
> +        g_free(last);
>  
> -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> -{
> -    AcpiBuildPciBusHotplugState *parent = parent_state;
> -    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> +        bus = bus->parent_dev->bus;
> +    }
>  
> -    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> +    last = name;
> +    name = g_strdup_printf("PCI0%s", name ? name : "");
> +    g_free(last);
>  
> -    return child;
> +    return name;
>  }
>  
> -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> +static GQueue *get_pci_bus_list(PCIBus *bus, bool pcihp_bridge_en)
>  {
> -    AcpiBuildPciBusHotplugState *child = bus_state;
> -    AcpiBuildPciBusHotplugState *parent = child->parent;
> -    GArray *bus_table = build_alloc_array();
> -    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> -    uint8_t op;
> -    int i;
> -    QObject *bsel;
> -    GArray *method;
> -    bool bus_hotplug_support = false;
> -
> -    /*
> -     * Skip bridge subtree creation if bridge hotplug is disabled
> -     * to make acpi tables compatible with legacy machine types.
> -     */
> -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> -        return;
> -    }
> -
> -    if (bus->parent_dev) {
> -        op = 0x82; /* DeviceOp */
> -        build_append_namestring(bus_table, "S%.02X",
> -                             bus->parent_dev->devfn);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "_SUN");
> -        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "_ADR");
> -        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 
> 16) |
> -                           PCI_FUNC(bus->parent_dev->devfn), 4);
> -    } else {
> -        op = 0x10; /* ScopeOp */;
> -        build_append_namestring(bus_table, "PCI0");
> -    }
> -
> -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, 
> NULL);
> -    if (bsel) {
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "BSEL");
> -        build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> -        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> -    } else {
> -        /* No bsel - no slots are hot-pluggable */
> -        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> +    GQueue *bus_list = g_queue_new();
> +    GQueue *tree_walk = g_queue_new();
> +
> +    /* build BUS list */
> +    g_queue_push_tail(tree_walk, bus);
> +    while (!g_queue_is_empty(tree_walk)) {
> +        PCIBus *sec;
> +        PCIBus *parent = g_queue_pop_tail(tree_walk);
> +
> +        g_queue_push_tail(bus_list, parent);
> +        if (!pcihp_bridge_en) {
> +            break;
> +        }
> +        QLIST_FOREACH(sec, &parent->child, sibling) {
> +            g_queue_push_tail(tree_walk, sec);
> +        }
>      }
> +    g_queue_free(tree_walk);
> +    return bus_list;
> +}
>  
> -    memset(slot_device_present, 0x00, sizeof slot_device_present);
> -    memset(slot_device_system, 0x00, sizeof slot_device_present);
> -    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> -    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> -
> -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> -        DeviceClass *dc;
> -        PCIDeviceClass *pc;
> -        PCIDevice *pdev = bus->devices[i];
> -        int slot = PCI_SLOT(i);
> -        bool bridge_in_acpi;
> -
> -        if (!pdev) {
> -            continue;
> +static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
> +                                         bool pcihp_bridge_en)
> +{
> +    GQueue *bus_list = get_pci_bus_list(bus, pcihp_bridge_en);
> +
> +    while (!g_queue_is_empty(bus_list)) {
> +        DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> +        GArray *bus_table = build_alloc_array();
> +        PCIBus *bus = g_queue_pop_head(bus_list);
> +        GArray *method;
> +        QObject *bsel;
> +        PCIBus *sec;
> +        int i;
> +
> +        char *scope_name = pci_bus_get_scope_name(bus);
> +        build_append_namestring(bus_table, "%s", scope_name);
> +        g_free(scope_name);
> +
> +        bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> +                                           NULL);
> +        if (bsel) {
> +            build_append_byte(bus_table, 0x08); /* NameOp */
> +            build_append_namestring(bus_table, "BSEL");
> +            build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> +            memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> +        } else {
> +            /* No bsel - no slots are hot-pluggable */
> +            memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
>          }
>  
> -        set_bit(slot, slot_device_present);
> -        pc = PCI_DEVICE_GET_CLASS(pdev);
> -        dc = DEVICE_GET_CLASS(pdev);
> +        memset(slot_device_present, 0x00, sizeof slot_device_present);
> +        memset(slot_device_system, 0x00, sizeof slot_device_present);
> +        memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> +        memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
>  
> -        /* When hotplug for bridges is enabled, bridges are
> -         * described in ACPI separately (see build_pci_bus_end).
> -         * In this case they aren't themselves hot-pluggable.
> -         */
> -        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> +        for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> +            DeviceClass *dc;
> +            PCIDeviceClass *pc;
> +            PCIDevice *pdev = bus->devices[i];
> +            int slot = PCI_SLOT(i);
>  
> -        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> -            set_bit(slot, slot_device_system);
> -        }
> +            if (!pdev) {
> +                continue;
> +            }
>  
> -        if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> -            set_bit(slot, slot_device_vga);
> +            set_bit(slot, slot_device_present);
> +            pc = PCI_DEVICE_GET_CLASS(pdev);
> +            dc = DEVICE_GET_CLASS(pdev);
>  
> -            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> -                set_bit(slot, slot_device_qxl);
> +            if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> +                set_bit(slot, slot_device_system);
>              }
> -        }
>  
> -        if (!dc->hotpluggable || pc->is_bridge) {
> -            clear_bit(slot, slot_hotplug_enable);
> -        }
> -    }
> +            if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> +                set_bit(slot, slot_device_vga);
>  
> -    /* Append Device object for each slot */
> -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> -        bool can_eject = test_bit(i, slot_hotplug_enable);
> -        bool present = test_bit(i, slot_device_present);
> -        bool vga = test_bit(i, slot_device_vga);
> -        bool qxl = test_bit(i, slot_device_qxl);
> -        bool system = test_bit(i, slot_device_system);
> -        if (can_eject) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIHP_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> -            patch_pcihp(i, pcihp);
> -            bus_hotplug_support = true;
> -        } else if (qxl) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIQXL_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> -            patch_pciqxl(i, pcihp);
> -        } else if (vga) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIVGA_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> -            patch_pcivga(i, pcihp);
> -        } else if (system) {
> -            /* Nothing to do: system devices are in DSDT or in SSDT above. */
> -        } else if (present) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCINOHP_SIZEOF);
> -            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> -            patch_pcinohp(i, pcihp);
> -        }
> -    }
> +                if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> +                    set_bit(slot, slot_device_qxl);
> +                }
> +            }
>  
> -    if (bsel) {
> -        method = build_alloc_method("DVNT", 2);
> +            if (!dc->hotpluggable || pc->is_bridge) {
> +                clear_bit(slot, slot_hotplug_enable);
> +            }
> +        }
>  
> +        /* Append Device object for each slot */
>          for (i = 0; i < PCI_SLOT_MAX; i++) {
> -            GArray *notify;
> -            uint8_t op;
> -
> -            if (!test_bit(i, slot_hotplug_enable)) {
> -                continue;
> +            bool can_eject = test_bit(i, slot_hotplug_enable);
> +            bool present = test_bit(i, slot_device_present);
> +            bool vga = test_bit(i, slot_device_vga);
> +            bool qxl = test_bit(i, slot_device_qxl);
> +            bool system = test_bit(i, slot_device_system);
> +            if (can_eject) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIHP_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> +                patch_pcihp(i, pcihp);
> +            } else if (qxl) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIQXL_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> +                patch_pciqxl(i, pcihp);
> +            } else if (vga) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIVGA_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> +                patch_pcivga(i, pcihp);
> +            } else if (system) {
> +                /* Nothing to do: system devices are in DSDT or in SSDT 
> above */
> +            } else if (present) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCINOHP_SIZEOF);
> +                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> +                patch_pcinohp(i, pcihp);
>              }
> +        }
>  
> -            notify = build_alloc_array();
> -            op = 0xA0; /* IfOp */
> -
> -            build_append_byte(notify, 0x7B); /* AndOp */
> -            build_append_byte(notify, 0x68); /* Arg0Op */
> -            build_append_int(notify, 0x1U << i);
> -            build_append_byte(notify, 0x00); /* NullName */
> -            build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> -            build_append_byte(notify, 0x69); /* Arg1Op */
> -
> -            /* Pack it up */
> -            build_package(notify, op);
> -
> -            build_append_array(method, notify);
> +        if (bsel) {
> +            method = build_alloc_method("DVNT", 2);
> +
> +            for (i = 0; i < PCI_SLOT_MAX; i++) {
> +                GArray *notify;
> +                uint8_t op;
> +
> +                if (!test_bit(i, slot_hotplug_enable)) {
> +                    continue;
> +                }
> +
> +                notify = build_alloc_array();
> +                op = 0xA0; /* IfOp */
> +
> +                build_append_byte(notify, 0x7B); /* AndOp */
> +                build_append_byte(notify, 0x68); /* Arg0Op */
> +                build_append_int(notify, 0x1U << i);
> +                build_append_byte(notify, 0x00); /* NullName */
> +                build_append_byte(notify, 0x86); /* NotifyOp */
> +                build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> +                build_append_byte(notify, 0x69); /* Arg1Op */
> +
> +                /* Pack it up */
> +                build_package(notify, op);
> +                build_append_array(method, notify);
> +                build_free_array(notify);
> +            }
>  
> -            build_free_array(notify);
> +            build_append_and_cleanup_method(bus_table, method);
>          }
>  
> -        build_append_and_cleanup_method(bus_table, method);
> -    }
> -
> -    /* Append PCNT method to notify about events on local and child buses.
> -     * Add unconditionally for root since DSDT expects it.
> -     */
> -    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) 
> {
> +        /* Append PCNT method to notify about events on local and child 
> buses.
> +         * Add unconditionally for root since DSDT expects it.
> +         */
>          method = build_alloc_method("PCNT", 0);
>  
>          /* If bus supports hotplug select it and notify about local events */
> @@ -847,36 +837,20 @@ static void build_pci_bus_end(PCIBus *bus, void 
> *bus_state)
>          }
>  
>          /* Notify about child bus events in any case */
> -        build_append_array(method, child->notify_table);
> -
> -        build_append_and_cleanup_method(bus_table, method);
> -
> -        /* Append description of child buses */
> -        build_append_array(bus_table, child->device_table);
> -
> -        /* Pack it up */
> -        if (bus->parent_dev) {
> -            build_extop_package(bus_table, op);
> -        } else {
> -            build_package(bus_table, op);
> +        if (pcihp_bridge_en) {
> +            QLIST_FOREACH(sec, &bus->child, sibling) {
> +                build_append_namestring(method, "^S%.02X.PCNT",
> +                                        sec->parent_dev->devfn);
> +            }
>          }
>  
> -        /* Append our bus description to parent table */
> -        build_append_array(parent->device_table, bus_table);
> +        build_append_and_cleanup_method(bus_table, method);
>  
> -        /* Also tell parent how to notify us, invoking PCNT method.
> -         * At the moment this is not needed for root as we have a single 
> root.
> -         */
> -        if (bus->parent_dev) {
> -            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> -                                 bus->parent_dev->devfn);
> -        }
> +        build_package(bus_table, 0x10); /* ScopeOp */
> +        build_append_array(parent_scope, bus_table);
> +        build_free_array(bus_table);
>      }
> -
> -    qobject_decref(bsel);
> -    build_free_array(bus_table);
> -    build_pci_bus_state_cleanup(child);
> -    g_free(child);
> +    g_queue_free(bus_list);
>  }
>  
>  static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> @@ -1008,7 +982,6 @@ build_ssdt(GArray *table_data, GArray *linker,
>          }
>  
>          {
> -            AcpiBuildPciBusHotplugState hotplug_state;
>              Object *pci_host;
>              PCIBus *bus = NULL;
>              bool ambiguous;
> @@ -1018,16 +991,11 @@ build_ssdt(GArray *table_data, GArray *linker,
>                  bus = PCI_HOST_BRIDGE(pci_host)->bus;
>              }
>  
> -            build_pci_bus_state_init(&hotplug_state, NULL, 
> pm->pcihp_bridge_en);
> -
>              if (bus) {
>                  /* Scan all PCI buses. Generate tables to support hotplug. */
> -                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> -                                             build_pci_bus_end, 
> &hotplug_state);
> +                build_append_pci_bus_devices(sb_scope, bus,
> +                                             pm->pcihp_bridge_en);
>              }
> -
> -            build_append_array(sb_scope, hotplug_state.device_table);
> -            build_pci_bus_state_cleanup(&hotplug_state);
>          }
>          build_package(sb_scope, op);
>          build_append_array(table_data, sb_scope);
> -- 
> 1.8.3.1