| Message ID | 1249280247-6732-1-git-send-email-sheng@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sheng Yang wrote: > 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). > > Also fix a mistake of still using old macro rather than new variable(). Two patches then? > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > --- > arch/x86/kvm/vmx.c | 2 +- > arch/x86/kvm/x86.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 7931c72..c5aaa1b 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2539e9a..977b705 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -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
On 08/03/2009 09:17 AM, Sheng Yang wrote: > 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<sheng@linux.intel.com> > --- > arch/x86/kvm/vmx.c | 2 +- > arch/x86/kvm/x86.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 7931c72..c5aaa1b 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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. > out: > up_write(&kvm->slots_lock); > return r; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2539e9a..977b705 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -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.
On Monday 03 August 2009 20:25:43 Avi Kivity wrote: > On 08/03/2009 09:17 AM, Sheng Yang wrote: > > 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<sheng@linux.intel.com> > > --- > > arch/x86/kvm/vmx.c | 2 +- > > arch/x86/kvm/x86.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 7931c72..c5aaa1b 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -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. Sure. (and thanks Jan's comment :) ) > > > out: > > up_write(&kvm->slots_lock); > > return r; > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 2539e9a..977b705 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -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. Yeah... Then I think I should fix the userspace. Would update the patch.
On Mon, Aug 03, 2009 at 02:17:27PM +0800, Sheng Yang wrote: > Copy u64 from guest result in chaos. Why? > > Also fix a mistake of still using old macro rather than new variable(). > > Signed-off-by: Sheng Yang <sheng@linux.intel.com> > --- > arch/x86/kvm/vmx.c | 2 +- > arch/x86/kvm/x86.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 7931c72..c5aaa1b 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -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; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2539e9a..977b705 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 04 August 2009 00:47:16 Marcelo Tosatti wrote: > On Mon, Aug 03, 2009 at 02:17:27PM +0800, Sheng Yang wrote: > > Copy u64 from guest result in chaos. > > Why? My fault in userspace...
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 7931c72..c5aaa1b 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -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; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2539e9a..977b705 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -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))
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 <sheng@linux.intel.com> --- arch/x86/kvm/vmx.c | 2 +- arch/x86/kvm/x86.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)