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

[Carl-Daniel Hailfinger]

On 30.11.2007 01:19, Marc Jones wrote:
> 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.
>   

OK.
>   
>> +        movl    $0x269, %ecx  /* fix4k_c8000*/
>> +
>> +#if CacheSize >= 0x8000
>>     
>
> Since you added the check above and there is no rounding, all the 
> CachSize #ifs can == .
>   

OK.
However, I have code pending which enables compile-time CacheSize
granularity of 4K. It depends on the preprocessor quite a bit, but it
will work fine.
If there is a way to retrieve the maximum supported CacheSize of the
processor at runtime, even better: I have code which will dynamically
set up CAR to exactly that size.

> I will get setup and test this on v2 tomorrow.
>   

Thanks!

Regards,
Carl-Daniel