QEMU currently crashes when you try to hot-plug an "nvdimm" device
on older machine types:
$ qemu-system-x86_64 -monitor stdio -M pc-1.1
QEMU 3.1.92 monitor - type 'help' for more information
(qemu) device_add nvdimm,id=nvdimmn1
qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
Assertion `*errp == ((void *)0)' failed.
Aborted (core dumped)
The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
added recently before the check whether nvdimm is enabled. It should
be done after the check. And while we're at it, also check the errp
after the hotplug_handler_pre_plug(), otherwise errors are silently
ignored here.
Fixes: 9040e6dfa8
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20190407092314.11066-1-thuth@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
In the accessor functions ld*_he_p() and st*_he_p() we use memcpy()
to perform a load or store to a pointer which might not be aligned
for the size of the type. We rely on the compiler to optimize this
memcpy() into an efficient load or store instruction where possible.
This is required for good performance, but at the moment it is also
required for correct operation, because some users of these functions
require that the access is atomic if the pointer is aligned, which
will only be the case if the compiler has optimized out the memcpy().
(The particular example where we discovered this is the virtio
vring_avail_idx() which calls virtio_lduw_phys_cached() which
eventually ends up calling lduw_he_p().)
Unfortunately some compile environments, such as the fortify-source
setup used in Alpine Linux, define memcpy() to a wrapper function
in a way that inhibits this compiler optimization.
The correct long-term fix here is to add a set of functions for
doing atomic accesses into AddressSpaces (and to other relevant
families of accessor functions like the virtio_*_phys_cached()
ones), and make sure that callsites which want atomic behaviour
use the correct functions.
In the meantime, switch to using __builtin_memcpy() in the
bswap.h accessor functions. This will make us robust against things
like this fortify library in the short term. In the longer term
it will mean that we don't end up with these functions being really
badly-performing even if the semantics of the out-of-line memcpy()
are correct.
Reported-by: Fernando Casas Schössow <casasfernando@outlook.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20190318112938.8298-1-peter.maydell@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Since commit f590a812c2 we build the EDK2 EfiRom utility
unconditionally.
Some distributions require to use extra compiler/linker flags,
i.e. SUSE which enforces the PIE protection (see [*]).
EDK2 build tools already provide a set of variables for that,
use them to allow the caller to easily inject compiler/linker
options..
Now build scripts can pass extra options, example:
$ make -C roms \
EDK2_BASETOOLS_OPTFLAGS='-fPIE' \
efirom
[*] https://lists.opensuse.org/opensuse-factory/2017-06/msg00403.html
Reported-by: Olaf Hering <olaf@aepfle.de>
Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190409134536.15548-3-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
The iPXE's 'veryclean' recipe removes $(EFIROM) even if the EFIROM
macro originates from elsewhere:
$ git checkout f590a812c21~
$ make -C roms clean EFIROM=$(type -P EfiRom)
make: Entering directory '/source/qemu/roms'
[...]
make -C ipxe/src veryclean
make[1]: Entering directory '/source/qemu/roms/ipxe/src'
rm -f bin{,-*}/*.* bin{,-*}/.certificate.* bin{,-*}/.certificates.* bin{,-*}/.private_key.* bin{,-*}/errors bin{,-*}/NIC ./util/zbin ./util/elf2efi32 ./util/elf2efi64 /usr/bin/EfiRom ./util/efifatbin ./util/iccfix ./util/einfo TAGS bin{,-*}/symtab
rm: cannot remove '/usr/bin/EfiRom': Permission denied
make[1]: *** [Makefile.housekeeping:1564: clean] Error 1
make[1]: Leaving directory '/source/qemu/roms/ipxe/src'
make: *** [Makefile:152: clean] Error 2
make: Leaving directory '/source/qemu/roms'
Before f590a812c2 this variable could be overridden or unset,
and the 'veryclean' Makefile rule would not complain.
Commit f590a812c2 enforces this variable to the Intel EfiRom
tool provided by the EDK2 project.
To avoid the name clash and make the difference between the
projects obvious, rename the variable used by the EDK2 project
as EDK2_EFIROM.
Fixes: f590a812c2
Reported-by: Olaf Hering <olaf@aepfle.de>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20190409134536.15548-2-philmd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Add a missing space to the error message used when giving up on a
server that insists on an alignment which renders the last few bytes
of the export unreadable.
Fixes: 3add3ab78
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190404145226.32649-1-eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
In commit 0c1d50bd, I added a couple of TODO comments about whether we
consult bl.request_alignment when responding to NBD_OPT_INFO. At the
time, qemu as server was hard-coding an advertised alignment of 512 to
clients that promised to obey constraints, and there was no function
for getting at a device's preferred alignment. But in hindsight,
advertising 512 when the block device prefers 1 caused other
compliance problems, and commit b0245d64 changed one of the two TODO
comments to advertise a more accurate alignment. Time to fix the other
TODO. Doesn't really impact qemu as client (our normal client doesn't
use NBD_OPT_INFO, and qemu-nbd --list promises to obey block sizes),
but it might prove useful to other clients.
Fixes: b0245d64
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-4-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
We've recently added traces for clients to flag server non-compliance;
let's do the same for servers to flag client non-compliance. According
to the spec, if the client requests NBD_INFO_BLOCK_SIZE, it is
promising to send all requests aligned to those boundaries. Of
course, if the client does not request NBD_INFO_BLOCK_SIZE, then it
made no promises so we shouldn't flag anything; and because we are
willing to handle clients that made no promises (the spec allows us to
use NBD_REP_ERR_BLOCK_SIZE_REQD if we had been unwilling), we already
have to handle unaligned requests (which the block layer already does
on our behalf). So even though the spec allows us to return EINVAL
for clients that promised to behave, it's easier to always answer
unaligned requests. Still, flagging non-compliance can be useful in
debugging a client that is trying to be maximally portable.
Qemu as client used to have one spot where it sent non-compliant
requests: if the server sends an unaligned reply to
NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire
disk, the next request would start at that unaligned point; this was
fixed in commit a39286dd when the client was taught to work around
server non-compliance; but is equally fixed if the server is patched
to not send unaligned replies in the first place (yes, qemu 4.0 as
server still has few such bugs, although they will be patched in
4.1). Fortunately, I did not find any more spots where qemu as client
was non-compliant. I was able to test the patch by using the following
hack to convince qemu-io to run various unaligned commands, coupled
with serving 512-byte alignment by intentionally omitting '-f raw' on
the server while viewing server traces.
| diff --git i/nbd/client.c w/nbd/client.c
| index 427980bdd22..1858b2aac35 100644
| --- i/nbd/client.c
| +++ w/nbd/client.c
| @@ -449,6 +449,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
| nbd_send_opt_abort(ioc);
| return -1;
| }
| + info->min_block = 1;//hack
| if (!is_power_of_2(info->min_block)) {
| error_setg(errp, "server minimum block size %" PRIu32
| " is not a power of two", info->min_block);
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-3-eblake@redhat.com>
[eblake: address minor review nits]
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Don't increment remaining_bytes until we know that we will actually be
including the current block status extent in the reply; otherwise, the
value traced will include a bytes value that is oversized by the
length of the next block status extent which did not get sent because
it instead ended the loop.
Fixes: fb7afc79
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
drive_new() returns null without setting an error when it provided
help. add_init_drive() assumes null means failure, and crashes trying
to report a null error.
Fixes: c4f26c9f37
Cc: qemu-stable@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
The glibc-2.29.9000-6.fc31.x86_64 package finally includes the gettid()
function as part of unistd.h when __USE_GNU is defined. This clashes
with linux-user code which unconditionally defines this function name
itself.
/home/berrange/src/virt/qemu/linux-user/syscall.c:253:16: error: static declaration of ‘gettid’ follows non-static declaration
253 | _syscall0(int, gettid)
| ^~~~~~
/home/berrange/src/virt/qemu/linux-user/syscall.c:184:13: note: in definition of macro ‘_syscall0’
184 | static type name (void) \
| ^~~~
In file included from /usr/include/unistd.h:1170,
from /home/berrange/src/virt/qemu/include/qemu/osdep.h:107,
from /home/berrange/src/virt/qemu/linux-user/syscall.c:20:
/usr/include/bits/unistd_ext.h:34:16: note: previous declaration of ‘gettid’ was here
34 | extern __pid_t gettid (void) __THROW;
| ^~~~~~
CC aarch64-linux-user/linux-user/signal.o
make[1]: *** [/home/berrange/src/virt/qemu/rules.mak:69: linux-user/syscall.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:449: subdir-aarch64-linux-user] Error 2
While we could make our definition conditional and rely on glibc's impl,
this patch simply renames our definition to sys_gettid() which is a
common pattern in this file.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Message-Id: <20190320161842.13908-3-berrange@redhat.com>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
The gettid syscall was introduced in Linux 2.4.11. This is old enough
that we can assume it always exists and thus not bother with the
conditional backcompat logic.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Message-Id: <20190320161842.13908-2-berrange@redhat.com>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
When bdrv_temp_snapshot_options() is called for snapshot=on, the
'discard' option in the options QDict hasn't been parsed and merged into
the flags yet. So copy the dict entry to make sure that the temporary
overlay enables discard when it was requested for the drive.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
The test uses the trick:
if (!opts) {
opts = &(QOSGraph...Options) { };
}
in a couple of places, however the temporary created
by the &() {} goes out of scope at the bottom of the if,
and results in a seg or assert when opts-> fields are
used (on fedora 30's gcc 9).
Fixes: fc281c8020
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190405184037.16799-1-dgilbert@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h.
FALSE and TRUE (with capital letters) are the constants defined by glib for
being used with the "gboolean" type of glib. But some parts of the code also use
TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>).
Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>
Message-Id: <1553351197-14581-4-git-send-email-cafer.abdi@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h.
FALSE and TRUE (with capital letters) are the constants defined by glib for
being used with the "gboolean" type of glib. But some parts of the code also use
TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>).
Signed-off-by: Jafar Abdi <cafer.abdi@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <1553351197-14581-3-git-send-email-cafer.abdi@gmail.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Otherwise we are setting err twice, what is wrong and causes an abort.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20190403114958.3705-2-quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
I found upstream codes conflict with COLO and lead to crash,
and I located to this patch:
commit 386a907b37
Author: Wei Wang <wei.w.wang@intel.com>
Date: Tue Dec 11 16:24:49 2018 +0800
migration: use bitmap_mutex in migration_bitmap_clear_dirty
My colleague Wei's patch add bitmap_mutex in migration_bitmap_clear_dirty,
but COLO didn't initialize the bitmap_mutex. So we always get an error
when COLO start up. like that:
qemu-system-x86_64: util/qemu-thread-posix.c:64: qemu_mutex_lock_impl: Assertion `mutex->initialized' failed.
This patch add the bitmap_mutex initialize and destroy in COLO
lifecycle.
Signed-off-by: Zhang Chen <chen.zhang@intel.com>
Message-Id: <20190329222951.28945-1-chen.zhang@intel.com>
Reviewed-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
This patch set contains a pair of tightly coupled PLIC bug fixes:
* We were calculating the PLIC addresses incorrectly.
* We were installing the wrong number of PLIC interrupts.
The two bugs togther resulted in a mostly-working system, but they're
impossible to seperate because fixing one bug would result in
significant breakage. As a result they're in the same patch.
There is also a cleanup to use qemu_log_mask(LOG_GUEST_ERROR,...) for
error reporting.
As far as I know these are the last outstanding RISC-V patches for 4.0.
v2 no longer fails "make check" for me... sorry!
-----BEGIN PGP SIGNATURE-----
iQJHBAABCAAxFiEEAM520YNJYN/OiG3470yhUCzLq0EFAlymonUTHHBhbG1lckBk
YWJiZWx0LmNvbQAKCRDvTKFQLMurQX+kD/wIOSTb7ZBAu5Jbs9JckaGhom9Kfu1+
D9Pxs+QHnXxvxzksTYIWtOVJ8otYvoz/zt8OntbBO9J5eHeHe5aQQ1B+L8+2+Z8Z
yVcKwu7UPHTY0u6gsE7tAGIhw/pPK+bSM0BA7jOTV6VB4wjLB4KnHbNZytiBTgg7
OzFDqxCgjva8lNjjUJO1vfdGBHfacHEEfVOGxWkotaXw6mXaSzd+lbPGtnwLsca+
NJOObR5Z25BIzS7R8Ud9epT84sK/iwffbZbfEUZ/cIu3Ghd57xl0diieZGCBJ5Xf
6Ngq8Pae6hP0mK4DhpKdN+OMpEqX95Vd+azBxxOLY2ITNaKC+v68t6k58a/kTL77
reBAGU8VKgcHTFx1atG7Sbfq/aOm53McVHOleWZL4W+peqhH2z5TUcGrSfvhIs/4
4rp7d2zo4J7R42TI2RO8HeLF9+fX857Qwz4GICaFrSZ5m7eoiZirt27YzjUutf6b
D1wTb0ZEJh3b2WZrne+mrV0p/nT1lgCk5byOjk20RTXeWVC5zEX8JiMt47qx1VVx
1KvnNy35aGUc872Fsa7zNvtErBXQ7UosuWZlLQh1dLBPXQ85/YY6W9fkgYLc8/LD
Lc5W6kf3vfWYajeVYLpFB/kF9QuU3f0OzZrnG+K/Vr68IhBuPlJTox7vQERlmp2S
bRzEh/91Indv0w==
=APjM
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/palmer/tags/riscv-for-master-4.0-rc3-v2' into staging
RISC-V Patches for 4.0-rc3, v2
This patch set contains a pair of tightly coupled PLIC bug fixes:
* We were calculating the PLIC addresses incorrectly.
* We were installing the wrong number of PLIC interrupts.
The two bugs togther resulted in a mostly-working system, but they're
impossible to seperate because fixing one bug would result in
significant breakage. As a result they're in the same patch.
There is also a cleanup to use qemu_log_mask(LOG_GUEST_ERROR,...) for
error reporting.
As far as I know these are the last outstanding RISC-V patches for 4.0.
v2 no longer fails "make check" for me... sorry!
# gpg: Signature made Fri 05 Apr 2019 01:33:57 BST
# gpg: using RSA key 00CE76D1834960DFCE886DF8EF4CA1502CCBAB41
# gpg: issuer "palmer@dabbelt.com"
# gpg: Good signature from "Palmer Dabbelt <palmer@dabbelt.com>" [unknown]
# gpg: aka "Palmer Dabbelt <palmer@sifive.com>" [unknown]
# gpg: WARNING: This key is not certified with a trusted signature!
# gpg: There is no indication that the signature belongs to the owner.
# Primary key fingerprint: 00CE 76D1 8349 60DF CE88 6DF8 EF4C A150 2CCB AB41
* remotes/palmer/tags/riscv-for-master-4.0-rc3-v2:
riscv: plic: Log guest errors
riscv: plic: Fix incorrect irq calculation
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Instead of using error_report() to print guest errors let's use
qemu_log_mask(LOG_GUEST_ERROR,...) to log the error.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
This patch fixes four different things, to maintain bisectability they
have been merged into a single patch. The following fixes are below:
sifive_plic: Fix incorrect irq calculation
The irq is incorrectly calculated to be off by one. It has worked in the
past as the priority_base offset has also been set incorrectly. We are
about to fix the priority_base offset so first first the irq
calculation.
sifive_u: Fix PLIC priority base offset and numbering
According to the FU540 manual the PLIC source priority address starts at
an offset of 0x04 and not 0x00. The same manual also specifies that the
PLIC only has 53 source priorities. Fix these two incorrect header
files.
We also need to over extend the plic_gpios[] array as the PLIC sources
count from 1 and not 0.
riscv: sifive_e: Fix PLIC priority base offset
According to the FE31 manual the PLIC source priority address starts at
an offset of 0x04 and not 0x00.
riscv: virt: Fix PLIC priority base offset
Update the virt offsets based on the newly updated SiFive U and SiFive E
offsets.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
The Xen blkif protocol requires that sector based quantities should be
interpreted strictly as multiples of 512 bytes. Specifically:
"first_sect and last_sect in blkif_request_segment, as well as
sector_number in blkif_request, are always expressed in 512-byte units."
Commit fcab2b464e "xen: add header and build dataplane/xen-block.c"
incorrectly modified behaviour to use the block device logical_block_size
property as the scale, instead of correctly shifting values by the
hardcoded BDRV_SECTOR_BITS (and hence scaling them to 512 byte units).
This patch undoes that change and restores compliance with the spec.
Furthermore, this patch also restores the original xen_disk behaviour
of advertizing a hardcoded 'sector-size' value of 512 in xenstore and
scaling 'sectors' accordingly. The realize() method is also modified to
fail if logical_block_size is set to anything other than 512.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Message-Id: <20190401121719.27208-1-paul.durrant@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
...and properly enable it when synthesizing a drive.
The Xen toolstack sets 'discard-enable' to '1' in xenstore when it wants
to enable discard on a specified image. The code in
xen_block_drive_create() correctly parses this and uses it to set
'discard' to 'unmap' for the file_layer, but fails to do the same for the
driver_layer (which effectively disables it). Meanwhile the code in
xen_block_realize() advertizes discard support to the frontend in the
default case (because conf->discard_granularity defaults to -1), even when
the underlying image may not handle it.
This patch adds the missing option to the driver_layer in
xen_block_driver_create() and checks whether BDRV_O_UNMAP is actually
set on the block device before advertizing discard to the frontend.
In the case that discard is supported it also makes sure that the
granularity is set to the physical block size.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Message-Id: <20190320142825.24565-1-paul.durrant@citrix.com>
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Compiling with GCC 9 complains
hw/s390x/3270-ccw.c: In function ‘emulated_ccw_3270_cb’:
hw/s390x/3270-ccw.c:81:19: error: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
81 | SCSW *s = &sch->curr_status.scsw;
| ^~~~~~~~~~~~~~~~~~~~~~
This local variable is only present to save a little bit of
typing when setting the field later. Get rid of this to avoid
the warning about unaligned accesses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190329111104.17223-15-berrange@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
Compiling with GCC 9 complains
hw/s390x/ipl.c: In function ‘s390_ipl_set_boot_menu’:
hw/s390x/ipl.c:256:25: warning: taking address of packed member of ‘struct QemuIplParameters’ may result in an unaligned pointer value [-Waddress-of-packed-member]
256 | uint32_t *timeout = &ipl->qipl.boot_menu_timeout;
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
This local variable is only present to save a little bit of
typing when setting the field later. Get rid of this to avoid
the warning about unaligned accesses.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190329111104.17223-14-berrange@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
The GCC 9 compiler complains about many places in s390 code
that take the address of members of the 'struct SCHIB' which
is marked packed:
hw/s390x/css.c: In function ‘sch_handle_clear_func’:
hw/s390x/css.c:698:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer val\
ue [-Waddress-of-packed-member]
698 | PMCW *p = &sch->curr_status.pmcw;
| ^~~~~~~~~~~~~~~~~~~~~~
hw/s390x/css.c:699:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer val\
ue [-Waddress-of-packed-member]
699 | SCSW *s = &sch->curr_status.scsw;
| ^~~~~~~~~~~~~~~~~~~~~~
...snip many more...
Almost all of these are just done for convenience to avoid
typing out long variable/field names when referencing struct
members. We can get most of this convenience by taking the
address of the 'struct SCHIB' instead, avoiding triggering
the compiler warnings.
In a couple of places we copy via a local variable which is
a technique already applied elsewhere in s390 code for this
problem.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190329111104.17223-13-berrange@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
The GCC 9 compiler complains about many places in s390 code
that take the address of members of the 'struct SCHIB' which
is marked packed:
hw/vfio/ccw.c: In function ‘vfio_ccw_io_notifier_handler’:
hw/vfio/ccw.c:133:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer value \
[-Waddress-of-packed-member]
133 | SCSW *s = &sch->curr_status.scsw;
| ^~~~~~~~~~~~~~~~~~~~~~
hw/vfio/ccw.c:134:15: warning: taking address of packed member of ‘struct SCHIB’ may result in an unaligned pointer value \
[-Waddress-of-packed-member]
134 | PMCW *p = &sch->curr_status.pmcw;
| ^~~~~~~~~~~~~~~~~~~~~~
...snip many more...
Almost all of these are just done for convenience to avoid
typing out long variable/field names when referencing struct
members. We can get most of this convenience by taking the
address of the 'struct SCHIB' instead, avoiding triggering
the compiler warnings.
In a couple of places we copy via a local variable which is
a technique already applied elsewhere in s390 code for this
problem.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20190329111104.17223-12-berrange@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
VTD_RTADDR_RTT is dropped even by the VT-d spec, so QEMU should
probably do the same thing (after all we never really implemented it).
Since we've had a field for that in the migration stream, to keep
compatibility we need to fill the hole up.
Please refer to VT-d spec 10.4.6.
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20190329061422.7926-3-peterx@redhat.com>
Reviewed-by: Liu, Yi L <yi.l.liu@intel.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
When introducing the initial support for scalable mode we added a
new field into vmstate however we blindly migrate that field without
notice. That'll break migration no matter forward or backward.
The normal way should be that we use something like
VMSTATE_UINT32_TEST() or subsections for the new vmstate field however
for this case of vt-d we can even make it simpler because we've
already migrated all the registers and it'll be fairly simple that we
re-generate root_scalable field from the register values during post
load of the device.
Fixes: fb43cf739e ("intel_iommu: scalable mode emulation")
Reviewed-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Message-Id: <20190329061422.7926-2-peterx@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
Message-Id: <20190321161832.10533-1-yuval.shaia@oracle.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
If we try to use the intel-iommu device with vfio-pci devices without
caching mode enabled, we're told:
qemu-system-x86_64: We need to set caching-mode=1 for intel-iommu to enable
device assignment with IOMMU protection.
But to enable caching mode, the option is actually "caching-mode=on".
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
Message-Id: <155364147432.16467.15898335025013220939.stgit@gimli.home>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Alex Williamson <<a href="mailto:alex.williamson@redhat.com" target="_blank" rel="noreferrer">alex.williamson@redhat.com</a>><br>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
The callers to bios_linker_find_file() assert that the file entry returned
is not NULL, except for those in bios_linker_loader_add_pointer(). Add two
asserts in that case for completeness and to facilitate static code analysis.
Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Message-Id: <1553199229-25318-1-git-send-email-liam.merwick@oracle.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Ensure watch IDs unique within a monitor and avoid integer wraparound
issues when many watches are set & unset over time.
-----BEGIN PGP SIGNATURE-----
iQIcBAABCAAGBQJco1tUAAoJEL6G67QVEE/f9xYP/2OTsAk6dHCcWqc///jXN0JW
OLcnpmjuh5KgUmOrs7NIRYRo2DARrhNDDXckHjDuX2HmnzoTB5p7DXLWMPgAzxj6
o83yxYT0Qqw79wxqbDmJ1y3kUVka1r94wCmmvUmxQZjLbDk8u2ytNUDbEOVlM0VJ
R7T800j8PhbY1pg9PiJJZonm7dxZAVNSCF4AgkrJVIes6MqhvNUhVsck5q/CC3yi
0jkglsiVTyXjKpJsioM9VhQXeqiHfFl3qjcPi0XQ99lVpTMFysNd4UdnVXhNs9NI
Xe52yQ7NG3fEggUivtFBmvVplNOg09K6U2vkyk9z8S1WTLZsbabwvwSvkVPW3Ty0
QaDgI6wNSiBb7Lfl2bbUR6X3j9QXUn8bhlczBsorS4uU4+NdHGyERsZLKLZazYYz
5t+xm4YBGJhtlt8IglNBjtaJFBuqgjrBEuBkfpevB2ocioWnU6APOdwWkKlSe3+r
EuKFUcbhRiRAGO05DjntNkePigb66IVkSiJXd4uSid/+akn5kQ19PSD8tXdfvZ7h
8HYTnWZa3OSLd7tZlwRzimnKuVSIsSnj+NobkIs8HSS3N7oN1t4FC5jH9D6GmK0R
djQs6a5mHQbonrc0dXxlb+P8u6I+ppxznPpfL/aQJ4pFM18V6gmcZEQluhwALNe9
rdPUnCudC/9SUNpMf3rI
=qriK
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/berrange/tags/filemon-next-pull-request' into staging
filemon: various fixes / improvements to file monitor for USB MTP
Ensure watch IDs unique within a monitor and avoid integer wraparound
issues when many watches are set & unset over time.
# gpg: Signature made Tue 02 Apr 2019 13:53:40 BST
# gpg: using RSA key BE86EBB415104FDF
# gpg: Good signature from "Daniel P. Berrange <dan@berrange.com>" [full]
# gpg: aka "Daniel P. Berrange <berrange@redhat.com>" [full]
# Primary key fingerprint: DAF3 A6FD B26B 6291 2D0E 8E3F BE86 EBB4 1510 4FDF
* remotes/berrange/tags/filemon-next-pull-request:
filemon: fix watch IDs to avoid potential wraparound issues
filemon: ensure watch IDs are unique to QFileMonitor scope
tests: refactor file monitor test to make it more understandable
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
- file-posix: Ignore unlock failure instead of crashing
- gluster: Limit the transfer size to 512 MiB
- stream: Fix backing chain freezing
- qemu-img: Enable BDRV_REQ_MAY_UNMAP for zero writes in convert
- iotests fixes
-----BEGIN PGP SIGNATURE-----
iQIcBAABAgAGBQJco1nvAAoJEH8JsnLIjy/WKSAP/AyAGt4DFbsfVq1SbX2MB/3F
QtLi3q3Yjy71OaNyMJsqgd8dxoqO1bWKC7MmLxwFZwvwmXhgq7EVbJ6vJ7kzuHw5
FMsVavm61mL/vsKypFieYHbahiKC31R7sw4Vl7yeKG02oDgi/0SnIQxxO0rmIZxG
Jx+p5tBDmrJ2WpgCWI0yr3VuYeJ/gJyw8la3Ubf1OzTBuTyfg94vSCsECX7GWm1w
0YkcRODct96mmqKCf6haHXQs9tTa6FnC9fsReenYcQg7nyqTttyXJz08fuqGQwWm
sNcmMp2/YxAg2Z85Aq21fmTkXbGopEHtoC2fg71WET1DzpqMvvJPFBIOneBSzUon
KHrZfZnziAzUESHDqenCQcHlbtWXxPY+7H0lojvhsLVzyEns6eLOSer093HqAh4s
+av55+ucTUYRl9Uk8/vFPdFBh0bNJRJB2yazp0Fk5Yoo8y70TXePQ8ZkpjH9VSpb
uQXy+FBc7LXYHB1KW/vhtuVoKgftbHi29rQ3imb9y6HYuJoJYZS9uGgrFpzF0YBN
/mBG17YqE8Imfuavt0WQqNLSub/km9eqFrejnMqDBka1hZblecOB+3Ujcud5C7Vk
HtKDdr2/Z+M1KApFMKhNZzEksXwhOVQ6VVZZfRGCjc/EXMOZ2gg8YlqOtotUzA0i
vV2wMgpqE2Ihx1gIIqw/
=a4LV
-----END PGP SIGNATURE-----
Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging
Block layer patches:
- file-posix: Ignore unlock failure instead of crashing
- gluster: Limit the transfer size to 512 MiB
- stream: Fix backing chain freezing
- qemu-img: Enable BDRV_REQ_MAY_UNMAP for zero writes in convert
- iotests fixes
# gpg: Signature made Tue 02 Apr 2019 13:47:43 BST
# gpg: using RSA key 7F09B272C88F2FD6
# gpg: Good signature from "Kevin Wolf <kwolf@redhat.com>" [full]
# Primary key fingerprint: DC3D EB15 9A9A F95D 3D74 56FE 7F09 B272 C88F 2FD6
* remotes/kevin/tags/for-upstream:
tests/qemu-iotests/235: Allow fallback to tcg
block: test block-stream with a base node that is used by block-commit
block: freeze the backing chain earlier in stream_start()
block: continue until base is found in bdrv_freeze_backing_chain() et al
block/file-posix: do not fail on unlock bytes
tests/qemu-iotests: Remove redundant COPYING file
block/gluster: limit the transfer size to 512 MiB
qemu-img: Enable BDRV_REQ_MAY_UNMAP in convert
iotests: Fix test 200 on s390x without virtio-pci
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Watch IDs are allocated from incrementing a int counter against
the QFileMonitor object. In very long life QEMU processes with
a huge amount of USB MTP activity creating & deleting directories
it is just about conceivable that the int counter can wrap
around. This would result in incorrect behaviour of the file
monitor watch APIs due to clashing watch IDs.
Instead of trying to detect this situation, this patch changes
the way watch IDs are allocated. It is turned into an int64_t
variable where the high 32 bits are set from the underlying
inotify "int" ID. This gives an ID that is guaranteed unique
for the directory as a whole, and we can rely on the kernel
to enforce this. QFileMonitor then sets the low 32 bits from
a per-directory counter.
The USB MTP device only sets watches on the directory as a
whole, not files within, so there is no risk of guest
triggered wrap around on the low 32 bits.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The watch IDs are mistakenly only unique within the scope of the
directory being monitored. This is not useful for clients which are
monitoring multiple directories. They require watch IDs to be unique
globally within the QFileMonitor scope.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Tested-by: Bandan Das <bsd@redhat.com>
Reviewed-by: Bandan Das <bsd@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The current file monitor unit tests are too clever for their own good
making it hard to understand the desired output.
Instead of trying to infer the expected events, explicitly list the
events we expect in the operation sequence.
Instead of dynamically building a matrix of tests, just have one giant
operation sequence that validates all scenarios in a single test.
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When the user specifies a list of accelerators, we pick the first one
that initializes successfully. Recent commit 1a3ec8c156 broke that.
Reproducer:
$ qemu-system-x86_64 --machine accel=xen:tcg
xencall: error: Could not obtain handle on privileged command interface: No such file or directory
xen be core: xen be core: can't open xen interface
can't open xen interface
qemu-system-x86_64: failed to initialize Xen: Operation not permitted
qemu-system-x86_64: /home/armbru/work/qemu/qom/object.c:436: object_set_accelerator_compat_props: Assertion `!object_compat_props[0]' failed.
Root cause: we register accelerator compat properties even when the
accelerator fails. The failed assertion is
object_set_accelerator_compat_props() telling us off. Fix by calling
it only for the accelerator that succeeded.
Fixes: 1a3ec8c156
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20190401090827.20793-6-armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190401090827.20793-5-armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
migrate_add_blocker() asserts we have a current_migration object, in
migrate_get_current(). We do only after migration_object_init().
This contributes to the following dependency cycle:
* configure_blockdev() must run before machine_set_property()
so machine properties can refer to block backends
* machine_set_property() before configure_accelerator()
so machine properties like kvm-irqchip get applied
* configure_accelerator() before migration_object_init()
so that Xen's accelerator compat properties get applied.
* migration_object_init() before configure_blockdev()
so configure_blockdev() can add migration blockers
The cycle was closed when recent commit cda4aa9a5a "Create block
backends before setting machine properties" added the first
dependency, and satisfied it by violating the last one. Broke block
backends that add migration blockers, as demonstrated by qemu-iotests
055.
To fix it, break the last dependency: make migrate_add_blocker()
usable before migration_object_init().
The previous commit already removed the use of migrate_get_current()
from migrate_add_blocker() itself. Didn't quite do the trick, as
there's another one hiding in migration_is_idle().
The use there isn't actually necessary: when no migration object has
been created yet, migration is surely idle. Make migration_is_idle()
return true then.
Fixes: cda4aa9a5a
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190401090827.20793-4-armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
This reverts commit 3df663e575.
This reverts commit b605c47b57.
Command line option --only-migratable is for disallowing any
configuration that can block migration.
Initially, --only-migratable set global variable @only_migratable.
Commit 3df663e575 "migration: move only_migratable to MigrationState"
replaced it by MigrationState member @only_migratable. That was a
mistake.
First, it doesn't make sense on the design level. MigrationState
captures the state of an individual migration, but --only-migratable
isn't a property of an individual migration, it's a restriction on
QEMU configuration. With fault tolerance, we could have several
migrations at once. --only-migratable would certainly protect all of
them. Storing it in MigrationState feels inappropriate.
Second, it contributes to a dependency cycle that manifests itself as
a bug now.
Putting @only_migratable into MigrationState means its available only
after migration_object_init().
We can't set it before migration_object_init(), so we delay setting it
with a global property (this is fixup commit b605c47b57 "migration:
fix handling for --only-migratable").
We can't get it before migration_object_init(), so anything that uses
it can only run afterwards.
Since migrate_add_blocker() needs to obey --only-migratable, any code
adding migration blockers can run only afterwards. This contributes
to the following dependency cycle:
* configure_blockdev() must run before machine_set_property()
so machine properties can refer to block backends
* machine_set_property() before configure_accelerator()
so machine properties like kvm-irqchip get applied
* configure_accelerator() before migration_object_init()
so that Xen's accelerator compat properties get applied.
* migration_object_init() before configure_blockdev()
so configure_blockdev() can add migration blockers
The cycle was closed when recent commit cda4aa9a5a "Create block
backends before setting machine properties" added the first
dependency, and satisfied it by violating the last one. Broke block
backends that add migration blockers.
Moving @only_migratable into MigrationState was a mistake. Revert it.
This doesn't quite break the "migration_object_init() before
configure_blockdev() dependency, since migrate_add_blocker() still has
another dependency on migration_object_init(). To be addressed the
next commit.
Note that the reverted commit made -only-migratable sugar for -global
migration.only-migratable=on below the hood. Documentation has only
ever mentioned -only-migratable. This commit removes the arcane &
undocumented alternative to -only-migratable again. Nobody should be
using it.
Conflicts:
include/migration/misc.h
migration/migration.c
migration/migration.h
vl.c
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20190401090827.20793-3-armbru@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
