Re: [coreboot] Bug in your code

[Aaron Durbin]

On Fri, Apr 18, 2014 at 11:08 AM, WANG FEI  wrote:
> The first option to change makemagic('B', 'S', 'S', ' ') of
> PAYLOAD_SEGMENT_BSS in util/cbfstool/cbfs.h would be a better solution.

It wasn't an either or proposition. The #2 piece is to fix big endian
machines that are inherently broken with the way the current code is
written. #1 definitely fixes your problem, though.
> On Fri, Apr 18, 2014 at 4:52 PM, Aaron Durbin  wrote:
> > On Fri, Apr 18, 2014 at 9:49 AM, ron minnich  wrote:
> > > Can somebody give me a sanity check? I can't see the error with the
> > > macro.
> > > I won't say too much here -- just take a look. I'm not convinced the
> > > code is wrong.
> >
> > http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/include/cbfs_c...
> >
> > shows
> >
> > #define PAYLOAD_SEGMENT_CODE   0x45444F43
> > #define PAYLOAD_SEGMENT_DATA   0x41544144
> > #define PAYLOAD_SEGMENT_BSS    0x20535342
> > #define PAYLOAD_SEGMENT_PARAMS 0x41524150
> > #define PAYLOAD_SEGMENT_ENTRY  0x52544E45
> >
> > and
> >
> > http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/lib/selfboot.c...
> >
> > does no endianness correction. The segment->type is taken verbatim
> > what is in cbfs.
> >
> > #define makemagic(b3, b2, b1, b0)\
> >         (((b3)<<24) | ((b2) << 16) | ((b1) << 8) | (b0))
> >
> > #define PAYLOAD_SEGMENT_CODE    makemagic('C', 'O', 'D', 'E')
> > #define PAYLOAD_SEGMENT_DATA    makemagic('D', 'A', 'T', 'A')
> > #define PAYLOAD_SEGMENT_BSS     makemagic(' ', 'B', 'S', 'S')
> > #define PAYLOAD_SEGMENT_PARAMS  makemagic('P', 'A', 'R', 'A')
> > #define PAYLOAD_SEGMENT_ENTRY   makemagic('E', 'N', 'T', 'R')
> >
> > I would see the following result for all host cbfstool compilations:
> >
> > 0x434F4445
> > 0x44415441
> > 0x20425353
> > 0x50415241
> > 0x454E5452
> >
> > But we xdr to big endian...  so one needs to make the
> > src/include/cbfs_core.h be architecture endian aware which it isn't.
> >
> > For the x86 case:
> >
> > makemagic('B', 'S', 'S', ' ') would make the xdr.be_put32 yield
> > 0x20535342 when a little endian machine read the 32-bit word from a
> > big endian serialization.
> >
> > The real question is what are the payload magic numbers and what
> > should the encoding be? If they are serialized as big endian then the
> > runtime needs an equivalent makemagic and be2host() to do the proper
> > comparison. Having the runtime code be compiled as a straight up
> > define is not correct for every machine because the underlying
> > endinanness could be different.
> >
> > I think people are getting hung up on strings when the code is dealing
> > w/ 32-bit values.
> > change makemagic('B', 'S', 'S', ' ') for PAYLOAD_SEGMENT_BSS that
> >
> > should fix little endian target machines
> > 2. fix the runtime in coreboot to match cbfsutil where it sucks out
> > the data as big endian and compares against the correct value.
> > > thanks
> > >
> > > ron
> > >
> > > On Fri, Apr 18, 2014 at 7:39 AM, WANG FEI 
> > > wrote:
> > > > Ronald,
> > > >
> > > > I just noticed a bug in your code, I've added the comment to coreboot
> > > > review
> > > > syste, but I'm not farmilar with this system, not sure if it will send
> > > > you a
> > > > notice mail automatically, so I just send you a mail to inform you
> > > > this.
> > > >
> > > > Here is the link of comment,
> > > > http://review.coreboot.org/#/c/5098/
> > > >
> > > > -Fei