[PATCH 0/4] xen/x86: import linkage.h and clean up x86/kexec.S and x86/entry.S

[PATCH 0/4] xen/x86: import linkage.h and clean up x86/kexec.S and x86/entry.S

[Jane Malalane]

Jane Malalane (4):
  x86/kexec: Add the '.L_' prefix to is_* and call_* labels
  xen: Port linkage.h from kernel code
  x86/entry: move .init.text section higher up in the code for
    readability
  x86: Use linkage.h helpers to add tags to symbols

 xen/arch/x86/x86_64/entry.S       | 124 +++++++++++-------
 xen/arch/x86/x86_64/kexec_reloc.S |  85 ++++++-------
 xen/include/xen/linkage.h         | 260 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 376 insertions(+), 93 deletions(-)
 create mode 100644 xen/include/xen/linkage.h

-- 
2.11.0

[PATCH 1/4] x86/kexec: Add the '.L_' prefix to is_* and call_* labels

[Jane Malalane]

These are local symbols and shouldn't be externally visible.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/x86_64/kexec_reloc.S | 42 +++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/x86_64/kexec_reloc.S b/xen/arch/x86/x86_64/kexec_reloc.S
index 89316bc3a7..f4842025eb 100644
--- a/xen/arch/x86/x86_64/kexec_reloc.S
+++ b/xen/arch/x86/x86_64/kexec_reloc.S
@@ -40,10 +40,10 @@ ENTRY(kexec_reloc)
         movq    %rsi, %cr3
 
         /* Jump to identity mapped code. */
-        leaq    (identity_mapped - kexec_reloc)(%rdi), %rax
+        leaq    (.L_identity_mapped - kexec_reloc)(%rdi), %rax
         jmpq    *%rax
 
-identity_mapped:
+.L_identity_mapped:
         /*
          * Set cr0 to a known state:
          *  - Paging enabled
@@ -70,14 +70,14 @@ identity_mapped:
 
         /* Need to switch to 32-bit mode? */
         testq   $KEXEC_RELOC_FLAG_COMPAT, %r8
-        jnz     call_32_bit
+        jnz     .L_call_32_bit
 
-call_64_bit:
+.L_call_64_bit:
         /* Call the image entry point.  This should never return. */
         callq   *%rbp
         ud2
 
-call_32_bit:
+.L_call_32_bit:
         /* Setup IDT. */
         lidt    compat_mode_idt(%rip)
 
@@ -102,41 +102,41 @@ relocate_pages:
         xorl    %edi, %edi
         xorl    %esi, %esi
 
-next_entry: /* top, read another word for the indirection page */
+.L_next_entry: /* top, read another word for the indirection page */
 
         movq    (%rbx), %rcx
         addq    $8, %rbx
-is_dest:
+.L_is_dest:
         testb   $IND_DESTINATION, %cl
-        jz      is_ind
+        jz      .L_is_ind
         movq    %rcx, %rdi
         andq    $PAGE_MASK, %rdi
-        jmp     next_entry
-is_ind:
+        jmp     .L_next_entry
+.L_is_ind:
         testb   $IND_INDIRECTION, %cl
-        jz      is_done
+        jz      .L_is_done
         movq    %rcx, %rbx
         andq    $PAGE_MASK, %rbx
-        jmp     next_entry
-is_done:
+        jmp     .L_next_entry
+.L_is_done:
         testb   $IND_DONE, %cl
-        jnz     done
-is_source:
+        jnz     .L_done
+.L_is_source:
         testb   $IND_SOURCE, %cl
-        jz      is_zero
+        jz      .L_is_zero
         movq    %rcx, %rsi      /* For every source page do a copy */
         andq    $PAGE_MASK, %rsi
         movl    $(PAGE_SIZE / 8), %ecx
         rep movsq
-        jmp     next_entry
-is_zero:
+        jmp     .L_next_entry
+.L_is_zero:
         testb   $IND_ZERO, %cl
-        jz      next_entry
+        jz      .L_next_entry
         movl    $(PAGE_SIZE / 8), %ecx  /* Zero the destination page. */
         xorl    %eax, %eax
         rep stosq
-        jmp     next_entry
-done:
+        jmp     .L_next_entry
+.L_done:
         popq    %rbx
         ret
 
-- 
2.11.0

[PATCH 2/4] xen: Port linkage.h from kernel code

[Jane Malalane]

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
---
 xen/include/xen/linkage.h | 260 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 260 insertions(+)
 create mode 100644 xen/include/xen/linkage.h

diff --git a/xen/include/xen/linkage.h b/xen/include/xen/linkage.h
new file mode 100644
index 0000000000..adc00c356b
--- /dev/null
+++ b/xen/include/xen/linkage.h
@@ -0,0 +1,260 @@
+#ifndef __XEN_LINKAGE_H
+#define __XEN_LINKAGE_H
+
+/*
+ * Imported from linux-5.19:include/linux/linkage.h
+ */
+
+/* Some toolchains use other characters (e.g. '`') to mark new line in macro */
+#ifndef ASM_NL
+#define ASM_NL		 ;
+#endif
+
+#ifdef __ASSEMBLY__
+
+/* SYM_T_FUNC -- type used by assembler to mark functions */
+#ifndef SYM_T_FUNC
+#define SYM_T_FUNC				STT_FUNC
+#endif
+
+/* SYM_T_OBJECT -- type used by assembler to mark data */
+#ifndef SYM_T_OBJECT
+#define SYM_T_OBJECT				STT_OBJECT
+#endif
+
+/* SYM_T_NONE -- type used by assembler to mark entries of unknown type */
+#ifndef SYM_T_NONE
+#define SYM_T_NONE				STT_NOTYPE
+#endif
+
+/* SYM_A_* -- align the symbol? */
+#define SYM_A_ALIGN				ALIGN
+#define SYM_A_NONE				/* nothing */
+
+/* SYM_L_* -- linkage of symbols */
+#define SYM_L_GLOBAL(name)			.globl name
+#define SYM_L_WEAK(name)			.weak name
+#define SYM_L_LOCAL(name)			/* nothing */
+
+/* === generic annotations === */
+
+/* SYM_ENTRY -- use only if you have to for non-paired symbols */
+#ifndef SYM_ENTRY
+#define SYM_ENTRY(name, linkage, align...)		\
+	linkage(name) ASM_NL				\
+	align ASM_NL					\
+	name:
+#endif
+
+/* SYM_START -- use only if you have to */
+#ifndef SYM_START
+#define SYM_START(name, linkage, align...)		\
+	SYM_ENTRY(name, linkage, align)
+#endif
+
+/* SYM_END -- use only if you have to */
+#ifndef SYM_END
+#define SYM_END(name, sym_type)				\
+	.type name sym_type ASM_NL			\
+	.set .L__sym_size_##name, .-name ASM_NL		\
+	.size name, .L__sym_size_##name
+#endif
+
+/* SYM_ALIAS -- use only if you have to */
+#ifndef SYM_ALIAS
+#define SYM_ALIAS(alias, name, linkage)			\
+	linkage(alias) ASM_NL				\
+	.set alias, name ASM_NL
+#endif
+
+/* === code annotations === */
+
+/*
+ * FUNC -- C-like functions (proper stack frame etc.)
+ * CODE -- non-C code (e.g. irq handlers with different, special stack etc.)
+ *
+ * Objtool validates stack for FUNC, but not for CODE.
+ * Objtool generates debug info for both FUNC & CODE, but needs special
+ * annotations for each CODE's start (to describe the actual stack frame).
+ *
+ * Objtool requires that all code must be contained in an ELF symbol. Symbol
+ * names that have a  .L prefix do not emit symbol table entries. .L
+ * prefixed symbols can be used within a code region, but should be avoided for
+ * denoting a range of code via ``SYM_*_START/END`` annotations.
+ *
+ * ALIAS -- does not generate debug info -- the aliased function will
+ */
+
+/* SYM_INNER_LABEL_ALIGN -- only for labels in the middle of code,
+ * w/ alignment
+ */
+#ifndef SYM_INNER_LABEL_ALIGN
+#define SYM_INNER_LABEL_ALIGN(name, linkage)	\
+	.type name SYM_T_NONE ASM_NL			\
+	SYM_ENTRY(name, linkage, SYM_A_ALIGN)
+#endif
+
+/* SYM_INNER_LABEL_LOCAL -- only for local labels in the middle of code */
+#ifndef SYM_INNER_LABEL_LOCAL
+#define SYM_INNER_LABEL_LOCAL(name)			\
+	.type name SYM_T_NONE ASM_NL			\
+	SYM_ENTRY(name, SYM_L_LOCAL, SYM_A_NONE)
+#endif
+
+/* SYM_INNER_LABEL_GLOBAL -- only for global labels in the middle of code */
+#ifndef SYM_INNER_LABEL_GLOBAL
+#define SYM_INNER_LABEL_GLOBAL(name)				    \
+            .type name SYM_T_NONE ASM_NL                    \
+	    SYM_ENTRY(name, SYM_L_GLOBAL, SYM_A_NONE)
+#endif
+
+/* SYM_FUNC_START -- use for global functions */
+#ifndef SYM_FUNC_START
+#define SYM_FUNC_START(name)				\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)
+#endif
+
+/* SYM_FUNC_START_NOALIGN -- use for global functions, w/o alignment */
+#ifndef SYM_FUNC_START_NOALIGN
+#define SYM_FUNC_START_NOALIGN(name)			\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_NONE)
+#endif
+
+/* SYM_FUNC_START_LOCAL -- use for local functions */
+#ifndef SYM_FUNC_START_LOCAL
+#define SYM_FUNC_START_LOCAL(name)			\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_ALIGN)
+#endif
+
+/* SYM_FUNC_START_LOCAL_NOALIGN -- use for local functions, w/o alignment */
+#ifndef SYM_FUNC_START_LOCAL_NOALIGN
+#define SYM_FUNC_START_LOCAL_NOALIGN(name)		\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_NONE)
+#endif
+
+/* SYM_FUNC_START_WEAK -- use for weak functions */
+#ifndef SYM_FUNC_START_WEAK
+#define SYM_FUNC_START_WEAK(name)			\
+	SYM_START(name, SYM_L_WEAK, SYM_A_ALIGN)
+#endif
+
+/* SYM_FUNC_START_WEAK_NOALIGN -- use for weak functions, w/o alignment */
+#ifndef SYM_FUNC_START_WEAK_NOALIGN
+#define SYM_FUNC_START_WEAK_NOALIGN(name)		\
+	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)
+#endif
+
+/*
+ * SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
+ * SYM_FUNC_START_WEAK, ...
+ */
+#ifndef SYM_FUNC_END
+#define SYM_FUNC_END(name)				\
+	SYM_END(name, SYM_T_FUNC)
+#endif
+
+/*
+ * SYM_FUNC_ALIAS -- define a global alias for an existing function
+ */
+#ifndef SYM_FUNC_ALIAS
+#define SYM_FUNC_ALIAS(alias, name)					\
+	SYM_ALIAS(alias, name, SYM_L_GLOBAL)
+#endif
+
+/*
+ * SYM_FUNC_ALIAS_LOCAL -- define a local alias for an existing function
+ */
+#ifndef SYM_FUNC_ALIAS_LOCAL
+#define SYM_FUNC_ALIAS_LOCAL(alias, name)				\
+	SYM_ALIAS(alias, name, SYM_L_LOCAL)
+#endif
+
+/*
+ * SYM_FUNC_ALIAS_WEAK -- define a weak global alias for an existing function
+ */
+#ifndef SYM_FUNC_ALIAS_WEAK
+#define SYM_FUNC_ALIAS_WEAK(alias, name)				\
+	SYM_ALIAS(alias, name, SYM_L_WEAK)
+#endif
+
+/* SYM_CODE_START -- use for non-C (special) functions */
+#ifndef SYM_CODE_START
+#define SYM_CODE_START(name)				\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)
+#endif
+
+/* SYM_CODE_START_NOALIGN -- use for non-C (special) functions, w/o alignment */
+#ifndef SYM_CODE_START_NOALIGN
+#define SYM_CODE_START_NOALIGN(name)			\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_NONE)
+#endif
+
+/* SYM_CODE_START_LOCAL -- use for local non-C (special) functions */
+#ifndef SYM_CODE_START_LOCAL
+#define SYM_CODE_START_LOCAL(name)			\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_ALIGN)
+#endif
+
+/*
+ * SYM_CODE_START_LOCAL_NOALIGN -- use for local non-C (special) functions,
+ * w/o alignment
+ */
+#ifndef SYM_CODE_START_LOCAL_NOALIGN
+#define SYM_CODE_START_LOCAL_NOALIGN(name)		\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_NONE)
+#endif
+
+/* SYM_CODE_END -- the end of SYM_CODE_START_LOCAL, SYM_CODE_START, ... */
+#ifndef SYM_CODE_END
+#define SYM_CODE_END(name)				\
+	SYM_END(name, SYM_T_NONE)
+#endif
+
+/* === data annotations === */
+
+/* SYM_DATA_START -- global data symbol */
+#ifndef SYM_DATA_START
+#define SYM_DATA_START(name)				\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_NONE)
+#endif
+
+/* SYM_DATA_START -- local data symbol */
+#ifndef SYM_DATA_START_LOCAL
+#define SYM_DATA_START_LOCAL(name)			\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_NONE)
+#endif
+
+/* SYM_DATA_END -- the end of SYM_DATA_START symbol */
+#ifndef SYM_DATA_END
+#define SYM_DATA_END(name)				\
+	SYM_END(name, SYM_T_OBJECT)
+#endif
+
+/* SYM_DATA_END_LABEL -- the labeled end of SYM_DATA_START symbol */
+#ifndef SYM_DATA_END_LABEL
+#define SYM_DATA_END_LABEL(name, linkage, label)	\
+	linkage(label) ASM_NL				\
+	.type label SYM_T_OBJECT ASM_NL			\
+	label:						\
+	SYM_END(name, SYM_T_OBJECT)
+#endif
+
+/* SYM_DATA -- start+end wrapper around simple global data */
+#ifndef SYM_DATA
+#define SYM_DATA(name, data...)				\
+	SYM_DATA_START(name) ASM_NL				\
+	data ASM_NL						\
+	SYM_DATA_END(name)
+#endif
+
+/* SYM_DATA_LOCAL -- start+end wrapper around simple local data */
+#ifndef SYM_DATA_LOCAL
+#define SYM_DATA_LOCAL(name, data...)			\
+	SYM_DATA_START_LOCAL(name) ASM_NL			\
+	data ASM_NL						\
+	SYM_DATA_END(name)
+#endif
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __XEN_LINKAGE_H */
-- 
2.11.0

[PATCH 3/4] x86/entry: move .init.text section higher up in the code for readability

[Jane Malalane]

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/x86_64/entry.S | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 26bf2f1941..4ad25d9c90 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -140,6 +140,15 @@ process_trap:
         call create_bounce_frame
         jmp  test_all_events
 
+        .pushsection .init.text, "ax", @progbits
+        ENTRY(early_page_fault)
+        ENDBR64
+        movl  $TRAP_page_fault,4(%rsp)
+        SAVE_ALL
+        movq  %rsp,%rdi
+        call  do_early_page_fault
+        jmp   restore_all_xen
+
         .section .text.entry, "ax", @progbits
 
 /* %rbx: struct vcpu, interrupts disabled */
@@ -982,16 +991,6 @@ ENTRY(double_fault)
         call  do_double_fault
         BUG   /* do_double_fault() shouldn't return. */
 
-        .pushsection .init.text, "ax", @progbits
-ENTRY(early_page_fault)
-        ENDBR64
-        movl  $TRAP_page_fault,4(%rsp)
-        SAVE_ALL
-        movq  %rsp,%rdi
-        call  do_early_page_fault
-        jmp   restore_all_xen
-        .popsection
-
 ENTRY(nmi)
         ENDBR64
         pushq $0
-- 
2.11.0

[RFC PATCH 4/4] x86: Use linkage.h helpers to add tags to symbols

[Jane Malalane]

Clean up x86_64/kexec_reloc.S and x86_64/entry.S.

This fixes the livepatching contents of entry.S.

RFC: I'm unsure on where the page_fault symbol should end, i.e. if
unlike current code handle_exception_saved should be within page_fault
like handle_exception is or not.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/x86_64/entry.S       | 105 +++++++++++++++++++++++++-------------
 xen/arch/x86/x86_64/kexec_reloc.S |  43 ++++++----------
 2 files changed, 86 insertions(+), 62 deletions(-)

diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 4ad25d9c90..7dc280aafa 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -11,6 +11,7 @@
 #include <asm/processor.h>
 #include <public/xen.h>
 #include <irq_vectors.h>
+#include <xen/linkage.h>
 
 /* %rsp: struct cpu_user_regs */
 .macro ASSERT_CONTEXT_IS_XEN
@@ -152,7 +153,7 @@ process_trap:
         .section .text.entry, "ax", @progbits
 
 /* %rbx: struct vcpu, interrupts disabled */
-restore_all_guest:
+SYM_CODE_START_LOCAL(restore_all_guest)
         ASSERT_INTERRUPTS_DISABLED
 
         /* Stash guest SPEC_CTRL value while we can read struct vcpu. */
@@ -239,6 +240,7 @@ iret_exit_to_guest:
         addq  $8,%rsp
 .Lft0:  iretq
         _ASM_PRE_EXTABLE(.Lft0, handle_exception)
+SYM_CODE_END(restore_all_guest)
 
 /*
  * When entering SYSCALL from kernel mode:
@@ -255,7 +257,7 @@ iret_exit_to_guest:
  *  - Guest %rsp stored in %rax
  *  - Xen stack loaded, pointing at the %ss slot
  */
-ENTRY(lstar_enter)
+SYM_CODE_START(lstar_enter)
 #ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
 #endif
@@ -290,9 +292,10 @@ ENTRY(lstar_enter)
         mov   %rsp, %rdi
         call  pv_hypercall
         jmp   test_all_events
+SYM_CODE_END(lstar_enter)
 
 /* See lstar_enter for entry register state. */
-ENTRY(cstar_enter)
+SYM_CODE_START(cstar_enter)
 #ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
 #endif
@@ -329,8 +332,9 @@ ENTRY(cstar_enter)
         jne   compat_syscall
 #endif
         jmp   switch_to_kernel
+SYM_CODE_END(cstar_enter)
 
-ENTRY(sysenter_entry)
+SYM_CODE_START(sysenter_entry)
         ENDBR64
 #ifdef CONFIG_XEN_SHSTK
         ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
@@ -339,7 +343,7 @@ ENTRY(sysenter_entry)
         pushq $FLAT_USER_SS
         pushq $0
         pushfq
-GLOBAL(sysenter_eflags_saved)
+SYM_INNER_LABEL_GLOBAL(sysenter_eflags_saved)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $3 /* ring 3 null cs */
         pushq $0 /* null rip */
@@ -393,8 +397,9 @@ UNLIKELY_END(sysenter_gpf)
         jne   compat_sysenter
 #endif
         jmp   .Lbounce_exception
+SYM_CODE_END(sysenter_entry)
 
-ENTRY(int80_direct_trap)
+SYM_CODE_START(int80_direct_trap)
         ENDBR64
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         pushq $0
@@ -465,8 +470,9 @@ UNLIKELY_END(msi_check)
 
         call  create_bounce_frame
         jmp   test_all_events
+SYM_CODE_END(int80_direct_trap)
 
-int80_slow_path:
+SYM_CODE_START_LOCAL(int80_slow_path)
         /* 
          * Setup entry vector and error code as if this was a GPF caused by an
          * IDT entry with DPL==0.
@@ -482,6 +488,7 @@ int80_slow_path:
          */
         GET_STACK_END(14)
         jmp   handle_exception_saved
+SYM_CODE_END(int80_slow_path)
 
         /* create_bounce_frame & helpers don't need to be in .text.entry */
         .text
@@ -657,9 +664,8 @@ ret_from_intr:
 
         .section .text.entry, "ax", @progbits
 
-        ALIGN
 /* No special register assumptions. */
-restore_all_xen:
+SYM_CODE_START_LOCAL(restore_all_xen)
         /*
          * Check whether we need to switch to the per-CPU page tables, in
          * case we return to late PV exit code (from an NMI or #MC).
@@ -676,8 +682,9 @@ UNLIKELY_END(exit_cr3)
 
         RESTORE_ALL adj=8
         iretq
+SYM_CODE_END(restore_all_xen)
 
-ENTRY(common_interrupt)
+SYM_CODE_START(common_interrupt)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         SAVE_ALL
 
@@ -706,12 +713,14 @@ ENTRY(common_interrupt)
         mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
         mov   %bl, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
         jmp ret_from_intr
+SYM_CODE_END(common_interrupt)
 
-ENTRY(page_fault)
+SYM_CODE_START(page_fault)
         ENDBR64
         movl  $TRAP_page_fault,4(%rsp)
+
 /* No special register assumptions. */
-GLOBAL(handle_exception)
+SYM_INNER_LABEL_GLOBAL(handle_exception)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         SAVE_ALL
 
@@ -734,7 +743,7 @@ GLOBAL(handle_exception)
         cmovnz %r12d, %r13d
 .Lxcpt_cr3_okay:
 
-handle_exception_saved:
+SYM_INNER_LABEL_LOCAL(handle_exception_saved)
         GET_CURRENT(bx)
         testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
         jz    exception_with_ints_disabled
@@ -842,9 +851,10 @@ handle_exception_saved:
         ASSERT_CONTEXT_IS_XEN
         jmp   restore_all_xen
 #endif
+SYM_CODE_END(page_fault)
 
 /* No special register assumptions. */
-exception_with_ints_disabled:
+SYM_CODE_START_LOCAL(exception_with_ints_disabled)
         testb $3,UREGS_cs(%rsp)         # interrupts disabled outside Xen?
         jnz   FATAL_exception_with_ints_disabled
         movq  %rsp,%rdi
@@ -874,99 +884,116 @@ exception_with_ints_disabled:
         mov   %r15, STACK_CPUINFO_FIELD(xen_cr3)(%r14)
         mov   %r13b, STACK_CPUINFO_FIELD(use_pv_cr3)(%r14)
         jmp   restore_all_xen           # return to fixup code
+SYM_CODE_END(exception_with_ints_disabled)
 
 /* No special register assumptions. */
-FATAL_exception_with_ints_disabled:
+SYM_CODE_START_LOCAL(FATAL_exception_with_ints_disabled)
         xorl  %esi,%esi
         movq  %rsp,%rdi
         call  fatal_trap
         BUG   /* fatal_trap() shouldn't return. */
+SYM_CODE_END(FATAL_exception_with_ints_disabled)
 
-ENTRY(divide_error)
+SYM_CODE_START(divide_error)
         ENDBR64
         pushq $0
         movl  $TRAP_divide_error,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(divide_error)
 
-ENTRY(coprocessor_error)
+SYM_CODE_START(coprocessor_error)
         ENDBR64
         pushq $0
         movl  $TRAP_copro_error,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(coprocessor_error)
 
-ENTRY(simd_coprocessor_error)
+SYM_CODE_START(simd_coprocessor_error)
         ENDBR64
         pushq $0
         movl  $TRAP_simd_error,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(simd_coprocessor_error)
 
-ENTRY(device_not_available)
+SYM_CODE_START(device_not_available)
         ENDBR64
         pushq $0
         movl  $TRAP_no_device,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(device_not_available)
 
-ENTRY(debug)
+SYM_CODE_START(debug)
         ENDBR64
         pushq $0
         movl  $TRAP_debug,4(%rsp)
         jmp   handle_ist_exception
+SYM_CODE_END(debug)
 
-ENTRY(int3)
+SYM_CODE_START(int3)
         ENDBR64
         pushq $0
         movl  $TRAP_int3,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(int3)
 
-ENTRY(overflow)
+SYM_CODE_START(overflow)
         ENDBR64
         pushq $0
         movl  $TRAP_overflow,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(overflow)
 
-ENTRY(bounds)
+SYM_CODE_START(bounds)
         ENDBR64
         pushq $0
         movl  $TRAP_bounds,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(bounds)
 
-ENTRY(invalid_op)
+SYM_CODE_START(invalid_op)
         ENDBR64
         pushq $0
         movl  $TRAP_invalid_op,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(invalid_op)
 
-ENTRY(invalid_TSS)
+SYM_CODE_START(invalid_TSS)
         ENDBR64
         movl  $TRAP_invalid_tss,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(invalid_TSS)
 
-ENTRY(segment_not_present)
+SYM_CODE_START(segment_not_present)
         ENDBR64
         movl  $TRAP_no_segment,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(segment_not_present)
 
-ENTRY(stack_segment)
+SYM_CODE_START(stack_segment)
         ENDBR64
         movl  $TRAP_stack_error,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(stack_segment)
 
-ENTRY(general_protection)
+SYM_CODE_START(general_protection)
         ENDBR64
         movl  $TRAP_gp_fault,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(general_protection)
 
-ENTRY(alignment_check)
+SYM_CODE_START(alignment_check)
         ENDBR64
         movl  $TRAP_alignment_check,4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(alignment_check)
 
-ENTRY(entry_CP)
+SYM_CODE_START(entry_CP)
         ENDBR64
         movl  $X86_EXC_CP, 4(%rsp)
         jmp   handle_exception
+SYM_CODE_END(entry_CP)
 
-ENTRY(double_fault)
+SYM_CODE_START(double_fault)
         ENDBR64
         movl  $TRAP_double_fault,4(%rsp)
         /* Set AC to reduce chance of further SMAP faults */
@@ -990,12 +1017,13 @@ ENTRY(double_fault)
         movq  %rsp,%rdi
         call  do_double_fault
         BUG   /* do_double_fault() shouldn't return. */
+SYM_CODE_END(double_fault)
 
-ENTRY(nmi)
+SYM_CODE_START(nmi)
         ENDBR64
         pushq $0
         movl  $TRAP_nmi,4(%rsp)
-handle_ist_exception:
+SYM_INNER_LABEL_LOCAL(handle_ist_exception)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
         SAVE_ALL
 
@@ -1119,17 +1147,20 @@ handle_ist_exception:
         ASSERT_CONTEXT_IS_XEN
         jmp   restore_all_xen
 #endif
+SYM_CODE_END(nmi)
 
-ENTRY(machine_check)
+SYM_CODE_START(machine_check)
         ENDBR64
         pushq $0
         movl  $TRAP_machine_check,4(%rsp)
         jmp   handle_ist_exception
+SYM_CODE_END(machine_check)
 
 /* No op trap handler.  Required for kexec crash path. */
-GLOBAL(trap_nop)
+SYM_CODE_START_NOALIGN(trap_nop)
         ENDBR64
         iretq
+SYM_CODE_END(trap_nop)
 
 /* Table of automatically generated entry points.  One per vector. */
         .pushsection .init.rodata, "a", @progbits
@@ -1142,7 +1173,8 @@ GLOBAL(autogen_entrypoints)
         .endm
 
         .popsection
-autogen_stubs: /* Automatically generated stubs. */
+/* Automatically generated stubs. */
+SYM_CODE_START_LOCAL(autogen_stubs)
 
         vec = 0
         .rept X86_NR_VECTORS
@@ -1186,6 +1218,7 @@ autogen_stubs: /* Automatically generated stubs. */
 
         vec = vec + 1
         .endr
+SYM_CODE_END(autogen_stubs)
 
         .section .init.rodata, "a", @progbits
         .size autogen_entrypoints, . - autogen_entrypoints
diff --git a/xen/arch/x86/x86_64/kexec_reloc.S b/xen/arch/x86/x86_64/kexec_reloc.S
index f4842025eb..5f96c74085 100644
--- a/xen/arch/x86/x86_64/kexec_reloc.S
+++ b/xen/arch/x86/x86_64/kexec_reloc.S
@@ -14,6 +14,7 @@
         .file __FILE__
 
 #include <xen/kimage.h>
+#include <xen/linkage.h>
 
 #include <asm/asm_defns.h>
 #include <asm/msr-index.h>
@@ -24,7 +25,7 @@
         .align PAGE_SIZE
         .code64
 
-ENTRY(kexec_reloc)
+SYM_FUNC_START(kexec_reloc)
         /* %rdi - code page maddr */
         /* %rsi - page table maddr */
         /* %rdx - indirection page maddr */
@@ -92,8 +93,9 @@ ENTRY(kexec_reloc)
 
         /* Enter compatibility mode. */
         ljmp    *compatibility_mode_far(%rip)
+SYM_FUNC_END(kexec_reloc)
 
-relocate_pages:
+SYM_FUNC_START_LOCAL(relocate_pages)
         /* %rdi - indirection page maddr */
         pushq   %rbx
 
@@ -141,8 +143,9 @@ relocate_pages:
         ret
 
         .code32
+SYM_FUNC_END(relocate_pages)
 
-compatibility_mode:
+SYM_FUNC_START_LOCAL(compatibility_mode)
         /* Setup some sane segments. */
         movl    $0x0008, %eax
         movl    %eax, %ds
@@ -169,46 +172,34 @@ compatibility_mode:
         /* Call the image entry point.  This should never return. */
         call    *%ebp
         ud2
+SYM_FUNC_END(compatibility_mode)
 
-        .align 4
-compatibility_mode_far:
+SYM_DATA_START_LOCAL(compatibility_mode_far)
         .long 0x00000000             /* set in call_32_bit above */
         .word 0x0010
+SYM_DATA_END(compatibility_mode_far)
 
-        .type compatibility_mode_far, @object
-        .size compatibility_mode_far, . - compatibility_mode_far
-
-compat_mode_gdt_desc:
+SYM_DATA_START_LOCAL(compat_mode_gdt_desc)
         .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
         .quad 0x0000000000000000     /* set in call_32_bit above */
+SYM_DATA_END(compat_mode_gdt_desc)
 
-        .type compat_mode_gdt_desc, @object
-        .size compat_mode_gdt_desc, . - compat_mode_gdt_desc
-
-        .align 8
-compat_mode_gdt:
+SYM_DATA_START_LOCAL(compat_mode_gdt)
         .quad 0x0000000000000000     /* null                              */
         .quad 0x00cf93000000ffff     /* 0x0008 ring 0 data                */
         .quad 0x00cf9b000000ffff     /* 0x0010 ring 0 code, compatibility */
 .Lcompat_mode_gdt_end:
+SYM_DATA_END(compat_mode_gdt)
 
-        .type compat_mode_gdt, @object
-        .size compat_mode_gdt, . - compat_mode_gdt
-
-compat_mode_idt:
+SYM_DATA_START_LOCAL(compat_mode_idt)
         .word 0                      /* limit */
         .long 0                      /* base */
-
-        .type compat_mode_idt, @object
-        .size compat_mode_idt, . - compat_mode_idt
+SYM_DATA_END(compat_mode_idt)
 
         /*
          * 16 words of stack are more than enough.
          */
-        .align 8
-reloc_stack:
+SYM_DATA_START_LOCAL(reloc_stack)
         .fill 16,8,0
 .Lreloc_stack_base:
-
-        .type reloc_stack, @object
-        .size reloc_stack, . - reloc_stack
+SYM_DATA_END(reloc_stack)
-- 
2.11.0

Re: [PATCH 3/4] x86/entry: move .init.text section higher up in the code for readability

[Andrew Cooper]

On 04/08/2022 16:04, Jane Malalane wrote:

Commit message wants to read "so it's not a random piece of non
.text.entry in the middle of .text.entry" or words to this effect.

> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>  xen/arch/x86/x86_64/entry.S | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 26bf2f1941..4ad25d9c90 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -140,6 +140,15 @@ process_trap:
>          call create_bounce_frame
>          jmp  test_all_events
>  
> +        .pushsection .init.text, "ax", @progbits

Given that you've (correctly) dropped the .popsection, this should be a
plain .section rather than .pushsection.

Both can be fixed on commit.

~Andrew

Re: [RFC PATCH 4/4] x86: Use linkage.h helpers to add tags to symbols

[Andrew Cooper]

On 04/08/2022 16:04, Jane Malalane wrote:
> Clean up x86_64/kexec_reloc.S and x86_64/entry.S.

It would probably help to split the patch into two, because the reloc
changes are not related to the livepatchability fixes in entry.S

> This fixes the livepatching contents of entry.S.

Well - its the first of several bugfixes.

Specifically, we are adding ELF metadata so that the
livepatch-build-tools can actually binary diff changes in this area.

>
> RFC: I'm unsure on where the page_fault symbol should end, i.e. if
> unlike current code handle_exception_saved should be within page_fault
> like handle_exception is or not.

Jan: we've got two examples (page fault, and NMI) which don't form any
reasonable function layout.  Both of these are fallthrough into
handle_{ist,}_exception.

I suggested labelling handle_{ist,}_exception as the main symbol, and
keeping {page_fault,nmi} as small stubs, because we want backtraces to
stay the same and not report {page_fault,nmi} for everything.

~Andrew

Re: [PATCH 0/4] xen/x86: import linkage.h and clean up x86/kexec.S and x86/entry.S

[Andrew Cooper]

On 04/08/2022 16:04, Jane Malalane wrote:
> Jane Malalane (4):
>   x86/kexec: Add the '.L_' prefix to is_* and call_* labels
>   xen: Port linkage.h from kernel code
>   x86/entry: move .init.text section higher up in the code for
>     readability
>   x86: Use linkage.h helpers to add tags to symbols

This probably deserves a bit of explanation.

Patch 4 is the first of several bugfixes which have been outstanding
since XSA-297/MDS (I think - pretty sure it was this XSA, but if not
then it was one around that time) where an attempt to patch
restore_all_guest failed in several creative ways.

First, we need ELF metadata so the livepatch build tools can actually do
their jobs.

Second (and in a series to follow) is teaching Xen's livepatch logic
that .text.entry is special and requires mapping in the XPTI pagetables too.


The choice to go with Linux's linkage.h is because it's already a
standard that people working in our area are familiar with, and because
my previous attempts to sort the ELF metadata have been resounding
failures of nitpicking.

My expectation is that over time, we'll move all asm code over to using
these and retire the current ad-hoc macros we have.

This series is all suggested/requested by me, so implicitly acked, but
should have acks from someone else too.

~Andrew

Re: [PATCH 1/4] x86/kexec: Add the '.L_' prefix to is_* and call_* labels

[Jan Beulich]

On 04.08.2022 17:04, Jane Malalane wrote:
> These are local symbols and shouldn't be externally visible.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

albeit I have to admit I'm not overly happy with the underscores you
add. I think we use .L not followed by an underscore about everywhere
else, on the basis (afaic) that the L being upper case already makes
prefix and actual name sufficiently separated. But yes, in some more
recent work from Andrew there are a few cases of .L_ ...

Jan

Re: [PATCH 2/4] xen: Port linkage.h from kernel code

[Jan Beulich]

On 04.08.2022 17:04, Jane Malalane wrote:
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

In the title you say "port", but then you don't say what customization
you've done beyond the obvious adjustment of inclusion guard and
adjustment (actually removal) of #include-s. How much customization we
want to do is important here, after all. I notice you did, for example,
add new flavors of SYM_INNER_LABEL, but then you didn't add anything to
use .hidden (which I have queued as a new patch on top of a supposed v2
of "x86: annotate entry points with type and size"). At the same time
you've left in objtool related commentary, when we don't use that tool
(and no-one knows if we ever will).

I'm further not sure I agree with the naming of some of your additions,
as they appear to not really fit with the naming model used elsewhere.
Your additions also look to not always match style used elsewhere in
this file.

You further want to mention what Linux version this was derived from,
to make it easier to determine what incremental changes to port over
subsequently.

Overall I'm not convinced this is a model we want to go with, first
and foremost because the commonly used macro names are too long for
my taste. What's wrong with ENTRY() and ENDPROC() / ENDDATA()?

Jan

Re: [PATCH 3/4] x86/entry: move .init.text section higher up in the code for readability

[Jan Beulich]

On 04.08.2022 17:04, Jane Malalane wrote:
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> ---
>  xen/arch/x86/x86_64/entry.S | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index 26bf2f1941..4ad25d9c90 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -140,6 +140,15 @@ process_trap:
>          call create_bounce_frame
>          jmp  test_all_events
>  
> +        .pushsection .init.text, "ax", @progbits
> +        ENTRY(early_page_fault)

Why is this line suddenly not unindented anymore (as labels ought to be)?

> +        ENDBR64
> +        movl  $TRAP_page_fault,4(%rsp)

As you're moving this code, may I ask that you add blanks after the comma
here and ...

> +        SAVE_ALL
> +        movq  %rsp,%rdi

... here? Then, with Andrew's comments also taken care of,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [RFC PATCH 4/4] x86: Use linkage.h helpers to add tags to symbols

[Jan Beulich]

On 04.08.2022 21:46, Andrew Cooper wrote:
> On 04/08/2022 16:04, Jane Malalane wrote:
>> RFC: I'm unsure on where the page_fault symbol should end, i.e. if
>> unlike current code handle_exception_saved should be within page_fault
>> like handle_exception is or not.
> 
> Jan: we've got two examples (page fault, and NMI) which don't form any
> reasonable function layout.  Both of these are fallthrough into
> handle_{ist,}_exception.
> 
> I suggested labelling handle_{ist,}_exception as the main symbol, and
> keeping {page_fault,nmi} as small stubs, because we want backtraces to
> stay the same and not report {page_fault,nmi} for everything.

I.e. the opposite of what the patch currently does. That's fine with me
in principle (sadly there's no STT_THUNK or alike, which might allow
better reflecting the purpose yet still not marking these as STT_FUNC
nor leaving them at STT_NOTYPE), but the small stubs then want an end
annotation, so that their code is covered by some [start,start+size)
pair in the symbol table. IOW I think that as a final result (not
necessarily right after this series) we want all code and data
contributions to be covered by such a range. Which in turn means for
this series that _if_ an area is touched, it should be brought into
that intended shape.

Jan

Re: [PATCH 2/4] xen: Port linkage.h from kernel code

[Jane Malalane]

On 05/08/2022 10:24, Jan Beulich wrote:
> On 04.08.2022 17:04, Jane Malalane wrote:
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> 
> In the title you say "port", but then you don't say what customization
> you've done beyond the obvious adjustment of inclusion guard and
> adjustment (actually removal) of #include-s. How much customization we
> want to do is important here, after all. I notice you did, for example,
> add new flavors of SYM_INNER_LABEL, but then you didn't add anything to
> use .hidden (which I have queued as a new patch on top of a supposed v2
> of "x86: annotate entry points with type and size"). At the same time
> you've left in objtool related commentary, when we don't use that tool
> (and no-one knows if we ever will).
> 
> I'm further not sure I agree with the naming of some of your additions,
> as they appear to not really fit with the naming model used elsewhere.
> Your additions also look to not always match style used elsewhere in
> this file.
> 
> You further want to mention what Linux version this was derived from,
> to make it easier to determine what incremental changes to port over
> subsequently.
> 
> Overall I'm not convinced this is a model we want to go with, first
> and foremost because the commonly used macro names are too long for
> my taste. What's wrong with ENTRY() and ENDPROC() / ENDDATA()?
Hi Jan,

Since I have no particular opinion on this I went through the discussion 
that took place before those macros were introduced in Linux. One of the 
points made was that PROC was an ambiguous term to use, since C 
functions are not actually procedures, at least not all. The other was 
that using START/END markers make it easier for a developer to remember 
to add the END marker, and eventhough you suggest using ENTRY and not 
just PROC as the start marker, START might still be clearer.

I welcome other input.

Thank you for your review,

Jane.

Re: [PATCH 2/4] xen: Port linkage.h from kernel code

[Jan Beulich]

On 16.08.2022 12:16, Jane Malalane wrote:
> On 05/08/2022 10:24, Jan Beulich wrote:
>> On 04.08.2022 17:04, Jane Malalane wrote:
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
>>
>> In the title you say "port", but then you don't say what customization
>> you've done beyond the obvious adjustment of inclusion guard and
>> adjustment (actually removal) of #include-s. How much customization we
>> want to do is important here, after all. I notice you did, for example,
>> add new flavors of SYM_INNER_LABEL, but then you didn't add anything to
>> use .hidden (which I have queued as a new patch on top of a supposed v2
>> of "x86: annotate entry points with type and size"). At the same time
>> you've left in objtool related commentary, when we don't use that tool
>> (and no-one knows if we ever will).
>>
>> I'm further not sure I agree with the naming of some of your additions,
>> as they appear to not really fit with the naming model used elsewhere.
>> Your additions also look to not always match style used elsewhere in
>> this file.
>>
>> You further want to mention what Linux version this was derived from,
>> to make it easier to determine what incremental changes to port over
>> subsequently.
>>
>> Overall I'm not convinced this is a model we want to go with, first
>> and foremost because the commonly used macro names are too long for
>> my taste. What's wrong with ENTRY() and ENDPROC() / ENDDATA()?
> Hi Jan,
> 
> Since I have no particular opinion on this I went through the discussion 
> that took place before those macros were introduced in Linux. One of the 
> points made was that PROC was an ambiguous term to use, since C 
> functions are not actually procedures, at least not all.

Just one remark here: We're talking about assembly code here, so what's
a procedure or function isn't well defined anyway. I wouldn't, btw, mind
ENDFUNC() if that's deemed better than ENDPROC().

Jan

> The other was 
> that using START/END markers make it easier for a developer to remember 
> to add the END marker, and eventhough you suggest using ENTRY and not 
> just PROC as the start marker, START might still be clearer.
> 
> I welcome other input.
> 
> Thank you for your review,
> 
> Jane.

Re: [PATCH 2/4] xen: Port linkage.h from kernel code

[Jane Malalane]

On 16/08/2022 14:06, Jan Beulich wrote:
> On 16.08.2022 12:16, Jane Malalane wrote:
>> On 05/08/2022 10:24, Jan Beulich wrote:
>>> On 04.08.2022 17:04, Jane Malalane wrote:
>>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
>>>
>>> In the title you say "port", but then you don't say what customization
>>> you've done beyond the obvious adjustment of inclusion guard and
>>> adjustment (actually removal) of #include-s. How much customization we
>>> want to do is important here, after all. I notice you did, for example,
>>> add new flavors of SYM_INNER_LABEL, but then you didn't add anything to
>>> use .hidden (which I have queued as a new patch on top of a supposed v2
>>> of "x86: annotate entry points with type and size"). At the same time
>>> you've left in objtool related commentary, when we don't use that tool
>>> (and no-one knows if we ever will).
>>>
>>> I'm further not sure I agree with the naming of some of your additions,
>>> as they appear to not really fit with the naming model used elsewhere.
>>> Your additions also look to not always match style used elsewhere in
>>> this file.
>>>
>>> You further want to mention what Linux version this was derived from,
>>> to make it easier to determine what incremental changes to port over
>>> subsequently.
>>>
>>> Overall I'm not convinced this is a model we want to go with, first
>>> and foremost because the commonly used macro names are too long for
>>> my taste. What's wrong with ENTRY() and ENDPROC() / ENDDATA()?
>> Hi Jan,
>>
>> Since I have no particular opinion on this I went through the discussion
>> that took place before those macros were introduced in Linux. One of the
>> points made was that PROC was an ambiguous term to use, since C
>> functions are not actually procedures, at least not all.
> 
> Just one remark here: We're talking about assembly code here, so what's
> a procedure or function isn't well defined anyway. I wouldn't, btw, mind
> ENDFUNC() if that's deemed better than ENDPROC().
Do you then propose that we use ENTRY() and ENDFUNC() and that for inner 
labels we keep them as is (so just "name:"), since using ENTRY() without 
a closing END() for some "functions" and not for others could get a bit 
confusing?

Jane.

Re: [PATCH 2/4] xen: Port linkage.h from kernel code

[Jan Beulich]

On 17.08.2022 10:56, Jane Malalane wrote:
> On 16/08/2022 14:06, Jan Beulich wrote:
>> On 16.08.2022 12:16, Jane Malalane wrote:
>>> On 05/08/2022 10:24, Jan Beulich wrote:
>>>> On 04.08.2022 17:04, Jane Malalane wrote:
>>>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
>>>>
>>>> In the title you say "port", but then you don't say what customization
>>>> you've done beyond the obvious adjustment of inclusion guard and
>>>> adjustment (actually removal) of #include-s. How much customization we
>>>> want to do is important here, after all. I notice you did, for example,
>>>> add new flavors of SYM_INNER_LABEL, but then you didn't add anything to
>>>> use .hidden (which I have queued as a new patch on top of a supposed v2
>>>> of "x86: annotate entry points with type and size"). At the same time
>>>> you've left in objtool related commentary, when we don't use that tool
>>>> (and no-one knows if we ever will).
>>>>
>>>> I'm further not sure I agree with the naming of some of your additions,
>>>> as they appear to not really fit with the naming model used elsewhere.
>>>> Your additions also look to not always match style used elsewhere in
>>>> this file.
>>>>
>>>> You further want to mention what Linux version this was derived from,
>>>> to make it easier to determine what incremental changes to port over
>>>> subsequently.
>>>>
>>>> Overall I'm not convinced this is a model we want to go with, first
>>>> and foremost because the commonly used macro names are too long for
>>>> my taste. What's wrong with ENTRY() and ENDPROC() / ENDDATA()?
>>> Hi Jan,
>>>
>>> Since I have no particular opinion on this I went through the discussion
>>> that took place before those macros were introduced in Linux. One of the
>>> points made was that PROC was an ambiguous term to use, since C
>>> functions are not actually procedures, at least not all.
>>
>> Just one remark here: We're talking about assembly code here, so what's
>> a procedure or function isn't well defined anyway. I wouldn't, btw, mind
>> ENDFUNC() if that's deemed better than ENDPROC().
> Do you then propose that we use ENTRY() and ENDFUNC() and that for inner 
> labels we keep them as is (so just "name:"), since using ENTRY() without 
> a closing END() for some "functions" and not for others could get a bit 
> confusing?

Almost - I don't see anything wrong with using ENTRY() without any END*()
for inner labels, if the implied alignment is wanted. If no alignment nor
type / size are wanted, we have GLOBAL(). And recall that I already did
post a patch adding various ENDPROC() (which could be converted to
ENDFUNC() if that name is indeed liked better, and which could easily
have ENDDATA() or some such added), see
https://lists.xen.org/archives/html/xen-devel/2022-04/msg00876.html .
With Andrew's odd reply I didn't see fit to post a v2 so far, where I'm
now having a further patch adding .hidden directives into GLOBAL() and
(indirectly) ENTRY(). Not the least because my reply (where I did already
indicate that Linux'es machinery looks a little too involved to me)
didn't have any further responses, which would at least have helped
clarify what - if anything - I had "rejected" long ago.

Jan