include/arch/x86/cpu.h is probably not the best place for this, but AFAIK it should be included by and work for all possible targets. Another possible solution is something like this, I don't think I like it as well, and it fails the KISS test IMO:
mainboard/Kconfig: config COREBOOT_ROMSIZE_KB_128 bool "128 KB" depends (0xffffffff - (128 * 1024) > (CONFIG_CARBASE + CONFIG_CARSIZE))) help Choose this option if you have a 128 KB ROM chip.
-Corey
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.
On 10.12.2008 17:53, Corey Osgood wrote:
include/arch/x86/cpu.h is probably not the best place for this, but AFAIK it should be included by and work for all possible targets. Another possible solution is something like this, I don't think I like it as well, and it fails the KISS test IMO:
mainboard/Kconfig: config COREBOOT_ROMSIZE_KB_128 bool "128 KB" depends (0xffffffff - (128 * 1024) > (CONFIG_CARBASE + CONFIG_CARSIZE))) help Choose this option if you have a 128 KB ROM chip.
Yes, the Kconfig test is ugly beyond belief.
Check that the CAR and ROM areas don't collide.
Signed-off-by: Corey Osgood corey.osgood@gmail.com
Index: include/arch/x86/cpu.h
--- include/arch/x86/cpu.h (revision 1066) +++ include/arch/x86/cpu.h (working copy) @@ -26,7 +26,15 @@ #include <device/device.h> #include <shared.h> #include <mtrr.h> +#include <config.h>
+/* Check that the CAR and ROM areas aren't going to collide */ +#if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE + CONFIG_CARSIZE))
Actually, that formula is off by one. A few equivalence transformations will show that:
#if (0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE + CONFIG_CARSIZE)
is equivalent to
#if 0xffffffff < CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB * 1024) is equivalent to #if CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB * 1024) > 0xffffffff
What you actually want is this test: #if CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB * 1024) > 0x100000000
+#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
#define X86_VENDOR_INTEL 0 #define X86_VENDOR_CYRIX 1 #define X86_VENDOR_AMD 2
If you fix the off-by-one, the patch is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@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
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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?
Thanks, Corey
On Wed, Dec 10, 2008 at 12:39 PM, Corey Osgood corey.osgood@gmail.comwrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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 *
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?
I think your original works fine. It was just confusing at first because of the mix of addresses and sizes.
+#if ((0xffffffff - (CONFIG_COREBOOT_ROMSIZE_KB * 1024)) < (CONFIG_CARBASE
+ CONFIG_CARSIZE))
If you write out the meaning in words it makes good sense.
If the last usable address before the ROM is less than the first usable address after CAR.
Looks good to me.
Thanks, Myles
On 10.12.2008 20:39, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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
On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 20:39, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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 *
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
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
On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 20:39, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com
wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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 *
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
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
On 10.12.2008 22:51, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 20:39, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com
wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:
coreboot-bounces@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 *
> 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
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
On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 22:51, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 4:46 PM, Myles Watson mylesgw@gmail.com wrote:
On Wed, Dec 10, 2008 at 2:27 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 20:39, Corey Osgood wrote:
On Wed, Dec 10, 2008 at 1:29 PM, Myles Watson mylesgw@gmail.com
wrote:
> -----Original Message----- > From: coreboot-bounces@coreboot.org [mailto: > > coreboot-bounces@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 * >
>> 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
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
On 10.12.2008 23:02, Myles Watson wrote:
On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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. ;-)
On 10.12.2008 22:51, Corey Osgood wrote:
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.
I hope to move CAR settings away from Kconfig. It was needed to store these settings in Kconfig when we had no processor-specific include files, but that's no longer the case. I'll send a patch.
Regards, Carl-Daniel
Patch committed in r1071, thanks.
On Wed, Dec 10, 2008 at 5:11 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
On 10.12.2008 23:02, Myles Watson wrote:
On Wed, Dec 10, 2008 at 2:57 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
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. ;-)
On 10.12.2008 22:51, Corey Osgood wrote:
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.
Yes and no, IMO. Yes in that, well, it gets caught earlier. No because there's no real way (that I know of...) to explain through Kconfig that the reason you can't set a larger ROM size is because of the CAR area, which could leave some users confused.
-Corey
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.
[There doesn't really exist anything called "cpp" anymore -- you mean to say "the C preprocessor"].
#if CONFIG_CARBASE + CONFIG_CARSIZE + (CONFIG_COREBOOT_ROMSIZE_KB * 1024) > 0x100000000
In C99, this is just fine. I'm not sure about C90; indeed, in C90 there is no requirement to support integer types of more than 32 bits at all!
You can always write "0x100000000LL" (or "ULL") instead, which isn't C90 either, but GCC supports it anyway.
What compiler mode is coreboot compiled in, anyway? I recommend we should use GNU99 mode (-std=gnu99), for v3 at least.
Segher
-
Carl-Daniel Hailfinger -
Corey Osgood -
Myles Watson -
Segher Boessenkool