From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53798) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScPX7-0006nt-P8 for qemu-devel@nongnu.org; Wed, 06 Jun 2012 19:20:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ScPX5-0006Qd-1L for qemu-devel@nongnu.org; Wed, 06 Jun 2012 19:20:57 -0400 Received: from e1.ny.us.ibm.com ([32.97.182.141]:59014) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScPX4-0006QW-Rr for qemu-devel@nongnu.org; Wed, 06 Jun 2012 19:20:54 -0400 Received: from /spool/local by e1.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 6 Jun 2012 19:20:50 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 3E7776E804B for ; Wed, 6 Jun 2012 19:20:49 -0400 (EDT) Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q56NKmCt164342 for ; Wed, 6 Jun 2012 19:20:48 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q56NKlgY021622 for ; Wed, 6 Jun 2012 17:20:47 -0600 Message-ID: <4FCFE5C5.2050202@us.ibm.com> Date: Thu, 07 Jun 2012 07:20:37 +0800 From: Anthony Liguori MIME-Version: 1.0 References: <1338858018-17189-1-git-send-email-mdroth@linux.vnet.ibm.com> <1338858018-17189-2-git-send-email-mdroth@linux.vnet.ibm.com> <4FCDDA12.4040907@redhat.com> In-Reply-To: <4FCDDA12.4040907@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: yamahata@valinux.co.jp, quintela@redhat.com, Michael Roth , qemu-devel@nongnu.org, owasserm@redhat.com, pbonzini@redhat.com, akong@redhat.com, afaerber@suse.de On 06/05/2012 06:06 PM, 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. > > >> +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? >> + > > > > 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]+') > > > > Surely there are available lexer/parser packages? Not part of the standard Python package. C is notoriously irregular too (even lexically) so even with a proper lexer, you tend to have to handle certain things with open coding (floating point literals and comments are a good example). The subset of grammar we're supporting is so simple that I think our current approach is pretty reasonable. Regards, Anthony Liguori >