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

[Myles Watson]

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