The recent introduction of a bus master container added
memory_region_add_subregion() into the PCI device registering path but
missed memory_region_del_subregion() in the unregistering path leaving
a reference to the root memory region of the new container.
This adds missing memory_region_del_subregion().
Fixes: 3716d5902d743 ("pci: introduce a bus master container")
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
hw/pci/pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e6b08e1988..bd8043c460 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
pci_dev->bus->devices[pci_dev->devfn] = NULL;
pci_config_free(pci_dev);
+ memory_region_del_subregion(&pci_dev->bus_master_container_region,
+ &pci_dev->bus_master_enable_region);
address_space_destroy(&pci_dev->bus_master_as);
}
--
2.11.0
On 27/03/2017 06:40, Alexey Kardashevskiy wrote: > The recent introduction of a bus master container added > memory_region_add_subregion() into the PCI device registering path but > missed memory_region_del_subregion() in the unregistering path leaving > a reference to the root memory region of the new container. > > This adds missing memory_region_del_subregion(). > > Fixes: 3716d5902d743 ("pci: introduce a bus master container") > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/pci/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index e6b08e1988..bd8043c460 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > pci_dev->bus->devices[pci_dev->devfn] = NULL; > pci_config_free(pci_dev); > > + memory_region_del_subregion(&pci_dev->bus_master_container_region, > + &pci_dev->bus_master_enable_region); > address_space_destroy(&pci_dev->bus_master_as); > } > > My own review fail. The enable subregion would be deleted when a memory region is finalized, but the enable subregions is keeping the owner alive. And until the owner is alive, the container region is not deleted either. So there is a reference count cycle, which we need to break. It's probably good to revisit commit 2e2b8eb ("memory: allow destroying a non-empty MemoryRegion", 2015-10-01). For 2.9, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
On Mon, Mar 27, 2017 at 05:28:17PM +0200, Paolo Bonzini wrote: > > > On 27/03/2017 06:40, Alexey Kardashevskiy wrote: > > The recent introduction of a bus master container added > > memory_region_add_subregion() into the PCI device registering path but > > missed memory_region_del_subregion() in the unregistering path leaving > > a reference to the root memory region of the new container. > > > > This adds missing memory_region_del_subregion(). > > > > Fixes: 3716d5902d743 ("pci: introduce a bus master container") > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > hw/pci/pci.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index e6b08e1988..bd8043c460 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > > pci_dev->bus->devices[pci_dev->devfn] = NULL; > > pci_config_free(pci_dev); > > > > + memory_region_del_subregion(&pci_dev->bus_master_container_region, > > + &pci_dev->bus_master_enable_region); > > address_space_destroy(&pci_dev->bus_master_as); > > } > > > > > > My own review fail. The enable subregion would be deleted when a memory > region is finalized, but the enable subregions is keeping the owner > alive. And until the owner is alive, the container region is not > deleted either. So there is a reference count cycle, which we need to > break. > > It's probably good to revisit commit 2e2b8eb ("memory: allow destroying > a non-empty MemoryRegion", 2015-10-01). For 2.9, > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Merged to ppc-for-2.9. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Tue, Mar 28, 2017 at 11:06:28AM +1100, David Gibson wrote: > On Mon, Mar 27, 2017 at 05:28:17PM +0200, Paolo Bonzini wrote: > > > > > > On 27/03/2017 06:40, Alexey Kardashevskiy wrote: > > > The recent introduction of a bus master container added > > > memory_region_add_subregion() into the PCI device registering path but > > > missed memory_region_del_subregion() in the unregistering path leaving > > > a reference to the root memory region of the new container. > > > > > > This adds missing memory_region_del_subregion(). > > > > > > Fixes: 3716d5902d743 ("pci: introduce a bus master container") > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > --- > > > hw/pci/pci.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > index e6b08e1988..bd8043c460 100644 > > > --- a/hw/pci/pci.c > > > +++ b/hw/pci/pci.c > > > @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > > > pci_dev->bus->devices[pci_dev->devfn] = NULL; > > > pci_config_free(pci_dev); > > > > > > + memory_region_del_subregion(&pci_dev->bus_master_container_region, > > > + &pci_dev->bus_master_enable_region); > > > address_space_destroy(&pci_dev->bus_master_as); > > > } > > > > > > > > > > My own review fail. The enable subregion would be deleted when a memory > > region is finalized, but the enable subregions is keeping the owner > > alive. And until the owner is alive, the container region is not > > deleted either. So there is a reference count cycle, which we need to > > break. > > > > It's probably good to revisit commit 2e2b8eb ("memory: allow destroying > > a non-empty MemoryRegion", 2015-10-01). For 2.9, > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Merged to ppc-for-2.9. I have it on pci branch, seems more appropriate. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Tue, Mar 28, 2017 at 05:33:48AM +0300, Michael S. Tsirkin wrote: > On Tue, Mar 28, 2017 at 11:06:28AM +1100, David Gibson wrote: > > On Mon, Mar 27, 2017 at 05:28:17PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 27/03/2017 06:40, Alexey Kardashevskiy wrote: > > > > The recent introduction of a bus master container added > > > > memory_region_add_subregion() into the PCI device registering path but > > > > missed memory_region_del_subregion() in the unregistering path leaving > > > > a reference to the root memory region of the new container. > > > > > > > > This adds missing memory_region_del_subregion(). > > > > > > > > Fixes: 3716d5902d743 ("pci: introduce a bus master container") > > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > --- > > > > hw/pci/pci.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > > > index e6b08e1988..bd8043c460 100644 > > > > --- a/hw/pci/pci.c > > > > +++ b/hw/pci/pci.c > > > > @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > > > > pci_dev->bus->devices[pci_dev->devfn] = NULL; > > > > pci_config_free(pci_dev); > > > > > > > > + memory_region_del_subregion(&pci_dev->bus_master_container_region, > > > > + &pci_dev->bus_master_enable_region); > > > > address_space_destroy(&pci_dev->bus_master_as); > > > > } > > > > > > > > > > > > > > My own review fail. The enable subregion would be deleted when a memory > > > region is finalized, but the enable subregions is keeping the owner > > > alive. And until the owner is alive, the container region is not > > > deleted either. So there is a reference count cycle, which we need to > > > break. > > > > > > It's probably good to revisit commit 2e2b8eb ("memory: allow destroying > > > a non-empty MemoryRegion", 2015-10-01). For 2.9, > > > > > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > > > Merged to ppc-for-2.9. > > I have it on pci branch, seems more appropriate. Good point. I've dropped it from my tree. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 2017年03月27日 12:40, Alexey Kardashevskiy wrote: > The recent introduction of a bus master container added > memory_region_add_subregion() into the PCI device registering path but > missed memory_region_del_subregion() in the unregistering path leaving > a reference to the root memory region of the new container. > > This adds missing memory_region_del_subregion(). > > Fixes: 3716d5902d743 ("pci: introduce a bus master container") > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/pci/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index e6b08e1988..bd8043c460 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -869,6 +869,8 @@ static void do_pci_unregister_device(PCIDevice *pci_dev) > pci_dev->bus->devices[pci_dev->devfn] = NULL; > pci_config_free(pci_dev); > > + memory_region_del_subregion(&pci_dev->bus_master_container_region, > + &pci_dev->bus_master_enable_region); > address_space_destroy(&pci_dev->bus_master_as); > } > Acked-by: Jason Wang <jasowang@redhat.com> Thanks!
© 2016 - 2024 Red Hat, Inc.