[Qemu-devel] [PATCH 0/3] Notify devices when a bus is attached

[Qemu-devel] [PATCH 0/3] Notify devices when a bus is attached

[Jesse Larrew]

The virtio-net driver can determine the required size of the config struct
dynamically by inspecting the feature bits in host_features. The natural
place to perform this calculation is within the driver's init routine.
However, host_features isn't set until later when the device is plugged
into a bus.

VirtioBusClass includes a callback method, device_plugged(), that allows
the bus to perform additional setup tasks when a device is plugged into the
bus. This patch set similarly extends VirtioDeviceClass to add a callback,
bus_plugged(), to allow devices the same setup opportunity.

This allows virtio-net to defer the config size calculation until
host_features is available, which I think will be a better long-term fix to
the problem addressed in e9016ee2bda1b7757072b856b2196f691aee3388.

 [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass
 [PATCH 2/3] virtio-net: implement bus_plugged()
 [PATCH 3/3] virtio-net: revert MAC address workaround

 hw/net/virtio-net.c        | 20 +++++++++++++++++++-
 hw/virtio/virtio.c         |  3 +++
 include/hw/virtio/virtio.h |  1 +
 3 files changed, 23 insertions(+), 1 deletion(-)

[Qemu-devel] [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass

[Jesse Larrew]

Virtio devices are initialized prior to plugging them into a bus. However,
other initializations (such as host_features) don't occur until after the
device is plugged into the bus. If a device needs to modify it's
configuration based on host_features, then it needs to be notified when the
bus is attached and host_features is available for use.

This patch extends struct VirtioDeviceClass to add a bus_plugged() method.
If implemented by a device, it will be called after the device is attached
to a bus.

Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
---
 hw/virtio/virtio.c         | 3 +++
 include/hw/virtio/virtio.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 8176c14..96735fa 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1114,6 +1114,9 @@ static int virtio_device_init(DeviceState *qdev)
         return -1;
     }
     virtio_bus_plug_device(vdev);
+    if (k->bus_plugged) {
+        k->bus_plugged(vdev);
+    }
     return 0;
 }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 6afdfd8..31fad30 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -156,6 +156,7 @@ typedef struct VirtioDeviceClass {
      * must mask in frontend instead.
      */
     void (*guest_notifier_mask)(VirtIODevice *vdev, int n, bool mask);
+    void (*bus_plugged)(VirtIODevice *vdev);
 } VirtioDeviceClass;
 
 void virtio_init(VirtIODevice *vdev, const char *name,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass

[Andreas Färber]

Hi,

Am 04.06.2013 18:22, schrieb Jesse Larrew:
> Virtio devices are initialized prior to plugging them into a bus. However,
> other initializations (such as host_features) don't occur until after the
> device is plugged into the bus. If a device needs to modify it's
> configuration based on host_features, then it needs to be notified when the
> bus is attached and host_features is available for use.
> 
> This patch extends struct VirtioDeviceClass to add a bus_plugged() method.
> If implemented by a device, it will be called after the device is attached
> to a bus.
> 
> Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>

I think this is backwards...

First of all, why is host_features not available before?

A hook on the bus makes sense because it allows central handling for any
devices on that bus.
However for a device, first TypeInfo::instance_init is run, then
qdev_set_parent_bus() connects the bus and finally DeviceClass::realize
is run - and we want to postpone realize further in the future.

So why can't this be in VirtioDevice's or VirtIONet's realize method? At
realize time we should definitely be on the bus in this case. I.e.,
create vdev->config only after we know how large it needs to be rather
than creating and later resizing it, which might fail.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass

[Jesse Larrew]

On 06/04/2013 12:35 PM, Andreas Färber wrote:
> Hi,
> 

Hi Andreas!

Thanks for the review. :)

> Am 04.06.2013 18:22, schrieb Jesse Larrew:
>> Virtio devices are initialized prior to plugging them into a bus. However,
>> other initializations (such as host_features) don't occur until after the
>> device is plugged into the bus. If a device needs to modify it's
>> configuration based on host_features, then it needs to be notified when the
>> bus is attached and host_features is available for use.
>>
>> This patch extends struct VirtioDeviceClass to add a bus_plugged() method.
>> If implemented by a device, it will be called after the device is attached
>> to a bus.
>>
>> Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> 
> I think this is backwards...
> 
> First of all, why is host_features not available before?
> 
> A hook on the bus makes sense because it allows central handling for any
> devices on that bus.
> However for a device, first TypeInfo::instance_init is run, then
> qdev_set_parent_bus() connects the bus and finally DeviceClass::realize
> is run - and we want to postpone realize further in the future.
> 

Yes! This would do perfectly.

> So why can't this be in VirtioDevice's or VirtIONet's realize method? At
> realize time we should definitely be on the bus in this case. I.e.,
> create vdev->config only after we know how large it needs to be rather
> than creating and later resizing it, which might fail.
> 

It appears that virtio hasn't been completely converted to use realize() yet.
Currently, device_realize() in virtio.c simply calls virtio_device_init(),
which looks like this:

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;
}

VirtioDeviceClass::init() calls virtio_init(), which allocates the config
struct. Then virtio_bus_plug_device() is called to attach the bus (and to
populate host_features). I wonder if it's safe to call
virtio_bus_plug_device() sooner...

> Regards,
> Andreas
> 

Sincerely,

Jesse Larrew
Software Engineer, KVM Team
IBM Linux Technology Center
Phone: (512) 973-2052 (T/L: 363-2052)
jlarrew@linux.vnet.ibm.com

Re: [Qemu-devel] [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass

[Frederic Konrad]

On 04/06/2013 22:02, Jesse Larrew wrote:
> On 06/04/2013 12:35 PM, Andreas Färber wrote:
>> Hi,
>>
> Hi Andreas!
>
> Thanks for the review. :)
>
>> Am 04.06.2013 18:22, schrieb Jesse Larrew:
>>> Virtio devices are initialized prior to plugging them into a bus. However,
>>> other initializations (such as host_features) don't occur until after the
>>> device is plugged into the bus. If a device needs to modify it's
>>> configuration based on host_features, then it needs to be notified when the
>>> bus is attached and host_features is available for use.
>>>
>>> This patch extends struct VirtioDeviceClass to add a bus_plugged() method.
>>> If implemented by a device, it will be called after the device is attached
>>> to a bus.
>>>
>>> Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>> I think this is backwards...
>>
>> First of all, why is host_features not available before?
>>
>> A hook on the bus makes sense because it allows central handling for any
>> devices on that bus.
>> However for a device, first TypeInfo::instance_init is run, then
>> qdev_set_parent_bus() connects the bus and finally DeviceClass::realize
>> is run - and we want to postpone realize further in the future.
>>
> Yes! This would do perfectly.
>
>> So why can't this be in VirtioDevice's or VirtIONet's realize method? At
>> realize time we should definitely be on the bus in this case. I.e.,
>> create vdev->config only after we know how large it needs to be rather
>> than creating and later resizing it, which might fail.
>>
> It appears that virtio hasn't been completely converted to use realize() yet.
> Currently, device_realize() in virtio.c simply calls virtio_device_init(),
> which looks like this:
>
> 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;
> }
>
> VirtioDeviceClass::init() calls virtio_init(), which allocates the config
> struct. Then virtio_bus_plug_device() is called to attach the bus (and to
> populate host_features). I wonder if it's safe to call
> virtio_bus_plug_device() sooner...

Hi Jesse,
Maybe with little change.

virtio_pci_device_plugged need the virtio-device to be initialized.

Fred
>
>> Regards,
>> Andreas
>>
> Sincerely,
>
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlarrew@linux.vnet.ibm.com
>

Re: [Qemu-devel] [PATCH 1/3] virtio: add bus_plugged() callback to VirtioDeviceClass

[Frederic Konrad]

On 04/06/2013 19:35, Andreas Färber wrote:
> Hi,
>
> Am 04.06.2013 18:22, schrieb Jesse Larrew:
>> Virtio devices are initialized prior to plugging them into a bus. However,
>> other initializations (such as host_features) don't occur until after the
>> device is plugged into the bus. If a device needs to modify it's
>> configuration based on host_features, then it needs to be notified when the
>> bus is attached and host_features is available for use.
>>
>> This patch extends struct VirtioDeviceClass to add a bus_plugged() method.
>> If implemented by a device, it will be called after the device is attached
>> to a bus.
>>
>> Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> I think this is backwards...
>
> First of all, why is host_features not available before?

Hi Andreas,

The major issue here is that host_features is modified after virtio 
devices are inited.

in virtio_pci_device_plugged:
     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);

and virtio_pci_device_plugged must be called after the virtio device is 
inited.

> A hook on the bus makes sense because it allows central handling for any
> devices on that bus.
> However for a device, first TypeInfo::instance_init is run, then
> qdev_set_parent_bus() connects the bus and finally DeviceClass::realize
> is run - and we want to postpone realize further in the future.
>
> So why can't this be in VirtioDevice's or VirtIONet's realize method? At
> realize time we should definitely be on the bus in this case. I.e.,

Is that possible with hotplugging virtio device on virtio-mmio?
> create vdev->config only after we know how large it needs to be rather
> than creating and later resizing it, which might fail.
>
> Regards,
> Andreas
>
Fred

[Qemu-devel] [PATCH 2/3] virtio-net: implement bus_plugged()

[Jesse Larrew]

Use the new bus_plugged() callback to calculate and (if necessary) resize
the config struct based on the requested host_features. This will help to
keep the size of the config struct as small as possible, which will help
prevent it from requiring a larger BAR size as future features are added.

Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3a6829c..e09288f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,7 @@
 #include "hw/virtio/virtio-net.h"
 #include "net/vhost_net.h"
 #include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-pci.h"
 
 #define VIRTIO_NET_VM_VERSION    11
 
@@ -1322,6 +1323,7 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
 void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
     int i, config_size = 0;
     host_features |= (1 << VIRTIO_NET_F_MAC);
     for (i = 0; feature_sizes[i].flags != 0; i++) {
@@ -1330,6 +1332,21 @@ void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
         }
     }
     n->config_size = config_size;
+    assert(config_size != 0);
+    if (config_size != vdev->config_len) {
+        vdev->config = g_realloc(vdev->config, config_size);
+        vdev->config_len = config_size;
+    }
+}
+
+static void virtio_net_bus_plugged(VirtIODevice *vdev)
+{
+    DeviceState *qdev = DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(qbus->parent);
+    VirtIONet *n = VIRTIO_NET(vdev);
+
+    virtio_net_set_config_size(n, proxy->host_features);
 }
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
@@ -1515,6 +1532,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->set_status = virtio_net_set_status;
     vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
     vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
+    vdc->bus_plugged = virtio_net_bus_plugged;
 }
 
 static const TypeInfo virtio_net_info = {
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/3] virtio-net: revert MAC address workaround

[Jesse Larrew]

With a more permanent solution in place, the workaround in commit
e9016ee2bda1b7757072b856b2196f691aee3388 is no longer needed.

Signed-off-by: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
---
 hw/net/virtio-net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index e09288f..c1c80aa 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1325,7 +1325,7 @@ void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     int i, config_size = 0;
-    host_features |= (1 << VIRTIO_NET_F_MAC);
+
     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.11.7