[Qemu-devel] [PATCH 0/3] pci: fix memory region lifecycle issues, document the rules

[Qemu-devel] [PATCH 0/3] pci: fix memory region lifecycle issues, document the rules

[Paolo Bonzini]

While these patches were originally in "part 3" of the RCU patches,
it turns out that the semantics they enforce are already important now
(reported by Alex Williamson and Matthew Rosato), so here they are!

Patch 1 fixes a MemoryRegion leak (and fixes it the right way, so that
the new lifecycle rules are respected!).

Patch 2 fixes a case where a memory region could be referenced (in
an RCU callback) when it had no parent, similar to the s390 case.

Patch 3 documents the MemoryRegion lifecycle rules now that (except for
s390, which Matthew is going to fix soon) QEMU actually follows them.

Please review and ACK.  Michael, okay to apply the first two through the
RCU tree?

Paolo

Paolo Bonzini (3):
  pcie: remove mmconfig memory leak and wrap mmconfig update with transaction
  pci: split shpc_cleanup and shpc_free
  docs: clarify memory region lifecycle

 docs/memory.txt                | 74 +++++++++++++++++++++++++++++++++---------
 hw/pci-bridge/pci_bridge_dev.c | 14 +++++---
 hw/pci/pcie_host.c             |  7 ++--
 hw/pci/shpc.c                  |  5 +++
 include/hw/pci/shpc.h          |  1 +
 5 files changed, 79 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] pcie: remove mmconfig memory leak and wrap mmconfig update with transaction

[Paolo Bonzini]

This memory leak was introduced inadvertently by omitting object_unparent.
A better fix is to use the new memory_region_set_size instead of destroying
and recreating the MMIO region on the fly.

Also, ensure that unmapping and remapping the region is done atomically.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pcie_host.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index dfb4a2b..d8afba8 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -88,6 +88,8 @@ static void pcie_host_init(Object *obj)
     PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
 
     e->base_addr = PCIE_BASE_ADDR_UNMAPPED;
+    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e, "pcie-mmcfg-mmio",
+                          PCIE_MMCFG_SIZE_MAX);
 }
 
 void pcie_host_mmcfg_unmap(PCIExpressHost *e)
@@ -104,8 +106,7 @@ void pcie_host_mmcfg_init(PCIExpressHost *e, uint32_t size)
     assert(size >= PCIE_MMCFG_SIZE_MIN);
     assert(size <= PCIE_MMCFG_SIZE_MAX);
     e->size = size;
-    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e,
-                          "pcie-mmcfg", e->size);
+    memory_region_set_size(&e->mmio, e->size);
 }
 
 void pcie_host_mmcfg_map(PCIExpressHost *e, hwaddr addr,
@@ -121,10 +122,12 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
                             hwaddr addr,
                             uint32_t size)
 {
+    memory_region_transaction_begin();
     pcie_host_mmcfg_unmap(e);
     if (enable) {
         pcie_host_mmcfg_map(e, addr, size);
     }
+    memory_region_transaction_commit();
 }
 
 static const TypeInfo pcie_host_type_info = {
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] pci: split shpc_cleanup and shpc_free

[Paolo Bonzini]

object_unparent should not be called until the parent device is going to be
destroyed.  Only remove the capability and do memory_region_del_subregion
at unrealize time.  Freeing the data structures is left in shpc_free, to
be called from the instance_finalize callback.

shpc_free follows the same coding style that Alex suggested for VFIO
(i.e. since a test for NULL is requested, clear the field at end).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++++----
 hw/pci/shpc.c                  | 11 ++++++++++-
 include/hw/pci/shpc.h          |  1 +
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 252ea5e..36f73e1 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -97,6 +97,11 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
     pci_bridge_exitfn(dev);
 }
 
+static void pci_bridge_dev_instance_finalize(Object *obj)
+{
+    shpc_free(PCI_DEVICE(obj));
+}
+
 static void pci_bridge_dev_write_config(PCIDevice *d,
                                         uint32_t address, uint32_t val, int len)
 {
@@ -154,10 +159,11 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo pci_bridge_dev_info = {
-    .name          = TYPE_PCI_BRIDGE_DEV,
-    .parent        = TYPE_PCI_BRIDGE,
-    .instance_size = sizeof(PCIBridgeDev),
-    .class_init = pci_bridge_dev_class_init,
+    .name              = TYPE_PCI_BRIDGE_DEV,
+    .parent            = TYPE_PCI_BRIDGE,
+    .instance_size     = sizeof(PCIBridgeDev),
+    .class_init        = pci_bridge_dev_class_init,
+    .instance_finalize = pci_bridge_dev_instance_finalize,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_HOTPLUG_HANDLER },
         { }
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 27c496e..5fd7f4b 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -663,13 +663,22 @@ void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
     SHPCDevice *shpc = d->shpc;
     d->cap_present &= ~QEMU_PCI_CAP_SHPC;
     memory_region_del_subregion(bar, &shpc->mmio);
-    object_unparent(OBJECT(&shpc->mmio));
     /* TODO: cleanup config space changes? */
+}
+
+void shpc_free(PCIDevice *d)
+{
+    SHPCDevice *shpc = d->shpc;
+    if (!shpc) {
+        return;
+    }
+    object_unparent(OBJECT(&shpc->mmio));
     g_free(shpc->config);
     g_free(shpc->cmask);
     g_free(shpc->wmask);
     g_free(shpc->w1cmask);
     g_free(shpc);
+    d->shpc = NULL;
 }
 
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 025bc5b..9bbea39 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -41,6 +41,7 @@ void shpc_reset(PCIDevice *d);
 int shpc_bar_size(PCIDevice *dev);
 int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
+void shpc_free(PCIDevice *dev);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] docs: clarify memory region lifecycle

[Paolo Bonzini]

Now that objects actually obey the rules, document them.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/memory.txt | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 16 deletions(-)

diff --git a/docs/memory.txt b/docs/memory.txt
index b12f1f0..2ceb348 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -73,17 +73,66 @@ stability.
 Region lifecycle
 ----------------
 
-A region is created by one of the constructor functions (memory_region_init*())
-and attached to an object.  It is then destroyed by object_unparent() or simply
-when the parent object dies.
+A region is created by one of the memory_region_init*() functions and
+attached to an object, which acts as its owner or parent.  QEMU ensures
+that the owner object remains alive as long as the region is visible to
+the guest, or as long as the region is in use by a virtual CPU or another
+device.  For example, the owner object will not die between an
+address_space_map operation and the corresponding address_space_unmap.
 
-In between, a region can be added to an address space
-by using memory_region_add_subregion() and removed using
-memory_region_del_subregion().  Destroying the region implicitly
-removes the region from the address space.
+After creation, a region can be added to an address space or a
+container with memory_region_add_subregion(), and removed using
+memory_region_del_subregion().
 
-Region attributes may be changed at any point; they take effect once
-the region becomes exposed to the guest.
+Various region attributes (read-only, dirty logging, coalesced mmio,
+ioeventfd) can be changed during the region lifecycle.  They take effect
+as soon as the region is made visible.  This can be immediately, later,
+or never.
+
+Destruction of a memory region happens automatically when the owner
+object dies.
+
+If however the memory region is part of a dynamically allocated data
+structure, you should call object_unparent() to destroy the memory region
+before the data structure is freed.  For an example see VFIOMSIXInfo
+and VFIOQuirk in hw/vfio/pci.c.
+
+You must not destroy a memory region as long as it may be in use by a
+device or CPU.  In order to do this, as a general rule do not create or
+destroy memory regions dynamically during a device's lifetime, and only
+call object_unparent() in the memory region owner's instance_finalize
+callback.  The dynamically allocated data structure that contains the
+memory region then should obviously be freed in the instance_finalize
+callback as well.
+
+If you break this rule, the following situation can happen:
+
+- the memory region's owner had a reference taken via memory_region_ref
+  (for example by address_space_map)
+
+- the region is unparented, and has no owner anymore
+
+- when address_space_unmap is called, the reference to the memory region's
+  owner is leaked.
+
+
+There is an exception to the above rule: it is okay to call
+object_unparent at any time for an alias or a container region.  It is
+therefore also okay to create or destroy alias and container regions
+dynamically during a device's lifetime.
+
+This exceptional usage is valid because aliases and containers only help
+QEMU building the guest's memory map; they are never accessed directly.
+memory_region_ref and memory_region_unref are never called on aliases
+or containers, and the above situation then cannot happen.  Exploiting
+this exception is rarely necessary, and therefore it is discouraged,
+but nevertheless it is used in a few places.
+
+For regions that "have no owner" (NULL is passed at creation time), the
+machine object is actually used as the owner.  Since instance_finalize is
+never called for the machine object, you must never call object_unparent
+on regions that have no owner, unless they are aliases or containers.
+
 
 Overlapping regions and priority
 --------------------------------
@@ -215,13 +264,6 @@ BAR containing MMIO registers is mapped after it.
 Note that if the guest maps a BAR outside the PCI hole, it would not be
 visible as the pci-hole alias clips it to a 0.5GB range.
 
-Attributes
-----------
-
-Various region attributes (read-only, dirty logging, coalesced mmio, ioeventfd)
-can be changed during the region lifecycle.  They take effect once the region
-is made visible (which can be immediately, later, or never).
-
 MMIO Operations
 ---------------
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] docs: clarify memory region lifecycle

[Matthew Rosato]

On 02/13/2015 09:57 AM, Paolo Bonzini wrote:
> Now that objects actually obey the rules, document them.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  docs/memory.txt | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/docs/memory.txt b/docs/memory.txt
> index b12f1f0..2ceb348 100644
> --- a/docs/memory.txt
> +++ b/docs/memory.txt
> @@ -73,17 +73,66 @@ stability.
>  Region lifecycle
>  ----------------
> 
> -A region is created by one of the constructor functions (memory_region_init*())
> -and attached to an object.  It is then destroyed by object_unparent() or simply
> -when the parent object dies.
> +A region is created by one of the memory_region_init*() functions and
> +attached to an object, which acts as its owner or parent.  QEMU ensures
> +that the owner object remains alive as long as the region is visible to
> +the guest, or as long as the region is in use by a virtual CPU or another
> +device.  For example, the owner object will not die between an
> +address_space_map operation and the corresponding address_space_unmap.
> 
> -In between, a region can be added to an address space
> -by using memory_region_add_subregion() and removed using
> -memory_region_del_subregion().  Destroying the region implicitly
> -removes the region from the address space.
> +After creation, a region can be added to an address space or a
> +container with memory_region_add_subregion(), and removed using
> +memory_region_del_subregion().
> 
> -Region attributes may be changed at any point; they take effect once
> -the region becomes exposed to the guest.
> +Various region attributes (read-only, dirty logging, coalesced mmio,
> +ioeventfd) can be changed during the region lifecycle.  They take effect
> +as soon as the region is made visible.  This can be immediately, later,
> +or never.
> +
> +Destruction of a memory region happens automatically when the owner
> +object dies.
> +
> +If however the memory region is part of a dynamically allocated data
> +structure, you should call object_unparent() to destroy the memory region
> +before the data structure is freed.  For an example see VFIOMSIXInfo
> +and VFIOQuirk in hw/vfio/pci.c.
> +
> +You must not destroy a memory region as long as it may be in use by a
> +device or CPU.  In order to do this, as a general rule do not create or
> +destroy memory regions dynamically during a device's lifetime, and only
> +call object_unparent() in the memory region owner's instance_finalize
> +callback.  The dynamically allocated data structure that contains the
> +memory region then should obviously be freed in the instance_finalize
> +callback as well.
> +
> +If you break this rule, the following situation can happen:
> +
> +- the memory region's owner had a reference taken via memory_region_ref
> +  (for example by address_space_map)
> +
> +- the region is unparented, and has no owner anymore
> +
> +- when address_space_unmap is called, the reference to the memory region's
> +  owner is leaked.
> +
> +
> +There is an exception to the above rule: it is okay to call
> +object_unparent at any time for an alias or a container region.  It is
> +therefore also okay to create or destroy alias and container regions
> +dynamically during a device's lifetime.
> +
> +This exceptional usage is valid because aliases and containers only help
> +QEMU building the guest's memory map; they are never accessed directly.
> +memory_region_ref and memory_region_unref are never called on aliases
> +or containers, and the above situation then cannot happen.  Exploiting
> +this exception is rarely necessary, and therefore it is discouraged,
> +but nevertheless it is used in a few places.
> +
> +For regions that "have no owner" (NULL is passed at creation time), the
> +machine object is actually used as the owner.  Since instance_finalize is
> +never called for the machine object, you must never call object_unparent
> +on regions that have no owner, unless they are aliases or containers.
> +
> 
>  Overlapping regions and priority
>  --------------------------------
> @@ -215,13 +264,6 @@ BAR containing MMIO registers is mapped after it.
>  Note that if the guest maps a BAR outside the PCI hole, it would not be
>  visible as the pci-hole alias clips it to a 0.5GB range.
> 
> -Attributes
> -----------
> -
> -Various region attributes (read-only, dirty logging, coalesced mmio, ioeventfd)
> -can be changed during the region lifecycle.  They take effect once the region
> -is made visible (which can be immediately, later, or never).
> -
>  MMIO Operations
>  ---------------
> 

Re: [Qemu-devel] [PATCH 2/3] pci: split shpc_cleanup and shpc_free

[Matthew Rosato]

On 02/13/2015 09:57 AM, Paolo Bonzini wrote:
> object_unparent should not be called until the parent device is going to be
> destroyed.  Only remove the capability and do memory_region_del_subregion
> at unrealize time.  Freeing the data structures is left in shpc_free, to
> be called from the instance_finalize callback.
> 
> shpc_free follows the same coding style that Alex suggested for VFIO
> (i.e. since a test for NULL is requested, clear the field at end).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>

> ---
>  hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++++----
>  hw/pci/shpc.c                  | 11 ++++++++++-
>  include/hw/pci/shpc.h          |  1 +
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 252ea5e..36f73e1 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -97,6 +97,11 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
>      pci_bridge_exitfn(dev);
>  }
> 
> +static void pci_bridge_dev_instance_finalize(Object *obj)
> +{
> +    shpc_free(PCI_DEVICE(obj));
> +}
> +
>  static void pci_bridge_dev_write_config(PCIDevice *d,
>                                          uint32_t address, uint32_t val, int len)
>  {
> @@ -154,10 +159,11 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>  }
> 
>  static const TypeInfo pci_bridge_dev_info = {
> -    .name          = TYPE_PCI_BRIDGE_DEV,
> -    .parent        = TYPE_PCI_BRIDGE,
> -    .instance_size = sizeof(PCIBridgeDev),
> -    .class_init = pci_bridge_dev_class_init,
> +    .name              = TYPE_PCI_BRIDGE_DEV,
> +    .parent            = TYPE_PCI_BRIDGE,
> +    .instance_size     = sizeof(PCIBridgeDev),
> +    .class_init        = pci_bridge_dev_class_init,
> +    .instance_finalize = pci_bridge_dev_instance_finalize,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_HOTPLUG_HANDLER },
>          { }
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 27c496e..5fd7f4b 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -663,13 +663,22 @@ void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
>      SHPCDevice *shpc = d->shpc;
>      d->cap_present &= ~QEMU_PCI_CAP_SHPC;
>      memory_region_del_subregion(bar, &shpc->mmio);
> -    object_unparent(OBJECT(&shpc->mmio));
>      /* TODO: cleanup config space changes? */
> +}
> +
> +void shpc_free(PCIDevice *d)
> +{
> +    SHPCDevice *shpc = d->shpc;
> +    if (!shpc) {
> +        return;
> +    }
> +    object_unparent(OBJECT(&shpc->mmio));
>      g_free(shpc->config);
>      g_free(shpc->cmask);
>      g_free(shpc->wmask);
>      g_free(shpc->w1cmask);
>      g_free(shpc);
> +    d->shpc = NULL;
>  }
> 
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index 025bc5b..9bbea39 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -41,6 +41,7 @@ void shpc_reset(PCIDevice *d);
>  int shpc_bar_size(PCIDevice *dev);
>  int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>  void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
> +void shpc_free(PCIDevice *dev);
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
> 
> 

Re: [Qemu-devel] [PATCH 1/3] pcie: remove mmconfig memory leak and wrap mmconfig update with transaction

[Igor Mammedov]

On Fri, 13 Feb 2015 15:57:09 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This memory leak was introduced inadvertently by omitting object_unparent.
> A better fix is to use the new memory_region_set_size instead of destroying
> and recreating the MMIO region on the fly.
> 
> Also, ensure that unmapping and remapping the region is done atomically.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/pci/pcie_host.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index dfb4a2b..d8afba8 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -88,6 +88,8 @@ static void pcie_host_init(Object *obj)
>      PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
>  
>      e->base_addr = PCIE_BASE_ADDR_UNMAPPED;
> +    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e, "pcie-mmcfg-mmio",
> +                          PCIE_MMCFG_SIZE_MAX);
>  }
>  
>  void pcie_host_mmcfg_unmap(PCIExpressHost *e)
> @@ -104,8 +106,7 @@ void pcie_host_mmcfg_init(PCIExpressHost *e, uint32_t size)
>      assert(size >= PCIE_MMCFG_SIZE_MIN);
>      assert(size <= PCIE_MMCFG_SIZE_MAX);
>      e->size = size;
> -    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e,
> -                          "pcie-mmcfg", e->size);
> +    memory_region_set_size(&e->mmio, e->size);
>  }
>  
>  void pcie_host_mmcfg_map(PCIExpressHost *e, hwaddr addr,
> @@ -121,10 +122,12 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>                              hwaddr addr,
>                              uint32_t size)
>  {
> +    memory_region_transaction_begin();
>      pcie_host_mmcfg_unmap(e);
>      if (enable) {
>          pcie_host_mmcfg_map(e, addr, size);
>      }
> +    memory_region_transaction_commit();
>  }
>  
>  static const TypeInfo pcie_host_type_info = {

Re: [Qemu-devel] [PATCH 1/3] pcie: remove mmconfig memory leak and wrap mmconfig update with transaction

[Michael S. Tsirkin]

On Fri, Feb 13, 2015 at 03:57:09PM +0100, Paolo Bonzini wrote:
> This memory leak was introduced inadvertently by omitting object_unparent.
> A better fix is to use the new memory_region_set_size instead of destroying
> and recreating the MMIO region on the fly.
> 
> Also, ensure that unmapping and remapping the region is done atomically.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci/pcie_host.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index dfb4a2b..d8afba8 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -88,6 +88,8 @@ static void pcie_host_init(Object *obj)
>      PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
>  
>      e->base_addr = PCIE_BASE_ADDR_UNMAPPED;
> +    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e, "pcie-mmcfg-mmio",
> +                          PCIE_MMCFG_SIZE_MAX);
>  }
>  
>  void pcie_host_mmcfg_unmap(PCIExpressHost *e)
> @@ -104,8 +106,7 @@ void pcie_host_mmcfg_init(PCIExpressHost *e, uint32_t size)
>      assert(size >= PCIE_MMCFG_SIZE_MIN);
>      assert(size <= PCIE_MMCFG_SIZE_MAX);
>      e->size = size;
> -    memory_region_init_io(&e->mmio, OBJECT(e), &pcie_mmcfg_ops, e,
> -                          "pcie-mmcfg", e->size);
> +    memory_region_set_size(&e->mmio, e->size);
>  }
>  
>  void pcie_host_mmcfg_map(PCIExpressHost *e, hwaddr addr,
> @@ -121,10 +122,12 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>                              hwaddr addr,
>                              uint32_t size)
>  {
> +    memory_region_transaction_begin();
>      pcie_host_mmcfg_unmap(e);
>      if (enable) {
>          pcie_host_mmcfg_map(e, addr, size);
>      }
> +    memory_region_transaction_commit();
>  }
>  
>  static const TypeInfo pcie_host_type_info = {
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 2/3] pci: split shpc_cleanup and shpc_free

[Michael S. Tsirkin]

On Fri, Feb 13, 2015 at 03:57:10PM +0100, Paolo Bonzini wrote:
> object_unparent should not be called until the parent device is going to be
> destroyed.  Only remove the capability and do memory_region_del_subregion
> at unrealize time.  Freeing the data structures is left in shpc_free, to
> be called from the instance_finalize callback.
> 
> shpc_free follows the same coding style that Alex suggested for VFIO
> (i.e. since a test for NULL is requested, clear the field at end).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/pci-bridge/pci_bridge_dev.c | 14 ++++++++++----
>  hw/pci/shpc.c                  | 11 ++++++++++-
>  include/hw/pci/shpc.h          |  1 +
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 252ea5e..36f73e1 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -97,6 +97,11 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
>      pci_bridge_exitfn(dev);
>  }
>  
> +static void pci_bridge_dev_instance_finalize(Object *obj)
> +{
> +    shpc_free(PCI_DEVICE(obj));
> +}
> +
>  static void pci_bridge_dev_write_config(PCIDevice *d,
>                                          uint32_t address, uint32_t val, int len)
>  {
> @@ -154,10 +159,11 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>  }
>  
>  static const TypeInfo pci_bridge_dev_info = {
> -    .name          = TYPE_PCI_BRIDGE_DEV,
> -    .parent        = TYPE_PCI_BRIDGE,
> -    .instance_size = sizeof(PCIBridgeDev),
> -    .class_init = pci_bridge_dev_class_init,
> +    .name              = TYPE_PCI_BRIDGE_DEV,
> +    .parent            = TYPE_PCI_BRIDGE,
> +    .instance_size     = sizeof(PCIBridgeDev),
> +    .class_init        = pci_bridge_dev_class_init,
> +    .instance_finalize = pci_bridge_dev_instance_finalize,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_HOTPLUG_HANDLER },
>          { }
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 27c496e..5fd7f4b 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -663,13 +663,22 @@ void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
>      SHPCDevice *shpc = d->shpc;
>      d->cap_present &= ~QEMU_PCI_CAP_SHPC;
>      memory_region_del_subregion(bar, &shpc->mmio);
> -    object_unparent(OBJECT(&shpc->mmio));
>      /* TODO: cleanup config space changes? */
> +}
> +
> +void shpc_free(PCIDevice *d)
> +{
> +    SHPCDevice *shpc = d->shpc;
> +    if (!shpc) {
> +        return;
> +    }
> +    object_unparent(OBJECT(&shpc->mmio));
>      g_free(shpc->config);
>      g_free(shpc->cmask);
>      g_free(shpc->wmask);
>      g_free(shpc->w1cmask);
>      g_free(shpc);
> +    d->shpc = NULL;
>  }
>  
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
> index 025bc5b..9bbea39 100644
> --- a/include/hw/pci/shpc.h
> +++ b/include/hw/pci/shpc.h
> @@ -41,6 +41,7 @@ void shpc_reset(PCIDevice *d);
>  int shpc_bar_size(PCIDevice *dev);
>  int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
>  void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
> +void shpc_free(PCIDevice *dev);
>  void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
>  
>  
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 0/3] pci: fix memory region lifecycle issues, document the rules

[Michael S. Tsirkin]

On Fri, Feb 13, 2015 at 03:57:08PM +0100, Paolo Bonzini wrote:
> While these patches were originally in "part 3" of the RCU patches,
> it turns out that the semantics they enforce are already important now
> (reported by Alex Williamson and Matthew Rosato), so here they are!
> 
> Patch 1 fixes a MemoryRegion leak (and fixes it the right way, so that
> the new lifecycle rules are respected!).
> 
> Patch 2 fixes a case where a memory region could be referenced (in
> an RCU callback) when it had no parent, similar to the s390 case.
> 
> Patch 3 documents the MemoryRegion lifecycle rules now that (except for
> s390, which Matthew is going to fix soon) QEMU actually follows them.
> 
> Please review and ACK.  Michael, okay to apply the first two through the
> RCU tree?
> 
> Paolo

Since you mention you have patches depending on these queued:

Acked-by: Michael S. Tsirkin <mst@redhat.com>


> Paolo Bonzini (3):
>   pcie: remove mmconfig memory leak and wrap mmconfig update with transaction
>   pci: split shpc_cleanup and shpc_free
>   docs: clarify memory region lifecycle
> 
>  docs/memory.txt                | 74 +++++++++++++++++++++++++++++++++---------
>  hw/pci-bridge/pci_bridge_dev.c | 14 +++++---
>  hw/pci/pcie_host.c             |  7 ++--
>  hw/pci/shpc.c                  |  5 +++
>  include/hw/pci/shpc.h          |  1 +
>  5 files changed, 79 insertions(+), 22 deletions(-)
> 
> -- 
> 1.8.3.1