Re: [Qemu-devel] [Qemu-trivial] [PATCH v3] ivshmem-server: ivshmem-client: Build when eventfd() is available
Kamil Rytarowski writes: > On 05.06.2017 16:29, Michael Tokarev wrote: >> 31.05.2017 15:00, Kamil Rytarowski wrote: >>> Currently ivshmem requires eventfd() which is Linux specific. >>> Do not and build it unconditionally on every Linux/BSD/Solaris. >>> >>> This patch indirectly fixes build failure on NetBSD, where these tools >>> additionally require -lrt for shm_open(3). In future there should be >>> added support for NetBSD and the linking addressed appropriately. >> >> Unfortunately this breaks static build. >> >> $ ../configure --disable-system --disable-linux-user --static >> $ make V=1 >> ... >> c++ -I/usr/include/pixman-1 -Werror -pthread -I/usr/include/glib-2.0 >> -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE >> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes >> -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes >> -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels >> -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body >> -Wnested-externs >> -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers >> -Wold-style-declaration -Wold-style-definition -Wtype-limits >> -fstack-protector-strong >> -I/usr/include/libpng16 -I/build/qemu/git/tests -O2 -U_FORTIFY_SOURCE >> -D_FORTIFY_SOURCE=2 -g -Wl,--warn-common -m64 -static -g -o ivshmem-server >> libqemuutil.a libqemustub.a -lm -lgthread-2.0 -pthread -lglib-2.0 -pthread >> -lpcre -pthread -lz -lrt -lz -lnettle -lutil >> /usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/crt1.o: In >> function `_start': >> (.text+0x20): undefined reference to `main' >> collect2: error: ld returned 1 exit status >> Makefile:475: error building «ivshmem-server» >> $ _ >> >> I'll take a look at this later. reverting it for now. >> >> Thanks, >> >> /mjt >> > > I will try to reproduce it locally. I haven't, but here's a quick guess on what we need to squash into the patch: diff --git a/Makefile b/Makefile index c830d7a..2ef5a78 100644 --- a/Makefile +++ b/Makefile @@ -469,10 +469,12 @@ ifneq ($(EXESUF),) qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER) $(QEMU_GA_MSI) endif +ifdef CONFIG_IVSHMEM ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) $(COMMON_LDADDS) $(call LINK, $^) ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) $(COMMON_LDADDS) $(call LINK, $^) +endif module_block.h: $(SRC_PATH)/scripts/modules/module_block.py config-host.mak $(call quiet-command,$(PYTHON) $< $@ \ diff --git a/tests/Makefile.include b/tests/Makefile.include index f42f3df..ab70d01 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -203,7 +203,7 @@ gcov-files-pci-y += hw/display/virtio-gpu-pci.c gcov-files-pci-$(CONFIG_VIRTIO_VGA) += hw/display/virtio-vga.c check-qtest-pci-y += tests/intel-hda-test$(EXESUF) gcov-files-pci-y += hw/audio/intel-hda.c hw/audio/hda-codec.c -check-qtest-pci-$(CONFIG_EVENTFD) += tests/ivshmem-test$(EXESUF) +check-qtest-pci-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF) gcov-files-pci-y += hw/misc/ivshmem.c check-qtest-i386-y = tests/endianness-test$(EXESUF)
Re: [Qemu-devel] [PATCH v2 2/3] tests: use QEMU_CACHELINE_SIZE instead of hard-coding it
On Mon, Jun 5, 2017 at 6:49 PM, Emilio G. Cota wrote: > Signed-off-by: Emilio G. Cota Reviewed-by: Pranith Kumar > --- > tests/atomic_add-bench.c | 4 ++-- > tests/qht-bench.c| 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c > index caa1e8e..c219109 100644 > --- a/tests/atomic_add-bench.c > +++ b/tests/atomic_add-bench.c > @@ -5,11 +5,11 @@ > > struct thread_info { > uint64_t r; > -} QEMU_ALIGNED(64); > +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); > > struct count { > unsigned long val; > -} QEMU_ALIGNED(64); > +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); > > static QemuThread *threads; > static struct thread_info *th_info; > diff --git a/tests/qht-bench.c b/tests/qht-bench.c > index 2afa09d..3f4b5eb 100644 > --- a/tests/qht-bench.c > +++ b/tests/qht-bench.c > @@ -28,7 +28,7 @@ struct thread_info { > uint64_t r; > bool write_op; /* writes alternate between insertions and removals */ > bool resize_down; > -} QEMU_ALIGNED(64); /* avoid false sharing among threads */ > +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); /* avoid false sharing among threads */ > > static struct qht ht; > static QemuThread *rw_threads; > -- > 2.7.4 > -- Pranith
Re: [Qemu-devel] [PATCH v2 1/3] compiler: define QEMU_CACHELINE_SIZE
On Mon, Jun 5, 2017 at 6:49 PM, Emilio G. Cota wrote: > This is a constant used as a hint for padding structs to hopefully avoid > false cache line sharing. > > The constant can be set at configure time by defining QEMU_CACHELINE_SIZE > via --extra-cflags. If not set there, we try to obtain the value from > the machine running the configure script. If we fail, we default to > reasonable values, i.e. 128 bytes for ppc64 and 64 bytes for all others. > > Note: the configure script only picks up the cache line size when run > on Linux hosts because I have no other platforms (e.g. Windows, BSD's) > to test on. > > Signed-off-by: Emilio G. Cota > --- > configure | 38 ++ > include/qemu/compiler.h | 17 + > 2 files changed, 55 insertions(+) > > diff --git a/configure b/configure > index 13e040d..6a68cb2 100755 > --- a/configure > +++ b/configure > @@ -4832,6 +4832,41 @@ EOF >fi > fi > > +# Find out the size of a cache line on the host > +# TODO: support more platforms > +cat > $TMPC< +#ifdef __linux__ > + > +#include > + > +#define SYSFS "/sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size" > + > +int main(int argc, char *argv[]) > +{ > +unsigned int size; > +FILE *fp; > + > +fp = fopen(SYSFS, "r"); > +if (fp == NULL) { > +return -1; > +} > +if (!fscanf(fp, "%u", &size)) { > +return -1; > +} > +return size; > +} > +#else > +#error Cannot find host cache line size > +#endif > +EOF Is there any reason not to use sysconf(_SC_LEVEL1_DCACHE_LINESIZE)? Thanks, -- Pranith
Re: [Qemu-devel] [PATCH v2 3/3] tcg: allocate TB structs before the corresponding translated code
On Mon, Jun 5, 2017 at 6:49 PM, Emilio G. Cota wrote: > Allocating an arbitrarily-sized array of tbs results in either > (a) a lot of memory wasted or (b) unnecessary flushes of the code > cache when we run out of TB structs in the array. > > An obvious solution would be to just malloc a TB struct when needed, > and keep the TB array as an array of pointers (recall that tb_find_pc() > needs the TB array to run in O(log n)). > > Perhaps a better solution, which is implemented in this patch, is to > allocate TB's right before the translated code they describe. This > results in some memory waste due to padding to have code and TBs in > separate cache lines--for instance, I measured 4.7% of padding in the > used portion of code_gen_buffer when booting aarch64 Linux on a > host with 64-byte cache lines. However, it can allow for optimizations > in some host architectures, since TCG backends could safely assume that > the TB and the corresponding translated code are very close to each > other in memory. See this message by rth for a detailed explanation: > > https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg05172.html > Subject: Re: GSoC 2017 Proposal: TCG performance enhancements > Message-ID: <1e67644b-4b30-887e-d329-1848e94c9...@twiddle.net> Reviewed-by: Pranith Kumar Thanks for doing this Emilio. Do you plan to continue working on rth's suggestions in that email? If so, can we co-ordinate our work? -- Pranith
[Qemu-devel] [PATCH v2 0/1] qemu/migration: fix the migration bug found by qemu-iotests case 068
Hi all, This patch is to fix the migration bug found by qemu-iotests case 068 and based on upstream master's commit: 199e19ee53: Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging. The bug was introduced by commit "660819b migration: shut src return path unconditionally". Change Log(v2): Got reviewed-by from Dr. David Alan Gilbert and Peter Xu. Thanks! QingFeng Hao (1): qemu/migration: fix the double free problem on from_src_file migration/savevm.c | 1 - 1 file changed, 1 deletion(-) -- 2.11.2
[Qemu-devel] [PATCH v2 1/1] qemu/migration: fix the double free problem on from_src_file
In load_snapshot, mis->from_src_file is freed twice, the first free is by qemu_fclose, the second is by migration_incoming_state_destroy and it causes Illegal instruction exception. The fix is just to remove the first free. This problem is found by qemu-iotests case 068 since commit "660819b migration: shut src return path unconditionally". The error is: 068 1s ... - output mismatch (see 068.out.bad) --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done Signed-off-by: QingFeng Hao Reviewed-by: Dr. David Alan Gilbert Reviewed-by: Peter Xu --- migration/savevm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f59d0..853e14e34e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); -qemu_fclose(f); aio_context_release(aio_context); migration_incoming_state_destroy(); -- 2.11.2
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:50, Peter Xu 写道: On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote: 在 2017/6/6 11:03, Peter Xu 写道: On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: In load_vmstate, mis->from_src_file is freed twice, the first free is by qemu_fclose, the second is by migration_incoming_state_destroy and it causes Illegal instruction exception. The fix is just to remove the first free. This problem is found by qemu-iotests case 068 since commit "660819b migration: shut src return path unconditionally". The error is: 068 1s ... - output mismatch (see 068.out.bad) --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done Signed-off-by: QingFeng Hao --- migration/savevm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f59d0..853e14e34e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); -qemu_fclose(f); aio_context_release(aio_context); migration_incoming_state_destroy(); Thanks QingFeng for the fix! Reviewed-by: Peter Xu Though here instead of removing the fclose(), not sure whether this is nicer: diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..1feb519 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) QEMUFile *f; int ret; AioContext *aio_context; -MigrationIncomingState *mis = migration_incoming_get_current(); if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) } qemu_system_reset(SHUTDOWN_CAUSE_NONE); -mis->from_src_file = f; aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); Thanks Peter. I have a doubt on this change because I see there are several places referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it seems to get byte from the file and it's called by qemu_loadvm_state. Anyway, you remind me for the description that is "In load_snapshot" rather than "In load_vmstate". thanks Oh I didn't really notice that. :) It shouldn't affect imho since load snapshot won't trigger postcopy codes. But sure current fix is good enough for me at least. Ok, thanks! Thanks, -- Regards QingFeng Hao
Re: [Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks
On 06/05/2017 04:38 PM, Eric Blake wrote: > I found a crasher and some odd behavior while rebasing my > bdrv_get_block_status series, so I figured I'd get these things > fixed first. This is based on top of Max's block branch. > > Available as a tag at: > git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4 > > Since v3: > - check all qemu-iotests (patch 1) > > 001/4:[0012] [FC] 'qemu-io: Don't die on second open' > 002/4:[] [--] 'block: Guarantee that *file is set on > bdrv_get_block_status()' > 003/4:[] [--] 'block: Simplify use of BDRV_BLOCK_RAW' > 004/4:[] [--] 'blkdebug: Support .bdrv_co_get_block_status' > > Eric Blake (4): > qemu-io: Don't die on second open > block: Guarantee that *file is set on bdrv_get_block_status() > block: Simplify use of BDRV_BLOCK_RAW > blkdebug: Support .bdrv_co_get_block_status > > include/block/block.h | 6 +++--- > block/blkdebug.c | 11 +++ > block/commit.c | 2 +- > block/io.c | 5 +++-- > block/mirror.c | 2 +- > block/raw-format.c | 2 +- > block/vpc.c| 2 +- > qemu-io.c | 7 --- > tests/qemu-iotests/060.out | 1 + > tests/qemu-iotests/114.out | 5 +++-- > tests/qemu-iotests/153.out | 6 ++ > tests/qemu-iotests/177 | 3 +++ > tests/qemu-iotests/177.out | 5 + > 13 files changed, 43 insertions(+), 14 deletions(-) > 3,4: Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote: > > > 在 2017/6/6 11:03, Peter Xu 写道: > >On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: > >>In load_vmstate, mis->from_src_file is freed twice, the first free is by > >>qemu_fclose, the second is by migration_incoming_state_destroy and > >>it causes Illegal instruction exception. The fix is just to remove the > >>first free. > >> > >>This problem is found by qemu-iotests case 068 since commit > >>"660819b migration: shut src return path unconditionally". The error is: > >>068 1s ... - output mismatch (see 068.out.bad) > >> --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 > >> +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200 > >> @@ -6,6 +6,8 @@ > >> QEMU X.Y.Z monitor - type 'help' for more information > >> (qemu) savevm 0 > >> (qemu) quit > >> +./common.config: line 107: 242472 Illegal instruction (core > >> dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then > >> +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > >> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > >> QEMU X.Y.Z monitor - type 'help' for more information > >> -(qemu) quit > >> -*** done > >> +(qemu) *** done > >> > >>Signed-off-by: QingFeng Hao > >>--- > >> migration/savevm.c | 1 - > >> 1 file changed, 1 deletion(-) > >> > >>diff --git a/migration/savevm.c b/migration/savevm.c > >>index 9c320f59d0..853e14e34e 100644 > >>--- a/migration/savevm.c > >>+++ b/migration/savevm.c > >>@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) > >> aio_context_acquire(aio_context); > >> ret = qemu_loadvm_state(f); > >>-qemu_fclose(f); > >> aio_context_release(aio_context); > >> migration_incoming_state_destroy(); > >Thanks QingFeng for the fix! > > > >Reviewed-by: Peter Xu > > > >Though here instead of removing the fclose(), not sure whether this is > >nicer: > > > >diff --git a/migration/savevm.c b/migration/savevm.c > >index 9c320f5..1feb519 100644 > >--- a/migration/savevm.c > >+++ b/migration/savevm.c > >@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) > > QEMUFile *f; > > int ret; > > AioContext *aio_context; > >-MigrationIncomingState *mis = migration_incoming_get_current(); > > > > if (!bdrv_all_can_snapshot(&bs)) { > > error_setg(errp, > >@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) > > } > > > > qemu_system_reset(SHUTDOWN_CAUSE_NONE); > >-mis->from_src_file = f; > > > > aio_context_acquire(aio_context); > > ret = qemu_loadvm_state(f); > Thanks Peter. I have a doubt on this change because I see there are several > places > referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it > seems to > get byte from the file and it's called by qemu_loadvm_state. > Anyway, you remind me for the description that is "In load_snapshot" rather > than > "In load_vmstate". thanks Oh I didn't really notice that. :) It shouldn't affect imho since load snapshot won't trigger postcopy codes. But sure current fix is good enough for me at least. Thanks, -- Peter Xu
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/6 11:03, Peter Xu 写道: On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: In load_vmstate, mis->from_src_file is freed twice, the first free is by qemu_fclose, the second is by migration_incoming_state_destroy and it causes Illegal instruction exception. The fix is just to remove the first free. This problem is found by qemu-iotests case 068 since commit "660819b migration: shut src return path unconditionally". The error is: 068 1s ... - output mismatch (see 068.out.bad) --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done Signed-off-by: QingFeng Hao --- migration/savevm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f59d0..853e14e34e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); -qemu_fclose(f); aio_context_release(aio_context); migration_incoming_state_destroy(); Thanks QingFeng for the fix! Reviewed-by: Peter Xu Though here instead of removing the fclose(), not sure whether this is nicer: diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..1feb519 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) QEMUFile *f; int ret; AioContext *aio_context; -MigrationIncomingState *mis = migration_incoming_get_current(); if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) } qemu_system_reset(SHUTDOWN_CAUSE_NONE); -mis->from_src_file = f; aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); Thanks Peter. I have a doubt on this change because I see there are several places referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it seems to get byte from the file and it's called by qemu_loadvm_state. Anyway, you remind me for the description that is "In load_snapshot" rather than "In load_vmstate". thanks Since I see we setup from_src_file but we don't really use it. If we remove that line, we can also remove the migration_incoming_get_current() call altogether. Either way work for me. So my r-b stands always. :-) -- Regards QingFeng Hao
Re: [Qemu-devel] [PATCH v1] virtio-net: enable configurable tx queue size
On 06/05/2017 11:38 PM, Michael S. Tsirkin wrote: On Mon, Jun 05, 2017 at 04:57:29PM +0800, Wei Wang wrote: /* * Calculate the number of bytes up to and including the given 'field' of @@ -57,6 +62,8 @@ static VirtIOFeature feature_sizes[] = { .end = endof(struct virtio_net_config, max_virtqueue_pairs)}, {.flags = 1 << VIRTIO_NET_F_MTU, .end = endof(struct virtio_net_config, mtu)}, +{.flags = 1 << VIRTIO_F_MAX_CHAIN_SIZE, + .end = endof(struct virtio_net_config, max_chain_size)}, {} }; Using a generic VIRTIO_F_MAX_CHAIN_SIZE flag, so this should go into the generic virtio section, not virtio_net_config. Do you mean VIRTIO_PCI_xx under virtio_pci.h? The reason that the config field wasn't added there is because I didn't find space to put into the new 16-bit field into the existing layout. The last one is "#define VIRTIO_MSI_QUEUE_VECTOR 22". The per-driver configuration space directly follows that last register by "#define VIRTIO_PCI_CONFIG_OFF(msix_enabled) ((msix_enabled) ? 24:20)" Changing the MACRO to something like VIRTIO_PCI_CONFIG_OFF(max_chain_size_enabled) would be difficult. Do you see a good way to add the new field here? @@ -84,6 +91,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) virtio_stw_p(vdev, &netcfg.status, n->status); virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues); virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu); +virtio_stw_p(vdev, &netcfg.max_chain_size, VIRTIO_NET_MAX_CHAIN_SIZE); memcpy(netcfg.mac, n->mac, ETH_ALEN); memcpy(config, &netcfg, n->config_size); } @@ -635,9 +643,33 @@ static inline uint64_t virtio_net_supported_guest_offloads(VirtIONet *n) return virtio_net_guest_offloads_by_features(vdev->guest_features); } +static bool is_tx(int queue_index) +{ +return queue_index % 2 == 1; +} + +static void virtio_net_reconfig_tx_queue_size(VirtIONet *n) +{ +VirtIODevice *vdev = VIRTIO_DEVICE(n); +int i, num_queues = virtio_get_num_queues(vdev); + +/* Return when the queue size is already less than the 1024 */ +if (n->net_conf.tx_queue_size < VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE) { +return; +} + +for (i = 0; i < num_queues; i++) { +if (is_tx(i)) { +n->net_conf.tx_queue_size = VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE / 2; +virtio_queue_set_num(vdev, i, n->net_conf.tx_queue_size); +} +} +} + static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) { VirtIONet *n = VIRTIO_NET(vdev); +NetClientState *nc = qemu_get_queue(n->nic); int i; virtio_net_set_multiqueue(n, @@ -649,6 +681,16 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) virtio_has_feature(features, VIRTIO_F_VERSION_1)); +/* + * When the traditional QEMU backend is used, using 1024 tx queue size + * requires the driver to support the VIRTIO_F_MAX_CHAIN_SIZE feature. If + * the feature is not supported, reconfigure the tx queue size to 512. + */ +if (!get_vhost_net(nc->peer) && +!virtio_has_feature(features, VIRTIO_F_MAX_CHAIN_SIZE)) { +virtio_net_reconfig_tx_queue_size(n); +} + if (n->has_vnet_hdr) { n->curr_guest_offloads = virtio_net_guest_offloads_by_features(features); @@ -1297,8 +1339,8 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q) out_num = elem->out_num; out_sg = elem->out_sg; -if (out_num < 1) { -virtio_error(vdev, "virtio-net header not in first element"); +if (out_num < 1 || out_num > VIRTIO_NET_MAX_CHAIN_SIZE) { +virtio_error(vdev, "no packet or too large vring desc chain"); virtqueue_detach_element(q->tx_vq, elem, 0); g_free(elem); return -EINVAL; what about rx vq? we need to limit that as well, do we not? I think the rx vq doesn't need the limit, because there is no off-by-one issue like the tx side writev(). diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h index b777069..b70cbfe 100644 --- a/include/standard-headers/linux/virtio_config.h +++ b/include/standard-headers/linux/virtio_config.h @@ -60,6 +60,9 @@ #define VIRTIO_F_ANY_LAYOUT 27 #endif /* VIRTIO_CONFIG_NO_LEGACY */ +/* Guest chains vring descriptors within a limit */ +#define VIRTIO_F_MAX_CHAIN_SIZE31 + Pls use a flag in the high 32 bit. I think this should be considered as a transport feature. How about changing VIRTIO_TRANSPORT_F_END to 35 (was 34), and use 34 for VIRTIO_F_MAX_CHAIN_SIZE? Best, Wei
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
在 2017/6/5 19:08, Dr. David Alan Gilbert 写道: * QingFeng Hao (ha...@linux.vnet.ibm.com) wrote: In load_vmstate, mis->from_src_file is freed twice, the first free is by qemu_fclose, the second is by migration_incoming_state_destroy and it causes Illegal instruction exception. The fix is just to remove the first free. This problem is found by qemu-iotests case 068 since commit "660819b migration: shut src return path unconditionally". The error is: 068 1s ... - output mismatch (see 068.out.bad) --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200 +++ 068.out.bad2017-06-03 13:59:55.360274640 +0200 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 242472 Illegal instruction (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done Signed-off-by: QingFeng Hao --- migration/savevm.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f59d0..853e14e34e 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); -qemu_fclose(f); aio_context_release(aio_context); Thanks! Reviewed-by: Dr. David Alan Gilbert Thanks David! migration_incoming_state_destroy(); -- 2.11.2 -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK -- Regards QingFeng Hao
Re: [Qemu-devel] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file
On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote: > In load_vmstate, mis->from_src_file is freed twice, the first free is by > qemu_fclose, the second is by migration_incoming_state_destroy and > it causes Illegal instruction exception. The fix is just to remove the > first free. > > This problem is found by qemu-iotests case 068 since commit > "660819b migration: shut src return path unconditionally". The error is: > 068 1s ... - output mismatch (see 068.out.bad) > --- tests/qemu-iotests/068.out2017-05-06 01:00:26.417270437 +0200 > +++ 068.out.bad 2017-06-03 13:59:55.360274640 +0200 > @@ -6,6 +6,8 @@ > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) savevm 0 > (qemu) quit > +./common.config: line 107: 242472 Illegal instruction (core dumped) > ( if [ -n "${QEMU_NEED_PID}" ]; then > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) quit > -*** done > +(qemu) *** done > > Signed-off-by: QingFeng Hao > --- > migration/savevm.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 9c320f59d0..853e14e34e 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp) > > aio_context_acquire(aio_context); > ret = qemu_loadvm_state(f); > -qemu_fclose(f); > aio_context_release(aio_context); > > migration_incoming_state_destroy(); Thanks QingFeng for the fix! Reviewed-by: Peter Xu Though here instead of removing the fclose(), not sure whether this is nicer: diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..1feb519 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp) QEMUFile *f; int ret; AioContext *aio_context; -MigrationIncomingState *mis = migration_incoming_get_current(); if (!bdrv_all_can_snapshot(&bs)) { error_setg(errp, @@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp) } qemu_system_reset(SHUTDOWN_CAUSE_NONE); -mis->from_src_file = f; aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); Since I see we setup from_src_file but we don't really use it. If we remove that line, we can also remove the migration_incoming_get_current() call altogether. Either way work for me. So my r-b stands always. :-) -- Peter Xu
[Qemu-devel] [PULL 13/17] spapr: Introduce DRC subclasses
Currently we only have a single QOM type for all DRCs, but lots of places where we switch behaviour based on the DRC's PAPR defined type. This is a poor use of our existing type system. So, instead create QOM subclasses for each PAPR defined DRC type. We also introduce intermediate subclasses for physical and logical DRCs, a division which will be useful later on. Instead of being stored in the DRC object itself, the PAPR type is now stored in the class structure. There are still many places where we switch directly on the PAPR type value, but this at least provides the basis to start to remove those. Signed-off-by: David Gibson Reviewed-by: Michael Roth Acked-by: Michael Roth --- hw/ppc/spapr.c | 5 +- hw/ppc/spapr_drc.c | 123 + hw/ppc/spapr_pci.c | 3 +- include/hw/ppc/spapr_drc.h | 47 +++-- 4 files changed, 139 insertions(+), 39 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index c1c0951..11a04d1 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1912,7 +1912,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) uint64_t addr; addr = i * lmb_size + spapr->hotplug_memory.base; -drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB, +drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB, addr/lmb_size); qemu_register_reset(spapr_drc_reset, drc); } @@ -2009,8 +2009,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) if (mc->has_hotpluggable_cpus) { sPAPRDRConnector *drc = -spapr_dr_connector_new(OBJECT(spapr), - SPAPR_DR_CONNECTOR_TYPE_CPU, +spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU, (core_id / smp_threads) * smt); qemu_register_reset(spapr_drc_reset, drc); diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index c1d4b10..969d727 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type) return shift; } +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc) +{ +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); + +return 1 << drck->typeshift; +} + uint32_t spapr_drc_index(sPAPRDRConnector *drc) { +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); + /* no set format for a drc index: it only needs to be globally * unique. this is how we encode the DRC type on bare-metal * however, so might as well do that here */ -return (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) | -(drc->id & DRC_INDEX_ID_MASK); +return (drck->typeshift << DRC_INDEX_TYPE_SHIFT) +| (drc->id & DRC_INDEX_ID_MASK); } static uint32_t set_isolation_state(sPAPRDRConnector *drc, @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, * If the LMB being removed doesn't belong to a DIMM device that is * actually being unplugged, fail the isolation request here. */ -if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) { +if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) { if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) && !drc->awaiting_release) { return RTAS_OUT_HW_ERROR; @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, } } -if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) { +if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { drc->allocation_state = state; if (drc->awaiting_release && drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, return RTAS_OUT_SUCCESS; } -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc) -{ -return drc->type; -} - static const char *get_name(sPAPRDRConnector *drc) { return drc->name; @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc) static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state) { if (drc->dev) { -if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI && +if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI && drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { /* for logical DR, we return a state of UNUSABLE * iff the allocation state UNUSABLE. @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense *state) *state = SPAPR_DR_ENTITY_SENSE_PRESENT; } } else { -if (drc->type == SPAPR_DR_CONNECTOR_TYPE_PCI) { +if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) { /* PCI devices, and only PCI devices, use
Re: [Qemu-devel] [PATCH 0/7] KVM: MMU: fast write protect
On 06/05/2017 03:36 PM, Jay Zhou wrote: /* enable ucontrol for s390 */ struct kvm_s390_ucas_mapping { diff --git a/memory.c b/memory.c index 4c95aaf..b836675 100644 --- a/memory.c +++ b/memory.c @@ -809,6 +809,13 @@ static void address_space_update_ioeventfds(AddressSpace *as) flatview_unref(view); } +static write_protect_all_fn write_func; I think there should be a declaration in memory.h, diff --git a/include/exec/memory.h b/include/exec/memory.h index 7fc3f48..31f3098 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1152,6 +1152,9 @@ void memory_global_dirty_log_start(void); */ void memory_global_dirty_log_stop(void); +typedef void (*write_protect_all_fn)(bool write); +void memory_register_write_protect_all(write_protect_all_fn func); + void mtree_info(fprintf_function mon_printf, void *f); Thanks for your suggestion, Jay! This code just demonstrates how to enable this feature in QEMU, i will carefully consider it and merger your suggestion when the formal patch is posted out.
[Qemu-devel] [PULL 06/17] spapr: Make DRC get_index and get_type methods into plain functions
These two methods only have one implementation, and the spec they're implementing means any other implementation is unlikely, verging on impossible. So replace them with simple functions. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier Tested-by: Daniel Barboza --- hw/ppc/spapr.c | 13 +++-- hw/ppc/spapr_drc.c | 66 ++ hw/ppc/spapr_events.c | 10 +++ hw/ppc/spapr_pci.c | 4 +-- include/hw/ppc/spapr_drc.h | 5 ++-- 5 files changed, 44 insertions(+), 54 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ab3aab1..5d10366 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -456,15 +456,13 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)}; int compat_smt = MIN(smp_threads, ppc_compat_max_threads(cpu)); sPAPRDRConnector *drc; -sPAPRDRConnectorClass *drck; int drc_index; uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ]; int i; drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index); if (drc) { -drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -drc_index = drck->get_index(drc); +drc_index = spapr_drc_index(drc); _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index))); } @@ -654,15 +652,13 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt) if (i >= hotplug_lmb_start) { sPAPRDRConnector *drc; -sPAPRDRConnectorClass *drck; drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i); g_assert(drc); -drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); dynamic_memory[0] = cpu_to_be32(addr >> 32); dynamic_memory[1] = cpu_to_be32(addr & 0x); -dynamic_memory[2] = cpu_to_be32(drck->get_index(drc)); +dynamic_memory[2] = cpu_to_be32(spapr_drc_index(drc)); dynamic_memory[3] = cpu_to_be32(0); /* reserved */ dynamic_memory[4] = cpu_to_be32(numa_get_node(addr, NULL)); if (memory_region_present(get_system_memory(), addr)) { @@ -2560,7 +2556,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs, - drck->get_index(drc)); + spapr_drc_index(drc)); } else { spapr_hotplug_req_add_by_count(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs); @@ -2770,8 +2766,7 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, addr_start / SPAPR_MEMORY_BLOCK_SIZE); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB, - nr_lmbs, - drck->get_index(drc)); + nr_lmbs, spapr_drc_index(drc)); out: error_propagate(errp, local_err); } diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 693571a..a35314e 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -70,7 +70,7 @@ static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type) return shift; } -static uint32_t get_index(sPAPRDRConnector *drc) +uint32_t spapr_drc_index(sPAPRDRConnector *drc) { /* no set format for a drc index: it only needs to be globally * unique. this is how we encode the DRC type on bare-metal @@ -85,7 +85,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, { sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -trace_spapr_drc_set_isolation_state(get_index(drc), state); +trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state); if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) { /* cannot unisolate a non-existent resource, and, or resources @@ -126,11 +126,12 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, * PAPR+ 2.7, 13.4 */ if (drc->awaiting_release) { +uint32_t drc_index = spapr_drc_index(drc); if (drc->configured) { -trace_spapr_drc_set_isolation_state_finalizing(get_index(drc)); +trace_spapr_drc_set_isolation_state_finalizing(drc_index); drck->detach(drc, DEVICE(drc->dev), NULL); } else { -trace_spapr_drc_set_isolation_state_deferring(get_index(drc)); +trace_spapr_drc_set_isolation_state_deferring(drc_index); } } drc->configured = false; @@ -142,7 +143
[Qemu-devel] [PULL 11/17] spapr: Allow boot from vhost-*-scsi backends
From: Felipe Franciosi The current implementation of spapr_get_fw_dev_path() doesn't take into consideration vhost-*-scsi devices. This makes said devices unbootable on PPC as SLOF is unable to work out the path to scan boot disks. This makes VMs bootable on spapr when using vhost-*-scsi by implementing a disk path for VHostSCSICommon (which currently includes both vhost-user-scsi and vhost-scsi). Signed-off-by: Felipe Franciosi Signed-off-by: Mike Cui Signed-off-by: David Gibson --- hw/ppc/spapr.c | 8 1 file changed, 8 insertions(+) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 5d10366..c1c0951 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -57,6 +57,7 @@ #include "hw/pci/pci.h" #include "hw/scsi/scsi.h" #include "hw/virtio/virtio-scsi.h" +#include "hw/virtio/vhost-scsi-common.h" #include "exec/address-spaces.h" #include "hw/usb.h" @@ -2384,6 +2385,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, ((type *)object_dynamic_cast(OBJECT(obj), (name))) SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); sPAPRPHBState *phb = CAST(sPAPRPHBState, dev, TYPE_SPAPR_PCI_HOST_BRIDGE); +VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, TYPE_VHOST_SCSI_COMMON); if (d) { void *spapr = CAST(void, bus->parent, "spapr-vscsi"); @@ -2440,6 +2442,12 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, BusState *bus, return g_strdup_printf("pci@%"PRIX64, phb->buid); } +if (vsc) { +/* Same logic as virtio above */ +unsigned id = 0x100 | (vsc->target << 16) | vsc->lun; +return g_strdup_printf("disk@%"PRIX64, (uint64_t)id << 32); +} + return NULL; } -- 2.9.4
[Qemu-devel] [PULL 05/17] spapr: Abolish DRC set_configured method
DRConnectorClass has a set_configured method, however: * There is only one implementation, and only ever likely to be one * There's exactly one caller, and that's (now) local * The implementation is very straightforward So abolish the method entirely, and just open-code what we need. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier Reviewed-by: Greg Kurz Tested-by: Daniel Barboza --- hw/ppc/spapr_drc.c | 24 include/hw/ppc/spapr_drc.h | 3 --- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index f5b7b68..693571a 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -199,18 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc) return drc->name; } -static void set_configured(sPAPRDRConnector *drc) -{ -trace_spapr_drc_set_configured(get_index(drc)); - -if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_UNISOLATED) { -/* guest should be not configuring an isolated device */ -trace_spapr_drc_set_configured_skipping(get_index(drc)); -return; -} -drc->configured = true; -} - /* has the guest been notified of device attachment? */ static void set_signalled(sPAPRDRConnector *drc) { @@ -745,7 +733,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) drck->get_index = get_index; drck->get_type = get_type; drck->get_name = get_name; -drck->set_configured = set_configured; drck->entity_sense = entity_sense; drck->attach = attach; drck->detach = detach; @@ -1113,7 +1100,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, uint64_t wa_offset; uint32_t drc_index; sPAPRDRConnector *drc; -sPAPRDRConnectorClass *drck; sPAPRConfigureConnectorState *ccs; sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE; int rc; @@ -1133,7 +1119,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, goto out; } -drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); if (!drc->fdt) { trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index); rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE; @@ -1170,10 +1155,17 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, case FDT_END_NODE: ccs->fdt_depth--; if (ccs->fdt_depth == 0) { +sPAPRDRIsolationState state = drc->isolation_state; /* done sending the device tree, don't need to track * the state anymore */ -drck->set_configured(drc); +trace_spapr_drc_set_configured(get_index(drc)); +if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) { +drc->configured = true; +} else { +/* guest should be not configuring an isolated device */ +trace_spapr_drc_set_configured_skipping(get_index(drc)); +} spapr_ccs_remove(spapr, ccs); ccs = NULL; resp = SPAPR_DR_CC_RESPONSE_SUCCESS; diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h index 80db955..90f4953 100644 --- a/include/hw/ppc/spapr_drc.h +++ b/include/hw/ppc/spapr_drc.h @@ -177,9 +177,6 @@ typedef struct sPAPRDRConnectorClass { uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state); -/* QEMU interfaces for managing FDT/configure-connector */ -void (*set_configured)(sPAPRDRConnector *drc); - /* QEMU interfaces for managing hotplug operations */ void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt, int fdt_start_offset, bool coldplug, Error **errp); -- 2.9.4
[Qemu-devel] [PULL 17/17] spapr: Remove some non-useful properties on DRC objects
* 'connector_type' is easily derived from the 'index' property, so there's no point to it (it's also implicit in the QOM type of the DRC) * 'isolation-state', 'indicator-state' and 'allocation-state' are part of the transaction between qemu and guest during PAPR hotplug operations, and outside tools really have no business looking at it (especially not changing, and these were RW properties) * 'entity-sense' is basically just a weird PAPR encoding of whether there is a device connected to this DRC Strictly speaking removing these properties is breaking the qemu interface. However, I'm pretty sure no management tools have ever used these. For debugging there are better alternatives. Therefore, I think removing these broken interfaces is the better option. Signed-off-by: David Gibson Reviewed-by: Michael Roth Acked-by: Michael Roth --- hw/ppc/spapr_drc.c | 29 - 1 file changed, 29 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 06df5d0..39e7f30 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -228,14 +228,6 @@ static void prop_get_index(Object *obj, Visitor *v, const char *name, visit_type_uint32(v, name, &value, errp); } -static void prop_get_type(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ -sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); -uint32_t value = (uint32_t)spapr_drc_type(drc); -visit_type_uint32(v, name, &value, errp); -} - static char *prop_get_name(Object *obj, Error **errp) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); @@ -243,17 +235,6 @@ static char *prop_get_name(Object *obj, Error **errp) return g_strdup(drck->get_name(drc)); } -static void prop_get_entity_sense(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ -sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); -sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -uint32_t value; - -drck->entity_sense(drc, &value); -visit_type_uint32(v, name, &value, errp); -} - static void prop_get_fdt(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -666,20 +647,10 @@ static void spapr_dr_connector_instance_init(Object *obj) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj); -object_property_add_uint32_ptr(obj, "isolation-state", - &drc->isolation_state, NULL); -object_property_add_uint32_ptr(obj, "indicator-state", - &drc->indicator_state, NULL); -object_property_add_uint32_ptr(obj, "allocation-state", - &drc->allocation_state, NULL); object_property_add_uint32_ptr(obj, "id", &drc->id, NULL); object_property_add(obj, "index", "uint32", prop_get_index, NULL, NULL, NULL, NULL); -object_property_add(obj, "connector_type", "uint32", prop_get_type, -NULL, NULL, NULL, NULL); object_property_add_str(obj, "name", prop_get_name, NULL, NULL); -object_property_add(obj, "entity-sense", "uint32", prop_get_entity_sense, -NULL, NULL, NULL, NULL); object_property_add(obj, "fdt", "struct", prop_get_fdt, NULL, NULL, NULL, NULL); } -- 2.9.4
[Qemu-devel] [PULL 08/17] target/ppc: Fixup set_spr error in h_register_process_table
From: Suraj Jitindar Singh set_spr is used in the function h_register_process_table() to update the LPCR_GTSE and LPCR_UPRT values based on the flags passed by the guest. The set_spr function takes the last two arguments mask and value used to mask and set the value of the spr respectively. The current call site passes these arguments in the wrong order and thus bot GTSE and UPRT will be set irrespective, which is obviously incorrect. Rearrange the function call so that these arguments are passed in the correct order and the correct behaviour is exhibited. It is worth noting that this wasn't detected earlier since these were always both set in all cases where this H_CALL was made. Fixes: 6de833070ca2 ("target/ppc: Set UPRT and GTSE on all cpus in H_REGISTER_PROCESS_TABLE") Signed-off-by: Suraj Jitindar Singh Signed-off-by: David Gibson --- hw/ppc/spapr_hcall.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index aae5a62..aa1ffea 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -992,9 +992,10 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu, /* Update the UPRT and GTSE bits in the LPCR for all cpus */ CPU_FOREACH(cs) { -set_spr(cs, SPR_LPCR, LPCR_UPRT | LPCR_GTSE, +set_spr(cs, SPR_LPCR, ((flags & (FLAG_RADIX | FLAG_HASH_PROC_TBL)) ? LPCR_UPRT : 0) | -((flags & FLAG_GTSE) ? LPCR_GTSE : 0)); +((flags & FLAG_GTSE) ? LPCR_GTSE : 0), +LPCR_UPRT | LPCR_GTSE); } if (kvm_enabled()) { -- 2.9.4
[Qemu-devel] [PULL 09/17] spapr_nvram: Check return value from blk_getlength()
From: Peter Maydell The blk_getlength() function can return an error value if the image size cannot be determined. Check for this rather than ploughing on and trying to g_malloc0() a negative number. (Spotted by Coverity, CID 1288484.) Signed-off-by: Peter Maydell Signed-off-by: David Gibson --- hw/nvram/spapr_nvram.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c index aa5d2c1..bc355a4 100644 --- a/hw/nvram/spapr_nvram.c +++ b/hw/nvram/spapr_nvram.c @@ -144,7 +144,15 @@ static void spapr_nvram_realize(VIOsPAPRDevice *dev, Error **errp) int ret; if (nvram->blk) { -nvram->size = blk_getlength(nvram->blk); +int64_t len = blk_getlength(nvram->blk); + +if (len < 0) { +error_setg_errno(errp, -len, + "could not get length of backing image"); +return; +} + +nvram->size = len; ret = blk_set_perm(nvram->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE, -- 2.9.4
[Qemu-devel] [PULL 16/17] spapr: Eliminate spapr_drc_get_type_str()
This function was used in generating the device tree. However, now that we have different QOM types for different DRC types we can easily store the information we need in the class structure and avoid this specialized lookup function. Signed-off-by: David Gibson Reviewed-by: Michael Roth Acked-by: Michael Roth --- hw/ppc/spapr_drc.c | 31 --- include/hw/ppc/spapr_drc.h | 1 + 2 files changed, 5 insertions(+), 27 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 783c621..06df5d0 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -712,6 +712,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data) sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k); drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU; +drck->typename = "CPU"; } static void spapr_drc_pci_class_init(ObjectClass *k, void *data) @@ -719,6 +720,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data) sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k); drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI; +drck->typename = "28"; } static void spapr_drc_lmb_class_init(ObjectClass *k, void *data) @@ -726,6 +728,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data) sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k); drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB; +drck->typename = "MEM"; } static const TypeInfo spapr_dr_connector_info = { @@ -795,31 +798,6 @@ sPAPRDRConnector *spapr_drc_by_id(const char *type, uint32_t id) | (id & DRC_INDEX_ID_MASK)); } -/* generate a string the describes the DRC to encode into the - * device tree. - * - * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1 - */ -static const char *spapr_drc_get_type_str(sPAPRDRConnectorType type) -{ -switch (type) { -case SPAPR_DR_CONNECTOR_TYPE_CPU: -return "CPU"; -case SPAPR_DR_CONNECTOR_TYPE_PHB: -return "PHB"; -case SPAPR_DR_CONNECTOR_TYPE_VIO: -return "SLOT"; -case SPAPR_DR_CONNECTOR_TYPE_PCI: -return "28"; -case SPAPR_DR_CONNECTOR_TYPE_LMB: -return "MEM"; -default: -g_assert(false); -} - -return NULL; -} - /** * spapr_drc_populate_dt * @@ -901,8 +879,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner, drc_names = g_string_insert_len(drc_names, -1, "\0", 1); /* ibm,drc-types */ -drc_types = g_string_append(drc_types, - spapr_drc_get_type_str(spapr_drc_type(drc))); +drc_types = g_string_append(drc_types, drck->typename); drc_types = g_string_insert_len(drc_types, -1, "\0", 1); } diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h index 7dbb478..c88e1be 100644 --- a/include/hw/ppc/spapr_drc.h +++ b/include/hw/ppc/spapr_drc.h @@ -212,6 +212,7 @@ typedef struct sPAPRDRConnectorClass { /*< public >*/ sPAPRDRConnectorTypeShift typeshift; +const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */ /* accessors for guest-visible (generally via RTAS) DR state */ uint32_t (*set_isolation_state)(sPAPRDRConnector *drc, -- 2.9.4
[Qemu-devel] [PULL 03/17] spapr: Move DRC RTAS calls into spapr_drc.c
Currently implementations of the RTAS calls related to DRCs are in spapr_rtas.c. They belong better in spapr_drc.c - that way they're closer to related code, and we'll be able to make some more things local. spapr_rtas.c was intended to contain the RTAS infrastructure and core calls that don't belong anywhere else, not every RTAS implementation. Code motion only. Signed-off-by: David Gibson Reviewed-by: Greg Kurz Tested-by: Daniel Barboza --- hw/ppc/spapr_drc.c | 322 ++-- hw/ppc/spapr_rtas.c | 304 - 2 files changed, 315 insertions(+), 311 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index cc2400b..ae8800d 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -27,6 +27,34 @@ #define DRC_INDEX_TYPE_SHIFT 28 #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1) +static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr, +uint32_t drc_index) +{ +sPAPRConfigureConnectorState *ccs = NULL; + +QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) { +if (ccs->drc_index == drc_index) { +break; +} +} + +return ccs; +} + +static void spapr_ccs_add(sPAPRMachineState *spapr, + sPAPRConfigureConnectorState *ccs) +{ +g_assert(!spapr_ccs_find(spapr, ccs->drc_index)); +QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next); +} + +static void spapr_ccs_remove(sPAPRMachineState *spapr, + sPAPRConfigureConnectorState *ccs) +{ +QTAILQ_REMOVE(&spapr->ccs_list, ccs, next); +g_free(ccs); +} + static sPAPRDRConnectorTypeShift get_type_shift(sPAPRDRConnectorType type) { uint32_t shift = 0; @@ -747,13 +775,6 @@ static const TypeInfo spapr_dr_connector_info = { .class_init= spapr_dr_connector_class_init, }; -static void spapr_drc_register_types(void) -{ -type_register_static(&spapr_dr_connector_info); -} - -type_init(spapr_drc_register_types) - /* helper functions for external users */ sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index) @@ -932,3 +953,290 @@ out: return ret; } + +/* + * RTAS calls + */ + +static bool sensor_type_is_dr(uint32_t sensor_type) +{ +switch (sensor_type) { +case RTAS_SENSOR_TYPE_ISOLATION_STATE: +case RTAS_SENSOR_TYPE_DR: +case RTAS_SENSOR_TYPE_ALLOCATION_STATE: +return true; +} + +return false; +} + +static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, + uint32_t token, uint32_t nargs, + target_ulong args, uint32_t nret, + target_ulong rets) +{ +uint32_t sensor_type; +uint32_t sensor_index; +uint32_t sensor_state; +uint32_t ret = RTAS_OUT_SUCCESS; +sPAPRDRConnector *drc; +sPAPRDRConnectorClass *drck; + +if (nargs != 3 || nret != 1) { +ret = RTAS_OUT_PARAM_ERROR; +goto out; +} + +sensor_type = rtas_ld(args, 0); +sensor_index = rtas_ld(args, 1); +sensor_state = rtas_ld(args, 2); + +if (!sensor_type_is_dr(sensor_type)) { +goto out_unimplemented; +} + +/* if this is a DR sensor we can assume sensor_index == drc_index */ +drc = spapr_dr_connector_by_index(sensor_index); +if (!drc) { +trace_spapr_rtas_set_indicator_invalid(sensor_index); +ret = RTAS_OUT_PARAM_ERROR; +goto out; +} +drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); + +switch (sensor_type) { +case RTAS_SENSOR_TYPE_ISOLATION_STATE: +/* if the guest is configuring a device attached to this + * DRC, we should reset the configuration state at this + * point since it may no longer be reliable (guest released + * device and needs to start over, or unplug occurred so + * the FDT is no longer valid) + */ +if (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) { +sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr, + sensor_index); +if (ccs) { +spapr_ccs_remove(spapr, ccs); +} +} +ret = drck->set_isolation_state(drc, sensor_state); +break; +case RTAS_SENSOR_TYPE_DR: +ret = drck->set_indicator_state(drc, sensor_state); +break; +case RTAS_SENSOR_TYPE_ALLOCATION_STATE: +ret = drck->set_allocation_state(drc, sensor_state); +break; +default: +goto out_unimplemented; +} + +out: +rtas_st(rets, 0, ret); +return; + +out_unimplemented: +/* currently only DR-related sensors are implemented */ +trace_spapr_rtas_set_indicator_not_supported(sensor_index, sensor_type); +rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED); +} + +static void rtas_get_sensor_state(PowerPCCPU *cpu, sPAPRMachineState *spapr,
[Qemu-devel] [PULL 12/17] spapr/drc: don't migrate DRC of cold-plugged CPUs and LMBs
From: Greg Kurz As explained in commit 5c0139a8c2f0 ("spapr: fix default DRC state for coldplugged LMBs"), guests expect cold-plugged LMBs to be pre-allocated and unisolated. The same goes for cold-plugged CPUs. While here, let's convert g_assert(false) to the better self documenting g_assert_not_reached(). Signed-off-by: Greg Kurz Signed-off-by: David Gibson --- hw/ppc/spapr_drc.c | 10 +++--- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index a35314e..c1d4b10 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -546,20 +546,16 @@ static bool spapr_drc_needed(void *opaque) */ switch (drc->type) { case SPAPR_DR_CONNECTOR_TYPE_PCI: -rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) && - (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) && - drc->configured && drc->signalled && !drc->awaiting_release); -break; case SPAPR_DR_CONNECTOR_TYPE_CPU: case SPAPR_DR_CONNECTOR_TYPE_LMB: -rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) && - (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) && +rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) && + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) && drc->configured && drc->signalled && !drc->awaiting_release); break; case SPAPR_DR_CONNECTOR_TYPE_PHB: case SPAPR_DR_CONNECTOR_TYPE_VIO: default: -g_assert(false); +g_assert_not_reached(); } return rc; } -- 2.9.4
[Qemu-devel] [PULL 14/17] spapr: Clean up spapr_dr_connector_by_*()
* Change names to something less ludicrously verbose * Now that we have QOM subclasses for the different DRC types, use a QOM typename instead of a PAPR type value parameter The latter allows removal of the get_type_shift() helper. Signed-off-by: David Gibson Reviewed-by: Michael Roth Acked-by: Michael Roth --- hw/ppc/spapr.c | 28 ++-- hw/ppc/spapr_drc.c | 34 ++ hw/ppc/spapr_events.c | 2 +- hw/ppc/spapr_pci.c | 6 ++ include/hw/ppc/spapr_drc.h | 5 ++--- 5 files changed, 29 insertions(+), 46 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 11a04d1..3fa9263 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -461,7 +461,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ]; int i; -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index); if (drc) { drc_index = spapr_drc_index(drc); _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index))); @@ -654,7 +654,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState *spapr, void *fdt) if (i >= hotplug_lmb_start) { sPAPRDRConnector *drc; -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, i); g_assert(drc); dynamic_memory[0] = cpu_to_be32(addr >> 32); @@ -2536,8 +2536,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, uint64_t addr = addr_start; for (i = 0; i < nr_lmbs; i++) { -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, -addr/SPAPR_MEMORY_BLOCK_SIZE); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, + addr / SPAPR_MEMORY_BLOCK_SIZE); g_assert(drc); fdt = create_device_tree(&fdt_size); @@ -2558,8 +2558,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, */ if (dev->hotplugged) { if (dedicated_hp_event_source) { -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, -addr_start / SPAPR_MEMORY_BLOCK_SIZE); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, + addr_start / SPAPR_MEMORY_BLOCK_SIZE); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs, @@ -2676,8 +2676,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms, addr = addr_start; for (i = 0; i < nr_lmbs; i++) { -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, - addr / SPAPR_MEMORY_BLOCK_SIZE); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, + addr / SPAPR_MEMORY_BLOCK_SIZE); g_assert(drc); if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) { avail_lmbs++; @@ -2760,8 +2760,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, addr = addr_start; for (i = 0; i < nr_lmbs; i++) { -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, -addr / SPAPR_MEMORY_BLOCK_SIZE); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, + addr / SPAPR_MEMORY_BLOCK_SIZE); g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); @@ -2769,8 +2769,8 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, addr += SPAPR_MEMORY_BLOCK_SIZE; } -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, - addr_start / SPAPR_MEMORY_BLOCK_SIZE); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, + addr_start / SPAPR_MEMORY_BLOCK_SIZE); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB, nr_lmbs, spapr_drc_index(drc)); @@ -2841,7 +2841,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, return; } -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt); g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); @@ -2876,7 +2876,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, cc->core_id); return; } -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt); +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt); g_assert(drc || !mc->has_hotpluggable_cpus); diff --git a/h
[Qemu-devel] [PULL 07/17] target-ppc: Fix openpic timer read register offset
From: Aaron Larson openpic_tmr_read() is incorrectly computing register offset of the TCCR, TBCR, TVPR, and TDR registers when accessing the open pic timer registers. Specifically the offset of timer registers for openpic_tmr_read() is not accounting for the timer frequency reporting register (TFFR) which is the first register in the "tmr" memory region. openpic_tmr_write() *is* correctly computing the offset by adding 0x10f0 to the address prior to computing the register index. This patch instead subtracts 0x10 in both the read and write routines and eliminates some other gratuitous differences between the functions. Signed-off-by: Aaron Larson Signed-off-by: David Gibson --- hw/intc/openpic.c | 22 ++ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index 4349e45..f966d06 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -796,27 +796,24 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len) } static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val, -unsigned len) + unsigned len) { OpenPICState *opp = opaque; int idx; -addr += 0x10f0; - DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n", -__func__, addr, val); +__func__, (addr + 0x10f0), val); if (addr & 0xF) { return; } -if (addr == 0x10f0) { +if (addr == 0) { /* TFRR */ opp->tfrr = val; return; } - +addr -= 0x10; /* correct for TFRR */ idx = (addr >> 6) & 0x3; -addr = addr & 0x30; switch (addr & 0x30) { case 0x00: /* TCCR */ @@ -844,16 +841,17 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) uint32_t retval = -1; int idx; -DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr); +DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0); if (addr & 0xF) { goto out; } -idx = (addr >> 6) & 0x3; -if (addr == 0x0) { +if (addr == 0) { /* TFRR */ retval = opp->tfrr; goto out; } +addr -= 0x10; /* correct for TFRR */ +idx = (addr >> 6) & 0x3; switch (addr & 0x30) { case 0x00: /* TCCR */ retval = opp->timers[idx].tccr; @@ -861,10 +859,10 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr addr, unsigned len) case 0x10: /* TBCR */ retval = opp->timers[idx].tbcr; break; -case 0x20: /* TIPV */ +case 0x20: /* TVPR */ retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx); break; -case 0x30: /* TIDE (TIDR) */ +case 0x30: /* TDR */ retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx); break; } -- 2.9.4
[Qemu-devel] [PULL 10/17] ppc/pnv: check the return value of fdt_setprop()
From: Cédric Le Goater Signed-off-by: Cédric Le Goater [dwg: Correct typo in commit message] Signed-off-by: David Gibson --- hw/ppc/pnv.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 231ed97..89b6801 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -378,8 +378,9 @@ static void powernv_populate_ipmi_bt(ISADevice *d, void *fdt, int lpc_off) _FDT(node); g_free(name); -fdt_setprop(fdt, node, "reg", io_regs, sizeof(io_regs)); -fdt_setprop(fdt, node, "compatible", compatible, sizeof(compatible)); +_FDT((fdt_setprop(fdt, node, "reg", io_regs, sizeof(io_regs; +_FDT((fdt_setprop(fdt, node, "compatible", compatible, + sizeof(compatible; /* Mark it as reserved to avoid Linux trying to claim it */ _FDT((fdt_setprop_string(fdt, node, "status", "reserved"))); -- 2.9.4
[Qemu-devel] [PULL 04/17] spapr: Abolish DRC get_fdt method
The DRConnectorClass includes a get_fdt method. However * There's only one implementation, and there's only likely to ever be one * Both callers are local to spapr_drc * Each caller only uses one half of the actual implementation So abolish get_fdt() entirely, and just open-code what we need. Signed-off-by: David Gibson Reviewed-by: Laurent Vivier Reviewed-by: Greg Kurz Tested-by: Daniel Barboza --- hw/ppc/spapr_drc.c | 23 ++- include/hw/ppc/spapr_drc.h | 1 - 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index ae8800d..f5b7b68 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -199,14 +199,6 @@ static const char *get_name(sPAPRDRConnector *drc) return drc->name; } -static const void *get_fdt(sPAPRDRConnector *drc, int *fdt_start_offset) -{ -if (fdt_start_offset) { -*fdt_start_offset = drc->fdt_start_offset; -} -return drc->fdt; -} - static void set_configured(sPAPRDRConnector *drc) { trace_spapr_drc_set_configured(get_index(drc)); @@ -753,7 +745,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data) drck->get_index = get_index; drck->get_type = get_type; drck->get_name = get_name; -drck->get_fdt = get_fdt; drck->set_configured = set_configured; drck->entity_sense = entity_sense; drck->attach = attach; @@ -1126,7 +1117,6 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, sPAPRConfigureConnectorState *ccs; sPAPRDRCCResponse resp = SPAPR_DR_CC_RESPONSE_CONTINUE; int rc; -const void *fdt; if (nargs != 2 || nret != 1) { rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); @@ -1144,8 +1134,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, } drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); -fdt = drck->get_fdt(drc, NULL); -if (!fdt) { +if (!drc->fdt) { trace_spapr_rtas_ibm_configure_connector_missing_fdt(drc_index); rc = SPAPR_DR_CC_RESPONSE_NOT_CONFIGURABLE; goto out; @@ -1154,7 +1143,7 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, ccs = spapr_ccs_find(spapr, drc_index); if (!ccs) { ccs = g_new0(sPAPRConfigureConnectorState, 1); -(void)drck->get_fdt(drc, &ccs->fdt_offset); +ccs->fdt_offset = drc->fdt_start_offset; ccs->drc_index = drc_index; spapr_ccs_add(spapr, ccs); } @@ -1165,12 +1154,12 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, const struct fdt_property *prop; int fdt_offset_next, prop_len; -tag = fdt_next_tag(fdt, ccs->fdt_offset, &fdt_offset_next); +tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next); switch (tag) { case FDT_BEGIN_NODE: ccs->fdt_depth++; -name = fdt_get_name(fdt, ccs->fdt_offset, NULL); +name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL); /* provide the name of the next OF node */ wa_offset = CC_VAL_DATA_OFFSET; @@ -1193,9 +1182,9 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu, } break; case FDT_PROP: -prop = fdt_get_property_by_offset(fdt, ccs->fdt_offset, +prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset, &prop_len); -name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff)); +name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff)); /* provide the name of the next OF property */ wa_offset = CC_VAL_DATA_OFFSET; diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h index 813b9ff..80db955 100644 --- a/include/hw/ppc/spapr_drc.h +++ b/include/hw/ppc/spapr_drc.h @@ -178,7 +178,6 @@ typedef struct sPAPRDRConnectorClass { uint32_t (*entity_sense)(sPAPRDRConnector *drc, sPAPRDREntitySense *state); /* QEMU interfaces for managing FDT/configure-connector */ -const void *(*get_fdt)(sPAPRDRConnector *drc, int *fdt_start_offset); void (*set_configured)(sPAPRDRConnector *drc); /* QEMU interfaces for managing hotplug operations */ -- 2.9.4
[Qemu-devel] [PULL 15/17] spapr: Move configure-connector state into DRC
Currently the sPAPRMachineState contains a list of sPAPRConfigureConnector structures which store intermediate state for the ibm,configure-connector RTAS call. This was an attempt to separate this state from the core of the DRC state. However the configure connector process is intimately tied to the DRC model, so there's really no point trying to have two levels of interface here. Moving the configure-connector state into its corresponding DRC allows removal of a number of helpers for maintaining the anciliary list. Signed-off-by: David Gibson Reviewed-by: Michael Roth Acked-by: Michael Roth --- hw/ppc/spapr.c | 4 --- hw/ppc/spapr_drc.c | 73 -- include/hw/ppc/spapr.h | 14 - include/hw/ppc/spapr_drc.h | 7 + 4 files changed, 25 insertions(+), 73 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 3fa9263..86e6228 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2340,10 +2340,6 @@ static void ppc_spapr_init(MachineState *machine) register_savevm_live(NULL, "spapr/htab", -1, 1, &savevm_htab_handlers, spapr); -/* used by RTAS */ -QTAILQ_INIT(&spapr->ccs_list); -qemu_register_reset(spapr_ccs_reset_hook, spapr); - qemu_register_boot_set(spapr_boot_set, spapr); if (kvm_enabled()) { diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 7ed2de1..783c621 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -27,34 +27,6 @@ #define DRC_INDEX_TYPE_SHIFT 28 #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1) -static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr, -uint32_t drc_index) -{ -sPAPRConfigureConnectorState *ccs = NULL; - -QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) { -if (ccs->drc_index == drc_index) { -break; -} -} - -return ccs; -} - -static void spapr_ccs_add(sPAPRMachineState *spapr, - sPAPRConfigureConnectorState *ccs) -{ -g_assert(!spapr_ccs_find(spapr, ccs->drc_index)); -QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next); -} - -static void spapr_ccs_remove(sPAPRMachineState *spapr, - sPAPRConfigureConnectorState *ccs) -{ -QTAILQ_REMOVE(&spapr->ccs_list, ccs, next); -g_free(ccs); -} - sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc) { sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); @@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state); +/* if the guest is configuring a device attached to this DRC, we + * should reset the configuration state at this point since it may + * no longer be reliable (guest released device and needs to start + * over, or unplug occurred so the FDT is no longer valid) + */ +if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) { +g_free(drc->ccs); +drc->ccs = NULL; +} + if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) { /* cannot unisolate a non-existent resource, and, or resources * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5) @@ -485,6 +467,10 @@ static void reset(DeviceState *d) sPAPRDREntitySense state; trace_spapr_drc_reset(spapr_drc_index(drc)); + +g_free(drc->ccs); +drc->ccs = NULL; + /* immediately upon reset we can safely assume DRCs whose devices * are pending removal can be safely removed, and that they will * subsequently be left in an ISOLATED state. move the DRC to this @@ -1019,19 +1005,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr, switch (sensor_type) { case RTAS_SENSOR_TYPE_ISOLATION_STATE: -/* if the guest is configuring a device attached to this - * DRC, we should reset the configuration state at this - * point since it may no longer be reliable (guest released - * device and needs to start over, or unplug occurred so - * the FDT is no longer valid) - */ -if (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) { -sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr, - sensor_index); -if (ccs) { -spapr_ccs_remove(spapr, ccs); -} -} ret = drck->set_isolation_state(drc, sensor_state); break; case RTAS_SENSOR_TYPE_DR: @@ -1115,16 +1088,6 @@ static void configure_connector_st(target_ulong addr, target_ulong offset, buf, MIN(len, CC_WA_LEN - offset)); } -void spapr_ccs_reset_hook(void *opaque) -{ -sPAPRMachineState *spapr = opaque; -sPAPRConfigureConnectorState *ccs, *ccs_tmp; - -QTAILQ_FOREACH_SAFE(ccs, &spapr->ccs_list, next, ccs_tmp) { -spapr_ccs
[Qemu-devel] [PULL 01/17] migration: remove register_savevm()
From: Laurent Vivier We can replace the four remaining calls of register_savevm() by calls to register_savevm_live(). So we can remove the function and as we don't allocate anymore the ops pointer with g_new0() we don't have to free it then. Signed-off-by: Laurent Vivier Reviewed-by: Juan Quintela Signed-off-by: David Gibson --- hw/net/vmxnet3.c| 8 ++-- hw/s390x/s390-skeys.c | 9 +++-- hw/s390x/s390-virtio-ccw.c | 8 ++-- include/migration/vmstate.h | 8 migration/savevm.c | 16 slirp/slirp.c | 8 ++-- 6 files changed, 25 insertions(+), 32 deletions(-) diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 8b1fab2..4df3110 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2262,6 +2262,11 @@ static const MemoryRegionOps b1_ops = { }, }; +static SaveVMHandlers savevm_vmxnet3_msix = { +.save_state = vmxnet3_msix_save, +.load_state = vmxnet3_msix_load, +}; + static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) { uint64_t dsn_payload; @@ -2331,8 +2336,7 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) vmxnet3_device_serial_num(s)); } -register_savevm(dev, "vmxnet3-msix", -1, 1, -vmxnet3_msix_save, vmxnet3_msix_load, s); +register_savevm_live(dev, "vmxnet3-msix", -1, 1, &savevm_vmxnet3_msix, s); } static void vmxnet3_instance_init(Object *obj) diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c index 619152c..35e7f63 100644 --- a/hw/s390x/s390-skeys.c +++ b/hw/s390x/s390-skeys.c @@ -362,6 +362,11 @@ static inline bool s390_skeys_get_migration_enabled(Object *obj, Error **errp) return ss->migration_enabled; } +static SaveVMHandlers savevm_s390_storage_keys = { +.save_state = s390_storage_keys_save, +.load_state = s390_storage_keys_load, +}; + static inline void s390_skeys_set_migration_enabled(Object *obj, bool value, Error **errp) { @@ -375,8 +380,8 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value, ss->migration_enabled = value; if (ss->migration_enabled) { -register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save, -s390_storage_keys_load, ss); +register_savevm_live(NULL, TYPE_S390_SKEYS, 0, 1, + &savevm_s390_storage_keys, ss); } else { unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss); } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index c9021f2..a806345 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -104,6 +104,11 @@ void s390_memory_init(ram_addr_t mem_size) s390_skeys_init(); } +static SaveVMHandlers savevm_gtod = { +.save_state = gtod_save, +.load_state = gtod_load, +}; + static void ccw_init(MachineState *machine) { int ret; @@ -151,8 +156,7 @@ static void ccw_init(MachineState *machine) s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw"); /* Register savevm handler for guest TOD clock */ -register_savevm(NULL, "todclock", 0, 1, -gtod_save, gtod_load, kvm_state); +register_savevm_live(NULL, "todclock", 0, 1, &savevm_gtod, kvm_state); } static void s390_cpu_plug(HotplugHandler *hotplug_dev, diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 6689562..8a3e9e6 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -59,14 +59,6 @@ typedef struct SaveVMHandlers { LoadStateHandler *load_state; } SaveVMHandlers; -int register_savevm(DeviceState *dev, -const char *idstr, -int instance_id, -int version_id, -SaveStateHandler *save_state, -LoadStateHandler *load_state, -void *opaque); - int register_savevm_live(DeviceState *dev, const char *idstr, int instance_id, diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..035c127 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -645,21 +645,6 @@ int register_savevm_live(DeviceState *dev, return 0; } -int register_savevm(DeviceState *dev, -const char *idstr, -int instance_id, -int version_id, -SaveStateHandler *save_state, -LoadStateHandler *load_state, -void *opaque) -{ -SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1); -ops->save_state = save_state; -ops->load_state = load_state; -return register_savevm_live(dev, idstr, instance_id, version_id, -ops, opaque); -} - void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) { SaveStateEntry *se, *new_se; @@ -679,7 +664,6 @@ void u
[Qemu-devel] [PULL 00/17] ppc-for-2.10 queue 20170606
The following changes since commit 199e19ee538eb61fd08b1c1ee5aa838ebdcc968e: Merge remote-tracking branch 'remotes/mjt/tags/trivial-patches-fetch' into staging (2017-06-05 15:28:12 +0100) are available in the git repository at: git://github.com/dgibson/qemu.git tags/ppc-for-2.10-20170606 for you to fetch changes up to 91dcb1ffa67cfa41a13dcc89dd44e2310b5773b0: spapr: Remove some non-useful properties on DRC objects (2017-06-06 09:24:25 +1000) ppc patch queue 2017-06-06 Accumulated patches for ppc targets and the pseries machine type. The big thing in this batch is a start on a substantial cleanup of the pseries hotplug mechanisms, which were pretty confusing. For now these shouldn't cause substantial behavioural changes, but I am hoping these lead to clearer code and eventually to fixes for the bugs we have in hotplug handling, particularly when hotplug and migration are combined. The remaining patches are mostly bugfixes. Aaron Larson (1): target-ppc: Fix openpic timer read register offset Cédric Le Goater (1): ppc/pnv: check the return value of fdt_setprop() David Gibson (10): migration: Mark CPU states dirty before incoming migration/loadvm spapr: Move DRC RTAS calls into spapr_drc.c spapr: Abolish DRC get_fdt method spapr: Abolish DRC set_configured method spapr: Make DRC get_index and get_type methods into plain functions spapr: Introduce DRC subclasses spapr: Clean up spapr_dr_connector_by_*() spapr: Move configure-connector state into DRC spapr: Eliminate spapr_drc_get_type_str() spapr: Remove some non-useful properties on DRC objects Felipe Franciosi (1): spapr: Allow boot from vhost-*-scsi backends Greg Kurz (1): spapr/drc: don't migrate DRC of cold-plugged CPUs and LMBs Laurent Vivier (1): migration: remove register_savevm() Peter Maydell (1): spapr_nvram: Check return value from blk_getlength() Suraj Jitindar Singh (1): target/ppc: Fixup set_spr error in h_register_process_table cpus.c | 9 + hw/intc/openpic.c | 22 +- hw/net/vmxnet3.c| 8 +- hw/nvram/spapr_nvram.c | 10 +- hw/ppc/pnv.c| 5 +- hw/ppc/spapr.c | 58 +++-- hw/ppc/spapr_drc.c | 573 +++- hw/ppc/spapr_events.c | 12 +- hw/ppc/spapr_hcall.c| 5 +- hw/ppc/spapr_pci.c | 13 +- hw/ppc/spapr_rtas.c | 304 --- hw/s390x/s390-skeys.c | 9 +- hw/s390x/s390-virtio-ccw.c | 8 +- include/hw/ppc/spapr.h | 14 -- include/hw/ppc/spapr_drc.h | 69 +- include/migration/vmstate.h | 8 - include/sysemu/cpus.h | 1 + include/sysemu/hax.h| 1 + include/sysemu/hw_accel.h | 10 + include/sysemu/kvm.h| 1 + kvm-all.c | 10 + migration/savevm.c | 18 +- slirp/slirp.c | 8 +- target/i386/hax-all.c | 10 + 24 files changed, 595 insertions(+), 591 deletions(-)
[Qemu-devel] [PULL 02/17] migration: Mark CPU states dirty before incoming migration/loadvm
As a rule, CPU internal state should never be updated when !cpu->kvm_vcpu_dirty (or the HAX equivalent). If that is done, then subsequent calls to cpu_synchronize_state() - usually safe and idempotent - will clobber state. However, we routinely do this during a loadvm or incoming migration. Usually this is called shortly after a reset, which will clear all the cpu dirty flags with cpu_synchronize_all_post_reset(). Nothing is expected to set the dirty flags again before the cpu state is loaded from the incoming stream. This means that it isn't safe to call cpu_synchronize_state() from a post_load handler, which is non-obvious and potentially inconvenient. We could cpu_synchronize_all_state() before the loadvm, but that would be overkill since a) we expect the state to already be synchronized from the reset and b) we expect to completely rewrite the state with a call to cpu_synchronize_all_post_init() at the end of qemu_loadvm_state(). To clear this up, this patch introduces cpu_synchronize_pre_loadvm() and associated helpers, which simply marks the cpu state as dirty without actually changing anything. i.e. it says we want to discard any existing KVM (or HAX) state and replace it with what we're going to load. Cc: Juan Quintela Cc: Dave Gilbert Signed-off-by: David Gibson Reviewed-by: Juan Quintela --- cpus.c| 9 + include/sysemu/cpus.h | 1 + include/sysemu/hax.h | 1 + include/sysemu/hw_accel.h | 10 ++ include/sysemu/kvm.h | 1 + kvm-all.c | 10 ++ migration/savevm.c| 2 ++ target/i386/hax-all.c | 10 ++ 8 files changed, 44 insertions(+) diff --git a/cpus.c b/cpus.c index 516e5cb..6398439 100644 --- a/cpus.c +++ b/cpus.c @@ -921,6 +921,15 @@ void cpu_synchronize_all_post_init(void) } } +void cpu_synchronize_all_pre_loadvm(void) +{ +CPUState *cpu; + +CPU_FOREACH(cpu) { +cpu_synchronize_pre_loadvm(cpu); +} +} + static int do_vm_stop(RunState state) { int ret = 0; diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h index a8053f1..731756d 100644 --- a/include/sysemu/cpus.h +++ b/include/sysemu/cpus.h @@ -27,6 +27,7 @@ void qemu_timer_notify_cb(void *opaque, QEMUClockType type); void cpu_synchronize_all_states(void); void cpu_synchronize_all_post_reset(void); void cpu_synchronize_all_post_init(void); +void cpu_synchronize_all_pre_loadvm(void); void qtest_clock_warp(int64_t dest); diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h index d9f0239..232a68a 100644 --- a/include/sysemu/hax.h +++ b/include/sysemu/hax.h @@ -33,6 +33,7 @@ int hax_populate_ram(uint64_t va, uint32_t size); void hax_cpu_synchronize_state(CPUState *cpu); void hax_cpu_synchronize_post_reset(CPUState *cpu); void hax_cpu_synchronize_post_init(CPUState *cpu); +void hax_cpu_synchronize_pre_loadvm(CPUState *cpu); #ifdef CONFIG_HAX diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h index c9b3105..469ffda 100644 --- a/include/sysemu/hw_accel.h +++ b/include/sysemu/hw_accel.h @@ -45,4 +45,14 @@ static inline void cpu_synchronize_post_init(CPUState *cpu) } } +static inline void cpu_synchronize_pre_loadvm(CPUState *cpu) +{ +if (kvm_enabled()) { +kvm_cpu_synchronize_pre_loadvm(cpu); +} +if (hax_enabled()) { +hax_cpu_synchronize_pre_loadvm(cpu); +} +} + #endif /* QEMU_HW_ACCEL_H */ diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 5cc83f2..a45c145 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -459,6 +459,7 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr, void kvm_cpu_synchronize_state(CPUState *cpu); void kvm_cpu_synchronize_post_reset(CPUState *cpu); void kvm_cpu_synchronize_post_init(CPUState *cpu); +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu); void kvm_init_cpu_signals(CPUState *cpu); diff --git a/kvm-all.c b/kvm-all.c index 7df27c8..494b925 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -1896,6 +1896,16 @@ void kvm_cpu_synchronize_post_init(CPUState *cpu) run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, RUN_ON_CPU_NULL); } +static void do_kvm_cpu_synchronize_pre_loadvm(CPUState *cpu, run_on_cpu_data arg) +{ +cpu->kvm_vcpu_dirty = true; +} + +void kvm_cpu_synchronize_pre_loadvm(CPUState *cpu) +{ +run_on_cpu(cpu, do_kvm_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL); +} + #ifdef KVM_HAVE_MCE_INJECTION static __thread void *pending_sigbus_addr; static __thread int pending_sigbus_code; diff --git a/migration/savevm.c b/migration/savevm.c index 035c127..1993ca2 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1999,6 +1999,8 @@ int qemu_loadvm_state(QEMUFile *f) } } +cpu_synchronize_all_pre_loadvm(); + ret = qemu_loadvm_state_main(f, mis); qemu_event_set(&mis->main_thread_load_event); diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c index 7346931..097db5c 100644 --- a/target/i386/hax-all.c
Re: [Qemu-devel] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW
On Mon, 06/05 15:38, Eric Blake wrote: > The lone caller that cares about a return of BDRV_BLOCK_RAW > (namely, io.c:bdrv_co_get_block_status) completely replaces the > return value, so there is no point in passing BDRV_BLOCK_DATA. > > Signed-off-by: Eric Blake > > --- > v3: further document BDRV_BLOCK_RAW > v2: fix subject, tweak commit message > --- > include/block/block.h | 6 +++--- > block/commit.c| 2 +- > block/mirror.c| 2 +- > block/raw-format.c| 2 +- > block/vpc.c | 2 +- > 5 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index 9b355e9..0a60444 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -131,9 +131,9 @@ typedef struct HDGeometry { > * layer (short for DATA || ZERO), set by block layer > * > * Internal flag: > - * BDRV_BLOCK_RAW: used internally to indicate that the request was > - * answered by a passthrough driver such as raw and that the > - * block layer should recompute the answer from bs->file. > + * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request > + * that the block layer recompute the answer from the > returned > + * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID. > * > * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) > * represent the offset in the returned BDS that is allocated for the > diff --git a/block/commit.c b/block/commit.c > index a3028b2..f56745b 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -239,7 +239,7 @@ static int64_t coroutine_fn > bdrv_commit_top_get_block_status( > { > *pnum = nb_sectors; > *file = bs->backing->bs; > -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > } > > diff --git a/block/mirror.c b/block/mirror.c > index a2a9703..892d53a 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1052,7 +1052,7 @@ static int64_t coroutine_fn > bdrv_mirror_top_get_block_status( > { > *pnum = nb_sectors; > *file = bs->backing->bs; > -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > } > > diff --git a/block/raw-format.c b/block/raw-format.c > index 36e6503..1136eba 100644 > --- a/block/raw-format.c > +++ b/block/raw-format.c > @@ -259,7 +259,7 @@ static int64_t coroutine_fn > raw_co_get_block_status(BlockDriverState *bs, > *pnum = nb_sectors; > *file = bs->file->bs; > sector_num += s->offset / BDRV_SECTOR_SIZE; > -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > } > > diff --git a/block/vpc.c b/block/vpc.c > index 4240ba9..b313c68 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -701,7 +701,7 @@ static int64_t coroutine_fn > vpc_co_get_block_status(BlockDriverState *bs, > if (be32_to_cpu(footer->type) == VHD_FIXED) { > *pnum = nb_sectors; > *file = bs->file->bs; > -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | > +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | > (sector_num << BDRV_SECTOR_BITS); > } > > -- > 2.9.4 > > Reviewed-by: Fam Zheng
Re: [Qemu-devel] [PATCH v2 1/4] dump: add DumpInfo structure
On Mon, Jun 05, 2017 at 09:24:38AM +0200, Andrew Jones wrote: > On Fri, Jun 02, 2017 at 09:46:35AM +, Marc-André Lureau wrote: > > Hi > > > > On Thu, Jun 1, 2017 at 10:19 PM Eric Blake wrote: > > > > > On 06/01/2017 01:06 PM, Laszlo Ersek wrote: > > > > On 06/01/17 15:03, Marc-André Lureau wrote: > > > >> One way or another, the guest could communicate various dump info (via > > > >> guest agent or vmcoreinfo device) and populate that structure. It can > > > >> then be used to augment the dump with various details, as done in the > > > >> following patch. > > > >> > > > >> Signed-off-by: Marc-André Lureau > > > >> --- > > > >> include/sysemu/dump-info.h | 18 ++ > > > >> dump.c | 3 +++ > > > >> 2 files changed, 21 insertions(+) > > > >> create mode 100644 include/sysemu/dump-info.h > > > >> > > > >> diff --git a/include/sysemu/dump-info.h b/include/sysemu/dump-info.h > > > >> new file mode 100644 > > > >> index 00..d2378e15e2 > > > >> --- /dev/null > > > >> +++ b/include/sysemu/dump-info.h > > > >> @@ -0,0 +1,18 @@ > > > >> +#ifndef DUMP_INFO_H > > > >> +#define DUMP_INFO_H > > > > > > >> > > > > > > > > Can you please spell out, in the commit message, the reason for > > > > introducing a new header file? (I suspect your reason, but it should be > > > > documented explicitly.) > > > > > > Also, should you have a copyright header in the new file? And does > > > MAINTAINERS cover it? > > > > > > > None of the dump support is covered. Based on commit history, I can suggest > > Wen Congyang, as original author. > > Sadly, Qiao Nuohan cannot be reached with his mail today (anyone knows if > > he is still contributing?). Laszlo has done significant changes and reviews > > too. I can also propose myself to help with reviews. > > > > Wen or Laszla, do you want to be the main maintainer? > > What about Peter Xu (CC'ed)? He'll have to state whether he has time and > interest, but I see he's contributed quite a bit to dump.c. I believe those were the first commits of mine when working on detached dump, after that I didn't really work on it anymore. So I guess I may not be the best candidate. Also, I admit my bandwidth is limited as well recently. Thanks Drew for mentioning me anyway. :-) -- Peter Xu
[Qemu-devel] [BUG] Failed to compile using gcc7.1
Hi all, After upgrading gcc from 6.3.1 to 7.1.1, qemu can't be compiled with gcc. The error is: -- CC block/blkdebug.o block/blkdebug.c: In function 'blkdebug_refresh_filename': block/blkdebug.c:693:31: error: '%s' directive output may be truncated writing up to 4095 bytes into a region of size 4086 [-Werror=format-truncation=] "blkdebug:%s:%s", s->config_file ?: "", ^~ In file included from /usr/include/stdio.h:939:0, from /home/adam/qemu/include/qemu/osdep.h:68, from block/blkdebug.c:25: /usr/include/bits/stdio2.h:64:10: note: '__builtin___snprintf_chk' output 11 or more bytes (assuming 4106) into a destination of size 4096 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~ __bos (__s), __fmt, __va_arg_pack ()); ~ cc1: all warnings being treated as errors make: *** [/home/adam/qemu/rules.mak:69: block/blkdebug.o] Error 1 -- It seems that gcc 7 is introducing more restrict check for printf. If using clang, although there are some extra warning, it can at least pass the compile. Thanks, Qu
Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally
On Mon, Jun 05, 2017 at 03:22:24PM -0500, Eric Blake wrote: > On 05/19/2017 01:43 AM, Peter Xu wrote: > > We were do the shutting off only for postcopy. Now we do this as long as > > the source return path is there. > > > > Moving the cleanup of from_src_file there too. > > > > Signed-off-by: Peter Xu > > --- > > migration/migration.c| 8 +++- > > migration/postcopy-ram.c | 1 - > > 2 files changed, 7 insertions(+), 2 deletions(-) > > This commit causes a regression in qemu-iotests 68: > > $ cd tests/qemu-iotests > $ ./check -qcow2 68 > ... > 068 1s ... - output mismatch (see 068.out.bad) > --- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out 2017-05-30 > 09:27:26.795821748 -0500 > +++ 068.out.bad 2017-06-05 15:21:18.566816816 -0500 > @@ -6,6 +6,8 @@ > QEMU X.Y.Z monitor - type 'help' for more information > (qemu) savevm 0 > (qemu) quit > +./common.config: line 107: 1912 Segmentation fault (core dumped) > ( if [ -n "${QEMU_NEED_PID}" ]; then > +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) > QEMU X.Y.Z monitor - type 'help' for more information > -(qemu) quit > -*** done > +(qemu) *** done > Failures: 068 > Failed 1 of 1 tests > > I didn't investigate further; but am hoping you'll be able to fix the > segfault and get the test working again. Sorry for that! I will have a look. -- Peter Xu
Re: [Qemu-devel] [PATCH v2 2/6] pci: Add comment for pci_add_capability2()
Hi, Marcel On 06/05/2017 09:34 PM, Marcel Apfelbaum wrote: On 02/06/2017 10:54, Mao Zhongyi wrote: Add a comment for pci_add_capability2() to explain the return value. This may help to make a correct return value check for its callers. Cc: m...@redhat.com Cc: mar...@redhat.com Cc: arm...@redhat.com Suggested-by: Markus Armbruster Signed-off-by: Mao Zhongyi --- hw/pci/pci.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 53566b8..9810d5f 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2276,6 +2276,10 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id, return ret; } +/* + * On success, pci_add_capability2() returns a positive value. Hi, I would specify what is the return value: the offset of the pci capability. Thanks, Marcel Thanks, I will. Mao + * On failure, it sets an error and returns a negative value. + */ int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id, uint8_t offset, uint8_t size, Error **errp)
Re: [Qemu-devel] [PATCH v2 3/6] pci: Fix the wrong return value judgment condition
Hi, Marcel On 06/06/2017 12:20 AM, Marcel Apfelbaum wrote: On 02/06/2017 10:54, Mao Zhongyi wrote: On success, pci_add_capability2() returns a positive value. On failure, it sets an error and return a negative value. It doesn't always return 0. So the judgment condtion of pci_add_capability2() is wrong if it contains the situation where return value equal to 0. Fix the error checks from its callers. Hi, I would suggest changing the above to a simpler: pci_add_capability returns a strictly positive value on success, correct asserts. Thanks, Marcel OK, I see. Thanks a lot. Mao Cc: dmi...@daynix.com Cc: jasow...@redhat.com Cc: alex.william...@redhat.com Cc: mar...@redhat.com Cc: m...@redhat.com Cc: arm...@redhat.com Signed-off-by: Mao Zhongyi --- hw/net/e1000e.c | 2 +- hw/net/eepro100.c | 2 +- hw/vfio/pci.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c index 6e23493..8259d67 100644 --- a/hw/net/e1000e.c +++ b/hw/net/e1000e.c @@ -374,7 +374,7 @@ e1000e_add_pm_capability(PCIDevice *pdev, uint8_t offset, uint16_t pmc) { int ret = pci_add_capability(pdev, PCI_CAP_ID_PM, offset, PCI_PM_SIZEOF); -if (ret >= 0) { +if (ret > 0) { pci_set_word(pdev->config + offset + PCI_PM_PMC, PCI_PM_CAP_VER_1_1 | pmc); diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c index 4bf71f2..da36816 100644 --- a/hw/net/eepro100.c +++ b/hw/net/eepro100.c @@ -571,7 +571,7 @@ static void e100_pci_reset(EEPRO100State * s) int cfg_offset = 0xdc; int r = pci_add_capability(&s->dev, PCI_CAP_ID_PM, cfg_offset, PCI_PM_SIZEOF); -assert(r >= 0); +assert(r > 0); pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21); #if 0 /* TODO: replace dummy code for power management emulation. */ /* TODO: Power Management Control / Status. */ diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 32aca77..5881968 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1744,7 +1744,7 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size, } pos = pci_add_capability(&vdev->pdev, PCI_CAP_ID_EXP, pos, size); -if (pos >= 0) { +if (pos > 0) { vdev->pdev.exp.exp_cap = pos; }
Re: [Qemu-devel] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
On 06/05/2017 04:38 PM, Eric Blake wrote: > We document that *file is valid if the return is not an error and > includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract > when a driver (such as blkdebug) lacks a callback. Messed up in > commit 67a0fd2 (v2.6), when we added the file parameter. > > Enhance qemu-iotest 177 to cover this, using a sequence that would > print garbage or even SEGV, because it was dererefencing through > uninitialized memory. [The resulting test output shows that we > have less-than-ideal block status from the blkdebug driver, but > that's a separate fix coming up soon.] > > Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is > enough to fix the crash, but we can go one step further: always > setting *file, even on error, means that a broken caller that > blindly dereferences file without checking for error is now more > likely to get a reliable SEGV instead of randomly acting on garbage, > making it easier to diagnose such buggy callers. Adding an > assertion that file is set where expected doesn't hurt either. > > CC: qemu-sta...@nongnu.org > Signed-off-by: Eric Blake > Reviewed-by: Fam Zheng > Reviewed-by: Max Reitz Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH v4 1/4] qemu-io: Don't die on second open
On 06/05/2017 04:38 PM, Eric Blake wrote: > Most callback commands in qemu-io return 0 to keep the interpreter > loop running, or 1 to quit immediately. However, open_f() just > passed through the return value of openfile(), which has different > semantics of returning 0 if a file was opened, or 1 on any failure. > > As a result of mixing the return semantics, we are forcing the > qemu-io interpreter to exit early on any failures, which is rather > annoying when some of the failures are obviously trying to give > the user a hint of how to proceed (if we didn't then kill qemu-io > out from under the user's feet): > > $ qemu-io > qemu-io> open foo > qemu-io> open foo > file open already, try 'help close' > $ echo $? > 0 > > In general, we WANT openfile() to report failures, since it is the > function used in the form 'qemu-io -c "$something" no_such_file' > for performing one or more -c options on a single file, and it is > not worth attempting $something if the file itself cannot be opened. > So the solution is to fix open_f() to always return 0 (when we are > in interactive mode, even failure to open should not end the > session), and save the return value of openfile() for command line > use in main(). > > Note, however, that we do have some qemu-iotests that do 'qemu-io > -c "open file" -c "$something"'; such tests will now proceed to > attempt $something whether or not the open succeeded, the same way > as if the two commands had been attempted in interactive mode. As > such, the expected output for those tests has to be modified. But it > also means that it is now possible to use -c close and have a single > qemu-io command line operate on more than one file even without > using interactive mode. Although the '-c open' action is a subtle > change in behavior, remember that qemu-io is for debugging purposes, > so as long as it serves the needs of qemu-iotests while still being > reasonable for interactive use, it should not be a problem that we > are changing tests to the new behavior. > > This has been awkward since at least as far back as commit > e3aff4f, in 2009. > > Signed-off-by: Eric Blake > Reviewed-by: Fam Zheng Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH v2 1/1] s390x: vmstatify config migration for virtio-ccw
* Eric Blake [2017-06-05 07:19:14 -0500]: Hi Eric, > On 06/04/2017 10:09 PM, Dong Jia Shi wrote: > > * Halil Pasic [2017-06-02 16:05:31 +0200]: > > > > Hi Halil, > > > > Sorry for the late show up. I just found some nits, which could be > > ignored for me. > > > >> Let's vmstatify virtio_ccw_save_config and virtio_ccw_load_config for > >> flexibility (extending using subsections) and for fun. > >> > >> To achieve this we need to hack the config_vector, which is VirtIODevice > >> (that is common virtio) state, in the middle of the VirtioCcwDevice state > >> representation. This is somewhat ugly, but we have no choice because the > > ^^ > > Nit:-++ > > What's wrong here? Two spaces between sentences is a common > typographical convention Thanks for letting me learn this. I didn't know this interesting knowledge before I saw this comment and searched in the Internet: https://en.wikipedia.org/wiki/Sentence_spacing > (true, the codebase is inconsistent on whether sentences are separated > with one or two spaces, but that's all the more reason to realize that > since we don't have a consistent standard, it is just churn to change > from one style to the other) I was thinking 1) inconsistence is strange and unwelcome, and 2) the inconsistence here may be something that Halil did not intend to have. Since it's a common typographical convention, and I'm not a native English speaker. I will follow your opinion. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dong Jia Shi
Re: [Qemu-devel] [PATCH v2] spapr: Allow boot from vhost-*-scsi backends
On Mon, Jun 05, 2017 at 04:55:18PM +0100, Felipe Franciosi wrote: > The current implementation of spapr_get_fw_dev_path() doesn't take into > consideration vhost-*-scsi devices. This makes said devices unbootable > on PPC as SLOF is unable to work out the path to scan boot disks. > > This makes VMs bootable on spapr when using vhost-*-scsi by implementing > a disk path for VHostSCSICommon (which currently includes both > vhost-user-scsi and vhost-scsi). > > Signed-off-by: Felipe Franciosi > Signed-off-by: Mike Cui Applied to ppc-for-2.10, thanks. > --- > hw/ppc/spapr.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ab3aab1..1c87886 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -57,6 +57,7 @@ > #include "hw/pci/pci.h" > #include "hw/scsi/scsi.h" > #include "hw/virtio/virtio-scsi.h" > +#include "hw/virtio/vhost-scsi-common.h" > > #include "exec/address-spaces.h" > #include "hw/usb.h" > @@ -2388,6 +2389,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, > BusState *bus, > ((type *)object_dynamic_cast(OBJECT(obj), (name))) > SCSIDevice *d = CAST(SCSIDevice, dev, TYPE_SCSI_DEVICE); > sPAPRPHBState *phb = CAST(sPAPRPHBState, dev, > TYPE_SPAPR_PCI_HOST_BRIDGE); > +VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, > TYPE_VHOST_SCSI_COMMON); > > if (d) { > void *spapr = CAST(void, bus->parent, "spapr-vscsi"); > @@ -2444,6 +2446,12 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, > BusState *bus, > return g_strdup_printf("pci@%"PRIX64, phb->buid); > } > > +if (vsc) { > +/* Same logic as virtio above */ > +unsigned id = 0x100 | (vsc->target << 16) | vsc->lun; > +return g_strdup_printf("disk@%"PRIX64, (uint64_t)id << 32); > +} > + > return NULL; > } > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCHv2 1/5] spapr: Introduce DRC subclasses
On Mon, Jun 05, 2017 at 11:32:07AM -0500, Michael Roth wrote: > Quoting David Gibson (2017-06-04 22:31:12) > > Currently we only have a single QOM type for all DRCs, but lots of > > places where we switch behaviour based on the DRC's PAPR defined type. > > This is a poor use of our existing type system. > > > > So, instead create QOM subclasses for each PAPR defined DRC type. We > > also introduce intermediate subclasses for physical and logical DRCs, > > a division which will be useful later on. > > > > Instead of being stored in the DRC object itself, the PAPR type is now > > stored in the class structure. There are still many places where we > > switch directly on the PAPR type value, but this at least provides the > > basis to start to remove those. > > > > Signed-off-by: David Gibson > > Reviewed-by: Michael Roth > > > --- > > hw/ppc/spapr.c | 5 +- > > hw/ppc/spapr_drc.c | 123 > > + > > hw/ppc/spapr_pci.c | 3 +- > > include/hw/ppc/spapr_drc.h | 47 +++-- > > 4 files changed, 139 insertions(+), 39 deletions(-) > > > > Mike, here's an updated version of the patch addressing the problems > > you pointed out. If I can get an ack, I can merge the series, which > > would be nice. > > Series: > > Acked-by: Michael Roth Thanks, I've merged this into ppc-for-2.10. > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 5d10366..456f9e7 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1911,7 +1911,7 @@ static void > > spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > > uint64_t addr; > > > > addr = i * lmb_size + spapr->hotplug_memory.base; > > -drc = spapr_dr_connector_new(OBJECT(spapr), > > SPAPR_DR_CONNECTOR_TYPE_LMB, > > +drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB, > > addr/lmb_size); > > qemu_register_reset(spapr_drc_reset, drc); > > } > > @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) > > > > if (mc->has_hotpluggable_cpus) { > > sPAPRDRConnector *drc = > > -spapr_dr_connector_new(OBJECT(spapr), > > - SPAPR_DR_CONNECTOR_TYPE_CPU, > > +spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU, > > (core_id / smp_threads) * smt); > > > > qemu_register_reset(spapr_drc_reset, drc); > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > index a35314e..8575022 100644 > > --- a/hw/ppc/spapr_drc.c > > +++ b/hw/ppc/spapr_drc.c > > @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift > > get_type_shift(sPAPRDRConnectorType type) > > return shift; > > } > > > > +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc) > > +{ > > +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > + > > +return 1 << drck->typeshift; > > +} > > + > > uint32_t spapr_drc_index(sPAPRDRConnector *drc) > > { > > +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > + > > /* no set format for a drc index: it only needs to be globally > > * unique. this is how we encode the DRC type on bare-metal > > * however, so might as well do that here > > */ > > -return (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) | > > -(drc->id & DRC_INDEX_ID_MASK); > > +return (drck->typeshift << DRC_INDEX_TYPE_SHIFT) > > +| (drc->id & DRC_INDEX_ID_MASK); > > } > > > > static uint32_t set_isolation_state(sPAPRDRConnector *drc, > > @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector > > *drc, > > * If the LMB being removed doesn't belong to a DIMM device that is > > * actually being unplugged, fail the isolation request here. > > */ > > -if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) { > > +if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) { > > if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) && > > !drc->awaiting_release) { > > return RTAS_OUT_HW_ERROR; > > @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector > > *drc, > > } > > } > > > > -if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) { > > +if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) { > > drc->allocation_state = state; > > if (drc->awaiting_release && > > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { > > @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector > > *drc, > > return RTAS_OUT_SUCCESS; > > } > > > > -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc) > > -{ > > -return drc->type; > > -} > > - > > static const char *get_name(sPAPRDRConnector *drc) > > { > > return drc->name; > > @@ -217,7 +221,7 @@ static void set_si
Re: [Qemu-devel] [PATCH] ppc/pnv: check the return value of fd_setprop()
On Mon, Jun 05, 2017 at 05:44:21PM +0200, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater Applied to ppc-for-2.10, thanks. > --- > hw/ppc/pnv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 231ed9735b65..89b6801f67b5 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -378,8 +378,9 @@ static void powernv_populate_ipmi_bt(ISADevice *d, void > *fdt, int lpc_off) > _FDT(node); > g_free(name); > > -fdt_setprop(fdt, node, "reg", io_regs, sizeof(io_regs)); > -fdt_setprop(fdt, node, "compatible", compatible, sizeof(compatible)); > +_FDT((fdt_setprop(fdt, node, "reg", io_regs, sizeof(io_regs; > +_FDT((fdt_setprop(fdt, node, "compatible", compatible, > + sizeof(compatible; > > /* Mark it as reserved to avoid Linux trying to claim it */ > _FDT((fdt_setprop_string(fdt, node, "status", "reserved"))); -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] spapr/drc: don't migrate DRC of cold-plugged CPUs and LMBs
On Fri, Jun 02, 2017 at 12:09:35PM +0200, Greg Kurz wrote: > As explained in commit 5c0139a8c2f0 ("spapr: fix default DRC state for > coldplugged LMBs"), guests expect cold-plugged LMBs to be pre-allocated > and unisolated. The same goes for cold-plugged CPUs. > > While here, let's convert g_assert(false) to the better self documenting > g_assert_not_reached(). > > Signed-off-by: Greg Kurz I've applied this to ppc-for-2.10. I'm a bit concerned that this is just another bandaid thrown at the mess which is the DRC code. I'm hoping to clean this up more thoroughly, which will obsolete this, but in the meantime it shouldn't be any worse than what we have. > --- > > FWIW > > $ git grep -i -E '(g_)?assert\((0|false)\)' | wc -l > 100 > $ git grep g_assert_not_reached | wc -l > 244 > --- > hw/ppc/spapr_drc.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index cc2400bcd57f..ab5f7cdf569c 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -538,20 +538,16 @@ static bool spapr_drc_needed(void *opaque) > */ > switch (drc->type) { > case SPAPR_DR_CONNECTOR_TYPE_PCI: > -rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) > && > - (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) && > - drc->configured && drc->signalled && !drc->awaiting_release); > -break; > case SPAPR_DR_CONNECTOR_TYPE_CPU: > case SPAPR_DR_CONNECTOR_TYPE_LMB: > -rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) && > - (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) > && > +rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) > && > + (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) && > drc->configured && drc->signalled && !drc->awaiting_release); > break; > case SPAPR_DR_CONNECTOR_TYPE_PHB: > case SPAPR_DR_CONNECTOR_TYPE_VIO: > default: > -g_assert(false); > +g_assert_not_reached(); > } > return rc; > } > -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] spapr_nvram: Check return value from blk_getlength()
On Mon, Jun 05, 2017 at 04:14:17PM +0100, Peter Maydell wrote: > The blk_getlength() function can return an error value if the > image size cannot be determined. Check for this rather than > ploughing on and trying to g_malloc0() a negative number. > (Spotted by Coverity, CID 1288484.) > > Signed-off-by: Peter Maydell Applied to ppc-for-2.10, thanks. > --- > hw/nvram/spapr_nvram.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c > index aa5d2c1..bc355a4 100644 > --- a/hw/nvram/spapr_nvram.c > +++ b/hw/nvram/spapr_nvram.c > @@ -144,7 +144,15 @@ static void spapr_nvram_realize(VIOsPAPRDevice *dev, > Error **errp) > int ret; > > if (nvram->blk) { > -nvram->size = blk_getlength(nvram->blk); > +int64_t len = blk_getlength(nvram->blk); > + > +if (len < 0) { > +error_setg_errno(errp, -len, > + "could not get length of backing image"); > +return; > +} > + > +nvram->size = len; > > ret = blk_set_perm(nvram->blk, > BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE, -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [Qemu-devel] BUG: KASAN: use-after-free in free_old_xmit_skbs
On Mon, Jun 05, 2017 at 05:08:25AM +0300, Michael S. Tsirkin wrote: > On Mon, Jun 05, 2017 at 12:48:53AM +0200, Jean-Philippe Menil wrote: > > Hi, > > > > while playing with xdp and ebpf, i'm hitting the following: > > > > [ 309.993136] > > == > > [ 309.994735] BUG: KASAN: use-after-free in > > free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net] > > [ 309.998396] Read of size 8 at addr 88006aa64220 by task sshd/323 > > [ 310.000650] > > [ 310.002305] CPU: 1 PID: 323 Comm: sshd Not tainted 4.12.0-rc3+ #2 > > [ 310.004018] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > 1.10.2-20170228_101828-anatol 04/01/2014 > > [ 310.006495] Call Trace: > > [ 310.007610] dump_stack+0xb8/0x14c > > [ 310.008748] ? _atomic_dec_and_lock+0x174/0x174 > > [ 310.009998] ? pm_qos_get_value.part.7+0x6/0x6 > > [ 310.011203] print_address_description+0x6f/0x280 > > [ 310.012416] kasan_report+0x27a/0x370 > > [ 310.013573] ? free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net] > > [ 310.014900] __asan_report_load8_noabort+0x19/0x20 > > [ 310.016136] free_old_xmit_skbs.isra.29+0x2b7/0x2e0 [virtio_net] > > [ 310.017467] ? virtnet_del_vqs+0xe0/0xe0 [virtio_net] > > [ 310.018759] ? packet_rcv+0x20d0/0x20d0 > > [ 310.019950] ? dev_queue_xmit_nit+0x5cd/0xaf0 > > [ 310.021168] start_xmit+0x1b4/0x1b10 [virtio_net] > > [ 310.022413] ? default_device_exit+0x2d0/0x2d0 > > [ 310.023634] ? virtnet_remove+0xf0/0xf0 [virtio_net] > > [ 310.024874] ? update_load_avg+0x1281/0x29f0 > > [ 310.026059] dev_hard_start_xmit+0x1ea/0x7f0 > > [ 310.027247] ? validate_xmit_skb_list+0x100/0x100 > > [ 310.028470] ? validate_xmit_skb+0x7f/0xc10 > > [ 310.029731] ? netif_skb_features+0x920/0x920 > > [ 310.033469] ? __skb_tx_hash+0x2f0/0x2f0 > > [ 310.035615] ? validate_xmit_skb_list+0xa3/0x100 > > [ 310.037782] sch_direct_xmit+0x2eb/0x7a0 > > [ 310.039842] ? dev_deactivate_queue.constprop.29+0x230/0x230 > > [ 310.041980] ? netdev_pick_tx+0x212/0x2b0 > > [ 310.043868] __dev_queue_xmit+0x12fa/0x20b0 > > [ 310.045564] ? netdev_pick_tx+0x2b0/0x2b0 > > [ 310.047210] ? __account_cfs_rq_runtime+0x630/0x630 > > [ 310.048301] ? update_stack_state+0x402/0x780 > > [ 310.049307] ? account_entity_enqueue+0x730/0x730 > > [ 310.050322] ? __rb_erase_color+0x27d0/0x27d0 > > [ 310.051286] ? update_curr_fair+0x70/0x70 > > [ 310.052206] ? enqueue_entity+0x2450/0x2450 > > [ 310.053124] ? entry_SYSCALL64_slow_path+0x25/0x25 > > [ 310.054082] ? dequeue_entity+0x27a/0x1520 > > [ 310.054967] ? bpf_prog_alloc+0x320/0x320 > > [ 310.055822] ? yield_to_task_fair+0x110/0x110 > > [ 310.056708] ? set_next_entity+0x2f2/0xa90 > > [ 310.057574] ? dequeue_task_fair+0xc09/0x2ec0 > > [ 310.058457] dev_queue_xmit+0x10/0x20 > > [ 310.059298] ip_finish_output2+0xacf/0x12a0 > > [ 310.060160] ? dequeue_entity+0x1520/0x1520 > > [ 310.063410] ? ip_fragment.constprop.47+0x220/0x220 > > [ 310.065078] ? ring_buffer_set_clock+0x50/0x50 > > [ 310.066677] ? __switch_to+0x685/0xda0 > > [ 310.068166] ? load_balance+0x38f0/0x38f0 > > [ 310.069544] ? compat_start_thread+0x80/0x80 > > [ 310.070989] ? trace_find_cmdline+0x60/0x60 > > [ 310.072402] ? rt_cpu_seq_show+0x2d0/0x2d0 > > [ 310.073579] ip_finish_output+0x407/0x880 > > [ 310.074441] ? ip_finish_output+0x407/0x880 > > [ 310.075255] ? update_stack_state+0x402/0x780 > > [ 310.076076] ip_output+0x1c0/0x640 > > [ 310.076843] ? ip_mc_output+0x1350/0x1350 > > [ 310.077642] ? __sk_dst_check+0x164/0x370 > > [ 310.078441] ? complete_formation.isra.53+0xa30/0xa30 > > [ 310.079313] ? __read_once_size_nocheck.constprop.7+0x20/0x20 > > [ 310.080265] ? sock_prot_inuse_add+0xa0/0xa0 > > [ 310.081097] ? memcpy+0x45/0x50 > > [ 310.081850] ? __copy_skb_header+0x1fa/0x280 > > [ 310.082676] ip_local_out+0x70/0x90 > > [ 310.083448] ip_queue_xmit+0x8a1/0x22a0 > > [ 310.084236] ? ip_build_and_send_pkt+0xe80/0xe80 > > [ 310.085079] ? tcp_v4_md5_lookup+0x13/0x20 > > [ 310.085884] tcp_transmit_skb+0x187a/0x3e00 > > [ 310.086696] ? __tcp_select_window+0xaf0/0xaf0 > > [ 310.087524] ? sock_sendmsg+0xba/0xf0 > > [ 310.088298] ? __vfs_write+0x4e0/0x960 > > [ 310.089074] ? vfs_write+0x155/0x4b0 > > [ 310.089838] ? SyS_write+0xf7/0x240 > > [ 310.090593] ? do_syscall_64+0x235/0x5b0 > > [ 310.091372] ? entry_SYSCALL64_slow_path+0x25/0x25 > > [ 310.094690] ? sock_sendmsg+0xba/0xf0 > > [ 310.096133] ? do_syscall_64+0x235/0x5b0 > > [ 310.097593] ? entry_SYSCALL64_slow_path+0x25/0x25 > > [ 310.099157] ? tcp_init_tso_segs+0x1e0/0x1e0 > > [ 310.100539] ? radix_tree_lookup+0xd/0x10 > > [ 310.101894] ? get_work_pool+0xcd/0x150 > > [ 310.103216] ? check_flush_dependency+0x330/0x330 > > [ 310.104113] tcp_write_xmit+0x498/0x52a0 > > [ 310.104905] ? kasan_unpoison_shadow+0x35/0x50 > > [ 310.105729] ? kasan_kmalloc+0xad/0xe0 > > [ 310.106505] ? tcp_transmit_skb+
Re: [Qemu-devel] [PATCH v2 20/20] block: Make bdrv_is_allocated_above() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. In the common case, allocation is unlikely to ever use > values that are not naturally sector-aligned, but it is possible > that byte-based values will let us be more precise about allocation > at the end of an unaligned file that can do byte-based access. > > Changing the signature of the function to use int64_t *pnum ensures > that the compiler enforces that all callers are updated. For now, > the io.c layer still assert()s that all callers are sector-aligned, > but that can be relaxed when a later patch implements byte-based > block status. Therefore, for the most part this patch is just the > addition of scaling at the callers followed by inverse scaling at > bdrv_is_allocated(). But some code, particularly stream_run(), > gets a lot simpler because it no longer has to mess with sectors. > > For ease of review, bdrv_is_allocated() was tackled separately. > > Signed-off-by: Eric Blake > Reviewed-by: John Snow
Re: [Qemu-devel] [PATCH v2 20/20] block: Make bdrv_is_allocated_above() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > -int64_t sector_num, int nb_sectors, int *pnum); > +int64_t offset, int64_t bytes, int64_t *pnum); Minor context conflict after this that, for whichever reason, git could not resolve on its own for me.
Re: [Qemu-devel] [PATCH v2 18/20] block: Make bdrv_is_allocated() byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. In the common case, allocation is unlikely to ever use > values that are not naturally sector-aligned, but it is possible > that byte-based values will let us be more precise about allocation > at the end of an unaligned file that can do byte-based access. > > Changing the signature of the function to use int64_t *pnum ensures > that the compiler enforces that all callers are updated. For now, > the io.c layer still assert()s that all callers are sector-aligned > on input and that *pnum is sector-aligned on return to the caller, > but that can be relaxed when a later patch implements byte-based > block status. Therefore, this code adds usages like > DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned > values, where the call might reasonbly give non-aligned results > in the future; on the other hand, no rounding is needed for callers > that should just continue to work with byte alignment. > > For the most part this patch is just the addition of scaling at the > callers followed by inverse scaling at bdrv_is_allocated(). But > some code, particularly bdrv_commit(), gets a lot simpler because it > no longer has to mess with sectors; also, it is now possible to pass > NULL if the caller does not care how much of the image is allocated > beyond the initial offset. > > For ease of review, bdrv_is_allocated_above() will be tackled > separately. > > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: rebase to earlier changes, tweak commit message > --- > include/block/block.h | 4 +-- > block/backup.c| 17 - > block/commit.c| 21 +++- > block/io.c| 49 +--- > block/stream.c| 5 ++-- > block/vvfat.c | 34 ++--- > migration/block.c | 9 --- > qemu-img.c| 5 +++- > qemu-io-cmds.c| 70 > +++ > 9 files changed, 114 insertions(+), 100 deletions(-) >
[Qemu-devel] [PATCH v2 3/3] tcg: allocate TB structs before the corresponding translated code
Allocating an arbitrarily-sized array of tbs results in either (a) a lot of memory wasted or (b) unnecessary flushes of the code cache when we run out of TB structs in the array. An obvious solution would be to just malloc a TB struct when needed, and keep the TB array as an array of pointers (recall that tb_find_pc() needs the TB array to run in O(log n)). Perhaps a better solution, which is implemented in this patch, is to allocate TB's right before the translated code they describe. This results in some memory waste due to padding to have code and TBs in separate cache lines--for instance, I measured 4.7% of padding in the used portion of code_gen_buffer when booting aarch64 Linux on a host with 64-byte cache lines. However, it can allow for optimizations in some host architectures, since TCG backends could safely assume that the TB and the corresponding translated code are very close to each other in memory. See this message by rth for a detailed explanation: https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg05172.html Subject: Re: GSoC 2017 Proposal: TCG performance enhancements Message-ID: <1e67644b-4b30-887e-d329-1848e94c9...@twiddle.net> Suggested-by: Richard Henderson Signed-off-by: Emilio G. Cota --- include/exec/exec-all.h | 2 +- include/exec/tb-context.h | 3 ++- tcg/tcg.c | 16 tcg/tcg.h | 2 +- translate-all.c | 37 ++--- 5 files changed, 42 insertions(+), 18 deletions(-) diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 87ae10b..00c0f43 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -363,7 +363,7 @@ struct TranslationBlock { */ uintptr_t jmp_list_next[2]; uintptr_t jmp_list_first; -}; +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); void tb_free(TranslationBlock *tb); void tb_flush(CPUState *cpu); diff --git a/include/exec/tb-context.h b/include/exec/tb-context.h index c7f17f2..25c2afe 100644 --- a/include/exec/tb-context.h +++ b/include/exec/tb-context.h @@ -31,8 +31,9 @@ typedef struct TBContext TBContext; struct TBContext { -TranslationBlock *tbs; +TranslationBlock **tbs; struct qht htable; +size_t tbs_size; int nb_tbs; /* any access to the tbs or the page table must use this lock */ QemuMutex tb_lock; diff --git a/tcg/tcg.c b/tcg/tcg.c index 564292f..f657c51 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -383,6 +383,22 @@ void tcg_context_init(TCGContext *s) } } +/* + * Allocate TBs right before their corresponding translated code, making + * sure that TBs and code are on different cache lines. + */ +TranslationBlock *tcg_tb_alloc(TCGContext *s) +{ +void *aligned; + +aligned = (void *)ROUND_UP((uintptr_t)s->code_gen_ptr, QEMU_CACHELINE_SIZE); +if (unlikely(aligned + sizeof(TranslationBlock) > s->code_gen_highwater)) { +return NULL; +} +s->code_gen_ptr += aligned - s->code_gen_ptr + sizeof(TranslationBlock); +return aligned; +} + void tcg_prologue_init(TCGContext *s) { size_t prologue_size, total_size; diff --git a/tcg/tcg.h b/tcg/tcg.h index 5ec48d1..9e37722 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -697,7 +697,6 @@ struct TCGContext { here, because there's too much arithmetic throughout that relies on addition and subtraction working on bytes. Rely on the GCC extension that allows arithmetic on void*. */ -int code_gen_max_blocks; void *code_gen_prologue; void *code_gen_epilogue; void *code_gen_buffer; @@ -756,6 +755,7 @@ static inline bool tcg_op_buf_full(void) /* tb_lock must be held for tcg_malloc_internal. */ void *tcg_malloc_internal(TCGContext *s, int size); void tcg_pool_reset(TCGContext *s); +TranslationBlock *tcg_tb_alloc(TCGContext *s); void tb_lock(void); void tb_unlock(void); diff --git a/translate-all.c b/translate-all.c index b3ee876..0eb9d13 100644 --- a/translate-all.c +++ b/translate-all.c @@ -781,12 +781,13 @@ static inline void code_gen_alloc(size_t tb_size) exit(1); } -/* Estimate a good size for the number of TBs we can support. We - still haven't deducted the prologue from the buffer size here, - but that's minimal and won't affect the estimate much. */ -tcg_ctx.code_gen_max_blocks -= tcg_ctx.code_gen_buffer_size / CODE_GEN_AVG_BLOCK_SIZE; -tcg_ctx.tb_ctx.tbs = g_new(TranslationBlock, tcg_ctx.code_gen_max_blocks); +/* size this conservatively -- realloc later if needed */ +tcg_ctx.tb_ctx.tbs_size = +tcg_ctx.code_gen_buffer_size / CODE_GEN_AVG_BLOCK_SIZE / 8; +if (unlikely(!tcg_ctx.tb_ctx.tbs_size)) { +tcg_ctx.tb_ctx.tbs_size = 64 * 1024; +} +tcg_ctx.tb_ctx.tbs = g_new(TranslationBlock *, tcg_ctx.tb_ctx.tbs_size); qemu_mutex_init(&tcg_ctx.tb_ctx.tb_lock); } @@ -828,13 +829,20 @@ bool tcg_enabled(void) static TranslationBlock *tb_alloc(target_ulong pc) { TranslationBlock *t
[Qemu-devel] [PATCH v2 1/3] compiler: define QEMU_CACHELINE_SIZE
This is a constant used as a hint for padding structs to hopefully avoid false cache line sharing. The constant can be set at configure time by defining QEMU_CACHELINE_SIZE via --extra-cflags. If not set there, we try to obtain the value from the machine running the configure script. If we fail, we default to reasonable values, i.e. 128 bytes for ppc64 and 64 bytes for all others. Note: the configure script only picks up the cache line size when run on Linux hosts because I have no other platforms (e.g. Windows, BSD's) to test on. Signed-off-by: Emilio G. Cota --- configure | 38 ++ include/qemu/compiler.h | 17 + 2 files changed, 55 insertions(+) diff --git a/configure b/configure index 13e040d..6a68cb2 100755 --- a/configure +++ b/configure @@ -4832,6 +4832,41 @@ EOF fi fi +# Find out the size of a cache line on the host +# TODO: support more platforms +cat > $TMPC< + +#define SYSFS "/sys/devices/system/cpu/cpu0/cache/index0/coherency_line_size" + +int main(int argc, char *argv[]) +{ +unsigned int size; +FILE *fp; + +fp = fopen(SYSFS, "r"); +if (fp == NULL) { +return -1; +} +if (!fscanf(fp, "%u", &size)) { +return -1; +} +return size; +} +#else +#error Cannot find host cache line size +#endif +EOF + +host_cacheline_size=0 +if compile_prog "" "" ; then +./$TMPE +host_cacheline_size=$? +fi + + ## # check for _Static_assert() @@ -5284,6 +5319,9 @@ fi if test "$bigendian" = "yes" ; then echo "HOST_WORDS_BIGENDIAN=y" >> $config_host_mak fi +if test "$host_cacheline_size" -gt 0 ; then +echo "HOST_CACHELINE_SIZE=$host_cacheline_size" >> $config_host_mak +fi if test "$mingw32" = "yes" ; then echo "CONFIG_WIN32=y" >> $config_host_mak rc_version=$(cat $source_path/VERSION) diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h index 340e5fd..178d831 100644 --- a/include/qemu/compiler.h +++ b/include/qemu/compiler.h @@ -40,6 +40,23 @@ # define QEMU_PACKED __attribute__((packed)) #endif +/* + * Cache line size of the host. Can be overriden. + * Note that this is just a compile-time hint to hopefully avoid false sharing + * of cache lines; code must be correct regardless of the constant's value. + */ +#ifndef QEMU_CACHELINE_SIZE +# ifdef HOST_CACHELINE_SIZE +# define QEMU_CACHELINE_SIZE HOST_CACHELINE_SIZE +# else +# if defined(__powerpc64__) +# define QEMU_CACHELINE_SIZE 128 +# else +# define QEMU_CACHELINE_SIZE 64 +# endif +# endif +#endif + #define QEMU_ALIGNED(X) __attribute__((aligned(X))) #ifndef glue -- 2.7.4
[Qemu-devel] [PATCH v2 2/3] tests: use QEMU_CACHELINE_SIZE instead of hard-coding it
Signed-off-by: Emilio G. Cota --- tests/atomic_add-bench.c | 4 ++-- tests/qht-bench.c| 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c index caa1e8e..c219109 100644 --- a/tests/atomic_add-bench.c +++ b/tests/atomic_add-bench.c @@ -5,11 +5,11 @@ struct thread_info { uint64_t r; -} QEMU_ALIGNED(64); +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); struct count { unsigned long val; -} QEMU_ALIGNED(64); +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); static QemuThread *threads; static struct thread_info *th_info; diff --git a/tests/qht-bench.c b/tests/qht-bench.c index 2afa09d..3f4b5eb 100644 --- a/tests/qht-bench.c +++ b/tests/qht-bench.c @@ -28,7 +28,7 @@ struct thread_info { uint64_t r; bool write_op; /* writes alternate between insertions and removals */ bool resize_down; -} QEMU_ALIGNED(64); /* avoid false sharing among threads */ +} QEMU_ALIGNED(QEMU_CACHELINE_SIZE); /* avoid false sharing among threads */ static struct qht ht; static QemuThread *rw_threads; -- 2.7.4
[Qemu-devel] [PATCH v2 0/3] tcg: allocate TB structs preceding translated code
v1: https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg00780.html Changes from v1: - Define QEMU_CACHELINE_SIZE as suggested by Richard. We try to get the value from the machine running configure, but if we fail we use some reasonable defaults. In any case the value can be overriden from --extra-cflags at configure time, which is particularly useful when cross-compiling. - Use QEMU_CACHELINE_SIZE where appropriate, namely in tests/. - In the unlikely case that code_gen_buffer_size / avg block / 8 is 0, then set tbs_size to 64K instead of just 1K, as suggested by Richard. This patchset applies on top of rth's tcg-next branch (pull-tcg-20170605 tag). NB. Apologies if some emails sent to me bounced during the last couple of days; my domain name (braap.org) was down. Thanks, Emilio
Re: [Qemu-devel] [PATCH v2 15/20] backup: Switch block_backup.h to byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Continue by converting > the public interface to backup jobs (no semantic change), including > a change to CowRequest to track by bytes instead of cluster indices. > > Note that this does not change the difference between the public > interface (starting point, and size of the subsequent range) and > the internal interface (starting and end points). > > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: change a couple more parameter names > --- > include/block/block_backup.h | 11 +-- > block/backup.c | 33 - > block/replication.c | 12 > 3 files changed, 29 insertions(+), 27 deletions(-) >
Re: [Qemu-devel] qemu-system-sh4 -M r2d serial is broken.
On 05/18/2017 06:01 PM, Aurelien Jarno wrote: > On 2017-05-18 17:37, Rob Landley wrote: >> On 05/18/2017 02:00 PM, Aurelien Jarno wrote: >>> On 2017-05-18 11:08, Rob Landley wrote: Serial input hangs after the first character in the 4.11 kernel: http://www.spinics.net/lists/linux-sh/msg51183.html Because they enabled support for a buffer size thing QEMU doesn't emulate right: http://www.spinics.net/lists/linux-sh/msg51189.html >>> >>> Indeed the SCIF emulation in QEMU is quite limited. The problem is that >>> it exposes many internal states to the software (and that's the same for >>> the SH4 CPU in general), and that's not really compatible with quick >>> emulation. In that case the timer should depend on the baud rate which >>> we don't really emulate. >>> >>> I'll try to have a look, that said my test environment is stuck with >>> kernel 4.8 due to the broken futex support on UP in kernel 4.9 (and >>> that's not QEMU specific). I'll try to build a more recent kernel with >>> additional patches. >> >> I thought Rich fixed that. Rich? > > I have sent a patch already, but TTBOMK it hasn't been applied yet. > > Aurelien I poked Rich about the futex patch again today, he's been buried up to his neck in work but has to flush his bugfix queue before -rc5 so that should get sorted this week. Also, how do I tell the kernel to read the persistent clock on r2d? Both CONFIG_RTC_DRV_R9701 (from r2d defconfig) and CONFIG_RTC_DRV_SH give error messages and fail to read anything at boot time. If you need a new test environment (simple one that doesn't use futexes that I'm aware of) https://github.com/landley/mkroot is nearing its first release. You'll need to follow the README instructions to build musl-cross-make toolchains and set up the mcm symlink, but then: ./cross.sh sh4 ./mkroot.sh kernel cd output/sh4 ./qemu-sh4.sh Should boot you to a shell prompt. And given that the root filesystem builder (mkroot.sh) is ~300 lines of bash and module/kernel is another 300 lines (mostly a big target-specific if/else staircase), it shouldn't be too hard to pull apart. :) Right now sh4 is the only target in the release list that hasn't got the full "boots to a shell prompt and exits when you type exit, clock is set to correct time, block device works, network card works" functionality list. (That's all working on arm64 armv5l armv7l i486 i686 mips mipsel powerpc s390x x86-64.) Rob
Re: [Qemu-devel] [PATCH v2 10/20] mirror: Switch mirror_cow_align() to byte-based
On 05/10/2017 10:20 PM, Eric Blake wrote: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Convert another internal > function (no semantic change), and add mirror_clip_bytes() as a > counterpart to mirror_clip_sectors(). Some of the conversion is > a bit tricky, requiring temporaries to convert between units; it > will be cleared up in a following patch. > > Signed-off-by: Eric Blake Reviewed-by: John Snow > > --- > v2: tweak mirror_clip_bytes() signature to match previous patch > ---
Re: [Qemu-devel] [PATCH v2 09/20] mirror: Update signature of mirror_clip_sectors()
On 05/10/2017 10:20 PM, Eric Blake wrote: > Rather than having a void function that modifies its input > in-place as the output, change the signature to reduce a layer > of indirection and return the result. > > Suggested-by: John Snow > Signed-off-by: Eric Blake > Reviewed-by: John Snow > --- > v2: new patch > --- > block/mirror.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) >
Re: [Qemu-devel] [PATCH v3 4/4] gdbstub: don't fail on vCont; C04:0; c packets
On 06/02/2017 10:05 AM, Alex Bennée wrote: The thread-id of 0 means any CPU but we then ignore the fact we find the first_cpu in this case who can have an index of 0. Instead of bailing out just test if we have managed to match up thread-id to a CPU. Otherwise you get: gdb_handle_packet: command='vCont;C04:0;c' put_packet: reply='E22' The actual reason for gdb sending vCont;C04:0;c was fixed in a previous commit where we ensure the first_cpu's tid is correctly reported to gdb however we should still behave correctly next time it does send 0. Signed-off-by: Alex Bennée Reviewed-by: Greg Kurz Reviewed-by: Claudio Imbrenda Reviewed-by: Philippe Mathieu-Daudé --- v2 - used Greg's less convoluted suggestion - expand commit message --- gdbstub.c | 15 --- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index 45a3a0b16b..6b1e72e9f7 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -937,23 +937,16 @@ static int gdb_handle_vcont(GDBState *s, const char *p) if (res) { goto out; } -idx = tmp; + /* 0 means any thread, so we pick the first valid CPU */ -if (!idx) { -idx = cpu_gdb_index(first_cpu); -} +cpu = tmp ? find_cpu(tmp) : first_cpu; -/* - * If we are in user mode, the thread specified is actually a - * thread id, and not an index. We need to find the actual - * CPU first, and only then we can use its index. - */ -cpu = find_cpu(idx); /* invalid CPU/thread specified */ -if (!idx || !cpu) { +if (!cpu) { res = -EINVAL; goto out; } + /* only use if no previous match occourred */ if (newstates[cpu->cpu_index] == 1) { newstates[cpu->cpu_index] = cur_action;
[Qemu-devel] [PATCH v4 1/4] qemu-io: Don't die on second open
Most callback commands in qemu-io return 0 to keep the interpreter loop running, or 1 to quit immediately. However, open_f() just passed through the return value of openfile(), which has different semantics of returning 0 if a file was opened, or 1 on any failure. As a result of mixing the return semantics, we are forcing the qemu-io interpreter to exit early on any failures, which is rather annoying when some of the failures are obviously trying to give the user a hint of how to proceed (if we didn't then kill qemu-io out from under the user's feet): $ qemu-io qemu-io> open foo qemu-io> open foo file open already, try 'help close' $ echo $? 0 In general, we WANT openfile() to report failures, since it is the function used in the form 'qemu-io -c "$something" no_such_file' for performing one or more -c options on a single file, and it is not worth attempting $something if the file itself cannot be opened. So the solution is to fix open_f() to always return 0 (when we are in interactive mode, even failure to open should not end the session), and save the return value of openfile() for command line use in main(). Note, however, that we do have some qemu-iotests that do 'qemu-io -c "open file" -c "$something"'; such tests will now proceed to attempt $something whether or not the open succeeded, the same way as if the two commands had been attempted in interactive mode. As such, the expected output for those tests has to be modified. But it also means that it is now possible to use -c close and have a single qemu-io command line operate on more than one file even without using interactive mode. Although the '-c open' action is a subtle change in behavior, remember that qemu-io is for debugging purposes, so as long as it serves the needs of qemu-iotests while still being reasonable for interactive use, it should not be a problem that we are changing tests to the new behavior. This has been awkward since at least as far back as commit e3aff4f, in 2009. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng --- v4: cover ALL qemu-iotests v3: tweak commit message to distinguish that '-c open' has a subtle new behavior from the command line v2: fix open_f(), not openfile() --- qemu-io.c | 7 --- tests/qemu-iotests/060.out | 1 + tests/qemu-iotests/114.out | 5 +++-- tests/qemu-iotests/153.out | 6 ++ 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/qemu-io.c b/qemu-io.c index 8e38b28..8074656 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char **argv) qemu_opts_reset(&empty_opts); if (optind == argc - 1) { -return openfile(argv[optind], flags, writethrough, force_share, opts); +openfile(argv[optind], flags, writethrough, force_share, opts); } else if (optind == argc) { -return openfile(NULL, flags, writethrough, force_share, opts); +openfile(NULL, flags, writethrough, force_share, opts); } else { QDECREF(opts); -return qemuio_command_usage(&open_cmd); +qemuio_command_usage(&open_cmd); } +return 0; } static int quit_f(BlockBackend *blk, int argc, char **argv) diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out index 3bc1461..5ca3af4 100644 --- a/tests/qemu-iotests/060.out +++ b/tests/qemu-iotests/060.out @@ -21,6 +21,7 @@ Format specific information: refcount bits: 16 corrupt: true can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write +no file open, try 'help open' read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) diff --git a/tests/qemu-iotests/114.out b/tests/qemu-iotests/114.out index b6d10e4..1a47a52 100644 --- a/tests/qemu-iotests/114.out +++ b/tests/qemu-iotests/114.out @@ -1,6 +1,6 @@ QA output created by 114 -Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 -Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base image: TEST_DIR/t.IMGFMT file format: IMGFMT virtual size: 64M (67108864 bytes) @@ -8,6 +8,7 @@ cluster_size: 65536 backing file: TEST_DIR/t.IMGFMT.base backing file format: foo can't open device TEST_DIR/t.qcow2: Could not open backing file: Unknown driver 'foo' +no file open, try 'help open' read 4096/4096 bytes at offset 0 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) *** done diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out index 5ba0b63..5b917b1 100644 --- a/tests/qemu-iotests/153.out +++ b/tests/qemu-iotests/153.out @@ -33,10 +33,12 @@ Is another process using the image? _qemu_io_wrapper -c open TEST_DIR/t.qcow2 -c read 0 512 can't open device TEST_DIR/t.qcow2: Failed to get "write" lock Is another process using the image? +no file open, tr
[Qemu-devel] [PATCH v4 3/4] block: Simplify use of BDRV_BLOCK_RAW
The lone caller that cares about a return of BDRV_BLOCK_RAW (namely, io.c:bdrv_co_get_block_status) completely replaces the return value, so there is no point in passing BDRV_BLOCK_DATA. Signed-off-by: Eric Blake --- v3: further document BDRV_BLOCK_RAW v2: fix subject, tweak commit message --- include/block/block.h | 6 +++--- block/commit.c| 2 +- block/mirror.c| 2 +- block/raw-format.c| 2 +- block/vpc.c | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index 9b355e9..0a60444 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -131,9 +131,9 @@ typedef struct HDGeometry { * layer (short for DATA || ZERO), set by block layer * * Internal flag: - * BDRV_BLOCK_RAW: used internally to indicate that the request was - * answered by a passthrough driver such as raw and that the - * block layer should recompute the answer from bs->file. + * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request + * that the block layer recompute the answer from the returned + * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID. * * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) * represent the offset in the returned BDS that is allocated for the diff --git a/block/commit.c b/block/commit.c index a3028b2..f56745b 100644 --- a/block/commit.c +++ b/block/commit.c @@ -239,7 +239,7 @@ static int64_t coroutine_fn bdrv_commit_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/mirror.c b/block/mirror.c index a2a9703..892d53a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1052,7 +1052,7 @@ static int64_t coroutine_fn bdrv_mirror_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/raw-format.c b/block/raw-format.c index 36e6503..1136eba 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -259,7 +259,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, *pnum = nb_sectors; *file = bs->file->bs; sector_num += s->offset / BDRV_SECTOR_SIZE; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/vpc.c b/block/vpc.c index 4240ba9..b313c68 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -701,7 +701,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, if (be32_to_cpu(footer->type) == VHD_FIXED) { *pnum = nb_sectors; *file = bs->file->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } -- 2.9.4
[Qemu-devel] [PATCH v4 4/4] blkdebug: Support .bdrv_co_get_block_status
Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by blkdebug appears 100% allocated as data. Better is treating it the same as the underlying file being wrapped. Update iotest 177 for the new expected output. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Reviewed-by: Max Reitz --- v3: rebase to earlier changes v2: tweak commit message --- block/blkdebug.c | 11 +++ tests/qemu-iotests/177.out | 5 - 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index a5196e8..1ad8d65 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -642,6 +642,16 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs, return bdrv_co_pdiscard(bs->file->bs, offset, count); } +static int64_t coroutine_fn blkdebug_co_get_block_status( +BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, +BlockDriverState **file) +{ +*pnum = nb_sectors; +*file = bs->file->bs; +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | +(sector_num << BDRV_SECTOR_BITS); +} + static void blkdebug_close(BlockDriverState *bs) { BDRVBlkdebugState *s = bs->opaque; @@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = { .bdrv_co_flush_to_disk = blkdebug_co_flush, .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes, .bdrv_co_pdiscard = blkdebug_co_pdiscard, +.bdrv_co_get_block_status = blkdebug_co_get_block_status, .bdrv_debug_event = blkdebug_debug_event, .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index fcfbfa3..43a7778 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352 read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File -0 0x800 json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"} +0 0x80TEST_DIR/t.IMGFMT +0x900x240 TEST_DIR/t.IMGFMT +0x3c0 0x110 TEST_DIR/t.IMGFMT +0x6a0 0x160 TEST_DIR/t.IMGFMT No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v4 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
We document that *file is valid if the return is not an error and includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract when a driver (such as blkdebug) lacks a callback. Messed up in commit 67a0fd2 (v2.6), when we added the file parameter. Enhance qemu-iotest 177 to cover this, using a sequence that would print garbage or even SEGV, because it was dererefencing through uninitialized memory. [The resulting test output shows that we have less-than-ideal block status from the blkdebug driver, but that's a separate fix coming up soon.] Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is enough to fix the crash, but we can go one step further: always setting *file, even on error, means that a broken caller that blindly dereferences file without checking for error is now more likely to get a reliable SEGV instead of randomly acting on garbage, making it easier to diagnose such buggy callers. Adding an assertion that file is set where expected doesn't hurt either. CC: qemu-sta...@nongnu.org Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Reviewed-by: Max Reitz --- v3: tweak commit message [Max], rebase test to pass v2: drop redundant assignment --- block/io.c | 5 +++-- tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/block/io.c b/block/io.c index ed31810..ce6cb0c 100644 --- a/block/io.c +++ b/block/io.c @@ -1736,6 +1736,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t n; int64_t ret, ret2; +*file = NULL; total_sectors = bdrv_nb_sectors(bs); if (total_sectors < 0) { return total_sectors; @@ -1756,11 +1757,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; if (bs->drv->protocol_name) { ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE); +*file = bs; } return ret; } -*file = NULL; bdrv_inc_in_flight(bs); ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum, file); @@ -1770,7 +1771,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, } if (ret & BDRV_BLOCK_RAW) { -assert(ret & BDRV_BLOCK_OFFSET_VALID); +assert(ret & BDRV_BLOCK_OFFSET_VALID && *file); ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS, *pnum, pnum, file); goto out; diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177 index 2005c17..f8ed8fb 100755 --- a/tests/qemu-iotests/177 +++ b/tests/qemu-iotests/177 @@ -43,6 +43,7 @@ _supported_proto file CLUSTER_SIZE=1M size=128M options=driver=blkdebug,image.driver=qcow2 +nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG echo echo "== setting up files ==" @@ -106,6 +107,8 @@ function verify_io() } verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io +$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \ +| _filter_qemu_img_map _check_test_img diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index e887542..fcfbfa3 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0 0x800 json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"} No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v4 0/4] more blkdebug tweaks
I found a crasher and some odd behavior while rebasing my bdrv_get_block_status series, so I figured I'd get these things fixed first. This is based on top of Max's block branch. Available as a tag at: git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v4 Since v3: - check all qemu-iotests (patch 1) 001/4:[0012] [FC] 'qemu-io: Don't die on second open' 002/4:[] [--] 'block: Guarantee that *file is set on bdrv_get_block_status()' 003/4:[] [--] 'block: Simplify use of BDRV_BLOCK_RAW' 004/4:[] [--] 'blkdebug: Support .bdrv_co_get_block_status' Eric Blake (4): qemu-io: Don't die on second open block: Guarantee that *file is set on bdrv_get_block_status() block: Simplify use of BDRV_BLOCK_RAW blkdebug: Support .bdrv_co_get_block_status include/block/block.h | 6 +++--- block/blkdebug.c | 11 +++ block/commit.c | 2 +- block/io.c | 5 +++-- block/mirror.c | 2 +- block/raw-format.c | 2 +- block/vpc.c| 2 +- qemu-io.c | 7 --- tests/qemu-iotests/060.out | 1 + tests/qemu-iotests/114.out | 5 +++-- tests/qemu-iotests/153.out | 6 ++ tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 5 + 13 files changed, 43 insertions(+), 14 deletions(-) -- 2.9.4
Re: [Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On Mon, Jun 05, 2017 at 06:01:38PM +0100, Peter Maydell wrote: > In qemu_gluster_parse_json(), the call to qdict_array_entries() > could return a negative error code, which we were ignoring > because we assigned the result to an unsigned variable. > Fix this by using the 'int' type instead, which matches the > return type of qdict_array_entries() and also the type > we use for the loop enumeration variable 'i'. > > (Spotted by Coverity, CID 1360960.) > > Signed-off-by: Peter Maydell > --- > block/gluster.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/block/gluster.c b/block/gluster.c > index 031596a..addceed 100644 > --- a/block/gluster.c > +++ b/block/gluster.c > @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster > *gconf, > Error *local_err = NULL; > char *str = NULL; > const char *ptr; > -size_t num_servers; > -int i, type; > +int i, type, num_servers; > > /* create opts info from runtime_json_opts list */ > opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); > -- > 2.7.4 > Thanks, Reviewed-by: Jeff Cody Also - applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc block -Jeff
Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally
On 05/19/2017 01:43 AM, Peter Xu wrote: > We were do the shutting off only for postcopy. Now we do this as long as > the source return path is there. > > Moving the cleanup of from_src_file there too. > > Signed-off-by: Peter Xu > --- > migration/migration.c| 8 +++- > migration/postcopy-ram.c | 1 - > 2 files changed, 7 insertions(+), 2 deletions(-) This commit causes a regression in qemu-iotests 68: $ cd tests/qemu-iotests $ ./check -qcow2 68 ... 068 1s ... - output mismatch (see 068.out.bad) --- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out 2017-05-30 09:27:26.795821748 -0500 +++ 068.out.bad 2017-06-05 15:21:18.566816816 -0500 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 1912 Segmentation fault (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done Failures: 068 Failed 1 of 1 tests I didn't investigate further; but am hoping you'll be able to fix the segfault and get the test working again. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On 06/05/2017 02:01 PM, Peter Maydell wrote: In qemu_gluster_parse_json(), the call to qdict_array_entries() could return a negative error code, which we were ignoring because we assigned the result to an unsigned variable. Fix this by using the 'int' type instead, which matches the return type of qdict_array_entries() and also the type we use for the loop enumeration variable 'i'. (Spotted by Coverity, CID 1360960.) Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé --- block/gluster.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 031596a..addceed 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, Error *local_err = NULL; char *str = NULL; const char *ptr; -size_t num_servers; -int i, type; +int i, type, num_servers; /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort);
Re: [Qemu-devel] [PATCH v1 1/1] char-socket: Don't report TCP socket waiting as an error
On 06/05/2017 03:34 PM, Alistair Francis wrote: When QEMU is waiting for a TCP socket connection it reports that message as an error. This isn't an error though, so let's change the report to just use qemu_log(). Signed-off-by: Alistair Francis Reviewed-by: Philippe Mathieu-Daudé --- chardev/char-socket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index ccc499cfa1..a9884fa85b 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -27,6 +27,7 @@ #include "io/channel-tls.h" #include "qemu/error-report.h" #include "qapi/error.h" +#include "qemu/log.h" #include "qapi/clone-visitor.h" #include "chardev/char-io.h" @@ -765,7 +766,7 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp) * in TLS and telnet cases, only wait for an accepted socket */ while (!s->ioc) { if (s->is_listen) { -error_report("QEMU waiting for connection on: %s", +qemu_log("QEMU waiting for connection on: %s", chr->filename); qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL); tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
Re: [Qemu-devel] [PATCH v3 1/4] qemu-io: Don't die on second open
On 06/05/2017 02:08 PM, Eric Blake wrote: > > Note, however, that we do have some qemu-iotests that do 'qemu-io > -c "open file" -c "$something"'; such tests will now proceed to > attempt $something whether or not the open succeeded, the same way > as if the two commands had been attempted in interactive mode; but it > also means that it is now possible to use -c close and have a single > qemu-io command line operate on more than one file even without > using interactive mode. Although the '-c open' action is a subtle > change in behavior, remember that qemu-io is for debugging purposes, > so as long as it serves the needs of qemu-iotests while still being > reasonable for interactive use, it should not be a problem. Bummer - iotest 60 catches me: +++ 060.out.bad 2017-06-05 14:55:48.336814834 -0500 @@ -21,6 +21,7 @@ refcount bits: 16 corrupt: true can't open device TEST_DIR/t.IMGFMT: IMGFMT: Image is corrupt; cannot be opened read/write +no file open, try 'help open' read 512/512 bytes at offset 0 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Looks like 114 and 153 as well. I'll have to post a v4 (serves me right for JUST testing 177). Also, I'm seeing a segfault on 68 that exists on current master: 068 1s ... - output mismatch (see 068.out.bad) --- /home/eblake/qemu/tests/qemu-iotests/068.out2017-06-01 17:01:25.113094957 -0500 +++ 068.out.bad 2017-06-05 14:55:54.140835402 -0500 @@ -6,6 +6,8 @@ QEMU X.Y.Z monitor - type 'help' for more information (qemu) savevm 0 (qemu) quit +./common.config: line 107: 1 Segmentation fault (core dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then +echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"; +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" ) QEMU X.Y.Z monitor - type 'help' for more information -(qemu) quit -*** done +(qemu) *** done -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 00/20] make bdrv_is_allocated[_above] byte-based
On 06/05/2017 03:39 PM, Eric Blake wrote: > ping > ACK. > On 05/10/2017 09:20 PM, Eric Blake wrote: >> There are patches floating around to add NBD_CMD_BLOCK_STATUS, >> but NBD wants to report status on byte granularity (even if the >> reporting will probably be naturally aligned to sectors or even >> much higher levels). I've therefore started the task of >> converting our block status code to report at a byte granularity >> rather than sectors. >> >> This is part one of that conversion: bdrv_is_allocated(). >> Other parts still need a v2, but here's the link to their v1: >> tracking dirty bitmaps by bytes: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html >> replacing bdrv_get_block_status() with a byte based callback >> in all the drivers: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02642.html >> >> Available as a tag at: >> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-allocated-v2 >> >> Since v1: >> - Add R-b from John as appropriate >> - Couple of new patches for cleanups he noticed >> - Rebase to Max's block branch >> >> 001/20:[] [--] 'blockjob: Track job ratelimits via bytes, not sectors' >> 002/20:[] [--] 'trace: Show blockjob actions via bytes, not sectors' >> 003/20:[] [--] 'stream: Switch stream_populate() to byte-based' >> 004/20:[] [--] 'stream: Switch stream_run() to byte-based' >> 005/20:[] [--] 'commit: Switch commit_populate() to byte-based' >> 006/20:[] [--] 'commit: Switch commit_run() to byte-based' >> 007/20:[] [--] 'mirror: Switch MirrorBlockJob to byte-based' >> 008/20:[] [--] 'mirror: Switch mirror_do_zero_or_discard() to byte-based' >> 009/20:[down] 'mirror: Update signature of mirror_clip_sectors()' >> 010/20:[0013] [FC] 'mirror: Switch mirror_cow_align() to byte-based' >> 011/20:[] [-C] 'mirror: Switch mirror_do_read() to byte-based' >> 012/20:[0014] [FC] 'mirror: Switch mirror_iteration() to byte-based' >> 013/20:[] [--] 'block: Drop unused bdrv_round_sectors_to_clusters()' >> 014/20:[] [--] 'backup: Switch BackupBlockJob to byte-based' >> 015/20:[0008] [FC] 'backup: Switch block_backup.h to byte-based' >> 016/20:[] [--] 'backup: Switch backup_do_cow() to byte-based' >> 017/20:[] [--] 'backup: Switch backup_run() to byte-based' >> 018/20:[0039] [FC] 'block: Make bdrv_is_allocated() byte-based' >> 019/20:[down] 'block: Minimize raw use of bds->total_sectors' >> 020/20:[0026] [FC] 'block: Make bdrv_is_allocated_above() byte-based' >> >> Eric Blake (20): >> blockjob: Track job ratelimits via bytes, not sectors >> trace: Show blockjob actions via bytes, not sectors >> stream: Switch stream_populate() to byte-based >> stream: Switch stream_run() to byte-based >> commit: Switch commit_populate() to byte-based >> commit: Switch commit_run() to byte-based >> mirror: Switch MirrorBlockJob to byte-based >> mirror: Switch mirror_do_zero_or_discard() to byte-based >> mirror: Update signature of mirror_clip_sectors() >> mirror: Switch mirror_cow_align() to byte-based >> mirror: Switch mirror_do_read() to byte-based >> mirror: Switch mirror_iteration() to byte-based >> block: Drop unused bdrv_round_sectors_to_clusters() >> backup: Switch BackupBlockJob to byte-based >> backup: Switch block_backup.h to byte-based >> backup: Switch backup_do_cow() to byte-based >> backup: Switch backup_run() to byte-based >> block: Make bdrv_is_allocated() byte-based >> block: Minimize raw use of bds->total_sectors >> block: Make bdrv_is_allocated_above() byte-based >> >> include/block/block.h| 10 +- >> include/block/block_backup.h | 11 +- >> include/qemu/ratelimit.h | 3 +- >> block/backup.c | 130 --- >> block/commit.c | 54 >> block/io.c | 92 +++-- >> block/mirror.c | 300 >> ++- >> block/replication.c | 29 +++-- >> block/stream.c | 35 +++-- >> block/vvfat.c| 34 +++-- >> migration/block.c| 9 +- >> qemu-img.c | 15 ++- >> qemu-io-cmds.c | 70 +- >> block/trace-events | 14 +- >> 14 files changed, 396 insertions(+), 410 deletions(-) >> >
Re: [Qemu-devel] [PATCH v2 00/20] make bdrv_is_allocated[_above] byte-based
ping On 05/10/2017 09:20 PM, Eric Blake wrote: > There are patches floating around to add NBD_CMD_BLOCK_STATUS, > but NBD wants to report status on byte granularity (even if the > reporting will probably be naturally aligned to sectors or even > much higher levels). I've therefore started the task of > converting our block status code to report at a byte granularity > rather than sectors. > > This is part one of that conversion: bdrv_is_allocated(). > Other parts still need a v2, but here's the link to their v1: > tracking dirty bitmaps by bytes: > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html > replacing bdrv_get_block_status() with a byte based callback > in all the drivers: > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02642.html > > Available as a tag at: > git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-allocated-v2 > > Since v1: > - Add R-b from John as appropriate > - Couple of new patches for cleanups he noticed > - Rebase to Max's block branch > > 001/20:[] [--] 'blockjob: Track job ratelimits via bytes, not sectors' > 002/20:[] [--] 'trace: Show blockjob actions via bytes, not sectors' > 003/20:[] [--] 'stream: Switch stream_populate() to byte-based' > 004/20:[] [--] 'stream: Switch stream_run() to byte-based' > 005/20:[] [--] 'commit: Switch commit_populate() to byte-based' > 006/20:[] [--] 'commit: Switch commit_run() to byte-based' > 007/20:[] [--] 'mirror: Switch MirrorBlockJob to byte-based' > 008/20:[] [--] 'mirror: Switch mirror_do_zero_or_discard() to byte-based' > 009/20:[down] 'mirror: Update signature of mirror_clip_sectors()' > 010/20:[0013] [FC] 'mirror: Switch mirror_cow_align() to byte-based' > 011/20:[] [-C] 'mirror: Switch mirror_do_read() to byte-based' > 012/20:[0014] [FC] 'mirror: Switch mirror_iteration() to byte-based' > 013/20:[] [--] 'block: Drop unused bdrv_round_sectors_to_clusters()' > 014/20:[] [--] 'backup: Switch BackupBlockJob to byte-based' > 015/20:[0008] [FC] 'backup: Switch block_backup.h to byte-based' > 016/20:[] [--] 'backup: Switch backup_do_cow() to byte-based' > 017/20:[] [--] 'backup: Switch backup_run() to byte-based' > 018/20:[0039] [FC] 'block: Make bdrv_is_allocated() byte-based' > 019/20:[down] 'block: Minimize raw use of bds->total_sectors' > 020/20:[0026] [FC] 'block: Make bdrv_is_allocated_above() byte-based' > > Eric Blake (20): > blockjob: Track job ratelimits via bytes, not sectors > trace: Show blockjob actions via bytes, not sectors > stream: Switch stream_populate() to byte-based > stream: Switch stream_run() to byte-based > commit: Switch commit_populate() to byte-based > commit: Switch commit_run() to byte-based > mirror: Switch MirrorBlockJob to byte-based > mirror: Switch mirror_do_zero_or_discard() to byte-based > mirror: Update signature of mirror_clip_sectors() > mirror: Switch mirror_cow_align() to byte-based > mirror: Switch mirror_do_read() to byte-based > mirror: Switch mirror_iteration() to byte-based > block: Drop unused bdrv_round_sectors_to_clusters() > backup: Switch BackupBlockJob to byte-based > backup: Switch block_backup.h to byte-based > backup: Switch backup_do_cow() to byte-based > backup: Switch backup_run() to byte-based > block: Make bdrv_is_allocated() byte-based > block: Minimize raw use of bds->total_sectors > block: Make bdrv_is_allocated_above() byte-based > > include/block/block.h| 10 +- > include/block/block_backup.h | 11 +- > include/qemu/ratelimit.h | 3 +- > block/backup.c | 130 --- > block/commit.c | 54 > block/io.c | 92 +++-- > block/mirror.c | 300 > ++- > block/replication.c | 29 +++-- > block/stream.c | 35 +++-- > block/vvfat.c| 34 +++-- > migration/block.c| 9 +- > qemu-img.c | 15 ++- > qemu-io-cmds.c | 70 +- > block/trace-events | 14 +- > 14 files changed, 396 insertions(+), 410 deletions(-) > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks
On 06/05/2017 02:31 PM, no-re...@patchew.org wrote: > Hi, > > This series failed automatic build test. Please find the testing commands and > their output below. If you have docker installed, you can probably reproduce > it > locally. > > GTESTER tests/test-blockjob > GTESTER tests/test-blockjob-txn > GTESTER tests/test-x86-cpuid > GTESTER tests/test-xbzrle > GTESTER tests/test-vmstate > Failed to load simple/primitive:b_1 > Failed to load simple/primitive:i64_2 > Failed to load simple/primitive:i32_1 > Failed to load simple/primitive:i32_1 > Failed to load test/with_tmp:a > Failed to load test/tmp_child_parent:f > Failed to load test/tmp_child:parent > Failed to load test/with_tmp:tmp > Failed to load test/tmp_child:diff > Failed to load test/with_tmp:tmp > Failed to load test/tmp_child:diff > Failed to load test/with_tmp:tmp > GTESTER tests/test-cutils Where is this noise coming from? > GTESTER tests/test-qmp-commands > GTESTER tests/test-string-input-visitor > GTESTER tests/test-string-output-visitor > GTESTER tests/test-qmp-event > GTESTER tests/test-opts-visitor > GTESTER tests/test-visitor-serialization > GTESTER tests/test-qht-par > ** > ERROR:/tmp/qemu-test/src/tests/test-qga.c:894:test_qga_guest_exec: assertion > failed: (exited) > socket_accept failed: Resource temporarily unavailable Looks like a false negative, or a problem with the tester? I don't see how my patch could affect test-qga. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks
Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Message-id: 20170605190824.25184-1-ebl...@redhat.com Subject: [Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks Type: series === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-mingw@fedora time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20170605190824.25184-1-ebl...@redhat.com -> patchew/20170605190824.25184-1-ebl...@redhat.com Switched to a new branch 'test' a760ce2 blkdebug: Support .bdrv_co_get_block_status adfab47 block: Simplify use of BDRV_BLOCK_RAW 7b4fbb6 block: Guarantee that *file is set on bdrv_get_block_status() b71a302 qemu-io: Don't die on second open === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-ie13aiac/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-ie13aiac/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPYRUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch gcc-4.4.7-17.el6.x86_64 git-1.7.1-4.el6_7.1.x86_64 glib2-devel-2.28.8-5.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=cd560983e1cf TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install No C++ compiler available; disabling C++ specific optional code Install prefix/var/tmp/qemu-build/install BIOS directory/var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compilercc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install pythonpython -B smbd /usr/sbin/smbd module supportno host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu tcg debug enabled no gprof enabled no sparse enabledno strip binariesyes profiler no static build no pixmansystem SDL support yes (1.2.14) GTK support no GTK GL supportno VTE support no TLS priority NORMAL GNUTLS supportno GNUTLS rndno libgcrypt no libgcrypt kdf no nettleno nettle kdfno libtasn1 no curses supportno virgl support no curl support no mingw32 support no Audio drivers oss Block whitelist (rw) Block whitelist (ro) VirtFS supportno VNC support yes VNC SASL support no VNC JPEG support no VNC PNG support no xen support no brlapi supportno
[Qemu-devel] SHPC in pci-bridge
Hi everyone, Does anyone use pci-bridge built-in SHPC and found it working (especially on x86 machines)? I really want to hear about such cases. When I'm trying to hotplug anything into the bridge without using ACPI on x86 machine, none of my linux guests can see it in lspci, and dmesg contains nothing about this insertion. Thanks -- Alexander Bezzubikov
[Qemu-devel] [PATCH v3 2/4] block: Guarantee that *file is set on bdrv_get_block_status()
We document that *file is valid if the return is not an error and includes BDRV_BLOCK_OFFSET_VALID, but forgot to obey this contract when a driver (such as blkdebug) lacks a callback. Messed up in commit 67a0fd2 (v2.6), when we added the file parameter. Enhance qemu-iotest 177 to cover this, using a sequence that would print garbage or even SEGV, because it was dererefencing through uninitialized memory. [The resulting test output shows that we have less-than-ideal block status from the blkdebug driver, but that's a separate fix coming up soon.] Setting *file on all paths that return BDRV_BLOCK_OFFSET_VALID is enough to fix the crash, but we can go one step further: always setting *file, even on error, means that a broken caller that blindly dereferences file without checking for error is now more likely to get a reliable SEGV instead of randomly acting on garbage, making it easier to diagnose such buggy callers. Adding an assertion that file is set where expected doesn't hurt either. CC: qemu-sta...@nongnu.org Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Reviewed-by: Max Reitz --- v3: tweak commit message [Max], rebase test to pass v2: drop redundant assignment --- block/io.c | 5 +++-- tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 2 ++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/block/io.c b/block/io.c index ed31810..ce6cb0c 100644 --- a/block/io.c +++ b/block/io.c @@ -1736,6 +1736,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t n; int64_t ret, ret2; +*file = NULL; total_sectors = bdrv_nb_sectors(bs); if (total_sectors < 0) { return total_sectors; @@ -1756,11 +1757,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, ret = BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED; if (bs->drv->protocol_name) { ret |= BDRV_BLOCK_OFFSET_VALID | (sector_num * BDRV_SECTOR_SIZE); +*file = bs; } return ret; } -*file = NULL; bdrv_inc_in_flight(bs); ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum, file); @@ -1770,7 +1771,7 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, } if (ret & BDRV_BLOCK_RAW) { -assert(ret & BDRV_BLOCK_OFFSET_VALID); +assert(ret & BDRV_BLOCK_OFFSET_VALID && *file); ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS, *pnum, pnum, file); goto out; diff --git a/tests/qemu-iotests/177 b/tests/qemu-iotests/177 index 2005c17..f8ed8fb 100755 --- a/tests/qemu-iotests/177 +++ b/tests/qemu-iotests/177 @@ -43,6 +43,7 @@ _supported_proto file CLUSTER_SIZE=1M size=128M options=driver=blkdebug,image.driver=qcow2 +nested_opts=image.file.driver=file,image.file.filename=$TEST_IMG echo echo "== setting up files ==" @@ -106,6 +107,8 @@ function verify_io() } verify_io | $QEMU_IO -r "$TEST_IMG" | _filter_qemu_io +$QEMU_IMG map --image-opts "$options,$nested_opts,align=4k" \ +| _filter_qemu_img_map _check_test_img diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index e887542..fcfbfa3 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -45,5 +45,7 @@ read 30408704/30408704 bytes at offset 80740352 29 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0 0x800 json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"} No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v3 3/4] block: Simplify use of BDRV_BLOCK_RAW
The lone caller that cares about a return of BDRV_BLOCK_RAW (namely, io.c:bdrv_co_get_block_status) completely replaces the return value, so there is no point in passing BDRV_BLOCK_DATA. Signed-off-by: Eric Blake --- v3: further document BDRV_BLOCK_RAW v2: fix subject, tweak commit message --- include/block/block.h | 6 +++--- block/commit.c| 2 +- block/mirror.c| 2 +- block/raw-format.c| 2 +- block/vpc.c | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/block/block.h b/include/block/block.h index 9b355e9..0a60444 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -131,9 +131,9 @@ typedef struct HDGeometry { * layer (short for DATA || ZERO), set by block layer * * Internal flag: - * BDRV_BLOCK_RAW: used internally to indicate that the request was - * answered by a passthrough driver such as raw and that the - * block layer should recompute the answer from bs->file. + * BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request + * that the block layer recompute the answer from the returned + * BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID. * * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK) * represent the offset in the returned BDS that is allocated for the diff --git a/block/commit.c b/block/commit.c index a3028b2..f56745b 100644 --- a/block/commit.c +++ b/block/commit.c @@ -239,7 +239,7 @@ static int64_t coroutine_fn bdrv_commit_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/mirror.c b/block/mirror.c index a2a9703..892d53a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1052,7 +1052,7 @@ static int64_t coroutine_fn bdrv_mirror_top_get_block_status( { *pnum = nb_sectors; *file = bs->backing->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/raw-format.c b/block/raw-format.c index 36e6503..1136eba 100644 --- a/block/raw-format.c +++ b/block/raw-format.c @@ -259,7 +259,7 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, *pnum = nb_sectors; *file = bs->file->bs; sector_num += s->offset / BDRV_SECTOR_SIZE; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } diff --git a/block/vpc.c b/block/vpc.c index 4240ba9..b313c68 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -701,7 +701,7 @@ static int64_t coroutine_fn vpc_co_get_block_status(BlockDriverState *bs, if (be32_to_cpu(footer->type) == VHD_FIXED) { *pnum = nb_sectors; *file = bs->file->bs; -return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA | +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | (sector_num << BDRV_SECTOR_BITS); } -- 2.9.4
[Qemu-devel] [PATCH v3 1/4] qemu-io: Don't die on second open
Most callback commands in qemu-io return 0 to keep the interpreter loop running, or 1 to quit immediately. However, open_f() just passed through the return value of openfile(), which has different semantics of returning 0 if a file was opened, or 1 on any failure. As a result of mixing the return semantics, we are forcing the qemu-io interpreter to exit early on any failures, which is rather annoying when some of the failures are obviously trying to give the user a hint of how to proceed (if we didn't then kill qemu-io out from under the user's feet): $ qemu-io qemu-io> open foo qemu-io> open foo file open already, try 'help close' $ echo $? 0 In general, we WANT openfile() to report failures, since it is the function used in the form 'qemu-io -c "$something" no_such_file' for performing one or more -c options on a single file, and it is not worth attempting $something if the file itself cannot be opened. So the solution is to fix open_f() to always return 0 (when we are in interactive mode, even failure to open should not end the session), and save the return value of openfile() for command line use in main(). Note, however, that we do have some qemu-iotests that do 'qemu-io -c "open file" -c "$something"'; such tests will now proceed to attempt $something whether or not the open succeeded, the same way as if the two commands had been attempted in interactive mode; but it also means that it is now possible to use -c close and have a single qemu-io command line operate on more than one file even without using interactive mode. Although the '-c open' action is a subtle change in behavior, remember that qemu-io is for debugging purposes, so as long as it serves the needs of qemu-iotests while still being reasonable for interactive use, it should not be a problem. This has been awkward since at least as far back as commit e3aff4f, in 2009. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng --- v3: tweak commit message to distinguish that '-c open' has a subtle new behavior from the command line v2: fix open_f(), not openfile() --- qemu-io.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/qemu-io.c b/qemu-io.c index 8e38b28..8074656 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -230,13 +230,14 @@ static int open_f(BlockBackend *blk, int argc, char **argv) qemu_opts_reset(&empty_opts); if (optind == argc - 1) { -return openfile(argv[optind], flags, writethrough, force_share, opts); +openfile(argv[optind], flags, writethrough, force_share, opts); } else if (optind == argc) { -return openfile(NULL, flags, writethrough, force_share, opts); +openfile(NULL, flags, writethrough, force_share, opts); } else { QDECREF(opts); -return qemuio_command_usage(&open_cmd); +qemuio_command_usage(&open_cmd); } +return 0; } static int quit_f(BlockBackend *blk, int argc, char **argv) -- 2.9.4
[Qemu-devel] [PATCH v3 4/4] blkdebug: Support .bdrv_co_get_block_status
Without a passthrough status of BDRV_BLOCK_RAW, anything wrapped by blkdebug appears 100% allocated as data. Better is treating it the same as the underlying file being wrapped. Update iotest 177 for the new expected output. Signed-off-by: Eric Blake Reviewed-by: Fam Zheng Reviewed-by: Max Reitz --- v3: rebase to earlier changes v2: tweak commit message --- block/blkdebug.c | 11 +++ tests/qemu-iotests/177.out | 5 - 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index a5196e8..1ad8d65 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -642,6 +642,16 @@ static int coroutine_fn blkdebug_co_pdiscard(BlockDriverState *bs, return bdrv_co_pdiscard(bs->file->bs, offset, count); } +static int64_t coroutine_fn blkdebug_co_get_block_status( +BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum, +BlockDriverState **file) +{ +*pnum = nb_sectors; +*file = bs->file->bs; +return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | +(sector_num << BDRV_SECTOR_BITS); +} + static void blkdebug_close(BlockDriverState *bs) { BDRVBlkdebugState *s = bs->opaque; @@ -912,6 +922,7 @@ static BlockDriver bdrv_blkdebug = { .bdrv_co_flush_to_disk = blkdebug_co_flush, .bdrv_co_pwrite_zeroes = blkdebug_co_pwrite_zeroes, .bdrv_co_pdiscard = blkdebug_co_pdiscard, +.bdrv_co_get_block_status = blkdebug_co_get_block_status, .bdrv_debug_event = blkdebug_debug_event, .bdrv_debug_breakpoint = blkdebug_debug_breakpoint, diff --git a/tests/qemu-iotests/177.out b/tests/qemu-iotests/177.out index fcfbfa3..43a7778 100644 --- a/tests/qemu-iotests/177.out +++ b/tests/qemu-iotests/177.out @@ -46,6 +46,9 @@ read 30408704/30408704 bytes at offset 80740352 read 23068672/23068672 bytes at offset 49056 22 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Offset Length File -0 0x800 json:{"image": {"driver": "IMGFMT", "file": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}, "driver": "blkdebug", "align": "4k"} +0 0x80TEST_DIR/t.IMGFMT +0x900x240 TEST_DIR/t.IMGFMT +0x3c0 0x110 TEST_DIR/t.IMGFMT +0x6a0 0x160 TEST_DIR/t.IMGFMT No errors were found on the image. *** done -- 2.9.4
[Qemu-devel] [PATCH v3 0/4] more blkdebug tweaks
I found a crasher and some odd behavior while rebasing my bdrv_get_block_status series, so I figured I'd get these things fixed first. This is based on top of Max's block branch. Available as a tag at: git fetch git://repo.or.cz/qemu/ericb.git nbd-blkdebug-status-v3 Since v2: - defer the original patch 3 to a later series - rebase to make sure test 177 passes at all times (I didn't pinpoint which recent merge caused it, but the test now produces json:{...} instead of blkdebug:... during patch 2) - add more documentation of BDRV_BLOCK_RAW - add R-b where appropriate 001/4:[] [--] 'qemu-io: Don't die on second open' 002/4:[0002] [FC] 'block: Guarantee that *file is set on bdrv_get_block_status()' 003/4:[0006] [FC] 'block: Simplify use of BDRV_BLOCK_RAW' 004/4:[0002] [FC] 'blkdebug: Support .bdrv_co_get_block_status' Eric Blake (4): qemu-io: Don't die on second open block: Guarantee that *file is set on bdrv_get_block_status() block: Simplify use of BDRV_BLOCK_RAW blkdebug: Support .bdrv_co_get_block_status include/block/block.h | 6 +++--- block/blkdebug.c | 11 +++ block/commit.c | 2 +- block/io.c | 5 +++-- block/mirror.c | 2 +- block/raw-format.c | 2 +- block/vpc.c| 2 +- qemu-io.c | 7 --- tests/qemu-iotests/177 | 3 +++ tests/qemu-iotests/177.out | 5 + 10 files changed, 33 insertions(+), 12 deletions(-) -- 2.9.4
[Qemu-devel] [PULL 10/10] scripts: Test script to look for -device crashes
Test code to check if we can crash QEMU using -device. It will test all accel/machine/device combinations by default, which may take a few hours (it's more than 90k test cases). There's a "-r" option that makes it test a random sample of combinations. The scripts contains a whitelist for: 1) known error messages that make QEMU exit cleanly; 2) known QEMU crashes. This is the behavior when the script finds a failure: * Known clean (exitcode=1) errors generate DEBUG messages (hidden by default) * Unknown clean (exitcode=1) errors will generate INFO messages (visible by default) * Known crashes generate error messages, but are not fatal (unless --strict mode is used) * Unknown crashes generate fatal error messages Having an updated whitelist of known clean errors is useful to make the script less verbose and run faster when in --quick mode, but the whitelist doesn't need to be always up to date. Signed-off-by: Eduardo Habkost Message-Id: <20170526181200.17227-4-ehabk...@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/device-crash-test | 624 ++ 1 file changed, 624 insertions(+) create mode 100755 scripts/device-crash-test diff --git a/scripts/device-crash-test b/scripts/device-crash-test new file mode 100755 index 00..5f90e9bb54 --- /dev/null +++ b/scripts/device-crash-test @@ -0,0 +1,624 @@ +#!/usr/bin/env python2.7 +# +# Copyright (c) 2017 Red Hat Inc +# +# Author: +# Eduardo Habkost +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with this program; if not, write to the Free Software Foundation, Inc., +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +""" +Run QEMU with all combinations of -machine and -device types, +check for crashes and unexpected errors. +""" + +import sys +import os +import glob +import logging +import traceback +import re +import random +import argparse +from itertools import chain + +sys.path.append(os.path.join(os.path.dirname(__file__), '..', 'scripts')) +from qemu import QEMUMachine + +logger = logging.getLogger('device-crash-test') +dbg = logger.debug + + +# Purposes of the following whitelist: +# * Avoiding verbose log messages when we find known non-fatal +# (exitcode=1) errors +# * Avoiding fatal errors when we find known crashes +# * Skipping machines/devices that are known not to work out of +# the box, when running in --quick mode +# +# Keeping the whitelist updated is desirable, but not required, +# because unexpected cases where QEMU exits with exitcode=1 will +# just trigger a INFO message. + +# Valid whitelist entry keys: +# * accel: regexp, full match only +# * machine: regexp, full match only +# * device: regexp, full match only +# * log: regexp, partial match allowed +# * exitcode: if not present, defaults to 1. If None, matches any exitcode +# * warn: if True, matching failures will be logged as warnings +# * expected: if True, QEMU is expected to always fail every time +# when testing the corresponding test case +# * loglevel: log level of log output when there's a match. +ERROR_WHITELIST = [ +# Machines that won't work out of the box: +# MACHINE | ERROR MESSAGE +{'machine':'niagara', 'expected':True}, # Unable to load a firmware for -M niagara +{'machine':'boston', 'expected':True},# Please provide either a -kernel or -bios argument +{'machine':'leon3_generic', 'expected':True}, # Can't read bios image (null) + +# devices that don't work out of the box because they require extra options to "-device DEV": +#DEVICE| ERROR MESSAGE +{'device':'.*-(i386|x86_64)-cpu', 'expected':True},# CPU socket-id is not set +{'device':'ARM,bitband-memory', 'expected':True}, # source-memory property not set +{'device':'arm.cortex-a9-global-timer', 'expected':True}, # a9_gtimer_realize: num-cpu must be between 1 and 4 +{'device':'arm_mptimer', 'expected':True}, # num-cpu must be between 1 and 4 +{'device':'armv7m', 'expected':True}, # memory property was not set +{'device':'aspeed.scu', 'expected':True}, # Unknown silicon revision: 0x0 +{'device':'aspeed.sdmc', 'expected':True}, # Unknown silicon revision: 0x0 +{'device':'bcm2835-dma', 'expected':True}, # bcm2835_dma_realize: required dma-mr link not found: Property '.dm
[Qemu-devel] [PULL 08/10] qemu.py: Don't set _popen=None on error/shutdown
Keep the Popen object around to we can query its exit code later. To keep the existing 'self._popen is None' checks working, add a is_running() method, that will check if the process is still running. Signed-off-by: Eduardo Habkost Message-Id: <20170526181200.17227-2-ehabk...@redhat.com> Reviewed-by: Markus Armbruster Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/qemu.py b/scripts/qemu.py index 6d1b6230b7..16934f1e02 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -85,8 +85,11 @@ class QEMUMachine(object): return raise +def is_running(self): +return self._popen and (self._popen.returncode is None) + def get_pid(self): -if not self._popen: +if not self.is_running(): return None return self._popen.pid @@ -128,16 +131,16 @@ class QEMUMachine(object): stderr=subprocess.STDOUT, shell=False) self._post_launch() except: -if self._popen: +if self.is_running(): self._popen.kill() +self._popen.wait() self._load_io_log() self._post_shutdown() -self._popen = None raise def shutdown(self): '''Terminate the VM and clean up''' -if not self._popen is None: +if self.is_running(): try: self._qmp.cmd('quit') self._qmp.close() @@ -149,7 +152,6 @@ class QEMUMachine(object): sys.stderr.write('qemu received signal %i: %s\n' % (-exitcode, ' '.join(self._args))) self._load_io_log() self._post_shutdown() -self._popen = None underscore_to_dash = string.maketrans('_', '-') def qmp(self, cmd, conv_keys=True, **args): -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 07/10] spapr: cleanup spapr_fixup_cpu_numa_dt() usage
From: Igor Mammedov even though spapr_fixup_cpu_numa_dt() has no effect on FDT if numa is disabled, don't call it uselessly. It makes it obvious at call sites that function is needed only when numa is enabled. Signed-off-by: Igor Mammedov Message-Id: <1496161442-96665-7-git-send-email-imamm...@redhat.com> Reviewed-by: Greg Kurz Signed-off-by: Eduardo Habkost --- hw/ppc/spapr.c | 22 ++ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 96471a5d89..70eb60efed 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -182,10 +182,8 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu, return ret; } -static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs) +static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu) { -int ret = 0; -PowerPCCPU *cpu = POWERPC_CPU(cs); int index = ppc_get_vcpu_dt_id(cpu); uint32_t associativity[] = {cpu_to_be32(0x5), cpu_to_be32(0x0), @@ -195,12 +193,8 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs) cpu_to_be32(index)}; /* Advertise NUMA via ibm,associativity */ -if (nb_numa_nodes > 1) { -ret = fdt_setprop(fdt, offset, "ibm,associativity", associativity, +return fdt_setprop(fdt, offset, "ibm,associativity", associativity, sizeof(associativity)); -} - -return ret; } /* Populate the "ibm,pa-features" property */ @@ -325,9 +319,11 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr) return ret; } -ret = spapr_fixup_cpu_numa_dt(fdt, offset, cs); -if (ret < 0) { -return ret; +if (nb_numa_nodes > 1) { +ret = spapr_fixup_cpu_numa_dt(fdt, offset, cpu); +if (ret < 0) { +return ret; +} } ret = spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt); @@ -542,7 +538,9 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset, _FDT((fdt_setprop(fdt, offset, "ibm,pft-size", pft_size_prop, sizeof(pft_size_prop; -_FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cs)); +if (nb_numa_nodes > 1) { +_FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cpu)); +} _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt)); -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 09/10] qemu.py: Add QEMUMachine.exitcode() method
Allow the exit code of QEMU to be queried by scripts. Signed-off-by: Eduardo Habkost Message-Id: <20170526181200.17227-3-ehabk...@redhat.com> Signed-off-by: Eduardo Habkost --- scripts/qemu.py | 5 + 1 file changed, 5 insertions(+) diff --git a/scripts/qemu.py b/scripts/qemu.py index 16934f1e02..880e3e8219 100644 --- a/scripts/qemu.py +++ b/scripts/qemu.py @@ -88,6 +88,11 @@ class QEMUMachine(object): def is_running(self): return self._popen and (self._popen.returncode is None) +def exitcode(self): +if self._popen is None: +return None +return self._popen.returncode + def get_pid(self): if not self.is_running(): return None -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 06/10] numa: move numa_node from CPUState into target specific classes
From: Igor Mammedov Move vcpu's associated numa_node field out of generic CPUState into inherited classes that actually care about cpu<->numa mapping, i.e: ARMCPU, PowerPCCPU, X86CPU. Signed-off-by: Igor Mammedov Message-Id: <1496161442-96665-6-git-send-email-imamm...@redhat.com> [ehabkost: s/CPU is belonging to/CPU belongs to/ on comments] Signed-off-by: Eduardo Habkost --- include/qom/cpu.h | 2 -- target/arm/cpu.h| 2 ++ target/i386/cpu.h | 1 + target/ppc/cpu.h| 1 + hw/ppc/spapr.c | 2 +- hw/ppc/spapr_cpu_core.c | 4 +++- target/arm/cpu.c| 2 +- target/i386/cpu.c | 2 +- 8 files changed, 10 insertions(+), 6 deletions(-) diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 55214ce131..89ddb686fb 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -265,7 +265,6 @@ struct qemu_work_item; * @cpu_index: CPU index (informative). * @nr_cores: Number of cores within this CPU package. * @nr_threads: Number of threads within this CPU. - * @numa_node: NUMA node this CPU is belonging to. * @host_tid: Host thread ID. * @running: #true if CPU is currently running (lockless). * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; @@ -314,7 +313,6 @@ struct CPUState { int nr_cores; int nr_threads; -int numa_node; struct QemuThread *thread; #ifdef _WIN32 diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 13da5036bc..16a1e59615 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -704,6 +704,8 @@ struct ARMCPU { ARMELChangeHook *el_change_hook; void *el_change_hook_opaque; + +int32_t node_id; /* NUMA node this CPU belongs to */ }; static inline ARMCPU *arm_env_get_cpu(CPUARMState *env) diff --git a/target/i386/cpu.h b/target/i386/cpu.h index c4602ca80d..cfe825f0a4 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1275,6 +1275,7 @@ struct X86CPU { struct kvm_msrs *kvm_msr_buf; +int32_t node_id; /* NUMA node this CPU belongs to */ int32_t socket_id; int32_t core_id; int32_t thread_id; diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 401e10e7da..d10808d9f4 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -1205,6 +1205,7 @@ struct PowerPCCPU { uint32_t compat_pvr; PPCVirtualHypervisor *vhyp; Object *intc; +int32_t node_id; /* NUMA node this CPU belongs to */ /* Fields related to migration compatibility hacks */ bool pre_2_8_migration; diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 94563cd8f5..96471a5d89 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -191,7 +191,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, CPUState *cs) cpu_to_be32(0x0), cpu_to_be32(0x0), cpu_to_be32(0x0), -cpu_to_be32(cs->numa_node), +cpu_to_be32(cpu->node_id), cpu_to_be32(index)}; /* Advertise NUMA via ibm,associativity */ diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index ff7058ecc0..029a14120e 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -184,15 +184,17 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp) for (i = 0; i < cc->nr_threads; i++) { char id[32]; CPUState *cs; +PowerPCCPU *cpu; obj = sc->threads + i * size; object_initialize(obj, size, typename); cs = CPU(obj); +cpu = POWERPC_CPU(cs); cs->cpu_index = cc->core_id + i; /* Set NUMA node for the threads belonged to core */ -cs->numa_node = sc->node_id; +cpu->node_id = sc->node_id; snprintf(id, sizeof(id), "thread[%d]", i); object_property_add_child(OBJECT(sc), id, obj, &local_err); diff --git a/target/arm/cpu.c b/target/arm/cpu.c index e748097860..82d739f832 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1587,7 +1587,7 @@ static Property arm_cpu_properties[] = { DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0), DEFINE_PROP_UINT64("mp-affinity", ARMCPU, mp_affinity, ARM64_AFFINITY_INVALID), -DEFINE_PROP_INT32("node-id", CPUState, numa_node, CPU_UNSET_NUMA_NODE_ID), +DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), DEFINE_PROP_END_OF_LIST() }; diff --git a/target/i386/cpu.c b/target/i386/cpu.c index a41d595c23..ffb5267162 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -3986,7 +3986,7 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_INT32("core-id", X86CPU, core_id, -1), DEFINE_PROP_INT32("socket-id", X86CPU, socket_id, -1), #endif -DEFINE_PROP_INT32("node-id", CPUState, numa_node, CPU_UNSET_NUMA_NODE_ID), +DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID), DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), { .name = "h
[Qemu-devel] [PULL 04/10] numa: make sure that all cpus have has_node_id set if numa is enabled
From: Igor Mammedov It fixes/add missing _PXM object for non mapped CPU (x86) and missing fdt node (virt-arm). It ensures that possible_cpus contains complete mapping if numa is enabled by the time machine_init() is executed. As result non completely mapped CPUs: 1) appear in ACPI/fdt blobs 2) QMP query-hotpluggable-cpus command shows bound nodes for such CPUs 3) allows to drop checks for has_node_id in numa only code, reducing number of invariants incomplete mapping could produce 4) moves fixup/implicit node init from runtime numa_cpu_pre_plug() (when CPU object is created) to machine_numa_finish_init() which helps to fix [1, 2] and make possible_cpus complete source of numa mapping available even before CPUs are created. Signed-off-by: Igor Mammedov Message-Id: <1496161442-96665-4-git-send-email-imamm...@redhat.com> Signed-off-by: Eduardo Habkost --- hw/arm/virt-acpi-build.c | 4 +--- hw/core/machine.c| 16 ++-- hw/i386/acpi-build.c | 3 +-- hw/i386/pc.c | 4 +--- numa.c | 16 +--- 5 files changed, 18 insertions(+), 25 deletions(-) diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 2079828c22..3d78ff68e6 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -496,12 +496,10 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) srat->reserved1 = cpu_to_le32(1); for (i = 0; i < cpu_list->len; ++i) { -int node_id = cpu_list->cpus[i].props.has_node_id ? -cpu_list->cpus[i].props.node_id : 0; core = acpi_data_push(table_data, sizeof(*core)); core->type = ACPI_SRAT_PROCESSOR_GICC; core->length = sizeof(*core); -core->proximity = cpu_to_le32(node_id); +core->proximity = cpu_to_le32(cpu_list->cpus[i].props.node_id); core->acpi_processor_uid = cpu_to_le32(i); core->flags = cpu_to_le32(1); } diff --git a/hw/core/machine.c b/hw/core/machine.c index c1434e6a69..2e7e9778cd 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -721,19 +721,23 @@ static void machine_numa_finish_init(MachineState *machine) const CPUArchId *cpu_slot = &possible_cpus->cpus[i]; if (!cpu_slot->props.has_node_id) { -if (default_mapping) { -/* fetch default mapping from board and enable it */ -CpuInstanceProperties props = cpu_slot->props; -props.has_node_id = true; -machine_set_cpu_numa_node(machine, &props, &error_fatal); -} else { +/* fetch default mapping from board and enable it */ +CpuInstanceProperties props = cpu_slot->props; + +if (!default_mapping) { /* record slots with not set mapping, * TODO: make it hard error in future */ char *cpu_str = cpu_slot_to_string(cpu_slot); g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i, cpu_str); g_free(cpu_str); + +/* non mapped cpus used to fallback to node 0 */ +props.node_id = 0; } + +props.has_node_id = true; +machine_set_cpu_numa_node(machine, &props, &error_fatal); } } if (s->len && !qtest_enabled()) { diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 82bd44f38e..ce74c84460 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2335,8 +2335,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine) srat->reserved1 = cpu_to_le32(1); for (i = 0; i < apic_ids->len; i++) { -int node_id = apic_ids->cpus[i].props.has_node_id ? -apic_ids->cpus[i].props.node_id : 0; +int node_id = apic_ids->cpus[i].props.node_id; uint32_t apic_id = apic_ids->cpus[i].arch_id; if (apic_id < 255) { diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 5b07be61cf..5b8c6fbbea 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -788,9 +788,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms) for (i = 0; i < cpus->len; i++) { unsigned int apic_id = cpus->cpus[i].arch_id; assert(apic_id < pcms->apic_id_limit); -if (cpus->cpus[i].props.has_node_id) { -numa_fw_cfg[apic_id + 1] = cpu_to_le64(cpus->cpus[i].props.node_id); -} +numa_fw_cfg[apic_id + 1] = cpu_to_le64(cpus->cpus[i].props.node_id); } for (i = 0; i < nb_numa_nodes; i++) { numa_fw_cfg[pcms->apic_id_limit + 1 + i] = diff --git a/numa.c b/numa.c index 4ec3eaf687..65701cb6c8 100644 --- a/numa.c +++ b/numa.c @@ -509,22 +509,16 @@ void parse_numa_opts(MachineState *ms) void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp) { -int mapped_node_id; /* set by -numa option */ int node_id = object_property_get_int(OBJECT(dev), "
[Qemu-devel] [PULL 05/10] numa: make hmp 'info numa' fetch numa nodes from qmp_query_cpus() result
From: Igor Mammedov HMP command 'info numa' is the last external user that access CPUState::numa_node field directly. In order to move it to CPU classes that actually use it, eliminate direct access and use an alternative approach by using result of qmp_query_cpus(), which provides topology properties CPU threads are associated with (including node-id). Signed-off-by: Igor Mammedov Message-Id: <1496161442-96665-5-git-send-email-imamm...@redhat.com> Signed-off-by: Eduardo Habkost --- monitor.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/monitor.c b/monitor.c index 75e7cd26d0..1e63ace2d4 100644 --- a/monitor.c +++ b/monitor.c @@ -1696,23 +1696,26 @@ static void hmp_info_mtree(Monitor *mon, const QDict *qdict) static void hmp_info_numa(Monitor *mon, const QDict *qdict) { int i; -CPUState *cpu; uint64_t *node_mem; +CpuInfoList *cpu_list, *cpu; +cpu_list = qmp_query_cpus(&error_abort); node_mem = g_new0(uint64_t, nb_numa_nodes); query_numa_node_mem(node_mem); monitor_printf(mon, "%d nodes\n", nb_numa_nodes); for (i = 0; i < nb_numa_nodes; i++) { monitor_printf(mon, "node %d cpus:", i); -CPU_FOREACH(cpu) { -if (cpu->numa_node == i) { -monitor_printf(mon, " %d", cpu->cpu_index); +for (cpu = cpu_list; cpu; cpu = cpu->next) { +if (cpu->value->has_props && cpu->value->props->has_node_id && +cpu->value->props->node_id == i) { +monitor_printf(mon, " %" PRIi64, cpu->value->CPU); } } monitor_printf(mon, "\n"); monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i, node_mem[i] >> 20); } +qapi_free_CpuInfoList(cpu_list); g_free(node_mem); } -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 03/10] numa: move default mapping init to machine
From: Igor Mammedov there is no need use cpu_index_to_instance_props() for setting default cpu -> node mapping. Generic machine code can do it without cpu_index by just enabling already preset defaults in possible_cpus. PS: as bonus it makes one less user of cpu_index_to_instance_props() Signed-off-by: Igor Mammedov Message-Id: <1496161442-96665-3-git-send-email-imamm...@redhat.com> Signed-off-by: Eduardo Habkost --- hw/core/machine.c | 33 +++-- numa.c| 26 -- 2 files changed, 23 insertions(+), 36 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index 3adebf14c4..c1434e6a69 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -701,26 +701,39 @@ static char *cpu_slot_to_string(const CPUArchId *cpu) return g_string_free(s, false); } -static void machine_numa_validate(MachineState *machine) +static void machine_numa_finish_init(MachineState *machine) { int i; +bool default_mapping; GString *s = g_string_new(NULL); MachineClass *mc = MACHINE_GET_CLASS(machine); const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine); assert(nb_numa_nodes); for (i = 0; i < possible_cpus->len; i++) { +if (possible_cpus->cpus[i].props.has_node_id) { +break; +} +} +default_mapping = (i == possible_cpus->len); + +for (i = 0; i < possible_cpus->len; i++) { const CPUArchId *cpu_slot = &possible_cpus->cpus[i]; -/* at this point numa mappings are initilized by CLI options - * or with default mappings so it's sufficient to list - * all not yet mapped CPUs here */ -/* TODO: make it hard error in future */ if (!cpu_slot->props.has_node_id) { -char *cpu_str = cpu_slot_to_string(cpu_slot); -g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i, - cpu_str); -g_free(cpu_str); +if (default_mapping) { +/* fetch default mapping from board and enable it */ +CpuInstanceProperties props = cpu_slot->props; +props.has_node_id = true; +machine_set_cpu_numa_node(machine, &props, &error_fatal); +} else { +/* record slots with not set mapping, + * TODO: make it hard error in future */ +char *cpu_str = cpu_slot_to_string(cpu_slot); +g_string_append_printf(s, "%sCPU %d [%s]", + s->len ? ", " : "", i, cpu_str); +g_free(cpu_str); +} } } if (s->len && !qtest_enabled()) { @@ -738,7 +751,7 @@ void machine_run_board_init(MachineState *machine) MachineClass *machine_class = MACHINE_GET_CLASS(machine); if (nb_numa_nodes) { -machine_numa_validate(machine); +machine_numa_finish_init(machine); } machine_class->init(machine); } diff --git a/numa.c b/numa.c index 1c35a79b57..4ec3eaf687 100644 --- a/numa.c +++ b/numa.c @@ -426,7 +426,6 @@ void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, void parse_numa_opts(MachineState *ms) { int i; -const CPUArchIdList *possible_cpus; MachineClass *mc = MACHINE_GET_CLASS(ms); if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) { @@ -484,31 +483,6 @@ void parse_numa_opts(MachineState *ms) numa_set_mem_ranges(); -/* assign CPUs to nodes using board provided default mapping */ -if (!mc->cpu_index_to_instance_props || !mc->possible_cpu_arch_ids) { -error_report("default CPUs to NUMA node mapping isn't supported"); -exit(1); -} - -possible_cpus = mc->possible_cpu_arch_ids(ms); -for (i = 0; i < possible_cpus->len; i++) { -if (possible_cpus->cpus[i].props.has_node_id) { -break; -} -} - -/* no CPUs are assigned to NUMA nodes */ -if (i == possible_cpus->len) { -for (i = 0; i < max_cpus; i++) { -CpuInstanceProperties props; -/* fetch default mapping from board and enable it */ -props = mc->cpu_index_to_instance_props(ms, i); -props.has_node_id = true; - -machine_set_cpu_numa_node(ms, &props, &error_fatal); -} -} - /* QEMU needs at least all unique node pair distances to build * the whole NUMA distance table. QEMU treats the distance table * as symmetric by default, i.e. distance A->B == distance B->A. -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 00/10] x86 and machine queue, 2017-06-05
The following changes since commit cb8b8ef4578dc17c350fd4b27700a9f178e2dad0: Merge remote-tracking branch 'remotes/elmarco/tags/chrfe-pull-request' into staging (2017-06-05 10:09:14 +0100) are available in the git repository at: git://github.com/ehabkost/qemu.git tags/x86-and-machine-pull-request for you to fetch changes up to 23ea4f30320bbd36a5d202ee469374ec3c747286: scripts: Test script to look for -device crashes (2017-06-05 14:59:09 -0300) x86 and machine queue, 2017-06-05 Eduardo Habkost (4): pc: Use "min-[x]level" on compat_props qemu.py: Don't set _popen=None on error/shutdown qemu.py: Add QEMUMachine.exitcode() method scripts: Test script to look for -device crashes Igor Mammedov (6): numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr numa: move default mapping init to machine numa: make sure that all cpus have has_node_id set if numa is enabled numa: make hmp 'info numa' fetch numa nodes from qmp_query_cpus() result numa: move numa_node from CPUState into target specific classes spapr: cleanup spapr_fixup_cpu_numa_dt() usage scripts/qemu.py | 17 +- include/hw/i386/pc.h | 42 +-- include/qom/cpu.h | 2 - include/sysemu/numa.h | 1 + target/arm/cpu.h | 2 + target/i386/cpu.h | 1 + target/ppc/cpu.h | 1 + hw/arm/virt-acpi-build.c | 4 +- hw/arm/virt.c | 16 +- hw/core/machine.c | 37 ++- hw/i386/acpi-build.c | 3 +- hw/i386/pc.c | 21 +- hw/ppc/spapr.c| 41 +-- hw/ppc/spapr_cpu_core.c | 4 +- monitor.c | 11 +- numa.c| 43 ++- target/arm/cpu.c | 2 +- target/i386/cpu.c | 2 +- tests/test-x86-cpuid-compat.c | 38 +++ scripts/device-crash-test | 624 ++ 20 files changed, 774 insertions(+), 138 deletions(-) create mode 100755 scripts/device-crash-test -- 2.11.0.259.g40922b1
[Qemu-devel] [PULL 02/10] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr
From: Igor Mammedov Signed-off-by: Igor Mammedov Reviewed-by: David Gibson Message-Id: <1496161442-96665-2-git-send-email-imamm...@redhat.com> [ehabkost: Fix indentation] Signed-off-by: Eduardo Habkost --- include/sysemu/numa.h | 1 + hw/arm/virt.c | 16 ++-- hw/i386/pc.c | 17 + hw/ppc/spapr.c| 17 + numa.c| 23 +++ 5 files changed, 28 insertions(+), 46 deletions(-) diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h index 7ffde5b119..610eece211 100644 --- a/include/sysemu/numa.h +++ b/include/sysemu/numa.h @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, int nb_nodes, ram_addr_t size); void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes, int nb_nodes, ram_addr_t size); +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp); #endif diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 4db2d4207c..010f7244bf 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1372,7 +1372,6 @@ static void machvirt_init(MachineState *machine) for (n = 0; n < possible_cpus->len; n++) { Object *cpuobj; CPUState *cs; -int node_id; if (n >= smp_cpus) { break; @@ -1385,19 +1384,8 @@ static void machvirt_init(MachineState *machine) cs = CPU(cpuobj); cs->cpu_index = n; -node_id = possible_cpus->cpus[cs->cpu_index].props.node_id; -if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) { -/* by default CPUState::numa_node was 0 if it's not set via CLI - * keep it this way for now but in future we probably should - * refuse to start up with incomplete numa mapping */ - node_id = 0; -} -if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) { -cs->numa_node = node_id; -} else { -/* CPU isn't device_add compatible yet, this shouldn't happen */ -error_setg(&error_abort, "user set node-id not implemented"); -} +numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), + &error_fatal); if (!vms->secure) { object_property_set_bool(cpuobj, false, "has_el3", NULL); diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 107a34125b..5b07be61cf 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1893,7 +1893,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { int idx; -int node_id; CPUState *cs; CPUArchId *cpu_slot; X86CPUTopoInfo topo; @@ -1984,21 +1983,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, cs = CPU(cpu); cs->cpu_index = idx; -node_id = cpu_slot->props.node_id; -if (!cpu_slot->props.has_node_id) { -/* by default CPUState::numa_node was 0 if it's not set via CLI - * keep it this way for now but in future we probably should - * refuse to start up with incomplete numa mapping */ -node_id = 0; -} -if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) { -cs->numa_node = node_id; -} else if (cs->numa_node != node_id) { -error_setg(errp, "node-id %d must match numa node specified" -"with -numa option for cpu-index %d", -cs->numa_node, cs->cpu_index); -return; -} +numa_cpu_pre_plug(cpu_slot, dev, errp); } static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ab3aab1279..94563cd8f5 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2922,11 +2922,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev); Error *local_err = NULL; CPUCore *cc = CPU_CORE(dev); -sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev); char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model); const char *type = object_get_typename(OBJECT(dev)); CPUArchId *core_slot; -int node_id; int index; if (dev->hotplugged && !mc->has_hotpluggable_cpus) { @@ -2967,20 +2965,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, goto out; } -node_id = core_slot->props.node_id; -if (!core_slot->props.has_node_id) { -/* by default CPUState::numa_node was 0 if it's not set via CLI - * keep it this way for now but in future we probably should - * refuse to start up with incomplete numa mapping */ -node_id = 0; -} -if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) { -sc->node_id = node_id; -} else if (sc->node_id != node_id) { -error_setg(&local_err, "node-id %d must match numa node specified" -"with -numa option for cpu-index %d"
[Qemu-devel] [PULL 01/10] pc: Use "min-[x]level" on compat_props
Since the automatic cpuid-level code was introduced in commit c39c0edf9bb3b968ba95484465a50c7b19f4aa3a ("target-i386: Automatically set level/xlevel/xlevel2 when needed"), the CPU model tables just define the default CPUID level code (set using "min-level"). Setting "[x]level" forces CPUID level to a specific value and disable the automatic-level logic. But the PC compat code was not updated and the existing "[x]level" compat properties broke compatibility for people using features that triggered the auto-level code. To keep previous behavior, we should set "min-[x]level" instead of "[x]level" on compat_props. This was not a problem for most cases, because old machine-types don't have full-cpuid-auto-level enabled. The only common use case it broke was the CPUID[7] auto-level code, that was already enabled since the first CPUID[7] feature was introduced (in QEMU 1.4.0). This causes the regression reported at: https://bugzilla.redhat.com/show_bug.cgi?id=1454641 Change the PC compat code to use "min-[x]level" instead of "[x]level" on compat_props, and add new test cases to ensure we don't break this again. Reported-by: "Guo, Zhiyi" Fixes: c39c0edf9bb ("target-i386: Automatically set level/xlevel/xlevel2 when needed") Cc: qemu-sta...@nongnu.org Acked-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- include/hw/i386/pc.h | 42 +- tests/test-x86-cpuid-compat.c | 38 ++ 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index e447f5d8f4..d071c9c0e9 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -566,75 +566,75 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); .value= "off",\ },{\ .driver = "qemu64" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(4),\ },{\ .driver = "kvm64" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(5),\ },{\ .driver = "pentium3" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(2),\ },{\ .driver = "n270" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(5),\ },{\ .driver = "Conroe" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(4),\ },{\ .driver = "Penryn" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(4),\ },{\ .driver = "Nehalem" "-" TYPE_X86_CPU,\ -.property = "level",\ +.property = "min-level",\ .value= stringify(4),\ },{\ .driver = "n270" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Penryn" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Conroe" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Nehalem" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Westmere" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "SandyBridge" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "IvyBridge" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Haswell" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Haswell-noTSX" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Broadwell" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = "Broadwell-noTSX" "-" TYPE_X86_CPU,\ -.property = "xlevel",\ +.property = "min-xlevel",\ .value= stringify(0x800a),\ },{\ .driver = TYPE_X86_CPU,\ @@ -860,7 +860,7 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); .value= st
Re: [Qemu-devel] [PATCH] pc: Use "min-[x]level" on compat_props
On Mon, Jun 05, 2017 at 07:07:21PM +0300, Michael S. Tsirkin wrote: [...] > > With above tweaks: > > Acked-by: Michael S. Tsirkin > > feel free to merge this through your tree. Thanks. Queued on my x86-next branch. -- Eduardo
[Qemu-devel] [PATCH v1 1/1] char-socket: Don't report TCP socket waiting as an error
When QEMU is waiting for a TCP socket connection it reports that message as an error. This isn't an error though, so let's change the report to just use qemu_log(). Signed-off-by: Alistair Francis --- chardev/char-socket.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chardev/char-socket.c b/chardev/char-socket.c index ccc499cfa1..a9884fa85b 100644 --- a/chardev/char-socket.c +++ b/chardev/char-socket.c @@ -27,6 +27,7 @@ #include "io/channel-tls.h" #include "qemu/error-report.h" #include "qapi/error.h" +#include "qemu/log.h" #include "qapi/clone-visitor.h" #include "chardev/char-io.h" @@ -765,7 +766,7 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp) * in TLS and telnet cases, only wait for an accepted socket */ while (!s->ioc) { if (s->is_listen) { -error_report("QEMU waiting for connection on: %s", +qemu_log("QEMU waiting for connection on: %s", chr->filename); qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL); tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr); -- 2.11.0
Re: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob()
Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1496679576-14336-1-git-send-email-peter.mayd...@linaro.org Subject: [Qemu-devel] [PATCH 0/2] slirp: handle errors in sosendoob() Type: series === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 9af1ed0 slirp: Handle error returns from sosendoob() ba36c86 slirp: Handle error returns from slirp_send() in sosendoob() === OUTPUT BEGIN === Checking PATCH 1/2: slirp: Handle error returns from slirp_send() in sosendoob()... ERROR: code indent should never use tabs #33: FILE: slirp/socket.c:354: +^I^Iuint32_t urgc = so->so_urgc;$ ERROR: code indent should never use tabs #36: FILE: slirp/socket.c:356: +^I^Iif (len > urgc) {$ ERROR: code indent should never use tabs #37: FILE: slirp/socket.c:357: +^I^I^Ilen = urgc;$ ERROR: code indent should never use tabs #38: FILE: slirp/socket.c:358: +^I^I}$ ERROR: code indent should never use tabs #42: FILE: slirp/socket.c:360: +^I^Iurgc -= len;$ ERROR: code indent should never use tabs #43: FILE: slirp/socket.c:361: +^I^Iif (urgc) {$ ERROR: code indent should never use tabs #46: FILE: slirp/socket.c:363: +^I^I^Iif (n > urgc) {$ ERROR: code indent should never use tabs #47: FILE: slirp/socket.c:364: +^I^I^I^In = urgc;$ ERROR: code indent should never use tabs #48: FILE: slirp/socket.c:365: +^I^I^I}$ ERROR: code indent should never use tabs #54: FILE: slirp/socket.c:370: +^I}$ ERROR: code indent should never use tabs #59: FILE: slirp/socket.c:373: +^Iif (n != len) {$ ERROR: code indent should never use tabs #60: FILE: slirp/socket.c:374: +^I^IDEBUG_ERROR((dfd, "Didn't send all data urgently X\n"));$ ERROR: code indent should never use tabs #61: FILE: slirp/socket.c:375: +^I}$ ERROR: code indent should never use tabs #64: FILE: slirp/socket.c:377: +^Iif (n < 0) {$ ERROR: code indent should never use tabs #65: FILE: slirp/socket.c:378: +^I^Ireturn n;$ ERROR: code indent should never use tabs #67: FILE: slirp/socket.c:380: +^Iso->so_urgc -= n;$ ERROR: line over 90 characters #68: FILE: slirp/socket.c:381: + DEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc)); ERROR: code indent should never use tabs #68: FILE: slirp/socket.c:381: +^IDEBUG_MISC((dfd, " ---2 sent %d bytes urgent data, %d urgent bytes left\n", n, so->so_urgc));$ total: 18 errors, 0 warnings, 51 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 2/2: slirp: Handle error returns from sosendoob()... ERROR: code indent should never use tabs #25: FILE: slirp/sbuf.c:94: +^I^I(void)sosendoob(so);$ ERROR: code indent should never use tabs #38: FILE: slirp/socket.c:407: +^I^Iif (sosendoob(so) < so->so_urgc) {$ ERROR: code indent should never use tabs #39: FILE: slirp/socket.c:408: +^I^I^I/* Treat a short write as a fatal error too,$ ERROR: code indent should never use tabs #40: FILE: slirp/socket.c:409: +^I^I^I * rather than continuing on and sending the urgent$ ERROR: code indent should never use tabs #41: FILE: slirp/socket.c:410: +^I^I^I * data as if it were non-urgent and leaving the$ ERROR: code indent should never use tabs #42: FILE: slirp/socket.c:411: +^I^I^I * so_urgc count wrong.$ ERROR: code indent should never use tabs #43: FILE: slirp/socket.c:412: +^I^I^I */$ ERROR: code indent should never use tabs #44: FILE: slirp/socket.c:413: +^I^I^Igoto err_disconnected;$ ERROR: code indent should never use tabs #45: FILE: slirp/socket.c:414: +^I^I}$ ERROR: code indent should never use tabs #58: FILE: slirp/socket.c:458: +^I^Igoto err_disconnected;$ WARNING: line over 80 characters #68: FILE: slirp/socket.c:487: + DEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n", ERROR: code indent should never use tabs #68: FILE: slirp/socket.c:487: +^IDEBUG_MISC((dfd, " --- sowrite disconnected, so->so_state = %x, errno = %d\n",$ ERROR: code indent should never use tabs #69: FILE: slirp/socket.c:488: +^I^Iso->so_state, errno));$ ERROR: code indent should never use tabs #70: FILE: slirp/socket.c:489: +^Isofcantsendmore(so);$ ERROR: code indent should never use tabs #71: FILE: slirp/socket.c:490: +^Itcp_sockclosed(sototcpcb(so));$ ERROR: code indent should never use tabs #72: FILE: slirp/socket.c:491: +^Ireturn -1;$ total: 15 errors, 1 warnings, 48 line
Re: [Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
On 06/05/2017 12:01 PM, Peter Maydell wrote: > In qemu_gluster_parse_json(), the call to qdict_array_entries() > could return a negative error code, which we were ignoring > because we assigned the result to an unsigned variable. > Fix this by using the 'int' type instead, which matches the > return type of qdict_array_entries() and also the type > we use for the loop enumeration variable 'i'. > > (Spotted by Coverity, CID 1360960.) > > Signed-off-by: Peter Maydell > --- > block/gluster.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] pc: Use "min-[x]level" on compat_props
On Mon, Jun 05, 2017 at 12:56:55PM -0300, Eduardo Habkost wrote: > Since the automatic cpuid-level code was introduced in commit > c39c0edf9bb3b968ba95484465a50c7b19f4aa3a, the CPU model tables just > define the default CPUID level code (set using "min-level"). Setting > "[x]level" forces CPUID level to a specific value and disable the > automatic-level logic. > > But the PC compat code was not updated and the existing "[x]level" > compat properties broke compatibility for people using features that > triggered the auto-level code. To keep previous behavior, we should set > "min-[x]level" instead of "[x]level" on compat_props. > > This was not a problem for most cases, because old machine-types don't > have full-cpuid-auto-level enabled. The only common use case it broke > was the CPUID[7] auto-level code, that was already enabled since the > first CPUID[7] feature was introduced (in QEMU 1.4.0). > > This causes the regression reported at: > https://bugzilla.redhat.com/show_bug.cgi?id=1454641 > > Change the PC compat code to use "min-[x]level" instead of "[x]level" on > compat_props, and add new test cases to ensure we don't break this > again. > > Reported-by: "Guo, Zhiyi" > Signd-off-by: Eduardo Habkost Signed-off-by: Eduardo Habkost -- Eduardo
[Qemu-devel] [PATCH v3] target-ppc: Enable open-pic timers to count and generate interrupts
Previously QEMU open-pic implemented the 4 open-pic timers including all timer registers, but the timers did not "count" or generate any interrupts. The patch makes the timers both count and generate interrupts. The timer clock frequency is fixed at 25MHZ. -- Responding to V2 patch comments. - Simplify clock frequency logic and commentary. - Remove camelCase variables. - Timer objects now created at init rather than lazily. Signed-off-by: Aaron Larson --- hw/intc/openpic.c | 117 +++--- 1 file changed, 111 insertions(+), 6 deletions(-) diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index f966d06..0dec51c 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -45,6 +45,7 @@ #include "qemu/bitops.h" #include "qapi/qmp/qerror.h" #include "qemu/log.h" +#include "qemu/timer.h" //#define DEBUG_OPENPIC @@ -54,8 +55,10 @@ static const int debug_openpic = 1; static const int debug_openpic = 0; #endif +static int get_current_cpu(void); #define DPRINTF(fmt, ...) do { \ if (debug_openpic) { \ +printf("Core%d: ", get_current_cpu()); \ printf(fmt , ## __VA_ARGS__); \ } \ } while (0) @@ -246,9 +249,31 @@ typedef struct IRQSource { #define IDR_EP 0x8000 /* external pin */ #define IDR_CI 0x4000 /* critical interrupt */ +/* Convert between openpic clock ticks and nanosecs. In the hardware the clock + frequency is driven by board inputs to the PIC which the PIC would then + divide by 4 or 8. For now hard code to 25MZ. +*/ +#define OPENPIC_TIMER_FREQ_MHZ 25 +#define OPENPIC_TIMER_NS_PER_TICK (1000 / OPENPIC_TIMER_FREQ_MHZ) +static inline uint64_t ns_to_ticks(uint64_t ns) +{ +return ns/ OPENPIC_TIMER_NS_PER_TICK; +} +static inline uint64_t ticks_to_ns(uint64_t ticks) +{ +return ticks * OPENPIC_TIMER_NS_PER_TICK; +} + typedef struct OpenPICTimer { uint32_t tccr; /* Global timer current count register */ uint32_t tbcr; /* Global timer base count register */ +int n_IRQ; +bool qemu_timer_active; /* Is the qemu_timer is running? */ +struct QEMUTimer *qemu_timer; +struct OpenPICState *opp; /* Device timer is part of. */ +/* The QEMU_CLOCK_VIRTUAL time (in ns) corresponding to the last + current_count written or read, only defined if qemu_timer_active. */ +uint64_t origin_time; } OpenPICTimer; typedef struct OpenPICMSI { @@ -795,6 +820,65 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr addr, unsigned len) return retval; } +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled); + +static void qemu_timer_cb(void *opaque) +{ +OpenPICTimer *tmr = opaque; +OpenPICState *opp = tmr->opp; +uint32_tn_IRQ = tmr->n_IRQ; +uint32_t val = tmr->tbcr & ~TBCR_CI; +uint32_t tog = ((tmr->tccr & TCCR_TOG) ^ TCCR_TOG); /* invert toggle. */ + +DPRINTF("%s n_IRQ=%d\n", __func__, n_IRQ); +/* Reload current count from base count and setup timer. */ +tmr->tccr = val | tog; +openpic_tmr_set_tmr(tmr, val, /*enabled=*/true); +/* Raise the interrupt. */ +opp->src[n_IRQ].destmask = read_IRQreg_idr(opp, n_IRQ); +openpic_set_irq(opp, n_IRQ, 1); +openpic_set_irq(opp, n_IRQ, 0); +} + +/* If enabled is true, arranges for an interrupt to be raised val clocks into + the future, if enabled is false cancels the timer. */ +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool enabled) +{ +uint64_t ns = ticks_to_ns(val & ~TCCR_TOG); +/* A count of zero causes a timer to be set to expire immediately. This + effectively stops the simulation since the timer is constantly expiring + which prevents guest code execution, so we don't honor that + configuration. On real hardware, this situation would generate an + interrupt on every clock cycle if the interrupt was unmasked. */ +if ((ns == 0) || !enabled) { +tmr->qemu_timer_active = false; +tmr->tccr = tmr->tccr & TCCR_TOG; +timer_del(tmr->qemu_timer); /* set timer to never expire. */ +} else { +tmr->qemu_timer_active = true; +uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); +tmr->origin_time = now; +timer_mod(tmr->qemu_timer, now + ns); /* set timer expiration. */ +} +} + +/* Returns the currrent tccr value, i.e., timer value (in clocks) with + appropriate TOG. */ +static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr) +{ +uint64_t retval; +if (!tmr->qemu_timer_active) { +retval = tmr->tccr; +} else { +uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); +uint64_t used = now - tmr->origin_time; /* nsecs */ +uint32_t used_ticks = (uint32_t)ns_to_ticks(used); +uint32_t count = (tmr->tccr & ~TCCR_TOG) - used_ticks; +retval = (uint32_t)((tmr->tccr & TCCR_TOG) | (count & ~TC
[Qemu-devel] [PULL 17/26] tcg/arm: Implement goto_ptr
Signed-off-by: Richard Henderson --- tcg/arm/tcg-target.h | 2 +- tcg/arm/tcg-target.inc.c | 25 + 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h index c114df7..5ef1086 100644 --- a/tcg/arm/tcg-target.h +++ b/tcg/arm/tcg-target.h @@ -123,7 +123,7 @@ extern bool use_idiv_instructions; #define TCG_TARGET_HAS_mulsh_i320 #define TCG_TARGET_HAS_div_i32 use_idiv_instructions #define TCG_TARGET_HAS_rem_i32 0 -#define TCG_TARGET_HAS_goto_ptr 0 +#define TCG_TARGET_HAS_goto_ptr 1 enum { TCG_AREG0 = TCG_REG_R6, diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c index 590c57d..9f5cb66 100644 --- a/tcg/arm/tcg-target.inc.c +++ b/tcg/arm/tcg-target.inc.c @@ -1655,8 +1655,14 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, switch (opc) { case INDEX_op_exit_tb: -tcg_out_movi32(s, COND_AL, TCG_REG_R0, args[0]); -tcg_out_goto(s, COND_AL, tb_ret_addr); +/* Reuse the zeroing that exists for goto_ptr. */ +a0 = args[0]; +if (a0 == 0) { +tcg_out_goto(s, COND_AL, s->code_gen_epilogue); +} else { +tcg_out_movi32(s, COND_AL, TCG_REG_R0, args[0]); +tcg_out_goto(s, COND_AL, tb_ret_addr); +} break; case INDEX_op_goto_tb: if (s->tb_jmp_insn_offset) { @@ -1671,6 +1677,9 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, } s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s); break; +case INDEX_op_goto_ptr: +tcg_out_bx(s, COND_AL, args[0]); +break; case INDEX_op_br: tcg_out_goto_label(s, COND_AL, arg_label(args[0])); break; @@ -1961,6 +1970,7 @@ static const TCGTargetOpDef arm_op_defs[] = { { INDEX_op_exit_tb, { } }, { INDEX_op_goto_tb, { } }, { INDEX_op_br, { } }, +{ INDEX_op_goto_ptr, { "r" } }, { INDEX_op_ld8u_i32, { "r", "r" } }, { INDEX_op_ld8s_i32, { "r", "r" } }, @@ -2136,9 +2146,16 @@ static void tcg_target_qemu_prologue(TCGContext *s) tcg_out_mov(s, TCG_TYPE_PTR, TCG_AREG0, tcg_target_call_iarg_regs[0]); tcg_out_bx(s, COND_AL, tcg_target_call_iarg_regs[1]); -tb_ret_addr = s->code_ptr; -/* Epilogue. We branch here via tb_ret_addr. */ +/* + * Return path for goto_ptr. Set return value to 0, a-la exit_tb, + * and fall through to the rest of the epilogue. + */ +s->code_gen_epilogue = s->code_ptr; +tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R0, 0); + +/* TB epilogue */ +tb_ret_addr = s->code_ptr; tcg_out_dat_rI(s, COND_AL, ARITH_ADD, TCG_REG_CALL_STACK, TCG_REG_CALL_STACK, stack_addend, 1); -- 2.9.4
[Qemu-devel] [PULL 26/26] target/alpha: Use goto_tb for fallthru between TBs
Signed-off-by: Richard Henderson --- target/alpha/translate.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/target/alpha/translate.c b/target/alpha/translate.c index 4523c4c..7c45ae3 100644 --- a/target/alpha/translate.c +++ b/target/alpha/translate.c @@ -89,6 +89,9 @@ typedef enum { updated the PC for the next instruction to be executed. */ EXIT_PC_STALE, +/* We are exiting the TB due to page crossing or space constraints. */ +EXIT_FALLTHRU, + /* We are ending the TB with a noreturn function call, e.g. longjmp. No following code will be executed. */ EXIT_NORETURN, @@ -2984,7 +2987,7 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb) || num_insns >= max_insns || singlestep || ctx.singlestep_enabled)) { -ret = EXIT_PC_STALE; +ret = EXIT_FALLTHRU; } } while (ret == NO_EXIT); @@ -2996,6 +2999,13 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb) case EXIT_GOTO_TB: case EXIT_NORETURN: break; +case EXIT_FALLTHRU: +if (use_goto_tb(&ctx, ctx.pc)) { +tcg_gen_goto_tb(0); +tcg_gen_movi_i64(cpu_pc, ctx.pc); +tcg_gen_exit_tb((uintptr_t)ctx.tb); +} +/* FALLTHRU */ case EXIT_PC_STALE: tcg_gen_movi_i64(cpu_pc, ctx.pc); /* FALLTHRU */ @@ -3007,7 +3017,7 @@ void gen_intermediate_code(CPUAlphaState *env, struct TranslationBlock *tb) } break; default: -abort(); +g_assert_not_reached(); } gen_tb_end(tb, num_insns); -- 2.9.4
[Qemu-devel] [PATCH] block/gluster.c: Handle qdict_array_entries() failure
In qemu_gluster_parse_json(), the call to qdict_array_entries() could return a negative error code, which we were ignoring because we assigned the result to an unsigned variable. Fix this by using the 'int' type instead, which matches the return type of qdict_array_entries() and also the type we use for the loop enumeration variable 'i'. (Spotted by Coverity, CID 1360960.) Signed-off-by: Peter Maydell --- block/gluster.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/block/gluster.c b/block/gluster.c index 031596a..addceed 100644 --- a/block/gluster.c +++ b/block/gluster.c @@ -493,8 +493,7 @@ static int qemu_gluster_parse_json(BlockdevOptionsGluster *gconf, Error *local_err = NULL; char *str = NULL; const char *ptr; -size_t num_servers; -int i, type; +int i, type, num_servers; /* create opts info from runtime_json_opts list */ opts = qemu_opts_create(&runtime_json_opts, NULL, 0, &error_abort); -- 2.7.4
[Qemu-devel] [PULL 15/26] tcg/s390: Implement goto_ptr
Tested-by: Aurelien Jarno Reviewed-by: Aurelien Jarno Signed-off-by: Richard Henderson --- tcg/s390/tcg-target.h | 2 +- tcg/s390/tcg-target.inc.c | 24 +--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h index 6b7bcfb..957f0c0 100644 --- a/tcg/s390/tcg-target.h +++ b/tcg/s390/tcg-target.h @@ -92,7 +92,7 @@ extern uint64_t s390_facilities; #define TCG_TARGET_HAS_mulsh_i32 0 #define TCG_TARGET_HAS_extrl_i64_i32 0 #define TCG_TARGET_HAS_extrh_i64_i32 0 -#define TCG_TARGET_HAS_goto_ptr 0 +#define TCG_TARGET_HAS_goto_ptr 1 #define TCG_TARGET_HAS_div2_i64 1 #define TCG_TARGET_HAS_rot_i641 diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c index a679280..5d7083e 100644 --- a/tcg/s390/tcg-target.inc.c +++ b/tcg/s390/tcg-target.inc.c @@ -1741,9 +1741,14 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, switch (opc) { case INDEX_op_exit_tb: -/* return value */ -tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R2, args[0]); -tgen_gotoi(s, S390_CC_ALWAYS, tb_ret_addr); +/* Reuse the zeroing that exists for goto_ptr. */ +a0 = args[0]; +if (a0 == 0) { +tgen_gotoi(s, S390_CC_ALWAYS, s->code_gen_epilogue); +} else { +tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R2, a0); +tgen_gotoi(s, S390_CC_ALWAYS, tb_ret_addr); +} break; case INDEX_op_goto_tb: @@ -1767,6 +1772,10 @@ static inline void tcg_out_op(TCGContext *s, TCGOpcode opc, s->tb_jmp_reset_offset[args[0]] = tcg_current_code_size(s); break; +case INDEX_op_goto_ptr: +tcg_out_insn(s, RR, BCR, S390_CC_ALWAYS, args[0]); +break; + OP_32_64(ld8u): /* ??? LLC (RXY format) is only present with the extended-immediate facility, whereas LLGC is always present. */ @@ -2241,6 +2250,7 @@ static const TCGTargetOpDef s390_op_defs[] = { { INDEX_op_exit_tb, { } }, { INDEX_op_goto_tb, { } }, { INDEX_op_br, { } }, +{ INDEX_op_goto_ptr, { "r" } }, { INDEX_op_ld8u_i32, { "r", "r" } }, { INDEX_op_ld8s_i32, { "r", "r" } }, @@ -2439,6 +2449,14 @@ static void tcg_target_qemu_prologue(TCGContext *s) /* br %r3 (go to TB) */ tcg_out_insn(s, RR, BCR, S390_CC_ALWAYS, tcg_target_call_iarg_regs[1]); +/* + * Return path for goto_ptr. Set return value to 0, a-la exit_tb, + * and fall through to the rest of the epilogue. + */ +s->code_gen_epilogue = s->code_ptr; +tcg_out_movi(s, TCG_TYPE_PTR, TCG_REG_R2, 0); + +/* TB epilogue */ tb_ret_addr = s->code_ptr; /* lmg %r6,%r15,fs+48(%r15) (restore registers) */ -- 2.9.4