Re: [XEN PATCH][for-4.19 v6 2/8] x86: add deviation for asm-only functions

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 01/11/2023 23:10, Stefano Stabellini wrote:
> On Wed, 1 Nov 2023, Nicola Vetrini wrote:
> > As stated in rules.rst, functions used only in asm modules
> > are allowed to have no prior declaration visible when being
> > defined, hence these functions are marked with an
> > 'asmlinkage' macro, which is then deviated for MISRA C:2012
> > Rule 8.4.
> >
> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > ---
> > Changes in v3:
> > - added SAF deviations for vmx counterparts to svm functions.
> > Changes in v5:
> > - drop SAF deviations in favour of the pseudo-attribute asmlinkage
> > Changes in v6:
> > - conditioned asmlinkage definition to make it overridable;
> > - move the pseudo-attribute after the return type
> > ---
> >   automation/eclair_analysis/ECLAIR/deviations.ecl | 9 +++++++++
> >   docs/misra/deviations.rst                        | 6 ++++++
> >   xen/arch/x86/hvm/svm/intr.c                      | 2 +-
> >   xen/arch/x86/hvm/svm/nestedsvm.c                 | 2 +-
> >   xen/arch/x86/hvm/svm/svm.c                       | 4 ++--
> >   xen/arch/x86/hvm/vmx/intr.c                      | 2 +-
> >   xen/arch/x86/hvm/vmx/vmx.c                       | 4 ++--
> >   xen/arch/x86/hvm/vmx/vvmx.c                      | 2 +-
> >   xen/arch/x86/traps.c                             | 2 +-
> >   xen/arch/x86/x86_64/traps.c                      | 2 +-
> >   xen/include/xen/compiler.h                       | 5 +++++
> >   11 files changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> > b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > index fa56e5c00a27..06619ecf7e8d 100644
> > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> > @@ -163,6 +163,15 @@ Therefore the absence of prior declarations is safe."
> >   -config=MC3R1.R8.4,reports+={safe, "first_area(any_loc(file(gcov)))"}
> >   -doc_end
> >   
> > +-doc_begin="Recognize the occurrence of current_stack_pointer as a declaration."
> > +-file_tag+={asm_defns, "^xen/arch/x86/include/asm/asm_defns\\.h$"}
> > +-config=MC3R1.R8.4,declarations+={safe, 
> > "loc(file(asm_defns))&&^current_stack_pointer$"}
> > +-doc_end
> > +
> > +-doc_begin="asmlinkage is a marker to indicate that the function is only used to 
> > interface with asm modules."
> > +-config=MC3R1.R8.4,declarations+={safe,"loc(text(^(?s).*asmlinkage.*$, 
> > -1..0))"}
> > +-doc_end
> > +
> >   -doc_begin="The following variables are compiled in multiple translation units
> >   belonging to different executables and therefore are safe."
> >   -config=MC3R1.R8.6,declarations+={safe, 
> > "name(current_stack_pointer||bsearch||sort)"}
> > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > index 8511a189253b..d468da2f5ce9 100644
> > --- a/docs/misra/deviations.rst
> > +++ b/docs/misra/deviations.rst
> > @@ -133,6 +133,12 @@ Deviations related to MISRA C:2012 Rules:
> >          configuration. Therefore, the absence of prior declarations is safe.
> >        - Tagged as `safe` for ECLAIR.
> >   
> > +   * - R8.4
> > +     - Functions and variables used only by asm modules are either marked with
> > +       the `asmlinkage` macro or with a SAF-1-safe textual deviation
> > +       (see safe.json).
> > +     - Tagged as `safe` for ECLAIR.
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> If Julien prefers a different wording I could modify on commit as needed
I think my comment on the previous version was misunderstood. I have 
asked to replace all SAF-1 with asmlinkage and I thought you agreed with 
that.
This is a similar situation to octal-ok. I don't think it is right to 
have multiple ways to tag a deviation.
I don't particular want to see more added, and I would like the current 
ones to be gone. So what's the plan to remove SAF-1-safe?
At minimum, the deviation should be extremely clear that there **must** 
be no new use of SAF-1-safe. Something like:
"- Functions and variables used only by asm modules are either marked 
with the `asmlinkage` macro. Existing code may use SAF-1-safe textual 
deviation (see safe.json) but new code should not use it.
"

Obviously this is not my preference.

Cheers,

--
Julien Grall