[coreboot] Bug in your code

[WANG FEI]

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 <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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20140418/9e7821c0/attachment.html>