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

[Julien Grall <julien@xxxxxxx>]

Hi Nicola,

On 23/11/2023 10:40, Nicola Vetrini wrote:
> 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@xxxxxxxxxxx>
> > > > > ---
> > > > > 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.
Ah so the wording has been updated! It is not 100% ideal if we decide to 
keep some SAF-1 for good reason. But better than any ambiguity on when 
to use one but not the other. We can reword once we have other cases.
Cheers,

--
Julien Grall