On 10.12.2008 22:51, Corey Osgood wrote: > On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson <[EMAIL PROTECTED]> wrote: > > >> On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger < >> [EMAIL PROTECTED]> wrote: >> >> >>> On 10.12.2008 20:39, Corey Osgood wrote: >>> >>>> On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson <[EMAIL PROTECTED]> >>>> >>> wrote: >>> >>>> >>>>>> -----Original Message----- >>>>>> From: [EMAIL PROTECTED] [mailto: >>>>>> >>>>>> >>>>> [EMAIL PROTECTED] >>>>> >>>>> >>>>>> 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/ -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
