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

[Myles Watson]

On Wed, Dec 10, 2008 at 12:39 PM, 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?

I think your original works fine.  It was just confusing at first because of
the mix of addresses and sizes.
> +#if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE

+ CONFIG_CARSIZE))

If you write out the meaning in words it makes good sense.

If the last usable address before the ROM is less than the first usable
address after CAR.

Looks good to me.

Thanks,
Myles