| Message ID | 1428519763-21644-4-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 08, 2015 at 04:02:42PM -0300, Eduardo Habkost wrote: [...] > + names = g_strsplit(fi->feat_names[bit], "|", 0); I forgot to implement the property aliases Igor asked for. I will submit v3 of this patch later. > + for (i = 0; names[i]; i++) { > + char *feat_name = names[i]; > + feat2prop(feat_name); > + char *prop_name = g_strdup_printf("cpuid-%s", feat_name); > + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit)); > + g_free(prop_name); > + } > + g_strfreev(names); > +} > +
On Thu, 9 Apr 2015 15:48:30 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Apr 08, 2015 at 04:02:42PM -0300, Eduardo Habkost wrote: > [...] > > + names = g_strsplit(fi->feat_names[bit], "|", 0); > > I forgot to implement the property aliases Igor asked for. I will > submit v3 of this patch later. > > > + for (i = 0; names[i]; i++) { > > + char *feat_name = names[i]; > > + feat2prop(feat_name); > > + char *prop_name = g_strdup_printf("cpuid-%s", feat_name); BTW: I've remembered why we've chosen feat- vs. cpuid- prefix it was to make CPU features platform neutral so that libvirt would use the same prefix for x86, arm other targets. > > + x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << > > bit)); > > + g_free(prop_name); > > + } > > + g_strfreev(names); > > +} > > +
On 10/04/2015 09:27, Igor Mammedov wrote: > > + for (i = 0; names[i]; i++) { > > + char *feat_name = names[i]; > > + feat2prop(feat_name); > > + char *prop_name = g_strdup_printf("cpuid-%s", feat_name); > > BTW: I've remembered why we've chosen feat- vs. cpuid- prefix > it was to make CPU features platform neutral so that libvirt > would use the same prefix for x86, arm other targets. Ok, that make sense, but if we want to make it platform-neutral, let's spell it "feature-" or remove the prefix altogether. If we remove it, perhaps we could add a QOM property with the list of features? But I don't want to bikeshed too much. Paolo
Am 10.04.2015 um 10:21 schrieb Paolo Bonzini: > On 10/04/2015 09:27, Igor Mammedov wrote: >>> + for (i = 0; names[i]; i++) { >>> + char *feat_name = names[i]; >>> + feat2prop(feat_name); >>> + char *prop_name = g_strdup_printf("cpuid-%s", feat_name); >> >> BTW: I've remembered why we've chosen feat- vs. cpuid- prefix >> it was to make CPU features platform neutral so that libvirt >> would use the same prefix for x86, arm other targets. > > Ok, that make sense, but if we want to make it platform-neutral, let's > spell it "feature-" or remove the prefix altogether. > > If we remove it, perhaps we could add a QOM property with the list of > features? > > But I don't want to bikeshed too much. I had suggested a container sub-object for property grouping but Anthony preferred a prefix. Btw a suffix could work as well and would read more natural. QMP don't sort alphabetically anyway. Andreas
On 10/04/2015 10:53, Andreas Färber wrote: > > Ok, that make sense, but if we want to make it platform-neutral, let's > > spell it "feature-" or remove the prefix altogether. > > > > If we remove it, perhaps we could add a QOM property with the list of > > features? > > > > But I don't want to bikeshed too much. > > I had suggested a container sub-object for property grouping but Anthony > preferred a prefix. And I agree. :) However, it seems to me that properties such as feature-words and filtered-features are a good precedent so we might as well add all-features. > Btw a suffix could work as well and would read more natural. QMP don't > sort alphabetically anyway. QMP doesn't, but the front-end (scripts/qom-list for example) could. Paolo
Am 10.04.2015 um 10:59 schrieb Paolo Bonzini: > On 10/04/2015 10:53, Andreas Färber wrote: >>> Ok, that make sense, but if we want to make it platform-neutral, let's >>> spell it "feature-" or remove the prefix altogether. >>> >>> If we remove it, perhaps we could add a QOM property with the list of >>> features? >>> >>> But I don't want to bikeshed too much. [...] >> Btw a suffix could work as well and would read more natural. QMP don't >> sort alphabetically anyway. > > QMP doesn't, but the front-end (scripts/qom-list for example) could. I don't think our scripts should modify QMP responses. Some commands transmit things in reverse and we should find out why and fix them instead of papering over it. In particular for scripts/qom-tree's sibling nodes (qom-list) I find that very annoying. Andreas
On Fri, Apr 10, 2015 at 10:21:49AM +0200, Paolo Bonzini wrote: > > > On 10/04/2015 09:27, Igor Mammedov wrote: > > > + for (i = 0; names[i]; i++) { > > > + char *feat_name = names[i]; > > > + feat2prop(feat_name); > > > + char *prop_name = g_strdup_printf("cpuid-%s", feat_name); > > > > BTW: I've remembered why we've chosen feat- vs. cpuid- prefix > > it was to make CPU features platform neutral so that libvirt > > would use the same prefix for x86, arm other targets. > > Ok, that make sense, but if we want to make it platform-neutral, let's > spell it "feature-" or remove the prefix altogether. I am liking the idea of removing the prefix. I can't find a compelling reason for making boolean properties that affect CPUID special and different from other boolean properties in X86CPU. > > If we remove it, perhaps we could add a QOM property with the list of > features? I am not even sure this will be necessary. libvirt, for example, is already aware of the list of CPU feature flags it can set/unset, so it probably wouldn't need the list. If it becomes necessary, we can add it. I will send v4 without the prefix.
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e657f10..7099027 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2848,12 +2848,118 @@ 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); + DeviceState *dev = DEVICE(obj); + CPUX86State *env = &cpu->env; + FeatureProperty *fp = opaque; + bool value; + + if (dev->realized) { + qdev_prop_set_after_realize(dev, name, errp); + return; + } + + visit_type_bool(v, &value, name, errp); + if (value) { + env->features[fp->word] |= fp->mask; + } else { + env->features[fp->word] &= ~fp->mask; + } +} + +static void x86_cpu_release_feature_prop(Object *obj, const char *name, + void *opaque) +{ + FeatureProperty *prop = opaque; + g_free(prop); +} + +/* 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, + x86_cpu_release_feature_prop, 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]; + feat2prop(feat_name); + char *prop_name = g_strdup_printf("cpuid-%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; @@ -2894,6 +3000,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> --- Changes v1 -> v2: * Use "cpuid-" prefix instead of "feat-" * Register release function for property * Convert '_' to '-' on feature name before registering property * Add dev->realized check to property setter --- target-i386/cpu.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+)