Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Hi Gleb and Paolo, What about organizing vmx_run() as follows: static int vmx_run() { u32 eax; bool ret; vmcs_write(HOST_RSP, get_rsp()); ret = vmlaunch(); while (!ret) { asm volatile( vmx_return:\n\t SAVE_GPR ); eax = exit_handler(); switch (eax) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_RESUME: break; default: printf(%s : unhandled ret from exit_handler.\n, __func__); return 1; } ret = vmresume(); } printf(%s : vmenter failed.\n, __func__); return 1; } Arthur On Fri, Jul 19, 2013 at 8:06 PM, Paolo Bonzini pbonz...@redhat.com wrote: Il 19/07/2013 11:40, Gleb Natapov ha scritto: Because this is written in C, and I know trying to fool the compiler is a losing game. So my reaction is okay, HOST_RIP must be set so that code will not jump around. If I see asm(vmlaunch) exit(-1) the reaction is the opposite: hmm, anything that jumps around would have a hard time with the compiler, there must be some assembly trampoline somewhere; let's check what HOST_RIP is. We do try to fool compiler often even without vmx: code patching. This is why asm goto was invented, no? So, like you said in previous emails, asm goto is appropriate way here to tell compiler what is going on. Code patching is only reimplementing an existing C structure (if/else) in a different manner. Here the actual code flow (location of HOST_RIP and value of HOST_RSP) cannot be expressed correctly to the compiler because we do not use the C label (we use an asm label). I don't think asm goto can be made to work for vmx_return, though if we go for a big blob it could be useful to jump to the error handling code. I don't see anything bad in jumping completely to a different context. The guest and host are sort of like two coroutines, they hardly have any connection with the code that called vmlaunch. You can see it as two coroutines, or you can see it as linear logic: do host stuff enter guest do guest stuff exit guest continue doing host stuff Both can be implemented, I prefer linear one. I would prefer linear one to coroutine in any code design, no connection to vmx. Sometimes coroutine are better than alternative (and in those cases alternative is never a linear logic), but this is not the case. Fair enough. As things stand, we have only one version that works reliably with past/present/future compilers, and that is the one with setjmp/longjmp. A v5 would be needed anyway. If Arthur (and Jan) want to write the vmentry as a big asm blob, that's fine by me. Still, some variety adds value in a testsuite: think of a hypothetic nested VMX implementation that ignores HOST_RIP and just falls through to the next host %rip, we want that to fail the tests! (*) (*) In fact, I think this must be a requirement even if Arthur goes for a big asm blob. If they prefer to keep setjmp/longjmp and fix the few remaining nits, I think that should be acceptable anyway. It would even make sense to have multiple vmentries if you can show they stress the hypervisor differently. In any case, I think we all agree (Arthur too) that this RFC doing mixed asm and C is the worst of both worlds. The actually differences in asm instruction between both version will not be bigger then a couple of lines (if we will not take setjmp/longjmp implementation into account :)), I was waiting for this parenthetical remark to appear. ;) I've put a smile there :) I realize this argument is not completely fair, but for the sake of argument everything goes! Yes, I caught the irony. ;) Paolo -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
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. Paolo SAVE_GPR ); eax = exit_handler(); switch (eax) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_RESUME: break; default: printf(%s : unhandled ret from exit_handler.\n, __func__); return 1; } ret = vmresume(); } printf(%s : vmenter failed.\n, __func__); return 1; } -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@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? Arthur Paolo SAVE_GPR ); eax = exit_handler(); switch (eax) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_RESUME: break; default: printf(%s : unhandled ret from exit_handler.\n, __func__); return 1; } ret = vmresume(); } printf(%s : vmenter failed.\n, __func__); return 1; } -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 24/07/2013 08:46, Arthur Chunqi Li ha scritto: On Wed, Jul 24, 2013 at 2:40 PM, Paolo Bonzini pbonz...@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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
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 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 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 pbonz...@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 pbonz...@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 -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On 2013-07-24 10:48, Arthur Chunqi Li wrote: 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 Just like in KVM, provide a flag to the asm block that selects vmlaunch or vmresume, then grab all the required information on return and leave the asm block quickly again. Jan setbe %0\n\t jne 4f\n\t 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 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 pbonz...@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 pbonz...@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 signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
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 pbonz...@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 pbonz...@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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
So what about this one. I merged all the exit reason to ret and remove the flag detection after vmlaunch/vmresume (because I think this detection is useless). Currently we support only one guest, so variant launched is located in vmx_run(). If we want to support multiple guest, we could move it to some structures (e.g. environment_ctxt). Now I just put it here. static int vmx_run() { u32 ret = 0; bool launched = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmp $0, %1\n\t jne 1f\n\t vmlaunch\n\t SAVE_GPR_C /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ mov %3, %0\n\t jmp 2f\n\t 1: vmresume\n\t SAVE_GPR_C /* vmresume error, return VMX_TEST_RESUME_ERR */ mov %4, %0\n\t jmp 2f\n\t vmx_return:\n\t SAVE_GPR_C call exit_handler\n\t /* set launched = 1 */ mov $0x1, %1\n\t cmp %5, %%eax\n\t je 0b\n\t mov %%eax, %0\n\t 2: : =r(ret), =r(launched) : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR), i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME) : rax, rbx, rdi, rsi, r8, r9, r10, r11, r12, r13, r14, r15, memory, cc ); switch (ret) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_LAUNCH_ERR: printf(%s : vmlaunch failed.\n, __func__); break; case VMX_TEST_RESUME_ERR: printf(%s : vmresume failed.\n, __func__); break; default: printf(%s : unhandled ret from exit_handler, ret=%d.\n, __func__, ret); break; } return 1; } On Wed, Jul 24, 2013 at 5:16 PM, Paolo Bonzini pbonz...@redhat.com wrote: 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 pbonz...@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 pbonz...@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 -- Arthur
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On 2013-07-24 11:56, Arthur Chunqi Li wrote: So what about this one. I merged all the exit reason to ret and remove the flag detection after vmlaunch/vmresume (because I think this detection is useless). Currently we support only one guest, so variant launched is located in vmx_run(). If we want to support multiple guest, we could move it to some structures (e.g. environment_ctxt). Now I just put it here. static int vmx_run() { u32 ret = 0; bool launched = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmp $0, %1\n\t jne 1f\n\t vmlaunch\n\t SAVE_GPR_C /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ mov %3, %0\n\t jmp 2f\n\t 1: vmresume\n\t SAVE_GPR_C /* vmresume error, return VMX_TEST_RESUME_ERR */ mov %4, %0\n\t jmp 2f\n\t Where do you store the flags now? You may want to differentiate / test if ZF of CF is set. Jan vmx_return:\n\t SAVE_GPR_C call exit_handler\n\t /* set launched = 1 */ mov $0x1, %1\n\t cmp %5, %%eax\n\t je 0b\n\t mov %%eax, %0\n\t 2: : =r(ret), =r(launched) : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR), i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME) : rax, rbx, rdi, rsi, r8, r9, r10, r11, r12, r13, r14, r15, memory, cc ); switch (ret) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_LAUNCH_ERR: printf(%s : vmlaunch failed.\n, __func__); break; case VMX_TEST_RESUME_ERR: printf(%s : vmresume failed.\n, __func__); break; default: printf(%s : unhandled ret from exit_handler, ret=%d.\n, __func__, ret); break; } return 1; } signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote: On 2013-07-24 11:56, Arthur Chunqi Li wrote: So what about this one. I merged all the exit reason to ret and remove the flag detection after vmlaunch/vmresume (because I think this detection is useless). Currently we support only one guest, so variant launched is located in vmx_run(). If we want to support multiple guest, we could move it to some structures (e.g. environment_ctxt). Now I just put it here. static int vmx_run() { u32 ret = 0; bool launched = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmp $0, %1\n\t jne 1f\n\t vmlaunch\n\t SAVE_GPR_C /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ mov %3, %0\n\t jmp 2f\n\t 1: vmresume\n\t SAVE_GPR_C /* vmresume error, return VMX_TEST_RESUME_ERR */ mov %4, %0\n\t jmp 2f\n\t Where do you store the flags now? You may want to differentiate / test if ZF of CF is set. I store the flags as a global variant. You mean I need to detect ZF/CF after vmlaunch/vmresume? Arthur Jan vmx_return:\n\t SAVE_GPR_C call exit_handler\n\t /* set launched = 1 */ mov $0x1, %1\n\t cmp %5, %%eax\n\t je 0b\n\t mov %%eax, %0\n\t 2: : =r(ret), =r(launched) : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR), i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME) : rax, rbx, rdi, rsi, r8, r9, r10, r11, r12, r13, r14, r15, memory, cc ); switch (ret) { case VMX_TEST_VMEXIT: return 0; case VMX_TEST_LAUNCH_ERR: printf(%s : vmlaunch failed.\n, __func__); break; case VMX_TEST_RESUME_ERR: printf(%s : vmresume failed.\n, __func__); break; default: printf(%s : unhandled ret from exit_handler, ret=%d.\n, __func__, ret); break; } return 1; } -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On 2013-07-24 12:16, Arthur Chunqi Li wrote: On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote: On 2013-07-24 11:56, Arthur Chunqi Li wrote: So what about this one. I merged all the exit reason to ret and remove the flag detection after vmlaunch/vmresume (because I think this detection is useless). Currently we support only one guest, so variant launched is located in vmx_run(). If we want to support multiple guest, we could move it to some structures (e.g. environment_ctxt). Now I just put it here. static int vmx_run() { u32 ret = 0; bool launched = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmp $0, %1\n\t jne 1f\n\t vmlaunch\n\t SAVE_GPR_C /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ mov %3, %0\n\t jmp 2f\n\t 1: vmresume\n\t SAVE_GPR_C /* vmresume error, return VMX_TEST_RESUME_ERR */ mov %4, %0\n\t jmp 2f\n\t Where do you store the flags now? You may want to differentiate / test if ZF of CF is set. I store the flags as a global variant. You mean I need to detect ZF/CF after vmlaunch/vmresume? Yes - if you want to check correct emulation of those instructions completely. Jan signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
And what about this version: static int vmx_run() { u32 ret = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmpl $0, %1\n\t jne 1f\n\t vmlaunch;seta %1\n\t /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ movl %3, %0\n\t SAVE_GPR_C jmp 2f\n\t 1: vmresume;seta %1\n\t /* vmresume error, return VMX_TEST_RESUME_ERR */ movl %4, %0\n\t SAVE_GPR_C jmp 2f\n\t vmx_return:\n\t SAVE_GPR_C call exit_handler\n\t /* set launched = 1 */ movl $0x1, %1\n\t /* jump to resume when VMX_TEST_RESUME */ cmp %5, %%eax\n\t je 0b\n\t mov %%eax, %0\n\t 2: : =m(ret), =m(launched) : i(HOST_RSP), i(VMX_TEST_LAUNCH_ERR), i(VMX_TEST_RESUME_ERR), i(VMX_TEST_RESUME) : rax, rbx, rdi, rsi, r8, r9, r10, r11, r12, r13, r14, r15, memory, cc ); switch (ret) { case VMX_TEST_VMEXIT: launched = 0; return 0; case VMX_TEST_LAUNCH_ERR: printf(%s : vmlaunch failed.\n, __func__); if (launched != 0) printf(\tvmlaunch set flags error\n); report(test vmlaunch, 0); break; case VMX_TEST_RESUME_ERR: printf(%s : vmresume failed.\n, __func__); if (launched != 0) printf(\tvmlaunch set flags error\n); report(test vmresume, 0); break; default: launched = 0; printf(%s : unhandled ret from exit_handler, ret=%d.\n, __func__, ret); break; } return 1; } On Wed, Jul 24, 2013 at 6:24 PM, Jan Kiszka jan.kis...@web.de wrote: On 2013-07-24 12:16, Arthur Chunqi Li wrote: On Wed, Jul 24, 2013 at 6:03 PM, Jan Kiszka jan.kis...@web.de wrote: On 2013-07-24 11:56, Arthur Chunqi Li wrote: So what about this one. I merged all the exit reason to ret and remove the flag detection after vmlaunch/vmresume (because I think this detection is useless). Currently we support only one guest, so variant launched is located in vmx_run(). If we want to support multiple guest, we could move it to some structures (e.g. environment_ctxt). Now I just put it here. static int vmx_run() { u32 ret = 0; bool launched = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmp $0, %1\n\t jne 1f\n\t vmlaunch\n\t SAVE_GPR_C /* vmlaunch error, return VMX_TEST_LAUNCH_ERR */ mov %3, %0\n\t jmp 2f\n\t 1: vmresume\n\t SAVE_GPR_C /* vmresume error, return VMX_TEST_RESUME_ERR */ mov %4, %0\n\t jmp 2f\n\t Where do you store the flags now? You may want to differentiate / test if ZF of CF is set. I store the flags as a global variant. You mean I need to detect ZF/CF after vmlaunch/vmresume? Yes - if you want to check correct emulation of those instructions completely. Jan -- Arthur Chunqi Li Department of Computer Science School of EECS Peking University Beijing, China -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On 2013-07-24 13:20, Arthur Chunqi Li wrote: And what about this version: static int vmx_run() { u32 ret = 0; asm volatile( mov %%rsp, %%rsi\n\t mov %2, %%edi\n\t call vmcs_write\n\t 0: LOAD_GPR_C cmpl $0, %1\n\t jne 1f\n\t vmlaunch;seta %1\n\t That doesn't differentiate between CF and ZF (so that you can check if vmlaunch set the right flag). Also, only one instruction per line, please. Jan signature.asc Description: OpenPGP digital signature
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 18/07/2013 21:57, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote: Il 18/07/2013 13:06, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism The way the code is written is deceiving. If I see asm(vmlaunch; seta %0) while (ret) I expect HOST_RIP to point at the seta or somewhere near, not at a completely different label somewhere else. Why would you expect that assuming you know what vmlaunch is? Because this is written in C, and I know trying to fool the compiler is a losing game. So my reaction is okay, HOST_RIP must be set so that code will not jump around. If I see asm(vmlaunch) exit(-1) the reaction is the opposite: hmm, anything that jumps around would have a hard time with the compiler, there must be some assembly trampoline somewhere; let's check what HOST_RIP is. instead of longjmp()? Look again at the setjmp/longjmp version. longjmp is not used to handle vmexit. It is used to jump back from the vmexit handler to main, which is exactly what setjmp/longjmp is meant for. That's because simple return will not work in that version, this is artifact of how vmexit was done. I think it can be made to work without setjmp/longjmp, but the code would be ugly. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. Right, but my point is: without a big asm blob, you don't know the right value to load. It depends on the generated code. And the big asm blob limits a lot the code looks nice value of this approach. I said it number of times already, this is not about code looking nice, code looks like KVM or use less assembler as possible, this is about linear data flow. It is not fun chasing code execution path. Yes, you can argue that vmlaunch/vmresume inherently non linear, but there is a difference between skipping one instruction and remain in the same function and on the same stack, or jump completely to a different context. I don't see anything bad in jumping completely to a different context. The guest and host are sort of like two coroutines, they hardly have any connection with the code that called vmlaunch. The actually differences in asm instruction between both version will not be bigger then a couple of lines (if we will not take setjmp/longjmp implementation into account :)), I was waiting for this parenthetical remark to appear. ;) but I do not even see why we discuss this argument since minimizing asm instructions here is not an point. We should not use more then needed to achieve the goal of course, but design should not be around number of assembly lines. I agree, I only mentioned it because you talked about asm C asm C and this is not what the setjmp/longjmp code looks like---using inlines for asm as if they were builtin functions is good, interspersing asm and C in the same function is bad. Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Fri, Jul 19, 2013 at 08:42:20AM +0200, Paolo Bonzini wrote: Il 18/07/2013 21:57, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote: Il 18/07/2013 13:06, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism The way the code is written is deceiving. If I see asm(vmlaunch; seta %0) while (ret) I expect HOST_RIP to point at the seta or somewhere near, not at a completely different label somewhere else. Why would you expect that assuming you know what vmlaunch is? Because this is written in C, and I know trying to fool the compiler is a losing game. So my reaction is okay, HOST_RIP must be set so that code will not jump around. If I see asm(vmlaunch) exit(-1) the reaction is the opposite: hmm, anything that jumps around would have a hard time with the compiler, there must be some assembly trampoline somewhere; let's check what HOST_RIP is. We do try to fool compiler often even without vmx: code patching. This is why asm goto was invented, no? So, like you said in previous emails, asm goto is appropriate way here to tell compiler what is going on. instead of longjmp()? Look again at the setjmp/longjmp version. longjmp is not used to handle vmexit. It is used to jump back from the vmexit handler to main, which is exactly what setjmp/longjmp is meant for. That's because simple return will not work in that version, this is artifact of how vmexit was done. I think it can be made to work without setjmp/longjmp, but the code would be ugly. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. Right, but my point is: without a big asm blob, you don't know the right value to load. It depends on the generated code. And the big asm blob limits a lot the code looks nice value of this approach. I said it number of times already, this is not about code looking nice, code looks like KVM or use less assembler as possible, this is about linear data flow. It is not fun chasing code execution path. Yes, you can argue that vmlaunch/vmresume inherently non linear, but there is a difference between skipping one instruction and remain in the same function and on the same stack, or jump completely to a different context. I don't see anything bad in jumping completely to a different context. The guest and host are sort of like two coroutines, they hardly have any connection with the code that called vmlaunch. You can see it as two coroutines, or you can see it as linear logic: do host stuff enter guest do guest stuff exit guest continue doing host stuff Both can be implemented, I prefer linear one. I would prefer linear one to coroutine in any code design, no connection to vmx. Sometimes coroutine are better than alternative (and in those cases alternative is never a linear logic), but this is not the case. The actually differences in asm instruction between both version will not be bigger then a couple of lines (if we will not take setjmp/longjmp implementation into account :)), I was waiting for this parenthetical remark to appear. ;) I've put a smile there :) I realize this argument is not completely fair, but for the sake of argument everything goes! -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 19/07/2013 11:40, Gleb Natapov ha scritto: Because this is written in C, and I know trying to fool the compiler is a losing game. So my reaction is okay, HOST_RIP must be set so that code will not jump around. If I see asm(vmlaunch) exit(-1) the reaction is the opposite: hmm, anything that jumps around would have a hard time with the compiler, there must be some assembly trampoline somewhere; let's check what HOST_RIP is. We do try to fool compiler often even without vmx: code patching. This is why asm goto was invented, no? So, like you said in previous emails, asm goto is appropriate way here to tell compiler what is going on. Code patching is only reimplementing an existing C structure (if/else) in a different manner. Here the actual code flow (location of HOST_RIP and value of HOST_RSP) cannot be expressed correctly to the compiler because we do not use the C label (we use an asm label). I don't think asm goto can be made to work for vmx_return, though if we go for a big blob it could be useful to jump to the error handling code. I don't see anything bad in jumping completely to a different context. The guest and host are sort of like two coroutines, they hardly have any connection with the code that called vmlaunch. You can see it as two coroutines, or you can see it as linear logic: do host stuff enter guest do guest stuff exit guest continue doing host stuff Both can be implemented, I prefer linear one. I would prefer linear one to coroutine in any code design, no connection to vmx. Sometimes coroutine are better than alternative (and in those cases alternative is never a linear logic), but this is not the case. Fair enough. As things stand, we have only one version that works reliably with past/present/future compilers, and that is the one with setjmp/longjmp. A v5 would be needed anyway. If Arthur (and Jan) want to write the vmentry as a big asm blob, that's fine by me. Still, some variety adds value in a testsuite: think of a hypothetic nested VMX implementation that ignores HOST_RIP and just falls through to the next host %rip, we want that to fail the tests! (*) (*) In fact, I think this must be a requirement even if Arthur goes for a big asm blob. If they prefer to keep setjmp/longjmp and fix the few remaining nits, I think that should be acceptable anyway. It would even make sense to have multiple vmentries if you can show they stress the hypervisor differently. In any case, I think we all agree (Arthur too) that this RFC doing mixed asm and C is the worst of both worlds. The actually differences in asm instruction between both version will not be bigger then a couple of lines (if we will not take setjmp/longjmp implementation into account :)), I was waiting for this parenthetical remark to appear. ;) I've put a smile there :) I realize this argument is not completely fair, but for the sake of argument everything goes! Yes, I caught the irony. ;) Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Thu, Jul 18, 2013 at 07:52:21AM +0200, Paolo Bonzini wrote: Il 17/07/2013 20:54, Arthur Chunqi Li ha scritto: + .globl entry_sysenter\n\t + entry_sysenter:\n\t + SAVE_GPR + and $0xf, %rax\n\t + push%rax\n\t push should be wrong here, the first argument is in %rdi. + callsyscall_handler\n\t + LOAD_GPR + vmresume\n\t +); + +int exit_handler() This (and syscall_handler too) needs __attribute__((__used__)) because it's only used from assembly. That was my question actually, why is it used from assembly? Calling it from see should not be a problem. Please add static to all functions except main, it triggers better compiler optimization and warnings and it will avoid nasty surprises in the future if the compiler becomes smarter. +{ + int ret; + + current-exits++; + current-guest_regs = regs; + if (is_hypercall()) + ret = handle_hypercall(); + else + ret = current-exit_handler(); + regs = current-guest_regs; + switch (ret) { + case VMX_TEST_VMEXIT: + case VMX_TEST_RESUME: + return ret; + case VMX_TEST_EXIT: + break; + default: + printf(ERROR : Invalid exit_handler return val %d.\n + , ret); + } Here have to propagate the exit codes through multiple levels, because we're not using setjmp/longjmp. Not a big deal. My worries with this version are below. + print_vmexit_info(); + exit(-1); + return 0; +} + +int vmx_run() +{ + bool ret; + u32 eax; + u64 rsp; + + asm volatile(mov %%rsp, %0\n\t : =r(rsp)); + vmcs_write(HOST_RSP, rsp); + asm volatile(vmlaunch;seta %0\n\t : =m(ret)); setbe (this path is probably untested because it never happens in practice). At least one of the tests should set up wrong vmcs state to verify that nested vmx handles it correctly. In fact one of the patches that Arthur have sent to nested vmx fixes exactly that code path. Also, missing memory clobber. + if (ret) { + printf(%s : vmlaunch failed.\n, __func__); + return 1; + } + while (1) { + asm volatile( + LOAD_GPR_C + vmresume;seta %0\n\t + : =m(ret)); setbe and missing memory clobber. + if (ret) { + printf(%s : vmresume failed.\n, __func__); + return 1; + } + asm volatile( + .global vmx_return\n\t .global should not be needed. + vmx_return:\n\t + SAVE_GPR_C + call exit_handler\n\t + mov %%eax, %0\n\t + : =r(eax) + ); I had written a long explanation here about why I don't trust the compiler to do the right thing, and ideas about how to fix that. But in the end the only workable solution is a single assembly blob like vmx.c in KVM to do vmlaunch/vmresume, so I'll get right to the point: * the similarity with KVM code and as little assembly as * * possible goals are mutually exclusive * I never said that code should be similar to KVM code or have as little assembly as possible :) Reread the thread. The only thing I asked for is to make code flow linear, if it makes code looks more like KVM or reduce amount of assembly this is just a bonus. and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? However, it is also very deceiving, because the processor is not doing what the code says. If I see: vmlaunch(); exit(-1); it is clear that magic happens in vmlaunch(). If I see asm volatile(vmlaunch;setbe %0\n\t : =m(ret)); if (ret) { ... } asm(vmx_return:) it is absolutely not clear that the setbe and if are skipped on a successful vmlaunch. So, at the very least you need a comment before those if (ret) to document the control flow, or you can use a jmp like this: asm volatile goto (vmlaunch;jmp %0\n\t : : : memory : vmlaunch_error); if (0) { vmlaunch_error: ... } The unconditional jump and asm goto make it clear that magic happens. Agree, I dislike this magic too, but this is fixed by you suggestion above about putting vmx_return at the beginning of while(). So the logic will looks like that: asm volatile(vmlaunch;setbe %0\n\t : =m(ret)); while(ret) { vmx_return: SAVE_GPR_C eax = exit_handler(); switch(eax) { }
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 18/07/2013 09:26, Gleb Natapov ha scritto: I had written a long explanation here about why I don't trust the compiler to do the right thing, and ideas about how to fix that. But in the end the only workable solution is a single assembly blob like vmx.c in KVM to do vmlaunch/vmresume, so I'll get right to the point: * the similarity with KVM code and as little assembly as * * possible goals are mutually exclusive * I never said that code should be similar to KVM code or have as little assembly as possible :) Reread the thread. The only thing I asked for is to make code flow linear, if it makes code looks more like KVM or reduce amount of assembly this is just a bonus. Point taken. and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. Agree, I dislike this magic too, but this is fixed by you suggestion above about putting vmx_return at the beginning of while(). So the logic will looks like that: asm volatile(vmlaunch;setbe %0\n\t : =m(ret)); while(ret) { while(!ret) if you use setbe, of course. vmx_return: SAVE_GPR_C eax = exit_handler(); switch(eax) { } LOAD_GPR_C asm volatile(vmresume;seta %0\n\t : =m(ret)); } It is still somewhat magic: the while (ret) is only there to please the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print different error messages for vmlaunch vs. vmresume failure. In the end the choice is between all in asm and small asm trampoline (which also happens to use setjmp/longjmp, but perhaps Arthur can propagate return codes without using setjmp/longjmp, too). Rewriting the whole guest entry exit path in asm like you suggest bellow will eliminate the magic too. I much prefer one big asm statement than many small asm statement scattered among two or three C lines. It's not many asm statements, it's just a dozen lines: align 4, 0x90 entry_vmx: SAVE_GPR call default_exit_handler /* Should not reach here*/ mov $1, %eax call exit .align 4, 0x90 entry_sysenter: SAVE_GPR and $0xf, %eax mov %eax, %edi calldefault_syscall_handler /* Arthur, is something missing here? :)) */ Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism instead of longjmp()? Agree, I dislike this magic too, but this is fixed by you suggestion above about putting vmx_return at the beginning of while(). So the logic will looks like that: asm volatile(vmlaunch;setbe %0\n\t : =m(ret)); while(ret) { while(!ret) if you use setbe, of course. Yeah, this not meant to be compilable code :) vmx_return: SAVE_GPR_C eax = exit_handler(); switch(eax) { } LOAD_GPR_C asm volatile(vmresume;seta %0\n\t : =m(ret)); } It is still somewhat magic: the while (ret) is only there to please No, it it there to catch vmlaunch/vmresume errors. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. different error messages for vmlaunch vs. vmresume failure. Just because the same variable is used to store error values :) Make vmlaunch use err1 and vmresume err2 and do while (!err1 !err2) In the end the choice is between all in asm and small asm trampoline (which also happens to use setjmp/longjmp, but perhaps Arthur can propagate return codes without using setjmp/longjmp, too). Rewriting the whole guest entry exit path in asm like you suggest bellow will eliminate the magic too. I much prefer one big asm statement than many small asm statement scattered among two or three C lines. It's not many asm statements, it's just a dozen lines: I am not talking about overall file, but the how vmx_run() is written: asm() C code asm() C code ... I much prefer: C code big asm() blob C code. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 18/07/2013 13:06, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism The way the code is written is deceiving. If I see asm(vmlaunch; seta %0) while (ret) I expect HOST_RIP to point at the seta or somewhere near, not at a completely different label somewhere else. instead of longjmp()? Look again at the setjmp/longjmp version. longjmp is not used to handle vmexit. It is used to jump back from the vmexit handler to main, which is exactly what setjmp/longjmp is meant for. It is still somewhat magic: the while (ret) is only there to please the compiler No, it it there to catch vmlaunch/vmresume errors. You could change it to while (0) and it would still work. That's what I mean by only there to please the compiler. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. Right, but my point is: without a big asm blob, you don't know the right value to load. It depends on the generated code. And the big asm blob limits a lot the code looks nice value of this approach. different error messages for vmlaunch vs. vmresume failure. Just because the same variable is used to store error values :) Make vmlaunch use err1 and vmresume err2 and do while (!err1 !err2) Yeah. :) In the end the choice is between all in asm and small asm trampoline (which also happens to use setjmp/longjmp, but perhaps Arthur can propagate return codes without using setjmp/longjmp, too). Rewriting the whole guest entry exit path in asm like you suggest bellow will eliminate the magic too. I much prefer one big asm statement than many small asm statement scattered among two or three C lines. It's not many asm statements, it's just a dozen lines: I am not talking about overall file, but the how vmx_run() is written: asm() C code asm() C code ... I much prefer: C code big asm() blob C code. Me too. But the trampoline version is neither, it is static inline void vmlaunch() { asm(vmlaunch) } static inline void vmresume() { asm(vmresume) } small asm() blob C code Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Thu, Jul 18, 2013 at 8:08 PM, Paolo Bonzini pbonz...@redhat.com wrote: Il 18/07/2013 13:06, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism The way the code is written is deceiving. If I see asm(vmlaunch; seta %0) while (ret) I expect HOST_RIP to point at the seta or somewhere near, not at a completely different label somewhere else. Here for me, I prefer a separate asm blob of entry_vmx instead of one hidden in a C function, which may make it much harder to trace for someone not familiar with vmx in KVM. instead of longjmp()? Look again at the setjmp/longjmp version. longjmp is not used to handle vmexit. It is used to jump back from the vmexit handler to main, which is exactly what setjmp/longjmp is meant for. It is still somewhat magic: the while (ret) is only there to please the compiler No, it it there to catch vmlaunch/vmresume errors. You could change it to while (0) and it would still work. That's what I mean by only there to please the compiler. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. Right, but my point is: without a big asm blob, you don't know the right value to load. It depends on the generated code. And the big asm blob limits a lot the code looks nice value of this approach. different error messages for vmlaunch vs. vmresume failure. Just because the same variable is used to store error values :) Make vmlaunch use err1 and vmresume err2 and do while (!err1 !err2) Yeah. :) In the end the choice is between all in asm and small asm trampoline (which also happens to use setjmp/longjmp, but perhaps Arthur can propagate return codes without using setjmp/longjmp, too). Rewriting the whole guest entry exit path in asm like you suggest bellow will eliminate the magic too. I much prefer one big asm statement than many small asm statement scattered among two or three C lines. It's not many asm statements, it's just a dozen lines: I am not talking about overall file, but the how vmx_run() is written: asm() C code asm() C code ... I much prefer: C code big asm() blob C code. Me too. But the trampoline version is neither, it is static inline void vmlaunch() { asm(vmlaunch) } static inline void vmresume() { asm(vmresume) } small asm() blob C code So this is a style problem on the basis of right code generation indeed. When I write codes of this version, it occurs that the compiler optimized some of my codes and something goes wrong. If we use C style, we need setjmp/longjmp, otherwise we need big asm blob to forbid compiler optimization. I prefer to Paolo indeed as to my writing the two versions. Writing asm in C is sometimes uncomfortable (e.g. %rax and %%rax, and considering the C codes before and after the asm blob). Actually, we can hide setjmp in vmx_on and longjmp in the asm codes after executing exit_handler, thus we just need to call vmx_on to enter VM and return a designed value (e.g. -1) in exit_handler to exit VM. Then any following codes don't need to care about setjmp/longjmp. Arthur Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
On Thu, Jul 18, 2013 at 02:08:51PM +0200, Paolo Bonzini wrote: Il 18/07/2013 13:06, Gleb Natapov ha scritto: On Thu, Jul 18, 2013 at 12:47:46PM +0200, Paolo Bonzini wrote: and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. Why do you prefer setjmp/longjmp then? Because it is still deceiving, and I dislike the deceit more than I like the linear code flow. What is deceiving about it? Of course for someone who has no idea how vmx works the code will not be obvious, but why should we care. For someone who knows what is deceiving about returning into the same function guest was launched from by using VMX mechanism The way the code is written is deceiving. If I see asm(vmlaunch; seta %0) while (ret) I expect HOST_RIP to point at the seta or somewhere near, not at a completely different label somewhere else. Why would you expect that assuming you know what vmlaunch is? So it is OK when HOST_RIP point to somewhere outside a function, but deceiving if it point 3 lines down in the same function? I have a hard time following this logic. instead of longjmp()? Look again at the setjmp/longjmp version. longjmp is not used to handle vmexit. It is used to jump back from the vmexit handler to main, which is exactly what setjmp/longjmp is meant for. That's because simple return will not work in that version, this is artifact of how vmexit was done. It is still somewhat magic: the while (ret) is only there to please the compiler No, it it there to catch vmlaunch/vmresume errors. You could change it to while (0) and it would still work. That's what I mean by only there to please the compiler. But while (1) will not, so the code is executed, it is not just for compiler there, but it is executed only if vmlaunch/vmresume fails. the compiler, and you rely on the compiler not changing %rsp between the vmlaunch and the vmx_return label. Minor nit, you cannot anymore print HOST_RSP should be loaded on each guest entry. Right, but my point is: without a big asm blob, you don't know the right value to load. It depends on the generated code. And the big asm blob limits a lot the code looks nice value of this approach. I said it number of times already, this is not about code looking nice, code looks like KVM or use less assembler as possible, this is about linear data flow. It is not fun chasing code execution path. Yes, you can argue that vmlaunch/vmresume inherently non linear, but there is a difference between skipping one instruction and remain in the same function and on the same stack, or jump completely to a different context. Speaking about KVM. Guest enter/exit assembly blob is around ~50 lines if assembly code and more then half of that is saving restoring context. And the code plays some tricks there for optimization that we do not need to do here, so I expect the code to be even smaller, not much more then 10 lines of assembly excluding state save/restore. different error messages for vmlaunch vs. vmresume failure. Just because the same variable is used to store error values :) Make vmlaunch use err1 and vmresume err2 and do while (!err1 !err2) Yeah. :) In the end the choice is between all in asm and small asm trampoline (which also happens to use setjmp/longjmp, but perhaps Arthur can propagate return codes without using setjmp/longjmp, too). Rewriting the whole guest entry exit path in asm like you suggest bellow will eliminate the magic too. I much prefer one big asm statement than many small asm statement scattered among two or three C lines. It's not many asm statements, it's just a dozen lines: I am not talking about overall file, but the how vmx_run() is written: asm() C code asm() C code ... I much prefer: C code big asm() blob C code. Me too. But the trampoline version is neither, it is static inline void vmlaunch() { asm(vmlaunch) } static inline void vmresume() { asm(vmresume) } small asm() blob I is small only because context save restore is hidden behind macro and 4 asm instructions (vmlaunch/seta/vmresume/seta) a hidden somewhere else. The actually differences in asm instruction between both version will not be bigger then a couple of lines (if we will not take setjmp/longjmp implementation into account :)), but I do not even see why we discuss this argument since minimizing asm instructions here is not an point. We should not use more then needed to achieve the goal of course, but design should not be around number of assembly lines. C code Paolo -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in
[RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
This is the first version of VMX nested environment. It contains the basic VMX instructions test cases, including VMXON/VMXOFF/VMXPTRLD/ VMXPTRST/VMCLEAR/VMLAUNCH/VMRESUME/VMCALL. This patchalso tests the basic execution routine in VMX nested environment andlet the VM print Hello World to inform its successfully run. The first release also includes a test suite for vmenter (vmlaunch and vmresume). Besides, hypercall mechanism is included and currently it is used to invoke VM normal exit. New files added: x86/vmx.h : contains all VMX related macro declerations x86/vmx.c : main file for VMX nested test case Signed-off-by: Arthur Chunqi Li yzt...@gmail.com --- config-x86-common.mak |2 + config-x86_64.mak |1 + lib/x86/msr.h |5 + x86/cstart64.S|4 + x86/unittests.cfg |6 + x86/vmx.c | 681 + x86/vmx.h | 455 + 7 files changed, 1154 insertions(+) create mode 100644 x86/vmx.c create mode 100644 x86/vmx.h diff --git a/config-x86-common.mak b/config-x86-common.mak index 455032b..34a41e1 100644 --- a/config-x86-common.mak +++ b/config-x86-common.mak @@ -101,6 +101,8 @@ $(TEST_DIR)/asyncpf.elf: $(cstart.o) $(TEST_DIR)/asyncpf.o $(TEST_DIR)/pcid.elf: $(cstart.o) $(TEST_DIR)/pcid.o +$(TEST_DIR)/vmx.elf: $(cstart.o) $(TEST_DIR)/vmx.o + arch_clean: $(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \ $(TEST_DIR)/.*.d $(TEST_DIR)/lib/.*.d $(TEST_DIR)/lib/*.o diff --git a/config-x86_64.mak b/config-x86_64.mak index 4e525f5..bb8ee89 100644 --- a/config-x86_64.mak +++ b/config-x86_64.mak @@ -9,5 +9,6 @@ tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \ $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \ $(TEST_DIR)/pcid.flat tests += $(TEST_DIR)/svm.flat +tests += $(TEST_DIR)/vmx.flat include config-x86-common.mak diff --git a/lib/x86/msr.h b/lib/x86/msr.h index 509a421..281255a 100644 --- a/lib/x86/msr.h +++ b/lib/x86/msr.h @@ -396,6 +396,11 @@ #define MSR_IA32_VMX_VMCS_ENUM 0x048a #define MSR_IA32_VMX_PROCBASED_CTLS20x048b #define MSR_IA32_VMX_EPT_VPID_CAP 0x048c +#define MSR_IA32_VMX_TRUE_PIN 0x048d +#define MSR_IA32_VMX_TRUE_PROC 0x048e +#define MSR_IA32_VMX_TRUE_EXIT 0x048f +#define MSR_IA32_VMX_TRUE_ENTRY0x0490 + /* AMD-V MSRs */ diff --git a/x86/cstart64.S b/x86/cstart64.S index 24df5f8..0fe76da 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -4,6 +4,10 @@ .globl boot_idt boot_idt = 0 +.globl idt_descr +.globl tss_descr +.globl gdt64_desc + ipi_vector = 0x20 max_cpus = 64 diff --git a/x86/unittests.cfg b/x86/unittests.cfg index bc9643e..85c36aa 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -149,3 +149,9 @@ extra_params = --append 1000 `date +%s` file = pcid.flat extra_params = -cpu qemu64,+pcid arch = x86_64 + +[vmx] +file = vmx.flat +extra_params = -cpu host,+vmx +arch = x86_64 + diff --git a/x86/vmx.c b/x86/vmx.c new file mode 100644 index 000..a1023b1 --- /dev/null +++ b/x86/vmx.c @@ -0,0 +1,681 @@ +#include libcflat.h +#include processor.h +#include vm.h +#include desc.h +#include vmx.h +#include msr.h +#include smp.h +#include io.h + +int fails = 0, tests = 0; +u32 *vmxon_region; +struct vmcs *vmcs_root; +u32 vpid_cnt; +void *guest_stack, *guest_syscall_stack; +u32 ctrl_pin, ctrl_enter, ctrl_exit, ctrl_cpu[2]; +ulong fix_cr0_set, fix_cr0_clr; +ulong fix_cr4_set, fix_cr4_clr; +struct regs regs; +struct vmx_test *current; +u64 hypercall_field = 0; + +extern u64 gdt64_desc[]; +extern u64 idt_descr[]; +extern u64 tss_descr[]; +extern void *vmx_return; +extern void *entry_sysenter; +extern void *guest_entry; + +void report(const char *name, int result) +{ + ++tests; + if (result) + printf(PASS: %s\n, name); + else { + printf(FAIL: %s\n, name); + ++fails; + } +} + +int vmcs_clear(struct vmcs *vmcs) +{ + bool ret; + asm volatile (vmclear %1; setbe %0 : =q (ret) : m (vmcs) : cc); + return ret; +} + +u64 vmcs_read(enum Encoding enc) +{ + u64 val; + asm volatile (vmread %1, %0 : =rm (val) : r ((u64)enc) : cc); + return val; +} + +int vmcs_write(enum Encoding enc, u64 val) +{ + bool ret; + asm volatile (vmwrite %1, %2; setbe %0 + : =q(ret) : rm (val), r ((u64)enc) : cc); + return ret; +} + +int make_vmcs_current(struct vmcs *vmcs) +{ + bool ret; + + asm volatile (vmptrld %1; setbe %0 : =q (ret) : m (vmcs) : cc); + return ret; +} + +int save_vmcs(struct vmcs **vmcs) +{ + bool ret; + + asm volatile (vmptrst %1; setbe %0 : =q (ret) : m (*vmcs) : cc); + return ret; +} + +/* entry_sysenter */ +asm( + .align 4, 0x90\n\t + .globl entry_sysenter\n\t + entry_sysenter:\n\t + SAVE_GPR +
Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case
Il 17/07/2013 20:54, Arthur Chunqi Li ha scritto: +/* entry_sysenter */ +asm( + .align 4, 0x90\n\t + .globl entry_sysenter\n\t + entry_sysenter:\n\t + SAVE_GPR +and $0xf, %rax\n\t +push%rax\n\t push should be wrong here, the first argument is in %rdi. +callsyscall_handler\n\t + LOAD_GPR +vmresume\n\t +); + +int exit_handler() This (and syscall_handler too) needs __attribute__((__used__)) because it's only used from assembly. Please add static to all functions except main, it triggers better compiler optimization and warnings and it will avoid nasty surprises in the future if the compiler becomes smarter. +{ + int ret; + + current-exits++; + current-guest_regs = regs; + if (is_hypercall()) + ret = handle_hypercall(); + else + ret = current-exit_handler(); + regs = current-guest_regs; + switch (ret) { + case VMX_TEST_VMEXIT: + case VMX_TEST_RESUME: + return ret; + case VMX_TEST_EXIT: + break; + default: + printf(ERROR : Invalid exit_handler return val %d.\n + , ret); + } Here have to propagate the exit codes through multiple levels, because we're not using setjmp/longjmp. Not a big deal. My worries with this version are below. + print_vmexit_info(); + exit(-1); + return 0; +} + +int vmx_run() +{ + bool ret; + u32 eax; + u64 rsp; + + asm volatile(mov %%rsp, %0\n\t : =r(rsp)); + vmcs_write(HOST_RSP, rsp); + asm volatile(vmlaunch;seta %0\n\t : =m(ret)); setbe (this path is probably untested because it never happens in practice). Also, missing memory clobber. + if (ret) { + printf(%s : vmlaunch failed.\n, __func__); + return 1; + } + while (1) { + asm volatile( + LOAD_GPR_C + vmresume;seta %0\n\t + : =m(ret)); setbe and missing memory clobber. + if (ret) { + printf(%s : vmresume failed.\n, __func__); + return 1; + } + asm volatile( + .global vmx_return\n\t .global should not be needed. + vmx_return:\n\t + SAVE_GPR_C + call exit_handler\n\t + mov %%eax, %0\n\t + : =r(eax) + ); I had written a long explanation here about why I don't trust the compiler to do the right thing, and ideas about how to fix that. But in the end the only workable solution is a single assembly blob like vmx.c in KVM to do vmlaunch/vmresume, so I'll get right to the point: * the similarity with KVM code and as little assembly as * * possible goals are mutually exclusive * and for a testsuite I'd prefer the latter---which means I'd still favor setjmp/longjmp. Now, here is the long explanation. I must admit that the code looks nice. There are some nits I'd like to see done differently (such as putting vmx_return at the beginning of the while (1), and the vmresume asm at the end), but it is indeed nice. However, it is also very deceiving, because the processor is not doing what the code says. If I see: vmlaunch(); exit(-1); it is clear that magic happens in vmlaunch(). If I see asm volatile(vmlaunch;setbe %0\n\t : =m(ret)); if (ret) { ... } asm(vmx_return:) it is absolutely not clear that the setbe and if are skipped on a successful vmlaunch. So, at the very least you need a comment before those if (ret) to document the control flow, or you can use a jmp like this: asm volatile goto (vmlaunch;jmp %0\n\t : : : memory : vmlaunch_error); if (0) { vmlaunch_error: ... } The unconditional jump and asm goto make it clear that magic happens. By the way, asm goto is also needed to tell the compiler that the vmlaunch and vmresume asms reenter at vmx_resume(*). Without it, there is no guarantee that rsp is the same when you save it and at the vmx_resume entry point (in fact, I'd feel safer if you did the vmwrite to HOST_RSP and vmlaunch in the same asm goto. Using asm goto to tell the compiler about vmx_resume poses another problem. asm goto takes a C label, vmx_resume is an assembly label. The compiler might put extra instruction between the C label and vmx_resume, for example to read back values it has spilled to the stack. Overall, I don't trust the compiler to compile this code right (especially if we later include a 32-bit version) and the only solution is to write the whole thing in assembly. If you want to write the logic in C, you need the setjmp/longjmp version. That version needs no such guarantee because all the trampolines are away from the hands