From: "Michael S. Tsirkin" <mst@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
Date: Wed, 28 Jan 2015 18:02:46 +0200 [thread overview]
Message-ID: <20150128160246.GC32439@redhat.com> (raw)
In-Reply-To: <20150128165336.5a306d0b@nial.brq.redhat.com>
On Wed, Jan 28, 2015 at 04:53:36PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:21:21 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Jan 28, 2015 at 02:34:50PM +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
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > v4:
> > > * keep original logic of creating bridge devices as it was done
> > > in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
> > > * if bus is non hotpluggable, add child slots to bus as non
> > > hotpluggable as it was done in original code.
> > > 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 | 275 +++++++++++++++++----------------------------------
> > > 1 file changed, 90 insertions(+), 185 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index b36ac45..c5d1eba 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -106,7 +106,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;
> > > @@ -650,69 +649,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");
> > > }
> > >
> > > @@ -721,17 +688,9 @@ 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;
> > > @@ -740,152 +699,104 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > 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);
> > > + } else {
> > > + /* no much sense to create empty non hotpluggable slots,
> > > + * but it's what original code did before. TODO: remove it.
> >
> > You can't remove it.
> > Check the commit log, and you will know why.
> I'll drop comment.
>
> >
> > > + */
> > > + void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
> > > + memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > > + patch_pcinohp(slot, pcihp);
> > > + }
> > > 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;
> > > }
> > > + bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
> > >
> > > 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 || bridge_in_acpi) {
> > > - 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 && !bridge_in_acpi) {
> > > + 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);
> > > + /* When hotplug for bridges is enabled, bridges that are
> > > + * described in ACPI separately aren't themselves hot-pluggable.
> > > + */
> > > + if (bridge_in_acpi) {
> > > + 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 */
> >
> > So we have scope even if it's a non root device.
> > I'd prefer it that we put everything in the device
> > directly, when later you rewrite DSDT as well,
> > you will be able to do it for root as well.
> yes, I did exactly this in AML series.
> So lets leave this as it is, AML series will convert it to
> Device(bridge) { child_socket_devices... }
> format
OK, but pls add a comment about this.
> >
> >
> > > + 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)
> > > @@ -1017,7 +928,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > }
> > >
> > > {
> > > - AcpiBuildPciBusHotplugState hotplug_state;
> > > Object *pci_host;
> > > PCIBus *bus = NULL;
> > > bool ambiguous;
> > > @@ -1027,16 +937,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
prev parent reply other threads:[~2015-01-28 16:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
2015-01-28 15:22 ` Michael S. Tsirkin
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper Igor Mammedov
2015-01-28 15:16 ` Michael S. Tsirkin
2015-01-28 15:44 ` Igor Mammedov
2015-01-28 16:07 ` Michael S. Tsirkin
2015-01-30 11:46 ` Igor Mammedov
2015-01-31 17:05 ` Michael S. Tsirkin
2015-02-02 8:31 ` Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
2015-01-28 15:21 ` Michael S. Tsirkin
2015-01-28 15:53 ` Igor Mammedov
2015-01-28 16:02 ` Michael S. Tsirkin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150128160246.GC32439@redhat.com \
--to=mst@redhat.com \
--cc=imammedo@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.