Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> > On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> > >On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> > >>On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> > >>>On 29/04/2013 17:14, Jesse Larrew wrote:
> > >>>>On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> > >>>>>On 29/04/2013 16:42, Jesse Larrew wrote:
> > >>>>>>On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> > >>>>>>>On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> > >>>>>>>>Commit 14f9b664 (hw/virtio-net.c: set config size using host 
> > >>>>>>>>features) tries to
> > >>>>>>>>calculate config size based on the host features. But it forgets the
> > >>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will lead a 
> > >>>>>>>>zero config
> > >>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and 
> > >>>>>>>>VIRTIO_NET_F_MQ were
> > >>>>>>>>disabled form command line. Then qemu will crash when user tries to 
> > >>>>>>>>read the
> > >>>>>>>>config of virtio-net.
> > >>>>>>>>
> > >>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the config at 
> > >>>>>>>>least contains
> > >>>>>>>>the mac address.
> > >>>>>>>>
> > >>>>>>>>Cc: Jesse Larrew <address@hidden>
> > >>>>>>>>Signed-off-by: Jason Wang <address@hidden>
> > >>>>>>>>---
> > >>>>>>>>    hw/net/virtio-net.c |    3 ++-
> > >>>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> > >>>>>>>>
> > >>>>>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>>>>>>>index 70c8fce..33a70ef 100644
> > >>>>>>>>--- a/hw/net/virtio-net.c
> > >>>>>>>>+++ b/hw/net/virtio-net.c
> > >>>>>>>>@@ -1264,7 +1264,8 @@ static void 
> > >>>>>>>>virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> > >>>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t 
> > >>>>>>>> host_features)
> > >>>>>>>>    {
> > >>>>>>>>-    int i, config_size = 0;
> > >>>>>>>>+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> > >>>>>>>>+    int i, config_size = feature_sizes[0].end;
> > >>>>>>>This would be cleaner:
> > >>>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> > >>>>>>>
> > >>>>>>>no need for a comment then.
> > >>>>>>>
> > >>>>>>It seems to me that the real problem here is that host_features isn't
> > >>>>>>properly populated before virtio_net_set_config_size() is called. 
> > >>>>>>Looking
> > >>>>>>at virtio_device_init(), we can see why:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>virtio_net_set_config_size() is currently being called as part of the
> > >>>>>>k->init call, but the host_features aren't properly setup until the 
> > >>>>>>device
> > >>>>>>is plugged into the bus using virtio_bus_plug_device().
> > >>>>>>
> > >>>>>>After talking with mdroth, I think the proper way to fix this would 
> > >>>>>>be to
> > >>>>>>extend VirtioDeviceClass to include a calculate_config_size() method 
> > >>>>>>that
> > >>>>>>can be called at the appropriate time during device initialization 
> > >>>>>>like so:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 
> > >>>>>>0) {
> > >>>>>>+       return -1;
> > >>>>>>+   }
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>This would ensure that host_features contains the proper data before 
> > >>>>>>any
> > >>>>>>devices try to make use of it to calculate the config size.
> > >>>>>Good point, I didn't saw that.
> > >>>>>
> > >>>>>but this was not the case with commit 14f9b664 no?
> > >>>>>
> > >>>>I suspect this bug was present in 14f9b664 as well. We just hadn't 
> > >>>>triggered
> > >>>>it yet. I'll confirm this afternoon.
> > >>>Ok, I think there is a problem with your patch:
> > >>>
> > >>>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> > >>>                                   n->config_size);
> > >>>
> > >>>is called in virtio_net_device_init (k->init).
> > >>>
> > >>>Is there a way to resize the config after that?
> > >>>
> > >>Nope. That's definitely a bug. I can send a patch to fix this today. Any
> > >>objection to the method suggested above (extending VirtioDeviceClass)?
> > >This needs more thought. All devices expected everything
> > >is setup during the init call. config size is
> > >likely not the only issue.
> > >
> > >So we need almost all of virtio_bus_plug_device before init,
> > >and then after init send the signal:
> > >
> > >     if (klass->device_plugged != NULL) {
> > >         klass->device_plugged(qbus->parent);
> > >     }
> > 
> > Seems the interesting part is in virtio_pci_device_plugged at the end:
> > 
> >     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >     proxy->host_features = virtio_bus_get_vdev_features(bus,
> > proxy->host_features);
> > 
> > This is and was called after set_config_size, isn't that the issue?
> 
> Basically devices expected everything to be setup at
> the point where init is called.
> We found one bug but let's fix it properly.
> There's no reason to delay parts of setup until after init,
> if we do, we'll trip on more uses of uninitialized data.
> 
> > >
> > >
> > >>>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> > >>>>>>>>            if (host_features & feature_sizes[i].flags) {
> > >>>>>>>>                config_size = MAX(feature_sizes[i].end, 
> > >>>>>>>> config_size);
> > >>>>>>>>-- 
> > >>>>>>>>1.7.1
> > >>>>Jesse Larrew
> > >>>>Software Engineer, KVM Team
> > >>>>IBM Linux Technology Center
> > >>>>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>>>address@hidden
> > >>>>
> > >>
> > >>-- 
> > >>
> > >>Jesse Larrew
> > >>Software Engineer, KVM Team
> > >>IBM Linux Technology Center
> > >>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>address@hidden


Basically this is what I propose. Warning! compile-tested
only. (I also dropped an unused return value).


Signed-off-by: Michael S. Tsirkin <address@hidden>

---

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1596a1c..c75c8ae 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
 #endif
 
 /* Plug the VirtIODevice */
-int virtio_bus_plug_device(VirtIODevice *vdev)
+void virtio_bus_plug_device(VirtIODevice *vdev)
 {
     DeviceState *qdev = DEVICE(vdev);
     BusState *qbus = BUS(qdev_get_parent_bus(qdev));
@@ -64,12 +64,18 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
     bus->bindings.set_host_notifier = klass->set_host_notifier;
     bus->bindings.vmstate_change = klass->vmstate_change;
     virtio_bind_device(bus->vdev, &bus->bindings, qbus->parent);
+}
+
+void virtio_bus_plugged(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtioBusState *bus = VIRTIO_BUS(qbus);
+    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
 
     if (klass->device_plugged != NULL) {
         klass->device_plugged(qbus->parent);
     }
-
-    return 0;
 }
 
 /* Reset the virtio_bus */
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1c2282c..65f69cf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1088,11 +1088,16 @@ static int virtio_device_init(DeviceState *qdev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
+
+    virtio_bus_plug_device(vdev);
+
     assert(k->init != NULL);
     if (k->init(vdev) < 0) {
         return -1;
     }
-    virtio_bus_plug_device(vdev);
+
+    virtio_bus_plugged(vdev);
+
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 311e8c7..471e55e 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -76,7 +76,8 @@ struct VirtioBusState {
     VirtIOBindings bindings;
 };
 
-int virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plugged(VirtIODevice *vdev);
 void virtio_bus_reset(VirtioBusState *bus);
 void virtio_bus_destroy_device(VirtioBusState *bus);
 /* Get the device id of the plugged device. */