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

[Andrew Jones <drjones@redhat.com>]

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