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

[Carl-Daniel Hailfinger]

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.
>   

Feel free to use my Ack for that one.


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/