Date: Mon, 21 Jul 2014 11:16:37 -0600 From: Warner Losh <imp@bsdimp.com> To: John-Mark Gurney <jmg@funkthat.com> Cc: Tim Kientzle <tim@kientzle.com>, freebsd-arm <freebsd-arm@freebsd.org>, Fabien Thomas <fabient@freebsd.org>, Ian Lepore <ian@freebsd.org>, arch@freebsd.org Subject: Re: [CFR] mge driver / elf reloc Message-ID: <B23A4513-FB0E-4972-91BA-976DCDF346E5@bsdimp.com> In-Reply-To: <20140721165619.GT45513@funkthat.com> References: <14D22EA6-B73C-47BA-9A86-A957D24F23B8@freebsd.org> <1405810447.85788.41.camel@revolution.hippie.lan> <20140720220514.GP45513@funkthat.com> <F6D53A17-FED0-4F08-BB5B-9F66C5AF5EF6@kientzle.com> <20140720231056.GQ45513@funkthat.com> <9464C309-B390-4A27-981A-E854921B1C98@bsdimp.com> <20140721162559.GS45513@funkthat.com> <467619B1-F530-49AF-91BF-14CA3A31908B@bsdimp.com> <20140721165619.GT45513@funkthat.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Jul 21, 2014, at 10:56 AM, John-Mark Gurney <jmg@funkthat.com> wrote: > Warner Losh wrote this message on Mon, Jul 21, 2014 at 10:31 -0600: >> >> On Jul 21, 2014, at 10:25 AM, John-Mark Gurney <jmg@funkthat.com> wrote: >> >>> Warner Losh wrote this message on Mon, Jul 21, 2014 at 08:46 -0600: >>>> >>>> On Jul 20, 2014, at 5:10 PM, John-Mark Gurney <jmg@funkthat.com> wrote: >>>> >>>>> Tim Kientzle wrote this message on Sun, Jul 20, 2014 at 15:25 -0700: >>>>>> >>>>>> On Jul 20, 2014, at 3:05 PM, John-Mark Gurney <jmg@funkthat.com> wrote: >>>>>> >>>>>>> Ian Lepore wrote this message on Sat, Jul 19, 2014 at 16:54 -0600: >>>>>>>> Sorry to take so long to reply to this, I'm trying to get caught up. I >>>>>>>> see you've already committed the mge fixes. I think the ELF alignment >>>>>>>> fix looks good and should also be committed. >>>>>>> >>>>>>> So, re the elf alignment... >>>>>>> >>>>>>> I think we should get a set of macros that handle load/stores to/from >>>>>>> unaligned addresses that are transparent to the caller.... I need >>>>>>> these for some other code I'm writing... >>>>>>> >>>>>>> I thought Open/Net had these available, but I can't seem to find them >>>>>>> right now... >>>>>> >>>>>> $ man 9 byteorder >>>>>> >>>>>> is most of what you want, lacking only some aliases to pick >>>>>> the correct macro for native byte order. >>>>> >>>>> Um, those doesn't help if you want native endian order? >>>> >>>> Ummm, yes they do. enc converts from native order. dec decodes to native byte >>> >>> No they don't.. If you want to read a value in memory that is native >>> endian order to native endian order (no conversion), they cannot be >>> used w/o using something like below? >> >> Missed the native to native. bcopy works, but is ugly, as you note below. >> >>>> order. They are more general cases than the ntoh* functions that are more traditional >>>> since they also work on byte streams that may not be completely aligned when >>>> sitting in memory. Which is what you are asking for. >>> >>> So, you're saying that I now need to write code like: >>> #if LITTLE_ENDIAN /* or how ever this is spelled*/ >>> var = le32enc(foo); >>> #else >>> var = be32enc(foo); >>> #endif >>> >>> If I want to read a arch native endian value? No thank you? >> >> I?m not saying that at all. >> >>>>> Also, only the enc/dec functions are documented to work on non-aligned >>>>> address, so that doesn't help in most cases? >>>> >>>> They work on all addresses. They are even documented to work on any address: >>>> >>>> The be16enc(), be16dec(), be32enc(), be32dec(), be64enc(), be64dec(), >>>> le16enc(), le16dec(), le32enc(), le32dec(), le64enc(), and le64dec() >>>> functions encode and decode integers to/from byte strings on any align- >>>> ment in big/little endian format. >>>> >>>> So they are quite useful in general. Peeking under the covers at the implementation >>>> also shows they will work for any alignment, so I?m having trouble understanding >>>> where this objection is really coming from. >>> >>> There are places where you write code such as: >>> int i; >>> memcpy(&i, inp, sizeof i); >>> /* use i */ >>> >>> In order to avoid alignment faults... None of the functions in byteorder >>> do NO conversion of endian, or you have to know which endian you are but >>> that doesn't work on MI code... >>> >>> Did you read what the commited code did? >> >> No, I missed that bit beaded on your reply (which seemed to imply you needed >> endian conversion) which implied the enc/dec are only documented to work on non-aligned >> which is what I was correcting. > > Hmm... It appears that byteorder(9) leaks to userland though isn't > documented to be available in userland... Should we fix this and make > it an offical userland API? How to document it? In my case, the > enc/dec version would have been enough for what I need, but not "part of > the userland API"... There is an xref from byteorder(3), but no > comment on either that they will work.. Yes. We should fix this now that it isn’t confined to the kernel. >> But maybe the more basic question is why do you even have packed >> structures that are native endian that you want to access as naturally >> aligned structures? How did they become native endian and why weren?t >> they converted to a more natural layout at that time? > > The original message said why… I personally think the original code should unconditionally call load_ptr() and store_ptr() and if the optimization for aligned access is actually worth doing, the test for it should be in those functions rather than inline throughout the code. The code will be clearer, and it would be easier to optimize those cases that actually matter. I’m frankly surprised that these relocations are being generated unaligned. Perhaps that’s the real bug here that should be fixed. While I’m OK with the original patch (subject to the above), I’d be curious what other cases there are for this functionality. You had said that you had additional use cases in the network stack, but I’m having trouble groking the use cases. If this is a huge deal, then defining functions to do this is trivial. I’m just not sure it is common enough to need a special macro/function call in the base. Warner [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJTzUr1AAoJEGwc0Sh9sBEAl6MP+QGfnLxyKTIOrOuCgJ8ps7dW SewJMNsTNebtICWMUBe+SWZ7NDnKEr6M7T61o9wpdckc3n7K2XtrPrppiUHmDF79 Y+nXzJWAMAsDxyJ9Y8axXupV1WsA0xqQ4+VnHWx5wLCAjVkfejw3O5CqF9ljMVr2 jnyge07Kdrfd31Jz/WguuHuBp9E32wv4UCvYWUweRDdwKUmMW2oLMtdbo1beD3pH RLV4lKDMrjpoaiTc9VkaRBT97e4AJFHZQYvUqfb6T0PGQQb3I3cCAir96WbOwpBP NQR/kTg4zRdwsjSos99bwyEkLu1k9JHduEdCMpfB+Y7itff1aybnqFMZzRlNT9X1 ILYpOvvQBlI9QtjbtiEQV/ABGQtLr+R1yMHYqMk5SafAvrR3yQ48FpYFIimnGd2d IIoDZuJxt/yTiM/sXsHhxSDx3qWyfSspcYwuGwdRWiMeG+XDrXBoeoiC48eGMuwG 9jbb3FLNa8cZ3eMJlQn2GEIBBK9qttVBbZxLWK09V/e+fqL0htUzA8XadLrCZUSR uUYoZsgGefPDfty/H51PPSCiOSYABdBkYuUuUZzAsWTp8SXbD7apbyK1TztgkJ6I 7mLv7R0s7I1gWH2qqmn9dwDuEW8Auhku85xTc0XMm06NyQ7RqRcx4f4mX0fxRmOI jpMqP2vyWDtX4xNOsfUD =QDdl -----END PGP SIGNATURE-----home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B23A4513-FB0E-4972-91BA-976DCDF346E5>