From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mnt5V-00054l-DD for qemu-devel@nongnu.org; Wed, 16 Sep 2009 07:54:17 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mnt5Q-000537-Cp for qemu-devel@nongnu.org; Wed, 16 Sep 2009 07:54:16 -0400 Received: from [199.232.76.173] (port=40357 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mnt5P-00052t-Qd for qemu-devel@nongnu.org; Wed, 16 Sep 2009 07:54:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44623) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mnt5P-0008Oh-9F for qemu-devel@nongnu.org; Wed, 16 Sep 2009 07:54:11 -0400 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n8GBsAuJ003634 for ; Wed, 16 Sep 2009 07:54:10 -0400 Date: Wed, 16 Sep 2009 14:52:24 +0300 From: "Michael S. Tsirkin" Message-ID: <20090916115224.GA4628@redhat.com> References: <20090916104620.GA4456@redhat.com> <20090916111845.GJ23157@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: optional feature List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org, Gleb Natapov On Wed, Sep 16, 2009 at 01:48:35PM +0200, Juan Quintela wrote: > Gleb Natapov wrote: > > On Wed, Sep 16, 2009 at 01:04:19PM +0200, Juan Quintela wrote: > >> "Michael S. Tsirkin" wrote: > >> > On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote: > >> >> How do we deal with optional features? > >> > > >> > Here's an idea that Gleb suggested in a private > >> > conversation: make optional features into > >> > separate, non-user-visible devices. > >> > > >> > Thus we would have vmstate for virtio and additionally, if msix is > >> > enabled, vmstate for msix. This solves the problem of the number of > >> > devices becoming exponential with the number of features: we have device > >> > per feature. > >> > > >> > I understand that RTC does something like this. > >> > >> And it is wrong :) I sent a patch to fix it properly, but we have the > >> problem of backward compatibility with kvm. > >> > >> Forget msix for virtio, virtio has the problem already with pci. > > What is wrong about it? > > See below, we are changing the state to one table, and tables don't have > neither if's or whiles (we have a limited for that just walks arrays). Let's just bite the bullet and add support for if's? It's not like it's hard to invent 'struct vmstate_condition' or some such. > I don't really know how to handle virtio in a sane way. The _saner_ way > that I thought was to split it into three devices as I sketched below, > but I still don't understood virtio creation to see how to "fix" it. > It is next on queue after cpu (BIGGG), and ide (was waiting for Gerd > patches to be commited. After that, I will thought again on how to > handle virtio. > > > >> > >> virtio_save() > >> { > >> > >> if (vdev->binding->save_config) > >> vdev->binding->save_config(vdev->binding_opaque, f); > >> > >> qemu_put_8s(f, &vdev->status); > >> > >> .... some other normal fields ... > >> > >> for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) { > >> if (vdev->vq[i].vring.num == 0) > >> break; > >> > >> Not a problem, we can precalculate i on pre_save() > >> > >> > >> qemu_put_be32(f, vdev->vq[i].vring.num); > >> qemu_put_be64(f, vdev->vq[i].pa); > >> qemu_put_be16s(f, &vdev->vq[i].last_avail_idx); > >> > >> This is sending a partial array of struct (the "i" 1st entries) > >> No problem here. > >> > >> if (vdev->binding->save_queue) > >> vdev->binding->save_queue(vdev->binding_opaque, i, f); > >> > >> Again, what to do with this one. > >> > >> } > >> > >> } > >> > >> Looking at what does virtio_pci_save_queue() > >> > >> static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) > >> { > >> VirtIOPCIProxy *proxy = opaque; > >> if (msix_present(&proxy->pci_dev)) > >> qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); > >> } > >> > >> i.e. and now, an optional field. > >> > >> And no, I don't have either a clean design that will be backward > >> compatible and clean. Clean design is easy: > >> > >> virtio > >> virtio-pci (it does the equivalent of save_config() and then call > >> virtio_save) > >> virtio-pci-msix (it calls virtio-pci and then send a partial array of > >> queues. (the save queue thing) > >> > >> Before you ask, partial arrays are sent: + array > >> where num_elems == 0 is valid. > >> > >> But this is the "good" design if we started _now_, that is not the case, > >> and I am trying to get something clean and bacward compatible. > >> > >> Later, Juan. > >> > >> PD. Optional fields are going to have to be in, arm cpus really need > >> them if we want to maintain backward compatibility. > > > > -- > > Gleb.