Re: [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 19 Jan 2015 17:23:41 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Dec 19, 2014 at 11:47:04AM +0000, Igor Mammedov wrote:
> > it basicaly does the same as original approach,
> > * just without bus/notify tables tracking (less obscure)
> >   which is easier to follow.
> > * drops unnecessary loops and bitmaps,
> >   creating devices and notification method in the same loop.
> > * saves us ~100LOC
> > 
> > change in behavior:
> > * generate hotpluggable device entries for empty slots
> >   only if BUS itself is hotpluggable, otherwise do not
> >   create them.
> 
> Hmm so how do you create a machine where this applies?
> Can you clarify?
it only can happen if bridge device marked as non hotpluggable,
i.e. not happens in practice for now. Perhaps I should drop
this comment altogether.

> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Overall looks fine to me, I'm fine with appying this after rebase.
> Although I would prefer it if we didn't add so many scope
> operations: I think it's cleaner to add content to devices
> directly, but not a must.

scope here is used to avoid construction of pcinohp template manually
by hand. and reuse current pcihohp template. However it should be easy
to drop scopes in following conversion where devices are constructed
manually. I'll take care of it in that series during its rebasing.

> 
> > ---
> > v3:
> >   * use hotpluggable device object instead of not hotpluggable
> >     for non present devices, and add it only when bus itself is
> >     hotpluggable
> > ---
> >  hw/i386/acpi-build.c | 265 +++++++++++++++------------------------------------
> >  1 file changed, 79 insertions(+), 186 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 94202b5..a893f5e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -103,7 +103,6 @@ typedef struct AcpiPmInfo {
> >  typedef struct AcpiMiscInfo {
> >      bool has_hpet;
> >      bool has_tpm;
> > -    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> >      const unsigned char *dsdt_code;
> >      unsigned dsdt_size;
> >      uint16_t pvpanic_port;
> > @@ -647,69 +646,37 @@ static void acpi_set_pci_info(void)
> >      }
> >  }
> >  
> > -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> > -                                     AcpiBuildPciBusHotplugState *parent,
> > -                                     bool pcihp_bridge_en)
> > +static void build_append_pcihp_notify_entry(GArray *method, int slot)
> >  {
> > -    state->parent = parent;
> > -    state->device_table = build_alloc_array();
> > -    state->notify_table = build_alloc_array();
> > -    state->pcihp_bridge_en = pcihp_bridge_en;
> > -}
> > -
> > -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> > -{
> > -    build_free_array(state->device_table);
> > -    build_free_array(state->notify_table);
> > -}
> > +    GArray *ifctx;
> >  
> > -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> > -{
> > -    AcpiBuildPciBusHotplugState *parent = parent_state;
> > -    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> > -
> > -    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> > +    ifctx = build_alloc_array();
> > +    build_append_byte(ifctx, 0x7B); /* AndOp */
> > +    build_append_byte(ifctx, 0x68); /* Arg0Op */
> > +    build_append_int(ifctx, 0x1U << slot);
> > +    build_append_byte(ifctx, 0x00); /* NullName */
> > +    build_append_byte(ifctx, 0x86); /* NotifyOp */
> > +    build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
> > +    build_append_byte(ifctx, 0x69); /* Arg1Op */
> >  
> > -    return child;
> > +    /* Pack it up */
> > +    build_package(ifctx, 0xA0 /* IfOp */);
> > +    build_append_array(method, ifctx);
> > +    build_free_array(ifctx);
> >  }
> >  
> > -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > +static void build_append_pci_bus_devices(GArray *parent_scope, 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;
> > +    GArray *method = NULL;
> >      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;
> > -    }
> > +    PCIBus *sec;
> > +    int i;
> >  
> >      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);
> > +        build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
> >      } else {
> > -        op = 0x10; /* ScopeOp */;
> >          build_append_namestring(bus_table, "PCI0");
> >      }
> >  
> > @@ -718,171 +685,103 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >          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);
> > +        method = build_alloc_method("DVNT", 2);
> >      }
> >  
> > -    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) {
> > +            if (bsel) {
> > +                void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> > +                patch_pcihp(slot, pcihp);
> > +
> > +                build_append_pcihp_notify_entry(method, slot);
> > +            }
> >              continue;
> >          }
> >  
> > -        set_bit(slot, slot_device_present);
> >          pc = PCI_DEVICE_GET_CLASS(pdev);
> >          dc = DEVICE_GET_CLASS(pdev);
> >  
> > -        /* 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;
> > -
> > -        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> > -            set_bit(slot, slot_device_system);
> > +        if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> > +            continue;
> >          }
> >  
> >          if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > -            set_bit(slot, slot_device_vga);
> >  
> >              if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> > -                set_bit(slot, slot_device_qxl);
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCIQXL_SIZEOF);
> > +                      memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> > +                      patch_pciqxl(slot, pcihp);
> > +            } else {
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCIVGA_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> > +                patch_pcivga(slot, pcihp);
> >              }
> > -        }
> > -
> > -        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++) {
> > -        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);
> > +        } else if (dc->hotpluggable && !pc->is_bridge) {
> > +            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 (bsel) {
> > -        method = build_alloc_method("DVNT", 2);
> > -
> > -        for (i = 0; i < PCI_SLOT_MAX; i++) {
> > -            GArray *notify;
> > -            uint8_t op;
> > +            patch_pcihp(slot, pcihp);
> >  
> > -            if (!test_bit(i, slot_hotplug_enable)) {
> > -                continue;
> > +            if (bsel) {
> > +                build_append_pcihp_notify_entry(method, slot);
> >              }
> > +        } else {
> > +            void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
> > +            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > +            patch_pcinohp(slot, 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 (pc->is_bridge) {
> > +                PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> >  
> > -            build_free_array(notify);
> > +                build_append_pci_bus_devices(bus_table, sec_bus,
> > +                                             pcihp_bridge_en);
> > +            }
> >          }
> > +    }
> >  
> > +    if (bsel) {
> >          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) {
> > -        method = build_alloc_method("PCNT", 0);
> > -
> > -        /* If bus supports hotplug select it and notify about local events */
> > -        if (bsel) {
> > -            build_append_byte(method, 0x70); /* StoreOp */
> > -            build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > -            build_append_namestring(method, "BNUM");
> > -            build_append_namestring(method, "DVNT");
> > -            build_append_namestring(method, "PCIU");
> > -            build_append_int(method, 1); /* Device Check */
> > -            build_append_namestring(method, "DVNT");
> > -            build_append_namestring(method, "PCID");
> > -            build_append_int(method, 3); /* Eject Request */
> > -        }
> > -
> > -        /* 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);
> > +    method = build_alloc_method("PCNT", 0);
> >  
> > -        /* Pack it up */
> > -        if (bus->parent_dev) {
> > -            build_extop_package(bus_table, op);
> > -        } else {
> > -            build_package(bus_table, op);
> > -        }
> > -
> > -        /* Append our bus description to parent table */
> > -        build_append_array(parent->device_table, bus_table);
> > +    /* If bus supports hotplug select it and notify about local events */
> > +    if (bsel) {
> > +        build_append_byte(method, 0x70); /* StoreOp */
> > +        build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > +        build_append_namestring(method, "BNUM");
> > +        build_append_namestring(method, "DVNT");
> > +        build_append_namestring(method, "PCIU");
> > +        build_append_int(method, 1); /* Device Check */
> > +        build_append_namestring(method, "DVNT");
> > +        build_append_namestring(method, "PCID");
> > +        build_append_int(method, 3); /* Eject Request */
> > +    }
> >  
> > -        /* 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);
> > +    /* Notify about child bus events in any case */
> > +    if (pcihp_bridge_en) {
> > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > +            build_append_namestring(method, "^S%.02X.PCNT",
> > +                                    sec->parent_dev->devfn);
> >          }
> >      }
> >  
> > -    qobject_decref(bsel);
> > +    build_append_and_cleanup_method(bus_table, method);
> > +
> > +    build_package(bus_table, 0x10); /* ScopeOp */
> > +    build_append_array(parent_scope, bus_table);
> >      build_free_array(bus_table);
> > -    build_pci_bus_state_cleanup(child);
> > -    g_free(child);
> >  }
> >  
> >  static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> > @@ -1014,7 +913,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          }
> >  
> >          {
> > -            AcpiBuildPciBusHotplugState hotplug_state;
> >              Object *pci_host;
> >              PCIBus *bus = NULL;
> >              bool ambiguous;
> > @@ -1024,16 +922,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
> > 
>