qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree ge


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation
Date: Wed, 21 Jan 2015 11:51:36 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0

On 01/21/2015 11:09 AM, 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 <address@hidden>
---
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 a010f3b..5255428 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;
@@ -651,69 +650,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");
      }

@@ -722,17 +689,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;
@@ -741,152 +700,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.
+                 */
+                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 */
+    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)
@@ -1035,7 +946,6 @@ build_ssdt(GArray *table_data, GArray *linker,
          }

          {
-            AcpiBuildPciBusHotplugState hotplug_state;
              Object *pci_host;
              PCIBus *bus = NULL;
              bool ambiguous;
@@ -1045,16 +955,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);


This one is tricky to review...
I do agree with the concept, I like the simplification,
and no obvious errors were found.

So ... "light":
Reviewed-by: Marcel Apfelbaum <address@hidden>




reply via email to

[Prev in Thread] Current Thread [Next in Thread]