[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus t
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based |
|
Date: |
Tue, 9 Dec 2014 16:29:13 +0200 |
On Tue, Dec 09, 2014 at 03:01:00PM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 22:43:24 +0200
> "Michael S. Tsirkin" <address@hidden> 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 <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.
> 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)
No, tree depth squared since you generate this per bridge.
> 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.
pci supports up to 256 nested bridges.
So that's 256K which isn't the end of the world but
not negligeable for a bios.
> >
> > 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.
Well, the way I see it, the main simplification is
that you can pass data from child to parent explicitly.
Maybe I'm not explaining this properly, I'll try to
find the time to send a patch for comparison.
> 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.
I agree the approach of unrolling the tree, then walking the list has
advantages, but if it's forcing a specific AML structure then that
really means it's too limited.
> 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.
I don't necessarily see how these two ideas are related.
> >
> >
> >
> >
> >
> > > 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
> >
[Qemu-devel] [PATCH 7/9] acpi: replace opencoded notify codes with named values, Igor Mammedov, 2014/12/08
[Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper, Igor Mammedov, 2014/12/08
Re: [Qemu-devel] [PATCH 0/9] pc: acpi: various fixes and cleanups, Michael S. Tsirkin, 2014/12/08