From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUJf7-0004mq-3Q for qemu-devel@nongnu.org; Mon, 22 Apr 2013 12:32:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUJf1-0002SF-Rs for qemu-devel@nongnu.org; Mon, 22 Apr 2013 12:32:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42139) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUJf1-0002Rx-KI for qemu-devel@nongnu.org; Mon, 22 Apr 2013 12:32:11 -0400 Date: Mon, 22 Apr 2013 18:30:59 +0200 From: Igor Mammedov Message-ID: <20130422183059.0ec7e482@nial.usersys.redhat.com> In-Reply-To: <517543AE.7050904@suse.de> References: <1366063976-4909-1-git-send-email-imammedo@redhat.com> <1366063976-4909-11-git-send-email-imammedo@redhat.com> <517543AE.7050904@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 10/16] target-i386: introduce apic-id property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: aliguori@us.ibm.com, ehabkost@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, lcapitulino@redhat.com, jfrei@linux.vnet.ibm.com, yang.z.zhang@intel.com, pbonzini@redhat.com, lig.fnst@cn.fujitsu.com, rth@twiddle.net On Mon, 22 Apr 2013 16:05:34 +0200 Andreas F=E4rber wrote: > Am 16.04.2013 00:12, schrieb Igor Mammedov: > > ... and use it from board level to set APIC ID for CPUs > > it creates. > >=20 > > Signed-off-by: Igor Mammedov > > Reviewed-by: Paolo Bonzini > > --- > > Note: > > * pc_new_cpu() function will be reused later in CPU hot-plug hook. >=20 > Suggest as main commit message to avoid the "...": >=20 > The property is used from board level to set APIC ID for CPUs it > creates. Do so in a new pc_new_cpu() helper, to be reused for hot-plug. I'll do on next respin. > >=20 > > v3: > > * user error_propagate() in property setter > > v2: > > * use generic cpu_exists() instead of custom one > > * make apic-id dynamic property, so it won't be possible to use it > > as global property, since it's instance specific > > --- > > hw/i386/pc.c | 25 ++++++++++++++++++++++++- > > target-i386/cpu.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 66 insertions(+), 1 deletion(-) > >=20 > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index dc1a78b..cb57878 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -889,9 +889,29 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, > > int level) } > > } > > =20 > > +static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error > > **errp) +{ > > + X86CPU *cpu; > > + > > + cpu =3D cpu_x86_create(cpu_model, errp); > > + if (!cpu) { > > + return; > > + } > > + > > + object_property_set_int(OBJECT(cpu), apic_id, "apic-id", errp); > > + object_property_set_bool(OBJECT(cpu), true, "realized", errp); > > + > > + if (error_is_set(errp)) { >=20 > Please use a local Error* variable with error_propagate, otherwise this > is fragile. done >=20 > > + if (cpu !=3D NULL) { > > + object_unref(OBJECT(cpu)); > > + } > > + } > > +} > > + > > void pc_cpus_init(const char *cpu_model) > > { > > int i; > > + Error *error =3D NULL; > > =20 > > /* init CPUs */ > > if (cpu_model =3D=3D NULL) { > > @@ -903,7 +923,10 @@ void pc_cpus_init(const char *cpu_model) > > } > > =20 > > for (i =3D 0; i < smp_cpus; i++) { > > - if (!cpu_x86_init(cpu_model)) { > > + pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i), &error); > > + if (error) { > > + fprintf(stderr, "%s\n", error_get_pretty(error)); > > + error_free(error); > > exit(1); > > } > > } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index a415fa3..6d6c527 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1271,6 +1271,45 @@ static void x86_cpuid_set_tsc_freq(Object *obj, > > Visitor *v, void *opaque, cpu->env.tsc_khz =3D value / 1000; > > } > > =20 > > +static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaqu= e, > > + const char *name, Error **errp) > > +{ > > + X86CPU *cpu =3D X86_CPU(obj); > > + int64_t value =3D cpu->env.cpuid_apic_id; > > + > > + visit_type_int(v, &value, name, errp); > > +} > > + > > +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaqu= e, > > + const char *name, Error **errp) > > +{ > > + X86CPU *cpu =3D X86_CPU(obj); > > + const int64_t min =3D 0; > > + const int64_t max =3D UINT32_MAX; > > + Error *error =3D NULL; > > + int64_t value; > > + > > + visit_type_int(v, &value, name, &error); > > + if (error) { > > + error_propagate(errp, error); > > + return; > > + } >=20 > > + if (value < min || value > max) { > > + error_setg(&error, "Property %s.%s doesn't take value %" PRId64 > > + " (minimum: %" PRId64 ", maximum: %" PRId64 ")" , > > + object_get_typename(obj), name, value, min, max); > > + error_propagate(errp, error); > > + return; > > + } > > + > > + if (cpu_exists(value)) { > > + error_setg(&error, "CPU with APIC ID %" PRIi64 " exists", valu= e); > > + error_propagate(errp, error); >=20 > You could do both error_setg()s directly on errp. it was so before, but I was requested to use local error here: http://lists.gnu.org/archive/html/qemu-devel/2013-04/msg01606.html >=20 > > + return; > > + } > > + cpu->env.cpuid_apic_id =3D value; > > +} > > + > > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *na= me) > > { > > x86_def_t *def; > > @@ -2259,6 +2298,9 @@ 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); > > =20 > > env->cpuid_apic_id =3D x86_cpu_apic_id_from_index(cs->cpu_index); > > =20 >=20 > Otherwise I like this x86-specific version now! >=20 > Andreas >=20