Date: Tue, 25 Apr 2006 17:54:09 -0600 From: Scott Long <scottl@samsco.org> To: Warner Losh <imp@bsdimp.com> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org, jhb@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/bce if_bcereg.h Message-ID: <444EB6A1.3060901@samsco.org> In-Reply-To: <20060425.173236.74726638.imp@bsdimp.com> References: <444E7750.206@samsco.org> <200604251540.00170.jhb@freebsd.org> <444E7BFE.4040800@samsco.org> <20060425.173236.74726638.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Warner Losh wrote: > From: Scott Long <scottl@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?444EB6A1.3060901>