| Message ID | 1408044362-11621-28-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote: > The "kvmclock" feature is special because it affects two bits in the KVM > CPUID leaf, so it has to be handled differently from the other feature > properties that will be added. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index b005b0d..0eb401b 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) > } > } > > +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. > + */ > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > + const char *prop_name, > + FeatureWord w, > + uint32_t mask) > +{ > + FeatureProperty *fp; > + fp = g_new0(FeatureProperty, 1); > + fp->word = w; > + fp->mask = mask; > + object_property_add(OBJECT(cpu), prop_name, "bool", > + x86_cpu_set_feature_prop, > + x86_cpu_get_feature_prop, > + NULL, fp, &error_abort); > +} > + This looks similar to what what DEFINE_PROP_BIT does. Can't this be reused in some way? > static void x86_cpu_initfn(Object *obj) > { > CPUState *cs = CPU(obj); > @@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj) > x86_cpu_get_feature_words, > NULL, NULL, (void *)cpu->filtered_features, NULL); > > + /* "feat-kvmclock" will affect both kvmclock feature bits */ > + x86_cpu_register_feature_prop(cpu, "feat-kvmclock", FEAT_KVM, > + (1UL << KVM_FEATURE_CLOCKSOURCE) | > + (1UL << KVM_FEATURE_CLOCKSOURCE2)); > + > + > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); > > -- > 1.9.3
On Thu, Aug 14, 2014 at 11:08:30PM +0200, Michael S. Tsirkin wrote: > On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote: > > The "kvmclock" feature is special because it affects two bits in the KVM > > CPUID leaf, so it has to be handled differently from the other feature > > properties that will be added. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index b005b0d..0eb401b 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) > > } > > } > > > > +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. > > + */ > > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > > + const char *prop_name, > > + FeatureWord w, > > + uint32_t mask) > > +{ > > + FeatureProperty *fp; > > + fp = g_new0(FeatureProperty, 1); > > + fp->word = w; > > + fp->mask = mask; > > + object_property_add(OBJECT(cpu), prop_name, "bool", > > + x86_cpu_set_feature_prop, > > + x86_cpu_get_feature_prop, > > + NULL, fp, &error_abort); > > +} > > + > > This looks similar to what what DEFINE_PROP_BIT does. > Can't this be reused in some way? DEFINE_PROP_BIT is from the static property system, and I understand we are preferring using object_property_add*() instead (and in the X86CPU features case, registering the properties dynamically using the feature name arrays saves us a lot of work). I will take a look at the DEFINE_PROP_BIT code to see if anything from that code can be reused, but I doubt so. It seems to be tightly coupled to the static property system. > > > > static void x86_cpu_initfn(Object *obj) > > { > > CPUState *cs = CPU(obj); > > @@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj) > > x86_cpu_get_feature_words, > > NULL, NULL, (void *)cpu->filtered_features, NULL); > > > > + /* "feat-kvmclock" will affect both kvmclock feature bits */ > > + x86_cpu_register_feature_prop(cpu, "feat-kvmclock", FEAT_KVM, > > + (1UL << KVM_FEATURE_CLOCKSOURCE) | > > + (1UL << KVM_FEATURE_CLOCKSOURCE2)); > > + > > + > > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); > > > > -- > > 1.9.3
On Thu, Aug 14, 2014 at 08:59:17PM -0300, Eduardo Habkost wrote: > On Thu, Aug 14, 2014 at 11:08:30PM +0200, Michael S. Tsirkin wrote: > > On Thu, Aug 14, 2014 at 04:25:56PM -0300, Eduardo Habkost wrote: > > > The "kvmclock" feature is special because it affects two bits in the KVM > > > CPUID leaf, so it has to be handled differently from the other feature > > > properties that will be added. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > target-i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index b005b0d..0eb401b 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) > > > } > > > } > > > > > > +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. > > > + */ > > > +static void x86_cpu_register_feature_prop(X86CPU *cpu, > > > + const char *prop_name, > > > + FeatureWord w, > > > + uint32_t mask) > > > +{ > > > + FeatureProperty *fp; > > > + fp = g_new0(FeatureProperty, 1); > > > + fp->word = w; > > > + fp->mask = mask; > > > + object_property_add(OBJECT(cpu), prop_name, "bool", > > > + x86_cpu_set_feature_prop, > > > + x86_cpu_get_feature_prop, > > > + NULL, fp, &error_abort); > > > +} > > > + > > > > This looks similar to what what DEFINE_PROP_BIT does. > > Can't this be reused in some way? > > DEFINE_PROP_BIT is from the static property system, and I understand we > are preferring using object_property_add*() instead (and in the X86CPU > features case, registering the properties dynamically using the feature > name arrays saves us a lot of work). > > I will take a look at the DEFINE_PROP_BIT code to see if anything from > that code can be reused, but I doubt so. It seems to be tightly coupled > to the static property system. Main point is, can't we find a way to reduce code duplication? It doesn't seem reasonable that we basically open-code each property from scratch. > > > > > > > static void x86_cpu_initfn(Object *obj) > > > { > > > CPUState *cs = CPU(obj); > > > @@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj) > > > x86_cpu_get_feature_words, > > > NULL, NULL, (void *)cpu->filtered_features, NULL); > > > > > > + /* "feat-kvmclock" will affect both kvmclock feature bits */ > > > + x86_cpu_register_feature_prop(cpu, "feat-kvmclock", FEAT_KVM, > > > + (1UL << KVM_FEATURE_CLOCKSOURCE) | > > > + (1UL << KVM_FEATURE_CLOCKSOURCE2)); > > > + > > > + > > > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > > > env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); > > > > > > -- > > > 1.9.3 > > -- > Eduardo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index b005b0d..0eb401b 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2774,6 +2774,61 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index) } } +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. + */ +static void x86_cpu_register_feature_prop(X86CPU *cpu, + const char *prop_name, + FeatureWord w, + uint32_t mask) +{ + FeatureProperty *fp; + fp = g_new0(FeatureProperty, 1); + fp->word = w; + fp->mask = mask; + object_property_add(OBJECT(cpu), prop_name, "bool", + x86_cpu_set_feature_prop, + x86_cpu_get_feature_prop, + NULL, fp, &error_abort); +} + static void x86_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -2819,6 +2874,12 @@ static void x86_cpu_initfn(Object *obj) x86_cpu_get_feature_words, NULL, NULL, (void *)cpu->filtered_features, NULL); + /* "feat-kvmclock" will affect both kvmclock feature bits */ + x86_cpu_register_feature_prop(cpu, "feat-kvmclock", FEAT_KVM, + (1UL << KVM_FEATURE_CLOCKSOURCE) | + (1UL << KVM_FEATURE_CLOCKSOURCE2)); + + cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
The "kvmclock" feature is special because it affects two bits in the KVM CPUID leaf, so it has to be handled differently from the other feature properties that will be added. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)