[PATCH] Add compatibility <sys/io.h>

[Robert Millan]

Hi Adrian,

El 9 de març de 2012 23:01, Adrian Chadd <adrian at freebsd.org> ha escrit:
> 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.

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....

El 10 de març de 2012 14:26, Bruce Evans <brde at optusnet.com.au> ha escrit:
> cpufunc.h has no user-serving parts inside, so it can't conflict with
> any legal include.  However, abusing it in userland is convenient.
>
> Even using the port i/o functions in it in the kernel is an error.
> Bus space functions should be used instead.  However, the i/o functions
> in it are much older than bus space, and abusing them in MD code in
> the kernel is convenient.
>
> [...]
> #error "no user serving parts inside" would prevent cpufunc.h being abused
> at any time :-).

I think this would give the expected results, as long as we provide a
suitable alternative for userland users.

>>>  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 :-(

> 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_*?

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

> To get this to work, I had to work around the following problems:
> - the prerequisite <sys/types.h> is undocumented
> - someone renamed I386_BUS_SPACE_IO and AMD64_BUS_SPACE_IO to
>  X86_BUS_SPACE_IO.  This gives portability problems, and I only remembered
>  the old names.  Having the machine name in the API at all is a bug.
>  Especially for BUS_SPACE_MEM, the machine arch can't make any difference.
>  There might be different types of memory mapped i/o, but a plain MEM should
>  mean "ordinary" memory, or the least-extraordinary memory that the machine
>  has, or if that is too extraordinary, then just don't implement
>  BUS_TYPE_MEM.  Similarly for IO.  It means port i/o for x86, and jusr
>  including the x86 header says that you want the x86 version of "ordinary"
>  ports.

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.

How about this:

- Create <machine/_cpufunc.h>
- Move outb() et al to that file, and prefix them with two underscores.
- 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() etc.

-- 
Robert Millan