From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v4 0/2] Basic nested VMX test suite Date: Wed, 17 Jul 2013 09:52:57 +0200 Message-ID: <51E64D59.5050307@redhat.com> References: <1374041153-32235-1-git-send-email-yzt356@gmail.com> <20130717062151.GJ8981@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Arthur Chunqi Li , kvm , Jan Kiszka To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:4354 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751358Ab3GQHxJ (ORCPT ); Wed, 17 Jul 2013 03:53:09 -0400 In-Reply-To: <20130717062151.GJ8981@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. Paolo