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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux