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 B5F931B536; Thu, 22 Nov 2018 20:50:32 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C7CD8C028358; Thu, 22 Nov 2018 19:50:31 +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 DA5F05C25A; Thu, 22 Nov 2018 19:50:28 +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> <5437708e-d8cf-fe17-6fd6-fad67b3a0eb7@redhat.com> <590b480d-e2db-b299-82da-bc96a556a5e9@intel.com> From: Maxime Coquelin Message-ID: <8f2a2b11-27f0-0602-c9b0-f40c2d6384c7@redhat.com> Date: Thu, 22 Nov 2018 20:50:26 +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: <590b480d-e2db-b299-82da-bc96a556a5e9@intel.com> 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.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 22 Nov 2018 19:50:32 +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 19:50:33 -0000 On 11/22/18 8:20 PM, Ferruh Yigit wrote: > On 11/22/2018 6:51 PM, Maxime Coquelin wrote: >> >> >> 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. > > Not required, I just want to confirm nothing missed. > >> >>>> - >>>> -#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). > > I see they are the old ones but is there any chance that kernel uses them again > (for some odd reason) ? Which can break the build. Should we add a protection? Yes, there is a chance, but that would not break build, it will just override 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 >>>> */ >>>> >>> >