Re: [coreboot] Bug in your code

[Aaron Durbin]

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.

1. 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