Am 22.04.2013 18:30, schrieb Igor Mammedov: > On Mon, 22 Apr 2013 16:05:34 +0200 > Andreas Färber <afaer...@suse.de> wrote: > >> 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 <imamm...@redhat.com> >>> Reviewed-by: Paolo Bonzini <pbonz...@redhat.com> >>> --- >>> 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. > >>> >>> 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) } >>> } >>> >>> +static void pc_new_cpu(const char *cpu_model, int64_t apic_id, Error >>> **errp) +{ >>> + X86CPU *cpu; >>> + >>> + cpu = 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 this >> is fragile. > done > >> >>> + if (cpu != NULL) { >>> + object_unref(OBJECT(cpu)); >>> + } >>> + } >>> +} >>> + >>> void pc_cpus_init(const char *cpu_model) >>> { >>> int i; >>> + Error *error = NULL; >>> >>> /* init CPUs */ >>> if (cpu_model == NULL) { >>> @@ -903,7 +923,10 @@ void pc_cpus_init(const char *cpu_model) >>> } >>> >>> for (i = 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 = value / 1000; >>> } >>> >>> +static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque, >>> + const char *name, Error **errp) >>> +{ >>> + X86CPU *cpu = X86_CPU(obj); >>> + int64_t value = cpu->env.cpuid_apic_id; >>> + >>> + visit_type_int(v, &value, name, errp); >>> +} >>> + >>> +static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque, >>> + const char *name, Error **errp) >>> +{ >>> + X86CPU *cpu = X86_CPU(obj); >>> + const int64_t min = 0; >>> + const int64_t max = UINT32_MAX; >>> + Error *error = 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 %" 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", value); >>> + 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 > >> >>> + return; >>> + } >>> + cpu->env.cpuid_apic_id = 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); >>> >>> env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index); >>> >> >> Otherwise I like this x86-specific version now! >> >> Andreas >> > > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg