在 2022/4/20 15:32, Maciej Szymański 写道:
On 20.04.2022 08:35, Michael S. Tsirkin wrote:
On Wed, Apr 20, 2022 at 11:07:00AM +0800, Jason Wang wrote:
On Tue, Apr 19, 2022 at 11:03 PM Michael S. Tsirkin <mst@xxxxxxxxxx>
wrote:
On Tue, Apr 19, 2022 at 04:12:31PM +0200, Maciej Szymański wrote:
Hello,
I've found a problem in virtio-net driver.
If virtio-net backend device advertises guest offload features,
there is
an unpermitted usage of control virtqueue before driver is
initialized.
According to VIRTIO specification 2.1.2 :
"The device MUST NOT consume buffers or send any used buffer
notifications to the driver before DRIVER_OK."
Right.
During an initialization, driver calls register_netdevice which
invokes
callback function virtnet_set_features from __netdev_update_features.
If guest offload features are advertised by the device,
virtnet_set_guest_offloads is using virtnet_send_command to write and
read from VQ.
That leads to initialization stuck as device is not permitted yet
to use VQ.
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.
When I disable guest offloads in my device - virtnet_set_guest_offloads
is not called and driver initialization completes successfully.
I have attached a patch for kernel 5.18-rc3 which fixes the
problem by
deferring feature set after virtio driver initialization.
Best Regards,
--
Maciej Szymański
Senior Staff Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Phone: +49 30 60 98 54 0 -86
Fax: +49 30 60 98 54 0 -99
E-Mail: maciej.szymanski@xxxxxxxxxxxxxxx
www.opensynergy.com
Handelsregister/Commercial Registry: Amtsgericht Charlottenburg,
HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah
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/>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 87838cb..a44462d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -264,6 +264,8 @@ struct virtnet_info {
unsigned long guest_offloads;
unsigned long guest_offloads_capable;
+ netdev_features_t features;
+
I don't much like how we are forced to keep a copy of features
here :( At least pls add a comment explaining what's going on,
who owns this etc.
Personally I don't like this as well but I think it is only way to keep
the features for deferred set.
/* failover when STANDBY feature enabled */
struct failover *failover;
};
@@ -2976,6 +2978,15 @@ static int
virtnet_get_phys_port_name(struct net_device *dev, char *buf,
static int virtnet_set_features(struct net_device *dev,
netdev_features_t features)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ vi->features = features;
+
+ return 0;
+}
Looks like this breaks changing features after initialization -
these will never be propagated to hardware now.
Indeed. An original function won't be used.
Yes, I think we need to have a check and only defer the setting when
virtio device is not ready.
Right. I have updated my patch to check status to deffer feature set.
That will also solve the problem from comment above.
Thanks
I think we should first understand how does the issue trigger,
is this a theoretical or a practical issue.
As mentioned above - practical issue. It does not occur for f.e. QEMU
built-in virtio-net device as it does not advertise guest offload
features.
Qemu has that support (enabled by default):
DEFINE_PROP_BIT64("ctrl_guest_offloads", VirtIONet, host_features,
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, true),
But Qemu doesn't check DRIVER_OK when processing ctrl vq, that's why we
never see any report before.
Thanks
+
+static int virtnet_set_features_deferred(struct net_device *dev,
+ netdev_features_t features)
{
struct virtnet_info *vi = netdev_priv(dev);
u64 offloads;
@@ -3644,6 +3655,13 @@ static int virtnet_probe(struct
virtio_device *vdev)
virtio_device_ready(vdev);
+ /* Deferred feature set after device ready */
+ err = virtnet_set_features_deferred(dev, vi->features);
It seems that if this is called e.g. for a device without a CVQ and
there are things that actually need to change then it will BUG_ON.
+ if (err) {
+ pr_debug("virtio_net: set features failed\n");
+ goto free_unregister_netdev;
+ }
+
err = virtnet_cpu_notif_add(vi);
if (err) {
pr_debug("virtio_net: registering cpu notifier
failed\n");
--
MST
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
