cvs commit: src/sys/dev/bce if_bcereg.h

[Scott Long]

Warner Losh wrote:
> From: Scott Long <scottl at samsco.org>
> Subject: Re: cvs commit: src/sys/dev/bce if_bcereg.h
> Date: Tue, 25 Apr 2006 13:43:58 -0600
> 
> 
>>John Baldwin wrote:
>>
>>>On Tuesday 25 April 2006 15:24, Scott Long wrote:
>>>
>>>
>>>>John Baldwin wrote:
>>>>
>>>>
>>>>>jhb         2006-04-25 19:18:48 UTC
>>>>>
>>>>> FreeBSD src repository
>>>>>
>>>>> Modified files:
>>>>>   sys/dev/bce          if_bcereg.h 
>>>>> Log:
>>>>> Fix half of the current i386 tinderbox failure.  max_bus_addr should be a
>>>>> bus_addr_t rather than a bus_size_t.
>>>>> 
>>>>> Revision  Changes    Path
>>>>> 1.2       +1 -1      src/sys/dev/bce/if_bcereg.h
>>>>
>>>>Actually, bus_size_t should also be aware of PAE.
>>>
>>>That may be true as well, I'll defer to your judgement on that one.  In
>>>this case bus_dma_tag_create()'s low_addr argument is supposed to be a
>>>bus_addr_t according to the man page. :)
>>>
>>
>>Both the lowaddr and highaddr arguments are bus_addr_t.  The more I 
>>think about it, the boundary argument should really be a bus_addr_t.
> 
> 
> The problem is that PAE's bus_size_t is a 32-bit quantity, when it
> should be a 64-bit quantity:
> 
> #ifdef PAE
> typedef uint64_t bus_addr_t;
> #else
> typedef uint32_t bus_addr_t;
> #endif
> typedef uint32_t bus_size_t;
> 
> For bus addresses, we should use bus_addr_t, of course, but the above
> is wrong.  I don't have a PAE machine, or I'd commit my local changes
> that fix this...
> 
> Warner

Ok, let's regroup.  The problem with if_bce that John fixed was that the
sc->max_bus_addr field was a bus_size_t, when it should have definitely 
been a bus_addr_t.  That change is fine.  I think that David is looking
for a clean way to handle his card's special case of needing to specify
0x10000000000 (2^40) as the lowaddr argument of the bus_dma_tag_create()
without the compiler whining on PAE vs non-PAE vs true 64-bit.  That's
mostly a style problem, and I'm offering to look at it and see if I can
make it any prettier.  Part of this also involves deciding on how to
define BUS_SPACE_MAXADDR on PAE.  Should it be 2^64-1, or should it be
2^36-1.  I'm inclined to say that latter.  However, that also opens up
a special case that busdma needs to handle.  Ideally David should be
passing 0x10000000000 as the lowaddr and BUS_SPACE_MAXADDR as the
highaddr, but that make the lowaddr > highaddr for PAE.  A minor
annoyance, but still one that needs to be validated.

The next problem is that the boundary argument of bus_dma_tag_create()
is a bus_size_t.  For all PCI Express devices, you need to be able to
stick a value of 0x100000000 (2^32) in here, have busdma do the right
thing with it, and not have the compiler complain.  I'm torn between
declaring that the boundary is actually an address and thus should be
declared as a bus_addr_t, and declaring that bus_size_t should be
64-bits on PAE just like it is on real 64-bit platforms.  The right
answer is probably to do both.  This means a core API change to busdma
and therefore to 90% of the hardware drivers in the tree, so it's not
easy to justify MFC'ing it.  It can be mostly worked around now anyways.

Does this sound accurate and/or reasonable?

Scott