From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Ehrhardt Subject: Re: [PATCH 3 of 3] [POWERPC] Implement an ioctl that creates a vcpu of a particular type Date: Tue, 05 Feb 2008 13:52:25 +0100 Message-ID: <47A85C09.5070609@linux.vnet.ibm.com> References: 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, kvm-ppc-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, Avi Kivity , Carsten Otte To: Hollis Blanchard Return-path: In-Reply-To: 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 Hollis Blanchard wrote: > The ioctl accepts a core name as input and calls kvm_vm_ioctl_create_vcpu= () > with the corresponding "guest operations" structure. That structure, which > will be extended with additional core-specific function pointers, is save= d into > the new vcpu by kvm_arch_vcpu_create(). > = > Signed-off-by: Hollis Blanchard > = [..] > +static struct kvmppc_core_spec kvmppc_supported_guests[] =3D { > + { > + .name =3D "ppc440", > + .vcpu_size =3D sizeof(struct kvm_vcpu), Just for clarification, here you want to add other configuration settings y= ou want to imply with the set of the cpu core right ? E.g. we have discussed the pvr initialization on kvm-powerpc-devel and #kvm= , do you intend to define the ppv440-pvr we want to set here in this struct? [...] > + > + if (type.typelen > 32) { > + r =3D -E2BIG; > + goto out; > + } > + > + coretype =3D vmalloc(type.typelen); > + if (!coretype) { > + r =3D -ENOMEM; > + goto out; > + } If I don't overlook something we could use strnlen here instead of type.typ= elen and not trust userspace in any way (which is always good) that it pass= es us a good value. > + if (copy_from_user(coretype, type.type, type.typelen)) { > + r =3D -EFAULT; > + goto out_free; > + } > + coretype[type.typelen-1] =3D '\0'; > + Alltogether I like your patch even if I would have done details very differ= ent (and worse) and the main question about all that I wanted to point out = is in your [0/3] mail: "... is this approach acceptable?" And if not what what would be alternative suggestions to pass information n= eeded for vcpu_create? > +struct kvm_vcpu_create_type { > + __u32 id; > + __u32 typelen; > + char type[0]; > +}; This should work for other architectures too. While we need to specify cpu = cores here others might specify something completely different with the sam= e interface. So kvm_vcpu_create_type might be misleading while something like kvm_vcpu_c= reate_info might be more generic. Just to get some background - do anyone else see the need to specify a deta= il on vcpu_create for their implementation in future ? Finally I wanted to add something more to think about while we discuss this= interface. The approach itself looks good to me, but it might somewhen nee= d an extension we should discuss and decide by now. Think of the following situation: a) In two years we might have some generic features which are part of the s= hared vcpu struct and we would need to specify sometihng for these features= on vcpu_create b) At the same time we would still have our need to specify arch dependent = information on vcpu create. Because the KVM_CREATE_VCPU_TYPE ioctl implies a vcpu create, we would need= to set up these new stuff prior to that create in another new call. I thin= k it might be much better if we would think now of how to specify multiple = things in this extended vcpu_create. This might either be done by removing the implicit vcpu creation for a work= flow like that: 1. set generic feature a enabled 2. set generic feature b disabled 3. set arch specific core type to foo 4. create_vcpu (using all that) Or on the other Hand we might think of a encapsulated or chained informatio= n that allows us to pass a variable number of settings with KVM_CREATE_VCPU= _TYPE. -- = Gr=FCsse / regards, = Christian Ehrhardt IBM Linux Technology Center, Open Virtualization ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/