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

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

[Nicola Vetrini]

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@bugseng.com>
---
 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;
     }
 
diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
index d2a781fc3fb5..d0623b72ccfa 100644
--- a/xen/arch/x86/hvm/svm/svm.h
+++ b/xen/arch/x86/hvm/svm/svm.h
@@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
 #define INSTR_INT3        INSTR_ENC(X86EMUL_OPC(   0, 0xcc), 0)
 #define INSTR_ICEBP       INSTR_ENC(X86EMUL_OPC(   0, 0xf1), 0)
 #define INSTR_HLT         INSTR_ENC(X86EMUL_OPC(   0, 0xf4), 0)
+/* SAF-2-safe */
 #define INSTR_XSETBV      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
+/* SAF-2-safe */
 #define INSTR_VMRUN       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
+/* SAF-2-safe */
 #define INSTR_VMCALL      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
+/* SAF-2-safe */
 #define INSTR_VMLOAD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
+/* SAF-2-safe */
 #define INSTR_VMSAVE      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
+/* SAF-2-safe */
 #define INSTR_STGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
+/* SAF-2-safe */
 #define INSTR_CLGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
+/* SAF-2-safe */
 #define INSTR_INVLPGA     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
+/* SAF-2-safe */
 #define INSTR_RDTSCP      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
 #define INSTR_INVD        INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
 #define INSTR_WBINVD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
diff --git a/xen/common/inflate.c b/xen/common/inflate.c
index 8fa4b96d12a3..be6a9115187e 100644
--- a/xen/common/inflate.c
+++ b/xen/common/inflate.c
@@ -1201,8 +1201,8 @@ static int __init gunzip(void)
     magic[1] = NEXTBYTE();
     method   = NEXTBYTE();
 
-    if (magic[0] != 037 ||
-        ((magic[1] != 0213) && (magic[1] != 0236))) {
+    /* SAF-2-safe */
+    if (magic[0] != 037 || ((magic[1] != 0213) && (magic[1] != 0236))) {
         error("bad gzip magic numbers");
         return -1;
     }
-- 
2.34.1

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

[andrew.cooper3]

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@bugseng.com>
> ---
>  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

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

[Luca Fancellu]

> On 3 Oct 2023, at 17:17, andrew.cooper3@citrix.com 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@bugseng.com>
>> ---
>> 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
> 

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

[Andrew Cooper]

On 03/10/2023 6:14 pm, Luca Fancellu wrote:
>
>> On 3 Oct 2023, at 17:17, andrew.cooper3@citrix.com 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@bugseng.com>
>>> ---
>>> 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.

Thankyou for proving my point perfectly.

The comment in the source code needs to be *far* clearer than it
currently is.

Even s/SAF/ANALYSIS/ would be an improvement, because it makes the
comment very clear that it's about code analysis.  An unknown initialism
like SAF does not convey enough meaning to be useful.

~Andrew

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5055 bytes --]

On Tue, 3 Oct 2023, Andrew Cooper wrote:
> On 03/10/2023 6:14 pm, Luca Fancellu wrote:
> >
> >> On 3 Oct 2023, at 17:17, andrew.cooper3@citrix.com 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@bugseng.com>
> >>> ---
> >>> 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.
> 
> Thankyou for proving my point perfectly.
> 
> The comment in the source code needs to be *far* clearer than it
> currently is.
> 
> Even s/SAF/ANALYSIS/ would be an improvement, because it makes the
> comment very clear that it's about code analysis.  An unknown initialism
> like SAF does not convey enough meaning to be useful.

Hi Andrew,

I am OK with a rename of the "SAF" tag.

The number of instances is still small enough that a rename can be done
at this point in time. Given that the SAF framework was reviewed by
multiple people, and we already have a few SAF tags in the code base and
even more in my for-4.19 branch, I suggest to start a separate thread on
the topic.

A new thread with a clear subject like "rename SAF to BLAH" and CCing
all the maintainers in the MISRA C working group to make sure everyone
is aware.

Ideally the email should also have a couple of good suggestions for new
tags. I don't have a strong opinion on the name. ANALYSIS is not great
because the tag is meant to say that the line below is safe even if some
MISRA C scanners might find an issue with it. SAF is meant to remind us
of "SAFE". So I would prefer to add the letter "E" and call it "SAFE".

If we can come up with 3-5 options then we can have a doodle poll or
something.

Cheers,

Stefano

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

[Stefano Stabellini]

On Tue, 3 Oct 2023, 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@bugseng.com>

"SAF" discussion aside that can be resolved elsewhere:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  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;
>      }
>  
> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> index d2a781fc3fb5..d0623b72ccfa 100644
> --- a/xen/arch/x86/hvm/svm/svm.h
> +++ b/xen/arch/x86/hvm/svm/svm.h
> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>  #define INSTR_INT3        INSTR_ENC(X86EMUL_OPC(   0, 0xcc), 0)
>  #define INSTR_ICEBP       INSTR_ENC(X86EMUL_OPC(   0, 0xf1), 0)
>  #define INSTR_HLT         INSTR_ENC(X86EMUL_OPC(   0, 0xf4), 0)
> +/* SAF-2-safe */
>  #define INSTR_XSETBV      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
> +/* SAF-2-safe */
>  #define INSTR_VMRUN       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
> +/* SAF-2-safe */
>  #define INSTR_VMCALL      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
> +/* SAF-2-safe */
>  #define INSTR_VMLOAD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
> +/* SAF-2-safe */
>  #define INSTR_VMSAVE      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
> +/* SAF-2-safe */
>  #define INSTR_STGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
> +/* SAF-2-safe */
>  #define INSTR_CLGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
> +/* SAF-2-safe */
>  #define INSTR_INVLPGA     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
> +/* SAF-2-safe */
>  #define INSTR_RDTSCP      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>  #define INSTR_INVD        INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>  #define INSTR_WBINVD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> diff --git a/xen/common/inflate.c b/xen/common/inflate.c
> index 8fa4b96d12a3..be6a9115187e 100644
> --- a/xen/common/inflate.c
> +++ b/xen/common/inflate.c
> @@ -1201,8 +1201,8 @@ static int __init gunzip(void)
>      magic[1] = NEXTBYTE();
>      method   = NEXTBYTE();
>  
> -    if (magic[0] != 037 ||
> -        ((magic[1] != 0213) && (magic[1] != 0236))) {
> +    /* SAF-2-safe */
> +    if (magic[0] != 037 || ((magic[1] != 0213) && (magic[1] != 0236))) {
>          error("bad gzip magic numbers");
>          return -1;
>      }
> -- 
> 2.34.1
> 

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

[andrew.cooper3]

[-- Attachment #1: Type: text/plain, Size: 3082 bytes --]

On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
> On Tue, 3 Oct 2023, 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@bugseng.com>
> "SAF" discussion aside that can be resolved elsewhere:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Well no.  "SAF" aside (and SAF does need fixing before reposting this
patch, otherwise it's just unnecessary churn), ...

>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>> index d2a781fc3fb5..d0623b72ccfa 100644
>> --- a/xen/arch/x86/hvm/svm/svm.h
>> +++ b/xen/arch/x86/hvm/svm/svm.h
>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>  #define INSTR_INT3        INSTR_ENC(X86EMUL_OPC(   0, 0xcc), 0)
>>  #define INSTR_ICEBP       INSTR_ENC(X86EMUL_OPC(   0, 0xf1), 0)
>>  #define INSTR_HLT         INSTR_ENC(X86EMUL_OPC(   0, 0xf4), 0)
>> +/* SAF-2-safe */
>>  #define INSTR_XSETBV      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>> +/* SAF-2-safe */
>>  #define INSTR_VMRUN       INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>> +/* SAF-2-safe */
>>  #define INSTR_VMCALL      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>> +/* SAF-2-safe */
>>  #define INSTR_VMLOAD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>> +/* SAF-2-safe */
>>  #define INSTR_VMSAVE      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>> +/* SAF-2-safe */
>>  #define INSTR_STGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>> +/* SAF-2-safe */
>>  #define INSTR_CLGI        INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>> +/* SAF-2-safe */
>>  #define INSTR_INVLPGA     INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>> +/* SAF-2-safe */
>>  #define INSTR_RDTSCP      INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>  #define INSTR_INVD        INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>  #define INSTR_WBINVD      INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)

... this has broken a tabulated structure to have comments ahead of
lines with octal numbers, while ...

>> 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 has comments at the end of lines with octal numbers.

So which is it?

~Andrew

[-- Attachment #2: Type: text/html, Size: 4186 bytes --]

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

[Luca Fancellu]

Hi Nicola,

> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
> 
> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>> 
>> "SAF" discussion aside that can be resolved elsewhere:
>> 
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
> 
>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>> +/* SAF-2-safe */
>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>> +/* SAF-2-safe */
>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>> +/* SAF-2-safe */
>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>> +/* SAF-2-safe */
>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>> +/* SAF-2-safe */
>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>> +/* SAF-2-safe */
>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>> +/* SAF-2-safe */
>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>> +/* SAF-2-safe */
>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>> +/* SAF-2-safe */
>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> 
> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
> 
>>> 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 has comments at the end of lines with octal numbers.
> 
> So which is it?

I agree with Andrew here in this sense: the in-code comment is supposed to be on the line *before* the violation,
not on the same line, so I’m also wondering how it is fixing the very first violation.

Cheers,
Luca

> 
> ~Andrew

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

[Nicola Vetrini]

On 04/10/2023 12:06, Luca Fancellu wrote:
> Hi Nicola,
> 
>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>> 
>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>> 
>>> "SAF" discussion aside that can be resolved elsewhere:
>>> 
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>> 
>> Well no.  "SAF" aside (and SAF does need fixing before reposting this 
>> patch, otherwise it's just unnecessary churn), ...
>> 
>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long 
>>>> linear, uint32_t asid)
>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>> +/* SAF-2-safe */
>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>> 
>> ... this has broken a tabulated structure to have comments ahead of 
>> lines with octal numbers, while ...
>> 
>>>> 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 has comments at the end of lines with octal numbers.
>> 
>> So which is it?
> 
> I agree with Andrew here in this sense: the in-code comment is
> supposed to be on the line *before* the violation,
> not on the same line, so I’m also wondering how it is fixing the very
> first violation.
> 
> Cheers,
> Luca
> 

Actually it justifies what is on either the previous line or the same 
because it's
translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how 
many lines besides
the current one are to be deviated (e.g. you can have 0 deviate only the 
current line).
Most of the times the current form is what's needed, as you would put 
the comment on a line
of its own. In the case of the if that would break the formatting. The 
downside of doing the same thing on the table is that the first entry 
not to be deviated would actually be deviated.

#define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)

This may not be problematic, since 0 could be considered an octal 
constant, but is an
exception explicitly listed in the MISRA rule.
For the same reason the line

return emul_len;

is deviated by the above comment, but putting an octal constant there 
would for sure
be the result of a deliberate choice. There's the alternative of:

                           /* SAF-2-safe */
    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) )

to make it consistent with the table and avoid any "hidden" deviated 
line or, again,
the modification of the translation script so that it doesn't use a 
fixed "1" offset, which
is motivated by what you wrote on the thread of the modification of 
xen_analysis.py.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Luca Fancellu]

> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> 
> On 04/10/2023 12:06, Luca Fancellu wrote:
>> Hi Nicola,
>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>> +/* SAF-2-safe */
>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
>>>>> 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 has comments at the end of lines with octal numbers.
>>> So which is it?
>> I agree with Andrew here in this sense: the in-code comment is
>> supposed to be on the line *before* the violation,
>> not on the same line, so I’m also wondering how it is fixing the very
>> first violation.
>> Cheers,
>> Luca
> 

Hi Nicola,

> Actually it justifies what is on either the previous line or the same because it's
> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
> the current one are to be deviated (e.g. you can have 0 deviate only the current line).

Just to understand, does this way:

<line A>
/* -E> safe MC3R1.R7.1 1 */
<line B>

Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
as intended.


> Most of the times the current form is what's needed, as you would put the comment on a line
> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
> 
> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> 
> This may not be problematic, since 0 could be considered an octal constant, but is an
> exception explicitly listed in the MISRA rule.
> For the same reason the line
> 
> return emul_len;
> 
> is deviated by the above comment, but putting an octal constant there would for sure
> be the result of a deliberate choice. There's the alternative of:
> 
>                          /* SAF-2-safe */
>   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) )
> 
> to make it consistent with the table and avoid any "hidden" deviated line or, again,
> the modification of the translation script so that it doesn't use a fixed "1" offset, which
> is motivated by what you wrote on the thread of the modification of xen_analysis.py.

From the documentation:

    In the Xen codebase, these tags will be used to document and suppress findings:

    - SAF-X-safe: This tag means that the next line of code contains a finding, but
      the non compliance to the checker is analysed and demonstrated to be safe.

I understand that Eclair is capable of suppressing also the line in which the in-code suppression
comment resides, but these generic Xen in-code suppression comment are meant to be used
by multiple static analysis tools and many of them suppress only the line next to the comment
(Coverity, cppcheck).

So I’m in favour of your approach below, clearly it depends on what the maintainers feedback is:

> 
>                          /* SAF-2-safe */
>   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) )


Cheer,
Luca

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

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

On 04/10/2023 11:52 am, Luca Fancellu wrote:
> From the documentation:
>
>     In the Xen codebase, these tags will be used to document and suppress findings:
>
>     - SAF-X-safe: This tag means that the next line of code contains a finding, but
>       the non compliance to the checker is analysed and demonstrated to be safe.
>
> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
> comment resides, but these generic Xen in-code suppression comment are meant to be used
> by multiple static analysis tools and many of them suppress only the line next to the comment
> (Coverity, cppcheck).
>
> So I’m in favour of your approach below, clearly it depends on what the maintainers feedback is:
>
>>                          /* SAF-2-safe */
>>   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) )

To be clear, this is illegible and a non-starter from a code maintenance
point of view.

It is bad enough needing annotations to start with, but the annotations
*must* not interfere with the prior legibility.

The form with comments on the end, that do not break up the tabulation
of the code, is tolerable, providing the SAF turns into something
meaningful.

~Andrew

P.S. to be clear, I'm not saying that an ahead-of-line comments are
unacceptable generally.  Something like

    /* $FOO-$N-safe */
    if ( blah )

might be fine in context, but that is a decision that needs to be made
based on how the code reads with the comment in place.

[-- Attachment #2: Type: text/html, Size: 2328 bytes --]

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

[Luca Fancellu]

> On 4 Oct 2023, at 12:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 04/10/2023 11:52 am, Luca Fancellu wrote:
>> 
>> From the documentation:
>> 
>> In the Xen codebase, these tags will be used to document and suppress findings:
>> 
>> - SAF-X-safe: This tag means that the next line of code contains a finding, but
>> the non compliance to the checker is analysed and demonstrated to be safe.
>> 
>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
>> comment resides, but these generic Xen in-code suppression comment are meant to be used
>> by multiple static analysis tools and many of them suppress only the line next to the comment
>> (Coverity, cppcheck).
>> 
>> So I’m in favour of your approach below, clearly it depends on what the maintainers feedback is:
>> 
>> 
>>> /* SAF-2-safe */
>>> 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) )
> 
> To be clear, this is illegible and a non-starter from a code maintenance point of view.
> 
> It is bad enough needing annotations to start with, but the annotations *must* not interfere with the prior legibility.

I agree with that, the code as above is not very nice, however as the current status it is the only way it can work,
maybe rewriting it in another way could solve the issue?

For example:

/* SAF-2-safe */
#define SENSIBLE_NAME_HERE(instr)   MASK_EXTR(instr, 0300)
/* SAF-2-safe */
#define SENSIBLE_NAME_HERE2(instr)   MASK_EXTR(instr, 0300)
/* SAF-2-safe */
#define SENSIBLE_NAME_HERE3(instr)   MASK_EXTR(instr, 0300)

And use these macro in the conditions above, however will it move the violation at the macro definition?

Having macro with a sensible name explaining the meaning of the value could also improve the readability of the code.

But this choice is up on you x86 maintainers.

> 
> The form with comments on the end, that do not break up the tabulation of the code, is tolerable, providing the SAF turns into something meaningful.
> 
> ~Andrew
> 
> P.S. to be clear, I'm not saying that an ahead-of-line comments are unacceptable generally.  Something like
> 
>     /* $FOO-$N-safe */
>     if ( blah )
> 
> might be fine in context, but that is a decision that needs to be made based on how the code reads with the comment in place.

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

[Nicola Vetrini]

On 04/10/2023 12:52, Luca Fancellu wrote:
>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> 
>> wrote:
>> 
>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>> Hi Nicola,
>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting 
>>>> this patch, otherwise it's just unnecessary churn), ...
>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h 
>>>>>> b/xen/arch/x86/hvm/svm/svm.h
>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long 
>>>>>> linear, uint32_t asid)
>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>>> +/* SAF-2-safe */
>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>>> ... this has broken a tabulated structure to have comments ahead of 
>>>> lines with octal numbers, while ...
>>>>>> 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 has comments at the end of lines with octal numbers.
>>>> So which is it?
>>> I agree with Andrew here in this sense: the in-code comment is
>>> supposed to be on the line *before* the violation,
>>> not on the same line, so I’m also wondering how it is fixing the very
>>> first violation.
>>> Cheers,
>>> Luca
>> 
> 
> Hi Nicola,
> 
>> Actually it justifies what is on either the previous line or the same 
>> because it's
>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is 
>> how many lines besides
>> the current one are to be deviated (e.g. you can have 0 deviate only 
>> the current line).
> 
> Just to understand, does this way:
> 
> <line A>
> /* -E> safe MC3R1.R7.1 1 */
> <line B>
> 
> Justifies only line B? Because I thought so, but now I want to be
> sure, otherwise it doesn’t act
> as intended.
> 
> 

Yes, line A is untouched.

<line A>
/* -E> safe MC3R1.R7.1 1 */ (deviated)
<line B>                    (deviated)



-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7521 bytes --]

On Wed, 4 Oct 2023, Luca Fancellu wrote:
> > On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> > On 04/10/2023 12:06, Luca Fancellu wrote:
> >> Hi Nicola,
> >>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
> >>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
> >>>> On Tue, 3 Oct 2023, 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@bugseng.com>
> >>>> "SAF" discussion aside that can be resolved elsewhere:
> >>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
> >>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> >>>>> index d2a781fc3fb5..d0623b72ccfa 100644
> >>>>> --- a/xen/arch/x86/hvm/svm/svm.h
> >>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
> >>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
> >>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
> >>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
> >>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
> >>>>> +/* SAF-2-safe */
> >>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
> >>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> >>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> >>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
> >>>>> 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 has comments at the end of lines with octal numbers.
> >>> So which is it?
> >> I agree with Andrew here in this sense: the in-code comment is
> >> supposed to be on the line *before* the violation,
> >> not on the same line, so I’m also wondering how it is fixing the very
> >> first violation.
> >> Cheers,
> >> Luca
> > 
> 
> Hi Nicola,
> 
> > Actually it justifies what is on either the previous line or the same because it's
> > translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
> > the current one are to be deviated (e.g. you can have 0 deviate only the current line).
> 
> Just to understand, does this way:
> 
> <line A>
> /* -E> safe MC3R1.R7.1 1 */
> <line B>
> 
> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
> as intended.
> 
> 
> > Most of the times the current form is what's needed, as you would put the comment on a line
> > of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
> > 
> > #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> > 
> > This may not be problematic, since 0 could be considered an octal constant, but is an
> > exception explicitly listed in the MISRA rule.
> > For the same reason the line
> > 
> > return emul_len;
> > 
> > is deviated by the above comment, but putting an octal constant there would for sure
> > be the result of a deliberate choice. There's the alternative of:
> > 
> >                          /* SAF-2-safe */
> >   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) )
> > 
> > to make it consistent with the table and avoid any "hidden" deviated line or, again,
> > the modification of the translation script so that it doesn't use a fixed "1" offset, which
> > is motivated by what you wrote on the thread of the modification of xen_analysis.py.
> 
> From the documentation:
> 
>     In the Xen codebase, these tags will be used to document and suppress findings:
> 
>     - SAF-X-safe: This tag means that the next line of code contains a finding, but
>       the non compliance to the checker is analysed and demonstrated to be safe.
> 
> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
> comment resides, but these generic Xen in-code suppression comment are meant to be used
> by multiple static analysis tools and many of them suppress only the line next to the comment
> (Coverity, cppcheck).

As we see more realistic examples, it turns out that this is limiting.

Given that the SAF-2-safe comment needs to go through xen-analysis.py
translations anyway, could we implement something a bit more flexible in
xen-analysis.py?

For instance, could we implement a format with the number of lines of
code like this as we discussed in a previous thread?

/* SAF-2-safe start */
if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
/* SAF-2-safe end */

Firstly, let ask Andrew, do you prefer this?


And also this second format:

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


Could we implement in xen-analysis.py a conversion that would turn the
two formats above that are not understood by cppcheck into:

/* cppcheck tag */
if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
/* cppcheck tag */
    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
/* cppcheck tag */
    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )

Or this is a problem because it would end up changing lines of code
numbers in the source file?

If we can hide the "ugliness" behind the xen-analysis conversion tool we
could have a clean codebase and still be compatible with multiple tools.

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

[Luca Fancellu]

> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Wed, 4 Oct 2023, Luca Fancellu wrote:
>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>>> Hi Nicola,
>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
>>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
>>>>>>> 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 has comments at the end of lines with octal numbers.
>>>>> So which is it?
>>>> I agree with Andrew here in this sense: the in-code comment is
>>>> supposed to be on the line *before* the violation,
>>>> not on the same line, so I’m also wondering how it is fixing the very
>>>> first violation.
>>>> Cheers,
>>>> Luca
>>> 
>> 
>> Hi Nicola,
>> 
>>> Actually it justifies what is on either the previous line or the same because it's
>>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
>>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
>> 
>> Just to understand, does this way:
>> 
>> <line A>
>> /* -E> safe MC3R1.R7.1 1 */
>> <line B>
>> 
>> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
>> as intended.
>> 
>> 
>>> Most of the times the current form is what's needed, as you would put the comment on a line
>>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
>>> 
>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>> 
>>> This may not be problematic, since 0 could be considered an octal constant, but is an
>>> exception explicitly listed in the MISRA rule.
>>> For the same reason the line
>>> 
>>> return emul_len;
>>> 
>>> is deviated by the above comment, but putting an octal constant there would for sure
>>> be the result of a deliberate choice. There's the alternative of:
>>> 
>>>                         /* SAF-2-safe */
>>>  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) )
>>> 
>>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
>>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
>>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
>> 
>> From the documentation:
>> 
>>    In the Xen codebase, these tags will be used to document and suppress findings:
>> 
>>    - SAF-X-safe: This tag means that the next line of code contains a finding, but
>>      the non compliance to the checker is analysed and demonstrated to be safe.
>> 
>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
>> comment resides, but these generic Xen in-code suppression comment are meant to be used
>> by multiple static analysis tools and many of them suppress only the line next to the comment
>> (Coverity, cppcheck).
> 
> As we see more realistic examples, it turns out that this is limiting.
> 
> Given that the SAF-2-safe comment needs to go through xen-analysis.py
> translations anyway, could we implement something a bit more flexible in
> xen-analysis.py?
> 
> For instance, could we implement a format with the number of lines of
> code like this as we discussed in a previous thread?
> 
> /* SAF-2-safe start */
> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> /* SAF-2-safe end */
> 
> Firstly, let ask Andrew, do you prefer this?
> 
> 
> And also this second format:
> 
> 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 */
> 
> 
> Could we implement in xen-analysis.py a conversion that would turn the
> two formats above that are not understood by cppcheck into:
> 
> /* cppcheck tag */
> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> /* cppcheck tag */
>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> /* cppcheck tag */
>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> 
> Or this is a problem because it would end up changing lines of code
> numbers in the source file?

Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end */


> 
> If we can hide the "ugliness" behind the xen-analysis conversion tool we
> could have a clean codebase and still be compatible with multiple tools.

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 8600 bytes --]

On Thu, 5 Oct 2023, Luca Fancellu wrote:
> > On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Wed, 4 Oct 2023, Luca Fancellu wrote:
> >>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> >>> On 04/10/2023 12:06, Luca Fancellu wrote:
> >>>> Hi Nicola,
> >>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
> >>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
> >>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
> >>>>>> "SAF" discussion aside that can be resolved elsewhere:
> >>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
> >>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> >>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
> >>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
> >>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
> >>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
> >>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
> >>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
> >>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
> >>>>>>> +/* SAF-2-safe */
> >>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
> >>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> >>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> >>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
> >>>>>>> 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 has comments at the end of lines with octal numbers.
> >>>>> So which is it?
> >>>> I agree with Andrew here in this sense: the in-code comment is
> >>>> supposed to be on the line *before* the violation,
> >>>> not on the same line, so I’m also wondering how it is fixing the very
> >>>> first violation.
> >>>> Cheers,
> >>>> Luca
> >>> 
> >> 
> >> Hi Nicola,
> >> 
> >>> Actually it justifies what is on either the previous line or the same because it's
> >>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
> >>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
> >> 
> >> Just to understand, does this way:
> >> 
> >> <line A>
> >> /* -E> safe MC3R1.R7.1 1 */
> >> <line B>
> >> 
> >> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
> >> as intended.
> >> 
> >> 
> >>> Most of the times the current form is what's needed, as you would put the comment on a line
> >>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
> >>> 
> >>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> >>> 
> >>> This may not be problematic, since 0 could be considered an octal constant, but is an
> >>> exception explicitly listed in the MISRA rule.
> >>> For the same reason the line
> >>> 
> >>> return emul_len;
> >>> 
> >>> is deviated by the above comment, but putting an octal constant there would for sure
> >>> be the result of a deliberate choice. There's the alternative of:
> >>> 
> >>>                         /* SAF-2-safe */
> >>>  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) )
> >>> 
> >>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
> >>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
> >>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
> >> 
> >> From the documentation:
> >> 
> >>    In the Xen codebase, these tags will be used to document and suppress findings:
> >> 
> >>    - SAF-X-safe: This tag means that the next line of code contains a finding, but
> >>      the non compliance to the checker is analysed and demonstrated to be safe.
> >> 
> >> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
> >> comment resides, but these generic Xen in-code suppression comment are meant to be used
> >> by multiple static analysis tools and many of them suppress only the line next to the comment
> >> (Coverity, cppcheck).
> > 
> > As we see more realistic examples, it turns out that this is limiting.
> > 
> > Given that the SAF-2-safe comment needs to go through xen-analysis.py
> > translations anyway, could we implement something a bit more flexible in
> > xen-analysis.py?
> > 
> > For instance, could we implement a format with the number of lines of
> > code like this as we discussed in a previous thread?
> > 
> > /* SAF-2-safe start */
> > if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> >    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> >    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> > /* SAF-2-safe end */
> > 
> > Firstly, let ask Andrew, do you prefer this?
> > 
> > 
> > And also this second format:
> > 
> > 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 */
> > 
> > 
> > Could we implement in xen-analysis.py a conversion that would turn the
> > two formats above that are not understood by cppcheck into:
> > 
> > /* cppcheck tag */
> > if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> > /* cppcheck tag */
> >    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> > /* cppcheck tag */
> >    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> > 
> > Or this is a problem because it would end up changing lines of code
> > numbers in the source file?
> 
> Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end */

Right so the results would be all off by a few lines of code so when
you go to read the report generated by cppcheck, the references
wouldn't match anymore.

Before giving up and accepting that we are constrained to only formats
that don't change the LOC numbers, can we check what Coverity supports?

I am asking because we could get away with implementing the formats
above in cppcheck, given that cppcheck is open source. But for Coverity
we need to stay with what is already supported by it.

Does Coverity support anything other than:

<tag on previous line>
<next line is code with deviation>

?

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

[Luca Fancellu]

> On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 5 Oct 2023, Luca Fancellu wrote:
>>> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Wed, 4 Oct 2023, Luca Fancellu wrote:
>>>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>>>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>>>>> Hi Nicola,
>>>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>>>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
>>>>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>>>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>>>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
>>>>>>>>> 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 has comments at the end of lines with octal numbers.
>>>>>>> So which is it?
>>>>>> I agree with Andrew here in this sense: the in-code comment is
>>>>>> supposed to be on the line *before* the violation,
>>>>>> not on the same line, so I’m also wondering how it is fixing the very
>>>>>> first violation.
>>>>>> Cheers,
>>>>>> Luca
>>>>> 
>>>> 
>>>> Hi Nicola,
>>>> 
>>>>> Actually it justifies what is on either the previous line or the same because it's
>>>>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
>>>>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
>>>> 
>>>> Just to understand, does this way:
>>>> 
>>>> <line A>
>>>> /* -E> safe MC3R1.R7.1 1 */
>>>> <line B>
>>>> 
>>>> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
>>>> as intended.
>>>> 
>>>> 
>>>>> Most of the times the current form is what's needed, as you would put the comment on a line
>>>>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
>>>>> 
>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>> 
>>>>> This may not be problematic, since 0 could be considered an octal constant, but is an
>>>>> exception explicitly listed in the MISRA rule.
>>>>> For the same reason the line
>>>>> 
>>>>> return emul_len;
>>>>> 
>>>>> is deviated by the above comment, but putting an octal constant there would for sure
>>>>> be the result of a deliberate choice. There's the alternative of:
>>>>> 
>>>>>                        /* SAF-2-safe */
>>>>> 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) )
>>>>> 
>>>>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
>>>>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
>>>>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
>>>> 
>>>> From the documentation:
>>>> 
>>>>   In the Xen codebase, these tags will be used to document and suppress findings:
>>>> 
>>>>   - SAF-X-safe: This tag means that the next line of code contains a finding, but
>>>>     the non compliance to the checker is analysed and demonstrated to be safe.
>>>> 
>>>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
>>>> comment resides, but these generic Xen in-code suppression comment are meant to be used
>>>> by multiple static analysis tools and many of them suppress only the line next to the comment
>>>> (Coverity, cppcheck).
>>> 
>>> As we see more realistic examples, it turns out that this is limiting.
>>> 
>>> Given that the SAF-2-safe comment needs to go through xen-analysis.py
>>> translations anyway, could we implement something a bit more flexible in
>>> xen-analysis.py?
>>> 
>>> For instance, could we implement a format with the number of lines of
>>> code like this as we discussed in a previous thread?
>>> 
>>> /* SAF-2-safe start */
>>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>>>   (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>>   (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
>>> /* SAF-2-safe end */
>>> 
>>> Firstly, let ask Andrew, do you prefer this?
>>> 
>>> 
>>> And also this second format:
>>> 
>>> 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 */
>>> 
>>> 
>>> Could we implement in xen-analysis.py a conversion that would turn the
>>> two formats above that are not understood by cppcheck into:
>>> 
>>> /* cppcheck tag */
>>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>>> /* cppcheck tag */
>>>   (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>> /* cppcheck tag */
>>>   (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
>>> 
>>> Or this is a problem because it would end up changing lines of code
>>> numbers in the source file?
>> 
>> Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end */
> 
> Right so the results would be all off by a few lines of code so when
> you go to read the report generated by cppcheck, the references
> wouldn't match anymore.
> 
> Before giving up and accepting that we are constrained to only formats
> that don't change the LOC numbers, can we check what Coverity supports?
> 
> I am asking because we could get away with implementing the formats
> above in cppcheck, given that cppcheck is open source. But for Coverity
> we need to stay with what is already supported by it.
> 
> Does Coverity support anything other than:
> 
> <tag on previous line>
> <next line is code with deviation>

Unfortunately not, from its documentation I can’t see anything apart from the above,
I can ask someone from synopsys though to double check.

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 9639 bytes --]

On Fri, 6 Oct 2023, Luca Fancellu wrote:
> > On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Thu, 5 Oct 2023, Luca Fancellu wrote:
> >>> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>> 
> >>> On Wed, 4 Oct 2023, Luca Fancellu wrote:
> >>>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
> >>>>> On 04/10/2023 12:06, Luca Fancellu wrote:
> >>>>>> Hi Nicola,
> >>>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
> >>>>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
> >>>>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
> >>>>>>>> "SAF" discussion aside that can be resolved elsewhere:
> >>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>>>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
> >>>>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
> >>>>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
> >>>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
> >>>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
> >>>>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
> >>>>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
> >>>>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
> >>>>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
> >>>>>>>>> +/* SAF-2-safe */
> >>>>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
> >>>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> >>>>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
> >>>>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
> >>>>>>>>> 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 has comments at the end of lines with octal numbers.
> >>>>>>> So which is it?
> >>>>>> I agree with Andrew here in this sense: the in-code comment is
> >>>>>> supposed to be on the line *before* the violation,
> >>>>>> not on the same line, so I’m also wondering how it is fixing the very
> >>>>>> first violation.
> >>>>>> Cheers,
> >>>>>> Luca
> >>>>> 
> >>>> 
> >>>> Hi Nicola,
> >>>> 
> >>>>> Actually it justifies what is on either the previous line or the same because it's
> >>>>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
> >>>>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
> >>>> 
> >>>> Just to understand, does this way:
> >>>> 
> >>>> <line A>
> >>>> /* -E> safe MC3R1.R7.1 1 */
> >>>> <line B>
> >>>> 
> >>>> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
> >>>> as intended.
> >>>> 
> >>>> 
> >>>>> Most of the times the current form is what's needed, as you would put the comment on a line
> >>>>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
> >>>>> 
> >>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> >>>>> 
> >>>>> This may not be problematic, since 0 could be considered an octal constant, but is an
> >>>>> exception explicitly listed in the MISRA rule.
> >>>>> For the same reason the line
> >>>>> 
> >>>>> return emul_len;
> >>>>> 
> >>>>> is deviated by the above comment, but putting an octal constant there would for sure
> >>>>> be the result of a deliberate choice. There's the alternative of:
> >>>>> 
> >>>>>                        /* SAF-2-safe */
> >>>>> 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) )
> >>>>> 
> >>>>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
> >>>>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
> >>>>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
> >>>> 
> >>>> From the documentation:
> >>>> 
> >>>>   In the Xen codebase, these tags will be used to document and suppress findings:
> >>>> 
> >>>>   - SAF-X-safe: This tag means that the next line of code contains a finding, but
> >>>>     the non compliance to the checker is analysed and demonstrated to be safe.
> >>>> 
> >>>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
> >>>> comment resides, but these generic Xen in-code suppression comment are meant to be used
> >>>> by multiple static analysis tools and many of them suppress only the line next to the comment
> >>>> (Coverity, cppcheck).
> >>> 
> >>> As we see more realistic examples, it turns out that this is limiting.
> >>> 
> >>> Given that the SAF-2-safe comment needs to go through xen-analysis.py
> >>> translations anyway, could we implement something a bit more flexible in
> >>> xen-analysis.py?
> >>> 
> >>> For instance, could we implement a format with the number of lines of
> >>> code like this as we discussed in a previous thread?
> >>> 
> >>> /* SAF-2-safe start */
> >>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> >>>   (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> >>>   (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> >>> /* SAF-2-safe end */
> >>> 
> >>> Firstly, let ask Andrew, do you prefer this?
> >>> 
> >>> 
> >>> And also this second format:
> >>> 
> >>> 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 */
> >>> 
> >>> 
> >>> Could we implement in xen-analysis.py a conversion that would turn the
> >>> two formats above that are not understood by cppcheck into:
> >>> 
> >>> /* cppcheck tag */
> >>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> >>> /* cppcheck tag */
> >>>   (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> >>> /* cppcheck tag */
> >>>   (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> >>> 
> >>> Or this is a problem because it would end up changing lines of code
> >>> numbers in the source file?
> >> 
> >> Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end */
> > 
> > Right so the results would be all off by a few lines of code so when
> > you go to read the report generated by cppcheck, the references
> > wouldn't match anymore.
> > 
> > Before giving up and accepting that we are constrained to only formats
> > that don't change the LOC numbers, can we check what Coverity supports?
> > 
> > I am asking because we could get away with implementing the formats
> > above in cppcheck, given that cppcheck is open source. But for Coverity
> > we need to stay with what is already supported by it.
> > 
> > Does Coverity support anything other than:
> > 
> > <tag on previous line>
> > <next line is code with deviation>
> 
> Unfortunately not, from its documentation I can’t see anything apart from the above,
> I can ask someone from synopsys though to double check.

I wonder how people would feel to have an exception to our coding style
in these cases and have longer than 80 chars lines. I am asking because
this is better than many of the other options above:

/* SAF-x-safe */
if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )

Any other ideas?

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

[Luca Fancellu]

>>> 
>>> Right so the results would be all off by a few lines of code so when
>>> you go to read the report generated by cppcheck, the references
>>> wouldn't match anymore.
>>> 
>>> Before giving up and accepting that we are constrained to only formats
>>> that don't change the LOC numbers, can we check what Coverity supports?
>>> 
>>> I am asking because we could get away with implementing the formats
>>> above in cppcheck, given that cppcheck is open source. But for Coverity
>>> we need to stay with what is already supported by it.
>>> 
>>> Does Coverity support anything other than:
>>> 
>>> <tag on previous line>
>>> <next line is code with deviation>
>> 
>> Unfortunately not, from its documentation I can’t see anything apart from the above,
>> I can ask someone from synopsys though to double check.
> 
> I wonder how people would feel to have an exception to our coding style
> in these cases and have longer than 80 chars lines. I am asking because
> this is better than many of the other options above:
> 
> /* SAF-x-safe */
> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> 
> Any other ideas?

Hi Stefano, 

I’ve suggested some mails ago to use #define for the MASK_EXTR so that
every instance of the violation goes into a different line, but this works only
If Eclair is throwing the violation at the definition and not at its usage.

I remember we had a discussion months ago about that but I don’t remember
If Roberto told us that this behaviour can be adjusted in Eclair or not.

An exception in the coding style would make the adoption of an automatic tool
difficult.

Cheers,
Luca

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

[Julien Grall]

Hi,

On 07/10/2023 01:43, Stefano Stabellini wrote:
> On Fri, 6 Oct 2023, Luca Fancellu wrote:
>>> On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>
>>> On Thu, 5 Oct 2023, Luca Fancellu wrote:
>>>>> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>>
>>>>> On Wed, 4 Oct 2023, Luca Fancellu wrote:
>>>>>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>>>>>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>>>>>>> Hi Nicola,
>>>>>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>>>>>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>>>>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>>>>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>>>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>>>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>>>>>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>>>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>>>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>>>>>>>> +/* SAF-2-safe */
>>>>>>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>>>>>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
>>>>>>>>>>> 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 has comments at the end of lines with octal numbers.
>>>>>>>>> So which is it?
>>>>>>>> I agree with Andrew here in this sense: the in-code comment is
>>>>>>>> supposed to be on the line *before* the violation,
>>>>>>>> not on the same line, so I’m also wondering how it is fixing the very
>>>>>>>> first violation.
>>>>>>>> Cheers,
>>>>>>>> Luca
>>>>>>>
>>>>>>
>>>>>> Hi Nicola,
>>>>>>
>>>>>>> Actually it justifies what is on either the previous line or the same because it's
>>>>>>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
>>>>>>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
>>>>>>
>>>>>> Just to understand, does this way:
>>>>>>
>>>>>> <line A>
>>>>>> /* -E> safe MC3R1.R7.1 1 */
>>>>>> <line B>
>>>>>>
>>>>>> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
>>>>>> as intended.
>>>>>>
>>>>>>
>>>>>>> Most of the times the current form is what's needed, as you would put the comment on a line
>>>>>>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
>>>>>>>
>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>>>
>>>>>>> This may not be problematic, since 0 could be considered an octal constant, but is an
>>>>>>> exception explicitly listed in the MISRA rule.
>>>>>>> For the same reason the line
>>>>>>>
>>>>>>> return emul_len;
>>>>>>>
>>>>>>> is deviated by the above comment, but putting an octal constant there would for sure
>>>>>>> be the result of a deliberate choice. There's the alternative of:
>>>>>>>
>>>>>>>                         /* SAF-2-safe */
>>>>>>> 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) )
>>>>>>>
>>>>>>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
>>>>>>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
>>>>>>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
>>>>>>
>>>>>>  From the documentation:
>>>>>>
>>>>>>    In the Xen codebase, these tags will be used to document and suppress findings:
>>>>>>
>>>>>>    - SAF-X-safe: This tag means that the next line of code contains a finding, but
>>>>>>      the non compliance to the checker is analysed and demonstrated to be safe.
>>>>>>
>>>>>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
>>>>>> comment resides, but these generic Xen in-code suppression comment are meant to be used
>>>>>> by multiple static analysis tools and many of them suppress only the line next to the comment
>>>>>> (Coverity, cppcheck).
>>>>>
>>>>> As we see more realistic examples, it turns out that this is limiting.
>>>>>
>>>>> Given that the SAF-2-safe comment needs to go through xen-analysis.py
>>>>> translations anyway, could we implement something a bit more flexible in
>>>>> xen-analysis.py?
>>>>>
>>>>> For instance, could we implement a format with the number of lines of
>>>>> code like this as we discussed in a previous thread?
>>>>>
>>>>> /* SAF-2-safe start */
>>>>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>>>>>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>>>>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
>>>>> /* SAF-2-safe end */
>>>>>
>>>>> Firstly, let ask Andrew, do you prefer this?
>>>>>
>>>>>
>>>>> And also this second format:
>>>>>
>>>>> 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 */
>>>>>
>>>>>
>>>>> Could we implement in xen-analysis.py a conversion that would turn the
>>>>> two formats above that are not understood by cppcheck into:
>>>>>
>>>>> /* cppcheck tag */
>>>>> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>>>>> /* cppcheck tag */
>>>>>    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>>>>> /* cppcheck tag */
>>>>>    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
>>>>>
>>>>> Or this is a problem because it would end up changing lines of code
>>>>> numbers in the source file?
>>>>
>>>> Yes this is the real issue why we didn’t do the /* ... start */ code /* ... end */
>>>
>>> Right so the results would be all off by a few lines of code so when
>>> you go to read the report generated by cppcheck, the references
>>> wouldn't match anymore.
>>>
>>> Before giving up and accepting that we are constrained to only formats
>>> that don't change the LOC numbers, can we check what Coverity supports?
>>>
>>> I am asking because we could get away with implementing the formats
>>> above in cppcheck, given that cppcheck is open source. But for Coverity
>>> we need to stay with what is already supported by it.
>>>
>>> Does Coverity support anything other than:
>>>
>>> <tag on previous line>
>>> <next line is code with deviation>
>>
>> Unfortunately not, from its documentation I can’t see anything apart from the above,
>> I can ask someone from synopsys though to double check.
> 
> I wonder how people would feel to have an exception to our coding style
> in these cases and have longer than 80 chars lines. I am asking because
> this is better than many of the other options above:

I am not sure this is better. This is a long line to read. But this is a 
personal opinion.

On the technical side, can we easily teach a tool to format this kind of 
exception? If not, then this should not be an exception we should implement.

> 
> /* SAF-x-safe */
> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> 
> Any other ideas?

Could we have a number in the comment to indicate the number of lines 
the comment applies to?

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 13371 bytes --]

On Mon, 9 Oct 2023, Julien Grall wrote:
> On 07/10/2023 01:43, Stefano Stabellini wrote:
> > On Fri, 6 Oct 2023, Luca Fancellu wrote:
> > > > On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabellini@kernel.org>
> > > > wrote:
> > > > 
> > > > On Thu, 5 Oct 2023, Luca Fancellu wrote:
> > > > > > On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@kernel.org>
> > > > > > wrote:
> > > > > > 
> > > > > > On Wed, 4 Oct 2023, Luca Fancellu wrote:
> > > > > > > > On 4 Oct 2023, at 11:29, Nicola Vetrini
> > > > > > > > <nicola.vetrini@bugseng.com> wrote:
> > > > > > > > On 04/10/2023 12:06, Luca Fancellu wrote:
> > > > > > > > > Hi Nicola,
> > > > > > > > > > On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
> > > > > > > > > > On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
> > > > > > > > > > > On Tue, 3 Oct 2023, 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@bugseng.com>
> > > > > > > > > > > "SAF" discussion aside that can be resolved elsewhere:
> > > > > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > > > > > > > > Well no.  "SAF" aside (and SAF does need fixing before
> > > > > > > > > > reposting this patch, otherwise it's just unnecessary
> > > > > > > > > > churn), ...
> > > > > > > > > > > > diff --git a/xen/arch/x86/hvm/svm/svm.h
> > > > > > > > > > > > b/xen/arch/x86/hvm/svm/svm.h
> > > > > > > > > > > > index d2a781fc3fb5..d0623b72ccfa 100644
> > > > > > > > > > > > --- a/xen/arch/x86/hvm/svm/svm.h
> > > > > > > > > > > > +++ b/xen/arch/x86/hvm/svm/svm.h
> > > > > > > > > > > > @@ -57,14 +57,23 @@ static inline void
> > > > > > > > > > > > svm_invlpga(unsigned long linear, uint32_t asid)
> > > > > > > > > > > > #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
> > > > > > > > > > > > #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
> > > > > > > > > > > > #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0321)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0330)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0331)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0332)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0333)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0334)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0335)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0337)
> > > > > > > > > > > > +/* SAF-2-safe */
> > > > > > > > > > > > #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01),
> > > > > > > > > > > > 0371)
> > > > > > > > > > > > #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> > > > > > > > > > > > #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09),
> > > > > > > > > > > > 0)
> > > > > > > > > > ... this has broken a tabulated structure to have comments
> > > > > > > > > > ahead of lines with octal numbers, while ...
> > > > > > > > > > > > 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 has comments at the end of lines with octal
> > > > > > > > > > numbers.
> > > > > > > > > > So which is it?
> > > > > > > > > I agree with Andrew here in this sense: the in-code comment is
> > > > > > > > > supposed to be on the line *before* the violation,
> > > > > > > > > not on the same line, so I’m also wondering how it is fixing
> > > > > > > > > the very
> > > > > > > > > first violation.
> > > > > > > > > Cheers,
> > > > > > > > > Luca
> > > > > > > > 
> > > > > > > 
> > > > > > > Hi Nicola,
> > > > > > > 
> > > > > > > > Actually it justifies what is on either the previous line or the
> > > > > > > > same because it's
> > > > > > > > translated to /* -E> safe MC3R1.R7.1 1 */, where the last number
> > > > > > > > is how many lines besides
> > > > > > > > the current one are to be deviated (e.g. you can have 0 deviate
> > > > > > > > only the current line).
> > > > > > > 
> > > > > > > Just to understand, does this way:
> > > > > > > 
> > > > > > > <line A>
> > > > > > > /* -E> safe MC3R1.R7.1 1 */
> > > > > > > <line B>
> > > > > > > 
> > > > > > > Justifies only line B? Because I thought so, but now I want to be
> > > > > > > sure, otherwise it doesn’t act
> > > > > > > as intended.
> > > > > > > 
> > > > > > > 
> > > > > > > > Most of the times the current form is what's needed, as you
> > > > > > > > would put the comment on a line
> > > > > > > > of its own. In the case of the if that would break the
> > > > > > > > formatting. The downside of doing the same thing on the table is
> > > > > > > > that the first entry not to be deviated would actually be
> > > > > > > > deviated.
> > > > > > > > 
> > > > > > > > #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
> > > > > > > > 
> > > > > > > > This may not be problematic, since 0 could be considered an
> > > > > > > > octal constant, but is an
> > > > > > > > exception explicitly listed in the MISRA rule.
> > > > > > > > For the same reason the line
> > > > > > > > 
> > > > > > > > return emul_len;
> > > > > > > > 
> > > > > > > > is deviated by the above comment, but putting an octal constant
> > > > > > > > there would for sure
> > > > > > > > be the result of a deliberate choice. There's the alternative
> > > > > > > > of:
> > > > > > > > 
> > > > > > > >                         /* SAF-2-safe */
> > > > > > > > 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) )
> > > > > > > > 
> > > > > > > > to make it consistent with the table and avoid any "hidden"
> > > > > > > > deviated line or, again,
> > > > > > > > the modification of the translation script so that it doesn't
> > > > > > > > use a fixed "1" offset, which
> > > > > > > > is motivated by what you wrote on the thread of the modification
> > > > > > > > of xen_analysis.py.
> > > > > > > 
> > > > > > >  From the documentation:
> > > > > > > 
> > > > > > >    In the Xen codebase, these tags will be used to document and
> > > > > > > suppress findings:
> > > > > > > 
> > > > > > >    - SAF-X-safe: This tag means that the next line of code
> > > > > > > contains a finding, but
> > > > > > >      the non compliance to the checker is analysed and
> > > > > > > demonstrated to be safe.
> > > > > > > 
> > > > > > > I understand that Eclair is capable of suppressing also the line
> > > > > > > in which the in-code suppression
> > > > > > > comment resides, but these generic Xen in-code suppression comment
> > > > > > > are meant to be used
> > > > > > > by multiple static analysis tools and many of them suppress only
> > > > > > > the line next to the comment
> > > > > > > (Coverity, cppcheck).
> > > > > > 
> > > > > > As we see more realistic examples, it turns out that this is
> > > > > > limiting.
> > > > > > 
> > > > > > Given that the SAF-2-safe comment needs to go through
> > > > > > xen-analysis.py
> > > > > > translations anyway, could we implement something a bit more
> > > > > > flexible in
> > > > > > xen-analysis.py?
> > > > > > 
> > > > > > For instance, could we implement a format with the number of lines
> > > > > > of
> > > > > > code like this as we discussed in a previous thread?
> > > > > > 
> > > > > > /* SAF-2-safe start */
> > > > > > if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> > > > > >    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> > > > > >    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> > > > > > /* SAF-2-safe end */
> > > > > > 
> > > > > > Firstly, let ask Andrew, do you prefer this?
> > > > > > 
> > > > > > 
> > > > > > And also this second format:
> > > > > > 
> > > > > > 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
> > > > > > */
> > > > > > 
> > > > > > 
> > > > > > Could we implement in xen-analysis.py a conversion that would turn
> > > > > > the
> > > > > > two formats above that are not understood by cppcheck into:
> > > > > > 
> > > > > > /* cppcheck tag */
> > > > > > if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
> > > > > > /* cppcheck tag */
> > > > > >    (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
> > > > > > /* cppcheck tag */
> > > > > >    (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> > > > > > 
> > > > > > Or this is a problem because it would end up changing lines of code
> > > > > > numbers in the source file?
> > > > > 
> > > > > Yes this is the real issue why we didn’t do the /* ... start */ code
> > > > > /* ... end */
> > > > 
> > > > Right so the results would be all off by a few lines of code so when
> > > > you go to read the report generated by cppcheck, the references
> > > > wouldn't match anymore.
> > > > 
> > > > Before giving up and accepting that we are constrained to only formats
> > > > that don't change the LOC numbers, can we check what Coverity supports?
> > > > 
> > > > I am asking because we could get away with implementing the formats
> > > > above in cppcheck, given that cppcheck is open source. But for Coverity
> > > > we need to stay with what is already supported by it.
> > > > 
> > > > Does Coverity support anything other than:
> > > > 
> > > > <tag on previous line>
> > > > <next line is code with deviation>
> > > 
> > > Unfortunately not, from its documentation I can’t see anything apart from
> > > the above,
> > > I can ask someone from synopsys though to double check.
> > 
> > I wonder how people would feel to have an exception to our coding style
> > in these cases and have longer than 80 chars lines. I am asking because
> > this is better than many of the other options above:
> 
> I am not sure this is better. This is a long line to read. But this is a
> personal opinion.
> 
> On the technical side, can we easily teach a tool to format this kind of
> exception? If not, then this should not be an exception we should implement.

I am not sure I understand what you mean by "can we easily teach a tool
to format this kind of exception". Do you mean whether we can teach a
tool to interpret a multiline statement as a single statement?

If so, my understanding is that neither Coverity not cppcheck can do
that.


> > /* SAF-x-safe */
> > if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) ==
> > MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == MASK_EXTR(instr_modrm,
> > 0007) )
> > 
> > Any other ideas?
> 
> Could we have a number in the comment to indicate the number of lines the
> comment applies to?

Luca can confirm that what I am about to write is correct; my
understanding is that ECLAIR supports it, but cppcheck does not. Which
means for cppcheck we would have to translate the SAF tag with
xen_analyize to:

/* cppcheck tag */
line1
/* cppcheck tag */
line2
/* cppcheck tag */
line3

and that would end up changing the line numbers in the source files so
the cppcheck report wouldn't match with the original line numbers any
longer

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

[Luca Fancellu]

> 
>>> /* SAF-x-safe */
>>> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) ==
>>> MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == MASK_EXTR(instr_modrm,
>>> 0007) )
>>> 
>>> Any other ideas?
>> 
>> Could we have a number in the comment to indicate the number of lines the
>> comment applies to?
> 
> Luca can confirm that what I am about to write is correct; my
> understanding is that ECLAIR supports it, but cppcheck does not. Which
> means for cppcheck we would have to translate the SAF tag with
> xen_analyize to:
> 
> /* cppcheck tag */
> line1
> /* cppcheck tag */
> line2
> /* cppcheck tag */
> line3
> 
> and that would end up changing the line numbers in the source files so
> the cppcheck report wouldn't match with the original line numbers any
> longer

Yes, but it’s not only Cppcheck, it’s also Coverity that supports only the above notation.

For Cppcheck we could do something, but for Coverity we can’t.

Anyway, Stefano or Nicola, I would like to understand where Eclair reports the violation
in the case of #define, does it report at the usage or at the definition?

Cheers,
Luca

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

[Nicola Vetrini]

On 10/10/2023 09:29, Luca Fancellu wrote:
>> 
>>>> /* SAF-x-safe */
>>>> if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && (modrm_reg & 7) ==
>>>> MASK_EXTR(instr_modrm, 0070) && (modrm_rm & 7)  == 
>>>> MASK_EXTR(instr_modrm,
>>>> 0007) )
>>>> 
>>>> Any other ideas?
>>> 
>>> Could we have a number in the comment to indicate the number of lines 
>>> the
>>> comment applies to?
>> 
>> Luca can confirm that what I am about to write is correct; my
>> understanding is that ECLAIR supports it, but cppcheck does not. Which
>> means for cppcheck we would have to translate the SAF tag with
>> xen_analyize to:
>> 
>> /* cppcheck tag */
>> line1
>> /* cppcheck tag */
>> line2
>> /* cppcheck tag */
>> line3
>> 
>> and that would end up changing the line numbers in the source files so
>> the cppcheck report wouldn't match with the original line numbers any
>> longer
> 
> Yes, but it’s not only Cppcheck, it’s also Coverity that supports only
> the above notation.
> 
> For Cppcheck we could do something, but for Coverity we can’t.
> 
> Anyway, Stefano or Nicola, I would like to understand where Eclair
> reports the violation
> in the case of #define, does it report at the usage or at the 
> definition?
> 
> Cheers,
> Luca

The report is at the usage site, but ECLAIR can be configured to deviate 
based on a comment
at the macro definition, or also just the macro name for reports of that 
rule (e.g. Q[1-3]):

#define M(a, b) (b)
/* -E> safe MC3R1.R7.1 1 blabla */
#define Q1(s) M(s, 0300)
/* -E> safe MC3R1.R7.1 1 blabla */
#define Q2(s) M(s, 0070)
/* -E> safe MC3R1.R7.1 1 blabla */
#define Q3(s) M(s, 0007)

void f(void) {
   int x = 1;
   int y = 2;
   if ( (x & 2) == Q1(y) &&
        (x & 3) == Q2(y) &&
        (x & 7) == Q3(y) )
   {
      y = y + 1;
   }
}

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Julien Grall]

Hi Stefano,

On 09/10/2023 23:19, Stefano Stabellini wrote:
>>
>> I am not sure this is better. This is a long line to read. But this is a
>> personal opinion.
>>
>> On the technical side, can we easily teach a tool to format this kind of
>> exception? If not, then this should not be an exception we should implement.
> 
> I am not sure I understand what you mean by "can we easily teach a tool
> to format this kind of exception". Do you mean whether we can teach a
> tool to interpret a multiline statement as a single statement?

Sorry for the wording was not clear. I was referring to tools formatting 
the code (e.g. clang-format). Hopefully, at some point, we will finally 
have a way to automatically format the code. So we need a coding style 
that can be easily be checked.

It is not clear to me whether your proposed exception would work. We may 
have to allow longer lines (and this has drawback).

> /* cppcheck tag */
> line1
> /* cppcheck tag */
> line2
> /* cppcheck tag */
> line3
> 
> and that would end up changing the line numbers in the source files so
> the cppcheck report wouldn't match with the original line numbers any
> longer

Would cppcheck accepts tag at the end of the line? If so, the following 
would not modify the line count:

/* cppcheck tag */
line1 /* added cppcheck tag */
line2 /* added cppcheck tag */
line3 /* added cppcheck tag */

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Tue, 10 Oct 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/10/2023 23:19, Stefano Stabellini wrote:
> > > 
> > > I am not sure this is better. This is a long line to read. But this is a
> > > personal opinion.
> > > 
> > > On the technical side, can we easily teach a tool to format this kind of
> > > exception? If not, then this should not be an exception we should
> > > implement.
> > 
> > I am not sure I understand what you mean by "can we easily teach a tool
> > to format this kind of exception". Do you mean whether we can teach a
> > tool to interpret a multiline statement as a single statement?
> 
> Sorry for the wording was not clear. I was referring to tools formatting the
> code (e.g. clang-format). Hopefully, at some point, we will finally have a way
> to automatically format the code. So we need a coding style that can be easily
> be checked.
> 
> It is not clear to me whether your proposed exception would work. We may have
> to allow longer lines (and this has drawback).

Yes, that's a good point. It will probably be an issue with clang-format.


> > /* cppcheck tag */
> > line1
> > /* cppcheck tag */
> > line2
> > /* cppcheck tag */
> > line3
> > 
> > and that would end up changing the line numbers in the source files so
> > the cppcheck report wouldn't match with the original line numbers any
> > longer
> 
> Would cppcheck accepts tag at the end of the line? If so, the following would
> not modify the line count:
> 
> /* cppcheck tag */
> line1 /* added cppcheck tag */
> line2 /* added cppcheck tag */
> line3 /* added cppcheck tag */

Luca answered to a similar, more generic, question a few days ago about
Coverity: https://marc.info/?l=xen-devel&m=169657904027210

So even if we get cppcheck to work like that, we would lose Coverity
support.

It doesn't seem there are a lot of good options here. In this case Luca
came up with a good idea on how to refactor the code so we should be
good.

But it doesn't seem we'll be able to come up with a general solution to
the multiline statement problem.

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

[Julien Grall]

Hi Stefano,

On 11/10/2023 00:39, Stefano Stabellini wrote:
> On Tue, 10 Oct 2023, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/10/2023 23:19, Stefano Stabellini wrote:
>>>>
>>>> I am not sure this is better. This is a long line to read. But this is a
>>>> personal opinion.
>>>>
>>>> On the technical side, can we easily teach a tool to format this kind of
>>>> exception? If not, then this should not be an exception we should
>>>> implement.
>>>
>>> I am not sure I understand what you mean by "can we easily teach a tool
>>> to format this kind of exception". Do you mean whether we can teach a
>>> tool to interpret a multiline statement as a single statement?
>>
>> Sorry for the wording was not clear. I was referring to tools formatting the
>> code (e.g. clang-format). Hopefully, at some point, we will finally have a way
>> to automatically format the code. So we need a coding style that can be easily
>> be checked.
>>
>> It is not clear to me whether your proposed exception would work. We may have
>> to allow longer lines (and this has drawback).
> 
> Yes, that's a good point. It will probably be an issue with clang-format.
> 
> 
>>> /* cppcheck tag */
>>> line1
>>> /* cppcheck tag */
>>> line2
>>> /* cppcheck tag */
>>> line3
>>>
>>> and that would end up changing the line numbers in the source files so
>>> the cppcheck report wouldn't match with the original line numbers any
>>> longer
>>
>> Would cppcheck accepts tag at the end of the line? If so, the following would
>> not modify the line count:
>>
>> /* cppcheck tag */
>> line1 /* added cppcheck tag */
>> line2 /* added cppcheck tag */
>> line3 /* added cppcheck tag */
> 
> Luca answered to a similar, more generic, question a few days ago about
> Coverity: https://marc.info/?l=xen-devel&m=169657904027210

Interesting.

> 
> So even if we get cppcheck to work like that, we would lose Coverity
> support.

I think my suggestion was probably misunderstood. So let me clarify it. 
To cover multi-line, we could write the following in Xen:

/* cppcheck tag next-3-lines */
line 1
line 2
line 3

AFAIU Eclair supports multi-line, so the tag would be transformed to 
there internal solution. For CPPCheck, this could be transformed to:

/* cppcheck tag next-3 lines */
line 1 /* generated cppcheck tag */
line 2 /* generated cppcheck tag */
line 3 /* generated cppcheck tag */

Now, I understand that coverity doesn't support any of the two format. 
So the only solution would be to add the comment before each line which 
would impact the line numbers. This is not great, but I think this is 
acceptable as the context would likely help to find where this is coming 
from.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 11/10/2023 10:45, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/10/2023 00:39, Stefano Stabellini wrote:
>> On Tue, 10 Oct 2023, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 09/10/2023 23:19, Stefano Stabellini wrote:
>>>>>
>>>>> I am not sure this is better. This is a long line to read. But this 
>>>>> is a
>>>>> personal opinion.
>>>>>
>>>>> On the technical side, can we easily teach a tool to format this 
>>>>> kind of
>>>>> exception? If not, then this should not be an exception we should
>>>>> implement.
>>>>
>>>> I am not sure I understand what you mean by "can we easily teach a tool
>>>> to format this kind of exception". Do you mean whether we can teach a
>>>> tool to interpret a multiline statement as a single statement?
>>>
>>> Sorry for the wording was not clear. I was referring to tools 
>>> formatting the
>>> code (e.g. clang-format). Hopefully, at some point, we will finally 
>>> have a way
>>> to automatically format the code. So we need a coding style that can 
>>> be easily
>>> be checked.
>>>
>>> It is not clear to me whether your proposed exception would work. We 
>>> may have
>>> to allow longer lines (and this has drawback).
>>
>> Yes, that's a good point. It will probably be an issue with clang-format.
>>
>>
>>>> /* cppcheck tag */
>>>> line1
>>>> /* cppcheck tag */
>>>> line2
>>>> /* cppcheck tag */
>>>> line3
>>>>
>>>> and that would end up changing the line numbers in the source files so
>>>> the cppcheck report wouldn't match with the original line numbers any
>>>> longer
>>>
>>> Would cppcheck accepts tag at the end of the line? If so, the 
>>> following would
>>> not modify the line count:
>>>
>>> /* cppcheck tag */
>>> line1 /* added cppcheck tag */
>>> line2 /* added cppcheck tag */
>>> line3 /* added cppcheck tag */
>>
>> Luca answered to a similar, more generic, question a few days ago about
>> Coverity: https://marc.info/?l=xen-devel&m=169657904027210
> 
> Interesting.
> 
>>
>> So even if we get cppcheck to work like that, we would lose Coverity
>> support.
> 
> I think my suggestion was probably misunderstood. So let me clarify it. 
> To cover multi-line, we could write the following in Xen:
> 
> /* cppcheck tag next-3-lines */
> line 1
> line 2
> line 3
> 
> AFAIU Eclair supports multi-line, so the tag would be transformed to 
> there internal solution. For CPPCheck, this could be transformed to:
> 
> /* cppcheck tag next-3 lines */
> line 1 /* generated cppcheck tag */
> line 2 /* generated cppcheck tag */
> line 3 /* generated cppcheck tag */
> 
> Now, I understand that coverity doesn't support any of the two format. 
> So the only solution would be to add the comment before each line which 
> would impact the line numbers. This is not great, but I think this is 
> acceptable as the context would likely help to find where this is coming 
> from.

Just to clarify, here I meant that for coverity, a script before the 
scan could convert to the multi-line version. So the line change only 
impact Coverity.

Cheers,

-- 
Julien Grall

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

[Luca Fancellu]

>>> 
>>> Luca answered to a similar, more generic, question a few days ago about
>>> Coverity: https://marc.info/?l=xen-devel&m=169657904027210
>> Interesting.
>>> 
>>> So even if we get cppcheck to work like that, we would lose Coverity
>>> support.
>> I think my suggestion was probably misunderstood. So let me clarify it. To cover multi-line, we could write the following in Xen:
>> /* cppcheck tag next-3-lines */
>> line 1
>> line 2
>> line 3
>> AFAIU Eclair supports multi-line, so the tag would be transformed to there internal solution. For CPPCheck, this could be transformed to:
>> /* cppcheck tag next-3 lines */
>> line 1 /* generated cppcheck tag */
>> line 2 /* generated cppcheck tag */
>> line 3 /* generated cppcheck tag */
>> Now, I understand that coverity doesn't support any of the two format. So the only solution would be to add the comment before each line which would impact the line numbers. This is not great, but I think this is acceptable as the context would likely help to find where this is coming from.
> 
> Just to clarify, here I meant that for coverity, a script before the scan could convert to the multi-line version. So the line change only impact Coverity.

Hi Julien,

We’ve tried to avoid that because when the tool insert lines, the resultant report would give wrong lines numbers if any violation is reported after the
insertion points. So there will be a mismatch between the codebase and the report findings from some point on in the file.

I’ve contacted Synopsys about the in-code comments, to discover if they have different syntax (only the one we know is proposed in the documentation),
I will let you know if something comes up.

Cheers,
Luca

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

[Julien Grall]

On 11/10/2023 11:53, Luca Fancellu wrote:
> 
>>>>
>>>> Luca answered to a similar, more generic, question a few days ago about
>>>> Coverity: https://marc.info/?l=xen-devel&m=169657904027210
>>> Interesting.
>>>>
>>>> So even if we get cppcheck to work like that, we would lose Coverity
>>>> support.
>>> I think my suggestion was probably misunderstood. So let me clarify it. To cover multi-line, we could write the following in Xen:
>>> /* cppcheck tag next-3-lines */
>>> line 1
>>> line 2
>>> line 3
>>> AFAIU Eclair supports multi-line, so the tag would be transformed to there internal solution. For CPPCheck, this could be transformed to:
>>> /* cppcheck tag next-3 lines */
>>> line 1 /* generated cppcheck tag */
>>> line 2 /* generated cppcheck tag */
>>> line 3 /* generated cppcheck tag */
>>> Now, I understand that coverity doesn't support any of the two format. So the only solution would be to add the comment before each line which would impact the line numbers. This is not great, but I think this is acceptable as the context would likely help to find where this is coming from.
>>
>> Just to clarify, here I meant that for coverity, a script before the scan could convert to the multi-line version. So the line change only impact Coverity.
> 
> Hi Julien,
> 
> We’ve tried to avoid that because when the tool insert lines, the resultant report would give wrong lines numbers if any violation is reported after the
> insertion points. So there will be a mismatch between the codebase and the report findings from some point on in the file.

I know. Stefano already pointed that out. But as I wrote, I don't think 
this is a big problem as it only affecte one tool (Coverity) and one 
would still be able to find the exact place based on the context.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

On Wed, 11 Oct 2023, Julien Grall wrote:
> On 11/10/2023 11:53, Luca Fancellu wrote:
> > 
> > > > > 
> > > > > Luca answered to a similar, more generic, question a few days ago
> > > > > about
> > > > > Coverity: https://marc.info/?l=xen-devel&m=169657904027210
> > > > Interesting.
> > > > > 
> > > > > So even if we get cppcheck to work like that, we would lose Coverity
> > > > > support.
> > > > I think my suggestion was probably misunderstood. So let me clarify it.
> > > > To cover multi-line, we could write the following in Xen:
> > > > /* cppcheck tag next-3-lines */
> > > > line 1
> > > > line 2
> > > > line 3
> > > > AFAIU Eclair supports multi-line, so the tag would be transformed to
> > > > there internal solution. For CPPCheck, this could be transformed to:
> > > > /* cppcheck tag next-3 lines */
> > > > line 1 /* generated cppcheck tag */
> > > > line 2 /* generated cppcheck tag */
> > > > line 3 /* generated cppcheck tag */
> > > > Now, I understand that coverity doesn't support any of the two format.
> > > > So the only solution would be to add the comment before each line which
> > > > would impact the line numbers. This is not great, but I think this is
> > > > acceptable as the context would likely help to find where this is coming
> > > > from.
> > > 
> > > Just to clarify, here I meant that for coverity, a script before the scan
> > > could convert to the multi-line version. So the line change only impact
> > > Coverity.
> > 
> > Hi Julien,
> > 
> > We’ve tried to avoid that because when the tool insert lines, the resultant
> > report would give wrong lines numbers if any violation is reported after the
> > insertion points. So there will be a mismatch between the codebase and the
> > report findings from some point on in the file.
> 
> I know. Stefano already pointed that out. But as I wrote, I don't think this
> is a big problem as it only affecte one tool (Coverity) and one would still be
> able to find the exact place based on the context.

Yeah I think we could consider going that way if it only affects 1 tool
out of 3.

We might still have to patch cppcheck to add the functionality but it
might not be that difficult.

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

[Jan Beulich]

On 05.10.2023 01:32, Stefano Stabellini wrote:
> On Wed, 4 Oct 2023, Luca Fancellu wrote:
>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote:
>>> On 04/10/2023 12:06, Luca Fancellu wrote:
>>>> Hi Nicola,
>>>>> On 4 Oct 2023, at 10:56, andrew.cooper3@citrix.com wrote:
>>>>> On 03/10/2023 9:46 pm, Stefano Stabellini wrote:
>>>>>> On Tue, 3 Oct 2023, 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@bugseng.com>
>>>>>> "SAF" discussion aside that can be resolved elsewhere:
>>>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>>>> Well no.  "SAF" aside (and SAF does need fixing before reposting this patch, otherwise it's just unnecessary churn), ...
>>>>>>> diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
>>>>>>> index d2a781fc3fb5..d0623b72ccfa 100644
>>>>>>> --- a/xen/arch/x86/hvm/svm/svm.h
>>>>>>> +++ b/xen/arch/x86/hvm/svm/svm.h
>>>>>>> @@ -57,14 +57,23 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
>>>>>>> #define INSTR_INT3 INSTR_ENC(X86EMUL_OPC( 0, 0xcc), 0)
>>>>>>> #define INSTR_ICEBP INSTR_ENC(X86EMUL_OPC( 0, 0xf1), 0)
>>>>>>> #define INSTR_HLT INSTR_ENC(X86EMUL_OPC( 0, 0xf4), 0)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
>>>>>>> +/* SAF-2-safe */
>>>>>>> #define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371)
>>>>>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>>>>> #define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
>>>>> ... this has broken a tabulated structure to have comments ahead of lines with octal numbers, while ...
>>>>>>> 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 has comments at the end of lines with octal numbers.
>>>>> So which is it?
>>>> I agree with Andrew here in this sense: the in-code comment is
>>>> supposed to be on the line *before* the violation,
>>>> not on the same line, so I’m also wondering how it is fixing the very
>>>> first violation.
>>>> Cheers,
>>>> Luca
>>>
>>
>> Hi Nicola,
>>
>>> Actually it justifies what is on either the previous line or the same because it's
>>> translated to /* -E> safe MC3R1.R7.1 1 */, where the last number is how many lines besides
>>> the current one are to be deviated (e.g. you can have 0 deviate only the current line).
>>
>> Just to understand, does this way:
>>
>> <line A>
>> /* -E> safe MC3R1.R7.1 1 */
>> <line B>
>>
>> Justifies only line B? Because I thought so, but now I want to be sure, otherwise it doesn’t act
>> as intended.
>>
>>
>>> Most of the times the current form is what's needed, as you would put the comment on a line
>>> of its own. In the case of the if that would break the formatting. The downside of doing the same thing on the table is that the first entry not to be deviated would actually be deviated.
>>>
>>> #define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
>>>
>>> This may not be problematic, since 0 could be considered an octal constant, but is an
>>> exception explicitly listed in the MISRA rule.
>>> For the same reason the line
>>>
>>> return emul_len;
>>>
>>> is deviated by the above comment, but putting an octal constant there would for sure
>>> be the result of a deliberate choice. There's the alternative of:
>>>
>>>                          /* SAF-2-safe */
>>>   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) )
>>>
>>> to make it consistent with the table and avoid any "hidden" deviated line or, again,
>>> the modification of the translation script so that it doesn't use a fixed "1" offset, which
>>> is motivated by what you wrote on the thread of the modification of xen_analysis.py.
>>
>> From the documentation:
>>
>>     In the Xen codebase, these tags will be used to document and suppress findings:
>>
>>     - SAF-X-safe: This tag means that the next line of code contains a finding, but
>>       the non compliance to the checker is analysed and demonstrated to be safe.
>>
>> I understand that Eclair is capable of suppressing also the line in which the in-code suppression
>> comment resides, but these generic Xen in-code suppression comment are meant to be used
>> by multiple static analysis tools and many of them suppress only the line next to the comment
>> (Coverity, cppcheck).
> 
> As we see more realistic examples, it turns out that this is limiting.
> 
> Given that the SAF-2-safe comment needs to go through xen-analysis.py
> translations anyway, could we implement something a bit more flexible in
> xen-analysis.py?
> 
> For instance, could we implement a format with the number of lines of
> code like this as we discussed in a previous thread?
> 
> /* SAF-2-safe start */
> if ( modrm_mod      == MASK_EXTR(instr_modrm, 0300) &&
>     (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) &&
>     (modrm_rm & 7)  == MASK_EXTR(instr_modrm, 0007) )
> /* SAF-2-safe end */
> 
> Firstly, let ask Andrew, do you prefer this?

The issue with this (or any other multi-line marking) is that it then
covers far more than just the offending piece of code. Yes, this is a
problem already for single lines of code, but the larger the scope,
the higher the risk of unintentionally silencing an analysis tool.

Furthermore what if more than one violation exists (and wants
silencing) within a such annotated block of code?

The case(s) at hand clearly back the original fear I had of such
annotations harming code readability more than acceptable.

Jan