Re: [PATCH v4 1/8] common: assembly entry point type/size annotations

[Julien Grall <julien@xxxxxxx>]

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