[Qemu-devel] Subject: [PATCH 00/27] Fix APIC-ID-based CPU topology, take 3

[Qemu-devel] Subject: [PATCH 00/27] Fix APIC-ID-based CPU topology, take 3

[Eduardo Habkost]

Overview of patches:
 * 1-3 are just header dependency/conflict cleanups
 * 4 is the APIC ID initialization patch sent earlier by Igor
 * 5-6 change the KVM code to use the APIC ID as vcpu_id, on i386
 * 7-21 refactor the PC initialization code to allow a compatibility
   argument to be easily set by the pc-1.2 machine-init function later
 * 22-23 just make the PC code ready to support APIC IDs != cpu_index
   (but keeps existing behavior)
 * 24 just changes testing infrastructure to allow a target-i386-specific
   unit-test to be added
 * 25 adds utility functions that implement the topology-based APIC ID
   calculation
 * 26 just makes pc-1.2 and pc-1.3 use different machine-init functions
 * 27 finally implements the fix, keeping compatibility on pc-1.2

Changes v2 -> v3:
 * Not creating a new "PC" class, anymore
 * Patch 1: move qemu_open() & qemu_close() to qemu-stdio.h, too
 * Include "[PATCH v5] target-i386: initialize APIC at CPU level" from Igor
 * Let the PC code set the APIC ID directly, between cpu_x86_init() and
   x86_cpu_realize() calls, instead of adding new arguments to X86CPU init
   functions
 * Changes to the PC initialization code: introduce a PCInitArgs struct
 * Use a separate machine init function, that enables the compatibility mode
   on pc-1.2

Changes v1 -> v2:

 * Created a qemu-stdio.h file for some I/O related definitions
 * Now all the topology-based APIC ID calculation logic is inside pc.c
 * I have now created a "PC" object just to carry the machine-type compatibility
   flag that keeps the previous behavior.
   * Dropped the qemu_global_get() function, as it is not needed anymore
 * Fixed compile errors on non-x86 targets with KVM enabled (ppc, s390x)
 * Add more descriptive comments on THE FW_CFG_MAX_CPUS calculation
 * Add pointers to documentation on topology.h

Bug description:

The CPU APIC IDs generated by QEMU are broken if the number of cores-per-socket
or threads-per-core are not powers of 2, as the bits on the APIC ID do not
correspond to what's expected to reflect the CPU sockets/cores/threads
topology[1].

[1] http://software.intel.com/en-us/articles/intel-64-architecture-processor-topolog
y-enumeration/

This series is available on git, at:
 https://github.com/ehabkost/qemu-hacks/commits/apicid-topology-v3


Eduardo Habkost (26):
  move I/O-related definitions from qemu-common.h to a new header
    (qemu-stdio.h)
  cpus.h: include qemu-stdio.h
  hw/apic.c: rename bit functions to not conflict with bitops.h
  kvm: create kvm_arch_vcpu_id() function
  target-i386: kvm: set vcpu_id to APIC ID instead of CPU index
  pc: pc_init1(): always use rom_memory on pc_memory_init() call
  pc: pc_init1(): remove MemoryRegion arguments
  pc: pc_init1(): get QEMUMachineInitArgs argument
  pc: create PCInitArgs struct
  pc: add PC_DEFAULT_CPU_MODEL #define
  pc: add PCInitArgs parameter to pc_cpus_init()
  pc: pass PCInitArgs struct to pc_memory_init()
  pc: use FWCfgState* instead of void* for fw_cfg data
  pc: rename bochs_bios_init() to pc_bios_init()
  pc: pass PCInitArgs struct to pc_bios_init()
  xen_machine_pv: use cpu_init() instead of cpu_x86_init()
  pc: isolate the code that create CPUs
  cpu_x86_init: check for x86_cpu_realize() errors
  target-i386: do not call x86_cpu_realize() on cpu_x86_init()
  fw_cfg: remove FW_CFG_MAX_CPUS from fw_cfg_init()
  pc: set CPU APIC ID explicitly
  pc: set fw_cfg data based on APIC ID calculation
  tests: support target-specific unit tests
  target-i386: topology & APIC ID utility functions
  pc: create separate init function for pc-1.3
  pc: generate APIC IDs according to CPU topology

Igor Mammedov (1):
  target-i386: initialize APIC at CPU level

 cpus.h                 |   2 +
 hw/apic.c              |  35 +++++-----
 hw/fw_cfg.c            |   1 -
 hw/pc.c                | 185 ++++++++++++++++++++++++++++---------------------
 hw/pc.h                |  32 ++++++---
 hw/pc_piix.c           | 140 +++++++++++++++++--------------------
 hw/ppc_newworld.c      |   1 +
 hw/ppc_oldworld.c      |   1 +
 hw/sun4m.c             |   3 +
 hw/sun4u.c             |   1 +
 hw/xen_machine_pv.c    |   4 +-
 kvm-all.c              |   2 +-
 kvm.h                  |   3 +
 qemu-common.h          |  59 +---------------
 qemu-stdio.h           |  76 ++++++++++++++++++++
 target-i386/cpu.c      |  57 +++++++++++++++
 target-i386/cpu.h      |  14 ++++
 target-i386/helper.c   |   7 +-
 target-i386/kvm.c      |   5 ++
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++
 target-ppc/kvm.c       |   5 ++
 target-s390x/kvm.c     |   5 ++
 tests/.gitignore       |   1 +
 tests/Makefile         |  19 ++++-
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++
 25 files changed, 645 insertions(+), 247 deletions(-)
 create mode 100644 qemu-stdio.h
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

-- 
1.7.11.7

[Qemu-devel] [PATCH 01/27] move I/O-related definitions from qemu-common.h to a new header (qemu-stdio.h)

[Eduardo Habkost]

This will help reduce the qemu-common.h dependency hell.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
--
Changes v1 -> v2:
 - move qemu_open() & qemu_close() to qemu-stdio.h, too
---
 qemu-common.h | 59 ++--------------------------------------------
 qemu-stdio.h  | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 57 deletions(-)
 create mode 100644 qemu-stdio.h

diff --git a/qemu-common.h b/qemu-common.h
index b54612b..adae22a 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -6,6 +6,8 @@
 #include "compiler.h"
 #include "config-host.h"
 
+#include "qemu-stdio.h"
+
 #if defined(__arm__) || defined(__sparc__) || defined(__mips__) || defined(__hppa__) || defined(__ia64__)
 #define WORDS_ALIGNED
 #endif
@@ -48,28 +50,6 @@ typedef struct MigrationParams MigrationParams;
 #include "qemu-os-posix.h"
 #endif
 
-#ifndef O_LARGEFILE
-#define O_LARGEFILE 0
-#endif
-#ifndef O_BINARY
-#define O_BINARY 0
-#endif
-#ifndef MAP_ANONYMOUS
-#define MAP_ANONYMOUS MAP_ANON
-#endif
-#ifndef ENOMEDIUM
-#define ENOMEDIUM ENODEV
-#endif
-#if !defined(ENOTSUP)
-#define ENOTSUP 4096
-#endif
-#if !defined(ECANCELED)
-#define ECANCELED 4097
-#endif
-#ifndef TIME_MAX
-#define TIME_MAX LONG_MAX
-#endif
-
 /* HOST_LONG_BITS is the size of a native pointer in bits. */
 #if UINTPTR_MAX == UINT32_MAX
 # define HOST_LONG_BITS 32
@@ -79,39 +59,6 @@ typedef struct MigrationParams MigrationParams;
 # error Unknown pointer size
 #endif
 
-#ifndef CONFIG_IOVEC
-#define CONFIG_IOVEC
-struct iovec {
-    void *iov_base;
-    size_t iov_len;
-};
-/*
- * Use the same value as Linux for now.
- */
-#define IOV_MAX		1024
-#else
-#include <sys/uio.h>
-#endif
-
-typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
-
-#ifdef _WIN32
-#define fsync _commit
-#if !defined(lseek)
-# define lseek _lseeki64
-#endif
-int qemu_ftruncate64(int, int64_t);
-#if !defined(ftruncate)
-# define ftruncate qemu_ftruncate64
-#endif
-
-static inline char *realpath(const char *path, char *resolved_path)
-{
-    _fullpath(resolved_path, path, _MAX_PATH);
-    return resolved_path;
-}
-#endif
 
 /* icount */
 void configure_icount(const char *option);
@@ -208,8 +155,6 @@ const char *path(const char *pathname);
 
 void *qemu_oom_check(void *ptr);
 
-int qemu_open(const char *name, int flags, ...);
-int qemu_close(int fd);
 ssize_t qemu_write_full(int fd, const void *buf, size_t count)
     QEMU_WARN_UNUSED_RESULT;
 ssize_t qemu_send_full(int fd, const void *buf, size_t count, int flags)
diff --git a/qemu-stdio.h b/qemu-stdio.h
new file mode 100644
index 0000000..b2e8eda
--- /dev/null
+++ b/qemu-stdio.h
@@ -0,0 +1,76 @@
+/* Some basic definitions related to stdio.h or other I/O interfaces
+ */
+#ifndef QEMU_STDIO_H
+#define QEMU_STDIO_H
+
+#include "compiler.h"
+#include "config-host.h"
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <sys/mman.h>
+
+#ifndef O_LARGEFILE
+#define O_LARGEFILE 0
+#endif
+#ifndef O_BINARY
+#define O_BINARY 0
+#endif
+#ifndef MAP_ANONYMOUS
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+#ifndef ENOMEDIUM
+#define ENOMEDIUM ENODEV
+#endif
+#if !defined(ENOTSUP)
+#define ENOTSUP 4096
+#endif
+#if !defined(ECANCELED)
+#define ECANCELED 4097
+#endif
+#ifndef TIME_MAX
+#define TIME_MAX LONG_MAX
+#endif
+
+#ifndef CONFIG_IOVEC
+#define CONFIG_IOVEC
+struct iovec {
+    void *iov_base;
+    size_t iov_len;
+};
+/*
+ * Use the same value as Linux for now.
+ */
+#define IOV_MAX     1024
+#else
+#include <sys/uio.h>
+#endif
+
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
+#ifdef _WIN32
+#define fsync _commit
+#if !defined(lseek)
+# define lseek _lseeki64
+#endif
+int qemu_ftruncate64(int, int64_t);
+#if !defined(ftruncate)
+# define ftruncate qemu_ftruncate64
+#endif
+
+static inline char *realpath(const char *path, char *resolved_path)
+{
+    _fullpath(resolved_path, path, _MAX_PATH);
+    return resolved_path;
+}
+#endif
+
+int qemu_open(const char *name, int flags, ...);
+int qemu_close(int fd);
+
+#endif /* QEMU_STDIO_H */
-- 
1.7.11.7

[Qemu-devel] [PATCH 02/27] cpus.h: include qemu-stdio.h

[Eduardo Habkost]

Needed for the definition of fprint_function.

This is not necessary right now, but it will be necessary if code that
doesn't include cpu-common.h includes cpus.h.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 cpus.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cpus.h b/cpus.h
index 81bd817..b7c3708 100644
--- a/cpus.h
+++ b/cpus.h
@@ -1,6 +1,8 @@
 #ifndef QEMU_CPUS_H
 #define QEMU_CPUS_H
 
+#include "qemu-stdio.h"
+
 /* cpus.c */
 void qemu_init_cpu_loop(void);
 void resume_all_vcpus(void);
-- 
1.7.11.7

[Qemu-devel] [PATCH 03/27] hw/apic.c: rename bit functions to not conflict with bitops.h

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Coding style change: break too-long line
---
 hw/apic.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 49f0015..1772f2c 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -51,7 +51,7 @@ static int ffs_bit(uint32_t value)
     return ctz32(value);
 }
 
-static inline void set_bit(uint32_t *tab, int index)
+static inline void apic_set_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -59,7 +59,7 @@ static inline void set_bit(uint32_t *tab, int index)
     tab[i] |= mask;
 }
 
-static inline void reset_bit(uint32_t *tab, int index)
+static inline void apic_reset_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -67,7 +67,7 @@ static inline void reset_bit(uint32_t *tab, int index)
     tab[i] &= ~mask;
 }
 
-static inline int get_bit(uint32_t *tab, int index)
+static inline int apic_get_bit(uint32_t *tab, int index)
 {
     int i, mask;
     i = index >> 5;
@@ -184,7 +184,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
         case APIC_DM_FIXED:
             if (!(lvt & APIC_LVT_LEVEL_TRIGGER))
                 break;
-            reset_bit(s->irr, lvt & 0xff);
+            apic_reset_bit(s->irr, lvt & 0xff);
             /* fall through */
         case APIC_DM_EXTINT:
             cpu_reset_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
@@ -379,13 +379,13 @@ void apic_poll_irq(DeviceState *d)
 
 static void apic_set_irq(APICCommonState *s, int vector_num, int trigger_mode)
 {
-    apic_report_irq_delivered(!get_bit(s->irr, vector_num));
+    apic_report_irq_delivered(!apic_get_bit(s->irr, vector_num));
 
-    set_bit(s->irr, vector_num);
+    apic_set_bit(s->irr, vector_num);
     if (trigger_mode)
-        set_bit(s->tmr, vector_num);
+        apic_set_bit(s->tmr, vector_num);
     else
-        reset_bit(s->tmr, vector_num);
+        apic_reset_bit(s->tmr, vector_num);
     if (s->vapic_paddr) {
         apic_sync_vapic(s, SYNC_ISR_IRR_TO_VAPIC);
         /*
@@ -405,8 +405,9 @@ static void apic_eoi(APICCommonState *s)
     isrv = get_highest_priority_int(s->isr);
     if (isrv < 0)
         return;
-    reset_bit(s->isr, isrv);
-    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) && get_bit(s->tmr, isrv)) {
+    apic_reset_bit(s->isr, isrv);
+    if (!(s->spurious_vec & APIC_SV_DIRECTED_IO) &&
+            apic_get_bit(s->tmr, isrv)) {
         ioapic_eoi_broadcast(isrv);
     }
     apic_sync_vapic(s, SYNC_FROM_VAPIC | SYNC_TO_VAPIC);
@@ -445,7 +446,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             int idx = apic_find_dest(dest);
             memset(deliver_bitmask, 0x00, MAX_APIC_WORDS * sizeof(uint32_t));
             if (idx >= 0)
-                set_bit(deliver_bitmask, idx);
+                apic_set_bit(deliver_bitmask, idx);
         }
     } else {
         /* XXX: cluster mode */
@@ -455,11 +456,11 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
             if (apic_iter) {
                 if (apic_iter->dest_mode == 0xf) {
                     if (dest & apic_iter->log_dest)
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                 } else if (apic_iter->dest_mode == 0x0) {
                     if ((dest & 0xf0) == (apic_iter->log_dest & 0xf0) &&
                         (dest & apic_iter->log_dest & 0x0f)) {
-                        set_bit(deliver_bitmask, i);
+                        apic_set_bit(deliver_bitmask, i);
                     }
                 }
             } else {
@@ -502,14 +503,14 @@ static void apic_deliver(DeviceState *d, uint8_t dest, uint8_t dest_mode,
         break;
     case 1:
         memset(deliver_bitmask, 0x00, sizeof(deliver_bitmask));
-        set_bit(deliver_bitmask, s->idx);
+        apic_set_bit(deliver_bitmask, s->idx);
         break;
     case 2:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
         break;
     case 3:
         memset(deliver_bitmask, 0xff, sizeof(deliver_bitmask));
-        reset_bit(deliver_bitmask, s->idx);
+        apic_reset_bit(deliver_bitmask, s->idx);
         break;
     }
 
@@ -566,8 +567,8 @@ int apic_get_interrupt(DeviceState *d)
         apic_sync_vapic(s, SYNC_TO_VAPIC);
         return s->spurious_vec & 0xff;
     }
-    reset_bit(s->irr, intno);
-    set_bit(s->isr, intno);
+    apic_reset_bit(s->irr, intno);
+    apic_set_bit(s->isr, intno);
     apic_sync_vapic(s, SYNC_TO_VAPIC);
 
     /* re-inject if there is still a pending PIC interrupt */
-- 
1.7.11.7

[Qemu-devel] [PATCH 04/27] target-i386: initialize APIC at CPU level

[Eduardo Habkost]

From: Igor Mammedov <imammedo@redhat.com>

(L)APIC is a part of cpu [1] so move APIC initialization inside of
x86_cpu object. Since cpu_model and override flags currently specify
whether APIC should be created or not, APIC creation&initialization is
moved into x86_cpu_apic_init() which is called from x86_cpu_realize().

[1] - all x86 cpus have integrated APIC if we overlook existence of i486,
and it's more convenient to model after majority of them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pc.c           | 56 +++++-------------------------------------------------
 target-i386/cpu.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 51 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 16de04c..408823b 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -71,8 +71,6 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
-#define MSI_ADDR_BASE 0xfee00000
-
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -846,35 +844,6 @@ DeviceState *cpu_get_current_apic(void)
     }
 }
 
-static DeviceState *apic_init(void *env, uint8_t apic_id)
-{
-    DeviceState *dev;
-    static int apic_mapped;
-
-    if (kvm_irqchip_in_kernel()) {
-        dev = qdev_create(NULL, "kvm-apic");
-    } else if (xen_enabled()) {
-        dev = qdev_create(NULL, "xen-apic");
-    } else {
-        dev = qdev_create(NULL, "apic");
-    }
-
-    qdev_prop_set_uint8(dev, "id", apic_id);
-    qdev_prop_set_ptr(dev, "cpu_env", env);
-    qdev_init_nofail(dev);
-
-    /* XXX: mapping more APICs at the same memory location */
-    if (apic_mapped == 0) {
-        /* NOTE: the APIC is directly connected to the CPU - it is not
-           on the global memory bus. */
-        /* XXX: what if the base changes? */
-        sysbus_mmio_map(sysbus_from_qdev(dev), 0, MSI_ADDR_BASE);
-        apic_mapped = 1;
-    }
-
-    return dev;
-}
-
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 {
     CPUX86State *s = opaque;
@@ -884,24 +853,6 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model)
-{
-    X86CPU *cpu;
-    CPUX86State *env;
-
-    cpu = cpu_x86_init(cpu_model);
-    if (cpu == NULL) {
-        fprintf(stderr, "Unable to find x86 CPU definition\n");
-        exit(1);
-    }
-    env = &cpu->env;
-    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
-        env->apic_state = apic_init(env, env->cpuid_apic_id);
-    }
-    cpu_reset(CPU(cpu));
-    return cpu;
-}
-
 void pc_cpus_init(const char *cpu_model)
 {
     int i;
@@ -915,8 +866,11 @@ void pc_cpus_init(const char *cpu_model)
 #endif
     }
 
-    for(i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(cpu_model);
+    for (i = 0; i < smp_cpus; i++) {
+        if (!cpu_x86_init(cpu_model)) {
+            fprintf(stderr, "Unable to find x86 CPU definition\n");
+            exit(1);
+        }
     }
 }
 
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f3708e6..b5cf37d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -37,6 +37,12 @@
 #include <linux/kvm_para.h>
 #endif
 
+#include "sysemu.h"
+#ifndef CONFIG_USER_ONLY
+#include "hw/xen.h"
+#include "hw/sysbus.h"
+#endif
+
 /* feature flags taken from "Intel Processor Identification and the CPUID
  * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
  * between feature naming conventions, aliases may be added.
@@ -1870,12 +1876,63 @@ static void mce_init(X86CPU *cpu)
     }
 }
 
+#define MSI_ADDR_BASE 0xfee00000
+
+#ifndef CONFIG_USER_ONLY
+static void x86_cpu_apic_init(X86CPU *cpu, Error **errp)
+{
+    static int apic_mapped;
+    CPUX86State *env = &cpu->env;
+    const char *apic_type = "apic";
+
+    if (kvm_irqchip_in_kernel()) {
+        apic_type = "kvm-apic";
+    } else if (xen_enabled()) {
+        apic_type = "xen-apic";
+    }
+
+    env->apic_state = qdev_try_create(NULL, apic_type);
+    if (env->apic_state == NULL) {
+        error_setg(errp, "APIC device '%s' could not be created", apic_type);
+        return;
+    }
+
+    object_property_add_child(OBJECT(cpu), "apic",
+                              OBJECT(env->apic_state), NULL);
+    qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
+    /* TODO: convert to link<> */
+    qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
+
+    if (qdev_init(env->apic_state)) {
+        error_setg(errp, "APIC device '%s' could not be initialized",
+                   object_get_typename(OBJECT(env->apic_state)));
+        return;
+    }
+
+    /* XXX: mapping more APICs at the same memory location */
+    if (apic_mapped == 0) {
+        /* NOTE: the APIC is directly connected to the CPU - it is not
+           on the global memory bus. */
+        /* XXX: what if the base changes? */
+        sysbus_mmio_map(sysbus_from_qdev(env->apic_state), 0, MSI_ADDR_BASE);
+        apic_mapped = 1;
+    }
+}
+#endif
+
 void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
 
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
+
+    if (cpu->env.cpuid_features & CPUID_APIC || smp_cpus > 1) {
+        x86_cpu_apic_init(cpu, errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
 #endif
 
     mce_init(cpu);
-- 
1.7.11.7

[Qemu-devel] [PATCH 05/27] kvm: create kvm_arch_vcpu_id() function

[Eduardo Habkost]

This will allow each architecture to define how the VCPU ID is set on
the KVM_CREATE_VCPU ioctl call.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 kvm-all.c          | 2 +-
 kvm.h              | 3 +++
 target-i386/kvm.c  | 5 +++++
 target-ppc/kvm.c   | 5 +++++
 target-s390x/kvm.c | 5 +++++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/kvm-all.c b/kvm-all.c
index 961e1db..2eedf9a 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUArchState *env)
 
     DPRINTF("kvm_init_vcpu\n");
 
-    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
+    ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(env));
     if (ret < 0) {
         DPRINTF("kvm_create_vcpu failed\n");
         goto err;
diff --git a/kvm.h b/kvm.h
index 2b26dcb..95f86a4 100644
--- a/kvm.h
+++ b/kvm.h
@@ -179,6 +179,9 @@ int kvm_arch_init(KVMState *s);
 
 int kvm_arch_init_vcpu(CPUArchState *env);
 
+/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
+unsigned long kvm_arch_vcpu_id(CPUArchState *env);
+
 void kvm_arch_reset_vcpu(CPUArchState *env);
 
 int kvm_arch_on_sigbus_vcpu(CPUArchState *env, int code, void *addr);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3aa62b2..6e82125 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -353,6 +353,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
     }
 }
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUX86State *env)
 {
     struct {
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 5cbe98a..76b3fd0 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -373,6 +373,11 @@ static inline void kvm_fixup_page_sizes(CPUPPCState *env)
 
 #endif /* !defined (TARGET_PPC64) */
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUPPCState *cenv)
 {
     int ret;
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 07edf93..9e4c6c6 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
     return 0;
 }
 
+unsigned long kvm_arch_vcpu_id(CPUArchState *env)
+{
+    return env->cpu_index;
+}
+
 int kvm_arch_init_vcpu(CPUS390XState *env)
 {
     int ret = 0;
-- 
1.7.11.7

[Qemu-devel] [PATCH 06/27] target-i386: kvm: set vcpu_id to APIC ID instead of CPU index

[Eduardo Habkost]

The CPU ID in KVM is supposed to be the APIC ID, so change the
KVM_CREATE_VCPU call to match it. The current behavior didn't break
anything yet because today the APIC ID is assumed to be == the CPU
index, but this won't be true in the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Change only i386 code (kvm_arch_vcpu_id())
---
 target-i386/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6e82125..a0880ca 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -355,7 +355,7 @@ static void cpu_update_state(void *opaque, int running, RunState state)
 
 unsigned long kvm_arch_vcpu_id(CPUArchState *env)
 {
-    return env->cpu_index;
+    return env->cpuid_apic_id;
 }
 
 int kvm_arch_init_vcpu(CPUX86State *env)
-- 
1.7.11.7

[Qemu-devel] [PATCH 07/27] pc: pc_init1(): always use rom_memory on pc_memory_init() call

[Eduardo Habkost]

When pci_enabled is false, rom_memory is already pointing to
system_memory, so there's no need to check pci_enabled again when
calling pc_memory_init().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 47ebc1a..3c4ce8f 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -177,7 +177,7 @@ static void pc_init1(MemoryRegion *system_memory,
         fw_cfg = pc_memory_init(system_memory,
                        kernel_filename, kernel_cmdline, initrd_filename,
                        below_4g_mem_size, above_4g_mem_size,
-                       pci_enabled ? rom_memory : system_memory, &ram_memory);
+                       rom_memory, &ram_memory);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
-- 
1.7.11.7

[Qemu-devel] [PATCH 08/27] pc: pc_init1(): remove MemoryRegion arguments

[Eduardo Habkost]

All calls to pc_init1() pass the same expressions as arguments, so move the
expressions inside pc_init1().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3c4ce8f..5093ce5 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -118,9 +118,7 @@ static void ioapic_init(GSIState *gsi_state)
 }
 
 /* PC hardware initialisation */
-static void pc_init1(MemoryRegion *system_memory,
-                     MemoryRegion *system_io,
-                     ram_addr_t ram_size,
+static void pc_init1(ram_addr_t ram_size,
                      const char *boot_device,
                      const char *kernel_filename,
                      const char *kernel_cmdline,
@@ -147,6 +145,8 @@ static void pc_init1(MemoryRegion *system_memory,
     MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
     MemoryRegion *rom_memory;
+    MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *system_io = get_system_io();
     void *fw_cfg = NULL;
 
     pc_cpus_init(cpu_model);
@@ -295,9 +295,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
+    pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
              initrd_filename, cpu_model, 1, 1);
 }
@@ -310,9 +308,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
     const char *boot_device = args->boot_device;
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
+    pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
              initrd_filename, cpu_model, 1, 0);
 }
@@ -327,9 +323,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
     const char *boot_device = args->boot_device;
     if (cpu_model == NULL)
         cpu_model = "486";
-    pc_init1(get_system_memory(),
-             get_system_io(),
-             ram_size, boot_device,
+    pc_init1(ram_size, boot_device,
              kernel_filename, kernel_cmdline,
              initrd_filename, cpu_model, 0, 1);
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 09/27] pc: pc_init1(): get QEMUMachineInitArgs argument

[Eduardo Habkost]

Instead of passing each QEMUMachineInitArgs field individually, just
pass the whole QEMUMachineInitArgs object to pc_init1().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc_piix.c | 47 ++++++++++++-----------------------------------
 1 file changed, 12 insertions(+), 35 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 5093ce5..3eaed60 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -118,12 +118,7 @@ static void ioapic_init(GSIState *gsi_state)
 }
 
 /* PC hardware initialisation */
-static void pc_init1(ram_addr_t ram_size,
-                     const char *boot_device,
-                     const char *kernel_filename,
-                     const char *kernel_cmdline,
-                     const char *initrd_filename,
-                     const char *cpu_model,
+static void pc_init1(QEMUMachineInitArgs *args,
                      int pci_enabled,
                      int kvmclock_enabled)
 {
@@ -148,6 +143,12 @@ static void pc_init1(ram_addr_t ram_size,
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     void *fw_cfg = NULL;
+    ram_addr_t ram_size = args->ram_size;
+    const char *cpu_model = args->cpu_model;
+    const char *kernel_filename = args->kernel_filename;
+    const char *kernel_cmdline = args->kernel_cmdline;
+    const char *initrd_filename = args->initrd_filename;
+    const char *boot_device = args->boot_device;
 
     pc_cpus_init(cpu_model);
 
@@ -289,43 +290,19 @@ static void pc_init1(ram_addr_t ram_size,
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
-    pc_init1(ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 1);
+    pc_init1(args, 1, 1);
 }
 
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
-    pc_init1(ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 1, 0);
+    pc_init1(args, 1, 0);
 }
 
 static void pc_init_isa(QEMUMachineInitArgs *args)
 {
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
-    if (cpu_model == NULL)
-        cpu_model = "486";
-    pc_init1(ram_size, boot_device,
-             kernel_filename, kernel_cmdline,
-             initrd_filename, cpu_model, 0, 1);
+    if (args->cpu_model == NULL)
+        args->cpu_model = "486";
+    pc_init1(args, 0, 1);
 }
 
 #ifdef CONFIG_XEN
-- 
1.7.11.7

[Qemu-devel] [PATCH 10/27] pc: create PCInitArgs struct

[Eduardo Habkost]

Instead of changing pc_init1() arguments every time some additional
machine-type-specific behavior has to be introduced, create a struct that will
carry all the information needed by the PC initialization functions.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.h      |  8 ++++++++
 hw/pc_piix.c | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/hw/pc.h b/hw/pc.h
index e7993ca..04051ca 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -9,6 +9,7 @@
 #include "net.h"
 #include "memory.h"
 #include "ioapic.h"
+#include "boards.h"
 
 /* PC-style peripherals (also used by other machines).  */
 
@@ -75,6 +76,13 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
 /* pc.c */
 extern int fd_bootchk;
 
+/* Initialization parameters for the PC init functions */
+typedef struct PCInitArgs {
+    QEMUMachineInitArgs *qemu_args;
+    bool pci_enabled;
+    bool kvmclock_enabled;
+} PCInitArgs;
+
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 3eaed60..058fd43 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -118,9 +118,7 @@ static void ioapic_init(GSIState *gsi_state)
 }
 
 /* PC hardware initialisation */
-static void pc_init1(QEMUMachineInitArgs *args,
-                     int pci_enabled,
-                     int kvmclock_enabled)
+static void pc_init1(PCInitArgs *pc_args)
 {
     int i;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
@@ -143,16 +141,18 @@ static void pc_init1(QEMUMachineInitArgs *args,
     MemoryRegion *system_memory = get_system_memory();
     MemoryRegion *system_io = get_system_io();
     void *fw_cfg = NULL;
-    ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
-    const char *kernel_filename = args->kernel_filename;
-    const char *kernel_cmdline = args->kernel_cmdline;
-    const char *initrd_filename = args->initrd_filename;
-    const char *boot_device = args->boot_device;
+    QEMUMachineInitArgs *qemu_args = pc_args->qemu_args;
+    ram_addr_t ram_size = qemu_args->ram_size;
+    const char *cpu_model = qemu_args->cpu_model;
+    const char *kernel_filename = qemu_args->kernel_filename;
+    const char *kernel_cmdline = qemu_args->kernel_cmdline;
+    const char *initrd_filename = qemu_args->initrd_filename;
+    const char *boot_device = qemu_args->boot_device;
+    bool pci_enabled = pc_args->pci_enabled;
 
     pc_cpus_init(cpu_model);
 
-    if (kvmclock_enabled) {
+    if (pc_args->kvmclock_enabled) {
         kvmclock_create();
     }
 
@@ -290,19 +290,34 @@ static void pc_init1(QEMUMachineInitArgs *args,
 
 static void pc_init_pci(QEMUMachineInitArgs *args)
 {
-    pc_init1(args, 1, 1);
+    PCInitArgs pc_args = {
+        .qemu_args = args,
+        .pci_enabled = true,
+        .kvmclock_enabled = true,
+    };
+    pc_init1(&pc_args);
 }
 
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
-    pc_init1(args, 1, 0);
+    PCInitArgs pc_args = {
+        .qemu_args = args,
+        .pci_enabled = true,
+        .kvmclock_enabled = false,
+    };
+    pc_init1(&pc_args);
 }
 
 static void pc_init_isa(QEMUMachineInitArgs *args)
 {
+    PCInitArgs pc_args = {
+        .qemu_args = args,
+        .pci_enabled = false,
+        .kvmclock_enabled = false,
+    };
     if (args->cpu_model == NULL)
         args->cpu_model = "486";
-    pc_init1(args, 0, 1);
+    pc_init1(&pc_args);
 }
 
 #ifdef CONFIG_XEN
-- 
1.7.11.7

[Qemu-devel] [PATCH 11/27] pc: add PC_DEFAULT_CPU_MODEL #define

[Eduardo Habkost]

Make the pc_cpus_init() code more clear, by removing an #ifdef from
the middle of the code.

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

diff --git a/hw/pc.c b/hw/pc.c
index 408823b..67fdbe2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -71,6 +71,12 @@
 #define FW_CFG_E820_TABLE (FW_CFG_ARCH_LOCAL + 3)
 #define FW_CFG_HPET (FW_CFG_ARCH_LOCAL + 4)
 
+#ifdef TARGET_X86_64
+#define PC_DEFAULT_CPU_MODEL "qemu64"
+#else
+#define PC_DEFAULT_CPU_MODEL "qemu32"
+#endif
+
 #define E820_NR_ENTRIES		16
 
 struct e820_entry {
@@ -859,11 +865,7 @@ void pc_cpus_init(const char *cpu_model)
 
     /* init CPUs */
     if (cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
+        cpu_model = PC_DEFAULT_CPU_MODEL;
     }
 
     for (i = 0; i < smp_cpus; i++) {
-- 
1.7.11.7

[Qemu-devel] [PATCH 12/27] pc: add PCInitArgs parameter to pc_cpus_init()

[Eduardo Habkost]

This will make the code simpler when the CPU initialization code start using
additional initialization parameters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c      | 8 ++++----
 hw/pc.h      | 2 +-
 hw/pc_piix.c | 3 +--
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 67fdbe2..f92c19f 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -859,17 +859,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-void pc_cpus_init(const char *cpu_model)
+void pc_cpus_init(PCInitArgs *args)
 {
     int i;
 
     /* init CPUs */
-    if (cpu_model == NULL) {
-        cpu_model = PC_DEFAULT_CPU_MODEL;
+    if (args->qemu_args->cpu_model == NULL) {
+        args->qemu_args->cpu_model = PC_DEFAULT_CPU_MODEL;
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        if (!cpu_x86_init(cpu_model)) {
+        if (!cpu_x86_init(args->qemu_args->cpu_model)) {
             fprintf(stderr, "Unable to find x86 CPU definition\n");
             exit(1);
         }
diff --git a/hw/pc.h b/hw/pc.h
index 04051ca..26388ba 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -86,7 +86,7 @@ typedef struct PCInitArgs {
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-void pc_cpus_init(const char *cpu_model);
+void pc_cpus_init(PCInitArgs *args);
 void *pc_memory_init(MemoryRegion *system_memory,
                     const char *kernel_filename,
                     const char *kernel_cmdline,
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 058fd43..57d0c3b 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -143,14 +143,13 @@ static void pc_init1(PCInitArgs *pc_args)
     void *fw_cfg = NULL;
     QEMUMachineInitArgs *qemu_args = pc_args->qemu_args;
     ram_addr_t ram_size = qemu_args->ram_size;
-    const char *cpu_model = qemu_args->cpu_model;
     const char *kernel_filename = qemu_args->kernel_filename;
     const char *kernel_cmdline = qemu_args->kernel_cmdline;
     const char *initrd_filename = qemu_args->initrd_filename;
     const char *boot_device = qemu_args->boot_device;
     bool pci_enabled = pc_args->pci_enabled;
 
-    pc_cpus_init(cpu_model);
+    pc_cpus_init(pc_args);
 
     if (pc_args->kvmclock_enabled) {
         kvmclock_create();
-- 
1.7.11.7

[Qemu-devel] [PATCH 13/27] pc: pass PCInitArgs struct to pc_memory_init()

[Eduardo Habkost]

Instead of having a large list of arguments, just pass the PCInitArgs
struct to pc_memory_init().

This is being done mainly to facilitate the addition of an argument to
be used by bochs_bios_init() (enabling compatibility mode for APIC ID
generation).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c      | 34 +++++++++++++++-------------------
 hw/pc.h      | 16 ++++++++--------
 hw/pc_piix.c | 45 +++++++++++++++++++--------------------------
 3 files changed, 42 insertions(+), 53 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index f92c19f..dce9ce1 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -876,21 +876,14 @@ void pc_cpus_init(PCInitArgs *args)
     }
 }
 
-void *pc_memory_init(MemoryRegion *system_memory,
-                    const char *kernel_filename,
-                    const char *kernel_cmdline,
-                    const char *initrd_filename,
-                    ram_addr_t below_4g_mem_size,
-                    ram_addr_t above_4g_mem_size,
-                    MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory)
+void *pc_memory_init(PCInitArgs *args)
 {
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
     void *fw_cfg;
 
-    linux_boot = (kernel_filename != NULL);
+    linux_boot = (args->qemu_args->kernel_filename != NULL);
 
     /* Allocate RAM.  We allocate it as a single memory region and use
      * aliases to address portions of it, mostly for backwards compatibility
@@ -898,29 +891,29 @@ void *pc_memory_init(MemoryRegion *system_memory,
      */
     ram = g_malloc(sizeof(*ram));
     memory_region_init_ram(ram, "pc.ram",
-                           below_4g_mem_size + above_4g_mem_size);
+                           args->below_4g_mem_size + args->above_4g_mem_size);
     vmstate_register_ram_global(ram);
-    *ram_memory = ram;
+    args->ram_memory = ram;
     ram_below_4g = g_malloc(sizeof(*ram_below_4g));
     memory_region_init_alias(ram_below_4g, "ram-below-4g", ram,
-                             0, below_4g_mem_size);
-    memory_region_add_subregion(system_memory, 0, ram_below_4g);
-    if (above_4g_mem_size > 0) {
+                             0, args->below_4g_mem_size);
+    memory_region_add_subregion(args->system_memory, 0, ram_below_4g);
+    if (args->above_4g_mem_size > 0) {
         ram_above_4g = g_malloc(sizeof(*ram_above_4g));
         memory_region_init_alias(ram_above_4g, "ram-above-4g", ram,
-                                 below_4g_mem_size, above_4g_mem_size);
-        memory_region_add_subregion(system_memory, 0x100000000ULL,
+                                 args->below_4g_mem_size, args->above_4g_mem_size);
+        memory_region_add_subregion(args->system_memory, 0x100000000ULL,
                                     ram_above_4g);
     }
 
 
     /* Initialize PC system firmware */
-    pc_system_firmware_init(rom_memory);
+    pc_system_firmware_init(args->rom_memory);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
     memory_region_init_ram(option_rom_mr, "pc.rom", PC_ROM_SIZE);
     vmstate_register_ram_global(option_rom_mr);
-    memory_region_add_subregion_overlap(rom_memory,
+    memory_region_add_subregion_overlap(args->rom_memory,
                                         PC_ROM_MIN_VGA,
                                         option_rom_mr,
                                         1);
@@ -929,7 +922,10 @@ void *pc_memory_init(MemoryRegion *system_memory,
     rom_set_fw(fw_cfg);
 
     if (linux_boot) {
-        load_linux(fw_cfg, kernel_filename, initrd_filename, kernel_cmdline, below_4g_mem_size);
+        load_linux(fw_cfg, args->qemu_args->kernel_filename,
+                   args->qemu_args->initrd_filename,
+                   args->qemu_args->kernel_cmdline,
+                   args->below_4g_mem_size);
     }
 
     for (i = 0; i < nb_option_roms; i++) {
diff --git a/hw/pc.h b/hw/pc.h
index 26388ba..53883f5 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -81,20 +81,20 @@ typedef struct PCInitArgs {
     QEMUMachineInitArgs *qemu_args;
     bool pci_enabled;
     bool kvmclock_enabled;
+
+    /* Memory regions & sizes: */
+    MemoryRegion *system_memory;
+    MemoryRegion *system_io;
+    MemoryRegion *rom_memory;
+    MemoryRegion *ram_memory;
+    ram_addr_t below_4g_mem_size, above_4g_mem_size;
 } PCInitArgs;
 
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
 void pc_cpus_init(PCInitArgs *args);
-void *pc_memory_init(MemoryRegion *system_memory,
-                    const char *kernel_filename,
-                    const char *kernel_cmdline,
-                    const char *initrd_filename,
-                    ram_addr_t below_4g_mem_size,
-                    ram_addr_t above_4g_mem_size,
-                    MemoryRegion *rom_memory,
-                    MemoryRegion **ram_memory);
+void *pc_memory_init(PCInitArgs *args);
 qemu_irq *pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 57d0c3b..7de8f0d 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -121,7 +121,6 @@ static void ioapic_init(GSIState *gsi_state)
 static void pc_init1(PCInitArgs *pc_args)
 {
     int i;
-    ram_addr_t below_4g_mem_size, above_4g_mem_size;
     PCIBus *pci_bus;
     ISABus *isa_bus;
     PCII440FXState *i440fx_state;
@@ -135,20 +134,16 @@ static void pc_init1(PCInitArgs *pc_args)
     BusState *idebus[MAX_IDE_BUS];
     ISADevice *rtc_state;
     ISADevice *floppy;
-    MemoryRegion *ram_memory;
     MemoryRegion *pci_memory;
-    MemoryRegion *rom_memory;
-    MemoryRegion *system_memory = get_system_memory();
-    MemoryRegion *system_io = get_system_io();
     void *fw_cfg = NULL;
     QEMUMachineInitArgs *qemu_args = pc_args->qemu_args;
     ram_addr_t ram_size = qemu_args->ram_size;
-    const char *kernel_filename = qemu_args->kernel_filename;
-    const char *kernel_cmdline = qemu_args->kernel_cmdline;
-    const char *initrd_filename = qemu_args->initrd_filename;
     const char *boot_device = qemu_args->boot_device;
     bool pci_enabled = pc_args->pci_enabled;
 
+    pc_args->system_memory = get_system_memory();
+    pc_args->system_io = get_system_io();
+
     pc_cpus_init(pc_args);
 
     if (pc_args->kvmclock_enabled) {
@@ -156,28 +151,26 @@ static void pc_init1(PCInitArgs *pc_args)
     }
 
     if (ram_size >= 0xe0000000 ) {
-        above_4g_mem_size = ram_size - 0xe0000000;
-        below_4g_mem_size = 0xe0000000;
+        pc_args->above_4g_mem_size = ram_size - 0xe0000000;
+        pc_args->below_4g_mem_size = 0xe0000000;
     } else {
-        above_4g_mem_size = 0;
-        below_4g_mem_size = ram_size;
+        pc_args->above_4g_mem_size = 0;
+        pc_args->below_4g_mem_size = ram_size;
     }
 
     if (pci_enabled) {
         pci_memory = g_new(MemoryRegion, 1);
         memory_region_init(pci_memory, "pci", INT64_MAX);
-        rom_memory = pci_memory;
+        pc_args->rom_memory = pci_memory;
     } else {
         pci_memory = NULL;
-        rom_memory = system_memory;
+        pc_args->rom_memory = pc_args->system_memory;
     }
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
-        fw_cfg = pc_memory_init(system_memory,
-                       kernel_filename, kernel_cmdline, initrd_filename,
-                       below_4g_mem_size, above_4g_mem_size,
-                       rom_memory, &ram_memory);
+        /* pc_memory_init() will set pc_args->ram_memory */
+        fw_cfg = pc_memory_init(pc_args);
     }
 
     gsi_state = g_malloc0(sizeof(*gsi_state));
@@ -191,18 +184,18 @@ static void pc_init1(PCInitArgs *pc_args)
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, &isa_bus, gsi,
-                              system_memory, system_io, ram_size,
-                              below_4g_mem_size,
-                              0x100000000ULL - below_4g_mem_size,
-                              0x100000000ULL + above_4g_mem_size,
+                              pc_args->system_memory, pc_args->system_io,
+                              ram_size, pc_args->below_4g_mem_size,
+                              0x100000000ULL - pc_args->below_4g_mem_size,
+                              0x100000000ULL + pc_args->above_4g_mem_size,
                               (sizeof(hwaddr) == 4
                                ? 0
                                : ((uint64_t)1 << 62)),
-                              pci_memory, ram_memory);
+                              pci_memory, pc_args->ram_memory);
     } else {
         pci_bus = NULL;
         i440fx_state = NULL;
-        isa_bus = isa_bus_new(NULL, system_io);
+        isa_bus = isa_bus_new(NULL, pc_args->system_io);
         no_hpet = 1;
     }
     isa_bus_irqs(isa_bus, gsi);
@@ -264,8 +257,8 @@ static void pc_init1(PCInitArgs *pc_args)
 
     audio_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
-    pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
-                 floppy, idebus[0], idebus[1], rtc_state);
+    pc_cmos_init(pc_args->below_4g_mem_size, pc_args->above_4g_mem_size,
+                 boot_device, floppy, idebus[0], idebus[1], rtc_state);
 
     if (pci_enabled && usb_enabled) {
         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
-- 
1.7.11.7

[Qemu-devel] [PATCH 14/27] pc: use FWCfgState* instead of void* for fw_cfg data

[Eduardo Habkost]

I don't know why the code uses void* if the FWCfgState typedef is declared at
the same header where fw_cfg_init() and other fw_cfg_*() functions are
declared.  This changes the code to use FWCfgState* instead of void* in the PC
initialization code.

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

diff --git a/hw/pc.c b/hw/pc.c
index dce9ce1..4e971c8 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -570,9 +570,9 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
-static void *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(void)
 {
-    void *fw_cfg;
+    FWCfgState *fw_cfg;
     uint8_t *smbios_table;
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
@@ -638,7 +638,7 @@ static long get_file_size(FILE *f)
     return size;
 }
 
-static void load_linux(void *fw_cfg,
+static void load_linux(FWCfgState *fw_cfg,
                        const char *kernel_filename,
 		       const char *initrd_filename,
 		       const char *kernel_cmdline,
@@ -881,7 +881,7 @@ void *pc_memory_init(PCInitArgs *args)
     int linux_boot, i;
     MemoryRegion *ram, *option_rom_mr;
     MemoryRegion *ram_below_4g, *ram_above_4g;
-    void *fw_cfg;
+    FWCfgState *fw_cfg;
 
     linux_boot = (args->qemu_args->kernel_filename != NULL);
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 15/27] pc: rename bochs_bios_init() to pc_bios_init()

[Eduardo Habkost]

This code is not specific to the Bochs BIOS, so rename it.

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

diff --git a/hw/pc.c b/hw/pc.c
index 4e971c8..c38d8b2 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -570,7 +570,7 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *pc_bios_init(void)
 {
     FWCfgState *fw_cfg;
     uint8_t *smbios_table;
@@ -918,7 +918,7 @@ void *pc_memory_init(PCInitArgs *args)
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    fw_cfg = pc_bios_init();
     rom_set_fw(fw_cfg);
 
     if (linux_boot) {
-- 
1.7.11.7

[Qemu-devel] [PATCH 16/27] pc: pass PCInitArgs struct to pc_bios_init()

[Eduardo Habkost]

The argument will be used later, when enabling compatibility mode for
the APIC ID generation code.

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

diff --git a/hw/pc.c b/hw/pc.c
index c38d8b2..0e01003 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -570,7 +570,7 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
-static FWCfgState *pc_bios_init(void)
+static FWCfgState *pc_bios_init(PCInitArgs *args)
 {
     FWCfgState *fw_cfg;
     uint8_t *smbios_table;
@@ -918,7 +918,7 @@ void *pc_memory_init(PCInitArgs *args)
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = pc_bios_init();
+    fw_cfg = pc_bios_init(args);
     rom_set_fw(fw_cfg);
 
     if (linux_boot) {
-- 
1.7.11.7

[Qemu-devel] [PATCH 17/27] xen_machine_pv: use cpu_init() instead of cpu_x86_init()

[Eduardo Habkost]

cpu_init() will be kept as the simple-to-use init+realize function that some
code uses (e.g., *-user, and now xen_machine_pv too).

This will make cpu_x86_init() be used directly only by the more specialized PC
code, that will need to do additional initialization steps between the CPU
object creation and the x86_cpu_realize() call.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/xen_machine_pv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/xen_machine_pv.c b/hw/xen_machine_pv.c
index 4264703..95833f4 100644
--- a/hw/xen_machine_pv.c
+++ b/hw/xen_machine_pv.c
@@ -35,7 +35,6 @@ static void xen_init_pv(QEMUMachineInitArgs *args)
     const char *kernel_filename = args->kernel_filename;
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
-    X86CPU *cpu;
     CPUX86State *env;
     DriveInfo *dinfo;
     int i;
@@ -48,8 +47,7 @@ static void xen_init_pv(QEMUMachineInitArgs *args)
         cpu_model = "qemu32";
 #endif
     }
-    cpu = cpu_x86_init(cpu_model);
-    env = &cpu->env;
+    env = cpu_init(cpu_model);
     env->halted = 1;
 
     /* Initialize backend core & drivers */
-- 
1.7.11.7

[Qemu-devel] [PATCH 18/27] pc: isolate the code that create CPUs

[Eduardo Habkost]

The code will get more complex, so put it outside the for loop, into a
separate function.

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

diff --git a/hw/pc.c b/hw/pc.c
index 0e01003..85eab04 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -859,6 +859,14 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
+static void pc_cpu_init(PCInitArgs *args, int cpu_index)
+{
+    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
+        fprintf(stderr, "Unable to find x86 CPU definition\n");
+        exit(1);
+    }
+}
+
 void pc_cpus_init(PCInitArgs *args)
 {
     int i;
@@ -869,10 +877,7 @@ void pc_cpus_init(PCInitArgs *args)
     }
 
     for (i = 0; i < smp_cpus; i++) {
-        if (!cpu_x86_init(args->qemu_args->cpu_model)) {
-            fprintf(stderr, "Unable to find x86 CPU definition\n");
-            exit(1);
-        }
+        pc_cpu_init(args, i);
     }
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 19/27] cpu_x86_init: check for x86_cpu_realize() errors

[Eduardo Habkost]

If x86_cpu_realize() set any errors, print an error message and return
NULL.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/helper.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-i386/helper.c b/target-i386/helper.c
index c5d42c5..1e5f61f 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -19,6 +19,7 @@
 
 #include "cpu.h"
 #include "kvm.h"
+#include "qemu-error.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu.h"
 #include "monitor.h"
@@ -1243,6 +1244,7 @@ X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
+    Error *err = NULL;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
@@ -1253,7 +1255,11 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
 
-    x86_cpu_realize(OBJECT(cpu), NULL);
+    x86_cpu_realize(OBJECT(cpu), &err);
+    if (err) {
+        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
+        return NULL;
+    }
 
     return cpu;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Eduardo Habkost]

The PC code will need to run additional steps when initializing the CPU
object, before x86_cpu_realize(). So, make cpu_x86_init() not call
x86_cpu_realize(), and add two x86_cpu_realize() calls:

- One on cpu_init(), that is called only by *-user
- One on pc_cpu_init(), that will include the more advanced PC CPU
  initialization steps

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/pc.c              | 12 +++++++++++-
 target-i386/cpu.h    | 14 ++++++++++++++
 target-i386/helper.c | 11 ++++-------
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 85eab04..c209d3d 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
 
 static void pc_cpu_init(PCInitArgs *args, int cpu_index)
 {
-    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
+    Error *err = NULL;
+    X86CPU *cpu;
+
+    cpu = cpu_x86_init(args->qemu_args->cpu_model);
+    if (!cpu) {
         fprintf(stderr, "Unable to find x86 CPU definition\n");
         exit(1);
     }
+
+    x86_cpu_realize(OBJECT(cpu), &err);
+    if (err) {
+        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
+        exit(1);
+    }
 }
 
 void pc_cpus_init(PCInitArgs *args)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 871c270..6853b17 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -21,6 +21,7 @@
 
 #include "config.h"
 #include "qemu-common.h"
+#include "qemu-error.h"
 
 #ifdef TARGET_X86_64
 #define TARGET_LONG_BITS 64
@@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 #define TARGET_VIRT_ADDR_SPACE_BITS 32
 #endif
 
+/* Helper for simple CPU initialization (for target-independent code)
+ *
+ * Note that the PC code doesn't use this function, as it does additional
+ * initialization steps between cpu_x86_init() and cpu_x86_realize() is called.
+ */
 static inline CPUX86State *cpu_init(const char *cpu_model)
 {
+    Error *err = NULL;
     X86CPU *cpu = cpu_x86_init(cpu_model);
     if (cpu == NULL) {
         return NULL;
     }
+
+    x86_cpu_realize(OBJECT(cpu), &err);
+    if (err) {
+        error_report("cpu_init: %s\n", error_get_pretty(err));
+        return NULL;
+    }
+
     return &cpu->env;
 }
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 1e5f61f..87a9221 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
     return 1;
 }
 
+/* Initialize X86CPU object
+ *
+ * Callers must eventually call x86_cpu_realize(), to finish initialization.
+ */
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
     CPUX86State *env;
-    Error *err = NULL;
 
     cpu = X86_CPU(object_new(TYPE_X86_CPU));
     env = &cpu->env;
@@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
         return NULL;
     }
 
-    x86_cpu_realize(OBJECT(cpu), &err);
-    if (err) {
-        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
-        return NULL;
-    }
-
     return cpu;
 }
 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Igor Mammedov]

On Wed, 24 Oct 2012 15:49:54 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The PC code will need to run additional steps when initializing the CPU
> object, before x86_cpu_realize(). So, make cpu_x86_init() not call
Killing cpu_x86_init() altogether will make future re-factoring even easier.
For present its code could be duplicated in cpu_init() and pc.c,

and with cpu subclasses cpu_init () would be reduced to
  cpu = object_new(X86CPU.QEMUxx);
  cpu.realize();
and pc_cpus_init()
  cpu = object_new(X86CPU.QEMUxx);
  make cpu a child of /machine ();
  apply custom properties ();
  cpu.realize();

I don't see any benefits in keeping cpu_x86_init() around and if we start
touching it then just lets get rid of it in one step.

> x86_cpu_realize(), and add two x86_cpu_realize() calls:
> 
> - One on cpu_init(), that is called only by *-user
> - One on pc_cpu_init(), that will include the more advanced PC CPU
>   initialization steps
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/pc.c              | 12 +++++++++++-
>  target-i386/cpu.h    | 14 ++++++++++++++
>  target-i386/helper.c | 11 ++++-------
>  3 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 85eab04..c209d3d 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level) 
>  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
>  {
> -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
> +    Error *err = NULL;
> +    X86CPU *cpu;
> +
> +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
> +    if (!cpu) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
>      }
> +
> +    x86_cpu_realize(OBJECT(cpu), &err);
> +    if (err) {
> +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> +        exit(1);
> +    }
>  }
>  
>  void pc_cpus_init(PCInitArgs *args)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 871c270..6853b17 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -21,6 +21,7 @@
>  
>  #include "config.h"
>  #include "qemu-common.h"
> +#include "qemu-error.h"
>  
>  #ifdef TARGET_X86_64
>  #define TARGET_LONG_BITS 64
> @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  #define TARGET_VIRT_ADDR_SPACE_BITS 32
>  #endif
>  
> +/* Helper for simple CPU initialization (for target-independent code)
> + *
> + * Note that the PC code doesn't use this function, as it does additional
> + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
> called.
> + */
>  static inline CPUX86State *cpu_init(const char *cpu_model)
>  {
> +    Error *err = NULL;
>      X86CPU *cpu = cpu_x86_init(cpu_model);
>      if (cpu == NULL) {
>          return NULL;
>      }
> +
> +    x86_cpu_realize(OBJECT(cpu), &err);
> +    if (err) {
> +        error_report("cpu_init: %s\n", error_get_pretty(err));
> +        return NULL;
> +    }
> +
>      return &cpu->env;
>  }
>  
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 1e5f61f..87a9221 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> unsigned int selector, return 1;
>  }
>  
> +/* Initialize X86CPU object
> + *
> + * Callers must eventually call x86_cpu_realize(), to finish
> initialization.
> + */
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> -    Error *err = NULL;
>  
>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
>      env = &cpu->env;
> @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>          return NULL;
>      }
>  
> -    x86_cpu_realize(OBJECT(cpu), &err);
> -    if (err) {
> -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
> -        return NULL;
> -    }
> -
>      return cpu;
>  }
>  

Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Andreas Färber]

Am 31.10.2012 17:32, schrieb Igor Mammedov:
> On Wed, 24 Oct 2012 15:49:54 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> The PC code will need to run additional steps when initializing the CPU
>> object, before x86_cpu_realize(). So, make cpu_x86_init() not call
> Killing cpu_x86_init() altogether will make future re-factoring even easier.
> For present its code could be duplicated in cpu_init() and pc.c,
> 
> and with cpu subclasses cpu_init () would be reduced to
>   cpu = object_new(X86CPU.QEMUxx);
>   cpu.realize();
> and pc_cpus_init()
>   cpu = object_new(X86CPU.QEMUxx);
>   make cpu a child of /machine ();
>   apply custom properties ();
>   cpu.realize();
> 
> I don't see any benefits in keeping cpu_x86_init() around and if we start
> touching it then just lets get rid of it in one step.

To my regret, CPU subclasses have moved to the end of your two queues. I
was considering doing a proposal to fast-track that for symmetry with
the other targets and gradually improve that with the pending properties
series (that now depend on qdev, which I find rather nasty to review),
but I have my hands quite full currently, no promises...

Andreas

> 
>> x86_cpu_realize(), and add two x86_cpu_realize() calls:
>>
>> - One on cpu_init(), that is called only by *-user
>> - One on pc_cpu_init(), that will include the more advanced PC CPU
>>   initialization steps
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  hw/pc.c              | 12 +++++++++++-
>>  target-i386/cpu.h    | 14 ++++++++++++++
>>  target-i386/helper.c | 11 ++++-------
>>  3 files changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index 85eab04..c209d3d 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
>> level) 
>>  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
>>  {
>> -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
>> +    Error *err = NULL;
>> +    X86CPU *cpu;
>> +
>> +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
>> +    if (!cpu) {
>>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>>          exit(1);
>>      }
>> +
>> +    x86_cpu_realize(OBJECT(cpu), &err);
>> +    if (err) {
>> +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
>> +        exit(1);
>> +    }
>>  }
>>  
>>  void pc_cpus_init(PCInitArgs *args)
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 871c270..6853b17 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -21,6 +21,7 @@
>>  
>>  #include "config.h"
>>  #include "qemu-common.h"
>> +#include "qemu-error.h"
>>  
>>  #ifdef TARGET_X86_64
>>  #define TARGET_LONG_BITS 64
>> @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>>  #define TARGET_VIRT_ADDR_SPACE_BITS 32
>>  #endif
>>  
>> +/* Helper for simple CPU initialization (for target-independent code)
>> + *
>> + * Note that the PC code doesn't use this function, as it does additional
>> + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
>> called.
>> + */
>>  static inline CPUX86State *cpu_init(const char *cpu_model)
>>  {
>> +    Error *err = NULL;
>>      X86CPU *cpu = cpu_x86_init(cpu_model);
>>      if (cpu == NULL) {
>>          return NULL;
>>      }
>> +
>> +    x86_cpu_realize(OBJECT(cpu), &err);
>> +    if (err) {
>> +        error_report("cpu_init: %s\n", error_get_pretty(err));
>> +        return NULL;
>> +    }
>> +
>>      return &cpu->env;
>>  }
>>  
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 1e5f61f..87a9221 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
>> unsigned int selector, return 1;
>>  }
>>  
>> +/* Initialize X86CPU object
>> + *
>> + * Callers must eventually call x86_cpu_realize(), to finish
>> initialization.
>> + */
>>  X86CPU *cpu_x86_init(const char *cpu_model)
>>  {
>>      X86CPU *cpu;
>>      CPUX86State *env;
>> -    Error *err = NULL;
>>  
>>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
>>      env = &cpu->env;
>> @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
>>          return NULL;
>>      }
>>  
>> -    x86_cpu_realize(OBJECT(cpu), &err);
>> -    if (err) {
>> -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
>> -        return NULL;
>> -    }
>> -
>>      return cpu;
>>  }
>>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Eduardo Habkost]

On Wed, Oct 31, 2012 at 05:43:48PM +0100, Andreas Färber wrote:
> Am 31.10.2012 17:32, schrieb Igor Mammedov:
> > On Wed, 24 Oct 2012 15:49:54 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> >> The PC code will need to run additional steps when initializing the CPU
> >> object, before x86_cpu_realize(). So, make cpu_x86_init() not call
> > Killing cpu_x86_init() altogether will make future re-factoring even easier.
> > For present its code could be duplicated in cpu_init() and pc.c,
> > 
> > and with cpu subclasses cpu_init () would be reduced to
> >   cpu = object_new(X86CPU.QEMUxx);
> >   cpu.realize();
> > and pc_cpus_init()
> >   cpu = object_new(X86CPU.QEMUxx);
> >   make cpu a child of /machine ();
> >   apply custom properties ();
> >   cpu.realize();
> > 
> > I don't see any benefits in keeping cpu_x86_init() around and if we start
> > touching it then just lets get rid of it in one step.
> 
> To my regret, CPU subclasses have moved to the end of your two queues. I
> was considering doing a proposal to fast-track that for symmetry with
> the other targets and gradually improve that with the pending properties
> series (that now depend on qdev, which I find rather nasty to review),
> but I have my hands quite full currently, no promises...

I always planned to send the CPU classes code after the CPU properties
are in, but maybe we can reverse the order.

I am not sure if we have time for that. What would be a reasonable
deadline to have a CPU classes series submitted, so it would be
reasonable/feasible to review it and get it into 1.3? Yesterday?  ;-)

> 
> Andreas
> 
> > 
> >> x86_cpu_realize(), and add two x86_cpu_realize() calls:
> >>
> >> - One on cpu_init(), that is called only by *-user
> >> - One on pc_cpu_init(), that will include the more advanced PC CPU
> >>   initialization steps
> >>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  hw/pc.c              | 12 +++++++++++-
> >>  target-i386/cpu.h    | 14 ++++++++++++++
> >>  target-i386/helper.c | 11 ++++-------
> >>  3 files changed, 29 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/pc.c b/hw/pc.c
> >> index 85eab04..c209d3d 100644
> >> --- a/hw/pc.c
> >> +++ b/hw/pc.c
> >> @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> >> level) 
> >>  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
> >>  {
> >> -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
> >> +    Error *err = NULL;
> >> +    X86CPU *cpu;
> >> +
> >> +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
> >> +    if (!cpu) {
> >>          fprintf(stderr, "Unable to find x86 CPU definition\n");
> >>          exit(1);
> >>      }
> >> +
> >> +    x86_cpu_realize(OBJECT(cpu), &err);
> >> +    if (err) {
> >> +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> >> +        exit(1);
> >> +    }
> >>  }
> >>  
> >>  void pc_cpus_init(PCInitArgs *args)
> >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> >> index 871c270..6853b17 100644
> >> --- a/target-i386/cpu.h
> >> +++ b/target-i386/cpu.h
> >> @@ -21,6 +21,7 @@
> >>  
> >>  #include "config.h"
> >>  #include "qemu-common.h"
> >> +#include "qemu-error.h"
> >>  
> >>  #ifdef TARGET_X86_64
> >>  #define TARGET_LONG_BITS 64
> >> @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >>  #define TARGET_VIRT_ADDR_SPACE_BITS 32
> >>  #endif
> >>  
> >> +/* Helper for simple CPU initialization (for target-independent code)
> >> + *
> >> + * Note that the PC code doesn't use this function, as it does additional
> >> + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
> >> called.
> >> + */
> >>  static inline CPUX86State *cpu_init(const char *cpu_model)
> >>  {
> >> +    Error *err = NULL;
> >>      X86CPU *cpu = cpu_x86_init(cpu_model);
> >>      if (cpu == NULL) {
> >>          return NULL;
> >>      }
> >> +
> >> +    x86_cpu_realize(OBJECT(cpu), &err);
> >> +    if (err) {
> >> +        error_report("cpu_init: %s\n", error_get_pretty(err));
> >> +        return NULL;
> >> +    }
> >> +
> >>      return &cpu->env;
> >>  }
> >>  
> >> diff --git a/target-i386/helper.c b/target-i386/helper.c
> >> index 1e5f61f..87a9221 100644
> >> --- a/target-i386/helper.c
> >> +++ b/target-i386/helper.c
> >> @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> >> unsigned int selector, return 1;
> >>  }
> >>  
> >> +/* Initialize X86CPU object
> >> + *
> >> + * Callers must eventually call x86_cpu_realize(), to finish
> >> initialization.
> >> + */
> >>  X86CPU *cpu_x86_init(const char *cpu_model)
> >>  {
> >>      X86CPU *cpu;
> >>      CPUX86State *env;
> >> -    Error *err = NULL;
> >>  
> >>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >>      env = &cpu->env;
> >> @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >>          return NULL;
> >>      }
> >>  
> >> -    x86_cpu_realize(OBJECT(cpu), &err);
> >> -    if (err) {
> >> -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
> >> -        return NULL;
> >> -    }
> >> -
> >>      return cpu;
> >>  }
> >>  
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Igor Mammedov]

On Wed, 31 Oct 2012 17:43:48 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 31.10.2012 17:32, schrieb Igor Mammedov:
> > On Wed, 24 Oct 2012 15:49:54 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> >> The PC code will need to run additional steps when initializing the CPU
> >> object, before x86_cpu_realize(). So, make cpu_x86_init() not call
> > Killing cpu_x86_init() altogether will make future re-factoring even
> > easier. For present its code could be duplicated in cpu_init() and pc.c,
> > 
> > and with cpu subclasses cpu_init () would be reduced to
> >   cpu = object_new(X86CPU.QEMUxx);
> >   cpu.realize();
> > and pc_cpus_init()
> >   cpu = object_new(X86CPU.QEMUxx);
> >   make cpu a child of /machine ();
> >   apply custom properties ();
> >   cpu.realize();
> > 
> > I don't see any benefits in keeping cpu_x86_init() around and if we start
> > touching it then just lets get rid of it in one step.
> 
> To my regret, CPU subclasses have moved to the end of your two queues. I
> was considering doing a proposal to fast-track that for symmetry with
> the other targets and gradually improve that with the pending properties
I could revive not qdev based version of cpu properties so we could move
forward if Anthony won't object (he argued in favor of qdev based one on IRC). 
> series (that now depend on qdev, which I find rather nasty to review),
Is there anything that could be done to improve review-ability of cpu-as-qdev
> but I have my hands quite full currently, no promises...
> 
> Andreas
> 
> > 
> >> x86_cpu_realize(), and add two x86_cpu_realize() calls:
> >>
> >> - One on cpu_init(), that is called only by *-user
> >> - One on pc_cpu_init(), that will include the more advanced PC CPU
> >>   initialization steps
> >>
> >> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> ---
> >>  hw/pc.c              | 12 +++++++++++-
> >>  target-i386/cpu.h    | 14 ++++++++++++++
> >>  target-i386/helper.c | 11 ++++-------
> >>  3 files changed, 29 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/pc.c b/hw/pc.c
> >> index 85eab04..c209d3d 100644
> >> --- a/hw/pc.c
> >> +++ b/hw/pc.c
> >> @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq,
> >> int level) 
> >>  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
> >>  {
> >> -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
> >> +    Error *err = NULL;
> >> +    X86CPU *cpu;
> >> +
> >> +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
> >> +    if (!cpu) {
> >>          fprintf(stderr, "Unable to find x86 CPU definition\n");
> >>          exit(1);
> >>      }
> >> +
> >> +    x86_cpu_realize(OBJECT(cpu), &err);
> >> +    if (err) {
> >> +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> >> +        exit(1);
> >> +    }
> >>  }
> >>  
> >>  void pc_cpus_init(PCInitArgs *args)
> >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> >> index 871c270..6853b17 100644
> >> --- a/target-i386/cpu.h
> >> +++ b/target-i386/cpu.h
> >> @@ -21,6 +21,7 @@
> >>  
> >>  #include "config.h"
> >>  #include "qemu-common.h"
> >> +#include "qemu-error.h"
> >>  
> >>  #ifdef TARGET_X86_64
> >>  #define TARGET_LONG_BITS 64
> >> @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >>  #define TARGET_VIRT_ADDR_SPACE_BITS 32
> >>  #endif
> >>  
> >> +/* Helper for simple CPU initialization (for target-independent code)
> >> + *
> >> + * Note that the PC code doesn't use this function, as it does
> >> additional
> >> + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
> >> called.
> >> + */
> >>  static inline CPUX86State *cpu_init(const char *cpu_model)
> >>  {
> >> +    Error *err = NULL;
> >>      X86CPU *cpu = cpu_x86_init(cpu_model);
> >>      if (cpu == NULL) {
> >>          return NULL;
> >>      }
> >> +
> >> +    x86_cpu_realize(OBJECT(cpu), &err);
> >> +    if (err) {
> >> +        error_report("cpu_init: %s\n", error_get_pretty(err));
> >> +        return NULL;
> >> +    }
> >> +
> >>      return &cpu->env;
> >>  }
> >>  
> >> diff --git a/target-i386/helper.c b/target-i386/helper.c
> >> index 1e5f61f..87a9221 100644
> >> --- a/target-i386/helper.c
> >> +++ b/target-i386/helper.c
> >> @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> >> unsigned int selector, return 1;
> >>  }
> >>  
> >> +/* Initialize X86CPU object
> >> + *
> >> + * Callers must eventually call x86_cpu_realize(), to finish
> >> initialization.
> >> + */
> >>  X86CPU *cpu_x86_init(const char *cpu_model)
> >>  {
> >>      X86CPU *cpu;
> >>      CPUX86State *env;
> >> -    Error *err = NULL;
> >>  
> >>      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >>      env = &cpu->env;
> >> @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >>          return NULL;
> >>      }
> >>  
> >> -    x86_cpu_realize(OBJECT(cpu), &err);
> >> -    if (err) {
> >> -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
> >> -        return NULL;
> >> -    }
> >> -
> >>      return cpu;
> >>  }
> >>  
> > 
> 
> 

Re: [Qemu-devel] [PATCH 20/27] target-i386: do not call x86_cpu_realize() on cpu_x86_init()

[Eduardo Habkost]

On Wed, Oct 31, 2012 at 05:32:33PM +0100, Igor Mammedov wrote:
> On Wed, 24 Oct 2012 15:49:54 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > The PC code will need to run additional steps when initializing the CPU
> > object, before x86_cpu_realize(). So, make cpu_x86_init() not call
> Killing cpu_x86_init() altogether will make future re-factoring even easier.
> For present its code could be duplicated in cpu_init() and pc.c,
> 
> and with cpu subclasses cpu_init () would be reduced to
>   cpu = object_new(X86CPU.QEMUxx);
>   cpu.realize();

I suspect *-user supports +feature,-feature on the CPU model string as
well. So both cases the cpu_model compat string parsing/property-setting
and the CPU class lookup would be necessary. So cpu_init() wouldn't look
so simple as above.


> and pc_cpus_init()
>   cpu = object_new(X86CPU.QEMUxx);
>   make cpu a child of /machine ();
>   apply custom properties ();
>   cpu.realize();
> 
> I don't see any benefits in keeping cpu_x86_init() around and if we start
> touching it then just lets get rid of it in one step.

I believe the sequence that creates the CPU object will look like this
on *-user:

  compat_normalize_cpu_model(cpu_model, &class_name, &features);
  class = type_get_by_name(class_name);
  cpu = object_new(class);
  cpu_x86_set_props(cpu, features);
  cpu.realize();

And on PC it will look like:
  
  compat_normalize_cpu_model(cpu_model, &class_name, &features);
  class = type_get_by_name(class_name);
  cpu = object_new(class);
  cpu_x86_set_props(cpu, features);
  cpu_init_steps_sepcific_for_pc(cpu); /* APIC ID, make a child of
                                        * /machine, whatever else.
                                        */
  cpu.realize();

If cpu_init() was going to be just object_new() + cpu.realize(), I
wouldn't mind duplicating the code. But I don't see a reason to
duplicate code and not use a common function for the "cpu_model parsing
+ class lookup + object_new() + compat props setting" steps. I think
cpu_x86_init() can be that common function.


> 
> > x86_cpu_realize(), and add two x86_cpu_realize() calls:
> > 
> > - One on cpu_init(), that is called only by *-user
> > - One on pc_cpu_init(), that will include the more advanced PC CPU
> >   initialization steps
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/pc.c              | 12 +++++++++++-
> >  target-i386/cpu.h    | 14 ++++++++++++++
> >  target-i386/helper.c | 11 ++++-------
> >  3 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 85eab04..c209d3d 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -861,10 +861,20 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> > level) 
> >  static void pc_cpu_init(PCInitArgs *args, int cpu_index)
> >  {
> > -    if (!cpu_x86_init(args->qemu_args->cpu_model)) {
> > +    Error *err = NULL;
> > +    X86CPU *cpu;
> > +
> > +    cpu = cpu_x86_init(args->qemu_args->cpu_model);
> > +    if (!cpu) {
> >          fprintf(stderr, "Unable to find x86 CPU definition\n");
> >          exit(1);
> >      }
> > +
> > +    x86_cpu_realize(OBJECT(cpu), &err);
> > +    if (err) {
> > +        error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> > +        exit(1);
> > +    }
> >  }
> >  
> >  void pc_cpus_init(PCInitArgs *args)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 871c270..6853b17 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include "config.h"
> >  #include "qemu-common.h"
> > +#include "qemu-error.h"
> >  
> >  #ifdef TARGET_X86_64
> >  #define TARGET_LONG_BITS 64
> > @@ -1008,12 +1009,25 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >  #define TARGET_VIRT_ADDR_SPACE_BITS 32
> >  #endif
> >  
> > +/* Helper for simple CPU initialization (for target-independent code)
> > + *
> > + * Note that the PC code doesn't use this function, as it does additional
> > + * initialization steps between cpu_x86_init() and cpu_x86_realize() is
> > called.
> > + */
> >  static inline CPUX86State *cpu_init(const char *cpu_model)
> >  {
> > +    Error *err = NULL;
> >      X86CPU *cpu = cpu_x86_init(cpu_model);
> >      if (cpu == NULL) {
> >          return NULL;
> >      }
> > +
> > +    x86_cpu_realize(OBJECT(cpu), &err);
> > +    if (err) {
> > +        error_report("cpu_init: %s\n", error_get_pretty(err));
> > +        return NULL;
> > +    }
> > +
> >      return &cpu->env;
> >  }
> >  
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index 1e5f61f..87a9221 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1240,11 +1240,14 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> > unsigned int selector, return 1;
> >  }
> >  
> > +/* Initialize X86CPU object
> > + *
> > + * Callers must eventually call x86_cpu_realize(), to finish
> > initialization.
> > + */
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> >      X86CPU *cpu;
> >      CPUX86State *env;
> > -    Error *err = NULL;
> >  
> >      cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >      env = &cpu->env;
> > @@ -1255,12 +1258,6 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >          return NULL;
> >      }
> >  
> > -    x86_cpu_realize(OBJECT(cpu), &err);
> > -    if (err) {
> > -        error_report("cpu_x86_init: %s\n", error_get_pretty(err));
> > -        return NULL;
> > -    }
> > -
> >      return cpu;
> >  }
> >  
> 

-- 
Eduardo

[Qemu-devel] [PATCH 21/27] fw_cfg: remove FW_CFG_MAX_CPUS from fw_cfg_init()

[Eduardo Habkost]

PC will not use max_cpus for that field, so move it outside the common
code so it can use a different value on PC.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/fw_cfg.c       | 1 -
 hw/pc.c           | 2 +-
 hw/ppc_newworld.c | 1 +
 hw/ppc_oldworld.c | 1 +
 hw/sun4m.c        | 3 +++
 hw/sun4u.c        | 1 +
 6 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 2b92cda..52938fe 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -518,7 +518,6 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
     fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
     fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)(display_type == DT_NOGRAPHIC));
     fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
-    fw_cfg_add_i16(s, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i16(s, FW_CFG_BOOT_MENU, (uint16_t)boot_menu);
     fw_cfg_bootsplash(s);
     fw_cfg_reboot(s);
diff --git a/hw/pc.c b/hw/pc.c
index c209d3d..0e9a00f 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -585,7 +585,7 @@ static FWCfgState *pc_bios_init(PCInitArgs *args)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
-
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables,
diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
index 15f74f9..e4dfb4b 100644
--- a/hw/ppc_newworld.c
+++ b/hw/ppc_newworld.c
@@ -381,6 +381,7 @@ static void ppc_core99_init(QEMUMachineInitArgs *args)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, machine_arch);
diff --git a/hw/ppc_oldworld.c b/hw/ppc_oldworld.c
index a4f899d..43d2b0e 100644
--- a/hw/ppc_oldworld.c
+++ b/hw/ppc_oldworld.c
@@ -296,6 +296,7 @@ static void ppc_heathrow_init(QEMUMachineInitArgs *args)
     /* No PCI init: the BIOS will do it */
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
diff --git a/hw/sun4m.c b/hw/sun4m.c
index 02673b2..283a6fe 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -1021,6 +1021,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef, ram_addr_t RAM_size,
                  hwdef->ecc_version);
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1658,6 +1659,7 @@ static void sun4d_hw_init(const struct sun4d_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4d");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
@@ -1858,6 +1860,7 @@ static void sun4c_hw_init(const struct sun4c_hwdef *hwdef, ram_addr_t RAM_size,
                "Sun4c");
 
     fw_cfg = fw_cfg_init(0, 0, CFG_ADDR, CFG_ADDR + 2);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 1621171..9ab37f1 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -878,6 +878,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
                            (uint8_t *)&nd_table[0].macaddr);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, hwdef->machine_id);
-- 
1.7.11.7

[Qemu-devel] [PATCH 22/27] pc: set CPU APIC ID explicitly

[Eduardo Habkost]

The PC code takes care of CPU topology, and CPU topology affect the CPU
APIC ID. So the PC CPU initialization code needs to set the APIC ID
explicitly.

By now, keep the existing behavior but create a apic_id_for_cpu()
function that will be changed later to implement appropriate
topology-dependent behavior.

The cpuid_apic_id field is used only at:

 - x86_cpu_apic_init(), called from x86_cpu_realize()
 - kvm_init_vcpu(), that is called from the VCPU thread
   created by qemu_init_vcpu(), called by x86_cpu_realize()
 - helper_cpuid(), called only when the VCPU is already running
 - kvm_arch_init_vcpu(), that's called by kvm_init_vcpu()

So it's safe to change it before x86_cpu_realize() is called.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
This is based on the patch that I have originally suybmitted as:
 Subject: pc: create apic_id_for_cpu() function (v3)
---
 hw/pc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/pc.c b/hw/pc.c
index 0e9a00f..eb68851 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -553,6 +553,21 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+/* Calculates initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
+ * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
+ * all CPUs up to max_cpus.
+ */
+static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
+{
+    /* right now APIC ID == CPU index. this will eventually change to use
+     * the CPU topology configuration properly
+     */
+    return cpu_index;
+}
+
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
 {
     int index = le32_to_cpu(e820_table.count);
@@ -870,6 +885,13 @@ static void pc_cpu_init(PCInitArgs *args, int cpu_index)
         exit(1);
     }
 
+    /* Override the default APIC set by the X86CPU init function.
+     * We need to do that because:
+     * - The APIC ID depends on the CPU topology;
+     * - The exact APIC ID used may depend on the machine-type init arguments.
+     */
+    cpu->env.cpuid_apic_id = apic_id_for_cpu(args, cpu_index);
+
     x86_cpu_realize(OBJECT(cpu), &err);
     if (err) {
         error_report("pc_cpu_init: %s\n", error_get_pretty(err));
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 22/27] pc: set CPU APIC ID explicitly

[Igor Mammedov]

On Wed, 24 Oct 2012 15:49:56 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The PC code takes care of CPU topology, and CPU topology affect the CPU
> APIC ID. So the PC CPU initialization code needs to set the APIC ID
> explicitly.
> 
> By now, keep the existing behavior but create a apic_id_for_cpu()
> function that will be changed later to implement appropriate
> topology-dependent behavior.
> 
> The cpuid_apic_id field is used only at:
> 
>  - x86_cpu_apic_init(), called from x86_cpu_realize()
>  - kvm_init_vcpu(), that is called from the VCPU thread
>    created by qemu_init_vcpu(), called by x86_cpu_realize()
>  - helper_cpuid(), called only when the VCPU is already running
>  - kvm_arch_init_vcpu(), that's called by kvm_init_vcpu()
> 
> So it's safe to change it before x86_cpu_realize() is called.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> This is based on the patch that I have originally suybmitted as:
>  Subject: pc: create apic_id_for_cpu() function (v3)
> ---
>  hw/pc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 0e9a00f..eb68851 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -553,6 +553,21 @@ static void bochs_bios_write(void *opaque, uint32_t
> addr, uint32_t val) }
>  }
>  
> +/* Calculates initial APIC ID for a specific CPU index
> + *
> + * Currently we need to be able to calculate the APIC ID from the CPU index
> + * alone (without requiring a CPU object), as the QEMU<->Seabios
> interfaces have
> + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC
> ID of
> + * all CPUs up to max_cpus.
> + */
> +static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
> +{
> +    /* right now APIC ID == CPU index. this will eventually change to use
> +     * the CPU topology configuration properly
> +     */
> +    return cpu_index;
> +}
> +
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
>  {
>      int index = le32_to_cpu(e820_table.count);
> @@ -870,6 +885,13 @@ static void pc_cpu_init(PCInitArgs *args, int
> cpu_index) exit(1);
>      }
>  
> +    /* Override the default APIC set by the X86CPU init function.
> +     * We need to do that because:
> +     * - The APIC ID depends on the CPU topology;
> +     * - The exact APIC ID used may depend on the machine-type init
> arguments.
> +     */
> +    cpu->env.cpuid_apic_id = apic_id_for_cpu(args, cpu_index);
Could you create property setter for apic_id and use it here, please.

> +
>      x86_cpu_realize(OBJECT(cpu), &err);
>      if (err) {
>          error_report("pc_cpu_init: %s\n", error_get_pretty(err));

Re: [Qemu-devel] [PATCH 22/27] pc: set CPU APIC ID explicitly

[Eduardo Habkost]

On Thu, Nov 01, 2012 at 03:04:05PM +0100, Igor Mammedov wrote:
> On Wed, 24 Oct 2012 15:49:56 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > The PC code takes care of CPU topology, and CPU topology affect the CPU
> > APIC ID. So the PC CPU initialization code needs to set the APIC ID
> > explicitly.
> > 
> > By now, keep the existing behavior but create a apic_id_for_cpu()
> > function that will be changed later to implement appropriate
> > topology-dependent behavior.
> > 
> > The cpuid_apic_id field is used only at:
> > 
> >  - x86_cpu_apic_init(), called from x86_cpu_realize()
> >  - kvm_init_vcpu(), that is called from the VCPU thread
> >    created by qemu_init_vcpu(), called by x86_cpu_realize()
> >  - helper_cpuid(), called only when the VCPU is already running
> >  - kvm_arch_init_vcpu(), that's called by kvm_init_vcpu()
> > 
> > So it's safe to change it before x86_cpu_realize() is called.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > This is based on the patch that I have originally suybmitted as:
> >  Subject: pc: create apic_id_for_cpu() function (v3)
> > ---
> >  hw/pc.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 0e9a00f..eb68851 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -553,6 +553,21 @@ static void bochs_bios_write(void *opaque, uint32_t
> > addr, uint32_t val) }
> >  }
> >  
> > +/* Calculates initial APIC ID for a specific CPU index
> > + *
> > + * Currently we need to be able to calculate the APIC ID from the CPU index
> > + * alone (without requiring a CPU object), as the QEMU<->Seabios
> > interfaces have
> > + * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC
> > ID of
> > + * all CPUs up to max_cpus.
> > + */
> > +static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
> > +{
> > +    /* right now APIC ID == CPU index. this will eventually change to use
> > +     * the CPU topology configuration properly
> > +     */
> > +    return cpu_index;
> > +}
> > +
> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> >  {
> >      int index = le32_to_cpu(e820_table.count);
> > @@ -870,6 +885,13 @@ static void pc_cpu_init(PCInitArgs *args, int
> > cpu_index) exit(1);
> >      }
> >  
> > +    /* Override the default APIC set by the X86CPU init function.
> > +     * We need to do that because:
> > +     * - The APIC ID depends on the CPU topology;
> > +     * - The exact APIC ID used may depend on the machine-type init
> > arguments.
> > +     */
> > +    cpu->env.cpuid_apic_id = apic_id_for_cpu(args, cpu_index);
> Could you create property setter for apic_id and use it here, please.

I was considering doing that, but I thought it could be overkill. I will
do it in the next version.

> 
> > +
> >      x86_cpu_realize(OBJECT(cpu), &err);
> >      if (err) {
> >          error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 22/27] pc: set CPU APIC ID explicitly

[Igor Mammedov]

On Thu, 1 Nov 2012 12:30:39 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Nov 01, 2012 at 03:04:05PM +0100, Igor Mammedov wrote:
> > On Wed, 24 Oct 2012 15:49:56 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > The PC code takes care of CPU topology, and CPU topology affect the CPU
> > > APIC ID. So the PC CPU initialization code needs to set the APIC ID
> > > explicitly.
> > > 
> > > By now, keep the existing behavior but create a apic_id_for_cpu()
> > > function that will be changed later to implement appropriate
> > > topology-dependent behavior.
> > > 
> > > The cpuid_apic_id field is used only at:
> > > 
> > >  - x86_cpu_apic_init(), called from x86_cpu_realize()
> > >  - kvm_init_vcpu(), that is called from the VCPU thread
> > >    created by qemu_init_vcpu(), called by x86_cpu_realize()
> > >  - helper_cpuid(), called only when the VCPU is already running
> > >  - kvm_arch_init_vcpu(), that's called by kvm_init_vcpu()
> > > 
> > > So it's safe to change it before x86_cpu_realize() is called.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > > This is based on the patch that I have originally suybmitted as:
> > >  Subject: pc: create apic_id_for_cpu() function (v3)
> > > ---
> > >  hw/pc.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/hw/pc.c b/hw/pc.c
> > > index 0e9a00f..eb68851 100644
> > > --- a/hw/pc.c
> > > +++ b/hw/pc.c
> > > @@ -553,6 +553,21 @@ static void bochs_bios_write(void *opaque, uint32_t
> > > addr, uint32_t val) }
> > >  }
> > >  
> > > +/* Calculates initial APIC ID for a specific CPU index
> > > + *
> > > + * Currently we need to be able to calculate the APIC ID from the CPU
> > > index
> > > + * alone (without requiring a CPU object), as the QEMU<->Seabios
> > > interfaces have
> > > + * no concept of "CPU index", and the NUMA tables on fw_cfg need the
> > > APIC ID of
> > > + * all CPUs up to max_cpus.
> > > + */
> > > +static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
> > > +{
> > > +    /* right now APIC ID == CPU index. this will eventually change to
> > > use
> > > +     * the CPU topology configuration properly
> > > +     */
> > > +    return cpu_index;
> > > +}
> > > +
> > >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> > >  {
> > >      int index = le32_to_cpu(e820_table.count);
> > > @@ -870,6 +885,13 @@ static void pc_cpu_init(PCInitArgs *args, int
> > > cpu_index) exit(1);
> > >      }
> > >  
> > > +    /* Override the default APIC set by the X86CPU init function.
> > > +     * We need to do that because:
> > > +     * - The APIC ID depends on the CPU topology;
> > > +     * - The exact APIC ID used may depend on the machine-type init
> > > arguments.
> > > +     */
> > > +    cpu->env.cpuid_apic_id = apic_id_for_cpu(args, cpu_index);
> > Could you create property setter for apic_id and use it here, please.
> 
> I was considering doing that, but I thought it could be overkill. I will
> do it in the next version.
Reason for asking it is not to have any CPU internals dangling outside of CPU
object and have CPU created only with help of QOM/qdev calls.

Thanks!
> > 
> > > +
> > >      x86_cpu_realize(OBJECT(cpu), &err);
> > >      if (err) {
> > >          error_report("pc_cpu_init: %s\n", error_get_pretty(err));
> > 
> 

[Qemu-devel] [PATCH 23/27] pc: set fw_cfg data based on APIC ID calculation

[Eduardo Habkost]

This changes FW_CFG_MAX_CPUS and FW_CFG_NUMA to use apic_id_for_cpu(),
so the NUMA table can be based on the APIC IDs, instead of CPU index
(SeaBIOS knows nothing about CPU indexes, just APIC IDs).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Get PC object as argument
 - Add more detailed comments explaining the reason for FW_CFG_MAX_CPUS
   not being simply 'max_cpus'

Changes v2 -> v3:
 - Use PCInitArgs instead of PC object
---
 hw/pc.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index eb68851..8895087 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -585,6 +585,15 @@ int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
     return index;
 }
 
+/* Returns the limit to APIC ID values
+ *
+ * This is used for FW_CFG_MAX_CPUS. See comments on bochs_bios_init().
+ */
+static unsigned int apic_id_limit(PCInitArgs *args)
+{
+    return apic_id_for_cpu(args, max_cpus - 1) + 1;
+}
+
 static FWCfgState *pc_bios_init(PCInitArgs *args)
 {
     FWCfgState *fw_cfg;
@@ -592,6 +601,7 @@ static FWCfgState *pc_bios_init(PCInitArgs *args)
     size_t smbios_len;
     uint64_t *numa_fw_cfg;
     int i, j;
+    unsigned int max_apic_id = apic_id_limit(args);
 
     register_ioport_write(0x8900, 1, 1, bochs_bios_write, NULL);
 
@@ -600,7 +610,21 @@ static FWCfgState *pc_bios_init(PCInitArgs *args)
     register_ioport_write(0x502, 1, 2, bochs_bios_write, NULL);
 
     fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
-    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_cpus);
+    /* 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".
+     *
+     * 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"
+     */
+    fw_cfg_add_i16(fw_cfg, FW_CFG_MAX_CPUS, (uint16_t)max_apic_id);
     fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
     fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
     fw_cfg_add_bytes(fw_cfg, FW_CFG_ACPI_TABLES, (uint8_t *)acpi_tables,
@@ -620,21 +644,24 @@ static FWCfgState *pc_bios_init(PCInitArgs *args)
      * of nodes, one word for each VCPU->node and one word for each node to
      * hold the amount of memory.
      */
-    numa_fw_cfg = g_malloc0((1 + max_cpus + nb_numa_nodes) * 8);
+    numa_fw_cfg = g_malloc0((1 + max_apic_id + nb_numa_nodes) * 8);
     numa_fw_cfg[0] = cpu_to_le64(nb_numa_nodes);
-    for (i = 0; i < max_cpus; i++) {
+    unsigned int cpu_idx;
+    for (cpu_idx = 0; cpu_idx < max_cpus; cpu_idx++) {
+        unsigned int apic_id = apic_id_for_cpu(args, cpu_idx);
+        assert(apic_id < max_apic_id);
         for (j = 0; j < nb_numa_nodes; j++) {
-            if (test_bit(i, node_cpumask[j])) {
-                numa_fw_cfg[i + 1] = cpu_to_le64(j);
+            if (test_bit(cpu_idx, node_cpumask[j])) {
+                numa_fw_cfg[apic_id + 1] = cpu_to_le64(j);
                 break;
             }
         }
     }
     for (i = 0; i < nb_numa_nodes; i++) {
-        numa_fw_cfg[max_cpus + 1 + i] = cpu_to_le64(node_mem[i]);
+        numa_fw_cfg[max_apic_id + 1 + i] = cpu_to_le64(node_mem[i]);
     }
     fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA, (uint8_t *)numa_fw_cfg,
-                     (1 + max_cpus + nb_numa_nodes) * 8);
+                     (1 + max_apic_id + nb_numa_nodes) * 8);
 
     return fw_cfg;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 24/27] tests: support target-specific unit tests

[Eduardo Habkost]

To make unit tests that depend on target-specific files, use
check-unit-<arch>-y and test-obj-<arch>-y.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/Makefile | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile b/tests/Makefile
index 26a67ce..ce4f6f2 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -40,8 +40,6 @@ test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
 test-qapi-obj-y += module.o
 
-$(test-obj-y): QEMU_INCLUDES += -Itests
-
 tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y)
 tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y)
 tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y)
@@ -75,9 +73,21 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
 
-# QTest rules
+# unit test rules:
 
 TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS)))
+
+# target-specific tests/objs:
+test-obj-y += $(foreach TARGET,$(TARGETS), $(test-obj-$(TARGET)-y))
+check-unit-y += $(foreach TARGET,$(TARGETS), $(check-unit-$(TARGET)-y))
+
+$(foreach TARGET,$(TARGETS),$(eval $(test-obj-$(TARGET)-y): QEMU_INCLUDES += -Itarget-$(TARGET)))
+
+
+$(test-obj-y): QEMU_INCLUDES += -Itests
+
+# QTest rules
+
 QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),))
 check-qtest-$(CONFIG_POSIX)=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 25/27] target-i386: topology & APIC ID utility functions

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Blue Swirl <blauwirbel@gmail.com>

Changes v1 -> v2:
 - Support 32-bit APIC IDs (in case x2APIC is going to be used)
 - Coding style changes
 - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
 - Rename topo_make_apic_id() to topo_apicid_for_cpu()
 - Rename __make_apicid() to topo_make_apicid()
 - Spaces around operators on test-x86-cpuid.c, as requested by
   Blue Swirl
 - Make test-x86-cpuid a target-specific test

Changes v2 -> v3:
 - Add documentation pointers to the code
 - Rename bits_for_count() to bitwidth_for_count()
 - Remove unused apicid_*_id() functions

Changes v3 -> v4:
 - Remove now-obsolete FIXME comment from test-x86-cpuid.c
---
 target-i386/topology.h | 133 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |   3 ++
 tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
 4 files changed, 238 insertions(+)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c

diff --git a/target-i386/topology.h b/target-i386/topology.h
new file mode 100644
index 0000000..6f4f2ff
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,133 @@
+/*
+ *  x86 CPU topology data structures and functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef TARGET_I386_TOPOLOGY_H
+#define TARGET_I386_TOPOLOGY_H
+
+/* This file implements the APIC-ID-based CPU topology enumeration logic,
+ * documented at the following document:
+ *   Intel® 64 Architecture Processor Topology Enumeration
+ *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
+ *
+ * This code should be compatible with AMD's "Extended Method" described at:
+ *   AMD CPUID Specification (Publication #25481)
+ *   Section 3: Multiple Core Calcuation
+ * as long as:
+ *  nr_threads is set to 1;
+ *  OFFSET_IDX is assumed to be 0;
+ *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#include "bitops.h"
+
+/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
+ */
+typedef uint32_t apic_id_t;
+
+/* Return the bit width needed for 'count' IDs
+ */
+static unsigned bitwidth_for_count(unsigned count)
+{
+    g_assert(count >= 1);
+    if (count == 1) {
+        return 0;
+    }
+    return bitops_flsl(count - 1) + 1;
+}
+
+/* Bit width of the SMT_ID (thread ID) field on the APIC ID
+ */
+static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return bitwidth_for_count(nr_cores);
+}
+
+/* Bit offset of the Core_ID field
+ */
+static inline unsigned apicid_core_offset(unsigned nr_cores,
+                                          unsigned nr_threads)
+{
+    return apicid_smt_width(nr_cores, nr_threads);
+}
+
+/* Bit offset of the Pkg_ID (socket ID) field
+ */
+static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned nr_threads)
+{
+    return apicid_core_offset(nr_cores, nr_threads) + \
+           apicid_core_width(nr_cores, nr_threads);
+}
+
+/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
+ *
+ * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
+ */
+static inline apic_id_t topo_make_apicid(unsigned nr_cores,
+                                         unsigned nr_threads,
+                                         unsigned pkg_id, unsigned core_id,
+                                         unsigned smt_id)
+{
+    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
+           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
+           smt_id;
+}
+
+/* Calculate thread/core/package IDs for a specific topology,
+ * based on (contiguous) CPU index
+ */
+static inline void topo_ids_from_idx(unsigned nr_cores, unsigned nr_threads,
+                                     unsigned cpu_index,
+                                     unsigned *pkg_id, unsigned *core_id,
+                                     unsigned *smt_id)
+{
+    unsigned core_index = cpu_index / nr_threads;
+    *smt_id = cpu_index % nr_threads;
+    *core_id = core_index % nr_cores;
+    *pkg_id = core_index / nr_cores;
+}
+
+/* Make APIC ID for the CPU 'cpu_index'
+ *
+ * 'cpu_index' is a sequential, contiguous ID for the CPU.
+ */
+static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
+                                            unsigned nr_threads,
+                                            unsigned cpu_index)
+{
+    unsigned pkg_id, core_id, smt_id;
+    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
+                      &pkg_id, &core_id, &smt_id);
+    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+}
+
+#endif /* TARGET_I386_TOPOLOGY_H */
diff --git a/tests/.gitignore b/tests/.gitignore
index f9041f3..38c94ef 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -10,4 +10,5 @@ test-qmp-commands.h
 test-qmp-commands
 test-qmp-input-strict
 test-qmp-marshal.c
+test-x86-cpuid
 *-test
diff --git a/tests/Makefile b/tests/Makefile
index ce4f6f2..83f622c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF)
 check-unit-y += tests/test-coroutine$(EXESUF)
 check-unit-y += tests/test-visitor-serialization$(EXESUF)
 check-unit-y += tests/test-iov$(EXESUF)
+check-unit-i386-y += tests/test-x86-cpuid$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -35,6 +36,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
 	tests/test-qmp-commands.o tests/test-visitor-serialization.o
+test-obj-i386-y = tests/test-x86-cpuid.o
 
 test-qapi-obj-y =  $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y)
 test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -48,6 +50,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y)
 tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y)
 tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o iov.o
+tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 
 tests/test-qapi-types.c tests/test-qapi-types.h :\
 $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
new file mode 100644
index 0000000..1fe9f30
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,101 @@
+/*
+ *  Test code for x86 CPUID and Topology functions
+ *
+ *  Copyright (c) 2012 Red Hat Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+
+#include "topology.h"
+
+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(topo_apicid_for_cpu(1, 1, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 2), ==, 2);
+    g_assert_cmpuint(topo_apicid_for_cpu(1, 1, 3), ==, 3);
+
+
+    /* Test field width calculation for multiple values
+     */
+    g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1);
+    g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2);
+    g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2);
+
+    g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4);
+    g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5);
+
+
+    g_assert_cmpuint(apicid_core_width(30, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(31, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(32, 2), ==, 5);
+    g_assert_cmpuint(apicid_core_width(33, 2), ==, 6);
+
+
+    /* build a weird topology and see if IDs are calculated correctly
+     */
+
+    /* This will use 2 bits for thread ID and 3 bits for core ID
+     */
+    g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2);
+    g_assert_cmpuint(apicid_core_width(6, 3), ==, 3);
+    g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 0), ==, 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1), ==, 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2), ==, 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 0), ==, (1 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 1), ==, (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 3 + 2), ==, (1 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 0), ==, (2 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 1), ==, (2 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 2 * 3 + 2), ==, (2 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2);
+
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
+                                         (1 << 5));
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
+                                         (1 << 5) | (1 << 2) | 1);
+    g_assert_cmpuint(topo_apicid_for_cpu(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
+                                         (3 << 5) | (5 << 2) | 2);
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/cpuid/topology/basic", test_topo_bits);
+
+    g_test_run();
+
+    return 0;
+}
-- 
1.7.11.7

[Qemu-devel] [PATCH 26/27] pc: create separate init function for pc-1.3

[Eduardo Habkost]

This:
 - Renames the init function for pc-1.2 and lower to pc_init_pci_v1_2;
 - Creates a pc_init_pci_v1_3 function for pc-1.3.

Right now both functions have exactly the same code, but the following patch
will change pc_init_v1_2 to set compatibility PCInitArgs fields.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>

This patch conflicts with a patch from Michael S. Tsirkin. If his patch
gets into the tree first (which is likely), I can rebase this one to use
the new function created by Michael.
---
 hw/pc_piix.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 7de8f0d..57a3228 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -280,7 +280,8 @@ static void pc_init1(PCInitArgs *pc_args)
     }
 }
 
-static void pc_init_pci(QEMUMachineInitArgs *args)
+/* PC init function for pc-1.3 and higher */
+static void pc_init_pci_v1_3(QEMUMachineInitArgs *args)
 {
     PCInitArgs pc_args = {
         .qemu_args = args,
@@ -290,6 +291,16 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
     pc_init1(&pc_args);
 }
 
+/* PC init function for pc-1.2 and lower */
+static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
+{
+    PCInitArgs pc_args = {
+        .qemu_args = args,
+        .pci_enabled = true,
+        .kvmclock_enabled = true,
+    };
+    pc_init1(&pc_args);
+}
 static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
     PCInitArgs pc_args = {
@@ -327,7 +338,7 @@ static QEMUMachine pc_machine_v1_3 = {
     .name = "pc-1.3",
     .alias = "pc",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_3,
     .max_cpus = 255,
     .is_default = 1,
 };
@@ -362,7 +373,7 @@ static QEMUMachine pc_machine_v1_3 = {
 static QEMUMachine pc_machine_v1_2 = {
     .name = "pc-1.2",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_2,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_2,
@@ -405,7 +416,7 @@ static QEMUMachine pc_machine_v1_2 = {
 static QEMUMachine pc_machine_v1_1 = {
     .name = "pc-1.1",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_2,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_1,
@@ -440,7 +451,7 @@ static QEMUMachine pc_machine_v1_1 = {
 static QEMUMachine pc_machine_v1_0 = {
     .name = "pc-1.0",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_2,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_1_0,
@@ -455,7 +466,7 @@ static QEMUMachine pc_machine_v1_0 = {
 static QEMUMachine pc_machine_v0_15 = {
     .name = "pc-0.15",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_2,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_0_15,
@@ -487,7 +498,7 @@ static QEMUMachine pc_machine_v0_15 = {
 static QEMUMachine pc_machine_v0_14 = {
     .name = "pc-0.14",
     .desc = "Standard PC",
-    .init = pc_init_pci,
+    .init = pc_init_pci_v1_2,
     .max_cpus = 255,
     .compat_props = (GlobalProperty[]) {
         PC_COMPAT_0_14, 
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 26/27] pc: create separate init function for pc-1.3

[Michael S. Tsirkin]

On Wed, Oct 24, 2012 at 03:50:00PM -0200, Eduardo Habkost wrote:
> This:
>  - Renames the init function for pc-1.2 and lower to pc_init_pci_v1_2;
>  - Creates a pc_init_pci_v1_3 function for pc-1.3.
> 
> Right now both functions have exactly the same code, but the following patch
> will change pc_init_v1_2 to set compatibility PCInitArgs fields.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> 
> This patch conflicts with a patch from Michael S. Tsirkin. If his patch
> gets into the tree first (which is likely), I can rebase this one to use
> the new function created by Michael.

Could you simply include my patch in your patchset instead?
This removes any confusion and git am automatically ignores
duplicates.

> ---
>  hw/pc_piix.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 7de8f0d..57a3228 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -280,7 +280,8 @@ static void pc_init1(PCInitArgs *pc_args)
>      }
>  }
>  
> -static void pc_init_pci(QEMUMachineInitArgs *args)
> +/* PC init function for pc-1.3 and higher */
> +static void pc_init_pci_v1_3(QEMUMachineInitArgs *args)
>  {
>      PCInitArgs pc_args = {
>          .qemu_args = args,
> @@ -290,6 +291,16 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>      pc_init1(&pc_args);
>  }
>  
> +/* PC init function for pc-1.2 and lower */
> +static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
> +{
> +    PCInitArgs pc_args = {
> +        .qemu_args = args,
> +        .pci_enabled = true,
> +        .kvmclock_enabled = true,
> +    };
> +    pc_init1(&pc_args);
> +}
>  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
>  {
>      PCInitArgs pc_args = {
> @@ -327,7 +338,7 @@ static QEMUMachine pc_machine_v1_3 = {
>      .name = "pc-1.3",
>      .alias = "pc",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_3,
>      .max_cpus = 255,
>      .is_default = 1,
>  };
> @@ -362,7 +373,7 @@ static QEMUMachine pc_machine_v1_3 = {
>  static QEMUMachine pc_machine_v1_2 = {
>      .name = "pc-1.2",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_2,
> @@ -405,7 +416,7 @@ static QEMUMachine pc_machine_v1_2 = {
>  static QEMUMachine pc_machine_v1_1 = {
>      .name = "pc-1.1",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_1,
> @@ -440,7 +451,7 @@ static QEMUMachine pc_machine_v1_1 = {
>  static QEMUMachine pc_machine_v1_0 = {
>      .name = "pc-1.0",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_1_0,
> @@ -455,7 +466,7 @@ static QEMUMachine pc_machine_v1_0 = {
>  static QEMUMachine pc_machine_v0_15 = {
>      .name = "pc-0.15",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_15,
> @@ -487,7 +498,7 @@ static QEMUMachine pc_machine_v0_15 = {
>  static QEMUMachine pc_machine_v0_14 = {
>      .name = "pc-0.14",
>      .desc = "Standard PC",
> -    .init = pc_init_pci,
> +    .init = pc_init_pci_v1_2,
>      .max_cpus = 255,
>      .compat_props = (GlobalProperty[]) {
>          PC_COMPAT_0_14, 
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH 26/27] pc: create separate init function for pc-1.3

[Eduardo Habkost]

On Wed, Oct 24, 2012 at 08:12:36PM +0200, Michael S. Tsirkin wrote:
> On Wed, Oct 24, 2012 at 03:50:00PM -0200, Eduardo Habkost wrote:
> > This:
> >  - Renames the init function for pc-1.2 and lower to pc_init_pci_v1_2;
> >  - Creates a pc_init_pci_v1_3 function for pc-1.3.
> > 
> > Right now both functions have exactly the same code, but the following patch
> > will change pc_init_v1_2 to set compatibility PCInitArgs fields.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > 
> > This patch conflicts with a patch from Michael S. Tsirkin. If his patch
> > gets into the tree first (which is likely), I can rebase this one to use
> > the new function created by Michael.
> 
> Could you simply include my patch in your patchset instead?
> This removes any confusion and git am automatically ignores
> duplicates.

I could, and maybe I will do that if I have to respin. I just didn't
want to introduce a dependency on the unrelated PV EOI fix, by now. If
you had two separate patches: one for splitting the functions, and
another one for the PV EOI change, I would surely pull the first one.

> 
> > ---
> >  hw/pc_piix.c | 25 ++++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 7de8f0d..57a3228 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -280,7 +280,8 @@ static void pc_init1(PCInitArgs *pc_args)
> >      }
> >  }
> >  
> > -static void pc_init_pci(QEMUMachineInitArgs *args)
> > +/* PC init function for pc-1.3 and higher */
> > +static void pc_init_pci_v1_3(QEMUMachineInitArgs *args)
> >  {
> >      PCInitArgs pc_args = {
> >          .qemu_args = args,
> > @@ -290,6 +291,16 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
> >      pc_init1(&pc_args);
> >  }
> >  
> > +/* PC init function for pc-1.2 and lower */
> > +static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
> > +{
> > +    PCInitArgs pc_args = {
> > +        .qemu_args = args,
> > +        .pci_enabled = true,
> > +        .kvmclock_enabled = true,
> > +    };
> > +    pc_init1(&pc_args);
> > +}
> >  static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> >  {
> >      PCInitArgs pc_args = {
> > @@ -327,7 +338,7 @@ static QEMUMachine pc_machine_v1_3 = {
> >      .name = "pc-1.3",
> >      .alias = "pc",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_3,
> >      .max_cpus = 255,
> >      .is_default = 1,
> >  };
> > @@ -362,7 +373,7 @@ static QEMUMachine pc_machine_v1_3 = {
> >  static QEMUMachine pc_machine_v1_2 = {
> >      .name = "pc-1.2",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_2,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_1_2,
> > @@ -405,7 +416,7 @@ static QEMUMachine pc_machine_v1_2 = {
> >  static QEMUMachine pc_machine_v1_1 = {
> >      .name = "pc-1.1",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_2,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_1_1,
> > @@ -440,7 +451,7 @@ static QEMUMachine pc_machine_v1_1 = {
> >  static QEMUMachine pc_machine_v1_0 = {
> >      .name = "pc-1.0",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_2,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_1_0,
> > @@ -455,7 +466,7 @@ static QEMUMachine pc_machine_v1_0 = {
> >  static QEMUMachine pc_machine_v0_15 = {
> >      .name = "pc-0.15",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_2,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_0_15,
> > @@ -487,7 +498,7 @@ static QEMUMachine pc_machine_v0_15 = {
> >  static QEMUMachine pc_machine_v0_14 = {
> >      .name = "pc-0.14",
> >      .desc = "Standard PC",
> > -    .init = pc_init_pci,
> > +    .init = pc_init_pci_v1_2,
> >      .max_cpus = 255,
> >      .compat_props = (GlobalProperty[]) {
> >          PC_COMPAT_0_14, 
> > -- 
> > 1.7.11.7

-- 
Eduardo

[Qemu-devel] [PATCH 27/27] pc: generate APIC IDs according to CPU topology

[Eduardo Habkost]

This keeps compatibility on machine-types pc-1.2 and older, and prints a
warning in case the requested configuration won't get the correct
topology.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Move code to cpu.c
 - keep using cpu_index on *-user
 - Use SMP.contiguous_apic_ids global property
 - Prints warning in case the compatibility mode will expose incorrect
   topology

Changes v2 -> v3:
 - Now all code is inside hw/pc.c
 - Use a real "PC" class and a "contiguous_apic_ids" property

Changes v3 -> v4:
 - Instead of using a global property, use a separate machine init
   function and a PCInitArgs field, to implement compatibility mode
 - Use error_report() instead of fprintf(stderr) for the warning
 - Use a field on PCInitArgs instead of a static variable to check
   if warning was already printed
---
 hw/pc.c      | 19 +++++++++++++++----
 hw/pc.h      |  6 ++++++
 hw/pc_piix.c |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 8895087..3d08271 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -51,6 +51,8 @@
 #include "exec-memory.h"
 #include "arch_init.h"
 #include "bitmap.h"
+#include "topology.h"
+#include "cpus.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ
@@ -562,10 +564,19 @@ static void bochs_bios_write(void *opaque, uint32_t addr, uint32_t val)
  */
 static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
 {
-    /* right now APIC ID == CPU index. this will eventually change to use
-     * the CPU topology configuration properly
-     */
-    return cpu_index;
+    uint32_t correct_id;
+
+    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
+    if (args->compat_contiguous_apic_ids) {
+        if (cpu_index != correct_id && !args->apic_id_warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            args->apic_id_warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
 }
 
 int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
diff --git a/hw/pc.h b/hw/pc.h
index 53883f5..a14944a 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -81,6 +81,12 @@ typedef struct PCInitArgs {
     QEMUMachineInitArgs *qemu_args;
     bool pci_enabled;
     bool kvmclock_enabled;
+    /* Enable compatibility (buggy) APIC ID generation, that keep APIC IDs
+     * contiguous
+     */
+    bool compat_contiguous_apic_ids;
+    /* Warning message about incorrect APIC ID was shown */
+    bool apic_id_warned;
 
     /* Memory regions & sizes: */
     MemoryRegion *system_memory;
diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 57a3228..79a54e6 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -298,6 +298,7 @@ static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
         .qemu_args = args,
         .pci_enabled = true,
         .kvmclock_enabled = true,
+        .compat_contiguous_apic_ids = true,
     };
     pc_init1(&pc_args);
 }
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 27/27] pc: generate APIC IDs according to CPU topology

[Igor Mammedov]

On Wed, 24 Oct 2012 15:50:01 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This keeps compatibility on machine-types pc-1.2 and older, and prints a
> warning in case the requested configuration won't get the correct
> topology.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
>  - Move code to cpu.c
>  - keep using cpu_index on *-user
>  - Use SMP.contiguous_apic_ids global property
>  - Prints warning in case the compatibility mode will expose incorrect
>    topology
> 
> Changes v2 -> v3:
>  - Now all code is inside hw/pc.c
>  - Use a real "PC" class and a "contiguous_apic_ids" property
> 
> Changes v3 -> v4:
>  - Instead of using a global property, use a separate machine init
>    function and a PCInitArgs field, to implement compatibility mode
>  - Use error_report() instead of fprintf(stderr) for the warning
>  - Use a field on PCInitArgs instead of a static variable to check
>    if warning was already printed
> ---
>  hw/pc.c      | 19 +++++++++++++++----
>  hw/pc.h      |  6 ++++++
>  hw/pc_piix.c |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pc.c b/hw/pc.c
> index 8895087..3d08271 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -51,6 +51,8 @@
>  #include "exec-memory.h"
>  #include "arch_init.h"
>  #include "bitmap.h"
> +#include "topology.h"
> +#include "cpus.h"
>  
>  /* debug PC/ISA interrupts */
>  //#define DEBUG_IRQ
> @@ -562,10 +564,19 @@ static void bochs_bios_write(void *opaque, uint32_t
> addr, uint32_t val) */
>  static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
>  {
> -    /* right now APIC ID == CPU index. this will eventually change to use
> -     * the CPU topology configuration properly
> -     */
> -    return cpu_index;
> +    uint32_t correct_id;
> +
> +    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
> +    if (args->compat_contiguous_apic_ids) {
> +        if (cpu_index != correct_id && !args->apic_id_warned) {
> +            error_report("APIC IDs set in compatibility mode, "
> +                         "CPU topology won't match the configuration");
> +            args->apic_id_warned = true;
> +        }
> +        return cpu_index;
> +    } else {
> +        return correct_id;
> +    }
>  }
>  
>  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> diff --git a/hw/pc.h b/hw/pc.h
> index 53883f5..a14944a 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -81,6 +81,12 @@ typedef struct PCInitArgs {
>      QEMUMachineInitArgs *qemu_args;
>      bool pci_enabled;
>      bool kvmclock_enabled;
> +    /* Enable compatibility (buggy) APIC ID generation, that keep APIC IDs
> +     * contiguous
> +     */
> +    bool compat_contiguous_apic_ids;
> +    /* Warning message about incorrect APIC ID was shown */
> +    bool apic_id_warned;
>  
>      /* Memory regions & sizes: */
>      MemoryRegion *system_memory;
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 57a3228..79a54e6 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -298,6 +298,7 @@ static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
>          .qemu_args = args,
>          .pci_enabled = true,
>          .kvmclock_enabled = true,
> +        .compat_contiguous_apic_ids = true,
I suppose libvirt/whatever would know that it's starting v1_2 machine, so it
would be able to provide valid APIC ID at hotplug time. But it looks fragile.

As Anthony once suggested, perhaps we could model sockets as links.
/machine/cpu[apic_id..]
So machine at level we would have a pre-defined topology with apic_id in link
name serving as initial apic_id selector pins in socket.

Pre-allocate them (sockets) in pc_cpus_init() for all possible CPUs and when
creating CPUs, apic_id property setter could accept link path and get
appropriate apic_id from link name and assign itself to a specified link.

It would be friendly with hot-plug and
-device X86CPU,id='/machine/cpu[apic_id]' cmdline if we ever decide in future
to create CPUs this way.

>      };
>      pc_init1(&pc_args);
>  }

Re: [Qemu-devel] [PATCH 27/27] pc: generate APIC IDs according to CPU topology

[Eduardo Habkost]

On Thu, Nov 01, 2012 at 03:46:08PM +0100, Igor Mammedov wrote:
> On Wed, 24 Oct 2012 15:50:01 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This keeps compatibility on machine-types pc-1.2 and older, and prints a
> > warning in case the requested configuration won't get the correct
> > topology.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Changes v1 -> v2:
> >  - Move code to cpu.c
> >  - keep using cpu_index on *-user
> >  - Use SMP.contiguous_apic_ids global property
> >  - Prints warning in case the compatibility mode will expose incorrect
> >    topology
> > 
> > Changes v2 -> v3:
> >  - Now all code is inside hw/pc.c
> >  - Use a real "PC" class and a "contiguous_apic_ids" property
> > 
> > Changes v3 -> v4:
> >  - Instead of using a global property, use a separate machine init
> >    function and a PCInitArgs field, to implement compatibility mode
> >  - Use error_report() instead of fprintf(stderr) for the warning
> >  - Use a field on PCInitArgs instead of a static variable to check
> >    if warning was already printed
> > ---
> >  hw/pc.c      | 19 +++++++++++++++----
> >  hw/pc.h      |  6 ++++++
> >  hw/pc_piix.c |  1 +
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 8895087..3d08271 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -51,6 +51,8 @@
> >  #include "exec-memory.h"
> >  #include "arch_init.h"
> >  #include "bitmap.h"
> > +#include "topology.h"
> > +#include "cpus.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> > @@ -562,10 +564,19 @@ static void bochs_bios_write(void *opaque, uint32_t
> > addr, uint32_t val) */
> >  static uint32_t apic_id_for_cpu(PCInitArgs *args, int cpu_index)
> >  {
> > -    /* right now APIC ID == CPU index. this will eventually change to use
> > -     * the CPU topology configuration properly
> > -     */
> > -    return cpu_index;
> > +    uint32_t correct_id;
> > +
> > +    correct_id = topo_apicid_for_cpu(smp_cores, smp_threads, cpu_index);
> > +    if (args->compat_contiguous_apic_ids) {
> > +        if (cpu_index != correct_id && !args->apic_id_warned) {
> > +            error_report("APIC IDs set in compatibility mode, "
> > +                         "CPU topology won't match the configuration");
> > +            args->apic_id_warned = true;
> > +        }
> > +        return cpu_index;
> > +    } else {
> > +        return correct_id;
> > +    }
> >  }
> >  
> >  int e820_add_entry(uint64_t address, uint64_t length, uint32_t type)
> > diff --git a/hw/pc.h b/hw/pc.h
> > index 53883f5..a14944a 100644
> > --- a/hw/pc.h
> > +++ b/hw/pc.h
> > @@ -81,6 +81,12 @@ typedef struct PCInitArgs {
> >      QEMUMachineInitArgs *qemu_args;
> >      bool pci_enabled;
> >      bool kvmclock_enabled;
> > +    /* Enable compatibility (buggy) APIC ID generation, that keep APIC IDs
> > +     * contiguous
> > +     */
> > +    bool compat_contiguous_apic_ids;
> > +    /* Warning message about incorrect APIC ID was shown */
> > +    bool apic_id_warned;
> >  
> >      /* Memory regions & sizes: */
> >      MemoryRegion *system_memory;
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 57a3228..79a54e6 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -298,6 +298,7 @@ static void pc_init_pci_v1_2(QEMUMachineInitArgs *args)
> >          .qemu_args = args,
> >          .pci_enabled = true,
> >          .kvmclock_enabled = true,
> > +        .compat_contiguous_apic_ids = true,
> I suppose libvirt/whatever would know that it's starting v1_2 machine, so it
> would be able to provide valid APIC ID at hotplug time. But it looks fragile.

Making the APIC IDs work properly while _not_ exposing them to the
outside is being hard enough, so I really wouldn't like to expose the
APIC IDs to the outside, and depend on libvirt setting APIC IDs
properly.

In either case, we need to make today's libvirt (that knows nothing
about APIC IDs) running qemu-1.2 to be able to live-migrate to a host
using the same libvirt verison, but running qemu-1.3. So it doesn't
matter how we model it internally or how we make the new interfaces look
like, I don't see how we could avoid having a compat_contiguous_apic_ids
flag somewhere, to make "qemu-1.3 -machine pc-1.2" keep the existing
behavior.

> 
> As Anthony once suggested, perhaps we could model sockets as links.
> /machine/cpu[apic_id..]
> So machine at level we would have a pre-defined topology with apic_id in link
> name serving as initial apic_id selector pins in socket.

Are you talking about having a socket/link for each VCPU (thread), or a
socket/link for each CPU package (that may contain multiple cores and
multiples threads).

I was expecting we would end up doing the latter, not the former. In
other words, that we would have:
a /machine/cpu[...] for each CPU socket/package,
a /machine/cpu[...]/core[...] for each core,
a /machine/cpu[...]/core[...]/thread[...] for each VCPU (that is
actually a CPU thread).

In that case, why not use the CPU socket/package index on the link name,
instead of the APIC ID?

Also, the actual CPU package object (containing multiple cores/threads)
will need to get more information from the parent object, not just the
APIC ID. It needs:
- The "base" APIC ID for the threads/cores inside the CPU;
- The number of bits it can use to identify its threads/cores
  (so it doesn't conflict with the APIC IDs used by other sockets).


> 
> Pre-allocate them (sockets) in pc_cpus_init() for all possible CPUs and when
> creating CPUs, apic_id property setter could accept link path and get
> appropriate apic_id from link name and assign itself to a specified link.

That could work. But I wouldn't like to make the modelling work (that is
really taking very long) to get into the way of fixing bugs.


> 
> It would be friendly with hot-plug and
> -device X86CPU,id='/machine/cpu[apic_id]' cmdline if we ever decide in future
> to create CPUs this way.

But if we make the interface based on CPU sockets/packages, not VCPU
threads, we wouldn't even need to expose the APIC IDs on the
externally-visibile -device link, right?


> 
> >      };
> >      pc_init1(&pc_args);
> >  }
> 

-- 
Eduardo