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

[Jan Kiszka <jan.kiszka@siemens.com>]

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