[Qemu-devel] Re: optional feature

[Juan Quintela <quintela@redhat.com>]

Gleb Natapov <gleb@redhat.com> wrote:
> On Wed, Sep 16, 2009 at 01:48:35PM +0200, Juan Quintela wrote:
>> Gleb Natapov <gleb@redhat.com> wrote:
>> > On Wed, Sep 16, 2009 at 01:04:19PM +0200, Juan Quintela wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> 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:

up rtc version +1
add the two fields that we need (together with rtc-td-hack value)

and now we:
- can still load old rtc state
- can save rtc state with/without rtc-td-hack value
  if rtc-td-hack is not enabled, it is just not used

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


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

Later, Juan.