[Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Eduardo Habkost]

Changes v4 -> v5:
 * Rewrite fake kvm_arch_get_supported_cpuid() code
 * Add new test to ensure KVM defaults won't override explicit command-line
   options

Changes v3 -> v4:
 * Move target_words_bigendian() prototype back inside tests/x86-stub.c.

Changes v2 -> v3:
 * Extra KVM "host" CPU model test cases
 * Move target_words_bigendian() prototype to exec-all.h

Changes v1 -> v2:
 * Make dependency list of test binary much simpler, now that cpus.o
   was removed.

Eduardo Habkost (7):
  tests: Support target-specific unit tests
  tests: Make test-x86-cpuid target-specific
  tests: Add unit test for X86CPU code
  target-i386: Isolate enabled-by-default features to a separate array
  tests: test-x86-cpu: Add TCG feature bit initialization test
  tests: test-x86-cpu: Add KVM feature bit initialization test
  tests: test-x86-cpu: Ensure that -cpu override KVM-specific defaults

 target-i386/cpu.c    |  12 ++-
 tests/.gitignore     |   1 +
 tests/Makefile       |  49 +++++++++---
 tests/test-x86-cpu.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/vl-stub.c      |  15 ++++
 tests/x86-stub.c     | 165 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 437 insertions(+), 13 deletions(-)
 create mode 100644 tests/test-x86-cpu.c
 create mode 100644 tests/vl-stub.c
 create mode 100644 tests/x86-stub.c

-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 1/7] tests: Support target-specific unit tests

[Eduardo Habkost]

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

Note that the qtest test cases were per-*arch* (e.g. i386, mips, ppc),
not per-*target* (e.g. i386-softmmu, x86_64-linux-user), because they
implicitly apply only to the -softmmu targets. Target-specific unit
tests, on the other hand, may apply to any target (e.g. they may test
*-softmmu and/or *-user code). To clarify this, $(TARGETS) was renamed
to $(QTEST_ARCHES).

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

diff --git a/tests/Makefile b/tests/Makefile
index 55aa745..33b74a2 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -374,12 +374,27 @@ ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil
 endif
 
+
+SOFTMMU_TARGETS=$(filter %-softmmu,$(TARGET_DIRS))
+SOFTMMU_ARCHES=$(patsubst %-softmmu,%, $(SOFTMMU_TARGETS))
+
+# unit test rules:
+
+# target-specific tests/objs:
+
+test-obj-y += $(foreach TARGET,$(TARGET_DIRS), $(test-obj-$(TARGET)-y))
+check-unit-y += $(foreach TARGET,$(TARGET_DIRS), $(check-unit-$(TARGET)-y))
+
+$(foreach TARGET,$(TARGET_DIRS),$(eval include $(TARGET)/config-target.mak) \
+                                $(eval $(test-obj-$(TARGET)-y): QEMU_CFLAGS += -I$(TARGET) -I$(SRC_PATH)/target-$(TARGET_BASE_ARCH) -DNEED_CPU_H))
+
+$(test-obj-y): QEMU_INCLUDES += -Itests
+
 # QTest rules
 
-TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS)))
 ifeq ($(CONFIG_POSIX),y)
-QTEST_TARGETS=$(foreach TARGET,$(TARGETS), $(if $(check-qtest-$(TARGET)-y), $(TARGET),))
-check-qtest-y=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)-y))
+QTEST_ARCHES=$(foreach ARCH,$(SOFTMMU_ARCHES), $(if $(check-qtest-$(ARCH)-y), $(ARCH),))
+check-qtest-y=$(foreach ARCH,$(QTEST_ARCHES), $(check-qtest-$(ARCH)-y))
 endif
 
 qtest-obj-y = tests/libqtest.o libqemuutil.a libqemustub.a
@@ -411,8 +426,8 @@ GCOV_OPTIONS = -n $(if $(V),-f,)
 
 # gtester tests, possibly with verbose output
 
-.PHONY: $(patsubst %, check-qtest-%, $(QTEST_TARGETS))
-$(patsubst %, check-qtest-%, $(QTEST_TARGETS)): check-qtest-%: $(check-qtest-y)
+.PHONY: $(patsubst %, check-qtest-%, $(QTEST_ARCHES))
+$(patsubst %, check-qtest-%, $(QTEST_ARCHES)): check-qtest-%: $(check-qtest-y)
 	$(if $(CONFIG_GCOV),@rm -f *.gcda */*.gcda */*/*.gcda */*/*/*.gcda,)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
 		MALLOC_PERTURB_=$${MALLOC_PERTURB_:-$$((RANDOM % 255 + 1))} \
@@ -435,7 +450,7 @@ $(patsubst %, check-%, $(check-unit-y)): check-%: %
 
 # gtester tests with XML output
 
-$(patsubst %, check-report-qtest-%.xml, $(QTEST_TARGETS)): check-report-qtest-%.xml: $(check-qtest-y)
+$(patsubst %, check-report-qtest-%.xml, $(QTEST_ARCHES)): check-report-qtest-%.xml: $(check-qtest-y)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
 	  gtester -q $(GTESTER_OPTIONS) -o $@ -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
 
@@ -444,7 +459,7 @@ check-report-unit.xml: $(check-unit-y)
 
 # Reports and overall runs
 
-check-report.xml: $(patsubst %,check-report-qtest-%.xml, $(QTEST_TARGETS)) check-report-unit.xml
+check-report.xml: $(patsubst %,check-report-qtest-%.xml, $(QTEST_ARCHES)) check-report-unit.xml
 	$(call quiet-command,$(SRC_PATH)/scripts/gtester-cat $^ > $@, "  GEN    $@")
 
 check-report.html: check-report.xml
@@ -478,7 +493,7 @@ $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
 
 .PHONY: check-qapi-schema check-qtest check-unit check check-clean
 check-qapi-schema: $(patsubst %,check-%, $(check-qapi-schema-y))
-check-qtest: $(patsubst %,check-qtest-%, $(QTEST_TARGETS))
+check-qtest: $(patsubst %,check-qtest-%, $(QTEST_ARCHES))
 check-unit: $(patsubst %,check-%, $(check-unit-y))
 check-block: $(patsubst %,check-%, $(check-block-y))
 check: check-qapi-schema check-unit check-qtest
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 2/7] tests: Make test-x86-cpuid target-specific

[Eduardo Habkost]

Instead of using a test-specific hack to add -I$(SRC_PATH)/target-i386, add
test-x86-cpuid to $(test-obj-x86_64-softmmu-y).

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

diff --git a/tests/Makefile b/tests/Makefile
index 33b74a2..1a78b86 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -45,7 +45,7 @@ check-unit-y += tests/test-thread-pool$(EXESUF)
 gcov-files-test-thread-pool-y = thread-pool.c
 gcov-files-test-hbitmap-y = util/hbitmap.c
 check-unit-y += tests/test-hbitmap$(EXESUF)
-check-unit-y += tests/test-x86-cpuid$(EXESUF)
+check-unit-x86_64-softmmu-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 gcov-files-test-x86-cpuid-y =
 ifeq ($(CONFIG_SOFTMMU),y)
@@ -235,6 +235,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-opts-visitor.o tests/test-qmp-event.o \
 	tests/rcutorture.o tests/test-rcu-list.o
 
+test-obj-x86_64-softmmu-y = tests/test-x86-cpuid.o
+
 test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 		  tests/test-qapi-event.o
 
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 3/7] tests: Add unit test for X86CPU code

[Eduardo Habkost]

The unit test includes target-i386/cpu.c instead of simply linking
against cpu.o because the test code will use static variables/functions
from cpu.c.

Reasoning for each object file included in the test binary:
 * qom/cpu.o - for TYPE_CPU. Dependencies:
   * qom/qom-qobject.o
 * qom/qdev.o - for TYPE_DEVICE. Dependencies:
   * qom/container.o
   * migration/vmstate.o. Dependencies:
     * migration/qemu-file.o
     * qjson.o
   * hw/core/hotplug.o
   * hw/core/irq.o
   * hw/core/fw-path-provider.o
   * hw/core/qdev-properties.o
 * qom/object.o - for TYPE_OBJECT
 * x86_64-softmmu/target-i386/machine.o - for vmstate_x86_cpu
 * qemu-log.o - for the logging API, used by target-i386/cpu.c
 * libqemuutil.a - for QAPI visitors, error API, and other symbols
 * libqemustub.a - existing stubs, including: savevm, monitor symbols

The remaining symbols used by target-i386/cpu.c were added as stubs to
either tests/vl-stub.c and tests/x86-stub.c.

Note: I couldn't add dependencies that ensure the target-specific object
files are compiled on demand when building the test binary, but "make
check" already requires "make" to be run first because of the qtest test
cases, so I assume this is OK.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v4 -> v5:
* Rewrote kvm_arch_get_supported_cpuid() to simplify test code

Changes v3 -> v4:
* Keep target_words_bigendian() prototype inside x86-stub.c

Changes v1 -> v2:
* Don't include cpus.o on test binary, making lots of stubs now
  unnecessary
---
 tests/.gitignore     |   1 +
 tests/Makefile       |  16 ++++-
 tests/test-x86-cpu.c |  68 +++++++++++++++++++++
 tests/vl-stub.c      |  15 +++++
 tests/x86-stub.c     | 165 +++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 264 insertions(+), 1 deletion(-)
 create mode 100644 tests/test-x86-cpu.c
 create mode 100644 tests/vl-stub.c
 create mode 100644 tests/x86-stub.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 0dcb618..24b5eb4 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -38,5 +38,6 @@ test-vmstate
 test-write-threshold
 test-x86-cpuid
 test-xbzrle
+test-x86-cpu
 *-test
 qapi-schema/*.test.*
diff --git a/tests/Makefile b/tests/Makefile
index 1a78b86..e2b1fcd 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -72,6 +72,7 @@ check-unit-y += tests/test-qemu-opts$(EXESUF)
 gcov-files-test-qemu-opts-y = qom/test-qemu-opts.c
 check-unit-y += tests/test-write-threshold$(EXESUF)
 gcov-files-test-write-threshold-y = block/write-threshold.c
+check-unit-x86_64-softmmu-y += tests/test-x86-cpu$(EXESUF)
 
 check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
 
@@ -235,7 +236,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-opts-visitor.o tests/test-qmp-event.o \
 	tests/rcutorture.o tests/test-rcu-list.o
 
-test-obj-x86_64-softmmu-y = tests/test-x86-cpuid.o
+test-obj-x86_64-softmmu-y = tests/test-x86-cpuid.o \
+	tests/test-x86-cpu.o tests/x86-stub.o
 
 test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 		  tests/test-qapi-event.o
@@ -371,6 +373,18 @@ tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(block-obj-y) libqemuutil.a libqemustub.a
+tests/test-x86-cpu$(EXESUF): tests/test-x86-cpu.o \
+	x86_64-softmmu/target-i386/machine.o \
+	qom/cpu.o \
+	qom/object.o qom/qom-qobject.o qom/container.o \
+	hw/core/qdev.o hw/core/qdev-properties.o \
+	hw/core/hotplug.o hw/core/irq.o hw/core/fw-path-provider.o \
+	migration/vmstate.o migration/qemu-file.o \
+	qemu-log.o \
+	qjson.o \
+	libqemuutil.a \
+	libqemustub.a \
+	tests/vl-stub.o tests/x86-stub.o
 
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil
diff --git a/tests/test-x86-cpu.c b/tests/test-x86-cpu.c
new file mode 100644
index 0000000..1ce7e1d
--- /dev/null
+++ b/tests/test-x86-cpu.c
@@ -0,0 +1,68 @@
+#include <glib.h>
+
+#include "cpu.c"
+
+
+/* stub to avoid requiring hw/intc/apic.o */
+void apic_poll_irq(DeviceState *dev)
+{
+}
+
+/* Special test modes for faking kvm_arch_get_supported_cpuid() results: */
+static bool use_fake_cpuid;  /* Enable fake_cpuid() usage */
+static bool all_unsupported; /* Return all features as unsupported */
+
+/* Fake get_supported_cpuid() which will just return as hash of
+ * function/index/reg. Use a (somewhat poor) hash function to do that.
+ */
+static uint32_t fake_cpuid(uint32_t function, uint32_t index, int reg)
+{
+    uint64_t i = (uint64_t)function * 16ULL * 4ULL + (uint64_t)index * 4 + reg;
+    return i * 2654435761ULL;
+}
+
+uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
+                                      uint32_t index, int reg)
+{
+    if (all_unsupported) {
+        return 0;
+    }
+    if (use_fake_cpuid) {
+        return fake_cpuid(function, index, reg);
+    }
+    /* Default is to simply report all features as supported */
+    return ~0;
+}
+
+static void test_cpu_creation(void)
+{
+    int i;
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
+        ObjectClass *oc;
+        X86CPUClass *xcc;
+        X86CPU *cpu;
+        X86CPUDefinition *def = &builtin_x86_defs[i];
+        char features[] = "";
+
+        oc = x86_cpu_class_by_name(def->name);
+        g_assert_true(oc);
+        xcc = X86_CPU_CLASS(oc);
+        g_assert_true(xcc);
+        cpu = X86_CPU(object_new(object_class_get_name(oc)));
+        x86_cpu_parse_featurestr(CPU(cpu), features, &error_abort);
+        object_unref(OBJECT(cpu));
+    }
+}
+
+int main(int argc, char *argv[])
+{
+    module_call_init(MODULE_INIT_QOM);
+
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/cpu/x86/creation", test_cpu_creation);
+
+    g_test_run();
+
+    return 0;
+}
diff --git a/tests/vl-stub.c b/tests/vl-stub.c
new file mode 100644
index 0000000..32085aa
--- /dev/null
+++ b/tests/vl-stub.c
@@ -0,0 +1,15 @@
+#include "sysemu/sysemu.h"
+#include "sysemu/kvm.h"
+#include "hw/hw.h"
+#include "hw/hw.h"
+
+int smp_cpus = 1;
+int smp_cores = 1;
+int smp_threads = 1;
+bool xen_allowed;
+
+bool tcg_enabled(void)
+{
+    return !kvm_allowed && !xen_allowed;
+}
+
diff --git a/tests/x86-stub.c b/tests/x86-stub.c
new file mode 100644
index 0000000..02b3017
--- /dev/null
+++ b/tests/x86-stub.c
@@ -0,0 +1,165 @@
+/* Stub functions for target-specific code (target-i386 files, cpu-exec.c,
+ * exec.c, etc.) */
+#include "target-i386/cpu.h"
+#include "target-i386/cpu-qom.h"
+#include "target-i386/kvm_i386.h"
+#include "exec/exec-all.h"
+#include "sysemu/kvm.h"
+#include "exec/gdbstub.h"
+
+struct CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
+
+void cpu_exec_init(CPUArchState *env)
+{
+}
+
+void kvm_arch_reset_vcpu(X86CPU *cpu)
+{
+}
+
+void optimize_flags_init(void)
+{
+}
+
+void x86_cpu_do_interrupt(CPUState *cs)
+{
+    abort();
+}
+
+bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+{
+    abort();
+}
+
+void x86_cpu_exec_enter(CPUState *cs)
+{
+    abort();
+}
+
+void x86_cpu_exec_exit(CPUState *cs)
+{
+    abort();
+}
+
+void x86_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
+                        int flags)
+{
+    abort();
+}
+
+int x86_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    abort();
+}
+
+int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
+{
+    abort();
+}
+
+int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    abort();
+}
+
+int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
+                                 void *opaque)
+{
+    abort();
+}
+
+
+int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
+                             int cpuid, void *opaque)
+{
+    abort();
+}
+
+int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cs,
+                                 void *opaque)
+{
+    abort();
+}
+
+void x86_cpu_get_memory_mapping(CPUState *cs, MemoryMappingList *list,
+                                Error **errp)
+{
+    abort();
+}
+
+hwaddr x86_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
+{
+    abort();
+}
+
+void hw_breakpoint_insert(CPUX86State *env, int index)
+{
+    abort();
+}
+
+void cpu_breakpoint_remove_all(CPUState *cpu, int mask)
+{
+    abort();
+}
+
+void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
+{
+    abort();
+}
+
+void breakpoint_handler(CPUState *cs)
+{
+    abort();
+}
+
+void tlb_flush(CPUState *cpu, int flush_global)
+{
+    abort();
+}
+
+void cpu_x86_update_cr0(CPUX86State *env, uint32_t new_cr0)
+{
+    abort();
+}
+
+void cpu_set_fpuc(CPUX86State *env, uint16_t val)
+{
+    abort();
+}
+
+void update_fp_status(CPUX86State *env)
+{
+    abort();
+}
+
+void cpu_get_fp80(uint64_t *pmant, uint16_t *pexp, floatx80 f)
+{
+    abort();
+}
+
+floatx80 cpu_set_fp80(uint64_t mant, uint16_t upper)
+{
+    abort();
+}
+
+uint64_t cpu_get_apic_base(DeviceState *dev)
+{
+    abort();
+}
+
+void apic_designate_bsp(DeviceState *dev)
+{
+    abort();
+}
+
+bool target_words_bigendian(void);
+bool target_words_bigendian(void)
+{
+    return false;
+}
+
+/* Include kvm-stub.c here instead of linking against kvm-stub.o because
+ * kvm-stub.o is built only if CONFIG_KVM is disabled
+ */
+#include "kvm-stub.c"
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 4/7] target-i386: Isolate enabled-by-default features to a separate array

[Eduardo Habkost]

This will make it easier to write unit tests for the feature
initialization logic.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b2d1c95..8557a98 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -481,6 +481,11 @@ static uint32_t kvm_default_unset_features[FEATURE_WORDS] = {
     [FEAT_8000_0001_ECX] = CPUID_EXT3_SVM,
 };
 
+/* Features that are added by default to all CPU models in any accelerator: */
+FeatureWordArray default_features_all = {
+    [FEAT_1_ECX] = CPUID_EXT_HYPERVISOR,
+};
+
 void x86_cpu_compat_kvm_no_autoenable(FeatureWord w, uint32_t features)
 {
     kvm_default_features[w] &= ~features;
@@ -2117,15 +2122,14 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     }
 
     /* Special cases not set in the X86CPUDefinition structs: */
-    if (kvm_enabled()) {
-        FeatureWord w;
-        for (w = 0; w < FEATURE_WORDS; w++) {
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        if (kvm_enabled()) {
             env->features[w] |= kvm_default_features[w];
             env->features[w] &= ~kvm_default_unset_features[w];
         }
+        env->features[w] |= default_features_all[w];
     }
 
-    env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
 
     /* sysenter isn't supported in compatibility mode on AMD,
      * syscall isn't supported in compatibility mode on Intel.
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 5/7] tests: test-x86-cpu: Add TCG feature bit initialization test

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/test-x86-cpu.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/tests/test-x86-cpu.c b/tests/test-x86-cpu.c
index 1ce7e1d..a0923e7 100644
--- a/tests/test-x86-cpu.c
+++ b/tests/test-x86-cpu.c
@@ -54,6 +54,30 @@ static void test_cpu_creation(void)
     }
 }
 
+static void test_cpu_features_tcg(void)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
+        FeatureWord w;
+        ObjectClass *oc;
+        X86CPU *cpu;
+        X86CPUDefinition *def = &builtin_x86_defs[i];
+        char features[] = "";
+
+        oc = x86_cpu_class_by_name(def->name);
+        cpu = X86_CPU(object_new(object_class_get_name(oc)));
+        x86_cpu_parse_featurestr(CPU(cpu), features, &error_abort);
+
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            uint32_t expected = def->features[w] | default_features_all[w];
+            uint32_t actual = cpu->env.features[w];
+            g_assert_cmpint(actual, ==, expected);
+        }
+        object_unref(OBJECT(cpu));
+    }
+}
+
 int main(int argc, char *argv[])
 {
     module_call_init(MODULE_INIT_QOM);
@@ -61,6 +85,7 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/cpu/x86/creation", test_cpu_creation);
+    g_test_add_func("/cpu/x86/features/tcg", test_cpu_features_tcg);
 
     g_test_run();
 
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 6/7] tests: test-x86-cpu: Add KVM feature bit initialization test

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/test-x86-cpu.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/tests/test-x86-cpu.c b/tests/test-x86-cpu.c
index a0923e7..b1b04ea 100644
--- a/tests/test-x86-cpu.c
+++ b/tests/test-x86-cpu.c
@@ -78,6 +78,88 @@ static void test_cpu_features_tcg(void)
     }
 }
 
+static void test_cpu_features_kvm(void)
+{
+    int i;
+    kvm_allowed = true;
+
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
+        FeatureWord w;
+        ObjectClass *oc;
+        X86CPU *cpu;
+        X86CPUDefinition *def = &builtin_x86_defs[i];
+        char features[] = "";
+
+        oc = x86_cpu_class_by_name(def->name);
+        cpu = X86_CPU(object_new(object_class_get_name(oc)));
+        x86_cpu_parse_featurestr(CPU(cpu), features, &error_abort);
+
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            uint32_t expected = (def->features[w] | default_features_all[w] |
+                                 kvm_default_features[w]) &
+                                ~kvm_default_unset_features[w];
+            uint32_t actual = cpu->env.features[w];
+            g_assert_cmpint(actual, ==, expected);
+        }
+        object_unref(OBJECT(cpu));
+    }
+}
+
+static void test_host_cpu_features_kvm(void)
+{
+    FeatureWord w;
+    ObjectClass *oc;
+    X86CPU *cpu;
+    char features1[] = "migratable=off";
+    char features2[] = "migratable=off";
+    char features3[] = "migratable=off";
+
+    kvm_allowed = true;
+    oc = x86_cpu_class_by_name("host");
+
+    /* fake_cpuid() test mode: */
+    use_fake_cpuid = true;
+    cpu = X86_CPU(object_new(object_class_get_name(oc)));
+    x86_cpu_parse_featurestr(CPU(cpu), features1, &error_abort);
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        FeatureWordInfo *wi = &feature_word_info[w];
+        uint32_t expected = fake_cpuid(wi->cpuid_eax, wi->cpuid_ecx,
+                                       wi->cpuid_reg);
+        uint32_t actual = cpu->env.features[w];
+        g_assert_cmpint(cpu->filtered_features[w], ==, 0);
+        g_assert_cmpint(actual, ==, expected);
+    }
+    object_unref(OBJECT(cpu));
+
+    /* all-supported test mode: */
+    use_fake_cpuid = false;
+    cpu = X86_CPU(object_new(object_class_get_name(oc)));
+    x86_cpu_parse_featurestr(CPU(cpu), features2, &error_abort);
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        uint32_t expected = ~0;
+        uint32_t actual = cpu->env.features[w];
+        g_assert_cmpint(cpu->filtered_features[w], ==, 0);
+        g_assert_cmpint(actual, ==, expected);
+    }
+    object_unref(OBJECT(cpu));
+
+    /* all-unsupported test mode: */
+    all_unsupported = true;
+    cpu = X86_CPU(object_new(object_class_get_name(oc)));
+    x86_cpu_parse_featurestr(CPU(cpu), features3, &error_abort);
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        uint32_t expected = 0;
+        uint32_t actual = cpu->env.features[w];
+        g_assert_cmpint(cpu->filtered_features[w], ==, 0);
+        g_assert_cmpint(actual, ==, expected);
+    }
+    object_unref(OBJECT(cpu));
+    all_unsupported = false;
+}
+
 int main(int argc, char *argv[])
 {
     module_call_init(MODULE_INIT_QOM);
@@ -86,6 +168,8 @@ int main(int argc, char *argv[])
 
     g_test_add_func("/cpu/x86/creation", test_cpu_creation);
     g_test_add_func("/cpu/x86/features/tcg", test_cpu_features_tcg);
+    g_test_add_func("/cpu/x86/features/kvm", test_cpu_features_kvm);
+    g_test_add_func("/cpu/x86/features/host", test_host_cpu_features_kvm);
 
     g_test_run();
 
-- 
2.1.0

[Qemu-devel] [PATCH for-2.4 v5 7/7] tests: test-x86-cpu: Ensure that -cpu override KVM-specific defaults

[Eduardo Habkost]

Add a test case to ensure reordering the feature handling code won't
make accelerator-specific defaults override explicit command-line
options. Explicit command-line options should always override the CPU
model defaults.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 tests/test-x86-cpu.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/test-x86-cpu.c b/tests/test-x86-cpu.c
index b1b04ea..618624b 100644
--- a/tests/test-x86-cpu.c
+++ b/tests/test-x86-cpu.c
@@ -160,6 +160,36 @@ static void test_host_cpu_features_kvm(void)
     all_unsupported = false;
 }
 
+/* Ensure that explicit CPU options always override KVM-specific defaults */
+static void test_kvm_override(void)
+{
+    int i;
+    kvm_allowed = true;
+
+    /* Even KVM defaults change on cpu.c, this test relies on x2apic being
+     * always enabled, and monitor being always disabled by KVM:
+     */
+    kvm_default_features[FEAT_1_ECX] |= CPUID_EXT_X2APIC;
+    kvm_default_unset_features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
+
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
+        ObjectClass *oc;
+        X86CPU *cpu;
+        X86CPUDefinition *def = &builtin_x86_defs[i];
+        char features[] = "-x2apic,+monitor";
+
+        oc = x86_cpu_class_by_name(def->name);
+        cpu = X86_CPU(object_new(object_class_get_name(oc)));
+        x86_cpu_parse_featurestr(CPU(cpu), features, &error_abort);
+
+        g_assert_cmpint(cpu->env.features[FEAT_1_ECX] & CPUID_EXT_MONITOR, ==,
+                        CPUID_EXT_MONITOR);
+        g_assert_cmpint(cpu->env.features[FEAT_1_ECX] & CPUID_EXT_X2APIC, ==,
+                        0);
+        object_unref(OBJECT(cpu));
+    }
+}
+
 int main(int argc, char *argv[])
 {
     module_call_init(MODULE_INIT_QOM);
@@ -170,6 +200,7 @@ int main(int argc, char *argv[])
     g_test_add_func("/cpu/x86/features/tcg", test_cpu_features_tcg);
     g_test_add_func("/cpu/x86/features/kvm", test_cpu_features_kvm);
     g_test_add_func("/cpu/x86/features/host", test_host_cpu_features_kvm);
+    g_test_add_func("/cpu/x86/features/kvm-override", test_kvm_override);
 
     g_test_run();
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Andreas Färber]

Am 23.03.2015 um 20:37 schrieb Eduardo Habkost:
> Changes v4 -> v5:
>  * Rewrite fake kvm_arch_get_supported_cpuid() code
>  * Add new test to ensure KVM defaults won't override explicit command-line
>    options
> 
> Changes v3 -> v4:
>  * Move target_words_bigendian() prototype back inside tests/x86-stub.c.
> 
> Changes v2 -> v3:
>  * Extra KVM "host" CPU model test cases
>  * Move target_words_bigendian() prototype to exec-all.h
> 
> Changes v1 -> v2:
>  * Make dependency list of test binary much simpler, now that cpus.o
>    was removed.

I don't recall seeing the previous four versions of this... I think
adding unit tests for whole devices is the wrong approach. We had that
discussion for tmp105. Instead, using the QTest framework - be it
extending my pc-cpu-test or adding a new one - allows to reuse the
device infrastructure in-place in the actual executable without the need
for stubs and without risking to break the test suite by forgetting to
run make check after device changes or forcing work duplication onto others.

Specifically, you added properties to allow inspection of CPUID feature
bits that - as I understood - today no one (including libvirt) is using.

Regards,
Andreas

> Eduardo Habkost (7):
>   tests: Support target-specific unit tests
>   tests: Make test-x86-cpuid target-specific
>   tests: Add unit test for X86CPU code
>   target-i386: Isolate enabled-by-default features to a separate array
>   tests: test-x86-cpu: Add TCG feature bit initialization test
>   tests: test-x86-cpu: Add KVM feature bit initialization test
>   tests: test-x86-cpu: Ensure that -cpu override KVM-specific defaults
> 
>  target-i386/cpu.c    |  12 ++-
>  tests/.gitignore     |   1 +
>  tests/Makefile       |  49 +++++++++---
>  tests/test-x86-cpu.c | 208 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/vl-stub.c      |  15 ++++
>  tests/x86-stub.c     | 165 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 437 insertions(+), 13 deletions(-)
>  create mode 100644 tests/test-x86-cpu.c
>  create mode 100644 tests/vl-stub.c
>  create mode 100644 tests/x86-stub.c

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Eduardo Habkost]

(CCing Jiri Denemark in case he has anything to add about libvirt
requirements)

On Wed, Apr 08, 2015 at 03:06:41PM +0200, Andreas Färber wrote:
> Am 23.03.2015 um 20:37 schrieb Eduardo Habkost:
> > Changes v4 -> v5:
> >  * Rewrite fake kvm_arch_get_supported_cpuid() code
> >  * Add new test to ensure KVM defaults won't override explicit command-line
> >    options
> > 
> > Changes v3 -> v4:
> >  * Move target_words_bigendian() prototype back inside tests/x86-stub.c.
> > 
> > Changes v2 -> v3:
> >  * Extra KVM "host" CPU model test cases
> >  * Move target_words_bigendian() prototype to exec-all.h
> > 
> > Changes v1 -> v2:
> >  * Make dependency list of test binary much simpler, now that cpus.o
> >    was removed.
> 
> I don't recall seeing the previous four versions of this... I think
> adding unit tests for whole devices is the wrong approach. We had that
> discussion for tmp105. Instead, using the QTest framework - be it
> extending my pc-cpu-test or adding a new one - allows to reuse the
> device infrastructure in-place in the actual executable without the need
> for stubs and without risking to break the test suite by forgetting to
> run make check after device changes or forcing work duplication onto others.

The problem with using the actual executable is that it requires
defining the external interfaces before writing tests, and the whole
point of the unit test is to be allow us to check if we are not doing
anything wrong _while implementing_ those external interfaces. This was
mentioned before, see:
  Message-ID: <20140926163410.GA3113@thinpad.lan.raisama.net>
  http://article.gmane.org/gmane.comp.emulators.qemu/299411

Also, I don't even expect external interfaces to be available for some
hooks that this test code require (e.g. changing the return value of
kvm_arch_get_supported_cpuid()).

Personally, I want (and will) run this test after every change made on
target-i386, even if the series is not accepted, but adding the test
code to qemu.git would make life easier for everyone. Having to create a
qemu-x86-unit-tests.git repository for it (even if temporarily) would be
unfortunate.

I don't understand the "forcing work duplication onto others" part.
What kind of work duplication is being forced onto others?

> 
> Specifically, you added properties to allow inspection of CPUID feature
> bits that - as I understood - today no one (including libvirt) is using.

Not sure why this is related to the test code, but:

libvirt didn't use the feature-words property because it requires
re-running QEMU for every CPU model. I don't know why libvirt doesn't
use the filtered-features property yet (as it is a more reliable way to
check if a given CPU configuration is runnable in a host, than the
current libvirt approach of running CPUID directly).

We also found out recently that libvirt have serious issues with the
fact that CPU models may change depending on the machine-type, and their
approach will probably change from "ask QEMU for CPU model information
and see if it matches what libvirt expects" (requiring more information
to flow from QEMU to libvirt through a probing interface) to "ensure
that QEMU will make the CPU match exactly what libvirt expects"
(requiring more information to flow from libvirt to QEMU through a more
flexible configuration interface). That makes the feature flag QOM
properties even more relevant for them.

-- 
Eduardo

Re: [Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Andreas Färber]

Am 08.04.2015 um 15:42 schrieb Eduardo Habkost:
> (CCing Jiri Denemark in case he has anything to add about libvirt
> requirements)
> 
> On Wed, Apr 08, 2015 at 03:06:41PM +0200, Andreas Färber wrote:
>> Am 23.03.2015 um 20:37 schrieb Eduardo Habkost:
>>> Changes v4 -> v5:
>>>  * Rewrite fake kvm_arch_get_supported_cpuid() code
>>>  * Add new test to ensure KVM defaults won't override explicit command-line
>>>    options
>>>
>>> Changes v3 -> v4:
>>>  * Move target_words_bigendian() prototype back inside tests/x86-stub.c.
>>>
>>> Changes v2 -> v3:
>>>  * Extra KVM "host" CPU model test cases
>>>  * Move target_words_bigendian() prototype to exec-all.h
>>>
>>> Changes v1 -> v2:
>>>  * Make dependency list of test binary much simpler, now that cpus.o
>>>    was removed.
>>
>> I don't recall seeing the previous four versions of this... I think
>> adding unit tests for whole devices is the wrong approach. We had that
>> discussion for tmp105. Instead, using the QTest framework - be it
>> extending my pc-cpu-test or adding a new one - allows to reuse the
>> device infrastructure in-place in the actual executable without the need
>> for stubs and without risking to break the test suite by forgetting to
>> run make check after device changes or forcing work duplication onto others.
> 
> The problem with using the actual executable is that it requires
> defining the external interfaces before writing tests, and the whole
> point of the unit test is to be allow us to check if we are not doing
> anything wrong _while implementing_ those external interfaces. This was
> mentioned before, see:
>   Message-ID: <20140926163410.GA3113@thinpad.lan.raisama.net>
>   http://article.gmane.org/gmane.comp.emulators.qemu/299411
> 
> Also, I don't even expect external interfaces to be available for some
> hooks that this test code require (e.g. changing the return value of
> kvm_arch_get_supported_cpuid()).
> 
> Personally, I want (and will) run this test after every change made on
> target-i386, even if the series is not accepted, but adding the test
> code to qemu.git would make life easier for everyone. Having to create a
> qemu-x86-unit-tests.git repository for it (even if temporarily) would be
> unfortunate.

Having code in qemu.git and having it in make check are two separate
things though, and my problem is with the latter.

> I don't understand the "forcing work duplication onto others" part.
> What kind of work duplication is being forced onto others?

Changes to random device etc. infrastructure can break your x86-specific
test case. If people don't expect such cross-subsystem effects then in
the worst case maintainers (me, mst, bonzini, armbru, ...) end up
sending a pull request to Peter, who then needs to reject it based on a
breaking x86 test case. When I work on PReP, I often just run make
check-qtest-ppc because the full suite takes much too long, and I expect
unit tests to be fine-granular like testing only an APIC ID API but not
the whole device infrastructure.

So please don't just think about your own requirements, think of the
consequences this design has for others. If tests don't get usage
because they are not run, that doesn't really help. For QTest we have
gcov for evaluating which APIs are covered, if you start from zero with
your own private framework now, you don't get that and we'll end up
having less ideal coverage in two separate places rather than increasing
coverage in the QTest framework we already have.

>> Specifically, you added properties to allow inspection of CPUID feature
>> bits that - as I understood - today no one (including libvirt) is using.
> 
> Not sure why this is related to the test code, [...]

Because that is *your* existing external interface that you could use to
implement your tests and thereby even give that interface some actual
testing and usage beyond my generic qom-test.

I don't care about libvirt requirements here, this is all about QEMU.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH for-2.4 v5 0/7] Target-specific unit test support, add unit tests for target-i386/cpu.c code

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 04:05:59PM +0200, Andreas Färber wrote:
> Am 08.04.2015 um 15:42 schrieb Eduardo Habkost:
> > (CCing Jiri Denemark in case he has anything to add about libvirt
> > requirements)
> > 
> > On Wed, Apr 08, 2015 at 03:06:41PM +0200, Andreas Färber wrote:
> >> Am 23.03.2015 um 20:37 schrieb Eduardo Habkost:
> >>> Changes v4 -> v5:
> >>>  * Rewrite fake kvm_arch_get_supported_cpuid() code
> >>>  * Add new test to ensure KVM defaults won't override explicit command-line
> >>>    options
> >>>
> >>> Changes v3 -> v4:
> >>>  * Move target_words_bigendian() prototype back inside tests/x86-stub.c.
> >>>
> >>> Changes v2 -> v3:
> >>>  * Extra KVM "host" CPU model test cases
> >>>  * Move target_words_bigendian() prototype to exec-all.h
> >>>
> >>> Changes v1 -> v2:
> >>>  * Make dependency list of test binary much simpler, now that cpus.o
> >>>    was removed.
> >>
> >> I don't recall seeing the previous four versions of this... I think
> >> adding unit tests for whole devices is the wrong approach. We had that
> >> discussion for tmp105. Instead, using the QTest framework - be it
> >> extending my pc-cpu-test or adding a new one - allows to reuse the
> >> device infrastructure in-place in the actual executable without the need
> >> for stubs and without risking to break the test suite by forgetting to
> >> run make check after device changes or forcing work duplication onto others.
> > 
> > The problem with using the actual executable is that it requires
> > defining the external interfaces before writing tests, and the whole
> > point of the unit test is to be allow us to check if we are not doing
> > anything wrong _while implementing_ those external interfaces. This was
> > mentioned before, see:
> >   Message-ID: <20140926163410.GA3113@thinpad.lan.raisama.net>
> >   http://article.gmane.org/gmane.comp.emulators.qemu/299411
> > 
> > Also, I don't even expect external interfaces to be available for some
> > hooks that this test code require (e.g. changing the return value of
> > kvm_arch_get_supported_cpuid()).
> > 
> > Personally, I want (and will) run this test after every change made on
> > target-i386, even if the series is not accepted, but adding the test
> > code to qemu.git would make life easier for everyone. Having to create a
> > qemu-x86-unit-tests.git repository for it (even if temporarily) would be
> > unfortunate.
> 
> Having code in qemu.git and having it in make check are two separate
> things though, and my problem is with the latter.

Understood, so let's understand the problem and try to solve it. If we
can't solve it by now, I will be happy to leave it out of "make check".

> 
> > I don't understand the "forcing work duplication onto others" part.
> > What kind of work duplication is being forced onto others?
> 
> Changes to random device etc. infrastructure can break your x86-specific
> test case.

I want to understand how exactly this could happen. Is this just about
the stubs, or are there additional problems I am not seeing?

> If people don't expect such cross-subsystem effects then in
> the worst case maintainers (me, mst, bonzini, armbru, ...) end up
> sending a pull request to Peter, who then needs to reject it based on a
> breaking x86 test case. When I work on PReP, I often just run make
> check-qtest-ppc because the full suite takes much too long, and I expect
> unit tests to be fine-granular like testing only an APIC ID API but not
> the whole device infrastructure.

This looks like a general problem with all unit tests for C files that
have too many dependencies?

Note that I do not want to test the whole device infrastructure: I just
want to test the feature flag initialization and filtering code that is
inside target-i386/cpu.c. And the most important part is: I want to do
it before making changes to that code (so having to heavily refactor the
code before writing the tests is not an option for me).

So, my current problem is: I want to be able to write unit tests for
functions that are currently inside target-i386/cpu.c without the
problems you mentioned. Is there a way to do that?

> 
> So please don't just think about your own requirements, think of the
> consequences this design has for others. If tests don't get usage
> because they are not run, that doesn't really help. For QTest we have
> gcov for evaluating which APIs are covered, if you start from zero with
> your own private framework now, you don't get that and we'll end up
> having less ideal coverage in two separate places rather than increasing
> coverage in the QTest framework we already have.

I am all for increasing QTest coverage when possible, I don't want a
private framework. Right now I just want to write unit tests that check
how a (preferably small) chunk of code behaves, before starting making
changes to implement additional external interfaces.

> 
> >> Specifically, you added properties to allow inspection of CPUID feature
> >> bits that - as I understood - today no one (including libvirt) is using.
> > 
> > Not sure why this is related to the test code, [...]
> 
> Because that is *your* existing external interface that you could use to
> implement your tests and thereby even give that interface some actual
> testing and usage beyond my generic qom-test.

I don't understand that part of your argument. The existing interface is
necessary but not sufficient for what I want to test. See, for example,
my comment about kvm_arch_get_supported_cpuid() above.

-- 
Eduardo