[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 18 Sep 2023 12:51:27 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=vH0Y9SSwVu3q5r397urDHgTrlVwifdC/Yj0+Vy7Gxis=; b=imnUmWFKeS19LIx3nq4zi5OPLnXd5ZxIg7uBgHr0xKXICyXAOzTkvxuRNfEbZpawK0L/67NOQyJDyz7R3kEixv43AUMqlnzg4gBL8QGYvfeeF51o5MtgDQHVcgLZPstDqVBTnUdJRDZ+7V/Aw8YiDUlNeltiQUEeHgS0pyIYK7uIK7UnBsfxipJBUpJgsiZvEkvjhuagI0lPEQnKpMYjW1A1HGgnwYZ/I92GojY7dtJoogG16Elt3LczTK2HdwX8PmGLo46SZ9JJlagN6ObEr6cnVodd7JbJ9myW49/uTOXOabj5WIdTTLbprlyU+d+3aN2bbR4Q0cJF6K1nU7A8Rw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IegZBPupIogUuqonH1rOUDGB6uJJtiyaBtoqcEnoLUSiq6AJhvclgNpdgag1jg27ZIobIn+BIPocSBvJjaVICO6Sr3Ywf7gJcLBEQPsndlGkPYCmEKUk5y7l/eDWQ74w5WJywrvXvEo41Om3/WYThoEPh1lI4F8GPMVW0P0g1TtyXHDxoO2aCLG9U8tXFYHBp06USeMtIrlsXpDS7ie+AU4IRg0xSrInGBZySwvoNhSdzWML9/9WDMceBqyJzNQ5WxhVL3jHHS/+4ERkKpckEnyqL1VAukqxOuP6sJ2i11Awydw+yhZ6LfNG67m2tyX3+ibVmSVWCzMZ2ceIr/vGcg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 18 Sep 2023 10:52:17 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.