| Message ID | 20110518055737.10458.65866.stgit@dhcp-91-7.nay.redhat.com.englab.nay.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 18, 2011 at 01:57:37PM +0800, Jason Wang wrote: > Current vm_running was not explicitly initialized and its value was changed by > vm state notifier, this may confuse the virtio device being hotplugged such as > virtio-net with vhost backend as it may think the vm was not running. Solve this > by initialize this value explicitly in virtio_common_init(). > > Signed-off-by: Jason Wang <jasowang@redhat.com> I think a slightly cleaner way would be to invoke virtio_vmstate_change after it is registered, have that update the vm_running field. > --- > 0 files changed, 0 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio.c b/hw/virtio.c > index 6e8814c..27d7e50 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -789,6 +789,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > vdev->queue_sel = 0; > vdev->config_vector = VIRTIO_NO_VECTOR; > vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); > + vdev->vm_running = vm_running; > for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { > vdev->vq[i].vector = VIRTIO_NO_VECTOR; > vdev->vq[i].vdev = vdev;
On 05/18/2011 04:54 PM, Michael S. Tsirkin wrote: > On Wed, May 18, 2011 at 01:57:37PM +0800, Jason Wang wrote: >> Current vm_running was not explicitly initialized and its value was changed by >> vm state notifier, this may confuse the virtio device being hotplugged such as >> virtio-net with vhost backend as it may think the vm was not running. Solve this >> by initialize this value explicitly in virtio_common_init(). >> >> Signed-off-by: Jason Wang<jasowang@redhat.com> > I think a slightly cleaner way would be to invoke > virtio_vmstate_change after it is registered, > have that update the vm_running field. > Do you mean call it in virtio_common_init() directly? But the bindings are not initialized then. Or is there anything I missed? >> --- >> 0 files changed, 0 insertions(+), 0 deletions(-) >> >> diff --git a/hw/virtio.c b/hw/virtio.c >> index 6e8814c..27d7e50 100644 >> --- a/hw/virtio.c >> +++ b/hw/virtio.c >> @@ -789,6 +789,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >> vdev->queue_sel = 0; >> vdev->config_vector = VIRTIO_NO_VECTOR; >> vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); >> + vdev->vm_running = vm_running; >> for(i = 0; i< VIRTIO_PCI_QUEUE_MAX; i++) { >> vdev->vq[i].vector = VIRTIO_NO_VECTOR; >> vdev->vq[i].vdev = vdev;
On Mon, May 23, 2011 at 01:57:57PM +0800, Jason Wang wrote: > On 05/18/2011 04:54 PM, Michael S. Tsirkin wrote: > >On Wed, May 18, 2011 at 01:57:37PM +0800, Jason Wang wrote: > >>Current vm_running was not explicitly initialized and its value was changed by > >>vm state notifier, this may confuse the virtio device being hotplugged such as > >>virtio-net with vhost backend as it may think the vm was not running. Solve this > >>by initialize this value explicitly in virtio_common_init(). > >> > >>Signed-off-by: Jason Wang<jasowang@redhat.com> > >I think a slightly cleaner way would be to invoke > >virtio_vmstate_change after it is registered, > >have that update the vm_running field. > > > > Do you mean call it in virtio_common_init() directly? But the > bindings are not initialized then. Or is there anything I missed? My idea was that when we register a callback, it can immediately be invoked with the current vm state. But this has other implications so would have to be tested more. So I think your patch is a good start. Acked-by: Michael S. Tsirkin <mst@redhat.com> > >>--- > >> 0 files changed, 0 insertions(+), 0 deletions(-) > >> > >>diff --git a/hw/virtio.c b/hw/virtio.c > >>index 6e8814c..27d7e50 100644 > >>--- a/hw/virtio.c > >>+++ b/hw/virtio.c > >>@@ -789,6 +789,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > >> vdev->queue_sel = 0; > >> vdev->config_vector = VIRTIO_NO_VECTOR; > >> vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); > >>+ vdev->vm_running = vm_running; > >> for(i = 0; i< VIRTIO_PCI_QUEUE_MAX; i++) { > >> vdev->vq[i].vector = VIRTIO_NO_VECTOR; > >> vdev->vq[i].vdev = vdev;
On Wed, May 18, 2011 at 01:57:37PM +0800, Jason Wang wrote: > Current vm_running was not explicitly initialized and its value was changed by > vm state notifier, this may confuse the virtio device being hotplugged such as > virtio-net with vhost backend as it may think the vm was not running. Solve this > by initialize this value explicitly in virtio_common_init(). > > Signed-off-by: Jason Wang <jasowang@redhat.com> Applied, thanks > --- > 0 files changed, 0 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio.c b/hw/virtio.c > index 6e8814c..27d7e50 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -789,6 +789,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > vdev->queue_sel = 0; > vdev->config_vector = VIRTIO_NO_VECTOR; > vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); > + vdev->vm_running = vm_running; > for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { > vdev->vq[i].vector = VIRTIO_NO_VECTOR; > vdev->vq[i].vdev = vdev;
diff --git a/hw/virtio.c b/hw/virtio.c index 6e8814c..27d7e50 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -789,6 +789,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, vdev->queue_sel = 0; vdev->config_vector = VIRTIO_NO_VECTOR; vdev->vq = qemu_mallocz(sizeof(VirtQueue) * VIRTIO_PCI_QUEUE_MAX); + vdev->vm_running = vm_running; for(i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { vdev->vq[i].vector = VIRTIO_NO_VECTOR; vdev->vq[i].vdev = vdev;
Current vm_running was not explicitly initialized and its value was changed by vm state notifier, this may confuse the virtio device being hotplugged such as virtio-net with vhost backend as it may think the vm was not running. Solve this by initialize this value explicitly in virtio_common_init(). Signed-off-by: Jason Wang <jasowang@redhat.com> --- 0 files changed, 0 insertions(+), 0 deletions(-)