[PATCH kvm-unit-tests 0/3] use setjmp/longjmp to catch exceptions

[PATCH kvm-unit-tests 0/3] use setjmp/longjmp to catch exceptions

[Paolo Bonzini]

This is an attempt to fix David's reported problem with set_exception_return
and make it more robust.

Patch 1 introduces setjmp; patches 2 and 3 replace test_for_exception
and set_exception_return with setjmp/longjmp.  Patch 4 provides further
cleanups.

Paolo

Paolo Bonzini (4):
  lib: add setjmp header and x86 implementation
  x86: replace set_exception_return with longjmp-based implementation
  x86: remove test_for_exception
  x86: apic: cleanup

 config/config-i386.mak       |  2 ++
 config/config-x86-common.mak |  4 +++-
 config/config-x86_64.mak     |  2 ++
 lib/setjmp.h                 | 12 +++++++++++
 lib/x86/desc.c               | 24 +++++++++------------
 lib/x86/desc.h               |  8 ++++---
 lib/x86/setjmp32.S           | 25 ++++++++++++++++++++++
 lib/x86/setjmp64.S           | 27 ++++++++++++++++++++++++
 x86/apic.c                   | 50 ++++++++++++++++++++------------------------
 x86/setjmp.c                 | 19 +++++++++++++++++
 x86/vmx.c                    | 43 ++++++++++++++++++++++---------------
 11 files changed, 154 insertions(+), 62 deletions(-)
 create mode 100644 lib/setjmp.h
 create mode 100644 lib/x86/setjmp32.S
 create mode 100644 lib/x86/setjmp64.S
 create mode 100644 x86/setjmp.c

-- 
2.5.0

[PATCH kvm-unit-tests 1/4] lib: add setjmp header and x86 implementation

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 config/config-i386.mak       |  2 ++
 config/config-x86-common.mak |  4 +++-
 config/config-x86_64.mak     |  2 ++
 lib/setjmp.h                 | 12 ++++++++++++
 lib/x86/setjmp32.S           | 25 +++++++++++++++++++++++++
 lib/x86/setjmp64.S           | 27 +++++++++++++++++++++++++++
 x86/setjmp.c                 | 19 +++++++++++++++++++
 7 files changed, 90 insertions(+), 1 deletion(-)
 create mode 100644 lib/setjmp.h
 create mode 100644 lib/x86/setjmp32.S
 create mode 100644 lib/x86/setjmp64.S
 create mode 100644 x86/setjmp.c

diff --git a/config/config-i386.mak b/config/config-i386.mak
index 691381c..e353387 100644
--- a/config/config-i386.mak
+++ b/config/config-i386.mak
@@ -3,6 +3,8 @@ bits = 32
 ldarch = elf32-i386
 CFLAGS += -I $(KERNELDIR)/include
 
+cflatobjs += lib/x86/setjmp32.o
+
 tests = $(TEST_DIR)/taskswitch.flat $(TEST_DIR)/taskswitch2.flat \
 	$(TEST_DIR)/cmpxchg8b.flat
 
diff --git a/config/config-x86-common.mak b/config/config-x86-common.mak
index f64874d..2bb2f46 100644
--- a/config/config-x86-common.mak
+++ b/config/config-x86-common.mak
@@ -34,7 +34,7 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
                $(TEST_DIR)/realmode.flat $(TEST_DIR)/msr.flat \
                $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
                $(TEST_DIR)/kvmclock_test.flat  $(TEST_DIR)/eventinj.flat \
-               $(TEST_DIR)/s3.flat $(TEST_DIR)/pmu.flat \
+               $(TEST_DIR)/s3.flat $(TEST_DIR)/pmu.flat $(TEST_DIR)/setjmp.flat \
                $(TEST_DIR)/tsc_adjust.flat $(TEST_DIR)/asyncpf.flat \
                $(TEST_DIR)/init.flat $(TEST_DIR)/smap.flat \
                $(TEST_DIR)/hyperv_synic.flat
@@ -115,6 +115,8 @@ $(TEST_DIR)/memory.elf: $(cstart.o) $(TEST_DIR)/memory.o
 
 $(TEST_DIR)/hyperv_synic.elf: $(cstart.o) $(TEST_DIR)/hyperv_synic.o
 
+$(TEST_DIR)/setjmp.elf: $(cstart.o) $(TEST_DIR)/setjmp.o
+
 arch_clean:
 	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
 	$(TEST_DIR)/.*.d lib/x86/.*.d
diff --git a/config/config-x86_64.mak b/config/config-x86_64.mak
index 1764701..d190be8 100644
--- a/config/config-x86_64.mak
+++ b/config/config-x86_64.mak
@@ -3,6 +3,8 @@ bits = 64
 ldarch = elf64-x86-64
 CFLAGS += -mno-red-zone
 
+cflatobjs += lib/x86/setjmp64.o
+
 tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
 	  $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
 	  $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \
diff --git a/lib/setjmp.h b/lib/setjmp.h
new file mode 100644
index 0000000..334f466
--- /dev/null
+++ b/lib/setjmp.h
@@ -0,0 +1,12 @@
+#ifndef LIBCFLAT_SETJMP_H
+#define LIBCFLAT_SETJMP_H 1
+
+typedef struct jmp_buf_tag {
+	long int regs[8];
+} jmp_buf[1];
+
+extern int setjmp (struct jmp_buf_tag env[1]);
+extern void longjmp (struct jmp_buf_tag env[1], int val)
+     __attribute__ ((__noreturn__));
+
+#endif /* setjmp.h  */
diff --git a/lib/x86/setjmp32.S b/lib/x86/setjmp32.S
new file mode 100644
index 0000000..b0be7c2
--- /dev/null
+++ b/lib/x86/setjmp32.S
@@ -0,0 +1,25 @@
+.globl setjmp
+setjmp:
+	mov (%esp), %ecx	// get return EIP
+	mov 4(%esp), %eax	// get jmp_buf
+	mov %ecx, (%eax)
+	mov %esp, 4(%eax)
+	mov %ebp, 8(%eax)
+	mov %ebx, 12(%eax)
+	mov %esi, 16(%eax)
+	mov %edi, 20(%eax)
+	xor %eax, %eax
+	ret
+
+.globl longjmp
+longjmp:
+	mov 8(%esp), %eax	// get return value
+	mov 4(%esp), %ecx	// get jmp_buf
+	mov 20(%ecx), %edi
+	mov 16(%ecx), %esi
+	mov 12(%ecx), %ebx
+	mov 8(%ecx), %ebp
+	mov 4(%ecx), %esp
+	mov (%ecx), %ecx	// get saved EIP
+	mov %ecx, (%esp)	// and store it on the stack
+	ret
diff --git a/lib/x86/setjmp64.S b/lib/x86/setjmp64.S
new file mode 100644
index 0000000..c8ae790
--- /dev/null
+++ b/lib/x86/setjmp64.S
@@ -0,0 +1,27 @@
+.globl setjmp
+setjmp:
+	mov (%rsp), %rsi
+	mov %rsi, (%rdi)
+	mov %rsp, 0x8(%rdi)
+	mov %rbp, 0x10(%rdi)
+	mov %rbx, 0x18(%rdi)
+	mov %r12, 0x20(%rdi)
+	mov %r13, 0x28(%rdi)
+	mov %r14, 0x30(%rdi)
+	mov %r15, 0x38(%rdi)
+	xor %eax, %eax
+	ret
+
+.globl longjmp
+longjmp:
+	mov %esi, %eax
+	mov 0x38(%rdi), %r15
+	mov 0x30(%rdi), %r14
+	mov 0x28(%rdi), %r13
+	mov 0x20(%rdi), %r12
+	mov 0x18(%rdi), %rbx
+	mov 0x10(%rdi), %rbp
+	mov 0x8(%rdi), %rsp
+	mov (%rdi), %rsi
+	mov %rsi, (%rsp)
+	ret
diff --git a/x86/setjmp.c b/x86/setjmp.c
new file mode 100644
index 0000000..46f0d9c
--- /dev/null
+++ b/x86/setjmp.c
@@ -0,0 +1,19 @@
+#include "stdio.h"
+#include "setjmp.h"
+
+int main()
+{
+    volatile int i;
+    jmp_buf j;
+
+    if (setjmp(j) == 0) {
+	    i = 0;
+    }
+    printf("%d\n", i);
+    if (++i < 10) {
+	    longjmp(j, 1);
+    }
+
+    printf("done\n");
+    return 0;
+}
-- 
2.5.0

[PATCH kvm-unit-tests 2/4] x86: replace set_exception_return with longjmp-based implementation

[Paolo Bonzini]

set_exception_return forces exceptions handlers to return to a specific
address instead of returning to the instruction address pushed by the
CPU at the time of the exception. The unit tests apic.c and vmx.c use
this functionality to recover from expected exceptions.

When using set_exception_return one would have to be careful not to modify
the stack (such as by doing a function call) as triggering the exception
will likely jump us past the instructions which undo the stack manipulation
(such as a ret).  This is unnecessarily brittle, and C already has a
mechanism to do non-local returns---setjmp.  Now that libcflat includes
an implementation of setjmp, replace set_exception_return with a wrapper
that takes care of restoring the processor flags as well.

Reported-by: David Matlack <dmatlack@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 lib/x86/desc.c | 15 +++++++++++----
 lib/x86/desc.h |  6 +++++-
 x86/apic.c     |  8 ++++----
 x86/vmx.c      | 18 +++++++++---------
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 4760026..acf29e3 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -1,6 +1,7 @@
 #include "libcflat.h"
 #include "desc.h"
 #include "processor.h"
+#include <setjmp.h>
 
 void set_idt_entry(int vec, void *addr, int dpl)
 {
@@ -315,12 +316,18 @@ void setup_alt_stack(void)
 #endif
 
 static bool exception;
-static void *exception_return;
+static jmp_buf *exception_jmpbuf;
+
+static void exception_handler_longjmp(void)
+{
+	longjmp(*exception_jmpbuf, 1);
+}
 
 static void exception_handler(struct ex_regs *regs)
 {
+	/* longjmp must happen after iret, so do not do it now.  */
 	exception = true;
-	regs->rip = (unsigned long)exception_return;
+	regs->rip = (unsigned long)&exception_handler_longjmp;
 }
 
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
@@ -333,7 +340,7 @@ bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 	return exception;
 }
 
-void set_exception_return(void *addr)
+void __set_exception_jmpbuf(jmp_buf *addr)
 {
-	exception_return = addr;
+	exception_jmpbuf = addr;
 }
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index 7733151..be52fd4 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -1,6 +1,8 @@
 #ifndef __IDT_TEST__
 #define __IDT_TEST__
 
+#include <setjmp.h>
+
 void setup_idt(void);
 void setup_alt_stack(void);
 
@@ -155,6 +157,8 @@ void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
 
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 			void *data);
-void set_exception_return(void *addr);
+void __set_exception_jmpbuf(jmp_buf *addr);
+#define set_exception_jmpbuf(jmpbuf) \
+	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
 
 #endif
diff --git a/x86/apic.c b/x86/apic.c
index d4eec52..2e04c82 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -63,10 +63,10 @@ static void test_tsc_deadline_timer(void)
 
 static void do_write_apicbase(void *data)
 {
-    set_exception_return(&&resume);
-    wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
-resume:
-    barrier();
+    jmp_buf jmpbuf;
+    if (set_exception_jmpbuf(jmpbuf) == 0) {
+        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
+    }
 }
 
 void test_enable_x2apic(void)
diff --git a/x86/vmx.c b/x86/vmx.c
index 47593bd..ee9aca8 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -619,19 +619,19 @@ static void init_vmx(void)
 
 static void do_vmxon_off(void *data)
 {
-	set_exception_return(&&resume);
-	vmx_on();
-	vmx_off();
-resume:
-	barrier();
+	jmp_buf jmpbuf;
+	if (set_exception_jmpbuf(jmpbuf) == 0) {
+		vmx_on();
+		vmx_off();
+	}
 }
 
 static void do_write_feature_control(void *data)
 {
-	set_exception_return(&&resume);
-	wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
-resume:
-	barrier();
+	jmp_buf jmpbuf;
+	if (set_exception_jmpbuf(jmpbuf) == 0) {
+		wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
+	}
 }
 
 static int test_vmx_feature_control(void)
-- 
2.5.0

[PATCH kvm-unit-tests 3/4] x86: remove test_for_exception

[Paolo Bonzini]

Test functions know whether an exception was generated simply by checking
the last value returned by set_exception_jmpbuf.  The exception number is
passed to set_exception_jmpbuf so that it can set up the exception handler.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 lib/x86/desc.c | 13 +------------
 lib/x86/desc.h |  8 +++-----
 x86/apic.c     | 34 +++++++++++++++++-----------------
 x86/vmx.c      | 29 +++++++++++++++++++----------
 4 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index acf29e3..acf66b6 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -315,7 +315,6 @@ void setup_alt_stack(void)
 }
 #endif
 
-static bool exception;
 static jmp_buf *exception_jmpbuf;
 
 static void exception_handler_longjmp(void)
@@ -326,21 +325,11 @@ static void exception_handler_longjmp(void)
 static void exception_handler(struct ex_regs *regs)
 {
 	/* longjmp must happen after iret, so do not do it now.  */
-	exception = true;
 	regs->rip = (unsigned long)&exception_handler_longjmp;
 }
 
-bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
-			void *data)
+void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr)
 {
 	handle_exception(ex, exception_handler);
-	exception = false;
-	trigger_func(data);
-	handle_exception(ex, NULL);
-	return exception;
-}
-
-void __set_exception_jmpbuf(jmp_buf *addr)
-{
 	exception_jmpbuf = addr;
 }
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index be52fd4..fceabd8 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -155,10 +155,8 @@ void set_intr_alt_stack(int e, void *fn);
 void print_current_tss_info(void);
 void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
 
-bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
-			void *data);
-void __set_exception_jmpbuf(jmp_buf *addr);
-#define set_exception_jmpbuf(jmpbuf) \
-	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
+void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr);
+#define set_exception_jmpbuf(ex, jmpbuf) \
+	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf((ex), &(jmpbuf)), 0))
 
 #endif
diff --git a/x86/apic.c b/x86/apic.c
index 2e04c82..de19724 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -61,12 +61,18 @@ static void test_tsc_deadline_timer(void)
     }
 }
 
-static void do_write_apicbase(void *data)
+static bool do_write_apicbase(u64 data)
 {
     jmp_buf jmpbuf;
-    if (set_exception_jmpbuf(jmpbuf) == 0) {
-        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
+    int ret;
+    if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
+        wrmsr(MSR_IA32_APICBASE, data);
+        ret = 0;
+    } else {
+        ret = 1;
     }
+    handle_exception(GP_VECTOR, NULL);
+    return ret;
 }
 
 void test_enable_x2apic(void)
@@ -80,24 +86,19 @@ void test_enable_x2apic(void)
         printf("x2apic enabled\n");
 
         report("x2apic enabled to invalid state",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &invalid_state));
+               do_write_apicbase(invalid_state));
         report("x2apic enabled to apic enabled",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &apic_enabled));
+               do_write_apicbase(apic_enabled));
 
         wrmsr(MSR_IA32_APICBASE, APIC_DEFAULT_PHYS_BASE | APIC_BSP);
         report("disabled to invalid state",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &invalid_state));
+               do_write_apicbase(invalid_state));
         report("disabled to x2apic enabled",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &x2apic_enabled));
+               do_write_apicbase(x2apic_enabled));
 
         wrmsr(MSR_IA32_APICBASE, apic_enabled);
         report("apic enabled to invalid state",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &invalid_state));
+               do_write_apicbase(invalid_state));
 
         wrmsr(MSR_IA32_APICBASE, x2apic_enabled);
         apic_write(APIC_SPIV, 0x1ff);
@@ -105,8 +106,7 @@ void test_enable_x2apic(void)
         printf("x2apic not detected\n");
 
         report("enable unsupported x2apic",
-               test_for_exception(GP_VECTOR, do_write_apicbase,
-                                  &x2apic_enabled));
+               do_write_apicbase(x2apic_enabled));
     }
 }
 
@@ -128,11 +128,11 @@ static void test_apicbase(void)
 
     value = orig_apicbase | (1UL << cpuid_maxphyaddr());
     report("reserved physaddr bits",
-           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
+           do_write_apicbase(value));
 
     value = orig_apicbase | 1;
     report("reserved low bits",
-           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
+           do_write_apicbase(value));
 
     wrmsr(MSR_IA32_APICBASE, orig_apicbase);
     apic_write(APIC_SPIV, 0x1ff);
diff --git a/x86/vmx.c b/x86/vmx.c
index ee9aca8..0b2cce0 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -617,21 +617,33 @@ static void init_vmx(void)
 	memset(guest_syscall_stack, 0, PAGE_SIZE);
 }
 
-static void do_vmxon_off(void *data)
+static bool do_vmxon_off(void)
 {
 	jmp_buf jmpbuf;
-	if (set_exception_jmpbuf(jmpbuf) == 0) {
+	bool ret;
+	if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
 		vmx_on();
 		vmx_off();
+		return 0;
+	} else {
+		return 1;
 	}
+	handle_exception(GP_VECTOR, NULL);
+	return ret;
 }
 
-static void do_write_feature_control(void *data)
+static bool do_write_feature_control(void)
 {
 	jmp_buf jmpbuf;
-	if (set_exception_jmpbuf(jmpbuf) == 0) {
+	bool ret;
+	if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
 		wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
+		ret = 0;
+	} else {
+		ret = 1;
 	}
+	handle_exception(GP_VECTOR, NULL);
+	return ret;
 }
 
 static int test_vmx_feature_control(void)
@@ -650,19 +662,16 @@ static int test_vmx_feature_control(void)
 	}
 
 	wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
-	report("test vmxon with FEATURE_CONTROL cleared",
-	       test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
+	report("test vmxon with FEATURE_CONTROL cleared", do_vmxon_off());
 
 	wrmsr(MSR_IA32_FEATURE_CONTROL, 0x4);
-	report("test vmxon without FEATURE_CONTROL lock",
-	       test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
+	report("test vmxon without FEATURE_CONTROL lock", do_vmxon_off());
 
 	wrmsr(MSR_IA32_FEATURE_CONTROL, 0x5);
 	vmx_enabled = ((rdmsr(MSR_IA32_FEATURE_CONTROL) & 0x5) == 0x5);
 	report("test enable VMX in FEATURE_CONTROL", vmx_enabled);
 
-	report("test FEATURE_CONTROL lock bit",
-	       test_for_exception(GP_VECTOR, &do_write_feature_control, NULL));
+	report("test FEATURE_CONTROL lock bit", do_write_feature_control());
 
 	return !vmx_enabled;
 }
-- 
2.5.0

[PATCH kvm-unit-tests 4/4] x86: apic: cleanup

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/apic.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/x86/apic.c b/x86/apic.c
index de19724..dfaea35 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -66,7 +66,7 @@ static bool do_write_apicbase(u64 data)
     jmp_buf jmpbuf;
     int ret;
     if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
-        wrmsr(MSR_IA32_APICBASE, data);
+        wrmsr(MSR_IA32_APICBASE, APIC_DEFAULT_PHYS_BASE | APIC_BSP | data);
         ret = 0;
     } else {
         ret = 1;
@@ -77,36 +77,32 @@ static bool do_write_apicbase(u64 data)
 
 void test_enable_x2apic(void)
 {
-    u64 invalid_state = APIC_DEFAULT_PHYS_BASE | APIC_BSP | APIC_EXTD;
-    u64 apic_enabled = APIC_DEFAULT_PHYS_BASE | APIC_BSP | APIC_EN;
-    u64 x2apic_enabled =
-        APIC_DEFAULT_PHYS_BASE | APIC_BSP | APIC_EN | APIC_EXTD;
-
     if (enable_x2apic()) {
         printf("x2apic enabled\n");
 
         report("x2apic enabled to invalid state",
-               do_write_apicbase(invalid_state));
+               do_write_apicbase(APIC_EXTD));
         report("x2apic enabled to apic enabled",
-               do_write_apicbase(apic_enabled));
+               do_write_apicbase(APIC_EN));
 
-        wrmsr(MSR_IA32_APICBASE, APIC_DEFAULT_PHYS_BASE | APIC_BSP);
+        report("x2apic enabled to disabled state",
+               !do_write_apicbase(0));
         report("disabled to invalid state",
-               do_write_apicbase(invalid_state));
+               do_write_apicbase(APIC_EXTD));
         report("disabled to x2apic enabled",
-               do_write_apicbase(x2apic_enabled));
+               do_write_apicbase(APIC_EN | APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, apic_enabled);
+        wrmsr(MSR_IA32_APICBASE, APIC_EN);
         report("apic enabled to invalid state",
-               do_write_apicbase(invalid_state));
+               do_write_apicbase(APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, x2apic_enabled);
+        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
         apic_write(APIC_SPIV, 0x1ff);
     } else {
         printf("x2apic not detected\n");
 
         report("enable unsupported x2apic",
-               do_write_apicbase(x2apic_enabled));
+               do_write_apicbase(APIC_EN | APIC_EXTD));
     }
 }
 
-- 
2.5.0

Re: [PATCH kvm-unit-tests 1/4] lib: add setjmp header and x86 implementation

[Andrew Jones]

On Tue, Dec 15, 2015 at 11:25:35AM +0100, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  config/config-i386.mak       |  2 ++
>  config/config-x86-common.mak |  4 +++-
>  config/config-x86_64.mak     |  2 ++
>  lib/setjmp.h                 | 12 ++++++++++++
>  lib/x86/setjmp32.S           | 25 +++++++++++++++++++++++++
>  lib/x86/setjmp64.S           | 27 +++++++++++++++++++++++++++
>  x86/setjmp.c                 | 19 +++++++++++++++++++
>  7 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 lib/setjmp.h
>  create mode 100644 lib/x86/setjmp32.S
>  create mode 100644 lib/x86/setjmp64.S
>  create mode 100644 x86/setjmp.c
> 
> diff --git a/config/config-i386.mak b/config/config-i386.mak
> index 691381c..e353387 100644
> --- a/config/config-i386.mak
> +++ b/config/config-i386.mak
> @@ -3,6 +3,8 @@ bits = 32
>  ldarch = elf32-i386
>  CFLAGS += -I $(KERNELDIR)/include
>  
> +cflatobjs += lib/x86/setjmp32.o
> +
>  tests = $(TEST_DIR)/taskswitch.flat $(TEST_DIR)/taskswitch2.flat \
>  	$(TEST_DIR)/cmpxchg8b.flat
>  
> diff --git a/config/config-x86-common.mak b/config/config-x86-common.mak
> index f64874d..2bb2f46 100644
> --- a/config/config-x86-common.mak
> +++ b/config/config-x86-common.mak
> @@ -34,7 +34,7 @@ tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
>                 $(TEST_DIR)/realmode.flat $(TEST_DIR)/msr.flat \
>                 $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
>                 $(TEST_DIR)/kvmclock_test.flat  $(TEST_DIR)/eventinj.flat \
> -               $(TEST_DIR)/s3.flat $(TEST_DIR)/pmu.flat \
> +               $(TEST_DIR)/s3.flat $(TEST_DIR)/pmu.flat $(TEST_DIR)/setjmp.flat \
>                 $(TEST_DIR)/tsc_adjust.flat $(TEST_DIR)/asyncpf.flat \
>                 $(TEST_DIR)/init.flat $(TEST_DIR)/smap.flat \
>                 $(TEST_DIR)/hyperv_synic.flat
> @@ -115,6 +115,8 @@ $(TEST_DIR)/memory.elf: $(cstart.o) $(TEST_DIR)/memory.o
>  
>  $(TEST_DIR)/hyperv_synic.elf: $(cstart.o) $(TEST_DIR)/hyperv_synic.o
>  
> +$(TEST_DIR)/setjmp.elf: $(cstart.o) $(TEST_DIR)/setjmp.o
> +
>  arch_clean:
>  	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
>  	$(TEST_DIR)/.*.d lib/x86/.*.d
> diff --git a/config/config-x86_64.mak b/config/config-x86_64.mak
> index 1764701..d190be8 100644
> --- a/config/config-x86_64.mak
> +++ b/config/config-x86_64.mak
> @@ -3,6 +3,8 @@ bits = 64
>  ldarch = elf64-x86-64
>  CFLAGS += -mno-red-zone
>  
> +cflatobjs += lib/x86/setjmp64.o
> +
>  tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
>  	  $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
>  	  $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \
> diff --git a/lib/setjmp.h b/lib/setjmp.h
> new file mode 100644
> index 0000000..334f466
> --- /dev/null
> +++ b/lib/setjmp.h
> @@ -0,0 +1,12 @@
> +#ifndef LIBCFLAT_SETJMP_H
> +#define LIBCFLAT_SETJMP_H 1
> +
> +typedef struct jmp_buf_tag {
> +	long int regs[8];
> +} jmp_buf[1];
> +
> +extern int setjmp (struct jmp_buf_tag env[1]);
> +extern void longjmp (struct jmp_buf_tag env[1], int val)
> +     __attribute__ ((__noreturn__));
> +
> +#endif /* setjmp.h  */
> diff --git a/lib/x86/setjmp32.S b/lib/x86/setjmp32.S
> new file mode 100644
> index 0000000..b0be7c2
> --- /dev/null
> +++ b/lib/x86/setjmp32.S
> @@ -0,0 +1,25 @@
> +.globl setjmp
> +setjmp:
> +	mov (%esp), %ecx	// get return EIP
> +	mov 4(%esp), %eax	// get jmp_buf
> +	mov %ecx, (%eax)
> +	mov %esp, 4(%eax)
> +	mov %ebp, 8(%eax)
> +	mov %ebx, 12(%eax)
> +	mov %esi, 16(%eax)
> +	mov %edi, 20(%eax)
> +	xor %eax, %eax
> +	ret
> +
> +.globl longjmp
> +longjmp:
> +	mov 8(%esp), %eax	// get return value
> +	mov 4(%esp), %ecx	// get jmp_buf
> +	mov 20(%ecx), %edi
> +	mov 16(%ecx), %esi
> +	mov 12(%ecx), %ebx
> +	mov 8(%ecx), %ebp
> +	mov 4(%ecx), %esp
> +	mov (%ecx), %ecx	// get saved EIP
> +	mov %ecx, (%esp)	// and store it on the stack
> +	ret
> diff --git a/lib/x86/setjmp64.S b/lib/x86/setjmp64.S
> new file mode 100644
> index 0000000..c8ae790
> --- /dev/null
> +++ b/lib/x86/setjmp64.S
> @@ -0,0 +1,27 @@
> +.globl setjmp
> +setjmp:
> +	mov (%rsp), %rsi
> +	mov %rsi, (%rdi)
> +	mov %rsp, 0x8(%rdi)
> +	mov %rbp, 0x10(%rdi)
> +	mov %rbx, 0x18(%rdi)
> +	mov %r12, 0x20(%rdi)
> +	mov %r13, 0x28(%rdi)
> +	mov %r14, 0x30(%rdi)
> +	mov %r15, 0x38(%rdi)
> +	xor %eax, %eax
> +	ret
> +
> +.globl longjmp
> +longjmp:
> +	mov %esi, %eax
> +	mov 0x38(%rdi), %r15
> +	mov 0x30(%rdi), %r14
> +	mov 0x28(%rdi), %r13
> +	mov 0x20(%rdi), %r12
> +	mov 0x18(%rdi), %rbx
> +	mov 0x10(%rdi), %rbp
> +	mov 0x8(%rdi), %rsp
> +	mov (%rdi), %rsi
> +	mov %rsi, (%rsp)
> +	ret
> diff --git a/x86/setjmp.c b/x86/setjmp.c
> new file mode 100644
> index 0000000..46f0d9c
> --- /dev/null
> +++ b/x86/setjmp.c
> @@ -0,0 +1,19 @@
> +#include "stdio.h"
> +#include "setjmp.h"
> +
> +int main()
> +{
> +    volatile int i;
> +    jmp_buf j;
> +
> +    if (setjmp(j) == 0) {
> +	    i = 0;
> +    }
> +    printf("%d\n", i);
> +    if (++i < 10) {
> +	    longjmp(j, 1);
> +    }
> +
> +    printf("done\n");
> +    return 0;

How about making this a "real" test, i.e.

report("longjmp", i == 10);
return report_summary();

I have patches that allow adding timeouts to tests, that I've been
thinking about posting upstream. With those we could add a short
timeout to this one, allowing us to get the FAIL when we loop
forever, as well as when we never loop.

Thanks,
drew


> +}
> -- 
> 2.5.0
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests 1/4] lib: add setjmp header and x86 implementation

[Paolo Bonzini]

On 15/12/2015 17:43, Andrew Jones wrote:
> How about making this a "real" test, i.e.
> 
> report("longjmp", i == 10);
> return report_summary();
> 
> I have patches that allow adding timeouts to tests, that I've been
> thinking about posting upstream. With those we could add a short
> timeout to this one, allowing us to get the FAIL when we loop
> forever, as well as when we never loop.

Good idea.

Paolo

Re: [PATCH kvm-unit-tests 3/4] x86: remove test_for_exception

[Andrew Jones]

On Tue, Dec 15, 2015 at 11:25:37AM +0100, Paolo Bonzini wrote:
> Test functions know whether an exception was generated simply by checking
> the last value returned by set_exception_jmpbuf.  The exception number is
> passed to set_exception_jmpbuf so that it can set up the exception handler.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c | 13 +------------
>  lib/x86/desc.h |  8 +++-----
>  x86/apic.c     | 34 +++++++++++++++++-----------------
>  x86/vmx.c      | 29 +++++++++++++++++++----------
>  4 files changed, 40 insertions(+), 44 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index acf29e3..acf66b6 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -315,7 +315,6 @@ void setup_alt_stack(void)
>  }
>  #endif
>  
> -static bool exception;
>  static jmp_buf *exception_jmpbuf;
>  
>  static void exception_handler_longjmp(void)
> @@ -326,21 +325,11 @@ static void exception_handler_longjmp(void)
>  static void exception_handler(struct ex_regs *regs)
>  {
>  	/* longjmp must happen after iret, so do not do it now.  */
> -	exception = true;
>  	regs->rip = (unsigned long)&exception_handler_longjmp;
>  }
>  
> -bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
> -			void *data)
> +void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr)
>  {
>  	handle_exception(ex, exception_handler);
> -	exception = false;
> -	trigger_func(data);
> -	handle_exception(ex, NULL);
> -	return exception;
> -}
> -
> -void __set_exception_jmpbuf(jmp_buf *addr)
> -{
>  	exception_jmpbuf = addr;
>  }
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index be52fd4..fceabd8 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -155,10 +155,8 @@ void set_intr_alt_stack(int e, void *fn);
>  void print_current_tss_info(void);
>  void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
>  
> -bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
> -			void *data);
> -void __set_exception_jmpbuf(jmp_buf *addr);
> -#define set_exception_jmpbuf(jmpbuf) \
> -	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
> +void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr);
> +#define set_exception_jmpbuf(ex, jmpbuf) \
> +	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf((ex), &(jmpbuf)), 0))
>  
>  #endif
> diff --git a/x86/apic.c b/x86/apic.c
> index 2e04c82..de19724 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -61,12 +61,18 @@ static void test_tsc_deadline_timer(void)
>      }
>  }
>  
> -static void do_write_apicbase(void *data)
> +static bool do_write_apicbase(u64 data)
>  {
>      jmp_buf jmpbuf;
> -    if (set_exception_jmpbuf(jmpbuf) == 0) {
> -        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
> +    int ret;
> +    if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
> +        wrmsr(MSR_IA32_APICBASE, data);
> +        ret = 0;
> +    } else {
> +        ret = 1;
>      }
> +    handle_exception(GP_VECTOR, NULL);
> +    return ret;
>  }
>  
>  void test_enable_x2apic(void)
> @@ -80,24 +86,19 @@ void test_enable_x2apic(void)
>          printf("x2apic enabled\n");
>  
>          report("x2apic enabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>          report("x2apic enabled to apic enabled",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &apic_enabled));
> +               do_write_apicbase(apic_enabled));
>  
>          wrmsr(MSR_IA32_APICBASE, APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>          report("disabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>          report("disabled to x2apic enabled",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &x2apic_enabled));
> +               do_write_apicbase(x2apic_enabled));
>  
>          wrmsr(MSR_IA32_APICBASE, apic_enabled);
>          report("apic enabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>  
>          wrmsr(MSR_IA32_APICBASE, x2apic_enabled);
>          apic_write(APIC_SPIV, 0x1ff);
> @@ -105,8 +106,7 @@ void test_enable_x2apic(void)
>          printf("x2apic not detected\n");
>  
>          report("enable unsupported x2apic",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &x2apic_enabled));
> +               do_write_apicbase(x2apic_enabled));
>      }
>  }
>  
> @@ -128,11 +128,11 @@ static void test_apicbase(void)
>  
>      value = orig_apicbase | (1UL << cpuid_maxphyaddr());
>      report("reserved physaddr bits",
> -           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
> +           do_write_apicbase(value));
>  
>      value = orig_apicbase | 1;
>      report("reserved low bits",
> -           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
> +           do_write_apicbase(value));
>  
>      wrmsr(MSR_IA32_APICBASE, orig_apicbase);
>      apic_write(APIC_SPIV, 0x1ff);
> diff --git a/x86/vmx.c b/x86/vmx.c
> index ee9aca8..0b2cce0 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -617,21 +617,33 @@ static void init_vmx(void)
>  	memset(guest_syscall_stack, 0, PAGE_SIZE);
>  }
>  
> -static void do_vmxon_off(void *data)
> +static bool do_vmxon_off(void)
>  {
>  	jmp_buf jmpbuf;
> -	if (set_exception_jmpbuf(jmpbuf) == 0) {
> +	bool ret;
> +	if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
>  		vmx_on();
>  		vmx_off();
> +		return 0;
> +	} else {
> +		return 1;
>  	}

typo: you wanted to do 'ret = 0', 'ret = 1' above


> +	handle_exception(GP_VECTOR, NULL);
> +	return ret;
>  }
>  
> -static void do_write_feature_control(void *data)
> +static bool do_write_feature_control(void)
>  {
>  	jmp_buf jmpbuf;
> -	if (set_exception_jmpbuf(jmpbuf) == 0) {
> +	bool ret;
> +	if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
>  		wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
> +		ret = 0;
> +	} else {
> +		ret = 1;
>  	}
> +	handle_exception(GP_VECTOR, NULL);
> +	return ret;
>  }
>  
>  static int test_vmx_feature_control(void)
> @@ -650,19 +662,16 @@ static int test_vmx_feature_control(void)
>  	}
>  
>  	wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
> -	report("test vmxon with FEATURE_CONTROL cleared",
> -	       test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
> +	report("test vmxon with FEATURE_CONTROL cleared", do_vmxon_off());
>  
>  	wrmsr(MSR_IA32_FEATURE_CONTROL, 0x4);
> -	report("test vmxon without FEATURE_CONTROL lock",
> -	       test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
> +	report("test vmxon without FEATURE_CONTROL lock", do_vmxon_off());
>  
>  	wrmsr(MSR_IA32_FEATURE_CONTROL, 0x5);
>  	vmx_enabled = ((rdmsr(MSR_IA32_FEATURE_CONTROL) & 0x5) == 0x5);
>  	report("test enable VMX in FEATURE_CONTROL", vmx_enabled);
>  
> -	report("test FEATURE_CONTROL lock bit",
> -	       test_for_exception(GP_VECTOR, &do_write_feature_control, NULL));
> +	report("test FEATURE_CONTROL lock bit", do_write_feature_control());
>  
>  	return !vmx_enabled;
>  }
> -- 
> 2.5.0
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests 2/4] x86: replace set_exception_return with longjmp-based implementation

[Andrew Jones]

On Tue, Dec 15, 2015 at 11:25:36AM +0100, Paolo Bonzini wrote:
> set_exception_return forces exceptions handlers to return to a specific
> address instead of returning to the instruction address pushed by the
> CPU at the time of the exception. The unit tests apic.c and vmx.c use
> this functionality to recover from expected exceptions.
> 
> When using set_exception_return one would have to be careful not to modify
> the stack (such as by doing a function call) as triggering the exception
> will likely jump us past the instructions which undo the stack manipulation
> (such as a ret).  This is unnecessarily brittle, and C already has a
> mechanism to do non-local returns---setjmp.  Now that libcflat includes
> an implementation of setjmp, replace set_exception_return with a wrapper
> that takes care of restoring the processor flags as well.
> 
> Reported-by: David Matlack <dmatlack@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c | 15 +++++++++++----
>  lib/x86/desc.h |  6 +++++-
>  x86/apic.c     |  8 ++++----
>  x86/vmx.c      | 18 +++++++++---------
>  4 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index 4760026..acf29e3 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -1,6 +1,7 @@
>  #include "libcflat.h"
>  #include "desc.h"
>  #include "processor.h"
> +#include <setjmp.h>
>  
>  void set_idt_entry(int vec, void *addr, int dpl)
>  {
> @@ -315,12 +316,18 @@ void setup_alt_stack(void)
>  #endif
>  
>  static bool exception;
> -static void *exception_return;
> +static jmp_buf *exception_jmpbuf;

While changing this, how about making it per_cpu?
i.e. exception_jmpbuf[NR_CPUS]

drew

Re: [PATCH kvm-unit-tests 3/4] x86: remove test_for_exception

[David Matlack]

On Tue, Dec 15, 2015 at 2:25 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Test functions know whether an exception was generated simply by checking
> the last value returned by set_exception_jmpbuf.  The exception number is
> passed to set_exception_jmpbuf so that it can set up the exception handler.

I like the idea of test_for_exception, it makes the unit tests simpler. But
this (and the previous) version, require the trigger function to be in on
the joke (either by it calling set_exception_return, or now by it calling
set_exception_jmpbuf and handle_exception(NULL)).

What do you think about this simplification?

bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
                        void *data)
        jmp_buf jmpbuf;
        int ret;

        handle_exception(ex, exception_handler);
        ret = set_exception_jmpbuf(&jmpbuf);
        if (ret == 0)
                trigger_func(data);
        handle_exception(ex, NULL);
        return ret;
}

Then trigger_func can be very simple, e.g.

static void do_write_apicbase(u64 data)
{
        wrmsr(MSR_IA32_APICBASE, data);
}

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c | 13 +------------
>  lib/x86/desc.h |  8 +++-----
>  x86/apic.c     | 34 +++++++++++++++++-----------------
>  x86/vmx.c      | 29 +++++++++++++++++++----------
>  4 files changed, 40 insertions(+), 44 deletions(-)
>
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index acf29e3..acf66b6 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -315,7 +315,6 @@ void setup_alt_stack(void)
>  }
>  #endif
>
> -static bool exception;
>  static jmp_buf *exception_jmpbuf;
>
>  static void exception_handler_longjmp(void)
> @@ -326,21 +325,11 @@ static void exception_handler_longjmp(void)
>  static void exception_handler(struct ex_regs *regs)
>  {
>         /* longjmp must happen after iret, so do not do it now.  */
> -       exception = true;
>         regs->rip = (unsigned long)&exception_handler_longjmp;
>  }
>
> -bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
> -                       void *data)
> +void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr)
>  {
>         handle_exception(ex, exception_handler);
> -       exception = false;
> -       trigger_func(data);
> -       handle_exception(ex, NULL);
> -       return exception;
> -}
> -
> -void __set_exception_jmpbuf(jmp_buf *addr)
> -{
>         exception_jmpbuf = addr;
>  }
> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> index be52fd4..fceabd8 100644
> --- a/lib/x86/desc.h
> +++ b/lib/x86/desc.h
> @@ -155,10 +155,8 @@ void set_intr_alt_stack(int e, void *fn);
>  void print_current_tss_info(void);
>  void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
>
> -bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
> -                       void *data);
> -void __set_exception_jmpbuf(jmp_buf *addr);
> -#define set_exception_jmpbuf(jmpbuf) \
> -       (setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
> +void __set_exception_jmpbuf(unsigned int ex, jmp_buf *addr);
> +#define set_exception_jmpbuf(ex, jmpbuf) \
> +       (setjmp(jmpbuf) ? : (__set_exception_jmpbuf((ex), &(jmpbuf)), 0))
>
>  #endif
> diff --git a/x86/apic.c b/x86/apic.c
> index 2e04c82..de19724 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -61,12 +61,18 @@ static void test_tsc_deadline_timer(void)
>      }
>  }
>
> -static void do_write_apicbase(void *data)
> +static bool do_write_apicbase(u64 data)
>  {
>      jmp_buf jmpbuf;
> -    if (set_exception_jmpbuf(jmpbuf) == 0) {
> -        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
> +    int ret;
> +    if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
> +        wrmsr(MSR_IA32_APICBASE, data);
> +        ret = 0;
> +    } else {
> +        ret = 1;
>      }
> +    handle_exception(GP_VECTOR, NULL);
> +    return ret;
>  }
>
>  void test_enable_x2apic(void)
> @@ -80,24 +86,19 @@ void test_enable_x2apic(void)
>          printf("x2apic enabled\n");
>
>          report("x2apic enabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>          report("x2apic enabled to apic enabled",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &apic_enabled));
> +               do_write_apicbase(apic_enabled));
>
>          wrmsr(MSR_IA32_APICBASE, APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>          report("disabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>          report("disabled to x2apic enabled",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &x2apic_enabled));
> +               do_write_apicbase(x2apic_enabled));
>
>          wrmsr(MSR_IA32_APICBASE, apic_enabled);
>          report("apic enabled to invalid state",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &invalid_state));
> +               do_write_apicbase(invalid_state));
>
>          wrmsr(MSR_IA32_APICBASE, x2apic_enabled);
>          apic_write(APIC_SPIV, 0x1ff);
> @@ -105,8 +106,7 @@ void test_enable_x2apic(void)
>          printf("x2apic not detected\n");
>
>          report("enable unsupported x2apic",
> -               test_for_exception(GP_VECTOR, do_write_apicbase,
> -                                  &x2apic_enabled));
> +               do_write_apicbase(x2apic_enabled));
>      }
>  }
>
> @@ -128,11 +128,11 @@ static void test_apicbase(void)
>
>      value = orig_apicbase | (1UL << cpuid_maxphyaddr());
>      report("reserved physaddr bits",
> -           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
> +           do_write_apicbase(value));
>
>      value = orig_apicbase | 1;
>      report("reserved low bits",
> -           test_for_exception(GP_VECTOR, do_write_apicbase, &value));
> +           do_write_apicbase(value));
>
>      wrmsr(MSR_IA32_APICBASE, orig_apicbase);
>      apic_write(APIC_SPIV, 0x1ff);
> diff --git a/x86/vmx.c b/x86/vmx.c
> index ee9aca8..0b2cce0 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -617,21 +617,33 @@ static void init_vmx(void)
>         memset(guest_syscall_stack, 0, PAGE_SIZE);
>  }
>
> -static void do_vmxon_off(void *data)
> +static bool do_vmxon_off(void)
>  {
>         jmp_buf jmpbuf;
> -       if (set_exception_jmpbuf(jmpbuf) == 0) {
> +       bool ret;
> +       if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
>                 vmx_on();
>                 vmx_off();
> +               return 0;
> +       } else {
> +               return 1;
>         }
> +       handle_exception(GP_VECTOR, NULL);
> +       return ret;
>  }
>
> -static void do_write_feature_control(void *data)
> +static bool do_write_feature_control(void)
>  {
>         jmp_buf jmpbuf;
> -       if (set_exception_jmpbuf(jmpbuf) == 0) {
> +       bool ret;
> +       if (set_exception_jmpbuf(GP_VECTOR, jmpbuf) == 0) {
>                 wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
> +               ret = 0;
> +       } else {
> +               ret = 1;
>         }
> +       handle_exception(GP_VECTOR, NULL);
> +       return ret;
>  }
>
>  static int test_vmx_feature_control(void)
> @@ -650,19 +662,16 @@ static int test_vmx_feature_control(void)
>         }
>
>         wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
> -       report("test vmxon with FEATURE_CONTROL cleared",
> -              test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
> +       report("test vmxon with FEATURE_CONTROL cleared", do_vmxon_off());
>
>         wrmsr(MSR_IA32_FEATURE_CONTROL, 0x4);
> -       report("test vmxon without FEATURE_CONTROL lock",
> -              test_for_exception(GP_VECTOR, &do_vmxon_off, NULL));
> +       report("test vmxon without FEATURE_CONTROL lock", do_vmxon_off());
>
>         wrmsr(MSR_IA32_FEATURE_CONTROL, 0x5);
>         vmx_enabled = ((rdmsr(MSR_IA32_FEATURE_CONTROL) & 0x5) == 0x5);
>         report("test enable VMX in FEATURE_CONTROL", vmx_enabled);
>
> -       report("test FEATURE_CONTROL lock bit",
> -              test_for_exception(GP_VECTOR, &do_write_feature_control, NULL));
> +       report("test FEATURE_CONTROL lock bit", do_write_feature_control());
>
>         return !vmx_enabled;
>  }
> --
> 2.5.0
>
>

Re: [PATCH kvm-unit-tests 3/4] x86: remove test_for_exception

[Paolo Bonzini]

On 15/12/2015 19:32, David Matlack wrote:
> What do you think about this simplification?
> 
> bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
>                         void *data)
>         jmp_buf jmpbuf;
>         int ret;
> 
>         handle_exception(ex, exception_handler);
>         ret = set_exception_jmpbuf(&jmpbuf);
>         if (ret == 0)
>                 trigger_func(data);
>         handle_exception(ex, NULL);
>         return ret;
> }
> 
> Then trigger_func can be very simple, e.g.
> 
> static void do_write_apicbase(u64 data)
> {
>         wrmsr(MSR_IA32_APICBASE, data);
> }

I agree that's the best of both worlds.

-------- 8< ------------
>From de1c13978b785e2aca82060ad3ccecdc04e75802 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 12 Jan 2016 13:40:19 +0100
Subject: [PATCH kvm-unit-tests] x86: simplify test_for_exception trigger
 functions

test_for_exception makes the unit tests simpler, but it requires the
trigger function to be in on the joke: before, by calling
set_exception_return; now, by calling set_exception_jmpbuf and
handle_exception(NULL).  The new setjmp-based implementation
lets us move all of this into test_for_exception itself, so
that the trigger_func can be very simple.

Suggested-by: David Matlack <dmatlack@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 lib/x86/desc.c | 10 +++++++---
 x86/apic.c     |  5 +----
 x86/vmx.c      | 12 +++---------
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index acf29e3..17aedcf 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -333,11 +333,15 @@ static void exception_handler(struct ex_regs *regs)
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 			void *data)
 {
+	jmp_buf jmpbuf;
+	int ret;
+
 	handle_exception(ex, exception_handler);
-	exception = false;
-	trigger_func(data);
+	ret = set_exception_jmpbuf(&jmpbuf);
+	if (ret == 0)
+		trigger_func(data);
 	handle_exception(ex, NULL);
-	return exception;
+	return ret;
 }
 
 void __set_exception_jmpbuf(jmp_buf *addr)
diff --git a/x86/apic.c b/x86/apic.c
index 2e04c82..61a0a76 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -63,10 +63,7 @@ static void test_tsc_deadline_timer(void)
 
 static void do_write_apicbase(void *data)
 {
-    jmp_buf jmpbuf;
-    if (set_exception_jmpbuf(jmpbuf) == 0) {
-        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
-    }
+    wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
 }
 
 void test_enable_x2apic(void)
diff --git a/x86/vmx.c b/x86/vmx.c
index 12ec396..3fa1a73 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -617,19 +617,13 @@ static void init_vmx(void)
 
 static void do_vmxon_off(void *data)
 {
-	jmp_buf jmpbuf;
-	if (set_exception_jmpbuf(jmpbuf) == 0) {
-		vmx_on();
-		vmx_off();
-	}
+	vmx_on();
+	vmx_off();
 }
 
 static void do_write_feature_control(void *data)
 {
-	jmp_buf jmpbuf;
-	if (set_exception_jmpbuf(jmpbuf) == 0) {
-		wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
-	}
+	wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
 }
 
 static int test_vmx_feature_control(void)
-- 
2.5.0

Re: [PATCH kvm-unit-tests 3/4] x86: remove test_for_exception

[David Matlack]

On Tue, Jan 12, 2016 at 4:42 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> On 15/12/2015 19:32, David Matlack wrote:
> > What do you think about this simplification?
> >
> > bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
> >                         void *data)
> >         jmp_buf jmpbuf;
> >         int ret;
> >
> >         handle_exception(ex, exception_handler);
> >         ret = set_exception_jmpbuf(&jmpbuf);
> >         if (ret == 0)
> >                 trigger_func(data);
> >         handle_exception(ex, NULL);
> >         return ret;
> > }
> >
> > Then trigger_func can be very simple, e.g.
> >
> > static void do_write_apicbase(u64 data)
> > {
> >         wrmsr(MSR_IA32_APICBASE, data);
> > }
>
> I agree that's the best of both worlds.
>
> -------- 8< ------------
> From de1c13978b785e2aca82060ad3ccecdc04e75802 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@redhat.com>
> Date: Tue, 12 Jan 2016 13:40:19 +0100
> Subject: [PATCH kvm-unit-tests] x86: simplify test_for_exception trigger
>  functions
>
> test_for_exception makes the unit tests simpler, but it requires the
> trigger function to be in on the joke: before, by calling
> set_exception_return; now, by calling set_exception_jmpbuf and
> handle_exception(NULL).  The new setjmp-based implementation
> lets us move all of this into test_for_exception itself, so
> that the trigger_func can be very simple.

Looks good. Thanks!

>
> Suggested-by: David Matlack <dmatlack@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  lib/x86/desc.c | 10 +++++++---
>  x86/apic.c     |  5 +----
>  x86/vmx.c      | 12 +++---------
>  3 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> index acf29e3..17aedcf 100644
> --- a/lib/x86/desc.c
> +++ b/lib/x86/desc.c
> @@ -333,11 +333,15 @@ static void exception_handler(struct ex_regs *regs)
>  bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
>                         void *data)
>  {
> +       jmp_buf jmpbuf;
> +       int ret;
> +
>         handle_exception(ex, exception_handler);
> -       exception = false;
> -       trigger_func(data);
> +       ret = set_exception_jmpbuf(&jmpbuf);
> +       if (ret == 0)
> +               trigger_func(data);
>         handle_exception(ex, NULL);
> -       return exception;
> +       return ret;
>  }
>
>  void __set_exception_jmpbuf(jmp_buf *addr)
> diff --git a/x86/apic.c b/x86/apic.c
> index 2e04c82..61a0a76 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -63,10 +63,7 @@ static void test_tsc_deadline_timer(void)
>
>  static void do_write_apicbase(void *data)
>  {
> -    jmp_buf jmpbuf;
> -    if (set_exception_jmpbuf(jmpbuf) == 0) {
> -        wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
> -    }
> +    wrmsr(MSR_IA32_APICBASE, *(u64 *)data);
>  }
>
>  void test_enable_x2apic(void)
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 12ec396..3fa1a73 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -617,19 +617,13 @@ static void init_vmx(void)
>
>  static void do_vmxon_off(void *data)
>  {
> -       jmp_buf jmpbuf;
> -       if (set_exception_jmpbuf(jmpbuf) == 0) {
> -               vmx_on();
> -               vmx_off();
> -       }
> +       vmx_on();
> +       vmx_off();
>  }
>
>  static void do_write_feature_control(void *data)
>  {
> -       jmp_buf jmpbuf;
> -       if (set_exception_jmpbuf(jmpbuf) == 0) {
> -               wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
> -       }
> +       wrmsr(MSR_IA32_FEATURE_CONTROL, 0);
>  }
>
>  static int test_vmx_feature_control(void)
> --
> 2.5.0
>