| Message ID | 1365691918-30594-9-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 11, 2013 at 04:51:47PM +0200, Igor Mammedov wrote: > get_arch_id() adds possibility for generic code to get guest visible > CPI id without accessing CPUArchState. If target doesn't override it, > it will return cpu_index. > > Override it on target-i386 to return APIC ID. The code implementing the method looks OK, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But I still need to understand better why exactly i386 needs to use it and make the externally-visible hotplug interface based on APIC IDs if all other architectures are going to be happy using CPU indexes to do the same thing. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > * it will be used later by new cpu_exists() generic function and > acpi_piix. > * s/cpu_firmware_id/cpu_arch_id/ > --- > include/qom/cpu.h | 2 ++ > qom/cpu.c | 6 ++++++ > target-i386/cpu.c | 10 ++++++++++ > 3 files changed, 18 insertions(+) > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 3664a1b..b376416 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; > * instantiatable CPU type. > * @reset: Callback to reset the #CPUState to its initial state. > * @do_interrupt: Callback for interrupt handling. > + * @get_arch_id: Callback for getting architecture depended CPU ID > * @vmsd: State description for migration. > * > * Represents a CPU family or model. > @@ -58,6 +59,7 @@ typedef struct CPUClass { > > void (*reset)(CPUState *cpu); > void (*do_interrupt)(CPUState *cpu); > + int64_t (*get_arch_id)(CPUState *cpu); > > const struct VMStateDescription *vmsd; > } CPUClass; > diff --git a/qom/cpu.c b/qom/cpu.c > index fe85960..90cbd77 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -77,6 +77,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) > } > } > > +static int64_t cpu_common_get_arch_id(CPUState *cpu) > +{ > + return cpu->cpu_index; > +} > + > static void cpu_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -84,6 +89,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > k->class_by_name = cpu_common_class_by_name; > k->reset = cpu_common_reset; > + k->get_arch_id = cpu_common_get_arch_id; > dc->realize = cpu_common_realizefn; > dc->no_user = 1; > } > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index c28436c..9cca031 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2272,6 +2272,14 @@ static void x86_cpu_initfn(Object *obj) > } > } > > +static int64_t x86_cpu_get_arch_id(CPUState *cpu) > +{ > + X86CPU *x86cpu = X86_CPU(cpu); > + CPUX86State *env = &x86cpu->env; > + > + return env->cpuid_apic_id; > +} > + > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > { > X86CPUClass *xcc = X86_CPU_CLASS(oc); > @@ -2286,6 +2294,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > cc->do_interrupt = x86_cpu_do_interrupt; > cpu_class_set_vmsd(cc, &vmstate_x86_cpu); > + > + cc->get_arch_id = x86_cpu_get_arch_id; > } > > static const TypeInfo x86_cpu_type_info = { > -- > 1.8.2 >
On Thu, 11 Apr 2013 16:04:26 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Apr 11, 2013 at 04:51:47PM +0200, Igor Mammedov wrote: > > get_arch_id() adds possibility for generic code to get guest visible > > CPI id without accessing CPUArchState. If target doesn't override it, > > it will return cpu_index. > > > > Override it on target-i386 to return APIC ID. > > The code implementing the method looks OK, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > But I still need to understand better why exactly i386 needs to use it > and make the externally-visible hotplug interface based on APIC IDs if > all other architectures are going to be happy using CPU indexes to do > the same thing. get_arch_id() was prompted by 10/19 which is used not only by target-i386. so it was introduced to make acpi_piix4 CPU hotplug more generic. And proved to useful in a number of other places. > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > * it will be used later by new cpu_exists() generic function and > > acpi_piix. > > * s/cpu_firmware_id/cpu_arch_id/ > > --- > > include/qom/cpu.h | 2 ++ > > qom/cpu.c | 6 ++++++ > > target-i386/cpu.c | 10 ++++++++++ > > 3 files changed, 18 insertions(+) > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > index 3664a1b..b376416 100644 > > --- a/include/qom/cpu.h > > +++ b/include/qom/cpu.h > > @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; > > * instantiatable CPU type. > > * @reset: Callback to reset the #CPUState to its initial state. > > * @do_interrupt: Callback for interrupt handling. > > + * @get_arch_id: Callback for getting architecture depended CPU ID > > * @vmsd: State description for migration. > > * > > * Represents a CPU family or model. > > @@ -58,6 +59,7 @@ typedef struct CPUClass { > > > > void (*reset)(CPUState *cpu); > > void (*do_interrupt)(CPUState *cpu); > > + int64_t (*get_arch_id)(CPUState *cpu); > > > > const struct VMStateDescription *vmsd; > > } CPUClass; > > diff --git a/qom/cpu.c b/qom/cpu.c > > index fe85960..90cbd77 100644 > > --- a/qom/cpu.c > > +++ b/qom/cpu.c > > @@ -77,6 +77,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) > > } > > } > > > > +static int64_t cpu_common_get_arch_id(CPUState *cpu) > > +{ > > + return cpu->cpu_index; > > +} > > + > > static void cpu_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -84,6 +89,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > > > k->class_by_name = cpu_common_class_by_name; > > k->reset = cpu_common_reset; > > + k->get_arch_id = cpu_common_get_arch_id; > > dc->realize = cpu_common_realizefn; > > dc->no_user = 1; > > } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index c28436c..9cca031 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -2272,6 +2272,14 @@ static void x86_cpu_initfn(Object *obj) > > } > > } > > > > +static int64_t x86_cpu_get_arch_id(CPUState *cpu) > > +{ > > + X86CPU *x86cpu = X86_CPU(cpu); > > + CPUX86State *env = &x86cpu->env; > > + > > + return env->cpuid_apic_id; > > +} > > + > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > { > > X86CPUClass *xcc = X86_CPU_CLASS(oc); > > @@ -2286,6 +2294,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > > cc->do_interrupt = x86_cpu_do_interrupt; > > cpu_class_set_vmsd(cc, &vmstate_x86_cpu); > > + > > + cc->get_arch_id = x86_cpu_get_arch_id; > > } > > > > static const TypeInfo x86_cpu_type_info = { > > -- > > 1.8.2 > > > > -- > Eduardo
On Fri, Apr 12, 2013 at 12:31:22PM +0200, Igor Mammedov wrote: > On Thu, 11 Apr 2013 16:04:26 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Apr 11, 2013 at 04:51:47PM +0200, Igor Mammedov wrote: > > > get_arch_id() adds possibility for generic code to get guest visible > > > CPI id without accessing CPUArchState. If target doesn't override it, > > > it will return cpu_index. > > > > > > Override it on target-i386 to return APIC ID. > > > > The code implementing the method looks OK, so: > > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > But I still need to understand better why exactly i386 needs to use it > > and make the externally-visible hotplug interface based on APIC IDs if > > all other architectures are going to be happy using CPU indexes to do > > the same thing. > get_arch_id() was prompted by 10/19 which is used not only by target-i386. > so it was introduced to make acpi_piix4 CPU hotplug more generic. And proved > to useful in a number of other places. OK, so get_arch_id() is useful for other code too, not just for the external cpu-add interface. It makes lots of sense to use it on acpi_piix4, yes. (My question about making the cpu-add interface based on arch IDs instead of arch-independent CPU indexes is being discussed under patch 18/19). > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > * it will be used later by new cpu_exists() generic function and > > > acpi_piix. > > > * s/cpu_firmware_id/cpu_arch_id/ > > > --- > > > include/qom/cpu.h | 2 ++ > > > qom/cpu.c | 6 ++++++ > > > target-i386/cpu.c | 10 ++++++++++ > > > 3 files changed, 18 insertions(+) > > > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > > index 3664a1b..b376416 100644 > > > --- a/include/qom/cpu.h > > > +++ b/include/qom/cpu.h > > > @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; > > > * instantiatable CPU type. > > > * @reset: Callback to reset the #CPUState to its initial state. > > > * @do_interrupt: Callback for interrupt handling. > > > + * @get_arch_id: Callback for getting architecture depended CPU ID > > > * @vmsd: State description for migration. > > > * > > > * Represents a CPU family or model. > > > @@ -58,6 +59,7 @@ typedef struct CPUClass { > > > > > > void (*reset)(CPUState *cpu); > > > void (*do_interrupt)(CPUState *cpu); > > > + int64_t (*get_arch_id)(CPUState *cpu); > > > > > > const struct VMStateDescription *vmsd; > > > } CPUClass; > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > index fe85960..90cbd77 100644 > > > --- a/qom/cpu.c > > > +++ b/qom/cpu.c > > > @@ -77,6 +77,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) > > > } > > > } > > > > > > +static int64_t cpu_common_get_arch_id(CPUState *cpu) > > > +{ > > > + return cpu->cpu_index; > > > +} > > > + > > > static void cpu_class_init(ObjectClass *klass, void *data) > > > { > > > DeviceClass *dc = DEVICE_CLASS(klass); > > > @@ -84,6 +89,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) > > > > > > k->class_by_name = cpu_common_class_by_name; > > > k->reset = cpu_common_reset; > > > + k->get_arch_id = cpu_common_get_arch_id; > > > dc->realize = cpu_common_realizefn; > > > dc->no_user = 1; > > > } > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index c28436c..9cca031 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -2272,6 +2272,14 @@ static void x86_cpu_initfn(Object *obj) > > > } > > > } > > > > > > +static int64_t x86_cpu_get_arch_id(CPUState *cpu) > > > +{ > > > + X86CPU *x86cpu = X86_CPU(cpu); > > > + CPUX86State *env = &x86cpu->env; > > > + > > > + return env->cpuid_apic_id; > > > +} > > > + > > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > { > > > X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > @@ -2286,6 +2294,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > > > > cc->do_interrupt = x86_cpu_do_interrupt; > > > cpu_class_set_vmsd(cc, &vmstate_x86_cpu); > > > + > > > + cc->get_arch_id = x86_cpu_get_arch_id; > > > } > > > > > > static const TypeInfo x86_cpu_type_info = { > > > -- > > > 1.8.2 > > > > > > > -- > > Eduardo > > > -- > Regards, > Igor
Am 11.04.2013 16:51, schrieb Igor Mammedov: > get_arch_id() adds possibility for generic code to get guest visible > CPI id without accessing CPUArchState. If target doesn't override it, > it will return cpu_index. > > Override it on target-i386 to return APIC ID. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > * it will be used later by new cpu_exists() generic function and > acpi_piix. > * s/cpu_firmware_id/cpu_arch_id/ > --- > include/qom/cpu.h | 2 ++ > qom/cpu.c | 6 ++++++ > target-i386/cpu.c | 10 ++++++++++ > 3 files changed, 18 insertions(+) I'm still unclear about this ID topic, but please don't add QOM methods with arch_ in the name. The arch is expressed through subclasses. Andreas
On Mon, 15 Apr 2013 17:24:36 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 11.04.2013 16:51, schrieb Igor Mammedov: > > get_arch_id() adds possibility for generic code to get guest visible > > CPI id without accessing CPUArchState. If target doesn't override it, > > it will return cpu_index. > > > > Override it on target-i386 to return APIC ID. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > * it will be used later by new cpu_exists() generic function and > > acpi_piix. > > * s/cpu_firmware_id/cpu_arch_id/ > > --- > > include/qom/cpu.h | 2 ++ > > qom/cpu.c | 6 ++++++ > > target-i386/cpu.c | 10 ++++++++++ > > 3 files changed, 18 insertions(+) > > I'm still unclear about this ID topic, but please don't add QOM methods > with arch_ in the name. The arch is expressed through subclasses. arch is there to explicitly show that returned ID is defined by target and not generic code. This method then allow to implement cpu_exists() and helps to init present cpus bitmap in acpi_piix4. Would be cpu_id() acceptable? > > Andreas >
Am 15.04.2013 17:34, schrieb Igor Mammedov: > On Mon, 15 Apr 2013 17:24:36 +0200 > Andreas Färber <afaerber@suse.de> wrote: > >> Am 11.04.2013 16:51, schrieb Igor Mammedov: >>> get_arch_id() adds possibility for generic code to get guest visible >>> CPI id without accessing CPUArchState. If target doesn't override it, >>> it will return cpu_index. >>> >>> Override it on target-i386 to return APIC ID. >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >>> --- >>> * it will be used later by new cpu_exists() generic function and >>> acpi_piix. >>> * s/cpu_firmware_id/cpu_arch_id/ >>> --- >>> include/qom/cpu.h | 2 ++ >>> qom/cpu.c | 6 ++++++ >>> target-i386/cpu.c | 10 ++++++++++ >>> 3 files changed, 18 insertions(+) >> >> I'm still unclear about this ID topic, but please don't add QOM methods >> with arch_ in the name. The arch is expressed through subclasses. > > arch is there to explicitly show that returned ID is defined by target and > not generic code. This method then allow to implement cpu_exists() and helps > to init present cpus bitmap in acpi_piix4. > > Would be cpu_id() acceptable? Certainly. Andreas
On Mon, Apr 15, 2013 at 05:24:36PM +0200, Andreas Färber wrote: > Am 11.04.2013 16:51, schrieb Igor Mammedov: > > get_arch_id() adds possibility for generic code to get guest visible > > CPI id without accessing CPUArchState. If target doesn't override it, > > it will return cpu_index. > > > > Override it on target-i386 to return APIC ID. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > * it will be used later by new cpu_exists() generic function and > > acpi_piix. > > * s/cpu_firmware_id/cpu_arch_id/ > > --- > > include/qom/cpu.h | 2 ++ > > qom/cpu.c | 6 ++++++ > > target-i386/cpu.c | 10 ++++++++++ > > 3 files changed, 18 insertions(+) > > I'm still unclear about this ID topic, but please don't add QOM methods > with arch_ in the name. The arch is expressed through subclasses. The naming seemed appropriate to me as it is an abstract method that is expected to have arch-specific implementations and arch-specific meaning. On the other hand, "firmware_id" looked clearer to me (as it was very specific about the meaning/usefulness of the return value), I don't know what was the original reason to change from firmware_id to arch_id. Other than "arch_id" and "firmware_id , I don't know how we could name that method. It's not "just an ID", but a special kind of ID, so I believe just "cpu_id" wouldn't work.
On Mon, 15 Apr 2013 12:47:52 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 15, 2013 at 05:24:36PM +0200, Andreas Färber wrote: > > Am 11.04.2013 16:51, schrieb Igor Mammedov: > > > get_arch_id() adds possibility for generic code to get guest visible > > > CPI id without accessing CPUArchState. If target doesn't override it, > > > it will return cpu_index. > > > > > > Override it on target-i386 to return APIC ID. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > * it will be used later by new cpu_exists() generic function and > > > acpi_piix. > > > * s/cpu_firmware_id/cpu_arch_id/ > > > --- > > > include/qom/cpu.h | 2 ++ > > > qom/cpu.c | 6 ++++++ > > > target-i386/cpu.c | 10 ++++++++++ > > > 3 files changed, 18 insertions(+) > > > > I'm still unclear about this ID topic, but please don't add QOM methods > > with arch_ in the name. The arch is expressed through subclasses. > > The naming seemed appropriate to me as it is an abstract method that is > expected to have arch-specific implementations and arch-specific > meaning. > > On the other hand, "firmware_id" looked clearer to me (as it was very > specific about the meaning/usefulness of the return value), I don't know > what was the original reason to change from firmware_id to arch_id. Objection here http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg01332.html In case of x86, it might be firmware defined id, but in general case it might be not the case, since it could be hardwired into CPU. So get_arch_id() seems more appropriate. > > Other than "arch_id" and "firmware_id , I don't know how we could name > that method. It's not "just an ID", but a special kind of ID, so I > believe just "cpu_id" wouldn't work. > > -- > Eduardo >
diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 3664a1b..b376416 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; * instantiatable CPU type. * @reset: Callback to reset the #CPUState to its initial state. * @do_interrupt: Callback for interrupt handling. + * @get_arch_id: Callback for getting architecture depended CPU ID * @vmsd: State description for migration. * * Represents a CPU family or model. @@ -58,6 +59,7 @@ typedef struct CPUClass { void (*reset)(CPUState *cpu); void (*do_interrupt)(CPUState *cpu); + int64_t (*get_arch_id)(CPUState *cpu); const struct VMStateDescription *vmsd; } CPUClass; diff --git a/qom/cpu.c b/qom/cpu.c index fe85960..90cbd77 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -77,6 +77,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) } } +static int64_t cpu_common_get_arch_id(CPUState *cpu) +{ + return cpu->cpu_index; +} + static void cpu_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -84,6 +89,7 @@ static void cpu_class_init(ObjectClass *klass, void *data) k->class_by_name = cpu_common_class_by_name; k->reset = cpu_common_reset; + k->get_arch_id = cpu_common_get_arch_id; dc->realize = cpu_common_realizefn; dc->no_user = 1; } diff --git a/target-i386/cpu.c b/target-i386/cpu.c index c28436c..9cca031 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2272,6 +2272,14 @@ static void x86_cpu_initfn(Object *obj) } } +static int64_t x86_cpu_get_arch_id(CPUState *cpu) +{ + X86CPU *x86cpu = X86_CPU(cpu); + CPUX86State *env = &x86cpu->env; + + return env->cpuid_apic_id; +} + static void x86_cpu_common_class_init(ObjectClass *oc, void *data) { X86CPUClass *xcc = X86_CPU_CLASS(oc); @@ -2286,6 +2294,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) cc->do_interrupt = x86_cpu_do_interrupt; cpu_class_set_vmsd(cc, &vmstate_x86_cpu); + + cc->get_arch_id = x86_cpu_get_arch_id; } static const TypeInfo x86_cpu_type_info = {
get_arch_id() adds possibility for generic code to get guest visible CPI id without accessing CPUArchState. If target doesn't override it, it will return cpu_index. Override it on target-i386 to return APIC ID. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- * it will be used later by new cpu_exists() generic function and acpi_piix. * s/cpu_firmware_id/cpu_arch_id/ --- include/qom/cpu.h | 2 ++ qom/cpu.c | 6 ++++++ target-i386/cpu.c | 10 ++++++++++ 3 files changed, 18 insertions(+)