On Thu, Apr 21, 2022 at 4:18 AM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Wed, Apr 20, 2022 at 08:57:18PM +0200, Maciej Szymański wrote:
> > On 20.04.2022 19:54, Michael S. Tsirkin wrote:
> > > 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?
> > The problem seems to be more complicated. In fact
> > VIRTIO_NET_F_GUEST_CSUM is offered by our device, but during feature
> > negotiation it is being dropped.
> > This most likely does not happen when we use MMIO, but for some reason
> > happens in QEMU for VHOST_USER + PCI.
> > I need to investigate this more deeply...
>
>
> I don't see where linux would drop it. I suspect it's dropped between
> QEMU and vhost user. I'd say let's fix it in the device first.
> We can next consider marking vqs broken before device is ready -
> Jason what do you think?
Yes, we can do that.
Thanks
> Finally, we can add code to avoid acking dependent features
> if the feature they depend on has not been negotiated - doing
> so is also a spec violation after all.
>
>
> >
> > >
> > > > 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/>
> >
> >
> > 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
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
