[Qemu-devel] [PATCH v2 00/10] pc: do not create invalid MADT.LAPIC/Processor entries

[Qemu-devel] [PATCH v2 00/10] pc: do not create invalid MADT.LAPIC/Processor entries

[Igor Mammedov]

Changes since v1:
 - rebased on top of PCI tree that contains
   Eduardo's guest_info removel series
 - fix ^2 times present CPU lookup when creating CPON
   package (spotted-by: Eduardo Habkost <ehabkost@redhat.com>)

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

Tested hoptlug with: RHEL72 and WS2003.
Git tree for testing:
https://github.com/imammedo/qemu.git pc_madt_dsdt_lapic_cleanups_v2

* 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 (10):
  cpu: rename cpu_exists() to qemu_get_cpu_by_arch_id()
  machine: introduce MachineClass.possible_cpu_arch_ids() hook
  pc: init pcms->apic_id_limit once and use it throughout pc.c
  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: move cpu->found_cpus bitmap to build_processor_devices()
  pc: acpi: clarify why possible LAPIC entries must be present in MADT
  pc: acpi: remove NOP assignment

 hw/i386/acpi-build.c | 144 +++++++++++++++++++++++++++------------------------
 hw/i386/pc.c         |  63 ++++++++++++----------
 include/hw/boards.h  |  18 +++++++
 include/qom/cpu.h    |  10 ++--
 qom/cpu.c            |   6 +--
 target-i386/cpu.c    |   2 +-
 6 files changed, 139 insertions(+), 104 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 01/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 fd8524f..9227bde 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 38dc713..2c4a4e3 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -27,7 +27,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;
 
@@ -35,10 +35,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

[Qemu-devel] [PATCH v2 02/10] 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 9227bde..548ec64 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1931,6 +1931,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
     return topo.pkg_id;
 }
 
+static GArray *pc_possible_cpu_arch_ids(void)
+{
+    int i;
+    GArray *list = g_array_new(FALSE, FALSE, sizeof(CPUArchId));
+
+    for (i = 0; i < max_cpus; i++) {
+        CPUArchId val;
+
+        val.arch_id = x86_cpu_apic_id_from_index(i);
+        val.cpu = qemu_get_cpu_by_arch_id(val.arch_id);
+        g_array_append_val(list, val);
+    }
+    return list;
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1953,6 +1968,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     pcmc->save_tsc_khz = true;
     mc->get_hotplug_handler = pc_get_hotpug_handler;
     mc->cpu_index_to_socket_id = pc_cpu_index_to_socket_id;
+    mc->possible_cpu_arch_ids = pc_possible_cpu_arch_ids;
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..bd85f46 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -8,6 +8,7 @@
 #include "sysemu/accel.h"
 #include "hw/qdev.h"
 #include "qom/object.h"
+#include "qom/cpu.h"
 
 void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
                                           const char *name,
@@ -42,6 +43,16 @@ bool machine_dump_guest_core(MachineState *machine);
 bool machine_mem_merge(MachineState *machine);
 
 /**
+ * CPUArchId:
+ * @arch_id - architecture-dependent CPU ID of present or possible CPU
+ * @cpu - pointer to corresponding CPU object ii it's present on NULL otherwise
+ */
+typedef struct {
+    uint64_t arch_id;
+    struct CPUState *cpu;
+} CPUArchId;
+
+/**
  * MachineClass:
  * @get_hotplug_handler: this function is called during bus-less
  *    device hotplug. If defined it returns pointer to an instance
@@ -57,6 +68,10 @@ bool machine_mem_merge(MachineState *machine);
  *    Set only by old machines because they need to keep
  *    compatibility on code that exposed QEMU_VERSION to guests in
  *    the past (and now use qemu_hw_version()).
+ * @possible_cpu_arch_ids:
+ *    Returns an array of @CPUArchId architecture-dependent CPU IDs
+ *    which includes CPU IDs for present and possible to hotplug CPUs.
+ *    Caller is responsible for freeing returned list.
  */
 struct MachineClass {
     /*< private >*/
@@ -99,8 +114,11 @@ struct MachineClass {
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
+    GArray *(*possible_cpu_arch_ids)(void);
 };
 
+#define FETCH_CPU_ARCH_ID(array, idx) g_array_index(array, CPUArchId, idx)
+
 /**
  * MachineState:
  */
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 03/10] pc: init pcms->apic_id_limit once and use it throughout pc.c

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 548ec64..b863265 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,6 @@ void pc_cpus_init(PCMachineState *pcms)
     int i;
     X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
-    unsigned long apic_id_limit;
 
     /* init CPUs */
     if (machine->cpu_model == NULL) {
@@ -1131,10 +1118,17 @@ 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);
+    /* Calculates the limit to CPU APIC ID values
+     *
+     * Limit for the APIC ID value, 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().
+     */
+    pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 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);
     }
 
@@ -1187,7 +1181,6 @@ void pc_guest_info_init(PCMachineState *pcms)
 {
     int i, j;
 
-    pcms->apic_id_limit = pc_apic_id_limit(max_cpus);
     pcms->apic_xrupt_override = kvm_allows_irq0_override();
     pcms->numa_nodes = nb_numa_nodes;
     pcms->node_mem = g_malloc0(pcms->numa_nodes *
@@ -1372,7 +1365,7 @@ void 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);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 04/10] 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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index caa2e87..4756924 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1972,13 +1972,12 @@ static Aml *build_q35_osc_method(void)
 static void
 build_dsdt(GArray *table_data, GArray *linker,
            AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc,
-           PcPciInfo *pci)
+           PcPciInfo *pci, 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());
     PCMachineState *pcms = PC_MACHINE(machine);
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
@@ -2401,7 +2400,7 @@ acpi_build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 }
 
 static void
-build_srat(GArray *table_data, GArray *linker)
+build_srat(GArray *table_data, GArray *linker, MachineState *machine)
 {
     AcpiSystemResourceAffinityTable *srat;
     AcpiSratProcessorAffinity *core;
@@ -2411,7 +2410,7 @@ build_srat(GArray *table_data, GArray *linker)
     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);
@@ -2614,9 +2613,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;
 }
@@ -2637,6 +2636,7 @@ void acpi_build(AcpiBuildTables *tables)
     size_t aml_len = 0;
     GArray *tables_blob = tables->table_data;
     AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
+    MachineState *machine = MACHINE(qdev_get_machine());
 
     acpi_get_cpu_info(&cpu);
     acpi_get_pm_info(&pm);
@@ -2662,7 +2662,7 @@ void acpi_build(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, &cpu, &pm, &misc, &pci, machine);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
      * sizing of ACPI tables.
@@ -2694,7 +2694,7 @@ void acpi_build(AcpiBuildTables *tables)
     }
     if (pcms->numa_nodes) {
         acpi_add_table(table_offsets, tables_blob);
-        build_srat(tables_blob, tables->linker);
+        build_srat(tables_blob, tables->linker, machine);
     }
     if (acpi_get_mcfg(&mcfg)) {
         acpi_add_table(table_offsets, tables_blob);
@@ -2705,7 +2705,7 @@ void acpi_build(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 v2 05/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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 4756924..de265aa 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2410,6 +2410,8 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
     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,
@@ -2421,12 +2423,15 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
     srat->reserved1 = cpu_to_le32(1);
     core = (void *)(srat + 1);
 
-    for (i = 0; i < pcms->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 = pcms->node_cpu[i];
+        core->local_apic_id = apic_id;
+        curnode = pcms->node_cpu[apic_id];
         core->proximity_lo = curnode;
         memset(core->proximity_hi, 0, 3);
         core->local_sapic_eid = 0;
@@ -2491,6 +2496,7 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
                  (void *)(table_data->data + srat_start),
                  "SRAT",
                  table_data->len - srat_start, 1, NULL, NULL);
+    g_array_free(apic_id_list, true);
 }
 
 static void
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 06/10] 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>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index de265aa..de55a72 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -961,7 +961,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, MachineState *machine,
                                     AcpiCpuInfo *cpu, AcpiPmInfo *pm)
 {
     int i;
@@ -971,11 +971,14 @@ 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();
+    PCMachineState *pcms = PC_MACHINE(machine);
 
     /* 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));
@@ -1000,22 +1003,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);
 
@@ -1027,10 +1035,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);
     }
@@ -1043,14 +1054,15 @@ 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 = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
         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,
@@ -1978,7 +1990,6 @@ build_dsdt(GArray *table_data, GArray *linker,
     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);
-    PCMachineState *pcms = PC_MACHINE(machine);
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *bus = NULL;
@@ -2279,7 +2290,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        build_processor_devices(sb_scope, pcms->apic_id_limit, cpu, pm);
+        build_processor_devices(sb_scope, machine, cpu, pm);
 
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
                              pm->mem_hp_io_len);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 07/10] 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 | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index de55a72..72ecb0c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -363,8 +363,10 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
 }
 
 static void
-build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu)
+build_madt(GArray *table_data, GArray *linker, MachineState *machine)
 {
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    GArray *apic_id_list = mc->possible_cpu_arch_ids();
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     int madt_start = table_data->len;
 
@@ -378,18 +380,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 < pcms->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);
@@ -2694,7 +2701,7 @@ void acpi_build(AcpiBuildTables *tables)
     aml_len += tables_blob->len - fadt;
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, &cpu);
+    build_madt(tables_blob, tables->linker, machine);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 08/10] pc: acpi: move cpu->found_cpus bitmap to build_processor_devices()

[Igor Mammedov]

cpu->found_cpus bitmap is used for setting present
flag in CPON AML package. But it takes a bunch of code
to fill bitmap and could be simplified by getting
presense info from possible CPUs list directly.

So move found_cpus bitmap to the only place that uses
it for building sparse CPON AML package.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 72ecb0c..6ac05b3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -77,10 +77,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;
@@ -122,31 +118,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();
@@ -969,7 +940,7 @@ static Aml *build_crs(PCIHostState *host,
 }
 
 static void build_processor_devices(Aml *sb_scope, MachineState *machine,
-                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm)
+                                    AcpiPmInfo *pm)
 {
     int i;
     Aml *dev;
@@ -981,7 +952,9 @@ static void build_processor_devices(Aml *sb_scope, MachineState *machine,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     GArray *apic_id_list = mc->possible_cpu_arch_ids();
     PCMachineState *pcms = PC_MACHINE(machine);
+    DECLARE_BITMAP(found_cpus, ACPI_CPU_HOTPLUG_ID_LIMIT);
 
+    memset(found_cpus, 0, sizeof found_cpus);
     /* 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);
@@ -1015,6 +988,14 @@ static void build_processor_devices(Aml *sb_scope, MachineState *machine,
         int apic_id = id.arch_id;
 
         assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
+        /*
+         * prefill present CPUs bitmap which will be used
+         * later for generating sparse CPU_ON_BITMAP
+         */
+        if (id.cpu != NULL) {
+            set_bit(apic_id, found_cpus);
+        }
+
         dev = aml_processor(apic_id, 0, 0, "CP%.02X", apic_id);
 
         method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
@@ -1065,7 +1046,7 @@ static void build_processor_devices(Aml *sb_scope, MachineState *machine,
                                        aml_varpackage(pcms->apic_id_limit);
 
     for (i = 0; i < pcms->apic_id_limit; i++) {
-        uint8_t b = test_bit(i, cpu->found_cpus) ? 0x01 : 0x00;
+        uint8_t b = test_bit(i, found_cpus) ? 0x01 : 0x00;
         aml_append(pkg, aml_int(b));
     }
     aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
@@ -1990,7 +1971,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, MachineState *machine)
 {
     CrsRangeEntry *entry;
@@ -2297,7 +2278,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        build_processor_devices(sb_scope, machine, cpu, pm);
+        build_processor_devices(sb_scope, machine, pm);
 
         build_memory_devices(sb_scope, nr_mem, pm->mem_hp_io_base,
                              pm->mem_hp_io_len);
@@ -2651,7 +2632,6 @@ void acpi_build(AcpiBuildTables *tables)
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
     unsigned facs, dsdt, rsdt, fadt;
-    AcpiCpuInfo cpu;
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
     AcpiMcfgInfo mcfg;
@@ -2662,7 +2642,6 @@ void acpi_build(AcpiBuildTables *tables)
     AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
     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);
@@ -2686,7 +2665,7 @@ void acpi_build(AcpiBuildTables *tables)
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
-    build_dsdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci, machine);
+    build_dsdt(tables_blob, tables->linker, &pm, &misc, &pci, machine);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
      * sizing of ACPI tables.
-- 
1.8.3.1

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@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 6ac05b3..5fc8019 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -363,6 +363,12 @@ build_madt(GArray *table_data, GArray *linker, MachineState *machine)
         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 v2 10/10] pc: acpi: remove NOP assignment

[Igor Mammedov]

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5fc8019..7ac46f4 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2426,7 +2426,6 @@ build_srat(GArray *table_data, GArray *linker, MachineState *machine)
 
     srat = acpi_data_push(table_data, sizeof *srat);
     srat->reserved1 = cpu_to_le32(1);
-    core = (void *)(srat + 1);
 
     for (i = 0; i < apic_id_list->len; i++) {
         CPUArchId id = FETCH_CPU_ARCH_ID(apic_id_list, i);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 02/10] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Eduardo Habkost]

On Fri, Feb 05, 2016 at 07:06:58PM +0100, Igor Mammedov wrote:
> on x86 currently range 0..max_cpus is used to generate
> architecture-dependent CPU ID (APIC Id) for each present
> and possible CPUs. However architecture-dependent CPU IDs
> list could be sparse and code that needs to enumerate
> all IDs (ACPI) ended up doing guess work enumerating all
> possible and impossible IDs up to
>   apic_id_limit = x86_cpu_apic_id_from_index(max_cpus).
> 
> That leads to creation of MADT entries and Processor
> objects in ACPI tables for not possible CPUs.
> Fix it by allowing board specify a concrete list of
> CPU IDs accourding its own rules (which for x86 depends
> on topology). So that code that needs this list could
> request it from board instead of trying to figure out
> what IDs are correct on its own.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c        | 16 ++++++++++++++++
>  include/hw/boards.h | 18 ++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 9227bde..548ec64 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1931,6 +1931,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
>      return topo.pkg_id;
>  }
>  
> +static GArray *pc_possible_cpu_arch_ids(void)
> +{
> +    int i;
> +    GArray *list = g_array_new(FALSE, FALSE, sizeof(CPUArchId));
> +
> +    for (i = 0; i < max_cpus; i++) {
> +        CPUArchId val;
> +
> +        val.arch_id = x86_cpu_apic_id_from_index(i);
> +        val.cpu = qemu_get_cpu_by_arch_id(val.arch_id);
> +        g_array_append_val(list, val);
> +    }
> +    return list;
> +}

You claim this version is linear, but I don't see any change from
v1, except for a whitespace change.

qemu_get_cpu_by_arch_id() is O(smp_cpus). This calls
qemu_get_cpu_by_arch_id() max_cpus times, so this is
O(max_cpus*smp_cpus).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 02/10] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

On Fri, 12 Feb 2016 16:33:28 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Feb 05, 2016 at 07:06:58PM +0100, Igor Mammedov wrote:
> > on x86 currently range 0..max_cpus is used to generate
> > architecture-dependent CPU ID (APIC Id) for each present
> > and possible CPUs. However architecture-dependent CPU IDs
> > list could be sparse and code that needs to enumerate
> > all IDs (ACPI) ended up doing guess work enumerating all
> > possible and impossible IDs up to
> >   apic_id_limit = x86_cpu_apic_id_from_index(max_cpus).
> > 
> > That leads to creation of MADT entries and Processor
> > objects in ACPI tables for not possible CPUs.
> > Fix it by allowing board specify a concrete list of
> > CPU IDs accourding its own rules (which for x86 depends
> > on topology). So that code that needs this list could
> > request it from board instead of trying to figure out
> > what IDs are correct on its own.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c        | 16 ++++++++++++++++
> >  include/hw/boards.h | 18 ++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 9227bde..548ec64 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1931,6 +1931,21 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
> >      return topo.pkg_id;
> >  }
> >  
> > +static GArray *pc_possible_cpu_arch_ids(void)
> > +{
> > +    int i;
> > +    GArray *list = g_array_new(FALSE, FALSE, sizeof(CPUArchId));
> > +
> > +    for (i = 0; i < max_cpus; i++) {
> > +        CPUArchId val;
> > +
> > +        val.arch_id = x86_cpu_apic_id_from_index(i);
> > +        val.cpu = qemu_get_cpu_by_arch_id(val.arch_id);
> > +        g_array_append_val(list, val);
> > +    }
> > +    return list;
> > +}  
> 
> You claim this version is linear, but I don't see any change from
> v1, except for a whitespace change.
> 
> qemu_get_cpu_by_arch_id() is O(smp_cpus). This calls
> qemu_get_cpu_by_arch_id() max_cpus times, so this is
> O(max_cpus*smp_cpus).
I'm sorry, I've fixed it for ACPI tables only but not here.
I've just posted and alternative patch for possible CPU
in CPU hotplug introspection context
 '[RFC] QMP: add query-hotpluggable-cpus'
which creates list in 2*O(N) and also makes access to list
more typesafe. Lets see how it goes and later I can respin
this series with that patch included.