Re: [PULL 0/8] s390x and misc fixes
On 15/03/2022 20.30, Peter Maydell wrote: On Tue, 15 Mar 2022 at 18:58, Peter Maydell wrote: On Tue, 15 Mar 2022 at 11:20, Thomas Huth wrote: Hi Peter! The following changes since commit 352998df1c53b366413690d95b35f76d0721ebed: Merge tag 'i2c-20220314' of https://github.com/philmd/qemu into staging (2022-03-14 14:39:33 +) are available in the Git repository at: https://gitlab.com/thuth/qemu.git tags/pull-request-2022-03-15 for you to fetch changes up to 36149534792dcf07a3c59867f967eaee23ab906c: meson: Update to version 0.61.3 (2022-03-15 10:32:36 +0100) * Fixes for s390x branch instruction emulation * Fixes for the tests/avocado/boot_linux.py:BootLinuxS390X test * Fix for "-cpu help" output * Bump meson to 0.61.3 to fix stderr log of the iotests This results in every "Linking" step on my macos box producing the warning: ld: warning: directory not found for option '-Lns/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/12.0.0' Obvious suspect here is the new meson version. Also, after rolling this merge attempt back, older meson barfs on whatever the new one left behind: [0/1] Regenerating build files. Traceback (most recent call last): File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/mesonmain.py", line 228, in run return options.run_func(options) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/msetup.py", line 281, in run app.generate() File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/msetup.py", line 177, in generate env = environment.Environment(self.source_dir, self.build_dir, self.options) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/environment.py", line 462, in __init__ self.coredata = coredata.load(self.get_build_dir()) # type: coredata.CoreData File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/coredata.py", line 1003, in load obj = pickle.load(f) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/mesonlib/universal.py", line 2076, in __setstate__ self.__init__(**state) # type: ignore TypeError: __init__() got an unexpected keyword argument 'module' FAILED: build.ninja /usr/local/opt/python@3.9/bin/python3.9 /Users/pm215/src/qemu-for-merges/meson/meson.py --internal regenerate /Users/pm215/src/qemu-for-merges /Users/pm215/src/qemu-for-merges/build/all --backend ninja ninja: error: rebuilding 'build.ninja': subcommand failed /usr/local/bin/ninja build.ninja && touch build.ninja.stamp GIT ui/keycodemapdb meson tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc capstone slirp [0/1] Regenerating build files. Traceback (most recent call last): File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/mesonmain.py", line 228, in run return options.run_func(options) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/msetup.py", line 281, in run app.generate() File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/msetup.py", line 177, in generate env = environment.Environment(self.source_dir, self.build_dir, self.options) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/environment.py", line 462, in __init__ self.coredata = coredata.load(self.get_build_dir()) # type: coredata.CoreData File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/coredata.py", line 1003, in load obj = pickle.load(f) File "/Users/pm215/src/qemu-for-merges/meson/mesonbuild/mesonlib/universal.py", line 2076, in __setstate__ self.__init__(**state) # type: ignore TypeError: __init__() got an unexpected keyword argument 'module' FAILED: build.ninja meson ought to be smart enough to spot that it's got data from an incompatible version and just discard its cache rather than choking on it. Ok, I'll respin without the meson update. Question is: Do we now want to revert the TAPification of the iotests for 7.0? I guess so, otherwise most people won't see the output of failed tests when doing "make check-block" ... Thomas
Re: [PATCH v2 04/10] block.c: bdrv_replace_child_noperm: first remove the child, and then call ->detach()
Unfortunately this patch is not safe: theoretically ->detach can call bdrv_unapply_subtree_drain, and if it polls, will can call a bh that for example reads the graph, finding it in an inconsistent state, since it is between the two writes QLIST_REMOVE(child, next_parent); and QLIST_REMOVE(child, next); Please ignore it. This patch could eventually go in the subtree_drain serie, if we decide to go in that direction. Emanuele Am 14/03/2022 um 14:18 schrieb Emanuele Giuseppe Esposito: > Doing the opposite can make ->detach() (more precisely > bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain > just performed to protect the removal of the child from the graph, > thus making the fully-enabled assert_bdrv_graph_writable fail. > > Note that assert_bdrv_graph_writable is not yet fully enabled. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index 372f16f4a0..d870ba5393 100644 > --- a/block.c > +++ b/block.c > @@ -1448,6 +1448,11 @@ static void bdrv_child_cb_attach(BdrvChild *child) > bdrv_apply_subtree_drain(child, bs); > } > > +/* > + * Unapply the drain in the whole child subtree, as > + * it has been already detached, and then remove from > + * child->opaque->children. > + */ > static void bdrv_child_cb_detach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > @@ -2864,14 +2869,18 @@ static void bdrv_replace_child_noperm(BdrvChild > **childp, > } > > if (old_bs) { > -/* Detach first so that the recursive drain sections coming from > @child > - * are already gone and we only end the drain sections that came from > - * elsewhere. */ > +/* First remove child from child->bs->parents list */ > +assert_bdrv_graph_writable(old_bs); > +QLIST_REMOVE(child, next_parent); > +/* > + * Then call ->detach() cb. > + * In child_of_bds case, update the child parent > + * (child->opaque) ->children list and > + * remove any drain left in the child subtree. > + */ > if (child->klass->detach) { > child->klass->detach(child); > } > -assert_bdrv_graph_writable(old_bs); > -QLIST_REMOVE(child, next_parent); > } > > child->bs = new_bs; >
Re: [PATCH v2 05/10] block.c: bdrv_replace_child_noperm: first call ->attach(), and then add child
Unfortunately this patch is not safe: theoretically ->attach can call bdrv_apply_subtree_drain, and if it polls, will can call a bh that for example reads the graph, finding it in an inconsistent state, since it is between the two writes QLIST_INSERT_HEAD(&bs->children, child, next); and QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); Please ignore it. This patch could eventually go in the subtree_drain serie, if we decide to go in that direction. Emanuele Am 14/03/2022 um 14:18 schrieb Emanuele Giuseppe Esposito: > Doing the opposite can make adding the child node to a non-drained node, > as apply_subtree_drain is only done in ->attach() and thus make > assert_bdrv_graph_writable fail. > > This can happen for example during a transaction rollback (test 245, > test_io_with_graph_changes): > 1. a node is removed from the graph, thus it is undrained > 2. then something happens, and we need to roll back the transactions >through tran_abort() > 3. at this point, the current code would first attach the undrained node >to the graph via QLIST_INSERT_HEAD, and then call ->attach() that >will take care of restoring the drain with apply_subtree_drain(), >leaving the node undrained between the two operations. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > block.c | 20 +++- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index d870ba5393..c6a550f9c6 100644 > --- a/block.c > +++ b/block.c > @@ -1434,6 +1434,11 @@ static void bdrv_inherited_options(BdrvChildRole role, > bool parent_is_format, > *child_flags = flags; > } > > +/* > + * Add the child node to child->opaque->children list, > + * and then apply the drain to the whole child subtree, > + * so that the drain count matches with the parent. > + */ > static void bdrv_child_cb_attach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > @@ -2889,8 +2894,6 @@ static void bdrv_replace_child_noperm(BdrvChild > **childp, > } > > if (new_bs) { > -assert_bdrv_graph_writable(new_bs); > -QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > > /* > * Detaching the old node may have led to the new node's > @@ -2901,12 +2904,19 @@ static void bdrv_replace_child_noperm(BdrvChild > **childp, > assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); > drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter; > > -/* Attach only after starting new drained sections, so that recursive > - * drain sections coming from @child don't get an extra > .drained_begin > - * callback. */ > +/* > + * First call ->attach() cb. > + * In child_of_bds case, add child to the parent > + * (child->opaque) ->children list and if > + * necessary add missing drains in the child subtree. > + */ > if (child->klass->attach) { > child->klass->attach(child); > } > + > +/* Then add child to new_bs->parents list */ > +assert_bdrv_graph_writable(new_bs); > +QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > } > > /* >
[PATCH v2] MAINTAINERS: change Vladimir's email address
Old vsement...@virtuozzo.com is not accessible anymore. Signed-off-by: Vladimir Sementsov-Ogievskiy --- v2: @ya.ru mailbox works bad with mailing lists and git send-email command, @mail.ru works normally. Probably, I'll have to change the email again in the near future. May be not. But I think it worth to change it now to something that works. MAINTAINERS | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index f2e9ce1da2..c34b7562b5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2500,7 +2500,7 @@ F: scsi/* Block Jobs M: John Snow -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: blockjob.c @@ -2539,7 +2539,7 @@ T: git https://repo.or.cz/qemu/armbru.git block-next Dirty Bitmaps M: Eric Blake -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy R: John Snow L: qemu-block@nongnu.org S: Supported @@ -2762,13 +2762,13 @@ F: scripts/*.py F: tests/*.py Benchmark util -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy S: Maintained F: scripts/simplebench/ T: git https://src.openvz.org/scm/~vsementsov/qemu.git simplebench Transactions helper -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy S: Maintained F: include/qemu/transactions.h F: util/transactions.c @@ -3352,7 +3352,7 @@ F: block/iscsi-opts.c Network Block Device (NBD) M: Eric Blake -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Maintained F: block/nbd* @@ -3448,7 +3448,7 @@ F: block/dmg.c parallels M: Stefan Hajnoczi M: Denis V. Lunev -M: Vladimir Sementsov-Ogievskiy +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: block/parallels.c -- 2.35.1
[PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
From: Marc-André Lureau Replace a config-time define with a compile time condition define (compatible with clang and gcc) that must be declared prior to its usage. This avoids having a global configure time define, but also prevents from bad usage, if the config header wasn't included before. This can help to make some code independent from qemu too. gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. Signed-off-by: Marc-André Lureau --- meson.build | 1 - accel/tcg/atomic_template.h | 4 +- audio/audio.h | 2 +- hw/display/pl110_template.h | 6 +-- hw/net/can/ctucan_core.h| 2 +- hw/net/vmxnet3.h| 4 +- include/exec/cpu-all.h | 4 +- include/exec/cpu-common.h | 2 +- include/exec/memop.h| 2 +- include/exec/memory.h | 2 +- include/fpu/softfloat-types.h | 2 +- include/hw/core/cpu.h | 2 +- include/hw/i386/intel_iommu.h | 6 +-- include/hw/i386/x86-iommu.h | 4 +- include/hw/virtio/virtio-access.h | 6 +-- include/hw/virtio/virtio-gpu-bswap.h| 2 +- include/libdecnumber/dconfig.h | 2 +- include/net/eth.h | 2 +- include/qemu/bswap.h| 8 ++-- include/qemu/compiler.h | 2 + include/qemu/host-utils.h | 2 +- include/qemu/int128.h | 2 +- include/ui/qemu-pixman.h| 2 +- net/util.h | 2 +- target/arm/cpu.h| 8 ++-- target/arm/translate-a64.h | 2 +- target/arm/vec_internal.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 2 +- target/ppc/cpu.h| 2 +- target/s390x/tcg/vec.h | 2 +- target/xtensa/cpu.h | 2 +- tests/fp/platform.h | 4 +- accel/kvm/kvm-all.c | 4 +- audio/dbusaudio.c | 2 +- disas.c | 2 +- hw/core/loader.c| 4 +- hw/display/artist.c | 6 +-- hw/display/pxa2xx_lcd.c | 2 +- hw/display/vga.c| 12 +++--- hw/display/virtio-gpu-gl.c | 2 +- hw/s390x/event-facility.c | 2 +- hw/virtio/vhost.c | 2 +- linux-user/arm/nwfpe/double_cpdo.c | 4 +- linux-user/arm/nwfpe/fpa11_cpdt.c | 4 +- linux-user/ppc/signal.c | 3 +- linux-user/syscall.c| 6 +-- net/net.c | 4 +- target/alpha/translate.c| 2 +- target/arm/crypto_helper.c | 2 +- target/arm/helper.c | 2 +- target/arm/kvm64.c | 4 +- target/arm/neon_helper.c| 2 +- target/arm/sve_helper.c | 4 +- target/arm/translate-sve.c | 6 +-- target/arm/translate-vfp.c | 2 +- target/arm/translate.c | 2 +- target/hppa/translate.c | 2 +- target/i386/tcg/translate.c | 2 +- target/mips/tcg/lmmi_helper.c | 2 +- target/mips/tcg/msa_helper.c| 54 - target/ppc/arch_dump.c | 2 +- target/ppc/int_helper.c | 22 +- target/ppc/kvm.c| 4 +- target/ppc/mem_helper.c | 2 +- target/riscv/vector_helper.c| 2 +- target/s390x/tcg/translate.c| 2 +- target/sparc/vis_helper.c | 4 +- tcg/tcg-op.c| 4 +- tcg/tcg.c | 12 +++--- tests/qtest/vhost-user-blk-test.c | 2 +- tests/qtest/virtio-blk-test.c | 2 +- ui/vdagent.c| 2 +- ui/vnc.c| 2 +- util/bitmap.c | 2 +- util/host-utils.c | 2 +- target/ppc/translate/vmx-impl.c.inc | 4 +- target/ppc/translate/vsx-impl.c.inc | 2 +- target/riscv/insn_trans/trans_rvv.c.inc | 4 +- target/s390x/tcg/translate_vx.c.inc | 2 +- tcg/aarch64/tcg-target.c.inc| 4 +- tcg/arm/tcg-target.c.inc| 4 +- tcg/mips/tcg-target.c.inc | 2 +- tcg/ppc/tcg-target.c.inc| 10 ++--- tcg/riscv/tcg-target.c.inc | 4 +- 85 files changed, 173 insertions(+), 173 deletions(-) diff --git a/meson.build b/meson.build index f20712cb93d7..88df1bc42973 100644 --- a/meson.build +++ b/meson.build @@ -1591,7 +1591,6 @@ config_host_data.set('QEMU_VERSION_MICRO', meson.project_version().split('.')[2] config_host_data.set_quoted('CONFIG_HOST_DSOSUF
[PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
From: Marc-André Lureau One less qemu-specific macro. It also helps to make some headers/units only depend on glib, and thus moved in standalone projects eventually. Signed-off-by: Marc-André Lureau --- audio/audio.h | 4 +-- block/qcow2.h | 2 +- bsd-user/qemu.h | 2 +- hw/display/qxl.h| 2 +- hw/net/rocker/rocker.h | 2 +- hw/xen/xen_pt.h | 2 +- include/chardev/char-fe.h | 2 +- include/disas/dis-asm.h | 2 +- include/hw/acpi/aml-build.h | 12 +++ include/hw/core/cpu.h | 2 +- include/hw/hw.h | 2 +- include/hw/virtio/virtio.h | 2 +- include/hw/xen/xen-bus-helper.h | 4 +-- include/hw/xen/xen-bus.h| 4 +-- include/hw/xen/xen_common.h | 2 +- include/hw/xen/xen_pvdev.h | 2 +- include/monitor/monitor.h | 4 +-- include/qapi/error.h| 20 ++-- include/qapi/qmp/qjson.h| 8 ++--- include/qemu/buffer.h | 2 +- include/qemu/compiler.h | 11 ++- include/qemu/error-report.h | 24 +++--- include/qemu/log-for-trace.h| 2 +- include/qemu/log.h | 2 +- include/qemu/qemu-print.h | 8 ++--- include/qemu/readline.h | 2 +- qga/guest-agent-core.h | 2 +- qga/vss-win32/requester.h | 2 +- scripts/cocci-macro-file.h | 2 +- tests/qtest/libqos/libqtest.h | 42 - tests/qtest/libqtest-single.h | 2 +- tests/qtest/migration-helpers.h | 6 ++-- audio/alsaaudio.c | 4 +-- audio/dsoundaudio.c | 4 +-- audio/ossaudio.c| 4 +-- audio/paaudio.c | 2 +- audio/sdlaudio.c| 2 +- block/blkverify.c | 2 +- block/ssh.c | 4 +-- fsdev/9p-marshal.c | 2 +- fsdev/virtfs-proxy-helper.c | 2 +- hw/9pfs/9p.c| 2 +- hw/acpi/aml-build.c | 4 +-- hw/mips/fuloong2e.c | 2 +- hw/mips/malta.c | 2 +- hw/net/rtl8139.c| 2 +- hw/virtio/virtio.c | 2 +- io/channel-websock.c| 2 +- monitor/hmp.c | 4 +-- nbd/server.c| 10 +++--- qemu-img.c | 4 +-- qemu-io.c | 2 +- qobject/json-parser.c | 2 +- softmmu/qtest.c | 4 +-- tests/qtest/libqtest.c | 2 +- tests/unit/test-qobject-input-visitor.c | 4 +-- audio/coreaudio.m | 4 +-- scripts/checkpatch.pl | 2 +- 58 files changed, 130 insertions(+), 137 deletions(-) diff --git a/audio/audio.h b/audio/audio.h index c8bde536b5cd..cbb10f4816e5 100644 --- a/audio/audio.h +++ b/audio/audio.h @@ -91,8 +91,8 @@ typedef struct QEMUAudioTimeStamp { uint64_t old_ts; } QEMUAudioTimeStamp; -void AUD_vlog (const char *cap, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); -void AUD_log (const char *cap, const char *fmt, ...) GCC_FMT_ATTR(2, 3); +void AUD_vlog (const char *cap, const char *fmt, va_list ap) G_GNUC_PRINTF(2, 0); +void AUD_log (const char *cap, const char *fmt, ...) G_GNUC_PRINTF(2, 3); void AUD_register_card (const char *name, QEMUSoundCard *card); void AUD_remove_card (QEMUSoundCard *card); diff --git a/block/qcow2.h b/block/qcow2.h index fd48a89d452c..ba436a8d0d68 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -838,7 +838,7 @@ int qcow2_update_header(BlockDriverState *bs); void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, int64_t size, const char *message_format, ...) - GCC_FMT_ATTR(5, 6); + G_GNUC_PRINTF(5, 6); int qcow2_validate_table(BlockDriverState *bs, uint64_t offset, uint64_t entries, size_t entry_len, diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index af272c2a802a..21c06f2e7003 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -175,7 +175,7 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, abi_long arg1, abi_long do_openbsd_syscall(void *cpu_env, int num, abi_long arg1, abi_long arg2, abi_long arg3, abi_long arg4, abi_long arg5, abi_long arg6); -void gemu_log(const char *fmt, ...) GCC_FMT_ATTR(1, 2); +void gemu_log(const char *fmt, ...) G_GNUC_PRINTF(1, 2); extern __thread CPUState *thread_cpu; void cp
[PATCH 24/27] Remove trailing ; after G_DEFINE_AUTO macro
From: Marc-André Lureau The macro doesn't need it. Signed-off-by: Marc-André Lureau --- configure| 2 +- nbd/server.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/configure b/configure index 9e9b85147a5f..6d9cb23ac562 100755 --- a/configure +++ b/configure @@ -2248,7 +2248,7 @@ static void foo_free(Foo *f) { g_free(f); } -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Foo, foo_free); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Foo, foo_free) int main(void) { return 0; } EOF if ! compile_prog "$glib_cflags -Werror" "$glib_libs" ; then diff --git a/nbd/server.c b/nbd/server.c index 735381aacfcb..c5644fd3f6ad 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2064,7 +2064,7 @@ static void nbd_extent_array_free(NBDExtentArray *ea) g_free(ea->extents); g_free(ea); } -G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free) /* Further modifications of the array after conversion are abandoned */ static void nbd_extent_array_convert_to_be(NBDExtentArray *ea) -- 2.35.1.273.ge6ebfd0e8cbb
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On 16/03/2022 10.53, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Replace a config-time define with a compile time condition define (compatible with clang and gcc) that must be declared prior to its usage. This avoids having a global configure time define, but also prevents from bad usage, if the config header wasn't included before. This can help to make some code independent from qemu too. gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. Signed-off-by: Marc-André Lureau --- [...] @@ -188,7 +188,7 @@ CPU_CONVERT(le, 64, uint64_t) * a compile-time constant if you pass in a constant. So this can be * used to initialize static variables. */ -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN # define const_le32(_x) \ _x) & 0x00ffU) << 24) | \ (((_x) & 0xff00U) << 8) | \ @@ -211,7 +211,7 @@ typedef union { typedef union { float64 d; -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN struct { uint32_t upper; uint32_t lower; @@ -235,7 +235,7 @@ typedef union { typedef union { float128 q; -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN struct { uint32_t upmost; uint32_t upper; diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 0a5e67fb970e..7fdd88adb368 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -7,6 +7,8 @@ #ifndef COMPILER_H #define COMPILER_H +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) Why don't you do it this way instead: #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ #define HOST_WORDS_BIGENDIAN 1 #endif ... that way you could avoid the churn in all the other files? Thomas
[PATCH 08/27] compiler.h: replace QEMU_WARN_UNUSED_RESULT with G_GNUC_WARN_UNUSED_RESULT
From: Marc-André Lureau One less qemu-specific macro. It also helps to make some headers/units only depend on glib, and thus moved in standalone projects eventually. Signed-off-by: Marc-André Lureau Reviewed-by: Richard Henderson --- include/qemu-common.h | 2 +- include/qemu/compiler.h| 2 -- include/qemu/range.h | 4 ++-- scripts/cocci-macro-file.h | 2 +- block/qcow2-refcount.c | 20 +++- scripts/checkpatch.pl | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/qemu-common.h b/include/qemu-common.h index a049ac073874..a577ca92b1f1 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -27,7 +27,7 @@ int qemu_main(int argc, char **argv, char **envp); #endif ssize_t qemu_write_full(int fd, const void *buf, size_t count) -QEMU_WARN_UNUSED_RESULT; +G_GNUC_WARN_UNUSED_RESULT; #ifndef _WIN32 int qemu_pipe(int pipefd[2]); diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index f2bd050e3b9a..8385e477c18e 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -19,8 +19,6 @@ #define QEMU_NORETURN __attribute__ ((__noreturn__)) -#define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) - #define QEMU_SENTINEL __attribute__((sentinel)) #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) diff --git a/include/qemu/range.h b/include/qemu/range.h index f62b363e0d12..7e2b1cc447af 100644 --- a/include/qemu/range.h +++ b/include/qemu/range.h @@ -114,8 +114,8 @@ static inline uint64_t range_upb(Range *range) * @size may be 0. If the range would overflow, returns -ERANGE, otherwise * 0. */ -static inline int QEMU_WARN_UNUSED_RESULT range_init(Range *range, uint64_t lob, - uint64_t size) +G_GNUC_WARN_UNUSED_RESULT +static inline int range_init(Range *range, uint64_t lob, uint64_t size) { if (lob + size < lob) { return -ERANGE; diff --git a/scripts/cocci-macro-file.h b/scripts/cocci-macro-file.h index c2fcea8e77a2..9daec24d7825 100644 --- a/scripts/cocci-macro-file.h +++ b/scripts/cocci-macro-file.h @@ -20,7 +20,7 @@ /* From qemu/compiler.h */ #define QEMU_NORETURN __attribute__ ((__noreturn__)) -#define QEMU_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) +#define G_GNUC_WARN_UNUSED_RESULT __attribute__((warn_unused_result)) #define QEMU_SENTINEL __attribute__((sentinel)) #if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__)) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 94033972bedc..b91499410c0c 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -33,9 +33,11 @@ static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size, uint64_t max); -static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, -int64_t offset, int64_t length, uint64_t addend, -bool decrease, enum qcow2_discard_type type); + +G_GNUC_WARN_UNUSED_RESULT +static int update_refcount(BlockDriverState *bs, + int64_t offset, int64_t length, uint64_t addend, + bool decrease, enum qcow2_discard_type type); static uint64_t get_refcount_ro0(const void *refcount_array, uint64_t index); static uint64_t get_refcount_ro1(const void *refcount_array, uint64_t index); @@ -803,12 +805,12 @@ found: /* XXX: cache several refcount block clusters ? */ /* @addend is the absolute value of the addend; if @decrease is set, @addend * will be subtracted from the current refcount, otherwise it will be added */ -static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs, - int64_t offset, - int64_t length, - uint64_t addend, - bool decrease, - enum qcow2_discard_type type) +static int update_refcount(BlockDriverState *bs, + int64_t offset, + int64_t length, + uint64_t addend, + bool decrease, + enum qcow2_discard_type type) { BDRVQcow2State *s = bs->opaque; int64_t start, last, cluster_offset; diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a07f0effb540..797738a8e839 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -224,7 +224,7 @@ our $Attribute = qr{ const| volatile| QEMU_NORETURN| - QEMU_WARN_UNUSED_RESULT| + G_GNUC_WARN_UNUSED_RESULT| QEMU_SENTINEL| QEMU_PACKED| G_GNUC_PRINTF -- 2.35
[PATCH 22/27] error-report: replace error progname with glib functions
From: Marc-André Lureau Signed-off-by: Marc-André Lureau --- include/qemu/error-report.h | 2 -- qemu-io.c| 10 +- softmmu/vl.c | 2 +- storage-daemon/qemu-storage-daemon.c | 2 +- trace/control.c | 2 +- util/qemu-error.c| 24 +--- 6 files changed, 13 insertions(+), 29 deletions(-) diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h index 33e662db44c6..b6f45e69d79a 100644 --- a/include/qemu/error-report.h +++ b/include/qemu/error-report.h @@ -72,8 +72,6 @@ void error_init(const char *argv0); fmt, ##__VA_ARGS__); \ }) -const char *error_get_progname(void); - extern bool message_with_timestamp; extern bool error_with_guestname; extern const char *error_guest_name; diff --git a/qemu-io.c b/qemu-io.c index e45a15c41aac..eb8afc8b413b 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -323,7 +323,7 @@ static char *get_prompt(void) static char prompt[FILENAME_MAX + 2 /*"> "*/ + 1 /*"\0"*/ ]; if (!prompt[0]) { -snprintf(prompt, sizeof(prompt), "%s> ", error_get_progname()); +snprintf(prompt, sizeof(prompt), "%s> ", g_get_prgname()); } return prompt; @@ -598,10 +598,10 @@ int main(int argc, char **argv) break; case 'V': printf("%s version " QEMU_FULL_VERSION "\n" - QEMU_COPYRIGHT "\n", error_get_progname()); + QEMU_COPYRIGHT "\n", g_get_prgname()); exit(0); case 'h': -usage(error_get_progname()); +usage(g_get_prgname()); exit(0); case 'U': force_share = true; @@ -613,13 +613,13 @@ int main(int argc, char **argv) imageOpts = true; break; default: -usage(error_get_progname()); +usage(g_get_prgname()); exit(1); } } if ((argc - optind) > 1) { -usage(error_get_progname()); +usage(g_get_prgname()); exit(1); } diff --git a/softmmu/vl.c b/softmmu/vl.c index 0b81f6153548..1b4be044d2a9 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -836,7 +836,7 @@ static void help(int exitcode) version(); printf("usage: %s [options] [disk_image]\n\n" "'disk_image' is a raw hard disk image for IDE hard disk 0\n\n", -error_get_progname()); +g_get_prgname()); #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask)\ if ((arch_mask) & arch_type) \ diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index dd18b2cde8c4..eb724072579a 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -141,7 +141,7 @@ static void help(void) " --pidfilewrite process ID to a file after startup\n" "\n" QEMU_HELP_BOTTOM "\n", -error_get_progname()); +g_get_prgname()); } enum { diff --git a/trace/control.c b/trace/control.c index d5b68e846eeb..6c77cc631800 100644 --- a/trace/control.c +++ b/trace/control.c @@ -161,7 +161,7 @@ void trace_list_events(FILE *f) fprintf(f, "This list of names of trace points may be incomplete " "when using the DTrace/SystemTap backends.\n" "Run 'qemu-trace-stap list %s' to print the full list.\n", -error_get_progname()); +g_get_prgname()); #endif } diff --git a/util/qemu-error.c b/util/qemu-error.c index 52a9e013c490..7769aee8e791 100644 --- a/util/qemu-error.c +++ b/util/qemu-error.c @@ -146,22 +146,6 @@ void loc_set_file(const char *fname, int lno) } } -static const char *progname; - -/* - * Set the program name for error_print_loc(). - */ -static void error_set_progname(const char *argv0) -{ -const char *p = strrchr(argv0, '/'); -progname = p ? p + 1 : argv0; -} - -const char *error_get_progname(void) -{ -return progname; -} - /* * Print current location to current monitor if we have one, else to stderr. */ @@ -171,8 +155,8 @@ static void print_loc(void) int i; const char *const *argp; -if (!monitor_cur() && progname) { -fprintf(stderr, "%s:", progname); +if (!monitor_cur() && g_get_prgname()) { +fprintf(stderr, "%s:", g_get_prgname()); sep = " "; } switch (cur_loc->kind) { @@ -400,8 +384,10 @@ static void qemu_log_func(const gchar *log_domain, void error_init(const char *argv0) { +const char *p = strrchr(argv0, '/'); + /* Set the program name for error_print_loc(). */ -error_set_progname(argv0); +g_set_prgname(p ? p + 1 : argv0); /* * This sets up glib logging so libraries using it also print their logs -- 2.35.1.273.ge6ebfd0e8cbb
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On Wed, 16 Mar 2022 11:28:59 +0100 Thomas Huth wrote: > On 16/03/2022 10.53, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Replace a config-time define with a compile time condition > > define (compatible with clang and gcc) that must be declared prior to > > its usage. This avoids having a global configure time define, but also > > prevents from bad usage, if the config header wasn't included before. > > > > This can help to make some code independent from qemu too. > > > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. > > > > Signed-off-by: Marc-André Lureau > > --- > [...] > > @@ -188,7 +188,7 @@ CPU_CONVERT(le, 64, uint64_t) > >* a compile-time constant if you pass in a constant. So this can be > >* used to initialize static variables. > >*/ > > -#if defined(HOST_WORDS_BIGENDIAN) > > +#if HOST_BIG_ENDIAN > > # define const_le32(_x) \ > > _x) & 0x00ffU) << 24) | \ > >(((_x) & 0xff00U) << 8) | \ > > @@ -211,7 +211,7 @@ typedef union { > > > > typedef union { > > float64 d; > > -#if defined(HOST_WORDS_BIGENDIAN) > > +#if HOST_BIG_ENDIAN > > struct { > > uint32_t upper; > > uint32_t lower; > > @@ -235,7 +235,7 @@ typedef union { > > > > typedef union { > > float128 q; > > -#if defined(HOST_WORDS_BIGENDIAN) > > +#if HOST_BIG_ENDIAN > > struct { > > uint32_t upmost; > > uint32_t upper; > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > > index 0a5e67fb970e..7fdd88adb368 100644 > > --- a/include/qemu/compiler.h > > +++ b/include/qemu/compiler.h > > @@ -7,6 +7,8 @@ > > #ifndef COMPILER_H > > #define COMPILER_H > > > > +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) > > Why don't you do it this way instead: > > #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > #define HOST_WORDS_BIGENDIAN 1 > #endif > > ... that way you could avoid the churn in all the other files? > I guess "prevents from bad usage, if the config header wasn't included before" from the commit message is the answer to that question. I agree that it is more robust. If we keep the #if defined we really can't differentiate between "not defined because not big-endian" and "not defined because the appropriate header was not included."
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On 16/03/2022 12.15, Halil Pasic wrote: On Wed, 16 Mar 2022 11:28:59 +0100 Thomas Huth wrote: On 16/03/2022 10.53, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Replace a config-time define with a compile time condition define (compatible with clang and gcc) that must be declared prior to its usage. This avoids having a global configure time define, but also prevents from bad usage, if the config header wasn't included before. This can help to make some code independent from qemu too. gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. Signed-off-by: Marc-André Lureau --- [...] @@ -188,7 +188,7 @@ CPU_CONVERT(le, 64, uint64_t) * a compile-time constant if you pass in a constant. So this can be * used to initialize static variables. */ -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN # define const_le32(_x) \ _x) & 0x00ffU) << 24) | \ (((_x) & 0xff00U) << 8) | \ @@ -211,7 +211,7 @@ typedef union { typedef union { float64 d; -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN struct { uint32_t upper; uint32_t lower; @@ -235,7 +235,7 @@ typedef union { typedef union { float128 q; -#if defined(HOST_WORDS_BIGENDIAN) +#if HOST_BIG_ENDIAN struct { uint32_t upmost; uint32_t upper; diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 0a5e67fb970e..7fdd88adb368 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -7,6 +7,8 @@ #ifndef COMPILER_H #define COMPILER_H +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) Why don't you do it this way instead: #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ #define HOST_WORDS_BIGENDIAN 1 #endif ... that way you could avoid the churn in all the other files? I guess "prevents from bad usage, if the config header wasn't included before" from the commit message is the answer to that question. I agree that it is more robust. If we keep the #if defined we really can't differentiate between "not defined because not big-endian" and "not defined because the appropriate header was not included." Ok, fair point, now I got it. Acked-by: Thomas Huth
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
Hi On Wed, Mar 16, 2022 at 3:16 PM Halil Pasic wrote: > > On Wed, 16 Mar 2022 11:28:59 +0100 > Thomas Huth wrote: > > > On 16/03/2022 10.53, marcandre.lur...@redhat.com wrote: > > > From: Marc-André Lureau > > > > > > Replace a config-time define with a compile time condition > > > define (compatible with clang and gcc) that must be declared prior to > > > its usage. This avoids having a global configure time define, but also > > > prevents from bad usage, if the config header wasn't included before. > > > > > > This can help to make some code independent from qemu too. > > > > > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. > > > > > > Signed-off-by: Marc-André Lureau > > > --- > > [...] > > > @@ -188,7 +188,7 @@ CPU_CONVERT(le, 64, uint64_t) > > >* a compile-time constant if you pass in a constant. So this can be > > >* used to initialize static variables. > > >*/ > > > -#if defined(HOST_WORDS_BIGENDIAN) > > > +#if HOST_BIG_ENDIAN > > > # define const_le32(_x) \ > > > _x) & 0x00ffU) << 24) | \ > > >(((_x) & 0xff00U) << 8) | \ > > > @@ -211,7 +211,7 @@ typedef union { > > > > > > typedef union { > > > float64 d; > > > -#if defined(HOST_WORDS_BIGENDIAN) > > > +#if HOST_BIG_ENDIAN > > > struct { > > > uint32_t upper; > > > uint32_t lower; > > > @@ -235,7 +235,7 @@ typedef union { > > > > > > typedef union { > > > float128 q; > > > -#if defined(HOST_WORDS_BIGENDIAN) > > > +#if HOST_BIG_ENDIAN > > > struct { > > > uint32_t upmost; > > > uint32_t upper; > > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > > > index 0a5e67fb970e..7fdd88adb368 100644 > > > --- a/include/qemu/compiler.h > > > +++ b/include/qemu/compiler.h > > > @@ -7,6 +7,8 @@ > > > #ifndef COMPILER_H > > > #define COMPILER_H > > > > > > +#define HOST_BIG_ENDIAN (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__) > > > > Why don't you do it this way instead: > > > > #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ > > #define HOST_WORDS_BIGENDIAN 1 > > #endif > > > > ... that way you could avoid the churn in all the other files? > > > > I guess "prevents from bad usage, if the config header wasn't included > before" from the commit message is the answer to that question. I agree > that it is more robust. If we keep the #if defined we really can't > differentiate between "not defined because not big-endian" and "not > defined because the appropriate header was not included." That's right, thanks
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On Wed, 16 Mar 2022 13:53:07 +0400 marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > Replace a config-time define with a compile time condition > define (compatible with clang and gcc) that must be declared prior to > its usage. This avoids having a global configure time define, but also > prevents from bad usage, if the config header wasn't included before. > > This can help to make some code independent from qemu too. > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. > > Signed-off-by: Marc-André Lureau LGTM For the s390x parts I'm involved in: Acked-by: Halil Pasic [..] > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -34,13 +34,13 @@ > > /* some important defines: > * > - * HOST_WORDS_BIGENDIAN : if defined, the host cpu is big endian and > + * HOST_BIG_ENDIAN : whether the host cpu is big endian and > * otherwise little endian. > * > * TARGET_WORDS_BIGENDIAN : same for target cpu > */ This comment does not seem spot on any more. BTW would it make sense to replace TARGET_WORDS_BIGENDIAN with TARGET_BIG_ENDIAN as well. I believe the bad usage argument applies equally to both, and IMHO we should keep the both consistent naming and usage wise. > > -#if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) > +#if HOST_BIG_ENDIAN != defined(TARGET_WORDS_BIGENDIAN) > #define BSWAP_NEEDED > #endif > [..]
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
On Wed, Mar 16, 2022 at 01:52:48PM +0400, marcandre.lur...@redhat.com wrote: > From: Marc-André Lureau > > One less qemu-specific macro. It also helps to make some headers/units > only depend on glib, and thus moved in standalone projects eventually. > > Signed-off-by: Marc-André Lureau I checked the replacements and couldn't spot any differences (I assume you used a 'perl -pi.bak -e s///' or similar rather than doing it by hand?). Also I checked the macro definitions in include/qemu/compiler.h vs /usr/include/glib-2.0/glib/gmacros.h and they're pretty much identical. I even learned about gnu_printf. So: Reviewed-by: Richard W.M. Jones Shouldn't there be a hunk which removes the definition of GCC_FMT_ATTR from include/qemu/compiler.h? Maybe that's in another place in the patch series. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
Re: [PATCH 22/27] error-report: replace error progname with glib functions
I'd prefer error: Use GLib to remember the program name marcandre.lur...@redhat.com writes: > From: Marc-André Lureau > > Signed-off-by: Marc-André Lureau Reviewed-by: Markus Armbruster
Re: [PATCH v2 4/6] vduse-blk: implements vduse-blk export
On Tue, Mar 15, 2022 at 07:52:03PM +0800, Yongji Xie wrote: > On Tue, Mar 15, 2022 at 7:08 PM Stefan Hajnoczi wrote: > > > > On Tue, Feb 15, 2022 at 06:59:41PM +0800, Xie Yongji wrote: > > > This implements a VDUSE block backends based on > > > the libvduse library. We can use it to export the BDSs > > > for both VM and container (host) usage. > > > > > > The new command-line syntax is: > > > > > > $ qemu-storage-daemon \ > > > --blockdev file,node-name=drive0,filename=test.img \ > > > --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on > > > > > > After the qemu-storage-daemon started, we need to use > > > the "vdpa" command to attach the device to vDPA bus: > > > > > > $ vdpa dev add name vduse-export0 mgmtdev vduse > > > > The per-QEMU export id is used as the global vdpa device name. If this > > becomes a problem in the future then a new --export > > vduse-blk,vdpa-dev-name= option can be added. > > > > Yes. > > > > +case VIRTIO_BLK_T_GET_ID: { > > > +size_t size = MIN(iov_size(&elem->in_sg[0], in_num), > > > + VIRTIO_BLK_ID_BYTES); > > > +snprintf(elem->in_sg[0].iov_base, size, "%s", > > > vblk_exp->export.id); > > > > Please use iov_from_buf(). The driver is allowed to submit as many > > in_sg[] elements as it wants and a compliant virtio-blk device > > implementation must support that. > > > > Got it. > > > Here is the VIRTIO specification section that covers message framing: > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-280004 > > > > > +features = (1ULL << VIRTIO_F_IOMMU_PLATFORM) | > > > + (1ULL << VIRTIO_F_VERSION_1) | > > > + (1ULL << VIRTIO_RING_F_EVENT_IDX) | > > > + (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > > + (1ULL << VIRTIO_BLK_F_SIZE_MAX) | > > > + (1ULL << VIRTIO_BLK_F_SEG_MAX) | > > > + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | > > > + (1ULL << VIRTIO_BLK_F_BLK_SIZE); > > > > The VIRTIO_F_ and VIRTIO_RING_F_ feature bits report the capabilities of > > libvduse. They should probably be defined in libvduse. That way no > > changes to vduse-blk.c are required when libvduse changes: > > > > features = LIBVDUSE_VIRTIO_FEATURES | > > (1ULL << VIRTIO_BLK_F_SIZE_MAX) | > > ...; > > It's OK to define the VIRTIO_F_* feature bits in libvduse since daemon > might not want to disable it. But maybe not including VIRTIO_RING_F_* > feature bits since daemon might want to disable them in some cases. The VIRTIO_RING_F_* functionality is implemented inside libvduse and not the device code. If the device wants to disable specific features, it can clear those bits. Thinking about the LIBVDUSE_VIRTIO_FEATURES idea I think it's slightly better to make it a function so that libvduse code supports dynamic linking. That way the device calls libvduse to query the feature bits instead of compiling a constant from the libvduse header file into the device executable. So something like: features = (vdu_get_virtio_features() & ~(...features we wish to clear...)) | (1ULL << VIRTIO_BLK_F_SIZE_MAX) | ...; Stefan signature.asc Description: PGP signature
Re: [PATCH v2 1/6] block: Support passing NULL ops to blk_set_dev_ops()
On Tue, Mar 15, 2022 at 03:30:22PM -0400, John Snow wrote: > On Tue, Mar 15, 2022 at 4:47 AM Stefan Hajnoczi wrote: > > > > On Mon, Mar 14, 2022 at 03:09:35PM -0400, John Snow wrote: > > > On Mon, Mar 14, 2022 at 1:23 PM Stefan Hajnoczi > > > wrote: > > > > > > > > On Tue, Feb 15, 2022 at 06:59:38PM +0800, Xie Yongji wrote: > > > > > This supports passing NULL ops to blk_set_dev_ops() > > > > > so that we can remove stale ops in some cases. > > > > > > > > > > Signed-off-by: Xie Yongji > > > > > --- > > > > > block/block-backend.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/block/block-backend.c b/block/block-backend.c > > > > > index 4ff6b4d785..08dd0a3093 100644 > > > > > --- a/block/block-backend.c > > > > > +++ b/block/block-backend.c > > > > > @@ -1015,7 +1015,7 @@ void blk_set_dev_ops(BlockBackend *blk, const > > > > > BlockDevOps *ops, > > > > > blk->dev_opaque = opaque; > > > > > > > > > > /* Are we currently quiesced? Should we enforce this right now? > > > > > */ > > > > > -if (blk->quiesce_counter && ops->drained_begin) { > > > > > +if (blk->quiesce_counter && ops && ops->drained_begin) { > > > > > ops->drained_begin(opaque); > > > > > } > > > > > } > > > > > > > > John: You added this code in f4d9cc88ee6. Does blk_set_dev_ops() need to > > > > call ->drained_end() when ops is set to NULL? > > > > > > > > Stefan > > > > > > I'm not sure I trust my memory from five years ago. > > > > > > From what I recall, the problem was that block jobs weren't getting > > > drained/paused when the backend was getting quiesced -- we wanted to > > > be sure that a blockjob wasn't continuing to run and submit requests > > > against a backend we wanted to have on ice during a sensitive > > > operation. This conditional stanza here is meant to check if the node > > > we're already attached to is *already quiesced* and we missed the > > > signal (so-to-speak), so we replay the drained_begin() request right > > > there. > > > > > > i.e. there was some case where blockjobs were getting added to an > > > already quiesced node, and this code here post-hoc relays that drain > > > request to the blockjob. This gets used in > > > 600ac6a0ef5c06418446ef2f37407bddcc51b21c to pause/unpause jobs. > > > Original thread is here: > > > https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg03416.html > > > > > > Now, I'm not sure why you want to set ops to NULL here. If we're in a > > > drained section, that sounds like it might be potentially bad because > > > we lose track of the operation to end the drained section. If your > > > intent is to destroy the thing that we'd need to call drained_end on, > > > I guess it doesn't matter -- provided you've cleaned up the target > > > object correctly. Just calling drained_end() pre-emptively seems like > > > it might be bad, what if it unpauses something you're in the middle of > > > trying to delete? > > > > > > I might need slightly more context to know what you're hoping to > > > accomplish, but I hope this info helps contextualize this code > > > somewhat. > > > > Setting to NULL in this patch is a subset of blk_detach_dev(), which > > gets called when a storage controller is hot unplugged. > > > > In this patch series there is no DeviceState because a VDUSE export is > > not a device. The VDUSE code calls blk_set_dev_ops() to > > register/unregister callbacks (e.g. ->resize_cb()). > > > > The reason I asked about ->drained_end() is for symmetry. If the > > device's ->drained_begin() callback changed state or allocated resources > > then they may need to be freed/reset. On the other hand, the > > blk_set_dev_ops(blk, NULL, NULL) call should be made by the dev_ops > > owner so they can clean up without a ->drained_end() call. > > OK, got it... Hm, we don't actually use these for BlockJobs anymore. > It looks like the only user of these callbacks now is the NBD driver. > > ad90febaf22d95e49fb6821bfb3ebd05b4919417 followed not long after my > initial patch and removed my intended user. I tried just removing the > fields, but the build chokes on NBD. > It looks like these usages are pretty modern, Sergio added them in > fd6afc50 (2021-06-02). So, I guess we do actually still use these > hooks. (After a period of maybe not using them for 4 years? Wow.) > > I'm not clear on what we *want* to happen here, though. It doesn't > sound like NBD is the anticipated use case, so maybe just make the > removal fail if the drained section is active and callbacks are > defined? That's the safe thing to do, probably. I'm not sure either. CCing Eric. Maybe the answer is to just leave it as-is. Stefan signature.asc Description: PGP signature
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Tue, Mar 15, 2022 at 06:29:50PM +0100, Paolo Bonzini wrote: > On 3/15/22 15:24, Peter Maydell wrote: > > On Tue, 15 Mar 2022 at 14:09, Stefan Hajnoczi wrote: > > > Also, once C++ is available people will > > > start submitting C++ patches simply because they are more comfortable > > > with C++ (especially one-time/infrequent contributors). > > > > This to my mind is the major argument against using C++ > > for coroutines... > > I agree on the need for a policy, but _what_ C++ are they going to be > contributing that we should be scared of? We're talking about: > > * major features contributed by one-time/infrequent participants (which is > already a once-in-a-year thing or so, at least for me) > > * ... in an area where there are no examples of using C++ in the tree (or > presumably the maintainer would be comfortable reviewing it) > > * ... but yet C++ offer killer features (right now there's only C++ > coroutines and fpu/) You are assuming people only choose C++ only when it offers features not available in C. I think they might simply be more comfortable in C++. In other words, if an existing file is compiled using a C++ compiler or they are adding a new file, they don't need a reason to use C++, they can just use it. You can define rules and a way to enforce a subset of C++, but I think over time the code will be C++. A policy that is complicated discourages contributors. For these reasons I think that if code runs through a C++ compiler we should just allow C++. Either way, it will take time but that way no one will feel betrayed when C++ creeps in. That said, I hope we find an option that doesn't involve C++. Stefan signature.asc Description: PGP signature
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Wed, Mar 16, 2022 at 12:08:33AM +0100, Paolo Bonzini wrote: > On 3/15/22 16:55, Daniel P. Berrangé wrote: > > Expecting maintainers to enforce a subset during code review feels > > like it would be a tedious burden, that will inevitably let stuff > > through because humans are fallible, especially when presented > > with uninspiring, tedious, repetitive tasks. > > > > Restricting ourselves to a subset is only viable if we have > > an automated tool that can reliably enforce that subset. I'm not > > sure that any such tool exists, and not convinced our time is > > best served by trying to write & maintainer one either. > > We don't need to have a policy on which features are used. We need to have > goals for what to use C++ for. I won't go into further details here, > because I had already posted "When and how to use C++"[1] about an hour > before your reply. > > > IOW, I fear one we allow C++ in any level, it won't be practical > > to constrain it as much we desire. I fear us turning QEMU into > > even more of a monster like other big C++ apps I see which take > > all hours to compile while using all available RAM in Fedora RPM > > build hosts. > > Sorry but this is FUD. There's plenty of C++ apps and libraries that do not > "take hours to compile while using all available RAM". You're probably > thinking of the Chromium/Firefox/Libreoffice triplet but those are an order > of magnitude larger than QEMU. And in fact, QEMU is *already* a monster > that takes longer to compile than most other packages, no matter the > language they're written in. > > Most of KDE and everything that uses Qt is written in C++, and so is > Inkscape in GTK+ land. LLVM and Clang are written in C++. Hotspot and V8 > are written in C++. Kodi, MAME and DolphinEmu are written in C++. GCC and > GDB have migrated to C++ and their compile times have not exploded. > > > My other question is whether adoption of C++ would complicate any > > desire to make more use of Rust in QEMU ? I know Rust came out of > > work by the Mozilla Firefox crew, and Firefox was C++, but I don't > > have any idea how they integrated use of Rust with Firefox, so > > whether there are any gotcha's for us or not ? > > Any Rust integration would go through C APIs. Using Rust in the block layer > would certainly be much harder, though perhaps not impossible, if the block > layer uses C++ coroutines. Rust supports something similar, but > two-direction interoperability would be hard. I haven't looked at this in depth but there is a solution for Rust-C++ interop: https://cxx.rs/ Stefan signature.asc Description: PGP signature
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
On 16/3/22 13:44, Philippe Mathieu-Daudé wrote: Hi, On 4/3/22 17:46, Kevin Wolf wrote: From: Emanuele Giuseppe Esposito All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. I'm getting this crash: $ qemu-system-i386 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. Abort trap: 6 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. qemu-system-i386 was compiled with optimization - stepping may behave oddly; variables may not be available. Process 76914 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt] 549 */ 550 BlockBackend *blk_all_next(BlockBackend *blk) 551 { -> 552 GLOBAL_STATE_CODE(); 553 return blk ? QTAILQ_NEXT(blk, link) 554 : QTAILQ_FIRST(&block_backends); 555 } Target 1: (qemu-system-i386) stopped. Forgot to paste the backtrace: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert frame #0: 0x0001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8 frame #1: 0x0001908fceb0 libsystem_pthread.dylib`pthread_kill + 288 frame #2: 0x00019083a314 libsystem_c.dylib`abort + 164 frame #3: 0x00019083972c libsystem_c.dylib`__assert_rtn + 300 * frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt] frame #5: 0x0001003c00b4 qemu-system-i386`blk_all_next(blk=) at block-backend.c:552:5 [opt] frame #6: 0x0001003d8f04 qemu-system-i386`qmp_query_block(errp=0x) at qapi.c:591:16 [opt] frame #7: 0x00010003ab0c qemu-system-i386`main [inlined] addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt] frame #8: 0x00010003ab04 qemu-system-i386`main(argc=, argv=) at cocoa.m:1980:5 [opt] frame #9: 0x0001012690f4 dyld`start + 520 Bisected to this patch: 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 is the first bad commit Signed-off-by: Emanuele Giuseppe Esposito Message-Id: <20220303151616.325444-9-eespo...@redhat.com> Signed-off-by: Kevin Wolf --- block/block-backend.c | 78 ++ softmmu/qdev-monitor.c | 2 ++ 2 files changed, 80 insertions(+)
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
Hi, On 4/3/22 17:46, Kevin Wolf wrote: From: Emanuele Giuseppe Esposito All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. I'm getting this crash: $ qemu-system-i386 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. Abort trap: 6 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. qemu-system-i386 was compiled with optimization - stepping may behave oddly; variables may not be available. Process 76914 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt] 549*/ 550 BlockBackend *blk_all_next(BlockBackend *blk) 551 { -> 552 GLOBAL_STATE_CODE(); 553 return blk ? QTAILQ_NEXT(blk, link) 554 : QTAILQ_FIRST(&block_backends); 555 } Target 1: (qemu-system-i386) stopped. Bisected to this patch: 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 is the first bad commit Signed-off-by: Emanuele Giuseppe Esposito Message-Id: <20220303151616.325444-9-eespo...@redhat.com> Signed-off-by: Kevin Wolf --- block/block-backend.c | 78 ++ softmmu/qdev-monitor.c | 2 ++ 2 files changed, 80 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index 462e18facf..4476b61b8b 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -239,6 +239,7 @@ static void blk_root_activate(BdrvChild *child, Error **errp) void blk_set_force_allow_inactivate(BlockBackend *blk) { +GLOBAL_STATE_CODE(); blk->force_allow_inactivate = true; } @@ -357,6 +358,8 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm) { BlockBackend *blk; +GLOBAL_STATE_CODE(); + blk = g_new0(BlockBackend, 1); blk->refcnt = 1; blk->ctx = ctx; @@ -394,6 +397,8 @@ BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm, { BlockBackend *blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm); +GLOBAL_STATE_CODE(); + if (blk_insert_bs(blk, bs, errp) < 0) { blk_unref(blk); return NULL; @@ -422,6 +427,8 @@ BlockBackend *blk_new_open(const char *filename, const char *reference, uint64_t perm = 0; uint64_t shared = BLK_PERM_ALL; +GLOBAL_STATE_CODE(); + /* * blk_new_open() is mainly used in .bdrv_create implementations and the * tools where sharing isn't a major concern because the BDS stays private @@ -499,6 +506,7 @@ static void drive_info_del(DriveInfo *dinfo) int blk_get_refcnt(BlockBackend *blk) { +GLOBAL_STATE_CODE(); return blk ? blk->refcnt : 0; } @@ -509,6 +517,7 @@ int blk_get_refcnt(BlockBackend *blk) void blk_ref(BlockBackend *blk) { assert(blk->refcnt > 0); +GLOBAL_STATE_CODE(); blk->refcnt++; } @@ -519,6 +528,7 @@ void blk_ref(BlockBackend *blk) */ void blk_unref(BlockBackend *blk) { +GLOBAL_STATE_CODE(); if (blk) { assert(blk->refcnt > 0); if (blk->refcnt > 1) { @@ -539,6 +549,7 @@ void blk_unref(BlockBackend *blk) */ BlockBackend *blk_all_next(BlockBackend *blk) { +GLOBAL_STATE_CODE(); return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&block_backends); } @@ -547,6 +558,8 @@ void blk_remove_all_bs(void) { BlockBackend *blk = NULL; +GLOBAL_STATE_CODE(); + while ((blk = blk_all_next(blk)) != NULL) { AioContext *ctx = blk_get_aio_context(blk); @@ -570,6 +583,7 @@ void blk_remove_all_bs(void) */ BlockBackend *blk_next(BlockBackend *blk) { +GLOBAL_STATE_CODE(); return blk ? QTAILQ_NEXT(blk, monitor_link) : QTAILQ_FIRST(&monitor_block_backends); } @@ -636,6 +650,7 @@ static void bdrv_next_reset(BdrvNextIterator *it) BlockDriverState *bdrv_first(BdrvNextIterator *it) { +GLOBAL_STATE_CODE(); bdrv_next_reset(it); return bdrv_next(it); } @@ -673,6 +688,7 @@ bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp) { assert(!blk->name); assert(name && name[0]); +GLOBAL_STATE_CODE(); if (!id_wellformed(name)) { error_setg(errp, "Invalid device name"); @@ -700,6 +716,8 @@ bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp) */ void monitor_remove_blk(BlockBackend *blk) { +GLOBAL_STATE_CODE(); + if (!blk->name) { return; } @@ -726,6 +744,7 @@ BlockBackend *blk_by_name(const char *name) { BlockBackend *blk = NULL; +GLOBAL_STATE_CODE(); assert(name); while ((blk = blk_next(blk)) != NULL) { if (!strcmp(name, blk->name)) { @@ -760,6 +779,7 @@ static Bl
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
Hi Philippe, How did you trigger this? What is the call stack? >From a first look, all callers of blk_all_next also call GLOBAL_STATE_CODE, except qmp_* functions. That would be useful to understand if we need to change the function assertion and classification, or the caller is breaking some invariants. Thank you, Emanuele Am 16/03/2022 um 13:44 schrieb Philippe Mathieu-Daudé: > Hi, > > On 4/3/22 17:46, Kevin Wolf wrote: >> From: Emanuele Giuseppe Esposito >> >> All the global state (GS) API functions will check that >> qemu_in_main_thread() returns true. If not, it means >> that the safety of BQL cannot be guaranteed, and >> they need to be moved to I/O. > > I'm getting this crash: > > $ qemu-system-i386 > Assertion failed: (qemu_in_main_thread()), function blk_all_next, file > block-backend.c, line 552. > Abort trap: 6 > > Assertion failed: (qemu_in_main_thread()), function blk_all_next, file > block-backend.c, line 552. > qemu-system-i386 was compiled with optimization - stepping may behave > oddly; variables may not be available. > Process 76914 stopped > * thread #1, queue = 'com.apple.main-thread', stop reason = hit program > assert > frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at > block-backend.c:552:5 [opt] > 549 */ > 550 BlockBackend *blk_all_next(BlockBackend *blk) > 551 { > -> 552 GLOBAL_STATE_CODE(); > 553 return blk ? QTAILQ_NEXT(blk, link) > 554 : QTAILQ_FIRST(&block_backends); > 555 } > Target 1: (qemu-system-i386) stopped. > > Bisected to this patch: > > 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 is the first bad commit > >> Signed-off-by: Emanuele Giuseppe Esposito >> Message-Id: <20220303151616.325444-9-eespo...@redhat.com> >> Signed-off-by: Kevin Wolf >> --- >> block/block-backend.c | 78 ++ >> softmmu/qdev-monitor.c | 2 ++ >> 2 files changed, 80 insertions(+) >> >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 462e18facf..4476b61b8b 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >> @@ -239,6 +239,7 @@ static void blk_root_activate(BdrvChild *child, >> Error **errp) >> void blk_set_force_allow_inactivate(BlockBackend *blk) >> { >> + GLOBAL_STATE_CODE(); >> blk->force_allow_inactivate = true; >> } >> @@ -357,6 +358,8 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t >> perm, uint64_t shared_perm) >> { >> BlockBackend *blk; >> + GLOBAL_STATE_CODE(); >> + >> blk = g_new0(BlockBackend, 1); >> blk->refcnt = 1; >> blk->ctx = ctx; >> @@ -394,6 +397,8 @@ BlockBackend *blk_new_with_bs(BlockDriverState >> *bs, uint64_t perm, >> { >> BlockBackend *blk = blk_new(bdrv_get_aio_context(bs), perm, >> shared_perm); >> + GLOBAL_STATE_CODE(); >> + >> if (blk_insert_bs(blk, bs, errp) < 0) { >> blk_unref(blk); >> return NULL; >> @@ -422,6 +427,8 @@ BlockBackend *blk_new_open(const char *filename, >> const char *reference, >> uint64_t perm = 0; >> uint64_t shared = BLK_PERM_ALL; >> + GLOBAL_STATE_CODE(); >> + >> /* >> * blk_new_open() is mainly used in .bdrv_create implementations >> and the >> * tools where sharing isn't a major concern because the BDS >> stays private >> @@ -499,6 +506,7 @@ static void drive_info_del(DriveInfo *dinfo) >> int blk_get_refcnt(BlockBackend *blk) >> { >> + GLOBAL_STATE_CODE(); >> return blk ? blk->refcnt : 0; >> } >> @@ -509,6 +517,7 @@ int blk_get_refcnt(BlockBackend *blk) >> void blk_ref(BlockBackend *blk) >> { >> assert(blk->refcnt > 0); >> + GLOBAL_STATE_CODE(); >> blk->refcnt++; >> } >> @@ -519,6 +528,7 @@ void blk_ref(BlockBackend *blk) >> */ >> void blk_unref(BlockBackend *blk) >> { >> + GLOBAL_STATE_CODE(); >> if (blk) { >> assert(blk->refcnt > 0); >> if (blk->refcnt > 1) { >> @@ -539,6 +549,7 @@ void blk_unref(BlockBackend *blk) >> */ >> BlockBackend *blk_all_next(BlockBackend *blk) >> { >> + GLOBAL_STATE_CODE(); >> return blk ? QTAILQ_NEXT(blk, link) >> : QTAILQ_FIRST(&block_backends); >> } >> @@ -547,6 +558,8 @@ void blk_remove_all_bs(void) >> { >> BlockBackend *blk = NULL; >> + GLOBAL_STATE_CODE(); >> + >> while ((blk = blk_all_next(blk)) != NULL) { >> AioContext *ctx = blk_get_aio_context(blk); >> @@ -570,6 +583,7 @@ void blk_remove_all_bs(void) >> */ >> BlockBackend *blk_next(BlockBackend *blk) >> { >> + GLOBAL_STATE_CODE(); >> return blk ? QTAILQ_NEXT(blk, monitor_link) >> : QTAILQ_FIRST(&monitor_block_backends); >> } >> @@ -636,6 +650,7 @@ static void bdrv_next_reset(BdrvNextIterator *it) >> BlockDriverState *bdrv_first(BdrvNextIterator *it) >> { >> + GLOBAL_STATE_CODE(); >> bdrv_next_reset(it); >> return bdrv_next(it); >> } >> @@ -673,6 +688,7 @@ bool moni
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On 16/3/22 10:53, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Replace a config-time define with a compile time condition define (compatible with clang and gcc) that must be declared prior to its usage. This avoids having a global configure time define, but also prevents from bad usage, if the config header wasn't included before. This can help to make some code independent from qemu too. gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. Signed-off-by: Marc-André Lureau --- meson.build | 1 - accel/tcg/atomic_template.h | 4 +- audio/audio.h | 2 +- hw/display/pl110_template.h | 6 +-- hw/net/can/ctucan_core.h| 2 +- hw/net/vmxnet3.h| 4 +- include/exec/cpu-all.h | 4 +- include/exec/cpu-common.h | 2 +- include/exec/memop.h| 2 +- include/exec/memory.h | 2 +- include/fpu/softfloat-types.h | 2 +- include/hw/core/cpu.h | 2 +- include/hw/i386/intel_iommu.h | 6 +-- include/hw/i386/x86-iommu.h | 4 +- include/hw/virtio/virtio-access.h | 6 +-- include/hw/virtio/virtio-gpu-bswap.h| 2 +- include/libdecnumber/dconfig.h | 2 +- include/net/eth.h | 2 +- include/qemu/bswap.h| 8 ++-- include/qemu/compiler.h | 2 + include/qemu/host-utils.h | 2 +- include/qemu/int128.h | 2 +- include/ui/qemu-pixman.h| 2 +- net/util.h | 2 +- target/arm/cpu.h| 8 ++-- target/arm/translate-a64.h | 2 +- target/arm/vec_internal.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 2 +- target/ppc/cpu.h| 2 +- target/s390x/tcg/vec.h | 2 +- target/xtensa/cpu.h | 2 +- tests/fp/platform.h | 4 +- accel/kvm/kvm-all.c | 4 +- audio/dbusaudio.c | 2 +- disas.c | 2 +- hw/core/loader.c| 4 +- hw/display/artist.c | 6 +-- hw/display/pxa2xx_lcd.c | 2 +- hw/display/vga.c| 12 +++--- hw/display/virtio-gpu-gl.c | 2 +- hw/s390x/event-facility.c | 2 +- hw/virtio/vhost.c | 2 +- linux-user/arm/nwfpe/double_cpdo.c | 4 +- linux-user/arm/nwfpe/fpa11_cpdt.c | 4 +- linux-user/ppc/signal.c | 3 +- linux-user/syscall.c| 6 +-- net/net.c | 4 +- target/alpha/translate.c| 2 +- target/arm/crypto_helper.c | 2 +- target/arm/helper.c | 2 +- target/arm/kvm64.c | 4 +- target/arm/neon_helper.c| 2 +- target/arm/sve_helper.c | 4 +- target/arm/translate-sve.c | 6 +-- target/arm/translate-vfp.c | 2 +- target/arm/translate.c | 2 +- target/hppa/translate.c | 2 +- target/i386/tcg/translate.c | 2 +- target/mips/tcg/lmmi_helper.c | 2 +- target/mips/tcg/msa_helper.c| 54 - target/ppc/arch_dump.c | 2 +- target/ppc/int_helper.c | 22 +- target/ppc/kvm.c| 4 +- target/ppc/mem_helper.c | 2 +- target/riscv/vector_helper.c| 2 +- target/s390x/tcg/translate.c| 2 +- target/sparc/vis_helper.c | 4 +- tcg/tcg-op.c| 4 +- tcg/tcg.c | 12 +++--- tests/qtest/vhost-user-blk-test.c | 2 +- tests/qtest/virtio-blk-test.c | 2 +- ui/vdagent.c| 2 +- ui/vnc.c| 2 +- util/bitmap.c | 2 +- util/host-utils.c | 2 +- target/ppc/translate/vmx-impl.c.inc | 4 +- target/ppc/translate/vsx-impl.c.inc | 2 +- target/riscv/insn_trans/trans_rvv.c.inc | 4 +- target/s390x/tcg/translate_vx.c.inc | 2 +- tcg/aarch64/tcg-target.c.inc| 4 +- tcg/arm/tcg-target.c.inc| 4 +- tcg/mips/tcg-target.c.inc | 2 +- tcg/ppc/tcg-target.c.inc| 10 ++--- tcg/riscv/tcg-target.c.inc | 4 +- 85 files changed, 173 insertions(+), 173 deletions(-) diff --git a/meson.build b/meson.build index f20712cb93d7..88df1bc42973 100644 --- a/meson.build +++ b/meson.build @@ -1591,7 +1
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Wed, Mar 16, 2022 at 12:32:48PM +, Stefan Hajnoczi wrote: > On Tue, Mar 15, 2022 at 06:29:50PM +0100, Paolo Bonzini wrote: > > On 3/15/22 15:24, Peter Maydell wrote: > > > On Tue, 15 Mar 2022 at 14:09, Stefan Hajnoczi wrote: > > > > Also, once C++ is available people will > > > > start submitting C++ patches simply because they are more comfortable > > > > with C++ (especially one-time/infrequent contributors). > > > > > > This to my mind is the major argument against using C++ > > > for coroutines... > > > > I agree on the need for a policy, but _what_ C++ are they going to be > > contributing that we should be scared of? We're talking about: > > > > * major features contributed by one-time/infrequent participants (which is > > already a once-in-a-year thing or so, at least for me) > > > > * ... in an area where there are no examples of using C++ in the tree (or > > presumably the maintainer would be comfortable reviewing it) > > > > * ... but yet C++ offer killer features (right now there's only C++ > > coroutines and fpu/) > > You are assuming people only choose C++ only when it offers features not > available in C. I think they might simply be more comfortable in C++. > > In other words, if an existing file is compiled using a C++ compiler or > they are adding a new file, they don't need a reason to use C++, they > can just use it. > > You can define rules and a way to enforce a subset of C++, but I think > over time the code will be C++. A policy that is complicated discourages > contributors. > > For these reasons I think that if code runs through a C++ compiler we > should just allow C++. Either way, it will take time but that way no one > will feel betrayed when C++ creeps in. > > That said, I hope we find an option that doesn't involve C++. The real show stopper with our current coroutine impl IIUC, is the undefined behaviour when we yield and restore across different threads. Is there any relastic hope that we can change QEMU's usage, such that each coroutine is confined to a single thread, to avoid the undefined behaviour ? With 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 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
On 16/3/22 10:52, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau One less qemu-specific macro. It also helps to make some headers/units only depend on glib, and thus moved in standalone projects eventually. Signed-off-by: Marc-André Lureau --- audio/audio.h | 4 +-- block/qcow2.h | 2 +- bsd-user/qemu.h | 2 +- hw/display/qxl.h| 2 +- hw/net/rocker/rocker.h | 2 +- hw/xen/xen_pt.h | 2 +- include/chardev/char-fe.h | 2 +- include/disas/dis-asm.h | 2 +- include/hw/acpi/aml-build.h | 12 +++ include/hw/core/cpu.h | 2 +- include/hw/hw.h | 2 +- include/hw/virtio/virtio.h | 2 +- include/hw/xen/xen-bus-helper.h | 4 +-- include/hw/xen/xen-bus.h| 4 +-- include/hw/xen/xen_common.h | 2 +- include/hw/xen/xen_pvdev.h | 2 +- include/monitor/monitor.h | 4 +-- include/qapi/error.h| 20 ++-- include/qapi/qmp/qjson.h| 8 ++--- include/qemu/buffer.h | 2 +- include/qemu/compiler.h | 11 ++- include/qemu/error-report.h | 24 +++--- include/qemu/log-for-trace.h| 2 +- include/qemu/log.h | 2 +- include/qemu/qemu-print.h | 8 ++--- include/qemu/readline.h | 2 +- qga/guest-agent-core.h | 2 +- qga/vss-win32/requester.h | 2 +- scripts/cocci-macro-file.h | 2 +- tests/qtest/libqos/libqtest.h | 42 - tests/qtest/libqtest-single.h | 2 +- tests/qtest/migration-helpers.h | 6 ++-- audio/alsaaudio.c | 4 +-- audio/dsoundaudio.c | 4 +-- audio/ossaudio.c| 4 +-- audio/paaudio.c | 2 +- audio/sdlaudio.c| 2 +- block/blkverify.c | 2 +- block/ssh.c | 4 +-- fsdev/9p-marshal.c | 2 +- fsdev/virtfs-proxy-helper.c | 2 +- hw/9pfs/9p.c| 2 +- hw/acpi/aml-build.c | 4 +-- hw/mips/fuloong2e.c | 2 +- hw/mips/malta.c | 2 +- hw/net/rtl8139.c| 2 +- hw/virtio/virtio.c | 2 +- io/channel-websock.c| 2 +- monitor/hmp.c | 4 +-- nbd/server.c| 10 +++--- qemu-img.c | 4 +-- qemu-io.c | 2 +- qobject/json-parser.c | 2 +- softmmu/qtest.c | 4 +-- tests/qtest/libqtest.c | 2 +- tests/unit/test-qobject-input-visitor.c | 4 +-- audio/coreaudio.m | 4 +-- scripts/checkpatch.pl | 2 +- 58 files changed, 130 insertions(+), 137 deletions(-) diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 3baa5e3790f7..f2bd050e3b9a 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -79,19 +79,12 @@ #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) -#if defined(__clang__) -/* clang doesn't support gnu_printf, so use printf. */ -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) -#else -/* Use gnu_printf (qemu uses standard format strings). */ -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) -# if defined(_WIN32) +#if !defined(__clang__) && defined(_WIN32) /* * Map __printf__ to __gnu_printf__ because we want standard format strings even * when MinGW or GLib include files use __printf__. */ -# define __printf__ __gnu_printf__ -# endif +# define __printf__ __gnu_printf__ #endif Can we also poison GCC_FMT_ATTR? Maybe split in 2 patches, 1 converting and another removing unused & poisoning?
Re: [PATCH 24/27] Remove trailing ; after G_DEFINE_AUTO macro
On 16/3/22 10:54, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau The macro doesn't need it. Signed-off-by: Marc-André Lureau --- configure| 2 +- nbd/server.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On Wed, Mar 16, 2022 at 5:04 PM Philippe Mathieu-Daudé wrote: > > On 16/3/22 10:53, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > Replace a config-time define with a compile time condition > > define (compatible with clang and gcc) that must be declared prior to > > its usage. This avoids having a global configure time define, but also > > prevents from bad usage, if the config header wasn't included before. > > > > This can help to make some code independent from qemu too. > > > > gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. > > > > Signed-off-by: Marc-André Lureau > > --- > > meson.build | 1 - > > accel/tcg/atomic_template.h | 4 +- > > audio/audio.h | 2 +- > > hw/display/pl110_template.h | 6 +-- > > hw/net/can/ctucan_core.h| 2 +- > > hw/net/vmxnet3.h| 4 +- > > include/exec/cpu-all.h | 4 +- > > include/exec/cpu-common.h | 2 +- > > include/exec/memop.h| 2 +- > > include/exec/memory.h | 2 +- > > include/fpu/softfloat-types.h | 2 +- > > include/hw/core/cpu.h | 2 +- > > include/hw/i386/intel_iommu.h | 6 +-- > > include/hw/i386/x86-iommu.h | 4 +- > > include/hw/virtio/virtio-access.h | 6 +-- > > include/hw/virtio/virtio-gpu-bswap.h| 2 +- > > include/libdecnumber/dconfig.h | 2 +- > > include/net/eth.h | 2 +- > > include/qemu/bswap.h| 8 ++-- > > include/qemu/compiler.h | 2 + > > include/qemu/host-utils.h | 2 +- > > include/qemu/int128.h | 2 +- > > include/ui/qemu-pixman.h| 2 +- > > net/util.h | 2 +- > > target/arm/cpu.h| 8 ++-- > > target/arm/translate-a64.h | 2 +- > > target/arm/vec_internal.h | 2 +- > > target/i386/cpu.h | 2 +- > > target/mips/cpu.h | 2 +- > > target/ppc/cpu.h| 2 +- > > target/s390x/tcg/vec.h | 2 +- > > target/xtensa/cpu.h | 2 +- > > tests/fp/platform.h | 4 +- > > accel/kvm/kvm-all.c | 4 +- > > audio/dbusaudio.c | 2 +- > > disas.c | 2 +- > > hw/core/loader.c| 4 +- > > hw/display/artist.c | 6 +-- > > hw/display/pxa2xx_lcd.c | 2 +- > > hw/display/vga.c| 12 +++--- > > hw/display/virtio-gpu-gl.c | 2 +- > > hw/s390x/event-facility.c | 2 +- > > hw/virtio/vhost.c | 2 +- > > linux-user/arm/nwfpe/double_cpdo.c | 4 +- > > linux-user/arm/nwfpe/fpa11_cpdt.c | 4 +- > > linux-user/ppc/signal.c | 3 +- > > linux-user/syscall.c| 6 +-- > > net/net.c | 4 +- > > target/alpha/translate.c| 2 +- > > target/arm/crypto_helper.c | 2 +- > > target/arm/helper.c | 2 +- > > target/arm/kvm64.c | 4 +- > > target/arm/neon_helper.c| 2 +- > > target/arm/sve_helper.c | 4 +- > > target/arm/translate-sve.c | 6 +-- > > target/arm/translate-vfp.c | 2 +- > > target/arm/translate.c | 2 +- > > target/hppa/translate.c | 2 +- > > target/i386/tcg/translate.c | 2 +- > > target/mips/tcg/lmmi_helper.c | 2 +- > > target/mips/tcg/msa_helper.c| 54 - > > target/ppc/arch_dump.c | 2 +- > > target/ppc/int_helper.c | 22 +- > > target/ppc/kvm.c| 4 +- > > target/ppc/mem_helper.c | 2 +- > > target/riscv/vector_helper.c| 2 +- > > target/s390x/tcg/translate.c| 2 +- > > target/sparc/vis_helper.c | 4 +- > > tcg/tcg-op.c| 4 +- > > tcg/tcg.c | 12 +++--- > > tests/qtest/vhost-user-blk-test.c | 2 +- > > tests/qtest/virtio-blk-test.c | 2 +- > > ui/vdagent.c| 2 +- > > ui/vnc.c| 2 +- > > util/bitmap.c | 2 +- > > util/host-utils.c | 2 +- > > target/ppc/translate/vmx-impl.c.inc | 4 +- > > target/ppc/translate/vsx-impl.c.inc | 2 +- > > target/riscv/insn_trans/trans_rvv.c.inc | 4 +- > > target/s390x/tcg/translate_vx.c.inc | 2 +- > >
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
On 16/03/2022 14.16, Philippe Mathieu-Daudé wrote: On 16/3/22 10:52, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau One less qemu-specific macro. It also helps to make some headers/units only depend on glib, and thus moved in standalone projects eventually. Signed-off-by: Marc-André Lureau --- audio/audio.h | 4 +-- block/qcow2.h | 2 +- bsd-user/qemu.h | 2 +- hw/display/qxl.h | 2 +- hw/net/rocker/rocker.h | 2 +- hw/xen/xen_pt.h | 2 +- include/chardev/char-fe.h | 2 +- include/disas/dis-asm.h | 2 +- include/hw/acpi/aml-build.h | 12 +++ include/hw/core/cpu.h | 2 +- include/hw/hw.h | 2 +- include/hw/virtio/virtio.h | 2 +- include/hw/xen/xen-bus-helper.h | 4 +-- include/hw/xen/xen-bus.h | 4 +-- include/hw/xen/xen_common.h | 2 +- include/hw/xen/xen_pvdev.h | 2 +- include/monitor/monitor.h | 4 +-- include/qapi/error.h | 20 ++-- include/qapi/qmp/qjson.h | 8 ++--- include/qemu/buffer.h | 2 +- include/qemu/compiler.h | 11 ++- include/qemu/error-report.h | 24 +++--- include/qemu/log-for-trace.h | 2 +- include/qemu/log.h | 2 +- include/qemu/qemu-print.h | 8 ++--- include/qemu/readline.h | 2 +- qga/guest-agent-core.h | 2 +- qga/vss-win32/requester.h | 2 +- scripts/cocci-macro-file.h | 2 +- tests/qtest/libqos/libqtest.h | 42 - tests/qtest/libqtest-single.h | 2 +- tests/qtest/migration-helpers.h | 6 ++-- audio/alsaaudio.c | 4 +-- audio/dsoundaudio.c | 4 +-- audio/ossaudio.c | 4 +-- audio/paaudio.c | 2 +- audio/sdlaudio.c | 2 +- block/blkverify.c | 2 +- block/ssh.c | 4 +-- fsdev/9p-marshal.c | 2 +- fsdev/virtfs-proxy-helper.c | 2 +- hw/9pfs/9p.c | 2 +- hw/acpi/aml-build.c | 4 +-- hw/mips/fuloong2e.c | 2 +- hw/mips/malta.c | 2 +- hw/net/rtl8139.c | 2 +- hw/virtio/virtio.c | 2 +- io/channel-websock.c | 2 +- monitor/hmp.c | 4 +-- nbd/server.c | 10 +++--- qemu-img.c | 4 +-- qemu-io.c | 2 +- qobject/json-parser.c | 2 +- softmmu/qtest.c | 4 +-- tests/qtest/libqtest.c | 2 +- tests/unit/test-qobject-input-visitor.c | 4 +-- audio/coreaudio.m | 4 +-- scripts/checkpatch.pl | 2 +- 58 files changed, 130 insertions(+), 137 deletions(-) diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 3baa5e3790f7..f2bd050e3b9a 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -79,19 +79,12 @@ #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) -#if defined(__clang__) -/* clang doesn't support gnu_printf, so use printf. */ -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) -#else -/* Use gnu_printf (qemu uses standard format strings). */ -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) -# if defined(_WIN32) +#if !defined(__clang__) && defined(_WIN32) /* * Map __printf__ to __gnu_printf__ because we want standard format strings even * when MinGW or GLib include files use __printf__. */ -# define __printf__ __gnu_printf__ -# endif +# define __printf__ __gnu_printf__ #endif Can we also poison GCC_FMT_ATTR? Maybe split in 2 patches, 1 converting and another removing unused & poisoning? I don't think that poisoning is required here since this macro is not used in "#ifdef" statements - so the compiler will complain to you if you still try to use it after the removal. Thomas
Re: [PATCH 10/27] Replace config-time define HOST_WORDS_BIGENDIAN
On 16/3/22 14:04, Philippe Mathieu-Daudé wrote: On 16/3/22 10:53, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau Replace a config-time define with a compile time condition define (compatible with clang and gcc) that must be declared prior to its usage. This avoids having a global configure time define, but also prevents from bad usage, if the config header wasn't included before. This can help to make some code independent from qemu too. gcc supports __BYTE_ORDER__ from about 4.6 and clang from 3.2. Signed-off-by: Marc-André Lureau --- meson.build | 1 - accel/tcg/atomic_template.h | 4 +- audio/audio.h | 2 +- hw/display/pl110_template.h | 6 +-- hw/net/can/ctucan_core.h | 2 +- hw/net/vmxnet3.h | 4 +- include/exec/cpu-all.h | 4 +- include/exec/cpu-common.h | 2 +- include/exec/memop.h | 2 +- include/exec/memory.h | 2 +- include/fpu/softfloat-types.h | 2 +- include/hw/core/cpu.h | 2 +- include/hw/i386/intel_iommu.h | 6 +-- include/hw/i386/x86-iommu.h | 4 +- include/hw/virtio/virtio-access.h | 6 +-- include/hw/virtio/virtio-gpu-bswap.h | 2 +- include/libdecnumber/dconfig.h | 2 +- include/net/eth.h | 2 +- include/qemu/bswap.h | 8 ++-- include/qemu/compiler.h | 2 + include/qemu/host-utils.h | 2 +- include/qemu/int128.h | 2 +- include/ui/qemu-pixman.h | 2 +- net/util.h | 2 +- target/arm/cpu.h | 8 ++-- target/arm/translate-a64.h | 2 +- target/arm/vec_internal.h | 2 +- target/i386/cpu.h | 2 +- target/mips/cpu.h | 2 +- target/ppc/cpu.h | 2 +- target/s390x/tcg/vec.h | 2 +- target/xtensa/cpu.h | 2 +- tests/fp/platform.h | 4 +- accel/kvm/kvm-all.c | 4 +- audio/dbusaudio.c | 2 +- disas.c | 2 +- hw/core/loader.c | 4 +- hw/display/artist.c | 6 +-- hw/display/pxa2xx_lcd.c | 2 +- hw/display/vga.c | 12 +++--- hw/display/virtio-gpu-gl.c | 2 +- hw/s390x/event-facility.c | 2 +- hw/virtio/vhost.c | 2 +- linux-user/arm/nwfpe/double_cpdo.c | 4 +- linux-user/arm/nwfpe/fpa11_cpdt.c | 4 +- linux-user/ppc/signal.c | 3 +- linux-user/syscall.c | 6 +-- net/net.c | 4 +- target/alpha/translate.c | 2 +- target/arm/crypto_helper.c | 2 +- target/arm/helper.c | 2 +- target/arm/kvm64.c | 4 +- target/arm/neon_helper.c | 2 +- target/arm/sve_helper.c | 4 +- target/arm/translate-sve.c | 6 +-- target/arm/translate-vfp.c | 2 +- target/arm/translate.c | 2 +- target/hppa/translate.c | 2 +- target/i386/tcg/translate.c | 2 +- target/mips/tcg/lmmi_helper.c | 2 +- target/mips/tcg/msa_helper.c | 54 - target/ppc/arch_dump.c | 2 +- target/ppc/int_helper.c | 22 +- target/ppc/kvm.c | 4 +- target/ppc/mem_helper.c | 2 +- target/riscv/vector_helper.c | 2 +- target/s390x/tcg/translate.c | 2 +- target/sparc/vis_helper.c | 4 +- tcg/tcg-op.c | 4 +- tcg/tcg.c | 12 +++--- tests/qtest/vhost-user-blk-test.c | 2 +- tests/qtest/virtio-blk-test.c | 2 +- ui/vdagent.c | 2 +- ui/vnc.c | 2 +- util/bitmap.c | 2 +- util/host-utils.c | 2 +- target/ppc/translate/vmx-impl.c.inc | 4 +- target/ppc/translate/vsx-impl.c.inc | 2 +- target/riscv/insn_trans/trans_rvv.c.inc | 4 +- target/s390x/tcg/translate_vx.c.inc | 2 +- tcg/aarch64/tcg-target.c.inc | 4 +- tcg/arm/tcg-target.c.inc | 4 +- tcg/mips/tcg-target.c.inc | 2 +- tcg/ppc/tcg-target.c.inc | 10 ++--- tcg/riscv/tcg-target.c.inc | 4 +- 85 files changed, 173 insertions(+), 173 deletions(-) diff --git a/meson.build b/meson.build index f20712cb93d7..88df1bc42973 100644
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
On Wed, Mar 16, 2022 at 01:52:48PM +0400, marcandre.lur...@redhat.com wrote: > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > index 3baa5e3790f7..f2bd050e3b9a 100644 > --- a/include/qemu/compiler.h > +++ b/include/qemu/compiler.h > @@ -79,19 +79,12 @@ > #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ > sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > > -#if defined(__clang__) > -/* clang doesn't support gnu_printf, so use printf. */ > -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) > -#else > -/* Use gnu_printf (qemu uses standard format strings). */ > -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) > -# if defined(_WIN32) > +#if !defined(__clang__) && defined(_WIN32) > /* > * Map __printf__ to __gnu_printf__ because we want standard format strings > even > * when MinGW or GLib include files use __printf__. > */ > -# define __printf__ __gnu_printf__ > -# endif > +# define __printf__ __gnu_printf__ > #endif I'm not convinced we shold have this remaining define, even before your patch. For code we've implemented, we should have used __gnu_printf__ already if we know it uses GNU format on Windows. For code in GLib, its header file uses __gnu_printf__ for anything that relies on its portable printf replacement, which is basically everything in GLib. For anything else we should honour whatever they declare, and not assume their impl is the GNU one. I guess it is easy enough to validate by deleting this and seeing if we get any warnings from the mingw CI jobs about printf/gnu_printf mismatch. With 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 08/27] compiler.h: replace QEMU_WARN_UNUSED_RESULT with G_GNUC_WARN_UNUSED_RESULT
On 16/3/22 10:52, marcandre.lur...@redhat.com wrote: From: Marc-André Lureau One less qemu-specific macro. It also helps to make some headers/units only depend on glib, and thus moved in standalone projects eventually. Signed-off-by: Marc-André Lureau Reviewed-by: Richard Henderson --- include/qemu-common.h | 2 +- include/qemu/compiler.h| 2 -- include/qemu/range.h | 4 ++-- scripts/cocci-macro-file.h | 2 +- block/qcow2-refcount.c | 20 +++- scripts/checkpatch.pl | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) Preferably poisoning QEMU_WARN_UNUSED_RESULT: Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 4/6] vduse-blk: implements vduse-blk export
On Wed, Mar 16, 2022 at 8:16 PM Stefan Hajnoczi wrote: > > On Tue, Mar 15, 2022 at 07:52:03PM +0800, Yongji Xie wrote: > > On Tue, Mar 15, 2022 at 7:08 PM Stefan Hajnoczi wrote: > > > > > > On Tue, Feb 15, 2022 at 06:59:41PM +0800, Xie Yongji wrote: > > > > This implements a VDUSE block backends based on > > > > the libvduse library. We can use it to export the BDSs > > > > for both VM and container (host) usage. > > > > > > > > The new command-line syntax is: > > > > > > > > $ qemu-storage-daemon \ > > > > --blockdev file,node-name=drive0,filename=test.img \ > > > > --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on > > > > > > > > After the qemu-storage-daemon started, we need to use > > > > the "vdpa" command to attach the device to vDPA bus: > > > > > > > > $ vdpa dev add name vduse-export0 mgmtdev vduse > > > > > > The per-QEMU export id is used as the global vdpa device name. If this > > > becomes a problem in the future then a new --export > > > vduse-blk,vdpa-dev-name= option can be added. > > > > > > > Yes. > > > > > > +case VIRTIO_BLK_T_GET_ID: { > > > > +size_t size = MIN(iov_size(&elem->in_sg[0], in_num), > > > > + VIRTIO_BLK_ID_BYTES); > > > > +snprintf(elem->in_sg[0].iov_base, size, "%s", > > > > vblk_exp->export.id); > > > > > > Please use iov_from_buf(). The driver is allowed to submit as many > > > in_sg[] elements as it wants and a compliant virtio-blk device > > > implementation must support that. > > > > > > > Got it. > > > > > Here is the VIRTIO specification section that covers message framing: > > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-280004 > > > > > > > +features = (1ULL << VIRTIO_F_IOMMU_PLATFORM) | > > > > + (1ULL << VIRTIO_F_VERSION_1) | > > > > + (1ULL << VIRTIO_RING_F_EVENT_IDX) | > > > > + (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) | > > > > + (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | > > > > + (1ULL << VIRTIO_BLK_F_SIZE_MAX) | > > > > + (1ULL << VIRTIO_BLK_F_SEG_MAX) | > > > > + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | > > > > + (1ULL << VIRTIO_BLK_F_BLK_SIZE); > > > > > > The VIRTIO_F_ and VIRTIO_RING_F_ feature bits report the capabilities of > > > libvduse. They should probably be defined in libvduse. That way no > > > changes to vduse-blk.c are required when libvduse changes: > > > > > > features = LIBVDUSE_VIRTIO_FEATURES | > > > (1ULL << VIRTIO_BLK_F_SIZE_MAX) | > > > ...; > > > > It's OK to define the VIRTIO_F_* feature bits in libvduse since daemon > > might not want to disable it. But maybe not including VIRTIO_RING_F_* > > feature bits since daemon might want to disable them in some cases. > > The VIRTIO_RING_F_* functionality is implemented inside libvduse and not > the device code. If the device wants to disable specific features, it > can clear those bits. > > Thinking about the LIBVDUSE_VIRTIO_FEATURES idea I think it's slightly > better to make it a function so that libvduse code supports dynamic > linking. That way the device calls libvduse to query the feature bits > instead of compiling a constant from the libvduse header file into the > device executable. > > So something like: > > features = (vdu_get_virtio_features() & ~(...features we wish to clear...)) > | > (1ULL << VIRTIO_BLK_F_SIZE_MAX) | > ...; > OK, it looks good to me. Thanks, Yongji
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Tue, Mar 15, 2022 at 07:38:12PM +0800, Yongji Xie wrote: > On Tue, Mar 15, 2022 at 5:48 PM Stefan Hajnoczi wrote: > > > > On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > > > VDUSE [1] is a linux framework that makes it possible to implement > > > software-emulated vDPA devices in userspace. This adds a library > > > as a subproject to help implementing VDUSE backends in QEMU. > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > > > > > Signed-off-by: Xie Yongji > > > --- > > > meson.build | 15 + > > > meson_options.txt |2 + > > > scripts/meson-buildoptions.sh |3 + > > > subprojects/libvduse/include/atomic.h |1 + > > > subprojects/libvduse/libvduse.c | 1152 +++ > > > subprojects/libvduse/libvduse.h | 225 > > > subprojects/libvduse/linux-headers/linux|1 + > > > subprojects/libvduse/meson.build| 10 + > > > subprojects/libvduse/standard-headers/linux |1 + > > > 9 files changed, 1410 insertions(+) > > > create mode 12 subprojects/libvduse/include/atomic.h > > > create mode 100644 subprojects/libvduse/libvduse.c > > > create mode 100644 subprojects/libvduse/libvduse.h > > > create mode 12 subprojects/libvduse/linux-headers/linux > > > create mode 100644 subprojects/libvduse/meson.build > > > create mode 12 subprojects/libvduse/standard-headers/linux > > > > Please update the ./MAINTAINERS file when adding new source files. > > OK, sure. And would you mind being one of the maintainers since I'm > not sure if I can do this job well. You're welcome to become the maintainer. It means that you will be CCed on patches affecting this code and sometimes people might send you questions about VDUSE exports. Is the issue lack of time? Stefan signature.asc Description: PGP signature
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
Hi On Wed, Mar 16, 2022 at 5:30 PM Daniel P. Berrangé wrote: > > On Wed, Mar 16, 2022 at 01:52:48PM +0400, marcandre.lur...@redhat.com wrote: > > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > > index 3baa5e3790f7..f2bd050e3b9a 100644 > > --- a/include/qemu/compiler.h > > +++ b/include/qemu/compiler.h > > @@ -79,19 +79,12 @@ > > #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - \ > > sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > > > > -#if defined(__clang__) > > -/* clang doesn't support gnu_printf, so use printf. */ > > -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) > > -#else > > -/* Use gnu_printf (qemu uses standard format strings). */ > > -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) > > -# if defined(_WIN32) > > +#if !defined(__clang__) && defined(_WIN32) > > /* > > * Map __printf__ to __gnu_printf__ because we want standard format > > strings even > > * when MinGW or GLib include files use __printf__. > > */ > > -# define __printf__ __gnu_printf__ > > -# endif > > +# define __printf__ __gnu_printf__ > > #endif > > I'm not convinced we shold have this remaining define, even > before your patch. > > For code we've implemented, we should have used __gnu_printf__ > already if we know it uses GNU format on Windows. > > For code in GLib, its header file uses __gnu_printf__ for anything > that relies on its portable printf replacement, which is basically > everything in GLib. > > For anything else we should honour whatever they declare, and not > assume their impl is the GNU one. > > > I guess it is easy enough to validate by deleting this and seeing > if we get any warnings from the mingw CI jobs about printf/gnu_printf > mismatch. Please comment on that thread: https://patchew.org/QEMU/20220224183701.608720-1-marcandre.lur...@redhat.com/20220224183701.608720-6-marcandre.lur...@redhat.com/ > > With 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 :| >
[PATCH v3 2/3] util/main-loop: Introduce the main loop into QOM
'event-loop-base' provides basic property handling for all 'AioContext' based event loops. So let's define a new 'MainLoopClass' that inherits from it. This will permit tweaking the main loop's properties through qapi as well as through the command line using the '-object' keyword[1]. Only one instance of 'MainLoopClass' might be created at any time. 'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to mark 'MainLoop' as non-deletable. [1] For example: -object main-loop,id=main-loop,aio-max-batch= Signed-off-by: Nicolas Saenz Julienne Reviewed-by: Stefan Hajnoczi --- Changes since v2: - Fix mainloop's qapi versioning Changes since v1: - Fix json files to differentiate between iothread and main-loop - Use OBJECT_DECLARE_TYPE() - Fix build dependencies event-loop-base.c| 13 include/qemu/main-loop.h | 10 ++ include/sysemu/event-loop-base.h | 1 + meson.build | 3 +- qapi/qom.json| 15 + util/main-loop.c | 56 6 files changed, 97 insertions(+), 1 deletion(-) diff --git a/event-loop-base.c b/event-loop-base.c index a924c73a7c..e7f99a6ec8 100644 --- a/event-loop-base.c +++ b/event-loop-base.c @@ -73,10 +73,23 @@ static void event_loop_base_complete(UserCreatable *uc, Error **errp) } } +static bool event_loop_base_can_be_deleted(UserCreatable *uc) +{ +EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc); +EventLoopBase *backend = EVENT_LOOP_BASE(uc); + +if (bc->can_be_deleted) { +return bc->can_be_deleted(backend); +} + +return true; +} + static void event_loop_base_class_init(ObjectClass *klass, void *class_data) { UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass); ucc->complete = event_loop_base_complete; +ucc->can_be_deleted = event_loop_base_can_be_deleted; object_class_property_add(klass, "aio-max-batch", "int", event_loop_base_get_param, diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 7a4d6a0920..0aa36a4f17 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -26,9 +26,19 @@ #define QEMU_MAIN_LOOP_H #include "block/aio.h" +#include "qom/object.h" +#include "sysemu/event-loop-base.h" #define SIG_IPI SIGUSR1 +#define TYPE_MAIN_LOOP "main-loop" +OBJECT_DECLARE_TYPE(MainLoop, MainLoopClass, MAIN_LOOP) + +struct MainLoop { +EventLoopBase parent_obj; +}; +typedef struct MainLoop MainLoop; + /** * qemu_init_main_loop: Set up the process so that it can run the main loop. * diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h index 8e77d8b69f..fced4c9fea 100644 --- a/include/sysemu/event-loop-base.h +++ b/include/sysemu/event-loop-base.h @@ -25,6 +25,7 @@ struct EventLoopBaseClass { void (*init)(EventLoopBase *base, Error **errp); void (*update_params)(EventLoopBase *base, Error **errp); +bool (*can_be_deleted)(EventLoopBase *base); }; struct EventLoopBase { diff --git a/meson.build b/meson.build index 281c0691cc..2931904ffe 100644 --- a/meson.build +++ b/meson.build @@ -2769,7 +2769,8 @@ libqemuutil = static_library('qemuutil', sources: util_ss.sources() + stub_ss.sources() + genh, dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman]) qemuutil = declare_dependency(link_with: libqemuutil, - sources: genh + version_res) + sources: genh + version_res, + dependencies: [event_loop_base]) if have_system or have_user decodetree = generator(find_program('scripts/decodetree.py'), diff --git a/qapi/qom.json b/qapi/qom.json index eeb5395ff3..10800166e8 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -528,6 +528,19 @@ '*poll-shrink': 'int', '*aio-max-batch': 'int' } } +## +# @MainLoopProperties: +# +# Properties for the main-loop object. +# +# @aio-max-batch: maximum number of requests in a batch for the AIO engine, +# 0 means that the engine will use its default (default:0) +# +# Since: 7.1 +## +{ 'struct': 'MainLoopProperties', + 'data': { '*aio-max-batch': 'int' } } + ## # @MemoryBackendProperties: # @@ -818,6 +831,7 @@ { 'name': 'input-linux', 'if': 'CONFIG_LINUX' }, 'iothread', +'main-loop', { 'name': 'memory-backend-epc', 'if': 'CONFIG_LINUX' }, 'memory-backend-file', @@ -883,6 +897,7 @@ 'input-linux':{ 'type': 'InputLinuxProperties', 'if': 'CONFIG_LINUX' }, 'iothread': 'IothreadProperties', + 'main-loop': 'MainLoopProperties', 'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties', 'if': 'CONFIG_LINUX' },
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Wed, Mar 16, 2022 at 9:28 PM Stefan Hajnoczi wrote: > > On Tue, Mar 15, 2022 at 07:38:12PM +0800, Yongji Xie wrote: > > On Tue, Mar 15, 2022 at 5:48 PM Stefan Hajnoczi wrote: > > > > > > On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > > > > VDUSE [1] is a linux framework that makes it possible to implement > > > > software-emulated vDPA devices in userspace. This adds a library > > > > as a subproject to help implementing VDUSE backends in QEMU. > > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > > > > > > > Signed-off-by: Xie Yongji > > > > --- > > > > meson.build | 15 + > > > > meson_options.txt |2 + > > > > scripts/meson-buildoptions.sh |3 + > > > > subprojects/libvduse/include/atomic.h |1 + > > > > subprojects/libvduse/libvduse.c | 1152 +++ > > > > subprojects/libvduse/libvduse.h | 225 > > > > subprojects/libvduse/linux-headers/linux|1 + > > > > subprojects/libvduse/meson.build| 10 + > > > > subprojects/libvduse/standard-headers/linux |1 + > > > > 9 files changed, 1410 insertions(+) > > > > create mode 12 subprojects/libvduse/include/atomic.h > > > > create mode 100644 subprojects/libvduse/libvduse.c > > > > create mode 100644 subprojects/libvduse/libvduse.h > > > > create mode 12 subprojects/libvduse/linux-headers/linux > > > > create mode 100644 subprojects/libvduse/meson.build > > > > create mode 12 subprojects/libvduse/standard-headers/linux > > > > > > Please update the ./MAINTAINERS file when adding new source files. > > > > OK, sure. And would you mind being one of the maintainers since I'm > > not sure if I can do this job well. > > You're welcome to become the maintainer. It means that you will be CCed > on patches affecting this code and sometimes people might send you > questions about VDUSE exports. > I see. I will try my best. > Is the issue lack of time? > I think the time is enough. But since I have no experience, I'm not sure if I can do this well. Thanks, Yongji
[PATCH v3 1/3] Introduce event-loop-base abstract class
Introduce the 'event-loop-base' abstract class, it'll hold the properties common to all event loops and provide the necessary hooks for their creation and maintenance. Then have iothread inherit from it. EventLoopBaseClass is defined as user creatable and provides a hook for its children to attach themselves to the user creatable class 'complete' function. It also provides an update_params() callback to propagate property changes onto its children. The new 'event-loop-base' class will live in the root directory. It is built on its own using the 'link_whole' option (there are no direct function dependencies between the class and its children, it all happens trough 'constructor' magic). And also imposes new compilation dependencies: qom <- event-loop-base <- blockdev (iothread.c) And in subsequent patches: qom <- event-loop-base <- qemuutil (util/main-loop.c) All this forced some amount of reordering in meson.build: - Moved qom build definition before qemuutil. Doing it the other way around (i.e. moving qemuutil after qom) isn't possible as a lot of core libraries that live in between the two depend on it. - Process the 'hw' subdir earlier, as it introduces files into the 'qom' source set. No functional changes intended. Signed-off-by: Nicolas Saenz Julienne --- Changes since v2: - reword commit message to better explain compilation dependencies. Changes since v1: - Rename to event-loop-base - Move event-loop-base into root directory - Build event-loop-base on its own, use link_whole to avoid the problem of the object file not being linked due to lacking direct calls from dependencies. - Move poll parameters into iothread, as main loop can't poll - Update Authorship (I took what iothread.c had and added myself, I hope that's fine) - Introduce update_params() callback event-loop-base.c| 104 +++ include/sysemu/event-loop-base.h | 36 +++ include/sysemu/iothread.h| 6 +- iothread.c | 65 ++- meson.build | 23 --- 5 files changed, 175 insertions(+), 59 deletions(-) create mode 100644 event-loop-base.c create mode 100644 include/sysemu/event-loop-base.h diff --git a/event-loop-base.c b/event-loop-base.c new file mode 100644 index 00..a924c73a7c --- /dev/null +++ b/event-loop-base.c @@ -0,0 +1,104 @@ +/* + * QEMU event-loop base + * + * Copyright (C) 2022 Red Hat Inc + * + * Authors: + * Stefan Hajnoczi + * Nicolas Saenz Julienne + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qom/object_interfaces.h" +#include "qapi/error.h" +#include "sysemu/event-loop-base.h" + +typedef struct { +const char *name; +ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */ +} EventLoopBaseParamInfo; + +static EventLoopBaseParamInfo aio_max_batch_info = { +"aio-max-batch", offsetof(EventLoopBase, aio_max_batch), +}; + +static void event_loop_base_get_param(Object *obj, Visitor *v, +const char *name, void *opaque, Error **errp) +{ +EventLoopBase *event_loop_base = EVENT_LOOP_BASE(obj); +EventLoopBaseParamInfo *info = opaque; +int64_t *field = (void *)event_loop_base + info->offset; + +visit_type_int64(v, name, field, errp); +} + +static void event_loop_base_set_param(Object *obj, Visitor *v, +const char *name, void *opaque, Error **errp) +{ +EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(obj); +EventLoopBase *base = EVENT_LOOP_BASE(obj); +EventLoopBaseParamInfo *info = opaque; +int64_t *field = (void *)base + info->offset; +int64_t value; + +if (!visit_type_int64(v, name, &value, errp)) { +return; +} + +if (value < 0) { +error_setg(errp, "%s value must be in range [0, %" PRId64 "]", + info->name, INT64_MAX); +return; +} + +*field = value; + +if (bc->update_params) { +bc->update_params(base, errp); +} + +return; +} + +static void event_loop_base_complete(UserCreatable *uc, Error **errp) +{ +EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc); +EventLoopBase *base = EVENT_LOOP_BASE(uc); + +if (bc->init) { +bc->init(base, errp); +} +} + +static void event_loop_base_class_init(ObjectClass *klass, void *class_data) +{ +UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass); +ucc->complete = event_loop_base_complete; + +object_class_property_add(klass, "aio-max-batch", "int", + event_loop_base_get_param, + event_loop_base_set_param, + NULL, &aio_max_batch_info); +} + +static const TypeInfo event_loop_base_info = { +.name = TYPE_EVENT_LOOP_BASE, +.parent = TYPE_OBJECT, +.instance_size = sizeof(EventLoopBase), +.class_size =
[PATCH v3 0/3] util/thread-pool: Expose minimun and maximum size
As discussed on the previous RFC[1] the thread-pool's dynamic thread management doesn't play well with real-time and latency sensitive systems. This series introduces a set of controls that'll permit achieving more deterministic behaviours, for example by fixing the pool's size. We first introduce a new common interface to event loop configuration by moving iothread's already available properties into an abstract class called 'EventLooopBackend' and have both 'IOThread' and the newly created 'MainLoop' inherit the properties from that class. With this new configuration interface in place it's relatively simple to introduce new options to fix the even loop's thread pool sizes. The resulting QAPI looks like this: -object main-loop,id=main-loop,thread-pool-min=1,thread-pool-max=1 Note that all patches are bisect friendly and pass all the tests. [1] https://patchwork.ozlabs.org/project/qemu-devel/patch/20220202175234.656711-1-nsaen...@redhat.com/ --- Changes since v2: - Get rid of wrong locking/waiting - Fix qapi versioning - Better commit messages Changes since v1: - Address all Stefan's comments - Introduce new fix Nicolas Saenz Julienne (3): Introduce event-loop-base abstract class util/main-loop: Introduce the main loop into QOM util/event-loop-base: Introduce options to set the thread pool size event-loop-base.c| 140 +++ include/block/aio.h | 10 +++ include/block/thread-pool.h | 3 + include/qemu/main-loop.h | 10 +++ include/sysemu/event-loop-base.h | 41 + include/sysemu/iothread.h| 6 +- iothread.c | 68 +-- meson.build | 26 +++--- qapi/qom.json| 34 +++- util/aio-posix.c | 1 + util/async.c | 20 + util/main-loop.c | 65 ++ util/thread-pool.c | 55 +++- 13 files changed, 414 insertions(+), 65 deletions(-) create mode 100644 event-loop-base.c create mode 100644 include/sysemu/event-loop-base.h -- 2.35.1
[PATCH v3 3/3] util/event-loop-base: Introduce options to set the thread pool size
The thread pool regulates itself: when idle, it kills threads until empty, when in demand, it creates new threads until full. This behaviour doesn't play well with latency sensitive workloads where the price of creating a new thread is too high. For example, when paired with qemu's '-mlock', or using safety features like SafeStack, creating a new thread has been measured take multiple milliseconds. In order to mitigate this let's introduce a new 'EventLoopBase' property to set the thread pool size. The threads will be created during the pool's initialization or upon updating the property's value, remain available during its lifetime regardless of demand, and destroyed upon freeing it. A properly characterized workload will then be able to configure the pool to avoid any latency spikes. Signed-off-by: Nicolas Saenz Julienne --- Changes since v2: - Don't wait when decreasing pool size - Fix qapi versioning Changes since v1: - Add INT_MAX check - Have copy of thread pool sizes in AioContext to properly decouple both instances - More coherent variable naming - Handle case where max_threads decreases - Code comments event-loop-base.c| 23 + include/block/aio.h | 10 ++ include/block/thread-pool.h | 3 ++ include/sysemu/event-loop-base.h | 4 +++ iothread.c | 3 ++ qapi/qom.json| 21 ++-- util/aio-posix.c | 1 + util/async.c | 20 util/main-loop.c | 9 ++ util/thread-pool.c | 55 +--- 10 files changed, 143 insertions(+), 6 deletions(-) diff --git a/event-loop-base.c b/event-loop-base.c index e7f99a6ec8..d5be4dc6fc 100644 --- a/event-loop-base.c +++ b/event-loop-base.c @@ -14,6 +14,7 @@ #include "qemu/osdep.h" #include "qom/object_interfaces.h" #include "qapi/error.h" +#include "block/thread-pool.h" #include "sysemu/event-loop-base.h" typedef struct { @@ -21,9 +22,22 @@ typedef struct { ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */ } EventLoopBaseParamInfo; +static void event_loop_base_instance_init(Object *obj) +{ +EventLoopBase *base = EVENT_LOOP_BASE(obj); + +base->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT; +} + static EventLoopBaseParamInfo aio_max_batch_info = { "aio-max-batch", offsetof(EventLoopBase, aio_max_batch), }; +static EventLoopBaseParamInfo thread_pool_min_info = { +"thread-pool-min", offsetof(EventLoopBase, thread_pool_min), +}; +static EventLoopBaseParamInfo thread_pool_max_info = { +"thread-pool-max", offsetof(EventLoopBase, thread_pool_max), +}; static void event_loop_base_get_param(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) @@ -95,12 +109,21 @@ static void event_loop_base_class_init(ObjectClass *klass, void *class_data) event_loop_base_get_param, event_loop_base_set_param, NULL, &aio_max_batch_info); +object_class_property_add(klass, "thread-pool-min", "int", + event_loop_base_get_param, + event_loop_base_set_param, + NULL, &thread_pool_min_info); +object_class_property_add(klass, "thread-pool-max", "int", + event_loop_base_get_param, + event_loop_base_set_param, + NULL, &thread_pool_max_info); } static const TypeInfo event_loop_base_info = { .name = TYPE_EVENT_LOOP_BASE, .parent = TYPE_OBJECT, .instance_size = sizeof(EventLoopBase), +.instance_init = event_loop_base_instance_init, .class_size = sizeof(EventLoopBaseClass), .class_init = event_loop_base_class_init, .abstract = true, diff --git a/include/block/aio.h b/include/block/aio.h index 5634173b12..d128558f1d 100644 --- a/include/block/aio.h +++ b/include/block/aio.h @@ -192,6 +192,8 @@ struct AioContext { QSLIST_HEAD(, Coroutine) scheduled_coroutines; QEMUBH *co_schedule_bh; +int thread_pool_min; +int thread_pool_max; /* Thread pool for performing work and receiving completion callbacks. * Has its own locking. */ @@ -769,4 +771,12 @@ void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns, void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch, Error **errp); +/** + * aio_context_set_thread_pool_params: + * @ctx: the aio context + * @min: min number of threads to have readily available in the thread pool + * @min: max number of threads the thread pool can contain + */ +void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min, +int64_t max, Error **errp); #endif diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h index 7dd7d730a0.
Re: [PATCH v3 2/3] util/main-loop: Introduce the main loop into QOM
Nicolas Saenz Julienne writes: > 'event-loop-base' provides basic property handling for all 'AioContext' > based event loops. So let's define a new 'MainLoopClass' that inherits > from it. This will permit tweaking the main loop's properties through > qapi as well as through the command line using the '-object' keyword[1]. > Only one instance of 'MainLoopClass' might be created at any time. > > 'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to > mark 'MainLoop' as non-deletable. > > [1] For example: > -object main-loop,id=main-loop,aio-max-batch= > > Signed-off-by: Nicolas Saenz Julienne > Reviewed-by: Stefan Hajnoczi [...] > diff --git a/qapi/qom.json b/qapi/qom.json > index eeb5395ff3..10800166e8 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -528,6 +528,19 @@ > '*poll-shrink': 'int', > '*aio-max-batch': 'int' } } > > +## > +# @MainLoopProperties: > +# > +# Properties for the main-loop object. > +# > +# @aio-max-batch: maximum number of requests in a batch for the AIO engine, > +# 0 means that the engine will use its default (default:0) > +# > +# Since: 7.1 > +## > +{ 'struct': 'MainLoopProperties', > + 'data': { '*aio-max-batch': 'int' } } > + IothreadProperties has the same member, with the same documentation. Do main loop and iothreads have a common ancestor, conceptually? If yes, it might make sense for MainLoopProperties and IothreadProperties to have a common base type, and put @aio-max-batch there. This is not a demand. > ## > # @MemoryBackendProperties: > # > @@ -818,6 +831,7 @@ > { 'name': 'input-linux', >'if': 'CONFIG_LINUX' }, > 'iothread', > +'main-loop', > { 'name': 'memory-backend-epc', >'if': 'CONFIG_LINUX' }, > 'memory-backend-file', > @@ -883,6 +897,7 @@ >'input-linux':{ 'type': 'InputLinuxProperties', >'if': 'CONFIG_LINUX' }, >'iothread': 'IothreadProperties', > + 'main-loop': 'MainLoopProperties', >'memory-backend-epc': { 'type': 'MemoryBackendEpcProperties', >'if': 'CONFIG_LINUX' }, >'memory-backend-file':'MemoryBackendFileProperties', QAPI schema Acked-by: Markus Armbruster
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
Am 16/03/2022 um 13:53 schrieb Philippe Mathieu-Daudé: > On 16/3/22 13:44, Philippe Mathieu-Daudé wrote: >> Hi, >> >> On 4/3/22 17:46, Kevin Wolf wrote: >>> From: Emanuele Giuseppe Esposito >>> >>> All the global state (GS) API functions will check that >>> qemu_in_main_thread() returns true. If not, it means >>> that the safety of BQL cannot be guaranteed, and >>> they need to be moved to I/O. >> >> I'm getting this crash: >> >> $ qemu-system-i386 >> Assertion failed: (qemu_in_main_thread()), function blk_all_next, file >> block-backend.c, line 552. >> Abort trap: 6 >> >> Assertion failed: (qemu_in_main_thread()), function blk_all_next, file >> block-backend.c, line 552. >> qemu-system-i386 was compiled with optimization - stepping may behave >> oddly; variables may not be available. >> Process 76914 stopped >> * thread #1, queue = 'com.apple.main-thread', stop reason = hit >> program assert >> frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 >> at block-backend.c:552:5 [opt] >> 549 */ >> 550 BlockBackend *blk_all_next(BlockBackend *blk) >> 551 { >> -> 552 GLOBAL_STATE_CODE(); >> 553 return blk ? QTAILQ_NEXT(blk, link) >> 554 : QTAILQ_FIRST(&block_backends); >> 555 } >> Target 1: (qemu-system-i386) stopped. > > Forgot to paste the backtrace: > > (lldb) bt > * thread #1, queue = 'com.apple.main-thread', stop reason = hit program > assert > frame #0: 0x0001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8 > frame #1: 0x0001908fceb0 libsystem_pthread.dylib`pthread_kill + 288 > frame #2: 0x00019083a314 libsystem_c.dylib`abort + 164 > frame #3: 0x00019083972c libsystem_c.dylib`__assert_rtn + 300 > * frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at > block-backend.c:552:5 [opt] > frame #5: 0x0001003c00b4 > qemu-system-i386`blk_all_next(blk=) at > block-backend.c:552:5 [opt] > frame #6: 0x0001003d8f04 > qemu-system-i386`qmp_query_block(errp=0x) at > qapi.c:591:16 [opt] > frame #7: 0x00010003ab0c qemu-system-i386`main [inlined] > addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt] > frame #8: 0x00010003ab04 > qemu-system-i386`main(argc=, argv=) at > cocoa.m:1980:5 [opt] > frame #9: 0x0001012690f4 dyld`start + 520 I think Paolo and Peter talked about this a couple of days ago on #qemu, and have already found a solution if I remember correctly. Maybe it's worth to check with them first. Emanuele > >> Bisected to this patch: >> >> 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 is the first bad commit >> >>> Signed-off-by: Emanuele Giuseppe Esposito >>> Message-Id: <20220303151616.325444-9-eespo...@redhat.com> >>> Signed-off-by: Kevin Wolf >>> --- >>> block/block-backend.c | 78 ++ >>> softmmu/qdev-monitor.c | 2 ++ >>> 2 files changed, 80 insertions(+) >
Re: [PATCH v2] block-qdict: Fix -Werror=maybe-uninitialized build failure
Murilo Opsfelder Araújo writes: > Hi, Philippe. > > On Monday, March 14, 2022 10:47:11 AM -03 Philippe Mathieu-Daudé wrote: >> On 11/3/22 23:16, Murilo Opsfelder Araujo wrote: >> > Building QEMU on Fedora 37 (Rawhide Prerelease) ppc64le failed with the >> > following error: >> > >> > $ ../configure --prefix=/usr/local/qemu-disabletcg >> > --target-list=ppc-softmmu,ppc64-softmmu --disable-tcg --disable-linux-user >> > ... >> > $ make -j$(nproc) >> > ... >> > FAILED: libqemuutil.a.p/qobject_block-qdict.c.o >> >> This part >>> >> >> > cc -m64 -mlittle-endian -Ilibqemuutil.a.p -I. -I.. >> > -Isubprojects/libvhost-user -I../subprojects/libvhost-user -Iqapi -Itrace >> > -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include >> > -I/usr/include/sysprof-4 -I/usr/include/lib >> > mount -I/usr/include/blkid -I/usr/include/gio-unix-2.0 >> > -I/usr/include/p11-kit-1 -I/usr/include/pixman-1 -fdiagnostics-color=auto >> > -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem >> > /root/qemu/linux-headers -isystem linux-headers -iquote >> > . -iquote /root/qemu -iquote /root/qemu/include -iquote >> > /root/qemu/disas/libvixl -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 >> > -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE >> > -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite >> > -strings -Wmissing-prototypes -fno-strict-aliasing -fno-common >> > -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits >> > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers >> > -Wempty-body -Wnested-externs -Wendif-label >> > s -Wexpansion-to-defined -Wimplicit-fallthrough=2 >> > -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi >> > -fstack-protector-strong -fPIE -MD -MQ >> > libqemuutil.a.p/qobject_block-qdict.c.o -MF >> > libqemuutil.a.p/qobject_block-qdict.c.o.d - >> > o libqemuutil.a.p/qobject_block-qdict.c.o -c ../qobject/block-qdict.c >> >> <<< is noise (doesn't provide any value) and could be stripped. > > Is this something the committer/maintainer could edit when applying the commit > or do you need I resend the v3? > > Cheers! I'll take care of it unless Kevin or Hanna beat me to the punch.
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
On 16/3/22 15:46, Emanuele Giuseppe Esposito wrote: Am 16/03/2022 um 13:53 schrieb Philippe Mathieu-Daudé: On 16/3/22 13:44, Philippe Mathieu-Daudé wrote: Hi, On 4/3/22 17:46, Kevin Wolf wrote: From: Emanuele Giuseppe Esposito All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. I'm getting this crash: $ qemu-system-i386 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. Abort trap: 6 Assertion failed: (qemu_in_main_thread()), function blk_all_next, file block-backend.c, line 552. qemu-system-i386 was compiled with optimization - stepping may behave oddly; variables may not be available. Process 76914 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt] 549 */ 550 BlockBackend *blk_all_next(BlockBackend *blk) 551 { -> 552 GLOBAL_STATE_CODE(); 553 return blk ? QTAILQ_NEXT(blk, link) 554 : QTAILQ_FIRST(&block_backends); 555 } Target 1: (qemu-system-i386) stopped. Forgot to paste the backtrace: (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert frame #0: 0x0001908c99b8 libsystem_kernel.dylib`__pthread_kill + 8 frame #1: 0x0001908fceb0 libsystem_pthread.dylib`pthread_kill + 288 frame #2: 0x00019083a314 libsystem_c.dylib`abort + 164 frame #3: 0x00019083972c libsystem_c.dylib`__assert_rtn + 300 * frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at block-backend.c:552:5 [opt] frame #5: 0x0001003c00b4 qemu-system-i386`blk_all_next(blk=) at block-backend.c:552:5 [opt] frame #6: 0x0001003d8f04 qemu-system-i386`qmp_query_block(errp=0x) at qapi.c:591:16 [opt] frame #7: 0x00010003ab0c qemu-system-i386`main [inlined] addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt] frame #8: 0x00010003ab04 qemu-system-i386`main(argc=, argv=) at cocoa.m:1980:5 [opt] frame #9: 0x0001012690f4 dyld`start + 520 I think Paolo and Peter talked about this a couple of days ago on #qemu, and have already found a solution if I remember correctly. Maybe it's worth to check with them first. Maybe this discussion? https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonz...@redhat.com/ Emanuele Bisected to this patch: 0439c5a4623d674efa0c72abd62ca6e98bb7cf87 is the first bad commit Signed-off-by: Emanuele Giuseppe Esposito Message-Id: <20220303151616.325444-9-eespo...@redhat.com> Signed-off-by: Kevin Wolf --- block/block-backend.c | 78 ++ softmmu/qdev-monitor.c | 2 ++ 2 files changed, 80 insertions(+)
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Wed, Mar 16, 2022 at 09:49:19PM +0800, Yongji Xie wrote: > On Wed, Mar 16, 2022 at 9:28 PM Stefan Hajnoczi wrote: > > > > On Tue, Mar 15, 2022 at 07:38:12PM +0800, Yongji Xie wrote: > > > On Tue, Mar 15, 2022 at 5:48 PM Stefan Hajnoczi > > > wrote: > > > > > > > > On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > > > > > VDUSE [1] is a linux framework that makes it possible to implement > > > > > software-emulated vDPA devices in userspace. This adds a library > > > > > as a subproject to help implementing VDUSE backends in QEMU. > > > > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > > > > > > > > > Signed-off-by: Xie Yongji > > > > > --- > > > > > meson.build | 15 + > > > > > meson_options.txt |2 + > > > > > scripts/meson-buildoptions.sh |3 + > > > > > subprojects/libvduse/include/atomic.h |1 + > > > > > subprojects/libvduse/libvduse.c | 1152 > > > > > +++ > > > > > subprojects/libvduse/libvduse.h | 225 > > > > > subprojects/libvduse/linux-headers/linux|1 + > > > > > subprojects/libvduse/meson.build| 10 + > > > > > subprojects/libvduse/standard-headers/linux |1 + > > > > > 9 files changed, 1410 insertions(+) > > > > > create mode 12 subprojects/libvduse/include/atomic.h > > > > > create mode 100644 subprojects/libvduse/libvduse.c > > > > > create mode 100644 subprojects/libvduse/libvduse.h > > > > > create mode 12 subprojects/libvduse/linux-headers/linux > > > > > create mode 100644 subprojects/libvduse/meson.build > > > > > create mode 12 subprojects/libvduse/standard-headers/linux > > > > > > > > Please update the ./MAINTAINERS file when adding new source files. > > > > > > OK, sure. And would you mind being one of the maintainers since I'm > > > not sure if I can do this job well. > > > > You're welcome to become the maintainer. It means that you will be CCed > > on patches affecting this code and sometimes people might send you > > questions about VDUSE exports. > > > > I see. I will try my best. > > > Is the issue lack of time? > > > > I think the time is enough. But since I have no experience, I'm not > sure if I can do this well. Great! Don't worry there will probably not be much you need to do. If someone submits a patch they'll CC you and you can post your "Reviewed-by:". Kevin or I can pick up the patches you've reviewed and send them with our pull requests to the qemu.git maintainer. Stefan signature.asc Description: PGP signature
Re: [PULL 21/50] block/block-backend.c: assertions for block-backend
Am 16.03.2022 um 16:25 hat Philippe Mathieu-Daudé geschrieben: > On 16/3/22 15:46, Emanuele Giuseppe Esposito wrote: > > Am 16/03/2022 um 13:53 schrieb Philippe Mathieu-Daudé: > > > On 16/3/22 13:44, Philippe Mathieu-Daudé wrote: > > > > Hi, > > > > > > > > On 4/3/22 17:46, Kevin Wolf wrote: > > > > > From: Emanuele Giuseppe Esposito > > > > > > > > > > All the global state (GS) API functions will check that > > > > > qemu_in_main_thread() returns true. If not, it means > > > > > that the safety of BQL cannot be guaranteed, and > > > > > they need to be moved to I/O. > > > > > > > > I'm getting this crash: > > > > > > > > $ qemu-system-i386 > > > > Assertion failed: (qemu_in_main_thread()), function blk_all_next, file > > > > block-backend.c, line 552. > > > > Abort trap: 6 > > > > > > > > Assertion failed: (qemu_in_main_thread()), function blk_all_next, file > > > > block-backend.c, line 552. > > > > qemu-system-i386 was compiled with optimization - stepping may behave > > > > oddly; variables may not be available. > > > > Process 76914 stopped > > > > * thread #1, queue = 'com.apple.main-thread', stop reason = hit > > > > program assert > > > > frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 > > > > at block-backend.c:552:5 [opt] > > > > 549 */ > > > > 550 BlockBackend *blk_all_next(BlockBackend *blk) > > > > 551 { > > > > -> 552 GLOBAL_STATE_CODE(); > > > > 553 return blk ? QTAILQ_NEXT(blk, link) > > > > 554 : QTAILQ_FIRST(&block_backends); > > > > 555 } > > > > Target 1: (qemu-system-i386) stopped. > > > > > > Forgot to paste the backtrace: > > > > > > (lldb) bt > > > * thread #1, queue = 'com.apple.main-thread', stop reason = hit program > > > assert > > > frame #0: 0x0001908c99b8 libsystem_kernel.dylib`__pthread_kill + > > > 8 > > > frame #1: 0x0001908fceb0 libsystem_pthread.dylib`pthread_kill + > > > 288 > > > frame #2: 0x00019083a314 libsystem_c.dylib`abort + 164 > > > frame #3: 0x00019083972c libsystem_c.dylib`__assert_rtn + 300 > > > * frame #4: 0x00010057c2d4 qemu-system-i386`blk_all_next.cold.1 at > > > block-backend.c:552:5 [opt] > > > frame #5: 0x0001003c00b4 > > > qemu-system-i386`blk_all_next(blk=) at > > > block-backend.c:552:5 [opt] > > > frame #6: 0x0001003d8f04 > > > qemu-system-i386`qmp_query_block(errp=0x) at > > > qapi.c:591:16 [opt] > > > frame #7: 0x00010003ab0c qemu-system-i386`main [inlined] > > > addRemovableDevicesMenuItems at cocoa.m:1756:21 [opt] > > > frame #8: 0x00010003ab04 > > > qemu-system-i386`main(argc=, argv=) at > > > cocoa.m:1980:5 [opt] > > > frame #9: 0x0001012690f4 dyld`start + 520 > > > > I think Paolo and Peter talked about this a couple of days ago on #qemu, > > and have already found a solution if I remember correctly. > > > > Maybe it's worth to check with them first. > > Maybe this discussion? > > https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonz...@redhat.com/ Yes, this looks like the right one. Can you give Paolo's patch a try? The problem is that the main thread didn't hold the BQL while calling code that requires holding the BQL and that now asserts that the BQL is held by the thread it's called from. Kevin
Re: [PATCH experiment 00/16] C++20 coroutine backend
Am 16.03.2022 um 13:40 hat Stefan Hajnoczi geschrieben: > On Wed, Mar 16, 2022 at 12:08:33AM +0100, Paolo Bonzini wrote: > > On 3/15/22 16:55, Daniel P. Berrangé wrote: > > > Expecting maintainers to enforce a subset during code review feels > > > like it would be a tedious burden, that will inevitably let stuff > > > through because humans are fallible, especially when presented > > > with uninspiring, tedious, repetitive tasks. > > > > > > Restricting ourselves to a subset is only viable if we have > > > an automated tool that can reliably enforce that subset. I'm not > > > sure that any such tool exists, and not convinced our time is > > > best served by trying to write & maintainer one either. > > > > We don't need to have a policy on which features are used. We need to have > > goals for what to use C++ for. I won't go into further details here, > > because I had already posted "When and how to use C++"[1] about an hour > > before your reply. > > > > > IOW, I fear one we allow C++ in any level, it won't be practical > > > to constrain it as much we desire. I fear us turning QEMU into > > > even more of a monster like other big C++ apps I see which take > > > all hours to compile while using all available RAM in Fedora RPM > > > build hosts. > > > > Sorry but this is FUD. There's plenty of C++ apps and libraries that do not > > "take hours to compile while using all available RAM". You're probably > > thinking of the Chromium/Firefox/Libreoffice triplet but those are an order > > of magnitude larger than QEMU. And in fact, QEMU is *already* a monster > > that takes longer to compile than most other packages, no matter the > > language they're written in. > > > > Most of KDE and everything that uses Qt is written in C++, and so is > > Inkscape in GTK+ land. LLVM and Clang are written in C++. Hotspot and V8 > > are written in C++. Kodi, MAME and DolphinEmu are written in C++. GCC and > > GDB have migrated to C++ and their compile times have not exploded. > > > > > My other question is whether adoption of C++ would complicate any > > > desire to make more use of Rust in QEMU ? I know Rust came out of > > > work by the Mozilla Firefox crew, and Firefox was C++, but I don't > > > have any idea how they integrated use of Rust with Firefox, so > > > whether there are any gotcha's for us or not ? > > > > Any Rust integration would go through C APIs. Using Rust in the block layer > > would certainly be much harder, though perhaps not impossible, if the block > > layer uses C++ coroutines. Rust supports something similar, but > > two-direction interoperability would be hard. > > I haven't looked at this in depth but there is a solution for Rust-C++ > interop: https://cxx.rs/ "Direct FFI of async functions is absolutely in scope for CXX (on C++20 and up) but is not implemented yet in the current release." With the current QEMU coroutines, calling Rust async fns from C is relatively easy, and calling C coroutine_fns from a Rust async fn is trivial when the Rust async fn is already called from a C coroutine (because qemu_coroutine_yield() just works, we still have a coroutine stack from the original C caller). I suppose calling Rust async fns from C++ could actually have the same implementation as with C when using Paolo's wrappers, but the other direction might be a bit harder - to be honest, I can't tell because I've never checked how C++ coroutines work internally. Could as well be that a Rust wrapper for them isn't that hard after all. Kevin signature.asc Description: PGP signature
Re: [PATCH experiment 00/16] C++20 coroutine backend
On Wed, Mar 16, 2022 at 01:06:02PM +, Daniel P. Berrangé wrote: > On Wed, Mar 16, 2022 at 12:32:48PM +, Stefan Hajnoczi wrote: > > On Tue, Mar 15, 2022 at 06:29:50PM +0100, Paolo Bonzini wrote: > > > On 3/15/22 15:24, Peter Maydell wrote: > > > > On Tue, 15 Mar 2022 at 14:09, Stefan Hajnoczi > > > > wrote: > > > > > Also, once C++ is available people will > > > > > start submitting C++ patches simply because they are more comfortable > > > > > with C++ (especially one-time/infrequent contributors). > > > > > > > > This to my mind is the major argument against using C++ > > > > for coroutines... > > > > > > I agree on the need for a policy, but _what_ C++ are they going to be > > > contributing that we should be scared of? We're talking about: > > > > > > * major features contributed by one-time/infrequent participants (which is > > > already a once-in-a-year thing or so, at least for me) > > > > > > * ... in an area where there are no examples of using C++ in the tree (or > > > presumably the maintainer would be comfortable reviewing it) > > > > > > * ... but yet C++ offer killer features (right now there's only C++ > > > coroutines and fpu/) > > > > You are assuming people only choose C++ only when it offers features not > > available in C. I think they might simply be more comfortable in C++. > > > > In other words, if an existing file is compiled using a C++ compiler or > > they are adding a new file, they don't need a reason to use C++, they > > can just use it. > > > > You can define rules and a way to enforce a subset of C++, but I think > > over time the code will be C++. A policy that is complicated discourages > > contributors. > > > > For these reasons I think that if code runs through a C++ compiler we > > should just allow C++. Either way, it will take time but that way no one > > will feel betrayed when C++ creeps in. > > > > That said, I hope we find an option that doesn't involve C++. > > The real show stopper with our current coroutine impl IIUC, is the > undefined behaviour when we yield and restore across different threads. > > Is there any relastic hope that we can change QEMU's usage, such that > each coroutine is confined to a single thread, to avoid the undefined > behaviour ? I don't think so. At the point where a coroutine stops executing in the vCPU thread the call stack is too deep. The benefit of coroutines would be lost because it would be necessary to use callback functions (BHs). Fixes that paper over the undefined behavior have been merged so the bugs should be kept at bay for a while. In theory we can continue with stack-based coroutines but we're likely to hit issues again in the future. Paolo also prototyped C stackless coroutines. That would require a source-to-source translation that converts a coroutine_fn into a Duff's device state machine. That solution doesn't require C++ but it would be necessary to develop the source-to-source translator and maintain it. Finally it may be possible to get coroutine in C from clang/gcc. They have the machinery to do it for C++ so maybe they could also offer it in C compiler mode. It would be great to have coroutines available in the toolchain - more reliable and supported than if we roll our own. Stefan signature.asc Description: PGP signature
Re: [PATCH 07/27] Replace GCC_FMT_ATTR with G_GNUC_PRINTF
Hi On Wed, Mar 16, 2022 at 5:28 PM Thomas Huth wrote: > > On 16/03/2022 14.16, Philippe Mathieu-Daudé wrote: > > On 16/3/22 10:52, marcandre.lur...@redhat.com wrote: > >> From: Marc-André Lureau > >> > >> One less qemu-specific macro. It also helps to make some headers/units > >> only depend on glib, and thus moved in standalone projects eventually. > >> > >> Signed-off-by: Marc-André Lureau > >> --- > >> audio/audio.h | 4 +-- > >> block/qcow2.h | 2 +- > >> bsd-user/qemu.h | 2 +- > >> hw/display/qxl.h| 2 +- > >> hw/net/rocker/rocker.h | 2 +- > >> hw/xen/xen_pt.h | 2 +- > >> include/chardev/char-fe.h | 2 +- > >> include/disas/dis-asm.h | 2 +- > >> include/hw/acpi/aml-build.h | 12 +++ > >> include/hw/core/cpu.h | 2 +- > >> include/hw/hw.h | 2 +- > >> include/hw/virtio/virtio.h | 2 +- > >> include/hw/xen/xen-bus-helper.h | 4 +-- > >> include/hw/xen/xen-bus.h| 4 +-- > >> include/hw/xen/xen_common.h | 2 +- > >> include/hw/xen/xen_pvdev.h | 2 +- > >> include/monitor/monitor.h | 4 +-- > >> include/qapi/error.h| 20 ++-- > >> include/qapi/qmp/qjson.h| 8 ++--- > >> include/qemu/buffer.h | 2 +- > >> include/qemu/compiler.h | 11 ++- > >> include/qemu/error-report.h | 24 +++--- > >> include/qemu/log-for-trace.h| 2 +- > >> include/qemu/log.h | 2 +- > >> include/qemu/qemu-print.h | 8 ++--- > >> include/qemu/readline.h | 2 +- > >> qga/guest-agent-core.h | 2 +- > >> qga/vss-win32/requester.h | 2 +- > >> scripts/cocci-macro-file.h | 2 +- > >> tests/qtest/libqos/libqtest.h | 42 - > >> tests/qtest/libqtest-single.h | 2 +- > >> tests/qtest/migration-helpers.h | 6 ++-- > >> audio/alsaaudio.c | 4 +-- > >> audio/dsoundaudio.c | 4 +-- > >> audio/ossaudio.c| 4 +-- > >> audio/paaudio.c | 2 +- > >> audio/sdlaudio.c| 2 +- > >> block/blkverify.c | 2 +- > >> block/ssh.c | 4 +-- > >> fsdev/9p-marshal.c | 2 +- > >> fsdev/virtfs-proxy-helper.c | 2 +- > >> hw/9pfs/9p.c| 2 +- > >> hw/acpi/aml-build.c | 4 +-- > >> hw/mips/fuloong2e.c | 2 +- > >> hw/mips/malta.c | 2 +- > >> hw/net/rtl8139.c| 2 +- > >> hw/virtio/virtio.c | 2 +- > >> io/channel-websock.c| 2 +- > >> monitor/hmp.c | 4 +-- > >> nbd/server.c| 10 +++--- > >> qemu-img.c | 4 +-- > >> qemu-io.c | 2 +- > >> qobject/json-parser.c | 2 +- > >> softmmu/qtest.c | 4 +-- > >> tests/qtest/libqtest.c | 2 +- > >> tests/unit/test-qobject-input-visitor.c | 4 +-- > >> audio/coreaudio.m | 4 +-- > >> scripts/checkpatch.pl | 2 +- > >> 58 files changed, 130 insertions(+), 137 deletions(-) > > > >> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h > >> index 3baa5e3790f7..f2bd050e3b9a 100644 > >> --- a/include/qemu/compiler.h > >> +++ b/include/qemu/compiler.h > >> @@ -79,19 +79,12 @@ > >> #define QEMU_BUILD_BUG_ON_ZERO(x) (sizeof(QEMU_BUILD_BUG_ON_STRUCT(x)) - > >> \ > >> sizeof(QEMU_BUILD_BUG_ON_STRUCT(x))) > >> -#if defined(__clang__) > >> -/* clang doesn't support gnu_printf, so use printf. */ > >> -# define GCC_FMT_ATTR(n, m) __attribute__((format(printf, n, m))) > >> -#else > >> -/* Use gnu_printf (qemu uses standard format strings). */ > >> -# define GCC_FMT_ATTR(n, m) __attribute__((format(gnu_printf, n, m))) > >> -# if defined(_WIN32) > >> +#if !defined(__clang__) && defined(_WIN32) > >> /* > >>* Map __printf__ to __gnu_printf__ because we want standard format > >> strings even > >>* when MinGW or GLib include files use __printf__. > >>*/ > >> -# define __printf__ __gnu_printf__ > >> -# endif > >> +# define __printf__ __gnu_printf__ > >> #endif > > > > Can we also poison GCC_FMT_ATTR? Maybe split in 2 patches, 1 converting > > and another removing unused & poisoning? > > I don't think that poisoning is required here since this macro is not used >
Re: [PATCH v3 3/3] nbd/server: Allow MULTI_CONN for shared writable exports
On Tue, Mar 15, 2022 at 01:14:41PM +, Richard W.M. Jones wrote: > The patches seem OK to me, but I don't really know enough about the > internals of qemu-nbd to give a line-by-line review. I did however > build and test qemu-nbd with the patches: > > $ ./build/qemu-nbd /var/tmp/test.qcow2 > $ nbdinfo nbd://localhost > ... > can_multi_conn: false > > > $ ./build/qemu-nbd -e 2 /var/tmp/test.qcow2 > $ nbdinfo nbd://localhost > ... > can_multi_conn: false > > ^^^ Is this expected? It also happens with -e 0. Yes, because qemu-nbd defaults to read-write connections, but to be conservative, this patch defaults '-m auto' to NOT advertise multi-conn for read-write; you need to be explicit: > > > $ ./build/qemu-nbd -e 2 -m on /var/tmp/test.qcow2 > $ nbdinfo nbd://localhost > ... > can_multi_conn: true either with '-m on' as you did here, or with build/qemu-nbd -r -e 2 /var/tmp/test.qcow2 where the '-m auto' default exposes multi-conn for a readonly client. > > > $ ./build/qemu-nbd -e 2 -m off /var/tmp/test.qcow2 > $ nbdinfo nbd://localhost > ... > can_multi_conn: false > > > Rich. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 3/3] nbd/server: Allow MULTI_CONN for shared writable exports
On Wed, Mar 16, 2022 at 04:07:21PM -0500, Eric Blake wrote: > On Tue, Mar 15, 2022 at 01:14:41PM +, Richard W.M. Jones wrote: > > The patches seem OK to me, but I don't really know enough about the > > internals of qemu-nbd to give a line-by-line review. I did however > > build and test qemu-nbd with the patches: > > > > $ ./build/qemu-nbd /var/tmp/test.qcow2 > > $ nbdinfo nbd://localhost > > ... > > can_multi_conn: false > > > > > > $ ./build/qemu-nbd -e 2 /var/tmp/test.qcow2 > > $ nbdinfo nbd://localhost > > ... > > can_multi_conn: false > > > > ^^^ Is this expected? It also happens with -e 0. > > Yes, because qemu-nbd defaults to read-write connections, but to be > conservative, this patch defaults '-m auto' to NOT advertise > multi-conn for read-write; you need to be explicit: > > > > > > > $ ./build/qemu-nbd -e 2 -m on /var/tmp/test.qcow2 > > $ nbdinfo nbd://localhost > > ... > > can_multi_conn: true > > either with '-m on' as you did here, or with > > build/qemu-nbd -r -e 2 /var/tmp/test.qcow2 > > where the '-m auto' default exposes multi-conn for a readonly client. I hit send prematurely - one more thought I wanted to make clear. For _this_ series, the behavior of '-m auto' depends solely on readonly vs. read-write; that being the most conservative choice of preserving pre-existing qemu-nbd behavior (that is, if you don't use -m, the only change in behavior should be the fact that --help output now lists -m). But in future versions of qemu, we might be able to improve '-m auto' to also take into consideration whether the backing image of a read-write device is known-cache-consistent (such as a local file system), where we can then manage to default to advertising multi-conn for those writable images, while still avoiding the advertisement when exposing other devices such as network-mounted devices where we are less confident on whether there are sufficient cache consistency guarantees. Use of explicit '-m off' or '-m on' gives guaranteed results no matter the qemu version, so it is only explicit (or implied) '-m auto' where we might get smarter defaults over time. Thus testing whether advertising multi-conn makes a difference in other tools like nbdcopy thus requires checking if qemu-nbd is new enough to support -m, and then being explicit that we are opting in to using it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 1/3] qapi: rename BlockDirtyBitmapMergeSource to BlockDirtyBitmapOrStr
On Tue, Mar 15, 2022 at 12:32:24AM +0300, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy > > Rename the type to be reused. Old name is "what is it for". To be > natively reused for other needs, let's name it exactly "what is it". > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/monitor/bitmap-qmp-cmds.c| 6 +++--- > include/block/block_int-global-state.h | 2 +- > qapi/block-core.json | 6 +++--- > qemu-img.c | 8 > 4 files changed, 11 insertions(+), 11 deletions(-) The type name does not affect QAPI interface stability, so this is safe. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair
On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy > > Hi all! Current logic of relying on search through backing chain is not > safe neither convenient. > > Sometimes it leads to necessity of extra bitmap copying. Also, we are > going to add "snapshot-access" driver, to access some snapshot state > through NBD. And this driver is not formally a filter, and of course > it's not a COW format driver. So, searching through backing chain will > not work. Instead of widening the workaround of bitmap searching, let's > extend the interface so that user can select bitmap precisely. > > Note, that checking for bitmap active status is not copied to the new > API, I don't see a reason for it, user should understand the risks. And > anyway, bitmap from other node is unrelated to this export being > read-only or read-write. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > blockdev-nbd.c | 8 +- > nbd/server.c | 63 +++--- > qapi/block-export.json | 5 +++- > qemu-nbd.c | 11 ++-- > 4 files changed, 61 insertions(+), 26 deletions(-) > > @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, > BlockExportOptions *exp_args, > } > exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps); > for (i = 0, bitmaps = arg->bitmaps; bitmaps; > - i++, bitmaps = bitmaps->next) { > -const char *bitmap = bitmaps->value; > + i++, bitmaps = bitmaps->next) > +{ > +const char *bitmap; I'm not sure if our prevailing style splits { to its own line on a multi-line 'for'. But this is a cosmetic question, not one of correctness. > +case QTYPE_QDICT: > +bitmap = bitmaps->value->u.external.name; > +bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node, > + bitmap, NULL, errp); > +if (!bm) { > +ret = -ENOENT; > +goto fail; > +} > +break; > +default: > +abort(); Not sure if g_assert_not_reached() or __builtin_unreachable() would be any better here. I'm fine with the abort() for now. > +++ b/qapi/block-export.json > @@ -6,6 +6,7 @@ > ## > > { 'include': 'sockets.json' } > +{ 'include': 'block-core.json' } Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, since that is why we created block-export.json in the first place (to minimize the stuff that qsd pulled in without needing all of block-core.json)? In other words, would it be better to move BlockDirtyBitmapOrStr to this file? Everything else looks okay with this patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2] MAINTAINERS: change Vladimir's email address
On Wed, Mar 16, 2022 at 12:27:02PM +0300, Vladimir Sementsov-Ogievskiy wrote: > Old vsement...@virtuozzo.com is not accessible anymore. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > v2: @ya.ru mailbox works bad with mailing lists and git send-email > command, @mail.ru works normally. > > Probably, I'll have to change the email again in the near future. May be > not. But I think it worth to change it now to something that works. Same comment as with your attempt with @ya.ru - I'm happy to incorporate this through my NBD tree, but want to confirm that we had a round-trip conversation so that you are happy with the address working to your needs. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept
Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito: >> Next, I have a problem in mind, that in past lead to a lot of iotest 30 >> failures. Next there were different fixes and improvements, but the core >> problem (as far as I understand) is still here: nothing protects us when >> we are in some graph modification process (for example block-job >> finalization) do yield, switch to other coroutine and enter another >> graph modification process (for example, another block-job finaliztion).. > That's another point to consider. I don't really have a solution for this. > On a side note, that might not be a real problem. If I understand correctly, your fear is that we are doing something like parent->children[x] = new_node // partial graph operation /* yield to another coroutine */ coroutine reads/writes parent->children[x] and/or new_node->parents[y] /* yield back */ new_node->parents[y] = parent // end of the initial graph operation Is that what you are pointing out here? If so, is there a concrete example for this? Because yields and drains (that eventually can poll) seem to be put either before or after the whole graph modification section. In other words, even if a coroutine enters, it will be always before or after the _whole_ graph modification is performed. Thank you, Emanuele
Re: [PATCH v2 3/3] iotests/223: check new possibility of exporting bitmaps by node/name
On Tue, Mar 15, 2022 at 12:32:26AM +0300, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy > > Add simple test that new interface introduced in previous commit works. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > tests/qemu-iotests/223 | 16 + > tests/qemu-iotests/223.out | 47 -- > 2 files changed, 61 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 3/3] nbd/server: Allow MULTI_CONN for shared writable exports
On Wed, Mar 16, 2022 at 04:15:53PM -0500, Eric Blake wrote: > On Wed, Mar 16, 2022 at 04:07:21PM -0500, Eric Blake wrote: > > On Tue, Mar 15, 2022 at 01:14:41PM +, Richard W.M. Jones wrote: > > > The patches seem OK to me, but I don't really know enough about the > > > internals of qemu-nbd to give a line-by-line review. I did however > > > build and test qemu-nbd with the patches: > > > > > > $ ./build/qemu-nbd /var/tmp/test.qcow2 > > > $ nbdinfo nbd://localhost > > > ... > > > can_multi_conn: false > > > > > > > > > $ ./build/qemu-nbd -e 2 /var/tmp/test.qcow2 > > > $ nbdinfo nbd://localhost > > > ... > > > can_multi_conn: false > > > > > > ^^^ Is this expected? It also happens with -e 0. > > > > Yes, because qemu-nbd defaults to read-write connections, but to be > > conservative, this patch defaults '-m auto' to NOT advertise > > multi-conn for read-write; you need to be explicit: > > > > > > > > > > > $ ./build/qemu-nbd -e 2 -m on /var/tmp/test.qcow2 > > > $ nbdinfo nbd://localhost > > > ... > > > can_multi_conn: true > > > > either with '-m on' as you did here, or with > > > > build/qemu-nbd -r -e 2 /var/tmp/test.qcow2 > > > > where the '-m auto' default exposes multi-conn for a readonly client. > > I hit send prematurely - one more thought I wanted to make clear. For > _this_ series, the behavior of '-m auto' depends solely on readonly > vs. read-write; that being the most conservative choice of preserving > pre-existing qemu-nbd behavior (that is, if you don't use -m, the only > change in behavior should be the fact that --help output now lists > -m). > > But in future versions of qemu, we might be able to improve '-m auto' > to also take into consideration whether the backing image of a > read-write device is known-cache-consistent (such as a local file > system), where we can then manage to default to advertising multi-conn > for those writable images, while still avoiding the advertisement when > exposing other devices such as network-mounted devices where we are > less confident on whether there are sufficient cache consistency > guarantees. Use of explicit '-m off' or '-m on' gives guaranteed > results no matter the qemu version, so it is only explicit (or > implied) '-m auto' where we might get smarter defaults over time. > > Thus testing whether advertising multi-conn makes a difference in > other tools like nbdcopy thus requires checking if qemu-nbd is new > enough to support -m, and then being explicit that we are opting in to > using it. I see. The commit message of patch 3 confused me because superficially it seems to say that multi-conn is safe (and therefore I assumed safe == enabled) when backed by a local filesystem. Could the commit message be improved to list the default for common combinations of flags? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Re: [PATCH v2 3/6] libvduse: Add VDUSE (vDPA Device in Userspace) library
On Wed, Mar 16, 2022 at 11:51 PM Stefan Hajnoczi wrote: > > On Wed, Mar 16, 2022 at 09:49:19PM +0800, Yongji Xie wrote: > > On Wed, Mar 16, 2022 at 9:28 PM Stefan Hajnoczi wrote: > > > > > > On Tue, Mar 15, 2022 at 07:38:12PM +0800, Yongji Xie wrote: > > > > On Tue, Mar 15, 2022 at 5:48 PM Stefan Hajnoczi > > > > wrote: > > > > > > > > > > On Tue, Feb 15, 2022 at 06:59:40PM +0800, Xie Yongji wrote: > > > > > > VDUSE [1] is a linux framework that makes it possible to implement > > > > > > software-emulated vDPA devices in userspace. This adds a library > > > > > > as a subproject to help implementing VDUSE backends in QEMU. > > > > > > > > > > > > [1] https://www.kernel.org/doc/html/latest/userspace-api/vduse.html > > > > > > > > > > > > Signed-off-by: Xie Yongji > > > > > > --- > > > > > > meson.build | 15 + > > > > > > meson_options.txt |2 + > > > > > > scripts/meson-buildoptions.sh |3 + > > > > > > subprojects/libvduse/include/atomic.h |1 + > > > > > > subprojects/libvduse/libvduse.c | 1152 > > > > > > +++ > > > > > > subprojects/libvduse/libvduse.h | 225 > > > > > > subprojects/libvduse/linux-headers/linux|1 + > > > > > > subprojects/libvduse/meson.build| 10 + > > > > > > subprojects/libvduse/standard-headers/linux |1 + > > > > > > 9 files changed, 1410 insertions(+) > > > > > > create mode 12 subprojects/libvduse/include/atomic.h > > > > > > create mode 100644 subprojects/libvduse/libvduse.c > > > > > > create mode 100644 subprojects/libvduse/libvduse.h > > > > > > create mode 12 subprojects/libvduse/linux-headers/linux > > > > > > create mode 100644 subprojects/libvduse/meson.build > > > > > > create mode 12 subprojects/libvduse/standard-headers/linux > > > > > > > > > > Please update the ./MAINTAINERS file when adding new source files. > > > > > > > > OK, sure. And would you mind being one of the maintainers since I'm > > > > not sure if I can do this job well. > > > > > > You're welcome to become the maintainer. It means that you will be CCed > > > on patches affecting this code and sometimes people might send you > > > questions about VDUSE exports. > > > > > > > I see. I will try my best. > > > > > Is the issue lack of time? > > > > > > > I think the time is enough. But since I have no experience, I'm not > > sure if I can do this well. > > Great! Don't worry there will probably not be much you need to do. > > If someone submits a patch they'll CC you and you can post your > "Reviewed-by:". Kevin or I can pick up the patches you've reviewed and > send them with our pull requests to the qemu.git maintainer. > OK, I got it. Thank you! Thanks, Yongji
Re: [PATCH v2 3/3] Use g_new() & friends where that makes obvious sense
On 15.03.2022 17:41, Markus Armbruster wrote: g_new(T, n) is neater than g_malloc(sizeof(T) * n). It's also safer, for two reasons. One, it catches multiplication overflowing size_t. Two, it returns T * rather than void *, which lets the compiler catch more type errors. This commit only touches allocations with size arguments of the form sizeof(T). Patch created mechanically with: $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \ --macro-file scripts/cocci-macro-file.h FILES... Signed-off-by: Markus Armbruster Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Cédric Le Goater Reviewed-by: Alex Bennée Acked-by: Dr. David Alan Gilbert --- replay/replay-char.c | 4 +-- replay/replay-events.c | 10 +++--- Reviewed-by: Pavel Dovgalyuk diff --git a/replay/replay-char.c b/replay/replay-char.c index dc0002367e..d2025948cf 100644 --- a/replay/replay-char.c +++ b/replay/replay-char.c @@ -50,7 +50,7 @@ void replay_register_char_driver(Chardev *chr) void replay_chr_be_write(Chardev *s, uint8_t *buf, int len) { -CharEvent *event = g_malloc0(sizeof(CharEvent)); +CharEvent *event = g_new0(CharEvent, 1); event->id = find_char_driver(s); if (event->id < 0) { @@ -85,7 +85,7 @@ void replay_event_char_read_save(void *opaque) void *replay_event_char_read_load(void) { -CharEvent *event = g_malloc0(sizeof(CharEvent)); +CharEvent *event = g_new0(CharEvent, 1); event->id = replay_get_byte(); replay_get_array_alloc(&event->buf, &event->len); diff --git a/replay/replay-events.c b/replay/replay-events.c index 15983dd250..ac47c89834 100644 --- a/replay/replay-events.c +++ b/replay/replay-events.c @@ -119,7 +119,7 @@ void replay_add_event(ReplayAsyncEventKind event_kind, return; } -Event *event = g_malloc0(sizeof(Event)); +Event *event = g_new0(Event, 1); event->event_kind = event_kind; event->opaque = opaque; event->opaque2 = opaque2; @@ -243,17 +243,17 @@ static Event *replay_read_event(int checkpoint) } break; case REPLAY_ASYNC_EVENT_INPUT: -event = g_malloc0(sizeof(Event)); +event = g_new0(Event, 1); event->event_kind = replay_state.read_event_kind; event->opaque = replay_read_input_event(); return event; case REPLAY_ASYNC_EVENT_INPUT_SYNC: -event = g_malloc0(sizeof(Event)); +event = g_new0(Event, 1); event->event_kind = replay_state.read_event_kind; event->opaque = 0; return event; case REPLAY_ASYNC_EVENT_CHAR_READ: -event = g_malloc0(sizeof(Event)); +event = g_new0(Event, 1); event->event_kind = replay_state.read_event_kind; event->opaque = replay_event_char_read_load(); return event; @@ -263,7 +263,7 @@ static Event *replay_read_event(int checkpoint) } break; case REPLAY_ASYNC_EVENT_NET: -event = g_malloc0(sizeof(Event)); +event = g_new0(Event, 1); event->event_kind = replay_state.read_event_kind; event->opaque = replay_event_net_load(); return event;