[coreboot] [PATCH][v3] Check that CAR and ROM areas don't collide
c-d.hailfinger.devel.2006 at gmx.net
Wed Dec 10 19:17:31 CET 2008
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:
> config COREBOOT_ROMSIZE_KB_128
> bool "128 KB"
> depends (0xffffffff - (128 * 1024) > (CONFIG_CARBASE + CONFIG_CARSIZE)))
> 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 at 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.
> #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 at gmx.net>
More information about the coreboot