On 29/04/2013 20:15, Michael S. Tsirkin wrote:
On Mon, Apr 29, 2013 at 08:01:52PM +0200, KONRAD Frédéric wrote:
On 29/04/2013 19:52, Michael S. Tsirkin wrote:
On Mon, Apr 29, 2013 at 07:23:49PM +0200, KONRAD Frédéric wrote:
On 29/04/2013 19:01, Michael S. Tsirkin wrote:
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;
}
Not sure this is what you want to do.
The device would be plugged before it is inited :/.
I think this is exacly what we want to do.
In fact, this is what other buses do, because
devices simply can't init properly if they
do not know on which bus they reside.
E.g. with pci:
do_pci_register_device (adds device on bus)
init
We can add an analog of hotplug bus callback
if bus wants to get notified after device initialization.
I don't see a need for this though.
Do you?
No, as we don't want to allow virtio-device hotplug.
but look at the plugged callback in virtio-pci:
pci_set_word(config + PCI_SUBSYSTEM_ID,
virtio_bus_get_vdev_id(bus));
proxy->host_features = virtio_bus_get_vdev_features(bus,
proxy->host_features);
are impossible before the virtio-net init.
At this point I have to admit I don't understand what's
going on any more.
virtio_net_instance_init sets config size to
some value, then virtio_net_set_config_size overrides it...
Help!
I am guessing it's this hack that backfires somehow.
It would be interferring if virtio_net_set_config_size was not called.
The best I think is to set the config size in the virtio_net_init function,
then remove the instance init.
The issue is that in the init function, the host_feature is not completely
computed:
it lacks the three line in virtio pci plugged function.
Maybe we can move the two firsts line in the virtio_net_pci_init before the
init if they are usefull in the
config_size computing:
proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;