Re: [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
> > 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