From patchwork Fri Aug 24 08:36:40 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 179789 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id BACDC2C01E5 for ; Fri, 24 Aug 2012 18:35:44 +1000 (EST) Received: from localhost ([::1]:56838 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T4pMk-0005TF-R2 for incoming@patchwork.ozlabs.org; Fri, 24 Aug 2012 04:35:42 -0400 Received: from eggs.gnu.org ([208.118.235.92]:37165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T4pMa-0005So-UX for qemu-devel@nongnu.org; Fri, 24 Aug 2012 04:35:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T4pMY-0005jG-QI for qemu-devel@nongnu.org; Fri, 24 Aug 2012 04:35:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:18774) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T4pMY-0005iw-IF for qemu-devel@nongnu.org; Fri, 24 Aug 2012 04:35:30 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q7O8ZSgc010311 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 24 Aug 2012 04:35:29 -0400 Received: from redhat.com (vpn1-4-36.ams2.redhat.com [10.36.4.36]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q7O8ZQ3k001538; Fri, 24 Aug 2012 04:35:27 -0400 Date: Fri, 24 Aug 2012 11:36:40 +0300 From: "Michael S. Tsirkin" To: Jan Kiszka Message-ID: <20120824083640.GD7830@redhat.com> References: <5037182A.7080902@web.de> <20120824081136.GB7830@redhat.com> <50373825.6020007@web.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <50373825.6020007@web.de> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-MIME-Autoconverted: from 8bit to quoted-printable by mx1.redhat.com id q7O8ZSgc010311 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Cam Macdonell , "qemu-devel@nongnu.org Developers" Subject: Re: [Qemu-devel] MSI-X bug with ivshmem since msix_reset moved to PCI X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Fri, Aug 24, 2012 at 10:15:33AM +0200, Jan Kiszka wrote: > On 2012-08-24 10:11, Michael S. Tsirkin wrote: > > On Fri, Aug 24, 2012 at 07:59:06AM +0200, Jan Kiszka wrote: > >> On 2012-08-24 01:13, Cam Macdonell wrote: > >>> Hi Jan, > >>> > >>> I've bisected a bug in which MSI interrupts are not being delivered to > >>> the following patch, where msix_reset was moved in tot he PCI core. > >>> > >>> commit cbd2d4342b3d42ab33baa99f5b7a23491b5692f2 > >>> Author: Jan Kiszka > >>> Date: Tue May 15 20:09:56 2012 -0300 > >>> > >>> msi: Invoke msi/msix_reset from PCI core > >>> > >>> There is no point in pushing this burden to the devices, they tend to > >>> forget to call them (like intel-hda, ahci, xhci did). Instead, reset > >>> functions are now called from pci_device_reset. They do nothing if > >>> MSI/MSI-X is not in use. > >>> > >>> I've been debugging and it seems that when msix_notify() is triggered > >>> the second test in the "if" fails > >>> > >>> /* Send an MSI-X message */ > >>> void msix_notify(PCIDevice *dev, unsigned vector) > >>> { > >>> MSIMessage msg; > >>> > >>> if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) > >>> return; > >>> > >>> … > >>> } > >>> > >>> here is some MSI-X debugging statements > >>> > >>> msix_init > >>> IVSHMEM: msix initialized (1 vectors) > >>> IVSHMEM: using vector 0 > >>> IVSHMEM: ivshmem_reset > >>> IVSHMEM: using vector 0 > >>> msix_reset > >>> msix_free_irq_entries 0x7fd52d1cea20 > >>> > >>> msix_free_irq_entries() sets dev->msix_entries_nr to 0, so I think it > >>> may be the cause. > >> > >> I suppose you mean it sets the msix_entry_used array to 0. > >> > >>> > >>> Shouldn't ivshmem's reset (which reenables the vectors) be triggered > >>> by the msix_reset? > >> > >> Actually, the whole msix vector usage tracking is useless today, this > >> just shows its downsides (in the absence of benefits). Megasas is > >> affected by this problem as well, virtio not as it calls msix_vector_use > >> during the configuration process the guest driver triggers. > >> > >> Two options: > >> - I can send my removal patch for msix_vector_use/unuse that I was > >> only planning for 1.3 so far, and we kill this pitfall earlier. > >> - We re-add msix_vector_use calls to the affected device models for > >> 1.2 and drop them later again for 1.3 when removing usage tracking. > >> [The third option to keep the usage tracking is a non-option for me. ;)] > >> > >> Michael? > > > > Second option seems more prudent to me. Can you send a patch pls? > > In fact, it's not as easy. ivshmem already tries to restore the usage > flag but fails due to reset handler ordering. I do not see ATM where > there is some "enable device" during re-initialization, at least for > ivshmem. Haven't checked megasas yet. > > I tend to think removing is simpler and less risky. Please have a look > at the patch I sent earlier. > > Jan > > Actually virtio is the only one that changes vector use so the only one that needs to reset it. I am thinking we should find a way to move vector use out from core to virtio-pci. But for now something like the below should do the trick. Untested unfortunately, will test virtio Sunday but would appreciate review and testing reports esp with ivshmem etc. --> msix: avoid need to use/unuse vectors for most devices The facility to use/unuse vectors is helpful for virtio but little else since everyone just seems to use vectors in their init function. Avoid clearing msix vector use info on reset and load. For virtio, clear it explicitly. Signed-off-by: Michael S. Tsirkin diff --git a/hw/msix.c b/hw/msix.c index 800fc32..d040cc2 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -340,6 +340,15 @@ static void msix_free_irq_entries(PCIDevice *dev) } } +static void msix_clear_all_vectors(PCIDevice *dev) +{ + int vector; + + for (vector = 0; vector < dev->msix_entries_nr; ++vector) { + msix_clr_pending(dev, vector); + } +} + /* Clean up resources for the device. */ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) { @@ -394,7 +403,7 @@ void msix_load(PCIDevice *dev, QEMUFile *f) return; } - msix_free_irq_entries(dev); + msix_clear_all_vectors(dev); qemu_get_buffer(f, dev->msix_table, n * PCI_MSIX_ENTRY_SIZE); qemu_get_buffer(f, dev->msix_pba, (n + 7) / 8); msix_update_function_masked(dev); @@ -440,7 +449,7 @@ void msix_reset(PCIDevice *dev) if (!msix_present(dev)) { return; } - msix_free_irq_entries(dev); + msix_clear_all_vectors(dev); dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &= ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET]; memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE); diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 125eded..ca0b204 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -131,6 +131,7 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f) if (ret) { return ret; } + msix_unuse_all_vectors(&proxy->pci_dev); msix_load(&proxy->pci_dev, f); if (msix_present(&proxy->pci_dev)) { qemu_get_be16s(f, &proxy->vdev->config_vector); @@ -246,6 +247,7 @@ void virtio_pci_reset(DeviceState *d) VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); virtio_pci_stop_ioeventfd(proxy); virtio_reset(proxy->vdev); + msix_unuse_all_vectors(&proxy->pci_dev); proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG; }