| Message ID | 1466693669-139967-6-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote: > custom apic-id setter/getter doesn't do any property specific > checks anymorer, so clean it up and use more compact static > property DEFINE_PROP_UINT32 instead. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 45 ++++++--------------------------------------- > 1 file changed, 6 insertions(+), 39 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 9511474..9294b3d 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; > } > > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name, > - void *opaque, Error **errp) > -{ > - X86CPU *cpu = X86_CPU(obj); > - int64_t value = cpu->apic_id; > - > - visit_type_int(v, name, &value, errp); > -} > - > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name, > - void *opaque, Error **errp) > -{ > - X86CPU *cpu = X86_CPU(obj); > - DeviceState *dev = DEVICE(obj); > - Error *error = NULL; > - int64_t value; > - > - if (dev->realized) { > - error_setg(errp, "Attempt to set property '%s' on '%s' after " > - "it was realized", name, object_get_typename(obj)); > - return; > - } > - > - visit_type_int(v, name, &value, &error); > - if (error) { > - error_propagate(errp, error); > - return; > - } > - cpu->apic_id = value; > -} > - > /* Generic getter for "feature-words" and "filtered-features" properties */ > static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > const char *name, void *opaque, > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj) > object_property_add(obj, "tsc-frequency", "int", > x86_cpuid_get_tsc_freq, > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > - object_property_add(obj, "apic-id", "int", > - x86_cpuid_get_apic_id, > - x86_cpuid_set_apic_id, NULL, NULL, NULL); > object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > x86_cpu_get_feature_words, > NULL, NULL, (void *)env->features, NULL); > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj) > > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > > -#ifndef CONFIG_USER_ONLY > - /* Any code creating new X86CPU objects have to set apic-id explicitly */ > - cpu->apic_id = UNASSIGNED_APIC_ID; > -#endif > - > for (w = 0; w < FEATURE_WORDS; w++) { > int bitnr; > > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs) > } > > static Property x86_cpu_properties[] = { > +#ifdef CONFIG_USER_ONLY > + /* apic_id = 0 by default for *-user, see commit 9886e834 */ > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0), > +#else > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID), > +#endif Suggestion for a follow-up patch: setting the default to UNASSIGNED_APIC_ID unconditionally, and just adding this to x86_cpu_realizefn(). #ifdef CONFIG_USER_ONLY if (cpu->apic_id == UNASSIGNED_APIC_ID) { cpu->apic_id = 0; } #endif
On Mon, 27 Jun 2016 14:55:24 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote: > > custom apic-id setter/getter doesn't do any property specific > > checks anymorer, so clean it up and use more compact static > > property DEFINE_PROP_UINT32 instead. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 45 ++++++--------------------------------------- > > 1 file changed, 6 insertions(+), 39 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 9511474..9294b3d 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > > cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; > > } > > > > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name, > > - void *opaque, Error **errp) > > -{ > > - X86CPU *cpu = X86_CPU(obj); > > - int64_t value = cpu->apic_id; > > - > > - visit_type_int(v, name, &value, errp); > > -} > > - > > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name, > > - void *opaque, Error **errp) > > -{ > > - X86CPU *cpu = X86_CPU(obj); > > - DeviceState *dev = DEVICE(obj); > > - Error *error = NULL; > > - int64_t value; > > - > > - if (dev->realized) { > > - error_setg(errp, "Attempt to set property '%s' on '%s' after " > > - "it was realized", name, object_get_typename(obj)); > > - return; > > - } > > - > > - visit_type_int(v, name, &value, &error); > > - if (error) { > > - error_propagate(errp, error); > > - return; > > - } > > - cpu->apic_id = value; > > -} > > - > > /* Generic getter for "feature-words" and "filtered-features" properties */ > > static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > > const char *name, void *opaque, > > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj) > > object_property_add(obj, "tsc-frequency", "int", > > x86_cpuid_get_tsc_freq, > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > - object_property_add(obj, "apic-id", "int", > > - x86_cpuid_get_apic_id, > > - x86_cpuid_set_apic_id, NULL, NULL, NULL); > > object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > > x86_cpu_get_feature_words, > > NULL, NULL, (void *)env->features, NULL); > > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj) > > > > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > > > > -#ifndef CONFIG_USER_ONLY > > - /* Any code creating new X86CPU objects have to set apic-id explicitly */ > > - cpu->apic_id = UNASSIGNED_APIC_ID; > > -#endif > > - > > for (w = 0; w < FEATURE_WORDS; w++) { > > int bitnr; > > > > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs) > > } > > > > static Property x86_cpu_properties[] = { > > +#ifdef CONFIG_USER_ONLY > > + /* apic_id = 0 by default for *-user, see commit 9886e834 */ > > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0), > > +#else > > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID), > > +#endif > > Suggestion for a follow-up patch: setting the default to > UNASSIGNED_APIC_ID unconditionally, and just adding this to > x86_cpu_realizefn(). > > #ifdef CONFIG_USER_ONLY > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > cpu->apic_id = 0; > } > #endif Putting default along with property definition seemed cleaner to me, considering that we want do similar thing for socket/core/thread-ids it still seems a better way. BTW: there is a v2 on list already, in case you missed it. [PATCH v2 00/18] pc: add CPU hot-add/hot-remove with device_add/device_del
On Tue, Jun 28, 2016 at 08:43:59AM +0200, Igor Mammedov wrote: > On Mon, 27 Jun 2016 14:55:24 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Jun 23, 2016 at 04:54:23PM +0200, Igor Mammedov wrote: > > > custom apic-id setter/getter doesn't do any property specific > > > checks anymorer, so clean it up and use more compact static > > > property DEFINE_PROP_UINT32 instead. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > target-i386/cpu.c | 45 ++++++--------------------------------------- > > > 1 file changed, 6 insertions(+), 39 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 9511474..9294b3d 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > > > cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; > > > } > > > > > > -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name, > > > - void *opaque, Error **errp) > > > -{ > > > - X86CPU *cpu = X86_CPU(obj); > > > - int64_t value = cpu->apic_id; > > > - > > > - visit_type_int(v, name, &value, errp); > > > -} > > > - > > > -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name, > > > - void *opaque, Error **errp) > > > -{ > > > - X86CPU *cpu = X86_CPU(obj); > > > - DeviceState *dev = DEVICE(obj); > > > - Error *error = NULL; > > > - int64_t value; > > > - > > > - if (dev->realized) { > > > - error_setg(errp, "Attempt to set property '%s' on '%s' after " > > > - "it was realized", name, object_get_typename(obj)); > > > - return; > > > - } > > > - > > > - visit_type_int(v, name, &value, &error); > > > - if (error) { > > > - error_propagate(errp, error); > > > - return; > > > - } > > > - cpu->apic_id = value; > > > -} > > > - > > > /* Generic getter for "feature-words" and "filtered-features" properties */ > > > static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > > > const char *name, void *opaque, > > > @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj) > > > object_property_add(obj, "tsc-frequency", "int", > > > x86_cpuid_get_tsc_freq, > > > x86_cpuid_set_tsc_freq, NULL, NULL, NULL); > > > - object_property_add(obj, "apic-id", "int", > > > - x86_cpuid_get_apic_id, > > > - x86_cpuid_set_apic_id, NULL, NULL, NULL); > > > object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", > > > x86_cpu_get_feature_words, > > > NULL, NULL, (void *)env->features, NULL); > > > @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj) > > > > > > cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; > > > > > > -#ifndef CONFIG_USER_ONLY > > > - /* Any code creating new X86CPU objects have to set apic-id explicitly */ > > > - cpu->apic_id = UNASSIGNED_APIC_ID; > > > -#endif > > > - > > > for (w = 0; w < FEATURE_WORDS; w++) { > > > int bitnr; > > > > > > @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs) > > > } > > > > > > static Property x86_cpu_properties[] = { > > > +#ifdef CONFIG_USER_ONLY > > > + /* apic_id = 0 by default for *-user, see commit 9886e834 */ > > > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0), > > > +#else > > > + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID), > > > +#endif > > > > Suggestion for a follow-up patch: setting the default to > > UNASSIGNED_APIC_ID unconditionally, and just adding this to > > x86_cpu_realizefn(). > > > > #ifdef CONFIG_USER_ONLY > > if (cpu->apic_id == UNASSIGNED_APIC_ID) { > > cpu->apic_id = 0; > > } > > #endif > Putting default along with property definition seemed cleaner to me, > considering that we want do similar thing for socket/core/thread-ids > it still seems a better way. Agreed that consistency is more important. It bothers me that we don't have a better mechanism to declare different defaults depending on CONFIG_USER_ONLY, but that's something to be discussed later. Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 9511474..9294b3d 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1824,37 +1824,6 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; } -static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - int64_t value = cpu->apic_id; - - visit_type_int(v, name, &value, errp); -} - -static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, const char *name, - void *opaque, Error **errp) -{ - X86CPU *cpu = X86_CPU(obj); - DeviceState *dev = DEVICE(obj); - Error *error = NULL; - int64_t value; - - if (dev->realized) { - error_setg(errp, "Attempt to set property '%s' on '%s' after " - "it was realized", name, object_get_typename(obj)); - return; - } - - visit_type_int(v, name, &value, &error); - if (error) { - error_propagate(errp, error); - return; - } - cpu->apic_id = value; -} - /* Generic getter for "feature-words" and "filtered-features" properties */ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, const char *name, void *opaque, @@ -3127,9 +3096,6 @@ static void x86_cpu_initfn(Object *obj) object_property_add(obj, "tsc-frequency", "int", x86_cpuid_get_tsc_freq, x86_cpuid_set_tsc_freq, NULL, NULL, NULL); - object_property_add(obj, "apic-id", "int", - x86_cpuid_get_apic_id, - x86_cpuid_set_apic_id, NULL, NULL, NULL); object_property_add(obj, "feature-words", "X86CPUFeatureWordInfo", x86_cpu_get_feature_words, NULL, NULL, (void *)env->features, NULL); @@ -3139,11 +3105,6 @@ static void x86_cpu_initfn(Object *obj) cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY; -#ifndef CONFIG_USER_ONLY - /* Any code creating new X86CPU objects have to set apic-id explicitly */ - cpu->apic_id = UNASSIGNED_APIC_ID; -#endif - for (w = 0; w < FEATURE_WORDS; w++) { int bitnr; @@ -3200,6 +3161,12 @@ static bool x86_cpu_has_work(CPUState *cs) } static Property x86_cpu_properties[] = { +#ifdef CONFIG_USER_ONLY + /* apic_id = 0 by default for *-user, see commit 9886e834 */ + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, 0), +#else + DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID), +#endif DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), { .name = "hv-spinlocks", .info = &qdev_prop_spinlocks }, DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, false),
custom apic-id setter/getter doesn't do any property specific checks anymorer, so clean it up and use more compact static property DEFINE_PROP_UINT32 instead. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 45 ++++++--------------------------------------- 1 file changed, 6 insertions(+), 39 deletions(-)