From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40005) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tlj6B-0000Ud-Fw for qemu-devel@nongnu.org; Thu, 20 Dec 2012 11:36:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tlj64-0006Nb-BJ for qemu-devel@nongnu.org; Thu, 20 Dec 2012 11:35:55 -0500 Date: Thu, 20 Dec 2012 18:38:58 +0200 From: "Michael S. Tsirkin" Message-ID: <20121220163858.GB316@redhat.com> References: <20121220160107.3975.53264.stgit@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20121220160107.3975.53264.stgit@bling.home> Subject: Re: [Qemu-devel] [PATCH] pci-assign: Enable MSIX on device to match guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: jan.kiszka@siemens.com, qemu-devel@nongnu.org, qemu-stable@nongnu.org On Thu, Dec 20, 2012 at 09:05:50AM -0700, Alex Williamson wrote: > When a guest enables MSIX on a device we evaluate the MSIX vector > table, typically find no unmasked vectors and don't switch the device > to MSIX mode. This generally works fine and the device will be > switched once the guest enables and therefore unmasks a vector. > Unfortunately some drivers enable MSIX, then use interfaces to send > commands between VF & PF or PF & firmware that act based on the host > state of the device. These therefore break when MSIX is managed > lazily. This change re-enables the previous test used to enable MSIX > (see qemu-kvm a6b402c9), which basically guesses whether a vector > will be used based on the data field of the vector table. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Alex Williamson Same question: can't we enable and mask MSIX through config sysfs? In this case it can be done in userspace ... > --- > hw/kvm/pci-assign.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > I think we might be able to do a little better than this, but I think > this is the right fix for stable and we can build on it to perhaps only > enable a single vector. > > diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c > index e80dad0..12a219b 100644 > --- a/hw/kvm/pci-assign.c > +++ b/hw/kvm/pci-assign.c > @@ -1025,6 +1025,19 @@ static bool assigned_dev_msix_masked(MSIXTableEntry *entry) > return (entry->ctrl & cpu_to_le32(0x1)) != 0; > } > > +/* > + * When MSI-X is first enabled the vector table typically has all the > + * vectors masked, so we can't use that as the obvious test to figure out > + * how many vectors to initially enable. Instead we look at the data field > + * because this is what worked for pci-assign for a long time. This makes > + * sure the physical MSI-X state tracks the guest's view, which is important > + * for some VF/PF and PF/fw communication channels. > + */ > +static bool assigned_dev_msix_skipped(MSIXTableEntry *entry) > +{ > + return !entry->data; > +} > + > static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev) > { > AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev); > @@ -1035,7 +1048,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev) > > /* Get the usable entry number for allocating */ > for (i = 0; i < adev->msix_max; i++, entry++) { > - if (assigned_dev_msix_masked(entry)) { > + if (assigned_dev_msix_skipped(entry)) { > continue; > } > entries_nr++; > @@ -1064,7 +1077,7 @@ static int assigned_dev_update_msix_mmio(PCIDevice *pci_dev) > for (i = 0; i < adev->msix_max; i++, entry++) { > adev->msi_virq[i] = -1; > > - if (assigned_dev_msix_masked(entry)) { > + if (assigned_dev_msix_skipped(entry)) { > continue; > } >