While not immediately useful - a new binutils release would first need cutting - I thought I'd post early the patches utilizing new functionality there. The changes made are largely benign with gas 2.38 or older. 1: improve .debug_line contents for assembly sources 2: annotate entry points with type and size Jan
The model introduced in patch 2 is now arch-agnostic, and all arch-es are being switched at least partly (to at least give examples of how things will look like). 1: common: move a few macros out of xen/lib.h 2: common: assembly entry point type/size annotations 3: x86: annotate entry points with type and size 4: x86: also mark assembler globals hidden 5: Arm: annotate entry points with type and size 6: RISC-V: annotate entry points with type and size 7: PPC: switch entry point annotations to common model 8: tools/binfile: switch to common annotations model Jan
Introduce xen/macros.h for this purpose. For now xen/lib.h simply
includes xen/macro.h, until consumers can be suitable cleaned up.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -1,26 +1,7 @@
#ifndef __LIB_H__
#define __LIB_H__
-#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
-
-#define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))
-
-#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
-#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
-
-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-
-#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
-#define count_args(args...) \
- count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
-
-/* Indirect macros required for expanded argument pasting. */
-#define PASTE_(a, b) a ## b
-#define PASTE(a, b) PASTE_(a, b)
-
-#define __STR(...) #__VA_ARGS__
-#define STR(...) __STR(__VA_ARGS__)
+#include <xen/macros.h>
#ifndef __ASSEMBLY__
--- /dev/null
+++ b/xen/include/xen/macros.h
@@ -0,0 +1,34 @@
+#ifndef __MACROS_H__
+#define __MACROS_H__
+
+#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
+
+#define IS_ALIGNED(val, align) (!((val) & ((align) - 1)))
+
+#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
+#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
+
+#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
+#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+
+#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x
+#define count_args(args...) \
+ count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+/* Indirect macros required for expanded argument pasting. */
+#define PASTE_(a, b) a ## b
+#define PASTE(a, b) PASTE_(a, b)
+
+#define __STR(...) #__VA_ARGS__
+#define STR(...) __STR(__VA_ARGS__)
+
+#endif /* __MACROS_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
On 7/10/23 3:51 AM, Jan Beulich wrote: > Introduce xen/macros.h for this purpose. For now xen/lib.h simply > includes xen/macro.h, until consumers can be suitable cleaned up. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: New. > > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -1,26 +1,7 @@ > #ifndef __LIB_H__ > #define __LIB_H__ > > -#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1)) > - > -#define IS_ALIGNED(val, align) (!((val) & ((align) - 1))) > - > -#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > -#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > - > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > - > -#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > -#define count_args(args...) \ > - count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) > - > -/* Indirect macros required for expanded argument pasting. */ > -#define PASTE_(a, b) a ## b > -#define PASTE(a, b) PASTE_(a, b) > - > -#define __STR(...) #__VA_ARGS__ > -#define STR(...) __STR(__VA_ARGS__) > +#include <xen/macros.h> > > #ifndef __ASSEMBLY__ > > --- /dev/null > +++ b/xen/include/xen/macros.h > @@ -0,0 +1,34 @@ Should there be an SPDX header here? > +#ifndef __MACROS_H__ > +#define __MACROS_H__ > + > +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1)) > + > +#define IS_ALIGNED(val, align) (!((val) & ((align) - 1))) > + > +#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > + > +#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > + > +#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > +#define count_args(args...) \ > + count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) > + > +/* Indirect macros required for expanded argument pasting. */ > +#define PASTE_(a, b) a ## b > +#define PASTE(a, b) PASTE_(a, b) > + > +#define __STR(...) #__VA_ARGS__ > +#define STR(...) __STR(__VA_ARGS__) > + > +#endif /* __MACROS_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ Reviewed-by: Shawn Anastasio <sanastasio@raptorengineering.com>
On 18.07.2023 21:49, Shawn Anastasio wrote: > On 7/10/23 3:51 AM, Jan Beulich wrote: >> --- /dev/null >> +++ b/xen/include/xen/macros.h >> @@ -0,0 +1,34 @@ > > Should there be an SPDX header here? Probably, but I wouldn't know which version to use, since lib.h doesn't have one either. Not putting one there leaves things a clear or vague as they are for others without the tag (and without a full license header); putting one there would firmly state something, which then may be wrong. Therefore I think this will need sorting in (more or less) one go for all of the "license-free" source files. >> +#ifndef __MACROS_H__ >> +#define __MACROS_H__ >> + >> +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1)) >> + >> +#define IS_ALIGNED(val, align) (!((val) & ((align) - 1))) >> + >> +#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) >> +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) >> + >> +#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) >> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) >> + >> +#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x >> +#define count_args(args...) \ >> + count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) >> + >> +/* Indirect macros required for expanded argument pasting. */ >> +#define PASTE_(a, b) a ## b >> +#define PASTE(a, b) PASTE_(a, b) >> + >> +#define __STR(...) #__VA_ARGS__ >> +#define STR(...) __STR(__VA_ARGS__) >> + >> +#endif /* __MACROS_H__ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ > > Reviewed-by: Shawn Anastasio <sanastasio@raptorengineering.com> Thanks. Jan
On Mon, 2023-07-10 at 10:51 +0200, Jan Beulich wrote: > Introduce xen/macros.h for this purpose. For now xen/lib.h simply > includes xen/macro.h, until consumers can be suitable cleaned up. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: New. > > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -1,26 +1,7 @@ > #ifndef __LIB_H__ > #define __LIB_H__ > > -#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1)) > - > -#define IS_ALIGNED(val, align) (!((val) & ((align) - 1))) > - > -#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > -#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > - > -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > - > -#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > -#define count_args(args...) \ > - count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) > - > -/* Indirect macros required for expanded argument pasting. */ > -#define PASTE_(a, b) a ## b > -#define PASTE(a, b) PASTE_(a, b) > - > -#define __STR(...) #__VA_ARGS__ > -#define STR(...) __STR(__VA_ARGS__) > +#include <xen/macros.h> > > #ifndef __ASSEMBLY__ > > --- /dev/null > +++ b/xen/include/xen/macros.h > @@ -0,0 +1,34 @@ > +#ifndef __MACROS_H__ > +#define __MACROS_H__ > + > +#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1)) > + > +#define IS_ALIGNED(val, align) (!((val) & ((align) - 1))) > + > +#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d)) > +#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > + > +#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) > +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) > + > +#define count_args_(dot, a1, a2, a3, a4, a5, a6, a7, a8, x, ...) x > +#define count_args(args...) \ > + count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) > + > +/* Indirect macros required for expanded argument pasting. */ > +#define PASTE_(a, b) a ## b > +#define PASTE(a, b) PASTE_(a, b) > + > +#define __STR(...) #__VA_ARGS__ > +#define STR(...) __STR(__VA_ARGS__) > + > +#endif /* __MACROS_H__ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii
Recent gas versions generate minimalistic Dwarf debug info for items
annotated as functions and having their sizes specified [1]. Furthermore
generating live patches wants items properly annotated. "Borrow" Arm's
END() and (remotely) derive other annotation infrastructure from
Linux'es, for all architectures to use.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28
---
v3: New, generalized from earlier x86-only version. LAST() (now
LASTARG()) moved to macros.h.
---
TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es
to define that in all cases?
TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be
specified (in case this has some special meaning on an arch;
conceivably it could mean to use some kind of arch default). We may
not strictly need that, and hence we could also make these power-of
-2 values (using .p2align).
Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we
still have ALIGN.
Note further that FUNC()'s etc "algn" parameter is intended to allow for
only no or a single argument. If we wanted to also make the fill value
customizable per call site, the constructs would need re-doing to some
degree.
--- /dev/null
+++ b/xen/include/xen/linkage.h
@@ -0,0 +1,56 @@
+#ifndef __LINKAGE_H__
+#define __LINKAGE_H__
+
+#ifdef __ASSEMBLY__
+
+#include <xen/macros.h>
+
+#ifndef CODE_ALIGN
+# define CODE_ALIGN ??
+#endif
+#ifndef CODE_FILL
+# define CODE_FILL ~0
+#endif
+
+#ifndef DATA_ALIGN
+# define DATA_ALIGN 0
+#endif
+#ifndef DATA_FILL
+# define DATA_FILL ~0
+#endif
+
+#define SYM_ALIGN(algn...) .balign algn
+
+#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_WEAK(name) .weak name
+#define SYM_L_LOCAL(name) /* nothing */
+
+#define SYM_T_FUNC STT_FUNC
+#define SYM_T_DATA STT_OBJECT
+#define SYM_T_NONE STT_NOTYPE
+
+#define SYM(name, typ, linkage, algn...) \
+ .type name, SYM_T_ ## typ; \
+ SYM_L_ ## linkage(name); \
+ SYM_ALIGN(algn); \
+ name:
+
+#define END(name) .size name, . - name
+
+#define FUNC(name, algn...) \
+ SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define LABEL(name, algn...) \
+ SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define DATA(name, algn...) \
+ SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
+
+#define FUNC_LOCAL(name, algn...) \
+ SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define LABEL_LOCAL(name, algn...) \
+ SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define DATA_LOCAL(name, algn...) \
+ SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __LINKAGE_H__ */
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -15,6 +15,15 @@
#define count_args(args...) \
count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+#define ARG1_(x, y...) (x)
+#define ARG2_(x, y...) ARG1_(y)
+#define ARG3_(x, y...) ARG2_(y)
+#define ARG4_(x, y...) ARG3_(y)
+
+#define ARG__(nr) ARG ## nr ## _
+#define ARG_(nr) ARG__(nr)
+#define LASTARG(x, y...) ARG_(count_args(x, ## y))(x, ## y)
+
/* Indirect macros required for expanded argument pasting. */
#define PASTE_(a, b) a ## b
#define PASTE(a, b) PASTE_(a, b)
On 10.07.2023 10:52, Jan Beulich wrote: > Recent gas versions generate minimalistic Dwarf debug info for items > annotated as functions and having their sizes specified [1]. Furthermore > generating live patches wants items properly annotated. "Borrow" Arm's > END() and (remotely) derive other annotation infrastructure from > Linux'es, for all architectures to use. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Looking over my action items from Prague I notice that I forgot one aspect here that was asked for by Andrew: To have the new framework also respect CONFIG_CC_SPLIT_SECTIONS. I'll work on that next, but perhaps better as in incremental patch on top. Jan
Use the generic framework in xen/linkage.h.
For switch_to_kernel() and restore_all_guest() so far implicit alignment
(from being first in their respective sections) is being made explicit
(as in: using FUNC() without 2nd argument). Whereas for
{,compat}create_bounce_frame() and autogen_entrypoints[] alignment is
newly arranged for.
Except for the added/adjusted alignment padding (including their
knock-on effects) no change in generated code/data. Note that the basis
for support of weak definitions is added despite them not having any use
right now.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: !PV variant of ret_from_intr is local. Introduction of macros split
off to separate patch. Also adjust ASM_INT(). Re-base.
v2: Full rework.
---
Only two of the assembly files are being converted for now. More could
be done right here or as follow-on in separate patches.
Note that the FB-label in autogen_stubs() cannot be converted just yet:
Such labels cannot be used with .type. We could further diverge from
Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type
labels get by default anyway).
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
I didn't think that I should make CODE_FILL evaluate to 0xCC right here;
IMO that wants to be a separate patch.
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -13,6 +13,7 @@
#include <asm/alternative.h>
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#include <asm/asm-defns.h>
#ifndef CONFIG_INDIRECT_THUNK
.equ CONFIG_INDIRECT_THUNK, 0
@@ -343,10 +344,7 @@ static always_inline void stac(void)
.popsection
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, @object
+ DATA(label, 4) .long (val); END(label)
#define ASM_CONSTANT(name, value) \
asm ( ".equ " #name ", %P0; .global " #name \
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -43,7 +43,9 @@
/* Linkage for x86 */
#ifdef __ASSEMBLY__
-#define ALIGN .align 16,0x90
+#define CODE_ALIGN 16
+#define CODE_FILL 0x90
+#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
.globl name; \
ALIGN; \
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -11,7 +11,7 @@
#include <public/xen.h>
#include <irq_vectors.h>
-ENTRY(entry_int82)
+FUNC(entry_int82)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -27,9 +27,10 @@ ENTRY(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+END(entry_int82)
/* %rbx: struct vcpu */
-ENTRY(compat_test_all_events)
+FUNC(compat_test_all_events)
ASSERT_NOT_IN_ATOMIC
cli # tests must not race interrupts
/*compat_test_softirqs:*/
@@ -66,24 +67,21 @@ compat_test_guest_events:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_softirqs:
+LABEL_LOCAL(compat_process_softirqs)
sti
call do_softirq
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx: struct trap_bounce */
-.Lcompat_process_trapbounce:
+LABEL_LOCAL(.Lcompat_process_trapbounce)
sti
.Lcompat_bounce_exception:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_mce:
+LABEL_LOCAL(compat_process_mce)
testb $1 << VCPU_TRAP_MCE,VCPU_async_exception_mask(%rbx)
jnz .Lcompat_test_guest_nmi
sti
@@ -97,9 +95,8 @@ compat_process_mce:
movb %dl,VCPU_async_exception_mask(%rbx)
jmp compat_process_trap
- ALIGN
/* %rbx: struct vcpu */
-compat_process_nmi:
+LABEL_LOCAL(compat_process_nmi)
testb $1 << VCPU_TRAP_NMI,VCPU_async_exception_mask(%rbx)
jnz compat_test_guest_events
sti
@@ -116,9 +113,10 @@ compat_process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_test_all_events)
/* %rbx: struct vcpu, interrupts disabled */
-ENTRY(compat_restore_all_guest)
+FUNC(compat_restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
mov $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11d
and UREGS_eflags(%rsp),%r11d
@@ -161,9 +159,10 @@ ENTRY(compat_restore_all_guest)
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(compat_restore_all_guest)
/* This mustn't modify registers other than %rax. */
-ENTRY(cr4_pv32_restore)
+FUNC(cr4_pv32_restore)
push %rdx
GET_CPUINFO_FIELD(cr4, dx)
mov (%rdx), %rax
@@ -193,8 +192,9 @@ ENTRY(cr4_pv32_restore)
pop %rdx
xor %eax, %eax
ret
+END(cr4_pv32_restore)
-ENTRY(compat_syscall)
+FUNC(compat_syscall)
/* Fix up reported %cs/%ss for compat domains. */
movl $FLAT_COMPAT_USER_SS, UREGS_ss(%rsp)
movl $FLAT_COMPAT_USER_CS, UREGS_cs(%rsp)
@@ -222,8 +222,9 @@ UNLIKELY_END(compat_syscall_gpf)
movw %si,TRAPBOUNCE_cs(%rdx)
movb %cl,TRAPBOUNCE_flags(%rdx)
jmp .Lcompat_bounce_exception
+END(compat_syscall)
-ENTRY(compat_sysenter)
+FUNC(compat_sysenter)
CR4_PV32_RESTORE
movq VCPU_trap_ctxt(%rbx),%rcx
cmpb $X86_EXC_GP, UREGS_entry_vector(%rsp)
@@ -236,17 +237,19 @@ ENTRY(compat_sysenter)
movw %ax,TRAPBOUNCE_cs(%rdx)
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_sysenter)
-ENTRY(compat_int80_direct_trap)
+FUNC(compat_int80_direct_trap)
CR4_PV32_RESTORE
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_int80_direct_trap)
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK: */
/* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-compat_create_bounce_frame:
+FUNC_LOCAL(compat_create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
mov %fs,%edi
ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
@@ -352,3 +355,4 @@ compat_crash_page_fault:
jmp .Lft14
.previous
_ASM_EXTABLE(.Lft14, .Lfx14)
+END(compat_create_bounce_frame)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -24,7 +24,7 @@
#ifdef CONFIG_PV
/* %rbx: struct vcpu */
-switch_to_kernel:
+FUNC_LOCAL(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
/* TB_eip = 32-bit syscall ? syscall32_addr : syscall_addr */
@@ -89,24 +89,21 @@ test_guest_events:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_softirqs:
+LABEL_LOCAL(process_softirqs)
sti
call do_softirq
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx struct trap_bounce */
-.Lprocess_trapbounce:
+LABEL_LOCAL(.Lprocess_trapbounce)
sti
.Lbounce_exception:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_mce:
+LABEL_LOCAL(process_mce)
testb $1 << VCPU_TRAP_MCE, VCPU_async_exception_mask(%rbx)
jnz .Ltest_guest_nmi
sti
@@ -120,9 +117,8 @@ process_mce:
movb %dl, VCPU_async_exception_mask(%rbx)
jmp process_trap
- ALIGN
/* %rbx: struct vcpu */
-process_nmi:
+LABEL_LOCAL(process_nmi)
testb $1 << VCPU_TRAP_NMI, VCPU_async_exception_mask(%rbx)
jnz test_guest_events
sti
@@ -139,11 +135,12 @@ process_trap:
leaq VCPU_trap_bounce(%rbx), %rdx
call create_bounce_frame
jmp test_all_events
+END(switch_to_kernel)
.section .text.entry, "ax", @progbits
/* %rbx: struct vcpu, interrupts disabled */
-restore_all_guest:
+FUNC_LOCAL(restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
/* Stash guest SPEC_CTRL value while we can read struct vcpu. */
@@ -220,8 +217,7 @@ restore_all_guest:
sysretq
1: sysretl
- ALIGN
-.Lrestore_rcx_iret_exit_to_guest:
+LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_guest)
movq 8(%rsp), %rcx # RIP
/* No special register assumptions. */
iret_exit_to_guest:
@@ -230,6 +226,7 @@ iret_exit_to_guest:
addq $8,%rsp
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(restore_all_guest)
/*
* When entering SYSCALL from kernel mode:
@@ -246,7 +243,7 @@ iret_exit_to_guest:
* - Guest %rsp stored in %rax
* - Xen stack loaded, pointing at the %ss slot
*/
-ENTRY(lstar_enter)
+FUNC(lstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -281,9 +278,10 @@ ENTRY(lstar_enter)
mov %rsp, %rdi
call pv_hypercall
jmp test_all_events
+END(lstar_enter)
/* See lstar_enter for entry register state. */
-ENTRY(cstar_enter)
+FUNC(cstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -321,8 +319,9 @@ ENTRY(cstar_enter)
jne compat_syscall
#endif
jmp switch_to_kernel
+END(cstar_enter)
-ENTRY(sysenter_entry)
+FUNC(sysenter_entry)
ENDBR64
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
@@ -331,7 +330,7 @@ ENTRY(sysenter_entry)
pushq $FLAT_USER_SS
pushq $0
pushfq
-GLOBAL(sysenter_eflags_saved)
+LABEL(sysenter_eflags_saved, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $3 /* ring 3 null cs */
pushq $0 /* null rip */
@@ -385,8 +384,9 @@ UNLIKELY_END(sysenter_gpf)
jne compat_sysenter
#endif
jmp .Lbounce_exception
+END(sysenter_entry)
-ENTRY(int80_direct_trap)
+FUNC(int80_direct_trap)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -474,6 +474,7 @@ int80_slow_path:
*/
GET_STACK_END(14)
jmp handle_exception_saved
+END(int80_direct_trap)
/* create_bounce_frame & helpers don't need to be in .text.entry */
.text
@@ -482,7 +483,7 @@ int80_slow_path:
/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-create_bounce_frame:
+FUNC_LOCAL(create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jnz 1f
@@ -618,6 +619,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+END(create_bounce_frame)
#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -626,7 +628,7 @@ ENTRY(dom_crash_sync_extable)
/* No special register assumptions. */
#ifdef CONFIG_PV
-ENTRY(continue_pv_domain)
+FUNC(continue_pv_domain)
ENDBR64
call check_wakeup_from_wait
ret_from_intr:
@@ -641,26 +643,28 @@ ret_from_intr:
#else
jmp test_all_events
#endif
+END(continue_pv_domain)
#else
-ret_from_intr:
+FUNC_LOCAL(ret_from_intr, 0)
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
+END(ret_from_intr)
#endif
.section .init.text, "ax", @progbits
-ENTRY(early_page_fault)
+FUNC(early_page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
SAVE_ALL
movq %rsp, %rdi
call do_early_page_fault
jmp restore_all_xen
+END(early_page_fault)
.section .text.entry, "ax", @progbits
- ALIGN
/* No special register assumptions. */
-restore_all_xen:
+FUNC_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).
@@ -677,8 +681,9 @@ UNLIKELY_END(exit_cr3)
RESTORE_ALL adj=8
iretq
+END(restore_all_xen)
-ENTRY(common_interrupt)
+FUNC(common_interrupt)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -707,12 +712,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
+END(common_interrupt)
-ENTRY(page_fault)
+FUNC(page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
+END(page_fault)
/* No special register assumptions. */
-GLOBAL(handle_exception)
+FUNC(handle_exception, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -880,92 +887,108 @@ FATAL_exception_with_ints_disabled:
xorl %esi,%esi
movq %rsp,%rdi
tailcall fatal_trap
+END(handle_exception)
-ENTRY(divide_error)
+FUNC(divide_error)
ENDBR64
pushq $0
movl $X86_EXC_DE, 4(%rsp)
jmp handle_exception
+END(divide_error)
-ENTRY(coprocessor_error)
+FUNC(coprocessor_error)
ENDBR64
pushq $0
movl $X86_EXC_MF, 4(%rsp)
jmp handle_exception
+END(coprocessor_error)
-ENTRY(simd_coprocessor_error)
+FUNC(simd_coprocessor_error)
ENDBR64
pushq $0
movl $X86_EXC_XM, 4(%rsp)
jmp handle_exception
+END(coprocessor_error)
-ENTRY(device_not_available)
+FUNC(device_not_available)
ENDBR64
pushq $0
movl $X86_EXC_NM, 4(%rsp)
jmp handle_exception
+END(device_not_available)
-ENTRY(debug)
+FUNC(debug)
ENDBR64
pushq $0
movl $X86_EXC_DB, 4(%rsp)
jmp handle_ist_exception
+END(debug)
-ENTRY(int3)
+FUNC(int3)
ENDBR64
pushq $0
movl $X86_EXC_BP, 4(%rsp)
jmp handle_exception
+END(int3)
-ENTRY(overflow)
+FUNC(overflow)
ENDBR64
pushq $0
movl $X86_EXC_OF, 4(%rsp)
jmp handle_exception
+END(overflow)
-ENTRY(bounds)
+FUNC(bounds)
ENDBR64
pushq $0
movl $X86_EXC_BR, 4(%rsp)
jmp handle_exception
+END(bounds)
-ENTRY(invalid_op)
+FUNC(invalid_op)
ENDBR64
pushq $0
movl $X86_EXC_UD, 4(%rsp)
jmp handle_exception
+END(invalid_op)
-ENTRY(invalid_TSS)
+FUNC(invalid_TSS)
ENDBR64
movl $X86_EXC_TS, 4(%rsp)
jmp handle_exception
+END(invalid_TSS)
-ENTRY(segment_not_present)
+FUNC(segment_not_present)
ENDBR64
movl $X86_EXC_NP, 4(%rsp)
jmp handle_exception
+END(segment_not_present)
-ENTRY(stack_segment)
+FUNC(stack_segment)
ENDBR64
movl $X86_EXC_SS, 4(%rsp)
jmp handle_exception
+END(stack_segment)
-ENTRY(general_protection)
+FUNC(general_protection)
ENDBR64
movl $X86_EXC_GP, 4(%rsp)
jmp handle_exception
+END(general_protection)
-ENTRY(alignment_check)
+FUNC(alignment_check)
ENDBR64
movl $X86_EXC_AC, 4(%rsp)
jmp handle_exception
+END(alignment_check)
-ENTRY(entry_CP)
+FUNC(entry_CP)
ENDBR64
movl $X86_EXC_CP, 4(%rsp)
jmp handle_exception
+END(entry_CP)
-ENTRY(double_fault)
+FUNC(double_fault)
ENDBR64
movl $X86_EXC_DF, 4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
@@ -988,8 +1011,9 @@ ENTRY(double_fault)
movq %rsp,%rdi
tailcall do_double_fault
+END(double_fault)
-ENTRY(nmi)
+FUNC(nmi)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
@@ -1116,21 +1140,24 @@ handle_ist_exception:
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
#endif
+END(nmi)
-ENTRY(machine_check)
+FUNC(machine_check)
ENDBR64
pushq $0
movl $X86_EXC_MC, 4(%rsp)
jmp handle_ist_exception
+END(machine_check)
/* No op trap handler. Required for kexec crash path. */
-GLOBAL(trap_nop)
+FUNC(trap_nop, 0)
ENDBR64
iretq
+END(trap_nop)
/* Table of automatically generated entry points. One per vector. */
.pushsection .init.rodata, "a", @progbits
-GLOBAL(autogen_entrypoints)
+DATA(autogen_entrypoints, 8)
/* pop into the .init.rodata section and record an entry point. */
.macro entrypoint ent
.pushsection .init.rodata, "a", @progbits
@@ -1139,7 +1166,7 @@ GLOBAL(autogen_entrypoints)
.endm
.popsection
-autogen_stubs: /* Automatically generated stubs. */
+FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
vec = 0
.rept X86_NR_VECTORS
@@ -1183,6 +1210,7 @@ autogen_stubs: /* Automatically generate
vec = vec + 1
.endr
+END(autogen_stubs)
.section .init.rodata, "a", @progbits
- .size autogen_entrypoints, . - autogen_entrypoints
+END(autogen_entrypoints)
Let's have assembler symbols be consistent with C ones. In principle
there are (a few) cases where gas can produce smaller code this way,
just that for now there's a gas bug causing smaller code to be emitted
even when that shouldn't be the case.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
v3: Re-base over generalization of the annotations.
v2: New.
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -47,11 +47,11 @@
#define CODE_FILL 0x90
#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
- .globl name; \
ALIGN; \
- name:
+ GLOBAL(name)
#define GLOBAL(name) \
.globl name; \
+ .hidden name; \
name:
#endif
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -21,7 +21,7 @@
#define SYM_ALIGN(algn...) .balign algn
-#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
Use the generic framework in xen/linkage.h. No change in generated code
except for the changed padding value (noticable when config.gz isn't a
multiple of 4 in size). Plus of course the converted symbols change to
be hidden ones.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Only one each of the assembly files is being converted for now. More
could be done right here or as follow-on in separate patches.
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
---
v3: New.
--- a/xen/arch/arm/arm32/lib/div64.S
+++ b/xen/arch/arm/arm32/lib/div64.S
@@ -42,7 +42,7 @@
* Clobbered regs: xl, ip
*/
-ENTRY(__do_div64)
+FUNC(__do_div64)
UNWIND(.fnstart)
@ Test for easy paths first.
@@ -206,4 +206,4 @@ Ldiv0_64:
ldr pc, [sp], #8
UNWIND(.fnend)
-ENDPROC(__do_div64)
+END(__do_div64)
--- a/xen/arch/arm/arm64/lib/clear_page.S
+++ b/xen/arch/arm/arm64/lib/clear_page.S
@@ -22,7 +22,7 @@
* Parameters:
* x0 - dest
*/
-ENTRY(clear_page)
+FUNC(clear_page)
mrs x1, dczid_el0
and w1, w1, #0xf
mov x2, #4
@@ -33,4 +33,4 @@ ENTRY(clear_page)
tst x0, #(PAGE_SIZE - 1)
b.ne 1b
ret
-ENDPROC(clear_page)
+END(clear_page)
--- a/xen/arch/arm/include/asm/asm_defns.h
+++ b/xen/arch/arm/include/asm/asm_defns.h
@@ -5,6 +5,7 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
+#include <xen/linkage.h>
#include <asm/processor.h>
/* Macros for generic assembly code */
@@ -28,10 +29,7 @@ label: .asciz msg;
.popsection
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, %object
+ DATA(label, 4) .long (val); END(label)
#endif /* __ARM_ASM_DEFNS_H__ */
/*
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -53,7 +53,8 @@
/* Linkage for ARM */
#ifdef __ASSEMBLY__
-#define ALIGN .align 2
+#define CODE_ALIGN 4
+#define ALIGN .balign CODE_ALIGN
#define ENTRY(name) \
.globl name; \
ALIGN; \
@@ -61,8 +62,6 @@
#define GLOBAL(name) \
.globl name; \
name:
-#define END(name) \
- .size name, .-name
#define ENDPROC(name) \
.type name, %function; \
END(name)
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbols change to be hidden ones and gain
a valid size.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Probably count_args_exp() should move to macros.h, but I first wanted to
see whether anyone can suggest any better approach for checking whether
a defined macro expands to nothing.
---
v3: New.
--- a/xen/arch/riscv/entry.S
+++ b/xen/arch/riscv/entry.S
@@ -5,7 +5,7 @@
#include <asm/traps.h>
/* WIP: only works while interrupting Xen context */
-ENTRY(handle_trap)
+FUNC(handle_trap)
/* Exceptions from xen */
save_to_stack:
@@ -92,3 +92,4 @@ restore_registers:
REG_L sp, CPU_USER_REGS_SP(sp)
sret
+END(handle_trap)
--- a/xen/arch/riscv/include/asm/asm.h
+++ b/xen/arch/riscv/include/asm/asm.h
@@ -7,6 +7,7 @@
#define _ASM_RISCV_ASM_H
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#define __ASM_STR(x) x
#else
#define __ASM_STR(x) #x
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -67,12 +67,8 @@
/* Linkage for RISCV */
#ifdef __ASSEMBLY__
-#define ALIGN .align 4
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 16
+#define CODE_FILL /* empty */
#endif
#ifdef CONFIG_RISCV_64
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -8,7 +8,7 @@
* a0 -> hart_id ( bootcpu_id )
* a1 -> dtb_base
*/
-ENTRY(start)
+FUNC(start)
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -30,13 +30,14 @@ ENTRY(start)
jal reset_stack
tail start_xen
+END(start)
.section .text, "ax", %progbits
-ENTRY(reset_stack)
+FUNC(reset_stack)
la sp, cpu0_boot_stack
li t0, STACK_SIZE
add sp, sp, t0
ret
-
+END(reset_stack)
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -37,17 +37,28 @@
#define END(name) .size name, . - name
+/*
+ * CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
+ * which case we also need to get rid of the comma in the .balign directive.
+ */
+#define count_args_exp(args...) count_args(args)
+#if count_args_exp(CODE_FILL)
+# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn), CODE_FILL
+#else
+# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn)
+#endif
+
#define FUNC(name, algn...) \
- SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn))
#define LABEL(name, algn...) \
- SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn))
#define DATA(name, algn...) \
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
#define FUNC_LOCAL(name, algn...) \
- SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn))
#define LABEL_LOCAL(name, algn...) \
- SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn))
#define DATA_LOCAL(name, algn...) \
SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
On 10.07.2023 10:56, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except of course the converted symbols change to be hidden ones and gain > a valid size. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I'm sorry, the Cc list was incomplete here. Adding back the remaining REST maintainers. Jan > --- > Probably count_args_exp() should move to macros.h, but I first wanted to > see whether anyone can suggest any better approach for checking whether > a defined macro expands to nothing. > --- > v3: New. > > --- a/xen/arch/riscv/entry.S > +++ b/xen/arch/riscv/entry.S > @@ -5,7 +5,7 @@ > #include <asm/traps.h> > > /* WIP: only works while interrupting Xen context */ > -ENTRY(handle_trap) > +FUNC(handle_trap) > > /* Exceptions from xen */ > save_to_stack: > @@ -92,3 +92,4 @@ restore_registers: > REG_L sp, CPU_USER_REGS_SP(sp) > > sret > +END(handle_trap) > --- a/xen/arch/riscv/include/asm/asm.h > +++ b/xen/arch/riscv/include/asm/asm.h > @@ -7,6 +7,7 @@ > #define _ASM_RISCV_ASM_H > > #ifdef __ASSEMBLY__ > +#include <xen/linkage.h> > #define __ASM_STR(x) x > #else > #define __ASM_STR(x) #x > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -67,12 +67,8 @@ > > /* Linkage for RISCV */ > #ifdef __ASSEMBLY__ > -#define ALIGN .align 4 > - > -#define ENTRY(name) \ > - .globl name; \ > - ALIGN; \ > - name: > +#define CODE_ALIGN 16 > +#define CODE_FILL /* empty */ > #endif > > #ifdef CONFIG_RISCV_64 > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -8,7 +8,7 @@ > * a0 -> hart_id ( bootcpu_id ) > * a1 -> dtb_base > */ > -ENTRY(start) > +FUNC(start) > /* Mask all interrupts */ > csrw CSR_SIE, zero > > @@ -30,13 +30,14 @@ ENTRY(start) > jal reset_stack > > tail start_xen > +END(start) > > .section .text, "ax", %progbits > > -ENTRY(reset_stack) > +FUNC(reset_stack) > la sp, cpu0_boot_stack > li t0, STACK_SIZE > add sp, sp, t0 > > ret > - > +END(reset_stack) > --- a/xen/include/xen/linkage.h > +++ b/xen/include/xen/linkage.h > @@ -37,17 +37,28 @@ > > #define END(name) .size name, . - name > > +/* > + * CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in > + * which case we also need to get rid of the comma in the .balign directive. > + */ > +#define count_args_exp(args...) count_args(args) > +#if count_args_exp(CODE_FILL) > +# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn), CODE_FILL > +#else > +# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn) > +#endif > + > #define FUNC(name, algn...) \ > - SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > + SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn)) > #define LABEL(name, algn...) \ > - SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > + SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn)) > #define DATA(name, algn...) \ > SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) > > #define FUNC_LOCAL(name, algn...) \ > - SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > + SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn)) > #define LABEL_LOCAL(name, algn...) \ > - SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > + SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn)) > #define DATA_LOCAL(name, algn...) \ > SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) > > >
On Mon, 2023-07-10 at 10:58 +0200, Jan Beulich wrote: > On 10.07.2023 10:56, Jan Beulich wrote: > > Use the generic framework in xen/linkage.h. No change in generated > > code > > except of course the converted symbols change to be hidden ones and > > gain > > a valid size. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I'm sorry, the Cc list was incomplete here. Adding back the remaining > REST > maintainers. > > Jan > > > --- > > Probably count_args_exp() should move to macros.h, but I first > > wanted to > > see whether anyone can suggest any better approach for checking > > whether > > a defined macro expands to nothing. What about introduction of conditional macros ? Something similar to: #include <stdio.h> #define CONDITIONAL_RETURN(arg1, arg2) CONDITIONAL_RETURN_IMPL(arg1, arg2, EMPTY) #define EMPTY(...) "" #define CONDITIONAL_RETURN_IMPL(arg1, arg2, empty_check) \ CONDITIONAL_RETURN_##empty_check(arg1, arg2) #define CONDITIONAL_RETURN_EMPTY(arg1, arg2) \ CONDITIONAL_RETURN_ARG1(arg1, arg2) #define CONDITIONAL_RETURN_ARG1(arg1, arg2) arg1, arg2 #define CONDITIONAL_RETURN_ARG2(arg1, arg2) arg1 int main() { int a = 42; const char* b = "hello"; // Second argument is not empty, both arguments are returned printf("Case 1: %d, %s\n", CONDITIONAL_RETURN(a, b)); // Prints: Case 1: 42, hello // Second argument is empty, only the first argument is returned printf("Case 2: %d, %s\n", CONDITIONAL_RETURN(a, "")); // Prints: Case 2: 42, return 0; } and then define DO_CODE_ALIGN using CONDITIONAL_RETURN? ~ Oleksii > > --- > > v3: New. > > > > --- a/xen/arch/riscv/entry.S > > +++ b/xen/arch/riscv/entry.S > > @@ -5,7 +5,7 @@ > > #include <asm/traps.h> > > > > /* WIP: only works while interrupting Xen context */ > > -ENTRY(handle_trap) > > +FUNC(handle_trap) > > > > /* Exceptions from xen */ > > save_to_stack: > > @@ -92,3 +92,4 @@ restore_registers: > > REG_L sp, CPU_USER_REGS_SP(sp) > > > > sret > > +END(handle_trap) > > --- a/xen/arch/riscv/include/asm/asm.h > > +++ b/xen/arch/riscv/include/asm/asm.h > > @@ -7,6 +7,7 @@ > > #define _ASM_RISCV_ASM_H > > > > #ifdef __ASSEMBLY__ > > +#include <xen/linkage.h> > > #define __ASM_STR(x) x > > #else > > #define __ASM_STR(x) #x > > --- a/xen/arch/riscv/include/asm/config.h > > +++ b/xen/arch/riscv/include/asm/config.h > > @@ -67,12 +67,8 @@ > > > > /* Linkage for RISCV */ > > #ifdef __ASSEMBLY__ > > -#define ALIGN .align 4 > > - > > -#define ENTRY(name) \ > > - .globl name; \ > > - ALIGN; \ > > - name: > > +#define CODE_ALIGN 16 > > +#define CODE_FILL /* empty */ > > #endif > > > > #ifdef CONFIG_RISCV_64 > > --- a/xen/arch/riscv/riscv64/head.S > > +++ b/xen/arch/riscv/riscv64/head.S > > @@ -8,7 +8,7 @@ > > * a0 -> hart_id ( bootcpu_id ) > > * a1 -> dtb_base > > */ > > -ENTRY(start) > > +FUNC(start) > > /* Mask all interrupts */ > > csrw CSR_SIE, zero > > > > @@ -30,13 +30,14 @@ ENTRY(start) > > jal reset_stack > > > > tail start_xen > > +END(start) > > > > .section .text, "ax", %progbits > > > > -ENTRY(reset_stack) > > +FUNC(reset_stack) > > la sp, cpu0_boot_stack > > li t0, STACK_SIZE > > add sp, sp, t0 > > > > ret > > - > > +END(reset_stack) > > --- a/xen/include/xen/linkage.h > > +++ b/xen/include/xen/linkage.h > > @@ -37,17 +37,28 @@ > > > > #define END(name) .size name, . - name > > > > +/* > > + * CODE_FILL in particular may need to expand to nothing (e.g. for > > RISC-V), in > > + * which case we also need to get rid of the comma in the .balign > > directive. > > + */ > > +#define count_args_exp(args...) count_args(args) > > +#if count_args_exp(CODE_FILL) > > +# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn), > > CODE_FILL > > +#else > > +# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn) > > +#endif > > + > > #define FUNC(name, algn...) \ > > - SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), > > CODE_FILL) > > + SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn)) > > #define LABEL(name, algn...) \ > > - SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), > > CODE_FILL) > > + SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn)) > > #define DATA(name, algn...) \ > > SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), > > DATA_FILL) > > > > #define FUNC_LOCAL(name, algn...) \ > > - SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), > > CODE_FILL) > > + SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn)) > > #define LABEL_LOCAL(name, algn...) \ > > - SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), > > CODE_FILL) > > + SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn)) > > #define DATA_LOCAL(name, algn...) \ > > SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), > > DATA_FILL) > >
On 26.07.2023 17:28, Oleksii wrote: > On Mon, 2023-07-10 at 10:58 +0200, Jan Beulich wrote: >> On 10.07.2023 10:56, Jan Beulich wrote: >>> Use the generic framework in xen/linkage.h. No change in generated >>> code >>> except of course the converted symbols change to be hidden ones and >>> gain >>> a valid size. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> I'm sorry, the Cc list was incomplete here. Adding back the remaining >> REST >> maintainers. >> >> Jan >> >>> --- >>> Probably count_args_exp() should move to macros.h, but I first >>> wanted to >>> see whether anyone can suggest any better approach for checking >>> whether >>> a defined macro expands to nothing. > What about introduction of conditional macros ? > Something similar to: > #include <stdio.h> > > #define CONDITIONAL_RETURN(arg1, arg2) CONDITIONAL_RETURN_IMPL(arg1, > arg2, EMPTY) > > #define EMPTY(...) "" > > #define CONDITIONAL_RETURN_IMPL(arg1, arg2, empty_check) \ > CONDITIONAL_RETURN_##empty_check(arg1, arg2) > > #define CONDITIONAL_RETURN_EMPTY(arg1, arg2) \ > CONDITIONAL_RETURN_ARG1(arg1, arg2) > > #define CONDITIONAL_RETURN_ARG1(arg1, arg2) arg1, arg2 > > #define CONDITIONAL_RETURN_ARG2(arg1, arg2) arg1 I don't see how this would be used in your scheme. It ... > int main() { > int a = 42; > const char* b = "hello"; > > // Second argument is not empty, both arguments are returned > printf("Case 1: %d, %s\n", CONDITIONAL_RETURN(a, b)); // Prints: > Case 1: 42, hello > > // Second argument is empty, only the first argument is returned > printf("Case 2: %d, %s\n", CONDITIONAL_RETURN(a, "")); // Prints: > Case 2: 42, ... certainly isn't here, or this likely would cause at least a warning from the compiler (for there being too few arguments to printf()) and then a runtime UB for interpreting something as a pointer to a string which likely isn't. > return 0; > } > > and then define DO_CODE_ALIGN using CONDITIONAL_RETURN? Afaict instead of getting rid of the comma, you'd actually add "" after it. What am I missing? Jan
On Wed, 2023-07-26 at 17:43 +0200, Jan Beulich wrote: > On 26.07.2023 17:28, Oleksii wrote: > > On Mon, 2023-07-10 at 10:58 +0200, Jan Beulich wrote: > > > On 10.07.2023 10:56, Jan Beulich wrote: > > > > Use the generic framework in xen/linkage.h. No change in > > > > generated > > > > code > > > > except of course the converted symbols change to be hidden ones > > > > and > > > > gain > > > > a valid size. > > > > > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > > > I'm sorry, the Cc list was incomplete here. Adding back the > > > remaining > > > REST > > > maintainers. > > > > > > Jan > > > > > > > --- > > > > Probably count_args_exp() should move to macros.h, but I first > > > > wanted to > > > > see whether anyone can suggest any better approach for checking > > > > whether > > > > a defined macro expands to nothing. > > What about introduction of conditional macros ? > > Something similar to: > > #include <stdio.h> > > > > #define CONDITIONAL_RETURN(arg1, arg2) > > CONDITIONAL_RETURN_IMPL(arg1, > > arg2, EMPTY) > > > > #define EMPTY(...) "" > > > > #define CONDITIONAL_RETURN_IMPL(arg1, arg2, empty_check) \ > > CONDITIONAL_RETURN_##empty_check(arg1, arg2) > > > > #define CONDITIONAL_RETURN_EMPTY(arg1, arg2) \ > > CONDITIONAL_RETURN_ARG1(arg1, arg2) > > > > #define CONDITIONAL_RETURN_ARG1(arg1, arg2) arg1, arg2 > > > > #define CONDITIONAL_RETURN_ARG2(arg1, arg2) arg1 > > I don't see how this would be used in your scheme. It ... > > > int main() { > > int a = 42; > > const char* b = "hello"; > > > > // Second argument is not empty, both arguments are returned > > printf("Case 1: %d, %s\n", CONDITIONAL_RETURN(a, b)); // > > Prints: > > Case 1: 42, hello > > > > // Second argument is empty, only the first argument is > > returned > > printf("Case 2: %d, %s\n", CONDITIONAL_RETURN(a, "")); // > > Prints: > > Case 2: 42, > > ... certainly isn't here, or this likely would cause at least a > warning > from the compiler (for there being too few arguments to printf()) and > then a runtime UB for interpreting something as a pointer to a string > which likely isn't. > > > return 0; > > } > > > > and then define DO_CODE_ALIGN using CONDITIONAL_RETURN? > > Afaict instead of getting rid of the comma, you'd actually add "" > after it. What am I missing? You are right. I missed that actually it returns "". ~ Oleksii
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbol changes to be a hidden one.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -31,12 +31,7 @@
/* Linkage for PPC */
#ifdef __ASSEMBLY__
-#define ALIGN .p2align 2
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 4
#endif
#define XEN_VIRT_START _AT(UL, 0x400000)
--- a/xen/arch/ppc/ppc64/head.S
+++ b/xen/arch/ppc/ppc64/head.S
@@ -1,8 +1,10 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <xen/linkage.h>
+
.section .text.header, "ax", %progbits
-ENTRY(start)
+FUNC(start)
/*
* Depending on how we were booted, the CPU could be running in either
* Little Endian or Big Endian mode. The following trampoline from Linux
@@ -25,6 +27,4 @@ ENTRY(start)
/* Now that the endianness is confirmed, continue */
1: b 1b
-
- .size start, . - start
- .type start, %function
+END(start)
Use DATA() / END() and drop the now redundant .global. No change in
generated data; of course the two symbols now properly gain "hidden"
binding.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
--- a/xen/tools/binfile
+++ b/xen/tools/binfile
@@ -29,15 +29,10 @@ cat <<EOF >$target
.section $section.rodata, "a", %progbits
- .p2align $align
- .global $varname
-$varname:
+DATA($varname, 1 << $align)
.incbin "$binsource"
.Lend:
+END($varname)
- .type $varname, %object
- .size $varname, .Lend - $varname
-
- .global ${varname}_size
ASM_INT(${varname}_size, .Lend - $varname)
EOF
Leverage the new infrastructure in xen/linkage.h to also switch to per-
function sections (when configured), deriving the specific name from the
"base" section in use at the time FUNC() is invoked.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Since we use .subsection in UNLIKELY_START(), a perhaps not really
wanted side effect of this change is that respective out-of-line
code now moves much closer to its original (invoking) code.
Note that we'd need to split DATA() in order to separate r/w and r/o
contributions. Further splitting might be needed to also support more
advanced attributes (e.g. merge), hence why this isn't done right here.
Sadly while a new section's name can be derived from the presently in
use, its attributes cannot be. Hence perhaps the only thing we can do is
give DATA() a 2nd mandatory parameter. Then again I guess most data
definitions could be moved to C anyway.
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -399,6 +399,9 @@ AFLAGS += -D__ASSEMBLY__
$(call cc-option-add,AFLAGS,CC,-Wa$(comma)--noexecstack)
+# Check to see whether the assmbler supports the --sectname-subst option.
+$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST)
+
LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments
CFLAGS += $(CFLAGS-y)
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -154,6 +154,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
*(.altinstr_replacement)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -106,6 +106,9 @@ SECTIONS
DECL_SECTION(.init.text) {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -92,6 +92,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -86,6 +86,9 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
_stextentry = .;
*(.text.entry)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.text.entry.*)
+#endif
. = ALIGN(PAGE_SIZE);
_etextentry = .;
@@ -214,6 +217,9 @@ SECTIONS
#endif
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
*(.text.startup)
_einittext = .;
/*
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -21,6 +21,14 @@
#define SYM_ALIGN(algn...) .balign algn
+#if defined(HAVE_AS_SECTNAME_SUBST) && defined(CONFIG_CC_SPLIT_SECTIONS)
+# define SYM_PUSH_SECTION(name, attr) \
+ .pushsection %S.name, attr, %progbits; \
+ .equ .Lsplit_section, 1
+#else
+# define SYM_PUSH_SECTION(name, attr)
+#endif
+
#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
@@ -35,7 +43,14 @@
SYM_ALIGN(algn); \
name:
-#define END(name) .size name, . - name
+#define END(name) \
+ .size name, . - name; \
+ .ifdef .Lsplit_section; \
+ .if .Lsplit_section; \
+ .popsection; \
+ .equ .Lsplit_section, 0; \
+ .endif; \
+ .endif
/*
* CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
@@ -49,6 +64,7 @@
#endif
#define FUNC(name, algn...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn))
#define LABEL(name, algn...) \
SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn))
@@ -56,6 +72,7 @@
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
#define FUNC_LOCAL(name, algn...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn))
#define LABEL_LOCAL(name, algn...) \
SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn))
On 17.07.2023 16:18, Jan Beulich wrote: > Leverage the new infrastructure in xen/linkage.h to also switch to per- > function sections (when configured), deriving the specific name from the > "base" section in use at the time FUNC() is invoked. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> This additional change --- a/Config.mk +++ b/Config.mk @@ -115,7 +115,7 @@ cc-option = $(shell if test -z "`echo 'v # Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6) cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3))) define cc-option-add-closure - ifneq ($$(call cc-option,$$($(2)),$(3),n),n) + ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n) $(1) += $(3) endif endef is needed for ... > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -399,6 +399,9 @@ AFLAGS += -D__ASSEMBLY__ > > $(call cc-option-add,AFLAGS,CC,-Wa$(comma)--noexecstack) > > +# Check to see whether the assmbler supports the --sectname-subst option. > +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST) ... the pair of options passed in one go here to work (when old gas is in use). Of course something with the same overall effect, but less impactful might do. E.g. $(filter-out -D%,$(3)) instead of $(firstword (3)). Thoughts anyone? Jan
The model introduced in patch 1 is now arch-agnostic, and all arch-es are being switched at least partly (to at least give examples of how things will look like); PPC and RISC-V are still small enough to switch completely in one go. 1: common: assembly entry point type/size annotations 2: x86: annotate entry points with type and size 3: x86: also mark assembler globals hidden 4: Arm: annotate entry points with type and size 5: RISC-V: annotate entry points with type and size 6: PPC: switch entry point annotations to common model 7: tools/binfile: switch to common annotations model 8: common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions Jan
On 04.08.2023 08:24, Jan Beulich wrote: > The model introduced in patch 1 is now arch-agnostic, and all arch-es > are being switched at least partly (to at least give examples of how > things will look like); PPC and RISC-V are still small enough to switch > completely in one go. > > 1: common: assembly entry point type/size annotations > 2: x86: annotate entry points with type and size > 3: x86: also mark assembler globals hidden > 4: Arm: annotate entry points with type and size > 5: RISC-V: annotate entry points with type and size > 6: PPC: switch entry point annotations to common model > 7: tools/binfile: switch to common annotations model I'm sorry, I notice I screwed up with the numbering of patches 6 and 7. I hope things are still clear enough. Jan
Recent gas versions generate minimalistic Dwarf debug info for items
annotated as functions and having their sizes specified [1]. Furthermore
generating live patches wants items properly annotated. "Borrow" Arm's
END() and (remotely) derive other annotation infrastructure from
Linux'es, for all architectures to use.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28
---
v3: New, generalized from earlier x86-only version. LAST() (now
LASTARG()) moved to macros.h.
---
TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es
to define that in all cases?
TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be
specified (in case this has some special meaning on an arch;
conceivably it could mean to use some kind of arch default). We may
not strictly need that, and hence we could also make these power-of
-2 values (using .p2align).
Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we
still have ALIGN.
Note further that FUNC()'s etc "algn" parameter is intended to allow for
only no or a single argument. If we wanted to also make the fill value
customizable per call site, the constructs would need re-doing to some
degree.
--- /dev/null
+++ b/xen/include/xen/linkage.h
@@ -0,0 +1,56 @@
+#ifndef __LINKAGE_H__
+#define __LINKAGE_H__
+
+#ifdef __ASSEMBLY__
+
+#include <xen/macros.h>
+
+#ifndef CODE_ALIGN
+# define CODE_ALIGN ??
+#endif
+#ifndef CODE_FILL
+# define CODE_FILL ~0
+#endif
+
+#ifndef DATA_ALIGN
+# define DATA_ALIGN 0
+#endif
+#ifndef DATA_FILL
+# define DATA_FILL ~0
+#endif
+
+#define SYM_ALIGN(algn...) .balign algn
+
+#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_WEAK(name) .weak name
+#define SYM_L_LOCAL(name) /* nothing */
+
+#define SYM_T_FUNC STT_FUNC
+#define SYM_T_DATA STT_OBJECT
+#define SYM_T_NONE STT_NOTYPE
+
+#define SYM(name, typ, linkage, algn...) \
+ .type name, SYM_T_ ## typ; \
+ SYM_L_ ## linkage(name); \
+ SYM_ALIGN(algn); \
+ name:
+
+#define END(name) .size name, . - name
+
+#define FUNC(name, algn...) \
+ SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define LABEL(name, algn...) \
+ SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define DATA(name, algn...) \
+ SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
+
+#define FUNC_LOCAL(name, algn...) \
+ SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define LABEL_LOCAL(name, algn...) \
+ SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+#define DATA_LOCAL(name, algn...) \
+ SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __LINKAGE_H__ */
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -15,6 +15,15 @@
#define count_args(args...) \
count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+#define ARG1_(x, y...) (x)
+#define ARG2_(x, y...) ARG1_(y)
+#define ARG3_(x, y...) ARG2_(y)
+#define ARG4_(x, y...) ARG3_(y)
+
+#define ARG__(nr) ARG ## nr ## _
+#define ARG_(nr) ARG__(nr)
+#define LASTARG(x, y...) ARG_(count_args(x, ## y))(x, ## y)
+
/* Indirect macros required for expanded argument pasting. */
#define PASTE_(a, b) a ## b
#define PASTE(a, b) PASTE_(a, b)
Hi Jan, On 04/08/2023 07:26, Jan Beulich wrote: > Recent gas versions generate minimalistic Dwarf debug info for items > annotated as functions and having their sizes specified [1]. Furthermore > generating live patches wants items properly annotated. "Borrow" Arm's > END() and (remotely) derive other annotation infrastructure from > Linux'es, for all architectures to use. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > [1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28 > --- > v3: New, generalized from earlier x86-only version. LAST() (now > LASTARG()) moved to macros.h. > --- > TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es > to define that in all cases? The code alignment is very specific to an architecture. So I think it would be better if there are no default. Otherwise, it will be more difficult for a developper to figure out that CODE_ALIGN may need an update. > > TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be > specified (in case this has some special meaning on an arch; > conceivably it could mean to use some kind of arch default). We may > not strictly need that, and hence we could also make these power-of > -2 values (using .p2align). I don't have a strong opinion on this one. > > Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > still have ALIGN. > > Note further that FUNC()'s etc "algn" parameter is intended to allow for > only no or a single argument. If we wanted to also make the fill value > customizable per call site, the constructs would need re-doing to some > degree. > > --- /dev/null > +++ b/xen/include/xen/linkage.h > @@ -0,0 +1,56 @@ > +#ifndef __LINKAGE_H__ > +#define __LINKAGE_H__ > + > +#ifdef __ASSEMBLY__ > + > +#include <xen/macros.h> > + > +#ifndef CODE_ALIGN > +# define CODE_ALIGN ?? > +#endif > +#ifndef CODE_FILL > +# define CODE_FILL ~0 > +#endif What's the value to allow the architecture to override CODE_FILL and ... > + > +#ifndef DATA_ALIGN > +# define DATA_ALIGN 0 > +#endif > +#ifndef DATA_FILL > +# define DATA_FILL ~0 > +#endif ... DATA_FILL? > + > +#define SYM_ALIGN(algn...) .balign algn I find the name 'algn' confusing (not even referring to the missing 'i'). Why not naming it 'args'? > + > +#define SYM_L_GLOBAL(name) .globl name > +#define SYM_L_WEAK(name) .weak name > +#define SYM_L_LOCAL(name) /* nothing */ > + > +#define SYM_T_FUNC STT_FUNC > +#define SYM_T_DATA STT_OBJECT > +#define SYM_T_NONE STT_NOTYPE SYM_* will be used only in SYM() below. So why not using STT_* directly? > + > +#define SYM(name, typ, linkage, algn...) \ > + .type name, SYM_T_ ## typ; \ > + SYM_L_ ## linkage(name); \ > + SYM_ALIGN(algn); \ > + name: > + > +#define END(name) .size name, . - name > + > +#define FUNC(name, algn...) \ > + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > +#define LABEL(name, algn...) \ > + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > +#define DATA(name, algn...) \ > + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) I think the alignment should be explicit for DATA. Otherwise, at least on Arm, we would default to 0 which could lead to unaligned access if not careful. > + > +#define FUNC_LOCAL(name, algn...) \ > + SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > +#define LABEL_LOCAL(name, algn...) \ > + SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) > +#define DATA_LOCAL(name, algn...) \ > + SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) Same here. > + > +#endif /* __ASSEMBLY__ */ > + > +#endif /* __LINKAGE_H__ */ > --- a/xen/include/xen/macros.h > +++ b/xen/include/xen/macros.h > @@ -15,6 +15,15 @@ > #define count_args(args...) \ > count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0) > > +#define ARG1_(x, y...) (x) > +#define ARG2_(x, y...) ARG1_(y) > +#define ARG3_(x, y...) ARG2_(y) > +#define ARG4_(x, y...) ARG3_(y) > + > +#define ARG__(nr) ARG ## nr ## _ > +#define ARG_(nr) ARG__(nr) > +#define LASTARG(x, y...) ARG_(count_args(x, ## y))(x, ## y) > + > /* Indirect macros required for expanded argument pasting. */ > #define PASTE_(a, b) a ## b > #define PASTE(a, b) PASTE_(a, b) > Cheers, -- Julien Grall
On 14.09.2023 23:06, Julien Grall wrote: > On 04/08/2023 07:26, Jan Beulich wrote: >> TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es >> to define that in all cases? > > The code alignment is very specific to an architecture. So I think it > would be better if there are no default. > > Otherwise, it will be more difficult for a developper to figure out that > CODE_ALIGN may need an update. Okay, I've dropped the fallback then. >> --- /dev/null >> +++ b/xen/include/xen/linkage.h >> @@ -0,0 +1,56 @@ >> +#ifndef __LINKAGE_H__ >> +#define __LINKAGE_H__ >> + >> +#ifdef __ASSEMBLY__ >> + >> +#include <xen/macros.h> >> + >> +#ifndef CODE_ALIGN >> +# define CODE_ALIGN ?? >> +#endif >> +#ifndef CODE_FILL >> +# define CODE_FILL ~0 >> +#endif > > What's the value to allow the architecture to override CODE_FILL and ... What is put between functions may be relevant to control. Without fall- through to a subsequent label, I think the intention is to use "int3" (0xcc) filler bytes, for example. (With fall-through to the subsequent label, NOPs would need using in any event.) >> + >> +#ifndef DATA_ALIGN >> +# define DATA_ALIGN 0 >> +#endif >> +#ifndef DATA_FILL >> +# define DATA_FILL ~0 >> +#endif > > ... DATA_FILL? For data the need is probably less strict; still I could see one arch to prefer zero filling while another might better like all-ones-filling. >> + >> +#define SYM_ALIGN(algn...) .balign algn > > I find the name 'algn' confusing (not even referring to the missing > 'i'). Why not naming it 'args'? I can name it "args", sure. It's just that "algn" is in line with the naming further down (where "args" isn't reasonable to use as substitution). >> +#define SYM_L_GLOBAL(name) .globl name >> +#define SYM_L_WEAK(name) .weak name >> +#define SYM_L_LOCAL(name) /* nothing */ >> + >> +#define SYM_T_FUNC STT_FUNC >> +#define SYM_T_DATA STT_OBJECT >> +#define SYM_T_NONE STT_NOTYPE > > SYM_* will be used only in SYM() below. So why not using STT_* directly? For one this is how the Linux original has it. And then to me DATA and NONE are neater to have at the use sites than the ELF-specific terms OBJECT and NOTYPE. But I'm willing to reconsider provided arguments towards the two given reasons not being overly relevant for us. >> + >> +#define SYM(name, typ, linkage, algn...) \ >> + .type name, SYM_T_ ## typ; \ >> + SYM_L_ ## linkage(name); \ >> + SYM_ALIGN(algn); \ >> + name: >> + >> +#define END(name) .size name, . - name >> + >> +#define FUNC(name, algn...) \ >> + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >> +#define LABEL(name, algn...) \ >> + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >> +#define DATA(name, algn...) \ >> + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) > > I think the alignment should be explicit for DATA. Otherwise, at least > on Arm, we would default to 0 which could lead to unaligned access if > not careful. I disagree. Even for byte-granular data (like strings) it may be desirable to have some default alignment, without every use site needing to repeat that specific value. Jan
Hi, On 18/09/2023 11:24, Jan Beulich wrote: > On 14.09.2023 23:06, Julien Grall wrote: >> On 04/08/2023 07:26, Jan Beulich wrote: >>> TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es >>> to define that in all cases? >> >> The code alignment is very specific to an architecture. So I think it >> would be better if there are no default. >> >> Otherwise, it will be more difficult for a developper to figure out that >> CODE_ALIGN may need an update. > > Okay, I've dropped the fallback then. > >>> --- /dev/null >>> +++ b/xen/include/xen/linkage.h >>> @@ -0,0 +1,56 @@ >>> +#ifndef __LINKAGE_H__ >>> +#define __LINKAGE_H__ >>> + >>> +#ifdef __ASSEMBLY__ >>> + >>> +#include <xen/macros.h> >>> + >>> +#ifndef CODE_ALIGN >>> +# define CODE_ALIGN ?? >>> +#endif >>> +#ifndef CODE_FILL >>> +# define CODE_FILL ~0 >>> +#endif >> >> What's the value to allow the architecture to override CODE_FILL and ... > > What is put between functions may be relevant to control. Without fall- > through to a subsequent label, I think the intention is to use "int3" (0xcc) > filler bytes, for example. (With fall-through to the subsequent label, NOPs > would need using in any event.) I guess for x86 it makes sense. For Arm, the filler is unlikely to be used as the instruction size is always fixed. > >>> + >>> +#ifndef DATA_ALIGN >>> +# define DATA_ALIGN 0 >>> +#endif >>> +#ifndef DATA_FILL >>> +# define DATA_FILL ~0 >>> +#endif >> >> ... DATA_FILL? > > For data the need is probably less strict; still I could see one arch to > prefer zero filling while another might better like all-ones-filling. It is unclear to me why an architecture would prefer one over the other. Can you provide a bit more details? > >>> + >>> +#define SYM_ALIGN(algn...) .balign algn >> >> I find the name 'algn' confusing (not even referring to the missing >> 'i'). Why not naming it 'args'? > > I can name it "args", sure. It's just that "algn" is in line with the > naming further down (where "args" isn't reasonable to use as substitution). If you want to be consistent then, I think it would be best to use 'align'. I think it should be fine as we don't seem to use '.align'. > >>> +#define SYM_L_GLOBAL(name) .globl name >>> +#define SYM_L_WEAK(name) .weak name >>> +#define SYM_L_LOCAL(name) /* nothing */ >>> + >>> +#define SYM_T_FUNC STT_FUNC >>> +#define SYM_T_DATA STT_OBJECT >>> +#define SYM_T_NONE STT_NOTYPE >> >> SYM_* will be used only in SYM() below. So why not using STT_* directly? > > For one this is how the Linux original has it. And then to me DATA and > NONE are neater to have at the use sites than the ELF-specific terms > OBJECT and NOTYPE. But I'm willing to reconsider provided arguments > towards the two given reasons not being overly relevant for us. > >>> + >>> +#define SYM(name, typ, linkage, algn...) \ >>> + .type name, SYM_T_ ## typ; \ >>> + SYM_L_ ## linkage(name); \ >>> + SYM_ALIGN(algn); \ >>> + name: >>> + >>> +#define END(name) .size name, . - name >>> + >>> +#define FUNC(name, algn...) \ >>> + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >>> +#define LABEL(name, algn...) \ >>> + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >>> +#define DATA(name, algn...) \ >>> + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) >> >> I think the alignment should be explicit for DATA. Otherwise, at least >> on Arm, we would default to 0 which could lead to unaligned access if >> not careful. > > I disagree. Even for byte-granular data (like strings) it may be desirable > to have some default alignment, without every use site needing to repeat > that specific value. I understand that some cases may want to use a default alignment. But my concern is the developer may not realize that alignment is necessary. So by making it mandatory, it would at least prompt the developper to think whether this is needed. For the string case, we could introduce a different macro. Cheers, -- Julien Grall
On 18.09.2023 12:34, Julien Grall wrote: > Hi, > > On 18/09/2023 11:24, Jan Beulich wrote: >> On 14.09.2023 23:06, Julien Grall wrote: >>> On 04/08/2023 07:26, Jan Beulich wrote: >>>> TBD: What to set CODE_ALIGN to by default? Or should we requires arch-es >>>> to define that in all cases? >>> >>> The code alignment is very specific to an architecture. So I think it >>> would be better if there are no default. >>> >>> Otherwise, it will be more difficult for a developper to figure out that >>> CODE_ALIGN may need an update. >> >> Okay, I've dropped the fallback then. >> >>>> --- /dev/null >>>> +++ b/xen/include/xen/linkage.h >>>> @@ -0,0 +1,56 @@ >>>> +#ifndef __LINKAGE_H__ >>>> +#define __LINKAGE_H__ >>>> + >>>> +#ifdef __ASSEMBLY__ >>>> + >>>> +#include <xen/macros.h> >>>> + >>>> +#ifndef CODE_ALIGN >>>> +# define CODE_ALIGN ?? >>>> +#endif >>>> +#ifndef CODE_FILL >>>> +# define CODE_FILL ~0 >>>> +#endif >>> >>> What's the value to allow the architecture to override CODE_FILL and ... >> >> What is put between functions may be relevant to control. Without fall- >> through to a subsequent label, I think the intention is to use "int3" (0xcc) >> filler bytes, for example. (With fall-through to the subsequent label, NOPs >> would need using in any event.) > > I guess for x86 it makes sense. For Arm, the filler is unlikely to be > used as the instruction size is always fixed. > >> >>>> + >>>> +#ifndef DATA_ALIGN >>>> +# define DATA_ALIGN 0 >>>> +#endif >>>> +#ifndef DATA_FILL >>>> +# define DATA_FILL ~0 >>>> +#endif >>> >>> ... DATA_FILL? >> >> For data the need is probably less strict; still I could see one arch to >> prefer zero filling while another might better like all-ones-filling. > > It is unclear to me why an architecture would prefer one over the other. > Can you provide a bit more details? > >> >>>> + >>>> +#define SYM_ALIGN(algn...) .balign algn >>> >>> I find the name 'algn' confusing (not even referring to the missing >>> 'i'). Why not naming it 'args'? >> >> I can name it "args", sure. It's just that "algn" is in line with the >> naming further down (where "args" isn't reasonable to use as substitution). > > If you want to be consistent then, I think it would be best to use > 'align'. I think it should be fine as we don't seem to use '.align'. I think I had a conflict from this somewhere, but that may have been very early when I hadn't switched to .balign yet. I'll see if renaming works out. >>>> +#define SYM(name, typ, linkage, algn...) \ >>>> + .type name, SYM_T_ ## typ; \ >>>> + SYM_L_ ## linkage(name); \ >>>> + SYM_ALIGN(algn); \ >>>> + name: >>>> + >>>> +#define END(name) .size name, . - name >>>> + >>>> +#define FUNC(name, algn...) \ >>>> + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >>>> +#define LABEL(name, algn...) \ >>>> + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL) >>>> +#define DATA(name, algn...) \ >>>> + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL) >>> >>> I think the alignment should be explicit for DATA. Otherwise, at least >>> on Arm, we would default to 0 which could lead to unaligned access if >>> not careful. >> >> I disagree. Even for byte-granular data (like strings) it may be desirable >> to have some default alignment, without every use site needing to repeat >> that specific value. > > I understand that some cases may want to use a default alignment. But my > concern is the developer may not realize that alignment is necessary. So > by making it mandatory, it would at least prompt the developper to think > whether this is needed. Forcing people to use a specific value every time, even when none would be needed. Anyway, if others think your way, then I can certainly change. But then I need to know whether others perhaps think alignment on functions (and maybe even labels) should also be explicit in all cases. > For the string case, we could introduce a different macro. Hmm, yet one more special thing then (for people to remember to use under certain circumstances). Jan
Use the generic framework in xen/linkage.h.
For switch_to_kernel() and restore_all_guest() so far implicit alignment
(from being first in their respective sections) is being made explicit
(as in: using FUNC() without 2nd argument). Whereas for
{,compat}create_bounce_frame() and autogen_entrypoints[] alignment is
newly arranged for.
Except for the added/adjusted alignment padding (including their
knock-on effects) no change in generated code/data. Note that the basis
for support of weak definitions is added despite them not having any use
right now.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Correct simd_coprocessor_error's (now entry_XM) END(). Re-base.
v3: !PV variant of ret_from_intr is local. Introduction of macros split
off to separate patch. Also adjust ASM_INT(). Re-base.
v2: Full rework.
---
Only two of the assembly files are being converted for now. More could
be done right here or as follow-on in separate patches.
Note that the FB-label in autogen_stubs() cannot be converted just yet:
Such labels cannot be used with .type. We could further diverge from
Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type
labels get by default anyway).
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
I didn't think that I should make CODE_FILL evaluate to 0xCC right here;
IMO that wants to be a separate patch.
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -13,6 +13,7 @@
#include <asm/alternative.h>
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#include <asm/asm-defns.h>
#ifndef CONFIG_INDIRECT_THUNK
.equ CONFIG_INDIRECT_THUNK, 0
@@ -343,10 +344,7 @@ static always_inline void stac(void)
.popsection
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, @object
+ DATA(label, 4) .long (val); END(label)
#define ASM_CONSTANT(name, value) \
asm ( ".equ " #name ", %P0; .global " #name \
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -43,7 +43,9 @@
/* Linkage for x86 */
#ifdef __ASSEMBLY__
-#define ALIGN .align 16,0x90
+#define CODE_ALIGN 16
+#define CODE_FILL 0x90
+#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
.globl name; \
ALIGN; \
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -11,7 +11,7 @@
#include <public/xen.h>
#include <irq_vectors.h>
-ENTRY(entry_int82)
+FUNC(entry_int82)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -27,9 +27,10 @@ ENTRY(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+END(entry_int82)
/* %rbx: struct vcpu */
-ENTRY(compat_test_all_events)
+FUNC(compat_test_all_events)
ASSERT_NOT_IN_ATOMIC
cli # tests must not race interrupts
/*compat_test_softirqs:*/
@@ -66,24 +67,21 @@ compat_test_guest_events:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_softirqs:
+LABEL_LOCAL(compat_process_softirqs)
sti
call do_softirq
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx: struct trap_bounce */
-.Lcompat_process_trapbounce:
+LABEL_LOCAL(.Lcompat_process_trapbounce)
sti
.Lcompat_bounce_exception:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_mce:
+LABEL_LOCAL(compat_process_mce)
testb $1 << VCPU_TRAP_MCE,VCPU_async_exception_mask(%rbx)
jnz .Lcompat_test_guest_nmi
sti
@@ -97,9 +95,8 @@ compat_process_mce:
movb %dl,VCPU_async_exception_mask(%rbx)
jmp compat_process_trap
- ALIGN
/* %rbx: struct vcpu */
-compat_process_nmi:
+LABEL_LOCAL(compat_process_nmi)
testb $1 << VCPU_TRAP_NMI,VCPU_async_exception_mask(%rbx)
jnz compat_test_guest_events
sti
@@ -116,9 +113,10 @@ compat_process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_test_all_events)
/* %rbx: struct vcpu, interrupts disabled */
-ENTRY(compat_restore_all_guest)
+FUNC(compat_restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
mov $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11d
and UREGS_eflags(%rsp),%r11d
@@ -161,9 +159,10 @@ ENTRY(compat_restore_all_guest)
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(compat_restore_all_guest)
/* This mustn't modify registers other than %rax. */
-ENTRY(cr4_pv32_restore)
+FUNC(cr4_pv32_restore)
push %rdx
GET_CPUINFO_FIELD(cr4, dx)
mov (%rdx), %rax
@@ -193,8 +192,9 @@ ENTRY(cr4_pv32_restore)
pop %rdx
xor %eax, %eax
ret
+END(cr4_pv32_restore)
-ENTRY(compat_syscall)
+FUNC(compat_syscall)
/* Fix up reported %cs/%ss for compat domains. */
movl $FLAT_COMPAT_USER_SS, UREGS_ss(%rsp)
movl $FLAT_COMPAT_USER_CS, UREGS_cs(%rsp)
@@ -222,8 +222,9 @@ UNLIKELY_END(compat_syscall_gpf)
movw %si,TRAPBOUNCE_cs(%rdx)
movb %cl,TRAPBOUNCE_flags(%rdx)
jmp .Lcompat_bounce_exception
+END(compat_syscall)
-ENTRY(compat_sysenter)
+FUNC(compat_sysenter)
CR4_PV32_RESTORE
movq VCPU_trap_ctxt(%rbx),%rcx
cmpb $X86_EXC_GP, UREGS_entry_vector(%rsp)
@@ -236,17 +237,19 @@ ENTRY(compat_sysenter)
movw %ax,TRAPBOUNCE_cs(%rdx)
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_sysenter)
-ENTRY(compat_int80_direct_trap)
+FUNC(compat_int80_direct_trap)
CR4_PV32_RESTORE
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_int80_direct_trap)
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK: */
/* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-compat_create_bounce_frame:
+FUNC_LOCAL(compat_create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
mov %fs,%edi
ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
@@ -352,3 +355,4 @@ compat_crash_page_fault:
jmp .Lft14
.previous
_ASM_EXTABLE(.Lft14, .Lfx14)
+END(compat_create_bounce_frame)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -24,7 +24,7 @@
#ifdef CONFIG_PV
/* %rbx: struct vcpu */
-switch_to_kernel:
+FUNC_LOCAL(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
/* TB_eip = 32-bit syscall ? syscall32_addr : syscall_addr */
@@ -89,24 +89,21 @@ test_guest_events:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_softirqs:
+LABEL_LOCAL(process_softirqs)
sti
call do_softirq
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx struct trap_bounce */
-.Lprocess_trapbounce:
+LABEL_LOCAL(.Lprocess_trapbounce)
sti
.Lbounce_exception:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_mce:
+LABEL_LOCAL(process_mce)
testb $1 << VCPU_TRAP_MCE, VCPU_async_exception_mask(%rbx)
jnz .Ltest_guest_nmi
sti
@@ -120,9 +117,8 @@ process_mce:
movb %dl, VCPU_async_exception_mask(%rbx)
jmp process_trap
- ALIGN
/* %rbx: struct vcpu */
-process_nmi:
+LABEL_LOCAL(process_nmi)
testb $1 << VCPU_TRAP_NMI, VCPU_async_exception_mask(%rbx)
jnz test_guest_events
sti
@@ -139,11 +135,12 @@ process_trap:
leaq VCPU_trap_bounce(%rbx), %rdx
call create_bounce_frame
jmp test_all_events
+END(switch_to_kernel)
.section .text.entry, "ax", @progbits
/* %rbx: struct vcpu, interrupts disabled */
-restore_all_guest:
+FUNC_LOCAL(restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
/* Stash guest SPEC_CTRL value while we can read struct vcpu. */
@@ -220,8 +217,7 @@ restore_all_guest:
sysretq
1: sysretl
- ALIGN
-.Lrestore_rcx_iret_exit_to_guest:
+LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_guest)
movq 8(%rsp), %rcx # RIP
/* No special register assumptions. */
iret_exit_to_guest:
@@ -230,6 +226,7 @@ iret_exit_to_guest:
addq $8,%rsp
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(restore_all_guest)
/*
* When entering SYSCALL from kernel mode:
@@ -246,7 +243,7 @@ iret_exit_to_guest:
* - Guest %rsp stored in %rax
* - Xen stack loaded, pointing at the %ss slot
*/
-ENTRY(lstar_enter)
+FUNC(lstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -281,9 +278,10 @@ ENTRY(lstar_enter)
mov %rsp, %rdi
call pv_hypercall
jmp test_all_events
+END(lstar_enter)
/* See lstar_enter for entry register state. */
-ENTRY(cstar_enter)
+FUNC(cstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -321,8 +319,9 @@ ENTRY(cstar_enter)
jne compat_syscall
#endif
jmp switch_to_kernel
+END(cstar_enter)
-ENTRY(sysenter_entry)
+FUNC(sysenter_entry)
ENDBR64
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
@@ -331,7 +330,7 @@ ENTRY(sysenter_entry)
pushq $FLAT_USER_SS
pushq $0
pushfq
-GLOBAL(sysenter_eflags_saved)
+LABEL(sysenter_eflags_saved, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $3 /* ring 3 null cs */
pushq $0 /* null rip */
@@ -385,8 +384,9 @@ UNLIKELY_END(sysenter_gpf)
jne compat_sysenter
#endif
jmp .Lbounce_exception
+END(sysenter_entry)
-ENTRY(int80_direct_trap)
+FUNC(int80_direct_trap)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -474,6 +474,7 @@ int80_slow_path:
*/
GET_STACK_END(14)
jmp handle_exception_saved
+END(int80_direct_trap)
/* create_bounce_frame & helpers don't need to be in .text.entry */
.text
@@ -482,7 +483,7 @@ int80_slow_path:
/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-create_bounce_frame:
+FUNC_LOCAL(create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jnz 1f
@@ -618,6 +619,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+END(create_bounce_frame)
#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -626,7 +628,7 @@ ENTRY(dom_crash_sync_extable)
/* No special register assumptions. */
#ifdef CONFIG_PV
-ENTRY(continue_pv_domain)
+FUNC(continue_pv_domain)
ENDBR64
call check_wakeup_from_wait
ret_from_intr:
@@ -641,26 +643,28 @@ ret_from_intr:
#else
jmp test_all_events
#endif
+END(continue_pv_domain)
#else
-ret_from_intr:
+FUNC_LOCAL(ret_from_intr, 0)
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
+END(ret_from_intr)
#endif
.section .init.text, "ax", @progbits
-ENTRY(early_page_fault)
+FUNC(early_page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
SAVE_ALL
movq %rsp, %rdi
call do_early_page_fault
jmp restore_all_xen
+END(early_page_fault)
.section .text.entry, "ax", @progbits
- ALIGN
/* No special register assumptions. */
-restore_all_xen:
+FUNC_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).
@@ -677,8 +681,9 @@ UNLIKELY_END(exit_cr3)
RESTORE_ALL adj=8
iretq
+END(restore_all_xen)
-ENTRY(common_interrupt)
+FUNC(common_interrupt)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -707,12 +712,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
+END(common_interrupt)
-ENTRY(entry_PF)
+FUNC(entry_PF)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
+END(entry_PF)
/* No special register assumptions. */
-GLOBAL(handle_exception)
+FUNC(handle_exception, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -880,92 +887,108 @@ FATAL_exception_with_ints_disabled:
xorl %esi,%esi
movq %rsp,%rdi
tailcall fatal_trap
+END(handle_exception)
-ENTRY(entry_DE)
+FUNC(entry_DE)
ENDBR64
pushq $0
movl $X86_EXC_DE, 4(%rsp)
jmp handle_exception
+END(entry_DE)
-ENTRY(entry_MF)
+FUNC(entry_MF)
ENDBR64
pushq $0
movl $X86_EXC_MF, 4(%rsp)
jmp handle_exception
+END(entry_MF)
-ENTRY(entry_XM)
+FUNC(entry_XM)
ENDBR64
pushq $0
movl $X86_EXC_XM, 4(%rsp)
jmp handle_exception
+END(entry_XM)
-ENTRY(entry_NM)
+FUNC(entry_NM)
ENDBR64
pushq $0
movl $X86_EXC_NM, 4(%rsp)
jmp handle_exception
+END(entry_NM)
-ENTRY(entry_DB)
+FUNC(entry_DB)
ENDBR64
pushq $0
movl $X86_EXC_DB, 4(%rsp)
jmp handle_ist_exception
+END(entry_DB)
-ENTRY(entry_BP)
+FUNC(entry_BP)
ENDBR64
pushq $0
movl $X86_EXC_BP, 4(%rsp)
jmp handle_exception
+END(entry_BP)
-ENTRY(entry_OF)
+FUNC(entry_OF)
ENDBR64
pushq $0
movl $X86_EXC_OF, 4(%rsp)
jmp handle_exception
+END(entry_OF)
-ENTRY(entry_BR)
+FUNC(entry_BR)
ENDBR64
pushq $0
movl $X86_EXC_BR, 4(%rsp)
jmp handle_exception
+END(entry_BR)
-ENTRY(entry_UD)
+FUNC(entry_UD)
ENDBR64
pushq $0
movl $X86_EXC_UD, 4(%rsp)
jmp handle_exception
+END(entry_UD)
-ENTRY(entry_TS)
+FUNC(entry_TS)
ENDBR64
movl $X86_EXC_TS, 4(%rsp)
jmp handle_exception
+END(entry_TS)
-ENTRY(entry_NP)
+FUNC(entry_NP)
ENDBR64
movl $X86_EXC_NP, 4(%rsp)
jmp handle_exception
+END(entry_NP)
-ENTRY(entry_SS)
+FUNC(entry_SS)
ENDBR64
movl $X86_EXC_SS, 4(%rsp)
jmp handle_exception
+END(entry_SS)
-ENTRY(entry_GP)
+FUNC(entry_GP)
ENDBR64
movl $X86_EXC_GP, 4(%rsp)
jmp handle_exception
+END(entry_GP)
-ENTRY(entry_AC)
+FUNC(entry_AC)
ENDBR64
movl $X86_EXC_AC, 4(%rsp)
jmp handle_exception
+END(entry_AC)
-ENTRY(entry_CP)
+FUNC(entry_CP)
ENDBR64
movl $X86_EXC_CP, 4(%rsp)
jmp handle_exception
+END(entry_CP)
-ENTRY(entry_DF)
+FUNC(entry_DF)
ENDBR64
movl $X86_EXC_DF, 4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
@@ -988,8 +1011,9 @@ ENTRY(entry_DF)
movq %rsp,%rdi
tailcall do_double_fault
+END(entry_DF)
-ENTRY(entry_NMI)
+FUNC(entry_NMI)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
@@ -1116,21 +1140,24 @@ handle_ist_exception:
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
#endif
+END(entry_NMI)
-ENTRY(entry_MC)
+FUNC(entry_MC)
ENDBR64
pushq $0
movl $X86_EXC_MC, 4(%rsp)
jmp handle_ist_exception
+END(entry_MC)
/* No op trap handler. Required for kexec crash path. */
-GLOBAL(trap_nop)
+FUNC(trap_nop, 0)
ENDBR64
iretq
+END(trap_nop)
/* Table of automatically generated entry points. One per vector. */
.pushsection .init.rodata, "a", @progbits
-GLOBAL(autogen_entrypoints)
+DATA(autogen_entrypoints, 8)
/* pop into the .init.rodata section and record an entry point. */
.macro entrypoint ent
.pushsection .init.rodata, "a", @progbits
@@ -1139,7 +1166,7 @@ GLOBAL(autogen_entrypoints)
.endm
.popsection
-autogen_stubs: /* Automatically generated stubs. */
+FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
vec = 0
.rept X86_NR_VECTORS
@@ -1183,6 +1210,7 @@ autogen_stubs: /* Automatically generate
vec = vec + 1
.endr
+END(autogen_stubs)
.section .init.rodata, "a", @progbits
- .size autogen_entrypoints, . - autogen_entrypoints
+END(autogen_entrypoints)
Let's have assembler symbols be consistent with C ones. In principle
there are (a few) cases where gas can produce smaller code this way,
just that for now there's a gas bug causing smaller code to be emitted
even when that shouldn't be the case.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
v3: Re-base over generalization of the annotations.
v2: New.
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -47,11 +47,11 @@
#define CODE_FILL 0x90
#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
- .globl name; \
ALIGN; \
- name:
+ GLOBAL(name)
#define GLOBAL(name) \
.globl name; \
+ .hidden name; \
name:
#endif
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -21,7 +21,7 @@
#define SYM_ALIGN(algn...) .balign algn
-#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
Use the generic framework in xen/linkage.h. No change in generated code
except for the changed padding value (noticable when config.gz isn't a
multiple of 4 in size). Plus of course the converted symbols change to
be hidden ones.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Only one each of the assembly files is being converted for now. More
could be done right here or as follow-on in separate patches.
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
---
v3: New.
--- a/xen/arch/arm/arm32/lib/div64.S
+++ b/xen/arch/arm/arm32/lib/div64.S
@@ -42,7 +42,7 @@
* Clobbered regs: xl, ip
*/
-ENTRY(__do_div64)
+FUNC(__do_div64)
UNWIND(.fnstart)
@ Test for easy paths first.
@@ -206,4 +206,4 @@ Ldiv0_64:
ldr pc, [sp], #8
UNWIND(.fnend)
-ENDPROC(__do_div64)
+END(__do_div64)
--- a/xen/arch/arm/arm64/lib/clear_page.S
+++ b/xen/arch/arm/arm64/lib/clear_page.S
@@ -22,7 +22,7 @@
* Parameters:
* x0 - dest
*/
-ENTRY(clear_page)
+FUNC(clear_page)
mrs x1, dczid_el0
and w1, w1, #0xf
mov x2, #4
@@ -33,4 +33,4 @@ ENTRY(clear_page)
tst x0, #(PAGE_SIZE - 1)
b.ne 1b
ret
-ENDPROC(clear_page)
+END(clear_page)
--- a/xen/arch/arm/include/asm/asm_defns.h
+++ b/xen/arch/arm/include/asm/asm_defns.h
@@ -5,6 +5,7 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
+#include <xen/linkage.h>
#include <asm/processor.h>
/* Macros for generic assembly code */
@@ -28,10 +29,7 @@ label: .asciz msg;
.popsection
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, %object
+ DATA(label, 4) .long (val); END(label)
#endif /* __ARM_ASM_DEFNS_H__ */
/*
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -53,7 +53,8 @@
/* Linkage for ARM */
#ifdef __ASSEMBLY__
-#define ALIGN .align 2
+#define CODE_ALIGN 4
+#define ALIGN .balign CODE_ALIGN
#define ENTRY(name) \
.globl name; \
ALIGN; \
@@ -61,8 +62,6 @@
#define GLOBAL(name) \
.globl name; \
name:
-#define END(name) \
- .size name, .-name
#define ENDPROC(name) \
.type name, %function; \
END(name)
Hi, On 04/08/2023 07:28, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except for the changed padding value (noticable when config.gz isn't a > multiple of 4 in size). Plus of course the converted symbols change to > be hidden ones. > > Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only > use site wants the symbol global anyway. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> > --- > Only one each of the assembly files is being converted for now. More > could be done right here or as follow-on in separate patches. I don't have a strong preference. Are you planning to do follow-up? (I am ok if it is no). Cheers, -- Julien Grall
On 14.09.2023 23:25, Julien Grall wrote: > On 04/08/2023 07:28, Jan Beulich wrote: >> Use the generic framework in xen/linkage.h. No change in generated code >> except for the changed padding value (noticable when config.gz isn't a >> multiple of 4 in size). Plus of course the converted symbols change to >> be hidden ones. >> >> Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only >> use site wants the symbol global anyway. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thanks. >> --- >> Only one each of the assembly files is being converted for now. More >> could be done right here or as follow-on in separate patches. > > I don't have a strong preference. Are you planning to do follow-up? (I > am ok if it is no). Well, I certainly can, but I wasn't expecting this series to remain pending for this long, so time's running out for 4.18. Jan
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbols change to be hidden ones and gain
a valid size.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Probably count_args_exp() should move to macros.h, but I first wanted to
see whether anyone can suggest any better approach for checking whether
a defined macro expands to nothing.
---
v4: Also drop #undef-s from linker script.
v3: New.
--- a/xen/arch/riscv/entry.S
+++ b/xen/arch/riscv/entry.S
@@ -5,7 +5,7 @@
#include <asm/traps.h>
/* WIP: only works while interrupting Xen context */
-ENTRY(handle_trap)
+FUNC(handle_trap)
/* Exceptions from xen */
save_to_stack:
@@ -92,3 +92,4 @@ restore_registers:
REG_L sp, CPU_USER_REGS_SP(sp)
sret
+END(handle_trap)
--- a/xen/arch/riscv/include/asm/asm.h
+++ b/xen/arch/riscv/include/asm/asm.h
@@ -7,6 +7,7 @@
#define _ASM_RISCV_ASM_H
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#define __ASM_STR(x) x
#else
#define __ASM_STR(x) #x
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -69,12 +69,8 @@
/* Linkage for RISCV */
#ifdef __ASSEMBLY__
-#define ALIGN .align 4
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 16
+#define CODE_FILL /* empty */
#endif
#ifdef CONFIG_RISCV_64
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -8,7 +8,7 @@
* a0 -> hart_id ( bootcpu_id )
* a1 -> dtb_base
*/
-ENTRY(start)
+FUNC(start)
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -30,13 +30,14 @@ ENTRY(start)
jal reset_stack
tail start_xen
+END(start)
.section .text, "ax", %progbits
-ENTRY(reset_stack)
+FUNC(reset_stack)
la sp, cpu0_boot_stack
li t0, STACK_SIZE
add sp, sp, t0
ret
-
+END(reset_stack)
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -1,9 +1,6 @@
#include <xen/lib.h>
#include <xen/xen.lds.h>
-#undef ENTRY
-#undef ALIGN
-
OUTPUT_ARCH(riscv)
ENTRY(start)
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -37,17 +37,28 @@
#define END(name) .size name, . - name
+/*
+ * CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
+ * which case we also need to get rid of the comma in the .balign directive.
+ */
+#define count_args_exp(args...) count_args(args)
+#if count_args_exp(CODE_FILL)
+# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn), CODE_FILL
+#else
+# define DO_CODE_ALIGN(algn...) LASTARG(CODE_ALIGN, ## algn)
+#endif
+
#define FUNC(name, algn...) \
- SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn))
#define LABEL(name, algn...) \
- SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn))
#define DATA(name, algn...) \
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
#define FUNC_LOCAL(name, algn...) \
- SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn))
#define LABEL_LOCAL(name, algn...) \
- SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## algn), CODE_FILL)
+ SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn))
#define DATA_LOCAL(name, algn...) \
SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbol changes to be a hidden one.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v4: Also drop #undef-s from linker script. Re-base.
v3: New.
--- a/xen/arch/ppc/include/asm/asm-defns.h
+++ b/xen/arch/ppc/include/asm/asm-defns.h
@@ -2,6 +2,8 @@
#ifndef _ASM_PPC_ASM_DEFNS_H
#define _ASM_PPC_ASM_DEFNS_H
+#include <xen/linkage.h>
+
/*
* Load a 64-bit immediate value into the specified GPR.
*/
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -31,12 +31,7 @@
/* Linkage for PPC */
#ifdef __ASSEMBLY__
-#define ALIGN .p2align 2
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 4
#endif
#define XEN_VIRT_START _AT(UL, 0xc000000000000000)
--- a/xen/arch/ppc/ppc64/head.S
+++ b/xen/arch/ppc/ppc64/head.S
@@ -5,7 +5,7 @@
.section .text.header, "ax", %progbits
-ENTRY(start)
+FUNC(start)
/*
* NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
*/
@@ -36,6 +36,4 @@ ENTRY(start)
/* should never return */
trap
-
- .size start, . - start
- .type start, %function
+END(start)
--- a/xen/arch/ppc/ppc64/of-call.S
+++ b/xen/arch/ppc/ppc64/of-call.S
@@ -23,7 +23,7 @@
.section .init.text, "ax", @progbits
-ENTRY(enter_of)
+FUNC(enter_of)
mflr %r0
std %r0, 16(%r1)
stdu %r1, -STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack space */
@@ -78,6 +78,4 @@ ENTRY(enter_of)
ld %r0, 16(%r1)
mtlr %r0
blr
-
- .size enter_of, . - enter_of
- .type enter_of, %function
+END(enter_of)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -1,9 +1,6 @@
#include <xen/lib.h>
#include <xen/xen.lds.h>
-#undef ENTRY
-#undef ALIGN
-
OUTPUT_ARCH(powerpc:common64)
ENTRY(start)
Use DATA() / END() and drop the now redundant .global. No change in
generated data; of course the two symbols now properly gain "hidden"
binding.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
--- a/xen/tools/binfile
+++ b/xen/tools/binfile
@@ -29,15 +29,10 @@ cat <<EOF >$target
.section $section.rodata, "a", %progbits
- .p2align $align
- .global $varname
-$varname:
+DATA($varname, 1 << $align)
.incbin "$binsource"
.Lend:
+END($varname)
- .type $varname, %object
- .size $varname, .Lend - $varname
-
- .global ${varname}_size
ASM_INT(${varname}_size, .Lend - $varname)
EOF
Hi Jan, On 04/08/2023 07:30, Jan Beulich wrote: > Use DATA() / END() and drop the now redundant .global. No change in > generated data; of course the two symbols now properly gain "hidden" > binding. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers, -- Julien Grall
Leverage the new infrastructure in xen/linkage.h to also switch to per-
function sections (when configured), deriving the specific name from the
"base" section in use at the time FUNC() is invoked.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really
wanted side effect of this change is that respective out-of-line
code now moves much closer to its original (invoking) code.
TBD: Of course something with the same overall effect, but less
impactful might do in Config.mk. E.g. $(filter-out -D%,$(3))
instead of $(firstword (3)).
Note that we'd need to split DATA() in order to separate r/w and r/o
contributions. Further splitting might be needed to also support more
advanced attributes (e.g. merge), hence why this isn't done right here.
Sadly while a new section's name can be derived from the presently in
use, its attributes cannot be. Perhaps the only thing we can do is give
DATA() a 2nd mandatory parameter. Then again I guess most data
definitions could be moved to C anyway.
---
v4: Re-base.
v2: Make detection properly fail on old gas (by adjusting
cc-option-add-closure).
--- a/Config.mk
+++ b/Config.mk
@@ -102,7 +102,7 @@ cc-option = $(shell if test -z "`echo 'v
# Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6)
cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3)))
define cc-option-add-closure
- ifneq ($$(call cc-option,$$($(2)),$(3),n),n)
+ ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n)
$(1) += $(3)
endif
endef
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -407,6 +407,9 @@ AFLAGS += -D__ASSEMBLY__
$(call cc-option-add,AFLAGS,CC,-Wa$(comma)--noexecstack)
+# Check to see whether the assmbler supports the --sectname-subst option.
+$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST)
+
LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments
CFLAGS += $(CFLAGS-y)
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -154,6 +154,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
*(.altinstr_replacement)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -90,6 +90,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -89,6 +89,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -86,6 +86,9 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
_stextentry = .;
*(.text.entry)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.text.entry.*)
+#endif
. = ALIGN(PAGE_SIZE);
_etextentry = .;
@@ -214,6 +217,9 @@ SECTIONS
#endif
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
*(.text.startup)
_einittext = .;
/*
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -21,6 +21,14 @@
#define SYM_ALIGN(algn...) .balign algn
+#if defined(HAVE_AS_SECTNAME_SUBST) && defined(CONFIG_CC_SPLIT_SECTIONS)
+# define SYM_PUSH_SECTION(name, attr) \
+ .pushsection %S.name, attr, %progbits; \
+ .equ .Lsplit_section, 1
+#else
+# define SYM_PUSH_SECTION(name, attr)
+#endif
+
#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
@@ -35,7 +43,14 @@
SYM_ALIGN(algn); \
name:
-#define END(name) .size name, . - name
+#define END(name) \
+ .size name, . - name; \
+ .ifdef .Lsplit_section; \
+ .if .Lsplit_section; \
+ .popsection; \
+ .equ .Lsplit_section, 0; \
+ .endif; \
+ .endif
/*
* CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
@@ -49,6 +64,7 @@
#endif
#define FUNC(name, algn...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(algn))
#define LABEL(name, algn...) \
SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(algn))
@@ -56,6 +72,7 @@
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## algn), DATA_FILL)
#define FUNC_LOCAL(name, algn...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(algn))
#define LABEL_LOCAL(name, algn...) \
SYM(name, NONE, LOCAL, DO_CODE_ALIGN(algn))
The model introduced in patch 1 is now arch-agnostic, and all arch-es are being switched at least partly (to at least give examples of how things will look like); PPC and RISC-V are still small enough to switch completely in one go. 1: common: assembly entry point type/size annotations 2: x86: annotate entry points with type and size 3: x86: also mark assembler globals hidden 4: Arm: annotate entry points with type and size 5: RISC-V: annotate entry points with type and size 6: PPC: switch entry point annotations to common model 7: tools/binfile: switch to common annotations model 8: common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions Jan
Recent gas versions generate minimalistic Dwarf debug info for items
annotated as functions and having their sizes specified [1]. Furthermore
generating live patches wants items properly annotated. "Borrow" Arm's
END() and (remotely) derive other annotation infrastructure from
Linux'es, for all architectures to use.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28
---
v5: Drop CODE_ALIGN fallback. s/algn/align/g.
v3: New, generalized from earlier x86-only version. LAST() (now
LASTARG()) moved to macros.h.
---
TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be
specified (in case this has some special meaning on an arch;
conceivably it could mean to use some kind of arch default). We may
not strictly need that, and hence we could also make these power-
of-2 values (using .p2align).
Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we
still have ALIGN.
Note further that FUNC()'s etc "align" parameter is intended to allow
for only no or a single argument. If we wanted to also make the fill
value customizable per call site, the constructs would need re-doing to
some degree.
--- /dev/null
+++ b/xen/include/xen/linkage.h
@@ -0,0 +1,54 @@
+#ifndef __LINKAGE_H__
+#define __LINKAGE_H__
+
+#ifdef __ASSEMBLY__
+
+#include <xen/macros.h>
+
+/* CODE_ALIGN needs to be specified by every architecture. */
+#ifndef CODE_FILL
+# define CODE_FILL ~0
+#endif
+
+#ifndef DATA_ALIGN
+# define DATA_ALIGN 0
+#endif
+#ifndef DATA_FILL
+# define DATA_FILL ~0
+#endif
+
+#define SYM_ALIGN(align...) .balign align
+
+#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_WEAK(name) .weak name
+#define SYM_L_LOCAL(name) /* nothing */
+
+#define SYM_T_FUNC STT_FUNC
+#define SYM_T_DATA STT_OBJECT
+#define SYM_T_NONE STT_NOTYPE
+
+#define SYM(name, typ, linkage, align...) \
+ .type name, SYM_T_ ## typ; \
+ SYM_L_ ## linkage(name); \
+ SYM_ALIGN(align); \
+ name:
+
+#define END(name) .size name, . - name
+
+#define FUNC(name, align...) \
+ SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+#define LABEL(name, align...) \
+ SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+#define DATA(name, align...) \
+ SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
+
+#define FUNC_LOCAL(name, align...) \
+ SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+#define LABEL_LOCAL(name, align...) \
+ SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+#define DATA_LOCAL(name, align...) \
+ SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* __LINKAGE_H__ */
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -21,6 +21,15 @@
#define count_args(args...) \
count_args_(., ## args, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+#define ARG1_(x, y...) (x)
+#define ARG2_(x, y...) ARG1_(y)
+#define ARG3_(x, y...) ARG2_(y)
+#define ARG4_(x, y...) ARG3_(y)
+
+#define ARG__(nr) ARG ## nr ## _
+#define ARG_(nr) ARG__(nr)
+#define LASTARG(x, y...) ARG_(count_args(x, ## y))(x, ## y)
+
/* Indirect macros required for expanded argument pasting. */
#define PASTE_(a, b) a ## b
#define PASTE(a, b) PASTE_(a, b)
On Mon, Jan 15, 2024 at 03:34:05PM +0100, Jan Beulich wrote: > Recent gas versions generate minimalistic Dwarf debug info for items > annotated as functions and having their sizes specified [1]. Furthermore > generating live patches wants items properly annotated. "Borrow" Arm's > END() and (remotely) derive other annotation infrastructure from > Linux'es, for all architectures to use. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > [1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28 > --- > v5: Drop CODE_ALIGN fallback. s/algn/align/g. > v3: New, generalized from earlier x86-only version. LAST() (now > LASTARG()) moved to macros.h. > --- > TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be > specified (in case this has some special meaning on an arch; > conceivably it could mean to use some kind of arch default). We may > not strictly need that, and hence we could also make these power- > of-2 values (using .p2align). > > Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > still have ALIGN. > > Note further that FUNC()'s etc "align" parameter is intended to allow > for only no or a single argument. If we wanted to also make the fill > value customizable per call site, the constructs would need re-doing to > some degree. > > --- /dev/null > +++ b/xen/include/xen/linkage.h > @@ -0,0 +1,54 @@ > +#ifndef __LINKAGE_H__ > +#define __LINKAGE_H__ > + > +#ifdef __ASSEMBLY__ > + > +#include <xen/macros.h> > + > +/* CODE_ALIGN needs to be specified by every architecture. */ > +#ifndef CODE_FILL > +# define CODE_FILL ~0 > +#endif > + > +#ifndef DATA_ALIGN > +# define DATA_ALIGN 0 > +#endif > +#ifndef DATA_FILL > +# define DATA_FILL ~0 > +#endif > + > +#define SYM_ALIGN(align...) .balign align > + > +#define SYM_L_GLOBAL(name) .globl name > +#define SYM_L_WEAK(name) .weak name > +#define SYM_L_LOCAL(name) /* nothing */ > + > +#define SYM_T_FUNC STT_FUNC > +#define SYM_T_DATA STT_OBJECT > +#define SYM_T_NONE STT_NOTYPE > + > +#define SYM(name, typ, linkage, align...) \ > + .type name, SYM_T_ ## typ; \ > + SYM_L_ ## linkage(name); \ > + SYM_ALIGN(align); \ > + name: > + > +#define END(name) .size name, . - name > + > +#define FUNC(name, align...) \ > + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) > +#define LABEL(name, align...) \ > + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) > +#define DATA(name, align...) \ > + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) > + > +#define FUNC_LOCAL(name, align...) \ > + SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) > +#define LABEL_LOCAL(name, align...) \ > + SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) One thing that I've noticed while reviewing further patches, the usage of CODE_ALIGN and CODE_FILL in LABEL_LOCAL() means that CODE_FILL must always be a nop, or else the assembler will put garbage while padding the symbol, and hence the preceding code will no longer fallthrough into the label? Looking further, LABEL_LOCAL() is always used when there's no fallthrough, but it's IMO not obvious from the description here. Thanks, Roger.
On 18.01.2024 15:52, Roger Pau Monné wrote: > On Mon, Jan 15, 2024 at 03:34:05PM +0100, Jan Beulich wrote: >> Recent gas versions generate minimalistic Dwarf debug info for items >> annotated as functions and having their sizes specified [1]. Furthermore >> generating live patches wants items properly annotated. "Borrow" Arm's >> END() and (remotely) derive other annotation infrastructure from >> Linux'es, for all architectures to use. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> [1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28 >> --- >> v5: Drop CODE_ALIGN fallback. s/algn/align/g. >> v3: New, generalized from earlier x86-only version. LAST() (now >> LASTARG()) moved to macros.h. >> --- >> TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be >> specified (in case this has some special meaning on an arch; >> conceivably it could mean to use some kind of arch default). We may >> not strictly need that, and hence we could also make these power- >> of-2 values (using .p2align). >> >> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we >> still have ALIGN. >> >> Note further that FUNC()'s etc "align" parameter is intended to allow >> for only no or a single argument. If we wanted to also make the fill >> value customizable per call site, the constructs would need re-doing to >> some degree. >> >> --- /dev/null >> +++ b/xen/include/xen/linkage.h >> @@ -0,0 +1,54 @@ >> +#ifndef __LINKAGE_H__ >> +#define __LINKAGE_H__ >> + >> +#ifdef __ASSEMBLY__ >> + >> +#include <xen/macros.h> >> + >> +/* CODE_ALIGN needs to be specified by every architecture. */ >> +#ifndef CODE_FILL >> +# define CODE_FILL ~0 >> +#endif >> + >> +#ifndef DATA_ALIGN >> +# define DATA_ALIGN 0 >> +#endif >> +#ifndef DATA_FILL >> +# define DATA_FILL ~0 >> +#endif >> + >> +#define SYM_ALIGN(align...) .balign align >> + >> +#define SYM_L_GLOBAL(name) .globl name >> +#define SYM_L_WEAK(name) .weak name >> +#define SYM_L_LOCAL(name) /* nothing */ >> + >> +#define SYM_T_FUNC STT_FUNC >> +#define SYM_T_DATA STT_OBJECT >> +#define SYM_T_NONE STT_NOTYPE >> + >> +#define SYM(name, typ, linkage, align...) \ >> + .type name, SYM_T_ ## typ; \ >> + SYM_L_ ## linkage(name); \ >> + SYM_ALIGN(align); \ >> + name: >> + >> +#define END(name) .size name, . - name >> + >> +#define FUNC(name, align...) \ >> + SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) >> +#define LABEL(name, align...) \ >> + SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) >> +#define DATA(name, align...) \ >> + SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL) >> + >> +#define FUNC_LOCAL(name, align...) \ >> + SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) >> +#define LABEL_LOCAL(name, align...) \ >> + SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL) > > One thing that I've noticed while reviewing further patches, the usage > of CODE_ALIGN and CODE_FILL in LABEL_LOCAL() means that CODE_FILL must > always be a nop, or else the assembler will put garbage while padding > the symbol, and hence the preceding code will no longer fallthrough > into the label? Well, except when an arch has no such cases, it'll need to override CODE_FILL. Hence why for now Arm and PPC architectures get away without such an override. It may well be that at least Arm will need to gain one as soon as the first case of falling through is converted to this new model. See also the RISC-V change, where it becomes permissible for CODE_FILL to expand to nothing (thus using assembler built-in defaults). > Looking further, LABEL_LOCAL() is always used when there's no > fallthrough, but it's IMO not obvious from the description here. Not sure what adjustment to the description you are thinking of. What's used where isn't dictated by this patch / framework. Jan
On Mon, Jan 15, 2024 at 03:34:05PM +0100, Jan Beulich wrote: > Recent gas versions generate minimalistic Dwarf debug info for items > annotated as functions and having their sizes specified [1]. Furthermore > generating live patches wants items properly annotated. "Borrow" Arm's > END() and (remotely) derive other annotation infrastructure from > Linux'es, for all architectures to use. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > [1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28 > --- > v5: Drop CODE_ALIGN fallback. s/algn/align/g. > v3: New, generalized from earlier x86-only version. LAST() (now > LASTARG()) moved to macros.h. > --- > TBD: {CODE,DATA}_ALIGN are byte granular, such that a value of 0 can be > specified (in case this has some special meaning on an arch; > conceivably it could mean to use some kind of arch default). We may > not strictly need that, and hence we could also make these power- > of-2 values (using .p2align). > > Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > still have ALIGN. SYM_ALIGN seems fine for the purpose here. > > Note further that FUNC()'s etc "align" parameter is intended to allow > for only no or a single argument. If we wanted to also make the fill > value customizable per call site, the constructs would need re-doing to > some degree. > > --- /dev/null > +++ b/xen/include/xen/linkage.h > @@ -0,0 +1,54 @@ > +#ifndef __LINKAGE_H__ > +#define __LINKAGE_H__ > + > +#ifdef __ASSEMBLY__ > + > +#include <xen/macros.h> > + > +/* CODE_ALIGN needs to be specified by every architecture. */ > +#ifndef CODE_FILL > +# define CODE_FILL ~0 > +#endif > + > +#ifndef DATA_ALIGN > +# define DATA_ALIGN 0 > +#endif > +#ifndef DATA_FILL > +# define DATA_FILL ~0 I find the fills a bit odd, compared to what we use now (nops for x86 IIRC). Thanks, Roger.
On 17.01.2024 18:02, Roger Pau Monné wrote: > On Mon, Jan 15, 2024 at 03:34:05PM +0100, Jan Beulich wrote: >> Recent gas versions generate minimalistic Dwarf debug info for items >> annotated as functions and having their sizes specified [1]. Furthermore >> generating live patches wants items properly annotated. "Borrow" Arm's >> END() and (remotely) derive other annotation infrastructure from >> Linux'es, for all architectures to use. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- /dev/null >> +++ b/xen/include/xen/linkage.h >> @@ -0,0 +1,54 @@ >> +#ifndef __LINKAGE_H__ >> +#define __LINKAGE_H__ >> + >> +#ifdef __ASSEMBLY__ >> + >> +#include <xen/macros.h> >> + >> +/* CODE_ALIGN needs to be specified by every architecture. */ >> +#ifndef CODE_FILL >> +# define CODE_FILL ~0 >> +#endif >> + >> +#ifndef DATA_ALIGN >> +# define DATA_ALIGN 0 >> +#endif >> +#ifndef DATA_FILL >> +# define DATA_FILL ~0 > > I find the fills a bit odd, compared to what we use now (nops for x86 > IIRC). Well, these are generic defaults. X86 then overrides CODE_FILL for it to remain NOP. ~0 is the best I can think of as an arch-agnostic default, considering the half dozen architectures I know at least a little. Jan
Use the generic framework in xen/linkage.h.
For switch_to_kernel() and restore_all_guest() so far implicit alignment
(from being first in their respective sections) is being made explicit
(as in: using FUNC() without 2nd argument). Whereas for
{,compat}create_bounce_frame() and autogen_entrypoints[] alignment is
newly arranged for.
Except for the added/adjusted alignment padding (including their
knock-on effects) no change in generated code/data. Note that the basis
for support of weak definitions is added despite them not having any use
right now.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Re-base.
v4: Correct simd_coprocessor_error's (now entry_XM) END(). Re-base.
v3: !PV variant of ret_from_intr is local. Introduction of macros split
off to separate patch. Also adjust ASM_INT(). Re-base.
v2: Full rework.
---
Only two of the assembly files are being converted for now. More could
be done right here or as follow-on in separate patches.
Note that the FB-label in autogen_stubs() cannot be converted just yet:
Such labels cannot be used with .type. We could further diverge from
Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type
labels get by default anyway).
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
I didn't think that I should make CODE_FILL evaluate to 0xCC right here;
IMO that wants to be a separate patch.
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -13,6 +13,7 @@
#include <asm/alternative.h>
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#include <asm/asm-defns.h>
#ifndef CONFIG_INDIRECT_THUNK
.equ CONFIG_INDIRECT_THUNK, 0
@@ -343,10 +344,7 @@ static always_inline void stac(void)
.popsection
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, @object
+ DATA(label, 4) .long (val); END(label)
#define ASM_CONSTANT(name, value) \
asm ( ".equ " #name ", %P0; .global " #name \
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -43,7 +43,9 @@
/* Linkage for x86 */
#ifdef __ASSEMBLY__
-#define ALIGN .align 16,0x90
+#define CODE_ALIGN 16
+#define CODE_FILL 0x90
+#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
.globl name; \
ALIGN; \
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -11,7 +11,7 @@
#include <public/xen.h>
#include <irq_vectors.h>
-ENTRY(entry_int82)
+FUNC(entry_int82)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -29,9 +29,10 @@ ENTRY(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+END(entry_int82)
/* %rbx: struct vcpu */
-ENTRY(compat_test_all_events)
+FUNC(compat_test_all_events)
ASSERT_NOT_IN_ATOMIC
cli # tests must not race interrupts
/*compat_test_softirqs:*/
@@ -68,24 +69,21 @@ compat_test_guest_events:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_softirqs:
+LABEL_LOCAL(compat_process_softirqs)
sti
call do_softirq
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx: struct trap_bounce */
-.Lcompat_process_trapbounce:
+LABEL_LOCAL(.Lcompat_process_trapbounce)
sti
.Lcompat_bounce_exception:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_mce:
+LABEL_LOCAL(compat_process_mce)
testb $1 << VCPU_TRAP_MCE,VCPU_async_exception_mask(%rbx)
jnz .Lcompat_test_guest_nmi
sti
@@ -99,9 +97,8 @@ compat_process_mce:
movb %dl,VCPU_async_exception_mask(%rbx)
jmp compat_process_trap
- ALIGN
/* %rbx: struct vcpu */
-compat_process_nmi:
+LABEL_LOCAL(compat_process_nmi)
testb $1 << VCPU_TRAP_NMI,VCPU_async_exception_mask(%rbx)
jnz compat_test_guest_events
sti
@@ -118,9 +115,10 @@ compat_process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_test_all_events)
/* %rbx: struct vcpu, interrupts disabled */
-ENTRY(compat_restore_all_guest)
+FUNC(compat_restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
mov $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11d
and UREGS_eflags(%rsp),%r11d
@@ -163,9 +161,10 @@ ENTRY(compat_restore_all_guest)
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(compat_restore_all_guest)
/* This mustn't modify registers other than %rax. */
-ENTRY(cr4_pv32_restore)
+FUNC(cr4_pv32_restore)
push %rdx
GET_CPUINFO_FIELD(cr4, dx)
mov (%rdx), %rax
@@ -195,8 +194,9 @@ ENTRY(cr4_pv32_restore)
pop %rdx
xor %eax, %eax
ret
+END(cr4_pv32_restore)
-ENTRY(compat_syscall)
+FUNC(compat_syscall)
/* Fix up reported %cs/%ss for compat domains. */
movl $FLAT_COMPAT_USER_SS, UREGS_ss(%rsp)
movl $FLAT_COMPAT_USER_CS, UREGS_cs(%rsp)
@@ -224,8 +224,9 @@ UNLIKELY_END(compat_syscall_gpf)
movw %si,TRAPBOUNCE_cs(%rdx)
movb %cl,TRAPBOUNCE_flags(%rdx)
jmp .Lcompat_bounce_exception
+END(compat_syscall)
-ENTRY(compat_sysenter)
+FUNC(compat_sysenter)
CR4_PV32_RESTORE
movq VCPU_trap_ctxt(%rbx),%rcx
cmpb $X86_EXC_GP, UREGS_entry_vector(%rsp)
@@ -238,17 +239,19 @@ ENTRY(compat_sysenter)
movw %ax,TRAPBOUNCE_cs(%rdx)
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_sysenter)
-ENTRY(compat_int80_direct_trap)
+FUNC(compat_int80_direct_trap)
CR4_PV32_RESTORE
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_int80_direct_trap)
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK: */
/* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-compat_create_bounce_frame:
+FUNC_LOCAL(compat_create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
mov %fs,%edi
ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
@@ -354,3 +357,4 @@ compat_crash_page_fault:
jmp .Lft14
.previous
_ASM_EXTABLE(.Lft14, .Lfx14)
+END(compat_create_bounce_frame)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -24,7 +24,7 @@
#ifdef CONFIG_PV
/* %rbx: struct vcpu */
-switch_to_kernel:
+FUNC_LOCAL(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
/* TB_eip = 32-bit syscall ? syscall32_addr : syscall_addr */
@@ -89,24 +89,21 @@ test_guest_events:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_softirqs:
+LABEL_LOCAL(process_softirqs)
sti
call do_softirq
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx struct trap_bounce */
-.Lprocess_trapbounce:
+LABEL_LOCAL(.Lprocess_trapbounce)
sti
.Lbounce_exception:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_mce:
+LABEL_LOCAL(process_mce)
testb $1 << VCPU_TRAP_MCE, VCPU_async_exception_mask(%rbx)
jnz .Ltest_guest_nmi
sti
@@ -120,9 +117,8 @@ process_mce:
movb %dl, VCPU_async_exception_mask(%rbx)
jmp process_trap
- ALIGN
/* %rbx: struct vcpu */
-process_nmi:
+LABEL_LOCAL(process_nmi)
testb $1 << VCPU_TRAP_NMI, VCPU_async_exception_mask(%rbx)
jnz test_guest_events
sti
@@ -139,11 +135,12 @@ process_trap:
leaq VCPU_trap_bounce(%rbx), %rdx
call create_bounce_frame
jmp test_all_events
+END(switch_to_kernel)
.section .text.entry, "ax", @progbits
/* %rbx: struct vcpu, interrupts disabled */
-restore_all_guest:
+FUNC_LOCAL(restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
/* Stash guest SPEC_CTRL value while we can read struct vcpu. */
@@ -220,8 +217,7 @@ restore_all_guest:
sysretq
1: sysretl
- ALIGN
-.Lrestore_rcx_iret_exit_to_guest:
+LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_guest)
movq 8(%rsp), %rcx # RIP
/* No special register assumptions. */
iret_exit_to_guest:
@@ -230,6 +226,7 @@ iret_exit_to_guest:
addq $8,%rsp
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(restore_all_guest)
/*
* When entering SYSCALL from kernel mode:
@@ -246,7 +243,7 @@ iret_exit_to_guest:
* - Guest %rsp stored in %rax
* - Xen stack loaded, pointing at the %ss slot
*/
-ENTRY(lstar_enter)
+FUNC(lstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -281,9 +278,10 @@ ENTRY(lstar_enter)
mov %rsp, %rdi
call pv_hypercall
jmp test_all_events
+END(lstar_enter)
/* See lstar_enter for entry register state. */
-ENTRY(cstar_enter)
+FUNC(cstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -321,8 +319,9 @@ ENTRY(cstar_enter)
jne compat_syscall
#endif
jmp switch_to_kernel
+END(cstar_enter)
-ENTRY(sysenter_entry)
+FUNC(sysenter_entry)
ENDBR64
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
@@ -330,7 +329,7 @@ ENTRY(sysenter_entry)
pushq $FLAT_USER_SS
pushq $0
pushfq
-GLOBAL(sysenter_eflags_saved)
+LABEL(sysenter_eflags_saved, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $3 /* ring 3 null cs */
pushq $0 /* null rip */
@@ -384,8 +383,9 @@ UNLIKELY_END(sysenter_gpf)
jne compat_sysenter
#endif
jmp .Lbounce_exception
+END(sysenter_entry)
-ENTRY(entry_int80)
+FUNC(entry_int80)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -473,6 +473,7 @@ int80_slow_path:
*/
GET_STACK_END(14)
jmp handle_exception_saved
+END(entry_int80)
/* create_bounce_frame & helpers don't need to be in .text.entry */
.text
@@ -481,7 +482,7 @@ int80_slow_path:
/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-create_bounce_frame:
+FUNC_LOCAL(create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jnz 1f
@@ -617,6 +618,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+END(create_bounce_frame)
#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -625,7 +627,7 @@ ENTRY(dom_crash_sync_extable)
/* No special register assumptions. */
#ifdef CONFIG_PV
-ENTRY(continue_pv_domain)
+FUNC(continue_pv_domain)
ENDBR64
call check_wakeup_from_wait
ret_from_intr:
@@ -640,26 +642,28 @@ ret_from_intr:
#else
jmp test_all_events
#endif
+END(continue_pv_domain)
#else
-ret_from_intr:
+FUNC_LOCAL(ret_from_intr, 0)
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
+END(ret_from_intr)
#endif
.section .init.text, "ax", @progbits
-ENTRY(early_page_fault)
+FUNC(early_page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
SAVE_ALL
movq %rsp, %rdi
call do_early_page_fault
jmp restore_all_xen
+END(early_page_fault)
.section .text.entry, "ax", @progbits
- ALIGN
/* %r12=ist_exit */
-restore_all_xen:
+FUNC_LOCAL(restore_all_xen)
#ifdef CONFIG_DEBUG
mov %rsp, %rdi
@@ -683,8 +687,9 @@ UNLIKELY_END(exit_cr3)
RESTORE_ALL adj=8
iretq
+END(restore_all_xen)
-ENTRY(common_interrupt)
+FUNC(common_interrupt)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -713,12 +718,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
+END(common_interrupt)
-ENTRY(entry_PF)
+FUNC(entry_PF)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
+END(entry_PF)
/* No special register assumptions. */
-GLOBAL(handle_exception)
+FUNC(handle_exception, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -886,92 +893,108 @@ FATAL_exception_with_ints_disabled:
xorl %esi,%esi
movq %rsp,%rdi
tailcall fatal_trap
+END(handle_exception)
-ENTRY(entry_DE)
+FUNC(entry_DE)
ENDBR64
pushq $0
movl $X86_EXC_DE, 4(%rsp)
jmp handle_exception
+END(entry_DE)
-ENTRY(entry_MF)
+FUNC(entry_MF)
ENDBR64
pushq $0
movl $X86_EXC_MF, 4(%rsp)
jmp handle_exception
+END(entry_MF)
-ENTRY(entry_XM)
+FUNC(entry_XM)
ENDBR64
pushq $0
movl $X86_EXC_XM, 4(%rsp)
jmp handle_exception
+END(entry_XM)
-ENTRY(entry_NM)
+FUNC(entry_NM)
ENDBR64
pushq $0
movl $X86_EXC_NM, 4(%rsp)
jmp handle_exception
+END(entry_NM)
-ENTRY(entry_DB)
+FUNC(entry_DB)
ENDBR64
pushq $0
movl $X86_EXC_DB, 4(%rsp)
jmp handle_ist_exception
+END(entry_DB)
-ENTRY(entry_BP)
+FUNC(entry_BP)
ENDBR64
pushq $0
movl $X86_EXC_BP, 4(%rsp)
jmp handle_exception
+END(entry_BP)
-ENTRY(entry_OF)
+FUNC(entry_OF)
ENDBR64
pushq $0
movl $X86_EXC_OF, 4(%rsp)
jmp handle_exception
+END(entry_OF)
-ENTRY(entry_BR)
+FUNC(entry_BR)
ENDBR64
pushq $0
movl $X86_EXC_BR, 4(%rsp)
jmp handle_exception
+END(entry_BR)
-ENTRY(entry_UD)
+FUNC(entry_UD)
ENDBR64
pushq $0
movl $X86_EXC_UD, 4(%rsp)
jmp handle_exception
+END(entry_UD)
-ENTRY(entry_TS)
+FUNC(entry_TS)
ENDBR64
movl $X86_EXC_TS, 4(%rsp)
jmp handle_exception
+END(entry_TS)
-ENTRY(entry_NP)
+FUNC(entry_NP)
ENDBR64
movl $X86_EXC_NP, 4(%rsp)
jmp handle_exception
+END(entry_NP)
-ENTRY(entry_SS)
+FUNC(entry_SS)
ENDBR64
movl $X86_EXC_SS, 4(%rsp)
jmp handle_exception
+END(entry_SS)
-ENTRY(entry_GP)
+FUNC(entry_GP)
ENDBR64
movl $X86_EXC_GP, 4(%rsp)
jmp handle_exception
+END(entry_GP)
-ENTRY(entry_AC)
+FUNC(entry_AC)
ENDBR64
movl $X86_EXC_AC, 4(%rsp)
jmp handle_exception
+END(entry_AC)
-ENTRY(entry_CP)
+FUNC(entry_CP)
ENDBR64
movl $X86_EXC_CP, 4(%rsp)
jmp handle_exception
+END(entry_CP)
-ENTRY(entry_DF)
+FUNC(entry_DF)
ENDBR64
movl $X86_EXC_DF, 4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
@@ -994,8 +1017,9 @@ ENTRY(entry_DF)
movq %rsp,%rdi
tailcall do_double_fault
+END(entry_DF)
-ENTRY(entry_NMI)
+FUNC(entry_NMI)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
@@ -1126,21 +1150,24 @@ handle_ist_exception:
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
#endif
+END(entry_NMI)
-ENTRY(entry_MC)
+FUNC(entry_MC)
ENDBR64
pushq $0
movl $X86_EXC_MC, 4(%rsp)
jmp handle_ist_exception
+END(entry_MC)
/* No op trap handler. Required for kexec crash path. */
-GLOBAL(trap_nop)
+FUNC(trap_nop, 0)
ENDBR64
iretq
+END(trap_nop)
/* Table of automatically generated entry points. One per vector. */
.pushsection .init.rodata, "a", @progbits
-GLOBAL(autogen_entrypoints)
+DATA(autogen_entrypoints, 8)
/* pop into the .init.rodata section and record an entry point. */
.macro entrypoint ent
.pushsection .init.rodata, "a", @progbits
@@ -1149,7 +1176,7 @@ GLOBAL(autogen_entrypoints)
.endm
.popsection
-autogen_stubs: /* Automatically generated stubs. */
+FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
vec = 0
.rept X86_NR_VECTORS
@@ -1193,6 +1220,7 @@ autogen_stubs: /* Automatically generate
vec = vec + 1
.endr
+END(autogen_stubs)
.section .init.rodata, "a", @progbits
- .size autogen_entrypoints, . - autogen_entrypoints
+END(autogen_entrypoints)
On Mon, Jan 15, 2024 at 03:34:56PM +0100, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. > > For switch_to_kernel() and restore_all_guest() so far implicit alignment > (from being first in their respective sections) is being made explicit > (as in: using FUNC() without 2nd argument). Whereas for > {,compat}create_bounce_frame() and autogen_entrypoints[] alignment is > newly arranged for. > > Except for the added/adjusted alignment padding (including their > knock-on effects) no change in generated code/data. Note that the basis > for support of weak definitions is added despite them not having any use > right now. > > Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only > use site wants the symbol global anyway. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On Mon, Jan 15, 2024 at 03:34:56PM +0100, Jan Beulich wrote: > @@ -625,7 +627,7 @@ ENTRY(dom_crash_sync_extable) > > /* No special register assumptions. */ > #ifdef CONFIG_PV > -ENTRY(continue_pv_domain) > +FUNC(continue_pv_domain) > ENDBR64 > call check_wakeup_from_wait > ret_from_intr: > @@ -640,26 +642,28 @@ ret_from_intr: > #else > jmp test_all_events > #endif > +END(continue_pv_domain) > #else > -ret_from_intr: > +FUNC_LOCAL(ret_from_intr, 0) Why does this need to have an alignment of 0? There's no fallthrough of previous code AFAICT. > ASSERT_CONTEXT_IS_XEN > jmp restore_all_xen > +END(ret_from_intr) > #endif > > .section .init.text, "ax", @progbits > -ENTRY(early_page_fault) > +FUNC(early_page_fault) > ENDBR64 > movl $X86_EXC_PF, 4(%rsp) > SAVE_ALL > movq %rsp, %rdi > call do_early_page_fault > jmp restore_all_xen > +END(early_page_fault) > > .section .text.entry, "ax", @progbits > > - ALIGN > /* %r12=ist_exit */ > -restore_all_xen: > +FUNC_LOCAL(restore_all_xen) > > #ifdef CONFIG_DEBUG > mov %rsp, %rdi > @@ -683,8 +687,9 @@ UNLIKELY_END(exit_cr3) > > RESTORE_ALL adj=8 > iretq > +END(restore_all_xen) > > -ENTRY(common_interrupt) > +FUNC(common_interrupt) > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > SAVE_ALL > > @@ -713,12 +718,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 > +END(common_interrupt) > > -ENTRY(entry_PF) > +FUNC(entry_PF) > ENDBR64 > movl $X86_EXC_PF, 4(%rsp) > +END(entry_PF) > /* No special register assumptions. */ > -GLOBAL(handle_exception) > +FUNC(handle_exception, 0) Given patch 8/8 that enables support for placing FUNC() into separate sections, the fallthrough arrangement here with entry_PF is no longer guaranteed, as the linker could re-order the sections and thus entry_PF could fallthrough into another text section? IOW: entry_PF needs a "jmp handle_exception", and then handle_exception itself can be padded as required by the default alignment? > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > SAVE_ALL > > @@ -886,92 +893,108 @@ FATAL_exception_with_ints_disabled: > xorl %esi,%esi > movq %rsp,%rdi > tailcall fatal_trap > +END(handle_exception) > > -ENTRY(entry_DE) > +FUNC(entry_DE) > ENDBR64 > pushq $0 > movl $X86_EXC_DE, 4(%rsp) > jmp handle_exception > +END(entry_DE) > > -ENTRY(entry_MF) > +FUNC(entry_MF) > ENDBR64 > pushq $0 > movl $X86_EXC_MF, 4(%rsp) > jmp handle_exception > +END(entry_MF) > > -ENTRY(entry_XM) > +FUNC(entry_XM) > ENDBR64 > pushq $0 > movl $X86_EXC_XM, 4(%rsp) > jmp handle_exception > +END(entry_XM) > > -ENTRY(entry_NM) > +FUNC(entry_NM) > ENDBR64 > pushq $0 > movl $X86_EXC_NM, 4(%rsp) > jmp handle_exception > +END(entry_NM) > > -ENTRY(entry_DB) > +FUNC(entry_DB) > ENDBR64 > pushq $0 > movl $X86_EXC_DB, 4(%rsp) > jmp handle_ist_exception > +END(entry_DB) > > -ENTRY(entry_BP) > +FUNC(entry_BP) > ENDBR64 > pushq $0 > movl $X86_EXC_BP, 4(%rsp) > jmp handle_exception > +END(entry_BP) > > -ENTRY(entry_OF) > +FUNC(entry_OF) > ENDBR64 > pushq $0 > movl $X86_EXC_OF, 4(%rsp) > jmp handle_exception > +END(entry_OF) > > -ENTRY(entry_BR) > +FUNC(entry_BR) > ENDBR64 > pushq $0 > movl $X86_EXC_BR, 4(%rsp) > jmp handle_exception > +END(entry_BR) > > -ENTRY(entry_UD) > +FUNC(entry_UD) > ENDBR64 > pushq $0 > movl $X86_EXC_UD, 4(%rsp) > jmp handle_exception > +END(entry_UD) > > -ENTRY(entry_TS) > +FUNC(entry_TS) > ENDBR64 > movl $X86_EXC_TS, 4(%rsp) > jmp handle_exception > +END(entry_TS) > > -ENTRY(entry_NP) > +FUNC(entry_NP) > ENDBR64 > movl $X86_EXC_NP, 4(%rsp) > jmp handle_exception > +END(entry_NP) > > -ENTRY(entry_SS) > +FUNC(entry_SS) > ENDBR64 > movl $X86_EXC_SS, 4(%rsp) > jmp handle_exception > +END(entry_SS) > > -ENTRY(entry_GP) > +FUNC(entry_GP) > ENDBR64 > movl $X86_EXC_GP, 4(%rsp) > jmp handle_exception > +END(entry_GP) > > -ENTRY(entry_AC) > +FUNC(entry_AC) > ENDBR64 > movl $X86_EXC_AC, 4(%rsp) > jmp handle_exception > +END(entry_AC) > > -ENTRY(entry_CP) > +FUNC(entry_CP) > ENDBR64 > movl $X86_EXC_CP, 4(%rsp) > jmp handle_exception > +END(entry_CP) > > -ENTRY(entry_DF) > +FUNC(entry_DF) > ENDBR64 > movl $X86_EXC_DF, 4(%rsp) > /* Set AC to reduce chance of further SMAP faults */ > @@ -994,8 +1017,9 @@ ENTRY(entry_DF) > > movq %rsp,%rdi > tailcall do_double_fault > +END(entry_DF) > > -ENTRY(entry_NMI) > +FUNC(entry_NMI) > ENDBR64 > pushq $0 > movl $X86_EXC_NMI, 4(%rsp) > @@ -1126,21 +1150,24 @@ handle_ist_exception: > ASSERT_CONTEXT_IS_XEN > jmp restore_all_xen > #endif > +END(entry_NMI) > > -ENTRY(entry_MC) > +FUNC(entry_MC) > ENDBR64 > pushq $0 > movl $X86_EXC_MC, 4(%rsp) > jmp handle_ist_exception > +END(entry_MC) > > /* No op trap handler. Required for kexec crash path. */ > -GLOBAL(trap_nop) > +FUNC(trap_nop, 0) Could this use the default alignment? > ENDBR64 > iretq > +END(trap_nop) > > /* Table of automatically generated entry points. One per vector. */ > .pushsection .init.rodata, "a", @progbits > -GLOBAL(autogen_entrypoints) > +DATA(autogen_entrypoints, 8) > /* pop into the .init.rodata section and record an entry point. */ > .macro entrypoint ent > .pushsection .init.rodata, "a", @progbits > @@ -1149,7 +1176,7 @@ GLOBAL(autogen_entrypoints) > .endm > > .popsection > -autogen_stubs: /* Automatically generated stubs. */ > +FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */ Won't it be good to align the stubs? As that's possible to make them faster? Thanks, Roger.
On 18.01.2024 18:45, Roger Pau Monné wrote: > On Mon, Jan 15, 2024 at 03:34:56PM +0100, Jan Beulich wrote: >> @@ -625,7 +627,7 @@ ENTRY(dom_crash_sync_extable) >> >> /* No special register assumptions. */ >> #ifdef CONFIG_PV >> -ENTRY(continue_pv_domain) >> +FUNC(continue_pv_domain) >> ENDBR64 >> call check_wakeup_from_wait >> ret_from_intr: >> @@ -640,26 +642,28 @@ ret_from_intr: >> #else >> jmp test_all_events >> #endif >> +END(continue_pv_domain) >> #else >> -ret_from_intr: >> +FUNC_LOCAL(ret_from_intr, 0) > > Why does this need to have an alignment of 0? There's no fallthrough > of previous code AFAICT. It doesn't have to, but see the description for where I thought it would make sense to newly introduce alignment; I simply didn't want to go too far with such changes leading to generated code being altered. This (and the other cases below) weren't in that group. Without ... >> ASSERT_CONTEXT_IS_XEN ... this I would be strongly inclined to switch ... >> jmp restore_all_xen ... to #define ret_from_intr restore_all_xen anyway. And perhaps we ought to change the "#else" above to "#elif !defined(NDEBUG)", at which point I'd say alignment isn't required here at all. >> @@ -713,12 +718,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 >> +END(common_interrupt) >> >> -ENTRY(entry_PF) >> +FUNC(entry_PF) >> ENDBR64 >> movl $X86_EXC_PF, 4(%rsp) >> +END(entry_PF) >> /* No special register assumptions. */ >> -GLOBAL(handle_exception) >> +FUNC(handle_exception, 0) > > Given patch 8/8 that enables support for placing FUNC() into separate > sections, the fallthrough arrangement here with entry_PF is no longer > guaranteed, as the linker could re-order the sections and thus > entry_PF could fallthrough into another text section? > > IOW: entry_PF needs a "jmp handle_exception", and then > handle_exception itself can be padded as required by the default > alignment? Oh, yes, very much so. Thanks for noticing. I'll do that in the later patch, though. >> @@ -1149,7 +1176,7 @@ GLOBAL(autogen_entrypoints) >> .endm >> >> .popsection >> -autogen_stubs: /* Automatically generated stubs. */ >> +FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */ > > Won't it be good to align the stubs? As that's possible to make them > faster? Well. If I used default alignment here, it would be the 1st stub only which gains alignment. I'd view that as simply inconsistent. You'll find there already is an ALIGN inside the .rept below. That covers only certain cases, but intentionally so, I believe: it's only entry points which shouldn't be reached anyway which get no alignment. So yes, in this case I clearly think there wants to explicitly be no alignment here. Jan
Let's have assembler symbols be consistent with C ones. In principle
there are (a few) cases where gas can produce smaller code this way,
just that for now there's a gas bug causing smaller code to be emitted
even when that shouldn't be the case.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
v5: Re-base over changes earlier in the series.
v3: Re-base over generalization of the annotations.
v2: New.
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -47,11 +47,11 @@
#define CODE_FILL 0x90
#define ALIGN .align CODE_ALIGN, CODE_FILL
#define ENTRY(name) \
- .globl name; \
ALIGN; \
- name:
+ GLOBAL(name)
#define GLOBAL(name) \
.globl name; \
+ .hidden name; \
name:
#endif
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -19,7 +19,7 @@
#define SYM_ALIGN(align...) .balign align
-#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
Use the generic framework in xen/linkage.h. No change in generated code
except for the changed padding value (noticable when config.gz isn't a
multiple of 4 in size). Plus of course the converted symbols change to
be hidden ones.
Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only
use site wants the symbol global anyway.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
Only one each of the assembly files is being converted for now. More
could be done right here or as follow-on in separate patches.
The ASM_INT() redundancy of .global will be eliminated by a subsequent
patch.
---
v3: New.
--- a/xen/arch/arm/arm32/lib/div64.S
+++ b/xen/arch/arm/arm32/lib/div64.S
@@ -42,7 +42,7 @@
* Clobbered regs: xl, ip
*/
-ENTRY(__do_div64)
+FUNC(__do_div64)
UNWIND(.fnstart)
@ Test for easy paths first.
@@ -206,4 +206,4 @@ Ldiv0_64:
ldr pc, [sp], #8
UNWIND(.fnend)
-ENDPROC(__do_div64)
+END(__do_div64)
--- a/xen/arch/arm/arm64/lib/clear_page.S
+++ b/xen/arch/arm/arm64/lib/clear_page.S
@@ -22,7 +22,7 @@
* Parameters:
* x0 - dest
*/
-ENTRY(clear_page)
+FUNC(clear_page)
mrs x1, dczid_el0
and w1, w1, #0xf
mov x2, #4
@@ -33,4 +33,4 @@ ENTRY(clear_page)
tst x0, #(PAGE_SIZE - 1)
b.ne 1b
ret
-ENDPROC(clear_page)
+END(clear_page)
--- a/xen/arch/arm/include/asm/asm_defns.h
+++ b/xen/arch/arm/include/asm/asm_defns.h
@@ -5,6 +5,7 @@
/* NB. Auto-generated from arch/.../asm-offsets.c */
#include <asm/asm-offsets.h>
#endif
+#include <xen/linkage.h>
#include <asm/processor.h>
/* Macros for generic assembly code */
@@ -30,10 +31,7 @@ label: .asciz msg;
#define RODATA_STR(label, msg) RODATA_SECT(.rodata.str, label, msg)
#define ASM_INT(label, val) \
- .p2align 2; \
-label: .long (val); \
- .size label, . - label; \
- .type label, %object
+ DATA(label, 4) .long (val); END(label)
#endif /* __ARM_ASM_DEFNS_H__ */
/*
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -53,7 +53,8 @@
/* Linkage for ARM */
#ifdef __ASSEMBLY__
-#define ALIGN .align 2
+#define CODE_ALIGN 4
+#define ALIGN .balign CODE_ALIGN
#define ENTRY(name) \
.globl name; \
ALIGN; \
@@ -61,8 +62,6 @@
#define GLOBAL(name) \
.globl name; \
name:
-#define END(name) \
- .size name, .-name
#define ENDPROC(name) \
.type name, %function; \
END(name)
On 15.01.2024 15:36, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except for the changed padding value (noticable when config.gz isn't a > multiple of 4 in size). Plus of course the converted symbols change to > be hidden ones. > > Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only > use site wants the symbol global anyway. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Reviewed-by: Julien Grall <jgrall@amazon.com> > --- > Only one each of the assembly files is being converted for now. More > could be done right here or as follow-on in separate patches. As this was meanwhile committed, I'd like to understand you preference for further conversion steps: I can certainly see to find time to make some actual progress here, but it might also be that you prefer to do so yourself. Please let me know. Jan
Hi Jan, On 22/01/2024 13:22, Jan Beulich wrote: > On 15.01.2024 15:36, Jan Beulich wrote: >> Use the generic framework in xen/linkage.h. No change in generated code >> except for the changed padding value (noticable when config.gz isn't a >> multiple of 4 in size). Plus of course the converted symbols change to >> be hidden ones. >> >> Note that ASM_INT() is switched to DATA(), not DATA_LOCAL(), as the only >> use site wants the symbol global anyway. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Reviewed-by: Julien Grall <jgrall@amazon.com> >> --- >> Only one each of the assembly files is being converted for now. More >> could be done right here or as follow-on in separate patches. > > As this was meanwhile committed, I'd like to understand you preference > for further conversion steps: I can certainly see to find time to make > some actual progress here, but it might also be that you prefer to do > so yourself. Please let me know. Sorry for the late reply. I will have a look. Cheers, -- Julien Grall
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbols change to be hidden ones and gain
a valid size.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Probably count_args_exp() should move to macros.h, but I first wanted to
see whether anyone can suggest any better approach for checking whether
a defined macro expands to nothing.
---
v5: Re-base.
v4: Also drop #undef-s from linker script.
v3: New.
--- a/xen/arch/riscv/entry.S
+++ b/xen/arch/riscv/entry.S
@@ -5,7 +5,7 @@
#include <asm/traps.h>
/* WIP: only works while interrupting Xen context */
-ENTRY(handle_trap)
+FUNC(handle_trap)
/* Exceptions from xen */
save_to_stack:
@@ -92,3 +92,4 @@ restore_registers:
REG_L sp, CPU_USER_REGS_SP(sp)
sret
+END(handle_trap)
--- a/xen/arch/riscv/include/asm/asm.h
+++ b/xen/arch/riscv/include/asm/asm.h
@@ -7,6 +7,7 @@
#define _ASM_RISCV_ASM_H
#ifdef __ASSEMBLY__
+#include <xen/linkage.h>
#define __ASM_STR(x) x
#else
#define __ASM_STR(x) #x
--- a/xen/arch/riscv/include/asm/config.h
+++ b/xen/arch/riscv/include/asm/config.h
@@ -69,12 +69,8 @@
/* Linkage for RISCV */
#ifdef __ASSEMBLY__
-#define ALIGN .align 4
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 16
+#define CODE_FILL /* empty */
#endif
#ifdef CONFIG_RISCV_64
--- a/xen/arch/riscv/riscv64/head.S
+++ b/xen/arch/riscv/riscv64/head.S
@@ -8,7 +8,7 @@
* a0 -> hart_id ( bootcpu_id )
* a1 -> dtb_base
*/
-ENTRY(start)
+FUNC(start)
/* Mask all interrupts */
csrw CSR_SIE, zero
@@ -60,19 +60,21 @@ ENTRY(start)
mv a1, s1
tail start_xen
+END(start)
.section .text, "ax", %progbits
-ENTRY(reset_stack)
+FUNC(reset_stack)
la sp, cpu0_boot_stack
li t0, STACK_SIZE
add sp, sp, t0
ret
+END(reset_stack)
.section .text.ident, "ax", %progbits
-ENTRY(turn_on_mmu)
+FUNC(turn_on_mmu)
sfence.vma
li t0, RV_STAGE1_MODE
@@ -84,3 +86,4 @@ ENTRY(turn_on_mmu)
csrw CSR_SATP, t1
jr a0
+END(turn_on_mmu)
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -1,9 +1,6 @@
#include <xen/lib.h>
#include <xen/xen.lds.h>
-#undef ENTRY
-#undef ALIGN
-
OUTPUT_ARCH(riscv)
ENTRY(start)
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -35,17 +35,28 @@
#define END(name) .size name, . - name
+/*
+ * CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
+ * which case we also need to get rid of the comma in the .balign directive.
+ */
+#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
+#else
+# define DO_CODE_ALIGN(align...) LASTARG(CODE_ALIGN, ## align)
+#endif
+
#define FUNC(name, align...) \
- SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+ SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(align))
#define LABEL(name, align...) \
- SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+ SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(align))
#define DATA(name, align...) \
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
#define FUNC_LOCAL(name, align...) \
- SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+ SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(align))
#define LABEL_LOCAL(name, align...) \
- SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), CODE_FILL)
+ SYM(name, NONE, LOCAL, DO_CODE_ALIGN(align))
#define DATA_LOCAL(name, align...) \
SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
On Mon, 2024-01-15 at 15:37 +0100, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated > code > except of course the converted symbols change to be hidden ones and > gain > a valid size. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Probably count_args_exp() should move to macros.h, but I first wanted > to > see whether anyone can suggest any better approach for checking > whether > a defined macro expands to nothing. > --- The current one approach looks good to me. Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > v5: Re-base. > v4: Also drop #undef-s from linker script. > v3: New. > > --- a/xen/arch/riscv/entry.S > +++ b/xen/arch/riscv/entry.S > @@ -5,7 +5,7 @@ > #include <asm/traps.h> > > /* WIP: only works while interrupting Xen context */ > -ENTRY(handle_trap) > +FUNC(handle_trap) > > /* Exceptions from xen */ > save_to_stack: > @@ -92,3 +92,4 @@ restore_registers: > REG_L sp, CPU_USER_REGS_SP(sp) > > sret > +END(handle_trap) > --- a/xen/arch/riscv/include/asm/asm.h > +++ b/xen/arch/riscv/include/asm/asm.h > @@ -7,6 +7,7 @@ > #define _ASM_RISCV_ASM_H > > #ifdef __ASSEMBLY__ > +#include <xen/linkage.h> > #define __ASM_STR(x) x > #else > #define __ASM_STR(x) #x > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -69,12 +69,8 @@ > > /* Linkage for RISCV */ > #ifdef __ASSEMBLY__ > -#define ALIGN .align 4 > - > -#define ENTRY(name) \ > - .globl name; \ > - ALIGN; \ > - name: > +#define CODE_ALIGN 16 > +#define CODE_FILL /* empty */ > #endif > > #ifdef CONFIG_RISCV_64 > --- a/xen/arch/riscv/riscv64/head.S > +++ b/xen/arch/riscv/riscv64/head.S > @@ -8,7 +8,7 @@ > * a0 -> hart_id ( bootcpu_id ) > * a1 -> dtb_base > */ > -ENTRY(start) > +FUNC(start) > /* Mask all interrupts */ > csrw CSR_SIE, zero > > @@ -60,19 +60,21 @@ ENTRY(start) > mv a1, s1 > > tail start_xen > +END(start) > > .section .text, "ax", %progbits > > -ENTRY(reset_stack) > +FUNC(reset_stack) > la sp, cpu0_boot_stack > li t0, STACK_SIZE > add sp, sp, t0 > > ret > +END(reset_stack) > > .section .text.ident, "ax", %progbits > > -ENTRY(turn_on_mmu) > +FUNC(turn_on_mmu) > sfence.vma > > li t0, RV_STAGE1_MODE > @@ -84,3 +86,4 @@ ENTRY(turn_on_mmu) > csrw CSR_SATP, t1 > > jr a0 > +END(turn_on_mmu) > --- a/xen/arch/riscv/xen.lds.S > +++ b/xen/arch/riscv/xen.lds.S > @@ -1,9 +1,6 @@ > #include <xen/lib.h> > #include <xen/xen.lds.h> > > -#undef ENTRY > -#undef ALIGN > - > OUTPUT_ARCH(riscv) > ENTRY(start) > > --- a/xen/include/xen/linkage.h > +++ b/xen/include/xen/linkage.h > @@ -35,17 +35,28 @@ > > #define END(name) .size name, . - name > > +/* > + * CODE_FILL in particular may need to expand to nothing (e.g. for > RISC-V), in > + * which case we also need to get rid of the comma in the .balign > directive. > + */ > +#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 > +#else > +# define DO_CODE_ALIGN(align...) LASTARG(CODE_ALIGN, ## align) > +#endif > + > #define FUNC(name, align...) \ > - SYM(name, FUNC, GLOBAL, LASTARG(CODE_ALIGN, ## align), > CODE_FILL) > + SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(align)) > #define LABEL(name, align...) \ > - SYM(name, NONE, GLOBAL, LASTARG(CODE_ALIGN, ## align), > CODE_FILL) > + SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(align)) > #define DATA(name, align...) \ > SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), > DATA_FILL) > > #define FUNC_LOCAL(name, align...) \ > - SYM(name, FUNC, LOCAL, LASTARG(CODE_ALIGN, ## align), > CODE_FILL) > + SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(align)) > #define LABEL_LOCAL(name, align...) \ > - SYM(name, NONE, LOCAL, LASTARG(CODE_ALIGN, ## align), > CODE_FILL) > + SYM(name, NONE, LOCAL, DO_CODE_ALIGN(align)) > #define DATA_LOCAL(name, align...) \ > SYM(name, DATA, LOCAL, LASTARG(DATA_ALIGN, ## align), > DATA_FILL) > >
Use the generic framework in xen/linkage.h. No change in generated code
except of course the converted symbols change to be hidden ones.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v5: Re-base.
v4: Also drop #undef-s from linker script. Re-base.
v3: New.
--- a/xen/arch/ppc/include/asm/asm-defns.h
+++ b/xen/arch/ppc/include/asm/asm-defns.h
@@ -3,6 +3,7 @@
#define _ASM_PPC_ASM_DEFNS_H
#include <asm/asm-offsets.h>
+#include <xen/linkage.h>
/*
* Load a 64-bit immediate value into the specified GPR.
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -32,12 +32,7 @@
/* Linkage for PPC */
#ifdef __ASSEMBLY__
-#define ALIGN .p2align 2
-
-#define ENTRY(name) \
- .globl name; \
- ALIGN; \
- name:
+#define CODE_ALIGN 4
#endif
#define XEN_VIRT_START _AC(0xc000000000000000, UL)
--- a/xen/arch/ppc/ppc64/exceptions-asm.S
+++ b/xen/arch/ppc/ppc64/exceptions-asm.S
@@ -6,7 +6,7 @@
.section .text.exceptions, "ax", %progbits
/* Helper to dump CPU state to struct cpu_user_regs pointed to by r1. */
-ENTRY(exception_common)
+FUNC(exception_common)
/*
* Save GPRs 1-31. TODO: The value of %r1 has already been modified by the
* ISR, so the value we save isn't the exact value we had on entry.
@@ -45,11 +45,10 @@ ENTRY(exception_common)
stdu %r0, -STACK_FRAME_OVERHEAD(%r1)
bl exception_handler
- .size exception_common, . - exception_common
- .type exception_common, %function
+ END(exception_common)
/* Same as exception_common, but for exceptions that set HSRR{0,1} */
-ENTRY(h_exception_common)
+FUNC(h_exception_common)
/*
* Save GPRs 1-31. TODO: The value of %r1 has already been modified by the
* ISR, so the value we save isn't the exact value we had on entry.
@@ -89,15 +88,14 @@ ENTRY(h_exception_common)
stdu %r0, -STACK_FRAME_OVERHEAD(%r1)
bl exception_handler
- .size h_exception_common, . - h_exception_common
- .type h_exception_common, %function
+ END(h_exception_common)
/*
* Declare an ISR for the provided exception that jumps to the specified handler
*/
.macro ISR name, exc, handler
. = (AIL_VECTOR_BASE - EXCEPTION_VECTORS_START) + \exc
- ENTRY(\name)
+ FUNC(\name)
/* TODO: switch stack */
/* Reserve space for struct cpu_user_regs */
@@ -113,8 +111,7 @@ ENTRY(h_exception_common)
/* Branch to common code */
b \handler
- .size \name, . - \name
- .type \name, %function
+ END(\name)
.endm
/*
--- a/xen/arch/ppc/ppc64/head.S
+++ b/xen/arch/ppc/ppc64/head.S
@@ -6,7 +6,7 @@
.section .text.header, "ax", %progbits
-ENTRY(start)
+FUNC(start)
/*
* NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
*/
@@ -64,11 +64,9 @@ ENTRY(start)
/* should never return */
trap
+END(start)
- .size start, . - start
- .type start, %function
-
-ENTRY(enable_mmu)
+FUNC(enable_mmu)
mflr %r3
mfmsr %r4
@@ -78,6 +76,4 @@ ENTRY(enable_mmu)
mtsrr0 %r3 /* return to caller after MMU enable */
mtsrr1 %r4
rfid
-
- .size enable_mmu, . - enable_mmu
- .type enable_mmu, %function
+END(enable_mmu)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -1,9 +1,6 @@
#include <xen/lib.h>
#include <xen/xen.lds.h>
-#undef ENTRY
-#undef ALIGN
-
OUTPUT_ARCH(powerpc:common64)
ENTRY(start)
On 15.01.2024 15:38, Jan Beulich wrote: > Use the generic framework in xen/linkage.h. No change in generated code > except of course the converted symbols change to be hidden ones. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> The other architectures have been at least partly switched; would be nice for PPC to follow suit. May I ask for an ack (or otherwise here), please? Thanks, Jan
Hi Jan, On 1/22/24 7:20 AM, Jan Beulich wrote: > On 15.01.2024 15:38, Jan Beulich wrote: >> Use the generic framework in xen/linkage.h. No change in generated code >> except of course the converted symbols change to be hidden ones. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > The other architectures have been at least partly switched; would be nice > for PPC to follow suit. May I ask for an ack (or otherwise here), please? > Sorry for the delay. Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com> > Thanks, Jan Thanks, Shawn
Use DATA() / END() and drop the now redundant .global. No change in
generated data; of course the two symbols now properly gain "hidden"
binding.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v3: New.
--- a/xen/tools/binfile
+++ b/xen/tools/binfile
@@ -29,15 +29,10 @@ cat <<EOF >$target
.section $section.rodata, "a", %progbits
- .p2align $align
- .global $varname
-$varname:
+DATA($varname, 1 << $align)
.incbin "$binsource"
.Lend:
+END($varname)
- .type $varname, %object
- .size $varname, .Lend - $varname
-
- .global ${varname}_size
ASM_INT(${varname}_size, .Lend - $varname)
EOF
Leverage the new infrastructure in xen/linkage.h to also switch to per-
function sections (when configured), deriving the specific name from the
"base" section in use at the time FUNC() is invoked.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really
wanted side effect of this change is that respective out-of-line
code now moves much closer to its original (invoking) code.
TBD: Of course something with the same overall effect, but less
impactful might do in Config.mk. E.g. $(filter-out -D%,$(3))
instead of $(firstword (3)).
TBD: On top of Roger's respective patch (for livepatch), also respect
CONFIG_FUNCTION_ALIGNMENT.
Note that we'd need to split DATA() in order to separate r/w and r/o
contributions. Further splitting might be needed to also support more
advanced attributes (e.g. merge), hence why this isn't done right here.
Sadly while a new section's name can be derived from the presently in
use, its attributes cannot be. Perhaps the only thing we can do is give
DATA() a 2nd mandatory parameter. Then again I guess most data
definitions could be moved to C anyway.
---
v5: Re-base over changes earlier in the series.
v4: Re-base.
v2: Make detection properly fail on old gas (by adjusting
cc-option-add-closure).
--- a/Config.mk
+++ b/Config.mk
@@ -102,7 +102,7 @@ cc-option = $(shell if $(1) $(2:-Wno-%=-
# Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6)
cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3)))
define cc-option-add-closure
- ifneq ($$(call cc-option,$$($(2)),$(3),n),n)
+ ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n)
$(1) += $(3)
endif
endef
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__
$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack)
+# Check to see whether the assmbler supports the --sectname-subst option.
+$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST)
+
LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments
CFLAGS += $(CFLAGS-y)
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -156,6 +156,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
*(.altinstr_replacement)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -104,6 +104,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -104,6 +104,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -86,6 +86,9 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
_stextentry = .;
*(.text.entry)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.text.entry.*)
+#endif
. = ALIGN(PAGE_SIZE);
_etextentry = .;
@@ -214,6 +217,9 @@ SECTIONS
#endif
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
*(.text.startup)
_einittext = .;
/*
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -19,6 +19,14 @@
#define SYM_ALIGN(align...) .balign align
+#if defined(HAVE_AS_SECTNAME_SUBST) && defined(CONFIG_CC_SPLIT_SECTIONS)
+# define SYM_PUSH_SECTION(name, attr) \
+ .pushsection %S.name, attr, %progbits; \
+ .equ .Lsplit_section, 1
+#else
+# define SYM_PUSH_SECTION(name, attr)
+#endif
+
#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
@@ -33,7 +41,14 @@
SYM_ALIGN(align); \
name:
-#define END(name) .size name, . - name
+#define END(name) \
+ .size name, . - name; \
+ .ifdef .Lsplit_section; \
+ .if .Lsplit_section; \
+ .popsection; \
+ .equ .Lsplit_section, 0; \
+ .endif; \
+ .endif
/*
* CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
@@ -47,6 +62,7 @@
#endif
#define FUNC(name, align...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(align))
#define LABEL(name, align...) \
SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(align))
@@ -54,6 +70,7 @@
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
#define FUNC_LOCAL(name, align...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(align))
#define LABEL_LOCAL(name, align...) \
SYM(name, NONE, LOCAL, DO_CODE_ALIGN(align))
On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote: > Leverage the new infrastructure in xen/linkage.h to also switch to per- > function sections (when configured), deriving the specific name from the > "base" section in use at the time FUNC() is invoked. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really > wanted side effect of this change is that respective out-of-line > code now moves much closer to its original (invoking) code. Hm, I'm afraid I don't have much useful suggestions here. > TBD: Of course something with the same overall effect, but less > impactful might do in Config.mk. E.g. $(filter-out -D%,$(3)) > instead of $(firstword (3)). I don't have a strong opinion re those two options My preference however (see below for reasoning) would be to put this detection in Kconfig. > TBD: On top of Roger's respective patch (for livepatch), also respect > CONFIG_FUNCTION_ALIGNMENT. I think you can drop that, as the series is blocked. > Note that we'd need to split DATA() in order to separate r/w and r/o > contributions. Further splitting might be needed to also support more > advanced attributes (e.g. merge), hence why this isn't done right here. > Sadly while a new section's name can be derived from the presently in > use, its attributes cannot be. Perhaps the only thing we can do is give > DATA() a 2nd mandatory parameter. Then again I guess most data > definitions could be moved to C anyway. > --- > v5: Re-base over changes earlier in the series. > v4: Re-base. > v2: Make detection properly fail on old gas (by adjusting > cc-option-add-closure). > > --- a/Config.mk > +++ b/Config.mk > @@ -102,7 +102,7 @@ cc-option = $(shell if $(1) $(2:-Wno-%=- > # Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6) > cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3))) > define cc-option-add-closure > - ifneq ($$(call cc-option,$$($(2)),$(3),n),n) > + ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n) > $(1) += $(3) > endif > endef > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__ > > $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack) > > +# Check to see whether the assmbler supports the --sectname-subst option. > +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST) I guess you already know what I'm going to comment on. I think this would be clearer if it was a Kconfig option. For once because I think we should gate livapatch support on the option being available, and secondly it would avoid having to pass the extra -D around. I think it's relevant to have a consistent set of build tool options requirements for livepatch support, so that when enabled the support is consistent across builds. With this approach livepatch could be enabled in Kconfig, but depending on the tools support the resulting binary might or might not support live patching of assembly code. Such behavior is IMO unhelpful from a user PoV, and can lead to surprises down the road. Thanks, Roger.
On 19.01.2024 11:36, Roger Pau Monné wrote: > On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote: >> Leverage the new infrastructure in xen/linkage.h to also switch to per- >> function sections (when configured), deriving the specific name from the >> "base" section in use at the time FUNC() is invoked. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really >> wanted side effect of this change is that respective out-of-line >> code now moves much closer to its original (invoking) code. > > Hm, I'm afraid I don't have much useful suggestions here. > >> TBD: Of course something with the same overall effect, but less >> impactful might do in Config.mk. E.g. $(filter-out -D%,$(3)) >> instead of $(firstword (3)). > > I don't have a strong opinion re those two options My preference > however (see below for reasoning) would be to put this detection in > Kconfig. > >> TBD: On top of Roger's respective patch (for livepatch), also respect >> CONFIG_FUNCTION_ALIGNMENT. > > I think you can drop that, as the series is blocked. Considering the series here has been pending for quite some time, too, I guess I'd like to keep it just in case that other functionality becomes unblocked or available by some other means, even if only to remind myself. >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__ >> >> $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack) >> >> +# Check to see whether the assmbler supports the --sectname-subst option. >> +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST) > > I guess you already know what I'm going to comment on. I think this > would be clearer if it was a Kconfig option. For once because I think > we should gate livapatch support on the option being available, and > secondly it would avoid having to pass the extra -D around. > > I think it's relevant to have a consistent set of build tool options > requirements for livepatch support, so that when enabled the support > is consistent across builds. With this approach livepatch could be > enabled in Kconfig, but depending on the tools support the resulting > binary might or might not support live patching of assembly code. > Such behavior is IMO unhelpful from a user PoV, and can lead to > surprises down the road. I can see the desire to have LIVEPATCH grow such a dependency. Yet there is the bigger still open topic of the criteria towards what, if anything, to probe for in Kconfig, what, if anything, to probe for in Makefile, and which of the probing perhaps do in both places. I'm afraid my attempts to move us closer to a resolution (topic on summit, making proposals on list) have utterly failed. IOW I don't currently see how the existing disagreement there can be resolved, which will result in me to continue following the (traditional) Makefile approach unless I clearly view Kconfig superior in a particular case. I could perhaps be talked into following a "mixed Kconfig/Makefile model", along the lines of "x86: convert CET tool chain feature checks to mixed Kconfig/Makefile model", albeit I'm sure you're aware there are issues to sort out there, which I see no value in putting time into as long as I can't expect things to make progress subsequently. Dropping this patch, while an option, would seem undesirable to me, since even without Kconfig probing using new enough tool chains the splitting would yield reliable results, and provide - imo - an improvement over what we have right now. Jan
On Mon, Jan 22, 2024 at 11:50:08AM +0100, Jan Beulich wrote: > On 19.01.2024 11:36, Roger Pau Monné wrote: > > On Mon, Jan 15, 2024 at 03:40:19PM +0100, Jan Beulich wrote: > >> Leverage the new infrastructure in xen/linkage.h to also switch to per- > >> function sections (when configured), deriving the specific name from the > >> "base" section in use at the time FUNC() is invoked. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really > >> wanted side effect of this change is that respective out-of-line > >> code now moves much closer to its original (invoking) code. > > > > Hm, I'm afraid I don't have much useful suggestions here. > > > >> TBD: Of course something with the same overall effect, but less > >> impactful might do in Config.mk. E.g. $(filter-out -D%,$(3)) > >> instead of $(firstword (3)). > > > > I don't have a strong opinion re those two options My preference > > however (see below for reasoning) would be to put this detection in > > Kconfig. > > > >> TBD: On top of Roger's respective patch (for livepatch), also respect > >> CONFIG_FUNCTION_ALIGNMENT. > > > > I think you can drop that, as the series is blocked. > > Considering the series here has been pending for quite some time, too, > I guess I'd like to keep it just in case that other functionality > becomes unblocked or available by some other means, even if only to > remind myself. So as you have seen I've posted a new version of just the function alignment patch, that I expected wasn't controversial. > >> --- a/xen/Makefile > >> +++ b/xen/Makefile > >> @@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__ > >> > >> $(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack) > >> > >> +# Check to see whether the assmbler supports the --sectname-subst option. > >> +$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST) > > > > I guess you already know what I'm going to comment on. I think this > > would be clearer if it was a Kconfig option. For once because I think > > we should gate livapatch support on the option being available, and > > secondly it would avoid having to pass the extra -D around. > > > > I think it's relevant to have a consistent set of build tool options > > requirements for livepatch support, so that when enabled the support > > is consistent across builds. With this approach livepatch could be > > enabled in Kconfig, but depending on the tools support the resulting > > binary might or might not support live patching of assembly code. > > Such behavior is IMO unhelpful from a user PoV, and can lead to > > surprises down the road. > > I can see the desire to have LIVEPATCH grow such a dependency. Yet there > is the bigger still open topic of the criteria towards what, if anything, > to probe for in Kconfig, what, if anything, to probe for in Makefile, and > which of the probing perhaps do in both places. I'm afraid my attempts to > move us closer to a resolution (topic on summit, making proposals on > list) have utterly failed. IOW I don't currently see how the existing > disagreement there can be resolved, which will result in me to continue > following the (traditional) Makefile approach unless I clearly view > Kconfig superior in a particular case. I could perhaps be talked into > following a "mixed Kconfig/Makefile model", along the lines of "x86: > convert CET tool chain feature checks to mixed Kconfig/Makefile model", > albeit I'm sure you're aware there are issues to sort out there, which I > see no value in putting time into as long as I can't expect things to > make progress subsequently. I think there are more subtle cases where it's indeed arguable that putting it in Kconfig or a Makefile makes no difference from a user experience PoV, hence in the context here I do believe it wants to be in Kconfig as LIVEPATCH can be make dependent on it. > Dropping this patch, while an option, would seem undesirable to me, since > even without Kconfig probing using new enough tool chains the splitting > would yield reliable results, and provide - imo - an improvement over > what we have right now. I could send a followup afterwards to arrange for the check to be in Kconfig, but it would feel a bit odd to me this is not done in the first place. I don't want to block the patch because I think it's useful, so worst case I'm willing to give my Ack and provide an alternative Kconfig based patch afterwards. Thanks, Roger.
The model introduced in patch 1 is in principle arch-agnostic, hence why I'm including Arm and RISC-V reviewers here as well. 1: annotate entry points with type and size 2: also mark assembler globals hidden Jan
Recent gas versions generate minimalistic Dwarf debug info for items
annotated as functions and having their sizes specified [1]. "Borrow"
Arm's END() and (remotely) derive other annotation infrastructure from
Linux'es.
For switch_to_kernel() and restore_all_guest() so far implicit alignment
(from being first in their respective sections) is being made explicit
(as in: using FUNC() without 2nd argument). Whereas for
{,compat}create_bounce_frame() and autogen_entrypoints[] alignment is
newly arranged for.
Except for the added alignment padding (including their knock-on
effects) no change in generated code/data.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
[1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28
---
v2: Full rework.
---
Only two of the assembly files are being converted for now. More could
be done right here or as follow-on in separate patches.
In principle the framework should be possible to use by other
architectures as well. If we want this, the main questions are going to
be:
- What header file name? (I don't really like Linux'es linkage.h, so I'd
prefer e.g. asm-defns.h or asm_defns.h as we already have in x86.)
- How much per-arch customization do we want to permit up front (i.e.
without knowing how much of it is going to be needed)? Initially I'd
expect only the default function alignment (and padding) to require
per-arch definitions.
Note that the FB-label in autogen_stubs() cannot be converted just yet:
Such labels cannot be used with .type. We could further diverge from
Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type
labels get by default anyway).
Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we
still have ALIGN.
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -81,6 +81,45 @@ register unsigned long current_stack_poi
#ifdef __ASSEMBLY__
+#define SYM_ALIGN(algn...) .balign algn
+
+#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_WEAK(name) .weak name
+#define SYM_L_LOCAL(name) /* nothing */
+
+#define SYM_T_FUNC STT_FUNC
+#define SYM_T_DATA STT_OBJECT
+#define SYM_T_NONE STT_NOTYPE
+
+#define SYM(name, typ, linkage, algn...) \
+ .type name, SYM_T_ ## typ; \
+ SYM_L_ ## linkage(name); \
+ SYM_ALIGN(algn); \
+ name:
+
+#define END(name) .size name, . - name
+
+#define ARG1_(x, y...) (x)
+#define ARG2_(x, y...) ARG1_(y)
+
+#define LAST__(nr) ARG ## nr ## _
+#define LAST_(nr) LAST__(nr)
+#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y)
+
+#define FUNC(name, algn...) \
+ SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90)
+#define LABEL(name, algn...) \
+ SYM(name, NONE, GLOBAL, LAST(16, ## algn), 0x90)
+#define DATA(name, algn...) \
+ SYM(name, DATA, GLOBAL, LAST(0, ## algn), 0xff)
+
+#define FUNC_LOCAL(name, algn...) \
+ SYM(name, FUNC, LOCAL, LAST(16, ## algn), 0x90)
+#define LABEL_LOCAL(name, algn...) \
+ SYM(name, NONE, LOCAL, LAST(16, ## algn), 0x90)
+#define DATA_LOCAL(name, algn...) \
+ SYM(name, DATA, LOCAL, LAST(0, ## algn), 0xff)
+
#ifdef HAVE_AS_QUOTED_SYM
#define SUBSECTION_LBL(tag) \
.ifndef .L.tag; \
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -8,10 +8,11 @@
#include <asm/page.h>
#include <asm/processor.h>
#include <asm/desc.h>
+#include <xen/lib.h>
#include <public/xen.h>
#include <irq_vectors.h>
-ENTRY(entry_int82)
+FUNC(entry_int82)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -27,9 +28,10 @@ ENTRY(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+END(entry_int82)
/* %rbx: struct vcpu */
-ENTRY(compat_test_all_events)
+FUNC(compat_test_all_events)
ASSERT_NOT_IN_ATOMIC
cli # tests must not race interrupts
/*compat_test_softirqs:*/
@@ -66,24 +68,21 @@ compat_test_guest_events:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_softirqs:
+LABEL_LOCAL(compat_process_softirqs)
sti
call do_softirq
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx: struct trap_bounce */
-.Lcompat_process_trapbounce:
+LABEL_LOCAL(.Lcompat_process_trapbounce)
sti
.Lcompat_bounce_exception:
call compat_create_bounce_frame
jmp compat_test_all_events
- ALIGN
/* %rbx: struct vcpu */
-compat_process_mce:
+LABEL_LOCAL(compat_process_mce)
testb $1 << VCPU_TRAP_MCE,VCPU_async_exception_mask(%rbx)
jnz .Lcompat_test_guest_nmi
sti
@@ -97,9 +96,8 @@ compat_process_mce:
movb %dl,VCPU_async_exception_mask(%rbx)
jmp compat_process_trap
- ALIGN
/* %rbx: struct vcpu */
-compat_process_nmi:
+LABEL_LOCAL(compat_process_nmi)
testb $1 << VCPU_TRAP_NMI,VCPU_async_exception_mask(%rbx)
jnz compat_test_guest_events
sti
@@ -116,9 +114,10 @@ compat_process_trap:
leaq VCPU_trap_bounce(%rbx),%rdx
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_test_all_events)
/* %rbx: struct vcpu, interrupts disabled */
-ENTRY(compat_restore_all_guest)
+FUNC(compat_restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
mov $~(X86_EFLAGS_IOPL | X86_EFLAGS_VM), %r11d
and UREGS_eflags(%rsp),%r11d
@@ -161,9 +160,10 @@ ENTRY(compat_restore_all_guest)
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(compat_restore_all_guest)
/* This mustn't modify registers other than %rax. */
-ENTRY(cr4_pv32_restore)
+FUNC(cr4_pv32_restore)
push %rdx
GET_CPUINFO_FIELD(cr4, dx)
mov (%rdx), %rax
@@ -193,8 +193,9 @@ ENTRY(cr4_pv32_restore)
pop %rdx
xor %eax, %eax
ret
+END(cr4_pv32_restore)
-ENTRY(compat_syscall)
+FUNC(compat_syscall)
/* Fix up reported %cs/%ss for compat domains. */
movl $FLAT_COMPAT_USER_SS, UREGS_ss(%rsp)
movl $FLAT_COMPAT_USER_CS, UREGS_cs(%rsp)
@@ -222,8 +223,9 @@ UNLIKELY_END(compat_syscall_gpf)
movw %si,TRAPBOUNCE_cs(%rdx)
movb %cl,TRAPBOUNCE_flags(%rdx)
jmp .Lcompat_bounce_exception
+END(compat_syscall)
-ENTRY(compat_sysenter)
+FUNC(compat_sysenter)
CR4_PV32_RESTORE
movq VCPU_trap_ctxt(%rbx),%rcx
cmpb $X86_EXC_GP, UREGS_entry_vector(%rsp)
@@ -236,17 +238,19 @@ ENTRY(compat_sysenter)
movw %ax,TRAPBOUNCE_cs(%rdx)
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_sysenter)
-ENTRY(compat_int80_direct_trap)
+FUNC(compat_int80_direct_trap)
CR4_PV32_RESTORE
call compat_create_bounce_frame
jmp compat_test_all_events
+END(compat_int80_direct_trap)
/* CREATE A BASIC EXCEPTION FRAME ON GUEST OS (RING-1) STACK: */
/* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-compat_create_bounce_frame:
+FUNC_LOCAL(compat_create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
mov %fs,%edi
ALTERNATIVE "", stac, X86_FEATURE_XEN_SMAP
@@ -352,3 +356,4 @@ compat_crash_page_fault:
jmp .Lft14
.previous
_ASM_EXTABLE(.Lft14, .Lfx14)
+END(compat_create_bounce_frame)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -9,6 +9,7 @@
#include <asm/asm_defns.h>
#include <asm/page.h>
#include <asm/processor.h>
+#include <xen/lib.h>
#include <public/xen.h>
#include <irq_vectors.h>
@@ -24,7 +25,7 @@
#ifdef CONFIG_PV
/* %rbx: struct vcpu */
-switch_to_kernel:
+FUNC_LOCAL(switch_to_kernel)
leaq VCPU_trap_bounce(%rbx),%rdx
/* TB_eip = 32-bit syscall ? syscall32_addr : syscall_addr */
@@ -89,24 +90,21 @@ test_guest_events:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_softirqs:
+LABEL_LOCAL(process_softirqs)
sti
call do_softirq
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu, %rdx struct trap_bounce */
-.Lprocess_trapbounce:
+LABEL_LOCAL(.Lprocess_trapbounce)
sti
.Lbounce_exception:
call create_bounce_frame
jmp test_all_events
- ALIGN
/* %rbx: struct vcpu */
-process_mce:
+LABEL_LOCAL(process_mce)
testb $1 << VCPU_TRAP_MCE, VCPU_async_exception_mask(%rbx)
jnz .Ltest_guest_nmi
sti
@@ -120,9 +118,8 @@ process_mce:
movb %dl, VCPU_async_exception_mask(%rbx)
jmp process_trap
- ALIGN
/* %rbx: struct vcpu */
-process_nmi:
+LABEL_LOCAL(process_nmi)
testb $1 << VCPU_TRAP_NMI, VCPU_async_exception_mask(%rbx)
jnz test_guest_events
sti
@@ -139,11 +136,12 @@ process_trap:
leaq VCPU_trap_bounce(%rbx), %rdx
call create_bounce_frame
jmp test_all_events
+END(switch_to_kernel)
.section .text.entry, "ax", @progbits
/* %rbx: struct vcpu, interrupts disabled */
-restore_all_guest:
+FUNC_LOCAL(restore_all_guest)
ASSERT_INTERRUPTS_DISABLED
/* Stash guest SPEC_CTRL value while we can read struct vcpu. */
@@ -220,8 +218,7 @@ restore_all_guest:
sysretq
1: sysretl
- ALIGN
-.Lrestore_rcx_iret_exit_to_guest:
+LABEL_LOCAL(.Lrestore_rcx_iret_exit_to_guest)
movq 8(%rsp), %rcx # RIP
/* No special register assumptions. */
iret_exit_to_guest:
@@ -230,6 +227,7 @@ iret_exit_to_guest:
addq $8,%rsp
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
+END(restore_all_guest)
/*
* When entering SYSCALL from kernel mode:
@@ -246,7 +244,7 @@ iret_exit_to_guest:
* - Guest %rsp stored in %rax
* - Xen stack loaded, pointing at the %ss slot
*/
-ENTRY(lstar_enter)
+FUNC(lstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -281,9 +279,10 @@ ENTRY(lstar_enter)
mov %rsp, %rdi
call pv_hypercall
jmp test_all_events
+END(lstar_enter)
/* See lstar_enter for entry register state. */
-ENTRY(cstar_enter)
+FUNC(cstar_enter)
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
#endif
@@ -321,8 +320,9 @@ ENTRY(cstar_enter)
jne compat_syscall
#endif
jmp switch_to_kernel
+END(cstar_enter)
-ENTRY(sysenter_entry)
+FUNC(sysenter_entry)
ENDBR64
#ifdef CONFIG_XEN_SHSTK
ALTERNATIVE "", "setssbsy", X86_FEATURE_XEN_SHSTK
@@ -331,7 +331,7 @@ ENTRY(sysenter_entry)
pushq $FLAT_USER_SS
pushq $0
pushfq
-GLOBAL(sysenter_eflags_saved)
+LABEL(sysenter_eflags_saved, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $3 /* ring 3 null cs */
pushq $0 /* null rip */
@@ -385,8 +385,9 @@ UNLIKELY_END(sysenter_gpf)
jne compat_sysenter
#endif
jmp .Lbounce_exception
+END(sysenter_entry)
-ENTRY(int80_direct_trap)
+FUNC(int80_direct_trap)
ENDBR64
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
pushq $0
@@ -474,6 +475,7 @@ int80_slow_path:
*/
GET_STACK_END(14)
jmp handle_exception_saved
+END(int80_direct_trap)
/* create_bounce_frame & helpers don't need to be in .text.entry */
.text
@@ -482,7 +484,7 @@ int80_slow_path:
/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
/* %rdx: trap_bounce, %rbx: struct vcpu */
/* On return only %rbx and %rdx are guaranteed non-clobbered. */
-create_bounce_frame:
+FUNC_LOCAL(create_bounce_frame)
ASSERT_INTERRUPTS_ENABLED
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jnz 1f
@@ -618,6 +620,7 @@ ENTRY(dom_crash_sync_extable)
xorl %edi,%edi
jmp asm_domain_crash_synchronous /* Does not return */
.popsection
+END(create_bounce_frame)
#endif /* CONFIG_PV */
/* --- CODE BELOW THIS LINE (MOSTLY) NOT GUEST RELATED --- */
@@ -626,7 +629,7 @@ ENTRY(dom_crash_sync_extable)
/* No special register assumptions. */
#ifdef CONFIG_PV
-ENTRY(continue_pv_domain)
+FUNC(continue_pv_domain)
ENDBR64
call check_wakeup_from_wait
ret_from_intr:
@@ -641,26 +644,28 @@ ret_from_intr:
#else
jmp test_all_events
#endif
+END(continue_pv_domain)
#else
-ret_from_intr:
+FUNC(ret_from_intr, 0)
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
+END(ret_from_intr)
#endif
.section .init.text, "ax", @progbits
-ENTRY(early_page_fault)
+FUNC(early_page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
SAVE_ALL
movq %rsp, %rdi
call do_early_page_fault
jmp restore_all_xen
+END(early_page_fault)
.section .text.entry, "ax", @progbits
- ALIGN
/* No special register assumptions. */
-restore_all_xen:
+FUNC_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).
@@ -677,8 +682,9 @@ UNLIKELY_END(exit_cr3)
RESTORE_ALL adj=8
iretq
+END(restore_all_xen)
-ENTRY(common_interrupt)
+FUNC(common_interrupt)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -707,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
+END(common_interrupt)
-ENTRY(page_fault)
+FUNC(page_fault)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
+END(page_fault)
/* No special register assumptions. */
-GLOBAL(handle_exception)
+FUNC(handle_exception, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
@@ -882,92 +890,108 @@ FATAL_exception_with_ints_disabled:
movq %rsp,%rdi
call fatal_trap
BUG /* fatal_trap() shouldn't return. */
+END(handle_exception)
-ENTRY(divide_error)
+FUNC(divide_error)
ENDBR64
pushq $0
movl $X86_EXC_DE, 4(%rsp)
jmp handle_exception
+END(divide_error)
-ENTRY(coprocessor_error)
+FUNC(coprocessor_error)
ENDBR64
pushq $0
movl $X86_EXC_MF, 4(%rsp)
jmp handle_exception
+END(coprocessor_error)
-ENTRY(simd_coprocessor_error)
+FUNC(simd_coprocessor_error)
ENDBR64
pushq $0
movl $X86_EXC_XM, 4(%rsp)
jmp handle_exception
+END(coprocessor_error)
-ENTRY(device_not_available)
+FUNC(device_not_available)
ENDBR64
pushq $0
movl $X86_EXC_NM, 4(%rsp)
jmp handle_exception
+END(device_not_available)
-ENTRY(debug)
+FUNC(debug)
ENDBR64
pushq $0
movl $X86_EXC_DB, 4(%rsp)
jmp handle_ist_exception
+END(debug)
-ENTRY(int3)
+FUNC(int3)
ENDBR64
pushq $0
movl $X86_EXC_BP, 4(%rsp)
jmp handle_exception
+END(int3)
-ENTRY(overflow)
+FUNC(overflow)
ENDBR64
pushq $0
movl $X86_EXC_OF, 4(%rsp)
jmp handle_exception
+END(overflow)
-ENTRY(bounds)
+FUNC(bounds)
ENDBR64
pushq $0
movl $X86_EXC_BR, 4(%rsp)
jmp handle_exception
+END(bounds)
-ENTRY(invalid_op)
+FUNC(invalid_op)
ENDBR64
pushq $0
movl $X86_EXC_UD, 4(%rsp)
jmp handle_exception
+END(invalid_op)
-ENTRY(invalid_TSS)
+FUNC(invalid_TSS)
ENDBR64
movl $X86_EXC_TS, 4(%rsp)
jmp handle_exception
+END(invalid_TSS)
-ENTRY(segment_not_present)
+FUNC(segment_not_present)
ENDBR64
movl $X86_EXC_NP, 4(%rsp)
jmp handle_exception
+END(segment_not_present)
-ENTRY(stack_segment)
+FUNC(stack_segment)
ENDBR64
movl $X86_EXC_SS, 4(%rsp)
jmp handle_exception
+END(stack_segment)
-ENTRY(general_protection)
+FUNC(general_protection)
ENDBR64
movl $X86_EXC_GP, 4(%rsp)
jmp handle_exception
+END(general_protection)
-ENTRY(alignment_check)
+FUNC(alignment_check)
ENDBR64
movl $X86_EXC_AC, 4(%rsp)
jmp handle_exception
+END(alignment_check)
-ENTRY(entry_CP)
+FUNC(entry_CP)
ENDBR64
movl $X86_EXC_CP, 4(%rsp)
jmp handle_exception
+END(entry_CP)
-ENTRY(double_fault)
+FUNC(double_fault)
ENDBR64
movl $X86_EXC_DF, 4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
@@ -991,8 +1015,9 @@ ENTRY(double_fault)
movq %rsp,%rdi
call do_double_fault
BUG /* do_double_fault() shouldn't return. */
+END(double_fault)
-ENTRY(nmi)
+FUNC(nmi)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
@@ -1120,21 +1145,24 @@ handle_ist_exception:
ASSERT_CONTEXT_IS_XEN
jmp restore_all_xen
#endif
+END(nmi)
-ENTRY(machine_check)
+FUNC(machine_check)
ENDBR64
pushq $0
movl $X86_EXC_MC, 4(%rsp)
jmp handle_ist_exception
+END(machine_check)
/* No op trap handler. Required for kexec crash path. */
-GLOBAL(trap_nop)
+FUNC(trap_nop, 0)
ENDBR64
iretq
+END(trap_nop)
/* Table of automatically generated entry points. One per vector. */
.pushsection .init.rodata, "a", @progbits
-GLOBAL(autogen_entrypoints)
+DATA(autogen_entrypoints, 8)
/* pop into the .init.rodata section and record an entry point. */
.macro entrypoint ent
.pushsection .init.rodata, "a", @progbits
@@ -1143,7 +1171,7 @@ GLOBAL(autogen_entrypoints)
.endm
.popsection
-autogen_stubs: /* Automatically generated stubs. */
+FUNC_LOCAL(autogen_stubs, 0) /* Automatically generated stubs. */
vec = 0
.rept X86_NR_VECTORS
@@ -1187,6 +1215,7 @@ autogen_stubs: /* Automatically generate
vec = vec + 1
.endr
+END(autogen_stubs)
.section .init.rodata, "a", @progbits
- .size autogen_entrypoints, . - autogen_entrypoints
+END(autogen_entrypoints)
On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: > Recent gas versions generate minimalistic Dwarf debug info for items > annotated as functions and having their sizes specified [1]. "Borrow" > Arm's END() and (remotely) derive other annotation infrastructure from > Linux'es. > > For switch_to_kernel() and restore_all_guest() so far implicit alignment > (from being first in their respective sections) is being made explicit > (as in: using FUNC() without 2nd argument). Whereas for > {,compat}create_bounce_frame() and autogen_entrypoints[] alignment is > newly arranged for. > > Except for the added alignment padding (including their knock-on > effects) no change in generated code/data. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > [1] https://sourceware.org/git?p=binutils-gdb.git;a=commitdiff;h=591cc9fbbfd6d51131c0f1d4a92e7893edcc7a28 > --- > v2: Full rework. > --- > Only two of the assembly files are being converted for now. More could > be done right here or as follow-on in separate patches. > > In principle the framework should be possible to use by other > architectures as well. If we want this, the main questions are going to > be: > - What header file name? (I don't really like Linux'es linkage.h, so I'd > prefer e.g. asm-defns.h or asm_defns.h as we already have in x86.) > - How much per-arch customization do we want to permit up front (i.e. > without knowing how much of it is going to be needed)? Initially I'd > expect only the default function alignment (and padding) to require > per-arch definitions. > > Note that the FB-label in autogen_stubs() cannot be converted just yet: > Such labels cannot be used with .type. We could further diverge from > Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type > labels get by default anyway). > > Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > still have ALIGN. FWIW, as I'm looking into using the newly added macros in order to add annotations suitable for live-patching, I would need to switch some of the LABEL usages into it's own functions, as it's not possible to livepatch a function that has labels jumped into from code paths outside of the function. > --- a/xen/arch/x86/include/asm/asm_defns.h > +++ b/xen/arch/x86/include/asm/asm_defns.h > @@ -81,6 +81,45 @@ register unsigned long current_stack_poi > > #ifdef __ASSEMBLY__ > > +#define SYM_ALIGN(algn...) .balign algn > + > +#define SYM_L_GLOBAL(name) .globl name > +#define SYM_L_WEAK(name) .weak name Won't this better be added when required? I can't spot any weak symbols in assembly ATM, and you don't introduce any _WEAK macro variants below. > +#define SYM_L_LOCAL(name) /* nothing */ > + > +#define SYM_T_FUNC STT_FUNC > +#define SYM_T_DATA STT_OBJECT > +#define SYM_T_NONE STT_NOTYPE > + > +#define SYM(name, typ, linkage, algn...) \ > + .type name, SYM_T_ ## typ; \ > + SYM_L_ ## linkage(name); \ > + SYM_ALIGN(algn); \ > + name: > + > +#define END(name) .size name, . - name > + > +#define ARG1_(x, y...) (x) > +#define ARG2_(x, y...) ARG1_(y) > + > +#define LAST__(nr) ARG ## nr ## _ > +#define LAST_(nr) LAST__(nr) > +#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) I find LAST not very descriptive, won't it better be named OPTIONAL() or similar? (and maybe placed in lib.h?) > + > +#define FUNC(name, algn...) \ > + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) A rant, should the alignment of functions use a different padding? (ie: ret or ud2?) In order to prevent stray jumps falling in the padding and fall trough into the next function. That would also prevent the implicit fall trough used in some places. > +#define LABEL(name, algn...) \ > + SYM(name, NONE, GLOBAL, LAST(16, ## algn), 0x90) > +#define DATA(name, algn...) \ > + SYM(name, DATA, GLOBAL, LAST(0, ## algn), 0xff) > + > +#define FUNC_LOCAL(name, algn...) \ > + SYM(name, FUNC, LOCAL, LAST(16, ## algn), 0x90) > +#define LABEL_LOCAL(name, algn...) \ > + SYM(name, NONE, LOCAL, LAST(16, ## algn), 0x90) Is there much value in adding local labels to the symbol table? AFAICT the main purpose of this macro is to be used to declare aligned labels, and here avoid the ALIGN + label name pair, but could likely drop the .type directive? > +#define DATA_LOCAL(name, algn...) \ > + SYM(name, DATA, LOCAL, LAST(0, ## algn), 0xff) > + > #ifdef HAVE_AS_QUOTED_SYM > #define SUBSECTION_LBL(tag) \ > .ifndef .L.tag; \ > --- a/xen/arch/x86/x86_64/compat/entry.S > +++ b/xen/arch/x86/x86_64/compat/entry.S > @@ -8,10 +8,11 @@ > #include <asm/page.h> > #include <asm/processor.h> > #include <asm/desc.h> > +#include <xen/lib.h> Shouldn't the inclusion of lib.h be in asm_defs.h, as that's where the usage of count_args() resides? (I assume that's why lib.h is added here). > #include <public/xen.h> > #include <irq_vectors.h> > > -ENTRY(entry_int82) > +FUNC(entry_int82) > ENDBR64 > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > pushq $0 > @@ -27,9 +28,10 @@ ENTRY(entry_int82) > > mov %rsp, %rdi > call do_entry_int82 > +END(entry_int82) > > /* %rbx: struct vcpu */ > -ENTRY(compat_test_all_events) > +FUNC(compat_test_all_events) > ASSERT_NOT_IN_ATOMIC > cli # tests must not race interrupts > /*compat_test_softirqs:*/ > @@ -66,24 +68,21 @@ compat_test_guest_events: > call compat_create_bounce_frame > jmp compat_test_all_events > > - ALIGN > /* %rbx: struct vcpu */ > -compat_process_softirqs: > +LABEL_LOCAL(compat_process_softirqs) Shouldn't this be a local function rather than a local label? It's fully isolated. I guess it would create issues with compat_process_trap, as we would then require a jump from the preceding compat_process_nmi. > sti > call do_softirq > jmp compat_test_all_events > > - ALIGN > /* %rbx: struct vcpu, %rdx: struct trap_bounce */ > -.Lcompat_process_trapbounce: > +LABEL_LOCAL(.Lcompat_process_trapbounce) It's my understanding that here the '.L' prefix is pointless, since LABEL_LOCAL() will forcefully create a symbol for the label due to the usage of .type? Thanks, Roger.
On 29.05.2023 15:34, Roger Pau Monné wrote: > On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: >> Note that the FB-label in autogen_stubs() cannot be converted just yet: >> Such labels cannot be used with .type. We could further diverge from >> Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type >> labels get by default anyway). >> >> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we >> still have ALIGN. > > FWIW, as I'm looking into using the newly added macros in order to add > annotations suitable for live-patching, I would need to switch some of > the LABEL usages into it's own functions, as it's not possible to > livepatch a function that has labels jumped into from code paths > outside of the function. Hmm, I'm not sure what the best way is to overcome that restriction. I'm not convinced we want to arbitrarily name things "functions". >> --- a/xen/arch/x86/include/asm/asm_defns.h >> +++ b/xen/arch/x86/include/asm/asm_defns.h >> @@ -81,6 +81,45 @@ register unsigned long current_stack_poi >> >> #ifdef __ASSEMBLY__ >> >> +#define SYM_ALIGN(algn...) .balign algn >> + >> +#define SYM_L_GLOBAL(name) .globl name >> +#define SYM_L_WEAK(name) .weak name > > Won't this better be added when required? I can't spot any weak > symbols in assembly ATM, and you don't introduce any _WEAK macro > variants below. Well, Andrew specifically mentioned to desire to also have Linux'es support for weak symbols. Hence I decided to add it here despite (for now) being unused). I can certainly drop that again, but in particular if we wanted to use the scheme globally, I think we may want to make it "complete". >> +#define SYM_L_LOCAL(name) /* nothing */ >> + >> +#define SYM_T_FUNC STT_FUNC >> +#define SYM_T_DATA STT_OBJECT >> +#define SYM_T_NONE STT_NOTYPE >> + >> +#define SYM(name, typ, linkage, algn...) \ >> + .type name, SYM_T_ ## typ; \ >> + SYM_L_ ## linkage(name); \ >> + SYM_ALIGN(algn); \ >> + name: >> + >> +#define END(name) .size name, . - name >> + >> +#define ARG1_(x, y...) (x) >> +#define ARG2_(x, y...) ARG1_(y) >> + >> +#define LAST__(nr) ARG ## nr ## _ >> +#define LAST_(nr) LAST__(nr) >> +#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) > > I find LAST not very descriptive, won't it better be named OPTIONAL() > or similar? (and maybe placed in lib.h?) I don't think OPTIONAL describes the purpose. I truly mean "last" here. As to placing in lib.h - perhaps, but then we may want to have forms with more than 2 arguments right away (and it would be a little unclear how far up to go). >> + >> +#define FUNC(name, algn...) \ >> + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) > > A rant, should the alignment of functions use a different padding? > (ie: ret or ud2?) In order to prevent stray jumps falling in the > padding and fall trough into the next function. That would also > prevent the implicit fall trough used in some places. Yes, but that's a separate topic (for which iirc patches are pending as well, just of course not integrated with the work here. There's the slight risk of overlooking some "fall-through" case ... >> +#define LABEL(name, algn...) \ >> + SYM(name, NONE, GLOBAL, LAST(16, ## algn), 0x90) >> +#define DATA(name, algn...) \ >> + SYM(name, DATA, GLOBAL, LAST(0, ## algn), 0xff) >> + >> +#define FUNC_LOCAL(name, algn...) \ >> + SYM(name, FUNC, LOCAL, LAST(16, ## algn), 0x90) >> +#define LABEL_LOCAL(name, algn...) \ >> + SYM(name, NONE, LOCAL, LAST(16, ## algn), 0x90) > > Is there much value in adding local labels to the symbol table? > > AFAICT the main purpose of this macro is to be used to declare aligned > labels, and here avoid the ALIGN + label name pair, but could likely > drop the .type directive? Right, .type ... NOTYPE is kind of redundant, but it fits the model better here. >> --- a/xen/arch/x86/x86_64/compat/entry.S >> +++ b/xen/arch/x86/x86_64/compat/entry.S >> @@ -8,10 +8,11 @@ >> #include <asm/page.h> >> #include <asm/processor.h> >> #include <asm/desc.h> >> +#include <xen/lib.h> > > Shouldn't the inclusion of lib.h be in asm_defs.h, as that's where the > usage of count_args() resides? (I assume that's why lib.h is added > here). When the uses are in macros I'm always largely undecided, and I slightly tend towards the (in general, perhaps not overly relevant here) "less dependencies" solution. As in: Source files not using the macros which use count_args() also don't need libs.h then. >> @@ -66,24 +68,21 @@ compat_test_guest_events: >> call compat_create_bounce_frame >> jmp compat_test_all_events >> >> - ALIGN >> /* %rbx: struct vcpu */ >> -compat_process_softirqs: >> +LABEL_LOCAL(compat_process_softirqs) > > Shouldn't this be a local function rather than a local label? It's > fully isolated. I guess it would create issues with > compat_process_trap, as we would then require a jump from the > preceding compat_process_nmi. Alternatives are possible, but right now I consider this an inner label of compat_test_all_events. >> sti >> call do_softirq >> jmp compat_test_all_events >> >> - ALIGN >> /* %rbx: struct vcpu, %rdx: struct trap_bounce */ >> -.Lcompat_process_trapbounce: >> +LABEL_LOCAL(.Lcompat_process_trapbounce) > > It's my understanding that here the '.L' prefix is pointless, since > LABEL_LOCAL() will forcefully create a symbol for the label due to the > usage of .type? I don't think .type has this effect. There's certainly no such label in the symbol table of the object file I have as a result. Jan
On Tue, May 30, 2023 at 10:06:27AM +0200, Jan Beulich wrote: > On 29.05.2023 15:34, Roger Pau Monné wrote: > > On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: > >> Note that the FB-label in autogen_stubs() cannot be converted just yet: > >> Such labels cannot be used with .type. We could further diverge from > >> Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type > >> labels get by default anyway). > >> > >> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > >> still have ALIGN. > > > > FWIW, as I'm looking into using the newly added macros in order to add > > annotations suitable for live-patching, I would need to switch some of > > the LABEL usages into it's own functions, as it's not possible to > > livepatch a function that has labels jumped into from code paths > > outside of the function. > > Hmm, I'm not sure what the best way is to overcome that restriction. I'm > not convinced we want to arbitrarily name things "functions". Any external entry point in the middle of a function-like block will prevent it from being live patched. If you want I can try to do a pass on top of your patch and see how that would end up looking. I'm attempting to think about other solutions, but every other solution seems quite horrible. > >> --- a/xen/arch/x86/include/asm/asm_defns.h > >> +++ b/xen/arch/x86/include/asm/asm_defns.h > >> @@ -81,6 +81,45 @@ register unsigned long current_stack_poi > >> > >> #ifdef __ASSEMBLY__ > >> > >> +#define SYM_ALIGN(algn...) .balign algn > >> + > >> +#define SYM_L_GLOBAL(name) .globl name > >> +#define SYM_L_WEAK(name) .weak name > > > > Won't this better be added when required? I can't spot any weak > > symbols in assembly ATM, and you don't introduce any _WEAK macro > > variants below. > > Well, Andrew specifically mentioned to desire to also have Linux'es > support for weak symbols. Hence I decided to add it here despite > (for now) being unused). I can certainly drop that again, but in > particular if we wanted to use the scheme globally, I think we may > want to make it "complete". OK, as long as we know it's unused. > >> +#define SYM_L_LOCAL(name) /* nothing */ > >> + > >> +#define SYM_T_FUNC STT_FUNC > >> +#define SYM_T_DATA STT_OBJECT > >> +#define SYM_T_NONE STT_NOTYPE > >> + > >> +#define SYM(name, typ, linkage, algn...) \ > >> + .type name, SYM_T_ ## typ; \ > >> + SYM_L_ ## linkage(name); \ > >> + SYM_ALIGN(algn); \ > >> + name: > >> + > >> +#define END(name) .size name, . - name > >> + > >> +#define ARG1_(x, y...) (x) > >> +#define ARG2_(x, y...) ARG1_(y) > >> + > >> +#define LAST__(nr) ARG ## nr ## _ > >> +#define LAST_(nr) LAST__(nr) > >> +#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) > > > > I find LAST not very descriptive, won't it better be named OPTIONAL() > > or similar? (and maybe placed in lib.h?) > > I don't think OPTIONAL describes the purpose. I truly mean "last" here. > As to placing in lib.h - perhaps, but then we may want to have forms > with more than 2 arguments right away (and it would be a little unclear > how far up to go). Hm, I would be fine with adding that version with just 2 arguments, as it's better to have the helper in a generic place IMO. > >> + > >> +#define FUNC(name, algn...) \ > >> + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) > > > > A rant, should the alignment of functions use a different padding? > > (ie: ret or ud2?) In order to prevent stray jumps falling in the > > padding and fall trough into the next function. That would also > > prevent the implicit fall trough used in some places. > > Yes, but that's a separate topic (for which iirc patches are pending > as well, just of course not integrated with the work here. There's > the slight risk of overlooking some "fall-through" case ... Oh, OK, wasn't aware patches are floating for this already, just came across it while reviewing. > >> --- a/xen/arch/x86/x86_64/compat/entry.S > >> +++ b/xen/arch/x86/x86_64/compat/entry.S > >> @@ -8,10 +8,11 @@ > >> #include <asm/page.h> > >> #include <asm/processor.h> > >> #include <asm/desc.h> > >> +#include <xen/lib.h> > > > > Shouldn't the inclusion of lib.h be in asm_defs.h, as that's where the > > usage of count_args() resides? (I assume that's why lib.h is added > > here). > > When the uses are in macros I'm always largely undecided, and I slightly > tend towards the (in general, perhaps not overly relevant here) "less > dependencies" solution. As in: Source files not using the macros which > use count_args() also don't need libs.h then. I tend to prefer headers to be self contained, as it overall leads to a clearer set of includes in source files. It's not obvious why entry.S needs lib.h unless the asm_macros.h usage is taken into account. > >> sti > >> call do_softirq > >> jmp compat_test_all_events > >> > >> - ALIGN > >> /* %rbx: struct vcpu, %rdx: struct trap_bounce */ > >> -.Lcompat_process_trapbounce: > >> +LABEL_LOCAL(.Lcompat_process_trapbounce) > > > > It's my understanding that here the '.L' prefix is pointless, since > > LABEL_LOCAL() will forcefully create a symbol for the label due to the > > usage of .type? > > I don't think .type has this effect. There's certainly no such label in > the symbol table of the object file I have as a result. I was expecting .type to force the creation of a symbol, so the '.L' prefix does prevent the symbol from being created even if .type is specified. Shouldn't the assembler complain that we are attempting to set a type for a not present symbol? Thanks, Roger.
On 30.05.2023 15:21, Roger Pau Monné wrote: > On Tue, May 30, 2023 at 10:06:27AM +0200, Jan Beulich wrote: >> On 29.05.2023 15:34, Roger Pau Monné wrote: >>> On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: >>>> Note that the FB-label in autogen_stubs() cannot be converted just yet: >>>> Such labels cannot be used with .type. We could further diverge from >>>> Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type >>>> labels get by default anyway). >>>> >>>> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we >>>> still have ALIGN. >>> >>> FWIW, as I'm looking into using the newly added macros in order to add >>> annotations suitable for live-patching, I would need to switch some of >>> the LABEL usages into it's own functions, as it's not possible to >>> livepatch a function that has labels jumped into from code paths >>> outside of the function. >> >> Hmm, I'm not sure what the best way is to overcome that restriction. I'm >> not convinced we want to arbitrarily name things "functions". > > Any external entry point in the middle of a function-like block will > prevent it from being live patched. Is there actually any particular reason for this restriction? As long as old and new code has the same external entry points, redirecting all old ones to their new counterparts would seem feasible. > If you want I can try to do a pass on top of your patch and see how > that would end up looking. I'm attempting to think about other > solutions, but every other solution seems quite horrible. Right, but splitting functions into piecemeal fragments isn't going to be very nice either. >>>> --- a/xen/arch/x86/include/asm/asm_defns.h >>>> +++ b/xen/arch/x86/include/asm/asm_defns.h >>>> @@ -81,6 +81,45 @@ register unsigned long current_stack_poi >>>> >>>> #ifdef __ASSEMBLY__ >>>> >>>> +#define SYM_ALIGN(algn...) .balign algn >>>> + >>>> +#define SYM_L_GLOBAL(name) .globl name >>>> +#define SYM_L_WEAK(name) .weak name >>> >>> Won't this better be added when required? I can't spot any weak >>> symbols in assembly ATM, and you don't introduce any _WEAK macro >>> variants below. >> >> Well, Andrew specifically mentioned to desire to also have Linux'es >> support for weak symbols. Hence I decided to add it here despite >> (for now) being unused). I can certainly drop that again, but in >> particular if we wanted to use the scheme globally, I think we may >> want to make it "complete". > > OK, as long as we know it's unused. I've added a sentence to this effect to the description. >>>> +#define SYM_L_LOCAL(name) /* nothing */ >>>> + >>>> +#define SYM_T_FUNC STT_FUNC >>>> +#define SYM_T_DATA STT_OBJECT >>>> +#define SYM_T_NONE STT_NOTYPE >>>> + >>>> +#define SYM(name, typ, linkage, algn...) \ >>>> + .type name, SYM_T_ ## typ; \ >>>> + SYM_L_ ## linkage(name); \ >>>> + SYM_ALIGN(algn); \ >>>> + name: >>>> + >>>> +#define END(name) .size name, . - name >>>> + >>>> +#define ARG1_(x, y...) (x) >>>> +#define ARG2_(x, y...) ARG1_(y) >>>> + >>>> +#define LAST__(nr) ARG ## nr ## _ >>>> +#define LAST_(nr) LAST__(nr) >>>> +#define LAST(x, y...) LAST_(count_args(x, ## y))(x, ## y) >>> >>> I find LAST not very descriptive, won't it better be named OPTIONAL() >>> or similar? (and maybe placed in lib.h?) >> >> I don't think OPTIONAL describes the purpose. I truly mean "last" here. >> As to placing in lib.h - perhaps, but then we may want to have forms >> with more than 2 arguments right away (and it would be a little unclear >> how far up to go). > > Hm, I would be fine with adding that version with just 2 arguments, as > it's better to have the helper in a generic place IMO. I'll think about this some more. >>>> + >>>> +#define FUNC(name, algn...) \ >>>> + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) >>> >>> A rant, should the alignment of functions use a different padding? >>> (ie: ret or ud2?) In order to prevent stray jumps falling in the >>> padding and fall trough into the next function. That would also >>> prevent the implicit fall trough used in some places. >> >> Yes, but that's a separate topic (for which iirc patches are pending >> as well, just of course not integrated with the work here. There's >> the slight risk of overlooking some "fall-through" case ... > > Oh, OK, wasn't aware patches are floating for this already, just came > across it while reviewing. Well, those don't cover padding yet, but they deal with straight-line speculation past RET or JMP. >>>> sti >>>> call do_softirq >>>> jmp compat_test_all_events >>>> >>>> - ALIGN >>>> /* %rbx: struct vcpu, %rdx: struct trap_bounce */ >>>> -.Lcompat_process_trapbounce: >>>> +LABEL_LOCAL(.Lcompat_process_trapbounce) >>> >>> It's my understanding that here the '.L' prefix is pointless, since >>> LABEL_LOCAL() will forcefully create a symbol for the label due to the >>> usage of .type? >> >> I don't think .type has this effect. There's certainly no such label in >> the symbol table of the object file I have as a result. > > I was expecting .type to force the creation of a symbol, so the '.L' > prefix does prevent the symbol from being created even if .type is > specified. > > Shouldn't the assembler complain that we are attempting to set a type > for a not present symbol? But .L symbols are still normal symbols to gas, just that it knows to not emit them to the symbol table (unless there's a need, e.g. through a use in a relocation that cannot be expressed as section-relative one). It could flag the pointless use, but then it may get this wrong if in the end the symbol does need emitting. Jan
On Tue, May 30, 2023 at 04:23:21PM +0200, Jan Beulich wrote: > On 30.05.2023 15:21, Roger Pau Monné wrote: > > On Tue, May 30, 2023 at 10:06:27AM +0200, Jan Beulich wrote: > >> On 29.05.2023 15:34, Roger Pau Monné wrote: > >>> On Tue, May 23, 2023 at 01:30:51PM +0200, Jan Beulich wrote: > >>>> Note that the FB-label in autogen_stubs() cannot be converted just yet: > >>>> Such labels cannot be used with .type. We could further diverge from > >>>> Linux'es model and avoid setting STT_NOTYPE explicitly (that's the type > >>>> labels get by default anyway). > >>>> > >>>> Note that we can't use ALIGN() (in place of SYM_ALIGN()) as long as we > >>>> still have ALIGN. > >>> > >>> FWIW, as I'm looking into using the newly added macros in order to add > >>> annotations suitable for live-patching, I would need to switch some of > >>> the LABEL usages into it's own functions, as it's not possible to > >>> livepatch a function that has labels jumped into from code paths > >>> outside of the function. > >> > >> Hmm, I'm not sure what the best way is to overcome that restriction. I'm > >> not convinced we want to arbitrarily name things "functions". > > > > Any external entry point in the middle of a function-like block will > > prevent it from being live patched. > > Is there actually any particular reason for this restriction? As long > as old and new code has the same external entry points, redirecting > all old ones to their new counterparts would seem feasible. Yes, that was another option, we could force asm patching to always be done with a jump (instead of in-place) and then add jumps at the old entry point addresses in order to redirect to the new addresses. Or assert that the addresses of any symbols inside the function is not changed in order to do in-place replacement of code. > > If you want I can try to do a pass on top of your patch and see how > > that would end up looking. I'm attempting to think about other > > solutions, but every other solution seems quite horrible. > > Right, but splitting functions into piecemeal fragments isn't going > to be very nice either. I'm not sure how much splitting would be required TBH. > >>>> + > >>>> +#define FUNC(name, algn...) \ > >>>> + SYM(name, FUNC, GLOBAL, LAST(16, ## algn), 0x90) > >>> > >>> A rant, should the alignment of functions use a different padding? > >>> (ie: ret or ud2?) In order to prevent stray jumps falling in the > >>> padding and fall trough into the next function. That would also > >>> prevent the implicit fall trough used in some places. > >> > >> Yes, but that's a separate topic (for which iirc patches are pending > >> as well, just of course not integrated with the work here. There's > >> the slight risk of overlooking some "fall-through" case ... > > > > Oh, OK, wasn't aware patches are floating for this already, just came > > across it while reviewing. > > Well, those don't cover padding yet, but they deal with straight-line > speculation past RET or JMP. Introducing the helpers does make it easy to convert the padding for all the existing users at least. > >>>> sti > >>>> call do_softirq > >>>> jmp compat_test_all_events > >>>> > >>>> - ALIGN > >>>> /* %rbx: struct vcpu, %rdx: struct trap_bounce */ > >>>> -.Lcompat_process_trapbounce: > >>>> +LABEL_LOCAL(.Lcompat_process_trapbounce) > >>> > >>> It's my understanding that here the '.L' prefix is pointless, since > >>> LABEL_LOCAL() will forcefully create a symbol for the label due to the > >>> usage of .type? > >> > >> I don't think .type has this effect. There's certainly no such label in > >> the symbol table of the object file I have as a result. > > > > I was expecting .type to force the creation of a symbol, so the '.L' > > prefix does prevent the symbol from being created even if .type is > > specified. > > > > Shouldn't the assembler complain that we are attempting to set a type > > for a not present symbol? > > But .L symbols are still normal symbols to gas, just that it knows to not > emit them to the symbol table (unless there's a need, e.g. through a use > in a relocation that cannot be expressed as section-relative one). It > could flag the pointless use, but then it may get this wrong if in the > end the symbol does need emitting. Thanks for the explanation. Roger.
Let's have assembler symbols be consistent with C ones. In principle
there are (a few) cases where gas can produce smaller code this way,
just that for now there's a gas bug causing smaller code to be emitted
even when that shouldn't be the case.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: New.
--- a/xen/arch/x86/include/asm/asm_defns.h
+++ b/xen/arch/x86/include/asm/asm_defns.h
@@ -83,7 +83,7 @@ register unsigned long current_stack_poi
#define SYM_ALIGN(algn...) .balign algn
-#define SYM_L_GLOBAL(name) .globl name
+#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -45,11 +45,11 @@
#ifdef __ASSEMBLY__
#define ALIGN .align 16,0x90
#define ENTRY(name) \
- .globl name; \
ALIGN; \
- name:
+ GLOBAL(name)
#define GLOBAL(name) \
.globl name; \
+ .hidden name; \
name:
#endif
On Tue, May 23, 2023 at 01:31:16PM +0200, Jan Beulich wrote: > Let's have assembler symbols be consistent with C ones. In principle > there are (a few) cases where gas can produce smaller code this way, > just that for now there's a gas bug causing smaller code to be emitted > even when that shouldn't be the case. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
1: common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions 2: SVM: convert entry point annotations 3: VMX: convert entry point annotations 4: x86/ACPI: annotate assembly functions with type and size 5: x86/kexec: convert entry point annotations 6: x86: convert misc assembly function annotations 7: x86: move ENTRY(), GLOBAL(), and ALIGN Jan
On 07.02.2024 14:34, Jan Beulich wrote: > 1: common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions > 2: SVM: convert entry point annotations > 3: VMX: convert entry point annotations > 4: x86/ACPI: annotate assembly functions with type and size > 5: x86/kexec: convert entry point annotations > 6: x86: convert misc assembly function annotations > 7: x86: move ENTRY(), GLOBAL(), and ALIGN Being the cover letter, the title was of course supposed to say 0/7. Jan
Leverage the new infrastructure in xen/linkage.h to also switch to per-
function sections (when configured), deriving the specific name from the
"base" section in use at the time FUNC() is invoked.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Since we use .subsection in UNLIKELY_START(), a perhaps not really
wanted side effect of this change is that respective out-of-line
code now moves much closer to its original (invoking) code.
TBD: Of course something with the same overall effect, but less
impactful might do in Config.mk. E.g. $(filter-out -D%,$(3))
instead of $(firstword (3)). In fact Roger wants the detection to
be in Kconfig, for LIVEPATCH to depend on it. Yet the whole
underlying discussion there imo would first need settling (and
therefore reviving).
TBD: On top of Roger's respective patch (for livepatch), also respect
CONFIG_FUNCTION_ALIGNMENT.
Note that we'd need to split DATA() in order to separate r/w and r/o
contributions. Further splitting might be needed to also support more
advanced attributes (e.g. merge), hence why this isn't done right here.
Sadly while a new section's name can be derived from the presently in
use, its attributes cannot be. Perhaps the only thing we can do is give
DATA() a 2nd mandatory parameter. Then again I guess most data
definitions could be moved to C anyway.
---
v6: Deal with x86'es entry_PF() and entry_int82() falling through to the
next "function". Re-base.
v5: Re-base over changes earlier in the series.
v4: Re-base.
v2: Make detection properly fail on old gas (by adjusting
cc-option-add-closure).
--- a/Config.mk
+++ b/Config.mk
@@ -102,7 +102,7 @@ cc-option = $(shell if $(1) $(2:-Wno-%=-
# Usage: $(call cc-option-add CFLAGS,CC,-march=winchip-c6)
cc-option-add = $(eval $(call cc-option-add-closure,$(1),$(2),$(3)))
define cc-option-add-closure
- ifneq ($$(call cc-option,$$($(2)),$(3),n),n)
+ ifneq ($$(call cc-option,$$($(2)),$(firstword $(3)),n),n)
$(1) += $(3)
endif
endef
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -409,6 +409,9 @@ AFLAGS += -D__ASSEMBLY__
$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--noexecstack)
+# Check to see whether the assembler supports the --sectname-subst option.
+$(call cc-option-add,AFLAGS,CC,-Wa$$(comma)--sectname-subst -DHAVE_AS_SECTNAME_SUBST)
+
LDFLAGS-$(call ld-option,--warn-rwx-segments) += --no-warn-rwx-segments
CFLAGS += $(CFLAGS-y)
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -157,6 +157,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
*(.altinstr_replacement)
--- a/xen/arch/ppc/xen.lds.S
+++ b/xen/arch/ppc/xen.lds.S
@@ -104,6 +104,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/riscv/xen.lds.S
+++ b/xen/arch/riscv/xen.lds.S
@@ -104,6 +104,9 @@ SECTIONS
.init.text : {
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
_einittext = .;
. = ALIGN(PAGE_SIZE); /* Avoid mapping alt insns executable */
} :text
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -29,6 +29,9 @@ FUNC(entry_int82)
mov %rsp, %rdi
call do_entry_int82
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ jmp compat_test_all_events
+#endif
END(entry_int82)
/* %rbx: struct vcpu */
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -723,6 +723,9 @@ END(common_interrupt)
FUNC(entry_PF)
ENDBR64
movl $X86_EXC_PF, 4(%rsp)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ jmp handle_exception
+#endif
END(entry_PF)
/* No special register assumptions. */
FUNC(handle_exception, 0)
@@ -1023,8 +1026,11 @@ FUNC(entry_NMI)
ENDBR64
pushq $0
movl $X86_EXC_NMI, 4(%rsp)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ jmp handle_ist_exception
+#endif
END(entry_NMI)
-
+/* No special register assumptions. */
FUNC(handle_ist_exception)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
SAVE_ALL
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -86,6 +86,9 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
_stextentry = .;
*(.text.entry)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.text.entry.*)
+#endif
. = ALIGN(PAGE_SIZE);
_etextentry = .;
@@ -214,6 +217,9 @@ SECTIONS
#endif
_sinittext = .;
*(.init.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+ *(.init.text.*)
+#endif
*(.text.startup)
_einittext = .;
/*
--- a/xen/include/xen/linkage.h
+++ b/xen/include/xen/linkage.h
@@ -19,6 +19,14 @@
#define SYM_ALIGN(align...) .balign align
+#if defined(HAVE_AS_SECTNAME_SUBST) && defined(CONFIG_CC_SPLIT_SECTIONS)
+# define SYM_PUSH_SECTION(name, attr) \
+ .pushsection %S.name, attr, %progbits; \
+ .equ .Lsplit_section, 1
+#else
+# define SYM_PUSH_SECTION(name, attr)
+#endif
+
#define SYM_L_GLOBAL(name) .globl name; .hidden name
#define SYM_L_WEAK(name) .weak name
#define SYM_L_LOCAL(name) /* nothing */
@@ -33,7 +41,14 @@
SYM_ALIGN(align); \
name:
-#define END(name) .size name, . - name
+#define END(name) \
+ .size name, . - name; \
+ .ifdef .Lsplit_section; \
+ .if .Lsplit_section; \
+ .popsection; \
+ .equ .Lsplit_section, 0; \
+ .endif; \
+ .endif
/*
* CODE_FILL in particular may need to expand to nothing (e.g. for RISC-V), in
@@ -47,6 +62,7 @@
#endif
#define FUNC(name, align...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, GLOBAL, DO_CODE_ALIGN(align))
#define LABEL(name, align...) \
SYM(name, NONE, GLOBAL, DO_CODE_ALIGN(align))
@@ -54,6 +70,7 @@
SYM(name, DATA, GLOBAL, LASTARG(DATA_ALIGN, ## align), DATA_FILL)
#define FUNC_LOCAL(name, align...) \
+ SYM_PUSH_SECTION(name, "ax"); \
SYM(name, FUNC, LOCAL, DO_CODE_ALIGN(align))
#define LABEL_LOCAL(name, align...) \
SYM(name, NONE, LOCAL, DO_CODE_ALIGN(align))
Use the generic framework from xen/linkage.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v6: New.
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -24,7 +24,7 @@
#include <asm/asm_defns.h>
#include <asm/page.h>
-ENTRY(svm_asm_do_resume)
+FUNC(svm_asm_do_resume)
GET_CURRENT(bx)
.Lsvm_do_resume:
call svm_intr_assist
@@ -132,7 +132,7 @@ __UNLIKELY_END(nsvm_hap)
* to safely resolve any Spectre-v1 concerns in the above logic.
*/
stgi
-GLOBAL(svm_stgi_label)
+LABEL(svm_stgi_label, 0)
call svm_vmexit_handler
jmp .Lsvm_do_resume
@@ -140,6 +140,4 @@ GLOBAL(svm_stgi_label)
sti
call do_softirq
jmp .Lsvm_do_resume
-
- .type svm_asm_do_resume, @function
- .size svm_asm_do_resume, . - svm_asm_do_resume
+END(svm_asm_do_resume)
On 07/02/2024 1:37 pm, Jan Beulich wrote: > Use the generic framework from xen/linkage.h. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Use the generic framework from xen/linkage.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v6: New.
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -24,7 +24,7 @@
#define VMRESUME .byte 0x0f,0x01,0xc3
#define VMLAUNCH .byte 0x0f,0x01,0xc2
-ENTRY(vmx_asm_vmexit_handler)
+FUNC(vmx_asm_vmexit_handler)
SAVE_ALL
mov %cr2,%rax
@@ -132,7 +132,7 @@ UNLIKELY_END(realmode)
call vmx_vmentry_failure
jmp .Lvmx_process_softirqs
-ENTRY(vmx_asm_do_vmentry)
+LABEL(vmx_asm_do_vmentry)
GET_CURRENT(bx)
jmp .Lvmx_do_vmentry
@@ -150,6 +150,4 @@ ENTRY(vmx_asm_do_vmentry)
sti
call do_softirq
jmp .Lvmx_do_vmentry
-
- .type vmx_asm_vmexit_handler, @function
- .size vmx_asm_vmexit_handler, . - vmx_asm_vmexit_handler
+END(vmx_asm_vmexit_handler)
On 07/02/2024 1:37 pm, Jan Beulich wrote: > Use the generic framework from xen/linkage.h. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v6: New. > > --- a/xen/arch/x86/hvm/vmx/entry.S > +++ b/xen/arch/x86/hvm/vmx/entry.S > @@ -24,7 +24,7 @@ > #define VMRESUME .byte 0x0f,0x01,0xc3 > #define VMLAUNCH .byte 0x0f,0x01,0xc2 > > -ENTRY(vmx_asm_vmexit_handler) > +FUNC(vmx_asm_vmexit_handler) > SAVE_ALL > > mov %cr2,%rax > @@ -132,7 +132,7 @@ UNLIKELY_END(realmode) > call vmx_vmentry_failure > jmp .Lvmx_process_softirqs > > -ENTRY(vmx_asm_do_vmentry) > +LABEL(vmx_asm_do_vmentry) This really is a function, not a label. xen.git/xen$ git grep vmx_asm_do_vmentry arch/x86/hvm/vmx/entry.S:135:ENTRY(vmx_asm_do_vmentry) arch/x86/hvm/vmx/vmcs.c:1855:void noreturn vmx_asm_do_vmentry(void); arch/x86/hvm/vmx/vmcs.c:1929: reset_stack_and_jump(vmx_asm_do_vmentry); It is giant mess, of two functions forming part of the same loop. Considering that you declines to take CODE, I don't know what to suggest. The point of CODE, distinct to FUNC, was to identify the places where weird things were going on, and this absolutely counts. ~Andrew
On 07.02.2024 14:55, Andrew Cooper wrote: > On 07/02/2024 1:37 pm, Jan Beulich wrote: >> Use the generic framework from xen/linkage.h. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v6: New. >> >> --- a/xen/arch/x86/hvm/vmx/entry.S >> +++ b/xen/arch/x86/hvm/vmx/entry.S >> @@ -24,7 +24,7 @@ >> #define VMRESUME .byte 0x0f,0x01,0xc3 >> #define VMLAUNCH .byte 0x0f,0x01,0xc2 >> >> -ENTRY(vmx_asm_vmexit_handler) >> +FUNC(vmx_asm_vmexit_handler) >> SAVE_ALL >> >> mov %cr2,%rax >> @@ -132,7 +132,7 @@ UNLIKELY_END(realmode) >> call vmx_vmentry_failure >> jmp .Lvmx_process_softirqs >> >> -ENTRY(vmx_asm_do_vmentry) >> +LABEL(vmx_asm_do_vmentry) > > This really is a function, not a label. > > xen.git/xen$ git grep vmx_asm_do_vmentry > arch/x86/hvm/vmx/entry.S:135:ENTRY(vmx_asm_do_vmentry) > arch/x86/hvm/vmx/vmcs.c:1855:void noreturn vmx_asm_do_vmentry(void); > arch/x86/hvm/vmx/vmcs.c:1929: reset_stack_and_jump(vmx_asm_do_vmentry); > > It is giant mess, of two functions forming part of the same loop. > > Considering that you declines to take CODE, I don't know what to > suggest. The point of CODE, distinct to FUNC, was to identify the > places where weird things were going on, and this absolutely counts. What's not clear to me: How would CODE() differ from both FUNC() and LABEL()? And if the symbol is to be a function, what's wrong with using FUNC() here as is? There's in particular no real need to have all FUNC()s paired with END()s, afaict. Jan
On 07.02.2024 15:25, Jan Beulich wrote: > On 07.02.2024 14:55, Andrew Cooper wrote: >> On 07/02/2024 1:37 pm, Jan Beulich wrote: >>> Use the generic framework from xen/linkage.h. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> --- >>> v6: New. >>> >>> --- a/xen/arch/x86/hvm/vmx/entry.S >>> +++ b/xen/arch/x86/hvm/vmx/entry.S >>> @@ -24,7 +24,7 @@ >>> #define VMRESUME .byte 0x0f,0x01,0xc3 >>> #define VMLAUNCH .byte 0x0f,0x01,0xc2 >>> >>> -ENTRY(vmx_asm_vmexit_handler) >>> +FUNC(vmx_asm_vmexit_handler) >>> SAVE_ALL >>> >>> mov %cr2,%rax >>> @@ -132,7 +132,7 @@ UNLIKELY_END(realmode) >>> call vmx_vmentry_failure >>> jmp .Lvmx_process_softirqs >>> >>> -ENTRY(vmx_asm_do_vmentry) >>> +LABEL(vmx_asm_do_vmentry) >> >> This really is a function, not a label. >> >> xen.git/xen$ git grep vmx_asm_do_vmentry >> arch/x86/hvm/vmx/entry.S:135:ENTRY(vmx_asm_do_vmentry) >> arch/x86/hvm/vmx/vmcs.c:1855:void noreturn vmx_asm_do_vmentry(void); >> arch/x86/hvm/vmx/vmcs.c:1929: reset_stack_and_jump(vmx_asm_do_vmentry); >> >> It is giant mess, of two functions forming part of the same loop. >> >> Considering that you declines to take CODE, I don't know what to >> suggest. The point of CODE, distinct to FUNC, was to identify the >> places where weird things were going on, and this absolutely counts. > > What's not clear to me: How would CODE() differ from both FUNC() and > LABEL()? And if the symbol is to be a function, what's wrong with > using FUNC() here as is? Well, I figured this one: FUNC() may switch sections following patch 1, so indeed we'd need something that is much like FUNC(), but without the (optional) section switch. Jan
Use the generic framework from xen/linkage.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The .Lsuspend_err label is used in a cross-function manner here, but
it's not clear to me what - if anything - to do about this.
---
v6: New.
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -7,7 +7,7 @@
.text
.code64
-ENTRY(do_suspend_lowlevel)
+FUNC(do_suspend_lowlevel)
push %rbp
push %rbx
push %r12
@@ -32,6 +32,7 @@ ENTRY(do_suspend_lowlevel)
/* It seems we didn't suspend. Get out of here. */
jmp .Lsuspend_err
+END(do_suspend_lowlevel)
/*
* do_suspend_lowlevel() is arranged to behave as a regular function
@@ -43,7 +44,7 @@ ENTRY(do_suspend_lowlevel)
*
* Everything else, including the stack, needs restoring.
*/
-ENTRY(s3_resume)
+FUNC(s3_resume)
lgdt boot_gdtr(%rip)
mov saved_cr0(%rip), %rax
@@ -132,6 +133,7 @@ ENTRY(s3_resume)
pop %rbx
pop %rbp
ret
+END(s3_resume)
.data
.align 16
@@ -142,5 +144,4 @@ saved_cr0: .quad 0
saved_ssp: .quad 0
#endif
-GLOBAL(saved_magic)
- .long 0x9abcdef0
+ASM_INT(saved_magic, 0x9abcdef0)
On 07/02/2024 1:37 pm, Jan Beulich wrote: > Use the generic framework from xen/linkage.h. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > The .Lsuspend_err label is used in a cross-function manner here, but > it's not clear to me what - if anything - to do about this. Well - again like VMX, this is special CODE. It really is one function, do_suspend_lowlevel() - see how it's caller uses it, with a magic position in the middle that is joined by the S3 path. s3_resume should be a CODE too, or whatever we call a not-LABEL. ~Andrew
Use the generic framework from xen/linkage.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Using the linker script like this feels fragile. Maybe it's better to
suppress (#undef) CONFIG_CC_SPLIT_SECTIONS for this one file?
---
v6: New.
--- a/xen/arch/x86/x86_64/kexec_reloc.S
+++ b/xen/arch/x86/x86_64/kexec_reloc.S
@@ -21,10 +21,9 @@
#include <asm/machine_kexec.h>
.section .text.kexec, "ax", @progbits
- .align PAGE_SIZE
.code64
-ENTRY(kexec_reloc)
+FUNC(kexec_reloc, PAGE_SIZE)
/* %rdi - code page maddr */
/* %rsi - page table maddr */
/* %rdx - indirection page maddr */
@@ -91,8 +90,9 @@ ENTRY(kexec_reloc)
push $0x10
push %rax
lretq
+END(kexec_reloc)
-relocate_pages:
+FUNC_LOCAL(relocate_pages)
/* %rdi - indirection page maddr */
pushq %rbx
@@ -138,10 +138,11 @@ relocate_pages:
.L_done:
popq %rbx
ret
+END(relocate_pages)
.code32
-compatibility_mode:
+FUNC_LOCAL(compatibility_mode)
/* Setup some sane segments. */
movl $0x0008, %eax
movl %eax, %ds
@@ -168,39 +169,29 @@ compatibility_mode:
/* Call the image entry point. This should never return. */
call *%ebp
ud2
+END(compatibility_mode)
- .align 4
-compat_mode_gdt_desc:
+DATA_LOCAL(compat_mode_gdt_desc, 4)
.word .Lcompat_mode_gdt_end - compat_mode_gdt -1
.quad 0x0000000000000000 /* set in call_32_bit above */
+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:
+DATA_LOCAL(compat_mode_gdt, 8)
.quad 0x0000000000000000 /* null */
.quad 0x00cf93000000ffff /* 0x0008 ring 0 data */
.quad 0x00cf9b000000ffff /* 0x0010 ring 0 code, compatibility */
.Lcompat_mode_gdt_end:
+END(compat_mode_gdt)
- .type compat_mode_gdt, @object
- .size compat_mode_gdt, . - compat_mode_gdt
-
-compat_mode_idt:
+DATA_LOCAL(compat_mode_idt)
.word 0 /* limit */
.long 0 /* base */
-
- .type compat_mode_idt, @object
- .size compat_mode_idt, . - compat_mode_idt
+END(compat_mode_idt)
/*
* 16 words of stack are more than enough.
*/
- .align 8
-reloc_stack:
+DATA_LOCAL(reloc_stack, 8)
.fill 16,8,0
.Lreloc_stack_base:
-
- .type reloc_stack, @object
- .size reloc_stack, . - reloc_stack
+END(reloc_stack)
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -92,7 +92,10 @@ SECTIONS
. = ALIGN(PAGE_SIZE);
_etextentry = .;
- *(.text.kexec) /* Page aligned in the object file. */
+ /* Page aligned in the object file. */
+ *(.text.kexec.kexec_reloc)
+ *(.text.kexec.*)
+ *(.text.kexec)
kexec_reloc_end = .;
*(.text.cold)
On 07/02/2024 1:38 pm, Jan Beulich wrote: > Use the generic framework from xen/linkage.h. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Using the linker script like this feels fragile. Maybe it's better to > suppress (#undef) CONFIG_CC_SPLIT_SECTIONS for this one file? This is specific glue code, needing to fit in a single page. I'd prefer to explicitly state that SPLIT_SECTIONS is inapplciable, than to try to undo the effects in the linker script. Everything else looks ok. I'll fix the preexiting errors after a rebase. ~Andrew
Use the generic framework from xen/linkage.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v6: New.
--- a/xen/arch/x86/clear_page.S
+++ b/xen/arch/x86/clear_page.S
@@ -1,8 +1,9 @@
.file __FILE__
+#include <xen/linkage.h>
#include <asm/page.h>
-ENTRY(clear_page_sse2)
+FUNC(clear_page_sse2)
mov $PAGE_SIZE/32, %ecx
xor %eax,%eax
@@ -16,6 +17,4 @@ ENTRY(clear_page_sse2)
sfence
ret
-
- .type clear_page_sse2, @function
- .size clear_page_sse2, . - clear_page_sse2
+END(clear_page_sse2)
--- a/xen/arch/x86/copy_page.S
+++ b/xen/arch/x86/copy_page.S
@@ -1,5 +1,6 @@
.file __FILE__
+#include <xen/linkage.h>
#include <asm/page.h>
#define src_reg %rsi
@@ -10,7 +11,7 @@
#define tmp3_reg %r10
#define tmp4_reg %r11
-ENTRY(copy_page_sse2)
+FUNC(copy_page_sse2)
mov $PAGE_SIZE/(4*WORD_SIZE)-3, %ecx
prefetchnta 2*4*WORD_SIZE(src_reg)
@@ -41,6 +42,4 @@ ENTRY(copy_page_sse2)
sfence
ret
-
- .type copy_page_sse2, @function
- .size copy_page_sse2, . - copy_page_sse2
+END(copy_page_sse2)
--- a/xen/arch/x86/guest/xen/hypercall_page.S
+++ b/xen/arch/x86/guest/xen/hypercall_page.S
@@ -3,13 +3,11 @@
#include <public/xen.h>
.section ".text.page_aligned", "ax", @progbits
- .p2align PAGE_SHIFT
-GLOBAL(hypercall_page)
+DATA(hypercall_page, PAGE_SIZE)
/* Poisoned with `ret` for safety before hypercalls are set up. */
.fill PAGE_SIZE, 1, 0xc3
- .type hypercall_page, STT_OBJECT
- .size hypercall_page, PAGE_SIZE
+END(hypercall_page)
/*
* Identify a specific hypercall in the hypercall page
--- a/xen/arch/x86/indirect-thunk.S
+++ b/xen/arch/x86/indirect-thunk.S
@@ -35,15 +35,13 @@
.macro GEN_INDIRECT_THUNK reg:req
.section .text.__x86_indirect_thunk_\reg, "ax", @progbits
-ENTRY(__x86_indirect_thunk_\reg)
+FUNC(__x86_indirect_thunk_\reg)
ALTERNATIVE_2 __stringify(IND_THUNK_RETPOLINE \reg), \
__stringify(IND_THUNK_LFENCE \reg), X86_FEATURE_IND_THUNK_LFENCE, \
__stringify(IND_THUNK_JMP \reg), X86_FEATURE_IND_THUNK_JMP
int3 /* Halt straight-line speculation */
-
- .size __x86_indirect_thunk_\reg, . - __x86_indirect_thunk_\reg
- .type __x86_indirect_thunk_\reg, @function
+END(__x86_indirect_thunk_\reg)
.endm
/* Instantiate GEN_INDIRECT_THUNK for each register except %rsp. */
--- a/xen/arch/x86/pv/gpr_switch.S
+++ b/xen/arch/x86/pv/gpr_switch.S
@@ -10,7 +10,7 @@
#include <asm/asm_defns.h>
/* Load guest GPRs. Parameter in %rdi, clobbers all registers. */
-ENTRY(load_guest_gprs)
+FUNC(load_guest_gprs)
movq UREGS_rdx(%rdi), %rdx
movq UREGS_rax(%rdi), %rax
movq UREGS_rbx(%rdi), %rbx
@@ -27,13 +27,10 @@ ENTRY(load_guest_gprs)
movq UREGS_rcx(%rdi), %rcx
movq UREGS_rdi(%rdi), %rdi
ret
-
- .size load_guest_gprs, . - load_guest_gprs
- .type load_guest_gprs, STT_FUNC
-
+END(load_guest_gprs)
/* Save guest GPRs. Parameter on the stack above the return address. */
-ENTRY(save_guest_gprs)
+FUNC(save_guest_gprs)
pushq %rdi
movq 2*8(%rsp), %rdi
movq %rax, UREGS_rax(%rdi)
@@ -52,6 +49,4 @@ ENTRY(save_guest_gprs)
movq %rdx, UREGS_rdx(%rdi)
movq %rcx, UREGS_rcx(%rdi)
ret
-
- .size save_guest_gprs, . - save_guest_gprs
- .type save_guest_gprs, STT_FUNC
+END(save_guest_gprs)
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -599,7 +599,7 @@ domain_crash_page_fault_0x8:
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
movq %rsi,%rdi
call show_page_walk
-ENTRY(dom_crash_sync_extable)
+LABEL(dom_crash_sync_extable, 0)
ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
# Get out of the guest-save area of the stack.
GET_STACK_END(ax)
On 07/02/2024 1:38 pm, Jan Beulich wrote: > --- a/xen/arch/x86/x86_64/entry.S > +++ b/xen/arch/x86/x86_64/entry.S > @@ -599,7 +599,7 @@ domain_crash_page_fault_0x8: > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > movq %rsi,%rdi > call show_page_walk > -ENTRY(dom_crash_sync_extable) > +LABEL(dom_crash_sync_extable, 0) > ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP > # Get out of the guest-save area of the stack. > GET_STACK_END(ax) > This again is a function, and one even used across-TUs. Furthermore, it's a (domain) fatal error path. It has the least excuse of all to not conform to a regular function-like layout. Everything else looks fine. If you want to split this out into a separate patch to address its function-ness, then consider the remainder Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
... to boot code, limiting their scope and thus allowing to drop
respective #undef-s from the linker script.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
An obvious alternative would be to convert boot code right away too, but
I think this has lower priority for now.
---
v6: New.
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -10,6 +10,15 @@
#include <asm/cpufeature.h>
#include <public/elfnote.h>
+#define ALIGN .align CODE_ALIGN, CODE_FILL
+#define ENTRY(name) \
+ ALIGN; \
+ GLOBAL(name)
+#define GLOBAL(name) \
+ .globl name; \
+ .hidden name; \
+ name:
+
.section .text.header, "ax", @progbits
.code32
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -45,14 +45,6 @@
#ifdef __ASSEMBLY__
#define CODE_ALIGN 16
#define CODE_FILL 0x90
-#define ALIGN .align CODE_ALIGN, CODE_FILL
-#define ENTRY(name) \
- ALIGN; \
- GLOBAL(name)
-#define GLOBAL(name) \
- .globl name; \
- .hidden name; \
- name:
#endif
#define NR_hypercalls 64
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -1199,7 +1199,7 @@ FUNC_LOCAL(autogen_stubs, 0) /* Automati
.if vec >= FIRST_IRQ_VECTOR
#endif
- ALIGN
+ .align CODE_ALIGN, CODE_FILL
1:
ENDBR64
pushq $0
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -5,8 +5,6 @@
#include <xen/lib.h>
#include <xen/xen.lds.h>
#include <asm/page.h>
-#undef ENTRY
-#undef ALIGN
#ifdef EFI
On 07/02/2024 1:39 pm, Jan Beulich wrote: > ... to boot code, limiting their scope and thus allowing to drop > respective #undef-s from the linker script. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > An obvious alternative would be to convert boot code right away too, but > I think this has lower priority for now. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> It would be nice to take these out, and it doesn't look as if it's too much work to do, but I guess we can settle for getting them out of config.h ~Andrew
© 2016 - 2024 Red Hat, Inc.