[Qemu-devel] [Bug 1221966] Re: SIGSEGV in static_code_gen_buffer

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

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

Title:
  SIGSEGV in static_code_gen_buffer

Status in QEMU:
  Expired

Bug description:
  Trying to run 'ls' (or, anything else as far as I can tell) from a
  SunOS 5.8 box under RHEL 6.4 linux, I get a segfault.  I've tried
  qemu-1.5.3, qemu-1.6.0, and I fetched git://git.qemu-
  project.org/qemu.git.  I've also tried a statically linked sh from
  /sbin/ and it also segfaulted.

  wcolburn@anotheruvula$ gdb bin/qemu-sparc
  GNU gdb (GDB) Red Hat Enterprise Linux (7.2-60.el6_4.1)
  Copyright (C) 2010 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later 
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "x86_64-redhat-linux-gnu".
  For bug reporting instructions, please see:
  ...
  Reading symbols from /home/anotheruvula/qemu/bin/qemu-sparc...done.
  (gdb) run ../sparc/ls
  Starting program: /home/anotheruvula/qemu/bin/qemu-sparc ../sparc/ls
  [Thread debugging using libthread_db enabled]

  Program received signal SIGSEGV, Segmentation fault.
  0x78259116 in static_code_gen_buffer ()
  Missing separate debuginfos, use: debuginfo-install glib2-2.22.5-7.el6.x86_64 
glibc-2.12-1.107.el6_4.4.x86_64
  (gdb) where
  #0  0x78259116 in static_code_gen_buffer ()
  #1  0x77f570cd in cpu_tb_exec (cpu=0x7a2b1210, tb_ptr=
  0x782590d0 "A\213n \205í\017\205Í")
  at /home/anotheruvula/qemu-devel/cpu-exec.c:56
  #2  0x77f57b2d in cpu_sparc_exec (env=0x7a2b1348)
  at /home/anotheruvula/qemu-devel/cpu-exec.c:631
  #3  0x77f77f36 in cpu_loop (env=0x7a2b1348)
  at /home/anotheruvula/qemu-devel/linux-user/main.c:1089
  #4  0x77f798ff in main (argc=2, argv=0x7fffdfc8, envp=
  0x7fffdfe0) at /home/anotheruvula/qemu-devel/linux-user/main.c:4083
  (gdb)

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

[Qemu-devel] [Bug 1243639] Re: qemu-1.5.3 segment fault with -vga qxl

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

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

Title:
  qemu-1.5.3   segment fault  with  -vga qxl

Status in QEMU:
  Expired

Bug description:
  execute " qemu-system-x86_64-enable-kvm -machine accel=kvm:tcg -m
  1G  -drive file=/dev/sda  --full-screen -spice
  addr=127.0.0.1,port=5900,disable-ticketing -vga qxl "  on shell will
  get  segment fault  after  a few seconds   if  I  don't connect to it
  with  spicec client  immediately.

  IF  excute  "spicec -h 127.0.0.1 -p 5900 "  immediately after
  the  qemu-system-x86_64  execution, then  no segment fault happens
  and  it runs well.

  =

  GDB output:

  root@kali-john:~# gdb /usr/local/bin/qemu-system-x86_64
  GNU gdb (GDB) 7.4.1-debian
  (gdb) run -enable-kvm -machine accel=kvm:tcg -m 1G  -drive file=/dev/sda  
--full-screen -spice addr=127.0.0.1,port=5900,disable-ticketing -vga qxl

  [Thread debugging using libthread_db enabled]
  Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
  [New Thread 0x73737700 (LWP 14797)]
  [New Thread 0x72d54700 (LWP 14798)]
  [New Thread 0x70fff700 (LWP 14799)]

  Program received signal SIGSEGV, Segmentation fault.
  0x7683ad70 in pixman_image_get_data () from 
/usr/lib/x86_64-linux-gnu/libpixman-1.so.0
  (gdb) bt
  #0  0x7683ad70 in pixman_image_get_data () from 
/usr/lib/x86_64-linux-gnu/libpixman-1.so.0
  #1  0x5581060a in surface_data (s=0x566183a0) at 
/zh-download/QEMU/qemu-1.5.3/include/ui/console.h:235
  #2  0x55818616 in vga_draw_graphic (s=0x5662c778, full_update=1) 
at /zh-download/QEMU/qemu-1.5.3/hw/display/vga.c:1788
  #3  0x55818c6a in vga_update_display (opaque=0x5662c778) at 
/zh-download/QEMU/qemu-1.5.3/hw/display/vga.c:1917
  #4  0x5580eb15 in qxl_hw_update (opaque=0x5662bd70) at 
/zh-download/QEMU/qemu-1.5.3/hw/display/qxl.c:1766
  #5  0x557bd6bc in graphic_hw_update (con=0x56618d00) at 
ui/console.c:254
  #6  0x557c8426 in qemu_spice_display_refresh (ssd=0x5662c418) at 
ui/spice-display.c:417
  #7  0x5580eff0 in display_refresh (dcl=0x5662c420) at 
/zh-download/QEMU/qemu-1.5.3/hw/display/qxl.c:1886
  #8  0x557c0cb1 in dpy_refresh (s=0x56618370) at ui/console.c:1436
  #9  0x557bd3af in gui_update (opaque=0x56618370) at 
ui/console.c:192
  #10 0x55797f20 in qemu_run_timers (clock=0x565b5a30) at 
qemu-timer.c:394
  #11 0x55798183 in qemu_run_all_timers () at qemu-timer.c:453
  #12 0x55760bb7 in main_loop_wait (nonblocking=0) at main-loop.c:470
  #13 0x557cd19c in main_loop () at vl.c:2029
  #14 0x557d43f2 in main (argc=13, argv=0x7fffe2b8, 
envp=0x7fffe328) at vl.c:4419
  (gdb) 

  
  ==

  http://www.spice-space.org/download/releases/spice-0.12.4.tar.bz2
  http://www.spice-space.org/download/releases/spice-protocol-0.12.6.tar.bz2
  spice  compiling 
./configure --enable-smartcard=no   && make

  qemu-1.5.3
  compiling 
  ./configure \
  --disable-strip  --enable-debug \
  --target-list=x86_64-softmmu,x86_64-linux-user  \
  --disable-sdl  --audio-drv-list=alsa --disable-vnc --disable-xen 
--disable-libiscsi  \
--disable-seccomp --disable-glusterfs --disable-libssh2 
--disable-smartcard-nss  \
--disable-usb-redir --disable-brlapi --disable-curl  --disable-bsd-user 
\
\
  --enable-kvm --enable-spice --enable-system --enable-guest-agent 
--enable-vhost-net 

  
  root@kali-john:~# qemu-system-x86_64 -version
  QEMU emulator version 1.5.3, Copyright (c) 2003-2008 Fabrice Bellard

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

[Qemu-devel] [Bug 1248469] Re: qemu 1.6.1 q35 ioh3420 not work in windows 7 32bit

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

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

Title:
  qemu 1.6.1 q35 ioh3420 not work in windows 7 32bit

Status in QEMU:
  Expired

Bug description:
  boot windows 7 32bit guest with -readconfig q35-chipset.cfg
  paramter,in guest's device manager,there's a device 3420 not work,it
  shows error "This device cannot find enough free resources that it can
  use(code 12)".

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

[Qemu-devel] [Bug 1646610] Re: "Assertion `!r->req.sg' failed." during live migration with VirtIO

[Launchpad Bug Tracker]

[Expired for QEMU because there has been no activity for 60 days.]

** Changed in: qemu
   Status: Incomplete => Expired

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

Title:
  "Assertion `!r->req.sg' failed." during live migration with VirtIO

Status in QEMU:
  Expired

Bug description:
  We've hit this issue twice so far, but don't have an obvious repro
  yet. It's pretty rare for us to hit it but I'm still trying so I can
  get a core and backtrace. The guest was Windows running a constant
  workload. We were using VirtIO SCSI drivers in both cases.

  In both cases we hit the assert here:

  hw/scsi/scsi-generic.c:

  static void scsi_generic_save_request(QEMUFile *f, SCSIRequest *req)
  {
  SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);

  qemu_put_sbe32s(f, >buflen);
  if (r->buflen && r->req.cmd.mode == SCSI_XFER_TO_DEV) {
  *** assert(!r->req.sg);
  qemu_put_buffer(f, r->buf, r->req.cmd.xfer);
  }
  }

  From code inspection, it seems that this will always happen if
  scsi_req_enqueue_internal in hw/scsi/scsi-bus.c is ever invoked.

  static void scsi_req_enqueue_internal(SCSIRequest *req)
  {
  assert(!req->enqueued);
  scsi_req_ref(req);
  if (req->bus->info->get_sg_list) {
  req->sg = req->bus->info->get_sg_list(req);
  } else {
  req->sg = NULL;
  }
  req->enqueued = true;
  QTAILQ_INSERT_TAIL(>dev->requests, req, next);
  }

  req->bus->info->get_sg_list will return >qsgl for a virtio-scsi
  bus since it's a method stored on the SCSIBusInfo struct. I didn't see
  anything that would clear the req->sg if scsi_req_enqueue_internal is
  called at least once.

  I think this can only happen if scsi_dma_restart_bh in hw/scsi/scsi-
  bus.c is called. The only other location I see
  scsi_req_enqueue_internal is on the load side for the destination of a
  migration.

  static void scsi_dma_restart_bh(void *opaque)
  {
  SCSIDevice *s = opaque;
  SCSIRequest *req, *next;

  qemu_bh_delete(s->bh);
  s->bh = NULL;

  QTAILQ_FOREACH_SAFE(req, >requests, next, next) {
  scsi_req_ref(req);
  if (req->retry) {
  req->retry = false;
  switch (req->cmd.mode) {
  case SCSI_XFER_FROM_DEV:
  case SCSI_XFER_TO_DEV:
  scsi_req_continue(req);
  break;
  case SCSI_XFER_NONE:
  scsi_req_dequeue(req);
  scsi_req_enqueue(req); // *** this calls 
scsi_req_enqueue_internal
  break;
  }
  }
  scsi_req_unref(req);
  }
  }

  Finally when put_scsi_requests is called for migration, it seems like
  it will call both virtio_scsi_save_request (from
  bus->info->save_request) and scsi_generic_save_request (from
  req->ops->save_request) and trigger the assert.

  I searched for a bit, but didn't find anyone else reporting this. Has
  anyone else seen this? It seems to me like that assert should check
  that the sg list is empty instead of checking that it exists. Is this
  an appropriate assessment? Assuming I find a repro, I'll try to test
  this solution.

  Thanks!

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

[Qemu-devel] [PATCH 2/3] slirp: Convert mbufs to use g_malloc() and g_free()

[Peter Maydell]

The mbuf code currently doesn't check the result of doing a malloc()
or realloc() of its data (spotted by Coverity, CID 1238946).
Since the m_inc() API assumes that extending an mbuf must succeed,
just convert to g_malloc() and g_free().

Signed-off-by: Peter Maydell 
---
 slirp/mbuf.c | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/slirp/mbuf.c b/slirp/mbuf.c
index 7eddc21..5ff2455 100644
--- a/slirp/mbuf.c
+++ b/slirp/mbuf.c
@@ -10,7 +10,7 @@
  * FreeBSD.  They are fixed size, determined by the MTU,
  * so that one whole packet can fit.  Mbuf's cannot be
  * chained together.  If there's more data than the mbuf
- * could hold, an external malloced buffer is pointed to
+ * could hold, an external g_malloced buffer is pointed to
  * by m_ext (and the data pointers) and M_EXT is set in
  * the flags
  */
@@ -41,26 +41,26 @@ void m_cleanup(Slirp *slirp)
 while ((struct quehead *) m != >m_usedlist) {
 next = m->m_next;
 if (m->m_flags & M_EXT) {
-free(m->m_ext);
+g_free(m->m_ext);
 }
-free(m);
+g_free(m);
 m = next;
 }
 m = (struct mbuf *) slirp->m_freelist.qh_link;
 while ((struct quehead *) m != >m_freelist) {
 next = m->m_next;
-free(m);
+g_free(m);
 m = next;
 }
 }
 
 /*
  * Get an mbuf from the free list, if there are none
- * malloc one
+ * allocate one
  *
  * Because fragmentation can occur if we alloc new mbufs and
  * free old mbufs, we mark all mbufs above mbuf_thresh as M_DOFREE,
- * which tells m_free to actually free() it
+ * which tells m_free to actually g_free() it
  */
 struct mbuf *
 m_get(Slirp *slirp)
@@ -71,8 +71,7 @@ m_get(Slirp *slirp)
DEBUG_CALL("m_get");
 
if (slirp->m_freelist.qh_link == >m_freelist) {
-   m = (struct mbuf *)malloc(SLIRP_MSIZE);
-   if (m == NULL) goto end_error;
+m = g_malloc(SLIRP_MSIZE);
slirp->mbuf_alloced++;
if (slirp->mbuf_alloced > MBUF_THRESH)
flags = M_DOFREE;
@@ -94,7 +93,6 @@ m_get(Slirp *slirp)
 m->m_prevpkt = NULL;
 m->resolution_requested = false;
 m->expiration_date = (uint64_t)-1;
-end_error:
DEBUG_ARG("m = %p", m);
return m;
 }
@@ -112,15 +110,15 @@ m_free(struct mbuf *m)
   remque(m);
 
/* If it's M_EXT, free() it */
-   if (m->m_flags & M_EXT)
-  free(m->m_ext);
-
+if (m->m_flags & M_EXT) {
+g_free(m->m_ext);
+}
/*
 * Either free() it or put it on the free list
 */
if (m->m_flags & M_DOFREE) {
m->slirp->mbuf_alloced--;
-   free(m);
+g_free(m);
} else if ((m->m_flags & M_FREELIST) == 0) {
insque(m,>slirp->m_freelist);
m->m_flags = M_FREELIST; /* Clobber other flags */
@@ -130,7 +128,7 @@ m_free(struct mbuf *m)
 
 /*
  * Copy data from one mbuf to the end of
- * the other.. if result is too big for one mbuf, malloc()
+ * the other.. if result is too big for one mbuf, allocate
  * an M_EXT data segment
  */
 void
@@ -160,12 +158,12 @@ m_inc(struct mbuf *m, int size)
 
 if (m->m_flags & M_EXT) {
  datasize = m->m_data - m->m_ext;
- m->m_ext = (char *)realloc(m->m_ext,size);
+  m->m_ext = g_realloc(m->m_ext, size);
  m->m_data = m->m_ext + datasize;
 } else {
  char *dat;
  datasize = m->m_data - m->m_dat;
- dat = (char *)malloc(size);
+  dat = g_malloc(size);
  memcpy(dat, m->m_dat, m->m_size);
 
  m->m_ext = dat;
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] slirp: tcp_listen(): Don't try to close() an fd we never opened

[Peter Maydell]

Coverity points out (CID 1005725) that an error-exit path in tcp_listen()
will try to close(s) even if the reason it got there was that the
qemu_socket() failed and s was never opened.  Not only that, this isn't even
the right function to use, because we need closesocket() to do the right
thing on Windows.  Change to using the right function and only calling it if
needed.

Signed-off-by: Peter Maydell 
---
 slirp/socket.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/slirp/socket.c b/slirp/socket.c
index 6c18971..8692772 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -713,7 +713,9 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, 
uint32_t laddr,
(listen(s,1) < 0)) {
int tmperrno = errno; /* Don't clobber the real reason we 
failed */
 
-   close(s);
+if (s >= 0) {
+closesocket(s);
+}
sofree(so);
/* Restore the real errno */
 #ifdef _WIN32
-- 
2.1.4

[Qemu-devel] [PATCH 0/3] slirp: fix 3 easy coverity warnings

[Peter Maydell]

This patchset fixes three easy-to-fix coverity warnings in the slirp
code (there are another 5 or so which are not quite so simple).

As usual, the preexisting tab-indent style for a lot of the slirp
code is well out of line with the QEMU/checkpatch preferences.
I opted to generally use QEMU style for the new lines but this
does look a bit of a mess in some ways; happy to adjust per
slirp maintainer preferences.

thanks
-- PMM

Peter Maydell (3):
  slirp: Check qemu_socket() return value in udp_listen()
  slirp: Convert mbufs to use g_malloc() and g_free()
  slirp: tcp_listen(): Don't try to close() an fd we never opened

 slirp/mbuf.c   | 30 ++
 slirp/socket.c |  4 +++-
 slirp/udp.c|  4 
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] slirp: Check qemu_socket() return value in udp_listen()

[Peter Maydell]

Check the return value from qemu_socket() rather than trying to
pass it to bind() as an fd argument even if it's negative.
This wouldn't have caused any negative consequences, because
it won't be a valid fd number and the bind call will fail;
but Coverity complains (CID 1005723).

Signed-off-by: Peter Maydell 
---
 slirp/udp.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/slirp/udp.c b/slirp/udp.c
index 93d7224..227d779 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -335,6 +335,10 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, 
uint32_t laddr,
return NULL;
}
so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
+if (so->s < 0) {
+sofree(so);
+return NULL;
+}
so->so_expire = curtime + SO_EXPIRE;
insque(so, >udb);
 
-- 
2.1.4

[Qemu-devel] [PATCH] linux-user: Use correct types in load_symbols()

[Peter Maydell]

Coverity doesn't like the code in load_symbols() which assumes
it can use 'int' for a variable that might hold an offset into
the guest ELF file, because in a 64-bit guest that could
overflow. Guest binaries with 2GB sections aren't very likely
and this isn't a security issue because we fully trust the
guest linux-user binary anyway, but we might as well use the
right types, which will placate Coverity. Use uint64_t to
hold section sizes, and bail out if the symbol table is too
large rather than just overflowing an int.

(Coverity issue CID1005776)

Signed-off-by: Peter Maydell 
---
 linux-user/elfload.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index c66cbbe..f4c7b0c 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2263,6 +2263,7 @@ static int symcmp(const void *s0, const void *s1)
 static void load_symbols(struct elfhdr *hdr, int fd, abi_ulong load_bias)
 {
 int i, shnum, nsyms, sym_idx = 0, str_idx = 0;
+uint64_t segsz;
 struct elf_shdr *shdr;
 char *strings = NULL;
 struct syminfo *s = NULL;
@@ -2294,19 +2295,26 @@ static void load_symbols(struct elfhdr *hdr, int fd, 
abi_ulong load_bias)
 goto give_up;
 }
 
-i = shdr[str_idx].sh_size;
-s->disas_strtab = strings = g_try_malloc(i);
-if (!strings || pread(fd, strings, i, shdr[str_idx].sh_offset) != i) {
+segsz = shdr[str_idx].sh_size;
+s->disas_strtab = strings = g_try_malloc(segsz);
+if (!strings ||
+pread(fd, strings, segsz, shdr[str_idx].sh_offset) != segsz) {
 goto give_up;
 }
 
-i = shdr[sym_idx].sh_size;
-syms = g_try_malloc(i);
-if (!syms || pread(fd, syms, i, shdr[sym_idx].sh_offset) != i) {
+segsz = shdr[sym_idx].sh_size;
+syms = g_try_malloc(segsz);
+if (!syms || pread(fd, syms, segsz, shdr[sym_idx].sh_offset) != segsz) {
 goto give_up;
 }
 
-nsyms = i / sizeof(struct elf_sym);
+if (segsz / sizeof(struct elf_sym) > INT_MAX) {
+/* Implausibly large symbol table: give up rather than ploughing
+ * on with the number of symbols calculation overflowing
+ */
+goto give_up;
+}
+nsyms = segsz / sizeof(struct elf_sym);
 for (i = 0; i < nsyms; ) {
 bswap_sym(syms + i);
 /* Throw away entries which we do not need.  */
-- 
2.1.4

Re: [Qemu-devel] [PATCH] Makefile: Make "install" depend on "trace-events-all"

[Lluís Vilanova]

Fam Zheng writes:

> We install this file to data dir but since 0ab8ed18 it's no longer
> required by any objects during "make". List it explicitly as a depended
> target of install and fix the broken "make install" command.

I'm probably wrong, but I remember someone worked on making traces
self-descriptive, so that simpletrace would no longer need access to the
generated trace-events-all file.

If the file is really never used, then all stray rules to generate it should be
removed, as well as its installation.

Cheers,
  Lluis

Re: [Qemu-devel] [PATCH v4 1/8] make: move top level dir to end of include search path

[Alberto Garcia]

On Wed, Jan 25, 2017 at 04:14:10PM +, Daniel P. Berrange wrote:

> One final complication is that the absolute '-I$(BUILD_DIR)/$(@D)'
> will sometimes end up pointing to a non-existant directory if
> that sub-dir does not have any target-independant files to be
> built. Rather than try to dynamically filter this, a simple
> 'mkdir' ensures $(BUILD_DIR)/$(@D) is guaranteed to exist at
> all times.
> 
> @@ -359,6 +374,7 @@ define unnest-vars
>  $(eval $(o:%.mo=%$(DSOSUF)): module-common.o $($o-objs)),
>  $(error $o added in $v but $o-objs is not set)))
>  $(shell mkdir -p ./ $(sort $(dir $($v
> +$(shell cd $(BUILD_DIR) && mkdir -p ./ $(sort $(dir $($v
>  # Include all the .d files
>  $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v
>  $(eval $v := $(filter-out %/,$($v

After this change building QEMU leaves a lot of empty directories in
the parent directory:

$ mkdir empty_dir
$ cd empty_dir
$ git clone https://github.com/qemu/qemu
$ cd qemu
$ ./configure ...
$ ls ..
qemu
$ make
$ ls ..
audio chardev  fsdev  linux-user  net   qom target
backends  crypto   hw migration   qapi  replay  ui
block disasio nbd qemu  slirp   util

Berto

Re: [Qemu-devel] [PATCH v3] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Paolo Bonzini]

On 04/02/2017 04:21, Ashijeet Acharya wrote:
> Commit a3a3d8c7 introduced a segfault bug while checking for
> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
> devices which do no set their 'dc->vmsd' yet while initialization.
> Place a 'dc->vmsd' check prior to it so that we do not segfault for
> such devices.
> 
> NOTE: This doesn't compromise the functioning of --only-migratable
> option as all the unmigratable devices do set their 'dc->vmsd'.
> 
> Also, move the only_migratable check inside device_set_realized() to avoid
> code duplication and fix the bug to display only_migratable error message
> only if the device constructuon is right.
> 
> Signed-off-by: Ashijeet Acharya 
> ---
> Changes is v3:
> - move only_migratable check inside device_set_realized() to avoid code 
> - duplication
> - I have dropped Juan's R-b tag for this one
> Changes in v2:
> - place dc->vmsd check in hw/usb/bus.c as well
> ---
>  hw/core/qdev.c | 10 ++
>  hw/usb/bus.c   | 19 ---
>  qdev-monitor.c |  9 -
>  3 files changed, 10 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5783442..4438cbf 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -37,6 +37,7 @@
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
>  #include "qapi-event.h"
> +#include "migration/migration.h"
>  
>  int qdev_hotplug = 0;
>  static bool qdev_hot_added = false;
> @@ -896,6 +897,15 @@ static void device_set_realized(Object *obj, bool value, 
> Error **errp)
>  }
>  
>  if (value && !dev->realized) {
> +if (only_migratable && dc->vmsd) {
> +if (dc->vmsd->unmigratable) {
> +error_setg(_err, "Device %s is not migratable, but "
> +   "--only-migratable was specified",
> +   object_get_typename(obj));
> +goto fail;
> +}
> +}
> +
>  if (!obj->parent) {
>  gchar *name = g_strdup_printf("device[%d]", unattached_count++);
>  
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 1dcc35c..25913ad 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -8,7 +8,6 @@
>  #include "monitor/monitor.h"
>  #include "trace.h"
>  #include "qemu/cutils.h"
> -#include "migration/migration.h"
>  
>  static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
>  
> @@ -687,8 +686,6 @@ USBDevice *usbdevice_create(const char *cmdline)
>  const char *params;
>  int len;
>  USBDevice *dev;
> -ObjectClass *klass;
> -DeviceClass *dc;
>  
>  params = strchr(cmdline,':');
>  if (params) {
> @@ -723,22 +720,6 @@ USBDevice *usbdevice_create(const char *cmdline)
>  return NULL;
>  }
>  
> -klass = object_class_by_name(f->name);
> -if (klass == NULL) {
> -error_report("Device '%s' not found", f->name);
> -return NULL;
> -}
> -
> -dc = DEVICE_CLASS(klass);
> -
> -if (only_migratable) {
> -if (dc->vmsd->unmigratable) {
> -error_report("Device %s is not migratable, but --only-migratable 
> "
> - "was specified", f->name);
> -return NULL;
> -}
> -}
> -
>  if (f->usbdevice_init) {
>  dev = f->usbdevice_init(bus, params);
>  } else {
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 549f45f..5f2fcdf 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -29,7 +29,6 @@
>  #include "qemu/error-report.h"
>  #include "qemu/help_option.h"
>  #include "sysemu/block-backend.h"
> -#include "migration/migration.h"
>  
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -579,14 +578,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
> **errp)
>  return NULL;
>  }
>  
> -if (only_migratable) {
> -if (dc->vmsd->unmigratable) {
> -error_setg(errp, "Device %s is not migratable, but "
> -   "--only-migratable was specified", driver);
> -return NULL;
> -}
> -}
> -
>  /* find bus */
>  path = qemu_opt_get(opts, "bus");
>  if (path != NULL) {
> 

Reviewed-bv: Paolo Bonzini 

[Qemu-devel] [Bug 1653384] Re: Assertion failed with USB pass through with XHCI controller

[Fabian Lesniak]

** Changed in: qemu
   Status: New => Fix Committed

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

Title:
  Assertion failed with USB pass through with XHCI controller

Status in QEMU:
  Fix Committed

Bug description:
  Starting qemu 2.8.0 with XHCI controller and host device passed
  through results in an assertion failure:

  qemu-system-x86_64: hw/usb/core.c:623: usb_packet_cleanup: Assertion
  `!usb_packet_is_inflight(p)' failed.

  Can be reproduced with the following command (passing through a Lenovo
  keyboard):

  qemu-system-x86_64 -usb  -device nec-usb-xhci,id=usb -device usb-
  host,vendorid=0x04b3,productid=0x3025,id=hostdev0,bus=usb.0,port=1

  If nec-usb-xhci is changed to usb-ehci, qemu tries to boot without
  assertion failures.

  
  Can be reproduced with the latest master (commit dbe2b65) and v2.8.0.

  Bisected the issue to following commit:
  first bad commit: [94b037f2a451b3dc855f9f2c346e5049a361bd55] xhci: use linked 
list for transfers

  
  Backtrace from commit dbe2b65:

  #0  0x7f2eb4657227 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:55
  resultvar = 0
  pid = 3453
  selftid = 3453
  #1  0x7f2eb465867a in __GI_abort () at abort.c:89
  save_stage = 2
  act = {__sigaction_handler = {sa_handler = 0x4, sa_sigaction = 0x4}, 
sa_mask = {__val = {140734740550528, 93876690035339, 
140734740550624, 48833659808, 0, 0, 0, 21474836480, 
140734740550792, 139838573009553, 140734740550560, 139838573043008, 
139838573024160, 9387665872, 139838702616576, 
139838573024160}}, sa_flags = 1528954938, 
sa_restorer = 0x55615b2202c0 <__PRETTY_FUNCTION__.38612>}
  sigs = {__val = {32, 0 }}
  #2  0x7f2eb46502cd in __assert_fail_base (fmt=0x7f2eb47893a0 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", 
  assertion=assertion@entry=0x55615b22003a "!usb_packet_is_inflight(p)", 
file=file@entry=0x55615b21fdf0 "hw/usb/core.c", line=line@entry=619, 
  function=function@entry=0x55615b2202c0 <__PRETTY_FUNCTION__.38612> 
"usb_packet_cleanup") at assert.c:92
  str = 0x55615cfdf510 ""
  total = 4096
  #3  0x7f2eb4650382 in __GI___assert_fail (assertion=0x55615b22003a 
"!usb_packet_is_inflight(p)", file=0x55615b21fdf0 "hw/usb/core.c", 
  line=619, function=0x55615b2202c0 <__PRETTY_FUNCTION__.38612> 
"usb_packet_cleanup") at assert.c:101
  No locals.
  #4  0x55615afc385e in usb_packet_cleanup ()
  No symbol table info available.
  #5  0x55615afda555 in xhci_ep_free_xfer ()
  No symbol table info available.
  #6  0x55615afdc156 in xhci_kick_epctx ()
  No symbol table info available.
  #7  0x55615afda099 in xhci_ep_kick_timer ()
  No symbol table info available.
  #8  0x55615b08ceee in timerlist_run_timers ()
  No symbol table info available.
  #9  0x55615b08cf36 in qemu_clock_run_timers ()
  No symbol table info available.
  #10 0x55615b08d2df in qemu_clock_run_all_timers ()
  No symbol table info available.
  #11 0x55615b08be40 in main_loop_wait ()
  No symbol table info available.
  #12 0x55615ae3870f in main_loop ()
  No symbol table info available.
  #13 0x55615ae4027b in main ()

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

[Qemu-devel] [Bug 1653384] Re: Assertion failed with USB pass through with XHCI controller

[Fabian Lesniak]

These patches solve my problems. All three devices I tested using xhci
work correctly now.

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

Title:
  Assertion failed with USB pass through with XHCI controller

Status in QEMU:
  New

Bug description:
  Starting qemu 2.8.0 with XHCI controller and host device passed
  through results in an assertion failure:

  qemu-system-x86_64: hw/usb/core.c:623: usb_packet_cleanup: Assertion
  `!usb_packet_is_inflight(p)' failed.

  Can be reproduced with the following command (passing through a Lenovo
  keyboard):

  qemu-system-x86_64 -usb  -device nec-usb-xhci,id=usb -device usb-
  host,vendorid=0x04b3,productid=0x3025,id=hostdev0,bus=usb.0,port=1

  If nec-usb-xhci is changed to usb-ehci, qemu tries to boot without
  assertion failures.

  
  Can be reproduced with the latest master (commit dbe2b65) and v2.8.0.

  Bisected the issue to following commit:
  first bad commit: [94b037f2a451b3dc855f9f2c346e5049a361bd55] xhci: use linked 
list for transfers

  
  Backtrace from commit dbe2b65:

  #0  0x7f2eb4657227 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:55
  resultvar = 0
  pid = 3453
  selftid = 3453
  #1  0x7f2eb465867a in __GI_abort () at abort.c:89
  save_stage = 2
  act = {__sigaction_handler = {sa_handler = 0x4, sa_sigaction = 0x4}, 
sa_mask = {__val = {140734740550528, 93876690035339, 
140734740550624, 48833659808, 0, 0, 0, 21474836480, 
140734740550792, 139838573009553, 140734740550560, 139838573043008, 
139838573024160, 9387665872, 139838702616576, 
139838573024160}}, sa_flags = 1528954938, 
sa_restorer = 0x55615b2202c0 <__PRETTY_FUNCTION__.38612>}
  sigs = {__val = {32, 0 }}
  #2  0x7f2eb46502cd in __assert_fail_base (fmt=0x7f2eb47893a0 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", 
  assertion=assertion@entry=0x55615b22003a "!usb_packet_is_inflight(p)", 
file=file@entry=0x55615b21fdf0 "hw/usb/core.c", line=line@entry=619, 
  function=function@entry=0x55615b2202c0 <__PRETTY_FUNCTION__.38612> 
"usb_packet_cleanup") at assert.c:92
  str = 0x55615cfdf510 ""
  total = 4096
  #3  0x7f2eb4650382 in __GI___assert_fail (assertion=0x55615b22003a 
"!usb_packet_is_inflight(p)", file=0x55615b21fdf0 "hw/usb/core.c", 
  line=619, function=0x55615b2202c0 <__PRETTY_FUNCTION__.38612> 
"usb_packet_cleanup") at assert.c:101
  No locals.
  #4  0x55615afc385e in usb_packet_cleanup ()
  No symbol table info available.
  #5  0x55615afda555 in xhci_ep_free_xfer ()
  No symbol table info available.
  #6  0x55615afdc156 in xhci_kick_epctx ()
  No symbol table info available.
  #7  0x55615afda099 in xhci_ep_kick_timer ()
  No symbol table info available.
  #8  0x55615b08ceee in timerlist_run_timers ()
  No symbol table info available.
  #9  0x55615b08cf36 in qemu_clock_run_timers ()
  No symbol table info available.
  #10 0x55615b08d2df in qemu_clock_run_all_timers ()
  No symbol table info available.
  #11 0x55615b08be40 in main_loop_wait ()
  No symbol table info available.
  #12 0x55615ae3870f in main_loop ()
  No symbol table info available.
  #13 0x55615ae4027b in main ()

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

[Qemu-devel] [PATCH] ps2: fix mouse mappings for right/middle button

[Fabian Lesniak]

Commit 8b0caab0 ("ps2: add support for mice with extra/side buttons")
accidentally swapped right and middle mouse buttons. This commit corrects
the mapping as expected by the ps2 controller.

Signed-off-by: Fabian Lesniak 
---
 include/hw/input/ps2.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/input/ps2.h b/include/hw/input/ps2.h
index 0fec91cdb0..7f0a80af9d 100644
--- a/include/hw/input/ps2.h
+++ b/include/hw/input/ps2.h
@@ -26,8 +26,8 @@
 #define HW_PS2_H
 
 #define PS2_MOUSE_BUTTON_LEFT   0x01
-#define PS2_MOUSE_BUTTON_MIDDLE 0x02
-#define PS2_MOUSE_BUTTON_RIGHT  0x04
+#define PS2_MOUSE_BUTTON_RIGHT  0x02
+#define PS2_MOUSE_BUTTON_MIDDLE 0x04
 #define PS2_MOUSE_BUTTON_SIDE   0x08
 #define PS2_MOUSE_BUTTON_EXTRA  0x10
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH] CODING_STYLE: Mention preferred comment form

[Fam Zheng]

On Fri, 02/03 17:58, Peter Maydell wrote:
> Our defacto coding style strongly prefers /* */ style comments
> over the single-line // style, and checkpatch enforces this,
> but we don't actually document this. Mention it in CODING_STYLE.
> 
> Suggested-by: Thomas Huth 
> Signed-off-by: Peter Maydell 
> ---
>  CODING_STYLE | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index f53180b..2fa0c0b 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -116,3 +116,10 @@ if (a == 1) {
>  Rationale: Yoda conditions (as in 'if (1 == a)') are awkward to read.
>  Besides, good compilers already warn users when '==' is mis-typed as '=',
>  even when the constant is on the right.
> +
> +7. Comment style
> +
> +We use traditional C-style /* */ comments and avoid // comments.
> +
> +Rationale: The // form is valid in C99, so this is purely a matter of
> +consistency of style. The checkpatch script will warn you about this.

Reviewed-by: Fam Zheng 

[Qemu-devel] [PATCH] Makefile: Make "install" depend on "trace-events-all"

[Fam Zheng]

We install this file to data dir but since 0ab8ed18 it's no longer
required by any objects during "make". List it explicitly as a depended
target of install and fix the broken "make install" command.

Signed-off-by: Fam Zheng 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4b72a4c..b993741 100644
--- a/Makefile
+++ b/Makefile
@@ -589,7 +589,7 @@ endif
 endif
 
 
-install: all $(if $(BUILD_DOCS),install-doc) \
+install: all $(if $(BUILD_DOCS),install-doc) $(BUILD_DIR)/trace-events-all \
 install-datadir install-localstatedir
 ifneq ($(TOOLS),)
$(call install-prog,$(subst 
qemu-ga,qemu-ga$(EXESUF),$(TOOLS)),$(DESTDIR)$(bindir))
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 2/2] target/arm: A32, T32: Create Instruction Syndromes for Data Aborts

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 05:48:55PM +, Peter Maydell wrote:
> Add support for generating the ISS (Instruction Specific Syndrome)
> for Data Abort exceptions taken from AArch32. These syndromes are
> used by hypervisors for example to trap and emulate memory accesses.
> 
> This is the equivalent for AArch32 guests of the work done for AArch64
> guests in commit aaa1f954d4cab243.

Hi Peter,


> 
> Signed-off-by: Peter Maydell 
> ---
>  target/arm/translate.c | 198 
> +
>  1 file changed, 149 insertions(+), 49 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 175b4c1..fc0ddcd 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -102,6 +102,63 @@ void arm_translate_init(void)
>  a64_translate_init();
>  }
>  
> +static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
> +{
> +/* We don't need to save all of the syndrome so we mask and shift
> + * out uneeded bits to help the sleb128 encoder do a better job.
> + */
> +syn &= ARM_INSN_START_WORD2_MASK;
> +syn >>= ARM_INSN_START_WORD2_SHIFT;
> +
> +/* We check and clear insn_start_idx to catch multiple updates.  */
> +assert(s->insn_start_idx != 0);
> +tcg_set_insn_param(s->insn_start_idx, 2, syn);
> +s->insn_start_idx = 0;
> +}

Could we move this into translate.h and share it with translate-a64.c?

Other than that this looks good to me!
Reviewed-by: Edgar E. Iglesias 

Thanks,
Edgar


> +
> +/* Flags for the disas_set_da_iss info argument:
> + * lower bits hold the Rt register number, higher bits are flags.
> + */
> +typedef enum ISSInfo {
> +ISSNone = 0,
> +ISSRegMask = 0x1f,
> +ISSInvalid = (1 << 5),
> +ISSIsAcqRel = (1 << 6),
> +ISSIsWrite = (1 << 7),
> +ISSIs16Bit = (1 << 8),
> +} ISSInfo;
> +
> +/* Save the syndrome information for a Data Abort */
> +static void disas_set_da_iss(DisasContext *s, TCGMemOp memop, ISSInfo 
> issinfo)
> +{
> +uint32_t syn;
> +int sas = memop & MO_SIZE;
> +bool sse = memop & MO_SIGN;
> +bool is_acqrel = issinfo & ISSIsAcqRel;
> +bool is_write = issinfo & ISSIsWrite;
> +bool is_16bit = issinfo & ISSIs16Bit;
> +int srt = issinfo & ISSRegMask;
> +
> +if (issinfo & ISSInvalid) {
> +/* Some callsites want to conditionally provide ISS info,
> + * eg "only if this was not a writeback"
> + */
> +return;
> +}
> +
> +if (srt == 15) {
> +/* For AArch32, insns where the src/dest is R15 never generate
> + * ISS information. Catching that here saves checking at all
> + * the call sites.
> + */
> +return;
> +}
> +
> +syn = syn_data_abort_with_iss(0, sas, sse, srt, 0, is_acqrel,
> +  0, 0, 0, is_write, 0, is_16bit);
> +disas_set_insn_syndrome(s, syn);
> +}
> +
>  static inline ARMMMUIdx get_a32_user_mem_index(DisasContext *s)
>  {
>  /* Return the mmu_idx to use for A32/T32 "unprivileged load/store"
> @@ -933,6 +990,14 @@ static inline void gen_aa32_ld##SUFF(DisasContext *s, 
> TCGv_i32 val,  \
>   TCGv_i32 a32, int index)\
>  {\
>  gen_aa32_ld_i32(s, val, a32, index, OPC | s->be_data);   \
> +}\
> +static inline void gen_aa32_ld##SUFF##_iss(DisasContext *s,  \
> +   TCGv_i32 val, \
> +   TCGv_i32 a32, int index,  \
> +   ISSInfo issinfo)  \
> +{\
> +gen_aa32_ld_i32(s, val, a32, index, OPC | s->be_data);   \
> +disas_set_da_iss(s, OPC, issinfo);   \
>  }
>  
>  #define DO_GEN_ST(SUFF, OPC) \
> @@ -940,6 +1005,14 @@ static inline void gen_aa32_st##SUFF(DisasContext *s, 
> TCGv_i32 val,  \
>   TCGv_i32 a32, int index)\
>  {\
>  gen_aa32_st_i32(s, val, a32, index, OPC | s->be_data);   \
> +}\
> +static inline void gen_aa32_st##SUFF##_iss(DisasContext *s,  \
> +   TCGv_i32 val, \
> +   TCGv_i32 a32, int index,  \
> +   ISSInfo issinfo)  \
> +{\
> +gen_aa32_st_i32(s, val, a32, 

Re: [Qemu-devel] [PATCH v2 1/2] target/arm: Abstract out pbit/wbit tests in ARM ldr/str decode

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 05:48:54PM +, Peter Maydell wrote:
> In the ARM ldr/str decode path, rather than directly testing
> "insn & (1 << 21)" and "insn & (1 << 24)", abstract these
> bits out into wbit and pbit local flags. (We will want to
> do more tests against them to determine whether we need to
> provide syndrome information.)
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Edgar E. Iglesias 


> ---
>  target/arm/translate.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 493c627..175b4c1 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8782,6 +8782,8 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>  } else {
>  int address_offset;
>  bool load = insn & (1 << 20);
> +bool wbit = insn & (1 << 21);
> +bool pbit = insn & (1 << 24);
>  bool doubleword = false;
>  /* Misc load/store */
>  rn = (insn >> 16) & 0xf;
> @@ -8799,8 +8801,9 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>  }
>  
>  addr = load_reg(s, rn);
> -if (insn & (1 << 24))
> +if (pbit) {
>  gen_add_datah_offset(s, insn, 0, addr);
> +}
>  address_offset = 0;
>  
>  if (doubleword) {
> @@ -8849,10 +8852,10 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
> ensure correct behavior with overlapping index registers.
> ldrd with base writeback is undefined if the
> destination and index registers overlap.  */
> -if (!(insn & (1 << 24))) {
> +if (!pbit) {
>  gen_add_datah_offset(s, insn, address_offset, addr);
>  store_reg(s, rn, addr);
> -} else if (insn & (1 << 21)) {
> +} else if (wbit) {
>  if (address_offset)
>  tcg_gen_addi_i32(addr, addr, address_offset);
>  store_reg(s, rn, addr);
> -- 
> 2.7.4
> 

Re: [Qemu-devel] Non-flat command line option argument syntax

[Fam Zheng]

On Sat, 02/04 14:35, Markus Armbruster wrote:
> Fam Zheng  writes:
> 
> > On Thu, 02/02 20:42, Markus Armbruster wrote:
> >> === Comparison ===
> >> 
> >> In my opinion, dotted keys are weird and ugly, but at least they don't
> >> add to the quoting mess.  Structured values look better, except when
> >> they do add to the quoting mess.
> >> 
> >> I'm having a hard time deciding which one I like less :)
> >> 
> >> Opinions?  Other ideas?
> >
> > Here's my poor attempt:
> >
> > The dotted syntax, as the simpler of two, can cover everyday use very well. 
> >  If
> > we introduce an "@reference" extension to it which can help the 
> > expresiveness,
> > we can have a hybrid solution. It's not the cleanest interface and syntax, 
> > but
> > escaping, nesting and quoting can all be divide-and-conqured in their 
> > optimal way.
> > What I'm imagining is something like:
> >
> > -json "id=children0,text=[
> > { 'driver': 'null-co://' },
> > { 'driver': 'null-co://' },
> > { 'driver': 'null-co://' }
> > ]" \
> > -dot \
> >   
> > id=quorum0,driver=quorum,read-pattern=fifo,vote-threshold=1,children=@children0
> >  \
> > -drive if=virtio,id=primary-disk0,driver=qcow2,file=@quorum0
> >
> > IOW "-json" and "-dot" define options that is intended to be referenced from
> > other dotted keys (quorum0 uses children0, and in turn primary-disk0 uses
> > quorum0).
> >
> > Note: "-dot" here could be replaced with a -blockdev in this specific case 
> > but
> > I'm demostrating it just in case it is useful generically.
> >
> > Fam
> 
> KEY=@REF for references creates the same issue as KEY=[VALUE,...] for
> arrays: you need to know the type of KEY to decide whether @REF is a
> reference or a string, unless we outlaw strings starting with '@'.
> 
> Not a show-stopper, but it does preclude a design where a simple parser
> feeds into a type-aware visitor, which is how the JSON -> QObject ->
> QAPI pipeline works.
> 
> If you get creative in the VALUE part, you complicate the parser and/or
> need to add quoting.
> 
> If you get creative in the KEY part, you need to restrict valid names.

How about KEY@=REF?

Fam

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Frederic Konrad]

On 02/04/2017 02:17 PM, Peter Maydell wrote:
> On 4 February 2017 at 12:52, Frederic Konrad  
> wrote:
>> Is that the case that we might get a Bad RAM address error or some such
>> if we are not on a page boundary (or too small as you say)?
>> I guess this is a limitation. Mapping on a page boundary shouldn't be
>> too much restrictive.
> 
> Yeah. I really ought to look more closely at what the flow of
> execution is here, because I think how it works right now
> is a bit weird and works as much by luck as by judgement
> (we can longjump out of the middle of translating code
> right back to the cpu-exec.c loop, and in some cases
> I think what happens is that we try to translate code,
> and as part of the "load didn't work" code path we
> nestedly try to translate the same thing again which
> of course fails again, only the second time around we
> realize and longjump out.
> 
> (At the moment for linux-user mode this is causing us to
> assert about taking the tb lock twice, because we hold
> the tb lock during translation and then try to grab it
> again to do the cpu_restore_state() in the signal handler.)
> 

Yes it seems there are some scary things happening there.

Fred

> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Frederic Konrad]

On 02/04/2017 01:41 PM, Paolo Bonzini wrote:
> 
...
>>
>> Doesn't hotplug use dynamic MemoryRegion? In which case we better
>> make that work with MTTCG. I wonder if we can't simply handle that
>> with a safe_work for this case?
> 
> Hot-unplug works because the backing memory is only freed when the
> device gets instance_finalize, so at that point the region cannot have
> any references.
> 
> What can go wrong is the following (from the docs):
> 
> - the memory region's owner had a reference taken via memory_region_ref
>   (for example by address_space_map)
> 
> - the region is unparented, and has no owner anymore
> 
> - when address_space_unmap is called, the reference to the memory
>   region's owner is leaked.

true.

> 
> In your case you have phys_section_add/phys_section_destroy instead of
> address_space_map/unmap, but the problem is the same.
> 
> I am a bit unsure about using the same lqspi_buf for caching different
> addresses, and about using the same buffer for MMIO execution and read.
> What if you allocate a different buffer every time
> lqspi_request_mmio_ptr is called (adding a char* argument to
> lqspi_load_cache) and free the old one from the safe_work item, after a
> global TLB flush?  Then you don't need the Notifiers which were the
> complicated and handwavy part of my proposal.

Actually this was just because it was the way xilinx_qspi worked.
It load 1K of SPI data and fill the buffer.

In some other case we don't want to free the pointer at all, just make
it not accessible for execution / read.
(BTW I'll add the read-only property to this).

What about a simple Object eg: mmio-execution-interface which has a
MemoryRegion? Instead of creating the MemoryRegion in request_pointer,
We create a mmio-execution-interface object which create and map the
region on the subregion.
Then in invalidate: we unplug the mmio-execution-interface, unref it and
it is freed all by magic when the map doesn't need it.

This avoid the reference issue and probably the bug with MTTCG.

Fred

> 
> Paolo
> 
>>
>> BTW the tests I have seems to pass without issues.
>>
>>>
>>> An alternative design could be:
>>>
>>> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
>>> a pointer, so that the device can map a subset of the device (e.g. a
>>> single page)
>>
>> I'm not aware of this MemoryRegionCache yet, it seems pretty new.
>> I'll take a look.
>>
>> Thanks,
>> Fred
>>
>>>
>>> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
>>> a Notifier
>>>
>>> - the device adds the Notifier to a NotifierList.  Before invalidating,
>>> it invokes the Notifier and empties the NotifierList.
>>>
>>> - for the TLB case, the Notifier calls tlb_flush_page.
>>>
>>> I like the general idea though!
>>>
>>> Paolo
>>>
 +}
 +}
>>
>>
>>

Re: [Qemu-devel] Non-flat command line option argument syntax

[Markus Armbruster]

Fam Zheng  writes:

> On Thu, 02/02 20:42, Markus Armbruster wrote:
>> === Comparison ===
>> 
>> In my opinion, dotted keys are weird and ugly, but at least they don't
>> add to the quoting mess.  Structured values look better, except when
>> they do add to the quoting mess.
>> 
>> I'm having a hard time deciding which one I like less :)
>> 
>> Opinions?  Other ideas?
>
> Here's my poor attempt:
>
> The dotted syntax, as the simpler of two, can cover everyday use very well.  
> If
> we introduce an "@reference" extension to it which can help the expresiveness,
> we can have a hybrid solution. It's not the cleanest interface and syntax, but
> escaping, nesting and quoting can all be divide-and-conqured in their optimal 
> way.
> What I'm imagining is something like:
>
> -json "id=children0,text=[
> { 'driver': 'null-co://' },
> { 'driver': 'null-co://' },
> { 'driver': 'null-co://' }
> ]" \
> -dot \
>   
> id=quorum0,driver=quorum,read-pattern=fifo,vote-threshold=1,children=@children0
>  \
> -drive if=virtio,id=primary-disk0,driver=qcow2,file=@quorum0
>
> IOW "-json" and "-dot" define options that is intended to be referenced from
> other dotted keys (quorum0 uses children0, and in turn primary-disk0 uses
> quorum0).
>
> Note: "-dot" here could be replaced with a -blockdev in this specific case but
> I'm demostrating it just in case it is useful generically.
>
> Fam

KEY=@REF for references creates the same issue as KEY=[VALUE,...] for
arrays: you need to know the type of KEY to decide whether @REF is a
reference or a string, unless we outlaw strings starting with '@'.

Not a show-stopper, but it does preclude a design where a simple parser
feeds into a type-aware visitor, which is how the JSON -> QObject ->
QAPI pipeline works.

If you get creative in the VALUE part, you complicate the parser and/or
need to add quoting.

If you get creative in the KEY part, you need to restrict valid names.

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Peter Maydell]

On 4 February 2017 at 12:52, Frederic Konrad  wrote:
> Is that the case that we might get a Bad RAM address error or some such
> if we are not on a page boundary (or too small as you say)?
> I guess this is a limitation. Mapping on a page boundary shouldn't be
> too much restrictive.

Yeah. I really ought to look more closely at what the flow of
execution is here, because I think how it works right now
is a bit weird and works as much by luck as by judgement
(we can longjump out of the middle of translating code
right back to the cpu-exec.c loop, and in some cases
I think what happens is that we try to translate code,
and as part of the "load didn't work" code path we
nestedly try to translate the same thing again which
of course fails again, only the second time around we
realize and longjump out.

(At the moment for linux-user mode this is causing us to
assert about taking the tb lock twice, because we hold
the tb lock during translation and then try to grab it
again to do the cpu_restore_state() in the signal handler.)

thanks
-- PMM

Re: [Qemu-devel] Non-flat command line option argument syntax

[Fam Zheng]

On Sat, 02/04 04:44, Paolo Bonzini wrote:
> 
> 
> On 04/02/2017 04:21, Fam Zheng wrote:
> > -json "id=children0,text=[
> > { 'driver': 'null-co://' },
> > { 'driver': 'null-co://' },
> 
> You meant ,, at the end of this lines.  Which throws a wrench in your
> proposal somewhat. :(

Ahh right.. We could just specially ditch the ,, escape here and start over with
a faithful JSON parser added to which a mere id syntax (i.e. the "ID=" prefix):

-json "children0=[
 { 'driver': 'null-co://' },
 { 'driver': 'null-co://' },
 { 'driver': 'null-co://' }
 ]" \


> 
> Paolo
> 
> > { 'driver': 'null-co://' }
> > ]" \
> > -dot \
> >   
> > id=quorum0,driver=quorum,read-pattern=fifo,vote-threshold=1,children=@children0
> >  \
> > -drive if=virtio,id=primary-disk0,driver=qcow2,file=@quorum0
> 

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Frederic Konrad]

On 02/04/2017 01:33 PM, Peter Maydell wrote:
> On 3 February 2017 at 17:06,   wrote:
>> From: KONRAD Frederic 
>>
>> This patch-set allows to execute code from mmio areas.
>> The main goal of this is to be able to run code for example from an SPI 
>> device.
>>
>> The three first patch fixes the way get_page_addr_code fills the TLB.
>>
>> The fourth patch implements the mmio execution helpers: the device must
>> implement the request_ptr callback of the MemoryRegion and will be notified 
>> when
>> the guest wants to execute code from it.
>>
>> The fifth patch implements the execution from the SPI memories in the
>> xilinx_spips model.
> 
> I like the general idea, but there's an awkward issue:
> at the moment our translation system assumes that when we're
> translating code then if the first instruction in the TB
> can be read OK then we won't ever get a fault trying to
> read subsequent bytes up to the end of the page. If we
> move from "we only translate code out of whole pages of
> RAM" to "we might translate code out of devices that
> are in subpages" then this assumption gets broken.
> (The symptom would be that we would report the fault
> in the wrong place, for the PC at the start of the TB.)
> 
> thanks
> -- PMM
> 

Hi Peter,

I think I see your point.
Is that the case that we might get a Bad RAM address error or some such
if we are not on a page boundary (or too small as you say)?
I guess this is a limitation. Mapping on a page boundary shouldn't be
too much restrictive.

Thanks,
Fred

Re: [Qemu-devel] Non-flat command line option argument syntax

[Paolo Bonzini]

On 04/02/2017 04:21, Fam Zheng wrote:
> -json "id=children0,text=[
> { 'driver': 'null-co://' },
> { 'driver': 'null-co://' },

You meant ,, at the end of this lines.  Which throws a wrench in your
proposal somewhat. :(

Paolo

> { 'driver': 'null-co://' }
> ]" \
> -dot \
>   
> id=quorum0,driver=quorum,read-pattern=fifo,vote-threshold=1,children=@children0
>  \
> -drive if=virtio,id=primary-disk0,driver=qcow2,file=@quorum0

Re: [Qemu-devel] Non-flat command line option argument syntax

[Paolo Bonzini]

On 04/02/2017 03:52, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
>> On 04/02/2017 01:45, Markus Armbruster wrote:
>  -drive driver=qcow2,
> file.driver=gluster,
> .volume=testvol,
> .path=/path/a.qcow2,
> .debug=9,
> file.server.0.type=tcp,
>  .host=1.2.3.4,
>  .port=24007,
> file.server.1.type=unix,
>  .socket=/var/run/glusterd.socket
>
> Mind, I'm not at all sure this is a *good* idea.  I suspect it's more
> magic than it's worth.
 As someone who likes dot syntax very much, I don't like it. If you
 structure it like this, it's OK, but then you can just write the full
 prefix (which gets the point across just as well because I can quickly
 tell from a glance that it's the same prefix).

 OTOH, when joined into a single line it doesn't change much in terms of
 legibility, in my opinion.
>>>
>>> Thanks!
>>
>> Actually I think it does improve legibility.
>>
>> It doesn't improve writability though, as anecdotally proved by Markus's
>> own mistake.
>>
>> I am a fan of the dot syntax too.  It seems to be the most incremental
>> solution, and it's still as expressive as JSON.
> 
> Noted.
> 
>> _However_ we could also extend -readconfig to support JSON, i.e. instead of
>>
>>  [drive "abc"]
>>  file = "foo"
>>
>> it could support
>>
>>  { 'drive': { 'file: 'foo' }, 'id': 'abc' }
>>
>> In other words [ would introduce key-value QemuOpts with dot syntax,
>> while { would introduce JSON.
> 
> Yes, we should support config files in JSON syntax.  Not sure mixing INI
> and JSON syntax in the same file is a good idea, though.

It's probably not, so we could have "-readconfig foo,syntax=json"
instead (yes, that's QemuOpts syntax---so meta).

Paolo

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Paolo Bonzini]

On 03/02/2017 13:09, Frederic Konrad wrote:
> On 02/03/2017 06:26 PM, Paolo Bonzini wrote:
>>
>>
>> On 03/02/2017 09:06, fred.kon...@greensocs.com wrote:
>>> +host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, , 
>>> );
>>> +
>>> +if (!host || !size) {
>>> +memory_region_transaction_commit();
>>> +return false;
>>> +}
>>> +
>>> +sub = g_new(MemoryRegion, 1);
>>> +memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
>>> +memory_region_add_subregion(mr, offset, sub);
>>> +memory_region_transaction_commit();
>>> +return true;
>>> +}
>>> +
>>> +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
>>> +   unsigned size)
>>> +{
>>> +MemoryRegionSection section = memory_region_find(mr, offset, size);
>>> +
>>> +if (section.mr != mr) {
>>> +memory_region_del_subregion(mr, section.mr);
>>> +/* memory_region_find add a ref on section.mr */
>>> +memory_region_unref(section.mr);
>>> +object_unparent(OBJECT(section.mr));
>>
>> I think this would cause a use-after-free when using MTTCG.  In general,
>> creating and dropping MemoryRegions dynamically can cause bugs that are
>> nondeterministic and hard to fix without rewriting everything.
> 
> Hi Paolo,
> 
> Thanks for your comment.
> Yes, I read in the docs that dynamically dropping MemoryRegions is badly
> broken when we use NULL as an owner because the machine owns it.
> But it seems nothing said this is the case with an owner.
> 
> But I think I see your point here:
>   * memory_region_unref will unref the owner.
>   * object_unparent will unref the memory region (which should be 1).
>   => the region will be dropped immediately.
> 
> Doesn't hotplug use dynamic MemoryRegion? In which case we better
> make that work with MTTCG. I wonder if we can't simply handle that
> with a safe_work for this case?

Hot-unplug works because the backing memory is only freed when the
device gets instance_finalize, so at that point the region cannot have
any references.

What can go wrong is the following (from the docs):

- the memory region's owner had a reference taken via memory_region_ref
  (for example by address_space_map)

- the region is unparented, and has no owner anymore

- when address_space_unmap is called, the reference to the memory
  region's owner is leaked.

In your case you have phys_section_add/phys_section_destroy instead of
address_space_map/unmap, but the problem is the same.

I am a bit unsure about using the same lqspi_buf for caching different
addresses, and about using the same buffer for MMIO execution and read.
What if you allocate a different buffer every time
lqspi_request_mmio_ptr is called (adding a char* argument to
lqspi_load_cache) and free the old one from the safe_work item, after a
global TLB flush?  Then you don't need the Notifiers which were the
complicated and handwavy part of my proposal.

Paolo

> 
> BTW the tests I have seems to pass without issues.
> 
>>
>> An alternative design could be:
>>
>> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
>> a pointer, so that the device can map a subset of the device (e.g. a
>> single page)
> 
> I'm not aware of this MemoryRegionCache yet, it seems pretty new.
> I'll take a look.
> 
> Thanks,
> Fred
> 
>>
>> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
>> a Notifier
>>
>> - the device adds the Notifier to a NotifierList.  Before invalidating,
>> it invokes the Notifier and empties the NotifierList.
>>
>> - for the TLB case, the Notifier calls tlb_flush_page.
>>
>> I like the general idea though!
>>
>> Paolo
>>
>>> +}
>>> +}
> 
> 
> 

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Peter Maydell]

On 3 February 2017 at 17:06,   wrote:
> From: KONRAD Frederic 
>
> This patch-set allows to execute code from mmio areas.
> The main goal of this is to be able to run code for example from an SPI 
> device.
>
> The three first patch fixes the way get_page_addr_code fills the TLB.
>
> The fourth patch implements the mmio execution helpers: the device must
> implement the request_ptr callback of the MemoryRegion and will be notified 
> when
> the guest wants to execute code from it.
>
> The fifth patch implements the execution from the SPI memories in the
> xilinx_spips model.

I like the general idea, but there's an awkward issue:
at the moment our translation system assumes that when we're
translating code then if the first instruction in the TB
can be read OK then we won't ever get a fault trying to
read subsequent bytes up to the end of the page. If we
move from "we only translate code out of whole pages of
RAM" to "we might translate code out of devices that
are in subpages" then this assumption gets broken.
(The symptom would be that we would report the fault
in the wrong place, for the PC at the start of the TB.)

thanks
-- PMM

Re: [Qemu-devel] Non-flat command line option argument syntax

[Fam Zheng]

On Thu, 02/02 20:42, Markus Armbruster wrote:
> === Comparison ===
> 
> In my opinion, dotted keys are weird and ugly, but at least they don't
> add to the quoting mess.  Structured values look better, except when
> they do add to the quoting mess.
> 
> I'm having a hard time deciding which one I like less :)
> 
> Opinions?  Other ideas?

Here's my poor attempt:

The dotted syntax, as the simpler of two, can cover everyday use very well.  If
we introduce an "@reference" extension to it which can help the expresiveness,
we can have a hybrid solution. It's not the cleanest interface and syntax, but
escaping, nesting and quoting can all be divide-and-conqured in their optimal 
way.
What I'm imagining is something like:

-json "id=children0,text=[
{ 'driver': 'null-co://' },
{ 'driver': 'null-co://' },
{ 'driver': 'null-co://' }
]" \
-dot \
  
id=quorum0,driver=quorum,read-pattern=fifo,vote-threshold=1,children=@children0 
\
-drive if=virtio,id=primary-disk0,driver=qcow2,file=@quorum0

IOW "-json" and "-dot" define options that is intended to be referenced from
other dotted keys (quorum0 uses children0, and in turn primary-disk0 uses
quorum0).

Note: "-dot" here could be replaced with a -blockdev in this specific case but
I'm demostrating it just in case it is useful generically.

Fam

[Qemu-devel] [PATCH v3] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Ashijeet Acharya]

Commit a3a3d8c7 introduced a segfault bug while checking for
'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
devices which do no set their 'dc->vmsd' yet while initialization.
Place a 'dc->vmsd' check prior to it so that we do not segfault for
such devices.

NOTE: This doesn't compromise the functioning of --only-migratable
option as all the unmigratable devices do set their 'dc->vmsd'.

Also, move the only_migratable check inside device_set_realized() to avoid
code duplication and fix the bug to display only_migratable error message
only if the device constructuon is right.

Signed-off-by: Ashijeet Acharya 
---
Changes is v3:
- move only_migratable check inside device_set_realized() to avoid code 
- duplication
- I have dropped Juan's R-b tag for this one
Changes in v2:
- place dc->vmsd check in hw/usb/bus.c as well
---
 hw/core/qdev.c | 10 ++
 hw/usb/bus.c   | 19 ---
 qdev-monitor.c |  9 -
 3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5783442..4438cbf 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -37,6 +37,7 @@
 #include "hw/boards.h"
 #include "hw/sysbus.h"
 #include "qapi-event.h"
+#include "migration/migration.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -896,6 +897,15 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
 }
 
 if (value && !dev->realized) {
+if (only_migratable && dc->vmsd) {
+if (dc->vmsd->unmigratable) {
+error_setg(_err, "Device %s is not migratable, but "
+   "--only-migratable was specified",
+   object_get_typename(obj));
+goto fail;
+}
+}
+
 if (!obj->parent) {
 gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 1dcc35c..25913ad 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -8,7 +8,6 @@
 #include "monitor/monitor.h"
 #include "trace.h"
 #include "qemu/cutils.h"
-#include "migration/migration.h"
 
 static void usb_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent);
 
@@ -687,8 +686,6 @@ USBDevice *usbdevice_create(const char *cmdline)
 const char *params;
 int len;
 USBDevice *dev;
-ObjectClass *klass;
-DeviceClass *dc;
 
 params = strchr(cmdline,':');
 if (params) {
@@ -723,22 +720,6 @@ USBDevice *usbdevice_create(const char *cmdline)
 return NULL;
 }
 
-klass = object_class_by_name(f->name);
-if (klass == NULL) {
-error_report("Device '%s' not found", f->name);
-return NULL;
-}
-
-dc = DEVICE_CLASS(klass);
-
-if (only_migratable) {
-if (dc->vmsd->unmigratable) {
-error_report("Device %s is not migratable, but --only-migratable "
- "was specified", f->name);
-return NULL;
-}
-}
-
 if (f->usbdevice_init) {
 dev = f->usbdevice_init(bus, params);
 } else {
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 549f45f..5f2fcdf 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -29,7 +29,6 @@
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
 #include "sysemu/block-backend.h"
-#include "migration/migration.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -579,14 +578,6 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 return NULL;
 }
 
-if (only_migratable) {
-if (dc->vmsd->unmigratable) {
-error_setg(errp, "Device %s is not migratable, but "
-   "--only-migratable was specified", driver);
-return NULL;
-}
-}
-
 /* find bus */
 path = qemu_opt_get(opts, "bus");
 if (path != NULL) {
-- 
2.6.2

Re: [Qemu-devel] [RFC 1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[Frederic Konrad]

On 02/04/2017 12:30 PM, Edgar E. Iglesias wrote:
> On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.kon...@greensocs.com wrote:
>> From: KONRAD Frederic 
>>
>> This replaces env1 and page_index variables by env and index
>> so we can use VICTIM_TLB_HIT macro later.
>>
> 
> Hi Fred,
> 
> A question, wouldn't it be more readable to add env and index arguments to 
> VICTIM_TLB_HIT?
> VICTIM_TLB_HIT could perhaps even be made a static inline?
> 
> Cheers,
> Edgar

Hi Edgar,

Well it seems the VICTIM_TLB_HIT macro is just here to hide those
variables actually.

in cputlb.c:
static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx,
   size_t index, size_t elt_ofs,
   target_ulong page)

and then:
/* Macro to call the above, with local variables from the use context.  */
#define VICTIM_TLB_HIT(TY, ADDR) \
  victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \
 (ADDR) & TARGET_PAGE_MASK)

My point of view is clearly that it makes more difficult to read.
So if everybody agrees I can drop the macro and call directly
victim_tlb_hit.

Fred


> 
> 
>  
>> Signed-off-by: KONRAD Frederic 
>> ---
>>  cputlb.c | 18 +-
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/cputlb.c b/cputlb.c
>> index 6c39927..665caea 100644
>> --- a/cputlb.c
>> +++ b/cputlb.c
>> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, 
>> target_ulong addr)
>>   * is actually a ram_addr_t (in system mode; the user mode emulation
>>   * version of this function returns a guest virtual address).
>>   */
>> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>>  {
>> -int mmu_idx, page_index, pd;
>> +int mmu_idx, index, pd;
>>  void *p;
>>  MemoryRegion *mr;
>> -CPUState *cpu = ENV_GET_CPU(env1);
>> +CPUState *cpu = ENV_GET_CPU(env);
>>  CPUIOTLBEntry *iotlbentry;
>>  
>> -page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> -mmu_idx = cpu_mmu_index(env1, true);
>> -if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
>> +index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> +mmu_idx = cpu_mmu_index(env, true);
>> +if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>>   (addr & TARGET_PAGE_MASK))) {
>> -cpu_ldub_code(env1, addr);
>> +cpu_ldub_code(env, addr);
>>  }
>> -iotlbentry = >iotlb[mmu_idx][page_index];
>> +iotlbentry = >iotlb[mmu_idx][index];
>>  pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>>  mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>>  if (memory_region_is_unassigned(mr)) {
>> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, 
>> target_ulong addr)
>>  exit(1);
>>  }
>>  }
>> -p = (void *)((uintptr_t)addr + 
>> env1->tlb_table[mmu_idx][page_index].addend);
>> +p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>>  return qemu_ram_addr_from_host_nofail(p);
>>  }
>>  
>> -- 
>> 1.8.3.1
>>

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 09:26:19AM -0800, Paolo Bonzini wrote:
> 
> 
> On 03/02/2017 09:06, fred.kon...@greensocs.com wrote:
> > +host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, , 
> > );
> > +
> > +if (!host || !size) {
> > +memory_region_transaction_commit();
> > +return false;
> > +}
> > +
> > +sub = g_new(MemoryRegion, 1);
> > +memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
> > +memory_region_add_subregion(mr, offset, sub);
> > +memory_region_transaction_commit();
> > +return true;
> > +}
> > +
> > +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
> > +   unsigned size)
> > +{
> > +MemoryRegionSection section = memory_region_find(mr, offset, size);
> > +
> > +if (section.mr != mr) {
> > +memory_region_del_subregion(mr, section.mr);
> > +/* memory_region_find add a ref on section.mr */
> > +memory_region_unref(section.mr);
> > +object_unparent(OBJECT(section.mr));
> 
> I think this would cause a use-after-free when using MTTCG.  In general,
> creating and dropping MemoryRegions dynamically can cause bugs that are
> nondeterministic and hard to fix without rewriting everything.
> 
> An alternative design could be:
> 
> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
> a pointer, so that the device can map a subset of the device (e.g. a
> single page)
> 
> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
> a Notifier
> 
> - the device adds the Notifier to a NotifierList.  Before invalidating,
> it invokes the Notifier and empties the NotifierList.
> 
> - for the TLB case, the Notifier calls tlb_flush_page.

Interesting! I totally missed the MemoryRegionCache patches. Cool concept.
I few lines about it in docs/memory.txt would have been nice too :-)


> I like the general idea though!

I think it's genial. I was expecting a solution for this to get ugly...
It solves some existing issues for us (like the QSPI one addressed in this 
series).
It also paves the way for certain use-cases when co-simulating with SystemC.
Nice one Fred!

Cheers,
Edgar



> 
> Paolo
> 
> > +}
> > +}

Re: [Qemu-devel] Non-flat command line option argument syntax

[Markus Armbruster]

Paolo Bonzini  writes:

> On 04/02/2017 01:45, Markus Armbruster wrote:
  -drive driver=qcow2,
 file.driver=gluster,
 .volume=testvol,
 .path=/path/a.qcow2,
 .debug=9,
 file.server.0.type=tcp,
  .host=1.2.3.4,
  .port=24007,
 file.server.1.type=unix,
  .socket=/var/run/glusterd.socket

 Mind, I'm not at all sure this is a *good* idea.  I suspect it's more
 magic than it's worth.
>>> As someone who likes dot syntax very much, I don't like it. If you
>>> structure it like this, it's OK, but then you can just write the full
>>> prefix (which gets the point across just as well because I can quickly
>>> tell from a glance that it's the same prefix).
>>>
>>> OTOH, when joined into a single line it doesn't change much in terms of
>>> legibility, in my opinion.
>> 
>> Thanks!
>
> Actually I think it does improve legibility.
>
> It doesn't improve writability though, as anecdotally proved by Markus's
> own mistake.
>
> I am a fan of the dot syntax too.  It seems to be the most incremental
> solution, and it's still as expressive as JSON.

Noted.

> _However_ we could also extend -readconfig to support JSON, i.e. instead of
>
>   [drive "abc"]
>   file = "foo"
>
> it could support
>
>   { 'drive': { 'file: 'foo' }, 'id': 'abc' }
>
> In other words [ would introduce key-value QemuOpts with dot syntax,
> while { would introduce JSON.

Yes, we should support config files in JSON syntax.  Not sure mixing INI
and JSON syntax in the same file is a good idea, though.

Re: [Qemu-devel] [RFC 1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.kon...@greensocs.com wrote:
> From: KONRAD Frederic 
> 
> This replaces env1 and page_index variables by env and index
> so we can use VICTIM_TLB_HIT macro later.
>

Hi Fred,

A question, wouldn't it be more readable to add env and index arguments to 
VICTIM_TLB_HIT?
VICTIM_TLB_HIT could perhaps even be made a static inline?

Cheers,
Edgar


 
> Signed-off-by: KONRAD Frederic 
> ---
>  cputlb.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 6c39927..665caea 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong 
> addr)
>   * is actually a ram_addr_t (in system mode; the user mode emulation
>   * version of this function returns a guest virtual address).
>   */
> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>  {
> -int mmu_idx, page_index, pd;
> +int mmu_idx, index, pd;
>  void *p;
>  MemoryRegion *mr;
> -CPUState *cpu = ENV_GET_CPU(env1);
> +CPUState *cpu = ENV_GET_CPU(env);
>  CPUIOTLBEntry *iotlbentry;
>  
> -page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -mmu_idx = cpu_mmu_index(env1, true);
> -if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
> +index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> +mmu_idx = cpu_mmu_index(env, true);
> +if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>   (addr & TARGET_PAGE_MASK))) {
> -cpu_ldub_code(env1, addr);
> +cpu_ldub_code(env, addr);
>  }
> -iotlbentry = >iotlb[mmu_idx][page_index];
> +iotlbentry = >iotlb[mmu_idx][index];
>  pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>  mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>  if (memory_region_is_unassigned(mr)) {
> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, 
> target_ulong addr)
>  exit(1);
>  }
>  }
> -p = (void *)((uintptr_t)addr + 
> env1->tlb_table[mmu_idx][page_index].addend);
> +p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>  return qemu_ram_addr_from_host_nofail(p);
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] Non-flat command line option argument syntax

[Paolo Bonzini]

On 04/02/2017 01:45, Markus Armbruster wrote:
>>>  -drive driver=qcow2,
>>> file.driver=gluster,
>>> .volume=testvol,
>>> .path=/path/a.qcow2,
>>> .debug=9,
>>> file.server.0.type=tcp,
>>>  .host=1.2.3.4,
>>>  .port=24007,
>>> file.server.1.type=unix,
>>>  .socket=/var/run/glusterd.socket
>>>
>>> Mind, I'm not at all sure this is a *good* idea.  I suspect it's more
>>> magic than it's worth.
>> As someone who likes dot syntax very much, I don't like it. If you
>> structure it like this, it's OK, but then you can just write the full
>> prefix (which gets the point across just as well because I can quickly
>> tell from a glance that it's the same prefix).
>>
>> OTOH, when joined into a single line it doesn't change much in terms of
>> legibility, in my opinion.
> 
> Thanks!

Actually I think it does improve legibility.

It doesn't improve writability though, as anecdotally proved by Markus's
own mistake.

I am a fan of the dot syntax too.  It seems to be the most incremental
solution, and it's still as expressive as JSON.

_However_ we could also extend -readconfig to support JSON, i.e. instead of

[drive "abc"]
file = "foo"

it could support

{ 'drive': { 'file: 'foo' }, 'id': 'abc' }

In other words [ would introduce key-value QemuOpts with dot syntax,
while { would introduce JSON.

Paolo

[Qemu-devel] [PATCH v4] qemu-nbd: Implement socket activation.

[Richard W.M. Jones]

v3 -> v4:

- Remove restriction on --fork again.

- Retest the patch using virt-p2v.

Rich.

[Qemu-devel] [PATCH v5] qemu-nbd: Implement socket activation.

[Richard W.M. Jones]

v3 -> v5:

- Use stringify() macro (thanks Markus).

- Remove --fork restriction again.

- Retest with virt-p2v.

Rich.

[Qemu-devel] [PATCH v5] qemu-nbd: Implement socket activation.

[Richard W.M. Jones]

Socket activation (sometimes known as systemd socket activation)
allows an Internet superserver to pass a pre-opened listening socket
to the process, instead of having qemu-nbd open a socket itself.  This
is done via the LISTEN_FDS and LISTEN_PID environment variables, and a
standard file descriptor range.

This change partially implements socket activation for qemu-nbd.  If
the environment variables are set correctly, then socket activation
will happen automatically, otherwise everything works as before.  The
limitation is that LISTEN_FDS must be 1.

Signed-off-by: Richard W.M. Jones.
---
 qemu-nbd.c | 172 +
 1 file changed, 163 insertions(+), 9 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index c734f62..e4fede6 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -463,6 +463,135 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, 
Error **errp)
 return creds;
 }
 
+static void setup_address_and_port(const char **address, const char **port)
+{
+if (*address == NULL) {
+*address = "0.0.0.0";
+}
+
+if (*port == NULL) {
+*port = stringify(NBD_DEFAULT_PORT);
+}
+}
+
+#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */
+
+#ifndef _WIN32
+/*
+ * Check if socket activation was requested via use of the
+ * LISTEN_FDS and LISTEN_PID environment variables.
+ *
+ * Returns 0 if no socket activation, or the number of FDs.
+ */
+static unsigned int check_socket_activation(void)
+{
+const char *s;
+unsigned long pid;
+unsigned long nr_fds;
+unsigned int i;
+int fd;
+int err;
+
+s = getenv("LISTEN_PID");
+if (s == NULL) {
+return 0;
+}
+err = qemu_strtoul(s, NULL, 10, );
+if (err) {
+if (verbose) {
+fprintf(stderr, "malformed %s environment variable (ignored)\n",
+"LISTEN_PID");
+}
+return 0;
+}
+if (pid != getpid()) {
+if (verbose) {
+fprintf(stderr, "%s was not for us (ignored)\n",
+"LISTEN_PID");
+}
+return 0;
+}
+
+s = getenv("LISTEN_FDS");
+if (s == NULL) {
+return 0;
+}
+err = qemu_strtoul(s, NULL, 10, _fds);
+if (err) {
+if (verbose) {
+fprintf(stderr, "malformed %s environment variable (ignored)\n",
+"LISTEN_FDS");
+}
+return 0;
+}
+assert(nr_fds <= UINT_MAX);
+
+/* A limitation of current qemu-nbd is that it can only listen on
+ * a single socket.  When that limitation is lifted, we can change
+ * this function to allow LISTEN_FDS > 1, and remove the assertion
+ * in the main function below.
+ */
+if (nr_fds > 1) {
+error_report("qemu-nbd does not support socket activation with %s > 1",
+ "LISTEN_FDS");
+exit(EXIT_FAILURE);
+}
+
+/* So these are not passed to any child processes we might start. */
+unsetenv("LISTEN_FDS");
+unsetenv("LISTEN_PID");
+
+/* So the file descriptors don't leak into child processes. */
+for (i = 0; i < nr_fds; ++i) {
+fd = FIRST_SOCKET_ACTIVATION_FD + i;
+if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) {
+/* If we cannot set FD_CLOEXEC then it probably means the file
+ * descriptor is invalid, so socket activation has gone wrong
+ * and we should exit.
+ */
+error_report("Socket activation failed: "
+ "invalid file descriptor fd = %d: %m",
+ fd);
+exit(EXIT_FAILURE);
+}
+}
+
+return (unsigned int) nr_fds;
+}
+
+#else /* !_WIN32 */
+static unsigned int check_socket_activation(void)
+{
+return 0;
+}
+#endif
+
+/*
+ * Check socket parameters compatibility when socket activation is used.
+ */
+static const char *socket_activation_validate_opts(const char *device,
+   const char *sockpath,
+   const char *address,
+   const char *port)
+{
+if (device != NULL) {
+return "NBD device can't be set when using socket activation";
+}
+
+if (sockpath != NULL) {
+return "Unix socket can't be set when using socket activation";
+}
+
+if (address != NULL) {
+return "The interface can't be set when using socket activation";
+}
+
+if (port != NULL) {
+return "TCP port number can't be set when using socket activation";
+}
+
+return NULL;
+}
 
 int main(int argc, char **argv)
 {
@@ -471,7 +600,7 @@ int main(int argc, char **argv)
 off_t dev_offset = 0;
 uint16_t nbdflags = 0;
 bool disconnect = false;
-const char *bindto = "0.0.0.0";
+const char *bindto = NULL;
 const char *port = NULL;
 char *sockpath = NULL;
 char *device = NULL;
@@ -533,6 +662,7 @@ int 

[Qemu-devel] [PATCH v4] qemu-nbd: Implement socket activation.

[Richard W.M. Jones]

Socket activation (sometimes known as systemd socket activation)
allows an Internet superserver to pass a pre-opened listening socket
to the process, instead of having qemu-nbd open a socket itself.  This
is done via the LISTEN_FDS and LISTEN_PID environment variables, and a
standard file descriptor range.

This change partially implements socket activation for qemu-nbd.  If
the environment variables are set correctly, then socket activation
will happen automatically, otherwise everything works as before.  The
limitation is that LISTEN_FDS must be 1.

Signed-off-by: Richard W.M. Jones.
---
 qemu-nbd.c | 175 +
 1 file changed, 166 insertions(+), 9 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index c734f62..c76fd76 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -463,6 +463,138 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, 
Error **errp)
 return creds;
 }
 
+#define MACRO_EXPAND_STRINGIFY(x) STRINGIFY(x)
+#define STRINGIFY(x) #x
+
+static void setup_address_and_port(const char **address, const char **port)
+{
+if (*address == NULL) {
+*address = "0.0.0.0";
+}
+
+if (*port == NULL) {
+*port = MACRO_EXPAND_STRINGIFY(NBD_DEFAULT_PORT);
+}
+}
+
+#define FIRST_SOCKET_ACTIVATION_FD 3 /* defined by systemd ABI */
+
+#ifndef _WIN32
+/*
+ * Check if socket activation was requested via use of the
+ * LISTEN_FDS and LISTEN_PID environment variables.
+ *
+ * Returns 0 if no socket activation, or the number of FDs.
+ */
+static unsigned int check_socket_activation(void)
+{
+const char *s;
+unsigned long pid;
+unsigned long nr_fds;
+unsigned int i;
+int fd;
+int err;
+
+s = getenv("LISTEN_PID");
+if (s == NULL) {
+return 0;
+}
+err = qemu_strtoul(s, NULL, 10, );
+if (err) {
+if (verbose) {
+fprintf(stderr, "malformed %s environment variable (ignored)\n",
+"LISTEN_PID");
+}
+return 0;
+}
+if (pid != getpid()) {
+if (verbose) {
+fprintf(stderr, "%s was not for us (ignored)\n",
+"LISTEN_PID");
+}
+return 0;
+}
+
+s = getenv("LISTEN_FDS");
+if (s == NULL) {
+return 0;
+}
+err = qemu_strtoul(s, NULL, 10, _fds);
+if (err) {
+if (verbose) {
+fprintf(stderr, "malformed %s environment variable (ignored)\n",
+"LISTEN_FDS");
+}
+return 0;
+}
+assert(nr_fds <= UINT_MAX);
+
+/* A limitation of current qemu-nbd is that it can only listen on
+ * a single socket.  When that limitation is lifted, we can change
+ * this function to allow LISTEN_FDS > 1, and remove the assertion
+ * in the main function below.
+ */
+if (nr_fds > 1) {
+error_report("qemu-nbd does not support socket activation with %s > 1",
+ "LISTEN_FDS");
+exit(EXIT_FAILURE);
+}
+
+/* So these are not passed to any child processes we might start. */
+unsetenv("LISTEN_FDS");
+unsetenv("LISTEN_PID");
+
+/* So the file descriptors don't leak into child processes. */
+for (i = 0; i < nr_fds; ++i) {
+fd = FIRST_SOCKET_ACTIVATION_FD + i;
+if (fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) {
+/* If we cannot set FD_CLOEXEC then it probably means the file
+ * descriptor is invalid, so socket activation has gone wrong
+ * and we should exit.
+ */
+error_report("Socket activation failed: "
+ "invalid file descriptor fd = %d: %m",
+ fd);
+exit(EXIT_FAILURE);
+}
+}
+
+return (unsigned int) nr_fds;
+}
+
+#else /* !_WIN32 */
+static unsigned int check_socket_activation(void)
+{
+return 0;
+}
+#endif
+
+/*
+ * Check socket parameters compatibility when socket activation is used.
+ */
+static const char *socket_activation_validate_opts(const char *device,
+   const char *sockpath,
+   const char *address,
+   const char *port)
+{
+if (device != NULL) {
+return "NBD device can't be set when using socket activation";
+}
+
+if (sockpath != NULL) {
+return "Unix socket can't be set when using socket activation";
+}
+
+if (address != NULL) {
+return "The interface can't be set when using socket activation";
+}
+
+if (port != NULL) {
+return "TCP port number can't be set when using socket activation";
+}
+
+return NULL;
+}
 
 int main(int argc, char **argv)
 {
@@ -471,7 +603,7 @@ int main(int argc, char **argv)
 off_t dev_offset = 0;
 uint16_t nbdflags = 0;
 bool disconnect = false;
-const char *bindto = "0.0.0.0";
+const char *bindto = NULL;
 const char *port = 

Re: [Qemu-devel] [PATCH] CODING_STYLE: Mention preferred comment form

[Markus Armbruster]

Peter Maydell  writes:

> Our defacto coding style strongly prefers /* */ style comments
> over the single-line // style, and checkpatch enforces this,
> but we don't actually document this. Mention it in CODING_STYLE.
>
> Suggested-by: Thomas Huth 
> Signed-off-by: Peter Maydell 
> ---
>  CODING_STYLE | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index f53180b..2fa0c0b 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -116,3 +116,10 @@ if (a == 1) {
>  Rationale: Yoda conditions (as in 'if (1 == a)') are awkward to read.
>  Besides, good compilers already warn users when '==' is mis-typed as '=',
>  even when the constant is on the right.
> +
> +7. Comment style
> +
> +We use traditional C-style /* */ comments and avoid // comments.
> +
> +Rationale: The // form is valid in C99, so this is purely a matter of
> +consistency of style. The checkpatch script will warn you about this.

Reviewed-by: Markus Armbruster 

Re: [Qemu-devel] [PATCH v2] qemu-nbd: Implement socket activation.

[Markus Armbruster]

"Richard W.M. Jones"  writes:

> On Fri, Feb 03, 2017 at 03:16:43PM +, Stefan Hajnoczi wrote:
>> On Thu, Feb 02, 2017 at 05:16:25PM +, Richard W.M. Jones wrote:
>> > +if (*port == NULL) {
>> > +*port = g_strdup_printf("%d", NBD_DEFAULT_PORT);;
>> 
>> Please stringify NBD_DEFAULT_PORT instead of using g_strdup_printf().
>> That avoids the memory leak.
>
> Oops.
>
> Do we have a macro for this already?  I couldn't see one, and the
> best I could come up with is:
>
> #define MACRO_EXPAND_STRINGIFY(x) STRINGIFY(x)
> #define STRINGIFY(x) #x

Check out stringify() in compiler.h.

(Yes, lower-case macros that don't behave like functions are bad style)

[...]

Re: [Qemu-devel] [Qemu-block] Non-flat command line option argument syntax

[Markus Armbruster]

Max Reitz  writes:

> I like both JSON and dot syntax. But I like them differently in
> different places.
>
> I love JSON when it's in some file where I can turn out syntax
> highlighting and let my $EDITOR match brackets and braces.
>
> I hate JSON when it's on the command line. You have to escape, you get
> strings in strings, and at least for QMP you sometimes even get strings
> in strings in strings (yes, I like my "echo | qemu -qmp stdio" with
> human-monitor-command). Apart from that, usually I don't format anything
> nicely on the command line anyway, so JSON and dot syntax are equally
> illegible then.
>
> JSON is great for reading, when formatted correctly. If it's not
> formatted nicely and you don't have a good editor with highlighting,
> it's pretty bad.
> It's good for writing in an editor. It's not so nice for writing in a shell.

Pro-tip: M-x shell blinks parenthesis.

But point taken.

> OTOH, it's hard to read dot syntax when formatted correctly and it's
> just as bad as JSON when it isn't. But even if you have an editor at
> hand, you can't make it better.
> It's very easy to write dot syntax, however. Just write down what you
> want. Oh, forgot a parameter for that dict three arrays ago? Just write
> it down now. Doesn't matter where you put it. How many braces do I need
> to close now? Oh, right, I don't need to close any. Nice!
>
> So dot syntax is pretty much a write-only syntax. But it's very good at
> that.

Tiresomely repetitive, though.

> On the command line I absolutely adore the dot syntax. It doesn't force
> you to quote, you can put everything anywhere and you don't need to
> count braces. I love it.
>
> However, nobody can read what you wrote. Usually doesn't matter. But for
> docs, that's bad. For scripts, it depends, but again, it usually is bad.
> For configuration files, there is pretty much no excuse. So in general,
> I'm very skeptic about dot syntax in files, to say the least.

Me too.

> So I think it would be good to allow full-JSON configuration. Put it in
> a file, great.
>
> But at the same time, I do not think that JSON is good for the command
> line. Dot syntax works fine and in my opinion it often doesn't actually
> matter whether it's legible or not.
>
>
> I don't like structured values very much because to me they are just
> "JSON light". Well, you don't have to quote keys and values (so no
> "string in string" mess), but other than that you still have to quote
> everything and you still have to count braces.
>
> Max
>
>
> PS: I personally actually think that structured representations such as
> JSON may in some situations be less legible than the dot syntax if you
> do not have syntax highlighting and it's not formatted nicely; and
> that's because you have to count braces not just when writing but also
> when reading. Imagine the following:
>
> a.b.c.d.e.f=42,a.b.c.g=23
>
> {"a":{"b":{"c":{"d":{"e":{"f":42}}},"g":23}}}
>
> I can read the first one much better than the second one. Of course,
> that's different with nice formatting and a good editor, but the above
> is how I would write it on the command line.
>
> I know it's a fabricated example and you'd just need to switch "g" and
> "d", but "}}" actually makes me a bit dizzy, so that may be even
> worse. Anyway, I just wanted to make the point that I think that compact
> JSON and dot syntax are both pretty much illegible.

Quoting myself: "both variants are basically illegible.  This is simply
something that belongs into a config file rather than the command line."

Re: [Qemu-devel] Non-flat command line option argument syntax

[Markus Armbruster]

"Richard W.M. Jones"  writes:

> On Thu, Feb 02, 2017 at 08:42:33PM +0100, Markus Armbruster wrote:
>> There's also the -drive file=json:... syntax.  It's a bad fit for
>> QemuOpts, because QemuOpts and JSON fight for the comma.  I'd show you
>> if I could get it to work.
>
> I think this refers to the json: syntax for qemu URIs?

The json: pseudo-protocol, i.e. "file names" starting with a "json:"
protocol prefix.  I think we mean the same.

> Anyway we're using this in virt-v2v
> (https://github.com/libguestfs/libguestfs/blob/master/v2v/input_libvirt_xen_ssh.ml)
>
> It's very useful, please try not to break it!

We're not going to take the json: pseudo-protocol away.  Perhaps we'll
deprecate it at some point, but even then it'll stay for a sufficiently
generous grace period.

Re: [Qemu-devel] [Qemu-block] Non-flat command line option argument syntax

[Markus Armbruster]

Max Reitz  writes:

> On 03.02.2017 08:50, Markus Armbruster wrote:
>> "Dr. David Alan Gilbert"  writes:
>> 
>>> * Markus Armbruster (arm...@redhat.com) wrote:
 = Introduction =

>>>
>>> 
>>>
 = Structured option argument syntax =

 == JSON ==

 The obvious way to provide the expressiveness of JSON on the command
 line is JSON.  Easy enough[2].  However, besides not being compatible,
 it's rather heavy on syntax, at least for simple cases.  Compare:

 -machine q35,accel=kvm
 -machine '{ "type": "q35", "accel": "kvm"}'

 It compares a bit more favourably in cases that use our non-flat hacks.
 Here's a flat list as KEY=VALUE,... with repeated keys, and as JSON:

 -semihosting-config enable,arg=eins,arg=zwei,arg=drei
 -semihosting-config '{ "enable": true, "arg": [ "eins", "zwei", "drei" 
 ] }'

 Arbitrary nesting with dotted key convention:

 -drive driver=qcow2,file.driver=gluster,
file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
file.server.0.type=tcp,
file.server.0.host=1.2.3.4,
file.server.0.port=24007,
file.server.1.type=unix,
file.server.1.socket=/var/run/glusterd.socket
 -drive '{ "driver": "qcow2",
   "file": {
   "driver": "gluster", "volume": "testvol",
   "path": "/path/a.qcow2", "debug": 9,
   "server": [ { "type": "tcp",
 "host": "1.2.3.4", "port": "24007"},
   { "type": "unix",
 "socket": "/var/run/glusterd.socket" } ] } 
 }'
>>>
>>> So while I generally hate JSON, the -drive dotted key syntax makes
>>> me mad when it gets like this;  have a look
>>> at the block replication and quorum setups especially, that can end up
>>> with (from docs/COLO-FT.txt):
>>>
>>>   -drive 
>>> if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
>>>  children.0.file.filename=1.raw,\
>>>  children.0.driver=raw -S
>>>
>>>that's just way too many .'s to ever properly understand.
>>> (I'm sure it used to be more complex).
>> 
>> Here's an idea to cut down on the dottery that drives you mad (and me
>> too): if KEY starts with '.', combine it with a prefix of the previous
>> one so that the result has the same number of name components.
>> 
>> Your example becomes
>> 
>> -drive 
>> if=virtio,id=primary-disk0,driver=quorum,read-pattern=fifo,vote-threshold=1,\
>>children.0.file.filename=1.raw,.driver=raw -S
>
> No, the last option would be children.0.file.driver=raw when expanded
> (which is wrong, it should be children.0.driver).

You're right.

>> My example
>> 
>>  -drive driver=qcow2,file.driver=gluster,
>> file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>> file.server.0.type=tcp,
>> file.server.0.host=1.2.3.4,
>> file.server.0.port=24007,
>> file.server.1.type=unix,
>> file.server.1.socket=/var/run/glusterd.socket
>> 
>> becomes
>> 
>>  -drive driver=qcow2,
>> file.driver=gluster,
>> .volume=testvol,
>> .path=/path/a.qcow2,
>> .debug=9,
>> file.server.0.type=tcp,
>>  .host=1.2.3.4,
>>  .port=24007,
>> file.server.1.type=unix,
>>  .socket=/var/run/glusterd.socket
>> 
>> Mind, I'm not at all sure this is a *good* idea.  I suspect it's more
>> magic than it's worth.
>
> As someone who likes dot syntax very much, I don't like it. If you
> structure it like this, it's OK, but then you can just write the full
> prefix (which gets the point across just as well because I can quickly
> tell from a glance that it's the same prefix).
>
> OTOH, when joined into a single line it doesn't change much in terms of
> legibility, in my opinion.

Thanks!

Re: [Qemu-devel] Non-flat command line option argument syntax

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert"  writes:
>> 
>> > * Markus Armbruster (arm...@redhat.com) wrote:
[...]
>> >> === Structured values ===
>> >> 
>> >> The dotted key convention messes with KEY syntax to permit structured
>> >> values.  Works, but the more conventional way to support structured
>> >> values is a syntax for structured values.  
>> >> 
>> >> An obvious one is to use { KEY=VALUE, ...} for objects, and [ VALUE,
>> >> ... ] for arrays.  Looks like this:
>> >> 
>> >> -drive 'driver=quorum,
>> >> child=[{ driver=file, filename=disk1.img },
>> >>{ driver=host_device, filename=/dev/sdb },
>> >>{ driver=nbd, host=localhost } ]'
>> >> 
>> >> Again, lines broken and indented for legibility; you need to join them
>> >> for actual use.
>> >> 
>> >> There's a syntactic catch, though: a value of the form [ ... ] can
>> >> either be an array or a string.  Which one it is depends on the type of
>> >> the key.  To parse this syntax, you need to know the types, unlike JSON
>> >> or traditional QemuOpts.  Unless we outlaw strings starting with '{' or
>> >> '[', which feels impractical.
>> >
>> > I don't understand why [ could imply a string.
>> 
>> Consider
>> 
>> -drive 'driver=quorum,
>> child=[{ driver=file, filename={"foolish":"name"} },
>>{ driver=host_device, filename=/dev/sdb },
>>{ driver=nbd, host=[::1] } ]'
>> 
>> Three KEY=VALUE have their VALUE start with '[' or '{':
>> 
>> * child=[{ driver=file, ...
>> 
>>   This is an array, not a string, because child is an array.
>> 
>> * host=[::1]
>> 
>>   This is a string, not an array containing the string "::1", because
>>   host is a string.
>
> Why is that accepted as valid input? Can't that just spit a
> 'I wanted a string not an array' ?

Because "[::1]" is a perfectly valid string.

It's also a valid array.  Unless the parser knows what type to expect
here, it cannot decide how to parse it.  That's why I wrote "to parse
this syntax, you need to know the types".

>> * filename={"foolish":"name"}
>> 
>>   This is a string, not an object, because filename is a string.
>> 
>> Clearer now?
>> 
>> >> But wait, there's another syntactic catch: in traditional QemuOpts, a
>> >> value ends at the next unescaped ',' or '\0'.  Inside an object, it now
>> >> also ends at the next unescaped '}', and inside an array, at the next
>> >> unescaped ']'.  Or perhaps at the next space (the example above assumes
>> >> it does).  That means we either have to provide a way to escape '}', ']'
>> >> and space, or find another way to delimit string values, say require '"'
>> >> around strings whenever the string contains "funny" characters.
>> >
>> > How about a tighter rule that if you've got a structured value -
>> > i.e. you're inside either of [ or {, then you must " all strings
>> > (except keys that we keep clean).
>> 
>> Matter of taste.
>> 
>> Regardless, we need a way to escape '"'.  Doubling it would be
>> consistent with the existing escape of ','.
>
> Doubling for escape feels hideous, especially for ".

No argument, but it beats counting backslashes.

Re: [Qemu-devel] [PATCH v2] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Paolo Bonzini]

On 04/02/2017 00:59, Ashijeet Acharya wrote:
> Commit a3a3d8c7 introduced a segfault bug while checking for
> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
> devices which do no set their 'dc->vmsd' yet while initialization.
> Place a 'dc->vmsd' check prior to it so that we do not segfault for
> such devices.
> 
> NOTE: This doesn't compromise the functioning of --only-migratable
> option as all the unmigratable devices do set their 'dc->vmsd'.
> 
> Signed-off-by: Ashijeet Acharya 
> Reviewed-by: Juan Quintela 
> ---
> Changes in v2:
> - place dc->vmsd check in hw/usb/bus.c as well
> ---
>  hw/usb/bus.c   | 2 +-
>  qdev-monitor.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 1dcc35c..1e39b2c 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -731,7 +731,7 @@ USBDevice *usbdevice_create(const char *cmdline)
>  
>  dc = DEVICE_CLASS(klass);
>  
> -if (only_migratable) {
> +if (only_migratable && dc->vmsd) {
>  if (dc->vmsd->unmigratable) {
>  error_report("Device %s is not migratable, but --only-migratable 
> "
>   "was specified", f->name);
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 549f45f..b72e5a4 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -579,7 +579,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>  return NULL;
>  }
>  
> -if (only_migratable) {
> +if (only_migratable && dc->vmsd) {
>  if (dc->vmsd->unmigratable) {
>  error_setg(errp, "Device %s is not migratable, but "
> "--only-migratable was specified", driver);
> 

Could you handle only_migratable in device_set_realized (inside "if
(value && !dev->realized)"), to avoid the code duplication?  Separate
patch from this one, of course.

Thanks,

Paolo

[Qemu-devel] [PATCH v2] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Ashijeet Acharya]

Commit a3a3d8c7 introduced a segfault bug while checking for
'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
devices which do no set their 'dc->vmsd' yet while initialization.
Place a 'dc->vmsd' check prior to it so that we do not segfault for
such devices.

NOTE: This doesn't compromise the functioning of --only-migratable
option as all the unmigratable devices do set their 'dc->vmsd'.

Signed-off-by: Ashijeet Acharya 
Reviewed-by: Juan Quintela 
---
Changes in v2:
- place dc->vmsd check in hw/usb/bus.c as well
---
 hw/usb/bus.c   | 2 +-
 qdev-monitor.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 1dcc35c..1e39b2c 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -731,7 +731,7 @@ USBDevice *usbdevice_create(const char *cmdline)
 
 dc = DEVICE_CLASS(klass);
 
-if (only_migratable) {
+if (only_migratable && dc->vmsd) {
 if (dc->vmsd->unmigratable) {
 error_report("Device %s is not migratable, but --only-migratable "
  "was specified", f->name);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 549f45f..b72e5a4 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -579,7 +579,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
 return NULL;
 }
 
-if (only_migratable) {
+if (only_migratable && dc->vmsd) {
 if (dc->vmsd->unmigratable) {
 error_setg(errp, "Device %s is not migratable, but "
"--only-migratable was specified", driver);
-- 
2.6.2

Re: [Qemu-devel] [PATCH] migrate: Introduce a 'dc->vmsd' check to avoid segfault for --only-migratable

[Ashijeet Acharya]

On Mon, Jan 30, 2017 at 10:08 PM, Juan Quintela  wrote:
> Peter Maydell  wrote:
>> On 30 January 2017 at 14:41, Ashijeet Acharya  
>> wrote:
>>> Commit a3a3d8c7 introduced a segfault bug while checking for
>>> 'dc->vmsd->unmigratable' which caused QEMU to crash when trying to add
>>> devices which do no set their 'dc->vmsd' yet while initialization.
>>> Place a 'dc->vmsd' check prior to it so that we do not segfault for
>>> such devices.
>>>
>>> NOTE: This doesn't compromise the functioning of --only-migratable
>>> option as all the unmigratable devices do set their 'dc->vmsd'.
>>>
>>> Signed-off-by: Ashijeet Acharya 
>>> ---
>>>  qdev-monitor.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 81d01df..a1106fd 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -578,7 +578,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error 
>>> **errp)
>>>  return NULL;
>>>  }
>>>
>>> -if (only_migratable) {
>>> +if (only_migratable && dc->vmsd) {
>>>  if (dc->vmsd->unmigratable) {
>>>  error_setg(errp, "Device %s is not migratable, but "
>>> "--only-migratable was specified", driver);
>>
>> This seems like a good fix for the crash as a short term fix,
>> but longer term I think it would be better to make setting
>> dc->vmsd mandatory. I think devices which don't set it fall into
>> one of these categories:
>>  * deliberately has no VMState struct as it has no state that
>>needs migrating -- we should have some kind of flag for
>>such devices to positively assert that they don't need to
>>deal with migration
>>  * accidentally failed to provide a VMState struct -- this is
>>a bug and the device should be fixed to at minimum mark
>>itself as unmigratable (and ideally fixed to implement
>>migration!)
>>  * didn't provide a VMState struct because they handle
>>migration manually using vmstate_register() -- we should
>>update these to use VMState, or failing that use the
>>flag to say "deliberately not providing VMState"
>>
>> Then we can make QEMU assert if dc->vmsd is NULL, and
>> catch future occurrences of "oops I didn't think about
>> migration" bugs in new device models. We also have an
>> easy way to grep the codebase for devices that need to
>> have migration implemented.
>
> +1
>
> Reviewed-by: Juan Quintela 

I just realised that I had to make this fix at two different places
i.e. inside usbdevice_create() in hw/usb/bus.c as well. I will send a
v2 for that and already include Juan's R-b tag.

Ashijeet

[Qemu-devel] [Bug 1661815] [NEW] Stack address is returned from function translate_one

[shqking]

Public bug reported:

The vulnerable version is qemu-2.8.0, and the vulnerable function is in
"target-s390x/translate.c".

The code snippet is as following.

static ExitStatus translate_one(CPUS390XState *env, DisasContext *s)
{
const DisasInsn *insn;
ExitStatus ret = NO_EXIT;
DisasFields f;
...
s->fields = 
...
s->pc = s->next_pc;
return ret;
}

A stack address, i.e. the address of local variable "f" is returned from
current function through the output parameter "s->fields" as a side
effect.

This issue is one kind of undefined behaviors, according the C Standard,
6.2.4 [ISO/IEC 9899:2011]
(https://www.securecoding.cert.org/confluence/display/c/DCL30-C.+Declare+objects+with+appropriate+storage+durations)

This dangerous defect may lead to an exploitable vulnerability.
We suggest sanitizing "s->fields" as null before return.

Note that this issue is reported by shqking and Zhenwei Zou together.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  Stack address is returned from function translate_one

Status in QEMU:
  New

Bug description:
  The vulnerable version is qemu-2.8.0, and the vulnerable function is
  in "target-s390x/translate.c".

  The code snippet is as following.

  static ExitStatus translate_one(CPUS390XState *env, DisasContext *s)
  {
  const DisasInsn *insn;
  ExitStatus ret = NO_EXIT;
  DisasFields f;
  ...
  s->fields = 
  ...
  s->pc = s->next_pc;
  return ret;
  }

  A stack address, i.e. the address of local variable "f" is returned
  from current function through the output parameter "s->fields" as a
  side effect.

  This issue is one kind of undefined behaviors, according the C
  Standard, 6.2.4 [ISO/IEC 9899:2011]
  
(https://www.securecoding.cert.org/confluence/display/c/DCL30-C.+Declare+objects+with+appropriate+storage+durations)

  This dangerous defect may lead to an exploitable vulnerability.
  We suggest sanitizing "s->fields" as null before return.

  Note that this issue is reported by shqking and Zhenwei Zou together.

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