Re: [PATCH kvm-unit-tests 4/4] x86/taskswitch_vm86: Task switches into/out of VM86

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jan 23, 2012 at 05:32:59PM +0100, Kevin Wolf wrote:
> Am 23.01.2012 17:22, schrieb Gleb Natapov:
> > On Mon, Jan 23, 2012 at 05:20:22PM +0100, Kevin Wolf wrote:
> >> Am 23.01.2012 17:10, schrieb Gleb Natapov:
> >>> On Mon, Jan 23, 2012 at 05:07:13PM +0100, Kevin Wolf wrote:
> >>>> This adds a test case that jumps into VM86 by iret-ing to a TSS and back
> >>>> to Protected Mode using a task gate in the IDT.
> >>>>
> >>> Can you add the test case to taskswitch2.c?
> >>
> > Running one test to check all aspects of taskswitch emulation.
> 
> (We all know that top-posting is disliked, but middle-posting looks even
> crazier!)
> 
Inserting replies at random places is a new cool thing!

> Does having one test provide any value in and of itself? It's just an
> implementation detail of the test suite. When testing the KVM patches I
> ran all three test cases with './run_tests.sh -g task', which is
> hopefully easy enough.
> 
I think it does. I do not have to use external script to combine tests
on the same topic or even remember that such script exists. We do not
create separate tests to test each instruction emulation either. And I
usually run qemu not on the same machine I compile it on, so I need
special tricks to make those test script work. Of course if putting this
code into existing test file is hard separate test is OK, but is this
really the case here?

> Kevin
> 
> > 
> >> That's actually what I intended to do at first, but there's nothing to
> >> share and having a clean environment that can't interfere with other
> >> tests feels nicer.
> >>
> >> What would we gain from merging the files?
> >>
> >> Kevin
> >>
> >>
> >>>
> >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>>> ---
> >>>>  config-i386.mak       |    3 +-
> >>>>  lib/x86/desc.c        |   37 +-----------------------------
> >>>>  lib/x86/desc.h        |   36 +++++++++++++++++++++++++++++
> >>>>  lib/x86/vm.c          |    4 +-
> >>>>  lib/x86/vm.h          |    1 +
> >>>>  x86/taskswitch_vm86.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>  x86/unittests.cfg     |    6 +++++
> >>>>  7 files changed, 107 insertions(+), 39 deletions(-)
> >>>>  create mode 100644 x86/taskswitch_vm86.c
> >>>>
> >>>> diff --git a/config-i386.mak b/config-i386.mak
> >>>> index de52f3d..b5c3b9c 100644
> >>>> --- a/config-i386.mak
> >>>> +++ b/config-i386.mak
> >>>> @@ -5,9 +5,10 @@ ldarch = elf32-i386
> >>>>  CFLAGS += -D__i386__
> >>>>  CFLAGS += -I $(KERNELDIR)/include
> >>>>  
> >>>> -tests = $(TEST_DIR)/taskswitch.flat $(TEST_DIR)/taskswitch2.flat
> >>>> +tests = $(TEST_DIR)/taskswitch.flat $(TEST_DIR)/taskswitch2.flat $(TEST_DIR)/taskswitch_vm86.flat
> >>>>  
> >>>>  include config-x86-common.mak
> >>>>  
> >>>>  $(TEST_DIR)/taskswitch.elf: $(cstart.o) $(TEST_DIR)/taskswitch.o
> >>>>  $(TEST_DIR)/taskswitch2.elf: $(cstart.o) $(TEST_DIR)/taskswitch2.o
> >>>> +$(TEST_DIR)/taskswitch_vm86.elf: $(cstart.o) $(TEST_DIR)/taskswitch_vm86.o
> >>>> diff --git a/lib/x86/desc.c b/lib/x86/desc.c
> >>>> index 770c250..c4a3607 100644
> >>>> --- a/lib/x86/desc.c
> >>>> +++ b/lib/x86/desc.c
> >>>> @@ -27,41 +27,6 @@ typedef struct {
> >>>>  	u8 base_high;
> >>>>  } gdt_entry_t;
> >>>>  
> >>>> -typedef struct {
> >>>> -	u16 prev;
> >>>> -	u16 res1;
> >>>> -	u32 esp0;
> >>>> -	u16 ss0;
> >>>> -	u16 res2;
> >>>> -	u32 esp1;
> >>>> -	u16 ss1;
> >>>> -	u16 res3;
> >>>> -	u32 esp2;
> >>>> -	u16 ss2;
> >>>> -	u16 res4;
> >>>> -	u32 cr3;
> >>>> -	u32 eip;
> >>>> -	u32 eflags;
> >>>> -	u32 eax, ecx, edx, ebx, esp, ebp, esi, edi;
> >>>> -	u16 es;
> >>>> -	u16 res5;
> >>>> -	u16 cs;
> >>>> -	u16 res6;
> >>>> -	u16 ss;
> >>>> -	u16 res7;
> >>>> -	u16 ds;
> >>>> -	u16 res8;
> >>>> -	u16 fs;
> >>>> -	u16 res9;
> >>>> -	u16 gs;
> >>>> -	u16 res10;
> >>>> -	u16 ldt;
> >>>> -	u16 res11;
> >>>> -	u16 t:1;
> >>>> -	u16 res12:15;
> >>>> -	u16 iomap_base;
> >>>> -} tss32_t;
> >>>> -
> >>>>  extern idt_entry_t boot_idt[256];
> >>>>  
> >>>>  void set_idt_entry(int vec, void *addr, int dpl)
> >>>> @@ -327,7 +292,7 @@ void setup_gdt(void)
> >>>>  		      ".Lflush2: "::"r"(0x10));
> >>>>  }
> >>>>  
> >>>> -static void set_idt_task_gate(int vec, u16 sel)
> >>>> +void set_idt_task_gate(int vec, u16 sel)
> >>>>  {
> >>>>      idt_entry_t *e = &boot_idt[vec];
> >>>>  
> >>>> diff --git a/lib/x86/desc.h b/lib/x86/desc.h
> >>>> index 0b4897c..f819452 100644
> >>>> --- a/lib/x86/desc.h
> >>>> +++ b/lib/x86/desc.h
> >>>> @@ -24,6 +24,41 @@ struct ex_regs {
> >>>>      unsigned long rflags;
> >>>>  };
> >>>>  
> >>>> +typedef struct {
> >>>> +	u16 prev;
> >>>> +	u16 res1;
> >>>> +	u32 esp0;
> >>>> +	u16 ss0;
> >>>> +	u16 res2;
> >>>> +	u32 esp1;
> >>>> +	u16 ss1;
> >>>> +	u16 res3;
> >>>> +	u32 esp2;
> >>>> +	u16 ss2;
> >>>> +	u16 res4;
> >>>> +	u32 cr3;
> >>>> +	u32 eip;
> >>>> +	u32 eflags;
> >>>> +	u32 eax, ecx, edx, ebx, esp, ebp, esi, edi;
> >>>> +	u16 es;
> >>>> +	u16 res5;
> >>>> +	u16 cs;
> >>>> +	u16 res6;
> >>>> +	u16 ss;
> >>>> +	u16 res7;
> >>>> +	u16 ds;
> >>>> +	u16 res8;
> >>>> +	u16 fs;
> >>>> +	u16 res9;
> >>>> +	u16 gs;
> >>>> +	u16 res10;
> >>>> +	u16 ldt;
> >>>> +	u16 res11;
> >>>> +	u16 t:1;
> >>>> +	u16 res12:15;
> >>>> +	u16 iomap_base;
> >>>> +} tss32_t;
> >>>> +
> >>>>  #define ASM_TRY(catch)                                  \
> >>>>      "movl $0, %%gs:4 \n\t"                              \
> >>>>      ".pushsection .data.ex \n\t"                        \
> >>>> @@ -44,6 +79,7 @@ unsigned exception_error_code(void);
> >>>>  void set_idt_entry(int vec, void *addr, int dpl);
> >>>>  void set_idt_sel(int vec, u16 sel);
> >>>>  void set_gdt_entry(int num, u32 base,  u32 limit, u8 access, u8 gran);
> >>>> +void set_idt_task_gate(int vec, u16 sel);
> >>>>  void set_intr_task_gate(int e, void *fn);
> >>>>  void print_current_tss_info(void);
> >>>>  void handle_exception(u8 v, void (*func)(struct ex_regs *regs));
> >>>> diff --git a/lib/x86/vm.c b/lib/x86/vm.c
> >>>> index abbb0c9..aae044a 100644
> >>>> --- a/lib/x86/vm.c
> >>>> +++ b/lib/x86/vm.c
> >>>> @@ -108,14 +108,14 @@ void install_large_page(unsigned long *cr3,
> >>>>                                unsigned long phys,
> >>>>                                void *virt)
> >>>>  {
> >>>> -    install_pte(cr3, 2, virt, phys | PTE_PRESENT | PTE_WRITE | PTE_PSE, 0);
> >>>> +    install_pte(cr3, 2, virt, phys | PTE_PRESENT | PTE_WRITE | PTE_USER | PTE_PSE, 0);
> >>>>  }
> >>>>  
> >>>>  void install_page(unsigned long *cr3,
> >>>>                    unsigned long phys,
> >>>>                    void *virt)
> >>>>  {
> >>>> -    install_pte(cr3, 1, virt, phys | PTE_PRESENT | PTE_WRITE, 0);
> >>>> +    install_pte(cr3, 1, virt, phys | PTE_PRESENT | PTE_WRITE | PTE_USER, 0);
> >>>>  }
> >>>>  
> >>>>  
> >>>> diff --git a/lib/x86/vm.h b/lib/x86/vm.h
> >>>> index bf8fd52..aebc5c3 100644
> >>>> --- a/lib/x86/vm.h
> >>>> +++ b/lib/x86/vm.h
> >>>> @@ -13,6 +13,7 @@
> >>>>  #define PTE_PRESENT (1ull << 0)
> >>>>  #define PTE_PSE     (1ull << 7)
> >>>>  #define PTE_WRITE   (1ull << 1)
> >>>> +#define PTE_USER    (1ull << 2)
> >>>>  #define PTE_ADDR    (0xffffffffff000ull)
> >>>>  
> >>>>  void setup_vm();
> >>>> diff --git a/x86/taskswitch_vm86.c b/x86/taskswitch_vm86.c
> >>>> new file mode 100644
> >>>> index 0000000..363cb00
> >>>> --- /dev/null
> >>>> +++ b/x86/taskswitch_vm86.c
> >>>> @@ -0,0 +1,59 @@
> >>>> +#include "libcflat.h"
> >>>> +#include "desc.h"
> >>>> +#include "processor.h"
> >>>> +#include "vm.h"
> >>>> +
> >>>> +static tss32_t main_tss;
> >>>> +static tss32_t vm86_tss;
> >>>> +
> >>>> +#define FREE_GDT_INDEX 4
> >>>> +#define MAIN_TSS_INDEX (FREE_GDT_INDEX + 0)
> >>>> +#define VM86_TSS_INDEX (FREE_GDT_INDEX + 1)
> >>>> +
> >>>> +extern void vm86_start(void);
> >>>> +
> >>>> +int main(void)
> >>>> +{
> >>>> +    u8 *vm86_start;
> >>>> +
> >>>> +    setup_vm();
> >>>> +    setup_idt();
> >>>> +    setup_gdt();
> >>>> +
> >>>> +    /* Write a 'ud2' instruction somewhere below 1 MB */
> >>>> +    vm86_start = (void*) 0x42000;
> >>>> +    vm86_start[0] = 0x0f;
> >>>> +    vm86_start[1] = 0x0b;
> >>>> +
> >>>> +    /* Main TSS */
> >>>> +    set_gdt_entry(MAIN_TSS_INDEX, (u32)&main_tss, sizeof(tss32_t) - 1, 0x89, 0);
> >>>> +    ltr(MAIN_TSS_INDEX << 3);
> >>>> +    main_tss = (tss32_t) {
> >>>> +        .prev   = VM86_TSS_INDEX << 3,
> >>>> +        .cr3    = read_cr3(),
> >>>> +    };
> >>>> +
> >>>> +    /* VM86 TSS (marked as busy, so we can iret to it) */
> >>>> +    set_gdt_entry(VM86_TSS_INDEX, (u32)&vm86_tss, sizeof(tss32_t) - 1, 0x8b, 0);
> >>>> +    vm86_tss = (tss32_t) {
> >>>> +        .eflags = 0x20002,
> >>>> +        .cr3    = read_cr3(),
> >>>> +        .eip    = (u32) vm86_start & 0x0f,
> >>>> +        .cs     = (u32) vm86_start >> 4,
> >>>> +        .ds     = 0x1234,
> >>>> +        .es     = 0x2345,
> >>>> +    };
> >>>> +
> >>>> +    /* Setup task gate to main TSS for #UD */
> >>>> +    set_idt_task_gate(6, MAIN_TSS_INDEX << 3);
> >>>> +
> >>>> +    /* Jump into VM86 task with iret, #UD lets it come back immediately */
> >>>> +    asm volatile(
> >>>> +        "pushf\n"
> >>>> +        "orw $0x4000, (%esp)\n"
> >>>> +        "popf\n"
> >>>> +        "iret\n"
> >>>> +    );
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>>> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> >>>> index dac7d44..194850b 100644
> >>>> --- a/x86/unittests.cfg
> >>>> +++ b/x86/unittests.cfg
> >>>> @@ -76,6 +76,12 @@ smp = 2
> >>>>  extra_params = -cpu qemu64,-svm
> >>>>  groups = task
> >>>>  
> >>>> +[taskswitch_vm86]
> >>>> +file = taskswitch_vm86.flat
> >>>> +smp = 2
> >>>> +extra_params = -cpu qemu64,-svm
> >>>> +groups = task
> >>>> +
> >>>>  [kvmclock_test]
> >>>>  file = kvmclock_test.flat
> >>>>  smp = 2
> >>>> -- 
> >>>> 1.7.6.5
> >>>>
> >>>> --
> >>>> 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
> >>>
> >>> --
> >>> 			Gleb.
> > 
> > --
> > 			Gleb.

--
			Gleb.