[Qemu-devel] [PULL 0/7] X86 patches

[Qemu-devel] [PULL 0/7] X86 patches

[Eduardo Habkost]

The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:

  Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' into staging (2015-03-09 14:04:14 +0000)

are available in the git repository at:

  https://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to 9886e834f47adabdbfd54ab606788ce7326e6779:

  target-i386: Require APIC ID to be explicitly set before CPU realize (2015-03-09 16:30:03 -0300)

----------------------------------------------------------------
X86 patches queued in the last few weeks. Mostly code cleanup and changes on
code assigning APIC ID.

----------------------------------------------------------------

Eduardo Habkost (7):
  target-i386: Move topology.h to include/hw/i386
  target-i386: Simplify listflags() function
  target-i386: Eliminate unnecessary get_cpuid_vendor() function
  target-i386: Remove unused APIC ID default code
  target-i386: Move CPUX86State::cpuid_apic_id to X86CPU::apic_id
  target-i386: Move APIC ID compatibility code to pc.c
  target-i386: Require APIC ID to be explicitly set before CPU realize

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

-- 
2.1.0

[Qemu-devel] [PULL 1/7] target-i386: Move topology.h to include/hw/i386

[Eduardo Habkost]

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

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 {target-i386 => include/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 => include/hw/i386}/topology.h (97%)

diff --git a/target-i386/topology.h b/include/hw/i386/topology.h
similarity index 97%
rename from target-i386/topology.h
rename to include/hw/i386/topology.h
index 07a6c5f..9c6f3a9 100644
--- a/target-i386/topology.h
+++ b/include/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 d543e2b..8fc5727 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 307035c..7d4b96d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -239,8 +239,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)
 {
-- 
2.1.0

[Qemu-devel] [PULL 2/7] target-i386: Simplify listflags() function

[Eduardo Habkost]

listflags() had lots of unnecessary complexity. Instead of printing to a
buffer that will be immediately printed, simply call the printing
function directly. Also, remove the fbits and flags arguments that were
always set to the same value. Also, there's no need to list the flags in
reverse order.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8fc5727..80e9b9d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1911,34 +1911,19 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     }
 }
 
-/* generate a composite string into buf of all cpuid names in featureset
- * selected by fbits.  indicate truncation at bufsize in the event of overflow.
- * if flags, suppress names undefined in featureset.
+/* Print all cpuid feature names in featureset
  */
-static void listflags(char *buf, int bufsize, uint32_t fbits,
-                      const char **featureset, uint32_t flags)
-{
-    const char **p = &featureset[31];
-    char *q, *b, bit;
-    int nc;
-
-    b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
-    *buf = '\0';
-    for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
-        if (fbits & 1 << bit && (*p || !flags)) {
-            if (*p)
-                nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
-            else
-                nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
-            if (bufsize <= nc) {
-                if (b) {
-                    memcpy(b, "...", sizeof("..."));
-                }
-                return;
-            }
-            q += nc;
-            bufsize -= nc;
+static void listflags(FILE *f, fprintf_function print, const char **featureset)
+{
+    int bit;
+    bool first = true;
+
+    for (bit = 0; bit < 32; bit++) {
+        if (featureset[bit]) {
+            print(f, "%s%s", first ? "" : " ", featureset[bit]);
+            first = false;
         }
+    }
 }
 
 /* generate CPU information. */
@@ -1963,8 +1948,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
         FeatureWordInfo *fw = &feature_word_info[i];
 
-        listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
-        (*cpu_fprintf)(f, "  %s\n", buf);
+        (*cpu_fprintf)(f, "  ");
+        listflags(f, cpu_fprintf, fw->feat_names);
+        (*cpu_fprintf)(f, "\n");
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PULL 3/7] target-i386: Eliminate unnecessary get_cpuid_vendor() function

[Eduardo Habkost]

The function was used in only two places. In one of them, the function
made the code less readable by requiring temporary te[bcd]x variables.
In the other one we can simply inline the existing code.

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 80e9b9d..2f3a450 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2213,14 +2213,6 @@ void x86_cpudef_setup(void)
     }
 }
 
-static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
-                             uint32_t *ecx, uint32_t *edx)
-{
-    *ebx = env->cpuid_vendor1;
-    *edx = env->cpuid_vendor2;
-    *ecx = env->cpuid_vendor3;
-}
-
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx)
@@ -2254,7 +2246,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     switch(index) {
     case 0:
         *eax = env->cpuid_level;
-        get_cpuid_vendor(env, ebx, ecx, edx);
+        *ebx = env->cpuid_vendor1;
+        *edx = env->cpuid_vendor2;
+        *ecx = env->cpuid_vendor3;
         break;
     case 1:
         *eax = env->cpuid_version;
@@ -2447,11 +2441,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
          * So dont set it here for Intel to make Linux guests happy.
          */
         if (cs->nr_cores * cs->nr_threads > 1) {
-            uint32_t tebx, tecx, tedx;
-            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
-            if (tebx != CPUID_VENDOR_INTEL_1 ||
-                tedx != CPUID_VENDOR_INTEL_2 ||
-                tecx != CPUID_VENDOR_INTEL_3) {
+            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
+                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
+                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
-- 
2.1.0

[Qemu-devel] [PULL 4/7] target-i386: Remove unused APIC ID default code

[Eduardo Habkost]

The existing apic_id = cpu_index code has no visible effect: the PC code
already initializes the APIC ID according to the topology on
pc_new_cpu(), and linux-user memcpy()s the CPU state (including
cpuid_apic_id) on cpu_copy().

Remove the dead code and simply let APIC ID to to be 0 by default. This
doesn't change behavior of PC because apic-id is already explicitly set,
and doesn't affect linux-user because APIC ID was already always 0.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2f3a450..3e1a0e7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,7 +2901,6 @@ 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);
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
-- 
2.1.0

[Qemu-devel] [PULL 5/7] target-i386: Move CPUX86State::cpuid_apic_id to X86CPU::apic_id

[Eduardo Habkost]

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

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  1 +
 target-i386/cpu.c     | 15 +++++++--------
 target-i386/cpu.h     |  1 -
 target-i386/kvm.c     |  2 +-
 4 files changed, 9 insertions(+), 10 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 3e1a0e7..6dd74f0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1690,7 +1690,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);
 }
@@ -1723,11 +1723,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 */
@@ -2252,7 +2252,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) {
@@ -2699,7 +2700,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";
@@ -2718,7 +2718,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;
@@ -2914,9 +2914,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 478450c..38bedc2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -944,7 +944,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 40d6a14..27fe2be 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
-- 
2.1.0

[Qemu-devel] [PULL 6/7] 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.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c      | 35 +++++++++++++++++++++++++++++++++++
 target-i386/cpu.c | 34 ----------------------------------
 target-i386/cpu.h |  1 -
 3 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 79eaad5..b5b2aad 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"
@@ -629,6 +631,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.
+ */
+static 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 6dd74f0..110716e 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"
@@ -2822,39 +2821,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);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 38bedc2..0638d24 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1328,7 +1328,6 @@ void x86_cpu_compat_kvm_no_autodisable(FeatureWord w, uint32_t features);
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
 
-uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index);
 void enable_compat_apic_id_mode(void);
 
 #define APIC_DEFAULT_ADDRESS 0xfee00000
-- 
2.1.0

[Qemu-devel] [PULL 7/7] target-i386: Require APIC ID to be explicitly set before CPU realize

[Eduardo Habkost]

On softmuu, instead of setting APIC ID automatically when creating a
X86CPU, require the property to be set before realizing the object
(which is already done by the CPU creation code on PC).

Keep apic_id = 0 by default on *-user so it can simply create a new CPU
object and realize it without extra steps (so target-i386 will be able
to use cpu_generic_init() eventually).

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  2 +-
 target-i386/cpu.c     | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 4a6f48a..31a0c1e 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,7 +93,7 @@ typedef struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
-    uint32_t apic_id;
+    int64_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 110716e..50907d0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2757,6 +2757,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     static bool ht_warned;
 
+    if (cpu->apic_id < 0) {
+        error_setg(errp, "apic-id property was not initialized properly");
+        return;
+    }
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }
@@ -2868,6 +2873,11 @@ static void x86_cpu_initfn(Object *obj)
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
 
+#ifndef CONFIG_USER_ONLY
+    /* Any code creating new X86CPU objects have to set apic-id explicitly */
+    cpu->apic_id = -1;
+#endif
+
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
     /* init various static tables used in TCG mode */
-- 
2.1.0

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Peter Maydell]

On 9 March 2015 at 20:40, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:
>
>   Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' into staging (2015-03-09 14:04:14 +0000)
>
> are available in the git repository at:
>
>   https://github.com/ehabkost/qemu.git tags/x86-pull-request
>
> for you to fetch changes up to 9886e834f47adabdbfd54ab606788ce7326e6779:
>
>   target-i386: Require APIC ID to be explicitly set before CPU realize (2015-03-09 16:30:03 -0300)
>
> ----------------------------------------------------------------
> X86 patches queued in the last few weeks. Mostly code cleanup and changes on
> code assigning APIC ID.
>
> ----------------------------------------------------------------

'make check' fails for the i386 targets, apparently because qemu-system-i386
segfaults on startup:

e104462:trusty:qemu-for-merges$ gdb --args
./build/all/i386-softmmu/qemu-system-i386 -display none
[...]

(gdb) r
Starting program:
/home/petmay01/linaro/qemu-for-merges/build/all/i386-softmmu/qemu-system-i386
-display none

[...]

Program received signal SIGSEGV, Segmentation fault.
object_get_class (obj=obj@entry=0x0) at
/home/petmay01/linaro/qemu-for-merges/qom/object.c:589
589         return obj->class;
(gdb) bt
#0  object_get_class (obj=obj@entry=0x0) at
/home/petmay01/linaro/qemu-for-merges/qom/object.c:589
#1  0x00005555556668c2 in apic_enable_tpr_access_reporting
(dev=<optimized out>, enable=<optimized out>)
    at /home/petmay01/linaro/qemu-for-merges/hw/intc/apic_common.c:86
#2  0x000055555562f881 in flush_queued_work (cpu=0x5555562c6b00)
    at /home/petmay01/linaro/qemu-for-merges/cpus.c:871
#3  qemu_wait_io_event_common (cpu=cpu@entry=0x5555562c6b00)
    at /home/petmay01/linaro/qemu-for-merges/cpus.c:888
#4  0x0000555555630a27 in qemu_tcg_cpu_thread_fn (arg=<optimized out>)
    at /home/petmay01/linaro/qemu-for-merges/cpus.c:1024
#5  0x00007ffff0865182 in start_thread (arg=0x7fffdf99e700) at
pthread_create.c:312
#6  0x00007ffff059200d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:111
(gdb)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Eduardo Habkost]

On Tue, Mar 10, 2015 at 11:44:22AM +0000, Peter Maydell wrote:
> On 9 March 2015 at 20:40, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:
> >
> >   Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' into staging (2015-03-09 14:04:14 +0000)
> >
> > are available in the git repository at:
> >
> >   https://github.com/ehabkost/qemu.git tags/x86-pull-request
> >
> > for you to fetch changes up to 9886e834f47adabdbfd54ab606788ce7326e6779:
> >
> >   target-i386: Require APIC ID to be explicitly set before CPU realize (2015-03-09 16:30:03 -0300)
> >
> > ----------------------------------------------------------------
> > X86 patches queued in the last few weeks. Mostly code cleanup and changes on
> > code assigning APIC ID.
> >
> > ----------------------------------------------------------------
> 
> 'make check' fails for the i386 targets, apparently because qemu-system-i386
> segfaults on startup:

Ouch.

But I couldn't reproduce it on my system using the tree at the tag
above. Was it a merge containing other changes that are not on qemu.git
yet?


> 
> e104462:trusty:qemu-for-merges$ gdb --args
> ./build/all/i386-softmmu/qemu-system-i386 -display none
> [...]
> 
> (gdb) r
> Starting program:
> /home/petmay01/linaro/qemu-for-merges/build/all/i386-softmmu/qemu-system-i386
> -display none
> 
> [...]
> 
> Program received signal SIGSEGV, Segmentation fault.
> object_get_class (obj=obj@entry=0x0) at
> /home/petmay01/linaro/qemu-for-merges/qom/object.c:589
> 589         return obj->class;
> (gdb) bt
> #0  object_get_class (obj=obj@entry=0x0) at
> /home/petmay01/linaro/qemu-for-merges/qom/object.c:589
> #1  0x00005555556668c2 in apic_enable_tpr_access_reporting
> (dev=<optimized out>, enable=<optimized out>)
>     at /home/petmay01/linaro/qemu-for-merges/hw/intc/apic_common.c:86
> #2  0x000055555562f881 in flush_queued_work (cpu=0x5555562c6b00)
>     at /home/petmay01/linaro/qemu-for-merges/cpus.c:871
> #3  qemu_wait_io_event_common (cpu=cpu@entry=0x5555562c6b00)
>     at /home/petmay01/linaro/qemu-for-merges/cpus.c:888
> #4  0x0000555555630a27 in qemu_tcg_cpu_thread_fn (arg=<optimized out>)
>     at /home/petmay01/linaro/qemu-for-merges/cpus.c:1024
> #5  0x00007ffff0865182 in start_thread (arg=0x7fffdf99e700) at
> pthread_create.c:312
> #6  0x00007ffff059200d in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> (gdb)
> 
> thanks
> -- PMM

-- 
Eduardo

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Peter Maydell]

On 10 March 2015 at 14:16, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 11:44:22AM +0000, Peter Maydell wrote:
>> 'make check' fails for the i386 targets, apparently because qemu-system-i386
>> segfaults on startup:
>
> Ouch.
>
> But I couldn't reproduce it on my system using the tree at the tag
> above. Was it a merge containing other changes that are not on qemu.git
> yet?

It was a merge against current head of master.

-- PMM

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Eduardo Habkost]

On Tue, Mar 10, 2015 at 02:19:09PM +0000, Peter Maydell wrote:
> On 10 March 2015 at 14:16, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Mar 10, 2015 at 11:44:22AM +0000, Peter Maydell wrote:
> >> 'make check' fails for the i386 targets, apparently because qemu-system-i386
> >> segfaults on startup:
> >
> > Ouch.
> >
> > But I couldn't reproduce it on my system using the tree at the tag
> > above. Was it a merge containing other changes that are not on qemu.git
> > yet?
> 
> It was a merge against current head of master.

Is it possible to provide a core file, a debug binary, and the exact
source tree ID somewhere? Otherwise I think it will be impossible for us
to move forward with a fix.

-- 
Eduardo

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Peter Maydell]

On 10 March 2015 at 19:45, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Mar 10, 2015 at 02:19:09PM +0000, Peter Maydell wrote:
>> On 10 March 2015 at 14:16, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Tue, Mar 10, 2015 at 11:44:22AM +0000, Peter Maydell wrote:
>> >> 'make check' fails for the i386 targets, apparently because qemu-system-i386
>> >> segfaults on startup:
>> >
>> > Ouch.
>> >
>> > But I couldn't reproduce it on my system using the tree at the tag
>> > above. Was it a merge containing other changes that are not on qemu.git
>> > yet?
>>
>> It was a merge against current head of master.
>
> Is it possible to provide a core file, a debug binary, and the exact
> source tree ID somewhere? Otherwise I think it will be impossible for us
> to move forward with a fix.

Well, I can't repro it at all now, so I've pushed the series to
master. I'm left suspecting maybe something didn't get rebuilt...

-- PMM

Re: [Qemu-devel] [PULL 0/7] X86 patches

[Eduardo Habkost]

On Tue, Mar 10, 2015 at 11:16:36AM -0300, Eduardo Habkost wrote:
> On Tue, Mar 10, 2015 at 11:44:22AM +0000, Peter Maydell wrote:
> > On 9 March 2015 at 20:40, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > The following changes since commit 277263e1b320d759a760ba6c5ea75ec268f929e5:
> > >
> > >   Merge remote-tracking branch 'remotes/agraf/tags/signed-ppc-for-upstream' into staging (2015-03-09 14:04:14 +0000)
> > >
> > > are available in the git repository at:
> > >
> > >   https://github.com/ehabkost/qemu.git tags/x86-pull-request
> > >
> > > for you to fetch changes up to 9886e834f47adabdbfd54ab606788ce7326e6779:
> > >
> > >   target-i386: Require APIC ID to be explicitly set before CPU realize (2015-03-09 16:30:03 -0300)
> > >
> > > ----------------------------------------------------------------
> > > X86 patches queued in the last few weeks. Mostly code cleanup and changes on
> > > code assigning APIC ID.
> > >
> > > ----------------------------------------------------------------
> > 
> > 'make check' fails for the i386 targets, apparently because qemu-system-i386
> > segfaults on startup:
> 
> Ouch.
> 
> But I couldn't reproduce it on my system using the tree at the tag
> above. Was it a merge containing other changes that are not on qemu.git
> yet?
> 

So, as I can't reproduce it I will try to understand what's happening
below:

> 
> > 
> > e104462:trusty:qemu-for-merges$ gdb --args
> > ./build/all/i386-softmmu/qemu-system-i386 -display none
> > [...]
> > 
> > (gdb) r
> > Starting program:
> > /home/petmay01/linaro/qemu-for-merges/build/all/i386-softmmu/qemu-system-i386
> > -display none
> > 
> > [...]
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > object_get_class (obj=obj@entry=0x0) at
> > /home/petmay01/linaro/qemu-for-merges/qom/object.c:589
> > 589         return obj->class;

So, obj is NULL here...

> > (gdb) bt
> > #0  object_get_class (obj=obj@entry=0x0) at
> > /home/petmay01/linaro/qemu-for-merges/qom/object.c:589
> > #1  0x00005555556668c2 in apic_enable_tpr_access_reporting
> > (dev=<optimized out>, enable=<optimized out>)
> >     at /home/petmay01/linaro/qemu-for-merges/hw/intc/apic_common.c:86

That means cpu->apic_state is NULL at:

static void vapic_enable_tpr_reporting(bool enable)
{
    VAPICEnableTPRReporting info = {
        .enable = enable,
    };
    CPUState *cs;
    X86CPU *cpu;

    CPU_FOREACH(cs) {
        cpu = X86_CPU(cs);
        info.apic = cpu->apic_state;
        run_on_cpu(cs, vapic_do_enable_tpr_reporting, &info);
    }
}

vapic_enable_tpr_reporting() is called at:
 * vapic_prepare(), which is called at:
   * vapic_write()
     * Which is vapic_ops.write, so it should be triggered by guest code only
   * vapic_post_load()
     * Which should run only for loadvm/migration
 * vapic_reset()
   * Which is DeviceClass::reset for TYPE_VAPIC


X86CPU::apic_state is set by x86_cpu_apic_create() (which checks for NULL and
reports an error in that case), which is called by x86_cpu_realizefn(), but
only if the CPU has CPUID_APIC set or smp_cpus > 1. CPUID_APIC is set on the
default CPU model (qemu64), so it should be always called.

Also, the TYPE_VAPIC object is created by apic_common_realize(), so no
TYPE_VAPIC object should exist until a TYPE_*_APIC object is created and
realized. So it looks like x86_cpu_apic_create() is really being called.

Maybe what's happening here is that the reset handler for TYPE_VAPIC is being
called too soon, even before x86_cpu_apic_create() returns? But why?

This is not making a lot of sense to me, and the fact that I can't reproduce
the error makes it more difficult.


  
> > #2  0x000055555562f881 in flush_queued_work (cpu=0x5555562c6b00)
> >     at /home/petmay01/linaro/qemu-for-merges/cpus.c:871
> > #3  qemu_wait_io_event_common (cpu=cpu@entry=0x5555562c6b00)
> >     at /home/petmay01/linaro/qemu-for-merges/cpus.c:888
> > #4  0x0000555555630a27 in qemu_tcg_cpu_thread_fn (arg=<optimized out>)
> >     at /home/petmay01/linaro/qemu-for-merges/cpus.c:1024
> > #5  0x00007ffff0865182 in start_thread (arg=0x7fffdf99e700) at
> > pthread_create.c:312
> > #6  0x00007ffff059200d in clone () at
> > ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
> > (gdb)
> > 
> > thanks
> > -- PMM
> 
> -- 
> Eduardo
> 

-- 
Eduardo