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

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

[Igor Mammedov]

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

v2 for reference:
[PATCH v2 00/14] pc: q35: x2APIC support in kvm_apic mode
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05630.html

Depends on following series:
[PATCH v5 0/7] intel_iommu: fix EIM
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg01981.html

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

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
  pc: acpi: x2APIC support for SRAT table
  acpi: cphp: support x2APIC entry in cpu._MAT
  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

 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 +
 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/intc/apic_common.c           |  52 ++++++++++++++++++-
 hw/ppc/spapr.c                  |   2 +-
 target-i386/cpu.c               |   2 +-
 14 files changed, 236 insertions(+), 79 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/acpi-defs.h | 18 +++++++++++
 hw/i386/acpi-build.c        | 78 +++++++++++++++++++++++++++++++--------------
 2 files changed, 72 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/i386/acpi-build.c b/hw/i386/acpi-build.c
index e999654..702d254 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 = uid;
+        apic->x2apic_id = 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 v3 02/13] pc: acpi: x2APIC support for SRAT table

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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 702d254..b9819f4 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 = 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 v3 03/13] acpi: cphp: support x2APIC entry in cpu._MAT

[Igor Mammedov]

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

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);
             }
-- 
2.7.4

[Qemu-devel] [PATCH v3 04/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 v3 05/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>
---
 hw/acpi/cpu_hotplug.c | 7 ++++++-
 hw/i386/pc.c          | 7 -------
 2 files changed, 6 insertions(+), 8 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 93ff49c..f1c1013 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
     for (i = 0; i < max_cpus; i++) {
         unsigned int apic_id = x86_cpu_apic_id_from_index(i);
-        assert(apic_id < pcms->apic_id_limit);
         j = numa_get_node_for_cpu(i);
         if (j < nb_numa_nodes) {
             numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
@@ -1190,12 +1189,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 v3 06/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 13505ab..b4b4342 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2872,7 +2872,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 v3 07/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 v3 08/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>
---
 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 v3 09/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
 - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
---
 hw/i386/kvm/apic.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index be55102..9a7dd03 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_enable_x2apic() && 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_enable_x2apic() && 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;
-- 
2.7.4

[Qemu-devel] [PATCH v3 10/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 f1c1013..f807932 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 v3 11/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>
---
CC: david@gibson.dropbear.id.au
CC: peter.maydell@linaro.org
CC: 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 795740d..186b5bd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1491,7 +1491,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 03e3803..fb3b498 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2388,7 +2388,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 v3 12/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 f807932..40eb43b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1084,17 +1084,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)
 {
@@ -1231,6 +1220,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)
 {
@@ -1239,7 +1233,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;
@@ -1260,8 +1254,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));
+        }
     }
 }
 
@@ -1785,9 +1786,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);
@@ -1841,7 +1844,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 v3 13/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 > 254.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 40eb43b..f7070e0 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
@@ -1264,6 +1265,18 @@ 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. "
+                         "IOMMU should have eim=on option set");
+            exit(EXIT_FAILURE);
+        }
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.7.4

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

[Paolo Bonzini]

On 13/10/2016 11:52, Igor Mammedov wrote:
> To play with the feature, one would also need x2apic enabled
> seabios counterpart:
> https://github.com/imammedo/seabios.git x2apic_v4

Is this going to make it into seabios 1.10?

Paolo

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

[Igor Mammedov]

On Thu, 13 Oct 2016 12:01:48 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 13/10/2016 11:52, Igor Mammedov wrote:
> > To play with the feature, one would also need x2apic enabled
> > seabios counterpart:
> > https://github.com/imammedo/seabios.git x2apic_v4  
> 
> Is this going to make it into seabios 1.10?

The only dependency here is a new fwcfg file etc/boot-cpus
which so far (RFC-v2) haven't risen any questions/complains
from QEMU/SeabIOS sides, so we could assume that it won't
change and merge SeaBIOS side first.

BTW:
I'm going to respin SeaBIOS side today dropping per Kevin's suggestion
 [PATCH v4 3/5] error out if present cpus count changed during SMP bringup

> Paolo

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

[Gerd Hoffmann]

On Do, 2016-10-13 at 12:01 +0200, Paolo Bonzini wrote:
> 
> On 13/10/2016 11:52, Igor Mammedov wrote:
> > To play with the feature, one would also need x2apic enabled
> > seabios counterpart:
> > https://github.com/imammedo/seabios.git x2apic_v4
> 
> Is this going to make it into seabios 1.10?

seabios patches are basically ready and are waiting for the qemu patches
being merged.  So, yes, if this series gets reviewed and merged quickly
it should be able to make 1.10

cheers,
  Gerd

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

[Andrew Jones]

On Thu, Oct 13, 2016 at 11:52:45AM +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>
> ---
> CC: david@gibson.dropbear.id.au
> CC: peter.maydell@linaro.org
> CC: 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 795740d..186b5bd 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1491,7 +1491,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 03e3803..fb3b498 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2388,7 +2388,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
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com> 

[Qemu-devel] [PATCH v3 14/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>
---
Forgot to inlude the last patch that
permits using more than 255 cpus since q35-2.8 machine type


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 v3 14/13] pc: q35: bump max_cpus to 288

[Radim Krčmář]

2016-10-13 15:24+0200, Igor Mammedov:
> along with it for machine versions 2.7 and older keep
> it at 255.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

Seems nicer than touching max_cpus in pc_machine_class_init(),

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

> Forgot to inlude the last patch that
> permits using more than 255 cpus since q35-2.8 machine type
> 
> 
> 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 v3 13/13] pc: require IRQ remapping and EIM if there could be x2APIC CPUs

[Radim Krčmář]

2016-10-13 11:52+0200, Igor Mammedov:
> it would prevent starting guest with incorrect configs
> where interrupts couldn't be delivered to CPUs with
> APIC IDs > 254.

off-by-one.

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

>  hw/i386/pc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 40eb43b..f7070e0 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
> @@ -1264,6 +1265,18 @@ 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. "
> +                         "IOMMU should have eim=on option set");
> +            exit(EXIT_FAILURE);
> +        }
> +    }
>  }
>  
>  void pc_guest_info_init(PCMachineState *pcms)
> -- 
> 2.7.4
> 
> 

Re: [Qemu-devel] [PATCH v3 09/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Radim Krčmář]

2016-10-13 11:52+0200, Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>  - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

>  hw/i386/kvm/apic.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index be55102..9a7dd03 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_enable_x2apic() && s->apicbase & MSR_IA32_APICBASE_EXTD) {

This will enable x2apic broadcast quirk even if a machine has no way of
using the standard x2APIC mode, so we'd change behavior if an OS
configured a device for broadcast.

I think Linux is the only user of the paravirtual x2APIC mode and it
doesn't configure devices for broadcast, so I think we're safe with it.

Thinking back, I could have just given it a non-controllable capability
in KVM ...

> +        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_enable_x2apic() && 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;
> -- 
> 2.7.4
> 
> 

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

[Radim Krčmář]

2016-10-13 11:52+0200, 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
> 
> 

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

[no-reply]

Hi,

Your series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Type: series
Message-id: 1476352367-69400-1-git-send-email-imammedo@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/13] pc: q35: x2APIC support in kvm_apic mode

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=16
make docker-test-quick@centos6
make docker-test-mingw@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
bca661e pc: q35: bump max_cpus to 288
17f6729 pc: require IRQ remapping and EIM if there could be x2APIC CPUs
8fc45a4 pc: add 'etc/boot-cpus' fw_cfg file for machine with more than 255 CPUs
a7d6417 increase MAX_CPUMASK_BITS from 255 to 288
b03673c pc: clarify FW_CFG_MAX_CPUS usage comment
f71d194 pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode
87d9001 pc: apic_common: reset APIC ID to initial ID when switching into x2APIC mode
426abcf pc: apic_common: restore APIC ID to initial ID on reset
9cb56de pc: apic_common: extend APIC ID property to 32bit
489db69 pc: leave max apic_id_limit only in legacy cpu hotplug code
a503ed4 acpi: cphp: force switch to modern cpu hotplug if APIC ID > 254
4899cb1 acpi: cphp: support x2APIC entry in cpu._MAT
b9fe39d pc: acpi: x2APIC support for SRAT table
4ffb548 pc: acpi: x2APIC support for MADT table

=== OUTPUT BEGIN ===
Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '65cc4d2748a2c2e6f27f1cf39e07a5dbabd80ebf'
  BUILD   centos6
  ARCHIVE qemu.tgz
  ARCHIVE dtc.tgz
  COPY    RUNNER
  RUN     test-quick in centos6
Packages installed:
SDL-devel-1.2.14-7.el6_7.1.x86_64
ccache-3.1.6-2.el6.x86_64
epel-release-6-8.noarch
gcc-4.4.7-17.el6.x86_64
git-1.7.1-4.el6_7.1.x86_64
glib2-devel-2.28.8-5.el6.x86_64
libfdt-devel-1.4.0-1.el6.x86_64
make-3.81-23.el6.x86_64
package g++ is not installed
pixman-devel-0.32.8-1.el6.x86_64
tar-1.23-15.el6_8.x86_64
zlib-devel-1.2.3-29.el6.x86_64

Environment variables:
PACKAGES=libfdt-devel ccache     tar git make gcc g++     zlib-devel glib2-devel SDL-devel pixman-devel     epel-release
HOSTNAME=b7cb6513802f
TERM=xterm
MAKEFLAGS= -j16
HISTSIZE=1000
J=16
USER=root
CCACHE_DIR=/var/tmp/ccache
EXTRA_CONFIGURE_OPTS=
V=
SHOW_ENV=1
MAIL=/var/spool/mail/root
PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/
LANG=en_US.UTF-8
TARGET_LIST=
HISTCONTROL=ignoredups
SHLVL=1
HOME=/root
TEST_DIR=/tmp/qemu-test
LOGNAME=root
LESSOPEN=||/usr/bin/lesspipe.sh %s
FEATURES= dtc
DEBUG=
G_BROKEN_FILENAMES=1
CCACHE_HASHDIR=
_=/usr/bin/env

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install
No C++ compiler available; disabling C++ specific optional code
Install prefix    /var/tmp/qemu-build/install
BIOS directory    /var/tmp/qemu-build/install/share/qemu
binary directory  /var/tmp/qemu-build/install/bin
library directory /var/tmp/qemu-build/install/lib
module directory  /var/tmp/qemu-build/install/lib/qemu
libexec directory /var/tmp/qemu-build/install/libexec
include directory /var/tmp/qemu-build/install/include
config directory  /var/tmp/qemu-build/install/etc
local state directory   /var/tmp/qemu-build/install/var
Manual directory  /var/tmp/qemu-build/install/share/man
ELF interp prefix /usr/gnemul/qemu-%M
Source path       /tmp/qemu-test/src
C compiler        cc
Host C compiler   cc
C++ compiler      
Objective-C compiler cc
ARFLAGS           rv
CFLAGS            -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g 
QEMU_CFLAGS       -I/usr/include/pixman-1    -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -fPIE -DPIE -m64 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all
LDFLAGS           -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g 
make              make
install           install
python            python -B
smbd              /usr/sbin/smbd
module support    no
host CPU          x86_64
host big endian   no
target list       x86_64-softmmu aarch64-softmmu
tcg debug enabled no
gprof enabled     no
sparse enabled    no
strip binaries    yes
profiler          no
static build      no
pixman            system
SDL support       yes (1.2.14)
GTK support       no 
GTK GL support    no
VTE support       no 
TLS priority      NORMAL
GNUTLS support    no
GNUTLS rnd        no
libgcrypt         no
libgcrypt kdf     no
nettle            no 
nettle kdf        no
libtasn1          no
curses support    no
virgl support     no
curl support      no
mingw32 support   no
Audio drivers     oss
Block whitelist (rw) 
Block whitelist (ro) 
VirtFS support    no
VNC support       yes
VNC SASL support  no
VNC JPEG support  no
VNC PNG support   no
xen support       no
brlapi support    no
bluez  support    no
Documentation     no
PIE               yes
vde support       no
netmap support    no
Linux AIO support no
ATTR/XATTR support yes
Install blobs     yes
KVM support       yes
RDMA support      no
TCG interpreter   no
fdt support       yes
preadv support    yes
fdatasync         yes
madvise           yes
posix_madvise     yes
libcap-ng support no
vhost-net support yes
vhost-scsi support yes
vhost-vsock support yes
Trace backends    log
spice support     no 
rbd support       no
xfsctl support    no
smartcard support no
libusb            no
usb net redir     no
OpenGL support    no
OpenGL dmabufs    no
libiscsi support  no
libnfs support    no
build guest agent yes
QGA VSS support   no
QGA w32 disk info no
QGA MSI support   no
seccomp support   no
coroutine backend ucontext
coroutine pool    yes
debug stack usage no
GlusterFS support no
Archipelago support no
gcov              gcov
gcov enabled      no
TPM support       yes
libssh2 support   no
TPM passthrough   yes
QOM debugging     yes
lzo support       no
snappy support    no
bzip2 support     no
NUMA host support no
tcmalloc support  no
jemalloc support  no
avx2 optimization no
replication support yes
  GEN     x86_64-softmmu/config-devices.mak.tmp
  GEN     aarch64-softmmu/config-devices.mak.tmp
  GEN     config-host.h
  GEN     qemu-options.def
  GEN     qmp-commands.h
  GEN     qapi-types.h
  GEN     qapi-visit.h
  GEN     qapi-event.h
  GEN     qmp-introspect.h
  GEN     module_block.h
  GEN     x86_64-softmmu/config-devices.mak
  GEN     tests/test-qapi-types.h
  GEN     tests/test-qapi-visit.h
  GEN     tests/test-qmp-commands.h
  GEN     tests/test-qapi-event.h
  GEN     tests/test-qmp-introspect.h
  GEN     aarch64-softmmu/config-devices.mak
  GEN     trace/generated-tracers.h
  GEN     trace/generated-tcg-tracers.h
  GEN     trace/generated-helpers-wrappers.h
  GEN     trace/generated-helpers.h
  GEN     config-all-devices.mak
  CC      tests/qemu-iotests/socket_scm_helper.o
  GEN     qga/qapi-generated/qga-qapi-types.h
  GEN     qga/qapi-generated/qga-qapi-visit.h
  GEN     qga/qapi-generated/qga-qmp-commands.h
  GEN     qga/qapi-generated/qga-qapi-types.c
  GEN     qga/qapi-generated/qga-qapi-visit.c
  GEN     qga/qapi-generated/qga-qmp-marshal.c
  GEN     qmp-introspect.c
  GEN     qapi-types.c
  GEN     qapi-visit.c
  GEN     qapi-event.c
  CC      qapi/qapi-visit-core.o
  CC      qapi/qapi-dealloc-visitor.o
  CC      qapi/qmp-input-visitor.o
  CC      qapi/qmp-output-visitor.o
  CC      qapi/qmp-registry.o
  CC      qapi/qmp-dispatch.o
  CC      qapi/string-input-visitor.o
  CC      qapi/string-output-visitor.o
  CC      qapi/opts-visitor.o
  CC      qapi/qapi-clone-visitor.o
  CC      qapi/qmp-event.o
  CC      qapi/qapi-util.o
  CC      qobject/qnull.o
  CC      qobject/qint.o
  CC      qobject/qstring.o
  CC      qobject/qdict.o
  CC      qobject/qlist.o
  CC      qobject/qfloat.o
  CC      qobject/qbool.o
  CC      qobject/qjson.o
  CC      qobject/qobject.o
  CC      qobject/json-lexer.o
  CC      qobject/json-streamer.o
  CC      qobject/json-parser.o
  GEN     trace/generated-tracers.c
  CC      trace/control.o
  CC      trace/qmp.o
  CC      util/osdep.o
  CC      util/cutils.o
  CC      util/unicode.o
  CC      util/qemu-timer-common.o
  CC      util/bufferiszero.o
  CC      util/compatfd.o
  CC      util/event_notifier-posix.o
  CC      util/mmap-alloc.o
  CC      util/oslib-posix.o
  CC      util/qemu-thread-posix.o
  CC      util/qemu-openpty.o
  CC      util/envlist.o
  CC      util/memfd.o
  CC      util/path.o
  CC      util/bitmap.o
  CC      util/module.o
  CC      util/bitops.o
  CC      util/hbitmap.o
  CC      util/fifo8.o
  CC      util/acl.o
  CC      util/error.o
  CC      util/qemu-error.o
  CC      util/id.o
  CC      util/iov.o
  CC      util/qemu-config.o
  CC      util/uri.o
  CC      util/qemu-sockets.o
  CC      util/notify.o
  CC      util/qemu-option.o
  CC      util/qemu-progress.o
  CC      util/hexdump.o
  CC      util/crc32c.o
  CC      util/uuid.o
  CC      util/throttle.o
  CC      util/getauxval.o
  CC      util/readline.o
  CC      util/rfifolock.o
  CC      util/rcu.o
  CC      util/qemu-coroutine.o
  CC      util/qemu-coroutine-lock.o
  CC      util/qemu-coroutine-io.o
  CC      util/qemu-coroutine-sleep.o
  CC      util/coroutine-ucontext.o
  CC      util/base64.o
  CC      util/buffer.o
  CC      util/timed-average.o
  CC      util/log.o
  CC      util/qdist.o
  CC      util/qht.o
  CC      util/range.o
  CC      crypto/pbkdf-stub.o
  CC      stubs/arch-query-cpu-def.o
  CC      stubs/arch-query-cpu-model-expansion.o
  CC      stubs/arch-query-cpu-model-comparison.o
  CC      stubs/arch-query-cpu-model-baseline.o
  CC      stubs/bdrv-next-monitor-owned.o
  CC      stubs/blk-commit-all.o
  CC      stubs/blockdev-close-all-bdrv-states.o
  CC      stubs/clock-warp.o
  CC      stubs/cpu-get-clock.o
  CC      stubs/cpu-get-icount.o
  CC      stubs/dump.o
  CC      stubs/fdset-add-fd.o
  CC      stubs/fdset-find-fd.o
  CC      stubs/fdset-get-fd.o
  CC      stubs/fdset-remove-fd.o
  CC      stubs/get-fd.o
  CC      stubs/gdbstub.o
  CC      stubs/get-next-serial.o
  CC      stubs/get-vm-name.o
  CC      stubs/iothread-lock.o
  CC      stubs/is-daemonized.o
  CC      stubs/machine-init-done.o
  CC      stubs/migr-blocker.o
  CC      stubs/mon-is-qmp.o
  CC      stubs/mon-printf.o
  CC      stubs/monitor-init.o
  CC      stubs/notify-event.o
  CC      stubs/qtest.o
  CC      stubs/replay.o
  CC      stubs/replay-user.o
  CC      stubs/reset.o
  CC      stubs/runstate-check.o
  CC      stubs/slirp.o
  CC      stubs/set-fd-handler.o
  CC      stubs/sysbus.o
  CC      stubs/trace-control.o
  CC      stubs/uuid.o
  CC      stubs/vm-stop.o
  CC      stubs/vmstate.o
  CC      stubs/cpus.o
  CC      stubs/kvm.o
  CC      stubs/qmp_pc_dimm_device_list.o
  CC      stubs/target-monitor-defs.o
  CC      stubs/target-get-monitor-def.o
  CC      stubs/vhost.o
  CC      stubs/iohandler.o
  CC      stubs/ipmi.o
  CC      stubs/smbios_type_38.o
  CC      stubs/pc_madt_cpu_entry.o
  CC      contrib/ivshmem-client/ivshmem-client.o
  CC      contrib/ivshmem-client/main.o
  CC      contrib/ivshmem-server/ivshmem-server.o
  CC      contrib/ivshmem-server/main.o
  CC      qemu-nbd.o
  CC      async.o
  CC      thread-pool.o
  CC      block.o
  CC      blockjob.o
  CC      main-loop.o
  CC      iohandler.o
  CC      qemu-timer.o
  CC      aio-posix.o
  CC      qemu-io-cmds.o
  CC      replication.o
  CC      block/raw_bsd.o
  CC      block/qcow.o
  CC      block/vdi.o
  CC      block/vmdk.o
  CC      block/cloop.o
  CC      block/bochs.o
  CC      block/vpc.o
  CC      block/vvfat.o
  CC      block/dmg.o
  CC      block/qcow2.o
  CC      block/qcow2-refcount.o
  CC      block/qcow2-cluster.o
  CC      block/qcow2-snapshot.o
  CC      block/qed.o
  CC      block/qcow2-cache.o
  CC      block/qed-gencb.o
  CC      block/qed-l2-cache.o
  CC      block/qed-table.o
  CC      block/qed-cluster.o
  CC      block/qed-check.o
  CC      block/vhdx.o
  CC      block/vhdx-endian.o
  CC      block/vhdx-log.o
  CC      block/quorum.o
  CC      block/parallels.o
  CC      block/blkdebug.o
  CC      block/blkverify.o
  CC      block/blkreplay.o
  CC      block/block-backend.o
  CC      block/snapshot.o
  CC      block/qapi.o
  CC      block/raw-posix.o
  CC      block/null.o
  CC      block/mirror.o
  CC      block/commit.o
  CC      block/io.o
  CC      block/throttle-groups.o
  CC      block/nbd-client.o
  CC      block/nbd.o
  CC      block/sheepdog.o
  CC      block/accounting.o
  CC      block/dirty-bitmap.o
  CC      block/write-threshold.o
  CC      block/backup.o
  CC      block/crypto.o
  CC      block/replication.o
  CC      nbd/server.o
  CC      nbd/client.o
  CC      nbd/common.o
  CC      crypto/init.o
  CC      crypto/aes.o
  CC      crypto/hash.o
  CC      crypto/hash-glib.o
  CC      crypto/desrfb.o
  CC      crypto/cipher.o
  CC      crypto/tlscreds.o
  CC      crypto/tlscredsanon.o
  CC      crypto/tlscredsx509.o
  CC      crypto/tlssession.o
  CC      crypto/secret.o
  CC      crypto/random-platform.o
  CC      crypto/pbkdf.o
  CC      crypto/ivgen-plain.o
  CC      crypto/ivgen.o
  CC      crypto/ivgen-essiv.o
  CC      crypto/ivgen-plain64.o
  CC      crypto/afsplit.o
  CC      crypto/xts.o
  CC      crypto/block.o
  CC      crypto/block-luks.o
  CC      crypto/block-qcow.o
  CC      io/channel-buffer.o
  CC      io/channel.o
  CC      io/channel-command.o
  CC      io/channel-file.o
  CC      io/channel-socket.o
  CC      io/channel-tls.o
  CC      io/channel-watch.o
  CC      io/channel-websock.o
  CC      io/channel-util.o
  CC      qom/object.o
  CC      io/task.o
  CC      qom/container.o
  CC      qom/qom-qobject.o
  CC      qom/object_interfaces.o
  GEN     qemu-img-cmds.h
  CC      qemu-bridge-helper.o
  CC      blockdev.o
  CC      qemu-io.o
  CC      blockdev-nbd.o
  CC      iothread.o
  CC      device-hotplug.o
  CC      qdev-monitor.o
  CC      os-posix.o
  CC      qemu-char.o
  CC      page_cache.o
  CC      accel.o
  CC      bt-host.o
  CC      bt-vhci.o
  CC      dma-helpers.o
  CC      vl.o
  CC      tpm.o
  CC      qmp.o
  CC      device_tree.o
  GEN     qmp-marshal.c
  CC      hmp.o
  CC      tcg-runtime.o
  CC      cpus-common.o
  CC      audio/audio.o
  CC      audio/noaudio.o
  CC      audio/wavaudio.o
  CC      audio/mixeng.o
  CC      audio/sdlaudio.o
  CC      audio/ossaudio.o
  CC      audio/wavcapture.o
  CC      backends/rng.o
  CC      backends/rng-random.o
  CC      backends/rng-egd.o
  CC      backends/msmouse.o
  CC      backends/testdev.o
  CC      backends/tpm.o
  CC      backends/hostmem-ram.o
  CC      backends/hostmem.o
  CC      backends/hostmem-file.o
  CC      block/stream.o
  CC      disas/arm.o
  CC      disas/i386.o
  CC      fsdev/qemu-fsdev-opts.o
  CC      fsdev/qemu-fsdev-dummy.o
  CC      hw/acpi/core.o
  CC      hw/acpi/piix4.o
  CC      hw/acpi/pcihp.o
  CC      hw/acpi/ich9.o
  CC      hw/acpi/tco.o
  CC      hw/acpi/cpu_hotplug.o
  CC      hw/acpi/memory_hotplug.o
  CC      hw/acpi/memory_hotplug_acpi_table.o
  CC      hw/acpi/cpu.o
  CC      hw/acpi/acpi_interface.o
  CC      hw/acpi/bios-linker-loader.o
  CC      hw/acpi/aml-build.o
  CC      hw/acpi/ipmi.o
  CC      hw/audio/sb16.o
  CC      hw/audio/es1370.o
  CC      hw/audio/ac97.o
  CC      hw/audio/fmopl.o
  CC      hw/audio/adlib.o
  CC      hw/audio/gus.o
  CC      hw/audio/gusemu_hal.o
  CC      hw/audio/gusemu_mixer.o
  CC      hw/audio/cs4231a.o
  CC      hw/audio/intel-hda.o
  CC      hw/audio/hda-codec.o
  CC      hw/audio/pcspk.o
  CC      hw/audio/wm8750.o
  CC      hw/audio/pl041.o
  CC      hw/audio/lm4549.o
  CC      hw/audio/marvell_88w8618.o
  CC      hw/block/block.o
  CC      hw/block/cdrom.o
  CC      hw/block/hd-geometry.o
  CC      hw/block/fdc.o
  CC      hw/block/m25p80.o
  CC      hw/block/nand.o
  CC      hw/block/pflash_cfi01.o
  CC      hw/block/pflash_cfi02.o
  CC      hw/block/ecc.o
  CC      hw/block/onenand.o
  CC      hw/block/nvme.o
  CC      hw/bt/core.o
  CC      hw/bt/l2cap.o
  CC      hw/bt/sdp.o
  CC      hw/bt/hci.o
  CC      hw/bt/hid.o
  CC      hw/bt/hci-csr.o
  CC      hw/char/ipoctal232.o
  CC      hw/char/parallel.o
  CC      hw/char/pl011.o
  CC      hw/char/serial.o
  CC      hw/char/serial-isa.o
  CC      hw/char/serial-pci.o
  CC      hw/char/virtio-console.o
  CC      hw/char/cadence_uart.o
  CC      hw/char/debugcon.o
  CC      hw/char/imx_serial.o
  CC      hw/core/qdev.o
  CC      hw/core/qdev-properties.o
  CC      hw/core/bus.o
  CC      hw/core/fw-path-provider.o
  CC      hw/core/irq.o
  CC      hw/core/hotplug.o
  CC      hw/core/ptimer.o
  CC      hw/core/sysbus.o
  CC      hw/core/machine.o
  CC      hw/core/null-machine.o
  CC      hw/core/qdev-properties-system.o
  CC      hw/core/loader.o
  CC      hw/core/register.o
  CC      hw/core/or-irq.o
  CC      hw/core/platform-bus.o
  CC      hw/display/ads7846.o
  CC      hw/display/cirrus_vga.o
  CC      hw/display/ssd0303.o
  CC      hw/display/pl110.o
  CC      hw/display/ssd0323.o
  CC      hw/display/vga-pci.o
  CC      hw/display/vga-isa.o
  CC      hw/display/vmware_vga.o
  CC      hw/display/blizzard.o
  CC      hw/display/exynos4210_fimd.o
  CC      hw/display/framebuffer.o
  CC      hw/display/tc6393xb.o
  CC      hw/dma/pl080.o
  CC      hw/dma/pl330.o
  CC      hw/dma/xlnx-zynq-devcfg.o
  CC      hw/dma/i8257.o
  CC      hw/gpio/max7310.o
  CC      hw/gpio/pl061.o
  CC      hw/gpio/zaurus.o
  CC      hw/gpio/gpio_key.o
  CC      hw/i2c/core.o
  CC      hw/i2c/smbus.o
  CC      hw/i2c/smbus_eeprom.o
  CC      hw/i2c/i2c-ddc.o
  CC      hw/i2c/versatile_i2c.o
  CC      hw/i2c/smbus_ich9.o
  CC      hw/i2c/pm_smbus.o
  CC      hw/i2c/bitbang_i2c.o
  CC      hw/i2c/exynos4210_i2c.o
  CC      hw/i2c/imx_i2c.o
  CC      hw/ide/core.o
  CC      hw/i2c/aspeed_i2c.o
  CC      hw/ide/atapi.o
  CC      hw/ide/qdev.o
  CC      hw/ide/pci.o
  CC      hw/ide/isa.o
  CC      hw/ide/piix.o
  CC      hw/ide/microdrive.o
  CC      hw/ide/ahci.o
  CC      hw/ide/ich.o
  CC      hw/input/hid.o
  CC      hw/input/lm832x.o
  CC      hw/input/pckbd.o
  CC      hw/input/pl050.o
  CC      hw/input/ps2.o
  CC      hw/input/stellaris_input.o
  CC      hw/input/tsc2005.o
  CC      hw/input/vmmouse.o
  CC      hw/input/virtio-input.o
  CC      hw/input/virtio-input-hid.o
  CC      hw/input/virtio-input-host.o
  CC      hw/intc/i8259_common.o
  CC      hw/intc/i8259.o
  CC      hw/intc/pl190.o
  CC      hw/intc/imx_avic.o
  CC      hw/intc/realview_gic.o
  CC      hw/intc/ioapic_common.o
  CC      hw/intc/arm_gic_common.o
  CC      hw/intc/arm_gicv2m.o
  CC      hw/intc/arm_gic.o
  CC      hw/intc/arm_gicv3_common.o
  CC      hw/intc/arm_gicv3.o
  CC      hw/intc/arm_gicv3_dist.o
  CC      hw/intc/arm_gicv3_redist.o
  CC      hw/intc/arm_gicv3_its_common.o
  CC      hw/intc/intc.o
  CC      hw/ipack/ipack.o
  CC      hw/ipack/tpci200.o
  CC      hw/ipmi/ipmi_bmc_sim.o
  CC      hw/ipmi/ipmi.o
  CC      hw/ipmi/isa_ipmi_bt.o
  CC      hw/isa/isa-bus.o
  CC      hw/ipmi/isa_ipmi_kcs.o
  CC      hw/ipmi/ipmi_bmc_extern.o
  CC      hw/isa/apm.o
  CC      hw/mem/pc-dimm.o
  CC      hw/mem/nvdimm.o
  CC      hw/misc/applesmc.o
  CC      hw/misc/max111x.o
  CC      hw/misc/tmp105.o
  CC      hw/misc/debugexit.o
  CC      hw/misc/sga.o
  CC      hw/misc/pc-testdev.o
  CC      hw/misc/pci-testdev.o
  CC      hw/misc/arm_l2x0.o
  CC      hw/misc/arm_integrator_debug.o
  CC      hw/misc/a9scu.o
  CC      hw/misc/arm11scu.o
  CC      hw/net/ne2000.o
  CC      hw/net/eepro100.o
  CC      hw/net/pcnet-pci.o
  CC      hw/net/pcnet.o
  CC      hw/net/e1000.o
  CC      hw/net/e1000x_common.o
  CC      hw/net/net_tx_pkt.o
  CC      hw/net/net_rx_pkt.o
  CC      hw/net/e1000e.o
  CC      hw/net/e1000e_core.o
  CC      hw/net/rtl8139.o
  CC      hw/net/vmxnet3.o
  CC      hw/net/smc91c111.o
  CC      hw/net/lan9118.o
  CC      hw/net/ne2000-isa.o
  CC      hw/net/xgmac.o
  CC      hw/net/allwinner_emac.o
  CC      hw/net/imx_fec.o
  CC      hw/net/cadence_gem.o
  CC      hw/net/stellaris_enet.o
  CC      hw/net/rocker/rocker.o
  CC      hw/net/rocker/rocker_fp.o
  CC      hw/net/rocker/rocker_desc.o
  CC      hw/net/rocker/rocker_world.o
  CC      hw/nvram/eeprom93xx.o
  CC      hw/net/rocker/rocker_of_dpa.o
  CC      hw/nvram/fw_cfg.o
  CC      hw/pci-bridge/pci_bridge_dev.o
  CC      hw/pci-bridge/pci_expander_bridge.o
  CC      hw/pci-bridge/xio3130_downstream.o
  CC      hw/pci-bridge/xio3130_upstream.o
  CC      hw/pci-bridge/ioh3420.o
  CC      hw/pci-bridge/i82801b11.o
  CC      hw/pci-host/pam.o
  CC      hw/pci-host/versatile.o
  CC      hw/pci-host/piix.o
  CC      hw/pci/pci.o
  CC      hw/pci-host/q35.o
  CC      hw/pci-host/gpex.o
  CC      hw/pci/pci_bridge.o
  CC      hw/pci/msix.o
  CC      hw/pci/msi.o
  CC      hw/pci/shpc.o
  CC      hw/pci/slotid_cap.o
  CC      hw/pci/pci_host.o
  CC      hw/pci/pcie_host.o
  CC      hw/pci/pcie.o
  CC      hw/pci/pcie_aer.o
  CC      hw/pci/pcie_port.o
  CC      hw/pci/pci-stub.o
  CC      hw/pcmcia/pcmcia.o
  CC      hw/scsi/scsi-disk.o
  CC      hw/scsi/scsi-generic.o
  CC      hw/scsi/scsi-bus.o
  CC      hw/scsi/lsi53c895a.o
  CC      hw/scsi/mptsas.o
  CC      hw/scsi/mptconfig.o
/tmp/qemu-test/src/hw/nvram/fw_cfg.c: In function ‘fw_cfg_dma_transfer’:
/tmp/qemu-test/src/hw/nvram/fw_cfg.c:330: warning: ‘read’ may be used uninitialized in this function
  CC      hw/scsi/megasas.o
  CC      hw/scsi/mptendian.o
  CC      hw/scsi/vmw_pvscsi.o
  CC      hw/scsi/esp.o
  CC      hw/scsi/esp-pci.o
  CC      hw/sd/pl181.o
  CC      hw/sd/ssi-sd.o
  CC      hw/sd/sd.o
  CC      hw/sd/core.o
  CC      hw/sd/sdhci.o
  CC      hw/smbios/smbios.o
  CC      hw/smbios/smbios_type_38.o
  CC      hw/ssi/pl022.o
  CC      hw/ssi/ssi.o
  CC      hw/ssi/xilinx_spips.o
  CC      hw/ssi/aspeed_smc.o
  CC      hw/ssi/stm32f2xx_spi.o
  CC      hw/timer/arm_timer.o
  CC      hw/timer/arm_mptimer.o
  CC      hw/timer/a9gtimer.o
  CC      hw/timer/cadence_ttc.o
  CC      hw/timer/ds1338.o
  CC      hw/timer/hpet.o
  CC      hw/timer/i8254_common.o
  CC      hw/timer/i8254.o
  CC      hw/timer/pl031.o
  CC      hw/timer/twl92230.o
  CC      hw/timer/imx_epit.o
  CC      hw/timer/imx_gpt.o
  CC      hw/timer/stm32f2xx_timer.o
  CC      hw/timer/aspeed_timer.o
  CC      hw/tpm/tpm_tis.o
  CC      hw/tpm/tpm_passthrough.o
  CC      hw/tpm/tpm_util.o
  CC      hw/usb/core.o
  CC      hw/usb/combined-packet.o
  CC      hw/usb/bus.o
  CC      hw/usb/libhw.o
  CC      hw/usb/desc-msos.o
  CC      hw/usb/desc.o
  CC      hw/usb/hcd-uhci.o
  CC      hw/usb/hcd-ohci.o
  CC      hw/usb/hcd-ehci.o
  CC      hw/usb/hcd-ehci-sysbus.o
  CC      hw/usb/hcd-ehci-pci.o
  CC      hw/usb/hcd-xhci.o
  CC      hw/usb/dev-hub.o
  CC      hw/usb/hcd-musb.o
  CC      hw/usb/dev-hid.o
  CC      hw/usb/dev-wacom.o
  CC      hw/usb/dev-uas.o
  CC      hw/usb/dev-audio.o
  CC      hw/usb/dev-storage.o
  CC      hw/usb/dev-serial.o
  CC      hw/usb/dev-bluetooth.o
  CC      hw/usb/dev-network.o
  CC      hw/usb/dev-smartcard-reader.o
  CC      hw/usb/dev-mtp.o
  CC      hw/usb/host-stub.o
  CC      hw/virtio/virtio-rng.o
  CC      hw/virtio/virtio-pci.o
  CC      hw/virtio/virtio-bus.o
  CC      hw/virtio/virtio-mmio.o
  CC      hw/watchdog/watchdog.o
  CC      hw/watchdog/wdt_i6300esb.o
  CC      hw/watchdog/wdt_ib700.o
  CC      migration/socket.o
  CC      migration/migration.o
  CC      migration/fd.o
  CC      migration/exec.o
  CC      migration/tls.o
  CC      migration/vmstate.o
  CC      migration/qemu-file-channel.o
  CC      migration/qemu-file.o
  CC      migration/xbzrle.o
  CC      migration/postcopy-ram.o
  CC      migration/qjson.o
  CC      migration/block.o
  CC      net/net.o
  CC      net/queue.o
  CC      net/checksum.o
  CC      net/util.o
  CC      net/hub.o
  CC      net/socket.o
  CC      net/dump.o
  CC      net/eth.o
  CC      net/l2tpv3.o
  CC      net/tap.o
  CC      net/vhost-user.o
  CC      net/tap-linux.o
  CC      net/slirp.o
  CC      net/filter.o
  CC      net/filter-buffer.o
  CC      net/filter-mirror.o
  CC      net/colo-compare.o
  CC      net/colo.o
  CC      net/filter-rewriter.o
  CC      qom/cpu.o
  CC      replay/replay.o
  CC      replay/replay-internal.o
  CC      replay/replay-time.o
  CC      replay/replay-events.o
  CC      replay/replay-input.o
  CC      replay/replay-char.o
  CC      replay/replay-snapshot.o
  CC      slirp/cksum.o
  CC      slirp/if.o
  CC      slirp/ip_icmp.o
  CC      slirp/ip6_icmp.o
  CC      slirp/ip6_input.o
  CC      slirp/ip6_output.o
  CC      slirp/ip_input.o
  CC      slirp/ip_output.o
  CC      slirp/dnssearch.o
/tmp/qemu-test/src/replay/replay-internal.c: In function ‘replay_put_array’:
/tmp/qemu-test/src/replay/replay-internal.c:65: warning: ignoring return value of ‘fwrite’, declared with attribute warn_unused_result
  CC      slirp/slirp.o
  CC      slirp/dhcpv6.o
  CC      slirp/mbuf.o
  CC      slirp/misc.o
  CC      slirp/sbuf.o
  CC      slirp/socket.o
  CC      slirp/tcp_input.o
/tmp/qemu-test/src/slirp/tcp_input.c: In function ‘tcp_input’:
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_p’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_len’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_tos’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_id’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_off’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_ttl’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_sum’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_src.s_addr’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:219: warning: ‘save_ip.ip_dst.s_addr’ may be used uninitialized in this function
/tmp/qemu-test/src/slirp/tcp_input.c:220: warning: ‘save_ip6.ip_nh’ may be used uninitialized in this function
  CC      slirp/tcp_output.o
  CC      slirp/tcp_timer.o
  CC      slirp/tcp_subr.o
  CC      slirp/bootp.o
  CC      slirp/udp.o
  CC      slirp/tftp.o
  CC      slirp/udp6.o
  CC      slirp/arp_table.o
  CC      ui/keymaps.o
  CC      slirp/ndp_table.o
  CC      ui/console.o
  CC      ui/cursor.o
  CC      ui/qemu-pixman.o
  CC      ui/input.o
  CC      ui/input-legacy.o
  CC      ui/input-linux.o
  CC      ui/input-keymap.o
  CC      ui/sdl_zoom.o
  CC      ui/sdl.o
  CC      ui/x_keymap.o
  CC      ui/vnc.o
  CC      ui/vnc-enc-zlib.o
  CC      ui/vnc-enc-hextile.o
  CC      ui/vnc-enc-tight.o
  CC      ui/vnc-palette.o
  CC      ui/vnc-enc-zrle.o
  CC      ui/vnc-auth-vencrypt.o
  CC      ui/vnc-ws.o
  CC      ui/vnc-jobs.o
  CC      qga/commands.o
  LINK    tests/qemu-iotests/socket_scm_helper
  CC      qga/guest-agent-command-state.o
  CC      qga/main.o
  CC      qga/commands-posix.o
  CC      qga/channel-posix.o
  CC      qga/qapi-generated/qga-qapi-types.o
  CC      qga/qapi-generated/qga-qapi-visit.o
  CC      qga/qapi-generated/qga-qmp-marshal.o
  CC      qmp-introspect.o
  CC      qapi-types.o
  CC      qapi-visit.o
  CC      qapi-event.o
  AR      libqemustub.a
  CC      qemu-img.o
  CC      trace/generated-tracers.o
  CC      qmp-marshal.o
  AR      libqemuutil.a
  CC      optionrom/linuxboot_dma.o
  AS      optionrom/kvmvapic.o
  AS      optionrom/multiboot.o
  AS      optionrom/linuxboot.o
cc: unrecognized option '-no-integrated-as'
cc: unrecognized option '-no-integrated-as'
  BUILD   optionrom/linuxboot_dma.img
  BUILD   optionrom/linuxboot_dma.raw
  BUILD   optionrom/multiboot.img
  BUILD   optionrom/kvmvapic.img
  BUILD   optionrom/linuxboot.img
  BUILD   optionrom/multiboot.raw
  BUILD   optionrom/kvmvapic.raw
  BUILD   optionrom/linuxboot.raw
  SIGN    optionrom/kvmvapic.bin
  SIGN    optionrom/linuxboot.bin
  SIGN    optionrom/multiboot.bin
  SIGN    optionrom/linuxboot_dma.bin
  LINK    qemu-ga
  LINK    ivshmem-client
  LINK    ivshmem-server
  LINK    qemu-nbd
  LINK    qemu-img
  LINK    qemu-io
  LINK    qemu-bridge-helper
  GEN     x86_64-softmmu/hmp-commands.h
  GEN     x86_64-softmmu/hmp-commands-info.h
  GEN     x86_64-softmmu/config-target.h
  GEN     aarch64-softmmu/hmp-commands.h
  GEN     aarch64-softmmu/hmp-commands-info.h
  GEN     aarch64-softmmu/config-target.h
  CC      x86_64-softmmu/exec.o
  CC      x86_64-softmmu/translate-all.o
  CC      x86_64-softmmu/cpu-exec.o
  CC      x86_64-softmmu/cpu-exec-common.o
  CC      x86_64-softmmu/translate-common.o
  CC      x86_64-softmmu/tcg/tcg-op.o
  CC      x86_64-softmmu/tcg/tcg.o
  CC      x86_64-softmmu/tcg/optimize.o
  CC      x86_64-softmmu/tcg/tcg-common.o
  CC      x86_64-softmmu/fpu/softfloat.o
  CC      x86_64-softmmu/disas.o
  CC      x86_64-softmmu/arch_init.o
  CC      x86_64-softmmu/cpus.o
  CC      x86_64-softmmu/monitor.o
  CC      x86_64-softmmu/gdbstub.o
  CC      x86_64-softmmu/balloon.o
  CC      aarch64-softmmu/exec.o
  CC      aarch64-softmmu/translate-all.o
  CC      aarch64-softmmu/cpu-exec.o
  CC      aarch64-softmmu/translate-common.o
  CC      aarch64-softmmu/cpu-exec-common.o
  CC      aarch64-softmmu/tcg/tcg.o
  CC      x86_64-softmmu/ioport.o
  CC      aarch64-softmmu/tcg/tcg-op.o
  CC      aarch64-softmmu/tcg/optimize.o
  CC      aarch64-softmmu/tcg/tcg-common.o
  CC      aarch64-softmmu/fpu/softfloat.o
  CC      aarch64-softmmu/disas.o
  GEN     aarch64-softmmu/gdbstub-xml.c
  CC      aarch64-softmmu/kvm-stub.o
  CC      aarch64-softmmu/cpus.o
  CC      aarch64-softmmu/arch_init.o
  CC      aarch64-softmmu/monitor.o
  CC      aarch64-softmmu/gdbstub.o
  CC      x86_64-softmmu/numa.o
  CC      aarch64-softmmu/balloon.o
  CC      x86_64-softmmu/qtest.o
  CC      x86_64-softmmu/bootdevice.o
  CC      aarch64-softmmu/ioport.o
  CC      aarch64-softmmu/numa.o
  CC      aarch64-softmmu/qtest.o
  CC      aarch64-softmmu/bootdevice.o
  CC      aarch64-softmmu/memory.o
  CC      aarch64-softmmu/cputlb.o
  CC      aarch64-softmmu/memory_mapping.o
  CC      x86_64-softmmu/kvm-all.o
  CC      aarch64-softmmu/dump.o
  CC      aarch64-softmmu/migration/ram.o
  CC      aarch64-softmmu/migration/savevm.o
  CC      aarch64-softmmu/xen-common-stub.o
  CC      aarch64-softmmu/xen-hvm-stub.o
  CC      x86_64-softmmu/memory.o
  CC      x86_64-softmmu/cputlb.o
  CC      x86_64-softmmu/memory_mapping.o
  CC      aarch64-softmmu/hw/adc/stm32f2xx_adc.o
  CC      aarch64-softmmu/hw/block/virtio-blk.o
  CC      aarch64-softmmu/hw/block/dataplane/virtio-blk.o
  CC      x86_64-softmmu/dump.o
  CC      x86_64-softmmu/migration/ram.o
  CC      aarch64-softmmu/hw/char/exynos4210_uart.o
  CC      x86_64-softmmu/migration/savevm.o
  CC      x86_64-softmmu/xen-common-stub.o
  CC      x86_64-softmmu/xen-hvm-stub.o
  CC      x86_64-softmmu/hw/acpi/nvdimm.o
  CC      aarch64-softmmu/hw/char/omap_uart.o
  CC      aarch64-softmmu/hw/char/digic-uart.o
  CC      aarch64-softmmu/hw/char/stm32f2xx_usart.o
  CC      aarch64-softmmu/hw/char/bcm2835_aux.o
  CC      x86_64-softmmu/hw/block/virtio-blk.o
  CC      x86_64-softmmu/hw/block/dataplane/virtio-blk.o
  CC      x86_64-softmmu/hw/char/virtio-serial-bus.o
  CC      x86_64-softmmu/hw/core/nmi.o
  CC      x86_64-softmmu/hw/core/generic-loader.o
  CC      aarch64-softmmu/hw/char/virtio-serial-bus.o
  CC      aarch64-softmmu/hw/core/nmi.o
  CC      aarch64-softmmu/hw/core/generic-loader.o
  CC      aarch64-softmmu/hw/cpu/arm11mpcore.o
  CC      aarch64-softmmu/hw/cpu/realview_mpcore.o
  CC      x86_64-softmmu/hw/cpu/core.o
  CC      aarch64-softmmu/hw/cpu/a9mpcore.o
  CC      aarch64-softmmu/hw/cpu/a15mpcore.o
  CC      aarch64-softmmu/hw/cpu/core.o
  CC      aarch64-softmmu/hw/display/omap_dss.o
  CC      x86_64-softmmu/hw/display/vga.o
  CC      aarch64-softmmu/hw/display/omap_lcdc.o
  CC      x86_64-softmmu/hw/display/virtio-gpu.o
  CC      aarch64-softmmu/hw/display/pxa2xx_lcd.o
  CC      x86_64-softmmu/hw/display/virtio-gpu-3d.o
  CC      aarch64-softmmu/hw/display/bcm2835_fb.o
  CC      aarch64-softmmu/hw/display/vga.o
  CC      x86_64-softmmu/hw/display/virtio-gpu-pci.o
  CC      x86_64-softmmu/hw/display/virtio-vga.o
  CC      aarch64-softmmu/hw/display/virtio-gpu.o
  CC      x86_64-softmmu/hw/intc/apic.o
  CC      aarch64-softmmu/hw/display/virtio-gpu-3d.o
  CC      aarch64-softmmu/hw/display/virtio-gpu-pci.o
  CC      aarch64-softmmu/hw/display/dpcd.o
  CC      aarch64-softmmu/hw/display/xlnx_dp.o
  CC      x86_64-softmmu/hw/intc/apic_common.o
  CC      aarch64-softmmu/hw/dma/xlnx_dpdma.o
  CC      x86_64-softmmu/hw/intc/ioapic.o
  CC      aarch64-softmmu/hw/dma/omap_dma.o
  CC      x86_64-softmmu/hw/isa/lpc_ich9.o
  CC      x86_64-softmmu/hw/misc/vmport.o
  CC      aarch64-softmmu/hw/dma/soc_dma.o
  CC      aarch64-softmmu/hw/dma/pxa2xx_dma.o
  CC      aarch64-softmmu/hw/dma/bcm2835_dma.o
  CC      x86_64-softmmu/hw/misc/ivshmem.o
  CC      aarch64-softmmu/hw/gpio/omap_gpio.o
  CC      aarch64-softmmu/hw/gpio/imx_gpio.o
  CC      x86_64-softmmu/hw/misc/pvpanic.o
  CC      aarch64-softmmu/hw/i2c/omap_i2c.o
  CC      x86_64-softmmu/hw/misc/edu.o
  CC      x86_64-softmmu/hw/misc/hyperv_testdev.o
  CC      aarch64-softmmu/hw/input/pxa2xx_keypad.o
  CC      aarch64-softmmu/hw/input/tsc210x.o
  CC      aarch64-softmmu/hw/intc/armv7m_nvic.o
  CC      aarch64-softmmu/hw/intc/exynos4210_gic.o
  CC      aarch64-softmmu/hw/intc/exynos4210_combiner.o
  CC      aarch64-softmmu/hw/intc/omap_intc.o
  CC      aarch64-softmmu/hw/intc/bcm2835_ic.o
  CC      aarch64-softmmu/hw/intc/bcm2836_control.o
  CC      x86_64-softmmu/hw/net/virtio-net.o
  CC      x86_64-softmmu/hw/net/vhost_net.o
  CC      aarch64-softmmu/hw/intc/allwinner-a10-pic.o
  CC      aarch64-softmmu/hw/intc/aspeed_vic.o
  CC      aarch64-softmmu/hw/misc/ivshmem.o
  CC      aarch64-softmmu/hw/intc/arm_gicv3_cpuif.o
  CC      x86_64-softmmu/hw/scsi/virtio-scsi.o
  CC      aarch64-softmmu/hw/misc/arm_sysctl.o
  CC      x86_64-softmmu/hw/scsi/virtio-scsi-dataplane.o
  CC      aarch64-softmmu/hw/misc/cbus.o
  CC      aarch64-softmmu/hw/misc/exynos4210_pmu.o
  CC      x86_64-softmmu/hw/scsi/vhost-scsi.o
  CC      x86_64-softmmu/hw/timer/mc146818rtc.o
  CC      x86_64-softmmu/hw/vfio/common.o
  CC      x86_64-softmmu/hw/vfio/pci.o
  CC      x86_64-softmmu/hw/vfio/pci-quirks.o
  CC      x86_64-softmmu/hw/vfio/platform.o
  CC      x86_64-softmmu/hw/vfio/calxeda-xgmac.o
  CC      x86_64-softmmu/hw/vfio/amd-xgbe.o
  CC      x86_64-softmmu/hw/vfio/spapr.o
  CC      aarch64-softmmu/hw/misc/imx_ccm.o
  CC      x86_64-softmmu/hw/virtio/virtio.o
  CC      x86_64-softmmu/hw/virtio/virtio-balloon.o
  CC      x86_64-softmmu/hw/virtio/vhost.o
  CC      aarch64-softmmu/hw/misc/imx31_ccm.o
  CC      x86_64-softmmu/hw/virtio/vhost-backend.o
  CC      x86_64-softmmu/hw/virtio/vhost-user.o
  CC      aarch64-softmmu/hw/misc/imx25_ccm.o
  CC      x86_64-softmmu/hw/virtio/vhost-vsock.o
  CC      x86_64-softmmu/hw/i386/multiboot.o
  CC      x86_64-softmmu/hw/i386/pc.o
  CC      x86_64-softmmu/hw/i386/pc_piix.o
  CC      aarch64-softmmu/hw/misc/imx6_ccm.o
  CC      x86_64-softmmu/hw/i386/pc_q35.o
  CC      x86_64-softmmu/hw/i386/pc_sysfw.o
  CC      x86_64-softmmu/hw/i386/x86-iommu.o
  CC      aarch64-softmmu/hw/misc/imx6_src.o
  CC      x86_64-softmmu/hw/i386/intel_iommu.o
  CC      aarch64-softmmu/hw/misc/mst_fpga.o
  CC      aarch64-softmmu/hw/misc/omap_clk.o
  CC      aarch64-softmmu/hw/misc/omap_gpmc.o
  CC      x86_64-softmmu/hw/i386/amd_iommu.o
  CC      aarch64-softmmu/hw/misc/omap_l4.o
  CC      aarch64-softmmu/hw/misc/omap_sdrc.o
  CC      aarch64-softmmu/hw/misc/omap_tap.o
  CC      x86_64-softmmu/hw/i386/kvmvapic.o
  CC      x86_64-softmmu/hw/i386/acpi-build.o
  CC      aarch64-softmmu/hw/misc/bcm2835_mbox.o
/tmp/qemu-test/src/hw/i386/pc.c: In function ‘pc_machine_done’:
/tmp/qemu-test/src/hw/i386/pc.c:1273: error: ‘IntelIOMMUState’ has no member named ‘intr_eim’
make[1]: *** [hw/i386/pc.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/hw/misc/bcm2835_property.o
  CC      aarch64-softmmu/hw/misc/zynq_slcr.o
  CC      aarch64-softmmu/hw/misc/zynq-xadc.o
  CC      aarch64-softmmu/hw/misc/stm32f2xx_syscfg.o
  CC      aarch64-softmmu/hw/misc/edu.o
  CC      aarch64-softmmu/hw/misc/auxbus.o
  CC      aarch64-softmmu/hw/misc/aspeed_scu.o
  CC      aarch64-softmmu/hw/misc/aspeed_sdmc.o
  CC      aarch64-softmmu/hw/net/vhost_net.o
  CC      aarch64-softmmu/hw/net/virtio-net.o
  CC      aarch64-softmmu/hw/pcmcia/pxa2xx.o
  CC      aarch64-softmmu/hw/scsi/virtio-scsi.o
  CC      aarch64-softmmu/hw/scsi/virtio-scsi-dataplane.o
  CC      aarch64-softmmu/hw/scsi/vhost-scsi.o
  CC      aarch64-softmmu/hw/sd/omap_mmc.o
  CC      aarch64-softmmu/hw/sd/pxa2xx_mmci.o
  CC      aarch64-softmmu/hw/ssi/omap_spi.o
  CC      aarch64-softmmu/hw/timer/exynos4210_mct.o
  CC      aarch64-softmmu/hw/ssi/imx_spi.o
  CC      aarch64-softmmu/hw/timer/exynos4210_pwm.o
  CC      aarch64-softmmu/hw/timer/exynos4210_rtc.o
  CC      aarch64-softmmu/hw/timer/omap_gptimer.o
  CC      aarch64-softmmu/hw/timer/omap_synctimer.o
  CC      aarch64-softmmu/hw/timer/pxa2xx_timer.o
  CC      aarch64-softmmu/hw/timer/digic-timer.o
  CC      aarch64-softmmu/hw/timer/allwinner-a10-pit.o
  CC      aarch64-softmmu/hw/usb/tusb6010.o
  CC      aarch64-softmmu/hw/vfio/common.o
  CC      aarch64-softmmu/hw/vfio/pci.o
  CC      aarch64-softmmu/hw/vfio/pci-quirks.o
  CC      aarch64-softmmu/hw/vfio/platform.o
/tmp/qemu-test/src/hw/i386/pc_piix.c: In function ‘igd_passthrough_isa_bridge_create’:
/tmp/qemu-test/src/hw/i386/pc_piix.c:1046: warning: ‘pch_rev_id’ may be used uninitialized in this function
  CC      aarch64-softmmu/hw/vfio/calxeda-xgmac.o
  CC      aarch64-softmmu/hw/vfio/amd-xgbe.o
  CC      aarch64-softmmu/hw/vfio/spapr.o
  CC      aarch64-softmmu/hw/virtio/virtio.o
  CC      aarch64-softmmu/hw/virtio/vhost.o
  CC      aarch64-softmmu/hw/virtio/virtio-balloon.o
  CC      aarch64-softmmu/hw/virtio/vhost-backend.o
  CC      aarch64-softmmu/hw/virtio/vhost-user.o
  CC      aarch64-softmmu/hw/arm/boot.o
  CC      aarch64-softmmu/hw/virtio/vhost-vsock.o
  CC      aarch64-softmmu/hw/arm/exynos4_boards.o
  CC      aarch64-softmmu/hw/arm/collie.o
  CC      aarch64-softmmu/hw/arm/gumstix.o
  CC      aarch64-softmmu/hw/arm/highbank.o
  CC      aarch64-softmmu/hw/arm/digic_boards.o
  CC      aarch64-softmmu/hw/arm/integratorcp.o
  CC      aarch64-softmmu/hw/arm/mainstone.o
  CC      aarch64-softmmu/hw/arm/musicpal.o
  CC      aarch64-softmmu/hw/arm/nseries.o
  CC      aarch64-softmmu/hw/arm/omap_sx1.o
  CC      aarch64-softmmu/hw/arm/palm.o
  CC      aarch64-softmmu/hw/arm/realview.o
  CC      aarch64-softmmu/hw/arm/spitz.o
  CC      aarch64-softmmu/hw/arm/stellaris.o
  CC      aarch64-softmmu/hw/arm/tosa.o
  CC      aarch64-softmmu/hw/arm/versatilepb.o
  CC      aarch64-softmmu/hw/arm/vexpress.o
  CC      aarch64-softmmu/hw/arm/virt.o
  CC      aarch64-softmmu/hw/arm/xilinx_zynq.o
  CC      aarch64-softmmu/hw/arm/z2.o
  CC      aarch64-softmmu/hw/arm/virt-acpi-build.o
  CC      aarch64-softmmu/hw/arm/netduino2.o
  CC      aarch64-softmmu/hw/arm/sysbus-fdt.o
  CC      aarch64-softmmu/hw/arm/armv7m.o
  CC      aarch64-softmmu/hw/arm/exynos4210.o
  CC      aarch64-softmmu/hw/arm/pxa2xx.o
  CC      aarch64-softmmu/hw/arm/pxa2xx_gpio.o
  CC      aarch64-softmmu/hw/arm/pxa2xx_pic.o
  CC      aarch64-softmmu/hw/arm/digic.o
  CC      aarch64-softmmu/hw/arm/omap1.o
  CC      aarch64-softmmu/hw/arm/omap2.o
  CC      aarch64-softmmu/hw/arm/strongarm.o
  CC      aarch64-softmmu/hw/arm/allwinner-a10.o
  CC      aarch64-softmmu/hw/arm/cubieboard.o
  CC      aarch64-softmmu/hw/arm/bcm2835_peripherals.o
  CC      aarch64-softmmu/hw/arm/bcm2836.o
  CC      aarch64-softmmu/hw/arm/raspi.o
  CC      aarch64-softmmu/hw/arm/stm32f205_soc.o
  CC      aarch64-softmmu/hw/arm/xlnx-zynqmp.o
  CC      aarch64-softmmu/hw/arm/xlnx-ep108.o
  CC      aarch64-softmmu/hw/arm/fsl-imx25.o
  CC      aarch64-softmmu/hw/arm/imx25_pdk.o
  CC      aarch64-softmmu/hw/arm/fsl-imx31.o
  CC      aarch64-softmmu/hw/arm/fsl-imx6.o
  CC      aarch64-softmmu/hw/arm/kzm.o
  CC      aarch64-softmmu/hw/arm/sabrelite.o
/tmp/qemu-test/src/hw/i386/acpi-build.c: In function ‘build_append_pci_bus_devices’:
/tmp/qemu-test/src/hw/i386/acpi-build.c:502: warning: ‘notify_method’ may be used uninitialized in this function
  CC      aarch64-softmmu/hw/arm/aspeed_soc.o
  CC      aarch64-softmmu/hw/arm/aspeed.o
  CC      aarch64-softmmu/target-arm/arm-semi.o
  CC      aarch64-softmmu/target-arm/machine.o
  CC      aarch64-softmmu/target-arm/psci.o
  CC      aarch64-softmmu/target-arm/arch_dump.o
  CC      aarch64-softmmu/target-arm/monitor.o
  CC      aarch64-softmmu/target-arm/kvm-stub.o
  CC      aarch64-softmmu/target-arm/translate.o
  CC      aarch64-softmmu/target-arm/op_helper.o
  CC      aarch64-softmmu/target-arm/helper.o
  CC      aarch64-softmmu/target-arm/cpu.o
  CC      aarch64-softmmu/target-arm/neon_helper.o
  CC      aarch64-softmmu/target-arm/iwmmxt_helper.o
  CC      aarch64-softmmu/target-arm/gdbstub.o
  CC      aarch64-softmmu/target-arm/cpu64.o
  CC      aarch64-softmmu/target-arm/translate-a64.o
  CC      aarch64-softmmu/target-arm/helper-a64.o
  CC      aarch64-softmmu/target-arm/gdbstub64.o
  CC      aarch64-softmmu/target-arm/arm-powerctl.o
  CC      aarch64-softmmu/target-arm/crypto_helper.o
make: *** [subdir-x86_64-softmmu] Error 2
make: *** Waiting for unfinished jobs....
/tmp/qemu-test/src/target-arm/translate-a64.c: In function ‘handle_shri_with_rndacc’:
/tmp/qemu-test/src/target-arm/translate-a64.c:6333: warning: ‘tcg_src_hi’ may be used uninitialized in this function
/tmp/qemu-test/src/target-arm/translate-a64.c: In function ‘disas_simd_scalar_two_reg_misc’:
/tmp/qemu-test/src/target-arm/translate-a64.c:8060: warning: ‘rmode’ may be used uninitialized in this function
  GEN     trace/generated-helpers.c
  CC      aarch64-softmmu/trace/control-target.o
  CC      aarch64-softmmu/gdbstub-xml.o
  CC      aarch64-softmmu/trace/generated-helpers.o
  LINK    aarch64-softmmu/qemu-system-aarch64
tests/docker/Makefile.include:107: recipe for target 'docker-run-test-quick@centos6' failed
make: *** [docker-run-test-quick@centos6] Error 2
=== OUTPUT END ===

Test command exited with code: 2


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Igor Mammedov]

On Thu, 13 Oct 2016 21:05:54 -0700 (PDT)
no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:

> Hi,
> 
> Your series failed automatic build test. Please find the testing commands and
> their output below. If you have docker installed, you can probably reproduce it
> locally.
[..]
> /tmp/qemu-test/src/hw/i386/pc.c: In function ‘pc_machine_done’:
> /tmp/qemu-test/src/hw/i386/pc.c:1273: error: ‘IntelIOMMUState’ has no member named ‘intr_eim’
> make[1]: *** [hw/i386/pc.o] Error 1
That's due to dependency on not yet merged EIM fixes series

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

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@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 13/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)
---
 hw/i386/pc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 40eb43b..f7070e0 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
@@ -1264,6 +1265,18 @@ 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. "
+                         "IOMMU should have eim=on option set");
+            exit(EXIT_FAILURE);
+        }
+    }
 }
 
 void pc_guest_info_init(PCMachineState *pcms)
-- 
2.7.4

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

[Radim Krčmář]

2016-10-14 13:21+0200, Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@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
> ---

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
[...]
> @@ -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);
> +    }

Moving the check here seems to make sense, but:

>  
>      /* 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 93ff49c..f1c1013 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)

[Added more context below to show the code around the change]

>      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
>      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
>      for (i = 0; i < max_cpus; i++) {
>          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> -        assert(apic_id < pcms->apic_id_limit);

If you really needed to remove this assert, that means you can
write beyond the end of numa_fw_fg[] below. Are you sure you need
to remove it?

>          j = numa_get_node_for_cpu(i);
>          if (j < nb_numa_nodes) {
>              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);

                           ^^^^^^^^^^^ here

>          }
>      }
> 
> @@ -1190,12 +1189,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
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:43AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>  - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
> ---
>  hw/i386/kvm/apic.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index be55102..9a7dd03 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_enable_x2apic() && s->apicbase & MSR_IA32_APICBASE_EXTD) {

This is going to enable x2apic unconditionally (not just check if
x2apic was enabled). Is this really what you want to do?


> +        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_enable_x2apic() && 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;
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

On Mon, 17 Oct 2016 19:51:12 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:43AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> >  - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
> > ---
> >  hw/i386/kvm/apic.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> > index be55102..9a7dd03 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_enable_x2apic() && s->apicbase & MSR_IA32_APICBASE_EXTD) {  
> 
> This is going to enable x2apic unconditionally (not just check if
> x2apic was enabled). Is this really what you want to do?
Fixed,
 v4 is in reply to the same note from Radim

> 
> 
> > +        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_enable_x2apic() && 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;
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Igor Mammedov]

On Mon, 17 Oct 2016 19:44:52 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> [...]
> > @@ -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);
> > +    }  
> 
> Moving the check here seems to make sense, but:
> 
> >  
> >      /* 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 93ff49c..f1c1013 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)  
> 
> [Added more context below to show the code around the change]
> 
> >      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> >      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> >      for (i = 0; i < max_cpus; i++) {
> >          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > -        assert(apic_id < pcms->apic_id_limit);  
> 
> If you really needed to remove this assert, that means you can
> write beyond the end of numa_fw_fg[] below. Are you sure you need
> to remove it?
> 
> >          j = numa_get_node_for_cpu(i);
> >          if (j < nb_numa_nodes) {
> >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);  
> 
>                            ^^^^^^^^^^^ here
Shouldn't above
  numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
allocate sufficiently sized array?

That's aside, the assert could be kept as it doesn't get in a way
if you'd prefer it that way.

> >          }
> >      }
> > 
> > @@ -1190,12 +1189,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
> >   
> 

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Igor Mammedov]

On Mon, 17 Oct 2016 19:44:52 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> [...]
> > @@ -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);
> > +    }  
> 
> Moving the check here seems to make sense, but:
> 
> >  
> >      /* 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 93ff49c..f1c1013 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)  
> 
> [Added more context below to show the code around the change]
> 
> >      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> >      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> >      for (i = 0; i < max_cpus; i++) {
> >          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > -        assert(apic_id < pcms->apic_id_limit);  
> 
> If you really needed to remove this assert, that means you can
> write beyond the end of numa_fw_fg[] below. Are you sure you need
> to remove it?
> 
> >          j = numa_get_node_for_cpu(i);
> >          if (j < nb_numa_nodes) {
> >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);  
> 
>                            ^^^^^^^^^^^ here
> 
> >          }
Another more radical way to deal with legacy FW_CFG_NUMA
could be to remove it altogether if machine has x2APIC cpus.
It'd be possible to do due to BIOS using QEMU provided
BIOS tables and not using/calling code for built in/legacy
ACPI tables.

Could do it as patch on top if that's sounds ok.

> >      }
> > 
> > @@ -1190,12 +1189,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
> >   
> 

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 11:02:54AM +0200, Igor Mammedov wrote:
> On Mon, 17 Oct 2016 19:44:52 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> > [...]
> > > @@ -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);
> > > +    }  
> > 
> > Moving the check here seems to make sense, but:
> > 
> > >  
> > >      /* 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 93ff49c..f1c1013 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)  
> > 
> > [Added more context below to show the code around the change]
> > 
> > >      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > >      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > >      for (i = 0; i < max_cpus; i++) {
> > >          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > -        assert(apic_id < pcms->apic_id_limit);  
> > 
> > If you really needed to remove this assert, that means you can
> > write beyond the end of numa_fw_fg[] below. Are you sure you need
> > to remove it?
> > 
> > >          j = numa_get_node_for_cpu(i);
> > >          if (j < nb_numa_nodes) {
> > >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);  
> > 
> >                            ^^^^^^^^^^^ here
> Shouldn't above
>   numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> allocate sufficiently sized array?

I believe it should, but that would mean the assert() is still
valid.

> 
> That's aside, the assert could be kept as it doesn't get in a way
> if you'd prefer it that way.

Yes, please. The assert() removal seems unnecessary (and
confusing, because it made me believe that the condition was not
going to be valid anymore).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 11:12:04AM +0200, Igor Mammedov wrote:
> On Mon, 17 Oct 2016 19:44:52 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> > [...]
> > > @@ -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);
> > > +    }  
> > 
> > Moving the check here seems to make sense, but:
> > 
> > >  
> > >      /* 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 93ff49c..f1c1013 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)  
> > 
> > [Added more context below to show the code around the change]
> > 
> > >      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > >      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > >      for (i = 0; i < max_cpus; i++) {
> > >          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > -        assert(apic_id < pcms->apic_id_limit);  
> > 
> > If you really needed to remove this assert, that means you can
> > write beyond the end of numa_fw_fg[] below. Are you sure you need
> > to remove it?
> > 
> > >          j = numa_get_node_for_cpu(i);
> > >          if (j < nb_numa_nodes) {
> > >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);  
> > 
> >                            ^^^^^^^^^^^ here
> > 
> > >          }
> Another more radical way to deal with legacy FW_CFG_NUMA
> could be to remove it altogether if machine has x2APIC cpus.
> It'd be possible to do due to BIOS using QEMU provided
> BIOS tables and not using/calling code for built in/legacy
> ACPI tables.
> 
> Could do it as patch on top if that's sounds ok.

I don't mind either way. Initializing the fields even if they are
not going to be used seems harmless, but if you believe skipping
it would make things simpler, go ahead.

In this case, what would happen if x2apic CPUs are available but
the BIOS is too old?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 09/13] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 09:17:48AM +0200, Igor Mammedov wrote:
> On Mon, 17 Oct 2016 19:51:12 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:43AM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v4:
> > >  - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead
> > > ---
> > >  hw/i386/kvm/apic.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> > > index be55102..9a7dd03 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_enable_x2apic() && s->apicbase & MSR_IA32_APICBASE_EXTD) {  
> > 
> > This is going to enable x2apic unconditionally (not just check if
> > x2apic was enabled). Is this really what you want to do?
> Fixed,
>  v4 is in reply to the same note from Radim

Oops, didn't notice v4 was already there. Sorry for the noise.

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:40AM +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>
> ---
> v3:
>    keep original behaviour where 'id' is readonly after
>    object is realized (pbonzini)
> ---
[...]
> 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
> @@ -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);
> +}

Who exactly is going to read this property and require this logic
to be in the property getter?

Do we really need to expose this to the outside as a magic
property that changes depending on hardware state? Returning
initial_apic_id sounds much simpler.

> +
> +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;

Do we really need to change s->id here too? Won't it be set
automatically to initial_apic_id on reset?

I'm asking this because making it read/write only initial_apic_id
would make it easier to eventually convert the property to a
field-based getter/setter API (maybe even keep it using the
static property system).

Or, even better: do we really need a writeable property named
"id" at all? Is there any valid use case for the user to set it
directly? We could make the code that creates the APIC set
apic->initial_apic_id directly (or use a clearer
"initial-apic-id" property name).

> +}
> +
> +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);

If you are going to add new properties, please register them
using object_class_property_add*().

> +}
> +
>  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 13505ab..b4b4342 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2872,7 +2872,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
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Oct 14, 2016 at 01:25:35PM +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>
> ---
> v4:
>  - s/254/255/ in commit message (Radim)
> ---
>  hw/i386/pc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 40eb43b..f7070e0 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
> @@ -1264,6 +1265,18 @@ 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. "
> +                         "IOMMU should have eim=on option set");

Suggestion for a follow-up patch:

* Error message explaining how to set eim=on if the iommu is
  available
* Error message explaining how to make sure the iommu is created,
  in case it was not even created.

-- 
Eduardo

[Qemu-devel] [PATCH v4 05/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>
---
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 93ff49c..2045525 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

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 01:37:46PM +0200, Igor Mammedov wrote:
> 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>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 05/13] pc: leave max apic_id_limit only in legacy cpu hotplug code

[Igor Mammedov]

On Tue, 18 Oct 2016 08:39:05 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 11:12:04AM +0200, Igor Mammedov wrote:
> > On Mon, 17 Oct 2016 19:44:52 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Oct 13, 2016 at 11:52:39AM +0200, Igor Mammedov wrote:
> > > [...]  
> > > > @@ -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);
> > > > +    }    
> > > 
> > > Moving the check here seems to make sense, but:
> > >   
> > > >  
> > > >      /* 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 93ff49c..f1c1013 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -778,7 +778,6 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)    
> > > 
> > > [Added more context below to show the code around the change]
> > >   
> > > >      numa_fw_cfg = g_new0(uint64_t, 1 + pcms->apic_id_limit + nb_numa_nodes);
> > > >      numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
> > > >      for (i = 0; i < max_cpus; i++) {
> > > >          unsigned int apic_id = x86_cpu_apic_id_from_index(i);
> > > > -        assert(apic_id < pcms->apic_id_limit);    
> > > 
> > > If you really needed to remove this assert, that means you can
> > > write beyond the end of numa_fw_fg[] below. Are you sure you need
> > > to remove it?
> > >   
> > > >          j = numa_get_node_for_cpu(i);
> > > >          if (j < nb_numa_nodes) {
> > > >              numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);    
> > > 
> > >                            ^^^^^^^^^^^ here
> > >   
> > > >          }  
> > Another more radical way to deal with legacy FW_CFG_NUMA
> > could be to remove it altogether if machine has x2APIC cpus.
> > It'd be possible to do due to BIOS using QEMU provided
> > BIOS tables and not using/calling code for built in/legacy
> > ACPI tables.
> > 
> > Could do it as patch on top if that's sounds ok.  
> 
> I don't mind either way. Initializing the fields even if they are
> not going to be used seems harmless, but if you believe skipping
> it would make things simpler, go ahead.
Keying of >255 cpus is a chance to drop legacy FW_CFG_NUMA which isn't
used by modern QEMU/SeaBIOS.
We can't do that for < 255 as it would break migration/BIOS where
it is already present.
I'll look at it some more from SeaBIOS perspective if it's feasible/useful.

> In this case, what would happen if x2apic CPUs are available but
> the BIOS is too old?
Old BIOS is unfixable is there are more than 255,
most often old BIOS would hang in smp_setup() waiting
cmos_smp_count CPUs to wakeup or crash later if smp_scan() lucky and
wins the race.

Or BIOS could fail earlier/later during buffer overflow/allocation
when initializing legacy ACPI tables/pir/mp_table if etc/max-cpus
is big enough.
Corresponding SeaBIOS series is mentioned in cover letter.

This series or dropping FW_CFG_NUMA should not affect (existing) VMs
with less that 255 CPUs though.

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

[Igor Mammedov]

On Tue, 18 Oct 2016 08:56:28 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:40AM +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>
> > ---
> > v3:
> >    keep original behaviour where 'id' is readonly after
> >    object is realized (pbonzini)
> > ---  
> [...]
> > 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
> > @@ -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);
> > +}  
> 
> Who exactly is going to read this property and require this logic
> to be in the property getter?
As it's set/read only from CPU we don't actually have to expose it
as property.
However, I've kept it as read/write property because it has already
been this way and been exposed to external users as some magic property.
Not sure is anyone cares.


> Do we really need to expose this to the outside as a magic
> property that changes depending on hardware state? Returning
> initial_apic_id sounds much simpler.
Well that's what it is now, so I've kept current behavior.
If we decide to change property behavior or drop it altogether
I can do it on top.

> 
> > +
> > +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;  
> 
> Do we really need to change s->id here too? Won't it be set
> automatically to initial_apic_id on reset?
it will after following patch,
[PATCH v3 07/13] pc: apic_common: restore APIC ID to initial ID on reset

but I'd prefer to set it here as well to make consistent and clear
where it comes from.


> I'm asking this because making it read/write only initial_apic_id
> would make it easier to eventually convert the property to a
> field-based getter/setter API (maybe even keep it using the
> static property system).
If we don't care about keeping current behavior then I can make
it static property like it used to be.

> Or, even better: do we really need a writeable property named
> "id" at all? Is there any valid use case for the user to set it
> directly?
if user does it, it likely would make VM unsable,
I don't see a way for user to dot it though as he/she is going to
see APIC object only in realized state where it's forbidden to
change property.

> We could make the code that creates the APIC set
> apic->initial_apic_id directly (or use a clearer
> "initial-apic-id" property name).
So we probably fine with making it readonly (still not static
but a bit less code in this patch) and set apic_id directly
by calling new callback.

 apic_common::set_apic_id(uint32_t new_apic_id){
    this->id = this->initial_apic_id = new_apic_id;
 }

 
> > +}
> > +
> > +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);  
> 
> If you are going to add new properties, please register them
> using object_class_property_add*().
Sure, will fix.


> 
> > +}
> > +
> >  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 13505ab..b4b4342 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2872,7 +2872,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
> >   
> 

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

[Igor Mammedov]

On Tue, 18 Oct 2016 09:27:37 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Oct 14, 2016 at 01:25:35PM +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>
> > ---
> > v4:
> >  - s/254/255/ in commit message (Radim)
> > ---
> >  hw/i386/pc.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 40eb43b..f7070e0 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
> > @@ -1264,6 +1265,18 @@ 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. "
> > +                         "IOMMU should have eim=on option set");  
> 
> Suggestion for a follow-up patch:
> 
> * Error message explaining how to set eim=on if the iommu is
>   available
> * Error message explaining how to make sure the iommu is created,
>   in case it was not even created.
Reason I didn't include how to create iommu/CLI example is that
it could be some other iommu in future so that message could
bit rot over time.

But I can add description if you'd prefer it.
How about something like this:
+            error_report("current -smp configuration requires "
+                         "Intel IOMMU with Extended Interrupt Mode enabled. "
+                         "To enable IOMMU add to command line: "
+                         "-device intel-iommu,intremap=on,eim=on");

Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

But I have a few questions below that are not directly related to
this patch:

> ---
>  include/hw/acpi/acpi-defs.h | 18 +++++++++++
>  hw/i386/acpi-build.c        | 78 +++++++++++++++++++++++++++++++--------------
>  2 files changed, 72 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/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e999654..702d254 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);

Shouldn't length, processor_id, and local_apic_id be converted to
little-endian too?

> +    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 = uid;
> +        apic->x2apic_id = 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);

Why adevc->madt_cpu exists if all devices use
pc_madt_cpu_entry()?

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

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 09:27:37 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Oct 14, 2016 at 01:25:35PM +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>
> > > ---
> > > v4:
> > >  - s/254/255/ in commit message (Radim)
> > > ---
> > >  hw/i386/pc.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 40eb43b..f7070e0 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
> > > @@ -1264,6 +1265,18 @@ 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. "
> > > +                         "IOMMU should have eim=on option set");  
> > 
> > Suggestion for a follow-up patch:
> > 
> > * Error message explaining how to set eim=on if the iommu is
> >   available
> > * Error message explaining how to make sure the iommu is created,
> >   in case it was not even created.
> Reason I didn't include how to create iommu/CLI example is that
> it could be some other iommu in future so that message could
> bit rot over time.

I see.

> 
> But I can add description if you'd prefer it.
> How about something like this:
> +            error_report("current -smp configuration requires "
> +                         "Intel IOMMU with Extended Interrupt Mode enabled. "
> +                         "To enable IOMMU add to command line: "
> +                         "-device intel-iommu,intremap=on,eim=on");

If there could be other iommu devices in the future, we can show
it as an example, but not an instruction to be followed as-is.

Maybe we can just say "You can add an IOMMU using:
-device intel-iommu,intremap=on,eim=on". It would mean it is one
way to add an IOMMU, but not necessarily the only way.

BTW, are there any plans to make machine code create an IOMMU
automatically if the VM config requires it?

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 08:56:28 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:40AM +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>
> > > ---
> > > v3:
> > >    keep original behaviour where 'id' is readonly after
> > >    object is realized (pbonzini)
> > > ---  
> > [...]
> > > 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
> > > @@ -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);
> > > +}  
> > 
> > Who exactly is going to read this property and require this logic
> > to be in the property getter?
> As it's set/read only from CPU we don't actually have to expose it
> as property.
> However, I've kept it as read/write property because it has already
> been this way and been exposed to external users as some magic property.
> Not sure is anyone cares.
> 
> 
> > Do we really need to expose this to the outside as a magic
> > property that changes depending on hardware state? Returning
> > initial_apic_id sounds much simpler.
> Well that's what it is now, so I've kept current behavior.
> If we decide to change property behavior or drop it altogether
> I can do it on top.
> 

I agree to make them static properties as follow-up patch. This
way, if the change breaks anything we can revert only that patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Igor Mammedov]

On Tue, 18 Oct 2016 10:47:02 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> But I have a few questions below that are not directly related to
> this patch:
> 
> > ---
> >  include/hw/acpi/acpi-defs.h | 18 +++++++++++
> >  hw/i386/acpi-build.c        | 78 +++++++++++++++++++++++++++++++--------------
> >  2 files changed, 72 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/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index e999654..702d254 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);  
> 
> Shouldn't length, processor_id, and local_apic_id be converted to
> little-endian too?
it's 1 byte fields only.

> 
> > +    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 = uid;
> > +        apic->x2apic_id = 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);  
> 
> Why adevc->madt_cpu exists if all devices use
> pc_madt_cpu_entry()?
it's been added with ARM target in mind, so that it could be
possible to have generic hotplug code which uses this callback as well
and maybe some day to generalize build_madt() as well.

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

Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 10:47:02AM -0200, Eduardo Habkost wrote:
> On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by withdrawn. See below:

[...]
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index e999654..702d254 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);
> 
> Shouldn't length, processor_id, and local_apic_id be converted to
> little-endian too?

Erm, those fields are all 8-bit. Nevermind. But that means the
new x2apic code below seems wrong:

> 
> > +    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 = uid;

cpu_to_le32()?

> > +        apic->x2apic_id = apic_id;

cpu_to_le32()?

> > +        if (apic_ids->cpus[uid].cpu != NULL) {
> > +            apic->flags = cpu_to_le32(1);
> > +        } else {
> > +            apic->flags = cpu_to_le32(0);
> > +        }
> >      }
> >  }
[...]

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:36AM +0200, Igor Mammedov wrote
[...]
> @@ -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 = apic_id;

cpu_to_le32()?

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 03/13] acpi: cphp: support x2APIC entry in cpu._MAT

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:37AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/cpu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> 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;
> +            }

Shouldn't this patch be applied before 01/13 to avoid triggering
the assert() below?

>              default:
>                  assert(0);
>              }
> -- 
> 2.7.4
> 

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Oct 13, 2016 at 11:52:38AM +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>
> ---
>  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);

What happens we are in legacy mode and this is triggered during
hotplug instead of machine init? Would it break something, or is
it safe?

Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
assert(!value). I assume this means we must replace the QOM
property with something that the user can't fiddle with, right?

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

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 01/13] pc: acpi: x2APIC support for MADT table

[Igor Mammedov]

On Tue, 18 Oct 2016 11:05:47 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 10:47:02AM -0200, Eduardo Habkost wrote:
> > On Thu, Oct 13, 2016 at 11:52:35AM +0200, Igor Mammedov wrote:  
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>  
> 
> Reviewed-by withdrawn. See below:
> 
> [...]
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index e999654..702d254 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);  
> > 
> > Shouldn't length, processor_id, and local_apic_id be converted to
> > little-endian too?  
> 
> Erm, those fields are all 8-bit. Nevermind. But that means the
> new x2apic code below seems wrong:
Thanks for noticing,
 it needs cpu_to_le32() at some places.
I'll fix it and post v4 here.

> 
> >   
> > > +    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 = uid;  
> 
> cpu_to_le32()?
> 
> > > +        apic->x2apic_id = apic_id;  
> 
> cpu_to_le32()?
> 
> > > +        if (apic_ids->cpus[uid].cpu != NULL) {
> > > +            apic->flags = cpu_to_le32(1);
> > > +        } else {
> > > +            apic->flags = cpu_to_le32(0);
> > > +        }
> > >      }
> > >  }  
> [...]
> 

Re: [Qemu-devel] [PATCH v3 03/13] acpi: cphp: support x2APIC entry in cpu._MAT

[Igor Mammedov]

On Tue, 18 Oct 2016 11:34:55 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:37AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/cpu.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > 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;
> > +            }  
> 
> Shouldn't this patch be applied before 01/13 to avoid triggering
> the assert() below?
There is no AcpiMadtProcessorX2Apic before 1/13,

how about squashing this patch into 1/13, that should be cleaner.

> 
> >              default:
> >                  assert(0);
> >              }
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH v3 03/13] acpi: cphp: support x2APIC entry in cpu._MAT

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 03:46:05PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 11:34:55 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:37AM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/acpi/cpu.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > 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;
> > > +            }  
> > 
> > Shouldn't this patch be applied before 01/13 to avoid triggering
> > the assert() below?
> There is no AcpiMadtProcessorX2Apic before 1/13,
> 
> how about squashing this patch into 1/13, that should be cleaner.

It would work, too. I assume you will squash it on v4 of 01/13.

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 18 Oct 2016 11:07:21 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:36AM +0200, Igor Mammedov wrote
> [...]
> > @@ -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 = apic_id;  
> 
> cpu_to_le32()?
Sure,
will fix and respin series as a lot of fixes are accumulated by now.

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

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

[Igor Mammedov]

On Tue, 18 Oct 2016 10:59:17 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 08:56:28 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Oct 13, 2016 at 11:52:40AM +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>
> > > > ---
> > > > v3:
> > > >    keep original behaviour where 'id' is readonly after
> > > >    object is realized (pbonzini)
> > > > ---    
> > > [...]  
> > > > 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
> > > > @@ -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);
> > > > +}    
> > > 
> > > Who exactly is going to read this property and require this logic
> > > to be in the property getter?  
> > As it's set/read only from CPU we don't actually have to expose it
> > as property.
> > However, I've kept it as read/write property because it has already
> > been this way and been exposed to external users as some magic property.
> > Not sure is anyone cares.
> > 
> >   
> > > Do we really need to expose this to the outside as a magic
> > > property that changes depending on hardware state? Returning
> > > initial_apic_id sounds much simpler.  
> > Well that's what it is now, so I've kept current behavior.
> > If we decide to change property behavior or drop it altogether
> > I can do it on top.
> >   
> 
> I agree to make them static properties as follow-up patch. This
> way, if the change breaks anything we can revert only that patch.
Static property won't work here as it should show APIC ID
depending on current CPU mode.

I could make it readonly property on respin,
and set apic.id/initial_apic_id directly from CPU.
Change would be
   - apic_common_set_id()
   + apic_common::set_apic_id() callback
It won't get us less LOC, more likely it will take even more
code to do so.
As it's in this patch, it's at least consistent in a way
values are get/set. And effectively it's readonly due to check:

+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }

as external user can see only realized apic device.

Re: [Qemu-devel] [PATCH v3 03/13] acpi: cphp: support x2APIC entry in cpu._MAT

[Igor Mammedov]

On Tue, 18 Oct 2016 11:47:11 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 03:46:05PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 11:34:55 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Oct 13, 2016 at 11:52:37AM +0200, Igor Mammedov wrote:  
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  hw/acpi/cpu.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > 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;
> > > > +            }    
> > > 
> > > Shouldn't this patch be applied before 01/13 to avoid triggering
> > > the assert() below?  
> > There is no AcpiMadtProcessorX2Apic before 1/13,
> > 
> > how about squashing this patch into 1/13, that should be cleaner.  
> 
> It would work, too. I assume you will squash it on v4 of 01/13.
> 

yep

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 04:01:56PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 10:59:17 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote:
> > > On Tue, 18 Oct 2016 08:56:28 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Oct 13, 2016 at 11:52:40AM +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>
> > > > > ---
> > > > > v3:
> > > > >    keep original behaviour where 'id' is readonly after
> > > > >    object is realized (pbonzini)
> > > > > ---    
> > > > [...]  
> > > > > 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
> > > > > @@ -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);
> > > > > +}    
> > > > 
> > > > Who exactly is going to read this property and require this logic
> > > > to be in the property getter?  
> > > As it's set/read only from CPU we don't actually have to expose it
> > > as property.
> > > However, I've kept it as read/write property because it has already
> > > been this way and been exposed to external users as some magic property.
> > > Not sure is anyone cares.
> > > 
> > >   
> > > > Do we really need to expose this to the outside as a magic
> > > > property that changes depending on hardware state? Returning
> > > > initial_apic_id sounds much simpler.  
> > > Well that's what it is now, so I've kept current behavior.
> > > If we decide to change property behavior or drop it altogether
> > > I can do it on top.
> > >   
> > 
> > I agree to make them static properties as follow-up patch. This
> > way, if the change breaks anything we can revert only that patch.
> Static property won't work here as it should show APIC ID
> depending on current CPU mode.

My suggestion is to _not_ show a different ID depending on the
current mode, to keep it simple. We can just have
"initial-apic-id" and "id" properties.

But, anyway, I didn't mean to suggest static properties
specifically. Where I say "static property" above, please read
"simple property that returns just a simple struct field and
don't need custom getter/setter code". That means either using a
static property (in case we still want it to be writeable, which
I doubt) or using object_property_add_uint*_ptr().

> 
> I could make it readonly property on respin,
> and set apic.id/initial_apic_id directly from CPU.
> Change would be
>    - apic_common_set_id()
>    + apic_common::set_apic_id() callback
> It won't get us less LOC, more likely it will take even more
> code to do so.
> As it's in this patch, it's at least consistent in a way
> values are get/set. And effectively it's readonly due to check:

Don't worry about doing it on the respin. I'm OK with keeping the
current version by now, and change it in a follow-up patch.

> 
> +    if (dev->realized) {
> +        qdev_prop_set_after_realize(dev, name, errp);
> +        return;
> +    }
> 
> as external user can see only realized apic device.

That's true. But we could avoid all the extra getter/setter code
if we just use a static property or a read-only
object_property_add_uint*_ptr() property.

(All I say above are suggestions for a follow-up patch. I'm OK
with keeping the existing behavior like you do in this patch, to
keep this series simple.)

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 18 Oct 2016 11:38:31 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Oct 13, 2016 at 11:52:38AM +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>
> > ---
> >  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);  
> 
> What happens we are in legacy mode and this is triggered during
> hotplug instead of machine init? Would it break something, or is
> it safe?
 case 1: guest with legacy hotplug AML (migrated for example) would use
         legacy interface and it won't be possible to trigger this path
         as target should be started with the same CLI as source
         (hence < 255 cpus)
 case 2: guest started on new QEMU will have new hotplug AML which switches
         QEMU to modern cpu hotplug interface at ACPI tables _INI time
         so this path is unreachable.

Originally it's been static rule:
    since 2.7 use new hotplug interface and old one for older machine types
Well it's complex but Michael insisted on keeping legacy hotplug
by default and do dynamic switching, so here we are.

this behavior is since 2.7:
    commit 679dd1a957df418453efdd3ed2914dba5cd73773
    pc: use new CPU hotplug interface since 2.7 machine type

 
> Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> assert(!value). I assume this means we must replace the QOM
> property with something that the user can't fiddle with, right?
it's readonly to user after machine starts, but allows user
to play modern hotplug interface on old machine types if needed.

assert is there to trap mistake of switching to legacy mode
(which is default) from compat_properties.

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

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

[Igor Mammedov]

On Tue, 18 Oct 2016 12:14:43 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 04:01:56PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 10:59:17 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Oct 18, 2016 at 02:36:10PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 18 Oct 2016 08:56:28 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Thu, Oct 13, 2016 at 11:52:40AM +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>
> > > > > > ---
> > > > > > v3:
> > > > > >    keep original behaviour where 'id' is readonly after
> > > > > >    object is realized (pbonzini)
> > > > > > ---      
> > > > > [...]    
> > > > > > 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
> > > > > > @@ -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);
> > > > > > +}      
> > > > > 
> > > > > Who exactly is going to read this property and require this logic
> > > > > to be in the property getter?    
> > > > As it's set/read only from CPU we don't actually have to expose it
> > > > as property.
> > > > However, I've kept it as read/write property because it has already
> > > > been this way and been exposed to external users as some magic property.
> > > > Not sure is anyone cares.
> > > > 
> > > >     
> > > > > Do we really need to expose this to the outside as a magic
> > > > > property that changes depending on hardware state? Returning
> > > > > initial_apic_id sounds much simpler.    
> > > > Well that's what it is now, so I've kept current behavior.
> > > > If we decide to change property behavior or drop it altogether
> > > > I can do it on top.
> > > >     
> > > 
> > > I agree to make them static properties as follow-up patch. This
> > > way, if the change breaks anything we can revert only that patch.  
> > Static property won't work here as it should show APIC ID
> > depending on current CPU mode.  
> 
> My suggestion is to _not_ show a different ID depending on the
> current mode, to keep it simple. We can just have
> "initial-apic-id" and "id" properties.
> 
> But, anyway, I didn't mean to suggest static properties
> specifically. Where I say "static property" above, please read
> "simple property that returns just a simple struct field and
> don't need custom getter/setter code". That means either using a
> static property (in case we still want it to be writeable, which
> I doubt) or using object_property_add_uint*_ptr().
> 
> > 
> > I could make it readonly property on respin,
> > and set apic.id/initial_apic_id directly from CPU.
> > Change would be
> >    - apic_common_set_id()
> >    + apic_common::set_apic_id() callback
> > It won't get us less LOC, more likely it will take even more
> > code to do so.
> > As it's in this patch, it's at least consistent in a way
> > values are get/set. And effectively it's readonly due to check:  
> 
> Don't worry about doing it on the respin. I'm OK with keeping the
> current version by now, and change it in a follow-up patch.
> 
> > 
> > +    if (dev->realized) {
> > +        qdev_prop_set_after_realize(dev, name, errp);
> > +        return;
> > +    }
> > 
> > as external user can see only realized apic device.  
> 
> That's true. But we could avoid all the extra getter/setter code
> if we just use a static property or a read-only
> object_property_add_uint*_ptr() property.
> 
> (All I say above are suggestions for a follow-up patch. I'm OK
> with keeping the existing behavior like you do in this patch, to
> keep this series simple.)
Ok, I'll respin this patch as is and think/try
the way you are suggesting in follow up.

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

[Igor Mammedov]

On Tue, 18 Oct 2016 10:55:57 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 02:44:55PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 09:27:37 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Fri, Oct 14, 2016 at 01:25:35PM +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>
> > > > ---
> > > > v4:
> > > >  - s/254/255/ in commit message (Radim)
> > > > ---
> > > >  hw/i386/pc.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index 40eb43b..f7070e0 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
> > > > @@ -1264,6 +1265,18 @@ 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. "
> > > > +                         "IOMMU should have eim=on option set");    
> > > 
> > > Suggestion for a follow-up patch:
> > > 
> > > * Error message explaining how to set eim=on if the iommu is
> > >   available
> > > * Error message explaining how to make sure the iommu is created,
> > >   in case it was not even created.  
> > Reason I didn't include how to create iommu/CLI example is that
> > it could be some other iommu in future so that message could
> > bit rot over time.  
> 
> I see.
> 
> > 
> > But I can add description if you'd prefer it.
> > How about something like this:
> > +            error_report("current -smp configuration requires "
> > +                         "Intel IOMMU with Extended Interrupt Mode enabled. "
> > +                         "To enable IOMMU add to command line: "
> > +                         "-device intel-iommu,intremap=on,eim=on");  
> 
> If there could be other iommu devices in the future, we can show
> it as an example, but not an instruction to be followed as-is.
> 
> Maybe we can just say "You can add an IOMMU using:
> -device intel-iommu,intremap=on,eim=on". It would mean it is one
> way to add an IOMMU, but not necessarily the only way.
ok, I'll amend error message.

> 
> BTW, are there any plans to make machine code create an IOMMU
> automatically if the VM config requires it?
not that I know of

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

[Eduardo Habkost]

On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@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);

What happens if:

* x2apic is enabled on CPUID;
* guest sets MSR_IA32_APICBASE_EXTD; an
* the x2apic API is not enabled?

Does that mean kvm_{put,get}_apic_state() was already broken, or
is the x2apic ID translated to the old format by the kernel when
the x2apic API is disabled?

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

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 11:38:31 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Oct 13, 2016 at 11:52:38AM +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>
> > > ---
> > >  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);  
> > 
> > What happens we are in legacy mode and this is triggered during
> > hotplug instead of machine init? Would it break something, or is
> > it safe?
>  case 1: guest with legacy hotplug AML (migrated for example) would use
>          legacy interface and it won't be possible to trigger this path
>          as target should be started with the same CLI as source
>          (hence < 255 cpus)
>  case 2: guest started on new QEMU will have new hotplug AML which switches
>          QEMU to modern cpu hotplug interface at ACPI tables _INI time
>          so this path is unreachable.

I see. Thanks for the explanation!

> 
> Originally it's been static rule:
>     since 2.7 use new hotplug interface and old one for older machine types
> Well it's complex but Michael insisted on keeping legacy hotplug
> by default and do dynamic switching, so here we are.
> 

This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU
hotplug _only_", because legacy CPU hotplug is always available
on startup, right?

> this behavior is since 2.7:
>     commit 679dd1a957df418453efdd3ed2914dba5cd73773
>     pc: use new CPU hotplug interface since 2.7 machine type
> 
>  
> > Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> > assert(!value). I assume this means we must replace the QOM
> > property with something that the user can't fiddle with, right?
> it's readonly to user after machine starts, but allows user
> to play modern hotplug interface on old machine types if needed.
> assert is there to trap mistake of switching to legacy mode
> (which is default) from compat_properties.

What exactly makes the property read-only to the user? Maybe we
should make the setter return an error instead, as all
object_property_set_bool("cpu-hotplug-legacy") calls already use
&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
> > >   
> > 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 18 Oct 2016 13:05:51 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 11:38:31 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Oct 13, 2016 at 11:52:38AM +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>
> > > > ---
> > > >  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);    
> > > 
> > > What happens we are in legacy mode and this is triggered during
> > > hotplug instead of machine init? Would it break something, or is
> > > it safe?  
> >  case 1: guest with legacy hotplug AML (migrated for example) would use
> >          legacy interface and it won't be possible to trigger this path
> >          as target should be started with the same CLI as source
> >          (hence < 255 cpus)
> >  case 2: guest started on new QEMU will have new hotplug AML which switches
> >          QEMU to modern cpu hotplug interface at ACPI tables _INI time
> >          so this path is unreachable.  
> 
> I see. Thanks for the explanation!
> 
> > 
> > Originally it's been static rule:
> >     since 2.7 use new hotplug interface and old one for older machine types
> > Well it's complex but Michael insisted on keeping legacy hotplug
> > by default and do dynamic switching, so here we are.
> >   
> 
> This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU
> hotplug _only_", because legacy CPU hotplug is always available
> on startup, right?
Sorry, I can't parse question, could you rephrase?

> 
> > this behavior is since 2.7:
> >     commit 679dd1a957df418453efdd3ed2914dba5cd73773
> >     pc: use new CPU hotplug interface since 2.7 machine type
> > 
> >    
> > > Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> > > assert(!value). I assume this means we must replace the QOM
> > > property with something that the user can't fiddle with, right?  
> > it's readonly to user after machine starts, but allows user
> > to play modern hotplug interface on old machine types if needed.
> > assert is there to trap mistake of switching to legacy mode
> > (which is default) from compat_properties.  
> 
> What exactly makes the property read-only to the user? Maybe we
> should make the setter return an error instead, as all
> object_property_set_bool("cpu-hotplug-legacy") calls already use
> &error_abort?
My mistake,
it's dynamic property with custom setters in piix4/ich9_pm and user
potentially can write there.

I was under impression that errors are ignored if property comes from
compat_props, if returning error would prevent machine from starting
when property comes from compat_props I can fix cpu-hotplug-legacy to
return error.

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

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

[Radim Krčmář]

2016-10-18 12:56-0200, Eduardo Habkost:
> On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote:
>> Signed-off-by: Igor Mammedov <imammedo@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
>> ---
>> diff --git 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);
> 
> What happens if:
> 
> * x2apic is enabled on CPUID;
> * guest sets MSR_IA32_APICBASE_EXTD; an
> * the x2apic API is not enabled?

KVM expects APIC ID to be in upper 8 bits of the register then.
Guest APIC mode does not come into play if the x2APIC API is not
enabled.  This is to keep compatibility with old KVMs that used xAPIC
format regardless of APIC mode.

> Does that mean kvm_{put,get}_apic_state() was already broken, or
> is the x2apic ID translated to the old format by the kernel when
> the x2apic API is disabled?

The latter.  KVM stores the 8 bits in an appropriate format, but it
doesn't really matter to QEMU: the exchange format without enabled
x2APIC API is defined to be the xAPIC one.  (KVM used to keep always
keep ID in xAPIC format and trapped x2APIC ID reads to shift the value.)

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 05:23:04PM +0200, Igor Mammedov wrote:
> On Tue, 18 Oct 2016 13:05:51 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote:
> > > On Tue, 18 Oct 2016 11:38:31 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Oct 13, 2016 at 11:52:38AM +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>
> > > > > ---
> > > > >  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);    
> > > > 
> > > > What happens we are in legacy mode and this is triggered during
> > > > hotplug instead of machine init? Would it break something, or is
> > > > it safe?  
> > >  case 1: guest with legacy hotplug AML (migrated for example) would use
> > >          legacy interface and it won't be possible to trigger this path
> > >          as target should be started with the same CLI as source
> > >          (hence < 255 cpus)
> > >  case 2: guest started on new QEMU will have new hotplug AML which switches
> > >          QEMU to modern cpu hotplug interface at ACPI tables _INI time
> > >          so this path is unreachable.  
> > 
> > I see. Thanks for the explanation!
> > 
> > > 
> > > Originally it's been static rule:
> > >     since 2.7 use new hotplug interface and old one for older machine types
> > > Well it's complex but Michael insisted on keeping legacy hotplug
> > > by default and do dynamic switching, so here we are.

Do you rememer the reasoning for that?

> > >   
> > 
> > This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU
> > hotplug _only_", because legacy CPU hotplug is always available
> > on startup, right?
> Sorry, I can't parse question, could you rephrase?

I was just trying to clarify the meaning of
PCMachineClass::legacy_cpu_hotplug.

I thought legacy CPU hotplug was available only if
PCMC::legacy_cpu_hotplug was set. But legacy hotplug is still
available even on pc-2.7 (and then switched dynamically).

So the difference is that PCMC::legacy_cpu_hotplug only disables
the ability to dynamically switch to modern hotplug, but doesn't
disable legacy hotplug completely. Correct?

> 
> > 
> > > this behavior is since 2.7:
> > >     commit 679dd1a957df418453efdd3ed2914dba5cd73773
> > >     pc: use new CPU hotplug interface since 2.7 machine type
> > > 
> > >    
> > > > Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> > > > assert(!value). I assume this means we must replace the QOM
> > > > property with something that the user can't fiddle with, right?  
> > > it's readonly to user after machine starts, but allows user
> > > to play modern hotplug interface on old machine types if needed.
> > > assert is there to trap mistake of switching to legacy mode
> > > (which is default) from compat_properties.  
> > 
> > What exactly makes the property read-only to the user? Maybe we
> > should make the setter return an error instead, as all
> > object_property_set_bool("cpu-hotplug-legacy") calls already use
> > &error_abort?
> My mistake,
> it's dynamic property with custom setters in piix4/ich9_pm and user
> potentially can write there.
> 
> I was under impression that errors are ignored if property comes from
> compat_props, if returning error would prevent machine from starting
> when property comes from compat_props I can fix cpu-hotplug-legacy to
> return error.

compat_props set errp to &error_abort, see
machine_register_compat_props().

-- 
Eduardo

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

[Eduardo Habkost]

On Tue, Oct 18, 2016 at 06:26:54PM +0200, Radim Krčmář wrote:
> 2016-10-18 12:56-0200, Eduardo Habkost:
> > On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote:
> >> Signed-off-by: Igor Mammedov <imammedo@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
> >> ---
> >> diff --git 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);
> > 
> > What happens if:
> > 
> > * x2apic is enabled on CPUID;
> > * guest sets MSR_IA32_APICBASE_EXTD; an
> > * the x2apic API is not enabled?
> 
> KVM expects APIC ID to be in upper 8 bits of the register then.
> Guest APIC mode does not come into play if the x2APIC API is not
> enabled.  This is to keep compatibility with old KVMs that used xAPIC
> format regardless of APIC mode.
> 
> > Does that mean kvm_{put,get}_apic_state() was already broken, or
> > is the x2apic ID translated to the old format by the kernel when
> > the x2apic API is disabled?
> 
> The latter.  KVM stores the 8 bits in an appropriate format, but it
> doesn't really matter to QEMU: the exchange format without enabled
> x2APIC API is defined to be the xAPIC one.  (KVM used to keep always
> keep ID in xAPIC format and trapped x2APIC ID reads to shift the value.)

Thanks for the clarification!

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

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 18 Oct 2016 14:37:39 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Oct 18, 2016 at 05:23:04PM +0200, Igor Mammedov wrote:
> > On Tue, 18 Oct 2016 13:05:51 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Oct 18, 2016 at 04:34:53PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 18 Oct 2016 11:38:31 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Thu, Oct 13, 2016 at 11:52:38AM +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>
> > > > > > ---
> > > > > >  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);      
> > > > > 
> > > > > What happens we are in legacy mode and this is triggered during
> > > > > hotplug instead of machine init? Would it break something, or is
> > > > > it safe?    
> > > >  case 1: guest with legacy hotplug AML (migrated for example) would use
> > > >          legacy interface and it won't be possible to trigger this path
> > > >          as target should be started with the same CLI as source
> > > >          (hence < 255 cpus)
> > > >  case 2: guest started on new QEMU will have new hotplug AML which switches
> > > >          QEMU to modern cpu hotplug interface at ACPI tables _INI time
> > > >          so this path is unreachable.    
> > > 
> > > I see. Thanks for the explanation!
> > >   
> > > > 
> > > > Originally it's been static rule:
> > > >     since 2.7 use new hotplug interface and old one for older machine types
> > > > Well it's complex but Michael insisted on keeping legacy hotplug
> > > > by default and do dynamic switching, so here we are.  
> 
> Do you rememer the reasoning for that?
Ancient SeaBIOS that doesn't get ACPI tables from QEMU supports only
legacy interface. So it's been argued that new machines should start
in legacy mode so that CPU hotplug would be operational with old SeaBIOS.
That's not applicable in practice as QEMU with new hotplug support ships
with SeaBIOS supporting ACPI tables from QEMU.
So the only case, when it could happen, is user overrides default bios
image with old one for purposes of debugging old bios with new machine
type. (that user could have used old machine type for that purpose and
we could have kept code simpler, but train already left and we are
stuck with supporting dynamic switching).

> > > >     
> > > 
> > > This means PCMachineClass::legacy_cpu_hotplug means "legacy CPU
> > > hotplug _only_", because legacy CPU hotplug is always available
> > > on startup, right?  
> > Sorry, I can't parse question, could you rephrase?  
> 
> I was just trying to clarify the meaning of
> PCMachineClass::legacy_cpu_hotplug.
> 
> I thought legacy CPU hotplug was available only if
> PCMC::legacy_cpu_hotplug was set. But legacy hotplug is still
> available even on pc-2.7 (and then switched dynamically).
both hoptlug methods available even on pre pc-2.7 machines,
and legacy hotplug is used by default. User can one way switch
mode to modern hotplug by setting cpu-hotplug-legacy prop to false
which is done by writing 0 at the beginning of legacy CPU bitmap
from AML by guest.

> 
> So the difference is that PCMC::legacy_cpu_hotplug only disables
> the ability to dynamically switch to modern hotplug, but doesn't
> disable legacy hotplug completely. Correct?
yep,
more exactly PCMC::legacy_cpu_hotplug picks whether legacy or modern
AML code should be generated by QEMU.

After guest is switched to modern mode, it can't switch back
'abort(!value)' takes care about that.

> 
> >   
> > >   
> > > > this behavior is since 2.7:
> > > >     commit 679dd1a957df418453efdd3ed2914dba5cd73773
> > > >     pc: use new CPU hotplug interface since 2.7 machine type
> > > > 
> > > >      
> > > > > Unrelated to this patch: piix4_set_cpu_hotplug_legacy() has an
> > > > > assert(!value). I assume this means we must replace the QOM
> > > > > property with something that the user can't fiddle with, right?    
> > > > it's readonly to user after machine starts, but allows user
> > > > to play modern hotplug interface on old machine types if needed.
> > > > assert is there to trap mistake of switching to legacy mode
> > > > (which is default) from compat_properties.    
> > > 
> > > What exactly makes the property read-only to the user? Maybe we
> > > should make the setter return an error instead, as all
> > > object_property_set_bool("cpu-hotplug-legacy") calls already use
> > > &error_abort?  
> > My mistake,
> > it's dynamic property with custom setters in piix4/ich9_pm and user
> > potentially can write there.
> > 
> > I was under impression that errors are ignored if property comes from
> > compat_props, if returning error would prevent machine from starting
> > when property comes from compat_props I can fix cpu-hotplug-legacy to
> > return error.  
> 
> compat_props set errp to &error_abort, see
> machine_register_compat_props().
Than I guess that assert could be replaced by error_setg(),
I'll post a separate patch for this (as it's unrelated to this series).