On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 11:35, Achin Gupta <[email protected]> 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 <[email protected]>
> >> ---
> >> 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?
Agree. This is what I am doing locally:
@@ -64,8 +64,9 @@ InvalidateInstructionCacheRange (
IN UINTN Length
)
{
- CacheRangeOperation (Address, Length, ArmCleanDataCacheEntryToPoUByMVA);
- ArmInvalidateInstructionCache ();
+ CacheRangeOperation (Address, Length, ArmDataCacheLineLength(),
ArmCleanDataCacheEntryToPoUByMVA);
+ CacheRangeOperation (Address, Length, ArmInstructionCacheLineLength(),
ArmInvalidateInstructionCacheEntryToPoUByMVA);
+ ArmInstructionSynchronizationBarrier();
return Address;
}
>
> >> 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
Makes sense! I think this is the reason why they have munged branch predictor
maintenance with cache maintenance on AARCH64.
cheers,
Achin
>
> Thanks,
> Ard.
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
