Re: [Qemu-block] [Qemu-devel] [PATCH v2 2/2] blockjob: do not cancel timer in resume
在 2018/5/8 21:54, Stefan Hajnoczi 写道: > Currently the timer is cancelled and the block job is entered by > block_job_resume(). This behavior causes drain to run extra blockjob > iterations when the job was sleeping due to the ratelimit. > > This patch leaves the job asleep when block_job_resume() is called. > Jobs can still be forcibly woken up using block_job_enter(), which is > used to cancel jobs. > > After this patch drain no longer runs extra blockjob iterations. This > is the expected behavior that qemu-iotests 185 used to rely on. We > temporarily changed the 185 test output to make it pass for the QEMU > 2.12 release but now it's time to address this issue. > Verified on s390x. Thx Reviewed-by: QingFeng Hao <ha...@linux.vnet.ibm.com> > Cc: QingFeng Hao <ha...@linux.vnet.ibm.com> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> > --- > blockjob.c | 22 +++--- > tests/qemu-iotests/185 | 5 + > tests/qemu-iotests/185.out | 12 +--- > 3 files changed, 21 insertions(+), 18 deletions(-) > [...] > *** done > -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] Why is there no qom_get in hmp.c?
在 2018/4/18 22:04, Dr. David Alan Gilbert 写道: > * QingFeng Hao (ha...@linux.vnet.ibm.com) wrote: >> Hi all, >> I did some investigation and found that "virsh qemu-monitor-command" >> supports qom-get, >> but qemu hmp doesn't. However, in hmp.c there are qom_list and qom_set. It >> confused me >> and my question is: why is this? And how can I get a property's value in >> hmp? e.g. >> qemu-system-* -nodefaults -machine accel=qtest -no-shutdown -nographic >> -monitor stdio -serial none -hda /root/t.qcow2 >> "info qtree" can only get a few properties. > > I did try that in 2016 (see my series from about September); but it got > bogged down in trying to fix output visitors; it's possible the visitor > code has been fixed since then though. > Thanks! I used your patch with a bit fix of compilation to test, which can work and print the json format string for structure property. The prior discussions seem no any eventual conclusion with some tricks. > The 'Show values and description when using "qom-list"' patch > that Ricardo Perez Blanco posted would do something similar. Yes, I saw the patch. thanks > > Dave > >> Thanks a lot! >> -- >> Regards >> QingFeng Hao >> >> > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK > -- Regards QingFeng Hao
[Qemu-block] Why is there no qom_get in hmp.c?
Hi all, I did some investigation and found that "virsh qemu-monitor-command" supports qom-get, but qemu hmp doesn't. However, in hmp.c there are qom_list and qom_set. It confused me and my question is: why is this? And how can I get a property's value in hmp? e.g. qemu-system-* -nodefaults -machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none -hda /root/t.qcow2 "info qtree" can only get a few properties. Thanks a lot! -- Regards QingFeng Hao
Re: [Qemu-block] [PATCH for-2.12 v2] qemu-iotests: update 185 output
在 2018/4/4 23:01, Stefan Hajnoczi 写道:
> Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> drain operation the block job iterates one more time than before. The
> 185 output no longer matches and the test is failing now.
>
> It may be possible to avoid the superfluous block job iteration, but
> that type of patch is not suitable late in the QEMU 2.12 release cycle.
>
> This patch simply updates the 185 output file. The new behavior is
> correct, just not optimal, so make the test pass again.
>
> Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()")
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: QingFeng Hao <ha...@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> tests/qemu-iotests/185 | 10 ++
> tests/qemu-iotests/185.out | 12 +++-
> 2 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
> index f5b47e4c1a..298d88d04e 100755
> --- a/tests/qemu-iotests/185
> +++ b/tests/qemu-iotests/185
> @@ -92,9 +92,8 @@ echo === Start commit job and exit qemu ===
> echo
>
> # Note that the reference output intentionally includes the 'offset' field in
> -# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
> -# predictable and any change in the offsets would hint at a bug in the job
> -# throttling code.
> +# BLOCK_JOB_* events for all of the following block jobs. They are
> predictable
> +# and any change in the offsets would hint at a bug in the job throttling
> code.
> #
> # In order to achieve these predictable offsets, all of the following tests
> # use speed=65536. Each job will perform exactly one iteration before it has
> @@ -102,11 +101,14 @@ echo
> # command to be received (after receiving the command, the rest runs
> # synchronously, so jobs can arbitrarily continue or complete).
> #
> +# Jobs present while QEMU is terminating iterate once more due to
> +# bdrv_drain_all().
> +#
> # The buffer size for commit and streaming is 512k (waiting for 8 seconds
> after
> # the first request), for active commit and mirror it's large enough to cover
> # the full 4M, and for backup it's the qcow2 cluster size, which we know is
> # 64k. As all of these are at least as large as the speed, we are sure that
> the
> -# offset doesn't advance after the first iteration before qemu exits.
> +# offset advances exactly twice before qemu exits.
>
> _send_qemu_cmd $h \
> "{ 'execute': 'block-commit',
Reviewed-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
> diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
> index 57eaf8d699..2c4b04de73 100644
[...]
>
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH] qemu-iotests: update 185 output
在 2018/4/3 22:03, Stefan Hajnoczi 写道:
> Commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()") added a bdrv_drain_all() call. As a side-effect of the
> drain operation the block job iterates one more time than before. The
> 185 output no longer matches and the test is failing now.
>
> It may be possible to avoid the superfluous block job iteration, but
> that type of patch is not suitable late in the QEMU 2.12 release cycle.
>
> This patch simply updates the 185 output file. The new behavior is
> correct, just not optimal, so make the test pass again.
>
> Fixes: 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268 ("vl: introduce
> vm_shutdown()")
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: QingFeng Hao <ha...@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> ---
> tests/qemu-iotests/185.out | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
> index 57eaf8d699..2c4b04de73 100644
> --- a/tests/qemu-iotests/185.out
> +++ b/tests/qemu-iotests/185.out
> @@ -20,7 +20,7 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
> backing_file=TEST_DIR/t.q
> {"return": {}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
> 524288, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
> 1048576, "speed": 65536, "type": "commit"}}
Need we add the comment in 185.out that this change is to make the test pass
for the superfluous block job iteration?
thanks
>
> === Start active commit job and exit qemu ===
>
> @@ -28,16 +28,18 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
> backing_file=TEST_DIR/t.q
> {"return": {}}
> {"return": {}}
> {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "SHUTDOWN", "data": {"guest": false}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
> 4194304, "speed": 65536, "type": "commit"}}
[...]
>
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] iotests: fix test case 185
在 2018/3/26 18:29, Kevin Wolf 写道:
Am 23.03.2018 um 04:43 hat QingFeng Hao geschrieben:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
The fix is to not resume the jobs that are already yielded and also change
185.out accordingly.
Suggested-by: Stefan Hajnoczi <stefa...@gmail.com>
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Stefan already commented on the fix itself, but I want to add two more
points:
Please change your subject line. "iotests: fix test case 185" means that
you are fixing the test case, not qemu code that makes the test case
fail. The subject line should describe the actual change. In all
likelihood it will start with "blockjob:" rather than "iotests:".
Sure! thanks for pointing that.
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fc645dac68..f8f208bbcf 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -99,6 +99,11 @@ typedef struct BlockJob {
bool ready;
/**
+ * Set to true when the job is yielded.
+ */
+bool yielded;
This is the same as !busy, so we don't need a new field for this.
Mostly yes, but the trick is that busy is set to be true in
block_job_do_yield.
Kevin
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] iotests: fix test case 185
在 2018/3/23 18:04, Stefan Hajnoczi 写道:
On Fri, Mar 23, 2018 at 3:43 AM, QingFeng Hao <ha...@linux.vnet.ibm.com> wrote:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
The fix is to not resume the jobs that are already yielded and also change
185.out accordingly.
Suggested-by: Stefan Hajnoczi <stefa...@gmail.com>
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
blockjob.c | 10 +-
include/block/blockjob.h | 5 +
tests/qemu-iotests/185.out | 11 +--
If drain no longer forces the block job to iterate, shouldn't the test
output remain the same? (The means the test is fixed by the QEMU
patch.)
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index ef3ed69ff1..fa9838ac97 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -206,11 +206,16 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob
*job)
static void block_job_pause(BlockJob *job)
{
-job->pause_count++;
+if (!job->yielded) {
+job->pause_count++;
+}
The pause cannot be ignored. This change introduces a bug.
Pause is not a synchronous operation that stops the job immediately.
Pause just remembers that the job needs to be paused. When the job
runs again (e.g. timer callback, fd handler) it eventually reaches
block_job_pause_point() where it really pauses.
The bug in this patch is:
1. The job has a timer pending.
2. block_job_pause() is called during drain.
3. The timer fires during drain but now the job doesn't know it needs
to pause, so it continues running!
Instead what needs to happen is that block_job_pause() remains
unmodified but block_job_resume if extended:
static void block_job_resume(BlockJob *job)
{
assert(job->pause_count > 0);
job->pause_count--;
if (job->pause_count) {
return;
}
+if (job_yielded_before_pause_and_is_still_yielded) {
Thanks a lot for your great comments! But I can't figure out how to
check this.
block_job_enter(job);
+}
}
This handles the case I mentioned above, where the yield ends before
pause ends (therefore resume must enter the job!).
To make this a little clearer, there are two cases to consider:
Case 1:
1. Job yields
2. Pause
3. Job is entered from timer/fd callback
How do I know that if job is entered from ...? thanks
4. Resume (enter job? yes)
Case 2:
1. Job yields
2. Pause
3. Resume (enter job? no)
4. Job is entered from timer/fd callback
Stefan
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v2 1/1] iotests: fix test case 185
Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
The fix is to not resume the jobs that are already yielded and also change
185.out accordingly.
Suggested-by: Stefan Hajnoczi <stefa...@gmail.com>
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
blockjob.c | 10 +-
include/block/blockjob.h | 5 +
tests/qemu-iotests/185.out | 11 +--
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index ef3ed69ff1..fa9838ac97 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -206,11 +206,16 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob
*job)
static void block_job_pause(BlockJob *job)
{
-job->pause_count++;
+if (!job->yielded) {
+job->pause_count++;
+}
}
static void block_job_resume(BlockJob *job)
{
+if (job->yielded) {
+return;
+}
assert(job->pause_count > 0);
job->pause_count--;
if (job->pause_count) {
@@ -371,6 +376,7 @@ static void block_job_sleep_timer_cb(void *opaque)
BlockJob *job = opaque;
block_job_enter(job);
+job->yielded = false;
}
void block_job_start(BlockJob *job)
@@ -935,6 +941,7 @@ void *block_job_create(const char *job_id, const
BlockJobDriver *driver,
job->cb= cb;
job->opaque= opaque;
job->busy = false;
+job->yielded = false;
job->paused= true;
job->pause_count = 1;
job->refcnt= 1;
@@ -1034,6 +1041,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
timer_mod(>sleep_timer, ns);
}
job->busy = false;
+job->yielded = true;
block_job_unlock();
qemu_coroutine_yield();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fc645dac68..f8f208bbcf 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -99,6 +99,11 @@ typedef struct BlockJob {
bool ready;
/**
+ * Set to true when the job is yielded.
+ */
+bool yielded;
+
+/**
* Set to true when the job has deferred work to the main loop.
*/
bool deferred_to_main_loop;
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 57eaf8d699..798282e196 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -7,6 +7,7 @@ Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
=== Creating backing chain ===
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"RESUME"}
Formatting 'TEST_DIR/t.qcow2.mid', fmt=qcow2 size=67108864
backing_file=TEST_DIR/t.qcow2.base backing_fmt=qcow2 cluster_size=65536
lazy_refcounts=off refcount_bits=16
{"return": {}}
wrote 4194304/4194304 bytes at offset 0
@@ -25,23 +26,28 @@ Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864
backing_file=TEST_DIR/t.q
=== Start active commit job and exit qemu ===
{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"RESUME"}
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
=== Start mirror job and exit qemu ===
{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"RESUME"}
Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=1
[Qemu-block] [PATCH v2 0/1] iotests: fix test case 185
Hi, This patch is to fix iotest case 185 and based on the latest commit d522e0bd18364 --- "gitmodules: Use the QEMU mirror of qemu-palcode". Thanks! Change Log (v2): * Recover the change on the call to bdrv_drain_all but don't resume the yielded jobs according to Stefan Hajnoczi's comment. * Change 185.out accordingly as job's status is changed as well. Change Log (v1): * Remove the call to bdrv_drain_all in vm_shutdown to make case 185 passed. QingFeng Hao (1): iotests: fix test case 185 blockjob.c | 10 +- include/block/blockjob.h | 5 + tests/qemu-iotests/185.out | 11 +-- 3 files changed, 23 insertions(+), 3 deletions(-) -- 2.13.5
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185
在 2018/3/21 11:12, QingFeng Hao 写道:
在 2018/3/20 22:31, Stefan Hajnoczi 写道:
On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:
Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:
On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao
<ha...@linux.vnet.ibm.com> wrote:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce
vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls
bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes
the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
Thus, let's not call bdrv_drain_all in vm_shutdown.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
cpus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool
send_stop)
qapi_event_send_stop(_abort);
}
}
-
- bdrv_drain_all();
+ if (send_stop) {
+ bdrv_drain_all();
+ }
Thanks for looking into this bug!
This if statement breaks the contract of the function when send_stop
is false. Drain ensures that pending I/O completes and that device
emulation finishes before this function returns. This is important
for live migration, where there must be no more guest-related activity
after vm_stop().
This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.
I suggest a different solution:
Test 185 relies on internal QEMU behavior which can change from time
to time. Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.
In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.
After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain. This has caused the test
failure.
Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.
Wouldnt it be better if the test actually masks out the values the
are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset
from
common.filter be what we want?
The test case has the following note:
# Note that the reference output intentionally includes the 'offset'
field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs.
They are
# predictable and any change in the offsets would hint at a bug in
the job
# throttling code.
Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.
I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.
Here is the reason for the additional I/O and how it could be
eliminated:
child_job_drained_begin() pauses and child_job_drained_end() resumes the
job. Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine. This is why draining BlockDriverState forces an
extra iteration of the blockjob.
The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time. When the I/O completes, the
job will see it must pause and it will yield at a pause point. It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.
In our case this behavior doesn't make sense though. The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time. We'd prefer to wait for the timer
expiry.
Thanks for all of your good comments! I think the better way is to
filter out this case but I am sure if this is a proper way to do it that
adds a yielded field in struct BlockJob to record if it's yielded by
block_job_do_yield. However, this way can only solve the offset problem
but not the status. Maybe we need also to change 185.out? I am bit
confused. thanks
What I did is setting job->yielded in block_job_do_yield and adding the
check for it in block_job_pause and block_job_resume that if yielded is
true, just return.
We need to distinguish these two cases to avoid resuming the job in the
latter case. I think this would be the proper way to avoid unnecessary
blockjob activity across drain.
I favor updating the test output though, because the code change adds
complexity and the practical benefi
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185
在 2018/3/20 22:31, Stefan Hajnoczi 写道:
On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:
Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:
On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao <ha...@linux.vnet.ibm.com> wrote:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
Thus, let's not call bdrv_drain_all in vm_shutdown.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
cpus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
qapi_event_send_stop(_abort);
}
}
-
-bdrv_drain_all();
+if (send_stop) {
+bdrv_drain_all();
+}
Thanks for looking into this bug!
This if statement breaks the contract of the function when send_stop
is false. Drain ensures that pending I/O completes and that device
emulation finishes before this function returns. This is important
for live migration, where there must be no more guest-related activity
after vm_stop().
This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.
I suggest a different solution:
Test 185 relies on internal QEMU behavior which can change from time
to time. Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.
In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.
After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain. This has caused the test
failure.
Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.
Wouldnt it be better if the test actually masks out the values the are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset from
common.filter be what we want?
The test case has the following note:
# Note that the reference output intentionally includes the 'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
# predictable and any change in the offsets would hint at a bug in the job
# throttling code.
Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.
I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.
Here is the reason for the additional I/O and how it could be
eliminated:
child_job_drained_begin() pauses and child_job_drained_end() resumes the
job. Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine. This is why draining BlockDriverState forces an
extra iteration of the blockjob.
The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time. When the I/O completes, the
job will see it must pause and it will yield at a pause point. It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.
In our case this behavior doesn't make sense though. The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time. We'd prefer to wait for the timer
expiry.
Thanks for all of your good comments! I think the better way is to
filter out this case but I am sure if this is a proper way to do it that
adds a yielded field in struct BlockJob to record if it's yielded by
block_job_do_yield. However, this way can only solve the offset problem
but not the status. Maybe we need also to change 185.out? I am bit
confused. thanks
We need to distinguish these two cases to avoid resuming the job in the
latter case. I think this would be the proper way to avoid unnecessary
blockjob activity across drain.
I favor updating the test output though, because the code change adds
complexity and the practical benefit is not obvious to me.
QingFeng, if you decide to modify blockjobs, please CC Jeffrey Cody
<jc...@redhat.com> and I'd also be happy to review the patch
Stefan
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v1 0/1] iotests: fix test case 185
Hi,
This patch is to remove the redundant call to bdrv_drain_all in vm_shutdown.
Thanks!
Test case 185 failed as below:
185 2s ... - output mismatch (see 185.out.bad)
--- /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out
2018-03-09 01:00:40.451603189 +0100
+++ /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out.bad
2018-03-19 09:40:22.781603189 +0100
@@ -20,7 +20,7 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
524288, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
1048576, "speed": 65536, "type": "commit"}}
=== Start active commit job and exit qemu ===
@@ -28,7 +28,8 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "commit"}}
=== Start mirror job and exit qemu ===
@@ -37,7 +38,8 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset":
4194304, "speed": 65536, "type": "mirror"}}
=== Start backup job and exit qemu ===
@@ -46,7 +48,7 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
65536, "speed": 65536, "type": "backup"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
131072, "speed": 65536, "type": "backup"}}
=== Start streaming job and exit qemu ===
@@ -54,6 +56,6 @@
{"return": {}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
524288, "speed": 65536, "type": "stream"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
"BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset":
1048576, "speed": 65536, "type": "stream"}}
No errors were found on the image.
*** done
Failures: 185
Failed 1 of 1 tests
QingFeng Hao (1):
iotests: fix test case 185
cpus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
--
2.13.5
[Qemu-block] [PATCH v1 1/1] iotests: fix test case 185
Test case 185 failed since commit 4486e89c219 --- "vl: introduce vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
Thus, let's not call bdrv_drain_all in vm_shutdown.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
cpus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool send_stop)
qapi_event_send_stop(_abort);
}
}
-
-bdrv_drain_all();
+if (send_stop) {
+bdrv_drain_all();
+}
replay_disable_events();
ret = bdrv_flush_all();
--
2.13.5
Re: [Qemu-block] [PATCH v1 1/1] iotests: bypass s390x for case 200
在 2018/3/6 15:56, Christian Borntraeger 写道:
Nack. This will be fixed by
s390/ipl: only print boot menu error if -boot menu=on was specified
You are right. After I applied that patch, the case is passed.
Please ignore this patch. Thanks
On 03/06/2018 08:54 AM, QingFeng Hao wrote:
In s390x, the case 200 failed as:
=== Starting QEMU VM ===
+QEMU_PROG: boot menu is not supported for this device type.
{"return": {}}
=== Sending stream/cancel, checking for SIGSEGV only ===
Failures: 200
Failed 1 of 1 tests
It was caused by the command which isn't supported by s390x now:
qemu-system-s390x -device pci-bridge,id=bridge1,chassis_nr=1,bus=pci.0 -object
iothread,id=iothread0 -device
virtio-scsi-pci,bus=bridge1,addr=0x1f,id=scsi0,iothread=iothread0 -drive
file=.../scratch/test.img,media=disk,if=none,cache=writeback,id=drive_sysdisk,format=qcow2
-device scsi-hd,drive=drive_sysdisk,bus=scsi0.0,id=sysdisk,bootindex=0
-nographic
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
tests/qemu-iotests/200 | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qemu-iotests/200 b/tests/qemu-iotests/200
index ddbdedc476..7e53bd7774 100755
--- a/tests/qemu-iotests/200
+++ b/tests/qemu-iotests/200
@@ -45,6 +45,10 @@ _supported_fmt qcow2 qed
_supported_proto file
_supported_os Linux
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
BACKING_IMG="${TEST_DIR}/backing.img"
TEST_IMG="${TEST_DIR}/test.img"
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v1 1/1] iotests: bypass s390x for case 200
In s390x, the case 200 failed as:
=== Starting QEMU VM ===
+QEMU_PROG: boot menu is not supported for this device type.
{"return": {}}
=== Sending stream/cancel, checking for SIGSEGV only ===
Failures: 200
Failed 1 of 1 tests
It was caused by the command which isn't supported by s390x now:
qemu-system-s390x -device pci-bridge,id=bridge1,chassis_nr=1,bus=pci.0 -object
iothread,id=iothread0 -device
virtio-scsi-pci,bus=bridge1,addr=0x1f,id=scsi0,iothread=iothread0 -drive
file=.../scratch/test.img,media=disk,if=none,cache=writeback,id=drive_sysdisk,format=qcow2
-device scsi-hd,drive=drive_sysdisk,bus=scsi0.0,id=sysdisk,bootindex=0
-nographic
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
tests/qemu-iotests/200 | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qemu-iotests/200 b/tests/qemu-iotests/200
index ddbdedc476..7e53bd7774 100755
--- a/tests/qemu-iotests/200
+++ b/tests/qemu-iotests/200
@@ -45,6 +45,10 @@ _supported_fmt qcow2 qed
_supported_proto file
_supported_os Linux
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
BACKING_IMG="${TEST_DIR}/backing.img"
TEST_IMG="${TEST_DIR}/test.img"
--
2.13.5
Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out
在 2017/10/16 20:31, Eric Blake 写道: On 10/16/2017 03:30 AM, Kevin Wolf wrote: Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben: The actual-size set in 191.out is wrong, which causes the test case failed and shall be 331776. The actual-size indicates the image's (e.g. t.qcow2.base) actual size defined by ImageInfo.actual_size. Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com> This is wrong. It may make the test work on your machine, but it breaks it on mine. The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter actual image size in 184 and 191". That, and ALL patches should be sent to qemu-de...@nongnu.org, in addition to whatever other lists are appropriate (you are correct that this patch should cc qemu-block, but you forgot qemu-devel). Thanks for your reminder. I'll pay attention next time. -- Regards QingFeng Hao
Re: [Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out
在 2017/10/16 16:30, Kevin Wolf 写道: Am 16.10.2017 um 07:23 hat QingFeng Hao geschrieben: The actual-size set in 191.out is wrong, which causes the test case failed and shall be 331776. The actual-size indicates the image's (e.g. t.qcow2.base) actual size defined by ImageInfo.actual_size. Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com> This is wrong. It may make the test work on your machine, but it breaks it on mine. The correct approach to fix this is Max' "[PATCH 2/2] iotests: Filter actual image size in 184 and 191". Thanks, I'll look into why it behaves differently in host file systems. Maybe it's related with block size etc. If that's true, Max's patch to filter it makes sense. Kevin -- Regards QingFeng Hao
[Qemu-block] [PATCH v1 1/1] iotests: fix the actual-size in 191.out
The actual-size set in 191.out is wrong, which causes the test case failed
and shall be 331776.
The actual-size indicates the image's (e.g. t.qcow2.base) actual size
defined by ImageInfo.actual_size.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
tests/qemu-iotests/191.out | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
index 7bfcd2d..d9a9e54 100644
--- a/tests/qemu-iotests/191.out
+++ b/tests/qemu-iotests/191.out
@@ -47,7 +47,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -136,7 +136,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -225,7 +225,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -242,7 +242,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.mid",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -283,7 +283,7 @@ wrote 65536/65536 bytes at offset 1048576
"virtual-size": 393216,
"filename": "TEST_DIR/t.qcow2.mid",
"format": "file",
-"actual-size": 397312,
+"actual-size": 331776,
"dirty-flag": false
},
"iops_wr": 0,
@@ -313,7 +313,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -351,7 +351,7 @@ wrote 65536/65536 bytes at offset 1048576
"virtual-size": 393216,
"filename": "TEST_DIR/t.qcow2.base",
"format": "file",
-"actual-size": 397312,
+"actual-size": 331776,
"dirty-flag": false
},
"iops_wr": 0,
@@ -450,7 +450,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -540,7 +540,7 @@ wrote 65536/65536 bytes at offset 1048576
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "
[Qemu-block] [PATCH v1 0/1] iotests: fix failed case 191
The actual-size 391312 in 191.out causes the test case 191 failed, after I
changed it to be 331776, the test succeeds.
The error output is:
191 - output mismatch (see 191.out.bad)
--- tests/qemu-iotests/191.out 2017-10-08 02:59:56.346200313 +0200
+++ tests/qemu-iotests/191.out.bad 2017-10-13 05:06:01.476200313 +0200
@@ -47,7 +47,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -136,7 +136,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -225,7 +225,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -242,7 +242,7 @@
"filename": "TEST_DIR/t.qcow2.mid",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -283,7 +283,7 @@
"virtual-size": 393216,
"filename": "TEST_DIR/t.qcow2.mid",
"format": "file",
-"actual-size": 397312,
+"actual-size": 331776,
"dirty-flag": false
},
"iops_wr": 0,
@@ -313,7 +313,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -351,7 +351,7 @@
"virtual-size": 393216,
"filename": "TEST_DIR/t.qcow2.base",
"format": "file",
-"actual-size": 397312,
+"actual-size": 331776,
"dirty-flag": false
},
"iops_wr": 0,
@@ -450,7 +450,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -540,7 +540,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
-"actual-size": 397312,
+"actual-size": 331776,
"format-specific": {
"type": "qcow2",
"data": {
@@ -647,7 +647,7 @@
"filename": "TEST_DIR/t.qcow2.base",
"cluster-size": 65536,
"format": "qcow2",
Re: [Qemu-block] [PATCH v2 0/3] iotests: cure s390x failures by switching to ccw/aliases
Reviewed-by: QingFeng Hao <ha...@linux.vnet.ibm.com> for the series of patches. Thanks 在 2017/9/13 17:10, Cornelia Huck 写道: Recent changes in s390x made pci support dependant on the zpci cpu feature, which is not provided on all models (and not on by default). This means we cannot instatiate pci devices on a standard qemu invocation for s390x. Moreover, the zpci instructions are not even wired up for tcg yet, so actually doing anything with those pci devices is bound to fail on tcg. For 040, 051, 139, and 182, this can be fixed by switching to virtio-ccw from virtio-pci on s390x. 051 also needs a bit of post-processing on the output. For 067, it is easier to switch to virtio aliases, which will pick virtio-ccw on s390x and virtio-pci elsewhere. It also exercises the aliasing path. v1->v2: - avoid adding new reference output by adding post-processing to 051 and switching to aliases for 067 Cornelia Huck (3): iotests: use -ccw on s390x for 040, 139, and 182 iotests: use -ccw on s390x for 051 iotests: use virtio aliases for 067 tests/qemu-iotests/040| 6 +- tests/qemu-iotests/051| 12 +++- tests/qemu-iotests/051.out| 2 +- tests/qemu-iotests/051.pc.out | 2 +- tests/qemu-iotests/067| 3 ++- tests/qemu-iotests/067.out| 2 +- tests/qemu-iotests/139| 12 ++-- tests/qemu-iotests/182| 13 +++-- 8 files changed, 42 insertions(+), 10 deletions(-) -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051
在 2017/9/6 15:32, Cornelia Huck 写道: On Wed, 6 Sep 2017 15:19:01 +0800 QingFeng Hao <ha...@linux.vnet.ibm.com> wrote: 在 2017/9/5 23:16, Cornelia Huck 写道: The default cpu model on s390x does not provide zPCI, which is not yet wired up on tcg. Moreover, virtio-ccw is the standard on s390x, so use the -ccw instead of the -pci versions of virtio devices on s390x. Provide an output file for s390x. Signed-off-by: Cornelia Huck <coh...@redhat.com> --- tests/qemu-iotests/051 | 9 +- tests/qemu-iotests/051.s390-ccw-virtio.out | 434 + 2 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index c8cfc764bc..f6ad0f4f0b 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -103,7 +103,14 @@ echo echo === Device without drive === echo -run_qemu -device virtio-scsi-pci -device scsi-hd +case "$QEMU_DEFAULT_MACHINE" in + s390-ccw-virtio) + run_qemu -device virtio-scsi-ccw -device scsi-hd + ;; + *) + run_qemu -device virtio-scsi-pci -device scsi-hd + ;; +esac echo echo === Overriding backing file === Regarding the new output file, I have a doubt that why there is no change on "check" script where the result check is located: if diff -w "$reference" $tmp.out >/dev/null 2>&1 Got it. It makes sense. The right output reference should be picked as of commit e166b4148208656635ea2fe39df8b1e875a34fb8 Author: Bo Tu <t...@linux.vnet.ibm.com> Date: Fri Jul 3 15:28:46 2015 +0800 qemu-iotests: qemu machine type support This patch adds qemu machine type support to the io test suite. Based on the qemu default machine type and alias of the default machine type the reference output file can now vary from the default to a machine specific output file if necessary. 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. Reviewed-by: Max Reitz <mre...@redhat.com> Reviewed-by: Michael Mueller <m...@linux.vnet.ibm.com> Reviewed-by: Sascha Silbe <si...@linux.vnet.ibm.com> Signed-off-by: Xiao Guang Chen <che...@linux.vnet.ibm.com> Signed-off-by: Kevin Wolf <kw...@redhat.com> -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH 2/3] iotests: use -ccw on s390x for 051
在 2017/9/5 23:16, Cornelia Huck 写道: The default cpu model on s390x does not provide zPCI, which is not yet wired up on tcg. Moreover, virtio-ccw is the standard on s390x, so use the -ccw instead of the -pci versions of virtio devices on s390x. Provide an output file for s390x. Signed-off-by: Cornelia Huck <coh...@redhat.com> --- tests/qemu-iotests/051 | 9 +- tests/qemu-iotests/051.s390-ccw-virtio.out | 434 + 2 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051 index c8cfc764bc..f6ad0f4f0b 100755 --- a/tests/qemu-iotests/051 +++ b/tests/qemu-iotests/051 @@ -103,7 +103,14 @@ echo echo === Device without drive === echo -run_qemu -device virtio-scsi-pci -device scsi-hd +case "$QEMU_DEFAULT_MACHINE" in + s390-ccw-virtio) + run_qemu -device virtio-scsi-ccw -device scsi-hd + ;; + *) + run_qemu -device virtio-scsi-pci -device scsi-hd + ;; +esac echo echo === Overriding backing file === Regarding the new output file, I have a doubt that why there is no change on "check" script where the result check is located: if diff -w "$reference" $tmp.out >/dev/null 2>&1 Thanks! diff --git a/tests/qemu-iotests/051.s390-ccw-virtio.out b/tests/qemu-iotests/051.s390-ccw-virtio.out new file mode 100644 index 00..7555f0b73e --- /dev/null +++ b/tests/qemu-iotests/051.s390-ccw-virtio.out [...] -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH 0/3] iotests: cure s390x failures by switching to ccw
在 2017/9/5 23:16, Cornelia Huck 写道:
Recent changes in s390x made pci support dependant on the zpci cpu
feature, which is not provided on all models (and not on by default).
This means we cannot instatiate pci devices on a standard qemu
invocation for s390x. Moreover, the zpci instructions are not even
wired up for tcg yet, so actually doing anything with those pci devices
is bound to fail on tcg.
Let's follow the existing example in 068 and switch to the (default)
virtio-ccw transport on s390x. The changes for 051 and 067 are split
out as they require adding an output file for s390x (the actual command
lines are part of the output).
We also found this error and YiMin suggested to change the code in ccw_init
as below:
if (pci_available) {
DeviceState *dev = qdev_create(NULL, TYPE_S390_PCI_HOST_BRIDGE);
...
}
We tested it and it can make the 5 cases passed.
How do you think this? :-)
Thanks!
Cornelia Huck (3):
iotests: use -ccw on s390x for 040, 139, and 182
iotests: use -ccw on s390x for 051
iotests: use -ccw on s390x for 067
tests/qemu-iotests/040 | 6 +-
tests/qemu-iotests/051 | 9 +-
tests/qemu-iotests/051.s390-ccw-virtio.out | 434 +++
tests/qemu-iotests/067 | 11 +-
tests/qemu-iotests/067.s390-ccw-virtio.out | 458 +
tests/qemu-iotests/139 | 12 +-
tests/qemu-iotests/182 | 13 +-
7 files changed, 936 insertions(+), 7 deletions(-)
create mode 100644 tests/qemu-iotests/051.s390-ccw-virtio.out
create mode 100644 tests/qemu-iotests/067.s390-ccw-virtio.out
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v4 0/1] virtio-scsi-ccw: fix iotest 068 for s390x
在 2017/7/5 23:15, Stefan Hajnoczi 写道: On Tue, Jul 04, 2017 at 03:23:49PM +0200, QingFeng Hao wrote: This commit fixes iotest 068 for s390x as s390x uses virtio-scsi-ccw. It's based on commit c324fd0a39c by Stefan Hajnoczi. Thanks! Change history: v4: Got Cornelia Huck's Reviewed-by and take the comment to change the commit message. v3: Take Christian Borntraeger and Cornelia Huck's comment to check if kvm is enabled in s390_assign_subch_ioeventfd instead of kvm_s390_assign_subch_ioeventfd to as the former is a general one. v2: Remove Stefan from sign-off list and change the patch's commit message according to Christian Borntraeger's comment. QingFeng Hao (1): virtio-scsi-ccw: use ioeventfd even when KVM is disabled hw/s390x/virtio-ccw.c | 2 +- target/s390x/cpu.h| 6 +- 2 files changed, 6 insertions(+), 2 deletions(-) I didn't realize s390 also has this old check. Thanks for fixing it! Thanks Stefan! Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com> -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v4 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/4 22:04, Christian Borntraeger 写道:
On 07/04/2017 03:23 PM, QingFeng Hao wrote:
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c ("virtio-pci: use ioeventfd even when KVM is disabled")
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
Currently we don't have an equivalent to "memory: emulate ioeventfd"
for ccw yet, but that this doesn't hurt and qemu-iotests 068 can pass with
skipping iothread arguments.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
I will take it via the s390-next tree.
thanks applied.
Ok, thanks.
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/cpu.h| 6 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9faca04b52..bdb9bdbc9d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1264,7 +1264,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+ return 0;
+}
}
static inline void s390_crypto_reset(void)
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v4 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c ("virtio-pci: use ioeventfd even when KVM is disabled")
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
Currently we don't have an equivalent to "memory: emulate ioeventfd"
for ccw yet, but that this doesn't hurt and qemu-iotests 068 can pass with
skipping iothread arguments.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <coh...@redhat.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/cpu.h| 6 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9faca04b52..bdb9bdbc9d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1264,7 +1264,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+return 0;
+}
}
static inline void s390_crypto_reset(void)
--
2.11.2
[Qemu-block] [PATCH v4 0/1] virtio-scsi-ccw: fix iotest 068 for s390x
This commit fixes iotest 068 for s390x as s390x uses virtio-scsi-ccw. It's based on commit c324fd0a39c by Stefan Hajnoczi. Thanks! Change history: v4: Got Cornelia Huck's Reviewed-by and take the comment to change the commit message. v3: Take Christian Borntraeger and Cornelia Huck's comment to check if kvm is enabled in s390_assign_subch_ioeventfd instead of kvm_s390_assign_subch_ioeventfd to as the former is a general one. v2: Remove Stefan from sign-off list and change the patch's commit message according to Christian Borntraeger's comment. QingFeng Hao (1): virtio-scsi-ccw: use ioeventfd even when KVM is disabled hw/s390x/virtio-ccw.c | 2 +- target/s390x/cpu.h| 6 +- 2 files changed, 6 insertions(+), 2 deletions(-) -- 2.11.2
Re: [Qemu-block] [PATCH v3 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/4 17:34, Cornelia Huck 写道:
On Tue, 4 Jul 2017 10:32:31 +0200
QingFeng Hao <ha...@linux.vnet.ibm.com> wrote:
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c (" virtio-pci: use ioeventfd even when KVM is disabled)
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
It might be good to add a sentence that we don't have an equivalent to
"memory: emulate ioeventfd" for ccw yet, but that this doesn't hurt.
Ok, I'll add it. thanks.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/cpu.h| 6 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9faca04b52..bdb9bdbc9d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1264,7 +1264,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+return 0;
+}
}
static inline void s390_crypto_reset(void)
Reviewed-by: Cornelia Huck <coh...@redhat.com>
Thanks!
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v3 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c (" virtio-pci: use ioeventfd even when KVM is disabled)
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/cpu.h| 6 +-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 9faca04b52..bdb9bdbc9d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1264,7 +1264,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+return 0;
+}
}
static inline void s390_crypto_reset(void)
--
2.11.2
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/4 15:06, Christian Borntraeger 写道:
On 07/04/2017 05:41 AM, QingFeng Hao wrote:
在 2017/7/3 18:20, Christian Borntraeger 写道:
On 07/03/2017 10:51 AM, QingFeng Hao wrote:
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c (" virtio-pci: use ioeventfd even when KVM is disabled)
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
thinking more about it. wouldnt it be better to do something like this instead
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 058ddad..cc47831 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1240,7 +1240,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+return 0;
+}
}
Thanks. It makes sense. I'll change it.
static inline void s390_crypto_reset(void)
FWIW, it seems that we (s390) do not have a functional equivalent function as
commit
8c56c1a592b5092d91da8d8943c1d6462a6f ("memory: emulate ioeventfd") , so we
will
not use the iothreads.
Ok, but might s390 have skipped the iothread arguments and passed the test, so
we could
still keep this test case?
Yes, lets just fix the test case. We can implement emulated ioeventfds later.
Fine, thanks!
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/3 19:48, Cornelia Huck 写道:
On Mon, 3 Jul 2017 09:38:36 +0200
QingFeng Hao <ha...@linux.vnet.ibm.com> wrote:
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit
8c56c1a592b5092d91da8d8943c1d6462a6f ("memory: emulate
ioeventfd") it has been possible to use ioeventfds in qtest or TCG
mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and
without iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void
virtio_ccw_device_realize(VirtioCcwDevice *dev, Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno, ccw_dev->devno.valid ?
"user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int
kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t
sch, .addr = sch, .len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
I'd prefer if you moved the kvm_enabled() check into
s390_assign_subch_ioeventfd().
Thanks and I'll change it just as Christian's comment.
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/3 18:20, Christian Borntraeger 写道:
On 07/03/2017 10:51 AM, QingFeng Hao wrote:
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c (" virtio-pci: use ioeventfd even when KVM is disabled)
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
thinking more about it. wouldnt it be better to do something like this instead
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 058ddad..cc47831 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -1240,7 +1240,11 @@ static inline int
s390_assign_subch_ioeventfd(EventNotifier *notifier,
uint32_t sch_id, int vq,
bool assign)
{
-return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+if (kvm_enabled()) {
+return kvm_s390_assign_subch_ioeventfd(notifier, sch_id, vq, assign);
+} else {
+return 0;
+}
}
Thanks. It makes sense. I'll change it.
static inline void s390_crypto_reset(void)
FWIW, it seems that we (s390) do not have a functional equivalent function as
commit
8c56c1a592b5092d91da8d8943c1d6462a6f ("memory: emulate ioeventfd") , so we
will
not use the iothreads.
Ok, but might s390 have skipped the iothread arguments and passed the
test, so we could
still keep this test case?
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v2 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
This patch is based on a similar patch from Stefan Hajnoczi -
commit c324fd0a39c (" virtio-pci: use ioeventfd even when KVM is disabled)
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
2.11.2
[Qemu-block] [PATCH v2 0/1] virtio-scsi-ccw: fix iotest 068 for s390x
This commit fixes iotest 068 for s390x as s390x uses virtio-scsi-ccw. It's based on commit c324fd0a39c by Stefan Hajnoczi. Thanks! Change history: v2: Remove Stefan from sign-off list and change the patch's commit message according to Christian Borntraeger's comment. QingFeng Hao (1): virtio-scsi-ccw: use ioeventfd even when KVM is disabled hw/s390x/virtio-ccw.c | 2 +- target/s390x/kvm.c| 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) -- 2.11.2
Re: [Qemu-block] [PATCH 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/3 16:21, Christian Borntraeger 写道:
On 07/03/2017 10:08 AM, QingFeng Hao wrote:
在 2017/7/3 15:41, Christian Borntraeger 写道:
On 07/03/2017 09:38 AM, QingFeng Hao wrote:
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
cut'n'paste mistake of adding Stefans signoff?
Otherwise it looks good.
I just want to mark that this patch is related with the former one from Stefan.
Is that ok to add this sign-off? thanks!
No, sign-off indicates who passes the patch along for integration, so only
Stefan
is allowed to add that - if he actually takes the patch. It is very important to
not mangle the sign-off-chain as it is actually used to track how a patch moved
from
the developer into the tree.
You can give credit to Stefan in the patch description - e.g. by saying in the
patch
description something like
based on a similar patch from Stefan Hajnoczi - commit c324fd0a39c ("
virtio-pci: use
ioeventfd even when KVM is disabled)
Thanks for your good explanation and I'll change the commit message.
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
Regards
QingFeng Hao
Re: [Qemu-block] [PATCH 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
在 2017/7/3 15:41, Christian Borntraeger 写道:
On 07/03/2017 09:38 AM, QingFeng Hao wrote:
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
cut'n'paste mistake of adding Stefans signoff?
Otherwise it looks good.
I just want to mark that this patch is related with the former one from
Stefan.
Is that ok to add this sign-off? thanks!
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
Regards
QingFeng Hao
[Qemu-block] [PATCH 1/1] virtio-scsi-ccw: use ioeventfd even when KVM is disabled
Do not check kvm_eventfds_enabled() when KVM is disabled since it
always returns 0. Since commit 8c56c1a592b5092d91da8d8943c1d6462a6f
("memory: emulate ioeventfd") it has been possible to use ioeventfds in
qtest or TCG mode.
This patch makes -device virtio-scsi-ccw,iothread=iothread0 work even
when KVM is disabled.
I have tested that virtio-scsi-ccw works under tcg both with and without
iothread.
This patch fixes qemu-iotests 068, which was accidentally merged early
despite the dependency on ioeventfd.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
---
hw/s390x/virtio-ccw.c | 2 +-
target/s390x/kvm.c| 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb9ff..35896eb007 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void virtio_ccw_device_realize(VirtioCcwDevice *dev,
Error **errp)
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index a3d00196f4..c37f9c3b9e 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2220,6 +2220,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier
*notifier, uint32_t sch,
.addr = sch,
.len = 8,
};
+if (!kvm_enabled()) {
+return 0;
+}
if (!kvm_check_extension(kvm_state, KVM_CAP_IOEVENTFD)) {
return -ENOSYS;
}
--
2.11.2
[Qemu-block] [PATCH 0/1] virtio-scsi-ccw: fix iotest 068 on s390x
This commit fixes iotest 068 for s390x as s390x uses virtio-scsi-ccw. The related commit is c324fd0a39c for other platforms by Stefan Hajnoczi. Thanks! QingFeng Hao (1): virtio-scsi-ccw: use ioeventfd even when KVM is disabled hw/s390x/virtio-ccw.c | 2 +- target/s390x/kvm.c| 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) -- 2.11.2
Re: [Qemu-block] [PULL 11/61] virtio-pci: use ioeventfd even when KVM is disabled
在 2017/6/28 18:22, Kevin Wolf 写道:
Am 28.06.2017 um 12:11 hat QingFeng Hao geschrieben:
在 2017/6/24 0:21, Kevin Wolf 写道:
From: Stefan Hajnoczi <stefa...@redhat.com>
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 8c56c1a592b5092d91da8d8943c1d6462a6f
("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 <m...@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Kevin Wolf <kw...@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 20d6a08..301920e 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;
}
This response is actually for mail thread "Re: [Qemu-devel] [PATCH
1/5] virtio-pci: use ioeventfd even when KVM is disabled"
which I didn't receive, sorry.
I also saw the failed case of 068 as Fam due to the same cause on
s390x and x86.
With this patch applied, no failure found. Further investigation
shows that the error is in
virtio_scsi_dataplane_setup:
if (!virtio_device_ioeventfd_enabled(vdev)) {
error_setg(errp, "ioeventfd is required for iothread");
return;
}
call flow is:
virtio_device_ioeventfd_enabled-->virtio_bus_ioeventfd_enabled
-->k->ioeventfd_enabled-->virtio_pci_ioeventfd_enabled
virtio_pci_ioeventfd_enabled checks flag
VIRTIO_PCI_FLAG_USE_IOEVENTFD which was
cleared in virtio_pci_realize if this patch isn't applied.
Yes, we know all of this. However, this patch is not correct and causes
'make check' failures on some platforms. The open question is where that
failure comes from. Before this is solved, the patch can't be applied.
Sorry that I found case 068 of the latest master still fails on s390x
(but passed
on x86) and the cause is that s390x uses "-device virtio-scsi-ccw"
instead of
"-device virtio-scsi-pci", so the change in virtio_ccw_device_realize is
also needed:
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 90d37cb..35896eb 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -711,7 +711,7 @@ static void
virtio_ccw_device_realize(VirtioCcwDevice *dev, Error
sch->cssid, sch->ssid, sch->schid, sch->devno,
ccw_dev->devno.valid ? "user-configured" : "auto-configured");
-if (!kvm_eventfds_enabled()) {
+if (kvm_enabled() && !kvm_eventfds_enabled()) {
dev->flags &= ~VIRTIO_CCW_FLAG_USE_IOEVENTFD;
}
I'll send out a patch for that. Thanks!
Kevin
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PULL 11/61] virtio-pci: use ioeventfd even when KVM is disabled
在 2017/6/28 18:22, Kevin Wolf 写道:
Am 28.06.2017 um 12:11 hat QingFeng Hao geschrieben:
在 2017/6/24 0:21, Kevin Wolf 写道:
From: Stefan Hajnoczi <stefa...@redhat.com>
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 8c56c1a592b5092d91da8d8943c1d6462a6f
("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 <m...@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Kevin Wolf <kw...@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 20d6a08..301920e 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;
}
This response is actually for mail thread "Re: [Qemu-devel] [PATCH
1/5] virtio-pci: use ioeventfd even when KVM is disabled"
which I didn't receive, sorry.
I also saw the failed case of 068 as Fam due to the same cause on
s390x and x86.
With this patch applied, no failure found. Further investigation
shows that the error is in
virtio_scsi_dataplane_setup:
if (!virtio_device_ioeventfd_enabled(vdev)) {
error_setg(errp, "ioeventfd is required for iothread");
return;
}
call flow is:
virtio_device_ioeventfd_enabled-->virtio_bus_ioeventfd_enabled
-->k->ioeventfd_enabled-->virtio_pci_ioeventfd_enabled
virtio_pci_ioeventfd_enabled checks flag
VIRTIO_PCI_FLAG_USE_IOEVENTFD which was
cleared in virtio_pci_realize if this patch isn't applied.
Yes, we know all of this. However, this patch is not correct and causes
'make check' failures on some platforms. The open question is where that
failure comes from. Before this is solved, the patch can't be applied.
Thanks Kevin. Maybe I am luck, I didn't encounter the failure when running
'make check' with this patch applied. thanks
Kevin
--
Regards
QingFeng Hao
Re: [Qemu-block] [PULL 11/61] virtio-pci: use ioeventfd even when KVM is disabled
在 2017/6/24 0:21, Kevin Wolf 写道:
From: Stefan Hajnoczi <stefa...@redhat.com>
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 8c56c1a592b5092d91da8d8943c1d6462a6f
("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 <m...@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Kevin Wolf <kw...@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 20d6a08..301920e 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;
}
This response is actually for mail thread "Re: [Qemu-devel] [PATCH 1/5]
virtio-pci: use ioeventfd even when KVM is disabled"
which I didn't receive, sorry.
I also saw the failed case of 068 as Fam due to the same cause on s390x
and x86.
With this patch applied, no failure found. Further investigation shows
that the error is in
virtio_scsi_dataplane_setup:
if (!virtio_device_ioeventfd_enabled(vdev)) {
error_setg(errp, "ioeventfd is required for iothread");
return;
}
call flow is:
virtio_device_ioeventfd_enabled-->virtio_bus_ioeventfd_enabled
-->k->ioeventfd_enabled-->virtio_pci_ioeventfd_enabled
virtio_pci_ioeventfd_enabled checks flag VIRTIO_PCI_FLAG_USE_IOEVENTFD
which was
cleared in virtio_pci_realize if this patch isn't applied.
Thanks!
--
Regards
QingFeng Hao
Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/7 20:18, Dr. David Alan Gilbert 写道: * QingFeng Hao (ha...@linux.vnet.ibm.com) wrote: 在 2017/6/6 20:49, Kevin Wolf 写道: Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben: I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't seem to remove a qemu_fclose() call there, but I can't see one left behind either. Was the file leaked before commit 660819b or am I missing something? I don't think so because loadvm_postcopy_handle_listen creates thread postcopy_ram_listen_thread and passes mis->from_src_file as its arg, which will be closed by migration_incoming_state_destroy. What confuses me is in the series function calls of qemu_loadvm_state_main etc, argument f looks to be redundant as mis already contains from_src_file which equals to f. In postcopy qemu_loadvm_state_main is called with two different file arguments but the same mis argument; see loadvm_handle_cmd_packaged for the other case where it's called on a packaged-file blob. yes, you are right, I missed that one. :) Furthermore, mis may be also redundant as it can be got via migration_incoming_get_current. Thanks! We keep changing our minds about the preferred style. Sometimes we think it's best to pass the pointer, sometimes we think it's best to call get_current. Got it. Thanks! Dave Kevin -- Regards QingFeng Hao -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Regards QingFeng Hao
Re: [Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 20:49, Kevin Wolf 写道:
Am 06.06.2017 um 07:24 hat QingFeng Hao geschrieben:
In load_snapshot, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Peter Xu <pet...@redhat.com>
Dave, as you only gave R-b rather than merging the patch, should this be
merged through the block tree?
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
Did we check other callers of migration_incoming_state_destroy()?
For example, qmp_xen_load_devices_state() looks suspicious, too.
Good reminder! Yes, I checked it and there is no assignment of
from_src_file there and f
is opened locally, so I think that qemu_fclose doesn't impact
migration_incoming_state_destroy.
migration_incoming_state_destroy is called in 4 places:
process_incoming_migration_bh, postcopy_ram_listen_thread,
qmp_xen_load_devices_state
and load_snapshot. process_incoming_migration_bh is launched by
process_incoming_migration_co
whose qemu_fclose is removed by commit 660819b.
For postcopy_ram_listen_thread, I didn't see where it calls qemu_fclose.
Actually to simplify the check for the problem, I just searched where
from_src_file
is assigned to and got 2 places: process_incoming_migration_co and
load_snapshot.
qemu_fclose in the first function is removed by commit 660819b, and
qemu_fclose in the
second is removed by this one. I think a potential risk might be opaque
is closed
by anywhere else than process_incoming_migration_co, but there is legacy
qemu_close
before commit 660819b, so the risk might be low? thanks :)
I can't tell for postcopy_ram_listen_thread() - commit 660819b didn't
seem to remove a qemu_fclose() call there, but I can't see one left
behind either. Was the file leaked before commit 660819b or am I
missing something?
I don't think so because loadvm_postcopy_handle_listen creates thread
postcopy_ram_listen_thread
and passes mis->from_src_file as its arg, which will be closed by
migration_incoming_state_destroy.
What confuses me is in the series function calls of
qemu_loadvm_state_main etc, argument f looks
to be redundant as mis already contains from_src_file which equals to f.
Furthermore, mis may be
also redundant as it can be got via migration_incoming_get_current. Thanks!
Kevin
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v2 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068
Hi all, This patch is to fix the migration bug found by qemu-iotests case 068 and based on upstream master's commit: 199e19ee53: Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging. The bug was introduced by commit "660819b migration: shut src return path unconditionally". Change Log(v2): Got reviewed-by from Dr. David Alan Gilbert and Peter Xu. Thanks! QingFeng Hao (1): qemu/migration: fix the double free problem on from_src_file migration/savevm.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.2
[Qemu-block] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
In load_snapshot, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) (
if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Reviewed-by: Peter Xu <pet...@redhat.com>
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
--
2.11.2
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:50, Peter Xu 写道:
On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
在 2017/6/6 11:03, Peter Xu 写道:
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
Thanks QingFeng for the fix!
Reviewed-by: Peter Xu <pet...@redhat.com>
Though here instead of removing the fclose(), not sure whether this is
nicer:
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUFile *f;
int ret;
AioContext *aio_context;
-MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot()) {
error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
}
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-mis->from_src_file = f;
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
Thanks Peter. I have a doubt on this change because I see there are several
places
referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
seems to
get byte from the file and it's called by qemu_loadvm_state.
Anyway, you remind me for the description that is "In load_snapshot" rather
than
"In load_vmstate". thanks
Oh I didn't really notice that. :)
It shouldn't affect imho since load snapshot won't trigger postcopy
codes. But sure current fix is good enough for me at least.
Ok, thanks!
Thanks,
--
Regards
QingFeng Hao
Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:03, Peter Xu 写道:
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
Thanks QingFeng for the fix!
Reviewed-by: Peter Xu <pet...@redhat.com>
Though here instead of removing the fclose(), not sure whether this is
nicer:
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f5..1feb519 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUFile *f;
int ret;
AioContext *aio_context;
-MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot()) {
error_setg(errp,
@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
}
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
-mis->from_src_file = f;
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
Thanks Peter. I have a doubt on this change because I see there are
several places
referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and
it seems to
get byte from the file and it's called by qemu_loadvm_state.
Anyway, you remind me for the description that is "In load_snapshot"
rather than
"In load_vmstate". thanks
Since I see we setup from_src_file but we don't really use it. If we
remove that line, we can also remove the
migration_incoming_get_current() call altogether.
Either way work for me. So my r-b stands always. :-)
--
Regards
QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/5 19:08, Dr. David Alan Gilbert 写道:
* QingFeng Hao (ha...@linux.vnet.ibm.com) wrote:
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
Thanks!
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Thanks David!
migration_incoming_state_destroy();
--
2.11.2
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
In load_vmstate, mis->from_src_file is freed twice, the first free is by
qemu_fclose, the second is by migration_incoming_state_destroy and
it causes Illegal instruction exception. The fix is just to remove the
first free.
This problem is found by qemu-iotests case 068 since commit
"660819b migration: shut src return path unconditionally". The error is:
068 1s ... - output mismatch (see 068.out.bad)
--- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
+++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200
@@ -6,6 +6,8 @@
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
(qemu) quit
+./common.config: line 107: 242472 Illegal instruction (core dumped) (
if [ -n "${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/savevm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 9c320f59d0..853e14e34e 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
-qemu_fclose(f);
aio_context_release(aio_context);
migration_incoming_state_destroy();
--
2.11.2
[Qemu-block] [PATCH v1 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068
Hi all, This patch is to fix the migration bug found by qemu-iotests case 068 and based on upstream master's commit: cb8b8ef4578: Merge remote-tracking branch 'remotes/elmarco/tags/chrfe-pull-request' into staging. The bug was introduced by commit "660819b migration: shut src return path unconditionally". Thanks! QingFeng Hao (1): qemu/migration: fix the double free problem on from_src_file migration/savevm.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.2
Re: [Qemu-block] [PATCH v1 1/1] vmstate: fix failed iotests case 68 and 91
在 2017/3/16 16:01, Juan Quintela 写道: QingFeng Hao <ha...@linux.vnet.ibm.com> wrote: This problem affects s390x only if we are running without KVM. Basically, S390CPU.irqstate is unused if we do not use KVM, and thus no buffer is allocated. This causes size=0, first_elem=NULL and n_elems=1 in vmstate_load_state and vmstate_save_state. And the assert fails. With this fix we can go back to the old behavior and support VMS_VBUFFER with size 0 and nullptr. Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> queued Thanks! -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] vmstate: fix failed iotests case 68 and 91
在 2017/3/14 22:13, Dr. David Alan Gilbert 写道:
* QingFeng Hao (ha...@linux.vnet.ibm.com) wrote:
This problem affects s390x only if we are running without KVM.
Basically, S390CPU.irqstate is unused if we do not use KVM,
and thus no buffer is allocated.
This causes size=0, first_elem=NULL and n_elems=1 in
vmstate_load_state and vmstate_save_state. And the assert fails.
With this fix we can go back to the old behavior and support
VMS_VBUFFER with size 0 and nullptr.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
Thanks, and fixes problem with vmxnet3 migration.
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Thank you, Dave!
Dave
---
migration/vmstate.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..7b4a607 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -109,7 +109,7 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
-assert(first_elem || !n_elems);
+assert(first_elem || !n_elems || !size);
}
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
@@ -117,7 +117,7 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
if (field->flags & VMS_ARRAY_OF_POINTER) {
curr_elem = *(void **)curr_elem;
}
-if (!curr_elem) {
+if (!curr_elem && size) {
/* if null pointer check placeholder and do not follow */
assert(field->flags & VMS_ARRAY_OF_POINTER);
ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
@@ -325,7 +325,7 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
-assert(first_elem || !n_elems);
+assert(first_elem || !n_elems || !size);
}
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
@@ -336,7 +336,7 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
assert(curr_elem);
curr_elem = *(void **)curr_elem;
}
-if (!curr_elem) {
+if (!curr_elem && size) {
/* if null pointer write placeholder and do not follow */
assert(field->flags & VMS_ARRAY_OF_POINTER);
vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
--
1.8.3.1
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
--
Regards
QingFeng Hao
[Qemu-block] [PATCH v1 1/1] vmstate: fix failed iotests case 68 and 91
This problem affects s390x only if we are running without KVM.
Basically, S390CPU.irqstate is unused if we do not use KVM,
and thus no buffer is allocated.
This causes size=0, first_elem=NULL and n_elems=1 in
vmstate_load_state and vmstate_save_state. And the assert fails.
With this fix we can go back to the old behavior and support
VMS_VBUFFER with size 0 and nullptr.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com>
---
migration/vmstate.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..7b4a607 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -109,7 +109,7 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
-assert(first_elem || !n_elems);
+assert(first_elem || !n_elems || !size);
}
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
@@ -117,7 +117,7 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
if (field->flags & VMS_ARRAY_OF_POINTER) {
curr_elem = *(void **)curr_elem;
}
-if (!curr_elem) {
+if (!curr_elem && size) {
/* if null pointer check placeholder and do not follow */
assert(field->flags & VMS_ARRAY_OF_POINTER);
ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
@@ -325,7 +325,7 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
-assert(first_elem || !n_elems);
+assert(first_elem || !n_elems || !size);
}
for (i = 0; i < n_elems; i++) {
void *curr_elem = first_elem + size * i;
@@ -336,7 +336,7 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
assert(curr_elem);
curr_elem = *(void **)curr_elem;
}
-if (!curr_elem) {
+if (!curr_elem && size) {
/* if null pointer write placeholder and do not follow */
assert(field->flags & VMS_ARRAY_OF_POINTER);
vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
--
1.8.3.1
[Qemu-block] [PATCH v1 0/1] vmstate: fix failed iotests case 68 and 91
Hi All, This patch is to fix the failed iotests case 68 and 91 and has been tested. It's based on commit dd4d2578215 "Merge remote-tracking branch 'remotes/kraxel/tags/pull-fixes-20170309-1' into staging" and according to Halil and Dave's comments. Also thanks for Fam and Kevin. QingFeng Hao (1): vmstate: fix failed iotests case 68 and 91 migration/vmstate.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) -- 1.8.3.1
Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91
在 2017/3/9 19:45, Halil Pasic 写道: On 03/09/2017 03:55 AM, QingFeng Hao wrote: 在 2017/3/8 19:33, Halil Pasic 写道: On 03/08/2017 08:05 AM, QingFeng Hao wrote: 在 2017/3/7 18:19, Halil Pasic 写道: On 03/07/2017 11:05 AM, Kevin Wolf wrote: Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben: On 03/07/2017 10:29 AM, Kevin Wolf wrote: Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben: [..] The question is: can we do that change and what the second assert of field's flags is used for? The assert is there to guard against unintended use of this nullptr-marker mechanism. I have tested so this should work. By the way a vmstate test covering this corner-case would be a good idea too (IMHO). Would you like to write one? Sorry, I don't know how to write that test case. Can you please write one? thanks! The test is just nice to have, but we definitely need the fix! Ok, I'll send out the discussed fix patch. Halil -- Regards QingFeng Hao
Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91
在 2017/3/8 19:33, Halil Pasic 写道:
On 03/08/2017 08:05 AM, QingFeng Hao wrote:
在 2017/3/7 18:19, Halil Pasic 写道:
On 03/07/2017 11:05 AM, Kevin Wolf wrote:
Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
On 03/07/2017 10:29 AM, Kevin Wolf wrote:
Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
I am not very clear about the logic in vmstate.c, but from its context in
vmstate_save_state, it seems size should not be 0, otherwise the followed
for loop will keep working on the same element. So I just add a simple
check to pass that case, not sure if it's right but it can pass iotest
case 68 and 91 now.
The iotest's failed output is:
068 1s ... - output mismatch (see 068.out.bad)
---
/home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out
2017-03-06 05:52:24.817328899 +0100
+++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
@@ -3,9 +3,13 @@
=== Saving and reloading a VM state to/from a qcow2 image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion
`first_elem || !n_elems' failed.
+./common.config: line 109: 52497 Aborted ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/vmstate.c | 8
1 file changed, 8 insertions(+)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..ff28dde 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
+if (size == 0) {
+field++;
+continue;
+}
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
@@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
int64_t old_offset, written_bytes;
QJSON *vmdesc_loop = vmdesc;
+if (size == 0) {
+field++;
+continue;
+}
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
This is really a live migration fix, so I'm adding Juan and Dave to CC.
You are right, this is migration stuff and has very little to do with
qemu-block.
I suspect the real question is why a field with size 0 was even stored
in the vmstate to begin with.
I have looked onto the issue. It affects s390x only if we
are running without KVM. Basically, S390CPU.irqstate is unused
if we do not use KVM, and thus no buffer is allocated.
IMHO this is a missing field and the cleaner way to handle such
missing fields is exist. However this used to work, and I recommended
QuiFeng Hao to discuss the problem upstream.
By the way, I think, if we want to go back to the old behavior
and support VMS_VBUFFER with size 0 and nullptr, a much
cleaner way to do the fix is to change the assert to:
assert(first_elem || !n_elems || !size)
Obviously the other clean way to fix is to implement exists.
If you're right that this specific vmstate was valid in earlier
versions, then I think it's clear that we need to make it work again.
Otherwise we're breaking migration from old versions.
Not really. We would not break migration because nothing was written to
the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
'debug only', and does not affect migration compatibility.
IMHO it is an API question. I would have said, there is no data, therefore
there is no field if it's from scratch. But with prior history, I agree
with Dave, we should restore old behavior -- which was changed unintentionally
because I made a wrong assumption.
Halil,
Unfortunately, another assert failed after I change the code as below in
vmstate_save_state and vmstate_load_state:
assert(first_elem || !n_elems || !size);
The error is:
+qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state: Assertion
`field->flags & VMS_ARRAY_OF_POINTER' failed.
It's the code as below:
if (!curr_elem) {
Sorry, I failed to mention this yesterday. You should also do
- if (!curr_elem) {
+ if (!curr_elem && size) {
yes, this works per my test!
/* if null pointer write placeholder and do not follow
*/
assert(field->flags & VMS_ARRAY_OF_POINTER);
After debug, I found that the assert failure was still of irqstate and its
field flags is 0x102(VMS_VBUFFER | VMS_POINTER
Re: [Qemu-block] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91
在 2017/3/7 18:19, Halil Pasic 写道:
On 03/07/2017 11:05 AM, Kevin Wolf wrote:
Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
On 03/07/2017 10:29 AM, Kevin Wolf wrote:
Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
I am not very clear about the logic in vmstate.c, but from its context in
vmstate_save_state, it seems size should not be 0, otherwise the followed
for loop will keep working on the same element. So I just add a simple
check to pass that case, not sure if it's right but it can pass iotest
case 68 and 91 now.
The iotest's failed output is:
068 1s ... - output mismatch (see 068.out.bad)
---
/home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out
2017-03-06 05:52:24.817328899 +0100
+++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
@@ -3,9 +3,13 @@
=== Saving and reloading a VM state to/from a qcow2 image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion
`first_elem || !n_elems' failed.
+./common.config: line 109: 52497 Aborted ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/vmstate.c | 8
1 file changed, 8 insertions(+)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..ff28dde 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
+if (size == 0) {
+field++;
+continue;
+}
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
@@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
int64_t old_offset, written_bytes;
QJSON *vmdesc_loop = vmdesc;
+if (size == 0) {
+field++;
+continue;
+}
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
This is really a live migration fix, so I'm adding Juan and Dave to CC.
You are right, this is migration stuff and has very little to do with
qemu-block.
I suspect the real question is why a field with size 0 was even stored
in the vmstate to begin with.
I have looked onto the issue. It affects s390x only if we
are running without KVM. Basically, S390CPU.irqstate is unused
if we do not use KVM, and thus no buffer is allocated.
IMHO this is a missing field and the cleaner way to handle such
missing fields is exist. However this used to work, and I recommended
QuiFeng Hao to discuss the problem upstream.
By the way, I think, if we want to go back to the old behavior
and support VMS_VBUFFER with size 0 and nullptr, a much
cleaner way to do the fix is to change the assert to:
assert(first_elem || !n_elems || !size)
Obviously the other clean way to fix is to implement exists.
If you're right that this specific vmstate was valid in earlier
versions, then I think it's clear that we need to make it work again.
Otherwise we're breaking migration from old versions.
Not really. We would not break migration because nothing was written to
the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
'debug only', and does not affect migration compatibility.
IMHO it is an API question. I would have said, there is no data, therefore
there is no field if it's from scratch. But with prior history, I agree
with Dave, we should restore old behavior -- which was changed unintentionally
because I made a wrong assumption.
Halil,
Unfortunately, another assert failed after I change the code as below in
vmstate_save_state and vmstate_load_state:
assert(first_elem || !n_elems || !size);
The error is:
+qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state:
Assertion `field->flags & VMS_ARRAY_OF_POINTER' failed.
It's the code as below:
if (!curr_elem) {
/* if null pointer write placeholder and do not
follow */
assert(field->flags & VMS_ARRAY_OF_POINTER);
After debug, I found that the assert failure was still of irqstate and
its field flags is 0x102(VMS_VBUFFER | VMS_POINTER),
while VMS_ARRAY_OF_POINTER is 0x040. The flags's value matches Dave's
former assumption
on machine.c although machine.c wasn't compiled, which also confuses me.
Then I commented out that assert(field->flags & VMS_ARRAY_OF_POINTER)
and the case 68 & 91
can both work
Re: [Qemu-block] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91
在 2017/3/7 14:37, Fam Zheng 写道:
On Tue, 03/07 03:53, QingFeng Hao wrote:
Hi All,
I am not sure if the fix is correct because I am not very clear about the
logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
due to the assert. So just send this draft patch for your comments!
The patch is based on commit 17783ac828a "Merge remote-tracking branch
'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".
I cannot reproduce the failure on either 17783ac828a or current head
56b51708e9e. Both passes for me. I wonder where do you get the size=0.
The error happens when running "savevm 0" in case 068. It can be
manually reproduced
by "./check -qcow2 68" or "./s390x-softmmu/qemu-system-s390x -nodefaults \
-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak", then
type
"savevm 0". t.img.bak is the backup image for t.img generated by 068.
I added the print in vmstate_save_state:
QJSON *vmdesc_loop = vmdesc;
+ error_report("haoqf:%s:opaque:%p, offset:%lx, size:%d, field name:%s,
vname:%s\n", __FUNCTION__, opaque, field->offset, size, field->name,
vmsd->name);
And here is the test log:
haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122e1, size:1,
field name:env.sigp_order, vname:cpu
haoqf:vmstate_size: field size:4, offset:0
haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:12300, size:4,
field name:irqstate_saved_size, vname:cpu
haoqf:vmstate_size: field size:0, offset:74496
haoqf:vmstate_size: calculated size:0
haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122f8, size:0,
field name:irqstate, vname:cpu
haoqf:vmstate_save_state:firstelem:(nil), elements: 1
qemu-system-s390x: ../migration/vmstate.c:336: vmstate_save_state:
Assertion `first_elem || !n_elems' failed.
Aborted (core dumped)
I also did the test for x86 with: "./x86_64-softmmu/qemu-system-x86_64
-nodefaults \
-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak",
and then ran "savevm 0", but it didn't core and the size are all non-zero:
haoqf:vmstate_save calling vmstate_save_state:
haoqf:vmstate_size: field size:4, offset:0
haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:4, size:4, field
name:size, vname:globalstate
haoqf:vmstate_size: field size:100, offset:0
haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:8, size:100, field
name:runstate, vname:globalstate
haoqf:vmstate_save:called vmstate_save_state
So probably x86 doesn't have this problem.
Fam
--
Regards
QingFeng Hao
[Qemu-block] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91
Hi All, I am not sure if the fix is correct because I am not very clear about the logic in vmstate.c. From my test, once size=0, the iotests case 68 failed due to the assert. So just send this draft patch for your comments! The patch is based on commit 17783ac828a "Merge remote-tracking branch 'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging". Thanks! QingFeng Hao (1): vmstate: fix the failure of iotests cases 68 and 91 migration/vmstate.c | 8 1 file changed, 8 insertions(+) -- 2.8.4
[Qemu-block] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91
I am not very clear about the logic in vmstate.c, but from its context in
vmstate_save_state, it seems size should not be 0, otherwise the followed
for loop will keep working on the same element. So I just add a simple
check to pass that case, not sure if it's right but it can pass iotest
case 68 and 91 now.
The iotest's failed output is:
068 1s ... - output mismatch (see 068.out.bad)
---
/home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out
2017-03-06 05:52:24.817328899 +0100
+++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
@@ -3,9 +3,13 @@
=== Saving and reloading a VM state to/from a qcow2 image ===
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion
`first_elem || !n_elems' failed.
+./common.config: line 109: 52497 Aborted ( if [ -n
"${QEMU_NEED_PID}" ]; then
+echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm 0
-(qemu) quit
+qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) quit
*** done
091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
--- tests/qemu-iotests/091.out 2016-08-30 12:35:04.207683276 +0200
+++ 091.out.bad 2017-03-06 13:08:03.717135426 +0100
@@ -11,18 +11,23 @@
vm1: qemu-io disk write complete
vm1: live migration started
-vm1: live migration completed
-
-=== VM 2: Post-migration, write to disk, verify running ===
-
-vm2: qemu-io disk write complete
-vm2: qemu process running successfully
-vm2: flush io, and quit
-Check image pattern
-read 4194304/4194304 bytes at offset 0
-4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Running 'qemu-img check -r all $TEST_IMG'
-No errors were found on the image.
-80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
-Image end offset: 5570560
-*** done
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+./common.qemu: line 110: write error: Broken pipe
+Timeout waiting for completed on handle 0
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
migration/vmstate.c | 8
1 file changed, 8 insertions(+)
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..ff28dde 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const
VMStateDescription *vmsd,
int i, n_elems = vmstate_n_elems(opaque, field);
int size = vmstate_size(opaque, field);
+if (size == 0) {
+field++;
+continue;
+}
vmstate_handle_alloc(first_elem, field, opaque);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
@@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const
VMStateDescription *vmsd,
int64_t old_offset, written_bytes;
QJSON *vmdesc_loop = vmdesc;
+if (size == 0) {
+field++;
+continue;
+}
trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
if (field->flags & VMS_POINTER) {
first_elem = *(void **)first_elem;
--
2.8.4
[Qemu-block] [PATCH v1 0/1] iotests: Fix a problem in common.filter
Hi all, The patch is based on master branch's commit: 6a928d2 Update version for v2.8.0-rc4 release Test case 144 will fail if I set TEST_DIR=/tmp. The cause is that TEST_DIR duplicates with 144's test image name tmp.qcow2 and common.filter will over replace the path name. Thanks! QingFeng Hao QingFeng Hao (1): iotests: Fix a problem in common.filter tests/qemu-iotests/common.filter | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.8.4
[Qemu-block] [PATCH v1 1/1] iotests: Fix a problem in common.filter
If TEST_DIR is set to /tmp, test case 144 will fail. The reason is that
TEST_DIR duplicates with 144's test image name tmp.qcow2.
when 144 is testing $TEST_DIR/tmp.qcow2, it wants to replace
$TEST_DIR/tmp.qcow2 to TEST_DIR/tmp.qcow2, but actually it will fail
and get TEST_DIRTEST_DIR.qcow2 in this case.
The fix is just to modify the code to replace $TEST_DIR/ with TEST_DIR/.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
---
tests/qemu-iotests/common.filter | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 240ed06..d29e965 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -35,7 +35,7 @@ _filter_generated_node_ids()
# replace occurrences of the actual TEST_DIR value with TEST_DIR
_filter_testdir()
{
-sed -e "s#$TEST_DIR#TEST_DIR#g"
+sed -e "s#$TEST_DIR\/#TEST_DIR\/#g"
}
# replace occurrences of the actual IMGFMT value with IMGFMT
--
2.8.4
[Qemu-block] [PATCH v3 1/1] block/vmdk: Fix the endian problem of buf_len and lba
The problem was triggered by qemu-iotests case 055. It failed when it
was comparing the compressed vmdk image with original test.img.
The cause is that buf_len in vmdk_write_extent wasn't converted to
little-endian before it was stored to disk. But later vmdk_read_extent
read it and converted it from little-endian to cpu endian.
If the cpu is big-endian like s390, the problem will happen and
the data length read by vmdk_read_extent will become invalid!
The fix is to add the conversion in vmdk_write_extent, meanwhile,
repair the endianness problem of lba field which shall also be converted
to little-endian before storing to disk.
Cc: qemu-sta...@nongnu.org
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Jing Liu <liuj...@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kw...@redhat.com>
Reviewed-by: Fam Zheng <f...@redhat.com>
---
block/vmdk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a11c27a..26e5f95 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1354,8 +1354,8 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
goto out;
}
-data->lba = offset >> BDRV_SECTOR_BITS;
-data->size = buf_len;
+data->lba = cpu_to_le64(offset >> BDRV_SECTOR_BITS);
+data->size = cpu_to_le32(buf_len);
n_bytes = buf_len + sizeof(VmdkGrainMarker);
iov = (struct iovec) {
--
2.8.4
[Qemu-block] [PATCH v3 0/1] qemu: fix the bug reported by qemu-iotests case 055
Hi all, v3 doesn't have code change but got the reviewed-by from Fam Zheng(thanks to Fam). v2 includes changes due to review comments by Kevin Wolf(thanks to Kevin). v3: * Got reviewed-by from Fam Zheng. * Based on master's commit: 6a928d25b6: Update version for v2.8.0-rc4 release v2: * Add endian conversion for lba field in vmdk_write_extent. * Based on master's commit: 00227fefd205: Update version for v2.8.0-rc1 release v1: * Add patch to fix the bug reported by qemu-iotests case 055. Jing Liu and I found the cause was in vmdk and the fix is in the followed patch. Based on master's commit: 00227fefd205: Update version for v2.8.0-rc1 release Upstream master's qemu-iotests case 055 reports the following error: +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument Thanks! QingFeng Hao Cc: qemu-sta...@nongnu.org QingFeng Hao (1): block/vmdk: Fix the endian problem of buf_len and lba block/vmdk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.8.4
[Qemu-block] [PATCH v2 1/1] block/vmdk: Fix the endian problem of buf_len and lba
The problem was triggered by qemu-iotests case 055. It failed when it
was comparing the compressed vmdk image with original test.img.
The cause is that buf_len in vmdk_write_extent wasn't converted to
little-endian before it was stored to disk. But later vmdk_read_extent
read it and converted it from little-endian to cpu endian.
If the cpu is big-endian like s390, the problem will happen and
the data length read by vmdk_read_extent will become invalid!
The fix is to add the conversion in vmdk_write_extent, meanwhile,
repair the endianness problem of lba field which shall also be converted
to little-endian before storing to disk.
Cc: qemu-sta...@nongnu.org
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Jing Liu <liuj...@linux.vnet.ibm.com>
Signed-off-by: Kevin Wolf <kw...@redhat.com>
---
block/vmdk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a11c27a..26e5f95 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1354,8 +1354,8 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
goto out;
}
-data->lba = offset >> BDRV_SECTOR_BITS;
-data->size = buf_len;
+data->lba = cpu_to_le64(offset >> BDRV_SECTOR_BITS);
+data->size = cpu_to_le32(buf_len);
n_bytes = buf_len + sizeof(VmdkGrainMarker);
iov = (struct iovec) {
--
2.8.4
[Qemu-block] [PATCH v2 0/1] qemu: fix the bug reported by qemu-iotests case 055
Hi all, v2 includes changes due to review comments by Kevin Wolf(thanks to Kevin). v2: * Add endian conversion for lba field in vmdk_write_extent. Based on master's commit: 00227fefd205: Update version for v2.8.0-rc1 release v1: * Add patch to fix the bug reported by qemu-iotests case 055. Jing Liu and I found the cause was in vmdk and the fix is in the followed patch. Based on master's commit: 00227fefd205: Update version for v2.8.0-rc1 release Upstream master's qemu-iotests case 055 reports the following error: +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument Thanks! QingFeng Hao (1): block/vmdk: Fix the endian problem of buf_len and lba block/vmdk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.8.4
[Qemu-block] [Qemu-devel] [PATCH v1 0/1] qemu: fix the bug reported by qemu-iotests case 055
Hi all, This patch is to fix the bug reported by qemu-iotests case 055 and based on upstream master's commit: 00227fefd205: Update version for v2.8.0-rc1 release Jing Liu and I found the cause was in vmdk and the fix is in the followed patch. Thanks! Upstream master's qemu-iotests case 055 reports the following error: +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument +qemu-img: Error while reading offset 0 of tests/qemu-iotests/scratch/blockdev-target.img: Invalid argument QingFeng Hao (1): block/vmdk: Fix the endian problem of buf_len block/vmdk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.8.4
[Qemu-block] [Qemu-devel] [PATCH v1 1/1] block/vmdk: Fix the endian problem of buf_len
The problem was triggered by qemu-iotests case 055. It failed when it
was comparing the compressed vmdk image with original test.img.
The cause is that buf_len in vmdk_write_extent wasn't converted to
little-endian before it was stored to disk. But later vmdk_read_extent
read it and converted it from little-endian to cpu endian.
If the cpu is big-endian like s390, the problem will happen and
the data length read by vmdk_read_extent will become invalid!
The fix is to add the conversion in vmdk_write_extent.
Signed-off-by: QingFeng Hao <ha...@linux.vnet.ibm.com>
Signed-off-by: Jing Liu <liuj...@linux.vnet.ibm.com>
---
block/vmdk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index a11c27a..bf6667f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1355,7 +1355,7 @@ static int vmdk_write_extent(VmdkExtent *extent, int64_t
cluster_offset,
}
data->lba = offset >> BDRV_SECTOR_BITS;
-data->size = buf_len;
+data->size = cpu_to_le32(buf_len);
n_bytes = buf_len + sizeof(VmdkGrainMarker);
iov = (struct iovec) {
--
2.8.4
