On Fri, Apr 18, 2014 at 11:08 AM, WANG FEI <[email protected]> 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 <[email protected]> wrote: >> >> On Fri, Apr 18, 2014 at 9:49 AM, ron minnich <[email protected]> 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_core.h;h=a1d8127de20d997359cb86757dc46345ac14a88c;hb=refs/heads/master >> >> 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;h=feff03e14385f85f0606e83359ae2b17ca52ea51;hb=refs/heads/master >> >> 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 <[email protected]> >> > 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: [email protected] >> > http://www.coreboot.org/mailman/listinfo/coreboot > > -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
