[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 <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> On 10.12.2008 22:51, Corey Osgood wrote:
> > On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson <mylesgw at gmail.com> wrote:
> >
> >
> >> On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger <
> >> c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >>
> >>
> >>> On 10.12.2008 20:39, Corey Osgood wrote:
> >>>
> >>>> On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson <mylesgw at gmail.com>
> >>>>
> >>> wrote:
> >>>
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: coreboot-bounces at coreboot.org [mailto:
> >>>>>>
> >>>>>>
> >>>>> coreboot-bounces at coreboot.org]
> >>>>>
> >>>>>
> >>>>>> 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 *
> >>>>>>
> >>> 1024)
> >>>
> >>>>>>> 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
> >>>
> >>> --
> >>> http://www.hailfinger.org/
> >>>
> >>>
> >> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081210/b9f6e9db/attachment.html>