Re: [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment

["Ard Biesheuvel" <ardb@kernel.org>]

On Thu, 20 Apr 2023 at 17:24, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> As the ASM_FUNC() macro performs a section switch, the preceding
> .balign directive applies the alignment constraint to the current
> location in the previous section. As the linker may not merge the
> sections in-order, ArmReplaceLiveTranslationEntry() may be left
> unaligned.
>
> Replace the explicit invocation of .balign with the ASM_FUNC_ALIGN()
> macro, which guarantees the alignment constraint is applied correctly.
> To make sure related issues are reliably caught in the future, align the
> end of the function before checking the total occupied size. This
> ensures crossing a 0x200 boundary will cause a compilation error.
>
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>

Thanks. I've queued these up as #4291


> ---
>  .../ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S      | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> index e936a5be4e11..887439bc042f 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibReplaceEntry.S
> @@ -69,17 +69,16 @@
>  .L2_\@:
>    .endm
>
> -  // Align this routine to a log2 upper bound of its size, so that it is
> -  // guaranteed not to cross a page or block boundary.
> -  .balign 0x200
> -
>  //VOID
>  //ArmReplaceLiveTranslationEntry (
>  //  IN  UINT64  *Entry,
>  //  IN  UINT64  Value,
>  //  IN  UINT64  Address
>  //  )
> -ASM_FUNC(ArmReplaceLiveTranslationEntry)
> +//
> +// Align this routine to a log2 upper bound of its size, so that it is
> +// guaranteed not to cross a page or block boundary.
> +ASM_FUNC_ALIGN(ArmReplaceLiveTranslationEntry, 0x200)
>
>    // disable interrupts
>    mrs   x4, daif
> @@ -101,5 +100,8 @@ ASM_GLOBAL ASM_PFX(ArmReplaceLiveTranslationEntrySize)
>  ASM_PFX(ArmReplaceLiveTranslationEntrySize):
>    .long   . - ArmReplaceLiveTranslationEntry
>
> -  // Double check that we did not overrun the assumed maximum size
> +  // Double check that we did not overrun the assumed maximum size or cross a
> +  // 0x200 boundary (and thus implicitly not any larger power of two, including
> +  // the page size).
> +  .balign 0x200
>    .org    ArmReplaceLiveTranslationEntry + 0x200
> --
> 2.40.0
>