* [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference @ 2017-03-27 4:40 Alexey Kardashevskiy 2017-03-27 15:28 ` Paolo Bonzini 2017-03-29 2:40 ` Jason Wang 0 siblings, 2 replies; 6+ messages in thread From: Alexey Kardashevskiy @ 2017-03-27 4:40 UTC (permalink / raw) To: qemu-devel Cc: Alexey Kardashevskiy, David Gibson, Marcel Apfelbaum, Michael S . Tsirkin, Paolo Bonzini, Jason Wang, Peter Maydell 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference 2017-03-27 4:40 [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference Alexey Kardashevskiy @ 2017-03-27 15:28 ` Paolo Bonzini 2017-03-28 0:06 ` David Gibson 2017-03-29 2:40 ` Jason Wang 1 sibling, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2017-03-27 15:28 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel Cc: David Gibson, Marcel Apfelbaum, Michael S . Tsirkin, Jason Wang, Peter Maydell 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference 2017-03-27 15:28 ` Paolo Bonzini @ 2017-03-28 0:06 ` David Gibson 2017-03-28 2:33 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: David Gibson @ 2017-03-28 0:06 UTC (permalink / raw) To: Paolo Bonzini Cc: Alexey Kardashevskiy, qemu-devel, Marcel Apfelbaum, Michael S . Tsirkin, Jason Wang, Peter Maydell [-- Attachment #1: Type: text/plain, Size: 1933 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference 2017-03-28 0:06 ` David Gibson @ 2017-03-28 2:33 ` Michael S. Tsirkin 2017-03-28 2:47 ` David Gibson 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2017-03-28 2:33 UTC (permalink / raw) To: David Gibson Cc: Paolo Bonzini, Alexey Kardashevskiy, qemu-devel, Marcel Apfelbaum, Jason Wang, Peter Maydell 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference 2017-03-28 2:33 ` Michael S. Tsirkin @ 2017-03-28 2:47 ` David Gibson 0 siblings, 0 replies; 6+ messages in thread From: David Gibson @ 2017-03-28 2:47 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Alexey Kardashevskiy, qemu-devel, Marcel Apfelbaum, Jason Wang, Peter Maydell [-- Attachment #1: Type: text/plain, Size: 2345 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference 2017-03-27 4:40 [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference Alexey Kardashevskiy 2017-03-27 15:28 ` Paolo Bonzini @ 2017-03-29 2:40 ` Jason Wang 1 sibling, 0 replies; 6+ messages in thread From: Jason Wang @ 2017-03-29 2:40 UTC (permalink / raw) To: Alexey Kardashevskiy, qemu-devel Cc: David Gibson, Marcel Apfelbaum, Michael S . Tsirkin, Paolo Bonzini, Peter Maydell 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! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-29 2:41 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-27 4:40 [Qemu-devel] [PATCH qemu] pci: Add missing drop of bus master AS reference Alexey Kardashevskiy 2017-03-27 15:28 ` Paolo Bonzini 2017-03-28 0:06 ` David Gibson 2017-03-28 2:33 ` Michael S. Tsirkin 2017-03-28 2:47 ` David Gibson 2017-03-29 2:40 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.