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

[Anthony Liguori <aliguori@us.ibm.com>]

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<mdroth@linux.vnet.ibm.com>
>> ---
>>   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.

>
>> +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.  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 :-)

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

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

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.

Regards,

Anthony Liguori

>
> Kevin
>