Re: [PATCH 3 of 3] [POWERPC] Implement an ioctl that creates a vcpu of a particular type

[Christian Ehrhardt <ehrhardt-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.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 saved into
> the new vcpu by kvm_arch_vcpu_create().
> 
> Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
[..]
> +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/