From owner-freebsd-arch@FreeBSD.ORG Mon Mar 12 02:18:53 2012 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 450921065674; Mon, 12 Mar 2012 02:18:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx10.syd.optusnet.com.au (fallbackmx10.syd.optusnet.com.au [211.29.132.251]) by mx1.freebsd.org (Postfix) with ESMTP id 8FDE98FC08; Mon, 12 Mar 2012 02:18:52 +0000 (UTC) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by fallbackmx10.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2C2Ip7c017526; Mon, 12 Mar 2012 13:18:51 +1100 Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2C2IfTA008798 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 12 Mar 2012 13:18:42 +1100 Date: Mon, 12 Mar 2012 13:18:41 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Robert Millan In-Reply-To: Message-ID: <20120312121852.P1122@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-404940735-1331518721=:1122" Cc: Adrian Chadd , freebsd-arch@freebsd.org Subject: Re: [PATCH] Add compatibility X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Mar 2012 02:18:53 -0000 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 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 ), 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 >> % #include >> % % 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. > >> - has undocumented namespace pollution. It includes >> , 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 , 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 > - 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 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 include and implement > outb() as a #define to __outb(). > > then can include 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--