[coreboot] Bug in your code
adurbin at chromium.org
Fri Apr 18 18:14:14 CEST 2014
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.
>> #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
>> 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:
>> 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
More information about the coreboot