On Mon, 2014-03-03 at 14:51 +1100, 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 ?
>
> 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 <wenxiong@xxxxxxxxxx>
> > Signed-off-by: Gavin Shan <shangw@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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 <linux/interrupt.h>
> > #include <linux/eventfd.h>
> > #include <linux/pci.h>
> > +#include <linux/msi.h>
> > #include <linux/file.h>
> > #include <linux/poll.h>
> > #include <linux/vfio.h>
> > @@ -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) {
I understand from talking to Alexey that the BARs are reset by this
BIST, but what about the MSIX capability? If that gets reset to be
disabled, where does it get re-enabled? QEMU won't know about the BIST,
so probably assumes nothing changed when the guest writes the enable
bit. VFIO doesn't allow userspace writes to the MSIX capability. So it
seems like the above would restore the table entry, but not necessarily
whether MSIX is enabled on the device.
When I talked with Alexey I noted that we already have a BAR restore
function, vfio_bar_restore(), that tries to do some fixup when backdoor
resets are noticed. If we were to trigger that upon noting the user
writing BAR values to what we think they're already set to, we could
extend it to trigger an interrupt restore that could fixup both the
capability and the table entries. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
