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
