[coreboot] [PATCH][v3] Check that CAR and ROM areas don'tcollide
mylesgw at gmail.com
Wed Dec 10 21:42:12 CET 2008
On Wed, Dec 10, 2008 at 12:39 PM, Corey Osgood <corey.osgood at gmail.com>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.
>> > 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
>> I realize that 1<<22 isn't pretty, but the rest doesn't seem too bad.
> 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
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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the coreboot