Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps

["Fernando Luis Vázquez Cao" <fernando-gVGce1chcLdL9jVzuh4AOg@public.gmane.org>]

On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>
>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>> 	__u32 padding1;
>>>> 	union {
>>>> 		void __user *dirty_bitmap; /* one bit per page */
>>>> -		__u64 padding2;
>>>> +		__u64 addr;
>>>
>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>
>> So the high 32 bits are zero. Where's the problem?
> 
> If we are careful enough to cast the addr appropriately we should be fine,
> even if we keep the padding field in the union. I am not saying that it
> breaks 32 architectures but that it can potentially be problematic.
> 
>>>> +	case KVM_SWITCH_DIRTY_LOG: {
>>>> +		struct kvm_dirty_log log;
>>>> +
>>>> +		r = -EFAULT;
>>>> +		if (copy_from_user(&log, argp, sizeof log))
>>>> +			goto out;
>>>> +		r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>> +		if (r)
>>>> +			goto out;
>>>> +		r = -EFAULT;
>>>> +		if (copy_to_user(argp, &log, sizeof log))
>>>> +			goto out;
>>>> +		r = 0;
>>>> +		break;
>>>> +	}
>>>
>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>
>> The compat code just forwards everything to the generic ioctls.
> 
> The compat code uses struct compat_kvm_dirty_log instead of
> struct kvm_dirty_log to communicate with user space so
> the necessary conversions needs to be done before invoking
> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
> 
> By the way we probable should move the definition of struct
> compat_kvm_dirty_log to a header file.

It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
Are you considering a different approach to tackle the issues that we
have with a big-endian userspace?

Thanks,

Fernando