[Qemu-devel] [PATCH 0/9] pc: do not create invalid MADT.LAPIC/Processor entries

[Qemu-devel] [PATCH 0/9] pc: do not create invalid MADT.LAPIC/Processor entries

[Igor Mammedov]

It's mostly clean up series that removes invalid CPU
entries from MADT/DSDT/SRAT tables when APIC IDs are
sparse distributed*.
Series also removes intermediate present CPUs bitmap
in ACPI tables generation code, replacing it with
machine reported presence status or if not possible*
with directly querying CPU for presence. That should
help later for consolidating and sharing CPU hotplug
codebase and extending supported CPU count above 256
on ACPI side, where I'm going to replace current
"not scalable" bitmap based CPU hotplug MMIO interface
with memory-hotplug like one, which could easily
scale and provide additional info for ACPI CPU device
objects.

Series depends on cotextual changes in following post:
    '[PATCH 0/3] merge SSDT into DSDT'
    https://www.mail-archive.com/qemu-devel@nongnu.org/msg347960.html

* example topology with sparse APIC IDs:
  -smp X,sockets=2,cores=3,maxcpus=6

* it's not possible to remove notion of apic_ad_limit
  since guest visible interfaces like CPU hoptlug MMIO
  (CPON array in ACPI + corresponding MMIO in QEMU) and
  FWCFG should stay the same for compat reasons with
  current setups and legacy SeaBIOS.

Igor Mammedov (9):
  cpu: rename cpu_exists() to qemu_get_cpu_by_arch_id()
  machine: introduce MachineClass.possible_cpu_arch_ids() hook
  pc: acpi: cleanup qdev_get_machine() calls
  pc: acpi: SRAT: create only valid processor lapic entries
  pc: acpi: create Processor and Notify objects only for valid lapics
  pc: acpi: create MADT.lapic entries only for valid lapics
  pc: acpi: drop not needed intermediate bitmap cpu->found_cpus
  pc: move apic_id_limit to PCMachineState
  pc: acpi: clarify why possible LAPIC entries must be present in MADT

 hw/i386/acpi-build.c | 138 ++++++++++++++++++++++++++-------------------------
 hw/i386/pc.c         |  67 ++++++++++++++-----------
 include/hw/boards.h  |  18 +++++++
 include/hw/i386/pc.h |   4 +-
 include/qom/cpu.h    |  10 ++--
 qom/cpu.c            |   6 +--
 target-i386/cpu.c    |   2 +-
 7 files changed, 138 insertions(+), 107 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

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);
 };
 
+#define FETCH_CPU_ARCH_ID(array, idx) g_array_index(array, CPUArchId, idx)
+
 /**
  * MachineState:
  */
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/9] pc: acpi: cleanup qdev_get_machine() calls

[Igor Mammedov]

call qdev_get_machine() once at acpi_build() and
pass its result to child functions that need it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Later it will also allow to reduce number of arguments
passed around by build_foo() functions called from
acpi_build() once guest_info fields are moved into
PCMachineState.
---
 hw/i386/acpi-build.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2028ed7..faf541c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1937,13 +1937,13 @@ static Aml *build_q35_osc_method(void)
 static void
 build_dsdt(GArray *table_data, GArray *linker,
            AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
-           PcPciInfo *pci, PcGuestInfo *guest_info)
+           PcPciInfo *pci, PcGuestInfo *guest_info,
+           MachineState *machine)
 {
     CrsRangeEntry *entry;
     Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
     GPtrArray *mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
     GPtrArray *io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
-    MachineState *machine = MACHINE(qdev_get_machine());
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *bus = NULL;
@@ -2365,7 +2365,8 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 }
 
 static void
-build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
+build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
+           MachineState *machine)
 {
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
@@ -2375,7 +2376,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
     uint64_t curnode;
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineState *pcms = PC_MACHINE(machine);
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
                                 NULL);
@@ -2579,9 +2580,9 @@ static bool acpi_has_iommu(void)
     return intel_iommu && !ambiguous;
 }
 
-static bool acpi_has_nvdimm(void)
+static bool acpi_has_nvdimm(MachineState *machine)
 {
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineState *pcms = PC_MACHINE(machine);
 
     return pcms->nvdimm;
 }
@@ -2599,6 +2600,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     uint8_t *u;
     size_t aml_len = 0;
     GArray *tables_blob = tables->table_data;
+    MachineState *machine = MACHINE(qdev_get_machine());
 
     acpi_get_cpu_info(&cpu);
     acpi_get_pm_info(&pm);
@@ -2624,7 +2626,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
     build_dsdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
-               guest_info);
+               guest_info, machine);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
      * sizing of ACPI tables.
@@ -2655,7 +2657,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     }
     if (guest_info->numa_nodes) {
         acpi_add_table(table_offsets, tables_blob);
-        build_srat(tables_blob, tables->linker, guest_info);
+        build_srat(tables_blob, tables->linker, guest_info, machine);
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
@@ -2666,7 +2668,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
         build_dmar_q35(tables_blob, tables->linker);
     }
 
-    if (acpi_has_nvdimm()) {
+    if (acpi_has_nvdimm(machine)) {
         nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/9] pc: acpi: SRAT: create only valid processor lapic entries

[Igor Mammedov]

When APIC IDs are sparse*, in addition to valid LAPIC
antries the SRAT is also filled invalid ones for non
posiible APIC IDs.
Fix it by asking machine for all possible APIC IDs
instead of wrongly assuming that all APIC IDs in
range 0..apic_id_limit are possible.

* sparse lapic topology CLI:
     -smp x,sockets=2,cores=3,maxcpus=6
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index faf541c..3077061 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2376,6 +2376,8 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
     uint64_t curnode;
     int srat_start, numa_start, slots;
     uint64_t mem_len, mem_base, next_base;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    GArray *apic_id_list = mc->possible_cpu_arch_ids();
     PCMachineState *pcms = PC_MACHINE(machine);
     ram_addr_t hotplugabble_address_space_size =
         object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
@@ -2387,12 +2389,15 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
     srat->reserved1 = cpu_to_le32(1);
     core = (void *)(srat + 1);
 
-    for (i = 0; i < guest_info->apic_id_limit; ++i) {
+    for (i = 0; i < apic_id_list->len; i++) {
+        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
+        int apic_id = id.arch_id;
+
         core = acpi_data_push(table_data, sizeof *core);
         core->type = ACPI_SRAT_PROCESSOR;
         core->length = sizeof(*core);
-        core->local_apic_id = i;
-        curnode = guest_info->node_cpu[i];
+        core->local_apic_id = apic_id;
+        curnode = guest_info->node_cpu[apic_id];
         core->proximity_lo = curnode;
         memset(core->proximity_hi, 0, 3);
         core->local_sapic_eid = 0;
@@ -2457,6 +2462,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
                  (void *)(table_data->data + srat_start),
                  "SRAT",
                  table_data->len - srat_start, 1, NULL);
+    g_array_free(apic_id_list, true);
 }
 
 static void
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/9] pc: acpi: create Processor and Notify objects only for valid lapics

[Igor Mammedov]

do not assume that all lapics in range 0..apic_id_limit
are valid and do not create Processor and Notify objects
for not possible lapics.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3077061..df13c7d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -960,7 +960,8 @@ static Aml *build_crs(PCIHostState *host,
 }
 
 static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
-                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm)
+                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm,
+                                    MachineState *machine)
 {
     int i;
     Aml *dev;
@@ -969,6 +970,8 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
     Aml *field;
     Aml *ifctx;
     Aml *method;
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    GArray *apic_id_list = mc->possible_cpu_arch_ids();
 
     /* The current AML generator can cover the APIC ID range [0..255],
      * inclusive, for VCPU hotplug. */
@@ -998,22 +1001,27 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
     aml_append(sb_scope, field);
 
     /* build Processor object for each processor */
-    for (i = 0; i < acpi_cpus; i++) {
-        dev = aml_processor(i, 0, 0, "CP%.02X", i);
+    for (i = 0; i < apic_id_list->len; i++) {
+        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
+        int apic_id = id.arch_id;
+
+        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
+        dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
 
         method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
         aml_append(method,
-            aml_return(aml_call1(CPU_MAT_METHOD, aml_int(i))));
+            aml_return(aml_call1(CPU_MAT_METHOD, aml_int(apic_id))));
         aml_append(dev, method);
 
         method = aml_method("_STA", 0, AML_NOTSERIALIZED);
         aml_append(method,
-            aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(i))));
+            aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(apic_id))));
         aml_append(dev, method);
 
         method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
         aml_append(method,
-            aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(i), aml_arg(0)))
+            aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(apic_id),
+                aml_arg(0)))
         );
         aml_append(dev, method);
 
@@ -1025,10 +1033,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
      */
     /* Arg0 = Processor ID = APIC ID */
     method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
-    for (i = 0; i < acpi_cpus; i++) {
-        ifctx = aml_if(aml_equal(aml_arg(0), aml_int(i)));
+    for (i = 0; i < apic_id_list->len; i++) {
+        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
+        int apic_id = id.arch_id;
+
+        ifctx = aml_if(aml_equal(aml_arg(0), aml_int(apic_id)));
         aml_append(ifctx,
-            aml_notify(aml_name("CP%.02X", i), aml_arg(1))
+            aml_notify(aml_name("CP%.02X", apic_id), aml_arg(1))
         );
         aml_append(method, ifctx);
     }
@@ -1049,6 +1060,7 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
         aml_append(pkg, aml_int(b));
     }
     aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
+    g_array_free(apic_id_list, true);
 }
 
 static void build_memory_devices(Aml *sb_scope, int nr_mem,
@@ -2244,7 +2256,8 @@ build_dsdt(GArray *table_data, GArray *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        build_processor_devices(sb_scope, guest_info->apic_id_limit, cpu, pm);
+        build_processor_devices(sb_scope, guest_info->apic_id_limit, cpu, pm,
+                                machine);
 
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
                              pm->mem_hp_io_len);
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Igor Mammedov]

do not assume that all lapics in range 0..apic_id_limit
are valid and do not create lapic entries for not
possible lapics in MADT.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index df13c7d..9eeeffa 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
 }
 
 static void
-build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
-           PcGuestInfo *guest_info)
+build_madt(GArray *table_data, GArray *linker,
+           MachineState *machine, PcGuestInfo *guest_info)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    GArray *apic_id_list = mc->possible_cpu_arch_ids();
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
     madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
     madt->flags = cpu_to_le32(1);
 
-    for (i = 0; i < guest_info->apic_id_limit; i++) {
+    for (i = 0; i < apic_id_list->len; i++) {
         AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
+        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
+        int apic_id = id.arch_id;
+
         apic->type = ACPI_APIC_PROCESSOR;
         apic->length = sizeof(*apic);
-        apic->processor_id = i;
-        apic->local_apic_id = i;
-        if (test_bit(i, cpu->found_cpus)) {
+        apic->processor_id = apic_id;
+        apic->local_apic_id = apic_id;
+        if (id.cpu != NULL) {
             apic->flags = cpu_to_le32(1);
         } else {
             apic->flags = cpu_to_le32(0);
         }
     }
+    g_array_free(apic_id_list, true);
+
     io_apic = acpi_data_push(table_data, sizeof *io_apic);
     io_apic->type = ACPI_APIC_IO;
     io_apic->length = sizeof(*io_apic);
@@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     aml_len += tables_blob->len - fadt;
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, &cpu, guest_info);
+    build_madt(tables_blob, tables->linker, machine, guest_info);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Igor Mammedov]

cpu->found_cpus bitmap is used for setting present
flag in CPON AML package at start up. But it takes
a bunch of code to fill bitmap and cloud be simplified
by calling qemu_get_cpu_by_arch_id(apic_id) directly.

Hence do so and remove not used anymore bitmap
with related utilities, which saves us ~32LOC
and also would simplify consolidating APCI parts
of CPU hotplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 42 +++++-------------------------------------
 1 file changed, 5 insertions(+), 37 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9eeeffa..921830e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -76,10 +76,6 @@
 #define ACPI_BUILD_DPRINTF(fmt, ...)
 #endif
 
-typedef struct AcpiCpuInfo {
-    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
-} AcpiCpuInfo;
-
 typedef struct AcpiMcfgInfo {
     uint64_t mcfg_base;
     uint32_t mcfg_size;
@@ -121,31 +117,6 @@ typedef struct AcpiBuildPciBusHotplugState {
     bool pcihp_bridge_en;
 } AcpiBuildPciBusHotplugState;
 
-static
-int acpi_add_cpu_info(Object *o, void *opaque)
-{
-    AcpiCpuInfo *cpu = opaque;
-    uint64_t apic_id;
-
-    if (object_dynamic_cast(o, TYPE_CPU)) {
-        apic_id = object_property_get_int(o, "apic-id", NULL);
-        assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
-
-        set_bit(apic_id, cpu->found_cpus);
-    }
-
-    object_child_foreach(o, acpi_add_cpu_info, opaque);
-    return 0;
-}
-
-static void acpi_get_cpu_info(AcpiCpuInfo *cpu)
-{
-    Object *root = object_get_root();
-
-    memset(cpu->found_cpus, 0, sizeof cpu->found_cpus);
-    object_child_foreach(root, acpi_add_cpu_info, cpu);
-}
-
 static void acpi_get_pm_info(AcpiPmInfo *pm)
 {
     Object *piix = piix4_pm_find();
@@ -967,8 +938,7 @@ static Aml *build_crs(PCIHostState *host,
 }
 
 static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
-                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm,
-                                    MachineState *machine)
+                                    AcpiPmInfo *pm, MachineState *machine)
 {
     int i;
     Aml *dev;
@@ -1063,7 +1033,7 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
                              aml_varpackage(acpi_cpus);
 
     for (i = 0; i < acpi_cpus; i++) {
-        uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
+        uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
         aml_append(pkg, aml_int(b));
     }
     aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
@@ -1955,7 +1925,7 @@ static Aml *build_q35_osc_method(void)
 
 static void
 build_dsdt(GArray *table_data, GArray *linker,
-           AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
+           AcpiPmInfo *pm, AcpiMiscInfo *misc,
            PcPciInfo *pci, PcGuestInfo *guest_info,
            MachineState *machine)
 {
@@ -2263,7 +2233,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        build_processor_devices(sb_scope, guest_info->apic_id_limit, cpu, pm,
+        build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
                                 machine);
 
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
@@ -2618,7 +2588,6 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
 {
     GArray *table_offsets;
     unsigned facs, dsdt, rsdt, fadt;
-    AcpiCpuInfo cpu;
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
     AcpiMcfgInfo mcfg;
@@ -2628,7 +2597,6 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     GArray *tables_blob = tables->table_data;
     MachineState *machine = MACHINE(qdev_get_machine());
 
-    acpi_get_cpu_info(&cpu);
     acpi_get_pm_info(&pm);
     acpi_get_misc_info(&misc);
     acpi_get_pci_info(&pci);
@@ -2651,7 +2619,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
-    build_dsdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
+    build_dsdt(tables_blob, tables->linker, &pm, &misc, &pci,
                guest_info, machine);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState

[Igor Mammedov]

yet another cleanup that replaces multiple apic_id_limit
variables that were initialized at differnt places with
a one initialization at pc_cpus_init() time and letting
other code to just read it. That also allows to reduce
number of a function arguments where it takes
MachineState as an argument.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 14 +++++++-------
 hw/i386/pc.c         | 49 +++++++++++++++++++++----------------------------
 include/hw/i386/pc.h |  4 +++-
 3 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 921830e..d6cd06a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -937,7 +937,7 @@ static Aml *build_crs(PCIHostState *host,
     return crs;
 }
 
-static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
+static void build_processor_devices(Aml *sb_scope,
                                     AcpiPmInfo *pm, MachineState *machine)
 {
     int i;
@@ -948,12 +948,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
     Aml *ifctx;
     Aml *method;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
+    PCMachineState *pcms = PC_MACHINE(machine);
     GArray *apic_id_list = mc->possible_cpu_arch_ids();
 
     /* The current AML generator can cover the APIC ID range [0..255],
      * inclusive, for VCPU hotplug. */
     QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
-    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
+    g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
 
     /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
     dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
@@ -1029,10 +1030,10 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
      * ith up to 255 elements. Windows guests up to win2k8 fail when
      * VarPackageOp is used.
      */
-    pkg = acpi_cpus <= 255 ? aml_package(acpi_cpus) :
-                             aml_varpackage(acpi_cpus);
+    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
+                                       aml_varpackage(pcms->apic_id_limit);
 
-    for (i = 0; i < acpi_cpus; i++) {
+    for (i = 0; i < pcms->apic_id_limit; i++) {
         uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
         aml_append(pkg, aml_int(b));
     }
@@ -2233,8 +2234,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
-                                machine);
+        build_processor_devices(sb_scope, pm, machine);
 
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
                              pm->mem_hp_io_len);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2fd8fc8..61fbb11 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
-/* Calculates the limit to CPU APIC ID values
- *
- * This function returns the limit for the APIC ID value, so that all
- * CPU APIC IDs are < pc_apic_id_limit().
- *
- * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
- */
-static unsigned int pc_apic_id_limit(unsigned int max_cpus)
-{
-    return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
-}
-
 static void pc_build_smbios(FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
@@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
     }
 }
 
-static FWCfgState *bochs_bios_init(AddressSpace *as)
+static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
 {
     FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i, j;
-    unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
 
     fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
 
@@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
      * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
      *     the APIC ID, not the "CPU index"
      */
-    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
                      acpi_tables, acpi_tables_len);
@@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
+    numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
     for (i = 0; i < max_cpus; i++) {
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
-        assert(apic_id < apic_id_limit);
+        assert(apic_id < pcms->apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
             if (test_bit(i, numa_info[j].node_cpu)) {
                 numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
@@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
         }
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
+        numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
+            cpu_to_le64(numa_info[i].node_mem);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
-                     (1 + apic_id_limit + nb_numa_nodes) *
+                     (1 + pcms->apic_id_limit + nb_numa_nodes) *
                      sizeof(*numa_fw_cfg));
 
     return fw_cfg;
@@ -1120,7 +1108,14 @@ void pc_cpus_init(PCMachineState *pcms)
     int i;
     X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
-    unsigned long apic_id_limit;
+
+    /* Calculates the limit to CPU APIC ID values so that all
+     * CPU APIC IDs are < pcms->apic_id_limit.
+     *
+     * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
+     * and bitmap based ACPI CPU hotplug interface
+     */
+    pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
 
     /* init CPUs */
     if (machine->cpu_model == NULL) {
@@ -1131,10 +1126,9 @@ void pc_cpus_init(PCMachineState *pcms)
 #endif
     }
 
-    apic_id_limit = pc_apic_id_limit(max_cpus);
-    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
-        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
-                     apic_id_limit - 1);
+    if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
+        error_report("max_cpus is too large. APIC ID of last CPU is %u",
+                     pcms->apic_id_limit - 1);
         exit(1);
     }
 
@@ -1197,7 +1191,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
 
     guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
     guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
-    guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
     guest_info->apic_xrupt_override = kvm_allows_irq0_override();
     guest_info->numa_nodes = nb_numa_nodes;
     guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
@@ -1206,12 +1199,12 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
         guest_info->node_mem[i] = numa_info[i].node_mem;
     }
 
-    guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
+    guest_info->node_cpu = g_malloc0(pcms->apic_id_limit *
                                      sizeof *guest_info->node_cpu);
 
     for (i = 0; i < max_cpus; i++) {
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
-        assert(apic_id < guest_info->apic_id_limit);
+        assert(apic_id < pcms->apic_id_limit);
         for (j = 0; j < nb_numa_nodes; j++) {
             if (test_bit(i, numa_info[j].node_cpu)) {
                 guest_info->node_cpu[apic_id] = j;
@@ -1386,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init(&address_space_memory);
+    fw_cfg = bochs_bios_init(&address_space_memory, pcms);
 
     rom_set_fw(fw_cfg);
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 65e8f24..315fed2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -59,6 +59,9 @@ struct PCMachineState {
 
     /* RAM information (sizes, addresses, configuration): */
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
+
+    /* APIC ID limit for current max_cpus */
+    unsigned apic_id_limit;
 };
 
 #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
@@ -154,7 +157,6 @@ typedef struct PcPciInfo {
 struct PcGuestInfo {
     bool isapc_ram_fw;
     hwaddr ram_size, ram_size_below_4g;
-    unsigned apic_id_limit;
     bool apic_xrupt_override;
     uint64_t numa_nodes;
     uint64_t *node_mem;
-- 
1.8.3.1

[Qemu-devel] [PATCH 9/9] pc: acpi: clarify why possible LAPIC entries must be present in MADT

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d6cd06a..2cc3fff 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -361,6 +361,12 @@ build_madt(GArray *table_data, GArray *linker,
         if (id.cpu != NULL) {
             apic->flags = cpu_to_le32(1);
         } else {
+            /* ACPI spec says that LAPIC entry for non present
+             * CPU may be omitted from MADT or it must be marked
+             * as disabled. However omitting non present CPU from
+             * MADT breaks hotplug on linux. So possible CPUs
+             * should be put in MADT but kept disabled.
+             */
             apic->flags = cpu_to_le32(0);
         }
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 1/9] cpu: rename cpu_exists() to qemu_get_cpu_by_arch_id()

[Igor Mammedov]

cpu_exists() already does CPU lookup but
discards found CPU and returns boolean instead.
Make it more useful by returning a found CPU
and also rename it more descriptive name.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      |  2 +-
 include/qom/cpu.h | 10 +++++-----
 qom/cpu.c         |  6 +++---
 target-i386/cpu.c |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 942ac06..d72246d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1088,7 +1088,7 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    if (cpu_exists(apic_id)) {
+    if (qemu_get_cpu_by_arch_id(apic_id)) {
         error_setg(errp, "Unable to add CPU: %" PRIi64
                    ", it already exists", id);
         return;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 035179c..bd26bf5 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -622,14 +622,14 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
 CPUState *qemu_get_cpu(int index);
 
 /**
- * cpu_exists:
- * @id: Guest-exposed CPU ID to lookup.
+ * qemu_get_cpu_by_arch_id:
+ * @id: Guest-exposed CPU ID to lookup returned by CPUState@get_arch_id()
  *
- * Search for CPU with specified ID.
+ * Gets a CPU matching @id.
  *
- * Returns: %true - CPU is found, %false - CPU isn't found.
+ * Returns: The CPU or %NULL if there is no matching CPU.
  */
-bool cpu_exists(int64_t id);
+CPUState *qemu_get_cpu_by_arch_id(int id);
 
 /**
  * cpu_throttle_set:
diff --git a/qom/cpu.c b/qom/cpu.c
index 8f537a4..e66989b 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -26,7 +26,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 
-bool cpu_exists(int64_t id)
+CPUState *qemu_get_cpu_by_arch_id(int id)
 {
     CPUState *cpu;
 
@@ -34,10 +34,10 @@ bool cpu_exists(int64_t id)
         CPUClass *cc = CPU_GET_CLASS(cpu);
 
         if (cc->get_arch_id(cpu) == id) {
-            return true;
+            return cpu;
         }
     }
-    return false;
+    return NULL;
 }
 
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b255644..3918f01 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1789,7 +1789,7 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    if ((value != cpu->apic_id) && cpu_exists(value)) {
+    if ((value != cpu->apic_id) && qemu_get_cpu_by_arch_id(value)) {
         error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
         return;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Marcel Apfelbaum]

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:
>    */
>

Re: [Qemu-devel] [PATCH 3/9] pc: acpi: cleanup qdev_get_machine() calls

[Marcel Apfelbaum]

On 02/04/2016 01:47 PM, Igor Mammedov wrote:
> call qdev_get_machine() once at acpi_build() and
> pass its result to child functions that need it.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> Later it will also allow to reduce number of arguments
> passed around by build_foo() functions called from
> acpi_build() once guest_info fields are moved into
> PCMachineState.
> ---
>   hw/i386/acpi-build.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2028ed7..faf541c 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1937,13 +1937,13 @@ static Aml *build_q35_osc_method(void)
>   static void
>   build_dsdt(GArray *table_data, GArray *linker,
>              AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
> -           PcPciInfo *pci, PcGuestInfo *guest_info)
> +           PcPciInfo *pci, PcGuestInfo *guest_info,
> +           MachineState *machine)
>   {
>       CrsRangeEntry *entry;
>       Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
>       GPtrArray *mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>       GPtrArray *io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
> -    MachineState *machine = MACHINE(qdev_get_machine());
>       uint32_t nr_mem = machine->ram_slots;
>       int root_bus_limit = 0xFF;
>       PCIBus *bus = NULL;
> @@ -2365,7 +2365,8 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>   }
>
>   static void
> -build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
> +build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
> +           MachineState *machine)
>   {
>       AcpiSystemResourceAffinityTable *srat;
>       AcpiSratProcessorAffinity *core;
> @@ -2375,7 +2376,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
>       uint64_t curnode;
>       int srat_start, numa_start, slots;
>       uint64_t mem_len, mem_base, next_base;
> -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineState *pcms = PC_MACHINE(machine);
>       ram_addr_t hotplugabble_address_space_size =
>           object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
>                                   NULL);
> @@ -2579,9 +2580,9 @@ static bool acpi_has_iommu(void)
>       return intel_iommu && !ambiguous;
>   }
>
> -static bool acpi_has_nvdimm(void)
> +static bool acpi_has_nvdimm(MachineState *machine)
>   {
> -    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> +    PCMachineState *pcms = PC_MACHINE(machine);
>
>       return pcms->nvdimm;
>   }
> @@ -2599,6 +2600,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>       uint8_t *u;
>       size_t aml_len = 0;
>       GArray *tables_blob = tables->table_data;
> +    MachineState *machine = MACHINE(qdev_get_machine());
>
>       acpi_get_cpu_info(&cpu);
>       acpi_get_pm_info(&pm);
> @@ -2624,7 +2626,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>       /* DSDT is pointed to by FADT */
>       dsdt = tables_blob->len;
>       build_dsdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
> -               guest_info);
> +               guest_info, machine);
>
>       /* Count the size of the DSDT and SSDT, we will need it for legacy
>        * sizing of ACPI tables.
> @@ -2655,7 +2657,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>       }
>       if (guest_info->numa_nodes) {
>           acpi_add_table(table_offsets, tables_blob);
> -        build_srat(tables_blob, tables->linker, guest_info);
> +        build_srat(tables_blob, tables->linker, guest_info, machine);
>       }
>       if (acpi_get_mcfg(&mcfg)) {
>           acpi_add_table(table_offsets, tables_blob);
> @@ -2666,7 +2668,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>           build_dmar_q35(tables_blob, tables->linker);
>       }
>
> -    if (acpi_has_nvdimm()) {
> +    if (acpi_has_nvdimm(machine)) {
>           nvdimm_build_acpi(table_offsets, tables_blob, tables->linker);
>       }
>
>

I like the approach to pass the machine as parameter instead of querying it
when the caller has already a reference to the machine.


Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>


Thanks,
Marcel

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

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:
> >    */
> >  
> 

Re: [Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState

[Igor Mammedov]

On Thu, 4 Feb 2016 14:48:58 +0200
Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:

> On 02/04/2016 01:47 PM, Igor Mammedov wrote:
> > yet another cleanup that replaces multiple apic_id_limit
> > variables that were initialized at differnt places with
> > a one initialization at pc_cpus_init() time and letting
> > other code to just read it. That also allows to reduce
> > number of a function arguments where it takes
> > MachineState as an argument.
> >  
> 
> This patch can collide with Eduardo's PcGuestInfo elimination series.
> I am not sure what is the status of those patches.
I guess it's Eduardo can take this patch or use his.

Eduardo,
I don't care much about this patch, just let me know
what would you prefer to do with it.

> 
> Thanks,
> Marcel
> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   hw/i386/acpi-build.c | 14 +++++++-------
> >   hw/i386/pc.c         | 49 +++++++++++++++++++++----------------------------
> >   include/hw/i386/pc.h |  4 +++-
> >   3 files changed, 31 insertions(+), 36 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 921830e..d6cd06a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -937,7 +937,7 @@ static Aml *build_crs(PCIHostState *host,
> >       return crs;
> >   }
> >
> > -static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > +static void build_processor_devices(Aml *sb_scope,
> >                                       AcpiPmInfo *pm, MachineState *machine)
> >   {
> >       int i;
> > @@ -948,12 +948,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> >       Aml *ifctx;
> >       Aml *method;
> >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    PCMachineState *pcms = PC_MACHINE(machine);
> >       GArray *apic_id_list = mc->possible_cpu_arch_ids();
> >
> >       /* The current AML generator can cover the APIC ID range [0..255],
> >        * inclusive, for VCPU hotplug. */
> >       QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> > -    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > +    g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> >
> >       /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> >       dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> > @@ -1029,10 +1030,10 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> >        * ith up to 255 elements. Windows guests up to win2k8 fail when
> >        * VarPackageOp is used.
> >        */
> > -    pkg = acpi_cpus <= 255 ? aml_package(acpi_cpus) :
> > -                             aml_varpackage(acpi_cpus);
> > +    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > +                                       aml_varpackage(pcms->apic_id_limit);
> >
> > -    for (i = 0; i < acpi_cpus; i++) {
> > +    for (i = 0; i < pcms->apic_id_limit; i++) {
> >           uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
> >           aml_append(pkg, aml_int(b));
> >       }
> > @@ -2233,8 +2234,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> >
> >       sb_scope = aml_scope("\\_SB");
> >       {
> > -        build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
> > -                                machine);
> > +        build_processor_devices(sb_scope, pm, machine);
> >
> >           build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> >                                pm->mem_hp_io_len);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 2fd8fc8..61fbb11 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> >       }
> >   }
> >
> > -/* Calculates the limit to CPU APIC ID values
> > - *
> > - * This function returns the limit for the APIC ID value, so that all
> > - * CPU APIC IDs are < pc_apic_id_limit().
> > - *
> > - * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > - */
> > -static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> > -{
> > -    return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > -}
> > -
> >   static void pc_build_smbios(FWCfgState *fw_cfg)
> >   {
> >       uint8_t *smbios_tables, *smbios_anchor;
> > @@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
> >       }
> >   }
> >
> > -static FWCfgState *bochs_bios_init(AddressSpace *as)
> > +static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
> >   {
> >       FWCfgState *fw_cfg;
> >       uint64_t *numa_fw_cfg;
> >       int i, j;
> > -    unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> >
> >       fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
> >
> > @@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> >        * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
> >        *     the APIC ID, not the "CPU index"
> >        */
> > -    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
> > +    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
> >       fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> >       fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
> >                        acpi_tables, acpi_tables_len);
> > @@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> >        * of nodes, one word for each VCPU->node and one word for each node to
> >        * hold the amount of memory.
> >        */
> > -    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
> > +    numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> >       numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> >       for (i = 0; i < max_cpus; i++) {
> >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > -        assert(apic_id < apic_id_limit);
> > +        assert(apic_id < pcms->apic_id_limit);
> >           for (j = 0; j < nb_numa_nodes; j++) {
> >               if (test_bit(i, numa_info[j].node_cpu)) {
> >                   numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
> > @@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> >           }
> >       }
> >       for (i = 0; i < nb_numa_nodes; i++) {
> > -        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
> > +        numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
> > +            cpu_to_le64(numa_info[i].node_mem);
> >       }
> >       fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
> > -                     (1 + apic_id_limit + nb_numa_nodes) *
> > +                     (1 + pcms->apic_id_limit + nb_numa_nodes) *
> >                        sizeof(*numa_fw_cfg));
> >
> >       return fw_cfg;
> > @@ -1120,7 +1108,14 @@ void pc_cpus_init(PCMachineState *pcms)
> >       int i;
> >       X86CPU *cpu = NULL;
> >       MachineState *machine = MACHINE(pcms);
> > -    unsigned long apic_id_limit;
> > +
> > +    /* Calculates the limit to CPU APIC ID values so that all
> > +     * CPU APIC IDs are < pcms->apic_id_limit.
> > +     *
> > +     * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > +     * and bitmap based ACPI CPU hotplug interface
> > +     */
> > +    pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> >
> >       /* init CPUs */
> >       if (machine->cpu_model == NULL) {
> > @@ -1131,10 +1126,9 @@ void pc_cpus_init(PCMachineState *pcms)
> >   #endif
> >       }
> >
> > -    apic_id_limit = pc_apic_id_limit(max_cpus);
> > -    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > -        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
> > -                     apic_id_limit - 1);
> > +    if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > +        error_report("max_cpus is too large. APIC ID of last CPU is %u",
> > +                     pcms->apic_id_limit - 1);
> >           exit(1);
> >       }
> >
> > @@ -1197,7 +1191,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> >
> >       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> >       guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
> > -    guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
> >       guest_info->apic_xrupt_override = kvm_allows_irq0_override();
> >       guest_info->numa_nodes = nb_numa_nodes;
> >       guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
> > @@ -1206,12 +1199,12 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> >           guest_info->node_mem[i] = numa_info[i].node_mem;
> >       }
> >
> > -    guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
> > +    guest_info->node_cpu = g_malloc0(pcms->apic_id_limit *
> >                                        sizeof *guest_info->node_cpu);
> >
> >       for (i = 0; i < max_cpus; i++) {
> >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > -        assert(apic_id < guest_info->apic_id_limit);
> > +        assert(apic_id < pcms->apic_id_limit);
> >           for (j = 0; j < nb_numa_nodes; j++) {
> >               if (test_bit(i, numa_info[j].node_cpu)) {
> >                   guest_info->node_cpu[apic_id] = j;
> > @@ -1386,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
> >                                           option_rom_mr,
> >                                           1);
> >
> > -    fw_cfg = bochs_bios_init(&address_space_memory);
> > +    fw_cfg = bochs_bios_init(&address_space_memory, pcms);
> >
> >       rom_set_fw(fw_cfg);
> >
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 65e8f24..315fed2 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -59,6 +59,9 @@ struct PCMachineState {
> >
> >       /* RAM information (sizes, addresses, configuration): */
> >       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> > +
> > +    /* APIC ID limit for current max_cpus */
> > +    unsigned apic_id_limit;
> >   };
> >
> >   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> > @@ -154,7 +157,6 @@ typedef struct PcPciInfo {
> >   struct PcGuestInfo {
> >       bool isapc_ram_fw;
> >       hwaddr ram_size, ram_size_below_4g;
> > -    unsigned apic_id_limit;
> >       bool apic_xrupt_override;
> >       uint64_t numa_nodes;
> >       uint64_t *node_mem;
> >  
> 

Re: [Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState

[Michael S. Tsirkin]

On Thu, Feb 04, 2016 at 06:08:38PM +0100, Igor Mammedov wrote:
> On Thu, 4 Feb 2016 14:48:58 +0200
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> 
> > On 02/04/2016 01:47 PM, Igor Mammedov wrote:
> > > yet another cleanup that replaces multiple apic_id_limit
> > > variables that were initialized at differnt places with
> > > a one initialization at pc_cpus_init() time and letting
> > > other code to just read it. That also allows to reduce
> > > number of a function arguments where it takes
> > > MachineState as an argument.
> > >  
> > 
> > This patch can collide with Eduardo's PcGuestInfo elimination series.
> > I am not sure what is the status of those patches.
> I guess it's Eduardo can take this patch or use his.
> 
> Eduardo,
> I don't care much about this patch, just let me know
> what would you prefer to do with it.

Eduardo's work is in my tree, you can rebase on top of it.

> > 
> > Thanks,
> > Marcel
> > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >   hw/i386/acpi-build.c | 14 +++++++-------
> > >   hw/i386/pc.c         | 49 +++++++++++++++++++++----------------------------
> > >   include/hw/i386/pc.h |  4 +++-
> > >   3 files changed, 31 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 921830e..d6cd06a 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -937,7 +937,7 @@ static Aml *build_crs(PCIHostState *host,
> > >       return crs;
> > >   }
> > >
> > > -static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > +static void build_processor_devices(Aml *sb_scope,
> > >                                       AcpiPmInfo *pm, MachineState *machine)
> > >   {
> > >       int i;
> > > @@ -948,12 +948,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > >       Aml *ifctx;
> > >       Aml *method;
> > >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    PCMachineState *pcms = PC_MACHINE(machine);
> > >       GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > >
> > >       /* The current AML generator can cover the APIC ID range [0..255],
> > >        * inclusive, for VCPU hotplug. */
> > >       QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> > > -    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > +    g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > >
> > >       /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> > >       dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> > > @@ -1029,10 +1030,10 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > >        * ith up to 255 elements. Windows guests up to win2k8 fail when
> > >        * VarPackageOp is used.
> > >        */
> > > -    pkg = acpi_cpus <= 255 ? aml_package(acpi_cpus) :
> > > -                             aml_varpackage(acpi_cpus);
> > > +    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > > +                                       aml_varpackage(pcms->apic_id_limit);
> > >
> > > -    for (i = 0; i < acpi_cpus; i++) {
> > > +    for (i = 0; i < pcms->apic_id_limit; i++) {
> > >           uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
> > >           aml_append(pkg, aml_int(b));
> > >       }
> > > @@ -2233,8 +2234,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> > >
> > >       sb_scope = aml_scope("\\_SB");
> > >       {
> > > -        build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
> > > -                                machine);
> > > +        build_processor_devices(sb_scope, pm, machine);
> > >
> > >           build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> > >                                pm->mem_hp_io_len);
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 2fd8fc8..61fbb11 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> > >       }
> > >   }
> > >
> > > -/* Calculates the limit to CPU APIC ID values
> > > - *
> > > - * This function returns the limit for the APIC ID value, so that all
> > > - * CPU APIC IDs are < pc_apic_id_limit().
> > > - *
> > > - * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > - */
> > > -static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> > > -{
> > > -    return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > > -}
> > > -
> > >   static void pc_build_smbios(FWCfgState *fw_cfg)
> > >   {
> > >       uint8_t *smbios_tables, *smbios_anchor;
> > > @@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
> > >       }
> > >   }
> > >
> > > -static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > +static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
> > >   {
> > >       FWCfgState *fw_cfg;
> > >       uint64_t *numa_fw_cfg;
> > >       int i, j;
> > > -    unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> > >
> > >       fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
> > >
> > > @@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > >        * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
> > >        *     the APIC ID, not the "CPU index"
> > >        */
> > > -    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
> > > +    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
> > >       fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> > >       fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
> > >                        acpi_tables, acpi_tables_len);
> > > @@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > >        * of nodes, one word for each VCPU->node and one word for each node to
> > >        * hold the amount of memory.
> > >        */
> > > -    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
> > > +    numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > >       numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > >       for (i = 0; i < max_cpus; i++) {
> > >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > -        assert(apic_id < apic_id_limit);
> > > +        assert(apic_id < pcms->apic_id_limit);
> > >           for (j = 0; j < nb_numa_nodes; j++) {
> > >               if (test_bit(i, numa_info[j].node_cpu)) {
> > >                   numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
> > > @@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > >           }
> > >       }
> > >       for (i = 0; i < nb_numa_nodes; i++) {
> > > -        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
> > > +        numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
> > > +            cpu_to_le64(numa_info[i].node_mem);
> > >       }
> > >       fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
> > > -                     (1 + apic_id_limit + nb_numa_nodes) *
> > > +                     (1 + pcms->apic_id_limit + nb_numa_nodes) *
> > >                        sizeof(*numa_fw_cfg));
> > >
> > >       return fw_cfg;
> > > @@ -1120,7 +1108,14 @@ void pc_cpus_init(PCMachineState *pcms)
> > >       int i;
> > >       X86CPU *cpu = NULL;
> > >       MachineState *machine = MACHINE(pcms);
> > > -    unsigned long apic_id_limit;
> > > +
> > > +    /* Calculates the limit to CPU APIC ID values so that all
> > > +     * CPU APIC IDs are < pcms->apic_id_limit.
> > > +     *
> > > +     * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > +     * and bitmap based ACPI CPU hotplug interface
> > > +     */
> > > +    pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > >
> > >       /* init CPUs */
> > >       if (machine->cpu_model == NULL) {
> > > @@ -1131,10 +1126,9 @@ void pc_cpus_init(PCMachineState *pcms)
> > >   #endif
> > >       }
> > >
> > > -    apic_id_limit = pc_apic_id_limit(max_cpus);
> > > -    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > -        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
> > > -                     apic_id_limit - 1);
> > > +    if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > +        error_report("max_cpus is too large. APIC ID of last CPU is %u",
> > > +                     pcms->apic_id_limit - 1);
> > >           exit(1);
> > >       }
> > >
> > > @@ -1197,7 +1191,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > >
> > >       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> > >       guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
> > > -    guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
> > >       guest_info->apic_xrupt_override = kvm_allows_irq0_override();
> > >       guest_info->numa_nodes = nb_numa_nodes;
> > >       guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
> > > @@ -1206,12 +1199,12 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > >           guest_info->node_mem[i] = numa_info[i].node_mem;
> > >       }
> > >
> > > -    guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
> > > +    guest_info->node_cpu = g_malloc0(pcms->apic_id_limit *
> > >                                        sizeof *guest_info->node_cpu);
> > >
> > >       for (i = 0; i < max_cpus; i++) {
> > >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > -        assert(apic_id < guest_info->apic_id_limit);
> > > +        assert(apic_id < pcms->apic_id_limit);
> > >           for (j = 0; j < nb_numa_nodes; j++) {
> > >               if (test_bit(i, numa_info[j].node_cpu)) {
> > >                   guest_info->node_cpu[apic_id] = j;
> > > @@ -1386,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
> > >                                           option_rom_mr,
> > >                                           1);
> > >
> > > -    fw_cfg = bochs_bios_init(&address_space_memory);
> > > +    fw_cfg = bochs_bios_init(&address_space_memory, pcms);
> > >
> > >       rom_set_fw(fw_cfg);
> > >
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 65e8f24..315fed2 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -59,6 +59,9 @@ struct PCMachineState {
> > >
> > >       /* RAM information (sizes, addresses, configuration): */
> > >       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> > > +
> > > +    /* APIC ID limit for current max_cpus */
> > > +    unsigned apic_id_limit;
> > >   };
> > >
> > >   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> > > @@ -154,7 +157,6 @@ typedef struct PcPciInfo {
> > >   struct PcGuestInfo {
> > >       bool isapc_ram_fw;
> > >       hwaddr ram_size, ram_size_below_4g;
> > > -    unsigned apic_id_limit;
> > >       bool apic_xrupt_override;
> > >       uint64_t numa_nodes;
> > >       uint64_t *node_mem;
> > >  
> > 

Re: [Qemu-devel] [PATCH 8/9] pc: move apic_id_limit to PCMachineState

[Igor Mammedov]

On Thu, 4 Feb 2016 20:18:04 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 04, 2016 at 06:08:38PM +0100, Igor Mammedov wrote:
> > On Thu, 4 Feb 2016 14:48:58 +0200
> > Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> >   
> > > On 02/04/2016 01:47 PM, Igor Mammedov wrote:  
> > > > yet another cleanup that replaces multiple apic_id_limit
> > > > variables that were initialized at differnt places with
> > > > a one initialization at pc_cpus_init() time and letting
> > > > other code to just read it. That also allows to reduce
> > > > number of a function arguments where it takes
> > > > MachineState as an argument.
> > > >    
> > > 
> > > This patch can collide with Eduardo's PcGuestInfo elimination series.
> > > I am not sure what is the status of those patches.  
> > I guess it's Eduardo can take this patch or use his.
> > 
> > Eduardo,
> > I don't care much about this patch, just let me know
> > what would you prefer to do with it.  
> 
> Eduardo's work is in my tree, you can rebase on top of it.
Ok thanks,
I'll wait for comments for a couple days and then rebase.

> 
> > > 
> > > Thanks,
> > > Marcel
> > >   
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >   hw/i386/acpi-build.c | 14 +++++++-------
> > > >   hw/i386/pc.c         | 49 +++++++++++++++++++++----------------------------
> > > >   include/hw/i386/pc.h |  4 +++-
> > > >   3 files changed, 31 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index 921830e..d6cd06a 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -937,7 +937,7 @@ static Aml *build_crs(PCIHostState *host,
> > > >       return crs;
> > > >   }
> > > >
> > > > -static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > > +static void build_processor_devices(Aml *sb_scope,
> > > >                                       AcpiPmInfo *pm, MachineState *machine)
> > > >   {
> > > >       int i;
> > > > @@ -948,12 +948,13 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > >       Aml *ifctx;
> > > >       Aml *method;
> > > >       MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > > +    PCMachineState *pcms = PC_MACHINE(machine);
> > > >       GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > > >
> > > >       /* The current AML generator can cover the APIC ID range [0..255],
> > > >        * inclusive, for VCPU hotplug. */
> > > >       QEMU_BUILD_BUG_ON(ACPI_CPU_HOTPLUG_ID_LIMIT > 256);
> > > > -    g_assert(acpi_cpus <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > > +    g_assert(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
> > > >
> > > >       /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> > > >       dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
> > > > @@ -1029,10 +1030,10 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
> > > >        * ith up to 255 elements. Windows guests up to win2k8 fail when
> > > >        * VarPackageOp is used.
> > > >        */
> > > > -    pkg = acpi_cpus <= 255 ? aml_package(acpi_cpus) :
> > > > -                             aml_varpackage(acpi_cpus);
> > > > +    pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
> > > > +                                       aml_varpackage(pcms->apic_id_limit);
> > > >
> > > > -    for (i = 0; i < acpi_cpus; i++) {
> > > > +    for (i = 0; i < pcms->apic_id_limit; i++) {
> > > >           uint8_t b = qemu_get_cpu_by_arch_id(i) ? 0x01 : 0x00;
> > > >           aml_append(pkg, aml_int(b));
> > > >       }
> > > > @@ -2233,8 +2234,7 @@ build_dsdt(GArray *table_data, GArray *linker,
> > > >
> > > >       sb_scope = aml_scope("\\_SB");
> > > >       {
> > > > -        build_processor_devices(sb_scope, guest_info->apic_id_limit, pm,
> > > > -                                machine);
> > > > +        build_processor_devices(sb_scope, pm, machine);
> > > >
> > > >           build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
> > > >                                pm->mem_hp_io_len);
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 2fd8fc8..61fbb11 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -700,18 +700,6 @@ static uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
> > > >       }
> > > >   }
> > > >
> > > > -/* Calculates the limit to CPU APIC ID values
> > > > - *
> > > > - * This function returns the limit for the APIC ID value, so that all
> > > > - * CPU APIC IDs are < pc_apic_id_limit().
> > > > - *
> > > > - * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > > - */
> > > > -static unsigned int pc_apic_id_limit(unsigned int max_cpus)
> > > > -{
> > > > -    return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > > > -}
> > > > -
> > > >   static void pc_build_smbios(FWCfgState *fw_cfg)
> > > >   {
> > > >       uint8_t *smbios_tables, *smbios_anchor;
> > > > @@ -749,12 +737,11 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
> > > >       }
> > > >   }
> > > >
> > > > -static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > > +static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
> > > >   {
> > > >       FWCfgState *fw_cfg;
> > > >       uint64_t *numa_fw_cfg;
> > > >       int i, j;
> > > > -    unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> > > >
> > > >       fw_cfg = fw_cfg_init_io_dma(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 4, as);
> > > >
> > > > @@ -772,7 +759,7 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > >        * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
> > > >        *     the APIC ID, not the "CPU index"
> > > >        */
> > > > -    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)apic_id_limit);
> > > > +    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)pcms->apic_id_limit);
> > > >       fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> > > >       fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES,
> > > >                        acpi_tables, acpi_tables_len);
> > > > @@ -790,11 +777,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > >        * of nodes, one word for each VCPU->node and one word for each node to
> > > >        * hold the amount of memory.
> > > >        */
> > > > -    numa_fw_cfg = g_new0(uint64_t, 1 + apic_id_limit + nb_numa_nodes);
> > > > +    numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > > >       numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > > >       for (i = 0; i < max_cpus; i++) {
> > > >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > > -        assert(apic_id < apic_id_limit);
> > > > +        assert(apic_id < pcms->apic_id_limit);
> > > >           for (j = 0; j < nb_numa_nodes; j++) {
> > > >               if (test_bit(i, numa_info[j].node_cpu)) {
> > > >                   numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
> > > > @@ -803,10 +790,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as)
> > > >           }
> > > >       }
> > > >       for (i = 0; i < nb_numa_nodes; i++) {
> > > > -        numa_fw_cfg[apic_id_limit + 1 + i] = cpu_to_le64(numa_info[i].node_mem);
> > > > +        numa_fw_cfg[pcms->apic_id_limit + 1 + i] =
> > > > +            cpu_to_le64(numa_info[i].node_mem);
> > > >       }
> > > >       fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, numa_fw_cfg,
> > > > -                     (1 + apic_id_limit + nb_numa_nodes) *
> > > > +                     (1 + pcms->apic_id_limit + nb_numa_nodes) *
> > > >                        sizeof(*numa_fw_cfg));
> > > >
> > > >       return fw_cfg;
> > > > @@ -1120,7 +1108,14 @@ void pc_cpus_init(PCMachineState *pcms)
> > > >       int i;
> > > >       X86CPU *cpu = NULL;
> > > >       MachineState *machine = MACHINE(pcms);
> > > > -    unsigned long apic_id_limit;
> > > > +
> > > > +    /* Calculates the limit to CPU APIC ID values so that all
> > > > +     * CPU APIC IDs are < pcms->apic_id_limit.
> > > > +     *
> > > > +     * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
> > > > +     * and bitmap based ACPI CPU hotplug interface
> > > > +     */
> > > > +    pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
> > > >
> > > >       /* init CPUs */
> > > >       if (machine->cpu_model == NULL) {
> > > > @@ -1131,10 +1126,9 @@ void pc_cpus_init(PCMachineState *pcms)
> > > >   #endif
> > > >       }
> > > >
> > > > -    apic_id_limit = pc_apic_id_limit(max_cpus);
> > > > -    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > > -        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
> > > > -                     apic_id_limit - 1);
> > > > +    if (pcms->apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
> > > > +        error_report("max_cpus is too large. APIC ID of last CPU is %u",
> > > > +                     pcms->apic_id_limit - 1);
> > > >           exit(1);
> > > >       }
> > > >
> > > > @@ -1197,7 +1191,6 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > > >
> > > >       guest_info->ram_size_below_4g = pcms->below_4g_mem_size;
> > > >       guest_info->ram_size = pcms->below_4g_mem_size + pcms->above_4g_mem_size;
> > > > -    guest_info->apic_id_limit = pc_apic_id_limit(max_cpus);
> > > >       guest_info->apic_xrupt_override = kvm_allows_irq0_override();
> > > >       guest_info->numa_nodes = nb_numa_nodes;
> > > >       guest_info->node_mem = g_malloc0(guest_info->numa_nodes *
> > > > @@ -1206,12 +1199,12 @@ PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > > >           guest_info->node_mem[i] = numa_info[i].node_mem;
> > > >       }
> > > >
> > > > -    guest_info->node_cpu = g_malloc0(guest_info->apic_id_limit *
> > > > +    guest_info->node_cpu = g_malloc0(pcms->apic_id_limit *
> > > >                                        sizeof *guest_info->node_cpu);
> > > >
> > > >       for (i = 0; i < max_cpus; i++) {
> > > >           unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > > -        assert(apic_id < guest_info->apic_id_limit);
> > > > +        assert(apic_id < pcms->apic_id_limit);
> > > >           for (j = 0; j < nb_numa_nodes; j++) {
> > > >               if (test_bit(i, numa_info[j].node_cpu)) {
> > > >                   guest_info->node_cpu[apic_id] = j;
> > > > @@ -1386,7 +1379,7 @@ FWCfgState *pc_memory_init(PCMachineState *pcms,
> > > >                                           option_rom_mr,
> > > >                                           1);
> > > >
> > > > -    fw_cfg = bochs_bios_init(&address_space_memory);
> > > > +    fw_cfg = bochs_bios_init(&address_space_memory, pcms);
> > > >
> > > >       rom_set_fw(fw_cfg);
> > > >
> > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > index 65e8f24..315fed2 100644
> > > > --- a/include/hw/i386/pc.h
> > > > +++ b/include/hw/i386/pc.h
> > > > @@ -59,6 +59,9 @@ struct PCMachineState {
> > > >
> > > >       /* RAM information (sizes, addresses, configuration): */
> > > >       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> > > > +
> > > > +    /* APIC ID limit for current max_cpus */
> > > > +    unsigned apic_id_limit;
> > > >   };
> > > >
> > > >   #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> > > > @@ -154,7 +157,6 @@ typedef struct PcPciInfo {
> > > >   struct PcGuestInfo {
> > > >       bool isapc_ram_fw;
> > > >       hwaddr ram_size, ram_size_below_4g;
> > > > -    unsigned apic_id_limit;
> > > >       bool apic_xrupt_override;
> > > >       uint64_t numa_nodes;
> > > >       uint64_t *node_mem;
> > > >    
> > >   
> 

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Eduardo Habkost]

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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/9] cpu: rename cpu_exists() to qemu_get_cpu_by_arch_id()

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:49:12PM +0100, Igor Mammedov wrote:
> cpu_exists() already does CPU lookup but
> discards found CPU and returns boolean instead.
> Make it more useful by returning a found CPU
> and also rename it more descriptive name.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

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.



> 

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Eduardo Habkost]

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;
> +}
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/9] pc: acpi: SRAT: create only valid processor lapic entries

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:47:30PM +0100, Igor Mammedov wrote:
> When APIC IDs are sparse*, in addition to valid LAPIC
> antries the SRAT is also filled invalid ones for non
> posiible APIC IDs.
> Fix it by asking machine for all possible APIC IDs
> instead of wrongly assuming that all APIC IDs in
> range 0..apic_id_limit are possible.
> 
> * sparse lapic topology CLI:
>      -smp x,sockets=2,cores=3,maxcpus=6
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

The only reason we did that is because we were trying to match
Seabios tables byte-by-byte, and Seabios didn't know which APIC
IDs were really valid or not.

Now QEMU has more information than Seabios has, and can be more
efficient.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

(However, see my suggestion to make possible_cpu_arch_ids() just
return an uint64_t array instead of struct CPUArchId).

> ---
>  hw/i386/acpi-build.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index faf541c..3077061 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2376,6 +2376,8 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
>      uint64_t curnode;
>      int srat_start, numa_start, slots;
>      uint64_t mem_len, mem_base, next_base;
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
>      PCMachineState *pcms = PC_MACHINE(machine);
>      ram_addr_t hotplugabble_address_space_size =
>          object_property_get_int(OBJECT(pcms), PC_MACHINE_MEMHP_REGION_SIZE,
> @@ -2387,12 +2389,15 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
>      srat->reserved1 = cpu_to_le32(1);
>      core = (void *)(srat + 1);
>  
> -    for (i = 0; i < guest_info->apic_id_limit; ++i) {
> +    for (i = 0; i < apic_id_list->len; i++) {
> +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> +        int apic_id = id.arch_id;
> +
>          core = acpi_data_push(table_data, sizeof *core);
>          core->type = ACPI_SRAT_PROCESSOR;
>          core->length = sizeof(*core);
> -        core->local_apic_id = i;
> -        curnode = guest_info->node_cpu[i];
> +        core->local_apic_id = apic_id;
> +        curnode = guest_info->node_cpu[apic_id];
>          core->proximity_lo = curnode;
>          memset(core->proximity_hi, 0, 3);
>          core->local_sapic_eid = 0;
> @@ -2457,6 +2462,7 @@ build_srat(GArray *table_data, GArray *linker, PcGuestInfo *guest_info,
>                   (void *)(table_data->data + srat_start),
>                   "SRAT",
>                   table_data->len - srat_start, 1, NULL);
> +    g_array_free(apic_id_list, true);
>  }
>  
>  static void
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 5/9] pc: acpi: create Processor and Notify objects only for valid lapics

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:47:31PM +0100, Igor Mammedov wrote:
> do not assume that all lapics in range 0..apic_id_limit
> are valid and do not create Processor and Notify objects
> for not possible lapics.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Any specific reason you split the Processor/Notify changes and
the MADT/lapic entries into patches 5 and 6? Won't guests be
confused if the Processor entries are missing but the lapic
entries are still there?

I wouldn't mind merging patches 4-6 into a single patch, just to
avoid risking unnecessary bisectability issues.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote:
> do not assume that all lapics in range 0..apic_id_limit
> are valid and do not create lapic entries for not
> possible lapics in MADT.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But there's one minor suggestion below:

> ---
>  hw/i386/acpi-build.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index df13c7d..9eeeffa 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
>  }
>  
>  static void
> -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> -           PcGuestInfo *guest_info)
> +build_madt(GArray *table_data, GArray *linker,
> +           MachineState *machine, PcGuestInfo *guest_info)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
>      int madt_start = table_data->len;
>  
>      AcpiMultipleApicTable *madt;
> @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
>      madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
>      madt->flags = cpu_to_le32(1);
>  
> -    for (i = 0; i < guest_info->apic_id_limit; i++) {
> +    for (i = 0; i < apic_id_list->len; i++) {
>          AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
> +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> +        int apic_id = id.arch_id;
> +
>          apic->type = ACPI_APIC_PROCESSOR;
>          apic->length = sizeof(*apic);
> -        apic->processor_id = i;
> -        apic->local_apic_id = i;
> -        if (test_bit(i, cpu->found_cpus)) {
> +        apic->processor_id = apic_id;
> +        apic->local_apic_id = apic_id;
> +        if (id.cpu != NULL) {

This seems to be the only place where CPUArchId.cpu is being used
(see my previous suggestion about making possible_cpu_arch_ids()
return just an uint64_t list).

Also, using the existing found_cpus bitmap is more efficient than
making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't
mind keeping the bitmap.

>              apic->flags = cpu_to_le32(1);
>          } else {
>              apic->flags = cpu_to_le32(0);
>          }
>      }
> +    g_array_free(apic_id_list, true);
> +
>      io_apic = acpi_data_push(table_data, sizeof *io_apic);
>      io_apic->type = ACPI_APIC_IO;
>      io_apic->length = sizeof(*io_apic);
> @@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
>      aml_len += tables_blob->len - fadt;
>  
>      acpi_add_table(table_offsets, tables_blob);
> -    build_madt(tables_blob, tables->linker, &cpu, guest_info);
> +    build_madt(tables_blob, tables->linker, machine, guest_info);
>  
>      if (misc.has_hpet) {
>          acpi_add_table(table_offsets, tables_blob);
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:47:33PM +0100, Igor Mammedov wrote:
> cpu->found_cpus bitmap is used for setting present
> flag in CPON AML package at start up. But it takes
> a bunch of code to fill bitmap and cloud be simplified
> by calling qemu_get_cpu_by_arch_id(apic_id) directly.
> 
> Hence do so and remove not used anymore bitmap
> with related utilities, which saves us ~32LOC
> and also would simplify consolidating APCI parts
> of CPU hotplug.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

This makes the code loops through all smp_cpus CPUs max_cpus
times, instead of just looping through the smp_cpus CPUs once.

I'm all for making the code simpler, and there's an upper bound:
looking at CPU objects 64k times (because MAX_CPUMASK_BITS is
255). But I want to make sure we agree this is an optimization we
want to drop.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 9/9] pc: acpi: clarify why possible LAPIC entries must be present in MADT

[Eduardo Habkost]

On Thu, Feb 04, 2016 at 12:47:35PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

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;
> > +}  
> [...]
> 

Re: [Qemu-devel] [PATCH 5/9] pc: acpi: create Processor and Notify objects only for valid lapics

[Igor Mammedov]

On Fri, 5 Feb 2016 13:17:14 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 04, 2016 at 12:47:31PM +0100, Igor Mammedov wrote:
> > do not assume that all lapics in range 0..apic_id_limit
> > are valid and do not create Processor and Notify objects
> > for not possible lapics.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Any specific reason you split the Processor/Notify changes and
> the MADT/lapic entries into patches 5 and 6? Won't guests be
> confused if the Processor entries are missing but the lapic
> entries are still there?
Guest shouldn't be confused as MADT is used during while
Processor during hotplug.

> 
> I wouldn't mind merging patches 4-6 into a single patch, just to
> avoid risking unnecessary bisectability issues.
Patches were split to make them smaller and more reviewable
but I'm ok with merging them together

> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Eduardo Habkost]

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).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Igor Mammedov]

On Fri, 5 Feb 2016 13:28:31 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote:
> > do not assume that all lapics in range 0..apic_id_limit
> > are valid and do not create lapic entries for not
> > possible lapics in MADT.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But there's one minor suggestion below:
> 
> > ---
> >  hw/i386/acpi-build.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index df13c7d..9eeeffa 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
> >  }
> >  
> >  static void
> > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > -           PcGuestInfo *guest_info)
> > +build_madt(GArray *table_data, GArray *linker,
> > +           MachineState *machine, PcGuestInfo *guest_info)
> >  {
> > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
> >      int madt_start = table_data->len;
> >  
> >      AcpiMultipleApicTable *madt;
> > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> >      madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
> >      madt->flags = cpu_to_le32(1);
> >  
> > -    for (i = 0; i < guest_info->apic_id_limit; i++) {
> > +    for (i = 0; i < apic_id_list->len; i++) {
> >          AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
> > +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> > +        int apic_id = id.arch_id;
> > +
> >          apic->type = ACPI_APIC_PROCESSOR;
> >          apic->length = sizeof(*apic);
> > -        apic->processor_id = i;
> > -        apic->local_apic_id = i;
> > -        if (test_bit(i, cpu->found_cpus)) {
> > +        apic->processor_id = apic_id;
> > +        apic->local_apic_id = apic_id;
> > +        if (id.cpu != NULL) {  
> 
> This seems to be the only place where CPUArchId.cpu is being used
> (see my previous suggestion about making possible_cpu_arch_ids()
> return just an uint64_t list).
> 
> Also, using the existing found_cpus bitmap is more efficient than
> making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't
> mind keeping the bitmap.
found_cpus bitmap is not better than (id.cpu != NULL) check
the cost of filling both is about the same.
The issue I have with bitmap is that it's harder to generalize
CPU hotplug code with it, while with possible_cpu_arch_ids()
returned array I have a list of CPUs to work with without any
assumptions on position in bitmap or array.
Also bitmap scales worse than a list of CPUs if ID space
is sparse and if ID is quite big.
That's why I'm dropping bitmap and switching to a list of
IDs which in worst case is upto max_cpus.

> 
> >              apic->flags = cpu_to_le32(1);
> >          } else {
> >              apic->flags = cpu_to_le32(0);
> >          }
> >      }
> > +    g_array_free(apic_id_list, true);
> > +
> >      io_apic = acpi_data_push(table_data, sizeof *io_apic);
> >      io_apic->type = ACPI_APIC_IO;
> >      io_apic->length = sizeof(*io_apic);
> > @@ -2659,7 +2666,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
> >      aml_len += tables_blob->len - fadt;
> >  
> >      acpi_add_table(table_offsets, tables_blob);
> > -    build_madt(tables_blob, tables->linker, &cpu, guest_info);
> > +    build_madt(tables_blob, tables->linker, machine, guest_info);
> >  
> >      if (misc.has_hpet) {
> >          acpi_add_table(table_offsets, tables_blob);
> > -- 
> > 1.8.3.1
> >   
> 

Re: [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Igor Mammedov]

On Fri, 5 Feb 2016 13:39:07 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 04, 2016 at 12:47:33PM +0100, Igor Mammedov wrote:
> > cpu->found_cpus bitmap is used for setting present
> > flag in CPON AML package at start up. But it takes
> > a bunch of code to fill bitmap and cloud be simplified
> > by calling qemu_get_cpu_by_arch_id(apic_id) directly.
> > 
> > Hence do so and remove not used anymore bitmap
> > with related utilities, which saves us ~32LOC
> > and also would simplify consolidating APCI parts
> > of CPU hotplug.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> This makes the code loops through all smp_cpus CPUs max_cpus
> times, instead of just looping through the smp_cpus CPUs once.
Yep, that looks bad.
I'll redo it using possible_cpu_arch_ids(), it is a little
bit bigger refactoring.

> 
> I'm all for making the code simpler, and there's an upper bound:
> looking at CPU objects 64k times (because MAX_CPUMASK_BITS is
> 255). But I want to make sure we agree this is an optimization we
> want to drop.
> 

Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

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)

Re: [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Igor Mammedov]

On Fri, 5 Feb 2016 17:19:50 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 5 Feb 2016 13:39:07 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 04, 2016 at 12:47:33PM +0100, Igor Mammedov wrote:  
> > > cpu->found_cpus bitmap is used for setting present
> > > flag in CPON AML package at start up. But it takes
> > > a bunch of code to fill bitmap and cloud be simplified
> > > by calling qemu_get_cpu_by_arch_id(apic_id) directly.
> > > 
> > > Hence do so and remove not used anymore bitmap
> > > with related utilities, which saves us ~32LOC
> > > and also would simplify consolidating APCI parts
> > > of CPU hotplug.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > 
> > This makes the code loops through all smp_cpus CPUs max_cpus
> > times, instead of just looping through the smp_cpus CPUs once.  
> Yep, that looks bad.
> I'll redo it using possible_cpu_arch_ids(), it is a little
> bit bigger refactoring.
I think I'll make bitmap local to build_processor_devices() as it's
needed only for building CPON package, that way it will go along
with related legacy hotplug and won't get in the way of new hotplug.

> 
> > 
> > I'm all for making the code simpler, and there's an upper bound:
> > looking at CPU objects 64k times (because MAX_CPUMASK_BITS is
> > 255). But I want to make sure we agree this is an optimization we
> > want to drop.
> >   
> 
> 

Re: [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Eduardo Habkost]

On Fri, Feb 05, 2016 at 05:44:49PM +0100, Igor Mammedov wrote:
> On Fri, 5 Feb 2016 17:19:50 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 5 Feb 2016 13:39:07 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Feb 04, 2016 at 12:47:33PM +0100, Igor Mammedov wrote:  
> > > > cpu->found_cpus bitmap is used for setting present
> > > > flag in CPON AML package at start up. But it takes
> > > > a bunch of code to fill bitmap and cloud be simplified
> > > > by calling qemu_get_cpu_by_arch_id(apic_id) directly.
> > > > 
> > > > Hence do so and remove not used anymore bitmap
> > > > with related utilities, which saves us ~32LOC
> > > > and also would simplify consolidating APCI parts
> > > > of CPU hotplug.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > 
> > > This makes the code loops through all smp_cpus CPUs max_cpus
> > > times, instead of just looping through the smp_cpus CPUs once.  
> > Yep, that looks bad.
> > I'll redo it using possible_cpu_arch_ids(), it is a little
> > bit bigger refactoring.
> I think I'll make bitmap local to build_processor_devices() as it's
> needed only for building CPON package, that way it will go along
> with related legacy hotplug and won't get in the way of new hotplug.

What about pc_possible_cpu_arch_ids() itself? It has exactly the
same problem: it looks at CPU objects smp_cpus*max_cpus times, to
fill the CPUArchId.cpu field. If that's a problem for
build_processor_devices(), I assume that's a problem for all
other possible_cpu_arch_ids() callers?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Eduardo Habkost]

On Fri, Feb 05, 2016 at 05:14:41PM +0100, Igor Mammedov wrote:
> On Fri, 5 Feb 2016 13:28:31 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote:
> > > do not assume that all lapics in range 0..apic_id_limit
> > > are valid and do not create lapic entries for not
> > > possible lapics in MADT.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > But there's one minor suggestion below:
> > 
> > > ---
> > >  hw/i386/acpi-build.c | 21 ++++++++++++++-------
> > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index df13c7d..9eeeffa 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
> > >  }
> > >  
> > >  static void
> > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > > -           PcGuestInfo *guest_info)
> > > +build_madt(GArray *table_data, GArray *linker,
> > > +           MachineState *machine, PcGuestInfo *guest_info)
> > >  {
> > > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > >      int madt_start = table_data->len;
> > >  
> > >      AcpiMultipleApicTable *madt;
> > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > >      madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
> > >      madt->flags = cpu_to_le32(1);
> > >  
> > > -    for (i = 0; i < guest_info->apic_id_limit; i++) {
> > > +    for (i = 0; i < apic_id_list->len; i++) {
> > >          AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
> > > +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> > > +        int apic_id = id.arch_id;
> > > +
> > >          apic->type = ACPI_APIC_PROCESSOR;
> > >          apic->length = sizeof(*apic);
> > > -        apic->processor_id = i;
> > > -        apic->local_apic_id = i;
> > > -        if (test_bit(i, cpu->found_cpus)) {
> > > +        apic->processor_id = apic_id;
> > > +        apic->local_apic_id = apic_id;
> > > +        if (id.cpu != NULL) {  
> > 
> > This seems to be the only place where CPUArchId.cpu is being used
> > (see my previous suggestion about making possible_cpu_arch_ids()
> > return just an uint64_t list).
> > 
> > Also, using the existing found_cpus bitmap is more efficient than
> > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't
> > mind keeping the bitmap.
> found_cpus bitmap is not better than (id.cpu != NULL) check
> the cost of filling both is about the same.

The cost doesn't look the same. Populating found_cpus should be
O(smp_cpus)[1], and is being done only once. Filling id.cpu in
pc_possible_cpu_arch_ids() is O(max_cpus*smp_cpus), and it is
called multiple times.

[1] I just noticed it is actually O(size_of_qom_tree), but it
    is still linear, and could be changed to O(smp_cpus).

> The issue I have with bitmap is that it's harder to generalize
> CPU hotplug code with it, while with possible_cpu_arch_ids()
> returned array I have a list of CPUs to work with without any
> assumptions on position in bitmap or array.
> Also bitmap scales worse than a list of CPUs if ID space
> is sparse and if ID is quite big.

Yes, I agree that a list is better depending on how the arch ID
space is used.

> That's why I'm dropping bitmap and switching to a list of
> IDs which in worst case is upto max_cpus.

I think the possible_cpu_arch_ids() interface looks good, maybe
we just need to optimize it.

But I'm not sure if we need to optimize it now, or if we can live
with the inefficient code and optimize it later. I won't complain
if we do it later, if we warn about it in the commit message or
comments.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 6/9] pc: acpi: create MADT.lapic entries only for valid lapics

[Igor Mammedov]

On Thu, 11 Feb 2016 14:11:34 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Feb 05, 2016 at 05:14:41PM +0100, Igor Mammedov wrote:
> > On Fri, 5 Feb 2016 13:28:31 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Feb 04, 2016 at 12:47:32PM +0100, Igor Mammedov wrote:  
> > > > do not assume that all lapics in range 0..apic_id_limit
> > > > are valid and do not create lapic entries for not
> > > > possible lapics in MADT.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > 
> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> > > 
> > > But there's one minor suggestion below:
> > >   
> > > > ---
> > > >  hw/i386/acpi-build.c | 21 ++++++++++++++-------
> > > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index df13c7d..9eeeffa 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -361,9 +361,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
> > > >  }
> > > >  
> > > >  static void
> > > > -build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > > > -           PcGuestInfo *guest_info)
> > > > +build_madt(GArray *table_data, GArray *linker,
> > > > +           MachineState *machine, PcGuestInfo *guest_info)
> > > >  {
> > > > +    MachineClass *mc = MACHINE_GET_CLASS(machine);
> > > > +    GArray *apic_id_list = mc->possible_cpu_arch_ids();
> > > >      int madt_start = table_data->len;
> > > >  
> > > >      AcpiMultipleApicTable *madt;
> > > > @@ -376,18 +378,23 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
> > > >      madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
> > > >      madt->flags = cpu_to_le32(1);
> > > >  
> > > > -    for (i = 0; i < guest_info->apic_id_limit; i++) {
> > > > +    for (i = 0; i < apic_id_list->len; i++) {
> > > >          AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
> > > > +        CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
> > > > +        int apic_id = id.arch_id;
> > > > +
> > > >          apic->type = ACPI_APIC_PROCESSOR;
> > > >          apic->length = sizeof(*apic);
> > > > -        apic->processor_id = i;
> > > > -        apic->local_apic_id = i;
> > > > -        if (test_bit(i, cpu->found_cpus)) {
> > > > +        apic->processor_id = apic_id;
> > > > +        apic->local_apic_id = apic_id;
> > > > +        if (id.cpu != NULL) {    
> > > 
> > > This seems to be the only place where CPUArchId.cpu is being used
> > > (see my previous suggestion about making possible_cpu_arch_ids()
> > > return just an uint64_t list).
> > > 
> > > Also, using the existing found_cpus bitmap is more efficient than
> > > making multiple calls to qemu_get_cpu_by_arch_id(). I wouldn't
> > > mind keeping the bitmap.  
> > found_cpus bitmap is not better than (id.cpu != NULL) check
> > the cost of filling both is about the same.  
> 
> The cost doesn't look the same. Populating found_cpus should be
> O(smp_cpus)[1], and is being done only once. Filling id.cpu in
> pc_possible_cpu_arch_ids() is O(max_cpus*smp_cpus), and it is
> called multiple times.
I've refactored patch to make pc_possible_cpu_arch_ids() linear,
pls see v2:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg351298.html

> 
> [1] I just noticed it is actually O(size_of_qom_tree), but it
>     is still linear, and could be changed to O(smp_cpus).
> 
> > The issue I have with bitmap is that it's harder to generalize
> > CPU hotplug code with it, while with possible_cpu_arch_ids()
> > returned array I have a list of CPUs to work with without any
> > assumptions on position in bitmap or array.
> > Also bitmap scales worse than a list of CPUs if ID space
> > is sparse and if ID is quite big.  
> 
> Yes, I agree that a list is better depending on how the arch ID
> space is used.
> 
> > That's why I'm dropping bitmap and switching to a list of
> > IDs which in worst case is upto max_cpus.  
> 
> I think the possible_cpu_arch_ids() interface looks good, maybe
> we just need to optimize it.
> 
> But I'm not sure if we need to optimize it now, or if we can live
> with the inefficient code and optimize it later. I won't complain
> if we do it later, if we warn about it in the commit message or
> comments.
> 

Re: [Qemu-devel] [PATCH 7/9] pc: acpi: drop not needed intermediate bitmap cpu->found_cpus

[Igor Mammedov]

On Thu, 11 Feb 2016 13:59:33 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Feb 05, 2016 at 05:44:49PM +0100, Igor Mammedov wrote:
> > On Fri, 5 Feb 2016 17:19:50 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Fri, 5 Feb 2016 13:39:07 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Feb 04, 2016 at 12:47:33PM +0100, Igor Mammedov wrote:    
> > > > > cpu->found_cpus bitmap is used for setting present
> > > > > flag in CPON AML package at start up. But it takes
> > > > > a bunch of code to fill bitmap and cloud be simplified
> > > > > by calling qemu_get_cpu_by_arch_id(apic_id) directly.
> > > > > 
> > > > > Hence do so and remove not used anymore bitmap
> > > > > with related utilities, which saves us ~32LOC
> > > > > and also would simplify consolidating APCI parts
> > > > > of CPU hotplug.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>      
> > > > 
> > > > This makes the code loops through all smp_cpus CPUs max_cpus
> > > > times, instead of just looping through the smp_cpus CPUs once.    
> > > Yep, that looks bad.
> > > I'll redo it using possible_cpu_arch_ids(), it is a little
> > > bit bigger refactoring.  
> > I think I'll make bitmap local to build_processor_devices() as it's
> > needed only for building CPON package, that way it will go along
> > with related legacy hotplug and won't get in the way of new hotplug.  
> 
> What about pc_possible_cpu_arch_ids() itself? It has exactly the
> same problem: it looks at CPU objects smp_cpus*max_cpus times, to
> fill the CPUArchId.cpu field. If that's a problem for
> build_processor_devices(), I assume that's a problem for all
> other possible_cpu_arch_ids() callers?
pls see v2
https://www.mail-archive.com/qemu-devel@nongnu.org/msg351298.html