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

[Paolo Bonzini <pbonzini@redhat.com>]

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?

The point of having contributors is to share the work, which implies
accepting different opinions.

>> 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.

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.

> 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.

Paolo