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 <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>
> 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 <m...@redhat.com>
---
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. */
