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 DF7F22B89 for ; Thu, 7 Dec 2017 09:39:17 +0100 (CET) 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 mx1.redhat.com (Postfix) with ESMTPS id 34BC0C02C005; Thu, 7 Dec 2017 08:39:17 +0000 (UTC) Received: from [10.36.112.55] (ovpn-112-55.ams2.redhat.com [10.36.112.55]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6E4F360C34; Thu, 7 Dec 2017 08:39:10 +0000 (UTC) To: Tiwei Bie Cc: dev@dpdk.org, yliu@fridaylinux.org, jianfeng.tan@intel.com, lprosek@redhat.com, lersek@redhat.com References: <20171206092048.3568-1-maxime.coquelin@redhat.com> <20171206092048.3568-2-maxime.coquelin@redhat.com> <20171207080820.jkzcam47deipsqcv@debian-xvivbkq> From: Maxime Coquelin Message-ID: <041f54c0-4222-eedc-615d-4aa6c2503438@redhat.com> Date: Thu, 7 Dec 2017 09:39:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171207080820.jkzcam47deipsqcv@debian-xvivbkq> 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.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 07 Dec 2017 08:39:17 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v3 1/4] vhost: prevent features to be changed while device is running 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, 07 Dec 2017 08:39:18 -0000 On 12/07/2017 09:08 AM, Tiwei Bie wrote: > On Wed, Dec 06, 2017 at 10:20:45AM +0100, Maxime Coquelin wrote: >> As section 2.2 of the Virtio spec states about features >> negotiation: >> "During device initialization, the driver reads this and tells >> the device the subset that it accepts. The only way to >> renegotiate is to reset the device." >> >> This patch implements a check to prevent illegal features change >> while the device is running. >> >> One exception is the VHOST_F_LOG_ALL feature bit, which is enabled >> when live-migration is initiated. But this feature is not negotiated >> with the Virtio driver, but directly with the Vhost master. >> >> Signed-off-by: Maxime Coquelin >> --- >> lib/librte_vhost/vhost_user.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >> index f4c7ce462..2d86c0ca8 100644 >> --- a/lib/librte_vhost/vhost_user.c >> +++ b/lib/librte_vhost/vhost_user.c >> @@ -183,7 +183,22 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features) >> return -1; >> } >> >> - if ((dev->flags & VIRTIO_DEV_RUNNING) && dev->features != features) { >> + if (dev->features == features) >> + return 0; >> + > > We couldn't return directly when dev->features == features. > Otherwise, if the features provided by virtio driver is 0, > dev->vhost_hlen won't get a chance to be initialized. Good catch. Either we do : if ((dev->features == features) && dev->vhost_len) return 0; Or we could initialize dev->vhost_len to sizeof(struct virtio_net_hdr) at alloc time. I prefer the former, what do you think? Thanks, Maxime > Best regards, > Tiwei Bie >