RFC: creating a particular vcpu type

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/

# HG changeset patch
# User Hollis Blanchard <hollisb-r/[email protected]> # Date 1202189664 21600
# Node ID bede9476e203f5bf59d21cc3cd71a30de2ce2c44
# Parent dfb0e1d58b57dfdf76b3111565815599bd38b92d

---

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

@@ -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;

@@ -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);
 }

@@ -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);
 

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

# HG changeset patch
# User Hollis Blanchard <hollisb-r/[email protected]> # Date 1202189666 21600
# Node ID 7dd50dab9096c8e0125792e3f48083c3f47fceab
# Parent bede9476e203f5bf59d21cc3cd71a30de2ce2c44

---

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

@@ -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);

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

# HG changeset patch
# User Hollis Blanchard <hollisb-r/[email protected]> # 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().

---

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

@@ -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;
 	}

@@ -49,6 +49,11 @@ struct tlbe {
 };
 
 struct kvm_arch {
+};
+
+struct kvmppc_core_spec {
+	const char *name;
+	unsigned int vcpu_size;
 };
 
 struct kvm_vcpu_arch {

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

[..]

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/

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/

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/

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.

I wasn't liking that part of the interface anyways, so strnlen() sounds good to me. :)

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.

After talking to Carsten, it sounds like S390 could use this approach as well.

Agreed, I think we can handle that with my thoughts above.

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/

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

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

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

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

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.

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/

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

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/

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/

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/