From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kavanagh, Mark B" Subject: Re: [PATCH] vhost: disable reply-ack protocol feature if iommu feature disabled Date: Mon, 6 Nov 2017 14:22:52 +0000 Message-ID: References: <20171103175735.24603-1-maxime.coquelin@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: Maxime Coquelin , "dev@dpdk.org" , "yliu@fridaylinux.org" , "thomas@monjalon.net" , "ktraynor@redhat.com" Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id AF07B1B2C8 for ; Mon, 6 Nov 2017 15:22:56 +0100 (CET) In-Reply-To: <20171103175735.24603-1-maxime.coquelin@redhat.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >Sent: Friday, November 3, 2017 5:58 PM >To: dev@dpdk.org; yliu@fridaylinux.org; Kavanagh, Mark B >; thomas@monjalon.net; ktraynor@redhat.com >Cc: Maxime Coquelin >Subject: [PATCH] vhost: disable reply-ack protocol feature if iommu featur= e >disabled > >If the application has disabled VIRTIO_F_IOMMU_PLATFORM, disable >VHOST_USER_PROTOCOL_F_REPLY_ACK protocol feature that is only >mandatory with IOMMU for now. > >This is done to provide a way for the application to support >multiqueue with old Qemu versions (v2.7.0 to v2.9.0) that have >reply-ack feature broken. > >Signed-off-by: Maxime Coquelin >--- > >This is an alternative to my proposition of adding a new flag at >vhost register time. Advantage of this solution is that it does >not bring API change. > > lib/librte_vhost/vhost_user.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c >index 1f6cba4b9..e35218688 100644 >--- a/lib/librte_vhost/vhost_user.c >+++ b/lib/librte_vhost/vhost_user.c >@@ -878,6 +878,27 @@ vhost_user_set_vring_enable(struct virtio_net **pdev, > } > > static void >+vhost_user_get_protocol_features(struct virtio_net *dev, >+ struct VhostUserMsg *msg) >+{ >+ uint64_t features, protocol_features =3D VHOST_USER_PROTOCOL_FEATURES; >+ >+ rte_vhost_driver_get_features(dev->ifname, &features); >+ >+ /* >+ * REPLY_ACK protocol feature is only mandatory for now >+ * for IOMMU feature. If IOMMU is explicitly disabled by the >+ * application, disable also REPLY_ACK feature for older buggy >+ * Qemu versions (from v2.7.0 to v2.9.0). >+ */ >+ if (!(features & (1ULL << VIRTIO_F_IOMMU_PLATFORM))) Hi Maxime, Thanks for this patch - I like this approach, as it maintains API compatibi= lity. Now for the gotchas, unfortunately: - VIRTIO_F_IOMMU_PLATFORM is defined in vhost.h, as opposed to rte_vhost.h,= so it is not exposed to OvS :( - Currently, we use other similar macros in OvS (VIRTIO_NET_F_CSUM, VIRTIO_= NET_F_HOST_TSO[4|6]); however, we obtain the definition of these from a ker= nel header file, as opposed to a DPDK header (linux/virtio_net.h) -- We could adopt the same approach for VIRTIO_F_IOMMU_PLATFORM, and inclu= de linux/virtio_config.h; unfortunately, the VIRTIO_F_IOMMU_PLATFORM macro = is only defined from kernel 4.8, which creates another problem entirely. One potential solution is to move the VIRTIO_* definitions to rte_vhost.h, = but at this stage in the DPDK release cycle, that's probably a tall ask. Any thoughts? Thanks again, Mark >+ protocol_features &=3D ~(1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK); >+ >+ msg->payload.u64 =3D protocol_features; >+ msg->size =3D sizeof(msg->payload.u64); >+} >+ >+static void > vhost_user_set_protocol_features(struct virtio_net *dev, > uint64_t protocol_features) > { >@@ -1248,8 +1269,7 @@ vhost_user_msg_handler(int vid, int fd) > break; > > case VHOST_USER_GET_PROTOCOL_FEATURES: >- msg.payload.u64 =3D VHOST_USER_PROTOCOL_FEATURES; >- msg.size =3D sizeof(msg.payload.u64); >+ vhost_user_get_protocol_features(dev, &msg); > send_vhost_reply(fd, &msg); > break; > case VHOST_USER_SET_PROTOCOL_FEATURES: >-- >2.13.6