| Message ID | 1378211609-16121-11-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Sep 03, 2013 at 02:33:01PM +0200, Paolo Bonzini wrote: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/misc/vfio.c | 1 + > hw/net/vmxnet3.c | 3 +++ > hw/pci/msix.c | 22 ++++++++++++++++------ > hw/virtio/virtio-pci.c | 1 + > include/hw/pci/msix.h | 1 + > 5 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c > index a1c08fb..1311d0e 100644 > --- a/hw/misc/vfio.c > +++ b/hw/misc/vfio.c > @@ -2144,6 +2144,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 49c2466..2a79e52 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 3430770..0618e3b 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -359,16 +359,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; > } > I dislike the if (...) here. This only works as long as all data is pointers. So I'd prefer QEMU_PCI_CAP_MSIX to be cleared in msix_free. This way msix_free can just check QEMU_PCI_CAP_MSIX and return if not there. > void msix_uninit_exclusive_bar(PCIDevice *dev) > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index f2c489b..ff6bed5 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -999,6 +999,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.3.1 >
Il 17/09/2013 11:21, Michael S. Tsirkin ha scritto: > On Tue, Sep 03, 2013 at 02:33:01PM +0200, Paolo Bonzini wrote: >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/misc/vfio.c | 1 + >> hw/net/vmxnet3.c | 3 +++ >> hw/pci/msix.c | 22 ++++++++++++++++------ >> hw/virtio/virtio-pci.c | 1 + >> include/hw/pci/msix.h | 1 + >> 5 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c >> index a1c08fb..1311d0e 100644 >> --- a/hw/misc/vfio.c >> +++ b/hw/misc/vfio.c >> @@ -2144,6 +2144,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 49c2466..2a79e52 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 3430770..0618e3b 100644 >> --- a/hw/pci/msix.c >> +++ b/hw/pci/msix.c >> @@ -359,16 +359,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; >> } >> > > I dislike the if (...) here. This only works as long > as all data is pointers. > So I'd prefer QEMU_PCI_CAP_MSIX to be cleared in msix_free. > This way msix_free can just check QEMU_PCI_CAP_MSIX > and return if not there. Makes sense. Paolo
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c index a1c08fb..1311d0e 100644 --- a/hw/misc/vfio.c +++ b/hw/misc/vfio.c @@ -2144,6 +2144,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 49c2466..2a79e52 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 3430770..0618e3b 100644 --- a/hw/pci/msix.c +++ b/hw/pci/msix.c @@ -359,16 +359,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 f2c489b..ff6bed5 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -999,6 +999,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);
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/misc/vfio.c | 1 + hw/net/vmxnet3.c | 3 +++ hw/pci/msix.c | 22 ++++++++++++++++------ hw/virtio/virtio-pci.c | 1 + include/hw/pci/msix.h | 1 + 5 files changed, 22 insertions(+), 6 deletions(-)