From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Fernando_Luis_V=E1zquez_Cao?= Subject: Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps Date: Fri, 23 Apr 2010 19:17:40 +0900 Message-ID: <4BD173C4.6060004@oss.ntt.co.jp> References: <20100420195349.dab60b1d.yoshikawa.takuya@oss.ntt.co.jp> <20100420200353.2d2a6dec.yoshikawa.takuya@oss.ntt.co.jp> <4BCEB777.6040505@oss.ntt.co.jp> <264937F8-98E4-4A73-9C7A-837793E1DAC8@suse.de> <4BCFB862.7010509@oss.ntt.co.jp> <4BCFE805.5040500@oss.ntt.co.jp> <4790E0BF-88C0-4D3E-9A32-110AB7AA84D4@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Takuya Yoshikawa , avi@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org, kvm-ppc@vger.kernel.org To: Alexander Graf Return-path: Received: from serv2.oss.ntt.co.jp ([222.151.198.100]:59864 "EHLO serv2.oss.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755693Ab0DWKRm (ORCPT ); Fri, 23 Apr 2010 06:17:42 -0400 In-Reply-To: <4790E0BF-88C0-4D3E-9A32-110AB7AA84D4@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On 04/23/2010 08:29 AM, Alexander Graf wrote: >=20 > On 22.04.2010, at 08:09, Fernando Luis V=E1zquez Cao wrote: >=20 >> On 04/22/2010 11:45 AM, Fernando Luis V=E1zquez Cao wrote: >>> On 04/21/2010 06:41 PM, Alexander Graf wrote: >>>> On 21.04.2010, at 10:29, Fernando Luis V=E1zquez 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 b= e fine, >>> even if we keep the padding field in the union. I am not saying tha= t it >>> breaks 32 architectures but that it can potentially be problematic. >>> >>>>>> + case KVM_SWITCH_DIRTY_LOG: { >>>>>> + struct kvm_dirty_log log; >>>>>> + >>>>>> + r =3D -EFAULT; >>>>>> + if (copy_from_user(&log, argp, sizeof log)) >>>>>> + goto out; >>>>>> + r =3D kvm_vm_ioctl_switch_dirty_log(kvm, &log); >>>>>> + if (r) >>>>>> + goto out; >>>>>> + r =3D -EFAULT; >>>>>> + if (copy_to_user(argp, &log, sizeof log)) >>>>>> + goto out; >>>>>> + r =3D 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 w= e >> have with a big-endian userspace? >=20 > IIRC the issue was a pointer inside of a nested structure, no? I would say the reason is that if we did not convert the user-space poi= nter to a "void *" kvm_get_dirty_log() would end up copying the dirty log to (log->dirty_bitmap << 32) | 0x00000000 Am I missing something? Thanks, =46ernando