| Message ID | 1454586455-10202-2-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/04/2016 01:47 PM, Igor Mammedov wrote: > on x86 currently range 0..max_cpus is used to generate > architecture-dependent CPU ID (APIC Id) for each present > and possible CPUs. However architecture-dependent CPU IDs > list could be sparse and code that needs to enumerate > all IDs (ACPI) ended up doing guess work enumerating all > possible and impossible IDs up to > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > That leads to creation of MADT entries and Processor > objects in ACPI tables for not possible CPUs. > Fix it by allowing board specify a concrete list of > CPU IDs accourding its own rules (which for x86 depends > on topology). So that code that needs this list could > request it from board instead of trying to figure out > what IDs are correct on its own. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 16 ++++++++++++++++ > include/hw/boards.h | 18 ++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d72246d..2fd8fc8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > return topo.pkg_id; > } > > +static GArray *pc_possible_cpu_arch_ids(void) > +{ > + int i; > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > + > + for (i = 0; i < max_cpus; i++) { > + CPUArchId val; > + > + val.arch_id = x86_cpu_apic_id_from_index(i); > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > + g_array_append_val(list, val); > + } > + return list; > +} > + > static void pc_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -1968,6 +1983,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > pcmc->save_tsc_khz = true; > mc->get_hotplug_handler = pc_get_hotpug_handler; > mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; > + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > mc->default_boot_order = "cad"; > mc->hot_add_cpu = pc_hot_add_cpu; > mc->max_cpus = 255; > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 0f30959..bd85f46 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -8,6 +8,7 @@ > #include "sysemu/accel.h" > #include "hw/qdev.h" > #include "qom/object.h" > +#include "qom/cpu.h" > > void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > const char *name, > @@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine); > bool machine_mem_merge(MachineState *machine); > > /** > + * CPUArchId: > + * @arch_id - architecture-dependent CPU ID of present or possible CPU > + * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise > + */ > +typedef struct { > + uint64_t arch_id; > + struct CPUState *cpu; > +} CPUArchId; > + > +/** > * MachineClass: > * @get_hotplug_handler: this function is called during bus-less > * device hotplug. If defined it returns pointer to an instance > @@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine); > * Set only by old machines because they need to keep > * compatibility on code that exposed QEMU_VERSION to guests in > * the past (and now use qemu_hw_version()). > + * @possible_cpu_arch_ids: > + * Returns an array of @CPUArchId architecture-dependent CPU IDs > + * which includes CPU IDs for present and possible to hotplug CPUs. > + * Caller is responsible for freeing returned list. > */ > struct MachineClass { > /*< private >*/ > @@ -99,8 +114,11 @@ struct MachineClass { > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > DeviceState *dev); > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); > + GArray *(*possible_cpu_arch_ids)(void); Hi Igor, Can't this be a GArray filled in at machine init time instead of a method? Just wondering. Thanks, Marcel > }; > > +#define FETCH_CPU_ARCH_ID(array, idx) g_array_index(array, CPUArchId, idx) > + > /** > * MachineState: > */ >
On Thu, 4 Feb 2016 14:18:10 +0200 Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > On 02/04/2016 01:47 PM, Igor Mammedov wrote: > > on x86 currently range 0..max_cpus is used to generate > > architecture-dependent CPU ID (APIC Id) for each present > > and possible CPUs. However architecture-dependent CPU IDs > > list could be sparse and code that needs to enumerate > > all IDs (ACPI) ended up doing guess work enumerating all > > possible and impossible IDs up to > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > That leads to creation of MADT entries and Processor > > objects in ACPI tables for not possible CPUs. > > Fix it by allowing board specify a concrete list of > > CPU IDs accourding its own rules (which for x86 depends > > on topology). So that code that needs this list could > > request it from board instead of trying to figure out > > what IDs are correct on its own. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/pc.c | 16 ++++++++++++++++ > > include/hw/boards.h | 18 ++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index d72246d..2fd8fc8 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > return topo.pkg_id; > > } > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > +{ > > + int i; > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > + > > + for (i = 0; i < max_cpus; i++) { > > + CPUArchId val; > > + > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > + g_array_append_val(list, val); > > + } > > + return list; > > +} > > + > > static void pc_machine_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc = MACHINE_CLASS(oc); > > @@ -1968,6 +1983,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > pcmc->save_tsc_khz = true; > > mc->get_hotplug_handler = pc_get_hotpug_handler; > > mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; > > + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > > mc->default_boot_order = "cad"; > > mc->hot_add_cpu = pc_hot_add_cpu; > > mc->max_cpus = 255; > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index 0f30959..bd85f46 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -8,6 +8,7 @@ > > #include "sysemu/accel.h" > > #include "hw/qdev.h" > > #include "qom/object.h" > > +#include "qom/cpu.h" > > > > void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > > const char *name, > > @@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine); > > bool machine_mem_merge(MachineState *machine); > > > > /** > > + * CPUArchId: > > + * @arch_id - architecture-dependent CPU ID of present or possible CPU > > + * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise > > + */ > > +typedef struct { > > + uint64_t arch_id; > > + struct CPUState *cpu; > > +} CPUArchId; > > + > > +/** > > * MachineClass: > > * @get_hotplug_handler: this function is called during bus-less > > * device hotplug. If defined it returns pointer to an instance > > @@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine); > > * Set only by old machines because they need to keep > > * compatibility on code that exposed QEMU_VERSION to guests in > > * the past (and now use qemu_hw_version()). > > + * @possible_cpu_arch_ids: > > + * Returns an array of @CPUArchId architecture-dependent CPU IDs > > + * which includes CPU IDs for present and possible to hotplug CPUs. > > + * Caller is responsible for freeing returned list. > > */ > > struct MachineClass { > > /*< private >*/ > > @@ -99,8 +114,11 @@ struct MachineClass { > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > DeviceState *dev); > > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); > > + GArray *(*possible_cpu_arch_ids)(void); > > Hi Igor, > > Can't this be a GArray filled in at machine init time instead of a method? > Just wondering. Then it wouldn't get CPU present status as there is no CPUs at that time. It would also require refatoring of vl.c as machine_init() happens before -smp is parsed. > > Thanks, > Marcel > > > }; > > > > +#define FETCH_CPU_ARCH_ID(array, idx) g_array_index(array, CPUArchId, idx) > > + > > /** > > * MachineState: > > */ > > >
On Thu, Feb 04, 2016 at 02:36:59PM +0100, Igor Mammedov wrote: > On Thu, 4 Feb 2016 14:18:10 +0200 > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > > > On 02/04/2016 01:47 PM, Igor Mammedov wrote: > > > on x86 currently range 0..max_cpus is used to generate > > > architecture-dependent CPU ID (APIC Id) for each present > > > and possible CPUs. However architecture-dependent CPU IDs > > > list could be sparse and code that needs to enumerate > > > all IDs (ACPI) ended up doing guess work enumerating all > > > possible and impossible IDs up to > > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > > > That leads to creation of MADT entries and Processor > > > objects in ACPI tables for not possible CPUs. > > > Fix it by allowing board specify a concrete list of > > > CPU IDs accourding its own rules (which for x86 depends > > > on topology). So that code that needs this list could > > > request it from board instead of trying to figure out > > > what IDs are correct on its own. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/i386/pc.c | 16 ++++++++++++++++ > > > include/hw/boards.h | 18 ++++++++++++++++++ > > > 2 files changed, 34 insertions(+) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index d72246d..2fd8fc8 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > > return topo.pkg_id; > > > } > > > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > > +{ > > > + int i; > > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > > + > > > + for (i = 0; i < max_cpus; i++) { > > > + CPUArchId val; > > > + > > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > > + g_array_append_val(list, val); > > > + } > > > + return list; > > > +} > > > + > > > static void pc_machine_class_init(ObjectClass *oc, void *data) > > > { > > > MachineClass *mc = MACHINE_CLASS(oc); > > > @@ -1968,6 +1983,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > > pcmc->save_tsc_khz = true; > > > mc->get_hotplug_handler = pc_get_hotpug_handler; > > > mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; > > > + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > > > mc->default_boot_order = "cad"; > > > mc->hot_add_cpu = pc_hot_add_cpu; > > > mc->max_cpus = 255; > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > index 0f30959..bd85f46 100644 > > > --- a/include/hw/boards.h > > > +++ b/include/hw/boards.h > > > @@ -8,6 +8,7 @@ > > > #include "sysemu/accel.h" > > > #include "hw/qdev.h" > > > #include "qom/object.h" > > > +#include "qom/cpu.h" > > > > > > void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > > > const char *name, > > > @@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine); > > > bool machine_mem_merge(MachineState *machine); > > > > > > /** > > > + * CPUArchId: > > > + * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > + * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise > > > + */ > > > +typedef struct { > > > + uint64_t arch_id; > > > + struct CPUState *cpu; > > > +} CPUArchId; > > > + > > > +/** > > > * MachineClass: > > > * @get_hotplug_handler: this function is called during bus-less > > > * device hotplug. If defined it returns pointer to an instance > > > @@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine); > > > * Set only by old machines because they need to keep > > > * compatibility on code that exposed QEMU_VERSION to guests in > > > * the past (and now use qemu_hw_version()). > > > + * @possible_cpu_arch_ids: > > > + * Returns an array of @CPUArchId architecture-dependent CPU IDs > > > + * which includes CPU IDs for present and possible to hotplug CPUs. > > > + * Caller is responsible for freeing returned list. > > > */ > > > struct MachineClass { > > > /*< private >*/ > > > @@ -99,8 +114,11 @@ struct MachineClass { > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > > DeviceState *dev); > > > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); > > > + GArray *(*possible_cpu_arch_ids)(void); > > > > Hi Igor, > > > > Can't this be a GArray filled in at machine init time instead of a method? > > Just wondering. > Then it wouldn't get CPU present status as there is no CPUs at that time. > It would also require refatoring of vl.c as machine_init() happens > before -smp is parsed. I believe Marcel meant MachineClass::init() and related code (machine initialization), not machine_init() (machine type registration). That said, calculating it only when actually needed sounds better to me. One less data field to worry about when we change ordering in the machine initialization code.
On Fri, 5 Feb 2016 12:13:58 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Feb 04, 2016 at 02:36:59PM +0100, Igor Mammedov wrote: > > On Thu, 4 Feb 2016 14:18:10 +0200 > > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > > > > > On 02/04/2016 01:47 PM, Igor Mammedov wrote: > > > > on x86 currently range 0..max_cpus is used to generate > > > > architecture-dependent CPU ID (APIC Id) for each present > > > > and possible CPUs. However architecture-dependent CPU IDs > > > > list could be sparse and code that needs to enumerate > > > > all IDs (ACPI) ended up doing guess work enumerating all > > > > possible and impossible IDs up to > > > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > > > > > That leads to creation of MADT entries and Processor > > > > objects in ACPI tables for not possible CPUs. > > > > Fix it by allowing board specify a concrete list of > > > > CPU IDs accourding its own rules (which for x86 depends > > > > on topology). So that code that needs this list could > > > > request it from board instead of trying to figure out > > > > what IDs are correct on its own. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/i386/pc.c | 16 ++++++++++++++++ > > > > include/hw/boards.h | 18 ++++++++++++++++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index d72246d..2fd8fc8 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > > > return topo.pkg_id; > > > > } > > > > > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > > > +{ > > > > + int i; > > > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > > > + > > > > + for (i = 0; i < max_cpus; i++) { > > > > + CPUArchId val; > > > > + > > > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > > > + g_array_append_val(list, val); > > > > + } > > > > + return list; > > > > +} > > > > + > > > > static void pc_machine_class_init(ObjectClass *oc, void *data) > > > > { > > > > MachineClass *mc = MACHINE_CLASS(oc); > > > > @@ -1968,6 +1983,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > > > pcmc->save_tsc_khz = true; > > > > mc->get_hotplug_handler = pc_get_hotpug_handler; > > > > mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; > > > > + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; > > > > mc->default_boot_order = "cad"; > > > > mc->hot_add_cpu = pc_hot_add_cpu; > > > > mc->max_cpus = 255; > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > > > index 0f30959..bd85f46 100644 > > > > --- a/include/hw/boards.h > > > > +++ b/include/hw/boards.h > > > > @@ -8,6 +8,7 @@ > > > > #include "sysemu/accel.h" > > > > #include "hw/qdev.h" > > > > #include "qom/object.h" > > > > +#include "qom/cpu.h" > > > > > > > > void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > > > > const char *name, > > > > @@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine); > > > > bool machine_mem_merge(MachineState *machine); > > > > > > > > /** > > > > + * CPUArchId: > > > > + * @arch_id - architecture-dependent CPU ID of present or possible CPU > > > > + * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise > > > > + */ > > > > +typedef struct { > > > > + uint64_t arch_id; > > > > + struct CPUState *cpu; > > > > +} CPUArchId; > > > > + > > > > +/** > > > > * MachineClass: > > > > * @get_hotplug_handler: this function is called during bus-less > > > > * device hotplug. If defined it returns pointer to an instance > > > > @@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine); > > > > * Set only by old machines because they need to keep > > > > * compatibility on code that exposed QEMU_VERSION to guests in > > > > * the past (and now use qemu_hw_version()). > > > > + * @possible_cpu_arch_ids: > > > > + * Returns an array of @CPUArchId architecture-dependent CPU IDs > > > > + * which includes CPU IDs for present and possible to hotplug CPUs. > > > > + * Caller is responsible for freeing returned list. > > > > */ > > > > struct MachineClass { > > > > /*< private >*/ > > > > @@ -99,8 +114,11 @@ struct MachineClass { > > > > HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > > > > DeviceState *dev); > > > > unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); > > > > + GArray *(*possible_cpu_arch_ids)(void); > > > > > > Hi Igor, > > > > > > Can't this be a GArray filled in at machine init time instead of a method? > > > Just wondering. > > Then it wouldn't get CPU present status as there is no CPUs at that time. > > It would also require refatoring of vl.c as machine_init() happens > > before -smp is parsed. > > I believe Marcel meant MachineClass::init() and related code > (machine initialization), not machine_init() (machine type > registration). > > That said, calculating it only when actually needed sounds better > to me. One less data field to worry about when we change ordering > in the machine initialization code. yep, it's dynamic information so it's better to calculate it on demand. I'll repsin series rebased on top of your guest info removal, once travis.build is complete. >
On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote: > on x86 currently range 0..max_cpus is used to generate > architecture-dependent CPU ID (APIC Id) for each present > and possible CPUs. However architecture-dependent CPU IDs > list could be sparse and code that needs to enumerate > all IDs (ACPI) ended up doing guess work enumerating all > possible and impossible IDs up to > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > That leads to creation of MADT entries and Processor > objects in ACPI tables for not possible CPUs. > Fix it by allowing board specify a concrete list of > CPU IDs accourding its own rules (which for x86 depends > on topology). So that code that needs this list could > request it from board instead of trying to figure out > what IDs are correct on its own. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/pc.c | 16 ++++++++++++++++ > include/hw/boards.h | 18 ++++++++++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index d72246d..2fd8fc8 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > return topo.pkg_id; > } > > +static GArray *pc_possible_cpu_arch_ids(void) > +{ > + int i; > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > + > + for (i = 0; i < max_cpus; i++) { > + CPUArchId val; > + > + val.arch_id = x86_cpu_apic_id_from_index(i); > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > + g_array_append_val(list, val); What about letting callers call qemu_get_cpu_by_arch_id() only if they really need it? If you do that, you just need to return an uint64_t array, and there's no need for struct CPUArchId. > + } > + return list; > +} [...]
On Fri, 5 Feb 2016 13:04:26 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote: > > on x86 currently range 0..max_cpus is used to generate > > architecture-dependent CPU ID (APIC Id) for each present > > and possible CPUs. However architecture-dependent CPU IDs > > list could be sparse and code that needs to enumerate > > all IDs (ACPI) ended up doing guess work enumerating all > > possible and impossible IDs up to > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > That leads to creation of MADT entries and Processor > > objects in ACPI tables for not possible CPUs. > > Fix it by allowing board specify a concrete list of > > CPU IDs accourding its own rules (which for x86 depends > > on topology). So that code that needs this list could > > request it from board instead of trying to figure out > > what IDs are correct on its own. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/pc.c | 16 ++++++++++++++++ > > include/hw/boards.h | 18 ++++++++++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index d72246d..2fd8fc8 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > return topo.pkg_id; > > } > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > +{ > > + int i; > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > + > > + for (i = 0; i < max_cpus; i++) { > > + CPUArchId val; > > + > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > + g_array_append_val(list, val); > > What about letting callers call qemu_get_cpu_by_arch_id() only if > they really need it? > > If you do that, you just need to return an uint64_t array, and > there's no need for struct CPUArchId. So far all callers that would use it would need to call qemu_get_cpu_by_arch_id() so doing it in one place (here) seems better than to duplicating that call over the code. > > > + } > > + return list; > > +} > [...] >
On Fri, Feb 05, 2016 at 04:39:46PM +0100, Igor Mammedov wrote: > On Fri, 5 Feb 2016 13:04:26 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote: > > > on x86 currently range 0..max_cpus is used to generate > > > architecture-dependent CPU ID (APIC Id) for each present > > > and possible CPUs. However architecture-dependent CPU IDs > > > list could be sparse and code that needs to enumerate > > > all IDs (ACPI) ended up doing guess work enumerating all > > > possible and impossible IDs up to > > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > > > That leads to creation of MADT entries and Processor > > > objects in ACPI tables for not possible CPUs. > > > Fix it by allowing board specify a concrete list of > > > CPU IDs accourding its own rules (which for x86 depends > > > on topology). So that code that needs this list could > > > request it from board instead of trying to figure out > > > what IDs are correct on its own. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/i386/pc.c | 16 ++++++++++++++++ > > > include/hw/boards.h | 18 ++++++++++++++++++ > > > 2 files changed, 34 insertions(+) > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > index d72246d..2fd8fc8 100644 > > > --- a/hw/i386/pc.c > > > +++ b/hw/i386/pc.c > > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > > return topo.pkg_id; > > > } > > > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > > +{ > > > + int i; > > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > > + > > > + for (i = 0; i < max_cpus; i++) { > > > + CPUArchId val; > > > + > > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > > + g_array_append_val(list, val); > > > > What about letting callers call qemu_get_cpu_by_arch_id() only if > > they really need it? > > > > If you do that, you just need to return an uint64_t array, and > > there's no need for struct CPUArchId. > So far all callers that would use it would need to call > qemu_get_cpu_by_arch_id() so doing it in one place (here) > seems better than to duplicating that call over the code. I see only one place using CPUArchId.cpu. All other callers don't use the field. Simply replacing "id.cpu" with "qemu_get_cpu_by_arch_id(id)" in one line seems worth it, if it's going to save us the trouble of defining another struct and avoid lots of unnecessary calls to qemu_get_cpu_by_arch_id() (that loops through all CPUs every time it's called).
On Fri, 5 Feb 2016 13:50:05 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Feb 05, 2016 at 04:39:46PM +0100, Igor Mammedov wrote: > > On Fri, 5 Feb 2016 13:04:26 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote: > > > > on x86 currently range 0..max_cpus is used to generate > > > > architecture-dependent CPU ID (APIC Id) for each present > > > > and possible CPUs. However architecture-dependent CPU IDs > > > > list could be sparse and code that needs to enumerate > > > > all IDs (ACPI) ended up doing guess work enumerating all > > > > possible and impossible IDs up to > > > > apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). > > > > > > > > That leads to creation of MADT entries and Processor > > > > objects in ACPI tables for not possible CPUs. > > > > Fix it by allowing board specify a concrete list of > > > > CPU IDs accourding its own rules (which for x86 depends > > > > on topology). So that code that needs this list could > > > > request it from board instead of trying to figure out > > > > what IDs are correct on its own. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/i386/pc.c | 16 ++++++++++++++++ > > > > include/hw/boards.h | 18 ++++++++++++++++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > > > index d72246d..2fd8fc8 100644 > > > > --- a/hw/i386/pc.c > > > > +++ b/hw/i386/pc.c > > > > @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) > > > > return topo.pkg_id; > > > > } > > > > > > > > +static GArray *pc_possible_cpu_arch_ids(void) > > > > +{ > > > > + int i; > > > > + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); > > > > + > > > > + for (i = 0; i < max_cpus; i++) { > > > > + CPUArchId val; > > > > + > > > > + val.arch_id = x86_cpu_apic_id_from_index(i); > > > > + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); > > > > + g_array_append_val(list, val); > > > > > > What about letting callers call qemu_get_cpu_by_arch_id() only if > > > they really need it? > > > > > > If you do that, you just need to return an uint64_t array, and > > > there's no need for struct CPUArchId. > > So far all callers that would use it would need to call > > qemu_get_cpu_by_arch_id() so doing it in one place (here) > > seems better than to duplicating that call over the code. > > I see only one place using CPUArchId.cpu. All other callers don't > use the field. > > Simply replacing "id.cpu" with "qemu_get_cpu_by_arch_id(id)" in > one line seems worth it, if it's going to save us the trouble of > defining another struct and avoid lots of unnecessary calls to > qemu_get_cpu_by_arch_id() (that loops through all CPUs every time > it's called). id.cpu is going to be used at other places when I add xlapic entries and cpu Devices in new CPU hotplug interface later it will be used for similar purposes for virt-arm machine. Another reason for struct is to discourage usage of direct access to elements of array, while with uint64_t it's very tempting to do so and easy to get wrong. (In the first attempt I did uint64_t array and then were looking for bugs due to wrong type casting)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index d72246d..2fd8fc8 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1946,6 +1946,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index) return topo.pkg_id; } +static GArray *pc_possible_cpu_arch_ids(void) +{ + int i; + GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId)); + + for (i = 0; i < max_cpus; i++) { + CPUArchId val; + + val.arch_id = x86_cpu_apic_id_from_index(i); + val.cpu = qemu_get_cpu_by_arch_id(val.arch_id); + g_array_append_val(list, val); + } + return list; +} + static void pc_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -1968,6 +1983,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) pcmc->save_tsc_khz = true; mc->get_hotplug_handler = pc_get_hotpug_handler; mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id; + mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids; mc->default_boot_order = "cad"; mc->hot_add_cpu = pc_hot_add_cpu; mc->max_cpus = 255; diff --git a/include/hw/boards.h b/include/hw/boards.h index 0f30959..bd85f46 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -8,6 +8,7 @@ #include "sysemu/accel.h" #include "hw/qdev.h" #include "qom/object.h" +#include "qom/cpu.h" void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, const char *name, @@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine); bool machine_mem_merge(MachineState *machine); /** + * CPUArchId: + * @arch_id - architecture-dependent CPU ID of present or possible CPU + * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise + */ +typedef struct { + uint64_t arch_id; + struct CPUState *cpu; +} CPUArchId; + +/** * MachineClass: * @get_hotplug_handler: this function is called during bus-less * device hotplug. If defined it returns pointer to an instance @@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine); * Set only by old machines because they need to keep * compatibility on code that exposed QEMU_VERSION to guests in * the past (and now use qemu_hw_version()). + * @possible_cpu_arch_ids: + * Returns an array of @CPUArchId architecture-dependent CPU IDs + * which includes CPU IDs for present and possible to hotplug CPUs. + * Caller is responsible for freeing returned list. */ struct MachineClass { /*< private >*/ @@ -99,8 +114,11 @@ struct MachineClass { HotplugHandler *(*get_hotplug_handler)(MachineState *machine, DeviceState *dev); unsigned (*cpu_index_to_socket_id)(unsigned cpu_index); + GArray *(*possible_cpu_arch_ids)(void); }; +#define FETCH_CPU_ARCH_ID(array, idx) g_array_index(array, CPUArchId, idx) + /** * MachineState: */
on x86 currently range 0..max_cpus is used to generate architecture-dependent CPU ID (APIC Id) for each present and possible CPUs. However architecture-dependent CPU IDs list could be sparse and code that needs to enumerate all IDs (ACPI) ended up doing guess work enumerating all possible and impossible IDs up to apic_id_limit = x86_cpu_apic_id_from_index(max_cpus). That leads to creation of MADT entries and Processor objects in ACPI tables for not possible CPUs. Fix it by allowing board specify a concrete list of CPU IDs accourding its own rules (which for x86 depends on topology). So that code that needs this list could request it from board instead of trying to figure out what IDs are correct on its own. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/pc.c | 16 ++++++++++++++++ include/hw/boards.h | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+)