Re: [PATCH v2] virtio: enable indirect descriptors feature

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 6 Sep 2016 06:49:26 +0000
"Pierre Pfister (ppfister)" <ppfister@cisco.com> wrote:

> > Le 5 sept. 2016 à 23:08, Stephen Hemminger <stephen@networkplumber.org> a écrit :
> > 
> > On Mon, 5 Sep 2016 16:24:13 +0200
> > Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
> >   
> >> Thanks Pierre for sending the fix.
> >> 
> >> Minor comments below:
> >> 
> >> On 09/05/2016 08:52 AM, Pierre Pfister (ppfister) wrote:  
> >>> Indirect descriptors support was disabled by commit 4a92b67151be11,
> >>> presumably by accident as it was correctly supported.
> >>> 
> >>> 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.
> >>>   
> >> 
> >> You should add the below line:
> >> Fixes: 4a92b671 ("virtio: clarify feature bit handling")
> >> 
> >> 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.
> >> 
> >> Other than that, you can add:
> >> Reviewed-by: Maxime Coquelin <maxime.coquelin@¶edhat.com>  
> > 
> > The patch is correct, but it doesn't fix a regression.
> > 
> > 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 during
> > guest negotiation at the time.
> > 
> > static void
> > virtio_negotiate_features(struct virtio_hw *hw)
> > {
> > -	uint32_t host_features, mask;
> > -
> > -	/* checksum offload not implemented */
> > -	mask = 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 |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
> > -	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
> > -	mask |= VTNET_LRO_FEATURES;
> > -
> > -	/* not negotiating INDIRECT descriptor table support */
> > -	mask |= VIRTIO_RING_F_INDIRECT_DESC;
> > +	uint32_t host_features;
> > 
> > 	/* Prepare guest_features: feature that driver wants to support */
> > -	hw->guest_features = VTNET_FEATURES & ~mask;
> > +	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
> > 	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
> > 		hw->guest_features);
> > 
> > Therefore INDIRECT descriptors were always disabled!  Don't blame any commit.
> > Use of indirect descriptors by DPDK did not happen until a later change.  
> 
> Oops. My bad, sorry. I will remove the blame from the commit message.
> It is strange though that 6dc5de3a6aef adds indirect buffer support but does 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.