Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case

[Paolo Bonzini <pbonzini@redhat.com>]

Il 24/07/2013 10:48, Arthur Chunqi Li ha scritto:
> So as what Gleb said, what about the following codes:
> 
> static int vmx_run2()
> {
>     u32 eax;
>     bool ret;
> 
>     asm volatile(
>         "mov %%rsp, %%rsi\n\t"
>         "mov %2, %%edi\n\t"
>         "call vmcs_write\n\t"
>         "vmlaunch\n\t"
>         "setbe %0\n\t"
>         "jne 4f\n\t"

setbe doesn't set the flags, so you need jbe here (and then you can have
a "mov $1, %0" at label 4 instead of using setbe).

> 
>         "vmx_return:\n\t"
>         SAVE_GPR_C
>         "call exit_handler\n\t"
>         "cmp %3, %%eax\n\t"
>         "je 2f\n\t"
>         "cmp %4, %%eax\n\t"
>         "je 1f\n\t"
>         "jmp 3f\n\t"
> 
>         /* VMX_TEST_RESUME */
>         "1:\n\t"
>         LOAD_GPR_C
>         "vmresume\n\t"

You need a SAVE_GPR_C here.  Then it is better to put the jbe at the
vmx_return label:

    ... do vmlaunch or vmreturn as Jan said ...
 vmx_return:
    SAVE_GPR_C
    jbe 4f
    call exit_handler
    etc.

Here is the relevant code from KVM that Jan was referring to:

                "jne 1f \n\t"
                __ex(ASM_VMX_VMLAUNCH) "\n\t"
                "jmp 2f \n\t"
                "1: " __ex(ASM_VMX_VMRESUME) "\n\t"
                "2: "

I'd prefer to have a "jbe vmx_return; ud2" after the vmlaunch/vmresume,
in order to test that the hypervisor respects HOST_RIP.

Paolo

>         "setbe %0\n\t"
>         "jne 4f\n\t"
>         /* VMX_TEST_VMEXIT */
>         "2:\n\t"
>         "mov $0, %1\n\t"
>         "jmp 5f\n\t"
>         /* undefined ret from exit_handler */
>         "3:\n\t"
>         "mov $2, %1\n\t"
>         "jmp 5f\n\t"
>         /* vmlaunch/vmresume failed, exit */
>         "4:\n\t"
>         "mov $1, %1\n\t"
>         "5:\n\t"
>         : "=r"(ret), "=r"(eax)
>         : "i"(HOST_RSP), "i"(VMX_TEST_VMEXIT),
>           "i"(VMX_TEST_RESUME)
>         : "rax", "rbx", "rdi", "rsi",
>            "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
>           "memory", "cc"
>     );
>     switch (eax) {
>     case 0:
>         return 0;
>     case 1:
>         printf("%s : vmenter failed.\n", __func__);
>         break;
>     default:
>         printf("%s : unhandled ret from exit_handler.\n", __func__);
>         break;
>     }
>     return 1;
> }
> 
> On Wed, Jul 24, 2013 at 2:48 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto:
>>> On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> Il 24/07/2013 08:11, Arthur Chunqi Li ha scritto:
>>>>>
>>>>> static int vmx_run()
>>>>> {
>>>>>     u32 eax;
>>>>>     bool ret;
>>>>>
>>>>>     vmcs_write(HOST_RSP, get_rsp());
>>>>>     ret = vmlaunch();
>>>>
>>>> The compiler can still change rsp between here...
>>>>
>>>>>     while (!ret) {
>>>>>         asm volatile(
>>>>>             "vmx_return:\n\t"
>>>>
>>>> ... and here.
>>>>
>>>> If you want to write it in C, the only thing that can be after
>>>> vmlaunch/vmresume is "exit()".  Else it has to be asm.
>>> Actually, you mean we need to write all the codes in asm to avoid
>>> changing to rsp, right?
>>
>> Not necessarily all the code.  It is also ok to use setjmp/longjmp with
>> a small asm trampoline, because this method won't care about the exact
>> rsp values that are used.  But if you want to do as Gleb said, and put
>> vmx_return just after vmlaunch, it has to be all asm as in KVM's
>> arch/x86/kvm/vmx.c.
>>
>> Paolo
> 
> 
>