[coreboot] [PATCH][v3] Check that CAR and ROM areas don't collide
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Dec 10 19:17:31 CET 2008
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 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.
> +#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 at gmx.net>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list