From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v4 0/2] Basic nested VMX test suite Date: Wed, 17 Jul 2013 13:31:53 +0300 Message-ID: <20130717103153.GA19599@redhat.com> References: <1374041153-32235-1-git-send-email-yzt356@gmail.com> <20130717062151.GJ8981@redhat.com> <51E64D59.5050307@redhat.com> <20130717090320.GS11772@redhat.com> <51E66FB4.9080100@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Arthur Chunqi Li , kvm , Jan Kiszka To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42774 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752959Ab3GQKb5 (ORCPT ); Wed, 17 Jul 2013 06:31:57 -0400 Content-Disposition: inline In-Reply-To: <51E66FB4.9080100@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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.