| Message ID | 1421706621-23731-2-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
Greg Bellows <greg.bellows@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.bellows@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? 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. > 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? > + > 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
Thanks Alex comments inline.... On Tue, Jan 20, 2015 at 8:19 AM, Alex Bennée <alex.bennee@linaro.org> wrote: > > Greg Bellows <greg.bellows@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.bellows@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 >
On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote: > Please do not use legacy +-feature format and support only foo=val format. > Other targets have it only for to being able support legacy setups > which use +- format. I thought this was the standard format for CPU features. Do you have an example of a CPU feature being set using foo=val format? -- PMM
On Tue, Jan 20, 2015 at 9:22 AM, Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 19 Jan 2015 16:30:17 -0600 > Greg Bellows <greg.bellows@linaro.org> wrote: > > > 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.bellows@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); > > 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] == '-') { > ... > > + } else if (featurestr[0] == '+') { > Please do not use legacy +-feature format and support only foo=val format. > Other targets have it only for to being able support legacy setups > which use +- format. > > Thanks Igor. I was under the impression that the +/- notation was still relevant. Perhaps it makes the most sense to convert to using object properties similar to how machine options are specified? What do you think Peter? > > + /* 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, ","); > > + } > > +} > > + > > 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 > >
On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 20 Jan 2015 15:34:23 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote: >> > Please do not use legacy +-feature format and support only foo=val format. >> > Other targets have it only for to being able support legacy setups >> > which use +- format. >> >> I thought this was the standard format for CPU features. Do you >> have an example of a CPU feature being set using foo=val format? > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and > in addition to it we ca use canonized format for generic properties > like, foo1=on,foo2=off,foo3=on > > We try to move out of legacy format, so that it would be possible > to reuse generic property parsing infrastructure like with any > device object. That would allow to use -device/device_add for CPUs. -device/-device_add for CPUs is pretty fraught in the general case because there's no obvious place to plug them and have them be wired up properly. You'd need to use -global for CPU properties, which is a nightmare... Anyway, I'm not particularly attached to the exact command line syntax we've used here -- I was just looking for "we have a CPU property, and use the same syntax for specifying CPU feature enable/disable that other CPUs do"... -- PMM
On Tue, Jan 20, 2015 at 10:25 AM, Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 20 Jan 2015 16:08:09 +0000 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 20 January 2015 at 15:59, Igor Mammedov <imammedo@redhat.com> wrote: >> > On Tue, 20 Jan 2015 15:34:23 +0000 >> > Peter Maydell <peter.maydell@linaro.org> wrote: >> > >> >> On 20 January 2015 at 15:22, Igor Mammedov <imammedo@redhat.com> wrote: >> >> > Please do not use legacy +-feature format and support only foo=val format. >> >> > Other targets have it only for to being able support legacy setups >> >> > which use +- format. >> >> >> >> I thought this was the standard format for CPU features. Do you >> >> have an example of a CPU feature being set using foo=val format? >> > Currently on x86 we can use either legacy +foo1,-foo2,foo3 and >> > in addition to it we ca use canonized format for generic properties >> > like, foo1=on,foo2=off,foo3=on >> > >> > We try to move out of legacy format, so that it would be possible >> > to reuse generic property parsing infrastructure like with any >> > device object. That would allow to use -device/device_add for CPUs. >> >> -device/-device_add for CPUs is pretty fraught in the general >> case because there's no obvious place to plug them and have >> them be wired up properly. > That depends on CPU of-cause, but we are close to having device_add > working with x86 CPUs. > >> You'd need to use -global for CPU >> properties, which is a nightmare... > mine thoughts on it were that '-cpu type,feat...' would eventually > do conversion of features to global properties transparently for > user using target specific cc->parse_features() callback. Which > Greg could actually do here. We would happy to reuse it with x86 CPUs. > >> >> Anyway, I'm not particularly attached to the exact command >> line syntax we've used here -- I was just looking for "we have >> a CPU property, and use the same syntax for specifying CPU >> feature enable/disable that other CPUs do"... > Then '-cpu arm_foo,featX=on/off' should do the job. > > >> >> -- PMM > For now I went with the "-cpu arm_foo,aarch64=off" approach so as to not complicate it right now with adding full-blown CPU properties.
Greg Bellows <greg.bellows@linaro.org> writes: > Thanks Alex comments inline.... > <snip> >> >> 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. Yeah - I realised this after re-reading the man page. Non-re-entrant isn't a particular problem these days but it still feels dirty.... >> 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. I suspect this discussion is trumped by moving to the feat_foo=on/off parsing style referenced elsewhere so we can use common code.
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); 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, ","); + } +} + 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
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.bellows@linaro.org> --- target-arm/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)