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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Tue, Jun 05, 2012 at 01:06:10PM +0300, Avi Kivity wrote:
> On 06/05/2012 04:00 AM, Michael Roth wrote:
> > 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.
> > 
> > +
> > +    typedef struct SerialDevice {
> > +        SysBusDevice parent;
> > +    
> > +        uint8_t thr;            // transmit holding register
> > +        uint8_t lsr;            // line status register
> > +        uint8_t ier;            // interrupt enable register
> > +    
> > +        int int_pending;        // whether we have a pending queued interrupt
> > +        CharDriverState *chr;   // backend
> > +    } SerialDevice;
> > +
> > +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.
> > +
> 
> Is is possible to let the compiler process the .c file, with the IDL
> delimited by some marker?  I like how device models are self contained
> in one file now.

It's possible, but only if we inject the generated visitor code into the
devices via an #include "qapi-generated/<device>-qapi-visit.c";

I'm not sure how acceptable that is... but it does reduce the churn
quite a bit.

> 
> 
> > +Do not include any function declarations in this header file as QC does not
> > +understand function declarations.
> > +
> > +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**.  
> 
> There is a fourth class, non-guest-visible state (below).  There is a
> fifth class, migrated by other means, which includes memory and block
> device state, but of course it isn't interesting in this context.

There's a higher-level annotation, qc_declaration, which denotes what
devices/structs should be processed by the QIDL compiler (and follow
it's rules). So there's an implied "handled by other means" for
everything that falls outside this category.

> 
> In addition, QC can decide at run time whether to
> > +suppress a field by assigning it a **default** value.
> > +
> > +## Immutable Fields
> > +
> > +If a field is only set during device construction, based on parameters passed to
> > +the device's constructor, then there is no need to send save and restore this
> > +value.  We call these fields immutable and we tell QC about this fact by using
> > +a **_immutable** marker.
> > +
> > +In our *SerialDevice* example, the *CharDriverState* pointer reflects the host
> > +backend that we use to send serial output to the user.  This is only assigned
> > +during device construction and never changes.  This means we can add an
> > +**_immutable** marker to it:
> 
> Even if it does change (suppose we add a monitor command to retarget a
> the serial device's chardev), it need not be migrated since it doesn't
> describe guest state, only host state.  Maybe we should mark *chr _host
> instead of _immutable.
> 
> > +
> > +    typedef struct SerialDevice {
> > +        SysBusDevice parent;
> > +    
> > +        uint8_t thr;            // transmit holding register
> > +        uint8_t lsr;            // line status register
> > +        uint8_t ier;            // interrupt enable register
> > +    
> > +        int int_pending;        // whether we have a pending queued interrupt
> > +        CharDriverState _immutable *chr;
> > +    } SerialDevice;
> > +
> > +When reviewing patches that make use of the **_immutable** marker, the following
> > +guidelines should be followed to determine if the marker is being used
> > +correctly.
> > +
> > + 1. Check to see if the field is assigned anywhere other than the device
> > +    initialization function.
> > +
> > + 2. Check to see if any function is being called that modifies the state of the
> > +    field outside of the initialization function.
> > +
> > +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.
> 
> I think this is where _host is useful.  You can have two QEMUTimers, one
> driven by the guest and the other by the host (like, say, the display
> refresh timer).  You would want to migrate one but not the other.
> 
> > +
> > +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.
> > +
> > +### Derived Fields
> > +
> > +If a field is set based on some other field in the device's structure, then its
> > +value is derived.  Since this is effectively duplicate state, we can avoid
> > +sending it and then recompute it when we need to.  Derived state requires a bit
> > +more handling that immutable state.
> > +
> > +In our *SerialDevice* example, our *int_pending* flag is really derived from
> > +two pieces of state.  It is set based on whether interrupts are enabled in the
> > +*ier* register and whether there is *THRE* flag is not set in the *lsr*
> > +register.
> 
> Device model authors should be encouraged to avoid derived state in
> simple cases like that, instead use a function.
> 
> > +
> > +To mark a field as derived, use the **_derived** marker.  To update our
> > +example, we would do:
> > +
> > +    typedef struct SerialDevice {
> > +        SysBusDevice parent;
> > +    
> > +        uint8_t thr;            // 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;
> > +
> > +There is one other critical step needed when marking a field as derived.  A
> > +*post_load* function must be added that updates this field after loading the
> > +rest of the device state.  This function is implemented in the device's source
> > +file, not in the QC header.  Below is an example of what this function may do:
> > +
> > +    static void serial_post_load(SerialDevice *s)
> > +    {
> > +        s->int_pending = !(s->lsr & THRE) && (s->ier & INTE);
> > +    }
> > +
> > +When reviewing a patch that marks a field as *_derived*, the following criteria
> > +should be used:
> > +
> > + 1. Does the device have a post load function?
> > +
> > + 2. Does the post load function assign a value to all of the derived fields?
> > +
> > + 3. Are there any obvious places where a derived field is holding unique state?
> > +
> 
> <snip>
> 
> Suggestion: add a _guest marker for ordinary state.  Fail the build on
> unmarked fields.  This ensures that some thought is given to each field,
> instead of having a default that may be correct most of the time, but
> not always.

Hmm, I my general thought was that is doesn't hurt to send extra, which
made serialization a reasonable default course of action.

But there is indeed a risk of overwriting target state with garbage if
we don't verify what fields really should/shouldn't be sent. A marker to
track this does seem useful in that regard...

> 
> Suggestion: add a mandatory position hint (_guest(7) or _pos(7)) that
> generates ordering within the fields.  This decouples the order of lines
> in the struct from the protocol, so you can add state where it make
> sense, or rearrange lines when they don't, and detect copy/paste errors.
> 

I'm in agreement with Gerd that the wire protocol we use should support
field names. I think device state constitutes a small enough percentage
of total migrated state that the performance impact would be negligable,
and migration will invariably add some negotiation/compatibility
functionality on top of the serialization that would make having field
names intact useful for analyzing/debugging things.

I personally like the idea of using compressed json, but I think we
could implement a QObject<->BER mechanism that would provide this as
well.

> 
> > diff --git a/scripts/qc.py b/scripts/qc.py
> > new file mode 100755
> > index 0000000..74f2a40
> > --- /dev/null
> > +++ b/scripts/qc.py
> > @@ -0,0 +1,494 @@
> > +#!/usr/bin/python
> > +
> > +import sys
> > +from ordereddict import OrderedDict
> > +
> > +marker = "qc_declaration"
> > +marked = False
> > +
> > +class Input(object):
> > +    def __init__(self, fp):
> > +        self.fp = fp
> > +        self.buf = ''
> > +        self.eof = False
> > +
> > +    def pop(self):
> > +        if len(self.buf) == 0:
> > +            if self.eof:
> > +                return ''
> > +
> > +            data = self.fp.read(1024)
> > +            if data == '':
> > +                self.eof = True
> > +                return ''
> > +
> > +            self.buf += data
> > +
> > +        ch = self.buf[0]
> > +        self.buf = self.buf[1:]
> > +        return ch
> 
> 
> Could be simplified as fp.read(1).  Does the performance really suffer
> if you read a byte at a time?

Currently, for 100 annotated header files identical to
mc146818rtc_state.h, it's about 4 seconds for me, reading from ssd, writing to
stdout. With the change it's roughly 4.2s. So yah, seems like a nice
simplification.

> 
> > +
> > +def in_range(ch, start, end):
> > +    if ch >= start and ch <= end:
> > +        return True
> > +    return False
> > +
> > +# D			[0-9]
> > +# L			[a-zA-Z_]
> > +# H			[a-fA-F0-9]
> > +# E			[Ee][+-]?{D}+
> > +# FS			(f|F|l|L)
> > +# IS			(u|U|l|L)*
> > +
> > +def is_D(ch):
> > +    return in_range(ch, '0', '9')
> > +
> > +def is_L(ch):
> > +    return in_range(ch, 'a', 'z') or in_range(ch, 'A', 'Z') or ch == '_'
> > +
> > +def is_H(ch):
> > +    return in_range(ch, 'a', 'f') or in_range(ch, 'A', 'F') or is_D(ch)
> > +
> > +def is_FS(ch):
> > +    return ch in 'fFlL'
> > +
> > +def is_IS(ch):
> > +    return ch in 'uUlL'
> 
> 
> import re
> 
> D = re.compile(r'[0-9]')
> ...
> IS = re.compile(r'[uUlL]+')
> 
> <snip parser>
> 
> Surely there are available lexer/parser packages?

This seems promising:

http://pygments.org/docs/lexerdevelopment/

> 
> 
> -- 
> error compiling committee.c: too many arguments to function
>