[coreboot] Bug in your code

[Aaron Durbin]

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