[LinuxBIOS] [PATCH v3] v2, v3: Bug in CAR setup with CacheSize!={4k, 8k, 16k}

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 29.11.2007 23:58, Marc Jones wrote:
>> Carl-Daniel Hailfinger wrote:
>>> Everything is set up correctly until now.
>>>
>>>>>>         /* enable caching for 16K/8K/4K using fixed mtrr */
>>>>>>         movl    $0x269, %ecx  /* fix4k_cc000*/
>>>>>> #if CacheSize == 0x4000
>>>>>>         movl    $0x06060606, %edx /* WB IO type */
>>>>>> #endif
>>>>>> #if CacheSize == 0x2000
>>>>>>         movl    $0x06060000, %edx /* WB IO type */
>>>>>> #endif
>>>>>> #if CacheSize == 0x1000
>>>>>>         movl    $0x06000000, %edx /* WB IO type */
>>>>>> #endif
>>>>>>     xorl    %eax, %eax
>>>>>>     wrmsr  
>>> This is where the bug is. I'm speaking of the two commands above,
>>> executed unconditionally. %ecx is 0x269, %eax is zeroed, %edx keeps its
>>> value ($0x06060606 in case of CacheSize>=32k). wrmsr is issued. Is there
>>> any reason to assume that this will not disable CAR again between 16k
>>> and 32k? And no, that code is not protected by an #ifdef.
>> Ah! You are right. This is a bad bug.
> 
> Fix multiple bugs in CAR setup for non-Geode x86. These bugs have been
> sitting there for years...
> The first bug unconditionally disabled CAR between 16k and 32k, even if
> 32k or 64k CAR size was specified.
> The second bug completely disabled CAR if a non-power-of-2-size or a
> size above 64k or below 4k was specified.
> Restructure and shrink the code a bit and fail the build if unsupported
> CAR sizes are requested.
> 
> Found by thorough reading through the code.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 
> Index: LinuxBIOSv3-CAR/arch/x86/stage0_i586.S
> ===================================================================
> --- LinuxBIOSv3-CAR/arch/x86/stage0_i586.S	(Revision 532)
> +++ LinuxBIOSv3-CAR/arch/x86/stage0_i586.S	(Arbeitskopie)
> @@ -301,37 +301,44 @@
>          jmp     clear_fixed_var_mtrr
>  clear_fixed_var_mtrr_out:
>  
> -#if CacheSize == 0x10000 
> -        /* enable caching for 64K using fixed mtrr */
> -        movl    $0x268, %ecx  /* fix4k_c0000*/
> -        movl    $0x06060606, %eax /* WB IO type */
> -	movl    %eax, %edx
> -        wrmsr
> -	movl	$0x269, %ecx
> -	wrmsr
> +#if (CacheSize & (CacheSize - 1))
> +#error Invalid CAR size, is not a power of two! This is a code limitation.
>  #endif
> +#if CacheSize > 0x10000
> +#error Invalid CAR size, must be at most 64k.
> +#endif
> +#if CacheSize < 0x1000
> +#error Invalid CAR size, must be at least 4k. This is a processor limitation.
> +#endif
>  
> -#if CacheSize == 0x8000
> +	/* We round down CAR size to the next power of 2 */


Fix comment. No longer rounding.

> +        movl    $0x269, %ecx  /* fix4k_c8000*/
> +
> +#if CacheSize >= 0x8000

Since you added the check above and there is no rounding, all the 
CachSize #ifs can == .

I will get setup and test this on v2 tomorrow.

Marc


-- 
Marc Jones
Senior Firmware Engineer
(970) 226-9684 Office
mailto:Marc.Jones at amd.com
http://www.amd.com/embeddedprocessors