On 10/13/12 03:26, Robert Watson wrote:
On Fri, 12 Oct 2012, Carl Delsey wrote:
Indeed -- and on non-x86, where there are uncached direct map
segments, and TLB entries that disable caching, reading 2x 32-bit vs
1x 64-bit have quite different effects in terms of atomicity. Where
uncached I/Os are being used, those differences may affect semantics
significantly -- e.g., if your device has a 64-bit memory-mapped
FIFO or registers, 2x 32-bit gives you two halves of two different
64-bit values, rather than two halves of the same value. As device
drivers depend on those atomicity semantics, we should (at the
busspace level) offer only the exactly expected semantics, rather
than trying to patch things up. If a device driver accessing 64-bit
fields wants to support doing it using two 32-bit reads, it can
figure out how to splice it together following
bus_space_read_region_4().
I wouldn't make any default behaviour for bus_space_read_8 on i386,
just amd64. My assumption (which may be unjustified) is that by far
the most common implementations to read a 64-bit register on i386
would be to read the lower 4 bytes first, followed by the upper 4
bytes (or vice versa) and then stitch them together. I think we
should provide helper functions for these two cases, otherwise I fear
our code base will be littered with multiple independent
implementations of this.
Some driver writer who wants to take advantage of these helper
functions would do something like
#ifdef i386
#define bus_space_read_8 bus_space_read_8_lower_first
#endif
otherwise, using bus_space_read_8 won't compile for i386 builds.
If these implementations won't work for their case, they are free to
write their own implementation or take whatever action is necessary.
I guess my question is, are these cases common enough that it is
worth helping developers by providing functions that do the double
read and shifts for them, or do we leave them to deal with it on
their own at the risk of possibly some duplicated code.
I was thinking we might suggest to developers that they use a KPI that
specifically captures the underlying semantics, so it's clear they
understand them. Untested example:
uint64_t v;
/*
* On 32-bit systems, read the 64-bit statistic using two 32-bit
* reads.
*
* XXX: This will sometimes lead to a race.
*
* XXX: Gosh, I wonder if some word-swapping is needed in the merge?
*/
#ifdef 32-bit
bus_space_read_region_4(space, handle, offset, (uint32_t *)&v, 2;
#else
bus_space_read_8(space, handle, offset, &v);
#endif
The potential need to word swap, however, suggests that you may be
right about the error-prone nature of manual merging.
Again, I have to apologize for the delay in replying. I'm still dealing
with my family emergency some.
I really like Robert's idea of using bus_space_read_region_4 and
treating the 64 bit variable as an array of two 32 bit variables. That
eliminates the need to shift. I'll try to incorporate that idea if I can.
Anyhow, after talking to jimharris@ I figured the best thing to do was
to break this up into separate patches. This first patch is to provide
bus_space_xxxx_8 for amd64 only. This way we can get the easy case out
of the way. I'll follow up with a couple more patches: one to modify the
cxgbe driver and maybe a few other drivers to use this new function, and
then a second that makes a stab at how to deal with i386 if we think
that is worthwhile.
This first patch is at:
http://people.freebsd.org/~jimharris/patches/bus_space_xxx_8.patch
I basically copied the ia64 implementation to use as a base for this
change. I wasn't quite sure how best to deal with the copyrights. The
ia64 implementation has about 3 copyright headers on it, which seemed a
bit ridiculous to copy, but probably is necessary to be strictly legal.
I compromised and took the copyright from marcel@ since he was the last
to change the file. Do we have any best practices on how to deal with this?
Thanks for the feedback and thanks in advance for taking the time to
look at this patch.
Carl
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
