[dpdk-dev,v2] virtio: fix modify drv_flags for specific device

Message ID 1461866939-38689-1-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Commit Message

Jianfeng Tan April 28, 2016, 6:08 p.m. UTC
  Issue: virtio's drv_flags are decided by devices types (modern vs legacy),
and which kernel driver is used, and the negotiated features (especially
VIRTIO_NET_STATUS) with backend, which makes it possible to multiple
virtio devices have different versions of drv_flags, but this variable
is currently shared by each virtio device.

How to fix: dev_flags is a device-specific variable to store this info.

Fixes: da978dfdc43 ("virtio: use port IO to get PCI resource")

Reported-by: David Marchand <david.marchand@6wind.com>
Suggested-by: David Marchand <david.marchand@6wind.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 v2: RTE_PCI_DRV_INTR_LSC -> RTE_ETH_DEV_INTR_LSC.

 drivers/net/virtio/virtio_ethdev.c | 25 ++++++++++++++-----------
 drivers/net/virtio/virtio_pci.c    | 13 +++++++------
 drivers/net/virtio/virtio_pci.h    |  3 ++-
 3 files changed, 23 insertions(+), 18 deletions(-)
  

Comments

Yuanhan Liu May 2, 2016, 5:55 p.m. UTC | #1
On Thu, Apr 28, 2016 at 06:08:59PM +0000, Jianfeng Tan wrote:
> Issue: virtio's drv_flags are decided by devices types (modern vs legacy),
> and which kernel driver is used, and the negotiated features (especially
> VIRTIO_NET_STATUS) with backend, which makes it possible to multiple
> virtio devices have different versions of drv_flags, but this variable
> is currently shared by each virtio device.
> 
> How to fix: dev_flags is a device-specific variable to store this info.
> 
> Fixes: da978dfdc43 ("virtio: use port IO to get PCI resource")
> 
> Reported-by: David Marchand <david.marchand@6wind.com>
> Suggested-by: David Marchand <david.marchand@6wind.com>

Hi David,

May I ask your review on this and give ACK when no issue to you?

Thanks.

	--yliu
  
David Marchand May 3, 2016, 8:05 a.m. UTC | #2
Hello Tan,

On Thu, Apr 28, 2016 at 8:08 PM, Jianfeng Tan <jianfeng.tan@intel.com> wrote:
> Issue: virtio's drv_flags are decided by devices types (modern vs legacy),
> and which kernel driver is used, and the negotiated features (especially
> VIRTIO_NET_STATUS) with backend, which makes it possible to multiple
> virtio devices have different versions of drv_flags, but this variable
> is currently shared by each virtio device.

The wording is a bit strange, maybe the sentence is a bit too long.
But the rest looks good to me.

Acked-by: David Marchand <david.marchand@6wind.com>
  
Yuanhan Liu May 4, 2016, 11:18 p.m. UTC | #3
On Tue, May 03, 2016 at 10:05:01AM +0200, David Marchand wrote:
> Hello Tan,
> 
> On Thu, Apr 28, 2016 at 8:08 PM, Jianfeng Tan <jianfeng.tan@intel.com> wrote:
> > Issue: virtio's drv_flags are decided by devices types (modern vs legacy),
> > and which kernel driver is used, and the negotiated features (especially
> > VIRTIO_NET_STATUS) with backend, which makes it possible to multiple
> > virtio devices have different versions of drv_flags, but this variable
> > is currently shared by each virtio device.
> 
> The wording is a bit strange, maybe the sentence is a bit too long.

Agreed.

Besides that, it just described the fact that we are sharing one
flag for all virtio devices, but it didn't state what's wrong with
it, and what's the per-device flag for. From this point of view,
I don't think you are actually solving an "issue", as I don't see
one from your description.

> But the rest looks good to me.
> 
> Acked-by: David Marchand <david.marchand@6wind.com>

Thanks for the review.

	--yliu
  
Jianfeng Tan May 9, 2016, 9:14 a.m. UTC | #4
Hi David and Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Thursday, May 5, 2016 7:18 AM
> To: Tan, Jianfeng
> Cc: David Marchand; dev@dpdk.org; Xie, Huawei
> Subject: Re: [PATCH v2] virtio: fix modify drv_flags for specific device
> 
> On Tue, May 03, 2016 at 10:05:01AM +0200, David Marchand wrote:
> > Hello Tan,
> >
> > On Thu, Apr 28, 2016 at 8:08 PM, Jianfeng Tan <jianfeng.tan@intel.com>
> wrote:
> > > Issue: virtio's drv_flags are decided by devices types (modern vs legacy),
> > > and which kernel driver is used, and the negotiated features (especially
> > > VIRTIO_NET_STATUS) with backend, which makes it possible to multiple
> > > virtio devices have different versions of drv_flags, but this variable
> > > is currently shared by each virtio device.
> >
> > The wording is a bit strange, maybe the sentence is a bit too long.
> 
> Agreed.
> 
> Besides that, it just described the fact that we are sharing one
> flag for all virtio devices, but it didn't state what's wrong with
> it, and what's the per-device flag for. From this point of view,
> I don't think you are actually solving an "issue", as I don't see
> one from your description.
> 
> > But the rest looks good to me.
> >
> > Acked-by: David Marchand <david.marchand@6wind.com>
> 
> Thanks for the review.
> 
> 	--yliu

Thank you for review and comment. How about change the commit message like this:

   The virtio PMD's drv_flags, which is shared by all virtio devices,
    is set and referred for per-device purpose. One side, we should not
    arbitrarily set drv_flags when each virtio device is initialized.
    This disobeys the semantics of _shared_. On the other side, we
    refer drv_flags instead of dev_flags for per-device purpose. When
    two virtio devices have different flags, it may lead to wrong
    result. Then some unexpected behaviors happen, such as virtio would
    set irq config when it's not supported.
    
    How to fix: change to set and refer dev_flags, which stores
    device-specific flags.

Thanks,
Jianfeng
  

Patch

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 63a368a..1fe90ae 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -59,7 +59,6 @@ 
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
 
-
 static int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
 static int eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev);
 static int  virtio_dev_configure(struct rte_eth_dev *dev);
@@ -491,7 +490,6 @@  static void
 virtio_dev_close(struct rte_eth_dev *dev)
 {
 	struct virtio_hw *hw = dev->data->dev_private;
-	struct rte_pci_device *pci_dev = dev->pci_dev;
 
 	PMD_INIT_LOG(DEBUG, "virtio_dev_close");
 
@@ -499,7 +497,7 @@  virtio_dev_close(struct rte_eth_dev *dev)
 		virtio_dev_stop(dev);
 
 	/* reset the NIC */
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		vtpci_irq_config(hw, VIRTIO_MSI_NO_VECTOR);
 	vtpci_reset(hw);
 	virtio_dev_free_mbufs(dev);
@@ -1034,6 +1032,7 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	struct virtio_net_config *config;
 	struct virtio_net_config local_config;
 	struct rte_pci_device *pci_dev;
+	uint32_t dev_flags = RTE_ETH_DEV_DETACHABLE;
 	int ret;
 
 	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr));
@@ -1057,7 +1056,7 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 
 	pci_dev = eth_dev->pci_dev;
 
-	ret = vtpci_init(pci_dev, hw);
+	ret = vtpci_init(pci_dev, hw, &dev_flags);
 	if (ret)
 		return ret;
 
@@ -1074,9 +1073,15 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 
 	/* If host does not support status then disable LSC */
 	if (!vtpci_with_feature(hw, VIRTIO_NET_F_STATUS))
-		pci_dev->driver->drv_flags &= ~RTE_PCI_DRV_INTR_LSC;
+		dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
 
 	rte_eth_copy_pci_info(eth_dev, pci_dev);
+	/* For virtio devices, dev_flags are decided according to feature
+	 * negotiation, aka if VIRTIO_NET_F_STATUS is set, and which kernel
+	 * driver is used, dynamically. And we should keep drv_flags shared
+	 * and unvaried.
+	 */
+	eth_dev->data->dev_flags = dev_flags;
 
 	rx_func_get(eth_dev);
 
@@ -1155,7 +1160,7 @@  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 			pci_dev->id.device_id);
 
 	/* Setup interrupt callback  */
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		rte_intr_callback_register(&pci_dev->intr_handle,
 				   virtio_interrupt_handler, eth_dev);
 
@@ -1190,7 +1195,7 @@  eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev)
 	eth_dev->data->mac_addrs = NULL;
 
 	/* reset interrupt callback  */
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+	if (eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		rte_intr_callback_unregister(&pci_dev->intr_handle,
 						virtio_interrupt_handler,
 						eth_dev);
@@ -1240,7 +1245,6 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
 	struct virtio_hw *hw = dev->data->dev_private;
-	struct rte_pci_device *pci_dev = dev->pci_dev;
 
 	PMD_INIT_LOG(DEBUG, "configure");
 
@@ -1258,7 +1262,7 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 		return -ENOTSUP;
 	}
 
-	if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)
+	if (dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 		if (vtpci_irq_config(hw, 0) == VIRTIO_MSI_NO_VECTOR) {
 			PMD_DRV_LOG(ERR, "failed to set config vector");
 			return -EBUSY;
@@ -1273,11 +1277,10 @@  virtio_dev_start(struct rte_eth_dev *dev)
 {
 	uint16_t nb_queues, i;
 	struct virtio_hw *hw = dev->data->dev_private;
-	struct rte_pci_device *pci_dev = dev->pci_dev;
 
 	/* check if lsc interrupt feature is enabled */
 	if (dev->data->dev_conf.intr_conf.lsc) {
-		if (!(pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC)) {
+		if (!(dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)) {
 			PMD_DRV_LOG(ERR, "link status not supported by host");
 			return -ENOTSUP;
 		}
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index c007959..9cdca06 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -199,15 +199,15 @@  legacy_virtio_has_msix(const struct rte_pci_addr *loc __rte_unused)
 
 static int
 legacy_virtio_resource_init(struct rte_pci_device *pci_dev,
-			    struct virtio_hw *hw)
+			    struct virtio_hw *hw, uint32_t *dev_flags)
 {
 	if (rte_eal_pci_ioport_map(pci_dev, 0, &hw->io) < 0)
 		return -1;
 
 	if (pci_dev->intr_handle.type != RTE_INTR_HANDLE_UNKNOWN)
-		pci_dev->driver->drv_flags |= RTE_PCI_DRV_INTR_LSC;
+		*dev_flags |= RTE_ETH_DEV_INTR_LSC;
 	else
-		pci_dev->driver->drv_flags &= ~RTE_PCI_DRV_INTR_LSC;
+		*dev_flags &= ~RTE_ETH_DEV_INTR_LSC;
 
 	return 0;
 }
@@ -630,7 +630,8 @@  next:
  * Return 0 on success.
  */
 int
-vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
+vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw,
+	   uint32_t *dev_flags)
 {
 	hw->dev = dev;
 
@@ -643,12 +644,12 @@  vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw)
 		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
 		hw->vtpci_ops = &modern_ops;
 		hw->modern    = 1;
-		dev->driver->drv_flags |= RTE_PCI_DRV_INTR_LSC;
+		*dev_flags |= RTE_ETH_DEV_INTR_LSC;
 		return 0;
 	}
 
 	PMD_INIT_LOG(INFO, "trying with legacy virtio pci.");
-	if (legacy_virtio_resource_init(dev, hw) < 0) {
+	if (legacy_virtio_resource_init(dev, hw, dev_flags) < 0) {
 		if (dev->kdrv == RTE_KDRV_UNKNOWN &&
 		    dev->devargs->type != RTE_DEVTYPE_WHITELISTED_PCI) {
 			PMD_INIT_LOG(INFO,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index b69785e..554efea 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -293,7 +293,8 @@  vtpci_with_feature(struct virtio_hw *hw, uint64_t bit)
 /*
  * Function declaration from virtio_pci.c
  */
-int vtpci_init(struct rte_pci_device *, struct virtio_hw *);
+int vtpci_init(struct rte_pci_device *, struct virtio_hw *,
+	       uint32_t *dev_flags);
 void vtpci_reset(struct virtio_hw *);
 
 void vtpci_reinit_complete(struct virtio_hw *);