From: Shijith Thotton <shijith.thotton@caviumnetworks.com>
To: "Wu, Jingjing" <jingjing.wu@intel.com>, hpatil@caviumnetworks.com
Cc: "Yigit, Ferruh" <ferruh.yigit@intel.com>,
"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: [PATCH v2 2/2] igb_uio: fix interrupt enablement after FLR in VM
Date: Fri, 13 Oct 2017 13:42:06 +0530 [thread overview]
Message-ID: <20171013081205.GA20678@localhost.localdomain> (raw)
In-Reply-To: <9BB6961774997848B5B42BEC655768F810E92434@SHSMSX103.ccr.corp.intel.com>
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
next prev parent reply other threads:[~2017-10-13 8:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-09 20:31 [PATCH 1/2] net/i40e: fix VF initialization error Jingjing Wu
2017-10-09 20:31 ` [PATCH 2/2] igb_uio: fix interrupt enablement after FLR in VM Jingjing Wu
2017-10-09 22:09 ` [PATCH v2 " Jingjing Wu
2017-10-12 5:43 ` Wu, Jingjing
2017-10-13 8:12 ` Shijith Thotton [this message]
2017-10-13 21:11 ` Ferruh Yigit
2017-10-13 21:21 ` Patil, Harish
2017-10-15 3:10 ` Gregory Etelson
2017-10-16 22:49 ` Patil, Harish
2017-10-16 23:52 ` Ferruh Yigit
2017-10-17 1:32 ` Patil, Harish
2017-10-17 1:37 ` Wu, Jingjing
2017-10-13 20:54 ` [PATCH " Ferruh Yigit
2017-10-13 21:15 ` [dpdk-stable] " Thomas Monjalon
2017-10-13 21:38 ` Ferruh Yigit
2017-10-09 22:08 ` [PATCH v2 1/2] net/i40e: fix VF initialization error Jingjing Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171013081205.GA20678@localhost.localdomain \
--to=shijith.thotton@caviumnetworks.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=gregory@weka.io \
--cc=hpatil@caviumnetworks.com \
--cc=jianfeng.tan@intel.com \
--cc=jingjing.wu@intel.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.