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
