Re: [coreboot] [PATCH][v3] Check that CAR and ROM areas don'tcollide

[Myles Watson]

On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> On 10.12.2008 22:51, Corey Osgood wrote:
> > On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson  wrote:
> > > On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger <
> > > [e-mail]> wrote:
> > > > On 10.12.2008 20:39, Corey Osgood wrote:
> > > > > On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson
> > > >
> > > > wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: [e-mail] [mailto:
> > > > > > >
> > > > > > >
> > > > > > [e-mail]]
> > > > > >
> > > > > > > On Behalf Of Carl-Daniel Hailfinger
> > > > > > > Sent: Wednesday, December 10, 2008 11:18 AM
> > > > > > > To: Corey Osgood
> > > > > > > Cc: Segher Boessenkool; coreboot
> > > > > > > Subject: Re: [coreboot] [PATCH][v3] Check that CAR and ROM areas
> > > > > > > don'tcollide
> > > > > > >
> > > > > > > Hi Segher,
> > > > > > >
> > > > > > > is the last test below with 0x100000000 (2^32) in the formula
> > > > > > >
> > > >
> > > > guaranteed
> > > > > > > to work or may cpp truncate the results to 32 bit? I'd rather avoid
> > > > > > > introducing a test that can never trigger.
> > > > > > >
> > > > > > >
> > > > > > ...snip...
> > > > > >
> > > > > > > What you actually want is this test:
> > > > > > > #if CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB *
> > > > > > >
> > > > > > >> 0x100000000
> > > > > > >>
> > > > > > >>
> > > > > > To avoid that problem, maybe we should /1024 instead of *.
> > > > > > #if CONFIG_CARBASE/1024 + CONFIG_CARSIZE/1024 +
> > > >
> > > > CONFIG_COREBOOT_ROMSIZE_KB
> > > > > > 1<<22
> > > > > >
> > > > > > I realize that 1<<22 isn't pretty, but the rest doesn't seem too bad.
> > > > > >
> > > > > > Thanks,
> > > > > > Myles
> > > > >
> > > > > I'm probably missing something, but I'm not seeing the off-by-one
>
> error
> > > > >
> > > >
> > > > in
> > > > > the original function. CONFIG_CARBASE and 0xffffffff are both
>
> addresses,
> > > > >
> > > >
> > > > you
> > > > > could say they're both off by one, but by comparing them the problem
>
> is
> > > > > negated. You could rearrange it to
> > > > >
> > > > > 0xffffffff - (CONFIG_CARBASE + CONFIG_CARSIZE) <
> > > >
> > > > CONFIG_COREBOOT_ROMSIZE_KB
> > > > > 1024
> > > > >
> > > > > which should, AFAIK, compare the remaining space with the ROM size.
> > > >
> > > > Right?
> > > >
> > > > Let me demonstrate the off-by-one error for you with your original
>
> code:
> > > > #define CONFIG_CARBASE 0xffef0000
> > > > #define CONFIG_CARSIZE 0x00010000
> > > > #define CONFIG_COREBOOT_ROMSIZE_KB 1024
> > > > #if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) <
>
> (CONFIG_CARBASE
> > > > CONFIG_CARSIZE))
> > > >
> > > > #error Your current Cache-As-Ram base does not allow room to map the
> > > > selected\
> > > >  chip size to memory. Please select a different chip size or move the
>
> CAR\
> > > > base to another sane location.
> > > > #endif
> > > >
> > > > Try to compile that snippet. It will error out although the
> > > > configuration is valid.
> > > > CAR is from 0xffef0000-0xffefffff (size 0x10000)
> > > > ROM is from 0xfff00000-0xffffffff (size 0x100000)
> > > >
> > > > Regards,
> > > > Carl-Daniel
> > >
> > > I agree with Carl-Daniel.
> > >
> > > How about this one:
> > > #if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) <
>
> (CONFIG_CARBASE +
> > > CONFIG_CARSIZE-1))
> > > if the last useable address before ROM < last CAR address (instead of
>
> next
> > > useable address after CAR)
> > >
> > > Thanks,
> > > Myles
> >
> > Carl-Daniel, thanks, I see it now.
>
> You're welcome. I admit that off-by-one errors are pretty hard to spot
> and your code really looked correct at the first glance.
> We have an old saying at my university about such bugs: "Programmers are
> either off by one or by a factor of two." It happened to me often
> enough. ;-)
> > Per Segher's email I'd prefer to go with
> > Carl-Daniel's original suggestion of using 0x100000000.

Sounds good.

Even though the Kconfig solution proposed was ugly, could we think of a
different one that would still live in Kconfig.  It seems a lot nicer to
catch it during configuration than during the build.

Thanks,
Myles