From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Sat, 1 Sep 2012 00:16:43 +0200 Subject: [U-Boot] [PATCH] usb: do explicit unaligned accesses In-Reply-To: <20120831222008.3665fecb@lilith> References: <1346368414-993-1-git-send-email-dev@lynxeye.de> <201208311815.30549.marex@denx.de> <20120831222008.3665fecb@lilith> Message-ID: <201209010016.43563.marex@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Albert ARIBAUD, > Hi Marek, > > On Fri, 31 Aug 2012 18:15:30 +0200, Marek Vasut wrote: > > Dear Albert ARIBAUD, > > > > > Hi Marek, > > > > > > On Fri, 31 Aug 2012 01:29:05 +0200, Marek Vasut > > > > > > wrote: > > > > Dear Lucas Stach, > > > > > > > > > usb_hub_descriptor has to be packed as it's used for > > > > > communication with the device. Member wHubCharacteristics > > > > > violates the natural alignment rules. > > > > > > > > > > Use explicit unaligned access functions for this member. > > > > > Fixes ARMv7 traping while using USB. > > > > > > > > Shouldn't a properly configured compiler prevent such behavior? > > > > > > This has been discussed before. As a summary: > > Argh, I must have missed this. > > > > > 1. A properly configured compiler can pad a structure so that the > > > fields always start at an aligned address (assuming the structure > > > base address is itself aligned). But that alters the structure, and > > > here there must be no alterations to the structure. > > > > There can be ... the compiler can even reorder the structures (that > > happened in Linux kernel, as I'm on a train now with shitty internet > > connection, I can't find it ... search LWN for that please). > > As a compiler bug, this can happen indeed. And we had our share of > compiler alignment bugs ourselves. But a conforming compiler should not > reorder fields. I'm no compiler expert ... so I won't argue here > > > 2. A properly (and differently) configured compiler can > > > automatically generate native unaligned accesses to unaligned > > > fields. This is acceptable on armv6+ architectures, has a > > > performance penalty on earlier architectures, and does not > > > necessarily work depending on the hardware configuration. > > > > Certainly, agreed. > > > > > 3. A properly (and differently yet) configured compiler can > > > automatically generate non-native unaligned accesses to unaligned > > > fields. This is acceptable on all architectures, has a performance > > > penalty on pre-armV6 architectures for all misaligned accesses, > > > whether voluntary or not. > > > > Correct, I'd vote for this solution -- let the compiler handle such > > unaligned cases. > > > > > The conclusion of the discussion is as follows -- or to be more > > > exact, following this discussion, this is my stance as the U-Boot > > > > custodian: > Someone please hold Wolfgang back while I correct myself: "as the *ARM* > U-Boot custodian..." > > :) %^) > > > i) All the code intended to run 'close to' U-Boot (i.e., U-Boot code > > > itself and application code) is controlled enough that we should be > > > able to know and limit which code requires misaligned access (such > > > as here for this USB structure field); > > > > > > ii) On some ARM architectures, and possibly some non-ARM > > > architectures as well, native misaligned access incur a performance > > > hit, and may even simply be impossible or forbidden by a hardware > > > or system design decision. > > > > > > iii) Thus, U-Boot should follow a strict policy of using native > > > aligned accesses only, possibly enforcing misaligned native access > > > prevention in hardware, and of explicitly emulating misaligned > > > accesses when they cannot be avoided. > > > > I do agree with i) and ii), but why not just let compiler handle the > > unaligned access for us? The compiler can optimize across the whole > > code, not only locally over one access, therefore it might be able to > > punt some of the unaligned accesses altogether if the code allows it. > > I think you are talking about lumping small-sized accesses together > into a bigger access possibly aligned. This is exactly what I mean. > If I am correct, then I don't > think this is related to misaligned accesses. Why won't it be? Such access can in the end turn out to be aligned, therefore leveraging all the penalty. > If I am not correct, can > you please detail what you meant? > > > Besides, right now, the code is much more readable. So I really don't > > like adding some strange macros to force crazy aligned access if the > > compiler can do it for us and can do it better. > > I personally would let the compiler do it too, but I prefer it to be > clearly indicated to the reader of the code when an access is > known to be misaligned. I'd enable enable the Alignment trapping in the CPU and die on an unaligned access at runtime -- to indicate the user that he should fix his bloody compiler. > > $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- ./MAKEALL m28evk > > Configuring for m28evk board... > > > > text data bss dec hex filename > > > > 415964 7688 288740 712392 adec8 ./u-boot > > > > 11754 788 12 12554 310a ./spl/u-boot-spl > > > > --------------------- SUMMARY ---------------------------- > > Boards compiled: 1 > > ---------------------------------------------------------- > > $ patch -Np1 -i /tmp/\[PATCH\ v2\]\ usb_do\ explicit\ unaligned\ > > accesses.mbox patching file common/usb_hub.c > > patching file drivers/usb/host/ehci-hcd.c > > Hunk #2 succeeded at 867 with fuzz 1 (offset -10 lines). > > $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- ./MAKEALL m28evk > > Configuring for m28evk board... > > > > text data bss dec hex filename > > > > 415968 7688 288736 712392 adec8 ./u-boot > > > > 11754 788 12 12554 310a ./spl/u-boot-spl > > > > --------------------- SUMMARY ---------------------------- > > Boards compiled: 1 > > ---------------------------------------------------------- > > > > Notice the text section grew a bit too, I dunno why, does anyone care > > enough to clarify please? > > > > > Hope this clarifies. > > > > > > Amicalement, > > > > Best regards, > > Marek Vasut > > Amicalement, Best regards, Marek Vasut