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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 8 Dec 2014 22:43:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <imammedo@redhat.com>
> > ---
> >  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.
If only reason for hierarchical AML is table size then
I don't see how scopes could possibly double the table size,
Compared to tree version scope adds only:

 ~5 bytes for package + NamePath(~[0-2] + 4 * tree depth)

Which is almost nothing compared to slots description
per a bridge.
One needs to configure insanely deep bridge nesting
for scope cost to significantly affect table size, which
doesn't sound as practical thing to do anyway except of
testing limitation on how many nested bridges QEMU would
stomach.

> 
> 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.
I've thought about it already, but tree creation means
creating temporary contexts for each scope, copying them
to parent context, keeping track of contexts to build
correct notifiers. It's basically the same or even worse
as current code just other way around.

And all that makes code quite hard to follow and
maintain, that is the main reason to drop recursion in
favor of simple flat scope sets. And it makes easier to
switch from AML+template patching to ASL API later
and hide from user direct access to table array, forcing
him to work only with ASL constructs when building table.

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