[kvm-unit-tests PATCH 00/11] Misc fixes and CI improvements

[kvm-unit-tests PATCH 00/11] Misc fixes and CI improvements

[Thomas Huth]

Here's a set of accumulated patches that fix the various problems with
our CI pipelines, and then update the Gitlab-CI to use Fedora 32 instead
of 30. Additionally, the new version of Clang in Fedora 32 is finally
also able to compile the kvm-unit-tests (with some small fixes included
in this series), so we can now also add a CI test with this compiler, too.

Andrew Jones (1):
  Fix out-of-tree builds

Bill Wendling (2):
  x86: use a non-negative number in shift
  x86: use inline asm to retrieve stack pointer

Mohammed Gamal (1):
  x86/access: Fix phys-bits parameter

Paolo Bonzini (1):
  x86: avoid multiply defined symbol

Thomas Huth (6):
  Fixes for the umip test
  Always compile the kvm-unit-tests with -fno-common
  Fix powerpc issue with the linker from Fedora 32
  Update the gitlab-ci to Fedora 32
  vmx_tests: Silence warning from Clang
  Compile the kvm-unit-tests also with Clang

 .gitlab-ci.yml       | 17 +++++++++++++++--
 Makefile             |  2 +-
 configure            |  8 +++-----
 lib/auxinfo.h        |  3 +--
 lib/x86/fault_test.c |  2 +-
 lib/x86/usermode.c   |  2 +-
 powerpc/flat.lds     | 19 ++++++++++++++++---
 x86/Makefile.common  |  1 +
 x86/svm_tests.c      |  2 +-
 x86/umip.c           |  6 ++++--
 x86/unittests.cfg    |  2 +-
 x86/vmx_tests.c      | 10 +++++++---
 12 files changed, 52 insertions(+), 22 deletions(-)

-- 
2.18.1

[kvm-unit-tests PATCH 01/11] x86/access: Fix phys-bits parameter

[Thomas Huth]

From: Mohammed Gamal <mgamal@redhat.com>

Some QEMU versions don't support setting phys-bits argument directly.
This causes breakage to Travis CI. Work around the bug by setting
host-phys-bits=on

Fixes: 1a296ac170f ("x86: access: Add tests for reserved bits of guest physical address")

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 x86/unittests.cfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index bf0d02e..d43cac2 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -116,7 +116,7 @@ extra_params = -cpu qemu64,+x2apic,+tsc-deadline -append tscdeadline_immed
 [access]
 file = access.flat
 arch = x86_64
-extra_params = -cpu host,phys-bits=36
+extra_params = -cpu host,host-phys-bits=on,phys-bits=36
 
 [smap]
 file = smap.flat
-- 
2.18.1

[kvm-unit-tests PATCH 02/11] Fix out-of-tree builds

[Thomas Huth]

From: Andrew Jones <drjones@redhat.com>

Since b16df9ee5f3b out-of-tree builds have been broken because we
started validating the newly user-configurable $erratatxt file
before linking it into the build dir. We fix this not by moving
the validation, but by removing the linking and instead using the
full path of the $erratatxt file. This allows one to keep that file
separate from the src and build dirs.

Fixes: b16df9ee5f3b ("arch-run: Add reserved variables to the default environ")
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/configure b/configure
index 5d2cd90..f9d030f 100755
--- a/configure
+++ b/configure
@@ -17,7 +17,7 @@ environ_default=yes
 u32_long=
 vmm="qemu"
 errata_force=0
-erratatxt="errata.txt"
+erratatxt="$srcdir/errata.txt"
 
 usage() {
     cat <<-EOF
@@ -89,7 +89,8 @@ while [[ "$1" = -* ]]; do
 	    environ_default=no
 	    ;;
 	--erratatxt)
-	    erratatxt="$arg"
+	    erratatxt=
+	    [ "$arg" ] && erratatxt=$(eval realpath "$arg")
 	    ;;
 	--help)
 	    usage
@@ -169,9 +170,6 @@ if test ! -e Makefile; then
 
     echo "linking scripts..."
     ln -sf "$srcdir/scripts"
-
-    echo "linking errata.txt..."
-    ln -sf "$srcdir/errata.txt"
 fi
 
 # link lib/asm for the architecture
-- 
2.18.1

[kvm-unit-tests PATCH 03/11] x86: avoid multiply defined symbol

[Thomas Huth]

From: Paolo Bonzini <pbonzini@redhat.com>

Fedora 32 croaks about a symbol that is defined twice, fix it.

Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 lib/x86/fault_test.c | 2 +-
 lib/x86/usermode.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/x86/fault_test.c b/lib/x86/fault_test.c
index 078dae3..e15a218 100644
--- a/lib/x86/fault_test.c
+++ b/lib/x86/fault_test.c
@@ -1,6 +1,6 @@
 #include "fault_test.h"
 
-jmp_buf jmpbuf;
+static jmp_buf jmpbuf;
 
 static void restore_exec_to_jmpbuf(void)
 {
diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index f01ad9b..f032523 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -14,7 +14,7 @@
 #define USERMODE_STACK_SIZE	0x2000
 #define RET_TO_KERNEL_IRQ	0x20
 
-jmp_buf jmpbuf;
+static jmp_buf jmpbuf;
 
 static void restore_exec_to_jmpbuf(void)
 {
-- 
2.18.1

[kvm-unit-tests PATCH 04/11] Fixes for the umip test

[Thomas Huth]

When compiling umip.c with -O2 instead of -O1, there are currently
two problems. First, the compiler complains:

 x86/umip.c: In function ‘do_ring3’:
 x86/umip.c:162:37: error: array subscript 4096 is above array bounds of
    ‘unsigned char[4096]’ [-Werror=array-bounds]
       [user_stack_top]"m"(user_stack[sizeof user_stack]),
                           ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~

This can be fixed by initializing the stack to point to one of the last
bytes of the array instead.

The second problem is that some tests are failing - and this is due
to the fact that the GP_ASM macro uses inline asm without the "volatile"
keyword - so that the compiler reorders this code in certain cases
where it should not. Fix it by adding "volatile" here.

Message-Id: <20200122160944.29750-1-thuth@redhat.com>
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 x86/umip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/x86/umip.c b/x86/umip.c
index 7eee294..37f1ab8 100644
--- a/x86/umip.c
+++ b/x86/umip.c
@@ -22,7 +22,8 @@ static void gp_handler(struct ex_regs *regs)
 
 
 #define GP_ASM(stmt, in, clobber)                  \
-     asm ("mov" W " $1f, %[expected_rip]\n\t"      \
+    asm volatile (                                 \
+          "mov" W " $1f, %[expected_rip]\n\t"      \
           "movl $2f-1f, %[skip_count]\n\t"         \
           "1: " stmt "\n\t"                        \
           "2: "                                    \
@@ -159,7 +160,8 @@ static int do_ring3(void (*fn)(const char *), const char *arg)
 		  : [ret] "=&a" (ret)
 		  : [user_ds] "i" (USER_DS),
 		    [user_cs] "i" (USER_CS),
-		    [user_stack_top]"m"(user_stack[sizeof user_stack]),
+		    [user_stack_top]"m"(user_stack[sizeof(user_stack) -
+						   sizeof(long)]),
 		    [fn]"r"(fn),
 		    [arg]"D"(arg),
 		    [kernel_ds]"i"(KERNEL_DS),
-- 
2.18.1

[kvm-unit-tests PATCH 05/11] Always compile the kvm-unit-tests with -fno-common

[Thomas Huth]

The new GCC v10 uses -fno-common by default. To avoid that we commit
code that declares global variables twice and thus fails to link with
the latest version, we should also compile with -fno-common when using
older versions of the compiler. However, this now also means that we
can not play the trick with the common auxinfo struct anymore. Thus
declare it as extern in the header now and link auxinfo.c on x86, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Makefile            | 2 +-
 lib/auxinfo.h       | 3 +--
 x86/Makefile.common | 1 +
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 754ed65..3ff2f91 100644
--- a/Makefile
+++ b/Makefile
@@ -49,7 +49,7 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile
 cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \
               > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;)
 
-COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing
+COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing -fno-common
 COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
 COMMON_CFLAGS += -Wignored-qualifiers -Werror
 
diff --git a/lib/auxinfo.h b/lib/auxinfo.h
index 08b96f8..a46a1e6 100644
--- a/lib/auxinfo.h
+++ b/lib/auxinfo.h
@@ -13,7 +13,6 @@ struct auxinfo {
 	unsigned long flags;
 };
 
-/* No extern!  Define a common symbol.  */
-struct auxinfo auxinfo;
+extern struct auxinfo auxinfo;
 #endif
 #endif
diff --git a/x86/Makefile.common b/x86/Makefile.common
index ab67ca0..2ea9c9f 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -5,6 +5,7 @@ all: directories test_cases
 cflatobjs += lib/pci.o
 cflatobjs += lib/pci-edu.o
 cflatobjs += lib/alloc.o
+cflatobjs += lib/auxinfo.o
 cflatobjs += lib/vmalloc.o
 cflatobjs += lib/alloc_page.o
 cflatobjs += lib/alloc_phys.o
-- 
2.18.1

[kvm-unit-tests PATCH 06/11] Fix powerpc issue with the linker from Fedora 32

[Thomas Huth]

The linker from Fedora 32 complains:

powerpc64-linux-gnu-ld: powerpc/selftest.elf: error: PHDR segment not
 covered by LOAD segment

Let's introduce some fake PHDRs to the linker script to get this
working again.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 powerpc/flat.lds | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/powerpc/flat.lds b/powerpc/flat.lds
index 53221e8..5eed368 100644
--- a/powerpc/flat.lds
+++ b/powerpc/flat.lds
@@ -1,7 +1,17 @@
 
+PHDRS
+{
+    text PT_LOAD FLAGS(5);
+    data PT_LOAD FLAGS(6);
+}
+
 SECTIONS
 {
-    .text : { *(.init) *(.text) *(.text.*) }
+    .text : {
+        *(.init)
+        *(.text)
+        *(.text.*)
+    } :text
     . = ALIGN(64K);
     etext = .;
     .opd : { *(.opd) }
@@ -19,9 +29,12 @@ SECTIONS
     .data : {
         *(.data)
         *(.data.rel*)
-    }
+    } :data
     . = ALIGN(16);
-    .rodata : { *(.rodata) *(.rodata.*) }
+    .rodata : {
+        *(.rodata)
+        *(.rodata.*)
+    } :data
     . = ALIGN(16);
     .bss : { *(.bss) }
     . = ALIGN(256);
-- 
2.18.1

[kvm-unit-tests PATCH 07/11] Update the gitlab-ci to Fedora 32

[Thomas Huth]

Fedora 30 is end of life, let's use the version 32 instead.

Unfortunately, we have to disable taskswitch2 in the gitlab-ci now.
It does not seem to work anymore with the latest version of gcc and/or
QEMU. We still check it in the travis-ci, though, so until somebody has
some spare time to debug this issue, it should be ok to disable it here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .gitlab-ci.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 3093239..13e1a1f 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,4 +1,4 @@
-image: fedora:30
+image: fedora:32
 
 before_script:
  - dnf update -y
@@ -77,6 +77,6 @@ build-i386:
  - ./configure --arch=i386
  - make -j2
  - ACCEL=tcg ./run_tests.sh
-     cmpxchg8b eventinj port80 setjmp sieve tsc taskswitch taskswitch2 umip
+     cmpxchg8b eventinj port80 setjmp sieve tsc taskswitch umip
      | tee results.txt
  - if grep -q FAIL results.txt ; then exit 1 ; fi
-- 
2.18.1

[kvm-unit-tests PATCH 08/11] x86: use a non-negative number in shift

[Thomas Huth]

From: Bill Wendling <morbo@google.com>

Shifting a negative number is undefined. Clang complains about it:

x86/svm.c:1131:38: error: shifting a negative signed value is undefined [-Werror,-Wshift-negative-value]
    test->vmcb->control.tsc_offset = TSC_OFFSET_VALUE;

Using "~0ull" results in identical asm code:

	before: movabsq $-281474976710656, %rsi
	after:  movabsq $-281474976710656, %rsi

Signed-off-by: Bill Wendling <morbo@google.com>
[thuth: Rebased to master - code is in svm_tests.c instead of svm.c now]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 x86/svm_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 2f53b8f..be1bddf 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -844,7 +844,7 @@ static bool npt_rw_l1mmio_check(struct svm_test *test)
 }
 
 #define TSC_ADJUST_VALUE    (1ll << 32)
-#define TSC_OFFSET_VALUE    (-1ll << 48)
+#define TSC_OFFSET_VALUE    (~0ull << 48)
 static bool ok;
 
 static void tsc_adjust_prepare(struct svm_test *test)
-- 
2.18.1

[kvm-unit-tests PATCH 09/11] x86: use inline asm to retrieve stack pointer

[Thomas Huth]

From: Bill Wendling <morbo@google.com>

According to GCC's documentation, the only supported use for specifying
registers for local variables is "to specify registers for input and
output operands when calling Extended asm." Using it as a shortcut to
get the value in a register isn't guaranteed to work, and clang
complains that the variable is uninitialized.

Signed-off-by: Bill Wendling <morbo@google.com>
Message-Id: <20191030210419.213407-7-morbo@google.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 x86/vmx_tests.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 0909adb..dbf5375 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2135,7 +2135,9 @@ static void into_guest_main(void)
 		.offset = (uintptr_t)&&into,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp;
+
+	asm volatile ("mov %%rsp, %0" : "=r"(rsp));
 
 	if (fp.offset != (uintptr_t)&&into) {
 		printf("Code address too high.\n");
@@ -3231,7 +3233,9 @@ static void try_compat_invvpid(void *unused)
 		.offset = (uintptr_t)&&invvpid,
 		.selector = KERNEL_CS32,
 	};
-	register uintptr_t rsp asm("rsp");
+	uintptr_t rsp;
+
+	asm volatile ("mov %%rsp, %0" : "=r"(rsp));
 
 	TEST_ASSERT_MSG(fp.offset == (uintptr_t)&&invvpid,
 			"Code address too high.");
-- 
2.18.1

[kvm-unit-tests PATCH 10/11] vmx_tests: Silence warning from Clang

[Thomas Huth]

Clang complains:

x86/vmx_tests.c:8429:40: error: converting the result of '<<' to a boolean
 always evaluates to true [-Werror,-Wtautological-constant-compare]
         vmx_preemption_timer_zero_inject_db(1 << DB_VECTOR);
                                               ^

Looking at the code, the "1 << DB_VECTOR" is done within the function
vmx_preemption_timer_zero_inject_db() indeed:

	vmcs_write(EXC_BITMAP, intercept_db ? 1 << DB_VECTOR : 0);

... so using "true" as parameter for the function should be appropriate
here.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 x86/vmx_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index dbf5375..c38115c 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -8426,7 +8426,7 @@ static void vmx_preemption_timer_zero_test(void)
 	 * an event that you injected.
 	 */
 	vmx_set_test_stage(1);
-	vmx_preemption_timer_zero_inject_db(1 << DB_VECTOR);
+	vmx_preemption_timer_zero_inject_db(true);
 	vmx_preemption_timer_zero_expect_preempt_at_rip(db_fault_address);
 	vmx_preemption_timer_zero_advance_past_vmcall();
 
-- 
2.18.1

[kvm-unit-tests PATCH 11/11] Compile the kvm-unit-tests also with Clang

[Thomas Huth]

To get some more test coverage, let's check compilation with Clang, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 .gitlab-ci.yml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 13e1a1f..3af53f0 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -80,3 +80,16 @@ build-i386:
      cmpxchg8b eventinj port80 setjmp sieve tsc taskswitch umip
      | tee results.txt
  - if grep -q FAIL results.txt ; then exit 1 ; fi
+
+build-clang:
+ script:
+ - dnf install -y qemu-system-x86 clang
+ - ./configure --arch=x86_64 --cc=clang
+ - make -j2
+ - ACCEL=tcg ./run_tests.sh
+     smptest smptest3 vmexit_cpuid vmexit_mov_from_cr8
+     vmexit_mov_to_cr8 vmexit_inl_pmtimer  vmexit_ipi vmexit_ipi_halt
+     vmexit_ple_round_robin vmexit_tscdeadline vmexit_tscdeadline_immed
+     eventinj msr port80 setjmp syscall tsc rmap_chain umip intel_iommu
+     | tee results.txt
+ - grep -q PASS results.txt && ! grep -q FAIL results.txt
-- 
2.18.1