Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor

[Kevin Wolf]

Am 05.06.2012 11:47, schrieb Anthony Liguori:
> On 06/05/2012 05:25 PM, Kevin Wolf wrote:
>> Am 05.06.2012 03:00, schrieb Michael Roth:
>>> This is an import of Anthony's qidl compiler, with some changes squashed
>>> in to add support for doing the visitor generation via QEMU's qapi code
>>> generators rather than directly.
>>>
>>> Documentation has been imported as well, as is also viewable at:
>>>
>>> https://github.com/aliguori/qidl/blob/master/qc.md
>>>
>>> This will be used to add annotations to device structs to aid in
>>> generating visitors that can be used to serialize/unserialize them.
>>>
>>> Signed-off-by: Michael Roth<address@hidden>
>>> ---
>>>   scripts/qc.md |  331 ++++++++++++++++++++++++++++++++++++++
>>>   scripts/qc.py |  494 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 825 insertions(+), 0 deletions(-)
>>>   create mode 100644 scripts/qc.md
>>>   create mode 100755 scripts/qc.py
>>>
>>> diff --git a/scripts/qc.md b/scripts/qc.md
>>> new file mode 100644
>>> index 0000000..4cf4b21
>>> --- /dev/null
>>> +++ b/scripts/qc.md
>>
>> I think docs/ would be a better place than scripts/
>>
>>> +Getting Started
>>> +---------------
>>> +
>>> +The first step is to move your device struct definition to a header file.  
>>> This
>>> +header file should only contain the struct definition and any preprocessor
>>> +declarations you need to define the structure.  This header file will act 
>>> as
>>> +the source for the QC IDL compiler.
>>> +
>>> +Do not include any function declarations in this header file as QC does not
>>> +understand function declarations.
>>
>> Couldn't we use a header file (or even source file) that has some magic
>> markers for the IDL compiler? Like:
>>
>> ... random stuff ...
>>
>> /* QIDL START */
>> struct Foo {
>>      ...
>> };
>> /* QIDL END */
>>
>> ... random stuff ...
>>
>> Adding a new header file for each device really doesn't look like a
>> desirable thing, and this way it could be avoided.
> 
> I'm pretty sure the version of QIDL I have locally actually handles defines 
> and 
> function declarations just fine so don't consider this a limitation anymore.

Can it parse actual source files, too? Many devices don't have a
separate header file today, and I'm not sure that they should have one.

>>> +Determining What State Gets Saved
>>> +---------------------------------
>>> +
>>> +By default, QC saves every field in a structure it sees.  This provides 
>>> maximum
>>> +correctness by default.  However, device structures generally contain state
>>> +that reflects state that is in someway duplicated or not guest visible.  
>>> This
>>> +more often that not reflects design implementation details.
>>> +
>>> +Since design implementation details change over time, saving this state 
>>> makes
>>> +compatibility hard to maintain since it would effectively lock down a 
>>> device's
>>> +implementation.
>>> +
>>> +QC allows a device author to suppress certain fields from being saved 
>>> although
>>> +there are very strict rules about when this is allowed and what needs to 
>>> be done
>>> +to ensure that this does not impact correctness.
>>> +
>>> +There are three cases where state can be suppressed: when it is 
>>> **immutable**,
>>> +**derived**, or **broken**.  In addition, QC can decide at run time 
>>> whether to
>>> +suppress a field by assigning it a **default** value.
>>
>> What about fields that have a known state or whose state doesn't matter?
>> For example, when migrating a disk, we know that no I/O requests are in
>> flight because they are flushed before sending the device state.
>>
>> The fields describing the in-flight request are not immutable, because
>> the do change a lot during runtime. They are not really derived either
>> because they don't depend on other fields and they don't need a
>> post-load function. Broken implies that there is a bug, but there isn't.
>>
>> It's just that their value after migration simply doesn't matter.
> 
> It's not that it doesn't matter, it's that its value is well known and 
> constant 
> and therefore doesn't need to be migrated. 

Assuming that we clear the values each time after a request has
completed, yes. Not completely sure if this covers all cases, but
possibly it does.

> IOW:
> 
> GSList _type_is(SCSIRequest) *pending_reqs _default_is(NULL);
> 
> This is valid QIDL syntax today and provides what you want.  Yes, I know that 
> you don't like GSList :-)

Seems I'm quite predictable: The time between reading the code line and
your comment was long enough to think "go away with your stupid GSList". ;-)

>>> +It can be subtle whether a field is truly immutable.  A good example is a
>>> +*QEMUTimer*.  Timer's will usually have their timeout modified with a call 
>>> to
>>> +*qemu_mod_timer()* even though they are only assigned in the device
>>> +initialization function.
>>> +
>>> +If the timer is always modified with a fixed value that is not dependent on
>>> +guest state, then the timer is immutable since it's unaffected by the 
>>> state of
>>> +the guest.
>>> +
>>> +On the other hand, if the timer is modified based on guest state (such as a
>>> +guest programmed time out), then the timer carries state.  It may be 
>>> necessary
>>> +to save/restore the timer or mark it as **_derived** and work with it
>>> +accordingly.
>>
>> Another instance of the above problem: What if a timer value is changed
>> dynamically for example as an optimisation, but for correctness it's not
>> really important what it is?
>>
>> And even with a fixed timeout, as long as it's guest visible, can you
>> avoid transferring it, strictly speaking? It carries information about
>> when the next timeout occurs.
> 
> If you have a fixed timeout that began at a well known time relative to 
> vm_clock, since vm_clock gets migrated during migration, the deadline on the 
> destination will be exactly the same as the deadline on the host.  This is 
> where 
> you can mark the timer as _immutable.  Anything else needs to be transfered.
> 
> You'll notice that a *lot* of QEMU state actually needs to be transfered and 
> is 
> not today.  There's always a good reason why migration appears to work 
> (guests 
> are very tolerant to time jitter and things like that).  But that doesn't 
> mean 
> what we're doing today is correct.  We just get away with it based on luck.

Yes. I'm not surprised about that.

>>> +### Default Values
>>> +
>>> +In many cases, a field that gets marked broken was not originally saved 
>>> because
>>> +in the vast majority of the time, the field does not contain a meaningful 
>>> value.
>>> +
>>> +In the case of our *thr* example, the field usually does not have a 
>>> meaningful
>>> +value.
>>> +
>>> +Instead of always saving the field, QC has another mechanism that allows 
>>> the
>>> +field to be saved only when it has a meaningful value.  This is done using 
>>> the
>>> +**_default()** marker.  The default marker tells QC that if the field 
>>> currently
>>> +has a specific value, do not save the value as part of serialization.
>>> +
>>> +When loading a field, QC will assign the default value to the field before 
>>> it
>>> +tries to load the field.  If the field cannot be loaded, QC will ignore the
>>> +error and rely on the default value.
>>> +
>>> +Using default values, we can fix broken fields while also minimizing the 
>>> cases
>>> +where we break live migration compatibility.  The **_default()** marker 
>>> can be
>>> +used in conjunction with the **_broken** marker.  We can extend our 
>>> example as
>>> +follows:
>>> +
>>> +    typedef struct SerialDevice {
>>> +        SysBusDevice parent;
>>> +
>>> +
>>> +        uint8_t thr _default(0); // transmit holding register
>>> +        uint8_t lsr;             // line status register
>>> +        uint8_t ier;             // interrupt enable register
>>> +
>>> +        int _derived int_pending; // whether we have a pending queued 
>>> interrupt
>>> +        CharDriverState _immutable *chr;
>>> +    } SerialDevice;
>>> +
>>> +The following guidelines should be followed when using a default marker:
>>> +
>>> + 1. Is the field set to the default value both during device 
>>> initialization and
>>> +    whenever the field is no longer in use?
>>> +
>>> + 2. If the non-default value is expected to occur often, then consider 
>>> using the
>>> +    **_broken** marker along with the default marker and using a flag day 
>>> to
>>> +    remove the **_broken** marker.
>>> +
>>> + 3. In general, setting default values as the value during device 
>>> initialization
>>> +    is a good idea even if the field was never broken.  This gives us 
>>> maximum
>>> +    flexibility in the long term.
>>> +
>>> + 4. Never change a default value without renaming a field.  The default 
>>> value is
>>> +    part of the device's ABI.
>>> +
>>> +The first guideline is particularly important.  In the case of QEMU's real
>>> +*SerialDevice*, it would be necessary to add code to set the *thr* 
>>> register to
>>> +zero after the byte has been successfully transmitted.  Otherwise, it is
>>> +unlikely that it would ever contain the default value.
>>
>> Does this use subsections internally? How would we convert existing
>> subsections that have a specific name and may contain more than one
>> field? We also have is_needed functions that are more complex than just
>> comparing to a default value.
>>
>> So I guess my real question is what the concept for maintaining
>> migration compatibility is.
> 
> No, this isn't subsections.  I think subsections are too complex.  The goal 
> of 
> qidl was to come up with a easy to follow strategy for ending up with correct 
> migration.

Unless you accept breaking migration with older qemu versions,
subsections are a part of correct migration.

> An 'is_needed' function makes state transfer completely open ended and makes 
> it 
> far more difficult to review whether the end result is correct migration.
> 
> How we go from what we have today to qidl is an interesting question.  The 
> first 
> observation is that by marshalling to Visitors, there's no requirement that 
> what 
> QIDL takes as input or produces as output goes directly on the wire.
> 
> Instead, we can have an intermediate translation layer that converts from 
> today's wire format (complete with subsections) to a Visitor that ultimately 
> feeds into QIDL.  This part isn't ready yet but I encouraged Mike to share 
> the 
> series as is to start getting feedback on the approach.

As long as the definition of the wire format (including subsections)
stays in the device code, I don't really mind how it's implemented. It
shouldn't be in an entirely different place, though.

I'm also unsure whether reviewing this part makes a lot of sense as long
as the compatibility part is missing. We'll continue to mess up in the
future, and so it's important to discuss how future versions can fix it
without breaking everything.

Kevin