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?
[...]
> +
> + 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.
> + if (copy_from_user(coretype, type.type, type.typelen)) {
> + r = -EFAULT;
> + goto out_free;
> + }
> + coretype[type.typelen-1] = '\0';
> +
Alltogether I like your patch even if I would have done details very different
(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
needed 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 same
interface.
So kvm_vcpu_create_type might be misleading while something like
kvm_vcpu_create_info might be more generic.
Just to get some background - do anyone else see the need to specify a detail
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 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.
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.
--
Grüsse / 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/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
