[PATCH 0 of 3] RFC: creating a particular vcpu type

[PATCH 0 of 3] RFC: creating a particular vcpu type

[Hollis Blanchard]

These patches allow PowerPC to create vcpus of a particular type. Since we are
actually emulating the core's supervisor mode, we can choose to emulate any
type of core. However, since the core chosen will change the size of the vcpu
structure (among other things), we need to know it at vcpu creation time,
rather than after the fact (which is how x86's cpuid is handled).

I've included the first example of how PowerPC will be using the new
capability, and this will be significantly extended in the future. I think you
get the idea...

I still need to update my tree and patch IA64 to match, but is this approach
acceptable?

6 files changed, 99 insertions(+), 11 deletions(-)
arch/powerpc/kvm/powerpc.c     |   80 ++++++++++++++++++++++++++++++++++++++--
arch/x86/kvm/x86.c             |    4 +-
include/asm-powerpc/kvm_host.h |    5 ++
include/linux/kvm.h            |    8 ++++
include/linux/kvm_host.h       |    4 +-
virt/kvm/kvm_main.c            |    9 ++--

-------------------------------------------------------------------------
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/

[PATCH 1 of 3] Pass an opaque parameter through kvm_vm_ioctl_vcpu_create() to kvm_arch_vcpu_create()

[Hollis Blanchard]

# HG changeset patch
# User Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
# Date 1202189664 21600
# Node ID bede9476e203f5bf59d21cc3cd71a30de2ce2c44
# Parent  dfb0e1d58b57dfdf76b3111565815599bd38b92d

Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

---
4 files changed, 9 insertions(+), 7 deletions(-)
arch/powerpc/kvm/powerpc.c |    3 ++-
arch/x86/kvm/x86.c         |    4 ++--
include/linux/kvm_host.h   |    3 ++-
virt/kvm/kvm_main.c        |    6 +++---


diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -460,7 +460,8 @@ int kvm_arch_set_memory_region(struct kv
 	return 0;
 }
 
-struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
+struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id,
+                                      void *opaque)
 {
 	struct kvm_vcpu *vcpu;
 	int err;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3052,8 +3052,8 @@ void kvm_arch_vcpu_free(struct kvm_vcpu 
 	kvm_x86_ops->vcpu_free(vcpu);
 }
 
-struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm,
-						unsigned int id)
+struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id,
+                                      void *opaque)
 {
 	return kvm_x86_ops->vcpu_create(kvm, id);
 }
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -237,7 +237,8 @@ void kvm_arch_vcpu_free(struct kvm_vcpu 
 void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu);
-struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id);
+struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id,
+                                      void *opaque);
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu);
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -733,7 +733,7 @@ static int create_vcpu_fd(struct kvm_vcp
 /*
  * Creates some virtual cpus.  Good luck creating more than one.
  */
-static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n)
+static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n, void *opaque)
 {
 	int r;
 	struct kvm_vcpu *vcpu;
@@ -741,7 +741,7 @@ static int kvm_vm_ioctl_create_vcpu(stru
 	if (!valid_vcpu(n))
 		return -EINVAL;
 
-	vcpu = kvm_arch_vcpu_create(kvm, n);
+	vcpu = kvm_arch_vcpu_create(kvm, n, opaque);
 	if (IS_ERR(vcpu))
 		return PTR_ERR(vcpu);
 
@@ -945,7 +945,7 @@ static long kvm_vm_ioctl(struct file *fi
 		return -EIO;
 	switch (ioctl) {
 	case KVM_CREATE_VCPU:
-		r = kvm_vm_ioctl_create_vcpu(kvm, arg);
+		r = kvm_vm_ioctl_create_vcpu(kvm, arg, NULL);
 		if (r < 0)
 			goto out;
 		break;

-------------------------------------------------------------------------
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/

[PATCH 2 of 3] Export kvm_vm_ioctl_create_vcpu() to be called from architecture modules

[Hollis Blanchard]

# HG changeset patch
# User Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
# Date 1202189666 21600
# Node ID 7dd50dab9096c8e0125792e3f48083c3f47fceab
# Parent  bede9476e203f5bf59d21cc3cd71a30de2ce2c44

Signed-off-by: Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

---
2 files changed, 3 insertions(+), 1 deletion(-)
include/linux/kvm_host.h |    1 +
virt/kvm/kvm_main.c      |    3 ++-


diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -208,6 +208,7 @@ int kvm_vm_ioctl_set_memory_region(struc
 				   struct
 				   kvm_userspace_memory_region *mem,
 				   int user_alloc);
+int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n, void *opaque);
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg);
 void kvm_arch_destroy_vm(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -733,7 +733,7 @@ static int create_vcpu_fd(struct kvm_vcp
 /*
  * Creates some virtual cpus.  Good luck creating more than one.
  */
-static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n, void *opaque)
+int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, int n, void *opaque)
 {
 	int r;
 	struct kvm_vcpu *vcpu;
@@ -774,6 +774,7 @@ vcpu_destroy:
 	kvm_arch_vcpu_destroy(vcpu);
 	return r;
 }
+EXPORT_SYMBOL_GPL(kvm_vm_ioctl_create_vcpu);
 
 static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu *vcpu, sigset_t *sigset)
 {

-------------------------------------------------------------------------
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/

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

[Hollis Blanchard]

# HG changeset patch
# User Hollis Blanchard <hollisb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
# Date 1202189668 21600
# Node ID e6e0239e8df55c6af4e0b2959350215aaa119254
# Parent  7dd50dab9096c8e0125792e3f48083c3f47fceab
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>

---
3 files changed, 87 insertions(+), 3 deletions(-)
arch/powerpc/kvm/powerpc.c     |   77 ++++++++++++++++++++++++++++++++++++++--
include/asm-powerpc/kvm_host.h |    5 ++
include/linux/kvm.h            |    8 ++++


diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -460,13 +460,22 @@ int kvm_arch_set_memory_region(struct kv
 	return 0;
 }
 
+/* XXX Make modules register kvmppc_core_spec pointers at init. */
+static struct kvmppc_core_spec kvmppc_supported_guests[] = {
+	{
+		.name = "ppc440",
+		.vcpu_size = sizeof(struct kvm_vcpu),
+	},
+};
+
 struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id,
                                       void *opaque)
 {
+	struct kvmppc_core_spec *s = opaque;
 	struct kvm_vcpu *vcpu;
 	int err;
 
-	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
+	vcpu = vmalloc(s->vcpu_size);
 	if (!vcpu) {
 		err = -ENOMEM;
 		goto out;
@@ -479,7 +488,7 @@ struct kvm_vcpu *kvm_arch_vcpu_create(st
 	return vcpu;
 
 free_vcpu:
-	kmem_cache_free(kvm_vcpu_cache, vcpu);
+	vfree(vcpu);
 out:
 	return ERR_PTR(err);
 }
@@ -487,7 +496,7 @@ void kvm_arch_vcpu_free(struct kvm_vcpu 
 void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu)
 {
 	kvm_vcpu_uninit(vcpu);
-	kmem_cache_free(kvm_vcpu_cache, vcpu);
+	vfree(vcpu);
 }
 
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -800,12 +809,74 @@ int kvm_vm_ioctl_get_dirty_log(struct kv
 	return -ENOTSUPP;
 }
 
+static struct kvmppc_core_spec *kvmppc_guest_type(char *coretype)
+{
+	struct kvmppc_core_spec *c = kvmppc_supported_guests;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kvmppc_supported_guests); i++,c++)
+		if (strcmp(c->name, coretype))
+			return c;
+
+	return 0;
+}
+
+static long kvmppc_arch_vcpu_create_type(struct kvm *kvm,
+                                  struct kvm_vcpu_create_type __user *user_type)
+{
+	struct kvm_vcpu_create_type type;
+	struct kvmppc_core_spec *s;
+	char *coretype;
+	long r;
+
+	if (copy_from_user(&type, user_type, sizeof(type))) {
+		r = -EFAULT;
+		goto out;
+	}
+
+	if (type.typelen > 32) {
+		r = -E2BIG;
+		goto out;
+	}
+
+	coretype = vmalloc(type.typelen);
+	if (!coretype) {
+		r = -ENOMEM;
+		goto out;
+	}
+
+	if (copy_from_user(coretype, type.type, type.typelen)) {
+		r = -EFAULT;
+		goto out_free;
+	}
+	coretype[type.typelen-1] = '\0';
+
+	s = kvmppc_guest_type(coretype);
+	if (!s) {
+		r = -ENOTSUPP;
+		goto out_free;
+	}
+
+	r = kvm_vm_ioctl_create_vcpu(kvm, type.id, s);
+
+out_free:
+	vfree(coretype);
+out:
+	return r;
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
                        unsigned int ioctl, unsigned long arg)
 {
+	struct kvm *kvm = filp->private_data;
+	void __user *argp = (void __user *)arg;
 	long r;
 
 	switch (ioctl) {
+	case KVM_CREATE_VCPU_TYPE: {
+		r = kvmppc_arch_vcpu_create_type(kvm, argp);
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
diff --git a/include/asm-powerpc/kvm_host.h b/include/asm-powerpc/kvm_host.h
--- a/include/asm-powerpc/kvm_host.h
+++ b/include/asm-powerpc/kvm_host.h
@@ -49,6 +49,11 @@ struct tlbe {
 };
 
 struct kvm_arch {
+};
+
+struct kvmppc_core_spec {
+	const char *name;
+	unsigned int vcpu_size;
 };
 
 struct kvm_vcpu_arch {
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -211,6 +211,12 @@ struct kvm_vapic_addr {
 	__u64 vapic_addr;
 };
 
+struct kvm_vcpu_create_type {
+	__u32 id;
+	__u32 typelen;
+	char type[0];
+};
+
 #define KVMIO 0xAE
 
 /*
@@ -257,6 +263,8 @@ struct kvm_vapic_addr {
 #define KVM_GET_DIRTY_LOG         _IOW(KVMIO, 0x42, struct kvm_dirty_log)
 #define KVM_SET_MEMORY_ALIAS      _IOW(KVMIO, 0x43, struct kvm_memory_alias)
 #define KVM_GET_SUPPORTED_CPUID   _IOWR(KVMIO, 0x48, struct kvm_cpuid2)
+#define KVM_CREATE_VCPU_TYPE      _IOW(KVMIO,  0x49, struct kvm_vcpu_create_type)
+
 /* Device model IOC */
 #define KVM_CREATE_IRQCHIP	  _IO(KVMIO,  0x60)
 #define KVM_IRQ_LINE		  _IOW(KVMIO, 0x61, struct kvm_irq_level)

-------------------------------------------------------------------------
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/

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

[Christian Ehrhardt]

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/

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

[Carsten Otte]

Christian Ehrhardt wrote:
> 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 ?
We do. For now, kvm_create_vcpu does create a cpu from a template. 
However our hardware control block allows to enable/disable various 
features. I would imagine that future userspace may want something 
different than the stock default values, and therefore a parameter 
would make sense for us too.

-------------------------------------------------------------------------
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/

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

[Carsten Otte]

Carsten Otte wrote:
> We do. For now, kvm_create_vcpu does create a cpu from a template. 
> However our hardware control block allows to enable/disable various 
> features. I would imagine that future userspace may want something 
> different than the stock default values, and therefore a parameter would 
> make sense for us too.
I suppose that parameter is supposed to show up in 
kvm_arch_vcpu_setup, not kvm_arch_vcpu_create?

-------------------------------------------------------------------------
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/

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

[Hollis Blanchard]

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 <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?

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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Anthony Liguori]

Why do this at the VCPU level?  Would you ever want a VM with two VCPUs 
with different cores?  You could just add a per-VM arch ioctl to set the 
core type that has to be issued before any VCPU creates.  Then you don't 
have to do ugly stuff like calling ioctls from modules.

Regards,

Anthony Liguori

Hollis Blanchard wrote:
> These patches allow PowerPC to create vcpus of a particular type. Since we are
> actually emulating the core's supervisor mode, we can choose to emulate any
> type of core. However, since the core chosen will change the size of the vcpu
> structure (among other things), we need to know it at vcpu creation time,
> rather than after the fact (which is how x86's cpuid is handled).
>
> I've included the first example of how PowerPC will be using the new
> capability, and this will be significantly extended in the future. I think you
> get the idea...
>
> I still need to update my tree and patch IA64 to match, but is this approach
> acceptable?
>
> 6 files changed, 99 insertions(+), 11 deletions(-)
> arch/powerpc/kvm/powerpc.c     |   80 ++++++++++++++++++++++++++++++++++++++--
> arch/x86/kvm/x86.c             |    4 +-
> include/asm-powerpc/kvm_host.h |    5 ++
> include/linux/kvm.h            |    8 ++++
> include/linux/kvm_host.h       |    4 +-
> virt/kvm/kvm_main.c            |    9 ++--
>
> -------------------------------------------------------------------------
> 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-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>   


-------------------------------------------------------------------------
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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Hollis Blanchard]

If it's the "ioctl" in the function name you object to, that's easily
changed.

I think it's reasonable to say that single-system-image software
requires identical cores, but that's not what we're talking about here.
Heterogeneous core designs are not common, but a VM needs to reflect
hardware layout, and people do it in hardware (again, not running a
single system image).

"VCPU type" is a VCPU property, and I think the design should reflect
that, and as you can see from the patch it's not at all difficult to do.

-- 
Hollis Blanchard
IBM Linux Technology Center

On Tue, 2008-02-05 at 10:44 -0600, Anthony Liguori wrote:
> Why do this at the VCPU level?  Would you ever want a VM with two VCPUs 
> with different cores?  You could just add a per-VM arch ioctl to set the 
> core type that has to be issued before any VCPU creates.  Then you don't 
> have to do ugly stuff like calling ioctls from modules.
> 
> Regards,
> 
> Anthony Liguori
> 
> Hollis Blanchard wrote:
> > These patches allow PowerPC to create vcpus of a particular type. Since we are
> > actually emulating the core's supervisor mode, we can choose to emulate any
> > type of core. However, since the core chosen will change the size of the vcpu
> > structure (among other things), we need to know it at vcpu creation time,
> > rather than after the fact (which is how x86's cpuid is handled).
> >
> > I've included the first example of how PowerPC will be using the new
> > capability, and this will be significantly extended in the future. I think you
> > get the idea...
> >
> > I still need to update my tree and patch IA64 to match, but is this approach
> > acceptable?
> >
> > 6 files changed, 99 insertions(+), 11 deletions(-)
> > arch/powerpc/kvm/powerpc.c     |   80 ++++++++++++++++++++++++++++++++++++++--
> > arch/x86/kvm/x86.c             |    4 +-
> > include/asm-powerpc/kvm_host.h |    5 ++
> > include/linux/kvm.h            |    8 ++++
> > include/linux/kvm_host.h       |    4 +-
> > virt/kvm/kvm_main.c            |    9 ++--
> >
> > -------------------------------------------------------------------------
> > 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-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > https://lists.sourceforge.net/lists/listinfo/kvm-devel
> >   



-------------------------------------------------------------------------
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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Anthony Liguori]

Hollis Blanchard wrote:
> If it's the "ioctl" in the function name you object to, that's easily
> changed.
>   

It's not the name, it's what you're doing.  You're introducing an 
architecture specific ioctl that essentially overrides an 
common-ioctl().  If anything, I would think it would be better to expand 
the existing common-ioctl with the notion and then have a 
per-architecture hook within that ioctl.

> I think it's reasonable to say that single-system-image software
> requires identical cores, but that's not what we're talking about here.
> Heterogeneous core designs are not common, but a VM needs to reflect
> hardware layout, and people do it in hardware (again, not running a
> single system image).
>
> "VCPU type" is a VCPU property, and I think the design should reflect
> that, and as you can see from the patch it's not at all difficult to do.
>   

There are all sorts of crazy things you can do in actual hardware.  Look 
at the way the cell processor handles memory for SPUs.  You would need 
to change the KVM memory code to have per-CPU slots or something to 
support that.

We shouldn't design for any possible thing that anyone could want in the 
future, but rather for the things that we actually can see doing.

If you don't think there's a reasonable chance that you'll be attempting 
to implement asymmetric cores in the near future, I wouldn't think it's 
wise to over complicate things.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Hollis Blanchard]

On Tue, 2008-02-05 at 12:05 -0600, Anthony Liguori wrote:
> 
> Hollis Blanchard wrote:
> > If it's the "ioctl" in the function name you object to, that's
> easily
> > changed.
> >   
> 
> It's not the name, it's what you're doing.  You're introducing an 
> architecture specific ioctl that essentially overrides an 
> common-ioctl().

No, I am introducing an architecture-specific ioctl that shares common
code, which after all is the goal.

> If anything, I would think it would be better to expand the existing
> common-ioctl with the notion and then have a 
> per-architecture hook within that ioctl.

I *am* expanding the common ioctl. I am also preserving the existing
ABI: CREATE_VCPU still works, and CREATE_VCPU_TYPE is the new ioctl. And
then, voila, we have an architecture-specific hook:
kvm_arch_vcpu_create().

I will happily move the KVM_CREATE_VCPU_TYPE case from
kvm_arch_vm_ioctl() to kvm_vm_ioctl(), and since the additional
parameter is necessarily architecture-specific, it will simply call
kvm_arch_vcpu_create_type().

-- 
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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Anthony Liguori]

Hollis Blanchard wrote:
>> If anything, I would think it would be better to expand the existing
>> common-ioctl with the notion and then have a 
>> per-architecture hook within that ioctl.
>>     
>
> I *am* expanding the common ioctl. I am also preserving the existing
> ABI: CREATE_VCPU still works, and CREATE_VCPU_TYPE is the new ioctl. And
> then, voila, we have an architecture-specific hook:
> kvm_arch_vcpu_create().
>
> I will happily move the KVM_CREATE_VCPU_TYPE case from
> kvm_arch_vm_ioctl() to kvm_vm_ioctl(), and since the additional
> parameter is necessarily architecture-specific, it will simply call
> kvm_arch_vcpu_create_type().
>   

So the new ioctl() has the extra data and the old ioctl() is just a 
compat interface which calls the new ioctl with a NULL extra data.  I 
think this is the better approach if you're going this route.

However, I still don't think that supporting asymmetric cores is really 
useful at the moment and that introducing a per-vm arch ioctl would be 
the best approach.

Regards,

Anthony Liguori


-------------------------------------------------------------------------
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/

Re: [PATCH 0 of 3] RFC: creating a particular vcpu type

[Avi Kivity]

Anthony Liguori wrote:
>
> So the new ioctl() has the extra data and the old ioctl() is just a 
> compat interface which calls the new ioctl with a NULL extra data.  I 
> think this is the better approach if you're going this route.
>
> However, I still don't think that supporting asymmetric cores is 
> really useful at the moment and that introducing a per-vm arch ioctl 
> would be the best approach.

Note that kvm/x86 supports slightly asymmetric cores, in that the cpuid 
results can be different for different vcpus.  I can't judge how 
important this feature is for ppc, though.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
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/