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

[Michael S. Tsirkin]

On Mon, Apr 29, 2013 at 06:41:08PM +0200, KONRAD Frédéric wrote:
> On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> >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>
> Which tree are you using? Is it up to date?

Heh, more changes came in.  So now, it's even simpler:

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

---

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index aab72ff..447ba16 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));
@@ -51,8 +51,6 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
     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 0f88c25..c5228e6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1091,11 +1091,11 @@ 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);
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 9ed60f9..da84141 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -72,7 +72,7 @@ struct VirtioBusState {
     VirtIODevice *vdev;
 };
 
-int virtio_bus_plug_device(VirtIODevice *vdev);
+void virtio_bus_plug_device(VirtIODevice *vdev);
 void virtio_bus_reset(VirtioBusState *bus);
 void virtio_bus_destroy_device(VirtioBusState *bus);
 /* Get the device id of the plugged device. */