[dpdk-dev] 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>
---
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(-)
Comments
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.
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
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.
@@ -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;
}
@@ -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,
@@ -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 *);