From patchwork Wed Aug 30 17:05:56 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Hildenbrand X-Patchwork-Id: 807825 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3xjClC3ZlDz9s8w for ; Thu, 31 Aug 2017 03:52:39 +1000 (AEST) Received: from localhost ([::1]:51927 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dn7AL-0006CW-75 for incoming@patchwork.ozlabs.org; Wed, 30 Aug 2017 13:52:37 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33408) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dn6Rn-0005kV-GK for qemu-devel@nongnu.org; Wed, 30 Aug 2017 13:06:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dn6Rl-0002Tb-FT for qemu-devel@nongnu.org; Wed, 30 Aug 2017 13:06:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46812) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dn6Rl-0002Sd-79 for qemu-devel@nongnu.org; Wed, 30 Aug 2017 13:06:33 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 48BE07E432; Wed, 30 Aug 2017 17:06:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 48BE07E432 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=david@redhat.com Received: from t460s.redhat.com (ovpn-116-98.ams2.redhat.com [10.36.116.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4A44480E72; Wed, 30 Aug 2017 17:06:30 +0000 (UTC) From: David Hildenbrand To: qemu-devel@nongnu.org Date: Wed, 30 Aug 2017 19:05:56 +0200 Message-Id: <20170830170601.15855-7-david@redhat.com> In-Reply-To: <20170830170601.15855-1-david@redhat.com> References: <20170830170601.15855-1-david@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 30 Aug 2017 17:06:32 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v1 06/11] target/s390x: cleanup cpu number/address handling X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: thuth@redhat.com, david@redhat.com, cohuck@redhat.com, Richard Henderson , Alexander Graf , borntraeger@de.ibm.com, Aurelien Jarno Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" Some time ago we discussed that using "id" as property name is not the right thing to do, as it is a reserved property for other devices. Switch to the term "addr" instead, which matches the definition in the PoP called "CPU address". There is no such thing as cpu number, so rename env.cpu_num to env.cpu_addr. We can get rid of cpu->id now. Keep cpu->index and env->cpu_addr in sync. cpu->index was already implicitly used by e.g. cpu_exists(), so keeping both in sync seems to be the right thing to do. cpu->index will now no longer automatically get set via cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed in sync. Our new cpu property "addr" can be a static property. Range checks can be avoided by using the correct type and the "setting after realized" check is done implicitly. AFAIK, s390x only supports cpu_add and not device_add for cpus. So we should be able to safely rename that property (no the "id" property could properly be used for device_add, which needs an artificial id for identification purposes). Signed-off-by: David Hildenbrand --- hw/s390x/s390-virtio-ccw.c | 2 +- target/s390x/cpu.c | 69 ++++++++++++---------------------------------- target/s390x/cpu.h | 5 ++-- target/s390x/cpu_models.c | 2 +- target/s390x/excp_helper.c | 2 +- target/s390x/helper.c | 4 +-- target/s390x/misc_helper.c | 4 +-- target/s390x/translate.c | 5 +--- 8 files changed, 28 insertions(+), 65 deletions(-) diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 03c88a524b..7754e3eaf9 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -306,7 +306,7 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev, S390CPU *cpu = S390_CPU(dev); CPUState *cs = CPU(dev); - name = g_strdup_printf("cpu[%i]", cpu->env.cpu_num); + name = g_strdup_printf("cpu[%i]", cpu->env.cpu_addr); object_property_set_link(OBJECT(hotplug_dev), OBJECT(cs), name, errp); g_free(name); diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 7267b60d41..156589e921 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -36,6 +36,7 @@ #include "trace.h" #include "qapi/visitor.h" #include "exec/exec-all.h" +#include "hw/qdev-properties.h" #ifndef CONFIG_USER_ONLY #include "hw/hw.h" #include "sysemu/arch_init.h" @@ -189,24 +190,26 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) } #if !defined(CONFIG_USER_ONLY) - if (cpu->id >= max_cpus) { - error_setg(&err, "Unable to add CPU: %" PRIi64 - ", max allowed: %d", cpu->id, max_cpus - 1); + if (cpu->env.cpu_addr >= max_cpus) { + error_setg(&err, "Unable to add CPU: %" PRIu32 ", max allowed: %d", + cpu->env.cpu_addr, max_cpus - 1); goto out; } #endif - if (cpu_exists(cpu->id)) { - error_setg(&err, "Unable to add CPU: %" PRIi64 - ", it already exists", cpu->id); + if (cpu_exists(cpu->env.cpu_addr)) { + error_setg(&err, "Unable to add CPU: %" PRIu32 ", it already exists", + cpu->env.cpu_addr); goto out; } - if (cpu->id != scc->next_cpu_id) { - error_setg(&err, "Unable to add CPU: %" PRIi64 - ", The next available id is %" PRIi64, cpu->id, + if (cpu->env.cpu_addr != scc->next_cpu_id) { + error_setg(&err, "Unable to add CPU: %" PRIu32 + ", the next available nr is %" PRIi64, cpu->env.cpu_addr, scc->next_cpu_id); goto out; } + /* sync cs->cpu_index and env->cpu_addr. The latter is needed for TCG. */ + cs->cpu_index = env->cpu_addr; cpu_exec_realizefn(cs, &err); if (err != NULL) { goto out; @@ -216,7 +219,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) #if !defined(CONFIG_USER_ONLY) qemu_register_reset(s390_cpu_machine_reset_cb, cpu); #endif - env->cpu_num = cpu->id; s390_cpu_gdb_init(cs); qemu_init_vcpu(cs); #if !defined(CONFIG_USER_ONLY) @@ -237,45 +239,6 @@ out: error_propagate(errp, err); } -static void s390x_cpu_get_id(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ - S390CPU *cpu = S390_CPU(obj); - int64_t value = cpu->id; - - visit_type_int(v, name, &value, errp); -} - -static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ - S390CPU *cpu = S390_CPU(obj); - DeviceState *dev = DEVICE(obj); - const int64_t min = 0; - const int64_t max = UINT32_MAX; - Error *err = NULL; - int64_t value; - - if (dev->realized) { - error_setg(errp, "Attempt to set property '%s' on '%s' after " - "it was realized", name, object_get_typename(obj)); - return; - } - - visit_type_int(v, name, &value, &err); - if (err) { - error_propagate(errp, err); - return; - } - if (value < min || value > max) { - error_setg(errp, "Property %s.%s doesn't take value %" PRId64 - " (minimum: %" PRId64 ", maximum: %" PRId64 ")" , - object_get_typename(obj), name, value, min, max); - return; - } - cpu->id = value; -} - static void s390_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -289,8 +252,6 @@ static void s390_cpu_initfn(Object *obj) cs->env_ptr = env; cs->halted = 1; cs->exception_index = EXCP_HLT; - object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id, - s390x_cpu_set_id, NULL, NULL, NULL); s390_cpu_model_register_props(obj); #if !defined(CONFIG_USER_ONLY) qemu_get_timedate(&tm, 0); @@ -487,6 +448,11 @@ static gchar *s390_gdb_arch_name(CPUState *cs) return g_strdup("s390:64-bit"); } +static Property s390x_cpu_properties[] = { + DEFINE_PROP_UINT32("addr", S390CPU, env.cpu_addr, 0), + DEFINE_PROP_END_OF_LIST() +}; + static void s390_cpu_class_init(ObjectClass *oc, void *data) { S390CPUClass *scc = S390_CPU_CLASS(oc); @@ -496,6 +462,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) scc->next_cpu_id = 0; scc->parent_realize = dc->realize; dc->realize = s390_cpu_realizefn; + dc->props = s390x_cpu_properties; scc->parent_reset = cc->reset; #if !defined(CONFIG_USER_ONLY) diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 4ec338077e..6766dbb579 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -149,7 +149,7 @@ typedef struct CPUS390XState { CPU_COMMON - uint32_t cpu_num; + uint32_t cpu_addr; /* same as cpu->index */ uint64_t cpuid; uint64_t tod_offset; @@ -193,7 +193,6 @@ struct S390CPU { /*< public >*/ CPUS390XState env; - int64_t id; S390CPUModel *model; /* needed for live migration */ void *irqstate; @@ -690,7 +689,7 @@ const char *s390_default_cpu_model_name(void); /* helper.c */ S390CPU *cpu_s390x_init(const char *cpu_model); #define cpu_init(model) CPU(cpu_s390x_init(model)) -S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp); +S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t addr, Error **errp); /* you can call this signal handler from your SIGBUS and SIGSEGV signal handlers to inform the virtual CPU of exceptions. non zero is returned if the signal was handled by the virtual CPU. */ diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 18cbf91d9c..1954c64c86 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -915,7 +915,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp) cpu->env.cpuid = s390_cpuid_from_cpu_model(cpu->model); if (tcg_enabled()) { /* basic mode, write the cpu address into the first 4 bit of the ID */ - cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.cpu_num); + cpu->env.cpuid = deposit64(cpu->env.cpuid, 54, 4, cpu->env.cpu_addr); } } diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c index 361f970db3..2ac36535f7 100644 --- a/target/s390x/excp_helper.c +++ b/target/s390x/excp_helper.c @@ -251,7 +251,7 @@ static void do_ext_interrupt(CPUS390XState *env) lowcore->ext_params2 = cpu_to_be64(q->param64); lowcore->external_old_psw.mask = cpu_to_be64(get_psw_mask(env)); lowcore->external_old_psw.addr = cpu_to_be64(env->psw.addr); - lowcore->cpu_addr = cpu_to_be16(env->cpu_num | VIRTIO_SUBCODE_64); + lowcore->cpu_addr = cpu_to_be16(env->cpu_addr | VIRTIO_SUBCODE_64); mask = be64_to_cpu(lowcore->external_new_psw.mask); addr = be64_to_cpu(lowcore->external_new_psw.addr); diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 3adb9de122..70d1ea8cf6 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -104,7 +104,7 @@ S390CPU *cpu_s390x_create(const char *cpu_model, Error **errp) return S390_CPU(CPU(object_new(typename))); } -S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp) +S390CPU *s390x_new_cpu(const char *cpu_model, uint32_t addr, Error **errp) { S390CPU *cpu; Error *err = NULL; @@ -114,7 +114,7 @@ S390CPU *s390x_new_cpu(const char *cpu_model, int64_t id, Error **errp) goto out; } - object_property_set_int(OBJECT(cpu), id, "id", &err); + object_property_set_int(OBJECT(cpu), addr, "addr", &err); if (err != NULL) { goto out; } diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c index 50cc046ca2..eb7accc0ce 100644 --- a/target/s390x/misc_helper.c +++ b/target/s390x/misc_helper.c @@ -230,7 +230,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, /* XXX make different for different CPUs? */ ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16); ebcdic_put(sysib.plant, "QEMU", 4); - stw_p(&sysib.cpu_addr, env->cpu_num); + stw_p(&sysib.cpu_addr, env->cpu_addr); cpu_physical_memory_write(a0, &sysib, sizeof(sysib)); } else if ((sel1 == 2) && (sel2 == 2)) { /* Basic Machine CPUs */ @@ -258,7 +258,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0, /* XXX make different for different CPUs? */ ebcdic_put(sysib.sequence, "QEMUQEMUQEMUQEMU", 16); ebcdic_put(sysib.plant, "QEMU", 4); - stw_p(&sysib.cpu_addr, env->cpu_num); + stw_p(&sysib.cpu_addr, env->cpu_addr); stw_p(&sysib.cpu_id, 0); cpu_physical_memory_write(a0, &sysib, sizeof(sysib)); } else if ((sel1 == 2) && (sel2 == 2)) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 4b0db7b7bd..80455bf8f0 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o) static ExitStatus op_stap(DisasContext *s, DisasOps *o) { check_privileged(s); - /* ??? Surely cpu address != cpu number. In any case the previous - version of this stored more than the required half-word, so it - is unlikely this has ever been tested. */ - tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num)); + tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_addr)); return NO_EXIT; }