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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 3 Oct 2023, at 17:17, andrew.cooper3@xxxxxxxxxx wrote:
> 
> 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;
>>     }
> 

Hi Andrew,

> This is line noise, and later examples are even worse.
> 
> What does SAF mean?  It's presumably not the Scalable Agile Framework.

Please have a look on docs/misra/documenting-violations.rst, you will find all 
the
info about it.

> 
> 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
>