Re: [PULL 0/8] s390x and misc fixes

[Thomas Huth]

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()

[Emanuele Giuseppe Esposito]

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

[Emanuele Giuseppe Esposito]

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

[Vladimir Sementsov-Ogievskiy]

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

[marcandre . lureau]

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

[marcandre . lureau]

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

[marcandre . lureau]

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

[Thomas Huth]

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

[marcandre . lureau]

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

[marcandre . lureau]

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

[Halil Pasic]

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

[Thomas Huth]

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

[Marc-André Lureau]

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

[Halil Pasic]

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

[Richard W.M. Jones]

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

[Markus Armbruster]

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

[Stefan Hajnoczi]

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()

[Stefan Hajnoczi]

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

[Stefan Hajnoczi]

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

[Stefan Hajnoczi]

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

[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


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

[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 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

[Emanuele Giuseppe Esposito]

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

[Philippe Mathieu-Daudé]

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

[Daniel P . Berrangé]

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

[Philippe Mathieu-Daudé]

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

[Philippe Mathieu-Daudé]

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

[Marc-André Lureau]

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

[Thomas Huth]

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

[Philippe Mathieu-Daudé]

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

[Daniel P . Berrangé]

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

[Philippe Mathieu-Daudé]

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

[Yongji Xie]

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

[Stefan Hajnoczi]

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

[Marc-André Lureau]

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

[Nicolas Saenz Julienne]

'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

[Yongji Xie]

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

[Nicolas Saenz Julienne]

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

[Nicolas Saenz Julienne]

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

[Nicolas Saenz Julienne]

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

[Markus Armbruster]

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

[Emanuele Giuseppe Esposito]

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

[Markus Armbruster]

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

[Philippe Mathieu-Daudé]

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

[Stefan Hajnoczi]

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

[Kevin Wolf]

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

[Kevin Wolf]

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

[Stefan Hajnoczi]

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

[Marc-André Lureau]

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

[Eric Blake]

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

[Eric Blake]

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

[Eric Blake]

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

[Eric Blake]

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

[Eric Blake]

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

[Emanuele Giuseppe Esposito]

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

[Eric Blake]

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

[Richard W.M. Jones]

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

[Yongji Xie]

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

[Pavel Dovgalyuk]

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;