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

[Jan Beulich <jbeulich@xxxxxxxx>]

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