[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calcula
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len |
|
Date: |
Mon, 29 Apr 2013 23:09:52 +0300 |
On Mon, Apr 29, 2013 at 08:45:50PM +0200, KONRAD Frédéric wrote:
> 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;
Bus can set VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
later, this is not going to affect anything.
Reason is, VIRTIO_F_NOTIFY_ON_EMPTY and VIRTIO_F_BAD_FEATURE
are compatibility hacks which is why we dont
have them for s390. It's probably a bug that
we have them for virtio-ccw.
>
> Then compute the last one directly in the init function which is the harder:
>
> virtio_net_get_features
The real fix is to set features in init.
Can we move host_features to struct VirtIODevice, and
init to the device init function?
The reason we didn't do this initially is exactly
because we need to specify them in -device flag,
and there was no way to do this for VirtIODevice,
since it's the proxy that is instanciated.
Does the new bus infrastructure allow this?
> Note that there is in virtio_net_get_features:
>
> features |= (1 << VIRTIO_NET_F_MAC);
>
> Which is exacty the issue the initial patch is solving.
>
> Fred
Yes. the lifetime of host features is as follows:
- configured in device by user, mostly set to "on" by default
- depending on device specific logic,
override some features - mostly to "off",
but we also force some required features to "on"
- Two exceptions (notify on empty+bad) are transport
specific. they are also not user-controllable.
--
MST
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, (continued)
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/29
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, KONRAD Frédéric, 2013/04/30
- Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len, Michael S. Tsirkin, 2013/04/30