Low-hanging fruit, and except for StatsFilter, the only members of
these unions lacking documentation.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-16-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qapi: Move @String out of common.json to discourage reuse
Use of String is problematic, because it results in awkward interface
documentation. The previous commit cleaned up one instance.
Move String out of common.json next to its remaining users in net.json
to discourage reuse elsewhere.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-15-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qapi: Improve documentation of file descriptor socket addresses
SocketAddress branch @fd is documented in enum SocketAddressType,
unlike the other branches. That's because the branch's type is String
from common.json.
Use a local copy of String, so we can put the documentation in the
usual place.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-14-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qapi: Plug trivial documentation holes around former simple unions
The conversion of simple to flat unions left the @data members
undocumented. Add documentation where it's trivial. Copy verbatim
from the wrapped type's description where possible.
Leftovers: String (to be taken care of in the next commit), and
TransActionAction (left for another day).
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-13-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qapi/dump: Clean up documentation of DumpGuestMemoryCapability
The type's doc comment describes its member, but it's not marked up as
such. Easy enough to fix.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-12-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The command's doc comment describes the argument, but it's not marked
up as such. Easy enough to fix.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-11-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Add missing return member documentation of guest-get-disks,
guest-get-devices, guest-get-diskstats, and guest-get-cpustats.
The NVMe SMART information returned by guest-getdisks remains
undocumented. Add a TODO there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-10-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qga/qapi-schema: Clean up documentation of guest-set-vcpus
The command's doc comment describes the argument, but it's not marked
up as such. Easy enough to fix.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-9-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qga/qapi-schema: Clean up documentation of guest-set-memory-blocks
The command's doc comment describes the argument, but it's not marked
up as such. Easy enough to fix.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-8-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
qapi: Require member documentation (with loophole)
The QAPI generator forces you to document your stuff. Except for
command arguments, event data, and members of enum and object types:
these the generator silently "documents" as "Not documented".
We can't require proper documentation there without first fixing all
the offenders. We've always had too many offenders to pull that off.
Right now, we have more than 500. Worse, we seem to fix old ones no
faster than we add new ones: in the past year, we fixed 22 ones, but
added 26 new ones.
To help arrest the backsliding, make missing documentation an error
unless the command, type, or event is in listed in new pragma
documentation-exceptions.
List all the current offenders: 117 commands and types in qapi/, and 9
in qga/.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-7-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
sphinx/qapidoc: Drop code to generate doc for simple union tag
QAPISchemaGenRSTVisitor._nodes_for_members() has a special case to
auto-generate documentation for a union tag member of implicit (enum)
type that lacks documentation.
This was useful for simple unions, where the tag member's type was
implicitly. The only implicit enum type left today is 'QType'. Not
worth a special case. Drop. No change to generated documentation.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-6-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
docs/devel/qapi-code-gen demands that the "second and subsequent lines
of sections other than "Example"/"Examples" should be indented".
Commit a937b6aa739q (qapi: Reformat doc comments to conform to current
conventions) missed a few instances, and messed up a few others.
Clean that up.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-5-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The description of @bins ends with a literal block:
# @bins: list of io request counts corresponding to histogram
# intervals, one more element than @boundaries has. For the
# example above, @bins may be something like [3, 1, 5, 2], and
# corresponding histogram looks like:
#
# ::
#
# 5| *
Except it actually ends *before* the block: the unindented '::' line
starts a new section. Makes no sense.
We could fix this by indenting the '::' line. Instead, double the
colon at the end of the preceding paragraph, and drop the '::' line.
This shifts the box for the literal block right in generated
documentation, so it lines up with the description.
Fixes: commit a0fcff383b34 (qapi: Use rST markup for literal blocks) Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-4-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Missed in commit a937b6aa739 (qapi: Reformat doc comments to conform
to current conventions).
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-3-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
docs/devel/qapi-code-gen: Normalize version refs x.y.0 to just x.y
Missed in commit 9bc6e893b72 (qapi: Normalize version references x.y.0
to just x.y).
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-ID: <20240205074709.3613229-2-armbru@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Peter Maydell [Fri, 9 Feb 2024 11:22:20 +0000 (11:22 +0000)]
Merge tag 'migration-staging-pull-request' of https://gitlab.com/peterx/qemu into staging
Migration pull
- William's fix on hwpoison migration which used to crash QEMU
- Peter's multifd cleanup + bugfix + optimizations
- Avihai's fix on multifd crash over non-socket channels
- Fabiano's multifd thread-race fix
- Peter's CI fix series
# -----BEGIN PGP SIGNATURE-----
#
# iIgEABYKADAWIQS5GE3CDMRX2s990ak7X8zN86vXBgUCZcREtRIccGV0ZXJ4QHJl
# ZGhhdC5jb20ACgkQO1/MzfOr1wacrwEAl2aeQkh51h/e+OKX7MG4/4Y6Edf6Oz7o
# IJLk/cyrUFQA/2exo2lOdv5zHNOJKwAYj8HYDraezrC/MK1eED4Wji0M
# =k53l
# -----END PGP SIGNATURE-----
# gpg: Signature made Thu 08 Feb 2024 03:04:21 GMT
# gpg: using EDDSA key B9184DC20CC457DACF7DD1A93B5FCCCDF3ABD706
# gpg: issuer "peterx@redhat.com"
# gpg: Good signature from "Peter Xu <xzpeter@gmail.com>" [marginal]
# gpg: aka "Peter Xu <peterx@redhat.com>" [marginal]
# gpg: WARNING: This key is not certified with sufficiently trusted signatures!
# gpg: It is not certain that the signature belongs to the owner.
# Primary key fingerprint: B918 4DC2 0CC4 57DA CF7D D1A9 3B5F CCCD F3AB D706
* tag 'migration-staging-pull-request' of https://gitlab.com/peterx/qemu: (34 commits)
ci: Update comment for migration-compat-aarch64
ci: Remove tag dependency for build-previous-qemu
tests/migration-test: Stick with gicv3 in aarch64 test
migration/multifd: Add a synchronization point for channel creation
migration/multifd: Unify multifd and TLS connection paths
migration/multifd: Move multifd_send_setup into migration thread
migration/multifd: Move multifd_send_setup error handling in to the function
migration/multifd: Remove p->running
migration/multifd: Join the TLS thread
migration: Fix logic of channels and transport compatibility check
migration/multifd: Optimize sender side to be lockless
migration/multifd: Fix MultiFDSendParams.packet_num race
migration/multifd: Stick with send/recv on function names
migration/multifd: Cleanup multifd_load_cleanup()
migration/multifd: Cleanup multifd_save_cleanup()
migration/multifd: Rewrite multifd_queue_page()
migration/multifd: Change retval of multifd_send_pages()
migration/multifd: Change retval of multifd_queue_page()
migration/multifd: Split multifd_send_terminate_threads()
migration/multifd: Forbid spurious wakeups
...
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
* tag 'for-upstream' of https://repo.or.cz/qemu/kevin:
virtio-blk: avoid using ioeventfd state in irqfd conditional
virtio-blk: Use ioeventfd_attach in start_ioeventfd
virtio: Re-enable notifications after drain
virtio-scsi: Attach event vq notifier with no_poll
blkio: Respect memory-alignment for bounce buffer allocations
scsi: Don't ignore most usb-storage properties
virtio-blk: do not use C99 mixed declarations
iotests: give tempdir an identifying name
iotests: fix leak of tmpdir in dry-run mode
scsi: Await request purging
block-backend: Allow concurrent context changes
monitor: use aio_co_reschedule_self()
virtio-blk: declare VirtIOBlock::rq with a type
virtio-blk: add vq_rq[] bounds check in virtio_blk_dma_restart_cb()
virtio-blk: clarify that there is at least 1 virtqueue
virtio-blk: enforce iothread-vq-mapping validation
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Peter Maydell [Thu, 8 Feb 2024 16:08:42 +0000 (16:08 +0000)]
Merge tag 'pull-tcg-20240205-2' of https://gitlab.com/rth7680/qemu into staging
tcg: Introduce TCG_COND_TST{EQ,NE}
target/alpha: Use TCG_COND_TST{EQ,NE}
target/m68k: Use TCG_COND_TST{EQ,NE} in gen_fcc_cond
target/sparc: Use TCG_COND_TSTEQ in gen_op_mulscc
target/s390x: Use TCG_COND_TSTNE for CC_OP_{TM,ICM}
target/s390x: Improve general case of disas_jcc
* tag 'pull-tcg-20240205-2' of https://gitlab.com/rth7680/qemu: (39 commits)
tcg/tci: Support TCG_COND_TST{EQ,NE}
tcg/s390x: Support TCG_COND_TST{EQ,NE}
tcg/s390x: Add TCG_CT_CONST_CMP
tcg/s390x: Split constraint A into J+U
tcg/ppc: Support TCG_COND_TST{EQ,NE}
tcg/ppc: Add TCG_CT_CONST_CMP
tcg/ppc: Tidy up tcg_target_const_match
tcg/ppc: Use cr0 in tcg_to_bc and tcg_to_isel
tcg/ppc: Sink tcg_to_bc usage into tcg_out_bc
tcg/sparc64: Support TCG_COND_TST{EQ,NE}
tcg/sparc64: Pass TCGCond to tcg_out_cmp
tcg/sparc64: Hoist read of tcg_cond_to_rcond
tcg/i386: Use TEST r,r to test 8/16/32 bits
tcg/i386: Improve TSTNE/TESTEQ vs powers of two
tcg/i386: Support TCG_COND_TST{EQ,NE}
tcg/i386: Move tcg_cond_to_jcc[] into tcg_out_cmp
tcg/i386: Pass x86 condition codes to tcg_out_cmov
tcg/arm: Support TCG_COND_TST{EQ,NE}
tcg/arm: Split out tcg_out_cmp()
tcg/aarch64: Generate CBNZ for TSTNE of UINT32_MAX
...
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Peter Maydell [Thu, 8 Feb 2024 11:59:28 +0000 (11:59 +0000)]
Merge tag 'pull-request-2024-02-06' of https://gitlab.com/thuth/qemu into staging
* Emulate CVB, CVBY, CVBG and CVDG s390x instructions
* Fix bug in lsi53c895a reentrancy counter
* Deprecate the "power5+" and "power7+" CPU names
* Fix problems in the freebsd VM test
* tag 'pull-request-2024-02-06' of https://gitlab.com/thuth/qemu:
meson: Link with libinotify on FreeBSD
test-util-filemonitor: Adapt to the FreeBSD inotify rename semantics
tests/vm/freebsd: Reload the sshd configuration
tests/vm: Set UseDNS=no in the sshd configuration
target/s390x: Prefer fast cpu_env() over slower CPU QOM cast macro
tests/tcg/s390x: Test CONVERT TO BINARY
tests/tcg/s390x: Test CONVERT TO DECIMAL
target/s390x: Emulate CVB, CVBY and CVBG
target/s390x: Emulate CVDG
docs/about: Deprecate the old "power5+" and "power7+" CPU names
target/ppc/cpu-models: Rename power5+ and power7+ for new QOM naming rules
hw/scsi/lsi53c895a: add missing decrement of reentrancy counter
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Peter Maydell [Thu, 8 Feb 2024 11:59:13 +0000 (11:59 +0000)]
Merge tag 'mem-2024-02-06-v3' of https://github.com/davidhildenbrand/qemu into staging
Hi,
"Host Memory Backends" and "Memory devices" queue ("mem"):
- Reintroduce memory region size checks for memory devices; the removal
lead to some undesired side effects
- Preallocate memory of memory backends in selected configurations
asynchronously (so we preallocate concurrently), to speed up QEMU
startup time.
* tag 'mem-2024-02-06-v3' of https://github.com/davidhildenbrand/qemu:
oslib-posix: initialize backend memory objects in parallel
memory-device: reintroduce memory region size check
hv-balloon: use get_min_alignment() to express 32 GiB alignment
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Stefan Hajnoczi [Mon, 22 Jan 2024 17:26:25 +0000 (12:26 -0500)]
virtio-blk: avoid using ioeventfd state in irqfd conditional
Requests that complete in an IOThread use irqfd to notify the guest
while requests that complete in the main loop thread use the traditional
qdev irq code path. The reason for this conditional is that the irq code
path requires the BQL:
There is a corner case where the conditional invokes the irq code path
instead of the irqfd code path:
static void virtio_blk_stop_ioeventfd(VirtIODevice *vdev)
{
...
/*
* Set ->ioeventfd_started to false before draining so that host notifiers
* are not detached/attached anymore.
*/
s->ioeventfd_started = false;
/* Wait for virtio_blk_dma_restart_bh() and in flight I/O to complete */
blk_drain(s->conf.conf.blk);
During blk_drain() the conditional produces the wrong result because
ioeventfd_started is false.
Use qemu_in_iothread() instead of checking the ioeventfd state.
Cc: qemu-stable@nongnu.org Buglink: https://issues.redhat.com/browse/RHEL-15394 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240122172625.415386-1-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek [Fri, 2 Feb 2024 15:31:58 +0000 (16:31 +0100)]
virtio-blk: Use ioeventfd_attach in start_ioeventfd
Commit d3f6f294aeadd5f88caf0155e4360808c95b3146 ("virtio-blk: always set
ioeventfd during startup") has made virtio_blk_start_ioeventfd() always
kick the virtqueue (set the ioeventfd), regardless of whether the BB is
drained. That is no longer necessary, because attaching the host
notifier will now set the ioeventfd, too; this happens either
immediately right here in virtio_blk_start_ioeventfd(), or later when
the drain ends, in virtio_blk_ioeventfd_attach().
With event_notifier_set() removed, the code becomes the same as the one
in virtio_blk_ioeventfd_attach(), so we can reuse that function.
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240202153158.788922-4-hreitz@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek [Fri, 2 Feb 2024 15:31:57 +0000 (16:31 +0100)]
virtio: Re-enable notifications after drain
During drain, we do not care about virtqueue notifications, which is why
we remove the handlers on it. When removing those handlers, whether vq
notifications are enabled or not depends on whether we were in polling
mode or not; if not, they are enabled (by default); if so, they have
been disabled by the io_poll_start callback.
Because we do not care about those notifications after removing the
handlers, this is fine. However, we have to explicitly ensure they are
enabled when re-attaching the handlers, so we will resume receiving
notifications. We do this in virtio_queue_aio_attach_host_notifier*().
If such a function is called while we are in a polling section,
attaching the notifiers will then invoke the io_poll_start callback,
re-disabling notifications.
Because we will always miss virtqueue updates in the drained section, we
also need to poll the virtqueue once after attaching the notifiers.
Buglink: https://issues.redhat.com/browse/RHEL-3934 Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240202153158.788922-3-hreitz@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek [Fri, 2 Feb 2024 15:31:56 +0000 (16:31 +0100)]
virtio-scsi: Attach event vq notifier with no_poll
As of commit 38738f7dbbda90fbc161757b7f4be35b52205552 ("virtio-scsi:
don't waste CPU polling the event virtqueue"), we only attach an io_read
notifier for the virtio-scsi event virtqueue instead, and no polling
notifiers. During operation, the event virtqueue is typically
non-empty, but none of the buffers are intended to be used immediately.
Instead, they only get used when certain events occur. Therefore, it
makes no sense to continuously poll it when non-empty, because it is
supposed to be and stay non-empty.
We do this by using virtio_queue_aio_attach_host_notifier_no_poll()
instead of virtio_queue_aio_attach_host_notifier() for the event
virtqueue.
Commit 766aa2de0f29b657148e04599320d771c36fd126 ("virtio-scsi: implement
BlockDevOps->drained_begin()") however has virtio_scsi_drained_end() use
virtio_queue_aio_attach_host_notifier() for all virtqueues, including
the event virtqueue. This can lead to it being polled again, undoing
the benefit of commit 38738f7dbbda90fbc161757b7f4be35b52205552.
Fix it by using virtio_queue_aio_attach_host_notifier_no_poll() for the
event virtqueue.
Reported-by: Fiona Ebner <f.ebner@proxmox.com> Fixes: 766aa2de0f29b657148e04599320d771c36fd126
("virtio-scsi: implement BlockDevOps->drained_begin()") Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Tested-by: Fiona Ebner <f.ebner@proxmox.com> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240202153158.788922-2-hreitz@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Kevin Wolf [Wed, 31 Jan 2024 17:31:40 +0000 (18:31 +0100)]
blkio: Respect memory-alignment for bounce buffer allocations
blkio_alloc_mem_region() requires that the requested buffer size is a
multiple of the memory-alignment property. If it isn't, the allocation
fails with a return value of -EINVAL.
Fix the call in blkio_resize_bounce_pool() to make sure the requested
size is properly aligned.
I observed this problem with vhost-vdpa, which requires page aligned
memory. As the virtio-blk device behind it still had 512 byte blocks, we
got bs->bl.request_alignment = 512, but actually any request that needed
a bounce buffer and was not aligned to 4k would fail without this fix.
Suggested-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20240131173140.42398-1-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Kevin Wolf [Wed, 31 Jan 2024 13:06:07 +0000 (14:06 +0100)]
scsi: Don't ignore most usb-storage properties
usb-storage is for the most part just a wrapper around an internally
created scsi-disk device. It uses DEFINE_BLOCK_PROPERTIES() to offer all
of the usual block device properties to the user, but then only forwards
a few select properties to the internal device while the rest is
silently ignored.
This changes scsi_bus_legacy_add_drive() to accept a whole BlockConf
instead of some individual values inside of it so that usb-storage can
now pass the whole configuration to the internal scsi-disk. This enables
the remaining block device properties, e.g. logical/physical_block_size
or discard_granularity.
Stefan Hajnoczi [Tue, 6 Feb 2024 14:04:09 +0000 (09:04 -0500)]
virtio-blk: do not use C99 mixed declarations
QEMU's coding style generally forbids C99 mixed declarations.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240206140410.65650-1-stefanha@redhat.com> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
If something goes wrong causing the iotests not to cleanup their
temporary directory, it is useful if the dir had an identifying
name to show what is to blame.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-ID: <20240205155158.1843304-1-berrange@redhat.com> Revieved-by: Michael Tokarev <mjt@tls.msk.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Creating an instance of the 'TestEnv' class will create a temporary
directory. This dir is only deleted, however, in the __exit__ handler
invoked by a context manager.
In dry-run mode, we don't use the TestEnv via a context manager, so
were leaking the temporary directory. Since meson invokes 'check'
5 times on each configure run, developers /tmp was filling up with
empty temporary directories.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-ID: <20240205154019.1841037-1-berrange@redhat.com> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek [Fri, 2 Feb 2024 14:47:55 +0000 (15:47 +0100)]
scsi: Await request purging
scsi_device_for_each_req_async() currently does not provide any way to
be awaited. One of its callers is scsi_device_purge_requests(), which
therefore currently does not guarantee that all requests are fully
settled when it returns.
We want all requests to be settled, because scsi_device_purge_requests()
is called through the unrealize path, including the one invoked by
virtio_scsi_hotunplug() through qdev_simple_device_unplug_cb(), which
most likely assumes that all SCSI requests are done then.
In fact, scsi_device_purge_requests() already contains a blk_drain(),
but this will not fully await scsi_device_for_each_req_async(), only the
I/O requests it potentially cancels (not the non-I/O requests).
However, we can have scsi_device_for_each_req_async() increment the BB
in-flight counter, and have scsi_device_for_each_req_async_bh()
decrement it when it is done. This way, the blk_drain() will fully
await all SCSI requests to be purged.
This also removes the need for scsi_device_for_each_req_async_bh() to
double-check the current context and potentially re-schedule itself,
should it now differ from the BB's context: Changing a BB's AioContext
with a root node is done through bdrv_try_change_aio_context(), which
creates a drained section. With this patch, we keep the BB in-flight
counter elevated throughout, so we know the BB's context cannot change.
Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240202144755.671354-3-hreitz@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek [Fri, 2 Feb 2024 14:47:54 +0000 (15:47 +0100)]
block-backend: Allow concurrent context changes
Since AioContext locks have been removed, a BlockBackend's AioContext
may really change at any time (only exception is that it is often
confined to a drained section, as noted in this patch). Therefore,
blk_get_aio_context() cannot rely on its root node's context always
matching that of the BlockBackend.
In practice, whether they match does not matter anymore anyway: Requests
can be sent to BDSs from any context, so anyone who requests the BB's
context should have no reason to require the root node to have the same
context. Therefore, we can and should remove the assertion to that
effect.
In addition, because the context can be set and queried from different
threads concurrently, it has to be accessed with atomic operations.
Buglink: https://issues.redhat.com/browse/RHEL-19381 Suggested-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240202144755.671354-2-hreitz@redhat.com> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
The code is actually safe because the iohandler and qemu_aio_context
AioContext run under the Big QEMU Lock. Nevertheless, set a good example
and use aio_co_reschedule_self() so it's obvious that there is no race.
Suggested-by: Hanna Reitz <hreitz@redhat.com> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240206190610.107963-6-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Acked-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Stefan Hajnoczi [Tue, 6 Feb 2024 19:06:09 +0000 (14:06 -0500)]
virtio-blk: declare VirtIOBlock::rq with a type
The VirtIOBlock::rq field has had the type void * since its introduction
in commit 869a5c6df19a ("Stop VM on error in virtio-blk. (Gleb
Natapov)").
Perhaps this was done to avoid the forward declaration of
VirtIOBlockReq.
Hanna Czenczek <hreitz@redhat.com> pointed out the missing type. Specify
the actual type because there is no need to use void * here.
Suggested-by: Hanna Czenczek <hreitz@redhat.com> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240206190610.107963-5-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
The code is correct because both rq->vq and vq_rq[] depend on
num_queues, but this is indirect and not 100% obvious. Add an assertion.
Suggested-by: Hanna Czenczek <hreitz@redhat.com> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240206190610.107963-4-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Stefan Hajnoczi [Tue, 6 Feb 2024 19:06:07 +0000 (14:06 -0500)]
virtio-blk: clarify that there is at least 1 virtqueue
It is not possible to instantiate a virtio-blk device with 0 virtqueues.
The following check is located in ->realize():
if (!conf->num_queues) {
error_setg(errp, "num-queues property must be larger than 0");
return;
}
Later on we access s->vq_aio_context[0] under the assumption that there
is as least one virtqueue. Hanna Czenczek <hreitz@redhat.com> noted that
it would help to show that the array index is already valid.
Add an assertion to document that s->vq_aio_context[0] is always
safe...and catch future code changes that break this assumption.
Suggested-by: Hanna Czenczek <hreitz@redhat.com> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240206190610.107963-3-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Hanna Czenczek <hreitz@redhat.com> noticed that the safety of
`vq_aio_context[vq->value] = ctx;` with user-defined vq->value inputs is
not obvious.
The code is structured in validate() + apply() steps so input validation
is there, but it happens way earlier and there is nothing that
guarantees apply() can only be called with validated inputs.
This patch moves the validate() call inside the apply() function so
validation is guaranteed. I also added the bounds checking assertion
that Hanna suggested.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Message-ID: <20240206190610.107963-2-stefanha@redhat.com> Reviewed-by: Kevin Wolf <kwolf@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Peter Xu [Wed, 7 Feb 2024 00:54:02 +0000 (08:54 +0800)]
ci: Remove tag dependency for build-previous-qemu
The new build-previous-qemu job relies on QEMU release tag being present,
while that may not be always true for personal git repositories since by
default tag is not pushed. The job can fail on those CI kicks, as reported
by Peter Maydell.
Fix it by fetching the tags remotely from the official repository, as
suggested by Dan.
Reported-by: Peter Maydell <peter.maydell@linaro.org> Suggested-by: "Daniel P. Berrangé" <berrange@redhat.com> Reviewed-by: "Daniel P. Berrangé" <berrange@redhat.com> Link: https://lore.kernel.org/r/20240207005403.242235-3-peterx@redhat.com Signed-off-by: Peter Xu <peterx@redhat.com>
Peter Xu [Wed, 7 Feb 2024 00:54:01 +0000 (08:54 +0800)]
tests/migration-test: Stick with gicv3 in aarch64 test
Recently we introduced cross-binary migration test. It's always wanted
that migration-test uses stable guest ABI for both QEMU binaries in this
case, so that both QEMU binaries will be compatible on the migration
stream with the cmdline specified.
Switch to a static gic version "3" rather than using version "max", so that
GIC should be stable now across any future QEMU binaries for migration-test.
Here the version can actually be anything as long as the ABI is stable. We
choose "3" because it's the majority of what we already use in QEMU while
still new enough: "git grep gic-version=3" shows 6 hit, while version 4 has
no direct user yet besides "max".
Note that even with this change, aarch64 won't be able to work yet with
migration cross binary test, but then the only missing piece will be the
stable CPU model.
Fabiano Rosas [Tue, 6 Feb 2024 21:51:18 +0000 (18:51 -0300)]
migration/multifd: Add a synchronization point for channel creation
It is possible that one of the multifd channels fails to be created at
multifd_new_send_channel_async() while the rest of the channel
creation tasks are still in flight.
This could lead to multifd_save_cleanup() executing the
qemu_thread_join() loop too early and not waiting for the threads
which haven't been created yet, leading to the freeing of resources
that the newly created threads will try to access and crash.
Add a synchronization point after which there will be no attempts at
thread creation and therefore calling multifd_save_cleanup() past that
point will ensure it properly waits for the threads.
A note about performance: Prior to this patch, if a channel took too
long to be established, other channels could finish connecting first
and already start taking load. Now we're bounded by the
slowest-connecting channel.
Fabiano Rosas [Tue, 6 Feb 2024 21:51:17 +0000 (18:51 -0300)]
migration/multifd: Unify multifd and TLS connection paths
During multifd channel creation (multifd_send_new_channel_async) when
TLS is enabled, the multifd_channel_connect function is called twice,
once to create the TLS handshake thread and another time after the
asynchrounous TLS handshake has finished.
This creates a slightly confusing call stack where
multifd_channel_connect() is called more times than the number of
channels. It also splits error handling between the two callers of
multifd_channel_connect() causing some code duplication. Lastly, it
gets in the way of having a single point to determine whether all
channel creation tasks have been initiated.
Refactor the code to move the reentrancy one level up at the
multifd_new_send_channel_async() level, de-duplicating the error
handling and allowing for the next patch to introduce a
synchronization point common to all the multifd channel creation,
regardless of TLS.
Note that the previous code would never fail once p->c had been set.
This patch changes this assumption, which affects refcounting, so add
comments around object_unref to explain the situation.
Fabiano Rosas [Tue, 6 Feb 2024 21:51:16 +0000 (18:51 -0300)]
migration/multifd: Move multifd_send_setup into migration thread
We currently have an unfavorable situation around multifd channels
creation and the migration thread execution.
We create the multifd channels with qio_channel_socket_connect_async
-> qio_task_run_in_thread, but only connect them at the
multifd_new_send_channel_async callback, called from
qio_task_complete, which is registered as a glib event.
So at multifd_send_setup() we create the channels, but they will only
be actually usable after the whole multifd_send_setup() calling stack
returns back to the main loop. Which means that the migration thread
is already up and running without any possibility for the multifd
channels to be ready on time.
We currently rely on the channels-ready semaphore blocking
multifd_send_sync_main() until channels start to come up and release
it. However there have been bugs recently found when a channel's
creation fails and multifd_send_cleanup() is allowed to run while
other channels are still being created.
Let's start to organize this situation by moving the
multifd_send_setup() call into the migration thread. That way we
unblock the main-loop to dispatch the completion callbacks and
actually have a chance of getting the multifd channels ready for when
the migration thread needs them.
The next patches will deal with the synchronization aspects.
Note that this takes multifd_send_setup() out of the BQL.
Fabiano Rosas [Tue, 6 Feb 2024 21:51:14 +0000 (18:51 -0300)]
migration/multifd: Remove p->running
We currently only need p->running to avoid calling qemu_thread_join()
on a non existent thread if the thread has never been created.
However, there are at least two bugs in this logic:
1) On the sending side, p->running is set too early and
qemu_thread_create() can be skipped due to an error during TLS
handshake, leaving the flag set and leading to a crash when
multifd_send_cleanup() calls qemu_thread_join().
2) During exit, the multifd thread clears the flag while holding the
channel lock. The counterpart at multifd_send_cleanup() reads the flag
outside of the lock and might free the mutex while the multifd thread
still has it locked.
Fix the first issue by setting the flag right before creating the
thread. Rename it from p->running to p->thread_created to clarify its
usage.
Fix the second issue by not clearing the flag at the multifd thread
exit. We don't have any use for that.
Note that these bugs are straight-forward logic issues and not race
conditions. There is still a gap for races to affect this code due to
multifd_send_cleanup() being allowed to run concurrently with the
thread creation loop. This issue is solved in the next patches.
Cc: qemu-stable <qemu-stable@nongnu.org> Fixes: 29647140157a ("migration/tls: add support for multifd tls-handshake") Reported-by: Avihai Horon <avihaih@nvidia.com> Reported-by: chenyuhui5@huawei.com Reviewed-by: Peter Xu <peterx@redhat.com> Signed-off-by: Fabiano Rosas <farosas@suse.de> Link: https://lore.kernel.org/r/20240206215118.6171-3-farosas@suse.de Signed-off-by: Peter Xu <peterx@redhat.com>
Avihai Horon [Thu, 25 Jan 2024 16:25:12 +0000 (18:25 +0200)]
migration: Fix logic of channels and transport compatibility check
The commit in the fixes line mistakenly modified the channels and
transport compatibility check logic so it now checks multi-channel
support only for socket transport type.
Thus, running multifd migration using a transport other than socket that
is incompatible with multi-channels (such as "exec") would lead to a
segmentation fault instead of an error message.
For example:
(qemu) migrate_set_capability multifd on
(qemu) migrate -d "exec:cat > /tmp/vm_state"
Segmentation fault (core dumped)
Fix it by checking multi-channel compatibility for all transport types.
Cc: qemu-stable <qemu-stable@nongnu.org> Fixes: d95533e1cdcc ("migration: modify migration_channels_and_uri_compatible() for new QAPI syntax") Signed-off-by: Avihai Horon <avihaih@nvidia.com> Reviewed-by: Peter Xu <peterx@redhat.com> Link: https://lore.kernel.org/r/20240125162528.7552-2-avihaih@nvidia.com Signed-off-by: Peter Xu <peterx@redhat.com>
After console_sshd_config(), the SSH server needs to be nudged to pick
up the new configs. The scripts for the other BSD flavors already do
this with a reboot, but a simple reload is sufficient.
Reviewed-by: Thomas Huth <thuth@redhat.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Message-ID: <20240206002344.12372-3-iii@linux.ibm.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
There is a 2s delay during connection, and ConnectTimeout is set to 1.
Raising it makes the issue go away, but we can do better. The server
truss shows:
target/s390x: Prefer fast cpu_env() over slower CPU QOM cast macro
Mechanical patch produced running the command documented
in scripts/coccinelle/cpu_env.cocci_template header.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240129164514.73104-25-philmd@linaro.org> Signed-off-by: Thomas Huth <thuth@redhat.com>
Convert to Binary - counterparts of the already implemented Convert
to Decimal (CVD*) instructions.
Example from the Principles of Operation: 25594C becomes 63FA.
Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20240205205830.6425-3-iii@linux.ibm.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
Mark Kanda [Wed, 31 Jan 2024 16:53:27 +0000 (10:53 -0600)]
oslib-posix: initialize backend memory objects in parallel
QEMU initializes preallocated backend memory as the objects are parsed from
the command line. This is not optimal in some cases (e.g. memory spanning
multiple NUMA nodes) because the memory objects are initialized in series.
Allow the initialization to occur in parallel (asynchronously). In order to
ensure optimal thread placement, asynchronous initialization requires prealloc
context threads to be in use.
Signed-off-by: Mark Kanda <mark.kanda@oracle.com>
Message-ID: <20240131165327.3154970-2-mark.kanda@oracle.com> Tested-by: Mario Casquero <mcasquer@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com>
memory-device: reintroduce memory region size check
We used to check that the memory region size is multiples of the overall
requested address alignment for the device memory address.
We removed that check, because there are cases (i.e., hv-balloon) where
devices unconditionally request an address alignment that has a very large
alignment (i.e., 32 GiB), but the actual memory device size might not be
multiples of that alignment.
However, this change:
(a) allows for some practically impossible DIMM sizes, like "1GB+1 byte".
(b) allows for DIMMs that partially cover hugetlb pages, previously
reported in [1].
Both scenarios don't make any sense: we might even waste memory.
So let's reintroduce that check, but only check that the
memory region size is multiples of the memory region alignment (i.e.,
page size, huge page size), but not any additional memory device
requirements communicated using md->get_min_alignment().
The following examples now fail again as expected:
(a) 1M with 2M THP
qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
-object memory-backend-ram,id=mem1,size=1M \
-device pc-dimm,id=dimm1,memdev=mem1
-> backend memory size must be multiple of 0x200000
(b) 1G+1byte
qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
-object memory-backend-ram,id=mem1,size=1073741825B \
-device pc-dimm,id=dimm1,memdev=mem1
-> backend memory size must be multiple of 0x200000
(c) Unliagned hugetlb size (2M)
qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
-object memory-backend-file,id=mem1,mem-path=/dev/hugepages/tmp,size=511M \
-device pc-dimm,id=dimm1,memdev=mem1
backend memory size must be multiple of 0x200000
(d) Unliagned hugetlb size (1G)
qemu-system-x86_64 -m 4g,maxmem=16g,slots=1 -S -nodefaults -nographic \
-object memory-backend-file,id=mem1,mem-path=/dev/hugepages1G/tmp,size=2047M \
-device pc-dimm,id=dimm1,memdev=mem1
-> backend memory size must be multiple of 0x40000000
Note that this fix depends on a hv-balloon change to communicate its
additional alignment requirements using get_min_alignment() instead of
through the memory region.
Peter Xu [Fri, 2 Feb 2024 10:28:57 +0000 (18:28 +0800)]
migration/multifd: Optimize sender side to be lockless
When reviewing my attempt to refactor send_prepare(), Fabiano suggested we
try out with dropping the mutex in multifd code [1].
I thought about that before but I never tried to change the code. Now
maybe it's time to give it a stab. This only optimizes the sender side.
The trick here is multifd has a clear provider/consumer model, that the
migration main thread publishes requests (either pending_job/pending_sync),
while the multifd sender threads are consumers. Here we don't have a lot
of complicated data sharing, and the jobs can logically be submitted
lockless.
Arm the code with atomic weapons. Two things worth mentioning:
- For multifd_send_pages(): we can use qatomic_load_acquire() when trying
to find a free channel, but that's expensive if we attach one ACQUIRE per
channel. Instead, keep the qatomic_read() on reading the pending_job
flag as we do already, meanwhile use one smp_mb_acquire() after the loop
to guarantee the memory ordering.
- For pending_sync: it doesn't have any extra data required since now
p->flags are never touched, it should be safe to not use memory barrier.
That's different from pending_job.
Provide rich comments for all the lockless operations to state how they are
paired. With that, we can remove the mutex.
Thomas Huth [Wed, 17 Jan 2024 14:10:54 +0000 (15:10 +0100)]
docs/about: Deprecate the old "power5+" and "power7+" CPU names
For consistency we should drop the names with a "+" in it in the
long run.
Message-ID: <20240117141054.73841-3-thuth@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
Thomas Huth [Wed, 17 Jan 2024 14:10:53 +0000 (15:10 +0100)]
target/ppc/cpu-models: Rename power5+ and power7+ for new QOM naming rules
The character "+" is now forbidden in QOM device names (see commit b447378e1217 - "Limit type names to alphanumerical and some few special
characters"). For the "power5+" and "power7+" CPU names, there is
currently a hack in type_name_is_valid() to still allow them for
compatibility reasons. However, there is a much nicer solution for this:
Simply use aliases! This way we can still support the old names without
the need for the ugly hack in type_name_is_valid().
Message-ID: <20240117141054.73841-2-thuth@redhat.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com> Signed-off-by: Thomas Huth <thuth@redhat.com>
Sven Schnelle [Sun, 28 Jan 2024 20:22:14 +0000 (21:22 +0100)]
hw/scsi/lsi53c895a: add missing decrement of reentrancy counter
When the maximum count of SCRIPTS instructions is reached, the code
stops execution and returns, but fails to decrement the reentrancy
counter. This effectively renders the SCSI controller unusable
because on next entry the reentrancy counter is still above the limit.
This bug was seen on HP-UX 10.20 which seems to trigger SCRIPTS
loops.
Fixes: b987718bbb ("hw/scsi/lsi53c895a: Fix reentrancy issues in the LSI controller (CVE-2023-0330)") Signed-off-by: Sven Schnelle <svens@stackframe.org>
Message-ID: <20240128202214.2644768-1-svens@stackframe.org> Reviewed-by: Thomas Huth <thuth@redhat.com> Tested-by: Helge Deller <deller@gmx.de> Signed-off-by: Thomas Huth <thuth@redhat.com>
As reported correctly by Fabiano [1] (while per Fabiano, it sourced back to
Elena's initial report in Oct 2023), MultiFDSendParams.packet_num is buggy
to be assigned and stored. Consider two consequent operations of: (1)
queue a job into multifd send thread X, then (2) queue another sync request
to the same send thread X. Then the MultiFDSendParams.packet_num will be
assigned twice, and the first assignment can get lost already.
To avoid that, we move the packet_num assignment from p->packet_num into
where the thread will fill in the packet. Use atomic operations to protect
the field, making sure there's no race.
Note that atomic fetch_add() may not be good for scaling purposes, however
multifd should be fine as number of threads should normally not go beyond
16 threads. Let's leave that concern for later but fix the issue first.
There's also a trick on how to make it always work even on 32 bit hosts for
uint64_t packet number. Switching to uintptr_t as of now to simply the
case. It will cause packet number to overflow easier on 32 bit, but that
shouldn't be a major concern for now as 32 bit systems is not the major
audience for any performance concerns like what multifd wants to address.
We also need to move multifd_send_state definition upper, so that
multifd_send_fill_packet() can reference it.
Peter Xu [Fri, 2 Feb 2024 10:28:53 +0000 (18:28 +0800)]
migration/multifd: Cleanup multifd_save_cleanup()
Shrink the function by moving relevant works into helpers: move the thread
join()s into multifd_send_terminate_threads(), then create two more helpers
to cover channel/state cleanups.
Add a TODO entry for the thread terminate process because p->running is
still buggy. We need to fix it at some point but not yet covered.
Peter Xu [Fri, 2 Feb 2024 10:28:52 +0000 (18:28 +0800)]
migration/multifd: Rewrite multifd_queue_page()
The current multifd_queue_page() is not easy to read and follow. It is not
good with a few reasons:
- No helper at all to show what exactly does a condition mean; in short,
readability is low.
- Rely on pages->ramblock being cleared to detect an empty queue. It's
slightly an overload of the ramblock pointer, per Fabiano [1], which I
also agree.
- Contains a self recursion, even if not necessary..
Rewrite this function. We add some comments to make it even clearer on
what it does.
Peter Xu [Fri, 2 Feb 2024 10:28:48 +0000 (18:28 +0800)]
migration/multifd: Forbid spurious wakeups
Now multifd's logic is designed to have no spurious wakeup. I still
remember a talk to Juan and he seems to agree we should drop it now, and if
my memory was right it was there because multifd used to hit that when
still debugging.
Let's drop it and see what can explode; as long as it's not reaching
soft-freeze.
Peter Xu [Fri, 2 Feb 2024 10:28:47 +0000 (18:28 +0800)]
migration/multifd: Move header prepare/fill into send_prepare()
This patch redefines the interfacing of ->send_prepare(). It further
simplifies multifd_send_thread() especially on zero copy.
Now with the new interface, we require the hook to do all the work for
preparing the IOVs to send. After it's completed, the IOVs should be ready
to be dumped into the specific multifd QIOChannel later.
Peter Xu [Fri, 2 Feb 2024 10:28:43 +0000 (18:28 +0800)]
migration/multifd: Rename p->num_packets and clean it up
This field, no matter whether on src or dest, is only used for debugging
purpose.
They can even be removed already, unless it still more or less provide some
accounting on "how many packets are sent/recved for this thread". The
other more important one is called packet_num, which is embeded in the
multifd packet headers (MultiFDPacket_t).
So let's keep them for now, but make them much easier to understand, by
doing below:
- Rename both of them to packets_sent / packets_recved, the old
name (num_packets) are waaay too confusing when we already have
MultiFDPacket_t.packets_num.
- Avoid worrying on the "initial packet": we know we will send it, that's
good enough. The accounting won't matter a great deal to start with 0 or
with 1.
- Move them to where we send/recv the packets. They're:
- multifd_send_fill_packet() for senders.
- multifd_recv_unfill_packet() for receivers.
Peter Xu [Fri, 2 Feb 2024 10:28:41 +0000 (18:28 +0800)]
migration/multifd: Simplify locking in sender thread
The sender thread will yield the p->mutex before IO starts, trying to not
block the requester thread. This may be unnecessary lock optimizations,
because the requester can already read pending_job safely even without the
lock, because the requester is currently the only one who can assign a
task.
Drop that lock complication on both sides:
(1) in the sender thread, always take the mutex until job done
(2) in the requester thread, check pending_job clear lockless
Peter Xu [Fri, 2 Feb 2024 10:28:40 +0000 (18:28 +0800)]
migration/multifd: Separate SYNC request with normal jobs
Multifd provide a threaded model for processing jobs. On sender side,
there can be two kinds of job: (1) a list of pages to send, or (2) a sync
request.
The sync request is a very special kind of job. It never contains a page
array, but only a multifd packet telling the dest side to synchronize with
sent pages.
Before this patch, both requests use the pending_job field, no matter what
the request is, it will boost pending_job, while multifd sender thread will
decrement it after it finishes one job.
However this should be racy, because SYNC is special in that it needs to
set p->flags with MULTIFD_FLAG_SYNC, showing that this is a sync request.
Consider a sequence of operations where:
- migration thread enqueue a job to send some pages, pending_job++ (0->1)
- [...before the selected multifd sender thread wakes up...]
- migration thread enqueue another job to sync, pending_job++ (1->2),
setup p->flags=MULTIFD_FLAG_SYNC
- multifd sender thread wakes up, found pending_job==2
- send the 1st packet with MULTIFD_FLAG_SYNC and list of pages
- send the 2nd packet with flags==0 and no pages
This is not expected, because MULTIFD_FLAG_SYNC should hopefully be done
after all the pages are received. Meanwhile, the 2nd packet will be
completely useless, which contains zero information.
I didn't verify above, but I think this issue is still benign in that at
least on the recv side we always receive pages before handling
MULTIFD_FLAG_SYNC. However that's not always guaranteed and just tricky.
One other reason I want to separate it is using p->flags to communicate
between the two threads is also not clearly defined, it's very hard to read
and understand why accessing p->flags is always safe; see the current impl
of multifd_send_thread() where we tried to cache only p->flags. It doesn't
need to be that complicated.
This patch introduces pending_sync, a separate flag just to show that the
requester needs a sync. Alongside, we remove the tricky caching of
p->flags now because after this patch p->flags should only be used by
multifd sender thread now, which will be crystal clear. So it is always
thread safe to access p->flags.
With that, we can also safely convert the pending_job into a boolean,
because we don't support >1 pending jobs anyway.
Always use atomic ops to access both flags to make sure no cache effect.
When at it, drop the initial setting of "pending_job = 0" because it's
always allocated using g_new0().
Peter Xu [Fri, 2 Feb 2024 10:28:39 +0000 (18:28 +0800)]
migration/multifd: Drop MultiFDSendParams.normal[] array
This array is redundant when p->pages exists. Now we extended the life of
p->pages to the whole period where pending_job is set, it should be safe to
always use p->pages->offset[] rather than p->normal[]. Drop the array.
Alongside, the normal_num is also redundant, which is the same to
p->pages->num.
This doesn't apply to recv side, because there's no extra buffering on recv
side, so p->normal[] array is still needed.
Peter Xu [Fri, 2 Feb 2024 10:28:38 +0000 (18:28 +0800)]
migration/multifd: Postpone reset of MultiFDPages_t
Now we reset MultiFDPages_t object in the multifd sender thread in the
middle of the sending job. That's not necessary, because the "*pages"
struct will not be reused anyway until pending_job is cleared.
Move that to the end after the job is completed, provide a helper to reset
a "*pages" object. Use that same helper when free the object too.
This prepares us to keep using p->pages in the follow up patches, where we
may drop p->normal[].
Merge them into the global one. The replacement is done by changing all
p->quit checks into the global var check. The global check doesn't need
any lock.
A few more things done on top of this altogether:
- multifd_send_terminate_threads()
Moving the xchg() of &multifd_send_state->exiting upper, so as to cover
the tracepoint, migrate_set_error() and migrate_set_state().
- multifd_send_sync_main()
In the 2nd loop, add one more check over the global var to make sure we
don't keep the looping if QEMU already decided to quit.
- multifd_tls_outgoing_handshake()
Use multifd_send_terminate_threads() to set the error state. That has
a benefit of updating MigrationState.error to that error too, so we can
persist that 1st error we hit in that specific channel.
- multifd_new_send_channel_async()
Take similar approach like above, drop the migrate_set_error() because
multifd_send_terminate_threads() already covers that. Unwrap the helper
multifd_new_send_channel_cleanup() along the way; not really needed.
William Roche [Tue, 30 Jan 2024 19:06:40 +0000 (19:06 +0000)]
migration: prevent migration when VM has poisoned memory
A memory page poisoned from the hypervisor level is no longer readable.
The migration of a VM will crash Qemu when it tries to read the
memory address space and stumbles on the poisoned page with a similar
stack trace:
Program terminated with signal SIGBUS, Bus error.
#0 _mm256_loadu_si256
#1 buffer_zero_avx2
#2 select_accel_fn
#3 buffer_is_zero
#4 save_zero_page
#5 ram_save_target_page_legacy
#6 ram_save_host_page
#7 ram_find_and_save_block
#8 ram_save_iterate
#9 qemu_savevm_state_iterate
#10 migration_iteration_run
#11 migration_thread
#12 qemu_thread_start
To avoid this VM crash during the migration, prevent the migration
when a known hardware poison exists on the VM.
hv-balloon: use get_min_alignment() to express 32 GiB alignment
Let's implement the get_min_alignment() callback for memory devices, and
copy for the device memory region the alignment of the host memory
region. This mimics what virtio-mem does, and allows for re-introducing
proper alignment checks for the memory region size (where we don't care
about additional device requirements) in memory device core.
Message-ID: <20240117135554.787344-2-david@redhat.com> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com> Signed-off-by: David Hildenbrand <david@redhat.com>
Better constraint for tcg_out_cmp, based on the comparison.
We can't yet remove the fallback to load constants into a
scratch because of tcg_out_cmp2, but that path should not
be as frequent.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Paolo Bonzini [Tue, 9 Jan 2024 22:30:32 +0000 (09:30 +1100)]
tcg/i386: Use TEST r,r to test 8/16/32 bits
Just like when testing against the sign bits, TEST r,r can be used when the
immediate is 0xff, 0xff00, 0xffff, 0xffffffff.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
projects / mirror_qemu.git / log