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. Per Segher's email I'd prefer to go with Carl-Daniel's original suggestion of using 0x100000000. Thanks, Corey
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
