Re: [Qemu-devel] [PATCH v2 12/13] virtio-gpu: Wrap in vmstate

[Gerd Hoffmann <kraxel@redhat.com>]

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