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

[Gleb Natapov <gleb@redhat.com>]

On Wed, Jul 17, 2013 at 12:19:32PM +0200, Paolo Bonzini wrote:
> Il 17/07/2013 11:03, Gleb Natapov ha scritto:
> > 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.
> 
> Then you may just as well write it yourself, no?
> 
What the point of the question? That may apply to any comment of any
reviewer if it does not point to a bug. So if it compiles - apply it?

> The point of having contributors is to share the work, which implies
> accepting different opinions.
> 
This is about design of a test suit, this is important. I am not
asking changing variable names for no good reason (sometimes there is a
reason to ask for that too).

> >> 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!
> 
> This is not super-optimized kernel code, this has to be readable first
> and foremost.  In C, the way to do global jumps and save/restore context
> is setjmp/longjmp.
If you do it right there will be _not point in doing_ global jumps. The
control flow will be linear, the code will be much more readable.

> 
> The way Arthur structured the code, also, relives me from the need of
> thinking about the ABI and about putting compiler barriers at the right
> places.  The hardware invokes assembly code that has no ABI
> requirements, the assembly code sets up whatever the C calling
> conventions need, and invokes a C function.
> 
Less assembly is good, not bad. Anyway I do not see how changing where
vmresume returns changes all that.

> > Overall the code looks like it wants to hide the leads where code
> > execution will take you next moment.
> 
> I agree there are some improvements that can be made in the code.  For
> example, there is no need for this kind of indirection:
> 
> +		case VMX_RESUME:
> +			goto vmx_resume;
> ...
> 
> +vmx_resume:
> +	vmx_resume();
> +	/* Should not reach here */
> +	exit(-1);
> 
> But with a few kinks fixed, something like this:
> 
> 	void default_exit_handler()
> 		...
> 
> 		switch (ret) {
> 		case VMXTEST_HALT:
> 			longjmp(env, 1);
> 			printf("Error in longjmp?\n");
> 			break;
> 		case VMXTEST_RESUME:
> 			vmx_resume();
> 			printf("Error in VMRESUME?\n");
> 			break;
> 		case VMXTEST_ABORT:
> 			break;
> 		default:
> 			printf("ERROR : Invalid exit_handler return "
> 			       "val, %d.\n", ret);
> 			break;
> 		}
> 		/* entry_vmx will exit */
> 	}
> 
> is perfectly readable and idiomatic C.
> 
Do I ask for something that will make it unreadable and non idiomatic C#?

--
			Gleb.