Bus space routines

Wed Jun 19 22:17:39 UTC 2013

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-06-19 10:56:41 -0400, John Baldwin wrote:
> On Tuesday, June 18, 2013 6:45:01 pm Marius Strobl wrote:
>> On Tue, Jun 18, 2013 at 04:19:16PM -0600, Warner Losh wrote:
>>> 
>>> On Jun 18, 2013, at 4:03 PM, Jung-uk Kim wrote:
>>> 
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>> 
>>>> On 2013-06-18 17:49:57 -0400, Marius Strobl wrote:
>>>>> On Tue, Jun 18, 2013 at 09:24:19PM +0000, Jung-uk Kim
>>>>> wrote:
>>>>>> On 2013-06-18 17:14:38 -0400, Jung-uk Kim wrote:
>>>>>>> On 2013-06-18 17:06:48 -0400, Jung-uk Kim wrote: 2013?
>>>>>>> 6? 18? 17:06, Jung-uk Kim ? ?:> On 2013-06-18 16:59:43
>>>>>>> -0400, Marius Strobl wrote:
>>>>>>>>> On Tue, Jun 18, 2013 at 02:17:53PM -0400, Jung-uk
>>>>>>>>> Kim wrote:
>>>>>>>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>>>>>>>> 
>>>>>>>>>> On 2013-06-18 08:40:38 -0400, Marius Strobl
>>>>>>>>>> wrote:
>>>>>>>>>>> On Tue, Jun 18, 2013 at 01:30:34PM +0200,
>>>>>>>>>>> Niclas Zeising wrote:
>>>>>>>>>>>> On 2013-06-18 13:13, Marius Strobl wrote:
>>>>>>>>>>>>> On Tue, Jun 18, 2013 at 12:20:14PM +0200,
>>>>>>>>>>>>> Niclas Zeising wrote:
>>>>>>>>>>>>>> This has been discussed before [1], but
>>>>>>>>>>>>>> there seem to still be a lack of
>>>>>>>>>>>>>> consensus, so I'll ask again.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Should in*/out* macros or bus_space*
>>>>>>>>>>>>>> functions be used in userland code? The
>>>>>>>>>>>>>> background is that the port
>>>>>>>>>>>>>> devel/libpciaccess uses these routines
>>>>>>>>>>>>>> on FreeBSD.  In a first incarnation it
>>>>>>>>>>>>>> used the bus_space* routines, see this
>>>>>>>>>>>>>> patch:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
> http://trillian.chruetertee.ch/ports/browser/trunk/devel/libpciaccess/files/patch-
>
> 
src-freebsd_pci.c?rev=591
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>> 
>>>>>>>>>>>>>> 
>>>> This was later changed to use the in*/out* macros directly,
>>>> with the
>>>>>>>>>>>>>> motivation that the bus_space* functions
>>>>>>>>>>>>>> is a KPI that shouldn't be used in
>>>>>>>>>>>>>> userland.  See following for an updated
>>>>>>>>>>>>>> patch:
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
> http://trillian.chruetertee.ch/ports/browser/trunk/devel/libpciaccess/files/patch-
>
> 
src-freebsd_pci.c?rev=815
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>> 
>>>>>>>>>>>>>> 
>>>> The problem is that the in*/out* macros differ between
>>>> FreeBSD and
>>>>>>>>>>>>>> Debian/kFreeBSD, and Debian/kFreeBSD want
>>>>>>>>>>>>>> to switch back to use bus_space* again.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> My question is simply, which one is
>>>>>>>>>>>>>> correct, or should libpciaccess be
>>>>>>>>>>>>>> reworked in a completely different way?
>>>>>>>>>>>>> 
>>>>>>>>>>>>> The latter; in*/out*() are somewhat okay if
>>>>>>>>>>>>> you want to restrict this to work on x86
>>>>>>>>>>>>> without PCI domains only. The above
>>>>>>>>>>>>> approach to using bus_space(9) is one big
>>>>>>>>>>>>> hack, though. The right way for employing
>>>>>>>>>>>>> that API is to allocate the corresponding
>>>>>>>>>>>>> bus resource of a particular device and
>>>>>>>>>>>>> then to obtain bus tag and handle via 
>>>>>>>>>>>>> rman_get_bus{tag,handle}(9) - which won't
>>>>>>>>>>>>> work from userland, however. The shortcut
>>>>>>>>>>>>> to just stick in {AMD64,I386}_BUS_SPACE_IO
>>>>>>>>>>>>> instead is totally unportable as generally
>>>>>>>>>>>>> a bus tag isn't a mere constant and also
>>>>>>>>>>>>> may depend on which PCI bus and domain a
>>>>>>>>>>>>> particular device is located on/in. What we
>>>>>>>>>>>>> really need is a proper interface allowing 
>>>>>>>>>>>>> userland to access PCI I/O and memory
>>>>>>>>>>>>> registers, f. e. via /dev/pci, and for
>>>>>>>>>>>>> libpciaccess to build upon that, i. e.
>>>>>>>>>>>>> essentially the same as things work on/with
>>>>>>>>>>>>> Linux and /sys/bus/pci/device. As a 
>>>>>>>>>>>>> side-effect this then also permits to
>>>>>>>>>>>>> properly sanity check PCI accesses from
>>>>>>>>>>>>> userland within the kernel.
>>>>>>>>>>>>> 
>>>>>>>>>>>> 
>>>>>>>>>>>> That is true, however, it won't build itself
>>>>>>>>>>>> today, and we need to have this working in
>>>>>>>>>>>> the meantime, so what do you suggest we use
>>>>>>>>>>>> for now?
>>>>>>>>>>> 
>>>>>>>>>>> That's hard to say as architecturally neither 
>>>>>>>>>>> in*/out*() nor bus_space(9) are the way to
>>>>>>>>>>> proceed. Tentatively, I'd go with abusing the
>>>>>>>>>>> latter as that approach _should_ allow to make
>>>>>>>>>>> things additionally work one another one or two
>>>>>>>>>>> architectures - in particular powerpc - without
>>>>>>>>>>> introducing an #ifdef hell.
>>>>>>>>>> 
>>>>>>>>>> AFAIK, bus_space(9) can only work for amd64 and
>>>>>>>>>> i386 in userland by pure luck.  It can never work
>>>>>>>>>> for powerpc if I'm reading the MD headers
>>>>>>>>>> correctly.
>>>>>>> 
>>>>>>>>> Actually, I think that by cloning bs_le_tag in
>>>>>>>>> userland as far as necessary, i. e. leaving out
>>>>>>>>> things like mapping/unmapping and
>>>>>>>>> allocation/deallocation etc., and using that as bus
>>>>>>>>> tag, bus_space(9) has a fairly good chance of
>>>>>>>>> working in userland for powerpc in this case. 
>>>>>>>>> Obviously, that's harder to do than faking the bus
>>>>>>>>> tag for x86, though.
>>>>>>> 
>>>>>>>> Please don't forget the point of this thread, i.e.,
>>>>>>>> finding simple MI interface. ;-)
>>>>>>> 
>>>>>>>>>> Also, I strongly disagree with abusing bus_space
>>>>>>>>>> because it gives a bad example.
>>>>>>> 
>>>>>>>>> Well, I strongly believe that both in*/out*() and 
>>>>>>>>> bus_space(9) give very bad examples for userland
>>>>>>>>> code :)
>>>>>>> 
>>>>>>>> If you insist, we can simply use io(4).
>>>>>>> 
>>>>>>> I went ahead and implemented it:
>>>>>>> 
>>>>>>> http://people.freebsd.org/~jkim/libpciaccess.diff
>>>>>>> 
>>>>>>> This should work with powerpc and other platforms with
>>>>>>> working io(4).
>>>>>> 
>>>>>> I just realized powerpc does not have /dev/io, sorry.
>>>>>> Anyway, my point was there is userland API already and we
>>>>>> don't have to reinvent the wheel.
>>>>>> 
>>>>> 
>>>>> Essentially, the issue with io(4) is the same as with
>>>>> in*/out*(); you can't implement it in a sane way on
>>>>> architectures such as powerpc that have busses of different
>>>>> endianesses, apart from some other complications.
>>>> 
>>>> Why can't we implement it to always get/set in host byte
>>>> order regardless of underlying bus?  Can't the MD portion of
>>>> the driver figure it out magically?
>>> 
>>> Not typically. Since all the world is intel, all the world is
>>> little
> endian... Plus, the PCI bus is defined to be little endian, not
> host endian. Not all drivers cope well with this.
>>> 
>> 
>> These are good arguments but IMO the real hard part would be to 
>> derive the endianess and whether it's an I/O or memory mapped 
>> register etc. solely from the port address supplied via
>> io(4)/in*/ out*(). We currently don't have any global
>> infrastructure in the kernel to query for that information; quite
>> the contrary, with newbus and bus_space(9) such information
>> generally is local to a specific bus instance. Generally, there
>> also isn't something like the fixed ISA range on x86, i. e. for
>> example on one sparc64 machine address 0x1000 might refer to a
>> memory mapped register on a big-endian SBus while on another one
>> it could translate to a I/O BAR on a little- endian PCI bus.
>> Consequently, from a kernel perspective it's way simpler if an
>> userland interface would allow to specify "read from offset A of
>> BAR B on PCI device C living on BUS D in domain E".
> 
> Yes, something like the config access, but having the address be 
> 'offset X in bar Y'.  This would be MI and would use bus_space in
> the kernel.  If you wanted a way to mmap BARs you could implement
> that (mostly) trivially as well.
> 
> What operations does libpciaccess want to do with BARs:
> read_[1248] and write_[1248]?

Yes.

> Does it want to do anything else (mmap BARs, etc.)?

Not ATM.

http://cgit.freedesktop.org/xorg/lib/libpciaccess/commit/src/common_io.c?id=5e8d4c1

    for (bar = 0; bar < 6; bar++) {
        struct pci_mem_region *region = &(dev->regions[bar]);
        if (!region->is_IO)
            continue;
    ...

Please note we do not support I/O via PCI BARs yet on *any* platform.
 In fact, we only support ISA I/O ports, i.e., just enough to support
VGA/VESA for x86, and possibly ia64.  AFAICT, the API is available but
X.org does not need it ATM.  In other words, there is no hurry at all.
 In fact, I don't think they will ever use the API because they
already moved these things to kernel-side anyway. ;-)

Jung-uk Kim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (FreeBSD)

iQEcBAEBAgAGBQJRwi3lAAoJECXpabHZMqHO2wYIAJURutCs8gKsuxrfycAR4vDL
T8hFcA5sR4WFJBuyMLsKxagcF0A6+HJtyTW8V7yC1dqt9lqMWLUAPHrAFv9Fdcsg
Ztbe3N8FGIbKva6VrtfdYWHjE4kv3Qb5OblVsW1s/kXm4Cetei5dQ4HXA1Q340LT
7OAlFxLaWMfLjkSj939BlBeXJ2vApnEEkYEFJiEGxDMm6laFeB/B1vNfSQbF2ncd
x8VYf5qwal1kBLs8w5mdAG7Eo9RAatWNa5/Cycr4qdALKpqRZXb/pTob07pBAaPH
1GZC4x1KFDwM/m3T81T+ntgQ2lJNyPuZZaO29s5mcppugdFUBF6yX1ISAWQ/ovQ=
=8LTg
-----END PGP SIGNATURE-----