| Message ID | 1422403117-16921-2-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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 <greg.bellows@linaro.org> > > --- > > 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
On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote: >> 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. [I meant "-cpu cpuname,aarch64=off", hadn't processed that we've updated to the new style syntax.] I had a think about this as I was cycling home, and I (re)convinced myself that modelling this as "remove the AArch64 feature flag" is the right way to do it, which then just leaves us with "what's the best user-facing UI we can manage to expose that?". Suppose that we had two properties/feature flag switches that do the same thing but have inverse sense: aarch64=off aarch32-only=on Would that help, or just be more confusing? -- PMM
On Tue, Feb 3, 2015 at 10:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> wrote: >>> 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. > > [I meant "-cpu cpuname,aarch64=off", hadn't processed that we've > updated to the new style syntax.] > > I had a think about this as I was cycling home, and I (re)convinced > myself that modelling this as "remove the AArch64 feature flag" is > the right way to do it, which then just leaves us with "what's the > best user-facing UI we can manage to expose that?". > > Suppose that we had two properties/feature flag switches that do > the same thing but have inverse sense: > > aarch64=off > aarch32-only=on > > Would that help, or just be more confusing? > that would help, imho. -Christoffer
On Tue, Feb 3, 2015 at 3:21 PM, Christoffer Dall < christoffer.dall@linaro.org> wrote: > On Tue, Feb 3, 2015 at 10:15 PM, Peter Maydell <peter.maydell@linaro.org> > wrote: > > On 3 February 2015 at 19:14, Peter Maydell <peter.maydell@linaro.org> > wrote: > >> On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> > wrote: > >>> 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. > > > > [I meant "-cpu cpuname,aarch64=off", hadn't processed that we've > > updated to the new style syntax.] > > > > I had a think about this as I was cycling home, and I (re)convinced > > myself that modelling this as "remove the AArch64 feature flag" is > > the right way to do it, which then just leaves us with "what's the > > best user-facing UI we can manage to expose that?". > > > > Suppose that we had two properties/feature flag switches that do > > the same thing but have inverse sense: > > > > aarch64=off > > aarch32-only=on > > > > Would that help, or just be more confusing? > > > that would help, imho. > > Hmmm..., while it is trivial to add the second option I think it would be confusing for users and begs for issues of having both specified. If we prefer the second name lets go with it otherwise my vote is against having two. If the behavior of the flag is "remove the AArch64 flag" it seems most natural to stick with what we have as it conveys just that. The other option seems counter-intuitive. > -Christoffer >
On Tue, Feb 3, 2015 at 1:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 January 2015 at 23:58, Greg Bellows <greg.bellows@linaro.org> 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 <greg.bellows@linaro.org> > > > > --- > > > > 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. > > Sure, I'll change it. > > + 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 >
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, ","); + 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); } static void aarch64_cpu_finalizefn(Object *obj)
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 <greg.bellows@linaro.org> --- 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(-)