CI-related fixes and improvements

Hi Paolo,

the following changes since commit 4b74c718c57d1697e3228e2c699b0fe9c1d24e97:

x86/pmu: Fix compilation on 32-bit hosts (2020-06-16 08:29:34 -0400)

are available in the Git repository at:

https://gitlab.com/huth/kvm-unit-tests.git tags/pull-request-2020-06-16

for you to fetch changes up to 9d7f9a2f0b3427bbeda242c64f698d14e21d2a42:

s390x: stsi: Make output tap13 compatible (2020-06-16 20:22:14 +0200)

----------------------------------------------------------------
* Lots of CI-related fixes and improvements
* Update the gitlab-CI to Fedora 32
* Test compilation with Clang
----------------------------------------------------------------

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

Janosch Frank (1):

      s390x: stsi: Make output tap13 compatible

Paolo Bonzini (2):
x86: avoid multiple defined symbol
x86: disable SSE on 32-bit hosts

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 ++++++++++++++++---
 s390x/stsi.c         |  6 +++---
 x86/Makefile.common  |  1 +
 x86/Makefile.i386    |  1 +
 x86/svm_tests.c      |  2 +-
 x86/umip.c           |  6 ++++--
 x86/vmx_tests.c      | 10 +++++++---
 13 files changed, 55 insertions(+), 24 deletions(-)

From: Andrew Jones <[email protected]>

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.

Message-Id: <[email protected]>

---
 configure | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

index 5d2cd90..f9d030f 100755
@@ -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

From: Paolo Bonzini <[email protected]>

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

Message-Id: <[email protected]>

---
 lib/x86/fault_test.c | 2 +-
 lib/x86/usermode.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 078dae3..e15a218 100644
@@ -1,6 +1,6 @@
 #include "fault_test.h"
 
-jmp_buf jmpbuf;
+static jmp_buf jmpbuf;
 
 static void restore_exec_to_jmpbuf(void)
 {

index f01ad9b..f032523 100644
@@ -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

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: <[email protected]>

---
 x86/umip.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index afb373d..c5700b3 100644
@@ -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

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.

Message-Id: <[email protected]>

---
 Makefile            | 2 +-
 lib/auxinfo.h       | 3 +--
 x86/Makefile.common | 1 +
 3 files changed, 3 insertions(+), 3 deletions(-)

index 754ed65..3ff2f91 100644
@@ -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
 

index 08b96f8..a46a1e6 100644
@@ -13,7 +13,6 @@ struct auxinfo {
 	unsigned long flags;
 };
 
-/* No extern!  Define a common symbol.  */
-struct auxinfo auxinfo;
+extern struct auxinfo auxinfo;
 #endif
 #endif

index ab67ca0..2ea9c9f 100644
@@ -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

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.

Message-Id: <[email protected]>

---
 powerpc/flat.lds | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

index 53221e8..5eed368 100644
@@ -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

From: Bill Wendling <[email protected]>

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

Message-Id: <[email protected]>

---
 x86/svm_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c1abd55..a645d66 100644
@@ -890,7 +890,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

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.

Message-Id: <[email protected]>

---
 .gitlab-ci.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 3093239..13e1a1f 100644
@@ -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

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.

Message-Id: <[email protected]>

---
 x86/vmx_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index fcd97a1..36e94fa 100644
@@ -8544,7 +8544,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

From: Paolo Bonzini <[email protected]>

On 64-bit hosts we are disabling SSE and SSE2. Depending on the compiler however it may use movq instructions for 64-bit transfers even when targeting 32-bit processors; when CR4.OSFXSR is not set, this results in an undefined opcode exception, so tell the compiler to avoid those instructions on 32-bit hosts as well.

Message-Id: <[email protected]>

---
 x86/Makefile.i386 | 1 +
 1 file changed, 1 insertion(+)

index d801b80..be9d6bc 100644
@@ -1,6 +1,7 @@
 cstart.o = $(TEST_DIR)/cstart.o
 bits = 32
 ldarch = elf32-i386
+COMMON_CFLAGS += -mno-sse -mno-sse2
 
 cflatobjs += lib/x86/setjmp32.o
 

2.18.1

From: Janosch Frank <[email protected]>

In tap13 output # is a special character and only "skip" and "todo" are allowed to come after it. Let's appease our CI environment and replace # with "count".

Message-Id: <[email protected]>

---
 s390x/stsi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 66b4257..b81cea7 100644
@@ -129,11 +129,11 @@ static void test_3_2_2(void)
 	}
 
 	report(!memcmp(data->vm[0].uuid, uuid, sizeof(uuid)), "uuid");
-	report(data->vm[0].conf_cpus == smp_query_num_cpus(), "cpu # configured");
+	report(data->vm[0].conf_cpus == smp_query_num_cpus(), "cpu count configured");
 	report(data->vm[0].total_cpus ==
 	       data->vm[0].reserved_cpus + data->vm[0].conf_cpus,
-	       "cpu # total == conf + reserved");
-	report(data->vm[0].standby_cpus == 0, "cpu # standby");
+	       "cpu count total == conf + reserved");
+	report(data->vm[0].standby_cpus == 0, "cpu count standby");
 	report(!memcmp(data->vm[0].name, vm_name, sizeof(data->vm[0].name)),
 	       "VM name == kvm-unit-test");
 

2.18.1

From: Bill Wendling <[email protected]>

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.

Message-Id: <[email protected]>

---
 x86/vmx_tests.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index 68f93d3..fcd97a1 100644
@@ -2253,7 +2253,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");

@@ -3349,7 +3351,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

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

Message-Id: <[email protected]>

---
 .gitlab-ci.yml | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 13e1a1f..3af53f0 100644
@@ -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

Pulled, thanks.

Paolo