[CFR] mge driver / elf reloc

[Ian Lepore]

On Mon, 2014-07-21 at 11:16 -0600, Warner Losh wrote:
> On Jul 21, 2014, at 10:56 AM, John-Mark Gurney <jmg at 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 at 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 at 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 at 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
> 

I was trying to figure out how unaligned relocs even happen, and I think
one possibility would be something like this...

  char thing[] = "x";
  struct {
    char x;
    char *something;
 } __packed foo = {'a', thing};

Which seems contrived enough that I wonder how it happens in real life.

-- Ian