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 
> ---
>  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)_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,
> -  _state));
> +   do_write_apicbase(invalid_state));
>  report("x2apic enabled to apic enabled",
> -   test_for_exception(GP_VECTOR, do_write_apicbase,
> -  _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,
> -  _state));
> +   do_write_apicbase(invalid_state));
>  report("disabled to x2apic enabled",
> -   test_for_exception(GP_VECTOR, do_write_apicbase,
> -  _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,
> -  _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,
> -  _enabled));
> +   do_write_apicbase(x2apic_enabled));
>  }
>  }
>  
> @@ -128,11 +128,11 @@ static void test_apicbase(void)
>  
>  value = orig_apicbase | (1UL << cpuid_maxphyaddr());
>  report("reserved physaddr 

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  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();
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 
> ---
>  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)_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,
> -  _state));
> +   do_write_apicbase(invalid_state));
>  report("x2apic enabled to apic enabled",
> -   test_for_exception(GP_VECTOR, do_write_apicbase,
> -  _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,
> -  _state));
> +   do_write_apicbase(invalid_state));
>  report("disabled to x2apic enabled",
> -   test_for_exception(GP_VECTOR, do_write_apicbase,
> -  _enabled));
> +   do_write_apicbase(x2apic_enabled));
>
>  wrmsr(MSR_IA32_APICBASE, 

[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 
---
 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)_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,
-  _state));
+   do_write_apicbase(invalid_state));
 report("x2apic enabled to apic enabled",
-   test_for_exception(GP_VECTOR, do_write_apicbase,
-  _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,
-  _state));
+   do_write_apicbase(invalid_state));
 report("disabled to x2apic enabled",
-   test_for_exception(GP_VECTOR, do_write_apicbase,
-  _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,
-  _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,
-  _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, ));
+   do_write_apicbase(value));
 
 value = orig_apicbase | 1;
 report("reserved low bits",
-   test_for_exception(GP_VECTOR, do_write_apicbase, ));
+   do_write_apicbase(value));