Re: [PATCH 3/3] drivers/vfio/pci: Fix MSIx message lost

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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@us.ibm.com>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  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) {