On Tue, Feb 10, 2015 at 10:03 PM, Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On 10 February 2015 at 10:50, Greg Bellows <greg.bell...@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.bell...@linaro.org>
> >
> > ---
> >
> > v3 -> v4
> > - Switch from using strtok to g_strsplit
> > - Add disablement of aarch64 option if KVM is not enabled.
> >
> > 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 | 5 ++++-
> > target-arm/cpu64.c | 39 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index d38af74..986f04c 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -544,13 +544,16 @@ 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_strsplit(cpu_model, ",", 1);
> > + typename = g_strdup_printf("%s-" TYPE_ARM_CPU, cpuname[0]);
> > oc = object_class_by_name(typename);
> > + g_strfreev(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..d03f203 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,42 @@ 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);
> > +
> > + /* At this time, this property is only allowed if KVM is enabled.
> This
> > + * restriction allows us to avoid fixing up functionality that
> assumes a
> > + * uniform execution state like do_interrupt.
> > + */
> > + if (!kvm_enabled()) {
> > + error_setg(errp,
> > + "'aarch64' option only supported with
> '-enable-kvm'");
> > + return;
>
> This check needs to go in the "we're unsetting the feature bit"
> code path, and we could make the error message a little clearer:
> "'aarch64' feature cannot be disabled unless KVM is enabled"
> (setting the feature to on is harmless and will work with TCG :-))
>
> Although it may be benign, the user may be doing something they think may
have an effect which is why I catch any case (not just off). I see this as
being cleaner.
As far as the message, the user does not really know about an "aarch64"
feature, that is internal to QEMU. Given this is a command line option
error, I believe it makes more sense to keep it in that domain. The
message as is describes the error in terms the command line options. Maybe
a compromise such as below would work:
"aarch64 execution state can only be disabled when enabling KVM using the
'-enable-kvm' option
> > + }
> > +
> > + 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);
>
> "Set on/off to enable/disable support for AArch64 execution
> state. Disable this to boot 32-bit guests in AArch32 state."
>
I'll add suggested wording.
>
> (Is that space at the end of your description deliberate or
> accidental?)
>
Space is inadvertant, I'll remove.
>
> Otherwise, Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
>
> -- PMM
>
