From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1IpBSM-0002CY-7W for qemu-devel@nongnu.org; Mon, 05 Nov 2007 18:34:10 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1IpBSK-0002CM-RC for qemu-devel@nongnu.org; Mon, 05 Nov 2007 18:34:08 -0500 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1IpBSK-0002CJ-MO for qemu-devel@nongnu.org; Mon, 05 Nov 2007 18:34:08 -0500 Received: from sp604002mt.neufgp.fr ([84.96.92.61] helo=sMtp.neuf.fr) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1IpBSK-0003Lx-D2 for qemu-devel@nongnu.org; Mon, 05 Nov 2007 18:34:08 -0500 Received: from [84.99.204.100] by sp604002mt.gpm.neuf.ld (Sun Java System Messaging Server 6.2-5.05 (built Feb 16 2006)) with ESMTP id <0JR2009WY44GCCT0@sp604002mt.gpm.neuf.ld> for qemu-devel@nongnu.org; Tue, 06 Nov 2007 00:33:53 +0100 (CET) Date: Tue, 06 Nov 2007 00:33:29 +0100 From: Fabrice Bellard Subject: Re: [Qemu-devel] Re: [PATCH] efault - add data type to put_user()/get_user() In-reply-to: <1194303622.5154.143.camel@phantasm.home.enterpriseandprosperity.com> Message-id: <472FA849.9070908@bellard.org> MIME-version: 1.0 Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7BIT References: <1193869827.19343.38.camel@phantasm.home.enterpriseandprosperity.com> <1193870136.19343.43.camel@phantasm.home.enterpriseandprosperity.com> <1193870631.19343.51.camel@phantasm.home.enterpriseandprosperity.com> <1194045983.2168.17.camel@phantasm.home.enterpriseandprosperity.com> <472CC669.30907@bellard.org> <1194294179.5154.86.camel@phantasm.home.enterpriseandprosperity.com> <472F8E59.800@bellard.org> <1194303622.5154.143.camel@phantasm.home.enterpriseandprosperity.com> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: thayne@c2.net, qemu-devel@nongnu.org Thayne Harbaugh wrote: > On Mon, 2007-11-05 at 22:42 +0100, Fabrice Bellard wrote: >> Thayne Harbaugh wrote: >>> On Sat, 2007-11-03 at 20:05 +0100, Fabrice Bellard wrote: >>>> I think that using host addresses in __put_user and __get_user is not >>>> logical. They should use target addresses as get_user and put_user. As >>>> Paul said, It is not worth mixing get/put/copy and lock/unlock functions. >>> Please see the "RFC: x86_64 Best way to fix 'cast to pointer'" email for >>> some discussion of get/put/copy and lock/unlock. {get,put}_user() is >>> used for individual ints or other atomically writable types that are >>> passed as pointers into a syscall. copy_{to,from}_user_() are >>> used for structures that are passed to a syscall. lock/unlock() will be >>> used internally in these because lock/unlock does address translation. >>> lock/unlock() are still needed and are independent. __{get,put}_user() >>> will operate internally in these functions on structure data members >>> where lock/unlock() access_ok() have already been called. >> I believed I was clear : once you keep lock/unlock, there is no point in >> modifying the code to use put_user/get_user and copy[to,from]_user. > > without lock/unlock how do you propose that target/host address > translation be performed? Are you suggesting a g2h() inside of > copy_{to,from}_user()? Yes. Keep in mind that g2h() is only the simple case in case the target address space is directly addressable. I don't want that the API makes this supposition, so in the general case copy_[to,from]user are the only method you have to exchange data with the user space. >> So either you keep the code as it is and extend lock and unlock to >> return an error code or you suppress all lock/unlock to use a Linux like >> API (i.e. put_user/get_user , copy[to,from]_user, access_ok, >> __put_user/__get_user). > > The error code because lock/unlock_user would then call access_ok()? Of course. >> So for gettimeofday, if we exclude the fact that the conversion of >> struct timeval will be factorized, you have either: >> >> { >> struct timeval tv; >> ret = get_errno(gettimeofday(&tv, NULL)); >> if (!is_error(ret)) { >> struct target_timeval *target_tv; >> >> lock_user_struct(target_tv, arg1, 0); >> target_tv->tv_sec = tswapl(tv->tv_sec); >> target_tv->tv_usec = tswapl(tv->tv_usec); >> if (unlock_user_struct(target_tv, arg1, 1)) { >> ret = -TARGET_EFAULT; >> goto fail; >> } >> } >> } >> >> Or >> >> { >> struct timeval tv; >> ret = get_errno(gettimeofday(&tv, NULL)); >> if (!is_error(ret)) { >> struct target_timeval target_tv; >> >> target_tv.tv_sec = tswapl(tv->tv_sec); >> target_tv.tv_usec = tswapl(tv->tv_usec); >> if (copy_to_user(arg1, &target_tv, sizeof(target_tv)) { >> ret = -TARGET_EFAULT; >> goto fail; >> } >> } >> } > > I don't see where the second one is handling target/host address > translation. copy_to_user() does it. > A problem with both of the above examples is that they use tswapl(). > Without the proper type casting tswapl() doesn't do proper sign > extension when dealing with 64bit/32bit host/target relationships. I've > fixed more than one location where this has resulted in bugs. This is an unrelated problem. If tswapl is not sufficient then another function can be added. > [...] Regards, Fabrice.