On Wed, Apr 20, 2022 at 04:58:51PM +0200, Maciej Szymański wrote:
> On 20.04.2022 13:10, Michael S. Tsirkin wrote:
> > On Wed, Apr 20, 2022 at 10:17:27AM +0200, Maciej Szymański wrote:
> > > > > > > > Hmm so we have this:
> > > > > > > >
> > > > > > > >
> > > > > > > > if ((dev->features ^ features) & NETIF_F_GRO_HW) {
> > > > > > > > if (vi->xdp_enabled)
> > > > > > > > return -EBUSY;
> > > > > > > >
> > > > > > > > if (features & NETIF_F_GRO_HW)
> > > > > > > > offloads = vi->guest_offloads_capable;
> > > > > > > > else
> > > > > > > > offloads = vi->guest_offloads_capable
> > > > > > > > &
> > > > > > > > ~GUEST_OFFLOAD_GRO_HW_MASK;
> > > > > > > >
> > > > > > > > err = virtnet_set_guest_offloads(vi,
> > > > > > > > offloads);
> > > > > > > > if (err)
> > > > > > > > return err;
> > > > > > > > vi->guest_offloads = offloads;
> > > > > > > > }
> > > > > > > >
> > > > > > > > which I guess should have prevented virtnet_set_guest_offloads
> > > > > > > > from ever running.
> > > > > > > >
> > > > > > > > From your description it sounds like you have observed this
> > > > > > > > in practice, right?
> > > > > > > >
> > > > > Yes. I have proprietary virtio-net device which advertises following
> > > > > guest offload features :
> > > > > - VIRTIO_NET_F_GUEST_CSUM
> > > > > - VIRTIO_NET_F_GUEST_TSO4
> > > > > - VIRTIO_NET_F_GUEST_TSO6
> > > > > - VIRTIO_NET_F_GUEST_UFO
> > > > >
> > > > > This feature set passes the condition in virtnet_set_features.
> > So why isn't dev->features equal to features?
> >
> I just double verified and found that my device advertises
> VIRTIO_NET_GUEST_TSO4 and VIRTIO_NET_F_GUEST_TSO6 but not
> VIRTIO_NET_F_GUEST_CSUM as mentioned before.
So, your device is out of spec:
VIRTIO_NET_F_GUEST_TSO4 Requires VIRTIO_NET_F_GUEST_CSUM.
And
The device MUST NOT offer a feature which requires another feature which was
not offered.
Is this a production device? Can it be fixed?
> That leads to the following situation :
>
> in virtio_probe :
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> dev->features |= NETIF_F_RXCSUM;
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
> dev->features |= NETIF_F_GRO_HW;
> if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))
> dev->hw_features |= NETIF_F_GRO_HW;
>
>
> while in netdev_fix_features :
>
> if (!(features & NETIF_F_RXCSUM)) {
> /* NETIF_F_GRO_HW implies doing RXCSUM since every packet
> * successfully merged by hardware must also have the
> * checksum verified by hardware. If the user does not
> * want to enable RXCSUM, logically, we should disable GRO_HW.
> */
> if (features & NETIF_F_GRO_HW) {
> netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no RXCSUM
> feature.\n");
> features &= ~NETIF_F_GRO_HW;
> }
> }
>
> As result dev->features and features passed from
> __netdev_update_features differs exactly in NETIF_F_GRO_HW bit.
>
>
> Please mind our privacy
> notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/>
> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13
> DSGVO finden Sie
> hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
