[patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

Hi,

This patch makes these two macros do something sensible on ia64 and
work the way qemu expects to use them. The old versions clearly couldn't
have worked since the get_regs() call expected pointers in the kvm_regs
struct which were never copied in.

Cheers,
Jes


[-- Attachment #2: 6000-kvm-ia64-get-regs-locking.patch --]
[-- Type: text/plain, Size: 3233 bytes --]

Fix kvm_arch_vcpu_ioctl_[gs]et_regs() to do something meaningful on
ia64. Old versions could never have worked since they required
pointers to be set in the ioctl payload which were never being set by
the ioctl handler for get_regs.

This version doesn't support copying the KVM kernel stack in/out of
the kernel. This should be implemented in a seperate ioctl call if
ever needed.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 arch/ia64/include/asm/kvm.h |    4 ++--
 arch/ia64/kvm/kvm-ia64.c    |   40 ++++++++++------------------------------
 2 files changed, 12 insertions(+), 32 deletions(-)

Index: linux-2.6.git/arch/ia64/include/asm/kvm.h
===================================================================
--- linux-2.6.git.orig/arch/ia64/include/asm/kvm.h
+++ linux-2.6.git/arch/ia64/include/asm/kvm.h
@@ -199,8 +199,6 @@
 };
 
 struct kvm_regs {
-	char *saved_guest;
-	char *saved_stack;
 	struct saved_vpd vpd;
 	/*Arch-regs*/
 	int mp_state;
@@ -233,6 +231,8 @@
 	unsigned long fp_psr;       /*used for lazy float register */
 	unsigned long saved_gp;
 	/*for phycial  emulation */
+
+	union context saved_guest;
 };
 
 struct kvm_sregs {
Index: linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/kvm-ia64.c
+++ linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
@@ -867,9 +867,8 @@
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	int i;
 	struct vpd *vpd = to_host(vcpu->kvm, vcpu->arch.vpd);
-	int r;
+	int i;
 
 	vcpu_load(vcpu);
 
@@ -886,18 +885,7 @@
 
 	vpd->vpr = regs->vpd.vpr;
 
-	r = -EFAULT;
-	r = copy_from_user(&vcpu->arch.guest, regs->saved_guest,
-						sizeof(union context));
-	if (r)
-		goto out;
-	r = copy_from_user(vcpu + 1, regs->saved_stack +
-			sizeof(struct kvm_vcpu),
-			KVM_STK_OFFSET - sizeof(struct kvm_vcpu));
-	if (r)
-		goto out;
-	vcpu->arch.exit_data =
-		((struct kvm_vcpu *)(regs->saved_stack))->arch.exit_data;
+	memcpy(&vcpu->arch.guest, &regs->saved_guest, sizeof(union context));
 
 	RESTORE_REGS(mp_state);
 	RESTORE_REGS(vmm_rr);
@@ -931,9 +919,8 @@
 	set_bit(KVM_REQ_RESUME, &vcpu->requests);
 
 	vcpu_put(vcpu);
-	r = 0;
-out:
-	return r;
+
+	return 0;
 }
 
 long kvm_arch_vm_ioctl(struct file *filp,
@@ -1418,9 +1405,9 @@
 
 int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	int i;
-	int r;
 	struct vpd *vpd = to_host(vcpu->kvm, vcpu->arch.vpd);
+	int i;
+
 	vcpu_load(vcpu);
 
 	for (i = 0; i < 16; i++) {
@@ -1435,14 +1422,8 @@
 	regs->vpd.vpsr = vpd->vpsr;
 	regs->vpd.vpr = vpd->vpr;
 
-	r = -EFAULT;
-	r = copy_to_user(regs->saved_guest, &vcpu->arch.guest,
-					sizeof(union context));
-	if (r)
-		goto out;
-	r = copy_to_user(regs->saved_stack, (void *)vcpu, KVM_STK_OFFSET);
-	if (r)
-		goto out;
+	memcpy(&regs->saved_guest, &vcpu->arch.guest, sizeof(union context));
+
 	SAVE_REGS(mp_state);
 	SAVE_REGS(vmm_rr);
 	memcpy(regs->itrs, vcpu->arch.itrs, sizeof(struct thash_data) * NITRS);
@@ -1470,10 +1451,9 @@
 	SAVE_REGS(metaphysical_saved_rr4);
 	SAVE_REGS(fp_psr);
 	SAVE_REGS(saved_gp);
+
 	vcpu_put(vcpu);
-	r = 0;
-out:
-	return r;
+	return 0;
 }
 
 void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)

RE: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Zhang, Xiantao]

Jes
   Good work!  I ever talked about the issue with Avi and wanted to enable the logic with the following patch, and Avi thought it may lead to security issues, so deferred to now :) 

One comment:  We still need the logic to save and restore the vcpu's stack for vcpu resuming to the guest in the same environment.  Maybe you can allocate one buf to save it in kvm_reg structure.
Xiantao


diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cf0ab8e..9761a7e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1390,6 +1390,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
                kvm_regs = kzalloc(sizeof(struct kvm_regs), GFP_KERNEL);
                if (!kvm_regs)
                        goto out;
+               r = -EFAULT;
+               if (copy_from_user(kvm_regs, argp, sizeof(struct kvm_regs)))
+                       goto out_free1;
                r = kvm_arch_vcpu_ioctl_get_regs(vcpu, kvm_regs);
                if (r)
                        goto out_free1;

Xiantao


-----Original Message-----
From: Jes Sorensen [mailto:jes@sgi.com] 
Sent: Tuesday, December 09, 2008 11:36 PM
To: Avi Kivity; kvm-ia64@vger.kernel.org; Zhang, Xiantao
Subject: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

Hi,

This patch makes these two macros do something sensible on ia64 and
work the way qemu expects to use them. The old versions clearly couldn't
have worked since the get_regs() call expected pointers in the kvm_regs
struct which were never copied in.

Cheers,
Jes

Re: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Jes Sorensen]

Zhang, Xiantao wrote:
> Jes
>    Good work!  I ever talked about the issue with Avi and wanted to enable the logic with the following patch, and Avi thought it may lead to security issues, so deferred to now :) 
> 
> One comment:  We still need the logic to save and restore the vcpu's stack for vcpu resuming to the guest in the same environment.  Maybe you can allocate one buf to save it in kvm_reg structure.
> Xiantao

Hi Xiantao,

I am with Avi on this one - we shouldn't be passing in pointers like
that. Either we can increase the size of kvm_regs as you mention, but it
will become *huge* since the stack is 64KB, or we introduce a new ioctl
just to handle the stack.

Do you think it would be a problem having the second ioctl for this? I
would prefer breaking it into two to avoid the kernel having to allocate
a 128KB chunk for kvm_regs.

Cheers,
Jes

RE: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Zhang, Xiantao]

Jes Sorensen wrote:
> Zhang, Xiantao wrote:
>> Jes
>>    Good work!  I ever talked about the issue with Avi and wanted to
>> enable the logic with the following patch, and Avi thought it may
>> lead to security issues, so deferred to now :)  
>> 
>> One comment:  We still need the logic to save and restore the vcpu's
>> stack for vcpu resuming to the guest in the same environment.  Maybe
>> you can allocate one buf to save it in kvm_reg structure. Xiantao 
> 
> Hi Xiantao,
> 
> I am with Avi on this one - we shouldn't be passing in pointers like
> that. Either we can increase the size of kvm_regs as you mention, but
> it will become *huge* since the stack is 64KB, or we introduce a new
> ioctl just to handle the stack.
> 
> Do you think it would be a problem having the second ioctl for this? I
> would prefer breaking it into two to avoid the kernel having to
> allocate a 128KB chunk for kvm_regs.

Fine to me to add the ioctl, and it should make the logic clear. 
Xiantao

Re: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Jes Sorensen]

Zhang, Xiantao wrote:
> Jes Sorensen wrote:
>> I am with Avi on this one - we shouldn't be passing in pointers like
>> that. Either we can increase the size of kvm_regs as you mention, but
>> it will become *huge* since the stack is 64KB, or we introduce a new
>> ioctl just to handle the stack.
>>
>> Do you think it would be a problem having the second ioctl for this? I
>> would prefer breaking it into two to avoid the kernel having to
>> allocate a 128KB chunk for kvm_regs.
> 
> Fine to me to add the ioctl, and it should make the logic clear. 
> Xiantao

Cool, I'll look into that later today, I hope. I'll base it on your old
code, but I will not be able to test it :-)

Cheers,
Jes

RE: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Zhang, Xiantao]

Jes Sorensen wrote:
> Zhang, Xiantao wrote:
>> Jes Sorensen wrote:
>>> I am with Avi on this one - we shouldn't be passing in pointers like
>>> that. Either we can increase the size of kvm_regs as you mention,
>>> but it will become *huge* since the stack is 64KB, or we introduce
>>> a new ioctl just to handle the stack.
>>> 
>>> Do you think it would be a problem having the second ioctl for
>>> this? I would prefer breaking it into two to avoid the kernel
>>> having to allocate a 128KB chunk for kvm_regs.
>> 
>> Fine to me to add the ioctl, and it should make the logic clear.
>> Xiantao
> 
> Cool, I'll look into that later today, I hope. I'll base it on your
> old code, but I will not be able to test it :-)

I just have the code of userspace for enabling live migration, and it should be a bit old.  Maybe you can add the ioctl from the scratch to support the stack's save&restore, and If you need the userspace code, I will post it to u. :)
Xiantao

Re: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Avi Kivity]

Jes Sorensen wrote:
> Hi,
>
> This patch makes these two macros do something sensible on ia64 and
> work the way qemu expects to use them. The old versions clearly couldn't
> have worked since the get_regs() call expected pointers in the kvm_regs
> struct which were never copied in.
>
> Fix kvm_arch_vcpu_ioctl_[gs]et_regs() to do something meaningful on
> ia64. Old versions could never have worked since they required
> pointers to be set in the ioctl payload which were never being set by
> the ioctl handler for get_regs.
>
> This version doesn't support copying the KVM kernel stack in/out of
> the kernel. This should be implemented in a seperate ioctl call if
> ever needed.
>
>  
>  struct kvm_regs {
> -	char *saved_guest;
> -	char *saved_stack;
>  	struct saved_vpd vpd;
>  	/*Arch-regs*/
>  	int mp_state;
> @@ -233,6 +231,8 @@
>  	unsigned long fp_psr;       /*used for lazy float register */
>  	unsigned long saved_gp;
>  	/*for phycial  emulation */
> +
> +	union context saved_guest;
>  };
>  
>   

This makes several tons of sense, but breaks backwards compatibility.  
If I understand correctly, get/set was never used so this shouldn't matter?

I suggest reserving some space at the end of kvm_regs in case further 
state needs to be added.

Please add a KVM_CAP_ entry to advertise the fixup.  This way userspace 
can determine that it's compiling or running on an old kernel and error 
out gracefully.

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

RE: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Zhang, Xiantao]

Avi Kivity wrote:
> Jes Sorensen wrote:
>> Hi,
>> 
>> This patch makes these two macros do something sensible on ia64 and
>> work the way qemu expects to use them. The old versions clearly
>> couldn't have worked since the get_regs() call expected pointers in
>> the kvm_regs struct which were never copied in.
>> 
>> Fix kvm_arch_vcpu_ioctl_[gs]et_regs() to do something meaningful on
>> ia64. Old versions could never have worked since they required
>> pointers to be set in the ioctl payload which were never being set by
>> the ioctl handler for get_regs.
>> 
>> This version doesn't support copying the KVM kernel stack in/out of
>> the kernel. This should be implemented in a seperate ioctl call if
>> ever needed.
>> 
>> 
>>  struct kvm_regs {
>> -	char *saved_guest;
>> -	char *saved_stack;
>>  	struct saved_vpd vpd;
>>  	/*Arch-regs*/
>>  	int mp_state;
>> @@ -233,6 +231,8 @@
>>  	unsigned long fp_psr;       /*used for lazy float register */ 
>>  	unsigned long saved_gp; /*for phycial  emulation */
>> +
>> +	union context saved_guest;
>>  };
>> 
>> 
> 
> This makes several tons of sense, but breaks backwards compatibility.
> If I understand correctly, get/set was never used so this shouldn't
> matter? 

It doesn't matter, we haven't enabled  userspace code for save/restore and live migration before, so shouldn't result in the issue.   

> I suggest reserving some space at the end of kvm_regs in case further
> state needs to be added.
> Please add a KVM_CAP_ entry to advertise the fixup.  This way
> userspace can determine that it's compiling or running on an old
> kernel and error out gracefully.

Agree. 

Xiantao

Re: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Jes Sorensen]

Avi Kivity wrote:
> This makes several tons of sense, but breaks backwards compatibility.  
> If I understand correctly, get/set was never used so this shouldn't matter?
> 
> I suggest reserving some space at the end of kvm_regs in case further 
> state needs to be added.

The code never worked, so it's not an issue to break backwards compat, 
but I agree. I'll add some space for future extensions.

Cheers,
Jes

Re: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Jes Sorensen]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

Zhang, Xiantao wrote:
> Jes
>    Good work!  I ever talked about the issue with Avi and wanted to enable the logic with the following patch, and Avi thought it may lead to security issues, so deferred to now :) 
> 
> One comment:  We still need the logic to save and restore the vcpu's stack for vcpu resuming to the guest in the same environment.  Maybe you can allocate one buf to save it in kvm_reg structure.
> Xiantao

Xiantao,

I have come up with a patch for this. Before we push it, would you mind
taking a look at it and let me know if it looks right? I don't have any
way of really testing this.

Cheers,
Jes


[-- Attachment #2: 6001-kvm-ia64-restore-stack-ioctl.patch --]
[-- Type: text/plain, Size: 4836 bytes --]

---
 arch/ia64/include/asm/kvm.h      |    8 +++
 arch/ia64/include/asm/kvm_host.h |    2 
 arch/ia64/kvm/kvm-ia64.c         |   92 +++++++++++++++++++++++++++++++++++++--
 include/linux/kvm.h              |    3 +
 4 files changed, 101 insertions(+), 4 deletions(-)

Index: linux-2.6.git/arch/ia64/include/asm/kvm.h
===================================================================
--- linux-2.6.git.orig/arch/ia64/include/asm/kvm.h
+++ linux-2.6.git/arch/ia64/include/asm/kvm.h
@@ -242,4 +242,12 @@
 
 struct kvm_fpu {
 };
+
+#define KVM_IA64_VCPU_STACK_SHIFT	16
+#define KVM_IA64_VCPU_STACK_SIZE	(1UL << KVM_IA64_VCPU_STACK_SHIFT)
+
+struct kvm_ia64_vcpu_stack {
+	unsigned char stack[KVM_IA64_VCPU_STACK_SIZE];
+};
+
 #endif
Index: linux-2.6.git/arch/ia64/include/asm/kvm_host.h
===================================================================
--- linux-2.6.git.orig/arch/ia64/include/asm/kvm_host.h
+++ linux-2.6.git/arch/ia64/include/asm/kvm_host.h
@@ -112,7 +112,7 @@
 #define VCPU_STRUCT_SHIFT	16
 #define VCPU_STRUCT_SIZE	(__IA64_UL_CONST(1)<< VCPU_STRUCT_SHIFT)
 
-#define KVM_STK_OFFSET		VCPU_STRUCT_SIZE
+#define KVM_STK_OFFSET		KVM_IA64_VCPU_STACK_SIZE
 
 #define KVM_VM_STRUCT_SHIFT	19
 #define KVM_VM_STRUCT_SIZE	(__IA64_UL_CONST(1) << KVM_VM_STRUCT_SHIFT)
Index: linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
===================================================================
--- linux-2.6.git.orig/arch/ia64/kvm/kvm-ia64.c
+++ linux-2.6.git/arch/ia64/kvm/kvm-ia64.c
@@ -1456,6 +1456,23 @@
 	return 0;
 }
 
+int kvm_arch_vcpu_ioctl_get_stack(struct kvm_vcpu *vcpu,
+				  struct kvm_ia64_vcpu_stack *stack)
+{
+	memcpy(stack, vcpu, KVM_IA64_VCPU_STACK_SIZE);
+	return 0;
+}
+
+int kvm_arch_vcpu_ioctl_set_stack(struct kvm_vcpu *vcpu,
+				  struct kvm_ia64_vcpu_stack *stack)
+{
+	memcpy(vcpu + 1, &stack->stack[0] + sizeof(struct kvm_vcpu),
+	       KVM_IA64_VCPU_STACK_SIZE - sizeof(struct kvm_vcpu));
+
+	vcpu->arch.exit_data = ((struct kvm_vcpu *)stack)->arch.exit_data;
+	return 0;
+}
+
 void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
 
@@ -1465,9 +1482,78 @@
 
 
 long kvm_arch_vcpu_ioctl(struct file *filp,
-		unsigned int ioctl, unsigned long arg)
+			 unsigned int ioctl, unsigned long arg)
 {
-	return -EINVAL;
+	struct kvm_vcpu *vcpu = filp->private_data;
+	void __user *argp = (void __user *)arg;
+	struct kvm_ia64_vcpu_stack *stack = NULL;
+	long r;
+
+	switch (ioctl) {
+	case KVM_IA64_VCPU_GET_STACK: {
+		struct kvm_ia64_vcpu_stack __user *user_stack;
+	        void __user *first_p = argp;
+
+		r = -EFAULT;
+		if (copy_from_user(&user_stack, first_p, sizeof(void *)))
+			goto out;
+
+		if (!access_ok(VERIFY_WRITE, user_stack,
+			       sizeof(struct kvm_ia64_vcpu_stack))) {
+			printk(KERN_INFO "KVM_IA64_VCPU_GET_STACK: "
+			       "Illegal user destination address for stack\n");
+			goto out;
+		}
+		stack = kzalloc(sizeof(struct kvm_ia64_vcpu_stack), GFP_KERNEL);
+		if (!stack) {
+			r = -ENOMEM;
+			goto out;
+		}
+
+		r = kvm_arch_vcpu_ioctl_get_stack(vcpu, stack);
+		if (r)
+			goto out;
+
+		if (copy_to_user(user_stack, stack,
+				 sizeof(struct kvm_ia64_vcpu_stack)))
+			goto out;
+
+		break;
+	}
+	case KVM_IA64_VCPU_SET_STACK: {
+		struct kvm_ia64_vcpu_stack __user *user_stack;
+	        void __user *first_p = argp;
+
+		r = -EFAULT;
+		if (copy_from_user(&user_stack, first_p, sizeof(void *)))
+			goto out;
+
+		if (!access_ok(VERIFY_READ, user_stack,
+			    sizeof(struct kvm_ia64_vcpu_stack))) {
+			printk(KERN_INFO "KVM_IA64_VCPU_SET_STACK: "
+			       "Illegal user address for stack\n");
+			goto out;
+		}
+		stack = kmalloc(sizeof(struct kvm_ia64_vcpu_stack), GFP_KERNEL);
+		if (!stack) {
+			r = -ENOMEM;
+			goto out;
+		}
+		if (copy_from_user(stack, user_stack,
+				   sizeof(struct kvm_ia64_vcpu_stack)))
+			goto out;
+
+		r = kvm_arch_vcpu_ioctl_set_stack(vcpu, stack);
+		break;
+	}
+
+	default:
+		r = -EINVAL;
+	}
+
+out:
+	kfree(stack);
+	return r;
 }
 
 int kvm_arch_set_memory_region(struct kvm *kvm,
@@ -1502,7 +1588,7 @@
 }
 
 long kvm_arch_dev_ioctl(struct file *filp,
-		unsigned int ioctl, unsigned long arg)
+			unsigned int ioctl, unsigned long arg)
 {
 	return -EINVAL;
 }
Index: linux-2.6.git/include/linux/kvm.h
===================================================================
--- linux-2.6.git.orig/include/linux/kvm.h
+++ linux-2.6.git/include/linux/kvm.h
@@ -459,6 +459,9 @@
 #define KVM_GET_MP_STATE          _IOR(KVMIO,  0x98, struct kvm_mp_state)
 #define KVM_SET_MP_STATE          _IOW(KVMIO,  0x99, struct kvm_mp_state)
 
+#define KVM_IA64_VCPU_GET_STACK   _IOR(KVMIO,  0x9a, void *)
+#define KVM_IA64_VCPU_SET_STACK   _IOW(KVMIO,  0x9b, void *)
+
 #define KVM_TRC_INJ_VIRQ         (KVM_TRC_HANDLER + 0x02)
 #define KVM_TRC_REDELIVER_EVT    (KVM_TRC_HANDLER + 0x03)
 #define KVM_TRC_PEND_INTR        (KVM_TRC_HANDLER + 0x04)

RE: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

[Zhang, Xiantao]

Jes,
     The patch is fine for me. Please push this patch and the privious one together to Avi, and Avi will help to push them into 2.6.29.  Thanks a lot!
Xiantao


Jes Sorensen wrote:
> Zhang, Xiantao wrote:
>> Jes
>>    Good work!  I ever talked about the issue with Avi and wanted to
>> enable the logic with the following patch, and Avi thought it may
>> lead to security issues, so deferred to now :)  
>> 
>> One comment:  We still need the logic to save and restore the vcpu's
>> stack for vcpu resuming to the guest in the same environment.  Maybe
>> you can allocate one buf to save it in kvm_reg structure. Xiantao 
> 
> Xiantao,
> 
> I have come up with a patch for this. Before we push it, would you
> mind taking a look at it and let me know if it looks right? I don't
> have any way of really testing this.
> 
> Cheers,
> Jes