From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41872) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNj7Z-0006PP-RN for qemu-devel@nongnu.org; Thu, 14 Jul 2016 12:04:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNj7U-0004Fs-P9 for qemu-devel@nongnu.org; Thu, 14 Jul 2016 12:04:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58325) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNj7U-0004Fk-FC for qemu-devel@nongnu.org; Thu, 14 Jul 2016 12:04:12 -0400 Date: Thu, 14 Jul 2016 17:04:07 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20160714160407.GB23279@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 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