[Qemu-devel] [PATCH v7 09/11] Dump: add qmp command "query-dump"

[Peter Xu]

When dump-guest-memory is requested with detach flag, after its
return, user could query its status using "query-dump" command (with
no argument). The result contains:

- status: current dump status
- completed: bytes written in the latest dump
- total: bytes to write in the latest dump

>From completed and total, we could know how much work
finished by calculating:

  100.0 * completed / total (%)

Signed-off-by: Peter Xu 
---
 dump.c   | 23 +++
 qapi-schema.json | 32 +++-
 qmp-commands.hx  | 27 ++-
 3 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/dump.c b/dump.c
index ca2400d..c697259 100644
--- a/dump.c
+++ b/dump.c
@@ -1456,7 +1456,7 @@ static void dump_state_prepare(DumpState *s)
 bool dump_in_progress(void)
 {
 DumpState *state = _state_global;
-return (state->status == DUMP_STATUS_ACTIVE);
+return (atomic_read(>status) == DUMP_STATUS_ACTIVE);
 }
 
 /* calculate total size of memory to be dumped (taking filter into
@@ -1669,9 +1669,12 @@ static void dump_process(DumpState *s, Error **errp)
 create_vmcore(s, _err);
 }
 
-s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
-error_propagate(errp, local_err);
+/* make sure status is written after written_size updates */
+smp_wmb();
+atomic_set(>status,
+   (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
 
+error_propagate(errp, local_err);
 dump_cleanup(s);
 }
 
@@ -1689,6 +1692,18 @@ static void *dump_thread(void *data)
 return NULL;
 }
 
+DumpQueryResult *qmp_query_dump(Error **errp)
+{
+DumpQueryResult *result = g_new(DumpQueryResult, 1);
+DumpState *state = _state_global;
+result->status = atomic_read(>status);
+/* make sure we are reading status and written_size in order */
+smp_rmb();
+result->completed = state->written_size;
+result->total = state->total_size;
+return result;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
bool has_detach, bool detach,
bool has_begin, int64_t begin, bool has_length,
@@ -1779,7 +1794,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
   begin, length, _err);
 if (local_err) {
 error_propagate(errp, local_err);
-s->status = DUMP_STATUS_FAILED;
+atomic_set(>status, DUMP_STATUS_FAILED);
 return;
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index ccd30c8..7b8f2a1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2196,7 +2196,8 @@
 #   is the fd's name.
 #
 # @detach: #optional if true, QMP will return immediately rather than
-#  waiting for the dump to finish. (since 2.6).
+#  waiting for the dump to finish. The user can track progress
+#  using "query-dump". (since 2.6).
 #
 # @begin: #optional if specified, the starting physical address.
 #
@@ -2237,6 +2238,35 @@
   'data': [ 'none', 'active', 'completed', 'failed' ] }
 
 ##
+# @DumpQueryResult
+#
+# The result format for 'query-dump'.
+#
+# @status: enum of @DumpStatus, which shows current dump status
+#
+# @completed: bytes written in latest dump (uncompressed)
+#
+# @total: total bytes to be written in latest dump (uncompressed)
+#
+# Since 2.6
+##
+{ 'struct': 'DumpQueryResult',
+  'data': { 'status': 'DumpStatus',
+'completed': 'int',
+'total': 'int' } }
+
+##
+# @query-dump
+#
+# Query latest dump status.
+#
+# Returns: A @DumpStatus object showing the dump status.
+#
+# Since: 2.6
+##
+{ 'command': 'query-dump', 'returns': 'DumpQueryResult' }
+
+##
 # @DumpGuestMemoryCapability:
 #
 # A list of the available formats for dump-guest-memory
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5dc7738..47351f3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -856,7 +856,8 @@ Arguments:
 - "protocol": destination file(started with "file:") or destination file
   descriptor (started with "fd:") (json-string)
 - "detach": if specified, command will return immediately, without waiting
-for the dump to finish (json-bool)
+for the dump to finish. The user can track progress using
+"query-dump". (json-bool)
 - "begin": the starting physical address. It's optional, and should be 
specified
with length together (json-int)
 - "length": the memory size, in bytes. It's optional, and should be specified
@@ -896,6 +897,30 @@ Example:
 
 EQMP
 
+{
+.name   = "query-dump",
+.args_type  = "",
+.params = "",
+.help   = "query background dump status",
+.mhandler.cmd_new = qmp_marshal_query_dump,
+},
+
+SQMP
+query-dump
+--
+
+Query background dump status.
+
+Arguments: None.
+
+Example:
+
+-> { "execute": "query-dump" }
+<- { "return": { "status": "active", "completed": 1024000,
+ 

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Neo Jia]

On Tue, Feb 16, 2016 at 07:40:47AM +, Tian, Kevin wrote:
> > From: Neo Jia [mailto:c...@nvidia.com]
> > Sent: Tuesday, February 16, 2016 3:37 PM
> > 
> > On Tue, Feb 16, 2016 at 07:27:09AM +, Tian, Kevin wrote:
> > > > From: Neo Jia [mailto:c...@nvidia.com]
> > > > Sent: Tuesday, February 16, 2016 3:13 PM
> > > >
> > > > On Tue, Feb 16, 2016 at 06:49:30AM +, Tian, Kevin wrote:
> > > > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > > > Sent: Thursday, February 04, 2016 3:33 AM
> > > > > >
> > > > > > On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> > > > > > >   Hi,
> > > > > > >
> > > > > > > > Actually I have a long puzzle in this area. Definitely libvirt 
> > > > > > > > will use UUID to
> > > > > > > > mark a VM. And obviously UUID is not recorded within KVM. Then 
> > > > > > > > how does
> > > > > > > > libvirt talk to KVM based on UUID? It could be a good reference 
> > > > > > > > to this design.
> > > > > > >
> > > > > > > libvirt keeps track which qemu instance belongs to which vm.
> > > > > > > qemu also gets started with "-uuid ...", so one can query qemu via
> > > > > > > monitor ("info uuid") to figure what the uuid is.  It is also in 
> > > > > > > the
> > > > > > > smbios tables so the guest can see it in the system information 
> > > > > > > table.
> > > > > > >
> > > > > > > The uuid is not visible to the kernel though, the kvm kernel 
> > > > > > > driver
> > > > > > > doesn't know what the uuid is (and neither does vfio).  qemu uses 
> > > > > > > file
> > > > > > > handles to talk to both kvm and vfio.  qemu notifies both kvm and 
> > > > > > > vfio
> > > > > > > about anything relevant events (guest address space changes etc) 
> > > > > > > and
> > > > > > > connects file descriptors (eventfd -> irqfd).
> > > > > >
> > > > > > I think the original link to using a VM UUID for the vGPU comes from
> > > > > > NVIDIA having a userspace component which might get launched from a 
> > > > > > udev
> > > > > > event as the vGPU is created or the set of vGPUs within that UUID is
> > > > > > started.  Using the VM UUID then gives them a way to associate that
> > > > > > userspace process with a VM instance.  Maybe it could register with
> > > > > > libvirt for some sort of service provided for the VM, I don't know.
> > > > >
> > > > > Intel doesn't have this requirement. It should be enough as long as
> > > > > libvirt maintains which sysfs vgpu node is associated to a VM UUID.
> > > > >
> > > > > >
> > > > > > > qemu needs a sysfs node as handle to the vfio device, something
> > > > > > > like /sys/devices/virtual/vgpu/.   can be a uuid if 
> > > > > > > you want
> > > > > > > have it that way, but it could be pretty much anything.  The 
> > > > > > > sysfs node
> > > > > > > will probably show up as-is in the libvirt xml when assign a vgpu 
> > > > > > > to a
> > > > > > > vm.  So the name should be something stable (i.e. when using a 
> > > > > > > uuid as
> > > > > > > name you should better not generate a new one on each boot).
> > > > > >
> > > > > > Actually I don't think there's really a persistent naming issue, 
> > > > > > that's
> > > > > > probably where we diverge from the SR-IOV model.  SR-IOV cannot
> > > > > > dynamically add a new VF, it needs to reset the number of VFs to 
> > > > > > zero,
> > > > > > then re-allocate all of them up to the new desired count.  That has 
> > > > > > some
> > > > > > obvious implications.  I think with both vendors here, we can
> > > > > > dynamically allocate new vGPUs, so I would expect that libvirt would
> > > > > > create each vGPU instance as it's needed.  None would be created by
> > > > > > default without user interaction.
> > > > > >
> > > > > > Personally I think using a UUID makes sense, but it needs to be
> > > > > > userspace policy whether that UUID has any implicit meaning like
> > > > > > matching the VM UUID.  Having an index within a UUID bothers me a 
> > > > > > bit,
> > > > > > but it doesn't seem like too much of a concession to enable the use 
> > > > > > case
> > > > > > that NVIDIA is trying to achieve.  Thanks,
> > > > > >
> > > > >
> > > > > I would prefer to making UUID an optional parameter, while not tieing
> > > > > sysfs vgpu naming to UUID. This would be more flexible to different
> > > > > scenarios where UUID might not be required.
> > > >
> > > > Hi Kevin,
> > > >
> > > > Happy Chinese New Year!
> > > >
> > > > I think having UUID as the vgpu device name will allow us to have an 
> > > > gpu vendor
> > > > agnostic solution for the upper layer software stack such as QEMU, who 
> > > > is
> > > > supposed to open the device.
> > > >
> > >
> > > Qemu can use whatever sysfs path provided to open the device, regardless
> > > of whether there is an UUID within the path...
> > >
> > 
> > Hi Kevin,
> > 
> > Then it will provide even more benefit of using UUID as libvirt can be
> > implemented as gpu vendor agnostic, right? :-)
> > 
> > The UUID can be VM UUID or vGPU group 

[Qemu-devel] [PATCH v7 08/11] DumpState: adding total_size and written_size fields

[Peter Xu]

Here, total_size is the size in bytes to be dumped (raw data, which
means before compression), while written_size are bytes handled (raw
size too).

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c| 32 
 include/sysemu/dump.h |  9 +
 2 files changed, 41 insertions(+)

diff --git a/dump.c b/dump.c
index 9210a72..ca2400d 100644
--- a/dump.c
+++ b/dump.c
@@ -331,6 +331,8 @@ static void write_data(DumpState *s, void *buf, int length, 
Error **errp)
 ret = fd_write_vmcore(buf, length, s);
 if (ret < 0) {
 error_setg(errp, "dump: failed to save memory");
+} else {
+s->written_size += length;
 }
 }
 
@@ -1324,6 +1326,7 @@ static void write_dump_pages(DumpState *s, Error **errp)
 goto out;
 }
 }
+s->written_size += TARGET_PAGE_SIZE;
 }
 
 ret = write_cache(_desc, NULL, 0, true);
@@ -1456,6 +1459,30 @@ bool dump_in_progress(void)
 return (state->status == DUMP_STATUS_ACTIVE);
 }
 
+/* calculate total size of memory to be dumped (taking filter into
+ * acoount.) */
+static int64_t dump_calculate_size(DumpState *s)
+{
+GuestPhysBlock *block;
+int64_t size = 0, total = 0, left = 0, right = 0;
+
+QTAILQ_FOREACH(block, >guest_phys_blocks.head, next) {
+if (s->has_filter) {
+/* calculate the overlapped region. */
+left = MAX(s->begin, block->target_start);
+right = MIN(s->begin + s->length, block->target_end);
+size = right - left;
+size = size > 0 ? size : 0;
+} else {
+/* count the whole region in */
+size = (block->target_end - block->target_start);
+}
+total += size;
+}
+
+return total;
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
   DumpGuestMemoryFormat format, bool paging, bool 
has_filter,
   int64_t begin, int64_t length, Error **errp)
@@ -1467,6 +1494,7 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 
 s->has_format = has_format;
 s->format = format;
+s->written_size = 0;
 
 /* kdump-compressed is conflict with paging and filter */
 if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
@@ -1498,6 +1526,10 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 
 guest_phys_blocks_init(>guest_phys_blocks);
 guest_phys_blocks_append(>guest_phys_blocks);
+s->total_size = dump_calculate_size(s);
+#ifdef DEBUG_DUMP_GUEST_MEMORY
+fprintf(stderr, "DUMP: total memory to dump: %lu\n", s->total_size);
+#endif
 
 s->start = get_start_block(s);
 if (s->start == -1) {
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 06393c3..ef931be 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -182,6 +182,15 @@ typedef struct DumpState {
 bool has_format;  /* whether format is provided */
 DumpGuestMemoryFormat format; /* valid only if has_format == true */
 QemuThread dump_thread;   /* thread for detached dump */
+
+int64_t total_size;  /* total memory size (in bytes) to
+  * be dumped. When filter is
+  * enabled, this will only count
+  * those to be written. */
+int64_t written_size;/* written memory size (in bytes),
+  * this could be used to calculate
+  * how much work we have
+  * finished. */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
-- 
2.4.3

[Qemu-devel] [PATCH v7 07/11] dump-guest-memory: add "detach" support

[Peter Xu]

If "detach" is provided, one thread is created to do the dump work,
while main thread will return immediately. For each GuestPhysBlock,
adding one more field "mr" to points to MemoryRegion that it
belongs, also ref the mr before use.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c  | 27 ++-
 include/sysemu/dump.h   |  1 +
 include/sysemu/memory_mapping.h |  4 
 memory_mapping.c|  3 +++
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/dump.c b/dump.c
index 923e3a5..9210a72 100644
--- a/dump.c
+++ b/dump.c
@@ -1643,6 +1643,20 @@ static void dump_process(DumpState *s, Error **errp)
 dump_cleanup(s);
 }
 
+static void *dump_thread(void *data)
+{
+Error *err = NULL;
+DumpState *s = (DumpState *)data;
+
+dump_process(s, );
+
+if (err) {
+/* TODO: notify user the error */
+error_free(err);
+}
+return NULL;
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
bool has_detach, bool detach,
bool has_begin, int64_t begin, bool has_length,
@@ -1653,6 +1667,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 int fd = -1;
 DumpState *s;
 Error *local_err = NULL;
+bool detach_p = false;
 
 if (runstate_check(RUN_STATE_INMIGRATE)) {
 error_setg(errp, "Dump not allowed during incoming migration.");
@@ -1684,6 +1699,9 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 error_setg(errp, QERR_MISSING_PARAMETER, "begin");
 return;
 }
+if (has_detach) {
+detach_p = detach;
+}
 
 /* check whether lzo/snappy is supported */
 #ifndef CONFIG_LZO
@@ -1733,7 +1751,14 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 return;
 }
 
-dump_process(s, errp);
+if (detach_p) {
+/* detached dump */
+qemu_thread_create(>dump_thread, "dump_thread", dump_thread,
+   s, QEMU_THREAD_DETACHED);
+} else {
+/* sync dump */
+dump_process(s, errp);
+}
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 1da3ddb..06393c3 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -181,6 +181,7 @@ typedef struct DumpState {
 
 bool has_format;  /* whether format is provided */
 DumpGuestMemoryFormat format; /* valid only if has_format == true */
+QemuThread dump_thread;   /* thread for detached dump */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/include/sysemu/memory_mapping.h b/include/sysemu/memory_mapping.h
index a75d59a..d46d879 100644
--- a/include/sysemu/memory_mapping.h
+++ b/include/sysemu/memory_mapping.h
@@ -16,6 +16,7 @@
 
 #include "qemu/queue.h"
 #include "qemu/typedefs.h"
+#include "exec/memory.h"
 
 typedef struct GuestPhysBlock {
 /* visible to guest, reflects PCI hole, etc */
@@ -27,6 +28,9 @@ typedef struct GuestPhysBlock {
 /* points into host memory */
 uint8_t *host_addr;
 
+/* points to the MemoryRegion that this block belongs to */
+MemoryRegion *mr;
+
 QTAILQ_ENTRY(GuestPhysBlock) next;
 } GuestPhysBlock;
 
diff --git a/memory_mapping.c b/memory_mapping.c
index 04db3ac..c8855de 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -178,6 +178,7 @@ void guest_phys_blocks_free(GuestPhysBlockList *list)
 
 QTAILQ_FOREACH_SAFE(p, >head, next, q) {
 QTAILQ_REMOVE(>head, p, next);
+memory_region_unref(p->mr);
 g_free(p);
 }
 list->num = 0;
@@ -241,6 +242,8 @@ static void guest_phys_blocks_region_add(MemoryListener 
*listener,
 block->target_start = target_start;
 block->target_end   = target_end;
 block->host_addr= host_addr;
+block->mr   = section->mr;
+memory_region_ref(section->mr);
 
 QTAILQ_INSERT_TAIL(>list->head, block, next);
 ++g->list->num;
-- 
2.4.3

[Qemu-devel] [PATCH v7 11/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Peter Xu]

One new QMP event DUMP_COMPLETED is added. When a dump finishes, one
DUMP_COMPLETED event will occur to notify the user.

Signed-off-by: Peter Xu 
Reviewed-by:   Fam Zheng 
---
 docs/qmp-events.txt | 18 ++
 dump.c  | 18 --
 qapi/event.json | 16 
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index 52eb7e2..4e3eb9e 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -220,6 +220,24 @@ Data:
   },
   "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 
+DUMP_COMPLETED
+--
+
+Emitted when the guest has finished one memory dump.
+
+Data:
+
+- "result": DumpQueryResult type described in qapi-schema.json
+- "error": Error message when dump failed. This is only a
+  human-readable string provided when dump failed. It should not be
+  parsed in any way (json-string, optional)
+
+Example:
+
+{ "event": "DUMP_COMPLETED",
+  "data": {"result": {"total": 1090650112, "status": "completed",
+  "completed": 1090650112} } }
+
 GUEST_PANICKED
 --
 
diff --git a/dump.c b/dump.c
index c697259..a5c40e6 100644
--- a/dump.c
+++ b/dump.c
@@ -25,6 +25,7 @@
 #include "sysemu/cpus.h"
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
+#include "qapi-event.h"
 
 #include 
 #ifdef CONFIG_LZO
@@ -1662,6 +1663,7 @@ cleanup:
 static void dump_process(DumpState *s, Error **errp)
 {
 Error *local_err = NULL;
+DumpQueryResult *result = NULL;
 
 if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
 create_kdump_vmcore(s, _err);
@@ -1674,6 +1676,15 @@ static void dump_process(DumpState *s, Error **errp)
 atomic_set(>status,
(local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
 
+/* send DUMP_COMPLETED message (unconditionally) */
+result = qmp_query_dump(NULL);
+/* should never fail */
+assert(result);
+qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
+   error_get_pretty(local_err) : NULL),
+   _abort);
+qapi_free_DumpQueryResult(result);
+
 error_propagate(errp, local_err);
 dump_cleanup(s);
 }
@@ -1682,13 +1693,8 @@ static void *dump_thread(void *data)
 {
 Error *err = NULL;
 DumpState *s = (DumpState *)data;
-
 dump_process(s, );
-
-if (err) {
-/* TODO: notify user the error */
-error_free(err);
-}
+error_free(err);
 return NULL;
 }
 
diff --git a/qapi/event.json b/qapi/event.json
index 390fd45..1a45a6c 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -369,3 +369,19 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @DUMP_COMPLETED
+#
+# Emitted when background dump has completed
+#
+# @result: DumpQueryResult type described in qapi-schema.json.
+#
+# @error: #optional human-readable error string that provides
+# hint on why dump failed. Only presents on failure. The
+# user should not try to interpret the error string.
+#
+# Since: 2.6
+##
+{ 'event': 'DUMP_COMPLETED' ,
+  'data': { 'result': 'DumpQueryResult', '*error': 'str' } }
-- 
2.4.3

[Qemu-devel] [PATCH v7 10/11] Dump: add hmp command "info dump"

[Peter Xu]

It will calculate percentage of finished work from completed and
total.

Signed-off-by: Peter Xu 
---
 hmp-commands-info.hx | 14 ++
 hmp.c| 17 +
 hmp.h|  1 +
 3 files changed, 32 insertions(+)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 9b71351..52539c3 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -786,6 +786,20 @@ STEXI
 Display the value of a storage key (s390 only)
 ETEXI
 
+{
+.name   = "dump",
+.args_type  = "",
+.params = "",
+.help   = "Display the latest dump status",
+.mhandler.cmd = hmp_info_dump,
+},
+
+STEXI
+@item info dump
+@findex dump
+Display the latest dump status.
+ETEXI
+
 STEXI
 @end table
 ETEXI
diff --git a/hmp.c b/hmp.c
index 3a0d9d4..92cf014 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2381,3 +2381,20 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict 
*qdict)
 
 qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_info_dump(Monitor *mon, const QDict *qdict)
+{
+DumpQueryResult *result = qmp_query_dump(NULL);
+
+assert(result && result->status < DUMP_STATUS_MAX);
+monitor_printf(mon, "Status: %s\n", DumpStatus_lookup[result->status]);
+
+if (result->status == DUMP_STATUS_ACTIVE) {
+float percent = 0;
+assert(result->total != 0);
+percent = 100.0 * result->completed / result->total;
+monitor_printf(mon, "Finished: %.2f %%\n", percent);
+}
+
+qapi_free_DumpQueryResult(result);
+}
diff --git a/hmp.h b/hmp.h
index a8c5b5a..093d65f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -131,5 +131,6 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
 void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
 void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
+void hmp_info_dump(Monitor *mon, const QDict *qdict);
 
 #endif
-- 
2.4.3

[Qemu-devel] [PATCH v7 06/11] dump-guest-memory: disable dump when in INMIGRATE state

[Peter Xu]

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/dump.c b/dump.c
index fed84a6..923e3a5 100644
--- a/dump.c
+++ b/dump.c
@@ -1654,6 +1654,11 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 DumpState *s;
 Error *local_err = NULL;
 
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+error_setg(errp, "Dump not allowed during incoming migration.");
+return;
+}
+
 /* if there is a dump in background, we should wait until the dump
  * finished */
 if (dump_in_progress()) {
-- 
2.4.3

[Qemu-devel] [PATCH v7 05/11] dump-guest-memory: introduce dump_process() helper function.

[Peter Xu]

No functional change. Cleanup only.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c| 31 +--
 include/sysemu/dump.h |  3 +++
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index 158d6ea..fed84a6 100644
--- a/dump.c
+++ b/dump.c
@@ -1465,6 +1465,9 @@ static void dump_init(DumpState *s, int fd, bool 
has_format,
 Error *err = NULL;
 int ret;
 
+s->has_format = has_format;
+s->format = format;
+
 /* kdump-compressed is conflict with paging and filter */
 if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
 assert(!paging && !has_filter);
@@ -1623,6 +1626,23 @@ cleanup:
 dump_cleanup(s);
 }
 
+/* this operation might be time consuming. */
+static void dump_process(DumpState *s, Error **errp)
+{
+Error *local_err = NULL;
+
+if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
+create_kdump_vmcore(s, _err);
+} else {
+create_vmcore(s, _err);
+}
+
+s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
+error_propagate(errp, local_err);
+
+dump_cleanup(s);
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
bool has_detach, bool detach,
bool has_begin, int64_t begin, bool has_length,
@@ -1708,16 +1728,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 return;
 }
 
-if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-create_kdump_vmcore(s, _err);
-} else {
-create_vmcore(s, _err);
-}
-
-s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
-error_propagate(errp, local_err);
-
-dump_cleanup(s);
+dump_process(s, errp);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 21fc02d..1da3ddb 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -178,6 +178,9 @@ typedef struct DumpState {
 size_t num_dumpable;/* number of page that can be dumped */
 uint32_t flag_compress; /* indicate the compression format */
 DumpStatus status;  /* current dump status */
+
+bool has_format;  /* whether format is provided */
+DumpGuestMemoryFormat format; /* valid only if has_format == true */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
-- 
2.4.3

[Qemu-devel] [PATCH v7 02/11] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Peter Xu]

This patch only adds the interfaces, but does not implement them.
"detach" parameter is made optional, to make sure that all the old
dump-guest-memory requests will still be able to work.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c   | 5 +++--
 hmp-commands.hx  | 5 +++--
 hmp.c| 9 +++--
 qapi-schema.json | 8 ++--
 qmp-commands.hx  | 6 --
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index 769c5f9..c4a62d9 100644
--- a/dump.c
+++ b/dump.c
@@ -1609,8 +1609,9 @@ cleanup:
 dump_cleanup(s);
 }
 
-void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
-   int64_t begin, bool has_length,
+void qmp_dump_guest_memory(bool paging, const char *file,
+   bool has_detach, bool detach,
+   bool has_begin, int64_t begin, bool has_length,
int64_t length, bool has_format,
DumpGuestMemoryFormat format, Error **errp)
 {
diff --git a/hmp-commands.hx b/hmp-commands.hx
index bb52e4d..664d794 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1056,10 +1056,11 @@ ETEXI
 
 {
 .name   = "dump-guest-memory",
-.args_type  = 
"paging:-p,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
-.params = "[-p] [-z|-l|-s] filename [begin length]",
+.args_type  = 
"paging:-p,detach:-d,zlib:-z,lzo:-l,snappy:-s,filename:F,begin:i?,length:i?",
+.params = "[-p] [-d] [-z|-l|-s] filename [begin length]",
 .help   = "dump guest memory into file 'filename'.\n\t\t\t"
   "-p: do paging to get guest's memory mapping.\n\t\t\t"
+  "-d: return immediately (do not wait for 
completion).\n\t\t\t"
   "-z: dump in kdump-compressed format, with zlib 
compression.\n\t\t\t"
   "-l: dump in kdump-compressed format, with lzo 
compression.\n\t\t\t"
   "-s: dump in kdump-compressed format, with snappy 
compression.\n\t\t\t"
diff --git a/hmp.c b/hmp.c
index c6419da..3a0d9d4 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 const char *file = qdict_get_str(qdict, "filename");
 bool has_begin = qdict_haskey(qdict, "begin");
 bool has_length = qdict_haskey(qdict, "length");
+bool has_detach = qdict_haskey(qdict, "detach");
 int64_t begin = 0;
 int64_t length = 0;
+bool detach = false;
 enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
 char *prot;
 
@@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict 
*qdict)
 if (has_length) {
 length = qdict_get_int(qdict, "length");
 }
+if (has_detach) {
+detach = qdict_get_bool(qdict, "detach");
+}
 
 prot = g_strconcat("file:", file, NULL);
 
-qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length, length,
-  true, dump_format, );
+qmp_dump_guest_memory(paging, prot, true, detach, has_begin, begin,
+  has_length, length, true, dump_format, );
 hmp_handle_error(mon, );
 g_free(prot);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 8d04897..caff580 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2195,6 +2195,9 @@
 #2. fd: the protocol starts with "fd:", and the following string
 #   is the fd's name.
 #
+# @detach: #optional if true, QMP will return immediately rather than
+#  waiting for the dump to finish. (since 2.6).
+#
 # @begin: #optional if specified, the starting physical address.
 #
 # @length: #optional if specified, the memory size, in bytes. If you don't
@@ -2211,8 +2214,9 @@
 # Since: 1.2
 ##
 { 'command': 'dump-guest-memory',
-  'data': { 'paging': 'bool', 'protocol': 'str', '*begin': 'int',
-'*length': 'int', '*format': 'DumpGuestMemoryFormat' } }
+  'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
+'*begin': 'int', '*length': 'int',
+'*format': 'DumpGuestMemoryFormat'} }
 
 ##
 # @DumpGuestMemoryCapability:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 020e5ee..5dc7738 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -838,8 +838,8 @@ EQMP
 
 {
 .name   = "dump-guest-memory",
-.args_type  = "paging:b,protocol:s,begin:i?,end:i?,format:s?",
-.params = "-p protocol [begin] [length] [format]",
+.args_type  = 
"paging:b,protocol:s,detach:b?,begin:i?,end:i?,format:s?",
+.params = "-p protocol [-d] [begin] [length] [format]",
 .help   = "dump guest memory to file",
 .mhandler.cmd_new = qmp_marshal_dump_guest_memory,
 },
@@ -855,6 +855,8 @@ Arguments:
 - "paging": do paging to get guest's memory mapping (json-bool)
 - "protocol": 

[Qemu-devel] [PATCH v7 03/11] dump-guest-memory: using static DumpState, add DumpStatus

[Peter Xu]

Instead of malloc/free each time for DumpState, make it
static. Added DumpStatus to show status for dump.

This is to be used for detached dump.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c| 21 -
 include/sysemu/dump.h |  2 ++
 qapi-schema.json  | 18 ++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/dump.c b/dump.c
index c4a62d9..434bc60 100644
--- a/dump.c
+++ b/dump.c
@@ -1442,6 +1442,14 @@ static void get_max_mapnr(DumpState *s)
 s->max_mapnr = dump_paddr_to_pfn(s, last_block->target_end);
 }
 
+static DumpState dump_state_global = { .status = DUMP_STATUS_NONE };
+
+static void dump_state_prepare(DumpState *s)
+{
+/* zero the struct, setting status to active */
+*s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
   DumpGuestMemoryFormat format, bool paging, bool 
has_filter,
   int64_t begin, int64_t length, Error **errp)
@@ -1676,24 +1684,27 @@ void qmp_dump_guest_memory(bool paging, const char 
*file,
 return;
 }
 
-s = g_malloc0(sizeof(DumpState));
+s = _state_global;
+dump_state_prepare(s);
 
 dump_init(s, fd, has_format, format, paging, has_begin,
   begin, length, _err);
 if (local_err) {
-g_free(s);
 error_propagate(errp, local_err);
+s->status = DUMP_STATUS_FAILED;
 return;
 }
 
 if (has_format && format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
-create_kdump_vmcore(s, errp);
+create_kdump_vmcore(s, _err);
 } else {
-create_vmcore(s, errp);
+create_vmcore(s, _err);
 }
 
+s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
+error_propagate(errp, local_err);
+
 dump_cleanup(s);
-g_free(s);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 2f04b24..21fc02d 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -38,6 +38,7 @@
 
 #include "sysemu/dump-arch.h"
 #include "sysemu/memory_mapping.h"
+#include "qapi-types.h"
 
 typedef struct QEMU_PACKED MakedumpfileHeader {
 char signature[16]; /* = "makedumpfile" */
@@ -176,6 +177,7 @@ typedef struct DumpState {
 off_t offset_page;  /* offset of page part in vmcore */
 size_t num_dumpable;/* number of page that can be dumped */
 uint32_t flag_compress; /* indicate the compression format */
+DumpStatus status;  /* current dump status */
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/qapi-schema.json b/qapi-schema.json
index caff580..ccd30c8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2219,6 +2219,24 @@
 '*format': 'DumpGuestMemoryFormat'} }
 
 ##
+# @DumpStatus
+#
+# Describe the status of a long-running background guest memory dump.
+#
+# @none: no dump-guest-memory has started yet.
+#
+# @active: there is one dump running in background.
+#
+# @completed: the last dump has finished successfully.
+#
+# @failed: the last dump has failed.
+#
+# Since 2.6
+##
+{ 'enum': 'DumpStatus',
+  'data': [ 'none', 'active', 'completed', 'failed' ] }
+
+##
 # @DumpGuestMemoryCapability:
 #
 # A list of the available formats for dump-guest-memory
-- 
2.4.3

[Qemu-devel] [PATCH v7 04/11] dump-guest-memory: add dump_in_progress() helper function

[Peter Xu]

For now, it has no effect. It will be used in dump detach support.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c| 13 +
 include/qemu-common.h |  4 
 qmp.c | 14 ++
 3 files changed, 31 insertions(+)

diff --git a/dump.c b/dump.c
index 434bc60..158d6ea 100644
--- a/dump.c
+++ b/dump.c
@@ -1450,6 +1450,12 @@ static void dump_state_prepare(DumpState *s)
 *s = (DumpState) { .status = DUMP_STATUS_ACTIVE };
 }
 
+bool dump_in_progress(void)
+{
+DumpState *state = _state_global;
+return (state->status == DUMP_STATUS_ACTIVE);
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
   DumpGuestMemoryFormat format, bool paging, bool 
has_filter,
   int64_t begin, int64_t length, Error **errp)
@@ -1628,6 +1634,13 @@ void qmp_dump_guest_memory(bool paging, const char *file,
 DumpState *s;
 Error *local_err = NULL;
 
+/* if there is a dump in background, we should wait until the dump
+ * finished */
+if (dump_in_progress()) {
+error_setg(errp, "There is a dump in process, please wait.");
+return;
+}
+
 /*
  * kdump-compressed format need the whole memory dumped, so paging or
  * filter is not supported here.
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f557be7..59ab759 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -494,4 +494,8 @@ int parse_debug_env(const char *name, int max, int initial);
 const char *qemu_ether_ntoa(const MACAddr *mac);
 void page_size_init(void);
 
+/* returns non-zero if dump is in progress, otherwise zero is
+ * returned. */
+bool dump_in_progress(void);
+
 #endif
diff --git a/qmp.c b/qmp.c
index 6ae7230..f4caf5a 100644
--- a/qmp.c
+++ b/qmp.c
@@ -103,6 +103,13 @@ void qmp_quit(Error **errp)
 
 void qmp_stop(Error **errp)
 {
+/* if there is a dump in background, we should wait until the dump
+ * finished */
+if (dump_in_progress()) {
+error_setg(errp, "There is a dump in process, please wait.");
+return;
+}
+
 if (runstate_check(RUN_STATE_INMIGRATE)) {
 autostart = 0;
 } else {
@@ -175,6 +182,13 @@ void qmp_cont(Error **errp)
 BlockBackend *blk;
 BlockDriverState *bs;
 
+/* if there is a dump in background, we should wait until the dump
+ * finished */
+if (dump_in_progress()) {
+error_setg(errp, "There is a dump in process, please wait.");
+return;
+}
+
 if (runstate_needs_reset()) {
 error_setg(errp, "Resetting the Virtual Machine is required");
 return;
-- 
2.4.3

[Qemu-devel] [PATCH v7 00/11] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Changes from v6:
- patch 9: fix leak of local_err due to patch switch.. [Fam]
- patch 10: assert "result" before use [Fam]
- patch 11: add Fam's reviewed-by.

For older patch, please refers to v6 series:

https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg01299.html

Peter Xu (11):
  dump-guest-memory: cleanup: removing dump_{error|cleanup}().
  dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
  dump-guest-memory: using static DumpState, add DumpStatus
  dump-guest-memory: add dump_in_progress() helper function
  dump-guest-memory: introduce dump_process() helper function.
  dump-guest-memory: disable dump when in INMIGRATE state
  dump-guest-memory: add "detach" support
  DumpState: adding total_size and written_size fields
  Dump: add qmp command "query-dump"
  Dump: add hmp command "info dump"
  dump-guest-memory: add qmp event DUMP_COMPLETED

 docs/qmp-events.txt |  18 
 dump.c  | 215 ++--
 hmp-commands-info.hx|  14 +++
 hmp-commands.hx |   5 +-
 hmp.c   |  26 -
 hmp.h   |   1 +
 include/qemu-common.h   |   4 +
 include/sysemu/dump.h   |  15 +++
 include/sysemu/memory_mapping.h |   4 +
 memory_mapping.c|   3 +
 qapi-schema.json|  56 ++-
 qapi/event.json |  16 +++
 qmp-commands.hx |  31 +-
 qmp.c   |  14 +++
 14 files changed, 359 insertions(+), 63 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v7 01/11] dump-guest-memory: cleanup: removing dump_{error|cleanup}().

[Peter Xu]

It might be a little bit confusing and error prone to do
dump_cleanup() in these two functions. A better way is to do
dump_cleanup() before dump finish, no matter whether dump has
succeeded or not.

Signed-off-by: Peter Xu 
Reviewed-by: Fam Zheng 
---
 dump.c | 78 +++---
 1 file changed, 32 insertions(+), 46 deletions(-)

diff --git a/dump.c b/dump.c
index 96e1fc1..769c5f9 100644
--- a/dump.c
+++ b/dump.c
@@ -82,12 +82,6 @@ static int dump_cleanup(DumpState *s)
 return 0;
 }
 
-static void dump_error(DumpState *s, const char *reason, Error **errp)
-{
-dump_cleanup(s);
-error_setg(errp, "%s", reason);
-}
-
 static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
 {
 DumpState *s = opaque;
@@ -128,7 +122,7 @@ static void write_elf64_header(DumpState *s, Error **errp)
 
 ret = fd_write_vmcore(_header, sizeof(elf_header), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write elf header", errp);
+error_setg(errp, "dump: failed to write elf header");
 }
 }
 
@@ -159,7 +153,7 @@ static void write_elf32_header(DumpState *s, Error **errp)
 
 ret = fd_write_vmcore(_header, sizeof(elf_header), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write elf header", errp);
+error_setg(errp, "dump: failed to write elf header");
 }
 }
 
@@ -182,7 +176,7 @@ static void write_elf64_load(DumpState *s, MemoryMapping 
*memory_mapping,
 
 ret = fd_write_vmcore(, sizeof(Elf64_Phdr), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write program header table", errp);
+error_setg(errp, "dump: failed to write program header table");
 }
 }
 
@@ -205,7 +199,7 @@ static void write_elf32_load(DumpState *s, MemoryMapping 
*memory_mapping,
 
 ret = fd_write_vmcore(, sizeof(Elf32_Phdr), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write program header table", errp);
+error_setg(errp, "dump: failed to write program header table");
 }
 }
 
@@ -225,7 +219,7 @@ static void write_elf64_note(DumpState *s, Error **errp)
 
 ret = fd_write_vmcore(, sizeof(Elf64_Phdr), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write program header table", errp);
+error_setg(errp, "dump: failed to write program header table");
 }
 }
 
@@ -245,7 +239,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
DumpState *s,
 id = cpu_index(cpu);
 ret = cpu_write_elf64_note(f, cpu, id, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write elf notes", errp);
+error_setg(errp, "dump: failed to write elf notes");
 return;
 }
 }
@@ -253,7 +247,7 @@ static void write_elf64_notes(WriteCoreDumpFunction f, 
DumpState *s,
 CPU_FOREACH(cpu) {
 ret = cpu_write_elf64_qemunote(f, cpu, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write CPU status", errp);
+error_setg(errp, "dump: failed to write CPU status");
 return;
 }
 }
@@ -275,7 +269,7 @@ static void write_elf32_note(DumpState *s, Error **errp)
 
 ret = fd_write_vmcore(, sizeof(Elf32_Phdr), s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write program header table", errp);
+error_setg(errp, "dump: failed to write program header table");
 }
 }
 
@@ -290,7 +284,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
DumpState *s,
 id = cpu_index(cpu);
 ret = cpu_write_elf32_note(f, cpu, id, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write elf notes", errp);
+error_setg(errp, "dump: failed to write elf notes");
 return;
 }
 }
@@ -298,7 +292,7 @@ static void write_elf32_notes(WriteCoreDumpFunction f, 
DumpState *s,
 CPU_FOREACH(cpu) {
 ret = cpu_write_elf32_qemunote(f, cpu, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write CPU status", errp);
+error_setg(errp, "dump: failed to write CPU status");
 return;
 }
 }
@@ -326,7 +320,7 @@ static void write_elf_section(DumpState *s, int type, Error 
**errp)
 
 ret = fd_write_vmcore(, shdr_size, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to write section header table", errp);
+error_setg(errp, "dump: failed to write section header table");
 }
 }
 
@@ -336,7 +330,7 @@ static void write_data(DumpState *s, void *buf, int length, 
Error **errp)
 
 ret = fd_write_vmcore(buf, length, s);
 if (ret < 0) {
-dump_error(s, "dump: failed to save memory", errp);
+error_setg(errp, "dump: failed to save memory");
 }
 }
 
@@ -568,11 +562,6 @@ static void dump_begin(DumpState *s, Error **errp)
 }
 }
 
-static void dump_completed(DumpState *s)
-{
-dump_cleanup(s);
-}
-
 static int get_next_block(DumpState *s, 

Re: [Qemu-devel] [PULL v2 22/45] ipmi: introduce a struct ipmi_sdr_compact

[Paolo Bonzini]

On 06/02/2016 20:13, Michael S. Tsirkin wrote:
>  
> -if (sdr[7] > MAX_SENSORS) {
> +if (sdr->sensor_owner_number > MAX_SENSORS) {

This is another off-by-one, it should have been >=.  Same for all these
occurrences later in the same file:

hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||
hw/ipmi/ipmi_bmc_sim.c:if ((cmd[2] > MAX_SENSORS) ||

Thanks,

Paolo

>  continue;
>  }

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Tian, Kevin]

> From: Neo Jia [mailto:c...@nvidia.com]
> Sent: Tuesday, February 16, 2016 3:37 PM
> 
> On Tue, Feb 16, 2016 at 07:27:09AM +, Tian, Kevin wrote:
> > > From: Neo Jia [mailto:c...@nvidia.com]
> > > Sent: Tuesday, February 16, 2016 3:13 PM
> > >
> > > On Tue, Feb 16, 2016 at 06:49:30AM +, Tian, Kevin wrote:
> > > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > > Sent: Thursday, February 04, 2016 3:33 AM
> > > > >
> > > > > On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> > > > > >   Hi,
> > > > > >
> > > > > > > Actually I have a long puzzle in this area. Definitely libvirt 
> > > > > > > will use UUID to
> > > > > > > mark a VM. And obviously UUID is not recorded within KVM. Then 
> > > > > > > how does
> > > > > > > libvirt talk to KVM based on UUID? It could be a good reference 
> > > > > > > to this design.
> > > > > >
> > > > > > libvirt keeps track which qemu instance belongs to which vm.
> > > > > > qemu also gets started with "-uuid ...", so one can query qemu via
> > > > > > monitor ("info uuid") to figure what the uuid is.  It is also in the
> > > > > > smbios tables so the guest can see it in the system information 
> > > > > > table.
> > > > > >
> > > > > > The uuid is not visible to the kernel though, the kvm kernel driver
> > > > > > doesn't know what the uuid is (and neither does vfio).  qemu uses 
> > > > > > file
> > > > > > handles to talk to both kvm and vfio.  qemu notifies both kvm and 
> > > > > > vfio
> > > > > > about anything relevant events (guest address space changes etc) and
> > > > > > connects file descriptors (eventfd -> irqfd).
> > > > >
> > > > > I think the original link to using a VM UUID for the vGPU comes from
> > > > > NVIDIA having a userspace component which might get launched from a 
> > > > > udev
> > > > > event as the vGPU is created or the set of vGPUs within that UUID is
> > > > > started.  Using the VM UUID then gives them a way to associate that
> > > > > userspace process with a VM instance.  Maybe it could register with
> > > > > libvirt for some sort of service provided for the VM, I don't know.
> > > >
> > > > Intel doesn't have this requirement. It should be enough as long as
> > > > libvirt maintains which sysfs vgpu node is associated to a VM UUID.
> > > >
> > > > >
> > > > > > qemu needs a sysfs node as handle to the vfio device, something
> > > > > > like /sys/devices/virtual/vgpu/.   can be a uuid if you 
> > > > > > want
> > > > > > have it that way, but it could be pretty much anything.  The sysfs 
> > > > > > node
> > > > > > will probably show up as-is in the libvirt xml when assign a vgpu 
> > > > > > to a
> > > > > > vm.  So the name should be something stable (i.e. when using a uuid 
> > > > > > as
> > > > > > name you should better not generate a new one on each boot).
> > > > >
> > > > > Actually I don't think there's really a persistent naming issue, 
> > > > > that's
> > > > > probably where we diverge from the SR-IOV model.  SR-IOV cannot
> > > > > dynamically add a new VF, it needs to reset the number of VFs to zero,
> > > > > then re-allocate all of them up to the new desired count.  That has 
> > > > > some
> > > > > obvious implications.  I think with both vendors here, we can
> > > > > dynamically allocate new vGPUs, so I would expect that libvirt would
> > > > > create each vGPU instance as it's needed.  None would be created by
> > > > > default without user interaction.
> > > > >
> > > > > Personally I think using a UUID makes sense, but it needs to be
> > > > > userspace policy whether that UUID has any implicit meaning like
> > > > > matching the VM UUID.  Having an index within a UUID bothers me a bit,
> > > > > but it doesn't seem like too much of a concession to enable the use 
> > > > > case
> > > > > that NVIDIA is trying to achieve.  Thanks,
> > > > >
> > > >
> > > > I would prefer to making UUID an optional parameter, while not tieing
> > > > sysfs vgpu naming to UUID. This would be more flexible to different
> > > > scenarios where UUID might not be required.
> > >
> > > Hi Kevin,
> > >
> > > Happy Chinese New Year!
> > >
> > > I think having UUID as the vgpu device name will allow us to have an gpu 
> > > vendor
> > > agnostic solution for the upper layer software stack such as QEMU, who is
> > > supposed to open the device.
> > >
> >
> > Qemu can use whatever sysfs path provided to open the device, regardless
> > of whether there is an UUID within the path...
> >
> 
> Hi Kevin,
> 
> Then it will provide even more benefit of using UUID as libvirt can be
> implemented as gpu vendor agnostic, right? :-)
> 
> The UUID can be VM UUID or vGPU group object UUID which really depends on the
> high level software stack, again the benefit is gpu vendor agnostic.
> 

There is case where libvirt is not used while another mgmt. stack doesn't use
UUID, e.g. in some Xen scenarios. So it's not about GPU vendor agnostic. It's
about high level mgmt. stack agnostic. That's why we need make UUID as

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Tian, Kevin]

> From: Neo Jia [mailto:c...@nvidia.com]
> Sent: Tuesday, February 16, 2016 3:13 PM
> 
> On Tue, Feb 16, 2016 at 06:49:30AM +, Tian, Kevin wrote:
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Thursday, February 04, 2016 3:33 AM
> > >
> > > On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> > > >   Hi,
> > > >
> > > > > Actually I have a long puzzle in this area. Definitely libvirt will 
> > > > > use UUID to
> > > > > mark a VM. And obviously UUID is not recorded within KVM. Then how 
> > > > > does
> > > > > libvirt talk to KVM based on UUID? It could be a good reference to 
> > > > > this design.
> > > >
> > > > libvirt keeps track which qemu instance belongs to which vm.
> > > > qemu also gets started with "-uuid ...", so one can query qemu via
> > > > monitor ("info uuid") to figure what the uuid is.  It is also in the
> > > > smbios tables so the guest can see it in the system information table.
> > > >
> > > > The uuid is not visible to the kernel though, the kvm kernel driver
> > > > doesn't know what the uuid is (and neither does vfio).  qemu uses file
> > > > handles to talk to both kvm and vfio.  qemu notifies both kvm and vfio
> > > > about anything relevant events (guest address space changes etc) and
> > > > connects file descriptors (eventfd -> irqfd).
> > >
> > > I think the original link to using a VM UUID for the vGPU comes from
> > > NVIDIA having a userspace component which might get launched from a udev
> > > event as the vGPU is created or the set of vGPUs within that UUID is
> > > started.  Using the VM UUID then gives them a way to associate that
> > > userspace process with a VM instance.  Maybe it could register with
> > > libvirt for some sort of service provided for the VM, I don't know.
> >
> > Intel doesn't have this requirement. It should be enough as long as
> > libvirt maintains which sysfs vgpu node is associated to a VM UUID.
> >
> > >
> > > > qemu needs a sysfs node as handle to the vfio device, something
> > > > like /sys/devices/virtual/vgpu/.   can be a uuid if you want
> > > > have it that way, but it could be pretty much anything.  The sysfs node
> > > > will probably show up as-is in the libvirt xml when assign a vgpu to a
> > > > vm.  So the name should be something stable (i.e. when using a uuid as
> > > > name you should better not generate a new one on each boot).
> > >
> > > Actually I don't think there's really a persistent naming issue, that's
> > > probably where we diverge from the SR-IOV model.  SR-IOV cannot
> > > dynamically add a new VF, it needs to reset the number of VFs to zero,
> > > then re-allocate all of them up to the new desired count.  That has some
> > > obvious implications.  I think with both vendors here, we can
> > > dynamically allocate new vGPUs, so I would expect that libvirt would
> > > create each vGPU instance as it's needed.  None would be created by
> > > default without user interaction.
> > >
> > > Personally I think using a UUID makes sense, but it needs to be
> > > userspace policy whether that UUID has any implicit meaning like
> > > matching the VM UUID.  Having an index within a UUID bothers me a bit,
> > > but it doesn't seem like too much of a concession to enable the use case
> > > that NVIDIA is trying to achieve.  Thanks,
> > >
> >
> > I would prefer to making UUID an optional parameter, while not tieing
> > sysfs vgpu naming to UUID. This would be more flexible to different
> > scenarios where UUID might not be required.
> 
> Hi Kevin,
> 
> Happy Chinese New Year!
> 
> I think having UUID as the vgpu device name will allow us to have an gpu 
> vendor
> agnostic solution for the upper layer software stack such as QEMU, who is
> supposed to open the device.
> 

Qemu can use whatever sysfs path provided to open the device, regardless
of whether there is an UUID within the path...

Thanks
Kevin

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Neo Jia]

On Tue, Feb 16, 2016 at 07:27:09AM +, Tian, Kevin wrote:
> > From: Neo Jia [mailto:c...@nvidia.com]
> > Sent: Tuesday, February 16, 2016 3:13 PM
> > 
> > On Tue, Feb 16, 2016 at 06:49:30AM +, Tian, Kevin wrote:
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Thursday, February 04, 2016 3:33 AM
> > > >
> > > > On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> > > > >   Hi,
> > > > >
> > > > > > Actually I have a long puzzle in this area. Definitely libvirt will 
> > > > > > use UUID to
> > > > > > mark a VM. And obviously UUID is not recorded within KVM. Then how 
> > > > > > does
> > > > > > libvirt talk to KVM based on UUID? It could be a good reference to 
> > > > > > this design.
> > > > >
> > > > > libvirt keeps track which qemu instance belongs to which vm.
> > > > > qemu also gets started with "-uuid ...", so one can query qemu via
> > > > > monitor ("info uuid") to figure what the uuid is.  It is also in the
> > > > > smbios tables so the guest can see it in the system information table.
> > > > >
> > > > > The uuid is not visible to the kernel though, the kvm kernel driver
> > > > > doesn't know what the uuid is (and neither does vfio).  qemu uses file
> > > > > handles to talk to both kvm and vfio.  qemu notifies both kvm and vfio
> > > > > about anything relevant events (guest address space changes etc) and
> > > > > connects file descriptors (eventfd -> irqfd).
> > > >
> > > > I think the original link to using a VM UUID for the vGPU comes from
> > > > NVIDIA having a userspace component which might get launched from a udev
> > > > event as the vGPU is created or the set of vGPUs within that UUID is
> > > > started.  Using the VM UUID then gives them a way to associate that
> > > > userspace process with a VM instance.  Maybe it could register with
> > > > libvirt for some sort of service provided for the VM, I don't know.
> > >
> > > Intel doesn't have this requirement. It should be enough as long as
> > > libvirt maintains which sysfs vgpu node is associated to a VM UUID.
> > >
> > > >
> > > > > qemu needs a sysfs node as handle to the vfio device, something
> > > > > like /sys/devices/virtual/vgpu/.   can be a uuid if you 
> > > > > want
> > > > > have it that way, but it could be pretty much anything.  The sysfs 
> > > > > node
> > > > > will probably show up as-is in the libvirt xml when assign a vgpu to a
> > > > > vm.  So the name should be something stable (i.e. when using a uuid as
> > > > > name you should better not generate a new one on each boot).
> > > >
> > > > Actually I don't think there's really a persistent naming issue, that's
> > > > probably where we diverge from the SR-IOV model.  SR-IOV cannot
> > > > dynamically add a new VF, it needs to reset the number of VFs to zero,
> > > > then re-allocate all of them up to the new desired count.  That has some
> > > > obvious implications.  I think with both vendors here, we can
> > > > dynamically allocate new vGPUs, so I would expect that libvirt would
> > > > create each vGPU instance as it's needed.  None would be created by
> > > > default without user interaction.
> > > >
> > > > Personally I think using a UUID makes sense, but it needs to be
> > > > userspace policy whether that UUID has any implicit meaning like
> > > > matching the VM UUID.  Having an index within a UUID bothers me a bit,
> > > > but it doesn't seem like too much of a concession to enable the use case
> > > > that NVIDIA is trying to achieve.  Thanks,
> > > >
> > >
> > > I would prefer to making UUID an optional parameter, while not tieing
> > > sysfs vgpu naming to UUID. This would be more flexible to different
> > > scenarios where UUID might not be required.
> > 
> > Hi Kevin,
> > 
> > Happy Chinese New Year!
> > 
> > I think having UUID as the vgpu device name will allow us to have an gpu 
> > vendor
> > agnostic solution for the upper layer software stack such as QEMU, who is
> > supposed to open the device.
> > 
> 
> Qemu can use whatever sysfs path provided to open the device, regardless
> of whether there is an UUID within the path...
> 

Hi Kevin,

Then it will provide even more benefit of using UUID as libvirt can be
implemented as gpu vendor agnostic, right? :-)

The UUID can be VM UUID or vGPU group object UUID which really depends on the
high level software stack, again the benefit is gpu vendor agnostic.

Thanks,
Neo

> Thanks
> Kevin

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> In disabled mode, virtio-blk dataplane seems to be enabled, but flow
> actually goes through the normal virtio path.  This patch simplifies a bit
> the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
> might be called even if s->dataplane is not NULL.
> 
> This is a bit tricky, because the current check for s->dataplane will
> always trigger, causing a continuous stream of calls to
> virtio_blk_data_plane_start.  Unfortunately, these calls will not
> do anything.  To fix this, set the "started" flag even in disabled
> mode, and skip virtio_blk_data_plane_start if the started flag is true.
> The resulting changes also prepare the code for the next patch, were
> virtio-blk dataplane will reuse the same virtio_blk_handle_output function
> as "regular" virtio-blk.
> 
> Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
> to move s->dataplane->started inside struct VirtIOBlock.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/block/dataplane/virtio-blk.c | 21 +
>  hw/block/virtio-blk.c   |  2 +-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 03b81bc..cc521c1 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -28,7 +28,6 @@
>  #include "qom/object_interfaces.h"
>  
>  struct VirtIOBlockDataPlane {
> -bool started;
>  bool starting;
>  bool stopping;
>  bool disabled;
> @@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  VirtQueue *vq;
>  int r;
>  
> -if (s->started || s->disabled) {
> -return;
> -}
> -
> -if (s->starting) {
> +if (vblk->dataplane_started || s->starting) {
>  return;
>  }
>  
> @@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  vblk->complete_request = complete_request_vring;
>  
>  s->starting = false;
> -s->started = true;
> +vblk->dataplane_started = true;
>  trace_virtio_blk_data_plane_start(s);
>  
>  blk_set_aio_context(s->conf->conf.blk, s->ctx);
> @@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  k->set_guest_notifiers(qbus->parent, 1, false);
>fail_guest_notifiers:
>  vring_teardown(>vring, s->vdev, 0);
> -s->disabled = true;
>fail_vring:
> +s->disabled = true;
>  s->starting = false;

Worth a comment here, or at definition of dataplane_started, explaining the
trick said in the commit message?

Reviewed-by: Fam Zheng 

> +vblk->dataplane_started = true;
>  }
>  

<...>

Re: [Qemu-devel] [PATCH 3/8] virtio: add AioContext-specific function for host notifiers

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> This is used to register ioeventfd with a dataplane thread.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/virtio/virtio.c | 16 
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 90f2545..3a5cca4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1785,6 +1785,22 @@ static void 
> virtio_queue_host_notifier_read(EventNotifier *n)
>  }
>  }
>  
> +void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext 
> *ctx,
> +bool assign, bool 
> set_handler)
> +{
> +if (assign && set_handler) {
> +aio_set_event_notifier(ctx, >host_notifier, true,
> +   virtio_queue_host_notifier_read);
> +} else {
> +aio_set_event_notifier(ctx, >host_notifier, true, NULL);
> +}
> +if (!assign) {
> +/* Test and clear notifier before after disabling event,

Does "before after" mean "after"? :)

Reviewed-by: Fam Zheng 

> + * in case poll callback didn't have time to run. */
> +virtio_queue_host_notifier_read(>host_notifier);
> +}
> +}
> +
>  void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> bool set_handler)
>  {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 108cdb0..4ce01a1 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -248,6 +248,8 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue 
> *vq, bool assign,
>  EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>  void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> bool set_handler);
> +void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext 
> *ctx,
> +bool assign, bool 
> set_handler);
>  void virtio_queue_notify_vq(VirtQueue *vq);
>  void virtio_irq(VirtQueue *vq);
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
> -- 
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH 4/8] virtio: export vring_notify as virtio_should_notify

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Virtio dataplane needs to trigger the irq manually through the
> guest notifier.  Export virtio_should_notify so that it can be
> used around event_notifier_set.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/virtio/virtio.c | 4 ++--
>  include/hw/virtio/virtio.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3a5cca4..abb97f4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1162,7 +1162,7 @@ void virtio_irq(VirtQueue *vq)
>  virtio_notify_vector(vq->vdev, vq->vector);
>  }
>  
> -static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
> +bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
>  uint16_t old, new;
>  bool v;
> @@ -1187,7 +1187,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue 
> *vq)
>  
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -if (!vring_notify(vdev, vq)) {
> +if (!virtio_should_notify(vdev, vq)) {
>  return;
>  }
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 4ce01a1..5884228 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -162,6 +162,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned 
> int *in_bytes,
> unsigned int *out_bytes,
> unsigned max_in_bytes, unsigned 
> max_out_bytes);
>  
> +bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> -- 
> 1.8.3.1
> 
> 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH v6 10/11] Dump: add hmp command "info dump"

[Peter Xu]

On Tue, Feb 16, 2016 at 02:46:15PM +0800, Fam Zheng wrote:
> On Wed, 12/09 10:42, Peter Xu wrote:
> > +
> > +void hmp_info_dump(Monitor *mon, const QDict *qdict)
> > +{
> > +DumpQueryResult *result = qmp_query_dump(NULL);
> 
> If something goes wrong, it is probably better to pass in local_err to
> qmp_query_dump and print the error info instead of assuming result's fields 
> are
> all valid and trying to interpret them.
> 
> Fam

This is because qmp_query_dump() will never fail, and it's not using
errp. Will assert "result" too before use.

Thanks.
Peter

Re: [Qemu-devel] [PATCH 2/8] vring: make vring_enable_notification return void

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Make the API more similar to the regular virtqueue API.  This will
> help when modifying the code to not use vring.c anymore.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/block/dataplane/virtio-blk.c | 3 ++-
>  hw/virtio/dataplane/vring.c | 3 +--
>  include/hw/virtio/dataplane/vring.h | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 0d99781..03b81bc 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -128,7 +128,8 @@ static void handle_notify(EventNotifier *e)
>  /* Re-enable guest->host notifies and stop processing the vring.
>   * But if the guest has snuck in more descriptors, keep 
> processing.
>   */
> -if (vring_enable_notification(s->vdev, >vring)) {
> +vring_enable_notification(s->vdev, >vring);
> +if (!vring_more_avail(s->vdev, >vring)) {
>  break;
>  }
>  } else { /* fatal error */
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 4308d9f..157e8b8 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -175,7 +175,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring 
> *vring)
>   *
>   * Return true if the vring is empty, false if there are more requests.
>   */
> -bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
> +void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
>  {
>  if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
>  vring_avail_event(>vr) = vring->vr.avail->idx;
> @@ -183,7 +183,6 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring 
> *vring)
>  vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
>  }
>  smp_mb(); /* ensure update is seen before reading avail_idx */
> -return !vring_more_avail(vdev, vring);
>  }
>  
>  /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
> diff --git a/include/hw/virtio/dataplane/vring.h 
> b/include/hw/virtio/dataplane/vring.h
> index e80985e..e1c2a65 100644
> --- a/include/hw/virtio/dataplane/vring.h
> +++ b/include/hw/virtio/dataplane/vring.h
> @@ -42,7 +42,7 @@ static inline void vring_set_broken(Vring *vring)
>  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
>  void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
>  void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
> -bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
> +void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
>  bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
>  void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
>  void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> -- 
> 1.8.3.1
> 
> 
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH 1/8] block-migration: acquire AioContext as necessary

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> This is needed because dataplane will run during block migration as well.
> 
> The block device migration code is quite liberal in taking the iothread
> mutex.  For simplicity, keep it the same way, even though one could
> actually choose between the BQL (for regular BlockDriverStates) and
> the AioContext (for dataplane BlockDriverStates).  When the block layer
> is made fully thread safe, aio_context_acquire shall go away altogether.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  migration/block.c | 61 
> ---
>  1 file changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index a444058..6dd2327 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -60,9 +60,15 @@ typedef struct BlkMigDevState {
>  int64_t cur_sector;
>  int64_t cur_dirty;
>  
> -/* Protected by block migration lock.  */
> +/* Data in the aio_bitmap is protected by block migration lock.
> + * Allocation and free happen during setup and cleanup respectively.
> + */
>  unsigned long *aio_bitmap;
> +
> +/* Protected by block migration lock.  */
>  int64_t completed_sectors;
> +
> +/* Protected by iothread lock / AioContext.  */
>  BdrvDirtyBitmap *dirty_bitmap;
>  Error *blocker;
>  } BlkMigDevState;
> @@ -100,7 +106,7 @@ typedef struct BlkMigState {
>  int prev_progress;
>  int bulk_completed;
>  
> -/* Lock must be taken _inside_ the iothread lock.  */
> +/* Lock must be taken _inside_ the iothread lock and any AioContexts.  */
>  QemuMutex lock;
>  } BlkMigState;
>  
> @@ -264,11 +270,13 @@ static int mig_save_device_bulk(QEMUFile *f, 
> BlkMigDevState *bmds)
>  
>  if (bmds->shared_base) {
>  qemu_mutex_lock_iothread();
> +aio_context_acquire(bdrv_get_aio_context(bs));
>  while (cur_sector < total_sectors &&
> !bdrv_is_allocated(bs, cur_sector, MAX_IS_ALLOCATED_SEARCH,
>_sectors)) {
>  cur_sector += nr_sectors;
>  }
> +aio_context_release(bdrv_get_aio_context(bs));
>  qemu_mutex_unlock_iothread();
>  }
>  
> @@ -302,11 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, 
> BlkMigDevState *bmds)
>  block_mig_state.submitted++;
>  blk_mig_unlock();
>  
> +/* We do not know if bs is under the main thread (and thus does
> + * not acquire the AioContext when doing AIO) or rather under
> + * dataplane.  Thus acquire both the iothread mutex and the
> + * AioContext.
> + *
> + * This is ugly and will disappear when we make bdrv_* thread-safe,
> + * without the need to acquire the AioContext.
> + */
>  qemu_mutex_lock_iothread();
> +aio_context_acquire(bdrv_get_aio_context(bmds->bs));
>  blk->aiocb = bdrv_aio_readv(bs, cur_sector, >qiov,
>  nr_sectors, blk_mig_read_cb, blk);
>  
>  bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
> +aio_context_release(bdrv_get_aio_context(bmds->bs));
>  qemu_mutex_unlock_iothread();
>  
>  bmds->cur_sector = cur_sector + nr_sectors;
> @@ -321,8 +339,9 @@ static int set_dirty_tracking(void)
>  int ret;
>  
>  QSIMPLEQ_FOREACH(bmds, _mig_state.bmds_list, entry) {
> +/* Creating/dropping dirty bitmaps only requires the big QEMU lock.  
> */

Why? I don't think it is safe today.  The BDS state is mutated and it can race
with bdrv_set_dirty() etc. (Also the refresh_total_sectors in bdrv_nb_sectors
can even do read/write, no?)

>  bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
>NULL, NULL);
>  if (!bmds->dirty_bitmap) {
>  ret = -errno;
>  goto fail;
> @@ -332,11 +352,14 @@ static int set_dirty_tracking(void)
>  return ret;
>  }
>  
> +/* Called with iothread lock taken.  */
> +
>  static void unset_dirty_tracking(void)
>  {
>  BlkMigDevState *bmds;
>  
>  QSIMPLEQ_FOREACH(bmds, _mig_state.bmds_list, entry) {
> +/* Creating/dropping dirty bitmaps only requires the big QEMU lock.  
> */

Ditto.

>  bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
>  }
>  }
> @@ -597,21 +627,28 @@ static void block_migration_cleanup(void *opaque)
>  {
>  BlkMigDevState *bmds;
>  BlkMigBlock *blk;
> +AioContext *ctx;
>  
>  bdrv_drain_all();
>  
>  unset_dirty_tracking();
>  
> -blk_mig_lock();

Why is it okay to skip the blk_mig_lock() for block_mig_state.bmds_list?

>  while ((bmds = QSIMPLEQ_FIRST(_mig_state.bmds_list)) != NULL) {
>  QSIMPLEQ_REMOVE_HEAD(_mig_state.bmds_list, entry);
>  bdrv_op_unblock_all(bmds->bs, bmds->blocker);
>  error_free(bmds->blocker);
> +
> +/* Save ctx, because bmds->bs can disappear during bdrv_unref.  

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Neo Jia]

On Tue, Feb 16, 2016 at 06:49:30AM +, Tian, Kevin wrote:
> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Thursday, February 04, 2016 3:33 AM
> > 
> > On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > Actually I have a long puzzle in this area. Definitely libvirt will use 
> > > > UUID to
> > > > mark a VM. And obviously UUID is not recorded within KVM. Then how does
> > > > libvirt talk to KVM based on UUID? It could be a good reference to this 
> > > > design.
> > >
> > > libvirt keeps track which qemu instance belongs to which vm.
> > > qemu also gets started with "-uuid ...", so one can query qemu via
> > > monitor ("info uuid") to figure what the uuid is.  It is also in the
> > > smbios tables so the guest can see it in the system information table.
> > >
> > > The uuid is not visible to the kernel though, the kvm kernel driver
> > > doesn't know what the uuid is (and neither does vfio).  qemu uses file
> > > handles to talk to both kvm and vfio.  qemu notifies both kvm and vfio
> > > about anything relevant events (guest address space changes etc) and
> > > connects file descriptors (eventfd -> irqfd).
> > 
> > I think the original link to using a VM UUID for the vGPU comes from
> > NVIDIA having a userspace component which might get launched from a udev
> > event as the vGPU is created or the set of vGPUs within that UUID is
> > started.  Using the VM UUID then gives them a way to associate that
> > userspace process with a VM instance.  Maybe it could register with
> > libvirt for some sort of service provided for the VM, I don't know.
> 
> Intel doesn't have this requirement. It should be enough as long as
> libvirt maintains which sysfs vgpu node is associated to a VM UUID.
> 
> > 
> > > qemu needs a sysfs node as handle to the vfio device, something
> > > like /sys/devices/virtual/vgpu/.   can be a uuid if you want
> > > have it that way, but it could be pretty much anything.  The sysfs node
> > > will probably show up as-is in the libvirt xml when assign a vgpu to a
> > > vm.  So the name should be something stable (i.e. when using a uuid as
> > > name you should better not generate a new one on each boot).
> > 
> > Actually I don't think there's really a persistent naming issue, that's
> > probably where we diverge from the SR-IOV model.  SR-IOV cannot
> > dynamically add a new VF, it needs to reset the number of VFs to zero,
> > then re-allocate all of them up to the new desired count.  That has some
> > obvious implications.  I think with both vendors here, we can
> > dynamically allocate new vGPUs, so I would expect that libvirt would
> > create each vGPU instance as it's needed.  None would be created by
> > default without user interaction.
> > 
> > Personally I think using a UUID makes sense, but it needs to be
> > userspace policy whether that UUID has any implicit meaning like
> > matching the VM UUID.  Having an index within a UUID bothers me a bit,
> > but it doesn't seem like too much of a concession to enable the use case
> > that NVIDIA is trying to achieve.  Thanks,
> > 
> 
> I would prefer to making UUID an optional parameter, while not tieing
> sysfs vgpu naming to UUID. This would be more flexible to different
> scenarios where UUID might not be required.

Hi Kevin,

Happy Chinese New Year!

I think having UUID as the vgpu device name will allow us to have an gpu vendor
agnostic solution for the upper layer software stack such as QEMU, who is
supposed to open the device.

Thanks,
Neo

> 
> Thanks
> Kevin

Re: [Qemu-devel] [PATCH] s390x: remove s390-zipl.rom

[Thomas Huth]

On 16.02.2016 07:07, Thomas Huth wrote:
> On 10.02.2016 12:35, Christian Borntraeger wrote:
>> On 02/10/2016 12:06 PM, Michael Tokarev wrote:
>>> This is an s390 boot rom which was used in s390-virtio machine.
>>> but since commit 3538fb6f89dd9bb2e7e59de2bfad52a45321c744
>>> "s390x: remove s390-virtio machine", this file isn't used.
>>> The only place it is referenced in the code is an unused
>>> which I'm modifying too, to refer to s390-ccw.rom instead.
>>>
>>> Cc: Pierre Morel 
>>> Signed-off-by: Michael Tokarev 
>>
>> a minor nit below, otherwise 
>> Acked-by: Christian Borntraeger 
>>
>>
>>> ---
>>>  Makefile   |   1 -
>>>  hw/s390x/ipl.c |   2 +-
>>>  hw/s390x/s390-virtio.c |   1 -
>>>  pc-bios/README |   4 
>>>  pc-bios/s390-zipl.rom  | Bin 3304 -> 0 bytes
>>>  5 files changed, 1 insertion(+), 7 deletions(-)
>>>  delete mode 100644 pc-bios/s390-zipl.rom
>>>
>>> diff --git a/Makefile b/Makefile
>>> index 30b1b2d..f9fae3a 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -400,7 +400,6 @@ efi-pcnet.rom efi-rtl8139.rom efi-virtio.rom \
>>>  qemu-icon.bmp qemu_logo_no_text.svg \
>>>  bamboo.dtb petalogix-s3adsp1800.dtb petalogix-ml605.dtb \
>>>  multiboot.bin linuxboot.bin kvmvapic.bin \
>>> -s390-zipl.rom \
>>>  s390-ccw.img \
>>>  spapr-rtas.bin slof.bin \
>>>  palcode-clipper \
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 6992add..4e6a0ac 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -106,7 +106,7 @@ static void s390_ipl_realize(DeviceState *dev, Error 
>>> **errp)
>>>  /* Adjust ELF start address to final location */
>>>  ipl->bios_start_addr += fwbase;
>>>  } else {
>>> -/* Try to load non-ELF file (e.g. s390-zipl.rom) */
>>> +/* Try to load non-ELF file (e.g. s390-ccw.rom) */
>>
>>  s390-ccw.img
> 
> Actually, s390-ccw.img is an ELF file, so this is a bad example for the
> above comment. Maybe it would be better to simply remove the parenthesis
> with the example instead?

... or even remove that else-branch completely since it is now not
necessary anymore? The ccw machine should always use an ELF image as
firmware, shouldn't it?

 Thomas

Re: [Qemu-devel] [PATCH v2 00/14] QOM'ify hw/timer/*

[<zxq_yx_...@163.com>]

Thanks Peter!
At 2016-02-16 02:22:09, "Peter Maydell"  wrote:
>On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
>> This patch series QOM'ify timer code under hw/timer directory.
>> Main idea is to split the initfn's work, some to TypeInfo.instance_init
>> and some is placed in DeviceClass::realize.
>> Drop the use of SysBusDeviceClass::init if possible.
>>
>> changes since v1:
>> fix a stupid typo (timmer->timer)
>
>Hi; thanks for this patchset. I've reviewed most of these
>and I think my comments on the remaining few patches would
>basically be the same as the ones I have reviewed.
>I would recommend that you cc the maintainers for the relevant
>devices on this patchset when you send out your next version.
>
>thanks
>-- PMM

Re: [Qemu-devel] [PATCH v6 09/11] Dump: add qmp command "query-dump"

[Peter Xu]

On Tue, Feb 16, 2016 at 02:40:59PM +0800, Fam Zheng wrote:
> On Wed, 12/09 10:42, Peter Xu wrote:
> > -s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
> > -error_propagate(errp, local_err);
> 
> This means the error is released by caller, ...
> 
> > +/* make sure status is written after written_size updates */
> > +smp_wmb();
> > +atomic_set(>status,
> > +   (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
> 
> but it is removed now.  Is local_err leaked now?

Right. Lost it during switching of patches and merges. Will
fix. Thanks!

Peter

Re: [Qemu-devel] [PATCH v6 11/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Fam Zheng]

On Wed, 12/09 10:42, Peter Xu wrote:
> One new QMP event DUMP_COMPLETED is added. When a dump finishes, one
> DUMP_COMPLETED event will occur to notify the user.
> 
> Signed-off-by: Peter Xu 
> ---
>  docs/qmp-events.txt | 18 ++
>  dump.c  | 19 +--
>  qapi/event.json | 16 
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
> index d2f1ce4..3a03004 100644
> --- a/docs/qmp-events.txt
> +++ b/docs/qmp-events.txt
> @@ -220,6 +220,24 @@ Data:
>},
>"timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  
> +DUMP_COMPLETED
> +--
> +
> +Emitted when the guest has finished one memory dump.
> +
> +Data:
> +
> +- "result": DumpQueryResult type described in qapi-schema.json
> +- "error": Error message when dump failed. This is only a
> +  human-readable string provided when dump failed. It should not be
> +  parsed in any way (json-string, optional)
> +
> +Example:
> +
> +{ "event": "DUMP_COMPLETED",
> +  "data": {"result": {"total": 1090650112, "status": "completed",
> +  "completed": 1090650112} } }
> +
>  GUEST_PANICKED
>  --
>  
> diff --git a/dump.c b/dump.c
> index 80afa1e..30be595 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -25,6 +25,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
>  #include "qmp-commands.h"
> +#include "qapi-event.h"
>  
>  #include 
>  #ifdef CONFIG_LZO
> @@ -1633,6 +1634,7 @@ cleanup:
>  static void dump_process(DumpState *s, Error **errp)
>  {
>  Error *local_err = NULL;
> +DumpQueryResult *result = NULL;
>  
>  if (s->has_format && s->format != DUMP_GUEST_MEMORY_FORMAT_ELF) {
>  create_kdump_vmcore(s, _err);
> @@ -1645,6 +1647,16 @@ static void dump_process(DumpState *s, Error **errp)
>  atomic_set(>status,
> (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));
>  
> +/* send DUMP_COMPLETED message (unconditionally) */
> +result = qmp_query_dump(NULL);
> +/* should never fail */
> +assert(result);
> +qapi_event_send_dump_completed(result, !!local_err, (local_err ? \
> +   error_get_pretty(local_err) : NULL),
> +   _abort);
> +qapi_free_DumpQueryResult(result);
> +
> +error_propagate(errp, local_err);
>  dump_cleanup(s);
>  }
>  
> @@ -1652,13 +1664,8 @@ static void *dump_thread(void *data)
>  {
>  Error *err = NULL;
>  DumpState *s = (DumpState *)data;
> -
>  dump_process(s, );
> -
> -if (err) {
> -/* TODO: notify user the error */
> -error_free(err);
> -}
> +error_free(err);
>  return NULL;
>  }
>  
> diff --git a/qapi/event.json b/qapi/event.json
> index f0cef01..bf7dd61 100644
> --- a/qapi/event.json
> +++ b/qapi/event.json
> @@ -356,3 +356,19 @@
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
>'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @DUMP_COMPLETED
> +#
> +# Emitted when background dump has completed
> +#
> +# @result: DumpQueryResult type described in qapi-schema.json.
> +#
> +# @error: #optional human-readable error string that provides
> +# hint on why dump failed. Only presents on failure. The
> +# user should not try to interpret the error string.
> +#
> +# Since: 2.6
> +##
> +{ 'event': 'DUMP_COMPLETED' ,
> +  'data': { 'result': 'DumpQueryResult', '*error': 'str' } }
> -- 
> 2.4.3
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [RFC PATCH v1 1/1] vGPU core driver : to provide common interface for vGPU.

[Tian, Kevin]

> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Thursday, February 04, 2016 3:33 AM
> 
> On Wed, 2016-02-03 at 09:28 +0100, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > Actually I have a long puzzle in this area. Definitely libvirt will use 
> > > UUID to
> > > mark a VM. And obviously UUID is not recorded within KVM. Then how does
> > > libvirt talk to KVM based on UUID? It could be a good reference to this 
> > > design.
> >
> > libvirt keeps track which qemu instance belongs to which vm.
> > qemu also gets started with "-uuid ...", so one can query qemu via
> > monitor ("info uuid") to figure what the uuid is.  It is also in the
> > smbios tables so the guest can see it in the system information table.
> >
> > The uuid is not visible to the kernel though, the kvm kernel driver
> > doesn't know what the uuid is (and neither does vfio).  qemu uses file
> > handles to talk to both kvm and vfio.  qemu notifies both kvm and vfio
> > about anything relevant events (guest address space changes etc) and
> > connects file descriptors (eventfd -> irqfd).
> 
> I think the original link to using a VM UUID for the vGPU comes from
> NVIDIA having a userspace component which might get launched from a udev
> event as the vGPU is created or the set of vGPUs within that UUID is
> started.  Using the VM UUID then gives them a way to associate that
> userspace process with a VM instance.  Maybe it could register with
> libvirt for some sort of service provided for the VM, I don't know.

Intel doesn't have this requirement. It should be enough as long as
libvirt maintains which sysfs vgpu node is associated to a VM UUID.

> 
> > qemu needs a sysfs node as handle to the vfio device, something
> > like /sys/devices/virtual/vgpu/.   can be a uuid if you want
> > have it that way, but it could be pretty much anything.  The sysfs node
> > will probably show up as-is in the libvirt xml when assign a vgpu to a
> > vm.  So the name should be something stable (i.e. when using a uuid as
> > name you should better not generate a new one on each boot).
> 
> Actually I don't think there's really a persistent naming issue, that's
> probably where we diverge from the SR-IOV model.  SR-IOV cannot
> dynamically add a new VF, it needs to reset the number of VFs to zero,
> then re-allocate all of them up to the new desired count.  That has some
> obvious implications.  I think with both vendors here, we can
> dynamically allocate new vGPUs, so I would expect that libvirt would
> create each vGPU instance as it's needed.  None would be created by
> default without user interaction.
> 
> Personally I think using a UUID makes sense, but it needs to be
> userspace policy whether that UUID has any implicit meaning like
> matching the VM UUID.  Having an index within a UUID bothers me a bit,
> but it doesn't seem like too much of a concession to enable the use case
> that NVIDIA is trying to achieve.  Thanks,
> 

I would prefer to making UUID an optional parameter, while not tieing
sysfs vgpu naming to UUID. This would be more flexible to different
scenarios where UUID might not be required.

Thanks
Kevin

Re: [Qemu-devel] [PATCH v6 10/11] Dump: add hmp command "info dump"

[Fam Zheng]

On Wed, 12/09 10:42, Peter Xu wrote:
> It will calculate percentage of finished work from completed and
> total.
> 
> Signed-off-by: Peter Xu 
> ---
>  hmp-commands-info.hx | 14 ++
>  hmp.c| 17 +
>  hmp.h|  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 9b71351..52539c3 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -786,6 +786,20 @@ STEXI
>  Display the value of a storage key (s390 only)
>  ETEXI
>  
> +{
> +.name   = "dump",
> +.args_type  = "",
> +.params = "",
> +.help   = "Display the latest dump status",
> +.mhandler.cmd = hmp_info_dump,
> +},
> +
> +STEXI
> +@item info dump
> +@findex dump
> +Display the latest dump status.
> +ETEXI
> +
>  STEXI
>  @end table
>  ETEXI
> diff --git a/hmp.c b/hmp.c
> index 1f4d0b6..c824064 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2383,3 +2383,20 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const 
> QDict *qdict)
>  
>  qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_info_dump(Monitor *mon, const QDict *qdict)
> +{
> +DumpQueryResult *result = qmp_query_dump(NULL);

If something goes wrong, it is probably better to pass in local_err to
qmp_query_dump and print the error info instead of assuming result's fields are
all valid and trying to interpret them.

Fam

> +
> +assert(result->status < DUMP_STATUS_MAX);
> +monitor_printf(mon, "Status: %s\n", DumpStatus_lookup[result->status]);
> +
> +if (result->status == DUMP_STATUS_ACTIVE) {
> +float percent = 0;
> +assert(result->total != 0);
> +percent = 100.0 * result->completed / result->total;
> +monitor_printf(mon, "Finished: %.2f %%\n", percent);
> +}
> +
> +qapi_free_DumpQueryResult(result);
> +}
> diff --git a/hmp.h b/hmp.h
> index a8c5b5a..093d65f 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -131,5 +131,6 @@ void hmp_rocker(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_ports(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict);
>  void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict);
> +void hmp_info_dump(Monitor *mon, const QDict *qdict);
>  
>  #endif
> -- 
> 2.4.3
> 

Re: [Qemu-devel] [PATCH v6 09/11] Dump: add qmp command "query-dump"

[Fam Zheng]

On Wed, 12/09 10:42, Peter Xu wrote:
> -s->status = (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED);
> -error_propagate(errp, local_err);

This means the error is released by caller, ...

> +/* make sure status is written after written_size updates */
> +smp_wmb();
> +atomic_set(>status,
> +   (local_err ? DUMP_STATUS_FAILED : DUMP_STATUS_COMPLETED));

but it is removed now.  Is local_err leaked now?

Fam

Re: [Qemu-devel] RE: [iGVT-g] VFIO based vGPU(was Re: [Announcement] 2015-Q3 release of XenGT - a Mediated ...)

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Thursday, February 04, 2016 11:02 AM
> 
> >
> > Thanks for your summary, Kevin.  It does seem like there are only a few
> > outstanding issues which should be manageable and hopefully the overall
> > approach is cleaner for QEMU, management tools, and provides a more
> > consistent user interface as well.  If we can translate the solution to
> > Xen, that's even better.  Thanks,
> >
> 
> Here is the main open in my head, after thinking about the role of VFIO:
> 
> For above 7 services required by vGPU device model, they can fall into
> two categories:
> 
> a) services to connect vGPU with VM, which are essentially what a device
> driver is doing (so VFIO can fit here), including:
>   1) Selectively pass-through a region to a VM
>   2) Trap-and-emulate a region
>   3) Inject a virtual interrupt
>   5) Pin/unpin guest memory
>   7) GPA->IOVA/HVA translation (as a side-effect)
> 
> b) services to support device emulation, which gonna be hypervisor
> specific, including:
>   4) Map/unmap guest memory
>   6) Write-protect a guest memory page
> 
> VFIO can fulfill category a), but not for b). A possible abstraction would
> be in vGPU core driver, to allow specific hypervisor registering callbacks
> for category b) (which means a KVMGT specific file say KVM-vGPU will
> be added to KVM to connect both together).
> 
> Then a likely layered blocks would be like:
> 
> VFIO-vGPU  <->  vGPU Core  <-> KVMGT-vGPU
> ^   ^
> |   |
> |   |
> v   v
>   nvidia   intel
>vGPUvGPU
> 
> Xen will register its own vGPU bus driver (not using VFIO today) and
> also hypervisor services using the same framework. With this design,
> everything is abstracted/registered through vGPU core driver, instead
> of talking with each other directly.
> 
> Thoughts?
> 
> P.S. from the description of above requirements, the whole framework
> might be also extended to cover any device type using same mediated
> pass-through approach. Though graphics has some special requirement,
> the majority are actually device agnostics. Maybe better not limiting it
> with a vGPU name at all. :-)
> 

Any feedback on above open?

btw, based on above description I believe the interaction between VFIO
and vGPU has become very clear. The remaining two services are related
to how a hypervisor provides emulation services to vendor specific vGPU 
device model (more generally it's not vGPU specific. Can apply to any in-kernel 
emulation requirement so KVMGT-vGPU might not be a good name). This part 
is not related to VFIO at all, so we'll start prototyping VFIO related changes 
in parallel.

Since this is related to KVM, Paolo, your comment is also welcomed. :-)

Thanks,
Kevin

[Qemu-devel] [PATCH] s390: remove misleading comment

[Michael Tokarev]

The comment talks about a non-ELF object while the
example gives ELF object.

Signed-off-by: Michael Tokarev 
---
 hw/s390x/ipl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index c9cf7cc..2213405 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -106,7 +106,7 @@ static void s390_ipl_realize(DeviceState *dev, Error **errp)
 /* Adjust ELF start address to final location */
 ipl->bios_start_addr += fwbase;
 } else {
-/* Try to load non-ELF file (e.g. s390-ccw.img) */
+/* Try to load non-ELF file */
 bios_size = load_image_targphys(bios_filename, ZIPL_IMAGE_START,
 4096);
 ipl->bios_start_addr = ZIPL_IMAGE_START;
-- 
2.1.4

Re: [Qemu-devel] [PATCH 3/3] replay: introduce block devices record/replay

[Pavel Dovgalyuk]

> From: Kevin Wolf [mailto:kw...@redhat.com]
> Am 15.02.2016 um 15:24 hat Pavel Dovgalyuk geschrieben:
> > > From: Kevin Wolf [mailto:kw...@redhat.com]
> >
> > > > There could be asynchronous events that occur in non-cpu threads.
> > > > For now these events are shutdown request and block task execution.
> > > > They may "hide" following clock (or another one) events. That is why
> > > > we process them until synchronous event (like clock, instructions
> > > > execution, or checkpoint) is met.
> > > >
> > > >
> > > > > Anyway, what does "can't proceed" mean? The coroutine yields because
> > > > > it's waiting for I/O, but it is never reentered? Or is it hanging 
> > > > > while
> > > > > trying to acquire a lock?
> > > >
> > > > I've solved this problem by slightly modifying the queue.
> > > > I haven't yet made BlockDriverState assignment to the request ids.
> > > > Therefore aio_poll was temporarily replaced with usleep.
> > > > Now execution starts and hangs at some random moment of OS loading.
> > > >
> > > > Here is the current version of blkreplay functions:
> > > >
> > > > static int coroutine_fn blkreplay_co_readv(BlockDriverState *bs,
> > > > int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
> > > > {
> > > > uint32_t reqid = request_id++;
> > > > Request *req;
> > > > req = block_request_insert(reqid, bs, qemu_coroutine_self());
> > > > bdrv_co_readv(bs->file->bs, sector_num, nb_sectors, qiov);
> > > >
> > > > if (replay_mode == REPLAY_MODE_RECORD) {
> > > > replay_save_block_event(reqid);
> > > > } else {
> > > > assert(replay_mode == REPLAY_MODE_PLAY);
> > > > qemu_coroutine_yield();
> > > > }
> > > > block_request_remove(req);
> > > >
> > > > return 0;
> > > > }
> > > >
> > > > void replay_run_block_event(uint32_t id)
> > > > {
> > > > Request *req;
> > > > if (replay_mode == REPLAY_MODE_PLAY) {
> > > > while (!(req = block_request_find(id))) {
> > > > //aio_poll(bdrv_get_aio_context(req->bs), true);
> > > > usleep(1);
> > > > }
> > >
> > > How is this loop supposed to make any progress?
> >
> > This loop does not supposed to make any progress. It waits until 
> > block_request_insert
> > call is added to the queue.
> 
> Yes. And who is supposed to add something to the queue? You are looping
> and doing nothing, so no other code gets to run in this thread. It's
> only aio_poll() that would run event handlers that could eventually add
> something to the list.

blkreplay_co_readv adds request to the queue.

> 
> > > And I still don't understand why aio_poll() doesn't work and where it
> > > hangs.
> >
> > aio_poll hangs if "req = block_request_insert(reqid, bs, 
> > qemu_coroutine_self());" line
> > is executed after bdrv_co_readv. When bdrv_co_readv yields, 
> > replay_run_block_event has no
> > information about pending request and cannot jump to its coroutine.
> > Maybe I should implement aio_poll execution there to make progress in that 
> > case?
> 
> Up in the thread, I posted code that was a bit more complex than what
> you have, and which considered both cases (completion before
> replay/completion after replay). If you implement it like this, it might
> just work. And yes, it involved an aio_poll() loop in the replay
> function.

You are right, but I found kind of fundamental problem here.
There are two possible approaches for replaying coroutine events:

1. Waiting with aio_poll in replay_run_block_event.
   In this case replay cannot be made, because of recursive mutex lock:
   aio_poll -> qemu_clock_get_ns ->  -> 
 replay_run_block_event -> aio_poll -> qemu_clock_get_ns ->  ->
 
   I.e. we have recursive aio_poll function calls that lead to recursive replay 
calls
   and to recursive mutex lock.

2. Adding block events to the queue until checkpoint is met.
   This will not work with synchronous requests, because they will wait 
infinitely
   and checkpoint will never happen. Correct me, if I'm not right for this case.
   If blkreplay_co_readv will yield, can vcpu thread unlock the BQL and wait 
for 
   the checkpoint in iothread?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH COLO-Frame v14 24/40] COLO: Process shutdown command for VM in COLO state

[Hailiang Zhang]

On 2016/2/12 23:09, Dr. David Alan Gilbert wrote:

* zhanghailiang (zhang.zhanghaili...@huawei.com) wrote:

If VM is in COLO FT state, we should do some extra work before normal shutdown
process. SVM will ignore the shutdown command if this command is issued directly
to it, PVM will send the shutdown command to SVM if it gets this command.

Cc: Paolo Bonzini 
Signed-off-by: zhanghailiang 
Signed-off-by: Li Zhijian 


Reviewed-by: Dr. David Alan Gilbert 

although one question below.



Thanks!


Dave


---
v14:
- Remove 'colo_shutdown' variable, use colo_shutdown_request directly
v13:
- Move COLO shutdown related codes to colo.c file (Dave's suggestion)
---
  include/migration/colo.h |  2 ++
  include/sysemu/sysemu.h  |  3 +++
  migration/colo.c | 42 --
  qapi-schema.json |  4 +++-
  stubs/migration-colo.c   |  5 +
  vl.c | 19 ---
  6 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index e32eef4..919b135 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -35,4 +35,6 @@ COLOMode get_colo_mode(void);

  /* failover */
  void colo_do_failover(MigrationState *s);
+
+bool colo_shutdown(void);
  #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3bb8897..91eeda3 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -52,6 +52,8 @@ typedef enum WakeupReason {
  QEMU_WAKEUP_REASON_OTHER,
  } WakeupReason;

+extern int colo_shutdown_requested;
+
  void qemu_system_reset_request(void);
  void qemu_system_suspend_request(void);
  void qemu_register_suspend_notifier(Notifier *notifier);
@@ -59,6 +61,7 @@ void qemu_system_wakeup_request(WakeupReason reason);
  void qemu_system_wakeup_enable(WakeupReason reason, bool enabled);
  void qemu_register_wakeup_notifier(Notifier *notifier);
  void qemu_system_shutdown_request(void);
+void qemu_system_shutdown_request_core(void);
  void qemu_system_powerdown_request(void);
  void qemu_register_powerdown_notifier(Notifier *notifier);
  void qemu_system_debug_request(void);
diff --git a/migration/colo.c b/migration/colo.c
index 515d561..92be985 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -330,6 +330,18 @@ static int colo_do_checkpoint_transaction(MigrationState 
*s,
  goto out;
  }

+if (colo_shutdown_requested) {
+colo_put_cmd(s->to_dst_file, COLO_MESSAGE_GUEST_SHUTDOWN, _err);
+if (local_err) {
+goto out;
+}


I wonder what actually happens in this case? Does it eventually shutdown?



No, the qemu will not exit from the main loop,
it seems that, we should ignore the error here, and go on the shutdown process.
I will fix it in next version.


+qemu_fflush(s->to_dst_file);
+colo_shutdown_requested = 0;
+qemu_system_shutdown_request_core();
+/* Fix me: Just let the colo thread exit ? */
+qemu_thread_exit(0);
+}
+
  ret = 0;
  /* Resume primary guest */
  qemu_mutex_lock_iothread();
@@ -390,8 +402,9 @@ static void colo_process_checkpoint(MigrationState *s)
  }

  current_time = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-if (current_time - checkpoint_time <
-s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY]) {
+if ((current_time - checkpoint_time <
+s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY]) &&
+!colo_shutdown_requested) {
  int64_t delay_ms;

  delay_ms = s->parameters[MIGRATION_PARAMETER_X_CHECKPOINT_DELAY] -
@@ -465,6 +478,15 @@ static void colo_wait_handle_cmd(QEMUFile *f, int 
*checkpoint_request,
  case COLO_MESSAGE_CHECKPOINT_REQUEST:
  *checkpoint_request = 1;
  break;
+case COLO_MESSAGE_GUEST_SHUTDOWN:
+qemu_mutex_lock_iothread();
+vm_stop_force_state(RUN_STATE_COLO);
+qemu_system_shutdown_request_core();
+qemu_mutex_unlock_iothread();
+/* the main thread will exit and terminate the whole
+* process, do we need some cleanup?
+*/
+qemu_thread_exit(0);
  default:
  *checkpoint_request = 0;
  error_setg(errp, "Got unknown COLO command: %d", cmd);
@@ -636,3 +658,19 @@ out:

  return NULL;
  }
+
+bool colo_shutdown(void)
+{
+/*
+* if in colo mode, we need do some significant work before respond
+* to the shutdown request.
+*/
+if (migration_incoming_in_colo_state()) {
+return true; /* primary's responsibility */
+}
+if (migration_in_colo_state()) {
+colo_shutdown_requested = 1;
+return true;
+}
+return false;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 7fec696..4d8ba04 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -752,12 +752,14 @@
  #
  # 

Re: [Qemu-devel] [PATCH] s390x: remove s390-zipl.rom

[Thomas Huth]

On 10.02.2016 12:35, Christian Borntraeger wrote:
> On 02/10/2016 12:06 PM, Michael Tokarev wrote:
>> This is an s390 boot rom which was used in s390-virtio machine.
>> but since commit 3538fb6f89dd9bb2e7e59de2bfad52a45321c744
>> "s390x: remove s390-virtio machine", this file isn't used.
>> The only place it is referenced in the code is an unused
>> which I'm modifying too, to refer to s390-ccw.rom instead.
>>
>> Cc: Pierre Morel 
>> Signed-off-by: Michael Tokarev 
> 
> a minor nit below, otherwise 
> Acked-by: Christian Borntraeger 
> 
> 
>> ---
>>  Makefile   |   1 -
>>  hw/s390x/ipl.c |   2 +-
>>  hw/s390x/s390-virtio.c |   1 -
>>  pc-bios/README |   4 
>>  pc-bios/s390-zipl.rom  | Bin 3304 -> 0 bytes
>>  5 files changed, 1 insertion(+), 7 deletions(-)
>>  delete mode 100644 pc-bios/s390-zipl.rom
>>
>> diff --git a/Makefile b/Makefile
>> index 30b1b2d..f9fae3a 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -400,7 +400,6 @@ efi-pcnet.rom efi-rtl8139.rom efi-virtio.rom \
>>  qemu-icon.bmp qemu_logo_no_text.svg \
>>  bamboo.dtb petalogix-s3adsp1800.dtb petalogix-ml605.dtb \
>>  multiboot.bin linuxboot.bin kvmvapic.bin \
>> -s390-zipl.rom \
>>  s390-ccw.img \
>>  spapr-rtas.bin slof.bin \
>>  palcode-clipper \
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 6992add..4e6a0ac 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -106,7 +106,7 @@ static void s390_ipl_realize(DeviceState *dev, Error 
>> **errp)
>>  /* Adjust ELF start address to final location */
>>  ipl->bios_start_addr += fwbase;
>>  } else {
>> -/* Try to load non-ELF file (e.g. s390-zipl.rom) */
>> +/* Try to load non-ELF file (e.g. s390-ccw.rom) */
> 
>   s390-ccw.img

Actually, s390-ccw.img is an ELF file, so this is a bad example for the
above comment. Maybe it would be better to simply remove the parenthesis
with the example instead?

 Thomas

Re: [Qemu-devel] [PATCH v9 2/3] quorum: implement bdrv_add_child() and bdrv_del_child()

[Changlong Xie]

On 02/09/2016 01:06 AM, Alberto Garcia wrote:

On Fri 22 Jan 2016 09:02:10 PM CET, "Dr. David Alan Gilbert" 
 wrote:


In general, what do you do to make sure that the data in a new
Quorum child is consistent with that of the rest of the array?


Quorum can have more than one child when it starts. But we don't
do the similar check. So I don't think we should do such check
here.


Yes, but when you start a VM you can verify in advance that all
members of the Quorum have the same data. If you do that on a
running VM how can you know if the new disk is consistent with the
others?


User error if it is not.  Just the same as it is user error if you
request a shallow drive-mirror but the destination is not the same
contents as the backing file.  I don't think qemu has to protect us
from user error in this case.


But the backing file is read-only so the user can guarantee that the
destination has the same data before the shallow mirror. How do you
do that in this case?


I think in the colo case they're relying on doing a block migrate to
synchronise the remote disk prior to switching into colo mode.


Yes but this is a general API that can be used independently from
COLO. I'd say if we want to allow that we should at least place a big
warning in the documentation.



Ok, that's fair enough. Will add in next version.

Thanks
-Xie

Berto


.

Re: [Qemu-devel] [PATCH 04/12] docker: Add test runner

[Fam Zheng]

On Mon, 02/15 14:10, Alex Bennée wrote:
> 
> Fam Zheng  writes:
> 
> > On Mon, 02/15 12:45, Alex Bennée wrote:
> >>
> >> Alex Bennée  writes:
> >>
> >> > Fam Zheng  writes:
> >> >
> >> >> It's better to have a launcher for all tests, to make it easier to
> >> >> initialize and manage the environment.
> >> >>
> >> >> Signed-off-by: Fam Zheng 
> >> >> ---
> >> >>  tests/docker/run | 23 +++
> >> >>  1 file changed, 23 insertions(+)
> >> >>  create mode 100755 tests/docker/run
> >> >>
> 
> >> OK that won't work if you have alternates in your git setup. It seems to
> >> me the creation of the pristine tree for docker should be done before we
> >> enter the docker environment.
> >>
> >> There is a slight wrinkle as to what happens if the user wants to enter
> >> an interactive session for debugging. However that doesn't seem possible
> >> via the makefile and perhaps that is just as well. Perhaps that should
> >> be a helper script for the user?
> >
> > We can use "make distclean" in the copied tree.
> 
> I wouldn't - distclean isn't always that clean due to *mumble mumble*
> makefile reasons. You could try this approach:
> 
> commit f838d3bbe2f71c731dfe172f1c3286084de203c8
> Author: Alex Bennée 
> Date:   Mon Feb 15 12:52:21 2016 +
> 
> tests/docker/Makefile.include: snapshot the src for docker
> 
> Instead of providing a live version of the source tree to the docker
> container we snapshot it with git-archive. This ensure the tree is in a
> pristine state for whatever operations the container is going to run on
> them.
> 
> Uncommitted changes known to files known by the git index will be
> included in the snapshot if there are any.
> 
> Signed-off-by: Alex Bennée 
> 
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index ca84c35..63a799c 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -2,7 +2,7 @@
> 
>  $(if $(quiet-command),,$(eval include $(SRC_PATH)/rules.mak))
> 
> -.PHONY: docker docker-build docker-run docker-clean
> +.PHONY: docker docker-build docker-qemu-src docker-run docker-clean
> 
>  DOCKER_SUFFIX = .docker
> 
> @@ -16,12 +16,17 @@ DOCKER_TOOLS := $(filter-out test-%, $(DOCKER_SCRIPTS))
>  TESTS ?= %
>  IMAGES ?= %
> 
> +docker-qemu-src:
> + $(if git diff-index --quiet HEAD --,
> \
> + git archive -1 HEAD --prefix=qemu-docker-snapshot/ 
> --format=tar.gz | tar -xz -C /tmp,   \
> + git archive -1 `git stash create` 
> --prefix=qemu-docker-snapshot/ --format=tar.gz | tar -xz -C /tmp)

Does this clutter the user's git stash list? Or we can just error out if the
working tree is not clean.

Fam

Re: [Qemu-devel] [PATCH v2 0/2] move qcow2_invalidate_cache() out of coroutine context

[Denis V. Lunev]

On 02/12/2016 09:39 AM, Denis V. Lunev wrote:

There is a possibility to hit an assert in qcow2_get_specific_info that
s->qcow_version is undefined. This happens when VM in starting from
suspended state, i.e. it processes incoming migration, and in the same
time 'info block' is called.

The problem is that qcow2_invalidate_cache() closes the image and
memset()s BDRVQcowState in the middle.

This operation should not be performed in coroutine context.

Changes from v1:
- fixed spelling. Eric, thank you for spell checking

Signed-off-by: Denis V. Lunev 
CC: Paolo Bonzini 
CC: Juan Quintela 
CC: Amit Shah 


ping

Re: [Qemu-devel] [PATCH v7 0/5] don't use NVDIMM for balooning

[Denis V. Lunev]

On 02/10/2016 11:49 AM, Vladimir Sementsov-Ogievskiy wrote:

v7:
   02: Reviewed-by: Igor Mammedov
   04: object instead of dimm
   05: arror_abort instead of NULL for getting size property,
   remove superfluous include

v6:
  add stubbed pc_dimm_build_list, fix compilation for
  !CONFIG_MEM_HOTPLUG targets - thx to Cornelia.

v5: do not use qapi
  0002-0004: new patches
  0005: white list instead of black list

v4:
  0001: Reviewed-by: Eric Blake 
  second patch is splitted to 0002 and 0003
  0002: Add 'type' field instead of 'balloonable' to PCDIMMDeviceInfo
  0003: chec 'type' instead of 'balloonable'

v3:
 - do not use additional class variable

NVDIMM for now is planned to use as a backing store for DAX filesystem
in the guest and thus this memory is excluded from guest memory
management and LRUs.

In this case libvirt running QEMU along with configured balloon almost
immediately inflates balloon and effectively kill the guest as
qemu counts nvdimm as part of the ram.


Vladimir Sementsov-Ogievskiy (5):
   move get_current_ram_size to virtio-balloon.c
   pc-dimm: rename pc_dimm_built_list()
   pc-dimm: add pc_dimm_build_list()
   virtio-balloon: rewrite get_current_ram_size()
   balloon: Use only 'pc-dimm' type dimm for ballooning

  hw/mem/pc-dimm.c| 47 -
  hw/virtio/virtio-balloon.c  | 18 
  include/exec/cpu-common.h   |  1 -
  include/hw/mem/pc-dimm.h|  3 +++
  stubs/Makefile.objs |  2 +-
  stubs/pc_dimm.c | 12 +++
  stubs/qmp_pc_dimm_device_list.c | 12 ---
  7 files changed, 52 insertions(+), 43 deletions(-)
  create mode 100644 stubs/pc_dimm.c
  delete mode 100644 stubs/qmp_pc_dimm_device_list.c


ping

[Qemu-devel] [Bug 1329956] Re: multi-core FreeBSD guest hangs after warm reboot

[Venkateswara Rao Dokku]

Can you please let us know the exact version of the kernel it got fixed?

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1329956

Title:
  multi-core FreeBSD guest hangs after warm reboot

Status in QEMU:
  Incomplete

Bug description:
  On some Linux KVM hosts in our environment, FreeBSD guests fail to
  reboot properly if they have more than one CPU (socket, core, and/or
  thread). They will boot fine the first time, but after issuing a
  "reboot" command via the OS the guest starts to boot but hangs during
  SMP initialization. Fully shutting down and restarting the guest works
  in all cases.

  The only meaningful difference between hosts with the problem and those 
without is the CPU. Hosts with Xeon E5-26xx v2 processors have the problem, 
including at least the "Intel(R) Xeon(R) CPU E5-2667 v2" and the "Intel(R) 
Xeon(R) CPU E5-2650 v2".
  Hosts with any other CPU, including "Intel(R) Xeon(R) CPU E5-2650 0", 
"Intel(R) Xeon(R) CPU E5-2620 0", or "AMD Opteron(TM) Processor 6274" do not 
have the problem. Note the "v2" in the names of the problematic CPUs.

  On hosts with a "v2" Xeon, I can reproduce the problem under Linux
  kernel 3.10 or 3.12 and Qemu 1.7.0 or 2.0.0.

  The problem occurs with all currently-supported versions of FreeBSD,
  including 8.4, 9.2, 10.0 and 11-CURRENT.

  On a Linux KVM host with a "v2" Xeon, this command line is adequate to
  reproduce the problem:

  /usr/bin/qemu-system-x86_64 -machine accel=kvm -name bsdtest -m 512
  -smp 2,sockets=1,cores=1,threads=2 -drive
  file=./20140613_FreeBSD_9.2-RELEASE_ufs.qcow2,if=none,id=drive0,format=qcow2
  -device virtio-blk-pci,scsi=off,drive=drive0 -vnc 0.0.0.0:0 -net none

  I have tried many variations including different models of -machine
  and -cpu for the guest with no visible difference.

  A native FreeBSD installation on a host with a "v2" Xeon does not have
  the problem, nor do a paravirtualized FreeBSD guests under bhyve (the
  BSD legacy-free hypervisor) using the same FreeBSD disk images as on
  the Linux hosts. So it seems unlikely the cause is on the FreeBSD side
  of things.

  I would greatly appreciate any feedback or developer attention to
  this. I am happy to provide additional details, test patches, etc.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1329956/+subscriptions

Re: [Qemu-devel] [PATCH] pseries: Include missing pseries-2.5 compat properties in pseries-2.4

[Laszlo Ersek]

On 02/16/16 03:20, David Gibson wrote:
> Commit 4b23699 "pseries: Add pseries-2.6 machine type" added a new
> SPAPR_COMPAT_2_5 macro in the usual way.  However, it didn't add this
> macro to the existing SPAPR_COMPAT_2_4 macro so that pseries-2.4
> inherits newer compatibility properties which are needed for 2.5 and
> earlier.
> 
> This corrects the oversight.
> 
> Reported-by: Laszlo Ersek 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 276d6b4..e214a34 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2391,6 +2391,7 @@ DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
>   * pseries-2.4
>   */
>  #define SPAPR_COMPAT_2_4 \
> +SPAPR_COMPAT_2_5 \
>  HW_COMPAT_2_4
>  
>  static void spapr_machine_2_4_instance_options(MachineState *machine)
> 

Reviewed-by: Laszlo Ersek 

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Corey Minyard]

On 02/15/2016 12:40 PM, Marcel Apfelbaum wrote:

On 02/15/2016 07:17 PM, Cédric Le Goater wrote:

On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:

This patch provides a simple FRU support for the BMC simulator. FRUs
are loaded from a file which name is specified in the object
properties, each entry having a fixed size, also specified in the
properties. If the file is unknown or not accessible for some reason,
a unique entry of 1024 bytes is created as a default. Just enough to
start some simulation.

Signed-off-by: Cédric Le Goater 
---
   hw/ipmi/ipmi_bmc_sim.c | 140 
+

   1 file changed, 140 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 69318eb6b556..b0754893fc08 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -80,6 +80,9 @@
   #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
   #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE0x2B
   #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#define IPMI_CMD_WRITE_FRU_DATA   0x12
   #define IPMI_CMD_GET_SEL_INFO 0x40
   #define IPMI_CMD_GET_SEL_ALLOC_INFO   0x41
   #define IPMI_CMD_RESERVE_SEL  0x42
@@ -122,6 +125,13 @@ typedef struct IPMISdr {
   uint8_t overflow;
   } IPMISdr;

+typedef struct IPMIFru {
+char *filename;
+unsigned int nentries;
+uint16_t size;
+uint8_t *data;
+} IPMIFru;
+
   typedef struct IPMISensor {
   uint8_t status;
   uint8_t reading;
@@ -208,6 +218,7 @@ struct IPMIBmcSim {

   IPMISel sel;
   IPMISdr sdr;
+IPMIFru fru;
   IPMISensor sensors[MAX_SENSORS];
   char *sdr_filename;

@@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
   IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
   }

+static void get_fru_area_info(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int max_rsp_len)
+{
+uint8_t fruid;
+uint16_t fru_entry_size;
+
+IPMI_CHECK_CMD_LEN(3);


Hi,

This is little awkward for me. The cmd_len and rsp
parameters of the macro are implied.


hmm, I am not sure what you mean. Are you concerned by that fact
we could overflow rsp and cmd ?


Not really, no. Something more simple:

IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, 
cmd_len, rsp).

What bothers me is that both cmd_len and rsp are implied by the macro.


I would tend to agree.  The hidden stuff in these macros was really a 
bad idea in

hindsight.

-corey



In other words, we don't know what parameters IPMI_CHECK_CMD_LEN 
really has.
"3" is for sure not enough, so we need to guess or look a the macro 
definition

to see what it uses.

But again, maybe is only me.

Thanks,
Marcel




I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
of these commands and use an array of expected argument lengths.
For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not
exceeding max_rsp_len


Am I the only one this bothers?


+
+fruid = cmd[2];
+
+if (fruid >= ibs->fru.nentries) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+fru_entry_size = ibs->fru.size;
+
+IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
+IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
+IPMI_ADD_RSP_DATA(0x0);


Same here. By the way, do you have some spec for the above or
is an ad-hoc encoding of the fields? If yes, you could
add a reference for the spec.(This is also for the other functions 
in this patch)


Yes I will add the reference.

Thanks,

C.



Thanks,
Marcel


+}
+
+#define min(x, y) ((x) < (y) ? (x) : (y))
+#define max(x, y) ((x) > (y) ? (x) : (y))
+
+static void read_fru_data(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int max_rsp_len)
+{
+uint8_t fruid;
+uint16_t offset;
+int i;
+uint8_t *fru_entry;
+unsigned int count;
+
+IPMI_CHECK_CMD_LEN(5);
+
+fruid = cmd[2];
+offset = (cmd[3] | cmd[4] << 8);
+
+if (fruid >= ibs->fru.nentries) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+if (offset >= ibs->fru.size - 1) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+fru_entry = >fru.data[fruid * ibs->fru.size];
+
+count = min(cmd[5], ibs->fru.size - offset);
+
+IPMI_ADD_RSP_DATA(count & 0xff);
+for (i = 0; i < count; i++) {
+IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
+}
+}
+
+static void write_fru_data(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int 

Re: [Qemu-devel] [PATCH 04/12] docker: Add test runner

[Fam Zheng]

On Mon, 02/15 14:10, Alex Bennée wrote:
> >> There is a slight wrinkle as to what happens if the user wants to enter
> >> an interactive session for debugging. However that doesn't seem possible
> >> via the makefile and perhaps that is just as well. Perhaps that should
> >> be a helper script for the user?

Good idea.  The "docker run" invocation should be able to drop stdio into an
interactive shell, with Makefile. I'll take a look in v2.

Fam

[Qemu-devel] [PATCH] pseries: Include missing pseries-2.5 compat properties in pseries-2.4

[David Gibson]

Commit 4b23699 "pseries: Add pseries-2.6 machine type" added a new
SPAPR_COMPAT_2_5 macro in the usual way.  However, it didn't add this
macro to the existing SPAPR_COMPAT_2_4 macro so that pseries-2.4
inherits newer compatibility properties which are needed for 2.5 and
earlier.

This corrects the oversight.

Reported-by: Laszlo Ersek 
Signed-off-by: David Gibson 
---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 276d6b4..e214a34 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2391,6 +2391,7 @@ DEFINE_SPAPR_MACHINE(2_5, "2.5", false);
  * pseries-2.4
  */
 #define SPAPR_COMPAT_2_4 \
+SPAPR_COMPAT_2_5 \
 HW_COMPAT_2_4
 
 static void spapr_machine_2_4_instance_options(MachineState *machine)
-- 
2.5.0

Re: [Qemu-devel] [PATCH 11/12] .gitignore: Ignore temporary dockerfile

[Fam Zheng]

On Mon, 02/15 14:42, Alex Bennée wrote:
> 
> Fam Zheng  writes:
> 
> > Signed-off-by: Fam Zheng 
> > ---
> >  .gitignore | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/.gitignore b/.gitignore
> > index 88a80ff..a335b7b 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -109,3 +109,4 @@ cscope.*
> >  tags
> >  TAGS
> >  *~
> > +/tests/docker/*.docker.tmp
> 
> Is this left over from the python bindings called in docker_build? If so
> can we say so in the commit if we can't persuade the bindings to not
> clutter up the tree?

Okay, it has to be in the tree because of the "docker build context".
docker_build should remove this file at exit but this is just in case it fails
to.

I'll update the commit message.

Fam

Re: [Qemu-devel] [PATCH 00/12] tests: Introducing docker tests

[Fam Zheng]

On Mon, 02/15 17:59, Alex Bennée wrote:
> 
> Fam Zheng  writes:
> 
> > v1: Since RFC, addressed comments from reviewers, and improved a lot of 
> > things.
> > Thanks to Daniel, Eric, Paolo, Stefan, for the feedback.
> >
> > This series adds a new "docker" make target family to run tests in created
> > docker containers.
> >
> > To begin with, this can be a place to store standard env/command 
> > combinations to
> > build and test QEMU.
> 
> I think I'm done for this review pass. Please CC me on your next
> version.

Sure!

> 
> I've run a quick test on my ARM64 Debian Jessie box which predictably
> failed. It looks like this is a docker hub issue for one thing as:
> 
>   docker pull debian:stable
> 
> creates an x86_64 image on the arm64 box which predictably fails. I
> suspect the only solution is going to be storing our own built images on
> the docker hub and tweaking the tooling to take host architecture into
> account.

Yes I think that makes sense.

> 
> Do you have any non-x86 boxes to test against or are you going to need
> some help with non-x86 support?

You're right docker isn't platform aware. At the moment  I don't have a non-x86
box to test, it is great if you can help.

Thanks!

Fam

Re: [Qemu-devel] [PATCH v6 00/11] Add basic "detach" support for dump-guest-memory

[Peter Xu]

Still think this might help in some cases, so ping again...

Peter

On Wed, Dec 09, 2015 at 10:42:12AM +0800, Peter Xu wrote:
> v6 changes:
> - patch 10
>   - English error fix [Fam]
> - patch 11
>   - remove useless var: "not_used" [me]
> - all
>   - move patch 8 to the end to be patch 11 (v5 patches 9-11 become
>   v6 patches 8-10) [Eric]
> 
> v5 changes:
> - patch 1
>   - comment English fix [Fam]
> - patch 2
>   - pass has_detach=true always in hmp_dump_guest_memory [Paolo]
> - patch 3
>   - always use local_err and error_propagate() when need to check
> the result [Fam]
> - patch 8
>   - add "DumpQueryResult" in DUMP_COMPLETED event [Eric]
> (since DumpQueryResult is introduced in patch 10, so doing it in
> patch 10 for convenience. Please let me know if I should not do
> this, e.g., if patch re-ordering is required)
> 
> v4 changes:
> - patch 2:
>   - hmp: fix default value lost [Eric]
>   - English errors [Eric]
> - patch 3:
>   - use global DumpState, leverage C99 struct init [Paolo]
>   - English errors [Eric]
> - patch 5:
>   - more cleanup for dump_process [Paolo]
> - patch 8:
>   - make sure qmp-events.txt is sorted [Eric]
>   - enhance error_get_pretty() [Eric]
>   - emit DUMP_COMPLETED no matter detach or not
> - patch 10:
>   - use g_new0 to replace g_malloc0 [Eric]
>   - rename "written_bytes" to "completed", "total_bytes" to "total"
> [Eric]
>   - use atomic ops and [rw]mb to protect status read/write [Paolo]
> - patch 12:
>   - English errors [Eric]
>   - merge contents into older patches [Eric]
> 
> v3 changes (patch number corresponds to v2 patch set):
> - patch 1
>   - fix commit message. no memory leak, only code cleanup [Fam]
> - patch 2
>   - better documentation for "dump-guest-memory" (new patch 9) [Fam]
> - patch 3
>   - remove rcu lock/unlock in dump_init() [Fam, Paolo]
>   - embed mr pointer into GuestPhysBlock [Paolo]
>   - remove global dump state [Paolo]
> - patch 4
>   - fix memory leak for error [Fam]
>   - evt DUMP_COMPLETED data: change to an optional "*error" [Paolo]
> - patch 5
>   - fix documents [Fam]
>   - change "dump-query" to "query-dump", HMP to "info dump" [Paolo]
> - patch 6
>   - for query-dump command: define enum for DumpStatus, use "int"
> for written/total [Paolo]
> - all
>   - reorder the commits as suggested, no fake values [Paolo]
>   - split big commit into smaller ones [me]
> 
> v2 changes:
> - fixed English errors [Drew]
> - reordered the "detach" field, first make it optional, then make sure
>   it's order is consistent [Drew, Fam]
> - added doc for new detach flag [Eric]
> - collected error msg even detached [Drew]
> - added qmp event DUMP_COMPLETED to notify user [Eric, Fam]
> - added "dump-query" QMP & HMP commands to query dump status [Eric]
> - "stop" is not allowed when dump in background (also include
>   "cont" and "dump-guest-memory") [Fam]
> - added codes to calculate how many dump work finished, which could
>   be queried from "dump-query" [Laszlo]
> - added list to track all used MemoryRegion objects, also ref before
>   use [Paolo]
> - dump-guest-memory will be forbidden during incoming migrate [Paolo]
> - taking rcu lock when collecting memory info [Paolo]
> 
> Test Done:
> - QMP & HMP
>   - test default dump (sync), work as usual
>   - test detached dump, command return immediately.
>   - When dump finished, will receive event DUMP_COMPLETED. 
>   - test query-dump before/during/after dump
>   - test kdump with zlib compression, w/ and w/o detach
> - libvirt
>   - test "virsh dump --memory-only" with default format and
> kdump-zlib format, work as usual
> 
> Peter Xu (11):
>   dump-guest-memory: cleanup: removing dump_{error|cleanup}().
>   dump-guest-memory: add "detach" flag for QMP/HMP interfaces.
>   dump-guest-memory: using static DumpState, add DumpStatus
>   dump-guest-memory: add dump_in_progress() helper function
>   dump-guest-memory: introduce dump_process() helper function.
>   dump-guest-memory: disable dump when in INMIGRATE state
>   dump-guest-memory: add "detach" support
>   DumpState: adding total_size and written_size fields
>   Dump: add qmp command "query-dump"
>   Dump: add hmp command "info dump"
>   dump-guest-memory: add qmp event DUMP_COMPLETED
> 
>  docs/qmp-events.txt |  18 
>  dump.c  | 215 
> ++--
>  hmp-commands-info.hx|  14 +++
>  hmp-commands.hx |   5 +-
>  hmp.c   |  26 -
>  hmp.h   |   1 +
>  include/qemu-common.h   |   4 +
>  include/sysemu/dump.h   |  15 +++
>  include/sysemu/memory_mapping.h |   4 +
>  memory_mapping.c|   3 +
>  qapi-schema.json|  56 ++-
>  qapi/event.json |  16 +++
>  qmp-commands.hx |  31 +-
>  qmp.c   |  14 +++
>  14 files changed, 359 insertions(+), 63 deletions(-)
> 
> -- 
> 

[Qemu-devel] [PATCH v2 1/1] quorum: Change vote rules for 64 bits hash

[Changlong Xie]

If quorum has two children(A, B). A do flush sucessfully, but B flush failed.
We MUST choice A as winner rather than just pick anyone of them. Otherwise
the filesystem of guest will become read-only with following errors:

end_request: I/O error, dev vda, sector 11159960
Aborting journal on device vda3-8
EXT4-fs error (device vda3): ext4_journal_start_sb:327: Detected abort journal
EXT4-fs (vda3): Remounting filesystem read-only

Signed-off-by: Wen Congyang 
Signed-off-by: Changlong Xie 
---
 block/quorum.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 11cc60b..f094208 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -447,7 +447,8 @@ static int quorum_compute_hash(QuorumAIOCB *acb, int i, 
QuorumVoteValue *hash)
 return 0;
 }
 
-static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
+static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes,
+ bool vote_error)
 {
 int max = 0;
 QuorumVoteVersion *candidate, *winner = NULL;
@@ -456,6 +457,12 @@ static QuorumVoteVersion 
*quorum_get_vote_winner(QuorumVotes *votes)
 if (candidate->vote_count > max) {
 max = candidate->vote_count;
 winner = candidate;
+continue;
+}
+/* For 64 bit hash */
+if (vote_error && candidate->vote_count == max
+&& candidate->value.l == 0) {
+winner = candidate;
 }
 }
 
@@ -545,7 +552,7 @@ static int quorum_vote_error(QuorumAIOCB *acb)
 }
 
 if (error) {
-winner = quorum_get_vote_winner(_votes);
+winner = quorum_get_vote_winner(_votes, false);
 ret = winner->value.l;
 }
 
@@ -610,7 +617,7 @@ static bool quorum_vote(QuorumAIOCB *acb)
 }
 
 /* vote to select the most represented version */
-winner = quorum_get_vote_winner(>votes);
+winner = quorum_get_vote_winner(>votes, false);
 
 /* if the winner count is smaller than threshold the read fails */
 if (winner->vote_count < s->threshold) {
@@ -770,7 +777,7 @@ static coroutine_fn int quorum_co_flush(BlockDriverState 
*bs)
 quorum_count_vote(_votes, _value, i);
 }
 
-winner = quorum_get_vote_winner(_votes);
+winner = quorum_get_vote_winner(_votes, true);
 result = winner->value.l;
 
 quorum_free_vote_list(_votes);
-- 
1.9.3

[Qemu-devel] [PATCH v2 0/1] change quorum vote rules for 64-bits hash

[Changlong Xie]

If quorum has two children(A, B). A do flush sucessfully, but 
B flush failed. We MUST choice A as winner, otherwise we will 
get following errors:

{"timestamp": {"seconds": 1455641588, "microseconds": 415937}, 
"event": "BLOCK_IO_ERROR", "data": {"device": "colo-disk", 
"nospace": false, "reason": "Bad file descriptor", "operation": 
"write", "action": "report"}}

And the filesystem of guest became read-only with following errors:

[xxx] end_request: I/O error, dev vda, sector 11159960
[xxx] Aborting journal on device vda3-8
[xxx] EXT4-fs error (device vda3): ext4_journal_start_sb:327: Detected abort 
journal
[xxx] EXT4-fs (vda3): Remounting filesystem read-only

[Ref] http://lists.nongnu.org/archive/html/qemu-devel/2016-01/msg05342.html 

Changlong Xie (1):
  quorum: Change vote rules for 64 bits hash

 block/quorum.c | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
1.9.3

[Qemu-devel] [Bug 1490611] Re: Using qemu >=2.2.1 to convert raw->VHD (fixed) adds extra padding to the result file, which Microsoft Azure rejects as invalid

[Cole Mickens]

I bisected to this commit:
c70221df1f89953e85a3f1f96ceefbd6888bb55f

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1490611

Title:
  Using qemu >=2.2.1 to convert raw->VHD (fixed) adds extra padding to
  the result file, which Microsoft Azure rejects as invalid

Status in QEMU:
  Fix Released

Bug description:
  Starting with a raw disk image, using "qemu-img convert" to convert
  from raw to VHD results in the output VHD file's virtual size being
  aligned to the nearest 516096 bytes (16 heads x 63 sectors per head x
  512 bytes per sector), instead of preserving the input file's size as
  the output VHD's virtual disk size.

  Microsoft Azure requires that disk images (VHDs) submitted for upload
  have virtual sizes aligned to a megabyte boundary. (Ex. 4096MB,
  4097MB, 4098MB, etc. are OK, 4096.5MB is rejected with an error.) This
  is reflected in Microsoft's documentation: https://azure.microsoft.com
  /en-us/documentation/articles/virtual-machines-linux-create-upload-
  vhd-generic/

  This is reproducible with the following set of commands (including the
  Azure command line tools from https://github.com/Azure/azure-xplat-
  cli). For the following example, I used qemu version 2.2.1:

  $ dd if=/dev/zero of=source-disk.img bs=1M count=4096

  $ stat source-disk.img 
File: ‘source-disk.img’
Size: 4294967296  Blocks: 798656 IO Block: 4096   regular file
  Device: fc01h/64513dInode: 13247963Links: 1
  Access: (0644/-rw-r--r--)  Uid: ( 1000/  smkent)   Gid: ( 1000/  smkent)
  Access: 2015-08-18 09:48:02.613988480 -0700
  Modify: 2015-08-18 09:48:02.825985646 -0700
  Change: 2015-08-18 09:48:02.825985646 -0700
   Birth: -

  $ qemu-img convert -f raw -o subformat=fixed -O vpc source-disk.img
  dest-disk.vhd

  $ stat dest-disk.vhd 
File: ‘dest-disk.vhd’
Size: 4296499712  Blocks: 535216 IO Block: 4096   regular file
  Device: fc01h/64513dInode: 13247964Links: 1
  Access: (0644/-rw-r--r--)  Uid: ( 1000/  smkent)   Gid: ( 1000/  smkent)
  Access: 2015-08-18 09:50:22.252077624 -0700
  Modify: 2015-08-18 09:49:24.424868868 -0700
  Change: 2015-08-18 09:49:24.424868868 -0700
   Birth: -

  $ azure vm image create testimage1 dest-disk.vhd -o linux -l "West US"
  info:Executing command vm image create
  + Retrieving storage accounts 
 
  info:VHD size : 4097 MB
  info:Uploading 4195800.5 KB
  Requested:100.0% Completed:100.0% Running:   0 Time: 1m 0s Speed:  6744 KB/s 
  info:https://[redacted].blob.core.windows.net/vm-images/dest-disk.vhd was 
uploaded successfully
  error:   The VHD 
https://[redacted].blob.core.windows.net/vm-images/dest-disk.vhd has an 
unsupported virtual size of 4296499200 bytes.  The size must be a whole number 
(in MBs).
  info:Error information has been recorded to /home/smkent/.azure/azure.err
  error:   vm image create command failed

  I also ran the above commands using qemu 2.4.0, which resulted in the
  same error as the conversion behavior is the same.

  However, qemu 2.1.1 and earlier (including qemu 2.0.0 installed by
  Ubuntu 14.04) does not pad the virtual disk size during conversion.
  Using qemu-img convert from qemu versions <=2.1.1 results in a VHD
  that is exactly the size of the raw input file plus 512 bytes (for the
  VHD footer). Those qemu versions do not attempt to realign the disk.
  As a result, Azure accepts VHD files created using those versions of
  qemu-img convert for upload.

  Is there a reason why newer qemu realigns the converted VHD file? It
  would be useful if an option were added to disable this feature, as
  current versions of qemu cannot be used to create VHD files for Azure
  using Microsoft's official instructions.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1490611/+subscriptions

[Qemu-devel] [Bug 1490611] Re: Using qemu >=2.2.1 to convert raw->VHD (fixed) adds extra padding to the result file, which Microsoft Azure rejects as invalid

[Cole Mickens]

Kevin Wolf, I added you since you were the author of the commit that I
bisected this bug to. Can you advise at all on this bug? Thank you.

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1490611

Title:
  Using qemu >=2.2.1 to convert raw->VHD (fixed) adds extra padding to
  the result file, which Microsoft Azure rejects as invalid

Status in QEMU:
  Fix Released

Bug description:
  Starting with a raw disk image, using "qemu-img convert" to convert
  from raw to VHD results in the output VHD file's virtual size being
  aligned to the nearest 516096 bytes (16 heads x 63 sectors per head x
  512 bytes per sector), instead of preserving the input file's size as
  the output VHD's virtual disk size.

  Microsoft Azure requires that disk images (VHDs) submitted for upload
  have virtual sizes aligned to a megabyte boundary. (Ex. 4096MB,
  4097MB, 4098MB, etc. are OK, 4096.5MB is rejected with an error.) This
  is reflected in Microsoft's documentation: https://azure.microsoft.com
  /en-us/documentation/articles/virtual-machines-linux-create-upload-
  vhd-generic/

  This is reproducible with the following set of commands (including the
  Azure command line tools from https://github.com/Azure/azure-xplat-
  cli). For the following example, I used qemu version 2.2.1:

  $ dd if=/dev/zero of=source-disk.img bs=1M count=4096

  $ stat source-disk.img 
File: ‘source-disk.img’
Size: 4294967296  Blocks: 798656 IO Block: 4096   regular file
  Device: fc01h/64513dInode: 13247963Links: 1
  Access: (0644/-rw-r--r--)  Uid: ( 1000/  smkent)   Gid: ( 1000/  smkent)
  Access: 2015-08-18 09:48:02.613988480 -0700
  Modify: 2015-08-18 09:48:02.825985646 -0700
  Change: 2015-08-18 09:48:02.825985646 -0700
   Birth: -

  $ qemu-img convert -f raw -o subformat=fixed -O vpc source-disk.img
  dest-disk.vhd

  $ stat dest-disk.vhd 
File: ‘dest-disk.vhd’
Size: 4296499712  Blocks: 535216 IO Block: 4096   regular file
  Device: fc01h/64513dInode: 13247964Links: 1
  Access: (0644/-rw-r--r--)  Uid: ( 1000/  smkent)   Gid: ( 1000/  smkent)
  Access: 2015-08-18 09:50:22.252077624 -0700
  Modify: 2015-08-18 09:49:24.424868868 -0700
  Change: 2015-08-18 09:49:24.424868868 -0700
   Birth: -

  $ azure vm image create testimage1 dest-disk.vhd -o linux -l "West US"
  info:Executing command vm image create
  + Retrieving storage accounts 
 
  info:VHD size : 4097 MB
  info:Uploading 4195800.5 KB
  Requested:100.0% Completed:100.0% Running:   0 Time: 1m 0s Speed:  6744 KB/s 
  info:https://[redacted].blob.core.windows.net/vm-images/dest-disk.vhd was 
uploaded successfully
  error:   The VHD 
https://[redacted].blob.core.windows.net/vm-images/dest-disk.vhd has an 
unsupported virtual size of 4296499200 bytes.  The size must be a whole number 
(in MBs).
  info:Error information has been recorded to /home/smkent/.azure/azure.err
  error:   vm image create command failed

  I also ran the above commands using qemu 2.4.0, which resulted in the
  same error as the conversion behavior is the same.

  However, qemu 2.1.1 and earlier (including qemu 2.0.0 installed by
  Ubuntu 14.04) does not pad the virtual disk size during conversion.
  Using qemu-img convert from qemu versions <=2.1.1 results in a VHD
  that is exactly the size of the raw input file plus 512 bytes (for the
  VHD footer). Those qemu versions do not attempt to realign the disk.
  As a result, Azure accepts VHD files created using those versions of
  qemu-img convert for upload.

  Is there a reason why newer qemu realigns the converted VHD file? It
  would be useful if an option were added to disable this feature, as
  current versions of qemu cannot be used to create VHD files for Azure
  using Microsoft's official instructions.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1490611/+subscriptions

Re: [Qemu-devel] [PATCH 1/1] quorum: change vote rules for 64 bits hash

[Changlong Xie]

On 02/16/2016 01:10 AM, Eric Blake wrote:

On 02/15/2016 02:52 AM, Changlong Xie wrote:

Before:
1) vote_count > max: winner = candidate, update max
2) vote_count <= max: do nothing
Current:
1) vote_count > max: winner = candidate, update max
2) vote_count = max: compare the value of winner with
candidate, if candidate->value.l == 0, winner = candidate,
else do nothing
3) vote_count < max: do nothing


This says what you did, but not why.  Can you demonstrate a scenario
that is broken without the patch, as part of your commit message?


Surely, will do in next version.





Signed-off-by: Wen Congyang 
Signed-off-by: Changlong Xie 
---
  block/quorum.c | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index a5ae4b8..e431ff4 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -446,7 +446,7 @@ static int quorum_compute_hash(QuorumAIOCB *acb, int i, 
QuorumVoteValue *hash)
  return 0;
  }

-static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes)
+static QuorumVoteVersion *quorum_get_vote_winner(QuorumVotes *votes, bool 
vote_error)


Long line. Please wrap things to stay within 80 columns.



Ok


  {
  int max = 0;
  QuorumVoteVersion *candidate, *winner = NULL;
@@ -455,6 +455,12 @@ static QuorumVoteVersion 
*quorum_get_vote_winner(QuorumVotes *votes)
  if (candidate->vote_count > max) {
  max = candidate->vote_count;
  winner = candidate;
+continue;
+}
+/* For 64 bit hash */
+if (vote_error == true && candidate->vote_count == max


s/ == true// (no need to do a redundant comparison of a bool against a
bool).


Ok

Thanks
-Xie

Re: [Qemu-devel] [PATCH 06/12] docker: Add basic test

[Fam Zheng]

On Mon, 02/15 14:52, Alex Bennée wrote:
> 
> Peter Maydell  writes:
> 
> > On 15 February 2016 at 14:34, Alex Bennée  wrote:
> >>
> >> Fam Zheng  writes:
> >>> +cd $(mktemp -d)
> >>> +mkdir build
> >>> +mkdir install
> >>> +cd build
> >>> +build_qemu --target-list=x86_64-softmmu --prefix="${pwd}/install"
> >
> > Why restrict the target list ?

There is no particular reason, just I wanted to keep the "basic test" basic.
I'll rename it to "quick".

That said, this is only one of many possible tests, multiplied by the number of
images we ship. We need to be careful with the matrix, because the point of
having these tests is to run them frequently (either manually by developers or
automatically by CI on every patch series or even every patch). 10+ minutes for
a single test/image combination just makes that hard.

> >
> >>> +make check $MAKEFLAGS
> >>> +make install
> >>
> >> From my excursions last week into the tests directory I discovered the
> >> unit tests are built for a number of different qtest binaries. For
> >> completeness we should probably include the whole list:
> >>
> >> #+name: qtest-targets
> >> #+begin_src sh :dir ~/lsrc/qemu/qemu.git :results scalar
> >> grep -E "check-qtest-[[:alnum:]]+-y " tests/Makefile | cut -d " " -f 1 | 
> >> sort -u
> >> #+end_src
> >>
> >> #+RESULTS: qtest-targets
> >> #+begin_example
> >> check-qtest-arm-y
> >> check-qtest-generic-y
> >> check-qtest-i386-y
> >> check-qtest-ipack-y
> >> check-qtest-microblazeel-y
> >> check-qtest-mips64el-y
> >> check-qtest-mips64-y
> >> check-qtest-mips-y
> >> check-qtest-pci-y
> >> check-qtest-ppc64-y
> >> check-qtest-ppc-y
> >> check-qtest-sh4eb-y
> >> check-qtest-sh4-y
> >> #check-qtest-sparc64-y
> >> check-qtest-sparc64-y
> >> #check-qtest-sparc-y
> >> check-qtest-virtioserial-y
> >> check-qtest-virtio-y
> >> check-qtest-x86_64-y
> >> check-qtest-xtensaeb-y
> >> check-qtest-y
> >> #+end_example
> >
> > I'm having difficulty figuring out what you're proposing here,
> > but it looks like you're suggesting listing all the check-qtest-*
> > test names again here, which seems worth avoiding. We should
> > just do a build and make check and let that take care of
> > running all the tests.
> 
> Well there is a halfway house between building one target and building
> all possible targets. Not all the softmmu targets include additional
> tests. Having said that it is probably simpler as you say to just build
> everything.

Sure, but I'd create a separate "full" test for that.

Fam

[Qemu-devel] [PATCH v10 11/13] qapi: Don't box branches of flat unions

[Eric Blake]

There's no reason to do two malloc's for a flat union; let's just
inline the branch struct directly into the C union branch of the
flat union.

Surprisingly, fewer clients were actually using explicit references
to the branch types in comparison to the number of flat unions
thus modified.

This lets us reduce the hack in qapi-types:gen_variants() added in
the previous patch; we no longer need to distinguish between
alternates and flat unions.  It also lets us get rid of all traces
of 'visit_type_implicit_FOO()' in qapi-visit, and reduce one (but
not all) special cases of simplie unions.

Unfortunately, simple unions are not as easy to convert; because
we are special-casing the hidden implicit type with a single 'data'
member, we really DO need to keep calling another layer of
visit_start_struct(), with a second malloc.  Hence,
gen_visit_fields_decl() has to special case implicit types (the
type for a simple union variant).

Note that after this patch, the only remaining use of
visit_start_implicit_struct() is for alternate types; the next
couple of patches will do further cleanups based on that fact.

Signed-off-by: Eric Blake 

---
v10: new patch

If anything, we could match our simple union wire format more closely
by teaching qapi-types to expose implicit types inline, and write:

struct SU {
SUKind type;
union {
struct {
Branch1 *data;
} branch1;
struct {
Branch2 *data;
} branch2;
} u;
};

where we would then access su.u.branch1.data->member instead of
the current su.u.branch1->member.
---
 scripts/qapi-types.py   | 10 +
 scripts/qapi-visit.py   | 45 +++--
 cpus.c  | 18 ++---
 hmp.c   | 12 +--
 tests/test-qmp-input-visitor.c  |  2 +-
 tests/test-qmp-output-visitor.c |  5 ++---
 6 files changed, 23 insertions(+), 69 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index aba2847..5071817 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,14 +116,6 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
%(c_name)s *obj)


 def gen_variants(variants):
-# HACK: Determine if this is an alternate (at least one variant
-# is not an object); unions have all branches as objects.
-inline = False
-for v in variants.variants:
-if not isinstance(v.type, QAPISchemaObjectType):
-inline = True
-break
-
 # FIXME: What purpose does data serve, besides preventing a union that
 # has a branch named 'data'? We use it in qapi-visit.py to decide
 # whether to bypass the switch statement if visiting the discriminator
@@ -144,7 +136,7 @@ def gen_variants(variants):
 ret += mcgen('''
 %(c_type)s %(c_name)s;
 ''',
- c_type=typ.c_type(is_member=inline),
+ c_type=typ.c_type(is_member=not var.simple_union_type()),
  c_name=c_name(var.name))

 ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 948bde4..68354d8 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,6 @@
 from qapi import *
 import re

-# visit_type_implicit_FOO() is emitted as needed; track if it has already
-# been output.
-implicit_structs_seen = set()
-
 # visit_type_alternate_FOO() is emitted as needed; track if it has already
 # been output.
 alternate_structs_seen = set()
@@ -39,40 +35,15 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_type)sobj, Error **


 def gen_visit_fields_decl(typ):
-ret = ''
-if typ.name not in struct_fields_seen:
-ret += mcgen('''
+if typ.is_implicit() or typ.name in struct_fields_seen:
+return ''
+struct_fields_seen.add(typ.name)
+
+return mcgen('''

 static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
**errp);
 ''',
- c_type=typ.c_name())
-struct_fields_seen.add(typ.name)
-return ret
-
-
-def gen_visit_implicit_struct(typ):
-if typ in implicit_structs_seen:
-return ''
-implicit_structs_seen.add(typ)
-
-ret = gen_visit_fields_decl(typ)
-
-ret += mcgen('''
-
-static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error 
**errp)
-{
-Error *err = NULL;
-
-visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), );
-if (!err) {
-visit_type_%(c_type)s_fields(v, *obj, errp);
-visit_end_implicit_struct(v);
-}
-error_propagate(errp, err);
-}
-''',
  c_type=typ.c_name())
-return ret


 def gen_visit_alternate_struct(typ):
@@ -250,9 +221,7 @@ def gen_visit_object(name, base, members, variants):

 if variants:
 for var in variants.variants:
-# Ugly special case for simple union TODO get rid of it
-if not var.simple_union_type():
-ret += 

[Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order

[Eric Blake]

Right now, we emit the branches of union types as a boxed pointer,
and it suffices to have a forward declaration of the type.  However,
a future patch will swap things to directly use the branch type,
instead of hiding it behind a pointer.  For this to work, the
compiler needs the full definition of the type, not just a forward
declaration, prior to the union that is including the branch type.
This patch just adds topological sorting to hoist all types
mentioned in a branch of a union to be fully declared before the
union itself.  The sort is always possible, because we do not
allow circular union types that include themselves as a direct
branch (it is, however, still possible to include a branch type
that itself has a pointer to the union, for a type that can
indirectly recursively nest itself - that remains safe, because
that the member of the branch type will remain a pointer, and the
QMP representation of such a type adds another {} for each recurring
layer of the union type).

Signed-off-by: Eric Blake 

---
v10: new patch
---
 scripts/qapi-types.py | 23 ---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 83f230a..2f23432 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -2,7 +2,7 @@
 # QAPI types generator
 #
 # Copyright IBM, Corp. 2011
-# Copyright (c) 2013-2015 Red Hat Inc.
+# Copyright (c) 2013-2016 Red Hat Inc.
 #
 # Authors:
 #  Anthony Liguori 
@@ -14,6 +14,11 @@
 from qapi import *


+# variants must be emitted before their container; track what has already
+# been output
+objects_seen = set()
+
+
 def gen_fwd_object_or_array(name):
 return mcgen('''

@@ -49,11 +54,23 @@ def gen_struct_fields(members):


 def gen_object(name, base, members, variants):
-ret = mcgen('''
+if name in objects_seen:
+return ''
+objects_seen.add(name)
+
+ret = ''
+if variants:
+for v in variants.variants:
+if isinstance(v.type, QAPISchemaObjectType) and \
+   not v.type.is_implicit():
+ret += gen_object(v.type.name, v.type.base,
+  v.type.local_members, v.type.variants)
+
+ret += mcgen('''

 struct %(c_name)s {
 ''',
-c_name=c_name(name))
+ c_name=c_name(name))

 if base:
 ret += mcgen('''
-- 
2.5.0

[Qemu-devel] [PATCH v10 13/13] qapi: Change visit_start_implicit_struct to visit_start_alternate

[Eric Blake]

After recent changes, the only remaining use of
visit_start_implicit_struct() is for allocating the space needed
when visiting an alternate.  Since the term 'implicit struct' is
hard to explain, rename the function to its current usage.  While
at it, we can merge the functionality of visit_get_next_type()
into the same function, making it more like visit_start_struct().

Generated code is now slightly smaller:

| {
| Error *err = NULL;
|
|-visit_start_implicit_struct(v, (void**) obj, sizeof(BlockdevRef), );
|+visit_start_alternate(v, name, (GenericAlternate **)obj, sizeof(**obj),
|+  true, );
| if (err) {
| goto out;
| }
|-visit_get_next_type(v, name, &(*obj)->type, true, );
|-if (err) {
|-goto out_obj;
|-}
| switch ((*obj)->type) {
| case QTYPE_QDICT:
| visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, 
);
| break;
...
| }
|-out_obj:
|-visit_end_implicit_struct(v);
|+visit_end_alternate(v);
| out:
| error_propagate(errp, err);
| }

Signed-off-by: Eric Blake 

---
v10: new patch
---
 include/qapi/visitor.h  | 50 ++---
 include/qapi/visitor-impl.h | 17 +++
 scripts/qapi-visit.py   | 10 +++--
 qapi/qapi-visit-core.c  | 40 +++-
 qapi/qapi-dealloc-visitor.c | 13 ++--
 qapi/qmp-input-visitor.c| 24 --
 6 files changed, 82 insertions(+), 72 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index b8ae1b5..83cad74 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,7 +19,6 @@
 #include "qapi/error.h"
 #include 

-
 /* This struct is layout-compatible with all other *List structs
  * created by the qapi generator.  It is used as a typical
  * singly-linked list. */
@@ -28,17 +27,52 @@ typedef struct GenericList {
 char padding[];
 } GenericList;

+/* This struct is layout-compatible with all Alternate types
+ * created by the qapi generator. */
+typedef struct GenericAlternate {
+QType type;
+char padding[];
+} GenericAlternate;
+
 void visit_start_struct(Visitor *v, const char *name, void **obj,
 size_t size, Error **errp);
 void visit_end_struct(Visitor *v, Error **errp);
-void visit_start_implicit_struct(Visitor *v, void **obj, size_t size,
- Error **errp);
-void visit_end_implicit_struct(Visitor *v);

 void visit_start_list(Visitor *v, const char *name, Error **errp);
 GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
 void visit_end_list(Visitor *v);

+/*
+ * Start the visit of an alternate @obj with the given @size.
+ *
+ * @name specifies the relationship to the containing struct (ignored
+ * for a top level visit, the name of the key if this alternate is
+ * part of an object, or NULL if this alternate is part of a list).
+ *
+ * @obj must not be NULL. Input visitors will allocate @obj and
+ * determine the qtype of the next thing to be visited, stored in
+ * (*@obj)->type.  Other visitors will leave @obj unchanged.
+ *
+ * If @promote_int, treat integers as QTYPE_FLOAT.
+ *
+ * If successful, this must be paired with visit_end_alternate(), even
+ * if visiting the contents of the alternate fails.
+ */
+void visit_start_alternate(Visitor *v, const char *name,
+   GenericAlternate **obj, size_t size,
+   bool promote_int, Error **errp);
+
+/*
+ * Finish visiting an alternate type.
+ *
+ * Must be called after a successful visit_start_alternate(), even if
+ * an error occurred in the meantime.
+ *
+ * TODO: Should all the visit_end_* interfaces take obj parameter, so
+ * that dealloc visitor need not track what was passed in visit_start?
+ */
+void visit_end_alternate(Visitor *v);
+
 /**
  * Check if an optional member @name of an object needs visiting.
  * For input visitors, set *@present according to whether the
@@ -47,14 +81,6 @@ void visit_end_list(Visitor *v);
  */
 bool visit_optional(Visitor *v, const char *name, bool *present);

-/**
- * Determine the qtype of the item @name in the current object visit.
- * For input visitors, set *@type to the correct qtype of a qapi
- * alternate type; for other visitors, leave *@type unchanged.
- * If @promote_int, treat integers as QTYPE_FLOAT.
- */
-void visit_get_next_type(Visitor *v, const char *name, QType *type,
- bool promote_int, Error **errp);
 void visit_type_enum(Visitor *v, const char *name, int *obj,
  const char *const strings[], Error **errp);
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp);
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index c4af3e0..6a1ddfb 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -22,22 +22,23 @@ struct Visitor
  

[Qemu-devel] [PATCH v10 08/13] qapi: Adjust layout of FooList types

[Eric Blake]

By sticking the next pointer first, we don't need a union with
64-bit padding for smaller types.  On 32-bit platforms, this
can reduce the size of uint8List from 16 bytes (or 12, depending
on whether 64-bit ints can tolerate 4-byte alignment) down to 8.
It has no effect on 64-bit platforms (where alignment still
dictates a 16-byte struct); but fewer anonymous unions is still
a win in my book.

It requires visit_next_list() to gain a size parameter, to know
what size element to allocate; comparable to the size parameter
of visit_start_struct().

I debated about going one step further, to allow for fewer casts,
by doing:
typedef GenericList GenericList;
struct GenericList {
GenericList *next;
};
struct FooList {
GenericList base;
Foo value;
};
so that you convert to 'GenericList *' by '>base', and
back by 'container_of(generic, GenericList, base)' (as opposed to
the existing '(GenericList *)foolist' and '(FooList *)generic').
But doing that would require hoisting the declaration of
GenericList prior to inclusion of qapi-types.h, rather than its
current spot in visitor.h; it also makes iteration a bit more
verbose through 'foolist->base.next' instead of 'foolist->next'.

Signed-off-by: Eric Blake 

---
v10: hoist earlier in series
v9: no change
v8: rebase to earlier changes
v7: new patch; probably too invasive to be worth it, especially if
we can't prove that our current size for uint8List is a bottleneck
---
 include/qapi/visitor.h   | 14 +++---
 include/qapi/visitor-impl.h  |  2 +-
 scripts/qapi-types.py|  5 +
 scripts/qapi-visit.py|  2 +-
 qapi/qapi-visit-core.c   |  4 ++--
 qapi/opts-visitor.c  |  4 ++--
 qapi/qapi-dealloc-visitor.c  |  3 ++-
 qapi/qmp-input-visitor.c |  5 +++--
 qapi/qmp-output-visitor.c|  3 ++-
 qapi/string-input-visitor.c  |  4 ++--
 qapi/string-output-visitor.c |  2 +-
 11 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 5e581dc..c131a32 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -19,13 +19,13 @@
 #include "qapi/error.h"
 #include 

-typedef struct GenericList
-{
-union {
-void *value;
-uint64_t padding;
-};
+
+/* This struct is layout-compatible with all other *List structs
+ * created by the qapi generator.  It is used as a typical
+ * singly-linked list. */
+typedef struct GenericList {
 struct GenericList *next;
+char padding[];
 } GenericList;

 void visit_start_struct(Visitor *v, const char *name, void **obj,
@@ -36,7 +36,7 @@ void visit_start_implicit_struct(Visitor *v, void **obj, 
size_t size,
 void visit_end_implicit_struct(Visitor *v);

 void visit_start_list(Visitor *v, const char *name, Error **errp);
-GenericList *visit_next_list(Visitor *v, GenericList **list);
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size);
 void visit_end_list(Visitor *v);

 /**
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index ea252f8..7905a28 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -29,7 +29,7 @@ struct Visitor

 void (*start_list)(Visitor *v, const char *name, Error **errp);
 /* Must be set */
-GenericList *(*next_list)(Visitor *v, GenericList **list);
+GenericList *(*next_list)(Visitor *v, GenericList **list, size_t size);
 /* Must be set */
 void (*end_list)(Visitor *v);

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 7b0dca8..83f230a 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -26,11 +26,8 @@ def gen_array(name, element_type):
 return mcgen('''

 struct %(c_name)s {
-union {
-%(c_type)s value;
-uint64_t padding;
-};
 %(c_name)s *next;
+%(c_type)s value;
 };
 ''',
  c_name=c_name(name), c_type=element_type.c_type())
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 177dfc4..9ff0337 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -129,7 +129,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 }

 for (prev = (GenericList **)obj;
- !err && (i = visit_next_list(v, prev)) != NULL;
+ !err && (i = visit_next_list(v, prev, sizeof(**obj))) != NULL;
  prev = ) {
 %(c_name)s *native_i = (%(c_name)s *)i;
 visit_type_%(c_elt_type)s(v, NULL, _i->value, );
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index f856286..6fa66f1 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -50,9 +50,9 @@ void visit_start_list(Visitor *v, const char *name, Error 
**errp)
 v->start_list(v, name, errp);
 }

-GenericList *visit_next_list(Visitor *v, GenericList **list)
+GenericList *visit_next_list(Visitor *v, GenericList **list, size_t size)
 {
-return v->next_list(v, list);
+return v->next_list(v, list, size);
 }

 void 

[Qemu-devel] [PATCH v10 07/13] qapi-visit: Less indirection in visit_type_Foo_fields()

[Eric Blake]

We were passing 'Foo **obj' to the internal helper function, but
all uses within the helper were via reads of '*obj'.  Refactor
things to pass one less level of indirection, by having the
callers dereference before calling.

For an example of the generated code change:

|-static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo **obj, 
Error **errp)
|+static void visit_type_BalloonInfo_fields(Visitor *v, BalloonInfo *obj, Error 
**errp)
| {
| Error *err = NULL;
|
|-visit_type_int(v, "actual", &(*obj)->actual, );
|+visit_type_int(v, "actual", >actual, );
| error_propagate(errp, err);
| }
|
|@@ -261,7 +261,7 @@ void visit_type_BalloonInfo(Visitor *v,
| if (!*obj) {
| goto out_obj;
| }
|-visit_type_BalloonInfo_fields(v, obj, );
|+visit_type_BalloonInfo_fields(v, *obj, );
| out_obj:

The refactoring will also make it easier to reuse the helpers in
a future patch when implicit structs are stored directly in the
parent struct rather than boxed through a pointer.

Signed-off-by: Eric Blake 

---
v10: new patch
---
 scripts/qapi-visit.py | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 786fe57..177dfc4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -39,7 +39,7 @@ def gen_visit_fields_decl(typ):
 if typ.name not in struct_fields_seen:
 ret += mcgen('''

-static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error 
**errp);
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error 
**errp);
 ''',
  c_type=typ.c_name())
 struct_fields_seen.add(typ.name)
@@ -61,7 +61,7 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
%(c_type)s **obj, Error *

 visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), );
 if (!err) {
-visit_type_%(c_type)s_fields(v, obj, errp);
+visit_type_%(c_type)s_fields(v, *obj, errp);
 visit_end_implicit_struct(v);
 }
 error_propagate(errp, err);
@@ -80,7 +80,7 @@ def gen_visit_struct_fields(name, base, members):
 struct_fields_seen.add(name)
 ret += mcgen('''

-static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error 
**errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error 
**errp)
 {
 Error *err = NULL;

@@ -89,13 +89,13 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
%(c_name)s **obj, Error **e

 if base:
 ret += mcgen('''
-visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, );
+visit_type_%(c_type)s_fields(v, (%(c_type)s *)obj, );
 ''',
  c_type=base.c_name())
 if members:
 ret += gen_err_check()

-ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)
+ret += gen_visit_fields(members, prefix='obj->', skiplast=True)

 # 'goto out' produced for base, and by gen_visit_fields() for each
 # member except the last
@@ -232,7 +232,7 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if (!*obj) {
 goto out_obj;
 }
-visit_type_%(c_name)s_fields(v, obj, );
+visit_type_%(c_name)s_fields(v, *obj, );
 ''',
  c_name=c_name(name))
 if variants:
-- 
2.5.0

[Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union()

[Eric Blake]

Commit cee2dedb noticed that if you have a partial flat union
(such as if an input parse failed due to a missing
discriminator), calling the dealloc visitor could result in
trying to dereference a NULL pointer if we attempted to visit
an object branch without an earlier successful call to
visit_start_implicit_struct() allocating the pointer for that
branch. But the "fix" it implemented requires the use of a
'.data' member in the union, which may or may not be the same
size as other branches of the union (consider a 32-bit platform
where one of the branches is an int64), which feels fairly dirty.
Plus, as mentioned in that commit, it only works if you can
assume that '.data' would be zero-initialized even if '.kind' was
uninitialized, which is rather poor logic: our usage of
visit_start_struct() happens to zero-initialize both fields,
which means '.kind' is never truly uninitialized - but if we
changed visit_start_struct() to use g_new() instead of g_new0(),
then '.data' would not be any more reliable as a condition on
whether to visit the branch matching '.kind', regardless of
whether '.kind' was 0).

Menawhile, now that we have just inlined the fields of all flat
unions, there is no longer the possibility of a null pointer to
dereference in the first place.  Where the branch structure used
to be separately allocated by visit_start_implicit_struct(), it
is now just pointing to a subset of the memory already
zero-allocated by visit_start_struct().

Thus, we can instead fix things to delete the misguided
visit_start_union(), as it is no longer providing any benefit.
And it finishes the cleanup we started in commit 7c91aabd when
we deleted visit_end_union().  Generated code changes as follows:

|@@ -2366,9 +2363,6 @@ void visit_type_ChardevBackend(Visitor *
| if (err) {
| goto out_obj;
| }
|-if (!visit_start_union(v, !!(*obj)->u.data, ) || err) {
|-goto out_obj;
|-}
| switch ((*obj)->type) {
| case CHARDEV_BACKEND_KIND_FILE:
| visit_type_ChardevFile(v, "data", &(*obj)->u.file, );

Signed-off-by: Eric Blake 

---
v10: retitle, hoist earlier in series, rebase, drop R-b
v9: no change
v8: rebase to 'name' motion
v7: rebase to earlier context changes, simplify 'obj && !*obj'
condition based on contract
v6: rebase due to deferring 7/46, and gen_err_check() improvements;
rewrite gen_visit_implicit_struct() more like other patterns
---
 include/qapi/visitor.h  |  1 -
 include/qapi/visitor-impl.h |  2 --
 scripts/qapi-visit.py   |  3 ---
 qapi/qapi-visit-core.c  |  8 
 qapi/qapi-dealloc-visitor.c | 26 --
 5 files changed, 40 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index c131a32..b8ae1b5 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -80,6 +80,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, 
Error **errp);
 void visit_type_number(Visitor *v, const char *name, double *obj,
Error **errp);
 void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp);
-bool visit_start_union(Visitor *v, bool data_present, Error **errp);

 #endif
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 7905a28..c4af3e0 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -58,8 +58,6 @@ struct Visitor

 /* May be NULL; most useful for input visitors. */
 void (*optional)(Visitor *v, const char *name, bool *present);
-
-bool (*start_union)(Visitor *v, bool data_present, Error **errp);
 };

 void input_type_enum(Visitor *v, const char *name, int *obj,
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 68354d8..02f0122 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if variants:
 ret += gen_err_check(label='out_obj')
 ret += mcgen('''
-if (!visit_start_union(v, !!(*obj)->u.data, ) || err) {
-goto out_obj;
-}
 switch ((*obj)->%(c_name)s) {
 ''',
  c_name=c_name(variants.tag_member.name))
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 6fa66f1..976106e 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -60,14 +60,6 @@ void visit_end_list(Visitor *v)
 v->end_list(v);
 }

-bool visit_start_union(Visitor *v, bool data_present, Error **errp)
-{
-if (v->start_union) {
-return v->start_union(v, data_present, errp);
-}
-return true;
-}
-
 bool visit_optional(Visitor *v, const char *name, bool *present)
 {
 if (v->optional) {
diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
index 6667e8c..4eae555 100644
--- a/qapi/qapi-dealloc-visitor.c
+++ b/qapi/qapi-dealloc-visitor.c
@@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char 
*name, int *obj,
 {
 }

-/* If there's no data 

[Qemu-devel] [PATCH v10 03/13] qapi: Reposition error checks in gen_visit_fields()

[Eric Blake]

Change the generated code for error checking after an optional
field visit to have one less level of indentation.  There is no
real semantic change (the compiler should be smart enough to
realize that err does not change if visit_optional() returns
false, and optimize out unneeded branching in that case); the
main reason for the change is making the next patch (removing
a pointless goto) easier to read.

| if (visit_optional(v, "node-name", _node_name)) {
| visit_type_str(v, "node-name", (char **)_name, );
|-if (err) {
|-goto out_obj;
|-}
|+}
|+if (err) {
|+goto out_obj;
| }

Signed-off-by: Eric Blake 

---
v10: new patch
---
 scripts/qapi.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f97236f..fab5001 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1673,13 +1673,14 @@ def gen_visit_fields(members, prefix='', 
need_cast=False, skiperr=False,
  c_type=memb.type.c_name(), prefix=prefix, cast=cast,
  c_name=c_name(memb.name), name=memb.name,
  errp=errparg)
-ret += gen_err_check(skiperr=skiperr, label=label)

 if memb.optional:
 pop_indent()
 ret += mcgen('''
 }
 ''')
+ret += gen_err_check(skiperr=skiperr, label=label)
+
 return ret


-- 
2.5.0

[Qemu-devel] [PATCH v10 05/13] qapi-visit: Simplify how we visit common union members

[Eric Blake]

From: Markus Armbruster 

For a simple union SU, gen_visit_union() generates a visit of its
single tag member, like this:

visit_type_SUKind(v, "type", &(*obj)->type, );

For a flat union FU with base B, it generates a visit of its base
fields:

visit_type_B_fields(v, (B **)obj, );

Instead, we can simply visit the common members using the same fields
visit function we use for structs, generated with
gen_visit_struct_fields().  This function visits the base if any, then
the local members.

For a simple union SU, visit_type_SU_fields() contains exactly the old
tag member visit, because there is no base, and the tag member is the
only member.  For instance, the code generated for qapi-schema.json's
KeyValue changes like this:

+static void visit_type_KeyValue_fields(Visitor *v, KeyValue **obj, Error 
**errp)
+{
+Error *err = NULL;
+
+visit_type_KeyValueKind(v, "type", &(*obj)->type, );
+error_propagate(errp, err);
+}
+
 void visit_type_KeyValue(Visitor *v, const char *name, KeyValue **obj, 
Error **errp)
 {
 Error *err = NULL;
@@ -4863,7 +4911,7 @@ void visit_type_KeyValue(Visitor *v, con
 if (!*obj) {
 goto out_obj;
 }
-visit_type_KeyValueKind(v, "type", &(*obj)->type, );
+visit_type_KeyValue_fields(v, obj, );
 if (err) {
 goto out_obj;
 }

For a flat union FU, visit_type_FU_fields() contains exactly the old
base fields visit, because there is a base, but no members.  For
instance, the code generated for qapi-schema.json's CpuInfo changes
like this:

 static void visit_type_CpuInfoBase_fields(Visitor *v, CpuInfoBase **obj, 
Error **errp);

+static void visit_type_CpuInfo_fields(Visitor *v, CpuInfo **obj, Error 
**errp)
+{
+Error *err = NULL;
+
+visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, );
+error_propagate(errp, err);
+}
+
 static void visit_type_CpuInfoX86_fields(Visitor *v, CpuInfoX86 **obj, 
Error **errp)
...
@@ -3485,7 +3509,7 @@ void visit_type_CpuInfo(Visitor *v, cons
 if (!*obj) {
 goto out_obj;
 }
-visit_type_CpuInfoBase_fields(v, (CpuInfoBase **)obj, );
+visit_type_CpuInfo_fields(v, obj, );
 if (err) {
 goto out_obj;
 }

As you see, the generated code grows a bit, but in practice, it's lost
in the noise: qapi-schema.json's qapi-visit.c gains roughly 1%.

This simplification became possible with commit 441cbac "qapi-visit:
Convert to QAPISchemaVisitor, fixing bugs".  It's a step towards
unifying gen_struct() and gen_union().

Signed-off-by: Markus Armbruster 
Message-Id: <1453902888-20457-2-git-send-email-arm...@redhat.com>
[rebase to avoid pointless gotos in new code]
Signed-off-by: Eric Blake 

---
v10: new patch, but effectively split out from 31/37
---
 scripts/qapi-visit.py | 27 ---
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0be396b..0530f2b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -240,11 +240,8 @@ out:
 return ret


-def gen_visit_union(name, base, variants):
-ret = ''
-
-if base:
-ret += gen_visit_fields_decl(base)
+def gen_visit_union(name, base, members, variants):
+ret = gen_visit_struct_fields(name, base, members)

 for var in variants.variants:
 # Ugly special case for simple union TODO get rid of it
@@ -264,21 +261,9 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 if (!*obj) {
 goto out_obj;
 }
+visit_type_%(c_name)s_fields(v, obj, );
 ''',
  c_name=c_name(name))
-
-if base:
-ret += mcgen('''
-visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, );
-''',
- c_name=base.c_name())
-else:
-ret += mcgen('''
-visit_type_%(c_type)s(v, "%(name)s", &(*obj)->%(c_name)s, );
-''',
- c_type=variants.tag_member.type.c_name(),
- c_name=c_name(variants.tag_member.name),
- name=variants.tag_member.name)
 ret += gen_err_check(label='out_obj')
 ret += mcgen('''
 if (!visit_start_union(v, !!(*obj)->u.data, ) || err) {
@@ -378,11 +363,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
 def visit_object_type(self, name, info, base, members, variants):
 self.decl += gen_visit_decl(name)
 if variants:
-if members:
-# Members other than variants.tag_member not implemented
-assert len(members) == 1
-assert members[0] == variants.tag_member
-self.defn += gen_visit_union(name, base, variants)
+self.defn += gen_visit_union(name, base, members, variants)
 else:
 self.defn += gen_visit_struct(name, base, 

[Qemu-devel] [PATCH v10 02/13] qapi: Forbid empty unions and useless alternates

[Eric Blake]

Empty unions serve no purpose, and while we compile with gcc
which permits them, strict C99 forbids them.  We could inject
a dummy member (and in fact, we do for empty structs), but while
empty structs make sense in qapi, empty unions don't add any
expressiveness to the QMP language.  So prohibit them at parse
time.  Update the documentation and testsuite to match.

Signed-off-by: Eric Blake 

---
v10: hoist into earlier series
[no v7, v8, or v9]
v6: rebase to earlier qapi.py cleanups
---
 scripts/qapi.py | 12 ++--
 docs/qapi-code-gen.txt  | 15 ---
 tests/qapi-schema/alternate-empty.err   |  1 +
 tests/qapi-schema/alternate-empty.exit  |  2 +-
 tests/qapi-schema/alternate-empty.json  |  2 +-
 tests/qapi-schema/alternate-empty.out   |  5 -
 tests/qapi-schema/flat-union-empty.err  |  1 +
 tests/qapi-schema/flat-union-empty.exit |  2 +-
 tests/qapi-schema/flat-union-empty.json |  2 +-
 tests/qapi-schema/flat-union-empty.out  |  9 -
 tests/qapi-schema/union-empty.err   |  1 +
 tests/qapi-schema/union-empty.exit  |  2 +-
 tests/qapi-schema/union-empty.json  |  2 +-
 tests/qapi-schema/union-empty.out   |  6 --
 14 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index f40dc9e..f97236f 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -590,7 +590,10 @@ def check_union(expr, expr_info):
 "Discriminator '%s' must be of enumeration "
 "type" % discriminator)

-# Check every branch
+# Check every branch; don't allow an empty union
+if len(members) == 0:
+raise QAPIExprError(expr_info,
+"Union '%s' cannot have empty 'data'" % name)
 for (key, value) in members.items():
 check_name(expr_info, "Member of union '%s'" % name, key)

@@ -613,7 +616,11 @@ def check_alternate(expr, expr_info):
 members = expr['data']
 types_seen = {}

-# Check every branch
+# Check every branch; require at least two branches
+if len(members) < 2:
+raise QAPIExprError(expr_info,
+"Alternate '%s' should have at least two branches "
+"in 'data'" % name)
 for (key, value) in members.items():
 check_name(expr_info, "Member of alternate '%s'" % name, key)

@@ -1059,6 +1066,7 @@ class QAPISchemaObjectTypeVariants(object):
 assert bool(tag_member) != bool(tag_name)
 assert (isinstance(tag_name, str) or
 isinstance(tag_member, QAPISchemaObjectTypeMember))
+assert len(variants) > 0
 for v in variants:
 assert isinstance(v, QAPISchemaObjectTypeVariant)
 self.tag_name = tag_name
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 128f074..999f3b9 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -187,11 +187,11 @@ prevent incomplete include files.

 Usage: { 'struct': STRING, 'data': DICT, '*base': STRUCT-NAME }

-A struct is a dictionary containing a single 'data' key whose
-value is a dictionary.  This corresponds to a struct in C or an Object
-in JSON. Each value of the 'data' dictionary must be the name of a
-type, or a one-element array containing a type name.  An example of a
-struct is:
+A struct is a dictionary containing a single 'data' key whose value is
+a dictionary; the dictionary may be empty.  This corresponds to a
+struct in C or an Object in JSON. Each value of the 'data' dictionary
+must be the name of a type, or a one-element array containing a type
+name.  An example of a struct is:

  { 'struct': 'MyType',
'data': { 'member1': 'str', 'member2': 'int', '*member3': 'str' } }
@@ -288,9 +288,10 @@ or:{ 'union': STRING, 'data': DICT, 'base': 
STRUCT-NAME,

 Union types are used to let the user choose between several different
 variants for an object.  There are two flavors: simple (no
-discriminator or base), flat (both discriminator and base).  A union
+discriminator or base), and flat (both discriminator and base).  A union
 type is defined using a data dictionary as explained in the following
-paragraphs.
+paragraphs.  The data dictionary for either type of union must not
+be empty.

 A simple union type defines a mapping from automatic discriminator
 values to data types like in this example:
diff --git a/tests/qapi-schema/alternate-empty.err 
b/tests/qapi-schema/alternate-empty.err
index e69de29..bb06c5b 100644
--- a/tests/qapi-schema/alternate-empty.err
+++ b/tests/qapi-schema/alternate-empty.err
@@ -0,0 +1 @@
+tests/qapi-schema/alternate-empty.json:2: Alternate 'Alt' should have at least 
two branches in 'data'
diff --git a/tests/qapi-schema/alternate-empty.exit 
b/tests/qapi-schema/alternate-empty.exit
index 573541a..d00491f 100644
--- a/tests/qapi-schema/alternate-empty.exit
+++ b/tests/qapi-schema/alternate-empty.exit
@@ -1 +1 @@
-0
+1
diff 

[Qemu-devel] [PATCH v10 00/13] prune some QAPI visitor cruft (was qapi cleanups subset E)

[Eric Blake]

I'm still working on my documentation patches for QAPI visitors
(https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg03504.html),
but am finding it easier to nuke bad code up front than to document
that it is bad only to later nuke it. So this pulls bits and pieces
of other patches that Markus I have previously posted, along with
some new glue, to get rid of some of the worst of the cruft.

I'm calling this v10 because it is based on some of the v9 patches
mentioned above, but the backport-diff can't see through retitles
or anything else, so claims most of it is new:

001/13:[] [--] 'qapi: Simplify excess input reporting in input visitors'
002/13:[down] 'qapi: Forbid empty unions and useless alternates'
003/13:[down] 'qapi: Reposition error checks in gen_visit_fields()'
004/13:[down] 'qapi: Drop pointless gotos in generated code'
005/13:[down] 'qapi-visit: Simplify how we visit common union members'
006/13:[0065] [FC] 'qapi-visit: Unify struct and union visit'
007/13:[down] 'qapi-visit: Less indirection in visit_type_Foo_fields()'
008/13:[down] 'qapi: Adjust layout of FooList types'
009/13:[down] 'qapi: Emit structs used as variants in topological order'
010/13:[down] 'qapi: Don't box struct branch of alternate'
011/13:[down] 'qapi: Don't box branches of flat unions'
012/13:[down] 'qapi: Delete unused visit_start_union()'
013/13:[down] 'qapi: Change visit_start_implicit_struct to 
visit_start_alternate'

Eric Blake (11):
  qapi: Simplify excess input reporting in input visitors
  qapi: Forbid empty unions and useless alternates
  qapi: Reposition error checks in gen_visit_fields()
  qapi: Drop pointless gotos in generated code
  qapi-visit: Less indirection in visit_type_Foo_fields()
  qapi: Adjust layout of FooList types
  qapi: Emit structs used as variants in topological order
  qapi: Don't box struct branch of alternate
  qapi: Don't box branches of flat unions
  qapi: Delete unused visit_start_union()
  qapi: Change visit_start_implicit_struct to visit_start_alternate

Markus Armbruster (2):
  qapi-visit: Simplify how we visit common union members
  qapi-visit: Unify struct and union visit

 include/qapi/visitor.h  |  63 +++---
 include/qapi/visitor-impl.h |  21 ++--
 scripts/qapi-event.py   |   7 +-
 scripts/qapi-types.py   |  30 +++--
 scripts/qapi-visit.py   | 198 +---
 scripts/qapi.py |  31 +++--
 qapi/qapi-visit-core.c  |  44 +++
 cpus.c  |  18 +--
 hmp.c   |  12 +-
 qapi/opts-visitor.c |  16 +--
 qapi/qapi-dealloc-visitor.c |  42 ++-
 qapi/qmp-input-visitor.c|  43 +++
 qapi/qmp-output-visitor.c   |   3 +-
 qapi/string-input-visitor.c |   4 +-
 qapi/string-output-visitor.c|   2 +-
 tests/test-qmp-input-visitor.c  |  31 -
 tests/test-qmp-output-visitor.c |   5 +-
 docs/qapi-code-gen.txt  |  15 +--
 tests/qapi-schema/alternate-empty.err   |   1 +
 tests/qapi-schema/alternate-empty.exit  |   2 +-
 tests/qapi-schema/alternate-empty.json  |   2 +-
 tests/qapi-schema/alternate-empty.out   |   5 -
 tests/qapi-schema/flat-union-empty.err  |   1 +
 tests/qapi-schema/flat-union-empty.exit |   2 +-
 tests/qapi-schema/flat-union-empty.json |   2 +-
 tests/qapi-schema/flat-union-empty.out  |   9 --
 tests/qapi-schema/qapi-schema-test.json |   2 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 tests/qapi-schema/union-empty.err   |   1 +
 tests/qapi-schema/union-empty.exit  |   2 +-
 tests/qapi-schema/union-empty.json  |   2 +-
 tests/qapi-schema/union-empty.out   |   6 -
 32 files changed, 295 insertions(+), 329 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v10 10/13] qapi: Don't box struct branch of alternate

[Eric Blake]

There's no reason to do two malloc's for an alternate type visiting
a QAPI struct; let's just inline the struct directly as the C union
branch of the struct.

Surprisingly, no clients were actually using the struct member prior
to this patch; some testsuite coverage is added to avoid future
regressions.

Ultimately, we want to do the same treatment for QAPI unions, but
as that touches a lot more client code, it is better as a separate
patch.  So in the meantime, I had to hack in a way to test if we
are visiting an alternate type, within qapi-types:gen_variants();
the hack is possible because an earlier patch guaranteed that all
alternates have at least two branches, with at most one object
branch; the hack will go away in a later patch.

The generated code difference to qapi-types.h is relatively small,
made possible by a new c_type(is_member) parameter in qapi.py:

| struct BlockdevRef {
| QType type;
| union { /* union tag is @type */
| void *data;
|-BlockdevOptions *definition;
|+BlockdevOptions definition;
| char *reference;
| } u;
| };

meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
comparable to visit_type_implicit_Foo():

|+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char 
*name, BlockdevOptions *obj, Error **errp)
|+{
|+Error *err = NULL;
|+
|+visit_start_struct(v, name, NULL, 0, );
|+if (err) {
|+goto out;
|+}
|+visit_type_BlockdevOptions_fields(v, obj, );
|+error_propagate(errp, err);
|+err = NULL;
|+visit_end_struct(v, );
|+out:
|+error_propagate(errp, err);
|+}

and use it like this:

| switch ((*obj)->type) {
| case QTYPE_QDICT:
|-visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, );
|+visit_type_alternate_BlockdevOptions(v, name, &(*obj)->u.definition, 
);
| break;
| case QTYPE_QSTRING:
| visit_type_str(v, name, &(*obj)->u.reference, );

Signed-off-by: Eric Blake 

---
v10: new patch
---
 scripts/qapi-types.py   | 10 ++-
 scripts/qapi-visit.py   | 49 +
 scripts/qapi.py | 10 ---
 tests/test-qmp-input-visitor.c  | 29 ++-
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 6 files changed, 91 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2f23432..aba2847 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const 
%(c_name)s *obj)


 def gen_variants(variants):
+# HACK: Determine if this is an alternate (at least one variant
+# is not an object); unions have all branches as objects.
+inline = False
+for v in variants.variants:
+if not isinstance(v.type, QAPISchemaObjectType):
+inline = True
+break
+
 # FIXME: What purpose does data serve, besides preventing a union that
 # has a branch named 'data'? We use it in qapi-visit.py to decide
 # whether to bypass the switch statement if visiting the discriminator
@@ -136,7 +144,7 @@ def gen_variants(variants):
 ret += mcgen('''
 %(c_type)s %(c_name)s;
 ''',
- c_type=typ.c_type(),
+ c_type=typ.c_type(is_member=inline),
  c_name=c_name(var.name))

 ret += mcgen('''
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 9ff0337..948bde4 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -15,10 +15,14 @@
 from qapi import *
 import re

-# visit_type_FOO_implicit() is emitted as needed; track if it has already
+# visit_type_implicit_FOO() is emitted as needed; track if it has already
 # been output.
 implicit_structs_seen = set()

+# visit_type_alternate_FOO() is emitted as needed; track if it has already
+# been output.
+alternate_structs_seen = set()
+
 # visit_type_FOO_fields() is always emitted; track if a forward declaration
 # or implementation has already been output.
 struct_fields_seen = set()
@@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v, 
%(c_type)s **obj, Error *
 return ret


+def gen_visit_alternate_struct(typ):
+if typ in alternate_structs_seen:
+return ''
+alternate_structs_seen.add(typ)
+
+ret = gen_visit_fields_decl(typ)
+
+ret += mcgen('''
+
+static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name, 
%(c_type)s *obj, Error **errp)
+{
+Error *err = NULL;
+
+visit_start_struct(v, name, NULL, 0, );
+if (err) {
+goto out;
+}
+visit_type_%(c_type)s_fields(v, obj, );
+error_propagate(errp, err);
+err = NULL;
+visit_end_struct(v, );
+out:
+error_propagate(errp, err);
+}
+''',
+ c_type=typ.c_name())
+return ret
+
+
 def gen_visit_struct_fields(name, base, 

[Qemu-devel] [PATCH v10 04/13] qapi: Drop pointless gotos in generated code

[Eric Blake]

There's no point in emitting a goto if the very next thing
emitted will be the label.  A bit of cleverness in gen_visit_fields()
will let us choose when to omit a final error check (basically,
swap the order of emitted text within the loop, with the error
check omitted on the first pass, then checking whether to emit a
final check after the loop); and in turn omit unused labels.

The change to generated code is a bit easier because we split out
the reindentation of the remaining gotos in the previous patch.
For example, in visit_type_ChardevReturn_fields():

| if (visit_optional(v, "pty", >has_pty)) {
| visit_type_str(v, "pty", >pty, );
| }
|-if (err) {
|-goto out;
|-}
|-
|-out:
| error_propagate(errp, err);

Signed-off-by: Eric Blake 

---
v10: new patch
---
 scripts/qapi-event.py |  7 +--
 scripts/qapi-visit.py | 10 ++
 scripts/qapi.py   |  8 ++--
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index 544ae12..b50ac29 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -68,9 +68,12 @@ def gen_event_send(name, arg_type):
  name=name)
 ret += gen_err_check()
 ret += gen_visit_fields(arg_type.members, need_cast=True,
-label='out_obj')
-ret += mcgen('''
+label='out_obj', skiplast=True)
+if len(arg_type.members) > 1:
+ret += mcgen('''
 out_obj:
+''')
+ret += mcgen('''
 visit_end_struct(v, err ? NULL : );
 if (err) {
 goto out;
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0cc9b08..0be396b 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -92,12 +92,14 @@ static void visit_type_%(c_name)s_fields(Visitor *v, 
%(c_name)s **obj, Error **e
 visit_type_%(c_type)s_fields(v, (%(c_type)s **)obj, );
 ''',
  c_type=base.c_name())
-ret += gen_err_check()
+if members:
+ret += gen_err_check()

-ret += gen_visit_fields(members, prefix='(*obj)->')
+ret += gen_visit_fields(members, prefix='(*obj)->', skiplast=True)

-# 'goto out' produced for base, and by gen_visit_fields() for each member
-if base or members:
+# 'goto out' produced for base, and by gen_visit_fields() for each
+# member except the last
+if bool(base) + len(members) > 1:
 ret += mcgen('''

 out:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index fab5001..4d75d75 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -1645,14 +1645,17 @@ def gen_err_check(label='out', skiperr=False):


 def gen_visit_fields(members, prefix='', need_cast=False, skiperr=False,
- label='out'):
+ label='out', skiplast=False):
 ret = ''
 if skiperr:
 errparg = 'NULL'
 else:
 errparg = ''
+local_skiperr = True

 for memb in members:
+ret += gen_err_check(skiperr=local_skiperr, label=label)
+local_skiperr = skiperr
 if memb.optional:
 ret += mcgen('''
 if (visit_optional(v, "%(name)s", &%(prefix)shas_%(c_name)s)) {
@@ -1679,8 +1682,9 @@ def gen_visit_fields(members, prefix='', need_cast=False, 
skiperr=False,
 ret += mcgen('''
 }
 ''')
+
+if members and not skiplast:
 ret += gen_err_check(skiperr=skiperr, label=label)
-
 return ret


-- 
2.5.0

[Qemu-devel] [PATCH v10 01/13] qapi: Simplify excess input reporting in input visitors

[Eric Blake]

When reporting that an unvisited member remains at the end of an
input visit for a struct, we were using g_hash_table_find()
coupled with a callback function that always returns true, to
locate an arbitrary member of the hash table.  But if all we
need is an arbitrary entry, we can get that from a single-use
iterator, without needing a tautological callback function.

Technically, our cast of &(GQueue *) to (void **) is not strict
C (while void * must be able to hold all other pointers, nothing
says a void ** has to be the same width or representation as a
GQueue **).  The kosher way to write it would be the verbose:

void *tmp;
GQueue *any;
if (g_hash_table_iter_next(, NULL, )) {
any = tmp;

But our code base (not to mention glib itself) already has other
cases of assuming that ALL pointers have the same width and
representation, where a compiler would have to go out of its way
to mis-compile our borderline behavior.

Suggested-by: Markus Armbruster 
Signed-off-by: Eric Blake 
Reviewed-by: Marc-André Lureau 

---
v10: enhance commit message
v9: no change
v8: rebase to earlier changes
v7: retitle, rebase to earlier context changes
v6: new patch, based on comments on RFC against v5 7/46
---
 qapi/opts-visitor.c  | 12 +++-
 qapi/qmp-input-visitor.c | 14 +-
 2 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index d54f75b..ae5b955 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -157,17 +157,11 @@ opts_start_struct(Visitor *v, const char *name, void 
**obj,
 }


-static gboolean
-ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
-{
-return TRUE;
-}
-
-
 static void
 opts_end_struct(Visitor *v, Error **errp)
 {
 OptsVisitor *ov = to_ov(v);
+GHashTableIter iter;
 GQueue *any;

 if (--ov->depth > 0) {
@@ -175,8 +169,8 @@ opts_end_struct(Visitor *v, Error **errp)
 }

 /* we should have processed all (distinct) QemuOpt instances */
-any = g_hash_table_find(ov->unprocessed_opts, _true, NULL);
-if (any) {
+g_hash_table_iter_init(, ov->unprocessed_opts);
+if (g_hash_table_iter_next(, NULL, (void **))) {
 const QemuOpt *first;

 first = g_queue_peek_head(any);
diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
index 362a1a3..2f48b95 100644
--- a/qapi/qmp-input-visitor.c
+++ b/qapi/qmp-input-visitor.c
@@ -90,12 +90,6 @@ static void qmp_input_push(QmpInputVisitor *qiv, QObject 
*obj, Error **errp)
 qiv->nb_stack++;
 }

-/** Only for qmp_input_pop. */
-static gboolean always_true(gpointer key, gpointer val, gpointer user_pkey)
-{
-*(const char **)user_pkey = (const char *)key;
-return TRUE;
-}

 static void qmp_input_pop(QmpInputVisitor *qiv, Error **errp)
 {
@@ -104,9 +98,11 @@ static void qmp_input_pop(QmpInputVisitor *qiv, Error 
**errp)
 if (qiv->strict) {
 GHashTable * const top_ht = qiv->stack[qiv->nb_stack - 1].h;
 if (top_ht) {
-if (g_hash_table_size(top_ht)) {
-const char *key;
-g_hash_table_find(top_ht, always_true, );
+GHashTableIter iter;
+const char *key;
+
+g_hash_table_iter_init(, top_ht);
+if (g_hash_table_iter_next(, (void **), NULL)) {
 error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
 }
 g_hash_table_unref(top_ht);
-- 
2.5.0

[Qemu-devel] [PATCH v10 06/13] qapi-visit: Unify struct and union visit

[Eric Blake]

From: Markus Armbruster 

gen_visit_union() is now just like gen_visit_struct() plus additional
code to handle variants.  Make that code conditional on variants, so
gen_visit_union() does exactly the same for structs as
gen_visit_struct().  Rename it to gen_visit_object(), use it for
structs, and drop gen_visit_struct().

Output is slightly changed due to an earlier placement of the
'out_obj' label for structs, but with no semantic impact:

| if (err) {
| goto out;
| }
| if (!*obj) {
| goto out_obj;
| }
| visit_type_ACPIOSTInfo_fields(v, obj, );
|+out_obj:
| error_propagate(errp, err);
| err = NULL;
|-out_obj:
| visit_end_struct(v, );
| out:

Signed-off-by: Markus Armbruster 
Message-Id: <1453902888-20457-4-git-send-email-arm...@redhat.com>
[rebase to earlier changes]
Signed-off-by: Eric Blake 

---
v10: new patch, replacing 31/37
---
 scripts/qapi-visit.py | 96 ++-
 1 file changed, 34 insertions(+), 62 deletions(-)

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 0530f2b..786fe57 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -111,40 +111,6 @@ out:
 return ret


-def gen_visit_struct(name, base, members):
-ret = gen_visit_struct_fields(name, base, members)
-
-# FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
-# *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
-# rather than leaving it non-NULL. As currently written, the caller must
-# call qapi_free_FOO() to avoid a memory leak of the partial FOO.
-ret += mcgen('''
-
-void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
Error **errp)
-{
-Error *err = NULL;
-
-visit_start_struct(v, name, (void **)obj, sizeof(%(c_name)s), );
-if (err) {
-goto out;
-}
-if (!*obj) {
-goto out_obj;
-}
-visit_type_%(c_name)s_fields(v, obj, );
-error_propagate(errp, err);
-err = NULL;
-out_obj:
-visit_end_struct(v, );
-out:
-error_propagate(errp, err);
-}
-''',
- c_name=c_name(name))
-
-return ret
-
-
 def gen_visit_list(name, element_type):
 # FIXME: if *obj is NULL on entry, and the first visit_next_list()
 # assigns to *obj, while a later one fails, we should clean up *obj
@@ -240,14 +206,19 @@ out:
 return ret


-def gen_visit_union(name, base, members, variants):
+def gen_visit_object(name, base, members, variants):
 ret = gen_visit_struct_fields(name, base, members)

-for var in variants.variants:
-# Ugly special case for simple union TODO get rid of it
-if not var.simple_union_type():
-ret += gen_visit_implicit_struct(var.type)
+if variants:
+for var in variants.variants:
+# Ugly special case for simple union TODO get rid of it
+if not var.simple_union_type():
+ret += gen_visit_implicit_struct(var.type)

+# FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
+# *obj, but then visit_type_FOO_fields() fails, we should clean up *obj
+# rather than leaving it non-NULL. As currently written, the caller must
+# call qapi_free_FOO() to avoid a memory leak of the partial FOO.
 ret += mcgen('''

 void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, 
Error **errp)
@@ -264,43 +235,47 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
 visit_type_%(c_name)s_fields(v, obj, );
 ''',
  c_name=c_name(name))
-ret += gen_err_check(label='out_obj')
-ret += mcgen('''
+if variants:
+ret += gen_err_check(label='out_obj')
+ret += mcgen('''
 if (!visit_start_union(v, !!(*obj)->u.data, ) || err) {
 goto out_obj;
 }
 switch ((*obj)->%(c_name)s) {
 ''',
- c_name=c_name(variants.tag_member.name))
+ c_name=c_name(variants.tag_member.name))

-for var in variants.variants:
-# TODO ugly special case for simple union
-simple_union_type = var.simple_union_type()
-ret += mcgen('''
+for var in variants.variants:
+# TODO ugly special case for simple union
+simple_union_type = var.simple_union_type()
+ret += mcgen('''
 case %(case)s:
 ''',
- case=c_enum_const(variants.tag_member.type.name,
-   var.name))
-if simple_union_type:
-ret += mcgen('''
+ case=c_enum_const(variants.tag_member.type.name,
+   var.name))
+if simple_union_type:
+ret += mcgen('''
 visit_type_%(c_type)s(v, "data", &(*obj)->u.%(c_name)s, );
 ''',
- c_type=simple_union_type.c_name(),
- 

Re: [Qemu-devel] [PATCH v3] linux-user: Fix qemu-binfmt-conf.h to store config across reboot

[Laurent Vivier]

Ping?

Le 29/01/2016 17:07, Laurent Vivier a écrit :
> Original qemu-binfmt-conf.h is only able to write configuration
> into /proc/sys/fs/binfmt_misc, and the configuration is lost on reboot.
> 
> This script can configure debian and systemd services to restore
> configuration on reboot. Moreover, it is able to manage binfmt
> credential and to configure the path of the interpreter.
> 
> List of supported CPU is:
> 
> i386 i486 alpha arm sparc32plus ppc ppc64 ppc64le
> m68k mips mipsel mipsn32 mipsn32el mips64 mips64el
> sh4 sh4eb s390x aarch64
> 
> Usage: qemu-binfmt-conf.sh [--qemu-path PATH][--debian][--systemd CPU]
>[--help][--credential yes|no][--exportdir PATH]
> 
>Configure binfmt_misc to use qemu interpreter
> 
>--help:   display this usage
>--qemu-path:  set path to qemu interpreter (/usr/local/bin)
>--debian: don't write into /proc,
>  instead generate update-binfmts templates
>--systemd:don't write into /proc,
>  instead generate file for systemd-binfmt.service
>  for the given CPU
>--exportdir:  define where to write configuration files
>  (default: /etc/binfmt.d or /usr/share/binfmts)
>--credential: if yes, credential an security tokens are
>  calculated according to the binary to interpret
> 
> To import templates with update-binfmts, use :
> 
> sudo update-binfmts --importdir /usr/share/binfmts --import qemu-CPU
> 
> To remove interpreter, use :
> 
> sudo update-binfmts --package qemu-CPU --remove qemu-CPU 
> /usr/local/bin
> 
> With systemd, binfmt files are loaded by systemd-binfmt.service
> 
> The environment variable HOST_ARCH allows to override 'uname' to generate
> configuration files for a different architecture than the current one.
> 
> Signed-off-by: Laurent Vivier 
> ---
> v3: change subject to be shorter
> fix typo
> remove "!EOF", "echo -n" and "[ ... -o ... ]"
> check cpu given by --systemd is in the list
> v2: replace some ERRORS by WARNINGS to be able to use the script inside a 
> package build
> check only the right to write in the directory, no need to be root
> merge systemd and binfmt_misc configuration generation
> s/qemu_generate_packages/qemu_generate_debian/
> add support of HOST_ARCH from debian, and update CPU families.
> allow to use --exportdir with --systemd and update "Usage".
> 
>  scripts/qemu-binfmt-conf.sh | 389 
> 
>  1 file changed, 320 insertions(+), 69 deletions(-)
>  mode change 100644 => 100755 scripts/qemu-binfmt-conf.sh
> 
> diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
> old mode 100644
> new mode 100755
> index 289b1a3..de4d1c1
> --- a/scripts/qemu-binfmt-conf.sh
> +++ b/scripts/qemu-binfmt-conf.sh
> @@ -1,72 +1,323 @@
>  #!/bin/sh
>  # enable automatic i386/ARM/M68K/MIPS/SPARC/PPC/s390 program execution by 
> the kernel
>  
> -# load the binfmt_misc module
> -if [ ! -d /proc/sys/fs/binfmt_misc ]; then
> -  /sbin/modprobe binfmt_misc
> -fi
> -if [ ! -f /proc/sys/fs/binfmt_misc/register ]; then
> -  mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc
> -fi
> -
> -# probe cpu type
> -cpu=`uname -m`
> -case "$cpu" in
> -  i386|i486|i586|i686|i86pc|BePC|x86_64)
> -cpu="i386"
> -  ;;
> -  m68k)
> -cpu="m68k"
> -  ;;
> -  mips*)
> -cpu="mips"
> -  ;;
> -  "Power Macintosh"|ppc|ppc64)
> -cpu="ppc"
> -  ;;
> -  armv[4-9]*)
> -cpu="arm"
> -  ;;
> -esac
> -
> -# register the interpreter for each cpu except for the native one
> -if [ $cpu != "i386" ] ; then
> -echo 
> ':i386:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/local/bin/qemu-i386:'
>  > /proc/sys/fs/binfmt_misc/register
> -echo 
> ':i486:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x06\x00:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/local/bin/qemu-i386:'
>  > /proc/sys/fs/binfmt_misc/register
> -fi
> -if [ $cpu != "alpha" ] ; then
> -echo 
> ':alpha:M::\x7fELF\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x26\x90:\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/local/bin/qemu-alpha:'
>  > /proc/sys/fs/binfmt_misc/register
> -fi
> -if [ $cpu != "arm" ] ; then
> -echo   
> ':arm:M::\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x28\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/usr/local/bin/qemu-arm:'
>  > /proc/sys/fs/binfmt_misc/register
> -echo   
> 

Re: [Qemu-devel] [PATCH 2/3] i.MX: Add the Freescale SPI Controller

[Jean-Christophe DUBOIS]

Le 15/02/2016 17:46, mar.krzeminski a écrit :



W dniu 15.02.2016 o 11:18, Jean-Christophe DUBOIS pisze:

Le 14/02/2016 20:17, mar.krzeminski a écrit :



W dniu 14.02.2016 o 17:56, Jean-Christophe DUBOIS pisze:

Le 14/02/2016 12:52, mar.krzeminski a écrit :

Hello,

W dniu 13.02.2016 o 17:06, Jean-Christophe Dubois pisze:

Signed-off-by: Jean-Christophe Dubois 
---
  hw/ssi/Makefile.objs |   1 +
  hw/ssi/imx_spi.c | 449 
+++

  include/hw/ssi/imx_spi.h | 104 +++
  3 files changed, 554 insertions(+)
  create mode 100644 hw/ssi/imx_spi.c
  create mode 100644 include/hw/ssi/imx_spi.h

diff --git a/hw/ssi/Makefile.objs b/hw/ssi/Makefile.objs
index 9555825..fcbb79e 100644
--- a/hw/ssi/Makefile.objs
+++ b/hw/ssi/Makefile.objs
@@ -4,3 +4,4 @@ common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
  common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
obj-$(CONFIG_OMAP) += omap_spi.o
+obj-$(CONFIG_IMX) += imx_spi.o
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
new file mode 100644
index 000..9f7f4fe
--- /dev/null
+++ b/hw/ssi/imx_spi.c
@@ -0,0 +1,449 @@
+/*
+ * IMX SPI Controller
+ *
+ * Copyright (c) 2016 Jean-Christophe Dubois 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 
2 or later.

+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/ssi/imx_spi.h"
+#include "sysemu/sysemu.h"
+
+#ifndef DEBUG_IMX_SPI
+#define DEBUG_IMX_SPI 0
+#endif
+
+#define DPRINTF(fmt, args...) \
+do { \
+if (DEBUG_IMX_SPI) { \
+fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SPI, \
+ __func__, ##args); \
+} \
+} while (0)
+
+static char const *imx_spi_reg_name(uint32_t reg)
+{
+static char unknown[20];
+
+switch (reg) {
+case ECSPI_RXDATA:
+return  "ECSPI_RXDATA";
+case ECSPI_TXDATA:
+return  "ECSPI_TXDATA";
+case ECSPI_CONREG:
+return  "ECSPI_CONREG";
+case ECSPI_CONFIGREG:
+return  "ECSPI_CONFIGREG";
+case ECSPI_INTREG:
+return  "ECSPI_INTREG";
+case ECSPI_DMAREG:
+return  "ECSPI_DMAREG";
+case ECSPI_STATREG:
+return  "ECSPI_STATREG";
+case ECSPI_PERIODREG:
+return  "ECSPI_PERIODREG";
+case ECSPI_TESTREG:
+return  "ECSPI_TESTREG";
+case ECSPI_MSGDATA:
+return  "ECSPI_MSGDATA";
+default:
+sprintf(unknown, "%d ?", reg);
+return unknown;
+}
+}
+
+static const VMStateDescription vmstate_imx_spi = {
+.name = TYPE_IMX_SPI,
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_FIFO32(tx_fifo, IMXSPIState),
+VMSTATE_FIFO32(rx_fifo, IMXSPIState),
+VMSTATE_UINT32_ARRAY(regs, IMXSPIState, ECSPI_MAX),
+VMSTATE_END_OF_LIST()
+},
+};
+
+static void imx_spi_txfifo_reset(IMXSPIState *s)
+{
+fifo32_reset(>tx_fifo);
+s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
+}
+
+static void imx_spi_rxfifo_reset(IMXSPIState *s)
+{
+fifo32_reset(>rx_fifo);
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RO;
+}
+
+static void imx_spi_update_irq(IMXSPIState *s)
+{
+int level;
+
+if (fifo32_is_empty(>rx_fifo)) {
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RR;
+} else {
+s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RR;
+}
+
+if (fifo32_is_full(>rx_fifo)) {
+s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RF;
+} else {
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_RF;
+}
+
+if (fifo32_is_empty(>tx_fifo)) {
+s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TE;
+} else {
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TE;
+}
+
+if (fifo32_is_full(>tx_fifo)) {
+s->regs[ECSPI_STATREG] |= ECSPI_STATREG_TF;
+} else {
+s->regs[ECSPI_STATREG] &= ~ECSPI_STATREG_TF;
+}
+
+level = s->regs[ECSPI_STATREG] & s->regs[ECSPI_INTREG] ? 1 : 0;
+
+if (s->previous_level != level) {
+DPRINTF("setting IRQ a level %d\n", level);
+s->previous_level = level;
+qemu_set_irq(s->irq, level);
+}
+
+DPRINTF("IRQ level is %d\n", level);
+}
+
+static uint8_t imx_spi_selected_channel(IMXSPIState *s)
+{
+return EXTRACT(s->regs[ECSPI_CONREG], 
ECSPI_CONREG_CHANNEL_SELECT);

+}
+
+static uint32_t imx_spi_burst_length(IMXSPIState *s)
+{
+return EXTRACT(s->regs[ECSPI_CONREG], 
ECSPI_CONREG_BURST_LENGTH) + 1;

+}
+
+static bool imx_spi_is_enabled(IMXSPIState *s)
+{
+return (s->regs[ECSPI_CONREG] & ECSPI_CONREG_EN) ? true : 
false;

+}
+
+static bool imx_spi_channel_is_master(IMXSPIState *s)
+{
+uint8_t mode = EXTRACT(s->regs[ECSPI_CONREG], 
ECSPI_CONREG_CHANNEL_MODE);

+
+return (mode & (1 << imx_spi_selected_channel(s))) ? true : 
false;

+}
+
+static 

[Qemu-devel] QEMU GSoC 2016: MTTCG project

[Pranith Kumar]

Hello,

I am interested in working on a portion of the MTTCG project as part
of GSoC 2016. I am writing to ask for guidance in creating a formal
proposal.

On IRC, Alex suggested a project to support proper modelling of memory
consistency between different host and guest architectures. This, I
think is going to based on top of Alvise's work. I am comfortable in
working with various memory consistency models and have a decent
programming experience.

Please let me know if you have any other suggestions for projects or
any other related advice.

Thanks!
-- 
Pranith

Re: [Qemu-devel] [PULL 0/5] tcg queued patches

[Richard Henderson]

On 02/16/2016 07:35 AM, Peter Maydell wrote:

On 15 February 2016 at 20:29, Richard Henderson  wrote:

On 02/16/2016 12:04 AM, Peter Maydell wrote:

I'm generally reluctant to suggest compiler bugs, but this does
look rather like a compiler bug...



There are at least 5 such bugs open against gcc at the moment.

 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56456

I couldn't reproduce this quickly with a freshly built gcc 4.8 branch on
i686-linux.  Could you please file a gcc bug with your preprocessed source?
There's a chance it isn't a duplicate, but...


Is it worth filing a bug that only repros on a non-upstream gcc?


Well, it doesn't repro on i686-linux; with a cross-compiler to mingw it still 
might, but filing the bug will save me setting up the full cross environment to 
be able to build qemu with mingw.




r~

Re: [Qemu-devel] [PATCH v2 0/4] TriCore exception patches

[Richard Henderson]

On 02/16/2016 12:10 AM, Bastian Koppelmann wrote:

Bastian Koppelmann (4):
   target-tricore: Add trap handling
   target-tricore: add context managment trap generation
   target-tricore: add illegal opcode trap generation
   target-tricore: add opd trap generation


Comments on patch 1.  Patches 2-4:

Reviewed-by: Richard Henderson  


r~

Re: [Qemu-devel] [PATCH v2 1/4] target-tricore: Add trap handling

[Richard Henderson]

On 02/16/2016 12:10 AM, Bastian Koppelmann wrote:

+
+void tricore_cpu_do_interrupt(CPUState *cs)
+{
+TriCoreCPU *cpu = TRICORE_CPU(cs);
+CPUTriCoreState *env = >env;
+
+if (cs->exception_index <= TRAPC_NMI) {
+/* The trap vector table is accessed to fetch the first instruction of
+   the trap handler. */
+env->PC = env->BTV | (cs->exception_index << 5);
+} else if (cs->exception_index == TRAPC_IRQ) {
+/* The interrupt vector table is accessed to fetch the first 
instruction
+   of the interrupt handler. */
+env->PC = env->BIV | ((env->ICR & MASK_ICR_PIPN) >> 10);
+}
+}


You've still got a path whereby you modify PC without saving the old one.

I don't think you want to add the do_interrupt hook at all until you're ready 
to do real async interrupts.



+/* PCXI.PCPN = ICR.CCPN */
+env->PCXI = (env->PCXI & 0xff) +
+((env->ICR & MASK_ICR_CCPN) << 24);
+cs->exception_index = class;
+cpu_loop_exit(cs);
+}
+


There's no reason you can't modify PC here at the end of 
raise_exception_sync_internal.  If you omit the set of exception_index here, 
you'll simply exit the cpu loop and immediately re-enter it at the new PC, 
without having to go through the do_interrupt hook.



r~

Re: [Qemu-devel] [PULL 0/5] tcg queued patches

[Peter Maydell]

On 15 February 2016 at 20:29, Richard Henderson  wrote:
> On 02/16/2016 12:04 AM, Peter Maydell wrote:
>> I'm generally reluctant to suggest compiler bugs, but this does
>> look rather like a compiler bug...
>
>
> There are at least 5 such bugs open against gcc at the moment.
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56456
>
> I couldn't reproduce this quickly with a freshly built gcc 4.8 branch on
> i686-linux.  Could you please file a gcc bug with your preprocessed source?
> There's a chance it isn't a duplicate, but...

Is it worth filing a bug that only repros on a non-upstream gcc?

> In the meantime... hmm.  I don't suppose removing the inline helps?
> Probably not, since there's only one caller...  Otherwise I guess we should
> go with your "rd & ~1" workaround.

We should give it a try with Eric's "stop overriding
'inline' to mean 'always inline', but as you say
with only one caller it's probably going to go ahead
and inline it anyway.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 0/2] ARM: add QMP command to query GIC version

[Peter Maydell]

On 15 February 2016 at 20:18, Andrew Jones  wrote:
> On Mon, Feb 15, 2016 at 08:40:54PM +0100, Markus Armbruster wrote:
>> How would the command line look like?
>>
>
> Here is what is available today
>
> # select gicv2 (this work with and without KVM)
> qemu-system-aarch64 -M virt  # v2 is the default
> qemu-system-aarch64 -M virt,gic-version=2 ...
>
> # select gicv3 (only works with KVM)
> qemu-system-aarch64 -M virt,gic-version=3 ...

This will work with TCG once we get the GICv3 emulation upstream.

> # select whatever the host has
> qemu-system-aarch64 -M virt,gic-version=host ...

This only works with KVM (like -cpu host).

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/5] tcg queued patches

[Richard Henderson]

On 02/16/2016 12:04 AM, Peter Maydell wrote:

On 15 February 2016 at 11:29, Richard Henderson  wrote:


Richard Henderson (5):
   tcg: Work around clang bug wrt enum ranges, part 2
   tcg: Implement indirect memory registers
   tcg: Allocate indirect_base temporaries in a different order
   target-sparc: Tidy global register initialization
   target-sparc: Use global registers for the register window

  target-sparc/translate.c | 196 ++-
  tcg/tcg.c| 138 +++--
  tcg/tcg.h|   2 +
  3 files changed, 192 insertions(+), 144 deletions(-)


This failed to build on the i686-w64-mingw32 compiler
("i686-w64-mingw32-gcc (GCC) 4.8.2", from the Ubuntu
gcc-mingw-w64-i686 package version 4.8.2-10ubuntu2+12):

target-sparc/translate.c: In function ‘gen_intermediate_code’:
target-sparc/translate.c:299:24: error: array subscript is above array
bounds [-Werror=array-bounds]
  return cpu_regs[reg];
 ^

Fiddling around with the source file to see which call to
gen_dest_gpr() is provoking this shows that it's the one
in gen_ldda_asi() at line 2157 -- if I change the second
argument from 'rd + 1' to 'rd' it compiles OK.

Changing the call site of gen_ldda_asi at line 4727 so its last
argument is 'rd & ~1' rather than 'rd' also suppresses the
error. (That can't possibly change the semantics because we've
just done "if (rd & 1) goto illegal_insn;"...)

I'm generally reluctant to suggest compiler bugs, but this does
look rather like a compiler bug...


There are at least 5 such bugs open against gcc at the moment.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56456

I couldn't reproduce this quickly with a freshly built gcc 4.8 branch on 
i686-linux.  Could you please file a gcc bug with your preprocessed source? 
There's a chance it isn't a duplicate, but...


In the meantime... hmm.  I don't suppose removing the inline helps?  Probably 
not, since there's only one caller...  Otherwise I guess we should go with your 
"rd & ~1" workaround.



r~

Re: [Qemu-devel] [PATCH] vl: fix migration from prelaunch state

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonz...@redhat.com) wrote:
> Reproducer is simply to migrate a virtual machine that was started with -S.
> 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Dr. David Alan Gilbert 

(I'm sure we have a bz for that as well somewhere)

Dave

> ---
>  vl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/vl.c b/vl.c
> index 1ec62ac..e600f8d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -590,6 +590,7 @@ static const RunStateTransition 
> runstate_transitions_def[] = {
>  { RUN_STATE_INMIGRATE, RUN_STATE_WATCHDOG },
>  { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
>  { RUN_STATE_INMIGRATE, RUN_STATE_FINISH_MIGRATE },
> +{ RUN_STATE_INMIGRATE, RUN_STATE_PRELAUNCH },
>  
>  { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
>  { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
> -- 
> 2.5.0
> 
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC PATCH 0/2] ARM: add QMP command to query GIC version

[Andrew Jones]

On Mon, Feb 15, 2016 at 08:40:54PM +0100, Markus Armbruster wrote:
> Peter Maydell  writes:
> 
> > On 15 February 2016 at 15:08, Markus Armbruster  wrote:
> >> Peter Xu  writes:
> >>> On Mon, Feb 15, 2016 at 10:52:01AM +0100, Markus Armbruster wrote:
>  Peter Xu  writes:
>  Adding ad hoc queries as we go won't scale.  Is there really no generic
>  way to get this information, e.g. with qom-get?
> >>>
> >>> Haven't used "qom-get" before, but it seems to fetch one property
> >>> for a specific object. If so, will it be strange to hide some
> >>> capability bits into every GIC objects (though there is possibly
> >>> one object)?
> >>
> >> Pardon my ignorance...  what are these "GIC objects"?
> >>
> >> What exactly is the "GIC type", and how would the result of
> >> query-gic-capability be used?
> >
> > The GIC type (for our purposes) is the revision of the interrupt
> > controller supported by the host, which comes in two versions
> > (v2 and v3). These are not compatible, unless your host has
> > the v3-with-v2-compat flavour. If a host is v3-only, it is not
> > possible for it to give the guest a v2 virtual interrupt
> > controller; if v2, it can't give the guest a v3 virtual interrupt
> > controller. (If you ask QEMU to do this via command line options
> > we will report an error at startup.)
> 
> How would the command line look like?
>

Here is what is available today

# select gicv2 (this work with and without KVM)
qemu-system-aarch64 -M virt  # v2 is the default
qemu-system-aarch64 -M virt,gic-version=2 ...

# select gicv3 (only works with KVM)
qemu-system-aarch64 -M virt,gic-version=3 ...

# select whatever the host has
qemu-system-aarch64 -M virt,gic-version=host ...


Thanks,
drew

Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Michael S. Tsirkin]

On Mon, Feb 15, 2016 at 02:50:41PM -0500, Kevin O'Connor wrote:
> On Mon, Feb 15, 2016 at 09:29:26PM +0200, Michael S. Tsirkin wrote:
> > I can build a generic interface to pass addresses
> > allocated by bios back to QEMU. It looks like this would
> > be useful for other purposes as well.  Interested?
> 
> If this is undertaken, I suggest extending fw_cfg to support "writable
> files" instead of introducing a whole new interface.
> 
> -Kevin

Exactly, this is what I had in mind.

-- 
MST

Re: [Qemu-devel] [PATCH] msix: fix msix_vector_masked

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> commit 428c3ece97179557f2753071fb0ca97a03437267 ("fix MSI injection on Xen")
> inadvertently enabled the xen-specific logic unconditionally.
> Limit it to only when xen is enabled.
> Additionally, msix data should be read with pci_get_log
> since the format is pci little-endian.
>
> Reported-by: "Daniel P. Berrange" 
> Cc: Stefano Stabellini 
> Signed-off-by: Michael S. Tsirkin 

Fixes ivshmem-test /x86_64/ivshmem/server-msi.  Not run by "make check"
unless you add SPEED=slow.  Quick reproducer:

  $ make tests/ivshmem-test && 
QTEST_QEMU_BINARY='x86_64-softmmu/qemu-system-x86_64' QTEST_QEMU_IMG=qemu-img 
MALLOC_PERTURB_=${MALLOC_PERTURB_:-$((RANDOM % 255 + 1))} gtester -k --verbose 
-m slow tests/ivshmem-test

Adding this to the commit message wouldn't hurt.

Thanks!

Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Alex Williamson]

On Mon, 15 Feb 2016 21:29:26 +0200
"Michael S. Tsirkin"  wrote:

> On Mon, Feb 15, 2016 at 12:20:23PM -0700, Alex Williamson wrote:
> > On Mon, 15 Feb 2016 10:54:51 +0100
> > Igor Mammedov  wrote:
> >   
> > > On Sat, 13 Feb 2016 18:03:31 -0700
> > > Alex Williamson  wrote:
> > >   
> > > > On Sat, 13 Feb 2016 19:20:32 -0500
> > > > "Kevin O'Connor"  wrote:
> > > > 
> > > > > On Sat, Feb 13, 2016 at 01:57:09PM -0700, Alex Williamson wrote:  
> > > > > > On Sat, 13 Feb 2016 15:05:09 -0500
> > > > > > "Kevin O'Connor"  wrote:
> > > > > > > On Sat, Feb 13, 2016 at 11:51:51AM -0700, Alex Williamson wrote:  
> > > > > > >   
> > > > > > > > On Sat, 13 Feb 2016 13:18:39 -0500
> > > > > > > > "Kevin O'Connor"  wrote:  
> > > > > > > > > On Sat, Feb 13, 2016 at 08:12:09AM -0700, Alex Williamson 
> > > > > > > > > wrote:  
> > > > > > > > > > On Fri, 12 Feb 2016 21:49:04 -0500
> > > > > > > > > > "Kevin O'Connor"  wrote:
> > > > > > > > > > > On Fri, Feb 12, 2016 at 05:23:18PM -0700, Alex Williamson 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > Intel IGD makes use of memory allocated and marked 
> > > > > > > > > > > > reserved by the
> > > > > > > > > > > > BIOS as a stolen memory range.  For the most part, 
> > > > > > > > > > > > guest drivers don't
> > > > > > > > > > > > make use of this, but our achilles heel is the vBIOS.  
> > > > > > > > > > > > The vBIOS
> > > > > > > > > > > > programs the device to use the host stolen memory range 
> > > > > > > > > > > > and it's used
> > > > > > > > > > > > in the pre-boot environment.  Generally the guest won't 
> > > > > > > > > > > > have access to
> > > > > > > > > > > > the host stolen memory area, so these accesses either 
> > > > > > > > > > > > land in VM
> > > > > > > > > > > > memory or unassigned space and generate IOMMU faults.  
> > > > > > > > > > > > By allocating
> > > > > > > > > > > > this range in SeaBIOS and programming it into the 
> > > > > > > > > > > > device, QEMU (via
> > > > > > > > > > > > vfio) can make sure this guest allocated stolen memory 
> > > > > > > > > > > > range is used
> > > > > > > > > > > > instead.  
> > > > > > > > > > > 
> > > > > > > > > > > What does "vBIOS" mean in this context?  Is it the video 
> > > > > > > > > > > device option
> > > > > > > > > > > rom or something else?
> > > > > > > > > > 
> > > > > > > > > > vBIOS = video BIOS, you're correct, it's just the video 
> > > > > > > > > > device option
> > > > > > > > > > ROM.
> > > > > > > > > 
> > > > > > > > > Is the problem from when the host runs the video option rom, 
> > > > > > > > > or is the
> > > > > > > > > problem from the guest (via SeaBIOS) running the video option 
> > > > > > > > > rom?  If
> > > > > > > > > the guest is running the option rom, is it the first time the 
> > > > > > > > > option
> > > > > > > > > rom has been run for the machine (ie, was the option rom not 
> > > > > > > > > executed
> > > > > > > > > on the host when the host machine first booted)?
> > > > > > > > > 
> > > > > > > > > FWIW, many of the chromebooks use coreboot with Intel 
> > > > > > > > > graphics and a
> > > > > > > > > number of users have installed SeaBIOS (running natively) on 
> > > > > > > > > their
> > > > > > > > > machines.  Running the intel video option rom more than once 
> > > > > > > > > has been
> > > > > > > > > known to cause issues.  
> > > > > > > > 
> > > > > > > > The issue is in the VM and it occurs every time the option ROM 
> > > > > > > > is
> > > > > > > > executed.  Standard VGA text mode displays fine (ex. SeaBIOS 
> > > > > > > > version
> > > > > > > > string and boot menu), but any sort of extended graphics mode 
> > > > > > > > (ex. live
> > > > > > > > CD boot menu) tries to make use of the host memory area which
> > > > > > > > corresponds to the stolen memory area of the physical device.  
> > > > > > > > We're
> > > > > > > > not really sure how the ROM execution arrives at these 
> > > > > > > > addresses (not
> > > > > > > > from the device according to access traces), but we can see 
> > > > > > > > when the
> > > > > > > > ROM is writing these addresses to the device and modify they 
> > > > > > > > addresses
> > > > > > > > to point at a VM memory range which we've allocated.  That's 
> > > > > > > > what this
> > > > > > > > code attempts to do, allocate a buffer and tell QEMU about it 
> > > > > > > > via the
> > > > > > > > BDSM (Base Data of Stolen Memory) register.  
> > > > > > > 
> > > > > > > Forgive me if I'm not fully understanding this.  If I read what 
> > > > > > > you're
> > > > > > > saying then the sequence is something like:
> > > > > > > 
> > > > > > > 1 - the host system bios (or vgabios) programs the GTT/stolen 
> > > > > > > memory
> > > > > > 

Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Kevin O'Connor]

On Mon, Feb 15, 2016 at 09:29:26PM +0200, Michael S. Tsirkin wrote:
> I can build a generic interface to pass addresses
> allocated by bios back to QEMU. It looks like this would
> be useful for other purposes as well.  Interested?

If this is undertaken, I suggest extending fw_cfg to support "writable
files" instead of introducing a whole new interface.

-Kevin

Re: [Qemu-devel] [RFC] QMP: add query-hotpluggable-cpus

[Markus Armbruster]

Igor Mammedov  writes:

> it will allow mgmt to query present and possible to hotplug CPUs
> it is required from a target platform that wish to support
> command to set board specific MachineClass.possible_cpus() hook,
> which will return a list of possible CPUs with options
> that would be needed for hotplugging possible CPUs.
>
> For RFC there are:
>'arch_id': 'int' - mandatory unique CPU number,
>   for x86 it's APIC ID for ARM it's MPIDR
>'type': 'str' - CPU object type for usage with device_add
>
> and a set of optional fields that would allows mgmt tools
> to know at what granularity and where a new CPU could be
> hotplugged;
> [node],[socket],[core],[thread]
> Hopefully that should cover needs for CPU hotplug porposes for
> magor targets and we can extend structure in future adding
> more fields if it will be needed.
>
> also for present CPUs there is a 'cpu_link' field which
> would allow mgmt inspect whatever object/abstraction
> the target platform considers as CPU object.
>
> For RFC purposes implements only for x86 target so far.

Adding ad hoc queries as we go won't scale.  Could this be solved by a
generic introspection interface?

Re: [Qemu-devel] [RFC PATCH 0/2] ARM: add QMP command to query GIC version

[Markus Armbruster]

Peter Maydell  writes:

> On 15 February 2016 at 15:08, Markus Armbruster  wrote:
>> Peter Xu  writes:
>>> On Mon, Feb 15, 2016 at 10:52:01AM +0100, Markus Armbruster wrote:
 Peter Xu  writes:
 Adding ad hoc queries as we go won't scale.  Is there really no generic
 way to get this information, e.g. with qom-get?
>>>
>>> Haven't used "qom-get" before, but it seems to fetch one property
>>> for a specific object. If so, will it be strange to hide some
>>> capability bits into every GIC objects (though there is possibly
>>> one object)?
>>
>> Pardon my ignorance...  what are these "GIC objects"?
>>
>> What exactly is the "GIC type", and how would the result of
>> query-gic-capability be used?
>
> The GIC type (for our purposes) is the revision of the interrupt
> controller supported by the host, which comes in two versions
> (v2 and v3). These are not compatible, unless your host has
> the v3-with-v2-compat flavour. If a host is v3-only, it is not
> possible for it to give the guest a v2 virtual interrupt
> controller; if v2, it can't give the guest a v3 virtual interrupt
> controller. (If you ask QEMU to do this via command line options
> we will report an error at startup.)

How would the command line look like?

I'm asking because the answer will point us to the introspection problem
to solve.

> The underlying aim is to allow libvirt to say "this VM config
> won't work on this host", rather than ploughing blindly on
> and creating a VM config that always errors on startup.

Yes, finding out with a suitable introspection interface is easier and
more robust than "try and guess what the error message might mean".

> The "GIC object" presumably is the GIC QOM device object.
> However we do the "does this host support this GIC version?"
> check in QEMU before we ever create the GIC device object,
> so trying to probe it for properties isn't going to work.

QMP introspection would ideally cover introspecting device objects:
simply introspect device_add.  But it doesn't cover it now, because QMP
introspection is really a special case of QAPI schema introspection, and
the QAPI schema cannot express device_add for two reasons:

1. The QAPI schema is fixed at compile time, but the device models
aren't.  Instead, whatever device models are linked into QEMU register
themselves during startup.  Collecting them at compile time is a
solvable problem, but unlikely to be fun.

2. The QAPI schema is data, but general QOM properties are code.  As
always, code is much harder to work with than data.  You can't extract
properties from code at compile time, you have to run the code.

To introspect QOM properties, you have to instantiate an object, examine
it, then destroy it.  Aside: this requires instantiate + destroy to have
no lasting side effects, and experience tells that device model code
gets that wrong easily.

Paolo suggested that we special-case device introspection instead of
somehow hook it into QMP introsprection.  Even if we do that, developing
it will take time.

It'll always be easier and more expedient to add another ad hoc query
instead.  But it leaves the generic problem unsolved while the ad hoc
queries pile up.

> The only other QOM object potentially available to probe would be
> the machine (board) object. However as I understand it libvirt
> does all its probing with the "none" machine type, and it seems
> a bit odd to put a bunch of properties on the "none" machine
> type. It would be possible though I guess.

Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Michael S. Tsirkin]

On Mon, Feb 15, 2016 at 12:20:23PM -0700, Alex Williamson wrote:
> On Mon, 15 Feb 2016 10:54:51 +0100
> Igor Mammedov  wrote:
> 
> > On Sat, 13 Feb 2016 18:03:31 -0700
> > Alex Williamson  wrote:
> > 
> > > On Sat, 13 Feb 2016 19:20:32 -0500
> > > "Kevin O'Connor"  wrote:
> > >   
> > > > On Sat, Feb 13, 2016 at 01:57:09PM -0700, Alex Williamson wrote:
> > > > > On Sat, 13 Feb 2016 15:05:09 -0500
> > > > > "Kevin O'Connor"  wrote:  
> > > > > > On Sat, Feb 13, 2016 at 11:51:51AM -0700, Alex Williamson wrote:
> > > > > >   
> > > > > > > On Sat, 13 Feb 2016 13:18:39 -0500
> > > > > > > "Kevin O'Connor"  wrote:
> > > > > > > > On Sat, Feb 13, 2016 at 08:12:09AM -0700, Alex Williamson 
> > > > > > > > wrote:
> > > > > > > > > On Fri, 12 Feb 2016 21:49:04 -0500
> > > > > > > > > "Kevin O'Connor"  wrote:  
> > > > > > > > > > On Fri, Feb 12, 2016 at 05:23:18PM -0700, Alex Williamson 
> > > > > > > > > > wrote:  
> > > > > > > > > > > Intel IGD makes use of memory allocated and marked 
> > > > > > > > > > > reserved by the
> > > > > > > > > > > BIOS as a stolen memory range.  For the most part, guest 
> > > > > > > > > > > drivers don't
> > > > > > > > > > > make use of this, but our achilles heel is the vBIOS.  
> > > > > > > > > > > The vBIOS
> > > > > > > > > > > programs the device to use the host stolen memory range 
> > > > > > > > > > > and it's used
> > > > > > > > > > > in the pre-boot environment.  Generally the guest won't 
> > > > > > > > > > > have access to
> > > > > > > > > > > the host stolen memory area, so these accesses either 
> > > > > > > > > > > land in VM
> > > > > > > > > > > memory or unassigned space and generate IOMMU faults.  By 
> > > > > > > > > > > allocating
> > > > > > > > > > > this range in SeaBIOS and programming it into the device, 
> > > > > > > > > > > QEMU (via
> > > > > > > > > > > vfio) can make sure this guest allocated stolen memory 
> > > > > > > > > > > range is used
> > > > > > > > > > > instead.
> > > > > > > > > > 
> > > > > > > > > > What does "vBIOS" mean in this context?  Is it the video 
> > > > > > > > > > device option
> > > > > > > > > > rom or something else?  
> > > > > > > > > 
> > > > > > > > > vBIOS = video BIOS, you're correct, it's just the video 
> > > > > > > > > device option
> > > > > > > > > ROM.  
> > > > > > > > 
> > > > > > > > Is the problem from when the host runs the video option rom, or 
> > > > > > > > is the
> > > > > > > > problem from the guest (via SeaBIOS) running the video option 
> > > > > > > > rom?  If
> > > > > > > > the guest is running the option rom, is it the first time the 
> > > > > > > > option
> > > > > > > > rom has been run for the machine (ie, was the option rom not 
> > > > > > > > executed
> > > > > > > > on the host when the host machine first booted)?
> > > > > > > > 
> > > > > > > > FWIW, many of the chromebooks use coreboot with Intel graphics 
> > > > > > > > and a
> > > > > > > > number of users have installed SeaBIOS (running natively) on 
> > > > > > > > their
> > > > > > > > machines.  Running the intel video option rom more than once 
> > > > > > > > has been
> > > > > > > > known to cause issues.
> > > > > > > 
> > > > > > > The issue is in the VM and it occurs every time the option ROM is
> > > > > > > executed.  Standard VGA text mode displays fine (ex. SeaBIOS 
> > > > > > > version
> > > > > > > string and boot menu), but any sort of extended graphics mode 
> > > > > > > (ex. live
> > > > > > > CD boot menu) tries to make use of the host memory area which
> > > > > > > corresponds to the stolen memory area of the physical device.  
> > > > > > > We're
> > > > > > > not really sure how the ROM execution arrives at these addresses 
> > > > > > > (not
> > > > > > > from the device according to access traces), but we can see when 
> > > > > > > the
> > > > > > > ROM is writing these addresses to the device and modify they 
> > > > > > > addresses
> > > > > > > to point at a VM memory range which we've allocated.  That's what 
> > > > > > > this
> > > > > > > code attempts to do, allocate a buffer and tell QEMU about it via 
> > > > > > > the
> > > > > > > BDSM (Base Data of Stolen Memory) register.
> > > > > > 
> > > > > > Forgive me if I'm not fully understanding this.  If I read what 
> > > > > > you're
> > > > > > saying then the sequence is something like:
> > > > > > 
> > > > > > 1 - the host system bios (or vgabios) programs the GTT/stolen memory
> > > > > > base register at host system bootup time and reserves it in the
> > > > > > host e820 map.
> > > > > > 
> > > > > > 2 - upon running qemu, the guest reruns the vga bios option rom 
> > > > > > which
> > > > > > seems to work (ie, text mode works)
> > > > > > 
> > > > > > 3 - in the guest, upon running a bootloader 

Re: [Qemu-devel] [RFC PATCH v3 3/3] fw/pci: Allocate IGD stolen memory

[Alex Williamson]

On Mon, 15 Feb 2016 10:54:51 +0100
Igor Mammedov  wrote:

> On Sat, 13 Feb 2016 18:03:31 -0700
> Alex Williamson  wrote:
> 
> > On Sat, 13 Feb 2016 19:20:32 -0500
> > "Kevin O'Connor"  wrote:
> >   
> > > On Sat, Feb 13, 2016 at 01:57:09PM -0700, Alex Williamson wrote:
> > > > On Sat, 13 Feb 2016 15:05:09 -0500
> > > > "Kevin O'Connor"  wrote:  
> > > > > On Sat, Feb 13, 2016 at 11:51:51AM -0700, Alex Williamson wrote:  
> > > > > > On Sat, 13 Feb 2016 13:18:39 -0500
> > > > > > "Kevin O'Connor"  wrote:
> > > > > > > On Sat, Feb 13, 2016 at 08:12:09AM -0700, Alex Williamson wrote:  
> > > > > > >   
> > > > > > > > On Fri, 12 Feb 2016 21:49:04 -0500
> > > > > > > > "Kevin O'Connor"  wrote:  
> > > > > > > > > On Fri, Feb 12, 2016 at 05:23:18PM -0700, Alex Williamson 
> > > > > > > > > wrote:  
> > > > > > > > > > Intel IGD makes use of memory allocated and marked reserved 
> > > > > > > > > > by the
> > > > > > > > > > BIOS as a stolen memory range.  For the most part, guest 
> > > > > > > > > > drivers don't
> > > > > > > > > > make use of this, but our achilles heel is the vBIOS.  The 
> > > > > > > > > > vBIOS
> > > > > > > > > > programs the device to use the host stolen memory range and 
> > > > > > > > > > it's used
> > > > > > > > > > in the pre-boot environment.  Generally the guest won't 
> > > > > > > > > > have access to
> > > > > > > > > > the host stolen memory area, so these accesses either land 
> > > > > > > > > > in VM
> > > > > > > > > > memory or unassigned space and generate IOMMU faults.  By 
> > > > > > > > > > allocating
> > > > > > > > > > this range in SeaBIOS and programming it into the device, 
> > > > > > > > > > QEMU (via
> > > > > > > > > > vfio) can make sure this guest allocated stolen memory 
> > > > > > > > > > range is used
> > > > > > > > > > instead.
> > > > > > > > > 
> > > > > > > > > What does "vBIOS" mean in this context?  Is it the video 
> > > > > > > > > device option
> > > > > > > > > rom or something else?  
> > > > > > > > 
> > > > > > > > vBIOS = video BIOS, you're correct, it's just the video device 
> > > > > > > > option
> > > > > > > > ROM.  
> > > > > > > 
> > > > > > > Is the problem from when the host runs the video option rom, or 
> > > > > > > is the
> > > > > > > problem from the guest (via SeaBIOS) running the video option 
> > > > > > > rom?  If
> > > > > > > the guest is running the option rom, is it the first time the 
> > > > > > > option
> > > > > > > rom has been run for the machine (ie, was the option rom not 
> > > > > > > executed
> > > > > > > on the host when the host machine first booted)?
> > > > > > > 
> > > > > > > FWIW, many of the chromebooks use coreboot with Intel graphics 
> > > > > > > and a
> > > > > > > number of users have installed SeaBIOS (running natively) on their
> > > > > > > machines.  Running the intel video option rom more than once has 
> > > > > > > been
> > > > > > > known to cause issues.
> > > > > > 
> > > > > > The issue is in the VM and it occurs every time the option ROM is
> > > > > > executed.  Standard VGA text mode displays fine (ex. SeaBIOS version
> > > > > > string and boot menu), but any sort of extended graphics mode (ex. 
> > > > > > live
> > > > > > CD boot menu) tries to make use of the host memory area which
> > > > > > corresponds to the stolen memory area of the physical device.  We're
> > > > > > not really sure how the ROM execution arrives at these addresses 
> > > > > > (not
> > > > > > from the device according to access traces), but we can see when the
> > > > > > ROM is writing these addresses to the device and modify they 
> > > > > > addresses
> > > > > > to point at a VM memory range which we've allocated.  That's what 
> > > > > > this
> > > > > > code attempts to do, allocate a buffer and tell QEMU about it via 
> > > > > > the
> > > > > > BDSM (Base Data of Stolen Memory) register.
> > > > > 
> > > > > Forgive me if I'm not fully understanding this.  If I read what you're
> > > > > saying then the sequence is something like:
> > > > > 
> > > > > 1 - the host system bios (or vgabios) programs the GTT/stolen memory
> > > > > base register at host system bootup time and reserves it in the
> > > > > host e820 map.
> > > > > 
> > > > > 2 - upon running qemu, the guest reruns the vga bios option rom which
> > > > > seems to work (ie, text mode works)
> > > > > 
> > > > > 3 - in the guest, upon running a bootloader that uses graphics mode,
> > > > > the bootloader calls the vgabios to switch to graphics mode, and
> > > > > the vgabios sends commands to the graphics hardware that somehow
> > > > > reference the host stolen memory  
> > > > 
> > > > What exactly happens here isn't clear to me, but this is a plausible
> > > > explanation.  What we see 

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: 140: use virtio-scsi instead of IDE

[Max Reitz]

On 10.02.2016 20:26, Sascha Silbe wrote:
> Dear Max,
> 
> Max Reitz  writes:
> 
> [tests/qemu-iotests/140]
>>> -_launch_qemu -drive 
>>> if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
>>> -2> >(_filter_nbd)
>>> +_launch_qemu -drive 
>>> if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \
>>> +-device virtio-scsi -device scsi-cd,drive=drv 2> >(_filter_nbd)
>>
>> Why not just omit the device (and the media=cdrom along with it, keeping
>> if=none)? This will change the reference output because there is no
>> longer any tray to be moved, but this will still test what it's supposed to.
>>
>> (This may sound hypocritical coming from me, because I wrote this test
>> so I could have just done so in the first place; I guess I just didn't
>> realize that 'eject' works on device-less drives, too.)
> 
> Is this supposed to work? I.e. can we rely on it?

Let's say I would rely on it. :-) (which is why I proposed it)

The test checks that ejecting a BlockDriverState tree from a
BlockBackend (that is, a medium from a drive) works even if that
BlockBackend is exposed via an NBD server. It doesn't really matter
whether the drive has a device or not, the main thing is that the NBD
server notices that the medium is ejected and automatically stops
offering the drive.

So I would think that we are supposed to be able to rely on it; if we
cannot, something else is probably broken.

>   If so, that would
> certainly be the easier route for this particular test. Test coverage
> should be unaffected as 139 already tests ejection (using virtio, unlike
> 118 which is PC-only).
> 
> The aliases patch has a value of its own, but that's a separate
> matter.

Yes, certainly.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] proposed schedule for 2.6 release

[Peter Maydell]

On 2 February 2016 at 15:45, Peter Maydell  wrote:
> I've sketched out some proposed dates for QEMU 2.6's
> release schedule (also at http://wiki.qemu.org/Planning/2.6)

> That would put softfreeze just three weeks away. Should we shove
> all the dates forward a week?

Deafening lack of feedback, so I'm going to do what suits me
personally best, and move the dates forward a week. That means:

2016-03-01 Soft feature freeze.
2016-03-29 Hard feature freeze. Tag -rc0
2016-04-05 Tag v2.6.0-rc1
2016-04-12 Tag v2.6.0-rc2
2016-04-19 Tag v2.6.0-rc3
2016-04-26 Tag v2.6.0

Advance warning: I don't want to slip into May if we can avoid it,
so I'm likely to be strict about letting things into the various
rcs. Please get your code and pull requests in *early*, not late,
or you may find they are too late!

thanks
-- PMM

[Qemu-devel] [PATCH] migration: fix incorrect memory_global_dirty_log_start outside BQL

[Paolo Bonzini]

This can cause various segmentation faults or aborts in qemu-iotests
test 091.

Fixes: 5b82b703b69acc67b78b98a5efc897a3912719eb
Cc: Dave Gilbert 
Cc: Stefan Hajnoczi 
Signed-off-by: Paolo Bonzini 
---
 migration/ram.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 96c749f..704f6a9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1920,6 +1920,9 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 acct_clear();
 }
 
+/* For memory_global_dirty_log_start below.  */
+qemu_mutex_lock_iothread();
+
 qemu_mutex_lock_ramlist();
 rcu_read_lock();
 bytes_transferred = 0;
@@ -1944,6 +1947,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 memory_global_dirty_log_start();
 migration_bitmap_sync();
 qemu_mutex_unlock_ramlist();
+qemu_mutex_unlock_iothread();
 
 qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
 
-- 
2.5.0

[Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

https://patchwork.ozlabs.org/patch/579325/

Mac OS X can be picky when it comes to allowing the user
to use physical devices in QEMU. Most mounted volumes
appear to be off limits to QEMU. If an issue is detected,
a message is displayed showing the user how to unmount a
volume. Now QEMU uses both CD and DVD media.

Signed-off-by: John Arbuckle 

---
Changed filename variable to const char * type.
Removed snprintf call for filename variable.
filename is set to bsd_path if using a physical device that isn't a DVD or CD.


 block/raw-posix.c |  167 -
 1 files changed, 127 insertions(+), 40 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6df3067..48dc5a8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -44,6 +44,7 @@
 #include 
 #include 
 //#include 
+#include 
 #include 
 #endif
 
@@ -1963,33 +1964,47 @@ BlockDriver bdrv_file = {
 /* host device */
 
 #if defined(__APPLE__) && defined(__MACH__)
-static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
 static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
 CFIndex maxPathSize, int flags);
-kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
+static char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
 {
-kern_return_t   kernResult;
+kern_return_t kernResult = KERN_FAILURE;
 mach_port_t masterPort;
 CFMutableDictionaryRef  classesToMatch;
+const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
+char *mediaType = NULL;
 
 kernResult = IOMasterPort( MACH_PORT_NULL,  );
 if ( KERN_SUCCESS != kernResult ) {
 printf( "IOMasterPort returned %d\n", kernResult );
 }
 
-classesToMatch = IOServiceMatching( kIOCDMediaClass );
-if ( classesToMatch == NULL ) {
-printf( "IOServiceMatching returned a NULL dictionary.\n" );
-} else {
-CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), 
kCFBooleanTrue );
-}
-kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, 
mediaIterator );
-if ( KERN_SUCCESS != kernResult )
-{
-printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
-}
+int index;
+for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
+classesToMatch = IOServiceMatching(matching_array[index]);
+if (classesToMatch == NULL) {
+error_report("IOServiceMatching returned NULL for %s",
+ matching_array[index]);
+continue;
+}
+CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
+ kCFBooleanTrue);
+kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
+  mediaIterator);
+if (kernResult != KERN_SUCCESS) {
+error_report("Note: IOServiceGetMatchingServices returned %d",
+ kernResult);
+continue;
+}
 
-return kernResult;
+/* If a match was found, leave the loop */
+if (*mediaIterator != 0) {
+DPRINTF("Matching using %s\n", matching_array[index]);
+mediaType = g_strdup(matching_array[index]);
+break;
+}
+}
+return mediaType;
 }
 
 kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
@@ -2021,7 +2036,46 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
char *bsdPath,
 return kernResult;
 }
 
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+int index, num_of_test_partitions = 2, fd;
+char test_partition[MAXPATHLEN];
+bool partition_found = false;
+
+/* look for a working partition */
+for (index = 0; index < num_of_test_partitions; index++) {
+snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
+ index);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partition_found = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* if a working partition on the device was not found */
+if (partition_found == false) {
+error_setg(errp, "Failed to find a working partition on disc");
+} else {
+DPRINTF("Using %s as optical disc\n", test_partition);
+pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+}
+return partition_found;
+}
+
+/* Prints directions on mounting and unmounting a device */
+static void print_unmounting_directions(const char *file_name)
+{
+error_report("If device %s is mounted on the desktop, unmount"
+ " it first before using it in QEMU", file_name);
+error_report("Command to unmount device: diskutil unmountDisk %s",
+ file_name);
+error_report("Command to 

Re: [Qemu-devel] [PATCH v2 06/11] nvdimm acpi: initialize the resource used by NVDIMM ACPI

[Xiao Guangrong]

On 02/16/2016 01:24 AM, Igor Mammedov wrote:

On Mon, 15 Feb 2016 23:53:13 +0800
Xiao Guangrong  wrote:


On 02/15/2016 09:32 PM, Igor Mammedov wrote:

On Mon, 15 Feb 2016 13:45:59 +0200
"Michael S. Tsirkin"  wrote:


On Mon, Feb 15, 2016 at 11:47:42AM +0100, Igor Mammedov wrote:

On Mon, 15 Feb 2016 18:13:38 +0800
Xiao Guangrong  wrote:


On 02/15/2016 05:18 PM, Michael S. Tsirkin wrote:

On Mon, Feb 15, 2016 at 10:11:05AM +0100, Igor Mammedov wrote:

On Sun, 14 Feb 2016 13:57:27 +0800
Xiao Guangrong  wrote:


On 02/08/2016 07:03 PM, Igor Mammedov wrote:

On Wed, 13 Jan 2016 02:50:05 +0800
Xiao Guangrong  wrote:


32 bits IO port starting from 0x0a18 in guest is reserved for NVDIMM
ACPI emulation. The table, NVDIMM_DSM_MEM_FILE, will be patched into
NVDIMM ACPI binary code

OSPM uses this port to tell QEMU the final address of the DSM memory
and notify QEMU to emulate the DSM method

Would you need to pass control to QEMU if each NVDIMM had its whole
label area MemoryRegion mapped right after its storage MemoryRegion?



No, label data is not mapped into guest's address space and it only
can be accessed by DSM method indirectly.

Yep, per spec label data should be accessed via _DSM but question
wasn't about it,


Ah, sorry, i missed your question.


Why would one map only 4Kb window and serialize label data
via it if it could be mapped as whole, that way _DMS method will be
much less complicated and there won't be need to add/support a protocol
for its serialization.



Is it ever accessed on data path? If not I prefer the current approach:


The label data is only accessed via two DSM commands - Get Namespace Label
Data and Set Namespace Label Data, no other place need to be emulated.


limit the window used, the serialization protocol seems rather simple.



Yes.

Label data is at least 128k which is big enough for BIOS as it allocates
memory at 0 ~ 4G which is tight region. It also needs guest OS to support
lager max-xfer (the max size that can be transferred one time), the size
in current Linux NVDIMM driver is 4k.

However, using lager DSM buffer can help us to simplify NVDIMM hotplug for
the case that too many nvdimm devices present in the system and their FIT
info can not be filled into one page. Each PMEM-only device needs 0xb8 bytes
and we can append 256 memory devices at most, so 12 pages are needed to
contain this info. The prototype we implemented is using ourself-defined
protocol to read piece of _FIT and concatenate them before return to Guest,
please refer to:
https://github.com/xiaogr/qemu/commit/c46ce01c8433ac0870670304360b3c4aa414143a

As 12 pages are not small region for BIOS and the _FIT size may be extended in 
the
future development (eg, if PBLK is introduced) i am not sure if we need this. Of
course, another approach to simplify it is that we limit the number of NVDIMM
device to make sure their _FIT < 4k.

My suggestion is not to have only one label area for every NVDIMM but
rather to map each label area right after each NVDIMM's data memory.
That way _DMS can be made non-serialized and guest could handle
label data in parallel.


I think that alignment considerations would mean we are burning up
1G of phys address space for this. For PAE we only have 64G
of this address space, so this would be a problem.

That's true that it will burning away address space, however that
just means that PAE guests would not be able to handle as many
NVDIMMs as 64bit guests. The same applies to DIMMs as well, with
alignment enforced. If one needs more DIMMs he/she can switch
to 64bit guest to use them.

It's trade of inefficient GPA consumption vs efficient NVDIMMs access.
Also with fully mapped label area for each NVDIMM we don't have to
introduce and maintain any guest visible serialization protocol
(protocol for serializing _DSM via 4K window) which becomes ABI.


It's true for label access but it is not for the long term as we will
need to support other _DSM commands such as vendor specific command,
PBLK dsm command, also NVDIMM MCE related commands will be introduced
in the future, so we will come back here at that time. :(

I believe for block mode NVDIMM would also need per NVDIMM mapping
for performance reasons (parallel access).
As for the rest could that commands go via MMIO that we usually
use for control path?


So both input data and output data go through single MMIO, we need to
introduce a protocol to pass these data, that is complex?

And is any MMIO we can reuse (more complexer？) or we should allocate this
MMIO page （the old question - where to allocated?）?

Re: [Qemu-devel] [PATCH 6/8] ipmi: provide support for FRUs

[Marcel Apfelbaum]

On 02/15/2016 07:17 PM, Cédric Le Goater wrote:

On 02/14/2016 10:25 AM, Marcel Apfelbaum wrote:

On 02/09/2016 02:13 PM, Cédric Le Goater wrote:

This patch provides a simple FRU support for the BMC simulator. FRUs
are loaded from a file which name is specified in the object
properties, each entry having a fixed size, also specified in the
properties. If the file is unknown or not accessible for some reason,
a unique entry of 1024 bytes is created as a default. Just enough to
start some simulation.

Signed-off-by: Cédric Le Goater 
---
   hw/ipmi/ipmi_bmc_sim.c | 140 
+
   1 file changed, 140 insertions(+)

diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 69318eb6b556..b0754893fc08 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -80,6 +80,9 @@
   #define IPMI_CMD_ENTER_SDR_REP_UPD_MODE   0x2A
   #define IPMI_CMD_EXIT_SDR_REP_UPD_MODE0x2B
   #define IPMI_CMD_RUN_INIT_AGENT   0x2C
+#define IPMI_CMD_GET_FRU_AREA_INFO0x10
+#define IPMI_CMD_READ_FRU_DATA0x11
+#define IPMI_CMD_WRITE_FRU_DATA   0x12
   #define IPMI_CMD_GET_SEL_INFO 0x40
   #define IPMI_CMD_GET_SEL_ALLOC_INFO   0x41
   #define IPMI_CMD_RESERVE_SEL  0x42
@@ -122,6 +125,13 @@ typedef struct IPMISdr {
   uint8_t overflow;
   } IPMISdr;

+typedef struct IPMIFru {
+char *filename;
+unsigned int nentries;
+uint16_t size;
+uint8_t *data;
+} IPMIFru;
+
   typedef struct IPMISensor {
   uint8_t status;
   uint8_t reading;
@@ -208,6 +218,7 @@ struct IPMIBmcSim {

   IPMISel sel;
   IPMISdr sdr;
+IPMIFru fru;
   IPMISensor sensors[MAX_SENSORS];
   char *sdr_filename;

@@ -1314,6 +1325,103 @@ static void get_sel_info(IPMIBmcSim *ibs,
   IPMI_ADD_RSP_DATA((ibs->sel.overflow << 7) | 0x02);
   }

+static void get_fru_area_info(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int max_rsp_len)
+{
+uint8_t fruid;
+uint16_t fru_entry_size;
+
+IPMI_CHECK_CMD_LEN(3);


Hi,

This is little awkward for me. The cmd_len and rsp
parameters of the macro are implied.


hmm, I am not sure what you mean. Are you concerned by that fact
we could overflow rsp and cmd ?


Not really, no. Something more simple:

IPMI_CHECK_CMD_LEN(3) should be actually IPMI_CHECK_CMD_LEN(3, cmd_len, rsp).
What bothers me is that both cmd_len and rsp are implied by the macro.

In other words, we don't know what parameters IPMI_CHECK_CMD_LEN really has.
"3" is for sure not enough, so we need to guess or look a the macro definition
to see what it uses.

But again, maybe is only me.

Thanks,
Marcel




I guess we could probably move the IPMI_CHECK_CMD_LEN in the caller
of these commands and use an array of expected argument lengths.
For rsp, IPMI_ADD_RSP_DATA needs to be used to check that we are not
exceeding max_rsp_len


Am I the only one this bothers?


+
+fruid = cmd[2];
+
+if (fruid >= ibs->fru.nentries) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+fru_entry_size = ibs->fru.size;
+
+IPMI_ADD_RSP_DATA(fru_entry_size & 0xff);
+IPMI_ADD_RSP_DATA(fru_entry_size >> 8 & 0xff);
+IPMI_ADD_RSP_DATA(0x0);


Same here. By the way, do you have some spec for the above or
is an ad-hoc encoding of the fields? If yes, you could
add a reference for the spec.(This is also for the other functions in this 
patch)


Yes I will add the reference.

Thanks,

C.



Thanks,
Marcel


+}
+
+#define min(x, y) ((x) < (y) ? (x) : (y))
+#define max(x, y) ((x) > (y) ? (x) : (y))
+
+static void read_fru_data(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int max_rsp_len)
+{
+uint8_t fruid;
+uint16_t offset;
+int i;
+uint8_t *fru_entry;
+unsigned int count;
+
+IPMI_CHECK_CMD_LEN(5);
+
+fruid = cmd[2];
+offset = (cmd[3] | cmd[4] << 8);
+
+if (fruid >= ibs->fru.nentries) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+if (offset >= ibs->fru.size - 1) {
+rsp[2] = IPMI_CC_INVALID_DATA_FIELD;
+return;
+}
+
+fru_entry = >fru.data[fruid * ibs->fru.size];
+
+count = min(cmd[5], ibs->fru.size - offset);
+
+IPMI_ADD_RSP_DATA(count & 0xff);
+for (i = 0; i < count; i++) {
+IPMI_ADD_RSP_DATA(fru_entry[offset + i]);
+}
+}
+
+static void write_fru_data(IPMIBmcSim *ibs,
+ uint8_t *cmd, unsigned int cmd_len,
+ uint8_t *rsp, unsigned int *rsp_len,
+ unsigned int max_rsp_len)
+{
+uint8_t fruid;
+uint16_t offset;
+uint8_t *fru_entry;
+unsigned int count;
+
+IPMI_CHECK_CMD_LEN(5);
+
+fruid = cmd[2];
+

[Qemu-devel] [PATCH] vl: fix migration from prelaunch state

[Paolo Bonzini]

Reproducer is simply to migrate a virtual machine that was started with -S.

Signed-off-by: Paolo Bonzini 
---
 vl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vl.c b/vl.c
index 1ec62ac..e600f8d 100644
--- a/vl.c
+++ b/vl.c
@@ -590,6 +590,7 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 { RUN_STATE_INMIGRATE, RUN_STATE_WATCHDOG },
 { RUN_STATE_INMIGRATE, RUN_STATE_GUEST_PANICKED },
 { RUN_STATE_INMIGRATE, RUN_STATE_FINISH_MIGRATE },
+{ RUN_STATE_INMIGRATE, RUN_STATE_PRELAUNCH },
 
 { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
 { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 00/14] QOM'ify hw/timer/*

[Peter Maydell]

On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
> This patch series QOM'ify timer code under hw/timer directory.
> Main idea is to split the initfn's work, some to TypeInfo.instance_init
> and some is placed in DeviceClass::realize.
> Drop the use of SysBusDeviceClass::init if possible.
>
> changes since v1:
> fix a stupid typo (timmer->timer)

Hi; thanks for this patchset. I've reviewed most of these
and I think my comments on the remaining few patches would
basically be the same as the ones I have reviewed.
I would recommend that you cc the maintainers for the relevant
devices on this patchset when you send out your next version.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 14/14] hw/timer: QOM'ify tusb6010 and remove all tabs

[Peter Maydell]

On 27 January 2016 at 02:56, xiaoqiang zhao  wrote:
> * assign tusb6010_init to tusb6010_info.instance_init and drop the 
> SysBusDeviceClass::init
> * use spaces instead of tabs

Please don't do whitespace/indentation changes and real
code changes in the same patch. Indent changes should always
go in their own patch if you really need to make them (though
in this case I would not bother personally).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 06/14] hw/timer: QOM'ify grlib_gptimer

[Peter Maydell]

On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
> * split grlib_gptimer_init into grlib_gptimer_info.instance_init and 
> grlib_gptimer_realize
> * use DeviceClass::realize instead of SysBusDeviceClass::init

Same comments as for other patches:
 * line wrapping
 * OBJECT casts now unnecessary
 * please cc the maintainer
(in this case also adding Mark Cave-Ayland 
as overall sparc maintainer would be a good idea).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 07/14] hw/timer: QOM'ify lm32_timer

[Peter Maydell]

On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
> * split lm32_timer_init into lm32_timer_info.instance_init and 
> lm32_timer_realize
> * use DeviceClass::realize instead of SysBusDeviceClass::init

Long lines again.

> Signed-off-by: xiaoqiang zhao 
> ---
>  hw/timer/lm32_timer.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/timer/lm32_timer.c b/hw/timer/lm32_timer.c
> index d2ab1e7..4ee080a 100644
> --- a/hw/timer/lm32_timer.c
> +++ b/hw/timer/lm32_timer.c
> @@ -175,21 +175,26 @@ static void timer_reset(DeviceState *d)
>  ptimer_stop(s->ptimer);
>  }
>
> -static int lm32_timer_init(SysBusDevice *dev)
> +static void lm32_timer_init(Object *obj)
>  {
> -LM32TimerState *s = LM32_TIMER(dev);
> +LM32TimerState *s = LM32_TIMER(obj);
> +SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>
>  sysbus_init_irq(dev, >irq);
>
>  s->bh = qemu_bh_new(timer_hit, s);
>  s->ptimer = ptimer_init(s->bh);
> -ptimer_set_freq(s->ptimer, s->freq_hz);
>
>  memory_region_init_io(>iomem, OBJECT(s), _ops, s,
>"timer", R_MAX * 4);
>  sysbus_init_mmio(dev, >iomem);
> +}

You could avoid the OBJECT() cast here now.

Otherwise:
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 09/14] hw/timer: QOM'ify milkymist_sysctl

[Peter Maydell]

On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
> * split milkymist_sysctl_init into milkymist_sysctl_info.instance_init and 
> milkymist_sysctl_realize

I think the "info" in this function name is wrong ?

> * use DeviceClass::realize instead of SysBusDeviceClass::init

Please make sure you line wrap your commit messages (at somewhere
around 70-72 columns is usual).

Otherwise:
Reviewed-by: Peter Maydell 

Again, you should cc the maintainer for this device.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 04/14] hw/timer: QOM'ify exynos4210_pwm

[Peter Maydell]

On 27 January 2016 at 02:54, xiaoqiang zhao  wrote:
> assign exynos4210_pwm_init to exynos4210_pwm_info.instance_init
> and drop the SysBusDeviceClass::init
>
> Signed-off-by: xiaoqiang zhao 
> ---
>  hw/timer/exynos4210_pwm.c | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Peter Maydell 

thanks
-- PMM