Re: [Qemu-devel] Re: optional feature

[Michael S. Tsirkin]

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