[Qemu-devel] [PATCH 0/8] target-i386: Simplify APIC ID initialization, move compat code to pc.c

[Qemu-devel] [PATCH 0/8] target-i386: Simplify APIC ID initialization, move compat code to pc.c

[Eduardo Habkost]

This series removes the APIC ID initialization code from x86_cpu_initfn()
(getting us one step closer to making object_new() of X86CPU have no dependency
on cpu_exec_init() and other global QEMU state), and moves the APIC ID
compatibility logic from target-i386/cpu.c to hw/i386/pc.c.

Eduardo Habkost (8):
  target-i386: Rename cpu_x86_init() to cpu_x86_init_user()
  target-i386: Eliminate cpu_init() function
  target-i386: Move CPUX86State.cpuid_apic_id to X86CPU.apic_id
  target-i386: Keep track of apic-id setting
  target-i386: Set APIC ID using cpu_index on CONFIG_USER
  target-i386: Don't set APIC ID on instance_init
  target-i386: Move topology.h to hw/i386/topology.h
  target-i386: Move APIC ID compatibility code to pc.c

 hw/i386/pc.c                        | 35 ++++++++++++++++++++
 {target-i386 => hw/i386}/topology.h |  6 ++--
 target-i386/cpu-qom.h               |  2 ++
 target-i386/cpu.c                   | 66 ++++++++++++-------------------------
 target-i386/cpu.h                   | 13 ++------
 target-i386/kvm.c                   |  2 +-
 tests/Makefile                      |  2 --
 tests/test-x86-cpuid.c              |  2 +-
 8 files changed, 66 insertions(+), 62 deletions(-)
 rename {target-i386 => hw/i386}/topology.h (97%)

-- 
1.9.3

[Qemu-devel] [PATCH 1/8] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Eduardo Habkost]

The function is used only for CONFIG_USER, so make its purpose clear.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 2 +-
 target-i386/cpu.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b81ac5c..91f80ed 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2150,7 +2150,7 @@ out:
     return cpu;
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+X86CPU *cpu_x86_init_user(const char *cpu_model)
 {
     Error *error = NULL;
     X86CPU *cpu;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 3ecff96..9250a9c 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1036,7 +1036,7 @@ typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init(const char *cpu_model);
+X86CPU *cpu_x86_init_user(const char *cpu_model);
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
@@ -1227,7 +1227,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 
 static inline CPUX86State *cpu_init(const char *cpu_model)
 {
-    X86CPU *cpu = cpu_x86_init(cpu_model);
+    X86CPU *cpu = cpu_x86_init_user(cpu_model);
     if (cpu == NULL) {
         return NULL;
     }
-- 
1.9.3

[Qemu-devel] [PATCH 2/8] target-i386: Eliminate cpu_init() function

[Eduardo Habkost]

Instead of putting extra logic inside cpu.h, just do everything inside
cpu_x86_init_user().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  4 ++--
 target-i386/cpu.h | 12 +++---------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 91f80ed..bbf1155 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2150,7 +2150,7 @@ out:
     return cpu;
 }
 
-X86CPU *cpu_x86_init_user(const char *cpu_model)
+CPUX86State *cpu_x86_init_user(const char *cpu_model)
 {
     Error *error = NULL;
     X86CPU *cpu;
@@ -2171,7 +2171,7 @@ out:
             cpu = NULL;
         }
     }
-    return cpu;
+    return &cpu->env;
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 9250a9c..432aa7e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1036,7 +1036,6 @@ typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init_user(const char *cpu_model);
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
@@ -1225,14 +1224,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 # define PHYS_ADDR_MASK 0xfffffffffLL
 # endif
 
-static inline CPUX86State *cpu_init(const char *cpu_model)
-{
-    X86CPU *cpu = cpu_x86_init_user(cpu_model);
-    if (cpu == NULL) {
-        return NULL;
-    }
-    return &cpu->env;
-}
+/* CPU creation function for *-user */
+CPUX86State *cpu_x86_init_user(const char *cpu_model);
+#define cpu_init cpu_x86_init_user
 
 #define cpu_exec cpu_x86_exec
 #define cpu_gen_code cpu_x86_gen_code
-- 
1.9.3

[Qemu-devel] [PATCH 3/8] target-i386: Move CPUX86State.cpuid_apic_id to X86CPU.apic_id

[Eduardo Habkost]

The field doesn't need to be inside CPUState, and it is not specific for
the CPUID instruction, so move and rename it.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  1 +
 target-i386/cpu.c     | 17 ++++++++---------
 target-i386/cpu.h     |  1 -
 target-i386/kvm.c     |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index b557b61..4a6f48a 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,6 +93,7 @@ typedef struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
+    uint32_t apic_id;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bbf1155..cbed717 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1691,7 +1691,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    int64_t value = cpu->env.cpuid_apic_id;
+    int64_t value = cpu->apic_id;
 
     visit_type_int(v, &value, name, errp);
 }
@@ -1724,11 +1724,11 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
+    if ((value != cpu->apic_id) && cpu_exists(value)) {
         error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
         return;
     }
-    cpu->env.cpuid_apic_id = value;
+    cpu->apic_id = value;
 }
 
 /* Generic getter for "feature-words" and "filtered-features" properties */
@@ -2274,7 +2274,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 1:
         *eax = env->cpuid_version;
-        *ebx = (env->cpuid_apic_id << 24) | 8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
+        *ebx = (cpu->apic_id << 24) |
+               8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
         *ecx = env->features[FEAT_1_ECX];
         *edx = env->features[FEAT_1_EDX];
         if (cs->nr_cores * cs->nr_threads > 1) {
@@ -2723,7 +2724,6 @@ static void mce_init(X86CPU *cpu)
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    CPUX86State *env = &cpu->env;
     DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
@@ -2742,7 +2742,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     object_property_add_child(OBJECT(cpu), "apic",
                               OBJECT(cpu->apic_state), NULL);
-    qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
+    qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
@@ -2925,7 +2925,7 @@ static void x86_cpu_initfn(Object *obj)
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-    env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
+    cpu->apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
@@ -2939,9 +2939,8 @@ static void x86_cpu_initfn(Object *obj)
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
-    CPUX86State *env = &cpu->env;
 
-    return env->cpuid_apic_id;
+    return cpu->apic_id;
 }
 
 static bool x86_cpu_get_paging_enabled(const CPUState *cs)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 432aa7e..5388abc 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -998,7 +998,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_version;
     FeatureWordArray features;
     uint32_t cpuid_model[12];
-    uint32_t cpuid_apic_id;
 
     /* MTRRs */
     uint64_t mtrr_fixed[11];
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f92edfe..b0e93ed 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -430,7 +430,7 @@ static void cpu_update_state(void *opaque, int running, RunState state)
 unsigned long kvm_arch_vcpu_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
-    return cpu->env.cpuid_apic_id;
+    return cpu->apic_id;
 }
 
 #ifndef KVM_CPUID_SIGNATURE_NEXT
-- 
1.9.3

[Qemu-devel] [PATCH 4/8] target-i386: Keep track of apic-id setting

[Eduardo Habkost]

Set a flag indicating that the apic-id property was set, so we can later
ensure we won't try to realize a X86CPU object before apic-id was set.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 target-i386/cpu-qom.h | 1 +
 target-i386/cpu.c     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 4a6f48a..ba0ee15 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -94,6 +94,7 @@ typedef struct X86CPU {
     bool migratable;
     bool host_features;
     uint32_t apic_id;
+    bool apic_id_set;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cbed717..bb9525d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1691,7 +1691,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    int64_t value = cpu->apic_id;
+    int64_t value = cpu->apic_id_set ? cpu->apic_id : -1;
 
     visit_type_int(v, &value, name, errp);
 }
@@ -1729,6 +1729,7 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
         return;
     }
     cpu->apic_id = value;
+    cpu->apic_id_set = true;
 }
 
 /* Generic getter for "feature-words" and "filtered-features" properties */
-- 
1.9.3

[Qemu-devel] [PATCH 5/8] target-i386: Set APIC ID using cpu_index on CONFIG_USER

[Eduardo Habkost]

The PC CPU initialization code already sets apic-id based on the CPU
topology, and CONFIG_USER doesn't need the topology-based APIC ID
calculation code.

Make CONFIG_USER set apic-id before realizing the CPU (just like PC
already does), so we can simplify x86_cpu_initfn later. As there is no
CPU topology configuration in CONFIG_USER, just use cpu_index as the
APIC ID.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 target-i386/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bb9525d..4b0e0a5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2161,6 +2161,12 @@ CPUX86State *cpu_x86_init_user(const char *cpu_model)
         goto out;
     }
 
+    object_property_set_int(OBJECT(cpu), CPU(cpu)->cpu_index, "apic-id",
+                            &error);
+    if (error) {
+        goto out;
+    }
+
     object_property_set_bool(OBJECT(cpu), true, "realized", &error);
 
 out:
-- 
1.9.3

[Qemu-devel] [PATCH 6/8] target-i386: Don't set APIC ID on instance_init

[Eduardo Habkost]

Instead of setting APIC ID automatically when creating a X86CPU, require
the property to be set before realizing the object (which all callers of
cpu_x86_create() already do).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
---
 target-i386/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4b0e0a5..4b6e19b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2789,6 +2789,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     static bool ht_warned;
 
+    if (!cpu->apic_id_set) {
+        error_setg(errp, "apic-id property was not set");
+        return;
+    }
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }
@@ -2932,7 +2937,6 @@ static void x86_cpu_initfn(Object *obj)
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-    cpu->apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
-- 
1.9.3

[Qemu-devel] [PATCH 7/8] target-i386: Move topology.h to hw/i386/topology.h

[Eduardo Habkost]

This will allow the PC code to use the header, and lets us eliminate the
QEMU_INCLUDES hack inside tests/Makefile.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 {target-i386 => hw/i386}/topology.h | 6 +++---
 target-i386/cpu.c                   | 2 +-
 tests/Makefile                      | 2 --
 tests/test-x86-cpuid.c              | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)
 rename {target-i386 => hw/i386}/topology.h (97%)

diff --git a/target-i386/topology.h b/hw/i386/topology.h
similarity index 97%
rename from target-i386/topology.h
rename to hw/i386/topology.h
index 07a6c5f..9c6f3a9 100644
--- a/target-i386/topology.h
+++ b/hw/i386/topology.h
@@ -21,8 +21,8 @@
  * 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
+#ifndef HW_I386_TOPOLOGY_H
+#define HW_I386_TOPOLOGY_H
 
 /* This file implements the APIC-ID-based CPU topology enumeration logic,
  * documented at the following document:
@@ -131,4 +131,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
     return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
 }
 
-#endif /* TARGET_I386_TOPOLOGY_H */
+#endif /* HW_I386_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4b6e19b..d8cd7c9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,7 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
diff --git a/tests/Makefile b/tests/Makefile
index e4ddb6a..bdc7cc5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -232,8 +232,6 @@ $(test-obj-y): QEMU_INCLUDES += -Itests
 QEMU_CFLAGS += -I$(SRC_PATH)/tests
 qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
 
-tests/test-x86-cpuid.o: QEMU_INCLUDES += -I$(SRC_PATH)/target-i386
-
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
 tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 8d9f96a..6cd20d4 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -24,7 +24,7 @@
 
 #include <glib.h>
 
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 static void test_topo_bits(void)
 {
-- 
1.9.3

[Qemu-devel] [PATCH 8/8] target-i386: Move APIC ID compatibility code to pc.c

[Eduardo Habkost]

The APIC ID compatibility code is required only for PC, and now that
x86_cpu_initfn() doesn't use x86_cpu_apic_id_from_index() anymore, that
code can be moved to pc.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c      | 35 +++++++++++++++++++++++++++++++++++
 target-i386/cpu.c | 34 ----------------------------------
 2 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c0e55a6..b534ab0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,6 +25,8 @@
 #include "hw/i386/pc.h"
 #include "hw/char/serial.h"
 #include "hw/i386/apic.h"
+#include "hw/i386/topology.h"
+#include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
 #include "hw/pci/pci.h"
@@ -626,6 +628,39 @@ bool e820_get_entry(int idx, uint32_t type, uint64_t *address, uint64_t *length)
     return false;
 }
 
+/* Enables contiguous-apic-ID mode, for compatibility */
+static bool compat_apic_id_mode;
+
+void enable_compat_apic_id_mode(void)
+{
+    compat_apic_id_mode = true;
+}
+
+/* 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.
+ */
+uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
+{
+    uint32_t correct_id;
+    static bool warned;
+
+    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
+    if (compat_apic_id_mode) {
+        if (cpu_index != correct_id && !warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
+}
+
 /* Calculates the limit to CPU APIC ID values
  *
  * This function returns the limit for the APIC ID value, so that all
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index d8cd7c9..5d128ee 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2858,39 +2857,6 @@ out:
     }
 }
 
-/* Enables contiguous-apic-ID mode, for compatibility */
-static bool compat_apic_id_mode;
-
-void enable_compat_apic_id_mode(void)
-{
-    compat_apic_id_mode = true;
-}
-
-/* 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.
- */
-uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
-{
-    uint32_t correct_id;
-    static bool warned;
-
-    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
-    if (compat_apic_id_mode) {
-        if (cpu_index != correct_id && !warned) {
-            error_report("APIC IDs set in compatibility mode, "
-                         "CPU topology won't match the configuration");
-            warned = true;
-        }
-        return cpu_index;
-    } else {
-        return correct_id;
-    }
-}
-
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
-- 
1.9.3

Re: [Qemu-devel] [PATCH 5/8] target-i386: Set APIC ID using cpu_index on CONFIG_USER

[Paolo Bonzini]

On 19/12/2014 03:41, Eduardo Habkost wrote:
> +    object_property_set_int(OBJECT(cpu), CPU(cpu)->cpu_index, "apic-id",
> +                            &error);
> +    if (error) {
> +        goto out;
> +    }
> +

Should this use &error_abort?

Paolo

Re: [Qemu-devel] [PATCH 4/8] target-i386: Keep track of apic-id setting

[Paolo Bonzini]

On 19/12/2014 03:41, Eduardo Habkost wrote:
> Set a flag indicating that the apic-id property was set, so we can later
> ensure we won't try to realize a X86CPU object before apic-id was set.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu-qom.h | 1 +
>  target-i386/cpu.c     | 3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> index 4a6f48a..ba0ee15 100644
> --- a/target-i386/cpu-qom.h
> +++ b/target-i386/cpu-qom.h
> @@ -94,6 +94,7 @@ typedef struct X86CPU {
>      bool migratable;
>      bool host_features;
>      uint32_t apic_id;
> +    bool apic_id_set;
>  
>      /* if true the CPUID code directly forward host cache leaves to the guest */
>      bool cache_info_passthrough;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cbed717..bb9525d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1691,7 +1691,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
>                                    const char *name, Error **errp)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> -    int64_t value = cpu->apic_id;
> +    int64_t value = cpu->apic_id_set ? cpu->apic_id : -1;

Should this return an error if the apic_id was not set?

Paolo

>      visit_type_int(v, &value, name, errp);
>  }
> @@ -1729,6 +1729,7 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>      cpu->apic_id = value;
> +    cpu->apic_id_set = true;
>  }
>  
>  /* Generic getter for "feature-words" and "filtered-features" properties */
> 

Re: [Qemu-devel] [PATCH 7/8] target-i386: Move topology.h to hw/i386/topology.h

[Paolo Bonzini]

On 19/12/2014 03:41, Eduardo Habkost wrote:
> This will allow the PC code to use the header, and lets us eliminate the
> QEMU_INCLUDES hack inside tests/Makefile.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Please use include/hw/i386/topology.h (the toplevel build and source
directories should not be part of the #include path, so it's a bug that
this works).

Paolo

> ---
>  {target-i386 => hw/i386}/topology.h | 6 +++---
>  target-i386/cpu.c                   | 2 +-
>  tests/Makefile                      | 2 --
>  tests/test-x86-cpuid.c              | 2 +-
>  4 files changed, 5 insertions(+), 7 deletions(-)
>  rename {target-i386 => hw/i386}/topology.h (97%)
> 
> diff --git a/target-i386/topology.h b/hw/i386/topology.h
> similarity index 97%
> rename from target-i386/topology.h
> rename to hw/i386/topology.h
> index 07a6c5f..9c6f3a9 100644
> --- a/target-i386/topology.h
> +++ b/hw/i386/topology.h
> @@ -21,8 +21,8 @@
>   * 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
> +#ifndef HW_I386_TOPOLOGY_H
> +#define HW_I386_TOPOLOGY_H
>  
>  /* This file implements the APIC-ID-based CPU topology enumeration logic,
>   * documented at the following document:
> @@ -131,4 +131,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
>      return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
>  }
>  
> -#endif /* TARGET_I386_TOPOLOGY_H */
> +#endif /* HW_I386_TOPOLOGY_H */
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4b6e19b..d8cd7c9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -25,7 +25,7 @@
>  #include "sysemu/kvm.h"
>  #include "sysemu/cpus.h"
>  #include "kvm_i386.h"
> -#include "topology.h"
> +#include "hw/i386/topology.h"
>  
>  #include "qemu/option.h"
>  #include "qemu/config-file.h"
> diff --git a/tests/Makefile b/tests/Makefile
> index e4ddb6a..bdc7cc5 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -232,8 +232,6 @@ $(test-obj-y): QEMU_INCLUDES += -Itests
>  QEMU_CFLAGS += -I$(SRC_PATH)/tests
>  qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
>  
> -tests/test-x86-cpuid.o: QEMU_INCLUDES += -I$(SRC_PATH)/target-i386
> -
>  tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
>  tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
>  tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
> diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
> index 8d9f96a..6cd20d4 100644
> --- a/tests/test-x86-cpuid.c
> +++ b/tests/test-x86-cpuid.c
> @@ -24,7 +24,7 @@
>  
>  #include <glib.h>
>  
> -#include "topology.h"
> +#include "hw/i386/topology.h"
>  
>  static void test_topo_bits(void)
>  {
> 

Re: [Qemu-devel] [PATCH 4/8] target-i386: Keep track of apic-id setting

[Eduardo Habkost]

On Fri, Dec 19, 2014 at 12:23:07PM +0100, Paolo Bonzini wrote:
> On 19/12/2014 03:41, Eduardo Habkost wrote:
> > Set a flag indicating that the apic-id property was set, so we can later
> > ensure we won't try to realize a X86CPU object before apic-id was set.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Gu Zheng <guz.fnst@cn.fujitsu.com>
> > ---
> >  target-i386/cpu-qom.h | 1 +
> >  target-i386/cpu.c     | 3 ++-
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> > index 4a6f48a..ba0ee15 100644
> > --- a/target-i386/cpu-qom.h
> > +++ b/target-i386/cpu-qom.h
> > @@ -94,6 +94,7 @@ typedef struct X86CPU {
> >      bool migratable;
> >      bool host_features;
> >      uint32_t apic_id;
> > +    bool apic_id_set;
> >  
> >      /* if true the CPUID code directly forward host cache leaves to the guest */
> >      bool cache_info_passthrough;
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index cbed717..bb9525d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1691,7 +1691,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
> >                                    const char *name, Error **errp)
> >  {
> >      X86CPU *cpu = X86_CPU(obj);
> > -    int64_t value = cpu->apic_id;
> > +    int64_t value = cpu->apic_id_set ? cpu->apic_id : -1;
> 
> Should this return an error if the apic_id was not set?

Both approaches look valid to me. But I wouldn't expect a property to
return errors by default when read, I expect it to simply have a default
value (in this case, -1).

If we return errors, a tool/command that reads all properties from
objects would need to treat it as a special case, instead of simply
printing "-1".

Do we have any other cases where we return errors from a property getter
(especially in the default case)? I didn't find any.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 7/8] target-i386: Move topology.h to hw/i386/topology.h

[Eduardo Habkost]

On Fri, Dec 19, 2014 at 12:24:05PM +0100, Paolo Bonzini wrote:
> 
> 
> On 19/12/2014 03:41, Eduardo Habkost wrote:
> > This will allow the PC code to use the header, and lets us eliminate the
> > QEMU_INCLUDES hack inside tests/Makefile.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Please use include/hw/i386/topology.h (the toplevel build and source
> directories should not be part of the #include path, so it's a bug that
> this works).

Oops! That was what I intended to do, but moved it to the wrong
directory. I will fix it in the next version.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 5/8] target-i386: Set APIC ID using cpu_index on CONFIG_USER

[Eduardo Habkost]

On Fri, Dec 19, 2014 at 12:22:30PM +0100, Paolo Bonzini wrote:
> On 19/12/2014 03:41, Eduardo Habkost wrote:
> > +    object_property_set_int(OBJECT(cpu), CPU(cpu)->cpu_index, "apic-id",
> > +                            &error);
> > +    if (error) {
> > +        goto out;
> > +    }
> > +
> 
> Should this use &error_abort?

I normally use &error_abort only if there is no way to indicate errors
to the caller at all. In this case, we print the error message and
return NULL.

However, I was assuming that all callers of cpu_init() check for NULL
properly, and cpu_copy() at linux-user/main.c doesn't. As I don't want
to touch linux-user code or change the cpu_init() signature, I will use
&error_abort in the next version.

-- 
Eduardo