From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 0/3] Architecture independence layer - v0 Date: Wed, 22 Aug 2007 12:32:10 +0300 Message-ID: <46CC029A.70400@qumranet.com> References: <46CBFDFD.3090501@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: "kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org" To: Christian Ehrhardt Return-path: In-Reply-To: <46CBFDFD.3090501-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Christian Ehrhardt wrote: > Basics I used: > - Since architecture will not change at runtime I do not use a arch_ops = > structure > - Instead I used a hidden Kconfig value specifying the kvm architecture = > which then > influence the make file to link together the generic and the arch = > specific part > to one kvm module > = Isn't there already such a config variable (e.g. CONFIG_X86, = CONFIG_X86_64, CONFIG_S390)? > - I split the code into kvm_main.c (generic) and kvm_x86.h/kvm_x86.c = > (arch portion) > = Just x86.[ch], please. Similarly for all other new files. > - A new header kvm_arch.h specifies the interface the architecture has = > to implement > - The ioctl interfaces are implemented by the generic kvm_main.c in = > three ways > a) a completely generic function is covered by kvm_main.c completely > b) a completely arch dependent function is not covered in kvm_main.c - = > at the end > each ioctl goes to a architecture mapping doing the rest of the ioctl = > not handled > in the generic part e.g. in kvm_vcpu_ioctl: > default: > r =3D kvm_arch_vcpu_ioctl(filp, ioctl, arg); > } > c) functions that have a significant amount of shared code are = > implemented by > kvm_main.c using kvm_arch_* to implement the arch specific part. = > Either by just > mapping the old *_ioctl_whatever to *_arch_whatever or by moving some = > more of the > code out of the arch function up to the generic portion e.g.: > case KVM_RUN: > r =3D -EINVAL; > if (arg) > goto out; > r =3D kvm_arch_vcpu_run(vcpu, vcpu->run); > break; > - there is still a lot of potential to move code up and down along these = > split, but > thats it so far. I strive to improve that patch series until we are = > happy with it > = I agree with all these points. In addition, I'd like to point out that = in some cases an API is generic, but some of its implementation isn't, = and its arguments are not generic. The example I have in mind is = KVM_GET_REGS/KVM_SET_REGS. In this case, the argument structures should = be defined in arch dependent headers. > The patch compiles and works without issues on my Opteron based machine = > here, > unfortunately I have no Intel base HW to test. > I look forward to every comment. Even if this approach may not be what = > we want in > the end, at least it starts the discussion how to do it :-) > > Gr=FCsse / Regards > Christian Ehrhardt > > P.S. Still a todo and not (yet) covered in this patch proposal is the = > arch splitting of structures like vcpu to a generic and an arch part. > = Yes. I don't think this will be difficult. -- = error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/