Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
Am 27.04.23 um 16:36 schrieb Juan Quintela: > Fiona Ebner wrote: >> Am 27.04.23 um 13:03 schrieb Kevin Wolf: >>> Am 26.04.2023 um 16:31 hat Fiona Ebner geschrieben: Am 20.04.23 um 08:55 schrieb Paolo Bonzini: > > Hi > >> Our function is a custom variant of saving a snapshot and uses >> qemu_savevm_state_setup(), which is why the qemu_mutex_unlock_iothread() >> is there. I looked for inspiration for how upstream does things and it >> turns out that upstream QEMU v8.0.0 has essentially the same issue with >> snapshot-save. When snapshot_save_job_bh runs in a vCPU thread instead >> of the main thread, the situation is the same: after >> qemu_mutex_unlock_iothread(), qemu_get_current_aio_context() will return >> 0x0 and then the assertion in the AIO_WAIT_WHILE_INTERNAL macro fails >> (this time the generated coroutine wrapper is bdrv_writev_vmstate)[0]. >> >> >> So all bottom halves scheduled for the main thread's AioContext can >> potentially get to run in a vCPU thread and need to be very careful with >> things like qemu_mutex_unlock_iothread. >> >> Is qemu_get_current_aio_context() returning 0x0 expected? I haven't >> looked into why it happens yet. Does there need to be a way to drop the >> BQL without also giving up the main thread's AioContext or would it be >> enough to re-acquire the context? >> >> CC-ing Juan as the migration maintainer. > > This is the world backwards. > The tradition is that migration people blame block layer people for > breaking things and for help, not the other way around O:-) Sorry, if I didn't provide enough context/explanation. See below for my attempt to re-iterate. I CC'ed you, because the issue happens as part of snapshot-save and in particular the qemu_mutex_unlock_iothread call in qemu_savevm_state is one of the ingredients leading to the problem. > >> Best Regards, >> Fiona >> >> [0]: >>> Thread 21 "CPU 0/KVM" received signal SIGABRT, Aborted. >>> __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 >>> 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. >>> (gdb) bt >>> #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 >>> #1 0x7f9027b3e537 in __GI_abort () at abort.c:79 >>> #2 0x7f9027b3e40f in __assert_fail_base (fmt=0x7f9027cb66a8 >>> "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", >>> assertion=0x558ed44fcec0 "qemu_get_current_aio_context() == >>> qemu_get_aio_context()", file=0x558ed44fce80 >>> "/home/febner/repos/qemu/block/block-gen.h", line=43, >>> function=) at assert.c:92 >>> #3 0x7f9027b4d662 in __GI___assert_fail >>> (assertion=0x558ed44fcec0 "qemu_get_current_aio_context() == >>> qemu_get_aio_context()", file=0x558ed44fce80 >>> "/home/febner/repos/qemu/block/block-gen.h", line=43, >>> function=0x558ed44fcf80 <__PRETTY_FUNCTION__.14> "bdrv_poll_co") at >>> assert.c:101 >>> #4 0x558ed412df5f in bdrv_poll_co (s=0x7f8ffcff37a0) at >>> /home/febner/repos/qemu/block/block-gen.h:43 >>> #5 0x558ed412f4cd in bdrv_writev_vmstate (bs=0x558ed60536a0, >>> qiov=0x7f8ffcff3840, pos=0) at block/block-gen.c:809 >>> #6 0x558ed3df36d0 in qio_channel_block_writev >>> (ioc=0x7f8ff40ac060, iov=0x7f8ff43f6350, niov=1, fds=0x0, nfds=0, >>> flags=0, errp=0x7f8ffcff39c0) at ../migration/channel-block.c:89 >>> #7 0x558ed40feedb in qio_channel_writev_full >>> (ioc=0x7f8ff40ac060, iov=0x7f8ff43f6350, niov=1, fds=0x0, nfds=0, >>> flags=0, errp=0x7f8ffcff39c0) at ../io/channel.c:108 >>> #8 0x558ed40ff3c3 in qio_channel_writev_full_all >>> (ioc=0x7f8ff40ac060, iov=0x7f8ff4648040, niov=1, fds=0x0, nfds=0, >>> flags=0, errp=0x7f8ffcff39c0) at ../io/channel.c:263 >>> #9 0x558ed40ff2e4 in qio_channel_writev_all (ioc=0x7f8ff40ac060, >>> iov=0x7f8ff4648040, niov=1, errp=0x7f8ffcff39c0) at >>> ../io/channel.c:242 >>> #10 0x558ed3dee4dc in qemu_fflush (f=0x7f8ff464) at >>> ../migration/qemu-file.c:302 >>> #11 0x558ed4050f91 in ram_save_setup (f=0x7f8ff464, >>> opaque=0x558ed4ca34c0 ) at ../migration/ram.c:3302 >>> #12 0x558ed3e141c8 in qemu_savevm_state_setup (f=0x7f8ff464) at >>> ../migration/savevm.c:1266 >>> #13 0x558ed3e14eed in qemu_savevm_state (f=0x7f8ff464, >>> errp=0x558ed68c5238) at ../migration/savevm.c:1626 >>> #14 0x558ed3e1755e in save_snapshot (name=0x558ed72af790 >>> "snap0", overwrite=false, vmstate=0x558ed6708ce0 "scsi0", >>> has_devices=true, devices=0x558ed66d6a60, errp=0x558ed68c5238) at >>> ../migration/savevm.c:2954 >>> #15 0x558ed3e17fb1 in snapshot_save_job_bh (opaque=0x558ed68c5170) at >>> ../migration/savevm.c:3253 >>> #16 0x558ed42f050a in aio_bh_call (bh=0x558ed671ae00) at >>> ../util/async.c:155 >>> #17 0x558ed42f0615 in aio_bh_poll (ctx=0x558ed5c62910) at >>> ../util/async.c:184 >>> #18 0x558ed42d47b8 in aio_poll (ctx=0x558ed5c62910, blocking=true) at >>> ../util/aio-posix.c:721 >>> #19 0x558ed412df1c in bdrv_poll_co (s=0x7f8ffcff3eb0) at >>> /home/febner/repos/qemu/block/block-gen.h:43 >>> #20 0x
Re: [PATCH v3 3/4] build: move COLO under CONFIG_REPLICATION
Vladimir Sementsov-Ogievskiy wrote:
> We don't allow to use x-colo capability when replication is not
> configured. So, no reason to build COLO when replication is disabled,
> it's unusable in this case.
>
> Note also that the check in migrate_caps_check() is not the only
> restriction: some functions in migration/colo.c will just abort if
> called with not defined CONFIG_REPLICATION, for example:
>
> migration_iteration_finish()
>case MIGRATION_STATUS_COLO:
>migrate_start_colo_process()
>colo_process_checkpoint()
>abort()
>
> It could probably make sense to have possibility to enable COLO without
> REPLICATION, but this requires deeper audit of colo & replication code,
> which may be done later if needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
Nice patch. Thanks.
> @@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
> static void secondary_vm_do_failover(void)
> {
> /* COLO needs enable block-replication */
> -#ifdef CONFIG_REPLICATION
> int old_state;
> MigrationIncomingState *mis = migration_incoming_get_current();
> Error *local_err = NULL;
> @@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
> if (mis->migration_incoming_co) {
> qemu_coroutine_enter(mis->migration_incoming_co);
> }
> -#else
> -abort();
> -#endif
> }
With only this chunks you have proved that your argument is right.
abort() is never a solution.
> diff --git a/migration/options.c b/migration/options.c
> index 912cbadddb..eef2bd0f16 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -171,7 +171,9 @@ Property migration_properties[] = {
> DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
> DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
> MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
> +#ifdef CONFIG_REPLICATION
> DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
> +#endif
> DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
> DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
> DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
> @@ -209,9 +211,13 @@ bool migrate_block(void)
> bool migrate_colo(void)
> {
> +#ifdef CONFIG_REPLICATION
> MigrationState *s = migrate_get_current();
>
> return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
> +#else
> +return false;
> +#endif
> }
#ifdef 1
>
> bool migrate_compress(void)
> @@ -401,7 +407,9 @@
> INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
> MIGRATION_CAPABILITY_RDMA_PIN_ALL,
> MIGRATION_CAPABILITY_COMPRESS,
> MIGRATION_CAPABILITY_XBZRLE,
> +#ifdef CONFIG_REPLICATION
> MIGRATION_CAPABILITY_X_COLO,
> +#endif
> MIGRATION_CAPABILITY_VALIDATE_UUID,
> MIGRATION_CAPABILITY_ZERO_COPY_SEND);
>
#ifdef 2
> @@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
> Error **errp)
> }
> #endif
>
> -#ifndef CONFIG_REPLICATION
> -if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
> -error_setg(errp, "QEMU compiled without replication module"
> - " can't enable COLO");
> -error_append_hint(errp, "Please enable replication before COLO.\n");
> -return false;
> -}
> -#endif
> -
> if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
> /* This check is reasonably expensive, so only when it's being
> * set the first time, also it's only the destination that needs
I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
trying to get all capabilities to this format.
> diff --git a/stubs/colo.c b/stubs/colo.c
> new file mode 100644
> index 00..f306ab45d6
> --- /dev/null
> +++ b/stubs/colo.c
> @@ -0,0 +1,37 @@
> +#include "qemu/osdep.h"
> +#include "qemu/notify.h"
> +#include "net/colo-compare.h"
> +#include "migration/colo.h"
> +#include "migration/migration.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
> +
> +void colo_shutdown(void)
> +{
> +abort();
> +}
This is wrong, it should be empty.
void migration_shutdown(void)
{
/*
* When the QEMU main thread exit, the COLO thread
* may wait a semaphore. So, we should wakeup the
* COLO thread before migration shutdown.
*/
colo_shutdown();
..
}
> +void *colo_process_incoming_thread(void *opaque)
> +{
> +abort();
> +}
At least print an error message?
> +void colo_checkpoint_notify(void *opaque)
> +{
> +abort();
> +}
Another error message.
It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c
changing
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
if (migration_in_colo_state()) {
colo_checkpoint_notify(s);
}
}
To
if (params->has_x_checkpoint_delay)
Re: [PATCH v3 4/4] configure: add --disable-colo-proxy option
Vladimir Sementsov-Ogievskiy wrote: > Add option to not build filter-mirror, filter-rewriter and > colo-compare when they are not needed. > > There could be more agile configuration, for example add separate > options for each filter, but that may be done in future on demand. The > aim of this patch is to make possible to disable the whole COLO Proxy > subsystem. > > Signed-off-by: Vladimir Sementsov-Ogievskiy As you have arrived to an agreement about what to do with filter-rewriter, the rest of the patch is ok with me. Reviewed-by: Juan Quintela
Re: [PULL 00/18] testing and doc updates
On 4/27/23 16:44, Alex Bennée wrote: The following changes since commit 1eb95e1baef852d0971a1dd62a3293cd68f1ec35: Merge tag 'migration-20230426-pull-request' ofhttps://gitlab.com/juan.quintela/qemu into staging (2023-04-27 10:47:14 +0100) are available in the Git repository at: https://gitlab.com/stsquad/qemu.git tags/pull-testing-docs-270423-1 for you to fetch changes up to ef46ae67ba9a785cf0cce58b5fc5a36ed3c6c7b9: docs/style: call out the use of GUARD macros (2023-04-27 14:58:51 +0100) Testing and documentation updates: - bump avocado to 101.0 - use snapshots for tuxrun baseline tests - add sbda-ref test to avocado - avoid spurious re-configure in gitlab - better description of blockdev options - drop FreeBSD 12 from Cirrus CI - fix up the ast2[56]00 tests to be more stable - improve coverage of ppc64 tests in tuxrun baselines - limit plugin tests to just the generic multiarch binaries Applied, thanks. Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate. r~
Re: [PATCH v11 06/13] tests/qtest: Adjust and document query-cpu-model-expansion test for arm
On 4/27/23 14:16, Fabiano Rosas wrote:
Richard Henderson writes:
On 4/26/23 19:00, Fabiano Rosas wrote:
We're about to move the 32-bit CPUs under CONFIG_TCG, so adjust the
query-cpu-model-expansion test to check against the cortex-a7, which
is already under CONFIG_TCG. That allows the next patch to contain
only code movement.
While here add comments clarifying what we're testing.
Signed-off-by: Fabiano Rosas
Suggested-by: Philippe Mathieu-Daudé
---
tests/qtest/arm-cpu-features.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
I don't see why you're changing the cpu model here.
Neither cpu will work, of course, but why change?
Because there's already a patch in master that puts the cortex-a7 under
CONFIG_TCG, so I can have the whole if/else in this patch.
If I keep the cortex-a15, this change needs to go into the next patch
("move cpu_tcg to tcg/cpu32.c") which moves the rest of the 32bit cpus,
which was supposed to be only code movement.
Well, I still think the change to a7 is wrong.
If the two patches need to be merged to break bisection,
then so be it -- just mention that fact in the commit message.
Peter, do you agree?
r~
Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
Fiona Ebner wrote: > Am 27.04.23 um 16:36 schrieb Juan Quintela: >> Fiona Ebner wrote: >>> Am 27.04.23 um 13:03 schrieb Kevin Wolf: Am 26.04.2023 um 16:31 hat Fiona Ebner geschrieben: > Am 20.04.23 um 08:55 schrieb Paolo Bonzini: >> >> Hi >> >>> Our function is a custom variant of saving a snapshot and uses >>> qemu_savevm_state_setup(), which is why the qemu_mutex_unlock_iothread() >>> is there. I looked for inspiration for how upstream does things and it >>> turns out that upstream QEMU v8.0.0 has essentially the same issue with >>> snapshot-save. When snapshot_save_job_bh runs in a vCPU thread instead >>> of the main thread, the situation is the same: after >>> qemu_mutex_unlock_iothread(), qemu_get_current_aio_context() will return >>> 0x0 and then the assertion in the AIO_WAIT_WHILE_INTERNAL macro fails >>> (this time the generated coroutine wrapper is bdrv_writev_vmstate)[0]. >>> >>> >>> So all bottom halves scheduled for the main thread's AioContext can >>> potentially get to run in a vCPU thread and need to be very careful with >>> things like qemu_mutex_unlock_iothread. >>> >>> Is qemu_get_current_aio_context() returning 0x0 expected? I haven't >>> looked into why it happens yet. Does there need to be a way to drop the >>> BQL without also giving up the main thread's AioContext or would it be >>> enough to re-acquire the context? >>> >>> CC-ing Juan as the migration maintainer. >> >> This is the world backwards. >> The tradition is that migration people blame block layer people for >> breaking things and for help, not the other way around O:-) > > Sorry, if I didn't provide enough context/explanation. See below for my > attempt to re-iterate. I CC'ed you, because the issue happens as part of > snapshot-save and in particular the qemu_mutex_unlock_iothread call in > qemu_savevm_state is one of the ingredients leading to the problem. This was a joke O:-) >> To see that I am understading this right: >> >> - you create a thread >> - that calls a memory_region operation >> - that calls a device write function >> - that calls the block layer >> - that creates a snapshot >> - that calls the migration code >> - that calls the block layer again >> >> Without further investigation, I have no clue what is going on here, >> sorry. >> >> Later, Juan. >> > > All I'm doing is using QEMU (a build of upstream's v8.0.0) in intended > ways, I promise! In particular, I'm doing two things at the same time > repeatedly: > 1. Write to a pflash drive from within the guest. > 2. Issue a snapshot-save QMP command (in a way that doesn't lead to an > early error). > > (I actually also used a debugger to break on pflash_update and > snapshot_save_job_bh, manually continuing until I triggered the > problematic situation. It's very racy, because it depends on the host OS > to switch threads at the correct time.) I think the block layer is the problem (famous last words) > > Now we need to be aware of two things: > 1. As discussed earlier in the mail thread, if the host OS switches > threads at an inconvenient time, it can happen that a bottom half > scheduled for the main thread's AioContext can be executed as part of a > vCPU thread's aio_poll. > 2. Generated coroutine wrappers for block layer functions spawn the > coroutine and use AIO_WAIT_WHILE/aio_poll to wait for it to finish. > > What happens in the backtrace above is: > 1. The write to the pflash drive uses blk_pwrite which leads to an > aio_poll in the vCPU thread. > 2. The snapshot_save_job_bh bottom half, that was scheduled for the main > thread's AioContext, is executed as part of the vCPU thread's aio_poll. > 3. qemu_savevm_state is called. > 4. qemu_mutex_unlock_iothread is called. Now > qemu_get_current_aio_context returns 0x0. Usually, snapshot_save_job_bh > runs in the main thread, in which case qemu_get_current_aio_context > still returns the main thread's AioContext at this point. I am perhaps a bit ingenuous here, but it is there a way to convince qemu that snapshot_save_job_bh *HAS* to run on the main thread? > 5. bdrv_writev_vmstate is executed as part of the usual savevm setup. > 6. bdrv_writev_vmstate is a generated coroutine wrapper, so it uses > AIO_WAIT_WHILE. > 7. The assertion to have the main thread's AioContext inside the > AIO_WAIT_WHILE macro fails. several problems here: a- There is no "test/qtest/snapshot-test" around Hint, Hint. Basically snapshots are the bastard sibling of migration, and nobody really tests them. b- In an ideal world, migration shouldn't need a bottom_handler Remember, it has its own thread. But there are functions that can only been called from the main thread. And no, I don't remember which, I just try very hard not to touch that part of the code. c- At that point the vcpus are stopped, for migration it doesn't matter a lot(*) to have to use a bottom handler. d- snapshots are a completely different beast, that don't really stop the guest in the same way at that point, and
Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
Am 27.04.23 um 16:56 schrieb Peter Xu: > On Thu, Apr 27, 2023 at 04:36:14PM +0200, Juan Quintela wrote: >> Fiona Ebner wrote: >>> Am 27.04.23 um 13:03 schrieb Kevin Wolf: Am 26.04.2023 um 16:31 hat Fiona Ebner geschrieben: > Am 20.04.23 um 08:55 schrieb Paolo Bonzini: >> >> Hi >> >>> Our function is a custom variant of saving a snapshot and uses >>> qemu_savevm_state_setup(), which is why the qemu_mutex_unlock_iothread() >>> is there. I looked for inspiration for how upstream does things and it >>> turns out that upstream QEMU v8.0.0 has essentially the same issue with >>> snapshot-save. When snapshot_save_job_bh runs in a vCPU thread instead >>> of the main thread, the situation is the same: after >>> qemu_mutex_unlock_iothread(), qemu_get_current_aio_context() will return >>> 0x0 and then the assertion in the AIO_WAIT_WHILE_INTERNAL macro fails >>> (this time the generated coroutine wrapper is bdrv_writev_vmstate)[0]. >>> >>> >>> So all bottom halves scheduled for the main thread's AioContext can >>> potentially get to run in a vCPU thread and need to be very careful with >>> things like qemu_mutex_unlock_iothread. >>> >>> Is qemu_get_current_aio_context() returning 0x0 expected? I haven't >>> looked into why it happens yet. Does there need to be a way to drop the >>> BQL without also giving up the main thread's AioContext or would it be >>> enough to re-acquire the context? >>> >>> CC-ing Juan as the migration maintainer. >> >> This is the world backwards. >> The tradition is that migration people blame block layer people for >> breaking things and for help, not the other way around O:-) >> >>> Best Regards, >>> Fiona >>> >>> [0]: Thread 21 "CPU 0/KVM" received signal SIGABRT, Aborted. __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x7f9027b3e537 in __GI_abort () at abort.c:79 #2 0x7f9027b3e40f in __assert_fail_base (fmt=0x7f9027cb66a8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x558ed44fcec0 "qemu_get_current_aio_context() == qemu_get_aio_context()", file=0x558ed44fce80 "/home/febner/repos/qemu/block/block-gen.h", line=43, function=) at assert.c:92 #3 0x7f9027b4d662 in __GI___assert_fail (assertion=0x558ed44fcec0 "qemu_get_current_aio_context() == qemu_get_aio_context()", file=0x558ed44fce80 "/home/febner/repos/qemu/block/block-gen.h", line=43, function=0x558ed44fcf80 <__PRETTY_FUNCTION__.14> "bdrv_poll_co") at assert.c:101 #4 0x558ed412df5f in bdrv_poll_co (s=0x7f8ffcff37a0) at /home/febner/repos/qemu/block/block-gen.h:43 #5 0x558ed412f4cd in bdrv_writev_vmstate (bs=0x558ed60536a0, qiov=0x7f8ffcff3840, pos=0) at block/block-gen.c:809 #6 0x558ed3df36d0 in qio_channel_block_writev (ioc=0x7f8ff40ac060, iov=0x7f8ff43f6350, niov=1, fds=0x0, nfds=0, flags=0, errp=0x7f8ffcff39c0) at ../migration/channel-block.c:89 #7 0x558ed40feedb in qio_channel_writev_full (ioc=0x7f8ff40ac060, iov=0x7f8ff43f6350, niov=1, fds=0x0, nfds=0, flags=0, errp=0x7f8ffcff39c0) at ../io/channel.c:108 #8 0x558ed40ff3c3 in qio_channel_writev_full_all (ioc=0x7f8ff40ac060, iov=0x7f8ff4648040, niov=1, fds=0x0, nfds=0, flags=0, errp=0x7f8ffcff39c0) at ../io/channel.c:263 #9 0x558ed40ff2e4 in qio_channel_writev_all (ioc=0x7f8ff40ac060, iov=0x7f8ff4648040, niov=1, errp=0x7f8ffcff39c0) at ../io/channel.c:242 #10 0x558ed3dee4dc in qemu_fflush (f=0x7f8ff464) at ../migration/qemu-file.c:302 #11 0x558ed4050f91 in ram_save_setup (f=0x7f8ff464, opaque=0x558ed4ca34c0 ) at ../migration/ram.c:3302 #12 0x558ed3e141c8 in qemu_savevm_state_setup (f=0x7f8ff464) at ../migration/savevm.c:1266 #13 0x558ed3e14eed in qemu_savevm_state (f=0x7f8ff464, errp=0x558ed68c5238) at ../migration/savevm.c:1626 #14 0x558ed3e1755e in save_snapshot (name=0x558ed72af790 "snap0", overwrite=false, vmstate=0x558ed6708ce0 "scsi0", has_devices=true, devices=0x558ed66d6a60, errp=0x558ed68c5238) at ../migration/savevm.c:2954 #15 0x558ed3e17fb1 in snapshot_save_job_bh (opaque=0x558ed68c5170) at ../migration/savevm.c:3253 #16 0x558ed42f050a in aio_bh_call (bh=0x558ed671ae00) at ../util/async.c:155 #17 0x558ed42f0615 in aio_bh_poll (ctx=0x558ed5c62910) at ../util/async.c:184 #18 0x558ed42d47b8 in aio_poll (ctx=0x558ed5c62910, blocking=true) at ../util/aio-posix.c:721 #19 0x558ed412df1c in bdrv_poll_co (s=0x7f8ffcff3eb0) at /home/febner/repos/qemu/block/block-gen.h:43 #20 0x558ed4130c3a in blk_pwrite (blk=0x558ed5ed4f60, offset=230912, bytes=512, buf=0x7f8ffc438600, flags=0) at block/block-gen.c
Re: [PATCH v2 01/21] meson.build Add CONFIG_HEXAGON_IDEF_PARSER
On 4/27/23 23:59, Taylor Simpson wrote: Enable conditional compilation depending on whether idef-parser is configured Signed-off-by: Taylor Simpson --- meson.build | 1 + 1 file changed, 1 insertion(+) Reviewed-by: Richard Henderson r~
Re: [PATCH v2 05/21] Hexagon (target/hexagon) Add overrides for clr[tf]new
On 4/27/23 23:59, Taylor Simpson wrote:
These instructions have implicit reads from p0, so we don't want
them in helpers when idef-parser is off.
Signed-off-by: Taylor Simpson
---
target/hexagon/gen_tcg.h | 16
target/hexagon/macros.h | 4
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/target/hexagon/gen_tcg.h b/target/hexagon/gen_tcg.h
index 7c5cb93297..f3e9c280b0 100644
--- a/target/hexagon/gen_tcg.h
+++ b/target/hexagon/gen_tcg.h
@@ -1097,6 +1097,22 @@
gen_jump(ctx, riV); \
} while (0)
+/* if (p0.new) r0 = #0 */
+#define fGEN_TCG_SA1_clrtnew(SHORTCODE) \
+do { \
+tcg_gen_movcond_tl(TCG_COND_EQ, RdV, \
+ hex_new_pred_value[0], tcg_constant_tl(0), \
+ RdV, tcg_constant_tl(0)); \
+} while (0)
+
+/* if (!p0.new) r0 = #0 */
+#define fGEN_TCG_SA1_clrfnew(SHORTCODE) \
+do { \
+tcg_gen_movcond_tl(TCG_COND_NE, RdV, \
+ hex_new_pred_value[0], tcg_constant_tl(0), \
+ RdV, tcg_constant_tl(0)); \
+} while (0)
+
Reviewed-by: Richard Henderson
r~
#define fGEN_TCG_J2_pause(SHORTCODE) \
do { \
uiV = uiV; \
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 3e162de3a7..2cb0647ce2 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -227,12 +227,8 @@ static inline void gen_cancel(uint32_t slot)
#ifdef QEMU_GENERATE
#define fLSBNEW(PVAL) tcg_gen_andi_tl(LSB, (PVAL), 1)
-#define fLSBNEW0tcg_gen_andi_tl(LSB, hex_new_pred_value[0], 1)
-#define fLSBNEW1tcg_gen_andi_tl(LSB, hex_new_pred_value[1], 1)
#else
#define fLSBNEW(PVAL) ((PVAL) & 1)
-#define fLSBNEW0(env->new_pred_value[0] & 1)
-#define fLSBNEW1(env->new_pred_value[1] & 1)
#endif
#ifdef QEMU_GENERATE
Re: [PATCH v2 14/21] Hexagon (target/hexagon) Short-circuit more HVX single instruction packets
On 4/28/23 00:00, Taylor Simpson wrote: The generated helpers for HVX use pass-by-reference, so they can't short-circuit when the reads/writes overlap. The instructions with overrides are OK because they use tcg_gen_gvec_*. We add a flag has_hvx_helper to DisasContext and extend gen_analyze_funcs to set the flag when the instruction is an HVX instruction with a generated helper. We add an override for V6_vcombine so that it can be short-circuited along with a test case in tests/tcg/hexagon/hvx_misc.c Signed-off-by: Taylor Simpson --- Reviewed-by: Richard Henderson r~
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag is set/checked prior to calling a device's MemoryRegion
> handlers, and set when device code initiates DMA. The purpose of this
> flag is to prevent two types of DMA-based reentrancy issues:
>
> 1.) mmio -> dma -> mmio case
> 2.) bh -> dma write -> mmio case
>
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
>
> Summary of the problem from Peter Maydell:
> https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> Resolves: CVE-2023-0330
>
> Signed-off-by: Alexander Bulekov
> Reviewed-by: Thomas Huth
> ---
> include/exec/memory.h | 5 +
> include/hw/qdev-core.h | 7 +++
> softmmu/memory.c | 16
> 3 files changed, 28 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 15ade918ba..e45ce6061f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -767,6 +767,8 @@ struct MemoryRegion {
> bool is_iommu;
> RAMBlock *ram_block;
> Object *owner;
> +/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access
> hotpath */
> +DeviceState *dev;
>
> const MemoryRegionOps *ops;
> void *opaque;
> @@ -791,6 +793,9 @@ struct MemoryRegion {
> unsigned ioeventfd_nb;
> MemoryRegionIoeventfd *ioeventfds;
> RamDiscardManager *rdm; /* Only for RAM */
> +
> +/* For devices designed to perform re-entrant IO into their own IO MRs */
> +bool disable_reentrancy_guard;
> };
>
> struct IOMMUMemoryRegion {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bd50ad5ee1..7623703943 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -162,6 +162,10 @@ struct NamedClockList {
> QLIST_ENTRY(NamedClockList) node;
> };
>
> +typedef struct {
> +bool engaged_in_io;
> +} MemReentrancyGuard;
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -194,6 +198,9 @@ struct DeviceState {
> int alias_required_for_version;
> ResettableState reset;
> GSList *unplug_blockers;
> +
> +/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy
> */
> +MemReentrancyGuard mem_reentrancy_guard;
> };
>
> struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index b1a6cae6f5..fe23f0e5ce 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
> access_size_max = 4;
> }
>
> +/* Do not allow more than one simultaneous access to a device's IO
> Regions */
> +if (mr->dev && !mr->disable_reentrancy_guard &&
> +!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
> +if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
> +warn_report("Blocked re-entrant IO on "
> +"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
> +memory_region_name(mr), addr);
> +return MEMTX_ACCESS_ERROR;
If we issue this warn_report on every invalid memory access, is this
going to become a denial of service by flooding logs, or is the
return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
*once* in the lifetime of the QEMU process ?
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 v10 1/8] memory: prevent dma-reentracy issues
On 28/04/2023 10.12, Daniel P. Berrangé wrote:
On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
include/exec/memory.h | 5 +
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 16
3 files changed, 28 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..fe23f0e5ce 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions */
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+warn_report("Blocked re-entrant IO on "
+"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
+memory_region_name(mr), addr);
+return MEMTX_ACCESS_ERROR;
If we issue this warn_report on every invalid memory access, is this
going to become a denial of service by flooding logs, or is the
return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
*once* in the lifetime of the QEMU process ?
Maybe it's better to use warn_report_once() here instead?
Thomas
Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
"Michael S. Tsirkin" wrote:
> On Tue, Apr 25, 2023 at 07:14:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On incoming migration we have the following sequence to load option
>> ROM:
>>
>> 1. On device realize we do normal load ROM from the file
>>
>> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>>block. If sizes mismatch we fail.
>
> let's mention an example error message:
> Size mismatch: :00:03.0/virtio-net-pci.rom: 0x4 != 0x8: Invalid
> argument
This is a mess(TM).
And no easy way to fix it. Everything has its problems.
Ok, I will elaborate.
We have source machine and destination machine.
Easy case, same version of qemu (or at least the same rom files).
The interesting ones is when the sizes are wrong.
Again this splits on two cases:
- target side is bigger
not big deal, during migration we just don't use all the space.
- target side is smaller
guess what, not easy way to get this working O:-)
We added some changes on the past for this, but I don't remember the
details.
If I understood his patch correctly, it set seems to try to fix this to
always do the right thing with respect to migration, i.e. using whatever
was on the source. I think this is nice.
But we still have left out the big elephant on the ROM, what happens
when we reboot.
Right now, when we reboot we still use the rom files for the source.
And I think that in the case of reboot, if the ROM files have changed
(because there was an upgrade or we migrate to a host with a never
version, etc,) we should always do a powerdown + start to let qemu use
the new ROM files.
As far as I know, no management app does that, and especially as we move
to UEFI (i.e. more complex firmware with more posibilities for CVE's) I
think we should considerd this case.
>> @@ -2293,10 +2294,16 @@ static void pci_add_option_rom(PCIDevice *pdev, bool
>> is_default_rom,
>> {
>> int64_t size;
>> g_autofree char *path = NULL;
>> -void *ptr;
>> char name[32];
>> const VMStateDescription *vmsd;
>>
>> +/*
>> + * In case of incoming migration ROM will come with migration stream, no
>> + * reason to load the file. Neither we want to fail if local ROM file
>> + * mismatches with specified romsize.
>> + */
>> +bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
>> +
>> if (!pdev->romfile) {
>> return;
>> }
>
> CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
> this the right way to figure out we are not going to use the
> device locally before incoming migration will overwrite ROM contents?
There is only a way to get into RUN_STATE_INMIGRATE, and that is that we
have started the guest with --incoming . So the check does
what it is intended.
Once told that, I have never been seen it used for this.
/me launches grep on source tree
At least the block layer and usb use it exactly for this. So I will say
it is the correct way of doing it (or at least I can think of a better
way right now).
The grep also shows this:
static void rom_reset(void *unused)
{
Rom *rom;
QTAILQ_FOREACH(rom, &roms, next) {
if (rom->fw_file) {
continue;
}
/*
* We don't need to fill in the RAM with ROM data because we'll fill
* the data in during the next incoming migration in all cases. Note
* that some of those RAMs can actually be modified by the guest.
*/
if (runstate_check(RUN_STATE_INMIGRATE)) {
if (rom->data && rom->isrom) {
/*
* Free it so that a rom_reset after migration doesn't
* overwrite a potentially modified 'rom'.
*/
rom_free_data(rom);
}
continue;
}
It is not exactly the problem at hand, but it is related. I am just
wondering if we can do something common.
Later, Juan.
Re: [PATCH v3 4/4] configure: add --disable-colo-proxy option
On 28.04.23 10:33, Juan Quintela wrote:
Vladimir Sementsov-Ogievskiy wrote:
Add option to not build filter-mirror, filter-rewriter and
colo-compare when they are not needed.
There could be more agile configuration, for example add separate
options for each filter, but that may be done in future on demand. The
aim of this patch is to make possible to disable the whole COLO Proxy
subsystem.
Signed-off-by: Vladimir Sementsov-Ogievskiy
As you have arrived to an agreement about what to do with
filter-rewriter, the rest of the patch is ok with me.
You mean filter-mirror, precisely this change to the patch:
@@ -22,7 +22,7 @@ if get_option('replication').allowed() or \
endif
if get_option('colo_proxy').allowed()
- softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
+ softmmu_ss.add(files('filter-rewriter.c'))
endif
softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
?
Reviewed-by: Juan Quintela
--
Best regards,
Vladimir
Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
Am 28.04.2023 um 09:47 hat Juan Quintela geschrieben: > Fiona Ebner wrote: > > Am 27.04.23 um 16:36 schrieb Juan Quintela: > >> Fiona Ebner wrote: > >>> Am 27.04.23 um 13:03 schrieb Kevin Wolf: > Am 26.04.2023 um 16:31 hat Fiona Ebner geschrieben: > > Am 20.04.23 um 08:55 schrieb Paolo Bonzini: > >> > >> Hi > >> > >>> Our function is a custom variant of saving a snapshot and uses > >>> qemu_savevm_state_setup(), which is why the qemu_mutex_unlock_iothread() > >>> is there. I looked for inspiration for how upstream does things and it > >>> turns out that upstream QEMU v8.0.0 has essentially the same issue with > >>> snapshot-save. When snapshot_save_job_bh runs in a vCPU thread instead > >>> of the main thread, the situation is the same: after > >>> qemu_mutex_unlock_iothread(), qemu_get_current_aio_context() will return > >>> 0x0 and then the assertion in the AIO_WAIT_WHILE_INTERNAL macro fails > >>> (this time the generated coroutine wrapper is bdrv_writev_vmstate)[0]. > >>> > >>> > >>> So all bottom halves scheduled for the main thread's AioContext can > >>> potentially get to run in a vCPU thread and need to be very careful with > >>> things like qemu_mutex_unlock_iothread. > >>> > >>> Is qemu_get_current_aio_context() returning 0x0 expected? I haven't > >>> looked into why it happens yet. Does there need to be a way to drop the > >>> BQL without also giving up the main thread's AioContext or would it be > >>> enough to re-acquire the context? > >>> > >>> CC-ing Juan as the migration maintainer. > >> > >> This is the world backwards. > >> The tradition is that migration people blame block layer people for > >> breaking things and for help, not the other way around O:-) > > > > Sorry, if I didn't provide enough context/explanation. See below for my > > attempt to re-iterate. I CC'ed you, because the issue happens as part of > > snapshot-save and in particular the qemu_mutex_unlock_iothread call in > > qemu_savevm_state is one of the ingredients leading to the problem. > > This was a joke O:-) > > >> To see that I am understading this right: > >> > >> - you create a thread > >> - that calls a memory_region operation > >> - that calls a device write function > >> - that calls the block layer > >> - that creates a snapshot > >> - that calls the migration code > >> - that calls the block layer again > >> > >> Without further investigation, I have no clue what is going on here, > >> sorry. > >> > >> Later, Juan. > >> > > > > All I'm doing is using QEMU (a build of upstream's v8.0.0) in intended > > ways, I promise! In particular, I'm doing two things at the same time > > repeatedly: > > 1. Write to a pflash drive from within the guest. > > 2. Issue a snapshot-save QMP command (in a way that doesn't lead to an > > early error). > > > > (I actually also used a debugger to break on pflash_update and > > snapshot_save_job_bh, manually continuing until I triggered the > > problematic situation. It's very racy, because it depends on the host OS > > to switch threads at the correct time.) > > I think the block layer is the problem (famous last words) > > > > > Now we need to be aware of two things: > > 1. As discussed earlier in the mail thread, if the host OS switches > > threads at an inconvenient time, it can happen that a bottom half > > scheduled for the main thread's AioContext can be executed as part of a > > vCPU thread's aio_poll. > > 2. Generated coroutine wrappers for block layer functions spawn the > > coroutine and use AIO_WAIT_WHILE/aio_poll to wait for it to finish. > > > > What happens in the backtrace above is: > > 1. The write to the pflash drive uses blk_pwrite which leads to an > > aio_poll in the vCPU thread. > > 2. The snapshot_save_job_bh bottom half, that was scheduled for the main > > thread's AioContext, is executed as part of the vCPU thread's aio_poll. > > 3. qemu_savevm_state is called. > > 4. qemu_mutex_unlock_iothread is called. Now > > qemu_get_current_aio_context returns 0x0. Usually, snapshot_save_job_bh > > runs in the main thread, in which case qemu_get_current_aio_context > > still returns the main thread's AioContext at this point. > > I am perhaps a bit ingenuous here, but it is there a way to convince > qemu that snapshot_save_job_bh *HAS* to run on the main thread? I believe we're talking about a technicality here. I asked another more fundamental question that nobody has answered yet: Why do you think that it's ok to call bdrv_writev_vmstate() without holding the BQL? Because if we come to the conclusion that it's not ok (which is what I think), then it doesn't matter whether we violate the condition in the main thread or a vcpu thread. It's wrong in both cases, just the failure mode differs - one crashes spectacularly with an assertion failure, the other has a race condition. Moving from the assertion failure to a race condition is not a proper fix. > > 5. bdrv_writev_vmstate is executed as part of the usual savevm setup. > > 6. bdrv_writev_vmstat
Re: [PATCH v3 3/4] build: move COLO under CONFIG_REPLICATION
On 28.04.23 10:30, Juan Quintela wrote:
Vladimir Sementsov-Ogievskiy wrote:
We don't allow to use x-colo capability when replication is not
configured. So, no reason to build COLO when replication is disabled,
it's unusable in this case.
Note also that the check in migrate_caps_check() is not the only
restriction: some functions in migration/colo.c will just abort if
called with not defined CONFIG_REPLICATION, for example:
migration_iteration_finish()
case MIGRATION_STATUS_COLO:
migrate_start_colo_process()
colo_process_checkpoint()
abort()
It could probably make sense to have possibility to enable COLO without
REPLICATION, but this requires deeper audit of colo & replication code,
which may be done later if needed.
Signed-off-by: Vladimir Sementsov-Ogievskiy
Nice patch. Thanks.
@@ -68,7 +66,6 @@ static bool colo_runstate_is_stopped(void)
static void secondary_vm_do_failover(void)
{
/* COLO needs enable block-replication */
-#ifdef CONFIG_REPLICATION
int old_state;
MigrationIncomingState *mis = migration_incoming_get_current();
Error *local_err = NULL;
@@ -133,14 +130,10 @@ static void secondary_vm_do_failover(void)
if (mis->migration_incoming_co) {
qemu_coroutine_enter(mis->migration_incoming_co);
}
-#else
-abort();
-#endif
}
With only this chunks you have proved that your argument is right.
abort() is never a solution.
diff --git a/migration/options.c b/migration/options.c
index 912cbadddb..eef2bd0f16 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -171,7 +171,9 @@ Property migration_properties[] = {
DEFINE_PROP_MIG_CAP("x-postcopy-ram", MIGRATION_CAPABILITY_POSTCOPY_RAM),
DEFINE_PROP_MIG_CAP("x-postcopy-preempt",
MIGRATION_CAPABILITY_POSTCOPY_PREEMPT),
+#ifdef CONFIG_REPLICATION
DEFINE_PROP_MIG_CAP("x-colo", MIGRATION_CAPABILITY_X_COLO),
+#endif
DEFINE_PROP_MIG_CAP("x-release-ram", MIGRATION_CAPABILITY_RELEASE_RAM),
DEFINE_PROP_MIG_CAP("x-block", MIGRATION_CAPABILITY_BLOCK),
DEFINE_PROP_MIG_CAP("x-return-path", MIGRATION_CAPABILITY_RETURN_PATH),
@@ -209,9 +211,13 @@ bool migrate_block(void)
bool migrate_colo(void)
{
+#ifdef CONFIG_REPLICATION
MigrationState *s = migrate_get_current();
return s->capabilities[MIGRATION_CAPABILITY_X_COLO];
+#else
+return false;
+#endif
}
#ifdef 1
bool migrate_compress(void)
@@ -401,7 +407,9 @@ INITIALIZE_MIGRATE_CAPS_SET(check_caps_background_snapshot,
MIGRATION_CAPABILITY_RDMA_PIN_ALL,
MIGRATION_CAPABILITY_COMPRESS,
MIGRATION_CAPABILITY_XBZRLE,
+#ifdef CONFIG_REPLICATION
MIGRATION_CAPABILITY_X_COLO,
+#endif
MIGRATION_CAPABILITY_VALIDATE_UUID,
MIGRATION_CAPABILITY_ZERO_COPY_SEND);
#ifdef 2
@@ -428,15 +436,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps,
Error **errp)
}
#endif
-#ifndef CONFIG_REPLICATION
-if (new_caps[MIGRATION_CAPABILITY_X_COLO]) {
-error_setg(errp, "QEMU compiled without replication module"
- " can't enable COLO");
-error_append_hint(errp, "Please enable replication before COLO.\n");
-return false;
-}
-#endif
-
if (new_caps[MIGRATION_CAPABILITY_POSTCOPY_RAM]) {
/* This check is reasonably expensive, so only when it's being
* set the first time, also it's only the destination that needs
I would preffer if you removed #ifdef 1 and 2 and let this one in. I am
trying to get all capabilities to this format.
OK, let's start with your idea, interface may be removed later if we want.
diff --git a/stubs/colo.c b/stubs/colo.c
new file mode 100644
index 00..f306ab45d6
--- /dev/null
+++ b/stubs/colo.c
@@ -0,0 +1,37 @@
+#include "qemu/osdep.h"
+#include "qemu/notify.h"
+#include "net/colo-compare.h"
+#include "migration/colo.h"
+#include "migration/migration.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
+
+void colo_shutdown(void)
+{
+abort();
+}
This is wrong, it should be empty.
Oops right. Good catch
void migration_shutdown(void)
{
/*
* When the QEMU main thread exit, the COLO thread
* may wait a semaphore. So, we should wakeup the
* COLO thread before migration shutdown.
*/
colo_shutdown();
..
}
+void *colo_process_incoming_thread(void *opaque)
+{
+abort();
+}
At least print an error message?
OK
+void colo_checkpoint_notify(void *opaque)
+{
+abort();
+}
Another error message.
It is independently of this patch, but I am thinking about changing the
interface and doing something like this in options.c
changing
if (params->has_x_checkpoint_delay) {
s->parameters.x_checkpoint_delay = params->x_checkpoint_delay;
if (migration_in_colo_state()) {
colo_checkpoint_notify(s);
}
}
To
if (params->has_x_checkpoint_de
[PATCH 0/2] migration: Remove unused parameters from tls functions
Hi After reorganization of the capabilities, this two functions don't use the MigrationState parameter anymore, just drop it. Vladimir suggested it while he was doing the review. Later, Juan. Juan Quintela (2): migration: Drop unused parameter for migration_tls_get_creds() migration: Drop unused parameter for migration_tls_client_create() migration/multifd.c | 2 +- migration/postcopy-ram.c | 2 +- migration/tls.c | 15 +-- migration/tls.h | 3 +-- 4 files changed, 8 insertions(+), 14 deletions(-) -- 2.40.0
[PATCH 2/2] migration: Drop unused parameter for migration_tls_client_create()
It is not needed since we moved the accessor for tls properties to
options.c.
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Juan Quintela
---
migration/multifd.c | 2 +-
migration/postcopy-ram.c | 2 +-
migration/tls.c | 5 ++---
migration/tls.h | 3 +--
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 6a59c03dd2..5019a79ff4 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -823,7 +823,7 @@ static void multifd_tls_channel_connect(MultiFDSendParams
*p,
const char *hostname = s->hostname;
QIOChannelTLS *tioc;
-tioc = migration_tls_client_create(s, ioc, hostname, errp);
+tioc = migration_tls_client_create(ioc, hostname, errp);
if (!tioc) {
return;
}
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 75aa276bb1..5615ec29eb 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1632,7 +1632,7 @@ postcopy_preempt_send_channel_new(QIOTask *task, gpointer
opaque)
}
if (migrate_channel_requires_tls_upgrade(ioc)) {
-tioc = migration_tls_client_create(s, ioc, s->hostname, &local_err);
+tioc = migration_tls_client_create(ioc, s->hostname, &local_err);
if (!tioc) {
goto out;
}
diff --git a/migration/tls.c b/migration/tls.c
index d4a76cf590..fa03d9136c 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -114,8 +114,7 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
object_unref(OBJECT(ioc));
}
-QIOChannelTLS *migration_tls_client_create(MigrationState *s,
- QIOChannel *ioc,
+QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
const char *hostname,
Error **errp)
{
@@ -141,7 +140,7 @@ void migration_tls_channel_connect(MigrationState *s,
{
QIOChannelTLS *tioc;
-tioc = migration_tls_client_create(s, ioc, hostname, errp);
+tioc = migration_tls_client_create(ioc, hostname, errp);
if (!tioc) {
return;
}
diff --git a/migration/tls.h b/migration/tls.h
index 98e23c9b0e..5797d153cb 100644
--- a/migration/tls.h
+++ b/migration/tls.h
@@ -28,8 +28,7 @@ void migration_tls_channel_process_incoming(MigrationState *s,
QIOChannel *ioc,
Error **errp);
-QIOChannelTLS *migration_tls_client_create(MigrationState *s,
- QIOChannel *ioc,
+QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
const char *hostname,
Error **errp);
--
2.40.0
[PATCH 1/2] migration: Drop unused parameter for migration_tls_get_creds()
It is not needed since we moved the accessor for tls properties to
options.c.
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Juan Quintela
---
migration/tls.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/migration/tls.c b/migration/tls.c
index cd29177957..d4a76cf590 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -29,9 +29,7 @@
#include "trace.h"
static QCryptoTLSCreds *
-migration_tls_get_creds(MigrationState *s,
-QCryptoTLSCredsEndpoint endpoint,
-Error **errp)
+migration_tls_get_creds(QCryptoTLSCredsEndpoint endpoint, Error **errp)
{
Object *creds;
const char *tls_creds = migrate_tls_creds();
@@ -80,8 +78,7 @@ void migration_tls_channel_process_incoming(MigrationState *s,
QCryptoTLSCreds *creds;
QIOChannelTLS *tioc;
-creds = migration_tls_get_creds(
-s, QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, errp);
+creds = migration_tls_get_creds(QCRYPTO_TLS_CREDS_ENDPOINT_SERVER, errp);
if (!creds) {
return;
}
@@ -124,8 +121,7 @@ QIOChannelTLS *migration_tls_client_create(MigrationState
*s,
{
QCryptoTLSCreds *creds;
-creds = migration_tls_get_creds(
-s, QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, errp);
+creds = migration_tls_get_creds(QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT, errp);
if (!creds) {
return NULL;
}
--
2.40.0
Re: [PATCH 1/2] migration: Drop unused parameter for migration_tls_get_creds()
On 28.04.23 11:34, Juan Quintela wrote: It is not needed since we moved the accessor for tls properties to options.c. Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Juan Quintela Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: QMP (without OOB) function running in thread different from the main thread as part of aio_poll
Kevin Wolf wrote: >> >> I am perhaps a bit ingenuous here, but it is there a way to convince >> qemu that snapshot_save_job_bh *HAS* to run on the main thread? > > I believe we're talking about a technicality here. I asked another more > fundamental question that nobody has answered yet: > > Why do you think that it's ok to call bdrv_writev_vmstate() without > holding the BQL? I will say this function starts by bdrv_ (i.e. block layer people) and endes with _vmstate (i.e. migration people). To be honest, I don't know. That is why I _supposed_ you have an idea. > Because if we come to the conclusion that it's not ok (which is what I > think), then it doesn't matter whether we violate the condition in the > main thread or a vcpu thread. It's wrong in both cases, just the failure > mode differs - one crashes spectacularly with an assertion failure, the > other has a race condition. Moving from the assertion failure to a race > condition is not a proper fix. Fully agree there. Just that I don't know the answer. Later, Juan.
Re: [PATCH 2/2] migration: Drop unused parameter for migration_tls_client_create()
On 28.04.23 11:34, Juan Quintela wrote: It is not needed since we moved the accessor for tls properties to options.c. Suggested-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Juan Quintela Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
RE: [PATCH v3 4/4] configure: add --disable-colo-proxy option
> -Original Message-
> From: Vladimir Sementsov-Ogievskiy
> Sent: Friday, April 28, 2023 5:30 AM
> To: Lukas Straub
> Cc: qemu-devel@nongnu.org; qemu-bl...@nongnu.org;
> michael.r...@amd.com; arm...@redhat.com; ebl...@redhat.com;
> jasow...@redhat.com; quint...@redhat.com; Zhang, Hailiang
> ; phi...@linaro.org; th...@redhat.com;
> berra...@redhat.com; marcandre.lur...@redhat.com;
> pbonz...@redhat.com; d...@treblig.org; hre...@redhat.com;
> kw...@redhat.com; Zhang, Chen ;
> lizhij...@fujitsu.com
> Subject: Re: [PATCH v3 4/4] configure: add --disable-colo-proxy option
>
> On 28.04.23 00:18, Lukas Straub wrote:
> > On Thu, 27 Apr 2023 23:29:46 +0300
> > Vladimir Sementsov-Ogievskiy wrote:
> >
> >> Add option to not build filter-mirror, filter-rewriter and
> >> colo-compare when they are not needed.
> >>
> >> There could be more agile configuration, for example add separate
> >> options for each filter, but that may be done in future on demand.
> >> The aim of this patch is to make possible to disable the whole COLO
> >> Proxy subsystem.
> >>
> >> Signed-off-by: Vladimir
> >> Sementsov-Ogievskiy
> >> ---
> >> meson_options.txt | 2 ++
> >> net/meson.build | 14 ++
> >> scripts/meson-buildoptions.sh | 3 +++
> >> stubs/colo-compare.c | 7 +++
> >> stubs/meson.build | 1 +
> >> 5 files changed, 23 insertions(+), 4 deletions(-)
> >> create mode 100644 stubs/colo-compare.c
> >>
> >> diff --git a/meson_options.txt b/meson_options.txt index
> >> 2471dd02da..b59e7ae342 100644
> >> --- a/meson_options.txt
> >> +++ b/meson_options.txt
> >> @@ -289,6 +289,8 @@ option('live_block_migration', type: 'feature',
> value: 'auto',
> >> description: 'block migration in the main migration stream')
> >> option('replication', type: 'feature', value: 'auto',
> >> description: 'replication support')
> >> +option('colo_proxy', type: 'feature', value: 'auto',
> >> + description: 'colo-proxy support')
> >> option('bochs', type: 'feature', value: 'auto',
> >> description: 'bochs image format support')
> >> option('cloop', type: 'feature', value: 'auto', diff --git
> >> a/net/meson.build b/net/meson.build index 87afca3e93..4cfc850c69
> >> 100644
> >> --- a/net/meson.build
> >> +++ b/net/meson.build
> >> @@ -1,13 +1,9 @@
> >> softmmu_ss.add(files(
> >> 'announce.c',
> >> 'checksum.c',
> >> - 'colo-compare.c',
> >> - 'colo.c',
> >> 'dump.c',
> >> 'eth.c',
> >> 'filter-buffer.c',
> >> - 'filter-mirror.c',
Need fix here for filter-mirror.c too.
> >> - 'filter-rewriter.c',
> >> 'filter.c',
> >> 'hub.c',
> >> 'net-hmp-cmds.c',
> >> @@ -19,6 +15,16 @@ softmmu_ss.add(files(
> >> 'util.c',
> >> ))
> >>
> >> +if get_option('replication').allowed() or \
> >> +get_option('colo_proxy').allowed()
> >> + softmmu_ss.add(files('colo-compare.c'))
> >> + softmmu_ss.add(files('colo.c'))
> >> +endif
> >> +
> >> +if get_option('colo_proxy').allowed()
> >> + softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
> >> +endif
> >> +
> > The last discussion didn't really come to a conclusion, but I still
> > think that 'filter-mirror.c' (which also contains filter-redirect)
> > should be left unchanged.
> >
>
> OK for me, I'll wait a bit for more comments and resend with
>
> @@ -22,7 +22,7 @@ if get_option('replication').allowed() or \
>endif
>
>if get_option('colo_proxy').allowed()
> - softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
> + softmmu_ss.add(files('filter-rewriter.c'))
>endif
>
>softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>
>
> applied here, if no other strong opinion.
>
It's OK to me except for the filter-mirror.c related comments.
Thanks
Chen
> --
> Best regards,
> Vladimir
Re: [PATCH v3 4/4] configure: add --disable-colo-proxy option
Vladimir Sementsov-Ogievskiy wrote:
> On 28.04.23 10:33, Juan Quintela wrote:
>> Vladimir Sementsov-Ogievskiy wrote:
>>> Add option to not build filter-mirror, filter-rewriter and
>>> colo-compare when they are not needed.
>>>
>>> There could be more agile configuration, for example add separate
>>> options for each filter, but that may be done in future on demand. The
>>> aim of this patch is to make possible to disable the whole COLO Proxy
>>> subsystem.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> As you have arrived to an agreement about what to do with
>> filter-rewriter, the rest of the patch is ok with me.
>
> You mean filter-mirror, precisely this change to the patch:
>
> @@ -22,7 +22,7 @@ if get_option('replication').allowed() or \
> endif
> if get_option('colo_proxy').allowed()
> - softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
> + softmmu_ss.add(files('filter-rewriter.c'))
> endif
> softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>
> ?
Oops, yes.
>> Reviewed-by: Juan Quintela
>>
Re: [PATCH v3 4/4] configure: add --disable-colo-proxy option
Vladimir Sementsov-Ogievskiy wrote:
> On 28.04.23 10:33, Juan Quintela wrote:
>> Vladimir Sementsov-Ogievskiy wrote:
>>> Add option to not build filter-mirror, filter-rewriter and
>>> colo-compare when they are not needed.
>>>
>>> There could be more agile configuration, for example add separate
>>> options for each filter, but that may be done in future on demand. The
>>> aim of this patch is to make possible to disable the whole COLO Proxy
>>> subsystem.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> As you have arrived to an agreement about what to do with
>> filter-rewriter, the rest of the patch is ok with me.
>
> You mean filter-mirror, precisely this change to the patch:
>
> @@ -22,7 +22,7 @@ if get_option('replication').allowed() or \
> endif
> if get_option('colo_proxy').allowed()
> - softmmu_ss.add(files('filter-mirror.c', 'filter-rewriter.c'))
> + softmmu_ss.add(files('filter-rewriter.c'))
> endif
> softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('filter-replay.c'))
>
> ?
Oops, yes.
>> Reviewed-by: Juan Quintela
>>
Re: [PATCH 07/19] migration/rdma: Unflod ram_control_before_iterate()
Juan Quintela wrote: > Once there: > - Remove unused data parameter > - unfold it in its callers. > - change all callers to call qemu_rdma_registration_start() > > Signed-off-by: Juan Quintela self-Nack from here. I just break the case when there is CONFIG_RDMA but it is not being used. Later, Juan.
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
On 230428 1015, Thomas Huth wrote:
> On 28/04/2023 10.12, Daniel P. Berrangé wrote:
> > On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:
> > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > This flag is set/checked prior to calling a device's MemoryRegion
> > > handlers, and set when device code initiates DMA. The purpose of this
> > > flag is to prevent two types of DMA-based reentrancy issues:
> > >
> > > 1.) mmio -> dma -> mmio case
> > > 2.) bh -> dma write -> mmio case
> > >
> > > These issues have led to problems such as stack-exhaustion and
> > > use-after-frees.
> > >
> > > Summary of the problem from Peter Maydell:
> > > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> > > Resolves: CVE-2023-0330
> > >
> > > Signed-off-by: Alexander Bulekov
> > > Reviewed-by: Thomas Huth
> > > ---
> > > include/exec/memory.h | 5 +
> > > include/hw/qdev-core.h | 7 +++
> > > softmmu/memory.c | 16
> > > 3 files changed, 28 insertions(+)
> > >
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index 15ade918ba..e45ce6061f 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -767,6 +767,8 @@ struct MemoryRegion {
> > > bool is_iommu;
> > > RAMBlock *ram_block;
> > > Object *owner;
> > > +/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access
> > > hotpath */
> > > +DeviceState *dev;
> > > const MemoryRegionOps *ops;
> > > void *opaque;
> > > @@ -791,6 +793,9 @@ struct MemoryRegion {
> > > unsigned ioeventfd_nb;
> > > MemoryRegionIoeventfd *ioeventfds;
> > > RamDiscardManager *rdm; /* Only for RAM */
> > > +
> > > +/* For devices designed to perform re-entrant IO into their own IO
> > > MRs */
> > > +bool disable_reentrancy_guard;
> > > };
> > > struct IOMMUMemoryRegion {
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index bd50ad5ee1..7623703943 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -162,6 +162,10 @@ struct NamedClockList {
> > > QLIST_ENTRY(NamedClockList) node;
> > > };
> > > +typedef struct {
> > > +bool engaged_in_io;
> > > +} MemReentrancyGuard;
> > > +
> > > /**
> > >* DeviceState:
> > >* @realized: Indicates whether the device has been fully constructed.
> > > @@ -194,6 +198,9 @@ struct DeviceState {
> > > int alias_required_for_version;
> > > ResettableState reset;
> > > GSList *unplug_blockers;
> > > +
> > > +/* Is the device currently in mmio/pio/dma? Used to prevent
> > > re-entrancy */
> > > +MemReentrancyGuard mem_reentrancy_guard;
> > > };
> > > struct DeviceListener {
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index b1a6cae6f5..fe23f0e5ce 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr
> > > addr,
> > > access_size_max = 4;
> > > }
> > > +/* Do not allow more than one simultaneous access to a device's IO
> > > Regions */
> > > +if (mr->dev && !mr->disable_reentrancy_guard &&
> > > +!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly)
> > > {
> > > +if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
> > > +warn_report("Blocked re-entrant IO on "
> > > +"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
> > > +memory_region_name(mr), addr);
> > > +return MEMTX_ACCESS_ERROR;
> >
> > If we issue this warn_report on every invalid memory access, is this
> > going to become a denial of service by flooding logs, or is the
> > return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
> > *once* in the lifetime of the QEMU process ?
>
> Maybe it's better to use warn_report_once() here instead?
Sounds good - should I respin the series to change this?
-Alex
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
On 28/04/2023 11.11, Alexander Bulekov wrote:
On 230428 1015, Thomas Huth wrote:
On 28/04/2023 10.12, Daniel P. Berrangé wrote:
On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
include/exec/memory.h | 5 +
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 16
3 files changed, 28 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..fe23f0e5ce 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions
*/
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+warn_report("Blocked re-entrant IO on "
+"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
+memory_region_name(mr), addr);
+return MEMTX_ACCESS_ERROR;
If we issue this warn_report on every invalid memory access, is this
going to become a denial of service by flooding logs, or is the
return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
*once* in the lifetime of the QEMU process ?
Maybe it's better to use warn_report_once() here instead?
Sounds good - should I respin the series to change this?
Not necessary, I've got v10 already queued, I'll fix it up there
Thomas
Re: [PATCH 2/2] target/riscv/vector_helper.c: make vext_set_tail_elems_1s() debug only
On 4/27/23 22:22, Weiwei Li wrote:
On 2023/4/28 04:57, Daniel Henrique Barboza wrote:
Commit 3479a814 ("target/riscv: rvv-1.0: add VMA and VTA") added vma and
vta fields in the vtype register, while also defining that QEMU doesn't
need to have a tail agnostic policy to be compliant with the RVV spec.
It ended up removing all tail handling code as well. Later, commit
752614ca ("target/riscv: rvv: Add tail agnostic for vector load / store
instructions") reintroduced the tail agnostic fill for vector load/store
instructions only.
This puts QEMU in a situation where some functions are 1-filling the
tail elements and others don't. This is still a valid implementation,
but the process of 1-filling the tail elements takes valuable emulation
time that can be used doing anything else. If the spec doesn't demand a
specific tail-agostic policy, a proper software wouldn't expect any
policy to be in place. This means that, more often than not, the work
we're doing by 1-filling tail elements is wasted. We would be better of
if vext_set_tail_elems_1s() is removed entirely from the code.
All this said, there's still a debug value associated with it. So,
instead of removing it, let's gate it with cpu->cfg.debug. This way
software can enable this code if desirable, but for the regular case we
shouldn't waste time with it.
Signed-off-by: Daniel Henrique Barboza
---
target/riscv/vector_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 8e6c99e573..e0a292ac24 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -272,7 +272,7 @@ static void vext_set_tail_elems_1s(CPURISCVState *env,
target_ulong vl,
uint32_t vta = vext_vta(desc);
int k;
- if (vta == 0) {
+ if (vta == 0 || !riscv_cpu_cfg(env)->debug) {
I think this is not correct. 'debug' property is used for debug spec. And this
feature is controlled by another property 'rvv_ta_all_1s' .
You're right. I wasn't aware that this flag exists:
$ git grep 'rvv_ta_all_1s'
target/riscv/cpu.c:DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU,
cfg.rvv_ta_all_1s, false),
target/riscv/cpu.h:bool rvv_ta_all_1s;
target/riscv/translate.c:ctx->vta = FIELD_EX32(tb_flags, TB_FLAGS, VTA) &&
cpu->cfg.rvv_ta_all_1s;
target/riscv/translate.c:ctx->cfg_vta_all_1s = cpu->cfg.rvv_ta_all_1s;
By the way, cfg.rvv_ta_all_1s have been ANDed intovta value. So additional
check on it is also unnecessary here.
Yes. We can drop this patch then since 'vta' is already accounting for
ta_all_1s.
Thanks,
Daniel
Regards,
Weiwei Li
return;
}
Re: [PATCH 0/2] target/riscv: RVV 1-fill tail element changes
On Thu, Apr 27, 2023 at 17:57:06 -0300, Daniel Henrique Barboza wrote: : Second patch makes the function debug only. The logic is explained in : the commit message, but long story short: we don't have to implement any : tail-agnostic policy at all to be spec compliant, but this function has : its uses for debug purposes, so keeping it as a debug option allow users : to disable it on demand. Yes, please don't remove this functionality completely -- our internal stress test tool for the vector crypto patches relies on it. -- Dickon HoodSenior Software Engineer Codethink Ltd. 3rd Floor, Dale House, 35 Dale Street, https://www.codethink.co.uk/Manchester, M1 2HF, United Kingdom.
[PATCH 1/2] target: riscv: fix typos
Fix a few minor typos for PMU events.
Signed-off-by: Yu Chien Peter Lin
---
target/riscv/cpu.h| 2 +-
target/riscv/cpu_helper.c | 2 +-
target/riscv/pmu.c| 8
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..eab518542c 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -812,7 +812,7 @@ enum riscv_pmu_event_idx {
RISCV_PMU_EVENT_HW_INSTRUCTIONS = 0x02,
RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS = 0x10019,
RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS = 0x1001B,
-RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS = 0x10021,
+RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS = 0x10021,
};
/* CSR function table */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..5d3e032ec9 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -1210,7 +1210,7 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu,
MMUAccessType access_type)
switch (access_type) {
case MMU_INST_FETCH:
-pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS;
+pmu_event_type = RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS;
break;
case MMU_DATA_LOAD:
pmu_event_type = RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS;
diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
index fa1e1484c2..0be0e8027b 100644
--- a/target/riscv/pmu.c
+++ b/target/riscv/pmu.c
@@ -62,17 +62,17 @@ void riscv_pmu_generate_fdt_node(void *fdt, int num_ctrs,
char *pmu_name)
fdt_event_ctr_map[4] = cpu_to_be32(0x0002);
fdt_event_ctr_map[5] = cpu_to_be32(cmask | 1 << 2);
- /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x00 type(0x01) */
+ /* SBI_PMU_HW_CACHE_DTLB : 0x03 READ : 0x00 MISS : 0x01 type(0x01) */
fdt_event_ctr_map[6] = cpu_to_be32(0x00010019);
fdt_event_ctr_map[7] = cpu_to_be32(0x00010019);
fdt_event_ctr_map[8] = cpu_to_be32(cmask);
- /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x00 type(0x01) */
+ /* SBI_PMU_HW_CACHE_DTLB : 0x03 WRITE : 0x01 MISS : 0x01 type(0x01) */
fdt_event_ctr_map[9] = cpu_to_be32(0x0001001B);
fdt_event_ctr_map[10] = cpu_to_be32(0x0001001B);
fdt_event_ctr_map[11] = cpu_to_be32(cmask);
- /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x00 type(0x01) */
+ /* SBI_PMU_HW_CACHE_ITLB : 0x04 READ : 0x00 MISS : 0x01 type(0x01) */
fdt_event_ctr_map[12] = cpu_to_be32(0x00010021);
fdt_event_ctr_map[13] = cpu_to_be32(0x00010021);
fdt_event_ctr_map[14] = cpu_to_be32(cmask);
@@ -317,7 +317,7 @@ int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t
value,
case RISCV_PMU_EVENT_HW_INSTRUCTIONS:
case RISCV_PMU_EVENT_CACHE_DTLB_READ_MISS:
case RISCV_PMU_EVENT_CACHE_DTLB_WRITE_MISS:
-case RISCV_PMU_EVENT_CACHE_ITLB_PREFETCH_MISS:
+case RISCV_PMU_EVENT_CACHE_ITLB_READ_MISS:
break;
default:
/* We don't support any raw events right now */
--
2.34.1
Re: [PATCH 17/16] docs/devel/qapi-code-gen: Describe some doc markup pitfalls
Vladimir Sementsov-Ogievskiy writes: > On 27.04.23 12:53, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster >> --- >> docs/devel/qapi-code-gen.rst | 53 >> 1 file changed, 53 insertions(+) >> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst >> index d81aac7a19..14983b074c 100644 >> --- a/docs/devel/qapi-code-gen.rst >> +++ b/docs/devel/qapi-code-gen.rst >> @@ -1059,6 +1059,59 @@ For example:: >> 'returns': ['BlockStats'] } >> +Markup pitfalls >> +~~~ >> + >> +A blank line is required between list items and paragraphs. Without >> +it, the list may not be recognized, resulting in garbled output. Good >> +example:: >> + >> + # An event's state is modified if: >> + # >> + # - its name matches the @name pattern, and >> + # - if @vcpu is given, the event has the "vcpu" property. >> + >> +Without the blank line this would be a single paragraph. >> + >> +Indentation matters. Bad example:: >> + >> + # @none: None (no memory side cache in this proximity domain, >> + # or cache associativity unknown) >> + >> +The description is parsed as a definition list with term "None (no >> +memory side cache in this proximity domain," and definition "or cache >> +associativity unknown)". > > May be add good example of indentation as well Patches I'm about to post will fill up this pitfall. They change the text to: # @none: None (no memory side cache in this proximity domain, # or cache associativity unknown) # (since 5.0) The last line's de-indent is wrong. The second and subsequent lines need to line up with each other, like this:: # @none: None (no memory side cache in this proximity domain, # or cache associativity unknown) # (since 5.0) Good enough? [...]
[PATCH 2/2] hw/riscv: virt: fix pmu subnode paths
The pmu encodes the event to counter mappings and is only used
by the SBI firmware. Currently, pmu is a subnode of soc but has
no reg properties included, causing the following failure when
checked with dt-validate.
/tmp/virt.dtb: soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2,
2, 524284], [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569,
524280]], 'compatible': ['riscv,pmu']} should not be valid under {'type':
'object'}
From schema:
/home/peterlin/.local/lib/python3.10/site-packages/dtschema/schemas/simple-bus.yaml
This patch moves the pmu to top level to make the dt-validate happy.
Signed-off-by: Yu Chien Peter Lin
---
hw/riscv/virt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 4e3efbee16..be8f0cb26e 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -731,7 +731,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
MachineState *ms = MACHINE(s);
RISCVCPU hart = s->soc[0].harts[0];
-pmu_name = g_strdup_printf("/soc/pmu");
+pmu_name = g_strdup_printf("/pmu");
qemu_fdt_add_subnode(ms->fdt, pmu_name);
qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
--
2.34.1
Re: [PATCH 00/16] qapi qga/qapi-schema: Doc fixes
Vladimir Sementsov-Ogievskiy writes: > On 04.04.23 14:58, Markus Armbruster wrote: >> It's always nice to get doc fixes into the release, but if it's too >> late, it's too late. >> Generated code does not change, except for the last patch, which moves >> a bit of code without changing it. > > > I didn't deeply check the details, but looked through and nothing seems wrong > to me. Good cleanup! > > all patches: > Reviewed-by: Vladimir Sementsov-Ogievskiy Thanks! (Belatedly) > PS: do you plan some automatic checks in build process to avoid similar > style/naming problems in future? I'm about to post patches that make it somewhat harder to screw up indentation (PATCH 10 fixes such screwups). Harder, not impossible, because indentation is meaningful in rST. I believe the best way to catch argument description screwups is making argument descriptions mandatory. I hope to find time for that. Other than that, no promising ideas, I'm afraid.
[PULL 01/13] s390x/gdb: Split s390-virt.xml
From: Ilya Leoshkevich
Both TCG and KVM emulate ckc, cputm, last_break and prefix, and it's
quite useful to have them during debugging. Right now they are grouped
together with KVM-only pp, pfault_token, pfault_select and
pfault_compare in s390-virt.xml, and are not available when debugging
TCG-emulated code.
Move KVM-only registers into the new s390-virt-kvm.xml file. Advertise
s390-virt.xml always, and the new s390-virt-kvm.xml only for KVM.
Signed-off-by: Ilya Leoshkevich
Message-Id: <20230314101813.174874-1-...@linux.ibm.com>
Acked-by: David Hildenbrand
Signed-off-by: Thomas Huth
---
configs/targets/s390x-linux-user.mak | 2 +-
configs/targets/s390x-softmmu.mak| 2 +-
target/s390x/gdbstub.c | 65 +++-
gdb-xml/s390-virt-kvm.xml| 14 ++
gdb-xml/s390-virt.xml| 4 --
5 files changed, 61 insertions(+), 26 deletions(-)
create mode 100644 gdb-xml/s390-virt-kvm.xml
diff --git a/configs/targets/s390x-linux-user.mak
b/configs/targets/s390x-linux-user.mak
index e2978248ed..24c04c8589 100644
--- a/configs/targets/s390x-linux-user.mak
+++ b/configs/targets/s390x-linux-user.mak
@@ -2,4 +2,4 @@ TARGET_ARCH=s390x
TARGET_SYSTBL_ABI=common,64
TARGET_SYSTBL=syscall.tbl
TARGET_BIG_ENDIAN=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-kvm.xml gdb-xml/s390-gs.xml
diff --git a/configs/targets/s390x-softmmu.mak
b/configs/targets/s390x-softmmu.mak
index 258b4cf358..70d2f9f0ba 100644
--- a/configs/targets/s390x-softmmu.mak
+++ b/configs/targets/s390x-softmmu.mak
@@ -1,4 +1,4 @@
TARGET_ARCH=s390x
TARGET_BIG_ENDIAN=y
TARGET_SUPPORTS_MTTCG=y
-TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-gs.xml
+TARGET_XML_FILES= gdb-xml/s390x-core64.xml gdb-xml/s390-acr.xml
gdb-xml/s390-fpr.xml gdb-xml/s390-vx.xml gdb-xml/s390-cr.xml
gdb-xml/s390-virt.xml gdb-xml/s390-virt-kvm.xml gdb-xml/s390-gs.xml
diff --git a/target/s390x/gdbstub.c b/target/s390x/gdbstub.c
index 0cb69395b4..6fbfd41bc8 100644
--- a/target/s390x/gdbstub.c
+++ b/target/s390x/gdbstub.c
@@ -206,12 +206,8 @@ static int cpu_write_c_reg(CPUS390XState *env, uint8_t
*mem_buf, int n)
#define S390_VIRT_CPUTM_REGNUM 1
#define S390_VIRT_BEA_REGNUM2
#define S390_VIRT_PREFIX_REGNUM 3
-#define S390_VIRT_PP_REGNUM 4
-#define S390_VIRT_PFT_REGNUM5
-#define S390_VIRT_PFS_REGNUM6
-#define S390_VIRT_PFC_REGNUM7
/* total number of registers in s390-virt.xml */
-#define S390_NUM_VIRT_REGS 8
+#define S390_NUM_VIRT_REGS 4
static int cpu_read_virt_reg(CPUS390XState *env, GByteArray *mem_buf, int n)
{
@@ -224,14 +220,6 @@ static int cpu_read_virt_reg(CPUS390XState *env,
GByteArray *mem_buf, int n)
return gdb_get_regl(mem_buf, env->gbea);
case S390_VIRT_PREFIX_REGNUM:
return gdb_get_regl(mem_buf, env->psa);
-case S390_VIRT_PP_REGNUM:
-return gdb_get_regl(mem_buf, env->pp);
-case S390_VIRT_PFT_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_token);
-case S390_VIRT_PFS_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_select);
-case S390_VIRT_PFC_REGNUM:
-return gdb_get_regl(mem_buf, env->pfault_compare);
default:
return 0;
}
@@ -256,19 +244,51 @@ static int cpu_write_virt_reg(CPUS390XState *env, uint8_t
*mem_buf, int n)
env->psa = ldtul_p(mem_buf);
cpu_synchronize_post_init(env_cpu(env));
return 8;
-case S390_VIRT_PP_REGNUM:
+default:
+return 0;
+}
+}
+
+/* the values represent the positions in s390-virt-kvm.xml */
+#define S390_VIRT_KVM_PP_REGNUM 0
+#define S390_VIRT_KVM_PFT_REGNUM1
+#define S390_VIRT_KVM_PFS_REGNUM2
+#define S390_VIRT_KVM_PFC_REGNUM3
+/* total number of registers in s390-virt-kvm.xml */
+#define S390_NUM_VIRT_KVM_REGS 4
+
+static int cpu_read_virt_kvm_reg(CPUS390XState *env, GByteArray *mem_buf, int
n)
+{
+switch (n) {
+case S390_VIRT_KVM_PP_REGNUM:
+return gdb_get_regl(mem_buf, env->pp);
+case S390_VIRT_KVM_PFT_REGNUM:
+return gdb_get_regl(mem_buf, env->pfault_token);
+case S390_VIRT_KVM_PFS_REGNUM:
+return gdb_get_regl(mem_buf, env->pfault_select);
+case S390_VIRT_KVM_PFC_REGNUM:
+return gdb_get_regl(mem_buf, env->pfault_compare);
+default:
+return 0;
+}
+}
+
+static int cpu_write_virt_kvm_reg(CPUS390XState *env, uint8_t *mem_buf, int n)
+{
+switch (n) {
+case S390_VIRT_KVM_PP_REGNUM:
env->pp = ldtul_p(mem_buf);
cpu_synchronize_post_init(env_cpu(env));
return 8;
-case S390_VIRT_PFT_REGNUM:
+case
[PULL 07/13] async: Add an optional reentrancy guard to the BH API
From: Alexander Bulekov
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
Message-Id: <20230427211013.2994127-3-alx...@bu.edu>
[thuth: Fix "line over 90 characters" checkpatch.pl error]
Signed-off-by: Thomas Huth
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 6 --
util/trace-events | 1 +
7 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType typ
[PULL 05/13] tests: vhost-user-test: release mutex on protocol violation
From: Paolo Bonzini
chr_read() is printing an error message and returning with s->data_mutex taken.
This can potentially cause a hang. Reported by Coverity.
Signed-off-by: Paolo Bonzini
Message-Id: <20230427125423.103536-1-pbonz...@redhat.com>
Reviewed-by: Thomas Huth
Signed-off-by: Thomas Huth
---
tests/qtest/vhost-user-test.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index bf9f7c4248..e4f95b2858 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -351,7 +351,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
if (size != msg.size) {
qos_printf("%s: Wrong message size received %d != %d\n",
__func__, size, msg.size);
-return;
+goto out;
}
}
@@ -509,6 +509,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int
size)
break;
}
+out:
g_mutex_unlock(&s->data_mutex);
}
--
2.31.1
[PULL 08/13] checkpatch: add qemu_bh_new/aio_bh_new checks
From: Alexander Bulekov
Advise authors to use the _guarded versions of the APIs, instead.
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
Message-Id: <20230427211013.2994127-4-alx...@bu.edu>
Signed-off-by: Thomas Huth
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers;
signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of
qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new()
to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc.
instead of module_init()\n" . $herecurr);
--
2.31.1
[PULL 02/13] hw/rdma: Remove unused macros PG_DIR_SZ and PG_TBL_SZ
They have apparently never been used.
Message-Id: <20230419103018.627115-1-th...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Thomas Huth
---
hw/rdma/rdma_rm.c | 4
1 file changed, 4 deletions(-)
diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index cfd85de3e6..038d564433 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -23,10 +23,6 @@
#include "rdma_backend.h"
#include "rdma_rm.h"
-/* Page directory and page tables */
-#define PG_DIR_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
-#define PG_TBL_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
-
void rdma_format_device_counters(RdmaDeviceResources *dev_res, GString *buf)
{
g_string_append_printf(buf, "\ttx : %" PRId64 "\n",
--
2.31.1
[PULL 04/13] hw/rdma: VMW_PVRDMA should depend on VMXNET3_PCI
The "pvrdma" device is only usable in conjunction with the "vmxnet3" NIC - see the check for TYPE_VMXNET3 in pvrdma_realize(). By adding this dependency, the amount of total files that have to be compiled for a configuration with all targets decreases by 64 files (!), since the rdma code is marked as target specific and thus got recompiled for all targets that enable PCI so far. Message-Id: <20230419111337.651673-1-th...@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Thomas Huth --- hw/rdma/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/rdma/Kconfig b/hw/rdma/Kconfig index 8e2211288f..840320bdc0 100644 --- a/hw/rdma/Kconfig +++ b/hw/rdma/Kconfig @@ -1,3 +1,3 @@ config VMW_PVRDMA default y if PCI_DEVICES -depends on PVRDMA && PCI && MSI_NONBROKEN +depends on PVRDMA && MSI_NONBROKEN && VMXNET3_PCI -- 2.31.1
[PULL 13/13] apic: disable reentrancy detection for apic-msi
From: Alexander Bulekov As the code is designed for re-entrant calls to apic-msi, mark apic-msi as reentrancy-safe. Signed-off-by: Alexander Bulekov Reviewed-by: Darren Kenny Message-Id: <20230427211013.2994127-9-alx...@bu.edu> Signed-off-by: Thomas Huth --- hw/intc/apic.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 20b5a94073..ac3d47d231 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -885,6 +885,13 @@ static void apic_realize(DeviceState *dev, Error **errp) memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi", APIC_SPACE_SIZE); +/* + * apic-msi's apic_mem_write can call into ioapic_eoi_broadcast, which can + * write back to apic-msi. As such mark the apic-msi region re-entrancy + * safe. + */ +s->io_memory.disable_reentrancy_guard = true; + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s); local_apics[s->id] = s; -- 2.31.1
[PULL 06/13] memory: prevent dma-reentracy issues
From: Alexander Bulekov
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
Message-Id: <20230427211013.2994127-2-alx...@bu.edu>
[thuth: Replace warn_report() with warn_report_once()]
Signed-off-by: Thomas Huth
---
include/exec/memory.h | 5 +
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 16
3 files changed, 28 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..b7b3386e9d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions
*/
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+warn_report_once("Blocked re-entrant IO on MemoryRegion: "
+ "%s at addr: 0x%" HWADDR_PRIX,
+ memory_region_name(mr), addr);
+return MEMTX_ACCESS_ERROR;
+}
+mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+}
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+if (mr->dev) {
+mr->dev->mem_reentrancy_guard.engaged_in_io = false;
+}
return r;
}
@@ -1170,6 +1185,7 @@ static void memory_region_do_init(MemoryRegion *mr,
}
mr->name = g_strdup(name);
mr->owner = owner;
+mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
mr->ram_block = NULL;
if (name) {
--
2.31.1
[PULL 09/13] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
From: Alexander Bulekov
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Reviewed-by: Thomas Huth
Message-Id: <20230427211013.2994127-5-alx...@bu.edu>
Signed-off-by: Thomas Huth
---
hw/ide/ahci_internal.h | 1 +
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 303fcd7235..2480455372 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -321,6 +321,7 @@ struct AHCIDevice {
bool init_d2h_sent;
AHCICmdHdr *cur_cmd;
NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
+MemReentrancyGuard mem_reentrancy_guard;
};
struct AHCIPCIState {
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_
[PULL 11/13] bcm2835_property: disable reentrancy detection for iomem
From: Alexander Bulekov As the code is designed for re-entrant calls from bcm2835_property to bcm2835_mbox and back into bcm2835_property, mark iomem as reentrancy-safe. Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth Message-Id: <20230427211013.2994127-7-alx...@bu.edu> Signed-off-by: Thomas Huth --- hw/misc/bcm2835_property.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 890ae7bae5..de056ea2df 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -382,6 +382,13 @@ static void bcm2835_property_init(Object *obj) memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_property_ops, s, TYPE_BCM2835_PROPERTY, 0x10); + +/* + * bcm2835_property_ops call into bcm2835_mbox, which in-turn reads from + * iomem. As such, mark iomem as re-entracy safe. + */ +s->iomem.disable_reentrancy_guard = true; + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(s), &s->mbox_irq); } -- 2.31.1
[PULL 00/13] DMA reentrancy fixes and other misc patches
Hi Richard! The following changes since commit cc5ee50fff9dbac0aac32cd892a7163c7babcca1: Merge tag 'pull-testing-docs-270423-1' of https://gitlab.com/stsquad/qemu into staging (2023-04-27 16:46:17 +0100) are available in the Git repository at: https://gitlab.com/thuth/qemu.git tags/pull-request-2023-04-28 for you to fetch changes up to 50795ee051a342c681a9b45671c552fbd6274db8: apic: disable reentrancy detection for apic-msi (2023-04-28 11:31:54 +0200) * Prevent reentrant DMA accesses by default * Only compile hw/rdma code when necessary * Fix a potential locking issue in the vhost-user-test * Offer more registers in GDB for s390x TCG Alexander Bulekov (8): memory: prevent dma-reentracy issues async: Add an optional reentrancy guard to the BH API checkpatch: add qemu_bh_new/aio_bh_new checks hw: replace most qemu_bh_new calls with qemu_bh_new_guarded lsi53c895a: disable reentrancy detection for script RAM bcm2835_property: disable reentrancy detection for iomem raven: disable reentrancy detection for iomem apic: disable reentrancy detection for apic-msi Ilya Leoshkevich (1): s390x/gdb: Split s390-virt.xml Paolo Bonzini (1): tests: vhost-user-test: release mutex on protocol violation Thomas Huth (3): hw/rdma: Remove unused macros PG_DIR_SZ and PG_TBL_SZ hw/rdma: Compile target-independent parts of the rdma code only once hw/rdma: VMW_PVRDMA should depend on VMXNET3_PCI docs/devel/multiple-iothreads.txt| 7 configs/targets/s390x-linux-user.mak | 2 +- configs/targets/s390x-softmmu.mak| 2 +- hw/ide/ahci_internal.h | 1 + include/block/aio.h | 18 -- include/exec/memory.h| 5 +++ include/hw/qdev-core.h | 7 include/qemu/main-loop.h | 7 ++-- hw/9pfs/xen-9p-backend.c | 5 ++- hw/block/dataplane/virtio-blk.c | 3 +- hw/block/dataplane/xen-block.c | 5 +-- hw/char/virtio-serial-bus.c | 3 +- hw/display/qxl.c | 9 +++-- hw/display/virtio-gpu.c | 6 ++-- hw/ide/ahci.c| 3 +- hw/ide/core.c| 4 ++- hw/intc/apic.c | 7 hw/misc/bcm2835_property.c | 7 hw/misc/imx_rngc.c | 6 ++-- hw/misc/macio/mac_dbdma.c| 2 +- hw/net/virtio-net.c | 3 +- hw/nvme/ctrl.c | 6 ++-- hw/pci-host/raven.c | 7 hw/rdma/rdma_rm.c| 4 --- hw/scsi/lsi53c895a.c | 6 hw/scsi/mptsas.c | 3 +- hw/scsi/scsi-bus.c | 3 +- hw/scsi/vmw_pvscsi.c | 3 +- hw/usb/dev-uas.c | 3 +- hw/usb/hcd-dwc2.c| 3 +- hw/usb/hcd-ehci.c| 3 +- hw/usb/hcd-uhci.c| 2 +- hw/usb/host-libusb.c | 6 ++-- hw/usb/redirect.c| 6 ++-- hw/usb/xen-usb.c | 3 +- hw/virtio/virtio-balloon.c | 5 +-- hw/virtio/virtio-crypto.c| 3 +- softmmu/memory.c | 16 + target/s390x/gdbstub.c | 65 +--- tests/qtest/vhost-user-test.c| 3 +- tests/unit/ptimer-test-stubs.c | 3 +- util/async.c | 18 +- util/main-loop.c | 6 ++-- gdb-xml/s390-virt-kvm.xml| 14 gdb-xml/s390-virt.xml| 4 --- hw/rdma/Kconfig | 2 +- hw/rdma/meson.build | 8 +++-- scripts/checkpatch.pl| 8 + util/trace-events| 1 + 49 files changed, 250 insertions(+), 76 deletions(-) create mode 100644 gdb-xml/s390-virt-kvm.xml
[PULL 03/13] hw/rdma: Compile target-independent parts of the rdma code only once
Some files of the rdma code do not depend on any target specific macros. Compile these only once to save some time during the build. Message-Id: <20230419114937.667221-1-th...@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Thomas Huth --- hw/rdma/meson.build | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/rdma/meson.build b/hw/rdma/meson.build index 7325f40c32..fc7917192f 100644 --- a/hw/rdma/meson.build +++ b/hw/rdma/meson.build @@ -1,10 +1,12 @@ -specific_ss.add(when: 'CONFIG_VMW_PVRDMA', if_true: files( +softmmu_ss.add(when: 'CONFIG_VMW_PVRDMA', if_true: files( 'rdma.c', 'rdma_backend.c', - 'rdma_rm.c', 'rdma_utils.c', + 'vmw/pvrdma_qp_ops.c', +)) +specific_ss.add(when: 'CONFIG_VMW_PVRDMA', if_true: files( + 'rdma_rm.c', 'vmw/pvrdma_cmd.c', 'vmw/pvrdma_dev_ring.c', 'vmw/pvrdma_main.c', - 'vmw/pvrdma_qp_ops.c', )) -- 2.31.1
[PULL 10/13] lsi53c895a: disable reentrancy detection for script RAM
From: Alexander Bulekov As the code is designed to use the memory APIs to access the script ram, disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion. In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion. Reported-by: Fiona Ebner Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth Reviewed-by: Darren Kenny Message-Id: <20230427211013.2994127-6-alx...@bu.edu> Signed-off-by: Thomas Huth --- hw/scsi/lsi53c895a.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index af93557a9a..db27872963 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); +/* + * Since we use the address-space API to interact with ram_io, disable the + * re-entrancy guard. + */ +s->ram_io.disable_reentrancy_guard = true; + address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1); -- 2.31.1
Re: [PATCH 17/16] docs/devel/qapi-code-gen: Describe some doc markup pitfalls
On 28.04.23 12:34, Markus Armbruster wrote: Vladimir Sementsov-Ogievskiy writes: On 27.04.23 12:53, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 53 1 file changed, 53 insertions(+) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index d81aac7a19..14983b074c 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -1059,6 +1059,59 @@ For example:: 'returns': ['BlockStats'] } +Markup pitfalls +~~~ + +A blank line is required between list items and paragraphs. Without +it, the list may not be recognized, resulting in garbled output. Good +example:: + + # An event's state is modified if: + # + # - its name matches the @name pattern, and + # - if @vcpu is given, the event has the "vcpu" property. + +Without the blank line this would be a single paragraph. + +Indentation matters. Bad example:: + + # @none: None (no memory side cache in this proximity domain, + # or cache associativity unknown) + +The description is parsed as a definition list with term "None (no +memory side cache in this proximity domain," and definition "or cache +associativity unknown)". May be add good example of indentation as well Patches I'm about to post will fill up this pitfall. They change the text to: # @none: None (no memory side cache in this proximity domain, # or cache associativity unknown) # (since 5.0) The last line's de-indent is wrong. The second and subsequent lines So you want to drop "The description is parsed as a definition list ..." ? need to line up with each other, like this:: # @none: None (no memory side cache in this proximity domain, # or cache associativity unknown) # (since 5.0) Good enough? Example of good indent is good) -- Best regards, Vladimir
[PULL 12/13] raven: disable reentrancy detection for iomem
From: Alexander Bulekov As the code is designed for re-entrant calls from raven_io_ops to pci-conf, mark raven_io_ops as reentrancy-safe. Signed-off-by: Alexander Bulekov Message-Id: <20230427211013.2994127-8-alx...@bu.edu> Signed-off-by: Thomas Huth --- hw/pci-host/raven.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c index 072ffe3c5e..9a11ac4b2b 100644 --- a/hw/pci-host/raven.c +++ b/hw/pci-host/raven.c @@ -294,6 +294,13 @@ static void raven_pcihost_initfn(Object *obj) memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f00); address_space_init(&s->pci_io_as, &s->pci_io, "raven-io"); +/* + * Raven's raven_io_ops use the address-space API to access pci-conf-idx + * (which is also owned by the raven device). As such, mark the + * pci_io_non_contiguous as re-entrancy safe. + */ +s->pci_io_non_contiguous.disable_reentrancy_guard = true; + /* CPU address space */ memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR, &s->pci_io); -- 2.31.1
[PATCH v7 0/1] arm: enable MTE for QEMU + kvm
v7 takes a different approach to wiring up MTE, so I still include a cover letter where I can explain things better, even though it is now only a single patch :) Previous versions used a cpu property to control MTE enablement, while keeping the same semantics for the virt machine "mte" property as used with tcg. This version now uses the machine property for kvm as well; while it continues to control allocation of tag memory for tcg, it now also controls enablement of the kvm capability. Since the cpu prop is now gone, so is the testing via the arm cpu props test; I don't have a good idea for testing mte on kvm automatically, since it has a hard dependency on host support. (Should I tack some futher documentation somewhere? I'm not seeing an obvious place.) A kvm guest with mte enabled still cannot be migrated (we need some uffd interface enhancements before we can support postcopy), so it is still off per default. Another open problem is mte vs mte3: tcg emulates mte3, kvm gives the guest whatever the host supports. Without migration support, this is not too much of a problem yet, but for compatibility handling, we'll need a way to keep QEMU from handing out mte3 for guests that might be migrated to a mte3-less host. We could tack this unto the mte property (specifying the version or max supported), or we could handle this via cpu properties if we go with handling compatibility via cpu models (sorting this out for kvm is probably going to be interesting in general.) In any case, I think we'll need a way to inform kvm of it. Tested with kvm selftests on FVP (as I still don't have any hardware with MTE...) and some make check(-tcg) incantations. Hopefully, I haven't (re)- introduced too many issues. Cornelia Huck (1): arm/kvm: add support for MTE hw/arm/virt.c| 69 +--- target/arm/cpu.c | 9 +++--- target/arm/cpu.h | 4 +++ target/arm/kvm.c | 35 ++ target/arm/kvm64.c | 5 target/arm/kvm_arm.h | 19 6 files changed, 107 insertions(+), 34 deletions(-) -- 2.40.0
[PATCH v7 1/1] arm/kvm: add support for MTE
Extend the 'mte' property for the virt machine to cover KVM as
well. For KVM, we don't allocate tag memory, but instead enable the
capability.
If MTE has been enabled, we need to disable migration, as we do not
yet have a way to migrate the tags as well. Therefore, MTE will stay
off with KVM unless requested explicitly.
Signed-off-by: Cornelia Huck
---
hw/arm/virt.c| 69 +---
target/arm/cpu.c | 9 +++---
target/arm/cpu.h | 4 +++
target/arm/kvm.c | 35 ++
target/arm/kvm64.c | 5
target/arm/kvm_arm.h | 19
6 files changed, 107 insertions(+), 34 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a89d699f0b76..544a6c5bec8f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2146,7 +2146,7 @@ static void machvirt_init(MachineState *machine)
exit(1);
}
-if (vms->mte && (kvm_enabled() || hvf_enabled())) {
+if (vms->mte && hvf_enabled()) {
error_report("mach-virt: %s does not support providing "
"MTE to the guest CPU",
current_accel_name());
@@ -2216,39 +2216,48 @@ static void machvirt_init(MachineState *machine)
}
if (vms->mte) {
-/* Create the memory region only once, but link to all cpus. */
-if (!tag_sysmem) {
-/*
- * The property exists only if MemTag is supported.
- * If it is, we must allocate the ram to back that up.
- */
-if (!object_property_find(cpuobj, "tag-memory")) {
-error_report("MTE requested, but not supported "
- "by the guest CPU");
-exit(1);
+if (tcg_enabled()) {
+/* Create the memory region only once, but link to all cpus. */
+if (!tag_sysmem) {
+/*
+ * The property exists only if MemTag is supported.
+ * If it is, we must allocate the ram to back that up.
+ */
+if (!object_property_find(cpuobj, "tag-memory")) {
+error_report("MTE requested, but not supported "
+ "by the guest CPU");
+exit(1);
+}
+
+tag_sysmem = g_new(MemoryRegion, 1);
+memory_region_init(tag_sysmem, OBJECT(machine),
+ "tag-memory", UINT64_MAX / 32);
+
+if (vms->secure) {
+secure_tag_sysmem = g_new(MemoryRegion, 1);
+memory_region_init(secure_tag_sysmem, OBJECT(machine),
+ "secure-tag-memory",
+ UINT64_MAX / 32);
+
+/* As with ram, secure-tag takes precedence over tag.
*/
+memory_region_add_subregion_overlap(secure_tag_sysmem,
+0, tag_sysmem, -1);
+}
}
-tag_sysmem = g_new(MemoryRegion, 1);
-memory_region_init(tag_sysmem, OBJECT(machine),
- "tag-memory", UINT64_MAX / 32);
-
+object_property_set_link(cpuobj, "tag-memory",
+ OBJECT(tag_sysmem), &error_abort);
if (vms->secure) {
-secure_tag_sysmem = g_new(MemoryRegion, 1);
-memory_region_init(secure_tag_sysmem, OBJECT(machine),
- "secure-tag-memory", UINT64_MAX / 32);
-
-/* As with ram, secure-tag takes precedence over tag. */
-memory_region_add_subregion_overlap(secure_tag_sysmem, 0,
-tag_sysmem, -1);
+object_property_set_link(cpuobj, "secure-tag-memory",
+ OBJECT(secure_tag_sysmem),
+ &error_abort);
}
-}
-
-object_property_set_link(cpuobj, "tag-memory", OBJECT(tag_sysmem),
- &error_abort);
-if (vms->secure) {
-object_property_set_link(cpuobj, "secure-tag-memory",
- OBJECT(secure_tag_sysmem),
- &error_abort);
+} else if (kvm_enabled()) {
+if (!kvm_arm_mte_supported()) {
+error_report("MTE requested, but not supported by KVM");
+exit(1);
+}
+kvm_arm_enable_mte(cpuobj, &error_abort);
}
}
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5182ed0c9113..f6a88e52ac
Re: [PULL 00/18] Migration 20230427 patches
On 4/27/23 16:22, Juan Quintela wrote: The following changes since commit 1eb95e1baef852d0971a1dd62a3293cd68f1ec35: Merge tag 'migration-20230426-pull-request' ofhttps://gitlab.com/juan.quintela/qemu into staging (2023-04-27 10:47:14 +0100) are available in the Git repository at: https://gitlab.com/juan.quintela/qemu.git tags/migration-20230427-pull-request for you to fetch changes up to 73208a336e249bc8e3bdd76a78d0af7ecaee9178: migration: Make dirty_bytes_last_sync atomic (2023-04-27 16:39:54 +0200) Migration Pull request (20230427 edition) Hi Everything that has been reviewed: - stat64_set() by paolo - atomic_counters series fully reviewed (juan) - move capabilities to options.c fully reviewed (juan) - fix the channels_ready semaphore (juan) - multifd flush optimization reviewed (juan) Please, apply. Applied, thanks. Please update https://wiki.qemu.org/ChangeLog/8.1 as appropriate. r~
[PATCH] cpus-common: stop using mb_set/mb_read
Use a store-release at the end of the work item, and a load-acquire when
waiting for the item to be completed. This is the standard message passing
pattern and is both enough and clearer than mb_read/mb_set.
Signed-off-by: Paolo Bonzini
---
cpus-common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cpus-common.c b/cpus-common.c
index b0047e456f93..a53716deb437 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -157,7 +157,7 @@ void do_run_on_cpu(CPUState *cpu, run_on_cpu_func func,
run_on_cpu_data data,
wi.exclusive = false;
queue_work_on_cpu(cpu, &wi);
-while (!qatomic_mb_read(&wi.done)) {
+while (!qatomic_load_acquire(&wi.done)) {
CPUState *self_cpu = current_cpu;
qemu_cond_wait(&qemu_work_cond, mutex);
@@ -363,7 +363,7 @@ void process_queued_cpu_work(CPUState *cpu)
if (wi->free) {
g_free(wi);
} else {
-qatomic_mb_set(&wi->done, true);
+qatomic_store_release(&wi->done, true);
}
}
qemu_mutex_unlock(&cpu->work_mutex);
--
2.40.0
Re: [PATCH 17/16] docs/devel/qapi-code-gen: Describe some doc markup pitfalls
Vladimir Sementsov-Ogievskiy writes: > On 28.04.23 12:34, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy writes: >> >>> On 27.04.23 12:53, Markus Armbruster wrote: Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 53 1 file changed, 53 insertions(+) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index d81aac7a19..14983b074c 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -1059,6 +1059,59 @@ For example:: 'returns': ['BlockStats'] } +Markup pitfalls +~~~ + +A blank line is required between list items and paragraphs. Without +it, the list may not be recognized, resulting in garbled output. Good +example:: + + # An event's state is modified if: + # + # - its name matches the @name pattern, and + # - if @vcpu is given, the event has the "vcpu" property. + +Without the blank line this would be a single paragraph. + +Indentation matters. Bad example:: + + # @none: None (no memory side cache in this proximity domain, + # or cache associativity unknown) + +The description is parsed as a definition list with term "None (no +memory side cache in this proximity domain," and definition "or cache +associativity unknown)". >>> >>> May be add good example of indentation as well >> >> Patches I'm about to post will fill up this pitfall. They change the >> text to: >> >> # @none: None (no memory side cache in this proximity domain, >> # or cache associativity unknown) >> # (since 5.0) >> The last line's de-indent is wrong. The second and subsequent lines > > So you want to drop "The description is parsed as a definition list ..." ? Yes, because that'll be factually wrong :) Happy to explain further once the patches are on the list. >> need to line up with each other, like this:: >> >> # @none: None (no memory side cache in this proximity domain, >> # or cache associativity unknown) >> # (since 5.0) >> >> Good enough? > > Example of good indent is good) Thanks!
[PULL 00/17] QAPI patches patches for 2023-04-28
The following changes since commit cc5ee50fff9dbac0aac32cd892a7163c7babcca1: Merge tag 'pull-testing-docs-270423-1' of https://gitlab.com/stsquad/qemu into staging (2023-04-27 16:46:17 +0100) are available in the Git repository at: https://repo.or.cz/qemu/armbru.git tags/pull-qapi-2023-04-28 for you to fetch changes up to e2e9e567f0e23971cac35ba1dee7edb51085b5f7: docs/devel/qapi-code-gen: Describe some doc markup pitfalls (2023-04-28 11:48:34 +0200) QAPI patches patches for 2023-04-28 Markus Armbruster (17): qga/qapi-schema: Tidy up documentation of guest-fsfreeze-status qga/qapi-schema: Fix a misspelled reference qapi: Fix misspelled references qapi: Fix up references to long gone error classes qapi/block-core: Clean up after removal of dirty bitmap @status qapi: @foo should be used to reference, not ``foo`` qapi: Tidy up examples qapi: Delete largely misleading "Stability Considerations" qapi: Fix bullet list markup in documentation qapi: Fix unintended definition lists in documentation qga/qapi-schema: Fix member documentation markup qapi: Fix argument documentation markup qapi: Replace ad hoc "since" documentation by member documentation qapi: Fix misspelled section tags in doc comments qapi: Format since information the conventional way: (since X.Y) qapi storage-daemon/qapi: Fix documentation section structure docs/devel/qapi-code-gen: Describe some doc markup pitfalls docs/devel/qapi-code-gen.rst | 62 +-- docs/interop/firmware.json | 6 +-- qapi/block-core.json | 82 ++-- qapi/block-export.json | 7 +-- qapi/block.json | 2 +- qapi/char.json | 4 +- qapi/control.json| 2 +- qapi/cryptodev.json | 4 ++ qapi/job.json| 4 +- qapi/machine-target.json | 2 +- qapi/machine.json| 30 +++-- qapi/migration.json | 37 ++-- qapi/misc.json | 13 +++--- qapi/net.json| 27 +--- qapi/qapi-schema.json| 24 +-- qapi/qdev.json | 2 +- qapi/qom.json| 4 +- qapi/rdma.json | 2 +- qapi/replay.json | 3 ++ qapi/run-state.json | 11 +++-- qapi/stats.json | 3 +- qapi/tpm.json| 3 +- qapi/trace.json | 1 + qapi/ui.json | 12 +++--- qapi/yank.json | 21 - qga/qapi-schema.json | 10 ++--- storage-daemon/qapi/qapi-schema.json | 22 +++--- 27 files changed, 231 insertions(+), 169 deletions(-) -- 2.39.2
[PULL 10/17] qapi: Fix unintended definition lists in documentation
rST parses something like first line second line as a definition list item, where "first line" is the term being defined by "second line". This bites us in a couple of places. Here's one: # @bps_max: total throughput limit during bursts, # in bytes (Since 1.7) scripts/qapi/parser.py parses this into an "argument section" with name "bps_max" and text total throughput limit during bursts, in bytes (Since 1.7) docs/sphinx/qapidoc.py duly passes the text to the rST parser, which parses it as another definition list. Comes out as nested definitions: term "bps_max: int (optional)" defined as term "total throughput limit during bursts," defined as "in bytes (Since 1.7)". rST truly is the Perl of ASCII-based markups. Fix by deleting the extra indentation. Fixes: 26ec4e53f2bf (qapi: Fix indent level on doc comments in json files) Fixes: c0ac533b6f97 (qapi: Stop using whitespace for alignment in comments) Fixes: 81ad2964e938 (net/vmnet: add vmnet backends to qapi/net) Reported-by: Peter Maydell Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Reviewed-by: Peter Maydell Message-Id: <20230425064223.820979-11-arm...@redhat.com> --- qapi/block-core.json | 52 ++-- qapi/control.json| 2 +- qapi/machine.json| 4 ++-- qapi/net.json| 2 +- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index a5a5007b28..2382772e17 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -488,41 +488,41 @@ # # @image: the info of image used (since: 1.6) # -# @bps_max: total throughput limit during bursts, -# in bytes (Since 1.7) +# @bps_max: total throughput limit during bursts, in bytes +# (Since 1.7) # -# @bps_rd_max: read throughput limit during bursts, -#in bytes (Since 1.7) +# @bps_rd_max: read throughput limit during bursts, in bytes +# (Since 1.7) # -# @bps_wr_max: write throughput limit during bursts, -#in bytes (Since 1.7) +# @bps_wr_max: write throughput limit during bursts, in bytes +# (Since 1.7) # -# @iops_max: total I/O operations per second during bursts, -# in bytes (Since 1.7) +# @iops_max: total I/O operations per second during bursts, in bytes +#(Since 1.7) # -# @iops_rd_max: read I/O operations per second during bursts, -# in bytes (Since 1.7) +# @iops_rd_max: read I/O operations per second during bursts, in bytes +# (Since 1.7) # -# @iops_wr_max: write I/O operations per second during bursts, -# in bytes (Since 1.7) +# @iops_wr_max: write I/O operations per second during bursts, in +# bytes (Since 1.7) # -# @bps_max_length: maximum length of the @bps_max burst -#period, in seconds. (Since 2.6) +# @bps_max_length: maximum length of the @bps_max burst period, in +# seconds. (Since 2.6) # -# @bps_rd_max_length: maximum length of the @bps_rd_max -# burst period, in seconds. (Since 2.6) +# @bps_rd_max_length: maximum length of the @bps_rd_max burst period, +# in seconds. (Since 2.6) # -# @bps_wr_max_length: maximum length of the @bps_wr_max -# burst period, in seconds. (Since 2.6) +# @bps_wr_max_length: maximum length of the @bps_wr_max burst period, +# in seconds. (Since 2.6) # -# @iops_max_length: maximum length of the @iops burst -# period, in seconds. (Since 2.6) +# @iops_max_length: maximum length of the @iops burst period, in +# seconds. (Since 2.6) # -# @iops_rd_max_length: maximum length of the @iops_rd_max -#burst period, in seconds. (Since 2.6) +# @iops_rd_max_length: maximum length of the @iops_rd_max burst +# period, in seconds. (Since 2.6) # -# @iops_wr_max_length: maximum length of the @iops_wr_max -#burst period, in seconds. (Since 2.6) +# @iops_wr_max_length: maximum length of the @iops_wr_max burst +# period, in seconds. (Since 2.6) # # @iops_size: an I/O size in bytes (Since 1.7) # @@ -948,7 +948,7 @@ # by the device (Since 4.2) # # @invalid_rd_operations: The number of invalid read operations -# performed by the device (Since 2.5) +# performed by the device (Since 2.5) # # @invalid_wr_operations: The number of invalid write operations # performed by the device (Since 2.5) @@ -3735,7 +3735,7 @@ # Driver specific block device options for Quorum # # @blkverify: true if the driver must print content mismatch -# set
[PULL 17/17] docs/devel/qapi-code-gen: Describe some doc markup pitfalls
Signed-off-by: Markus Armbruster Message-Id: <20230427095346.1238913-1-arm...@redhat.com> Reviewed-by: Juan Quintela Reviewed-by: Vladimir Sementsov-Ogievskiy --- docs/devel/qapi-code-gen.rst | 53 1 file changed, 53 insertions(+) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index ea0592034a..af1986f33e 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -1060,6 +1060,59 @@ For example:: 'returns': ['BlockStats'] } +Markup pitfalls +~~~ + +A blank line is required between list items and paragraphs. Without +it, the list may not be recognized, resulting in garbled output. Good +example:: + + # An event's state is modified if: + # + # - its name matches the @name pattern, and + # - if @vcpu is given, the event has the "vcpu" property. + +Without the blank line this would be a single paragraph. + +Indentation matters. Bad example:: + + # @none: None (no memory side cache in this proximity domain, + # or cache associativity unknown) + +The description is parsed as a definition list with term "None (no +memory side cache in this proximity domain," and definition "or cache +associativity unknown)". + +Section tags are case-sensitive and end with a colon. Good example:: + + # Since: 7.1 + +Bad examples (all ordinary paragraphs):: + + # since: 7.1 + + # Since 7.1 + + # Since : 7.1 + +Likewise, member descriptions require a colon. Good example:: + + # @interface-id: Interface ID + +Bad examples (all ordinary paragraphs):: + + # @interface-id Interface ID + + # @interface-id : Interface ID + +Undocumented members are not flagged, yet. Instead, the generated +documentation describes them as "Not documented". Think twice before +adding more undocumented members. + +When you change documentation comments, please check the generated +documentation comes out as intended! + + Client JSON Protocol introspection == -- 2.39.2
[PULL 15/17] qapi: Format since information the conventional way: (since X.Y)
Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-16-arm...@redhat.com> --- qapi/block-core.json | 6 +++--- qapi/stats.json | 2 +- qapi/tpm.json| 3 +-- qapi/ui.json | 6 +++--- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 9dd5ed9a47..b57978957f 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1318,10 +1318,10 @@ # value is monotonically increasing. # # @busy: false if the job is known to be in a quiescent state, with -#no pending I/O. Since 1.3. +#no pending I/O. (Since 1.3) # # @paused: whether the job is paused or, if @busy is true, will -# pause itself as soon as possible. Since 1.3. +# pause itself as soon as possible. (Since 1.3) # # @speed: the rate limit, bytes per second # @@ -2741,7 +2741,7 @@ # # @on-error: the action to take on an error (default report). #'stop' and 'enospc' can only be used if the block device -#supports io-status (see BlockInfo). Since 1.3. +#supports io-status (see BlockInfo). (Since 1.3) # # @filter-node-name: the node name that should be assigned to the #filter driver that the stream job inserts into the graph diff --git a/qapi/stats.json b/qapi/stats.json index f17495ee65..36d5f4dc94 100644 --- a/qapi/stats.json +++ b/qapi/stats.json @@ -69,7 +69,7 @@ # # @vcpu: statistics that apply to a single virtual CPU. # -# @cryptodev: statistics that apply to a crypto device. since 8.0 +# @cryptodev: statistics that apply to a crypto device (since 8.0) # # Since: 7.1 ## diff --git a/qapi/tpm.json b/qapi/tpm.json index 4e2ea9756a..eac87d30b2 100644 --- a/qapi/tpm.json +++ b/qapi/tpm.json @@ -44,8 +44,7 @@ # An enumeration of TPM types # # @passthrough: TPM passthrough type -# @emulator: Software Emulator TPM type -#Since: 2.11 +# @emulator: Software Emulator TPM type (since 2.11) # # Since: 1.5 ## diff --git a/qapi/ui.json b/qapi/ui.json index fa05bc1365..b5650974fc 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -1207,13 +1207,13 @@ # window resizes (virtio-gpu) this will default to "on", # assuming the guest will resize the display to match # the window size then. Otherwise it defaults to "off". -# Since 3.1 +# (Since 3.1) # @show-tabs: Display the tab bar for switching between the various graphical # interfaces (e.g. VGA and virtual console character devices) # by default. -# Since 7.1 +# (Since 7.1) # @show-menubar: Display the main window menubar. Defaults to "on". -#Since 8.0 +#(Since 8.0) # # Since: 2.12 ## -- 2.39.2
[PULL 03/17] qapi: Fix misspelled references
query-cpu-definitions returns a list of CpuDefinitionInfo, but documentation claims CpuDefInfo, which doesn't exist. query-migrate-capabilities returns a list of MigrationCapabilityStatus, but documentation claims MigrationCapabilitiesStatus, which doesn't exist. balloon and query-balloon can fail with KVMMissingCap, but documentation claims KvmMissingCap, which doesn't exist. Fix the documentation. Fixes: e4e31c6324af (qapi: add query-cpu-definitions command (v2)) Fixes: bbf6da32b5bd (Add migration capabilities) Fixes: d72f326431e2 (qapi: Convert balloon) Fixes: 96637bcdf9e0 (qapi: Convert query-balloon) Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Reviewed-by: David Hildenbrand Message-Id: <20230425064223.820979-4-arm...@redhat.com> Reviewed-by: Juan Quintela --- qapi/machine-target.json | 2 +- qapi/machine.json| 4 ++-- qapi/migration.json | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/qapi/machine-target.json b/qapi/machine-target.json index 2e267fa458..b94fbdb65e 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -331,7 +331,7 @@ # # Return a list of supported virtual CPU definitions # -# Returns: a list of CpuDefInfo +# Returns: a list of CpuDefinitionInfo # # Since: 1.2 ## diff --git a/qapi/machine.json b/qapi/machine.json index 604b686e59..8c3c58c763 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1029,7 +1029,7 @@ # # Returns: - Nothing on success # - If the balloon driver is enabled but not functional because the KVM -#kernel module cannot support it, KvmMissingCap +#kernel module cannot support it, KVMMissingCap # - If no balloon device is present, DeviceNotActive # # Notes: This command just issues a request to the guest. When it returns, @@ -1067,7 +1067,7 @@ # # Returns: - @BalloonInfo on success # - If the balloon driver is enabled but not functional because the KVM -#kernel module cannot support it, KvmMissingCap +#kernel module cannot support it, KVMMissingCap # - If no balloon device is present, DeviceNotActive # # Since: 0.14 diff --git a/qapi/migration.json b/qapi/migration.json index 2c35b7b9cf..91c1773739 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -531,7 +531,7 @@ # # Returns information about the current migration capabilities status # -# Returns: @MigrationCapabilitiesStatus +# Returns: @MigrationCapabilityStatus # # Since: 1.2 # -- 2.39.2
[PULL 08/17] qapi: Delete largely misleading "Stability Considerations"
Documentation section "Stability Considerations" dates back to the
early days of QMP (commit 82a56f0d83d (Monitor: Introduce the
qmp-commands.hx file)). It became largely misleading years ago.
Delete it.
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-9-arm...@redhat.com>
---
qapi/qapi-schema.json | 22 --
1 file changed, 22 deletions(-)
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index 7c09af5cc8..e57d8ff801 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -28,28 +28,6 @@
#
# Please, refer to the QMP specification (docs/interop/qmp-spec.txt) for
# detailed information on the Server command and response formats.
-#
-# = Stability Considerations
-#
-# The current QMP command set (described in this file) may be useful for a
-# number of use cases, however it's limited and several commands have bad
-# defined semantics, specially with regard to command completion.
-#
-# These problems are going to be solved incrementally in the next QEMU releases
-# and we're going to establish a deprecation policy for badly defined commands.
-#
-# If you're planning to adopt QMP, please observe the following:
-#
-# 1. The deprecation policy will take effect and be documented soon, please
-#check the documentation of each used command as soon as a new release
of
-#QEMU is available
-#
-# 2. DO NOT rely on anything which is not explicit documented
-#
-# 3. Errors, in special, are not documented. Applications should NOT check
-#for specific errors classes or data (it's strongly recommended to only
-#check for the "error" key)
-#
##
{ 'include': 'pragma.json' }
--
2.39.2
[PULL 05/17] qapi/block-core: Clean up after removal of dirty bitmap @status
Commit 81cbfd50886 (block: remove dirty bitmaps 'status' field) removed deprecated BlockDirtyInfo member @status. It neglected to remove references to its enumeration values from the documentation of its replacements. Do that now. Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Reviewed-by: John Snow Message-Id: <20230425064223.820979-6-arm...@redhat.com> --- qapi/block-core.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 75f7c62405..eeb2ed3f16 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -582,11 +582,11 @@ # @granularity: granularity of the dirty bitmap in bytes (since 1.4) # # @recording: true if the bitmap is recording new writes from the guest. -# Replaces ``active`` and ``disabled`` statuses. (since 4.0) +# (since 4.0) # # @busy: true if the bitmap is in-use by some operation (NBD or jobs) #and cannot be modified via QMP or used by another operation. -#Replaces ``locked`` and ``frozen`` statuses. (since 4.0) +#(since 4.0) # # @persistent: true if the bitmap was stored on disk, is scheduled to be stored # on disk, or both. (since 4.0) -- 2.39.2
[PULL 11/17] qga/qapi-schema: Fix member documentation markup
GuestDiskStatsInfo's member documentation is parsed as ordinary text
due to missing colons. The generated documentation shows these
members as "Not documented".
The fix is obvious: add the missing colons.
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-12-arm...@redhat.com>
---
qga/qapi-schema.json | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index bb9bac0655..6a20eeb297 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1553,11 +1553,11 @@
##
# @GuestDiskStatsInfo:
#
-# @name disk name
+# @name: disk name
#
-# @major major device number of disk
+# @major: major device number of disk
#
-# @minor minor device number of disk
+# @minor: minor device number of disk
##
{ 'struct': 'GuestDiskStatsInfo',
'data': {'name': 'str',
--
2.39.2
[PULL 12/17] qapi: Fix argument documentation markup
Member / argument documentation of BlockdevAmendOptionsQcow2, job-resume, and RDMA_GID_STATUS_CHANGED is parsed as ordinary text due to missing colon or space before the colon. The generated documentation shows these members / arguments as "Not documented". The fix is obvious: add missing colons, delete extra spaces. Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-13-arm...@redhat.com> --- qapi/block-core.json | 2 +- qapi/job.json| 2 +- qapi/rdma.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 2382772e17..9dd5ed9a47 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -5155,7 +5155,7 @@ # Driver specific image amend options for qcow2. # For now, only encryption options can be amended # -# @encrypt Encryption options to be amended +# @encrypt: Encryption options to be amended # # Since: 5.1 ## diff --git a/qapi/job.json b/qapi/job.json index d5f84e9615..bc4104757a 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -148,7 +148,7 @@ # This command returns immediately after resuming a paused job. Resuming an # already running job is an error. # -# @id : The job identifier. +# @id: The job identifier. # # Since: 3.0 ## diff --git a/qapi/rdma.json b/qapi/rdma.json index a1d2175a8b..5b6b66afa4 100644 --- a/qapi/rdma.json +++ b/qapi/rdma.json @@ -17,7 +17,7 @@ # # @subnet-prefix: Subnet Prefix # -# @interface-id : Interface ID +# @interface-id: Interface ID # # Since: 4.0 # -- 2.39.2
[PULL 04/17] qapi: Fix up references to long gone error classes
Commit de253f14912e88f4 (qmp: switch to the new error format on the wire) removed most error classes. Several later commits mistakenly mentioned them in documentation. Replace them by the actual error class there. Fixes: 44e3e053af56 (qmp: add interface blockdev-snapshot-delete-internal-sync) Fixes: f323bc9e8b3b (qmp: add interface blockdev-snapshot-internal-sync) Fixes: ba1c048a8f9c (qapi: Introduce add-fd, remove-fd, query-fdsets) Fixes: ed61fc10e8c8 (QAPI: add command for live block commit, 'block-commit') Fixes: e4c8f004c55d (qapi: convert sendkey) Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-5-arm...@redhat.com> --- qapi/block-core.json | 4 ++-- qapi/misc.json | 6 +++--- qapi/ui.json | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index c05ad0c07e..75f7c62405 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -5784,7 +5784,7 @@ # - If any snapshot matching @name exists, or @name is empty, #GenericError # - If the format of the image used does not support it, -#BlockFormatFeatureNotSupported +#GenericError # # Since: 1.7 # @@ -5820,7 +5820,7 @@ # - If @device is not a valid block device, GenericError # - If snapshot not found, GenericError # - If the format of the image used does not support it, -#BlockFormatFeatureNotSupported +#GenericError # - If @id and @name are both not specified, GenericError # # Since: 1.7 diff --git a/qapi/misc.json b/qapi/misc.json index 6ddd16ea28..7e278ca1eb 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -349,8 +349,8 @@ # @opaque: A free-form string that can be used to describe the fd. # # Returns: - @AddfdInfo on success -# - If file descriptor was not received, FdNotSupplied -# - If @fdset-id is a negative value, InvalidParameterValue +# - If file descriptor was not received, GenericError +# - If @fdset-id is a negative value, GenericError # # Notes: The list of fd sets is shared by all monitor connections. # @@ -379,7 +379,7 @@ # @fd: The file descriptor that is to be removed. # # Returns: - Nothing on success -# - If @fdset-id or @fd is not found, FdNotFound +# - If @fdset-id or @fd is not found, GenericError # # Since: 1.2 # diff --git a/qapi/ui.json b/qapi/ui.json index 7ddd27a932..2d9b34b105 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -985,7 +985,7 @@ # to 100 # # Returns: - Nothing on success -# - If key is unknown or redundant, InvalidParameter +# - If key is unknown or redundant, GenericError # # Since: 1.3 # -- 2.39.2
[PULL 14/17] qapi: Fix misspelled section tags in doc comments
Section tags are case sensitive and end with a colon. Screwing up
either gets them interpreted as ordinary paragraph. Fix a few.
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-15-arm...@redhat.com>
---
qapi/machine.json | 4 ++--
qapi/migration.json | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/qapi/machine.json b/qapi/machine.json
index 5a18913767..fcd69965e5 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -517,7 +517,7 @@
# @targets: Target root bridge IDs from -device ...,id= for each root
# bridge.
#
-# Since 7.1
+# Since: 7.1
##
{ 'struct': 'CXLFixedMemoryWindowOptions',
'data': {
@@ -532,7 +532,7 @@
#
# @cxl-fmw: List of CXLFixedMemoryWindowOptions
#
-# Since 7.1
+# Since: 7.1
##
{ 'struct' : 'CXLFMWProperties',
'data': { 'cxl-fmw': ['CXLFixedMemoryWindowOptions'] }
diff --git a/qapi/migration.json b/qapi/migration.json
index 015b22c970..82000adce4 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1963,7 +1963,7 @@
#
# data: migration thread name
#
-# returns: information about migration threads
+# Returns: information about migration threads
#
# Since: 7.2
##
--
2.39.2
[PULL 01/17] qga/qapi-schema: Tidy up documentation of guest-fsfreeze-status
Delete "error state indicates", because it doesn't make sense.
I suspect it was an accident.
Signed-off-by: Markus Armbruster
Reviewed-by: Konstantin Kostiuk
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-2-arm...@redhat.com>
---
qga/qapi-schema.json | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 796434ed34..f349345116 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -420,7 +420,7 @@
##
# @guest-fsfreeze-status:
#
-# Get guest fsfreeze state. error state indicates
+# Get guest fsfreeze state.
#
# Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined below)
#
--
2.39.2
[PULL 06/17] qapi: @foo should be used to reference, not ``foo``
Documentation suggests @foo is merely shorthand for ``foo``. It's not, it carries additional meaning: it's a reference to a QAPI schema name. Reword the documentation to spell that out. Fix up the few ``foo`` that should be @foo. Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-7-arm...@redhat.com> --- docs/devel/qapi-code-gen.rst | 8 +--- docs/interop/firmware.json | 6 +++--- qapi/qom.json| 2 +- qapi/ui.json | 2 +- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index 879a649e8c..d81aac7a19 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -924,9 +924,11 @@ first character of the first line. The usual strong, *\*emphasized\** and literal markup should be used. If you need a single literal ``*``, you will need to -backslash-escape it. As an extension beyond the usual rST syntax, you -can also use ``@foo`` to reference a name in the schema; this is rendered -the same way as foo. +backslash-escape it. + +Use ``@foo`` to reference a name in the schema. This is an rST +extension. It is rendered the same way as foo, but carries +additional meaning. Example:: diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json index 56814f02b3..cc8f869186 100644 --- a/docs/interop/firmware.json +++ b/docs/interop/firmware.json @@ -258,7 +258,7 @@ # # @mode: Describes how the firmware build handles code versus variable #storage. If not present, it must be treated as if it was -#configured with value ``split``. Since: 7.0.0 +#configured with value @split. Since: 7.0.0 # # @executable: Identifies the firmware executable. The @mode # indicates whether there will be an associated @@ -267,13 +267,13 @@ # -drive if=none,id=pflash0,readonly=on,file=@executable.@filename,format=@executable.@format # -machine pflash0=pflash0 # or equivalent -blockdev instead of -drive. When -# @mode is ``combined`` the executable must be +# @mode is @combined the executable must be # cloned before use and configured with readonly=off. # With QEMU versions older than 4.0, you have to use # -drive if=pflash,unit=0,readonly=on,file=@executable.@filename,format=@executable.@format # # @nvram-template: Identifies the NVRAM template compatible with -# @executable, when @mode is set to ``split``, +# @executable, when @mode is set to @split, # otherwise it should not be present. # Management software instantiates an # individual copy -- a specific NVRAM file -- from diff --git a/qapi/qom.json b/qapi/qom.json index a877b879b9..4fe7a93a75 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -637,7 +637,7 @@ # # @discard-data: if true, the file contents can be destroyed when QEMU exits, #to avoid unnecessarily flushing data to the backing file. Note -#that ``discard-data`` is only an optimization, and QEMU might +#that @discard-data is only an optimization, and QEMU might #not discard file contents if it aborts unexpectedly or is #terminated using SIGKILL. (default: false) # diff --git a/qapi/ui.json b/qapi/ui.json index 2d9b34b105..2fa41c8ab0 100644 --- a/qapi/ui.json +++ b/qapi/ui.json @@ -1247,7 +1247,7 @@ # available node on the host. # # @p2p: Whether to use peer-to-peer connections (accepted through -# ``add_client``). +# @add_client). # # @audiodev: Use the specified DBus audiodev to export audio. # -- 2.39.2
[PULL 07/17] qapi: Tidy up examples
A few examples neglect to prefix QMP input with '->'. Fix that.
Two examples have extra space after '<-'. Delete it.
A few examples neglect to show output. Provide some. The example
output for query-vcpu-dirty-limit could use further improvement. Add
a TODO comment.
Use "Examples:" instead of "Example:" where multiple examples are
given.
One example section numbers its two examples. Not done elsewhere;
drop.
Another example section separates them with "or". Likewise.
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-8-arm...@redhat.com>
Reviewed-by: Juan Quintela
---
qapi/block-core.json | 14 ++
qapi/block.json | 2 +-
qapi/char.json | 4 ++--
qapi/machine.json| 7 ---
qapi/migration.json | 33 ++---
qapi/misc.json | 7 +++
qapi/net.json| 4 +---
qapi/qdev.json | 2 +-
qapi/qom.json| 2 +-
qapi/replay.json | 3 +++
qapi/run-state.json | 5 ++---
qapi/ui.json | 2 +-
12 files changed, 47 insertions(+), 38 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index eeb2ed3f16..a5a5007b28 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4574,9 +4574,8 @@
#
# Since: 2.9
#
-# Example:
+# Examples:
#
-# 1.
# -> { "execute": "blockdev-add",
# "arguments": {
# "driver": "qcow2",
@@ -4589,7 +4588,6 @@
# }
# <- { "return": {} }
#
-# 2.
# -> { "execute": "blockdev-add",
# "arguments": {
# "driver": "qcow2",
@@ -5596,7 +5594,7 @@
#
# Since: 2.7
#
-# Example:
+# Examples:
#
# 1. Add a new node to a quorum
# -> { "execute": "blockdev-add",
@@ -5646,7 +5644,7 @@
#
# Since: 2.12
#
-# Example:
+# Examples:
#
# 1. Move a node into an IOThread
# -> { "execute": "x-blockdev-set-iothread",
@@ -5731,18 +5729,18 @@
#
# Since: 2.0
#
-# Example:
+# Examples:
#
# 1. Read operation
#
-# { "event": "QUORUM_REPORT_BAD",
+# <- { "event": "QUORUM_REPORT_BAD",
# "data": { "node-name": "node0", "sector-num": 345435, "sectors-count":
5,
#"type": "read" },
# "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
#
# 2. Flush operation
#
-# { "event": "QUORUM_REPORT_BAD",
+# <- { "event": "QUORUM_REPORT_BAD",
# "data": { "node-name": "node0", "sector-num": 0, "sectors-count":
2097120,
#"type": "flush", "error": "Broken pipe" },
# "timestamp": { "seconds": 1456406829, "microseconds": 291763 } }
diff --git a/qapi/block.json b/qapi/block.json
index 5fe068f903..94339a1761 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -457,7 +457,7 @@
#
# Since: 1.1
#
-# Example:
+# Examples:
#
# -> { "execute": "block_set_io_throttle",
# "arguments": { "id": "virtio-blk-pci0/virtio-backend",
diff --git a/qapi/char.json b/qapi/char.json
index 923dc5056d..c9431dd0a7 100644
--- a/qapi/char.json
+++ b/qapi/char.json
@@ -637,7 +637,7 @@
#
# Since: 1.4
#
-# Example:
+# Examples:
#
# -> { "execute" : "chardev-add",
# "arguments" : { "id" : "foo",
@@ -673,7 +673,7 @@
#
# Since: 2.10
#
-# Example:
+# Examples:
#
# -> { "execute" : "chardev-change",
# "arguments" : { "id" : "baz",
diff --git a/qapi/machine.json b/qapi/machine.json
index 8c3c58c763..20541cb319 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -954,7 +954,7 @@
#
# Since: 2.7
#
-# Example:
+# Examples:
#
# For pseries machine type started with -smp 2,cores=2,maxcpus=4 -cpu POWER8:
#
@@ -1677,8 +1677,9 @@
# Since: 7.2
#
# Example:
-# {"execute": "dumpdtb"}
-#"arguments": { "filename": "fdt.dtb" } }
+# -> { "execute": "dumpdtb" }
+# "arguments": { "filename": "fdt.dtb" } }
+# <- { "return": {} }
#
##
{ 'command': 'dumpdtb',
diff --git a/qapi/migration.json b/qapi/migration.json
index 91c1773739..015b22c970 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -273,7 +273,7 @@
#
# Since: 0.14
#
-# Example:
+# Examples:
#
# 1. Before the first migration
#
@@ -521,6 +521,7 @@
#
# -> { "execute": "migrate-set-capabilities" , "arguments":
# { "capabilities": [ { "capability": "xbzrle", "state": true } ] } }
+# <- { "return": {} }
#
##
{ 'command': 'migrate-set-capabilities',
@@ -989,6 +990,7 @@
#
# -> { "execute": "migrate-set-parameters" ,
# "arguments": { "compress-level": 1 } }
+# <- { "return": {} }
#
##
{ 'command': 'migrate-set-parameters', 'boxed': true,
@@ -1251,8 +1253,8 @@
#
# Example:
#
-# { "timestamp": {"seconds": 1449669631, "microseconds": 239225},
-# "event": "MIGRATION_PASS", "data": {"pass": 2} }
+# <- { "timestamp": {"seconds": 1449669631, "microseconds": 239225},
+# "event": "MIGRATION_PASS", "data": {"pass": 2} }
#
##
{ 'event': 'MIGRATION_PASS',
@@ -1833,8 +1835,9 @@
#
# Example:
#
-# {"execute": "calc-dirty-rate", "arguments": {"calc-time": 1,
-#
[PULL 16/17] qapi storage-daemon/qapi: Fix documentation section structure
In the QEMU QMP Reference Manual, subsection "Block core (VM
unrelated)" is empty. Its contents is at the end of subsection
"Background jobs" instead. That's because qapi/job.json is included
first from qapi/block-core.json, which makes qapi/job.json's
documentation go between qapi/block-core.json's subsection heading and
contents.
In the QEMU Storage Daemon QMP Reference Manual, section "Block
Devices" contains nothing but an empty subsection "Block core (VM
unrelated)". The latter's contents is at the end section "Socket data
types", along with subsection "Block device exports". Subsection
"Background jobs" is at the end of section "Cryptography". All this
is because storage-daemon/qapi/qapi-schema.json includes modules in a
confused order.
Fix both as follows.
Turn subsection "Background jobs" into a section.
Move it before section "Block devices" in the QEMU QMP Reference
Manual, by including qapi/jobs.json right before qapi/block.json.
Reorder include directives in storage-daemon/qapi/qapi-schema.json to
match the order in qapi/qapi-schema.json, so that the QEMU Storage
Daemon QMP Reference Manual's section structure the QEMU QMP Reference
Manual's.
In the QEMU QMP Reference Manual, qapi/cryptodev.json's documentation
is at the end of section "Virtio devices". That's because it lacks a
section heading, and therefore gets squashed into whatever section
happens to precede it.
Add section heading so it's in section "Cryptography devices".
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Reviewed-by: Eric Blake
Acked-by: zhenwei pi
Message-Id: <20230425064223.820979-17-arm...@redhat.com>
---
qapi/cryptodev.json | 4
qapi/job.json| 2 +-
qapi/qapi-schema.json| 2 +-
storage-daemon/qapi/qapi-schema.json | 22 +++---
4 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/qapi/cryptodev.json b/qapi/cryptodev.json
index f33f96a692..cf960ea81f 100644
--- a/qapi/cryptodev.json
+++ b/qapi/cryptodev.json
@@ -4,6 +4,10 @@
# This work is licensed under the terms of the GNU GPL, version 2 or later.
# See the COPYING file in the top-level directory.
+##
+# = Cryptography devices
+##
+
##
# @QCryptodevBackendAlgType:
#
diff --git a/qapi/job.json b/qapi/job.json
index bc4104757a..9e29a796c5 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -2,7 +2,7 @@
# vim: filetype=python
##
-# == Background jobs
+# = Background jobs
##
##
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index e57d8ff801..bb7217da26 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -43,11 +43,11 @@
{ 'include': 'sockets.json' }
{ 'include': 'run-state.json' }
{ 'include': 'crypto.json' }
+{ 'include': 'job.json' }
{ 'include': 'block.json' }
{ 'include': 'block-export.json' }
{ 'include': 'char.json' }
{ 'include': 'dump.json' }
-{ 'include': 'job.json' }
{ 'include': 'net.json' }
{ 'include': 'rdma.json' }
{ 'include': 'rocker.json' }
diff --git a/storage-daemon/qapi/qapi-schema.json
b/storage-daemon/qapi/qapi-schema.json
index 67749d1101..f10c949490 100644
--- a/storage-daemon/qapi/qapi-schema.json
+++ b/storage-daemon/qapi/qapi-schema.json
@@ -15,18 +15,26 @@
{ 'include': '../../qapi/pragma.json' }
+# Documentation generated with qapi-gen.py is in source order, with
+# included sub-schemas inserted at the first include directive
+# (subsequent include directives have no effect). To get a sane and
+# stable order, it's best to include each sub-schema just once, or
+# include it first right here.
+
+{ 'include': '../../qapi/common.json' }
+{ 'include': '../../qapi/sockets.json' }
+{ 'include': '../../qapi/crypto.json' }
+{ 'include': '../../qapi/job.json' }
+
##
# = Block devices
##
{ 'include': '../../qapi/block-core.json' }
{ 'include': '../../qapi/block-export.json' }
+
{ 'include': '../../qapi/char.json' }
-{ 'include': '../../qapi/common.json' }
-{ 'include': '../../qapi/control.json' }
-{ 'include': '../../qapi/crypto.json' }
-{ 'include': '../../qapi/introspect.json' }
-{ 'include': '../../qapi/job.json' }
{ 'include': '../../qapi/authz.json' }
-{ 'include': '../../qapi/qom.json' }
-{ 'include': '../../qapi/sockets.json' }
{ 'include': '../../qapi/transaction.json' }
+{ 'include': '../../qapi/control.json' }
+{ 'include': '../../qapi/introspect.json' }
+{ 'include': '../../qapi/qom.json' }
--
2.39.2
Re: [PATCH v3 1/2] igb: RX descriptors handling cleanup
On 2023/04/27 19:47, Tomasz Dzieciol wrote:
Format of Intel 82576 was changed in comparison to Intel 82574 extended
descriptors. This change updates filling of advanced descriptors fields
accordingly:
* remove TCP ACK detection
* add IPv6 with extensions traffic detection
* fragment checksum and IP ID is filled only when RXCSUM.IPPCSE is set (in
addition to RXCSUM.PCSD bit cleared condition)
Please split up those changes into seperate patches. The relevant
documentation is "Split up long patches" section of:
docs/devel/submitting-a-patch.rst
Refactoring is done in preparation for support of multiple advanced
descriptors RX modes, especially packet-split modes.
Signed-off-by: Tomasz Dzieciol
---
hw/net/e1000e_core.c | 18 +-
hw/net/e1000x_regs.h | 1 +
hw/net/igb_core.c| 478 ---
hw/net/igb_regs.h| 12 +-
hw/net/trace-events | 6 +-
tests/qtest/libqos/igb.c | 3 +
6 files changed, 316 insertions(+), 202 deletions(-)
diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 78373d7db7..0085ad53c2 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1418,11 +1418,11 @@ e1000e_write_hdr_to_rx_buffers(E1000ECore *core,
}
static void
-e1000e_write_to_rx_buffers(E1000ECore *core,
- hwaddr ba[MAX_PS_BUFFERS],
- e1000e_ba_state *bastate,
- const char *data,
- dma_addr_t data_len)
+e1000e_write_payload_frag_to_rx_buffers(E1000ECore *core,
+hwaddr ba[MAX_PS_BUFFERS],
+e1000e_ba_state *bastate,
+const char *data,
+dma_addr_t data_len)
{
while (data_len > 0) {
uint32_t cur_buf_len = core->rxbuf_sizes[bastate->cur_idx];
@@ -1594,8 +1594,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct
NetRxPkt *pkt,
while (copy_size) {
iov_copy = MIN(copy_size, iov->iov_len - iov_ofs);
-e1000e_write_to_rx_buffers(core, ba, &bastate,
-iov->iov_base + iov_ofs, iov_copy);
+e1000e_write_payload_frag_to_rx_buffers(core, ba, &bastate,
+iov->iov_base +
+iov_ofs,
+iov_copy);
copy_size -= iov_copy;
iov_ofs += iov_copy;
@@ -1607,7 +1609,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct
NetRxPkt *pkt,
if (desc_offset + desc_size >= total_size) {
/* Simulate FCS checksum presence in the last descriptor
*/
-e1000e_write_to_rx_buffers(core, ba, &bastate,
+e1000e_write_payload_frag_to_rx_buffers(core, ba, &bastate,
(const char *) &fcs_pad, e1000x_fcs_len(core->mac));
}
}
diff --git a/hw/net/e1000x_regs.h b/hw/net/e1000x_regs.h
index 13760c66d3..344fd10359 100644
--- a/hw/net/e1000x_regs.h
+++ b/hw/net/e1000x_regs.h
@@ -827,6 +827,7 @@ union e1000_rx_desc_packet_split {
/* Receive Checksum Control bits */
#define E1000_RXCSUM_IPOFLD 0x100 /* IP Checksum Offload Enable */
#define E1000_RXCSUM_TUOFLD 0x200 /* TCP/UDP Checksum Offload Enable */
+#define E1000_RXCSUM_IPPCSE 0x1000 /* IP Payload Checksum enable */
#define E1000_RXCSUM_PCSD 0x2000 /* Packet Checksum Disable */
#define E1000_RING_DESC_LEN (16)
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index 96b7335b31..1cb64402aa 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -267,6 +267,21 @@ igb_rx_use_legacy_descriptor(IGBCore *core)
return false;
}
+typedef struct E1000E_RingInfo_st {
+int dbah;
+int dbal;
+int dlen;
+int dh;
+int dt;
+int idx;
+} E1000E_RingInfo;
+
+static uint32_t
+igb_rx_queue_desctyp_get(IGBCore *core, const E1000E_RingInfo *r)
+{
+return core->mac[E1000_SRRCTL(r->idx) >> 2] & E1000_SRRCTL_DESCTYPE_MASK;
+}
+
static inline bool
igb_rss_enabled(IGBCore *core)
{
@@ -694,15 +709,6 @@ static uint32_t igb_rx_wb_eic(IGBCore *core, int queue_idx)
return (ent & E1000_IVAR_VALID) ? BIT(ent & 0x1f) : 0;
}
-typedef struct E1000E_RingInfo_st {
-int dbah;
-int dbal;
-int dlen;
-int dh;
-int dt;
-int idx;
-} E1000E_RingInfo;
-
static inline bool
igb_ring_empty(IGBCore *core, const E1000E_RingInfo *r)
{
@@ -941,6 +947,14 @@ igb_has_rxbufs(IGBCore *core, const E1000E_RingInfo *r,
size_t total_size)
bufsize;
}
+static uint32_t
+igb_get_queue_rx_header_buf_size(IGBCore *core, const E1000E_RingInfo *r)
+{
+u
[PULL 09/17] qapi: Fix bullet list markup in documentation
Peter Maydell's commit 100cc4fe0f08 explains: rST insists on a blank line before and after a bulleted list [...] Add some extra blank lines in the doc comments so they're acceptable rST input. It missed one in qapi/trace.json. Paolo Bonzini later added another instance in qapi/stats.json, providing further, if unintended, evidence for his quip that rST is the Perl of ASCII-based markups. Both are parsed as ordinary paragraph, resulting in garbled output. John Snow missed the need for a blank line when converting docs/devel/qapi-code-gen.txt to rST. Add the blank lines we need to get the bullet lists recognized as such. Kevin Wolf and Lukas Straub added two more, but indented. Sphinx recognizes them as (indented) bullet lists. The indentation looks slightly off. Insert a blank line and delete the extra indentation. Fixes: 100cc4fe0f0827f8da1a5c05f9c65e2aaa40e03d (qapi: Add blank lines before bulleted lists) Fixes: 467ef823d83e (qmp: add filtering of statistics by target vCPU) Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-10-arm...@redhat.com> [Fix of docs/devel/qapi-code-gen.rst squashed, commit message adjusted] --- docs/devel/qapi-code-gen.rst | 1 + qapi/block-export.json | 7 --- qapi/stats.json | 1 + qapi/trace.json | 1 + qapi/yank.json | 21 +++-- 5 files changed, 18 insertions(+), 13 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index d81aac7a19..ea0592034a 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -934,6 +934,7 @@ Example:: ## # Some text foo with **bold** and *emphasis* + # # 1. with a list # 2. like that # diff --git a/qapi/block-export.json b/qapi/block-export.json index 4627bbc4e6..3be3de357f 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -275,9 +275,10 @@ # @deprecated: This command is deprecated. Use @block-export-del instead. # # Returns: error if -#- the server is not running -#- export is not found -#- mode is 'safe' and there are existing connections +# +# - the server is not running +# - export is not found +# - mode is 'safe' and there are existing connections # # Since: 2.12 ## diff --git a/qapi/stats.json b/qapi/stats.json index 1f5d3c59ab..f17495ee65 100644 --- a/qapi/stats.json +++ b/qapi/stats.json @@ -107,6 +107,7 @@ # The arguments to the query-stats command; specifies a target for which to # request statistics and optionally the required subset of information for # that target: +# # - which vCPUs to request statistics for # - which providers to request statistics from # - which named values to return within each provider diff --git a/qapi/trace.json b/qapi/trace.json index 6c6982a587..f425d10764 100644 --- a/qapi/trace.json +++ b/qapi/trace.json @@ -87,6 +87,7 @@ # @vcpu: The vCPU to act upon (all by default; since 2.7). # # An event's state is modified if: +# # - its name matches the @name pattern, and # - if @vcpu is given, the event has the "vcpu" property. # diff --git a/qapi/yank.json b/qapi/yank.json index 167a775594..1639744ada 100644 --- a/qapi/yank.json +++ b/qapi/yank.json @@ -50,16 +50,17 @@ # hanging QEMU. # # Currently implemented yank instances: -# - nbd block device: -#Yanking it will shut down the connection to the nbd server without -#attempting to reconnect. -# - socket chardev: -#Yanking it will shut down the connected socket. -# - migration: -#Yanking it will shut down all migration connections. Unlike -#@migrate_cancel, it will not notify the migration process, so migration -#will go into @failed state, instead of @cancelled state. @yank should be -#used to recover from hangs. +# +# - nbd block device: +# Yanking it will shut down the connection to the nbd server without +# attempting to reconnect. +# - socket chardev: +# Yanking it will shut down the connected socket. +# - migration: +# Yanking it will shut down all migration connections. Unlike +# @migrate_cancel, it will not notify the migration process, so migration +# will go into @failed state, instead of @cancelled state. @yank should be +# used to recover from hangs. # # Since: 6.0 ## -- 2.39.2
[PULL 02/17] qga/qapi-schema: Fix a misspelled reference
Code returns a list of GuestNetworkInterface, documentation claims GuestNetworkInfo, which doesn't exist. Fix the documentation. Fixes: 3424fc9f16a1 (qemu-ga: add guest-network-get-interfaces command) Signed-off-by: Markus Armbruster Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Marc-André Lureau Message-Id: <20230425064223.820979-3-arm...@redhat.com> --- qga/qapi-schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index f349345116..bb9bac0655 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -722,7 +722,7 @@ # Get list of guest IP addresses, MAC addresses # and netmasks. # -# Returns: List of GuestNetworkInfo on success. +# Returns: List of GuestNetworkInterface on success. # # Since: 1.1 ## -- 2.39.2
[PULL 13/17] qapi: Replace ad hoc "since" documentation by member documentation
MemoryDeviceInfoKind, NetClientDriver, and GuestPanicAction mention
some members only in ad hoc since documentation. The generated
documentation shows these members as "Not documented".
Replace by formal member documentation.
Add actual documentation text for the GuestPanicAction members, to
match existing member documentation there. For the others, merely
move existing "since" information.
Signed-off-by: Markus Armbruster
Reviewed-by: Vladimir Sementsov-Ogievskiy
Reviewed-by: Marc-André Lureau
Message-Id: <20230425064223.820979-14-arm...@redhat.com>
---
qapi/machine.json | 11 ---
qapi/net.json | 21 -
qapi/run-state.json | 6 +-
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/qapi/machine.json b/qapi/machine.json
index 1a90376f4e..5a18913767 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1260,6 +1260,14 @@
##
# @MemoryDeviceInfoKind:
#
+# @nvdimm: since 2.12
+#
+# @virtio-pmem: since 4.1
+#
+# @virtio-mem: since 5.1
+#
+# @sgx-epc: since 6.2.
+#
# Since: 2.1
##
{ 'enum': 'MemoryDeviceInfoKind',
@@ -1302,9 +1310,6 @@
#
# Union containing information about a memory device
#
-# nvdimm is included since 2.12. virtio-pmem is included since 4.1.
-# virtio-mem is included since 5.1. sgx-epc is included since 6.2.
-#
# Since: 2.1
##
{ 'union': 'MemoryDeviceInfo',
diff --git a/qapi/net.json b/qapi/net.json
index ec66b39b70..3606d9d27f 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -641,14 +641,15 @@
#
# Available netdev drivers.
#
-# Since: 2.7
+# @l2tpv3: since 2.1
+# @vhost-vdpa: since 5.1
+# @vmnet-host: since 7.1
+# @vmnet-shared: since 7.1
+# @vmnet-bridged: since 7.1
+# @stream: since 7.2
+# @dgram: since 7.2
#
-#@vhost-vdpa since 5.1
-#@vmnet-host since 7.1
-#@vmnet-shared since 7.1
-#@vmnet-bridged since 7.1
-#@stream since 7.2
-#@dgram since 7.2
+# Since: 2.7
##
{ 'enum': 'NetClientDriver',
'data': [ 'none', 'nic', 'user', 'tap', 'l2tpv3', 'socket', 'stream',
@@ -669,12 +670,6 @@
#
# Since: 1.2
#
-#'l2tpv3' - since 2.1
-#'vmnet-host' - since 7.1
-#'vmnet-shared' - since 7.1
-#'vmnet-bridged' - since 7.1
-#'stream' since 7.2
-#'dgram' since 7.2
##
{ 'union': 'Netdev',
'base': { 'id': 'str', 'type': 'NetClientDriver' },
diff --git a/qapi/run-state.json b/qapi/run-state.json
index bfc15ecad5..e5f5d31395 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -467,7 +467,11 @@
#
# @pause: system pauses
#
-# Since: 2.1 (poweroff since 2.8, run since 5.0)
+# @poweroff: system powers off (since 2.8)
+#
+# @run: system continues to run (since 5.0)
+#
+# Since: 2.1
##
{ 'enum': 'GuestPanicAction',
'data': [ 'pause', 'poweroff', 'run' ] }
--
2.39.2
Re: [PULL 03/18] tests/avocado: Add set of boot tests on SBSA-ref
On 27/04/2023 17.44, Alex Bennée wrote: From: Philippe Mathieu-Daudé This change adds set of boot tests on SBSA-ref machine: 1. boot firmware up to the EDK2 banner 2. boot Alpine Linux Prebuilt flash volumes are included, built using upstream documentation. To unify tests for AArch64/virt and AArch64/sbsa-ref we boot the same Alpine Linux image on both. Signed-off-by: Marcin Juszkiewicz Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20230323082813.971535-1-marcin.juszkiew...@linaro.org> Reviewed-by: Leif Lindholm Message-Id: <20230328171426.14258-1-phi...@linaro.org> Signed-off-by: Alex Bennée Message-Id: <20230424092249.58552-4-alex.ben...@linaro.org> ... +def test_sbsaref_edk2_firmware(self): +""" +:avocado: tags=cpu:cortex-a57 +""" This is failing for me in the gitlab-CI: https://gitlab.com/thuth/qemu/-/jobs/4196177756#L489 Could you please have a look? Thanks, Thomas
[PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time
Drop inline marker: let compiler decide.
Change return type to uint64_t: this matches the computation in the
return statement and the local variable assignment in the caller.
Rename local to dirty_ring_size_MB to fix typo.
Simplify conversion to MiB via qemu_target_page_bits and right shift.
Signed-off-by: Richard Henderson
---
softmmu/dirtylimit.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 82986c1499..71bf6dc7a4 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -232,18 +232,23 @@ bool dirtylimit_vcpu_index_valid(int cpu_index)
cpu_index >= ms->smp.max_cpus);
}
-static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
+static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
{
static uint64_t max_dirtyrate;
-uint32_t dirty_ring_size = kvm_dirty_ring_size();
-uint64_t dirty_ring_size_meory_MB =
-dirty_ring_size * qemu_target_page_size() >> 20;
+unsigned target_page_bits = qemu_target_page_bits();
+uint64_t dirty_ring_size_MB;
+
+/* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
+assert(target_page_bits < 20);
+
+/* Convert ring size (pages) to MiB (2**20). */
+dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
if (max_dirtyrate < dirtyrate) {
max_dirtyrate = dirtyrate;
}
-return dirty_ring_size_meory_MB * 100 / max_dirtyrate;
+return dirty_ring_size_MB * 100 / max_dirtyrate;
}
static inline bool dirtylimit_done(uint64_t quota,
--
2.34.1
Re: [PATCH 1/5] hw/display/virtio-gpu-virgl: virtio_gpu_gl -> virtio_gpu_virgl
On 2023/04/28 11:52, Gurchetan Singh wrote:
From: Gurchetan Singh
The virtio-gpu GL device has a heavy dependence on virgl.
Acknowledge this by naming functions accurately.
Signed-off-by: Gurchetan Singh
Reviewed-by: Philippe Mathieu-Daudé
---
v1:
- (Philippe) virtio_gpu_virglrenderer_reset -->
virtio_gpu_virgl_reset_renderer
hw/display/virtio-gpu-gl.c | 27 ++-
hw/display/virtio-gpu-virgl.c | 2 +-
include/hw/virtio/virtio-gpu.h | 2 +-
3 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index e06be60dfb..7d69050b8c 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -25,9 +25,10 @@
#include
-static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
- struct virtio_gpu_scanout *s,
- uint32_t resource_id)
+static void
+virtio_gpu_virgl_update_cursor(VirtIOGPU *g,
+ struct virtio_gpu_scanout *s,
+ uint32_t resource_id)
nit: This adds a line break between type and name, but it seems to fit
in a line even without the new line break.
{
uint32_t width, height;
uint32_t pixels, *data;
@@ -48,14 +49,14 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
free(data);
}
-static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
+static void virtio_gpu_virgl_flushed(VirtIOGPUBase *b)
{
VirtIOGPU *g = VIRTIO_GPU(b);
virtio_gpu_process_cmdq(g);
}
-static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_gpu_virgl_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
{
VirtIOGPU *g = VIRTIO_GPU(vdev);
VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -71,7 +72,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
}
if (gl->renderer_reset) {
gl->renderer_reset = false;
-virtio_gpu_virgl_reset(g);
+virtio_gpu_virgl_reset_renderer(g);
}
cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
@@ -87,7 +88,7 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
virtio_gpu_virgl_fence_poll(g);
}
-static void virtio_gpu_gl_reset(VirtIODevice *vdev)
+static void virtio_gpu_virgl_reset(VirtIODevice *vdev)
{
VirtIOGPU *g = VIRTIO_GPU(vdev);
VirtIOGPUGL *gl = VIRTIO_GPU_GL(vdev);
@@ -104,7 +105,7 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
}
}
-static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
+static void virtio_gpu_virgl_device_realize(DeviceState *qdev, Error **errp)
{
VirtIOGPU *g = VIRTIO_GPU(qdev);
@@ -143,13 +144,13 @@ static void virtio_gpu_gl_class_init(ObjectClass *klass, void *data)
VirtIOGPUBaseClass *vbc = VIRTIO_GPU_BASE_CLASS(klass);
VirtIOGPUClass *vgc = VIRTIO_GPU_CLASS(klass);
-vbc->gl_flushed = virtio_gpu_gl_flushed;
-vgc->handle_ctrl = virtio_gpu_gl_handle_ctrl;
+vbc->gl_flushed = virtio_gpu_virgl_flushed;
+vgc->handle_ctrl = virtio_gpu_virgl_handle_ctrl;
vgc->process_cmd = virtio_gpu_virgl_process_cmd;
-vgc->update_cursor_data = virtio_gpu_gl_update_cursor_data;
+vgc->update_cursor_data = virtio_gpu_virgl_update_cursor;
-vdc->realize = virtio_gpu_gl_device_realize;
-vdc->reset = virtio_gpu_gl_reset;
+vdc->realize = virtio_gpu_virgl_device_realize;
+vdc->reset = virtio_gpu_virgl_reset;
device_class_set_props(dc, virtio_gpu_gl_properties);
}
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 1c47603d40..ffe4ec7f3d 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -599,7 +599,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
}
}
-void virtio_gpu_virgl_reset(VirtIOGPU *g)
+void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g)
{
virgl_renderer_reset();
}
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 2e28507efe..21b0f55bc8 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -281,7 +281,7 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd);
void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
-void virtio_gpu_virgl_reset(VirtIOGPU *g);
+void virtio_gpu_virgl_reset_renderer(VirtIOGPU *g);
int virtio_gpu_virgl_init(VirtIOGPU *g);
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
[PATCH] rcu: remove qatomic_mb_set, expand comments
Signed-off-by: Paolo Bonzini
---
include/qemu/rcu.h | 5 -
util/rcu.c | 24 +++-
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 313fc414bc2a..661c1a146872 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -87,7 +87,10 @@ static inline void rcu_read_lock(void)
ctr = qatomic_read(&rcu_gp_ctr);
qatomic_set(&p_rcu_reader->ctr, ctr);
-/* Write p_rcu_reader->ctr before reading RCU-protected pointers. */
+/*
+ * Read rcu_gp_ptr and write p_rcu_reader->ctr before reading
+ * RCU-protected pointers.
+ */
smp_mb_placeholder();
}
diff --git a/util/rcu.c b/util/rcu.c
index b6d6c71cff5c..e5b6e52be6f8 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -83,12 +83,6 @@ static void wait_for_readers(void)
*/
qemu_event_reset(&rcu_gp_event);
-/* Instead of using qatomic_mb_set for index->waiting, and
- * qatomic_mb_read for index->ctr, memory barriers are placed
- * manually since writes to different threads are independent.
- * qemu_event_reset has acquire semantics, so no memory barrier
- * is needed here.
- */
QLIST_FOREACH(index, ®istry, node) {
qatomic_set(&index->waiting, true);
}
@@ -96,6 +90,10 @@ static void wait_for_readers(void)
/* Here, order the stores to index->waiting before the loads of
* index->ctr. Pairs with smp_mb_placeholder() in rcu_read_unlock(),
* ensuring that the loads of index->ctr are sequentially consistent.
+ *
+ * If this is the last iteration, this barrier also prevents
+ * frees from seeping upwards, and orders the two wait phases
+ * on architectures with 32-bit longs; see synchronize_rcu().
*/
smp_mb_global();
@@ -104,7 +102,7 @@ static void wait_for_readers(void)
QLIST_REMOVE(index, node);
QLIST_INSERT_HEAD(&qsreaders, index, node);
-/* No need for mb_set here, worst of all we
+/* No need for memory barriers here, worst of all we
* get some extra futex wakeups.
*/
qatomic_set(&index->waiting, false);
@@ -149,26 +147,26 @@ void synchronize_rcu(void)
/* Write RCU-protected pointers before reading p_rcu_reader->ctr.
* Pairs with smp_mb_placeholder() in rcu_read_lock().
+ *
+ * Also orders write to RCU-protected pointers before
+ * write to rcu_gp_ctr.
*/
smp_mb_global();
QEMU_LOCK_GUARD(&rcu_registry_lock);
if (!QLIST_EMPTY(®istry)) {
-/* In either case, the qatomic_mb_set below blocks stores that free
- * old RCU-protected pointers.
- */
if (sizeof(rcu_gp_ctr) < 8) {
/* For architectures with 32-bit longs, a two-subphases algorithm
* ensures we do not encounter overflow bugs.
*
* Switch parity: 0 -> 1, 1 -> 0.
*/
-qatomic_mb_set(&rcu_gp_ctr, rcu_gp_ctr ^ RCU_GP_CTR);
+qatomic_set(&rcu_gp_ctr, rcu_gp_ctr ^ RCU_GP_CTR);
wait_for_readers();
-qatomic_mb_set(&rcu_gp_ctr, rcu_gp_ctr ^ RCU_GP_CTR);
+qatomic_set(&rcu_gp_ctr, rcu_gp_ctr ^ RCU_GP_CTR);
} else {
/* Increment current grace period. */
-qatomic_mb_set(&rcu_gp_ctr, rcu_gp_ctr + RCU_GP_CTR);
+qatomic_set(&rcu_gp_ctr, rcu_gp_ctr + RCU_GP_CTR);
}
wait_for_readers();
--
2.40.0
[PATCH 02/17] docs/devel/qapi-code-gen: Turn FIXME admonitions into comments
We have two FIXME notes. These FIXMEs are for QAPI developers. They are not useful for QAPI schema developers. They are marked up as admonitions, which makes them look important in generated HTML. Turn them into comments. QAPI developers will still see them (they read and write the .rst). QAPI schema developers may still see them (if they read the .rst instead of the generated .html), but "this is just for QAPI developers" should be more obvious. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index 289869c53b..ff7b74bdb2 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -979,14 +979,9 @@ description:: # in the second style. The number of spaces between the ':' and the text is not significant. +.. FIXME The parser accepts these things in almost any order. -.. admonition:: FIXME - - The parser accepts these things in almost any order. - -.. admonition:: FIXME - - union branches should be described, too. +.. FIXME union branches should be described, too. Extensions added after the definition was first released carry a "(since x.y.z)" comment. -- 2.39.2
[PATCH 07/17] qapi: Tidy up a slightly awkward TODO comment
MigrateSetParameters has a TODO comment sitting right behind its doc
comment. I wrote it this way to keep it out of the manual, but that
reason is not obvious.
The previous commit (sphinx/qapidoc: Do not emit TODO sections into
user manuals) lets me move it into the doc comment as a TODO section.
Signed-off-by: Markus Armbruster
---
qapi/migration.json | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 82000adce4..11c09800c2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -945,10 +945,11 @@
# Features:
# @unstable: Member @x-checkpoint-delay is experimental.
#
+# TODO: either fuse back into MigrationParameters, or make
+# MigrationParameters members mandatory
+#
# Since: 2.4
##
-# TODO either fuse back into MigrationParameters, or make
-# MigrationParameters members mandatory
{ 'struct': 'MigrateSetParameters',
'data': { '*announce-initial': 'size',
'*announce-max': 'size',
--
2.39.2
[PATCH 04/17] meson: Fix to make QAPI generator output depend on main.py
@qapi_gen_depends is missing scripts/qapi/main.py. Fix that, and drop a duplicate scripts/qapi/common.py. Signed-off-by: Markus Armbruster --- meson.build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meson.build b/meson.build index c44d05a13f..ed24df2ade 100644 --- a/meson.build +++ b/meson.build @@ -2834,12 +2834,12 @@ qapi_gen_depends = [ meson.current_source_dir() / 'scripts/qapi/__init__.py', meson.current_source_dir() / 'scripts/qapi/expr.py', meson.current_source_dir() / 'scripts/qapi/gen.py', meson.current_source_dir() / 'scripts/qapi/introspect.py', + meson.current_source_dir() / 'scripts/qapi/main.py', meson.current_source_dir() / 'scripts/qapi/parser.py', meson.current_source_dir() / 'scripts/qapi/schema.py', meson.current_source_dir() / 'scripts/qapi/source.py', meson.current_source_dir() / 'scripts/qapi/types.py', meson.current_source_dir() / 'scripts/qapi/visit.py', - meson.current_source_dir() / 'scripts/qapi/common.py', meson.current_source_dir() / 'scripts/qapi-gen.py' ] -- 2.39.2
[PATCH 15/17] docs/devel/qapi-code-gen: Update doc comment conventions
The commit before previous relaxed the indentation rules to let us improve the doc comment conventions. This commit changes the written conventions. The next commits will update QAPI schemas to conform to them. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 46 +--- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index 875f893be2..5618a80378 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -947,6 +947,11 @@ Example:: # <- get that ## +For legibility, wrap text paragraphs so every line is at most 70 +characters long. + +Separate sentences with two spaces. + Definition documentation @@ -963,22 +968,12 @@ commands and events), member (for structs and unions), branch (for alternates), or value (for enums), a description of each feature (if any), and finally optional tagged sections. -The description of an argument or feature 'name' starts with -'\@name:'. The description text can start on the line following the -'\@name:', in which case it must not be indented at all. It can also -start on the same line as the '\@name:'. In this case if it spans -multiple lines then second and subsequent lines must be indented to -line up with the first character of the first line of the -description:: +Descriptions start with '\@name:'. The description text should be +indented like this:: - # @argone: - # This is a two line description - # in the first style. - # - # @argtwo: This is a two line description - # in the second style. + # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed + # do eiusmod tempor incididunt ut labore et dolore magna aliqua. -The number of spaces between the ':' and the text is not significant. .. FIXME The parser accepts these things in almost any order. .. FIXME union branches should be described, too. @@ -990,23 +985,26 @@ The feature descriptions must be preceded by a line "Features:", like this:: # Features: + # # @feature: Description text A tagged section starts with one of the following words: "Note:"/"Notes:", "Since:", "Example"/"Examples", "Returns:", "TODO:". The section ends with the start of a new section. -The text of a section can start on a new line, in -which case it must not be indented at all. It can also start -on the same line as the "Note:", "Returns:", etc tag. In this -case if it spans multiple lines then second and subsequent -lines must be indented to match the first, in the same way as -multiline argument descriptions. +The second and subsequent lines of sections other than +"Example"/"Examples" should be indented like this:: + + # Note: Ut enim ad minim veniam, quis nostrud exercitation ullamco + # laboris nisi ut aliquip ex ea commodo consequat. + # + # Duis aute irure dolor in reprehenderit in voluptate velit esse + # cillum dolore eu fugiat nulla pariatur. A "Since: x.y.z" tagged section lists the release that introduced the definition. -An "Example" or "Examples" section is automatically rendered entirely +An "Example" or "Examples" section is rendered entirely as literal fixed-width text. "TODO" sections are not rendered at all (they are for developers, not users of QMP). In other sections, the text is formatted, and rST markup can be used. @@ -1019,7 +1017,7 @@ For example:: # Statistics of a virtual block device or a block backing device. # # @device: If the stats are for a virtual block device, the name - # corresponding to the virtual block device. + # corresponding to the virtual block device. # # @node-name: The node name of the device. (since 2.3) # @@ -1036,8 +1034,8 @@ For example:: # # Query the @BlockStats for all virtual block devices. # - # @query-nodes: If true, the command will query all the - # block nodes ... explain, explain ... (since 2.3) + # @query-nodes: If true, the command will query all the block nodes + # ... explain, explain ... (since 2.3) # # Returns: A list of @BlockStats for each virtual block devices. # -- 2.39.2
[PATCH 16/17] qga/qapi-schema: Reformat doc comments to conform to current conventions
Change # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed #do eiusmod tempor incididunt ut labore et dolore magna aliqua. to # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed # do eiusmod tempor incididunt ut labore et dolore magna aliqua. See recent commit "qapi: Relax doc string @name: description indentation rules" for rationale. Reflow paragraphs to 70 columns width, and consistently use two spaces to separate sentences. To check the generated documentation does not change, I compared the generated HTML before and after this commit with "wdiff -3". Finds no differences. Comparing with diff is not useful, as the reflown paragraphs are visible there. Signed-off-by: Markus Armbruster --- qga/qapi-schema.json | 668 +-- 1 file changed, 382 insertions(+), 286 deletions(-) diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json index 6a20eeb297..721a0baf3e 100644 --- a/qga/qapi-schema.json +++ b/qga/qapi-schema.json @@ -4,10 +4,10 @@ ## # = General note concerning the use of guest agent interfaces # -# "unsupported" is a higher-level error than the errors that individual -# commands might document. The caller should always be prepared to receive -# QERR_UNSUPPORTED, even if the given command doesn't specify it, or doesn't -# document any failure mode at all. +# "unsupported" is a higher-level error than the errors that +# individual commands might document. The caller should always be +# prepared to receive QERR_UNSUPPORTED, even if the given command +# doesn't specify it, or doesn't document any failure mode at all. ## ## @@ -38,28 +38,27 @@ ## # @guest-sync-delimited: # -# Echo back a unique integer value, and prepend to response a -# leading sentinel byte (0xFF) the client can check scan for. +# Echo back a unique integer value, and prepend to response a leading +# sentinel byte (0xFF) the client can check scan for. # -# This is used by clients talking to the guest agent over the -# wire to ensure the stream is in sync and doesn't contain stale -# data from previous client. It must be issued upon initial -# connection, and after any client-side timeouts (including -# timeouts on receiving a response to this command). +# This is used by clients talking to the guest agent over the wire to +# ensure the stream is in sync and doesn't contain stale data from +# previous client. It must be issued upon initial connection, and +# after any client-side timeouts (including timeouts on receiving a +# response to this command). # # After issuing this request, all guest agent responses should be -# ignored until the response containing the unique integer value -# the client passed in is returned. Receival of the 0xFF sentinel -# byte must be handled as an indication that the client's -# lexer/tokenizer/parser state should be flushed/reset in -# preparation for reliably receiving the subsequent response. As -# an optimization, clients may opt to ignore all data until a -# sentinel value is receiving to avoid unnecessary processing of -# stale data. +# ignored until the response containing the unique integer value the +# client passed in is returned. Receival of the 0xFF sentinel byte +# must be handled as an indication that the client's +# lexer/tokenizer/parser state should be flushed/reset in preparation +# for reliably receiving the subsequent response. As an optimization, +# clients may opt to ignore all data until a sentinel value is +# receiving to avoid unnecessary processing of stale data. # -# Similarly, clients should also precede this *request* -# with a 0xFF byte to make sure the guest agent flushes any -# partially read JSON data from a previous client connection. +# Similarly, clients should also precede this *request* with a 0xFF +# byte to make sure the guest agent flushes any partially read JSON +# data from a previous client connection. # # @id: randomly generated 64-bit integer # @@ -76,28 +75,27 @@ # # Echo back a unique integer value # -# This is used by clients talking to the guest agent over the -# wire to ensure the stream is in sync and doesn't contain stale -# data from previous client. All guest agent responses should be -# ignored until the provided unique integer value is returned, -# and it is up to the client to handle stale whole or -# partially-delivered JSON text in such a way that this response -# can be obtained. +# This is used by clients talking to the guest agent over the wire to +# ensure the stream is in sync and doesn't contain stale data from +# previous client. All guest agent responses should be ignored until +# the provided unique integer value is returned, and it is up to the +# client to handle stale whole or partially-delivered JSON text in +# such a way that this response can be obtained. # -# In cases where a partial stale response was previously -# received by the client, this cannot always be done reliably. -# One particula
[PATCH 01/17] docs/devel/qapi-code-gen: Clean up use of quotes a bit
Section "Definition documentation" uses both single and double quotes around doc text snippets. Stick to double quotes. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index af1986f33e..289869c53b 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -989,7 +989,7 @@ The number of spaces between the ':' and the text is not significant. union branches should be described, too. Extensions added after the definition was first released carry a -'(since x.y.z)' comment. +"(since x.y.z)" comment. The feature descriptions must be preceded by a line "Features:", like this:: @@ -1003,17 +1003,17 @@ The section ends with the start of a new section. The text of a section can start on a new line, in which case it must not be indented at all. It can also start -on the same line as the 'Note:', 'Returns:', etc tag. In this +on the same line as the "Note:", "Returns:", etc tag. In this case if it spans multiple lines then second and subsequent lines must be indented to match the first, in the same way as multiline argument descriptions. -A 'Since: x.y.z' tagged section lists the release that introduced the +A "Since: x.y.z" tagged section lists the release that introduced the definition. -An 'Example' or 'Examples' section is automatically rendered -entirely as literal fixed-width text. In other sections, -the text is formatted, and rST markup can be used. +An "Example" or "Examples" section is automatically rendered entirely +as literal fixed-width text. In other sections, the text is +formatted, and rST markup can be used. For example:: -- 2.39.2
[PATCH 08/17] qapi/dump: Indent bulleted lists consistently
Documentation of dump-guest-memory contains two bulleted lists. The first one is indented, the second one isn't. Delete the first one's indentation for a more consistent look. Signed-off-by: Markus Armbruster --- qapi/dump.json | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/qapi/dump.json b/qapi/dump.json index 6fc215dd47..24af1df7db 100644 --- a/qapi/dump.json +++ b/qapi/dump.json @@ -44,12 +44,12 @@ # # Also, paging=true has the following limitations: # -# 1. The guest may be in a catastrophic state or can have corrupted -#memory, which cannot be trusted -# 2. The guest can be in real-mode even if paging is enabled. For -#example, the guest uses ACPI to sleep, and ACPI sleep state -#goes in real-mode -# 3. Currently only supported on i386 and x86_64. +# 1. The guest may be in a catastrophic state or can have corrupted +# memory, which cannot be trusted +# 2. The guest can be in real-mode even if paging is enabled. For +# example, the guest uses ACPI to sleep, and ACPI sleep state +# goes in real-mode +# 3. Currently only supported on i386 and x86_64. # # @protocol: the filename or file descriptor of the vmcore. The supported #protocols are: -- 2.39.2
[PATCH 09/17] tests/qapi-schema/doc-good: Improve a comment
The QAPI generator doesn't reject undocumented members and features (yet). doc-good.json covers this, with clear "is undocumented" notes to signal intent. Except for @Variant1 member @var1, where it's "(but no @var: line)". Less clear. Replace by "@var1 is undocumented". Signed-off-by: Markus Armbruster --- tests/qapi-schema/doc-good.json | 4 +++- tests/qapi-schema/doc-good.out | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json index 74745fb405..445471daee 100644 --- a/tests/qapi-schema/doc-good.json +++ b/tests/qapi-schema/doc-good.json @@ -83,7 +83,9 @@ # # A paragraph # -# Another paragraph (but no @var: line) +# Another paragraph +# +# @var1 is undocumented # # Features: # @variant1-feat: a feature diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 9dd65b9d92..afa48dcd94 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -123,6 +123,8 @@ doc symbol=Variant1 A paragraph Another paragraph (but no @var: line) + +@var1 is undocumented arg=var1 feature=variant1-feat -- 2.39.2
[PATCH 11/17] qapi: Fix argument description indentation stripping
When an argument's description starts on the line after the "#arg: " line, indentation is stripped only from the description's first line, as demonstrated by the previous commit. Moreover, subsequent lines with less indentation are not rejected. Make the first line's indentation the expected indentation for the remainder of the description. This fixes indentation stripping, and also requires at least that much indentation. Signed-off-by: Markus Armbruster --- scripts/qapi/parser.py | 20 +++- tests/qapi-schema/doc-good.out | 2 +- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 7b49d3ab05..ddc14ceaba 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -483,7 +483,9 @@ def append(self, line: str) -> None: # Blank lines are always OK. if line: indent = must_match(r'\s*', line).end() -if indent < self._indent: +if self._indent < 0: +self._indent = indent +elif indent < self._indent: raise QAPIParseError( self._parser, "unexpected de-indent (expected at least %d spaces)" % @@ -631,9 +633,9 @@ def _append_args_line(self, line: str) -> None: indent = must_match(r'@\S*:\s*', line).end() line = line[indent:] if not line: -# Line was just the "@arg:" header; following lines -# are not indented -indent = 0 +# Line was just the "@arg:" header +# The next non-blank line determines expected indent +indent = -1 else: line = ' ' * indent + line self._start_args_section(name[1:-1], indent) @@ -666,9 +668,9 @@ def _append_features_line(self, line: str) -> None: indent = must_match(r'@\S*:\s*', line).end() line = line[indent:] if not line: -# Line was just the "@arg:" header; following lines -# are not indented -indent = 0 +# Line was just the "@arg:" header +# The next non-blank line determines expected indent +indent = -1 else: line = ' ' * indent + line self._start_features_section(name[1:-1], indent) @@ -712,8 +714,8 @@ def _append_various_line(self, line: str) -> None: indent = must_match(r'\S*:\s*', line).end() line = line[indent:] if not line: -# Line was just the "Section:" header; following lines -# are not indented +# Line was just the "Section:" header +# The next non-blank line determines expected indent indent = 0 else: line = ' ' * indent + line diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out index 2ba72ae558..277371acc8 100644 --- a/tests/qapi-schema/doc-good.out +++ b/tests/qapi-schema/doc-good.out @@ -159,7 +159,7 @@ doc symbol=cmd arg=arg1 description starts on a new line, -indented +indented arg=arg2 the second argument arg=arg3 -- 2.39.2
Re: [PATCH] async: Suppress GCC13 false positive in aio_bh_poll()
Queued, thanks. Paolo
[PATCH 03/17] qapi: Fix crash on stray double quote character
When the lexer chokes on a stray character, its shows the characters
until the next structural character in the error message. It uses a
regular expression to match a non-empty string of non-structural
characters. Bug: the regular expression treats '"' as structural.
When the lexer chokes on '"', the match fails, and trips
must_match()'s assertion. Fix the regular expression.
Fixes: 14c32795024c (qapi: Improve reporting of lexical errors)
Signed-off-by: Markus Armbruster
---
scripts/qapi/parser.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 878f90b458..7b49d3ab05 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -346,7 +346,7 @@ def accept(self, skip_comment: bool = True) -> None:
elif not self.tok.isspace():
# Show up to next structural, whitespace or quote
# character
-match = must_match('[^[\\]{}:,\\s\'"]+',
+match = must_match('[^[\\]{}:,\\s\']+',
self.src[self.cursor-1:])
raise QAPIParseError(self, "stray '%s'" % match.group(0))
--
2.39.2
[PATCH 05/17] Revert "qapi: BlockExportRemoveMode: move comments to TODO"
This reverts commit 97cd74f77231f3897838f8db32b659d94803e01f. The next commit will hide TODO: sections. See there for rationale. Signed-off-by: Markus Armbruster --- qapi/block-export.json | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/qapi/block-export.json b/qapi/block-export.json index 3be3de357f..3ec8ad0ce7 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -249,13 +249,13 @@ # # @hard: Drop all connections immediately and remove export. # -# TODO: Potential additional modes to be added in the future: +# Potential additional modes to be added in the future: # -# hide: Just hide export from new clients, leave existing connections as is. -# Remove export after all clients are disconnected. +# hide: Just hide export from new clients, leave existing connections as is. +# Remove export after all clients are disconnected. # -# soft: Hide export from new clients, answer with ESHUTDOWN for all further -# requests from existing clients. +# soft: Hide export from new clients, answer with ESHUTDOWN for all further +# requests from existing clients. # # Since: 2.12 ## -- 2.39.2
[PATCH 00/17] qapi: Reformat doc comments
This series improves the doc comment formatting rules, then reformats doc comments to conform to them. I don't like reformatting code. But I'm tired of looking at ugly doc comments. People imitate them in new work (not blaming them for that), which leads to tiresome arguments about style. I've come to the conclusion that reformatting them is the lesser evil. Prior discussion: qapi: [RFC] Doc comment convention for @arg: sections Message-ID: <87v8irv7zq@pond.sub.org> https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05921.html PATCH 01-12 are preliminary fixes, cleanups and test improvements, ranging from losely related to not related at all. PATCH 13-14 improve the QAPI generator to support the style I want. PATCH 15 writes it down. PATCH 16-17 reformat the doc comments to conform to it. Markus Armbruster (17): docs/devel/qapi-code-gen: Clean up use of quotes a bit docs/devel/qapi-code-gen: Turn FIXME admonitions into comments qapi: Fix crash on stray double quote character meson: Fix to make QAPI generator output depend on main.py Revert "qapi: BlockExportRemoveMode: move comments to TODO" sphinx/qapidoc: Do not emit TODO sections into user manuals qapi: Tidy up a slightly awkward TODO comment qapi/dump: Indent bulleted lists consistently tests/qapi-schema/doc-good: Improve a comment tests/qapi-schema/doc-good: Improve argument description tests qapi: Fix argument description indentation stripping qapi: Rewrite parsing of doc comment section symbols and tags qapi: Relax doc string @name: description indentation rules qapi: Section parameter @indent is no longer used, drop docs/devel/qapi-code-gen: Update doc comment conventions qga/qapi-schema: Reformat doc comments to conform to current conventions qapi: Reformat doc comments to conform to current conventions docs/devel/qapi-code-gen.rst | 74 +- docs/sphinx/qapidoc.py|3 + meson.build |2 +- qapi/acpi.json| 50 +- qapi/audio.json | 85 +- qapi/authz.json | 29 +- qapi/block-core.json | 2801 + qapi/block-export.json| 244 ++- qapi/block.json | 214 +- qapi/char.json| 134 +- qapi/common.json | 19 +- qapi/compat.json | 13 +- qapi/control.json | 59 +- qapi/crypto.json | 261 ++- qapi/cryptodev.json |3 + qapi/cxl.json | 74 +- qapi/dump.json| 78 +- qapi/error.json |6 +- qapi/introspect.json | 89 +- qapi/job.json | 139 +- qapi/machine-target.json | 303 +-- qapi/machine.json | 389 ++-- qapi/migration.json | 1120 +- qapi/misc-target.json | 67 +- qapi/misc.json| 180 +- qapi/net.json | 260 ++- qapi/pci.json | 35 +- qapi/qapi-schema.json | 25 +- qapi/qdev.json| 63 +- qapi/qom.json | 404 ++-- qapi/rdma.json|1 - qapi/replay.json | 48 +- qapi/rocker.json | 20 +- qapi/run-state.json | 215 +- qapi/sockets.json | 50 +- qapi/stats.json | 83 +- qapi/tpm.json | 20 +- qapi/trace.json | 34 +- qapi/transaction.json | 87 +- qapi/ui.json | 435 ++-- qapi/virtio.json | 84 +- qapi/yank.json| 42 +- qga/qapi-schema.json | 668 +++--- scripts/qapi/parser.py| 137 +- tests/qapi-schema/doc-bad-indent.err |2 +- tests/qapi-schema/doc-bad-indent.json |3 +- tests/qapi-schema/doc-good.json | 20 +- tests/qapi-schema/doc-good.out| 19 +- 48 files changed, 4822 insertions(+), 4369 deletions(-) -- 2.39.2
[PATCH 06/17] sphinx/qapidoc: Do not emit TODO sections into user manuals
QAPI doc comments are for QMP users: they go into the "QEMU QMP
Reference Manual" and the "QEMU Storage Daemon QMP Reference Manual".
The doc comment TODO sections are for somebody else, namely for the
people who can do: developers. Do not emit them into the user
manuals.
This elides the following TODOs:
* SchemaInfoCommand
# TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
This is a note to developers adding introspection to the guest
agent. It makes no sense to users.
* @query-hotpluggable-cpus
# TODO: Better documentation; currently there is none.
This is a reminder for developers. It doesn't help users.
* @device_add
# TODO: This command effectively bypasses QAPI completely due to its
# "additional arguments" business. It shouldn't have been added to
# the schema in this form. It should be qapified properly, or
# replaced by a properly qapified command.
Likewise.
Eliding them is an improvement.
Signed-off-by: Markus Armbruster
---
docs/devel/qapi-code-gen.rst | 5 +++--
docs/sphinx/qapidoc.py | 3 +++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
index ff7b74bdb2..6386b58881 100644
--- a/docs/devel/qapi-code-gen.rst
+++ b/docs/devel/qapi-code-gen.rst
@@ -1007,8 +1007,9 @@ A "Since: x.y.z" tagged section lists the release that
introduced the
definition.
An "Example" or "Examples" section is automatically rendered entirely
-as literal fixed-width text. In other sections, the text is
-formatted, and rST markup can be used.
+as literal fixed-width text. "TODO" sections are not rendered at all
+(they are for developers, not users of QMP). In other sections, the
+text is formatted, and rST markup can be used.
For example::
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index d791b59492..8f3b9997a1 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -268,6 +268,9 @@ def _nodes_for_sections(self, doc):
"""Return list of doctree nodes for additional sections"""
nodelist = []
for section in doc.sections:
+if section.name and section.name == 'TODO':
+# Hide TODO: sections
+continue
snode = self._make_section(section.name)
if section.name and section.name.startswith('Example'):
snode += self._nodes_for_example(section.text)
--
2.39.2
[PATCH 13/17] qapi: Relax doc string @name: description indentation rules
The QAPI schema doc comment language provides special syntax for command and event arguments, struct and union members, alternate branches, enumeration values, and features: descriptions starting with "@name:". By convention, we format them like this: # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, #sed do eiusmod tempor incididunt ut labore et dolore #magna aliqua. Okay for names as short as "name", but we have much longer ones. Their description gets squeezed against the right margin, like this: # @dirty-sync-missed-zero-copy: Number of times dirty RAM synchronization could # not avoid copying dirty pages. This is between # 0 and @dirty-sync-count * @multifd-channels. # (since 7.1) The description text is effectively just 50 characters wide. Easy enough to read, but can be cumbersome to write. The awkward squeeze against the right margin makes people go beyond it, which produces two undesirables: arguments about style, and descriptions that are unnecessarily hard to read, like this one: # @postcopy-vcpu-blocktime: list of the postcopy blocktime per vCPU. This is # only present when the postcopy-blocktime migration capability # is enabled. (Since 3.0) We could instead format it like # @postcopy-vcpu-blocktime: # list of the postcopy blocktime per vCPU. This is only present # when the postcopy-blocktime migration capability is # enabled. (Since 3.0) or, since the commit before previous, like # @postcopy-vcpu-blocktime: # list of the postcopy blocktime per vCPU. This is only present # when the postcopy-blocktime migration capability is # enabled. (Since 3.0) However, I'd rather have # @postcopy-vcpu-blocktime: list of the postcopy blocktime per vCPU. # This is only present when the postcopy-blocktime migration # capability is enabled. (Since 3.0) because this is how rST field and option lists work. To get this, we need to let the first non-blank line after the "@name:" line determine expected indentation. This fills up the indentation pitfall mentioned in docs/devel/qapi-code-gen.rst. A related pitfall still exists. Update the text to show it. Signed-off-by: Markus Armbruster --- docs/devel/qapi-code-gen.rst | 10 ++-- scripts/qapi/parser.py| 73 +++ tests/qapi-schema/doc-bad-indent.err | 2 +- tests/qapi-schema/doc-bad-indent.json | 3 +- tests/qapi-schema/doc-good.json | 3 +- tests/qapi-schema/doc-good.out| 3 +- 6 files changed, 32 insertions(+), 62 deletions(-) diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst index 6386b58881..875f893be2 100644 --- a/docs/devel/qapi-code-gen.rst +++ b/docs/devel/qapi-code-gen.rst @@ -1074,10 +1074,14 @@ Indentation matters. Bad example:: # @none: None (no memory side cache in this proximity domain, # or cache associativity unknown) + # (since 5.0) -The description is parsed as a definition list with term "None (no -memory side cache in this proximity domain," and definition "or cache -associativity unknown)". +The last line's de-indent is wrong. The second and subsequent lines +need to line up with each other, like this:: + + # @none: None (no memory side cache in this proximity domain, + # or cache associativity unknown) + # (since 5.0) Section tags are case-sensitive and end with a colon. Good example:: diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index fc04c4573e..3e29b7bf48 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -474,17 +474,21 @@ def __init__(self, parser: QAPISchemaParser, self._parser = parser # optional section name (argument/member or section name) self.name = name +# section text without section name self.text = '' -# the expected indent level of the text of this section -self._indent = indent +# indentation to strip (None means indeterminate) +self._indent = None if self.name else 0 def append(self, line: str) -> None: -# Strip leading spaces corresponding to the expected indent level -# Blank lines are always OK. +line = line.rstrip() + if line: indent = must_match(r'\s*', line).end() -if self._indent < 0: -self._indent = indent +if self._indent is None: +# indeterminate indentation +if self.text != '': +# non-blank, non-first line determines indentation +self._indent = indent elif indent < self._indent: raise QAPIPa
Re: [PATCH] softmmu: Tidy dirtylimit_dirty_ring_full_time
On 28/04/2023 12.34, Richard Henderson wrote:
Drop inline marker: let compiler decide.
Change return type to uint64_t: this matches the computation in the
return statement and the local variable assignment in the caller.
Rename local to dirty_ring_size_MB to fix typo.
Simplify conversion to MiB via qemu_target_page_bits and right shift.
Signed-off-by: Richard Henderson
---
softmmu/dirtylimit.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index 82986c1499..71bf6dc7a4 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -232,18 +232,23 @@ bool dirtylimit_vcpu_index_valid(int cpu_index)
cpu_index >= ms->smp.max_cpus);
}
-static inline int64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
+static uint64_t dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
{
static uint64_t max_dirtyrate;
-uint32_t dirty_ring_size = kvm_dirty_ring_size();
-uint64_t dirty_ring_size_meory_MB =
-dirty_ring_size * qemu_target_page_size() >> 20;
+unsigned target_page_bits = qemu_target_page_bits();
+uint64_t dirty_ring_size_MB;
+
+/* So far, the largest (non-huge) page size is 64k, i.e. 16 bits. */
+assert(target_page_bits < 20);
+
+/* Convert ring size (pages) to MiB (2**20). */
+dirty_ring_size_MB = kvm_dirty_ring_size() >> (20 - target_page_bits);
if (max_dirtyrate < dirtyrate) {
max_dirtyrate = dirtyrate;
}
-return dirty_ring_size_meory_MB * 100 / max_dirtyrate;
+return dirty_ring_size_MB * 100 / max_dirtyrate;
}
Reviewed-by: Thomas Huth
[PATCH 10/17] tests/qapi-schema/doc-good: Improve argument description tests
Improve the comments to better describe what they test.
Cover argument description starting on a new line indented. This
style isn't documented in docs/devel/qapi-code-gen.rst. qapi-gen.py
accepts it, but messes up indentation: it's stripped from the first
line, not subsequent ones. The next commit will fix this.
Signed-off-by: Markus Armbruster
---
tests/qapi-schema/doc-good.json | 15 +--
tests/qapi-schema/doc-good.out | 16 +---
2 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 445471daee..34c3dcbe97 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -54,7 +54,7 @@
##
# @Enum:
#
-# @one: The _one_ {and only}
+# @one: The _one_ {and only}, description on the same line
#
# Features:
# @enum-feat: Also _one_ {and only}
@@ -73,7 +73,8 @@
# @Base:
#
# @base1:
-# the first member
+# description starts on a new line,
+# not indented
##
{ 'struct': 'Base', 'data': { 'base1': 'Enum' },
'if': { 'all': ['IFALL1', 'IFALL2'] } }
@@ -120,7 +121,8 @@
##
# @Alternate:
#
-# @i: an integer
+# @i: description starts on the same line
+# remainder indented the same
# @b is undocumented
#
# Features:
@@ -138,10 +140,11 @@
##
# @cmd:
#
-# @arg1: the first argument
+# @arg1:
+# description starts on a new line,
+# indented
#
-# @arg2: the second
-#argument
+# @arg2: the second argument
#
# Features:
# @cmd-feat1: a feature
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index afa48dcd94..2ba72ae558 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -104,7 +104,7 @@ doc symbol=Enum
body=
arg=one
-The _one_ {and only}
+The _one_ {and only}, description on the same line
arg=two
feature=enum-feat
@@ -117,12 +117,13 @@ doc symbol=Base
body=
arg=base1
-the first member
+description starts on a new line,
+not indented
doc symbol=Variant1
body=
A paragraph
-Another paragraph (but no @var: line)
+Another paragraph
@var1 is undocumented
arg=var1
@@ -143,7 +144,8 @@ doc symbol=Alternate
body=
arg=i
-an integer
+description starts on the same line
+remainder indented the same
@b is undocumented
arg=b
@@ -156,10 +158,10 @@ doc symbol=cmd
body=
arg=arg1
-the first argument
+description starts on a new line,
+indented
arg=arg2
-the second
-argument
+the second argument
arg=arg3
feature=cmd-feat1
--
2.39.2
[PATCH 14/17] qapi: Section parameter @indent is no longer used, drop
Signed-off-by: Markus Armbruster
---
scripts/qapi/parser.py | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 3e29b7bf48..22ee631198 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -468,8 +468,7 @@ class QAPIDoc:
class Section:
# pylint: disable=too-few-public-methods
def __init__(self, parser: QAPISchemaParser,
- name: Optional[str] = None, indent: int = 0):
-
+ name: Optional[str] = None):
# parser, for error messages about indentation
self._parser = parser
# optional section name (argument/member or section name)
@@ -500,8 +499,8 @@ def append(self, line: str) -> None:
class ArgSection(Section):
def __init__(self, parser: QAPISchemaParser,
- name: str, indent: int = 0):
-super().__init__(parser, name, indent)
+ name: str):
+super().__init__(parser, name)
self.member: Optional['QAPISchemaMember'] = None
def connect(self, member: 'QAPISchemaMember') -> None:
@@ -626,7 +625,7 @@ def _append_args_line(self, line: str) -> None:
"""
if match := self._match_at_name_colon(line):
line = line[match.end():]
-self._start_args_section(match.group(1), 0)
+self._start_args_section(match.group(1))
elif self._match_section_tag(line):
self._append_line = self._append_various_line
self._append_various_line(line)
@@ -646,7 +645,7 @@ def _append_args_line(self, line: str) -> None:
def _append_features_line(self, line: str) -> None:
if match := self._match_at_name_colon(line):
line = line[match.end():]
-self._start_features_section(match.group(1), 0)
+self._start_features_section(match.group(1))
elif self._match_section_tag(line):
self._append_line = self._append_various_line
self._append_various_line(line)
@@ -677,15 +676,14 @@ def _append_various_line(self, line: str) -> None:
% (match.group(1), self.sections[0].name))
if match := self._match_section_tag(line):
line = line[match.end():]
-self._start_section(match.group(1), 0)
+self._start_section(match.group(1))
self._append_freeform(line)
def _start_symbol_section(
self,
symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
-name: str,
-indent: int) -> None:
+name: str) -> None:
# FIXME invalid names other than the empty string aren't flagged
if not name:
raise QAPIParseError(self._parser, "invalid parameter name")
@@ -693,22 +691,21 @@ def _start_symbol_section(
raise QAPIParseError(self._parser,
"'%s' parameter name duplicated" % name)
assert not self.sections
-new_section = QAPIDoc.ArgSection(self._parser, name, indent)
+new_section = QAPIDoc.ArgSection(self._parser, name)
self._switch_section(new_section)
symbols_dict[name] = new_section
-def _start_args_section(self, name: str, indent: int) -> None:
-self._start_symbol_section(self.args, name, indent)
+def _start_args_section(self, name: str) -> None:
+self._start_symbol_section(self.args, name)
-def _start_features_section(self, name: str, indent: int) -> None:
-self._start_symbol_section(self.features, name, indent)
+def _start_features_section(self, name: str) -> None:
+self._start_symbol_section(self.features, name)
-def _start_section(self, name: Optional[str] = None,
- indent: int = 0) -> None:
+def _start_section(self, name: Optional[str] = None) -> None:
if name in ('Returns', 'Since') and self.has_section(name):
raise QAPIParseError(self._parser,
"duplicated '%s' section" % name)
-new_section = QAPIDoc.Section(self._parser, name, indent)
+new_section = QAPIDoc.Section(self._parser, name)
self._switch_section(new_section)
self.sections.append(new_section)
--
2.39.2
[PATCH 12/17] qapi: Rewrite parsing of doc comment section symbols and tags
To recognize a line starting with a section symbol and or tag, we
first split it at the first space, then examine the part left of the
space. We can just as well examine the unsplit line, so do that.
Signed-off-by: Markus Armbruster
---
scripts/qapi/parser.py | 51 +++---
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ddc14ceaba..fc04c4573e 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -560,12 +560,12 @@ def end_comment(self) -> None:
self._switch_section(QAPIDoc.NullSection(self._parser))
@staticmethod
-def _is_section_tag(name: str) -> bool:
-return name in ('Returns:', 'Since:',
-# those are often singular or plural
-'Note:', 'Notes:',
-'Example:', 'Examples:',
-'TODO:')
+def _match_at_name_colon(string: str) -> re.Match:
+return re.match(r'@([^:]*): *', string)
+
+@staticmethod
+def _match_section_tag(string: str) -> re.Match:
+return re.match(r'(Returns|Since|Notes?|Examples?|TODO): *', string)
def _append_body_line(self, line: str) -> None:
"""
@@ -581,7 +581,6 @@ def _append_body_line(self, line: str) -> None:
Else, append the line to the current section.
"""
-name = line.split(' ', 1)[0]
# FIXME not nice: things like '# @foo:' and '# @foo: ' aren't
# recognized, and get silently treated as ordinary text
if not self.symbol and not self.body.text and line.startswith('@'):
@@ -595,12 +594,12 @@ def _append_body_line(self, line: str) -> None:
self._parser, "name required after '@'")
elif self.symbol:
# This is a definition documentation block
-if name.startswith('@') and name.endswith(':'):
+if self._match_at_name_colon(line):
self._append_line = self._append_args_line
self._append_args_line(line)
elif line == 'Features:':
self._append_line = self._append_features_line
-elif self._is_section_tag(name):
+elif self._match_section_tag(line):
self._append_line = self._append_various_line
self._append_various_line(line)
else:
@@ -621,16 +620,15 @@ def _append_args_line(self, line: str) -> None:
Else, append the line to the current section.
"""
-name = line.split(' ', 1)[0]
-
-if name.startswith('@') and name.endswith(':'):
+if match := self._match_at_name_colon(line):
# If line is "@arg: first line of description", find
# the index of 'f', which is the indent we expect for any
# following lines. We then remove the leading "@arg:"
# from line and replace it with spaces so that 'f' has the
# same index as it did in the original line and can be
# handled the same way we will handle following lines.
-indent = must_match(r'@\S*:\s*', line).end()
+name = match.group(1)
+indent = match.end()
line = line[indent:]
if not line:
# Line was just the "@arg:" header
@@ -638,8 +636,8 @@ def _append_args_line(self, line: str) -> None:
indent = -1
else:
line = ' ' * indent + line
-self._start_args_section(name[1:-1], indent)
-elif self._is_section_tag(name):
+self._start_args_section(name, indent)
+elif self._match_section_tag(line):
self._append_line = self._append_various_line
self._append_various_line(line)
return
@@ -656,16 +654,15 @@ def _append_args_line(self, line: str) -> None:
self._append_freeform(line)
def _append_features_line(self, line: str) -> None:
-name = line.split(' ', 1)[0]
-
-if name.startswith('@') and name.endswith(':'):
+if match := self._match_at_name_colon(line):
# If line is "@arg: first line of description", find
# the index of 'f', which is the indent we expect for any
# following lines. We then remove the leading "@arg:"
# from line and replace it with spaces so that 'f' has the
# same index as it did in the original line and can be
# handled the same way we will handle following lines.
-indent = must_match(r'@\S*:\s*', line).end()
+name = match.group(1)
+indent = match.end()
line = line[indent:]
if not line:
# Line was just the "@arg:" header
@@ -673,8 +670,8 @@ def _append_features_line(self, line: str) -> None:
indent = -1
else:
line = ' ' * indent + line
-self._sta
Re: [PATCH 00/17] qapi: Reformat doc comments
Forgot to mention: it's based on my "[PULL 00/17] QAPI patches patches for 2023-04-28". Apologies!
[PATCH] test-aio-multithread: do not use mb_read/mb_set for simple flags
The remaining use of mb_read/mb_set is just to force a thread to exit
eventually. It does not order two memory accesses and therefore can be
just read/set.
Signed-off-by: Paolo Bonzini
---
tests/unit/test-aio-multithread.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tests/unit/test-aio-multithread.c
b/tests/unit/test-aio-multithread.c
index a555cc883505..3c61526a0b46 100644
--- a/tests/unit/test-aio-multithread.c
+++ b/tests/unit/test-aio-multithread.c
@@ -202,7 +202,7 @@ static CoMutex comutex;
static void coroutine_fn test_multi_co_mutex_entry(void *opaque)
{
-while (!qatomic_mb_read(&now_stopping)) {
+while (!qatomic_read(&now_stopping)) {
qemu_co_mutex_lock(&comutex);
counter++;
qemu_co_mutex_unlock(&comutex);
@@ -236,7 +236,7 @@ static void test_multi_co_mutex(int threads, int seconds)
g_usleep(seconds * 100);
-qatomic_mb_set(&now_stopping, true);
+qatomic_set(&now_stopping, true);
while (running > 0) {
g_usleep(10);
}
@@ -327,7 +327,7 @@ static void mcs_mutex_unlock(void)
static void test_multi_fair_mutex_entry(void *opaque)
{
-while (!qatomic_mb_read(&now_stopping)) {
+while (!qatomic_read(&now_stopping)) {
mcs_mutex_lock();
counter++;
mcs_mutex_unlock();
@@ -355,7 +355,7 @@ static void test_multi_fair_mutex(int threads, int seconds)
g_usleep(seconds * 100);
-qatomic_mb_set(&now_stopping, true);
+qatomic_set(&now_stopping, true);
while (running > 0) {
g_usleep(10);
}
@@ -383,7 +383,7 @@ static QemuMutex mutex;
static void test_multi_mutex_entry(void *opaque)
{
-while (!qatomic_mb_read(&now_stopping)) {
+while (!qatomic_read(&now_stopping)) {
qemu_mutex_lock(&mutex);
counter++;
qemu_mutex_unlock(&mutex);
@@ -411,7 +411,7 @@ static void test_multi_mutex(int threads, int seconds)
g_usleep(seconds * 100);
-qatomic_mb_set(&now_stopping, true);
+qatomic_set(&now_stopping, true);
while (running > 0) {
g_usleep(10);
}
--
2.40.0
[PATCH v6 0/1] util/async-teardown: appear in query-command-line-options
Add new -run-with option with an async-teardown=on|off parameter. It is visible in the output of query-command-line-options QMP command, so it can be discovered and used by libvirt. The option -async-teardown is now redundant, deprecate it. v5->v6 * deprecate the old -async-teardown option instead of removing it, since it has now appeared in 2 QEMU releases * use -run-with as a grab bag commandline option for the async-teardown boolean parameter [paolo,markus,thomas] v4->v5 * reword commit message [Markus] * document the removal of the -async-teardown commandline option in docs/about/removed-features.rst [Markus] v3->v4 * completely remove the useless -async-teardown option, since it was not wired up properly and it had no users [thomas] * QEMU should be always uppercase in text and documentation [thomas] * if the new -teardown option fails to parse, exit immediately instead of returning an error [thomas] v2->v3 * add a new teardown option with an async parameter [Markus] * reworded documentation of existing -async-teardown option so that it points to the new teardown option v1->v2 * remove the unneeded .implied_opt_name initializer [Thomas] Claudio Imbrenda (1): util/async-teardown: wire up query-command-line-options docs/about/deprecated.rst | 5 + os-posix.c| 15 +++ qemu-options.hx | 34 +++--- util/async-teardown.c | 21 + 4 files changed, 64 insertions(+), 11 deletions(-) -- 2.40.0
[PATCH v6 1/1] util/async-teardown: wire up query-command-line-options
Add new -run-with option with an async-teardown=on|off parameter. It is
visible in the output of query-command-line-options QMP command, so it
can be discovered and used by libvirt.
The option -async-teardown is now redundant, deprecate it.
Reported-by: Boris Fiuczynski
Fixes: c891c24b1a ("os-posix: asynchronous teardown for shutdown on Linux")
Signed-off-by: Claudio Imbrenda
---
docs/about/deprecated.rst | 5 +
os-posix.c| 15 +++
qemu-options.hx | 34 +++---
util/async-teardown.c | 21 +
4 files changed, 64 insertions(+), 11 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 1ca9dc33d6..0986db9a86 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -111,6 +111,11 @@ Use ``-machine acpi=off`` instead.
The HAXM project has been retired (see https://github.com/intel/haxm#status).
Use "whpx" (on Windows) or "hvf" (on macOS) instead.
+``-async-teardown`` (since 8.1)
+,,,
+
+Use ``-run-with async-teardown=on`` instead.
+
QEMU Machine Protocol (QMP) commands
diff --git a/os-posix.c b/os-posix.c
index 5adc69f560..117ad2bdc1 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -36,6 +36,8 @@
#include "qemu/log.h"
#include "sysemu/runstate.h"
#include "qemu/cutils.h"
+#include "qemu/config-file.h"
+#include "qemu/option.h"
#ifdef CONFIG_LINUX
#include
@@ -132,6 +134,8 @@ static bool os_parse_runas_uid_gid(const char *optarg)
*/
int os_parse_cmd_args(int index, const char *optarg)
{
+QemuOpts *opts;
+
switch (index) {
case QEMU_OPTION_runas:
user_pwd = getpwnam(optarg);
@@ -152,9 +156,20 @@ int os_parse_cmd_args(int index, const char *optarg)
daemonize = 1;
break;
#if defined(CONFIG_LINUX)
+/* deprecated */
case QEMU_OPTION_asyncteardown:
init_async_teardown();
break;
+case QEMU_OPTION_run_with:
+opts = qemu_opts_parse_noisily(qemu_find_opts("run-with"),
+ optarg, false);
+if (!opts) {
+exit(1);
+}
+if (qemu_opt_get_bool(opts, "async-teardown", false)) {
+init_async_teardown();
+}
+break;
#endif
default:
return -1;
diff --git a/qemu-options.hx b/qemu-options.hx
index b5efa648ba..24a1d63bbe 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4799,20 +4799,32 @@ DEF("qtest-log", HAS_ARG, QEMU_OPTION_qtest_log, "",
QEMU_ARCH_ALL)
DEF("async-teardown", 0, QEMU_OPTION_asyncteardown,
"-async-teardown enable asynchronous teardown\n",
QEMU_ARCH_ALL)
-#endif
SRST
``-async-teardown``
-Enable asynchronous teardown. A new process called "cleanup/"
-will be created at startup sharing the address space with the main qemu
-process, using clone. It will wait for the main qemu process to
-terminate completely, and then exit.
-This allows qemu to terminate very quickly even if the guest was
-huge, leaving the teardown of the address space to the cleanup
-process. Since the cleanup process shares the same cgroups as the
-main qemu process, accounting is performed correctly. This only
-works if the cleanup process is not forcefully killed with SIGKILL
-before the main qemu process has terminated completely.
+This option is deprecated and should no longer be used. The new option
+``-run-with async-teardown=on`` is a replacement.
ERST
+DEF("run-with", HAS_ARG, QEMU_OPTION_run_with,
+"-run-with async-teardown[=on|off]\n"
+"misc QEMU process lifecycle options\n"
+"async-teardown=on enables asynchronous teardown\n",
+QEMU_ARCH_ALL)
+SRST
+``-run-with``
+Set QEMU process lifecycle options.
+
+``async-teardown=on`` enables asynchronous teardown. A new process called
+"cleanup/" will be created at startup sharing the address
+space with the main QEMU process, using clone. It will wait for the
+main QEMU process to terminate completely, and then exit. This allows
+QEMU to terminate very quickly even if the guest was huge, leaving the
+teardown of the address space to the cleanup process. Since the cleanup
+process shares the same cgroups as the main QEMU process, accounting is
+performed correctly. This only works if the cleanup process is not
+forcefully killed with SIGKILL before the main QEMU process has
+terminated completely.
+ERST
+#endif
DEF("msg", HAS_ARG, QEMU_OPTION_msg,
"-msg [timestamp[=on|off]][,guest-name=[on|off]]\n"
diff --git a/util/async-teardown.c b/util/async-teardown.c
index 62cdeb0f20..3ab19c8740 100644
--- a/util/async-teardown.c
+++ b/util/async-teardown.c
@@ -12,6 +12,9 @@
*/
#include "qemu/osdep.h"
+#include "qemu/config-file.h"
+#include "qemu/option.h"
+#include "qemu/module.h"
#include
#include
#include
@@ -144,3 +147
