Re: [XEN PATCH][for-4.19 v2 2/2] docs/misra: add deviations.rst to document additional deviations.

[Julien Grall <julien@xxxxxxx>]

Hi,

On 10/10/2023 23:27, Stefano Stabellini wrote:
> On Tue, 10 Oct 2023, Nicola Vetrini wrote:
> > On 10/10/2023 03:19, Stefano Stabellini wrote:
> > > +Henry
> > >
> > > On Mon, 9 Oct 2023, Nicola Vetrini wrote:
> > > > This file contains the deviation that are not marked by
> > > > a deviation comment, as specified in
> > > > docs/misra/documenting-violations.rst.
> > > >
> > > > Suggested-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > This is great! Thank you so much!
> > >
> > > I have a few questions below but even as-is it is way better than
> > > nothing. I think we should add this for 4.18
> > >
> > >
> > > > ---
> > > >   docs/index.rst            |   1 +
> > > >   docs/misra/deviations.rst | 240 ++++++++++++++++++++++++++++++++++++++
> > > >   docs/misra/rules.rst      |   2 +-
> > > >   3 files changed, 242 insertions(+), 1 deletion(-)
> > > >   create mode 100644 docs/misra/deviations.rst
> > > >
> > > > diff --git a/docs/index.rst b/docs/index.rst
> > > > index 2c47cfa999f2..f3f779f89ce5 100644
> > > > --- a/docs/index.rst
> > > > +++ b/docs/index.rst
> > > > @@ -63,6 +63,7 @@ Xen hypervisor code.
> > > >      :maxdepth: 2
> > > >
> > > >      misra/rules
> > > > +   misra/deviations
> > > >
> > > >
> > > >   Miscellanea
> > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
> > > > new file mode 100644
> > > > index 000000000000..19743e34ce03
> > > > --- /dev/null
> > > > +++ b/docs/misra/deviations.rst
> > > > @@ -0,0 +1,240 @@
> > > > +.. SPDX-License-Identifier: CC-BY-4.0
> > > > +
> > > > +MISRA C deviations for Xen
> > > > +==========================
> > > > +
> > > > +The following is the list of MISRA C:2012 deviations for the Xen codebase
> > > > that
> > > > +are not covered by a `SAF-x-safe` or `SAF-x-false-positive-<tool>`
> > > > comment, as
> > > > +specified in docs/misra/documenting-violations.rst; the lack of
> > > > +such comments is usually due to the excessive clutter they would bring to
> > > > the
> > > > +codebase or the impossibility to express such a deviation (e.g., if it's
> > > > +composed of several conditions).
> > > > +
> > > > +Deviations related to MISRA C:2012 Directives:
> > > > +----------------------------------------------
> > > > +
> > > > +.. list-table::
> > > > +   :header-rows: 1
> > > > +
> > > > +   * - Directive identifier
> > > > +     - Justification
> > > > +     - Notes
> > > > +
> > > > +   * - D4.3
> > > > +     - Accepted for the ARM64 codebase
> > > > +     - Tagged as `disapplied` for ECLAIR on any other violation report.
> > > This mean it has been applied for ARM64 but not x86, right?
> > Yes.
> >
> > > > +   * - D4.3
> > > > +     - The inline asm in 'xen/arch/arm/arm64/lib/bitops.c' is tightly
> > > > coupled
> > > > +       with the surronding C code that acts as a wrapper, so it has been
> > > > decided
> > > > +       not to add an additional encapsulation layer.
> > > > +     - Tagged as `deliberate` for ECLAIR.
> > > > +
> > > > +Deviations related to MISRA C:2012 Rules:
> > > > +-----------------------------------------
> > > > +
> > > > +.. list-table::
> > > > +   :header-rows: 1
> > > > +
> > > > +   * - Rule identifier
> > > > +     - Justification
> > > > +     - Notes
> > > > +
> > > > +   * - R2.1
> > > > +     - The compiler implementation guarantees that the unreachable code
> > > > is
> > > > +       removed. Constant expressions and unreachable branches of if and
> > > > switch
> > > > +       statements are expected.
> > > > +     - Tagged as `safe` for ECLAIR.
> > > > +
> > > > +   * - R2.1
> > > > +     - Some functions are intended not to be referenced.
> > > > +     - Tagged as `deliberate` for ECLAIR.
> > > What does it mean "some functions" in this case? Should we list which
> > > functions?
> > Well, there are a lot, typically resulting from build configurations that do
> > not
> > use them, or because they are used only in asm code. I can mention these
> > reasons in the
> > document, to make it easier to understand.
> Yes, I think we need to clarify further this point, because saying "Some
> functions" doesn't help the reader understand:
> - whether all functions can be not referenced
> - which subset of functions can be not referenced
>
> How to distinguish between? How do we know whether a certain patch is
> violating the rule or not?
>
> If there is a clear list of functions that can be not referenced, then
> we should list them here. If there is a methodology we can use to
> distinguish between them (e.g. functions called from asm only) then we
> can write the methodology here. Either way it is fine as long as the
> criteria to know if it is OK if a function is not referenced is clear.
Aren't they more or less the one we tagged with SAF-1-safe because there 
were no prototype? If so, we could use the same tags.
We could introduce an extra tags for the others. An alternative would be 
to add an attribute (e.g. asmcall) to mark each function used by assembly.
Cheers,

--
Julien Grall