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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Nov 29 23:57:26 CET 2007


On 29.11.2007 00:50, Marc Jones wrote:
>
> Carl-Daniel Hailfinger wrote:
>> On 28.11.2007 23:52, Marc Jones wrote:
>>>
>>> Carl-Daniel Hailfinger wrote:
>>>> Marc?
>>>> This has been sitting in my tree for a while now.
>>>>
>>>> On 16.11.2007 16:00, Carl-Daniel Hailfinger wrote:
>>>>> Hi,
>>>>>
>>>>> v2 and v3 have almost identical CAR setup code with identical bugs
>>>>> for
>>>>> CAR sizes != {4k,8k,16k}. In v3, the erroneous code paths are not
>>>>> triggered and the bug is latent, but we have at a few boards in v2
>>>>> which
>>>>> trigger these bugs, resulting in holes and/or smaller size of the
>>>>> CAR area.
>>>
>>> Sorry, I have been very busy and I have been putting this off.
>>>
>>> I think you are correct that CAR expects power of 2 cache sizes. How
>>> about just error if the size isn't power of 2 between 4K and 64K? If
>>> you wanted to support non power of 2 you should round up otherwise you
>>> might write off the end.
>>
>> OK, will prepare an updated patch.
>>
>> What about the bugs which cause 32k CAR to end up as 16k and 64k CAR to
>> have a hole between 16k and 32k?
>>
>
> I am not very familiar with the code but it looks like the size is
> growing from 0xCFFFF down to 0xC0000. I don't see a gap. The movl
> %eax, %edx make the entire MSR 0x0606060606060606 which would be 32K
> and then setting that in both 0x269 and 0x268 would be 64K. But I
> could be misunderstanding the code.

Quoting from my original mail:
> [...]
>   
>> > 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
>> > #endif
>>     
>
> OK, 64k are working.
>
>   
>> > #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
>>     
>
> OK, 32k are working.
>   

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.

> Disable CAR between 16k and 32k unconditionally. Even if CacheSize is
> 32k or 64k. Not nice.

Regards,
Carl-Daniel




More information about the coreboot mailing list