public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN()
@ 2023-04-20 15:24 Marvin Häuser
  2023-04-20 15:24 ` [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment Marvin Häuser
  0 siblings, 1 reply; 3+ messages in thread
From: Marvin Häuser @ 2023-04-20 15:24 UTC (permalink / raw)
  To: devel
  Cc: Marvin Häuser, Leif Lindholm, Ard Biesheuvel, Sami Mujawar,
	Vitaly Cheptsov

With the current ASM_FUNC() macro, there is no good way to declare an
alignment constraint for a function. As ASM_FUNC() switches sections,
declaring the constraint before the macro invocation applies it to the
current location in the previous section. Declaring the constraint after
the macro invocation lets the function label point to the location prior
to alignment. Depending on toolchain behaviour, this may cause the label
to point to alignment padding preceding the actual function definition.

To address these issues, introduce the ASM_FUNC_ALIGN() macro, which
declares the alignment constraint right before the function label.

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>
---
 ArmPkg/Include/AsmMacroIoLibV8.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/ArmPkg/Include/AsmMacroIoLibV8.h b/ArmPkg/Include/AsmMacroIoLibV8.h
index 135aaeca5d0b..81164ea9c9e7 100644
--- a/ArmPkg/Include/AsmMacroIoLibV8.h
+++ b/ArmPkg/Include/AsmMacroIoLibV8.h
@@ -41,8 +41,19 @@
   Name:                           ; \
   AARCH64_BTI(c)
 
+#define _ASM_FUNC_ALIGN(Name, Section, Align)       \
+  .global   Name                                  ; \
+  .section  #Section, "ax"                        ; \
+  .type     Name, %function                       ; \
+  .balign   Align                                 ; \
+  Name:                                           ; \
+  AARCH64_BTI(c)
+
 #define ASM_FUNC(Name)  _ASM_FUNC(ASM_PFX(Name), .text. ## Name)
 
+#define ASM_FUNC_ALIGN(Name, Align)  \
+  _ASM_FUNC_ALIGN(ASM_PFX(Name), .text. ## Name, Align)
+
 #define MOV32(Reg, Val)                   \
   movz      Reg, (Val) >> 16, lsl #16   ; \
   movk      Reg, (Val) & 0xffff
-- 
2.40.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment
  2023-04-20 15:24 [PATCH v3 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN() Marvin Häuser
@ 2023-04-20 15:24 ` Marvin Häuser
  2023-04-20 15:58   ` Ard Biesheuvel
  0 siblings, 1 reply; 3+ messages in thread
From: Marvin Häuser @ 2023-04-20 15:24 UTC (permalink / raw)
  To: devel
  Cc: Marvin Häuser, Leif Lindholm, Ard Biesheuvel, Sami Mujawar,
	Vitaly Cheptsov

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>
---
 .../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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment
  2023-04-20 15:24 ` [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment Marvin Häuser
@ 2023-04-20 15:58   ` Ard Biesheuvel
  0 siblings, 0 replies; 3+ messages in thread
From: Ard Biesheuvel @ 2023-04-20 15:58 UTC (permalink / raw)
  To: Marvin Häuser
  Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar,
	Vitaly Cheptsov

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
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-04-20 15:58 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-20 15:24 [PATCH v3 1/2] ArmPkg/AsmMacroIoLibV8: Introduce ASM_FUNC_ALIGN() Marvin Häuser
2023-04-20 15:24 ` [PATCH v3 2/2] ArmPkg/ArmMmuLib: Fix ArmReplaceLiveTranslationEntry() alignment Marvin Häuser
2023-04-20 15:58   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox