From owner-svn-src-head@FreeBSD.ORG Mon Nov 17 21:21:54 2008 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E19721065775 for ; Mon, 17 Nov 2008 21:21:54 +0000 (UTC) (envelope-from peter@wemm.org) Received: from rn-out-0910.google.com (rn-out-0910.google.com [64.233.170.191]) by mx1.freebsd.org (Postfix) with ESMTP id 9B1898FC17 for ; Mon, 17 Nov 2008 21:21:54 +0000 (UTC) (envelope-from peter@wemm.org) Received: by rn-out-0910.google.com with SMTP id j71so2637775rne.12 for ; Mon, 17 Nov 2008 13:21:54 -0800 (PST) Received: by 10.142.156.19 with SMTP id d19mr2172789wfe.289.1226956913141; Mon, 17 Nov 2008 13:21:53 -0800 (PST) Received: by 10.142.255.21 with HTTP; Mon, 17 Nov 2008 13:21:53 -0800 (PST) Message-ID: Date: Mon, 17 Nov 2008 13:21:53 -0800 From: "Peter Wemm" To: "John Baldwin" In-Reply-To: <200811171613.36602.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200811170237.mAH2bjY5088186@ambrisko.com> <200811171211.42740.jhb@freebsd.org> <20081117193541.GG90129@deviant.kiev.zoral.com.ua> <200811171613.36602.jhb@freebsd.org> Cc: Doug Ambrisko , src-committers@freebsd.org, Doug Ambrisko , svn-src-all@freebsd.org, svn-src-head@freebsd.org, Kostik Belousov Subject: Re: svn commit: r184974 - head/sys/dev/mfi X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Nov 2008 21:21:55 -0000 On Mon, Nov 17, 2008 at 1:13 PM, John Baldwin wrote: > On Monday 17 November 2008 02:35:41 pm Kostik Belousov wrote: >> On Mon, Nov 17, 2008 at 12:11:41PM -0500, John Baldwin wrote: >> > On Sunday 16 November 2008 09:37:45 pm Doug Ambrisko wrote: >> > > Kostik Belousov writes: >> > > | On Fri, Nov 14, 2008 at 09:05:45PM +0000, Doug Ambrisko wrote: >> > > | > Author: ambrisko >> > > | > Date: Fri Nov 14 21:05:45 2008 >> > > | > New Revision: 184974 >> > > | > URL: http://svn.freebsd.org/changeset/base/184974 >> > > | > >> > > | > Log: >> > > | > When running a 32bit app. on amd64, ensure the bits above 32bit >> > > | > are zero for the copyout. Confirmed by LSI. >> > > | > >> > > | > Modified: >> > > | > head/sys/dev/mfi/mfi.c >> > > | > >> > > | > Modified: head/sys/dev/mfi/mfi.c >> > > | > >> > > ============================================================================== >> > > | > --- head/sys/dev/mfi/mfi.c Fri Nov 14 18:09:19 2008 (r184973) >> > > | > +++ head/sys/dev/mfi/mfi.c Fri Nov 14 21:05:45 2008 (r184974) >> > > | > @@ -2130,6 +2130,14 @@ mfi_ioctl(struct cdev *dev, u_long cmd, >> > > | > ->mfi_frame.raw[ioc->mfi_sense_off], >> > > | > &sense_ptr.sense_ptr_data[0], >> > > | > sizeof(sense_ptr.sense_ptr_data)); >> > > | > +#ifdef __amd64__ >> > > | > + if (cmd != MFI_CMD) { >> > > | > + /* >> > > | > + * not 64bit native so zero out any address >> > > | > + * over 32bit */ >> > > | > + sense_ptr.high = 0; >> > > | > + } >> > > | > +#endif >> > > | > error = copyout(cm->cm_sense, sense_ptr.user_space, >> > > | > ioc->mfi_sense_len); >> > > | > if (error != 0) { >> > > | > @@ -2353,6 +2361,13 @@ mfi_linux_ioctl_int(struct cdev *dev, u_ >> > > | > ->lioc_frame.raw[l_ioc.lioc_sense_off], >> > > | > &sense_ptr.sense_ptr_data[0], >> > > | > sizeof(sense_ptr.sense_ptr_data)); >> > > | > +#ifdef __amd64__ >> > > | > + /* >> > > | > + * only 32bit Linux support so zero out any >> > > | > + * address over 32bit >> > > | > + */ >> > > | > + sense_ptr.high = 0; >> > > | > +#endif >> > > | > error = copyout(cm->cm_sense, sense_ptr.user_space, >> > > | > l_ioc.lioc_sense_len); >> > > | > if (error != 0) { >> > > | >> > > | Would it make sense to perform this cut slightly more generically, by >> > > | checking whether the current process is 32bit ? >> > > | >> > > | We still have not grew the easy to check flag or attribute of the > image, >> > > | but usual practice is to compare p_sysent with corresponding sysvec, >> > > | like >> > > | if (td->td_proc->p_sysent == &ia32_freebsd_sysvec) >> > > | or >> > > | if (td->td_proc->p_sysent == &elf_linux_sysvec) >> > > >> > > So far we do it based on the ioctl since the 32bit or 64bit ioctl >> > > value is different. I put in that comment for Linux since there >> > > is talk/work for Linux amd64 emulation. For 64bit Linux ioctl >> > > support we need a 64bit structure defined. When the ioctl can't >> > > be figured out then I've used the p_sysent. Eventually, something >> > > that is more generic then the #ifdef __amd64__ should be done >> > > in all the drivers that do this emulation. >> > >> > I prefer depending on things like ioctl values and the 32-bit sysctl flag > when >> > possible. If we do have to directly check for the ABI, I'd much rather > have >> > a flags field in sysent rather than trying to compare against global > symbols, >> > as you can't compare against a global symbol unless it is present in the >> > kernel. Something like: >> > >> > if (td->td_proc->p_sysent->sy_flags & SY_32) >> > >> > or some such. I've wanted to have a COMPAT_FREEBSD32 that gets > auto-enabled >> > when you turn on COMPAT_IA32 on amd64 and ia64. It would also potentially > be >> > enabled by a COMPAT_SPARC8 or some such on sparc64 if we ever had that. >> >> Ok, what about the following. I only compiled it on i386/amd64. And, >> there are more places to convert to such checks, for sure. >> >> [yummy patch] > > Commit! Note that this is not MFC'able due to ABI breakage for older linux.ko > modules, but this is the proper solution for 8.0+. > > -- > John Baldwin > I was thinking of suggesting a macro to replace the verbose test if curproc->td_proc->p_sysent->sv_flags, but I couldn't think of something off the top of my head. - if (curthread->td_proc->p_sysent == &ia32_freebsd_sysvec) { + if (curthread->td_proc->p_sysent->sv_flags & SV_ILP32) { if (SV_FLAGS(curthread) & SV_ILP32) ... or the like. I'm not set on this, it just seemed like it might be worth mentioning. Also, change curthread->td_proc with curproc, for what its worth. -- Peter Wemm - peter@wemm.org; peter@FreeBSD.org; peter@yahoo-inc.com; KI6FJV "All of this is for nothing if we don't go to the stars" - JMS/B5 "If Java had true garbage collection, most programs would delete themselves upon execution." -- Robert Sewell