Patchwork [11/39] msix: split msix_free from msix_uninit

login
register
mail settings
Submitter Paolo Bonzini
Date June 4, 2013, 6:52 p.m.
Message ID <1370371954-8479-12-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/248827/
State New
Headers show

Comments

Paolo Bonzini - June 4, 2013, 6:52 p.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/vfio.c         |  1 +
 hw/net/vmxnet3.c       |  3 +++
 hw/pci/msix.c          | 26 ++++++++++++++++++++------
 hw/virtio/virtio-pci.c |  1 +
 include/hw/pci/msix.h  |  1 +
 5 files changed, 26 insertions(+), 6 deletions(-)
Michael S. Tsirkin - June 4, 2013, 10:03 p.m.
On Tue, Jun 04, 2013 at 08:52:06PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Please add a commit log with a bit more detail and
documenting the motivation for change.

> ---
>  hw/misc/vfio.c         |  1 +
>  hw/net/vmxnet3.c       |  3 +++
>  hw/pci/msix.c          | 26 ++++++++++++++++++++------
>  hw/virtio/virtio-pci.c |  1 +
>  include/hw/pci/msix.h  |  1 +
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 3c0dc9f..2bcef7a 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -2167,6 +2167,7 @@ static void vfio_teardown_msi(VFIODevice *vdev)
>      if (vdev->msix) {
>          msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem,
>                      &vdev->bars[vdev->msix->pba_bar].mem);
> +        msix_free(&vdev->pdev);
>      }
>  }
>  
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5f483e7..9de2586 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1979,6 +1979,7 @@ vmxnet3_init_msix(VMXNET3State *s)
>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>              VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
>              msix_uninit(d, &s->msix_bar, &s->msix_bar);
> +            msix_free(d);
>              s->msix_used = false;
>          } else {
>              s->msix_used = true;
> @@ -1996,6 +1997,8 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>          msix_vector_unuse(d, VMXNET3_MAX_INTRS);
>          msix_uninit(d, &s->msix_bar, &s->msix_bar);
>      }
> +
> +    msix_free(d);
>  }
>  
>  #define VMXNET3_MSI_NUM_VECTORS   (1)
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 6da75ec..be96b06 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -274,6 +274,10 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>                                               MSIX_MASKALL_MASK;
>  
> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> +        msix_free(dev);
> +    }
> +
>      dev->msix_table = g_malloc0(table_size);
>      dev->msix_pba = g_malloc0(pba_size);
>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);

Wow msix_init calls msix_free, and not on error path?
What's going on here?

> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>      msix_free_irq_entries(dev);
>      dev->msix_entries_nr = 0;
>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> +}
> +
> +void msix_free(PCIDevice *dev)
> +{
> +    if (dev->msix_pba) {
> +        memory_region_destroy(&dev->msix_pba_mmio);
> +        g_free(dev->msix_pba);
> +    }
> +    dev->msix_pba = NULL;
> +
> +    if (dev->msix_table) {
> +        memory_region_destroy(&dev->msix_table_mmio);
> +        g_free(dev->msix_table);
> +    }
>      dev->msix_table = NULL;
> +
>      g_free(dev->msix_entry_used);
>      dev->msix_entry_used = NULL;
> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>  }
>  
>  void msix_uninit_exclusive_bar(PCIDevice *dev)

As long as we had init and uninit, it was mostly
self-documenting.
Now, there are two cleanup functions, so please add documentation.


> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 444b71a..cbfec6b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -998,6 +998,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
>      virtio_pci_stop_ioeventfd(proxy);
>      memory_region_destroy(&proxy->bar);
>      msix_uninit_exclusive_bar(pci_dev);
> +    msix_free(pci_dev);
>  }
>  
>  static void virtio_pci_reset(DeviceState *qdev)
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 954d82b..34e3ba2 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -18,6 +18,7 @@ void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
>                   MemoryRegion *pba_bar);
>  void msix_uninit_exclusive_bar(PCIDevice *dev);
> +void msix_free(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>  
> -- 
> 1.8.1.4
>
Paolo Bonzini - June 4, 2013, 10:40 p.m.
Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>> > +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>> > +        msix_free(dev);
>> > +    }
>> > +
>> >      dev->msix_table = g_malloc0(table_size);
>> >      dev->msix_pba = g_malloc0(pba_size);
>> >      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> Wow msix_init calls msix_free, and not on error path?
> What's going on here?

I wasn't too sure that you could get here only with NULL
msix_table/pba/entry_used and wanted to protect against leaks.  I'll
change it to an assertion.

>> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>> >      msix_free_irq_entries(dev);
>> >      dev->msix_entries_nr = 0;
>> >      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
>> > -    memory_region_destroy(&dev->msix_pba_mmio);
>> > -    g_free(dev->msix_pba);
>> > -    dev->msix_pba = NULL;
>> >      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
>> > -    memory_region_destroy(&dev->msix_table_mmio);
>> > -    g_free(dev->msix_table);
>> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>> > +}
>> > +
>> > +void msix_free(PCIDevice *dev)
>> > +{
>> > +    if (dev->msix_pba) {
>> > +        memory_region_destroy(&dev->msix_pba_mmio);
>> > +        g_free(dev->msix_pba);
>> > +    }
>> > +    dev->msix_pba = NULL;
>> > +
>> > +    if (dev->msix_table) {
>> > +        memory_region_destroy(&dev->msix_table_mmio);
>> > +        g_free(dev->msix_table);
>> > +    }
>> >      dev->msix_table = NULL;
>> > +
>> >      g_free(dev->msix_entry_used);
>> >      dev->msix_entry_used = NULL;
>> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>> >  }
>> >  
>> >  void msix_uninit_exclusive_bar(PCIDevice *dev)
> As long as we had init and uninit, it was mostly
> self-documenting.
> Now, there are two cleanup functions, so please add documentation.

Yes, will do.

Paolo
Michael S. Tsirkin - June 5, 2013, 4:53 a.m.
On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
> >> > +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> >> > +        msix_free(dev);
> >> > +    }
> >> > +
> >> >      dev->msix_table = g_malloc0(table_size);
> >> >      dev->msix_pba = g_malloc0(pba_size);
> >> >      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > Wow msix_init calls msix_free, and not on error path?
> > What's going on here?
> 
> I wasn't too sure that you could get here only with NULL
> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
> change it to an assertion.

I don't think we should require users allocate all memory with g_malloc0.
So no assertion either.
If there's a leak there was always a leak, let's focus on the
API change in this series, OK?

> >> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >> >      msix_free_irq_entries(dev);
> >> >      dev->msix_entries_nr = 0;
> >> >      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> >> > -    memory_region_destroy(&dev->msix_pba_mmio);
> >> > -    g_free(dev->msix_pba);
> >> > -    dev->msix_pba = NULL;
> >> >      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> >> > -    memory_region_destroy(&dev->msix_table_mmio);
> >> > -    g_free(dev->msix_table);
> >> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >> > +}
> >> > +
> >> > +void msix_free(PCIDevice *dev)
> >> > +{
> >> > +    if (dev->msix_pba) {
> >> > +        memory_region_destroy(&dev->msix_pba_mmio);
> >> > +        g_free(dev->msix_pba);
> >> > +    }
> >> > +    dev->msix_pba = NULL;
> >> > +
> >> > +    if (dev->msix_table) {
> >> > +        memory_region_destroy(&dev->msix_table_mmio);
> >> > +        g_free(dev->msix_table);
> >> > +    }
> >> >      dev->msix_table = NULL;
> >> > +
> >> >      g_free(dev->msix_entry_used);
> >> >      dev->msix_entry_used = NULL;
> >> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >> >  }
> >> >  
> >> >  void msix_uninit_exclusive_bar(PCIDevice *dev)
> > As long as we had init and uninit, it was mostly
> > self-documenting.
> > Now, there are two cleanup functions, so please add documentation.
> 
> Yes, will do.
> 
> Paolo
Paolo Bonzini - June 5, 2013, 7:48 a.m.
Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>>>>> +        msix_free(dev);
>>>>> +    }
>>>>> +
>>>>>      dev->msix_table = g_malloc0(table_size);
>>>>>      dev->msix_pba = g_malloc0(pba_size);
>>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
>>> Wow msix_init calls msix_free, and not on error path?
>>> What's going on here?
>>
>> I wasn't too sure that you could get here only with NULL
>> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
>> change it to an assertion.
> 
> I don't think we should require users allocate all memory with g_malloc0.
> So no assertion either.

Assertion that is is NULL, followed by g_malloc0?

> If there's a leak there was always a leak

No, there wasn't because msix_uninit would have freed the memory.  That is,

    msix_init
    msix_uninit
    msix_init
    msix_uninit

had no leak.  Instead, now msix_free is going to be called just once,
right before freeing the object itself:

    msix_init
    msix_uninit
    msix_init     ***
    msix_uninit
    msix_free

and will have a leak at ***.  I don't think this can happen, unrealize
should never be followed by another realize right now, but perhaps in
the future it will be if we implement something like "device_poweroff"
and "device_poweron".

Paolo

, let's focus on the
> API change in this series, OK?
> 
>>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>>>>>      msix_free_irq_entries(dev);
>>>>>      dev->msix_entries_nr = 0;
>>>>>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
>>>>> -    memory_region_destroy(&dev->msix_pba_mmio);
>>>>> -    g_free(dev->msix_pba);
>>>>> -    dev->msix_pba = NULL;
>>>>>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
>>>>> -    memory_region_destroy(&dev->msix_table_mmio);
>>>>> -    g_free(dev->msix_table);
>>>>> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>>>>> +}
>>>>> +
>>>>> +void msix_free(PCIDevice *dev)
>>>>> +{
>>>>> +    if (dev->msix_pba) {
>>>>> +        memory_region_destroy(&dev->msix_pba_mmio);
>>>>> +        g_free(dev->msix_pba);
>>>>> +    }
>>>>> +    dev->msix_pba = NULL;
>>>>> +
>>>>> +    if (dev->msix_table) {
>>>>> +        memory_region_destroy(&dev->msix_table_mmio);
>>>>> +        g_free(dev->msix_table);
>>>>> +    }
>>>>>      dev->msix_table = NULL;
>>>>> +
>>>>>      g_free(dev->msix_entry_used);
>>>>>      dev->msix_entry_used = NULL;
>>>>> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>>>>>  }
>>>>>  
>>>>>  void msix_uninit_exclusive_bar(PCIDevice *dev)
>>> As long as we had init and uninit, it was mostly
>>> self-documenting.
>>> Now, there are two cleanup functions, so please add documentation.
>>
>> Yes, will do.
>>
>> Paolo
> 
>
Michael S. Tsirkin - June 5, 2013, 10:32 a.m.
On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
> > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
> >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
> >>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> >>>>> +        msix_free(dev);
> >>>>> +    }
> >>>>> +
> >>>>>      dev->msix_table = g_malloc0(table_size);
> >>>>>      dev->msix_pba = g_malloc0(pba_size);
> >>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> >>> Wow msix_init calls msix_free, and not on error path?
> >>> What's going on here?
> >>
> >> I wasn't too sure that you could get here only with NULL
> >> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
> >> change it to an assertion.
> > 
> > I don't think we should require users allocate all memory with g_malloc0.
> > So no assertion either.
> 
> Assertion that is is NULL, followed by g_malloc0?

No because who sets it to NULL the first time?
msix_init just started.

> > If there's a leak there was always a leak
> 
> No, there wasn't because msix_uninit would have freed the memory.  That is,
> 
>     msix_init
>     msix_uninit
>     msix_init
>     msix_uninit
> 
> had no leak.  Instead, now msix_free is going to be called just once,
> right before freeing the object itself:
> 
>     msix_init
>     msix_uninit
>     msix_init     ***
>     msix_uninit
>     msix_free
> 
> and will have a leak at ***.

Yes. And this looks completely sane from outside,
so this is a bad API.
The way to fix it is not with asserts in code, we need a good API:
alloc/free init/uninit ...

The problem apparently starts in generic code, let's fix it there?

>  I don't think this can happen, unrealize
> should never be followed by another realize right now,

This is not an msix specific problem, I don't think msix should
debug generic core - this will just lead to proliferation of asserts.

This really should be documented prominently in generic code.

Also how about some asserts in generic code making sure ordering
is sane?


> but perhaps in
> the future it will be if we implement something like "device_poweroff"
> and "device_poweron".
> 
> Paolo

> , let's focus on the
> > API change in this series, OK?
> > 
> >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >>>>>      msix_free_irq_entries(dev);
> >>>>>      dev->msix_entries_nr = 0;
> >>>>>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> -    g_free(dev->msix_pba);
> >>>>> -    dev->msix_pba = NULL;
> >>>>>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_table_mmio);
> >>>>> -    g_free(dev->msix_table);
> >>>>> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>> +}
> >>>>> +
> >>>>> +void msix_free(PCIDevice *dev)
> >>>>> +{
> >>>>> +    if (dev->msix_pba) {
> >>>>> +        memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> +        g_free(dev->msix_pba);
> >>>>> +    }
> >>>>> +    dev->msix_pba = NULL;
> >>>>> +
> >>>>> +    if (dev->msix_table) {
> >>>>> +        memory_region_destroy(&dev->msix_table_mmio);
> >>>>> +        g_free(dev->msix_table);
> >>>>> +    }
> >>>>>      dev->msix_table = NULL;
> >>>>> +
> >>>>>      g_free(dev->msix_entry_used);
> >>>>>      dev->msix_entry_used = NULL;
> >>>>> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>>  }
> >>>>>  
> >>>>>  void msix_uninit_exclusive_bar(PCIDevice *dev)
> >>> As long as we had init and uninit, it was mostly
> >>> self-documenting.
> >>> Now, there are two cleanup functions, so please add documentation.
> >>
> >> Yes, will do.
> >>
> >> Paolo
> > 
> >
Paolo Bonzini - June 7, 2013, 1:01 a.m.
Il 05/06/2013 06:32, Michael S. Tsirkin ha scritto:
> On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
>>> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
>>>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>>>>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>>>>>>> +        msix_free(dev);
>>>>>>> +    }
>>>>>>> +
>>>>>>>      dev->msix_table = g_malloc0(table_size);
>>>>>>>      dev->msix_pba = g_malloc0(pba_size);
>>>>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
>>>>> Wow msix_init calls msix_free, and not on error path?
>>>>> What's going on here?
>>>>
>>>> I wasn't too sure that you could get here only with NULL
>>>> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
>>>> change it to an assertion.
>>>
>>> I don't think we should require users allocate all memory with g_malloc0.
>>> So no assertion either.
>>
>> Assertion that is is NULL, followed by g_malloc0?
> 
> No because who sets it to NULL the first time?
> msix_init just started.

When an object is created, it is all-zeroed.

>>> If there's a leak there was always a leak
>>
>> No, there wasn't because msix_uninit would have freed the memory.  That is,
>>
>>     msix_init
>>     msix_uninit
>>     msix_init
>>     msix_uninit
>>
>> had no leak.  Instead, now msix_free is going to be called just once,
>> right before freeing the object itself:
>>
>>     msix_init
>>     msix_uninit
>>     msix_init     ***
>>     msix_uninit
>>     msix_free
>>
>> and will have a leak at ***.
> 
> Yes. And this looks completely sane from outside,
> so this is a bad API.
> The way to fix it is not with asserts in code, we need a good API:
> alloc/free init/uninit ...

Can't, because table_size/pba_size is not available at init time (e.g.
for VFIO not until the host BARs are processed).  What about using
g_realloc + memset?

Paolo

Patch

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 3c0dc9f..2bcef7a 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2167,6 +2167,7 @@  static void vfio_teardown_msi(VFIODevice *vdev)
     if (vdev->msix) {
         msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem,
                     &vdev->bars[vdev->msix->pba_bar].mem);
+        msix_free(&vdev->pdev);
     }
 }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5f483e7..9de2586 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1979,6 +1979,7 @@  vmxnet3_init_msix(VMXNET3State *s)
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
+            msix_free(d);
             s->msix_used = false;
         } else {
             s->msix_used = true;
@@ -1996,6 +1997,8 @@  vmxnet3_cleanup_msix(VMXNET3State *s)
         msix_vector_unuse(d, VMXNET3_MAX_INTRS);
         msix_uninit(d, &s->msix_bar, &s->msix_bar);
     }
+
+    msix_free(d);
 }
 
 #define VMXNET3_MSI_NUM_VECTORS   (1)
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 6da75ec..be96b06 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -274,6 +274,10 @@  int msix_init(struct PCIDevice *dev, unsigned short nentries,
     dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
                                              MSIX_MASKALL_MASK;
 
+    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
+        msix_free(dev);
+    }
+
     dev->msix_table = g_malloc0(table_size);
     dev->msix_pba = g_malloc0(pba_size);
     dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
@@ -359,16 +363,26 @@  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
     msix_free_irq_entries(dev);
     dev->msix_entries_nr = 0;
     memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
-    memory_region_destroy(&dev->msix_pba_mmio);
-    g_free(dev->msix_pba);
-    dev->msix_pba = NULL;
     memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
-    memory_region_destroy(&dev->msix_table_mmio);
-    g_free(dev->msix_table);
+    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
+}
+
+void msix_free(PCIDevice *dev)
+{
+    if (dev->msix_pba) {
+        memory_region_destroy(&dev->msix_pba_mmio);
+        g_free(dev->msix_pba);
+    }
+    dev->msix_pba = NULL;
+
+    if (dev->msix_table) {
+        memory_region_destroy(&dev->msix_table_mmio);
+        g_free(dev->msix_table);
+    }
     dev->msix_table = NULL;
+
     g_free(dev->msix_entry_used);
     dev->msix_entry_used = NULL;
-    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
 }
 
 void msix_uninit_exclusive_bar(PCIDevice *dev)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 444b71a..cbfec6b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -998,6 +998,7 @@  static void virtio_pci_exit(PCIDevice *pci_dev)
     virtio_pci_stop_ioeventfd(proxy);
     memory_region_destroy(&proxy->bar);
     msix_uninit_exclusive_bar(pci_dev);
+    msix_free(pci_dev);
 }
 
 static void virtio_pci_reset(DeviceState *qdev)
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 954d82b..34e3ba2 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -18,6 +18,7 @@  void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
                  MemoryRegion *pba_bar);
 void msix_uninit_exclusive_bar(PCIDevice *dev);
+void msix_free(PCIDevice *dev);
 
 unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);