From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [PATCH v2] virtio: enable indirect descriptors feature Date: Tue, 6 Sep 2016 08:32:13 -0700 Message-ID: <20160906083213.6d2cc3bd@xeon-e3> References: <516F65D3-4706-4CC5-916B-6ECD29CBE177@cisco.com> <20160905022028.GF30752@yliu-dev.sh.intel.com> <6EFF45F1-172E-4470-B4D7-AED90474DE50@cisco.com> <1dfca8cf-e5ef-1040-8a40-d617ed03e5c0@redhat.com> <20160905140824.1f0c80a2@xeon-e3> <27D7470D-6FC7-4922-92A4-341CAD203689@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: Maxime Coquelin , Yuanhan Liu , "dev@dpdk.org" To: "Pierre Pfister (ppfister)" Return-path: Received: from mail-pf0-f178.google.com (mail-pf0-f178.google.com [209.85.192.178]) by dpdk.org (Postfix) with ESMTP id 3FF6B68D1 for ; Tue, 6 Sep 2016 17:32:04 +0200 (CEST) Received: by mail-pf0-f178.google.com with SMTP id w87so17555363pfk.2 for ; Tue, 06 Sep 2016 08:32:04 -0700 (PDT) In-Reply-To: <27D7470D-6FC7-4922-92A4-341CAD203689@cisco.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, 6 Sep 2016 06:49:26 +0000 "Pierre Pfister (ppfister)" wrote: > > Le 5 sept. 2016 =C3=A0 23:08, Stephen Hemminger a =C3=A9crit : > >=20 > > On Mon, 5 Sep 2016 16:24:13 +0200 > > Maxime Coquelin wrote: > > =20 > >> Thanks Pierre for sending the fix. > >>=20 > >> Minor comments below: > >>=20 > >> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote: =20 > >>> Indirect descriptors support was disabled by commit 4a92b67151be11, > >>> presumably by accident as it was correctly supported. > >>>=20 > >>> This patch simply adds VIRTIO_RING_F_INDIRECT_DESC back to > >>> the supported features bit mask, hence enabling the use of > >>> indirect descriptors when the feature is negociated with the > >>> device. > >>> =20 > >>=20 > >> You should add the below line: > >> Fixes: 4a92b671 ("virtio: clarify feature bit handling") > >>=20 > >> Also, maybe we should consider add stable@dpdk.org in cc:, > >> because the regression was introduced before v16.07 final tag. > >> But the problem is that all the final validation has been done > >> without this feature enabled, and it impact quite a few lines of > >> code in Virtio PMD. > >>=20 > >> Other than that, you can add: > >> Reviewed-by: Maxime Coquelin =20 > >=20 > > The patch is correct, but it doesn't fix a regression. > >=20 > > The original virtio DPDK did not support INDIRECT descriptors at all. > > The original code in virtio_negotiate features was the inverse of what = it is now. > > Read carefully, the values in mask were the bits that were rejected dur= ing > > guest negotiation at the time. > >=20 > > static void > > virtio_negotiate_features(struct virtio_hw *hw) > > { > > - uint32_t host_features, mask; > > - > > - /* checksum offload not implemented */ > > - mask =3D VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM; > > - > > - /* TSO and LRO are only available when their corresponding > > - * checksum offload feature is also negotiated. > > - */ > > - mask |=3D VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NE= T_F_HOST_ECN; > > - mask |=3D VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_= NET_F_GUEST_ECN; > > - mask |=3D VTNET_LRO_FEATURES; > > - > > - /* not negotiating INDIRECT descriptor table support */ > > - mask |=3D VIRTIO_RING_F_INDIRECT_DESC; > > + uint32_t host_features; > >=20 > > /* Prepare guest_features: feature that driver wants to support */ > > - hw->guest_features =3D VTNET_FEATURES & ~mask; > > + hw->guest_features =3D VIRTIO_PMD_GUEST_FEATURES; > > PMD_INIT_LOG(DEBUG, "guest_features before negotiate =3D %x", > > hw->guest_features); > >=20 > > Therefore INDIRECT descriptors were always disabled! Don't blame any c= ommit. > > Use of indirect descriptors by DPDK did not happen until a later change= . =20 >=20 > Oops. My bad, sorry. I will remove the blame from the commit message. > It is strange though that 6dc5de3a6aef adds indirect buffer support but d= oes not > provide a way to enable it. I agree that indirect descriptors should be negotiated. Probably the lack of the bit is left over from testing the three ways to send.