[Qemu-devel] [PATCH 0/3] Improve error message on invalid CPU topologies

[Qemu-devel] [PATCH 0/3] Improve error message on invalid CPU topologies

[Radim Krčmář]

A not-a-bug reported that QEMU fails when cores=6,maxcpus=240,
https://bugzilla.redhat.com/show_bug.cgi?id=1159264

The error message is
  qemu-kvm: max_cpus is too large. APIC ID of last CPU is 317

Misconfiguration, but how large can maxcpus be?
There probably aren't many QEMU users with knowledge about initial APIC ID
assignment, so it would be better to prevent confusion by making the error
message more accessible.  Especially since big VMs are spreading.

Output after this patch is
  qemu-kvm: invalid CPU topology: maxcpus can't exceed 192 if cores=6 and threads=1

Looking for better wording ...

(The main problem of this series is benefit/code ratio.)

Radim Krčmář (3):
  target-i386: add apicid_pkg_width to topology.h
  target-i386: introduce x86_cpu_nr_apic_ids
  pc: improve error message with impossible max_cpus

 hw/i386/pc.c           | 12 +++++++-----
 target-i386/cpu.c      | 16 ++++++++++++++++
 target-i386/cpu.h      |  1 +
 target-i386/topology.h | 13 +++++++++++++
 tests/test-x86-cpuid.c |  9 +++++++++
 5 files changed, 46 insertions(+), 5 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] target-i386: add apicid_pkg_width to topology.h

[Radim Krčmář]

The APIC ID topology is made of three elements:
smt, core and pkg id;  bit width of first two is determined by the
actual number of hyperthreads/cores and pkg gets the rest.

Basic xAPIC unit tests are included.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 target-i386/topology.h | 13 +++++++++++++
 tests/test-x86-cpuid.c |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/target-i386/topology.h b/target-i386/topology.h
index 07a6c5f..e87a685 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -70,6 +70,19 @@ static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
     return apicid_bitwidth_for_count(nr_cores);
 }
 
+/* Bit width of the Pkg_ID field
+ */
+static inline unsigned apicid_pkg_width(unsigned nr_cores,
+                                        unsigned nr_threads,
+                                        unsigned apicid_limit)
+{
+    unsigned core_smt_width = apicid_core_width(nr_cores, nr_threads) +
+                              apicid_smt_width(nr_cores, nr_threads);
+    unsigned apicid_width = apicid_bitwidth_for_count(apicid_limit);
+
+    return apicid_width > core_smt_width ? apicid_width - core_smt_width : 0;
+}
+
 /* Bit offset of the Core_ID field
  */
 static inline unsigned apicid_core_offset(unsigned nr_cores,
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 8d9f96a..6b74f08 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -31,6 +31,8 @@ static void test_topo_bits(void)
     /* simple tests for 1 thread per core, 1 core per socket */
     g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0);
     g_assert_cmpuint(apicid_core_width(1, 1), ==, 0);
+    g_assert_cmpuint(apicid_pkg_width(1, 1, 255), ==, 8);
+    g_assert_cmpuint(apicid_pkg_width(1, 1, 256), ==, 8);
 
     g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 0), ==, 0);
     g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 1), ==, 1);
@@ -55,6 +57,13 @@ static void test_topo_bits(void)
     g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
     g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
 
+    g_assert_cmpuint(apicid_pkg_width( 2,  2, 255), ==, 6);
+    g_assert_cmpuint(apicid_pkg_width(16, 16, 255), ==, 0);
+    g_assert_cmpuint(apicid_pkg_width(99, 99, 255), ==, 0);
+    g_assert_cmpuint(apicid_pkg_width( 7,  3, 255), ==, 3);
+    g_assert_cmpuint(apicid_pkg_width( 8,  3, 255), ==, 3);
+    g_assert_cmpuint(apicid_pkg_width( 9,  3, 255), ==, 2);
+    g_assert_cmpuint(apicid_pkg_width(12,  6, 255), ==, 1);
 
     /* build a weird topology and see if IDs are calculated correctly
      */
-- 
2.1.0

[Qemu-devel] [PATCH 2/3] target-i386: introduce x86_cpu_nr_apic_ids

[Radim Krčmář]

The number of available APIC IDs depends on chosen topology, because
core/smt choices different from a power of two waste some IDs.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 target-i386/cpu.c | 16 ++++++++++++++++
 target-i386/cpu.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e194601..c77ec76 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2815,6 +2815,22 @@ uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
     }
 }
 
+/* The number of useable APIC IDs based on current topology.
+ *
+ * id_limit is the highest possible APIC ID + 1.
+ */
+unsigned x86_cpu_nr_apic_ids(unsigned id_limit)
+{
+    unsigned package_width;
+
+    if (compat_apic_id_mode)
+        return id_limit;
+
+    package_width = apicid_pkg_width(smp_cores, smp_threads, id_limit);
+
+    return MIN(id_limit, (1 << package_width) * smp_cores * smp_threads);
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9f01831..367968d 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1372,6 +1372,7 @@ void x86_cpu_compat_disable_kvm_features(FeatureWord w, uint32_t features);
 const char *get_register_name_32(unsigned int reg);
 
 uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
+unsigned x86_cpu_nr_apic_ids(unsigned id_limit);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
-- 
2.1.0

[Qemu-devel] [PATCH 3/3] pc: improve error message on invalid topologies

[Radim Krčmář]

VCPU topologies, whose number of cores or hyperthreads doesn't equal
a power of two, won't assign all APIC IDs.
We require an APIC ID for every CPU, so the number of wasted APIC IDs
lowers our maximal CPU count.

We exited while printing how high APIC ID of last CPU is, while users
are likely interested about the maximal maxcpus, so use that instead.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/pc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 61aba9f..557b3cc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -41,6 +41,7 @@
 #include "hw/pci/msi.h"
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "kvm_i386.h"
 #include "hw/xen/xen.h"
@@ -1026,7 +1027,7 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     int i;
     X86CPU *cpu = NULL;
     Error *error = NULL;
-    unsigned long apic_id_limit;
+    unsigned apic_maxcpus;
 
     /* init CPUs */
     if (cpu_model == NULL) {
@@ -1038,10 +1039,11 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
     }
     current_cpu_model = cpu_model;
 
-    apic_id_limit = pc_apic_id_limit(max_cpus);
-    if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) {
-        error_report("max_cpus is too large. APIC ID of last CPU is %lu",
-                     apic_id_limit - 1);
+    apic_maxcpus = x86_cpu_nr_apic_ids(ACPI_CPU_HOTPLUG_ID_LIMIT);
+    if (apic_maxcpus < max_cpus) {
+        error_report("invalid CPU topology: maxcpus can't exceed %u"
+                     " if cores=%u and threads=%u",
+                     apic_maxcpus, smp_cores, smp_threads);
         exit(1);
     }
 
-- 
2.1.0