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 <[EMAIL PROTECTED]> > > 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:[EMAIL PROTECTED] http://www.amd.com/embeddedprocessors -- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios
