[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