diff mbox

[v2,12/13] virtio-gpu: Wrap in vmstate

Message ID 1468325965-22818-13-git-send-email-dgilbert@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dr. David Alan Gilbert July 12, 2016, 12:19 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Forcibly convert it to a vmstate wrapper;  proper conversion
comes later.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/display/virtio-gpu.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Gerd Hoffmann July 12, 2016, 1:54 p.m. UTC | #1 
> @@ -1170,9 +1166,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
>  
>      if (virtio_gpu_virgl_enabled(g->conf)) {
>          vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
> -    } else {
> -        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
> -                        virtio_gpu_save, virtio_gpu_load, g);
>      }
>  }
>  
> @@ -1220,6 +1213,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
>  #endif
>  }
>  
> +VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
> +                      virtio_gpu_save);
> +
>  static Property virtio_gpu_properties[] = {
>      DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
>  #ifdef CONFIG_VIRGL
> @@ -1245,6 +1241,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
>      vdc->reset = virtio_gpu_reset;
>  
>      dc->props = virtio_gpu_properties;
> +    dc->vmsd = &vmstate_virtio_gpu;
>  }
>  
>  static const TypeInfo virtio_gpu_info = {

This is confusing.  I think for the virtio_gpu_virgl_enabled() case we
install *two* vmstates now ...

I think you should move up VMSTATE_VIRTIO_DEVICE, then simply replace
the register_savevm() call with a vmstate_register() call.

cheers,
  Gerd
Cornelia Huck July 12, 2016, 2:02 p.m. UTC | #2 
On Tue, 12 Jul 2016 15:54:57 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> > @@ -1170,9 +1166,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> >  
> >      if (virtio_gpu_virgl_enabled(g->conf)) {
> >          vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
> > -    } else {
> > -        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
> > -                        virtio_gpu_save, virtio_gpu_load, g);
> >      }
> >  }
> >  
> > @@ -1220,6 +1213,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
> >  #endif
> >  }
> >  
> > +VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
> > +                      virtio_gpu_save);
> > +
> >  static Property virtio_gpu_properties[] = {
> >      DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
> >  #ifdef CONFIG_VIRGL
> > @@ -1245,6 +1241,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
> >      vdc->reset = virtio_gpu_reset;
> >  
> >      dc->props = virtio_gpu_properties;
> > +    dc->vmsd = &vmstate_virtio_gpu;
> >  }
> >  
> >  static const TypeInfo virtio_gpu_info = {
> 
> This is confusing.  I think for the virtio_gpu_virgl_enabled() case we
> install *two* vmstates now ...

I don't think that matters, as the unmigratable state already blocks,
no?

> 
> I think you should move up VMSTATE_VIRTIO_DEVICE, then simply replace
> the register_savevm() call with a vmstate_register() call.

It would make virtio-gpu look different from all other devices, though.
Gerd Hoffmann July 12, 2016, 3:09 p.m. UTC | #3 
On Di, 2016-07-12 at 16:02 +0200, Cornelia Huck wrote:
> On Tue, 12 Jul 2016 15:54:57 +0200
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> > > @@ -1170,9 +1166,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> > >  
> > >      if (virtio_gpu_virgl_enabled(g->conf)) {
> > >          vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
> > > -    } else {
> > > -        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
> > > -                        virtio_gpu_save, virtio_gpu_load, g);
> > >      }
> > >  }
> > >  
> > > @@ -1220,6 +1213,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
> > >  #endif
> > >  }
> > >  
> > > +VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
> > > +                      virtio_gpu_save);
> > > +
> > >  static Property virtio_gpu_properties[] = {
> > >      DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
> > >  #ifdef CONFIG_VIRGL
> > > @@ -1245,6 +1241,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
> > >      vdc->reset = virtio_gpu_reset;
> > >  
> > >      dc->props = virtio_gpu_properties;
> > > +    dc->vmsd = &vmstate_virtio_gpu;
> > >  }
> > >  
> > >  static const TypeInfo virtio_gpu_info = {
> > 
> > This is confusing.  I think for the virtio_gpu_virgl_enabled() case we
> > install *two* vmstates now ...
> 
> I don't think that matters, as the unmigratable state already blocks,
> no?

As long the core isn't confused if we register twice for the same device
it should work, yes ...

> > I think you should move up VMSTATE_VIRTIO_DEVICE, then simply replace
> > the register_savevm() call with a vmstate_register() call.
> 
> It would make virtio-gpu look different from all other devices, though.

Sure, because depending on device configuration you can migrate it or
not, which isn't the case for all other devices.

I'd prefer to have the logic for that in one place as it makes more
clear how things are working:  "if (!migrateable) register(block) else
register(normal);"

cheers,
  Gerd
Dr. David Alan Gilbert July 12, 2016, 3:12 p.m. UTC | #4 
* Gerd Hoffmann (kraxel@redhat.com) wrote:
> On Di, 2016-07-12 at 16:02 +0200, Cornelia Huck wrote:
> > On Tue, 12 Jul 2016 15:54:57 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > > > @@ -1170,9 +1166,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> > > >  
> > > >      if (virtio_gpu_virgl_enabled(g->conf)) {
> > > >          vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
> > > > -    } else {
> > > > -        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
> > > > -                        virtio_gpu_save, virtio_gpu_load, g);
> > > >      }
> > > >  }
> > > >  
> > > > @@ -1220,6 +1213,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
> > > >  #endif
> > > >  }
> > > >  
> > > > +VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
> > > > +                      virtio_gpu_save);
> > > > +
> > > >  static Property virtio_gpu_properties[] = {
> > > >      DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
> > > >  #ifdef CONFIG_VIRGL
> > > > @@ -1245,6 +1241,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
> > > >      vdc->reset = virtio_gpu_reset;
> > > >  
> > > >      dc->props = virtio_gpu_properties;
> > > > +    dc->vmsd = &vmstate_virtio_gpu;
> > > >  }
> > > >  
> > > >  static const TypeInfo virtio_gpu_info = {
> > > 
> > > This is confusing.  I think for the virtio_gpu_virgl_enabled() case we
> > > install *two* vmstates now ...
> > 
> > I don't think that matters, as the unmigratable state already blocks,
> > no?
> 
> As long the core isn't confused if we register twice for the same device
> it should work, yes ...
> 
> > > I think you should move up VMSTATE_VIRTIO_DEVICE, then simply replace
> > > the register_savevm() call with a vmstate_register() call.
> > 
> > It would make virtio-gpu look different from all other devices, though.
> 
> Sure, because depending on device configuration you can migrate it or
> not, which isn't the case for all other devices.
> 
> I'd prefer to have the logic for that in one place as it makes more
> clear how things are working:  "if (!migrateable) register(block) else
> register(normal);"

Hmm yes I'll look at this again; I agree registering it twice is probably
a bad idea (whether it happens to work or not, it still seems a bad idea).
virtio-gpu is just a bit special in this case and I'll dig further.

Dave

> cheers,
>   Gerd
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert July 14, 2016, 4:04 p.m. UTC | #5 
* Gerd Hoffmann (kraxel@redhat.com) wrote:
> On Di, 2016-07-12 at 16:02 +0200, Cornelia Huck wrote:
> > On Tue, 12 Jul 2016 15:54:57 +0200
> > Gerd Hoffmann <kraxel@redhat.com> wrote:
> > 
> > > > @@ -1170,9 +1166,6 @@ static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
> > > >  
> > > >      if (virtio_gpu_virgl_enabled(g->conf)) {
> > > >          vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
> > > > -    } else {
> > > > -        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
> > > > -                        virtio_gpu_save, virtio_gpu_load, g);
> > > >      }
> > > >  }
> > > >  
> > > > @@ -1220,6 +1213,9 @@ static void virtio_gpu_reset(VirtIODevice *vdev)
> > > >  #endif
> > > >  }
> > > >  
> > > > +VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
> > > > +                      virtio_gpu_save);
> > > > +
> > > >  static Property virtio_gpu_properties[] = {
> > > >      DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
> > > >  #ifdef CONFIG_VIRGL
> > > > @@ -1245,6 +1241,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
> > > >      vdc->reset = virtio_gpu_reset;
> > > >  
> > > >      dc->props = virtio_gpu_properties;
> > > > +    dc->vmsd = &vmstate_virtio_gpu;
> > > >  }
> > > >  
> > > >  static const TypeInfo virtio_gpu_info = {
> > > 
> > > This is confusing.  I think for the virtio_gpu_virgl_enabled() case we
> > > install *two* vmstates now ...
> > 
> > I don't think that matters, as the unmigratable state already blocks,
> > no?
> 
> As long the core isn't confused if we register twice for the same device
> it should work, yes ...
> > > I think you should move up VMSTATE_VIRTIO_DEVICE, then simply replace
> > > the register_savevm() call with a vmstate_register() call.
> > 
> > It would make virtio-gpu look different from all other devices, though.
> 
> Sure, because depending on device configuration you can migrate it or
> not, which isn't the case for all other devices.
> 
> I'd prefer to have the logic for that in one place as it makes more
> clear how things are working:  "if (!migrateable) register(block) else
> register(normal);"

Hmm yes; I think the right fix here is to convert that into
  migrate_add_blocker / migrate_del_blocker

since that's all you're currently doing.
When you actually want to make it migratable add it to virtio_gpu_load/store
(until they get borged into vmstate)

I'll rework with migrate_add/del_blocker calls.

Dave

> 
> cheers,
>   Gerd
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Gerd Hoffmann July 15, 2016, 9:53 a.m. UTC | #6 
Hi,

> Hmm yes; I think the right fix here is to convert that into
>   migrate_add_blocker / migrate_del_blocker

Oh, didn't notice we have that, yes that sounds good.

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index f8b0274..24149f7 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -991,7 +991,7 @@  static const VMStateDescription vmstate_virtio_gpu_unmigratable = {
     .unmigratable = 1,
 };
 
-static void virtio_gpu_save(QEMUFile *f, void *opaque)
+static void virtio_gpu_save(QEMUFile *f, void *opaque, size_t size)
 {
     VirtIOGPU *g = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(g);
@@ -1021,7 +1021,7 @@  static void virtio_gpu_save(QEMUFile *f, void *opaque)
     vmstate_save_state(f, &vmstate_virtio_gpu_scanouts, g, NULL);
 }
 
-static int virtio_gpu_load(QEMUFile *f, void *opaque, int version_id)
+static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size)
 {
     VirtIOGPU *g = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(g);
@@ -1030,11 +1030,7 @@  static int virtio_gpu_load(QEMUFile *f, void *opaque, int version_id)
     uint32_t resource_id, pformat;
     int i, ret;
 
-    if (version_id != VIRTIO_GPU_VM_VERSION) {
-        return -EINVAL;
-    }
-
-    ret = virtio_load(vdev, f, version_id);
+    ret = virtio_load(vdev, f, VIRTIO_GPU_VM_VERSION);
     if (ret) {
         return ret;
     }
@@ -1170,9 +1166,6 @@  static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
 
     if (virtio_gpu_virgl_enabled(g->conf)) {
         vmstate_register(qdev, -1, &vmstate_virtio_gpu_unmigratable, g);
-    } else {
-        register_savevm(qdev, "virtio-gpu", -1, VIRTIO_GPU_VM_VERSION,
-                        virtio_gpu_save, virtio_gpu_load, g);
     }
 }
 
@@ -1220,6 +1213,9 @@  static void virtio_gpu_reset(VirtIODevice *vdev)
 #endif
 }
 
+VMSTATE_VIRTIO_DEVICE(gpu, VIRTIO_GPU_VM_VERSION, virtio_gpu_load,
+                      virtio_gpu_save);
+
 static Property virtio_gpu_properties[] = {
     DEFINE_PROP_UINT32("max_outputs", VirtIOGPU, conf.max_outputs, 1),
 #ifdef CONFIG_VIRGL
@@ -1245,6 +1241,7 @@  static void virtio_gpu_class_init(ObjectClass *klass, void *data)
     vdc->reset = virtio_gpu_reset;
 
     dc->props = virtio_gpu_properties;
+    dc->vmsd = &vmstate_virtio_gpu;
 }
 
 static const TypeInfo virtio_gpu_info = {