Tiwei Bie (6): net/virtio-user: do not stop stopped device again net/virtio-user: do not make vhost user channel nonblock net/virtio-user: do not reset owner when driver resets net/virtio-user: fix device features for server mode net/virtio-user: simplify device features preparation net/virtio: fix guest announce support drivers/net/virtio/virtio_ethdev.c | 16 +++-- .../net/virtio/virtio_user/virtio_user_dev.c | 65 ++++++++++--------- .../net/virtio/virtio_user/virtio_user_dev.h | 1 + drivers/net/virtio/virtio_user_ethdev.c | 6 +- 4 files changed, 48 insertions(+), 40 deletions(-) -- 2.19.1
Without this change, virtio-user still works, but it will show annoying error messages like this on shutdown: vhost_kernel_set_backend(): VHOST_NET_SET_BACKEND fails, Operation not permitted vhost_kernel_ioctl(): VHOST_RESET_OWNER failed: Operation not permitted Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost") Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug") Cc: stable@dpdk.org Reported-by: Stephen Hemminger <stephen@networkplumber.org> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index a185aed34..684702c56 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -184,6 +184,9 @@ int virtio_user_stop_device(struct virtio_user_dev *dev) uint32_t i; pthread_mutex_lock(&dev->mutex); + if (!dev->started) + goto out; + for (i = 0; i < dev->max_queue_pairs; ++i) dev->ops->enable_qp(dev, i, 0); @@ -193,6 +196,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev) return -1; } dev->started = false; +out: pthread_mutex_unlock(&dev->mutex); return 0; -- 2.19.1
There is no need to make the vhost user channel nonblock, and making it nonblock will make vhost_user_read() fail with EAGAIN when vhost messages need a reply. Fixes: bd8f50a45d0f ("net/virtio-user: support server mode") Cc: stable@dpdk.org Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- drivers/net/virtio/virtio_user_ethdev.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index b51cbc85b..2100e43db 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -28,7 +28,6 @@ static int virtio_user_server_reconnect(struct virtio_user_dev *dev) { int ret; - int flag; int connectfd; struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id]; @@ -49,9 +48,6 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev) dev->features &= dev->device_features; - flag = fcntl(connectfd, F_GETFD); - fcntl(connectfd, F_SETFL, flag | O_NONBLOCK); - ret = virtio_user_start_device(dev); if (ret < 0) return -1; -- 2.19.1
When driver resets the device, virtio-user just needs to send GET_VRING_BASE messages to stop the vhost backend, and that's what QEMU does. With this change, we won't need to set owner when starting virtio-user device anymore. This will help us to get rid of below error message on startup: vhost_kernel_ioctl(): VHOST_SET_OWNER failed: Device or resource busy Fixes: bce7e9050f9b ("net/virtio-user: fix start with kernel vhost") Fixes: 0d6a8752ac9d ("net/virtio-user: fix crash as features change") Cc: stable@dpdk.org Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- .../net/virtio/virtio_user/virtio_user_dev.c | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 684702c56..be70414a1 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -134,9 +134,6 @@ virtio_user_start_device(struct virtio_user_dev *dev) if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0) goto error; - /* Do not check return as already done in init, or reset in stop */ - dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL); - /* Step 0: tell vhost to create queues */ if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0) goto error; @@ -181,7 +178,9 @@ virtio_user_start_device(struct virtio_user_dev *dev) int virtio_user_stop_device(struct virtio_user_dev *dev) { + struct vhost_vring_state state; uint32_t i; + int error = 0; pthread_mutex_lock(&dev->mutex); if (!dev->started) @@ -190,16 +189,23 @@ int virtio_user_stop_device(struct virtio_user_dev *dev) for (i = 0; i < dev->max_queue_pairs; ++i) dev->ops->enable_qp(dev, i, 0); - if (dev->ops->send_request(dev, VHOST_USER_RESET_OWNER, NULL) < 0) { - PMD_DRV_LOG(INFO, "Failed to reset the device\n"); - pthread_mutex_unlock(&dev->mutex); - return -1; + /* Stop the backend. */ + for (i = 0; i < dev->max_queue_pairs * 2; ++i) { + state.index = i; + if (dev->ops->send_request(dev, VHOST_USER_GET_VRING_BASE, + &state) < 0) { + PMD_DRV_LOG(ERR, "get_vring_base failed, index=%u\n", + i); + error = -1; + goto out; + } } + dev->started = false; out: pthread_mutex_unlock(&dev->mutex); - return 0; + return error; } static inline void -- 2.19.1
We need to save the supported frontend features (which won't be announced by vhost backend), otherwise we will lost them when the connection to vhost-user backend is established in server mode. Fixes: 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode") Cc: stable@dpdk.org Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++--- drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 + drivers/net/virtio/virtio_user_ethdev.c | 2 ++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index be70414a1..44e093eb7 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -421,6 +421,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, dev->queue_pairs = 1; /* mq disabled by default */ dev->queue_size = queue_size; dev->mac_specified = 0; + dev->frontend_features = 0; dev->unsupported_features = 0; parse_mac(dev, mac); @@ -468,7 +469,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, } if (dev->mac_specified) { - dev->device_features |= (1ull << VIRTIO_NET_F_MAC); + dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC); } else { dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC); dev->unsupported_features |= (1ull << VIRTIO_NET_F_MAC); @@ -478,7 +479,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, /* device does not really need to know anything about CQ, * so if necessary, we just claim to support CQ */ - dev->device_features |= (1ull << VIRTIO_NET_F_CTRL_VQ); + dev->frontend_features |= (1ull << VIRTIO_NET_F_CTRL_VQ); } else { dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ); /* Also disable features depends on VIRTIO_NET_F_CTRL_VQ */ @@ -499,8 +500,9 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, /* The backend will not report this feature, we add it explicitly */ if (is_vhost_user_by_type(dev->path)) - dev->device_features |= (1ull << VIRTIO_NET_F_STATUS); + dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS); + dev->device_features |= dev->frontend_features; dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES; dev->unsupported_features |= ~VIRTIO_USER_SUPPORTED_FEATURES; diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index d6e0e137b..c42ce5d4b 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -33,6 +33,7 @@ struct virtio_user_dev { * and will be sync with device */ uint64_t device_features; /* supported features by device */ + uint64_t frontend_features; /* enabled frontend features */ uint64_t unsupported_features; /* unsupported features mask */ uint8_t status; uint16_t port_id; diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 2100e43db..c419d5b9d 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -43,6 +43,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev) return -1; } + dev->device_features |= dev->frontend_features; + /* umask vhost-user unsupported features */ dev->device_features &= ~(dev->unsupported_features); -- 2.19.1
Get rid of the duplicated code in device features preparation which looks awful. Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- .../net/virtio/virtio_user/virtio_user_dev.c | 31 +++++++------------ 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 44e093eb7..20abfcd19 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -422,7 +422,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, dev->queue_size = queue_size; dev->mac_specified = 0; dev->frontend_features = 0; - dev->unsupported_features = 0; + dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES; parse_mac(dev, mac); if (*ifname) { @@ -458,22 +458,16 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES; } - if (!mrg_rxbuf) { - dev->device_features &= ~(1ull << VIRTIO_NET_F_MRG_RXBUF); + if (!mrg_rxbuf) dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF); - } - if (!in_order) { - dev->device_features &= ~(1ull << VIRTIO_F_IN_ORDER); + if (!in_order) dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER); - } - if (dev->mac_specified) { + if (dev->mac_specified) dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC); - } else { - dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC); + else dev->unsupported_features |= (1ull << VIRTIO_NET_F_MAC); - } if (cq) { /* device does not really need to know anything about CQ, @@ -481,14 +475,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, */ dev->frontend_features |= (1ull << VIRTIO_NET_F_CTRL_VQ); } else { - dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ); - /* Also disable features depends on VIRTIO_NET_F_CTRL_VQ */ - dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_RX); - dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VLAN); - dev->device_features &= ~(1ull << VIRTIO_NET_F_GUEST_ANNOUNCE); - dev->device_features &= ~(1ull << VIRTIO_NET_F_MQ); - dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_MAC_ADDR); dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_VQ); + /* Also disable features that depend on VIRTIO_NET_F_CTRL_VQ */ dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_RX); dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_VLAN); dev->unsupported_features |= @@ -502,9 +490,12 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, if (is_vhost_user_by_type(dev->path)) dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS); + /* + * Device features = + * (frontend_features | backend_features) & ~unsupported_features; + */ dev->device_features |= dev->frontend_features; - dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES; - dev->unsupported_features |= ~VIRTIO_USER_SUPPORTED_FEATURES; + dev->device_features &= ~dev->unsupported_features; if (rte_mem_event_callback_register(VIRTIO_USER_MEM_EVENT_CLB_NAME, virtio_user_mem_event_cb, dev)) { -- 2.19.1
We need to check the status field in virtio net config structure instead of the bits read from ISR register to know whether we need to do guest announce. Fixes: 7365504f77e3 ("net/virtio: support guest announce") Cc: stable@dpdk.org Signed-off-by: Tiwei Bie <tiwei.bie@intel.com> --- drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index 10a7e3fcc..27088e3a6 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -1288,6 +1288,7 @@ virtio_interrupt_handler(void *param) struct rte_eth_dev *dev = param; struct virtio_hw *hw = dev->data->dev_private; uint8_t isr; + uint16_t status; /* Read interrupt status which clears interrupt */ isr = vtpci_isr(hw); @@ -1301,12 +1302,17 @@ virtio_interrupt_handler(void *param) _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_LSC, NULL); - } - if (isr & VIRTIO_NET_S_ANNOUNCE) { - virtio_notify_peers(dev); - if (hw->cvq) - virtio_ack_link_announce(dev); + if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) { + vtpci_read_dev_config(hw, + offsetof(struct virtio_net_config, status), + &status, sizeof(status)); + if (status & VIRTIO_NET_S_ANNOUNCE) { + virtio_notify_peers(dev); + if (hw->cvq) + virtio_ack_link_announce(dev); + } + } } } -- 2.19.1
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> Without this change, virtio-user still works, but it will show
> annoying error messages like this on shutdown:
>
> vhost_kernel_set_backend(): VHOST_NET_SET_BACKEND fails, Operation not permitted
> vhost_kernel_ioctl(): VHOST_RESET_OWNER failed: Operation not permitted
>
> Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost")
> Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
> Cc: stable@dpdk.org
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++
> 1 file changed, 4 insertions(+)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> There is no need to make the vhost user channel nonblock, and
> making it nonblock will make vhost_user_read() fail with EAGAIN
> when vhost messages need a reply.
>
> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/net/virtio/virtio_user_ethdev.c | 4 ----
> 1 file changed, 4 deletions(-)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> When driver resets the device, virtio-user just needs to send
> GET_VRING_BASE messages to stop the vhost backend, and that's
> what QEMU does. With this change, we won't need to set owner
> when starting virtio-user device anymore. This will help us to
> get rid of below error message on startup:
>
> vhost_kernel_ioctl(): VHOST_SET_OWNER failed: Device or resource busy
>
> Fixes: bce7e9050f9b ("net/virtio-user: fix start with kernel vhost")
> Fixes: 0d6a8752ac9d ("net/virtio-user: fix crash as features change")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> .../net/virtio/virtio_user/virtio_user_dev.c | 22 ++++++++++++-------
> 1 file changed, 14 insertions(+), 8 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> We need to save the supported frontend features (which won't be
> announced by vhost backend), otherwise we will lost them when the
> connection to vhost-user backend is established in server mode.
>
> Fixes: 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++---
> drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 2 ++
> 3 files changed, 8 insertions(+), 3 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> Get rid of the duplicated code in device features preparation
> which looks awful.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> .../net/virtio/virtio_user/virtio_user_dev.c | 31 +++++++------------
> 1 file changed, 11 insertions(+), 20 deletions(-)
Welcomed cleanup!
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> We need to check the status field in virtio net config structure
> instead of the bits read from ISR register to know whether we need
> to do guest announce.
>
> Fixes: 7365504f77e3 ("net/virtio: support guest announce")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
On 10/29/18 6:28 AM, Tiwei Bie wrote:
> Tiwei Bie (6):
> net/virtio-user: do not stop stopped device again
> net/virtio-user: do not make vhost user channel nonblock
> net/virtio-user: do not reset owner when driver resets
> net/virtio-user: fix device features for server mode
> net/virtio-user: simplify device features preparation
> net/virtio: fix guest announce support
>
> drivers/net/virtio/virtio_ethdev.c | 16 +++--
> .../net/virtio/virtio_user/virtio_user_dev.c | 65 ++++++++++---------
> .../net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 6 +-
> 4 files changed, 48 insertions(+), 40 deletions(-)
>
Applied to dpdk-next-virtio/master
Thanks,
Maxime