From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53360) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMzMd-0006PJ-Ql for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:12:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMzMb-0001t5-42 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:12:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMzMa-0001sz-S7 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 11:12:45 -0400 Date: Tue, 12 Jul 2016 16:12:40 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160712151239.GA3664@work-vm> References: <1468325965-22818-1-git-send-email-dgilbert@redhat.com> <1468325965-22818-13-git-send-email-dgilbert@redhat.com> <1468331697.30552.7.camel@redhat.com> <20160712160236.57c4a2b4.cornelia.huck@de.ibm.com> <1468336197.30552.16.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1468336197.30552.16.camel@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 12/13] virtio-gpu: Wrap in vmstate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: Cornelia Huck , amit.shah@redhat.com, cornelia.huck@del.ibm.com, quintela@redhat.com, qemu-devel@nongnu.org, mst@redhat.com * 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 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