If the is_write argument is true, address_space_rw writes to memory
and thus reads from the buffer. The opposite holds if is_write is
false. Fix the model.
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
This is a small step towards making libcacard standalone.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
This is better and prepares for the next patch. When we copy
libs_softmmu's value into LIBS with a := assignment, we cannot
anymore modify libs_softmmu in the Makefiles.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Of the block devices that poked into -drive options via drive_get_next,
m25p80 was the only one who also did not attach itself to the BlockBackend.
Since sd does it, and all other devices go through a "drive" property,
with this change all block backends attached to the guest will have a
non-NULL result for blk_get_attached_dev().
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Message-id: 1429025387-11077-1-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
bdrv_aio_* APIs can use coroutines to achieve asynchronicity. However,
the coroutine may terminate without having yielded back to the caller
(for example because of something that invokes a nested event loop,
or because the coroutine is doing nothing at all). In this case,
the bdrv_aio_* API must delay the completion to the next iteration
of the main loop, because bdrv_aio_* will never invoke the callback
before returning.
This can be done with a bottom half, and indeed bdrv_aio_* is always
using one for simplicity. It is possible to gain some performance
(~3%) by avoiding this in the common case. A new field in the
BlockAIOCBCoroutine struct is set to true until the first time the
corotine has yielded to its creator, and completion goes through a
new function bdrv_co_complete. If the flag is false, bdrv_co_complete
invokes the callback immediately. If it is true, the caller will
notice that the coroutine has completed and schedule the bottom
half itself.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1427524638-28157-1-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This is the first step towards having fine-grained critical sections in
dataplane threads, which resolves lock ordering problems between
address_space_* functions (which need the BQL when doing MMIO, even
after we complete RCU-based dispatch) and the AioContext.
Because AioContext does not use contention callbacks anymore, the
unit test has to be changed.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1424449612-18215-4-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
By using thread-local storage, aio_poll can stop using global data during
g_poll_ns. This will make it possible to drop callbacks from rfifolock.
[Moved npfd = 0 assignment to end of walking_handlers region as
suggested by Paolo. This resolves the assert(npfd == 0) assertion
failure in pollfds_cleanup().
--Stefan]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 1424449612-18215-2-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
When an I/O port is more than 1 byte long, ioport.c is currently
creating "short" regions, for example 0x1ce-0x1ce for the 16-bit
Bochs index port. When I/O ports are memory mapped, and thus
accessed via a subpage_ops memory region, subpage_accepts gets
confused because it finds a hole at 0x1cf and rejects the access.
In order to fix this, modify registration of the region to cover
the whole size of the I/O port. Attempts to access an invalid
port will be blocked by find_portio returning NULL.
This only affects the VBE DISPI regions. For all other cases,
the MemoryRegionPortio entries for 2- or 4-byte accesses overlap
an entry for 1-byte accesses, thus the size of the memory region
is not affected.
Reported-by: Zoltan Balaton <balaton@eik.bme.hu>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Right now, ioport.c assumes that the entire range specified with
MemoryRegionPortio includes a region with size == 1. This however
is not true for the VBE DISPI ports, which are 16-bit only. The
next patch will make these regions' length equal to two, which can
cause the assertions to trigger. Replace them with simple conditionals.
Also, ioport.c will emulate a 16-bit ioport with two distinct reads
or writes, even if one of the two accesses is out of the bounds given
by the MemoryRegionPortio array. Do not do this anymore, instead
discard writes to the incorrect register and read it as all-ones.
This ensures that the mrp->read and mrp->write callbacks get an
in-range ioport number.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
ioport.c has not been using an alias since commit b40acf9 (ioport:
Switch dispatching to memory core layer, 2013-06-24). Remove the
obsolete comment.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
IDE PIO data must be written, for example, at 0x1f0. You cannot
do word or dword writes to 0x1f1..0x1f3 to access the data register.
Adjust the ide_portio_list accordingly.
Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Remove 16-bit reads/writes, since ioport.c is able to synthesize them.
Remove the two MIDI registers (0x300 and 0x301) from gus_portio_list1,
and add the second MIDI register (0x301) to gus_portio_list2.
Tested with Second Reality.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
ioport.c is already able to split a 16-bit access into two 8-bit
accesses to consecutive ports. Tested with Epic Pinball.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
ram_addr_t is an internal interface, everyone should go through
MemoryRegion. Clean it up by making rom_add_blob return a
MemoryRegion* and using the new qemu_ram_resize infrastructure.
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
As far as the QMP parser is concerned, neither the 'O' nor the 'q' format specifiers
put any constraint on the command. However, there are two differences:
1) from a documentation point of view 'O' says that this command takes
a dictionary. The dictionary will be converted to QemuOpts in the
handler to match the corresponding HMP command.
2) 'O' sets QMP_ACCEPT_UNKNOWNS, resulting in the command accepting invalid
extra arguments. For example the following is accepted:
{ "execute": "send-key",
"arguments": { "keys": [ { "type": "qcode", "data": "ctrl" },
{ "type": "qcode", "data": "alt" },
{ "type": "qcode", "data": "delete" } ], "foo": "bar" } }
Neither send-key nor migrate-set-capabilities take a QemuOpts-like
dictionary; they take an array of dictionaries. And neither command
really wants to have extra unknown arguments. Thus, the right
specifier to use in this case is 'q'; with this patch the above
command fails with
{"error": {"class": "GenericError", "desc": "Invalid parameter 'foo'"}}
as intended.
Reported-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
After commit 5312bd8 the bonito_readl() and bonito_writel() have been
accessing incorrect addresses. Consequently QEMU is crashing when trying
to boot Linux kernel on fulong2e machine.
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
There are two problems with memory barriers in async.c. The fix is
to use atomic_xchg in order to achieve sequential consistency between
the scheduling of a bottom half and the corresponding execution.
First, if bh->scheduled is already 1 in qemu_bh_schedule, QEMU does
not execute a memory barrier to order any writes needed by the callback
before the read of bh->scheduled. If the other side sees req->state as
THREAD_ACTIVE, the callback is not invoked and you get deadlock.
Second, the memory barrier in aio_bh_poll is too weak. Without this
patch, it is possible that bh->scheduled = 0 is not "published" until
after the callback has returned. Another thread wants to schedule the
bottom half, but it sees bh->scheduled = 1 and does nothing. This causes
a lost wakeup. The memory barrier should have been changed to smp_mb()
in commit 924fe12 (aio: fix qemu_bh_schedule() bh->ctx race condition,
2014-06-03) together with qemu_bh_schedule()'s. Guess who reviewed
that patch?
Both of these involve a store and a load, so they are reproducible on
x86_64 as well. It is however much easier on aarch64, where the
libguestfs test suite triggers the bug fairly easily. Even there the
failure can go away or appear depending on compiler optimization level,
tracing options, or even kernel debugging options.
Paul Leveille however reported how to trigger the problem within 15
minutes on x86_64 as well. His (untested) recipe, reproduced here
for reference, is the following:
1) Qcow2 (or 3) is critical – raw files alone seem to avoid the problem.
2) Use “cache=directsync” rather than the default of
“cache=none” to make it happen easier.
3) Use a server with a write-back RAID controller to allow for rapid
IO rates.
4) Run a random-access load that (mostly) writes chunks to various
files on the virtual block device.
a. I use ‘diskload.exe c:25’, a Microsoft HCT load
generator, on Windows VMs.
b. Iometer can probably be configured to generate a similar load.
5) Run multiple VMs in parallel, against the same storage device,
to shake the failure out sooner.
6) IvyBridge and Haswell processors for certain; not sure about others.
A similar patch survived over 12 hours of testing, where an unpatched
QEMU would fail within 15 minutes.
This bug is, most likely, also the cause of failures in the libguestfs
testsuite on AArch64.
Thanks to Laszlo Ersek for initially reporting this bug, to Stefan
Hajnoczi for suggesting closer examination of qemu_bh_schedule, and to
Paul for providing test input and a prototype patch.
Reported-by: Laszlo Ersek <lersek@redhat.com>
Reported-by: Paul Leveille <Paul.Leveille@stratus.com>
Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1428419779-26062-1-git-send-email-pbonzini@redhat.com
Suggested-by: Paul Leveille <Paul.Leveille@stratus.com>
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
After qemu_iovec_destroy, the QEMUIOVector's size is zeroed and
the zero size ultimately is used to compute virtqueue_push's len
argument. Therefore, reads from virtio-blk devices did not
migrate their results correctly. (Writes were okay).
Save the size in virtio_blk_handle_request, and use it when the request
is completed.
Based on a patch by Wen Congyang.
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Tested-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Message-id: 1427997044-392-1-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
This reverts commit c3c1bb99d1.
It causes problems with boards that declare memory regions shorter
than the registers they contain.
Reported-by: Zoltan Balaton <balaton@eik.bme.hu>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
If QEMU forks after the CPU threads have been created, qemu_mutex_lock_iothread
will not be able to do qemu_cpu_kick_thread. There is no solution other than
assuming that forks after the CPU threads have been created will end up in an
exec. Forks before the CPU threads have been created (such as -daemonize)
have to call rcu_after_fork manually.
Notably, the oxygen theme for GTK+ forks and shows a "No such process" error
without this patch.
This patch can be reverted once the iothread loses the "kick the TCG thread"
magic.
User-mode emulation does not use the iothread, so it can also call
rcu_after_fork.
Reported by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
It is not necessary to munmap an area before remapping it with MAP_FIXED;
if the memory region specified by addr and len overlaps pages of any
existing mapping, then the overlapped part of the existing mapping will
be discarded.
On the other hand, if QEMU does munmap the pages, there is a small
probability that another mmap sneaks in and catches the just-freed
portion of the address space. In effect, munmap followed by
mmap(MAP_FIXED) is a use-after-free error, and Coverity flags it
as such. Fix it.
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Capture the explicit setting of "usb=no" into a separate bool, and
use it to skip the update of machine->usb in the board init function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
It is wrong to use address_space_memory directly, because there could be an
IOMMU in the middle. Passing the entire PVSCSIRingInfo to RS_GET_FIELD
and RS_SET_FIELD makes it easy to go back to the PVSCSIState.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
32-bit PPC cannot do atomic operations on long long. Inside the loops,
we are already using local counters that are summed at the end of
the run---with some exceptions (rcu_stress_count for rcutorture,
n_nodes for test-rcu-list): fix them to use the same technique.
For test-rcu-list, remove the mostly unused member "val" from the
list. Then, use a mutex to protect the global counts.
Performance does not matter there because every thread will only enter
the critical section once.
Remaining uses of atomic instructions are for ints or pointers.
Reported-by: Andreas Faerber <afaerber@suse.de>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
monitor_fdset_add_fd returns an AddfdInfo struct (used by the QMP
command add_fd). Free it.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
The .d file name must match exactly what is used in the SUBDIR_DEVICES_MAK_DEP
variable. Instead of making assumptions in the make_device_config.sh script,
just pass it in.
Similarly, the makefile target may not match the output file name, because
Makefile uses a temporary file. Instead of making assumptions on what the
Makefile does, emit the config-devices.mak file to stdout, and use the
passed-in destination as the makefile target
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Using net_host_check_device is unnecessary. qemu_del_net_client asserts
for the non-peer case that it can only process NIC type NetClientStates,
and that assertion is valid for the peered case as well, so move it and
use the same check in net_host_device_remove. host_net_remove_completion
is already checking the type.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Jason Wang <jasowang@redhat.com>
Message-id: 1419353600-30519-2-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
There is a not-so-subtle race in QSLIST_INSERT_HEAD_ATOMIC.
Because atomic_cmpxchg returns the old value instead of a success flag,
QSLIST_INSERT_HEAD_ATOMIC was checking for success by comparing against
the second argument to atomic_cmpxchg. Unfortunately, this only works
if the second argument is a local or thread-local variable.
If it is in memory, it can be subject to common subexpression elimination
(and then everything's fine) or reloaded after the atomic_cmpxchg,
depending on the compiler's whims. If the latter happens, the race can
happen. A thread can sneak in, doing something on elm->field.sle_next
after the atomic_cmpxchg and before the comparison. This causes a wrong
failure, and then two threads are using "elm" at the same time. In the
case discovered by Christian, the sequence was likely something like this:
thread 1 | thread 2
QSLIST_INSERT_HEAD_ATOMIC |
atomic_cmpxchg succeeds |
elm added to list |
| steal release_pool
| QSLIST_REMOVE_HEAD
| elm removed from list
| ...
| QSLIST_INSERT_HEAD_ATOMIC
| (overwrites sle_next)
spurious failure |
atomic_cmpxchg succeeds |
elm added to list again |
|
steal release_pool |
QSLIST_REMOVE_HEAD |
elm removed again |
The last three steps could be done by a third thread as well.
A reproducer that failed in a matter of seconds is as follows:
- the guest has 32 VCPUs on a 28 core host (hyperthreading was enabled),
memory was 16G just to err on the safe side (the host has 64G, but hey
at least you need no s390)
- the guest has 24 null-aio virtio-blk devices using dataplane
(-object iothread,id=ioN -drive if=none,id=blkN,driver=null-aio,size=500G
-device virtio-blk-pci,iothread=ioN,drive=blkN)
- the guest also has a single network interface. It's only doing loopback
tests so slirp vs. tap and the model doesn't matter.
- the guest is running fio with the following script:
[global]
rw=randread
blocksize=16k
ioengine=libaio
runtime=10m
buffered=0
fallocate=none
time_based
iodepth=32
[virtio1a]
filename=/dev/block/252\:16
[virtio1b]
filename=/dev/block/252\:16
...
[virtio24a]
filename=/dev/block/252\:384
[virtio24b]
filename=/dev/block/252\:384
[listen1]
protocol=tcp
ioengine=net
port=12345
listen
rw=read
bs=4k
size=1000g
[connect1]
protocol=tcp
hostname=localhost
ioengine=net
port=12345
protocol=tcp
rw=write
startdelay=1
size=1000g
...
[listen8]
protocol=tcp
ioengine=net
port=12352
listen
rw=read
bs=4k
size=1000g
[connect8]
protocol=tcp
hostname=localhost
ioengine=net
port=12352
rw=write
startdelay=1
size=1000g
Moral of the story: I should refrain from writing more clever stuff.
At least it looks like it is not too clever to be undebuggable.
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1426002357-6889-1-git-send-email-pbonzini@redhat.com
Fixes: c740ad92d0
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
This is easy, since start_dma already restarts processing from the
beginning of the PRDT.
Migration is also easy to cover; the comment about busy_slot is
wrong, busy_slot will only be set if there is an error. In this
case we have nothing to do really. The core IDE code will restart
the operation and command list processing will proceed after the
erroring command has been completed.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1424708286-16483-16-git-send-email-jsnow@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Resetting the io_buffer_index to 0 is commonized,
with the exception of the case within ide_atapi_cmd_reply,
where we need to reset this index to 0 prior to the
ide_atapi_cmd_reply_end call.
Note that not all calls to ide_atapi_cmd_reply_end
expect the index to be 0, so setting it there is
not appropriate.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1424708286-16483-12-git-send-email-jsnow@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This only breaks backwards migration compatibility if the bus is in
an error state. It is in principle possible to avoid this by making
two subsections (one for version 1, and one for version 2, but with
the same name) with different "_needed" callbacks. The v1 callback would
return true if error_status != 0 and the bus is PATA; the v2 callback
would return true if error_status != 0 and the bus is AHCI.
Forward migration keeps working.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1424708286-16483-11-git-send-email-jsnow@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
With BMDMA specific excised from the restart functions,
create a HBA-agnostic restart callback to be shared
between the different HBAs.
Change the callback registered with the vmstate_change
handler to always point to ide_restart_cb instead of
relying on the IDEDMAOps.restart_cb() member.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 1424708286-16483-7-git-send-email-jsnow@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This fixes a NULL pointer dereference in s390x-softmmu.
On pretty much all other architectures, creating an MMIO region calls
cpu_reload_memory_map. On s390, however, there are no MMIO regions
and everything is done via hypercalls.
Fixes: 9d82b5a792
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
After forking, only the calling thread is duplicated in the child process.
The call_rcu thread has to be recreated in the child. Exploit the fact
that only one thread exists (same as when constructors run), and just redo
the entire initialization to ensure the threads are in the proper state.
The only additional things to do are emptying the list of threads
registered with RCU, and unlocking the lock that was taken in the prepare
callback (implementations are allowed to fail pthread_mutex_init()
if the mutex is still locked).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
PTHREAD_MUTEX_ERRORCHECK is completely broken with respect to fork.
The way to safely do fork is to bring all threads to a quiescent
state by acquiring locks (either in callers---as we do for the
iothread mutex---or using pthread_atfork's prepare callbacks)
and then release them in the child.
The problem is that releasing error-checking locks in the child
fails under glibc with EPERM, because the mutex stores a different
owner tid than the duplicated thread in the child process. We
could make it work for locks acquired via pthread_atfork, by
recreating the mutex in the child instead of unlocking it
(we know that there are no other threads that could have taken
the mutex; but when the lock is acquired in fork's caller
that would not be possible.
The simplest solution is just to forgo error checking.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Coverity reports that s->chr is checked after put_packet dereferences it.
Move the check earlier, consistent with the code used for user-mode
emulation.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Avoid truncation of a 64-bit long to a 32-bit int, and check for errno
(especially ERANGE).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
If the iothread lock isn't taken by the main thread, the RCU callbacks
might run concurrently with the main thread. QEMU's not ready for that.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
Tested-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
For good measure, ensure that the following sequence:
thread 1 calls qemu_mutex_lock_iothread
thread 2 calls qemu_mutex_lock_iothread
VCPU thread are created
VCPU thread enters execution loop
results in the VCPU threads letting the other two threads run
and obeying iothread_requesting_mutex even if the VCPUs are
not halted. To do this, check iothread_requesting_mutex
before execution starts.
Tested-by: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
When two threads (other than the low-priority TCG VCPU thread)
are competing for the iothread lock, a deadlock can happen. This
is because iothread_requesting_mutex is set to false by the first
thread that gets the mutex, and then the VCPU thread might never
yield from the execution loop. If iothread_requesting_mutex is
changed from a bool to a counter, the deadlock is fixed.
However, there is another bug in qemu_mutex_lock_iothread that
can be triggered by the new call_rcu thread. The bug happens
if qemu_mutex_lock_iothread is called before the CPUs are
created. In that case, first_cpu is NULL and the caller
segfaults in qemu_mutex_lock_iothread. To fix this, just
do not do the kick if first_cpu is NULL.
Reported-by: Leon Alrae <leon.alrae@imgtec.com>
Reported-by: Andreas Gustafsson <gson@gson.org>
Tested-by: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This mimics what is done in qdev_device_add, and lets the device be
freed in case something goes wrong. Otherwise, object_unparent returns
immediately without freeing the device, which is on the other hand left
in the parent bus's list of children.
scsi_bus_legacy_handle_cmdline then returns an error, and the HBA is
destroyed as well with object_unparent. But the lingering device that
was not removed in scsi_bus_legacy_add_drive cannot be removed now either,
and bus_unparent gets stuck in an infinite loop trying to empty the list
of children.
The right fix of course would be to assert in bus_add_child that the
device already has a bus, and remove the "safety net" that adds the
drive to the QOM tree in device_set_realized. I am not yet sure whether
that would entail changing all callers to qdev_create (as well as
isa_create and usb_create and the corresponding _try_create versions).
Reported-by: Markus Armbruster <armbru@redhat.com>
Tested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Note that even after this patch, most callers of address_space_*
functions must still be under the big QEMU lock, otherwise the memory
region returned by address_space_translate can disappear as soon as
address_space_translate returns. This will be fixed in the next part
of this series.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
After the previous patch, TLBs will be flushed on every change to
the memory mapping. This patch augments that with synchronization
of the MemoryRegionSections referred to in the iotlb array.
With this change, it is guaranteed that iotlb_to_region will access
the correct memory map, even once the TLB will be accessed outside
the BQL.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This for now is a simple TLB flush. This can change later for two
reasons:
1) an AddressSpaceDispatch will be cached in the CPUState object
2) it will not be possible to do tlb_flush once the TCG-generated code
runs outside the BQL.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
object_unparent should not be called until the parent device is going to be
destroyed. Only remove the capability and do memory_region_del_subregion
at unrealize time. Freeing the data structures is left in shpc_free, to
be called from the instance_finalize callback.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This memory leak was introduced inadvertently by omitting object_unparent.
A better fix is to use the new memory_region_set_size instead of destroying
and recreating the MMIO region on the fly.
Also, ensure that unmapping and remapping the region is done atomically.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This needs to go away sooner or later, but one complication is the
complex VFIO data structures that are modified in instance_finalize.
Take a shortcut for now.
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Always process them within a short time. Even though waiting a little
is useful, it is not okay to delay e.g. qemu_opts_del forever.
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Tested-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In the case of VFIO, the unrealize callback is too early to munmap the
BARs. The munmap must be delayed until memory accesses are complete.
To do this, split vfio_unmap_bars in two. The removal step, now called
vfio_unregister_bars, remains in vfio_exitfn. The reclamation step
is vfio_unmap_bars and is moved to the instance_finalize callback.
Similarly, quirk MemoryRegions have to be removed during
vfio_unregister_bars, but freeing the data structure must be delayed
to vfio_unmap_bars.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
In order to enable out-of-BQL address space lookup, destruction of
devices needs to be split in two phases.
Unrealize is the first phase; once it complete no new accesses will
be started, but there may still be pending memory accesses can still
be completed.
The second part is freeing the device, which only happens once all memory
accesses are complete. At this point the reference count has dropped to
zero, an RCU grace period must have completed (because the RCU-protected
FlatViews hold a reference to the device via memory_region_ref). This is
when instance_finalize is called.
Freeing data belongs in an instance_finalize callback, because the
dynamically allocated memory can still be used after unrealize by the
pending memory accesses.
This starts the process by creating an instance_finalize callback and
freeing most of the dynamically-allocated data in instance_finalize.
Because instance_finalize is also called on error paths or also when
the device is actually not realized, the common code needs some changes
to be ready for this. The error path in vfio_initfn can be simplified too.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Now that vfio_put_base_device is called unconditionally at instance_finalize
time, it can be called twice if vfio_populate_device fails. This works
but it is slightly harder to follow.
Change vfio_get_device to not touch the vbasedev struct until it will
definitely succeed, moving the vfio_populate_device call back to vfio-pci.
This way, vfio_put_base_device will only be called once.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
address_space_destroy_dispatch is called from an RCU callback and hence
outside the iothread mutex (BQL). However, after address_space_destroy
no new accesses can hit the destroyed AddressSpace so it is not necessary
to observe changes to the memory map. Move the memory_listener_unregister
call earlier, to make it thread-safe again.
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Fixes: 374f2981d1
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
bits is checked to be 128, 192 or 256 at the beginning of the function.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Coverity complains about not checking the returned value of mkstemp. While
at it, also improve error checking for snprintf, and refine error messages
in general.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Use MIN instead of an "if" statement. Move "tb" assignment where
the value is actually used.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
All uses of TB inside cpu_exec are dominated by "tb = tb_find_fast(env)",
and there are no uses after the switch statement. So the assignment
is dead, as reported by Coverity.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Pavel Dovgalyuk reports that TimerExpire and the timer are not restored
correctly on the receiving end of migration.
It is not clear to me whether this is really the case, but we can take
the occasion to get rid of the complicated code that computes PCSTimeout
on the fly upon changes to IntrStatus/IntrMask. Just always keep a
timer running, it will fire every ~130 seconds at most if the interrupt
is masked with TimerInt != 0.
This makes rtl8139_set_next_tctr_time idempotent (when the virtual clock
is stopped between two calls, as is the case during migration).
Tested with Frediano's qtest.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-id: 1421765099-26190-1-git-send-email-pbonzini@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
With the introduction of QEMU_CLOCK_VIRTUAL_RT, the computation of
sc->diff_clk can be simplified nicely:
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
qemu_clock_get_ns(QEMU_CLOCK_REALTIME) +
cpu_get_clock_offset()
= qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
(qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - cpu_get_clock_offset())
= qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
(qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timers_state.cpu_clock_offset)
= qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT)
Cc: Sebastian Tanase <sebastian.tanase@openwide.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
sc->diff_clk is already equal to sleep_delay (split in a second and a
nanosecond part). If you subtract sleep_delay - rem_delay, the result
is exactly rem_delay.
Cc: Sebastian Tanase <sebastian.tanase@openwide.fr>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Do the entire lookup under RCU, which avoids atomic operations
in flatview_ref and flatview_unref.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Replace the flat_view_mutex with RCU, avoiding futex contention for
dataplane on large systems and many iothreads.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Asynchronous callbacks provided by call_rcu are particularly important
for QEMU, because the BQL makes it hard to use synchronize_rcu.
In addition, the current RCU implementation is not particularly friendly
to multiple concurrent synchronize_rcu callers, making call_rcu even
more important.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This includes a (mangled) copy of the liburcu code. The main changes
are: 1) removing dependencies on many other header files in liburcu; 2)
removing for simplicity the tentative busy waiting in synchronize_rcu,
which has limited performance effects; 3) replacing futexes in
synchronize_rcu with QemuEvents for Win32 portability. The API is
the same as liburcu, so it should be possible in the future to require
liburcu on POSIX systems for example and use our copy only on Windows.
Among the various versions available I chose urcu-mb, which is the
least invasive implementation even though it does not have the
fastest rcu_read_{lock,unlock} implementation. The urcu flavor can
be changed later, after benchmarking.
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This had a possible deadlock that was visible with rcutorture.
qemu_event_set qemu_event_wait
----------------------------------------------------------------
cmpxchg reads FREE, writes BUSY
futex_wait: pthread_mutex_lock
futex_wait: value == BUSY
xchg reads BUSY, writes SET
futex_wake: pthread_cond_broadcast
futex_wait: pthread_cond_wait
<deadlock>
The fix is simply to avoid condvar tricks and do the obvious locking
around pthread_cond_broadcast:
qemu_event_set qemu_event_wait
----------------------------------------------------------------
cmpxchg reads FREE, writes BUSY
futex_wait: pthread_mutex_lock
futex_wait: value == BUSY
xchg reads BUSY, writes SET
futex_wake: pthread_mutex_lock
(blocks)
futex_wait: pthread_cond_wait
(mutex unlocked)
futex_wake: pthread_cond_broadcast
futex_wake: pthread_mutex_unlock
futex_wait: pthread_mutex_unlock
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Coverity flags this as "dereference after null check". Not quite a
dereference, since it will just EFAULT, but still nice to fix.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Right now you only get to apic_init_reset if you have an APIC
(do_cpu_init is reached only if CPU_INTERRUPT_INIT is set and
that only happens in hw/intc/apic.c). However, this is wrong
because for example a port 92 or keyboard controller reset is
really an INIT, and that can happen also with no APIC. So
keep the check and fix the error that Coverity reported.
Reported-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In some cases, a timer was set to NULL so that we could check if it is
initialized. Use the timer_list field instead, and add a timer_deinit
function that NULLs it.
It then makes sense that timer_del be a no-op (instead of a crasher) on
such a de-initialized timer. It avoids the need to poke at the timerlist
field to check if the timers are initialized.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Right now, the AVX512 registers are split in many different fields:
xmm_regs for the low 128 bits of the first 16 registers, ymmh_regs
for the next 128 bits of the same first 16 registers, zmmh_regs
for the next 256 bits of the same first 16 registers, and finally
hi16_zmm_regs for the full 512 bits of the second 16 bit registers.
This makes it simple to move data in and out of the xsave region,
but would be a nightmare for a hypothetical TCG implementation and
leads to a proliferation of [XYZ]MM_[BWLSQD] macros. Instead,
this patch marshals data manually from the xsave region to a single
32x512-bit array, simplifying the macro jungle and clarifying which
bits are in which vmstate subsection.
The migration format is unaffected.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
After the next patch, each vmstate field will extract parts of a larger
(32x512-bit) array, so we cannot check the vmstate field against the
type of the array.
While changing this, change the macros to accept the index of the first
element (which will not be 0 for Hi16_ZMM_REGS) instead of the number
of elements (which is always CPU_NB_REGS).
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Due to a typo, instead of disabling KVM_FEATURE_PV_EOI (bit
6) these machine types are disabling bits 1 and 2, which are
KVM_FEATURE_NOP_IO_DELAY and KVM_FEATURE_MMU_OP. Not a big deal
because they aren't very important and KVM_FEATURE_MMU_OP is
disabled anyway. The worst part is actually that KVM_FEATURE_PV_EOI
is remaining enabled.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Paolo Bonzini