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

[Frederic Konrad <fred.konrad@greensocs.com>]

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
>