On Tue, 2008-02-05 at 13:52 +0100, Christian Ehrhardt wrote:
> 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 saved
> > into
> > the new vcpu by kvm_arch_vcpu_create().
> >
> > Signed-off-by: Hollis Blanchard <[EMAIL PROTECTED]>
> >
> [..]
> > +static struct kvmppc_core_spec kvmppc_supported_guests[] = {
> > + {
> > + .name = "ppc440",
> > + .vcpu_size = sizeof(struct kvm_vcpu),
>
> Just for clarification, here you want to add other configuration
> settings you 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?
Yes, I tried to hint at this in the first mail. Note however, that this
approach is an alternate design: rather than embedded a "cpu type"
integer in the vcpu and adding switch tables at all decision points, we
will add callbacks to this structure.
I have a followup patch to handle our TLB type disambiguation, but it
was late and I didn't want to clean it up.
> > +
> > + if (type.typelen > 32) {
> > + r = -E2BIG;
> > + goto out;
> > + }
> > +
> > + coretype = vmalloc(type.typelen);
> > + if (!coretype) {
> > + r = -ENOMEM;
> > + goto out;
> > + }
>
> If I don't overlook something we could use strnlen here instead of
> type.typelen and not trust userspace in any way (which is always good)
> that it passes us a good value.
I wasn't liking that part of the interface anyways, so strnlen() sounds
good to me. :)
> > +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 same interface.
> So kvm_vcpu_create_type might be misleading while something like
> kvm_vcpu_create_info might be more generic.
Yup, I think it makes sense to abstract this from a string to an
arch-defined structure.
Another arch-specific interface might be needed to query capabilities in
a similar way. Right now the extension stuff is based on encoded
constants, but I'd explicitly like to avoid that because then you must
also know the meaning of each constant. In other words, if you have
#define POWERPC440 1
both the kernel and userspace must understand what "1" means as a
parameter. That's why I went with strings...
On the other hand, encoded constants might be a better approach. For
example, if we change what information we want to pass down for a
particular vcpu type, we could do it like this:
struct vcpu_type {
__u32 type;
union {
struct { foo; } type1;
struct { foo; bar; } type2;
}
}
and we could then use extension queries to find out if type=1 and type=2
are supported.
> Just to get some background - do anyone else see the need to specify a
> detail on vcpu_create for their implementation in future ?
After talking to Carsten, it sounds like S390 could use this approach as
well.
> 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 need 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 shared 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 think it might be much better if we would think now of how to
> specify multiple things in this extended vcpu_create.
Agreed, I think we can handle that with my thoughts above.
> This might either be done by removing the implicit vcpu creation for a
> workflow 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
> information that allows us to pass a variable number of settings with
> KVM_CREATE_VCPU_TYPE.
I don't like this design because it's inherently racy. It depends on no
other actions taking place between "set feature bit" and "create vcpu".
It's far better IMHO to supply all needed information at the same time.
--
Hollis Blanchard
IBM Linux Technology Center
-------------------------------------------------------------------------
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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
