[Qemu-devel] Re: optional feature

[Gleb Natapov]

On Wed, Sep 16, 2009 at 02:23:33PM +0200, Juan Quintela wrote:
> Gleb Natapov <address@hidden> wrote:
> > On Wed, Sep 16, 2009 at 01:48:35PM +0200, Juan Quintela wrote:
> >> Gleb Natapov <address@hidden> wrote:
> >> > On Wed, Sep 16, 2009 at 01:04:19PM +0200, Juan Quintela wrote:
> >> >> "Michael S. Tsirkin" <address@hidden> 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).
> >> 
> > I don't know virtio enough to understand all those things below. I am
> > asking what is wrong about how RTC did it? You don't need if's or
> > whiles.
> 
> Sorry, I missunderstood your question.
> 
> > You have general RTC sate in one table and things that needed by
> > rtc-td-hack in another table.
> 
> 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. And if source runs without rtc-td-hack rtc-td
table is not registered and is not migrated.

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

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

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

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

> > 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? One way is to have "migration 
containers"
for each isolated feature (I don't like to call them "imaginary devices").

--
                        Gleb.