From 1bc7e522d9cf1b58f2de9c8f1737be0bb5129c35 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:19 +0200 Subject: [PATCH 1/9] exec: Reduce CONFIG_USER_ONLY ifdeffenery Signed-off-by: Igor Mammedov Reviewed-by: David Gibson Signed-off-by: Eduardo Habkost --- bsd-user/qemu.h | 2 -- exec.c | 17 +++-------------- include/exec/exec-all.h | 12 ++++++++++++ linux-user/qemu.h | 2 -- 4 files changed, 15 insertions(+), 18 deletions(-) diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h index 6ccc544e7d..2b2b9184e0 100644 --- a/bsd-user/qemu.h +++ b/bsd-user/qemu.h @@ -209,8 +209,6 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size, abi_ulong new_addr); int target_msync(abi_ulong start, abi_ulong len, int flags); extern unsigned long last_brk; -void cpu_list_lock(void); -void cpu_list_unlock(void); #if defined(CONFIG_USE_NPTL) void mmap_fork_start(void); void mmap_fork_end(int child); diff --git a/exec.c b/exec.c index 60cf46a5b5..2f57c624a5 100644 --- a/exec.c +++ b/exec.c @@ -642,23 +642,17 @@ void cpu_exec_exit(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); -#if defined(CONFIG_USER_ONLY) cpu_list_lock(); -#endif if (cpu->cpu_index == -1) { /* cpu_index was never allocated by this @cpu or was already freed. */ -#if defined(CONFIG_USER_ONLY) cpu_list_unlock(); -#endif return; } QTAILQ_REMOVE(&cpus, cpu, node); cpu_release_index(cpu); cpu->cpu_index = -1; -#if defined(CONFIG_USER_ONLY) cpu_list_unlock(); -#endif if (cc->vmsd != NULL) { vmstate_unregister(NULL, cc->vmsd, cpu); @@ -670,7 +664,7 @@ void cpu_exec_exit(CPUState *cpu) void cpu_exec_init(CPUState *cpu, Error **errp) { - CPUClass *cc = CPU_GET_CLASS(cpu); + CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); Error *local_err = NULL; cpu->as = NULL; @@ -694,22 +688,17 @@ void cpu_exec_init(CPUState *cpu, Error **errp) object_ref(OBJECT(cpu->memory)); #endif -#if defined(CONFIG_USER_ONLY) cpu_list_lock(); -#endif cpu->cpu_index = cpu_get_free_index(&local_err); if (local_err) { error_propagate(errp, local_err); -#if defined(CONFIG_USER_ONLY) cpu_list_unlock(); -#endif return; } QTAILQ_INSERT_TAIL(&cpus, cpu, node); -#if defined(CONFIG_USER_ONLY) - (void) cc; cpu_list_unlock(); -#else + +#ifndef CONFIG_USER_ONLY if (qdev_get_vmsd(DEVICE(cpu)) == NULL) { vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common, cpu); } diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index acda7b613d..d008296c1b 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -56,6 +56,18 @@ TranslationBlock *tb_gen_code(CPUState *cpu, target_ulong pc, target_ulong cs_base, uint32_t flags, int cflags); +#if defined(CONFIG_USER_ONLY) +void cpu_list_lock(void); +void cpu_list_unlock(void); +#else +static inline void cpu_list_unlock(void) +{ +} +static inline void cpu_list_lock(void) +{ +} +#endif + void cpu_exec_init(CPUState *cpu, Error **errp); void QEMU_NORETURN cpu_loop_exit(CPUState *cpu); void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc); diff --git a/linux-user/qemu.h b/linux-user/qemu.h index cdf23a723a..bef465de4d 100644 --- a/linux-user/qemu.h +++ b/linux-user/qemu.h @@ -419,8 +419,6 @@ int target_msync(abi_ulong start, abi_ulong len, int flags); extern unsigned long last_brk; extern abi_ulong mmap_next_start; abi_ulong mmap_find_vma(abi_ulong, abi_ulong); -void cpu_list_lock(void); -void cpu_list_unlock(void); void mmap_fork_start(void); void mmap_fork_end(int child); From 8b1b835035fda831b405c1947210efcf758a7ca8 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:20 +0200 Subject: [PATCH 2/9] exec: Don't use cpu_index to detect if cpu_exec_init()'s been called Instead use QTAIL's tqe_prev field to detect if cpu's been placed in list by cpu_exec_init() which is always set if QTAIL element is in list. Fixes SIGSEGV on failure path in case cpu_index is assigned by board and cpu.relalize() fails before cpu_exec_init() is called. In follow up patches, cpu_index will be assigned by boards that support cpu hot(un)plug and need stable cpu_index that doesn't depend on order cpus are created/removed. Signed-off-by: Igor Mammedov Reported-by: David Gibson Reviewed-by: David Gibson Signed-off-by: Eduardo Habkost --- exec.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/exec.c b/exec.c index 2f57c624a5..ae45a707b6 100644 --- a/exec.c +++ b/exec.c @@ -643,13 +643,14 @@ void cpu_exec_exit(CPUState *cpu) CPUClass *cc = CPU_GET_CLASS(cpu); cpu_list_lock(); - if (cpu->cpu_index == -1) { - /* cpu_index was never allocated by this @cpu or was already freed. */ + if (cpu->node.tqe_prev == NULL) { + /* there is nothing to undo since cpu_exec_init() hasn't been called */ cpu_list_unlock(); return; } QTAILQ_REMOVE(&cpus, cpu, node); + cpu->node.tqe_prev = NULL; cpu_release_index(cpu); cpu->cpu_index = -1; cpu_list_unlock(); From a07f953ef4ef48058c24fb50b49e6fa28bf5f5f4 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:21 +0200 Subject: [PATCH 3/9] exec: Set cpu_index only if it's not been explictly set It keeps the legacy behavior for all users that doesn't care about stable cpu_index value, but would allow boards that would support device_add/device_del to set stable cpu_index that won't depend on order in which cpus are created/destroyed. While at that simplify cpu_get_free_index() as cpu_index generated by USER_ONLY and softmmu variants is the same since none of the users support cpu-remove so far, except of not yet released spapr/x86 device_add/delr, which will be altered by follow up patches to set stable cpu_index manually. Signed-off-by: Igor Mammedov Reviewed-by: David Gibson Signed-off-by: Eduardo Habkost --- exec.c | 44 ++++++-------------------------------------- include/qom/cpu.h | 2 ++ qom/cpu.c | 2 +- 3 files changed, 9 insertions(+), 39 deletions(-) diff --git a/exec.c b/exec.c index ae45a707b6..50e3ee237c 100644 --- a/exec.c +++ b/exec.c @@ -598,30 +598,7 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx) } #endif -#ifndef CONFIG_USER_ONLY -static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS); - -static int cpu_get_free_index(Error **errp) -{ - int cpu = find_first_zero_bit(cpu_index_map, MAX_CPUMASK_BITS); - - if (cpu >= MAX_CPUMASK_BITS) { - error_setg(errp, "Trying to use more CPUs than max of %d", - MAX_CPUMASK_BITS); - return -1; - } - - bitmap_set(cpu_index_map, cpu, 1); - return cpu; -} - -static void cpu_release_index(CPUState *cpu) -{ - bitmap_clear(cpu_index_map, cpu->cpu_index, 1); -} -#else - -static int cpu_get_free_index(Error **errp) +static int cpu_get_free_index(void) { CPUState *some_cpu; int cpu_index = 0; @@ -632,12 +609,6 @@ static int cpu_get_free_index(Error **errp) return cpu_index; } -static void cpu_release_index(CPUState *cpu) -{ - return; -} -#endif - void cpu_exec_exit(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); @@ -651,8 +622,7 @@ void cpu_exec_exit(CPUState *cpu) QTAILQ_REMOVE(&cpus, cpu, node); cpu->node.tqe_prev = NULL; - cpu_release_index(cpu); - cpu->cpu_index = -1; + cpu->cpu_index = UNASSIGNED_CPU_INDEX; cpu_list_unlock(); if (cc->vmsd != NULL) { @@ -666,7 +636,7 @@ void cpu_exec_exit(CPUState *cpu) void cpu_exec_init(CPUState *cpu, Error **errp) { CPUClass *cc ATTRIBUTE_UNUSED = CPU_GET_CLASS(cpu); - Error *local_err = NULL; + Error *local_err ATTRIBUTE_UNUSED = NULL; cpu->as = NULL; cpu->num_ases = 0; @@ -690,11 +660,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp) #endif cpu_list_lock(); - cpu->cpu_index = cpu_get_free_index(&local_err); - if (local_err) { - error_propagate(errp, local_err); - cpu_list_unlock(); - return; + if (cpu->cpu_index == UNASSIGNED_CPU_INDEX) { + cpu->cpu_index = cpu_get_free_index(); + assert(cpu->cpu_index != UNASSIGNED_CPU_INDEX); } QTAILQ_INSERT_TAIL(&cpus, cpu, node); cpu_list_unlock(); diff --git a/include/qom/cpu.h b/include/qom/cpu.h index cbcd64c92b..ce0c406f27 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -883,4 +883,6 @@ extern const struct VMStateDescription vmstate_cpu_common; .offset = 0, \ } +#define UNASSIGNED_CPU_INDEX -1 + #endif diff --git a/qom/cpu.c b/qom/cpu.c index 42b5631c7c..2553247907 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -340,7 +340,7 @@ static void cpu_common_initfn(Object *obj) CPUState *cpu = CPU(obj); CPUClass *cc = CPU_GET_CLASS(obj); - cpu->cpu_index = -1; + cpu->cpu_index = UNASSIGNED_CPU_INDEX; cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs; qemu_mutex_init(&cpu->work_mutex); QTAILQ_INIT(&cpu->breakpoints); From 69382d8b3e8600b349c191394d761dcb480502cf Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:22 +0200 Subject: [PATCH 4/9] qdev: Fix object reference leak in case device.realize() fails If device doesn't have parent assined before its realize is called, device_set_realized() will implicitly set parent to '/machine/unattached'. However device_set_realized() may fail after that point at several other points leaving not realized object dangling in '/machine/unattached' and as result caller of obj = object_new() obj->ref == 1 object_property_set_bool(obj,..., true, "realized",...) obj->ref == 2 if (fail) object_unref(obj); obj->ref == 1 will get object leak instead of expected object destruction. Fix it by making device_set_realized() to cleanup after itself in case of failure. Signed-off-by: Igor Mammedov Reviewed-by: David Gibson Signed-off-by: Eduardo Habkost --- hw/core/qdev.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 6680089154..ee4a083e64 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -885,6 +885,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp) HotplugHandler *hotplug_ctrl; BusState *bus; Error *local_err = NULL; + bool unattached_parent = false; + static int unattached_count; if (dev->hotplugged && !dc->hotpluggable) { error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj)); @@ -893,12 +895,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (value && !dev->realized) { if (!obj->parent) { - static int unattached_count; gchar *name = g_strdup_printf("device[%d]", unattached_count++); object_property_add_child(container_get(qdev_get_machine(), "/unattached"), name, obj, &error_abort); + unattached_parent = true; g_free(name); } @@ -987,6 +989,10 @@ post_realize_fail: fail: error_propagate(errp, local_err); + if (unattached_parent) { + object_unparent(OBJECT(dev)); + unattached_count--; + } } static bool device_get_hotpluggable(Object *obj, Error **errp) From a15d2728a9aadeb801370adf5bf0b411a774a2d2 Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:23 +0200 Subject: [PATCH 5/9] pc: Init CPUState->cpu_index with index in possible_cpus[] It will enshure that cpu_index for a given cpu stays the same regardless of the order cpus has been created/deleted. No compat code is needed as for initial cpus index in possible_cpus[] matches cpu_index that's been auto-allocated in cpu_exec_init(). Tha same applies for hotplug with cpu-add command if cpus are added sequentially in increasing order as 'id' matches cpu_index. If cpu-add had been used for creating out-of-order cpus, that created unmigratable instance since it were not possible to start target with the same cpu_index using old way of migrating instance with hotplugged cpus: * source QEMU with CLI (-smp 1,maxcpus=3 and cpu-add id=2) following set of cpu_index is allocated [0, 1] with apics set [0, 2] respectivelly * target QEMU is started with CLI -smp 2,maxcpus=3 resulting in set of cpu_index [0, 1] but with set of apics [0, 1] wich doesn't match source. So we don't need compat code in this case as it's never worked and newelly added device_add support would use stable cpu_index set by machine to begin with, so it won't have above limitation and source QEMU could be migrated to destination regardless of the order cpus were created. Signed-off-by: Igor Mammedov Reviewed-by: David Gibson Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- hw/i386/pc.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 9e3c70fb23..d6f034757f 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1875,6 +1875,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { int idx; + CPUState *cs; CPUArchId *cpu_slot; X86CPUTopoInfo topo; X86CPU *cpu = X86_CPU(dev); @@ -1975,6 +1976,9 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, return; } cpu->thread_id = topo.smt_id; + + cs = CPU(cpu); + cs->cpu_index = idx; } static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev, From 9527e7bde5b59005ddb2d902973915b81b4c5b2c Mon Sep 17 00:00:00 2001 From: Igor Mammedov Date: Mon, 25 Jul 2016 11:59:24 +0200 Subject: [PATCH 6/9] Revert "pc: Enforce adding CPUs contiguously and removing them in opposite order" This reverts commit 4da7faaeb0c7dd3f7f233165d336c878f78fd1eb. Since commit: pc: init CPUState->cpu_index with index in possible_cpus[] cpu_index is stable regardless of the order cpus were created and QEMU instance stays migratable always so limitation added by 4da7faaeb could be safely removed. Signed-off-by: Igor Mammedov Reviewed-by: Michael S. Tsirkin Signed-off-by: Eduardo Habkost --- hw/i386/pc.c | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d6f034757f..47593b741a 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1818,23 +1818,6 @@ static void pc_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, goto out; } - if (idx < pcms->possible_cpus->len - 1 && - pcms->possible_cpus->cpus[idx + 1].cpu != NULL) { - X86CPU *cpu; - - for (idx = pcms->possible_cpus->len - 1; - pcms->possible_cpus->cpus[idx].cpu == NULL; idx--) { - ;; - } - - cpu = X86_CPU(pcms->possible_cpus->cpus[idx].cpu); - error_setg(&local_err, "CPU [socket-id: %u, core-id: %u," - " thread-id: %u] should be removed first", - cpu->socket_id, cpu->core_id, cpu->thread_id); - goto out; - - } - hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); hhc->unplug_request(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); @@ -1932,23 +1915,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev, return; } - if (idx != 0 && pcms->possible_cpus->cpus[idx - 1].cpu == NULL) { - PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); - - for (idx = 1; pcms->possible_cpus->cpus[idx].cpu != NULL; idx++) { - ;; - } - - x86_topo_ids_from_apicid(pcms->possible_cpus->cpus[idx].arch_id, - smp_cores, smp_threads, &topo); - - if (!pcmc->legacy_cpu_hotplug) { - error_setg(errp, "CPU [socket: %u, core: %u, thread: %u] should be" - " added first", topo.pkg_id, topo.core_id, topo.smt_id); - return; - } - } - /* if 'address' properties socket-id/core-id/thread-id are not set, set them * so that query_hotpluggable_cpus would show correct values */ From 78a3930685c3159c4b8f953bd9f9e7a28655d823 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Wed, 13 Jul 2016 20:11:45 +0200 Subject: [PATCH 7/9] machine: Add comment to abort path in machine_set_kernel_irqchip We're not supposed to abort when the user passes a bogus value. Since the checking is done in visit_type_OnOffSplit(), the call to abort() is legitimate. Let's add a comment to make it explicit. Signed-off-by: Greg Kurz Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- hw/core/machine.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index 2fe6ff6f30..e5a456f21d 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -65,6 +65,9 @@ static void machine_set_kernel_irqchip(Object *obj, Visitor *v, ms->kernel_irqchip_split = true; break; default: + /* The value was checked in visit_type_OnOffSplit() above. If + * we get here, then something is wrong in QEMU. + */ abort(); } } From b3443f43f45e06971d87c985bb0316c1e40259c9 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Thu, 21 Jul 2016 23:58:37 +0200 Subject: [PATCH 8/9] qdev: ignore GlobalProperty.errp for hotplugged devices This patch ensures QEMU won't terminate while hotplugging a device if the global property cannot be set and errp points to error_fatal or error_abort. While here, it also fixes indentation of the typename argument. Suggested-by: Eduardo Habkost Signed-off-by: Greg Kurz Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- hw/core/qdev-properties.c | 4 ++-- include/hw/qdev-core.h | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 14e544ab17..311af6da76 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -1084,7 +1084,7 @@ int qdev_prop_check_globals(void) } static void qdev_prop_set_globals_for_type(DeviceState *dev, - const char *typename) + const char *typename) { GList *l; @@ -1100,7 +1100,7 @@ static void qdev_prop_set_globals_for_type(DeviceState *dev, if (err != NULL) { error_prepend(&err, "can't apply global %s.%s=%s: ", prop->driver, prop->property, prop->value); - if (prop->errp) { + if (!dev->hotplugged && prop->errp) { error_propagate(prop->errp, err); } else { assert(prop->user_provided); diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 1d1f8612a9..4b4b33bec8 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -261,7 +261,9 @@ struct PropertyInfo { * @used: Set to true if property was used when initializing a device. * @errp: Error destination, used like first argument of error_setg() * in case property setting fails later. If @errp is NULL, we - * print warnings instead of ignoring errors silently. + * print warnings instead of ignoring errors silently. For + * hotplugged devices, errp is always ignored and warnings are + * printed instead. */ typedef struct GlobalProperty { const char *driver; From 03f28efbbb0ee521611e0eb28b45096b3598fb34 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Fri, 22 Jul 2016 00:00:57 +0200 Subject: [PATCH 9/9] vl: exit if a bad property value is passed to -global When passing '-global driver=host-powerpc64-cpu,property=compat,value=foo' on the command line, without this patch, we get the following warning per device (which means many lines if the guests has many cpus): qemu-system-ppc64: Warning: can't apply global host-powerpc64-cpu.compat=foo: Invalid compatibility mode "foo" ... and QEMU continues execution, ignoring the property. With this patch, we get a single line: qemu-system-ppc64: can't apply global host-powerpc64-cpu.compat=foo: Invalid compatibility mode "foo" ... and QEMU exits. The previous behavior is kept for hotplugged devices since we don't want QEMU to exit when doing device_add. Reviewed-by: David Gibson Signed-off-by: Greg Kurz Reviewed-by: Eduardo Habkost Signed-off-by: Eduardo Habkost --- vl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/vl.c b/vl.c index a455947b4f..e7c2c628de 100644 --- a/vl.c +++ b/vl.c @@ -2922,6 +2922,7 @@ static int global_init_func(void *opaque, QemuOpts *opts, Error **errp) g->property = qemu_opt_get(opts, "property"); g->value = qemu_opt_get(opts, "value"); g->user_provided = true; + g->errp = &error_fatal; qdev_prop_register_global(g); return 0; }