Re: [Qemu-devel] [PATCH V8 14/17] filter: Add handle_event method for NetFilterClass
On 2018年06月11日 14:46, Zhang Chen wrote: On Mon, Jun 11, 2018 at 9:56 AM, Jason Wang wrote: On 2018年06月10日 22:09, Zhang Chen wrote: I think when COLO is enabled, qemu should reject all other types of backends. Yes, you are right. May be we should add a assert here? Because we can not get backend info when colo-compare start up. Have any idea? Is there a global variable or function to detect the COLO existence? If yes, maybe we could use that. May be we can reuse the "qmp_query_colo_status" in 11/17 patch. I will add this check in next version. I think it's better to have an internal helper. Calling qmp function from net is odd. Thanks Thanks Zhang Chen Thanks
[Qemu-devel] [PATCH v2] net: Fix a potential segfault
If user forgets to provide any backend types for '-netdev' in qemu CLI,
It triggers seg fault.
e.g.
Expected:
$ qemu -netdev id=net0
qemu-system-x86_64: Parameter 'type' is missing
Actual:
$ qemu -netdev id=net0
Segmentation fault (core dumped)
Signed-off-by: Lin Ma
---
net/net.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/net/net.c b/net/net.c
index efb9eaf779..f89790be4a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1093,9 +1093,12 @@ static int net_client_init(QemuOpts *opts, bool
is_netdev, Error **errp)
int ret = -1;
Visitor *v = opts_visitor_new(opts);
-if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
-show_netdevs();
-exit(0);
+if (is_netdev) {
+const char *type = qemu_opt_get(opts, "type");
+if (type && is_help_option(type)) {
+show_netdevs();
+exit(0);
+}
} else {
/* Parse convenience option format ip6-net=fec0::0[/64] */
const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
--
2.16.2
Re: [Qemu-devel] [PATCH v6 47/49] tests: add top-level make dependency for docker builds
Philippe Mathieu-Daudé writes: > Hi Alex, > > On 06/08/2018 09:33 AM, Alex Bennée wrote: >> One problem with satisfying your docker dependencies in a sub-make it >> you might end up trying to satisfy the dependency multiple times. This >> is especially a problem with debian-sid based cross compilers and CI >> setups. We solve this by doing a docker build pass at the top level >> before any sub-makes are called. >> >> We still need to satisfy dependencies in the Makefile.target call so >> people can run tests from individual target directories. We introduce >> a new Makefile.probe which gets called for each PROBE_TARGET and >> allows us to build up the list. It does require multiply including >> config-target.mak which shouldn't cause any issues as it shouldn't >> define anything that clashes with config-host.mak. However we undefine >> a few key variables each time around. >> >> Signed-off-by: Alex Bennée >> --- >> tests/Makefile.include | 14 +- >> tests/tcg/Makefile.probe | 31 +++ >> 2 files changed, 44 insertions(+), 1 deletion(-) >> create mode 100644 tests/tcg/Makefile.probe >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index ca00247e36..049a387436 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -931,7 +931,19 @@ BUILD_TCG_TARGET_RULES=$(patsubst %,build-tcg-tests-%, >> $(LINUX_USER_TARGETS)) >> CLEAN_TCG_TARGET_RULES=$(patsubst %,clean-tcg-tests-%, >> $(LINUX_USER_TARGETS)) >> RUN_TCG_TARGET_RULES=$(patsubst %,run-tcg-tests-%, $(LINUX_USER_TARGETS)) >> >> -build-tcg-tests-%: >> +# Probe for the Docker Builds we need for later >> +DOCKER_BUILD_TARGETS:= >> +$(foreach PROBE_TARGET,$(TARGET_LIST), \ >> +$(eval -include $(SRC_PATH)/tests/tcg/Makefile.probe) \ >> +$(if $(DOCKER_PREREQ), \ >> +$(eval DOCKER_BUILD_TARGETS+=$(DOCKER_PREREQ >> + >> +$(info DOCKER_BUILD_TARGETS=$(sort $(DOCKER_BUILD_TARGETS))) > > Is this verbose info on purpose? Ahh that slipped through, removed now. > > Else this could be surrounded by something equiv to: > > ifneq ($(V)$(DEBUG),) > ... > endif > >> + >> +.PHONY: build-docker-prereqs >> +build-docker-prereqs: $(sort $(DOCKER_BUILD_TARGETS)) >> + >> +build-tcg-tests-%: build-docker-prereqs >> $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $* V="$(V)" >> TARGET_DIR="$*/" guest-tests,) >> >> run-tcg-tests-%: build-tcg-tests-% >> diff --git a/tests/tcg/Makefile.probe b/tests/tcg/Makefile.probe >> new file mode 100644 >> index 00..7529e203ad >> --- /dev/null >> +++ b/tests/tcg/Makefile.probe >> @@ -0,0 +1,31 @@ >> +# -*- Mode: makefile -*- >> +# >> +# TCG Compiler Probe >> +# >> +# This Makefile fragement is included multiple times in the main make >> +# script to probe for available compilers. This is used to build up a >> +# selection of required docker targets before we invoke a sub-make for >> +# each target. >> + >> +# First we need the target makefile which tells us the target architecture >> +-include $(BUILD_DIR)/$(PROBE_TARGET)/config-target.mak >> + >> +# Then we load up the target architecture makefiles which tell us >> +# about the compilers >> +undefine CROSS_CC_GUEST >> +undefine DOCKER_IMAGE >> +DOCKER_PREREQ:= >> + >> +-include $(SRC_PATH)/tests/tcg/$(TARGET_BASE_ARCH)/Makefile.include >> +-include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.include >> + >> +ifndef CROSS_CC_GUEST >> +ifneq ($(DOCKER_IMAGE),) >> +DOCKER_PREREQ:=docker-image-$(DOCKER_IMAGE) >> +endif >> +endif >> + >> +# Clean-up >> +undefine TARGET_NAME >> +undefine TARGET_BASE_ARCH >> +undefine TARGET_ABI_DIR >> -- Alex Bennée
Re: [Qemu-devel] [PATCH v6 47/49] tests: add top-level make dependency for docker builds
Philippe Mathieu-Daudé writes: > On 06/08/2018 09:33 AM, Alex Bennée wrote: >> One problem with satisfying your docker dependencies in a sub-make it >> you might end up trying to satisfy the dependency multiple times. This >> is especially a problem with debian-sid based cross compilers and CI >> setups. We solve this by doing a docker build pass at the top level >> before any sub-makes are called. >> >> We still need to satisfy dependencies in the Makefile.target call so >> people can run tests from individual target directories. We introduce >> a new Makefile.probe which gets called for each PROBE_TARGET and >> allows us to build up the list. It does require multiply including >> config-target.mak which shouldn't cause any issues as it shouldn't >> define anything that clashes with config-host.mak. However we undefine >> a few key variables each time around. >> >> Signed-off-by: Alex Bennée >> --- >> tests/Makefile.include | 14 +- >> tests/tcg/Makefile.probe | 31 +++ >> 2 files changed, 44 insertions(+), 1 deletion(-) >> create mode 100644 tests/tcg/Makefile.probe >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index ca00247e36..049a387436 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -931,7 +931,19 @@ BUILD_TCG_TARGET_RULES=$(patsubst %,build-tcg-tests-%, >> $(LINUX_USER_TARGETS)) >> CLEAN_TCG_TARGET_RULES=$(patsubst %,clean-tcg-tests-%, >> $(LINUX_USER_TARGETS)) >> RUN_TCG_TARGET_RULES=$(patsubst %,run-tcg-tests-%, $(LINUX_USER_TARGETS)) >> >> -build-tcg-tests-%: >> +# Probe for the Docker Builds we need for later >> +DOCKER_BUILD_TARGETS:= >> +$(foreach PROBE_TARGET,$(TARGET_LIST), \ >> +$(eval -include $(SRC_PATH)/tests/tcg/Makefile.probe) \ >> +$(if $(DOCKER_PREREQ), \ >> +$(eval DOCKER_BUILD_TARGETS+=$(DOCKER_PREREQ >> + >> +$(info DOCKER_BUILD_TARGETS=$(sort $(DOCKER_BUILD_TARGETS))) >> + >> +.PHONY: build-docker-prereqs >> +build-docker-prereqs: $(sort $(DOCKER_BUILD_TARGETS)) > > It seems now all docker images are built, even if you want to test a > single arch... Using ~7GB of storage at once. I've moved build-docker-prereqs to build-tcg and made check-tcg depend on that. The single arch test case now relies on the sub-make building things. > >> + >> +build-tcg-tests-%: build-docker-prereqs >> $(call quiet-command,$(MAKE) $(SUBDIR_MAKEFLAGS) -C $* V="$(V)" >> TARGET_DIR="$*/" guest-tests,) >> >> run-tcg-tests-%: build-tcg-tests-% >> diff --git a/tests/tcg/Makefile.probe b/tests/tcg/Makefile.probe >> new file mode 100644 >> index 00..7529e203ad >> --- /dev/null >> +++ b/tests/tcg/Makefile.probe >> @@ -0,0 +1,31 @@ >> +# -*- Mode: makefile -*- >> +# >> +# TCG Compiler Probe >> +# >> +# This Makefile fragement is included multiple times in the main make >> +# script to probe for available compilers. This is used to build up a >> +# selection of required docker targets before we invoke a sub-make for >> +# each target. >> + >> +# First we need the target makefile which tells us the target architecture >> +-include $(BUILD_DIR)/$(PROBE_TARGET)/config-target.mak >> + >> +# Then we load up the target architecture makefiles which tell us >> +# about the compilers >> +undefine CROSS_CC_GUEST >> +undefine DOCKER_IMAGE >> +DOCKER_PREREQ:= >> + >> +-include $(SRC_PATH)/tests/tcg/$(TARGET_BASE_ARCH)/Makefile.include >> +-include $(SRC_PATH)/tests/tcg/$(TARGET_NAME)/Makefile.include >> + >> +ifndef CROSS_CC_GUEST >> +ifneq ($(DOCKER_IMAGE),) >> +DOCKER_PREREQ:=docker-image-$(DOCKER_IMAGE) >> +endif >> +endif >> + >> +# Clean-up >> +undefine TARGET_NAME >> +undefine TARGET_BASE_ARCH >> +undefine TARGET_ABI_DIR >> -- Alex Bennée
[Qemu-devel] [PATCH] vhost-user: delete net client if necessary
As qemu_new_net_client create new ncs but error happens later, ncs will be left in global net_clients list, so we need to cleanup them. Signed-off-by: linzhecheng diff --git a/net/vhost-user.c b/net/vhost-user.c index 608b837175..1c7ee48b60 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -345,6 +345,7 @@ err: s->vhost_user = NULL; } } +qemu_del_net_client(nc0); return -1; } -- 2.12.2.windows.2
Re: [Qemu-devel] [PATCH 01/12] migration: do not wait if no free thread
On Mon, Jun 04, 2018 at 05:55:09PM +0800, guangrong.x...@gmail.com wrote:
> From: Xiao Guangrong
>
> Instead of putting the main thread to sleep state to wait for
> free compression thread, we can directly post it out as normal
> page that reduces the latency and uses CPUs more efficiently
The feature looks good, though I'm not sure whether we should make a
capability flag for this feature since otherwise it'll be hard to
switch back to the old full-compression way no matter for what
reason. Would that be a problem?
>
> Signed-off-by: Xiao Guangrong
> ---
> migration/ram.c | 34 +++---
> 1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 5bcbf7a9f9..0caf32ab0a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1423,25 +1423,18 @@ static int compress_page_with_multi_thread(RAMState
> *rs, RAMBlock *block,
>
> thread_count = migrate_compress_threads();
> qemu_mutex_lock(&comp_done_lock);
Can we drop this lock in this case?
> -while (true) {
> -for (idx = 0; idx < thread_count; idx++) {
> -if (comp_param[idx].done) {
> -comp_param[idx].done = false;
> -bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> -qemu_mutex_lock(&comp_param[idx].mutex);
> -set_compress_params(&comp_param[idx], block, offset);
> -qemu_cond_signal(&comp_param[idx].cond);
> -qemu_mutex_unlock(&comp_param[idx].mutex);
> -pages = 1;
> -ram_counters.normal++;
> -ram_counters.transferred += bytes_xmit;
> -break;
> -}
> -}
> -if (pages > 0) {
> +for (idx = 0; idx < thread_count; idx++) {
> +if (comp_param[idx].done) {
> +comp_param[idx].done = false;
> +bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
> +qemu_mutex_lock(&comp_param[idx].mutex);
> +set_compress_params(&comp_param[idx], block, offset);
> +qemu_cond_signal(&comp_param[idx].cond);
> +qemu_mutex_unlock(&comp_param[idx].mutex);
> +pages = 1;
> +ram_counters.normal++;
> +ram_counters.transferred += bytes_xmit;
> break;
> -} else {
> -qemu_cond_wait(&comp_done_cond, &comp_done_lock);
> }
> }
> qemu_mutex_unlock(&comp_done_lock);
Regards,
--
Peter Xu
Re: [Qemu-devel] [PATCH v2] net: Fix a potential segfault
On 11.06.2018 09:06, Lin Ma wrote:
> If user forgets to provide any backend types for '-netdev' in qemu CLI,
> It triggers seg fault.
>
> e.g.
>
> Expected:
> $ qemu -netdev id=net0
> qemu-system-x86_64: Parameter 'type' is missing
>
> Actual:
> $ qemu -netdev id=net0
> Segmentation fault (core dumped)
Ok, thanks for adding the description!
> Signed-off-by: Lin Ma
> ---
> net/net.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index efb9eaf779..f89790be4a 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1093,9 +1093,12 @@ static int net_client_init(QemuOpts *opts, bool
> is_netdev, Error **errp)
> int ret = -1;
> Visitor *v = opts_visitor_new(opts);
>
> -if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
> -show_netdevs();
> -exit(0);
> +if (is_netdev) {
> +const char *type = qemu_opt_get(opts, "type");
> +if (type && is_help_option(type)) {
> +show_netdevs();
> +exit(0);
> +}
> } else {
> /* Parse convenience option format ip6-net=fec0::0[/64] */
> const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
>
I think you've got to do it in a slightly different way:
const char *type = qemu_opt_get(opts, "type");
if (is_netdev && type && is_help_option(type)) {
show_netdevs();
exit(0);
} else ...
otherwise the "else" branch is not entered anymore in case it is a
non-help netdev option.
Thomas
Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
On 06/10/2018 03:12 PM, Thomas Huth wrote:
> Add a sanity check to fix the following crash:
>
> $ echo "Insane in the mainframe" > /tmp/test.txt
> $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt
> Segmentation fault (core dumped)
>
> Signed-off-by: Thomas Huth
Reviewed-by: Christian Borntraeger
I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE. No?
> ---
> hw/s390x/ipl.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 04245b5..9bb9b50 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error
> **errp)
> * we can not rely on the ELF entry point - it was 0x800 (the SALIPL
> * loader) and it won't work. For this case we force it to 0x1,
> too.
> */
> -if (pentry == KERN_IMAGE_START || pentry == 0x800) {
> +if ((pentry == KERN_IMAGE_START || pentry == 0x800) &&
> +kernel_size > KERN_PARM_AREA + strlen(ipl->cmdline)) {
> ipl->start_addr = KERN_IMAGE_START;
> /* Overwrite parameters in the kernel image, which are "rom" */
> strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>
Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
Keno Fischer writes:
> strchrnul is a GNU extension and thus unavailable on a number of targets.
> In the review for a commit removing strchrnul from 9p, I was asked to
> create a qemu_strchrnul helper to factor out this functionality.
> Do so, and use it in a number of other places in the code base that inlined
> the replacement pattern in a place where strchrnul could be used.
>
> Signed-off-by: Keno Fischer
> Acked-by: Greg Kurz
> ---
>
> Changes since v3:
> - Fix bug in configure check
>
> configure | 15 +++
> hmp.c | 8
> hw/9pfs/9p-local.c| 2 +-
> include/qemu/cutils.h | 8
> monitor.c | 8 ++--
> util/cutils.c | 15 +++
> util/qemu-option.c| 6 +-
> util/uri.c| 6 ++
> 8 files changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/configure b/configure
> index 14b1113..f5ca850 100755
> --- a/configure
> +++ b/configure
> @@ -4747,6 +4747,18 @@ if compile_prog "" "" ; then
> fi
>
> ##
> +# check if we have strchrnul
> +
> +strchrnul=no
> +cat > $TMPC << EOF
> +#include
> +int main(void) { (void)strchrnul("Hello World", 'W'); return 0; }
Suggest return strchrnul("Hello World", 'W') != 6, to avoid worries
about a sufficiently smart compilers optimizing out a call that would
otherwise fail to link, say because headers don't match libraries.
> +EOF
> +if compile_prog "" "" ; then
> +strchrnul=yes
> +fi
> +
> +##
> # check if trace backend exists
>
> $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends"
> --check-backends > /dev/null 2> /dev/null
You're not printing $strchrnul like we print other configuration
results. Hmm, we're not printing several of them. Question for
maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three
coughed up by get_maintainer.pl): bug or feature? If feature, how do we
decide what to print?
> @@ -6229,6 +6241,9 @@ fi
> if test "$sem_timedwait" = "yes" ; then
>echo "CONFIG_SEM_TIMEDWAIT=y" >> $config_host_mak
> fi
> +if test "$strchrnul" = "yes" ; then
> + echo "CONFIG_STRCHRNUL=y" >> $config_host_mak
> +fi
> if test "$byteswap_h" = "yes" ; then
>echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak
> fi
> diff --git a/hmp.c b/hmp.c
> index ef93f48..416d4c9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2123,12 +2123,12 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
> int has_hold_time = qdict_haskey(qdict, "hold-time");
> int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
> Error *err = NULL;
> -char *separator;
> +const char *separator;
> int keyname_len;
>
> while (1) {
> -separator = strchr(keys, '-');
> -keyname_len = separator ? separator - keys : strlen(keys);
> +separator = qemu_strchrnul(keys, '-');
> +keyname_len = separator - keys;
>
> /* Be compatible with old interface, convert user inputted "<" */
> if (keys[0] == '<' && keyname_len == 1) {
> @@ -2165,7 +2165,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
> keylist->value->u.qcode.data = idx;
> }
>
> -if (!separator) {
> +if (!*separator) {
> break;
> }
> keys = separator + 1;
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 5721eff..828e8d6 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -65,7 +65,7 @@ int local_open_nofollow(FsContext *fs_ctx, const char
> *path, int flags,
> assert(*path != '/');
>
> head = g_strdup(path);
> -c = strchrnul(path, '/');
> +c = qemu_strchrnul(path, '/');
> if (*c) {
> /* Intermediate path element */
> head[c - path] = 0;
> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
> index a663340..809090c 100644
> --- a/include/qemu/cutils.h
> +++ b/include/qemu/cutils.h
> @@ -122,6 +122,14 @@ int qemu_strnlen(const char *s, int max_len);
> * Returns: the pointer originally in @input.
> */
> char *qemu_strsep(char **input, const char *delim);
> +#ifdef CONFIG_STRCHRNUL
> +static inline const char *qemu_strchrnul(const char *s, int c)
> +{
> +return strchrnul(s, c);
> +}
> +#else
> +const char *qemu_strchrnul(const char *s, int c);
> +#endif
Could we simply provide strchrnul() when it's missing? Something like
+#ifndef CONFIG_STRCHRNUL
+const char *strchrnul(const char *s, int c);
+#endif
in qemu/osdep.h. Hmm, it looks like tacking qemu_ onto such things is
common practice. Nevermind then.
> time_t mktimegm(struct tm *tm);
> int qemu_fdatasync(int fd);
> int fcntl_setfl(int fd, int flag);
> diff --git a/monitor.c b/monitor.c
> index 6d0cec5..4484d74 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -797,9 +797,7 @@ static int compare_cmd(const char *name, const char *list)
> p = list;
> for(;;) {
>
Re: [Qemu-devel] [PATCH v2 0/2] Avoid using RUN_STATE_PRECONFIG unless explicitly requested with --preconfig
On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote: > The change to use RUN_STATE_PRECONFIG as the default initial state, even > when not requested with --preconfig has caused a number of problems. This > series introduces a new RUN_STATE_NONE to act as the initial state, so > that we never use RUN_STATE_PRECONFIG unless the mgmt app has explicitly > requested todo so. > > Daniel P. Berrangé (2): > vl: don't use RUN_STATE_PRECONFIG as initial state > vl: fix use of --daemonize with --preconfig > > qapi/run-state.json | 6 +- > vl.c| 49 +++-- > 2 files changed, 35 insertions(+), 20 deletions(-) > So do we have some agreement here? I'm running qemu from git and I'm still using my patch to make libvirt work. Michal
Re: [Qemu-devel] [PATCH] monitor: postpone monitor_qmp_cleanup_queues
Peter Xu writes: > On Fri, Jun 08, 2018 at 05:11:54PM +0800, Peter Xu wrote: [...] > So I think I agree with Markus's solution, we just flush the response > queue when we get CLOSED (but we don't close the output fd; IMHO > that's chardev backend's job). Would that work? I figure you're right on close() being the chardev backend's job.
Re: [Qemu-devel] [PATCH 00/12] migration: improve multithreads for compression and decompression
On Mon, Jun 04, 2018 at 05:55:08PM +0800, guangrong.x...@gmail.com wrote: > From: Xiao Guangrong > > Background > -- > Current implementation of compression and decompression are very > hard to be enabled on productions. We noticed that too many wait-wakes > go to kernel space and CPU usages are very low even if the system > is really free > > The reasons are: > 1) there are two many locks used to do synchronous，there > is a global lock and each single thread has its own lock, > migration thread and work threads need to go to sleep if > these locks are busy > > 2) migration thread separately submits request to the thread >however, only one request can be pended, that means, the >thread has to go to sleep after finishing the request > > Our Ideas > - > To make it work better, we introduce a new multithread model, > the user, currently it is the migration thread, submits request > to each thread with round-robin manner, the thread has its own > ring whose capacity is 4 and puts the result to a global ring > which is lockless for multiple producers, the user fetches result > out from the global ring and do remaining operations for the > request, e.g, posting the compressed data out for migration on > the source QEMU > > Other works in this patchset is offering some statistics to see > if compression works as we expected and making the migration thread > work fast so it can feed more requests to the threads Hi, Guangrong, I'm not sure whether my understanding is correct, but AFAIU the old code has a major defect that it depends too much on the big lock. The critial section of the small lock seems to be very short always, and also that's per-thread. However we use the big lock in lots of places: flush compress data, queue every page, or send the notifies in the compression thread. I haven't yet read the whole work, this work seems to be quite nice according to your test results. However have you thought about firstly remove the big lock without touching much of other part of the code, then continue to improve it? Or have you ever tried to do so? I don't think you need to do extra work for this, but I would appreciate if you have existing test results to share. In other words, would it be nicer to separate the work into two pieces? - one to refactor the existing locks, to see what we can gain by simplify the locks to minimum. AFAIU now the locking used is still not ideal, my thinking is that _maybe_ we can start by removing the big lock, and use a semaphore or something to replace the "done" notification while still keep the small lock? Even some busy looping? - one to introduce the lockless ring buffer, to demostrate how the lockless data structure helps comparing to the locking ways Then we can know which item contributed how much to the performance numbers. After all the new ring and thread model seems to be a big chunk of work (sorry I haven't read them yet, but I will). What do you think? Thanks, -- Peter Xu
Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 32/40] hw/ppc: Use the IEC binary prefix definitions
On Sun, 10 Jun 2018, Philippe Mathieu-Daudé wrote:
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 2a98c10664..b35e3fff1c 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -12,8 +12,9 @@
*/
#include "qemu/osdep.h"
+#include "qemu/units.h"
#include "qemu-common.h"
-#include "qemu/cutils.h"
+#include "qemu/units.h"
I think one of these includes of qemu/units.h is enough.
#include "qemu/error-report.h"
#include "qapi/error.h"
#include "hw/hw.h"
@@ -46,7 +47,7 @@
/* from Sam460 U-Boot include/configs/Sam460ex.h */
#define FLASH_BASE 0xfff0
#define FLASH_BASE_H 0x4
-#define FLASH_SIZE (1 << 20)
+#define FLASH_SIZE (1 * MiB)
#define UBOOT_LOAD_BASE0xfff8
#define UBOOT_SIZE 0x0008
#define UBOOT_ENTRY0xfffc
@@ -71,7 +72,8 @@
/* FIXME: See u-boot.git 8ac41e, also fix in ppc440_uc.c */
static const unsigned int ppc460ex_sdram_bank_sizes[] = {
-1024 << 20, 512 << 20, 256 << 20, 128 << 20, 64 << 20, 32 << 20, 0
+1 * GiB, 512 * MiB, 256 * MiB, 128 * MiB,
+64 * MiB, 32 * MiB, 0
};
You said elsewhere:
On Sun, 10 Jun 2018, Philippe Mathieu-Daudé wrote:
Each use this saves 3 char of the 80 columns limit!
Except when not but at least should not be longer than before and maybe
more readable. But why are you splitting this line into two? I prefer one
line for readability if it's not over the line limit.
struct boot_info {
@@ -225,7 +227,7 @@ static int sam460ex_load_uboot(void)
fl_sectors = (bios_size + 65535) >> 16;
if (!pflash_cfi01_register(base, NULL, "sam460ex.flash", bios_size,
- blk, (64 * 1024), fl_sectors,
+ blk, 64 * KiB, fl_sectors,
1, 0x89, 0x18, 0x, 0x0, 1)) {
error_report("qemu: Error registering flash memory.");
/* XXX: return an error instead? */
@@ -359,14 +361,14 @@ static void main_cpu_reset(void *opaque)
/* either we have a kernel to boot or we jump to U-Boot */
if (bi->entry != UBOOT_ENTRY) {
-env->gpr[1] = (16 << 20) - 8;
+env->gpr[1] = (16 * MiB) - 8;
env->gpr[3] = FDT_ADDR;
env->nip = bi->entry;
/* Create a mapping for the kernel. */
mmubooke_create_initial_mapping(env, 0, 0);
env->gpr[6] = tswap32(EPAPR_MAGIC);
-env->gpr[7] = (16 << 20) - 8; /*bi->ima_size;*/
+env->gpr[7] = (16 * MiB) - 8; /* bi->ima_size; */
} else {
env->nip = UBOOT_ENTRY;
@@ -479,8 +481,8 @@ static void sam460ex_init(MachineState *machine)
/* 256K of L2 cache as memory */
ppc4xx_l2sram_init(env);
/* FIXME: remove this after fixing l2sram mapping in ppc440_uc.c? */
-memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram", 256 << 10,
- &error_abort);
+memory_region_init_ram(l2cache_ram, NULL, "ppc440.l2cache_ram",
+ 256 * KiB, &error_abort);
Line is split differently here too for no apparent reason.
Regards,
BALATON Zoltan
Re: [Qemu-devel] storing machine data in qcow images?
On Mon, 11 Jun 2018 05:06:53 +0300 "Michael S. Tsirkin" wrote: > On Sat, Jun 09, 2018 at 11:34:03PM +0200, Max Reitz wrote: > > Dave was saying that the worst thing about the whole q35 thing is > > that users download an image and have no idea why it isn't > > working. Figuring that out may take a long time, because nothing > > is even throwing an error message. > > > > If we had a new format, users couldn't even run it in qemu, so they > > would quickly figure out that in order to run this VM, they need to > > update their stack. > > Since then users of old software can't use your > image at all, then most people simply will not create > the new fangled image format. They will have to because without the new fangles the images just don't work. > > > If we just add this information to qcow2, those users with outdated > > qemu versions would again have to figure out why the image isn't > > working. > > By that metric compatibility is never worth it unless you have > ability to add new functionality retroactively to existing > software. Compatibility is worth it when you add a new extension that is useful but not critical. When the image does not work without interpreting the extended metadata, the failure is hard to diagnose, and old versions of qemu just ignore the extended metadata the extension failed the primary purpose: making images work out of the box. Thanks Michal
Re: [Qemu-devel] [PATCH v6 40/49] tests/tcg: enable building for PowerPC
Philippe Mathieu-Daudé writes: > On 06/08/2018 09:32 AM, Alex Bennée wrote: >> Now we have restored debian-image-powerpc-cross using Debian SID >> compilers we can build for 32 bit powerpc. Although PPC32 supports a >> range of pages sizes currently only 4k works so the others are >> commented out for now. >> >> We can also merge the ppc64 support under the base architecture >> directory to avoid too much proliferation of directories. > > ppc64el doesn't seem to work: > > $ make subdir-ppc64le-linux-user build-tcg-tests-ppc64el-linux-user -j1 > make[1]: *** ppc64el-linux-user: No such file or directory. Stop. > make: *** [tests/Makefile.include:947: > build-tcg-tests-ppc64el-linux-user] Error 2 09:17:30 [alex@zen:~/l/q/qemu.git] testing/tcg-testing-revival-v7 + make run-tcg-tests-ppc64le-linux-user BUILD debian9 BUILD debian-ppc64el-cross CROSS-BUILD ppc64le guest-tests with docker qemu:debian-ppc64el-cross BUILD debian9 BUILD debian-ppc64el-cross CROSS-BUILD ppc64le guest-tests with docker qemu:debian-ppc64el-cross CC ppc64le-linux-user/linux-user/main.o LINKppc64le-linux-user/qemu-ppc64le RUN-TESTS for ppc64le TESTtest-mmap (default) on ppc64le TESTsha1 on ppc64le TESTlinux-test on ppc64le TESTtestthread on ppc64le TESTtest-mmap (4096 byte pages) on ppc64le The Debian and QEMU naming conventions switch > >> Signed-off-by: Alex Bennée > > ppc32: > > Reviewed-by: Philippe Mathieu-Daudé > Tested-by: Philippe Mathieu-Daudé > >> >> --- >> v5 >> - new for v5 >> --- >> tests/tcg/ppc/Makefile.include | 7 +++ >> tests/tcg/ppc/Makefile.target | 12 >> tests/tcg/ppc64le/Makefile.include | 2 -- >> 3 files changed, 19 insertions(+), 2 deletions(-) >> create mode 100644 tests/tcg/ppc/Makefile.include >> create mode 100644 tests/tcg/ppc/Makefile.target >> delete mode 100644 tests/tcg/ppc64le/Makefile.include >> >> diff --git a/tests/tcg/ppc/Makefile.include b/tests/tcg/ppc/Makefile.include >> new file mode 100644 >> index 00..b062c30dd3 >> --- /dev/null >> +++ b/tests/tcg/ppc/Makefile.include >> @@ -0,0 +1,7 @@ >> +ifeq ($(TARGET_NAME),ppc) >> +DOCKER_IMAGE=debian-powerpc-cross >> +DOCKER_CROSS_COMPILER=powerpc-linux-gnu-gcc >> +else ifeq ($(TARGET_NAME),ppc64le) >> +DOCKER_IMAGE=debian-ppc64el-cross >> +DOCKER_CROSS_COMPILER=powerpc64le-linux-gnu-gcc >> +endif >> diff --git a/tests/tcg/ppc/Makefile.target b/tests/tcg/ppc/Makefile.target >> new file mode 100644 >> index 00..f5e08c7376 >> --- /dev/null >> +++ b/tests/tcg/ppc/Makefile.target >> @@ -0,0 +1,12 @@ >> +# -*- Mode: makefile -*- >> +# >> +# PPC - included from tests/tcg/Makefile >> +# >> + >> +ifneq (,$(findstring 64,$(TARGET_NAME))) >> +# On PPC64 Linux can be configured with 4k (default) or 64k pages >> (currently broken) >> +EXTRA_RUNS+=run-test-mmap-4096 #run-test-mmap-65536 >> +else >> +# On PPC32 Linux supports 4K/16K/64K/256K (but currently only 4k works) >> +EXTRA_RUNS+=run-test-mmap-4096 #run-test-mmap-16384 run-test-mmap-65536 >> run-test-mmap-262144 >> +endif >> diff --git a/tests/tcg/ppc64le/Makefile.include >> b/tests/tcg/ppc64le/Makefile.include >> deleted file mode 100644 >> index d71cfc9aa7..00 >> --- a/tests/tcg/ppc64le/Makefile.include >> +++ /dev/null >> @@ -1,2 +0,0 @@ >> -DOCKER_IMAGE=debian-ppc64el-cross >> -DOCKER_CROSS_COMPILER=powerpc64le-linux-gnu-gcc >> -- Alex Bennée
Re: [Qemu-devel] [PATCH v6 00/49] fix building of tests/tcg
Philippe Mathieu-Daudé writes: > On 06/08/2018 09:32 AM, Alex Bennée wrote: >> Hi, >> >> Not a super amount has changed since the last version but review >> comments and review tags have been added. The new patches at the end >> enable a .travis.yml run and try and make the image building part of >> check-tcg -j safe. Essentially the problem is trying to avoid >> re-building the images multiple times. The additional issue is wanting >> an upto date Debian SID image whenever we actually build an image but >> not forcing a rebuild every time. >> >> Essentially I'd like to encode a conditional dependency when: >> - the target image doesn't exist >> - or the target image is out of date w.r.t. the dockerfike >> >> I'm thinking this is going to involve some sort of extension to the >> docker.py script to feed the Makefile. >> >> A number of the prerequisite patches have already been pulled in the >> docker fixes series however I think this series is ready to go in now. >> Unless there are any objections I'll send a pull on Monday. >> >> Current unreviewed patches: >> >> patch 0017/tests tcg i386 add runner for test i386 fprem.patch needs review >> patch 0039/docker move debian powerpc cross to sid based bui.patch needs >> review >> patch 0040/tests tcg enable building for PowerPC.patch needs review >> patch 0042/Makefile.target add clean build guest tests targe.patch needs >> review >> patch 0044/tests tcg add run diff and skip helper macros.patch needs review >> patch 0045/tests tcg override runners for broken tests.patch needs review >> patch 0046/target sh4 Fix translator.c assertion failure for.patch needs >> review > > Is this patch related/required to this series? > It looks it should enter via another tree. > > The SH4 tests pass without it: > > $ make run-tcg-tests-sh4-linux-user -j1 > ... > RUN-TESTS for sh4 > TESTtest-mmap (default) on sh4 > TESTsha1 on sh4 > TESTlinux-test on sh4 > TESTtestthread on sh4 > TESTtest-mmap (4096 byte pages) on sh4 Not with --enable-debug, but yeah I'll drop as it should come via Rich's TCG tree. > >> patch 0047/tests add top level make dependency for docker bu.patch needs >> review >> patch 0048/tests docker prevent sub makes re building debian.patch needs >> review >> patch 0049/.travis.yml add check tcg test.patch needs review >> >> >> Alex Bennée (46): >> configure: add support for --cross-cc-FOO >> configure: move i386_cc to cross_cc_i386 >> configure: allow user to specify --cross-cc-cflags-foo= >> configure: set cross_cc_FOO for host compiler >> docker: Add "cc" subcommand >> docker: extend "cc" command to accept compiler >> docker: allow "cc" command to run in user context >> docker: Makefile.include introduce DOCKER_SCRIPT >> tests/tcg: move architecture independent tests into subdir >> tests/tcg/multiarch: enable additional linux-test tests >> tests/tcg/multiarch: move most output to stdout >> tests/tcg: move i386 specific tests into subdir >> tests/tcg: enable building for i386 >> tests/tcg/i386: fix test-i386 >> tests/tcg/i386: add runner for test-i386-fprem >> tests/tcg/x86_64: add Makefile.target >> tests/tcg/i386/test-i386: use modern vector_size attributes >> tests/tcg/i386/test-i386: fix printf format >> tests/tcg: move ARM specific tests into subdir >> tests/tcg: enable building for ARM >> tests/tcg/arm: fix up test-arm-iwmmxt test >> tests/tcg: enable building for AArch64 >> tests/tcg/arm: add fcvt test cases for AArch32/64 >> tests/tcg: move MIPS specific tests into subdir >> tests/tcg: enable building for MIPS >> tests/tcg/mips: include common mips hello-mips >> tests/tcg: enable building for s390x >> tests/tcg: enable building for ppc64 >> tests/tcg: enable building for Alpha >> tests/tcg/alpha: add Alpha specific tests >> tests/tcg: enable building for HPPA >> tests/tcg: enable building for m68k >> tests/tcg: enable building for sh4 >> tests/tcg: enable building for sparc64 >> tests/tcg: enable building for mips64 >> tests/tcg: enable building for RISCV64 >> docker: move debian-powerpc-cross to sid based build >> tests/tcg: enable building for PowerPC >> tests/tcg/Makefile: update to be called from Makefile.target >> Makefile.target: add (clean-/build-)guest-tests targets >> tests/Makefile.include: add [build|clean|check]-tcg targets >> tests/tcg: add run, diff, and skip helper macros >> tests/tcg: override runners for broken tests >> tests: add top-level make dependency for docker builds >> tests/docker: prevent sub-makes re-building debian-sid >> .travis.yml: add check-tcg test >> >> Fam Zheng (2): >> tests/tcg/multiarch: Build fix for linux-test >> tests/tcg/i386: Build fix for hello-i386 >> >> Richard Henderson (1): >> target/sh4: Fix translator.c assertion failure for gUSA >> >> .travis.yml |6 + >> MAINTAINERS
Re: [Qemu-devel] [PATCH v6 49/49] .travis.yml: add check-tcg test
Philippe Mathieu-Daudé writes:
> Hi Alex,
>
> On 06/08/2018 09:33 AM, Alex Bennée wrote:
>> Signed-off-by: Alex Bennée
>> ---
>> .travis.yml | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 814be151f4..f1d2d9edec 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -152,3 +152,9 @@ matrix:
>> - TEST_CMD=""
>>before_script:
>> - ./configure ${CONFIG} --extra-cflags="-g3 -O0 -fsanitize=thread
>> -fuse-ld=gold" || cat config.log
>> +- env:
>> +- CONFIG="--disable-system --disable-docs"
>> +- TEST_CMD="make check-tcg"
>
> Since we have default MAKEFLAGS="-j3"
>
> I'm getting many failures:
>
> CROSS-BUILD ppc64le guest-tests with docker qemu:debian-ppc64el-cross
> BUILD debian-riscv64-cross
> BUILD debian-s390x-cross
> CROSS-BUILD riscv64 guest-tests with docker qemu:debian-riscv64-cross
> CROSS-BUILD s390x guest-tests with docker qemu:debian-s390x-cross
> Error in atexit._run_exitfuncs:
> Traceback (most recent call last):
> File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
> func(*targs, **kargs)
> File "/home/travis/qemu/tests/docker/docker.py", line 174, in
> _kill_instances
> return self._do_kill_instances(True)
> File "/home/travis/qemu/tests/docker/docker.py", line 154, in
> _do_kill_instances
> resp = self._output(["inspect", i])
> File "/home/travis/qemu/tests/docker/docker.py", line 179, in _output
> **kwargs)
> File "/usr/lib/python2.7/subprocess.py", line 223, in check_output
> raise CalledProcessError(retcode, cmd, output=output)
> CalledProcessError: Command '['docker', 'inspect', 'c71dbe973c55']'
> returned non-zero exit status 1
> Error in sys.exitfunc:
> Traceback (most recent call last):
> File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
> func(*targs, **kargs)
> File "/home/travis/qemu/tests/docker/docker.py", line 174, in
> _kill_instances
> return self._do_kill_instances(True)
> File "/home/travis/qemu/tests/docker/docker.py", line 154, in
> _do_kill_instances
> resp = self._output(["inspect", i])
> File "/home/travis/qemu/tests/docker/docker.py", line 179, in _output
> **kwargs)
> File "/usr/lib/python2.7/subprocess.py", line 223, in check_output
> raise CalledProcessError(retcode, cmd, output=output)
> subprocess.CalledProcessError: Command '['docker', 'inspect',
> 'c71dbe973c55']' returned non-zero exit status 1
> BUILD debian-sid
> GEN sh4eb-linux-user/config-target.h
> CROSS-BUILD sh4eb guest-tests SKIPPED
> CROSS-BUILD sparc guest-tests SKIPPED
> GEN sparc32plus-linux-user/config-target.h
> CROSS-BUILD sparc32plus guest-tests SKIPPED
> BUILD debian-sid
>
> This works fine adding:
>
> - MAKEFLAGS="-j1"
Hmm I would really like to get this working properly. But I thought I
did by building everything at the top level. I messed around with
conditional dependencies currently in:
https://github.com/stsquad/qemu/tree/testing/tcg-testing-revival-v7
To try and make this better.
>
> Adding MAKEFLAGS="-j1":
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
>
>> + sudo: required
>> + dist: trusty
>> + compiler: gcc
>>
--
Alex Bennée
Re: [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
On 2018/5/31 19:01, Auger Eric wrote: > Hi, > > On 05/31/2018 05:15 AM, Shannon Zhao wrote: >> While for_each_dist_irq_reg loop starts from GIC_INTERNAL, it forgot to >> offset the date array and index. This will overlap the GICR registers >> value and leave the last GIC_INTERNAL irq's registers out of update. >> >> Fixes: 367b9f527becdd20ddf116e17a3c0c2bbc486920 >> Cc: qemu-sta...@nongnu.org >> Reviewed-by: Peter Maydell > Reviewed-by: Eric Auger > Hi Peter, Looks like we missed picking up this patch. I didn't include this in v6 since you and Eric both reviewed it. Could you please pick it up? Thanks, -- Shannon
Re: [Qemu-devel] storing machine data in qcow images?
On Wed, Jun 06, 2018 at 01:32:47PM +0200, Max Reitz wrote: > ext2? I wrote an nbdkit plugin for ext2/ext3/ext4 last week. https://github.com/libguestfs/nbdkit/tree/master/plugins/ext2 It uses libext2fs from e2fsprogs and I think there are some lessons for anyone who wants to use ext2 to store disk images. (1) You cannot have more than one host process accessing a single filesystem image, even read-only. This is because opening an ext2+ filesystem even read-only causes writes, replaying the journal (for ext3+) or writing to the superblock. I'm sure there are some common use-cases such as overlays sharing a common backing store which are excluded by this restriction. (2) Within a single process you cannot have more than one libext2fs handle open on the filesystem image. This could make qemu block drivers a bit awkward (although not impossible) because if two instances of an ext2 qemu block driver both opened different disks in the same filesystem they'd need to share a handle. (3) You can resize files in the filesystem, although because we're waiting for the NBD resize extension to be finalized my plugin does not do that. (4) Trim/discard appears to be possible (and it should be possible to punch holes in the filesystem image) but I couldn't actually understand how to make it work. Also fast zeroing. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
Re: [Qemu-devel] [RFC v1 2/2] crypto/virtio-crypto: Register an algo only if it's supported
> -Original Message-
> From: Farhan Ali [mailto:al...@linux.ibm.com]
> Sent: Saturday, June 09, 2018 3:09 AM
> To: linux-ker...@vger.kernel.org; k...@vger.kernel.org
> Cc: m...@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> ; longpeng ;
> pa...@linux.ibm.com; fran...@linux.ibm.com; borntrae...@de.ibm.com;
> al...@linux.ibm.com
> Subject: [RFC v1 2/2] crypto/virtio-crypto: Register an algo only if it's
> supported
>
> From: Farhan Ali
>
> Register a crypto algo with the Linux crypto layer only if
> the algorithm is supported by the backend virtio-crypto
> device.
>
> Also route crypto requests to a virtio-crypto
> device, only if it can support the requested service and
> algorithm.
>
> Signed-off-by: Farhan Ali
> ---
> drivers/crypto/virtio/virtio_crypto_algs.c | 110
> ++-
> drivers/crypto/virtio/virtio_crypto_common.h | 11 ++-
> drivers/crypto/virtio/virtio_crypto_mgr.c| 81 ++--
> 3 files changed, 158 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c
> b/drivers/crypto/virtio/virtio_crypto_algs.c
> index ba190cf..fef112a 100644
> --- a/drivers/crypto/virtio/virtio_crypto_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_algs.c
> @@ -49,12 +49,18 @@ struct virtio_crypto_sym_request {
> bool encrypt;
> };
>
> +struct virtio_crypto_algo {
> + uint32_t algonum;
> + uint32_t service;
> + unsigned int active_devs;
> + struct crypto_alg algo;
> +};
> +
> /*
> * The algs_lock protects the below global virtio_crypto_active_devs
> * and crypto algorithms registion.
> */
> static DEFINE_MUTEX(algs_lock);
> -static unsigned int virtio_crypto_active_devs;
> static void virtio_crypto_ablkcipher_finalize_req(
> struct virtio_crypto_sym_request *vc_sym_req,
> struct ablkcipher_request *req,
> @@ -312,13 +318,19 @@ static int virtio_crypto_ablkcipher_setkey(struct
> crypto_ablkcipher *tfm,
>unsigned int keylen)
> {
> struct virtio_crypto_ablkcipher_ctx *ctx = crypto_ablkcipher_ctx(tfm);
> + uint32_t alg;
> int ret;
>
> + ret = virtio_crypto_alg_validate_key(keylen, &alg);
> + if (ret)
> + return ret;
> +
> if (!ctx->vcrypto) {
> /* New key */
> int node = virtio_crypto_get_current_node();
> struct virtio_crypto *vcrypto =
> - virtcrypto_get_dev_node(node);
> + virtcrypto_get_dev_node(node,
> + VIRTIO_CRYPTO_SERVICE_CIPHER, alg);
> if (!vcrypto) {
> pr_err("virtio_crypto: Could not find a virtio device
> in the
> system\n");
We'd better change the above error message now. What about:
" virtio_crypto: Could not find a virtio device in the system or unsupported
algo" ?
Regards,
-Gonglei
Re: [Qemu-devel] [PATCH v3 1/7] hmp: Add flag for preconfig commands
"Dr. David Alan Gilbert (git)" writes:
> From: "Dr. David Alan Gilbert"
>
> Add a flag to command definitions to allow them to be used in preconfig
> and check it.
> If users try to use commands that aren't available, tell them to use
> the exit_preconfig comand we're adding in a few patches.
>
> Signed-off-by: Dr. David Alan Gilbert
> ---
> monitor.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 6d0cec552e..f4a16e6a03 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -128,6 +128,7 @@ typedef struct mon_cmd_t {
> const char *args_type;
> const char *params;
> const char *help;
> +const char *flags; /* p=preconfig */
I'd add a bool for each flag instead of encoding them in a string. No
need for a comment then (yours is cryptic). But your artistic license
applies.
> void (*cmd)(Monitor *mon, const QDict *qdict);
> /* @sub_table is a list of 2nd level of commands. If it does not exist,
> * cmd should be used. If it exists, sub_table[?].cmd should be
> @@ -936,6 +937,19 @@ static int parse_cmdline(const char *cmdline,
> return -1;
> }
>
> +/*
> + * Returns true if the command can be executed in preconfig mode
> + * i.e. it has the 'p' flag.
> + */
> +static bool cmd_can_preconfig(const mon_cmd_t *cmd)
> +{
> +if (!cmd->flags) {
> +return false;
> +}
> +
> +return strchr(cmd->flags, 'p');
> +}
> +
> static void help_cmd_dump_one(Monitor *mon,
>const mon_cmd_t *cmd,
>char **prefix_args,
> @@ -2976,6 +2990,12 @@ static const mon_cmd_t *monitor_parse_command(Monitor
> *mon,
> (int)(p - cmdp_start), cmdp_start);
> return NULL;
> }
> +if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> +monitor_printf(mon, "Command '%.*s' not available with -preconfig; "
> +"use exit_preconfig after configuration.\n",
> + (int)(p - cmdp_start), cmdp_start);
> +return NULL;
> +}
>
> /* filter out following useless space */
> while (qemu_isspace(*p)) {
Suggest "not available with -preconfig until after exit_preconfig".
Regardless,
Reviewed-by: Markus Armbruster
Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
On 11 June 2018 at 08:56, Markus Armbruster wrote: > You're not printing $strchrnul like we print other configuration > results. Hmm, we're not printing several of them. Question for > maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three > coughed up by get_maintainer.pl): bug or feature? If feature, how do we > decide what to print? If we printed everything that we tested for then the output would be unhelpfully enormous. My view is that we should print the "interesting" things for the user, ie the higher-level things that the user could potentially turn on by installing more libraries or has turned off explicitly or whatever. Reporting whether the host OS has strchrnul or whether we've had to provide our own implementation is doubly uninteresting: * there's nothing the user could do to change this * there is no visible effect (missing features, worse performance) There's an argument that we should also log every config check result somehow (I think autoconf configures do this), but I don't think that our 'print stuff to stdout' is the right place for that. thanks -- PMM
Re: [Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
On Fri, Jun 08, 2018 at 11:52:11AM -0300, Philippe Mathieu-Daudé wrote:
> On 06/01/2018 01:27 PM, Marc-André Lureau wrote:
> > A socket chardev may not have associated address (when adding client
> > fd manually for example). But on disconnect, updating socket filename
> > expects an address and may lead to this crash:
> >
> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> > "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> > /home/elmarco/src/qq/chardev/char-socket.c:388
> > 388 switch (addr->type) {
> > (gdb) bt
> > #0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> > "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> > /home/elmarco/src/qq/chardev/char-socket.c:388
> > #1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00)
> > at /home/elmarco/src/qq/chardev/char-socket.c:419
> > #2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
> > /home/elmarco/src/qq/chardev/char-socket.c:438
> > #3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690,
> > cond=G_IO_HUP, opaque=0x56b1ed00) at
> > /home/elmarco/src/qq/chardev/char-socket.c:482
> > #4 0x55da596e in qio_channel_fd_source_dispatch
> > (source=0x56bb68b0, callback=0x55d8cb58 ,
> > user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > chardev/char-socket.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 159e69c3b1..f1b7907798 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev
> > *s)
> > Chardev *chr = CHARDEV(s);
> >
> > g_free(chr->filename);
> > -chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> > - s->is_listen, s->is_telnet);
> > +chr->filename = NULL;
> > +if (s->addr) {
>
> Isn't it more robust to add this check in SocketAddress_to_str()?
IMHO that just shifts the problem elsewhere - currently SocketAddress_to_str
is assumed to return non-NULL, or to abort(). Shifting the check means it
can now return NULL, so there's every chance the caller will now reference
the NULL pointer that's returned.
>
> static char *SocketAddress_to_str(const char *prefix, SocketAddress
> *addr,
> bool is_listen, bool is_telnet)
> {
> if (!addr) {
> return NULL;
> }
> switch (addr->type) {
> ...
>
> > +chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> > + s->is_listen, s->is_telnet);
> > +}
> > }
> >
> > /* NB may be called even if tcp_chr_connect has not been
> >
>
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands
"Dr. David Alan Gilbert (git)" writes:
> From: "Dr. David Alan Gilbert"
>
> Allow the 'help' command in preconfig state but
> make it only list the preconfig commands.
>
> Signed-off-by: Dr. David Alan Gilbert
> Reviewed-by: Peter Xu
> Reviewed-by: Igor Mammedov
> ---
> hmp-commands.hx | 1 +
> monitor.c | 8 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 0734fea931..8bf590ae4b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -15,6 +15,7 @@ ETEXI
> .params = "[cmd]",
> .help = "show the help",
> .cmd= do_help_cmd,
> +.flags = "p",
> },
>
> STEXI
> diff --git a/monitor.c b/monitor.c
> index f4a16e6a03..31c8f5dc88 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
> {
> int i;
>
> +if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> +return;
> +}
> +
> for (i = 0; i < prefix_args_nb; i++) {
> monitor_printf(mon, "%s ", prefix_args[i]);
> }
> @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t
> *cmds,
>
> /* Find one entry to dump */
> for (cmd = cmds; cmd->name != NULL; cmd++) {
> -if (compare_cmd(args[arg_index], cmd->name)) {
> +if (compare_cmd(args[arg_index], cmd->name) &&
> +((!runstate_check(RUN_STATE_PRECONFIG) ||
> +cmd_can_preconfig(cmd {
Why do we need this check in addition to the one in help_cmd_dump_one()?
> if (cmd->sub_table) {
> /* continue with next arg */
> help_cmd_dump(mon, cmd->sub_table,
Re: [Qemu-devel] [PATCH v3 3/7] hmp: Restrict auto-complete in preconfig
"Dr. David Alan Gilbert (git)" writes:
> From: "Dr. David Alan Gilbert"
>
> Don't show the commands that aren't available.
>
> Signed-off-by: Dr. David Alan Gilbert
> Reviewed-by: Peter Xu
> Reviewed-by: Igor Mammedov
> ---
> monitor.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 31c8f5dc88..c369b392db 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3951,12 +3951,17 @@ static void monitor_find_completion_by_table(Monitor
> *mon,
> cmdname = args[0];
> readline_set_completion_index(mon->rs, strlen(cmdname));
> for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> -cmd_completion(mon, cmdname, cmd->name);
> +if (!runstate_check(RUN_STATE_PRECONFIG) ||
> + cmd_can_preconfig(cmd)) {
> +cmd_completion(mon, cmdname, cmd->name);
> +}
> }
> } else {
> /* find the command */
> for (cmd = cmd_table; cmd->name != NULL; cmd++) {
> -if (compare_cmd(args[0], cmd->name)) {
> +if (compare_cmd(args[0], cmd->name) &&
> +(!runstate_check(RUN_STATE_PRECONFIG) ||
> + cmd_can_preconfig(cmd))) {
> break;
> }
> }
Hmm, I keep seeing
!runstate_check(RUN_STATE_PRECONFIG) || cmd_can_preconfig(cmd)
Would a helper be worthwhile? cmd_available(cmd)?
Re: [Qemu-devel] [RFC v2 01/12] chardev: avoid crash if no associated address
On Fri, Jun 01, 2018 at 06:27:38PM +0200, Marc-André Lureau wrote:
> A socket chardev may not have associated address (when adding client
> fd manually for example). But on disconnect, updating socket filename
> expects an address and may lead to this crash:
>
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> 388 switch (addr->type) {
> (gdb) bt
> #0 0x55d8c70c in SocketAddress_to_str (prefix=0x56043062
> "disconnected:", addr=0x0, is_listen=false, is_telnet=false) at
> /home/elmarco/src/qq/chardev/char-socket.c:388
> #1 0x55d8c8aa in update_disconnected_filename (s=0x56b1ed00)
> at /home/elmarco/src/qq/chardev/char-socket.c:419
> #2 0x55d8c959 in tcp_chr_disconnect (chr=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:438
> #3 0x55d8cba1 in tcp_chr_hup (channel=0x56b75690,
> cond=G_IO_HUP, opaque=0x56b1ed00) at
> /home/elmarco/src/qq/chardev/char-socket.c:482
> #4 0x55da596e in qio_channel_fd_source_dispatch
> (source=0x56bb68b0, callback=0x55d8cb58 ,
> user_data=0x56b1ed00) at /home/elmarco/src/qq/io/channel-watch.c:84
>
> Signed-off-by: Marc-André Lureau
> ---
> chardev/char-socket.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..f1b7907798 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -416,8 +416,11 @@ static void update_disconnected_filename(SocketChardev
> *s)
> Chardev *chr = CHARDEV(s);
>
> g_free(chr->filename);
> -chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> - s->is_listen, s->is_telnet);
> +chr->filename = NULL;
> +if (s->addr) {
> +chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> + s->is_listen, s->is_telnet);
> +}
> }
This will mean 'chr->filename' as NULL, which means other code using this
field may get NULL - especially the monitor looks like it will printf()
passing a NULL, which is a crash on some platforms. So I think you need
an else clause that will set it to some dummy value.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg
On 07.06.2018 14:22, Thomas Huth wrote: > This patch series adds pxelinux.cfg-style network booting to the s390-ccw > firmware. The core pxelinux.cfg loading and parsing logic has recently > been merged to SLOF, so these patches now just have to make sure to call > the right functions to get the config file loaded and parsed. Once this is > done, the kernel and initrd are loaded separately, and are then glued > together in RAM. > > v2: > - Update SLOF submodule now that the git mirror is in sync again > - Last parameter to tftp_get_error_info() must not be NULL > - Check CC when calling STSI, and use a #define for the UUID offset > - Only support files with the magic "# pxelinux" string comment when >trying to guess the contents of a file that has been downloaded via >the "bootfile" DHCP parameter. This is just for developers' convenience, >the official way to specify pxelinux.cfg files is to use the DHCP >options 209 and 210 instead. > > Thomas Huth (4): > roms: Update SLOF submodule to current status > pc-bios/s390-ccw/net: Update code for the latest changes in SLOF > pc-bios/s390-ccw/net: Add support for pxelinux-style config files > pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the > UUID > > pc-bios/s390-ccw/netboot.mak | 9 +- > pc-bios/s390-ccw/netmain.c | 226 > +-- > roms/SLOF| 2 +- > 3 files changed, 162 insertions(+), 75 deletions(-) > I tested the series both with a self-created pxelinux.0 blob (to verify backward compatibility) and without an existing pxelinux.0 file (to force the standard pxelinux pattern). Both worked as expected, although the built-in search took significantly longer (timeout?). Tested-by: Viktor Mihajlovski -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg
On 06/07/2018 02:22 PM, Thomas Huth wrote: > This patch series adds pxelinux.cfg-style network booting to the s390-ccw > firmware. The core pxelinux.cfg loading and parsing logic has recently > been merged to SLOF, so these patches now just have to make sure to call > the right functions to get the config file loaded and parsed. Once this is > done, the kernel and initrd are loaded separately, and are then glued > together in RAM. > > v2: > - Update SLOF submodule now that the git mirror is in sync again > - Last parameter to tftp_get_error_info() must not be NULL > - Check CC when calling STSI, and use a #define for the UUID offset > - Only support files with the magic "# pxelinux" string comment when >trying to guess the contents of a file that has been downloaded via >the "bootfile" DHCP parameter. This is just for developers' convenience, >the official way to specify pxelinux.cfg files is to use the DHCP >options 209 and 210 instead. > > Thomas Huth (4): > roms: Update SLOF submodule to current status > pc-bios/s390-ccw/net: Update code for the latest changes in SLOF > pc-bios/s390-ccw/net: Add support for pxelinux-style config files > pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the > UUID > > pc-bios/s390-ccw/netboot.mak | 9 +- > pc-bios/s390-ccw/netmain.c | 226 > +-- > roms/SLOF| 2 +- > 3 files changed, 162 insertions(+), 75 deletions(-) > Series Acked-by: Christian Borntraeger
[Qemu-devel] [PATCH] util/async: avoid NULL pointer dereference
if laio_init create linux_aio failed and return NULL, NULL pointer
dereference will occur when laio_attach_aio_context dereference
linux_aio in aio_get_linux_aio, so add assert to avoid it.
Signed-off-by: Jie Wang
---
util/async.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/util/async.c b/util/async.c
index 03f62787f2..7766bcd8bc 100644
--- a/util/async.c
+++ b/util/async.c
@@ -327,6 +327,7 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx)
{
if (!ctx->linux_aio) {
ctx->linux_aio = laio_init();
+assert(ctx->linux_aio);
laio_attach_aio_context(ctx->linux_aio, ctx);
}
return ctx->linux_aio;
--
2.15.0.windows.1
Re: [Qemu-devel] [PATCH] Purge uses of banned g_assert_FOO()
On Mon, Jun 11, 2018 at 08:06:49AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
> > On 06/08/2018 02:12 PM, Daniel P. Berrangé wrote:
> >> On Fri, Jun 08, 2018 at 07:02:31PM +0200, Markus Armbruster wrote:
> >>> We banned use of certain g_assert_FOO() functions outside tests, and
> >>> made checkpatch.pl flag them (commit 6e9389563e5). We neglected to
> >>> purge existing uses. Do that now.
> >>>
> >>> Signed-off-by: Markus Armbruster
> >>> ---
> >>> hw/ide/ahci.c | 2 +-
> >>> hw/ppc/spapr_ovec.c | 12 ++--
> >>> hw/usb/dev-smartcard-reader.c | 2 +-
> >>> qom/object.c | 10 +-
> >>> util/qht.c| 2 +-
> >>> 5 files changed, 14 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> >>
> >>> diff --git a/hw/usb/dev-smartcard-reader.c b/hw/usb/dev-smartcard-reader.c
> >>> index 2131e33d27..13d0befd9c 100644
> >>> --- a/hw/usb/dev-smartcard-reader.c
> >>> +++ b/hw/usb/dev-smartcard-reader.c
> >>> @@ -786,7 +786,7 @@ static void ccid_write_data_block(USBCCIDState *s,
> >>> uint8_t slot, uint8_t seq,
> >>> DPRINTF(s, D_VERBOSE, "error %d\n", p->b.bError);
> >>> }
> >>> if (len) {
> >>> -g_assert_nonnull(data);
> >>> +assert(data);
> >>
> >> nitpick - all the other conversions used g_assert()
>
> The other conversions are in files that predominantly or exclusively use
> g_assert(). This one is in a file that exclusively uses assert().
Ah ok, makes sense then.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
On Mon, 11 Jun 2018 09:49:39 +0200
Christian Borntraeger wrote:
> On 06/10/2018 03:12 PM, Thomas Huth wrote:
> > Add a sanity check to fix the following crash:
> >
> > $ echo "Insane in the mainframe" > /tmp/test.txt
> > $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt
> > Segmentation fault (core dumped)
> >
> > Signed-off-by: Thomas Huth
>
> Reviewed-by: Christian Borntraeger
>
> I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE.
> No?
I think so as well.
>
>
> > ---
> > hw/s390x/ipl.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index 04245b5..9bb9b50 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error
> > **errp)
> > * we can not rely on the ELF entry point - it was 0x800 (the
> > SALIPL
> > * loader) and it won't work. For this case we force it to
> > 0x1, too.
> > */
> > -if (pentry == KERN_IMAGE_START || pentry == 0x800) {
> > +if ((pentry == KERN_IMAGE_START || pentry == 0x800) &&
> > +kernel_size > KERN_PARM_AREA + strlen(ipl->cmdline)) {
> > ipl->start_addr = KERN_IMAGE_START;
> > /* Overwrite parameters in the kernel image, which are "rom" */
> > strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
> >
>
The outcome of this is that we don't write into areas we must not write
into, but we still have a broken "kernel" and will simply fail if the
thing we're pointing to isn't a valid PSW. I guess that's what we want
("crap in, crap out"), i.e. no fallback to the bios or something like
that?
[Qemu-devel] [PATCH v3] net: Fix a potential segfault
If user forgets to provide any backend types for '-netdev' in qemu CLI,
It triggers seg fault.
e.g.
Expected:
$ qemu -netdev id=net0
qemu-system-x86_64: Parameter 'type' is missing
Actual:
$ qemu -netdev id=net0
Segmentation fault (core dumped)
Signed-off-by: Lin Ma
---
net/net.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/net.c b/net/net.c
index efb9eaf779..2a3133990c 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1093,7 +1093,9 @@ static int net_client_init(QemuOpts *opts, bool
is_netdev, Error **errp)
int ret = -1;
Visitor *v = opts_visitor_new(opts);
-if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
+const char *type = qemu_opt_get(opts, "type");
+
+if (is_netdev && type && is_help_option(type)) {
show_netdevs();
exit(0);
} else {
--
2.16.2
Re: [Qemu-devel] [PATCH v2 0/2] python: Remove unused compatibility modules
On Fri, Jun 08, 2018 at 02:52:50PM -0300, Eduardo Habkost wrote: > Changes v1 -> v2: > * Remove references to ordereddict.py from Makefiles > (oops) > > Now that we require Python >= 2.7, we don't need > scripts/argparse.py and scripts/ordereddict.py anymore. > > Eduardo Habkost (2): > python: Remove scripts/argparse.py FYI see v1 for some comments I sent on this path before noticing your v2. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH 1/2] python: Remove scripts/argparse.py
On Fri, Jun 08, 2018 at 02:35:57PM -0300, Eduardo Habkost wrote:
> Python 2.7 (the minimum Python version we require) already
> provides the argparse module on the standard library.
>
> Signed-off-by: Eduardo Habkost
> ---
> scripts/argparse.py | 2406 ---
> 1 file changed, 2406 deletions(-)
> delete mode 100644 scripts/argparse.py
When this was added in 47e1cb1f0a130baa438d895eff5d05004c9a9aa2, it
also added a COPYING.PYTHON file so that needs removing in this
commit too - eg a straight revert of that commit.
Second, various source files were then modified to add 'scripts' to
their import path with a line like this:
sys.path.append(os.path.join(os.path.dirname(__file__),
'..', '..', 'scripts'))
so those all need removing too, but I'd suggest that is done
as a separate patch.
> diff --git a/scripts/argparse.py b/scripts/argparse.py
> deleted file mode 100644
> index 27d1f28935..00
> --- a/scripts/argparse.py
> +++ /dev/null
> @@ -1,2406 +0,0 @@
> -# This is a local copy of the standard library argparse module taken from
> PyPI.
> -# It is licensed under the Python Software Foundation License. This is a
> -# fallback for Python 2.6 which does not include this module. Python 2.7+
> and
> -# 3+ will never load this module because built-in modules are loaded before
> -# anything in sys.path.
> -#
> -# If your script is not located in the same directory as this file, import it
> -# like this:
> -#
> -# import os
> -# import sys
> -# sys.path.append(os.path.join(os.path.dirname(__file__), ..., 'scripts'))
> -# import argparse
> -
> -# Author: Steven J. Bethard .
> -# Maintainer: Thomas Waldmann
> -
> -"""Command-line parsing library
> -
> -This module is an optparse-inspired command-line parsing library that:
> -
> -- handles both optional and positional arguments
> -- produces highly informative usage messages
> -- supports parsers that dispatch to sub-parsers
> -
> -The following is a simple usage example that sums integers from the
> -command-line and writes the result to a file::
> -
> -parser = argparse.ArgumentParser(
> -description='sum the integers at the command line')
> -parser.add_argument(
> -'integers', metavar='int', nargs='+', type=int,
> -help='an integer to be summed')
> -parser.add_argument(
> -'--log', default=sys.stdout, type=argparse.FileType('w'),
> -help='the file where the sum should be written')
> -args = parser.parse_args()
> -args.log.write('%s' % sum(args.integers))
> -args.log.close()
> -
> -The module contains the following public classes:
> -
> -- ArgumentParser -- The main entry point for command-line parsing. As the
> -example above shows, the add_argument() method is used to populate
> -the parser with actions for optional and positional arguments. Then
> -the parse_args() method is invoked to convert the args at the
> -command-line into an object with attributes.
> -
> -- ArgumentError -- The exception raised by ArgumentParser objects when
> -there are errors with the parser's actions. Errors raised while
> -parsing the command-line are caught by ArgumentParser and emitted
> -as command-line messages.
> -
> -- FileType -- A factory for defining types of files to be created. As the
> -example above shows, instances of FileType are typically passed as
> -the type= argument of add_argument() calls.
> -
> -- Action -- The base class for parser actions. Typically actions are
> -selected by passing strings like 'store_true' or 'append_const' to
> -the action= argument of add_argument(). However, for greater
> -customization of ArgumentParser actions, subclasses of Action may
> -be defined and passed as the action= argument.
> -
> -- HelpFormatter, RawDescriptionHelpFormatter, RawTextHelpFormatter,
> -ArgumentDefaultsHelpFormatter -- Formatter classes which
> -may be passed as the formatter_class= argument to the
> -ArgumentParser constructor. HelpFormatter is the default,
> -RawDescriptionHelpFormatter and RawTextHelpFormatter tell the parser
> -not to change the formatting for help text, and
> -ArgumentDefaultsHelpFormatter adds information about argument
> defaults
> -to the help.
> -
> -All other classes in this module are considered implementation details.
> -(Also note that HelpFormatter and RawDescriptionHelpFormatter are only
> -considered public as object names -- the API of the formatter objects is
> -still considered an implementation detail.)
> -"""
> -
> -__version__ = '1.4.0' # we use our own version number independent of the
> - # one in stdlib and we release this on pypi.
> -
> -__external_lib__ = True # to make sure the tests really test THIS lib,
> - # not the builtin
Re: [Qemu-devel] [PATCH] util/async: avoid NULL pointer dereference
On Mon, 06/11 15:04, Jie Wang wrote:
> if laio_init create linux_aio failed and return NULL, NULL pointer
> dereference will occur when laio_attach_aio_context dereference
> linux_aio in aio_get_linux_aio, so add assert to avoid it.
>
> Signed-off-by: Jie Wang
> ---
> util/async.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/util/async.c b/util/async.c
> index 03f62787f2..7766bcd8bc 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -327,6 +327,7 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx)
> {
> if (!ctx->linux_aio) {
> ctx->linux_aio = laio_init();
> +assert(ctx->linux_aio);
> laio_attach_aio_context(ctx->linux_aio, ctx);
> }
> return ctx->linux_aio;
> --
I'm afraid this is not the correct fix. If laio_init() can fail, this function
should skip laio_attach_aio_context() and return NULL, then callers should check
the return value and handle the error. E.g. Set s->use_linux_aio to false and
fall back to posix I/O, and perhaps report the error with error_report. Or even
better, call laio_init during raw_open and use error_setg(errp, ...).
assert() will simply crash the program, it is not the right way to catch errors.
Fam
Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
* Peter Maydell (peter.mayd...@linaro.org) wrote: > On 11 June 2018 at 08:56, Markus Armbruster wrote: > > You're not printing $strchrnul like we print other configuration > > results. Hmm, we're not printing several of them. Question for > > maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three > > coughed up by get_maintainer.pl): bug or feature? If feature, how do we > > decide what to print? > > If we printed everything that we tested for then the output would > be unhelpfully enormous. My view is that we should print the > "interesting" things for the user, ie the higher-level things > that the user could potentially turn on by installing more > libraries or has turned off explicitly or whatever. Reporting > whether the host OS has strchrnul or whether we've had to > provide our own implementation is doubly uninteresting: > * there's nothing the user could do to change this > * there is no visible effect (missing features, worse performance) > > There's an argument that we should also log every config check > result somehow (I think autoconf configures do this), but I > don't think that our 'print stuff to stdout' is the right place > for that. We're not completely consistent, but I agree that we shouldn't print the tiny things: We're printing things that: a) Are user visible (e.g. KVM support) b) Reasonably major choices we make (e.g. coroutine backend) c) Some lesser things (madvise/posix_madvise/posix_memalign) However even (c) could be a problem if none were found We probably shouldn't bother with printing things that have no impact to either the user, or someone trying to cofnigure it and wondering why a feature is missing. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
On Mon, Jun 11, 2018 at 09:52:55AM +0100, Peter Maydell wrote: > On 11 June 2018 at 08:56, Markus Armbruster wrote: > > You're not printing $strchrnul like we print other configuration > > results. Hmm, we're not printing several of them. Question for > > maintainers (MAINTAINERS doesn't have any, so I'm cc'ing the top three > > coughed up by get_maintainer.pl): bug or feature? If feature, how do we > > decide what to print? > > If we printed everything that we tested for then the output would > be unhelpfully enormous. My view is that we should print the > "interesting" things for the user, ie the higher-level things > that the user could potentially turn on by installing more > libraries or has turned off explicitly or whatever. Reporting > whether the host OS has strchrnul or whether we've had to > provide our own implementation is doubly uninteresting: > * there's nothing the user could do to change this > * there is no visible effect (missing features, worse performance) > > There's an argument that we should also log every config check > result somehow (I think autoconf configures do this), but I > don't think that our 'print stuff to stdout' is the right place > for that. If you look at most non-trivial apps using autoconf, they have soo many checks printed out, that they end up having to manually print out a summary of the "important stuff" at the end so users can actually see something they have a chance of reading. QEMU's configure output is essentially equivalent to this summary data, which is good. It is sometimes useful to know the answer of individual checks, but if we really wanted todo that, we should just create a logfile for that info, or just write more into our existing config.log file. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[Qemu-devel] [Bug 1769053] Re: Cannot start a guest with more than 1TB of RAM
Since all but the libvirt task to expose these are set to invalid in regard to the issue here I'm changing the title accordingly. As a short term solution for Ubuntu users I forked bug 1776189 to provide a machine type based solution until this here is implemented and widely available and exploited. ** Summary changed: - Cannot start a guest with more than 1TB of RAM + Ability to control phys-bits through libvirt -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1769053 Title: Ability to control phys-bits through libvirt Status in libvirt: Unknown Status in QEMU: Invalid Status in libvirt package in Ubuntu: Triaged Status in qemu package in Ubuntu: Invalid Bug description: Attempting to start a KVM guest with more than 1TB of RAM fails. It looks like we might need some extra patches: https://lists.gnu.org/archive/html/qemu-discuss/2017-12/msg5.html ProblemType: Bug DistroRelease: Ubuntu 18.04 Package: qemu-system-x86 1:2.11+dfsg-1ubuntu7 ProcVersionSignature: Ubuntu 4.15.0-20.21-generic 4.15.17 Uname: Linux 4.15.0-20-generic x86_64 ApportVersion: 2.20.9-0ubuntu7 Architecture: amd64 CurrentDesktop: Unity:Unity7:ubuntu Date: Fri May 4 16:21:14 2018 InstallationDate: Installed on 2017-04-05 (393 days ago) InstallationMedia: Ubuntu 16.10 "Yakkety Yak" - Release amd64 (20161012.2) MachineType: Dell Inc. XPS 13 9360 ProcKernelCmdLine: BOOT_IMAGE=/vmlinuz-4.15.0-20-generic root=/dev/mapper/ubuntu--vg-root ro quiet splash transparent_hugepage=madvise vt.handoff=1 SourcePackage: qemu UpgradeStatus: Upgraded to bionic on 2018-04-30 (3 days ago) dmi.bios.date: 02/26/2018 dmi.bios.vendor: Dell Inc. dmi.bios.version: 2.6.2 dmi.board.name: 0PF86Y dmi.board.vendor: Dell Inc. dmi.board.version: A00 dmi.chassis.type: 9 dmi.chassis.vendor: Dell Inc. dmi.modalias: dmi:bvnDellInc.:bvr2.6.2:bd02/26/2018:svnDellInc.:pnXPS139360:pvr:rvnDellInc.:rn0PF86Y:rvrA00:cvnDellInc.:ct9:cvr: dmi.product.family: XPS dmi.product.name: XPS 13 9360 dmi.sys.vendor: Dell Inc. To manage notifications about this bug go to: https://bugs.launchpad.net/libvirt/+bug/1769053/+subscriptions
Re: [Qemu-devel] [PATCH v3] net: Fix a potential segfault
On 11.06.2018 11:23, Lin Ma wrote:
> If user forgets to provide any backend types for '-netdev' in qemu CLI,
> It triggers seg fault.
>
> e.g.
>
> Expected:
> $ qemu -netdev id=net0
> qemu-system-x86_64: Parameter 'type' is missing
>
> Actual:
> $ qemu -netdev id=net0
> Segmentation fault (core dumped)
>
> Signed-off-by: Lin Ma
> ---
> net/net.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/net.c b/net/net.c
> index efb9eaf779..2a3133990c 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1093,7 +1093,9 @@ static int net_client_init(QemuOpts *opts, bool
> is_netdev, Error **errp)
> int ret = -1;
> Visitor *v = opts_visitor_new(opts);
>
> -if (is_netdev && is_help_option(qemu_opt_get(opts, "type"))) {
> +const char *type = qemu_opt_get(opts, "type");
> +
> +if (is_netdev && type && is_help_option(type)) {
> show_netdevs();
> exit(0);
> } else {
>
Reviewed-by: Thomas Huth
Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
On 11.06.2018 11:24, Cornelia Huck wrote:
> On Mon, 11 Jun 2018 09:49:39 +0200
> Christian Borntraeger wrote:
>
>> On 06/10/2018 03:12 PM, Thomas Huth wrote:
>>> Add a sanity check to fix the following crash:
>>>
>>> $ echo "Insane in the mainframe" > /tmp/test.txt
>>> $ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt
>>> Segmentation fault (core dumped)
>>>
>>> Signed-off-by: Thomas Huth
>>
>> Reviewed-by: Christian Borntraeger
>>
>> I think a similar problem exists for INITRD_PARM_START and INITRD_PARM_SIZE.
>> No?
>
> I think so as well.
You're right:
$ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt \
-initrd /tmp/test.txt
Segmentation fault (core dumped)
Shall I sent a v2 of this patch, or do you prefer a separate patch for
that issue?
>>> ---
>>> hw/s390x/ipl.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index 04245b5..9bb9b50 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error
>>> **errp)
>>> * we can not rely on the ELF entry point - it was 0x800 (the
>>> SALIPL
>>> * loader) and it won't work. For this case we force it to
>>> 0x1, too.
>>> */
>>> -if (pentry == KERN_IMAGE_START || pentry == 0x800) {
>>> +if ((pentry == KERN_IMAGE_START || pentry == 0x800) &&
>>> +kernel_size > KERN_PARM_AREA + strlen(ipl->cmdline)) {
>>> ipl->start_addr = KERN_IMAGE_START;
>>> /* Overwrite parameters in the kernel image, which are "rom" */
>>> strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>
> The outcome of this is that we don't write into areas we must not write
> into, but we still have a broken "kernel" and will simply fail if the
> thing we're pointing to isn't a valid PSW. I guess that's what we want
> ("crap in, crap out"), i.e. no fallback to the bios or something like
> that?
Yes, I think "crap in, crap out" is ok here. Theoretically, the user
could also have a self-made micro-kernel that is just one byte smaller
than KERN_PARM_AREA, and this would still work with this patch, so no
need for an extra error message in that case.
Thomas
Re: [Qemu-devel] [PATCH v4] cutils: Provide strchrnul
On 11 June 2018 at 10:38, Daniel P. Berrangé wrote: > On Mon, Jun 11, 2018 at 09:52:55AM +0100, Peter Maydell wrote: >> There's an argument that we should also log every config check >> result somehow (I think autoconf configures do this), but I >> don't think that our 'print stuff to stdout' is the right place >> for that. > It is sometimes useful to know the answer of individual checks, but > if we really wanted todo that, we should just create a logfile for > that info, or just write more into our existing config.log file. Yes, that's what I had in mind. On the other hand, if we're not in practice running into issues from users that would be easier to debug with that detailed log info, it doesn't seem worth the effort of trying to improve our config.log data... thanks -- PMM
Re: [Qemu-devel] [PULL 00/31] target-arm queue
On 8 June 2018 at 13:44, Peter Maydell wrote: > target-arm queue: aspeed patches from Cédric, and > cleanup and sd card patches from Philippe. > > thanks > -- PMM > > The following changes since commit bac5ba3dc5da706f52c149fa6c0bd1dc96899bec: > > Merge remote-tracking branch 'remotes/thibault/tags/samuel-thibault' into > staging (2018-06-08 10:26:16 +0100) > > are available in the Git repository at: > > git://git.linaro.org/people/pmaydell/qemu-arm.git > tags/pull-target-arm-20180608 > > for you to fetch changes up to 113f31c06c6bf16451892b2459d83c9b9c5e9844: > > sdcard: Disable CMD19/CMD23 for Spec v2 (2018-06-08 13:15:34 +0100) > > Applied, thanks. -- PMM
[Qemu-devel] reminder: soft freeze is in three weeks
Reminder that 3.0 soft freeze is due in about 3 weeks, on the 3rd July. Please try to get new features in in good time so we don't have a last-minute deluge of huge pull requests :-) thanks -- PMM
[Qemu-devel] [Bug 1775555] Re: guest migration 100% cpu freeze bug
I do like to add that I only saw this when the source we migrate from is running on a relatively new CPU: Intel(R) Xeon(R) Gold 6126 CPU @ 2.60GHz. vendor_id : GenuineIntel cpu family : 6 model : 85 model name : Intel(R) Xeon(R) Gold 6126 CPU @ 2.60GHz stepping: 4 microcode : 0x22c cpu MHz : 1048.950 cache size : 19712 KB physical id : 0 siblings: 24 core id : 6 cpu cores : 12 apicid : 12 initial apicid : 12 fpu : yes fpu_exception : yes cpuid level : 22 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc art arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch epb invpcid_single retpoline intel_pt kaiser tpr_shadow vnmi flexpriority ept vpid fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm cqm mpx avx512f avx512dq rdseed adx smap clflushopt clwb avx512cd avx512bw avx512vl xsaveopt xsavec xgetbv1 xsaves cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local dtherm ida arat pln pts hwp hwp_act_window hwp_epp hwp_pkg_req pku ospke bugs: cpu_meltdown spectre_v1 spectre_v2 bogomips: 5200.00 clflush size: 64 cache_alignment : 64 address sizes : 46 bits physical, 48 bits virtual power management: I did use this CPU at the source while migrating in loops but was not able to reproduce. > You're doing a heck of a lot of migrates; does 2.6.2->2.6.2 and > 2.11.1->2.11.1 work fine for you - i.e. is it only the cross version that's > broken? I did not see this issue amongst 2.6.2 -> 2.6.2 migrations, but I do have to add that it wasn't that obvious. I did not see any of these issues during 2.6.2 -> 2.6.2 migrations and also "not yet" between 2.11.1 -> 2.11.1, I am writing "not yet" as I am not really migrating that much between 2.11.1 at this moment. What I can do is migrate VMs around and skip the "Gold 6126" source CPU so we can perhaps rule this out. VMs having this issue are not that hard to pinpoint because of the spiking vcpus. I will try to run crash against a dump of the VMs memory with an exact copy of the running kernel to try and get a state of the running kernel and trying to read dmesg. > You should probably look at what the other CPUs are doing as well. I tried this but attaching gdb and running info registers or stepping through does not show me the same RIP values as I see with the `qemu-monitor-command` info registers command. Is there another way so I could get the register values of all individual vcpus? > You could take the kernel RIP values you see in your last test and see if they match some particular driver; make sure you have *exactly* the same guest kernel version when trying to match the values up. I will get back to you on this. -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/177 Title: guest migration 100% cpu freeze bug Status in QEMU: New Bug description: # Investigate migration cpu hog(100%) bug I have some issues when migrating from qemu 2.6.2 to qemu 2.11.1. The hypervisors are running kernel 4.9.92 on debian stretch with libvirt v4.0.0. Linux, libvirt and qemu are all custom compiled. I migrated around 21.000 vms from qemu 2.6.2 to qemu 2.11.1 and every once in a while a vm is stuck at 100% cpu after the migration from 2.6.2 to 2.11.1. This happend with about 50-60 vms so far. I attached gdb to a vcpu thread of one stuck vm, and a bt showed the following info: #0 0x7f4f19949dd7 in ioctl () at ../sysdeps/unix/syscall-template.S:84 #1 0x557c9edede47 in kvm_vcpu_ioctl (cpu=cpu@entry=0x557ca1058840, type=type@entry=0xae80) at /home/dbosschieter/src/qemu-pkg/src/accel/kvm/kvm-all.c:2050 #2 0x557c9ededfb6 in kvm_cpu_exec (cpu=cpu@entry=0x557ca1058840) at /home/dbosschieter/src/qemu-pkg/src/accel/kvm/kvm-all.c:1887 #3 0x557c9edcab44 in qemu_kvm_cpu_thread_fn (arg=0x557ca1058840) at /home/dbosschieter/src/qemu-pkg/src/cpus.c:1128 #4 0x7f4f19c0f494 in start_thread (arg=0x7f4f053f3700) at pthread_create.c:333 #5 0x7f4f19951acf in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97 The ioctl call is a ioctl(18, KVM_RUN and it looks like it is looping inside the vm itself. I saved the state of the VM (with `virsh save`) after I found it was hanging on its vcpu threads. Then I restored this vm on a test environment running the same kernel, QEMU and libvirt version). After the restore the VM still was haning at 100% cpu usage on all the vcpus. I tried to use the perf kvm gue
Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands
* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" writes:
>
> > From: "Dr. David Alan Gilbert"
> >
> > Allow the 'help' command in preconfig state but
> > make it only list the preconfig commands.
> >
> > Signed-off-by: Dr. David Alan Gilbert
> > Reviewed-by: Peter Xu
> > Reviewed-by: Igor Mammedov
> > ---
> > hmp-commands.hx | 1 +
> > monitor.c | 8 +++-
> > 2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index 0734fea931..8bf590ae4b 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -15,6 +15,7 @@ ETEXI
> > .params = "[cmd]",
> > .help = "show the help",
> > .cmd= do_help_cmd,
> > +.flags = "p",
> > },
> >
> > STEXI
> > diff --git a/monitor.c b/monitor.c
> > index f4a16e6a03..31c8f5dc88 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
> > {
> > int i;
> >
> > +if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
> > +return;
> > +}
> > +
> > for (i = 0; i < prefix_args_nb; i++) {
> > monitor_printf(mon, "%s ", prefix_args[i]);
> > }
> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const mon_cmd_t
> > *cmds,
> >
> > /* Find one entry to dump */
> > for (cmd = cmds; cmd->name != NULL; cmd++) {
> > -if (compare_cmd(args[arg_index], cmd->name)) {
> > +if (compare_cmd(args[arg_index], cmd->name) &&
> > +((!runstate_check(RUN_STATE_PRECONFIG) ||
> > +cmd_can_preconfig(cmd {
>
> Why do we need this check in addition to the one in help_cmd_dump_one()?
To gate entire subtables (we've only currently got 'info' and that's enabled,
anyway, so it never actually triggers).
Dave
> > if (cmd->sub_table) {
> > /* continue with next arg */
> > help_cmd_dump(mon, cmd->sub_table,
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] An emulation failure occurs, if I hotplug vcpus immediately after the VM start
On 07.06.2018 18:03, 浙大邮箱 wrote:
> Hi，all
> I still have a question after reading your discussion: Will seabios detect
> the change of address space even if we add_region and del_region
> automatically? I guess that seabios may not take this change into
> consideration.
Hi,
We would just change the way how KVM memory slots are updated. This is
right now not atomic, but would be later on. It should not have any
other effect.
Thanks
>
> Looking forward to your replies,
> Thanks
>
>>> On Jun 7, 2018, at 20:55, David Hildenbrand wrote:
>>>
>>> On 07.06.2018 14:36, Paolo Bonzini wrote:
>>> On 07/06/2018 13:36, David Hildenbrand wrote:
> The dirty bitmap would be synced in kvm_region_del (so it's not true
> that kvm_region_del would disappear, but almost :)).
I was rather concerned when doing a KVM_SET_USER_MEMORY_REGIONS while
some (already present) memory region is performing dirty tracking and
therefore has a dirty_bitmap pointer assigned.
As we have to expect that all different kinds of parameters can change
(e.g. the size of a slot as I pointed out), the old bitmap cannot be
reused. At least not atomically - we could create a new one and the
simply or the old content.
Well, we could make that dirty tracking a special case ("all dirty
tracking data will be lost in case doing a KVM_SET_USER_MEMORY_REGIONS"
- but this again could lead to races (if the bitmap sync happens before
KVM_SET_USER_MEMORY_REGIONS)). It's tricky to get it right :)
>>>
>>> At the point where QEMU calls region_del, the guest is not supposed to
>>> access the region anymore, so it's okay to do the final sync there. The
>>> same race exists now already.
>>
>> The point is that KVM_SET_USER_MEMORY_REGIONS is the big hammer, not the
>> fine grained region_add/region_del. All regions are suddenly involved.
>> So we have to handle dirty_bitmaps somehow.
>>
>> But I agree that those that would actually change
>> (split/resized/whatever) should not be currently dirty tracked (or at if
>> they are, they should be able to live with the possible race).
>>
>> Devil is in the detail :)
>>
>>>
>>> Paolo
>>>
> The rmap is more interesting. Perhaps it can be just rebuilt on every
> KVM_SET_USER_MEMORY_REGIONS call.
>>
>>
>> --
>>
>> Thanks,
>>
>> David / dhildenb
>
--
Thanks,
David / dhildenb
Re: [Qemu-devel] [PATCH v6 00/49] fix building of tests/tcg
On 06/11/2018 05:19 AM, Alex Bennée wrote: > Philippe Mathieu-Daudé writes: >> On 06/08/2018 09:32 AM, Alex Bennée wrote: >>> Hi, >>> >>> Not a super amount has changed since the last version but review >>> comments and review tags have been added. The new patches at the end >>> enable a .travis.yml run and try and make the image building part of >>> check-tcg -j safe. Essentially the problem is trying to avoid >>> re-building the images multiple times. The additional issue is wanting >>> an upto date Debian SID image whenever we actually build an image but >>> not forcing a rebuild every time. >>> >>> Essentially I'd like to encode a conditional dependency when: >>> - the target image doesn't exist >>> - or the target image is out of date w.r.t. the dockerfike >>> >>> I'm thinking this is going to involve some sort of extension to the >>> docker.py script to feed the Makefile. >>> >>> A number of the prerequisite patches have already been pulled in the >>> docker fixes series however I think this series is ready to go in now. >>> Unless there are any objections I'll send a pull on Monday. >>> >>> Current unreviewed patches: >>> >>> patch 0017/tests tcg i386 add runner for test i386 fprem.patch needs >>> review >>> patch 0039/docker move debian powerpc cross to sid based bui.patch needs >>> review >>> patch 0040/tests tcg enable building for PowerPC.patch needs review >>> patch 0042/Makefile.target add clean build guest tests targe.patch needs >>> review >>> patch 0044/tests tcg add run diff and skip helper macros.patch needs >>> review >>> patch 0045/tests tcg override runners for broken tests.patch needs review >>> patch 0046/target sh4 Fix translator.c assertion failure for.patch needs >>> review >> >> Is this patch related/required to this series? >> It looks it should enter via another tree. >> >> The SH4 tests pass without it: >> >> $ make run-tcg-tests-sh4-linux-user -j1 >> ... >> RUN-TESTS for sh4 >> TESTtest-mmap (default) on sh4 >> TESTsha1 on sh4 >> TESTlinux-test on sh4 >> TESTtestthread on sh4 >> TESTtest-mmap (4096 byte pages) on sh4 > > Not with --enable-debug, but yeah I'll drop as it should come via Rich's > TCG tree. Oh I just tested the basic options, didn't go with all the matrix :/ Indeed, RUN-TESTS for sh4 TESTtest-mmap (default) on sh4 TESTsha1 on sh4 TESTlinux-test on sh4 TESTtestthread on sh4 qemu-sh4: accel/tcg/translator.c:66: translator_loop: Assertion `db->is_jmp == DISAS_NEXT' failed. qemu: uncaught target signal 11 (Segmentation fault) - core dumped Segmentation fault make: *** [tests/Makefile.include:950: run-tcg-tests-sh4-linux-user] Error 2 I'd still prefer this enter via Richard tree, and keep this PR testing-only (in case if bisection in translate.c, this might goes very slow due to your changes in ./configure triggering full rebuild). > >> >>> patch 0047/tests add top level make dependency for docker bu.patch needs >>> review >>> patch 0048/tests docker prevent sub makes re building debian.patch needs >>> review >>> patch 0049/.travis.yml add check tcg test.patch needs review >>> >>> >>> Alex Bennée (46): >>> configure: add support for --cross-cc-FOO >>> configure: move i386_cc to cross_cc_i386 >>> configure: allow user to specify --cross-cc-cflags-foo= >>> configure: set cross_cc_FOO for host compiler >>> docker: Add "cc" subcommand >>> docker: extend "cc" command to accept compiler >>> docker: allow "cc" command to run in user context >>> docker: Makefile.include introduce DOCKER_SCRIPT >>> tests/tcg: move architecture independent tests into subdir >>> tests/tcg/multiarch: enable additional linux-test tests >>> tests/tcg/multiarch: move most output to stdout >>> tests/tcg: move i386 specific tests into subdir >>> tests/tcg: enable building for i386 >>> tests/tcg/i386: fix test-i386 >>> tests/tcg/i386: add runner for test-i386-fprem >>> tests/tcg/x86_64: add Makefile.target >>> tests/tcg/i386/test-i386: use modern vector_size attributes >>> tests/tcg/i386/test-i386: fix printf format >>> tests/tcg: move ARM specific tests into subdir >>> tests/tcg: enable building for ARM >>> tests/tcg/arm: fix up test-arm-iwmmxt test >>> tests/tcg: enable building for AArch64 >>> tests/tcg/arm: add fcvt test cases for AArch32/64 >>> tests/tcg: move MIPS specific tests into subdir >>> tests/tcg: enable building for MIPS >>> tests/tcg/mips: include common mips hello-mips >>> tests/tcg: enable building for s390x >>> tests/tcg: enable building for ppc64 >>> tests/tcg: enable building for Alpha >>> tests/tcg/alpha: add Alpha specific tests >>> tests/tcg: enable building for HPPA >>> tests/tcg: enable building for m68k >>> tests/tcg: enable building for sh4 >>> tests/tcg: enable building for sparc64 >>> tests/tcg: enable building for mips64 >>> tests/tcg: enable building for RISCV64 >>> docker: move debian-powerpc-cross
[Qemu-devel] [PULL 1/9] target/m68k: Use DISAS_NORETURN for exceptions
From: Richard Henderson
The raise_exception helper does not return. Do not generate
any code following that.
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-2-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 4b5dbdb51c..de1be58f65 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -291,18 +291,18 @@ static void gen_jmp(DisasContext *s, TCGv dest)
s->is_jmp = DISAS_JUMP;
}
-static void gen_raise_exception(int nr)
+static void gen_exception(DisasContext *s, uint32_t dest, int nr)
{
-TCGv_i32 tmp = tcg_const_i32(nr);
+TCGv_i32 tmp;
+update_cc_op(s);
+tcg_gen_movi_i32(QREG_PC, dest);
+
+tmp = tcg_const_i32(nr);
gen_helper_raise_exception(cpu_env, tmp);
tcg_temp_free_i32(tmp);
-}
-static void gen_exception(DisasContext *s, uint32_t where, int nr)
-{
-gen_jmp_im(s, where);
-gen_raise_exception(nr);
+s->is_jmp = DISAS_NORETURN;
}
static inline void gen_addr_fault(DisasContext *s)
@@ -6106,7 +6106,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock
*tb)
if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
gen_exception(dc, dc->pc, EXCP_DEBUG);
-dc->is_jmp = DISAS_JUMP;
/* The address covered by the breakpoint must be included in
[tb->pc, tb->pc + tb->size) in order to for it to be
properly cleared -- thus we increment the PC here so that
@@ -6150,6 +6149,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock
*tb)
tcg_gen_exit_tb(NULL, 0);
break;
case DISAS_TB_JUMP:
+case DISAS_NORETURN:
/* nothing more to generate */
break;
}
--
2.14.4
[Qemu-devel] [PULL 0/9] M68k for 3.0 patches
The following changes since commit 0d2fa03dae4fbe185a082f361342b1e30aed4582: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180608' into staging (2018-06-08 16:26:51 +0100) are available in the Git repository at: git://github.com/vivier/qemu-m68k.git tags/m68k-for-3.0-pull-request for you to fetch changes up to a56f36c1d2bccbc50a53fa8093b93d205607f1b8: target/m68k: Merge disas_m68k_insn into m68k_tr_translate_insn (2018-06-11 12:43:42 +0200) Convert to TranslatorOps I've updated the series to fix conflicts with: 21528149eb target/m68k: Add trailing '\n' to qemu_log() call 07ea28b418 tcg: Pass tb and index to tcg_gen_exit_tb separately Richard Henderson (9): target/m68k: Use DISAS_NORETURN for exceptions target/m68k: Replace DISAS_TB_JUMP with DISAS_NORETURN target/m68k: Remove DISAS_JUMP_NEXT as unused target/m68k: Use lookup_and_goto_tb for DISAS_JUMP target/m68k: Rename DISAS_UPDATE and gen_lookup_tb target/m68k: Convert to DisasContextBase target/m68k: Convert to TranslatorOps target/m68k: Improve ending TB at page boundaries target/m68k: Merge disas_m68k_insn into m68k_tr_translate_insn target/m68k/translate.c | 356 1 file changed, 180 insertions(+), 176 deletions(-) -- 2.14.4
[Qemu-devel] [PULL 4/9] target/m68k: Use lookup_and_goto_tb for DISAS_JUMP
From: Richard Henderson These are all indirect or out-of-page direct jumps. We can indirectly chain to the next TB without going back to the main loop. Signed-off-by: Richard Henderson Message-Id: <20180512050250.12774-5-richard.hender...@linaro.org> Signed-off-by: Laurent Vivier --- target/m68k/translate.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/target/m68k/translate.c b/target/m68k/translate.c index 6238d9edc9..4b92a20c05 100644 --- a/target/m68k/translate.c +++ b/target/m68k/translate.c @@ -6139,8 +6139,11 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb) update_cc_op(dc); gen_jmp_tb(dc, 0, dc->pc); break; -default: case DISAS_JUMP: +/* We updated CC_OP and PC in gen_jmp/gen_jmp_im. */ +tcg_gen_lookup_and_goto_ptr(); +break; +default: case DISAS_UPDATE: update_cc_op(dc); /* indicate that the hash table must be used to find the next TB */ -- 2.14.4
[Qemu-devel] [PULL 2/9] target/m68k: Replace DISAS_TB_JUMP with DISAS_NORETURN
From: Richard Henderson We have exited the TB after using goto_tb; there is no distinction from DISAS_NORETURN. Signed-off-by: Richard Henderson Message-Id: <20180512050250.12774-3-richard.hender...@linaro.org> Signed-off-by: Laurent Vivier --- target/m68k/translate.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/target/m68k/translate.c b/target/m68k/translate.c index de1be58f65..bfa30cde0a 100644 --- a/target/m68k/translate.c +++ b/target/m68k/translate.c @@ -199,7 +199,6 @@ static void do_writebacks(DisasContext *s) /* is_jmp field values */ #define DISAS_JUMP DISAS_TARGET_0 /* only pc was modified dynamically */ #define DISAS_UPDATEDISAS_TARGET_1 /* cpu state was modified dynamically */ -#define DISAS_TB_JUMP DISAS_TARGET_2 /* only pc was modified statically */ #define DISAS_JUMP_NEXT DISAS_TARGET_3 #if defined(CONFIG_USER_ONLY) @@ -1496,7 +1495,7 @@ static void gen_jmp_tb(DisasContext *s, int n, uint32_t dest) gen_jmp_im(s, dest); tcg_gen_exit_tb(NULL, 0); } -s->is_jmp = DISAS_TB_JUMP; +s->is_jmp = DISAS_NORETURN; } DISAS_INSN(scc) @@ -6148,7 +6147,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb) /* indicate that the hash table must be used to find the next TB */ tcg_gen_exit_tb(NULL, 0); break; -case DISAS_TB_JUMP: case DISAS_NORETURN: /* nothing more to generate */ break; -- 2.14.4
Re: [Qemu-devel] [PATCH] hw/s390x/ipl: Fix crash that occurs when -kernel is used with small images
On 06/11/2018 12:08 PM, Thomas Huth wrote:
> On 11.06.2018 11:24, Cornelia Huck wrote:
>> On Mon, 11 Jun 2018 09:49:39 +0200
>> Christian Borntraeger wrote:
>>
>>> On 06/10/2018 03:12 PM, Thomas Huth wrote:
Add a sanity check to fix the following crash:
$ echo "Insane in the mainframe" > /tmp/test.txt
$ s390x-softmmu/qemu-system-s390x -nographic -kernel /tmp/test.txt
Segmentation fault (core dumped)
Signed-off-by: Thomas Huth
>>>
>>> Reviewed-by: Christian Borntraeger
>>>
>>> I think a similar problem exists for INITRD_PARM_START and
>>> INITRD_PARM_SIZE. No?
>>
>> I think so as well.
>
> You're right:
>
> $ s390x-softmmu/qemu-system-s390x -kernel /tmp/test.txt \
> -initrd /tmp/test.txt
> Segmentation fault (core dumped)
>
> Shall I sent a v2 of this patch, or do you prefer a separate patch for
> that issue?
Whatever is easier for you.
>
---
hw/s390x/ipl.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 04245b5..9bb9b50 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -168,7 +168,8 @@ static void s390_ipl_realize(DeviceState *dev, Error
**errp)
* we can not rely on the ELF entry point - it was 0x800 (the
SALIPL
* loader) and it won't work. For this case we force it to
0x1, too.
*/
-if (pentry == KERN_IMAGE_START || pentry == 0x800) {
+if ((pentry == KERN_IMAGE_START || pentry == 0x800) &&
+kernel_size > KERN_PARM_AREA + strlen(ipl->cmdline)) {
ipl->start_addr = KERN_IMAGE_START;
/* Overwrite parameters in the kernel image, which are "rom"
*/
strcpy(rom_ptr(KERN_PARM_AREA), ipl->cmdline);
>>
>> The outcome of this is that we don't write into areas we must not write
>> into, but we still have a broken "kernel" and will simply fail if the
>> thing we're pointing to isn't a valid PSW. I guess that's what we want
>> ("crap in, crap out"), i.e. no fallback to the bios or something like
>> that?
>
> Yes, I think "crap in, crap out" is ok here. Theoretically, the user
> could also have a self-made micro-kernel that is just one byte smaller
> than KERN_PARM_AREA, and this would still work with this patch, so no
> need for an extra error message in that case.
[Qemu-devel] [PULL 5/9] target/m68k: Rename DISAS_UPDATE and gen_lookup_tb
From: Richard Henderson
The name gen_lookup_tb is at odds with tcg_gen_lookup_and_goto_tb.
For these cases, we do indeed want to exit back to the main loop.
Similarly, DISAS_UPDATE performs no actual update, whereas DISAS_EXIT
does what it says.
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-6-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 4b92a20c05..0cbd715550 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -198,7 +198,7 @@ static void do_writebacks(DisasContext *s)
/* is_jmp field values */
#define DISAS_JUMP DISAS_TARGET_0 /* only pc was modified dynamically */
-#define DISAS_UPDATEDISAS_TARGET_1 /* cpu state was modified dynamically */
+#define DISAS_EXIT DISAS_TARGET_1 /* cpu state was modified dynamically */
#if defined(CONFIG_USER_ONLY)
#define IS_USER(s) 1
@@ -1446,11 +1446,11 @@ static void gen_jmpcc(DisasContext *s, int cond,
TCGLabel *l1)
}
/* Force a TB lookup after an instruction that changes the CPU state. */
-static void gen_lookup_tb(DisasContext *s)
+static void gen_exit_tb(DisasContext *s)
{
update_cc_op(s);
tcg_gen_movi_i32(QREG_PC, s->pc);
-s->is_jmp = DISAS_UPDATE;
+s->is_jmp = DISAS_EXIT;
}
#define SRC_EA(env, result, opsize, op_sign, addrp) do {\
@@ -4604,7 +4604,7 @@ DISAS_INSN(move_to_sr)
return;
}
gen_move_to_sr(env, s, insn, false);
-gen_lookup_tb(s);
+gen_exit_tb(s);
}
DISAS_INSN(move_from_usp)
@@ -4680,7 +4680,7 @@ DISAS_INSN(cf_movec)
reg = DREG(ext, 12);
}
gen_helper_cf_movec_to(cpu_env, tcg_const_i32(ext & 0xfff), reg);
-gen_lookup_tb(s);
+gen_exit_tb(s);
}
DISAS_INSN(m68k_movec)
@@ -4705,7 +4705,7 @@ DISAS_INSN(m68k_movec)
} else {
gen_helper_m68k_movec_from(reg, cpu_env, tcg_const_i32(ext & 0xfff));
}
-gen_lookup_tb(s);
+gen_exit_tb(s);
}
DISAS_INSN(intouch)
@@ -5749,7 +5749,7 @@ DISAS_INSN(to_macsr)
TCGv val;
SRC_EA(env, val, OS_LONG, 0, NULL);
gen_helper_set_macsr(cpu_env, val);
-gen_lookup_tb(s);
+gen_exit_tb(s);
}
DISAS_INSN(to_mask)
@@ -6144,9 +6144,9 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock
*tb)
tcg_gen_lookup_and_goto_ptr();
break;
default:
-case DISAS_UPDATE:
-update_cc_op(dc);
-/* indicate that the hash table must be used to find the next TB */
+case DISAS_EXIT:
+/* We updated CC_OP and PC in gen_exit_tb, but also modified
+ other state that may require returning to the main loop. */
tcg_gen_exit_tb(NULL, 0);
break;
case DISAS_NORETURN:
--
2.14.4
[Qemu-devel] [PULL 6/9] target/m68k: Convert to DisasContextBase
From: Richard Henderson
Removed ctx->insn_pc in favour of ctx->base.pc_next.
Yes, it is annoying, but didn't want to waste its 4 bytes.
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-7-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 137 +++-
1 file changed, 67 insertions(+), 70 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 0cbd715550..12a74b0b59 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -111,14 +111,11 @@ void m68k_tcg_init(void)
/* internal defines */
typedef struct DisasContext {
+DisasContextBase base;
CPUM68KState *env;
-target_ulong insn_pc; /* Start of the current instruction. */
target_ulong pc;
-int is_jmp;
CCOp cc_op; /* Current CC operation */
int cc_op_synced;
-struct TranslationBlock *tb;
-int singlestep_enabled;
TCGv_i64 mactmp;
int done_mac;
int writeback_mask;
@@ -203,10 +200,10 @@ static void do_writebacks(DisasContext *s)
#if defined(CONFIG_USER_ONLY)
#define IS_USER(s) 1
#else
-#define IS_USER(s) (!(s->tb->flags & TB_FLAGS_MSR_S))
-#define SFC_INDEX(s) ((s->tb->flags & TB_FLAGS_SFC_S) ? \
+#define IS_USER(s) (!(s->base.tb->flags & TB_FLAGS_MSR_S))
+#define SFC_INDEX(s) ((s->base.tb->flags & TB_FLAGS_SFC_S) ? \
MMU_KERNEL_IDX : MMU_USER_IDX)
-#define DFC_INDEX(s) ((s->tb->flags & TB_FLAGS_DFC_S) ? \
+#define DFC_INDEX(s) ((s->base.tb->flags & TB_FLAGS_DFC_S) ? \
MMU_KERNEL_IDX : MMU_USER_IDX)
#endif
@@ -278,7 +275,7 @@ static void gen_jmp_im(DisasContext *s, uint32_t dest)
{
update_cc_op(s);
tcg_gen_movi_i32(QREG_PC, dest);
-s->is_jmp = DISAS_JUMP;
+s->base.is_jmp = DISAS_JUMP;
}
/* Generate a jump to the address in qreg DEST. */
@@ -286,7 +283,7 @@ static void gen_jmp(DisasContext *s, TCGv dest)
{
update_cc_op(s);
tcg_gen_mov_i32(QREG_PC, dest);
-s->is_jmp = DISAS_JUMP;
+s->base.is_jmp = DISAS_JUMP;
}
static void gen_exception(DisasContext *s, uint32_t dest, int nr)
@@ -300,12 +297,12 @@ static void gen_exception(DisasContext *s, uint32_t dest,
int nr)
gen_helper_raise_exception(cpu_env, tmp);
tcg_temp_free_i32(tmp);
-s->is_jmp = DISAS_NORETURN;
+s->base.is_jmp = DISAS_NORETURN;
}
static inline void gen_addr_fault(DisasContext *s)
{
-gen_exception(s, s->insn_pc, EXCP_ADDRESS);
+gen_exception(s, s->base.pc_next, EXCP_ADDRESS);
}
/* Generate a load from the specified address. Narrow values are
@@ -1003,7 +1000,7 @@ static void gen_load_fp(DisasContext *s, int opsize, TCGv
addr, TCGv_ptr fp,
break;
case OS_EXTENDED:
if (m68k_feature(s->env, M68K_FEATURE_CF_FPU)) {
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(s, s->base.pc_next, EXCP_FP_UNIMP);
break;
}
tcg_gen_qemu_ld32u(tmp, addr, index);
@@ -1017,7 +1014,7 @@ static void gen_load_fp(DisasContext *s, int opsize, TCGv
addr, TCGv_ptr fp,
/* unimplemented data type on 68040/ColdFire
* FIXME if needed for another FPU
*/
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(s, s->base.pc_next, EXCP_FP_UNIMP);
break;
default:
g_assert_not_reached();
@@ -1057,7 +1054,7 @@ static void gen_store_fp(DisasContext *s, int opsize,
TCGv addr, TCGv_ptr fp,
break;
case OS_EXTENDED:
if (m68k_feature(s->env, M68K_FEATURE_CF_FPU)) {
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(s, s->base.pc_next, EXCP_FP_UNIMP);
break;
}
tcg_gen_ld16u_i32(tmp, fp, offsetof(FPReg, l.upper));
@@ -1071,7 +1068,7 @@ static void gen_store_fp(DisasContext *s, int opsize,
TCGv addr, TCGv_ptr fp,
/* unimplemented data type on 68040/ColdFire
* FIXME if needed for another FPU
*/
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(s, s->base.pc_next, EXCP_FP_UNIMP);
break;
default:
g_assert_not_reached();
@@ -1203,7 +1200,7 @@ static int gen_ea_mode_fp(CPUM68KState *env, DisasContext
*s, int mode,
break;
case OS_EXTENDED:
if (m68k_feature(s->env, M68K_FEATURE_CF_FPU)) {
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(s, s->base.pc_next, EXCP_FP_UNIMP);
break;
}
tmp = tcg_const_i32(read_im32(env, s) >> 16);
@@ -1217,7 +1214,7 @@ static int gen_ea_mode_fp(CPUM68KState *env, DisasContext
*s, int mode,
/* unimplemented data type on 68040/ColdFire
* FIXME if needed for another FPU
*/
-gen_exception(s, s->insn_pc, EXCP_FP_UNIMP);
+gen_exception(
[Qemu-devel] [PULL 8/9] target/m68k: Improve ending TB at page boundaries
From: Richard Henderson
Rather than limit total TB size to PAGE-32 bytes, end the TB when
near the end of a page. This should provide proper semantics of
SIGSEGV when executing near the end of a page.
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-9-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 22 +++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 62a96bedcd..ff3493d8ab 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6105,9 +6105,25 @@ static void m68k_tr_translate_insn(DisasContextBase
*dcbase, CPUState *cpu)
disas_m68k_insn(cpu->env_ptr, dc);
dc->base.pc_next = dc->pc;
-if (dc->base.is_jmp == DISAS_NEXT
-&& dc->pc - dc->base.pc_first >= TARGET_PAGE_SIZE - 32) {
-dc->base.is_jmp = DISAS_TOO_MANY;
+if (dc->base.is_jmp == DISAS_NEXT) {
+/* Stop translation when the next insn might touch a new page.
+ * This ensures that prefetch aborts at the right place.
+ *
+ * We cannot determine the size of the next insn without
+ * completely decoding it. However, the maximum insn size
+ * is 32 bytes, so end if we do not have that much remaining.
+ * This may produce several small TBs at the end of each page,
+ * but they will all be linked with goto_tb.
+ *
+ * ??? ColdFire maximum is 4 bytes; MC68000's maximum is also
+ * smaller than MC68020's.
+ */
+target_ulong start_page_offset
+= dc->pc - (dc->base.pc_first & TARGET_PAGE_MASK);
+
+if (start_page_offset >= TARGET_PAGE_SIZE - 32) {
+dc->base.is_jmp = DISAS_TOO_MANY;
+}
}
}
--
2.14.4
[Qemu-devel] [PULL 3/9] target/m68k: Remove DISAS_JUMP_NEXT as unused
From: Richard Henderson Signed-off-by: Richard Henderson Message-Id: <20180512050250.12774-4-richard.hender...@linaro.org> Signed-off-by: Laurent Vivier --- target/m68k/translate.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target/m68k/translate.c b/target/m68k/translate.c index bfa30cde0a..6238d9edc9 100644 --- a/target/m68k/translate.c +++ b/target/m68k/translate.c @@ -199,7 +199,6 @@ static void do_writebacks(DisasContext *s) /* is_jmp field values */ #define DISAS_JUMP DISAS_TARGET_0 /* only pc was modified dynamically */ #define DISAS_UPDATEDISAS_TARGET_1 /* cpu state was modified dynamically */ -#define DISAS_JUMP_NEXT DISAS_TARGET_3 #if defined(CONFIG_USER_ONLY) #define IS_USER(s) 1 -- 2.14.4
[Qemu-devel] [PULL 9/9] target/m68k: Merge disas_m68k_insn into m68k_tr_translate_insn
From: Richard Henderson
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-10-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index ff3493d8ab..ae3651b867 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6049,16 +6049,6 @@ void register_m68k_insns (CPUM68KState *env)
#undef INSN
}
-/* ??? Some of this implementation is not exception safe. We should always
- write back the result to memory before setting the condition codes. */
-static void disas_m68k_insn(CPUM68KState * env, DisasContext *s)
-{
-uint16_t insn = read_im16(env, s);
-opcode_table[insn](env, s, insn);
-do_writebacks(s);
-do_release(s);
-}
-
static void m68k_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -6101,8 +6091,13 @@ static bool m68k_tr_breakpoint_check(DisasContextBase
*dcbase, CPUState *cpu,
static void m68k_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
{
DisasContext *dc = container_of(dcbase, DisasContext, base);
+CPUM68KState *env = cpu->env_ptr;
+uint16_t insn = read_im16(env, dc);
+
+opcode_table[insn](env, dc, insn);
+do_writebacks(dc);
+do_release(dc);
-disas_m68k_insn(cpu->env_ptr, dc);
dc->base.pc_next = dc->pc;
if (dc->base.is_jmp == DISAS_NEXT) {
--
2.14.4
[Qemu-devel] [PULL 7/9] target/m68k: Convert to TranslatorOps
From: Richard Henderson
Signed-off-by: Richard Henderson
Message-Id: <20180512050250.12774-8-richard.hender...@linaro.org>
Signed-off-by: Laurent Vivier
---
target/m68k/translate.c | 180 +++-
1 file changed, 88 insertions(+), 92 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 12a74b0b59..62a96bedcd 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6059,113 +6059,109 @@ static void disas_m68k_insn(CPUM68KState * env,
DisasContext *s)
do_release(s);
}
-/* generate intermediate code for basic block 'tb'. */
-void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
+static void m68k_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu)
{
-CPUM68KState *env = cs->env_ptr;
-DisasContext dc1, *dc = &dc1;
-target_ulong pc_start;
-int pc_offset;
-int num_insns;
-int max_insns;
-
-/* generate intermediate code */
-pc_start = tb->pc;
-
-dc->base.tb = tb;
+DisasContext *dc = container_of(dcbase, DisasContext, base);
+CPUM68KState *env = cpu->env_ptr;
dc->env = env;
-dc->base.is_jmp = DISAS_NEXT;
-dc->pc = pc_start;
+dc->pc = dc->base.pc_first;
dc->cc_op = CC_OP_DYNAMIC;
dc->cc_op_synced = 1;
-dc->base.singlestep_enabled = cs->singlestep_enabled;
dc->done_mac = 0;
dc->writeback_mask = 0;
-num_insns = 0;
-max_insns = tb_cflags(tb) & CF_COUNT_MASK;
-if (max_insns == 0) {
-max_insns = CF_COUNT_MASK;
-}
-if (max_insns > TCG_MAX_INSNS) {
-max_insns = TCG_MAX_INSNS;
-}
-
init_release_array(dc);
+}
-gen_tb_start(tb);
-do {
-pc_offset = dc->pc - pc_start;
-tcg_gen_insn_start(dc->pc, dc->cc_op);
-num_insns++;
-
-if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
-gen_exception(dc, dc->pc, EXCP_DEBUG);
-/* The address covered by the breakpoint must be included in
- [tb->pc, tb->pc + tb->size) in order to for it to be
- properly cleared -- thus we increment the PC here so that
- the logic setting tb->size below does the right thing. */
-dc->pc += 2;
-break;
-}
+static void m68k_tr_tb_start(DisasContextBase *dcbase, CPUState *cpu)
+{
+}
-if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
-gen_io_start();
-}
+static void m68k_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
+{
+DisasContext *dc = container_of(dcbase, DisasContext, base);
+tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
+}
-dc->base.pc_next = dc->pc;
- disas_m68k_insn(env, dc);
-} while (!dc->base.is_jmp && !tcg_op_buf_full() &&
- !cs->singlestep_enabled &&
- !singlestep &&
- (pc_offset) < (TARGET_PAGE_SIZE - 32) &&
- num_insns < max_insns);
-
-if (tb_cflags(tb) & CF_LAST_IO)
-gen_io_end();
-if (unlikely(cs->singlestep_enabled)) {
-/* Make sure the pc is updated, and raise a debug exception. */
-if (!dc->base.is_jmp) {
-update_cc_op(dc);
-tcg_gen_movi_i32(QREG_PC, dc->pc);
-}
+static bool m68k_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
+ const CPUBreakpoint *bp)
+{
+DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+gen_exception(dc, dc->base.pc_next, EXCP_DEBUG);
+/* The address covered by the breakpoint must be included in
+ [tb->pc, tb->pc + tb->size) in order to for it to be
+ properly cleared -- thus we increment the PC here so that
+ the logic setting tb->size below does the right thing. */
+dc->base.pc_next += 2;
+
+return true;
+}
+
+static void m68k_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+{
+DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+disas_m68k_insn(cpu->env_ptr, dc);
+dc->base.pc_next = dc->pc;
+
+if (dc->base.is_jmp == DISAS_NEXT
+&& dc->pc - dc->base.pc_first >= TARGET_PAGE_SIZE - 32) {
+dc->base.is_jmp = DISAS_TOO_MANY;
+}
+}
+
+static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
+{
+DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+if (dc->base.is_jmp == DISAS_NORETURN) {
+return;
+}
+if (dc->base.singlestep_enabled) {
gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
-} else {
-switch (dc->base.is_jmp) {
-case DISAS_NEXT:
-update_cc_op(dc);
-gen_jmp_tb(dc, 0, dc->pc);
-break;
-case DISAS_JUMP:
-/* We updated CC_OP and PC in gen_jmp/gen_jmp_im. */
-tcg_gen_lookup_and_goto_ptr();
-break;
-default:
-case DISAS_EXIT:
-/* We updated CC_OP and PC in gen_exit_tb, but also modified
-
[Qemu-devel] [PATCH] DO NOT APPLY - demo broken unnest-vars with clashing .o filenames
In a sub-directory source files may be conditionally built using a Makefile.objs line such as crypto-obj-$(CONFIG_CTHULHU) = foo.o It is also possible to add custom linker flags for when this foo.o is later linked into a binary foo.o-libs = -lcthulhu When the unqualfied 'foo.o' filename matches the name of an object in another directory, however, it appears the linker flags leak out and affect the linking of the foo.o from the other directory. This patch demos the bug by adding a crypto/piix.o file. It should have no effect because CONFIG_CTHULHU is not defined, however, the existence of hw/pci-host/piix.o and hw/ide/piix.o in fact causes '-lcthulhu' to be added to the link line twice, causing build failure: $ make LINKx86_64-softmmu/qemu-system-x86_64 /usr/bin/ld: cannot find -lcthuhlu /usr/bin/ld: cannot find -lcthuhlu collect2: error: ld returned 1 exit status make[1]: *** [Makefile:193: qemu-system-x86_64] Error 1 make: *** [Makefile:482: subdir-x86_64-softmmu] Error 2 Signed-off-by: Daniel P. Berrangé --- crypto/Makefile.objs | 4 crypto/piix.c| 2 ++ 2 files changed, 6 insertions(+) create mode 100644 crypto/piix.c diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs index 2b99e08062..73b4548f12 100644 --- a/crypto/Makefile.objs +++ b/crypto/Makefile.objs @@ -38,3 +38,7 @@ crypto-obj-y += block-luks.o crypto-aes-obj-y = aes.o stub-obj-y += pbkdf-stub.o + + +crypto-obj-$(CONFIG_CTHUHLU) += piix.o +piix.o-libs = -lcthuhlu diff --git a/crypto/piix.c b/crypto/piix.c new file mode 100644 index 00..367683f7bb --- /dev/null +++ b/crypto/piix.c @@ -0,0 +1,2 @@ + +random garbage as this ".c" file will never be built -- 2.17.0
Re: [Qemu-devel] [PATCH v13 02/12] migration: Create multifd packet
* Juan Quintela (quint...@redhat.com) wrote:
> We still don't put anything there.
>
> Signed-off-by: Juan Quintela
Reviewed-by: Dr. David Alan Gilbert
>
> --
> fix magic (dave)
> check offset/ramblock (dave)
> ---
> migration/ram.c | 145 +++-
> 1 file changed, 144 insertions(+), 1 deletion(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 23cc5625eb..54350db8b0 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -510,6 +510,17 @@ typedef struct {
> uint8_t id;
> } __attribute__((packed)) MultiFDInit_t;
>
> +typedef struct {
> +uint32_t magic;
> +uint32_t version;
> +uint32_t flags;
> +uint32_t size;
> +uint32_t used;
> +uint32_t seq;
> +char ramblock[256];
> +uint64_t offset[];
> +} __attribute__((packed)) MultiFDPacket_t;
> +
> typedef struct {
> /* number of used pages */
> uint32_t used;
> @@ -544,6 +555,14 @@ typedef struct {
> bool quit;
> /* array of pages to sent */
> MultiFDPages_t *pages;
> +/* packet allocated len */
> +uint32_t packet_len;
> +/* pointer to the packet */
> +MultiFDPacket_t *packet;
> +/* multifd flags for each packet */
> +uint32_t flags;
> +/* global number of generated multifd packets */
> +uint32_t seq;
> } MultiFDSendParams;
>
> typedef struct {
> @@ -566,6 +585,14 @@ typedef struct {
> bool quit;
> /* array of pages to receive */
> MultiFDPages_t *pages;
> +/* packet allocated len */
> +uint32_t packet_len;
> +/* pointer to the packet */
> +MultiFDPacket_t *packet;
> +/* multifd flags for each packet */
> +uint32_t flags;
> +/* global number of generated multifd packets */
> +uint32_t seq;
> } MultiFDRecvParams;
>
> static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
> @@ -654,6 +681,99 @@ static void multifd_pages_clear(MultiFDPages_t *pages)
> g_free(pages);
> }
>
> +static void multifd_send_fill_packet(MultiFDSendParams *p)
> +{
> +MultiFDPacket_t *packet = p->packet;
> +int i;
> +
> +packet->magic = cpu_to_be32(MULTIFD_MAGIC);
> +packet->version = cpu_to_be32(MULTIFD_VERSION);
> +packet->flags = cpu_to_be32(p->flags);
> +packet->size = cpu_to_be32(migrate_multifd_page_count());
> +packet->used = cpu_to_be32(p->pages->used);
> +packet->seq = cpu_to_be32(p->seq);
> +
> +if (p->pages->block) {
> +strncpy(packet->ramblock, p->pages->block->idstr, 256);
> +}
> +
> +for (i = 0; i < p->pages->used; i++) {
> +packet->offset[i] = cpu_to_be64(p->pages->offset[i]);
> +}
> +}
> +
> +static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
> +{
> +MultiFDPacket_t *packet = p->packet;
> +RAMBlock *block;
> +int i;
> +
> +/* ToDo: We can't use it until we haven't received a message */
> +return 0;
> +
> +be32_to_cpus(&packet->magic);
> +if (packet->magic != MULTIFD_MAGIC) {
> +error_setg(errp, "multifd: received packet "
> + "magic %x and expected magic %x",
> + packet->magic, MULTIFD_MAGIC);
> +return -1;
> +}
> +
> +be32_to_cpus(&packet->version);
> +if (packet->version != MULTIFD_VERSION) {
> +error_setg(errp, "multifd: received packet "
> + "version %d and expected version %d",
> + packet->version, MULTIFD_VERSION);
> +return -1;
> +}
> +
> +p->flags = be32_to_cpu(packet->flags);
> +
> +be32_to_cpus(&packet->size);
> +if (packet->size > migrate_multifd_page_count()) {
> +error_setg(errp, "multifd: received packet "
> + "with size %d and expected maximum size %d",
> + packet->size, migrate_multifd_page_count()) ;
> +return -1;
> +}
> +
> +p->pages->used = be32_to_cpu(packet->used);
> +if (p->pages->used > packet->size) {
> +error_setg(errp, "multifd: received packet "
> + "with size %d and expected maximum size %d",
> + p->pages->used, packet->size) ;
> +return -1;
> +}
> +
> +p->seq = be32_to_cpu(packet->seq);
> +
> +if (p->pages->used) {
> +/* make sure that ramblock is 0 terminated */
> +packet->ramblock[255] = 0;
> +block = qemu_ram_block_by_name(packet->ramblock);
> +if (!block) {
> +error_setg(errp, "multifd: unknown ram block %s",
> + packet->ramblock);
> +return -1;
> +}
> +}
> +
> +for (i = 0; i < p->pages->used; i++) {
> +ram_addr_t offset = be64_to_cpu(packet->offset[i]);
> +
> +if (offset > (block->used_length - TARGET_PAGE_SIZE)) {
> +error_setg(errp, "multifd: offset too long %" PRId64
> + " (max %" PRId64 ")",
> + offset, block->max_length);
> +return -1;
> +}
> +
Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 2/2] vfio-ccw: support for halt/clear subchannel
On Fri, 8 Jun 2018 22:40:39 +0200 Halil Pasic wrote: > Your welcome. The discussion is kind of taking place all over the > place. I'm actively trying to find the best place to answer, and avoid > overtalking topics -- but it does not seem to work. Please bear with me. To help with this discussion (and with vfio-ccw design and development in general), I've created https://wiki.qemu.org/ToDo/Channel_I/O_Passthrough to collect some things. Let me know if any of you folks still needs a wiki account.
Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg
On 11.06.2018 11:08, Viktor VM Mihajlovski wrote: > On 07.06.2018 14:22, Thomas Huth wrote: >> This patch series adds pxelinux.cfg-style network booting to the s390-ccw >> firmware. The core pxelinux.cfg loading and parsing logic has recently >> been merged to SLOF, so these patches now just have to make sure to call >> the right functions to get the config file loaded and parsed. Once this is >> done, the kernel and initrd are loaded separately, and are then glued >> together in RAM. >> >> v2: >> - Update SLOF submodule now that the git mirror is in sync again >> - Last parameter to tftp_get_error_info() must not be NULL >> - Check CC when calling STSI, and use a #define for the UUID offset >> - Only support files with the magic "# pxelinux" string comment when >>trying to guess the contents of a file that has been downloaded via >>the "bootfile" DHCP parameter. This is just for developers' convenience, >>the official way to specify pxelinux.cfg files is to use the DHCP >>options 209 and 210 instead. >> >> Thomas Huth (4): >> roms: Update SLOF submodule to current status >> pc-bios/s390-ccw/net: Update code for the latest changes in SLOF >> pc-bios/s390-ccw/net: Add support for pxelinux-style config files >> pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the >> UUID >> >> pc-bios/s390-ccw/netboot.mak | 9 +- >> pc-bios/s390-ccw/netmain.c | 226 >> +-- >> roms/SLOF| 2 +- >> 3 files changed, 162 insertions(+), 75 deletions(-) >> > I tested the series both with a self-created pxelinux.0 blob (to verify > backward compatibility) and without an existing pxelinux.0 file (to > force the standard pxelinux pattern). Both worked as expected, although > the built-in search took significantly longer (timeout?). > > Tested-by: Viktor Mihajlovski Thanks a lot for the testing! Hmm, I've got no real clue why there should be a big difference in the amount of time here ... is there maybe a lot of unrelated broadcast network traffic from other hosts going on in that network where you've tested it? It could be that the virtio-net driver of the s390-ccw bios can not deal with that situation very well yet. You could try to increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c to see whether it makes a difference. Or if you've got some spare time, could you maybe run Wireshark on the server side to have a look at the time-stamps of the related packets, and to see whether there are duplicated TFTP read requests? This could indicate that the firmware missed a packet and thus ran into a timeout. Thomas PS: After I posted v1 of the patch set, you reported on IRC that you saw a problem with a corrupted initrd download or something similar. Is that problem now fixed for you?
Re: [Qemu-devel] [PATCH] CODING_STYLE: Define our preferred form for multiline comments
I'm late to the party, but anyway... Am 07.06.2018 um 14:02 hat Markus Armbruster geschrieben: > Peter Maydell writes: > > > On 5 June 2018 at 08:46, Cornelia Huck wrote: > >> On Tue, 5 Jun 2018 06:33:22 +0200 > >> Thomas Huth wrote: > >>> On 05.06.2018 03:17, Alex Williamson wrote: > >>> > On Mon, 4 Jun 2018 17:21:40 +0100 > >>> > Peter Maydell wrote: > >>> >> +Multiline comments blocks should have a row of stars on the left > >>> >> +and the terminating */ on its own line: > >>> >> +/* like > >>> >> + * this > >>> >> + */ > > Uh, winging just one end of the comment offends my eyes. +1, this is the ugliest style of all. > >>> >> +Putting the initial /* on its own line is accepted, but not required. > >>> > > >>> > Could we say "at maintainer discretion", or is that always implied? The > >>> > asymmetry of the proposed standard is not my favorite and a mostly > >>> > blank line before and after further supports standing out from > >>> > surrounding code. > >>> I also don't like the asymmetry. I'd prefer more dense comments, though: > >>> > >>> /* like > >>>* this */ > > > > Wow, I think that looks terrible :-) > > Even more terrible, you wanted to say ;) I actually prefer this one for short (2 or 3 lines) not too important comments in order to save some screen space. For longer or important comments, it's kernel-style. > >>> Anyway, could we either use that dense format or the kernel-style > >>> multi-lines-comment format, please? Mixing it asymmetrically is just ugly. > >> > >> I'd vote for the kernel style, then. > > > > I don't particularly object to the kernel style (though it's not > > how I personally default to writing comments). I just didn't want > > to rule a huge chunk of our existing comments as out-of-standard > > for what I see as a relatively minor divergence in form -- > > we do have a lot of no-leading-separate-/* comments. I can live > > with mandating kernel-style if it means we can rule out GNU-form > > and other weirdnesses though :-) > > Let's mandate kernel-style for new code. I could live with giving > maintainers license to tolerate certain other styles. The fewer, the > better, though. Kernel-style + give maintainers license to tolerate more compact forms (mostly for short comments) works for me. Kevin
Re: [Qemu-devel] [PATCH] DO NOT APPLY - demo broken unnest-vars with clashing .o filenames
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 2018060327.21218-1-berra...@redhat.com Subject: [Qemu-devel] [PATCH] DO NOT APPLY - demo broken unnest-vars with clashing .o filenames === 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-mingw@fedora === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update]patchew/20180523111817.1463-1-quint...@redhat.com -> patchew/20180523111817.1463-1-quint...@redhat.com Switched to a new branch 'test' 5a24bbf68d DO NOT APPLY - demo broken unnest-vars with clashing .o filenames === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-li1g4td4/src/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-li1g4td4/src' GEN /var/tmp/patchew-tester-tmp-li1g4td4/src/docker-src.2018-06-11-07.21.33.7990/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-li1g4td4/src/docker-src.2018-06-11-07.21.33.7990/qemu.tar.vroot'... done. Checking out files: 50% (3157/6219) Checking out files: 51% (3172/6219) Checking out files: 52% (3234/6219) Checking out files: 53% (3297/6219) Checking out files: 54% (3359/6219) Checking out files: 55% (3421/6219) Checking out files: 56% (3483/6219) Checking out files: 57% (3545/6219) Checking out files: 58% (3608/6219) Checking out files: 59% (3670/6219) Checking out files: 60% (3732/6219) Checking out files: 61% (3794/6219) Checking out files: 62% (3856/6219) Checking out files: 63% (3918/6219) Checking out files: 64% (3981/6219) Checking out files: 65% (4043/6219) Checking out files: 66% (4105/6219) Checking out files: 67% (4167/6219) Checking out files: 68% (4229/6219) Checking out files: 69% (4292/6219) Checking out files: 70% (4354/6219) Checking out files: 71% (4416/6219) Checking out files: 72% (4478/6219) Checking out files: 73% (4540/6219) Checking out files: 74% (4603/6219) Checking out files: 75% (4665/6219) Checking out files: 76% (4727/6219) Checking out files: 77% (4789/6219) Checking out files: 78% (4851/6219) Checking out files: 79% (4914/6219) Checking out files: 80% (4976/6219) Checking out files: 81% (5038/6219) Checking out files: 82% (5100/6219) Checking out files: 83% (5162/6219) Checking out files: 84% (5224/6219) Checking out files: 85% (5287/6219) Checking out files: 86% (5349/6219) Checking out files: 87% (5411/6219) Checking out files: 88% (5473/6219) Checking out files: 89% (5535/6219) Checking out files: 90% (5598/6219) Checking out files: 91% (5660/6219) Checking out files: 92% (5722/6219) Checking out files: 93% (5784/6219) Checking out files: 94% (5846/6219) Checking out files: 95% (5909/6219) Checking out files: 96% (5971/6219) Checking out files: 97% (6033/6219) Checking out files: 98% (6095/6219) Checking out files: 99% (6157/6219) Checking out files: 100% (6219/6219) Checking out files: 100% (6219/6219), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-li1g4td4/src/docker-src.2018-06-11-07.21.33.7990/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-li1g4td4/src/docker-src.2018-06-11-07.21.33.7990/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-mingw in qemu:fedora Packages installed: SDL2-devel-2.0.8-5.fc28.x86_64 bc-1.07.1-5.fc28.x86_64 bison-3.0.4-9.fc28.x86_64 bluez-libs-devel-5.49-3.fc28.x86_64 brlapi-devel-0.6.7-12.fc28.x86_64 bzip2-1.0.6-26.fc28.x86_64 bzip2-devel-1.0.6-26.fc28.x86_64 ccache-3.4.2-2.fc28.x86_64 clang-6.0.0-5.fc28.x86_64 device-mapper-multipath-devel-0.7.4-2.git07e7bd5.fc28.x86_64 findutils-4.6.0-19.fc28.x86_64 flex-2.6.1-7.fc28.x86_64 gcc-8.1.1-1.fc28.x86_64 gcc-c++-8.1.1-1.fc28.x86_64 gettext-0.19.8.1-14.fc28.x86_64 git-2.17.1-2.fc28.x86_64 glib2-devel-2.56.1-3.fc28.x86_64 glusterfs-api-devel-4.0.2-1.fc28.x86_64 gnutls-devel-3.6.2-1.fc28.x86_64 gtk3-devel-3.22.30-1.fc28.x86_64 hostname-3.20-3.fc28.x86_64 libaio-devel-0.3.110-11.fc28.x86_64 libasan-8.1.1-1.fc28.x86_64 libattr-devel-2.4.47-23.fc28.x86_64 libcap-devel
Re: [Qemu-devel] [PATCH v3 4/7] qmp: enable query-[chardev|version|name|uuid|iothreads|memdev] commands in preconfig state
"Dr. David Alan Gilbert (git)" writes:
> From: Igor Mammedov
>
> subj commands, are informational and do not depend on machine being
> initialized. Make them available early in preconfig runstate to make
> the later a little bit more useful.
>
> Signed-off-by: Igor Mammedov
Excessively long subject line. Suggested fix:
qmp: Enable a few query- commands in preconfig state
Commands query-chardev, query-version, query-name, query-uuid,
query-iothreads, query-memdev are informational and do not depend on
the machine being initialized. Make them available in preconfig
runstate to make the latter a little bit more useful.
> ---
> qapi/char.json | 3 ++-
> qapi/misc.json | 12 +++-
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/qapi/char.json b/qapi/char.json
> index ae19dcd1ed..60f31d83fc 100644
> --- a/qapi/char.json
> +++ b/qapi/char.json
> @@ -62,7 +62,8 @@
> #}
> #
> ##
> -{ 'command': 'query-chardev', 'returns': ['ChardevInfo'] }
> +{ 'command': 'query-chardev', 'returns': ['ChardevInfo'],
> + 'allow-preconfig': true }
Okay because
if (qemu_opts_foreach(qemu_find_opts("chardev"),
chardev_init_func, NULL, NULL)) {
exit(1);
}
runs before -preconfig's main_loop().
>
> ##
> # @ChardevBackendInfo:
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f83a63a0ab..1be1728c0e 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -117,7 +117,8 @@
> #}
> #
> ##
> -{ 'command': 'query-version', 'returns': 'VersionInfo' }
> +{ 'command': 'query-version', 'returns': 'VersionInfo',
> + 'allow-preconfig': true }
Returns data fixed at compile time. Okay.
>
> ##
> # @CommandInfo:
> @@ -241,7 +242,7 @@
> # <- { "return": { "name": "qemu-name" } }
> #
> ##
> -{ 'command': 'query-name', 'returns': 'NameInfo' }
> +{ 'command': 'query-name', 'returns': 'NameInfo', 'allow-preconfig': true }
Okay because parse_name() runs before -preconfig's main_loop().
> ##
> # @KvmInfo:
> @@ -301,7 +302,7 @@
> # <- { "return": { "UUID": "550e8400-e29b-41d4-a716-44665544" } }
> #
> ##
> -{ 'command': 'query-uuid', 'returns': 'UuidInfo' }
> +{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
Okay because qemu_uuid(optarg, &qemu_uuid) runs before -preconfig's
main_loop().
>
> ##
> # @EventInfo:
> @@ -710,7 +711,8 @@
> #}
> #
> ##
> -{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'] }
> +{ 'command': 'query-iothreads', 'returns': ['IOThreadInfo'],
> + 'allow-preconfig': true }
Feeling lazy... why is this one okay?
>
> ##
> # @BalloonInfo:
> @@ -2902,7 +2904,7 @@
> #}
> #
> ##
> -{ 'command': 'query-memdev', 'returns': ['Memdev'] }
> +{ 'command': 'query-memdev', 'returns': ['Memdev'], 'allow-preconfig': true }
>
> ##
> # @PCDIMMDeviceInfo:
And this one?
Re: [Qemu-devel] [PULL 00/30] Ide patches
On 8 June 2018 at 18:47, John Snow wrote: > The following changes since commit 0d2fa03dae4fbe185a082f361342b1e30aed4582: > > Merge remote-tracking branch > 'remotes/pmaydell/tags/pull-target-arm-20180608' into staging (2018-06-08 > 16:26:51 +0100) > > are available in the Git repository at: > > https://github.com/jnsnow/qemu.git tags/ide-pull-request > > for you to fetch changes up to c173723f247c69974a83af1395020d0f01a0d334: > > ide: introduce ide_transfer_start_norecurse (2018-06-08 13:36:31 -0400) > > > > AHCI Pull request > > Applied, thanks. -- PMM
Re: [Qemu-devel] [PATCH v13 07/12] migration: Synchronize multifd threads with main thread
* Juan Quintela (quint...@redhat.com) wrote:
> We synchronize all threads each RAM_SAVE_FLAG_EOS. Bitmap
> synchronizations don't happen inside a ram section, so we are safe
> about two channels trying to overwrite the same memory.
>
> Signed-off-by: Juan Quintela
> ---
> migration/ram.c| 117 +
> migration/trace-events | 6 +++
> 2 files changed, 112 insertions(+), 11 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index c9a9bd79f3..3e99d48123 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -503,6 +503,8 @@ exit:
> #define MULTIFD_MAGIC 0x11223344U
> #define MULTIFD_VERSION 1
>
> +#define MULTIFD_FLAG_SYNC (1 << 0)
> +
> typedef struct {
> uint32_t magic;
> uint32_t version;
> @@ -570,6 +572,8 @@ typedef struct {
> uint32_t num_packets;
> /* pages sent through this channel */
> uint32_t num_pages;
> +/* syncs main thread and channels */
> +QemuSemaphore sem_sync;
> } MultiFDSendParams;
>
> typedef struct {
> @@ -607,6 +611,8 @@ typedef struct {
> uint32_t num_packets;
> /* pages sent through this channel */
> uint32_t num_pages;
> +/* syncs main thread and channels */
> +QemuSemaphore sem_sync;
> } MultiFDRecvParams;
>
> static int multifd_send_initial_packet(MultiFDSendParams *p, Error **errp)
> @@ -794,6 +800,10 @@ struct {
> int count;
> /* array of pages to sent */
> MultiFDPages_t *pages;
> +/* syncs main thread and channels */
> +QemuSemaphore sem_sync;
> +/* global number of generated multifd packets */
> +uint32_t seq;
> } *multifd_send_state;
>
> static void multifd_send_terminate_threads(Error *err)
> @@ -841,6 +851,7 @@ int multifd_save_cleanup(Error **errp)
> p->c = NULL;
> qemu_mutex_destroy(&p->mutex);
> qemu_sem_destroy(&p->sem);
> +qemu_sem_destroy(&p->sem_sync);
> g_free(p->name);
> p->name = NULL;
> multifd_pages_clear(p->pages);
> @@ -849,6 +860,7 @@ int multifd_save_cleanup(Error **errp)
> g_free(p->packet);
> p->packet = NULL;
> }
> +qemu_sem_destroy(&multifd_send_state->sem_sync);
> g_free(multifd_send_state->params);
> multifd_send_state->params = NULL;
> multifd_pages_clear(multifd_send_state->pages);
> @@ -858,6 +870,33 @@ int multifd_save_cleanup(Error **errp)
> return ret;
> }
>
> +static void multifd_send_sync_main(void)
> +{
> +int i;
> +
> +if (!migrate_use_multifd()) {
> +return;
> +}
> +for (i = 0; i < migrate_multifd_channels(); i++) {
> +MultiFDSendParams *p = &multifd_send_state->params[i];
> +
> +trace_multifd_send_sync_main_signal(p->id);
> +
> +qemu_mutex_lock(&p->mutex);
> +p->flags |= MULTIFD_FLAG_SYNC;
> +p->pending_job++;
> +qemu_mutex_unlock(&p->mutex);
> +qemu_sem_post(&p->sem);
> +}
> +for (i = 0; i < migrate_multifd_channels(); i++) {
> +MultiFDSendParams *p = &multifd_send_state->params[i];
> +
> +trace_multifd_send_sync_main_wait(p->id);
> +qemu_sem_wait(&multifd_send_state->sem_sync);
> +}
> +trace_multifd_send_sync_main(multifd_send_state->seq);
> +}
> +
> static void *multifd_send_thread(void *opaque)
> {
> MultiFDSendParams *p = opaque;
> @@ -894,15 +933,17 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_lock(&p->mutex);
> p->pending_job--;
> qemu_mutex_unlock(&p->mutex);
> -continue;
> +
> +if (flags & MULTIFD_FLAG_SYNC) {
> +qemu_sem_post(&multifd_send_state->sem_sync);
> +}
> } else if (p->quit) {
> qemu_mutex_unlock(&p->mutex);
> break;
> +} else {
> +qemu_mutex_unlock(&p->mutex);
> +/* sometimes there are spurious wakeups */
> }
> -qemu_mutex_unlock(&p->mutex);
> -/* this is impossible */
> -error_setg(&local_err, "multifd_send_thread: Unknown command");
> -break;
> }
>
> out:
> @@ -954,12 +995,14 @@ int multifd_save_setup(void)
> multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
> atomic_set(&multifd_send_state->count, 0);
> multifd_send_state->pages = multifd_pages_init(page_count);
> +qemu_sem_init(&multifd_send_state->sem_sync, 0);
>
> for (i = 0; i < thread_count; i++) {
> MultiFDSendParams *p = &multifd_send_state->params[i];
>
> qemu_mutex_init(&p->mutex);
> qemu_sem_init(&p->sem, 0);
> +qemu_sem_init(&p->sem_sync, 0);
> p->quit = false;
> p->pending_job = 0;
> p->id = i;
> @@ -977,6 +1020,10 @@ struct {
> MultiFDRecvParams *params;
> /* number of created threads */
> int count;
> +/* syncs main thread and channels */
> +QemuSemaphore sem_sync;
> +/* global number of g
Re: [Qemu-devel] [PATCH v3 5/7] hmp: Add info commands for preconfig
"Dr. David Alan Gilbert (git)" writes: > From: "Dr. David Alan Gilbert" > > Allow a bunch of the info commands to be used in preconfig. > > version, chardev, name, uuid,memdev, iothreads > Were enabled in QMP in the previous patch from Igor Yes, these are okay together with PATCH 4. > status, hotpluggable_cpus > Was enabled in the original allow-preconfig series query-status looks okay to me. > history > is HMP specific Yes. > usbhost, qom-tree, numa > Don't have a QMP equivalent HMP commands without a QMP equivalent are okay if their functionality makes no sense in QMP, or is of use only for human users. Example for "makes no sense in QMP": setting the current CPU, because a QMP monitor doesn't have a current CPU. Examples for "is of use only for human users": HMP command "help", the integrated pocket calculator. Now let's review the three commands: * Gerd, why does "info usbhost" have no QMP equivalent? * Eduardo, why does "info numa" have no QMP equivalent? * "info qom-tree" is a recursive variant of qom-list that skips anything but children. This convenience command exists so you don't have to filter and string together output from many qom-list. I think it stands to reason that if providing "info qom-tree" makes sense, then so does qom-list (HMP and QMP). If qom-list, then qom-list-types, qom-list-properties, qom-get, and probably even qom-set (I've always been suspicious of qom-set, but that has nothing to do with preconfig state). It might make sense to split off the whole QOM shebang into a separate patch. > Signed-off-by: Dr. David Alan Gilbert
Re: [Qemu-devel] [PATCH v2 0/4] pc-bios/s390-ccw: Allow network booting via pxelinux.cfg
On 11.06.2018 13:12, Thomas Huth wrote: > On 11.06.2018 11:08, Viktor VM Mihajlovski wrote: >> On 07.06.2018 14:22, Thomas Huth wrote: >>> This patch series adds pxelinux.cfg-style network booting to the s390-ccw >>> firmware. The core pxelinux.cfg loading and parsing logic has recently >>> been merged to SLOF, so these patches now just have to make sure to call >>> the right functions to get the config file loaded and parsed. Once this is >>> done, the kernel and initrd are loaded separately, and are then glued >>> together in RAM. >>> >>> v2: >>> - Update SLOF submodule now that the git mirror is in sync again >>> - Last parameter to tftp_get_error_info() must not be NULL >>> - Check CC when calling STSI, and use a #define for the UUID offset >>> - Only support files with the magic "# pxelinux" string comment when >>>trying to guess the contents of a file that has been downloaded via >>>the "bootfile" DHCP parameter. This is just for developers' convenience, >>>the official way to specify pxelinux.cfg files is to use the DHCP >>>options 209 and 210 instead. >>> >>> Thomas Huth (4): >>> roms: Update SLOF submodule to current status >>> pc-bios/s390-ccw/net: Update code for the latest changes in SLOF >>> pc-bios/s390-ccw/net: Add support for pxelinux-style config files >>> pc-bios/s390-ccw/net: Try to load pxelinux.cfg file accoring to the >>> UUID >>> >>> pc-bios/s390-ccw/netboot.mak | 9 +- >>> pc-bios/s390-ccw/netmain.c | 226 >>> +-- >>> roms/SLOF| 2 +- >>> 3 files changed, 162 insertions(+), 75 deletions(-) >>> >> I tested the series both with a self-created pxelinux.0 blob (to verify >> backward compatibility) and without an existing pxelinux.0 file (to >> force the standard pxelinux pattern). Both worked as expected, although >> the built-in search took significantly longer (timeout?). >> >> Tested-by: Viktor Mihajlovski > > Thanks a lot for the testing! > > Hmm, I've got no real clue why there should be a big difference in the > amount of time here ... is there maybe a lot of unrelated broadcast > network traffic from other hosts going on in that network where you've > tested it? It could be that the virtio-net driver of the s390-ccw bios > can not deal with that situation very well yet. You could try to > increase the "64" in virtio_net_init() in pc-bios/s390-ccw/virtio-net.c > to see whether it makes a difference. Or if you've got some spare time, > could you maybe run Wireshark on the server side to have a look at the > time-stamps of the related packets, and to see whether there are > duplicated TFTP read requests? This could indicate that the firmware > missed a packet and thus ran into a timeout. FWIW: I'm using the isolated libvirt network on an otherwise idle host, so it's unlikely that there's interference. If I find the time I'll have a look at the traffic. > > Thomas > > PS: After I posted v1 of the patch set, you reported on IRC that you saw > a problem with a corrupted initrd download or something similar. Is that > problem now fixed for you? > Yes, it was my bad. I used an older kernel with the following problem https://lkml.org/lkml/2017/3/13/683. -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH v3 6/7] hmp: add exit_preconfig
"Dr. David Alan Gilbert (git)" writes:
> From: "Dr. David Alan Gilbert"
>
> Add the exit_preconfig command to return to normality.
>
> Signed-off-by: Dr. David Alan Gilbert
> Reviewed-by: Peter Xu
> Reviewed-by: Igor Mammedov
> ---
> hmp-commands.hx | 15 +++
> hmp.c | 7 +++
> hmp.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index dc82ed526f..40e2f6ca08 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -55,6 +55,21 @@ STEXI
> @item q or quit
> @findex quit
> Quit the emulator.
> +ETEXI
> +
> +{
> +.name = "exit_preconfig",
> +.args_type = "",
> +.params = "",
> +.help = "exit the preconfig state",
> +.cmd= hmp_exit_preconfig,
> +.flags = "p",
> +},
> +
> +STEXI
> +@item exit_preconfig
> +@findex exit_preconfig
> +Exit the preconfig state
This is awfully terse. Suggest to steal from the QMP documentation in
misc.json:
# This command makes QEMU exit the preconfig state and proceed with
# VM initialization using configuration data provided on the command line
# and via the QMP monitor during the preconfig state. The command is only
# available during the preconfig state (i.e. when the --preconfig command
# line option was in use).
> ETEXI
>
> {
> diff --git a/hmp.c b/hmp.c
> index ef93f4878b..c7be6ed394 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1065,6 +1065,13 @@ void hmp_system_powerdown(Monitor *mon, const QDict
> *qdict)
> qmp_system_powerdown(NULL);
> }
>
> +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict)
> +{
> +Error *err = NULL;
> +qmp_exit_preconfig(&err);
> +hmp_handle_error(mon, &err);
> +}
> +
Blank line between declaration and statements, please.
> void hmp_cpu(Monitor *mon, const QDict *qdict)
> {
> int64_t cpu_index;
> diff --git a/hmp.h b/hmp.h
> index 20f27439d3..33354f1bdd 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -44,6 +44,7 @@ void hmp_quit(Monitor *mon, const QDict *qdict);
> void hmp_stop(Monitor *mon, const QDict *qdict);
> void hmp_system_reset(Monitor *mon, const QDict *qdict);
> void hmp_system_powerdown(Monitor *mon, const QDict *qdict);
> +void hmp_exit_preconfig(Monitor *mon, const QDict *qdict);
> void hmp_cpu(Monitor *mon, const QDict *qdict);
> void hmp_memsave(Monitor *mon, const QDict *qdict);
> void hmp_pmemsave(Monitor *mon, const QDict *qdict);
Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode
Let's also provide "quit", both in HMP and QMP.
Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode
* Markus Armbruster (arm...@redhat.com) wrote: > Let's also provide "quit", both in HMP and QMP. I tried enabling quit, but it fails with an incorrect state transition. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-devel] [PATCH v1 02/11] nvdimm: no need to overwrite get_vmstate_memory_region()
Our parent class (PC_DIMM) provides exactly the same function.
Signed-off-by: David Hildenbrand
---
hw/mem/nvdimm.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 4087aca25e..f974accbdd 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -166,11 +166,6 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm,
const void *buf,
memory_region_set_dirty(mr, backend_offset, size);
}
-static MemoryRegion *nvdimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
-}
-
static void nvdimm_class_init(ObjectClass *oc, void *data)
{
PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
@@ -178,7 +173,6 @@ static void nvdimm_class_init(ObjectClass *oc, void *data)
ddc->realize = nvdimm_realize;
ddc->get_memory_region = nvdimm_get_memory_region;
-ddc->get_vmstate_memory_region = nvdimm_get_vmstate_memory_region;
nvc->read_label_data = nvdimm_read_label_data;
nvc->write_label_data = nvdimm_write_label_data;
--
2.17.1
[Qemu-devel] [PATCH v1 01/11] pc-dimm: remove leftover "struct pc_dimms_capacity"
Not needed anymore, let's drop it.
Signed-off-by: David Hildenbrand
---
hw/mem/pc-dimm.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 12da89d562..62b34a992e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,11 +27,6 @@
#include "sysemu/numa.h"
#include "trace.h"
-typedef struct pc_dimms_capacity {
- uint64_t size;
- Error**errp;
-} pc_dimms_capacity;
-
void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
uint64_t align, Error **errp)
{
--
2.17.1
[Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups
This is another set of cleanups as the result from
[PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on the two series
[PATCH v1 0/2] memory: fix alignment checks/asserts
[PATCH v2 0/6] spapr: machine hotplug handler cleanups
These cleanup are the last step before factoring out the
pre_plug, plug and unplug logic of memory devices completely, to make it
more independent from pc-dimm.
But these patches will already try to make an important point: Assigning
physical addresses of memory devices will not be done in pre_plug but
during plug. Other properties, like the "slot" property, however can be
handled during pre_plug and is done in this patch series, because they
don't realy on the device to be realized.
Igor proposed to move all property checks + assigmnets to the pre_plug
stage. Hovewer for determining a physical address of a memory device, we
need to know the exact size and the alignment of the underlying memory
region.
This region might not be available and initialized before the device has
been realized (e.g. for NVDIMM). So my point is: Accessing derived
"properties" ("memory region" set up based on "memdev" property and maybe
others e.g. for NVDIMM) via device class functions should not be done
before the device has been realized. These functions should not be
called during pre_plug.
Enforcing this, already leads to sime nice cleanup numbers in pc-dimm
code.
David Hildenbrand (11):
pc-dimm: remove leftover "struct pc_dimms_capacity"
nvdimm: no need to overwrite get_vmstate_memory_region()
pc: factor out pc-dimm checks into pc_dimm_pre_plug()
hostmem: drop error variable from host_memory_backend_get_memory()
spapr: move memory hotplug size check into plug code
pc-dimm: don't allow to access "size" before the device was realized
pc-dimm: get_memory_region() can never fail
pc-dimm: get_memory_region() will never return a NULL pointer
pc-dimm: remove pc_dimm_get_vmstate_memory_region()
pc-dimm: introduce and use pc_dimm_memory_pre_plug()
pc-dimm: assign and verify the "slot" property during pre_plug
backends/hostmem.c | 3 +-
hw/i386/pc.c | 53 ++
hw/mem/nvdimm.c | 12 ++
hw/mem/pc-dimm.c | 82 +++-
hw/misc/ivshmem.c| 3 +-
hw/ppc/spapr.c | 36 +-
include/hw/mem/pc-dimm.h | 6 ++-
include/sysemu/hostmem.h | 3 +-
numa.c | 3 +-
9 files changed, 81 insertions(+), 120 deletions(-)
--
2.17.1
[Qemu-devel] [PATCH v1 07/11] pc-dimm: get_memory_region() can never fail
We already verify when realizing that the memdev property has been
set. We have no more accesses to get_memory_region() before the device
is realized.
So this function will never fail. Remove the stale check and the
error variable. Add a comment to the functions stating that they should
never be called on uninitialized devices.
Signed-off-by: David Hildenbrand
---
hw/i386/pc.c | 7 +--
hw/mem/nvdimm.c | 2 +-
hw/mem/pc-dimm.c | 21 ++---
hw/ppc/spapr.c | 14 +++---
include/hw/mem/pc-dimm.h | 4 +++-
5 files changed, 14 insertions(+), 34 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 85c040482e..017396fe84 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1706,15 +1706,10 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr;
+MemoryRegion *mr = ddc->get_memory_region(dimm);
uint64_t align = TARGET_PAGE_SIZE;
bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
-mr = ddc->get_memory_region(dimm, &local_err);
-if (local_err) {
-goto out;
-}
-
if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
align = memory_region_get_alignment(mr);
}
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index df9716231f..b2dc2bbb50 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -96,7 +96,7 @@ static void nvdimm_init(Object *obj)
nvdimm_get_unarmed, nvdimm_set_unarmed, NULL);
}
-static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
{
NVDIMMDevice *nvdimm = NVDIMM(dimm);
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 5294734529..7bb6ce509c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -35,14 +35,9 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState
*machine,
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
Error *local_err = NULL;
-MemoryRegion *mr;
+MemoryRegion *mr = ddc->get_memory_region(dimm);
uint64_t addr;
-mr = ddc->get_memory_region(dimm, &local_err);
-if (local_err) {
-goto out;
-}
-
addr = object_property_get_uint(OBJECT(dimm),
PC_DIMM_ADDR_PROP, &local_err);
if (local_err) {
@@ -89,7 +84,7 @@ void pc_dimm_memory_unplug(DeviceState *dev, MachineState
*machine)
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
-MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
+MemoryRegion *mr = ddc->get_memory_region(dimm);
memory_device_unplug_region(machine, mr);
vmstate_unregister_ram(vmstate_mr, dev);
@@ -172,7 +167,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const
char *name,
return;
}
-mr = ddc->get_memory_region(dimm, errp);
+mr = ddc->get_memory_region(dimm);
if (!mr) {
return;
}
@@ -223,13 +218,9 @@ static void pc_dimm_unrealize(DeviceState *dev, Error
**errp)
host_memory_backend_set_mapped(dimm->hostmem, false);
}
-static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error
**errp)
+static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
{
-if (!dimm->hostmem) {
-error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set");
-return NULL;
-}
-
+g_assert(dimm->hostmem);
return host_memory_backend_get_memory(dimm->hostmem);
}
@@ -252,7 +243,7 @@ static uint64_t pc_dimm_md_get_region_size(const
MemoryDeviceState *md)
const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
MemoryRegion *mr;
-mr = ddc->get_memory_region(dimm, &error_abort);
+mr = ddc->get_memory_region(dimm);
if (!mr) {
return 0;
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a5f1bbd58a..214286fd2f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3142,14 +3142,10 @@ static void spapr_memory_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr;
+MemoryRegion *mr = ddc->get_memory_region(dimm);
uint64_t align, size, addr;
uint32_t node;
-mr = ddc->get_memory_region(dimm, &local_err);
-if (local_err) {
-goto out;
-}
align = memory_region_get_alignment(mr);
size = memory_region_size(mr);
@@ -3263,7 +3259,7 @@ static sPAPRDIMMState
*spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
{
sPAPRDRConnector *drc;
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLA
[Qemu-devel] [PATCH v1 03/11] pc: factor out pc-dimm checks into pc_dimm_pre_plug()
We can perform these checks before the device is actually realized.
Signed-off-by: David Hildenbrand
---
hw/i386/pc.c | 44 ++--
1 file changed, 26 insertions(+), 18 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3befe6721..85c040482e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1674,6 +1674,29 @@ void ioapic_init_gsi(GSIState *gsi_state, const char
*parent_name)
}
}
+static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
+const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+
+/*
+ * When -no-acpi is used with Q35 machine type, no ACPI is built,
+ * but pcms->acpi_dev is still created. Check !acpi_enabled in
+ * addition to cover this case.
+ */
+if (!pcms->acpi_dev || !acpi_enabled) {
+error_setg(errp,
+ "memory hotplug is not enabled: missing acpi device or acpi
disabled");
+return;
+}
+
+if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
+error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+return;
+}
+}
+
static void pc_dimm_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -1696,23 +1719,6 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
align = memory_region_get_alignment(mr);
}
-/*
- * When -no-acpi is used with Q35 machine type, no ACPI is built,
- * but pcms->acpi_dev is still created. Check !acpi_enabled in
- * addition to cover this case.
- */
-if (!pcms->acpi_dev || !acpi_enabled) {
-error_setg(&local_err,
- "memory hotplug is not enabled: missing acpi device or acpi
disabled");
-goto out;
-}
-
-if (is_nvdimm && !pcms->acpi_nvdimm_state.is_enabled) {
-error_setg(&local_err,
- "nvdimm is not enabled: missing 'nvdimm' in '-M'");
-goto out;
-}
-
pc_dimm_memory_plug(dev, MACHINE(pcms), align, &local_err);
if (local_err) {
goto out;
@@ -2006,7 +2012,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
-if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+pc_dimm_pre_plug(hotplug_dev, dev, errp);
+} else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
pc_cpu_pre_plug(hotplug_dev, dev, errp);
}
}
--
2.17.1
[Qemu-devel] [PATCH v1 05/11] spapr: move memory hotplug size check into plug code
This might look like a step backwards, but it is not. get_memory_region()
should not be called on uninititalized devices. In general, only
properties should be access, but no "derived" satte like the memory
region.
1. We need duplicate error checks if memdev is actually already set.
realize() performs these checks, no need to duplicate.
2. This is bad practise as one can see when looking at the NVDIMM
implemetation. The call does not return sane data before the device
is realized. Although spapr does not use NVDIMM, conceptually it is
wrong.
So let's just move this call to the right place. We can then cleanup
get_memory_region().
Signed-off-by: David Hildenbrand
---
hw/ppc/spapr.c | 21 ++---
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f5daac..a5f1bbd58a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3153,6 +3153,12 @@ static void spapr_memory_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
align = memory_region_get_alignment(mr);
size = memory_region_size(mr);
+if (size % SPAPR_MEMORY_BLOCK_SIZE) {
+error_setg(&local_err, "Hotplugged memory size must be a multiple of "
+ "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
+goto out;
+}
+
pc_dimm_memory_plug(dev, MACHINE(ms), align, &local_err);
if (local_err) {
goto out;
@@ -3186,9 +3192,6 @@ static void spapr_memory_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
{
const sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
PCDIMMDevice *dimm = PC_DIMM(dev);
-PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
-MemoryRegion *mr;
-uint64_t size;
char *mem_dev;
if (!smc->dr_lmb_enabled) {
@@ -3196,18 +3199,6 @@ static void spapr_memory_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
return;
}
-mr = ddc->get_memory_region(dimm, errp);
-if (!mr) {
-return;
-}
-size = memory_region_size(mr);
-
-if (size % SPAPR_MEMORY_BLOCK_SIZE) {
-error_setg(errp, "Hotplugged memory size must be a multiple of "
- "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE);
-return;
-}
-
mem_dev = object_property_get_str(OBJECT(dimm), PC_DIMM_MEMDEV_PROP, NULL);
if (mem_dev && !kvmppc_is_mem_backend_page_size_ok(mem_dev)) {
error_setg(errp, "Memory backend has bad page size. "
--
2.17.1
[Qemu-devel] [PATCH v1 08/11] pc-dimm: get_memory_region() will never return a NULL pointer
This is guaranteed by passing into host_memory_backend_get_memory() a
value that is not NULL - which is what we always do.
Signed-off-by: David Hildenbrand
---
hw/mem/pc-dimm.c | 15 ++-
1 file changed, 2 insertions(+), 13 deletions(-)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 7bb6ce509c..9a0da5d441 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -157,7 +157,6 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const
char *name,
void *opaque, Error **errp)
{
uint64_t value;
-MemoryRegion *mr;
PCDIMMDevice *dimm = PC_DIMM(obj);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
@@ -167,11 +166,7 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
const char *name,
return;
}
-mr = ddc->get_memory_region(dimm);
-if (!mr) {
-return;
-}
-value = memory_region_size(mr);
+value = memory_region_size(ddc->get_memory_region(dimm));
visit_type_uint64(v, name, &value, errp);
}
@@ -241,14 +236,8 @@ static uint64_t pc_dimm_md_get_region_size(const
MemoryDeviceState *md)
/* dropping const here is fine as we don't touch the memory region */
PCDIMMDevice *dimm = PC_DIMM(md);
const PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(md);
-MemoryRegion *mr;
-
-mr = ddc->get_memory_region(dimm);
-if (!mr) {
-return 0;
-}
-return memory_region_size(mr);
+return memory_region_size(ddc->get_memory_region(dimm));
}
static void pc_dimm_md_fill_device_info(const MemoryDeviceState *md,
--
2.17.1
[Qemu-devel] [PATCH v1 06/11] pc-dimm: don't allow to access "size" before the device was realized
"size" should not be queried before the device was realized. Let' make
that explicit.
Signed-off-by: David Hildenbrand
---
hw/mem/pc-dimm.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 86fbcf2d0c..5294734529 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -166,6 +166,12 @@ static void pc_dimm_get_size(Object *obj, Visitor *v,
const char *name,
PCDIMMDevice *dimm = PC_DIMM(obj);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
+if (!DEVICE(obj)->realized) {
+error_setg(errp, "Property \"%s\" not accessible before realized",
+ name);
+return;
+}
+
mr = ddc->get_memory_region(dimm, errp);
if (!mr) {
return;
--
2.17.1
[Qemu-devel] [PATCH v1 10/11] pc-dimm: introduce and use pc_dimm_memory_pre_plug()
We'll be factoring out some pc-dimm specific and some memory-device
checks next.
Signed-off-by: David Hildenbrand
---
hw/i386/pc.c | 2 ++
hw/mem/pc-dimm.c | 5 +
hw/ppc/spapr.c | 1 +
include/hw/mem/pc-dimm.h | 2 ++
4 files changed, 10 insertions(+)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 017396fe84..dc8e7b033b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1695,6 +1695,8 @@ static void pc_dimm_pre_plug(HotplugHandler *hotplug_dev,
DeviceState *dev,
error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
return;
}
+
+pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
}
static void pc_dimm_plug(HotplugHandler *hotplug_dev,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index bc79dd04d8..995ce22d8d 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -27,6 +27,11 @@
#include "sysemu/numa.h"
#include "trace.h"
+void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
+ Error **errp)
+{
+}
+
void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
uint64_t align, Error **errp)
{
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 214286fd2f..54eddc0069 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3202,6 +3202,7 @@ static void spapr_memory_pre_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
goto out;
}
+pc_dimm_memory_pre_plug(dev, MACHINE(hotplug_dev), errp);
out:
g_free(mem_dev);
}
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index f0e6867803..7d46a0a0cb 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -80,6 +80,8 @@ typedef struct PCDIMMDeviceClass {
int pc_dimm_get_free_slot(const int *hint, int max_slots, Error **errp);
+void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
+ Error **errp);
void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
uint64_t align, Error **errp);
void pc_dimm_memory_unplug(DeviceState *dev, MachineState *machine);
--
2.17.1
[Qemu-devel] [PATCH v1 04/11] hostmem: drop error variable from host_memory_backend_get_memory()
Unused, so let's remove it.
Signed-off-by: David Hildenbrand
---
backends/hostmem.c | 3 +--
hw/mem/nvdimm.c | 4 ++--
hw/mem/pc-dimm.c | 4 ++--
hw/misc/ivshmem.c| 3 +--
include/sysemu/hostmem.h | 3 +--
numa.c | 3 +--
6 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 3627e61584..4908946cd3 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -247,8 +247,7 @@ bool host_memory_backend_mr_inited(HostMemoryBackend
*backend)
return memory_region_size(&backend->mr) != 0;
}
-MemoryRegion *
-host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend)
{
return host_memory_backend_mr_inited(backend) ? &backend->mr : NULL;
}
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index f974accbdd..df9716231f 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -105,7 +105,7 @@ static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice
*dimm, Error **errp)
static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
{
-MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem);
NVDIMMDevice *nvdimm = NVDIMM(dimm);
uint64_t align, pmem_size, size = memory_region_size(mr);
@@ -161,7 +161,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm,
const void *buf,
memcpy(nvdimm->label_data + offset, buf, size);
-mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+mr = host_memory_backend_get_memory(dimm->hostmem);
backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
memory_region_set_dirty(mr, backend_offset, size);
}
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 62b34a992e..86fbcf2d0c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -224,12 +224,12 @@ static MemoryRegion
*pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
return NULL;
}
-return host_memory_backend_get_memory(dimm->hostmem, errp);
+return host_memory_backend_get_memory(dimm->hostmem);
}
static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
{
-return host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+return host_memory_backend_get_memory(dimm->hostmem);
}
static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 16f03701b7..ee01c5e66b 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -909,8 +909,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error
**errp)
if (s->hostmem != NULL) {
IVSHMEM_DPRINTF("using hostmem\n");
-s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem,
- &error_abort);
+s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
} else {
Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
assert(chr);
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 5beb0ef8ab..6e6bd2c1cb 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -62,8 +62,7 @@ struct HostMemoryBackend {
};
bool host_memory_backend_mr_inited(HostMemoryBackend *backend);
-MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
- Error **errp);
+MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend);
void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
diff --git a/numa.c b/numa.c
index 33572bfa74..94f758c757 100644
--- a/numa.c
+++ b/numa.c
@@ -523,8 +523,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr,
Object *owner,
if (!backend) {
continue;
}
-MemoryRegion *seg = host_memory_backend_get_memory(backend,
- &error_fatal);
+MemoryRegion *seg = host_memory_backend_get_memory(backend);
if (memory_region_is_mapped(seg)) {
char *path = object_get_canonical_path_component(OBJECT(backend));
--
2.17.1
Re: [Qemu-devel] [PATCH v2 00/20] Drain fixes and cleanups, part 3
ping? Am 29.05.2018 um 19:21 hat Kevin Wolf geschrieben: > This is the third and hopefully for now last part of my work to fix > drain. The main goal of this series is to make drain robust against > graph changes that happen in any callbacks of in-flight requests while > we drain a block node. > > The individual patches describe the details, but the rough plan is to > change all three drain types (single node, subtree and all) to work like > this: > > 1. First call all the necessary callbacks to quiesce external sources >for new requests. This includes the block driver callbacks, the child >node callbacks and disabling external AioContext events. This is done >recursively. > >Much of the trouble we had with drain resulted from the fact that the >graph changed while we were traversing the graph recursively. None of >the callbacks called in this phase may change the graph. > > 2. Then do a single AIO_WAIT_WHILE() to drain the requests of all >affected nodes. The aio_poll() called by it is where graph changes >can happen and we need to be careful. > >However, while evaluating the loop condition, the graph can't change, >so we can safely call all necessary callbacks, if needed recursively, >to determine whether there are still pending requests in any affected >nodes. We just need to make sure that we don't rely on the set of >nodes being the same between any two evaluation of the condition. > > There are a few more smaller, mostly self-contained changes needed > before we're actually safe, but this is the main mechanism that will > help you understand what we're working towards during the series. > > v2: > > - Rebased on top of current master (e.g. including Job infrastructure) > > - Avoid unnecessary parent callbacks for .drained_begin/poll/end: > * subtree drains: Don't propagate the drain to the parent that we came > from recursively > * drain_all: Don't propagate the drain to BDS parents (which are > already separately drained), but only to non-BDS > parents like BBs or Jobs > > - Separate bdrv_drain_poll_top_level() function instead of having a > top_level parameter for bdrv_drain_poll(). > > - A few commit message and comment improvements > > > Kevin Wolf (19): > test-bdrv-drain: bdrv_drain() works with cross-AioContext events > block: Use bdrv_do_drain_begin/end in bdrv_drain_all() > block: Remove 'recursive' parameter from bdrv_drain_invoke() > block: Don't manually poll in bdrv_drain_all() > tests/test-bdrv-drain: bdrv_drain_all() works in coroutines now > block: Avoid unnecessary aio_poll() in AIO_WAIT_WHILE() > block: Really pause block jobs on drain > block: Remove bdrv_drain_recurse() > block: Drain recursively with a single BDRV_POLL_WHILE() > test-bdrv-drain: Test node deletion in subtree recursion > block: Don't poll in parent drain callbacks > test-bdrv-drain: Graph change through parent callback > block: Defer .bdrv_drain_begin callback to polling phase > test-bdrv-drain: Test that bdrv_drain_invoke() doesn't poll > block: Allow AIO_WAIT_WHILE with NULL ctx > block: Move bdrv_drain_all_begin() out of coroutine context > block: ignore_bds_parents parameter for drain functions > block: Allow graph changes in bdrv_drain_all_begin/end sections > test-bdrv-drain: Test graph changes in drain_all section > > Max Reitz (1): > test-bdrv-drain: Add test for node deletion > > include/block/aio-wait.h | 25 +- > include/block/block.h| 31 +- > include/block/block_int.h| 14 + > include/block/blockjob_int.h | 8 + > block.c | 52 +++- > block/io.c | 332 > block/mirror.c | 8 + > block/vvfat.c| 1 + > blockjob.c | 23 ++ > tests/test-bdrv-drain.c | 705 > +-- > 10 files changed, 1032 insertions(+), 167 deletions(-) > > -- > 2.13.6 >
Re: [Qemu-devel] [RFC] Intermediate block mirroring
On Mon 11 Jun 2018 02:20:06 PM CEST, Kevin Wolf wrote:
> Am 01.06.2018 um 12:51 hat Alberto Garcia geschrieben:
>> On Thu 03 May 2018 02:22:41 PM CEST, Kevin Wolf wrote:
>> >> > Were the (more or less) exact requirements of QMP blockdev-reopen
>> >> > discussed? How is it different from qemu-io's "reopen" command?
>> >> > What are the options that you can and can not change?
>> >>
>> >> I can't quite remember, I'm afraid. I think it was supposed to be
>> >> pretty much qemu-io's reopen (so just bdrv_reopen()). I suppose you
>> >> cannot change the driver (obviously) or probably the node name, because
>> >> either would result in the node being replaced by a completely new one.
>> >>
>> >> Other than that, it probably depends on what the block driver supports,
>> >> but ideally you should be able to change everything.
>> >
>> > Honestly the design of bdrv_reopen() is quite broken because of the
>> > way it tries to maintain old options if they aren't specified, and
>> > guesses what you might mean when you add flags to the mix. The exact
>> > semantics are quite complicated and I'd rather avoid them in a stable
>> > API.
>> >
>> > A clean QMP command would probably apply the same defaults as
>> > blockdev-add, so you just get to specify the full options again.
>>
>> I have a prototype of this working and almost ready to be published, but
>> there's a tricky thing with this part:
>>
>> If we want blockdev-reopen to apply the defaults for all options except
>> from the ones expliclity specified by the user, then it means that we
>> need to check not just the options that are present, but also the ones
>> that are omitted.
>>
>> For example:
>>
>>{ "execute": "blockdev-add",
>> "arguments": { "driver": "null-aio",
>> "node-name": "root",
>> "size": 1024 }
>>
>> This adds a null-aio block device with the "size" option set to 1024
>> (the default is 1 << 30).
>>
>> null_reopen_prepare() allows reopening that block device, but it does
>> not allow changing any of its options. Attempting to change the value of
>> "size" is detected by the loop that checks unhandled options at the end
>> of bdrv_reopen_prepare() and returns "Cannot change the option 'size'".
>>
>> So far, so good. We have this generic check for all options that works
>> with all drivers, so as long as we only specify options that we know
>> that can be changed, everything is fine.
>>
>> However if we want blockdev-reopen to apply the default values for all
>> omitted options, then omitting "size" would be equivalent to setting it
>> to its default value (1 << 30). And if "size" cannot be changed then
>> QEMU should complain unless we explicitly set "size" to 1024 again on
>> reopen.
>>
>> This complicates things a bit, because we would go from "the options
>> that can't be changed are the ones that are not handled by each driver's
>> _prepare() function" to "options that are absent can also produce an
>> error".
>
> To be honest, I think this is fine. If the user can specify the size
> once (in blockdev-add), they can do it again (in blockdev-reopen).
>
> We just need to make sure that we don't break existing bdrv_reopen*()
> calls that come from places other than the monitor.
Good. I have the first revision of the series almost ready, expect it
this week.
Berto
[Qemu-devel] [PATCH v1 09/11] pc-dimm: remove pc_dimm_get_vmstate_memory_region()
We can reuse pc_dimm_get_memory_region() now, as both functions are
(besides the assert which is also correct), equal.
Signed-off-by: David Hildenbrand
---
hw/mem/pc-dimm.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 9a0da5d441..bc79dd04d8 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -219,11 +219,6 @@ static MemoryRegion
*pc_dimm_get_memory_region(PCDIMMDevice *dimm)
return host_memory_backend_get_memory(dimm->hostmem);
}
-static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm)
-{
-return host_memory_backend_get_memory(dimm->hostmem);
-}
-
static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
{
const PCDIMMDevice *dimm = PC_DIMM(md);
@@ -282,7 +277,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
dc->desc = "DIMM memory module";
ddc->get_memory_region = pc_dimm_get_memory_region;
-ddc->get_vmstate_memory_region = pc_dimm_get_vmstate_memory_region;
+ddc->get_vmstate_memory_region = pc_dimm_get_memory_region;
mdc->get_addr = pc_dimm_md_get_addr;
/* for a dimm plugged_size == region_size */
--
2.17.1
[Qemu-devel] [PATCH v1 11/11] pc-dimm: assign and verify the "slot" property during pre_plug
We can assign and verify the slot before realizing and trying to plug.
reading/writing the slot property should never change, so let's reduce
error handling a bit by using &error_abort.
Signed-off-by: David Hildenbrand
---
hw/mem/pc-dimm.c | 31 ++-
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 995ce22d8d..88423f95a3 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -30,12 +30,25 @@
void pc_dimm_memory_pre_plug(DeviceState *dev, MachineState *machine,
Error **errp)
{
+Error *local_err = NULL;
+int slot;
+
+slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
+ &error_abort);
+slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL :
&slot,
+ machine->ram_slots, &local_err);
+if (local_err) {
+goto out;
+}
+object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP,
&error_abort);
+trace_mhp_pc_dimm_assigned_slot(slot);
+out:
+error_propagate(errp, local_err);
}
void pc_dimm_memory_plug(DeviceState *dev, MachineState *machine,
uint64_t align, Error **errp)
{
-int slot;
PCDIMMDevice *dimm = PC_DIMM(dev);
PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm);
@@ -61,22 +74,6 @@ void pc_dimm_memory_plug(DeviceState *dev, MachineState
*machine,
}
trace_mhp_pc_dimm_assigned_address(addr);
-slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
-if (local_err) {
-goto out;
-}
-
-slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL :
&slot,
- machine->ram_slots, &local_err);
-if (local_err) {
-goto out;
-}
-object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
-if (local_err) {
-goto out;
-}
-trace_mhp_pc_dimm_assigned_slot(slot);
-
memory_device_plug_region(machine, mr, addr);
vmstate_register_ram(vmstate_mr, dev);
--
2.17.1
Re: [Qemu-devel] [RFC] Intermediate block mirroring
Am 01.06.2018 um 12:51 hat Alberto Garcia geschrieben:
> On Thu 03 May 2018 02:22:41 PM CEST, Kevin Wolf wrote:
> >> > Were the (more or less) exact requirements of QMP blockdev-reopen
> >> > discussed? How is it different from qemu-io's "reopen" command?
> >> > What are the options that you can and can not change?
> >>
> >> I can't quite remember, I'm afraid. I think it was supposed to be
> >> pretty much qemu-io's reopen (so just bdrv_reopen()). I suppose you
> >> cannot change the driver (obviously) or probably the node name, because
> >> either would result in the node being replaced by a completely new one.
> >>
> >> Other than that, it probably depends on what the block driver supports,
> >> but ideally you should be able to change everything.
> >
> > Honestly the design of bdrv_reopen() is quite broken because of the
> > way it tries to maintain old options if they aren't specified, and
> > guesses what you might mean when you add flags to the mix. The exact
> > semantics are quite complicated and I'd rather avoid them in a stable
> > API.
> >
> > A clean QMP command would probably apply the same defaults as
> > blockdev-add, so you just get to specify the full options again.
>
> I have a prototype of this working and almost ready to be published, but
> there's a tricky thing with this part:
>
> If we want blockdev-reopen to apply the defaults for all options except
> from the ones expliclity specified by the user, then it means that we
> need to check not just the options that are present, but also the ones
> that are omitted.
>
> For example:
>
>{ "execute": "blockdev-add",
> "arguments": { "driver": "null-aio",
> "node-name": "root",
> "size": 1024 }
>
> This adds a null-aio block device with the "size" option set to 1024
> (the default is 1 << 30).
>
> null_reopen_prepare() allows reopening that block device, but it does
> not allow changing any of its options. Attempting to change the value of
> "size" is detected by the loop that checks unhandled options at the end
> of bdrv_reopen_prepare() and returns "Cannot change the option 'size'".
>
> So far, so good. We have this generic check for all options that works
> with all drivers, so as long as we only specify options that we know
> that can be changed, everything is fine.
>
> However if we want blockdev-reopen to apply the default values for all
> omitted options, then omitting "size" would be equivalent to setting it
> to its default value (1 << 30). And if "size" cannot be changed then
> QEMU should complain unless we explicitly set "size" to 1024 again on
> reopen.
>
> This complicates things a bit, because we would go from "the options
> that can't be changed are the ones that are not handled by each driver's
> _prepare() function" to "options that are absent can also produce an
> error".
To be honest, I think this is fine. If the user can specify the size
once (in blockdev-add), they can do it again (in blockdev-reopen).
We just need to make sure that we don't break existing bdrv_reopen*()
calls that come from places other than the monitor.
Kevin
Re: [Qemu-devel] An emulation failure occurs, if I hotplug vcpus immediately after the VM start
Hi David and Paolo, > -Original Message- > From: David Hildenbrand [mailto:da...@redhat.com] > Sent: Monday, June 11, 2018 6:44 PM > To: 浙大邮箱 > Cc: Paolo Bonzini ; Gonglei (Arei) > ; Igor Mammedov ; > xuyandong ; Zhanghailiang > ; wangxin (U) > ; lidonglin ; > k...@vger.kernel.org; qemu-devel@nongnu.org; Huangweidong (C) > > Subject: Re: An emulation failure occurs,if I hotplug vcpus immediately after > the > VM start > > On 07.06.2018 18:03, 浙大邮箱 wrote: > > Hi，all > > I still have a question after reading your discussion: Will seabios detect > > the > change of address space even if we add_region and del_region automatically? I > guess that seabios may not take this change into consideration. > > Hi, > > We would just change the way how KVM memory slots are updated. This is > right now not atomic, but would be later on. It should not have any > other effect. > Yes. Do you have any plans to do that? Thanks, -Gonglei
Re: [Qemu-devel] An emulation failure occurs, if I hotplug vcpus immediately after the VM start
On 11.06.2018 14:25, Gonglei (Arei) wrote: > > Hi David and Paolo, > >> -Original Message- >> From: David Hildenbrand [mailto:da...@redhat.com] >> Sent: Monday, June 11, 2018 6:44 PM >> To: 浙大邮箱 >> Cc: Paolo Bonzini ; Gonglei (Arei) >> ; Igor Mammedov ; >> xuyandong ; Zhanghailiang >> ; wangxin (U) >> ; lidonglin ; >> k...@vger.kernel.org; qemu-devel@nongnu.org; Huangweidong (C) >> >> Subject: Re: An emulation failure occurs,if I hotplug vcpus immediately >> after the >> VM start >> >> On 07.06.2018 18:03, 浙大邮箱 wrote: >>> Hi，all >>> I still have a question after reading your discussion: Will seabios detect >>> the >> change of address space even if we add_region and del_region automatically? I >> guess that seabios may not take this change into consideration. >> >> Hi, >> >> We would just change the way how KVM memory slots are updated. This is >> right now not atomic, but would be later on. It should not have any >> other effect. >> > Yes. Do you have any plans to do that? Well, I have plans to work on atomically resizable memory regions (atomic del + add), and what Paolo described could also work for that use case. However, I won't have time to look into that in the near future. So if somebody else wants to jump it, perfect. If not, it will have to wait unfortunately. > > Thanks, > -Gonglei > -- Thanks, David / dhildenb
Re: [Qemu-devel] [PULL 0/9] M68k for 3.0 patches
On 11 June 2018 at 11:49, Laurent Vivier wrote: > The following changes since commit 0d2fa03dae4fbe185a082f361342b1e30aed4582: > > Merge remote-tracking branch > 'remotes/pmaydell/tags/pull-target-arm-20180608' into staging (2018-06-08 > 16:26:51 +0100) > > are available in the Git repository at: > > git://github.com/vivier/qemu-m68k.git tags/m68k-for-3.0-pull-request > > for you to fetch changes up to a56f36c1d2bccbc50a53fa8093b93d205607f1b8: > > target/m68k: Merge disas_m68k_insn into m68k_tr_translate_insn (2018-06-11 > 12:43:42 +0200) > > > Convert to TranslatorOps > > I've updated the series to fix conflicts with: > > 21528149eb target/m68k: Add trailing '\n' to qemu_log() call > 07ea28b418 tcg: Pass tb and index to tcg_gen_exit_tb separately > > > Applied, thanks. -- PMM
[Qemu-devel] [Bug 1776224] [NEW] QEMU's SPICE server not getting leds callback when sending capslock
Public bug reported: I'm having troubles using the QEMU's SPICE server for remote views. When trying to sync leds from my SPICE client to QEMU, I can see that the caps lock keycodes are sent but the server state never gets updated (which leads in funny behaviours like caps lock "blinking" in the guest if the clients resyncs its state often). That behaviour doesn't happen at all with the numlock led which does the same thing but with different keycodes. Here is an example of what's happening when the client tries to resync with a capslock difference: > Spice received: 58 > ps2_put_keycode: 88 > > Spice received: 186 > ps2_put_keycode: 240 > ps2_put_keycode: 88 > ps2_queue: 186 And with numlock: > Spice received: 69 > ps2_put_keycode: 119 > > Spice received: 197 > ps2_put_keycode: 240 > ps2_put_keycode: 119 > ps2_queue: 197 > ps2_set_ledstate: 0 > Spice new ledstate: 0 This behaviour is consistent across SPICE clients and only appears in a linux TTY (it works fine if I start an X server and on windows). It still happens on QEMU latest commit at the time (0d2fa03dae4fbe185a082f361342b1e30aed4582) Spice registers a callback for leds via qemu_add_led_event_handler but it's never called for capslock. Is that a normal behaviour ? Is there any reasons for the capslock led not to be updated ? ** Affects: qemu Importance: Undecided Status: New ** Attachment added: "Trace of a capslock press on both linux and windows" https://bugs.launchpad.net/bugs/1776224/+attachment/5151280/+files/trace_kbd -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1776224 Title: QEMU's SPICE server not getting leds callback when sending capslock Status in QEMU: New Bug description: I'm having troubles using the QEMU's SPICE server for remote views. When trying to sync leds from my SPICE client to QEMU, I can see that the caps lock keycodes are sent but the server state never gets updated (which leads in funny behaviours like caps lock "blinking" in the guest if the clients resyncs its state often). That behaviour doesn't happen at all with the numlock led which does the same thing but with different keycodes. Here is an example of what's happening when the client tries to resync with a capslock difference: > Spice received: 58 > ps2_put_keycode: 88 > > Spice received: 186 > ps2_put_keycode: 240 > ps2_put_keycode: 88 > ps2_queue: 186 And with numlock: > Spice received: 69 > ps2_put_keycode: 119 > > Spice received: 197 > ps2_put_keycode: 240 > ps2_put_keycode: 119 > ps2_queue: 197 > ps2_set_ledstate: 0 > Spice new ledstate: 0 This behaviour is consistent across SPICE clients and only appears in a linux TTY (it works fine if I start an X server and on windows). It still happens on QEMU latest commit at the time (0d2fa03dae4fbe185a082f361342b1e30aed4582) Spice registers a callback for leds via qemu_add_led_event_handler but it's never called for capslock. Is that a normal behaviour ? Is there any reasons for the capslock led not to be updated ? To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1776224/+subscriptions
Re: [Qemu-devel] [PATCH v3 0/7] Reenable hmp for preconfig mode
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: >> Let's also provide "quit", both in HMP and QMP. > > I tried enabling quit, but it fails with an incorrect > state transition. Igor, this one's for you.
[Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr
This patchset is two vaguely related patches that I wrote while I was trying to understand the cputlb code enough to add support for small-MPU-regions for v7M/v8M. Patch 1 is just a docs comment patch, describing what the CPUIOTLBEntry 'addr' field actually contains. Patch 2 fixes a bug that doesn't yet have any visible effects (we were passing cpu_transaction_failed() the wrong value for the physaddr) roughly as suggested by Paolo back in https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg02419.html thanks -- PMM Peter Maydell (2): cpu-defs.h: Document CPUIOTLBEntry 'addr' field cputlb: Pass cpu_transaction_failed() the correct physaddr include/exec/cpu-defs.h | 9 +++ include/exec/exec-all.h | 13 -- accel/tcg/cputlb.c | 56 +++-- exec.c | 5 ++-- 4 files changed, 66 insertions(+), 17 deletions(-) -- 2.17.1
[Qemu-devel] [PATCH 1/2] cpu-defs.h: Document CPUIOTLBEntry 'addr' field
The 'addr' field in the CPUIOTLBEntry struct has a rather non-obvious
use; add a comment documenting it (reverse-engineered from what
the code that sets it is doing).
Signed-off-by: Peter Maydell
---
include/exec/cpu-defs.h | 9 +
accel/tcg/cputlb.c | 12
2 files changed, 21 insertions(+)
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index e43ff8346b1..452e82d21c6 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -127,6 +127,15 @@ QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 <<
CPU_TLB_ENTRY_BITS));
* structs into one.)
*/
typedef struct CPUIOTLBEntry {
+/*
+ * @addr contains:
+ * - in the lower TARGET_PAGE_BITS, a physical section number
+ * - with the lower TARGET_PAGE_BITS masked off, an offset which
+ *must be added to the virtual address to obtain:
+ * + the ramaddr_t of the target RAM (if the physical section
+ * number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM)
+ * + the offset within the target MemoryRegion (otherwise)
+ */
hwaddr addr;
MemTxAttrs attrs;
} CPUIOTLBEntry;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 05439039e91..355ded27024 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -664,6 +664,18 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong
vaddr,
env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index];
/* refill the tlb */
+/*
+ * At this point iotlb contains a physical section number in the lower
+ * TARGET_PAGE_BITS, and either
+ * + the ramaddr_t of the page base of the target RAM (if NOTDIRTY or ROM)
+ * + the offset within section->mr of the page base (otherwise)
+ * We subtract the vaddr (which is page aligned and thus won't
+ * disturb the low bits) to give an offset which can be added to the
+ * (non-page-aligned) vaddr of the eventual memory access to get
+ * the MemoryRegion offset for the access. Note that the vaddr we
+ * subtract here is that of the page base, and not the same as the
+ * vaddr we add back in io_readx()/io_writex()/get_page_addr_code().
+ */
env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
env->iotlb[mmu_idx][index].attrs = attrs;
--
2.17.1
[Qemu-devel] [PATCH 2/2] cputlb: Pass cpu_transaction_failed() the correct physaddr
The API for cpu_transaction_failed() says that it takes the physical
address for the failed transaction. However we were actually passing
it the offset within the target MemoryRegion. We don't currently
have any target CPU implementations of this hook that require the
physical address; fix this bug so we don't get confused if we ever
do add one.
Suggested-by: Paolo Bonzini
Signed-off-by: Peter Maydell
---
include/exec/exec-all.h | 13 ++--
accel/tcg/cputlb.c | 44 +
exec.c | 5 +++--
3 files changed, 45 insertions(+), 17 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 4d09eaba72d..aed557d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -437,8 +437,17 @@ void tb_lock_reset(void);
#if !defined(CONFIG_USER_ONLY)
-struct MemoryRegion *iotlb_to_region(CPUState *cpu,
- hwaddr index, MemTxAttrs attrs);
+/**
+ * iotlb_to_section:
+ * @cpu: CPU performing the access
+ * @index: TCG CPU IOTLB entry
+ *
+ * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
+ * it refers to. @index will have been initially created and returned
+ * by memory_region_section_get_iotlb().
+ */
+struct MemoryRegionSection *iotlb_to_section(CPUState *cpu,
+ hwaddr index, MemTxAttrs attrs);
void tlb_fill(CPUState *cpu, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx, uintptr_t retaddr);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 355ded27024..77aa982cfd8 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -777,13 +777,16 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry
*iotlbentry,
target_ulong addr, uintptr_t retaddr, int size)
{
CPUState *cpu = ENV_GET_CPU(env);
-hwaddr physaddr = iotlbentry->addr;
-MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
+hwaddr mr_offset;
+MemoryRegionSection *section;
+MemoryRegion *mr;
uint64_t val;
bool locked = false;
MemTxResult r;
-physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
+section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+mr = section->mr;
+mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
cpu->mem_io_pc = retaddr;
if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
cpu_io_recompile(cpu, retaddr);
@@ -795,9 +798,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry
*iotlbentry,
qemu_mutex_lock_iothread();
locked = true;
}
-r = memory_region_dispatch_read(mr, physaddr,
+r = memory_region_dispatch_read(mr, mr_offset,
&val, size, iotlbentry->attrs);
if (r != MEMTX_OK) {
+hwaddr physaddr = mr_offset +
+section->offset_within_address_space -
+section->offset_within_region;
+
cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
mmu_idx, iotlbentry->attrs, r, retaddr);
}
@@ -814,12 +821,15 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry
*iotlbentry,
uintptr_t retaddr, int size)
{
CPUState *cpu = ENV_GET_CPU(env);
-hwaddr physaddr = iotlbentry->addr;
-MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
+hwaddr mr_offset;
+MemoryRegionSection *section;
+MemoryRegion *mr;
bool locked = false;
MemTxResult r;
-physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
+section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+mr = section->mr;
+mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
cpu_io_recompile(cpu, retaddr);
}
@@ -830,9 +840,13 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry
*iotlbentry,
qemu_mutex_lock_iothread();
locked = true;
}
-r = memory_region_dispatch_write(mr, physaddr,
+r = memory_region_dispatch_write(mr, mr_offset,
val, size, iotlbentry->attrs);
if (r != MEMTX_OK) {
+hwaddr physaddr = mr_offset +
+section->offset_within_address_space -
+section->offset_within_region;
+
cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
mmu_idx, iotlbentry->attrs, r, retaddr);
}
@@ -880,12 +894,13 @@ static bool victim_tlb_hit(CPUArchState *env, size_t
mmu_idx, size_t index,
*/
tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
{
-int mmu_idx, index, pd;
+int mmu_idx, index;
void *p;
MemoryRegion *mr;
+MemoryRegionSection *section;
CPUState *cpu = ENV_GET_CPU(env);
CPUIOTLBEntry *iotlbentry;
-hwaddr physaddr;
+hwad
Re: [Qemu-devel] [PATCH v5 1/2] arm_gicv3_kvm: kvm_dist_get/put_priority: skip the registers banked by GICR_IPRIORITYR
On 11 June 2018 at 09:32, Shannon Zhao wrote: > Hi Peter, > > Looks like we missed picking up this patch. I didn't include this in v6 > since you and Eric both reviewed it. > > Could you please pick it up? Oops; added to target-arm.next. (I generally forget entirely about vN of a patchset as soon as vN+1 appears on the list, so better not to drop patches from a set unless I've specifically said I've put them in target-arm.next or they're on the list as their own patchset.) thanks -- PMM
Re: [Qemu-devel] [PATCH v6 2/2] vl: fix use of --daemonize with --preconfig
On Fri, 8 Jun 2018 10:21:05 -0300
Eduardo Habkost wrote:
> On Thu, Jun 07, 2018 at 02:00:09PM +0200, Igor Mammedov wrote:
> > When using --daemonize, the initial lead process will fork a child and
> > then wait to be notified that setup is complete via a pipe, before it
> > exits. When using --preconfig there is an extra call to main_loop()
> > before the notification is done from os_setup_post(). Thus the parent
> > process won't exit until the mgmt application connects to the monitor
> > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> > won't connect to the monitor until daemonizing has completed though.
> >
> > This is a chicken and egg problem, leading to deadlock at startup.
> >
> > The only viable way to fix this is to call os_setup_post() before
> > the early main_loop() call when --preconfig is used. This has the
> > downside that any errors from this point onwards won't be handled
> > well by the mgmt application, because it will think QEMU has started
> > successfully, so not be expecting an abrupt exit. Moving as much user
> > input validation as possible to before the main_loop() call might help,
> > but mgmt application should stop assuming that QEMU has started
> > successfuly and use other means to collect errors from QEMU (logfile).
> >
> > Signed-off-by: Daniel P. Berrangé
> > Signed-off-by: Igor Mammedov
> > ---
> > v5:
> > * use original Daniel's patch [1], but addapt it to apply on top of
> > "[PATCH v3 1/2] cli: Don't run early event loop if no --preconfig was
> > specified"
> > with extra comment and massage commit message a little bit.
> > v6:
> > * hide os_setup_post_done flag inside of os_setup_post() as it was in v4
> >
> > CC: berra...@redhat.com
> > CC: mre...@redhat.com
> > CC: pbonz...@redhat.com
> > CC: ehabk...@redhat.com
> > CC: ldok...@redhat.com
> > CC: ebl...@redhat.com
> > ---
> > os-posix.c | 6 ++
> > vl.c | 6 ++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/os-posix.c b/os-posix.c
> > index 9ce6f74..0246195 100644
> > --- a/os-posix.c
> > +++ b/os-posix.c
> > @@ -309,8 +309,14 @@ void os_daemonize(void)
> >
> > void os_setup_post(void)
> > {
> > +static bool os_setup_post_done;
> > int fd = 0;
> >
> > +if (os_setup_post_done) {
> > +return;
> > +}
> > +os_setup_post_done = true;
> > +
> > if (daemonize) {
> > if (chdir("/")) {
> > error_report("not able to chdir to /: %s", strerror(errno));
> > diff --git a/vl.c b/vl.c
> > index fa44138..457ff2a 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4578,6 +4578,12 @@ int main(int argc, char **argv, char **envp)
> > parse_numa_opts(current_machine);
> >
> > /* do monitor/qmp handling at preconfig state if requested */
> > +if (!preconfig_exit_requested && is_daemonized()) {
> > +/* signal parent QEMU to exit, libvirt treats it as a sign
> > + * that monitor socket is ready to accept connections
> > + */
> > +os_setup_post();
> > +}
>
> I was looking at the daemonize logic, and noticed it we have a
> huge amount of code between this line and the next
> os_setup_post() call that could either:
>
> * call exit() and/or error_report(); or
logging would work to the extent mentioned in commit message,
i.e. it' would work fine when log file is used otherwise it
errors will go to /dev/null
so it should be more or less fine on this point
> * be unable to finish machine initialization because of
> chdir("/"), change_root(), or change_process_uid().
this one really no go.
I see 2 options here,
* move init code that opens files to early stage (before preconfig monitor)
or split it to open files early.
(I've spotted several obvious places fwcfg/vnc/replay_start/migration)
but there might be code somewhere in callbacks that would do it too,
so it rather risky to go this route.
(I'd do this anyways one place at the time using sanitizing
initialization sequence pretext.)
* split out signaling part that tells parent process to exit into
separate helper that's called once before/from main_loop().
This option seems low risk and additionally error output to
stderr will work as it does currently (until os_setup_post())
> Doesn't this make -preconfig and -daemonize fundamentally
> incompatible?
Don't see anything that prevents both to work together fundamentally.
essentially -preconfig is extra configuration after CLI,
we potentially would be able execute commands that open files there,
so we should leave chroot & co where it is now.
>
>
> > main_loop();
> >
> > /* from here on runstate is RUN_STATE_PRELAUNCH */
> > --
> > 2.7.4
> >
> >
>
Re: [Qemu-devel] [PATCH v3 2/7] hmp: Allow help on preconfig commands
"Dr. David Alan Gilbert" writes:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" writes:
>>
>> > From: "Dr. David Alan Gilbert"
>> >
>> > Allow the 'help' command in preconfig state but
>> > make it only list the preconfig commands.
>> >
>> > Signed-off-by: Dr. David Alan Gilbert
>> > Reviewed-by: Peter Xu
>> > Reviewed-by: Igor Mammedov
>> > ---
>> > hmp-commands.hx | 1 +
>> > monitor.c | 8 +++-
>> > 2 files changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hmp-commands.hx b/hmp-commands.hx
>> > index 0734fea931..8bf590ae4b 100644
>> > --- a/hmp-commands.hx
>> > +++ b/hmp-commands.hx
>> > @@ -15,6 +15,7 @@ ETEXI
>> > .params = "[cmd]",
>> > .help = "show the help",
>> > .cmd= do_help_cmd,
>> > +.flags = "p",
>> > },
>> >
>> > STEXI
>> > diff --git a/monitor.c b/monitor.c
>> > index f4a16e6a03..31c8f5dc88 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -957,6 +957,10 @@ static void help_cmd_dump_one(Monitor *mon,
>> > {
>> > int i;
>> >
>> > +if (runstate_check(RUN_STATE_PRECONFIG) && !cmd_can_preconfig(cmd)) {
>> > +return;
>> > +}
>> > +
>> > for (i = 0; i < prefix_args_nb; i++) {
>> > monitor_printf(mon, "%s ", prefix_args[i]);
>> > }
>> > @@ -979,7 +983,9 @@ static void help_cmd_dump(Monitor *mon, const
>> > mon_cmd_t *cmds,
>> >
>> > /* Find one entry to dump */
>> > for (cmd = cmds; cmd->name != NULL; cmd++) {
>> > -if (compare_cmd(args[arg_index], cmd->name)) {
>> > +if (compare_cmd(args[arg_index], cmd->name) &&
>> > +((!runstate_check(RUN_STATE_PRECONFIG) ||
>> > +cmd_can_preconfig(cmd {
>>
>> Why do we need this check in addition to the one in help_cmd_dump_one()?
>
> To gate entire subtables (we've only currently got 'info' and that's enabled,
> anyway, so it never actually triggers).
Something's bothering me around here, but I can't put a finger on it...
let me think aloud.
There's an asymmetry between command execution and help. The former has
just one check, in monitor_parse_command(). If the command has a
sub-table, and there are arguments, monitor_parse_command() recurses
into the sub-table. Thus, if the command is unavailable, the
sub-commands are unavailable as well, regardless of their "p" flags.
Help's recursion is different. It's limited to two levels, unlike
execution. help_cmd_dump_one()'s check applies to the last level.
help_cmd_dump()'s check applies to the first level. If there's just one
level, we check twice. Perhaps less than elegant, but harmless and
simple.
You can't make a sub-command available without also making the command
available. Makes sense, I guess. If you try, your "p" flags on the
sub-commands are silently ignored. That's a bit ugly, but if it doesn't
bother you, I can pretend I didn't see it ;)
> Dave
>
>> > if (cmd->sub_table) {
>> > /* continue with next arg */
>> > help_cmd_dump(mon, cmd->sub_table,
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] An emulation failure occurs, if I hotplug vcpus immediately after the VM start
> -Original Message- > From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On > Behalf Of David Hildenbrand > Sent: Monday, June 11, 2018 8:36 PM > To: Gonglei (Arei) ; 浙大邮箱 > Subject: Re: An emulation failure occurs,if I hotplug vcpus immediately after > the > VM start > > On 11.06.2018 14:25, Gonglei (Arei) wrote: > > > > Hi David and Paolo, > > > >> -Original Message- > >> From: David Hildenbrand [mailto:da...@redhat.com] > >> Sent: Monday, June 11, 2018 6:44 PM > >> To: 浙大邮箱 > >> Cc: Paolo Bonzini ; Gonglei (Arei) > >> ; Igor Mammedov ; > >> xuyandong ; Zhanghailiang > >> ; wangxin (U) > >> ; lidonglin ; > >> k...@vger.kernel.org; qemu-devel@nongnu.org; Huangweidong (C) > >> > >> Subject: Re: An emulation failure occurs,if I hotplug vcpus immediately > >> after > the > >> VM start > >> > >> On 07.06.2018 18:03, 浙大邮箱 wrote: > >>> Hi，all > >>> I still have a question after reading your discussion: Will seabios > >>> detect the > >> change of address space even if we add_region and del_region > automatically? I > >> guess that seabios may not take this change into consideration. > >> > >> Hi, > >> > >> We would just change the way how KVM memory slots are updated. This is > >> right now not atomic, but would be later on. It should not have any > >> other effect. > >> > > Yes. Do you have any plans to do that? > > Well, I have plans to work on atomically resizable memory regions > (atomic del + add), and what Paolo described could also work for that > use case. However, I won't have time to look into that in the near > future. So if somebody else wants to jump it, perfect. If not, it will > have to wait unfortunately. > Got it. :) Thanks, -Gonglei
Re: [Qemu-devel] [PATCH v3] qemu-ga: make get-fsinfo work over pci bridges
Hi Michael
On Mon, May 14, 2018 at 2:38 PM, Marc-André Lureau
wrote:
> Iterate over the PCI bridges to lookup the PCI device associated with
> the block device.
>
> This allows to lookup the driver under the following syspath:
> /sys/devices/pci:00/:00:02.2/:03:00.0/virtio2/block/vda/vda3
>
> It also works with an "old-style" Q35 libvirt hierarchy: root complex
> -> DMI-PCI bridge -> PCI-PCI bridge -> virtio controller, ex:
> /sys/devices/pci:00/:00:03.0/:01:01.0/:02:01.0/virtio1/block/vda/vda3
>
> The setup can be reproduced with the following qemu command line
> (Thanks Marcel for help):
>
> qemu-system-x86_64 -M q35 \
> -device i82801b11-bridge,id=dmi2pci_bridge,bus=pcie.0
> -device pci-bridge,id=pci_bridge,bus=dmi2pci_bridge,addr=0x1,chassis_nr=1
> -device
> virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1,bus=pci_bridge,addr=0x1
>
> For consistency with other syspath-related debug messages, replace a
> \"%s\" in the message with '%s'.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1567041
>
> Signed-off-by: Marc-André Lureau
> Reviewed-by: Laszlo Ersek
> ---
ping,
are you planning to send a pull request for qga patches
>
> v3:
> - replace \"%s\" in the debug message with '%s'
> - simplify the code by getting rid of an "else if"
>
> qga/commands-posix.c | 23 +++
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0dc219dbcf..624b0dc0c3 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -875,13 +875,28 @@ static void build_guest_fsinfo_for_real_device(char
> const *syspath,
> p = strstr(syspath, "/devices/pci");
> if (!p || sscanf(p + 12, "%*x:%*x/%x:%x:%x.%x%n",
> pci, pci + 1, pci + 2, pci + 3, &pcilen) < 4) {
> -g_debug("only pci device is supported: sysfs path \"%s\"", syspath);
> +g_debug("only pci device is supported: sysfs path '%s'", syspath);
> return;
> }
>
> -driver = get_pci_driver(syspath, (p + 12 + pcilen) - syspath, errp);
> -if (!driver) {
> -goto cleanup;
> +p += 12 + pcilen;
> +while (true) {
> +driver = get_pci_driver(syspath, p - syspath, errp);
> +if (driver && (g_str_equal(driver, "ata_piix") ||
> + g_str_equal(driver, "sym53c8xx") ||
> + g_str_equal(driver, "virtio-pci") ||
> + g_str_equal(driver, "ahci"))) {
> +break;
> +}
> +
> +if (sscanf(p, "/%x:%x:%x.%x%n",
> + pci, pci + 1, pci + 2, pci + 3, &pcilen) == 4) {
> +p += pcilen;
> +continue;
> +}
> +
> +g_debug("unsupported driver or sysfs path '%s'", syspath);
> +return;
> }
>
> p = strstr(syspath, "/target");
> --
> 2.17.0.253.g3dd125b46d
>
>
--
Marc-André Lureau
