On 6 November 2015 at 17:33, Leif Lindholm <[email protected]> wrote:
> On Fri, Nov 06, 2015 at 05:13:44PM +0100, Ard Biesheuvel wrote:
>> On 6 November 2015 at 17:02, Leif Lindholm <[email protected]> wrote:
>> > On Fri, Nov 06, 2015 at 03:03:53PM +0100, Ard Biesheuvel wrote:
>> >> The stride used by the cache maintenance by MVA instructions should
>> >> be retrieved from CTR_EL0.DminLine and CTR_EL0.IminLine, whose values
>> >> reflect the actual geometry of the caches. Using CCSIDR for this purpose
>> >> violates the architecture.
>> >
>> > Does it?
>> > Sure, you'd need to iterate over all levels of I/D/U cache and pick
>> > the smallest, but... If the current code doesn't, then it's broken (as
>> > opposed to violating the architecture).
>>
>> Yes, it does. The ARM ARM states quite clearly that the contents of
>> CCSIDR should not be used to make any inferences about the actual
>> cache geometry.
>
> It says it should not be used to infer actual sizes of caches, but is
> also says that it describes "architecturally visible parameters that
> are required for the cache maintenance by Set/Way instructions". (At
> least the v7 one.)
>
Indeed. The Set/Way fields of those cache maintenance instructions are
formatted according to these parameters, so the CCSIDR values need to
be in sync with what those instructions expect. It is thus legal to
expose 1 set, 1 way and a 16 byte linesize (i.e., all zeroes, as the
Denver CPU does afaik) while the actual geometry is completely
different.
> It's a note of warning to people to not try to infer performance
> characteristics based on these values.
>
No, it's a note saying 'look here for the values to poke into the DC
ISW/CSW/CISW instructions but don't use them for anything else'
In the kernel, we were using this information to calculate the waysize
and compare it to the PAGE_SIZE, to infer whether a VIPT I-cache could
alias or not, and the NVIDIA engineers proposed some patches to remove
it since the CCSIDR info is not suitable for doing that.
>> >> Also, move the line length accessors to common code, since there is no
>> >> need to keep them separate between ARMv7 and AArch64.
>> >
>> > There is for Arm9 though - the CTR format changed for ARMv7.
>> > So either we turn this back into an Arm9 purge as part of the cache
>> > maintenance fixes (which I would like to avoid) or I think we need to
>> > keep the per-arch line length accessors for now.
>>
>> Actually, you can't build the ARM9 version anymore, since ArmLib now
>> depends on <Chipset/ArmV7.h> whereas the ARM9 specific code includes
>> <Chipset/ARM926EJ-S.h> as well, resulting in a conflict. I don't think
>> this has been buildable since bd6b97994ab6 ("ArmPkg/ArmLib: Clean
>> ArmV7Lib") dated somewhere back in 2011
>
> Yeah :/
>
> Oh well, guess it's time for a purge.
>
Indeed.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
