[Qemu-devel] [PATCH v4 00/13] pc: q35: x2APIC support in kvm_apic mode

[Qemu-devel] [PATCH v4 00/13] pc: q35: x2APIC support in kvm_apic mode

[Igor Mammedov]

Changes since v3:
  - fix endianness issues when filling MADT/SRAT entries (Eduardo)
  - squash "acpi: cphp: support x2APIC entry in cpu._MAT" into
    "pc: acpi: x2APIC support for MADT table and _MAT method" (Eduardo)
  - keep assert() as it doesn't affect x2APIC cpus in bochs_bios_init() (Eduardo)
  - restore kvm_has_x2apic_api() and use it to avoid side-effects
    of kvm_enable_x2apic(). x2APIC API will be enabled by iommu
    if it's present or not enabled at all. (Radim)
  - add intel iommu example to error message (Eduardo)

Changes since v2:
  - rebase on top of EIM fixes
  - drop kvm_has_x2apic_ids() and reuse kvm_enable_x2apic()
    from Radim's EIM fixes
  - fix hang on reboot in BIOS due to not updated 'etc/boot-cpus' fwcfg file
    after CPU hotplug
  - drop not used anymore pc_present_cpus_count() and incrementally count
    present VCPUs as they are added/removed at (un)plug callbacks time

Changes since v1:
  - rebase on top of 2.7
  - drop add 2.8 machine and linux headers update patches
  - drop numa related patches (will post separately as unrelated)
  - change default mc->maxcpus only for q35

Changes since RFC:
  - use new KVM_CAP_X2APIC_API to detect x2APIC IDs support
  - rebase on top of 2.7-rc1, since many deps were merged
  - fix etc/boot-cpus to account for -device provided cpus
  - include not yet merged _PXM fix as prereq
  - add 2.8 machine type and bump up maxcpus count since it

Series extends current CPU/kvm_apic/Q35 machine
code to support x2APIC and upto 288 VCPUs when QEMU
is used with KVM's lapic.

Due to FW_CFG_MAX_CPUS (which is actually apic_id_limit)
being limited to uint16_t, the max possible APIC ID is
limitted to 2^16 with this series but that should
be sufficient for bumping VCPUs number for quite a while.

Not yet fixed kernel x2APIC issues:
CPU hotplug doesn't work for CPUs where APIC ID > 254
(guest has to be rebooted to pickup hotplugged CPUs)
due to kernel_irqchip loosing directed IPIs (INIT/SIPI)
to APICs above 254 as a hotplugged CPU is in after power-on
state (i.e. not in x2APIC mode).
Radim's going to post KVM patch to fix it and on top of it
I'll post a followup QEMU sanity check patch to detect if host
supports directed IPIs to CPUs with APIC IDs above 254 in
after power-on state.


Tested with following CLI:
 QEMU -M q35 -enable-kvm \
      -device intel-iommu,intremap=on,eim=on -machine kernel_irqchip=split \
      -smp 1,sockets=9,cores=32,threads=1,maxcpus=288 \
      -device qemu64-x86_64-cpu,socket-id=8,core-id=30,thread-id=0       \
      -bios x2apic_bios.bin

v3 for reference:
[PATCH v3 00/13] pc: q35: x2APIC support in kvm_apic mode
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02865.html

git gree for testing:
https://github.com/imammedo/qemu.git x2apic_v4

To play with the feature, one would also need x2apic enabled
seabios counterpart:
https://github.com/imammedo/seabios.git x2apic_v4

Cc: kraxel@redhat.com
Cc: ehabkost@redhat.com
Cc: liuxiaojian6@huawei.com
Cc: mst@redhat.com
Cc: rkrcmar@redhat.com
Cc: peterx@redhat.com
Cc: kevin@koconnor.net
Cc: pbonzini@redhat.com
Cc: lersek@redhat.com
Cc: chao.gao@intel.com

Igor Mammedov (13):
  pc: acpi: x2APIC support for MADT table and _MAT method
  pc: acpi: x2APIC support for SRAT table
  acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254
  pc: leave max apic_id_limit only in legacy cpu hotplug code
  pc: apic_common: extend APIC ID property to 32bit
  pc: apic_common: restore APIC ID to initial ID on reset
  pc: apic_common: reset APIC ID to initial ID when switching into
    x2APIC mode
  pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode
  pc: clarify FW_CFG_MAX_CPUS usage comment
  increase MAX_CPUMASK_BITS from 255 to 288
  pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255
    CPUs
  pc: require IRQ remapping and EIM if there could be x2APIC CPUs
  pc: q35: bump max_cpus to 288

 include/hw/acpi/acpi-defs.h     |  29 +++++++++++
 include/hw/i386/apic_internal.h |   3 +-
 include/hw/i386/pc.h            |   2 +
 include/sysemu/sysemu.h         |   2 +-
 target-i386/cpu.h               |   1 +
 target-i386/kvm_i386.h          |   1 +
 hw/acpi/cpu.c                   |   5 ++
 hw/acpi/cpu_hotplug.c           |  17 ++++--
 hw/arm/virt.c                   |   2 +-
 hw/i386/acpi-build.c            | 112 ++++++++++++++++++++++++++++------------
 hw/i386/kvm/apic.c              |  12 ++++-
 hw/i386/pc.c                    |  74 +++++++++++++++-----------
 hw/i386/pc_q35.c                |   2 +
 hw/intc/apic_common.c           |  52 ++++++++++++++++++-
 hw/ppc/spapr.c                  |   2 +-
 target-i386/cpu.c               |   2 +-
 target-i386/kvm.c               |  13 +++--
 17 files changed, 250 insertions(+), 81 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 01/13] pc: acpi: x2APIC support for MADT table and _MAT method

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  - use cpu_to_le32() when assigning to uid/x2apic_id (Eduardo)
  - squash "acpi: cphp: support x2APIC entry in cpu._MAT"
    into this patch (Eduardo)
---
 include/hw/acpi/acpi-defs.h | 18 +++++++++++
 hw/acpi/cpu.c               |  5 +++
 hw/i386/acpi-build.c        | 78 +++++++++++++++++++++++++++++++--------------
 3 files changed, 77 insertions(+), 24 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 9c1b7cb..e94123c 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -343,6 +343,24 @@ struct AcpiMadtLocalNmi {
 } QEMU_PACKED;
 typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
 
+struct AcpiMadtProcessorX2Apic {
+    ACPI_SUB_HEADER_DEF
+    uint16_t reserved;
+    uint32_t x2apic_id;              /* Processor's local x2APIC ID */
+    uint32_t flags;
+    uint32_t uid;                    /* Processor object _UID */
+} QEMU_PACKED;
+typedef struct AcpiMadtProcessorX2Apic AcpiMadtProcessorX2Apic;
+
+struct AcpiMadtLocalX2ApicNmi {
+    ACPI_SUB_HEADER_DEF
+    uint16_t flags;                  /* MPS INTI flags */
+    uint32_t uid;                    /* Processor object _UID */
+    uint8_t  lint;                   /* Local APIC LINT# */
+    uint8_t  reserved[3];            /* Local APIC LINT# */
+} QEMU_PACKED;
+typedef struct AcpiMadtLocalX2ApicNmi AcpiMadtLocalX2ApicNmi;
+
 struct AcpiMadtGenericInterrupt {
     ACPI_SUB_HEADER_DEF
     uint16_t reserved;
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 902f5c9..5ac89fe 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -531,6 +531,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                 apic->flags = cpu_to_le32(1);
                 break;
             }
+            case ACPI_APIC_LOCAL_X2APIC: {
+                AcpiMadtProcessorX2Apic *apic = (void *)madt_buf->data;
+                apic->flags = cpu_to_le32(1);
+                break;
+            }
             default:
                 assert(0);
             }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e999654..385f9fc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -340,24 +340,38 @@ build_fadt(GArray *table_data, BIOSLinker *linker, AcpiPmInfo *pm,
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        CPUArchIdList *apic_ids, GArray *entry)
 {
-    int apic_id;
-    AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
-
-    apic_id = apic_ids->cpus[uid].arch_id;
-    apic->type = ACPI_APIC_PROCESSOR;
-    apic->length = sizeof(*apic);
-    apic->processor_id = uid;
-    apic->local_apic_id = apic_id;
-    if (apic_ids->cpus[uid].cpu != NULL) {
-        apic->flags = cpu_to_le32(1);
+    uint32_t apic_id = apic_ids->cpus[uid].arch_id;
+
+    /* 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.
+     */
+    if (apic_id < 255) {
+        AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
+
+        apic->type = ACPI_APIC_PROCESSOR;
+        apic->length = sizeof(*apic);
+        apic->processor_id = uid;
+        apic->local_apic_id = apic_id;
+        if (apic_ids->cpus[uid].cpu != NULL) {
+            apic->flags = cpu_to_le32(1);
+        } else {
+            apic->flags = cpu_to_le32(0);
+        }
     } 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);
+        AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic);
+
+        apic->type = ACPI_APIC_LOCAL_X2APIC;
+        apic->length = sizeof(*apic);
+        apic->uid = cpu_to_le32(uid);
+        apic->x2apic_id = cpu_to_le32(apic_id);
+        if (apic_ids->cpus[uid].cpu != NULL) {
+            apic->flags = cpu_to_le32(1);
+        } else {
+            apic->flags = cpu_to_le32(0);
+        }
     }
 }
 
@@ -369,11 +383,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     int madt_start = table_data->len;
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
     AcpiDeviceIf *adev = ACPI_DEVICE_IF(pcms->acpi_dev);
+    bool x2apic_mode = false;
 
     AcpiMultipleApicTable *madt;
     AcpiMadtIoApic *io_apic;
     AcpiMadtIntsrcovr *intsrcovr;
-    AcpiMadtLocalNmi *local_nmi;
     int i;
 
     madt = acpi_data_push(table_data, sizeof *madt);
@@ -382,6 +396,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 
     for (i = 0; i < apic_ids->len; i++) {
         adevc->madt_cpu(adev, i, apic_ids, table_data);
+        if (apic_ids->cpus[i].arch_id > 254) {
+            x2apic_mode = true;
+        }
     }
     g_free(apic_ids);
 
@@ -414,12 +431,25 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
         intsrcovr->flags  = cpu_to_le16(0xd); /* active high, level triggered */
     }
 
-    local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
-    local_nmi->type         = ACPI_APIC_LOCAL_NMI;
-    local_nmi->length       = sizeof(*local_nmi);
-    local_nmi->processor_id = 0xff; /* all processors */
-    local_nmi->flags        = cpu_to_le16(0);
-    local_nmi->lint         = 1; /* ACPI_LINT1 */
+    if (x2apic_mode) {
+        AcpiMadtLocalX2ApicNmi *local_nmi;
+
+        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
+        local_nmi->type   = ACPI_APIC_LOCAL_X2APIC_NMI;
+        local_nmi->length = sizeof(*local_nmi);
+        local_nmi->uid    = 0xFFFFFFFF; /* all processors */
+        local_nmi->flags  = cpu_to_le16(0);
+        local_nmi->lint   = 1; /* ACPI_LINT1 */
+    } else {
+        AcpiMadtLocalNmi *local_nmi;
+
+        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
+        local_nmi->type         = ACPI_APIC_LOCAL_NMI;
+        local_nmi->length       = sizeof(*local_nmi);
+        local_nmi->processor_id = 0xff; /* all processors */
+        local_nmi->flags        = cpu_to_le16(0);
+        local_nmi->lint         = 1; /* ACPI_LINT1 */
+    }
 
     build_header(linker, table_data,
                  (void *)(table_data->data + madt_start), "APIC",
-- 
2.7.4

[Qemu-devel] [PATCH v4 02/13] pc: acpi: x2APIC support for SRAT table

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  - use cpu_to_le32() when assigning to x2apic_id (Eduardo)
v3:
  - rebase on top in 2.7 + updated cpu PXM patches
---
 include/hw/acpi/acpi-defs.h | 11 +++++++++++
 hw/i386/acpi-build.c        | 34 ++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index e94123c..fa89abc 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -503,6 +503,17 @@ struct AcpiSratProcessorAffinity
 } QEMU_PACKED;
 typedef struct AcpiSratProcessorAffinity AcpiSratProcessorAffinity;
 
+struct AcpiSratProcessorX2ApicAffinity {
+    ACPI_SUB_HEADER_DEF
+    uint16_t    reserved;
+    uint32_t    proximity_domain;
+    uint32_t    x2apic_id;
+    uint32_t    flags;
+    uint32_t    clk_domain;
+    uint32_t    reserved2;
+} QEMU_PACKED;
+typedef struct AcpiSratProcessorX2ApicAffinity AcpiSratProcessorX2ApicAffinity;
+
 struct AcpiSratMemoryAffinity
 {
     ACPI_SUB_HEADER_DEF
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 385f9fc..93be96f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2421,7 +2421,6 @@ static void
 build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 {
     AcpiSystemResourceAffinityTable *srat;
-    AcpiSratProcessorAffinity *core;
     AcpiSratMemoryAffinity *numamem;
 
     int i;
@@ -2441,18 +2440,33 @@ build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
 
     for (i = 0; i < apic_ids->len; i++) {
         int j = numa_get_node_for_cpu(i);
-        int apic_id = apic_ids->cpus[i].arch_id;
+        uint32_t apic_id = apic_ids->cpus[i].arch_id;
 
-        core = acpi_data_push(table_data, sizeof *core);
-        core->type = ACPI_SRAT_PROCESSOR_APIC;
-        core->length = sizeof(*core);
-        core->local_apic_id = apic_id;
-        if (j < nb_numa_nodes) {
+        if (apic_id < 255) {
+            AcpiSratProcessorAffinity *core;
+
+            core = acpi_data_push(table_data, sizeof *core);
+            core->type = ACPI_SRAT_PROCESSOR_APIC;
+            core->length = sizeof(*core);
+            core->local_apic_id = apic_id;
+            if (j < nb_numa_nodes) {
                 core->proximity_lo = j;
+            }
+            memset(core->proximity_hi, 0, 3);
+            core->local_sapic_eid = 0;
+            core->flags = cpu_to_le32(1);
+        } else {
+            AcpiSratProcessorX2ApicAffinity *core;
+
+            core = acpi_data_push(table_data, sizeof *core);
+            core->type = ACPI_SRAT_PROCESSOR_x2APIC;
+            core->length = sizeof(*core);
+            core->x2apic_id = cpu_to_le32(apic_id);
+            if (j < nb_numa_nodes) {
+                core->proximity_domain = cpu_to_le32(j);
+            }
+            core->flags = cpu_to_le32(1);
         }
-        memset(core->proximity_hi, 0, 3);
-        core->local_sapic_eid = 0;
-        core->flags = cpu_to_le32(1);
     }
 
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 03/13] acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254

[Igor Mammedov]

Switch to modern cpu hotplug at machine startup time if
a cpu present at boot has apic-id in range unsupported
by legacy cpu hotplug interface (i.e. > 254), to avoid
killing QEMU from legacy cpu hotplug code with error:
   "acpi: invalid cpu id: #apic-id#"

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

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index e19d902..c2ab9b8 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -63,7 +63,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
 
     cpu_id = k->get_arch_id(cpu);
     if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
-        error_setg(errp, "acpi: invalid cpu id: %" PRIi64, cpu_id);
+        object_property_set_bool(g->device, false, "cpu-hotplug-legacy",
+                                 &error_abort);
         return;
     }
 
@@ -85,13 +86,14 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
 {
     CPUState *cpu;
 
-    CPU_FOREACH(cpu) {
-        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
-    }
     memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
                           gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
     memory_region_add_subregion(parent, base, &gpe_cpu->io);
     gpe_cpu->device = owner;
+
+    CPU_FOREACH(cpu) {
+        acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
+    }
 }
 
 void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
-- 
2.7.4

[Qemu-devel] [PATCH v4 04/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Igor Mammedov]

that's enough to make old code that depends on it
to prevent QEMU starting with more than 255 CPUs.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v4:
  - keep assert() as it doesn't affect x2APIC cpus (Eduardo)
---
 hw/acpi/cpu_hotplug.c | 7 ++++++-
 hw/i386/pc.c          | 6 ------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index c2ab9b8..f15a240 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "qom/cpu.h"
 #include "hw/i386/pc.h"
+#include "qemu/error-report.h"
 
 #define CPU_EJECT_METHOD "CPEJ"
 #define CPU_MAT_METHOD "CPMA"
@@ -236,7 +237,11 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *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(pcms->apic_id_limit <= ACPI_CPU_HOTPLUG_ID_LIMIT);
+    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);
+    }
 
     /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
     dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f4b0cda..6f4075f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1190,12 +1190,6 @@ void pc_cpus_init(PCMachineState *pcms)
      * 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);
-    }
-
     pcms->possible_cpus = g_malloc0(sizeof(CPUArchIdList) +
                                     sizeof(CPUArchId) * max_cpus);
     for (i = 0; i < max_cpus; i++) {
-- 
2.7.4

[Qemu-devel] [PATCH v4 05/13] pc: apic_common: extend APIC ID property to 32bit

[Igor Mammedov]

ACPI ID is 32 bit wide on CPUs with x2APIC support.
Extend 'id' property to support it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
   keep original behaviour where 'id' is readonly after
   object is realized (pbonzini)
---
 include/hw/i386/apic_internal.h |  3 ++-
 target-i386/cpu.h               |  1 +
 hw/intc/apic_common.c           | 46 ++++++++++++++++++++++++++++++++++++++++-
 target-i386/cpu.c               |  2 +-
 4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index cdd11fb..1209eb4 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -160,7 +160,8 @@ struct APICCommonState {
     MemoryRegion io_memory;
     X86CPU *cpu;
     uint32_t apicbase;
-    uint8_t id;
+    uint8_t id; /* legacy APIC ID */
+    uint32_t initial_apic_id;
     uint8_t version;
     uint8_t arb_id;
     uint8_t tpr;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index e645698..6303d65 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -325,6 +325,7 @@
 #define MSR_IA32_APICBASE               0x1b
 #define MSR_IA32_APICBASE_BSP           (1<<8)
 #define MSR_IA32_APICBASE_ENABLE        (1<<11)
+#define MSR_IA32_APICBASE_EXTD          (1 << 10)
 #define MSR_IA32_APICBASE_BASE          (0xfffffU<<12)
 #define MSR_IA32_FEATURE_CONTROL        0x0000003a
 #define MSR_TSC_ADJUST                  0x0000003b
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 8d01c9c..30f2af0 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -22,6 +22,7 @@
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "cpu.h"
+#include "qapi/visitor.h"
 #include "hw/i386/apic.h"
 #include "hw/i386/apic_internal.h"
 #include "trace.h"
@@ -428,7 +429,6 @@ static const VMStateDescription vmstate_apic_common = {
 };
 
 static Property apic_properties_common[] = {
-    DEFINE_PROP_UINT8("id", APICCommonState, id, -1),
     DEFINE_PROP_UINT8("version", APICCommonState, version, 0x14),
     DEFINE_PROP_BIT("vapic", APICCommonState, vapic_control, VAPIC_ENABLE_BIT,
                     true),
@@ -437,6 +437,49 @@ static Property apic_properties_common[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void apic_common_get_id(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(obj);
+    int64_t value;
+
+    value = s->apicbase & MSR_IA32_APICBASE_EXTD ? s->initial_apic_id : s->id;
+    visit_type_int(v, name, &value, errp);
+}
+
+static void apic_common_set_id(Object *obj, Visitor *v, const char *name,
+                               void *opaque, Error **errp)
+{
+    APICCommonState *s = APIC_COMMON(obj);
+    DeviceState *dev = DEVICE(obj);
+    Error *local_err = NULL;
+    int64_t value;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_int(v, name, &value, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    s->initial_apic_id = value;
+    s->id = (uint8_t)value;
+}
+
+static void apic_common_initfn(Object *obj)
+{
+    APICCommonState *s = APIC_COMMON(obj);
+
+    s->id = s->initial_apic_id = -1;
+    object_property_add(obj, "id", "int",
+                        apic_common_get_id,
+                        apic_common_set_id, NULL, NULL, NULL);
+}
+
 static void apic_common_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -456,6 +499,7 @@ static const TypeInfo apic_common_type = {
     .name = TYPE_APIC_COMMON,
     .parent = TYPE_DEVICE,
     .instance_size = sizeof(APICCommonState),
+    .instance_init = apic_common_initfn,
     .class_size = sizeof(APICCommonClass),
     .class_init = apic_common_class_init,
     .abstract = true,
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d95514c..7dc6f62 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2945,7 +2945,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
                               OBJECT(cpu->apic_state), &error_abort);
     object_unref(OBJECT(cpu->apic_state));
 
-    qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
+    qdev_prop_set_uint32(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
-- 
2.7.4

[Qemu-devel] [PATCH v4 06/13] pc: apic_common: restore APIC ID to initial ID on reset

[Igor Mammedov]

APIC ID should be restored to initial APIC ID
state after Reset and Power-On.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/intc/apic_common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 30f2af0..ea3c8ca 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -243,6 +243,7 @@ static void apic_reset_common(DeviceState *dev)
 
     bsp = s->apicbase & MSR_IA32_APICBASE_BSP;
     s->apicbase = APIC_DEFAULT_ADDRESS | bsp | MSR_IA32_APICBASE_ENABLE;
+    s->id = s->initial_apic_id;
 
     s->vapic_paddr = 0;
     info->vapic_base_update(s);
-- 
2.7.4

[Qemu-devel] [PATCH v4 07/13] pc: apic_common: reset APIC ID to initial ID when switching into x2APIC mode

[Igor Mammedov]

SDM: x2APIC State Transitions:
         State Changes From xAPIC Mode to x2APIC Mode
"
Any APIC ID value written to the memory-mapped
local APIC ID register is not preserved
"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/intc/apic_common.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index ea3c8ca..d78c885 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -40,6 +40,11 @@ void cpu_set_apic_base(DeviceState *dev, uint64_t val)
     if (dev) {
         APICCommonState *s = APIC_COMMON(dev);
         APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
+        /* switching to x2APIC, reset possibly modified xAPIC ID */
+        if (!(s->apicbase & MSR_IA32_APICBASE_EXTD) &&
+            (val & MSR_IA32_APICBASE_EXTD)) {
+            s->id = s->initial_apic_id;
+        }
         info->set_base(s, val);
     }
 }
-- 
2.7.4

[Qemu-devel] [PATCH v4 08/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v4:
 - restore kvm_has_x2apic_api() and use it to avoid side-effects
   of kvm_enable_x2apic(). x2APIC API will be enabled by iommu
   if it's present or not enabled at all.
v3:
 - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
---
 target-i386/kvm_i386.h |  1 +
 hw/i386/kvm/apic.c     | 12 ++++++++++--
 target-i386/kvm.c      | 13 ++++++++++---
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 5c369b1..7607929 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -44,4 +44,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
 bool kvm_enable_x2apic(void);
+bool kvm_has_x2apic_api(void);
 #endif
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index be55102..39b73e7 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -34,7 +34,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic
     int i;
 
     memset(kapic, 0, sizeof(*kapic));
-    kvm_apic_set_reg(kapic, 0x2, s->id << 24);
+    if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) {
+        kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id);
+    } else {
+        kvm_apic_set_reg(kapic, 0x2, s->id << 24);
+    }
     kvm_apic_set_reg(kapic, 0x8, s->tpr);
     kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
     kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
@@ -59,7 +63,11 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
     APICCommonState *s = APIC_COMMON(dev);
     int i, v;
 
-    s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
+    if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) {
+        assert(kvm_apic_get_reg(kapic, 0x2) == s->initial_apic_id);
+    } else {
+        s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
+    }
     s->tpr = kvm_apic_get_reg(kapic, 0x8);
     s->arb_id = kvm_apic_get_reg(kapic, 0x9);
     s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0472f45..86b41a9 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -129,9 +129,8 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags)
     return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
 }
 
-#define MEMORIZE(fn) \
+#define MEMORIZE(fn, _result) \
     ({ \
-        static typeof(fn) _result; \
         static bool _memorized; \
         \
         if (_memorized) { \
@@ -141,11 +140,19 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags)
         _result = fn; \
     })
 
+static bool has_x2apic_api;
+
+bool kvm_has_x2apic_api(void)
+{
+    return has_x2apic_api;
+}
+
 bool kvm_enable_x2apic(void)
 {
     return MEMORIZE(
              kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
-                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK));
+                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK),
+             has_x2apic_api);
 }
 
 static int kvm_get_tsc(CPUState *cs)
-- 
2.7.4

[Qemu-devel] [PATCH v4 09/13] pc: clarify FW_CFG_MAX_CPUS usage comment

[Igor Mammedov]

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6f4075f..4b97795 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -746,17 +746,15 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
 
     /* FW_CFG_MAX_CPUS is a bit confusing/problematic on x86:
      *
-     * SeaBIOS needs FW_CFG_MAX_CPUS for CPU hotplug, but the CPU hotplug
-     * QEMU<->SeaBIOS interface is not based on the "CPU index", but on the APIC
-     * ID of hotplugged CPUs[1]. This means that FW_CFG_MAX_CPUS is not the
-     * "maximum number of CPUs", but the "limit to the APIC ID values SeaBIOS
-     * may see".
+     * For machine types prior to 1.8, SeaBIOS needs FW_CFG_MAX_CPUS for
+     * building MPTable, ACPI MADT, ACPI CPU hotplug and ACPI SRAT table,
+     * that tables are based on xAPIC ID and QEMU<->SeaBIOS interface
+     * for CPU hotplug also uses APIC ID and not "CPU index".
+     * This means that FW_CFG_MAX_CPUS is not the "maximum number of CPUs",
+     * but the "limit to the APIC ID values SeaBIOS may see".
      *
-     * So, this means we must not use max_cpus, here, but the maximum possible
-     * APIC ID value, plus one.
-     *
-     * [1] The only kind of "CPU identifier" used between SeaBIOS and QEMU is
-     *     the APIC ID, not the "CPU index"
+     * So for compatibility reasons with old BIOSes we are stuck with
+     * "etc/max-cpus" actually being 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);
-- 
2.7.4

[Qemu-devel] [PATCH v4 10/13] increase MAX_CPUMASK_BITS from 255 to 288

[Igor Mammedov]

so that it would be possible to increase maxcpus limit
for x86 target. Keep spapr/virt_arm at limit they used
to have 255.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 include/sysemu/sysemu.h | 2 +-
 hw/arm/virt.c           | 2 +-
 hw/ppc/spapr.c          | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b668833..66c6f15 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -173,7 +173,7 @@ extern int mem_prealloc;
  *
  * Note that cpu->get_arch_id() may be larger than MAX_CPUMASK_BITS.
  */
-#define MAX_CPUMASK_BITS 255
+#define MAX_CPUMASK_BITS 288
 
 #define MAX_OPTION_ROMS 16
 typedef struct QEMUOptionRom {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 895446f..c3a1e92 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1494,7 +1494,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
      * it later in machvirt_init, where we have more information about the
      * configuration of the particular instance.
      */
-    mc->max_cpus = MAX_CPUMASK_BITS;
+    mc->max_cpus = 255;
     mc->has_dynamic_sysbus = true;
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ddb7438..486f57d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2438,7 +2438,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->init = ppc_spapr_init;
     mc->reset = ppc_spapr_reset;
     mc->block_default_type = IF_SCSI;
-    mc->max_cpus = MAX_CPUMASK_BITS;
+    mc->max_cpus = 255;
     mc->no_parallel = 1;
     mc->default_boot_order = "";
     mc->default_ram_size = 512 * M_BYTE;
-- 
2.7.4

[Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
to get number of CPUs present at boot. However 1 byte is
not enough to handle more than 255 CPUs.  So add a new
fw_cfg file that would allow QEMU to tell it.
For compat reasons add file only for machine types that
support more than 255 CPUs.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * make boot_cpus to PCMachineState so it could be kept uptodate
    when a cpu is added/removed and BIOS would read current value
    on system reset
---
 include/hw/i386/pc.h |  2 ++
 hw/i386/pc.c         | 36 +++++++++++++++++++++---------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b16c448..17fff80 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -37,6 +37,7 @@
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
+ * @boot_cpus_le: number of present VCPUs, referenced by 'etc/boot-cpus' fw_cfg
  */
 struct PCMachineState {
     /*< private >*/
@@ -69,6 +70,7 @@ struct PCMachineState {
     bool apic_xrupt_override;
     unsigned apic_id_limit;
     CPUArchIdList *possible_cpus;
+    uint16_t boot_cpus_le;
 
     /* NUMA information: */
     uint64_t numa_nodes;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b97795..7b57c9d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1085,17 +1085,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static int pc_present_cpus_count(PCMachineState *pcms)
-{
-    int i, boot_cpus = 0;
-    for (i = 0; i < pcms->possible_cpus->len; i++) {
-        if (pcms->possible_cpus->cpus[i].cpu) {
-            boot_cpus++;
-        }
-    }
-    return boot_cpus;
-}
-
 static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
@@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
     fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
 }
 
+static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
+{
+    rtc_set_memory(rtc, 0x5f, cpus_count - 1);
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
@@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     PCIBus *bus = pcms->bus;
 
     /* set the number of CPUs */
-    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
+    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
 
     if (bus) {
         int extra_hosts = 0;
@@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
+        MachineClass *mc = MACHINE_GET_CLASS(pcms);
+
         pc_build_smbios(pcms->fw_cfg);
         pc_build_feature_control_file(pcms);
+
+        if (mc->max_cpus > 255) {
+            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
+                            sizeof(pcms->boot_cpus_le));
+        }
     }
 }
 
@@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
         }
     }
 
+    /* increment the number of CPUs */
+    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);
     if (dev->hotplugged) {
-        /* increment the number of CPUs */
-        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
+        /* Update the number of CPUs in CMOS */
+        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
     }
 
     found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
@@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     found_cpu->cpu = NULL;
     object_unparent(OBJECT(dev));
 
-    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
+    /* decrement the number of CPUs */
+    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
+    /* Update the number of CPUs in CMOS */
+    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
  out:
     error_propagate(errp, local_err);
 }
-- 
2.7.4

[Qemu-devel] [PATCH v4 12/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs

[Igor Mammedov]

it would prevent starting guest with incorrect configs
where interrupts couldn't be delivered to CPUs with
APIC IDs > 255.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4:
 - s/254/255/ in commit message (Radim)
 - add intel iommu example to error message (Eduardo)
---
 hw/i386/pc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7b57c9d..1218b69 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -68,6 +68,7 @@
 #include "qapi-visit.h"
 #include "qom/cpu.h"
 #include "hw/nmi.h"
+#include "hw/i386/intel_iommu.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -1265,6 +1266,19 @@ void pc_machine_done(Notifier *notifier, void *data)
                             sizeof(pcms->boot_cpus_le));
         }
     }
+
+    if (pcms->apic_id_limit > 255) {
+        IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
+
+        if (!iommu || !iommu->x86_iommu.intr_supported ||
+            iommu->intr_eim != ON_OFF_AUTO_ON) {
+            error_report("current -smp configuration requires "
+                         "Extended Interrupt Mode enabled. "
+                         "You can add an IOMMU using: "
+                         "-device intel-iommu,intremap=on,eim=on");
+            exit(EXIT_FAILURE);
+        }
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.7.4

[Qemu-devel] [PATCH v4 13/13] pc: q35: bump max_cpus to 288

[Igor Mammedov]

along with it for machine versions 2.7 and older keep
it at 255.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
v2:
 - make 288 cpus available only since q35-2.8 machine type
---
 hw/i386/pc_q35.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0b214f2..b40d19e 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -291,6 +291,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_display = "std";
     m->no_floppy = 1;
     m->has_dynamic_sysbus = true;
+    m->max_cpus = 288;
 }
 
 static void pc_q35_2_8_machine_options(MachineClass *m)
@@ -306,6 +307,7 @@ static void pc_q35_2_7_machine_options(MachineClass *m)
 {
     pc_q35_2_8_machine_options(m);
     m->alias = NULL;
+    m->max_cpus = 255;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 01/13] pc: acpi: x2APIC support for MADT table and _MAT method

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:31PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>   - use cpu_to_le32() when assigning to uid/x2apic_id (Eduardo)
>   - squash "acpi: cphp: support x2APIC entry in cpu._MAT"
>     into this patch (Eduardo)

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 02/13] pc: acpi: x2APIC support for SRAT table

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:32PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>   - use cpu_to_le32() when assigning to x2apic_id (Eduardo)
> v3:
>   - rebase on top in 2.7 + updated cpu PXM patches

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 03/13] acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:33PM +0200, Igor Mammedov wrote:
> Switch to modern cpu hotplug at machine startup time if
> a cpu present at boot has apic-id in range unsupported
> by legacy cpu hotplug interface (i.e. > 254), to avoid
> killing QEMU from legacy cpu hotplug code with error:
>    "acpi: invalid cpu id: #apic-id#"
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 05/13] pc: apic_common: extend APIC ID property to 32bit

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:35PM +0200, Igor Mammedov wrote:
> ACPI ID is 32 bit wide on CPUs with x2APIC support.
> Extend 'id' property to support it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 06/13] pc: apic_common: restore APIC ID to initial ID on reset

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:36PM +0200, Igor Mammedov wrote:
> APIC ID should be restored to initial APIC ID
> state after Reset and Power-On.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 07/13] pc: apic_common: reset APIC ID to initial ID when switching into x2APIC mode

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:37PM +0200, Igor Mammedov wrote:
> SDM: x2APIC State Transitions:
>          State Changes From xAPIC Mode to x2APIC Mode
> "
> Any APIC ID value written to the memory-mapped
> local APIC ID register is not preserved
> "
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 09/13] pc: clarify FW_CFG_MAX_CPUS usage comment

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:39PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:
> Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> to get number of CPUs present at boot. However 1 byte is
> not enough to handle more than 255 CPUs.  So add a new
> fw_cfg file that would allow QEMU to tell it.
> For compat reasons add file only for machine types that
> support more than 255 CPUs.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[...]
>  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
>                            Error **errp)
>  {
> @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
>      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
>  }
>  
> +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> +{
> +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);

If we have more than 255 CPUs, shouldn't we at least tell the old
BIOS that we have 255, instead of silently truncating bits?

> +}
> +
>  static
>  void pc_machine_done(Notifier *notifier, void *data)
>  {
> @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
>      PCIBus *bus = pcms->bus;
>  
>      /* set the number of CPUs */
> -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>  
>      if (bus) {
>          int extra_hosts = 0;
> @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
>  
>      acpi_setup();
>      if (pcms->fw_cfg) {
> +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> +
>          pc_build_smbios(pcms->fw_cfg);
>          pc_build_feature_control_file(pcms);
> +
> +        if (mc->max_cpus > 255) {
> +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> +                            sizeof(pcms->boot_cpus_le));
> +        }
>      }
>  }
>  
> @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>          }
>      }
>  
> +    /* increment the number of CPUs */
> +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);

Is this really safe? What if the guest is in the middle of a
etc/boot-cpus read?

>      if (dev->hotplugged) {
> -        /* increment the number of CPUs */
> -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> +        /* Update the number of CPUs in CMOS */
> +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>      }
>  
>      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
>      found_cpu->cpu = NULL;
>      object_unparent(OBJECT(dev));
>  
> -    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
> +    /* decrement the number of CPUs */
> +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
> +    /* Update the number of CPUs in CMOS */
> +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
>   out:
>      error_propagate(errp, local_err);
>  }
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 10/13] increase MAX_CPUMASK_BITS from 255 to 288

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:40PM +0200, Igor Mammedov wrote:
> so that it would be possible to increase maxcpus limit
> for x86 target. Keep spapr/virt_arm at limit they used
> to have 255.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 12/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:42PM +0200, Igor Mammedov wrote:
> it would prevent starting guest with incorrect configs
> where interrupts couldn't be delivered to CPUs with
> APIC IDs > 255.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 13/13] pc: q35: bump max_cpus to 288

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 02:05:43PM +0200, Igor Mammedov wrote:
> along with it for machine versions 2.7 and older keep
> it at 255.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v2:
>  - make 288 cpus available only since q35-2.8 machine type

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 00/13] pc: q35: x2APIC support in kvm_apic mode

[Daniel P. Berrange]

On Wed, Oct 19, 2016 at 02:05:30PM +0200, Igor Mammedov wrote:
> 
> Changes since v3:
>   - fix endianness issues when filling MADT/SRAT entries (Eduardo)
>   - squash "acpi: cphp: support x2APIC entry in cpu._MAT" into
>     "pc: acpi: x2APIC support for MADT table and _MAT method" (Eduardo)
>   - keep assert() as it doesn't affect x2APIC cpus in bochs_bios_init() (Eduardo)
>   - restore kvm_has_x2apic_api() and use it to avoid side-effects
>     of kvm_enable_x2apic(). x2APIC API will be enabled by iommu
>     if it's present or not enabled at all. (Radim)
>   - add intel iommu example to error message (Eduardo)
> 
> Changes since v2:
>   - rebase on top of EIM fixes
>   - drop kvm_has_x2apic_ids() and reuse kvm_enable_x2apic()
>     from Radim's EIM fixes
>   - fix hang on reboot in BIOS due to not updated 'etc/boot-cpus' fwcfg file
>     after CPU hotplug
>   - drop not used anymore pc_present_cpus_count() and incrementally count
>     present VCPUs as they are added/removed at (un)plug callbacks time
> 
> Changes since v1:
>   - rebase on top of 2.7
>   - drop add 2.8 machine and linux headers update patches
>   - drop numa related patches (will post separately as unrelated)
>   - change default mc->maxcpus only for q35
> 
> Changes since RFC:
>   - use new KVM_CAP_X2APIC_API to detect x2APIC IDs support
>   - rebase on top of 2.7-rc1, since many deps were merged
>   - fix etc/boot-cpus to account for -device provided cpus
>   - include not yet merged _PXM fix as prereq
>   - add 2.8 machine type and bump up maxcpus count since it
> 
> Series extends current CPU/kvm_apic/Q35 machine
> code to support x2APIC and upto 288 VCPUs when QEMU
> is used with KVM's lapic.
> 
> Due to FW_CFG_MAX_CPUS (which is actually apic_id_limit)
> being limited to uint16_t, the max possible APIC ID is
> limitted to 2^16 with this series but that should
> be sufficient for bumping VCPUs number for quite a while.

Not a problem, just curiosity - 2^16 gives us 65536, so where
is the 288 limit coming from ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

On Wed, 19 Oct 2016 11:15:46 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:
> > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > to get number of CPUs present at boot. However 1 byte is
> > not enough to handle more than 255 CPUs.  So add a new
> > fw_cfg file that would allow QEMU to tell it.
> > For compat reasons add file only for machine types that
> > support more than 255 CPUs.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> [...]
> >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> >                            Error **errp)
> >  {
> > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> >  }
> >  
> > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > +{
> > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);  
> 
> If we have more than 255 CPUs, shouldn't we at least tell the old
> BIOS that we have 255, instead of silently truncating bits?
It won't do any good to BIOS as it would hang in AP wakeup due to
 (expected != woken up) condition.

> 
> > +}
> > +
> >  static
> >  void pc_machine_done(Notifier *notifier, void *data)
> >  {
> > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> >      PCIBus *bus = pcms->bus;
> >  
> >      /* set the number of CPUs */
> > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> >  
> >      if (bus) {
> >          int extra_hosts = 0;
> > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> >  
> >      acpi_setup();
> >      if (pcms->fw_cfg) {
> > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > +
> >          pc_build_smbios(pcms->fw_cfg);
> >          pc_build_feature_control_file(pcms);
> > +
> > +        if (mc->max_cpus > 255) {
> > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > +                            sizeof(pcms->boot_cpus_le));
> > +        }
> >      }
> >  }
> >  
> > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> >          }
> >      }
> >  
> > +    /* increment the number of CPUs */
> > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);  
> 
> Is this really safe? What if the guest is in the middle of a
> etc/boot-cpus read?
It's safe for boot CPUs but
it's not safe to hotplug cpus CPU during BIOS boot at all
as number of CPUs read from boot_cpus might not match number
of CPUs that received INIT/SIPI wakeup.
This problem is ignored for now, I've dropped related SeaBIOS patch
by Kevin's request and would explore it some more to avoid race there.

Anyways,
Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?

> 
> >      if (dev->hotplugged) {
> > -        /* increment the number of CPUs */
> > -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> > +        /* Update the number of CPUs in CMOS */
> > +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> >      }
> >  
> >      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> > @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
> >      found_cpu->cpu = NULL;
> >      object_unparent(OBJECT(dev));
> >  
> > -    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
> > +    /* decrement the number of CPUs */
> > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
> > +    /* Update the number of CPUs in CMOS */
> > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> >   out:
> >      error_propagate(errp, local_err);
> >  }
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH v4 00/13] pc: q35: x2APIC support in kvm_apic mode

[Igor Mammedov]

On Wed, 19 Oct 2016 14:29:22 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Oct 19, 2016 at 02:05:30PM +0200, Igor Mammedov wrote:
> > 
> > Changes since v3:
> >   - fix endianness issues when filling MADT/SRAT entries (Eduardo)
> >   - squash "acpi: cphp: support x2APIC entry in cpu._MAT" into
> >     "pc: acpi: x2APIC support for MADT table and _MAT method" (Eduardo)
> >   - keep assert() as it doesn't affect x2APIC cpus in bochs_bios_init() (Eduardo)
> >   - restore kvm_has_x2apic_api() and use it to avoid side-effects
> >     of kvm_enable_x2apic(). x2APIC API will be enabled by iommu
> >     if it's present or not enabled at all. (Radim)
> >   - add intel iommu example to error message (Eduardo)
> > 
> > Changes since v2:
> >   - rebase on top of EIM fixes
> >   - drop kvm_has_x2apic_ids() and reuse kvm_enable_x2apic()
> >     from Radim's EIM fixes
> >   - fix hang on reboot in BIOS due to not updated 'etc/boot-cpus' fwcfg file
> >     after CPU hotplug
> >   - drop not used anymore pc_present_cpus_count() and incrementally count
> >     present VCPUs as they are added/removed at (un)plug callbacks time
> > 
> > Changes since v1:
> >   - rebase on top of 2.7
> >   - drop add 2.8 machine and linux headers update patches
> >   - drop numa related patches (will post separately as unrelated)
> >   - change default mc->maxcpus only for q35
> > 
> > Changes since RFC:
> >   - use new KVM_CAP_X2APIC_API to detect x2APIC IDs support
> >   - rebase on top of 2.7-rc1, since many deps were merged
> >   - fix etc/boot-cpus to account for -device provided cpus
> >   - include not yet merged _PXM fix as prereq
> >   - add 2.8 machine type and bump up maxcpus count since it
> > 
> > Series extends current CPU/kvm_apic/Q35 machine
> > code to support x2APIC and upto 288 VCPUs when QEMU
> > is used with KVM's lapic.
> > 
> > Due to FW_CFG_MAX_CPUS (which is actually apic_id_limit)
> > being limited to uint16_t, the max possible APIC ID is
> > limitted to 2^16 with this series but that should
> > be sufficient for bumping VCPUs number for quite a while.  
> 
> Not a problem, just curiosity - 2^16 gives us 65536, so where
> is the 288 limit coming from ?
It comes from KVM but it basically limited to HW we can test it on
(even though it might need a little bit of extra work to handle
large number of CPUs).

> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:
> On Wed, 19 Oct 2016 11:15:46 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:
> > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > to get number of CPUs present at boot. However 1 byte is
> > > not enough to handle more than 255 CPUs.  So add a new
> > > fw_cfg file that would allow QEMU to tell it.
> > > For compat reasons add file only for machine types that
> > > support more than 255 CPUs.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > [...]
> > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > >                            Error **errp)
> > >  {
> > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > >  }
> > >  
> > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > +{
> > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);  
> > 
> > If we have more than 255 CPUs, shouldn't we at least tell the old
> > BIOS that we have 255, instead of silently truncating bits?
> It won't do any good to BIOS as it would hang in AP wakeup due to
>  (expected != woken up) condition.

Even in this case, truncating bits makes it a bit unpredictable:
having 257 CPUs would set RTC memory to 0, BIOS will believe it
is a UP system.

> 
> > 
> > > +}
> > > +
> > >  static
> > >  void pc_machine_done(Notifier *notifier, void *data)
> > >  {
> > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > >      PCIBus *bus = pcms->bus;
> > >  
> > >      /* set the number of CPUs */
> > > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > >  
> > >      if (bus) {
> > >          int extra_hosts = 0;
> > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> > >  
> > >      acpi_setup();
> > >      if (pcms->fw_cfg) {
> > > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > > +
> > >          pc_build_smbios(pcms->fw_cfg);
> > >          pc_build_feature_control_file(pcms);
> > > +
> > > +        if (mc->max_cpus > 255) {
> > > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > > +                            sizeof(pcms->boot_cpus_le));
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> > >          }
> > >      }
> > >  
> > > +    /* increment the number of CPUs */
> > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);  
> > 
> > Is this really safe? What if the guest is in the middle of a
> > etc/boot-cpus read?
> It's safe for boot CPUs but
> it's not safe to hotplug cpus CPU during BIOS boot at all
> as number of CPUs read from boot_cpus might not match number
> of CPUs that received INIT/SIPI wakeup.
> This problem is ignored for now, I've dropped related SeaBIOS patch
> by Kevin's request and would explore it some more to avoid race there.
> 
> Anyways,
> Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?

No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
could be updating the data between two reads. I don't think we
want to design a fw_cfg guest<->host synchronization mechanism.

I believe the solution is to not change it at all after booting
the guest. I suggest initializing/reinitializing fw_cfg data only
on reset.

After that, we could block or delay CPU hotplug until we know the
guest will be able to handle it. Do we have anything that
prevents or delays hotplug until we know the guest is able to
handle the hotplug events?

> 
> > 
> > >      if (dev->hotplugged) {
> > > -        /* increment the number of CPUs */
> > > -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> > > +        /* Update the number of CPUs in CMOS */
> > > +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > >      }
> > >  
> > >      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> > > @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
> > >      found_cpu->cpu = NULL;
> > >      object_unparent(OBJECT(dev));
> > >  
> > > -    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
> > > +    /* decrement the number of CPUs */
> > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
> > > +    /* Update the number of CPUs in CMOS */
> > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > >   out:
> > >      error_propagate(errp, local_err);
> > >  }
> > > -- 
> > > 2.7.4
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

On Wed, 19 Oct 2016 16:29:29 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:
> > On Wed, 19 Oct 2016 11:15:46 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:  
> > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > to get number of CPUs present at boot. However 1 byte is
> > > > not enough to handle more than 255 CPUs.  So add a new
> > > > fw_cfg file that would allow QEMU to tell it.
> > > > For compat reasons add file only for machine types that
> > > > support more than 255 CPUs.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > [...]  
> > > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > > >                            Error **errp)
> > > >  {
> > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > > >  }
> > > >  
> > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > > +{
> > > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);    
> > > 
> > > If we have more than 255 CPUs, shouldn't we at least tell the old
> > > BIOS that we have 255, instead of silently truncating bits?  
> > It won't do any good to BIOS as it would hang in AP wakeup due to
> >  (expected != woken up) condition.  
> 
> Even in this case, truncating bits makes it a bit unpredictable:
> having 257 CPUs would set RTC memory to 0, BIOS will believe it
> is a UP system.
and it will do AP wakeup regardless, where old BIOS will hang
due to unexpectedly woken-up CPUs regardless of value in cmos.

Anyways I don't care so if you'd really prefer to have cmos
set to some static value if cpus count more than 256, just
tell me what value and what justification I shall put in
comment so we won't wonder later why it's there.

> > >   
> > > > +}
> > > > +
> > > >  static
> > > >  void pc_machine_done(Notifier *notifier, void *data)
> > > >  {
> > > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > >      PCIBus *bus = pcms->bus;
> > > >  
> > > >      /* set the number of CPUs */
> > > > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > >  
> > > >      if (bus) {
> > > >          int extra_hosts = 0;
> > > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > >  
> > > >      acpi_setup();
> > > >      if (pcms->fw_cfg) {
> > > > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > > > +
> > > >          pc_build_smbios(pcms->fw_cfg);
> > > >          pc_build_feature_control_file(pcms);
> > > > +
> > > > +        if (mc->max_cpus > 255) {
> > > > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > > > +                            sizeof(pcms->boot_cpus_le));
> > > > +        }
> > > >      }
> > > >  }
> > > >  
> > > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> > > >          }
> > > >      }
> > > >  
> > > > +    /* increment the number of CPUs */
> > > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);    
> > > 
> > > Is this really safe? What if the guest is in the middle of a
> > > etc/boot-cpus read?  
> > It's safe for boot CPUs but
> > it's not safe to hotplug cpus CPU during BIOS boot at all
> > as number of CPUs read from boot_cpus might not match number
> > of CPUs that received INIT/SIPI wakeup.
> > This problem is ignored for now, I've dropped related SeaBIOS patch
> > by Kevin's request and would explore it some more to avoid race there.
> > 
> > Anyways,
> > Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?  
> 
> No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
> could be updating the data between two reads. I don't think we
> want to design a fw_cfg guest<->host synchronization mechanism.
> 
> I believe the solution is to not change it at all after booting
> the guest. I suggest initializing/reinitializing fw_cfg data only
> on reset.
I've checked it once more and value read by BIOS atomically
as both QEMU and SeaBIOS use dma interface to transfer data.
BIOS transfers control to QEMU once via

 outl(PORT_QEMU_CFG_DMA_ADDR_LOW)

and after that access to pcms->boot_cpus_le is protected by BQL.
So there is no need to invent some sort of synchronization
or limit update to fixed points (machine_done/reset).

> After that, we could block or delay CPU hotplug until we know the
> guest will be able to handle it. Do we have anything that
> prevents or delays hotplug until we know the guest is able to
> handle the hotplug events?
Not that I know of,
and I'd like to fix BIOS side to handle AP wakeup gracefully
even if cpus are hotplugged while BIOS is running instead of
putting band-aids to workaround the issue.


> >   
> > >   
> > > >      if (dev->hotplugged) {
> > > > -        /* increment the number of CPUs */
> > > > -        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
> > > > +        /* Update the number of CPUs in CMOS */
> > > > +        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > >      }
> > > >  
> > > >      found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
> > > > @@ -1842,7 +1845,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
> > > >      found_cpu->cpu = NULL;
> > > >      object_unparent(OBJECT(dev));
> > > >  
> > > > -    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
> > > > +    /* decrement the number of CPUs */
> > > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
> > > > +    /* Update the number of CPUs in CMOS */
> > > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > >   out:
> > > >      error_propagate(errp, local_err);
> > > >  }
> > > > -- 
> > > > 2.7.4
> > > >     
> > >   
> >   
> 

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Thu, Oct 20, 2016 at 01:27:34PM +0200, Igor Mammedov wrote:
> On Wed, 19 Oct 2016 16:29:29 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:
> > > On Wed, 19 Oct 2016 11:15:46 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:  
> > > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > > to get number of CPUs present at boot. However 1 byte is
> > > > > not enough to handle more than 255 CPUs.  So add a new
> > > > > fw_cfg file that would allow QEMU to tell it.
> > > > > For compat reasons add file only for machine types that
> > > > > support more than 255 CPUs.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > > [...]  
> > > > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > > > >                            Error **errp)
> > > > >  {
> > > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > > > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > > > >  }
> > > > >  
> > > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > > > +{
> > > > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);    
> > > > 
> > > > If we have more than 255 CPUs, shouldn't we at least tell the old
> > > > BIOS that we have 255, instead of silently truncating bits?  
> > > It won't do any good to BIOS as it would hang in AP wakeup due to
> > >  (expected != woken up) condition.  
> > 
> > Even in this case, truncating bits makes it a bit unpredictable:
> > having 257 CPUs would set RTC memory to 0, BIOS will believe it
> > is a UP system.
> and it will do AP wakeup regardless, where old BIOS will hang
> due to unexpectedly woken-up CPUs regardless of value in cmos.

0 (1 CPU) seems to be the only value that will not hang (at least
it won't hang at the same point). If cmos_cpu_count is 1, SeaBIOS
won't even wait for the other CPUs to wake up.

> 
> Anyways I don't care so if you'd really prefer to have cmos
> set to some static value if cpus count more than 256, just
> tell me what value and what justification I shall put in
> comment so we won't wonder later why it's there.

I would set it to 254. Any value except 0 (1 CPU) would likely
hang. The most likely candidates to me are:

* 0 (1 CPU): will hint to the BIOS that it shouldn't try
  to initialize SMP
* 255 (256 CPUs): the largest possible value
* 254 (255 CPUs): the largest possible value that won't overflow
  in case the BIOS uses 8-bit for the CPU count

I was going to suggest 254 (255 CPUs), but maybe 0 (1 CPU) would
even allow the system to boot in UP mode with today's SeaBIOS?
(Why does SeaBIOS need to start the other CPUs, anyway, except
for building the APIC ID list for the ACPI tables?)

254 and 255 could also confuse the BIOS because it will look at
8-bit APIC IDs in CPUID[1].EBX.

That said, setting it to 0 (1 CPU) sounds like the best option.
But I would be OK with any other value as long as it is
predictable.

> 
> > > >   
> > > > > +}
> > > > > +
> > > > >  static
> > > > >  void pc_machine_done(Notifier *notifier, void *data)
> > > > >  {
> > > > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > >      PCIBus *bus = pcms->bus;
> > > > >  
> > > > >      /* set the number of CPUs */
> > > > > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > > > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > > >  
> > > > >      if (bus) {
> > > > >          int extra_hosts = 0;
> > > > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > >  
> > > > >      acpi_setup();
> > > > >      if (pcms->fw_cfg) {
> > > > > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > > > > +
> > > > >          pc_build_smbios(pcms->fw_cfg);
> > > > >          pc_build_feature_control_file(pcms);
> > > > > +
> > > > > +        if (mc->max_cpus > 255) {
> > > > > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > > > > +                            sizeof(pcms->boot_cpus_le));
> > > > > +        }
> > > > >      }
> > > > >  }
> > > > >  
> > > > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> > > > >          }
> > > > >      }
> > > > >  
> > > > > +    /* increment the number of CPUs */
> > > > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);    
> > > > 
> > > > Is this really safe? What if the guest is in the middle of a
> > > > etc/boot-cpus read?  
> > > It's safe for boot CPUs but
> > > it's not safe to hotplug cpus CPU during BIOS boot at all
> > > as number of CPUs read from boot_cpus might not match number
> > > of CPUs that received INIT/SIPI wakeup.
> > > This problem is ignored for now, I've dropped related SeaBIOS patch
> > > by Kevin's request and would explore it some more to avoid race there.
> > > 
> > > Anyways,
> > > Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?  
> > 
> > No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
> > could be updating the data between two reads. I don't think we
> > want to design a fw_cfg guest<->host synchronization mechanism.
> > 
> > I believe the solution is to not change it at all after booting
> > the guest. I suggest initializing/reinitializing fw_cfg data only
> > on reset.
> I've checked it once more and value read by BIOS atomically
> as both QEMU and SeaBIOS use dma interface to transfer data.
> BIOS transfers control to QEMU once via
> 
>  outl(PORT_QEMU_CFG_DMA_ADDR_LOW)
> 
> and after that access to pcms->boot_cpus_le is protected by BQL.
> So there is no need to invent some sort of synchronization
> or limit update to fixed points (machine_done/reset).

You're right. I was assuming the worst case and looking at the
non-DMA path.

In this case, I believe we're safe except when running old BIOS
that doesn't support fw_cfg DMA.

> 
> > After that, we could block or delay CPU hotplug until we know the
> > guest will be able to handle it. Do we have anything that
> > prevents or delays hotplug until we know the guest is able to
> > handle the hotplug events?
> Not that I know of,
> and I'd like to fix BIOS side to handle AP wakeup gracefully
> even if cpus are hotplugged while BIOS is running instead of
> putting band-aids to workaround the issue.
> 

No problem to me, as the DMA path seems safe.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

On Thu, 20 Oct 2016 10:27:33 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 20, 2016 at 01:27:34PM +0200, Igor Mammedov wrote:
> > On Wed, 19 Oct 2016 16:29:29 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:  
> > > > On Wed, 19 Oct 2016 11:15:46 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:    
> > > > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > > > to get number of CPUs present at boot. However 1 byte is
> > > > > > not enough to handle more than 255 CPUs.  So add a new
> > > > > > fw_cfg file that would allow QEMU to tell it.
> > > > > > For compat reasons add file only for machine types that
> > > > > > support more than 255 CPUs.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>      
> > > > > [...]    
> > > > > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > > > > >                            Error **errp)
> > > > > >  {
> > > > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > > > > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > > > > >  }
> > > > > >  
> > > > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > > > > +{
> > > > > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);      
> > > > > 
> > > > > If we have more than 255 CPUs, shouldn't we at least tell the old
> > > > > BIOS that we have 255, instead of silently truncating bits?    
> > > > It won't do any good to BIOS as it would hang in AP wakeup due to
> > > >  (expected != woken up) condition.    
> > > 
> > > Even in this case, truncating bits makes it a bit unpredictable:
> > > having 257 CPUs would set RTC memory to 0, BIOS will believe it
> > > is a UP system.  
> > and it will do AP wakeup regardless, where old BIOS will hang
> > due to unexpectedly woken-up CPUs regardless of value in cmos.  
> 
> 0 (1 CPU) seems to be the only value that will not hang (at least
> it won't hang at the same point). If cmos_cpu_count is 1, SeaBIOS
> won't even wait for the other CPUs to wake up.
I don't see it in code

here is current/old seabios smp init flow:

    if (BSP HAS APIC DISABLED) {                             
        // No apic - only the main cpu is present.                               
        dprintf(1, "No apic - only the main cpu is present.\n");                 
        CountCPUs= 1;                                                            
        return;                                                                  
    }     

    AP WAKEUP regardless of CMOS_BIOS_SMP_COUNT value

    u8 cmos_smp_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;                       
    while (cmos_smp_count != CountCPUs) // we are doomed here due to AP race and
                                        // mostly hang here regardless of cmos_smp_count value
                                        // as CountCPUs could be anything at this point and counting up
                                        

and it's been pretty much the same logic throughout history,
that's why it doesn't matter  if rtc_read(CMOS_BIOS_SMP_COUNT) is 0 or something else.

SMP support has been introduced by:
(e97ca7bd1 Forward port bochs smp changes; rename smpdetect.c to smp.c.)


[...]
> That said, setting it to 0 (1 CPU) sounds like the best option.
> But I would be OK with any other value as long as it is
> predictable.
So counting above SeaBIOS behavior shall I still set cmos to 0?

> 
> >   
> > > > >     
> > > > > > +}
> > > > > > +
> > > > > >  static
> > > > > >  void pc_machine_done(Notifier *notifier, void *data)
> > > > > >  {
> > > > > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > > >      PCIBus *bus = pcms->bus;
> > > > > >  
> > > > > >      /* set the number of CPUs */
> > > > > > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > > > > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > > > >  
> > > > > >      if (bus) {
> > > > > >          int extra_hosts = 0;
> > > > > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > > >  
> > > > > >      acpi_setup();
> > > > > >      if (pcms->fw_cfg) {
> > > > > > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > > > > > +
> > > > > >          pc_build_smbios(pcms->fw_cfg);
> > > > > >          pc_build_feature_control_file(pcms);
> > > > > > +
> > > > > > +        if (mc->max_cpus > 255) {
> > > > > > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > > > > > +                            sizeof(pcms->boot_cpus_le));
> > > > > > +        }
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> > > > > >          }
> > > > > >      }
> > > > > >  
> > > > > > +    /* increment the number of CPUs */
> > > > > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);      
> > > > > 
> > > > > Is this really safe? What if the guest is in the middle of a
> > > > > etc/boot-cpus read?    
> > > > It's safe for boot CPUs but
> > > > it's not safe to hotplug cpus CPU during BIOS boot at all
> > > > as number of CPUs read from boot_cpus might not match number
> > > > of CPUs that received INIT/SIPI wakeup.
> > > > This problem is ignored for now, I've dropped related SeaBIOS patch
> > > > by Kevin's request and would explore it some more to avoid race there.
> > > > 
> > > > Anyways,
> > > > Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?    
> > > 
> > > No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
> > > could be updating the data between two reads. I don't think we
> > > want to design a fw_cfg guest<->host synchronization mechanism.
> > > 
> > > I believe the solution is to not change it at all after booting
> > > the guest. I suggest initializing/reinitializing fw_cfg data only
> > > on reset.  
> > I've checked it once more and value read by BIOS atomically
> > as both QEMU and SeaBIOS use dma interface to transfer data.
> > BIOS transfers control to QEMU once via
> > 
> >  outl(PORT_QEMU_CFG_DMA_ADDR_LOW)
> > 
> > and after that access to pcms->boot_cpus_le is protected by BQL.
> > So there is no need to invent some sort of synchronization
> > or limit update to fixed points (machine_done/reset).  
> 
> You're right. I was assuming the worst case and looking at the
> non-DMA path.
> 
> In this case, I believe we're safe except when running old BIOS
> that doesn't support fw_cfg DMA.
old bios won't read this file (as it doesn't know about it)


> 
> >   
> > > After that, we could block or delay CPU hotplug until we know the
> > > guest will be able to handle it. Do we have anything that
> > > prevents or delays hotplug until we know the guest is able to
> > > handle the hotplug events?  
> > Not that I know of,
> > and I'd like to fix BIOS side to handle AP wakeup gracefully
> > even if cpus are hotplugged while BIOS is running instead of
> > putting band-aids to workaround the issue.
> >   
> 
> No problem to me, as the DMA path seems safe.
> 

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Thu, Oct 20, 2016 at 03:27:50PM +0200, Igor Mammedov wrote:
> On Thu, 20 Oct 2016 10:27:33 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 20, 2016 at 01:27:34PM +0200, Igor Mammedov wrote:
> > > On Wed, 19 Oct 2016 16:29:29 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:  
> > > > > On Wed, 19 Oct 2016 11:15:46 -0200
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >     
> > > > > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:    
> > > > > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > > > > to get number of CPUs present at boot. However 1 byte is
> > > > > > > not enough to handle more than 255 CPUs.  So add a new
> > > > > > > fw_cfg file that would allow QEMU to tell it.
> > > > > > > For compat reasons add file only for machine types that
> > > > > > > support more than 255 CPUs.
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>      
> > > > > > [...]    
> > > > > > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > > > > > >                            Error **errp)
> > > > > > >  {
> > > > > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > > > > > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > > > > > >  }
> > > > > > >  
> > > > > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > > > > > +{
> > > > > > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);      
> > > > > > 
> > > > > > If we have more than 255 CPUs, shouldn't we at least tell the old
> > > > > > BIOS that we have 255, instead of silently truncating bits?    
> > > > > It won't do any good to BIOS as it would hang in AP wakeup due to
> > > > >  (expected != woken up) condition.    
> > > > 
> > > > Even in this case, truncating bits makes it a bit unpredictable:
> > > > having 257 CPUs would set RTC memory to 0, BIOS will believe it
> > > > is a UP system.  
> > > and it will do AP wakeup regardless, where old BIOS will hang
> > > due to unexpectedly woken-up CPUs regardless of value in cmos.  
> > 
> > 0 (1 CPU) seems to be the only value that will not hang (at least
> > it won't hang at the same point). If cmos_cpu_count is 1, SeaBIOS
> > won't even wait for the other CPUs to wake up.
> I don't see it in code
> 
> here is current/old seabios smp init flow:
> 
>     if (BSP HAS APIC DISABLED) {                             
>         // No apic - only the main cpu is present.                               
>         dprintf(1, "No apic - only the main cpu is present.\n");                 
>         CountCPUs= 1;                                                            
>         return;                                                                  
>     }     
> 

We also have these lines:

    u8 apic_id = ebx>>24;
    FoundAPICIDs[apic_id/32] |= (1 << (apic_id % 32));
    CountCPUs = 1;

>     AP WAKEUP regardless of CMOS_BIOS_SMP_COUNT value
> 
>     u8 cmos_smp_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;                       
>     while (cmos_smp_count != CountCPUs) // we are doomed here due to AP race and
>                                         // mostly hang here regardless of cmos_smp_count value
>                                         // as CountCPUs could be anything at this point and counting up

CountCPUs will be always 1 on the first (cmos_smp_count !=
CountCPUs) check, because handle_smp() (which change CountCPUs)
is protected by SMPLock.

>                                         
> 
> and it's been pretty much the same logic throughout history,
> that's why it doesn't matter  if rtc_read(CMOS_BIOS_SMP_COUNT) is 0 or something else.
> 
> SMP support has been introduced by:
> (e97ca7bd1 Forward port bochs smp changes; rename smpdetect.c to smp.c.)
> 
> 
> [...]
> > That said, setting it to 0 (1 CPU) sounds like the best option.
> > But I would be OK with any other value as long as it is
> > predictable.
> So counting above SeaBIOS behavior shall I still set cmos to 0?

In the case we get unpredictable behavior from SeaBIOS for any
value (I will make some tests to confirm that), setting it to 0
still seems more likely to avoid weird things from happening with
BIOSes or OSes we don't control.

> 
> > 
> > >   
> > > > > >     
> > > > > > > +}
> > > > > > > +
> > > > > > >  static
> > > > > > >  void pc_machine_done(Notifier *notifier, void *data)
> > > > > > >  {
> > > > > > > @@ -1240,7 +1234,7 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > > > >      PCIBus *bus = pcms->bus;
> > > > > > >  
> > > > > > >      /* set the number of CPUs */
> > > > > > > -    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
> > > > > > > +    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
> > > > > > >  
> > > > > > >      if (bus) {
> > > > > > >          int extra_hosts = 0;
> > > > > > > @@ -1261,8 +1255,15 @@ void pc_machine_done(Notifier *notifier, void *data)
> > > > > > >  
> > > > > > >      acpi_setup();
> > > > > > >      if (pcms->fw_cfg) {
> > > > > > > +        MachineClass *mc = MACHINE_GET_CLASS(pcms);
> > > > > > > +
> > > > > > >          pc_build_smbios(pcms->fw_cfg);
> > > > > > >          pc_build_feature_control_file(pcms);
> > > > > > > +
> > > > > > > +        if (mc->max_cpus > 255) {
> > > > > > > +            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
> > > > > > > +                            sizeof(pcms->boot_cpus_le));
> > > > > > > +        }
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > @@ -1786,9 +1787,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
> > > > > > >          }
> > > > > > >      }
> > > > > > >  
> > > > > > > +    /* increment the number of CPUs */
> > > > > > > +    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);      
> > > > > > 
> > > > > > Is this really safe? What if the guest is in the middle of a
> > > > > > etc/boot-cpus read?    
> > > > > It's safe for boot CPUs but
> > > > > it's not safe to hotplug cpus CPU during BIOS boot at all
> > > > > as number of CPUs read from boot_cpus might not match number
> > > > > of CPUs that received INIT/SIPI wakeup.
> > > > > This problem is ignored for now, I've dropped related SeaBIOS patch
> > > > > by Kevin's request and would explore it some more to avoid race there.
> > > > > 
> > > > > Anyways,
> > > > > Do you have an idea how to improve reading from pcms->boot_cpus_le and make it atomic?    
> > > > 
> > > > No idea. SeaBIOS seems to use insb to read fw_cfg files, so we
> > > > could be updating the data between two reads. I don't think we
> > > > want to design a fw_cfg guest<->host synchronization mechanism.
> > > > 
> > > > I believe the solution is to not change it at all after booting
> > > > the guest. I suggest initializing/reinitializing fw_cfg data only
> > > > on reset.  
> > > I've checked it once more and value read by BIOS atomically
> > > as both QEMU and SeaBIOS use dma interface to transfer data.
> > > BIOS transfers control to QEMU once via
> > > 
> > >  outl(PORT_QEMU_CFG_DMA_ADDR_LOW)
> > > 
> > > and after that access to pcms->boot_cpus_le is protected by BQL.
> > > So there is no need to invent some sort of synchronization
> > > or limit update to fixed points (machine_done/reset).  
> > 
> > You're right. I was assuming the worst case and looking at the
> > non-DMA path.
> > 
> > In this case, I believe we're safe except when running old BIOS
> > that doesn't support fw_cfg DMA.
> old bios won't read this file (as it doesn't know about it)

Oops, you're right. :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

On Thu, 20 Oct 2016 12:15:07 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 20, 2016 at 03:27:50PM +0200, Igor Mammedov wrote:
> > On Thu, 20 Oct 2016 10:27:33 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Oct 20, 2016 at 01:27:34PM +0200, Igor Mammedov wrote:  
> > > > On Wed, 19 Oct 2016 16:29:29 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Wed, Oct 19, 2016 at 05:18:38PM +0200, Igor Mammedov wrote:    
> > > > > > On Wed, 19 Oct 2016 11:15:46 -0200
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >       
> > > > > > > On Wed, Oct 19, 2016 at 02:05:41PM +0200, Igor Mammedov wrote:      
> > > > > > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > > > > > to get number of CPUs present at boot. However 1 byte is
> > > > > > > > not enough to handle more than 255 CPUs.  So add a new
> > > > > > > > fw_cfg file that would allow QEMU to tell it.
> > > > > > > > For compat reasons add file only for machine types that
> > > > > > > > support more than 255 CPUs.
> > > > > > > > 
> > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>        
> > > > > > > [...]      
> > > > > > > >  static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> > > > > > > >                            Error **errp)
> > > > > > > >  {
> > > > > > > > @@ -1232,6 +1221,11 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> > > > > > > >      fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> > > > > > > > +{
> > > > > > > > +    rtc_set_memory(rtc, 0x5f, cpus_count - 1);        
> > > > > > > 
> > > > > > > If we have more than 255 CPUs, shouldn't we at least tell the old
> > > > > > > BIOS that we have 255, instead of silently truncating bits?      
> > > > > > It won't do any good to BIOS as it would hang in AP wakeup due to
> > > > > >  (expected != woken up) condition.      
> > > > > 
> > > > > Even in this case, truncating bits makes it a bit unpredictable:
> > > > > having 257 CPUs would set RTC memory to 0, BIOS will believe it
> > > > > is a UP system.    
> > > > and it will do AP wakeup regardless, where old BIOS will hang
> > > > due to unexpectedly woken-up CPUs regardless of value in cmos.    
> > > 
> > > 0 (1 CPU) seems to be the only value that will not hang (at least
> > > it won't hang at the same point). If cmos_cpu_count is 1, SeaBIOS
> > > won't even wait for the other CPUs to wake up.  
> > I don't see it in code
> > 
> > here is current/old seabios smp init flow:
> > 
> >     if (BSP HAS APIC DISABLED) {                             
> >         // No apic - only the main cpu is present.                               
> >         dprintf(1, "No apic - only the main cpu is present.\n");                 
> >         CountCPUs= 1;                                                            
> >         return;                                                                  
> >     }     
> >   
> 
> We also have these lines:
> 
>     u8 apic_id = ebx>>24;
>     FoundAPICIDs[apic_id/32] |= (1 << (apic_id % 32));
>     CountCPUs = 1;
> 
> >     AP WAKEUP regardless of CMOS_BIOS_SMP_COUNT value
> > 
> >     u8 cmos_smp_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;                       
> >     while (cmos_smp_count != CountCPUs) // we are doomed here due to AP race and
> >                                         // mostly hang here regardless of cmos_smp_count value
> >                                         // as CountCPUs could be anything at this point and counting up  
> 
> CountCPUs will be always 1 on the first (cmos_smp_count !=
> CountCPUs) check, because handle_smp() (which change CountCPUs)
> is protected by SMPLock.
I've overlooked this, that's correct however
BIOS still crashes later in handle_smp()
on APs running wild (usually with KVM emulation error)


> >                                         
> > 
> > and it's been pretty much the same logic throughout history,
> > that's why it doesn't matter  if rtc_read(CMOS_BIOS_SMP_COUNT) is 0 or something else.
> > 
> > SMP support has been introduced by:
> > (e97ca7bd1 Forward port bochs smp changes; rename smpdetect.c to smp.c.)
> > 
> > 
> > [...]  
> > > That said, setting it to 0 (1 CPU) sounds like the best option.
> > > But I would be OK with any other value as long as it is
> > > predictable.  
> > So counting above SeaBIOS behavior shall I still set cmos to 0?  
> 
> In the case we get unpredictable behavior from SeaBIOS for any
> value (I will make some tests to confirm that), setting it to 0
> still seems more likely to avoid weird things from happening with
> BIOSes or OSes we don't control.
ok, it doesn't really matter to me.
I'll set it to 0 and post v5 as reply here

[...]

Re: [Qemu-devel] [PATCH v4 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Kevin O'Connor]

On Thu, Oct 20, 2016 at 10:27:33AM -0200, Eduardo Habkost wrote:
> (Why does SeaBIOS need to start the other CPUs, anyway, except
> for building the APIC ID list for the ACPI tables?)

SeaBIOS only starts the other CPUs when running on QEMU (it does not
on coreboot).  The main reason is to initialize the MTRR registers on
those CPUs.  It's also used to create the apic id list which is used
during the generation of the mptable, and during the generation of the
ACPI tables if an old QEMU machine type is running that doesn't pass
ACPI tables to SeaBIOS.

-Kevin

[Qemu-devel] [PATCH v5 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
to get number of CPUs present at boot. However 1 byte is
not enough to handle more than 255 CPUs.  So add a new
fw_cfg file that would allow QEMU to tell it.
For compat reasons add file only for machine types that
support more than 255 CPUs.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
  * set CMOS value to 0 if cpus count is more than 256 (Eduardo)
v3:
  * make boot_cpus to PCMachineState so it could be kept uptodate
    when a cpu is added/removed and BIOS would read current value
    on system reset
---
 include/hw/i386/pc.h |  2 ++
 hw/i386/pc.c         | 40 +++++++++++++++++++++++++---------------
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b16c448..17fff80 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -37,6 +37,7 @@
 /**
  * PCMachineState:
  * @acpi_dev: link to ACPI PM device that performs ACPI hotplug handling
+ * @boot_cpus_le: number of present VCPUs, referenced by 'etc/boot-cpus' fw_cfg
  */
 struct PCMachineState {
     /*< private >*/
@@ -69,6 +70,7 @@ struct PCMachineState {
     bool apic_xrupt_override;
     unsigned apic_id_limit;
     CPUArchIdList *possible_cpus;
+    uint16_t boot_cpus_le;
 
     /* NUMA information: */
     uint64_t numa_nodes;
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b97795..a60141d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1085,17 +1085,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static int pc_present_cpus_count(PCMachineState *pcms)
-{
-    int i, boot_cpus = 0;
-    for (i = 0; i < pcms->possible_cpus->len; i++) {
-        if (pcms->possible_cpus->cpus[i].cpu) {
-            boot_cpus++;
-        }
-    }
-    return boot_cpus;
-}
-
 static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
@@ -1232,6 +1221,15 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
     fw_cfg_add_file(pcms->fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
 }
 
+static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
+{
+    if (cpus_count > 0xff) {
+        rtc_set_memory(rtc, 0x5f, 0);
+    } else {
+        rtc_set_memory(rtc, 0x5f, cpus_count - 1);
+    }
+}
+
 static
 void pc_machine_done(Notifier *notifier, void *data)
 {
@@ -1240,7 +1238,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     PCIBus *bus = pcms->bus;
 
     /* set the number of CPUs */
-    rtc_set_memory(pcms->rtc, 0x5f, pc_present_cpus_count(pcms) - 1);
+    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
 
     if (bus) {
         int extra_hosts = 0;
@@ -1261,8 +1259,15 @@ void pc_machine_done(Notifier *notifier, void *data)
 
     acpi_setup();
     if (pcms->fw_cfg) {
+        MachineClass *mc = MACHINE_GET_CLASS(pcms);
+
         pc_build_smbios(pcms->fw_cfg);
         pc_build_feature_control_file(pcms);
+
+        if (mc->max_cpus > 255) {
+            fw_cfg_add_file(pcms->fw_cfg, "etc/boot-cpus", &pcms->boot_cpus_le,
+                            sizeof(pcms->boot_cpus_le));
+        }
     }
 }
 
@@ -1786,9 +1791,11 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
         }
     }
 
+    /* increment the number of CPUs */
+    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) + 1);
     if (dev->hotplugged) {
-        /* increment the number of CPUs */
-        rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1);
+        /* Update the number of CPUs in CMOS */
+        rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
     }
 
     found_cpu = pc_find_cpu_slot(pcms, CPU(dev), NULL);
@@ -1842,7 +1849,10 @@ static void pc_cpu_unplug_cb(HotplugHandler *hotplug_dev,
     found_cpu->cpu = NULL;
     object_unparent(OBJECT(dev));
 
-    rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) - 1);
+    /* decrement the number of CPUs */
+    pcms->boot_cpus_le = cpu_to_le16(le16_to_cpu(pcms->boot_cpus_le) - 1);
+    /* Update the number of CPUs in CMOS */
+    rtc_set_cpus_count(pcms->rtc, le16_to_cpu(pcms->boot_cpus_le));
  out:
     error_propagate(errp, local_err);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 11/13] pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> to get number of CPUs present at boot. However 1 byte is
> not enough to handle more than 255 CPUs.  So add a new
> fw_cfg file that would allow QEMU to tell it.
> For compat reasons add file only for machine types that
> support more than 255 CPUs.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

-- 
Eduardo

[Qemu-devel] [PATCH] fixup! pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> to get number of CPUs present at boot. However 1 byte is
> not enough to handle more than 255 CPUs.  So add a new
> fw_cfg file that would allow QEMU to tell it.
> For compat reasons add file only for machine types that
> support more than 255 CPUs.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I suggest squashing this into the patch, to clarify why we are
setting it to 0.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c6f6747..a03b384 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1225,6 +1225,10 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
 static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
 {
     if (cpus_count > 0xff) {
+        /* If the number of CPUs can't be represented in 8 bits, the
+         * BIOS must use "etc/boot-cpus". Set RTC field to 0 just
+         * to make old BIOSes fail more predictably.
+         */
         rtc_set_memory(rtc, 0x5f, 0);
     } else {
         rtc_set_memory(rtc, 0x5f, cpus_count - 1);
-- 
2.7.4

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fixup! pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Igor Mammedov]

On Thu, 20 Oct 2016 16:51:48 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > to get number of CPUs present at boot. However 1 byte is
> > not enough to handle more than 255 CPUs.  So add a new
> > fw_cfg file that would allow QEMU to tell it.
> > For compat reasons add file only for machine types that
> > support more than 255 CPUs.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> I suggest squashing this into the patch, to clarify why we are
> setting it to 0.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Could you squash it in while merging series?

> ---
>  hw/i386/pc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c6f6747..a03b384 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1225,6 +1225,10 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
>  static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
>  {
>      if (cpus_count > 0xff) {
> +        /* If the number of CPUs can't be represented in 8 bits, the
> +         * BIOS must use "etc/boot-cpus". Set RTC field to 0 just
> +         * to make old BIOSes fail more predictably.
> +         */
>          rtc_set_memory(rtc, 0x5f, 0);
>      } else {
>          rtc_set_memory(rtc, 0x5f, cpus_count - 1);

Re: [Qemu-devel] [PATCH] fixup! pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Fri, Oct 21, 2016 at 10:53:27AM +0200, Igor Mammedov wrote:
> On Thu, 20 Oct 2016 16:51:48 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > to get number of CPUs present at boot. However 1 byte is
> > > not enough to handle more than 255 CPUs.  So add a new
> > > fw_cfg file that would allow QEMU to tell it.
> > > For compat reasons add file only for machine types that
> > > support more than 255 CPUs.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > I suggest squashing this into the patch, to clarify why we are
> > setting it to 0.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Could you squash it in while merging series?

I will do.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fixup! pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Michael S. Tsirkin]

On Fri, Oct 21, 2016 at 10:53:27AM +0200, Igor Mammedov wrote:
> On Thu, 20 Oct 2016 16:51:48 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > to get number of CPUs present at boot. However 1 byte is
> > > not enough to handle more than 255 CPUs.  So add a new
> > > fw_cfg file that would allow QEMU to tell it.
> > > For compat reasons add file only for machine types that
> > > support more than 255 CPUs.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > I suggest squashing this into the patch, to clarify why we are
> > setting it to 0.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Could you squash it in while merging series?

I'm not sure who's merging it (Eduardo do you want to?)
if me, I'd rather  see a clean repost, with a squash and acks
included.

> > ---
> >  hw/i386/pc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c6f6747..a03b384 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1225,6 +1225,10 @@ static void pc_build_feature_control_file(PCMachineState *pcms)
> >  static void rtc_set_cpus_count(ISADevice *rtc, uint16_t cpus_count)
> >  {
> >      if (cpus_count > 0xff) {
> > +        /* If the number of CPUs can't be represented in 8 bits, the
> > +         * BIOS must use "etc/boot-cpus". Set RTC field to 0 just
> > +         * to make old BIOSes fail more predictably.
> > +         */
> >          rtc_set_memory(rtc, 0x5f, 0);
> >      } else {
> >          rtc_set_memory(rtc, 0x5f, cpus_count - 1);

Re: [Qemu-devel] [PATCH] fixup! pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs

[Eduardo Habkost]

On Fri, Oct 28, 2016 at 01:08:32AM +0300, Michael S. Tsirkin wrote:
> On Fri, Oct 21, 2016 at 10:53:27AM +0200, Igor Mammedov wrote:
> > On Thu, 20 Oct 2016 16:51:48 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Oct 20, 2016 at 04:58:42PM +0200, Igor Mammedov wrote:
> > > > Currently firmware uses 1 byte at 0x5F offset in RTC CMOS
> > > > to get number of CPUs present at boot. However 1 byte is
> > > > not enough to handle more than 255 CPUs.  So add a new
> > > > fw_cfg file that would allow QEMU to tell it.
> > > > For compat reasons add file only for machine types that
> > > > support more than 255 CPUs.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > > 
> > > I suggest squashing this into the patch, to clarify why we are
> > > setting it to 0.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > Could you squash it in while merging series?
> 
> I'm not sure who's merging it (Eduardo do you want to?)
> if me, I'd rather  see a clean repost, with a squash and acks
> included.

It's already merged. See commit
080ac219cc7d9c55adf925c3545b7450055ad625.

-- 
Eduardo