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

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

[Jianfeng Tan]

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>
---
 drivers/net/virtio/virtio_ethdev.c | 27 ++++++++++++++++-----------
 drivers/net/virtio/virtio_pci.c    | 13 +++++++------
 drivers/net/virtio/virtio_pci.h    |  3 ++-
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 63a368a..b144a58 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -59,6 +59,7 @@
 #include "virtqueue.h"
 #include "virtio_rxtx.h"
 
+#define VIRTIO_DRV_FLAGS	RTE_PCI_DRV_DETACHABLE
 
 static int eth_virtio_dev_init(struct rte_eth_dev *eth_dev);
 static int eth_virtio_dev_uninit(struct rte_eth_dev *eth_dev);
@@ -491,7 +492,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 +499,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_PCI_DRV_INTR_LSC)
 		vtpci_irq_config(hw, VIRTIO_MSI_NO_VECTOR);
 	vtpci_reset(hw);
 	virtio_dev_free_mbufs(dev);
@@ -1034,6 +1034,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 = VIRTIO_DRV_FLAGS;
 	int ret;
 
 	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr));
@@ -1057,7 +1058,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 +1075,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_PCI_DRV_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 +1162,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_PCI_DRV_INTR_LSC)
 		rte_intr_callback_register(&pci_dev->intr_handle,
 				   virtio_interrupt_handler, eth_dev);
 
@@ -1190,7 +1197,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_PCI_DRV_INTR_LSC)
 		rte_intr_callback_unregister(&pci_dev->intr_handle,
 						virtio_interrupt_handler,
 						eth_dev);
@@ -1205,7 +1212,7 @@ static struct eth_driver rte_virtio_pmd = {
 	.pci_drv = {
 		.name = "rte_virtio_pmd",
 		.id_table = pci_id_virtio_map,
-		.drv_flags = RTE_PCI_DRV_DETACHABLE,
+		.drv_flags = VIRTIO_DRV_FLAGS,
 	},
 	.eth_dev_init = eth_virtio_dev_init,
 	.eth_dev_uninit = eth_virtio_dev_uninit,
@@ -1240,7 +1247,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 +1264,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_PCI_DRV_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 +1279,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_PCI_DRV_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..78afe94 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_PCI_DRV_INTR_LSC;
 	else
-		pci_dev->driver->drv_flags &= ~RTE_PCI_DRV_INTR_LSC;
+		*dev_flags &= ~RTE_PCI_DRV_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_PCI_DRV_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 *);
-- 
2.1.4

Re: [dpdk-dev] [PATCH] virtio: fix modify drv_flags for specific device

[David Marchand]

On Tue, Apr 26, 2016 at 4:24 AM, 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.
>
> 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>

- ethdev dev_flags is supposed to be filled with
RTE_ETH_DEV_DETACHABLE, RTE_ETH_DEV_INTR_LSC etc... not pci macros.

- I would have kept the init code as it is until the
rte_eth_copy_pci_info() step, then sanitise the dev_flags, but this
might be a matter of taste.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] virtio: fix modify drv_flags for specific device

[Tan, Jianfeng]

Hi,

On 4/26/2016 7:53 PM, David Marchand wrote:
> On Tue, Apr 26, 2016 at 4:24 AM, 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.
>>
>> 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>
> - ethdev dev_flags is supposed to be filled with
> RTE_ETH_DEV_DETACHABLE, RTE_ETH_DEV_INTR_LSC etc... not pci macros.

Oops, I'll correct it in next version.

>
> - I would have kept the init code as it is until the
> rte_eth_copy_pci_info() step, then sanitise the dev_flags, but this
> might be a matter of taste.
>
Yes, if we keep that until rte_eth_copy_pci_info() step, the drv_flags 
has already been changed.

Besides, I see bnx2x pmd could have similar issue, and the QLogic bnx2x 
maintainers are CCed to confirm.

Thanks,
Jianfeng

Re: [dpdk-dev] [PATCH] virtio: fix modify drv_flags for specific device

[David Marchand]

Hello,

On Wed, Apr 27, 2016 at 7:10 AM, Tan, Jianfeng <jianfeng.tan@intel.com> wrote:
> Besides, I see bnx2x pmd could have similar issue, and the QLogic bnx2x
> maintainers are CCed to confirm.

Well, a way to detect those kind of issues would be to constify drv_flags.


-- 
David Marchand

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

[Jianfeng Tan]

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(-)

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 *);
-- 
2.1.4

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

[Yuanhan Liu]

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

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

[David Marchand]

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>

-- 
David Marchand

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

[Yuanhan Liu]

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

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

[Tan, Jianfeng]

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

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

[Yuanhan Liu]

On Mon, May 09, 2016 at 09:14:41AM +0000, Tan, Jianfeng wrote:
>     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.

Yes, that's the issue; and that's exactly something I was looking for in
a commit log.

Patch is applied to dpdk-next-virtio, with following changes:

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 1864186..995618a 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1076,11 +1076,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 		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);
---

Without this patch as the context, mentioning "drv_flags" here confuses
people.

And FYI, I made folloiwng commit log rewords.

Thanks.

	--yliu

 ---
 virtio: fix overwritten drv_flags

 The "drv_flags" is set with device as the input, which means different
 device (say, modern vs legacy) could end up with a different value. And
 the fact that "drv_flags" is shared by all devices means that every time
 we add a new device, it simply overwrites the value configured from the
 last device.

 Therefore, when two virtio devices have different flags, it may lead to
 wrong result, such as virtio would set irq config when it's not supported.

 Making the flag per device (using "dev->data->dev_flags") could let us
 have different value for each device, which would avoid the above issue.