Re: [PATCH v4 0/2] Basic nested VMX test suite

[Gleb Natapov <gleb@redhat.com>]

On Wed, Jul 17, 2013 at 09:52:57AM +0200, Paolo Bonzini wrote:
> Il 17/07/2013 08:21, Gleb Natapov ha scritto:
> > On Wed, Jul 17, 2013 at 02:08:54PM +0800, Arthur Chunqi Li wrote:
> > > Hi Gleb and Paolo,
> > > As your suggestion, I add general interface for adding test suite in
> > > this version. It is similar to the achievement of x86/vmx.c, and I
> > > also move tests for vmenter (vmlaunch and vmresume test) to an
> > > independent test suite.
> > > 
> > 
> > The general interface looks fine, can be extended if needed, but you
> > ignored my comment about refactoring vmx_run() to make vmexit return
> > just after vmresume. Do it, you will see how clearer the code and the
> > logic will be. 99% of code we are dealing with as a programmers is
> > linear, we are much better following liner logic.
> 
> It's normal to have "different taste", and if vmx.c is librarified it is
> quite expected that it looks somewhat different from KVM).  Besides, I
> think Arthur should look at KVM code as little as possible when writing
> the testsuite.
> 
This is not about taste, this is about hackability of the code. I will maintain it
and I want it to be as simple as possible given task it does. Looking similar to KVM
is additional bonus because the code will naturally look familiar to KVM maintainer.

> I think the current version is mostly fine, but I'd prefer to move the
> inline functions to vmx.h, and the tests to a separate file.  Perhaps
> lib/x86/vmx.h, lib/x86/vmx.c, and x86/vmx.c.
> 
> All knowledge of setjmp and longjmp should then be hidden in
> lib/x86/vmx.c, perhaps by putting
> 
> 	if (setjmp(env) == 0) {
> 		vmx_run();
> 		return 1;
> 	} else
> 		return 0;
> 
> or something like that in a new lib/x86/vmx.c function.
> 
Use of setjmp to redirect control flow here is absolutely unnecessary. HW
provides you with capability to return control flow back where you want
it but you ignore it and save/restore context by yourself. Why?! Just tell
HW to return to the point you want to return to!

Overall the code looks like it wants to hide the leads where code
execution will take you next moment.

--
			Gleb.