On Tue, 6 Sep 2016 06:49:26 +0000
"Pierre Pfister (ppfister)" <ppfister at cisco.com> wrote:
> > Le 5 sept. 2016 ? 23:08, Stephen Hemminger <stephen at networkplumber.org>
> > a ?crit :
> >
> > On Mon, 5 Sep 2016 16:24:13 +0200
> > Maxime Coquelin <maxime.coquelin at 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 at 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.
