[Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Jason Wang]

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(-)

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;

Re: [Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Michael S. Tsirkin]

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;

Re: [Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Jason Wang]

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;

Re: [Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Michael S. Tsirkin]

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;

Re: [Qemu-devel] [PATCH] virtio: correctly initialize vm_running

[Michael S. Tsirkin]

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;