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

[Paolo Bonzini <pbonzini@redhat.com>]

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
		call	default_syscall_handler
		/* Arthur, is something missing here? :)) */

Paolo