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

[Avi Kivity <avi@redhat.com>]

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.


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

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.

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.


> 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?

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


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