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

[Paolo Bonzini <pbonzini@redhat.com>]

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;
> +}