| Message ID | 1370362965-3937-2-git-send-email-jlarrew@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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,
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(+)