fix kvm_arch_vcpu_ioctl_[gs]et_regs()

PATCH In review Jes Sorensen 3 participants 11 messages Dec 9, 2008 – Dec 13, 2008 KVM

lore

Share & tools

lore

Jes Sorensen

author

2008-12-09 15:36 UTC #0 lore raw

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

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 <[email protected]>

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

arch/ia64/include/asm/kvm.h +2 -2

Index: 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 {

arch/ia64/kvm/kvm-ia64.c +10 ~~-30~~

Index: 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)

Zhang, Xiantao

2008-12-10 02:39 UTC #1 lore raw

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

virt/kvm/kvm_main.c +3 -0

index cf0ab8e..9761a7e 100644
@@ -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:[email protected]]
Sent: Tuesday, December 09, 2008 11:36 PM
To: Avi Kivity; [email protected]; Zhang, Xiantao
Subject: [patch] fix kvm_arch_vcpu_ioctl_[gs]et_regs()

Hi,

Cheers,
Jes

Jes Sorensen

author

2008-12-10 08:28 UTC #2 lore raw

In reply to Jes Sorensen

Quoted 5 lines · 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

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

Zhang, Xiantao

2008-12-10 08:36 UTC #3 lore raw

In reply to Jes Sorensen

Quoted 12 lines · Jes Sorensen

Quoted 8 lines · 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
Hi Xiantao,

Quoted 8 lines

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

Jes Sorensen

author

2008-12-10 08:39 UTC #4 lore raw

In reply to Jes Sorensen

Quoted 6 lines · Xiantao

Quoted 5 lines · Jes Sorensen
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.

Quoted 6 lines

Quoted 3 lines
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

Zhang, Xiantao

2008-12-10 08:44 UTC #5 lore raw

In reply to Jes Sorensen

Quoted 11 lines · Jes Sorensen

Quoted 10 lines · Xiantao
Quoted 8 lines · Jes Sorensen
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.

Quoted 5 lines

Quoted 2 lines
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

Avi Kivity

2008-12-10 09:00 UTC #6 lore raw

In reply to Jes Sorensen

Quoted 24 lines · Jes Sorensen

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 {
arch/ia64/include/asm/kvm.h +0 -2
-	char *saved_guest;
-	char *saved_stack;
 	struct saved_vpd vpd;
 	/*Arch-regs*/
 	int mp_state;

arch/ia64/include/asm/kvm.h line 238

Quoted 8 lines

arch/ia64/include/asm/kvm.h +2 -0

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

Signature · 1 line

error compiling committee.c: too many arguments to function

Zhang, Xiantao

2008-12-10 09:20 UTC #7 lore raw

In reply to Jes Sorensen

Quoted 28 lines · Avi Kivity

Quoted 27 lines · Jes Sorensen
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 {
arch/ia64/include/asm/kvm.h +1 -2
-	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 */
+

Quoted 8 lines

arch/ia64/include/asm/kvm.h
Quoted 4 lines
arch/ia64/include/asm/kvm.h +1 -0
+	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.

Quoted 5 lines

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

Jes Sorensen

author

2008-12-10 09:55 UTC #8 lore raw

In reply to Jes Sorensen

Quoted 5 lines · Avi Kivity

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

Jes Sorensen

author

2008-12-12 14:44 UTC #9 lore raw

In reply to Jes Sorensen

Quoted 5 lines · 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

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

---
 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(-)

arch/ia64/include/asm/kvm.h +8 -0

Index: 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

arch/ia64/include/asm/kvm_host.h +1 -1

Index: 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)

arch/ia64/kvm/kvm-ia64.c +89 -3

111 lines not on this page · raw message

include/linux/kvm.h +3 -0

Index: 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)

Zhang, Xiantao

2008-12-13 15:54 UTC #10 lore raw

In reply to Jes Sorensen

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

Quoted 18 lines · Jes Sorensen

Quoted 8 lines · 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
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