<br><br><div class="gmail_quote">On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="Wj3C7c">On 10.12.2008 22:51, Corey Osgood wrote:<br>
> On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson <<a href="mailto:mylesgw@gmail.com">mylesgw@gmail.com</a>> wrote:<br>
><br>
><br>
>> On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger <<br>
>> <a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>> wrote:<br>
>><br>
>><br>
>>> On 10.12.2008 20:39, Corey Osgood wrote:<br>
>>><br>
>>>> On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson <<a href="mailto:mylesgw@gmail.com">mylesgw@gmail.com</a>><br>
>>>><br>
>>> wrote:<br>
>>><br>
>>>><br>
>>>>>> -----Original Message-----<br>
>>>>>> From: <a href="mailto:coreboot-bounces@coreboot.org">coreboot-bounces@coreboot.org</a> [mailto:<br>
>>>>>><br>
>>>>>><br>
>>>>> <a href="mailto:coreboot-bounces@coreboot.org">coreboot-bounces@coreboot.org</a>]<br>
>>>>><br>
>>>>><br>
>>>>>> On Behalf Of Carl-Daniel Hailfinger<br>
>>>>>> Sent: Wednesday, December 10, 2008 11:18 AM<br>
>>>>>> To: Corey Osgood<br>
>>>>>> Cc: Segher Boessenkool; coreboot<br>
>>>>>> Subject: Re: [coreboot] [PATCH][v3] Check that CAR and ROM areas<br>
>>>>>> don'tcollide<br>
>>>>>><br>
>>>>>> Hi Segher,<br>
>>>>>><br>
>>>>>> is the last test below with 0x100000000 (2^32) in the formula<br>
>>>>>><br>
>>> guaranteed<br>
>>><br>
>>>>>> to work or may cpp truncate the results to 32 bit? I'd rather avoid<br>
>>>>>> introducing a test that can never trigger.<br>
>>>>>><br>
>>>>>><br>
>>>>> ...snip...<br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>>> What you actually want is this test:<br>
>>>>>> #if CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB *<br>
>>>>>><br>
>>> 1024)<br>
>>><br>
>>>>>>> 0x100000000<br>
>>>>>>><br>
>>>>>>><br>
>>>>> To avoid that problem, maybe we should /1024 instead of *.<br>
>>>>> #if CONFIG_CARBASE/1024 + CONFIG_CARSIZE/1024 +<br>
>>>>><br>
>>> CONFIG_COREBOOT_ROMSIZE_KB<br>
>>><br>
>>>>> 1<<22<br>
>>>>><br>
>>>>> I realize that 1<<22 isn't pretty, but the rest doesn't seem too bad.<br>
>>>>><br>
>>>>> Thanks,<br>
>>>>> Myles<br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>> I'm probably missing something, but I'm not seeing the off-by-one error<br>
>>>><br>
>>> in<br>
>>><br>
>>>> the original function. CONFIG_CARBASE and 0xffffffff are both addresses,<br>
>>>><br>
>>> you<br>
>>><br>
>>>> could say they're both off by one, but by comparing them the problem is<br>
>>>> negated. You could rearrange it to<br>
>>>><br>
>>>> 0xffffffff - (CONFIG_CARBASE + CONFIG_CARSIZE) <<br>
>>>><br>
>>> CONFIG_COREBOOT_ROMSIZE_KB<br>
>>><br>
>>>> * 1024<br>
>>>><br>
>>>> which should, AFAIK, compare the remaining space with the ROM size.<br>
>>>><br>
>>> Right?<br>
>>><br>
>>> Let me demonstrate the off-by-one error for you with your original code:<br>
>>><br>
>>> #define CONFIG_CARBASE 0xffef0000<br>
>>> #define CONFIG_CARSIZE 0x00010000<br>
>>> #define CONFIG_COREBOOT_ROMSIZE_KB 1024<br>
>>> #if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE<br>
>>> + CONFIG_CARSIZE))<br>
>>> #error Your current Cache-As-Ram base does not allow room to map the<br>
>>> selected\<br>
>>>  chip size to memory. Please select a different chip size or move the CAR\<br>
>>>  base to another sane location.<br>
>>> #endif<br>
>>><br>
>>><br>
>>> Try to compile that snippet. It will error out although the<br>
>>> configuration is valid.<br>
>>> CAR is from 0xffef0000-0xffefffff (size 0x10000)<br>
>>> ROM is from 0xfff00000-0xffffffff (size 0x100000)<br>
>>><br>
>>> Regards,<br>
>>> Carl-Daniel<br>
>>><br>
>>> --<br>
>>> <a href="http://www.hailfinger.org/" target="_blank">http://www.hailfinger.org/</a><br>
>>><br>
>>><br>
>> I agree with Carl-Daniel.<br>
>><br>
>> How about this one:<br>
>> #if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE +<br>
>> CONFIG_CARSIZE-1))<br>
>> if the last useable address before ROM < last CAR address (instead of next<br>
>> useable address after CAR)<br>
>><br>
>> Thanks,<br>
>> Myles<br>
>><br>
>><br>
><br>
> Carl-Daniel, thanks, I see it now.<br>
<br>
</div></div>You're welcome. I admit that off-by-one errors are pretty hard to spot<br>
and your code really looked correct at the first glance.<br>
We have an old saying at my university about such bugs: "Programmers are<br>
either off by one or by a factor of two." It happened to me often<br>
enough. ;-)<br>
<div class="Ih2E3d"><br>
<br>
> Per Segher's email I'd prefer to go with<br>
> Carl-Daniel's original suggestion of using 0x100000000.<br></div></blockquote><div><br>Sounds good. <br></div><div><br><br>Even though the Kconfig solution proposed was ugly, could we think of a different one that would still live in Kconfig.  It seems a lot nicer to catch it during configuration than during the build.<br>
<br>Thanks,<br>Myles <br></div></div><br>