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

Corey Osgood corey.osgood at gmail.com
Wed Dec 10 22:51:39 CET 2008


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. Per Segher's email I'd prefer to go with
Carl-Daniel's original suggestion of using 0x100000000.

Thanks,
Corey
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20081210/a51d45fb/attachment.html>


More information about the coreboot mailing list