Re: [XEN PATCH v3] xen: replace occurrences of SAF-1-safe with asmlinkage attribute

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2023-11-23 11:26, Julien Grall wrote:
> Hi Nicola,
> 
> On 23/11/2023 09:25, Nicola Vetrini wrote:
>> On 2023-11-23 09:57, Jan Beulich wrote:
>>> On 16.11.2023 10:08, Nicola Vetrini wrote:
>>>> The comment-based justifications for MISRA C:2012 Rule 8.4 are 
>>>> replaced
>>>> by the asmlinkage pseudo-attribute, for the sake of uniformity.
>>>> 
>>>> Add missing 'xen/compiler.h' #include-s where needed.
>>>> 
>>>> The text in docs/misra/deviations.rst and docs/misra/safe.json
>>>> is modified to reflect this change.
>>>> 
>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>> ---
>>>> This patch should be applied after patch 2 of this series.
>>>> The request made by Julien to update the wording is
>>>> contained in the present patch.
>>> 
>>> Along with this request he supplied you with an ack. Did you drop 
>>> that
>>> for a particular reason, or did you simply forget to record it here?
>>> 
>> 
>> I forgot and added it in a reply.
>> 
>>>> --- a/xen/arch/x86/boot/reloc.c
>>>> +++ b/xen/arch/x86/boot/reloc.c
>>>> @@ -28,6 +28,7 @@ asm (
>>>> 
>>>>  #include "defs.h"
>>>> 
>>>> +#include <xen/compiler.h>
>>>>  #include <xen/kconfig.h>
>>>>  #include <xen/multiboot.h>
>>>>  #include <xen/multiboot2.h>
>>>> @@ -348,9 +349,8 @@ static multiboot_info_t *mbi2_reloc(uint32_t 
>>>> mbi_in, uint32_t video_out)
>>>>      return mbi_out;
>>>>  }
>>>> 
>>>> -/* SAF-1-safe */
>>>> -void *__stdcall reloc(uint32_t magic, uint32_t in, uint32_t 
>>>> trampoline,
>>>> -                      uint32_t video_info)
>>>> +void *asmlinkage __stdcall reloc(uint32_t magic, uint32_t in,
>>>> +                                 uint32_t trampoline, uint32_t 
>>>> video_info)
>>>>  {
>>> 
>>> One purpose of asmlinkage is to possibly alter the default C calling 
>>> convention
>>> to some more easy to use in assembly code. At least over a period of 
>>> time Linux
>>> for example used that on ix86. If we decided to do something like 
>>> this, there
>>> would be a collision with __stdcall. Hence I'm not convinced we can 
>>> put
>>> asmlinkage here. At which point the complete removal of SAF-1-safe 
>>> also would
>>> need dropping.
>>> 
>> 
>> Ok, so we can keep SAF-1 here and below and leave the wording in 
>> deviations.rst and safe.sjon as is.
> 
> I guess you mean without this patch applied and not including my 
> proposed rewording (i.e. to deprecated SAF-1)?
> 
> If so, then we are back to the initial concern I raised. We would have 
> two ways to address the deviation. From a user perspective, it would be 
> unclear which one should be used.
> 

The wording that is now on staging (committed by Stefano).

- Functions and variables used only by asm modules are marked with
   the `asmlinkage` macro. Existing code may use a SAF-1-safe
   textual deviation (see safe.json), but new code should not use
   it.

I guess special cases can be dealt with as they arise. That may or may 
not be mentioned in the deviation.

> In particular, I would rather favor asmlinkage whenever it is possible 
> and only use SAF-1 when it is not possible to use it (like here).
> 
> Another possibility would be to deviate __stdcall like we do for 
> asmlinkage (I will let Jan confirm if this is desirable). With this 
> approach, there is less ambiguity when to use either of them.
> 
> Cheers,

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)