The first option to change makemagic('B', 'S', 'S', ' ') of
PAYLOAD_SEGMENT_BSS in util/cbfstool/cbfs.h would be a better solution.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
