From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mnvcr-0006yk-H1 for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:36:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mnvcm-0006yU-V5 for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:36:53 -0400 Received: from [199.232.76.173] (port=50467 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mnvcm-0006yR-R5 for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:36:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36057) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mnvcm-0007ew-9B for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:36:48 -0400 Date: Wed, 16 Sep 2009 17:34:59 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: optional feature Message-ID: <20090916143459.GD5287@redhat.com> References: <20090916111845.GJ23157@redhat.com> <20090916115726.GL23157@redhat.com> <20090916123535.GM23157@redhat.com> <4AB0F17B.7000107@codemonkey.ws> <20090916141245.GC5287@redhat.com> <4AB0F45A.7000100@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4AB0F45A.7000100@codemonkey.ws> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org, Gleb Natapov , Juan Quintela On Wed, Sep 16, 2009 at 09:21:14AM -0500, Anthony Liguori wrote: > Michael S. Tsirkin wrote: >> On Wed, Sep 16, 2009 at 09:08:59AM -0500, Anthony Liguori wrote: >> >>> Juan Quintela wrote: >>> >>>>>> up rtc version +1 >>>>>> add the two fields that we need (together with rtc-td-hack value) >>>>>> >>>>> And why this is better? You can't migrate old VM to new qemu even if you >>>>> don't use rtc-td-hack on new one. >>>>> >>>> I think the difference between us is: >>>> - is rtc-td-hack a hack that should only be used for some users >>>> - it is a valid rtc feature that should be available to everybody >>>> - it is independent, or it needs an rtc to have any value. >>>> >>> We need a single table that contains the full state for the device. >>> >>> Many devices will have knobs. There are two likely types of knobs: >>> >>> 1) something that indicates how an array of state is going to be >>> 2) a boolean that indicates whether a portion of state is valid >>> >>> rtc-td falls into the second category. It makes sense to me that the >>> table state would contain a boolean to indicate whether a given set >>> of state was valid. You may need a grouping mechanism for this. >>> It probably makes sense to do this as separate tables. For >>> instance, >>> >>> .fields = (VMStateField []) { >>> VMSTATE_BOOL(td_hack, RTCState, (VMStateField[]){ >>> VMSTATE_INT32(irq_coalesced, RTCState), >>> VMSTATE_INT32(period, RTCState), >>> VMSTATE_END_OF_LIST()}), >>> } >>> >>> If we can't maintain backwards compatibility using this approach (we >>> definitely can't for rtc-td) then we'll just have to live with that. >>> >> >> We have to? Why do we? > > We could have an open loading function to load old versions of this > device. It's ugly, but there's really no other way. > >> And not only won't we have backwards >> compatibility now, we will also break it and have to break it each time >> we add a knob. >> > > No, we bump the version number and add more fields to the state. > > If we need to make crazy changes (like make a previously non-optional > state, optional) then we can introduce two tables if we have to. > >> If instead we would only save/load the part of state if >> the knob is set, we won't have a problem. >> > > The rtc device happens to support an optional feature by splitting the > optional bits into a separate section. Not every device does this > though so if you want to convert other devices to this style, you'll > break their backwards compatibility. > > The mechanisms are functionally the same. One is no more expressive > than the other. Yes, separate devices variant is more expressive. It is more modular. With optional features A B C, versioning can not support saving only A and C but not B. This is bad for example for backporting features between versions: if C happened to be introduced after B, backporting C will force backporting B. But you can support it if you put each one in a migration container. > It's the difference of vN introduces these optional > features vs expliciting introducing new sections. What I don't like > about the later is that these all need to be tied together in some sort > of cohesive way. I don't understand what this means, sorry. >>> I also think arrays should be expressed like this FWIW. Today we >>> have explicit typed arrays. I would rather see: >>> >>> .fields = (VMStateField []) { >>> VMSTATE_ARRAY(nirq, PCIBus, (VMStateField[]) { >>> VMSTATE_INT32(irq_count[0], PCIBus), >>> VMSTATE_END_OF_LIST()}), >>> } >>> >> >> Same problem here. >> > > I don't see what the problem is. if you are not saving irq state, it's better to skip the array that create a 0 size array. The former works for non-array fields, the later does not, and you have to invent a separate valid bit. > Regards, > > Anthony Liguori