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

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

[Igor Mammedov]

Changes since v2:
 - build possible CPUs list only once.
     Marcel Apfelbaum <marcel@redhat.com>
 - replace MachineClass possible_cpu_arch_ids() hook with
   QOM interface, so only targets that need it would implement it
 - fix ^2 times present CPU lookup for initial CPUs
    Eduardo Habkost <ehabkost@redhat.com>
 - drop found_cpus bitmap altogether

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 / WS2012R2.
Git tree for testing:
https://github.com/imammedo/qemu.git pc_madt_dsdt_lapic_cleanups_v3

* 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 (8):
  pc: init pcms->apic_id_limit once and use it throughout pc.c
  cpu: introduce possible-cpus interface
  pc: acpi: cleanup qdev_get_machine() calls
  pc: acpi: SRAT: create only valid processor lapic entries
  pc: acpi: create MADT.lapic entries only for valid lapics
  pc: acpi: create Processor and Notify objects only for valid lapics
  pc: acpi: drop cpu->found_cpus bitmap
  pc: acpi: clarify why possible LAPIC entries must be present in MADT

 hw/i386/acpi-build.c | 150 ++++++++++++++++++++++++++-------------------------
 hw/i386/pc.c         |  91 ++++++++++++++++++++-----------
 include/hw/boards.h  |   1 +
 include/hw/i386/pc.h |   1 +
 include/qom/cpu.h    |  47 ++++++++++++++++
 qom/cpu.c            |   7 +++
 6 files changed, 193 insertions(+), 104 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/8] 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 0aeefd2..151a64c 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 v3 2/8] cpu: introduce possible-cpus interface

[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/SRAT 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.

This interface will also allow to help making AML
part of CPU hotplug target independent so it could
be reused for ARM target.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
CC: afaerber@suse.de
---
 hw/i386/pc.c         | 46 ++++++++++++++++++++++++++++++++++++++++++----
 include/hw/boards.h  |  1 +
 include/hw/i386/pc.h |  1 +
 include/qom/cpu.h    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 qom/cpu.c            |  7 +++++++
 5 files changed, 98 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 151a64c..bfc5bae 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1132,10 +1132,17 @@ void pc_cpus_init(PCMachineState *pcms)
         exit(1);
     }
 
-    for (i = 0; i < smp_cpus; i++) {
-        cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
-                         &error_fatal);
-        object_unref(OBJECT(cpu));
+    pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
+                                    sizeof(CPUArchId) * (max_cpus - 1));
+    for (i = 0; i < max_cpus; i++) {
+        pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
+        pcms->possible_cpus->len++;
+        if (i < smp_cpus) {
+            cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
+                             &error_fatal);
+            pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
+            object_unref(OBJECT(cpu));
+        }
     }
 
     /* tell smbios about cpuid version and features */
@@ -1658,9 +1665,19 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev,
     error_propagate(errp, local_err);
 }
 
+static int pc_apic_cmp(const void *a, const void *b)
+{
+   CPUArchId *apic_a = (CPUArchId *)a;
+   CPUArchId *apic_b = (CPUArchId *)b;
+
+   return apic_a->arch_id - apic_b->arch_id;
+}
+
 static void pc_cpu_plug(HotplugHandler *hotplug_dev,
                         DeviceState *dev, Error **errp)
 {
+    CPUClass *cc = CPU_GET_CLASS(dev);
+    CPUArchId apic_id, *found_cpu;
     HotplugHandlerClass *hhc;
     Error *local_err = NULL;
     PCMachineState *pcms = PC_MACHINE(hotplug_dev);
@@ -1683,6 +1700,13 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
 
     /* increment the number of CPUs */
     rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
+
+    apic_id.arch_id = cc->get_arch_id(CPU(dev));
+    found_cpu = bsearch(&apic_id, pcms->possible_cpus->cpus,
+        pcms->possible_cpus->len, sizeof(*pcms->possible_cpus->cpus),
+        pc_apic_cmp);
+    assert(found_cpu);
+    found_cpu->cpu = CPU(dev);
 out:
     error_propagate(errp, local_err);
 }
@@ -1925,11 +1949,23 @@ static unsigned pc_cpu_index_to_socket_id(unsigned cpu_index)
     return topo.pkg_id;
 }
 
+static CPUArchIdList *pc_get_possible_cpus_list(PossibleCpus *pcs)
+{
+    PCMachineState *pcms = PC_MACHINE(pcs);
+    int len = sizeof(CPUArchIdList) +
+              sizeof(CPUArchId) * (pcms->possible_cpus->len - 1);
+    CPUArchIdList *list = g_malloc(len);
+
+    memcpy(list, pcms->possible_cpus, len);
+    return list;
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     PCMachineClass *pcmc = PC_MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    PossibleCpusClass *psc = POSSIBLE_CPUS_CLASS(oc);
 
     pcmc->get_hotplug_handler = mc->get_hotplug_handler;
     pcmc->pci_enabled = true;
@@ -1954,6 +1990,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     hc->plug = pc_machine_device_plug_cb;
     hc->unplug_request = pc_machine_device_unplug_request_cb;
     hc->unplug = pc_machine_device_unplug_cb;
+    psc->get_possible_cpus_list = pc_get_possible_cpus_list;
 }
 
 static const TypeInfo pc_machine_info = {
@@ -1966,6 +2003,7 @@ static const TypeInfo pc_machine_info = {
     .class_init = pc_machine_class_init,
     .interfaces = (InterfaceInfo[]) {
          { TYPE_HOTPLUG_HANDLER },
+         { TYPE_POSSIBLE_CPUS },
          { }
     },
 };
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 0f30959..90a9d15 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,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8b3546e..3e09232 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -65,6 +65,7 @@ struct PCMachineState {
     /* CPU and apic information: */
     bool apic_xrupt_override;
     unsigned apic_id_limit;
+    CPUArchIdList *possible_cpus;
 
     /* NUMA information: */
     uint64_t numa_nodes;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index ff54600..eedd3f2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -837,4 +837,51 @@ extern const struct VMStateDescription vmstate_cpu_common;
     .offset = 0,                                                            \
 }
 
+#define TYPE_POSSIBLE_CPUS "possible-cpus"
+
+#define POSSIBLE_CPUS_CLASS(klass) \
+     OBJECT_CLASS_CHECK(PossibleCpusClass, (klass), \
+                        TYPE_POSSIBLE_CPUS)
+#define POSSIBLE_CPUS_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(PossibleCpusClass, (obj), \
+                      TYPE_POSSIBLE_CPUS)
+#define POSSIBLE_CPUS(obj) \
+     INTERFACE_CHECK(PossibleCpus, (obj), \
+                     TYPE_POSSIBLE_CPUS)
+
+typedef struct PossibleCpus {
+    /* <private> */
+    Object Parent;
+} PossibleCpus;
+
+/**
+ * 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;
+
+typedef struct {
+    int len;
+    CPUArchId cpus[1];
+} CPUArchIdList;
+
+/**
+ * PossibleCpusClass:
+ * @get_possible_cpus_list:
+ *    Returns asorted by arch_id array of possible CPUs,
+ *    which includes CPU IDs for present and possible to hotplug CPUs.
+ *    Caller is responsible for freeing returned list.
+ */
+typedef struct PossibleCpusClass {
+    /* <private> */
+    InterfaceClass parent_class;
+
+    /* <public> */
+    CPUArchIdList *(*get_possible_cpus_list)(PossibleCpus *pcs);
+} PossibleCpusClass;
+
 #endif
diff --git a/qom/cpu.c b/qom/cpu.c
index aeb32f1..8e9968f 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -384,9 +384,16 @@ static const TypeInfo cpu_type_info = {
     .class_init = cpu_class_init,
 };
 
+static const TypeInfo possible_cpus_interface_info = {
+    .name          = TYPE_POSSIBLE_CPUS,
+    .parent        = TYPE_INTERFACE,
+    .class_size = sizeof(PossibleCpusClass),
+};
+
 static void cpu_register_types(void)
 {
     type_register_static(&cpu_type_info);
+    type_register_static(&possible_cpus_interface_info);
 }
 
 type_init(cpu_register_types)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/8] cpu: introduce possible-cpus interface

[Eduardo Habkost]

On Tue, Feb 23, 2016 at 05:05:54PM +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/SRAT 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.
> 
> This interface will also allow to help making AML
> part of CPU hotplug target independent so it could
> be reused for ARM target.

Do you expect any non-TYPE_MACHINE class to implement this
interface? If not, why not just make it a simple MachineClass
field? Machines that don't implement the get_possible_cpus_list()
method would just have it set to NULL.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 2/8] cpu: introduce possible-cpus interface

[Igor Mammedov]

On Tue, 23 Feb 2016 18:38:33 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Feb 23, 2016 at 05:05:54PM +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/SRAT 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.
> > 
> > This interface will also allow to help making AML
> > part of CPU hotplug target independent so it could
> > be reused for ARM target.  
> 
> Do you expect any non-TYPE_MACHINE class to implement this
> interface? If not, why not just make it a simple MachineClass
> field? Machines that don't implement the get_possible_cpus_list()
> method would just have it set to NULL.
I redid it with interface so that not clatter MachineClass with fields
that won't be used by all(most) machines, as they don't care about
hotplug. There is no other reason than that.
If you think it's better to do with MachineClass hook I can quickly
respin series with it.

Marcel,
what is your opinion on it?

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

[Igor Mammedov]

cache qdev_get_machine() result in acpi_setup/acpi_build_update
time and pass it as an argument to child functions that need it.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4554eb8..3b4468e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -362,9 +362,9 @@ 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, PCMachineState *pcms,
+           AcpiCpuInfo *cpu)
 {
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -1938,13 +1938,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;
@@ -2367,7 +2366,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;
@@ -2377,7 +2376,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);
@@ -2580,17 +2579,17 @@ 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;
 }
 
 static
-void acpi_build(AcpiBuildTables *tables)
+void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 {
-    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
+    PCMachineState *pcms = PC_MACHINE(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
     unsigned facs, dsdt, rsdt, fadt;
@@ -2628,7 +2627,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.
@@ -2643,7 +2642,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, pcms, &cpu);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
@@ -2660,7 +2659,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);
@@ -2671,7 +2670,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);
     }
 
@@ -2765,7 +2764,7 @@ static void acpi_build_update(void *build_opaque)
 
     acpi_build_tables_init(&tables);
 
-    acpi_build(&tables);
+    acpi_build(&tables, MACHINE(qdev_get_machine()));
 
     acpi_ram_update(build_state->table_mr, tables.table_data);
 
@@ -2830,7 +2829,7 @@ void acpi_setup(void)
     acpi_set_pci_info();
 
     acpi_build_tables_init(&tables);
-    acpi_build(&tables);
+    acpi_build(&tables, MACHINE(pcms));
 
     /* Now expose it all to Guest */
     build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-- 
1.8.3.1

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

[Igor Mammedov]

When APIC IDs are sparse*, in addition to valid LAPIC
entries the SRAT is also filled invalid ones for non
possible 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 3b4468e..69e1324 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2376,6 +2376,9 @@ 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;
+    PossibleCpusClass *psc = POSSIBLE_CPUS_GET_CLASS(machine);
+    CPUArchIdList *apic_ids =
+        psc->get_possible_cpus_list(POSSIBLE_CPUS(machine));
     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 +2390,14 @@ 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_ids->len; i++) {
+        int apic_id = apic_ids->cpus[i].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;
@@ -2457,6 +2462,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_free(apic_ids);
 }
 
 static void
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 5/8] 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 | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 69e1324..0a90a54 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -362,9 +362,11 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
 }
 
 static void
-build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms,
-           AcpiCpuInfo *cpu)
+build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
 {
+    PossibleCpusClass *psc = POSSIBLE_CPUS_GET_CLASS(pcms);
+    CPUArchIdList *apic_ids =
+        psc->get_possible_cpus_list(POSSIBLE_CPUS(pcms));
     int madt_start = table_data->len;
 
     AcpiMultipleApicTable *madt;
@@ -377,18 +379,22 @@ build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms,
     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_ids->len; i++) {
         AcpiMadtProcessorApic *apic = acpi_data_push(table_data, sizeof *apic);
+        int apic_id = apic_ids->cpus[i].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 (apic_ids->cpus[i].cpu != NULL) {
             apic->flags = cpu_to_le32(1);
         } else {
             apic->flags = cpu_to_le32(0);
         }
     }
+    g_free(apic_ids);
+
     io_apic = acpi_data_push(table_data, sizeof *io_apic);
     io_apic->type = ACPI_APIC_IO;
     io_apic->length = sizeof(*io_apic);
@@ -2648,7 +2654,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     aml_len += tables_blob->len - fadt;
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, pcms, &cpu);
+    build_madt(tables_blob, tables->linker, pcms);
 
     if (misc.has_hpet) {
         acpi_add_table(table_offsets, tables_blob);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 6/8] 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 | 40 +++++++++++++++++++++++++---------------
 1 file changed, 25 insertions(+), 15 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0a90a54..4c3e8fc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -966,7 +966,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;
@@ -976,11 +976,15 @@ static void build_processor_devices(Aml *sb_scope, unsigned acpi_cpus,
     Aml *field;
     Aml *ifctx;
     Aml *method;
+    PossibleCpusClass *psc = POSSIBLE_CPUS_GET_CLASS(machine);
+    CPUArchIdList *apic_ids =
+        psc->get_possible_cpus_list(POSSIBLE_CPUS(machine));
+    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));
@@ -1005,22 +1009,26 @@ 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_ids->len; i++) {
+        int apic_id = apic_ids->cpus[i].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);
 
@@ -1032,10 +1040,12 @@ 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_ids->len; i++) {
+        int apic_id = apic_ids->cpus[i].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);
     }
@@ -1048,14 +1058,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_free(apic_ids);
 }
 
 static void build_memory_devices(Aml *sb_scope, int nr_mem,
@@ -1950,7 +1961,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;
@@ -2251,7 +2261,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 v3 7/8] pc: acpi: drop cpu->found_cpus bitmap

[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 drop cpu->found_cpus bitmap and unroll possible
CPUs list into APIC index array at the place where
CPUON AML package is created.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4c3e8fc..e66c8b7 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,9 +938,9 @@ static Aml *build_crs(PCIHostState *host,
 }
 
 static void build_processor_devices(Aml *sb_scope, MachineState *machine,
-                                    AcpiCpuInfo *cpu, AcpiPmInfo *pm)
+                                    AcpiPmInfo *pm)
 {
-    int i;
+    int i, apic_idx;
     Aml *dev;
     Aml *crs;
     Aml *pkg;
@@ -1013,6 +984,7 @@ static void build_processor_devices(Aml *sb_scope, MachineState *machine,
         int apic_id = apic_ids->cpus[i].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);
@@ -1061,9 +1033,14 @@ static void build_processor_devices(Aml *sb_scope, MachineState *machine,
     pkg = pcms->apic_id_limit <= 255 ? aml_package(pcms->apic_id_limit) :
                                        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;
-        aml_append(pkg, aml_int(b));
+    for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
+        int apic_id = apic_ids->cpus[i].arch_id;
+
+        for (; apic_idx < apic_id; apic_idx++) {
+            aml_append(pkg, aml_int(0));
+        }
+        aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
+        apic_idx = apic_id + 1;
     }
     aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
     g_free(apic_ids);
@@ -1954,7 +1931,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;
@@ -2261,7 +2238,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);
@@ -2615,7 +2592,6 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
     unsigned facs, dsdt, rsdt, fadt;
-    AcpiCpuInfo cpu;
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
     AcpiMcfgInfo mcfg;
@@ -2625,7 +2601,6 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     GArray *tables_blob = tables->table_data;
     AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
 
-    acpi_get_cpu_info(&cpu);
     acpi_get_pm_info(&pm);
     acpi_get_misc_info(&misc);
     acpi_get_pci_info(&pci);
@@ -2649,7 +2624,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     /* 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 v3 8/8] 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 e66c8b7..534a855 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -361,6 +361,12 @@ build_madt(GArray *table_data, GArray *linker, PCMachineState *pcms)
         if (apic_ids->cpus[i].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