From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id A87D0A052A; Tue, 26 Jan 2021 11:23:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B44C9141422; Tue, 26 Jan 2021 11:18:36 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mails.dpdk.org (Postfix) with ESMTP id 97E2D141409 for ; Tue, 26 Jan 2021 11:18:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611656312; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4SRz9ueTC2/7EMwBBDCK2vWiSCbCDsghh7nTjqTFlkk=; b=WNSvzJrGbMYUownhh82pfqU0jBd5c70qEt3cNMxyouE2joPLGTxt2oSO4Hg8QonKh58mjV nifc6KfqWwKOzOJkTuVPNkBq5XNfCLNiQV+2S1s/sk8bM1ebeXv4RTZFLyyq4XJm7vLUlA onJqJJTovQz152z8gN/HN2qn2ImAEBM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-454-XGxdqv5MPjigAoMrB_7JJg-1; Tue, 26 Jan 2021 05:18:31 -0500 X-MC-Unique: XGxdqv5MPjigAoMrB_7JJg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 141F3B8100; Tue, 26 Jan 2021 10:18:30 +0000 (UTC) Received: from max-t490s.redhat.com (unknown [10.36.110.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id B13B86F45C; Tue, 26 Jan 2021 10:18:25 +0000 (UTC) From: Maxime Coquelin To: dev@dpdk.org, chenbo.xia@intel.com, olivier.matz@6wind.com, amorenoz@redhat.com, david.marchand@redhat.com Cc: Maxime Coquelin Date: Tue, 26 Jan 2021 11:16:32 +0100 Message-Id: <20210126101639.250481-38-maxime.coquelin@redhat.com> In-Reply-To: <20210126101639.250481-1-maxime.coquelin@redhat.com> References: <20210126101639.250481-1-maxime.coquelin@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" Subject: [dpdk-dev] [PATCH v4 37/44] net/virtio: make server mode blocking X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" This patch makes the Vhost-user backend server mode blocking at init, waiting for the client connection. The goal is to make the driver more reliable, as without waiting for client connection, the Virtio driver has to assume the Vhost-user backend will support all the features it has advertized, which could lead to undefined behaviour. For example, without this patch, if the user enables packed ring Virtio feature but the backend does not support it, the ring initialized by the driver will not be compatible with the backend. Signed-off-by: Maxime Coquelin Reviewed-by: Chenbo Xia --- drivers/net/virtio/virtio_user/vhost_user.c | 9 +- .../net/virtio/virtio_user/virtio_user_dev.c | 121 +++++++----------- drivers/net/virtio/virtio_user_ethdev.c | 5 - 3 files changed, 55 insertions(+), 80 deletions(-) diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c index ba02fdcb65..90fed6fe97 100644 --- a/drivers/net/virtio/virtio_user/vhost_user.c +++ b/drivers/net/virtio/virtio_user/vhost_user.c @@ -692,6 +692,14 @@ virtio_user_start_server(struct virtio_user_dev *dev, struct sockaddr_un *un) if (ret < 0) return -1; + PMD_DRV_LOG(NOTICE, "(%s) waiting for client connection...", dev->path); + dev->vhostfd = accept(fd, NULL, NULL); + if (dev->vhostfd < 0) { + PMD_DRV_LOG(ERR, "Failed to accept initial client connection (%s)", + strerror(errno)); + return -1; + } + flag = fcntl(fd, F_GETFL); if (fcntl(fd, F_SETFL, flag | O_NONBLOCK) < 0) { PMD_DRV_LOG(ERR, "fcntl failed, %s", strerror(errno)); @@ -736,7 +744,6 @@ vhost_user_setup(struct virtio_user_dev *dev) close(fd); return -1; } - dev->vhostfd = -1; } else { if (connect(fd, (struct sockaddr *)&un, sizeof(un)) < 0) { PMD_DRV_LOG(ERR, "connect error, %s", strerror(errno)); diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 95204ea955..c2a41fe3a0 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -144,10 +144,6 @@ virtio_user_dev_set_features(struct virtio_user_dev *dev) pthread_mutex_lock(&dev->mutex); - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER && - dev->vhostfd < 0) - goto error; - /* Step 0: tell vhost to create queues */ if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0) goto error; @@ -190,11 +186,6 @@ virtio_user_start_device(struct virtio_user_dev *dev) rte_mcfg_mem_read_lock(); pthread_mutex_lock(&dev->mutex); - /* Vhost-user client not connected yet, will start later */ - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER && - dev->vhostfd < 0) - goto out; - /* Step 2: share memory regions */ ret = dev->ops->set_memory_table(dev); if (ret < 0) @@ -213,7 +204,7 @@ virtio_user_start_device(struct virtio_user_dev *dev) goto error; dev->started = true; -out: + pthread_mutex_unlock(&dev->mutex); rte_mcfg_mem_read_unlock(); @@ -422,36 +413,36 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) PMD_DRV_LOG(ERR, "Server mode only supports vhost-user!"); return -1; } + } + + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) { dev->ops = &virtio_ops_user; - } else { - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) { - dev->ops = &virtio_ops_user; - } else if (dev->backend_type == - VIRTIO_USER_BACKEND_VHOST_KERNEL) { - dev->ops = &virtio_ops_kernel; - - dev->vhostfds = malloc(dev->max_queue_pairs * - sizeof(int)); - dev->tapfds = malloc(dev->max_queue_pairs * - sizeof(int)); - if (!dev->vhostfds || !dev->tapfds) { - PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path); - return -1; - } - - for (q = 0; q < dev->max_queue_pairs; ++q) { - dev->vhostfds[q] = -1; - dev->tapfds[q] = -1; - } - } else if (dev->backend_type == - VIRTIO_USER_BACKEND_VHOST_VDPA) { - dev->ops = &virtio_ops_vdpa; - } else { - PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path); + } else if (dev->backend_type == + VIRTIO_USER_BACKEND_VHOST_KERNEL) { + dev->ops = &virtio_ops_kernel; + + dev->vhostfds = malloc(dev->max_queue_pairs * + sizeof(int)); + dev->tapfds = malloc(dev->max_queue_pairs * + sizeof(int)); + if (!dev->vhostfds || !dev->tapfds) { + PMD_INIT_LOG(ERR, "(%s) Failed to allocate FDs", dev->path); return -1; } + + for (q = 0; q < dev->max_queue_pairs; ++q) { + dev->vhostfds[q] = -1; + dev->tapfds[q] = -1; + } + } else if (dev->backend_type == + VIRTIO_USER_BACKEND_VHOST_VDPA) { + dev->ops = &virtio_ops_vdpa; + } else { + PMD_DRV_LOG(ERR, "(%s) Unknown backend type", dev->path); + return -1; } + if (dev->ops->setup(dev) < 0) { PMD_INIT_LOG(ERR, "(%s) Failed to setup backend\n", dev->path); return -1; @@ -541,54 +532,36 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, dev->unsupported_features |= (1ULL << VHOST_USER_F_PROTOCOL_FEATURES); - if (!dev->is_server) { - if (dev->ops->set_owner(dev) < 0) { - PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path); - return -1; - } + if (dev->ops->set_owner(dev) < 0) { + PMD_INIT_LOG(ERR, "(%s) Failed to set backend owner", dev->path); + return -1; + } + + if (dev->ops->get_features(dev, &dev->device_features) < 0) { + PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path); + return -1; + } - if (dev->ops->get_features(dev, &dev->device_features) < 0) { - PMD_INIT_LOG(ERR, "(%s) Failed to get backend features", dev->path); + if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) { + if (dev->ops->get_protocol_features(dev, &protocol_features)) { + PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features", + dev->path); return -1; } + dev->protocol_features &= protocol_features; - if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || - (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) { - if (dev->ops->get_protocol_features(dev, &protocol_features)) { - PMD_INIT_LOG(ERR, "(%s) Failed to get backend protocol features", - dev->path); - return -1; - } - - dev->protocol_features &= protocol_features; - - if (dev->ops->set_protocol_features(dev, dev->protocol_features)) { - PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features", - dev->path); - return -1; - } - - if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) - dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ); + if (dev->ops->set_protocol_features(dev, dev->protocol_features)) { + PMD_INIT_LOG(ERR, "(%s) Failed to set backend protocol features", + dev->path); + return -1; } - } else { - /* We just pretend vhost-user can support all these features. - * Note that this could be problematic that if some feature is - * negotiated but not supported by the vhost-user which comes - * later. - */ - dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES; - /* We cannot assume VHOST_USER_PROTOCOL_F_STATUS is supported - * until it's negotiated - */ - dev->protocol_features &= - ~(1ULL << VHOST_USER_PROTOCOL_F_STATUS); + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) + dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ); } - - if (!mrg_rxbuf) dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF); diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 5e256f4e6d..e822d93690 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -174,11 +174,6 @@ virtio_user_delayed_handler(void *param) if (dev->vhostfd >= 0) { close(dev->vhostfd); dev->vhostfd = -1; - /* Until the featuers are negotiated again, don't assume - * the backend supports VHOST_USER_PROTOCOL_F_STATUS - */ - dev->protocol_features &= - ~(1ULL << VHOST_USER_PROTOCOL_F_STATUS); } eth_dev->intr_handle->fd = dev->listenfd; rte_intr_callback_register(eth_dev->intr_handle, -- 2.29.2