Re: [XEN PATCH] xen: Add SAF deviations for MISRA C:2012 Rule 7.1

[andrew.cooper3@xxxxxxxxxx]

On 03/10/2023 4:37 pm, Nicola Vetrini wrote:
> As specified in rules.rst, these constants can be used
> in the code.
> Their deviation is now accomplished by using a SAF comment,
> rather than an ECLAIR configuration.
>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ------
>  docs/misra/safe.json                             | 8 ++++++++
>  xen/arch/x86/hvm/svm/emulate.c                   | 6 +++---
>  xen/arch/x86/hvm/svm/svm.h                       | 9 +++++++++
>  xen/common/inflate.c                             | 4 ++--
>  5 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
> b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index d8170106b449..fbb806a75d73 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -132,12 +132,6 @@ safe."
>  # Series 7.
>  #
>  
> --doc_begin="Usage of the following constants is safe, since they are given 
> as-is
> -in the inflate algorithm specification and there is therefore no risk of them
> -being interpreted as decimal constants."
> --config=MC3R1.R7.1,literals={safe, 
> "^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
> --doc_end
> -
>  -doc_begin="Violations in files that maintainers have asked to not modify in 
> the
>  context of R7.2."
>  -file_tag+={adopted_r7_2,"^xen/include/xen/libfdt/.*$"}
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 39c5c056c7d4..7ea47344ffcc 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -20,6 +20,14 @@
>          },
>          {
>              "id": "SAF-2-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.R7.1"
> +            },
> +            "name": "Rule 7.1: constants defined in specifications, manuals, 
> and algorithm descriptions",
> +            "text": "It is safe to use certain octal constants the way they 
> are defined in specifications, manuals, and algorithm descriptions."
> +        },
> +        {
> +            "id": "SAF-3-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index aa2c61c433b3..c5e3341c6316 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -90,9 +90,9 @@ unsigned int svm_get_insn_len(struct vcpu *v, unsigned int 
> instr_enc)
>          if ( !instr_modrm )
>              return emul_len;
>  
> -        if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) &&
> -             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> -             (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )
> +        if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* 
> SAF-2-safe */
> +             (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* 
> SAF-2-safe */
> +             (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* 
> SAF-2-safe */
>              return emul_len;
>      }

This is line noise, and later examples are even worse.

What does SAF mean?  It's presumably not the Scalable Agile Framework.

It is meaningless to anyone reading the code who doesn't know it's a
magic identifier to suppress violations.

Looking in scripts/xen_analysis, it appears to be a labelling scheme
we've in invented for the purpose of cross-referencing, in which case it
needs to be changed to something more obviously safety/misra/etc related
to make the code clearer to follow.

~Andrew