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 E5C5CA0A02; Fri, 15 Jan 2021 11:49:04 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AE381140EF3; Fri, 15 Jan 2021 11:49:04 +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 41A80140EF2 for ; Fri, 15 Jan 2021 11:49:03 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610707742; 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=o0PAL34ayft/YqpLDuRCRZRZ3WK8otmTe5t3SLh1zMY=; b=KTbvcl8od2qabqa3Il0UeUKmfrz4i1t7Dpt8F0jjK4hLjBZzgGBnU67DLdeEU4zRrzt1JU pMSwcjbEtZBLfS0DW1Yi5gfoLoViAXf1Jd8Aao0vVOYnQZQFSZ3J3A8GYZb06lMp5z6Niw OnjoZoHWeFfu0ceGraQR8OzAB1CXUzM= 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-425-oiZFFs2pMN6BME-qyUngKg-1; Fri, 15 Jan 2021 05:49:01 -0500 X-MC-Unique: oiZFFs2pMN6BME-qyUngKg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E04F3100C663; Fri, 15 Jan 2021 10:48:59 +0000 (UTC) Received: from [10.36.110.24] (unknown [10.36.110.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A9C4270911; Fri, 15 Jan 2021 10:48:54 +0000 (UTC) To: "Xia, Chenbo" , "dev@dpdk.org" , "olivier.matz@6wind.com" , "amorenoz@redhat.com" , "david.marchand@redhat.com" References: <20201220211405.313012-1-maxime.coquelin@redhat.com> <20201220211405.313012-32-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: Date: Fri, 15 Jan 2021 11:48:53 +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.13 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: 7bit Subject: Re: [dpdk-dev] [PATCH 31/40] net/virtio: add Virtio-user status ops 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/6/21 1:09 PM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Monday, December 21, 2020 5:14 AM >> To: dev@dpdk.org; Xia, Chenbo ; olivier.matz@6wind.com; >> amorenoz@redhat.com; david.marchand@redhat.com >> Cc: Maxime Coquelin >> Subject: [PATCH 31/40] net/virtio: add Virtio-user status ops >> >> This patch introduces new callbacks to >> get and set the device status. >> >> Signed-off-by: Maxime Coquelin >> --- >> drivers/net/virtio/virtio_user/vhost.h | 2 + >> drivers/net/virtio/virtio_user/vhost_kernel.c | 14 ++ >> drivers/net/virtio/virtio_user/vhost_user.c | 121 +++++++++++++----- >> drivers/net/virtio/virtio_user/vhost_vdpa.c | 16 ++- >> .../net/virtio/virtio_user/virtio_user_dev.c | 42 ++---- >> 5 files changed, 129 insertions(+), 66 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_user/vhost.h >> b/drivers/net/virtio/virtio_user/vhost.h >> index 956eb58728..c85ba9a47b 100644 >> --- a/drivers/net/virtio/virtio_user/vhost.h >> +++ b/drivers/net/virtio/virtio_user/vhost.h >> @@ -114,6 +114,8 @@ struct virtio_user_backend_ops { >> int (*set_vring_call)(struct virtio_user_dev *dev, struct >> vhost_vring_file *file); >> int (*set_vring_kick)(struct virtio_user_dev *dev, struct >> vhost_vring_file *file); >> int (*set_vring_addr)(struct virtio_user_dev *dev, struct >> vhost_vring_addr *addr); >> + int (*get_status)(struct virtio_user_dev *dev, uint8_t *status); >> + int (*set_status)(struct virtio_user_dev *dev, uint8_t status); >> int (*send_request)(struct virtio_user_dev *dev, >> enum vhost_user_request req, >> void *arg); >> diff --git a/drivers/net/virtio/virtio_user/vhost_kernel.c >> b/drivers/net/virtio/virtio_user/vhost_kernel.c >> index 8cd86b72c6..0b74f73c97 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_kernel.c >> +++ b/drivers/net/virtio/virtio_user/vhost_kernel.c >> @@ -316,6 +316,18 @@ vhost_kernel_set_vring_addr(struct virtio_user_dev *dev, >> struct vhost_vring_addr >> return 0; >> } >> >> +static int >> +vhost_kernel_get_status(struct virtio_user_dev *dev __rte_unused, uint8_t >> *status __rte_unused) >> +{ >> + return -ENOTSUP; >> +} >> + >> +static int >> +vhost_kernel_set_status(struct virtio_user_dev *dev __rte_unused, uint8_t >> status __rte_unused) >> +{ >> + return -ENOTSUP; >> +} >> + >> static uint64_t vhost_req_user_to_kernel[] = { >> [VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER, >> }; >> @@ -487,6 +499,8 @@ struct virtio_user_backend_ops virtio_ops_kernel = { >> .set_vring_call = vhost_kernel_set_vring_call, >> .set_vring_kick = vhost_kernel_set_vring_kick, >> .set_vring_addr = vhost_kernel_set_vring_addr, >> + .get_status = vhost_kernel_get_status, >> + .set_status = vhost_kernel_set_status, >> .send_request = vhost_kernel_send_request, >> .enable_qp = vhost_kernel_enable_queue_pair >> }; >> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c >> b/drivers/net/virtio/virtio_user/vhost_user.c >> index 59cf366683..d426b44fe7 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_user.c >> +++ b/drivers/net/virtio/virtio_user/vhost_user.c >> @@ -544,13 +544,100 @@ vhost_user_set_vring_addr(struct virtio_user_dev *dev, >> struct vhost_vring_addr * >> return 0; >> } >> >> +static int >> +vhost_user_get_status(struct virtio_user_dev *dev, uint8_t *status) >> +{ >> + int ret; >> + struct vhost_user_msg msg = { >> + .request = VHOST_USER_GET_STATUS, >> + .flags = VHOST_USER_VERSION, >> + }; >> + >> + /* >> + * If features have not been negotiated, we don't know if the backend >> + * supports protocol features >> + */ >> + if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK)) >> + return -ENOTSUP; >> + >> + /* Status protocol feature requires protocol features support */ >> + if (!(dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) >> + return -ENOTSUP; >> + >> + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> + return -ENOTSUP; >> + >> + ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0); >> + if (ret < 0) { >> + PMD_DRV_LOG(ERR, "Failed to send request"); >> + goto err; >> + } >> + >> + ret = vhost_user_read(dev->vhostfd, &msg); >> + if (ret < 0) { >> + PMD_DRV_LOG(ERR, "Failed to recv request"); >> + goto err; >> + } >> + >> + if (msg.request != VHOST_USER_GET_STATUS) { >> + PMD_DRV_LOG(ERR, "Unexpected request type (%d)", msg.request); >> + goto err; >> + } >> + >> + if (msg.size != sizeof(msg.payload.u64)) { >> + PMD_DRV_LOG(ERR, "Unexpected payload size (%d)", msg.size); > > Better use %u here? If you will change this, please change other logs in > vhost_user_get_protocol_features and vhost_user_get_features too. Sorry > that I miss them in previous patches... No problem, fixing in other patches too. >> + goto err; >> + } >> + >> + *status = (uint8_t)msg.payload.u64; >> + >> + return 0; >> +err: >> + PMD_DRV_LOG(ERR, "Failed to get device status"); >> + return -1; >> +} >> + >> +static int >> +vhost_user_set_status(struct virtio_user_dev *dev, uint8_t status) >> +{ >> + int ret; >> + struct vhost_user_msg msg = { >> + .request = VHOST_USER_SET_STATUS, >> + .flags = VHOST_USER_VERSION, >> + .size = sizeof(msg.payload.u64), >> + .payload.u64 = status, >> + }; >> + >> + if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> + msg.flags |= VHOST_USER_NEED_REPLY_MASK; > > Move the above check after checking device supports VHOST_USER_F_PROTOCOL_FEATURES? > Logically, it's meaningless to check we supports certain protocol feature if we do > not support protocol feature at all :) Makes perfect sense, fixed. Maxime > Thanks, > Chenbo > >> + >> + /* >> + * If features have not been negotiated, we don't know if the backend >> + * supports protocol features >> + */ >> + if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK)) >> + return -ENOTSUP; >> + >> + /* Status protocol feature requires protocol features support */ >> + if (!(dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) >> + return -ENOTSUP; >> + >> + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) >> + return -ENOTSUP; >> + >> + ret = vhost_user_write(dev->vhostfd, &msg, NULL, 0); >> + if (ret < 0) { >> + PMD_DRV_LOG(ERR, "Failed to send get status request"); >> + return -1; >> + } >> + >> + return vhost_user_check_reply_ack(dev, &msg); >> +} >> >> static struct vhost_user_msg m; >> >> const char * const vhost_msg_strings[] = { >> [VHOST_USER_RESET_OWNER] = "VHOST_RESET_OWNER", >> - [VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS", >> - [VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS", >> }; >> >> static int >> @@ -561,7 +648,6 @@ vhost_user_sock(struct virtio_user_dev *dev, >> struct vhost_user_msg msg; >> struct vhost_vring_file *file = 0; >> int need_reply = 0; >> - int has_reply_ack = 0; >> int fds[VHOST_MEMORY_MAX_NREGIONS]; >> int fd_num = 0; >> int vhostfd = dev->vhostfd; >> @@ -573,31 +659,11 @@ vhost_user_sock(struct virtio_user_dev *dev, >> if (dev->is_server && vhostfd < 0) >> return -1; >> >> - if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)) >> - has_reply_ack = 1; >> - >> msg.request = req; >> msg.flags = VHOST_USER_VERSION; >> msg.size = 0; >> >> switch (req) { >> - case VHOST_USER_GET_STATUS: >> - if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) || >> - (!(dev->protocol_features & >> - (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))) >> - return -ENOTSUP; >> - need_reply = 1; >> - break; >> - >> - case VHOST_USER_SET_STATUS: >> - if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) || >> - (!(dev->protocol_features & >> - (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))) >> - return -ENOTSUP; >> - >> - if (has_reply_ack) >> - msg.flags |= VHOST_USER_NEED_REPLY_MASK; >> - /* Fallthrough */ >> case VHOST_USER_SET_LOG_BASE: >> msg.payload.u64 = *((__u64 *)arg); >> msg.size = sizeof(m.payload.u64); >> @@ -644,13 +710,6 @@ vhost_user_sock(struct virtio_user_dev *dev, >> } >> >> switch (req) { >> - case VHOST_USER_GET_STATUS: >> - if (msg.size != sizeof(m.payload.u64)) { >> - PMD_DRV_LOG(ERR, "Received bad msg size"); >> - return -1; >> - } >> - *((__u64 *)arg) = msg.payload.u64; >> - break; >> default: >> /* Reply-ack handling */ >> if (msg.size != sizeof(m.payload.u64)) { >> @@ -784,6 +843,8 @@ struct virtio_user_backend_ops virtio_ops_user = { >> .set_vring_call = vhost_user_set_vring_call, >> .set_vring_kick = vhost_user_set_vring_kick, >> .set_vring_addr = vhost_user_set_vring_addr, >> + .get_status = vhost_user_get_status, >> + .set_status = vhost_user_set_status, >> .send_request = vhost_user_sock, >> .enable_qp = vhost_user_enable_queue_pair >> }; >> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c >> b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> index e09e7c9fb8..f4331884c3 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c >> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c >> @@ -36,8 +36,6 @@ >> >> static uint64_t vhost_req_user_to_vdpa[] = { >> [VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER, >> - [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, >> - [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, >> }; >> >> /* no alignment requirement */ >> @@ -253,6 +251,18 @@ vhost_vdpa_set_vring_addr(struct virtio_user_dev *dev, >> struct vhost_vring_addr * >> return vhost_vdpa_ioctl(dev->vhostfd, VHOST_SET_VRING_ADDR, addr); >> } >> >> +static int >> +vhost_vdpa_get_status(struct virtio_user_dev *dev, uint8_t *status) >> +{ >> + return vhost_vdpa_ioctl(dev->vhostfd, VHOST_VDPA_GET_STATUS, status); >> +} >> + >> +static int >> +vhost_vdpa_set_status(struct virtio_user_dev *dev, uint8_t status) >> +{ >> + return vhost_vdpa_ioctl(dev->vhostfd, VHOST_VDPA_SET_STATUS, &status); >> +} >> + >> /* with below features, vhost vdpa does not need to do the checksum and TSO, >> * these info will be passed to virtio_user through virtio net header. >> */ >> @@ -363,6 +373,8 @@ struct virtio_user_backend_ops virtio_ops_vdpa = { >> .set_vring_call = vhost_vdpa_set_vring_call, >> .set_vring_kick = vhost_vdpa_set_vring_kick, >> .set_vring_addr = vhost_vdpa_set_vring_addr, >> + .get_status = vhost_vdpa_get_status, >> + .set_status = vhost_vdpa_set_status, >> .send_request = vhost_vdpa_send_request, >> .enable_qp = vhost_vdpa_enable_queue_pair, >> .dma_map = vhost_vdpa_dma_map, >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index 48ca7a2548..e1f016aa8c 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -805,21 +805,12 @@ int >> virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status) >> { >> int ret; >> - uint64_t arg = status; >> >> pthread_mutex_lock(&dev->mutex); >> dev->status = status; >> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) >> - ret = dev->ops->send_request(dev, >> - VHOST_USER_SET_STATUS, &arg); >> - else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) >> - ret = dev->ops->send_request(dev, >> - VHOST_USER_SET_STATUS, &status); >> - else >> - ret = -ENOTSUP; >> - >> + ret = dev->ops->set_status(dev, status); >> if (ret && ret != -ENOTSUP) { >> - PMD_INIT_LOG(ERR, "VHOST_USER_SET_STATUS failed (%d): %s", ret, >> + PMD_INIT_LOG(ERR, "Virtio-user set status failed (%d): %s", ret, >> strerror(errno)); >> } >> >> @@ -830,29 +821,13 @@ virtio_user_dev_set_status(struct virtio_user_dev *dev, >> uint8_t status) >> int >> virtio_user_dev_update_status(struct virtio_user_dev *dev) >> { >> - uint64_t ret; >> + int ret; >> uint8_t status; >> - int err; >> >> pthread_mutex_lock(&dev->mutex); >> - if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) { >> - err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret); >> - if (!err && ret > UINT8_MAX) { >> - PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS " >> - "response 0x%" PRIx64 "\n", ret); >> - err = -1; >> - goto error; >> - } >> >> - status = ret; >> - } else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) { >> - err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, >> - &status); >> - } else { >> - err = -ENOTSUP; >> - } >> - >> - if (!err) { >> + ret = dev->ops->get_status(dev, &status); >> + if (!ret) { >> dev->status = status; >> PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n" >> "\t-RESET: %u\n" >> @@ -870,12 +845,11 @@ virtio_user_dev_update_status(struct virtio_user_dev >> *dev) >> !!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK), >> !!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET), >> !!(dev->status & VIRTIO_CONFIG_STATUS_FAILED)); >> - } else if (err != -ENOTSUP) { >> - PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err, >> + } else if (ret != -ENOTSUP) { >> + PMD_INIT_LOG(ERR, "Virtio-user get status failed (%d): %s", ret, >> strerror(errno)); >> } >> >> -error: >> pthread_mutex_unlock(&dev->mutex); >> - return err; >> + return ret; >> } >> -- >> 2.29.2 >