Skip to contentSkip to conversation

fix kvm_arch_vcpu_ioctl_[gs]et_regs() - v2

PATCH Applied Jes Sorensen 3 participants 10 messages Dec 16, 2008 – Dec 18, 2008

lore
Share & tools
lore

Jes Sorensen

author
#0 lore raw

PATCH 1/1 fix kvm_arch_vcpu_ioctl_[gs]et_regs() - v2

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.

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.

Hi Avi,

Here's an update version of this patch, which leaves some space for future extensions in struct kvm_regs. The KVM_CAP is unnecessary as GET/SET_REGS never worked on ia64.

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.

In addition reserve extra space for future extensions.

The change of layout of struct kvm_regs doesn't require adding a new CAP since get/set regs never worked on ia64 until now.

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 |    6 ++++--
 arch/ia64/kvm/kvm-ia64.c    |   40 ++++++++++------------------------------
 2 files changed, 14 insertions(+), 32 deletions(-)
arch/ia64/include/asm/kvm.h +4 -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,10 @@
 	unsigned long fp_psr;       /*used for lazy float register */
 	unsigned long saved_gp;
 	/*for phycial  emulation */
+
+	unsigned long reserved[64];	/* for future use */
+
+	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)
Acked-by on v1 1/1
#1 lore raw
Quoted 12 lines · Jes Sorensen
Quoted 10 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.

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.

Quoted 5 lines

Hi Avi,

Here's an update version of this patch, which leaves some space for future extensions in struct kvm_regs. The KVM_CAP is unnecessary as GET/SET_REGS never worked on ia64.

Avi,
Please help to queue it for 2.6.28 merge. We will push changes in userland for live migration , and 2.6.29 should be ready for live migration.
Xiantao

Acked-by Xiantao Zhang <[email protected]>
Acked-by on v1 1/1
#2 lore raw
In reply to Jes Sorensen
Quoted 12 lines · Jes Sorensen
Quoted 10 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.

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.

Quoted 5 lines

Hi Avi,

Here's an update version of this patch, which leaves some space for future extensions in struct kvm_regs. The KVM_CAP is unnecessary as GET/SET_REGS never worked on ia64.

Thanks, Jes!
Xiantao

Acked-by Xiantao Zhang <[email protected]>
Applied
#3 lore raw
In reply to Jes Sorensen
Quoted 5 lines · Jes Sorensen

Here's an update version of this patch, which leaves some space for future extensions in struct kvm_regs. The KVM_CAP is unnecessary as GET/SET_REGS never worked on ia64.

Applied, thanks. I moved the reserved bits to the end to be more traditional.

Signature · 1 line
error compiling committee.c: too many arguments to function
#4 lore raw
In reply to Jes Sorensen
Quoted 4 lines · Xiantao

Please help to queue it for 2.6.28 merge. We will push changes in userland for live migration , and 2.6.29 should be ready for live migration.
Xiantao

Since it doesn't fix a regression, I'll queue it for 2.6.29.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#5 lore raw
In reply to Jes Sorensen
Quoted 9 lines · Avi Kivity
Quoted 5 lines · Jes Sorensen

Here's an update version of this patch, which leaves some space for future extensions in struct kvm_regs. The KVM_CAP is unnecessary as GET/SET_REGS never worked on ia64.

Applied, thanks. I moved the reserved bits to the end to be more traditional.

Hi Avi,

Please don't do that, then they'll end up after a 16KB struct, I put them where they were deliberately.

Cheers,
Jes

#6 lore raw
In reply to Jes Sorensen
Quoted 4 lines · Jes Sorensen

Please don't do that, then they'll end up after a 16KB struct, I put them where they were deliberately.

Why does it matter? I don't see any performance issue, we copy everything anyway, and this is not a high usage interface.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#7 lore raw
In reply to Jes Sorensen
Quoted 6 lines · Avi Kivity
Quoted 3 lines · Jes Sorensen

Please don't do that, then they'll end up after a 16KB struct, I put them where they were deliberately.

Why does it matter? I don't see any performance issue, we copy everything anyway, and this is not a high usage interface.

Well it just makes the generated code uglier.

Jes

#8 lore raw
In reply to Jes Sorensen
Quoted 10 lines · Jes Sorensen
Quoted 6 lines · Avi Kivity
Quoted 3 lines · Jes Sorensen

Please don't do that, then they'll end up after a 16KB struct, I put them where they were deliberately.

Why does it matter? I don't see any performance issue, we copy everything anyway, and this is not a high usage interface.

Well it just makes the generated code uglier.

Who cares?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#9 lore raw
In reply to Jes Sorensen
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Jes Sorensen
Quoted 4 lines
Quoted 2 lines

Please don't do that, then they'll end up after a 16KB struct, I put them where they were deliberately.

Why does it matter? I don't see any performance issue, we copy everything anyway, and this is not a high usage interface.

Well it just makes the generated code uglier.

Who cares?

Me :-)

Anyway, ignore it, just leave it as it is.

Cheers,
Jes