[Qemu-devel] [PATCH 1/2] acpi-build: fix memory leak with bridge hp off

[Qemu-devel] [PATCH 1/2] acpi-build: fix memory leak with bridge hp off

[Michael S. Tsirkin]

When bridge hotplug is disabled for old machine types,
we never free memory allocated for temporary tables.
Fix this up.

Cc: qemu-stable@nongnu.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4944249..74586f3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -859,6 +859,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
      * to make acpi tables compatible with legacy machine types.
      */
     if (!child->pcihp_bridge_en && bus->parent_dev) {
+        build_free_array(bus_table);
+        build_pci_bus_state_cleanup(child);
+        g_free(child);
         return;
     }
 
-- 
MST

[Qemu-devel] [PATCH 2/2] acpi-build: skip hotplugged bridges

[Michael S. Tsirkin]

hotplugged bridges don't get bsel allocated so acpi hotplug doesn't work
for them anyway.  OTOH adding them in ACPI creates a host of problems,
e.g. they can't be hot-unplugged themselves which is surprising to
users.

So let's just skip these.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 74586f3..ff42de5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -857,8 +857,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
     /*
      * Skip bridge subtree creation if bridge hotplug is disabled
      * to make acpi tables compatible with legacy machine types.
+     * Skip creation for hotplugged bridges as well.
      */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
+    if (bus->parent_dev && (!child->pcihp_bridge_en ||
+                            DEVICE(bus->parent_dev)->hotplugged)) {
         build_free_array(bus_table);
         build_pci_bus_state_cleanup(child);
         g_free(child);
@@ -915,8 +917,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         /* 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.
+         * Hotplugged bridges *are* hot-pluggable.
          */
-        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
+        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en &&
+         !DEVICE(pdev)->hotplugged;
 
         if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
             set_bit(slot, slot_device_system);
-- 
MST

Re: [Qemu-devel] [PATCH 2/2] acpi-build: skip hotplugged bridges

[Igor Mammedov]

On Wed, 28 Jan 2015 18:30:38 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> hotplugged bridges don't get bsel allocated so acpi hotplug doesn't work
> for them anyway.  OTOH adding them in ACPI creates a host of problems,
> e.g. they can't be hot-unplugged themselves which is surprising to
> users.
> 
> So let's just skip these.

While reviewing this patch I've found out that in current master
hotplug bridge + reboot creates shrunk SSDT  which leads to
RSDT offset shift -> static RSDP no longer points at it.

Sequence of events are following:
 
  1: on reboot create subtree with hotplugged bridge device but without slots since
       1: hotplugged bridge doesn't have BSEL -> can_eject = false for every slot
       2: every slot marked as non present since no device plugged in it
     that leads to skipping creation of slot devices inside bridge
  2. condition
       if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev)
     also completely skips creation of notify_table and device_table,
     hence parent bus never adds bridge subtree to its context.
  3. since for the slot where bridge was hotplugged
       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); 
     its parent bus, completely skips creation of slot device relying
     on it being provided by child->device_table but due to #2
     it doesn't happen.
As result of all above PCI0 tree description shrinks on 56 bytes,
size of skipped original hotplug slot.


> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/acpi-build.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 74586f3..ff42de5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -857,8 +857,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>      /*
>       * Skip bridge subtree creation if bridge hotplug is disabled
>       * to make acpi tables compatible with legacy machine types.
> +     * Skip creation for hotplugged bridges as well.
>       */
> -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> +    if (bus->parent_dev && (!child->pcihp_bridge_en ||
> +                            DEVICE(bus->parent_dev)->hotplugged)) {
according to above findings this doesn't change anything,
since device_table won't be populated in the end anyway.
It just makes outcome clearer.

but following hunk makes a difference.

>          build_free_array(bus_table);
>          build_pci_bus_state_cleanup(child);
>          g_free(child);
> @@ -915,8 +917,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          /* 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.
> +         * Hotplugged bridges *are* hot-pluggable.
>           */
> -        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> +        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en &&
> +         !DEVICE(pdev)->hotplugged;
with this, current bus tree keeps hotplugged slot description for slot
where bridge was hotplugged and keeps SSDT size exactly the same across
reboot. But it has side effect that it would be possible to invoke
hot-unplug on it -> I don't know what side effects it will cause at the end.

However it doesn't fix immutable RSDP pointing to fixed RSDT offset,
i.e. it's fixing symptoms until the next time we hit shifted RSDT problem.
Proper fix would be make RSDP updated on reboot along with currently
updated ACPI tables blob.

That also rises a question: do we need to rebuild ACPI tables on
reboot at all? If yes, Why?

>  
>          if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
>              set_bit(slot, slot_device_system);

Re: [Qemu-devel] [PATCH 1/2] acpi-build: fix memory leak with bridge hp off

[Igor Mammedov]

On Wed, 28 Jan 2015 18:30:34 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> When bridge hotplug is disabled for old machine types,
> we never free memory allocated for temporary tables.
> Fix this up.
patch "pc: acpi-build: simplify PCI bus tree generation"
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04030.html
obsoletes this patch since it replaces old code with leak with
a more simple one where this bug do not exists.

> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/acpi-build.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4944249..74586f3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -859,6 +859,9 @@ static void build_pci_bus_end(PCIBus *bus, void
> *bus_state)
>       * to make acpi tables compatible with legacy machine types.
>       */
>      if (!child->pcihp_bridge_en && bus->parent_dev) {
> +        build_free_array(bus_table);
> +        build_pci_bus_state_cleanup(child);
> +        g_free(child);
>          return;
>      }
>