Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 12 Mar 2012 13:18:41 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Robert Millan <rmh@freebsd.org>
Cc:        Adrian Chadd <adrian@freebsd.org>, freebsd-arch@freebsd.org
Subject:   Re: [PATCH] Add compatibility <sys/io.h>
Message-ID:  <20120312121852.P1122@besplex.bde.org>
In-Reply-To: <CAOfDtXOCyjga5QHz98Re3jXkefNzB-MbULcAVUQH89ToVLkw9g@mail.gmail.com>
References:  <CAOfDtXPGPP0reN9NTBw_5%2BNwXZ56Yy0oyx_fH%2BDOvmpc1O%2BQdQ@mail.gmail.com> <CAJ-Vmonu_ApSd192cjvsW6k3eNNK4Kz=MmAMe_e=zmwbrS8Ayw@mail.gmail.com> <CAOfDtXOCyjga5QHz98Re3jXkefNzB-MbULcAVUQH89ToVLkw9g@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--0-404940735-1331518721=:1122
Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE

On Sun, 11 Mar 2012, Robert Millan wrote:

> Hi Adrian,
>
> El 9 de mar=C3=A7 de 2012 23:01, Adrian Chadd <adrian@freebsd.org> ha esc=
rit:
>> I really, really don't like the idea of having the same function have
>> different argument orders based on the include file you're using.
>>
>> That will lead to all kinds of weird ass debugging issues later on.
>
> Definitely.  This disparity is terribly harmful.  I think everybody
> will agree this is the biggest problem here.

Why terribly?  Code that is chummy enough with the implementation to
include an undocumented header to get an undocumented API must know
what it is doing.  (outb() is not quite undocumented.  In all of
/usr/share/man, it is mentioned in passing in lpt(4) and ppbus(4),
and is fully supported by loader(8), where it is a Forth word.  So
a non-chummy userland must translate to Forth and exec loader(8) to
do outb() for it :-).)

> I don't think this problem is introduced by my patch though, rather I
> think my proposal is one of the possible ways to address it.  But of
> course it's not the only way :-)
>
>> Why is it that we can't fix the upstream code?
>
> Well even with current FreeBSD codebase I see two possible kind of
> trouble in upstream code:
>
> - fails to build
> - builds successfully but sends I/O to the wrong ports
>
> there are many ways upstream code can missbehave and reach either of
> these problems.  As the second one is clearly the worst result, I
> think that preventing that should be the priority.
>
> Bruce made some interesting suggestions....

I would prefer to make it fail to build if it gets the arg order wrong,
but I don't see how to do that, since both args are integers.

>> [...]
>> #error "no user serving parts inside" would prevent cpufunc.h being abus=
ed
>> at any time :-).
>
> I think this would give the expected results, as long as we provide a
> suitable alternative for userland users.

Why should we provide it?  They should have written their own 1 line
of asm.  Or they can try using bus space for portability.

>>>>  FreeBSD code: outw(port, data);
>>>>  Glibc code: outw(data, port);
>>
>> The FreeBSD order seemed natural to me since I was familiar with
>> Intel/DOS and they never used AT&T asm order.  I put the i/o functions
>> in cpufunc.h, but 386BSD seems to have had them in the Intel order,
>> since 4.4BSD has them in Intel order.
>
> Either way in both cases it looks arbitrary to me.  The problem is
> that they don't match :-(

One conforms to the manufacturer's order and existing practice.

>> A sys header is more unsuitable, since this is very machine-dependent.
>> Even bus space's header is not in sys (it is <machine/bus.h>), though
>> many of its APIs are MI (with very MD internals for the typeded types).
>>
>> Userland has even more reasons to avoid the machine dependencies.
>> Bus space is more complex, but seems to work OK in userland:
>>
>> % #include <sys/types.h>
>> % #include <machine/bus.h>
>> % % void
>> % my_outb(unsigned int port, unsigned char data)
>> % {
>> %       bus_space_write_1(X86_BUS_SPACE_IO, port, 0, data);
>> % }
>>
>> This gives only minor pessimizations relative to the raw inline asm
>> outb().
>
> Looks good.  So you suggest we tell userspace users to switch to
> bus_space_write_*?

The problem with that is that if you don't do the switch yourself then
most users won't even know that it is necessary.  You have to remove
the functionaility in cpufunc.h and/or add messy userland ifdefs as
well as messy kernel ifdefs to unremove it, so that users who don't
know what they are doing and which you haven't adjusted get warned
by build failures.  All users that knew what they were doing have to
do it differently.

> Btw what are the pessimizations?  I can't see anything in that
> function which can't be optimized away after inlining.

There seemed to be an extra load.  I'm not sure if the extra level
of inlines can do constant folding.  But a quick test with inb(0xc2)
written as bus_space_read_1(X86_BUS_SPACE_IO, 0xc1, 1) gave good
results -- only the source code is ugly.

> I could prepare a patch to fix those.
>
>> - <machine/bus.h> has undocumented namespace pollution.  It includes
>>  <machine/cpufunc.h>, and even uses outb() from it.  So I had to
>>  rename my function my_outb() to get it to compile.
>
> I wouldn't duplicate outb() in <machine/bus.h>, IIRC its
> implementation is compiler-dependant so it's not just a one-liner.

It's a matter of copying a 1-liner from cpufunc.h.  outsb() is a few
lines, but now it's next to 8 lines of rather bogus inline asm for the
MEM case of bus_space_write_multi_1.  The IO case takes only 3 lines,
and it's strange that the MEM case takes so many more.

The inline asm for the MEM case is rather bogus since it is "optimized"
to use the lodsb and loop instructions which have been pessimizations
for almost 20 years.  A simple C loop is probably faster on 486's and
later.  The x86 bus.h uses these pessimal instructions for most "multi"
and "region" MEM i/o's and even for some IO (duh) i/o's.  I think
simple C loops that build up "multi" and "region" i/o's from "single"
(size 1/2/4/8) i/o's would be better on 486's and later in general.
This depends on the compiler optimizing away multiple tags comparisons
and the like.  For most hardware, the tag and loop overhead is
irrelevant, since it take a couple of cycles run in parallel with the
actual i/o which takes hundreds or thousands of cycles.

The x86 bus.h still has many bogus prototypes for static inline
functions, with about 15% of the file consisting of duplication for
them.  In NetBSD, these were to support K&R function definitions, but
those never made any sense for inline functions, and FreeBSD never
supported K&R for bus space.

> How about this:
>
> - Create <machine/_cpufunc.h>
> - Move outb() et al to that file, and prefix them with two underscores.

Churn.  It's already undefined by being documented.

However, most of the headers in <machine> are bugs.  Most of them
should have been put in the ${ARCH}/${ARCH} directory (e.g., i386/i386)
near the kernel sources that use them.

> - Make <machine/cpufunc.h> include <machine/_cpufunc.h> and implement
> outb() as a #define to __outb().
>
> then <machine/bus.h> can include <machine/_cpufunc.h> and use __outb() et=
c.

I don't agree with renaming kernel headers or APIs because applications
are chummy with them but not chummy enough to abuse them safely.

Bruce
--0-404940735-1331518721=:1122--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120312121852.P1122>