[PATCH v6 0/3] xen: introduce Kconfig function alignment option

[PATCH v6 0/3] xen: introduce Kconfig function alignment option

[Roger Pau Monne]

Hello,

The following series adds an additional Kconfig option for the per-arch
code alignment.  Such alignment is to be used in all assembly code
symbols and C functions unless specified otherwise.

Last patch also uses such alignment in order to guarantee enough
distance between function entry points, so that there's always space in
order to do the instruction replacements required by livepatch.

Thanks, Roger.

Roger Pau Monne (3):
  xen: introduce Kconfig function alignment option
  xen: use explicit function alignment if supported by compiler
  xen/livepatch: align functions to ensure minimal distance between
    entry points

 xen/Kconfig                         | 22 ++++++++++++++++++++++
 xen/Makefile                        |  1 +
 xen/arch/arm/Kconfig                |  1 +
 xen/arch/arm/include/asm/config.h   |  3 +--
 xen/arch/arm/livepatch.c            |  2 ++
 xen/arch/arm/xen.lds.S              |  4 ++++
 xen/arch/ppc/Kconfig                |  1 +
 xen/arch/ppc/include/asm/config.h   |  3 ---
 xen/arch/ppc/xen.lds.S              |  4 ++++
 xen/arch/riscv/Kconfig              |  1 +
 xen/arch/riscv/include/asm/config.h |  1 -
 xen/arch/riscv/xen.lds.S            |  4 ++++
 xen/arch/x86/Kconfig                |  1 +
 xen/arch/x86/include/asm/config.h   |  3 +--
 xen/arch/x86/livepatch.c            |  4 ++++
 xen/arch/x86/xen.lds.S              |  4 ++++
 xen/common/Kconfig                  |  5 ++++-
 xen/include/xen/linkage.h           |  5 +++--
 18 files changed, 58 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Roger Pau Monne]

And use it to replace CODE_ALIGN in assembly.  This allows to generalize the
way the code alignment gets set across all architectures.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v5:
 - New in this version.
---
 xen/Kconfig                         | 17 +++++++++++++++++
 xen/arch/arm/Kconfig                |  1 +
 xen/arch/arm/include/asm/config.h   |  3 +--
 xen/arch/ppc/Kconfig                |  1 +
 xen/arch/ppc/include/asm/config.h   |  3 ---
 xen/arch/riscv/Kconfig              |  1 +
 xen/arch/riscv/include/asm/config.h |  1 -
 xen/arch/x86/Kconfig                |  1 +
 xen/arch/x86/include/asm/config.h   |  3 +--
 xen/include/xen/linkage.h           |  5 +++--
 10 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 134e6e68ad84..1e1b041fd52f 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,23 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
 config CC_SPLIT_SECTIONS
 	bool
 
+# Set code alignment.
+#
+# Allow setting on a boolean basis, and then convert such selection to an
+# integer for the build system and code to consume more easily.
+config FUNCTION_ALIGNMENT_4B
+	bool
+config FUNCTION_ALIGNMENT_8B
+	bool
+config FUNCTION_ALIGNMENT_16B
+	bool
+config FUNCTION_ALIGNMENT
+	int
+	default 16 if FUNCTION_ALIGNMENT_16B
+	default  8 if  FUNCTION_ALIGNMENT_8B
+	default  4 if  FUNCTION_ALIGNMENT_4B
+	default  0
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 50e9bfae1ac8..80fb5b14f04e 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -16,6 +16,7 @@ config ARM
 	select HAS_PASSTHROUGH
 	select HAS_UBSAN
 	select IOMMU_FORCE_PT_SHARE
+	select FUNCTION_ALIGNMENT_4B
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 3b6d829197a4..a2e22b659d53 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -53,8 +53,7 @@
 
 /* Linkage for ARM */
 #ifdef __ASSEMBLY__
-#define CODE_ALIGN 4
-#define ALIGN .balign CODE_ALIGN
+#define ALIGN .balign CONFIG_FUNCTION_ALIGNMENT
 #define ENTRY(name)                             \
   .globl name;                                  \
   ALIGN;                                        \
diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
index ab116ffb2a70..6b3b2bb95f56 100644
--- a/xen/arch/ppc/Kconfig
+++ b/xen/arch/ppc/Kconfig
@@ -1,6 +1,7 @@
 config PPC
 	def_bool y
 	select HAS_DEVICE_TREE
+	select FUNCTION_ALIGNMENT_4B
 
 config PPC64
 	def_bool y
diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h
index e5d201e16c50..e0a0abfeb408 100644
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -31,9 +31,6 @@
 #define INVALID_VCPU_ID MAX_VIRT_CPUS
 
 /* Linkage for PPC */
-#ifdef __ASSEMBLY__
-#define CODE_ALIGN 4
-#endif
 
 #define XEN_VIRT_START _AC(0xc000000000000000, UL)
 
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index f382b36f6c82..b4b354a7786e 100644
--- a/xen/arch/riscv/Kconfig
+++ b/xen/arch/riscv/Kconfig
@@ -1,5 +1,6 @@
 config RISCV
 	def_bool y
+	select FUNCTION_ALIGNMENT_16B
 
 config RISCV_64
 	def_bool y
diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
index a80cdd4f857c..99ea5635208b 100644
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -69,7 +69,6 @@
 
 /* Linkage for RISCV */
 #ifdef __ASSEMBLY__
-#define CODE_ALIGN 16
 #define CODE_FILL /* empty */
 #endif
 
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 1acdffc51c22..3dd8f18b46ef 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -29,6 +29,7 @@ config X86
 	select HAS_UBSAN
 	select HAS_VPCI if HVM
 	select NEEDS_LIBELF
+	select FUNCTION_ALIGNMENT_16B
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index 660246d1dae5..ab7288cb3682 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -43,9 +43,8 @@
 
 /* Linkage for x86 */
 #ifdef __ASSEMBLY__
-#define CODE_ALIGN 16
 #define CODE_FILL 0x90
-#define ALIGN .align CODE_ALIGN, CODE_FILL
+#define ALIGN .align CONFIG_FUNCTION_ALIGNMENT, CODE_FILL
 #define ENTRY(name)                             \
   ALIGN;                                        \
   GLOBAL(name)
diff --git a/xen/include/xen/linkage.h b/xen/include/xen/linkage.h
index 0997e16810b2..770ae49963b8 100644
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -41,9 +41,10 @@
  */
 #define count_args_exp(args...) count_args(args)
 #if count_args_exp(CODE_FILL)
-# define DO_CODE_ALIGN(align...) LASTARG(CODE_ALIGN, ## align), CODE_FILL
+# define DO_CODE_ALIGN(align...) LASTARG(CONFIG_FUNCTION_ALIGNMENT, ## align), \
+                                 CODE_FILL
 #else
-# define DO_CODE_ALIGN(align...) LASTARG(CODE_ALIGN, ## align)
+# define DO_CODE_ALIGN(align...) LASTARG(CONFIG_FUNCTION_ALIGNMENT, ## align)
 #endif
 
 #define FUNC(name, align...) \
-- 
2.43.0

[PATCH v6 2/3] xen: use explicit function alignment if supported by compiler

[Roger Pau Monne]

Introduce a new Kconfig check for whether the compiler supports
-falign-functions and if supported use it to align functions to the per-arch
selected value, just like it's done for assembly ENTRY() and FUNC() symbols.

Note that it's possible for the compiler to end up using a higher function
alignment regardless of the passed value.  Different compilers handle the
option differently, as clang will ignore -falign-functions value if it's
smaller than the one that would be set by the optimization level, while gcc
seems to always honor the function alignment passed in -falign-functions.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v5:
 - New in this version.
---
 xen/Kconfig  | 5 +++++
 xen/Makefile | 1 +
 2 files changed, 6 insertions(+)

diff --git a/xen/Kconfig b/xen/Kconfig
index 1e1b041fd52f..040cba1b4b73 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -41,6 +41,11 @@ config CC_SPLIT_SECTIONS
 #
 # Allow setting on a boolean basis, and then convert such selection to an
 # integer for the build system and code to consume more easily.
+#
+# Requires clang >= 7.0.0
+config CC_HAS_FUNCTION_ALIGNMENT
+	def_bool $(cc-option,-falign-functions)
+
 config FUNCTION_ALIGNMENT_4B
 	bool
 config FUNCTION_ALIGNMENT_8B
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..7c8249ab3a33 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -390,6 +390,7 @@ CFLAGS += -fomit-frame-pointer
 endif
 
 CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
+CFLAGS-$(CONFIG_CC_HAS_FUNCTION_ALIGNMENT) += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
-- 
2.43.0

[PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monne]

The minimal function size requirements for an x86 livepatch are either 5 bytes
(for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
distance between functions entry points is always at least of the minimal
required size for livepatch instruction replacement to be successful.

Add an additional align directive to the linker scripts, in order to ensure that
the next section placed after the .text.* (per-function sections) is also
aligned to the required boundary, so that the distance of the last function
entry point with the next symbol is also of minimal size.

Note that livepatch-build-tools will take into account each per-function
section alignment in order to decide whether there's enough padding.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v5:
 - Split chunks into pre-patches to make review easier.

Changes since v4:
 - Split from the rest of the livepatch testing series.
 - Reword and expand a bit the commit message.
 - Add a comment about falign-functions clang version requirement.

Changes since v3:
 - Test for compiler option with -falign-functions.
 - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT.
 - Set 16byte function alignment for x86 release builds.

Changes since v2:
 - Add Arm side.
 - Align end of section in the linker script to ensure enough padding for the
   last function.
 - Expand commit message and subject.
 - Rework Kconfig options.
 - Check that the compiler supports the option.

Changes since v1:
 - New in this version.
---
 xen/arch/arm/livepatch.c | 2 ++
 xen/arch/arm/xen.lds.S   | 4 ++++
 xen/arch/ppc/xen.lds.S   | 4 ++++
 xen/arch/riscv/xen.lds.S | 4 ++++
 xen/arch/x86/livepatch.c | 4 ++++
 xen/arch/x86/xen.lds.S   | 4 ++++
 xen/common/Kconfig       | 5 ++++-
 7 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index bbca1e5a5ed3..aa8ae8c38d28 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -68,6 +68,8 @@ void arch_livepatch_revive(void)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
+    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > CONFIG_FUNCTION_ALIGNMENT);
+
     /* If NOPing only do up to maximum amount we can put in the ->opaque. */
     if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE ||
          func->new_size % ARCH_PATCH_INSN_SIZE) )
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 470c8f22084f..0126a3afae53 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -46,6 +46,10 @@ SECTIONS
 #ifdef CONFIG_CC_SPLIT_SECTIONS
        *(.text.*)
 #endif
+#ifdef CONFIG_LIVEPATCH
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
        *(.fixup)
        *(.gnu.warning)
diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
index 030e1ee37b55..f300c03a666f 100644
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -32,6 +32,10 @@ SECTIONS
 #ifdef CONFIG_CC_SPLIT_SECTIONS
         *(.text.*)
 #endif
+#ifdef CONFIG_LIVEPATCH
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
         *(.fixup)
         *(.gnu.warning)
diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
index 8510a87c4d06..1fb9af11c1cc 100644
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -27,6 +27,10 @@ SECTIONS
 #ifdef CONFIG_CC_SPLIT_SECTIONS
         *(.text.*)
 #endif
+#ifdef CONFIG_LIVEPATCH
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
         . = ALIGN(IDENT_AREA_SIZE);
         _ident_start = .;
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..b00ad7120da9 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -109,6 +109,10 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
          */
         uint8_t needed = ARCH_PATCH_INSN_SIZE;
 
+        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE +
+                     (IS_ENABLED(CONIFG_XEN_IBT) ? ENDBR64_LEN : 0) >
+                     CONFIG_FUNCTION_ALIGNMENT);
+
         if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) )
             needed += ENDBR64_LEN;
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8930e14fc40e..6649bc16dc48 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -101,6 +101,10 @@ SECTIONS
        *(.text.*)
 #endif
        *(.text.__x86_indirect_thunk_*)
+#ifdef CONFIG_LIVEPATCH
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
        *(.fixup)
        *(.gnu.warning)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 310ad4229cdf..63fba175c99f 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -395,8 +395,11 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support"
 	default X86
-	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
 	select CC_SPLIT_SECTIONS
+	select FUNCTION_ALIGNMENT_16B if XEN_IBT
+	select FUNCTION_ALIGNMENT_8B  if X86
+	select FUNCTION_ALIGNMENT_4B  if ARM
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
-- 
2.43.0

Re: [PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Jan Beulich]

On 07.02.2024 15:55, Roger Pau Monne wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -29,6 +29,7 @@ config X86
>  	select HAS_UBSAN
>  	select HAS_VPCI if HVM
>  	select NEEDS_LIBELF
> +	select FUNCTION_ALIGNMENT_16B

With the insertion here as well as for Arm and PPC obeying alphabetic
sorting:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 07.02.2024 15:55, Roger Pau Monne wrote:
> The minimal function size requirements for an x86 livepatch are either 5 bytes
> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
> distance between functions entry points is always at least of the minimal
> required size for livepatch instruction replacement to be successful.
> 
> Add an additional align directive to the linker scripts, in order to ensure that
> the next section placed after the .text.* (per-function sections) is also
> aligned to the required boundary, so that the distance of the last function
> entry point with the next symbol is also of minimal size.

Perhaps "... minimal required size"?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -395,8 +395,11 @@ config CRYPTO
>  config LIVEPATCH
>  	bool "Live patching support"
>  	default X86
> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
>  	select CC_SPLIT_SECTIONS
> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
> +	select FUNCTION_ALIGNMENT_8B  if X86
> +	select FUNCTION_ALIGNMENT_4B  if ARM

This isn't strictly needed, is it? Would be nice to avoid re-selection
of what the default for an arch is anyway, as otherwise this will start
looking clumsy when a couple more architectures are added. Preferably
with that dropped (or it being clarified why it's still desirable to
have):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Michal Orzel]

Hi,

On 07/02/2024 15:55, Roger Pau Monne wrote:
> 
> 
> And use it to replace CODE_ALIGN in assembly.  This allows to generalize the
> way the code alignment gets set across all architectures.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
In xen/linkage.h, there is still a comment at the top mentioning that CODE_ALIGN needs to
be specified by each arch. I think this wants to be removed now. With that and Jan's remark
addressed, for Arm:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
> On 07.02.2024 15:55, Roger Pau Monne wrote:
> > The minimal function size requirements for an x86 livepatch are either 5 bytes
> > (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
> > distance between functions entry points is always at least of the minimal
> > required size for livepatch instruction replacement to be successful.
> > 
> > Add an additional align directive to the linker scripts, in order to ensure that
> > the next section placed after the .text.* (per-function sections) is also
> > aligned to the required boundary, so that the distance of the last function
> > entry point with the next symbol is also of minimal size.
> 
> Perhaps "... minimal required size"?

Yes.

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -395,8 +395,11 @@ config CRYPTO
> >  config LIVEPATCH
> >  	bool "Live patching support"
> >  	default X86
> > -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> > +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
> >  	select CC_SPLIT_SECTIONS
> > +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
> > +	select FUNCTION_ALIGNMENT_8B  if X86
> > +	select FUNCTION_ALIGNMENT_4B  if ARM
> 
> This isn't strictly needed, is it? Would be nice to avoid re-selection
> of what the default for an arch is anyway, as otherwise this will start
> looking clumsy when a couple more architectures are added.

My worry was that the default per-arch could change, ie: for example
x86 moving from 16 to 8 and then it would hamper livepatch support if
IBT is also enabled.  I however think it's very unlikely to reduce the
default alignment, and in any case we would hit a build time assert if
that ever happens.

So yes, I'm fine with dropping those.

> Preferably
> with that dropped (or it being clarified why it's still desirable to
> have):
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, Roger.

Re: [PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Roger Pau Monné]

On Tue, Feb 13, 2024 at 04:51:13PM +0100, Jan Beulich wrote:
> On 07.02.2024 15:55, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -29,6 +29,7 @@ config X86
> >  	select HAS_UBSAN
> >  	select HAS_VPCI if HVM
> >  	select NEEDS_LIBELF
> > +	select FUNCTION_ALIGNMENT_16B
> 
> With the insertion here as well as for Arm and PPC obeying alphabetic
> sorting:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Would you like me to resend with that adjusted?

Thanks, Roger.

Re: [PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Jan Beulich]

On 26.02.2024 12:33, Roger Pau Monné wrote:
> On Tue, Feb 13, 2024 at 04:51:13PM +0100, Jan Beulich wrote:
>> On 07.02.2024 15:55, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -29,6 +29,7 @@ config X86
>>>  	select HAS_UBSAN
>>>  	select HAS_VPCI if HVM
>>>  	select NEEDS_LIBELF
>>> +	select FUNCTION_ALIGNMENT_16B
>>
>> With the insertion here as well as for Arm and PPC obeying alphabetic
>> sorting:
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Would you like me to resend with that adjusted?

I guess it can be taken care of while committing; I've taken note of
this. Sadly there is still at least one missing ack.

Jan

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 26.02.2024 12:32, Roger Pau Monné wrote:
> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
>> On 07.02.2024 15:55, Roger Pau Monne wrote:
>>> The minimal function size requirements for an x86 livepatch are either 5 bytes
>>> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
>>> distance between functions entry points is always at least of the minimal
>>> required size for livepatch instruction replacement to be successful.
>>>
>>> Add an additional align directive to the linker scripts, in order to ensure that
>>> the next section placed after the .text.* (per-function sections) is also
>>> aligned to the required boundary, so that the distance of the last function
>>> entry point with the next symbol is also of minimal size.
>>
>> Perhaps "... minimal required size"?
> 
> Yes.
> 
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -395,8 +395,11 @@ config CRYPTO
>>>  config LIVEPATCH
>>>  	bool "Live patching support"
>>>  	default X86
>>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
>>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
>>>  	select CC_SPLIT_SECTIONS
>>> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
>>> +	select FUNCTION_ALIGNMENT_8B  if X86
>>> +	select FUNCTION_ALIGNMENT_4B  if ARM
>>
>> This isn't strictly needed, is it? Would be nice to avoid re-selection
>> of what the default for an arch is anyway, as otherwise this will start
>> looking clumsy when a couple more architectures are added.
> 
> My worry was that the default per-arch could change, ie: for example
> x86 moving from 16 to 8 and then it would hamper livepatch support if
> IBT is also enabled.  I however think it's very unlikely to reduce the
> default alignment, and in any case we would hit a build time assert if
> that ever happens.
> 
> So yes, I'm fine with dropping those.

Oh, no - not "those", only "that", i.e. only the last (Arm) one.

Jan

Re: [PATCH v6 1/3] xen: introduce Kconfig function alignment option

[Shawn Anastasio]

Hi Roger,

On 2/7/24 8:55 AM, Roger Pau Monne wrote:
> And use it to replace CODE_ALIGN in assembly.  This allows to generalize the
> way the code alignment gets set across all architectures.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>

Thanks,
Shawn

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote:
> On 26.02.2024 12:32, Roger Pau Monné wrote:
> > On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
> >> On 07.02.2024 15:55, Roger Pau Monne wrote:
> >>> The minimal function size requirements for an x86 livepatch are either 5 bytes
> >>> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
> >>> distance between functions entry points is always at least of the minimal
> >>> required size for livepatch instruction replacement to be successful.
> >>>
> >>> Add an additional align directive to the linker scripts, in order to ensure that
> >>> the next section placed after the .text.* (per-function sections) is also
> >>> aligned to the required boundary, so that the distance of the last function
> >>> entry point with the next symbol is also of minimal size.
> >>
> >> Perhaps "... minimal required size"?
> > 
> > Yes.
> > 
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -395,8 +395,11 @@ config CRYPTO
> >>>  config LIVEPATCH
> >>>  	bool "Live patching support"
> >>>  	default X86
> >>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> >>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
> >>>  	select CC_SPLIT_SECTIONS
> >>> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
> >>> +	select FUNCTION_ALIGNMENT_8B  if X86
> >>> +	select FUNCTION_ALIGNMENT_4B  if ARM
> >>
> >> This isn't strictly needed, is it? Would be nice to avoid re-selection
> >> of what the default for an arch is anyway, as otherwise this will start
> >> looking clumsy when a couple more architectures are added.
> > 
> > My worry was that the default per-arch could change, ie: for example
> > x86 moving from 16 to 8 and then it would hamper livepatch support if
> > IBT is also enabled.  I however think it's very unlikely to reduce the
> > default alignment, and in any case we would hit a build time assert if
> > that ever happens.
> > 
> > So yes, I'm fine with dropping those.
> 
> Oh, no - not "those", only "that", i.e. only the last (Arm) one.

Oh, I see what you mean, even x86 selects the default one when IBT is
enabled, and when not the requirement for livepatch is < than the
default anyway.  That's why I said that we could even drop all of them
and just rely on the build time assert to catch any changes here.

Feel free to drop the ARM one.

Thanks, Roger.

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 27.02.2024 09:15, Roger Pau Monné wrote:
> On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote:
>> On 26.02.2024 12:32, Roger Pau Monné wrote:
>>> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
>>>> On 07.02.2024 15:55, Roger Pau Monne wrote:
>>>>> The minimal function size requirements for an x86 livepatch are either 5 bytes
>>>>> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
>>>>> distance between functions entry points is always at least of the minimal
>>>>> required size for livepatch instruction replacement to be successful.
>>>>>
>>>>> Add an additional align directive to the linker scripts, in order to ensure that
>>>>> the next section placed after the .text.* (per-function sections) is also
>>>>> aligned to the required boundary, so that the distance of the last function
>>>>> entry point with the next symbol is also of minimal size.
>>>>
>>>> Perhaps "... minimal required size"?
>>>
>>> Yes.
>>>
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -395,8 +395,11 @@ config CRYPTO
>>>>>  config LIVEPATCH
>>>>>  	bool "Live patching support"
>>>>>  	default X86
>>>>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
>>>>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
>>>>>  	select CC_SPLIT_SECTIONS
>>>>> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
>>>>> +	select FUNCTION_ALIGNMENT_8B  if X86
>>>>> +	select FUNCTION_ALIGNMENT_4B  if ARM
>>>>
>>>> This isn't strictly needed, is it? Would be nice to avoid re-selection
>>>> of what the default for an arch is anyway, as otherwise this will start
>>>> looking clumsy when a couple more architectures are added.
>>>
>>> My worry was that the default per-arch could change, ie: for example
>>> x86 moving from 16 to 8 and then it would hamper livepatch support if
>>> IBT is also enabled.  I however think it's very unlikely to reduce the
>>> default alignment, and in any case we would hit a build time assert if
>>> that ever happens.
>>>
>>> So yes, I'm fine with dropping those.
>>
>> Oh, no - not "those", only "that", i.e. only the last (Arm) one.
> 
> Oh, I see what you mean, even x86 selects the default one when IBT is
> enabled, and when not the requirement for livepatch is < than the
> default anyway.  That's why I said that we could even drop all of them
> and just rely on the build time assert to catch any changes here.

Just to clarify: The default I mean is the architecture imposed one.
Leaving aside Thumb mode, Arm instructions are all 32-bit words, and
hence less than 4-byte alignment makes no sense (and may even be
disallowed by the architecture). Whereas for x86 what you're talking
about is just a compiler default, which isn't really guaranteed to
never be lower (with -Os for example I'd expect it to be perhaps as
low as 1).

Jan

> Feel free to drop the ARM one.
> 
> Thanks, Roger.

Re: [PATCH v6 3/3] xen/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Tue, Feb 27, 2024 at 09:53:24AM +0100, Jan Beulich wrote:
> On 27.02.2024 09:15, Roger Pau Monné wrote:
> > On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote:
> >> On 26.02.2024 12:32, Roger Pau Monné wrote:
> >>> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote:
> >>>> On 07.02.2024 15:55, Roger Pau Monne wrote:
> >>>>> The minimal function size requirements for an x86 livepatch are either 5 bytes
> >>>>> (for jmp) or 9 bytes (for endbr + jmp), and always 4 bytes on Arm.  Ensure that
> >>>>> distance between functions entry points is always at least of the minimal
> >>>>> required size for livepatch instruction replacement to be successful.
> >>>>>
> >>>>> Add an additional align directive to the linker scripts, in order to ensure that
> >>>>> the next section placed after the .text.* (per-function sections) is also
> >>>>> aligned to the required boundary, so that the distance of the last function
> >>>>> entry point with the next symbol is also of minimal size.
> >>>>
> >>>> Perhaps "... minimal required size"?
> >>>
> >>> Yes.
> >>>
> >>>>> --- a/xen/common/Kconfig
> >>>>> +++ b/xen/common/Kconfig
> >>>>> @@ -395,8 +395,11 @@ config CRYPTO
> >>>>>  config LIVEPATCH
> >>>>>  	bool "Live patching support"
> >>>>>  	default X86
> >>>>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> >>>>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
> >>>>>  	select CC_SPLIT_SECTIONS
> >>>>> +	select FUNCTION_ALIGNMENT_16B if XEN_IBT
> >>>>> +	select FUNCTION_ALIGNMENT_8B  if X86
> >>>>> +	select FUNCTION_ALIGNMENT_4B  if ARM
> >>>>
> >>>> This isn't strictly needed, is it? Would be nice to avoid re-selection
> >>>> of what the default for an arch is anyway, as otherwise this will start
> >>>> looking clumsy when a couple more architectures are added.
> >>>
> >>> My worry was that the default per-arch could change, ie: for example
> >>> x86 moving from 16 to 8 and then it would hamper livepatch support if
> >>> IBT is also enabled.  I however think it's very unlikely to reduce the
> >>> default alignment, and in any case we would hit a build time assert if
> >>> that ever happens.
> >>>
> >>> So yes, I'm fine with dropping those.
> >>
> >> Oh, no - not "those", only "that", i.e. only the last (Arm) one.
> > 
> > Oh, I see what you mean, even x86 selects the default one when IBT is
> > enabled, and when not the requirement for livepatch is < than the
> > default anyway.  That's why I said that we could even drop all of them
> > and just rely on the build time assert to catch any changes here.
> 
> Just to clarify: The default I mean is the architecture imposed one.
> Leaving aside Thumb mode, Arm instructions are all 32-bit words, and
> hence less than 4-byte alignment makes no sense (and may even be
> disallowed by the architecture). Whereas for x86 what you're talking
> about is just a compiler default, which isn't really guaranteed to
> never be lower (with -Os for example I'd expect it to be perhaps as
> low as 1).

Right, it's a compiler default, but in patch 1 we already set the
default alignment for x86 to be 16 bytes.

When in your first comment you mentioned "... default for an arch is
anyway" I assumed you mean the default in the arch Kconfig file, not
what the ISA mandates.

Thanks, Roger.

Re: [PATCH v6 2/3] xen: use explicit function alignment if supported by compiler

[Roger Pau Monné]

Ping?

On Wed, Feb 07, 2024 at 03:55:46PM +0100, Roger Pau Monne wrote:
> Introduce a new Kconfig check for whether the compiler supports
> -falign-functions and if supported use it to align functions to the per-arch
> selected value, just like it's done for assembly ENTRY() and FUNC() symbols.
> 
> Note that it's possible for the compiler to end up using a higher function
> alignment regardless of the passed value.  Different compilers handle the
> option differently, as clang will ignore -falign-functions value if it's
> smaller than the one that would be set by the optimization level, while gcc
> seems to always honor the function alignment passed in -falign-functions.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v5:
>  - New in this version.
> ---
>  xen/Kconfig  | 5 +++++
>  xen/Makefile | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/xen/Kconfig b/xen/Kconfig
> index 1e1b041fd52f..040cba1b4b73 100644
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -41,6 +41,11 @@ config CC_SPLIT_SECTIONS
>  #
>  # Allow setting on a boolean basis, and then convert such selection to an
>  # integer for the build system and code to consume more easily.
> +#
> +# Requires clang >= 7.0.0
> +config CC_HAS_FUNCTION_ALIGNMENT
> +	def_bool $(cc-option,-falign-functions)
> +
>  config FUNCTION_ALIGNMENT_4B
>  	bool
>  config FUNCTION_ALIGNMENT_8B
> diff --git a/xen/Makefile b/xen/Makefile
> index 21832d640225..7c8249ab3a33 100644
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -390,6 +390,7 @@ CFLAGS += -fomit-frame-pointer
>  endif
>  
>  CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
> +CFLAGS-$(CONFIG_CC_HAS_FUNCTION_ALIGNMENT) += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
>  
>  CFLAGS += -nostdinc -fno-builtin -fno-common
>  CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
> -- 
> 2.43.0
>