From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50109) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVlcj-0002v3-1I for qemu-devel@nongnu.org; Fri, 26 Apr 2013 12:35:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVlch-0001Dv-9d for qemu-devel@nongnu.org; Fri, 26 Apr 2013 12:35:48 -0400 Received: from cantor2.suse.de ([195.135.220.15]:35562 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVlch-0001Dk-0W for qemu-devel@nongnu.org; Fri, 26 Apr 2013 12:35:47 -0400 Message-ID: <517AACDE.5080200@suse.de> Date: Fri, 26 Apr 2013 18:35:42 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1366063976-4909-1-git-send-email-imammedo@redhat.com> <1366063976-4909-11-git-send-email-imammedo@redhat.com> <517543AE.7050904@suse.de> <20130422183059.0ec7e482@nial.usersys.redhat.com> In-Reply-To: <20130422183059.0ec7e482@nial.usersys.redhat.com> 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: Igor Mammedov 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 Am 22.04.2013 18:30, schrieb Igor Mammedov: > On Mon, 22 Apr 2013 16:05:34 +0200 > Andreas F=E4rber wrote: >=20 >> Am 16.04.2013 00:12, schrieb Igor Mammedov: >>> ... and use it from board level to set APIC ID for CPUs >>> it creates. >>> >>> Signed-off-by: Igor Mammedov >>> Reviewed-by: Paolo Bonzini >>> --- >>> Note: >>> * pc_new_cpu() function will be reused later in CPU hot-plug hook. >> >> Suggest as main commit message to avoid the "...": >> >> 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(-) >>> >>> 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)) { >> >> Please use a local Error* variable with error_propagate, otherwise thi= s >> 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 *opa= que, >>> + 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 *opa= que, >>> + 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; >>> + } >> >>> + if (value < min || value > max) { >>> + error_setg(&error, "Property %s.%s doesn't take value %" PRI= d64 >>> + " (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", va= lue); >>> + error_propagate(errp, error); >> >> 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 The only thing Paolo requested there (and which I +1) is not to do error_is_set(errp). That is independent of error_setg() though, which is followed by a return, so has no functional difference. Andreas >=20 >> >>> + return; >>> + } >>> + cpu->env.cpuid_apic_id =3D value; >>> +} >>> + >>> static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *= name) >>> { >>> 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 >> >> Otherwise I like this x86-specific version now! >> >> Andreas >> >=20 >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg