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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).