[Qemu-devel] [PATCH] virtio-pci: fix MSI memory region use after tree

[Qemu-devel] [PATCH] virtio-pci: fix MSI memory region use after tree

[Paolo Bonzini]

After memory region QOMification QEMU is stricter in detecting
wrong usage of the memory region API.  Here it detected a
memory_region_destroy done before the corresponding
memory_region_del_subregion; the memory_region_destroy is
done by msix_uninit_exclusive_bar, the memory_region_del_subregion
is done by the PCI core's pci_unregister_io_regions before
pc->exit is called.

The misuse caused an assertion when hot-unplugging virtio
devices.  Using the API correctly fixes the assertion.

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

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 3c42cda..ecb2097 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1003,11 +1003,9 @@ static void virtio_pci_device_plugged(DeviceState *d)
 
 static void virtio_pci_device_unplugged(DeviceState *d)
 {
-    PCIDevice *pci_dev = PCI_DEVICE(d);
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
 
     virtio_pci_stop_ioeventfd(proxy);
-    msix_uninit_exclusive_bar(pci_dev);
 }
 
 static int virtio_pci_init(PCIDevice *pci_dev)
@@ -1024,6 +1022,8 @@ static int virtio_pci_init(PCIDevice *pci_dev)
 static void virtio_pci_exit(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
+
+    msix_uninit_exclusive_bar(pci_dev);
     memory_region_destroy(&proxy->bar);
 }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH] virtio-pci: fix MSI memory region use after tree

[Michael S. Tsirkin]

On Fri, Jul 04, 2014 at 11:43:49AM +0200, Paolo Bonzini wrote:
> After memory region QOMification QEMU is stricter in detecting
> wrong usage of the memory region API.  Here it detected a
> memory_region_destroy done before the corresponding
> memory_region_del_subregion; the memory_region_destroy is
> done by msix_uninit_exclusive_bar, the memory_region_del_subregion
> is done by the PCI core's pci_unregister_io_regions before
> pc->exit is called.
> 
> The misuse caused an assertion when hot-unplugging virtio
> devices.  Using the API correctly fixes the assertion.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied, thanks!
I also added some historical context in the comments
(the API misuse was introduced in 06a1307379fcd6c551185ad87679cd7ed896b9ea)


> ---
>  hw/virtio/virtio-pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 3c42cda..ecb2097 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1003,11 +1003,9 @@ static void virtio_pci_device_plugged(DeviceState *d)
>  
>  static void virtio_pci_device_unplugged(DeviceState *d)
>  {
> -    PCIDevice *pci_dev = PCI_DEVICE(d);
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
>  
>      virtio_pci_stop_ioeventfd(proxy);
> -    msix_uninit_exclusive_bar(pci_dev);
>  }
>  
>  static int virtio_pci_init(PCIDevice *pci_dev)
> @@ -1024,6 +1022,8 @@ static int virtio_pci_init(PCIDevice *pci_dev)
>  static void virtio_pci_exit(PCIDevice *pci_dev)
>  {
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> +
> +    msix_uninit_exclusive_bar(pci_dev);
>      memory_region_destroy(&proxy->bar);
>  }
>  
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH] virtio-pci: fix MSI memory region use after tree

[Michael S. Tsirkin]

On Sun, Jul 06, 2014 at 08:39:53AM +0300, Michael S. Tsirkin wrote:
> On Fri, Jul 04, 2014 at 11:43:49AM +0200, Paolo Bonzini wrote:
> > After memory region QOMification QEMU is stricter in detecting
> > wrong usage of the memory region API.  Here it detected a
> > memory_region_destroy done before the corresponding
> > memory_region_del_subregion; the memory_region_destroy is
> > done by msix_uninit_exclusive_bar, the memory_region_del_subregion
> > is done by the PCI core's pci_unregister_io_regions before
> > pc->exit is called.
> > 
> > The misuse caused an assertion when hot-unplugging virtio
> > devices.  Using the API correctly fixes the assertion.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Applied, thanks!
> I also added some historical context in the comments
> (the API misuse was introduced in 06a1307379fcd6c551185ad87679cd7ed896b9ea)
> 

Fixed subject typo as well s/tree/free/

> > ---
> >  hw/virtio/virtio-pci.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 3c42cda..ecb2097 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1003,11 +1003,9 @@ static void virtio_pci_device_plugged(DeviceState *d)
> >  
> >  static void virtio_pci_device_unplugged(DeviceState *d)
> >  {
> > -    PCIDevice *pci_dev = PCI_DEVICE(d);
> >      VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
> >  
> >      virtio_pci_stop_ioeventfd(proxy);
> > -    msix_uninit_exclusive_bar(pci_dev);
> >  }
> >  
> >  static int virtio_pci_init(PCIDevice *pci_dev)
> > @@ -1024,6 +1022,8 @@ static int virtio_pci_init(PCIDevice *pci_dev)
> >  static void virtio_pci_exit(PCIDevice *pci_dev)
> >  {
> >      VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
> > +
> > +    msix_uninit_exclusive_bar(pci_dev);
> >      memory_region_destroy(&proxy->bar);
> >  }
> >  
> > -- 
> > 1.9.3