[dpdk-stable] [PATCH 1/2] net/i40e: fix VF initialization error

[dpdk-stable] [PATCH 1/2] net/i40e: fix VF initialization error

[Jingjing Wu]

In igb_uio, FLR is issued during open device file. i40evf is trying
to initialize admin queue when driver probe, while the FLR is not
done by host driver. That will cause initialization fail.

This patch is adding the checking if VF reset is done before
adimin queue initialization.

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Cc: stable@dpdk.org
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 45 +++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 111ac39..43b63da 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1111,10 +1111,31 @@ i40evf_enable_irq0(struct i40e_hw *hw)
 }
 
 static int
-i40evf_reset_vf(struct i40e_hw *hw)
+i40evf_check_vf_reset_done(struct i40e_hw *hw)
 {
 	int i, reset;
 
+	for (i = 0; i < MAX_RESET_WAIT_CNT; i++) {
+		reset = I40E_READ_REG(hw, I40E_VFGEN_RSTAT) &
+			I40E_VFGEN_RSTAT_VFR_STATE_MASK;
+		reset = reset >> I40E_VFGEN_RSTAT_VFR_STATE_SHIFT;
+		if (reset == VIRTCHNL_VFR_VFACTIVE ||
+		    reset == VIRTCHNL_VFR_COMPLETED)
+			break;
+		else
+			rte_delay_ms(50);
+	}
+
+	if (i >= MAX_RESET_WAIT_CNT)
+		return -1;
+
+	return 0;
+}
+static int
+i40evf_reset_vf(struct i40e_hw *hw)
+{
+	int ret;
+
 	if (i40e_vf_reset(hw) != I40E_SUCCESS) {
 		PMD_INIT_LOG(ERR, "Reset VF NIC failed");
 		return -1;
@@ -1130,19 +1151,10 @@ i40evf_reset_vf(struct i40e_hw *hw)
 	  */
 	rte_delay_ms(200);
 
-	for (i = 0; i < MAX_RESET_WAIT_CNT; i++) {
-		reset = rd32(hw, I40E_VFGEN_RSTAT) &
-			I40E_VFGEN_RSTAT_VFR_STATE_MASK;
-		reset = reset >> I40E_VFGEN_RSTAT_VFR_STATE_SHIFT;
-		if (VIRTCHNL_VFR_COMPLETED == reset || VIRTCHNL_VFR_VFACTIVE == reset)
-			break;
-		else
-			rte_delay_ms(50);
-	}
-
-	if (i >= MAX_RESET_WAIT_CNT) {
-		PMD_INIT_LOG(ERR, "Reset VF NIC failed");
-		return -1;
+	ret = i40evf_check_vf_reset_done(hw);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "VF is still resetting");
+		return ret;
 	}
 
 	return 0;
@@ -1165,6 +1177,10 @@ i40evf_init_vf(struct rte_eth_dev *dev)
 		goto err;
 	}
 
+	err = i40evf_check_vf_reset_done(hw);
+	if (err)
+		goto err;
+
 	i40e_init_adminq_parameter(hw);
 	err = i40e_init_adminq(hw);
 	if (err) {
@@ -1189,6 +1205,7 @@ i40evf_init_vf(struct rte_eth_dev *dev)
 		PMD_INIT_LOG(ERR, "init_adminq failed");
 		goto err;
 	}
+
 	vf->aq_resp = rte_zmalloc("vf_aq_resp", I40E_AQ_BUF_SZ, 0);
 	if (!vf->aq_resp) {
 		PMD_INIT_LOG(ERR, "unable to allocate vf_aq_resp memory");
-- 
2.7.4

[dpdk-stable] [PATCH 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Jingjing Wu]

If pass-through a VF by vfio-pci to a Qemu VM, after FLR
in VM, the interrupt setting is not recoverd correctly
to host as below:
 in VM guest:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
 in Host:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-

That was because in pci_reset_function, it first reads the
PCI configure and set FLR reset, and then writes PCI configure
as restoration. But not all the writing are successful to Host.
Becuase vfio-pci driver doesn't allow directly write PCI MSI-X
Cap.

To fix this issue, we need to move the interrupt enablement from
igb_uio probe to open device file. While is also the similar as
the behaviour in vfio_pci kernel module code.

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Cc: stable@dpdk.org

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 222 ++++++++++++++++--------------
 1 file changed, 120 insertions(+), 102 deletions(-)

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index c8dd5f4..e426b52 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
 
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
-
 /* sriov sysfs */
 static ssize_t
 show_max_vfs(struct device *dev, struct device_attribute *attr,
@@ -144,8 +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
  * If yes, disable it here and will be enable later.
  */
 static irqreturn_t
-igbuio_pci_irqhandler(int irq, struct uio_info *info)
+igbuio_pci_irqhandler(int irq, void *dev_id)
 {
+	struct uio_device *idev = (struct uio_device *)dev_id;
+	struct uio_info *info = idev->info;
 	struct rte_uio_pci_dev *udev = info->priv;
 
 	/* Legacy mode need to mask in hardware */
@@ -153,10 +154,116 @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
 	    !pci_check_and_mask_intx(udev->pdev))
 		return IRQ_NONE;
 
+	uio_event_notify(info);
+
 	/* Message signal mode, no share IRQ and automasked */
 	return IRQ_HANDLED;
 }
 
+static int
+igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	int err = 0;
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	struct msix_entry msix_entry;
+#endif
+
+	switch (igbuio_intr_mode_preferred) {
+	case RTE_INTR_MODE_MSIX:
+		/* Only 1 msi-x vector needed */
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		msix_entry.entry = 0;
+		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = msix_entry.vector;
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
+			pr_info("calling pci_enable_msix ... \n");
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#endif
+
+	/* fall back to MSI */
+	case RTE_INTR_MODE_MSI:
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		if (pci_enable_msi(udev->pdev) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#endif
+	/* fall back to INTX */
+	case RTE_INTR_MODE_LEGACY:
+		if (pci_intx_mask_supported(udev->pdev)) {
+			dev_dbg(&udev->pdev->dev, "using INTX");
+			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_LEGACY;
+			break;
+		}
+		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
+	/* fall back to no IRQ */
+	case RTE_INTR_MODE_NONE:
+		udev->mode = RTE_INTR_MODE_NONE;
+		udev->info.irq = UIO_IRQ_NONE;
+		break;
+
+	default:
+		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
+			igbuio_intr_mode_preferred);
+		udev->info.irq = UIO_IRQ_NONE;
+		err = -EINVAL;
+	}
+
+	if (udev->info.irq != UIO_IRQ_NONE)
+		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
+				  udev->info.irq_flags, udev->info.name,
+				  udev->info.uio_dev);
+	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
+		 udev->info.irq);
+
+	return err;
+}
+
+static void
+igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	if (udev->info.irq) {
+		free_irq(udev->info.irq, udev->info.uio_dev);
+		udev->info.irq = 0;
+	}
+
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	if (udev->mode == RTE_INTR_MODE_MSIX)
+		pci_disable_msix(udev->pdev);
+	if (udev->mode == RTE_INTR_MODE_MSI)
+		pci_disable_msi(udev->pdev);
+#else
+	if (udev->mode == RTE_INTR_MODE_MSIX ||
+	    udev->mode == RTE_INTR_MODE_MSI)
+		pci_free_irq_vectors(udev->pdev);
+#endif
+}
+
+
 /**
  * This gets called while opening uio device file.
  */
@@ -165,12 +272,19 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode)
 {
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
+	int err;
 
 	pci_reset_function(dev);
 
 	/* set bus master, which was cleared by the reset function */
 	pci_set_master(dev);
 
+	/* enable interrupts */
+	err = igbuio_pci_enable_interrupts(udev);
+	if (err) {
+		dev_err(&dev->dev, "Enable interrupt fails\n");
+		return err;
+	}
 	return 0;
 }
 
@@ -180,6 +294,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
 
+	/* disable interrupts */
+	igbuio_pci_disable_interrupts(udev);
+
 	/* stop the device from further DMA */
 	pci_clear_master(dev);
 
@@ -250,94 +367,6 @@ igbuio_pci_release_iomem(struct uio_info *info)
 }
 
 static int
-igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
-{
-	int err = 0;
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	struct msix_entry msix_entry;
-#endif
-
-	switch (igbuio_intr_mode_preferred) {
-	case RTE_INTR_MODE_MSIX:
-		/* Only 1 msi-x vector needed */
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		msix_entry.entry = 0;
-		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = msix_entry.vector;
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#endif
-	/* fall back to MSI */
-	case RTE_INTR_MODE_MSI:
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		if (pci_enable_msi(udev->pdev) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#endif
-	/* fall back to INTX */
-	case RTE_INTR_MODE_LEGACY:
-		if (pci_intx_mask_supported(udev->pdev)) {
-			dev_dbg(&udev->pdev->dev, "using INTX");
-			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_LEGACY;
-			break;
-		}
-		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
-	/* fall back to no IRQ */
-	case RTE_INTR_MODE_NONE:
-		udev->mode = RTE_INTR_MODE_NONE;
-		udev->info.irq = UIO_IRQ_NONE;
-		break;
-
-	default:
-		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
-			igbuio_intr_mode_preferred);
-		err = -EINVAL;
-	}
-
-	return err;
-}
-
-static void
-igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
-{
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	if (udev->mode == RTE_INTR_MODE_MSIX)
-		pci_disable_msix(udev->pdev);
-	if (udev->mode == RTE_INTR_MODE_MSI)
-		pci_disable_msi(udev->pdev);
-#else
-	if (udev->mode == RTE_INTR_MODE_MSIX ||
-	    udev->mode == RTE_INTR_MODE_MSI)
-		pci_free_irq_vectors(udev->pdev);
-#endif
-}
-
-static int
 igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
 {
 	int i, iom, iop, ret;
@@ -427,20 +456,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* fill uio infos */
 	udev->info.name = "igb_uio";
 	udev->info.version = "0.1";
-	udev->info.handler = igbuio_pci_irqhandler;
 	udev->info.irqcontrol = igbuio_pci_irqcontrol;
 	udev->info.open = igbuio_pci_open;
 	udev->info.release = igbuio_pci_release;
 	udev->info.priv = udev;
 	udev->pdev = dev;
 
-	err = igbuio_pci_enable_interrupts(udev);
-	if (err != 0)
-		goto fail_release_iomem;
-
 	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
 	if (err != 0)
-		goto fail_disable_interrupts;
+		goto fail_release_iomem;
 
 	/* register uio driver */
 	err = uio_register_device(&dev->dev, &udev->info);
@@ -449,9 +473,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	pci_set_drvdata(dev, udev);
 
-	dev_info(&dev->dev, "uio device registered with irq %lx\n",
-		 udev->info.irq);
-
 	/*
 	 * Doing a harmless dma mapping for attaching the device to
 	 * the iommu identity mapping if kernel boots with iommu=pt.
@@ -477,8 +498,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 fail_remove_group:
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
-fail_disable_interrupts:
-	igbuio_pci_disable_interrupts(udev);
 fail_release_iomem:
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
@@ -495,7 +514,6 @@ igbuio_pci_remove(struct pci_dev *dev)
 
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
 	uio_unregister_device(&udev->info);
-	igbuio_pci_disable_interrupts(udev);
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
 	pci_set_drvdata(dev, NULL);
-- 
2.7.4

[dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Jingjing Wu]

If pass-through a VF by vfio-pci to a Qemu VM, after FLR
in VM, the interrupt setting is not recoverd correctly
to host as below:
 in VM guest:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
 in Host:
        Capabilities: [70] MSI-X: Enable+ Count=5 Masked-

That was because in pci_reset_function, it first reads the
PCI configure and set FLR reset, and then writes PCI configure
as restoration. But not all the writing are successful to Host.
Because vfio-pci driver doesn't allow directly write PCI MSI-X
Cap.

To fix this issue, we need to move the interrupt enablement from
igb_uio probe to open device file. While it is also the similar as
the behaviour in vfio_pci kernel module code.

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Cc: stable@dpdk.org

Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
v2 change:
 - fix typo
 - remove duplicated debug info

 lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
 1 file changed, 119 insertions(+), 102 deletions(-)

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index c8dd5f4..d943795 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
 
 static char *intr_mode;
 static enum rte_intr_mode igbuio_intr_mode_preferred = RTE_INTR_MODE_MSIX;
-
 /* sriov sysfs */
 static ssize_t
 show_max_vfs(struct device *dev, struct device_attribute *attr,
@@ -144,8 +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
  * If yes, disable it here and will be enable later.
  */
 static irqreturn_t
-igbuio_pci_irqhandler(int irq, struct uio_info *info)
+igbuio_pci_irqhandler(int irq, void *dev_id)
 {
+	struct uio_device *idev = (struct uio_device *)dev_id;
+	struct uio_info *info = idev->info;
 	struct rte_uio_pci_dev *udev = info->priv;
 
 	/* Legacy mode need to mask in hardware */
@@ -153,10 +154,115 @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
 	    !pci_check_and_mask_intx(udev->pdev))
 		return IRQ_NONE;
 
+	uio_event_notify(info);
+
 	/* Message signal mode, no share IRQ and automasked */
 	return IRQ_HANDLED;
 }
 
+static int
+igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	int err = 0;
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	struct msix_entry msix_entry;
+#endif
+
+	switch (igbuio_intr_mode_preferred) {
+	case RTE_INTR_MODE_MSIX:
+		/* Only 1 msi-x vector needed */
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		msix_entry.entry = 0;
+		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = msix_entry.vector;
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
+			dev_dbg(&udev->pdev->dev, "using MSI-X");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSIX;
+			break;
+		}
+#endif
+
+	/* fall back to MSI */
+	case RTE_INTR_MODE_MSI:
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+		if (pci_enable_msi(udev->pdev) == 0) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#else
+		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
+			dev_dbg(&udev->pdev->dev, "using MSI");
+			udev->info.irq_flags = IRQF_NO_THREAD;
+			udev->info.irq = pci_irq_vector(udev->pdev, 0);
+			udev->mode = RTE_INTR_MODE_MSI;
+			break;
+		}
+#endif
+	/* fall back to INTX */
+	case RTE_INTR_MODE_LEGACY:
+		if (pci_intx_mask_supported(udev->pdev)) {
+			dev_dbg(&udev->pdev->dev, "using INTX");
+			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
+			udev->info.irq = udev->pdev->irq;
+			udev->mode = RTE_INTR_MODE_LEGACY;
+			break;
+		}
+		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
+	/* fall back to no IRQ */
+	case RTE_INTR_MODE_NONE:
+		udev->mode = RTE_INTR_MODE_NONE;
+		udev->info.irq = UIO_IRQ_NONE;
+		break;
+
+	default:
+		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
+			igbuio_intr_mode_preferred);
+		udev->info.irq = UIO_IRQ_NONE;
+		err = -EINVAL;
+	}
+
+	if (udev->info.irq != UIO_IRQ_NONE)
+		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
+				  udev->info.irq_flags, udev->info.name,
+				  udev->info.uio_dev);
+	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
+		 udev->info.irq);
+
+	return err;
+}
+
+static void
+igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
+{
+	if (udev->info.irq) {
+		free_irq(udev->info.irq, udev->info.uio_dev);
+		udev->info.irq = 0;
+	}
+
+#ifndef HAVE_ALLOC_IRQ_VECTORS
+	if (udev->mode == RTE_INTR_MODE_MSIX)
+		pci_disable_msix(udev->pdev);
+	if (udev->mode == RTE_INTR_MODE_MSI)
+		pci_disable_msi(udev->pdev);
+#else
+	if (udev->mode == RTE_INTR_MODE_MSIX ||
+	    udev->mode == RTE_INTR_MODE_MSI)
+		pci_free_irq_vectors(udev->pdev);
+#endif
+}
+
+
 /**
  * This gets called while opening uio device file.
  */
@@ -165,12 +271,19 @@ igbuio_pci_open(struct uio_info *info, struct inode *inode)
 {
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
+	int err;
 
 	pci_reset_function(dev);
 
 	/* set bus master, which was cleared by the reset function */
 	pci_set_master(dev);
 
+	/* enable interrupts */
+	err = igbuio_pci_enable_interrupts(udev);
+	if (err) {
+		dev_err(&dev->dev, "Enable interrupt fails\n");
+		return err;
+	}
 	return 0;
 }
 
@@ -180,6 +293,9 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode)
 	struct rte_uio_pci_dev *udev = info->priv;
 	struct pci_dev *dev = udev->pdev;
 
+	/* disable interrupts */
+	igbuio_pci_disable_interrupts(udev);
+
 	/* stop the device from further DMA */
 	pci_clear_master(dev);
 
@@ -250,94 +366,6 @@ igbuio_pci_release_iomem(struct uio_info *info)
 }
 
 static int
-igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev)
-{
-	int err = 0;
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	struct msix_entry msix_entry;
-#endif
-
-	switch (igbuio_intr_mode_preferred) {
-	case RTE_INTR_MODE_MSIX:
-		/* Only 1 msi-x vector needed */
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		msix_entry.entry = 0;
-		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = msix_entry.vector;
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI-X");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSIX;
-			break;
-		}
-#endif
-	/* fall back to MSI */
-	case RTE_INTR_MODE_MSI:
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-		if (pci_enable_msi(udev->pdev) == 0) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#else
-		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1) {
-			dev_dbg(&udev->pdev->dev, "using MSI");
-			udev->info.irq_flags = IRQF_NO_THREAD;
-			udev->info.irq = pci_irq_vector(udev->pdev, 0);
-			udev->mode = RTE_INTR_MODE_MSI;
-			break;
-		}
-#endif
-	/* fall back to INTX */
-	case RTE_INTR_MODE_LEGACY:
-		if (pci_intx_mask_supported(udev->pdev)) {
-			dev_dbg(&udev->pdev->dev, "using INTX");
-			udev->info.irq_flags = IRQF_SHARED | IRQF_NO_THREAD;
-			udev->info.irq = udev->pdev->irq;
-			udev->mode = RTE_INTR_MODE_LEGACY;
-			break;
-		}
-		dev_notice(&udev->pdev->dev, "PCI INTX mask not supported\n");
-	/* fall back to no IRQ */
-	case RTE_INTR_MODE_NONE:
-		udev->mode = RTE_INTR_MODE_NONE;
-		udev->info.irq = UIO_IRQ_NONE;
-		break;
-
-	default:
-		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
-			igbuio_intr_mode_preferred);
-		err = -EINVAL;
-	}
-
-	return err;
-}
-
-static void
-igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev)
-{
-#ifndef HAVE_ALLOC_IRQ_VECTORS
-	if (udev->mode == RTE_INTR_MODE_MSIX)
-		pci_disable_msix(udev->pdev);
-	if (udev->mode == RTE_INTR_MODE_MSI)
-		pci_disable_msi(udev->pdev);
-#else
-	if (udev->mode == RTE_INTR_MODE_MSIX ||
-	    udev->mode == RTE_INTR_MODE_MSI)
-		pci_free_irq_vectors(udev->pdev);
-#endif
-}
-
-static int
 igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)
 {
 	int i, iom, iop, ret;
@@ -427,20 +455,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	/* fill uio infos */
 	udev->info.name = "igb_uio";
 	udev->info.version = "0.1";
-	udev->info.handler = igbuio_pci_irqhandler;
 	udev->info.irqcontrol = igbuio_pci_irqcontrol;
 	udev->info.open = igbuio_pci_open;
 	udev->info.release = igbuio_pci_release;
 	udev->info.priv = udev;
 	udev->pdev = dev;
 
-	err = igbuio_pci_enable_interrupts(udev);
-	if (err != 0)
-		goto fail_release_iomem;
-
 	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
 	if (err != 0)
-		goto fail_disable_interrupts;
+		goto fail_release_iomem;
 
 	/* register uio driver */
 	err = uio_register_device(&dev->dev, &udev->info);
@@ -449,9 +472,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	pci_set_drvdata(dev, udev);
 
-	dev_info(&dev->dev, "uio device registered with irq %lx\n",
-		 udev->info.irq);
-
 	/*
 	 * Doing a harmless dma mapping for attaching the device to
 	 * the iommu identity mapping if kernel boots with iommu=pt.
@@ -477,8 +497,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 fail_remove_group:
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
-fail_disable_interrupts:
-	igbuio_pci_disable_interrupts(udev);
 fail_release_iomem:
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
@@ -495,7 +513,6 @@ igbuio_pci_remove(struct pci_dev *dev)
 
 	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
 	uio_unregister_device(&udev->info);
-	igbuio_pci_disable_interrupts(udev);
 	igbuio_pci_release_iomem(&udev->info);
 	pci_disable_device(dev);
 	pci_set_drvdata(dev, NULL);
-- 
2.7.4

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Wu, Jingjing]

Hi, Shjith

Could you help to review and verify if this fix can meet your requirement?

Thanks
Jingjing

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Tuesday, October 10, 2017 6:09 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Tan, Jianfeng
> <jianfeng.tan@intel.com>; shijith.thotton@caviumnetworks.com;
> gregory@weka.io; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
> 
> If pass-through a VF by vfio-pci to a Qemu VM, after FLR in VM, the interrupt
> setting is not recoverd correctly to host as below:
>  in VM guest:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-  in Host:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> 
> That was because in pci_reset_function, it first reads the PCI configure and set
> FLR reset, and then writes PCI configure as restoration. But not all the writing
> are successful to Host.
> Because vfio-pci driver doesn't allow directly write PCI MSI-X Cap.
> 
> To fix this issue, we need to move the interrupt enablement from igb_uio probe
> to open device file. While it is also the similar as the behaviour in vfio_pci
> kernel module code.
> 
> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
> v2 change:
>  - fix typo
>  - remove duplicated debug info
> 
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
>  1 file changed, 119 insertions(+), 102 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index c8dd5f4..d943795 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
> 
>  static char *intr_mode;
>  static enum rte_intr_mode igbuio_intr_mode_preferred =
> RTE_INTR_MODE_MSIX;
> -
>  /* sriov sysfs */
>  static ssize_t
>  show_max_vfs(struct device *dev, struct device_attribute *attr, @@ -144,8
> +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
>   * If yes, disable it here and will be enable later.
>   */
>  static irqreturn_t
> -igbuio_pci_irqhandler(int irq, struct uio_info *info)
> +igbuio_pci_irqhandler(int irq, void *dev_id)
>  {
> +	struct uio_device *idev = (struct uio_device *)dev_id;
> +	struct uio_info *info = idev->info;
>  	struct rte_uio_pci_dev *udev = info->priv;
> 
>  	/* Legacy mode need to mask in hardware */ @@ -153,10 +154,115
> @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
>  	    !pci_check_and_mask_intx(udev->pdev))
>  		return IRQ_NONE;
> 
> +	uio_event_notify(info);
> +
>  	/* Message signal mode, no share IRQ and automasked */
>  	return IRQ_HANDLED;
>  }
> 
> +static int
> +igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) {
> +	int err = 0;
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +	struct msix_entry msix_entry;
> +#endif
> +
> +	switch (igbuio_intr_mode_preferred) {
> +	case RTE_INTR_MODE_MSIX:
> +		/* Only 1 msi-x vector needed */
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +		msix_entry.entry = 0;
> +		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = msix_entry.vector;
> +			udev->mode = RTE_INTR_MODE_MSIX;
> +			break;
> +		}
> +#else
> +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> {
> +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> +			udev->mode = RTE_INTR_MODE_MSIX;
> +			break;
> +		}
> +#endif
> +
> +	/* fall back to MSI */
> +	case RTE_INTR_MODE_MSI:
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +		if (pci_enable_msi(udev->pdev) == 0) {
> +			dev_dbg(&udev->pdev->dev, "using MSI");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = udev->pdev->irq;
> +			udev->mode = RTE_INTR_MODE_MSI;
> +			break;
> +		}
> +#else
> +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> {
> +			dev_dbg(&udev->pdev->dev, "using MSI");
> +			udev->info.irq_flags = IRQF_NO_THREAD;
> +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> +			udev->mode = RTE_INTR_MODE_MSI;
> +			break;
> +		}
> +#endif
> +	/* fall back to INTX */
> +	case RTE_INTR_MODE_LEGACY:
> +		if (pci_intx_mask_supported(udev->pdev)) {
> +			dev_dbg(&udev->pdev->dev, "using INTX");
> +			udev->info.irq_flags = IRQF_SHARED |
> IRQF_NO_THREAD;
> +			udev->info.irq = udev->pdev->irq;
> +			udev->mode = RTE_INTR_MODE_LEGACY;
> +			break;
> +		}
> +		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> supported\n");
> +	/* fall back to no IRQ */
> +	case RTE_INTR_MODE_NONE:
> +		udev->mode = RTE_INTR_MODE_NONE;
> +		udev->info.irq = UIO_IRQ_NONE;
> +		break;
> +
> +	default:
> +		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> +			igbuio_intr_mode_preferred);
> +		udev->info.irq = UIO_IRQ_NONE;
> +		err = -EINVAL;
> +	}
> +
> +	if (udev->info.irq != UIO_IRQ_NONE)
> +		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
> +				  udev->info.irq_flags, udev->info.name,
> +				  udev->info.uio_dev);
> +	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
> +		 udev->info.irq);
> +
> +	return err;
> +}
> +
> +static void
> +igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) {
> +	if (udev->info.irq) {
> +		free_irq(udev->info.irq, udev->info.uio_dev);
> +		udev->info.irq = 0;
> +	}
> +
> +#ifndef HAVE_ALLOC_IRQ_VECTORS
> +	if (udev->mode == RTE_INTR_MODE_MSIX)
> +		pci_disable_msix(udev->pdev);
> +	if (udev->mode == RTE_INTR_MODE_MSI)
> +		pci_disable_msi(udev->pdev);
> +#else
> +	if (udev->mode == RTE_INTR_MODE_MSIX ||
> +	    udev->mode == RTE_INTR_MODE_MSI)
> +		pci_free_irq_vectors(udev->pdev);
> +#endif
> +}
> +
> +
>  /**
>   * This gets called while opening uio device file.
>   */
> @@ -165,12 +271,19 @@ igbuio_pci_open(struct uio_info *info, struct inode
> *inode)  {
>  	struct rte_uio_pci_dev *udev = info->priv;
>  	struct pci_dev *dev = udev->pdev;
> +	int err;
> 
>  	pci_reset_function(dev);
> 
>  	/* set bus master, which was cleared by the reset function */
>  	pci_set_master(dev);
> 
> +	/* enable interrupts */
> +	err = igbuio_pci_enable_interrupts(udev);
> +	if (err) {
> +		dev_err(&dev->dev, "Enable interrupt fails\n");
> +		return err;
> +	}
>  	return 0;
>  }
> 
> @@ -180,6 +293,9 @@ igbuio_pci_release(struct uio_info *info, struct inode
> *inode)
>  	struct rte_uio_pci_dev *udev = info->priv;
>  	struct pci_dev *dev = udev->pdev;
> 
> +	/* disable interrupts */
> +	igbuio_pci_disable_interrupts(udev);
> +
>  	/* stop the device from further DMA */
>  	pci_clear_master(dev);
> 
> @@ -250,94 +366,6 @@ igbuio_pci_release_iomem(struct uio_info *info)  }
> 
>  static int
> -igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) -{
> -	int err = 0;
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -	struct msix_entry msix_entry;
> -#endif
> -
> -	switch (igbuio_intr_mode_preferred) {
> -	case RTE_INTR_MODE_MSIX:
> -		/* Only 1 msi-x vector needed */
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -		msix_entry.entry = 0;
> -		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = msix_entry.vector;
> -			udev->mode = RTE_INTR_MODE_MSIX;
> -			break;
> -		}
> -#else
> -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> {
> -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> -			udev->mode = RTE_INTR_MODE_MSIX;
> -			break;
> -		}
> -#endif
> -	/* fall back to MSI */
> -	case RTE_INTR_MODE_MSI:
> -#ifndef HAVE_ALLOC_IRQ_VECTORS
> -		if (pci_enable_msi(udev->pdev) == 0) {
> -			dev_dbg(&udev->pdev->dev, "using MSI");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = udev->pdev->irq;
> -			udev->mode = RTE_INTR_MODE_MSI;
> -			break;
> -		}
> -#else
> -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> {
> -			dev_dbg(&udev->pdev->dev, "using MSI");
> -			udev->info.irq_flags = IRQF_NO_THREAD;
> -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> -			udev->mode = RTE_INTR_MODE_MSI;
> -			break;
> -		}
> -#endif
> -	/* fall back to INTX */
> -	case RTE_INTR_MODE_LEGACY:
> -		if (pci_intx_mask_supported(udev->pdev)) {
> -			dev_dbg(&udev->pdev->dev, "using INTX");
> -			udev->info.irq_flags = IRQF_SHARED |
> IRQF_NO_THREAD;
> -			udev->info.irq = udev->pdev->irq;
> -			udev->mode = RTE_INTR_MODE_LEGACY;
> -			break;
> -		}
> -		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> supported\n");
> -	/* fall back to no IRQ */
> -	case RTE_INTR_MODE_NONE:
> -		udev->mode = RTE_INTR_MODE_NONE;
> -		udev->info.irq = UIO_IRQ_NONE;
> -		break;
> -
> -	default:
> -		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> -			igbuio_intr_mode_preferred);
> -		err = -EINVAL;
> -	}
> -
> -	return err;
> -}
> -
> -static void
> -igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) -{ -#ifndef
> HAVE_ALLOC_IRQ_VECTORS
> -	if (udev->mode == RTE_INTR_MODE_MSIX)
> -		pci_disable_msix(udev->pdev);
> -	if (udev->mode == RTE_INTR_MODE_MSI)
> -		pci_disable_msi(udev->pdev);
> -#else
> -	if (udev->mode == RTE_INTR_MODE_MSIX ||
> -	    udev->mode == RTE_INTR_MODE_MSI)
> -		pci_free_irq_vectors(udev->pdev);
> -#endif
> -}
> -
> -static int
>  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)  {
>  	int i, iom, iop, ret;
> @@ -427,20 +455,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
>  	/* fill uio infos */
>  	udev->info.name = "igb_uio";
>  	udev->info.version = "0.1";
> -	udev->info.handler = igbuio_pci_irqhandler;
>  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
>  	udev->info.open = igbuio_pci_open;
>  	udev->info.release = igbuio_pci_release;
>  	udev->info.priv = udev;
>  	udev->pdev = dev;
> 
> -	err = igbuio_pci_enable_interrupts(udev);
> -	if (err != 0)
> -		goto fail_release_iomem;
> -
>  	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
>  	if (err != 0)
> -		goto fail_disable_interrupts;
> +		goto fail_release_iomem;
> 
>  	/* register uio driver */
>  	err = uio_register_device(&dev->dev, &udev->info); @@ -449,9 +472,6
> @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> 
>  	pci_set_drvdata(dev, udev);
> 
> -	dev_info(&dev->dev, "uio device registered with irq %lx\n",
> -		 udev->info.irq);
> -
>  	/*
>  	 * Doing a harmless dma mapping for attaching the device to
>  	 * the iommu identity mapping if kernel boots with iommu=pt.
> @@ -477,8 +497,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> 
>  fail_remove_group:
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> -fail_disable_interrupts:
> -	igbuio_pci_disable_interrupts(udev);
>  fail_release_iomem:
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
> @@ -495,7 +513,6 @@ igbuio_pci_remove(struct pci_dev *dev)
> 
>  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
>  	uio_unregister_device(&udev->info);
> -	igbuio_pci_disable_interrupts(udev);
>  	igbuio_pci_release_iomem(&udev->info);
>  	pci_disable_device(dev);
>  	pci_set_drvdata(dev, NULL);
> --
> 2.7.4

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Shijith Thotton]

On Thu, Oct 12, 2017 at 05:43:29AM +0000, Wu, Jingjing wrote:
> Hi, Shjith
> 
> Could you help to review and verify if this fix can meet your requirement?
> 
> Thanks
> Jingjing
> 
> > -----Original Message-----
> > From: Wu, Jingjing
> > Sent: Tuesday, October 10, 2017 6:09 AM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>; Tan, Jianfeng
> > <jianfeng.tan@intel.com>; shijith.thotton@caviumnetworks.com;
> > gregory@weka.io; Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; stable@dpdk.org
> > Subject: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
> > 
> > If pass-through a VF by vfio-pci to a Qemu VM, after FLR in VM, the interrupt
> > setting is not recoverd correctly to host as below:
> >  in VM guest:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-  in Host:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> > 
> > That was because in pci_reset_function, it first reads the PCI configure and set
> > FLR reset, and then writes PCI configure as restoration. But not all the writing
> > are successful to Host.
> > Because vfio-pci driver doesn't allow directly write PCI MSI-X Cap.
> > 
> > To fix this issue, we need to move the interrupt enablement from igb_uio probe
> > to open device file. While it is also the similar as the behaviour in vfio_pci
> > kernel module code.
> > 
> > Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Tested-by: Shijith Thotton <shijith.thotton@caviumnetworks.com>

> > ---
> > v2 change:
> >  - fix typo
> >  - remove duplicated debug info
> > 
> >  lib/librte_eal/linuxapp/igb_uio/igb_uio.c | 221 ++++++++++++++++--------------
> >  1 file changed, 119 insertions(+), 102 deletions(-)
> > 
> > diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > index c8dd5f4..d943795 100644
> > --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> > @@ -49,7 +49,6 @@ struct rte_uio_pci_dev {
> > 
> >  static char *intr_mode;
> >  static enum rte_intr_mode igbuio_intr_mode_preferred =
> > RTE_INTR_MODE_MSIX;
> > -
> >  /* sriov sysfs */
> >  static ssize_t
> >  show_max_vfs(struct device *dev, struct device_attribute *attr, @@ -144,8
> > +143,10 @@ igbuio_pci_irqcontrol(struct uio_info *info, s32 irq_state)
> >   * If yes, disable it here and will be enable later.
> >   */
> >  static irqreturn_t
> > -igbuio_pci_irqhandler(int irq, struct uio_info *info)
> > +igbuio_pci_irqhandler(int irq, void *dev_id)
> >  {
> > +	struct uio_device *idev = (struct uio_device *)dev_id;
> > +	struct uio_info *info = idev->info;
> >  	struct rte_uio_pci_dev *udev = info->priv;
> > 
> >  	/* Legacy mode need to mask in hardware */ @@ -153,10 +154,115
> > @@ igbuio_pci_irqhandler(int irq, struct uio_info *info)
> >  	    !pci_check_and_mask_intx(udev->pdev))
> >  		return IRQ_NONE;
> > 
> > +	uio_event_notify(info);
> > +
> >  	/* Message signal mode, no share IRQ and automasked */
> >  	return IRQ_HANDLED;
> >  }
> > 
> > +static int
> > +igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) {
> > +	int err = 0;
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +	struct msix_entry msix_entry;
> > +#endif
> > +
> > +	switch (igbuio_intr_mode_preferred) {
> > +	case RTE_INTR_MODE_MSIX:
> > +		/* Only 1 msi-x vector needed */
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +		msix_entry.entry = 0;
> > +		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> > +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = msix_entry.vector;
> > +			udev->mode = RTE_INTR_MODE_MSIX;
> > +			break;
> > +		}
> > +#else
> > +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> > {
> > +			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > +			udev->mode = RTE_INTR_MODE_MSIX;
> > +			break;
> > +		}
> > +#endif
> > +
> > +	/* fall back to MSI */
> > +	case RTE_INTR_MODE_MSI:
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +		if (pci_enable_msi(udev->pdev) == 0) {
> > +			dev_dbg(&udev->pdev->dev, "using MSI");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = udev->pdev->irq;
> > +			udev->mode = RTE_INTR_MODE_MSI;
> > +			break;
> > +		}
> > +#else
> > +		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> > {
> > +			dev_dbg(&udev->pdev->dev, "using MSI");
> > +			udev->info.irq_flags = IRQF_NO_THREAD;
> > +			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > +			udev->mode = RTE_INTR_MODE_MSI;
> > +			break;
> > +		}
> > +#endif
> > +	/* fall back to INTX */
> > +	case RTE_INTR_MODE_LEGACY:
> > +		if (pci_intx_mask_supported(udev->pdev)) {
> > +			dev_dbg(&udev->pdev->dev, "using INTX");
> > +			udev->info.irq_flags = IRQF_SHARED |
> > IRQF_NO_THREAD;
> > +			udev->info.irq = udev->pdev->irq;
> > +			udev->mode = RTE_INTR_MODE_LEGACY;
> > +			break;
> > +		}
> > +		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> > supported\n");
> > +	/* fall back to no IRQ */
> > +	case RTE_INTR_MODE_NONE:
> > +		udev->mode = RTE_INTR_MODE_NONE;
> > +		udev->info.irq = UIO_IRQ_NONE;
> > +		break;
> > +
> > +	default:
> > +		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> > +			igbuio_intr_mode_preferred);
> > +		udev->info.irq = UIO_IRQ_NONE;
> > +		err = -EINVAL;
> > +	}
> > +
> > +	if (udev->info.irq != UIO_IRQ_NONE)
> > +		err = request_irq(udev->info.irq, igbuio_pci_irqhandler,
> > +				  udev->info.irq_flags, udev->info.name,
> > +				  udev->info.uio_dev);
> > +	dev_info(&udev->pdev->dev, "uio device registered with irq %lx\n",
> > +		 udev->info.irq);
> > +
> > +	return err;
> > +}
> > +
> > +static void
> > +igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) {
> > +	if (udev->info.irq) {
> > +		free_irq(udev->info.irq, udev->info.uio_dev);
> > +		udev->info.irq = 0;
> > +	}
> > +
> > +#ifndef HAVE_ALLOC_IRQ_VECTORS
> > +	if (udev->mode == RTE_INTR_MODE_MSIX)
> > +		pci_disable_msix(udev->pdev);
> > +	if (udev->mode == RTE_INTR_MODE_MSI)
> > +		pci_disable_msi(udev->pdev);
> > +#else
> > +	if (udev->mode == RTE_INTR_MODE_MSIX ||
> > +	    udev->mode == RTE_INTR_MODE_MSI)
> > +		pci_free_irq_vectors(udev->pdev);
> > +#endif
> > +}
> > +
> > +
> >  /**
> >   * This gets called while opening uio device file.
> >   */
> > @@ -165,12 +271,19 @@ igbuio_pci_open(struct uio_info *info, struct inode
> > *inode)  {
> >  	struct rte_uio_pci_dev *udev = info->priv;
> >  	struct pci_dev *dev = udev->pdev;
> > +	int err;
> > 
> >  	pci_reset_function(dev);
> > 
> >  	/* set bus master, which was cleared by the reset function */
> >  	pci_set_master(dev);
> > 
> > +	/* enable interrupts */
> > +	err = igbuio_pci_enable_interrupts(udev);
> > +	if (err) {
> > +		dev_err(&dev->dev, "Enable interrupt fails\n");
> > +		return err;
> > +	}
> >  	return 0;
> >  }
> > 
> > @@ -180,6 +293,9 @@ igbuio_pci_release(struct uio_info *info, struct inode
> > *inode)
> >  	struct rte_uio_pci_dev *udev = info->priv;
> >  	struct pci_dev *dev = udev->pdev;
> > 
> > +	/* disable interrupts */
> > +	igbuio_pci_disable_interrupts(udev);
> > +
> >  	/* stop the device from further DMA */
> >  	pci_clear_master(dev);
> > 
> > @@ -250,94 +366,6 @@ igbuio_pci_release_iomem(struct uio_info *info)  }
> > 
> >  static int
> > -igbuio_pci_enable_interrupts(struct rte_uio_pci_dev *udev) -{
> > -	int err = 0;
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -	struct msix_entry msix_entry;
> > -#endif
> > -
> > -	switch (igbuio_intr_mode_preferred) {
> > -	case RTE_INTR_MODE_MSIX:
> > -		/* Only 1 msi-x vector needed */
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -		msix_entry.entry = 0;
> > -		if (pci_enable_msix(udev->pdev, &msix_entry, 1) == 0) {
> > -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = msix_entry.vector;
> > -			udev->mode = RTE_INTR_MODE_MSIX;
> > -			break;
> > -		}
> > -#else
> > -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSIX) == 1)
> > {
> > -			dev_dbg(&udev->pdev->dev, "using MSI-X");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > -			udev->mode = RTE_INTR_MODE_MSIX;
> > -			break;
> > -		}
> > -#endif
> > -	/* fall back to MSI */
> > -	case RTE_INTR_MODE_MSI:
> > -#ifndef HAVE_ALLOC_IRQ_VECTORS
> > -		if (pci_enable_msi(udev->pdev) == 0) {
> > -			dev_dbg(&udev->pdev->dev, "using MSI");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = udev->pdev->irq;
> > -			udev->mode = RTE_INTR_MODE_MSI;
> > -			break;
> > -		}
> > -#else
> > -		if (pci_alloc_irq_vectors(udev->pdev, 1, 1, PCI_IRQ_MSI) == 1)
> > {
> > -			dev_dbg(&udev->pdev->dev, "using MSI");
> > -			udev->info.irq_flags = IRQF_NO_THREAD;
> > -			udev->info.irq = pci_irq_vector(udev->pdev, 0);
> > -			udev->mode = RTE_INTR_MODE_MSI;
> > -			break;
> > -		}
> > -#endif
> > -	/* fall back to INTX */
> > -	case RTE_INTR_MODE_LEGACY:
> > -		if (pci_intx_mask_supported(udev->pdev)) {
> > -			dev_dbg(&udev->pdev->dev, "using INTX");
> > -			udev->info.irq_flags = IRQF_SHARED |
> > IRQF_NO_THREAD;
> > -			udev->info.irq = udev->pdev->irq;
> > -			udev->mode = RTE_INTR_MODE_LEGACY;
> > -			break;
> > -		}
> > -		dev_notice(&udev->pdev->dev, "PCI INTX mask not
> > supported\n");
> > -	/* fall back to no IRQ */
> > -	case RTE_INTR_MODE_NONE:
> > -		udev->mode = RTE_INTR_MODE_NONE;
> > -		udev->info.irq = UIO_IRQ_NONE;
> > -		break;
> > -
> > -	default:
> > -		dev_err(&udev->pdev->dev, "invalid IRQ mode %u",
> > -			igbuio_intr_mode_preferred);
> > -		err = -EINVAL;
> > -	}
> > -
> > -	return err;
> > -}
> > -
> > -static void
> > -igbuio_pci_disable_interrupts(struct rte_uio_pci_dev *udev) -{ -#ifndef
> > HAVE_ALLOC_IRQ_VECTORS
> > -	if (udev->mode == RTE_INTR_MODE_MSIX)
> > -		pci_disable_msix(udev->pdev);
> > -	if (udev->mode == RTE_INTR_MODE_MSI)
> > -		pci_disable_msi(udev->pdev);
> > -#else
> > -	if (udev->mode == RTE_INTR_MODE_MSIX ||
> > -	    udev->mode == RTE_INTR_MODE_MSI)
> > -		pci_free_irq_vectors(udev->pdev);
> > -#endif
> > -}
> > -
> > -static int
> >  igbuio_setup_bars(struct pci_dev *dev, struct uio_info *info)  {
> >  	int i, iom, iop, ret;
> > @@ -427,20 +455,15 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> >  	/* fill uio infos */
> >  	udev->info.name = "igb_uio";
> >  	udev->info.version = "0.1";
> > -	udev->info.handler = igbuio_pci_irqhandler;
> >  	udev->info.irqcontrol = igbuio_pci_irqcontrol;
> >  	udev->info.open = igbuio_pci_open;
> >  	udev->info.release = igbuio_pci_release;
> >  	udev->info.priv = udev;
> >  	udev->pdev = dev;
> > 
> > -	err = igbuio_pci_enable_interrupts(udev);
> > -	if (err != 0)
> > -		goto fail_release_iomem;
> > -
> >  	err = sysfs_create_group(&dev->dev.kobj, &dev_attr_grp);
> >  	if (err != 0)
> > -		goto fail_disable_interrupts;
> > +		goto fail_release_iomem;
> > 
> >  	/* register uio driver */
> >  	err = uio_register_device(&dev->dev, &udev->info); @@ -449,9 +472,6
> > @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > 
> >  	pci_set_drvdata(dev, udev);
> > 
> > -	dev_info(&dev->dev, "uio device registered with irq %lx\n",
> > -		 udev->info.irq);
> > -
> >  	/*
> >  	 * Doing a harmless dma mapping for attaching the device to
> >  	 * the iommu identity mapping if kernel boots with iommu=pt.
> > @@ -477,8 +497,6 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> > pci_device_id *id)
> > 
> >  fail_remove_group:
> >  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> > -fail_disable_interrupts:
> > -	igbuio_pci_disable_interrupts(udev);
> >  fail_release_iomem:
> >  	igbuio_pci_release_iomem(&udev->info);
> >  	pci_disable_device(dev);
> > @@ -495,7 +513,6 @@ igbuio_pci_remove(struct pci_dev *dev)
> > 
> >  	sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> >  	uio_unregister_device(&udev->info);
> > -	igbuio_pci_disable_interrupts(udev);
> >  	igbuio_pci_release_iomem(&udev->info);
> >  	pci_disable_device(dev);
> >  	pci_set_drvdata(dev, NULL);
> > --
> > 2.7.4

Hi Jingjing,

This patch perfectly meets requirements as both resets are retained
(open and release). Tested it with LiquidIO NIC and it works fine.
I can see MSI-X re-enabled on each run with new patch.

Gregory, Harish,
Please verify the patch on your setup if possible.

Thanks,
Shijith

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Ferruh Yigit]

On 10/13/2017 9:12 AM, Shijith Thotton wrote:
<...>

> Hi Jingjing,
> 
> This patch perfectly meets requirements as both resets are retained
> (open and release). Tested it with LiquidIO NIC and it works fine.
> I can see MSI-X re-enabled on each run with new patch.
> 
> Gregory, Harish,
> Please verify the patch on your setup if possible.

Hi Gregory, Harish,

Did you able to test this patch?

Thanks,
ferruh

> 
> Thanks,
> Shijith
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Patil, Harish]

-----Original Message-----
From: dev <dev-bounces@dpdk.org> on behalf of Ferruh Yigit
<ferruh.yigit@intel.com>
Date: Friday, October 13, 2017 at 2:11 PM
To: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"
<jingjing.wu@intel.com>, Harish Patil <Harish.Patil@cavium.com>
Cc: "Tan, Jianfeng" <jianfeng.tan@intel.com>, "gregory@weka.io"
<gregory@weka.io>, "Xing, Beilei" <beilei.xing@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 2/2] igb_uio: fix interrupt
enablement	after FLR in VM

>On 10/13/2017 9:12 AM, Shijith Thotton wrote:
><...>
>
>> Hi Jingjing,
>> 
>> This patch perfectly meets requirements as both resets are retained
>> (open and release). Tested it with LiquidIO NIC and it works fine.
>> I can see MSI-X re-enabled on each run with new patch.
>> 
>> Gregory, Harish,
>> Please verify the patch on your setup if possible.
>
>Hi Gregory, Harish,
>
>Did you able to test this patch?
>
>Thanks,
>Ferruh

[Harish] No, I haven’t.
BTW, the igb_uio change also caused PDA (physical device assignment) mode
failure, where the entire device is PCI passed through to VM). Earlier we
reported only for SR-IOV VF passthru scenario.
Thanks.

>
>> 
>> Thanks,
>> Shijith
>> 
>

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Gregory Etelson]

I'll start to build setup environment this week.
Regards,
Gregory

On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 10/13/2017 9:12 AM, Shijith Thotton wrote:
> <...>
>
> > Hi Jingjing,
> >
> > This patch perfectly meets requirements as both resets are retained
> > (open and release). Tested it with LiquidIO NIC and it works fine.
> > I can see MSI-X re-enabled on each run with new patch.
> >
> > Gregory, Harish,
> > Please verify the patch on your setup if possible.
>
> Hi Gregory, Harish,
>
> Did you able to test this patch?
>
> Thanks,
> ferruh
>
> >
> > Thanks,
> > Shijith
> >
>
>

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Patil, Harish]

From: Gregory Etelson <gregory@weka.io<mailto:gregory@weka.io>>
Date: Saturday, October 14, 2017 at 8:10 PM
To: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>
Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com<mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing" <jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>>, Harish Patil <Harish.Patil@cavium.com<mailto:Harish.Patil@cavium.com>>, "Tan, Jianfeng" <jianfeng.tan@intel.com<mailto:jianfeng.tan@intel.com>>, "Xing, Beilei" <beilei.xing@intel.com<mailto:beilei.xing@intel.com>>, "dev@dpdk.org<mailto:dev@dpdk.org>" <dev@dpdk.org<mailto:dev@dpdk.org>>, "stable@dpdk.org<mailto:stable@dpdk.org>" <stable@dpdk.org<mailto:stable@dpdk.org>>
Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

I'll start to build setup environment this week.
Regards,
Gregory

On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>> wrote:
On 10/13/2017 9:12 AM, Shijith Thotton wrote:
<...>

> Hi Jingjing,
>
> This patch perfectly meets requirements as both resets are retained
> (open and release). Tested it with LiquidIO NIC and it works fine.
> I can see MSI-X re-enabled on each run with new patch.
>
> Gregory, Harish,
> Please verify the patch on your setup if possible.

Hi Gregory, Harish,

Did you able to test this patch?

Thanks,
ferruh

>
> Thanks,
> Shijith
>


Hi all,
Its not working for qede VF device.
So request to back out all igb_uio changes related to the patch:
igb_uio: issue FLR during open and release of device file

Thanks,
Harish

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Ferruh Yigit]

On 10/16/2017 3:49 PM, Patil, Harish wrote:
> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>
> Date: Saturday, October 14, 2017 at 8:10 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>>
> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com
> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"
> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil
> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,
> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,
> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,
> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org
> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"
> <stable@dpdk.org <mailto:stable@dpdk.org>>
> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR
> in VM
> 
>     I'll start to build setup environment this week.
>     Regards,
>     Gregory
> 
>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit
>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:
>         <...>
> 
>         > Hi Jingjing,
>         >
>         > This patch perfectly meets requirements as both resets are retained
>         > (open and release). Tested it with LiquidIO NIC and it works fine.
>         > I can see MSI-X re-enabled on each run with new patch.
>         >
>         > Gregory, Harish,
>         > Please verify the patch on your setup if possible.
> 
>         Hi Gregory, Harish,
> 
>         Did you able to test this patch?
> 
>         Thanks,
>         ferruh
> 
>         >
>         > Thanks,
>         > Shijith
>         >
> 
> 
> 
> Hi all,
> Its not working for qede VF device.
> So request to back out all igb_uio changes related to the patch:
> igb_uio: issue FLR during open and release of device file 

Hi Harish,

Thanks for testing. We tried.

For this release, agreed to revert back to original state, I will send a
patch soon.


But for further investigation in next release, can you please share more
details? What is not working exactly, what is your setup, any
kernel/DPDK log to share?

Thanks,
ferruh

> 
> Thanks,
> Harish
> 

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Patil, Harish]

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com>
Date: Monday, October 16, 2017 at 4:52 PM
To: Harish Patil <Harish.Patil@cavium.com>, Gregory Etelson
<gregory@weka.io>
Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"
<jingjing.wu@intel.com>, "Tan, Jianfeng" <jianfeng.tan@intel.com>, "Xing,
Beilei" <beilei.xing@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in
VM

>On 10/16/2017 3:49 PM, Patil, Harish wrote:
>> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>
>> Date: Saturday, October 14, 2017 at 8:10 PM
>> To: Ferruh Yigit <ferruh.yigit@intel.com
>><mailto:ferruh.yigit@intel.com>>
>> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com
>> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"
>> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil
>> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,
>> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,
>> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,
>> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org
>> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"
>> <stable@dpdk.org <mailto:stable@dpdk.org>>
>> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR
>> in VM
>> 
>>     I'll start to build setup environment this week.
>>     Regards,
>>     Gregory
>> 
>>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit
>>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
>> 
>>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:
>>         <...>
>> 
>>         > Hi Jingjing,
>>         >
>>         > This patch perfectly meets requirements as both resets are
>>retained
>>         > (open and release). Tested it with LiquidIO NIC and it works
>>fine.
>>         > I can see MSI-X re-enabled on each run with new patch.
>>         >
>>         > Gregory, Harish,
>>         > Please verify the patch on your setup if possible.
>> 
>>         Hi Gregory, Harish,
>> 
>>         Did you able to test this patch?
>> 
>>         Thanks,
>>         ferruh
>> 
>>         >
>>         > Thanks,
>>         > Shijith
>>         >
>> 
>> 
>> 
>> Hi all,
>> Its not working for qede VF device.
>> So request to back out all igb_uio changes related to the patch:
>> igb_uio: issue FLR during open and release of device file
>
>Hi Harish,
>
>Thanks for testing. We tried.
>
>For this release, agreed to revert back to original state, I will send a
>patch soon.
>
>
>But for further investigation in next release, can you please share more
>details? What is not working exactly, what is your setup, any
>kernel/DPDK log to share?
>
>Thanks,
>Ferruh

Okay, sure.
thanks.
>
>> 
>> Thanks,
>> Harish
>> 
>

Re: [dpdk-stable] [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Wu, Jingjing]

> -----Original Message-----
> From: Patil, Harish [mailto:Harish.Patil@cavium.com]
> Sent: Tuesday, October 17, 2017 9:32 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Gregory Etelson <gregory@weka.io>
> Cc: Thotton, Shijith <Shijith.Thotton@cavium.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
> 
> 
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Date: Monday, October 16, 2017 at 4:52 PM
> To: Harish Patil <Harish.Patil@cavium.com>, Gregory Etelson
> <gregory@weka.io>
> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com>, "Wu, Jingjing"
> <jingjing.wu@intel.com>, "Tan, Jianfeng" <jianfeng.tan@intel.com>, "Xing,
> Beilei" <beilei.xing@intel.com>, "dev@dpdk.org" <dev@dpdk.org>,
> "stable@dpdk.org" <stable@dpdk.org>
> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in
> VM
> 
> >On 10/16/2017 3:49 PM, Patil, Harish wrote:
> >> From: Gregory Etelson <gregory@weka.io <mailto:gregory@weka.io>>
> >> Date: Saturday, October 14, 2017 at 8:10 PM
> >> To: Ferruh Yigit <ferruh.yigit@intel.com
> >><mailto:ferruh.yigit@intel.com>>
> >> Cc: "Thotton, Shijith" <Shijith.Thotton@cavium.com
> >> <mailto:Shijith.Thotton@cavium.com>>, "Wu, Jingjing"
> >> <jingjing.wu@intel.com <mailto:jingjing.wu@intel.com>>, Harish Patil
> >> <Harish.Patil@cavium.com <mailto:Harish.Patil@cavium.com>>, "Tan,
> >> Jianfeng" <jianfeng.tan@intel.com <mailto:jianfeng.tan@intel.com>>,
> >> "Xing, Beilei" <beilei.xing@intel.com <mailto:beilei.xing@intel.com>>,
> >> "dev@dpdk.org <mailto:dev@dpdk.org>" <dev@dpdk.org
> >> <mailto:dev@dpdk.org>>, "stable@dpdk.org <mailto:stable@dpdk.org>"
> >> <stable@dpdk.org <mailto:stable@dpdk.org>>
> >> Subject: Re: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR
> >> in VM
> >>
> >>     I'll start to build setup environment this week.
> >>     Regards,
> >>     Gregory
> >>
> >>     On Sat, Oct 14, 2017 at 12:11 AM, Ferruh Yigit
> >>     <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> >>
> >>         On 10/13/2017 9:12 AM, Shijith Thotton wrote:
> >>         <...>
> >>
> >>         > Hi Jingjing,
> >>         >
> >>         > This patch perfectly meets requirements as both resets are
> >>retained
> >>         > (open and release). Tested it with LiquidIO NIC and it works
> >>fine.
> >>         > I can see MSI-X re-enabled on each run with new patch.
> >>         >
> >>         > Gregory, Harish,
> >>         > Please verify the patch on your setup if possible.
> >>
> >>         Hi Gregory, Harish,
> >>
> >>         Did you able to test this patch?
> >>
> >>         Thanks,
> >>         ferruh
> >>
> >>         >
> >>         > Thanks,
> >>         > Shijith
> >>         >
> >>
> >>
> >>
> >> Hi all,
> >> Its not working for qede VF device.
> >> So request to back out all igb_uio changes related to the patch:
> >> igb_uio: issue FLR during open and release of device file
> >
> >Hi Harish,
> >
> >Thanks for testing. We tried.
> >
> >For this release, agreed to revert back to original state, I will send a
> >patch soon.
> >
> >
> >But for further investigation in next release, can you please share more
> >details? What is not working exactly, what is your setup, any
> >kernel/DPDK log to share?
> >
> >Thanks,
> >Ferruh
> 
> Okay, sure.
> thanks.
> >
> >>
> >> Thanks,
> >> Harish
> >>
> >
Can you share more details? Only doesn't work in vfio pci passthrough?

Re: [dpdk-stable] [PATCH 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Ferruh Yigit]

On 10/9/2017 9:31 PM, Jingjing Wu wrote:
> If pass-through a VF by vfio-pci to a Qemu VM, after FLR
> in VM, the interrupt setting is not recoverd correctly
> to host as below:
>  in VM guest:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
>  in Host:
>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> 
> That was because in pci_reset_function, it first reads the
> PCI configure and set FLR reset, and then writes PCI configure
> as restoration. But not all the writing are successful to Host.
> Becuase vfio-pci driver doesn't allow directly write PCI MSI-X
> Cap.
> 
> To fix this issue, we need to move the interrupt enablement from
> igb_uio probe to open device file. While is also the similar as
> the behaviour in vfio_pci kernel module code.

So I guess this also explains why pci_reset in open() cause the problem,
when this is called for VF, interrupts stays disable for both VF and PF?

> 
> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>


We have two options, getting this patch or revert the original patch,
Thomas already has a patch for reverting.

The original patch is to make igb_uio safer. To not leave device in
unwanted stated. Problem related to this has been reported a few times,
I believe it is good to fix this problem. And since we already have some
movement towards fix, I say lets continue instead of revert.

Only problem is the scope of the patch is wide, and in previous release
it already break some uses cases, this is a little scary, please support
on testing this more.

I suggest getting this fix for rc1 and let it to be tested properly, and
if we hit some problem, we can always revert and work on problem for
next release.

Thanks,
ferruh

Re: [dpdk-stable] [PATCH 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Thomas Monjalon]

13/10/2017 22:54, Ferruh Yigit:
> On 10/9/2017 9:31 PM, Jingjing Wu wrote:
> > If pass-through a VF by vfio-pci to a Qemu VM, after FLR
> > in VM, the interrupt setting is not recoverd correctly
> > to host as below:
> >  in VM guest:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> >  in Host:
> >         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
> > 
> > That was because in pci_reset_function, it first reads the
> > PCI configure and set FLR reset, and then writes PCI configure
> > as restoration. But not all the writing are successful to Host.
> > Becuase vfio-pci driver doesn't allow directly write PCI MSI-X
> > Cap.
> > 
> > To fix this issue, we need to move the interrupt enablement from
> > igb_uio probe to open device file. While is also the similar as
> > the behaviour in vfio_pci kernel module code.
> 
> So I guess this also explains why pci_reset in open() cause the problem,
> when this is called for VF, interrupts stays disable for both VF and PF?
> 
> > 
> > Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
> > 
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> 
> We have two options, getting this patch or revert the original patch,
> Thomas already has a patch for reverting.
> 
> The original patch is to make igb_uio safer. To not leave device in
> unwanted stated. Problem related to this has been reported a few times,
> I believe it is good to fix this problem. And since we already have some
> movement towards fix, I say lets continue instead of revert.
> 
> Only problem is the scope of the patch is wide, and in previous release
> it already break some uses cases, this is a little scary, please support
> on testing this more.
> 
> I suggest getting this fix for rc1 and let it to be tested properly, and
> if we hit some problem, we can always revert and work on problem for
> next release.

OK, let's try.

Harish, please help testing this patch with qede.

Thanks

Re: [dpdk-stable] [PATCH 2/2] igb_uio: fix interrupt enablement after FLR in VM

[Ferruh Yigit]

On 10/13/2017 9:54 PM, Ferruh Yigit wrote:
> On 10/9/2017 9:31 PM, Jingjing Wu wrote:
>> If pass-through a VF by vfio-pci to a Qemu VM, after FLR
>> in VM, the interrupt setting is not recoverd correctly
>> to host as below:
>>  in VM guest:
>>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
>>  in Host:
>>         Capabilities: [70] MSI-X: Enable+ Count=5 Masked-
>>
>> That was because in pci_reset_function, it first reads the
>> PCI configure and set FLR reset, and then writes PCI configure
>> as restoration. But not all the writing are successful to Host.
>> Becuase vfio-pci driver doesn't allow directly write PCI MSI-X
>> Cap.
>>
>> To fix this issue, we need to move the interrupt enablement from
>> igb_uio probe to open device file. While is also the similar as
>> the behaviour in vfio_pci kernel module code.>>
>> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")
>>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk/master, thanks.

[dpdk-stable] [PATCH v2 1/2] net/i40e: fix VF initialization error

[Jingjing Wu]

In igb_uio, FLR is issued during open device file. i40evf is trying
to initialize admin queue when driver probe, while the FLR is not
done by host driver. That will cause initialization fail.

This patch is adding the checking if VF reset is done before
adimin queue initialization.

Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of device file")

Cc: stable@dpdk.org
Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
---
V2 change:
 remove useless code.

 drivers/net/i40e/i40e_ethdev_vf.c | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 111ac39..64e771c 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -1111,10 +1111,30 @@ i40evf_enable_irq0(struct i40e_hw *hw)
 }
 
 static int
-i40evf_reset_vf(struct i40e_hw *hw)
+i40evf_check_vf_reset_done(struct i40e_hw *hw)
 {
 	int i, reset;
 
+	for (i = 0; i < MAX_RESET_WAIT_CNT; i++) {
+		reset = I40E_READ_REG(hw, I40E_VFGEN_RSTAT) &
+			I40E_VFGEN_RSTAT_VFR_STATE_MASK;
+		reset = reset >> I40E_VFGEN_RSTAT_VFR_STATE_SHIFT;
+		if (reset == VIRTCHNL_VFR_VFACTIVE ||
+		    reset == VIRTCHNL_VFR_COMPLETED)
+			break;
+		rte_delay_ms(50);
+	}
+
+	if (i >= MAX_RESET_WAIT_CNT)
+		return -1;
+
+	return 0;
+}
+static int
+i40evf_reset_vf(struct i40e_hw *hw)
+{
+	int ret;
+
 	if (i40e_vf_reset(hw) != I40E_SUCCESS) {
 		PMD_INIT_LOG(ERR, "Reset VF NIC failed");
 		return -1;
@@ -1130,19 +1150,10 @@ i40evf_reset_vf(struct i40e_hw *hw)
 	  */
 	rte_delay_ms(200);
 
-	for (i = 0; i < MAX_RESET_WAIT_CNT; i++) {
-		reset = rd32(hw, I40E_VFGEN_RSTAT) &
-			I40E_VFGEN_RSTAT_VFR_STATE_MASK;
-		reset = reset >> I40E_VFGEN_RSTAT_VFR_STATE_SHIFT;
-		if (VIRTCHNL_VFR_COMPLETED == reset || VIRTCHNL_VFR_VFACTIVE == reset)
-			break;
-		else
-			rte_delay_ms(50);
-	}
-
-	if (i >= MAX_RESET_WAIT_CNT) {
-		PMD_INIT_LOG(ERR, "Reset VF NIC failed");
-		return -1;
+	ret = i40evf_check_vf_reset_done(hw);
+	if (ret) {
+		PMD_INIT_LOG(ERR, "VF is still resetting");
+		return ret;
 	}
 
 	return 0;
@@ -1165,6 +1176,10 @@ i40evf_init_vf(struct rte_eth_dev *dev)
 		goto err;
 	}
 
+	err = i40evf_check_vf_reset_done(hw);
+	if (err)
+		goto err;
+
 	i40e_init_adminq_parameter(hw);
 	err = i40e_init_adminq(hw);
 	if (err) {
@@ -1189,6 +1204,7 @@ i40evf_init_vf(struct rte_eth_dev *dev)
 		PMD_INIT_LOG(ERR, "init_adminq failed");
 		goto err;
 	}
+
 	vf->aq_resp = rte_zmalloc("vf_aq_resp", I40E_AQ_BUF_SZ, 0);
 	if (!vf->aq_resp) {
 		PMD_INIT_LOG(ERR, "unable to allocate vf_aq_resp memory");
-- 
2.7.4