On 11 May 2016 at 11:35, Achin Gupta <achin.gu...@arm.com> wrote:
> Hi Ard,
>
> Some comments inline!
>
> On Wed, May 11, 2016 at 10:41:57AM +0200, Ard Biesheuvel wrote:
>> Instead of cleaning the data cache to the PoU by virtual address and
>> subsequently invalidating the entire I-cache, invalidate only the
>> range that we just cleaned. This way, we don't invalidate other
>> cachelines unnecessarily.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> ArmPkg/Include/Library/ArmLib.h | 10
>> ++++++++--
>> ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 3 ++-
>> ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 5 +++++
>> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S | 5 +++++
>> ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm | 6 ++++++
>> 5 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/ArmPkg/Include/Library/ArmLib.h
>> b/ArmPkg/Include/Library/ArmLib.h
>> index 1689f0072db6..4608b0cccccc 100644
>> --- a/ArmPkg/Include/Library/ArmLib.h
>> +++ b/ArmPkg/Include/Library/ArmLib.h
>> @@ -183,13 +183,19 @@ ArmInvalidateDataCacheEntryByMVA (
>>
>> VOID
>> EFIAPI
>> -ArmCleanDataCacheEntryToPoUByMVA(
>> +ArmCleanDataCacheEntryToPoUByMVA (
>> IN UINTN Address
>> );
>>
>> VOID
>> EFIAPI
>> -ArmCleanDataCacheEntryByMVA(
>> +ArmInvalidateInstructionCacheEntryToPoUByMVA (
>> + IN UINTN Address
>> + );
>> +
>> +VOID
>> +EFIAPI
>> +ArmCleanDataCacheEntryByMVA (
>> IN UINTN Address
>> );
>>
>> diff --git a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
>> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
>> index 1045f9068f4d..cc7555061428 100644
>> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
>> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c
>> @@ -65,7 +65,8 @@ InvalidateInstructionCacheRange (
>> )
>> {
>> CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryToPoUByMVA);
>> - ArmInvalidateInstructionCache ();
>> + CacheRangeOperation (Address, Length,
>> + ArmInvalidateInstructionCacheEntryToPoUByMVA);
>
> Afaics, CacheRangeOperation() uses the data cache line length by default. This
> will match the I$ line length in most cases. But, it would be better to use
> the
> ArmInstructionCacheLineLength() function instead. I suggest that we add a
> cache
> line length parameter to CacheRangeOperation() to allow the distinction.
>
Good point.
> Also, from my reading of the ARMv8 ARM B2.6.5 & E2.6.5, we need an ISB at the
> end of all the operations.
>
I would assume that a single call to
ArmInstructionSynchronizationBarrier() at the end of
InvalidateInstructionCacheRange () should suffice?
>> return Address;
>> }
>>
>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
>> b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
>> index 43f7a795acec..9441f47e30ba 100644
>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
>> @@ -23,6 +23,7 @@ GCC_ASM_EXPORT (ArmInvalidateInstructionCache)
>> GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA)
>> GCC_ASM_EXPORT (ArmCleanDataCacheEntryByMVA)
>> GCC_ASM_EXPORT (ArmCleanDataCacheEntryToPoUByMVA)
>> +GCC_ASM_EXPORT (ArmInvalidateInstructionCacheEntryToPoUByMVA)
>> GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryByMVA)
>> GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryBySetWay)
>> GCC_ASM_EXPORT (ArmCleanDataCacheEntryBySetWay)
>> @@ -80,6 +81,10 @@ ASM_PFX(ArmCleanDataCacheEntryToPoUByMVA):
>> dc cvau, x0 // Clean single data cache line to PoU
>> ret
>>
>> +ASM_PFX(ArmInvalidateInstructionCacheEntryToPoUByMVA):
>> + ic ivau, x0 // Invalidate single instruction cache line to PoU
>> + ret
>> +
>>
>> ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA):
>> dc civac, x0 // Clean and invalidate single data cache line
>> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
>> b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
>> index 50c760f335de..c765032c9e4d 100644
>> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
>> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.S
>> @@ -18,6 +18,7 @@
>>
>> GCC_ASM_EXPORT (ArmInvalidateInstructionCache)
>> GCC_ASM_EXPORT (ArmInvalidateDataCacheEntryByMVA)
>> +GCC_ASM_EXPORT (ArmInvalidateInstructionCacheEntryToPoUByMVA)
>> GCC_ASM_EXPORT (ArmCleanDataCacheEntryByMVA)
>> GCC_ASM_EXPORT (ArmCleanDataCacheEntryToPoUByMVA)
>> GCC_ASM_EXPORT (ArmCleanInvalidateDataCacheEntryByMVA)
>> @@ -74,6 +75,10 @@ ASM_PFX(ArmCleanDataCacheEntryToPoUByMVA):
>> mcr p15, 0, r0, c7, c11, 1 @clean single data cache line to PoU
>> bx lr
>>
>> +ASM_PFX(ArmInvalidateInstructionCacheEntryToPoUByMVA):
>> + mcr p15, 0, r0, c7, c5, 1 @Invalidate single instruction cache line
>> to PoU
>> + mcr p15, 0, r0, c7, c5, 7 @Invalidate branch predictor
>
> Is it reasonable to assume that for every Icache invalidation by MVA, the BP
> will have to be invalidated for that MVA as well? I guess so!
>
I don't see how we would distinguish cases where we have to from cases
were we don't, so we should just invalidate the BPs all the time imo
Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
