From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 12E741B58A; Thu, 22 Nov 2018 19:51:15 +0100 (CET) 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 mx1.redhat.com (Postfix) with ESMTPS id 5422B46202; Thu, 22 Nov 2018 18:51:14 +0000 (UTC) Received: from [10.36.112.63] (ovpn-112-63.ams2.redhat.com [10.36.112.63]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7F99717C56; Thu, 22 Nov 2018 18:51:11 +0000 (UTC) To: Ferruh Yigit , dev@dpdk.org, thomas@monjalon.net, tiwei.bie@intel.com, zhihong.wang@intel.com, jfreiman@redhat.com Cc: stable@dpdk.org References: <20181122170922.15007-1-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: <5437708e-d8cf-fe17-6fd6-fad67b3a0eb7@redhat.com> Date: Thu, 22 Nov 2018 19:51:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 22 Nov 2018 18:51:14 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] vhost: fix packed ring defines declaration 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: , X-List-Received-Date: Thu, 22 Nov 2018 18:51:15 -0000 On 11/22/18 7:23 PM, Ferruh Yigit wrote: > On 11/22/2018 5:09 PM, Maxime Coquelin wrote: >> The packed ring defines were declared only if kernel >> header does not declare them. >> The problem is that they are not applied in upstream kernel, >> and some changes in the names have been required. >> >> This patch declares the defines unconditionally, which >> fixes potential build issues. > > +1 to address possible build issues. > >> >> Fixes: 297b1e7350f6 ("vhost: add virtio packed virtqueue defines") >> Cc: stable@dpdk.org >> >> Signed-off-by: Maxime Coquelin >> --- >> lib/librte_vhost/vhost.h | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h >> index 760f42192..5218f1b12 100644 >> --- a/lib/librte_vhost/vhost.h >> +++ b/lib/librte_vhost/vhost.h >> @@ -219,13 +219,6 @@ struct vhost_msg { >> >> #define VIRTIO_F_RING_PACKED 34 >> >> -#define VRING_DESC_F_NEXT 1 >> -#define VRING_DESC_F_WRITE 2 >> -#define VRING_DESC_F_INDIRECT 4 > > Why these are not re-defined below? Not used? These defines are in kernel headers since the beginning of virtio support, so there are no reasons to keep them (and they aren't packed- ring specific). I can let them if you prefer, it does not hurt but shouldn't be here. >> - >> -#define VRING_DESC_F_AVAIL (1ULL << 7) >> -#define VRING_DESC_F_USED (1ULL << 15) >> - >> struct vring_packed_desc { >> uint64_t addr; >> uint32_t len; >> @@ -233,16 +226,23 @@ struct vring_packed_desc { >> uint16_t flags; >> }; >> >> -#define VRING_EVENT_F_ENABLE 0x0 >> -#define VRING_EVENT_F_DISABLE 0x1 >> -#define VRING_EVENT_F_DESC 0x2 >> - >> struct vring_packed_desc_event { >> uint16_t off_wrap; >> uint16_t flags; >> }; >> #endif >> >> +/* >> + * Declare below packed ring defines unconditionally >> + * as Kernel header might use different names. >> + */ >> +#define VRING_DESC_F_AVAIL (1ULL << 7) >> +#define VRING_DESC_F_USED (1ULL << 15) >> + >> +#define VRING_EVENT_F_ENABLE 0x0 >> +#define VRING_EVENT_F_DISABLE 0x1 >> +#define VRING_EVENT_F_DESC 0x2 > > What if Linux changes mind and uses old names again, build will fail again. If > related part in Linux is not released yet, what do you think being on safe side > and adding these defines with "#ifndef" wrap? There are the "old" ones. In any case it will work (I tested it). In a future release, I plan to get rid of this dependency with Kernel which only causes problems. There was a benefit at the beginning when all needed declarations where in the Kernel headers. But now, every time we add a new virtio feature, we have to check kernel supports it and if not define it. Thanks, Maxime >> + >> /* >> * Available and used descs are in same order >> */ >> >