* [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:57 ` Anthony Liguori
` (3 more replies)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 02/17] qidl: add qc definitions Michael Roth
` (15 subsequent siblings)
16 siblings, 4 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
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
@@ -0,0 +1,331 @@
+How to Serialize Device State with QC
+======================================
+
+This document describes how to implement save/restore of a device in QEMU using
+the QC IDL compiler. The QC IDL compiler makes it easier to support live
+migration in devices by converging the serialization description with the
+device type declaration. It has the following features:
+
+ 1. Single description of device state and how to serialize
+
+ 2. Fully inclusive serialization description--fields that aren't serialized
+ are explicitly marked as such including the reason why.
+
+ 3. Optimized for the common case. Even without any special annotations,
+ many devices will Just Work out of the box.
+
+ 4. Build time schema definition. Since QC runs at build time, we have full
+ access to the schema during the build which means we can fail the build if
+ the schema breaks.
+
+For the rest, of the document, the following simple device will be used as an
+example.
+
+ 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.
+
+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**. 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:
+
+ 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.
+
+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.
+
+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?
+
+### Broken State
+
+QEMU does migration with a lot of devices today. When applying this methodology
+to these devices, one will quickly discover that there are a lot of fields that
+are not being saved today that are not derived or immutable state.
+
+These are all bugs. It just so happens that these bugs are usually not very
+serious. In many cases, they cause small functionality glitches that so far
+have not created any problems.
+
+Consider our *SerialDevice* example. In QEMU's real *SerialState* device, the
+*thr* register is not saved yet we have not marked it immutable or derived.
+
+The *thr* register is a temporary holding register that the next character to
+transmit is placed in while we wait for the next baud cycle. In QEMU, we
+emulate a very fast baud rate regardless of what guest programs. This means
+that the contents of the *thr* register only matter for a very small period of
+time (measured in microseconds).
+
+The likelihood of a migration converging in that very small period of time when
+the *thr* register has a meaningful value is very small. Moreover, the worst
+thing that can happen by not saving this register is that we lose a byte in the
+data stream. Even if this has happened in practice, the chances of someone
+noticing this as a bug is pretty small.
+
+Nonetheless, this is a bug and needs to be eventually fixed. However, it would
+be very inconvenient to constantly break migration by fixing all of these bugs
+one-by-one. Instead, QC has a **_broken** marker. This indicates that a field
+is not currently saved, but should be in the future.
+
+The idea behind the broken marker is that we can convert a large number of
+devices without breaking migration compatibility, and then institute a flag day
+where we go through and remove broken markers en-mass.
+
+Below is an update of our example to reflect our real life serial device:
+
+ typedef struct SerialDevice {
+ SysBusDevice parent;
+
+ uint8_t _broken 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;
+
+When reviewing the use of the broken marker, the following things should be
+considered:
+
+ 1. What are the ramifications of not sending this data field?
+
+ 2. If the not sending this data field can cause data corruption or very poor
+ behavior within the guest, the broken marker is not appropriate to use.
+
+ 3. Assigning a default value to a field can also be used to fix a broken field
+ without significantly impacting live migration compatibility.
+
+### 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.
+
+Arrays
+------
+
+QC has support for multiple types of arrays. The following sections describe
+the different rules for arrays.
+
+Fixed Sized Arrays
+------------------
+
+A fixed sized array has a size that is known at build time. A typical example
+would be:
+
+ struct SerialFIFO {
+ uint8_t data[UART_FIFO_LENGTH];
+ uint8_t count;
+ uint8_t itl;
+ uint8_t tail;
+ uint8_t head;
+ };
+
+In this example, *data* is a fixed sized array. No special annotation is needed
+for QC to marshal this area correctly. The following guidelines apply to
+fixed sized arrays:
+
+ 1. The size of the array is part of the device ABI. It should not change
+ without renaming the field.
+
+Variable Sized, Fixed Capacity Arrays
+-------------------------------------
+
+Sometimes it's desirable to have a variable sized array. QC currently supported
+variable sized arrays provided that the maximum capacity is fixed and part of
+the device structure memory.
+
+A typical example would be a slightly modified version of our above example:
+
+ struct SerialFIFO {
+ uint8_t count;
+ uint8_t _size_is(count) data[UART_FIFO_LENGTH];
+ uint8_t itl;
+ uint8_t tail;
+ uint8_t head;
+ };
+
+In this example, *data* is a variable sized array with a fixed capacity of
+*UART_FIFO_LENGTH*. When we serialize, we want only want to serialize *count*
+members.
+
+The ABI implications of capacity are a bit more relaxed with variable sized
+arrays. In general, you can increase or decrease the capacity without breaking
+the ABI although you may cause some instances of migration to fail between
+versions of QEMU with different capacities.
+
+When reviewing variable sized, fixed capacity arrays, keep the following things
+in mind:
+
+ 1. The variable size must occur before the array element in the state
+ structure.
+
+ 2. The capacity can change without breaking the ABI, but care should be used
+ when making these types of changes.
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
+
+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'
+
+def find_marker(ch, fp):
+ global marked
+
+ # scan for marker before full processing
+
+ while not marked and not fp.eof:
+ token = ''
+ if is_L(ch):
+ token += ch
+ while True:
+ ch = fp.pop()
+ if not is_L(ch) and not is_D(ch):
+ break
+ token += ch
+ if token == marker:
+ marked = True
+ return
+ ch = fp.pop()
+ return
+
+def lexer(fp):
+ global marked
+ ch = fp.pop()
+
+ while not fp.eof:
+ if not marked:
+ find_marker(ch, fp)
+ ch = fp.pop()
+ token = ''
+
+ if is_L(ch):
+ token += ch
+
+ ch = fp.pop()
+ while is_L(ch) or is_D(ch):
+ token += ch
+ ch = fp.pop()
+ if token in [ 'auto', 'break', 'case', 'const', 'continue',
+ 'default', 'do', 'else', 'enum', 'extern',
+ 'for', 'goto', 'if', 'register', 'return', 'signed',
+ 'sizeof',
+ 'static', 'struct', 'typedef', 'union', 'unsigned',
+ 'void', 'volatile', 'while' ]:
+ yield (token, token)
+ else:
+ yield ('symbol', token)
+ elif ch == "'":
+ token += ch
+
+ ch = fp.pop()
+ if ch == '\\':
+ token += ch
+ token += fp.pop()
+ else:
+ token += ch
+ token += fp.pop()
+ ch = fp.pop()
+ yield ('literal', token)
+ elif ch == '"':
+ token += ch
+
+ ch = fp.pop()
+ while ch not in ['', '"']:
+ token += ch
+ if ch == '\\':
+ token += fp.pop()
+ ch = fp.pop()
+ token += ch
+ yield ('literal', token)
+ ch = fp.pop()
+ elif ch in '.><+-*/%&^|!;{},:=()[]~?':
+ token += ch
+ ch = fp.pop()
+ tmp_token = token + ch
+ if tmp_token in ['<:']:
+ yield ('operator', '[')
+ ch = fp.pop()
+ elif tmp_token in [':>']:
+ yield ('operator', ']')
+ ch = fp.pop()
+ elif tmp_token in ['<%']:
+ yield ('operator', '{')
+ ch = fp.pop()
+ elif tmp_token in ['%>']:
+ yield ('operator', '}')
+ ch = fp.pop()
+ elif tmp_token == '//':
+ token = tmp_token
+ ch = fp.pop()
+ while ch != '\n' and ch != '':
+ token += ch
+ ch = fp.pop()
+ yield ('comment', token)
+ elif tmp_token == '/*':
+ token = tmp_token
+
+ ch = fp.pop()
+ while True:
+ while ch != '*':
+ token += ch
+ ch = fp.pop()
+ token += ch
+ ch = fp.pop()
+ if ch == '/':
+ token += ch
+ break
+ yield ('comment', token)
+ ch = fp.pop()
+ elif tmp_token in [ '+=', '-=', '*=', '/=', '%=', '&=', '^=',
+ '|=', '>>', '<<', '++', '--', '->', '&&',
+ '||', '<=', '>=', '==', '!=' ]:
+ yield ('operator', tmp_token)
+ ch = fp.pop()
+ else:
+ yield ('operator', token)
+ elif ch == '0':
+ token += ch
+ ch = fp.pop()
+ if ch in 'xX':
+ token += ch
+ ch = fp.pop()
+ while is_H(ch):
+ token += ch
+ ch = fp.pop()
+ while is_IS(ch):
+ token += ch
+ ch = fp.pop()
+ elif is_D(ch):
+ token += ch
+ ch = fp.pop()
+ while is_D(ch):
+ token += ch
+ ch = fp.pop()
+ yield ('literal', token)
+ elif is_D(ch):
+ token += ch
+ ch = fp.pop()
+ while is_D(ch):
+ token += ch
+ ch = fp.pop()
+ yield ('literal', token)
+ elif ch in ' \t\v\n\f':
+ token += ch
+ ch = fp.pop()
+ while len(ch) and ch in ' \t\v\n\f':
+ token += ch
+ ch = fp.pop()
+ yield ('whitespace', token)
+ elif ch in '#':
+ token += ch
+ ch = fp.pop()
+ while len(ch) and ch != '\n':
+ token += ch
+ ch = fp.pop()
+ yield ('directive', token)
+ else:
+ yield ('unknown', ch)
+ ch = fp.pop()
+
+class LookAhead(object):
+ def __init__(self, container):
+ self.i = container.__iter__()
+ self.la = []
+ self.full = False
+
+ def at(self, i):
+ if not marked:
+ self.la = []
+ self.full = False
+ if i >= len(self.la):
+ if self.full:
+ raise StopIteration()
+ else:
+ try:
+ self.la.append(self.i.next())
+ except StopIteration, e:
+ self.full = True
+ raise StopIteration()
+
+ return self.la[i]
+
+ def eof(self):
+ try:
+ self.at(len(self.la))
+ except StopIteration, e:
+ return True
+
+ return False
+
+def skip(c):
+ for token, value in c:
+ if token in ['whitespace', 'comment', 'directive']:
+ continue
+ yield (token, value)
+
+def expect(la, index, first, second=None):
+ if la.at(index)[0] != first:
+ raise Exception("expected '%s', got %s %s" % (first, la.at(index)[0], la.at(index)[1]))
+ if second != None:
+ if la.at(index)[1] != second:
+ raise Exception("expected '%s', got %s" % (second, la.at(index)[1]))
+ return index + 1, la.at(index)[1]
+
+def choice(la, index, first, second=None):
+ if la.at(index)[0] != first:
+ return False
+ if second != None:
+ if la.at(index)[1] != second:
+ return False
+ return True
+
+def parse_type(la, index):
+ next = index
+
+ typename = ''
+ if choice(la, next, 'struct', 'struct'):
+ typename = 'struct '
+ next += 1
+
+ next, rest = expect(la, next, 'symbol')
+ typename += rest
+
+ ret = { 'type': typename }
+
+ if choice(la, next, 'symbol', '_derived'):
+ next += 1
+ ret['is_derived'] = True
+ elif choice(la, next, 'symbol', '_immutable'):
+ next += 1
+ ret['is_immutable'] = True
+ elif choice(la, next, 'symbol', '_broken'):
+ next += 1
+ ret['is_broken'] = True
+ elif choice(la, next, 'symbol', '_version'):
+ next += 1
+
+ next, _ = expect(la, next, 'operator', '(')
+ next, version = expect(la, next, 'literal')
+ next, _ = expect(la, next, 'operator', ')')
+
+ ret['version'] = version
+ elif choice(la, next, 'symbol', '_size_is'):
+ next += 1
+
+ next, _ = expect(la, next, 'operator', '(')
+ next, array_size = expect(la, next, 'symbol')
+ next, _ = expect(la, next, 'operator', ')')
+
+ ret['is_array'] = True
+ ret['array_size'] = array_size
+
+ if choice(la, next, 'operator', '*'):
+ next += 1
+ ret['is_pointer'] = True
+
+ next, variable = expect(la, next, 'symbol')
+ ret['variable'] = variable
+
+ if choice(la, next, 'operator', '['):
+ next += 1
+
+ if not ret.has_key('is_array'):
+ ret['is_array'] = True
+ ret['array_size'] = la.at(next)[1]
+ else:
+ ret['array_capacity'] = la.at(next)[1]
+ next += 1
+
+ next, _ = expect(la, next, 'operator', ']')
+
+ if choice(la, next, 'symbol', '_default'):
+ next += 1
+
+ next, _ = expect(la, next, 'operator', '(')
+ next, default = expect(la, next, 'literal')
+ next, _ = expect(la, next, 'operator', ')')
+
+ ret['default'] = default
+
+ next, _ = expect(la, next, 'operator', ';')
+
+ return (next - index), ret
+
+def parse_struct(la, index):
+ next = index
+
+ next, _ = expect(la, next, 'struct', 'struct')
+
+ name = None
+ if choice(la, next, 'symbol'):
+ name = la.at(next)[1]
+ next += 1
+
+ next, _ = expect(la, next, 'operator', '{')
+
+ nodes = []
+
+ while not choice(la, next, 'operator', '}'):
+ offset, node = parse_type(la, next)
+ next += offset
+ nodes.append(node)
+
+ next += 1
+
+ return (next - index), { 'struct': name, 'fields': nodes }
+
+def parse_typedef(la, index):
+ next = index
+
+ next, _ = expect(la, next, 'typedef', 'typedef')
+
+ offset, node = parse_struct(la, next)
+ next += offset
+
+ next, typename = expect(la, next, 'symbol')
+
+ return (next - index), { 'typedef': typename, 'type': node }
+
+def qapi_format(node, is_save=True):
+ if node.has_key('typedef'):
+ dtype = node['typedef']
+ fields = node['type']['fields']
+ else:
+ dtype = node['struct']
+ fields = node['fields']
+
+ if is_save:
+ print 'void qc_save_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
+ else:
+ print 'void qc_load_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
+ print '{'
+ print ' visit_start_struct(v, "%s", name, errp);' % (dtype)
+ for field in fields:
+ if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
+ continue
+
+ if field['type'].endswith('_t'):
+ typename = field['type'][:-2]
+ else:
+ typename = field['type']
+
+ if field.has_key('is_array'):
+ if field.has_key('array_capacity'):
+ print ' if (%(array_size)s > %(array_capacity)s) {' % field
+ print ' error_set(errp, QERR_FAULT, "Array size greater than capacity.");'
+ print ' }'
+ print ' %(array_size)s = MIN(%(array_size)s, %(array_capacity)s);' % field
+ print ' visit_start_array(v, "%s", errp);' % (field['variable'])
+ print ' for (size_t i = 0; i < %s; i++) {' % (field['array_size'])
+ print ' visit_type_%s(v, &s->%s[i], NULL, errp);' % (typename, field['variable'])
+ print ' }'
+ print ' visit_end_array(v, errp);'
+ elif field.has_key('default'):
+ if is_save:
+ print ' if (s->%s != %s) {' % (field['variable'], field['default'])
+ print ' visit_type_%s(v, &s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
+ print ' }'
+ else:
+ print ' s->%s = %s;' % (field['variable'], field['default'])
+ print ' visit_type_%s(v, &s->%s, "%s", NULL);' % (typename, field['variable'], field['variable'])
+ else:
+ print ' visit_type_%s(v, &s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
+ print ' visit_end_struct(v, errp);'
+ print '}'
+ print
+
+import json
+
+def type_dump(node):
+ print json.dumps(node, sort_keys=True, indent=4)
+
+def qapi_schema(node):
+ schema = OrderedDict()
+ data = OrderedDict()
+ fields = None
+ if node.has_key('typedef'):
+ schema['type'] = node['typedef']
+ fields = node['type']['fields']
+ elif node.has_key('struct'):
+ schema['type'] = node['struct']
+ fields = node['fields']
+ else:
+ raise Exception("top-level neither typedef nor struct")
+
+ for field in fields:
+ if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
+ continue
+
+ description = {}
+
+ if field['type'].endswith('_t'):
+ typename = field['type'][:-2]
+ elif field['type'].startswith('struct '):
+ typename = field['type'].split(" ")[1]
+ else:
+ typename = field['type']
+
+ if field.has_key('is_array') and field['is_array']:
+ description['type'] = [typename]
+ description['<annotated>'] = 'true'
+ if field.has_key('array_size'):
+ description['array_size'] = field['array_size']
+ if field.has_key('array_capacity'):
+ description['array_capacity'] = field['array_capacity']
+ else:
+ #description['type'] = typename
+ description = typename
+
+ data[field['variable']] = description
+
+ schema['data'] = data
+ print json.dumps(schema).replace("\"", "'")
+
+
+if __name__ == '__main__':
+ la = LookAhead(skip(lexer(Input(sys.stdin))))
+
+ index = 0
+ while True:
+ try:
+ if choice(la, index, 'typedef'):
+ offset, node = parse_typedef(la, index)
+ elif choice(la, index, 'struct'):
+ offset, node = parse_struct(la, index)
+ else:
+ continue
+
+ index, _ = expect(la, index + offset, 'operator', ';')
+ marked = False
+ index = 0
+ except StopIteration, e:
+ break
+
+ #qapi_format(node, True)
+ #qapi_format(node, False)
+ #type_dump(node)
+ qapi_schema(node)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
@ 2012-06-05 1:57 ` Anthony Liguori
2012-06-05 9:25 ` Kevin Wolf
` (2 subsequent siblings)
3 siblings, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 1:57 UTC (permalink / raw)
To: Michael Roth
Cc: Peter Maydell, quintela, owasserm, qemu-devel, yamahata,
Avi Kivity, pbonzini, akong, afaerber
Hi,
Thanks for sending this Mike. If you're on CC, please do read the qc.md at
least. There is a good bit of theory here about how to rigorously approach
serialization of device state. I think the approach is sound and could solve a
lot of the problems we face today but it could use review.
On 06/05/2012 09: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.
>
> 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
> @@ -0,0 +1,331 @@
> +How to Serialize Device State with QC
> +======================================
> +
> +This document describes how to implement save/restore of a device in QEMU using
> +the QC IDL compiler. The QC IDL compiler makes it easier to support live
> +migration in devices by converging the serialization description with the
> +device type declaration. It has the following features:
> +
> + 1. Single description of device state and how to serialize
> +
> + 2. Fully inclusive serialization description--fields that aren't serialized
> + are explicitly marked as such including the reason why.
> +
> + 3. Optimized for the common case. Even without any special annotations,
> + many devices will Just Work out of the box.
> +
> + 4. Build time schema definition. Since QC runs at build time, we have full
> + access to the schema during the build which means we can fail the build if
> + the schema breaks.
> +
> +For the rest, of the document, the following simple device will be used as an
> +example.
> +
> + 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.
> +
> +Do not include any function declarations in this header file as QC does not
> +understand function declarations.
This is out of date I think. I believe that QC now supports parsing 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**. 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:
> +
> + 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.
> +
> +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.
> +
> +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?
> +
> +### Broken State
> +
> +QEMU does migration with a lot of devices today. When applying this methodology
> +to these devices, one will quickly discover that there are a lot of fields that
> +are not being saved today that are not derived or immutable state.
> +
> +These are all bugs. It just so happens that these bugs are usually not very
> +serious. In many cases, they cause small functionality glitches that so far
> +have not created any problems.
> +
> +Consider our *SerialDevice* example. In QEMU's real *SerialState* device, the
> +*thr* register is not saved yet we have not marked it immutable or derived.
> +
> +The *thr* register is a temporary holding register that the next character to
> +transmit is placed in while we wait for the next baud cycle. In QEMU, we
> +emulate a very fast baud rate regardless of what guest programs. This means
> +that the contents of the *thr* register only matter for a very small period of
> +time (measured in microseconds).
> +
> +The likelihood of a migration converging in that very small period of time when
> +the *thr* register has a meaningful value is very small. Moreover, the worst
> +thing that can happen by not saving this register is that we lose a byte in the
> +data stream. Even if this has happened in practice, the chances of someone
> +noticing this as a bug is pretty small.
> +
> +Nonetheless, this is a bug and needs to be eventually fixed. However, it would
> +be very inconvenient to constantly break migration by fixing all of these bugs
> +one-by-one. Instead, QC has a **_broken** marker. This indicates that a field
> +is not currently saved, but should be in the future.
> +
> +The idea behind the broken marker is that we can convert a large number of
> +devices without breaking migration compatibility, and then institute a flag day
> +where we go through and remove broken markers en-mass.
> +
> +Below is an update of our example to reflect our real life serial device:
> +
> + typedef struct SerialDevice {
> + SysBusDevice parent;
> +
> + uint8_t _broken 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;
> +
> +When reviewing the use of the broken marker, the following things should be
> +considered:
> +
> + 1. What are the ramifications of not sending this data field?
> +
> + 2. If the not sending this data field can cause data corruption or very poor
> + behavior within the guest, the broken marker is not appropriate to use.
> +
> + 3. Assigning a default value to a field can also be used to fix a broken field
> + without significantly impacting live migration compatibility.
> +
> +### 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.
> +
> +Arrays
> +------
> +
> +QC has support for multiple types of arrays. The following sections describe
> +the different rules for arrays.
> +
> +Fixed Sized Arrays
> +------------------
> +
> +A fixed sized array has a size that is known at build time. A typical example
> +would be:
> +
> + struct SerialFIFO {
> + uint8_t data[UART_FIFO_LENGTH];
> + uint8_t count;
> + uint8_t itl;
> + uint8_t tail;
> + uint8_t head;
> + };
> +
> +In this example, *data* is a fixed sized array. No special annotation is needed
> +for QC to marshal this area correctly. The following guidelines apply to
> +fixed sized arrays:
> +
> + 1. The size of the array is part of the device ABI. It should not change
> + without renaming the field.
> +
> +Variable Sized, Fixed Capacity Arrays
> +-------------------------------------
> +
> +Sometimes it's desirable to have a variable sized array. QC currently supported
> +variable sized arrays provided that the maximum capacity is fixed and part of
> +the device structure memory.
> +
> +A typical example would be a slightly modified version of our above example:
> +
> + struct SerialFIFO {
> + uint8_t count;
> + uint8_t _size_is(count) data[UART_FIFO_LENGTH];
> + uint8_t itl;
> + uint8_t tail;
> + uint8_t head;
> + };
> +
> +In this example, *data* is a variable sized array with a fixed capacity of
> +*UART_FIFO_LENGTH*. When we serialize, we want only want to serialize *count*
> +members.
> +
> +The ABI implications of capacity are a bit more relaxed with variable sized
> +arrays. In general, you can increase or decrease the capacity without breaking
> +the ABI although you may cause some instances of migration to fail between
> +versions of QEMU with different capacities.
> +
> +When reviewing variable sized, fixed capacity arrays, keep the following things
> +in mind:
> +
> + 1. The variable size must occur before the array element in the state
> + structure.
> +
> + 2. The capacity can change without breaking the ABI, but care should be used
> + when making these types of changes.
For future patches, please split the README from the script.
> 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 @@
Needs a copyright.
> +#!/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
> +
> +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'
> +
> +def find_marker(ch, fp):
> + global marked
> +
> + # scan for marker before full processing
> +
> + while not marked and not fp.eof:
> + token = ''
> + if is_L(ch):
> + token += ch
> + while True:
> + ch = fp.pop()
> + if not is_L(ch) and not is_D(ch):
> + break
> + token += ch
> + if token == marker:
> + marked = True
> + return
> + ch = fp.pop()
> + return
> +
> +def lexer(fp):
> + global marked
> + ch = fp.pop()
> +
> + while not fp.eof:
> + if not marked:
> + find_marker(ch, fp)
> + ch = fp.pop()
> + token = ''
> +
> + if is_L(ch):
> + token += ch
> +
> + ch = fp.pop()
> + while is_L(ch) or is_D(ch):
> + token += ch
> + ch = fp.pop()
> + if token in [ 'auto', 'break', 'case', 'const', 'continue',
> + 'default', 'do', 'else', 'enum', 'extern',
> + 'for', 'goto', 'if', 'register', 'return', 'signed',
> + 'sizeof',
> + 'static', 'struct', 'typedef', 'union', 'unsigned',
> + 'void', 'volatile', 'while' ]:
> + yield (token, token)
> + else:
> + yield ('symbol', token)
> + elif ch == "'":
> + token += ch
> +
> + ch = fp.pop()
> + if ch == '\\':
> + token += ch
> + token += fp.pop()
> + else:
> + token += ch
> + token += fp.pop()
> + ch = fp.pop()
> + yield ('literal', token)
> + elif ch == '"':
> + token += ch
> +
> + ch = fp.pop()
> + while ch not in ['', '"']:
> + token += ch
> + if ch == '\\':
> + token += fp.pop()
> + ch = fp.pop()
> + token += ch
> + yield ('literal', token)
> + ch = fp.pop()
> + elif ch in '.><+-*/%&^|!;{},:=()[]~?':
> + token += ch
> + ch = fp.pop()
> + tmp_token = token + ch
> + if tmp_token in ['<:']:
> + yield ('operator', '[')
> + ch = fp.pop()
> + elif tmp_token in [':>']:
> + yield ('operator', ']')
> + ch = fp.pop()
> + elif tmp_token in ['<%']:
> + yield ('operator', '{')
> + ch = fp.pop()
> + elif tmp_token in ['%>']:
> + yield ('operator', '}')
> + ch = fp.pop()
> + elif tmp_token == '//':
> + token = tmp_token
> + ch = fp.pop()
> + while ch != '\n' and ch != '':
> + token += ch
> + ch = fp.pop()
> + yield ('comment', token)
> + elif tmp_token == '/*':
> + token = tmp_token
> +
> + ch = fp.pop()
> + while True:
> + while ch != '*':
> + token += ch
> + ch = fp.pop()
> + token += ch
> + ch = fp.pop()
> + if ch == '/':
> + token += ch
> + break
> + yield ('comment', token)
> + ch = fp.pop()
> + elif tmp_token in [ '+=', '-=', '*=', '/=', '%=', '&=', '^=',
> + '|=', '>>','<<', '++', '--', '->','&&',
> + '||', '<=', '>=', '==', '!=' ]:
> + yield ('operator', tmp_token)
> + ch = fp.pop()
> + else:
> + yield ('operator', token)
> + elif ch == '0':
> + token += ch
> + ch = fp.pop()
> + if ch in 'xX':
> + token += ch
> + ch = fp.pop()
> + while is_H(ch):
> + token += ch
> + ch = fp.pop()
> + while is_IS(ch):
> + token += ch
> + ch = fp.pop()
> + elif is_D(ch):
> + token += ch
> + ch = fp.pop()
> + while is_D(ch):
> + token += ch
> + ch = fp.pop()
> + yield ('literal', token)
> + elif is_D(ch):
> + token += ch
> + ch = fp.pop()
> + while is_D(ch):
> + token += ch
> + ch = fp.pop()
> + yield ('literal', token)
> + elif ch in ' \t\v\n\f':
> + token += ch
> + ch = fp.pop()
> + while len(ch) and ch in ' \t\v\n\f':
> + token += ch
> + ch = fp.pop()
> + yield ('whitespace', token)
> + elif ch in '#':
> + token += ch
> + ch = fp.pop()
> + while len(ch) and ch != '\n':
> + token += ch
> + ch = fp.pop()
> + yield ('directive', token)
> + else:
> + yield ('unknown', ch)
> + ch = fp.pop()
This lexer is based off of http://www.lysator.liu.se/c/ANSI-C-grammar-l.html
It would be good to put a reference to this in the code.
> +class LookAhead(object):
> + def __init__(self, container):
> + self.i = container.__iter__()
> + self.la = []
> + self.full = False
> +
> + def at(self, i):
> + if not marked:
> + self.la = []
> + self.full = False
> + if i>= len(self.la):
> + if self.full:
> + raise StopIteration()
> + else:
> + try:
> + self.la.append(self.i.next())
> + except StopIteration, e:
> + self.full = True
> + raise StopIteration()
> +
> + return self.la[i]
> +
> + def eof(self):
> + try:
> + self.at(len(self.la))
> + except StopIteration, e:
> + return True
> +
> + return False
> +
> +def skip(c):
> + for token, value in c:
> + if token in ['whitespace', 'comment', 'directive']:
> + continue
> + yield (token, value)
> +
> +def expect(la, index, first, second=None):
> + if la.at(index)[0] != first:
> + raise Exception("expected '%s', got %s %s" % (first, la.at(index)[0], la.at(index)[1]))
> + if second != None:
> + if la.at(index)[1] != second:
> + raise Exception("expected '%s', got %s" % (second, la.at(index)[1]))
> + return index + 1, la.at(index)[1]
> +
> +def choice(la, index, first, second=None):
> + if la.at(index)[0] != first:
> + return False
> + if second != None:
> + if la.at(index)[1] != second:
> + return False
> + return True
> +
> +def parse_type(la, index):
> + next = index
> +
> + typename = ''
> + if choice(la, next, 'struct', 'struct'):
> + typename = 'struct '
> + next += 1
> +
> + next, rest = expect(la, next, 'symbol')
> + typename += rest
> +
> + ret = { 'type': typename }
> +
> + if choice(la, next, 'symbol', '_derived'):
> + next += 1
> + ret['is_derived'] = True
> + elif choice(la, next, 'symbol', '_immutable'):
> + next += 1
> + ret['is_immutable'] = True
> + elif choice(la, next, 'symbol', '_broken'):
> + next += 1
> + ret['is_broken'] = True
> + elif choice(la, next, 'symbol', '_version'):
> + next += 1
> +
> + next, _ = expect(la, next, 'operator', '(')
> + next, version = expect(la, next, 'literal')
> + next, _ = expect(la, next, 'operator', ')')
> +
> + ret['version'] = version
> + elif choice(la, next, 'symbol', '_size_is'):
> + next += 1
> +
> + next, _ = expect(la, next, 'operator', '(')
> + next, array_size = expect(la, next, 'symbol')
> + next, _ = expect(la, next, 'operator', ')')
> +
> + ret['is_array'] = True
> + ret['array_size'] = array_size
> +
> + if choice(la, next, 'operator', '*'):
> + next += 1
> + ret['is_pointer'] = True
> +
> + next, variable = expect(la, next, 'symbol')
> + ret['variable'] = variable
> +
> + if choice(la, next, 'operator', '['):
> + next += 1
> +
> + if not ret.has_key('is_array'):
> + ret['is_array'] = True
> + ret['array_size'] = la.at(next)[1]
> + else:
> + ret['array_capacity'] = la.at(next)[1]
> + next += 1
> +
> + next, _ = expect(la, next, 'operator', ']')
> +
> + if choice(la, next, 'symbol', '_default'):
> + next += 1
> +
> + next, _ = expect(la, next, 'operator', '(')
> + next, default = expect(la, next, 'literal')
> + next, _ = expect(la, next, 'operator', ')')
> +
> + ret['default'] = default
> +
> + next, _ = expect(la, next, 'operator', ';')
> +
> + return (next - index), ret
> +
> +def parse_struct(la, index):
> + next = index
> +
> + next, _ = expect(la, next, 'struct', 'struct')
> +
> + name = None
> + if choice(la, next, 'symbol'):
> + name = la.at(next)[1]
> + next += 1
> +
> + next, _ = expect(la, next, 'operator', '{')
> +
> + nodes = []
> +
> + while not choice(la, next, 'operator', '}'):
> + offset, node = parse_type(la, next)
> + next += offset
> + nodes.append(node)
> +
> + next += 1
> +
> + return (next - index), { 'struct': name, 'fields': nodes }
> +
> +def parse_typedef(la, index):
> + next = index
> +
> + next, _ = expect(la, next, 'typedef', 'typedef')
> +
> + offset, node = parse_struct(la, next)
> + next += offset
> +
> + next, typename = expect(la, next, 'symbol')
> +
> + return (next - index), { 'typedef': typename, 'type': node }
I think the rest should be a separate .py file.
> +
> +def qapi_format(node, is_save=True):
> + if node.has_key('typedef'):
> + dtype = node['typedef']
> + fields = node['type']['fields']
> + else:
> + dtype = node['struct']
> + fields = node['fields']
> +
> + if is_save:
> + print 'void qc_save_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
> + else:
> + print 'void qc_load_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
> + print '{'
> + print ' visit_start_struct(v, "%s", name, errp);' % (dtype)
> + for field in fields:
> + if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
> + continue
> +
> + if field['type'].endswith('_t'):
> + typename = field['type'][:-2]
> + else:
> + typename = field['type']
> +
> + if field.has_key('is_array'):
> + if field.has_key('array_capacity'):
> + print ' if (%(array_size)s> %(array_capacity)s) {' % field
> + print ' error_set(errp, QERR_FAULT, "Array size greater than capacity.");'
> + print ' }'
> + print ' %(array_size)s = MIN(%(array_size)s, %(array_capacity)s);' % field
> + print ' visit_start_array(v, "%s", errp);' % (field['variable'])
> + print ' for (size_t i = 0; i< %s; i++) {' % (field['array_size'])
> + print ' visit_type_%s(v,&s->%s[i], NULL, errp);' % (typename, field['variable'])
> + print ' }'
> + print ' visit_end_array(v, errp);'
> + elif field.has_key('default'):
> + if is_save:
> + print ' if (s->%s != %s) {' % (field['variable'], field['default'])
> + print ' visit_type_%s(v,&s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
> + print ' }'
> + else:
> + print ' s->%s = %s;' % (field['variable'], field['default'])
> + print ' visit_type_%s(v,&s->%s, "%s", NULL);' % (typename, field['variable'], field['variable'])
> + else:
> + print ' visit_type_%s(v,&s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
> + print ' visit_end_struct(v, errp);'
> + print '}'
> + print
> +
> +import json
> +
> +def type_dump(node):
> + print json.dumps(node, sort_keys=True, indent=4)
> +
> +def qapi_schema(node):
> + schema = OrderedDict()
> + data = OrderedDict()
> + fields = None
> + if node.has_key('typedef'):
> + schema['type'] = node['typedef']
> + fields = node['type']['fields']
> + elif node.has_key('struct'):
> + schema['type'] = node['struct']
> + fields = node['fields']
> + else:
> + raise Exception("top-level neither typedef nor struct")
> +
> + for field in fields:
> + if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
> + continue
> +
> + description = {}
> +
> + if field['type'].endswith('_t'):
> + typename = field['type'][:-2]
> + elif field['type'].startswith('struct '):
> + typename = field['type'].split(" ")[1]
> + else:
> + typename = field['type']
> +
> + if field.has_key('is_array') and field['is_array']:
> + description['type'] = [typename]
> + description['<annotated>'] = 'true'
> + if field.has_key('array_size'):
> + description['array_size'] = field['array_size']
> + if field.has_key('array_capacity'):
> + description['array_capacity'] = field['array_capacity']
> + else:
> + #description['type'] = typename
> + description = typename
> +
> + data[field['variable']] = description
> +
> + schema['data'] = data
> + print json.dumps(schema).replace("\"", "'")
> +
> +
> +if __name__ == '__main__':
Should be in a main() function.
Regards,
Anthony Liguori
> + la = LookAhead(skip(lexer(Input(sys.stdin))))
> +
> + index = 0
> + while True:
> + try:
> + if choice(la, index, 'typedef'):
> + offset, node = parse_typedef(la, index)
> + elif choice(la, index, 'struct'):
> + offset, node = parse_struct(la, index)
> + else:
> + continue
> +
> + index, _ = expect(la, index + offset, 'operator', ';')
> + marked = False
> + index = 0
> + except StopIteration, e:
> + break
> +
> + #qapi_format(node, True)
> + #qapi_format(node, False)
> + #type_dump(node)
> + qapi_schema(node)
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
2012-06-05 1:57 ` Anthony Liguori
@ 2012-06-05 9:25 ` Kevin Wolf
2012-06-05 9:47 ` Anthony Liguori
2012-06-05 16:21 ` Michael Roth
2012-06-05 10:00 ` Peter Maydell
2012-06-05 10:06 ` Avi Kivity
3 siblings, 2 replies; 81+ messages in thread
From: Kevin Wolf @ 2012-06-05 9:25 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
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.
> +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
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 9:25 ` Kevin Wolf
@ 2012-06-05 9:47 ` Anthony Liguori
2012-06-05 10:11 ` Kevin Wolf
2012-06-05 16:21 ` Michael Roth
1 sibling, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 9:47 UTC (permalink / raw)
To: Kevin Wolf
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
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
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 9:47 ` Anthony Liguori
@ 2012-06-05 10:11 ` Kevin Wolf
0 siblings, 0 replies; 81+ messages in thread
From: Kevin Wolf @ 2012-06-05 10:11 UTC (permalink / raw)
To: Anthony Liguori
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
Am 05.06.2012 11:47, schrieb Anthony Liguori:
> 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.
Can it parse actual source files, too? Many devices don't have a
separate header file today, and I'm not sure that they should have one.
>>> +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.
Assuming that we clear the values each time after a request has
completed, yes. Not completely sure if this covers all cases, but
possibly it does.
> 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 :-)
Seems I'm quite predictable: The time between reading the code line and
your comment was long enough to think "go away with your stupid 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.
Yes. I'm not surprised about that.
>>> +### 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.
Unless you accept breaking migration with older qemu versions,
subsections are a part of 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.
As long as the definition of the wire format (including subsections)
stays in the device code, I don't really mind how it's implemented. It
shouldn't be in an entirely different place, though.
I'm also unsure whether reviewing this part makes a lot of sense as long
as the compatibility part is missing. We'll continue to mess up in the
future, and so it's important to discuss how future versions can fix it
without breaking everything.
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 9:25 ` Kevin Wolf
2012-06-05 9:47 ` Anthony Liguori
@ 2012-06-05 16:21 ` Michael Roth
2012-06-05 19:56 ` Paolo Bonzini
2012-06-06 7:30 ` Kevin Wolf
1 sibling, 2 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 16:21 UTC (permalink / raw)
To: Kevin Wolf
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On Tue, Jun 05, 2012 at 11:25:01AM +0200, 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.
We could always make the compiler smarter (Anthony has already done
so apparently), and in the version posted here the compiler actually
scans line-by-line for the "qc_declaration" annotation before it begins
processing the annotated structure, so it behaves much like you're
suggestion.
The main reason device state needs to be moved to a seperate file is
because the generated visitors need to access that device state.
The only way I can think of getting around this is to do nasty things
like adding an
#include "qapi-generated/mc146818rtc-qapi-visit.c";
in hw/mc146818rtc.c.
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 16:21 ` Michael Roth
@ 2012-06-05 19:56 ` Paolo Bonzini
2012-06-05 23:40 ` Anthony Liguori
2012-06-06 7:30 ` Kevin Wolf
1 sibling, 1 reply; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-05 19:56 UTC (permalink / raw)
To: Michael Roth
Cc: Kevin Wolf, aliguori, owasserm, quintela, qemu-devel, yamahata,
akong, afaerber
Il 05/06/2012 18:21, Michael Roth ha scritto:
> The only way I can think of getting around this is to do nasty things
> like adding an
>
> #include "qapi-generated/mc146818rtc-qapi-visit.c";
>
> in hw/mc146818rtc.c.
It doesn't look that ugly, though perhaps I'm biased because that's
again exactly what GCC does.
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 19:56 ` Paolo Bonzini
@ 2012-06-05 23:40 ` Anthony Liguori
2012-06-06 5:12 ` Paolo Bonzini
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 23:40 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, yamahata, quintela, qemu-devel, Michael Roth,
owasserm, akong, afaerber
On 06/06/2012 03:56 AM, Paolo Bonzini wrote:
> Il 05/06/2012 18:21, Michael Roth ha scritto:
>> The only way I can think of getting around this is to do nasty things
>> like adding an
>>
>> #include "qapi-generated/mc146818rtc-qapi-visit.c";
>>
>> in hw/mc146818rtc.c.
>
> It doesn't look that ugly, though perhaps I'm biased because that's
> again exactly what GCC does.
A cleaner way to do this this to have mc146818rtc-qapi-visit.c have:
#include "mc146818rtc.c"
As the first line of the file. Then don't build mc146818rtc.c directly and
instead only build the qapi-visit.o variant.
This could all be done through make magic too.
Regards,
Anthony Liguori
>
> Paolo
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 23:40 ` Anthony Liguori
@ 2012-06-06 5:12 ` Paolo Bonzini
2012-06-06 5:43 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-06 5:12 UTC (permalink / raw)
To: Anthony Liguori
Cc: Kevin Wolf, owasserm, quintela, Michael Roth, qemu-devel,
yamahata, akong, afaerber
Il 06/06/2012 01:40, Anthony Liguori ha scritto:
>>
>>> The only way I can think of getting around this is to do nasty things
>>> like adding an
>>>
>>> #include "qapi-generated/mc146818rtc-qapi-visit.c";
>>>
>>> in hw/mc146818rtc.c.
>>
>> It doesn't look that ugly, though perhaps I'm biased because that's
>> again exactly what GCC does.
>
> A cleaner way to do this this to have mc146818rtc-qapi-visit.c have:
>
> #include "mc146818rtc.c"
>
> As the first line of the file. Then don't build mc146818rtc.c directly
> and instead only build the qapi-visit.o variant.
>
> This could all be done through make magic too.
Sounds like we have very different definitions of clean. :)
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 5:12 ` Paolo Bonzini
@ 2012-06-06 5:43 ` Anthony Liguori
0 siblings, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 5:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Kevin Wolf, owasserm, quintela, Michael Roth, qemu-devel,
yamahata, akong, afaerber
On 06/06/2012 01:12 PM, Paolo Bonzini wrote:
> Il 06/06/2012 01:40, Anthony Liguori ha scritto:
>>>
>>>> The only way I can think of getting around this is to do nasty things
>>>> like adding an
>>>>
>>>> #include "qapi-generated/mc146818rtc-qapi-visit.c";
>>>>
>>>> in hw/mc146818rtc.c.
>>>
>>> It doesn't look that ugly, though perhaps I'm biased because that's
>>> again exactly what GCC does.
>>
>> A cleaner way to do this this to have mc146818rtc-qapi-visit.c have:
>>
>> #include "mc146818rtc.c"
>>
>> As the first line of the file. Then don't build mc146818rtc.c directly
>> and instead only build the qapi-visit.o variant.
>>
>> This could all be done through make magic too.
>
> Sounds like we have very different definitions of clean. :)
Fair enough :-)
Either approach is fine by me FWIW.
Regards,
Anthony Liguori
>
> Paolo
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 16:21 ` Michael Roth
2012-06-05 19:56 ` Paolo Bonzini
@ 2012-06-06 7:30 ` Kevin Wolf
1 sibling, 0 replies; 81+ messages in thread
From: Kevin Wolf @ 2012-06-06 7:30 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
Am 05.06.2012 18:21, schrieb Michael Roth:
> On Tue, Jun 05, 2012 at 11:25:01AM +0200, 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.
>
> We could always make the compiler smarter (Anthony has already done
> so apparently), and in the version posted here the compiler actually
> scans line-by-line for the "qc_declaration" annotation before it begins
> processing the annotated structure, so it behaves much like you're
> suggestion.
>
> The main reason device state needs to be moved to a seperate file is
> because the generated visitors need to access that device state.
>
> The only way I can think of getting around this is to do nasty things
> like adding an
>
> #include "qapi-generated/mc146818rtc-qapi-visit.c";
>
> in hw/mc146818rtc.c.
If the alternative is making private structs public, which is even
nastier IMHO, this might be the lesser evil.
(And I agree with Paolo that Anthony's version is a bit too much magic
and doesn't really make it cleaner)
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
2012-06-05 1:57 ` Anthony Liguori
2012-06-05 9:25 ` Kevin Wolf
@ 2012-06-05 10:00 ` Peter Maydell
2012-06-05 10:10 ` Anthony Liguori
2012-06-11 7:13 ` Andreas Färber
2012-06-05 10:06 ` Avi Kivity
3 siblings, 2 replies; 81+ messages in thread
From: Peter Maydell @ 2012-06-05 10:00 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 5 June 2012 02:00, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> +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.
I don't think this is a fantastic idea -- the device struct should be
private to the device, and having it in a standalone header file is
asking for users of the device to illicitly include it and access
internals that they shouldn't. (This could be mitigated with some
kind of naming convention so it's easy to spot bogus includes in
code review, like "mydevice.int.h" or something.)
Whatever format we use, we should make sure it has the potential
to be extensible with markup for "this field is a qom property",
"this class foo is a subclass of class bar", "this class has
methods x,y,z" etc, so we can improve our autogeneration later to
cut out repetitive boilerplate like property array definitions,
initializers and wrapper functions.
(I wouldn't object to a not-actually-C syntax definition which
produced a header with the C struct as one of its outputs.)
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:00 ` Peter Maydell
@ 2012-06-05 10:10 ` Anthony Liguori
2012-06-11 7:13 ` Andreas Färber
1 sibling, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 10:10 UTC (permalink / raw)
To: Peter Maydell
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/05/2012 06:00 PM, Peter Maydell wrote:
> On 5 June 2012 02:00, Michael Roth<mdroth@linux.vnet.ibm.com> wrote:
>> +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.
>
> I don't think this is a fantastic idea -- the device struct should be
> private to the device, and having it in a standalone header file is
> asking for users of the device to illicitly include it and access
> internals that they shouldn't. (This could be mitigated with some
> kind of naming convention so it's easy to spot bogus includes in
> code review, like "mydevice.int.h" or something.)
Another way to solve this problem is having more directory hierarchy and using
public includes (include/qemu) verses private includes
(devices/pci/virtio/virtio_ring.h).
>
> Whatever format we use, we should make sure it has the potential
> to be extensible with markup for "this field is a qom property",
I've already added a '_property' marker which can be used to generate QOM static
property listings.
Regards,
Anthony Liguori
> "this class foo is a subclass of class bar", "this class has
> methods x,y,z" etc, so we can improve our autogeneration later to
> cut out repetitive boilerplate like property array definitions,
> initializers and wrapper functions.
>
> (I wouldn't object to a not-actually-C syntax definition which
> produced a header with the C struct as one of its outputs.)
>
> -- PMM
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:00 ` Peter Maydell
2012-06-05 10:10 ` Anthony Liguori
@ 2012-06-11 7:13 ` Andreas Färber
2012-06-11 7:20 ` Paolo Bonzini
2012-06-11 13:09 ` Peter Maydell
1 sibling, 2 replies; 81+ messages in thread
From: Andreas Färber @ 2012-06-11 7:13 UTC (permalink / raw)
To: Peter Maydell
Cc: aliguori, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, yamahata
Am 05.06.2012 12:00, schrieb Peter Maydell:
> On 5 June 2012 02:00, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> +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.
>
> I don't think this is a fantastic idea -- the device struct should be
> private to the device, and having it in a standalone header file is
> asking for users of the device to illicitly include it and access
> internals that they shouldn't.
But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
structs will need to be made public so that they can be embedded.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:13 ` Andreas Färber
@ 2012-06-11 7:20 ` Paolo Bonzini
2012-06-11 7:56 ` Andreas Färber
2012-06-11 13:12 ` Anthony Liguori
2012-06-11 13:09 ` Peter Maydell
1 sibling, 2 replies; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-11 7:20 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, aliguori, quintela, Michael Roth, qemu-devel,
owasserm, akong, yamahata
Il 11/06/2012 09:13, Andreas Färber ha scritto:
>>> >> +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.
>> >
>> > I don't think this is a fantastic idea -- the device struct should be
>> > private to the device, and having it in a standalone header file is
>> > asking for users of the device to illicitly include it and access
>> > internals that they shouldn't.
> But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
> structs will need to be made public so that they can be embedded.
I thought that was just a convenience choice, not a necessity. The
children objects could just as well be heap-allocated.
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:20 ` Paolo Bonzini
@ 2012-06-11 7:56 ` Andreas Färber
2012-06-11 7:59 ` Paolo Bonzini
2012-06-11 8:04 ` Andreas Färber
2012-06-11 13:12 ` Anthony Liguori
1 sibling, 2 replies; 81+ messages in thread
From: Andreas Färber @ 2012-06-11 7:56 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, aliguori, quintela, Michael Roth, qemu-devel,
owasserm, akong, yamahata
Am 11.06.2012 09:20, schrieb Paolo Bonzini:
> Il 11/06/2012 09:13, Andreas Färber ha scritto:
>>>>>> +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.
>>>>
>>>> I don't think this is a fantastic idea -- the device struct should be
>>>> private to the device, and having it in a standalone header file is
>>>> asking for users of the device to illicitly include it and access
>>>> internals that they shouldn't.
>> But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
>> structs will need to be made public so that they can be embedded.
>
> I thought that was just a convenience choice, not a necessity. The
> children objects could just as well be heap-allocated.
In that case we'd need to change the instance_init signature. As far as
I've understood from our discussions with Anthony, realize must not
allocate new objects because that may collide with the recursive realize
model, and instance_init is not supposed to fail. Thus the in-place init
demonstrated for i440fx and now adopted for prep_pci and my tegra2.
Saying "the device struct should be private to the device" leaves
virtually no choice, whether for convenience or necessity, and requires
also suggesting a design solution for QOM initialization.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:56 ` Andreas Färber
@ 2012-06-11 7:59 ` Paolo Bonzini
2012-06-11 9:02 ` Andreas Färber
2012-06-11 8:04 ` Andreas Färber
1 sibling, 1 reply; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-11 7:59 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, aliguori, quintela, Michael Roth, qemu-devel,
owasserm, akong, yamahata
Il 11/06/2012 09:56, Andreas Färber ha scritto:
>> > I thought that was just a convenience choice, not a necessity. The
>> > children objects could just as well be heap-allocated.
> In that case we'd need to change the instance_init signature. As far as
> I've understood from our discussions with Anthony, realize must not
> allocate new objects because that may collide with the recursive realize
> model, and instance_init is not supposed to fail.
But qemu_malloc is also not supposed to fail. It will exit QEMU if it
fails.
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:59 ` Paolo Bonzini
@ 2012-06-11 9:02 ` Andreas Färber
0 siblings, 0 replies; 81+ messages in thread
From: Andreas Färber @ 2012-06-11 9:02 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, aliguori, quintela, Jan Kiszka, Michael Roth,
qemu-devel, owasserm, Stefan Hajnoczi, akong, yamahata
Am 11.06.2012 09:59, schrieb Paolo Bonzini:
> Il 11/06/2012 09:56, Andreas Färber ha scritto:
>>>> I thought that was just a convenience choice, not a necessity. The
>>>> children objects could just as well be heap-allocated.
>> In that case we'd need to change the instance_init signature. As far as
>> I've understood from our discussions with Anthony, realize must not
>> allocate new objects because that may collide with the recursive realize
>> model, and instance_init is not supposed to fail.
>
> But qemu_malloc is also not supposed to fail. It will exit QEMU if it
> fails.
(s/qemu_malloc/g_malloc/g)
...which is not always desired.
void *objp = g_try_malloc(sizeof(MyState)); /* may fail without abort */
if (objp != NULL) {
object_initialize(objp, TYPE_MY); /* may not fail */
} else {
/* report error to caller */
}
This becomes more important the more objects we allocate at runtime in
response to users' QMP commands.
When we talk about QEMU startup then we are in violent agreement that it
can abort. When influenced by user-tunable options like -m then it
starts to become less clear, there seemed agreement that it shouldn't
abort but fail with a nice error message (seems not followed up with a
patch though?).
But as I've seen in v0.15.x bug reports and reported last fall for
v1.0-rcX, allocations can fail while the guest is running (even without
active user interaction: network? block? memory usage varied HIGHly
according to simpletrace analyzed) and also being far from the hard
physical limit depending on ulimit settings (e.g., 41G'ish on a 64 GB
RAM system with default soft virtual limit of 80%).
Examples I have in mind where user interaction shouldn't crash the guest
is hotplugging an x86 CPU with its APIC or hotplugging a ppc CPU with
its large'ish opcode table. About a 5-char g_strdup() I'm not as worried. ;)
So from my view, the solution is to have basic versions such as
qdev_create() abort and to also provide advanced versions like
qdev_try_create() to handle more sophisticated enterprise use cases. QOM
does not provide a double API in the same way qdev did, but the above
snippet shows that if proper QOM modelling is used - the topic of this
thread - it can be compensated with GLib. Either way it requires
knowledge about struct sizes so they need to be exposed somehow.
What I started playing with a few weeks ago is reporting OOM via Error;
that's a bit trickier since allocating a new object might fail as well,
so it needs to be preallocated and there was some reference-counting
iirc that didn't play nicely with a never-to-be-freed object. Use cases
might include adding properties - which is and must be done in
instance_init - and retrieving string properties.
Not the highest priority for me at the moment, but something we
shouldn't make impossible by Bad Design.
I have deriving a public BNC# for this on my TODO list, to preserve my
graphs and analysis tools...
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:56 ` Andreas Färber
2012-06-11 7:59 ` Paolo Bonzini
@ 2012-06-11 8:04 ` Andreas Färber
1 sibling, 0 replies; 81+ messages in thread
From: Andreas Färber @ 2012-06-11 8:04 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, aliguori, quintela, qemu-devel, Michael Roth,
owasserm, akong, yamahata
Am 11.06.2012 09:56, schrieb Andreas Färber:
> Am 11.06.2012 09:20, schrieb Paolo Bonzini:
>> Il 11/06/2012 09:13, Andreas Färber ha scritto:
>>>>>>> +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.
>>>>>
>>>>> I don't think this is a fantastic idea -- the device struct should be
>>>>> private to the device, and having it in a standalone header file is
>>>>> asking for users of the device to illicitly include it and access
>>>>> internals that they shouldn't.
>>> But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
>>> structs will need to be made public so that they can be embedded.
>>
>> I thought that was just a convenience choice, not a necessity. The
>> children objects could just as well be heap-allocated.
>
> In that case we'd need to change the instance_init signature. As far as
> I've understood from our discussions with Anthony, realize must not
> allocate new objects because that may collide with the recursive realize
> model, and instance_init is not supposed to fail. Thus the in-place init
> demonstrated for i440fx and now adopted for prep_pci and my tegra2.
> Saying "the device struct should be private to the device" leaves
> virtually no choice, whether for convenience or necessity, and requires
> also suggesting a design solution for QOM initialization.
Forgot to mention that this has nothing to do with child<> properties.
It's about composition, i.e. instantiating on object without needing to
know about its inner structure - paradoxically this needs that inner
structure to be exposed even if not accessed.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:20 ` Paolo Bonzini
2012-06-11 7:56 ` Andreas Färber
@ 2012-06-11 13:12 ` Anthony Liguori
2012-06-11 13:37 ` Peter Maydell
1 sibling, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-11 13:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Peter Maydell, quintela, qemu-devel, Michael Roth, owasserm,
akong, Andreas Färber, yamahata
On 06/11/2012 02:20 AM, Paolo Bonzini wrote:
> Il 11/06/2012 09:13, Andreas Färber ha scritto:
>>>>>> +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.
>>>>
>>>> I don't think this is a fantastic idea -- the device struct should be
>>>> private to the device, and having it in a standalone header file is
>>>> asking for users of the device to illicitly include it and access
>>>> internals that they shouldn't.
>> But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
>> structs will need to be made public so that they can be embedded.
>
> I thought that was just a convenience choice, not a necessity. The
> children objects could just as well be heap-allocated.
Objects need some sort of type signature publicly. The question is whether
that's a forward reference, a structure using PIMPL, or the full blown structure.
I really don't see the overwhelming need to keep structures private. Being able
to directly reference public members is extremely handy.
I'd rather we focus on modeling more devices in a proper fashion such that we
better understand their interactions and then make a decision about whether
devices ought to be fully private or not.
Regards,
Anthony Liguori
> Paolo
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 13:12 ` Anthony Liguori
@ 2012-06-11 13:37 ` Peter Maydell
0 siblings, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2012-06-11 13:37 UTC (permalink / raw)
To: Anthony Liguori
Cc: quintela, Michael Roth, qemu-devel, owasserm, Paolo Bonzini,
akong, Andreas Färber, yamahata
On 11 June 2012 14:12, Anthony Liguori <aliguori@us.ibm.com> wrote:
> I really don't see the overwhelming need to keep structures private. Being
> able to directly reference public members is extremely handy.
It keeps people honest. Last time I looked at your pin series it had
some changes which were using private info they shouldn't be, IIRC.
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-11 7:13 ` Andreas Färber
2012-06-11 7:20 ` Paolo Bonzini
@ 2012-06-11 13:09 ` Peter Maydell
1 sibling, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2012-06-11 13:09 UTC (permalink / raw)
To: Andreas Färber
Cc: aliguori, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, yamahata
On 11 June 2012 08:13, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.06.2012 12:00, schrieb Peter Maydell:
>> I don't think this is a fantastic idea -- the device struct should be
>> private to the device, and having it in a standalone header file is
>> asking for users of the device to illicitly include it and access
>> internals that they shouldn't.
>
> But that is exactly where realize is headed. PCIBus, a9mp_priv etc.
> structs will need to be made public so that they can be embedded.
I think that's a bad thing -- our object model shouldn't require all
that stuff to be publicly visible. Qdev/sysbus doesn't need it to
be publicly visible at the moment, and we shouldn't go backwards.
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
` (2 preceding siblings ...)
2012-06-05 10:00 ` Peter Maydell
@ 2012-06-05 10:06 ` Avi Kivity
2012-06-05 12:19 ` Gerd Hoffmann
` (3 more replies)
3 siblings, 4 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-05 10:06 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
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
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:06 ` Avi Kivity
@ 2012-06-05 12:19 ` Gerd Hoffmann
2012-06-05 23:41 ` Anthony Liguori
2012-06-06 7:19 ` Avi Kivity
2012-06-05 21:11 ` Michael Roth
` (2 subsequent siblings)
3 siblings, 2 replies; 81+ messages in thread
From: Gerd Hoffmann @ 2012-06-05 12:19 UTC (permalink / raw)
To: Avi Kivity
Cc: aliguori, owasserm, quintela, qemu-devel, Michael Roth, yamahata,
pbonzini, akong, afaerber
Hi,
> <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.
I wouldn't make the position mandatory. I expect the next generation
migration wire protocol transmits the field names anyway, so the
ordering will not matter any more, we need it for compatibility with
today's format only.
cheers,
Gerd
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 12:19 ` Gerd Hoffmann
@ 2012-06-05 23:41 ` Anthony Liguori
2012-06-06 7:19 ` Avi Kivity
1 sibling, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 23:41 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: yamahata, quintela, qemu-devel, Michael Roth, owasserm,
Avi Kivity, pbonzini, akong, afaerber
On 06/05/2012 08:19 PM, Gerd Hoffmann wrote:
> Hi,
>
>> <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.
>
> I wouldn't make the position mandatory. I expect the next generation
> migration wire protocol transmits the field names anyway, so the
> ordering will not matter any more, we need it for compatibility with
> today's format only.
+1
Regards,
Anthony Liguori
>
> cheers,
> Gerd
>
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 12:19 ` Gerd Hoffmann
2012-06-05 23:41 ` Anthony Liguori
@ 2012-06-06 7:19 ` Avi Kivity
1 sibling, 0 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 7:19 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: aliguori, owasserm, quintela, qemu-devel, Michael Roth, yamahata,
pbonzini, akong, afaerber
On 06/05/2012 03:19 PM, Gerd Hoffmann wrote:
> Hi,
>
>> <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.
>
> I wouldn't make the position mandatory. I expect the next generation
> migration wire protocol transmits the field names anyway, so the
> ordering will not matter any more, we need it for compatibility with
> today's format only.
We can remove the position hints when the old protocol is no longer
supported, but until then, it's IMO too easy to break compatibility by
moving lines around.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:06 ` Avi Kivity
2012-06-05 12:19 ` Gerd Hoffmann
@ 2012-06-05 21:11 ` Michael Roth
2012-06-06 7:31 ` Avi Kivity
2012-06-05 23:51 ` Anthony Liguori
2012-06-06 23:20 ` Anthony Liguori
3 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 21:11 UTC (permalink / raw)
To: Avi Kivity
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
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
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 21:11 ` Michael Roth
@ 2012-06-06 7:31 ` Avi Kivity
2012-06-06 21:36 ` Michael Roth
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 7:31 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 12:11 AM, Michael Roth wrote:
>>
>> 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.
We could make qc add this #include (or even inject the code directly) by
emitting a new C file (with #line directives to direct the debugger to
the original) and compiling this intermediate file instead of the source.
>> > +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.
Right, but within a qc_declaration struct there can be "other means" fields.
>>
>> <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...
I don't think the default is unsafe. I just dislike ABIs being cast
into stone by carelessness, it can be hard to fix up later.
Suppose we have state X and derived state Y that is sent by mistake.
But it can also be said that Y is the state and X derives from it, so
can we ever remove one or the other? It would be a bigger problem if
there were multiple implementations of the protocol (instead of just
qemu), but still, I'd rather see more thought going into the protcol
when defining it rather than when trying to change it.
>
>>
>> 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.
I'd like to see BER too. But we will have to support the old protocol
for quite some time (I'd say at least 3 years from the first release
that supports the new protocol).
We could put the ordering some other place, but that makes it harder to
write qc_declarations.
>> Surely there are available lexer/parser packages?
>
> This seems promising:
>
> http://pygments.org/docs/lexerdevelopment/
IMO some external tool is really needed. I'm sure qc will pick up new
features quickly, so separating the protocol description's description
from the protocol description's parser is important. You can't get a
lot more meta than that.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 7:31 ` Avi Kivity
@ 2012-06-06 21:36 ` Michael Roth
2012-06-07 7:08 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-06 21:36 UTC (permalink / raw)
To: Avi Kivity
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On Wed, Jun 06, 2012 at 10:31:56AM +0300, Avi Kivity wrote:
> On 06/06/2012 12:11 AM, Michael Roth wrote:
> >>
> >> 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.
>
> We could make qc add this #include (or even inject the code directly) by
> emitting a new C file (with #line directives to direct the debugger to
> the original) and compiling this intermediate file instead of the source.
>
> >> > +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.
>
> Right, but within a qc_declaration struct there can be "other means" fields.
>
> >>
> >> <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...
>
> I don't think the default is unsafe. I just dislike ABIs being cast
> into stone by carelessness, it can be hard to fix up later.
>
> Suppose we have state X and derived state Y that is sent by mistake.
> But it can also be said that Y is the state and X derives from it, so
> can we ever remove one or the other? It would be a bigger problem if
> there were multiple implementations of the protocol (instead of just
> qemu), but still, I'd rather see more thought going into the protcol
> when defining it rather than when trying to change it.
>
> >
> >>
> >> 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.
>
> I'd like to see BER too. But we will have to support the old protocol
> for quite some time (I'd say at least 3 years from the first release
> that supports the new protocol).
>
> We could put the ordering some other place, but that makes it harder to
> write qc_declarations.
>
> >> Surely there are available lexer/parser packages?
> >
> > This seems promising:
> >
> > http://pygments.org/docs/lexerdevelopment/
>
> IMO some external tool is really needed. I'm sure qc will pick up new
> features quickly, so separating the protocol description's description
> from the protocol description's parser is important. You can't get a
> lot more meta than that.
There's also pyparsing.
It would need to be something fairly ubiquitous though, and pygments is
the more well-used library yet still isn't available on RHEL5 and
probably a few other distros we need to support.
Although, we *could* just probe/disable schema/vmstate generation lacking
such dependencies, but that absolutely requires us to check in modified
schemas/vmstate (which we already do) and build based on those rather
than autobuilt code. Based on comments elsewhere I think that's the
direction we wanna go anyway though, so we have some options. I'll look
into it.
>
> --
> error compiling committee.c: too many arguments to function
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 21:36 ` Michael Roth
@ 2012-06-07 7:08 ` Avi Kivity
0 siblings, 0 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-07 7:08 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/07/2012 12:36 AM, Michael Roth wrote:
> It would need to be something fairly ubiquitous though, and pygments is
> the more well-used library yet still isn't available on RHEL5 and
> probably a few other distros we need to support.
If someone wants to build qemu from source on RHEL5, they can also build
whatever parser package we need from source. If there is sufficient
demand, we can package the parser for EPEL.
>
> Although, we *could* just probe/disable schema/vmstate generation lacking
> such dependencies, but that absolutely requires us to check in modified
> schemas/vmstate (which we already do) and build based on those rather
> than autobuilt code. Based on comments elsewhere I think that's the
> direction we wanna go anyway though, so we have some options. I'll look
> into it.
I think a hard dependency on the parser is fine, as long as it is
available on recent distributions.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:06 ` Avi Kivity
2012-06-05 12:19 ` Gerd Hoffmann
2012-06-05 21:11 ` Michael Roth
@ 2012-06-05 23:51 ` Anthony Liguori
2012-06-06 1:25 ` Peter Maydell
2012-06-06 7:45 ` Avi Kivity
2012-06-06 23:20 ` Anthony Liguori
3 siblings, 2 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 23:51 UTC (permalink / raw)
To: Avi Kivity
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/05/2012 06:06 PM, Avi Kivity wrote:
> On 06/05/2012 04:00 AM, Michael Roth wrote:
> 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.
No, this is just another example of C's type system sucking and being ambiguous.
Consider the following example:
struct PS2Keyboard {
DeviceState parent;
PCKBDState _immutable *controller; // link
...
};
This is definitely '_immutable' even though *something* has to marshal that
PCKBDState. That's because this is a reference to an externally managed object.
As long as references don't change due to guest initiated actions, they're
immutable.
In the case of a CharDriverState, the reference is always immutable. If we
supported changing char backends dynamically, it would not happen by changing
the reference, but almost certainly by having the ability to reopen the char
driver in place. IOW, while the referenced object changes, the reference
doesn't change.
Contrast that to:
struct PS2Keyboard {
DeviceState parent;
QEMUTimer *timer;
...
};
This could be just as well written as:
struct PS2Keyboard {
DeviceState parent;
QEMUTimer timer;
...
};
It's just a detail of our implementation that we allocate the timer on the heap.
So while it's still a pointer, it's not really a reference to an external
object. It's an embedded object and therefore must be considered for serialization.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 23:51 ` Anthony Liguori
@ 2012-06-06 1:25 ` Peter Maydell
2012-06-06 7:45 ` Avi Kivity
1 sibling, 0 replies; 81+ messages in thread
From: Peter Maydell @ 2012-06-06 1:25 UTC (permalink / raw)
To: Anthony Liguori
Cc: yamahata, quintela, qemu-devel, Michael Roth, owasserm,
Avi Kivity, pbonzini, akong, afaerber
On 6 June 2012 00:51, Anthony Liguori <aliguori@us.ibm.com> wrote:
> In the case of a CharDriverState, the reference is always immutable. If we
> supported changing char backends dynamically, it would not happen by
> changing the reference, but almost certainly by having the ability to reopen
> the char driver in place.
Incidentally, we may not support it but we do it anyway; see hw/omap_uart.c
omap_uart_attach()... (That's a nasty hack which it would be good to clean
up somehow.)
-- PMM
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 23:51 ` Anthony Liguori
2012-06-06 1:25 ` Peter Maydell
@ 2012-06-06 7:45 ` Avi Kivity
2012-06-06 8:27 ` Anthony Liguori
1 sibling, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 7:45 UTC (permalink / raw)
To: Anthony Liguori
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/06/2012 02:51 AM, Anthony Liguori wrote:
>>> +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.
>
> No, this is just another example of C's type system sucking and being
> ambiguous.
>
> Consider the following example:
>
> struct PS2Keyboard {
> DeviceState parent;
>
> PCKBDState _immutable *controller; // link
> ...
> };
>
> This is definitely '_immutable' even though *something* has to marshal
> that PCKBDState. That's because this is a reference to an externally
> managed object. As long as references don't change due to guest
> initiated actions, they're immutable.
In fact C allows you to express this:
const T *foo; // *foo may not change, but foo may
T * const foo; // foo may not change, but *foo may
Although every time I see this, I have to stop and think, which activity
I usually try to avoid. So your example would be written
PCKBDState * _immutable controller; // link
if we adopt the same system. But I wasn't referring to that at all.
Instead, some state is simply not relevant to the guest view, even
though it is state. We don't have a lot of that since most host state
is behind nice interfaces, but think of a vga device that keeps track of
the host window size and alt-ctrl status. Those are not guest state and
need not be migrated.
> In the case of a CharDriverState, the reference is always immutable. If
> we supported changing char backends dynamically, it would not happen by
> changing the reference, but almost certainly by having the ability to
> reopen the char driver in place. IOW, while the referenced object
> changes, the reference doesn't change.
In either case, state changes. The reason we don't send it over is
because it's not guest state, not because we reopen in place or not (and
migration shouldn't depend on how we choose to implement changing
chardev backends -- reopen or delete/new).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 7:45 ` Avi Kivity
@ 2012-06-06 8:27 ` Anthony Liguori
2012-06-06 8:37 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 8:27 UTC (permalink / raw)
To: Avi Kivity
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/06/2012 03:45 PM, Avi Kivity wrote:
> On 06/06/2012 02:51 AM, Anthony Liguori wrote:
>>>> +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.
>>
>> No, this is just another example of C's type system sucking and being
>> ambiguous.
>>
>> Consider the following example:
>>
>> struct PS2Keyboard {
>> DeviceState parent;
>>
>> PCKBDState _immutable *controller; // link
>> ...
>> };
>>
>> This is definitely '_immutable' even though *something* has to marshal
>> that PCKBDState. That's because this is a reference to an externally
>> managed object. As long as references don't change due to guest
>> initiated actions, they're immutable.
>
> In fact C allows you to express this:
>
> const T *foo; // *foo may not change, but foo may
> T * const foo; // foo may not change, but *foo may
>
> Although every time I see this, I have to stop and think, which activity
> I usually try to avoid. So your example would be written
>
> PCKBDState * _immutable controller; // link
>
> if we adopt the same system.
I think you meant to add a const in there, but yeah, I know.
> But I wasn't referring to that at all.
> Instead, some state is simply not relevant to the guest view, even
> though it is state. We don't have a lot of that since most host state
> is behind nice interfaces, but think of a vga device that keeps track of
> the host window size and alt-ctrl status. Those are not guest state and
> need not be migrated.
Both the host window size and alt-ctrl status are not device state. They are
host backend state and they ought to be stored in a different data structure.
This is exactly how we do it (both things are stored in sdl.c or vnc.c).
If we're storing mutable host state in a device structure, that's a bug IMHO.
That's sort of the premise of the QIDL annotations at least.
>> In the case of a CharDriverState, the reference is always immutable. If
>> we supported changing char backends dynamically, it would not happen by
>> changing the reference, but almost certainly by having the ability to
>> reopen the char driver in place. IOW, while the referenced object
>> changes, the reference doesn't change.
>
> In either case, state changes. The reason we don't send it over is
> because it's not guest state, not because we reopen in place or not (and
> migration shouldn't depend on how we choose to implement changing
> chardev backends -- reopen or delete/new).
Well the goal is here not to support any possible way of separating guest state
and host state. The goal is here is to be very opinionated about how we
separate guest and host state such that we can take a rigorous approach to
migrating guest state.
I think we're actually pretty good today about separating host and guest state.
I can't think of an example within devices where we don't cleanly separate
things outside of block devices.
Regards,
Anthony Liguori
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 8:27 ` Anthony Liguori
@ 2012-06-06 8:37 ` Avi Kivity
2012-06-06 8:45 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 8:37 UTC (permalink / raw)
To: Anthony Liguori
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/06/2012 11:27 AM, Anthony Liguori wrote:
> On 06/06/2012 03:45 PM, Avi Kivity wrote:
>> On 06/06/2012 02:51 AM, Anthony Liguori wrote:
>>>>> +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.
>>>
>>> No, this is just another example of C's type system sucking and being
>>> ambiguous.
>>>
>>> Consider the following example:
>>>
>>> struct PS2Keyboard {
>>> DeviceState parent;
>>>
>>> PCKBDState _immutable *controller; // link
>>> ...
>>> };
>>>
>>> This is definitely '_immutable' even though *something* has to marshal
>>> that PCKBDState. That's because this is a reference to an externally
>>> managed object. As long as references don't change due to guest
>>> initiated actions, they're immutable.
>>
>> In fact C allows you to express this:
>>
>> const T *foo; // *foo may not change, but foo may
>> T * const foo; // foo may not change, but *foo may
>>
>> Although every time I see this, I have to stop and think, which activity
>> I usually try to avoid. So your example would be written
>>
>> PCKBDState * _immutable controller; // link
>>
>> if we adopt the same system.
>
> I think you meant to add a const in there, but yeah, I know.
Maybe also a _stupid. How can one get a one line example wrong by
omitting the important word out of five?
>
>> But I wasn't referring to that at all.
>> Instead, some state is simply not relevant to the guest view, even
>> though it is state. We don't have a lot of that since most host state
>> is behind nice interfaces, but think of a vga device that keeps track of
>> the host window size and alt-ctrl status. Those are not guest state and
>> need not be migrated.
>
> Both the host window size and alt-ctrl status are not device state.
> They are host backend state and they ought to be stored in a different
> data structure. This is exactly how we do it (both things are stored in
> sdl.c or vnc.c).
>
> If we're storing mutable host state in a device structure, that's a bug
> IMHO. That's sort of the premise of the QIDL annotations at least.
Ok. But then the backend pointer is not 'const Backend * const', it's
'Backend * const' (if we don't allow retargeting). So we can't say it's
_immutable (and it isn't). It's _host.
>
>>> In the case of a CharDriverState, the reference is always immutable. If
>>> we supported changing char backends dynamically, it would not happen by
>>> changing the reference, but almost certainly by having the ability to
>>> reopen the char driver in place. IOW, while the referenced object
>>> changes, the reference doesn't change.
>>
>> In either case, state changes. The reason we don't send it over is
>> because it's not guest state, not because we reopen in place or not (and
>> migration shouldn't depend on how we choose to implement changing
>> chardev backends -- reopen or delete/new).
>
> Well the goal is here not to support any possible way of separating
> guest state and host state. The goal is here is to be very opinionated
> about how we separate guest and host state such that we can take a
> rigorous approach to migrating guest state.
>
> I think we're actually pretty good today about separating host and guest
> state. I can't think of an example within devices where we don't
> cleanly separate things outside of block devices.
And I agree that if we find such an example we should unexample it
quickly. But you still have a pointer (or even object) in there that is
not immutable. Why not mark it clearly instead of saying "well, the
important bits in it are immutable, we don't care about the rest"?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 8:37 ` Avi Kivity
@ 2012-06-06 8:45 ` Anthony Liguori
2012-06-06 8:59 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 8:45 UTC (permalink / raw)
To: Avi Kivity
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 04:37 PM, Avi Kivity wrote:
> On 06/06/2012 11:27 AM, Anthony Liguori wrote:
>> On 06/06/2012 03:45 PM, Avi Kivity wrote:
>>
>> I think you meant to add a const in there, but yeah, I know.
>
> Maybe also a _stupid. How can one get a one line example wrong by
> omitting the important word out of five?
For the same reason so few people actually write 'Type * const' in C, it's an
incredibly awkward syntax :-/
>>> But I wasn't referring to that at all.
>>> Instead, some state is simply not relevant to the guest view, even
>>> though it is state. We don't have a lot of that since most host state
>>> is behind nice interfaces, but think of a vga device that keeps track of
>>> the host window size and alt-ctrl status. Those are not guest state and
>>> need not be migrated.
>>
>> Both the host window size and alt-ctrl status are not device state.
>> They are host backend state and they ought to be stored in a different
>> data structure. This is exactly how we do it (both things are stored in
>> sdl.c or vnc.c).
>>
>> If we're storing mutable host state in a device structure, that's a bug
>> IMHO. That's sort of the premise of the QIDL annotations at least.
>
> Ok. But then the backend pointer is not 'const Backend * const', it's
> 'Backend * const' (if we don't allow retargeting). So we can't say it's
> _immutable (and it isn't). It's _host.
'Backend * const' is immutable
That is, the *pointer* is immutable. What it points to is not.
If you want to '#define _host _immutable' as a way to improve readability, I'm
not against that. But from a conceptional perspective, we don't migrate it
because nothing the guest does changes it. It's immutable from the guests PoV.
>>>> In the case of a CharDriverState, the reference is always immutable. If
>>>> we supported changing char backends dynamically, it would not happen by
>>>> changing the reference, but almost certainly by having the ability to
>>>> reopen the char driver in place. IOW, while the referenced object
>>>> changes, the reference doesn't change.
>>>
>>> In either case, state changes. The reason we don't send it over is
>>> because it's not guest state, not because we reopen in place or not (and
>>> migration shouldn't depend on how we choose to implement changing
>>> chardev backends -- reopen or delete/new).
>>
>> Well the goal is here not to support any possible way of separating
>> guest state and host state. The goal is here is to be very opinionated
>> about how we separate guest and host state such that we can take a
>> rigorous approach to migrating guest state.
>>
>> I think we're actually pretty good today about separating host and guest
>> state. I can't think of an example within devices where we don't
>> cleanly separate things outside of block devices.
>
> And I agree that if we find such an example we should unexample it
> quickly. But you still have a pointer (or even object) in there that is
> not immutable. Why not mark it clearly instead of saying "well, the
> important bits in it are immutable, we don't care about the rest"?
If your argument is, we can improve readability by further classifying immutable
things into why their immutable, I'm with you :-)
Regards,
Anthony Liguori
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 8:45 ` Anthony Liguori
@ 2012-06-06 8:59 ` Avi Kivity
2012-06-06 9:17 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 8:59 UTC (permalink / raw)
To: Anthony Liguori
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 11:45 AM, Anthony Liguori wrote:
>> Ok. But then the backend pointer is not 'const Backend * const', it's
>> 'Backend * const' (if we don't allow retargeting). So we can't say it's
>> _immutable (and it isn't). It's _host.
>
> 'Backend * const' is immutable
>
> That is, the *pointer* is immutable. What it points to is not.
So, is it reasonable to say
uint32_t * _immutable irrp; // Interrupt Request Register
and allocate it on the heap during initialization?
>
> If you want to '#define _host _immutable' as a way to improve
> readability, I'm not against that. But from a conceptional perspective,
> we don't migrate it because nothing the guest does changes it. It's
> immutable from the guests PoV.
It's not only immutable from the guest point of view, it's unreadable as
well. That's what _host means -- it doesn't exist as far as the guest
is concerned. IMO that's different than immutable.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 8:59 ` Avi Kivity
@ 2012-06-06 9:17 ` Anthony Liguori
2012-06-06 9:58 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 9:17 UTC (permalink / raw)
To: Avi Kivity
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 04:59 PM, Avi Kivity wrote:
> On 06/06/2012 11:45 AM, Anthony Liguori wrote:
>>> Ok. But then the backend pointer is not 'const Backend * const', it's
>>> 'Backend * const' (if we don't allow retargeting). So we can't say it's
>>> _immutable (and it isn't). It's _host.
>>
>> 'Backend * const' is immutable
>>
>> That is, the *pointer* is immutable. What it points to is not.
>
> So, is it reasonable to say
>
> uint32_t * _immutable irrp; // Interrupt Request Register
>
> and allocate it on the heap during initialization?
No, this would be wrong.
'_immutable' means that the *state* is immutable from the guests point of view.
The state stored by:
struct MyDevice {
Backend _immutable *backend;
}
Is the *reference* to the backend. The state of the backend is not part of the
device's state structure. Only the *reference* to the backend is part of the
device's state and that's immutable.
Contrast this to:
struct MyDevice {
uint32_t *irrp;
}
Where iirp is stored on the heap. While the state is on the heap, it is
effectively owned by the MyDevice structure. irrp is not a reference to another
object but just an indirect address of an extension of the device state.
If you think the syntax is confusing, then once you find a time machine, I'll
happily travel with you 40 years into the past and we can try to convince K&R to
introduce a richer pointer syntax that allows for differentiating between
various use-cases of pointers.
>> If you want to '#define _host _immutable' as a way to improve
>> readability, I'm not against that. But from a conceptional perspective,
>> we don't migrate it because nothing the guest does changes it. It's
>> immutable from the guests PoV.
>
> It's not only immutable from the guest point of view, it's unreadable as
> well. That's what _host means -- it doesn't exist as far as the guest
> is concerned. IMO that's different than immutable.
This isn't true. Block device backends are absolutely readable within the
guest. But for live migration, they don't change because we assume shared
storage. Therefore it's immutable :-)
Regards,
Anthony Liguori
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 9:17 ` Anthony Liguori
@ 2012-06-06 9:58 ` Avi Kivity
2012-06-06 11:12 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 9:58 UTC (permalink / raw)
To: Anthony Liguori
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 12:17 PM, Anthony Liguori wrote:
>>
>> So, is it reasonable to say
>>
>> uint32_t * _immutable irrp; // Interrupt Request Register
>>
>> and allocate it on the heap during initialization?
>
> No, this would be wrong.
>
> '_immutable' means that the *state* is immutable from the guests point
> of view. The state stored by:
>
> struct MyDevice {
> Backend _immutable *backend;
> }
>
> Is the *reference* to the backend. The state of the backend is not part
> of the device's state structure. Only the *reference* to the backend is
> part of the device's state and that's immutable.
I think this has degenerated into a disagreement about naming. Yet I
think this is important. I don't think _immutable suggests "immutable
from the guest's point of view" or even "we assume shared storage [1],
therefore it's immutable" to a device model author or reviewer. I think
we should choose the names under the assumption that the author did not
read the documentation (why bother when you can copy paste another
device model implementation) or read it and immediately forgot it. This
goes double for the reviewer(s). We need to make this as unsubtle as
possible (but no unsubtler).
> If you think the syntax is confusing, then once you find a time machine,
> I'll happily travel with you 40 years into the past and we can try to
> convince K&R to introduce a richer pointer syntax that allows for
> differentiating between various use-cases of pointers.
A go port of qemu would be interesting.
[1] we can also live migrate storage; the real reason it doesn't need
serialization is that it falls under the "by other means" category.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 9:58 ` Avi Kivity
@ 2012-06-06 11:12 ` Anthony Liguori
2012-06-06 11:25 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 11:12 UTC (permalink / raw)
To: Avi Kivity
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 05:58 PM, Avi Kivity wrote:
> On 06/06/2012 12:17 PM, Anthony Liguori wrote:
>>>
>>> So, is it reasonable to say
>>>
>>> uint32_t * _immutable irrp; // Interrupt Request Register
>>>
>>> and allocate it on the heap during initialization?
>>
>> No, this would be wrong.
>>
>> '_immutable' means that the *state* is immutable from the guests point
>> of view. The state stored by:
>>
>> struct MyDevice {
>> Backend _immutable *backend;
>> }
>>
>> Is the *reference* to the backend. The state of the backend is not part
>> of the device's state structure. Only the *reference* to the backend is
>> part of the device's state and that's immutable.
>
> I think this has degenerated into a disagreement about naming. Yet I
> think this is important. I don't think _immutable suggests "immutable
> from the guest's point of view" or even "we assume shared storage [1],
> therefore it's immutable" to a device model author or reviewer. I think
> we should choose the names under the assumption that the author did not
> read the documentation (why bother when you can copy paste another
> device model implementation) or read it and immediately forgot it. This
> goes double for the reviewer(s). We need to make this as unsubtle as
> possible (but no unsubtler).
Okay, we're talking past each other then.
I'm not really taking a position on the best naming convention to use for these
things. This is too early of a patch series. Whether we should have multiple
variants of '_immutable' that make it easier for reviewers is something that I'm
100% open too.
But I think it's important to have a strong conceptional model. So far, it's
built on the following:
All device state is serialized unless:
a) It's derived from other state
b) It's immutable (from the guest PoV)
c) We should migrate it but don't know and don't immediately want to change that
If we want to subdivide (b) into a set of more specific things, that's perfectly
fine by me. But I'm reluctant to just add a "(d) it's host state" because it
breaks my mental model.
>
>> If you think the syntax is confusing, then once you find a time machine,
>> I'll happily travel with you 40 years into the past and we can try to
>> convince K&R to introduce a richer pointer syntax that allows for
>> differentiating between various use-cases of pointers.
>
> A go port of qemu would be interesting.
Perhaps in 10 years. I think go is a little too immature right now. Submit
your talks now for KVM Forum 2022 ;-)
Regards,
Anthony Liguori
>
> [1] we can also live migrate storage; the real reason it doesn't need
> serialization is that it falls under the "by other means" category.
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-06 11:12 ` Anthony Liguori
@ 2012-06-06 11:25 ` Avi Kivity
0 siblings, 0 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 11:25 UTC (permalink / raw)
To: Anthony Liguori
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/06/2012 02:12 PM, Anthony Liguori wrote:
> On 06/06/2012 05:58 PM, Avi Kivity wrote:
>> On 06/06/2012 12:17 PM, Anthony Liguori wrote:
>>>>
>>>> So, is it reasonable to say
>>>>
>>>> uint32_t * _immutable irrp; // Interrupt Request Register
>>>>
>>>> and allocate it on the heap during initialization?
>>>
>>> No, this would be wrong.
>>>
>>> '_immutable' means that the *state* is immutable from the guests point
>>> of view. The state stored by:
>>>
>>> struct MyDevice {
>>> Backend _immutable *backend;
>>> }
>>>
>>> Is the *reference* to the backend. The state of the backend is not part
>>> of the device's state structure. Only the *reference* to the backend is
>>> part of the device's state and that's immutable.
>>
>> I think this has degenerated into a disagreement about naming. Yet I
>> think this is important. I don't think _immutable suggests "immutable
>> from the guest's point of view" or even "we assume shared storage [1],
>> therefore it's immutable" to a device model author or reviewer. I think
>> we should choose the names under the assumption that the author did not
>> read the documentation (why bother when you can copy paste another
>> device model implementation) or read it and immediately forgot it. This
>> goes double for the reviewer(s). We need to make this as unsubtle as
>> possible (but no unsubtler).
>
> Okay, we're talking past each other then.
>
> I'm not really taking a position on the best naming convention to use
> for these things. This is too early of a patch series. Whether we
> should have multiple variants of '_immutable' that make it easier for
> reviewers is something that I'm 100% open too.
>
> But I think it's important to have a strong conceptional model. So far,
> it's built on the following:
>
> All device state is serialized unless:
>
> a) It's derived from other state
>
> b) It's immutable (from the guest PoV)
I'm harping again on naming, but using _immutable to mean
_immutable_from_the_guest_point_of_view is confusing. _immutable means
_immutable. I don't think people will be able to answer "is this
immutable from a guest point of view" easily.
>
> c) We should migrate it but don't know and don't immediately want to
> change that
d) the RAM migration code takes care of migrating it
e) the block layer takes care of migrating it
> If we want to subdivide (b) into a set of more specific things, that's
> perfectly fine by me. But I'm reluctant to just add a "(d) it's host
> state" because it breaks my mental model.
Suppose you save/restore a guest that is connected to a host cdrom. The
cdrom tray state (and indeed the cdrom data) should not be
save/restored, because you want the real (host) data to be used after
restore. The same is true for a serial device that is connected to a
host serial device and reads line state from it.
>
>>
>>> If you think the syntax is confusing, then once you find a time machine,
>>> I'll happily travel with you 40 years into the past and we can try to
>>> convince K&R to introduce a richer pointer syntax that allows for
>>> differentiating between various use-cases of pointers.
>>
>> A go port of qemu would be interesting.
>
> Perhaps in 10 years. I think go is a little too immature right now.
> Submit your talks now for KVM Forum 2022 ;-)
In 10 years go would be old and crusty and I'd be drumming for the hot
new language of the day.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor
2012-06-05 10:06 ` Avi Kivity
` (2 preceding siblings ...)
2012-06-05 23:51 ` Anthony Liguori
@ 2012-06-06 23:20 ` Anthony Liguori
3 siblings, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-06 23:20 UTC (permalink / raw)
To: Avi Kivity
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
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?
>> +
>
> <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?
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
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 9:25 ` Kevin Wolf
` (2 more replies)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 03/17] qapi: add visitor interfaces for arrays Michael Roth
` (14 subsequent siblings)
16 siblings, 3 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Define away the annotations so we can still compile.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qapi/qc.h | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
create mode 100644 qapi/qc.h
diff --git a/qapi/qc.h b/qapi/qc.h
new file mode 100644
index 0000000..3b3a8b9
--- /dev/null
+++ b/qapi/qc.h
@@ -0,0 +1,11 @@
+#ifndef QC_H
+#define QC_H
+
+#define qc_declaration
+#define _immutable
+#define _derived
+#define _broken
+#define _version(x)
+#define _size_is(x)
+
+#endif
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 1:00 ` [Qemu-devel] [PATCH 02/17] qidl: add qc definitions Michael Roth
@ 2012-06-05 9:25 ` Kevin Wolf
2012-06-05 10:35 ` Jan Kiszka
2012-06-05 14:08 ` Paolo Bonzini
2 siblings, 0 replies; 81+ messages in thread
From: Kevin Wolf @ 2012-06-05 9:25 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
Am 05.06.2012 03:00, schrieb Michael Roth:
> Define away the annotations so we can still compile.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qapi/qc.h | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
> create mode 100644 qapi/qc.h
>
> diff --git a/qapi/qc.h b/qapi/qc.h
> new file mode 100644
> index 0000000..3b3a8b9
> --- /dev/null
> +++ b/qapi/qc.h
> @@ -0,0 +1,11 @@
> +#ifndef QC_H
> +#define QC_H
> +
> +#define qc_declaration
> +#define _immutable
> +#define _derived
> +#define _broken
> +#define _version(x)
> +#define _size_is(x)
> +
> +#endif
I believe there are still ways to improve the documentation here. :-)
At the very least there should be a reference to the documentation of
patch 1.
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 1:00 ` [Qemu-devel] [PATCH 02/17] qidl: add qc definitions Michael Roth
2012-06-05 9:25 ` Kevin Wolf
@ 2012-06-05 10:35 ` Jan Kiszka
2012-06-05 11:12 ` Anthony Liguori
2012-06-05 14:08 ` Paolo Bonzini
2 siblings, 1 reply; 81+ messages in thread
From: Jan Kiszka @ 2012-06-05 10:35 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 2012-06-05 03:00, Michael Roth wrote:
> Define away the annotations so we can still compile.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qapi/qc.h | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
> create mode 100644 qapi/qc.h
>
> diff --git a/qapi/qc.h b/qapi/qc.h
> new file mode 100644
> index 0000000..3b3a8b9
> --- /dev/null
> +++ b/qapi/qc.h
> @@ -0,0 +1,11 @@
> +#ifndef QC_H
> +#define QC_H
> +
> +#define qc_declaration
> +#define _immutable
> +#define _derived
> +#define _broken
> +#define _version(x)
> +#define _size_is(x)
> +
> +#endif
These tags require a different prefix than the reserved '_'.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 10:35 ` Jan Kiszka
@ 2012-06-05 11:12 ` Anthony Liguori
2012-06-05 11:26 ` Jan Kiszka
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 11:12 UTC (permalink / raw)
To: Jan Kiszka
Cc: yamahata, quintela, Michael Roth, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/05/2012 06:35 PM, Jan Kiszka wrote:
> On 2012-06-05 03:00, Michael Roth wrote:
>> Define away the annotations so we can still compile.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>> qapi/qc.h | 11 +++++++++++
>> 1 files changed, 11 insertions(+), 0 deletions(-)
>> create mode 100644 qapi/qc.h
>>
>> diff --git a/qapi/qc.h b/qapi/qc.h
>> new file mode 100644
>> index 0000000..3b3a8b9
>> --- /dev/null
>> +++ b/qapi/qc.h
>> @@ -0,0 +1,11 @@
>> +#ifndef QC_H
>> +#define QC_H
>> +
>> +#define qc_declaration
>> +#define _immutable
>> +#define _derived
>> +#define _broken
>> +#define _version(x)
>> +#define _size_is(x)
>> +
>> +#endif
>
> These tags require a different prefix than the reserved '_'.
The rationale is that QIDL is part of the "compiler/library implementation" that
this namespace is reserved for.
Regards,
Anthony Liguori
>
> Jan
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 11:12 ` Anthony Liguori
@ 2012-06-05 11:26 ` Jan Kiszka
2012-06-05 11:42 ` Kevin Wolf
0 siblings, 1 reply; 81+ messages in thread
From: Jan Kiszka @ 2012-06-05 11:26 UTC (permalink / raw)
To: Anthony Liguori
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 2012-06-05 13:12, Anthony Liguori wrote:
> On 06/05/2012 06:35 PM, Jan Kiszka wrote:
>> On 2012-06-05 03:00, Michael Roth wrote:
>>> Define away the annotations so we can still compile.
>>>
>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>> ---
>>> qapi/qc.h | 11 +++++++++++
>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>> create mode 100644 qapi/qc.h
>>>
>>> diff --git a/qapi/qc.h b/qapi/qc.h
>>> new file mode 100644
>>> index 0000000..3b3a8b9
>>> --- /dev/null
>>> +++ b/qapi/qc.h
>>> @@ -0,0 +1,11 @@
>>> +#ifndef QC_H
>>> +#define QC_H
>>> +
>>> +#define qc_declaration
>>> +#define _immutable
>>> +#define _derived
>>> +#define _broken
>>> +#define _version(x)
>>> +#define _size_is(x)
>>> +
>>> +#endif
>>
>> These tags require a different prefix than the reserved '_'.
>
> The rationale is that QIDL is part of the "compiler/library implementation" that
> this namespace is reserved for.
It's a QEMU-internal thing, and we want to be portable. Not sure if it's
worth to risk collisions in some distant corner.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 11:26 ` Jan Kiszka
@ 2012-06-05 11:42 ` Kevin Wolf
0 siblings, 0 replies; 81+ messages in thread
From: Kevin Wolf @ 2012-06-05 11:42 UTC (permalink / raw)
To: Jan Kiszka
Cc: Anthony Liguori, owasserm@redhat.com, quintela@redhat.com,
qemu-devel@nongnu.org, Michael Roth, yamahata@valinux.co.jp,
pbonzini@redhat.com, akong@redhat.com, afaerber@suse.de
Am 05.06.2012 13:26, schrieb Jan Kiszka:
> On 2012-06-05 13:12, Anthony Liguori wrote:
>> On 06/05/2012 06:35 PM, Jan Kiszka wrote:
>>> On 2012-06-05 03:00, Michael Roth wrote:
>>>> Define away the annotations so we can still compile.
>>>>
>>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>>> ---
>>>> qapi/qc.h | 11 +++++++++++
>>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>>> create mode 100644 qapi/qc.h
>>>>
>>>> diff --git a/qapi/qc.h b/qapi/qc.h
>>>> new file mode 100644
>>>> index 0000000..3b3a8b9
>>>> --- /dev/null
>>>> +++ b/qapi/qc.h
>>>> @@ -0,0 +1,11 @@
>>>> +#ifndef QC_H
>>>> +#define QC_H
>>>> +
>>>> +#define qc_declaration
>>>> +#define _immutable
>>>> +#define _derived
>>>> +#define _broken
>>>> +#define _version(x)
>>>> +#define _size_is(x)
>>>> +
>>>> +#endif
>>>
>>> These tags require a different prefix than the reserved '_'.
>>
>> The rationale is that QIDL is part of the "compiler/library implementation" that
>> this namespace is reserved for.
>
> It's a QEMU-internal thing, and we want to be portable. Not sure if it's
> worth to risk collisions in some distant corner.
I agree, this isn't really convincing. If QIDL was part of the system
environment, it wouldn't be in the qemu source tree. After all these
rules aren't there just for fun but in order to avoid naming conflicts,
and conflicts between qemu and qemu are much less likely than between
some exotic libc and qemu.
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 1:00 ` [Qemu-devel] [PATCH 02/17] qidl: add qc definitions Michael Roth
2012-06-05 9:25 ` Kevin Wolf
2012-06-05 10:35 ` Jan Kiszka
@ 2012-06-05 14:08 ` Paolo Bonzini
2012-06-05 21:44 ` Michael Roth
2 siblings, 1 reply; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-05 14:08 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, akong,
afaerber
Il 05/06/2012 03:00, Michael Roth ha scritto:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qapi/qc.h | 11 +++++++++++
> 1 files changed, 11 insertions(+), 0 deletions(-)
> create mode 100644 qapi/qc.h
>
> diff --git a/qapi/qc.h b/qapi/qc.h
> new file mode 100644
> index 0000000..3b3a8b9
> --- /dev/null
> +++ b/qapi/qc.h
> @@ -0,0 +1,11 @@
> +#ifndef QC_H
> +#define QC_H
> +
> +#define qc_declaration
> +#define _immutable
> +#define _derived
> +#define _broken
> +#define _version(x)
> +#define _size_is(x)
Would it be feasible to make the declaration look like the GCC attribute
extension, e.g.
struct RTCState QIDL() {
int foo QIDL(immutable);
int bar QIDL(derived);
};
so that you can just use "#define QIDL(...)"? This is how GCC
developers did their introspection annotations.
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 14:08 ` Paolo Bonzini
@ 2012-06-05 21:44 ` Michael Roth
2012-06-05 23:35 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 21:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: aliguori, owasserm, quintela, qemu-devel, yamahata, akong,
afaerber
On Tue, Jun 05, 2012 at 04:08:55PM +0200, Paolo Bonzini wrote:
> Il 05/06/2012 03:00, Michael Roth ha scritto:
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > qapi/qc.h | 11 +++++++++++
> > 1 files changed, 11 insertions(+), 0 deletions(-)
> > create mode 100644 qapi/qc.h
> >
> > diff --git a/qapi/qc.h b/qapi/qc.h
> > new file mode 100644
> > index 0000000..3b3a8b9
> > --- /dev/null
> > +++ b/qapi/qc.h
> > @@ -0,0 +1,11 @@
> > +#ifndef QC_H
> > +#define QC_H
> > +
> > +#define qc_declaration
> > +#define _immutable
> > +#define _derived
> > +#define _broken
> > +#define _version(x)
> > +#define _size_is(x)
>
> Would it be feasible to make the declaration look like the GCC attribute
> extension, e.g.
>
> struct RTCState QIDL() {
> int foo QIDL(immutable);
> int bar QIDL(derived);
> };
>
> so that you can just use "#define QIDL(...)"? This is how GCC
> developers did their introspection annotations.
This does seem a lot cleaner to me, also simplifies the lexer and we get
namespacing for free.
>
> Paolo
>
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 02/17] qidl: add qc definitions
2012-06-05 21:44 ` Michael Roth
@ 2012-06-05 23:35 ` Anthony Liguori
0 siblings, 0 replies; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 23:35 UTC (permalink / raw)
To: Michael Roth
Cc: quintela, owasserm, qemu-devel, yamahata, Paolo Bonzini, akong,
afaerber
On 06/06/2012 05:44 AM, Michael Roth wrote:
> On Tue, Jun 05, 2012 at 04:08:55PM +0200, Paolo Bonzini wrote:
>> Il 05/06/2012 03:00, Michael Roth ha scritto:
>>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>>> ---
>>> qapi/qc.h | 11 +++++++++++
>>> 1 files changed, 11 insertions(+), 0 deletions(-)
>>> create mode 100644 qapi/qc.h
>>>
>>> diff --git a/qapi/qc.h b/qapi/qc.h
>>> new file mode 100644
>>> index 0000000..3b3a8b9
>>> --- /dev/null
>>> +++ b/qapi/qc.h
>>> @@ -0,0 +1,11 @@
>>> +#ifndef QC_H
>>> +#define QC_H
>>> +
>>> +#define qc_declaration
>>> +#define _immutable
>>> +#define _derived
>>> +#define _broken
>>> +#define _version(x)
>>> +#define _size_is(x)
>>
>> Would it be feasible to make the declaration look like the GCC attribute
>> extension, e.g.
>>
>> struct RTCState QIDL() {
>> int foo QIDL(immutable);
>> int bar QIDL(derived);
>> };
>>
>> so that you can just use "#define QIDL(...)"? This is how GCC
>> developers did their introspection annotations.
>
> This does seem a lot cleaner to me, also simplifies the lexer and we get
> namespacing for free.
Makes sense to me.
Regards,
Anthony Liguori
>
>>
>> Paolo
>>
>
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 03/17] qapi: add visitor interfaces for arrays
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 02/17] qidl: add qc definitions Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 04/17] qapi: QmpOutputVisitor, implement array handling Michael Roth
` (13 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qapi/qapi-visit-core.c | 25 +++++++++++++++++++++++++
qapi/qapi-visit-core.h | 8 ++++++++
scripts/qapi-visit.py | 26 ++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 9a29674..482bab6 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -306,3 +306,28 @@ void input_type_enum(Visitor *v, int *obj, const char *strings[],
g_free(enum_str);
*obj = value;
}
+
+void visit_start_array(Visitor *v, void **obj, const char *name,
+ size_t elem_count, size_t elem_size, Error **errp)
+{
+ g_assert(v->start_array);
+ if (!error_is_set(errp)) {
+ v->start_array(v, obj, name, elem_count, elem_size, errp);
+ }
+}
+
+void visit_next_array(Visitor *v, Error **errp)
+{
+ g_assert(v->next_array);
+ if (!error_is_set(errp)) {
+ v->next_array(v, errp);
+ }
+}
+
+void visit_end_array(Visitor *v, Error **errp)
+{
+ g_assert(v->end_array);
+ if (!error_is_set(errp)) {
+ v->end_array(v, errp);
+ }
+}
diff --git a/qapi/qapi-visit-core.h b/qapi/qapi-visit-core.h
index a19d70c..19de382 100644
--- a/qapi/qapi-visit-core.h
+++ b/qapi/qapi-visit-core.h
@@ -43,6 +43,10 @@ struct Visitor
void (*type_str)(Visitor *v, char **obj, const char *name, Error **errp);
void (*type_number)(Visitor *v, double *obj, const char *name,
Error **errp);
+ void (*start_array)(Visitor *v, void **obj, const char *name,
+ size_t elem_count, size_t elem_size, Error **errp);
+ void (*next_array)(Visitor *v, Error **errp);
+ void (*end_array)(Visitor *v, Error **errp);
/* May be NULL */
void (*start_optional)(Visitor *v, bool *present, const char *name,
@@ -88,5 +92,9 @@ void visit_type_int64(Visitor *v, int64_t *obj, const char *name, Error **errp);
void visit_type_bool(Visitor *v, bool *obj, const char *name, Error **errp);
void visit_type_str(Visitor *v, char **obj, const char *name, Error **errp);
void visit_type_number(Visitor *v, double *obj, const char *name, Error **errp);
+void visit_start_array(Visitor *v, void **obj, const char *name,
+ size_t elem_count, size_t elem_size, Error **errp);
+void visit_next_array(Visitor *v, Error **errp);
+void visit_end_array(Visitor *v, Error **errp);
#endif
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 8d4e94a..3a03537 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -17,6 +17,32 @@ import os
import getopt
import errno
+def generate_visit_array_body(name, info):
+ if info.has_key('array_capacity'):
+ array_capacity = info['array_capacity']
+ else:
+ array_capacity = info['array_size']
+
+ if info['array_size'].isdigit():
+ array_size = info['array_size']
+ else:
+ array_size = "(*obj)->%s" % info['array_size']
+
+ ret = mcgen('''
+visit_start_array(m, (void **)obj, "%(name)s", %(array_capacity)s, sizeof(%(type)s), errp);
+int %(name)s_i;
+for (%(name)s_i = 0; %(name)s_i < %(array_size)s; %(name)s_i++) {
+ visit_type_%(type_short)s(m, &(*obj)->%(name)s[%(name)s_i], NULL, errp);
+ visit_next_array(m, errp);
+}
+visit_end_array(m, errp);
+''',
+ name=name, type=c_type(info['type'][0]),
+ type_short=info['type'][0],
+ array_size=array_size,
+ array_capacity=array_capacity)
+ return ret
+
def generate_visit_struct_body(field_prefix, members):
ret = ""
if len(field_prefix):
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 04/17] qapi: QmpOutputVisitor, implement array handling
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (2 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 03/17] qapi: add visitor interfaces for arrays Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 05/17] qapi: qapi-visit.py, support arrays and complex qapi definitions Michael Roth
` (12 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qapi/qmp-output-visitor.c | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 2bce9d5..78e8ff4 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -181,6 +181,24 @@ static void qmp_output_type_number(Visitor *v, double *obj, const char *name,
qmp_output_add(qov, name, qfloat_from_double(*obj));
}
+static void qmp_output_start_array(Visitor *v, void **obj, const char *name,
+ size_t elem_count, size_t elem_size,
+ Error **errp)
+{
+ qmp_output_start_list(v, name, errp);
+}
+
+
+static void qmp_output_next_array(Visitor *v, Error **errp)
+{
+}
+
+static void qmp_output_end_array(Visitor *v, Error **errp)
+{
+ QmpOutputVisitor *qov = to_qov(v);
+ qmp_output_pop(qov);
+}
+
QObject *qmp_output_get_qobject(QmpOutputVisitor *qov)
{
QObject *obj = qmp_output_first(qov);
@@ -228,6 +246,9 @@ QmpOutputVisitor *qmp_output_visitor_new(void)
v->visitor.type_bool = qmp_output_type_bool;
v->visitor.type_str = qmp_output_type_str;
v->visitor.type_number = qmp_output_type_number;
+ v->visitor.start_array = qmp_output_start_array;
+ v->visitor.next_array = qmp_output_next_array;
+ v->visitor.end_array = qmp_output_end_array;
QTAILQ_INIT(&v->stack);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 05/17] qapi: qapi-visit.py, support arrays and complex qapi definitions
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (3 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 04/17] qapi: QmpOutputVisitor, implement array handling Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 06/17] qapi: qapi-visit.py, add gen support for existing types Michael Roth
` (11 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Add support for arrays in the code generators.
Complex field descriptions can now be used to provide additional
information to the visitor generators, such as the max size of an array,
or the field within a struct to use to determine how many elements are
present in the array to avoid serializing uninitialized elements.
Add handling for these in the code generators as well.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qapi-commands.py | 8 ++++----
scripts/qapi-types.py | 2 +-
scripts/qapi-visit.py | 30 +++++++++++++++++++++++++-----
scripts/qapi.py | 10 ++++++++--
4 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 9eed40e..52221d6 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -32,7 +32,7 @@ void %(visitor)s(Visitor *m, %(name)s * obj, const char *name, Error **errp);
def generate_command_decl(name, args, ret_type):
arglist=""
- for argname, argtype, optional, structured in parse_args(args):
+ for argname, argtype, optional, structured, annotated in parse_args(args):
argtype = c_type(argtype)
if argtype == "char *":
argtype = "const char *"
@@ -50,7 +50,7 @@ def gen_sync_call(name, args, ret_type, indent=0):
retval=""
if ret_type:
retval = "retval = "
- for argname, argtype, optional, structured in parse_args(args):
+ for argname, argtype, optional, structured, annotated in parse_args(args):
if optional:
arglist += "has_%s, " % c_var(argname)
arglist += "%s, " % (c_var(argname))
@@ -106,7 +106,7 @@ Visitor *v;
def gen_visitor_input_vars_decl(args):
ret = ""
push_indent()
- for argname, argtype, optional, structured in parse_args(args):
+ for argname, argtype, optional, structured, annotated in parse_args(args):
if optional:
ret += mcgen('''
bool has_%(argname)s = false;
@@ -145,7 +145,7 @@ v = qmp_input_get_visitor(mi);
''',
obj=obj)
- for argname, argtype, optional, structured in parse_args(args):
+ for argname, argtype, optional, structured, annotated in parse_args(args):
if optional:
ret += mcgen('''
visit_start_optional(v, &has_%(c_name)s, "%(name)s", errp);
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 4a734f5..fb2b9ea 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -35,7 +35,7 @@ struct %(name)s
''',
name=structname)
- for argname, argentry, optional, structured in parse_args(members):
+ for argname, argentry, optional, structured, annotated in parse_args(members):
if optional:
ret += mcgen('''
bool has_%(c_name)s;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3a03537..6562226 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -16,6 +16,22 @@ import sys
import os
import getopt
import errno
+import types
+
+def generate_visit_array_body(name, info):
+ ret = mcgen('''
+visit_start_array(m, (void **)obj, "%(name)s", %(count)s, sizeof(%(type)s), errp);
+int %(name)s_i;
+for (%(name)s_i = 0; %(name)s_i < %(count)s; %(name)s_i++) {
+ visit_type_%(type_short)s(m, &(*obj)->%(name)s[%(name)s_i], NULL, errp);
+ visit_next_array(m, errp);
+}
+visit_end_array(m, errp);
+''',
+ name=name, type=c_type(info['type'][0]),
+ type_short=info['type'][0],
+ count=info['array_size'])
+ return ret
def generate_visit_array_body(name, info):
if info.has_key('array_capacity'):
@@ -47,7 +63,7 @@ def generate_visit_struct_body(field_prefix, members):
ret = ""
if len(field_prefix):
field_prefix = field_prefix + "."
- for argname, argentry, optional, structured in parse_args(members):
+ for argname, argentry, optional, structured, annotated in parse_args(members):
if optional:
ret += mcgen('''
visit_start_optional(m, (obj && *obj) ? &(*obj)->%(c_prefix)shas_%(c_name)s : NULL, "%(name)s", errp);
@@ -67,12 +83,16 @@ visit_start_struct(m, NULL, "", "%(name)s", 0, errp);
visit_end_struct(m, errp);
''')
else:
- ret += mcgen('''
+ if annotated:
+ if isinstance(argentry['type'], types.ListType):
+ ret += generate_visit_array_body(argname, argentry)
+ else:
+ ret += mcgen('''
visit_type_%(type)s(m, (obj && *obj) ? &(*obj)->%(c_prefix)s%(c_name)s : NULL, "%(name)s", errp);
''',
- c_prefix=c_var(field_prefix), prefix=field_prefix,
- type=type_name(argentry), c_name=c_var(argname),
- name=argname)
+ c_prefix=c_var(field_prefix), prefix=field_prefix,
+ type=type_name(argentry), c_name=c_var(argname),
+ name=argname)
if optional:
pop_indent()
diff --git a/scripts/qapi.py b/scripts/qapi.py
index e062336..e8bd230 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -99,12 +99,16 @@ def parse_args(typeinfo):
argentry = typeinfo[member]
optional = False
structured = False
+ annotated = False
if member.startswith('*'):
argname = member[1:]
optional = True
if isinstance(argentry, OrderedDict):
- structured = True
- yield (argname, argentry, optional, structured)
+ if argentry.has_key('<annotated>'):
+ annotated = True
+ else:
+ structured = True
+ yield (argname, argentry, optional, structured, annotated)
def de_camel_case(name):
new_name = ''
@@ -171,6 +175,8 @@ def c_type(name):
return 'void'
elif name == name.upper():
return '%sEvent *' % camel_case(name)
+ elif name.replace("u", "").replace("int", "") in ["8", "16", "32", "64"]:
+ return name + "_t"
else:
return '%s *' % name
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 06/17] qapi: qapi-visit.py, add gen support for existing types
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (4 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 05/17] qapi: qapi-visit.py, support arrays and complex qapi definitions Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 07/17] qapi: add open-coded visitors for QEMUTimer/struct tm types Michael Roth
` (10 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
For qidl-annotated devices, we generate visitors for existing types,
rather than defined the types via qapi. Modify qapi-visit.py so that we
can pass it a header containing the type definition rather that having
it expect a generated one.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qapi-visit.py | 34 +++++++++++++++++++++++++++++-----
1 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 6562226..a319e66 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -229,8 +229,10 @@ void visit_type_%(name)s(Visitor *m, %(name)s * obj, const char *name, Error **e
name=name)
try:
- opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
- ["source", "header", "prefix=", "output-dir="])
+ opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:ei:",
+ ["source", "header", "prefix=",
+ "output-dir=", "existing-types",
+ "include="])
except getopt.GetoptError, err:
print str(err)
sys.exit(1)
@@ -239,9 +241,11 @@ output_dir = ""
prefix = ""
c_file = 'qapi-visit.c'
h_file = 'qapi-visit.h'
+includes = []
do_c = False
do_h = False
+existing_types = False
for o, a in opts:
if o in ("-p", "--prefix"):
@@ -252,6 +256,10 @@ for o, a in opts:
do_c = True
elif o in ("-h", "--header"):
do_h = True
+ elif o in ("-e", "--existing-types"):
+ existing_types = True
+ elif o in ("-i", "--include"):
+ includes.append(a)
if not do_c and not do_h:
do_c = True
@@ -316,19 +324,35 @@ fdecl.write(mcgen('''
#define %(guard)s
#include "qapi/qapi-visit-core.h"
-#include "%(prefix)sqapi-types.h"
''',
prefix=prefix, guard=guardname(h_file)))
+if not existing_types:
+ fdecl.write(mcgen('''
+#include "%(prefix)sqapi-types.h"
+''',
+ prefix=prefix))
+
+for include in includes:
+ fdecl.write(mcgen('''
+#include "%(include)s"
+''',
+ include=include))
+
+
exprs = parse_schema(sys.stdin)
for expr in exprs:
if expr.has_key('type'):
ret = generate_visit_struct(expr['type'], expr['data'])
- ret += generate_visit_list(expr['type'], expr['data'])
+ if not existing_types:
+ ret += generate_visit_list(expr['type'], expr['data'])
fdef.write(ret)
- ret = generate_declaration(expr['type'], expr['data'])
+ if existing_types:
+ ret = generate_declaration(expr['type'], expr['data'], False)
+ else:
+ ret = generate_declaration(expr['type'], expr['data'], True)
fdecl.write(ret)
elif expr.has_key('union'):
ret = generate_visit_union(expr['union'], expr['data'])
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 07/17] qapi: add open-coded visitors for QEMUTimer/struct tm types
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (5 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 06/17] qapi: qapi-visit.py, add gen support for existing types Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 08/17] rtc: move RTCState declaration to header Michael Roth
` (9 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
These are common types that don't lend themselves to script-generated
visitors and require special handling. These will be useful for device
state serialization.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qapi/misc-qapi-visit.c | 35 +++++++++++++++++++++++++++++++++++
qapi/qc.h | 7 +++++++
2 files changed, 42 insertions(+), 0 deletions(-)
create mode 100644 qapi/misc-qapi-visit.c
diff --git a/qapi/misc-qapi-visit.c b/qapi/misc-qapi-visit.c
new file mode 100644
index 0000000..fedc063
--- /dev/null
+++ b/qapi/misc-qapi-visit.c
@@ -0,0 +1,35 @@
+#include <time.h>
+#include "qapi-visit-core.h"
+#include "qc.h"
+#include "qemu-timer.h"
+
+void visit_type_tm(Visitor *v, struct tm *obj, const char *name, Error **errp)
+{
+ visit_start_struct(v, NULL, "struct tm", name, 0, errp);
+ visit_type_int32(v, &obj->tm_year, "tm_year", errp);
+ visit_type_int32(v, &obj->tm_mon, "tm_mon", errp);
+ visit_type_int32(v, &obj->tm_mday, "tm_mday", errp);
+ visit_type_int32(v, &obj->tm_hour, "tm_hour", errp);
+ visit_type_int32(v, &obj->tm_min, "tm_min", errp);
+ visit_type_int32(v, &obj->tm_sec, "tm_sec", errp);
+ visit_end_struct(v, errp);
+}
+
+void visit_type_QEMUTimer(Visitor *v, QEMUTimer **obj, const char *name,
+ Error **errp)
+{
+ int64_t expire_time, expire_time_cpy;
+ expire_time = expire_time_cpy = qemu_timer_expire_time_ns(*obj);
+ visit_start_struct(v, NULL, "QEMUTimer", name, 0, errp);
+ visit_type_int64(v, &expire_time, "expire_time", errp);
+ visit_end_struct(v, errp);
+
+ /* if we're modifying a QEMUTimer, re-arm/delete accordingly */
+ if (expire_time != expire_time_cpy) {
+ if (expire_time != -1) {
+ qemu_mod_timer_ns(*obj, expire_time);
+ } else {
+ qemu_del_timer(*obj);
+ }
+ }
+}
diff --git a/qapi/qc.h b/qapi/qc.h
index 3b3a8b9..cf12c07 100644
--- a/qapi/qc.h
+++ b/qapi/qc.h
@@ -1,6 +1,8 @@
#ifndef QC_H
#define QC_H
+#include "qemu-timer.h"
+
#define qc_declaration
#define _immutable
#define _derived
@@ -8,4 +10,9 @@
#define _version(x)
#define _size_is(x)
+/* misc. visitors */
+void visit_type_tm(Visitor *m, struct tm *obj, const char *name, Error **errp);
+void visit_type_QEMUTimer(Visitor *v, QEMUTimer **obj, const char *name,
+ Error **errp);
+
#endif
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 08/17] rtc: move RTCState declaration to header
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (6 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 07/17] qapi: add open-coded visitors for QEMUTimer/struct tm types Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 09/17] rtc: add qc annotations Michael Roth
` (8 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
We need these definitions in a header file since the generated visitors
need to know the struct definitions to do their job. This also reduces
the amount of code we need to feed into the QIDL compiler once we
annotate it.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/mc146818rtc.c | 27 +--------------------------
hw/mc146818rtc_state.h | 32 ++++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 26 deletions(-)
create mode 100644 hw/mc146818rtc_state.h
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 3777f85..7490d28 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -25,6 +25,7 @@
#include "qemu-timer.h"
#include "sysemu.h"
#include "mc146818rtc.h"
+#include "mc146818rtc_state.h"
#ifdef TARGET_I386
#include "apic.h"
@@ -47,32 +48,6 @@
#define RTC_REINJECT_ON_ACK_COUNT 20
-typedef struct RTCState {
- ISADevice dev;
- MemoryRegion io;
- uint8_t cmos_data[128];
- uint8_t cmos_index;
- struct tm current_tm;
- int32_t base_year;
- qemu_irq irq;
- qemu_irq sqw_irq;
- int it_shift;
- /* periodic timer */
- QEMUTimer *periodic_timer;
- int64_t next_periodic_time;
- /* second update */
- int64_t next_second_time;
- uint16_t irq_reinject_on_ack_count;
- uint32_t irq_coalesced;
- uint32_t period;
- QEMUTimer *coalesced_timer;
- QEMUTimer *second_timer;
- QEMUTimer *second_timer2;
- Notifier clock_reset_notifier;
- LostTickPolicy lost_tick_policy;
- Notifier suspend_notifier;
-} RTCState;
-
static void rtc_set_time(RTCState *s);
static void rtc_copy_date(RTCState *s);
diff --git a/hw/mc146818rtc_state.h b/hw/mc146818rtc_state.h
new file mode 100644
index 0000000..f819e15
--- /dev/null
+++ b/hw/mc146818rtc_state.h
@@ -0,0 +1,32 @@
+#ifndef MC146818RTC_STATE_H
+#define MC146818RTC_STATE_H
+
+#include "isa.h"
+
+typedef struct RTCState {
+ ISADevice dev;
+ MemoryRegion io;
+ uint8_t cmos_data[128];
+ uint8_t cmos_index;
+ struct tm current_tm;
+ int32_t base_year;
+ qemu_irq irq;
+ qemu_irq sqw_irq;
+ int it_shift;
+ /* periodic timer */
+ QEMUTimer *periodic_timer;
+ int64_t next_periodic_time;
+ /* second update */
+ int64_t next_second_time;
+ uint16_t irq_reinject_on_ack_count;
+ uint32_t irq_coalesced;
+ uint32_t period;
+ QEMUTimer *coalesced_timer;
+ QEMUTimer *second_timer;
+ QEMUTimer *second_timer2;
+ Notifier clock_reset_notifier;
+ LostTickPolicy lost_tick_policy;
+ Notifier suspend_notifier;
+} RTCState;
+
+#endif /* !MC146818RTC_STATE_H */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 09/17] rtc: add qc annotations
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (7 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 08/17] rtc: move RTCState declaration to header Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 10:25 ` Avi Kivity
2012-06-05 1:00 ` [Qemu-devel] [PATCH 10/17] Makefile: add infrastructure to incorporate qidl-generated files Michael Roth
` (7 subsequent siblings)
16 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Add our annotations according to QIDL documentation.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/mc146818rtc_state.h | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/hw/mc146818rtc_state.h b/hw/mc146818rtc_state.h
index f819e15..9347ee6 100644
--- a/hw/mc146818rtc_state.h
+++ b/hw/mc146818rtc_state.h
@@ -2,31 +2,32 @@
#define MC146818RTC_STATE_H
#include "isa.h"
+#include "qapi/qc.h"
-typedef struct RTCState {
- ISADevice dev;
- MemoryRegion io;
+qc_declaration typedef struct RTCState {
+ ISADevice _immutable dev;
+ MemoryRegion _immutable io;
uint8_t cmos_data[128];
uint8_t cmos_index;
struct tm current_tm;
int32_t base_year;
- qemu_irq irq;
- qemu_irq sqw_irq;
- int it_shift;
+ qemu_irq _immutable irq;
+ qemu_irq _immutable sqw_irq;
+ int32_t _immutable it_shift;
/* periodic timer */
QEMUTimer *periodic_timer;
int64_t next_periodic_time;
/* second update */
int64_t next_second_time;
- uint16_t irq_reinject_on_ack_count;
+ uint16_t _derived irq_reinject_on_ack_count;
uint32_t irq_coalesced;
uint32_t period;
- QEMUTimer *coalesced_timer;
+ QEMUTimer _broken *coalesced_timer;
QEMUTimer *second_timer;
QEMUTimer *second_timer2;
- Notifier clock_reset_notifier;
- LostTickPolicy lost_tick_policy;
- Notifier suspend_notifier;
+ Notifier _broken clock_reset_notifier;
+ LostTickPolicy _immutable lost_tick_policy;
+ Notifier _broken suspend_notifier;
} RTCState;
#endif /* !MC146818RTC_STATE_H */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] rtc: add qc annotations
2012-06-05 1:00 ` [Qemu-devel] [PATCH 09/17] rtc: add qc annotations Michael Roth
@ 2012-06-05 10:25 ` Avi Kivity
2012-06-05 10:40 ` Jan Kiszka
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-05 10:25 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/05/2012 04:00 AM, Michael Roth wrote:
> Add our annotations according to QIDL documentation.
>
> +qc_declaration typedef struct RTCState {
> + ISADevice _immutable dev;
> + MemoryRegion _immutable io;
> uint8_t cmos_data[128];
> uint8_t cmos_index;
> struct tm current_tm;
> int32_t base_year;
> - qemu_irq irq;
> - qemu_irq sqw_irq;
> - int it_shift;
> + qemu_irq _immutable irq;
> + qemu_irq _immutable sqw_irq;
How is qemu_irq immutable? We're raising and lowering it many times a
second. It's _derived, perhaps, but not immutable.
> + int32_t _immutable it_shift;
> /* periodic timer */
> QEMUTimer *periodic_timer;
> int64_t next_periodic_time;
> /* second update */
> int64_t next_second_time;
> - uint16_t irq_reinject_on_ack_count;
> + uint16_t _derived irq_reinject_on_ack_count;
It's not derived from anything. It's _host, maybe.
> uint32_t irq_coalesced;
> uint32_t period;
> - QEMUTimer *coalesced_timer;
> + QEMUTimer _broken *coalesced_timer;
> QEMUTimer *second_timer;
> QEMUTimer *second_timer2;
> - Notifier clock_reset_notifier;
> - LostTickPolicy lost_tick_policy;
> - Notifier suspend_notifier;
> + Notifier _broken clock_reset_notifier;
Why broken?
> + LostTickPolicy _immutable lost_tick_policy;
_host; nothign prevents us from changing it dynamically in theory.
> + Notifier _broken suspend_notifier;
Why broken?
> } RTCState;
>
> #endif /* !MC146818RTC_STATE_H */
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] rtc: add qc annotations
2012-06-05 10:25 ` Avi Kivity
@ 2012-06-05 10:40 ` Jan Kiszka
2012-06-05 12:42 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Jan Kiszka @ 2012-06-05 10:40 UTC (permalink / raw)
To: Avi Kivity
Cc: aliguori, owasserm, quintela, qemu-devel, Michael Roth, yamahata,
pbonzini, akong, afaerber
On 2012-06-05 12:25, Avi Kivity wrote:
> On 06/05/2012 04:00 AM, Michael Roth wrote:
>> Add our annotations according to QIDL documentation.
>>
>> +qc_declaration typedef struct RTCState {
>> + ISADevice _immutable dev;
>> + MemoryRegion _immutable io;
>> uint8_t cmos_data[128];
>> uint8_t cmos_index;
>> struct tm current_tm;
>> int32_t base_year;
>> - qemu_irq irq;
>> - qemu_irq sqw_irq;
>> - int it_shift;
>> + qemu_irq _immutable irq;
>> + qemu_irq _immutable sqw_irq;
>
> How is qemu_irq immutable? We're raising and lowering it many times a
> second. It's _derived, perhaps, but not immutable.
No, IRQState in its current form is immutable, doesn't contain any
volatile state.
>
>> + int32_t _immutable it_shift;
>> /* periodic timer */
>> QEMUTimer *periodic_timer;
>> int64_t next_periodic_time;
>> /* second update */
>> int64_t next_second_time;
>> - uint16_t irq_reinject_on_ack_count;
>> + uint16_t _derived irq_reinject_on_ack_count;
>
> It's not derived from anything. It's _host, maybe.
I think it is _broken.
>
>> uint32_t irq_coalesced;
>> uint32_t period;
>> - QEMUTimer *coalesced_timer;
>> + QEMUTimer _broken *coalesced_timer;
>> QEMUTimer *second_timer;
>> QEMUTimer *second_timer2;
>> - Notifier clock_reset_notifier;
>> - LostTickPolicy lost_tick_policy;
>> - Notifier suspend_notifier;
>> + Notifier _broken clock_reset_notifier;
>
> Why broken?
>
>> + LostTickPolicy _immutable lost_tick_policy;
>
> _host; nothign prevents us from changing it dynamically in theory.
_host makes no sense to me. Either it is _immutable or _broken - or is
properly saved/restored. What should be the semantic of _host?
>
>> + Notifier _broken suspend_notifier;
>
> Why broken?
>
>> } RTCState;
>>
>> #endif /* !MC146818RTC_STATE_H */
>
>
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] rtc: add qc annotations
2012-06-05 10:40 ` Jan Kiszka
@ 2012-06-05 12:42 ` Avi Kivity
2012-06-05 22:07 ` Michael Roth
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-05 12:42 UTC (permalink / raw)
To: Jan Kiszka
Cc: aliguori, owasserm, quintela, qemu-devel, Michael Roth, yamahata,
pbonzini, akong, afaerber
On 06/05/2012 01:40 PM, Jan Kiszka wrote:
> On 2012-06-05 12:25, Avi Kivity wrote:
>> On 06/05/2012 04:00 AM, Michael Roth wrote:
>>> Add our annotations according to QIDL documentation.
>>>
>>> +qc_declaration typedef struct RTCState {
>>> + ISADevice _immutable dev;
>>> + MemoryRegion _immutable io;
>>> uint8_t cmos_data[128];
>>> uint8_t cmos_index;
>>> struct tm current_tm;
>>> int32_t base_year;
>>> - qemu_irq irq;
>>> - qemu_irq sqw_irq;
>>> - int it_shift;
>>> + qemu_irq _immutable irq;
>>> + qemu_irq _immutable sqw_irq;
>>
>> How is qemu_irq immutable? We're raising and lowering it many times a
>> second. It's _derived, perhaps, but not immutable.
>
> No, IRQState in its current form is immutable, doesn't contain any
> volatile state.
Good point. So it's just like any pointer: it depends on the pointed-to
type. If it saves its state, then great, but if the pointed-to type
doesn't, then it's broken.
>
>>
>>> + int32_t _immutable it_shift;
>>> /* periodic timer */
>>> QEMUTimer *periodic_timer;
>>> int64_t next_periodic_time;
>>> /* second update */
>>> int64_t next_second_time;
>>> - uint16_t irq_reinject_on_ack_count;
>>> + uint16_t _derived irq_reinject_on_ack_count;
>>
>> It's not derived from anything. It's _host, maybe.
>
> I think it is _broken.
I think it's _complicated. Migration involves downtime and so lost
ticks. In the case of RTC we probably need to trigger compensation code
that will try to handle it according to policy.
>>> + LostTickPolicy _immutable lost_tick_policy;
>>
>> _host; nothign prevents us from changing it dynamically in theory.
>
> _host makes no sense to me. Either it is _immutable or _broken - or is
> properly saved/restored. What should be the semantic of _host?
An emulated device manages some state, and also talks to a host device,
often through an interface like BlockDriverState or CharState. _host is
anything related to the host device that we should be able to
reconstruct on resume.
Example of host state is a CharDriverState filename. Even if we allow
it to change, there is no point in migrating it since it belongs to the
source namespace, not destination.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 09/17] rtc: add qc annotations
2012-06-05 12:42 ` Avi Kivity
@ 2012-06-05 22:07 ` Michael Roth
0 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 22:07 UTC (permalink / raw)
To: Avi Kivity
Cc: aliguori, quintela, Jan Kiszka, owasserm, qemu-devel, yamahata,
pbonzini, akong, afaerber
On Tue, Jun 05, 2012 at 03:42:30PM +0300, Avi Kivity wrote:
> On 06/05/2012 01:40 PM, Jan Kiszka wrote:
> > On 2012-06-05 12:25, Avi Kivity wrote:
> >> On 06/05/2012 04:00 AM, Michael Roth wrote:
> >>> Add our annotations according to QIDL documentation.
> >>>
> >>> +qc_declaration typedef struct RTCState {
> >>> + ISADevice _immutable dev;
> >>> + MemoryRegion _immutable io;
> >>> uint8_t cmos_data[128];
> >>> uint8_t cmos_index;
> >>> struct tm current_tm;
> >>> int32_t base_year;
> >>> - qemu_irq irq;
> >>> - qemu_irq sqw_irq;
> >>> - int it_shift;
> >>> + qemu_irq _immutable irq;
> >>> + qemu_irq _immutable sqw_irq;
> >>
> >> How is qemu_irq immutable? We're raising and lowering it many times a
> >> second. It's _derived, perhaps, but not immutable.
> >
> > No, IRQState in its current form is immutable, doesn't contain any
> > volatile state.
>
> Good point. So it's just like any pointer: it depends on the pointed-to
> type. If it saves its state, then great, but if the pointed-to type
> doesn't, then it's broken.
>
> >
> >>
> >>> + int32_t _immutable it_shift;
> >>> /* periodic timer */
> >>> QEMUTimer *periodic_timer;
> >>> int64_t next_periodic_time;
> >>> /* second update */
> >>> int64_t next_second_time;
> >>> - uint16_t irq_reinject_on_ack_count;
> >>> + uint16_t _derived irq_reinject_on_ack_count;
> >>
> >> It's not derived from anything. It's _host, maybe.
> >
> > I think it is _broken.
Agreed, using _derived was an error on my part
>
> I think it's _complicated. Migration involves downtime and so lost
> ticks. In the case of RTC we probably need to trigger compensation code
> that will try to handle it according to policy.
We'd likely only be able to compensate based on calculated downtime or
something along that line. I think we'd still want to send accumulated ticks
as well, even if it's of little importance, since it's still guest state
of a sort (in the sense that it guest-perceivable) that we should avoid
discarding.
>
> >>> + LostTickPolicy _immutable lost_tick_policy;
> >>
> >> _host; nothign prevents us from changing it dynamically in theory.
> >
> > _host makes no sense to me. Either it is _immutable or _broken - or is
> > properly saved/restored. What should be the semantic of _host?
>
> An emulated device manages some state, and also talks to a host device,
> often through an interface like BlockDriverState or CharState. _host is
> anything related to the host device that we should be able to
> reconstruct on resume.
>
> Example of host state is a CharDriverState filename. Even if we allow
> it to change, there is no point in migrating it since it belongs to the
> source namespace, not destination.
>
> --
> error compiling committee.c: too many arguments to function
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 10/17] Makefile: add infrastructure to incorporate qidl-generated files
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (8 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 09/17] rtc: add qc annotations Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc Michael Roth
` (6 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
This will rebuild the qidl-generated qapi schemas in
$(SRC_DIR)/qidl-generated whenever we do a build where of the annotated
sources has been modified. As a result, changes will be reflected in the
working tree so we can detect changes and commit them in cases where the
changes are correct/expected.
We also add the necessary targets for compiling and linking in the
qapi-generated visitors for device state.
RTC is the first user.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile.objs | 6 ++++++
Makefile.target | 37 ++++++++++++++++++++++++++++++++++++-
2 files changed, 42 insertions(+), 1 deletions(-)
diff --git a/Makefile.objs b/Makefile.objs
index 70c5c79..373d5af 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -467,6 +467,12 @@ qga-obj-y += qemu-ga.o module.o
qga-obj-$(CONFIG_WIN32) += oslib-win32.o
qga-obj-$(CONFIG_POSIX) += oslib-posix.o qemu-sockets.o qemu-option.o
+######################################################################
+# qidl
+
+qidl-obj-y += $(qapi-obj-y) qapi/misc-qapi-visit.o
+
+
vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
diff --git a/Makefile.target b/Makefile.target
index 1582904..45e4459 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -20,6 +20,10 @@ QEMU_CFLAGS += -I../linux-headers
endif
QEMU_CFLAGS += -I.. -I$(TARGET_PATH) -DNEED_CPU_H
+qidl-dir := $(SRC_PATH)/qidl-generated
+qapi-dir := $(BUILD_DIR)/qapi-generated
+QEMU_CFLAGS += -I$(qapi-dir)
+
include $(SRC_PATH)/Makefile.objs
QEMU_CFLAGS+=-I$(SRC_PATH)/include
@@ -76,6 +80,35 @@ all: $(PROGS) stap
@true
#########################################################
+# QAPI-generated device state visitors
+QIDL_SOURCES_SHORT=hw/mc146818rtc_state.h
+QIDL_SOURCES=$(addprefix $(SRC_PATH)/, $(QIDL_SOURCES_SHORT))
+QIDL_SCHEMAS=$(patsubst %.h,$(qidl-dir)/%.json, $(subst _state,, $(notdir $(QIDL_SOURCES))))
+
+$(QIDL_SCHEMAS) qidl: $(QIDL_SOURCES) $(SRC_PATH)/scripts/qc.py
+ mkdir -p $(qidl-dir)
+ for f in $(QIDL_SOURCES); do \
+ json_filename=$(qidl-dir)/`basename $$(basename $$f .h) _state`.json; \
+ $(PYTHON) $(SRC_PATH)/scripts/qc.py < $$f >$$json_filename; \
+ done
+
+QIDL_DEVICES=mc146818rtc.o
+QIDL_DEVICE_VISITORS=$(qapi-dir)/mc146818rtc-qapi-visit.o
+QIDL_DEVICE_VISITOR_SOURCES=$(patsubst %.o,%.c, $(QIDL_DEVICE_VISITORS))
+QIDL_DEVICES_VISITOR_SOURCES+=$(patsubst %.o,%.h, $(QIDL_DEVICE_VISITORS))
+
+$(QIDL_DEVICE_VISITOR_SOURCES): $(QIDL_SCHEMAS) $(SRC_PATH)/scripts/qapi-visit.py
+ echo $(QIDL_SCHEMAS)
+ $(call quiet-command,\
+ $(PYTHON) $(SRC_PATH)/scripts/qapi-visit.py -e -o "$(qapi-dir)" \
+ -i hw/$(patsubst %-qapi-visit,%_state.h, $(basename $(notdir $@))) \
+ -p "$(patsubst %-qapi-visit,%-, $(basename $(notdir $@)))" \
+ < $(qidl-dir)/$(patsubst %-qapi-visit,%.json, $(basename $(notdir $@))), " GEN $@")
+
+$(QIDL_DEVICE_VISITORS): $(QIDL_DEVICE_VISITOR_SOURCES)
+$(QIDL_DEVICES): $(QIDL_DEVICE_VISITORS)
+
+#########################################################
# cpu emulator library
libobj-y = exec.o translate-all.o cpu-exec.o translate.o
libobj-y += tcg/tcg.o tcg/optimize.o
@@ -130,6 +163,7 @@ obj-y = main.o syscall.o strace.o mmap.o signal.o thunk.o \
elfload.o linuxload.o uaccess.o gdbstub.o cpu-uname.o \
user-exec.o $(oslib-obj-y)
+
obj-$(TARGET_HAS_BFLT) += flatload.o
obj-$(TARGET_I386) += vm86.o
@@ -220,7 +254,7 @@ obj-$(CONFIG_IVSHMEM) += ivshmem.o
obj-y += device-hotplug.o
# Hardware support
-obj-i386-y += mc146818rtc.o pc.o
+obj-i386-y += mc146818rtc.o $(qapi-dir)/mc146818rtc-qapi-visit.o pc.o
obj-i386-y += apic_common.o apic.o kvmvapic.o
obj-i386-y += sga.o ioapic_common.o ioapic.o piix_pci.o
obj-i386-y += vmport.o
@@ -399,6 +433,7 @@ obj-y += $(addprefix ../libdis/, $(libdis-y))
obj-y += $(libobj-y)
obj-y += $(addprefix $(HWDIR)/, $(hw-obj-y))
obj-y += $(addprefix ../, $(trace-obj-y))
+obj-y += $(addprefix ../, $(qidl-obj-y))
endif # CONFIG_SOFTMMU
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (9 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 10/17] Makefile: add infrastructure to incorporate qidl-generated files Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 9:29 ` Kevin Wolf
2012-06-05 10:11 ` Avi Kivity
2012-06-05 1:00 ` [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state Michael Roth
` (5 subsequent siblings)
16 siblings, 2 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qidl-generated/mc146818rtc.json | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 qidl-generated/mc146818rtc.json
diff --git a/qidl-generated/mc146818rtc.json b/qidl-generated/mc146818rtc.json
new file mode 100644
index 0000000..3be6fb6
--- /dev/null
+++ b/qidl-generated/mc146818rtc.json
@@ -0,0 +1 @@
+{'type': 'RTCState', 'data': {'cmos_data': {'type': ['uint8'], '<annotated>': 'true', 'array_size': '128'}, 'cmos_index': 'uint8', 'current_tm': 'tm', 'base_year': 'int32', 'periodic_timer': 'QEMUTimer', 'next_periodic_time': 'int64', 'next_second_time': 'int64', 'irq_coalesced': 'uint32', 'period': 'uint32', 'second_timer': 'QEMUTimer', 'second_timer2': 'QEMUTimer'}}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-05 1:00 ` [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc Michael Roth
@ 2012-06-05 9:29 ` Kevin Wolf
2012-06-05 16:03 ` Michael Roth
2012-06-05 10:11 ` Avi Kivity
1 sibling, 1 reply; 81+ messages in thread
From: Kevin Wolf @ 2012-06-05 9:29 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
Am 05.06.2012 03:00, schrieb Michael Roth:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qidl-generated/mc146818rtc.json | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
> create mode 100644 qidl-generated/mc146818rtc.json
I haven't looked at the Makefiles, but does this commit mean that the
files aren't generated automatically but you have to run the generator
manually after changing any device struct?
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-05 9:29 ` Kevin Wolf
@ 2012-06-05 16:03 ` Michael Roth
2012-06-06 7:38 ` Kevin Wolf
0 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 16:03 UTC (permalink / raw)
To: Kevin Wolf
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On Tue, Jun 05, 2012 at 11:29:24AM +0200, Kevin Wolf wrote:
> Am 05.06.2012 03:00, schrieb Michael Roth:
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > qidl-generated/mc146818rtc.json | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> > create mode 100644 qidl-generated/mc146818rtc.json
>
> I haven't looked at the Makefiles, but does this commit mean that the
> files aren't generated automatically but you have to run the generator
> manually after changing any device struct?
Nope, the files are automatically generated when changes are made to
QIDL sources and you do a build.
The reason they're still checked-in is so that changes to a device's
serialization schema can be "signed-off" by the author that made the
change. This applies to qidl-generated vmstate descriptions as well.
It also makes an automated `make check-qidl` and, in the case of
qidl-generated vmstate descriptions, `make check-vmstate` possible, so
that a submitter/maintainer can detect and bring attention to changes to
serialized device state that need to be addressed/signed-off when
testing/reviewing patches.
We could get part of the way there by just keeping tabs on changes to qidl
sources, but ultimately how we do the serialization is a matter of how the
generated visitors look, in which case the generated QAPI schemas are the more
reliable representation. Annotations are hints, schemas are ABI, so
tracking the latter is more important.
Similar rationale for vmstate: the relationship between annotations and
the generated vmstate descriptions isn't strong enough that we can
easily infer changes based on qidl annotations, and in many cases those
inferred changes will be overwritten by special handling in the vmstate
generator.
It is ugly though... and I'm certainly open to alternative approaches.
>
> Kevin
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-05 16:03 ` Michael Roth
@ 2012-06-06 7:38 ` Kevin Wolf
2012-06-06 22:40 ` Michael Roth
0 siblings, 1 reply; 81+ messages in thread
From: Kevin Wolf @ 2012-06-06 7:38 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
Am 05.06.2012 18:03, schrieb Michael Roth:
> On Tue, Jun 05, 2012 at 11:29:24AM +0200, Kevin Wolf wrote:
>> Am 05.06.2012 03:00, schrieb Michael Roth:
>>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> ---
>>> qidl-generated/mc146818rtc.json | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>> create mode 100644 qidl-generated/mc146818rtc.json
>>
>> I haven't looked at the Makefiles, but does this commit mean that the
>> files aren't generated automatically but you have to run the generator
>> manually after changing any device struct?
>
> Nope, the files are automatically generated when changes are made to
> QIDL sources and you do a build.
>
> The reason they're still checked-in is so that changes to a device's
> serialization schema can be "signed-off" by the author that made the
> change. This applies to qidl-generated vmstate descriptions as well.
Doesn't really make sense to me. You already have a sign-off for the
changed header/source file.
> It also makes an automated `make check-qidl` and, in the case of
> qidl-generated vmstate descriptions, `make check-vmstate` possible, so
> that a submitter/maintainer can detect and bring attention to changes to
> serialized device state that need to be addressed/signed-off when
> testing/reviewing patches.
Why can't 'make check-qidl' generate the new version itself like a
simple 'make' would do?
> We could get part of the way there by just keeping tabs on changes to qidl
> sources, but ultimately how we do the serialization is a matter of how the
> generated visitors look, in which case the generated QAPI schemas are the more
> reliable representation. Annotations are hints, schemas are ABI, so
> tracking the latter is more important.
So your statement is that the generator is likely buggy and therefore
its output should be reviewed as well as the source changes?
> Similar rationale for vmstate: the relationship between annotations and
> the generated vmstate descriptions isn't strong enough that we can
> easily infer changes based on qidl annotations, and in many cases those
> inferred changes will be overwritten by special handling in the vmstate
> generator.
I don't understand. Is this file generated or manually edited? If the
former, why does having it in the repository add anything new when you
can (and with appropriate Makefile magic will) always run the generator
after pulling changes to source files? If the latter, why does the
subject say it's generated?
Kevin
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-06 7:38 ` Kevin Wolf
@ 2012-06-06 22:40 ` Michael Roth
0 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-06 22:40 UTC (permalink / raw)
To: Kevin Wolf
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On Wed, Jun 06, 2012 at 09:38:24AM +0200, Kevin Wolf wrote:
> Am 05.06.2012 18:03, schrieb Michael Roth:
> > On Tue, Jun 05, 2012 at 11:29:24AM +0200, Kevin Wolf wrote:
> >> Am 05.06.2012 03:00, schrieb Michael Roth:
> >>> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> >>> ---
> >>> qidl-generated/mc146818rtc.json | 1 +
> >>> 1 files changed, 1 insertions(+), 0 deletions(-)
> >>> create mode 100644 qidl-generated/mc146818rtc.json
> >>
> >> I haven't looked at the Makefiles, but does this commit mean that the
> >> files aren't generated automatically but you have to run the generator
> >> manually after changing any device struct?
> >
> > Nope, the files are automatically generated when changes are made to
> > QIDL sources and you do a build.
> >
> > The reason they're still checked-in is so that changes to a device's
> > serialization schema can be "signed-off" by the author that made the
> > change. This applies to qidl-generated vmstate descriptions as well.
>
> Doesn't really make sense to me. You already have a sign-off for the
> changed header/source file.
>
> > It also makes an automated `make check-qidl` and, in the case of
> > qidl-generated vmstate descriptions, `make check-vmstate` possible, so
> > that a submitter/maintainer can detect and bring attention to changes to
> > serialized device state that need to be addressed/signed-off when
> > testing/reviewing patches.
>
> Why can't 'make check-qidl' generate the new version itself like a
> simple 'make' would do?
>
Because unless the generated code is checked in at some point, make
check-qidl doesn't have anything to check the newly-generated code against
to flag uncommitted/"undocumented" changes to the ABI.
> > We could get part of the way there by just keeping tabs on changes to qidl
> > sources, but ultimately how we do the serialization is a matter of how the
> > generated visitors look, in which case the generated QAPI schemas are the more
> > reliable representation. Annotations are hints, schemas are ABI, so
> > tracking the latter is more important.
>
> So your statement is that the generator is likely buggy and therefore
> its output should be reviewed as well as the source changes?
The QIDL annotations are a tool/guideline for developers to ease the burdon
of having to think too much about the serialized data representation, but
they can still be misused or neglected, and those changes can still slip
by and break migration just as easily vmstate can be broken today.
Tracking changes to generated QAPI schemas is a way to draw attention to
such changes.
It's easy for some odd field to get a s/int/int64_t/ as
part of a substantial patch series and break compatability.
By committing the generated code, it's easy to detect when this happens,
easily note implications for migration compatability, then go back and review
those device state changes for correctness.
And keeping tabs on QAPI schemas changes for QMP/guest agent ABIs has
proven pretty successful, whereas keeping tabs on device state
changes hasn't.
>
> > Similar rationale for vmstate: the relationship between annotations and
> > the generated vmstate descriptions isn't strong enough that we can
> > easily infer changes based on qidl annotations, and in many cases those
> > inferred changes will be overwritten by special handling in the vmstate
> > generator.
>
> I don't understand. Is this file generated or manually edited? If the
> former, why does having it in the repository add anything new when you
> can (and with appropriate Makefile magic will) always run the generator
> after pulling changes to source files? If the latter, why does the
> subject say it's generated?
It's generated. The rationale is similar to the case for checking in
generated QAPI schemas: we track changes to the generated code to easily
infer the implications for migration on a patch-by-patch basis. It's
easilly to tell when changes have been made to a VMStateDescription in
qidl-generated/, much easier than spotting every extra field or type
change etc. to the various device state structures, especially given
that those are generally part of longer changesets whereas the files in
qidl-generated/ serve one well-known purpose.
>
> Kevin
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc
2012-06-05 1:00 ` [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc Michael Roth
2012-06-05 9:29 ` Kevin Wolf
@ 2012-06-05 10:11 ` Avi Kivity
1 sibling, 0 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-05 10:11 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/05/2012 04:00 AM, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> qidl-generated/mc146818rtc.json | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
> create mode 100644 qidl-generated/mc146818rtc.json
>
> diff --git a/qidl-generated/mc146818rtc.json b/qidl-generated/mc146818rtc.json
> new file mode 100644
> index 0000000..3be6fb6
> --- /dev/null
> +++ b/qidl-generated/mc146818rtc.json
> @@ -0,0 +1 @@
> +{'type': 'RTCState', 'data': {'cmos_data': {'type': ['uint8'], '<annotated>': 'true', 'array_size': '128'}, 'cmos_index': 'uint8', 'current_tm': 'tm', 'base_year': 'int32', 'periodic_timer': 'QEMUTimer', 'next_periodic_time': 'int64', 'next_second_time': 'int64', 'irq_coalesced': 'uint32', 'period': 'uint32', 'second_timer': 'QEMUTimer', 'second_timer2': 'QEMUTimer'}}
Why are we committing generated files?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (10 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 11/17] qapi: add qidl-generated qapi schema for rtc Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 14:14 ` Paolo Bonzini
2012-06-05 1:00 ` [Qemu-devel] [PATCH 13/17] rtc: add _version() qidl annotations Michael Roth
` (4 subsequent siblings)
16 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
This will eventually be used to serialize device state for the purposes
of migration/savevm, and is also useful for introspection/testing.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/mc146818rtc.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 7490d28..2dfc233 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -26,6 +26,7 @@
#include "sysemu.h"
#include "mc146818rtc.h"
#include "mc146818rtc_state.h"
+#include "qapi-generated/mc146818rtc-qapi-visit.h"
#ifdef TARGET_I386
#include "apic.h"
@@ -590,6 +591,14 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
visit_end_struct(v, errp);
}
+static void rtc_get_state(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ ISADevice *isa = ISA_DEVICE(obj);
+ RTCState *s = DO_UPCAST(RTCState, dev, isa);
+ visit_type_RTCState(v, &s, name, errp);
+}
+
static int rtc_initfn(ISADevice *dev)
{
RTCState *s = DO_UPCAST(RTCState, dev, dev);
@@ -638,6 +647,9 @@ static int rtc_initfn(ISADevice *dev)
object_property_add(OBJECT(s), "date", "struct tm",
rtc_get_date, NULL, NULL, s, NULL);
+ object_property_add(OBJECT(s), "state", "RTCState",
+ rtc_get_state, NULL, NULL, s, NULL);
+
return 0;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state
2012-06-05 1:00 ` [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state Michael Roth
@ 2012-06-05 14:14 ` Paolo Bonzini
2012-06-05 17:54 ` Michael Roth
0 siblings, 1 reply; 81+ messages in thread
From: Paolo Bonzini @ 2012-06-05 14:14 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, akong,
afaerber
Il 05/06/2012 03:00, Michael Roth ha scritto:
> This will eventually be used to serialize device state for the purposes
> of migration/savevm, and is also useful for introspection/testing.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> hw/mc146818rtc.c | 12 ++++++++++++
> 1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 7490d28..2dfc233 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -26,6 +26,7 @@
> #include "sysemu.h"
> #include "mc146818rtc.h"
> #include "mc146818rtc_state.h"
> +#include "qapi-generated/mc146818rtc-qapi-visit.h"
>
> #ifdef TARGET_I386
> #include "apic.h"
> @@ -590,6 +591,14 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
> visit_end_struct(v, errp);
> }
>
> +static void rtc_get_state(Object *obj, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + ISADevice *isa = ISA_DEVICE(obj);
> + RTCState *s = DO_UPCAST(RTCState, dev, isa);
> + visit_type_RTCState(v, &s, name, errp);
> +}
> +
> static int rtc_initfn(ISADevice *dev)
> {
> RTCState *s = DO_UPCAST(RTCState, dev, dev);
> @@ -638,6 +647,9 @@ static int rtc_initfn(ISADevice *dev)
> object_property_add(OBJECT(s), "date", "struct tm",
> rtc_get_date, NULL, NULL, s, NULL);
>
> + object_property_add(OBJECT(s), "state", "RTCState",
> + rtc_get_state, NULL, NULL, s, NULL);
> +
> return 0;
> }
>
Nice! The next obvious step would be to use this from vmstate instead
of hardcoded struct offsets.
Paolo
^ permalink raw reply [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state
2012-06-05 14:14 ` Paolo Bonzini
@ 2012-06-05 17:54 ` Michael Roth
0 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 17:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: aliguori, owasserm, quintela, qemu-devel, yamahata, akong,
afaerber
On Tue, Jun 05, 2012 at 04:14:44PM +0200, Paolo Bonzini wrote:
> Il 05/06/2012 03:00, Michael Roth ha scritto:
> > This will eventually be used to serialize device state for the purposes
> > of migration/savevm, and is also useful for introspection/testing.
> >
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > hw/mc146818rtc.c | 12 ++++++++++++
> > 1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> > index 7490d28..2dfc233 100644
> > --- a/hw/mc146818rtc.c
> > +++ b/hw/mc146818rtc.c
> > @@ -26,6 +26,7 @@
> > #include "sysemu.h"
> > #include "mc146818rtc.h"
> > #include "mc146818rtc_state.h"
> > +#include "qapi-generated/mc146818rtc-qapi-visit.h"
> >
> > #ifdef TARGET_I386
> > #include "apic.h"
> > @@ -590,6 +591,14 @@ static void rtc_get_date(Object *obj, Visitor *v, void *opaque,
> > visit_end_struct(v, errp);
> > }
> >
> > +static void rtc_get_state(Object *obj, Visitor *v, void *opaque,
> > + const char *name, Error **errp)
> > +{
> > + ISADevice *isa = ISA_DEVICE(obj);
> > + RTCState *s = DO_UPCAST(RTCState, dev, isa);
> > + visit_type_RTCState(v, &s, name, errp);
> > +}
> > +
> > static int rtc_initfn(ISADevice *dev)
> > {
> > RTCState *s = DO_UPCAST(RTCState, dev, dev);
> > @@ -638,6 +647,9 @@ static int rtc_initfn(ISADevice *dev)
> > object_property_add(OBJECT(s), "date", "struct tm",
> > rtc_get_date, NULL, NULL, s, NULL);
> >
> > + object_property_add(OBJECT(s), "state", "RTCState",
> > + rtc_get_state, NULL, NULL, s, NULL);
> > +
> > return 0;
> > }
> >
>
> Nice! The next obvious step would be to use this from vmstate instead
> of hardcoded struct offsets.
Heh, indeed. I actually looked into this quite a bit thinking it was the
next logical progression and was hoping to make this part of the RFC.
The main issues I ran into were the numerous cases where vmstate
descriptions will "flatten" embedded struct fields rather than marking them as
VMS_STRUCT, as it creates a hard incompatibility with, for instance, the
way "struct tm current_tm" is serialized. We end up needing to do things
like parsing VMStateField.name for "." characters so we know additional
levels of traversal are needed to fetch the serialized data. There are also
areas where we unroll arrays that cause similar pains.
I think it's still doable, but it ends up adding a lot of complexity to
the last place we need it.
The main benefit, in my mind, was decoupling vmstate from the actual device
state such that the vmstate field descriptions became purely a stable "wire"
protocol of sorts, and the serialized data could be transformed to
maintain that compatibility regardless of how much an actual device's
structure changed. But the cases where manipulating a QObject would
cover more cases than adding/modifying vmstate field descriptors seem
like they'd be too rare to warrant the added complexity. Having this
capability in the context of a wire protocol that's better seperated from the
data representation makes sense, but in the case of vmstate there's too much
overlap and too much complexity IMO.
>
> Paolo
>
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 13/17] rtc: add _version() qidl annotations
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (11 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 12/17] rtc: add a QOM property for accessing device state Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 14/17] qidl: add qidl-based generation of vmstate field bindings Michael Roth
` (3 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
VMState relies on per-field versioning in some cases. We don't use this
for serialization, but it is needed for proper generation of VMState
field decriptions.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/mc146818rtc_state.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/mc146818rtc_state.h b/hw/mc146818rtc_state.h
index 9347ee6..3085bed 100644
--- a/hw/mc146818rtc_state.h
+++ b/hw/mc146818rtc_state.h
@@ -20,8 +20,8 @@ qc_declaration typedef struct RTCState {
/* second update */
int64_t next_second_time;
uint16_t _derived irq_reinject_on_ack_count;
- uint32_t irq_coalesced;
- uint32_t period;
+ uint32_t _version(2) irq_coalesced;
+ uint32_t _version(2) period;
QEMUTimer _broken *coalesced_timer;
QEMUTimer *second_timer;
QEMUTimer *second_timer2;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 14/17] qidl: add qidl-based generation of vmstate field bindings
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (12 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 13/17] rtc: add _version() qidl annotations Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 15/17] Makefile: add qidl-generation of vmstate field descriptions Michael Roth
` (2 subsequent siblings)
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Add support for generating VMState field bindings/descriptions based on
QIDL annotations. Use a look-up table with hard-coded fields for cases where
we can't rely infer it from QIDL. These hard-coded fields will be quite
numerous, but the majority of fields should be coverable, and as such will
allow for automatic detection of migration changes for a broad swath of
device state.
And we almost get these for free from the serialization annotations so why
not.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
scripts/qc.py | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 106 insertions(+), 1 deletions(-)
diff --git a/scripts/qc.py b/scripts/qc.py
index 74f2a40..0f3942c 100755
--- a/scripts/qc.py
+++ b/scripts/qc.py
@@ -2,6 +2,7 @@
import sys
from ordereddict import OrderedDict
+from qapi import *
marker = "qc_declaration"
marked = False
@@ -468,6 +469,107 @@ def qapi_schema(node):
schema['data'] = data
print json.dumps(schema).replace("\"", "'")
+def vmstate_field_hacks(node, field):
+ # yes, this is gonna get hairy. may want to move to a seperate file at
+ # some point
+ push_indent()
+ vms_field=""
+ if node.has_key('typedef') and node['typedef'] == 'RTCState':
+ if field['type'] == 'struct tm':
+ vms_field += mcgen('''
+VMSTATE_INT32(%(variable)s.tm_sec, RTCState),
+VMSTATE_INT32(%(variable)s.tm_min, RTCState),
+VMSTATE_INT32(%(variable)s.tm_hour, RTCState),
+VMSTATE_INT32(%(variable)s.tm_wday, RTCState),
+VMSTATE_INT32(%(variable)s.tm_mday, RTCState),
+VMSTATE_INT32(%(variable)s.tm_mon, RTCState),
+VMSTATE_INT32(%(variable)s.tm_year, RTCState),
+''',
+ variable=field['variable']).rstrip()
+ pop_indent()
+ return vms_field
+
+def vmstate_fields(node):
+ fields = None
+ state_type = ""
+ vms_primitives = ['INT8', 'INT16', 'INT32', 'INT64', 'UINT8', 'UINT16',
+ 'UINT32', 'UINT64', 'BOOL', 'TIMER']
+ if node.has_key('typedef'):
+ fields = node['type']['fields']
+ state_type = node['typedef']
+ elif node.has_key('struct'):
+ fields = node['fields']
+ state_type = node['struct']
+ else:
+ raise Exception("top-level neither typedef nor struct")
+
+ print mcgen('''
+VMStateField vmstate_%s[] = {
+''' % state_type.lower()).rstrip()
+
+ for field in fields:
+ if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
+ continue
+
+ vms_field_hack = vmstate_field_hacks(node, field)
+ if vms_field_hack:
+ print vms_field_hack
+ continue
+
+ vms_field = "VMSTATE"
+
+ if field['type'].endswith('_t'):
+ vms_type = field['type'][:-2].upper()
+ elif field['type'] == 'int':
+ vms_type = 'INT32'
+ elif field['type'] == 'bool':
+ vms_type = 'BOOL'
+ elif field['type'] == 'QEMUTimer':
+ vms_type = 'TIMER'
+ else:
+ raise Exception("unable to process field:\n%s\nfrom node:\n%s" % (field, node))
+
+ if field.has_key('is_array'):
+ if field.has_key('array_capacity'):
+ vms_field += '_VARRAY'
+ else:
+ if vms_type == 'UINT8':
+ vms_field += '_BUFFER'
+ else:
+ vms_field += '_ARRAY'
+
+ if not vms_field.endswith("_BUFFER"):
+ vms_field += "_" + vms_type
+
+ if field.has_key('version'):
+ vms_field += "_V"
+ else:
+ field['version'] = "0"
+
+ if vms_field in map(lambda x: "VMSTATE_" + x, vms_primitives):
+ vms_field += "(%s, %s)," % (field['variable'], state_type)
+ elif vms_field in map(lambda x: "VMSTATE_" + x + "_V", vms_primitives):
+ vms_field += "(%s, %s, %s)," % (field['variable'], state_type,
+ field['version'])
+ elif vms_field == 'VMSTATE_BUFFER':
+ vms_field += "(%s, %s)," % (field['variable'], state_type)
+ elif vms_field in map(lambda x: "VMSTATE_VARRAY_" + x, vms_primitives):
+ args = ", ".join([field['variable'], state_type,
+ field['array_size'], field['version'],
+ "vmstate_info_" + field['type'].strip("_t"),
+ field['type']])
+ vms_field += "(%s)," % args
+ else:
+ raise Exception("unable to process field:\n%s\nfrom node:\n%s" % (field, node))
+
+ push_indent()
+ print cgen(vms_field).rstrip()
+ pop_indent()
+
+ print mcgen('''
+ VMSTATE_END_OF_LIST()
+};
+''',).rstrip()
if __name__ == '__main__':
la = LookAhead(skip(lexer(Input(sys.stdin))))
@@ -491,4 +593,7 @@ if __name__ == '__main__':
#qapi_format(node, True)
#qapi_format(node, False)
#type_dump(node)
- qapi_schema(node)
+ if '--vmstate' in sys.argv[1:]:
+ vmstate_fields(node)
+ else:
+ qapi_schema(node)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 15/17] Makefile: add qidl-generation of vmstate field descriptions
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (13 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 14/17] qidl: add qidl-based generation of vmstate field bindings Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc Michael Roth
2012-06-05 1:00 ` [Qemu-devel] [PATCH 17/17] rtc: use qidl-generated vmstate bindings Michael Roth
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
This will rebuild the vmstate descriptions in
$(SRC_DIR)/qidl-generated whenever we do a build where one of the
annotated sources has been modified. As a result, changes will be
reflected in the working tree so we can detect changes and commit
them in cases where the changes are correct/expected.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
Makefile.target | 17 +++++++++++++++--
1 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/Makefile.target b/Makefile.target
index 45e4459..0844401 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -22,7 +22,7 @@ QEMU_CFLAGS += -I.. -I$(TARGET_PATH) -DNEED_CPU_H
qidl-dir := $(SRC_PATH)/qidl-generated
qapi-dir := $(BUILD_DIR)/qapi-generated
-QEMU_CFLAGS += -I$(qapi-dir)
+QEMU_CFLAGS += -I$(qapi-dir) -I$(qidl-dir)
include $(SRC_PATH)/Makefile.objs
@@ -74,7 +74,7 @@ else
stap:
endif
-all: $(PROGS) stap
+all: $(PROGS) stap $(QIDL_VMSTATE)
# Dummy command so that make thinks it has done something
@true
@@ -109,6 +109,19 @@ $(QIDL_DEVICE_VISITORS): $(QIDL_DEVICE_VISITOR_SOURCES)
$(QIDL_DEVICES): $(QIDL_DEVICE_VISITORS)
#########################################################
+# QIDL-generated vmstate field descriptions
+QIDL_SOURCES_VMSTATE_SHORT=hw/mc146818rtc_state.h
+QIDL_SOURCES_VMSTATE=$(addprefix $(SRC_PATH)/, $(QIDL_SOURCES_VMSTATE_SHORT))
+QIDL_VMSTATE=$(patsubst %.h,$(qidl-dir)/%_vmstate.h, $(subst _state,, $(notdir $(QIDL_SOURCES_VMSTATE))))
+
+$(QIDL_VMSTATE) qidl-vmstate: $(QIDL_SOURCES_VMSTATE)
+ mkdir -p $(qidl-dir)
+ for f in $(QIDL_SOURCES_VMSTATE); do \
+ vmstate_filename=$(qidl-dir)/`basename $$(basename $$f .h) _state`_vmstate.h; \
+ $(PYTHON) $(SRC_PATH)/scripts/qc.py --vmstate < $$f >$$vmstate_filename; \
+ done
+
+#########################################################
# cpu emulator library
libobj-y = exec.o translate-all.o cpu-exec.o translate.o
libobj-y += tcg/tcg.o tcg/optimize.o
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (14 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 15/17] Makefile: add qidl-generation of vmstate field descriptions Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
2012-06-05 10:26 ` Avi Kivity
2012-06-05 1:00 ` [Qemu-devel] [PATCH 17/17] rtc: use qidl-generated vmstate bindings Michael Roth
16 siblings, 1 reply; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Initial check-in of the qidl-generated vmstate fields for rtc.
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
qidl-generated/mc146818rtc_vmstate.h | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
create mode 100644 qidl-generated/mc146818rtc_vmstate.h
diff --git a/qidl-generated/mc146818rtc_vmstate.h b/qidl-generated/mc146818rtc_vmstate.h
new file mode 100644
index 0000000..cc9dca9
--- /dev/null
+++ b/qidl-generated/mc146818rtc_vmstate.h
@@ -0,0 +1,20 @@
+VMStateField vmstate_rtcstate[] = {
+ VMSTATE_BUFFER(cmos_data, RTCState),
+ VMSTATE_UINT8(cmos_index, RTCState),
+ VMSTATE_INT32(current_tm.tm_sec, RTCState),
+ VMSTATE_INT32(current_tm.tm_min, RTCState),
+ VMSTATE_INT32(current_tm.tm_hour, RTCState),
+ VMSTATE_INT32(current_tm.tm_wday, RTCState),
+ VMSTATE_INT32(current_tm.tm_mday, RTCState),
+ VMSTATE_INT32(current_tm.tm_mon, RTCState),
+ VMSTATE_INT32(current_tm.tm_year, RTCState),
+ VMSTATE_INT32(base_year, RTCState),
+ VMSTATE_TIMER(periodic_timer, RTCState),
+ VMSTATE_INT64(next_periodic_time, RTCState),
+ VMSTATE_INT64(next_second_time, RTCState),
+ VMSTATE_UINT32_V(irq_coalesced, RTCState, 2),
+ VMSTATE_UINT32_V(period, RTCState, 2),
+ VMSTATE_TIMER(second_timer, RTCState),
+ VMSTATE_TIMER(second_timer2, RTCState),
+ VMSTATE_END_OF_LIST()
+};
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread* Re: [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc
2012-06-05 1:00 ` [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc Michael Roth
@ 2012-06-05 10:26 ` Avi Kivity
2012-06-05 23:38 ` Anthony Liguori
0 siblings, 1 reply; 81+ messages in thread
From: Avi Kivity @ 2012-06-05 10:26 UTC (permalink / raw)
To: Michael Roth
Cc: aliguori, yamahata, quintela, qemu-devel, owasserm, pbonzini,
akong, afaerber
On 06/05/2012 04:00 AM, Michael Roth wrote:
> Initial check-in of the qidl-generated vmstate fields for rtc.
>
Don't, please.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc
2012-06-05 10:26 ` Avi Kivity
@ 2012-06-05 23:38 ` Anthony Liguori
2012-06-06 7:47 ` Avi Kivity
0 siblings, 1 reply; 81+ messages in thread
From: Anthony Liguori @ 2012-06-05 23:38 UTC (permalink / raw)
To: Avi Kivity
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/05/2012 06:26 PM, Avi Kivity wrote:
> On 06/05/2012 04:00 AM, Michael Roth wrote:
>> Initial check-in of the qidl-generated vmstate fields for rtc.
>>
>
> Don't, please.
As Mike said in another note, we want to keep the device schema in the tree so
we can determine when it changes in 'make check' and complain loudly.
But I think we need a better way to do this. Something like a 'make
schema-update' target that will regenerate schemas and prepare them for checkin.
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 81+ messages in thread
* Re: [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc
2012-06-05 23:38 ` Anthony Liguori
@ 2012-06-06 7:47 ` Avi Kivity
0 siblings, 0 replies; 81+ messages in thread
From: Avi Kivity @ 2012-06-06 7:47 UTC (permalink / raw)
To: Anthony Liguori
Cc: owasserm, quintela, qemu-devel, Michael Roth, yamahata, pbonzini,
akong, afaerber
On 06/06/2012 02:38 AM, Anthony Liguori wrote:
> On 06/05/2012 06:26 PM, Avi Kivity wrote:
>> On 06/05/2012 04:00 AM, Michael Roth wrote:
>>> Initial check-in of the qidl-generated vmstate fields for rtc.
>>>
>>
>> Don't, please.
>
> As Mike said in another note, we want to keep the device schema in the
> tree so we can determine when it changes in 'make check' and complain
> loudly.
>
> But I think we need a better way to do this. Something like a 'make
> schema-update' target that will regenerate schemas and prepare them for
> checkin.
Yes, that's much better. Also would be good to pretty-print them so
that incremental changes are easy to spot using 'diff'.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 81+ messages in thread
* [Qemu-devel] [PATCH 17/17] rtc: use qidl-generated vmstate bindings
2012-06-05 1:00 [Qemu-devel] [RFC] Use QEMU IDL for device serialization/vmstate Michael Roth
` (15 preceding siblings ...)
2012-06-05 1:00 ` [Qemu-devel] [PATCH 16/17] qidl: add qidl-generated vmstate fields for rtc Michael Roth
@ 2012-06-05 1:00 ` Michael Roth
16 siblings, 0 replies; 81+ messages in thread
From: Michael Roth @ 2012-06-05 1:00 UTC (permalink / raw)
To: qemu-devel
Cc: aliguori, quintela, owasserm, yamahata, pbonzini, akong, afaerber
Make use of the qidl-generated vmstate bindings
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
hw/mc146818rtc.c | 21 ++-------------------
1 files changed, 2 insertions(+), 19 deletions(-)
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2dfc233..14a9b4d 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -27,6 +27,7 @@
#include "mc146818rtc.h"
#include "mc146818rtc_state.h"
#include "qapi-generated/mc146818rtc-qapi-visit.h"
+#include "mc146818rtc_vmstate.h"
#ifdef TARGET_I386
#include "apic.h"
@@ -505,25 +506,7 @@ static const VMStateDescription vmstate_rtc = {
.minimum_version_id = 1,
.minimum_version_id_old = 1,
.post_load = rtc_post_load,
- .fields = (VMStateField []) {
- VMSTATE_BUFFER(cmos_data, RTCState),
- VMSTATE_UINT8(cmos_index, RTCState),
- VMSTATE_INT32(current_tm.tm_sec, RTCState),
- VMSTATE_INT32(current_tm.tm_min, RTCState),
- VMSTATE_INT32(current_tm.tm_hour, RTCState),
- VMSTATE_INT32(current_tm.tm_wday, RTCState),
- VMSTATE_INT32(current_tm.tm_mday, RTCState),
- VMSTATE_INT32(current_tm.tm_mon, RTCState),
- VMSTATE_INT32(current_tm.tm_year, RTCState),
- VMSTATE_TIMER(periodic_timer, RTCState),
- VMSTATE_INT64(next_periodic_time, RTCState),
- VMSTATE_INT64(next_second_time, RTCState),
- VMSTATE_TIMER(second_timer, RTCState),
- VMSTATE_TIMER(second_timer2, RTCState),
- VMSTATE_UINT32_V(irq_coalesced, RTCState, 2),
- VMSTATE_UINT32_V(period, RTCState, 2),
- VMSTATE_END_OF_LIST()
- }
+ .fields = vmstate_rtcstate,
};
static void rtc_notify_clock_reset(Notifier *notifier, void *data)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 81+ messages in thread