Re: [PATCH v2 1/2] x86: annotate entry points with type and size

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxx>
> 
> [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.