Re: [Qemu-devel] [PATCH 3/9] virtio-blk: fix disabled mode

[tu bo]

Hi Paolo:

On 03/30/2016 08:48 PM, Paolo Bonzini wrote:

The missing check on dataplane_disabled caused a segmentation
fault in notify_guest_bh, because s->guest_notifier was NULL.

Signed-off-by: Paolo Bonzini 
---
  hw/block/dataplane/virtio-blk.c | 7 +++
  hw/block/virtio-blk.c   | 2 +-
  include/hw/virtio/virtio-blk.h  | 1 +
  3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0d76110..378feb3 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -28,7 +28,6 @@
  struct VirtIOBlockDataPlane {
  bool starting;
  bool stopping;
-bool disabled;

  VirtIOBlkConf *conf;

@@ -233,7 +232,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
fail_host_notifier:
  k->set_guest_notifiers(qbus->parent, 1, false);
fail_guest_notifiers:
-s->disabled = true;
+vblk->dataplane_disabled = true;
  s->starting = false;
  vblk->dataplane_started = true;
  }
@@ -250,8 +249,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
  }

  /* Better luck next time. */
-if (s->disabled) {
-s->disabled = false;
+if (vblk->dataplane_disabled) {
+vblk->dataplane_disabled = false;
  vblk->dataplane_started = false;
  return;
  }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index d0b8248..77221c1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -53,7 +53,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, 
unsigned char status)

  stb_p(&req->in->status, status);
  virtqueue_push(s->vq, &req->elem, req->in_len);
-if (s->dataplane) {
+if (s->dataplane_started && !s->dataplane_disabled) {
  virtio_blk_data_plane_notify(s->dataplane);
  } else {
  virtio_notify(vdev, s->vq);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5cb66cd..073c632 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,6 +53,7 @@ typedef struct VirtIOBlock {
  unsigned short sector_mask;
  bool original_wce;
  VMChangeStateEntry *change;
+bool dataplane_disabled;
  bool dataplane_started;
  int reentrancy_test;


There is no "int reentrancy_test;" in the latest qemu master. thx


  struct VirtIOBlockDataPlane *dataplane;

Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race

[tu bo]

Hi Cornelia:

On 03/31/2016 10:43 AM, tu bo wrote:

Hi Cornelia:

I saw qemu crash for this patch on qemu master yesterday.

However, scsi disks of my lpar is not available because of s38 firmware
update.  I'll double check the test when it's ready. thx


I still can see crash like below,

(gdb) bt
#0  blk_aio_read_entry (opaque=0x0) at block/block-backend.c:916
#1  0x02aa0d6e873e in coroutine_trampoline (i0=, 
i1=1275094848) at util/coroutine-ucontext.c:78

#2  0x03ffab4d150a in __makecontext_ret () from /lib64/libc.so.6
(gdb) list
911 static void blk_aio_read_entry(void *opaque)
912 {
913 BlkAioEmAIOCB *acb = opaque;
914 BlkRwCo *rwco = &acb->rwco;
915 
916 rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, rwco->qiov->size,
917   rwco->qiov, rwco->flags);
918 blk_aio_complete(acb);
919 }
920 




On 03/29/2016 10:17 PM, Cornelia Huck wrote:

The ->set_host_notifier() callback is invoked whenever we want to
switch from or to the generic ioeventfd handler. Currently, all
transports deregister the ioeventfd backing and then re-register
it. This opens a race window where we are without ioeventfd
backing for a time period: In the virtio-blk dataplane case, we
observed notifications coming in from both the vcpu thread and
the iothread.

Let's change pci, mmio and ccw to keep the ioeventfd during
->set_host_notifier() and only switch the ioeventfd handler.

Signed-off-by: Cornelia Huck 
---
  hw/s390x/virtio-ccw.c  | 22 +-
  hw/virtio/virtio-mmio.c| 27 +--
  hw/virtio/virtio-pci.c | 28 ++--
  include/hw/virtio/virtio-bus.h |  4 
  4 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cb887ba..7b1088e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1196,14 +1196,26 @@ static bool
virtio_ccw_query_guest_notifiers(DeviceState *d)
  static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool
assign)
  {
  VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+VirtQueue *vq = virtio_get_queue(vdev, n);

-/* Stop using the generic ioeventfd, we are doing eventfd handling
- * ourselves below */
-dev->ioeventfd_disabled = assign;
  if (assign) {
-virtio_ccw_stop_ioeventfd(dev);
+/* Stop using the generic ioeventfd, we are doing eventfd
handling
+ * ourselves below */
+dev->ioeventfd_disabled = true;
+};
+/*
+ * Just switch the handler, don't deassign the ioeventfd.
+ * Otherwise, there's a window where we don't have an
+ * ioeventfd and we may end up with a notification where
+ * we don't expect one.
+ */
+virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+if (!assign) {
+/* Use generic ioeventfd handler again. */
+dev->ioeventfd_disabled = false;
  }
-return virtio_ccw_set_guest2host_notifier(dev, n, assign, false);
+return 0;
  }

  static int virtio_ccw_get_mappings(VirtioCcwDevice *dev)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index d4cd91f..aafebdf 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -502,19 +502,26 @@ static int
virtio_mmio_set_host_notifier(DeviceState *opaque, int n,
   bool assign)
  {
  VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+VirtQueue *vq = virtio_get_queue(vdev, n);

-/* Stop using ioeventfd for virtqueue kick if the device starts
using host
- * notifiers.  This makes it easy to avoid stepping on each
others' toes.
- */
-proxy->ioeventfd_disabled = assign;
  if (assign) {
-virtio_mmio_stop_ioeventfd(proxy);
+/* Stop using the generic ioeventfd, we are doing eventfd
handling
+ * ourselves below */
+proxy->ioeventfd_disabled = true;
+};
+/*
+ * Just switch the handler, don't deassign the ioeventfd.
+ * Otherwise, there's a window where we don't have an
+ * ioeventfd and we may end up with a notification where
+ * we don't expect one.
+ */
+virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+if (!assign) {
+/* Use generic ioeventfd handler again. */
+proxy->ioeventfd_disabled = false;
  }
-/* We don't need to start here: it's not needed because backend
- * currently only stops on status change away from ok,
- * reset, vmstop and such. If we do add code to start here,
- * need to check vmstate, device state etc. */
-return virtio_mmio_set_host_notifier_internal(proxy, n, assign,
false);
+return 0;
  }

  /*

Re: [Qemu-devel] [PATCH 0/6] virtio: refactor host notifiers

[tu bo]

Hi Christian:

I got the same crash with qemu master + assertion patch + "[PATCH 0/6] 
virtio: refactor host notifiers" +  Paolo's fix,


(gdb) bt
#0  blk_aio_read_entry (opaque=0x0) at block/block-backend.c:916
#1  0x02aa2e8e88fe in coroutine_trampoline (i0=, 
i1=-1677703696) at util/coroutine-ucontext.c:78

#2  0x03ffa85d150a in __makecontext_ret () from /lib64/libc.so.6


On 03/30/2016 12:27 AM, Christian Borntraeger wrote:

On 03/29/2016 03:50 PM, Paolo Bonzini wrote:



On 29/03/2016 13:45, Cornelia Huck wrote:

Hi Tu Bo,

please always include the assertion patch at
https://lists.gnu.org/archive/html/qemu-block/2016-03/msg00546.html in
your tests.  Can you include the backtrace from all threads with that patch?


thanks for your reminder about the assertion patch. Here is the
backtrace with qemu master + assertion patch + "[PATCH 0/6] virtio:
refactor host notifiers",


FWIW, I've been running this in a reboot loop for the last 2 1/2 hours.
Could you perhaps share your command line?


 From code inspection, the following is also necessary or at least a
good idea:

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 6fb29e3..7fa8477 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -258,7 +258,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
  aio_context_acquire(s->ctx);

  /* Stop notifications for new requests from guest */
-virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
+virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false);

  /* Drain and switch bs back to the QEMU main loop */
  blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());



Right. Tu Bo, you seem to have the best testcase for this.
Does your setup runs fine with this on top?

CHristian

Re: [Qemu-devel] [PATCH 0/6] virtio: refactor host notifiers

[tu bo]

Hi Christian:

On 03/29/2016 07:54 PM, Christian Borntraeger wrote:

On 03/29/2016 11:14 AM, tu bo wrote:

Hi Paolo:

On 03/29/2016 02:11 AM, Paolo Bonzini wrote:

On 28/03/2016 05:55, TU BO wrote:

Hi Cornelia:

I got two crash with qemu master + "[PATCH 0/6] virtio: refactor host
notifiers",


Hi Tu Bo,

please always include the assertion patch at
https://lists.gnu.org/archive/html/qemu-block/2016-03/msg00546.html in
your tests.  Can you include the backtrace from all threads with that patch?


thanks for your reminder about the assertion patch. Here is the backtrace with qemu 
master + assertion patch + "[PATCH 0/6] virtio: refactor host notifiers",

I got two crashes,

1. For 1st crash,
(gdb) thread apply all bt

Thread 8 (Thread 0x3ff8daf1910 (LWP 52859)):
#0  0x03ff9718ec62 in do_futex_timed_wait () from /lib64/libpthread.so.0
#1  0x03ff9718ed76 in sem_timedwait () from /lib64/libpthread.so.0
#2  0x02aa2d755868 in qemu_sem_timedwait (sem=0x3ff88000fa8, ms=) at util/qemu-thread-posix.c:245
#3  0x02aa2d6803e4 in worker_thread (opaque=0x3ff88000f40) at 
thread-pool.c:92
#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 7 (Thread 0x3ff8e679910 (LWP 52856)):
#0  0x03ff9718ec62 in do_futex_timed_wait () from /lib64/libpthread.so.0
#1  0x03ff9718ed76 in sem_timedwait () from /lib64/libpthread.so.0
#2  0x02aa2d755868 in qemu_sem_timedwait (sem=0x2aa2e1fbfa8, ms=) at util/qemu-thread-posix.c:245
#3  0x02aa2d6803e4 in worker_thread (opaque=0x2aa2e1fbf40) at 
thread-pool.c:92
#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 6 (Thread 0x3ff9497f910 (LWP 52850)):
#0  0x03ff9718c50e in pthread_cond_wait@@GLIBC_2.3.2 () from 
/lib64/libpthread.so.0
#1  0x03ff96d19792 in g_cond_wait () from /lib64/libglib-2.0.so.0
#2  0x02aa2d7165d2 in wait_for_trace_records_available () at 
trace/simple.c:147
---Type  to continue, or q  to quit---
#3  writeout_thread (opaque=) at trace/simple.c:165
#4  0x03ff96cfa44c in g_thread_proxy () from /lib64/libglib-2.0.so.0
#5  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#6  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 5 (Thread 0x3ff8efff910 (LWP 52855)):
#0  0x03ff967f819a in ioctl () from /lib64/libc.so.6
#1  0x02aa2d546f3e in kvm_vcpu_ioctl (cpu=cpu@entry=0x2aa2e239030, 
type=type@entry=44672)
 at /usr/src/debug/qemu-2.5.50/kvm-all.c:1984
#2  0x02aa2d54701e in kvm_cpu_exec (cpu=0x2aa2e239030) at 
/usr/src/debug/qemu-2.5.50/kvm-all.c:1834
#3  0x02aa2d533cd6 in qemu_kvm_cpu_thread_fn (arg=) at 
/usr/src/debug/qemu-2.5.50/cpus.c:1056
#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 4 (Thread 0x3ff951ff910 (LWP 52849)):
#0  0x03ff967fcf56 in syscall () from /lib64/libc.so.6
#1  0x02aa2d755a36 in futex_wait (val=, ev=) 
at util/qemu-thread-posix.c:292
#2  qemu_event_wait (ev=0x2aa2ddb5914 ) at 
util/qemu-thread-posix.c:399
#3  0x02aa2d765002 in call_rcu_thread (opaque=) at 
util/rcu.c:250
#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6
---Type  to continue, or q  to quit---

Thread 3 (Thread 0x3ff978e0bf0 (LWP 52845)):
#0  0x03ff967f66e6 in ppoll () from /lib64/libc.so.6
#1  0x02aa2d68928e in ppoll (__ss=0x0, __timeout=0x0, __nfds=, 
__fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, timeout=-1) at 
qemu-timer.c:313
#3  0x02aa2d688b02 in os_host_main_loop_wait (timeout=-1) at main-loop.c:251
#4  main_loop_wait (nonblocking=) at main-loop.c:505
#5  0x02aa2d4faade in main_loop () at vl.c:1933
#6  main (argc=, argv=, envp=) at 
vl.c:4646

Thread 2 (Thread 0x3ff8910 (LWP 52851)):
#0  0x03ff967f66e6 in ppoll () from /lib64/libc.so.6
#1  0x02aa2d68928e in ppoll (__ss=0x0, __timeout=0x0, __nfds=, 
__fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, timeout=-1) at 
qemu-timer.c:313
#3  0x02aa2d68a788 in aio_poll (ctx=0x2aa2de77e00, blocking=) at aio-posix.c:453
#4  0x02aa2d5b909c in iothread_run (opaque=0x2aa2de77220) at iothread.c:46
#5  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#6  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 1 (Thread 0x3ff8f7ff910 (LWP 52854)):
#0  0x03ff9673b650 in raise () from /lib64/libc.so.6
---Type  to continue, or q  to quit---
#1  0x03ff9673ced8 in abort () from /lib64/libc.so.6
#2  0x03ff96733666 in __assert_fail_base () from /lib64/libc.so.6
#3  0x03ff967336f4 in __assert_fail () from /lib64/libc.so.6
#4  0x02aa2d562608 in virtio_blk_handle_output (vdev=, 
vq=)
 at /usr/src/debug/qemu-2.5.50/hw/block/virtio-blk.c:595


Hmm

Re: [Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race

[tu bo]

Hi Cornelia:

I saw qemu crash for this patch on qemu master yesterday.

However, scsi disks of my lpar is not available because of s38 firmware 
update.  I'll double check the test when it's ready. thx


On 03/29/2016 10:17 PM, Cornelia Huck wrote:

The ->set_host_notifier() callback is invoked whenever we want to
switch from or to the generic ioeventfd handler. Currently, all
transports deregister the ioeventfd backing and then re-register
it. This opens a race window where we are without ioeventfd
backing for a time period: In the virtio-blk dataplane case, we
observed notifications coming in from both the vcpu thread and
the iothread.

Let's change pci, mmio and ccw to keep the ioeventfd during
->set_host_notifier() and only switch the ioeventfd handler.

Signed-off-by: Cornelia Huck 
---
  hw/s390x/virtio-ccw.c  | 22 +-
  hw/virtio/virtio-mmio.c| 27 +--
  hw/virtio/virtio-pci.c | 28 ++--
  include/hw/virtio/virtio-bus.h |  4 
  4 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cb887ba..7b1088e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1196,14 +1196,26 @@ static bool 
virtio_ccw_query_guest_notifiers(DeviceState *d)
  static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign)
  {
  VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+VirtQueue *vq = virtio_get_queue(vdev, n);

-/* Stop using the generic ioeventfd, we are doing eventfd handling
- * ourselves below */
-dev->ioeventfd_disabled = assign;
  if (assign) {
-virtio_ccw_stop_ioeventfd(dev);
+/* Stop using the generic ioeventfd, we are doing eventfd handling
+ * ourselves below */
+dev->ioeventfd_disabled = true;
+};
+/*
+ * Just switch the handler, don't deassign the ioeventfd.
+ * Otherwise, there's a window where we don't have an
+ * ioeventfd and we may end up with a notification where
+ * we don't expect one.
+ */
+virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+if (!assign) {
+/* Use generic ioeventfd handler again. */
+dev->ioeventfd_disabled = false;
  }
-return virtio_ccw_set_guest2host_notifier(dev, n, assign, false);
+return 0;
  }

  static int virtio_ccw_get_mappings(VirtioCcwDevice *dev)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index d4cd91f..aafebdf 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -502,19 +502,26 @@ static int virtio_mmio_set_host_notifier(DeviceState 
*opaque, int n,
   bool assign)
  {
  VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+VirtQueue *vq = virtio_get_queue(vdev, n);

-/* Stop using ioeventfd for virtqueue kick if the device starts using host
- * notifiers.  This makes it easy to avoid stepping on each others' toes.
- */
-proxy->ioeventfd_disabled = assign;
  if (assign) {
-virtio_mmio_stop_ioeventfd(proxy);
+/* Stop using the generic ioeventfd, we are doing eventfd handling
+ * ourselves below */
+proxy->ioeventfd_disabled = true;
+};
+/*
+ * Just switch the handler, don't deassign the ioeventfd.
+ * Otherwise, there's a window where we don't have an
+ * ioeventfd and we may end up with a notification where
+ * we don't expect one.
+ */
+virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+if (!assign) {
+/* Use generic ioeventfd handler again. */
+proxy->ioeventfd_disabled = false;
  }
-/* We don't need to start here: it's not needed because backend
- * currently only stops on status change away from ok,
- * reset, vmstop and such. If we do add code to start here,
- * need to check vmstate, device state etc. */
-return virtio_mmio_set_host_notifier_internal(proxy, n, assign, false);
+return 0;
  }

  /* virtio-mmio device */
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 0dadb66..a91c1e8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1115,18 +1115,26 @@ static int virtio_pci_set_host_notifier(DeviceState *d, 
int n, bool assign)
  {
  VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);

-/* Stop using ioeventfd for virtqueue kick if the device starts using host
- * notifiers.  This makes it easy to avoid stepping on each others' toes.
- */
-proxy->ioeventfd_disabled = assign;
+VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+VirtQueue *vq = virtio_get_queue(vdev, n);
+
  if (assign) {
-virtio_pci_stop_ioeventfd(proxy);
+/* Stop using the generic ioeventfd, we are doing eventfd handling
+ * ourselves below */
+pro

Re: [Qemu-devel] [PATCH 0/6] virtio: refactor host notifiers

[tu bo]

Hi Christian:

On 03/30/2016 12:27 AM, Christian Borntraeger wrote:

On 03/29/2016 03:50 PM, Paolo Bonzini wrote:



On 29/03/2016 13:45, Cornelia Huck wrote:

Hi Tu Bo,

please always include the assertion patch at
https://lists.gnu.org/archive/html/qemu-block/2016-03/msg00546.html in
your tests.  Can you include the backtrace from all threads with that patch?


thanks for your reminder about the assertion patch. Here is the
backtrace with qemu master + assertion patch + "[PATCH 0/6] virtio:
refactor host notifiers",


FWIW, I've been running this in a reboot loop for the last 2 1/2 hours.
Could you perhaps share your command line?


 From code inspection, the following is also necessary or at least a
good idea:

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 6fb29e3..7fa8477 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -258,7 +258,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
  aio_context_acquire(s->ctx);

  /* Stop notifications for new requests from guest */
-virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
+virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false);

  /* Drain and switch bs back to the QEMU main loop */
  blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());



Right. Tu Bo, you seem to have the best testcase for this.
Does your setup runs fine with this on top?


My test needs at least four scsi disks, which was broken now because of 
the s38 firmware update. I'll test it when s38 scsi is ready. thx




CHristian

Re: [Qemu-devel] [PATCH 0/2] dataplane: fix start/stop races

[tu bo]

Hi Christian:

On 03/30/2016 12:31 AM, Christian Borntraeger wrote:

On 03/29/2016 03:42 PM, Michael S. Tsirkin wrote:

This works around races that data plane introduces
simply by exiting immediately if we detect
that dataplane is active.

It's a small but ugly patch, it's only justification
is that it's minimally intrusive, and that it clearly
has no chance to break non data plane users.

The idea is to rework it all post 2.6.

Michael S. Tsirkin (2):
   virtio: add aio handler
   virtio-blk: use aio handler for data plane

  include/hw/virtio/virtio-blk.h  |  2 ++
  include/hw/virtio/virtio.h  |  4 
  hw/block/dataplane/virtio-blk.c | 13 +
  hw/block/virtio-blk.c   | 28 ++--
  hw/virtio/virtio.c  | 36 
  5 files changed, 69 insertions(+), 14 deletions(-)



This also seems to help on my setup.Tu Bo, would be good if you
can double check this patch set as well on your setup?


With qemu master +  [PATCH 0/2] dataplane: fix start/stop races,

I did NOT see any crash, result is good in my box. thanks



Christian

Re: [Qemu-devel] [PATCH 0/6] virtio: refactor host notifiers

[tu bo]

Hi Paolo:

On 03/29/2016 02:11 AM, Paolo Bonzini wrote:

On 28/03/2016 05:55, TU BO wrote:

Hi Cornelia:

I got two crash with qemu master + "[PATCH 0/6] virtio: refactor host
notifiers",


Hi Tu Bo,

please always include the assertion patch at
https://lists.gnu.org/archive/html/qemu-block/2016-03/msg00546.html in
your tests.  Can you include the backtrace from all threads with that patch?

thanks for your reminder about the assertion patch. Here is the 
backtrace with qemu master + assertion patch + "[PATCH 0/6] virtio: 
refactor host notifiers",


I got two crashes,

1. For 1st crash,
(gdb) thread apply all bt

Thread 8 (Thread 0x3ff8daf1910 (LWP 52859)):
#0  0x03ff9718ec62 in do_futex_timed_wait () from /lib64/libpthread.so.0
#1  0x03ff9718ed76 in sem_timedwait () from /lib64/libpthread.so.0
#2  0x02aa2d755868 in qemu_sem_timedwait (sem=0x3ff88000fa8, 
ms=) at util/qemu-thread-posix.c:245
#3  0x02aa2d6803e4 in worker_thread (opaque=0x3ff88000f40) at 
thread-pool.c:92

#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 7 (Thread 0x3ff8e679910 (LWP 52856)):
#0  0x03ff9718ec62 in do_futex_timed_wait () from /lib64/libpthread.so.0
#1  0x03ff9718ed76 in sem_timedwait () from /lib64/libpthread.so.0
#2  0x02aa2d755868 in qemu_sem_timedwait (sem=0x2aa2e1fbfa8, 
ms=) at util/qemu-thread-posix.c:245
#3  0x02aa2d6803e4 in worker_thread (opaque=0x2aa2e1fbf40) at 
thread-pool.c:92

#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 6 (Thread 0x3ff9497f910 (LWP 52850)):
#0  0x03ff9718c50e in pthread_cond_wait@@GLIBC_2.3.2 () from 
/lib64/libpthread.so.0

#1  0x03ff96d19792 in g_cond_wait () from /lib64/libglib-2.0.so.0
#2  0x02aa2d7165d2 in wait_for_trace_records_available () at 
trace/simple.c:147

---Type  to continue, or q  to quit---
#3  writeout_thread (opaque=) at trace/simple.c:165
#4  0x03ff96cfa44c in g_thread_proxy () from /lib64/libglib-2.0.so.0
#5  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#6  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 5 (Thread 0x3ff8efff910 (LWP 52855)):
#0  0x03ff967f819a in ioctl () from /lib64/libc.so.6
#1  0x02aa2d546f3e in kvm_vcpu_ioctl (cpu=cpu@entry=0x2aa2e239030, 
type=type@entry=44672)

at /usr/src/debug/qemu-2.5.50/kvm-all.c:1984
#2  0x02aa2d54701e in kvm_cpu_exec (cpu=0x2aa2e239030) at 
/usr/src/debug/qemu-2.5.50/kvm-all.c:1834
#3  0x02aa2d533cd6 in qemu_kvm_cpu_thread_fn (arg=) 
at /usr/src/debug/qemu-2.5.50/cpus.c:1056

#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 4 (Thread 0x3ff951ff910 (LWP 52849)):
#0  0x03ff967fcf56 in syscall () from /lib64/libc.so.6
#1  0x02aa2d755a36 in futex_wait (val=, ev=out>) at util/qemu-thread-posix.c:292
#2  qemu_event_wait (ev=0x2aa2ddb5914 ) at 
util/qemu-thread-posix.c:399
#3  0x02aa2d765002 in call_rcu_thread (opaque=) at 
util/rcu.c:250

#4  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6
---Type  to continue, or q  to quit---

Thread 3 (Thread 0x3ff978e0bf0 (LWP 52845)):
#0  0x03ff967f66e6 in ppoll () from /lib64/libc.so.6
#1  0x02aa2d68928e in ppoll (__ss=0x0, __timeout=0x0, 
__nfds=, __fds=) at 
/usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, timeout=-1) 
at qemu-timer.c:313
#3  0x02aa2d688b02 in os_host_main_loop_wait (timeout=-1) at 
main-loop.c:251

#4  main_loop_wait (nonblocking=) at main-loop.c:505
#5  0x02aa2d4faade in main_loop () at vl.c:1933
#6  main (argc=, argv=, envp=out>) at vl.c:4646


Thread 2 (Thread 0x3ff8910 (LWP 52851)):
#0  0x03ff967f66e6 in ppoll () from /lib64/libc.so.6
#1  0x02aa2d68928e in ppoll (__ss=0x0, __timeout=0x0, 
__nfds=, __fds=) at 
/usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, timeout=-1) 
at qemu-timer.c:313
#3  0x02aa2d68a788 in aio_poll (ctx=0x2aa2de77e00, 
blocking=) at aio-posix.c:453
#4  0x02aa2d5b909c in iothread_run (opaque=0x2aa2de77220) at 
iothread.c:46

#5  0x03ff971884c6 in start_thread () from /lib64/libpthread.so.0
#6  0x03ff96802ec2 in thread_start () from /lib64/libc.so.6

Thread 1 (Thread 0x3ff8f7ff910 (LWP 52854)):
#0  0x03ff9673b650 in raise () from /lib64/libc.so.6
---Type  to continue, or q  to quit---
#1  0x03ff9673ced8 in abort () from /lib64/libc.so.6
#2  0x03ff96733666 in __assert_fail_base () from /lib64/libc.so.6
#3  0x03ff967336f4 in __assert_fail () from /lib64/libc.so.6
#4  0x02aa2d562608 in virtio_blk_handle_output (vdev=out>, vq=)

at /usr/src/debug/qemu-2.5.50/hw/block/virtio-blk.c:595
#5  0x02aa2d587464 in virtio_ccw_hcall_notify (args=) 
at /usr/src/de

Re: [Qemu-devel] [PATCH 0/6] virtio: refactor host notifiers

[TU BO]

Hi Cornelia:

I got two crash with qemu master + "[PATCH 0/6] virtio: refactor host 
notifiers",


I can get first crash very often.
(gdb) bt
#0  blk_aio_read_entry (opaque=0x0) at block/block-backend.c:922
#1  0x02aa17a65f0e in coroutine_trampoline (i0=, 
i1=-1677713216) at util/coroutine-ucontext.c:78

#2  0x03ffabfd150a in __makecontext_ret () from /lib64/libc.so.6
(gdb) list
917static void blk_aio_read_entry(void *opaque)
918{
919BlkAioEmAIOCB *acb = opaque;
920BlkRwCo *rwco = &acb->rwco;
921
922rwco->ret = blk_co_preadv(rwco->blk, rwco->offset, 
rwco->qiov->size,

923  rwco->qiov, rwco->flags);
924blk_aio_complete(acb);
925}


For 2nd crash,  I just saw it once, and didn't reproduce it later.
(gdb) bt
#0  ioq_submit (s=s@entry=0x2aa3fe2cc80) at block/linux-aio.c:197
#1  0x02aa3f645b36 in qemu_laio_completion_bh (opaque=0x2aa3fe2cc80) 
at block/linux-aio.c:143

#2  0x02aa3f5ffbf0 in aio_bh_call (bh=) at async.c:65
#3  aio_bh_poll (ctx=0x2aa3fdf7e00) at async.c:93
#4  0x02aa3f60a51e in aio_dispatch (ctx=ctx@entry=0x2aa3fdf7e00) at 
aio-posix.c:306
#5  0x02aa3f60a7ca in aio_poll (ctx=0x2aa3fdf7e00, 
blocking=) at aio-posix.c:475
#6  0x02aa3f53903c in iothread_run (opaque=0x2aa3fdf7220) at 
iothread.c:46

#7  0x03ffa86084c6 in start_thread () from /lib64/libpthread.so.0
#8  0x03ffa7c82ec2 in thread_start () from /lib64/libc.so.6
(gdb) list
192struct iocb *iocbs[MAX_QUEUED_IO];
193QSIMPLEQ_HEAD(, qemu_laiocb) completed;
194
195do {
196len = 0;
197QSIMPLEQ_FOREACH(aiocb, &s->io_q.pending, next) {
198iocbs[len++] = &aiocb->iocb;
199if (len == MAX_QUEUED_IO) {
200break;
201}


On 16/3/25 上午12:15, Cornelia Huck wrote:

Here's the next version of my refactoring of the virtio host notifiers.
This one actually survives a bit of testing for me (reboot loop).

As this patchset fixes a latent bug exposed by the recent dataplane
changes (we have a deassigned ioeventfd for a short period of time
during dataplane start, which leads to the virtqueue handler being
called in both the vcpu thread and the iothread simultaneously), I'd
like to see this in 2.6.

Changes from RFC:
- Fixed some silly errors (checking for !disabled instead of disabled,
   virtio_ccw_stop_ioeventfd() calling virtio_bus_start_ioeventfd()).
- Completely reworked set_host_notifier(): We only want to set/unset
   the actual handler function and don't want to do anything to the
   ioeventfd backing, so reduce the function to actually doing only
   that.
- With the change above, we can lose the 'assign' parameter in
   virtio_bus_stop_ioeventfd() again.
- Added more comments that hopefully make it clearer what is going on.

I'd appreciate it if people could give it some testing; I'll be back
to look at the fallout after Easter.

Cornelia Huck (6):
   virtio-bus: common ioeventfd infrastructure
   virtio-bus: have callers tolerate new host notifier api
   virtio-ccw: convert to ioeventfd callbacks
   virtio-pci: convert to ioeventfd callbacks
   virtio-mmio: convert to ioeventfd callbacks
   virtio-bus: remove old set_host_notifier callback

  hw/block/dataplane/virtio-blk.c |   6 +-
  hw/s390x/virtio-ccw.c   | 133 ++--
  hw/scsi/virtio-scsi-dataplane.c |   9 ++-
  hw/virtio/vhost.c   |  13 ++--
  hw/virtio/virtio-bus.c  | 132 +++
  hw/virtio/virtio-mmio.c | 128 +-
  hw/virtio/virtio-pci.c  | 124 +
  include/hw/virtio/virtio-bus.h  |  31 +-
  8 files changed, 303 insertions(+), 273 deletions(-)

Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH

[tu bo]

Hi Christian:

On 03/23/2016 05:12 PM, Christian Borntraeger wrote:

On 03/23/2016 10:08 AM, Paolo Bonzini wrote:



On 23/03/2016 09:10, Cornelia Huck wrote:

-/* Kick right away to begin processing requests already in vring */
-event_notifier_set(virtio_queue_get_host_notifier(s->vq));
+vblk->dataplane_started = true;

-/* Get this show started by hooking up our callbacks */
+/* Get this show started by hooking up our callbacks.  */
  aio_context_acquire(s->ctx);
  virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
  aio_context_release(s->ctx);
+atomic_dec(&s->starting);
+
+/* Kick right away to begin processing requests already in vring */
+event_notifier_set(virtio_queue_get_host_notifier(s->vq));


I'm wondering whether moving this event_notifier_set() masks something?
IOW, may we run into trouble if the event notifier is set from some
other path before the callbacks are set up properly?


The reentrancy check should catch that...  But:

1) the patch really makes no difference, your fix is enough for me



Tu Bo, can you test with master + Cornelias 6 refactoring patches and nothing 
on top?


With qemu master + Cornelia's 6 refactoring patches and nothing on top, 
I did NOT see crash so far.







2) vblk->dataplane_started becomes true before the callbacks are set;
that should be enough.

3) this matches what I tested, but it would of course be better if the
assertions on s->starting suffice


-if (!vblk->dataplane_started || s->stopping) {
+if (!vblk->dataplane_started) {


No fear of reentrancy here?


No, because this is only invoked from reset, hence only from the CPU
thread and only under the BQL.

On start, reentrancy happens between iothread (running outside BQL) and
a CPU thread (running within BQL).

Paolo


  return;
  }

@@ -255,7 +251,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
  vblk->dataplane_started = false;
  return;
  }
-s->stopping = true;
+
  trace_virtio_blk_data_plane_stop(s);

  aio_context_acquire(s->ctx);
@@ -274,5 +270,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
  k->set_guest_notifiers(qbus->parent, 1, false);

  vblk->dataplane_started = false;
-s->stopping = false;
  }

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

Hi Fam:

On 03/21/2016 06:57 PM, Fam Zheng wrote:

On Thu, 03/17 19:03, tu bo wrote:


On 03/17/2016 08:39 AM, Fam Zheng wrote:

On Wed, 03/16 14:45, Paolo Bonzini wrote:



On 16/03/2016 14:38, Christian Borntraeger wrote:

If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


Great, I'll prepare a patch to virtio then sketching the solution that
Conny agreed with.

While Fam and I agreed that patch 1 is not required, I'm not sure if the
mutex is necessary in the end.


If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
to have it. I'm not sure about the BH.

And on a hindsight I realize we don't want patches 2-3 too. Actually the
begin/end pair won't work as expected because of the blk_set_aio_context.

Let's hold on this series.



So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
and both with/without Fam's patches, it would be great.


Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.


1. without the virtio_queue_host_notifier_read calls,  without patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa165da37e in coroutine_trampoline (i0=,
i1=1812051552) at util/coroutine-ucontext.c:79
#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6


2. without the virtio_queue_host_notifier_read calls, with patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa39dda43e in coroutine_trampoline (i0=,
i1=-1677715600) at util/coroutine-ucontext.c:79
#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6




Tu Bo,

Could you help test this patch (on top of master, without patch 4)?

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..47f8043 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)

  void virtio_queue_notify(VirtIODevice *vdev, int n)
  {
-virtio_queue_notify_vq(&vdev->vq[n]);
+VirtQueue *vq = &vdev->vq[n];
+EventNotifier *n;
+n = virtio_queue_get_host_notifier(vq);
+if (n) {
+event_notifier_set(n);
+} else {
+virtio_queue_notify_vq(vq);
+}
  }

  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)




I got a build error as below,

/BUILD/qemu-2.5.50/hw/virtio/virtio.c: In function 'virtio_queue_notify':
/BUILD/qemu-2.5.50/hw/virtio/virtio.c:1102:20: error: 'n' redeclared as 
different kind of symbol

 EventNotifier *n;
^
/BUILD/qemu-2.5.50/hw/virtio/virtio.c:1099:50: note: previous definition 
of 'n' was here

 void virtio_queue_notify(VirtIODevice *vdev, int n)


Then I did some change for your patch as below,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..a10da39 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)

 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
-virtio_queue_notify_vq(&vdev->vq[n]);
+VirtQueue *vq = &vdev->vq[n];
+EventNotifier *en;
+en = virtio_queue_get_host_notifier(vq);
+if (en) {
+event_notifier_set(en);
+} else {
+virtio_queue_notify_vq(vq);
+}
 }

 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)

With qemu master + modified patch above(without patch 4, without Conny's 
patches), I did NOT get crash so far.  thanks

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[TU BO]

On 16/3/18 下午11:03, Paolo Bonzini wrote:


On 17/03/2016 17:08, Christian Borntraeger wrote:

Good (or bad?) news is the assert also triggers on F23, it just seems
to take longer.

I guess good news, because we can rule out the kernel (not that I
believed it was a kernel problem, but the thought is always there in
the background...).

The interaction between ioeventfd and dataplane is too complicated.  I
think if we get rid of the start/stop ioeventfd calls (just set up the
ioeventfd as soon as possible and then only set/clear the handlers)
things would be much simpler.

I'll see if I can produce something based on Conny's patches, which are
already a start.  Today I had a short day so I couldn't play with the
bug; out of curiosity, does the bug reproduce with her work + patch 4
from this series + the reentrancy assertion?
I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor 
host notifiers" from Conny + patch 4 + assertion.  thx


Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

On 03/17/2016 08:39 AM, Fam Zheng wrote:

On Wed, 03/16 14:45, Paolo Bonzini wrote:



On 16/03/2016 14:38, Christian Borntraeger wrote:

If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


Great, I'll prepare a patch to virtio then sketching the solution that
Conny agreed with.

While Fam and I agreed that patch 1 is not required, I'm not sure if the
mutex is necessary in the end.


If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
to have it. I'm not sure about the BH.

And on a hindsight I realize we don't want patches 2-3 too. Actually the
begin/end pair won't work as expected because of the blk_set_aio_context.

Let's hold on this series.



So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
and both with/without Fam's patches, it would be great.


Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.


1. without the virtio_queue_host_notifier_read calls,  without patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa165da37e in coroutine_trampoline (i0=, 
i1=1812051552) at util/coroutine-ucontext.c:79

#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6


2. without the virtio_queue_host_notifier_read calls, with patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa39dda43e in coroutine_trampoline (i0=, 
i1=-1677715600) at util/coroutine-ucontext.c:79

#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6



Thanks,
Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

On 03/16/2016 09:38 PM, Christian Borntraeger wrote:

On 03/16/2016 01:55 PM, Paolo Bonzini wrote:



On 16/03/2016 12:24, Christian Borntraeger wrote:

On 03/16/2016 12:09 PM, Paolo Bonzini wrote:

On 16/03/2016 11:49, Christian Borntraeger wrote:

#3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
/home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
#4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
#5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
#6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785


If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


remove the calls to virtio_queue_host_notifier_read, and keeping patches 
2-4 in,


I got same crash as before,
(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa0c65d786 in coroutine_trampoline (i0=, 
i1=-2013204784) at util/coroutine-ucontext.c:79

#2  0x03ff99ad150a in __makecontext_ret () from /lib64/libc.so.6






Paolo


#7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
(vq=0xba305270, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
#8  0x80109c50 in virtio_ccw_set_guest2host_notifier (dev=0xb9eed6a0, 
n=0, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
#9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154

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

[tu bo]

On 03/15/2016 08:45 PM, Fam Zheng wrote:

On Fri, 03/11 11:28, Paolo Bonzini wrote:



On 10/03/2016 10:40, Christian Borntraeger wrote:

On 03/10/2016 10:03 AM, Christian Borntraeger wrote:

On 03/10/2016 02:51 AM, Fam Zheng wrote:
[...]

The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
suspicious:

main thread  iothread

 virtio_blk_handle_output()
  virtio_blk_data_plane_start()
   vblk->dataplane_started = true;
   blk_set_aio_context()
bdrv_set_aio_context()
 bdrv_drain()
  aio_poll()
   
virtio_blk_handle_output()
 /* s->dataplane_started is true */
!!!   ->virtio_blk_handle_request()
  event_notifier_set(ioeventfd)
 aio_poll()
  
virtio_blk_handle_request()

Christian, could you try the followed patch? The aio_poll above is replaced
with a "limited aio_poll" that doesn't disptach ioeventfd.

(Note: perhaps moving "vblk->dataplane_started = true;" after
blk_set_aio_context() also *works around* this.)

---

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,

  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
  {
-bdrv_drain(bs); /* ensure there are no in-flight requests */
+/* ensure there are no in-flight requests */
+bdrv_drained_begin(bs);
+bdrv_drained_end(bs);

  bdrv_detach_aio_context(bs);



That seems to do the trick.


Or not. Crashed again :-(


I would put bdrv_drained_end just before aio_context_release.


This won't work. bdrv_drained_end must be called with the same ctx as
bdrv_drained_begin, which is only true before bdrv_detach_aio_context().



But secondarily, I'm thinking of making the logic simpler to understand
in two ways:

1) adding a mutex around virtio_blk_data_plane_start/stop.

2) moving

 event_notifier_set(virtio_queue_get_host_notifier(s->vq));
 virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);

to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
true starts the processing.


Like this? If it captures your idea, could Bo or Christian help test?




With this patch, I still can get qemu crash as before,
(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa17f5a4a6 in coroutine_trampoline (i0=, 
i1=-1677707808) at util/coroutine-ucontext.c:79

#2  0x03ffac25150a in __makecontext_ret () from /lib64/libc.so.6


Good news is that frequency of qemu crash is much less that before.


---


 From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
From: Fam Zheng 
Date: Thu, 10 Mar 2016 10:26:36 +0800
Subject: [PATCH] virtio-blk dataplane start crash fix

Suggested-by: Paolo Bonzini 
Signed-off-by: Fam Zheng 
---
  block.c |  4 +++-
  hw/block/dataplane/virtio-blk.c | 39 ---
  2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,

  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
  {
-bdrv_drain(bs); /* ensure there are no in-flight requests */
+/* ensure there are no in-flight requests */
+bdrv_drained_begin(bs);
+bdrv_drained_end(bs);

  bdrv_detach_aio_context(bs);

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 36f3d2b..6db5c22 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {

  /* Operation blocker on BDS */
  Error *blocker;
+
+QemuMutex start_lock;
  };

  /* Raise an interrupt to signal guest, if necessary */
@@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, 
VirtIOBlkConf *conf,
  s = g_new0(VirtIOBlockDataPlane, 1);
  s->vdev = vdev;
  s->conf = conf;
+qemu_mutex_init(&s->start_lock);

  if (conf->iothread) {
  s->iothread = conf->iothread;
@@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane 
*s)
  g_free(s);
  }

+typedef struct {
+VirtIOBlockDataPlane *s;
+QEMUBH *bh;
+} VirtIOBlockStartData;
+
+static void virtio_blk_data_plane_start_bh_cb(void *opaque)
+{
+VirtIOBlockStartData *data = opaque;
+VirtIOBlockDataPlane *s = data->s;
+
+/* Kick right away to begin processing requests already in vring */
+event_notifier_set(virtio_queue_get_host_notifier(s->vq));
+
+/* Get this show started by hooking up our callbacks */
+virti

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

[tu bo]

Using the latest qemu from master, and got a new qemu crash as below,

(gdb) bt
#0  0x03ffabb3b650 in raise () from /lib64/libc.so.6
#1  0x03ffabb3ced8 in abort () from /lib64/libc.so.6
#2  0x10384c30 in qemu_coroutine_enter (co=0x10a2ed40, 
opaque=0x0) at util/qemu-coroutine.c:112
#3  0x102fd5c2 in bdrv_co_io_em_complete (opaque=0x3ff22beb518, 
ret=0) at block/io.c:2311
#4  0x102f1428 in qemu_laio_process_completion (s=0x10a25e30, 
laiocb=0x3ffa400a2a0) at block/linux-aio.c:92
#5  0x102f15e8 in qemu_laio_completion_bh (opaque=0x10a25e30) at 
block/linux-aio.c:139

#6  0x10281d70 in aio_bh_call (bh=0x109e3580) at async.c:65
#7  0x10281eb8 in aio_bh_poll (ctx=0x109efe10) at async.c:93
#8  0x1029538e in aio_dispatch (ctx=0x109efe10) at aio-posix.c:306
#9  0x10295da6 in aio_poll (ctx=0x109efe10, blocking=false) at 
aio-posix.c:475

#10 0x1014662e in iothread_run (opaque=0x109ef8d0) at iothread.c:46
#11 0x03ffabd084c6 in start_thread () from /lib64/libpthread.so.0
#12 0x03ffabc02ec2 in thread_start () from /lib64/libc.so.6
(gdb) frame 2
#2  0x10384c30 in qemu_coroutine_enter (co=0x10a2ed40, 
opaque=0x0) at util/qemu-coroutine.c:112

112 abort();
(gdb) list
107 
108 trace_qemu_coroutine_enter(self, co, opaque);
109 
110 if (co->caller) {
111 fprintf(stderr, "Co-routine re-entered recursively\n");
112 abort();
113 }
114 
115 co->caller = self;
116 co->entry_arg = opaque;


Messages in the log file of "/var/log/libvirt/qemu/" as below,
LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin 
QEMU_AUDIO_DRV=none /usr/bin/qemu-kvm -name rt_vm2 -S -machine 
s390-ccw-virtio-2.6,accel=kvm,usb=off -m 2048 -realtime mlock=off -smp 
2,sockets=2,cores=1,threads=1 -object iothread,id=iothread1 -uuid 
80cfa525-b35b-4341-aa20-a581bb528fbf -nographic -no-user-config 
-nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-rt_vm2/monitor.sock,server,nowait 
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc 
-no-shutdown -boot strict=on -drive 
file=/dev/mapper/36005076305ffc1ae8036,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native 
-device 
virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 
-netdev tap,fd=27,id=hostnet0,vhost=on,vhostfd=29 -device 
virtio-net-ccw,netdev=hostnet0,id=net0,mac=02:e7:24:dc:dc:11,devno=fe.0. 
-netdev tap,fd=30,id=hostnet1,vhost=on,vhostfd=31 -device 
virtio-net-ccw,netdev=hostnet1,id=net1,mac=52:54:00:e3:0a:44,devno=fe.0.0002 
-chardev pty,id=charconsole0 -device 
sclpconsole,chardev=charconsole0,id=console0 -device 
virtio-balloon-ccw,id=balloon0,devno=fe.3.ffba -msg timestamp=on

char device redirected to /dev/pts/6 (label charconsole0)

Co-routine re-entered recursively
2016-03-14 09:05:37.075+: shutting down


On 03/11/2016 06:28 PM, Paolo Bonzini wrote:



On 10/03/2016 10:40, Christian Borntraeger wrote:

On 03/10/2016 10:03 AM, Christian Borntraeger wrote:

On 03/10/2016 02:51 AM, Fam Zheng wrote:
[...]

The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
suspicious:

main thread  iothread

 virtio_blk_handle_output()
  virtio_blk_data_plane_start()
   vblk->dataplane_started = true;
   blk_set_aio_context()
bdrv_set_aio_context()
 bdrv_drain()
  aio_poll()
   
virtio_blk_handle_output()
 /* s->dataplane_started is true */
!!!   ->virtio_blk_handle_request()
  event_notifier_set(ioeventfd)
 aio_poll()
  
virtio_blk_handle_request()

Christian, could you try the followed patch? The aio_poll above is replaced
with a "limited aio_poll" that doesn't disptach ioeventfd.

(Note: perhaps moving "vblk->dataplane_started = true;" after
blk_set_aio_context() also *works around* this.)

---

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,

  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
  {
-bdrv_drain(bs); /* ensure there are no in-flight requests */
+/* ensure there are no in-flight requests */
+bdrv_drained_begin(bs);
+bdrv_drained_end(bs);

  bdrv_detach_aio_context(bs);



That seems to do the trick.


Or not. Crashed again :-(


I would put bdrv_drained_end just before aio_context_release.

But secondarily, I'm thinking of making the logic simpler to understand
in two ways:

1) adding a mutex around virtio_blk_data_plane_start/stop.

2) moving

 event_notifier_set(virtio_queue_get_host_n

Re: [Qemu-devel] [PATCH v3 2/3] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max:

On 12/02/2015 11:48 PM, Max Reitz wrote:

On 01.12.2015 08:35, tu bo wrote:

Hi Max:

在 2015/12/1 3:38, Max Reitz 写道:

On 26.11.2015 10:53, Bo Tu wrote:

From: Bo Tu 

The tests for device type "ide_cd" should only be tested for the pc
platform.
The default device id of hard disk on the s390 platform differs to that
of the x86 platform. A new variable device_id is defined and "virtio0"
set for the s390 platform. A x86 platform specific output file is also
needed.
Warning message expected for s390x when drive without device.

Reviewed-by: Max Reitz 


I can't remember having given this for this patch. ;-)


It's my mistake, please forgive me for it.


No problem.


Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
   tests/qemu-iotests/051  |  99 ++
   tests/qemu-iotests/051.out  | 422

   tests/qemu-iotests/051.pc.out   | 422

   tests/qemu-iotests/051.s390.out | 335 +++
   tests/qemu-iotests/Makefile |   7 +-
   5 files changed, 828 insertions(+), 457 deletions(-)
   delete mode 100644 tests/qemu-iotests/051.out
   create mode 100644 tests/qemu-iotests/051.pc.out
   create mode 100644 tests/qemu-iotests/051.s390.out


I didn't even know we had a Makefile in tests/qemu-iotests. I don't
think it is invoked automatically, and if it was, it should not modify
the source tree. For instance, I have a separate build directory so my
source tree remains clean.

I don't think expecting the user to invoke the Makefile manually is a
good idea either. When I said "copy" I literally meant adding a copy of
051.s390.out with this patch (even though it is ugly, I'd still consider
it better).


Adding a copy of 051.s390.out is not perfect, but acceptable.



Anyway, we can circumvent the whole issue anyway. While 051 does test
some devices explicitly which are only available on the PC platform, the
thing in question which would require a s390-specific output, too, is
the final part of the test ("=== Snapshot mode ===").

You can just drop the "case" introduced by this patch and set device_id
to whatever you want (e.g. "drive0" or "none0"). Then, you replace every
"-drive file..." by "-drive if=none,id=$device_id,file=..." and you're
done (obviously, the reference output needs to be adjusted for the
changed drive ID).

Then, you don't need an 051.s390.out at all, and can just make that the
common output (051.out).


I got your idea. we can get the common output(051.out) for the final
part of the test ("=== Snapshot mode ===") with this solution.

However, other parts of this test has different outputs as below,
1. s390x has no output for some test commands for "=== No medium ===",
"=== Read-only ===" and "=== Cache modes ===". for example, "line 204 -
209" has output for pc, but no output for s390x.

 202 case "$QEMU_DEFAULT_MACHINE" in
 203 pc)
 204 run_qemu -drive media=cdrom,cache=none
 205 run_qemu -drive media=cdrom,cache=directsync
 206 run_qemu -drive media=cdrom,cache=writeback
 207 run_qemu -drive media=cdrom,cache=writethrough
 208 run_qemu -drive media=cdrom,cache=unsafe
 209 run_qemu -drive media=cdrom,cache=invalid_value
 210 ;;
 211  *)
 212 ;;
 213 esac


Yes, indeed. However, it has this output only for pc, but it will not
have any output for any other platform type. Therefore, we don't need an
s390-specific reference output, but only one reference output for pc
(051.pc.out) and one for all the other platforms (051.out).


2. s390x has "Warning: Orphaned drive without device:" for
run_qemu -drive file="$TEST_IMG",if=scsi,readonly=on
run_qemu -drive if=scsi

Please refer
http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg04836.html.


Yes, and that's exactly the reason why I advocated filtering out that
line because we don't know whether it is visible on any platform but s390.

(e.g. see the _filter_orphan added in
http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05992.html)

Even if we decide against adding such a filter I think we should put the
s390 output into the common 051.out file anyway. If someone tries to run
the tests on a non-s390 and non-pc platform and does not get this
warning, thus failing the test, we can still talk about adding that filter.


I prefer to keep the warning in 051.out.




I plan to change the final part of the test("=== Snapshot mode ===")
with your solution, and adding a copy of 051.s390.out. If this is fine
to you, I plan to send a new version of patch for review asap, since
Christmas is coming :-)


I'm fine with patches after christmas, too. ;-)

If you change

Re: [Qemu-devel] [PATCH v2 2/4] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max:

On 11/25/2015 11:41 PM, Max Reitz wrote:

On 24.11.2015 22:17, Sascha Silbe wrote:

This PC/s390x-only hunk looks like an oversight to me.


Not really, see
http://lists.nongnu.org/archive/html/qemu-devel/2015-02/msg01906.html
and
http://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg02851.html

I noticed, but I am fine with it since the tests probably won't run on
anything but x86/pc and s390 anyway (without modifications; most of the
changes this series is making to make the iotests work on s390 are
necessary for other non-pc platforms as well, and that shows to me that
apparently nobody tried to run the iotests on non-pc platforms before
s390, or didn't care enough about them to fix them).


We should make
one of the options the default. I'd prefer defaulting to virtio (see
below), but since the test previously hard-coded IDE that would be fine,
too.


In my first reply above, I noted that virtio0 may not be available on
all platforms either. Therefore, I'd rather have an explicit list of
platforms there than an asterisk where it does not belong.

However, my second reply above spawned a bit of a discussion, where
Kevin simply proposed to change the ID of the drive to something known,
i.e. just set the ID by adding an id=drive0 or something to the -drive
parameter.

Thanks for reminding me of the above, I had already forgotten. Indeed,
we should just add id=drive0 to the -drive parameter and use drive0. A
similar solution may be possible in most other places as well where PC
and s390 differ due to the names of the default devices available.


thanks for the reminder :-)

Yes, Kevin mentioned that we can use "id=testdisk" because it's the same 
on all platforms. Please refer this link:

http://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03715.html

For test 130, I used "qemu -drive id=testdisk" for both pc and s390x.
For test 051, I didn't find a way to do the same thing for qemu-io.

Re: [Qemu-devel] [PATCH v2 2/4] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max, Sascha:

On 11/21/2015 12:24 AM, Max Reitz wrote:

On 19.11.2015 08:28, tu bo wrote:

Hi Max:

On 11/19/2015 12:52 AM, Max Reitz wrote:

On 04.11.2015 03:26, Bo Tu wrote:

The tests for device type "ide_cd" should only be tested for the pc
platform.
The default device id of hard disk on the s390 platform differs to that
of the x86 platform. A new variable device_id is defined and "virtio0"
set for the s390 platform. A x86 platform specific output file is also
needed.
Warning message expected for s390x when drive without device.

Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
   tests/qemu-iotests/051|  99 ++
   tests/qemu-iotests/051.out| 143 +++---
   tests/qemu-iotests/051.pc.out | 422
++
   3 files changed, 515 insertions(+), 149 deletions(-)
   create mode 100644 tests/qemu-iotests/051.pc.out



[...]


diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7765aa0..d17c969 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out


[...]


@@ -109,20 +109,23 @@ QEMU X.Y.Z monitor - type 'help' for more
information

   Testing: -drive if=floppy
   QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) qququiquit
+(qemu) Warning: Orphaned drive without device:
id=floppy0,file=,if=floppy,bus=0,unit=0
+qququiquit


I'd still like these warnings to be filtered out (as I wrote in my reply
to the original RFC's v4, and as was done in later versions of that RFC
(the _filter_orphan function e.g. in
http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg04816.html).



I did more investigation about this.
"Warning: Orphaned drive without device:.*"  is expected for s390x, when
we define a drive without device, also this drive is not default
and interface is not "none".  Please refer line 228,229 in blockdev.c:
 218 bool drive_check_orphaned(void)
 219 {
 220 BlockBackend *blk;
 221 DriveInfo *dinfo;
 222 bool rs = false;
 223
 224 for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
 225 dinfo = blk_legacy_dinfo(blk);
 226 /* If dinfo->bdrv->dev is NULL, it has no device
attached. */
 227 /* Unless this is a default drive, this may be an
oversight. */
 228 if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
 229 dinfo->type != IF_NONE) {
 230 fprintf(stderr, "Warning: Orphaned drive without
device: "
 231 "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
 232 blk_name(blk), blk_bs(blk)->filename,
if_name[dinfo->type],
 233 dinfo->bus, dinfo->unit);
 234 rs = true;
 235 }
 236 }

For example,  "run_qemu -drive if=scsi"  will generate orphan warning
message for s390x,  but "run_qemu -drive if=virtio"  will not generate
orphan warning message,  since virtio is a default drive.

so I removed  _filter_orphan() in common.filter,  and added orphan
warning message(ie: for the output of "run_qemu -drive if=scsi") in
output file for s390x.   thanks


OK, so it is expected for s390x; however, this is strictly speaking not
the output file for s390x but for any platform but PC. That's why I'd
rather not have it in this “generic” reference output.

This patch already assumes that the iotests only support s390 and PC
(hunk @@ -251,28 +273,37 @@ in 051 adds a case statement which knows
only these two cases). Therefore, I would be fine with renaming this
reference output file to "051.s390.out" with a copy
"051.s390-ccw-virtio.out" and just removing the generic "051.out". Then
you can keep the warnings in.



It looks good to me. thanks

Hi Sascha:
This change is to rename 051.out to "051.s390.out" with a copy
"051.s390-ccw-virtio.out" and just to remove the generic "051.out". Is 
this fine to you?  thanks



(Or you filter the warnings out and keep it as "051.out".)

Max

Re: [Qemu-devel] [PATCH] qemu-iotests: Add -nographic when starting QEMU in 120

[tu bo]

Hi Fam, Max:

On 11/23/2015 10:33 AM, Fam Zheng wrote:

On Mon, 11/23 10:29, tu bo wrote:

Hi Max:

On 11/21/2015 12:17 AM, Max Reitz wrote:

On 20.11.2015 10:35, Fam Zheng wrote:

Otherwise, a window flashes on my desktop (built with SDL). Other
iotest cases have that.

Signed-off-by: Fam Zheng 
---
  tests/qemu-iotests/120 | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index 9f13078..d899a3f 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
{'execute': 'human-monitor-command',
 'arguments': {'command-line': 'qemu-io drv \"write -P 42 0 64k\"'}}
{'execute': 'quit'}" \
-| $QEMU -qmp stdio -nodefaults \
+| $QEMU -qmp stdio -nographic -nodefaults \
  -drive 
id=drv,if=none,file="$TEST_IMG",driver=raw,file.driver=$IMGFMT \
  | _filter_qmp | _filter_qemu_io
  $QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io



This is the same patch as
http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00623.html,
but while both are correct, both need to fix 119, too, I think.

(And while I would be fine with merging this and then taking a follow-up
patch, I don't think we need to hurry for 2.5. Releases and iotests
don't really care about each other, other than that we should pass all
the iotests before a release unless we know what's wrong and don't care.)



thanks for your advice. I plan to fix 119 in a follow-up patch.
currently, 119 is notrun for s390x, and I hope to spend some time to
investigate it later.



I'm sending a new version fixing both scripts.


Good news.

Hi Max:

If it's fine to you, I'll remove patch for 120 from my patch set. thanks



Fam

Re: [Qemu-devel] [PATCH] qemu-iotests: Add -nographic when starting QEMU in 120

[tu bo]

Hi Max:

On 11/21/2015 12:17 AM, Max Reitz wrote:

On 20.11.2015 10:35, Fam Zheng wrote:

Otherwise, a window flashes on my desktop (built with SDL). Other
iotest cases have that.

Signed-off-by: Fam Zheng 
---
  tests/qemu-iotests/120 | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index 9f13078..d899a3f 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
{'execute': 'human-monitor-command',
 'arguments': {'command-line': 'qemu-io drv \"write -P 42 0 64k\"'}}
{'execute': 'quit'}" \
-| $QEMU -qmp stdio -nodefaults \
+| $QEMU -qmp stdio -nographic -nodefaults \
  -drive 
id=drv,if=none,file="$TEST_IMG",driver=raw,file.driver=$IMGFMT \
  | _filter_qmp | _filter_qemu_io
  $QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io



This is the same patch as
http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00623.html,
but while both are correct, both need to fix 119, too, I think.

(And while I would be fine with merging this and then taking a follow-up
patch, I don't think we need to hurry for 2.5. Releases and iotests
don't really care about each other, other than that we should pass all
the iotests before a release unless we know what's wrong and don't care.)



thanks for your advice. I plan to fix 119 in a follow-up patch. 
currently, 119 is notrun for s390x, and I hope to spend some time to 
investigate it later.



Max

Re: [Qemu-devel] [PATCH v2 2/4] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max:

On 11/19/2015 12:52 AM, Max Reitz wrote:

On 04.11.2015 03:26, Bo Tu wrote:

The tests for device type "ide_cd" should only be tested for the pc
platform.
The default device id of hard disk on the s390 platform differs to that
of the x86 platform. A new variable device_id is defined and "virtio0"
set for the s390 platform. A x86 platform specific output file is also
needed.
Warning message expected for s390x when drive without device.

Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/051|  99 ++
  tests/qemu-iotests/051.out| 143 +++---
  tests/qemu-iotests/051.pc.out | 422 ++
  3 files changed, 515 insertions(+), 149 deletions(-)
  create mode 100644 tests/qemu-iotests/051.pc.out



[...]


diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7765aa0..d17c969 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out


[...]


@@ -109,20 +109,23 @@ QEMU X.Y.Z monitor - type 'help' for more information

  Testing: -drive if=floppy
  QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) qququiquit
+(qemu) Warning: Orphaned drive without device: 
id=floppy0,file=,if=floppy,bus=0,unit=0
+qququiquit


I'd still like these warnings to be filtered out (as I wrote in my reply
to the original RFC's v4, and as was done in later versions of that RFC
(the _filter_orphan function e.g. in
http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg04816.html).



I did more investigation about this.
"Warning: Orphaned drive without device:.*"  is expected for s390x, 
when we define a drive without device, also this drive is not default

and interface is not "none".  Please refer line 228,229 in blockdev.c:
218 bool drive_check_orphaned(void)
219 {
220 BlockBackend *blk;
221 DriveInfo *dinfo;
222 bool rs = false;
223
224 for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
225 dinfo = blk_legacy_dinfo(blk);
226 /* If dinfo->bdrv->dev is NULL, it has no device 
attached. */
227 /* Unless this is a default drive, this may be an 
oversight. */

228 if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
229 dinfo->type != IF_NONE) {
230 fprintf(stderr, "Warning: Orphaned drive without 
device: "

231 "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
232 blk_name(blk), blk_bs(blk)->filename, 
if_name[dinfo->type],

233 dinfo->bus, dinfo->unit);
234 rs = true;
235 }
236 }

For example,  "run_qemu -drive if=scsi"  will generate orphan warning 
message for s390x,  but "run_qemu -drive if=virtio"  will not generate 
orphan warning message,  since virtio is a default drive.


so I removed  _filter_orphan() in common.filter,  and added orphan 
warning message(ie: for the output of "run_qemu -drive if=scsi") in 
output file for s390x.   thanks



Looks good other than that.

Max

Re: [Qemu-devel] [PATCH v2 4/4] qemu-iotests: disable VNC server for test 120

[tu bo]

Hi Max:

On 11/19/2015 12:56 AM, Max Reitz wrote:

On 04.11.2015 03:26, Bo Tu wrote:

Ever since qemu-iotest 120 was introduced, its expected output didn't
include the output from the built-in VNC server:

  QA output created by 120
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
  QMP_VERSION
+VNC server running on `::1:5900'
  {"return": {}}
  wrote 65536/65536 bytes at offset 0
  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

Since some architectures do not even have a graphical console, we just
pass -nographic to avoid the output.

Fixes: a68197ff5b11 ("iotests: Add tests for overriding
BDRV_O_PROTOCOL")

Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/120 | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/120 b/tests/qemu-iotests/120
index 9f13078..fb69efd 100755
--- a/tests/qemu-iotests/120
+++ b/tests/qemu-iotests/120
@@ -49,7 +49,7 @@ echo "{'execute': 'qmp_capabilities'}
{'execute': 'human-monitor-command',
 'arguments': {'command-line': 'qemu-io drv \"write -P 42 0 64k\"'}}
{'execute': 'quit'}" \
-| $QEMU -qmp stdio -nodefaults \
+| $QEMU -qmp stdio -nodefaults -nographic \
  -drive 
id=drv,if=none,file="$TEST_IMG",driver=raw,file.driver=$IMGFMT \
  | _filter_qmp | _filter_qemu_io
  $QEMU_IO -c 'read -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io


Looks good, but I think we need the same for 119.


I agree with you that 119 need to add the parameter of "-nographic" at 
line 53.


 52 | $QEMU -drive id=drv,if=none,file="$TEST_IMG",driver=nbd \
 53 -qmp stdio -nodefaults \



Max

Re: [Qemu-devel] [PATCH v2 1/4] qemu-iotests: refine common.config

[tu bo]

Hi Max:

On 11/19/2015 12:45 AM, Max Reitz wrote:

On 04.11.2015 03:26, Bo Tu wrote:

Replacing sed with awk, then it's easier to read.


I think you meant "awk with sed".


It's my fault. thanks :-)




Replacing "[ ! -z "$default_alias_machine" ]" with
"[[ $default_alias_machine ]]", then it's slightly shorter.

Suggested-By: Sascha Silbe 
Reviewed-by: Sascha Silbe 
Reviewed-by: Eric Blake   
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/common.config | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..08f4b4e 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -128,11 +128,10 @@ export QEMU_IMG=_qemu_img_wrapper
  export QEMU_IO=_qemu_io_wrapper
  export QEMU_NBD=_qemu_nbd_wrapper

-default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')
-default_alias_machine=$($QEMU -machine \? |\
-awk -v var_default_machine="$default_machine"\)\
-'{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print 
$1}}')
-if [ ! -z "$default_alias_machine" ]; then
+default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
+default_alias_machine=$($QEMU -machine help | \
+   sed -n "/(alias of $default_machine)"/' { s/ .*//p; q; }')


Could be shortened to "/(alias of $default_machine)/ { s/ .*//p; q; }"
(superfluous quotation marks), but that doesn't make it less correct.


Yes. it can reduce two quotation marks. Good suggestion.




With the commit message fixed:

Reviewed-by: Max Reitz 


I'll add it in the patch of v3.




+if [[ "$default_alias_machine" ]]; then
  default_machine="$default_alias_machine"
  fi

Re: [Qemu-devel] [PATCH v1 1/4] qemu-iotests: refine common.config

[tu bo]

Hi Eric:

thanks for your review.

On 10/31/2015 01:36 AM, Eric Blake wrote:

On 10/30/2015 01:13 AM, Bo Tu wrote:

Be easier to read, and be slightly shorter.

You mentioned a very short "what" in the subject line (good), but the
"why" in the commit body ("easier to read, shorter") is rather terse and
subjective.  It would be nicer to go into details (change the definition
of default_alias_machine) or give a sample of what changes (use grep
instead of awk).


I agree with you. Adding more details as below,
Replacing sed with awk, then it's easier to read.
Replacing "[ ! -z "$default_alias_machine" ]" with "[[ $default_alias_machine 
]]",
then it's slightly shorter.



[meta-comment]

When sending a series, please include a 0/4 cover letter.  You may want
to do:
git config format.coverLetter auto
to make it automatic when using git format-patch/send-email.


My understanding is that cover letter is needed if the patch set is a little 
bit complicated.
Cover letter is not needed if patch set just has minor change and comment is 
already in the
git message for every patch.
If my understanding above is wrong, please correct me. I just hope to be more 
clear about process :-)


Suggested-By: Sascha Silbe 
Reviewed-by: Sascha Silbe 
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/common.config | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..0a165e8 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -129,10 +129,9 @@ export QEMU_IO=_qemu_io_wrapper
  export QEMU_NBD=_qemu_nbd_wrapper
  
  default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')

-default_alias_machine=$($QEMU -machine \? |\
-awk -v var_default_machine="$default_machine"\)\
-'{if ($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print 
$1}}')
-if [ ! -z "$default_alias_machine" ]; then
+default_alias_machine=$($QEMU -machine \? \

As long as we are touching this, we ought to switch to using '-machine
help' rather than the deprecated '-machine \?'.


thanks to point this.




+| grep -F "(alias of ${default_machine})" |cut -d ' ' -f 1 |head -n 1)

Why are you moving the | across lines?


thanks to point this



If we are rewriting to avoid awk, why not do it with a single sed
process, rather than a grep|cut|head pipeline?  For that matter, why not
rewrite default_machine to also avoid awk?


The goal is not to avoid awk. The line of default_machine is easy to read, but 
the
line of default_alias_machine as below is not easy to read, that's why Sascha
suggest me to change it for the line of default_alias_machine.

/-default_alias_machine=$($QEMU -machine \? |\ -awk -v 
var_default_machine="$default_machine"\)\ -'{if 
($(NF-2)=="(alias"&&$(NF-1)=="of"&&$(NF)==var_default_machine){print $1}}')/




default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p')
default_alias_machine=$($QEMU -machine help | \
   sed -n "/(alias of $default_machine)"' { s/ .*//p; q; }')

(which happens to work even if $default_machine contains '.', but might
get a bit dicey if the machine names could ever contain ?, [, *, or
other regex metacharacters)


I like the single sed process. However, I got error message as below after 
running it,
/sed: -e expression #1, char 38: unknown command: `.' /I guess you missed a '/' 
which is marked as red as below, So I change it as below,
/default_machine=$($QEMU -machine help | sed -n '/(default)/ s/ .*//p') 
default_alias_machine=$($QEMU -machine help | \ sed -n "/(alias of 
$default_machine)"/' { s/ .*//p; q; }') /





+if [ -n "$default_alias_machine" ]; then

Could shorten this to:

if [[ $default_alias_machine ]]; then


thanks to point this.



since we are already using bash.

Re: [Qemu-devel] [kvm-s390] qemu-system-s390x: cannot use stdio by multiple character devices

[tu bo]

Hi Christian:

"-no-shutdown"  works fine for s390.   thanks a lot for your suggestion.

I  added the parameter of "-no-shutdown" only for s390-ccw-virtio as below,

  _make_test_img $IMG_SIZE
+
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+  platform_parm="-no-shutdown"
+  ;;
+  *)
+  platform_parm=""
+  ;;
+esac
+
 # Give qemu some time to boot before saving the VM state
 bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+$QEMU $platform_parm -nographic -monitor stdio -serial none -hda 
"$TEST_IMG" -machine accel=kvm |\

 _filter_qemu
 # Now try to continue from that VM state (this should just work)
 echo quit |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" 
-loadvm 0 |\
+$QEMU $platform_parm -nographic -monitor stdio -serial none -hda 
"$TEST_IMG" -loadvm 0 -machine accel=kvm |\

 _filter_qemu


On 09/02/2015 05:13 PM, Christian Borntraeger wrote:

Am 20.08.2015 um 16:57 schrieb Alexander Graf:


On 20.08.15 01:20, tu bo wrote:

Hi Alex:

Ping you again just in case you did not get my mail  :-)

On 08/13/2015 03:52 PM, tu bo wrote:

Hi Alex:

I added one disk device for test case 068(qemu/tests/qemu-iotests/068,
which is for for loading a saved VM state from a qcow2 image ),
and got the same problem for s390-virtio-ccw.  Below is my steps:
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# ../../s390x-softmmu/qemu-system-s390x
-nodefaults -nographic -monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.94 monitor - type 'help' for more information
(qemu) [root@r17lp42 qemu-iotests]#

For s390-virtio,  test result is as expected
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.50 monitor - type 'help' for more information
(qemu) info roms
addr=9000 size=0x000ce8 mem=ram
name="/usr/share/qemu/s390-zipl.rom"
(qemu) savevm 0
(qemu)
(qemu) quit
3.[root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0
QEMU 2.3.50 monitor - type 'help' for more information
(qemu)

For x86-64, test result is as expected,
1. [gavin@oc646435 qemu-iotests]$ qemu-img create -f qcow2
scratch/t.qcow2 64M
2. [gavin@oc646435 qemu-iotests]$
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic
-monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.94 monitor - type 'help' for more information
(qemu) info roms
fw=genroms/kvmvapic.bin size=0x002400 name="kvmvapic.bin"
addr=fffc size=0x04 mem=rom name="bios-256k.bin"
/rom@etc/acpi/tables size=0x20 name="etc/acpi/tables"
/rom@etc/table-loader size=0x001000 name="etc/table-loader"
/rom@etc/acpi/rsdp size=0x24 name="etc/acpi/rsdp"
(qemu) savevm 0
(qemu)
3. [gavin@oc646435 qemu-iotests]$
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic
-monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0
QEMU 2.3.94 monitor - type 'help' for more information
(qemu)

Could you share me why s390-virtio-ccw has different behavior with
s390-virtio & x86_64 for this scenario?  thanks

Because the s390 folks at IBM thought it'd be cool to emit a panic
(read: shut down) in the ccw bootloader when there is a problem? ;)

Which is the right thing to do on an s390. On fatal errors, disabled
wait is used in all operating systems.


If this breaks test cases for you, please coordinate with Christian
Borntraeger and Eugene Dvurechenski whether it makes sense to change it.

-no-shutdown  might help, e.g. something like this


diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index b72e555..2185477 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -52,11 +52,11 @@ echo
  _make_test_img $IMG_SIZE
  # Give qemu some time to boot before saving the VM state
  bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+$QEMU -no-shutdown -nographic -nodefaults -monitor stdio -serial none -hda 
"$TEST_IMG" |\
  _filter_qemu
  # Now try to continue from that VM state (this should just work)
  echo quit |\
-$QEMU -nographic -monitor stdio -serial none -hda "$TEST_IMG" -loadvm 0 |\
+$QEMU -no-shutdown -nographic -nodefaults -monitor stdio -serial none -hda 
"$TEST_IMG" -loadvm 0 |\
  _filter_qemu

  # success, all done

Re: [Qemu-devel] [kvm-s390] qemu-system-s390x: cannot use stdio by multiple character devices

[tu bo]

Hi Christian:

Test case 068(qemu/tests/qemu-iotests/068, which is for loading a saved 
VM state from a qcow2 image)
was broken because s390-virtio-ccw uses the new bootloader of 
s390-ccw.img, instead of s390-zipl.rom.


1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# ../../s390x-softmmu/qemu-system-s390x
-nodefaults -nographic -monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.94 monitor - type 'help' for more information
(qemu) [root@r17lp42 qemu-iotests]#
3. I can get error message from s390-ccw.img as below,
Using guessed DASD geometry.
Using ECKD scheme (block size  4096),
CDL
! No zIPL section in IPL2 record. !

in qemu/pc-bios/s390-ccw/bootmap.c
213 static void ipl_eckd_cdl(void)
214 {
215 XEckdMbr *mbr;
216 Ipl2 *ipl2 = (void *)sec;
217 IplVolumeLabel *vlbl = (void *)sec;
218 block_number_t block_nr;
219
220 /* we have just read the block #0 and recognized it as "IPL1" */
*221 sclp_print("CDL\n");*
222
223 memset(sec, FREE_SPACE_FILLER, sizeof(sec));
224 read_block(1, ipl2, "Cannot read IPL2 record at block 1");
225
226 mbr = &ipl2->u.x.mbr;
227 IPL_assert(magic_match(mbr, ZIPL_MAGIC), "*No zIPL section in IPL2 
record.*");

We may have two solutions,
1. providing a very small linux image(assuming name is t.qcow2) for s390x which can 
be IPLed, via "s390x-softmmu/qemu-system-s390x
-nodefaults -nographic -monitor stdio -serial none -hda scratch/t.qcow2"
2. disable test case 068 for s390x

What's your opinion?  thanks


On 08/20/2015 10:57 PM, Alexander Graf wrote:


On 20.08.15 01:20, tu bo wrote:

Hi Alex:

Ping you again just in case you did not get my mail  :-)

On 08/13/2015 03:52 PM, tu bo wrote:

Hi Alex:

I added one disk device for test case 068(qemu/tests/qemu-iotests/068,
which is for for loading a saved VM state from a qcow2 image ),
and got the same problem for s390-virtio-ccw.  Below is my steps:
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# ../../s390x-softmmu/qemu-system-s390x
-nodefaults -nographic -monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.94 monitor - type 'help' for more information
(qemu) [root@r17lp42 qemu-iotests]#

For s390-virtio,  test result is as expected
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.50 monitor - type 'help' for more information
(qemu) info roms
addr=9000 size=0x000ce8 mem=ram
name="/usr/share/qemu/s390-zipl.rom"
(qemu) savevm 0
(qemu)
(qemu) quit
3.[root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0
QEMU 2.3.50 monitor - type 'help' for more information
(qemu)

For x86-64, test result is as expected,
1. [gavin@oc646435 qemu-iotests]$ qemu-img create -f qcow2
scratch/t.qcow2 64M
2. [gavin@oc646435 qemu-iotests]$
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic
-monitor stdio -serial none  -hda scratch/t.qcow2
QEMU 2.3.94 monitor - type 'help' for more information
(qemu) info roms
fw=genroms/kvmvapic.bin size=0x002400 name="kvmvapic.bin"
addr=fffc size=0x04 mem=rom name="bios-256k.bin"
/rom@etc/acpi/tables size=0x20 name="etc/acpi/tables"
/rom@etc/table-loader size=0x001000 name="etc/table-loader"
/rom@etc/acpi/rsdp size=0x24 name="etc/acpi/rsdp"
(qemu) savevm 0
(qemu)
3. [gavin@oc646435 qemu-iotests]$
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic
-monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0
QEMU 2.3.94 monitor - type 'help' for more information
(qemu)

Could you share me why s390-virtio-ccw has different behavior with
s390-virtio & x86_64 for this scenario?  thanks

Because the s390 folks at IBM thought it'd be cool to emit a panic
(read: shut down) in the ccw bootloader when there is a problem? ;)

If this breaks test cases for you, please coordinate with Christian
Borntraeger and Eugene Dvurechenski whether it makes sense to change it.


Alex

Re: [Qemu-devel] [kvm-s390] qemu-system-s390x: cannot use stdio by multiple character devices

[tu bo]

Hi Alex:

Ping you again just in case you did not get my mail  :-)

On 08/13/2015 03:52 PM, tu bo wrote:

Hi Alex:

I added one disk device for test case 068(qemu/tests/qemu-iotests/068, 
which is for for loading a saved VM state from a qcow2 image ),

and got the same problem for s390-virtio-ccw.  Below is my steps:
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# ../../s390x-softmmu/qemu-system-s390x 
-nodefaults -nographic -monitor stdio -serial none  -hda scratch/t.qcow2

QEMU 2.3.94 monitor - type 'help' for more information
(qemu) [root@r17lp42 qemu-iotests]#

For s390-virtio,  test result is as expected
1. qemu-img create -f qcow2 scratch/t.qcow2 64M
2. [root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults 
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2

QEMU 2.3.50 monitor - type 'help' for more information
(qemu) info roms
addr=9000 size=0x000ce8 mem=ram 
name="/usr/share/qemu/s390-zipl.rom"

(qemu) savevm 0
(qemu)
(qemu) quit
3.[root@r17lp42 qemu-iotests]# qemu-system-s390x -nodefaults 
-nographic -monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0

QEMU 2.3.50 monitor - type 'help' for more information
(qemu)

For x86-64, test result is as expected,
1. [gavin@oc646435 qemu-iotests]$ qemu-img create -f qcow2 
scratch/t.qcow2 64M
2. [gavin@oc646435 qemu-iotests]$ 
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic 
-monitor stdio -serial none  -hda scratch/t.qcow2

QEMU 2.3.94 monitor - type 'help' for more information
(qemu) info roms
fw=genroms/kvmvapic.bin size=0x002400 name="kvmvapic.bin"
addr=fffc size=0x04 mem=rom name="bios-256k.bin"
/rom@etc/acpi/tables size=0x20 name="etc/acpi/tables"
/rom@etc/table-loader size=0x001000 name="etc/table-loader"
/rom@etc/acpi/rsdp size=0x24 name="etc/acpi/rsdp"
(qemu) savevm 0
(qemu)
3. [gavin@oc646435 qemu-iotests]$ 
../../x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic 
-monitor stdio -serial none  -hda scratch/t.qcow2 -loadvm 0

QEMU 2.3.94 monitor - type 'help' for more information
(qemu)

Could you share me why s390-virtio-ccw has different behavior with 
s390-virtio & x86_64 for this scenario?  thanks



On 08/10/2015 03:51 PM, Alexander Graf wrote:



Am 10.08.2015 um 09:15 schrieb tu bo <mailto:t...@linux.vnet.ibm.com>>:



Dear  Alexander,  Mimu:

Failure of test case 068 for s390x is caused by the commit of 
"/1f68f1d36c3af09ed31a529ad69c3d09880d10fd//"

//Author: Alexander Graf //
//Date:   Tue Jun 16 23:06:33 2015 +0200//
//
//s390x: Switch to s390-ccw machine as default//

@@ -216,6 +216,7 @@ static void ccw_machine_class_init(ObjectClass 
*oc, void *data)

 mc->no_sdcard = 1;
 mc->use_sclp = 1;
 mc->max_cpus = 255;
*+mc->is_default = 1;*
 nc->nmi_monitor_handler = s390_nmi;
 }

@@ -345,7 +345,6 @@ static void s390_machine_class_init(ObjectClass 
*oc, void *data)

 mc->no_floppy = 1;
 mc->no_cdrom = 1;
 mc->no_sdcard = 1;
*-mc->is_default = 1;*
 nc->nmi_monitor_handler = s390_nmi;
 }

/Without this commit,   s390-virtio is default machine and default 
ipl device is *s390-zipl.rom*
[root@r17lp42 qemu]#  s390x-softmmu/qemu-system-s390x -nodefaults 
-nographic -monitor stdio -serial none

QEMU 2.3.94 monitor - type 'help' for more information
(qemu) info cpus
* CPU #0: thread_id=39761
(qemu) info roms
addr=9000 size=0x000ce8 mem=ram name="pc-bios/s390-zipl.rom"

With this commit,  s390-virtio-ccw is default machine and default 
ipl device is *s390-ccw.img*, When running
"s390x-softmmu/qemu-system-s390x -nodefaults -nographic -monitor 
stdio -serial none -machine accel=kvm" ,  the cpu status is

*halted* which causes the failure of test case 068.
[root@r17lp42 qemu]# s390x-softmmu/qemu-system-s390x -nodefaults 
-nographic -monitor stdio -serial none -machine accel=kvm

QEMU 2.3.94 monitor - type 'help' for more information
(qemu) info cpus
* CPU #0: *(halted)* thread_id=39746
(qemu) info roms
addr=07e0 size=0x002dd0 mem=ram name="phdr #2: 
/disks/bo.home/vs1403/qemu/pc-bios/s390-ccw.img"
addr=07e03f00 size=0x01d100 mem=ram name="phdr #3: 
/disks/bo.home/vs1403/qemu/pc-bios/s390-ccw.img"


With this commit,  when running "s390x-softmmu/qemu-system-s390x 
-nodefaults -nographic -monitor stdio -serial none", then qemu will 
exit as below,
[root@r17lp42 qemu]#  s390x-softmmu/qemu-system-s390x -nodefaults 
-nographic -monitor stdio -serial none

QEMU 2.3.94 monitor - type 'help' for more information
(qemu) [root@r17lp42 qemu]#

Is this the expected behavior of s390-virtio-ccw and s390-ccw.img?  
thanks


Yes, the ccw machine finds no device to load from and errors out, 
exiting thr vm. The s390-virtio boot loader wasn't smart enough for 
that and just hung iirc.



Alex

Re: [Qemu-devel] [PATCH v11 0/5] Update tests/qemu-iotests failing cases for the s390 platform

[tu bo]

Max replied that ignoring the UTF-8 error for 130 patch reported by 
checkpatch.pl is fine.
Could you please apply the series if there are no further objections.   
thanks


On 07/03/2015 03:28 PM, Bo Tu wrote:

v11.
1. Add Reviewed-by of Sascha
2. Refine code change in common.config in order to be easier to read and be 
shorter
3. Add more comments in patch description
4. Combine the fix for 041 and 055 in one patch since they address the same 
issue
5. Remove the fix for 051 since it fails now
6. checkpatch.pl reports invaid UTF-8 error for 130 patch, because 130.out 
contains
some non-text data.

v10.
1. Add Reviewed-by statements for test 049
2. Removed the backslash in qemu-option.c
3. Please apply the series if there are no further objections

v9.
1.Fix issue of line over 80 characters for test 049
2.Add Reviewed-by statements for test 051,130

v8.
1.Modify error message in qemu-option.c when image size is invalid
2.Remove Reviewed-by statements if any functional changes in a new patch version
for test 049,051,130
3.Change patch subject for test 130
4.Add id definition for a drive which will work for all platforms in test 130
5.Disable virtio-scsi-pci for non-PCI systems in test 051

v7.
1. Add a pc specific output file for test 130.
2. A new variable device_id is defined in test 130 to support multiplatform.
3. Update the output file for test 051 based on it's current output.
4. change util/qemu-option.c and test case 049, generate error message
when image size is a negtive value or exceeds the maximum of uint64

v6.
1. Change the filter name from _filter_s390 to _filter_orphan.
2. Update the output file for tese case 081 because no default floopy and 
cd-rom.

v5:
1. Add a pc specific output file for test 051.
2. Add a filter to test case 051 to filter s390 specific warnings.
3. Check whether the machine type is pc or not rather than check whether the 
machine type
is s390.
4. When using a machine specific reference file if the default machine has an 
alias then
use the alias as the output file name otherwise use the default machine name as 
the output
file name.

v4:
1. Generate all patches based on the latest master branch.
2. Rearrange patches

v3:
1. Fix a typo in v2.

v2:
1. Drop the patches for test 039 for it has been fixed in upstream.
2. Integrate patches for test 071, 067 and 087.
3. Keep the other patches.

v1:
1. updated the test suite to be default-machine-type-aware, from the previous 
platform-aware
2. created a new patch "qemu-iotests: run qemu with -nodefaults" to counterpart 
the impact from the commit:
 c88930a6866e74953e931ae749781e98e486e5c8
 qemu-char: Permit only a single "stdio" character device

 When more than one is used, the terminal settings aren't restored
 correctly on exit.  Fixable.  However, such usage makes no sense,
 because the users race for input, so outlaw it instead.

 If you want to connect multiple things to stdio, use the mux
 chardev.
3. updated all the checking of platform name to the current machine name

Bo Tu (5):
   qemu-iotests: qemu machine type support
   qemu-iotests: disable default qemu devices for cross-platform
 compatibility
   qemu-iotests: s390x: fix test 041 and 055
   qemu-iotests: s390x: fix test 049, reject negative sizes in QemuOpts
   qemu-iotests: s390x: fix test 130

  tests/qemu-iotests/041   |   6 +
  tests/qemu-iotests/049.out   |  10 +-
  tests/qemu-iotests/055   |   9 ++
  tests/qemu-iotests/067   |   8 +-
  tests/qemu-iotests/067.out   | 266 +--
  tests/qemu-iotests/071.out   |   4 -
  tests/qemu-iotests/081.out   |   2 -
  tests/qemu-iotests/087.out   |  12 --
  tests/qemu-iotests/130   |   8 +-
  tests/qemu-iotests/130.out   |   4 +-
  tests/qemu-iotests/check |   5 +
  tests/qemu-iotests/common|   1 +
  tests/qemu-iotests/common.config |  11 +-
  tests/qemu-iotests/common.qemu   |   2 +-
  tests/qemu-iotests/iotests.py|   1 +
  util/qemu-option.c   |   5 +
  16 files changed, 53 insertions(+), 301 deletions(-)

Re: [Qemu-devel] [PATCH v10 7/7] qemu-iotests: s390x: fix test 130

[tu bo]

I got one issue after running checkpatch.pl  below,

[gavin@oc646435 qemu]$ ./scripts/checkpatch.pl
home/gavin/patch/v8/0007-qemu-iotests-s390x-fix-test-130.patch //
ERROR: Invalid UTF-8, patch and commit message should be encoded in
UTF-8//
#52: FILE: tests/qemu-iotests/130.out:12://
+(qemu) commit testdisk//
 ^//

ERROR: Invalid UTF-8, patch and commit message should be encoded in
UTF-8//
#60: FILE: tests/qemu-iotests/130.out:19://
+(qemu) commit testdisk//
 ^//

total: 2 errors, 0 warnings, 36 lines checked/

The reason is that 130.out contains some non-text data.  Do I need to
report an issue to checkpatch.pl,  or ignore this error message? thanks

On 05/29/2015 11:32 AM, Bo Tu wrote:

The default device id of hard disk on the s390 platform is "virtio0"
which differs to the "ide0-hd0" for the x86 platform. Setting id in
the drive definition, ie:"qemu -drive id=testdisk", will be the same
on all platforms.

Reviewed-by: Max Reitz 
Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/130 | 8 
  tests/qemu-iotests/130.out | 4 ++--
  2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index bc26247..9209992 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -59,8 +59,8 @@ echo
  # bdrv_make_empty() involves a header update for qcow2

  # Test that a backing file isn't written
-_launch_qemu -drive file="$TEST_IMG",backing.file.filename="$TEST_IMG.base"
-_send_qemu_cmd $QEMU_HANDLE "commit ide0-hd0" "(qemu)"
+_launch_qemu -drive 
id=testdisk,file="$TEST_IMG",backing.file.filename="$TEST_IMG.base"
+_send_qemu_cmd $QEMU_HANDLE "commit testdisk" "(qemu)"
  _send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
  _cleanup_qemu
  _img_info | _filter_img_info
@@ -68,8 +68,8 @@ _img_info | _filter_img_info
  # Make sure that if there was a backing file that was just overridden on the
  # command line, that backing file is retained, with the right format
  _make_test_img -F raw -b "$TEST_IMG.orig" 64M
-_launch_qemu -drive 
file="$TEST_IMG",backing.file.filename="$TEST_IMG.base",backing.driver=$IMGFMT
-_send_qemu_cmd $QEMU_HANDLE "commit ide0-hd0" "(qemu)"
+_launch_qemu -drive 
id=testdisk,file="$TEST_IMG",backing.file.filename="$TEST_IMG.base",backing.driver=$IMGFMT
+_send_qemu_cmd $QEMU_HANDLE "commit testdisk" "(qemu)"
  _send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
  _cleanup_qemu
  _img_info | _filter_img_info
diff --git a/tests/qemu-iotests/130.out b/tests/qemu-iotests/130.out
index ea68b5d..9ec9d2a 100644
--- a/tests/qemu-iotests/130.out
+++ b/tests/qemu-iotests/130.out
@@ -9,14 +9,14 @@ virtual size: 64M (67108864 bytes)
  === HMP commit ===

  QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) 
ccocomcommcommicommitcommit
 commit icommit 
idcommit 
idecommit 
ide0commit 
ide0-commit 
ide0-hcommit 
ide0-hdcommit ide0-hd0
+(qemu) 
ccocomcommcommicommitcommit
 commit tcommit 
tecommit 
tescommit 
testcommit 
testdcommit 
testdicommit 
testdiscommit testdisk
  (qemu)
  image: TEST_DIR/t.IMGFMT
  file format: IMGFMT
  virtual size: 64M (67108864 bytes)
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file='TEST_DIR/t.IMGFMT.orig' backing_fmt='raw'
  QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) 
ccocomcommcommicommitcommit
 commit icommit 
idcommit 
idecommit 
ide0commit 
ide0-commit 
ide0-hcommit 
ide0-hdcommit ide0-hd0
+(qemu) 
ccocomcommcommicommitcommit
 commit tcommit 
tecommit 
tescommit 
testcommit 
testdcommit 
testdicommit 
testdiscommit testdisk
  (qemu)
  image: TE

Re: [Qemu-devel] [PATCH RFC v9 0/7] Update tests/qemu-iotests failing cases for the s390 platform

[tu bo]

Hi Thomas:

thanks a lot.  I'll remove 'RFC' from the subject line and drop the 
backslash according to Max's advice, and create v10.


On 05/28/2015 06:01 PM, Thomas Huth wrote:

  Hi Tu Bo!

On Thu, 28 May 2015 12:29:57 +0800
tu bo  wrote:


Hi Kevin:

On 05/26/2015 07:55 PM, Kevin Wolf wrote:

...

Is this still an RFC series, as the subject indicates, or is it meant
for inclusion?

This is my first task for qemu which I took over from Xiao Guang Chen,
and I'm not sure if I understand it well.  Does 'RFC' mean 'Reference
for Candidate'?  please correct me if I'm wrong.  If other actions I
need to take, please let me know.

RFC means "Request for comments". You put it into a patch title if you
want to get some comments about your patches, but don't consider them
good enough for inclusion yet. See also the patch submission guideline
for details: http://qemu-project.org/Contribute/SubmitAPatch


Anyway,  this patch series is final version.  thanks

I that case the title should normally not include "RFC" anymore.

  Regards,
   Thomas

Re: [Qemu-devel] [PATCH RFC v9 0/7] Update tests/qemu-iotests failing cases for the s390 platform

[tu bo]

Hi Kevin:

On 05/26/2015 07:55 PM, Kevin Wolf wrote:

Am 25.05.2015 um 05:30 hat Bo Tu geschrieben:

Bo Tu (3):
   qemu-iotests: s390x: fix test 049
   qemu-iotests: s390x: fix test 051
   qemu-iotests: s390x: fix test 130

Xiao Guang Chen (4):
   qemu-iotests: qemu machine type support
   qemu-iotests: run qemu with -nodefaults and fix 067,071,081 and 087
   qemu-iotests: s390x: fix test 041
   qemu-iotests: s390x: fix test 055

Is this supposed to be v10? There was already a v9 thread a few days
earlier.
Max helped to review v9, and v9 is fine for him, so v9 is the final 
version. This is the reason why I didn't create v10.


Is this still an RFC series, as the subject indicates, or is it meant
for inclusion?
This is my first task for qemu which I took over from Xiao Guang Chen, 
and I'm not sure if I understand it well.  Does 'RFC' mean 'Reference 
for Candidate'?  please correct me if I'm wrong.  If other actions I 
need to take, please let me know.

Anyway,  this patch series is final version.  thanks


Kevin

Re: [Qemu-devel] [PATCH RFC v7 7/7] qemu-iotests-s390x-fix-test-130

[tu bo]

Hi Kevin:

On 04/28/2015 04:23 PM, Kevin Wolf wrote:

Am 28.04.2015 um 04:59 hat tu bo geschrieben:

Hi Kevin:

On 04/27/2015 07:34 PM, Kevin Wolf wrote:

 Am 27.04.2015 um 13:24 hat Max Reitz geschrieben:

 On 27.04.2015 09:15, tu bo wrote:

 Hi Max:

 On 04/24/2015 01:07 AM, Max Reitz wrote:

 Well, that's a peculiar commit title. :-)

 I guess it's supposed to be "qemu-iotests: s390x: fix test 
130"?

 You're right. I will change it in the next version :-)


 On 23.04.2015 04:42, Xiao Guang Chen wrote:

 From: Bo Tu 

 The tests for device type "ide_cd" should only be tested 
for the pc
 platform.
 The default device id of hard disk on the s390 platform 
differs to
 that
 of the x86 platform. A new variable device_id is defined 
and
 "virtio0"
 set for the s390 platform. A x86 platform specific output 
file is
 also
 needed.

 Signed-off-by: Bo Tu 
 ---
   tests/qemu-iotests/130| 13 +++--
   tests/qemu-iotests/130.out|  4 ++--
   tests/qemu-iotests/130.pc.out | 43
 +++
   3 files changed, 56 insertions(+), 4 deletions(-)
   create mode 100644 tests/qemu-iotests/130.pc.out

 diff --git a/tests/qemu-iotests/130 
b/tests/qemu-iotests/130
 index bc26247..de40c7b 100755
 --- a/tests/qemu-iotests/130
 +++ b/tests/qemu-iotests/130
 @@ -58,9 +58,18 @@ echo "=== HMP commit ==="
   echo
   # bdrv_make_empty() involves a header update for qcow2
   +case "$QEMU_DEFAULT_MACHINE" in
 +pc)
 +device_id="ide0-hd0"
 +;;
 +s390)
 +device_id="virtio0"
 +;;


 I think I mentioned before that I don't really like not taking 
the fact
 into account that there are other machine types, too. I'm still
 accepting it based on the fact that I think those machine 
types won't
 pass the tests right now anyway, so not caring for them in 
these case
 blocks won't break any tests, but it still feels like 
something we can
 avoid (like defaulting to virtio0 for any non-pc platform).

 Anyway, because I seem to remember I accepted it before:

 With the commit title fixed:

 Reviewed-by: Max Reitz 

 I guess you discussed with Xiao Guang Chen and accepted it in 
"[PATCH RFC
 v5 6/7] qemu-iotests s390x fix test-051",  because test 130 and 
051 are
 using the same fix solution, and test 051 was fixed in v5. Seeing 
section
 of v5 in cover letter as below:


 Indeed we discussed it. Just for clarification, I disliked having only 
cases
 for "pc" and "s390" -- there are other platforms, too, which will 
simply break
 by not including them in these case statements. We could try to avoid 
this by
 defaulting to virtio0 for every non-pc platform, and it will probably 
work for
 most without having to do further work here.

 However, I did accept it because all those non-PC (and non-s390) 
platforms
 won't pass the tests before this patch set either (because these test 
cases try
 to use IDE devices which will not be available there). So the series 
will not
 break them because they didn't work before either.

 Bottom line: I'm fine with this solution as it is.

 Maybe I'm missing the obvious, but why don't you just specify an
 explicit ID instead of guessing the default ID that qemu will use
 depending on the platform?

Please forgive me that I'm very sure about the meaning of "default ID"  you
mentioned. Maybe you mean "default device ID"? If I'm wrong, please correct me
:-)

The default device id of hard disk on the s390 platform differs to the device
id on the x86 platform,  so we need to use different device id for different
platform. For instance,  using "virtio0" for s390x, and using "ide0-hd0" for
x86 as below:
+_send_qemu_cmd $QEMU_HANDLE "commit "  "virtio0" "(qemu)"
+_send_qemu_cmd $QEMU_HANDLE "

Re: [Qemu-devel] qcow2 problem: while a qcow2 image run out of disk space the guest paused

[tu bo]

Dears,

Xiao Guang Chen left IBM last week, and I changed owner from XiangGuang 
to me for this issue.


XiaoGuang told me this is by design, and probably I can close it if no 
news.  My understanding is that I can close it now. If I'm wrong, please 
correct me.  thanks a lot



From:Kevin Wolf 
To:Alexandre DERUMIER 
Cc:chenxg , stefanha 
, m...@linux.vnet.ibm.com, Bo Tu/China/IBM@IBMCN, 
borntrae...@de.ibm.com, Holger Wolf , qemu-devel 


Date:04/17/2015 03:57 PM
Subject:Re: [Qemu-devel] qcow2 problem: while a qcow2 image run out 
of disk space the guest paused.



Am 17.04.2015 um 06:30 hat Alexandre DERUMIER geschrieben:
> Hi,
>
> Isn't it related to drive options ?
>
>
> "
> werror=action,rerror=action
>
> Specify which action to take on write and read errors. Valid actions 
are: “ignore” (ignore the error and try to continue), “stop” (pause 
QEMU), “report” (report the error to the guest), “enospc” (pause QEMU 
only if the host disk is full; report the error to the guest 
otherwise).  The default setting is werror=enospc and rerror=report.

> "

Yes, it is.

Generally you don't want to report such errors to the guest, because the
guest sees that the disk is broken and stops using it. Even if you free
some space, you wouldn't easily get the guest to use the disk again
without rebooting. But if you really want, you can configure it so.

It is not expected that the guest gets I/O errors (except possibly
network timeouts if the VM is stopped for too long) or even qemu
crashes. If it crashes again for you, please provide a gdb backtrace.

Kevin

> - Mail original -
> De: "chenxg" 
> À: "Kevin Wolf" , "stefanha" , 
m...@linux.vnet.ibm.com, t...@cn.ibm.com, borntrae...@de.ibm.com, 
"Holger Wolf" , "qemu-devel" 

> Envoyé: Vendredi 17 Avril 2015 04:22:57
> Objet: [Qemu-devel] qcow2 problem: while a qcow2 image run out of 
disk space the guest paused.

>
> Hi Kevin,
>
> While installing a guest into a qcow2 image if the qcow2 image run out
> of disk space, the guest will stop work and change state to paused
> without messages.
>
> When we tried to free up disk space in the host and the virsh resume to
> work. The guest then got I/O errors but later on QEMU also crashed.
>
> A guest in paused state is certainly not the best result.
>
> Probably good way to react are:
> - return I/O errors to the guest and the let the guest handle those
> (Linux file system code might just crash)
> - allow resume only when disk is free again.
>
> What's your opinion?
>

Re: [Qemu-devel] [PATCH RFC v7 7/7] qemu-iotests-s390x-fix-test-130

[tu bo]

Hi Kevin:

On 04/27/2015 07:34 PM, Kevin Wolf wrote:

Am 27.04.2015 um 13:24 hat Max Reitz geschrieben:

On 27.04.2015 09:15, tu bo wrote:

 Hi Max:

 On 04/24/2015 01:07 AM, Max Reitz wrote:

 Well, that's a peculiar commit title. :-)

 I guess it's supposed to be "qemu-iotests: s390x: fix test 130"?

 You're right. I will change it in the next version :-)


 On 23.04.2015 04:42, Xiao Guang Chen wrote:

 From: Bo Tu 

 The tests for device type "ide_cd" should only be tested for the pc
 platform.
 The default device id of hard disk on the s390 platform differs to
 that
 of the x86 platform. A new variable device_id is defined and
 "virtio0"
 set for the s390 platform. A x86 platform specific output file is
 also
 needed.

 Signed-off-by: Bo Tu 
 ---
   tests/qemu-iotests/130| 13 +++--
   tests/qemu-iotests/130.out|  4 ++--
   tests/qemu-iotests/130.pc.out | 43
 +++
   3 files changed, 56 insertions(+), 4 deletions(-)
   create mode 100644 tests/qemu-iotests/130.pc.out

 diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
 index bc26247..de40c7b 100755
 --- a/tests/qemu-iotests/130
 +++ b/tests/qemu-iotests/130
 @@ -58,9 +58,18 @@ echo "=== HMP commit ==="
   echo
   # bdrv_make_empty() involves a header update for qcow2
   +case "$QEMU_DEFAULT_MACHINE" in
 +pc)
 +device_id="ide0-hd0"
 +;;
 +s390)
 +device_id="virtio0"
 +;;


 I think I mentioned before that I don't really like not taking the fact
 into account that there are other machine types, too. I'm still
 accepting it based on the fact that I think those machine types won't
 pass the tests right now anyway, so not caring for them in these case
 blocks won't break any tests, but it still feels like something we can
 avoid (like defaulting to virtio0 for any non-pc platform).

 Anyway, because I seem to remember I accepted it before:

 With the commit title fixed:

 Reviewed-by: Max Reitz 

 I guess you discussed with Xiao Guang Chen and accepted it in "[PATCH RFC
 v5 6/7] qemu-iotests s390x fix test-051",  because test 130 and 051 are
 using the same fix solution, and test 051 was fixed in v5. Seeing section
 of v5 in cover letter as below:


Indeed we discussed it. Just for clarification, I disliked having only cases
for "pc" and "s390" -- there are other platforms, too, which will simply break
by not including them in these case statements. We could try to avoid this by
defaulting to virtio0 for every non-pc platform, and it will probably work for
most without having to do further work here.

However, I did accept it because all those non-PC (and non-s390) platforms
won't pass the tests before this patch set either (because these test cases try
to use IDE devices which will not be available there). So the series will not
break them because they didn't work before either.

Bottom line: I'm fine with this solution as it is.

Maybe I'm missing the obvious, but why don't you just specify an
explicit ID instead of guessing the default ID that qemu will use
depending on the platform?
Please forgive me that I'm very sure about the meaning of "default ID"  
you mentioned. Maybe you mean "default device ID"? If I'm wrong, please 
correct me :-)


The default device id of hard disk on the s390 platform differs to the 
device id on the x86 platform,  so we need to use different device id 
for different platform. For instance,  using "virtio0" for s390x, and 
using "ide0-hd0" for x86 as below:

/+_send_qemu_cmd $QEMU_HANDLE "commit " *"virtio0"* "(qemu)"//
//+_send_qemu_cmd $QEMU_HANDLE "commit " *"ide0-hd0"* "(qemu)"/





Kevin

Re: [Qemu-devel] [PATCH RFC v7 7/7] qemu-iotests-s390x-fix-test-130

[tu bo]

Hi Max:

On 04/24/2015 01:07 AM, Max Reitz wrote:

Well, that's a peculiar commit title. :-)

I guess it's supposed to be "qemu-iotests: s390x: fix test 130"?

You're right. I will change it in the next version :-)


On 23.04.2015 04:42, Xiao Guang Chen wrote:

From: Bo Tu 

The tests for device type "ide_cd" should only be tested for the pc 
platform.

The default device id of hard disk on the s390 platform differs to that
of the x86 platform. A new variable device_id is defined and "virtio0"
set for the s390 platform. A x86 platform specific output file is also
needed.

Signed-off-by: Bo Tu 
---
  tests/qemu-iotests/130| 13 +++--
  tests/qemu-iotests/130.out|  4 ++--
  tests/qemu-iotests/130.pc.out | 43 
+++

  3 files changed, 56 insertions(+), 4 deletions(-)
  create mode 100644 tests/qemu-iotests/130.pc.out

diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index bc26247..de40c7b 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -58,9 +58,18 @@ echo "=== HMP commit ==="
  echo
  # bdrv_make_empty() involves a header update for qcow2
  +case "$QEMU_DEFAULT_MACHINE" in
+pc)
+device_id="ide0-hd0"
+;;
+s390)
+device_id="virtio0"
+;;


I think I mentioned before that I don't really like not taking the 
fact into account that there are other machine types, too. I'm still 
accepting it based on the fact that I think those machine types won't 
pass the tests right now anyway, so not caring for them in these case 
blocks won't break any tests, but it still feels like something we can 
avoid (like defaulting to virtio0 for any non-pc platform).


Anyway, because I seem to remember I accepted it before:

With the commit title fixed:

Reviewed-by: Max Reitz 
I guess you discussed with Xiao Guang Chen and accepted it in "[PATCH 
RFC v5 6/7] qemu-iotests s390x fix test-051",  because test 130 and 051 
are using the same fix solution, and test 051 was fixed in v5. Seeing 
section of v5 in cover letter as below:

/v5://
//1. Add a pc specific output file for test 051.//
//2. Add a filter to test case 051 to filter s390 specific warnings.//
//3. Check whether the machine type is pc or not rather than check 
whether the machine type //

//is s390.//
//4. When using a machine specific reference file if the default machine 
has an alias then//
//use the alias as the output file name otherwise use the default 
machine name as the output//

//file name/



+esac
+
  # Test that a backing file isn't written
  _launch_qemu -drive 
file="$TEST_IMG",backing.file.filename="$TEST_IMG.base"

-_send_qemu_cmd $QEMU_HANDLE "commit ide0-hd0" "(qemu)"
+_send_qemu_cmd $QEMU_HANDLE "commit " $device_id "(qemu)"


("commit $device_id" "(qemu)" would've worked, too, but it's fine the 
way _send_qemu_cmd works)



  _send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
  _cleanup_qemu
  _img_info | _filter_img_info
@@ -69,7 +78,7 @@ _img_info | _filter_img_info
  # command line, that backing file is retained, with the right format
  _make_test_img -F raw -b "$TEST_IMG.orig" 64M
  _launch_qemu -drive 
file="$TEST_IMG",backing.file.filename="$TEST_IMG.base",backing.driver=$IMGFMT

-_send_qemu_cmd $QEMU_HANDLE "commit ide0-hd0" "(qemu)"
+_send_qemu_cmd $QEMU_HANDLE "commit " $device_id "(qemu)"
  _send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
  _cleanup_qemu
  _img_info | _filter_img_info
diff --git a/tests/qemu-iotests/130.out b/tests/qemu-iotests/130.out
index ea68b5d..cc4a15a 100644
--- a/tests/qemu-iotests/130.out
+++ b/tests/qemu-iotests/130.out
@@ -9,14 +9,14 @@ virtual size: 64M (67108864 bytes)
  === HMP commit ===
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) 
ccocomcommcommicommitcommit 
commit 
icommit 
idcommit 
idecommit 
ide0commit ide0-commit 
ide0-hcommit ide0-hdcommit 
ide0-hd0
+(qemu) 
ccocomcommcommicommitcommit 
commit 
vcommit 
vicommit 
vircommit 
virtcommit virticommit 
virtiocommit virtio0

  (qemu)
  image: TEST_DIR/t.IMGFMT
  file format: IMGFMT
  virtual size: 64M (67108864 bytes)
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file='TEST_DIR/t.IMGFMT.orig' backing_fmt='raw'

  QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) 
ccocomcommcommi

Re: [Qemu-devel] [PATCH RFC v7 6/7] qemu-iotests: s390x: fix test 051

[tu bo]

Hi Max:

On 04/24/2015 01:00 AM, Max Reitz wrote:

On 23.04.2015 04:42, Xiao Guang Chen wrote:
The tests for device type "ide_cd" should only be tested for the pc 
platform.

The default device id of hard disk on the s390 platform differs to that
of the x86 platform. A new variable device_id is defined and "virtio0"
set for the s390 platform. A x86 platform specific output file is also
needed.
A new filter was added to filter orphan warnings.

Reviewed-by: Max Reitz 
Reviewed-by: Michael Mueller 
Signed-off-by: Xiao Guang Chen 
---
  tests/qemu-iotests/051   |  79 ---
  tests/qemu-iotests/051.out   | 156 +-
  tests/qemu-iotests/051.pc.out| 433 
+++

  tests/qemu-iotests/common.filter |   7 +
  4 files changed, 545 insertions(+), 130 deletions(-)
  create mode 100644 tests/qemu-iotests/051.pc.out

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 0360f37..8de16a5 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -147,13 +147,19 @@ run_qemu -drive if=ide
  run_qemu -drive if=virtio
  run_qemu -drive if=scsi
  -run_qemu -drive if=none,id=disk -device ide-cd,drive=disk
-run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-cd,drive=disk

-
-run_qemu -drive if=none,id=disk -device ide-drive,drive=disk
-run_qemu -drive if=none,id=disk -device ide-hd,drive=disk
-run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-disk,drive=disk
-run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-hd,drive=disk

+case "$QEMU_DEFAULT_MACHINE" in
+pc)
+run_qemu -drive if=none,id=disk -device ide-cd,drive=disk
+run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-cd,drive=disk

+
+run_qemu -drive if=none,id=disk -device ide-drive,drive=disk
+run_qemu -drive if=none,id=disk -device ide-hd,drive=disk
+run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-disk,drive=disk
+run_qemu -drive if=none,id=disk -device lsi53c895a -device 
scsi-hd,drive=disk

+;;
+*)
+;;
+esac
echo
  echo === Read-only ===
@@ -167,13 +173,19 @@ run_qemu -drive 
file="$TEST_IMG",if=ide,readonly=on

  run_qemu -drive file="$TEST_IMG",if=virtio,readonly=on
  run_qemu -drive file="$TEST_IMG",if=scsi,readonly=on
  -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device ide-cd,drive=disk
-run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on -device 
lsi53c895a -device scsi-cd,drive=disk

+case "$QEMU_DEFAULT_MACHINE" in
+pc)
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device ide-cd,drive=disk
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device lsi53c895a -device scsi-cd,drive=disk
  -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device ide-drive,drive=disk
-run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on -device 
ide-hd,drive=disk
-run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on -device 
lsi53c895a -device scsi-disk,drive=disk
-run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on -device 
lsi53c895a -device scsi-hd,drive=disk
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device ide-drive,drive=disk
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device ide-hd,drive=disk
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device lsi53c895a -device scsi-disk,drive=disk
+run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
-device lsi53c895a -device scsi-hd,drive=disk

+;;
+*)
+;;
+esac
echo
  echo === Cache modes ===
@@ -182,12 +194,12 @@ echo
  # Cannot use the test image because cache=none might not work on 
the host FS

  # Use cdrom so that we won't get errors about missing media
  -run_qemu -drive media=cdrom,cache=none
-run_qemu -drive media=cdrom,cache=directsync
-run_qemu -drive media=cdrom,cache=writeback
-run_qemu -drive media=cdrom,cache=writethrough
-run_qemu -drive media=cdrom,cache=unsafe
-run_qemu -drive media=cdrom,cache=invalid_value
+run_qemu -drive if=scsi,media=cdrom,cache=none
+run_qemu -drive if=scsi,media=cdrom,cache=directsync
+run_qemu -drive if=scsi,media=cdrom,cache=writeback
+run_qemu -drive if=scsi,media=cdrom,cache=writethrough
+run_qemu -drive if=scsi,media=cdrom,cache=unsafe
+run_qemu -drive if=scsi,media=cdrom,cache=invalid_value
echo
  echo === Specifying the protocol layer ===
@@ -251,28 +263,37 @@ echo
  echo === Snapshot mode ===
  echo
  +case "$QEMU_DEFAULT_MACHINE" in
+pc)
+device_id="ide0-hd0"
+;;
+s390)
+device_id="virtio0"
+;;
+esac
+
  $QEMU_IO -c "write -P 0x11 0 4k" "$TEST_IMG" | _filter_qemu_io
  -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
file="$TEST_IMG" -snapshot | _filter_qemu_io
-echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
file="$TEST_IMG",snapshot=on | _filte

Re: [Qemu-devel] [PATCH RFC v7 5/7] qemu-iotests: s390x: fix test 049

[tu bo]

Hello Max:

Xiao Guang Chen left IBM last week, and I took over this job. thanks for 
your comments :-)


On 04/24/2015 12:47 AM, Max Reitz wrote:

On 23.04.2015 04:42, Xiao Guang Chen wrote:

From: Bo Tu 


Hm, why is Bo Tu the patch author, but doesn't have an S-o-b in the 
commit message?
I created the patch, but faild to send out it via 'git send-email". So 
XiaoGuang sent the patch with his account on his machine. Perhaps that's 
the reason.

when creating an image qemu-img enable us specifying the size of the
image using -o size=xx options. But when we specify an invalid size
such as a negtive size then different platform gives different result.

parse_option_size() function in util/qemu-option.c will be called to
parse the size, a cast was called in the function to cast the input
(saved as a double in the function) size to an unsigned int64 value,
when the input is a negtive value or exceeds the maximum of uint64, then
the result is undefined.

Language spec 6.3.1.4 Real floating and integers:
the result of this assignment/cast is undefined if the float is not
in the open interval (-1, U_MAX+1).


Thank you for pointing to the specific section. I guess there are 
always new things to discover in C...



Signed-off-by: Xiao Guang Chen 
---
  tests/qemu-iotests/049.out | 10 --
  util/qemu-option.c |  5 +
  2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 9f93666..75d90b2 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -95,17 +95,15 @@ qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1024
  qemu-img: Image size must be less than 8 EiB!
qemu-img create -f qcow2 -o size=-1024 TEST_DIR/t.qcow2
-qemu-img: qcow2 doesn't support shrinking images yet
-qemu-img: TEST_DIR/t.qcow2: Could not resize image: Operation not 
supported
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=-1024 encryption=off 
cluster_size=65536 lazy_refcounts=off refcount_bits=16
+qemu-img: Parameter 'size' expects a positive number and must not 
exceeds the maximum UINT64

+qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- -1k
  qemu-img: Image size must be less than 8 EiB!
qemu-img create -f qcow2 -o size=-1k TEST_DIR/t.qcow2
-qemu-img: qcow2 doesn't support shrinking images yet
-qemu-img: TEST_DIR/t.qcow2: Could not resize image: Operation not 
supported
-Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=-1024 encryption=off 
cluster_size=65536 lazy_refcounts=off refcount_bits=16
+qemu-img: Parameter 'size' expects a positive number and must not 
exceeds the maximum UINT64

+qemu-img: TEST_DIR/t.qcow2: Invalid options for file format 'qcow2'
qemu-img create -f qcow2 TEST_DIR/t.qcow2 -- 1kilobyte
  qemu-img: Invalid image size specified! You may use k, M, G, T, P 
or E suffixes for

diff --git a/util/qemu-option.c b/util/qemu-option.c
index fda4e5f..1c50fa4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -179,6 +179,11 @@ void parse_option_size(const char *name, const 
char *value,

if (value != NULL) {
  sizef = strtod(value, &postfix);
+if (sizef < 0 || sizef > UINT64_MAX) {
+error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a 
positive "
+   "number and must not exceeds the maximum 
UINT64");


I think Markus would like to see these error macros not getting used 
anymore, so I think it should be dropped and the full string should be 
given here. I'll let him do the arguing, though. :-)


If you keep the macro, I'd propose "a non-negative number below 2^64" 
(or actually give the decimal value of UINT64_MAX, using 'a 
non-negative number not exceeding "%" PRIu64, UINT64_MAX'). Remember 
that 0 is not positive, but still a valid choice.
Good suggestion. I'll change the error message like "a non-negative 
number not exceeding "%" PRIu64, UINT64_MAX')" in v8.
If you drop the macro, I'd propose error_setg(errp, "'%s' must be a 
non-negative number below 2^64", name) or, like it is now, 
error_setg(errp, "Parameter '%s' expects a non-negative number below 
2^64", name).


Max


+return;
+}
  switch (*postfix) {
  case 'T':
  sizef *= 1024;