[dpdk-dev,v2] virtio: fix modify drv_flags for specific device
Commit Message
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
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
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>
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
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
@@ -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;
}
@@ -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,
@@ -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 *);