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 <jlar...@linux.vnet.ibm.com>
> > >>>>>>>>Signed-off-by: Jason Wang <jasow...@redhat.com>
> > >>>>>>>>---
> > >>>>>>>> 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)
> > >>>>jlar...@linux.vnet.ibm.com
> > >>>>
> > >>
> > >>--
> > >>
> > >>Jesse Larrew
> > >>Software Engineer, KVM Team
> > >>IBM Linux Technology Center
> > >>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>jlar...@linux.vnet.ibm.com
Basically this is what I propose. Warning! compile-tested
only. (I also dropped an unused return value).
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
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. */
