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

[Jan Beulich <jbeulich@suse.com>]

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