From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (xvm-189-124.dc0.ghst.net [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 53815A09FF for ; Tue, 5 Jan 2021 17:54:18 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 48E4D160823; Tue, 5 Jan 2021 17:54:17 +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 6E4F71607F9 for ; Tue, 5 Jan 2021 17:54:15 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1609865654; 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=FVoNXHkQhuq5tiDzuX4T/9Xm0N1sjRSPRKDcZZOu40k=; b=G9TQRj/yjJaW+RYIlPFzmwgJISafK1wRAlV6kBTNpOpZWUUpLEUzLEbNSr4NvfGROQqBGC bRRLUBQI3r81+lfcfTNc16n1x2wcoboquHOfS7WFccJ1cswTh6wEgXXneWKsHlRDl8LyD+ kJOzCZSnFfmuQ0lalfDweXxWZ91rUJA= 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-211-YmbUUIdnNSKAX9CPuJZDXA-1; Tue, 05 Jan 2021 11:54:01 -0500 X-MC-Unique: YmbUUIdnNSKAX9CPuJZDXA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E00FCDF8C6; Tue, 5 Jan 2021 16:53:51 +0000 (UTC) Received: from [10.36.110.9] (unknown [10.36.110.9]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 740C85D9F1; Tue, 5 Jan 2021 16:53:13 +0000 (UTC) To: "Xia, Chenbo" , "dev@dpdk.org" , "amorenoz@redhat.com" , "jasowang@redhat.com" , "david.marchand@redhat.com" Cc: "stable@dpdk.org" References: <20201125152120.183691-1-maxime.coquelin@redhat.com> <20201125152120.183691-2-maxime.coquelin@redhat.com> <74faa657-2d2f-5093-4eba-fcdc01807257@redhat.com> From: Maxime Coquelin Message-ID: <37ae3af7-ac73-abe1-41c9-cb9b6775965d@redhat.com> Date: Tue, 5 Jan 2021 17:53:11 +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.14 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-stable] [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features negotiation X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On 12/23/20 6:37 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin >> Sent: Tuesday, December 22, 2020 6:56 PM >> To: Xia, Chenbo ; dev@dpdk.org; amorenoz@redhat.com; >> jasowang@redhat.com; david.marchand@redhat.com >> Cc: stable@dpdk.org >> Subject: Re: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features >> negotiation >> >> >> >> On 12/21/20 7:23 AM, Xia, Chenbo wrote: >>> Hi Maxime, >>> >>>> -----Original Message----- >>>> From: Maxime Coquelin >>>> Sent: Wednesday, November 25, 2020 11:21 PM >>>> To: dev@dpdk.org; Xia, Chenbo ; amorenoz@redhat.com; >>>> jasowang@redhat.com; david.marchand@redhat.com >>>> Cc: Maxime Coquelin ; stable@dpdk.org >>>> Subject: [PATCH 21.02 v2 1/2] net/virtio: fix missing backend features >>>> negotiation >>>> >>>> This patch adds missing backend features negotiation for >>>> in Vhost-vDPA. Without it, IOTLB messages v2 could be sent >>>> by Virtio-user PMD while not supported by the backend. >>>> >>>> Fixes: 6b901437056e ("net/virtio: introduce vhost-vDPA backend") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Maxime Coquelin >>>> --- >>>> drivers/net/virtio/virtio_user/vhost.h | 4 ++++ >>>> drivers/net/virtio/virtio_user/vhost_vdpa.c | 14 ++++++++++++++ >>>> drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 ++++++++++---- >>>> 3 files changed, 28 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h >>>> b/drivers/net/virtio/virtio_user/vhost.h >>>> index 210a3704e7..c1dcc50b58 100644 >>>> --- a/drivers/net/virtio/virtio_user/vhost.h >>>> +++ b/drivers/net/virtio/virtio_user/vhost.h >>>> @@ -86,6 +86,10 @@ enum vhost_user_request { >>>> VHOST_USER_MAX >>>> }; >>>> >>>> +#ifndef VHOST_BACKEND_F_IOTLB_MSG_V2 >>>> +#define VHOST_BACKEND_F_IOTLB_MSG_V2 1 >>>> +#endif >>>> + >>>> extern const char * const vhost_msg_strings[VHOST_USER_MAX]; >>>> >>>> struct vhost_memory_region { >>>> diff --git a/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> b/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> index c7b9349fc8..b6c81d6f17 100644 >>>> --- a/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> +++ b/drivers/net/virtio/virtio_user/vhost_vdpa.c >>>> @@ -35,6 +35,8 @@ >>>> #define VHOST_VDPA_SET_STATUS _IOW(VHOST_VIRTIO, 0x72, __u8) >>>> #define VHOST_VDPA_SET_VRING_ENABLE _IOW(VHOST_VIRTIO, 0x75, \ >>>> struct vhost_vring_state) >>>> +#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64) >>>> +#define VHOST_GET_BACKEND_FEATURES _IOR(VHOST_VIRTIO, 0x26, __u64) >>>> >>>> static uint64_t vhost_req_user_to_vdpa[] = { >>>> [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER, >>>> @@ -51,6 +53,8 @@ static uint64_t vhost_req_user_to_vdpa[] = { >>>> [VHOST_USER_SET_STATUS] = VHOST_VDPA_SET_STATUS, >>>> [VHOST_USER_GET_STATUS] = VHOST_VDPA_GET_STATUS, >>>> [VHOST_USER_SET_VRING_ENABLE] = VHOST_VDPA_SET_VRING_ENABLE, >>>> + [VHOST_USER_GET_PROTOCOL_FEATURES] = VHOST_GET_BACKEND_FEATURES, >>>> + [VHOST_USER_SET_PROTOCOL_FEATURES] = VHOST_SET_BACKEND_FEATURES, >>>> }; >>>> >>>> /* no alignment requirement */ >>>> @@ -86,6 +90,11 @@ vhost_vdpa_dma_map(struct virtio_user_dev *dev, void >> *addr, >>>> { >>>> struct vhost_msg msg = {}; >>>> >>>> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >>>> { >>>> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >>>> + return -1; >>>> + } >>>> + >>>> msg.type = VHOST_IOTLB_MSG_V2; >>>> msg.iotlb.type = VHOST_IOTLB_UPDATE; >>>> msg.iotlb.iova = iova; >>>> @@ -108,6 +117,11 @@ vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, >>>> __rte_unused void *addr, >>>> { >>>> struct vhost_msg msg = {}; >>>> >>>> + if (!(dev->protocol_features & (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2))) >>>> { >>>> + PMD_DRV_LOG(ERR, "IOTLB_MSG_V2 not supported by the backend."); >>>> + return -1; >>>> + } >>>> + >>>> msg.type = VHOST_IOTLB_MSG_V2; >>>> msg.iotlb.type = VHOST_IOTLB_INVALIDATE; >>>> msg.iotlb.iova = iova; >>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> index 053f0267ca..96bc6b232d 100644 >>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >>>> @@ -439,11 +439,13 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) >>>> 1ULL << VIRTIO_F_RING_PACKED | \ >>>> 1ULL << VHOST_USER_F_PROTOCOL_FEATURES) >>>> >>>> -#define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ >>>> +#define VHOST_USER_SUPPORTED_PROTOCOL_FEATURES \ >>>> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ >>>> 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ >>>> 1ULL << VHOST_USER_PROTOCOL_F_STATUS) >>>> >>>> +#define VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES \ >>>> + (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2) >>>> int >>>> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, >>>> int cq, int queue_size, const char *mac, char **ifname, >>>> @@ -462,9 +464,13 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char >>>> *path, int queues, >>>> dev->mac_specified = 0; >>>> dev->frontend_features = 0; >>>> dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES; >>>> - dev->protocol_features = VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES; >>>> dev->backend_type = backend_type; >>>> >>>> + if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) >>>> + dev->protocol_features = VHOST_USER_SUPPORTED_PROTOCOL_FEATURES; >>>> + else if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA) >>>> + dev->protocol_features = VHOST_VDPA_SUPPORTED_PROTOCOL_FEATURES; >>>> + >>>> parse_mac(dev, mac); >>>> >>>> if (*ifname) { >>>> @@ -497,8 +503,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char >>>> *path, int queues, >>>> } >>>> >>>> >>>> - if (dev->device_features & >>>> - (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) { >>>> + if ((dev->device_features & (1ULL << >>>> VHOST_USER_F_PROTOCOL_FEATURES) || >>>> + dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) >>> >>> Do you mean: >>> >>> if ((dev->device_features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)) || >>> (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_VDPA)) >> { >>> >>> here? >> >> Indeed! >> >>> Besides, I think it's better to update here as vhost-vdpa also uses >> protocol_features. >>> >> (http://code.dpdk.org/dpdk/v20.08/source/drivers/net/virtio/virtio_user/virtio >> _user_dev.h#L44) >> >> Not sure what you mean here? Can you please elaborate? > > Sorry, I'm not clear on this. It's just a small point about code comment. > > In http://code.dpdk.org/dpdk/v20.11/source/drivers/net/virtio/virtio_user/virtio_user_dev.h#L52, > It says 'negotiated protocol features(vhost-user only)'. Since it's also used by vhost-vdpa now, > maybe it's better to improve it. What do you think? I think it makes sense! Note that in my Virtio PMD rework series, this field disappears, meaning that once this series applied, I will need to rebase my rework on top of it so that vhost-vdpa backend features is moved into vhost_vdpa.c. In the mean time, I will remove the "(vhost-user only)" comment. Thanks, Maxime > Thanks! > Chenbo > >> >> Thanks! >> Maxime >> >>> Thanks! >>> Chenbo >>> >>>> { >>>> if (dev->ops->send_request(dev, >>>> VHOST_USER_GET_PROTOCOL_FEATURES, >>>> &protocol_features)) >>>> -- >>>> 2.26.2 >>> >