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

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

[Alexander Graf]

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, I just make it bounce twice, but admittedly that is not ideal.

I'm open for suggestions here.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 virt/kvm/kvm_main.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e27b7a9..91c0225 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -720,6 +720,11 @@ int kvm_get_dirty_log(struct kvm *kvm,
 
 	r = -EFAULT;
 	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
+#ifdef __BIG_ENDIAN
+		/* Did we get a 32 bit pointer? */
+		if (copy_to_user((void*)((u64)log->dirty_bitmap >> 32),
+				 memslot->dirty_bitmap, n))
+#endif
 		goto out;
 
 	if (any)
-- 
1.6.0.2

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

[Avi Kivity]

On 09/29/2009 10:18 AM, Alexander Graf wrote:
> 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, I just make it bounce twice, but admittedly that is not ideal.
>
> I'm open for suggestions here.
>
>    

How about adding a new union member to struct kvm_dirty_log:

   __u64 dirty_bitmap_virt;

and deprecate dirty_bitmap.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e27b7a9..91c0225 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -720,6 +720,11 @@ int kvm_get_dirty_log(struct kvm *kvm,
>
>   	r = -EFAULT;
>   	if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> +#ifdef __BIG_ENDIAN
> +		/* Did we get a 32 bit pointer? */
> +		if (copy_to_user((void*)((u64)log->dirty_bitmap>>  32),
> +				 memslot->dirty_bitmap, n))
> +#endif
>   		goto out;
>
>   	if (any)
>    


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

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

[Alexander Graf]

On 29.09.2009, at 11:14, Avi Kivity wrote:

> On 09/29/2009 10:18 AM, Alexander Graf wrote:
>> 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, I just make it bounce twice, but admittedly that is not  
>> ideal.
>>
>> I'm open for suggestions here.
>>
>>
>
> How about adding a new union member to struct kvm_dirty_log:
>
>  __u64 dirty_bitmap_virt;

And modifying userspace to write to that one?

Alex

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

[Avi Kivity]

On 09/29/2009 11:17 AM, Alexander Graf wrote:
>
> On 29.09.2009, at 11:14, Avi Kivity wrote:
>
>> On 09/29/2009 10:18 AM, Alexander Graf wrote:
>>> 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, I just make it bounce twice, but admittedly that is not
>>> ideal.
>>>
>>> I'm open for suggestions here.
>>>
>>>
>>
>> How about adding a new union member to struct kvm_dirty_log:
>>
>> __u64 dirty_bitmap_virt;
>
> And modifying userspace to write to that one?
>

Yes - old userspace will still build and work (we don't remove the old 
field) on little endian or BE32, new userspace will work on all 
flavours.  We need new userspace anyway to take advantage of dirty logging.

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

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

[Alexander Graf]

Am 29.09.2009 um 06:25 schrieb Avi Kivity <avi@redhat.com>:

> On 09/29/2009 11:17 AM, Alexander Graf wrote:
>>
>> On 29.09.2009, at 11:14, Avi Kivity wrote:
>>
>>> On 09/29/2009 10:18 AM, Alexander Graf wrote:
>>>> 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, I just make it bounce twice, but admittedly that is not
>>>> ideal.
>>>>
>>>> I'm open for suggestions here.
>>>>
>>>>
>>>
>>> How about adding a new union member to struct kvm_dirty_log:
>>>
>>> __u64 dirty_bitmap_virt;
>>
>> And modifying userspace to write to that one?
>>
>
> Yes - old userspace will still build and work (we don't remove the  
> old field) on little endian or BE32, new userspace will work on all  
> flavours.  We need new userspace anyway to take advantage of dirty  
> logging.

Uh, the dirty logging bits are in place already IIRC :)

But yes, sounds like the cleaner way to do it.

Alex

>

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

[Alexander Graf]

On 29.09.2009, at 15:25, Avi Kivity wrote:

> On 09/29/2009 11:17 AM, Alexander Graf wrote:
>>
>> On 29.09.2009, at 11:14, Avi Kivity wrote:
>>
>>> On 09/29/2009 10:18 AM, Alexander Graf wrote:
>>>> 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, I just make it bounce twice, but admittedly that is not
>>>> ideal.
>>>>
>>>> I'm open for suggestions here.
>>>>
>>>>
>>>
>>> How about adding a new union member to struct kvm_dirty_log:
>>>
>>> __u64 dirty_bitmap_virt;
>>
>> And modifying userspace to write to that one?
>>
>
> Yes - old userspace will still build and work (we don't remove the  
> old field) on little endian or BE32, new userspace will work on all  
> flavours.  We need new userspace anyway to take advantage of dirty  
> logging.

How about this one? (broken whitespace!)

 From c3864a2c5e1fccff7839e47f12c09d9739ca441e Mon Sep 17 00:00:00 2001
From: Alexander Graf <agraf@suse.de>
Date: Thu, 23 Jul 2009 21:05:57 +0200
Subject: [PATCH] Enable 32bit dirty log pointers on 64bit host

With big endian userspace, passing a pointer from 32-bit userspace to
64-bit kernel space breaks.

This is what happens with dirty logging. To get the pointer interpreted
correctly, we can just check the guest's 32bit flag and treat the  
pointer
as 32 bits then.

Signed-off-by: Alexander Graf <agraf@suse.de>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e27b7a9..00f2c59 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -703,6 +703,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
         int r, i;
         int n;
         unsigned long any = 0;
+       void *target_bm;

         r = -EINVAL;
         if (log->slot >= KVM_MEMORY_SLOTS)
@@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
         for (i = 0; !any && i < n/sizeof(long); ++i)
                 any = memslot->dirty_bitmap[i];

+#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
+       /* Need to convert user pointers */
+       if (test_thread_flag(TIF_32BIT))
+               target_bm = (void*)((u64)log->dirty_bitmap >> 32);
+       else
+#endif
+       target_bm = log->dirty_bitmap;
         r = -EFAULT;
-       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
+       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
                 goto out;

         if (any)

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

[Avi Kivity]

On 09/29/2009 06:29 PM, Alexander Graf wrote:
>
> How about this one? (broken whitespace!)
>
> From c3864a2c5e1fccff7839e47f12c09d9739ca441e Mon Sep 17 00:00:00 2001
> From: Alexander Graf <agraf@suse.de>
> Date: Thu, 23 Jul 2009 21:05:57 +0200
> Subject: [PATCH] Enable 32bit dirty log pointers on 64bit host
>
> With big endian userspace, passing a pointer from 32-bit userspace to
> 64-bit kernel space breaks.
>
> This is what happens with dirty logging. To get the pointer interpreted
> correctly, we can just check the guest's 32bit flag and treat the pointer
> as 32 bits then.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index e27b7a9..00f2c59 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -703,6 +703,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
>         int r, i;
>         int n;
>         unsigned long any = 0;
> +       void *target_bm;

void __user *target_bm;

>
>         r = -EINVAL;
>         if (log->slot >= KVM_MEMORY_SLOTS)
> @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
>         for (i = 0; !any && i < n/sizeof(long); ++i)
>                 any = memslot->dirty_bitmap[i];
>
> +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
> +       /* Need to convert user pointers */
> +       if (test_thread_flag(TIF_32BIT))
> +               target_bm = (void*)((u64)log->dirty_bitmap >> 32);
> +       else
> +#endif
> +       target_bm = log->dirty_bitmap;
>         r = -EFAULT;
> -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
>                 goto out;
>
>         if (any)

Ah, that's much better.  Plus a mental note not to put pointers in 
user-visible structures in the future.  This can serve as a reminder :)

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

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

[Alexander Graf]

On 29.09.2009, at 18:42, Avi Kivity wrote:

> On 09/29/2009 06:29 PM, Alexander Graf wrote:
>>
>> How about this one? (broken whitespace!)
>>
>> From c3864a2c5e1fccff7839e47f12c09d9739ca441e Mon Sep 17 00:00:00  
>> 2001
>> From: Alexander Graf <agraf@suse.de>
>> Date: Thu, 23 Jul 2009 21:05:57 +0200
>> Subject: [PATCH] Enable 32bit dirty log pointers on 64bit host
>>
>> With big endian userspace, passing a pointer from 32-bit userspace to
>> 64-bit kernel space breaks.
>>
>> This is what happens with dirty logging. To get the pointer  
>> interpreted
>> correctly, we can just check the guest's 32bit flag and treat the  
>> pointer
>> as 32 bits then.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index e27b7a9..00f2c59 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -703,6 +703,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
>>        int r, i;
>>        int n;
>>        unsigned long any = 0;
>> +       void *target_bm;
>
> void __user *target_bm;

k, done.

>
>>
>>        r = -EINVAL;
>>        if (log->slot >= KVM_MEMORY_SLOTS)
>> @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
>>        for (i = 0; !any && i < n/sizeof(long); ++i)
>>                any = memslot->dirty_bitmap[i];
>>
>> +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
>> +       /* Need to convert user pointers */
>> +       if (test_thread_flag(TIF_32BIT))
>> +               target_bm = (void*)((u64)log->dirty_bitmap >> 32);
>> +       else
>> +#endif
>> +       target_bm = log->dirty_bitmap;
>>        r = -EFAULT;
>> -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap,  
>> n))
>> +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
>>                goto out;
>>
>>        if (any)
>
> Ah, that's much better.  Plus a mental note not to put pointers in  
> user-visible structures in the future.  This can serve as a  
> reminder :)

Heh, yeah :). At times like this I see the benefits of little endian...

The new code is in git://csgraf.de/kvm branch ppc-v5.
(Don't expect to pull updates from that branch until I release it - I  
will push -f in there, breaking git pull.)

Alex

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

[Arnd Bergmann]

On Tuesday 29 September 2009, Avi Kivity wrote:
> >
> >         r = -EINVAL;
> >         if (log->slot >= KVM_MEMORY_SLOTS)
> > @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
> >         for (i = 0; !any && i < n/sizeof(long); ++i)
> >                 any = memslot->dirty_bitmap[i];
> >
> > +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT)
> > +       /* Need to convert user pointers */
> > +       if (test_thread_flag(TIF_32BIT))
> > +               target_bm = (void*)((u64)log->dirty_bitmap >> 32);
> > +       else
> > +#endif
> > +       target_bm = log->dirty_bitmap;
> >         r = -EFAULT;
> > -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
> > +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
> >                 goto out;
> >
> >         if (any)
> 
> Ah, that's much better.  Plus a mental note not to put pointers in 
> user-visible structures in the future.  This can serve as a reminder :)

It's still broken on s390, which 

1. uses TIF_31BIT instead of TIF_32BIT
2. needs to call compat_ptr() to do a real conversion instead of a cast

The TIF_32BIT method is also not reliable. E.g. on x86_64 you are supposed
to get the 32 bit ABI when calling through INT80 instead of syscall/sysenter,
independent of the value of TIF_32BIT.

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

The patch below is an example for the canonical way to do this. Not tested!

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 897bff3..20f88ad 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2297,6 +2297,49 @@ 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 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.log);
+		if (r)
+			goto out;
+		break;
+	default:
+		r = kvm_vm_ioctl(filp, ioctl, arg);
+	}
+
+	return r;
+}
+#endif
+
 static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct page *page[1];
@@ -2331,7 +2374,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,
 };
 

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

[Avi Kivity]

On 09/30/2009 02:04 PM, Arnd Bergmann wrote:
> On Tuesday 29 September 2009, Avi Kivity wrote:
>    
>>>          r = -EINVAL;
>>>          if (log->slot>= KVM_MEMORY_SLOTS)
>>> @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
>>>          for (i = 0; !any&&  i<  n/sizeof(long); ++i)
>>>                  any = memslot->dirty_bitmap[i];
>>>
>>> +#if defined(__BIG_ENDIAN)&&  defined(CONFIG_64BIT)
>>> +       /* Need to convert user pointers */
>>> +       if (test_thread_flag(TIF_32BIT))
>>> +               target_bm = (void*)((u64)log->dirty_bitmap>>  32);
>>> +       else
>>> +#endif
>>> +       target_bm = log->dirty_bitmap;
>>>          r = -EFAULT;
>>> -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>>> +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
>>>                  goto out;
>>>
>>>          if (any)
>>>        
>> Ah, that's much better.  Plus a mental note not to put pointers in
>> user-visible structures in the future.  This can serve as a reminder :)
>>      
> It's still broken on s390, which
>
> 1. uses TIF_31BIT instead of TIF_32BIT
> 2. needs to call compat_ptr() to do a real conversion instead of a cast
>
> The TIF_32BIT method is also not reliable. E.g. on x86_64 you are supposed
> to get the 32 bit ABI when calling through INT80 instead of syscall/sysenter,
> independent of the value of TIF_32BIT.
>
> A better way to do this is to add a separate compat_ioctl() method that
> converts this for you.
>
> The patch below is an example for the canonical way to do this. Not tested!
>
> Signed-off-by: Arnd Bergmann<arnd@arndb.de>
> ---
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 897bff3..20f88ad 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2297,6 +2297,49 @@ 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 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.log);
> +		if (r)
> +			goto out;
> +		break;
> +	default:
> +		r = kvm_vm_ioctl(filp, ioctl, arg);
> +	}
> +
> +	return r;
> +}
> +#endif
> +
>   static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>   {
>   	struct page *page[1];
> @@ -2331,7 +2374,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,
>   };
>    

This is a bit painful - I tried to avoid compat_ioctl.  Maybe it's 
better to have dirty_bitmap_virt, given no existing users are impacted.

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

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

[Avi Kivity]

On 09/30/2009 03:17 PM, Avi Kivity wrote:
>>   {
>>       struct page *page[1];
>> @@ -2331,7 +2374,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,
>>   };
>   static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault 
> *vmf)
>
> This is a bit painful - I tried to avoid compat_ioctl.  Maybe it's 
> better to have dirty_bitmap_virt, given no existing users are impacted.
>

But that misses compat_ptr().  So it looks like we'll need compat_ioctl.

Patch looks fine, except s/log.log/log/.  I'd also sizeof(compat_log) 
instead of sizeof(log) to avoid frightening reviewers.

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

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

[Alexander Graf]

On 30.09.2009, at 15:29, Avi Kivity wrote:

> On 09/30/2009 03:17 PM, Avi Kivity wrote:
>>>  {
>>>      struct page *page[1];
>>> @@ -2331,7 +2374,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,
>>>  };
>>  static int kvm_vm_fault(struct vm_area_struct *vma, struct  
>> vm_fault *vmf)
>>
>> This is a bit painful - I tried to avoid compat_ioctl.  Maybe it's  
>> better to have dirty_bitmap_virt, given no existing users are  
>> impacted.
>>
>
> But that misses compat_ptr().  So it looks like we'll need  
> compat_ioctl.
>
> Patch looks fine, except s/log.log/log/.  I'd also sizeof 
> (compat_log) instead of sizeof(log) to avoid frightening reviewers.

So has there been any decision on which road to take here?

Alex

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

[Avi Kivity]

On 10/20/2009 07:09 PM, Alexander Graf wrote:
>
> On 30.09.2009, at 15:29, Avi Kivity wrote:
>
>> On 09/30/2009 03:17 PM, Avi Kivity wrote:
>>>>  {
>>>>      struct page *page[1];
>>>> @@ -2331,7 +2374,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,
>>>>  };
>>>  static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault 
>>> *vmf)
>>>
>>> This is a bit painful - I tried to avoid compat_ioctl.  Maybe it's 
>>> better to have dirty_bitmap_virt, given no existing users are impacted.
>>>
>>
>> But that misses compat_ptr().  So it looks like we'll need compat_ioctl.
>>
>> Patch looks fine, except s/log.log/log/.  I'd also sizeof(compat_log) 
>> instead of sizeof(log) to avoid frightening reviewers.
>
> So has there been any decision on which road to take here?

compat_ioctl, and being more careful in the future.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

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

[Alexander Graf]

On 20.10.2009, at 15:23, Avi Kivity wrote:

> On 10/20/2009 07:09 PM, Alexander Graf wrote:
>>
>> On 30.09.2009, at 15:29, Avi Kivity wrote:
>>
>>> On 09/30/2009 03:17 PM, Avi Kivity wrote:
>>>>> {
>>>>>     struct page *page[1];
>>>>> @@ -2331,7 +2374,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,
>>>>> };
>>>> static int kvm_vm_fault(struct vm_area_struct *vma, struct  
>>>> vm_fault *vmf)
>>>>
>>>> This is a bit painful - I tried to avoid compat_ioctl.  Maybe  
>>>> it's better to have dirty_bitmap_virt, given no existing users  
>>>> are impacted.
>>>>
>>>
>>> But that misses compat_ptr().  So it looks like we'll need  
>>> compat_ioctl.
>>>
>>> Patch looks fine, except s/log.log/log/.  I'd also sizeof 
>>> (compat_log) instead of sizeof(log) to avoid frightening reviewers.
>>
>> So has there been any decision on which road to take here?
>
> compat_ioctl, and being more careful in the future.

So I'll include Arnd's patch in my patchset instead?

Alex

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

[Avi Kivity]

On 10/20/2009 10:28 PM, Alexander Graf wrote:
>> compat_ioctl, and being more careful in the future.
>
>
> So I'll include Arnd's patch in my patchset instead?

Send it independently and Marcelo or myself will apply it.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.