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

[Jesse Larrew <jlarrew@linux.vnet.ibm.com>]

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