| Message ID | 1366063976-4909-11-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 16, 2013 at 12:12:50AM +0200, Igor Mammedov wrote: > ... and use it from board level to set APIC ID for CPUs > it creates. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > Note: > * pc_new_cpu() function will be reused later in CPU hot-plug hook. > > 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)) { > + 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); > + 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); > > -- > 1.8.2 >
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 <imammedo@redhat.com> > Reviewed-by: Paolo Bonzini <pbonzini@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. > > 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. > + 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. > + 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
On Mon, 22 Apr 2013 16:05:34 +0200 Andreas Färber <afaerber@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 <imammedo@redhat.com> > > Reviewed-by: Paolo Bonzini <pbonzini@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 > > > + 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 >
Am 22.04.2013 18:30, schrieb Igor Mammedov: > On Mon, 22 Apr 2013 16:05:34 +0200 > Andreas Färber <afaerber@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 <imammedo@redhat.com> >>> Reviewed-by: Paolo Bonzini <pbonzini@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 >> > >
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)) { + 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); + 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);