Skip to contentSkip to conversation

KVM: Fix EPT identity IOCTL on 32pae

PATCH In review Sheng Yang 4 participants 6 messages Aug 3, 2009

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Sheng Yang

author
#0 lore raw

PATCH 1/1 KVM: Fix EPT identity IOCTL on 32pae

Copy u64 from guest result in chaos.

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/x86.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -1
index 7931c72..c5aaa1b 100644
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 		goto out;
 
 	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
 out:
 	up_write(&kvm->slots_lock);
 	return r;
arch/x86/kvm/x86.c +1 -1
index 2539e9a..977b705 100644
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		break;
 	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
 		r = -EFAULT;
 		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
Signature · 1 line
1.5.4.5
#1 lore raw
Quoted 1 line · Sheng Yang

Copy u64 from guest result in chaos.

Not doing so, while the ABI as been defined as u64, will surely result in chaos as well - just different one. I'm recalling the problems Alex recently ran into on big-endian PowerPC and sizeof(userland_long) != sizeof(kernel_long).

Quoted 2 lines

Also fix a mistake of still using old macro rather than new variable().

Two patches then?

Quoted 25 lines
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/x86.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -1
index 7931c72..c5aaa1b 100644
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 		goto out;
 
 	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
 out:
 	up_write(&kvm->slots_lock);
 	return r;
arch/x86/kvm/x86.c +0 -0
index 2539e9a..977b705 100644

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 2184

Quoted 8 lines
arch/x86/kvm/x86.c +1 -1
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		break;
 	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
 		r = -EFAULT;
 		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))

I don't know what as to be fixed here, but you must stick to the ABI. Maybe you just want to convert u64 -> ulong afterwards?

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
#2 lore raw
In reply to Sheng Yang
Quoted 15 lines · Sheng Yang

Copy u64 from guest result in chaos.

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by Sheng Yang<[email protected]>
---
  arch/x86/kvm/vmx.c |    2 +-
  arch/x86/kvm/x86.c |    2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +0 -0
index 7931c72..c5aaa1b 100644

arch/x86/kvm/vmx.c static int alloc_identity_pagetable(struct kvm *kvm) · line 2203

Quoted 6 lines
arch/x86/kvm/vmx.c +1 -1
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
  		goto out;
 
  	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR>>  PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr>>  PAGE_SHIFT);

This is a fix, but as Jan says, separate patch.

Quoted 9 lines
arch/x86/kvm/vmx.c +0 -0
  out:
  	up_write(&kvm->slots_lock);

return r;

arch/x86/kvm/x86.c +0 -0
index 2539e9a..977b705 100644

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 2184

Quoted 8 lines
arch/x86/kvm/x86.c +1 -1
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
  			goto out;
  		break;
  	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
  		r = -EFAULT;
  		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))

This doesn't look right. Consider 32-bit userspace running on a 32-bit kernel and the same userspace running on a 64-bit kernel. We need to copy the same size in both cases.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw
Quoted 18 lines · On Monday 03 August 2009 20:25:43 Avi Kivity wrote:
Quoted 17 lines · Sheng Yang

Copy u64 from guest result in chaos.

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by Sheng Yang<[email protected]>
---
  arch/x86/kvm/vmx.c |    2 +-
  arch/x86/kvm/x86.c |    2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +0 -0
index 7931c72..c5aaa1b 100644
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm
 *kvm) goto out;
Quoted 5 lines

arch/x86/kvm/vmx.c static int alloc_identity_pagetable(struct kvm · line 2202

Quoted 3 lines
arch/x86/kvm/vmx.c +1 -1
  	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR>>  PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr>>  PAGE_SHIFT);

This is a fix, but as Jan says, separate patch.

Sure. (and thanks Jan's comment :) )

Quoted 15 lines
Quoted 15 lines
arch/x86/kvm/vmx.c +0 -0
  out:
  	up_write(&kvm->slots_lock);
  	return r;
arch/x86/kvm/x86.c +1 -1
index 2539e9a..977b705 100644
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
  			goto out;
  		break;
  	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
Quoted 6 lines

arch/x86/kvm/x86.c long kvm_arch_vm_ioctl(struct file *filp, · line 2184

Quoted 2 lines
arch/x86/kvm/x86.c +0 -0
  		r = -EFAULT;
  		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))

This doesn't look right. Consider 32-bit userspace running on a 32-bit kernel and the same userspace running on a 64-bit kernel. We need to copy the same size in both cases.

Yeah... Then I think I should fix the userspace. Would update the patch.

Signature · 2 lines
regards
Yang, Sheng
#4 lore raw
In reply to Sheng Yang
Quoted 1 line · Sheng Yang

Copy u64 from guest result in chaos.

Why?

Quoted 37 lines

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/x86.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -1
index 7931c72..c5aaa1b 100644
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 		goto out;
 
 	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
 out:
 	up_write(&kvm->slots_lock);
 	return r;
arch/x86/kvm/x86.c +1 -1
index 2539e9a..977b705 100644
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		break;
 	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
 		r = -EFAULT;
 		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
Signature · 1 line
1.5.4.5
author
#5 lore raw
Quoted 4 lines · On Tuesday 04 August 2009 00:47:16 Marcelo Tosatti wrote:
Quoted 1 line · Sheng Yang

Copy u64 from guest result in chaos.

Why?

My fault in userspace...

Signature · 2 lines
regards
Yang, Sheng
Quoted 37 lines
Quoted 37 lines

Also fix a mistake of still using old macro rather than new variable().

Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/kvm/vmx.c |    2 +-
 arch/x86/kvm/x86.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/vmx.c +1 -1
index 7931c72..c5aaa1b 100644
@@ -2199,7 +2199,7 @@ static int alloc_identity_pagetable(struct kvm
 *kvm) goto out;
 
 	kvm->arch.ept_identity_pagetable = gfn_to_page(kvm,
-			VMX_EPT_IDENTITY_PAGETABLE_ADDR >> PAGE_SHIFT);
+			kvm->arch.ept_identity_map_addr >> PAGE_SHIFT);
 out:
 	up_write(&kvm->slots_lock);
 	return r;
arch/x86/kvm/x86.c +1 -2
index 2539e9a..977b705 100644
@@ -2178,7 +2178,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		break;
 	case KVM_SET_IDENTITY_MAP_ADDR: {
-		u64 ident_addr;
+		unsigned long ident_addr;
 
 		r = -EFAULT;
 		if (copy_from_user(&ident_addr, argp, sizeof ident_addr))
--

1.5.4.5