svn commit: r184974 - head/sys/dev/mfi

[Peter Wemm]

On Mon, Nov 17, 2008 at 1:13 PM, John Baldwin <jhb at freebsd.org> 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 at wemm.org; peter at FreeBSD.org; peter at 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