From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44345) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLs1M-0004vP-7G for qemu-devel@nongnu.org; Tue, 17 Sep 2013 05:56:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VLs1D-00084s-TV for qemu-devel@nongnu.org; Tue, 17 Sep 2013 05:56:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10328) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLs1D-00084l-I1 for qemu-devel@nongnu.org; Tue, 17 Sep 2013 05:56:27 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8H9uQIQ017782 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 17 Sep 2013 05:56:26 -0400 Message-ID: <52382755.6090509@redhat.com> Date: Tue, 17 Sep 2013 11:56:37 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1378211609-16121-1-git-send-email-pbonzini@redhat.com> <1378211609-16121-11-git-send-email-pbonzini@redhat.com> <20130917092137.GC18186@redhat.com> In-Reply-To: <20130917092137.GC18186@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 10/38] msix: split msix_free from msix_uninit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org 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 >> --- >> 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