From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Subject: Re: [PATCH 3/3] drivers/vfio/pci: Fix MSIx message lost Date: Mon, 03 Mar 2014 15:43:27 +1100 Message-ID: <5314086F.10108@ozlabs.ru> References: <1393817042-13758-1-git-send-email-shangw@linux.vnet.ibm.com> <1393817042-13758-3-git-send-email-shangw@linux.vnet.ibm.com> <1393818710.10727.22.camel@pasglop> Mime-Version: 1.0 Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, alex.williamson@redhat.com To: Benjamin Herrenschmidt , Gavin Shan Return-path: Received: from mail-pa0-f48.google.com ([209.85.220.48]:38901 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752469AbaCCEnd (ORCPT ); Sun, 2 Mar 2014 23:43:33 -0500 Received: by mail-pa0-f48.google.com with SMTP id kx10so3274359pab.21 for ; Sun, 02 Mar 2014 20:43:32 -0800 (PST) In-Reply-To: <1393818710.10727.22.camel@pasglop> Sender: kvm-owner@vger.kernel.org List-ID: On 03/03/2014 02:51 PM, Benjamin Herrenschmidt wrote: > On Mon, 2014-03-03 at 11:24 +0800, Gavin Shan wrote: >> The problem is specific to the case of BIST issued applied to IPR >> adapter on the guest side. After BIST reset, we lose everything >> in MSIx table and we never have chance update MSIx messages for >> those enabled interrupts to MSIx table. >> >> The patch fixes it by writing MSIx message to MSIx table before >> reenabling them. > > That needs a better explanation... the guest does try to restore the > MSI-X (in a way similar to resuming from suspend) by writing the > message value back into the table. > > My understanding is that we trap that and turn that into a call to > vfio_msi_set_vector_signal, is that correct ? Yep. Gavin sent me a backtrace of what happens when the guest tries writing to a BAR: qemu/hw/pci/msix.c::msix_table_mmio_write msix_handle_mask_update msix_fire_vector_notifier qemu/hw/misc/vfio.c::vfio_msix_vector_use vfio_msix_vector_do_use IOCTL-to-VFIO: VFIO_DEVICE_SET_IRQS host/drivers/vfio/pci/vfio_pci.c::vfio_pci_ioctl host/drivers/vfio/pci/vfio_pci_intrs.c::vfio_pci_set_irqs_ioctl vfio_pci_set_msi_trigger vfio_msi_set_block vfio_msi_set_vector_signal While it works for our particular problem and seems correct, it has one flaw - hw/pci/msix.c will not generate this backtrace if masking bit does not change which can happen in general: === static void msix_handle_mask_update(PCIDevice *dev, int vector, bool was_masked) { bool is_masked = msix_is_masked(dev, vector); if (is_masked == was_masked) { return; } === Or if masking bit is the same, nothing bad is expected?... > In that case, it does indeed make sense to have that function rewrite > the cached message. > > (Just trying to understand how this patch fixes the problem we saw) > > I suppose other drivers would have similar issues if they have some > kind of internal "reset" path and try to save and restore the MSI-X > table. > Cheers, > Ben. > >> Reported-by: Wen Xiong >> Signed-off-by: Gavin Shan >> --- >> drivers/vfio/pci/vfio_pci_intrs.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c >> index 2103576..279ebd0 100644 >> --- a/drivers/vfio/pci/vfio_pci_intrs.c >> +++ b/drivers/vfio/pci/vfio_pci_intrs.c >> @@ -17,6 +17,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -517,6 +518,7 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, >> struct pci_dev *pdev = vdev->pdev; >> int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; >> char *name = msix ? "vfio-msix" : "vfio-msi"; >> + struct msi_msg msg; >> struct eventfd_ctx *trigger; >> int ret; >> >> @@ -544,6 +546,23 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, >> return PTR_ERR(trigger); >> } >> >> + /* We possiblly lose the MSI/MSIx message in some cases. >> + * For example, BIST reset on IPR adapter. The MSIx table >> + * is cleaned out. However, we never get chance to put >> + * MSIx messages to MSIx table because all MSIx stuff is >> + * being cached in QEMU. Here, we had the trick to put the >> + * MSI/MSIx message back. >> + * >> + * Basically, we needn't worry about MSI messages. However, >> + * it's not harmful and there might be cases of PCI config data >> + * lost because of cached PCI config data in QEMU again. >> + * >> + * Note that we should flash the message prior to enabling >> + * the corresponding interrupt by request_irq(). >> + */ >> + get_cached_msi_msg(irq, &msg); >> + write_msi_msg(irq, &msg); >> + >> ret = request_irq(irq, vfio_msihandler, 0, >> vdev->ctx[vector].name, trigger); >> if (ret) { > > -- Alexey