Message ID | 1355760092-18755-11-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > It prepares for converting "+feature,-feature,feature=foo,feature" into > a set of key,value property pairs that will be applied to CPU by > cpu_x86_set_props(). > > Each feature handled by cpu_x86_parse_featurestr() will be converted into > foo,val pair and a corresponding property setter by following patches. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU object as parameter and set the properties directly? Or you can see some use case where saving the property-setting dictionary for later use will be useful? (This will probably require calling cpudef_2_x86_cpu() before cpu_x86_parse_featurestr(), and changing the existing cpu_x86_parse_featurestr() code to change the cpu object, not a x86_def_t struct, but I think this will simplify the logic a lot) > --- > target-i386/cpu.c | 33 +++++++++++++++++++++++++++------ > 1 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index e075b59..a74d74b 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > return 0; > } > > +/* Set features on X86CPU object based on a provide key,value list */ > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp) > +{ > + const QDictEntry *ent; > + > + for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) { > + const QString *qval = qobject_to_qstring(qdict_entry_value(ent)); > + object_property_parse(OBJECT(cpu), qstring_get_str(qval), > + qdict_entry_key(ent), errp); > + if (error_is_set(errp)) { > + return; > + } > + } > +} > + > /* Parse "+feature,-feature,feature=foo" CPU feature string > */ > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > + QDict **props) > { > unsigned int i; > char *featurestr; /* Single 'key=value" string being parsed */ > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > uint32_t minus_kvm_features = 0, minus_svm_features = 0; > uint32_t minus_7_0_ebx_features = 0; > uint32_t numvalue; > + gchar **feat_array = g_strsplit(features ? features : "", ",", 0); > + *props = qdict_new(); > + int j = 0; > > - featurestr = features ? strtok(features, ",") : NULL; > - > - while (featurestr) { > + while ((featurestr = feat_array[j++])) { > char *val; > if (featurestr[0] == '+') { > add_flagname_to_bitmaps(featurestr + 1, &plus_features, > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); > goto error; > } > - featurestr = strtok(NULL, ","); > } > x86_cpu_def->features |= plus_features; > x86_cpu_def->ext_features |= plus_ext_features; > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > x86_cpu_def->kvm_features &= ~minus_kvm_features; > x86_cpu_def->svm_features &= ~minus_svm_features; > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > + g_strfreev(feat_array); > return 0; > > error: > + g_strfreev(feat_array); > return -1; > } > > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu) > int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > { > x86_def_t def1, *def = &def1; > + QDict *props = NULL; > Error *error = NULL; > char *name, *features; > gchar **model_pieces; > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > &def->ext3_features, &def->kvm_features, > &def->svm_features, &def->cpuid_7_0_ebx_features); > > - if (cpu_x86_parse_featurestr(def, features) < 0) { > + if (cpu_x86_parse_featurestr(def, features, &props) < 0) { > error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); > goto out; > } > > cpudef_2_x86_cpu(cpu, def, &error); > + cpu_x86_set_props(cpu, props, &error); > > out: > + QDECREF(props); > g_strfreev(model_pieces); > if (error) { > fprintf(stderr, "%s\n", error_get_pretty(error)); > -- > 1.7.1 > >
On Wed, 19 Dec 2012 14:54:30 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > > It prepares for converting "+feature,-feature,feature=foo,feature" into > > a set of key,value property pairs that will be applied to CPU by > > cpu_x86_set_props(). > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted into > > foo,val pair and a corresponding property setter by following patches. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU > object as parameter and set the properties directly? Or you can see some > use case where saving the property-setting dictionary for later use will > be useful? I plan to use cpu_x86_parse_featurestr() + list of properties later when we have properties and subclasses in place. Then it would be possible to transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of global properties. In the end the option handler for -cpu XXX,... could be changed into: cpu_opt_parser() { // hides legacy target specific ugliness target_XXX_cpu_compat_parser_callback() { cpu_class_name = get_class_name(optval) // dumb compatibility parser, which converts old format into // canonical form feature,val property list prop_list = parse_featurestr(optval) // with classes and global properties we could get rid of the field // cpu_model_str in CPUxxxState return prop_list, cpu_class_name } foreach (prop_list) add_global_prop(cpu_class_name,prop,val) // could be later transformed to property of board object and // we could get rid of one more global var cpu_model = cpu_class_name } > > (This will probably require calling cpudef_2_x86_cpu() before > cpu_x86_parse_featurestr(), and changing the existing > cpu_x86_parse_featurestr() code to change the cpu object, not a > x86_def_t struct, but I think this will simplify the logic a lot) You cannot set/uset +-feat directly on CPU without breaking current behavior where -feat overrides all +feat no matter in what order they are specified. That's why dictionary is used to easily maintain "if(not feat) ignore" logic and avoid duplication. Pls, look at commit https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3 that converts +-feat into canonical feat=on/off form. And if features are applied directly to CPU, it would require to another rewrite if global properties are to be used. Which IMHO should be eventually done since -cpu ... looks like global parameters for a specific cpu type. > > > --- > > target-i386/cpu.c | 33 +++++++++++++++++++++++++++------ > > 1 files changed, 27 insertions(+), 6 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index e075b59..a74d74b 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > > return 0; > > } > > > > +/* Set features on X86CPU object based on a provide key,value list */ > > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp) > > +{ > > + const QDictEntry *ent; > > + > > + for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) { > > + const QString *qval = qobject_to_qstring(qdict_entry_value(ent)); > > + object_property_parse(OBJECT(cpu), qstring_get_str(qval), > > + qdict_entry_key(ent), errp); > > + if (error_is_set(errp)) { > > + return; > > + } > > + } > > +} > > + > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > */ > > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > > + QDict **props) > > { > > unsigned int i; > > char *featurestr; /* Single 'key=value" string being parsed */ > > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > uint32_t minus_kvm_features = 0, minus_svm_features = 0; > > uint32_t minus_7_0_ebx_features = 0; > > uint32_t numvalue; > > + gchar **feat_array = g_strsplit(features ? features : "", ",", 0); > > + *props = qdict_new(); > > + int j = 0; > > > > - featurestr = features ? strtok(features, ",") : NULL; > > - > > - while (featurestr) { > > + while ((featurestr = feat_array[j++])) { > > char *val; > > if (featurestr[0] == '+') { > > add_flagname_to_bitmaps(featurestr + 1, &plus_features, > > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); > > goto error; > > } > > - featurestr = strtok(NULL, ","); > > } > > x86_cpu_def->features |= plus_features; > > x86_cpu_def->ext_features |= plus_ext_features; > > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > x86_cpu_def->svm_features &= ~minus_svm_features; > > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > > + g_strfreev(feat_array); > > return 0; > > > > error: > > + g_strfreev(feat_array); > > return -1; > > } > > > > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu) > > int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > { > > x86_def_t def1, *def = &def1; > > + QDict *props = NULL; > > Error *error = NULL; > > char *name, *features; > > gchar **model_pieces; > > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > &def->ext3_features, &def->kvm_features, > > &def->svm_features, &def->cpuid_7_0_ebx_features); > > > > - if (cpu_x86_parse_featurestr(def, features) < 0) { > > + if (cpu_x86_parse_featurestr(def, features, &props) < 0) { > > error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); > > goto out; > > } > > > > cpudef_2_x86_cpu(cpu, def, &error); > > + cpu_x86_set_props(cpu, props, &error); > > > > out: > > + QDECREF(props); > > g_strfreev(model_pieces); > > if (error) { > > fprintf(stderr, "%s\n", error_get_pretty(error)); > > -- > > 1.7.1 > > > > > > -- > Eduardo
On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote: > On Wed, 19 Dec 2012 14:54:30 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > > > It prepares for converting "+feature,-feature,feature=foo,feature" into > > > a set of key,value property pairs that will be applied to CPU by > > > cpu_x86_set_props(). > > > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted into > > > foo,val pair and a corresponding property setter by following patches. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU > > object as parameter and set the properties directly? Or you can see some > > use case where saving the property-setting dictionary for later use will > > be useful? > > I plan to use cpu_x86_parse_featurestr() + list of properties later when we > have properties and subclasses in place. Then it would be possible to > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of global > properties. Is it really worth it to use global properties when you can simply set the properties at the code that handles the "-cpu" string (and already has to create the CPU object)? (more comments about globals are below) > > In the end the option handler for -cpu XXX,... could be changed into: > > cpu_opt_parser() { > // hides legacy target specific ugliness > target_XXX_cpu_compat_parser_callback() { > cpu_class_name = get_class_name(optval) > > // dumb compatibility parser, which converts old format into > // canonical form feature,val property list > prop_list = parse_featurestr(optval) I'm expecting it to look different: instead of a target-specific parsing function, you just need target-specific legacy properties on the CPU object, that would be set by the (generic) featurestr parsing code if present. (e.g. "legacy-xlevel" could be used for the legacy "1 becomes 0x80000001" behavior, and "xlevel" would be the more sane xlevel property without any magic). > > // with classes and global properties we could get rid of the field > // cpu_model_str in CPUxxxState > return prop_list, cpu_class_name > } > > foreach (prop_list) > add_global_prop(cpu_class_name,prop,val) > > // could be later transformed to property of board object and > // we could get rid of one more global var > cpu_model = cpu_class_name The cpu_model string will be immediately used to create the CPU object just after this code runs. So do we really need to do the parsing before creating the CPU object and use global properties for that? It looks like unnecessary added complexity. > } > > > > > (This will probably require calling cpudef_2_x86_cpu() before > > cpu_x86_parse_featurestr(), and changing the existing > > cpu_x86_parse_featurestr() code to change the cpu object, not a > > x86_def_t struct, but I think this will simplify the logic a lot) > You cannot set/uset +-feat directly on CPU without breaking current > behavior where -feat overrides all +feat no matter in what order they are > specified. That's why dictionary is used to easily maintain "if(not feat) > ignore" logic and avoid duplication. Pls, look at commit > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3 > that converts +-feat into canonical feat=on/off form. Well, you can make feature parsing code could simply save the +feat/-feat results internally, and set/unset the properties directly at the CPU in the end. You can even use a dictionary internally, the point is that you don't need to expose the dictionary-based interface to the outside if not necessary (making the API much simpler, IMO). > > And if features are applied directly to CPU, it would require to another > rewrite if global properties are to be used. Which IMHO should be eventually > done since -cpu ... looks like global parameters for a specific cpu type. If we really are going to use global properties for the featurestring result, we will need to parse the featurestring before creating the CPU object, then you are correct. I'm questioning the need to use global properties for the featurestring parsing, if we can simply set the properties after creating the CPU object. I expect "-cpu" to be easily translatable to "-device" options, not to "-global" options. In other words, I expect this: -cpu foo,+xxx,-yyy to be translated in the future into something like: -device foo-x86-cpu,xxx=on,yyy=off and not to: -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu In other words, I disagree about "-cpu ..." looking like global parameters for a specific CPU type. > > > > > > --- > > > target-i386/cpu.c | 33 +++++++++++++++++++++++++++------ > > > 1 files changed, 27 insertions(+), 6 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index e075b59..a74d74b 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) > > > return 0; > > > } > > > > > > +/* Set features on X86CPU object based on a provide key,value list */ > > > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp) > > > +{ > > > + const QDictEntry *ent; > > > + > > > + for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) { > > > + const QString *qval = qobject_to_qstring(qdict_entry_value(ent)); > > > + object_property_parse(OBJECT(cpu), qstring_get_str(qval), > > > + qdict_entry_key(ent), errp); > > > + if (error_is_set(errp)) { > > > + return; > > > + } > > > + } > > > +} > > > + > > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > > */ > > > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > > > + QDict **props) > > > { > > > unsigned int i; > > > char *featurestr; /* Single 'key=value" string being parsed */ > > > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > > uint32_t minus_kvm_features = 0, minus_svm_features = 0; > > > uint32_t minus_7_0_ebx_features = 0; > > > uint32_t numvalue; > > > + gchar **feat_array = g_strsplit(features ? features : "", ",", 0); > > > + *props = qdict_new(); > > > + int j = 0; > > > > > > - featurestr = features ? strtok(features, ",") : NULL; > > > - > > > - while (featurestr) { > > > + while ((featurestr = feat_array[j++])) { > > > char *val; > > > if (featurestr[0] == '+') { > > > add_flagname_to_bitmaps(featurestr + 1, &plus_features, > > > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > > fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); > > > goto error; > > > } > > > - featurestr = strtok(NULL, ","); > > > } > > > x86_cpu_def->features |= plus_features; > > > x86_cpu_def->ext_features |= plus_ext_features; > > > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) > > > x86_cpu_def->kvm_features &= ~minus_kvm_features; > > > x86_cpu_def->svm_features &= ~minus_svm_features; > > > x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; > > > + g_strfreev(feat_array); > > > return 0; > > > > > > error: > > > + g_strfreev(feat_array); > > > return -1; > > > } > > > > > > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu) > > > int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > > { > > > x86_def_t def1, *def = &def1; > > > + QDict *props = NULL; > > > Error *error = NULL; > > > char *name, *features; > > > gchar **model_pieces; > > > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > > &def->ext3_features, &def->kvm_features, > > > &def->svm_features, &def->cpuid_7_0_ebx_features); > > > > > > - if (cpu_x86_parse_featurestr(def, features) < 0) { > > > + if (cpu_x86_parse_featurestr(def, features, &props) < 0) { > > > error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); > > > goto out; > > > } > > > > > > cpudef_2_x86_cpu(cpu, def, &error); > > > + cpu_x86_set_props(cpu, props, &error); > > > > > > out: > > > + QDECREF(props); > > > g_strfreev(model_pieces); > > > if (error) { > > > fprintf(stderr, "%s\n", error_get_pretty(error)); > > > -- > > > 1.7.1 > > > > > > > > > > -- > > Eduardo > > > -- > Regards, > Igor
On Thu, 20 Dec 2012 12:10:25 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: It's a log list of answers. please look through them all to the end. > On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote: > > On Wed, 19 Dec 2012 14:54:30 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > > > > It prepares for converting "+feature,-feature,feature=foo,feature" > > > > into a set of key,value property pairs that will be applied to CPU by > > > > cpu_x86_set_props(). > > > > > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted > > > > into foo,val pair and a corresponding property setter by following > > > > patches. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU > > > object as parameter and set the properties directly? Or you can see some > > > use case where saving the property-setting dictionary for later use will > > > be useful? > > > > I plan to use cpu_x86_parse_featurestr() + list of properties later when > > we have properties and subclasses in place. Then it would be possible to > > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of > > global properties. > > Is it really worth it to use global properties when you can simply set > the properties at the code that handles the "-cpu" string (and already > has to create the CPU object)? > > (more comments about globals are below) > > > > > > In the end the option handler for -cpu XXX,... could be changed into: > > > > cpu_opt_parser() { > > // hides legacy target specific ugliness > > target_XXX_cpu_compat_parser_callback() { > > cpu_class_name = get_class_name(optval) > > > > // dumb compatibility parser, which converts old format into > > // canonical form feature,val property list > > prop_list = parse_featurestr(optval) > > I'm expecting it to look different: instead of a target-specific parsing > function, you just need target-specific legacy properties on the CPU > object, that would be set by the (generic) featurestr parsing code if > present. > > (e.g. "legacy-xlevel" could be used for the legacy "1 becomes > 0x80000001" behavior, and "xlevel" would be the more sane xlevel > property without any magic). Introducing 'legacy-xlevel' doesn't solve problem, because with new name compatibility will be broken and user has to fix qemu invocation anyway. The same effect could be achieved if qemu exits with invalid value error and forces user to fix value on qemu invocation without introducing new properties. An idea of creating legacy properties would be a negative for following reasons: - new legacy properties with new names could break current users, and force them to fix their code + converting old feature names into the same property names with same behavior without adding new ones. But sometimes old behavior is wrong or hard to convert into property, so not good for cpu model eventually. - it burdens CPU object with properties that do almost the same as its 'correct' counterpart but not quite so. That complicates API of CPU object a lot. - more properties lead to more code, more invariants, resulting in more complex behavior. - increased maintenance (fixing/rewriting/testing/name it) - maintenance will multiply by target amount that has legacy features that do not map 1:1 into properties Legacy format converter allows to mitigate or reduce above mention problems: + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and expected not to behave in legacy way. + targets provide simple converter from command line format (could be painlessly deprecated in future) + allows to centralize point of conversion in one place, using utilities that target provides if it cares. + 'using global properties' and 'cpu_name=>class_name conversion in converter' opens easy road to removing CPUxxxState.cpu_model_str and as result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init() + cpu_init() would be easier to unify/simplify and could be reduced to object_new() + set instance specific properties() + realize() - practically every QOMified CPU that would use unified cpu_init, should provide as minimum converter cpu_name=>class_name, but it could be fixed gradually per target. - there will be ugly converter callback for a while, but it's temporary, after it's deprecation users would be expected to use foo-cpu-class,foo=xxx format In summary I think it would be better to keep CPU object as simple as possible with a stable API(a property set) for the every interface and convert legacy command-line format into this API. It will keep legacy code contained inside one function, which eventually should disappear as legacy features are deprecated. > > > > > > // with classes and global properties we could get rid of the > > field // cpu_model_str in CPUxxxState > > return prop_list, cpu_class_name > > } > > > > foreach (prop_list) > > add_global_prop(cpu_class_name,prop,val) > > > > // could be later transformed to property of board object and > > // we could get rid of one more global var > > cpu_model = cpu_class_name > > The cpu_model string will be immediately used to create the CPU object > just after this code runs. So do we really need to do the parsing before > creating the CPU object and use global properties for that? It looks > like unnecessary added complexity. It might be used immediately or might be not in case of hot-plug or copy_cpu() or depending where converter is called. Ideally there won't be cpu_model string at all. Presently all created CPUs use the same cpu_model string so parsing it only once instead of N times would be already an improvement. Pushing common features for type into global properties also makes sense, isn't global properties made specifically for this? If common features are pushed in global properties, cpu hot-plug could look like: device_add driver=cpu_x86_foo_class,[apicid|id]=xxx and all common features that user has overridden on command line would applied to a new cpu without extra work from user. > > > } > > > > > > > > (This will probably require calling cpudef_2_x86_cpu() before > > > cpu_x86_parse_featurestr(), and changing the existing > > > cpu_x86_parse_featurestr() code to change the cpu object, not a > > > x86_def_t struct, but I think this will simplify the logic a lot) > > You cannot set/uset +-feat directly on CPU without breaking current > > behavior where -feat overrides all +feat no matter in what order they are > > specified. That's why dictionary is used to easily maintain "if(not feat) > > ignore" logic and avoid duplication. Pls, look at commit > > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3 > > that converts +-feat into canonical feat=on/off form. > > Well, you can make feature parsing code could simply save the > +feat/-feat results internally, and set/unset the properties directly at > the CPU in the end. You can even use a dictionary internally, the point > is that you don't need to expose the dictionary-based interface to the > outside if not necessary (making the API much simpler, IMO). dictionary outside of cpu_x86_parse_featurestr() is temporary, until all features is converted into static properties, after that it could be moved inside cpu_x86_parse_featurestr(), which would push custom properties into global properties list and be called only once. Additionally it would be possible to remove introduced here cpu_x86_set_props(), simplifying cpu_x86_(init|create)(). I'd like to keep legacy handling in one compact place and as much as possible separated from CPU object itself. > > > > > And if features are applied directly to CPU, it would require to another > > rewrite if global properties are to be used. Which IMHO should be > > eventually done since -cpu ... looks like global parameters for a > > specific cpu type. > > If we really are going to use global properties for the featurestring > result, we will need to parse the featurestring before creating the CPU > object, then you are correct. > > I'm questioning the need to use global properties for the > featurestring parsing, if we can simply set the properties after > creating the CPU object. I expect "-cpu" to be easily translatable to > "-device" options, not to "-global" options. > > In other words, I expect this: > > -cpu foo,+xxx,-yyy > > to be translated in the future into something like: > > -device foo-x86-cpu,xxx=on,yyy=off > > and not to: > > -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu > > In other words, I disagree about "-cpu ..." looking like global > parameters for a specific CPU type. I can't agree with you here, on the contrary second option makes more sense for me. But both ways are lead to the same result and both ways will be possible to use. However if you look at current -cpu cpu_name,foo=xxx,... it is template for all CPUs of type cpu_name, qemu is creating. That's maps perfectly into global properties for a specific type. If you are concerned with command line length the for single/several cpus case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.." will be shorter, but with many cpus syntax: -global foo-cpu.yyy=off -device foo-cpu ... will be more compact. And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage of global properties as well. I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be translated into something like: -global foo-cpu.foo=x -device foo-cpu,id=a,node=nodeA -device foo-cpu,id=b,node=nodeB ... or -device foo-cpu,id=f(cpuN,nodeN) ... where common options are pushed into global properties and only instance specific ones are specified per instance. Do you see any issues ahead that global properties would introduce? > > [snip] > > > > -- > > > > 1.7.1 > > > > > > > > > > > > > > -- > > > Eduardo > > > > > > -- > > Regards, > > Igor >
On Thu, Dec 20, 2012 at 09:22:49PM +0100, Igor Mammedov wrote: > On Thu, 20 Dec 2012 12:10:25 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > It's a log list of answers. please look through them all to the end. > > On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote: > > > On Wed, 19 Dec 2012 14:54:30 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > > > > > It prepares for converting "+feature,-feature,feature=foo,feature" > > > > > into a set of key,value property pairs that will be applied to CPU by > > > > > cpu_x86_set_props(). > > > > > > > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted > > > > > into foo,val pair and a corresponding property setter by following > > > > > patches. > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU > > > > object as parameter and set the properties directly? Or you can see some > > > > use case where saving the property-setting dictionary for later use will > > > > be useful? > > > > > > I plan to use cpu_x86_parse_featurestr() + list of properties later when > > > we have properties and subclasses in place. Then it would be possible to > > > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of > > > global properties. > > > > Is it really worth it to use global properties when you can simply set > > the properties at the code that handles the "-cpu" string (and already > > has to create the CPU object)? > > > > (more comments about globals are below) > > > > > > > > > > In the end the option handler for -cpu XXX,... could be changed into: > > > > > > cpu_opt_parser() { > > > // hides legacy target specific ugliness > > > target_XXX_cpu_compat_parser_callback() { > > > cpu_class_name = get_class_name(optval) > > > > > > // dumb compatibility parser, which converts old format into > > > // canonical form feature,val property list > > > prop_list = parse_featurestr(optval) > > > > I'm expecting it to look different: instead of a target-specific parsing > > function, you just need target-specific legacy properties on the CPU > > object, that would be set by the (generic) featurestr parsing code if > > present. > > > > (e.g. "legacy-xlevel" could be used for the legacy "1 becomes > > 0x80000001" behavior, and "xlevel" would be the more sane xlevel > > property without any magic). > Introducing 'legacy-xlevel' doesn't solve problem, because with new name > compatibility will be broken and user has to fix qemu invocation anyway. > The same effect could be achieved if qemu exits with invalid value error and > forces user to fix value on qemu invocation without introducing new > properties. > > An idea of creating legacy properties would be a negative for following > reasons: > - new legacy properties with new names could break current users, and force > them to fix their code No, the point is to keep compatibility (just like your per-target parse_featurestr() approach), the only difference is that compatibility details would be encoded on the legacy properties, not inside parse_featurestr(). > + converting old feature names into the same property names with same > behavior without adding new ones. But sometimes old behavior is wrong or > hard to convert into property, so not good for cpu model eventually. That's why the legacy properties would be "legacy" one. The only code that would be supposed to use it would be the (legacy) "-cpu" parsing code. > - it burdens CPU object with properties that do almost the same as its > 'correct' counterpart but not quite so. That complicates API of CPU > object a lot. I believe the legacy property is as heavy a burden as a legacy featurestr-parsing code. It's no burden to the CPU object to have property setters that are almost never used. They would be just sitting there, unused. > - more properties lead to more code, more invariants, resulting in > more complex behavior. I don't think it's more code. It's less code: instead of writing multiple parsers, you have a target-independent parser, and the compatibility hacks would be isolated on the legacy property-setters. > - increased maintenance (fixing/rewriting/testing/name it) It looks like as much maintenance as having a target-specific parse_featurestr() function. Maybe even less, because now we can reuse the same parser for the other targets. > - maintenance will multiply by target amount that has legacy features that > do not map 1:1 into properties If a legacy feature do not map 1:1 into a property, you just add a legacy property-setter that will translate it into the "true" properties. It would contain exactly the same code you would put inside an "else if (!strcmp(...)) { ... }" block inside parse_featurestr(). > > Legacy format converter allows to mitigate or reduce above mention problems: > + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and > expected not to behave in legacy way. The legacy property doesn't even need to be a "static" one, and doesn't even need to have a getter. Other code would just never use it. It would be similar to the legacy properties registered by qdev_property_add_legacy() (maybe we could simply reuse that existing mechanism?). > + targets provide simple converter from command line format (could be > painlessly deprecated in future) This would have exactly the same functionality: targets provide simple list of legacy property-setters to translate old options. But now with a target-independent parser. > + allows to centralize point of conversion in one place, using utilities > that target provides if it cares. This is the main difference, I guess. But I think not having to duplicate parsing code and just have to provide a bunch of property-setter functions would be better than having target-specific parsing code just to "keep things in one place". You can easily put all the legacy setter functions in the same place inside the code, if you want to group them into a visible place. By the way, I don't think they _must_ be properties. All I want to avoid is having to duplicate the full featurestring parser for each target, when the only difference is that the targets will handle a few specific options differently from -device and QMP. Then featurestr parser could be a generic parser that simply set properties. But I would be happy with an approch based with something like a: parse_single_feature(const char *key, const char *value) callback in CPUClass, called by the generic parser. > + 'using global properties' and 'cpu_name=>class_name conversion in > converter' opens easy road to removing CPUxxxState.cpu_model_str and as > result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init() > + cpu_init() would be easier to unify/simplify and could be reduced to > object_new() + set instance specific properties() + realize() > - practically every QOMified CPU that would use unified cpu_init, should > provide as minimum converter cpu_name=>class_name, but it could be fixed > gradually per target. I don't think this has anything to do with the legacy-parse_featurestr handling. You can simplify/generalize cpu_init() in exactly the same way, the difference is that (wherever you call it) you can call a generic featurestr parser, instead of a target-specific one. IIRC, other target maintainers expressed interest in allowing options to be set in "-cpu". And we already have a feature string parser in x86, we just need to make it target-independent so other targets could use it. > - there will be ugly converter callback for a while, but it's temporary, > after it's deprecation users would be expected to use > foo-cpu-class,foo=xxx format The same can be said by the legacy property-setter (or a parse_single_feature() callback). They could be temporary and would be used only if a "-cpu" option that required compatibility translation was used. > > > In summary I think it would be better to keep CPU object as simple as possible > with a stable API(a property set) for the every interface and convert legacy > command-line format into this API. > It will keep legacy code contained inside one function, which eventually > should disappear as legacy features are deprecated. All above said, I am not strongly against your approach, but I believe we could try to make the feature string parsing code generic and reusable (with target-specific properties or callbacks), instead of having to write a new generic feature string parsing function. Anyway, the above is just bikeshedding to me, and your approach is an improvement, already, even if we try to make the parser more generic later. But the questions below about global properties seem important/interesting: > > > > > > > > > > > // with classes and global properties we could get rid of the > > > field // cpu_model_str in CPUxxxState > > > return prop_list, cpu_class_name > > > } > > > > > > foreach (prop_list) > > > add_global_prop(cpu_class_name,prop,val) > > > > > > // could be later transformed to property of board object and > > > // we could get rid of one more global var > > > cpu_model = cpu_class_name > > > > The cpu_model string will be immediately used to create the CPU object > > just after this code runs. So do we really need to do the parsing before > > creating the CPU object and use global properties for that? It looks > > like unnecessary added complexity. > It might be used immediately or might be not in case of hot-plug or > copy_cpu() or depending where converter is called. Ideally there won't be > cpu_model string at all. I don't see the difference between storing a cpu_model string and storing a dictionary to be used later. It would just be a micro-optimization for the code that creates new CPUs. > > Presently all created CPUs use the same cpu_model string so parsing it > only once instead of N times would be already an improvement. I don't believe we would want to make the code more complex just to optimize string parsing that is run a few times during QEMU initialization. That would be unnecessary optimization. > > Pushing common features for type into global properties also makes sense, > isn't global properties made specifically for this? Yes, but do we want to make "-cpu" affect every single -device/device_add call for CPUs, or only the ones created on initialization? That's the main question, I believe. > > If common features are pushed in global properties, cpu hot-plug could look > like: > device_add driver=cpu_x86_foo_class,[apicid|id]=xxx > and all common features that user has overridden on command line would > applied to a new cpu without extra work from user. CPU hotplug is an interesting case: if thinking only about the CPUs created from the command-line the "-global vs -device" question wouldn't make such a big difference. But in the case of device_add for CPU hotplug, it would be very different. But I am not sure which option is less surprising for users. What others think? Andreas? Should "-cpu" affect only the CPUs created on initialization, or all CPUs created by -device/device_add for the lifetime of the QEMU process? (in other words, should the options to "-cpu" be translated to "-device" arguments, or to "-global" arguments?) (BTW about the device_add example above: I believe we will want the hotplug interface to be based on a "cpu_index" parameter/property, to abstract out the complexity of the APIC ID calculation) > > > > > > } > > > > > > > > > > > (This will probably require calling cpudef_2_x86_cpu() before > > > > cpu_x86_parse_featurestr(), and changing the existing > > > > cpu_x86_parse_featurestr() code to change the cpu object, not a > > > > x86_def_t struct, but I think this will simplify the logic a lot) > > > You cannot set/uset +-feat directly on CPU without breaking current > > > behavior where -feat overrides all +feat no matter in what order they are > > > specified. That's why dictionary is used to easily maintain "if(not feat) > > > ignore" logic and avoid duplication. Pls, look at commit > > > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3 > > > that converts +-feat into canonical feat=on/off form. > > > > Well, you can make feature parsing code could simply save the > > +feat/-feat results internally, and set/unset the properties directly at > > the CPU in the end. You can even use a dictionary internally, the point > > is that you don't need to expose the dictionary-based interface to the > > outside if not necessary (making the API much simpler, IMO). > dictionary outside of cpu_x86_parse_featurestr() is temporary, > until all features is converted into static properties, after that it could > be moved inside cpu_x86_parse_featurestr(), which would push custom properties > into global properties list and be called only once. > Additionally it would be possible to remove introduced here > cpu_x86_set_props(), simplifying cpu_x86_(init|create)(). > I'd like to keep legacy handling in one compact place and as much as possible > separated from CPU object itself. > I see. If you are going to use global properties, the dictionary approach makes a lot of sense. > > > > > > > > And if features are applied directly to CPU, it would require to another > > > rewrite if global properties are to be used. Which IMHO should be > > > eventually done since -cpu ... looks like global parameters for a > > > specific cpu type. > > > > If we really are going to use global properties for the featurestring > > result, we will need to parse the featurestring before creating the CPU > > object, then you are correct. > > > > I'm questioning the need to use global properties for the > > featurestring parsing, if we can simply set the properties after > > creating the CPU object. I expect "-cpu" to be easily translatable to > > "-device" options, not to "-global" options. > > > > In other words, I expect this: > > > > -cpu foo,+xxx,-yyy > > > > to be translated in the future into something like: > > > > -device foo-x86-cpu,xxx=on,yyy=off > > > > and not to: > > > > -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu > > > > In other words, I disagree about "-cpu ..." looking like global > > parameters for a specific CPU type. > I can't agree with you here, on the contrary second option makes more sense > for me. > But both ways are lead to the same result and both ways will be possible to > use. Both ways have different results if we think about using device_add for CPU hotplug, though. > > However if you look at current -cpu cpu_name,foo=xxx,... it is template for > all CPUs of type cpu_name, qemu is creating. That's maps perfectly into > global properties for a specific type. I see it as a template of the CPUs QEMU is creating _at initialization_, but I understand that seeing it as a template for all CPUs QEMU will ever create makes sense, too. We just have to decide which way to go. > > If you are concerned with command line length the for single/several cpus > case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.." > will be shorter, but with many cpus syntax: > -global foo-cpu.yyy=off -device foo-cpu ... > will be more compact. command-line length won't be an issue if users use a config file. :-) > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage > of global properties as well. > > I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be > translated into something like: > -global foo-cpu.foo=x > -device foo-cpu,id=a,node=nodeA > -device foo-cpu,id=b,node=nodeB > ... > or > -device foo-cpu,id=f(cpuN,nodeN) > ... > where common options are pushed into global properties and only instance > specific ones are specified per instance. > > Do you see any issues ahead that global properties would introduce? I see no issue, except that it was not what I was expecting at first. I just don't know which one is less surprising for users (including libvirt). Summary: I can see that translating "-cpu" to global properties may make sense, too. Now I'm divided. I hope to hear what Andreas (and other maintainers) think about this. [...]
On Thu, 20 Dec 2012 20:13:19 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Dec 20, 2012 at 09:22:49PM +0100, Igor Mammedov wrote: > > On Thu, 20 Dec 2012 12:10:25 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > It's a log list of answers. please look through them all to the end. > > > On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote: > > > > On Wed, 19 Dec 2012 14:54:30 -0200 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote: > > > > > > It prepares for converting "+feature,-feature,feature=foo,feature" > > > > > > into a set of key,value property pairs that will be applied to CPU by > > > > > > cpu_x86_set_props(). > > > > > > > > > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted > > > > > > into foo,val pair and a corresponding property setter by following > > > > > > patches. > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU > > > > > object as parameter and set the properties directly? Or you can see some > > > > > use case where saving the property-setting dictionary for later use will > > > > > be useful? > > > > > > > > I plan to use cpu_x86_parse_featurestr() + list of properties later when > > > > we have properties and subclasses in place. Then it would be possible to > > > > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of > > > > global properties. > > > > > > Is it really worth it to use global properties when you can simply set > > > the properties at the code that handles the "-cpu" string (and already > > > has to create the CPU object)? > > > > > > (more comments about globals are below) > > > > > > > > > > > > > > In the end the option handler for -cpu XXX,... could be changed into: > > > > > > > > cpu_opt_parser() { > > > > // hides legacy target specific ugliness > > > > target_XXX_cpu_compat_parser_callback() { > > > > cpu_class_name = get_class_name(optval) > > > > > > > > // dumb compatibility parser, which converts old format into > > > > // canonical form feature,val property list > > > > prop_list = parse_featurestr(optval) > > > > > > I'm expecting it to look different: instead of a target-specific parsing > > > function, you just need target-specific legacy properties on the CPU > > > object, that would be set by the (generic) featurestr parsing code if > > > present. > > > > > > (e.g. "legacy-xlevel" could be used for the legacy "1 becomes > > > 0x80000001" behavior, and "xlevel" would be the more sane xlevel > > > property without any magic). > > Introducing 'legacy-xlevel' doesn't solve problem, because with new name > > compatibility will be broken and user has to fix qemu invocation anyway. > > The same effect could be achieved if qemu exits with invalid value error and > > forces user to fix value on qemu invocation without introducing new > > properties. > > > > An idea of creating legacy properties would be a negative for following > > reasons: > > - new legacy properties with new names could break current users, and force > > them to fix their code > > No, the point is to keep compatibility (just like your per-target > parse_featurestr() approach), the only difference is that compatibility > details would be encoded on the legacy properties, not inside > parse_featurestr(). > > > + converting old feature names into the same property names with same > > behavior without adding new ones. But sometimes old behavior is wrong or > > hard to convert into property, so not good for cpu model eventually. > > That's why the legacy properties would be "legacy" one. The only code > that would be supposed to use it would be the (legacy) "-cpu" parsing > code. > > > - it burdens CPU object with properties that do almost the same as its > > 'correct' counterpart but not quite so. That complicates API of CPU > > object a lot. > > I believe the legacy property is as heavy a burden as a legacy > featurestr-parsing code. It's no burden to the CPU object to have > property setters that are almost never used. They would be just sitting > there, unused. > > > - more properties lead to more code, more invariants, resulting in > > more complex behavior. > > I don't think it's more code. It's less code: instead of writing > multiple parsers, you have a target-independent parser, and the > compatibility hacks would be isolated on the legacy property-setters. > > > - increased maintenance (fixing/rewriting/testing/name it) > > It looks like as much maintenance as having a target-specific > parse_featurestr() function. Maybe even less, because now we can reuse > the same parser for the other targets. > > > - maintenance will multiply by target amount that has legacy features that > > do not map 1:1 into properties > > If a legacy feature do not map 1:1 into a property, you just add a > legacy property-setter that will translate it into the "true" > properties. It would contain exactly the same code you would put inside > an "else if (!strcmp(...)) { ... }" block inside parse_featurestr(). > > > > > Legacy format converter allows to mitigate or reduce above mention problems: > > + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and > > expected not to behave in legacy way. > > The legacy property doesn't even need to be a "static" one, and doesn't > even need to have a getter. Other code would just never use it. It would > be similar to the legacy properties registered by > qdev_property_add_legacy() (maybe we could simply reuse that existing > mechanism?). > > > > + targets provide simple converter from command line format (could be > > painlessly deprecated in future) > > This would have exactly the same functionality: targets provide simple > list of legacy property-setters to translate old options. But now with a > target-independent parser. > > > > + allows to centralize point of conversion in one place, using utilities > > that target provides if it cares. > > This is the main difference, I guess. But I think not having to > duplicate parsing code and just have to provide a bunch of > property-setter functions would be better than having target-specific > parsing code just to "keep things in one place". You can easily put all > the legacy setter functions in the same place inside the code, if you > want to group them into a visible place. > > By the way, I don't think they _must_ be properties. All I want to avoid > is having to duplicate the full featurestring parser for each target, > when the only difference is that the targets will handle a few specific > options differently from -device and QMP. Then featurestr parser could > be a generic parser that simply set properties. But I would be happy > with an approch based with something like a: > parse_single_feature(const char *key, const char *value) > callback in CPUClass, called by the generic parser. > > > > + 'using global properties' and 'cpu_name=>class_name conversion in > > converter' opens easy road to removing CPUxxxState.cpu_model_str and as > > result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init() > > + cpu_init() would be easier to unify/simplify and could be reduced to > > object_new() + set instance specific properties() + realize() > > - practically every QOMified CPU that would use unified cpu_init, should > > provide as minimum converter cpu_name=>class_name, but it could be fixed > > gradually per target. > > > I don't think this has anything to do with the legacy-parse_featurestr > handling. You can simplify/generalize cpu_init() in exactly the same > way, the difference is that (wherever you call it) you can call a > generic featurestr parser, instead of a target-specific one. > > IIRC, other target maintainers expressed interest in allowing options to > be set in "-cpu". And we already have a feature string parser in x86, we > just need to make it target-independent so other targets could use it. > > > > - there will be ugly converter callback for a while, but it's temporary, > > after it's deprecation users would be expected to use > > foo-cpu-class,foo=xxx format > > The same can be said by the legacy property-setter (or a > parse_single_feature() callback). They could be temporary and would be > used only if a "-cpu" option that required compatibility translation was > used. > > > > > > > > In summary I think it would be better to keep CPU object as simple as possible > > with a stable API(a property set) for the every interface and convert legacy > > command-line format into this API. > > It will keep legacy code contained inside one function, which eventually > > should disappear as legacy features are deprecated. > > All above said, I am not strongly against your approach, but I believe > we could try to make the feature string parsing code generic and > reusable (with target-specific properties or callbacks), instead of > having to write a new generic feature string parsing function. > > Anyway, the above is just bikeshedding to me, and your approach is an > improvement, already, even if we try to make the parser more generic > later. > feature string parser in this series isn't meant to be generic, it's just a simplification of the current function and splitting parsing features & setting properties into separate steps. kind of what you did with cpu_name+feature_string split. It's not clear to me what you mean under legacy properties, could you throw in a hack using as example xlevel feature to demonstrate what you mean, please? > But the questions below about global properties seem important/interesting: > > > > > > > > > > > > > > > > > // with classes and global properties we could get rid of the > > > > field // cpu_model_str in CPUxxxState > > > > return prop_list, cpu_class_name > > > > } > > > > > > > > foreach (prop_list) > > > > add_global_prop(cpu_class_name,prop,val) > > > > > > > > // could be later transformed to property of board object and > > > > // we could get rid of one more global var > > > > cpu_model = cpu_class_name > > > > > > The cpu_model string will be immediately used to create the CPU object > > > just after this code runs. So do we really need to do the parsing before > > > creating the CPU object and use global properties for that? It looks > > > like unnecessary added complexity. > > It might be used immediately or might be not in case of hot-plug or > > copy_cpu() or depending where converter is called. Ideally there won't be > > cpu_model string at all. > > I don't see the difference between storing a cpu_model string and > storing a dictionary to be used later. It would just be a > micro-optimization for the code that creates new CPUs. Dictionary is an implementation detail, a temporary list of properties that would be applied to global properties and then list destroyed. It is there now because: 1. it's just a convenient data structure to implement current weird +-foo semantic 2. a necessary intermediate step to hold list of properties because properties/features is not yet converted into static properties. Properties could/should be pushed directly into global properties if possible (i.e. when property is static), then there won't be any need in intermediate property list at all. There is not compelling reason to use dictionary in generic code, it could be removed or be hidden inside current featurestr parser. > > > > > > Presently all created CPUs use the same cpu_model string so parsing it > > only once instead of N times would be already an improvement. > > I don't believe we would want to make the code more complex just to > optimize string parsing that is run a few times during QEMU > initialization. That would be unnecessary optimization. If we move generalized parsing out into vl.c & *-user/main.c option parsing code, it would simplify every target. I would be better to do option parsing at the time when options are parsed in a single place and let boards/targets to concentrate on creating cpus (without caring about options at all, not really their job). > > > > > Pushing common features for type into global properties also makes sense, > > isn't global properties made specifically for this? > > Yes, but do we want to make "-cpu" affect every single > -device/device_add call for CPUs, or only the ones created on > initialization? That's the main question, I believe. If user overrides default cpu model features with -cpu option it would be only logical for hot-plugged cpus to use that overrides as well, instead of creating cpu that would be different (it may confuse guests, to the point of crashing). IMHO, It would be surprising for user to get a different cpu on hot-plug than already existing ones, in most cases. And if user likes to shoot in its own foot, it will be possible to override global features by adding extra foo=xxx to a specific device_add/-device command. > > > > > If common features are pushed in global properties, cpu hot-plug could look > > like: > > device_add driver=cpu_x86_foo_class,[apicid|id]=xxx > > and all common features that user has overridden on command line would > > applied to a new cpu without extra work from user. > > CPU hotplug is an interesting case: if thinking only about the CPUs > created from the command-line the "-global vs -device" question wouldn't > make such a big difference. But in the case of device_add for CPU > hotplug, it would be very different. But I am not sure which option is > less surprising for users. > > What others think? Andreas? Should "-cpu" affect only the CPUs created > on initialization, or all CPUs created by -device/device_add for the > lifetime of the QEMU process? (in other words, should the options to > "-cpu" be translated to "-device" arguments, or to "-global" arguments?) > > (BTW about the device_add example above: I believe we will want the > hotplug interface to be based on a "cpu_index" parameter/property, to > abstract out the complexity of the APIC ID calculation) there was other opinion about cpu_index http://permalink.gmane.org/gmane.comp.emulators.qemu/151626 However if board will allocate sockets (Anthony suggested to try links for this) then user won't have to calculate ID, instead of user will have to enumerate existing cpu links and use IDs embedded in it for managing cpus. > > > > > > > > > > > > } > > > > > > > > > > > > > > (This will probably require calling cpudef_2_x86_cpu() before > > > > > cpu_x86_parse_featurestr(), and changing the existing > > > > > cpu_x86_parse_featurestr() code to change the cpu object, not a > > > > > x86_def_t struct, but I think this will simplify the logic a lot) > > > > You cannot set/uset +-feat directly on CPU without breaking current > > > > behavior where -feat overrides all +feat no matter in what order they are > > > > specified. That's why dictionary is used to easily maintain "if(not feat) > > > > ignore" logic and avoid duplication. Pls, look at commit > > > > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3 > > > > that converts +-feat into canonical feat=on/off form. > > > > > > Well, you can make feature parsing code could simply save the > > > +feat/-feat results internally, and set/unset the properties directly at > > > the CPU in the end. You can even use a dictionary internally, the point > > > is that you don't need to expose the dictionary-based interface to the > > > outside if not necessary (making the API much simpler, IMO). > > dictionary outside of cpu_x86_parse_featurestr() is temporary, > > until all features is converted into static properties, after that it could > > be moved inside cpu_x86_parse_featurestr(), which would push custom properties > > into global properties list and be called only once. > > Additionally it would be possible to remove introduced here > > cpu_x86_set_props(), simplifying cpu_x86_(init|create)(). > > I'd like to keep legacy handling in one compact place and as much as possible > > separated from CPU object itself. > > > > I see. If you are going to use global properties, the dictionary > approach makes a lot of sense. > > > > > > > > > > > > > And if features are applied directly to CPU, it would require to another > > > > rewrite if global properties are to be used. Which IMHO should be > > > > eventually done since -cpu ... looks like global parameters for a > > > > specific cpu type. > > > > > > If we really are going to use global properties for the featurestring > > > result, we will need to parse the featurestring before creating the CPU > > > object, then you are correct. > > > > > > I'm questioning the need to use global properties for the > > > featurestring parsing, if we can simply set the properties after > > > creating the CPU object. I expect "-cpu" to be easily translatable to > > > "-device" options, not to "-global" options. > > > > > > In other words, I expect this: > > > > > > -cpu foo,+xxx,-yyy > > > > > > to be translated in the future into something like: > > > > > > -device foo-x86-cpu,xxx=on,yyy=off > > > > > > and not to: > > > > > > -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu > > > > > > In other words, I disagree about "-cpu ..." looking like global > > > parameters for a specific CPU type. > > I can't agree with you here, on the contrary second option makes more sense > > for me. > > But both ways are lead to the same result and both ways will be possible to > > use. > > Both ways have different results if we think about using device_add for > CPU hotplug, though. > > > > > However if you look at current -cpu cpu_name,foo=xxx,... it is template for > > all CPUs of type cpu_name, qemu is creating. That's maps perfectly into > > global properties for a specific type. > > I see it as a template of the CPUs QEMU is creating _at initialization_, > but I understand that seeing it as a template for all CPUs QEMU will > ever create makes sense, too. We just have to decide which way to go. That's what I was trying to explain, I'm sorry my poor English that it took so many words to get here. > > > > > > If you are concerned with command line length the for single/several cpus > > case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.." > > will be shorter, but with many cpus syntax: > > -global foo-cpu.yyy=off -device foo-cpu ... > > will be more compact. > > command-line length won't be an issue if users use a config file. :-) > > > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage > > of global properties as well. > > > > I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be > > translated into something like: > > -global foo-cpu.foo=x > > -device foo-cpu,id=a,node=nodeA > > -device foo-cpu,id=b,node=nodeB > > ... > > or > > -device foo-cpu,id=f(cpuN,nodeN) > > ... > > where common options are pushed into global properties and only instance > > specific ones are specified per instance. > > > > Do you see any issues ahead that global properties would introduce? > > I see no issue, except that it was not what I was expecting at first. I > just don't know which one is less surprising for users (including > libvirt). users shouldn't be affected, -cpu will continue to works the same way until we start deprecating legacy behavior. > > Summary: I can see that translating "-cpu" to global properties may make > sense, too. Now I'm divided. I hope to hear what Andreas (and other > maintainers) think about this. > > [...] > > -- > Eduardo >
On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote: [...] > > All above said, I am not strongly against your approach, but I believe > > we could try to make the feature string parsing code generic and > > reusable (with target-specific properties or callbacks), instead of > > having to write a new generic feature string parsing function. > > > > Anyway, the above is just bikeshedding to me, and your approach is an > > improvement, already, even if we try to make the parser more generic > > later. > > > feature string parser in this series isn't meant to be generic, it's just > a simplification of the current function and splitting parsing > features & setting properties into separate steps. kind of what you did with > cpu_name+feature_string split. > > It's not clear to me what you mean under legacy properties, could you > throw in a hack using as example xlevel feature to demonstrate what you mean, > please? I will try to show it below, but note that we can try to do that _after_ we introduce the true/final/clean properties that won't have those hacks, and after changing the current parse_featurestr() implementation to contain the compatibility hacks (that's the work you are already doing). I was thinking about using the "legacy-%s" feature inside qdev_prop_parse(), this way: /* Generic featurestr parsing function */ void generic_parse_featurestr(CPUClass *cc, const char *featurestr) { opts = g_strsplit(featurestr, ",") for (each opt in opts) { if (opt[0] == '+') { dict[opt+1] = 'true'; } else if (opt[0] == ' dict[opt+1] = 'false ; } else if (opt contains '=) { key, value = g_strsplit(opt, "="); dict[key] = value; } } for (each key in dict) { /* qdev_prop_set_globals() uses qdev_prop_parse(), that already * checks for a "legacy-<name>" property. */ qdev_add_one_global(key, dict[key]); } } /* target-specific handling of legacy values for the -cpu option */ void cpu_set_legacy_xlevel(X86CPU *cpu, Visitor *v, ...) { int64_t i; visit_type_int(v, &i, ...); if (i < 0x80000000) i += 0x80000000; object_property_set_int(cpu, i, "xlevel"); } void cpu_set_legacy_foobar(...) { ... } void cpu_x86_class_init(CPUClass *cc, ...) { ... qdev_class_add_property("legacy-xlevel", cpu_set_legacy_xlevel, NULL, ...); /* only a setter, no getter */ qdev_class_add_property("legacy-foobar", cpu_set_legacy_foobar, NULL, ...); /* only a setter, no getter */ } Note: - We don't have a qdev_class_add_property() function, so we have to add the legacy property to the ->props array; - The props array is based on name/offset/type, so we would need to define (for example) a "PropertyInfo cpu_prop_xlevel" struct; - On the other hand, setting a ->parse function on PropertyInfo gives us the legacy-* property registration for free when registering "xlevel" (see device_initfn()/qdev_property_add_legacy()) - But I would prefer a qdev_class_add_property() interface, as it would be much simpler... So, in the end it could be too complex due to the interfaces provided by qdev. > > > But the questions below about global properties seem important/interesting: > > > > > > > > > > > > > > > > > > > > > > > // with classes and global properties we could get rid of the > > > > > field // cpu_model_str in CPUxxxState > > > > > return prop_list, cpu_class_name > > > > > } > > > > > > > > > > foreach (prop_list) > > > > > add_global_prop(cpu_class_name,prop,val) > > > > > > > > > > // could be later transformed to property of board object and > > > > > // we could get rid of one more global var > > > > > cpu_model = cpu_class_name > > > > > > > > The cpu_model string will be immediately used to create the CPU object > > > > just after this code runs. So do we really need to do the parsing before > > > > creating the CPU object and use global properties for that? It looks > > > > like unnecessary added complexity. > > > It might be used immediately or might be not in case of hot-plug or > > > copy_cpu() or depending where converter is called. Ideally there won't be > > > cpu_model string at all. > > > > I don't see the difference between storing a cpu_model string and > > storing a dictionary to be used later. It would just be a > > micro-optimization for the code that creates new CPUs. > Dictionary is an implementation detail, a temporary list of properties that > would be applied to global properties and then list destroyed. It is there now > because: > 1. it's just a convenient data structure to implement current weird +-foo > semantic > 2. a necessary intermediate step to hold list of properties because > properties/features is not yet converted into static properties. Properties > could/should be pushed directly into global properties if possible (i.e. > when property is static), then there won't be any need in intermediate > property list at all. > > There is not compelling reason to use dictionary in generic code, it could be > removed or be hidden inside current featurestr parser. I wasn't taking into account that the dictionary would be simply used to feed the global property list, so nevermind. > > > > > > > > > > > Presently all created CPUs use the same cpu_model string so parsing it > > > only once instead of N times would be already an improvement. > > > > I don't believe we would want to make the code more complex just to > > optimize string parsing that is run a few times during QEMU > > initialization. That would be unnecessary optimization. > If we move generalized parsing out into vl.c & *-user/main.c option parsing > code, it would simplify every target. I would be better to do option parsing > at the time when options are parsed in a single place and let boards/targets to > concentrate on creating cpus (without caring about options at all, not really > their job). Even if we did it the "inefficient" way (parsing it multiple times), it could be moved to generic CPU creation code. Anyway, my assumption about "making the code more complex" doesn't apply if the dictionary is used only internally and we use global properties. > > > > > > > > > Pushing common features for type into global properties also makes sense, > > > isn't global properties made specifically for this? > > > > Yes, but do we want to make "-cpu" affect every single > > -device/device_add call for CPUs, or only the ones created on > > initialization? That's the main question, I believe. > If user overrides default cpu model features with -cpu option it would be only > logical for hot-plugged cpus to use that overrides as well, instead of creating > cpu that would be different (it may confuse guests, to the point of crashing). > IMHO, It would be surprising for user to get a different cpu on hot-plug than > already existing ones, in most cases. > > And if user likes to shoot in its own foot, it will be possible to override > global features by adding extra foo=xxx to a specific device_add/-device > command. This is a good argument, and I am now convinced that translating "-cpu" to global properties would be a good idea. :) > > > > > > If common features are pushed in global properties, cpu hot-plug could look > > > like: > > > device_add driver=cpu_x86_foo_class,[apicid|id]=xxx > > > and all common features that user has overridden on command line would > > > applied to a new cpu without extra work from user. > > > > CPU hotplug is an interesting case: if thinking only about the CPUs > > created from the command-line the "-global vs -device" question wouldn't > > make such a big difference. But in the case of device_add for CPU > > hotplug, it would be very different. But I am not sure which option is > > less surprising for users. > > > > What others think? Andreas? Should "-cpu" affect only the CPUs created > > on initialization, or all CPUs created by -device/device_add for the > > lifetime of the QEMU process? (in other words, should the options to > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?) > > > > (BTW about the device_add example above: I believe we will want the > > hotplug interface to be based on a "cpu_index" parameter/property, to > > abstract out the complexity of the APIC ID calculation) > there was other opinion about cpu_index > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626 > > However if board will allocate sockets (Anthony suggested to try links for > this) then user won't have to calculate ID, instead of user will have to > enumerate existing cpu links and use IDs embedded in it for managing > cpus. I don't care if we use "IDs", "links", a "cpu_index" property, socket objects, socket IDs, or whatever. I just strongly recommend we don't force users/management-tools to calculate the APIC ID themselves. We could even kill the cpu_index field (as suggested at the URL above). But to allow the APIC ID to be calculated, we need to let the CPU know where it is "located" in the system (its cpu_index/socket_index/whatever), and the CPU topology of the system, somehow. (I believe asked multiple times why devices can't know their parents by design [so they can't just ask the sockets/motherboard for the topology], and I never got an answer...) [...] > > > > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage > > > of global properties as well. > > > > > > I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be > > > translated into something like: > > > -global foo-cpu.foo=x > > > -device foo-cpu,id=a,node=nodeA > > > -device foo-cpu,id=b,node=nodeB > > > ... > > > or > > > -device foo-cpu,id=f(cpuN,nodeN) > > > ... > > > where common options are pushed into global properties and only instance > > > specific ones are specified per instance. > > > > > > Do you see any issues ahead that global properties would introduce? > > > > I see no issue, except that it was not what I was expecting at first. I > > just don't know which one is less surprising for users (including > > libvirt). > users shouldn't be affected, -cpu will continue to works the same way until we > start deprecating legacy behavior. I mean: users will be affected by our decision once they start using device_add for CPU hotplug (then both approaches would have different results).
On Fri, 21 Dec 2012 11:50:26 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote: > [...] > > > All above said, I am not strongly against your approach, but I believe > > > we could try to make the feature string parsing code generic and > > > reusable (with target-specific properties or callbacks), instead of > > > having to write a new generic feature string parsing function. > > > > > > Anyway, the above is just bikeshedding to me, and your approach is an > > > improvement, already, even if we try to make the parser more generic > > > later. > > > > > feature string parser in this series isn't meant to be generic, it's just > > a simplification of the current function and splitting parsing > > features & setting properties into separate steps. kind of what you did with > > cpu_name+feature_string split. > > > > It's not clear to me what you mean under legacy properties, could you > > throw in a hack using as example xlevel feature to demonstrate what you mean, > > please? > > I will try to show it below, but note that we can try to do that _after_ > we introduce the true/final/clean properties that won't have those > hacks, and after changing the current parse_featurestr() implementation > to contain the compatibility hacks (that's the work you are already > doing). > > I was thinking about using the "legacy-%s" feature inside > qdev_prop_parse(), this way: > > /* Generic featurestr parsing function */ > void generic_parse_featurestr(CPUClass *cc, const char *featurestr) > { > opts = g_strsplit(featurestr, ",") > for (each opt in opts) { > if (opt[0] == '+') { > dict[opt+1] = 'true'; > } else if (opt[0] == ' > dict[opt+1] = 'false ; > } else if (opt contains '=) { > key, value = g_strsplit(opt, "="); > dict[key] = value; > } > } It's only sparc and i386 targets that use +- format for features. I'd rather avoid exposing +- format parsing to other targets. BTW it's a bit more complex for i386 target due to f-* name conversion of features into properties. We should work towards deprecation of this format and not introducing it to other targets. When all this legacy stuff is deprecated we should have only foo=xxx format left as the other devices, then there won't be need in dedicated parser. > for (each key in dict) { > /* qdev_prop_set_globals() uses qdev_prop_parse(), that already > * checks for a "legacy-<name>" property. > */ > qdev_add_one_global(key, dict[key]); > } > } > In addition, having per target converter makes deprecation easier and possible to do on per target basis instead of fixing everything at once. > > /* target-specific handling of legacy values for the -cpu option */ > void cpu_set_legacy_xlevel(X86CPU *cpu, Visitor *v, ...) > { > int64_t i; > visit_type_int(v, &i, ...); > if (i < 0x80000000) > i += 0x80000000; > object_property_set_int(cpu, i, "xlevel"); > } > > void cpu_set_legacy_foobar(...) > { > ... > } > > void cpu_x86_class_init(CPUClass *cc, ...) > { > ... > qdev_class_add_property("legacy-xlevel", cpu_set_legacy_xlevel, NULL, ...); /* only a setter, no getter */ > qdev_class_add_property("legacy-foobar", cpu_set_legacy_foobar, NULL, ...); /* only a setter, no getter */ > } > > > Note: > > - We don't have a qdev_class_add_property() function, so we have to add > the legacy property to the ->props array; > - The props array is based on name/offset/type, so we would need to > define (for example) a "PropertyInfo cpu_prop_xlevel" struct; > - On the other hand, setting a ->parse function on PropertyInfo gives > us the legacy-* property registration for free when registering > "xlevel" (see device_initfn()/qdev_property_add_legacy()) > - But I would prefer a qdev_class_add_property() interface, as it > would be much simpler... > > So, in the end it could be too complex due to the interfaces provided by > qdev. Concentrating legacy conversion to one function /especially if function already exists/ and gradually srinking it to NOP looks a bit simpler than introducing new legacy-* code and/or qdev interfaces. > > > > > > But the questions below about global properties seem important/interesting: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > // with classes and global properties we could get rid of the > > > > > > field // cpu_model_str in CPUxxxState > > > > > > return prop_list, cpu_class_name > > > > > > } > > > > > > > > > > > > foreach (prop_list) > > > > > > add_global_prop(cpu_class_name,prop,val) > > > > > > > > > > > > // could be later transformed to property of board object and > > > > > > // we could get rid of one more global var > > > > > > cpu_model = cpu_class_name > > > > > > > > > > The cpu_model string will be immediately used to create the CPU object > > > > > just after this code runs. So do we really need to do the parsing before > > > > > creating the CPU object and use global properties for that? It looks > > > > > like unnecessary added complexity. > > > > It might be used immediately or might be not in case of hot-plug or > > > > copy_cpu() or depending where converter is called. Ideally there won't be > > > > cpu_model string at all. > > > > > > I don't see the difference between storing a cpu_model string and > > > storing a dictionary to be used later. It would just be a > > > micro-optimization for the code that creates new CPUs. > > Dictionary is an implementation detail, a temporary list of properties that > > would be applied to global properties and then list destroyed. It is there now > > because: > > 1. it's just a convenient data structure to implement current weird +-foo > > semantic > > 2. a necessary intermediate step to hold list of properties because > > properties/features is not yet converted into static properties. Properties > > could/should be pushed directly into global properties if possible (i.e. > > when property is static), then there won't be any need in intermediate > > property list at all. > > > > There is not compelling reason to use dictionary in generic code, it could be > > removed or be hidden inside current featurestr parser. > > I wasn't taking into account that the dictionary would be simply used to > feed the global property list, so nevermind. > > > > > > > > > > > > > > > > > Presently all created CPUs use the same cpu_model string so parsing it > > > > only once instead of N times would be already an improvement. > > > > > > I don't believe we would want to make the code more complex just to > > > optimize string parsing that is run a few times during QEMU > > > initialization. That would be unnecessary optimization. > > If we move generalized parsing out into vl.c & *-user/main.c option parsing > > code, it would simplify every target. I would be better to do option parsing > > at the time when options are parsed in a single place and let boards/targets to > > concentrate on creating cpus (without caring about options at all, not really > > their job). > > Even if we did it the "inefficient" way (parsing it multiple times), it > could be moved to generic CPU creation code. > > Anyway, my assumption about "making the code more complex" doesn't apply > if the dictionary is used only internally and we use global properties. > > > > > > > > > > > > > > > Pushing common features for type into global properties also makes sense, > > > > isn't global properties made specifically for this? > > > > > > Yes, but do we want to make "-cpu" affect every single > > > -device/device_add call for CPUs, or only the ones created on > > > initialization? That's the main question, I believe. > > If user overrides default cpu model features with -cpu option it would be only > > logical for hot-plugged cpus to use that overrides as well, instead of creating > > cpu that would be different (it may confuse guests, to the point of crashing). > > IMHO, It would be surprising for user to get a different cpu on hot-plug than > > already existing ones, in most cases. > > > > And if user likes to shoot in its own foot, it will be possible to override > > global features by adding extra foo=xxx to a specific device_add/-device > > command. > > This is a good argument, and I am now convinced that translating "-cpu" > to global properties would be a good idea. :) > > > > > > > > > > If common features are pushed in global properties, cpu hot-plug could look > > > > like: > > > > device_add driver=cpu_x86_foo_class,[apicid|id]=xxx > > > > and all common features that user has overridden on command line would > > > > applied to a new cpu without extra work from user. > > > > > > CPU hotplug is an interesting case: if thinking only about the CPUs > > > created from the command-line the "-global vs -device" question wouldn't > > > make such a big difference. But in the case of device_add for CPU > > > hotplug, it would be very different. But I am not sure which option is > > > less surprising for users. > > > > > > What others think? Andreas? Should "-cpu" affect only the CPUs created > > > on initialization, or all CPUs created by -device/device_add for the > > > lifetime of the QEMU process? (in other words, should the options to > > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?) > > > > > > (BTW about the device_add example above: I believe we will want the > > > hotplug interface to be based on a "cpu_index" parameter/property, to > > > abstract out the complexity of the APIC ID calculation) > > there was other opinion about cpu_index > > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626 > > > > However if board will allocate sockets (Anthony suggested to try links for > > this) then user won't have to calculate ID, instead of user will have to > > enumerate existing cpu links and use IDs embedded in it for managing > > cpus. > > I don't care if we use "IDs", "links", a "cpu_index" property, socket > objects, socket IDs, or whatever. I just strongly recommend we don't > force users/management-tools to calculate the APIC ID themselves. > > We could even kill the cpu_index field (as suggested at the URL above). > But to allow the APIC ID to be calculated, we need to let the CPU know > where it is "located" in the system (its > cpu_index/socket_index/whatever), and the CPU topology of the system, > somehow. > > (I believe asked multiple times why devices can't know their parents by > design [so they can't just ask the sockets/motherboard for the > topology], and I never got an answer...) Can we create core_id/package_id/node_id properties for CPU and use them when creating/hot-plugging CPUs for APICID calculation? > > > [...] > > > > > > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage > > > > of global properties as well. > > > > > > > > I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be > > > > translated into something like: > > > > -global foo-cpu.foo=x > > > > -device foo-cpu,id=a,node=nodeA > > > > -device foo-cpu,id=b,node=nodeB > > > > ... > > > > or > > > > -device foo-cpu,id=f(cpuN,nodeN) > > > > ... > > > > where common options are pushed into global properties and only instance > > > > specific ones are specified per instance. > > > > > > > > Do you see any issues ahead that global properties would introduce? > > > > > > I see no issue, except that it was not what I was expecting at first. I > > > just don't know which one is less surprising for users (including > > > libvirt). > > users shouldn't be affected, -cpu will continue to works the same way until we > > start deprecating legacy behavior. > > I mean: users will be affected by our decision once they start using > device_add for CPU hotplug (then both approaches would have different > results). One more reason to make them global /i.e. to have the same behavior/ > > -- > Eduardo
On Thu, Dec 27, 2012 at 03:33:04PM +0100, Igor Mammedov wrote: > On Fri, 21 Dec 2012 11:50:26 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote: > > [...] > > > > All above said, I am not strongly against your approach, but I believe > > > > we could try to make the feature string parsing code generic and > > > > reusable (with target-specific properties or callbacks), instead of > > > > having to write a new generic feature string parsing function. > > > > > > > > Anyway, the above is just bikeshedding to me, and your approach is an > > > > improvement, already, even if we try to make the parser more generic > > > > later. > > > > > > > feature string parser in this series isn't meant to be generic, it's just > > > a simplification of the current function and splitting parsing > > > features & setting properties into separate steps. kind of what you did with > > > cpu_name+feature_string split. > > > > > > It's not clear to me what you mean under legacy properties, could you > > > throw in a hack using as example xlevel feature to demonstrate what you mean, > > > please? > > > > I will try to show it below, but note that we can try to do that _after_ > > we introduce the true/final/clean properties that won't have those > > hacks, and after changing the current parse_featurestr() implementation > > to contain the compatibility hacks (that's the work you are already > > doing). > > > > I was thinking about using the "legacy-%s" feature inside > > qdev_prop_parse(), this way: > > > > /* Generic featurestr parsing function */ > > void generic_parse_featurestr(CPUClass *cc, const char *featurestr) > > { > > opts = g_strsplit(featurestr, ",") > > for (each opt in opts) { > > if (opt[0] == '+') { > > dict[opt+1] = 'true'; > > } else if (opt[0] == ' > > dict[opt+1] = 'false ; > > } else if (opt contains '=) { > > key, value = g_strsplit(opt, "="); > > dict[key] = value; > > } > > } > It's only sparc and i386 targets that use +- format for features. > I'd rather avoid exposing +- format parsing to other targets. > BTW it's a bit more complex for i386 target due to f-* name conversion of > features into properties. > > We should work towards deprecation of this format and not introducing it to > other targets. When all this legacy stuff is deprecated we should have only > foo=xxx format left as the other devices, then there won't be need in > dedicated parser. OK, that would be a good reason to have a separate parser function. [...] > > > > > If common features are pushed in global properties, cpu hot-plug could look > > > > > like: > > > > > device_add driver=cpu_x86_foo_class,[apicid|id]=xxx > > > > > and all common features that user has overridden on command line would > > > > > applied to a new cpu without extra work from user. > > > > > > > > CPU hotplug is an interesting case: if thinking only about the CPUs > > > > created from the command-line the "-global vs -device" question wouldn't > > > > make such a big difference. But in the case of device_add for CPU > > > > hotplug, it would be very different. But I am not sure which option is > > > > less surprising for users. > > > > > > > > What others think? Andreas? Should "-cpu" affect only the CPUs created > > > > on initialization, or all CPUs created by -device/device_add for the > > > > lifetime of the QEMU process? (in other words, should the options to > > > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?) > > > > > > > > (BTW about the device_add example above: I believe we will want the > > > > hotplug interface to be based on a "cpu_index" parameter/property, to > > > > abstract out the complexity of the APIC ID calculation) > > > there was other opinion about cpu_index > > > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626 > > > > > > However if board will allocate sockets (Anthony suggested to try links for > > > this) then user won't have to calculate ID, instead of user will have to > > > enumerate existing cpu links and use IDs embedded in it for managing > > > cpus. > > > > I don't care if we use "IDs", "links", a "cpu_index" property, socket > > objects, socket IDs, or whatever. I just strongly recommend we don't > > force users/management-tools to calculate the APIC ID themselves. > > > > We could even kill the cpu_index field (as suggested at the URL above). > > But to allow the APIC ID to be calculated, we need to let the CPU know > > where it is "located" in the system (its > > cpu_index/socket_index/whatever), and the CPU topology of the system, > > somehow. > > > > (I believe asked multiple times why devices can't know their parents by > > design [so they can't just ask the sockets/motherboard for the > > topology], and I never got an answer...) > Can we create core_id/package_id/node_id properties for CPU and use them when > creating/hot-plugging CPUs for APICID calculation? That's how I think we have to do it, but It would be interesting to not require the caller to keep track of all those IDs and explicitly set them on the device_add arguments. I believe this is similar to the allocation of addresses in many device busses, so there may be an existing solution for this that I don't know about. > > [...] > > > > > > > > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage > > > > > of global properties as well. > > > > > > > > > > I think "-cpu foo-cpu,foo=x -smp n -numa node,cpus=..." could be > > > > > translated into something like: > > > > > -global foo-cpu.foo=x > > > > > -device foo-cpu,id=a,node=nodeA > > > > > -device foo-cpu,id=b,node=nodeB > > > > > ... > > > > > or > > > > > -device foo-cpu,id=f(cpuN,nodeN) > > > > > ... > > > > > where common options are pushed into global properties and only instance > > > > > specific ones are specified per instance. > > > > > > > > > > Do you see any issues ahead that global properties would introduce? > > > > > > > > I see no issue, except that it was not what I was expecting at first. I > > > > just don't know which one is less surprising for users (including > > > > libvirt). > > > users shouldn't be affected, -cpu will continue to works the same way until we > > > start deprecating legacy behavior. > > > > I mean: users will be affected by our decision once they start using > > device_add for CPU hotplug (then both approaches would have different > > results). > One more reason to make them global /i.e. to have the same behavior/ True. (That said, maybe we could even save the CPU model name and topology in global properties so we could just use "device_add cpu-package" [with no arguments] and have everything magically set [including the CPU model] depending on the globals?)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e075b59..a74d74b 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name) return 0; } +/* Set features on X86CPU object based on a provide key,value list */ +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp) +{ + const QDictEntry *ent; + + for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) { + const QString *qval = qobject_to_qstring(qdict_entry_value(ent)); + object_property_parse(OBJECT(cpu), qstring_get_str(qval), + qdict_entry_key(ent), errp); + if (error_is_set(errp)) { + return; + } + } +} + /* Parse "+feature,-feature,feature=foo" CPU feature string */ -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, + QDict **props) { unsigned int i; char *featurestr; /* Single 'key=value" string being parsed */ @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) uint32_t minus_kvm_features = 0, minus_svm_features = 0; uint32_t minus_7_0_ebx_features = 0; uint32_t numvalue; + gchar **feat_array = g_strsplit(features ? features : "", ",", 0); + *props = qdict_new(); + int j = 0; - featurestr = features ? strtok(features, ",") : NULL; - - while (featurestr) { + while ((featurestr = feat_array[j++])) { char *val; if (featurestr[0] == '+') { add_flagname_to_bitmaps(featurestr + 1, &plus_features, @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error; } - featurestr = strtok(NULL, ","); } x86_cpu_def->features |= plus_features; x86_cpu_def->ext_features |= plus_ext_features; @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features) x86_cpu_def->kvm_features &= ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features; x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features; + g_strfreev(feat_array); return 0; error: + g_strfreev(feat_array); return -1; } @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu) int cpu_x86_register(X86CPU *cpu, const char *cpu_model) { x86_def_t def1, *def = &def1; + QDict *props = NULL; Error *error = NULL; char *name, *features; gchar **model_pieces; @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) &def->ext3_features, &def->kvm_features, &def->svm_features, &def->cpuid_7_0_ebx_features); - if (cpu_x86_parse_featurestr(def, features) < 0) { + if (cpu_x86_parse_featurestr(def, features, &props) < 0) { error_setg(&error, "Invalid cpu_model string format: %s", cpu_model); goto out; } cpudef_2_x86_cpu(cpu, def, &error); + cpu_x86_set_props(cpu, props, &error); out: + QDECREF(props); g_strfreev(model_pieces); if (error) { fprintf(stderr, "%s\n", error_get_pretty(error));
It prepares for converting "+feature,-feature,feature=foo,feature" into a set of key,value property pairs that will be applied to CPU by cpu_x86_set_props(). Each feature handled by cpu_x86_parse_featurestr() will be converted into foo,val pair and a corresponding property setter by following patches. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 33 +++++++++++++++++++++++++++------ 1 files changed, 27 insertions(+), 6 deletions(-)