From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [RFC PATCH] kvm-unit-tests : Basic architecture of VMX nested test case Date: Thu, 18 Jul 2013 07:52:21 +0200 Message-ID: <51E78295.2010700@redhat.com> References: <1374087242-6125-1-git-send-email-yzt356@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, jan.kiszka@web.de, gleb@redhat.com To: Arthur Chunqi Li Return-path: Received: from mail-ee0-f51.google.com ([74.125.83.51]:40708 "EHLO mail-ee0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755872Ab3GRFwf (ORCPT ); Thu, 18 Jul 2013 01:52:35 -0400 Received: by mail-ee0-f51.google.com with SMTP id e52so1455128eek.24 for ; Wed, 17 Jul 2013 22:52:33 -0700 (PDT) In-Reply-To: <1374087242-6125-1-git-send-email-yzt356@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. > + " call syscall_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 of the compiler. Paolo > + 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; > + } > + } > + return 0; > +}