Thanks Alex comments inline....
On Tue, Jan 20, 2015 at 8:19 AM, Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Greg Bellows <greg.bell...@linaro.org> writes:
>
> > Adds a CPU feature parsing function and assigns to the CPU class. The
> only
> > feature added was "-aarch64" which disabled the AArch64 execution state
> on a
> > 64-bit ARM CPU.
> >
> > 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>
> > ---
> > target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 285947f..f327dd7 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);
>
> Aren't we leaking here? strtok returns the next token (or NULL) so don't
> we loose the original ptr?
>
>
As I understand it, strtok uses static pointers to track the location
within an existing string rather than allocating storage that the user must
free. This is apparently what makes the version I used non-reentrant. In
which case, there should not be an leak due to its use.
> Also while using glib you might want to consider using glib's own
> tokenising functions (e.g. g_strsplit). This has the advantage of having
> helper functions like g_strfreev() which can clean-up everything in one go.
>
I certainly can use the glib version, but in this case I did not see the
advantage. In fact, it actually may be less performant to use the glib
version given it needs to allocate/free memory. I am fine either way if
anyone feels strongly.
>
> > if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
> > object_class_is_abstract(oc)) {
> > @@ -1163,6 +1167,44 @@ static Property arm_cpu_properties[] = {
> > DEFINE_PROP_END_OF_LIST()
> > };
> >
> > +static void arm_cpu_parse_features(CPUState *cs, char *features,
> > + Error **errp)
> > +{
> > + ARMCPU *cpu = ARM_CPU(cs);
> > + char *featurestr;
> > +
> > + featurestr = features ? strtok(features, ",") : NULL;
> > + while (featurestr) {
> > + if (featurestr[0] == '-') {
> > + if (!strcmp(featurestr+1, "aarch64")) {
> > + /* If AArch64 is disabled then we need to unset the
> feature */
> > + unset_feature(&cpu->env, ARM_FEATURE_AARCH64);
> > + } else {
> > + /* Everyting else is unsupported */
> > + error_setg(errp, "unsupported CPU property '%s'",
> > + &featurestr[1]);
> > + return;
> > + }
> > + } else if (featurestr[0] == '+') {
> > + /* No '+' properties supported yet */
> > + error_setg(errp, "unsupported CPU property '%s'",
> > + &featurestr[1]);
> > + return;
> > + } else if (g_strstr_len(featurestr, -1, "=")) {
> > + /* No '=' properties supported yet */
> > + char *prop = strtok(featurestr, "=");
> > + error_setg(errp, "unsupported CPU property '%s'", prop);
> > + return;
> > + } else {
> > + /* Everything else is a bad format */
> > + error_setg(errp, "CPU property string '%s' not in format "
> > + "(+feature|-feature|feature=xyz)",
> featurestr);
> > + return;
> > + }
> > + featurestr = strtok(NULL, ",");
> > + }
> > +}
>
> I only point to this for reference to a "gliby" approach to the parsing,
> relative beauty being in the eye of the beholder ;-)
>
>
> https://github.com/stsquad/qemu/commit/86bc88f661141b93cbe5b107c4d5b4322b563241#diff-286aa0f2c1f0d862c4197781280a92efR116
>
> It does make me think boilerplate but I wonder if this is a generic
> enough problem to be solved more generally in QEMU?
>
If we were to go with a boilerplate approach then it would make sense to
follow the mechanism used for machine properties. However, this was how
other arch were doing the CPU props, so we stuck to this approach. It does
look very similar to the code you pointed at, but I think that the
conditional piece looking for {+-=} would bt the only common piece once you
add unique options and handling.
>
> > +
> > static void arm_cpu_class_init(ObjectClass *oc, void *data)
> > {
> > ARMCPUClass *acc = ARM_CPU_CLASS(oc);
> > @@ -1183,6 +1225,7 @@ static void arm_cpu_class_init(ObjectClass *oc,
> void *data)
> > cc->set_pc = arm_cpu_set_pc;
> > cc->gdb_read_register = arm_cpu_gdb_read_register;
> > cc->gdb_write_register = arm_cpu_gdb_write_register;
> > + cc->parse_features = arm_cpu_parse_features;
> > #ifdef CONFIG_USER_ONLY
> > cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
> > #else
>
> --
> Alex Bennée
>
