[PATCH] Enable 32bit dirty log pointers on 64bit host

[PATCH] Enable 32bit dirty log pointers on 64bit host

[Alexander Graf]

From: Arnd Bergmann <arnd@arndb.de>

With big endian userspace, we can't quite figure out if a pointer
is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.

This is what happens with dirty logging. To get the pointer interpreted
correctly, we thus need Arnd's patch to implement a compat layer for
the ioctl:

A better way to do this is to add a separate compat_ioctl() method that
converts this for you.

From: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Alexander Graf <agraf@suse.de>

---

Changes from Arnd's example version:

  - s/log.log/log/ (Avi)
  - use sizeof(compat_log) (Avi)
  - compile fixes
---
 virt/kvm/kvm_main.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cac69c4..54a272f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -43,6 +43,7 @@
 #include <linux/swap.h>
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
+#include <linux/compat.h>
 
 #include <asm/processor.h>
 #include <asm/io.h>
@@ -1542,6 +1543,52 @@ out:
 	return r;
 }
 
+#ifdef CONFIG_COMPAT
+struct compat_kvm_dirty_log {
+	__u32 slot;
+	__u32 padding1;
+	union {
+		compat_uptr_t dirty_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+static long kvm_vm_compat_ioctl(struct file *filp,
+			   unsigned int ioctl, unsigned long arg)
+{
+	struct kvm *kvm = filp->private_data;
+	int r;
+
+	if (kvm->mm != current->mm)
+		return -EIO;
+	switch (ioctl) {
+	case KVM_GET_DIRTY_LOG: {
+		struct compat_kvm_dirty_log compat_log;
+		struct kvm_dirty_log log;
+
+		r = -EFAULT;
+		if (copy_from_user(&compat_log, (void __user *)arg,
+				   sizeof(compat_log)))
+			goto out;
+		log.slot	 = compat_log.slot;
+		log.padding1	 = compat_log.padding1;
+		log.padding2	 = compat_log.padding2;
+		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
+
+		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
+		if (r)
+			goto out;
+		break;
+	}
+	default:
+		r = kvm_vm_ioctl(filp, ioctl, arg);
+	}
+
+out:
+	return r;
+}
+#endif
+
 static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct page *page[1];
@@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma)
 static struct file_operations kvm_vm_fops = {
 	.release        = kvm_vm_release,
 	.unlocked_ioctl = kvm_vm_ioctl,
-	.compat_ioctl   = kvm_vm_ioctl,
+	.compat_ioctl   = kvm_vm_compat_ioctl,
 	.mmap           = kvm_vm_mmap,
 };
 
-- 
1.6.0.2

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Avi Kivity]

On 10/21/2009 04:08 PM, Alexander Graf wrote:
> From: Arnd Bergmann<arnd@arndb.de>
>
> With big endian userspace, we can't quite figure out if a pointer
> is 32 bit (shifted>>  32) or 64 bit when we read a 64 bit pointer.
>
> This is what happens with dirty logging. To get the pointer interpreted
> correctly, we thus need Arnd's patch to implement a compat layer for
> the ioctl:
>
> A better way to do this is to add a separate compat_ioctl() method that
> converts this for you.
>
> From: Arnd Bergmann<arnd@arndb.de>
> Signed-off-by: Arnd Bergmann<arnd@arndb.de>
> Acked-by: Alexander Graf<agraf@suse.de>
>
>    

If you send someone's patch, you need to sign this off.  That says you 
are legally allowed to send it along.

Ack means "I have a say in this area and it looks good to me", you add 
it when someone else is doing the sending.

> Changes from Arnd's example version:
>    

This goes double when changing the patch.

With all the legalese out of the way, the actual code looks good.

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

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Alexander Graf]

On 22.10.2009, at 12:23, Avi Kivity wrote:

> On 10/21/2009 04:08 PM, Alexander Graf wrote:
>> From: Arnd Bergmann<arnd@arndb.de>
>>
>> With big endian userspace, we can't quite figure out if a pointer
>> is 32 bit (shifted>>  32) or 64 bit when we read a 64 bit pointer.
>>
>> This is what happens with dirty logging. To get the pointer  
>> interpreted
>> correctly, we thus need Arnd's patch to implement a compat layer for
>> the ioctl:
>>
>> A better way to do this is to add a separate compat_ioctl() method  
>> that
>> converts this for you.
>>
>> From: Arnd Bergmann<arnd@arndb.de>
>> Signed-off-by: Arnd Bergmann<arnd@arndb.de>
>> Acked-by: Alexander Graf<agraf@suse.de>
>>
>>
>
> If you send someone's patch, you need to sign this off.  That says  
> you are legally allowed to send it along.
>
> Ack means "I have a say in this area and it looks good to me", you  
> add it when someone else is doing the sending.

Ok, so is it still a From: Arnd then? Is it still signed-off by Arnd  
even though I change it?

Is there a Based-on-patch-by: tag? :-)

Alex

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Avi Kivity]

On 10/22/2009 12:25 PM, Alexander Graf wrote:
>> If you send someone's patch, you need to sign this off.  That says 
>> you are legally allowed to send it along.
>>
>> Ack means "I have a say in this area and it looks good to me", you 
>> add it when someone else is doing the sending.
>
>
> Ok, so is it still a From: Arnd then? Is it still signed-off by Arnd 
> even though I change it?

Yes.  Most kvm patches are From: other people, I just add my signoff 
(and definitely don't remote theirs).

Whether to change From: depends on the changes.  If you rewrite the 
patch, it's From: you.  If you just fix a couple of bugs, you generally 
leave From: alone.

> Is there a Based-on-patch-by: tag? :-)

You can always say "Based on initial patch from ...".

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

[PATCH] Enable 32bit dirty log pointers on 64bit host

[Alexander Graf]

From: Arnd Bergmann <arnd@arndb.de>

With big endian userspace, we can't quite figure out if a pointer
is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.

This is what happens with dirty logging. To get the pointer interpreted
correctly, we thus need Arnd's patch to implement a compat layer for
the ioctl:

A better way to do this is to add a separate compat_ioctl() method that
converts this for you.

Based on initial patch from Arnd Bergmann.

From: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

Changes from Arnd's example version:

  - s/log.log/log/ (Avi)
  - use sizeof(compat_log) (Avi)
  - compile fixes
---
 virt/kvm/kvm_main.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 48 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cac69c4..54a272f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -43,6 +43,7 @@
 #include <linux/swap.h>
 #include <linux/bitops.h>
 #include <linux/spinlock.h>
+#include <linux/compat.h>
 
 #include <asm/processor.h>
 #include <asm/io.h>
@@ -1542,6 +1543,52 @@ out:
 	return r;
 }
 
+#ifdef CONFIG_COMPAT
+struct compat_kvm_dirty_log {
+	__u32 slot;
+	__u32 padding1;
+	union {
+		compat_uptr_t dirty_bitmap; /* one bit per page */
+		__u64 padding2;
+	};
+};
+
+static long kvm_vm_compat_ioctl(struct file *filp,
+			   unsigned int ioctl, unsigned long arg)
+{
+	struct kvm *kvm = filp->private_data;
+	int r;
+
+	if (kvm->mm != current->mm)
+		return -EIO;
+	switch (ioctl) {
+	case KVM_GET_DIRTY_LOG: {
+		struct compat_kvm_dirty_log compat_log;
+		struct kvm_dirty_log log;
+
+		r = -EFAULT;
+		if (copy_from_user(&compat_log, (void __user *)arg,
+				   sizeof(compat_log)))
+			goto out;
+		log.slot	 = compat_log.slot;
+		log.padding1	 = compat_log.padding1;
+		log.padding2	 = compat_log.padding2;
+		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
+
+		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
+		if (r)
+			goto out;
+		break;
+	}
+	default:
+		r = kvm_vm_ioctl(filp, ioctl, arg);
+	}
+
+out:
+	return r;
+}
+#endif
+
 static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct page *page[1];
@@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma)
 static struct file_operations kvm_vm_fops = {
 	.release        = kvm_vm_release,
 	.unlocked_ioctl = kvm_vm_ioctl,
-	.compat_ioctl   = kvm_vm_ioctl,
+	.compat_ioctl   = kvm_vm_compat_ioctl,
 	.mmap           = kvm_vm_mmap,
 };
 
-- 
1.6.0.2

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Marcelo Tosatti]

On Wed, Oct 21, 2009 at 04:08:29PM +0200, Alexander Graf wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> With big endian userspace, we can't quite figure out if a pointer
> is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.
> 
> This is what happens with dirty logging. To get the pointer interpreted
> correctly, we thus need Arnd's patch to implement a compat layer for
> the ioctl:
> 
> A better way to do this is to add a separate compat_ioctl() method that
> converts this for you.
> 
> From: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> Changes from Arnd's example version:
> 
>   - s/log.log/log/ (Avi)
>   - use sizeof(compat_log) (Avi)
>   - compile fixes

Applied, thanks.

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Jan Kiszka]

Alexander Graf wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> With big endian userspace, we can't quite figure out if a pointer
> is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.
> 
> This is what happens with dirty logging. To get the pointer interpreted
> correctly, we thus need Arnd's patch to implement a compat layer for
> the ioctl:
> 
> A better way to do this is to add a separate compat_ioctl() method that
> converts this for you.
> 
> From: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> Changes from Arnd's example version:
> 
>   - s/log.log/log/ (Avi)
>   - use sizeof(compat_log) (Avi)
>   - compile fixes
> ---
>  virt/kvm/kvm_main.c |   49 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 48 insertions(+), 1 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index cac69c4..54a272f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -43,6 +43,7 @@
>  #include <linux/swap.h>
>  #include <linux/bitops.h>
>  #include <linux/spinlock.h>
> +#include <linux/compat.h>
>  
>  #include <asm/processor.h>
>  #include <asm/io.h>
> @@ -1542,6 +1543,52 @@ out:
>  	return r;
>  }
>  
> +#ifdef CONFIG_COMPAT
> +struct compat_kvm_dirty_log {
> +	__u32 slot;
> +	__u32 padding1;
> +	union {
> +		compat_uptr_t dirty_bitmap; /* one bit per page */
> +		__u64 padding2;
> +	};
> +};
> +
> +static long kvm_vm_compat_ioctl(struct file *filp,
> +			   unsigned int ioctl, unsigned long arg)
> +{
> +	struct kvm *kvm = filp->private_data;
> +	int r;
> +
> +	if (kvm->mm != current->mm)
> +		return -EIO;
> +	switch (ioctl) {
> +	case KVM_GET_DIRTY_LOG: {
> +		struct compat_kvm_dirty_log compat_log;
> +		struct kvm_dirty_log log;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&compat_log, (void __user *)arg,
> +				   sizeof(compat_log)))
> +			goto out;
> +		log.slot	 = compat_log.slot;
> +		log.padding1	 = compat_log.padding1;
> +		log.padding2	 = compat_log.padding2;
> +		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
> +
> +		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
> +		if (r)
> +			goto out;
> +		break;
> +	}
> +	default:
> +		r = kvm_vm_ioctl(filp, ioctl, arg);
> +	}
> +
> +out:
> +	return r;
> +}
> +#endif
> +
>  static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
>  	struct page *page[1];
> @@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma)
>  static struct file_operations kvm_vm_fops = {
>  	.release        = kvm_vm_release,
>  	.unlocked_ioctl = kvm_vm_ioctl,
> -	.compat_ioctl   = kvm_vm_ioctl,
> +	.compat_ioctl   = kvm_vm_compat_ioctl,

This fails in the absence of CONFIG_COMPAT.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Alexander Graf]

On 23.10.2009, at 10:41, Jan Kiszka wrote:

> Alexander Graf wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> With big endian userspace, we can't quite figure out if a pointer
>> is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.
>>
>> This is what happens with dirty logging. To get the pointer  
>> interpreted
>> correctly, we thus need Arnd's patch to implement a compat layer for
>> the ioctl:
>>
>> A better way to do this is to add a separate compat_ioctl() method  
>> that
>> converts this for you.
>>
>> From: Arnd Bergmann <arnd@arndb.de>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Acked-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> Changes from Arnd's example version:
>>
>>  - s/log.log/log/ (Avi)
>>  - use sizeof(compat_log) (Avi)
>>  - compile fixes
>> ---
>> virt/kvm/kvm_main.c |   49 +++++++++++++++++++++++++++++++++++++++++ 
>> +++++++-
>> 1 files changed, 48 insertions(+), 1 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index cac69c4..54a272f 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -43,6 +43,7 @@
>> #include <linux/swap.h>
>> #include <linux/bitops.h>
>> #include <linux/spinlock.h>
>> +#include <linux/compat.h>
>>
>> #include <asm/processor.h>
>> #include <asm/io.h>
>> @@ -1542,6 +1543,52 @@ out:
>> 	return r;
>> }
>>
>> +#ifdef CONFIG_COMPAT
>> +struct compat_kvm_dirty_log {
>> +	__u32 slot;
>> +	__u32 padding1;
>> +	union {
>> +		compat_uptr_t dirty_bitmap; /* one bit per page */
>> +		__u64 padding2;
>> +	};
>> +};
>> +
>> +static long kvm_vm_compat_ioctl(struct file *filp,
>> +			   unsigned int ioctl, unsigned long arg)
>> +{
>> +	struct kvm *kvm = filp->private_data;
>> +	int r;
>> +
>> +	if (kvm->mm != current->mm)
>> +		return -EIO;
>> +	switch (ioctl) {
>> +	case KVM_GET_DIRTY_LOG: {
>> +		struct compat_kvm_dirty_log compat_log;
>> +		struct kvm_dirty_log log;
>> +
>> +		r = -EFAULT;
>> +		if (copy_from_user(&compat_log, (void __user *)arg,
>> +				   sizeof(compat_log)))
>> +			goto out;
>> +		log.slot	 = compat_log.slot;
>> +		log.padding1	 = compat_log.padding1;
>> +		log.padding2	 = compat_log.padding2;
>> +		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
>> +
>> +		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
>> +		if (r)
>> +			goto out;
>> +		break;
>> +	}
>> +	default:
>> +		r = kvm_vm_ioctl(filp, ioctl, arg);
>> +	}
>> +
>> +out:
>> +	return r;
>> +}
>> +#endif
>> +
>> static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault  
>> *vmf)
>> {
>> 	struct page *page[1];
>> @@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file,  
>> struct vm_area_struct *vma)
>> static struct file_operations kvm_vm_fops = {
>> 	.release        = kvm_vm_release,
>> 	.unlocked_ioctl = kvm_vm_ioctl,
>> -	.compat_ioctl   = kvm_vm_ioctl,
>> +	.compat_ioctl   = kvm_vm_compat_ioctl,
>
> This fails in the absence of CONFIG_COMPAT.


So should it rather be

#ifdef CONFIG_COMPAT
	.compat_ioctl = kvm_vm_compat_ioctl,
#else
	.compat_ioctl = kvm_vm_ioctl,
#endif

or

#ifdef CONFIG_COMPAT
	.compat_ioctl = kvm_vm_compat_ioctl,
#endif

?

Alex

Re: [PATCH] Enable 32bit dirty log pointers on 64bit host

[Jan Kiszka]

Alexander Graf wrote:
> On 23.10.2009, at 10:41, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> With big endian userspace, we can't quite figure out if a pointer
>>> is 32 bit (shifted >> 32) or 64 bit when we read a 64 bit pointer.
>>>
>>> This is what happens with dirty logging. To get the pointer  
>>> interpreted
>>> correctly, we thus need Arnd's patch to implement a compat layer for
>>> the ioctl:
>>>
>>> A better way to do this is to add a separate compat_ioctl() method  
>>> that
>>> converts this for you.
>>>
>>> From: Arnd Bergmann <arnd@arndb.de>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Acked-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> Changes from Arnd's example version:
>>>
>>>  - s/log.log/log/ (Avi)
>>>  - use sizeof(compat_log) (Avi)
>>>  - compile fixes
>>> ---
>>> virt/kvm/kvm_main.c |   49 +++++++++++++++++++++++++++++++++++++++++ 
>>> +++++++-
>>> 1 files changed, 48 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index cac69c4..54a272f 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -43,6 +43,7 @@
>>> #include <linux/swap.h>
>>> #include <linux/bitops.h>
>>> #include <linux/spinlock.h>
>>> +#include <linux/compat.h>
>>>
>>> #include <asm/processor.h>
>>> #include <asm/io.h>
>>> @@ -1542,6 +1543,52 @@ out:
>>> 	return r;
>>> }
>>>
>>> +#ifdef CONFIG_COMPAT
>>> +struct compat_kvm_dirty_log {
>>> +	__u32 slot;
>>> +	__u32 padding1;
>>> +	union {
>>> +		compat_uptr_t dirty_bitmap; /* one bit per page */
>>> +		__u64 padding2;
>>> +	};
>>> +};
>>> +
>>> +static long kvm_vm_compat_ioctl(struct file *filp,
>>> +			   unsigned int ioctl, unsigned long arg)
>>> +{
>>> +	struct kvm *kvm = filp->private_data;
>>> +	int r;
>>> +
>>> +	if (kvm->mm != current->mm)
>>> +		return -EIO;
>>> +	switch (ioctl) {
>>> +	case KVM_GET_DIRTY_LOG: {
>>> +		struct compat_kvm_dirty_log compat_log;
>>> +		struct kvm_dirty_log log;
>>> +
>>> +		r = -EFAULT;
>>> +		if (copy_from_user(&compat_log, (void __user *)arg,
>>> +				   sizeof(compat_log)))
>>> +			goto out;
>>> +		log.slot	 = compat_log.slot;
>>> +		log.padding1	 = compat_log.padding1;
>>> +		log.padding2	 = compat_log.padding2;
>>> +		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
>>> +
>>> +		r = kvm_vm_ioctl_get_dirty_log(kvm, &log);
>>> +		if (r)
>>> +			goto out;
>>> +		break;
>>> +	}
>>> +	default:
>>> +		r = kvm_vm_ioctl(filp, ioctl, arg);
>>> +	}
>>> +
>>> +out:
>>> +	return r;
>>> +}
>>> +#endif
>>> +
>>> static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault  
>>> *vmf)
>>> {
>>> 	struct page *page[1];
>>> @@ -1576,7 +1623,7 @@ static int kvm_vm_mmap(struct file *file,  
>>> struct vm_area_struct *vma)
>>> static struct file_operations kvm_vm_fops = {
>>> 	.release        = kvm_vm_release,
>>> 	.unlocked_ioctl = kvm_vm_ioctl,
>>> -	.compat_ioctl   = kvm_vm_ioctl,
>>> +	.compat_ioctl   = kvm_vm_compat_ioctl,
>> This fails in the absence of CONFIG_COMPAT.
> 
> 
> So should it rather be
> 
> #ifdef CONFIG_COMPAT
> 	.compat_ioctl = kvm_vm_compat_ioctl,
> #else
> 	.compat_ioctl = kvm_vm_ioctl,
> #endif
> 
> or
> 
> #ifdef CONFIG_COMPAT
> 	.compat_ioctl = kvm_vm_compat_ioctl,
> #endif
> 
> ?

I would say the latter as .compat_ioctl should simply be unused in case
of !CONFIG_COMPAT.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux