[dpdk-dev] [PATCH v2] virtio: fix modify drv_flags for specific device
On Mon, May 09, 2016 at 09:14:41AM +, 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.
[dpdk-dev] [PATCH v2] virtio: fix modify drv_flags for specific device
Hi David and Yuanhan, > -Original Message- > From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com] > Sent: Thursday, May 5, 2016 7:18 AM > To: Tan, Jianfeng > Cc: David Marchand; dev at 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 > 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 > > 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
[dpdk-dev] [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 > 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 Thanks for the review. --yliu
[dpdk-dev] [PATCH v2] virtio: fix modify drv_flags for specific device
Hello Tan, On Thu, Apr 28, 2016 at 8:08 PM, 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. 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
[dpdk-dev] [PATCH v2] virtio: fix modify drv_flags for specific device
On Thu, Apr 28, 2016 at 06:08:59PM +, 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
> Suggested-by: David Marchand
Hi David,
May I ask your review on this and give ACK when no issue to you?
Thanks.
--yliu
[dpdk-dev] [PATCH v2] virtio: fix modify drv_flags for specific device
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
Suggested-by: David Marchand
Signed-off-by: Jianfeng Tan
---
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
