On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger < [EMAIL PROTECTED]> wrote:
> 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. > Sounds good. 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. Thanks, Myles
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
