[edk2] ArmPkg/ArmLib: don't invalidate entire I-cache on range operation

Message ID 1462956117-15663-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel May 11, 2016, 8:41 a.m.
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.biesheuvel@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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Achin Gupta May 11, 2016, 9:35 a.m. | #1
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.biesheuvel@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.

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.

>    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!

cheers,
Achin

> +  bx      lr

>

>  ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA):

>    mcr     p15, 0, r0, c7, c14, 1  @clean and invalidate single data cache line

> diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm

> index a460bd2da7a9..2363ee457632 100644

> --- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm

> +++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm

> @@ -34,6 +34,12 @@ CTRL_I_BIT      EQU     (1 << 12)

>    bx      lr

>

>

> + RVCT_ASM_EXPORT 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

> +  bx      lr

> +

> +

>   RVCT_ASM_EXPORT ArmCleanDataCacheEntryToPoUByMVA

>    mcr     p15, 0, r0, c7, c11, 1  ; clean single data cache line to PoU

>    bx      lr

> --

> 2.7.4

>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel May 11, 2016, 10:07 a.m. | #2
On 11 May 2016 at 11:35, Achin Gupta <achin.gupta@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.biesheuvel@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
Mark Rutland May 11, 2016, 10:22 a.m. | #3
On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 11:35, Achin Gupta <achin.gupta@arm.com> wrote:

> >> 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?


Almost. You also need DSBs to complete the maintenance ops. e.g. a
sequence:

	d-cache maintenance ops
	DSB
	i-cache maintenance ops
	DSB
	ISB

Thanks,
Mark.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Achin Gupta May 11, 2016, 10:22 a.m. | #4
On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 11:35, Achin Gupta <achin.gupta@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.biesheuvel@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?


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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel May 11, 2016, 10:23 a.m. | #5
On 11 May 2016 at 12:22, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:

>> On 11 May 2016 at 11:35, Achin Gupta <achin.gupta@arm.com> wrote:

>> >> 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?

>

> Almost. You also need DSBs to complete the maintenance ops. e.g. a

> sequence:

>

>         d-cache maintenance ops

>         DSB

>         i-cache maintenance ops

>         DSB

>         ISB

>


Yes, but the DSB is already performed by CacheRangeOperation (). So
adding the ISB in InvalidateInstructionCacheRange () results in
exactly the above.

Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Mark Rutland May 11, 2016, 10:28 a.m. | #6
On Wed, May 11, 2016 at 12:23:58PM +0200, Ard Biesheuvel wrote:
> On 11 May 2016 at 12:22, Mark Rutland <mark.rutland@arm.com> wrote:

> > On Wed, May 11, 2016 at 12:07:51PM +0200, Ard Biesheuvel wrote:

> >> On 11 May 2016 at 11:35, Achin Gupta <achin.gupta@arm.com> wrote:

> >> >> 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?

> >

> > Almost. You also need DSBs to complete the maintenance ops. e.g. a

> > sequence:

> >

> >         d-cache maintenance ops

> >         DSB

> >         i-cache maintenance ops

> >         DSB

> >         ISB

> >

> 

> Yes, but the DSB is already performed by CacheRangeOperation (). So

> adding the ISB in InvalidateInstructionCacheRange () results in

> exactly the above.


Ah, sorry, I managed to miss that when scanning the source code.

I guess I'm just saying "yes" then. :)

THanks,
Mark.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

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);
   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
+  bx      lr
 
 ASM_PFX(ArmCleanInvalidateDataCacheEntryByMVA):
   mcr     p15, 0, r0, c7, c14, 1  @clean and invalidate single data cache line
diff --git a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
index a460bd2da7a9..2363ee457632 100644
--- a/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
+++ b/ArmPkg/Library/ArmLib/ArmV7/ArmV7Support.asm
@@ -34,6 +34,12 @@  CTRL_I_BIT      EQU     (1 << 12)
   bx      lr
 
 
+ RVCT_ASM_EXPORT 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
+  bx      lr
+
+
  RVCT_ASM_EXPORT ArmCleanDataCacheEntryToPoUByMVA
   mcr     p15, 0, r0, c7, c11, 1  ; clean single data cache line to PoU
   bx      lr