[Qemu-devel] Re: optional feature

[Juan Quintela <quintela@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Sep 16, 2009 at 02:26:58PM +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > On Wed, Sep 16, 2009 at 02:14:32PM +0200, Juan Quintela wrote:
>> >> >> 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).
>> >> >
>> >> > Let's just bite the bullet and add support for if's?  It's not like it's
>> >> > hard to invent 'struct vmstate_condition' or some such.
>> >> 
>> >> I have to do it.  The problem is not adding an optional field, is adding
>> >> it conditionally on _what_, and that _what_ should also be ideally on vmstate.
>> >> 
>> >> Later, Juan.
>> >
>> > pci config is on vmstate already, I don't see a problem here.
>> 
>> vmstate don't understand pci config.
>
> How can it save it then? What's more, how can it load it sanely? E.g.
> when loading we must make sure that device id etc match.

We don't test it at all.  We just load an ne2000 image into an ne2000
device, and things like that.  VMState has zero knowledge of pci.  It
just saves some fields.

>> I want to manipulate images with
>> the informantion that you have given vmstate.  Sending another byte
>> meaning:
>> 
>> msix_enabled
>> 
>> and now depending on that value another field is ok with me.
>
> 1. this bit is there in config already.
>    sending it twice is redundant and so, wrong:
>    we will just have to add even more code
>    to check that these values match

You need to write the code anyways.  There is no way that VMState will
learn PCI (or whatever else), it does'nt matter if you do:

pre_save()
....
        s->msix_enabled_vmstate = foo(s);
....

or we add a new feature to VMState that allows you to call a function
(foo() in this case) to know if we send/don't send vmstate.  You need to
write foo anyways.

> 2. it's also not backward compatible, is it?

Optional fields were not allowed, really before.  Format was abused and
then we got problems that we have to live with them.   If you mean that
in the past devices used optional features, that is right.

>
>> You can
>> calculate msix_enable at pre_save time whenever way that you see fit.
>
> Not everything has to fit in a global variable, we can have a table
> per-device. Then each time msix_enable bit is changed, I can tell
> vmstate about it.

No.  You only need to tell before saving.  And there is nice function
(pre_save())  that would be called before doing a save.  You can put
there any code to fill that variable.  It is ok to create
  msix_enabled_vmstate variable, that is only used for vmstate, no
  problems with that.

>
>> What vmstate needs is a only to now if msix_enable is 0 or 1, how you
>> calculate it, VMState don't care.
>
> Add a condition including offset into pci config and bitmask?

No, defining a variable is easy for anybody.  if I teach PCI to VMState,
then the damn things at i2c will want me to teach it i2c, and then
SunBus, and then ISA, and then ..... VMState understands all qemu.

There is no problem at all doing in pre_save()

msix_enabled = foo()

And you can share that foo function for all msix devices if you want it.
VMState don't have to learn msix, and efect is the same.

Later, Juan.