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

[Kevin Wolf]

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.

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

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

Kevin