From: "KONRAD Frédéric" <fred.konrad@greensocs.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: agraf@suse.de, aliguori@us.ibm.com,
Michael Roth <mdroth@linux.vnet.ibm.com>,
Jason Wang <jasowang@redhat.com>,
qemu-devel@nongnu.org, Jesse Larrew <jlarrew@linux.vnet.ibm.com>,
cornelia.huck@de.ibm.com
Subject: Re: [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len
Date: Tue, 30 Apr 2013 10:47:34 +0200 [thread overview]
Message-ID: <517F8526.9070509@greensocs.com> (raw)
In-Reply-To: <20130429200952.GA19286@redhat.com>
On 29/04/2013 22:09, Michael S. Tsirkin wrote:
> 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 <jlarrew@linux.vnet.ibm.com>
>> Signed-off-by: Jason Wang <jasowang@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)
>> jlarrew@linux.vnet.ibm.com
>>
>>
>> --
>>
>> Jesse Larrew
>> Software Engineer, KVM Team
>> IBM Linux Technology Center
>> Phone: (512) 973-2052 (T/L: 363-2052)
>> jlarrew@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 <mst@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 <mst@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;
>> }
>>
>>
>> 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?
Yes, I think it's possible for PCI and S390, but it seems more difficult
for CCW.
I don't really understand how it's working with CCW devices, there is an
array of host_features?
Cornelia any idea?
>
>> 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.
>
>
next prev parent reply other threads:[~2013-04-30 8:47 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-25 6:21 [Qemu-devel] [PATCH] virtio-net: count VIRTIO_NET_F_MAC when calculating config_len Jason Wang
2013-04-25 6:59 ` Michael S. Tsirkin
2013-04-25 7:02 ` Jason Wang
2013-04-25 7:06 ` Michael S. Tsirkin
2013-04-25 7:52 ` Jason Wang
2013-04-29 14:42 ` Jesse Larrew
2013-04-29 14:55 ` KONRAD Frédéric
2013-04-29 15:14 ` Jesse Larrew
2013-04-29 15:29 ` KONRAD Frédéric
2013-04-29 15:55 ` Jesse Larrew
2013-04-29 16:02 ` Michael S. Tsirkin
2013-04-29 16:14 ` KONRAD Frédéric
2013-04-29 16:21 ` Michael S. Tsirkin
2013-04-29 16:30 ` Michael S. Tsirkin
2013-04-29 16:41 ` KONRAD Frédéric
2013-04-29 17:01 ` Michael S. Tsirkin
2013-04-29 17:23 ` KONRAD Frédéric
2013-04-29 17:52 ` Michael S. Tsirkin
2013-04-29 18:01 ` KONRAD Frédéric
2013-04-29 18:15 ` Michael S. Tsirkin
2013-04-29 18:45 ` KONRAD Frédéric
2013-04-29 20:09 ` Michael S. Tsirkin
2013-04-30 8:47 ` KONRAD Frédéric [this message]
2013-04-30 9:02 ` Michael S. Tsirkin
2013-05-02 9:02 ` Cornelia Huck
2013-05-02 8:59 ` Cornelia Huck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=517F8526.9070509@greensocs.com \
--to=fred.konrad@greensocs.com \
--cc=agraf@suse.de \
--cc=aliguori@us.ibm.com \
--cc=cornelia.huck@de.ibm.com \
--cc=jasowang@redhat.com \
--cc=jlarrew@linux.vnet.ibm.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).