[Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Stefan Hajnoczi]

This series extends qemu-iotests 068 to also run with iothread enabled.  Doing
so was harder than expected because:

1. ioeventfd is disabled without -M accel=kvm even though it should work
2. loadvm still has an iothread bug

Instead of adding a ./check -iothread option I decided to always run the test.
Kevin recently recommended this approach; the advantage is that iothread *will*
always be tested iothread mode whereas people won't run ./check -iothread.

Stefan Hajnoczi (5):
  virtio-pci: use ioeventfd even when KVM is disabled
  migration: hold AioContext lock for loadvm qemu_fclose()
  qemu-iotests: 068: extract _qemu() function
  qemu-iotests: 068: use -drive/-device instead of -hda
  qemu-iotests: 068: test iothread mode

 hw/virtio/virtio-pci.c     |  2 +-
 migration/savevm.c         |  2 +-
 tests/qemu-iotests/068     | 37 +++++++++++++++++++++++++------------
 tests/qemu-iotests/068.out | 11 ++++++++++-
 4 files changed, 37 insertions(+), 15 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Stefan Hajnoczi]

Old kvm.ko versions only supported a tiny number of ioeventfds so
virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.

Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.

This patch makes -device virtio-blk-pci,iothread=iothread0 work even
when KVM is disabled.

I have tested that virtio-blk-pci works under TCG both with and without
iothread.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f9b7244..9f55476 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1740,7 +1740,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
     bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
                      !pci_bus_is_root(pci_dev->bus);
 
-    if (!kvm_has_many_ioeventfds()) {
+    if (kvm_enabled() && !kvm_has_many_ioeventfds()) {
         proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
     }
 
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Michael S. Tsirkin]

On Thu, Jun 15, 2017 at 05:38:09PM +0100, Stefan Hajnoczi wrote:
> Old kvm.ko versions only supported a tiny number of ioeventfds so
> virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> 
> Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> qtest or TCG mode.
> 
> This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> when KVM is disabled.
> 
> I have tested that virtio-blk-pci works under TCG both with and without
> iothread.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Don't we need to check we are on a host that supports eventfd?

> ---
>  hw/virtio/virtio-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9f55476 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1740,7 +1740,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>      bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
>                       !pci_bus_is_root(pci_dev->bus);
>  
> -    if (!kvm_has_many_ioeventfds()) {
> +    if (kvm_enabled() && !kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
>  
> -- 
> 2.9.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Fri, Jun 16, 2017 at 06:26:01AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 15, 2017 at 05:38:09PM +0100, Stefan Hajnoczi wrote:
> > Old kvm.ko versions only supported a tiny number of ioeventfds so
> > virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> > 
> > Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> > always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> > ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> > qtest or TCG mode.
> > 
> > This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> > when KVM is disabled.
> > 
> > I have tested that virtio-blk-pci works under TCG both with and without
> > iothread.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Don't we need to check we are on a host that supports eventfd?

That is not necessary because the ioeventfd memory API is based on
EventNotifier instead of raw eventfds.

EventNotifier falls back to pipes on POSIX platforms and uses native
Event objects on Windows.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Michael S. Tsirkin]

On Thu, Jun 15, 2017 at 05:38:09PM +0100, Stefan Hajnoczi wrote:
> Old kvm.ko versions only supported a tiny number of ioeventfds so
> virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> 
> Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> qtest or TCG mode.
> 
> This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> when KVM is disabled.
> 
> I have tested that virtio-blk-pci works under TCG both with and without
> iothread.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Do you want to merge this with the rest of the patchset,
or should I?

> ---
>  hw/virtio/virtio-pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f9b7244..9f55476 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1740,7 +1740,7 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>      bool pcie_port = pci_bus_is_express(pci_dev->bus) &&
>                       !pci_bus_is_root(pci_dev->bus);
>  
> -    if (!kvm_has_many_ioeventfds()) {
> +    if (kvm_enabled() && !kvm_has_many_ioeventfds()) {
>          proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD;
>      }
>  
> -- 
> 2.9.4

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

On Fri, Jun 16, 2017 at 05:12:18PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 15, 2017 at 05:38:09PM +0100, Stefan Hajnoczi wrote:
> > Old kvm.ko versions only supported a tiny number of ioeventfds so
> > virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> > 
> > Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> > always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> > ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> > qtest or TCG mode.
> > 
> > This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> > when KVM is disabled.
> > 
> > I have tested that virtio-blk-pci works under TCG both with and without
> > iothread.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Do you want to merge this with the rest of the patchset,
> or should I?

It can go through Kevin's tree as part of this patch series.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Fam Zheng]

On Thu, 06/15 17:38, Stefan Hajnoczi wrote:
> Old kvm.ko versions only supported a tiny number of ioeventfds so
> virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> 
> Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> qtest or TCG mode.
> 
> This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> when KVM is disabled.
> 
> I have tested that virtio-blk-pci works under TCG both with and without
> iothread.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

This one was dropped out from Kevin's pull request but the iotest case update on
068 which depends on it is merged. Now the test fails for me:

068 2s ... - output mismatch (see 068.out.bad)
--- /stor/work/qemu/tests/qemu-iotests/068.out	2017-06-27 16:22:55.003815188 +0800
+++ 068.out.bad	2017-06-27 16:41:37.903626275 +0800
@@ -12,9 +12,8 @@
 === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-x86_64: -device virtio-scsi-pci,id=hba0: ioeventfd is required for iothread
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) savevm 0
-(qemu) quit
+(qemu) qemu-system-x86_64: -device virtio-scsi-pci,id=hba0: ioeventfd is required for iothread
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Failures: 068
Failed 1 of 1 tests

Fam

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Kevin Wolf]

Am 27.06.2017 um 10:43 hat Fam Zheng geschrieben:
> On Thu, 06/15 17:38, Stefan Hajnoczi wrote:
> > Old kvm.ko versions only supported a tiny number of ioeventfds so
> > virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> > 
> > Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> > always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> > ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> > qtest or TCG mode.
> > 
> > This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> > when KVM is disabled.
> > 
> > I have tested that virtio-blk-pci works under TCG both with and without
> > iothread.
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> This one was dropped out from Kevin's pull request but the iotest case
> update on 068 which depends on it is merged. Now the test fails for
> me:

Whoops, sorry about that. Anyway, I think if we can, the way to fix it
is to find out why this patch is failing qtest, and merge a fixed v2,
rather than reverting the test cases.

Stefan, can you reproduce the failure?

Kevin

> 068 2s ... - output mismatch (see 068.out.bad)
> --- /stor/work/qemu/tests/qemu-iotests/068.out	2017-06-27 16:22:55.003815188 +0800
> +++ 068.out.bad	2017-06-27 16:41:37.903626275 +0800
> @@ -12,9 +12,8 @@
>  === Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
>  
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> +qemu-system-x86_64: -device virtio-scsi-pci,id=hba0: ioeventfd is required for iothread
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) savevm 0
> -(qemu) quit
> +(qemu) qemu-system-x86_64: -device virtio-scsi-pci,id=hba0: ioeventfd is required for iothread
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
> Failures: 068
> Failed 1 of 1 tests
> 
> Fam

Re: [Qemu-devel] [PATCH 1/5] virtio-pci: use ioeventfd even when KVM is disabled

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1558 bytes --]

On Tue, Jun 27, 2017 at 01:07:35PM +0200, Kevin Wolf wrote:
> Am 27.06.2017 um 10:43 hat Fam Zheng geschrieben:
> > On Thu, 06/15 17:38, Stefan Hajnoczi wrote:
> > > Old kvm.ko versions only supported a tiny number of ioeventfds so
> > > virtio-pci avoids ioeventfds when kvm_has_many_ioeventfds() returns 0.
> > > 
> > > Do not check kvm_has_many_ioeventfds() when KVM is disabled since it
> > > always returns 0.  Since commit 8c56c1a592b5092d91da8d8943c17777d6462a6f
> > > ("memory: emulate ioeventfd") it has been possible to use ioeventfds in
> > > qtest or TCG mode.
> > > 
> > > This patch makes -device virtio-blk-pci,iothread=iothread0 work even
> > > when KVM is disabled.
> > > 
> > > I have tested that virtio-blk-pci works under TCG both with and without
> > > iothread.
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > This one was dropped out from Kevin's pull request but the iotest case
> > update on 068 which depends on it is merged. Now the test fails for
> > me:
> 
> Whoops, sorry about that. Anyway, I think if we can, the way to fix it
> is to find out why this patch is failing qtest, and merge a fixed v2,
> rather than reverting the test cases.
> 
> Stefan, can you reproduce the failure?

I will merge this patch via my tree and send a pull request today.

The purpose of this patch is to make -device virtio-blk-pci,iothread=ID
work under TCG/qtest.  This is necessary because qemu-iotests isn't
allowed to depend on KVM.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/5] migration: hold AioContext lock for loadvm qemu_fclose()

[Stefan Hajnoczi]

migration_incoming_state_destroy() uses qemu_fclose() on the vmstate
file.  Make sure to call it inside an AioContext acquire/release region.

This fixes an 'qemu: qemu_mutex_unlock: Operation not permitted' abort
in loadvm.

This patch closes the vmstate file before ending the drained region.
Previously we closed the vmstate file after ending the drained region.
The order does not matter.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 migration/savevm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index ff126a1..943a43c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2292,11 +2292,11 @@ int load_snapshot(const char *name, Error **errp)
 
     aio_context_acquire(aio_context);
     ret = qemu_loadvm_state(f);
+    migration_incoming_state_destroy();
     aio_context_release(aio_context);
 
     bdrv_drain_all_end();
 
-    migration_incoming_state_destroy();
     if (ret < 0) {
         error_setg(errp, "Error %d while loading VM state", ret);
         return ret;
-- 
2.9.4

[Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Stefan Hajnoczi]

Avoid duplicating the QEMU command-line.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/068 | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 9c1687d..653e23c 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -59,14 +59,17 @@ case "$QEMU_DEFAULT_MACHINE" in
       ;;
 esac
 
-# Give qemu some time to boot before saving the VM state
-bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
+_qemu()
+{
     $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+          "$@" |\
     _filter_qemu | _filter_hmp
+}
+
+# Give qemu some time to boot before saving the VM state
+bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
 # Now try to continue from that VM state (this should just work)
-echo quit |\
-    $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" -loadvm 0 |\
-    _filter_qemu | _filter_hmp
+echo quit | _qemu -loadvm 0
 
 # success, all done
 echo "*** done"
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Kevin Wolf]

Am 15.06.2017 um 18:38 hat Stefan Hajnoczi geschrieben:
> Avoid duplicating the QEMU command-line.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/068 | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
> index 9c1687d..653e23c 100755
> --- a/tests/qemu-iotests/068
> +++ b/tests/qemu-iotests/068
> @@ -59,14 +59,17 @@ case "$QEMU_DEFAULT_MACHINE" in
>        ;;
>  esac
>  
> -# Give qemu some time to boot before saving the VM state
> -bash -c 'sleep 1; echo -e "savevm 0\nquit"' |\
> +_qemu()
> +{
>      $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\

This line shouldn't end with a pipe now. The bug goes away with the next
patch anyway, so I can just remove that one character while applying the
series.

> +          "$@" |\
>      _filter_qemu | _filter_hmp
> +}

Kevin

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
> Avoid duplicating the QEMU command-line.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/qemu-iotests/068 | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 

> +# Give qemu some time to boot before saving the VM state
> +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu

Are we sure that 'bash' on PATH is the same as the /bin/bash running the
script?

Also, while bash has more deterministic behavior for 'echo -e' (it is
non-portable if you are porting a script to other shells), it is still
possible to set bash to a mode where it does not work (see xhopt
xpg_echo).  I'd much prefer you use 'printf' instead of 'set -e'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]

On 06/27/2017 06:40 AM, Eric Blake wrote:
> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
>> Avoid duplicating the QEMU command-line.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  tests/qemu-iotests/068 | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
> 
>> +# Give qemu some time to boot before saving the VM state
>> +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
> 
> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
> script?
> 
> Also, while bash has more deterministic behavior for 'echo -e' (it is
> non-portable if you are porting a script to other shells), it is still
> possible to set bash to a mode where it does not work (see xhopt

shopt (I can't type first thing in the morning)

> xpg_echo).  I'd much prefer you use 'printf' instead of 'set -e'.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 950 bytes --]

On Tue, Jun 27, 2017 at 06:40:04AM -0500, Eric Blake wrote:
> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
> > Avoid duplicating the QEMU command-line.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/qemu-iotests/068 | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> 
> > +# Give qemu some time to boot before saving the VM state
> > +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
> 
> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
> script?
> 
> Also, while bash has more deterministic behavior for 'echo -e' (it is
> non-portable if you are porting a script to other shells), it is still
> possible to set bash to a mode where it does not work (see xhopt
> xpg_echo).  I'd much prefer you use 'printf' instead of 'set -e'.

Do you want to send a separate patch?

The purpose of this patch was just to move this code.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On 06/28/2017 07:13 AM, Stefan Hajnoczi wrote:
> On Tue, Jun 27, 2017 at 06:40:04AM -0500, Eric Blake wrote:
>> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
>>> Avoid duplicating the QEMU command-line.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  tests/qemu-iotests/068 | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>
>>> +# Give qemu some time to boot before saving the VM state
>>> +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
>>
>> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
>> script?
>>
>> Also, while bash has more deterministic behavior for 'echo -e' (it is
>> non-portable if you are porting a script to other shells), it is still
>> possible to set bash to a mode where it does not work (see xhopt
>> xpg_echo).  I'd much prefer you use 'printf' instead of 'set -e'.

instead of 'echo -e'

> 
> Do you want to send a separate patch?

Sure.

> 
> The purpose of this patch was just to move this code.

Fair enough. Besides, more than just 068 use 'echo -e' where 'printf'
would be better.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

Am 27.06.2017 um 13:40 hat Eric Blake geschrieben:
> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
> > Avoid duplicating the QEMU command-line.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  tests/qemu-iotests/068 | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> 
> > +# Give qemu some time to boot before saving the VM state
> > +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
> 
> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
> script?

Do we even need to explicitly call bash here? Doesn't this do the same
as far as this test case is concerned?

    ( sleep 1; echo "..." ) | _qemu

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On 06/28/2017 07:57 AM, Kevin Wolf wrote:
> Am 27.06.2017 um 13:40 hat Eric Blake geschrieben:
>> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
>>> Avoid duplicating the QEMU command-line.
>>>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  tests/qemu-iotests/068 | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>
>>> +# Give qemu some time to boot before saving the VM state
>>> +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
>>
>> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
>> script?
> 
> Do we even need to explicitly call bash here? Doesn't this do the same
> as far as this test case is concerned?
> 
>     ( sleep 1; echo "..." ) | _qemu

Or save a process:

{ sleep 1; echo "..."; } | _qemu

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: 068: extract _qemu() function

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

Am 28.06.2017 um 16:02 hat Eric Blake geschrieben:
> On 06/28/2017 07:57 AM, Kevin Wolf wrote:
> > Am 27.06.2017 um 13:40 hat Eric Blake geschrieben:
> >> On 06/15/2017 11:38 AM, Stefan Hajnoczi wrote:
> >>> Avoid duplicating the QEMU command-line.
> >>>
> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> ---
> >>>  tests/qemu-iotests/068 | 13 ++++++++-----
> >>>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>
> >>> +# Give qemu some time to boot before saving the VM state
> >>> +bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
> >>
> >> Are we sure that 'bash' on PATH is the same as the /bin/bash running the
> >> script?
> > 
> > Do we even need to explicitly call bash here? Doesn't this do the same
> > as far as this test case is concerned?
> > 
> >     ( sleep 1; echo "..." ) | _qemu
> 
> Or save a process:
> 
> { sleep 1; echo "..."; } | _qemu

Ah, I knew that something like this must work. I missed the semicolon...

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [PATCH 4/5] qemu-iotests: 068: use -drive/-device instead of -hda

[Stefan Hajnoczi]

The legacy -hda option does not support -drive/-device parameters.  They
will be required by the next patch that extends this test case.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/068 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 653e23c..7292643 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -53,15 +53,20 @@ _make_test_img $IMG_SIZE
 case "$QEMU_DEFAULT_MACHINE" in
   s390-ccw-virtio)
       platform_parm="-no-shutdown"
+      hba=virtio-scsi-ccw
       ;;
   *)
       platform_parm=""
+      hba=virtio-scsi-pci
       ;;
 esac
 
 _qemu()
 {
-    $QEMU $platform_parm -nographic -monitor stdio -serial none -hda "$TEST_IMG" |\
+    $QEMU $platform_parm -nographic -monitor stdio -serial none \
+          -drive if=none,id=drive0,file="$TEST_IMG",format="$IMGFMT" \
+          -device $hba,id=hba0 \
+          -device scsi-hd,drive=drive0 \
           "$@" |\
     _filter_qemu | _filter_hmp
 }
-- 
2.9.4

[Qemu-devel] [PATCH 5/5] qemu-iotests: 068: test iothread mode

[Stefan Hajnoczi]

Perform the savevm/loadvm test with both iothread on and off.  This
covers the recently found savevm/loadvm hang when iothread is enabled.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/068     | 23 ++++++++++++++---------
 tests/qemu-iotests/068.out | 11 ++++++++++-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 7292643..3801b65 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -45,11 +45,6 @@ _supported_os Linux
 IMGOPTS="compat=1.1"
 IMG_SIZE=128K
 
-echo
-echo "=== Saving and reloading a VM state to/from a qcow2 image ==="
-echo
-_make_test_img $IMG_SIZE
-
 case "$QEMU_DEFAULT_MACHINE" in
   s390-ccw-virtio)
       platform_parm="-no-shutdown"
@@ -71,10 +66,20 @@ _qemu()
     _filter_qemu | _filter_hmp
 }
 
-# Give qemu some time to boot before saving the VM state
-bash -c 'sleep 1; echo -e "savevm 0\nquit"' | _qemu
-# Now try to continue from that VM state (this should just work)
-echo quit | _qemu -loadvm 0
+for extra_args in \
+    "" \
+    "-object iothread,id=iothread0 -set device.hba0.iothread=iothread0"; do
+    echo
+    echo "=== Saving and reloading a VM state to/from a qcow2 image ($extra_args) ==="
+    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 $extra_args
+    # Now try to continue from that VM state (this should just work)
+    echo quit | _qemu $extra_args -loadvm 0
+done
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/068.out b/tests/qemu-iotests/068.out
index 0fa5340..aa063cf 100644
--- a/tests/qemu-iotests/068.out
+++ b/tests/qemu-iotests/068.out
@@ -1,6 +1,15 @@
 QA output created by 068
 
-=== Saving and reloading a VM state to/from a qcow2 image ===
+=== Saving and reloading a VM state to/from a qcow2 image () ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) savevm 0
+(qemu) quit
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) quit
+
+=== Saving and reloading a VM state to/from a qcow2 image (-object iothread,id=iothread0 -set device.hba0.iothread=iothread0) ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
 QEMU X.Y.Z monitor - type 'help' for more information
-- 
2.9.4

Re: [Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Stefan Hajnoczi]

On Thu, Jun 15, 2017 at 5:38 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> This series extends qemu-iotests 068 to also run with iothread enabled.

I forgot to mention: this series is based on Kevin's 'block' branch:

  git://repo.or.cz/qemu/kevin.git block

This is necessary in order to get the "[PATCH v3 0/4] block: fix
'savevm' hang with -object iothread" patches that fix savevm.

Stefan

Re: [Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Pavel Butsykin]

On 15.06.2017 19:38, Stefan Hajnoczi wrote:
> This series extends qemu-iotests 068 to also run with iothread enabled.  Doing
> so was harder than expected because:
> 
> 1. ioeventfd is disabled without -M accel=kvm even though it should work
> 2. loadvm still has an iothread bug
> 
> Instead of adding a ./check -iothread option I decided to always run the test.
> Kevin recently recommended this approach; the advantage is that iothread *will*
> always be tested iothread mode whereas people won't run ./check -iothread.

Why not just add -iothread option in check-block.sh? We can do an
additional run with -iothread, as it is done for different block
formats.

Because all the test cases already exist, we can just reuse them.
> Stefan Hajnoczi (5):
>    virtio-pci: use ioeventfd even when KVM is disabled
>    migration: hold AioContext lock for loadvm qemu_fclose()
>    qemu-iotests: 068: extract _qemu() function
>    qemu-iotests: 068: use -drive/-device instead of -hda
>    qemu-iotests: 068: test iothread mode
> 
>   hw/virtio/virtio-pci.c     |  2 +-
>   migration/savevm.c         |  2 +-
>   tests/qemu-iotests/068     | 37 +++++++++++++++++++++++++------------
>   tests/qemu-iotests/068.out | 11 ++++++++++-
>   4 files changed, 37 insertions(+), 15 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Mon, Jun 19, 2017 at 03:26:56PM +0300, Pavel Butsykin wrote:
> On 15.06.2017 19:38, Stefan Hajnoczi wrote:
> > This series extends qemu-iotests 068 to also run with iothread enabled.  Doing
> > so was harder than expected because:
> > 
> > 1. ioeventfd is disabled without -M accel=kvm even though it should work
> > 2. loadvm still has an iothread bug
> > 
> > Instead of adding a ./check -iothread option I decided to always run the test.
> > Kevin recently recommended this approach; the advantage is that iothread *will*
> > always be tested iothread mode whereas people won't run ./check -iothread.
> 
> Why not just add -iothread option in check-block.sh? We can do an
> additional run with -iothread, as it is done for different block
> formats.
> 
> Because all the test cases already exist, we can just reuse them.

From the email:

  "the advantage is that iothread *will* always be tested iothread mode
  whereas people won't run ./check -iothread"

Both approaches have pros and cons.

If we decide to use ./check -iothread then it's necessary to figure out
which tests -iothread even applies to.  Tests that use qemu-img/qemu-io
do not use iothread.  Only tests that launch QEMU can take advantage of
iothread today.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Pavel Butsykin]

On 05.07.2017 15:55, Stefan Hajnoczi wrote:
> On Mon, Jun 19, 2017 at 03:26:56PM +0300, Pavel Butsykin wrote:
>> On 15.06.2017 19:38, Stefan Hajnoczi wrote:
>>> This series extends qemu-iotests 068 to also run with iothread enabled.  Doing
>>> so was harder than expected because:
>>>
>>> 1. ioeventfd is disabled without -M accel=kvm even though it should work
>>> 2. loadvm still has an iothread bug
>>>
>>> Instead of adding a ./check -iothread option I decided to always run the test.
>>> Kevin recently recommended this approach; the advantage is that iothread *will*
>>> always be tested iothread mode whereas people won't run ./check -iothread.
>>
>> Why not just add -iothread option in check-block.sh? We can do an
>> additional run with -iothread, as it is done for different block
>> formats.
>>
>> Because all the test cases already exist, we can just reuse them.
> 
>  From the email:
> 
>    "the advantage is that iothread *will* always be tested iothread mode
>    whereas people won't run ./check -iothread"
> 
> Both approaches have pros and cons.

Yeah, it'd just be nice to add iothread support for all iotests.

> If we decide to use ./check -iothread then it's necessary to figure out
> which tests -iothread even applies to.  Tests that use qemu-img/qemu-io
> do not use iothread.  Only tests that launch QEMU can take advantage of
> iothread today.

Well, for tests which don't support the -iothread option we can define
something like this:

_unsupported_iothread

or

_unsupported_opts "iothread"

But overall it looks like a separate patch series, which can be done in
the future.

> Stefan
> 

Re: [Qemu-devel] [PATCH 0/5] qemu-iotests: test savevm/loadvm iothread (and make it work!)

[Kevin Wolf]

Am 15.06.2017 um 18:38 hat Stefan Hajnoczi geschrieben:
> This series extends qemu-iotests 068 to also run with iothread enabled.  Doing
> so was harder than expected because:
> 
> 1. ioeventfd is disabled without -M accel=kvm even though it should work
> 2. loadvm still has an iothread bug
> 
> Instead of adding a ./check -iothread option I decided to always run the test.
> Kevin recently recommended this approach; the advantage is that iothread *will*
> always be tested iothread mode whereas people won't run ./check -iothread.

Thanks, fixed the intermediate state after patch 3 (without changing the
final state after applying all patches) and applied to the block branch.

Kevin