qemu-devel
[Top][All Lists]
Advanced

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

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
Date: Wed, 28 Jan 2015 17:21:21 +0200

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 <address@hidden>
> Reviewed-by: Marcel Apfelbaum <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 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.

> +                 */
> +                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.


> +    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



reply via email to

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