From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53357) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHmkj-0007HP-O7 for qemu-devel@nongnu.org; Tue, 28 Jun 2016 02:44:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHmke-00037n-Ll for qemu-devel@nongnu.org; Tue, 28 Jun 2016 02:44:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41336) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHmke-00037a-Bw for qemu-devel@nongnu.org; Tue, 28 Jun 2016 02:44:04 -0400 Date: Tue, 28 Jun 2016 08:43:59 +0200 From: Igor Mammedov Message-ID: <20160628084359.5bed6d53@nial.brq.redhat.com> In-Reply-To: <20160627175524.GQ3332@thinpad.lan.raisama.net> References: <1466693669-139967-1-git-send-email-imammedo@redhat.com> <1466693669-139967-6-git-send-email-imammedo@redhat.com> <20160627175524.GQ3332@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/11] target-i386: cpu: replace custom apic-id setter/getter with static property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, rth@twiddle.net, mst@redhat.com, armbru@redhat.com, eduardo.otubo@profitbricks.com, zhugh.fnst@cn.fujitsu.com, eblake@redhat.com On Mon, 27 Jun 2016 14:55:24 -0300 Eduardo Habkost 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 > > --- > > 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