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

[Julien Grall <julien@xxxxxxx>]

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