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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
