Patchwork [10/38] msix: split msix_free from msix_uninit

login
register
mail settings
Submitter Paolo Bonzini
Date Sept. 3, 2013, 12:33 p.m.
Message ID <1378211609-16121-11-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/272250/
State New
Headers show

Comments

Paolo Bonzini - Sept. 3, 2013, 12:33 p.m.
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(-)
Michael S. Tsirkin - Sept. 17, 2013, 9:21 a.m.
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
>
Paolo Bonzini - Sept. 17, 2013, 9:56 a.m.
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

Patch

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);