Re: [PATCH v4 00/16] 64bit block-layer: part I
02.02.2021 05:56, Eric Blake wrote:
On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
We want 64bit write-zeroes, and for this, convert all io functions to
64bit.
We chose signed type, to be consistent with off_t (which is signed) and
with possibility for signed return type (where negative value means
error).
Please refer to initial cover-letter
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
for more info.
v4: I found, that some more work is needed for block/block-backend, so
decided to make partI, converting block/io
v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
for BDRV_MAX_LENGTH
changes:
01-05: new
06: add Alberto's r-b
07: new
08-16: rebase, add new-style request check, improve commit-msg, drop r-bs
I had planned to send a pull request for this series today, but ran into
a snag. Without this series applied, './check -qcow2' fails 030, 185,
and 297. With it applied, I now also get a failure in 206. I'm trying
to bisect which patch caused the problem, but here's the failure:
206 fail [20:54:54] [20:55:01] 6.9s (last: 6.7s) output
mismatch (see 206.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/206.out
+++ 206.out.bad
@@ -180,7 +180,7 @@
{"execute": "blockdev-create", "arguments": {"job-id": "job0",
"options": {"driver": "qcow2", "file": "node0", "size":
9223372036854775296}}}
{"return": {}}
-Job failed: Could not resize image: Required too big image size, it
must be not greater than 9223372035781033984
+Job failed: Could not resize image: offset(9223372036854775296) exceeds
maximum(9223372035781033984)
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
Looks like it is just a changed error message, so I can touch up the
correct patch and then repackage the pull request tomorrow (it's too
late for me today). Oh, and the 0 exit status of ./check when a test
fails is something I see you already plan on fixing...
Yes, Kevin have already sent a pull with "iotests: check: return 1 on failure"
--
Best regards,
Vladimir
Re: [PATCH v4 00/16] 64bit block-layer: part I
On 12/11/20 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> We want 64bit write-zeroes, and for this, convert all io functions to
> 64bit.
>
> We chose signed type, to be consistent with off_t (which is signed) and
> with possibility for signed return type (where negative value means
> error).
>
> Please refer to initial cover-letter
> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg08723.html
> for more info.
>
> v4: I found, that some more work is needed for block/block-backend, so
> decided to make partI, converting block/io
>
> v4 is based on Kevin's block branch ([PULL 00/34] Block layer patches)
>for BDRV_MAX_LENGTH
>
> changes:
> 01-05: new
> 06: add Alberto's r-b
> 07: new
> 08-16: rebase, add new-style request check, improve commit-msg, drop r-bs
I had planned to send a pull request for this series today, but ran into
a snag. Without this series applied, './check -qcow2' fails 030, 185,
and 297. With it applied, I now also get a failure in 206. I'm trying
to bisect which patch caused the problem, but here's the failure:
206 fail [20:54:54] [20:55:01] 6.9s (last: 6.7s) output
mismatch (see 206.out.bad)
--- /home/eblake/qemu/tests/qemu-iotests/206.out
+++ 206.out.bad
@@ -180,7 +180,7 @@
{"execute": "blockdev-create", "arguments": {"job-id": "job0",
"options": {"driver": "qcow2", "file": "node0", "size":
9223372036854775296}}}
{"return": {}}
-Job failed: Could not resize image: Required too big image size, it
must be not greater than 9223372035781033984
+Job failed: Could not resize image: offset(9223372036854775296) exceeds
maximum(9223372035781033984)
{"execute": "job-dismiss", "arguments": {"id": "job0"}}
{"return": {}}
Looks like it is just a changed error message, so I can touch up the
correct patch and then repackage the pull request tomorrow (it's too
late for me today). Oh, and the 0 exit status of ./check when a test
fails is something I see you already plan on fixing...
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v5 5/5] hw/block/nvme: add simple copy command
On Jan 29 10:15, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Add support for TP 4065a ("Simple Copy Command"), v2020.05.04
> ("Ratified").
>
> The implementation uses a bounce buffer to first read in the source
> logical blocks, then issue a write of that bounce buffer. The default
> maximum number of source logical blocks is 128, translating to 512 KiB
> for 4k logical blocks which aligns with the default value of MDTS.
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme-ns.h| 4 +
> hw/block/nvme.h | 1 +
> hw/block/nvme-ns.c| 8 ++
> hw/block/nvme.c | 253 +-
> hw/block/trace-events | 7 ++
> 5 files changed, 272 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/trace-events b/hw/block/trace-events
> index c083000b8c1f..b26866ba4338 100644
> --- a/hw/block/trace-events
> +++ b/hw/block/trace-events
> @@ -43,12 +43,18 @@ pci_nvme_admin_cmd(uint16_t cid, uint16_t sqid, uint8_t
> opcode, const char *opna
> pci_nvme_read(uint16_t cid, uint32_t nsid, uint32_t nlb, uint64_t count,
> uint64_t lba) "cid %"PRIu16" nsid %"PRIu32" nlb %"PRIu32" count %"PRIu64" lba
> 0x%"PRIx64""
> pci_nvme_write(uint16_t cid, const char *verb, uint32_t nsid, uint32_t nlb,
> uint64_t count, uint64_t lba) "cid %"PRIu16" opname '%s' nsid %"PRIu32" nlb
> %"PRIu32" count %"PRIu64" lba 0x%"PRIx64""
> pci_nvme_rw_cb(uint16_t cid, const char *blkname) "cid %"PRIu16" blk '%s'"
> +pci_nvme_copy(uint16_t cid, uint32_t nsid, uint16_t nr, uint8_t format) "cid
> %"PRIu16" nsid %"PRIu32" nr %"PRIu16" format 0x%"PRIx8""
> +pci_nvme_copy_source_range(uint64_t slba, uint32_t nlb) "slba 0x%"PRIx64"
> nlb %"PRIu32""
> +pci_nvme_copy_in_complete(uint16_t cid) "cid %"PRIu16""
> +pci_nvme_copy_cb(uint16_t cid) "cid %"PRIu16""
> +pci_nvme_write_zeroes(uint16_t cid, uint32_t nsid, uint64_t slba, uint32_t
> nlb) "cid %"PRIu16" nsid %"PRIu32" slba %"PRIu64" nlb %"PRIu32""
Woops. An old trace event ended up in there when rebasing.
signature.asc
Description: PGP signature
Re: [PULL 0/2] block: Fix iotests to respect configured Python binary
On Fri, 29 Jan 2021 at 17:22, Peter Maydell wrote: > > On Fri, 29 Jan 2021 at 16:13, Peter Maydell wrote: > > > > On Fri, 29 Jan 2021 at 14:52, Kevin Wolf wrote: > > > > > > Block layer patches: > > > > > > - Fix iotests to respect configured Python binary > > > > > > > > > > This is definitely better so I'm going to apply it, but it seems > > to reveal a pile of iotest failures on FreeBSD: > > These seem to be intermittent -- a rerun was fine. Intermittent, but not very intermittent -- I've just run about three lots of the NetBSD vm test run in a row trying to get a pass through, but something in iotests dies every time :-( thanks -- PMM
Re: [PATCH RFC 0/1] QOM type names and QAPI
On Fri, Jan 29, 2021 at 02:25:56PM +0100, Paolo Bonzini wrote: > On 29/01/21 13:17, Daniel P. Berrangé wrote: > > > On this one, my vote would be "no". "Versioned machine names > > > include the QEMU version number" is pretty well entrenched, > > > and requiring users to remember that when they want version 4.2 > > > they need to remember some other way of writing it than "4.2" > > > seems rather unfriendly. And 550 uses of '.' is a lot. > > We can't make keyval_parse() accept "/" instead of ".", but can > > we make it accept "/" in addition to ".", and then encourage "/" ? > > > > People simply wouldnt be able to use "." as keyval separator if > > they're using typenames containing "." (or would have to escape > > the typename. > > '.' is much more common than '/', and is shared by about all programming > languages that have JSON-ish data structures natively. So using '/' seems > decidedly worse to me. Worse than what, exactly? Accepting "/" when "." is ambiguous seems decidedly better than the following alternatives: - renaming machine types to names like "q35-5-0"; or - having to escape "." in the command line. -- Eduardo
Re: [PATCH RFC 1/1] hw: Replace anti-social QOM type names
On 29/01/2021 08:15, Markus Armbruster wrote: Several QOM type names contain ',': ARM,bitband-memory etraxfs,pic etraxfs,serial etraxfs,timer fsl,imx25 fsl,imx31 fsl,imx6 fsl,imx6ul fsl,imx7 grlib,ahbpnp grlib,apbpnp grlib,apbuart grlib,gptimer grlib,irqmp qemu,register SUNW,bpp SUNW,CS4231 SUNW,DBRI SUNW,DBRI.prom SUNW,fdtwo SUNW,sx SUNW,tcx My personal preference for the SUNW prefix would be to either drop it completely or change it to "sun-" instead. The reason being that the "SUNW," prefix is the standard way to reference Sun devices/packages, and looking at the replacements which still have a SUNW prefix in captials makes me thing the hyphen is either a typo or my fingers go on autopilot and type a comma anyway... ATB, Mark.
Re: [PATCH 09/10] target: Move ARM_COMPATIBLE_SEMIHOSTING feature to target Kconfig
On Sun, Jan 31, 2021 at 3:14 AM Philippe Mathieu-Daudé wrote: > > ARM_COMPATIBLE_SEMIHOSTING is an architecture feature, move its > declaration to each target/ARCH/. > > Note, we do not modify the linux-user targets, as user-mode builds > don't use Kconfig. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alistair Francis Alistair > --- > default-configs/devices/arm-softmmu.mak | 1 - > default-configs/devices/riscv32-softmmu.mak | 1 - > default-configs/devices/riscv64-softmmu.mak | 1 - > target/arm/Kconfig | 1 + > target/riscv/Kconfig| 2 ++ > 5 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/default-configs/devices/arm-softmmu.mak > b/default-configs/devices/arm-softmmu.mak > index 341d439de6f..0824e9be795 100644 > --- a/default-configs/devices/arm-softmmu.mak > +++ b/default-configs/devices/arm-softmmu.mak > @@ -41,5 +41,4 @@ CONFIG_MICROBIT=y > CONFIG_FSL_IMX25=y > CONFIG_FSL_IMX7=y > CONFIG_FSL_IMX6UL=y > -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y > CONFIG_ALLWINNER_H3=y > diff --git a/default-configs/devices/riscv32-softmmu.mak > b/default-configs/devices/riscv32-softmmu.mak > index 5c9ad2590ef..94a236c9c25 100644 > --- a/default-configs/devices/riscv32-softmmu.mak > +++ b/default-configs/devices/riscv32-softmmu.mak > @@ -3,7 +3,6 @@ > # Uncomment the following lines to disable these optional devices: > # > #CONFIG_PCI_DEVICES=n > -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y > > # Boards: > # > diff --git a/default-configs/devices/riscv64-softmmu.mak > b/default-configs/devices/riscv64-softmmu.mak > index d5b2e25b6df..76b61956489 100644 > --- a/default-configs/devices/riscv64-softmmu.mak > +++ b/default-configs/devices/riscv64-softmmu.mak > @@ -3,7 +3,6 @@ > # Uncomment the following lines to disable these optional devices: > # > #CONFIG_PCI_DEVICES=n > -CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y > > # Boards: > # > diff --git a/target/arm/Kconfig b/target/arm/Kconfig > index 1f05de47ca6..ae89d05c7e5 100644 > --- a/target/arm/Kconfig > +++ b/target/arm/Kconfig > @@ -1,5 +1,6 @@ > config ARM > bool > +select ARM_COMPATIBLE_SEMIHOSTING > > config AARCH64 > bool > diff --git a/target/riscv/Kconfig b/target/riscv/Kconfig > index b9e5932f13f..c3b9d8a1cf1 100644 > --- a/target/riscv/Kconfig > +++ b/target/riscv/Kconfig > @@ -1,5 +1,7 @@ > config RISCV32 > bool > +select ARM_COMPATIBLE_SEMIHOSTING > > config RISCV64 > bool > +select ARM_COMPATIBLE_SEMIHOSTING > -- > 2.26.2 > >
Re: [PATCH 08/10] default-configs: Remove unnecessary SEMIHOSTING selection
On Sun, Jan 31, 2021 at 3:24 AM Philippe Mathieu-Daudé wrote:
>
> Commit 56b5170c87e ("semihosting: Move ARM semihosting code to
> shared directories") selected ARM_COMPATIBLE_SEMIHOSTING which
> already selects SEMIHOSTING. No need to select it again.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> default-configs/devices/arm-softmmu.mak | 1 -
> default-configs/devices/riscv32-softmmu.mak | 1 -
> default-configs/devices/riscv64-softmmu.mak | 1 -
> 3 files changed, 3 deletions(-)
>
> diff --git a/default-configs/devices/arm-softmmu.mak
> b/default-configs/devices/arm-softmmu.mak
> index 0500156a0c7..341d439de6f 100644
> --- a/default-configs/devices/arm-softmmu.mak
> +++ b/default-configs/devices/arm-softmmu.mak
> @@ -41,6 +41,5 @@ CONFIG_MICROBIT=y
> CONFIG_FSL_IMX25=y
> CONFIG_FSL_IMX7=y
> CONFIG_FSL_IMX6UL=y
> -CONFIG_SEMIHOSTING=y
> CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
> CONFIG_ALLWINNER_H3=y
> diff --git a/default-configs/devices/riscv32-softmmu.mak
> b/default-configs/devices/riscv32-softmmu.mak
> index d847bd5692e..5c9ad2590ef 100644
> --- a/default-configs/devices/riscv32-softmmu.mak
> +++ b/default-configs/devices/riscv32-softmmu.mak
> @@ -3,7 +3,6 @@
> # Uncomment the following lines to disable these optional devices:
> #
> #CONFIG_PCI_DEVICES=n
> -CONFIG_SEMIHOSTING=y
> CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>
> # Boards:
> diff --git a/default-configs/devices/riscv64-softmmu.mak
> b/default-configs/devices/riscv64-softmmu.mak
> index d5eec75f05e..d5b2e25b6df 100644
> --- a/default-configs/devices/riscv64-softmmu.mak
> +++ b/default-configs/devices/riscv64-softmmu.mak
> @@ -3,7 +3,6 @@
> # Uncomment the following lines to disable these optional devices:
> #
> #CONFIG_PCI_DEVICES=n
> -CONFIG_SEMIHOSTING=y
> CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
>
> # Boards:
> --
> 2.26.2
>
>
[PULL 6/6] iotests: Fix -makecheck output
For -makecheck, the old 'check' implementation skipped the output when
starting a test. It only had the condensed output at the end of a test.
testrunner.py prints the normal output when starting a test even for
-makecheck. This output contains '\r' at the end so that it can be
overwritten with the result at the end of the test. However, for
-makecheck this is shorter output in a different format, so effectively
we end up with garbled output that mixes both output forms.
Revert to the old behaviour of only printing a message after the test
had completed in -makecheck mode.
Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628
Signed-off-by: Kevin Wolf
Message-Id: <20210201161024.127921-1-kw...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/testrunner.py | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 25754e9a09..1fc61fcaa3 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -301,8 +301,10 @@ class TestRunner(ContextManager['TestRunner']):
last_el = self.last_elapsed.get(test)
start = datetime.datetime.now().strftime('%H:%M:%S')
-self.test_print_one_line(test=test, starttime=start, lasttime=last_el,
- end='\r', test_field_width=test_field_width)
+if not self.makecheck:
+self.test_print_one_line(test=test, starttime=start,
+ lasttime=last_el, end='\r',
+ test_field_width=test_field_width)
res = self.do_run_test(test)
--
2.29.2
[PULL 4/6] iotests/297: pylint: ignore too many statements
From: Vladimir Sementsov-Ogievskiy Ignore two complains, which now lead to 297 failure on testenv.py and testrunner.py. Fixes: 2e5a2f57db481f18fcf70be2a36b1417370b8476 Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628 Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210129161323.615027-1-vsement...@virtuozzo.com> Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- tests/qemu-iotests/pylintrc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc index cd3702e23c..7a6c0a9474 100644 --- a/tests/qemu-iotests/pylintrc +++ b/tests/qemu-iotests/pylintrc @@ -21,6 +21,8 @@ disable=invalid-name, unsubscriptable-object, # These are temporary, and should be removed: missing-docstring, +too-many-return-statements, +too-many-statements [FORMAT] -- 2.29.2
[PULL 5/6] iotests: check: return 1 on failure
From: Vladimir Sementsov-Ogievskiy
We should indicate failure by exit code, not only output.
Reported-by: Peter Maydell
Fixes: f203080bbd9f9e5b31041b1f2afcd6040c5aaec5
Signed-off-by: Vladimir Sementsov-Ogievskiy
Message-Id: <20210201085041.3079-1-vsement...@virtuozzo.com>
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/testrunner.py | 4 +++-
tests/qemu-iotests/check | 5 -
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 24b3fba115..25754e9a09 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -318,7 +318,7 @@ class TestRunner(ContextManager['TestRunner']):
return res
-def run_tests(self, tests: List[str]) -> None:
+def run_tests(self, tests: List[str]) -> bool:
n_run = 0
failed = []
notrun = []
@@ -363,5 +363,7 @@ class TestRunner(ContextManager['TestRunner']):
if failed:
print('Failures:', ' '.join(failed))
print(f'Failed {len(failed)} of {n_run} iotests')
+return False
else:
print(f'Passed all {n_run} iotests')
+return True
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 5190dee82e..d1c87ceaf1 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -140,4 +140,7 @@ if __name__ == '__main__':
else:
with TestRunner(env, makecheck=args.makecheck,
color=args.color) as tr:
-tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])
+paths = [os.path.join(env.source_iotests, t) for t in tests]
+ok = tr.run_tests(paths)
+if not ok:
+sys.exit(1)
--
2.29.2
[PULL 1/6] MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs
From: Vladimir Sementsov-Ogievskiy I'm developing Qemu backup for several years, and finally new backup architecture, including block-copy generic engine and backup-top filter landed upstream, great thanks to reviewers and especially to Max Reitz! I also have plans of moving other block-jobs onto block-copy, so that we finally have one generic block copying path, fast and well-formed. So, now I suggest to bring all parts of backup architecture into "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and qemu-co-shared-resource can be reused somewhere else, but I'd keep an eye on them in context of block-jobs) and add myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210128144144.27617-1-vsement...@virtuozzo.com> Reviewed-by: Markus Armbruster Reviewed-by: John Snow Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- MAINTAINERS | 10 ++ 1 file changed, 10 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index bcd88668bc..00626941f1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2210,6 +2210,7 @@ F: scsi/* Block Jobs M: John Snow +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: blockjob.c @@ -,7 +2223,16 @@ F: block/commit.c F: block/stream.c F: block/mirror.c F: qapi/job.json +F: block/block-copy.c +F: include/block/block-copy.c +F: block/backup-top.h +F: block/backup-top.c +F: include/block/aio_task.h +F: block/aio_task.c +F: util/qemu-co-shared-resource.c +F: include/qemu/co-shared-resource.h T: git https://gitlab.com/jsnow/qemu.git jobs +T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs Block QAPI, monitor, command line M: Markus Armbruster -- 2.29.2
[PULL 3/6] block: move blk_exp_close_all() to qemu_cleanup()
From: Sergio Lopez
Move blk_exp_close_all() from bdrv_close() to qemu_cleanup(), before
bdrv_drain_all_begin().
Export drivers may have coroutines yielding at some point in the block
layer, so we need to shut them down before draining the block layer,
as otherwise they may get stuck blk_wait_while_drained().
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505
Signed-off-by: Sergio Lopez
Message-Id: <20210201125032.44713-3-...@redhat.com>
Signed-off-by: Kevin Wolf
---
block.c | 1 -
qemu-nbd.c | 1 +
softmmu/runstate.c | 9 +
storage-daemon/qemu-storage-daemon.c | 1 +
4 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c428e1595..4e52b1c588 100644
--- a/block.c
+++ b/block.c
@@ -4435,7 +4435,6 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
assert(job_next(NULL) == NULL);
-blk_exp_close_all();
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0d513cb38c..608c63e82a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -503,6 +503,7 @@ static const char *socket_activation_validate_opts(const
char *device,
static void qemu_nbd_shutdown(void)
{
job_cancel_sync_all();
+blk_exp_close_all();
bdrv_close_all();
}
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index beee050815..a7fcb603f7 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "audio/audio.h"
#include "block/block.h"
+#include "block/export.h"
#include "chardev/char.h"
#include "crypto/cipher.h"
#include "crypto/init.h"
@@ -784,6 +785,14 @@ void qemu_cleanup(void)
*/
migration_shutdown();
+/*
+ * Close the exports before draining the block layer. The export
+ * drivers may have coroutines yielding on it, so we need to clean
+ * them up before the drain, as otherwise they may be get stuck in
+ * blk_wait_while_drained().
+ */
+blk_exp_close_all();
+
/*
* We must cancel all block jobs while the block layer is drained,
* or cancelling will be affected by throttling and thus may block
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index e0c87edbdd..d8d172cc60 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -314,6 +314,7 @@ int main(int argc, char *argv[])
main_loop_wait(false);
}
+blk_exp_close_all();
bdrv_drain_all_begin();
bdrv_close_all();
--
2.29.2
[PULL 2/6] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
From: Sergio Lopez
Some graphs may contain an indirect reference to the first BDS in the
chain that can be reached while walking it bottom->up from one its
children.
Doubling-processing of a BDS is especially problematic for the
aio_notifiers, as they might attempt to work on both the old and the
new AIO contexts.
To avoid this problem, add every child and parent to the ignore list
before actually processing them.
Suggested-by: Kevin Wolf
Signed-off-by: Sergio Lopez
Message-Id: <20210201125032.44713-2-...@redhat.com>
Signed-off-by: Kevin Wolf
---
block.c | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 91a66d4f3e..5c428e1595 100644
--- a/block.c
+++ b/block.c
@@ -6439,7 +6439,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
AioContext *new_context, GSList **ignore)
{
AioContext *old_context = bdrv_get_aio_context(bs);
-BdrvChild *child;
+GSList *children_to_process = NULL;
+GSList *parents_to_process = NULL;
+GSList *entry;
+BdrvChild *child, *parent;
g_assert(qemu_get_current_aio_context() == qemu_get_aio_context());
@@ -6454,16 +6457,33 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
continue;
}
*ignore = g_slist_prepend(*ignore, child);
-bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+children_to_process = g_slist_prepend(children_to_process, child);
}
-QLIST_FOREACH(child, &bs->parents, next_parent) {
-if (g_slist_find(*ignore, child)) {
+
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (g_slist_find(*ignore, parent)) {
continue;
}
-assert(child->klass->set_aio_ctx);
-*ignore = g_slist_prepend(*ignore, child);
-child->klass->set_aio_ctx(child, new_context, ignore);
+*ignore = g_slist_prepend(*ignore, parent);
+parents_to_process = g_slist_prepend(parents_to_process, parent);
+}
+
+for (entry = children_to_process;
+ entry != NULL;
+ entry = g_slist_next(entry)) {
+child = entry->data;
+bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+}
+g_slist_free(children_to_process);
+
+for (entry = parents_to_process;
+ entry != NULL;
+ entry = g_slist_next(entry)) {
+parent = entry->data;
+assert(parent->klass->set_aio_ctx);
+parent->klass->set_aio_ctx(parent, new_context, ignore);
}
+g_slist_free(parents_to_process);
bdrv_detach_aio_context(bs);
--
2.29.2
[PULL 0/6] Block layer patches
The following changes since commit 74208cd252c5da9d867270a178799abd802b9338: Merge remote-tracking branch 'remotes/berrange-gitlab/tags/misc-fixes-pull-request' into staging (2021-01-29 19:51:25 +) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 47c9af7f4c78d03986aecc9afb0aab9b19d77cb5: iotests: Fix -makecheck output (2021-02-01 17:58:49 +0100) Block layer patches: - Fix double processing of nodes in bdrv_set_aio_context() - Fix potential hang in block export shutdown - iotests: Some more fixups for the 'check' rewrite - MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs Kevin Wolf (1): iotests: Fix -makecheck output Sergio Lopez (2): block: Avoid processing BDS twice in bdrv_set_aio_context_ignore() block: move blk_exp_close_all() to qemu_cleanup() Vladimir Sementsov-Ogievskiy (3): MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs iotests/297: pylint: ignore too many statements iotests: check: return 1 on failure block.c | 35 +++ qemu-nbd.c | 1 + softmmu/runstate.c | 9 + storage-daemon/qemu-storage-daemon.c | 1 + tests/qemu-iotests/testrunner.py | 10 +++--- MAINTAINERS | 10 ++ tests/qemu-iotests/check | 5 - tests/qemu-iotests/pylintrc | 2 ++ 8 files changed, 61 insertions(+), 12 deletions(-)
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On Mon, 1 Feb 2021, Daniel P. Berrangé wrote: On Mon, Feb 01, 2021 at 06:25:33PM +0100, Paolo Bonzini wrote: On 01/02/21 17:54, Kevin Wolf wrote: How does this option parsing work? Would then multiple patterns separated by comma as in -trace pattern1,pattern2 also work? This would be interpreted as an implied "enable" option with a value of "pattern1,pattern2". I don't think anything splits that string at the comma, so it would look for a trace event matching that string. Even worse, it would be interpreted as "-trace enable=pattern1,pattern2=on" (and raise a warning since recently). Maybe we're trying to solve the problem at the wrong level. There's no problem to solve, just trying to understand better what are the valid options. It's already possible to enable multiple patterns with either events=file or repeating -trace options (with or without enable=) so that's already sufficient, I was just curious what other options are there and if there's a simpler way that we could document. If not, using the current ways that are now documented is OK I think. The pattern is currently matched using the GLib glob matching APIs. If we switched to use the GLib regex matching APIs, then we don't need to repeat the args at all. We could just use regex syntax: -trace 'enable=(kvm|virtio)*' It is a little tedious to have to quote the args to avoid shell expansion, but as a tradeoff you get much stronger ability todo complex matches to filter out irrelevant cruft. I guess we have enough expressiveness with current pattern matching and globs are more easily understood by mere users so regex may not be needed here. If we want to maintain back compat for glob syntax, then we should support both in parallel and accept a different parameter name for the regex style. That would be (even more) confusing so better to just stay with globs. Regards, BALATON Zoltan
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On Mon, Feb 01, 2021 at 06:25:33PM +0100, Paolo Bonzini wrote: > On 01/02/21 17:54, Kevin Wolf wrote: > > > How does this option parsing work? Would then multiple patterns separated > > > by > > > comma as in -trace pattern1,pattern2 also work? > > This would be interpreted as an implied "enable" option with a value of > > "pattern1,pattern2". I don't think anything splits that string at the > > comma, so it would look for a trace event matching that string. > > Even worse, it would be interpreted as "-trace enable=pattern1,pattern2=on" > (and raise a warning since recently). Maybe we're trying to solve the problem at the wrong level. The pattern is currently matched using the GLib glob matching APIs. If we switched to use the GLib regex matching APIs, then we don't need to repeat the args at all. We could just use regex syntax: -trace 'enable=(kvm|virtio)*' It is a little tedious to have to quote the args to avoid shell expansion, but as a tradeoff you get much stronger ability todo complex matches to filter out irrelevant cruft. If we want to maintain back compat for glob syntax, then we should support both in parallel and accept a different parameter name for the regex style. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On Mon, 1 Feb 2021, Paolo Bonzini wrote: On 01/02/21 17:54, Kevin Wolf wrote: How does this option parsing work? Would then multiple patterns separated by comma as in -trace pattern1,pattern2 also work? This would be interpreted as an implied "enable" option with a value of "pattern1,pattern2". I don't think anything splits that string at the comma, so it would look for a trace event matching that string. Even worse, it would be interpreted as "-trace enable=pattern1,pattern2=on" (and raise a warning since recently). Not very intuitive... What would -trace enable=pattern1,enable=pattern2 do then?
Re: [PATCH v6 06/11] target/arm: Restrict ARMv7 R-profile cpus to TCG accel
Philippe Mathieu-Daudé writes: > On 1/31/21 12:50 PM, Philippe Mathieu-Daudé wrote: >> KVM requires the target cpu to be at least ARMv8 architecture >> (support on ARMv7 has been dropped in commit 82bf7ae84ce: >> "target/arm: Remove KVM support for 32-bit Arm hosts"). >> >> Beside, KVM only supports A-profile, thus won't be able to run >> R-profile cpus. >> >> Only enable the following ARMv7 R-Profile CPUs when TCG is available: >> >> - Cortex-R5 >> - Cortex-R5F >> >> The following machine is no more built when TCG is disabled: >> >> - xlnx-zcu102 Xilinx ZynqMP ZCU102 board with 4xA53s and 2xR5Fs >> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> default-configs/devices/aarch64-softmmu.mak | 1 - >> hw/arm/Kconfig | 2 ++ >> target/arm/Kconfig | 4 >> 3 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/default-configs/devices/aarch64-softmmu.mak >> b/default-configs/devices/aarch64-softmmu.mak >> index 958b1e08e40..a4202f56817 100644 >> --- a/default-configs/devices/aarch64-softmmu.mak >> +++ b/default-configs/devices/aarch64-softmmu.mak >> @@ -3,6 +3,5 @@ >> # We support all the 32 bit boards so need all their config >> include arm-softmmu.mak >> >> -CONFIG_XLNX_ZYNQMP_ARM=y >> CONFIG_XLNX_VERSAL=y >> CONFIG_SBSA_REF=y >> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig >> index 6c4bce4d637..4baf1f97694 100644 >> --- a/hw/arm/Kconfig >> +++ b/hw/arm/Kconfig >> @@ -360,8 +360,10 @@ config STM32F405_SOC >> >> config XLNX_ZYNQMP_ARM >> bool >> +default y if TCG && ARM > > The correct line is: > > "default y if TCG && AARCH64" Ahh yes, TIL we had some R-profile cores in QEMU ;-) with the fix: Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On 01/02/21 17:54, Kevin Wolf wrote: How does this option parsing work? Would then multiple patterns separated by comma as in -trace pattern1,pattern2 also work? This would be interpreted as an implied "enable" option with a value of "pattern1,pattern2". I don't think anything splits that string at the comma, so it would look for a trace event matching that string. Even worse, it would be interpreted as "-trace enable=pattern1,pattern2=on" (and raise a warning since recently). Paolo
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
01.02.2021 19:58, Kevin Wolf wrote: Am 01.02.2021 um 17:20 hat Vladimir Sementsov-Ogievskiy geschrieben: 01.02.2021 17:50, Kevin Wolf wrote: Am 01.02.2021 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: 28.01.2021 18:28, John Snow wrote: On 1/28/21 10:09 AM, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: I'm developing Qemu backup for several years, and finally new backup architecture, including block-copy generic engine and backup-top filter landed upstream, great thanks to reviewers and especially to Max Reitz! I also have plans of moving other block-jobs onto block-copy, so that we finally have one generic block copying path, fast and well-formed. So, now I suggest to bring all parts of backup architecture into "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and qemu-co-shared-resource can be reused somewhere else, but I'd keep an eye on them in context of block-jobs) and add myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy With pleasure: Reviewed-by: Markus Armbruster Absolutely! Glad to see it. Reviewed-by: John Snow [..] Great! Reviewed-by: Max Reitz Thanks! Could someone pull it? I've put it in my block branch (with s/suggest myself/Add Vladimir/ in the subject line), but I don't know when I'll send the next pull request. If someone else sends one first, feel free to include it with: Acked-by: Kevin Wolf I don't have any signed PGP key for now, to send pull requests :\ Interesting, could I get one while sitting in Moscow? If you're planning to send pull requests, should a git tree of yours be added to the MAINTAINERS sections, too? I didn't add it because of signed key absence. As it turned out, Denis Lunev (my boss) already has a signed key, so it's not a problem. I think it's appropriate to add T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs I've added it to the patch in my queue. Now you just need to actually create that branch. :-) done :) -- Best regards, Vladimir
Re: [PATCH v6 05/11] target/arm: Restrict ARMv6 cpus to TCG accel
Philippe Mathieu-Daudé writes:
> KVM requires the target cpu to be at least ARMv8 architecture
> (support on ARMv7 has been dropped in commit 82bf7ae84ce:
> "target/arm: Remove KVM support for 32-bit Arm hosts").
>
> Only enable the following ARMv6 CPUs when TCG is available:
>
> - ARM1136
> - ARM1176
> - ARM11MPCore
> - Cortex-M0
>
> The following machines are no more built when TCG is disabled:
>
> - kzm ARM KZM Emulation Baseboard (ARM1136)
> - microbit BBC micro:bit (Cortex-M0)
> - n800 Nokia N800 tablet aka. RX-34 (OMAP2420)
> - n810 Nokia N810 tablet aka. RX-44 (OMAP2420)
> - realview-eb-mpcore ARM RealView Emulation Baseboard (ARM11MPCore)
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> default-configs/devices/arm-softmmu.mak | 2 --
> hw/arm/realview.c | 2 +-
> tests/qtest/cdrom-test.c| 2 +-
> hw/arm/Kconfig | 6 ++
> target/arm/Kconfig | 4
> 5 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/default-configs/devices/arm-softmmu.mak
> b/default-configs/devices/arm-softmmu.mak
> index 0aad35da0c4..175530595ce 100644
> --- a/default-configs/devices/arm-softmmu.mak
> +++ b/default-configs/devices/arm-softmmu.mak
> @@ -10,9 +10,7 @@ CONFIG_ARM_VIRT=y
> CONFIG_CUBIEBOARD=y
> CONFIG_EXYNOS4=y
> CONFIG_HIGHBANK=y
> -CONFIG_FSL_IMX31=y
> CONFIG_MUSCA=y
> -CONFIG_NSERIES=y
> CONFIG_STELLARIS=y
> CONFIG_REALVIEW=y
> CONFIG_VEXPRESS=y
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 2dcf0a4c23e..0606d22da14 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -463,8 +463,8 @@ static void realview_machine_init(void)
> {
> if (tcg_builtin()) {
> type_register_static(&realview_eb_type);
> +type_register_static(&realview_eb_mpcore_type);
> }
> -type_register_static(&realview_eb_mpcore_type);
> type_register_static(&realview_pb_a8_type);
> type_register_static(&realview_pbx_a9_type);
> }
This confuses me - are we even able to run a realview image under KVM?
Surely the whole of realview should be TCG only?
The rest looks fine to me though:
Reviewed-by: Alex Bennée
--
Alex Bennée
Re: [PATCH v6 03/11] target/arm: Restrict ARMv4 cpus to TCG accel
Philippe Mathieu-Daudé writes: > KVM requires the target cpu to be at least ARMv8 architecture > (support on ARMv7 has been dropped in commit 82bf7ae84ce: > "target/arm: Remove KVM support for 32-bit Arm hosts"). > > Only enable the following ARMv4 CPUs when TCG is available: > > - StrongARM (SA1100/1110) > - OMAP1510 (TI925T) > > The following machines are no more built when TCG is disabled: > > - cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) > - sx1 Siemens SX1 (OMAP310) V2 > - sx1-v1 Siemens SX1 (OMAP310) V1 > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
Am 01.02.2021 um 17:20 hat Vladimir Sementsov-Ogievskiy geschrieben: > 01.02.2021 17:50, Kevin Wolf wrote: > > Am 01.02.2021 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 28.01.2021 18:28, John Snow wrote: > > > > On 1/28/21 10:09 AM, Markus Armbruster wrote: > > > > > Vladimir Sementsov-Ogievskiy writes: > > > > > > > > > > > I'm developing Qemu backup for several years, and finally new backup > > > > > > architecture, including block-copy generic engine and backup-top > > > > > > filter > > > > > > landed upstream, great thanks to reviewers and especially to > > > > > > Max Reitz! > > > > > > > > > > > > I also have plans of moving other block-jobs onto block-copy, so > > > > > > that > > > > > > we finally have one generic block copying path, fast and > > > > > > well-formed. > > > > > > > > > > > > So, now I suggest to bring all parts of backup architecture into > > > > > > "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and > > > > > > qemu-co-shared-resource can be reused somewhere else, but I'd keep > > > > > > an > > > > > > eye on them in context of block-jobs) and add myself as > > > > > > co-maintainer. > > > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > > > > > > > > > > > > With pleasure: > > > > > Reviewed-by: Markus Armbruster > > > > > > > > > > > > > Absolutely! Glad to see it. > > > > > > > > Reviewed-by: John Snow > > > > > > > > > > [..] > > > > > > > Great! > > > > > > > > Reviewed-by: Max Reitz > > > > > > Thanks! > > > > > > Could someone pull it? > > > > I've put it in my block branch (with s/suggest myself/Add Vladimir/ in > > the subject line), but I don't know when I'll send the next pull > > request. If someone else sends one first, feel free to include it with: > > > > Acked-by: Kevin Wolf > > > > > I don't have any signed PGP key for now, to send pull requests :\ > > > Interesting, could I get one while sitting in Moscow? > > > > If you're planning to send pull requests, should a git tree of yours be > > added to the MAINTAINERS sections, too? > > > > I didn't add it because of signed key absence. As it turned out, Denis > Lunev (my boss) already has a signed key, so it's not a problem. > > I think it's appropriate to add > > T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs I've added it to the patch in my queue. Now you just need to actually create that branch. :-) Kevin
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
Am 01.02.2021 um 17:29 hat BALATON Zoltan geschrieben: > On Mon, 1 Feb 2021, Kevin Wolf wrote: > > Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben: > > > On Mon, 1 Feb 2021, Stefan Hajnoczi wrote: > > > > It is possible to repeat the --trace option to specify multiple > > > > patterns. This may be preferrable to users who do not want to create a > > > > file with a list of patterns. > > > > > > > > Suggested-by: BALATON Zoltan > > > > Signed-off-by: Stefan Hajnoczi > > > > Reviewed-by: Philippe Mathieu-Daudé > > > > Message-id: 20210112165859.225534-2-stefa...@redhat.com > > > > Signed-off-by: Stefan Hajnoczi > > > > --- > > > > docs/devel/tracing.rst | 9 +++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst > > > > index af395e957d..e8f9b82c5e 100644 > > > > --- a/docs/devel/tracing.rst > > > > +++ b/docs/devel/tracing.rst > > > > @@ -22,10 +22,15 @@ events:: > > > > This output comes from the "log" trace backend that is enabled by > > > > default when > > > > ``./configure --enable-trace-backends=BACKENDS`` was not explicitly > > > > specified. > > > > > > > > -More than one trace event pattern can be specified by providing a file > > > > -instead:: > > > > +Multiple patterns can be specified by repeating the ``--trace`` > > > > option:: > > > > + > > > > +$ qemu --trace "kvm_*" --trace "virtio_*" ... > > > > > > Does that actually work? I've always used -trace enable="pattern1" -trace > > > enable="pattern2" Not sure if omitting enable= is the same. > > > > qemu_trace_opts has .implied_opt_name = "enable", so without having > > tested it, I assume this works. > > How does this option parsing work? Would then multiple patterns separated by > comma as in -trace pattern1,pattern2 also work? This would be interpreted as an implied "enable" option with a value of "pattern1,pattern2". I don't think anything splits that string at the comma, so it would look for a trace event matching that string. Kevin
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On Mon, 1 Feb 2021, Kevin Wolf wrote: Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben: On Mon, 1 Feb 2021, Stefan Hajnoczi wrote: It is possible to repeat the --trace option to specify multiple patterns. This may be preferrable to users who do not want to create a file with a list of patterns. Suggested-by: BALATON Zoltan Signed-off-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Message-id: 20210112165859.225534-2-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/tracing.rst | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst index af395e957d..e8f9b82c5e 100644 --- a/docs/devel/tracing.rst +++ b/docs/devel/tracing.rst @@ -22,10 +22,15 @@ events:: This output comes from the "log" trace backend that is enabled by default when ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified. -More than one trace event pattern can be specified by providing a file -instead:: +Multiple patterns can be specified by repeating the ``--trace`` option:: + +$ qemu --trace "kvm_*" --trace "virtio_*" ... Does that actually work? I've always used -trace enable="pattern1" -trace enable="pattern2" Not sure if omitting enable= is the same. qemu_trace_opts has .implied_opt_name = "enable", so without having tested it, I assume this works. How does this option parsing work? Would then multiple patterns separated by comma as in -trace pattern1,pattern2 also work? (Although quoting * in that may be a challenge don't know if it should be "a*,b*" "a*","b*" or a\*,b\* or any of these would be OK.) I've just found one way by repeating -trace options that worked and then used that without ever trying to explore other ways but if we're about to document it maybe somebody who actually knows how it works could tell what is the best way. Regards, BALATON Zoltan
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On 2/1/21 5:13 PM, Kevin Wolf wrote: > Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben: >> On Mon, 1 Feb 2021, Stefan Hajnoczi wrote: >>> It is possible to repeat the --trace option to specify multiple >>> patterns. This may be preferrable to users who do not want to create a >>> file with a list of patterns. >>> >>> Suggested-by: BALATON Zoltan >>> Signed-off-by: Stefan Hajnoczi >>> Reviewed-by: Philippe Mathieu-Daudé >>> Message-id: 20210112165859.225534-2-stefa...@redhat.com >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> docs/devel/tracing.rst | 9 +++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst >>> index af395e957d..e8f9b82c5e 100644 >>> --- a/docs/devel/tracing.rst >>> +++ b/docs/devel/tracing.rst >>> @@ -22,10 +22,15 @@ events:: >>> This output comes from the "log" trace backend that is enabled by default >>> when >>> ``./configure --enable-trace-backends=BACKENDS`` was not explicitly >>> specified. >>> >>> -More than one trace event pattern can be specified by providing a file >>> -instead:: >>> +Multiple patterns can be specified by repeating the ``--trace`` option:: >>> + >>> +$ qemu --trace "kvm_*" --trace "virtio_*" ... >> >> Does that actually work? I've always used -trace enable="pattern1" -trace >> enable="pattern2" Not sure if omitting enable= is the same. > > qemu_trace_opts has .implied_opt_name = "enable", so without having > tested it, I assume this works. I use -trace \*pci\* -trace memory\*, and Kevin said -trace and --trace are the same.
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
01.02.2021 17:50, Kevin Wolf wrote: Am 01.02.2021 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: 28.01.2021 18:28, John Snow wrote: On 1/28/21 10:09 AM, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: I'm developing Qemu backup for several years, and finally new backup architecture, including block-copy generic engine and backup-top filter landed upstream, great thanks to reviewers and especially to Max Reitz! I also have plans of moving other block-jobs onto block-copy, so that we finally have one generic block copying path, fast and well-formed. So, now I suggest to bring all parts of backup architecture into "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and qemu-co-shared-resource can be reused somewhere else, but I'd keep an eye on them in context of block-jobs) and add myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy With pleasure: Reviewed-by: Markus Armbruster Absolutely! Glad to see it. Reviewed-by: John Snow [..] Great! Reviewed-by: Max Reitz Thanks! Could someone pull it? I've put it in my block branch (with s/suggest myself/Add Vladimir/ in the subject line), but I don't know when I'll send the next pull request. If someone else sends one first, feel free to include it with: Acked-by: Kevin Wolf I don't have any signed PGP key for now, to send pull requests :\ Interesting, could I get one while sitting in Moscow? If you're planning to send pull requests, should a git tree of yours be added to the MAINTAINERS sections, too? I didn't add it because of signed key absence. As it turned out, Denis Lunev (my boss) already has a signed key, so it's not a problem. I think it's appropriate to add T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs -- Best regards, Vladimir
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
Am 01.02.2021 um 17:05 hat BALATON Zoltan geschrieben: > On Mon, 1 Feb 2021, Stefan Hajnoczi wrote: > > It is possible to repeat the --trace option to specify multiple > > patterns. This may be preferrable to users who do not want to create a > > file with a list of patterns. > > > > Suggested-by: BALATON Zoltan > > Signed-off-by: Stefan Hajnoczi > > Reviewed-by: Philippe Mathieu-Daudé > > Message-id: 20210112165859.225534-2-stefa...@redhat.com > > Signed-off-by: Stefan Hajnoczi > > --- > > docs/devel/tracing.rst | 9 +++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst > > index af395e957d..e8f9b82c5e 100644 > > --- a/docs/devel/tracing.rst > > +++ b/docs/devel/tracing.rst > > @@ -22,10 +22,15 @@ events:: > > This output comes from the "log" trace backend that is enabled by default > > when > > ``./configure --enable-trace-backends=BACKENDS`` was not explicitly > > specified. > > > > -More than one trace event pattern can be specified by providing a file > > -instead:: > > +Multiple patterns can be specified by repeating the ``--trace`` option:: > > + > > +$ qemu --trace "kvm_*" --trace "virtio_*" ... > > Does that actually work? I've always used -trace enable="pattern1" -trace > enable="pattern2" Not sure if omitting enable= is the same. qemu_trace_opts has .implied_opt_name = "enable", so without having tested it, I assume this works. Kevin
Re: [PATCH] iotests: Fix -makecheck output
01.02.2021 19:10, Kevin Wolf wrote:
For -makecheck, the old 'check' implementation skipped the output when
starting a test. It only had the condensed output at the end of a test.
testrunner.py prints the normal output when starting a test even for
-makecheck. This output contains '\r' at the end so that it can be
overwritten with the result at the end of the test. However, for
-makecheck this is shorter output in a different format, so effectively
we end up with garbled output that mixes both output forms.
Revert to the old behaviour of only printing a message after the test
had completed in -makecheck mode.
Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/testrunner.py | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 25754e9a09..1fc61fcaa3 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -301,8 +301,10 @@ class TestRunner(ContextManager['TestRunner']):
last_el = self.last_elapsed.get(test)
start = datetime.datetime.now().strftime('%H:%M:%S')
-self.test_print_one_line(test=test, starttime=start, lasttime=last_el,
- end='\r', test_field_width=test_field_width)
+if not self.makecheck:
+self.test_print_one_line(test=test, starttime=start,
+ lasttime=last_el, end='\r',
+ test_field_width=test_field_width)
res = self.do_run_test(test)
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
[PATCH] iotests: Fix -makecheck output
For -makecheck, the old 'check' implementation skipped the output when
starting a test. It only had the condensed output at the end of a test.
testrunner.py prints the normal output when starting a test even for
-makecheck. This output contains '\r' at the end so that it can be
overwritten with the result at the end of the test. However, for
-makecheck this is shorter output in a different format, so effectively
we end up with garbled output that mixes both output forms.
Revert to the old behaviour of only printing a message after the test
had completed in -makecheck mode.
Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628
Signed-off-by: Kevin Wolf
---
tests/qemu-iotests/testrunner.py | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 25754e9a09..1fc61fcaa3 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -301,8 +301,10 @@ class TestRunner(ContextManager['TestRunner']):
last_el = self.last_elapsed.get(test)
start = datetime.datetime.now().strftime('%H:%M:%S')
-self.test_print_one_line(test=test, starttime=start, lasttime=last_el,
- end='\r', test_field_width=test_field_width)
+if not self.makecheck:
+self.test_print_one_line(test=test, starttime=start,
+ lasttime=last_el, end='\r',
+ test_field_width=test_field_width)
res = self.do_run_test(test)
--
2.29.2
Re: [PULL 10/11] trace: document how to specify multiple --trace patterns
On Mon, 1 Feb 2021, Stefan Hajnoczi wrote: It is possible to repeat the --trace option to specify multiple patterns. This may be preferrable to users who do not want to create a file with a list of patterns. Suggested-by: BALATON Zoltan Signed-off-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Message-id: 20210112165859.225534-2-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/tracing.rst | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst index af395e957d..e8f9b82c5e 100644 --- a/docs/devel/tracing.rst +++ b/docs/devel/tracing.rst @@ -22,10 +22,15 @@ events:: This output comes from the "log" trace backend that is enabled by default when ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified. -More than one trace event pattern can be specified by providing a file -instead:: +Multiple patterns can be specified by repeating the ``--trace`` option:: + +$ qemu --trace "kvm_*" --trace "virtio_*" ... Does that actually work? I've always used -trace enable="pattern1" -trace enable="pattern2" Not sure if omitting enable= is the same. Regards, BALATON Zoltan + +When patterns are used frequently it is more convenient to store them in a +file to avoid long command-line options:: $ echo "memory_region_ops_*" >/tmp/events +$ echo "kvm_*" >>/tmp/events $ qemu --trace events=/tmp/events ... Trace events -- 2.29.2
Re: [PATCH] iotests: check: return 1 on failure
Am 01.02.2021 um 09:50 hat Vladimir Sementsov-Ogievskiy geschrieben: > We should indicate failure by exit code, not only output. > > Reported-by: Peter Maydell > Fixes: f203080bbd9f9e5b31041b1f2afcd6040c5aaec5 > Signed-off-by: Vladimir Sementsov-Ogievskiy Thanks, applied to the block branch. Kevin
[PULL 10/11] trace: document how to specify multiple --trace patterns
It is possible to repeat the --trace option to specify multiple patterns. This may be preferrable to users who do not want to create a file with a list of patterns. Suggested-by: BALATON Zoltan Signed-off-by: Stefan Hajnoczi Reviewed-by: Philippe Mathieu-Daudé Message-id: 20210112165859.225534-2-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/tracing.rst | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst index af395e957d..e8f9b82c5e 100644 --- a/docs/devel/tracing.rst +++ b/docs/devel/tracing.rst @@ -22,10 +22,15 @@ events:: This output comes from the "log" trace backend that is enabled by default when ``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified. -More than one trace event pattern can be specified by providing a file -instead:: +Multiple patterns can be specified by repeating the ``--trace`` option:: + +$ qemu --trace "kvm_*" --trace "virtio_*" ... + +When patterns are used frequently it is more convenient to store them in a +file to avoid long command-line options:: $ echo "memory_region_ops_*" >/tmp/events +$ echo "kvm_*" >>/tmp/events $ qemu --trace events=/tmp/events ... Trace events -- 2.29.2
[PULL 09/11] simpletrace: build() missing 2 required positional arguments
From: Volker Rümelin
Commit 4e66c9ef64 "tracetool: add input filename and line number to
Event" forgot to add a line number and a filename argument at one
build method call site.
Traceback (most recent call last):
File "./scripts/simpletrace.py", line 261, in
run(Formatter())
File "./scripts/simpletrace.py", line 236, in run
process(events, sys.argv[2], analyzer, read_header=read_header)
File "./scripts/simpletrace.py", line 177, in process
dropped_event =
Event.build("Dropped_Event(uint64_t num_events_dropped)")
TypeError: build() missing 2 required positional arguments:
'lineno' and 'filename'
Add the missing arguments.
Signed-off-by: Volker Rümelin
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20210131173415.3392-1-vr_q...@t-online.de
Signed-off-by: Stefan Hajnoczi
---
scripts/simpletrace.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/scripts/simpletrace.py b/scripts/simpletrace.py
index 20f0026066..d61fb0bd87 100755
--- a/scripts/simpletrace.py
+++ b/scripts/simpletrace.py
@@ -174,7 +174,9 @@ def process(events, log, analyzer, read_header=True):
if read_header:
read_trace_header(log)
-dropped_event = Event.build("Dropped_Event(uint64_t num_events_dropped)")
+frameinfo = inspect.getframeinfo(inspect.currentframe())
+dropped_event = Event.build("Dropped_Event(uint64_t num_events_dropped)",
+frameinfo.lineno + 1, frameinfo.filename)
edict = {"dropped": dropped_event}
idtoname = {dropped_event_id: "dropped"}
--
2.29.2
[PULL 05/11] tracetool: also strip %l and %ll from systemtap format strings
From: Daniel P. Berrangé
All variables are 64-bit and so %l / %ll are not required, and the
latter is actually invalid:
$ sudo stap -e 'probe begin{printf ("BEGIN")}' -I .
parse error: invalid or missing conversion specifier
saw: operator ',' at ./qemu-system-x86_64-log.stp:15118:101
source: printf("%d@%d vhost_vdpa_set_log_base dev: %p base: 0x%x
size: %llu
refcnt: %d fd: %d log: %p\n", pid(), gettimeofday_ns(), dev, base, size,
refcnt, fd, log)
^
Signed-off-by: Daniel P. Berrangé
Reviewed-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Laurent Vivier
Message-id: 20210106130239.1004729-1-berra...@redhat.com
[Fixed "simiarly" typo found by Laurent Vivier
--Stefan]
Signed-off-by: Stefan Hajnoczi
---
scripts/tracetool/format/log_stap.py | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/scripts/tracetool/format/log_stap.py
b/scripts/tracetool/format/log_stap.py
index 3e1186ae9c..0b6549d534 100644
--- a/scripts/tracetool/format/log_stap.py
+++ b/scripts/tracetool/format/log_stap.py
@@ -78,7 +78,12 @@ def c_fmt_to_stap(fmt):
elif state == STATE_LITERAL:
bits.append(literal)
-fmt = re.sub("%(\d*)z(x|u|d)", "%\\1\\2", "".join(bits))
+# All variables in systemtap are 64-bit in size
+# The "%l" integer size qualifier is thus redundant
+# and "%ll" is not valid at all. Similarly the size_t
+# based "%z" size qualifier is not valid. We just
+# strip all size qualifiers for sanity.
+fmt = re.sub("%(\d*)(l+|z)(x|u|d)", "%\\1\\3", "".join(bits))
return fmt
def generate(events, backend, group):
--
2.29.2
[PULL 11/11] trace: update docs with meson build information
The documentation still refers to the makefile and the old sub-directory layout. Meson works differently: tracetool output is placed into the builddir with mangled filenames like /trace/trace-accel_kvm.h for the accel/kvm/ trace.h definition. This meson setup also requires a manually-created accel/kvm/trace.h file that #includes the /trace/trace-accel_kvm.h file. Document this! Reported-by: Peter Maydell Signed-off-by: Stefan Hajnoczi Message-id: 20210112165859.225534-3-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/tracing.rst | 57 +- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst index e8f9b82c5e..ba83954899 100644 --- a/docs/devel/tracing.rst +++ b/docs/devel/tracing.rst @@ -39,40 +39,51 @@ Trace events Sub-directory setup --- -Each directory in the source tree can declare a set of static trace events -in a local "trace-events" file. All directories which contain "trace-events" -files must be listed in the "trace-events-subdirs" make variable in the top -level Makefile.objs. During build, the "trace-events" file in each listed -subdirectory will be processed by the "tracetool" script to generate code for -the trace events. +Each directory in the source tree can declare a set of trace events in a local +"trace-events" file. All directories which contain "trace-events" files must be +listed in the "trace_events_subdirs" variable in the top level meson.build +file. During build, the "trace-events" file in each listed subdirectory will be +processed by the "tracetool" script to generate code for the trace events. The individual "trace-events" files are merged into a "trace-events-all" file, which is also installed into "/usr/share/qemu" with the name "trace-events". This merged file is to be used by the "simpletrace.py" script to later analyse traces in the simpletrace data format. -In the sub-directory the following files will be automatically generated +The following files are automatically generated in /trace/ during the +build: - - trace.c - the trace event state declarations - - trace.h - the trace event enums and probe functions - - trace-dtrace.h - DTrace event probe specification - - trace-dtrace.dtrace - DTrace event probe helper declaration - - trace-dtrace.o - binary DTrace provider (generated by dtrace) - - trace-ust.h - UST event probe helper declarations + - trace-.c - the trace event state declarations + - trace-.h - the trace event enums and probe functions + - trace-dtrace-.h - DTrace event probe specification + - trace-dtrace-.dtrace - DTrace event probe helper declaration + - trace-dtrace-.o - binary DTrace provider (generated by dtrace) + - trace-ust-.h - UST event probe helper declarations -Source files in the sub-directory should #include the local 'trace.h' file, -without any sub-directory path prefix. eg io/channel-buffer.c would do:: +Here is the sub-directory path with '/' replaced by '_'. For example, +"accel/kvm" becomes "accel_kvm" and the final filename for "trace-.c" +becomes "trace-accel_kvm.c". + +Source files in the source tree do not directly include generated files in +"/trace/". Instead they #include the local "trace.h" file, without +any sub-directory path prefix. eg io/channel-buffer.c would do:: #include "trace.h" -To access the 'io/trace.h' file. While it is possible to include a trace.h -file from outside a source file's own sub-directory, this is discouraged in -general. It is strongly preferred that all events be declared directly in -the sub-directory that uses them. The only exception is where there are some -shared trace events defined in the top level directory trace-events file. -The top level directory generates trace files with a filename prefix of -"trace/trace-root" instead of just "trace". This is to avoid ambiguity between -a trace.h in the current directory, vs the top level directory. +The "io/trace.h" file must be created manually with an #include of the +corresponding "trace/trace-.h" file that will be generated in the +builddir:: + + $ echo '#include "trace/trace-io.h"' >io/trace.h + +While it is possible to include a trace.h file from outside a source file's own +sub-directory, this is discouraged in general. It is strongly preferred that +all events be declared directly in the sub-directory that uses them. The only +exception is where there are some shared trace events defined in the top level +directory trace-events file. The top level directory generates trace files +with a filename prefix of "trace/trace-root" instead of just "trace". This is +to avoid ambiguity between a trace.h in the current directory, vs the top level +directory. Using trace events -- -- 2.29.2
[PULL 02/11] tracing: convert documentation to rST
This is a simple rST conversion of the documentation.
Reviewed-by: Peter Maydell
Signed-off-by: Stefan Hajnoczi
Message-id: 20201216160923.722894-3-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
docs/devel/index.rst| 1 +
docs/devel/{tracing.txt => tracing.rst} | 134 ++--
2 files changed, 81 insertions(+), 54 deletions(-)
rename docs/devel/{tracing.txt => tracing.rst} (89%)
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index ea0e1e17ae..98a7016a9b 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -28,6 +28,7 @@ Contents:
secure-coding-practices
tcg
tcg-icount
+ tracing
multi-thread-tcg
tcg-plugins
bitops
diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.rst
similarity index 89%
rename from docs/devel/tracing.txt
rename to docs/devel/tracing.rst
index 313b8ea4e9..f7e589f67c 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.rst
@@ -1,32 +1,38 @@
-= Tracing =
+===
+Tracing
+===
-== Introduction ==
+Introduction
+
This document describes the tracing infrastructure in QEMU and how to use it
for debugging, profiling, and observing execution.
-== Quickstart ==
+Quickstart
+==
-1. Build with the 'simple' trace backend:
+1. Build with the 'simple' trace backend::
./configure --enable-trace-backends=simple
make
-2. Create a file with the events you want to trace:
+2. Create a file with the events you want to trace::
- echo memory_region_ops_read >/tmp/events
+echo memory_region_ops_read >/tmp/events
-3. Run the virtual machine to produce a trace file:
+3. Run the virtual machine to produce a trace file::
qemu --trace events=/tmp/events ... # your normal QEMU invocation
-4. Pretty-print the binary trace file:
+4. Pretty-print the binary trace file::
./scripts/simpletrace.py trace-events-all trace-* # Override * with QEMU
-== Trace events ==
+Trace events
+
-=== Sub-directory setup ===
+Sub-directory setup
+---
Each directory in the source tree can declare a set of static trace events
in a local "trace-events" file. All directories which contain "trace-events"
@@ -50,7 +56,7 @@ In the sub-directory the following files will be
automatically generated
- trace-ust.h - UST event probe helper declarations
Source files in the sub-directory should #include the local 'trace.h' file,
-without any sub-directory path prefix. eg io/channel-buffer.c would do
+without any sub-directory path prefix. eg io/channel-buffer.c would do::
#include "trace.h"
@@ -63,9 +69,10 @@ The top level directory generates trace files with a
filename prefix of
"trace/trace-root" instead of just "trace". This is to avoid ambiguity between
a trace.h in the current directory, vs the top level directory.
-=== Using trace events ===
+Using trace events
+--
-Trace events are invoked directly from source code like this:
+Trace events are invoked directly from source code like this::
#include "trace.h" /* needed for trace event prototype */
@@ -82,7 +89,8 @@ Trace events are invoked directly from source code like this:
return ptr;
}
-=== Declaring trace events ===
+Declaring trace events
+--
The "tracetool" script produces the trace.h header file which is included by
every source file that uses trace events. Since many source files include
@@ -116,13 +124,14 @@ Format strings must not end with a newline character. It
is the responsibility
of backends to adapt line ending for proper logging.
Each event declaration will start with the event name, then its arguments,
-finally a format string for pretty-printing. For example:
+finally a format string for pretty-printing. For example::
qemu_vmalloc(size_t size, void *ptr) "size %zu ptr %p"
qemu_vfree(void *ptr) "ptr %p"
-=== Hints for adding new trace events ===
+Hints for adding new trace events
+-
1. Trace state changes in the code. Interesting points in the code usually
involve a state change like starting, stopping, allocating, freeing. State
@@ -141,7 +150,8 @@ finally a format string for pretty-printing. For example:
4. Name trace events after their function. If there are multiple trace events
in one function, append a unique distinguisher at the end of the name.
-== Generic interface and monitor commands ==
+Generic interface and monitor commands
+==
You can programmatically query and control the state of trace events through a
backend-agnostic interface provided by the header "trace/control.h".
@@ -152,11 +162,11 @@ header "trace/control.h" to see which routines are
backend-dependent).
The state of events can also be queried and modified through monitor commands:
-* info trace-events
+* ``info trace-events``
View available trace events and their state. State 1
Re: [PATCH v2] iotests/297: pylint: ignore too many statements
Am 29.01.2021 um 17:13 hat Vladimir Sementsov-Ogievskiy geschrieben: > Ignore two complains, which now lead to 297 failure on testenv.py and > testrunner.py. > > Fixes: 2e5a2f57db481f18fcf70be2a36b1417370b8476 > Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628 > Signed-off-by: Vladimir Sementsov-Ogievskiy Thanks, applied to the block branch. Kevin
[PULL 06/11] trace: add meson custom_target() depend_files for tracetool
Re-generate tracetool output when the tracetool source code changes. Use
the same approach as qapi_gen_depends and introduce a tracetool_depends
files list so meson is aware of the dependencies.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Paolo Bonzini
Message-id: 20210125110958.214017-1-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
meson.build | 28 +++-
trace/meson.build | 21 ++---
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/meson.build b/meson.build
index f00b7754fd..2d8b433ff0 100644
--- a/meson.build
+++ b/meson.build
@@ -1632,6 +1632,31 @@ tracetool = [
python, files('scripts/tracetool.py'),
'--backend=' + config_host['TRACE_BACKENDS']
]
+tracetool_depends = files(
+ 'scripts/tracetool/backend/log.py',
+ 'scripts/tracetool/backend/__init__.py',
+ 'scripts/tracetool/backend/dtrace.py',
+ 'scripts/tracetool/backend/ftrace.py',
+ 'scripts/tracetool/backend/simple.py',
+ 'scripts/tracetool/backend/syslog.py',
+ 'scripts/tracetool/backend/ust.py',
+ 'scripts/tracetool/format/tcg_h.py',
+ 'scripts/tracetool/format/ust_events_c.py',
+ 'scripts/tracetool/format/ust_events_h.py',
+ 'scripts/tracetool/format/__init__.py',
+ 'scripts/tracetool/format/d.py',
+ 'scripts/tracetool/format/tcg_helper_c.py',
+ 'scripts/tracetool/format/simpletrace_stap.py',
+ 'scripts/tracetool/format/c.py',
+ 'scripts/tracetool/format/h.py',
+ 'scripts/tracetool/format/tcg_helper_h.py',
+ 'scripts/tracetool/format/log_stap.py',
+ 'scripts/tracetool/format/stap.py',
+ 'scripts/tracetool/format/tcg_helper_wrapper_h.py',
+ 'scripts/tracetool/__init__.py',
+ 'scripts/tracetool/transform.py',
+ 'scripts/tracetool/vcpu.py'
+)
qemu_version_cmd = [find_program('scripts/qemu-version.sh'),
meson.current_source_dir(),
@@ -2219,7 +2244,8 @@ foreach target : target_dirs
'--target-type=' + target_type,
'--probe-prefix=qemu.' + target_type + '.' +
target_name,
'@INPUT@', '@OUTPUT@'
- ])
+ ],
+ depend_files: tracetool_depends)
endforeach
endif
endforeach
diff --git a/trace/meson.build b/trace/meson.build
index a0be8f9b0d..08f83a15c3 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -12,17 +12,20 @@ foreach dir : [ '.' ] + trace_events_subdirs
trace_h = custom_target(fmt.format('trace', 'h'),
output: fmt.format('trace', 'h'),
input: trace_events_file,
- command: [ tracetool, group, '--format=h',
'@INPUT@', '@OUTPUT@' ])
+ command: [ tracetool, group, '--format=h',
'@INPUT@', '@OUTPUT@' ],
+ depend_files: tracetool_depends)
genh += trace_h
trace_c = custom_target(fmt.format('trace', 'c'),
output: fmt.format('trace', 'c'),
input: trace_events_file,
- command: [ tracetool, group, '--format=c',
'@INPUT@', '@OUTPUT@' ])
+ command: [ tracetool, group, '--format=c',
'@INPUT@', '@OUTPUT@' ],
+ depend_files: tracetool_depends)
if 'CONFIG_TRACE_UST' in config_host
trace_ust_h = custom_target(fmt.format('trace-ust', 'h'),
output: fmt.format('trace-ust', 'h'),
input: trace_events_file,
-command: [ tracetool, group,
'--format=ust-events-h', '@INPUT@', '@OUTPUT@' ])
+command: [ tracetool, group,
'--format=ust-events-h', '@INPUT@', '@OUTPUT@' ],
+depend_files: tracetool_depends)
trace_ss.add(trace_ust_h, lttng, urcubp)
genh += trace_ust_h
endif
@@ -31,7 +34,8 @@ foreach dir : [ '.' ] + trace_events_subdirs
trace_dtrace = custom_target(fmt.format('trace-dtrace', 'dtrace'),
output: fmt.format('trace-dtrace', 'dtrace'),
input: trace_events_file,
- command: [ tracetool, group, '--format=d',
'@INPUT@', '@OUTPUT@' ])
+ command: [ tracetool, group, '--format=d',
'@INPUT@', '@OUTPUT@' ],
+ depend_files: tracetool_depends)
trace_dtrace_h = custom_target(fmt.format('trace-dtrace', 'h'),
output: fmt.format('trace-dtrace', 'h'),
input: trace_dtrace,
@@ -66,7 +70,8 @@ foreach d : [
gen = custom_target(d[0],
output: d[0],
input: meson.source_root() / 'trace-events',
-command: [ tracetool, '--group=root',
'--format=@0@'.format(d[1]), '@INPUT@', '@OUTPUT@' ])
+command: [ tracetool, '--group=root',
'--format=@0@'.format(d[1]), '
[PULL 04/11] tracetool: fix "PRI" macro decoding
From: Laurent Vivier
macro is not reset after use, so the format decoded is always the
one of the first "PRI" in the format string.
For instance:
vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, \
uint32_t flags) "dev: %p offset: %"PRIu32" \
size: %"PRIu32" flags: 0x%"PRIx32
generates:
printf("%d@%d vhost_vdpa_set_config dev: %p offset: %u size: %u \
flags: 0x%u\n", pid(), gettimeofday_ns(), dev, offset, \
size, flags)
for the "flags" parameter, we can see a "0x%u" rather than a "0x%x"
because the first macro was "PRIu32" (for offset).
In the loop, macro becomes "PRIu32PRIu32PRIx32", and c_macro_to_format()
returns always macro[3] ('u' in this case). This patch resets macro after
the format has been decoded.
Signed-off-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Daniel P. Berrangé
Message-id: 20210105191721.120463-3-lviv...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
scripts/tracetool/format/log_stap.py | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/tracetool/format/log_stap.py
b/scripts/tracetool/format/log_stap.py
index b486beb672..3e1186ae9c 100644
--- a/scripts/tracetool/format/log_stap.py
+++ b/scripts/tracetool/format/log_stap.py
@@ -54,6 +54,7 @@ def c_fmt_to_stap(fmt):
else:
if state == STATE_MACRO:
bits.append(c_macro_to_format(macro))
+macro = ""
state = STATE_LITERAL
elif fmt[i] == ' ' or fmt[i] == '\t':
if state == STATE_MACRO:
--
2.29.2
[PULL 03/11] trace: recommend "log" backend for getting started with tracing
The "simple" backend is actually more complicated to use than the "log" backend. Update the quickstart documentation to feature the "log" backend instead of the "simple" backend. Suggested-by: Peter Maydell Signed-off-by: Stefan Hajnoczi Reviewed-by: Peter Maydell Message-id: 20201216160923.722894-4-stefa...@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/tracing.rst | 35 ++- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst index f7e589f67c..4ebf8e38ea 100644 --- a/docs/devel/tracing.rst +++ b/docs/devel/tracing.rst @@ -11,22 +11,22 @@ for debugging, profiling, and observing execution. Quickstart == -1. Build with the 'simple' trace backend:: +Enable tracing of ``memory_region_ops_read`` and ``memory_region_ops_write`` +events:: -./configure --enable-trace-backends=simple -make +$ qemu --trace "memory_region_ops_*" ... +... +719585@1608130130.441188:memory_region_ops_read cpu 0 mr 0x562fdfbb3820 addr 0x3cc value 0x67 size 1 +719585@1608130130.441190:memory_region_ops_write cpu 0 mr 0x562fdfbd2f00 addr 0x3d4 value 0x70e size 2 -2. Create a file with the events you want to trace:: +This output comes from the "log" trace backend that is enabled by default when +``./configure --enable-trace-backends=BACKENDS`` was not explicitly specified. -echo memory_region_ops_read >/tmp/events +More than one trace event pattern can be specified by providing a file +instead:: -3. Run the virtual machine to produce a trace file:: - -qemu --trace events=/tmp/events ... # your normal QEMU invocation - -4. Pretty-print the binary trace file:: - -./scripts/simpletrace.py trace-events-all trace-* # Override * with QEMU +$ echo "memory_region_ops_*" >/tmp/events +$ qemu --trace events=/tmp/events ... Trace events @@ -195,7 +195,7 @@ script. The trace backends are chosen at configure time:: -./configure --enable-trace-backends=simple +./configure --enable-trace-backends=simple,dtrace For a list of supported trace backends, try ./configure --help or see below. If multiple backends are enabled, the trace is sent to them all. @@ -227,10 +227,11 @@ uses DPRINTF(). Simpletrace --- -The "simple" backend supports common use cases and comes as part of the QEMU -source tree. It may not be as powerful as platform-specific or third-party -trace backends but it is portable. This is the recommended trace backend -unless you have specific needs for more advanced backends. +The "simple" backend writes binary trace logs to a file from a thread, making +it lower overhead than the "log" backend. A Python API is available for writing +offline trace file analysis scripts. It may not be as powerful as +platform-specific or third-party trace backends but it is portable and has no +special library dependencies. Monitor commands -- 2.29.2
[PULL 08/11] trace: make the 'log' backend timestamp configurable
Timestamps in tracing output can be distracting. Make it possible to
control tid/timestamp printing with -msg timestamp=on|off. The default
is no tid/timestamps. Previously they were always printed.
Suggested-by: BALATON Zoltan
Signed-off-by: Stefan Hajnoczi
Tested-by: Philippe Mathieu-Daudé
Tested-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20210125113507.224287-3-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
docs/devel/tracing.rst | 3 +++
scripts/tracetool/backend/log.py | 19 +--
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/docs/devel/tracing.rst b/docs/devel/tracing.rst
index 4ebf8e38ea..af395e957d 100644
--- a/docs/devel/tracing.rst
+++ b/docs/devel/tracing.rst
@@ -224,6 +224,9 @@ effectively turns trace events into debug printfs.
This is the simplest backend and can be used together with existing code that
uses DPRINTF().
+The -msg timestamp=on|off command-line option controls whether or not to print
+the tid/timestamp prefix for each trace event.
+
Simpletrace
---
diff --git a/scripts/tracetool/backend/log.py b/scripts/tracetool/backend/log.py
index bc43dbb4f4..17ba1cd90e 100644
--- a/scripts/tracetool/backend/log.py
+++ b/scripts/tracetool/backend/log.py
@@ -20,6 +20,7 @@ PUBLIC = True
def generate_h_begin(events, group):
out('#include "qemu/log-for-trace.h"',
+'#include "qemu/error-report.h"',
'')
@@ -35,14 +36,20 @@ def generate_h(event, group):
cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper())
out('if (%(cond)s && qemu_loglevel_mask(LOG_TRACE)) {',
-'struct timeval _now;',
-'gettimeofday(&_now, NULL);',
+'if (message_with_timestamp) {',
+'struct timeval _now;',
+'gettimeofday(&_now, NULL);',
'#line %(event_lineno)d "%(event_filename)s"',
-'qemu_log("%%d@%%zu.%%06zu:%(name)s " %(fmt)s "\\n",',
-' qemu_get_thread_id(),',
-' (size_t)_now.tv_sec, (size_t)_now.tv_usec',
-' %(argnames)s);',
+'qemu_log("%%d@%%zu.%%06zu:%(name)s " %(fmt)s "\\n",',
+' qemu_get_thread_id(),',
+' (size_t)_now.tv_sec, (size_t)_now.tv_usec',
+' %(argnames)s);',
'#line %(out_next_lineno)d "%(out_filename)s"',
+'} else {',
+'#line %(event_lineno)d "%(event_filename)s"',
+'qemu_log("%(name)s " %(fmt)s "\\n"%(argnames)s);',
+'#line %(out_next_lineno)d "%(out_filename)s"',
+'}',
'}',
cond=cond,
event_lineno=event.lineno,
--
2.29.2
[PULL 07/11] error: rename error_with_timestamp to message_with_timestamp
The -msg timestamp=on|off option controls whether a timestamp is printed
with error_report() messages. The "-msg" name suggests that this option
has a wider effect than just error_report(). The next patch extends it
to the 'log' trace backend, so rename the variable from
error_with_timestamp to message_with_timestamp.
Signed-off-by: Stefan Hajnoczi
Tested-by: Philippe Mathieu-Daudé
Tested-by: BALATON Zoltan
Reviewed-by: Philippe Mathieu-Daudé
Message-id: 20210125113507.224287-2-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
include/qemu/error-report.h | 2 +-
softmmu/vl.c| 2 +-
util/qemu-error.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index a5ad95ff1b..9d197daca3 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -74,7 +74,7 @@ void error_init(const char *argv0);
const char *error_get_progname(void);
-extern bool error_with_timestamp;
+extern bool message_with_timestamp;
extern bool error_with_guestname;
extern const char *error_guest_name;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index a8876b8965..bd55468669 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -737,7 +737,7 @@ static void realtime_init(void)
static void configure_msg(QemuOpts *opts)
{
-error_with_timestamp = qemu_opt_get_bool(opts, "timestamp", false);
+message_with_timestamp = qemu_opt_get_bool(opts, "timestamp", false);
error_with_guestname = qemu_opt_get_bool(opts, "guest-name", false);
}
diff --git a/util/qemu-error.c b/util/qemu-error.c
index aa30f03564..52a9e013c4 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -25,7 +25,7 @@ typedef enum {
} report_type;
/* Prepend timestamp to messages */
-bool error_with_timestamp;
+bool message_with_timestamp;
bool error_with_guestname;
const char *error_guest_name;
@@ -208,7 +208,7 @@ static void vreport(report_type type, const char *fmt,
va_list ap)
GTimeVal tv;
gchar *timestr;
-if (error_with_timestamp && !monitor_cur()) {
+if (message_with_timestamp && !monitor_cur()) {
g_get_current_time(&tv);
timestr = g_time_val_to_iso8601(&tv);
error_printf("%s ", timestr);
--
2.29.2
[PULL 01/11] trace: fix simpletrace doc mismerge
The simpletrace documentation section was accidentally split when the
ftrace section was introduced. Move the simpletrace-specific
documentation back into the simpletrace section.
Fixes: e64dd5efb2c6d522a3bc9d096cd49a4e53f0ae10 ("trace: document ftrace
backend")
Reviewed-by: Peter Maydell
Signed-off-by: Stefan Hajnoczi
Message-id: 20201216160923.722894-2-stefa...@redhat.com
Signed-off-by: Stefan Hajnoczi
---
docs/devel/tracing.txt | 34 +-
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/docs/devel/tracing.txt b/docs/devel/tracing.txt
index dba43fc7a4..313b8ea4e9 100644
--- a/docs/devel/tracing.txt
+++ b/docs/devel/tracing.txt
@@ -218,6 +218,23 @@ source tree. It may not be as powerful as
platform-specific or third-party
trace backends but it is portable. This is the recommended trace backend
unless you have specific needs for more advanced backends.
+ Monitor commands
+
+* trace-file on|off|flush|set
+ Enable/disable/flush the trace file or set the trace file name.
+
+ Analyzing trace files
+
+The "simple" backend produces binary trace files that can be formatted with the
+simpletrace.py script. The script takes the "trace-events-all" file and the
+binary trace:
+
+./scripts/simpletrace.py trace-events-all trace-12345
+
+You must ensure that the same "trace-events-all" file was used to build QEMU,
+otherwise trace event declarations may have changed and output will not be
+consistent.
+
=== Ftrace ===
The "ftrace" backend writes trace data to ftrace marker. This effectively
@@ -246,23 +263,6 @@ NOTE: syslog may squash duplicate consecutive trace events
and apply rate
Restriction: "syslog" backend is restricted to POSIX compliant OS.
- Monitor commands
-
-* trace-file on|off|flush|set
- Enable/disable/flush the trace file or set the trace file name.
-
- Analyzing trace files
-
-The "simple" backend produces binary trace files that can be formatted with the
-simpletrace.py script. The script takes the "trace-events-all" file and the
-binary trace:
-
-./scripts/simpletrace.py trace-events-all trace-12345
-
-You must ensure that the same "trace-events-all" file was used to build QEMU,
-otherwise trace event declarations may have changed and output will not be
-consistent.
-
=== LTTng Userspace Tracer ===
The "ust" backend uses the LTTng Userspace Tracer library. There are no
--
2.29.2
[PULL 00/11] Tracing patches
The following changes since commit 74208cd252c5da9d867270a178799abd802b9338:
Merge remote-tracking branch
'remotes/berrange-gitlab/tags/misc-fixes-pull-request' into staging (2021-01-29
19:51:25 +)
are available in the Git repository at:
https://gitlab.com/stefanha/qemu.git tags/tracing-pull-request
for you to fetch changes up to 0dfb3ca73c54fc105ab78e37e31ab05bed1360aa:
trace: update docs with meson build information (2021-02-01 11:23:04 +)
Pull request
Daniel P. Berrangé (1):
tracetool: also strip %l and %ll from systemtap format strings
Laurent Vivier (1):
tracetool: fix "PRI" macro decoding
Stefan Hajnoczi (8):
trace: fix simpletrace doc mismerge
tracing: convert documentation to rST
trace: recommend "log" backend for getting started with tracing
trace: add meson custom_target() depend_files for tracetool
error: rename error_with_timestamp to message_with_timestamp
trace: make the 'log' backend timestamp configurable
trace: document how to specify multiple --trace patterns
trace: update docs with meson build information
Volker Rümelin (1):
simpletrace: build() missing 2 required positional arguments
docs/devel/index.rst| 1 +
docs/devel/{tracing.txt => tracing.rst} | 248 ++--
meson.build | 28 ++-
include/qemu/error-report.h | 2 +-
softmmu/vl.c| 2 +-
util/qemu-error.c | 4 +-
scripts/simpletrace.py | 4 +-
scripts/tracetool/backend/log.py| 19 +-
scripts/tracetool/format/log_stap.py| 8 +-
trace/meson.build | 21 +-
10 files changed, 216 insertions(+), 121 deletions(-)
rename docs/devel/{tracing.txt => tracing.rst} (72%)
--
2.29.2
Re: [PATCH v4 0/2] nbd/server: Quiesce coroutines on context switch
Am 01.02.2021 um 13:50 hat Sergio Lopez geschrieben: > This series allows the NBD server to properly switch between AIO contexts, > having quiesced recv_coroutine and send_coroutine before doing the transition. > > We need this because we send back devices running in IO Thread owned contexts > to the main context when stopping the data plane, something that can happen > multiple times during the lifetime of a VM (usually during the boot sequence > or > on a reboot), and we drag the NBD server of the correspoing export with it. > > While there, fix also a problem caused by a cross-dependency between > closing the export's client connections and draining the block > layer. The visible effect of this problem was QEMU getting hung when > the guest request a power off while there's an active NBD client. Reviewed-by: Kevin Wolf
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
Am 01.02.2021 um 12:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > 28.01.2021 18:28, John Snow wrote: > > On 1/28/21 10:09 AM, Markus Armbruster wrote: > > > Vladimir Sementsov-Ogievskiy writes: > > > > > > > I'm developing Qemu backup for several years, and finally new backup > > > > architecture, including block-copy generic engine and backup-top filter > > > > landed upstream, great thanks to reviewers and especially to > > > > Max Reitz! > > > > > > > > I also have plans of moving other block-jobs onto block-copy, so that > > > > we finally have one generic block copying path, fast and well-formed. > > > > > > > > So, now I suggest to bring all parts of backup architecture into > > > > "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and > > > > qemu-co-shared-resource can be reused somewhere else, but I'd keep an > > > > eye on them in context of block-jobs) and add myself as co-maintainer. > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > > > > With pleasure: > > > Reviewed-by: Markus Armbruster > > > > > > > Absolutely! Glad to see it. > > > > Reviewed-by: John Snow > > > > [..] > > > Great! > > > > Reviewed-by: Max Reitz > > Thanks! > > Could someone pull it? I've put it in my block branch (with s/suggest myself/Add Vladimir/ in the subject line), but I don't know when I'll send the next pull request. If someone else sends one first, feel free to include it with: Acked-by: Kevin Wolf > I don't have any signed PGP key for now, to send pull requests :\ > Interesting, could I get one while sitting in Moscow? If you're planning to send pull requests, should a git tree of yours be added to the MAINTAINERS sections, too? Kevin
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
On Mon, Feb 01, 2021 at 01:50:26PM +, Alex Bennée wrote: > > Vladimir Sementsov-Ogievskiy writes: > > > 28.01.2021 18:28, John Snow wrote: > >> On 1/28/21 10:09 AM, Markus Armbruster wrote: > >>> Vladimir Sementsov-Ogievskiy writes: > >>> > I'm developing Qemu backup for several years, and finally new backup > architecture, including block-copy generic engine and backup-top filter > landed upstream, great thanks to reviewers and especially to > Max Reitz! > > >> Great! > >> > >> Reviewed-by: Max Reitz > > > > > > Thanks! > > > > Could someone pull it? > > > > I don't have any signed PGP key for now, to send pull requests :\ > > Interesting, could I get one while sitting in Moscow? > > Hmm this does point somewhat to a hole in our maintainer process that has > previously relied on semi-regular physical meet-up for key signing > purposes. It might be some time before we return to a new normal. Are > there any other maintainers in Moscow that you could safely meet for a > socially distanced key-signing? AFAIK, we've never actually set expectations for what process a key signing must use. Merely that key should be signed by one or more people who are already QEMU maintainers. It is more or less up to the person signing the key what hoops they want to jump through before adding their signature. So while physically meeting is a traditional standard, some might be fine to do key signing verification over a video conference system, especially if both sides already know each other by sight after previous physical meetings. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v6 01/11] sysemu/tcg: Introduce tcg_builtin() helper
On 1/31/21 4:23 PM, Philippe Mathieu-Daudé wrote:
> On 1/31/21 3:18 PM, Claudio Fontana wrote:
>> On 1/31/21 12:50 PM, Philippe Mathieu-Daudé wrote:
>>> Modules are registered early with type_register_static().
>>>
>>> We would like to call tcg_enabled() when registering QOM types,
>>
>>
>> Hi Philippe,
>>
>> could this not be controlled by meson at this stage?
>> On X86, I register the tcg-specific types in tcg/* in modules that are only
>> built for TCG.
>>
>> Maybe tcg_builtin() is useful anyway, thinking long term at loadable modules,
>> but there we are interested in whether tcg code is available or not,
>> regardless of whether it's builtin,
>> or needs to be loaded via a .so plugin..
>>
>> maybe tcg_available()?
>
> The alternatives I found:
>
> - reorder things in vl.c?
>
> - use ugly #ifdef'ry, see this patch:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg08037.html
Not sure it's that ugly,
if it is followed (or replaced by) exporting those pieces to separate files,
which are only built by meson on CONFIG_TCG.
I did not try to do it, so you know best of course.
Ciao,
Claudio
>
> - this earlier approach I previously discarded:
>
> -- >8 --
> diff --git a/include/qom/object.h b/include/qom/object.h
> index d378f13a116..30590c6fac3 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -403,9 +403,12 @@ struct Object
> * parent class initialization has occurred, but before the class itself
> * is initialized. This is the function to use to undo the effects of
> * memcpy from the parent class to the descendants.
> - * @class_data: Data to pass to the @class_init,
> + * @class_data: Data to pass to the @class_registerable, @class_init,
> * @class_base_init. This can be useful when building dynamic
> * classes.
> + * @registerable: This function is called when modules are registered,
> + * prior to any class initialization. When present and returning %false,
> + * the type is not registered, the class is not present (not usable).
> * @interfaces: The list of interfaces associated with this type. This
> * should point to a static array that's terminated with a zero filled
> * element.
> @@ -428,6 +431,7 @@ struct TypeInfo
> void (*class_base_init)(ObjectClass *klass, void *data);
> void *class_data;
>
> +bool (*registerable)(void *data);
> InterfaceInfo *interfaces;
> };
>
> diff --git a/qom/object.c b/qom/object.c
> index 2fa0119647c..0febaffa12e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -138,6 +138,10 @@ static TypeImpl *type_new(const TypeInfo *info)
> static TypeImpl *type_register_internal(const TypeInfo *info)
> {
> TypeImpl *ti;
> +
> +if (info->registerable && !info->registerable(info->class_data)) {
> +return NULL;
> +}
> ti = type_new(info);
>
> type_table_add(ti);
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 990509d3852..1a2b1889da4 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -24,6 +24,7 @@
> #include "hw/loader.h"
> #include "hw/arm/boot.h"
> #include "sysemu/sysemu.h"
> +#include "sysemu/tcg.h"
> #include "qom/object.h"
>
> #define SMPBOOT_ADDR0x300 /* this should leave enough space for
> ATAGS */
> @@ -368,18 +369,26 @@ static void raspi3b_machine_class_init(ObjectClass
> *oc, void *data)
> };
> #endif /* TARGET_AARCH64 */
>
> +static bool raspi_machine_requiring_tcg_accel(void *data)
> +{
> +return tcg_builtin();
> +}
> +
> static const TypeInfo raspi_machine_types[] = {
> {
> .name = MACHINE_TYPE_NAME("raspi0"),
> .parent = TYPE_RASPI_MACHINE,
> +.registerable = raspi_machine_requiring_tcg_accel,
> .class_init = raspi0_machine_class_init,
> }, {
> .name = MACHINE_TYPE_NAME("raspi1ap"),
> .parent = TYPE_RASPI_MACHINE,
> +.registerable = raspi_machine_requiring_tcg_accel,
> .class_init = raspi1ap_machine_class_init,
> }, {
> .name = MACHINE_TYPE_NAME("raspi2b"),
> .parent = TYPE_RASPI_MACHINE,
> +.registerable = raspi_machine_requiring_tcg_accel,
> .class_init = raspi2b_machine_class_init,
> #ifdef TARGET_AARCH64
> }, {
> ---
>
>>
>> Ciao,
>>
>> Claudio
>>
>>> but tcg_enabled() returns tcg_allowed which is a runtime property
>>> initialized later (See commit 2f181fbd5a9 which introduced the
>>> MachineInitPhase in "hw/qdev-core.h" representing the different
>>> phases of machine initialization and commit 0427b6257e2 which
>>> document the initialization order).
>>>
>>> As we are only interested if the TCG accelerator is builtin,
>>> regardless of being enabled, introduce the tcg_builtin() helper.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé
>>> ---
>>> Cc: Markus Armbruster
>>> ---
>>> include/sysemu/tcg.h | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/include/sysemu/tcg.h b/include/sy
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
Vladimir Sementsov-Ogievskiy writes: > 28.01.2021 18:28, John Snow wrote: >> On 1/28/21 10:09 AM, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy writes: >>> I'm developing Qemu backup for several years, and finally new backup architecture, including block-copy generic engine and backup-top filter landed upstream, great thanks to reviewers and especially to Max Reitz! >> Great! >> >> Reviewed-by: Max Reitz > > > Thanks! > > Could someone pull it? > > I don't have any signed PGP key for now, to send pull requests :\ > Interesting, could I get one while sitting in Moscow? Hmm this does point somewhat to a hole in our maintainer process that has previously relied on semi-regular physical meet-up for key signing purposes. It might be some time before we return to a new normal. Are there any other maintainers in Moscow that you could safely meet for a socially distanced key-signing? -- Alex Bennée
Re: [PATCH v6 02/11] exec: Restrict TCG specific headers
Philippe Mathieu-Daudé writes: > Fixes when building with --disable-tcg on ARM: > > In file included from target/arm/helper.c:16: > include/exec/helper-proto.h:42:10: fatal error: tcg-runtime.h: No such file > or directory > 42 | #include "tcg-runtime.h" > | ^~~ I think the problem here is that we have stuff in helper.c which is needed by non-TCG builds. > > Signed-off-by: Philippe Mathieu-Daudé > --- > include/exec/helper-proto.h | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/include/exec/helper-proto.h b/include/exec/helper-proto.h > index 659f9298e8f..740bff3bb4d 100644 > --- a/include/exec/helper-proto.h > +++ b/include/exec/helper-proto.h > @@ -39,8 +39,10 @@ dh_ctype(ret) HELPER(name) (dh_ctype(t1), dh_ctype(t2), > dh_ctype(t3), \ > > #include "helper.h" > #include "trace/generated-helpers.h" > +#ifdef CONFIG_TCG > #include "tcg-runtime.h" > #include "plugin-helpers.h" > +#endif /* CONFIG_TCG */ If we are including helper-proto.h then we are defining helpers which are (should be) TCG only. > > #undef IN_HELPER_PROTO -- Alex Bennée
Re:Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
Peng, In my analysis, the root casue should be the lock: aio_context, qemu main thread do an unnecessary release/aquire action, That's why IO thread could get the lock it shouldn't hold at this stage. Thanks, Michael At 2021-02-01 20:44:00, "Vladimir Sementsov-Ogievskiy" wrote: >Hi! > >01.02.2021 15:07, Peng Liang wrote: >> Hi, >> >> I encountered the problem months ago too. Could we move the creation of >> the block job (block_job_create) before appending the new bs to >> mirror_top_bs (bdrv_append) as I wrote in [*]? I found that after >> bdrv_append, qemu will use mirror_top_bs to do write. And when writing, >> qemu will use bs->opaque, which maybe NULL. >> >> [*] >> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200826131910.1879079-1-liangpen...@huawei.com/ >> > >In this patch you create job over original bs, when jobs are normally created >over job-filter bs. I don't know is it wrong, but it at least requires some >research, and probably the code that removes the filter should be adjusted >somehow. Also, you make bs->opaque be non-zero. But still, job is not fully >initialized, and some another problem may occur. So, do we create job prior to >filter insertion or after it, parallel io requests to bs should not interrupt >mirror_start_job(). So I think Michael's patch is closer to real problem to >fix. > > >-- >Best regards, >Vladimir
[PATCH v4 2/2] block: move blk_exp_close_all() to qemu_cleanup()
Move blk_exp_close_all() from bdrv_close() to qemu_cleanup(), before
bdrv_drain_all_begin().
Export drivers may have coroutines yielding at some point in the block
layer, so we need to shut them down before draining the block layer,
as otherwise they may get stuck blk_wait_while_drained().
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505
Signed-off-by: Sergio Lopez
---
block.c | 1 -
qemu-nbd.c | 1 +
softmmu/runstate.c | 9 +
storage-daemon/qemu-storage-daemon.c | 1 +
4 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 3da99312db..9682c82fa8 100644
--- a/block.c
+++ b/block.c
@@ -4435,7 +4435,6 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
assert(job_next(NULL) == NULL);
-blk_exp_close_all();
/* Drop references from requests still in flight, such as canceled block
* jobs whose AIO context has not been polled yet */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0d513cb38c..608c63e82a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -503,6 +503,7 @@ static const char *socket_activation_validate_opts(const
char *device,
static void qemu_nbd_shutdown(void)
{
job_cancel_sync_all();
+blk_exp_close_all();
bdrv_close_all();
}
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 6177693a30..ac4b2e2540 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -25,6 +25,7 @@
#include "qemu/osdep.h"
#include "audio/audio.h"
#include "block/block.h"
+#include "block/export.h"
#include "chardev/char.h"
#include "crypto/cipher.h"
#include "crypto/init.h"
@@ -783,6 +784,14 @@ void qemu_cleanup(void)
*/
migration_shutdown();
+/*
+ * Close the exports before draining the block layer. The export
+ * drivers may have coroutines yielding on it, so we need to clean
+ * them up before the drain, as otherwise they may be get stuck in
+ * blk_wait_while_drained().
+ */
+blk_exp_close_all();
+
/*
* We must cancel all block jobs while the block layer is drained,
* or cancelling will be affected by throttling and thus may block
diff --git a/storage-daemon/qemu-storage-daemon.c
b/storage-daemon/qemu-storage-daemon.c
index e0c87edbdd..d8d172cc60 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -314,6 +314,7 @@ int main(int argc, char *argv[])
main_loop_wait(false);
}
+blk_exp_close_all();
bdrv_drain_all_begin();
bdrv_close_all();
--
2.26.2
[PATCH v4 1/2] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
Some graphs may contain an indirect reference to the first BDS in the
chain that can be reached while walking it bottom->up from one its
children.
Doubling-processing of a BDS is especially problematic for the
aio_notifiers, as they might attempt to work on both the old and the
new AIO contexts.
To avoid this problem, add every child and parent to the ignore list
before actually processing them.
Suggested-by: Kevin Wolf
Signed-off-by: Sergio Lopez
---
block.c | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index 8b9d457546..3da99312db 100644
--- a/block.c
+++ b/block.c
@@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
AioContext *new_context, GSList **ignore)
{
AioContext *old_context = bdrv_get_aio_context(bs);
-BdrvChild *child;
+GSList *children_to_process = NULL;
+GSList *parents_to_process = NULL;
+GSList *entry;
+BdrvChild *child, *parent;
g_assert(qemu_get_current_aio_context() == qemu_get_aio_context());
@@ -6429,16 +6432,33 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
continue;
}
*ignore = g_slist_prepend(*ignore, child);
-bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+children_to_process = g_slist_prepend(children_to_process, child);
}
-QLIST_FOREACH(child, &bs->parents, next_parent) {
-if (g_slist_find(*ignore, child)) {
+
+QLIST_FOREACH(parent, &bs->parents, next_parent) {
+if (g_slist_find(*ignore, parent)) {
continue;
}
-assert(child->klass->set_aio_ctx);
-*ignore = g_slist_prepend(*ignore, child);
-child->klass->set_aio_ctx(child, new_context, ignore);
+*ignore = g_slist_prepend(*ignore, parent);
+parents_to_process = g_slist_prepend(parents_to_process, parent);
+}
+
+for (entry = children_to_process;
+ entry != NULL;
+ entry = g_slist_next(entry)) {
+child = entry->data;
+bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+}
+g_slist_free(children_to_process);
+
+for (entry = parents_to_process;
+ entry != NULL;
+ entry = g_slist_next(entry)) {
+parent = entry->data;
+assert(parent->klass->set_aio_ctx);
+parent->klass->set_aio_ctx(parent, new_context, ignore);
}
+g_slist_free(parents_to_process);
bdrv_detach_aio_context(bs);
--
2.26.2
[PATCH v4 0/2] nbd/server: Quiesce coroutines on context switch
This series allows the NBD server to properly switch between AIO contexts, having quiesced recv_coroutine and send_coroutine before doing the transition. We need this because we send back devices running in IO Thread owned contexts to the main context when stopping the data plane, something that can happen multiple times during the lifetime of a VM (usually during the boot sequence or on a reboot), and we drag the NBD server of the correspoing export with it. While there, fix also a problem caused by a cross-dependency between closing the export's client connections and draining the block layer. The visible effect of this problem was QEMU getting hung when the guest request a power off while there's an active NBD client. v4: - Call to blk_exp_close_all() from qemu-nbd and qemu-storage-daemon too. (Kevin Wolf) v3: - Drop already merged "block: Honor blk_set_aio_context() context requirements" and "nbd/server: Quiesce coroutines on context switch" - Change the strategy for avoiding processing BDS twice to adding every child and parent to the ignore list in advance before processing them. (Kevin Wolf) - Replace "nbd/server: Quiesce coroutines on context switch" with "block: move blk_exp_close_all() to qemu_cleanup()" v2: - Replace "virtio-blk: Acquire context while switching them on dataplane start" with "block: Honor blk_set_aio_context() context requirements" (Kevin Wolf) - Add "block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()" - Add "block: Close block exports in two steps" - Rename nbd_read_eof() to nbd_server_read_eof() (Eric Blake) - Fix double space and typo in comment. (Eric Blake) Sergio Lopez (2): block: Avoid processing BDS twice in bdrv_set_aio_context_ignore() block: move blk_exp_close_all() to qemu_cleanup() block.c | 35 +--- qemu-nbd.c | 1 + softmmu/runstate.c | 9 +++ storage-daemon/qemu-storage-daemon.c | 1 + 4 files changed, 38 insertions(+), 8 deletions(-) -- 2.26.2
Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
Hi! 01.02.2021 15:07, Peng Liang wrote: Hi, I encountered the problem months ago too. Could we move the creation of the block job (block_job_create) before appending the new bs to mirror_top_bs (bdrv_append) as I wrote in [*]? I found that after bdrv_append, qemu will use mirror_top_bs to do write. And when writing, qemu will use bs->opaque, which maybe NULL. [*] http://patchwork.ozlabs.org/project/qemu-devel/patch/20200826131910.1879079-1-liangpen...@huawei.com/ In this patch you create job over original bs, when jobs are normally created over job-filter bs. I don't know is it wrong, but it at least requires some research, and probably the code that removes the filter should be adjusted somehow. Also, you make bs->opaque be non-zero. But still, job is not fully initialized, and some another problem may occur. So, do we create job prior to filter insertion or after it, parallel io requests to bs should not interrupt mirror_start_job(). So I think Michael's patch is closer to real problem to fix. -- Best regards, Vladimir
Re: [PATCH v3 2/2] block: move blk_exp_close_all() to qemu_cleanup()
On Mon, Feb 01, 2021 at 01:20:30PM +0100, Kevin Wolf wrote: > Am 21.01.2021 um 18:07 hat Sergio Lopez geschrieben: > > Move blk_exp_close_all() from bdrv_close() to qemu_cleanup(), before > > bdrv_drain_all_begin(). > > > > Export drivers may have coroutines yielding at some point in the block > > layer, so we need to shut them down before draining the block layer, > > as otherwise they may get stuck blk_wait_while_drained(). > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505 > > Signed-off-by: Sergio Lopez > > This patch loses the call in qemu-nbd and qemu-storage-daemon. You're right, I'll prepare a v4 right away. Thanks, Sergio. signature.asc Description: PGP signature
Re: [PATCH v3 1/2] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
On Mon, Feb 01, 2021 at 01:06:31PM +0100, Kevin Wolf wrote:
> Am 21.01.2021 um 18:06 hat Sergio Lopez geschrieben:
> > Some graphs may contain an indirect reference to the first BDS in the
> > chain that can be reached while walking it bottom->up from one its
> > children.
> >
> > Doubling-processing of a BDS is especially problematic for the
> > aio_notifiers, as they might attempt to work on both the old and the
> > new AIO contexts.
> >
> > To avoid this problem, add every child and parent to the ignore list
> > before actually processing them.
> >
> > Suggested-by: Kevin Wolf
> > Signed-off-by: Sergio Lopez
> > ---
> > block.c | 34 +++---
> > 1 file changed, 27 insertions(+), 7 deletions(-)
>
> The patch looks correct to me, I'm just wondering about one thing:
>
> > diff --git a/block.c b/block.c
> > index 8b9d457546..3da99312db 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState
> > *bs,
> > AioContext *new_context, GSList **ignore)
> > {
> > AioContext *old_context = bdrv_get_aio_context(bs);
> > -BdrvChild *child;
> > +GSList *children_to_process = NULL;
> > +GSList *parents_to_process = NULL;
>
> Why do we need these separate lists? Can't we just iterate over
> bs->parents/children a second time? I don't think the graph can change
> between the first and the second loop (and if it could, the result would
> be broken anyway).
It's not strictly needed, but this makes the code more readable by
making our intentions clearer. To my eyes, at least.
Sergio.
signature.asc
Description: PGP signature
Re: [PATCH v3 2/2] block: move blk_exp_close_all() to qemu_cleanup()
Am 21.01.2021 um 18:07 hat Sergio Lopez geschrieben: > Move blk_exp_close_all() from bdrv_close() to qemu_cleanup(), before > bdrv_drain_all_begin(). > > Export drivers may have coroutines yielding at some point in the block > layer, so we need to shut them down before draining the block layer, > as otherwise they may get stuck blk_wait_while_drained(). > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505 > Signed-off-by: Sergio Lopez This patch loses the call in qemu-nbd and qemu-storage-daemon. Kevin
Re: [PATCH] block/mirror: fix core when using iothreads
On 9/23/2020 10:50 PM, John Snow wrote: > On 8/26/20 9:19 AM, Peng Liang wrote: >> We found an issue when doing block-commit with iothreads, which tries to >> dereference a NULL pointer. >> > > I'm clearing out my patch backlog. I am a bit out of the loop on block > issues at the moment, did this issue get addressed in a later patch that > I am not seeing, or does it still need attention? > > --js > Hi John, I'm very sorry for missing your reply. Michael encountered the problem too and gave a reproducer script. Thanks, Peng
Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
Hi, I encountered the problem months ago too. Could we move the creation of the block job (block_job_create) before appending the new bs to mirror_top_bs (bdrv_append) as I wrote in [*]? I found that after bdrv_append, qemu will use mirror_top_bs to do write. And when writing, qemu will use bs->opaque, which maybe NULL. [*] http://patchwork.ozlabs.org/project/qemu-devel/patch/20200826131910.1879079-1-liangpen...@huawei.com/ Thanks, Peng On 2/1/2021 7:26 PM, 仇大玉 wrote: > I'm so sorry, forgive my mail client(outlook) > > I have try your solution, It doesn't work, still cause crash. > > The reason is: we come to bdrv_mirror_top_pwritev() (which means that > mirror-top node is already inserted into block graph), but its > bs->opaque->job is not initialized" > > But the root cause is that in block_job_create() we released(unnecessary) the > aio_context, and the iothread get the context. > > Script has to part, one is run in the VM (to give some workload) we named > script A: > #!/bin/sh > For((i=1;i<=1;i++)); > Do > dd if=/dev/zero of=./xxx bs=1M count=200 > sleep 6 > done > > Another one is in the hypervisor, we named script B: > #!/bin/sh > for((i=1;i<=1000;i++)); > do > virsh snapshot-create-as fqtest --atomic --no-metadata --name fq6 --disk-only > --diskspec vda,snapshot=external,file=/home/michael/snapshot/fq6.qcow2; > virsh blockcommit fqtest /home/michael/snapshot/fq6.qcow2 --shallow --verbose > --wait --pivot --top /home/michael/snapshot/fq6.qcow2; > rm -r fq6.qcow2 > done > > How to reproduce: > 1. start a VM, my case is use libvirt, named fqtest > 2. run script B in hypervisor > 3. after guest boot up, login and run script A in vda. > > Make sure, the IO thread enabled for vda. > > Mostly, just wait for several minutes, it will crash. > > The whole thread backtrace is: > > [Switching to Thread 0x7f7c7d91f700 (LWP 99907)] > 0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437 > 1437../block/mirror.c: No such file or directory. > (gdb) p s->job > $17 = (MirrorBlockJob *) 0x0 > (gdb) p s->stop > $18 = false > > (gdb) bt > #0 0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437 > #1 0x5576d0f7f3ab in bdrv_driver_pwritev at ../block/io.c:1174 > #2 0x5576d0f8139d in bdrv_aligned_pwritev at ../block/io.c:1988 > #3 0x5576d0f81b65 in bdrv_co_pwritev_part at ../block/io.c:2156 > #4 0x5576d0f8e6b7 in blk_do_pwritev_part at ../block/block-backend.c:1260 > #5 0x5576d0f8e84d in blk_aio_write_entry at ../block/block-backend.c:1476 > #6 0x5576d1060ddb in coroutine_trampoline at > ../util/coroutine-ucontext.c:173 > #7 0x7f7c8d3be0d0 in __start_context at /lib/../lib64/libc.so.6 > #8 0x7f7b52beb1e0 in () > #9 0x in () > > Switch to qemu main thread: > #0 0x7f903be704ed in __lll_lock_wait at > /lib/../lib64/libpthread.so.0 > #1 0x7f903be6bde6 in _L_lock_941 at /lib/../lib64/libpthread.so.0 > #2 0x7f903be6bcdf in pthread_mutex_lock at > /lib/../lib64/libpthread.so.0 > #3 0x564b21456889 in qemu_mutex_lock_impl at > ../util/qemu-thread-posix.c:79 > #4 0x564b213af8a5 in block_job_add_bdrv at ../blockjob.c:224 > #5 0x564b213b00ad in block_job_create at ../blockjob.c:440 > #6 0x564b21357c0a in mirror_start_job at ../block/mirror.c:1622 > #7 0x564b2135a9af in commit_active_start at ../block/mirror.c:1867 > #8 0x564b2133d132 in qmp_block_commit at ../blockdev.c:2768 > #9 0x564b2141fef3 in qmp_marshal_block_commit at > qapi/qapi-commands-block-core.c:346 > #10 0x564b214503c9 in do_qmp_dispatch_bh at > ../qapi/qmp-dispatch.c:110 > #11 0x564b21451996 in aio_bh_poll at ../util/async.c:164 > #12 0x564b2146018e in aio_dispatch at ../util/aio-posix.c:381 > #13 0x564b2145187e in aio_ctx_dispatch at ../util/async.c:306 > #14 0x7f9040239049 in g_main_context_dispatch at > /lib/../lib64/libglib-2.0.so.0 > #15 0x564b21447368 in main_loop_wait at ../util/main-loop.c:232 > #16 0x564b21447368 in main_loop_wait at ../util/main-loop.c:255 > #17 0x564b21447368 in main_loop_wait at ../util/main-loop.c:531 > #18 0x564b212304e1 in qemu_main_loop at ../softmmu/runstate.c:721 > #19 0x564b20f7975e in main at ../softmmu/main.c:50 > > Thanks, > Michael > > -Original Message- > From: Vladimir Sementsov-Ogievskiy > Sent: 2021年2月1日 18:28 > To: 08005...@163.com; kw...@redhat.com; mre...@redhat.com; js...@redhat.com > Cc: 仇大玉 ; qemu-de...@nongnu.org; qemu-block@nongnu.org > Subject: Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit > after snapshot > > Hi! > > Tanks for fixing and sorry for a delay! > > Please send each new version of a patch as a separate branch. It's a rule > from https://wiki.qemu.org/Contribute/SubmitAPatch and it is more readable > and less probable that your patch will be missed. > > 28.01.2021 04:30, 08005...@163.com wrote: >> From: Michael Qiu >> >> v4: rebase to lates
Re: [PATCH v3 1/2] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
Am 21.01.2021 um 18:06 hat Sergio Lopez geschrieben:
> Some graphs may contain an indirect reference to the first BDS in the
> chain that can be reached while walking it bottom->up from one its
> children.
>
> Doubling-processing of a BDS is especially problematic for the
> aio_notifiers, as they might attempt to work on both the old and the
> new AIO contexts.
>
> To avoid this problem, add every child and parent to the ignore list
> before actually processing them.
>
> Suggested-by: Kevin Wolf
> Signed-off-by: Sergio Lopez
> ---
> block.c | 34 +++---
> 1 file changed, 27 insertions(+), 7 deletions(-)
The patch looks correct to me, I'm just wondering about one thing:
> diff --git a/block.c b/block.c
> index 8b9d457546..3da99312db 100644
> --- a/block.c
> +++ b/block.c
> @@ -6414,7 +6414,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
> AioContext *new_context, GSList **ignore)
> {
> AioContext *old_context = bdrv_get_aio_context(bs);
> -BdrvChild *child;
> +GSList *children_to_process = NULL;
> +GSList *parents_to_process = NULL;
Why do we need these separate lists? Can't we just iterate over
bs->parents/children a second time? I don't think the graph can change
between the first and the second loop (and if it could, the result would
be broken anyway).
Kevin
Re: [PATCH 10/10] target: Move SEMIHOSTING feature to target Kconfig
Philippe Mathieu-Daudé writes: > SEMIHOSTING is an architecture feature, move its declaration to > each target/ARCH/. I'm going to punt on this one and leave it to the arch maintainers to opine because AIUI in a lot of cases semihosting is more of a "useful hack" than something mandated by the architecture. > > Signed-off-by: Philippe Mathieu-Daudé > --- > default-configs/devices/lm32-softmmu.mak| 2 -- > default-configs/devices/m68k-softmmu.mak| 2 -- > default-configs/devices/mips-softmmu-common.mak | 3 --- > default-configs/devices/nios2-softmmu.mak | 2 -- > default-configs/devices/unicore32-softmmu.mak | 1 - > default-configs/devices/xtensa-softmmu.mak | 2 -- > target/lm32/Kconfig | 1 + > target/m68k/Kconfig | 1 + > target/mips/Kconfig | 1 + > target/nios2/Kconfig| 1 + > target/unicore32/Kconfig| 1 + > target/xtensa/Kconfig | 1 + > 12 files changed, 6 insertions(+), 12 deletions(-) > > diff --git a/default-configs/devices/lm32-softmmu.mak > b/default-configs/devices/lm32-softmmu.mak > index 1bce3f6e8b6..1f69795b749 100644 > --- a/default-configs/devices/lm32-softmmu.mak > +++ b/default-configs/devices/lm32-softmmu.mak > @@ -4,8 +4,6 @@ > # > #CONFIG_MILKYMIST_TMU2=n# disabling it actually causes compile-time > failures > > -CONFIG_SEMIHOSTING=y > - > # Boards: > # > CONFIG_LM32_EVR=y > diff --git a/default-configs/devices/m68k-softmmu.mak > b/default-configs/devices/m68k-softmmu.mak > index 6629fd2aa33..4fef4bd731d 100644 > --- a/default-configs/devices/m68k-softmmu.mak > +++ b/default-configs/devices/m68k-softmmu.mak > @@ -1,7 +1,5 @@ > # Default configuration for m68k-softmmu > > -CONFIG_SEMIHOSTING=y > - > # Boards: > # > CONFIG_AN5206=y > diff --git a/default-configs/devices/mips-softmmu-common.mak > b/default-configs/devices/mips-softmmu-common.mak > index ea78fe72759..af652ec7bdd 100644 > --- a/default-configs/devices/mips-softmmu-common.mak > +++ b/default-configs/devices/mips-softmmu-common.mak > @@ -1,8 +1,5 @@ > # Common mips*-softmmu CONFIG defines > > -# CONFIG_SEMIHOSTING is always required on this architecture > -CONFIG_SEMIHOSTING=y > - > CONFIG_ISA_BUS=y > CONFIG_PCI=y > CONFIG_PCI_DEVICES=y > diff --git a/default-configs/devices/nios2-softmmu.mak > b/default-configs/devices/nios2-softmmu.mak > index 1bc4082ea99..e130d024e62 100644 > --- a/default-configs/devices/nios2-softmmu.mak > +++ b/default-configs/devices/nios2-softmmu.mak > @@ -1,7 +1,5 @@ > # Default configuration for nios2-softmmu > > -CONFIG_SEMIHOSTING=y > - > # Boards: > # > CONFIG_NIOS2_10M50=y > diff --git a/default-configs/devices/unicore32-softmmu.mak > b/default-configs/devices/unicore32-softmmu.mak > index 899288e3d71..0bfce48c6da 100644 > --- a/default-configs/devices/unicore32-softmmu.mak > +++ b/default-configs/devices/unicore32-softmmu.mak > @@ -3,4 +3,3 @@ > # Boards: > # > CONFIG_PUV3=y > -CONFIG_SEMIHOSTING=y > diff --git a/default-configs/devices/xtensa-softmmu.mak > b/default-configs/devices/xtensa-softmmu.mak > index 4fe1bf00c94..49e4c9da88c 100644 > --- a/default-configs/devices/xtensa-softmmu.mak > +++ b/default-configs/devices/xtensa-softmmu.mak > @@ -1,7 +1,5 @@ > # Default configuration for Xtensa > > -CONFIG_SEMIHOSTING=y > - > # Boards: > # > CONFIG_XTENSA_SIM=y > diff --git a/target/lm32/Kconfig b/target/lm32/Kconfig > index 09de5b703a3..286710fd47b 100644 > --- a/target/lm32/Kconfig > +++ b/target/lm32/Kconfig > @@ -1,2 +1,3 @@ > config LM32 > bool > +select SEMIHOSTING > diff --git a/target/m68k/Kconfig b/target/m68k/Kconfig > index 23debad519a..9eae71486ff 100644 > --- a/target/m68k/Kconfig > +++ b/target/m68k/Kconfig > @@ -1,2 +1,3 @@ > config M68K > bool > +select SEMIHOSTING > diff --git a/target/mips/Kconfig b/target/mips/Kconfig > index 6adf1453548..eb19c94c7d4 100644 > --- a/target/mips/Kconfig > +++ b/target/mips/Kconfig > @@ -1,5 +1,6 @@ > config MIPS > bool > +select SEMIHOSTING > > config MIPS64 > bool > diff --git a/target/nios2/Kconfig b/target/nios2/Kconfig > index 1529ab8950d..c65550c861a 100644 > --- a/target/nios2/Kconfig > +++ b/target/nios2/Kconfig > @@ -1,2 +1,3 @@ > config NIOS2 > bool > +select SEMIHOSTING > diff --git a/target/unicore32/Kconfig b/target/unicore32/Kconfig > index 62c9d10b38f..c699d5238ea 100644 > --- a/target/unicore32/Kconfig > +++ b/target/unicore32/Kconfig > @@ -1,2 +1,3 @@ > config UNICORE32 > bool > +select SEMIHOSTING > diff --git a/target/xtensa/Kconfig b/target/xtensa/Kconfig > index a3c8dc7f6d7..5e46049262d 100644 > --- a/target/xtensa/Kconfig > +++ b/target/xtensa/Kconfig > @@ -1,2 +1,3 @@ > config XTENSA > bool > +select SEMIHOSTING -- Alex Bennée
Re: [PATCH 09/10] target: Move ARM_COMPATIBLE_SEMIHOSTING feature to target Kconfig
Philippe Mathieu-Daudé writes: > ARM_COMPATIBLE_SEMIHOSTING is an architecture feature, move its > declaration to each target/ARCH/. > > Note, we do not modify the linux-user targets, as user-mode builds > don't use Kconfig. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH 08/10] default-configs: Remove unnecessary SEMIHOSTING selection
Philippe Mathieu-Daudé writes:
> Commit 56b5170c87e ("semihosting: Move ARM semihosting code to
> shared directories") selected ARM_COMPATIBLE_SEMIHOSTING which
> already selects SEMIHOSTING. No need to select it again.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alex Bennée
--
Alex Bennée
RE: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
I'm so sorry, forgive my mail client(outlook) I have try your solution, It doesn't work, still cause crash. The reason is: we come to bdrv_mirror_top_pwritev() (which means that mirror-top node is already inserted into block graph), but its bs->opaque->job is not initialized" But the root cause is that in block_job_create() we released(unnecessary) the aio_context, and the iothread get the context. Script has to part, one is run in the VM (to give some workload) we named script A: #!/bin/sh For((i=1;i<=1;i++)); Do dd if=/dev/zero of=./xxx bs=1M count=200 sleep 6 done Another one is in the hypervisor, we named script B: #!/bin/sh for((i=1;i<=1000;i++)); do virsh snapshot-create-as fqtest --atomic --no-metadata --name fq6 --disk-only --diskspec vda,snapshot=external,file=/home/michael/snapshot/fq6.qcow2; virsh blockcommit fqtest /home/michael/snapshot/fq6.qcow2 --shallow --verbose --wait --pivot --top /home/michael/snapshot/fq6.qcow2; rm -r fq6.qcow2 done How to reproduce: 1. start a VM, my case is use libvirt, named fqtest 2. run script B in hypervisor 3. after guest boot up, login and run script A in vda. Make sure, the IO thread enabled for vda. Mostly, just wait for several minutes, it will crash. The whole thread backtrace is: [Switching to Thread 0x7f7c7d91f700 (LWP 99907)] 0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437 1437../block/mirror.c: No such file or directory. (gdb) p s->job $17 = (MirrorBlockJob *) 0x0 (gdb) p s->stop $18 = false (gdb) bt #0 0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437 #1 0x5576d0f7f3ab in bdrv_driver_pwritev at ../block/io.c:1174 #2 0x5576d0f8139d in bdrv_aligned_pwritev at ../block/io.c:1988 #3 0x5576d0f81b65 in bdrv_co_pwritev_part at ../block/io.c:2156 #4 0x5576d0f8e6b7 in blk_do_pwritev_part at ../block/block-backend.c:1260 #5 0x5576d0f8e84d in blk_aio_write_entry at ../block/block-backend.c:1476 #6 0x5576d1060ddb in coroutine_trampoline at ../util/coroutine-ucontext.c:173 #7 0x7f7c8d3be0d0 in __start_context at /lib/../lib64/libc.so.6 #8 0x7f7b52beb1e0 in () #9 0x in () Switch to qemu main thread: #0 0x7f903be704ed in __lll_lock_wait at /lib/../lib64/libpthread.so.0 #1 0x7f903be6bde6 in _L_lock_941 at /lib/../lib64/libpthread.so.0 #2 0x7f903be6bcdf in pthread_mutex_lock at /lib/../lib64/libpthread.so.0 #3 0x564b21456889 in qemu_mutex_lock_impl at ../util/qemu-thread-posix.c:79 #4 0x564b213af8a5 in block_job_add_bdrv at ../blockjob.c:224 #5 0x564b213b00ad in block_job_create at ../blockjob.c:440 #6 0x564b21357c0a in mirror_start_job at ../block/mirror.c:1622 #7 0x564b2135a9af in commit_active_start at ../block/mirror.c:1867 #8 0x564b2133d132 in qmp_block_commit at ../blockdev.c:2768 #9 0x564b2141fef3 in qmp_marshal_block_commit at qapi/qapi-commands-block-core.c:346 #10 0x564b214503c9 in do_qmp_dispatch_bh at ../qapi/qmp-dispatch.c:110 #11 0x564b21451996 in aio_bh_poll at ../util/async.c:164 #12 0x564b2146018e in aio_dispatch at ../util/aio-posix.c:381 #13 0x564b2145187e in aio_ctx_dispatch at ../util/async.c:306 #14 0x7f9040239049 in g_main_context_dispatch at /lib/../lib64/libglib-2.0.so.0 #15 0x564b21447368 in main_loop_wait at ../util/main-loop.c:232 #16 0x564b21447368 in main_loop_wait at ../util/main-loop.c:255 #17 0x564b21447368 in main_loop_wait at ../util/main-loop.c:531 #18 0x564b212304e1 in qemu_main_loop at ../softmmu/runstate.c:721 #19 0x564b20f7975e in main at ../softmmu/main.c:50 Thanks, Michael -Original Message- From: Vladimir Sementsov-Ogievskiy Sent: 2021年2月1日 18:28 To: 08005...@163.com; kw...@redhat.com; mre...@redhat.com; js...@redhat.com Cc: 仇大玉 ; qemu-de...@nongnu.org; qemu-block@nongnu.org Subject: Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot Hi! Tanks for fixing and sorry for a delay! Please send each new version of a patch as a separate branch. It's a rule from https://wiki.qemu.org/Contribute/SubmitAPatch and it is more readable and less probable that your patch will be missed. 28.01.2021 04:30, 08005...@163.com wrote: > From: Michael Qiu > > v4: rebase to latest code > > v3: reformat the commit log, remove duplicate content > > v2: modify the coredump backtrace within commit log with the newest > qemu with master branch Such things shouldn't be in a commit message. You may put such comments after --- line[*] in a generated patch email > > Currently, if guest has workloads, IO thread will acquire aio_context > lock before do io_submit, it leads to segmentfault when do block > commit after snapshot. Just like below: Do you have some reproducer script? > > Program received signal SIGSEGV, Segmentation fault. > [Switching to Thread 0x7f7c7d91f700 (LWP 99907)] 0x5576d0f65aab in > bdrv_mirror_top_pwritev at ../block/mirror.c:1437 > 1437
Re: [PATCH 07/10] target/arm: Move V7M feature to target Kconfig
Philippe Mathieu-Daudé writes: > V7M is an architecture feature, move its declaration to target/arm/. > > Signed-off-by: Philippe Mathieu-Daudé modulo previous comments: Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH 05/10] meson: Introduce target-specific Kconfig
Philippe Mathieu-Daudé writes:
> Add a target-specific Kconfig.
>
> Target foo now has CONFIG_FOO defined.
>
> Two architecture have a particularity, ARM and MIPS:
> their 64-bit version include the 32-bit subset.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> I suppose X86_64 should also select I386?
> No clue about PPC/RISCV.
> ---
> meson.build | 3 ++-
> Kconfig | 1 +
> target/Kconfig| 23 +++
> target/alpha/Kconfig | 2 ++
Repeating myself through the magic of copy and paste:
In docs/devel/kconfig.rst we make the distinction between:
**subsystems**, of which **buses** are a special case
**devices**
**device groups**
**boards**
**internal elements**
I think we need to document the target/* Kconfigs in kconfig.rst at the
same time as adding all of these.
> target/arm/Kconfig| 6 ++
> target/avr/Kconfig| 2 ++
> target/cris/Kconfig | 2 ++
> target/hppa/Kconfig | 2 ++
> target/i386/Kconfig | 5 +
> target/lm32/Kconfig | 2 ++
> target/m68k/Kconfig | 2 ++
> target/microblaze/Kconfig | 2 ++
> target/mips/Kconfig | 6 ++
> target/moxie/Kconfig | 2 ++
> target/nios2/Kconfig | 2 ++
> target/openrisc/Kconfig | 2 ++
> target/ppc/Kconfig| 5 +
> target/riscv/Kconfig | 5 +
> target/rx/Kconfig | 2 ++
> target/s390x/Kconfig | 2 ++
> target/sh4/Kconfig| 2 ++
> target/sparc/Kconfig | 5 +
> target/tilegx/Kconfig | 2 ++
> target/tricore/Kconfig| 2 ++
> target/unicore32/Kconfig | 2 ++
> target/xtensa/Kconfig | 2 ++
> 26 files changed, 92 insertions(+), 1 deletion(-)
> create mode 100644 target/Kconfig
> create mode 100644 target/alpha/Kconfig
> create mode 100644 target/arm/Kconfig
> create mode 100644 target/avr/Kconfig
> create mode 100644 target/cris/Kconfig
> create mode 100644 target/hppa/Kconfig
> create mode 100644 target/i386/Kconfig
> create mode 100644 target/lm32/Kconfig
> create mode 100644 target/m68k/Kconfig
> create mode 100644 target/microblaze/Kconfig
> create mode 100644 target/mips/Kconfig
> create mode 100644 target/moxie/Kconfig
> create mode 100644 target/nios2/Kconfig
> create mode 100644 target/openrisc/Kconfig
> create mode 100644 target/ppc/Kconfig
> create mode 100644 target/riscv/Kconfig
> create mode 100644 target/rx/Kconfig
> create mode 100644 target/s390x/Kconfig
> create mode 100644 target/sh4/Kconfig
> create mode 100644 target/sparc/Kconfig
> create mode 100644 target/tilegx/Kconfig
> create mode 100644 target/tricore/Kconfig
> create mode 100644 target/unicore32/Kconfig
> create mode 100644 target/xtensa/Kconfig
>
> diff --git a/meson.build b/meson.build
> index f00b7754fd4..a2dda0ce95e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1322,7 +1322,8 @@
>command: [minikconf,
> get_option('default_devices') ? '--defconfig' :
> '--allnoconfig',
> config_devices_mak, '@DEPFILE@', '@INPUT@',
> -host_kconfig, accel_kconfig])
> +host_kconfig, accel_kconfig,
> +'CONFIG_' + config_target['TARGET_ARCH'].to_upper() + '=y'])
>
> config_devices_data = configuration_data()
> config_devices = keyval.load(config_devices_mak)
> diff --git a/Kconfig b/Kconfig
> index bf694c42afe..c01e261e4e9 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -1,4 +1,5 @@
> source Kconfig.host
> source backends/Kconfig
> source accel/Kconfig
> +source target/Kconfig
> source hw/Kconfig
> diff --git a/target/Kconfig b/target/Kconfig
> new file mode 100644
> index 000..a6f719f223a
> --- /dev/null
> +++ b/target/Kconfig
> @@ -0,0 +1,23 @@
> +source alpha/Kconfig
> +source arm/Kconfig
> +source avr/Kconfig
> +source cris/Kconfig
> +source hppa/Kconfig
> +source i386/Kconfig
> +source lm32/Kconfig
> +source m68k/Kconfig
> +source microblaze/Kconfig
> +source mips/Kconfig
> +source moxie/Kconfig
> +source nios2/Kconfig
> +source openrisc/Kconfig
> +source ppc/Kconfig
> +source riscv/Kconfig
> +source rx/Kconfig
> +source s390x/Kconfig
> +source sh4/Kconfig
> +source sparc/Kconfig
> +source tilegx/Kconfig
> +source tricore/Kconfig
> +source unicore32/Kconfig
> +source xtensa/Kconfig
> diff --git a/target/alpha/Kconfig b/target/alpha/Kconfig
> new file mode 100644
> index 000..267222c05b8
> --- /dev/null
> +++ b/target/alpha/Kconfig
> @@ -0,0 +1,2 @@
> +config ALPHA
> +bool
> diff --git a/target/arm/Kconfig b/target/arm/Kconfig
> new file mode 100644
> index 000..3f3394a22b2
> --- /dev/null
> +++ b/target/arm/Kconfig
> @@ -0,0 +1,6 @@
> +config ARM
> +bool
> +
> +config AARCH64
> +bool
> +select ARM
> diff --git a/target/avr/Kconfig b/target/avr/Kconfig
> new file mode 100644
> index 000..155592d3537
> --- /dev/null
> +++ b/target/avr/Kconfig
> @@ -0,0 +1,2 @@
> +config AVR
> +bool
Re: [PATCH 06/10] target/i386: Move SEV feature to target Kconfig
Philippe Mathieu-Daudé writes: > SEV is an architecture feature, move its declaration to target/i386/. In docs/devel/kconfig.rst we make the distinction between: **subsystems**, of which **buses** are a special case **devices** **device groups** **boards** **internal elements** Are we treating architecture features as internal elements or should we add some additional words to the kconfig document before we starting to move stuff there. In fact I realise this is better directed at 5/10 so for this patch: Reviewed-by: Alex Bennée > > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/i386/Kconfig | 4 > target/i386/Kconfig | 4 > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig > index 7f91f30877f..3d67c172dab 100644 > --- a/hw/i386/Kconfig > +++ b/hw/i386/Kconfig > @@ -1,7 +1,3 @@ > -config SEV > -bool > -depends on KVM > - > config PC > bool > imply APPLESMC > diff --git a/target/i386/Kconfig b/target/i386/Kconfig > index ce6968906ee..27c76c554c7 100644 > --- a/target/i386/Kconfig > +++ b/target/i386/Kconfig > @@ -3,3 +3,7 @@ config I386 > > config X86_64 > bool > + > +config SEV > +bool > +depends on KVM && I386 -- Alex Bennée
Re: [PATCH 04/10] hw/lm32/Kconfig: Have MILKYMIST select LM32_PERIPHERALS
Philippe Mathieu-Daudé writes: > The Milkymist board requires more than the PTIMER. Directly > select the LM32_PERIPHERALS. This fixes: > > /usr/bin/ld: > libqemu-lm32-softmmu.fa.p/target_lm32_gdbstub.c.o: in function > `lm32_cpu_gdb_read_register': > target/lm32/gdbstub.c:46: undefined reference to `lm32_pic_get_im' > target/lm32/gdbstub.c:48: undefined reference to `lm32_pic_get_ip' > libqemu-lm32-softmmu.fa.p/target_lm32_op_helper.c.o: in function > `helper_wcsr_im': > target/lm32/op_helper.c:107: undefined reference to `lm32_pic_set_im' > libqemu-lm32-softmmu.fa.p/target_lm32_op_helper.c.o: in function > `helper_wcsr_ip': > target/lm32/op_helper.c:114: undefined reference to `lm32_pic_set_ip' > libqemu-lm32-softmmu.fa.p/target_lm32_op_helper.c.o: in function > `helper_wcsr_jtx': > target/lm32/op_helper.c:120: undefined reference to `lm32_juart_set_jtx' > libqemu-lm32-softmmu.fa.p/target_lm32_op_helper.c.o: in function > `helper_wcsr_jrx': > target/lm32/op_helper.c:125: undefined reference to `lm32_juart_set_jrx' > libqemu-lm32-softmmu.fa.p/target_lm32_translate.c.o: in function > `lm32_cpu_dump_state': > target/lm32/translate.c:1161: undefined reference to `lm32_pic_get_ip' > target/lm32/translate.c:1161: undefined reference to `lm32_pic_get_im' > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH] MAINTAINERS: suggest myself as co-maintainer for Block Jobs
28.01.2021 18:28, John Snow wrote: On 1/28/21 10:09 AM, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: I'm developing Qemu backup for several years, and finally new backup architecture, including block-copy generic engine and backup-top filter landed upstream, great thanks to reviewers and especially to Max Reitz! I also have plans of moving other block-jobs onto block-copy, so that we finally have one generic block copying path, fast and well-formed. So, now I suggest to bring all parts of backup architecture into "Block Jobs" subsystem (actually, aio_task is shared with qcow2 and qemu-co-shared-resource can be reused somewhere else, but I'd keep an eye on them in context of block-jobs) and add myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy With pleasure: Reviewed-by: Markus Armbruster Absolutely! Glad to see it. Reviewed-by: John Snow [..] Great! Reviewed-by: Max Reitz Thanks! Could someone pull it? I don't have any signed PGP key for now, to send pull requests :\ Interesting, could I get one while sitting in Moscow? -- Best regards, Vladimir
Re: [PATCH 03/10] hw/sh4/Kconfig: Rename CONFIG_LM32 -> CONFIG_LM32_PERIPHERALS
Philippe Mathieu-Daudé writes: > We want to be able to use the 'LM32' config for architecture > specific features. As CONFIG_LM32 is only used to select > peripherals, rename it CONFIG_LM32_PERIPHERALS. > > Signed-off-by: Philippe Mathieu-Daudé _DEVICES if you want, either way: Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH v4] blockjob: Fix crash with IOthread when block commit after snapshot
Hi!
Tanks for fixing and sorry for a delay!
Please send each new version of a patch as a separate branch. It's a rule from
https://wiki.qemu.org/Contribute/SubmitAPatch and it is more readable and less
probable that your patch will be missed.
28.01.2021 04:30, 08005...@163.com wrote:
From: Michael Qiu
v4: rebase to latest code
v3: reformat the commit log, remove duplicate content
v2: modify the coredump backtrace within commit log with the newest
qemu with master branch
Such things shouldn't be in a commit message. You may put such comments after
--- line[*] in a generated patch email
Currently, if guest has workloads, IO thread will acquire aio_context
lock before do io_submit, it leads to segmentfault when do block commit
after snapshot. Just like below:
Do you have some reproducer script?
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f7c7d91f700 (LWP 99907)]
0x5576d0f65aab in bdrv_mirror_top_pwritev at ../block/mirror.c:1437
1437../block/mirror.c: No such file or directory.
(gdb) p s->job
$17 = (MirrorBlockJob *) 0x0
(gdb) p s->stop
$18 = false
(gdb) bt
Switch to qemu main thread:
/lib/../lib64/libpthread.so.0
/lib/../lib64/libpthread.so.0
../util/qemu-thread-posix.c:79
qapi/qapi-commands-block-core.c:346
../qapi/qmp-dispatch.c:110
/lib/../lib64/libglib-2.0.so.0
Not very informative bt..
In IO thread when do bdrv_mirror_top_pwritev, the job is NULL, and stop field
is false, this means the MirrorBDSOpaque "s" object has not been initialized
yet,
and this object is initialized by block_job_create(), but the initialize
process is stuck in acquiring the lock.
Could you show another thread bt?
Hm, so you argue that we come to bdrv_mirror_top_pwritev() (which means that
mirror-top node is already inserted into block graph), but its bs->opaque is
not initialized?
Hmm, really in mirror_start_job we do insert mirror_top_bs before
block_job_create() call.
But we should do that all in a drained section, so that no parallel io requests
may come.
And we have a drained section but it finishes immediately after bdrv_append,
when
bs_opaque is still not initialized. Probably we just need to expand it?
May be:
diff --git a/block/mirror.c b/block/mirror.c
index 8e1ad6eceb..0a6bfc1230 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1610,11 +1610,11 @@ static BlockJob *mirror_start_job(
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
bdrv_append(mirror_top_bs, bs, &local_err);
-bdrv_drained_end(bs);
if (local_err) {
bdrv_unref(mirror_top_bs);
error_propagate(errp, local_err);
+bdrv_drained_end(bs);
return NULL;
}
@@ -1789,6 +1789,8 @@ static BlockJob *mirror_start_job(
trace_mirror_start(bs, s, opaque);
job_start(&s->common.job);
+bdrv_drained_end(bs);
+
return &s->common;
fail:
@@ -1813,6 +1815,8 @@ fail:
bdrv_unref(mirror_top_bs);
+bdrv_drained_end(bs);
+
return NULL;
}
Could you check, does it help?
The rootcause is that qemu do release/acquire when hold the lock,
at the same time, IO thread get the lock after release stage, and the crash
occured.
Actually, in this situation, job->job.aio_context will not equal to
qemu_get_aio_context(), and will be the same as bs->aio_context,
thus, no need to release the lock, becasue bdrv_root_attach_child()
will not change the context.
This patch fix this issue.
Signed-off-by: Michael Qiu
---
[*] here you could add any comments, which will not go into final commit
message, like version history.
blockjob.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/blockjob.c b/blockjob.c
index 98ac8af982..51a09f3b60 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -214,13 +214,15 @@ int block_job_add_bdrv(BlockJob *job, const char *name,
BlockDriverState *bs,
BdrvChild *c;
bdrv_ref(bs);
-if (job->job.aio_context != qemu_get_aio_context()) {
+if (bdrv_get_aio_context(bs) != job->job.aio_context &&
+job->job.aio_context != qemu_get_aio_context()) {
aio_context_release(job->job.aio_context);
}
c = bdrv_root_attach_child(bs, name, &child_job, 0,
job->job.aio_context, perm, shared_perm, job,
errp);
-if (job->job.aio_context != qemu_get_aio_context()) {
+if (bdrv_get_aio_context(bs) != job->job.aio_context &&
+job->job.aio_context != qemu_get_aio_context()) {
that's a wrong check, it will never reacquire the lock on success path, as
after successful attach, bs context would definitely equal to job context.
I think you need a boolean variable at start of function, initialized to the
condition, and after _attach_child() you not recheck the condition but rely on
variable.
aio_context_acquire(job->job.aio_context);
}
if (c == NULL) {
The code was introduced by Kevin in 132ada80c4a6 "block: Ad
Re: [PATCH 02/10] hw/lm32/Kconfig: Introduce CONFIG_LM32_EVR for lm32-evr/uclinux boards
Philippe Mathieu-Daudé writes: > We want to be able to use the 'LM32' config for architecture > specific features. Introduce CONFIG_LM32_EVR to select the > lm32-evr / lm32-uclinux boards. > > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Alex Bennée -- Alex Bennée
Re: [PATCH 01/10] hw/sh4/Kconfig: Rename CONFIG_SH4 -> CONFIG_SH4_PERIPHERALS
Philippe Mathieu-Daudé writes: > We want to be able to use the 'SH4' config for architecture > specific features. As CONFIG_SH4 is only used to select > peripherals, rename it CONFIG_SH4_PERIPHERALS. > > Signed-off-by: Philippe Mathieu-Daudé I agree with Balaton Zoltan that _DEVICES might be a bit shorter. Either way: Reviewed-by: Alex Bennée -- Alex Bennée
[PATCH] iotests: check: return 1 on failure
We should indicate failure by exit code, not only output.
Reported-by: Peter Maydell
Fixes: f203080bbd9f9e5b31041b1f2afcd6040c5aaec5
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/check | 5 -
tests/qemu-iotests/testrunner.py | 4 +++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 5190dee82e..d1c87ceaf1 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -140,4 +140,7 @@ if __name__ == '__main__':
else:
with TestRunner(env, makecheck=args.makecheck,
color=args.color) as tr:
-tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])
+paths = [os.path.join(env.source_iotests, t) for t in tests]
+ok = tr.run_tests(paths)
+if not ok:
+sys.exit(1)
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 24b3fba115..25754e9a09 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -318,7 +318,7 @@ class TestRunner(ContextManager['TestRunner']):
return res
-def run_tests(self, tests: List[str]) -> None:
+def run_tests(self, tests: List[str]) -> bool:
n_run = 0
failed = []
notrun = []
@@ -363,5 +363,7 @@ class TestRunner(ContextManager['TestRunner']):
if failed:
print('Failures:', ' '.join(failed))
print(f'Failed {len(failed)} of {n_run} iotests')
+return False
else:
print(f'Passed all {n_run} iotests')
+return True
--
2.29.2
Re: [PULL 0/2] block: Fix iotests to respect configured Python binary
29.01.2021 20:17, Kevin Wolf wrote: Am 29.01.2021 um 17:13 hat Peter Maydell geschrieben: On Fri, 29 Jan 2021 at 14:52, Kevin Wolf wrote: The following changes since commit 5101d00d2f1138a73344dc4833587f76d7a5fa5c: Merge remote-tracking branch 'remotes/vivier2/tags/trivial-branch-for-6.0-pull-request' into staging (2021-01-29 10:10:43 +) are available in the Git repository at: git://repo.or.cz/qemu/kevin.git tags/for-upstream for you to fetch changes up to 4cea90be62f4f15a63e1a8f7d5d0958f79fdf290: tests/Makefile.include: export PYTHON for check-block.sh (2021-01-29 12:32:36 +0100) Block layer patches: - Fix iotests to respect configured Python binary This is definitely better so I'm going to apply it, but it seems to reveal a pile of iotest failures on FreeBSD: [...] Failures: 030 040 041 127 256 Hm... I did run 'make check-block' on a FreeBSD VM before sending the pull request, but it turns out I ran it on the wrong branch. *sigh* After rerunning it with the right one (and manually so that I get all tests, not just those in 'make check-block'), however, while I'm seeing some failures, my list of failing cases and your list are completely disjoint: Failures: 115 125 145 182 286 298 300 migrate-bitmaps-postcopy-test migrate-bitmaps-test All of these are test cases that are not run during 'make check-block', so I would assume that they were already broken before. So there seems to be something more specific to the environment that made your test cases fail than just FreeBSD. though the entire build goes on to succeed (which is probably a test framework bug somewhere...) This is not good, and it can easily be reproduced on Linux by simply changing the reference output of a test so that it fails. 'check' doesn't seem to return the right exit code any more. That's bad. I'll make a patch. -- Best regards, Vladimir
Re: [PATCH 1/7] qemu/queue: add some useful QLIST_ and QTAILQ_ macros
01.02.2021 11:29, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy writes:
Add QLIST_FOREACH_FUNC_SAFE(), QTAILQ_FOREACH_FUNC_SAFE() and
QTAILQ_POP_HEAD(), to be used in following commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
include/qemu/queue.h | 14 ++
1 file changed, 14 insertions(+)
diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index e029e7bf66..03e1fce85f 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -173,6 +173,13 @@ struct {
\
(var) && ((next_var) = ((var)->field.le_next), 1); \
(var) = (next_var))
+#define QLIST_FOREACH_FUNC_SAFE(head, field, func) do { \
+typeof(*QLIST_FIRST(head)) *qffs_var, *qffs_next_var; \
+QLIST_FOREACH_SAFE(qffs_var, (head), field, qffs_next_var) {\
+(func)(qffs_var); \
+} \
+} while (/*CONSTCOND*/0)
+
/*
* List access methods.
*/
@@ -490,6 +497,13 @@ union {
\
(var) && ((prev_var) = QTAILQ_PREV(var, field), 1);\
(var) = (prev_var))
+#define QTAILQ_FOREACH_FUNC_SAFE(head, field, func) do {\
+typeof(*QTAILQ_FIRST(head)) *qffs_var, *qffs_next_var; \
+QTAILQ_FOREACH_SAFE(qffs_var, (head), field, qffs_next_var) { \
+(func)(qffs_var); \
+} \
+} while (/*CONSTCOND*/0)
+
/*
* Tail queue access methods.
*/
I wonder whether these are worth having. Can you show the difference
they make in the next patch?
Not big difference, so I can easily drop them. But I think it's a good idea and
can be reused.. Why don't you like it?
--
Best regards,
Vladimir
Re: [PATCH 1/7] qemu/queue: add some useful QLIST_ and QTAILQ_ macros
Vladimir Sementsov-Ogievskiy writes:
> Add QLIST_FOREACH_FUNC_SAFE(), QTAILQ_FOREACH_FUNC_SAFE() and
> QTAILQ_POP_HEAD(), to be used in following commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> include/qemu/queue.h | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/qemu/queue.h b/include/qemu/queue.h
> index e029e7bf66..03e1fce85f 100644
> --- a/include/qemu/queue.h
> +++ b/include/qemu/queue.h
> @@ -173,6 +173,13 @@ struct {
>\
> (var) && ((next_var) = ((var)->field.le_next), 1); \
> (var) = (next_var))
>
> +#define QLIST_FOREACH_FUNC_SAFE(head, field, func) do { \
> +typeof(*QLIST_FIRST(head)) *qffs_var, *qffs_next_var; \
> +QLIST_FOREACH_SAFE(qffs_var, (head), field, qffs_next_var) {\
> +(func)(qffs_var); \
> +} \
> +} while (/*CONSTCOND*/0)
> +
> /*
> * List access methods.
> */
> @@ -490,6 +497,13 @@ union {
>\
> (var) && ((prev_var) = QTAILQ_PREV(var, field), 1);\
> (var) = (prev_var))
>
> +#define QTAILQ_FOREACH_FUNC_SAFE(head, field, func) do {\
> +typeof(*QTAILQ_FIRST(head)) *qffs_var, *qffs_next_var; \
> +QTAILQ_FOREACH_SAFE(qffs_var, (head), field, qffs_next_var) { \
> +(func)(qffs_var); \
> +} \
> +} while (/*CONSTCOND*/0)
> +
> /*
> * Tail queue access methods.
> */
I wonder whether these are worth having. Can you show the difference
they make in the next patch?
Re: [PATCH 0/7] qcow2: compressed write cache
29.01.2021 20:30, no-re...@patchew.org wrote: Patchew URL: https://patchew.org/QEMU/20210129165030.640169-1-vsement...@virtuozzo.com/ Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20210129165030.640169-1-vsement...@virtuozzo.com Subject: [PATCH 0/7] qcow2: compressed write cache === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu 5101d00..3701c07 master -> master - [tag update] patchew/20210129110012.8660-1-peter.mayd...@linaro.org -> patchew/20210129110012.8660-1-peter.mayd...@linaro.org * [new tag] patchew/20210129165030.640169-1-vsement...@virtuozzo.com -> patchew/20210129165030.640169-1-vsement...@virtuozzo.com Switched to a new branch 'test' d7783a4 simplebench/bench-backup: add target-cache argument ddf4442 simplebench/bench-backup: add --compressed option 47ee627 simplebench: bench_one(): support count=1 2b80e33 simplebench: bench_one(): add slow_limit argument acf2fb6 block/qcow2: use compressed write cache d96e35f block/qcow2: introduce cache for compressed writes 0d009e1 qemu/queue: add some useful QLIST_ and QTAILQ_ macros === OUTPUT BEGIN === 1/7 Checking commit 0d009e16280d (qemu/queue: add some useful QLIST_ and QTAILQ_ macros) ERROR: spaces required around that '*' (ctx:WxV) #25: FILE: include/qemu/queue.h:177: +typeof(*QLIST_FIRST(head)) *qffs_var, *qffs_next_var; \ ^ WARNING: Block comments use a leading /* on a separate line #29: FILE: include/qemu/queue.h:181: +} while (/*CONSTCOND*/0) ERROR: spaces required around that '*' (ctx:WxV) #39: FILE: include/qemu/queue.h:501: +typeof(*QTAILQ_FIRST(head)) *qffs_var, *qffs_next_var; \ ^ WARNING: Block comments use a leading /* on a separate line #43: FILE: include/qemu/queue.h:505: +} while (/*CONSTCOND*/0) all false positive -- Best regards, Vladimir
