From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MnvtP-0001Hh-8s for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:53:59 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MnvtK-0001H3-JL for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:53:59 -0400 Received: from [199.232.76.173] (port=53723 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MnvtK-0001H0-Es for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:53:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3248) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MnvtJ-00025r-PX for qemu-devel@nongnu.org; Wed, 16 Sep 2009 10:53:54 -0400 From: Juan Quintela In-Reply-To: <20090916143459.GD5287@redhat.com> (Michael S. Tsirkin's message of "Wed, 16 Sep 2009 17:34:59 +0300") 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> <20090916143459.GD5287@redhat.com> Date: Wed, 16 Sep 2009 16:53:47 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: optional feature List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Gleb Natapov , qemu-devel@nongnu.org "Michael S. Tsirkin" wrote: > 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. No problem again. You backport, and you add to the state both B and C. You just don't care about B values. I leave you a name for them: reserved0 reserved1 reserved2 And you are done. And what is worse, what happens when you have to upgrade B and C with new fields? Now you have: A, B, B', C, C' And what options are valid? How you differentiate between B and B', C and C', a version number? We are back at stage 1? And how many features do you support? Exponential again. With linear version numbers you are going to have: A A+B A+B+C A+B'+C A+B'+C' (you can switch the 2 last ones depending the order in which changes happen). And that is it, no exponential cases again. we added 4 features and we have 4 new versions. It looks very linear to me. And we can still load all the previous versions. > But you can support it if you put each one in a migration container. My opinion: We don't even want to think about this. > if you are not saving irq state, it's better > to skip the array that create a 0 size array. Why? > The former works for non-array fields, the later does not, > and you have to invent a separate valid bit. VMStateDescription, think of it as a contract. Would you like that the other part would be able to insert whole paragraphs when he wants? Without ever telling you that it changed (i.e. same version). I don't think so. I am sure I would preffer that it will told me clearly that contract changed (new version), and the changes are this, this and this. Later, Juan.