Re: [LinuxBIOS] [PATCH] rewrite generic x86 CAR code

[Carl-Daniel Hailfinger]

On 08.01.2008 01:40, Marc Jones wrote:
> Carl-Daniel Hailfinger wrote:
> > Marc?
> >
> > While the patch is against the generic x86 CAR code, it can also be
> > easily modified to work with AMD K8/K9/K10 CAR code. Especially the
> > recent K10 commit made AMD CAR an #ifdef mess which could be sorted out
> > nicely.
>
> Yes, It would be good to clean them all up.

OK, will prepare a patch to do so after we have finalized the generic
x86 CAR patch.
> > This patch is part of my quest to clean up those v2 code parts which
> > will someday end up in v3.
>
> A good cause :)

Thanks.
> > > -#if CacheSize == 0x10000 -        /* enable caching for 64K using
> > > fixed mtrr */
> > > +/* 0x06 is the WB IO type for a given 4k segment.
> > > segs is the number of 4k segments we want to use for CAR.
> > > subpart is the nth 32-bit window into IO type configuration.
> > > reg is the register where the IO type should be stored.
> > > */
> > >
> > > +.macro extractmask segs, subpart, reg
> > > +.if \segs - (\subpart * 4) <= 0
> > > xorl \reg, \reg
> > >
> > > +.elseif \segs - (\subpart * 4) == 1
> > > movl $0x06000000, \reg
> > >
> > > +.elseif \segs - (\subpart * 4) == 2
> > > movl $0x06060000, \reg
> > >
> > > +.elseif \segs - (\subpart * 4) == 3
> > > movl $0x06060600, \reg
> > >
> > > +.elseif \segs - (\subpart * 4) >= 4
> > > movl $0x06060606, \reg
> > >
> > > +.endif
> > > +.endm
>
> Why not just pass in the number of 4k pieces, \segs - (\subpart * 4)?

To simplify understanding the formula for readers of the code. But I
agree moving calculations makes the code more readable in the function
above.
> > > +.macro simplemask_helper segs, subpart
> > > +.if \subpart & 0x1
> > > extractmask \segs, \subpart, %eax
> > >
> > > +.else
> > > extractmask \segs, \subpart, %edx
> > >
> > > +.endif
> > > +.endm
> > > +/* size is the cache size in bytes we want to use for CAR.
> > > part is the nth 64-bit window into IO type configuration.
> > > */
> > >
> > > +.macro simplemask size, part
> > > simplemask_helper (\size / 0x1000), (\part * 2 + 1)
> > > simplemask_helper (\size / 0x1000), (\part * 2)
> > >
> > > +.endm
>
> The \part stuff isn't really intuitive. I think an .if size > 32K
> would be better and then the caller doesn't have to know \part.
> Building on that the macro could fill in ecx as well.

I'll rewrite the patch a bit to make it more intuitive. I disagree with
the ".if size > 32k" part, though. But once you see my new code, it may
be elegant enough to not worry about the 32k boundary anymore.

Regards,
Carl-Daniel