On Fri, Apr 18, 2014 at 11:08 AM, WANG FEI wangfei.jimei@gmail.com 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 adurbin@chromium.org wrote:
On Fri, Apr 18, 2014 at 9:49 AM, ron minnich rminnich@gmail.com 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 wangfei.jimei@gmail.com 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
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot