[Qemu-devel] Re: optional feature

[Juan Quintela]

Gleb Natapov <address@hidden> wrote:

Hi

>> 
>> You have a rtc-td table that _needs_ rtc-td unconditionally, and that
>> makes no sense at all, then another way of doing it is:
>> 
> I don't understand why this makes no sense. It does for me. You have to
> run "qemu -incoming" with same parameters as original one, so you have
> to run it with rtc-td-hack if migration source did it. Otherwise
> migration should fail.

Yes.

> And if source runs without rtc-td-hack rtc-td
> table is not registered and is not migrated.

I am not telling that it will not work.

>> 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.

See for instance ide, pci-ide has two devices, and it includes its state
inside the "ide" bigger device.  it is not that we had isa-ide working
before, and now that we have pci, we add a pci-ide with the missing
fields.

Form me it happens that:
- rtc-td-hack is a valid feature, and I think should be available to
  everybody
- for me it is not independent (at all) of the rtc, is just another
  feature that should be saved with it.

But that is my opinion :)

>> and now we:
>> - can still load old rtc state
> how? It has another version.

You can have it compatible.
old_version = 1
new_verion = 1

if loading from old_version -> only old fields are read
  we put on post_load() save values for missing fields
saving -> all fields are saved
loading for new_version : we read all fields

No problems at all with compatibility.
See my mail:

From: Juan Quintela <address@hidden>
Subject: [RFC PATCH v2 1/4] mc145818rtc: fix saving of rtc-td
        hack properly upgrading the version number
To: address@hidden
Date: Thu, 10 Sep 2009 01:12:54 +0200

Where I try to do it properly.

I only tried to change the savevm part.  If this patch was agreed, I
would have cleaned the hack part to "feel" more integrated.

>> - can save rtc state with/without rtc-td-hack value
>>   if rtc-td-hack is not enabled, it is just not used
> Nothing that you can't do with two table approach as far as I can see.

As I told on other email.  This is not an VMState problem.  This is in
the: it works, and has no "technical" problems.  My problem here is with
the design of the feature.

>> if we ever get to the point that we decide that rtc-td-hack should
>> always be enabled, everything is working already.
>> 
>> > From vmstate point of view those are not connected.
>> 
>> This is not VMState related.  It is that you need another two fields to
>> get a new feature of rtc.  Are we agreeing that everything is easier if
>> you added the fields to rtc instead fo creating a new device for this
>> two values?  That was my point about the correct way of handling this to
> You don't create another device. You create another "migration container"
>
>> values.  And yes, "correct" here don't have into account that kvm was a
>> fork of qemu.  There are "historic" reasons why it made sense to create
>> a new device for rtc-td-hack, but that reasons don't mean that this is
>> the more correct way of doing it.
>> 
>> 
> It has nothing to do with fork of qemu. It was nice way to add
> functionality + migration support for it without breaking plane RTC
> migration.

See my patch, migration without rtc-td-hack is maintained.  Just we have
an rtc++ that is able to coalesce interrupts.

>> > Serialization/deserialization should support matching of
>> > incomming binary blob to deserialize function. When entire incomming
>> > stream is consumed check has to be made that there is no uninitialized
>> > table (deserialize callback that was not called) and if there is -
>> > abort.
>> 
>> As I told you, this one was not VMState related.  From a technical point
>> of view, adding the new device was not a problem.  What I was discussing
>> was if this was the better way of handling this problem.  rtc-td-hack is
>> an imaginary device to save two flags that rtc don't save for you.  If
>> you just remove the "hack" for the name, the ifdefs for the code, and
>> leave the coalesced field names, it indeed looks like a nice new feature
>> of the rtc?  A feature that makes sense to have for everybody?
>> 
> So? I don't get your point. We are talking about how we can add features
> to devices and don't break migration, no?

we are talking several things :)

a- how to maintain backwards compatibility when we do changes.
   Agreement was that we have to be able to load old state.  Going from
   newer qemu to old qemu is not supported.  THis one is preserved with
   the two approaches (yes, yours also allows to go to old qemu).

b- There have been changes at some points that _shouldn't_ have
   happened.  Maintain backward compatibility with that changes
   is quite "difficult".  We are trying here to maintaing bacward
   compatibility, but telling people: please, no more changes like this
   one.  rtc-td-hack here was not here either.

c- Some changes can be implemented in more that one way.  Some ways have
   some advantages, and other has other compromises.  Here we are.

In rtc case,  I am discussing c)  Current code:
- creates one imaginary device for the new needed fields.  Allows
  backwards compatiblity (as if only uses rtc, noting has changed).

- my patch: only one device either exist.  rtc-td code looks more
  integrated with the rest of the rtc code.  We have one less device
  that care about (where did you put the rtc-td device on qdev)?  How do
  you explain it on user documentation, ....

Obviously, I preffer my patch, but that don't means that it is the only
way.  It is just a different compromise.  One that I consider that is
better.  But at this point, I think that everybody agrees that you don't
think that my compromise is better.

Later, Juan.







> One way is to have "migration containers"
> for each isolated feature (I don't like to call them "imaginary devices").
>
> --
>                       Gleb.