From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41476) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIivg-0002mX-2k for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:14:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIivd-0007PP-9S for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:14:32 -0500 Received: from mail-lb0-f181.google.com ([209.85.217.181]:48123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIivd-0007Ok-2Y for qemu-devel@nongnu.org; Tue, 03 Feb 2015 14:14:29 -0500 Received: by mail-lb0-f181.google.com with SMTP id u10so40442862lbd.12 for ; Tue, 03 Feb 2015 11:14:28 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1422403117-16921-2-git-send-email-greg.bellows@linaro.org> References: <1422403117-16921-1-git-send-email-greg.bellows@linaro.org> <1422403117-16921-2-git-send-email-greg.bellows@linaro.org> From: Peter Maydell Date: Tue, 3 Feb 2015 19:14:08 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v3 1/4] target-arm: Add CPU property to disable AArch64 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Bellows Cc: =?UTF-8?B?QWxleCBCZW5uw6ll?= , QEMU Developers , Christoffer Dall On 27 January 2015 at 23:58, Greg Bellows wrote: > Adds registration and get/set functions for enabling/disabling the AArch64 > execution state on AArch64 CPUs. By default AArch64 execution state is enabled > on AArch64 CPUs, setting the property to off, will disable the execution state. > The below QEMU invocation would have AArch64 execution state disabled. > > $ ./qemu-system-aarch64 -machine virt -cpu cortex-a57,aarch64=off > > Also adds stripping of features from CPU model string in acquiring the ARM CPU > by name. > > Signed-off-by: Greg Bellows > > --- > > v1 -> v2 > - Scrap the custom CPU feature parsing in favor of using the default CPU > parsing. > - Add registration of CPU AArch64 property to disable/enable the AArch64 > feature. > --- > target-arm/cpu.c | 6 +++++- > target-arm/cpu64.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 1 deletion(-) > > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 285947f..29ed691 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -514,13 +514,17 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) > { > ObjectClass *oc; > char *typename; > + char *cpuname; > > if (!cpu_model) { > return NULL; > } > > - typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpu_model); > + cpuname = g_strdup(cpu_model); > + cpuname = strtok(cpuname, ","); strtok isn't thread safe. Let's just use g_strsplit like we did in virt.c and then we don't have to worry about whether or not multiple threads could ever end up here at the same time. > + typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname); > oc = object_class_by_name(typename); > + g_free(cpuname); > g_free(typename); > if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) || > object_class_is_abstract(oc)) { > diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c > index bb778b3..5a59280 100644 > --- a/target-arm/cpu64.c > +++ b/target-arm/cpu64.c > @@ -32,6 +32,11 @@ static inline void set_feature(CPUARMState *env, int feature) > env->features |= 1ULL << feature; > } > > +static inline void unset_feature(CPUARMState *env, int feature) > +{ > + env->features &= ~(1ULL << feature); > +} > + > #ifndef CONFIG_USER_ONLY > static uint64_t a57_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > @@ -170,8 +175,32 @@ static const ARMCPUInfo aarch64_cpus[] = { > { .name = NULL } > }; > > +static bool aarch64_cpu_get_aarch64(Object *obj, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + return arm_feature(&cpu->env, ARM_FEATURE_AARCH64); > +} > + > +static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + > + if (value == false) { > + unset_feature(&cpu->env, ARM_FEATURE_AARCH64); > + } else { > + set_feature(&cpu->env, ARM_FEATURE_AARCH64); > + } > +} > + > static void aarch64_cpu_initfn(Object *obj) > { > + object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64, > + aarch64_cpu_set_aarch64, NULL); > + object_property_set_description(obj, "aarch64", > + "Set on/off to enable/disable aarch64 " > + "execution state ", > + NULL); > } This all looks OK code-wise. Still need to think about whether we can manage to end up with a nicer interface to the user than cpuname,-aarch64, though. -- PMM