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

[Carl-Daniel Hailfinger]

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 */
+        movl    $0x269, %ecx  /* fix4k_c8000*/
+
+#if CacheSize >= 0x8000
         /* enable caching for 32K using fixed mtrr */
-        movl    $0x269, %ecx  /* fix4k_c8000*/
         movl    $0x06060606, %eax /* WB IO type */
 	movl    %eax, %edx
 	wrmsr
 #endif
 
+#if CacheSize >= 0x10000 
+        /* enable caching for 32K-64K using fixed mtrr */
+        movl    $0x268, %ecx  /* fix4k_c0000*/
+        wrmsr
+#endif
+
+#if CacheSize < 0x10000
         /* enable caching for 16K/8K/4K using fixed mtrr */
-        movl    $0x269, %ecx  /* fix4k_cc000*/
-#if CacheSize == 0x4000
+#if CacheSize >= 0x4000
         movl    $0x06060606, %edx /* WB IO type */
-#endif
-#if CacheSize == 0x2000
+#elif CacheSize >= 0x2000
         movl    $0x06060000, %edx /* WB IO type */
-#endif
-#if CacheSize == 0x1000
+#elif CacheSize >= 0x1000
         movl    $0x06000000, %edx /* WB IO type */
 #endif
 	xorl    %eax, %eax
 	wrmsr
+#endif
 
 #if defined(CONFIG_XIP_ROM_SIZE) && defined(CONFIG_XIP_ROM_BASE)
         /* enable write base caching so we can do execute in place