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 B03E8A0A04; Fri, 15 Jan 2021 17:50:07 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 964D51411EC; Fri, 15 Jan 2021 17:50:07 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id C3D07141196 for ; Fri, 15 Jan 2021 17:50:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610729405; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TKKTy3EW4c7tGyFW10zQGKhSbod55fYAl0NEaNgxrdA=; b=Ez+wMlAnD/I9FcJ7oezCFLmMPSMdUVk8GOAS+HqKdAfkXAmlpllr0a2iNL43ofk0dRjaqH lcZE3p8qD7niuepLmTklXwnaxXonc37nrdOwut+3OTuGpqve5nnw2ulVDZg5vBaucgh+Qf Hn2pKURgawkqz2m1UU8rEBjYtSyP9O4= 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-163-1x_5d551M_SNPxe8uffxDg-1; Fri, 15 Jan 2021 11:50:03 -0500 X-MC-Unique: 1x_5d551M_SNPxe8uffxDg-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 23084802B45; Fri, 15 Jan 2021 16:50:02 +0000 (UTC) Received: from [10.36.110.24] (unknown [10.36.110.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9D1DE189B6; Fri, 15 Jan 2021 16:49:57 +0000 (UTC) To: Adrian Moreno , dev@dpdk.org, chenbo.xia@intel.com, olivier.matz@6wind.com, david.marchand@redhat.com References: <20201220211405.313012-1-maxime.coquelin@redhat.com> <20201220211405.313012-38-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: <125ac9d9-a168-65f9-06a8-7f80d3c9c4f0@redhat.com> Date: Fri, 15 Jan 2021 17:49:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: 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-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 37/40] net/virtio: introduce backend data 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" On 1/13/21 6:18 PM, Adrian Moreno wrote: > > > On 12/20/20 10:14 PM, Maxime Coquelin wrote: >> The goal of this patch is to introduce backend-specific >> data in order to better isolate what is backend-specific >> from what is generic to Virtio-user. >> >> For now, only Vhost-user protocol features are moved to >> Vhost-user backend data. >> >> Signed-off-by: Maxime Coquelin >> --- >> drivers/net/virtio/virtio_user/vhost.h | 1 + >> drivers/net/virtio/virtio_user/vhost_kernel.c | 7 +++ >> drivers/net/virtio/virtio_user/vhost_user.c | 46 +++++++++++++++---- >> drivers/net/virtio/virtio_user/vhost_vdpa.c | 8 ++++ >> .../net/virtio/virtio_user/virtio_user_dev.c | 2 + >> .../net/virtio/virtio_user/virtio_user_dev.h | 3 +- >> 6 files changed, 58 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h >> index b488ac78a1..ee5598226d 100644 >> --- a/drivers/net/virtio/virtio_user/vhost.h >> +++ b/drivers/net/virtio/virtio_user/vhost.h >> @@ -57,6 +57,7 @@ struct virtio_user_dev; >> >> struct virtio_user_backend_ops { >> int (*setup)(struct virtio_user_dev *dev); >> + int (*destroy)(struct virtio_user_dev *dev); >> int (*get_backend_features)(uint64_t *features); >> int (*set_owner)(struct virtio_user_dev *dev); >> int (*get_features)(struct virtio_user_dev *dev, uint64_t *features); >> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c b/drivers/net/virtio/virtio_user/vhost_kernel.c >> index 2fd00afa84..023fddcd69 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c >> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c >> @@ -357,6 +357,12 @@ vhost_kernel_setup(struct virtio_user_dev *dev) >> return 0; >> } >> >> +static int >> +vhost_kernel_destroy(struct virtio_user_dev *dev) >> +{ >> + return 0; >> +} >> + >> static int >> vhost_kernel_set_backend(int vhostfd, int tapfd) >> { >> @@ -455,6 +461,7 @@ vhost_kernel_get_backend_features(uint64_t *features) >> >> struct virtio_user_backend_ops virtio_ops_kernel = { >> .setup = vhost_kernel_setup, >> + .destroy = vhost_kernel_destroy, >> .get_backend_features = vhost_kernel_get_backend_features, >> .set_owner = vhost_kernel_set_owner, >> .get_features = vhost_kernel_get_features, >> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c >> index f67c40e047..e96b1d8b9c 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_user.c >> +++ b/drivers/net/virtio/virtio_user/vhost_user.c >> @@ -17,6 +17,9 @@ >> #include "vhost.h" >> #include "virtio_user_dev.h" >> >> +struct vhost_user_data { >> + uint64_t protocol_features; >> +}; >> >> #ifndef VHOST_USER_F_PROTOCOL_FEATURES >> #define VHOST_USER_F_PROTOCOL_FEATURES 30 >> @@ -274,6 +277,7 @@ static int >> vhost_user_get_features(struct virtio_user_dev *dev, uint64_t *features) >> { >> int ret; >> + struct vhost_user_data *data = dev->backend_data; >> struct vhost_user_msg msg = { >> .request = VHOST_USER_GET_FEATURES, >> .flags = VHOST_USER_VERSION, >> @@ -303,17 +307,17 @@ vhost_user_get_features(struct virtio_user_dev *dev, uint64_t *features) >> return 0; >> >> /* Negotiate protocol features */ >> - ret = vhost_user_get_protocol_features(dev, &dev->protocol_features); >> + ret = vhost_user_get_protocol_features(dev, &data->protocol_features); >> if (ret < 0) >> goto err; >> >> - dev->protocol_features &= VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; >> + data->protocol_features &= VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; >> >> - ret = vhost_user_set_protocol_features(dev, dev->protocol_features); >> + ret = vhost_user_set_protocol_features(dev, data->protocol_features); >> if (ret < 0) >> goto err; >> >> - if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) >> + if (!(data->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_MQ))) >> dev->unsupported_features |= (1ull << VIRTIO_NET_F_MQ); >> >> return 0; >> @@ -429,12 +433,13 @@ vhost_user_set_memory_table(struct virtio_user_dev *dev) >> struct walk_arg wa; >> int fds[VHOST_MEMORY_MAX_NREGIONS]; >> int ret, fd_num; >> + struct vhost_user_data *data = dev->backend_data; >> struct vhost_user_msg msg = { >> .request = VHOST_USER_SET_MEM_TABLE, >> .flags = VHOST_USER_VERSION, >> }; >> >> - if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> + if (data->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> msg.flags |= VHOST_USER_NEED_REPLY_MASK; >> >> wa.region_nr = 0; >> @@ -613,6 +618,7 @@ static int >> vhost_user_get_status(struct virtio_user_dev *dev, uint8_t *status) >> { >> int ret; >> + struct vhost_user_data *data = dev->backend_data; >> struct vhost_user_msg msg = { >> .request = VHOST_USER_GET_STATUS, >> .flags = VHOST_USER_VERSION, >> @@ -629,7 +635,7 @@ vhost_user_get_status(struct virtio_user_dev *dev, uint8_t *status) >> if (!(dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) >> return -ENOTSUP; >> >> - if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> + if (!(data->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> return -ENOTSUP; >> >> ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0); >> @@ -666,6 +672,7 @@ static int >> vhost_user_set_status(struct virtio_user_dev *dev, uint8_t status) >> { >> int ret; >> + struct vhost_user_data *data = dev->backend_data; >> struct vhost_user_msg msg = { >> .request = VHOST_USER_SET_STATUS, >> .flags = VHOST_USER_VERSION, >> @@ -673,7 +680,7 @@ vhost_user_set_status(struct virtio_user_dev *dev, uint8_t status) >> .payload.u64 = status, >> }; >> >> - if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> + if (data->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> msg.flags |= VHOST_USER_NEED_REPLY_MASK; >> >> /* >> @@ -687,7 +694,7 @@ vhost_user_set_status(struct virtio_user_dev *dev, uint8_t status) >> if (!(dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) >> return -ENOTSUP; >> >> - if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> + if (!(data->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> return -ENOTSUP; >> >> ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0); >> @@ -747,6 +754,17 @@ vhost_user_setup(struct virtio_user_dev *dev) >> int fd; >> int flag; >> struct sockaddr_un un; >> + struct vhost_user_data *data; >> + >> + data = malloc(sizeof(*data)); >> + if (!data) { >> + PMD_DRV_LOG(ERR, "(%s) Failed to allocate Vhost-user data\n", dev->path); >> + return -1; >> + } >> + >> + memset(data, 0, sizeof(*data)); >> + >> + dev->backend_data = data; >> >> fd = socket(AF_UNIX, SOCK_STREAM, 0); >> if (fd < 0) { > > There are a number of "return -1;" statements below which, if executed, would > now leak. Fixed. > Also, there are two in virtio_user_dev.c:virtio_user_dev_setup() Good catch, I'm adding a new patch to handle failure properly. Also, I'll fix destroy callbacks not to crash if called twice or called after setup callback failure. >> @@ -781,6 +799,17 @@ vhost_user_setup(struct virtio_user_dev *dev) >> return 0; >> } >> >> +static int >> +vhost_user_destroy(struct virtio_user_dev *dev) >> +{ >> + if (dev->backend_data) { >> + free(dev->backend_data); >> + dev->backend_data = NULL; >> + } >> + >> + return 0; >> +} >> + >> static int >> vhost_user_enable_queue_pair(struct virtio_user_dev *dev, >> uint16_t pair_idx, >> @@ -815,6 +844,7 @@ vhost_user_get_backend_features(uint64_t *features) >> >> struct virtio_user_backend_ops virtio_ops_user = { >> .setup = vhost_user_setup, >> + .destroy = vhost_user_destroy, >> .get_backend_features = vhost_user_get_backend_features, >> .set_owner = vhost_user_set_owner, >> .get_features = vhost_user_get_features, >> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> index c826f333e0..b29426d767 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c >> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> @@ -287,6 +287,13 @@ vhost_vdpa_setup(struct virtio_user_dev *dev) >> return 0; >> } >> >> +static int >> +vhost_vdpa_destroy(struct virtio_user_dev *dev __rte_unused) >> +{ >> + return; >> + return 0; >> +} >> + >> static int >> vhost_vdpa_enable_queue_pair(struct virtio_user_dev *dev, >> uint16_t pair_idx, >> @@ -322,6 +329,7 @@ vhost_vdpa_get_backend_features(uint64_t *features) >> >> struct virtio_user_backend_ops virtio_ops_vdpa = { >> .setup = vhost_vdpa_setup, >> + .destroy = vhost_vdpa_destroy, >> .get_backend_features = vhost_vdpa_get_backend_features, >> .set_owner = vhost_vdpa_set_owner, >> .get_features = vhost_vdpa_get_features, >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index 974de133bc..8d19a0addd 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -620,6 +620,8 @@ virtio_user_dev_uninit(struct virtio_user_dev *dev) >> >> if (dev->is_server) >> unlink(dev->path); >> + >> + dev->ops->destroy(dev); >> } >> >> uint8_t >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> index b2da2944e9..ec73d5de11 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> @@ -30,7 +30,6 @@ struct virtio_user_dev { >> int vhostfd; >> int listenfd; /* listening fd */ >> bool is_server; /* server or client mode */ >> - uint64_t protocol_features; /* negotiated protocol features*/ >> >> /* for vhost_kernel backend */ >> char *ifname; >> @@ -66,6 +65,8 @@ struct virtio_user_dev { >> struct virtio_user_backend_ops *ops; >> pthread_mutex_t mutex; >> bool started; >> + >> + void *backend_data; >> }; >> >> int virtio_user_dev_set_features(struct virtio_user_dev *dev); >> > > > >