| Message ID | 1454695626-70225-3-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 05, 2016 at 07:06:58PM +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 9227bde..548ec64 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1931,6 +1931,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; > +} You claim this version is linear, but I don't see any change from v1, except for a whitespace change. qemu_get_cpu_by_arch_id() is O(smp_cpus). This calls qemu_get_cpu_by_arch_id() max_cpus times, so this is O(max_cpus*smp_cpus).
On Fri, 12 Feb 2016 16:33:28 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Feb 05, 2016 at 07:06:58PM +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 9227bde..548ec64 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -1931,6 +1931,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; > > +} > > You claim this version is linear, but I don't see any change from > v1, except for a whitespace change. > > qemu_get_cpu_by_arch_id() is O(smp_cpus). This calls > qemu_get_cpu_by_arch_id() max_cpus times, so this is > O(max_cpus*smp_cpus). I'm sorry, I've fixed it for ACPI tables only but not here. I've just posted and alternative patch for possible CPU in CPU hotplug introspection context '[RFC] QMP: add query-hotpluggable-cpus' which creates list in 2*O(N) and also makes access to list more typesafe. Lets see how it goes and later I can respin this series with that patch included.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 9227bde..548ec64 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1931,6 +1931,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); @@ -1953,6 +1968,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(+)