From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38954) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJuJQ-0001PC-87 for qemu-devel@nongnu.org; Thu, 04 Oct 2012 18:54:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJuJO-00016J-Ue for qemu-devel@nongnu.org; Thu, 04 Oct 2012 18:54:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3717) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJuJO-000167-MC for qemu-devel@nongnu.org; Thu, 04 Oct 2012 18:54:34 -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 q94MsXRQ003063 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 4 Oct 2012 18:54:33 -0400 From: Alex Williamson Date: Thu, 04 Oct 2012 16:17:58 -0600 Message-ID: <20121004221757.3189.92167.stgit@bling.home> In-Reply-To: <20121004220545.3189.52569.stgit@bling.home> References: <20121004220545.3189.52569.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] [PATCH 04/11] vfio-pci: Rework MSIX setup/teardown List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: alex.williamson@redhat.com We try to do lazy initialization of MSIX since we don't actually need to setup anything until MSIX vectors start getting used. This leads to problems if MSIX is enabled, but never used (we can end up trying to re-enable INTx while it's still enabled). We also run into problems trying to expand our reset function to tear down interrupts as we can then get vector release notifications after we've released data structures. By making explicit initialization and teardown we can avoid both of these problems and behave more similar to bare metal. Signed-off-by: Alex Williamson --- hw/vfio_pci.c | 108 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 55 insertions(+), 53 deletions(-) diff --git a/hw/vfio_pci.c b/hw/vfio_pci.c index 7413f2d..89e00ba 100644 --- a/hw/vfio_pci.c +++ b/hw/vfio_pci.c @@ -72,8 +72,6 @@ static void vfio_disable_irqindex(VFIODevice *vdev, int index) }; ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, &irq_set); - - vdev->interrupt = VFIO_INT_NONE; } /* @@ -278,10 +276,6 @@ static int vfio_enable_vectors(VFIODevice *vdev, bool msix) g_free(irq_set); - if (!ret) { - vdev->interrupt = msix ? VFIO_INT_MSIX : VFIO_INT_MSI; - } - return ret; } @@ -296,15 +290,6 @@ static int vfio_msix_vector_use(PCIDevice *pdev, vdev->host.domain, vdev->host.bus, vdev->host.slot, vdev->host.function, nr); - if (vdev->interrupt != VFIO_INT_MSIX) { - vfio_disable_interrupts(vdev); - } - - if (!vdev->msi_vectors) { - vdev->msi_vectors = g_malloc0(vdev->msix->entries * - sizeof(VFIOMSIVector)); - } - vector = &vdev->msi_vectors[nr]; vector->vdev = vdev; vector->use = true; @@ -457,6 +442,23 @@ static void msi_set_qsize(PCIDevice *pdev, uint8_t size) pci_set_word(config + PCI_MSI_FLAGS, flags); } +static void vfio_enable_msix(VFIODevice *vdev) +{ + vfio_disable_interrupts(vdev); + + vdev->msi_vectors = g_malloc0(vdev->msix->entries * sizeof(VFIOMSIVector)); + + vdev->interrupt = VFIO_INT_MSIX; + + if (msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use, + vfio_msix_vector_release)) { + error_report("vfio: msix_set_vector_notifiers failed\n"); + } + + DPRINTF("%s(%04x:%02x:%02x.%x)\n", __func__, vdev->host.domain, + vdev->host.bus, vdev->host.slot, vdev->host.function); +} + static void vfio_enable_msi(VFIODevice *vdev) { int ret, i; @@ -529,17 +531,43 @@ retry: msi_set_qsize(&vdev->pdev, vdev->nr_vectors); + vdev->interrupt = VFIO_INT_MSI; + DPRINTF("%s(%04x:%02x:%02x.%x) Enabled %d MSI vectors\n", __func__, vdev->host.domain, vdev->host.bus, vdev->host.slot, vdev->host.function, vdev->nr_vectors); } -static void vfio_disable_msi_x(VFIODevice *vdev, bool msix) +static void vfio_disable_msi_common(VFIODevice *vdev) +{ + g_free(vdev->msi_vectors); + vdev->msi_vectors = NULL; + vdev->nr_vectors = 0; + vdev->interrupt = VFIO_INT_NONE; + + vfio_enable_intx(vdev); +} + +static void vfio_disable_msix(VFIODevice *vdev) +{ + msix_unset_vector_notifiers(&vdev->pdev); + + if (vdev->nr_vectors) { + vfio_disable_irqindex(vdev, VFIO_PCI_MSIX_IRQ_INDEX); + } + + vfio_disable_msi_common(vdev); + + DPRINTF("%s(%04x:%02x:%02x.%x, msi%s)\n", __func__, + vdev->host.domain, vdev->host.bus, vdev->host.slot, + vdev->host.function, msix ? "x" : ""); +} + +static void vfio_disable_msi(VFIODevice *vdev) { int i; - vfio_disable_irqindex(vdev, msix ? VFIO_PCI_MSIX_IRQ_INDEX : - VFIO_PCI_MSI_IRQ_INDEX); + vfio_disable_irqindex(vdev, VFIO_PCI_MSI_IRQ_INDEX); for (i = 0; i < vdev->nr_vectors; i++) { VFIOMSIVector *vector = &vdev->msi_vectors[i]; @@ -558,26 +586,15 @@ static void vfio_disable_msi_x(VFIODevice *vdev, bool msix) NULL, NULL, NULL); } - if (msix) { - msix_vector_unuse(&vdev->pdev, i); - } - event_notifier_cleanup(&vector->interrupt); } - g_free(vdev->msi_vectors); - vdev->msi_vectors = NULL; - vdev->nr_vectors = 0; - - if (!msix) { - msi_set_qsize(&vdev->pdev, 0); /* Actually still means 1 vector */ - } + vfio_disable_msi_common(vdev); - DPRINTF("%s(%04x:%02x:%02x.%x, msi%s)\n", __func__, - vdev->host.domain, vdev->host.bus, vdev->host.slot, - vdev->host.function, msix ? "x" : ""); + msi_set_qsize(&vdev->pdev, 0); /* Actually still means 1 vector */ - vfio_enable_intx(vdev); + DPRINTF("%s(%04x:%02x:%02x.%x)\n", __func__, vdev->host.domain, + vdev->host.bus, vdev->host.slot, vdev->host.function); } /* @@ -763,7 +780,7 @@ static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr, if (!was_enabled && is_enabled) { vfio_enable_msi(vdev); } else if (was_enabled && !is_enabled) { - vfio_disable_msi_x(vdev, false); + vfio_disable_msi(vdev); } } @@ -776,9 +793,9 @@ static void vfio_pci_write_config(PCIDevice *pdev, uint32_t addr, is_enabled = msix_enabled(pdev); if (!was_enabled && is_enabled) { - /* vfio_msix_vector_use handles this automatically */ + vfio_enable_msix(vdev); } else if (was_enabled && !is_enabled) { - vfio_disable_msi_x(vdev, true); + vfio_disable_msix(vdev); } } } @@ -973,10 +990,10 @@ static void vfio_disable_interrupts(VFIODevice *vdev) vfio_disable_intx(vdev); break; case VFIO_INT_MSI: - vfio_disable_msi_x(vdev, false); + vfio_disable_msi(vdev); break; case VFIO_INT_MSIX: - vfio_disable_msi_x(vdev, true); + vfio_disable_msix(vdev); break; } } @@ -1094,15 +1111,6 @@ static int vfio_setup_msix(VFIODevice *vdev, int pos) return ret; } - ret = msix_set_vector_notifiers(&vdev->pdev, vfio_msix_vector_use, - vfio_msix_vector_release); - if (ret) { - error_report("vfio: msix_set_vector_notifiers failed %d\n", ret); - msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem, - &vdev->bars[vdev->msix->pba_bar].mem); - return ret; - } - return 0; } @@ -1111,12 +1119,6 @@ static void vfio_teardown_msi(VFIODevice *vdev) msi_uninit(&vdev->pdev); if (vdev->msix) { - /* FIXME: Why can't unset just silently do nothing?? */ - if (vdev->pdev.msix_vector_use_notifier && - vdev->pdev.msix_vector_release_notifier) { - msix_unset_vector_notifiers(&vdev->pdev); - } - msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem, &vdev->bars[vdev->msix->pba_bar].mem); }