| Message ID | 1428439603-8549-4-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/04/2015 22:46, Eduardo Habkost wrote: > This uses the feature name arrays to register "feat-*" QOM properties > for feature flags. This simply adds the properties so they can be > configured using -global, but doesn't change x86_cpu_parse_featurestr() > to use them yet. Out of curiosity, why the prefix? (Also, perhaps a prefix such as "cpuid-*" would be better since the property often only affects the cpuid leaves, rather than the availability of the feature itself). Paolo > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 099ed03..f29e55e 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2883,12 +2883,103 @@ out: > } > } > > +typedef struct FeatureProperty { > + FeatureWord word; > + uint32_t mask; > +} FeatureProperty; > + > + > +static void x86_cpu_get_feature_prop(Object *obj, > + struct Visitor *v, > + void *opaque, > + const char *name, > + Error **errp) > +{ > + X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &cpu->env; > + FeatureProperty *fp = opaque; > + bool value = (env->features[fp->word] & fp->mask) == fp->mask; > + visit_type_bool(v, &value, name, errp); > +} > + > +static void x86_cpu_set_feature_prop(Object *obj, > + struct Visitor *v, > + void *opaque, > + const char *name, > + Error **errp) > +{ > + X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &cpu->env; > + FeatureProperty *fp = opaque; > + bool value; > + visit_type_bool(v, &value, name, errp); > + if (value) { > + env->features[fp->word] |= fp->mask; > + } else { > + env->features[fp->word] &= ~fp->mask; > + } > +} > + > +/* Register a boolean feature-bits property. > + * If mask has multiple bits, all must be set for the property to return true. > + * The same property name can be registered multiple times to make it affect > + * multiple bits in the same FeatureWord. > + */ > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > + const char *prop_name, > + FeatureWord w, > + uint32_t mask) > +{ > + FeatureProperty *fp; > + ObjectProperty *op; > + op = object_property_find(OBJECT(cpu), prop_name, NULL); > + if (op) { > + fp = op->opaque; > + assert(fp->word == w); > + fp->mask |= mask; > + } else { > + fp = g_new0(FeatureProperty, 1); > + fp->word = w; > + fp->mask = mask; > + object_property_add(OBJECT(cpu), prop_name, "bool", > + x86_cpu_get_feature_prop, > + x86_cpu_set_feature_prop, > + NULL, fp, &error_abort); > + } > +} > + > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, > + FeatureWord w, > + int bit) > +{ > + int i; > + char **names; > + FeatureWordInfo *fi = &feature_word_info[w]; > + > + if (!fi->feat_names) { > + return; > + } > + if (!fi->feat_names[bit]) { > + return; > + } > + > + names = g_strsplit(fi->feat_names[bit], "|", 0); > + for (i = 0; names[i]; i++) { > + char *feat_name = names[i]; > + char *prop_name = g_strdup_printf("feat-%s", feat_name); > + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit)); > + g_free(prop_name); > + } > + g_strfreev(names); > +} > + > static void x86_cpu_initfn(Object *obj) > { > CPUState *cs = CPU(obj); > X86CPU *cpu = X86_CPU(obj); > X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); > CPUX86State *env = &cpu->env; > + FeatureWord w; > static int inited; > > cs->env_ptr = env; > @@ -2935,6 +3026,13 @@ static void x86_cpu_initfn(Object *obj) > cpu->apic_id = -1; > #endif > > + for (w = 0; w < FEATURE_WORDS; w++) { > + int bit; > + for (bit = 0; bit < 32; bit++) { > + x86_cpu_register_feature_bit_props(cpu, w, bit); > + } > + } > + > x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); > > /* init various static tables used in TCG mode */ >
On Wed, Apr 08, 2015 at 10:30:52AM +0200, Paolo Bonzini wrote: > > > On 07/04/2015 22:46, Eduardo Habkost wrote: > > This uses the feature name arrays to register "feat-*" QOM properties > > for feature flags. This simply adds the properties so they can be > > configured using -global, but doesn't change x86_cpu_parse_featurestr() > > to use them yet. > > Out of curiosity, why the prefix? (Also, perhaps a prefix such as > "cpuid-*" would be better since the property often only affects the > cpuid leaves, rather than the availability of the feature itself). The prefix exists to allow those properties to be easily identified by software that doesn't know the full list of feature names (I even took advantage of that on the x86-cpu-model-dump script). About the reason for using the "feat-" prefix, the short answer is "it's the prefix used in the last patch that implemented this (by Igor)". I think the first suggestion was to use "f-", then we changed to "feature-" or "feat-", and simply stayed using "feat-" in the last few versions. But I like the "cpuid-" suggestion and plan to use it on v2. Any objections?
On Tue, 7 Apr 2015 17:46:40 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > This uses the feature name arrays to register "feat-*" QOM properties > for feature flags. This simply adds the properties so they can be > configured using -global, but doesn't change x86_cpu_parse_featurestr() > to use them yet. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 099ed03..f29e55e 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2883,12 +2883,103 @@ out: > } > } > > +typedef struct FeatureProperty { > + FeatureWord word; > + uint32_t mask; > +} FeatureProperty; > + > + > +static void x86_cpu_get_feature_prop(Object *obj, > + struct Visitor *v, > + void *opaque, > + const char *name, > + Error **errp) > +{ > + X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &cpu->env; > + FeatureProperty *fp = opaque; > + bool value = (env->features[fp->word] & fp->mask) == fp->mask; > + visit_type_bool(v, &value, name, errp); > +} > + > +static void x86_cpu_set_feature_prop(Object *obj, > + struct Visitor *v, > + void *opaque, > + const char *name, > + Error **errp) > +{ > + X86CPU *cpu = X86_CPU(obj); > + CPUX86State *env = &cpu->env; > + FeatureProperty *fp = opaque; > + bool value; > + visit_type_bool(v, &value, name, errp); > + if (value) { > + env->features[fp->word] |= fp->mask; > + } else { > + env->features[fp->word] &= ~fp->mask; > + } > +} > + > +/* Register a boolean feature-bits property. > + * If mask has multiple bits, all must be set for the property to return true. > + * The same property name can be registered multiple times to make it affect > + * multiple bits in the same FeatureWord. > + */ > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > + const char *prop_name, > + FeatureWord w, > + uint32_t mask) > +{ > + FeatureProperty *fp; > + ObjectProperty *op; > + op = object_property_find(OBJECT(cpu), prop_name, NULL); > + if (op) { > + fp = op->opaque; > + assert(fp->word == w); > + fp->mask |= mask; > + } else { > + fp = g_new0(FeatureProperty, 1); > + fp->word = w; > + fp->mask = mask; > + object_property_add(OBJECT(cpu), prop_name, "bool", > + x86_cpu_get_feature_prop, > + x86_cpu_set_feature_prop, > + NULL, fp, &error_abort); > + } > +} it would be better to create generic bit property and replace above code with it something similar to object_property_add_uint32_ptr() > + > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, > + FeatureWord w, > + int bit) > +{ > + int i; > + char **names; > + FeatureWordInfo *fi = &feature_word_info[w]; > + > + if (!fi->feat_names) { > + return; > + } > + if (!fi->feat_names[bit]) { > + return; > + } > + > + names = g_strsplit(fi->feat_names[bit], "|", 0); > + for (i = 0; names[i]; i++) { > + char *feat_name = names[i]; > + char *prop_name = g_strdup_printf("feat-%s", feat_name); > + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit)); it might be better instead of creating duplicate property to make an alias
On Wed, Apr 08, 2015 at 01:36:29PM +0200, Igor Mammedov wrote: > On Tue, 7 Apr 2015 17:46:40 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > This uses the feature name arrays to register "feat-*" QOM properties > > for feature flags. This simply adds the properties so they can be > > configured using -global, but doesn't change x86_cpu_parse_featurestr() > > to use them yet. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 98 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 099ed03..f29e55e 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2883,12 +2883,103 @@ out: > > } > > } > > > > +typedef struct FeatureProperty { > > + FeatureWord word; > > + uint32_t mask; > > +} FeatureProperty; > > + > > + > > +static void x86_cpu_get_feature_prop(Object *obj, > > + struct Visitor *v, > > + void *opaque, > > + const char *name, > > + Error **errp) > > +{ > > + X86CPU *cpu = X86_CPU(obj); > > + CPUX86State *env = &cpu->env; > > + FeatureProperty *fp = opaque; > > + bool value = (env->features[fp->word] & fp->mask) == fp->mask; > > + visit_type_bool(v, &value, name, errp); > > +} > > + > > +static void x86_cpu_set_feature_prop(Object *obj, > > + struct Visitor *v, > > + void *opaque, > > + const char *name, > > + Error **errp) > > +{ > > + X86CPU *cpu = X86_CPU(obj); > > + CPUX86State *env = &cpu->env; > > + FeatureProperty *fp = opaque; > > + bool value; > > + visit_type_bool(v, &value, name, errp); > > + if (value) { > > + env->features[fp->word] |= fp->mask; > > + } else { > > + env->features[fp->word] &= ~fp->mask; > > + } > > +} > > + > > +/* Register a boolean feature-bits property. > > + * If mask has multiple bits, all must be set for the property to return true. > > + * The same property name can be registered multiple times to make it affect > > + * multiple bits in the same FeatureWord. > > + */ > > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > > + const char *prop_name, > > + FeatureWord w, > > + uint32_t mask) > > +{ > > + FeatureProperty *fp; > > + ObjectProperty *op; > > + op = object_property_find(OBJECT(cpu), prop_name, NULL); > > + if (op) { > > + fp = op->opaque; > > + assert(fp->word == w); > > + fp->mask |= mask; > > + } else { > > + fp = g_new0(FeatureProperty, 1); > > + fp->word = w; > > + fp->mask = mask; > > + object_property_add(OBJECT(cpu), prop_name, "bool", > > + x86_cpu_get_feature_prop, > > + x86_cpu_set_feature_prop, > > + NULL, fp, &error_abort); > > + } > > +} > it would be better to create generic bit property and replace above code with it > something similar to object_property_add_uint32_ptr() object_property_add_*_ptr() adds read-only properties, and I didn't want to make object_property_add_bit_ptr() inconsistent with the other functions. But maybe it is better to have an inconsistent but reusable API than making the new code non-reusable by keeping it inside target-i386/cpu.c. I will give it a try. BTW, it is on my wishlist to remove the existing duplication in DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to support the same data types without duplicating code, but this may take a while. > > > > + > > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, > > + FeatureWord w, > > + int bit) > > +{ > > + int i; > > + char **names; > > + FeatureWordInfo *fi = &feature_word_info[w]; > > + > > + if (!fi->feat_names) { > > + return; > > + } > > + if (!fi->feat_names[bit]) { > > + return; > > + } > > + > > + names = g_strsplit(fi->feat_names[bit], "|", 0); > > + for (i = 0; names[i]; i++) { > > + char *feat_name = names[i]; > > + char *prop_name = g_strdup_printf("feat-%s", feat_name); > > + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit)); > it might be better instead of creating duplicate property to make an alias I wasn't aware of property aliases. I will take a look. Thanks!
On 08/04/2015 14:20, Eduardo Habkost wrote: >> > it would be better to create generic bit property and replace above code with it >> > something similar to object_property_add_uint32_ptr() > object_property_add_*_ptr() adds read-only properties, and I didn't want > to make object_property_add_bit_ptr() inconsistent with the other > functions. But maybe it is better to have an inconsistent but reusable > API than making the new code non-reusable by keeping it inside > target-i386/cpu.c. I will give it a try. add_*_ptr() is read-only because read-only properties do not require validations (at least that's the common case). So I think the inconsistent API is worse than a local one. > BTW, it is on my wishlist to remove the existing duplication in > DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to > support the same data types without duplicating code, but this may take > a while. Yeah, that would be nice... Paolo
Am 08.04.2015 um 13:06 schrieb Eduardo Habkost: > On Wed, Apr 08, 2015 at 10:30:52AM +0200, Paolo Bonzini wrote: >> On 07/04/2015 22:46, Eduardo Habkost wrote: >>> This uses the feature name arrays to register "feat-*" QOM properties >>> for feature flags. This simply adds the properties so they can be >>> configured using -global, but doesn't change x86_cpu_parse_featurestr() >>> to use them yet. >> >> Out of curiosity, why the prefix? (Also, perhaps a prefix such as >> "cpuid-*" would be better since the property often only affects the >> cpuid leaves, rather than the availability of the feature itself). > > The prefix exists to allow those properties to be easily identified by > software that doesn't know the full list of feature names (I even took > advantage of that on the x86-cpu-model-dump script). > > About the reason for using the "feat-" prefix, the short answer is "it's > the prefix used in the last patch that implemented this (by Igor)". I > think the first suggestion was to use "f-", then we changed to > "feature-" or "feat-", and simply stayed using "feat-" in the last few > versions. Fair to mention that they did not all get equal review. ;) > But I like the "cpuid-" suggestion and plan to use it on v2. Any > objections? Assuming it's technically correct, +1 for cpuid-. Andreas
On Wed, 08 Apr 2015 14:24:35 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 08/04/2015 14:20, Eduardo Habkost wrote: > >> > it would be better to create generic bit property and replace above code with it > >> > something similar to object_property_add_uint32_ptr() > > object_property_add_*_ptr() adds read-only properties, and I didn't want > > to make object_property_add_bit_ptr() inconsistent with the other > > functions. But maybe it is better to have an inconsistent but reusable > > API than making the new code non-reusable by keeping it inside > > target-i386/cpu.c. I will give it a try. > > add_*_ptr() is read-only because read-only properties do not require > validations (at least that's the common case). So I think the > inconsistent API is worse than a local one. I've had on my todo list adding add_*_ptr_RW() variants because in common case there isn't any need in validation and with current API we are forced to duplicate custom setter/getter every time for trivial type. For example properties like "level" or "xlevel". The same goes for generic bitmap setter (i.e.) without validation, which is what we need in this case. We [could] do validation at realize time when it's possible to analyze combination of feature flags. > > > BTW, it is on my wishlist to remove the existing duplication in > > DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to > > support the same data types without duplicating code, but this may take > > a while. > > Yeah, that would be nice... > > Paolo >
On Wed, Apr 08, 2015 at 04:09:32PM +0200, Igor Mammedov wrote: > On Wed, 08 Apr 2015 14:24:35 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > On 08/04/2015 14:20, Eduardo Habkost wrote: > > >> > it would be better to create generic bit property and replace above code with it > > >> > something similar to object_property_add_uint32_ptr() > > > object_property_add_*_ptr() adds read-only properties, and I didn't want > > > to make object_property_add_bit_ptr() inconsistent with the other > > > functions. But maybe it is better to have an inconsistent but reusable > > > API than making the new code non-reusable by keeping it inside > > > target-i386/cpu.c. I will give it a try. > > > > add_*_ptr() is read-only because read-only properties do not require > > validations (at least that's the common case). So I think the > > inconsistent API is worse than a local one. > I've had on my todo list adding add_*_ptr_RW() variants > because in common case there isn't any need in validation and > with current API we are forced to duplicate custom setter/getter > every time for trivial type. > For example properties like "level" or "xlevel". > > The same goes for generic bitmap setter (i.e.) without validation, > which is what we need in this case. We [could] do validation at realize > time when it's possible to analyze combination of feature flags. Even if no property value validation is done at the setter, we still want to prevent the property from being changed after realize (which is something I forgot to do in this patch, and we forgot to do on most X86CPU setters).
On Wed, 8 Apr 2015 12:01:26 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 08, 2015 at 04:09:32PM +0200, Igor Mammedov wrote: > > On Wed, 08 Apr 2015 14:24:35 +0200 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > > > On 08/04/2015 14:20, Eduardo Habkost wrote: > > > >> > it would be better to create generic bit property and replace above code with it > > > >> > something similar to object_property_add_uint32_ptr() > > > > object_property_add_*_ptr() adds read-only properties, and I didn't want > > > > to make object_property_add_bit_ptr() inconsistent with the other > > > > functions. But maybe it is better to have an inconsistent but reusable > > > > API than making the new code non-reusable by keeping it inside > > > > target-i386/cpu.c. I will give it a try. > > > > > > add_*_ptr() is read-only because read-only properties do not require > > > validations (at least that's the common case). So I think the > > > inconsistent API is worse than a local one. > > I've had on my todo list adding add_*_ptr_RW() variants > > because in common case there isn't any need in validation and > > with current API we are forced to duplicate custom setter/getter > > every time for trivial type. > > For example properties like "level" or "xlevel". > > > > The same goes for generic bitmap setter (i.e.) without validation, > > which is what we need in this case. We [could] do validation at realize > > time when it's possible to analyze combination of feature flags. > > Even if no property value validation is done at the setter, we still > want to prevent the property from being changed after realize (which is > something I forgot to do in this patch, and we forgot to do on most > X86CPU setters). Yep, the same goes for every generic setters in current API since realize is device specific thingy that's why it's supported by static properties only now. We should have something like Stefan's object_property_add_link(...,*check,...) but for generic properties.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 099ed03..f29e55e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2883,12 +2883,103 @@ out: } } +typedef struct FeatureProperty { + FeatureWord word; + uint32_t mask; +} FeatureProperty; + + +static void x86_cpu_get_feature_prop(Object *obj, + struct Visitor *v, + void *opaque, + const char *name, + Error **errp) +{ + X86CPU *cpu = X86_CPU(obj); + CPUX86State *env = &cpu->env; + FeatureProperty *fp = opaque; + bool value = (env->features[fp->word] & fp->mask) == fp->mask; + visit_type_bool(v, &value, name, errp); +} + +static void x86_cpu_set_feature_prop(Object *obj, + struct Visitor *v, + void *opaque, + const char *name, + Error **errp) +{ + X86CPU *cpu = X86_CPU(obj); + CPUX86State *env = &cpu->env; + FeatureProperty *fp = opaque; + bool value; + visit_type_bool(v, &value, name, errp); + if (value) { + env->features[fp->word] |= fp->mask; + } else { + env->features[fp->word] &= ~fp->mask; + } +} + +/* Register a boolean feature-bits property. + * If mask has multiple bits, all must be set for the property to return true. + * The same property name can be registered multiple times to make it affect + * multiple bits in the same FeatureWord. + */ +static void x86_cpu_register_feature_prop(X86CPU *cpu, + const char *prop_name, + FeatureWord w, + uint32_t mask) +{ + FeatureProperty *fp; + ObjectProperty *op; + op = object_property_find(OBJECT(cpu), prop_name, NULL); + if (op) { + fp = op->opaque; + assert(fp->word == w); + fp->mask |= mask; + } else { + fp = g_new0(FeatureProperty, 1); + fp->word = w; + fp->mask = mask; + object_property_add(OBJECT(cpu), prop_name, "bool", + x86_cpu_get_feature_prop, + x86_cpu_set_feature_prop, + NULL, fp, &error_abort); + } +} + +static void x86_cpu_register_feature_bit_props(X86CPU *cpu, + FeatureWord w, + int bit) +{ + int i; + char **names; + FeatureWordInfo *fi = &feature_word_info[w]; + + if (!fi->feat_names) { + return; + } + if (!fi->feat_names[bit]) { + return; + } + + names = g_strsplit(fi->feat_names[bit], "|", 0); + for (i = 0; names[i]; i++) { + char *feat_name = names[i]; + char *prop_name = g_strdup_printf("feat-%s", feat_name); + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit)); + g_free(prop_name); + } + g_strfreev(names); +} + static void x86_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); X86CPU *cpu = X86_CPU(obj); X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); CPUX86State *env = &cpu->env; + FeatureWord w; static int inited; cs->env_ptr = env; @@ -2935,6 +3026,13 @@ static void x86_cpu_initfn(Object *obj) cpu->apic_id = -1; #endif + for (w = 0; w < FEATURE_WORDS; w++) { + int bit; + for (bit = 0; bit < 32; bit++) { + x86_cpu_register_feature_bit_props(cpu, w, bit); + } + } + x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort); /* init various static tables used in TCG mode */
This uses the feature name arrays to register "feat-*" QOM properties for feature flags. This simply adds the properties so they can be configured using -global, but doesn't change x86_cpu_parse_featurestr() to use them yet. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+)