On Tue, Dec 15, 2015 at 2:25 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
