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

[Gleb Natapov <gleb@redhat.com>]

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.
> 
> > +	"	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.
> 
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) {
   }
   LOAD_GPR_C
   asm volatile("vmresume;seta %0\n\t" : "=m"(ret));
}


Rewriting the whole guest entry exit path in asm like you suggest bellow
will eliminate the magic too.

> 
> 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.
> 
I much prefer one big asm statement than many small asm statement
scattered among two or three C lines.

--
			Gleb.