From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 318AFA00C5; Mon, 6 Jul 2020 10:49:09 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id D9B3D1DA75; Mon, 6 Jul 2020 10:49:08 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id 0801F1D940 for ; Mon, 6 Jul 2020 10:49:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594025347; 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:autocrypt:autocrypt; bh=+01rI0/dQBBzc0sqc+nn+jCTLUJZMh1F/eWBKiJjxW4=; b=DKBPSmi2b9fuOKhBOPFagAAS2YerZ3hI+ClBwYY2EYC7mFYky0Y6H/SstOXOyzTtwKRSNT EjhhfudnkELsEn0AFFxpmmDd6mzefFsLCQYiUWI9ece3B89JRi34hCL3NjQ4ovjignUhdF 293TI3Et65LsBxbU2A2+vqNapO6DWQ0= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-417-LUmIMOgPPwuUoI1jmwIggQ-1; Mon, 06 Jul 2020 04:48:59 -0400 X-MC-Unique: LUmIMOgPPwuUoI1jmwIggQ-1 Received: by mail-wm1-f71.google.com with SMTP id g6so39412796wmk.4 for ; Mon, 06 Jul 2020 01:48:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:autocrypt :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=+01rI0/dQBBzc0sqc+nn+jCTLUJZMh1F/eWBKiJjxW4=; b=IcItRCunxlOAOmxsWRifTdUbCYHIZzEpJVVNwoIPvaXySfng351OQInXpmK5mmXUtr sUEY0h4/WJ4Fa0INjS7GRQ/AbUsFXLcisjfjT3sNamuTIRMwebLipJE2aBu/V/9XNZic +0DO3W/IWKi5Q39PVQ7WNEiyLZyDYuu9vgj6+WpmAqsvHYfxME9Cnmt2B7pO6RSKLp81 2cnoSn2ug+7P/571MAlEXlf7Jv8KAMuJf4VLYm4dFJ694ZiOpGSHgRoykl7alQO0zW5a 6Ng4qTUN3oC9N8HUiDb73Kjj9DUfFY45WDieo/3jNiIRs9qPLgp/koBugfp8IwK4N1oZ lErg== X-Gm-Message-State: AOAM531Bo00R12KBnpWMbnZQDQ6LTazSb+x4zU4K8cw8ForOEIJNlpid VVI2j7U21SzyY3Wusv7+6lZUxbOJFNnHhDAm6nABIhdP0jgAtcp+MFkIT439rBLoScPbAzEUttm CXPo= X-Received: by 2002:a1c:4d11:: with SMTP id o17mr47160054wmh.134.1594025338234; Mon, 06 Jul 2020 01:48:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwmJO7GwetallYqd6f6PYcCZUsFZQleC6Rcu90r4FP9piDc6LrWCf4oJtsMW4KkK0xc84sEkw== X-Received: by 2002:a1c:4d11:: with SMTP id o17mr47160034wmh.134.1594025338014; Mon, 06 Jul 2020 01:48:58 -0700 (PDT) Received: from amorenoz.users.ipa.redhat.com ([94.73.58.18]) by smtp.gmail.com with ESMTPSA id i101sm2231409wri.47.2020.07.06.01.48.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jul 2020 01:48:57 -0700 (PDT) To: "Xia, Chenbo" , "dev@dpdk.org" , "shahafs@mellanox.com" , "matan@mellanox.com" , "maxime.coquelin@redhat.com" , "Wang, Xiao W" , "viacheslavo@mellanox.com" Cc: "jasowang@redhat.com" , "lulu@redhat.com" References: <20200702083237.1215652-1-amorenoz@redhat.com> <20200702083237.1215652-7-amorenoz@redhat.com> From: Adrian Moreno Autocrypt: addr=amorenoz@redhat.com; prefer-encrypt=mutual; keydata= mQENBF1syNUBCADQ9dk3fDMxOZ/+OQpmbanpodYxEv8IRtDz8PXw8YX7UyGfozOpLjQ8Fftj ZxuubYNbt2QVbSgviFilFdNWu2eTnN/JaGtfhmTOLPVoakkPHZF8lbgImMoch7L0fH8wN2IM KPxQyPNlX+K9FD5brHsV1lfe1TwAxmhcvLW8yNrVq+9eDIDykxc7tH4exIqXgZroahGxMHKy c8Ti2kJka/t6pDfRaY0J+6J7I1nrn6GXXSMNA45EH8+0N/QlcXhP3rfftnoPeVmpjswzvJqY FNjf/Q5VPLx7RX0Qx+y8mMB2JcChV5Bl7D7x5EUbItj6+Sy7QfOgCtPegk9HSrBCNYaLABEB AAG0I0FkcmlhbiBNb3Jlbm8gPGFtb3Jlbm96QHJlZGhhdC5jb20+iQFUBBMBCAA+FiEEogUD gihhmbOPHy26d5C5fbYeFsUFAl1syNUCGwMFCQHhM4AFCwkIBwIGFQoJCAsCBBYCAwECHgEC F4AACgkQd5C5fbYeFsX7qwgArGHSkX+ILNcujkVzjTG4OtkpJMPFlkn/1PxSEKD0jLuzx14B COzpg/Mqj3Re/QBuOas+ci9bsUA0/2nORtmmEBvzDOJpR5FH1jaGCx8USlY4WM6QqEDNZgTw hsy9KhjFzFjMk+oo3HyItXA+Uq9yrRBTjNBGTXxezMRcMuUZ4MIAfY0IRBglL2BufiuL43jD BvTENNFLoQ/wFV7qkFWSkv+8IjTsxr7M6XUo1QLd29Hn0dvwssN579HL1+BP46i2REpzeBEG L75iVChi+YnIQQNMJ9NYarVabZx4Y1Gn8+7B/1SNArDV+IDgnYgt7E58otoV2Ap310dmtuvE VbxGpbkBDQRdbMjVAQgAqyp9oA7WDu7/Y9T4Ommt69iZx8os7shUIfdgPEy5xrcPn6qGwN1/ HQ4j8nWfBG9uuX1X0RXUZIUEtYTxtED4yaCQMTqDUf9cBAwAA2mYxBfoiNYx8YqxM+sT0/J4 2qmDd+y+20UR4yzHE8AmIbspTzDFIJDAi+jKSR8F355z0sfW7CIMDC4ZWrPsskjEy1YN/U10 r6tRRH1kNyrCSbTG0d9MtcQO58h7DLXuzUhErB+BtG52A04t5cweIJTJC+koV5XPeilzlHnm RFoj0ncruGa9Odns21BNt3cy9wLfK+aUnWuAB1uc6bJGQPiAwjkilz7g7MBRUuIQ2Zt7HGLc SwARAQABiQE8BBgBCAAmFiEEogUDgihhmbOPHy26d5C5fbYeFsUFAl1syNUCGwwFCQHhM4AA CgkQd5C5fbYeFsUlSwf8CH+u/IXaE7WeWxwFkMaORfW8cM4q0xrL3M6yRGuQNW+kMjnrvK9U J9G+L1/5uTRbDQ/4LdoKqize8LjehA+iF6ba4t9Npikh8fLKWgaJfQ/hPhH4C3O5gWPOLTW6 ylGxiuER4CdFwQIoAMMslhFA7G+teeOKBq36E+1+zrybI6Xy1UBSlpDK9j4CtTnMQejjuSQb Qhle+l8VroaUHq869wjAhRHHhqmtJKggI+OvzgQpDIwfHIDypb1BuKydi2W6cVYEALUYyCLS dTBDhzj8zR5tPCsga8J7+TclQzkWOiI2C6ZtiWrMsL/Uym3uXk5nsoc7lSj7yLd/MrBRhYfP JQ== Message-ID: <8b55488a-2aea-a836-2756-743194644029@redhat.com> Date: Mon, 6 Jul 2020 10:48:56 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=amorenoz@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 7/6/20 5:22 AM, Xia, Chenbo wrote: > Hi Adrian, > >> -----Original Message----- >> From: dev On Behalf Of Adrian Moreno >> Sent: Thursday, July 2, 2020 4:33 PM >> To: dev@dpdk.org; Ye, Xiaolong ; >> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com; >> Wang, Xiao W ; viacheslavo@mellanox.com >> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno >> >> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status >> message >> >> This patch adds support to the new Virtio device get status Vhost-user message. >> >> The driver can send this new message to read the device status. >> >> One of the uses of this message is to ensure the feature negotiation has >> succeded. According to the virtio spec, after completing the feature negotiation, >> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device >> has accepted the features. >> >> This patch also clears the FEATURE_OK status bit if the feature negotiation has >> failed to let the driver know about his failure. >> >> Signed-off-by: Adrian Moreno >> --- >> lib/librte_vhost/vhost.h | 2 ++ >> lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++ >> lib/librte_vhost/vhost_user.h | 1 + >> 3 files changed, 35 insertions(+) >> >> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index >> 25d31c71b..e743821cc 100644 >> --- a/lib/librte_vhost/vhost.h >> +++ b/lib/librte_vhost/vhost.h >> @@ -32,6 +32,8 @@ >> #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4 >> /* Used to indicate that the device has its own data path and configured */ >> #define VIRTIO_DEV_VDPA_CONFIGURED 8 >> +/* Used to indicate that the feature negotiation failed */ #define >> +VIRTIO_DEV_FEATURES_FAILED 16 >> >> /* Backend value set by guest. */ >> #define VIRTIO_DEV_STOPPED -1 >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index >> 8d3d13913..00da7bf18 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX] >> = { >> [VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD", >> [VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD", >> [VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS", >> + [VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS", >> }; >> >> static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6 >> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct >> VhostUserMsg *msg, >> VHOST_LOG_CONFIG(ERR, >> "(%d) received invalid negotiated features.\n", >> dev->vid); >> + dev->flags |= VIRTIO_DEV_FEATURES_FAILED; >> + dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK; >> + >> return RTE_VHOST_MSG_RESULT_ERR; >> } >> >> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct >> VhostUserMsg *msg, >> if (vdpa_dev) >> vdpa_dev->ops->set_features(dev->vid); >> >> + dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED; >> return RTE_VHOST_MSG_RESULT_OK; >> } >> >> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev, >> struct VhostUserMsg *msg, >> return RTE_VHOST_MSG_RESULT_REPLY; >> } >> >> +static int >> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg, >> + int main_fd __rte_unused) >> +{ >> + struct virtio_net *dev = *pdev; >> + >> + if (validate_msg_fds(msg, 0) != 0) >> + return RTE_VHOST_MSG_RESULT_ERR; >> + >> + msg->payload.u64 = dev->status; >> + msg->size = sizeof(msg->payload.u64); >> + msg->fd_num = 0; >> + >> + return RTE_VHOST_MSG_RESULT_OK; >> +} >> + >> static int >> vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg, >> int main_fd __rte_unused) >> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev, >> struct VhostUserMsg *msg, >> >> dev->status = msg->payload.u64; >> >> + if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) && >> + (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) { >> + VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature >> negotiation failed\n"); >> + /* >> + * Clear the bit to let the driver know about the feature >> + * negotiation failure >> + */ >> + dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK; >> + } >> + > > There's a coding style issue because of above '}' alignment. Could you fix this? > > Another thing I'm not sure: if above condition happens, should it be treated > as err? If set status is with replay-ack (this will happen, right?), would QEMU > like to know this status is not set? As QEMU should know it during SET_FEATURES, > I'm not sure whether this will also need NACK when reply-ack enabled. What's > your opinion? > My interpretation was that, since we have already NACKed SET_FEATURES, SET_STATUS should only NACK if we were unable to set the status (device is not present, invalid message, etc), and according to the virtio standard the driver must read again and verify FEATURES_OK is still set, therefore NACKing the SET_STATUS would only hide the real problem. Besides, for a driver (e.g: qemu) that implements the virtio/vhost logic agnostic of the underlying vhost type (vhost-net or vhost-user) a spec-oriented way of expressing errors is preferred. See as an example a (still unmerged) use of this feature in function "static int vhost_vdpa_set_features()" in: https://patchwork.ozlabs.org/project/qemu-devel/patch/20200701145538.22333-14-lulu@redhat.com/ Having said all this, I realize this should be a rare case. This mechanism is in place to prevent the driver from configuring an incompatible combination of features. However, the vhost backend only checks that qemu has honored it's original feature set which the driver must do according to the spec. So I'm happy to change it if you have a strong opinion on this. > Thanks! > Chenbo > >> VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n" >> "\t-ACKNOWLEDGE: %u\n" >> "\t-DRIVER: %u\n" >> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t >> vhost_message_handlers[VHOST_USER_MAX] = { >> [VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd, >> [VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd, >> [VHOST_USER_SET_STATUS] = vhost_user_set_status, >> + [VHOST_USER_GET_STATUS] = vhost_user_get_status, >> }; >> >> /* return bytes# of read on success or negative val on failure. */ diff --git >> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index >> 82885ab5e..16fe03f88 100644 >> --- a/lib/librte_vhost/vhost_user.h >> +++ b/lib/librte_vhost/vhost_user.h >> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest { >> VHOST_USER_GET_INFLIGHT_FD = 31, >> VHOST_USER_SET_INFLIGHT_FD = 32, >> VHOST_USER_SET_STATUS = 39, >> + VHOST_USER_GET_STATUS = 40, >> VHOST_USER_MAX = 41 >> } VhostUserRequest; >> >> -- >> 2.26.2 > -- Adrián Moreno