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

[XEN PATCH][for-4.19 v2] 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. Suitable deviations records are added in deviations.rst

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- replace some SAF deviations with configurations
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 8 ++++----
 docs/misra/deviations.rst                        | 6 ++++++
 docs/misra/safe.json                             | 8 ++++++++
 xen/common/inflate.c                             | 4 ++--
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..3bf8e8b8fdec 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -85,10 +85,10 @@ conform to the directive."
 # 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_begin="Octal constants used as arguments to macro INSTR_ENC or MASK_EXTR
+can be used, because they appear as is in specifications, manuals, and
+algorithm descriptions."
+-config=MC3R1.R7.1,reports+={safe, "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
 -doc_end

 -doc_begin="Violations in files that maintainers have asked to not modify in the
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..f26eb371f3e4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,12 @@ Deviations related to MISRA C:2012 Rules:
          - __emulate_2op and __emulate_2op_nobyte
          - read_debugreg and write_debugreg

+   * - R7.1
+     - It is safe to use certain octal constants the way they are defined in
+       specifications, manuals, and algorithm descriptions as arguments to
+       macros 'INSTR_ENC' and 'MASK_EXTR'.
+     - Tagged as `safe` for ECLAIR.
+
    * - R7.2
      - Violations caused by __HYPERVISOR_VIRT_START are related to the
        particular use of it done in xen_mk_ulong.
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/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][for-4.19 v2] xen: Add SAF deviations for MISRA C:2012 Rule 7.1

[Jan Beulich]

On 19.10.2023 13:04, Nicola Vetrini wrote:
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -85,10 +85,10 @@ conform to the directive."
>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC or MASK_EXTR
> +can be used, because they appear as is in specifications, manuals, and
> +algorithm descriptions."
> +-config=MC3R1.R7.1,reports+={safe, "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}

INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the same
name could imo be introduced without issues in, say, Arm code. The above
would then needlessly suppress findings there, aiui.

MASK_EXTR() otoh is a global macro which ise used for various purposes.
Excluding checking there is imo going too far, too.

> --- a/docs/misra/deviations.rst
> +++ b/docs/misra/deviations.rst
> @@ -90,6 +90,12 @@ Deviations related to MISRA C:2012 Rules:
>           - __emulate_2op and __emulate_2op_nobyte
>           - read_debugreg and write_debugreg
> 
> +   * - R7.1
> +     - It is safe to use certain octal constants the way they are defined in
> +       specifications, manuals, and algorithm descriptions as arguments to
> +       macros 'INSTR_ENC' and 'MASK_EXTR'.
> +     - Tagged as `safe` for ECLAIR.

Similarly this wording is imo inappropriate, while ...

> --- 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."
> +        },

... this reads good to me.

Jan

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

[Nicola Vetrini]

On 19/10/2023 17:57, Jan Beulich wrote:
> On 19.10.2023 13:04, Nicola Vetrini wrote:
>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>> @@ -85,10 +85,10 @@ conform to the directive."
>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC or 
>> MASK_EXTR
>> +can be used, because they appear as is in specifications, manuals, 
>> and
>> +algorithm descriptions."
>> +-config=MC3R1.R7.1,reports+={safe, 
>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
> 
> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the 
> same
> name could imo be introduced without issues in, say, Arm code. The 
> above
> would then needlessly suppress findings there, aiui.
> 
> MASK_EXTR() otoh is a global macro which ise used for various purposes.
> Excluding checking there is imo going too far, too.

I should have thought about it; I can simply enforce the deviation to 
additionally match
only a specific file for each of the macros.

> 
>> --- a/docs/misra/deviations.rst
>> +++ b/docs/misra/deviations.rst
>> @@ -90,6 +90,12 @@ Deviations related to MISRA C:2012 Rules:
>>           - __emulate_2op and __emulate_2op_nobyte
>>           - read_debugreg and write_debugreg
>> 
>> +   * - R7.1
>> +     - It is safe to use certain octal constants the way they are 
>> defined in
>> +       specifications, manuals, and algorithm descriptions as 
>> arguments to
>> +       macros 'INSTR_ENC' and 'MASK_EXTR'.
>> +     - Tagged as `safe` for ECLAIR.
> 
> Similarly this wording is imo inappropriate, while ...
> 

I tried to be a bit more specific about what is actually being deviated, 
on the assumption
that the maintainers and contributors would find it more useful than 
parsing ecl files, but
if you prefer it to be more general, no problem.

>> --- 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."
>> +        },
> 
> ... this reads good to me.
> 
> Jan

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

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

[Jan Beulich]

On 19.10.2023 18:34, Nicola Vetrini wrote:
> On 19/10/2023 17:57, Jan Beulich wrote:
>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC or 
>>> MASK_EXTR
>>> +can be used, because they appear as is in specifications, manuals, 
>>> and
>>> +algorithm descriptions."
>>> +-config=MC3R1.R7.1,reports+={safe, 
>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>
>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the 
>> same
>> name could imo be introduced without issues in, say, Arm code. The 
>> above
>> would then needlessly suppress findings there, aiui.
>>
>> MASK_EXTR() otoh is a global macro which ise used for various purposes.
>> Excluding checking there is imo going too far, too.
> 
> I should have thought about it; I can simply enforce the deviation to 
> additionally match
> only a specific file for each of the macros.

That'll work for INSTR_ENC(), but not for MASK_EXTR().

>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -90,6 +90,12 @@ Deviations related to MISRA C:2012 Rules:
>>>           - __emulate_2op and __emulate_2op_nobyte
>>>           - read_debugreg and write_debugreg
>>>
>>> +   * - R7.1
>>> +     - It is safe to use certain octal constants the way they are 
>>> defined in
>>> +       specifications, manuals, and algorithm descriptions as 
>>> arguments to
>>> +       macros 'INSTR_ENC' and 'MASK_EXTR'.
>>> +     - Tagged as `safe` for ECLAIR.
>>
>> Similarly this wording is imo inappropriate, while ...
>>
> 
> I tried to be a bit more specific about what is actually being deviated, 
> on the assumption
> that the maintainers and contributors would find it more useful than 
> parsing ecl files, but
> if you prefer it to be more general, no problem.

Just dropping everything after the last comma would deal with my concern.

Jan

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

[Nicola Vetrini]

On 20/10/2023 08:38, Jan Beulich wrote:
> On 19.10.2023 18:34, Nicola Vetrini wrote:
>> On 19/10/2023 17:57, Jan Beulich wrote:
>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC or
>>>> MASK_EXTR
>>>> +can be used, because they appear as is in specifications, manuals,
>>>> and
>>>> +algorithm descriptions."
>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>> 
>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the
>>> same
>>> name could imo be introduced without issues in, say, Arm code. The
>>> above
>>> would then needlessly suppress findings there, aiui.
>>> 
>>> MASK_EXTR() otoh is a global macro which ise used for various 
>>> purposes.
>>> Excluding checking there is imo going too far, too.
>> 
>> I should have thought about it; I can simply enforce the deviation to
>> additionally match
>> only a specific file for each of the macros.
> 
> That'll work for INSTR_ENC(), but not for MASK_EXTR().
> 

Why? What I'm deviating is reports due to octal constants used in 
expressions
that contain MASK_EXTR in their expansion if and only if these are 
located in the
file svm.h.
No extra octal constant will match all these constraints.

>>>> --- a/docs/misra/deviations.rst
>>>> +++ b/docs/misra/deviations.rst
>>>> @@ -90,6 +90,12 @@ Deviations related to MISRA C:2012 Rules:
>>>>           - __emulate_2op and __emulate_2op_nobyte
>>>>           - read_debugreg and write_debugreg
>>>> 
>>>> +   * - R7.1
>>>> +     - It is safe to use certain octal constants the way they are
>>>> defined in
>>>> +       specifications, manuals, and algorithm descriptions as
>>>> arguments to
>>>> +       macros 'INSTR_ENC' and 'MASK_EXTR'.
>>>> +     - Tagged as `safe` for ECLAIR.
>>> 
>>> Similarly this wording is imo inappropriate, while ...
>>> 
>> 
>> I tried to be a bit more specific about what is actually being 
>> deviated,
>> on the assumption
>> that the maintainers and contributors would find it more useful than
>> parsing ecl files, but
>> if you prefer it to be more general, no problem.
> 
> Just dropping everything after the last comma would deal with my 
> concern.
> 
> Jan

Ok

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

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

[Jan Beulich]

On 20.10.2023 12:33, Nicola Vetrini wrote:
> On 20/10/2023 08:38, Jan Beulich wrote:
>> On 19.10.2023 18:34, Nicola Vetrini wrote:
>>> On 19/10/2023 17:57, Jan Beulich wrote:
>>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC or
>>>>> MASK_EXTR
>>>>> +can be used, because they appear as is in specifications, manuals,
>>>>> and
>>>>> +algorithm descriptions."
>>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>>>
>>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the
>>>> same
>>>> name could imo be introduced without issues in, say, Arm code. The
>>>> above
>>>> would then needlessly suppress findings there, aiui.
>>>>
>>>> MASK_EXTR() otoh is a global macro which ise used for various 
>>>> purposes.
>>>> Excluding checking there is imo going too far, too.
>>>
>>> I should have thought about it; I can simply enforce the deviation to
>>> additionally match
>>> only a specific file for each of the macros.
>>
>> That'll work for INSTR_ENC(), but not for MASK_EXTR().
>>
> 
> Why? What I'm deviating is reports due to octal constants used in 
> expressions
> that contain MASK_EXTR in their expansion if and only if these are 
> located in the
> file svm.h.
> No extra octal constant will match all these constraints.

New MASK_EXTR() uses can appear at any time, without necessarily
matching the justification.

Jan

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

[Nicola Vetrini]

On 20/10/2023 15:24, Jan Beulich wrote:
> On 20.10.2023 12:33, Nicola Vetrini wrote:
>> On 20/10/2023 08:38, Jan Beulich wrote:
>>> On 19.10.2023 18:34, Nicola Vetrini wrote:
>>>> On 19/10/2023 17:57, Jan Beulich wrote:
>>>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC 
>>>>>> or
>>>>>> MASK_EXTR
>>>>>> +can be used, because they appear as is in specifications, 
>>>>>> manuals,
>>>>>> and
>>>>>> +algorithm descriptions."
>>>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>>>> 
>>>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the
>>>>> same
>>>>> name could imo be introduced without issues in, say, Arm code. The
>>>>> above
>>>>> would then needlessly suppress findings there, aiui.
>>>>> 
>>>>> MASK_EXTR() otoh is a global macro which ise used for various
>>>>> purposes.
>>>>> Excluding checking there is imo going too far, too.
>>>> 
>>>> I should have thought about it; I can simply enforce the deviation 
>>>> to
>>>> additionally match
>>>> only a specific file for each of the macros.
>>> 
>>> That'll work for INSTR_ENC(), but not for MASK_EXTR().
>>> 
>> 
>> Why? What I'm deviating is reports due to octal constants used in
>> expressions
>> that contain MASK_EXTR in their expansion if and only if these are
>> located in the
>> file svm.h.
>> No extra octal constant will match all these constraints.
> 
> New MASK_EXTR() uses can appear at any time, without necessarily
> matching the justification.
> 
> Jan

Sorry, but I don't understand what's your concern exactly. With the 
improvements I proposed
(hence a new patch revision) I see the following possible future 
scenarios:

1. an use of MASK_EXTR() in a file other than x86/hvm/svm/emulate.c 
appears, with no
    use of octal constants in the expansion. This won't be deviated;
2. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with no use 
of octal
    constants in the expansion. This won't be deviated;
3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with octal
    constants in the expansion. This will be deviated;
4. an use of any other macro with an octal constant in its expansion 
won't be deviated,
    unless the configuration is suitably edited.

Does this address your concern?
-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

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

[Jan Beulich]

On 20.10.2023 16:58, Nicola Vetrini wrote:
> On 20/10/2023 15:24, Jan Beulich wrote:
>> On 20.10.2023 12:33, Nicola Vetrini wrote:
>>> On 20/10/2023 08:38, Jan Beulich wrote:
>>>> On 19.10.2023 18:34, Nicola Vetrini wrote:
>>>>> On 19/10/2023 17:57, Jan Beulich wrote:
>>>>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>>>>  # 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_begin="Octal constants used as arguments to macro INSTR_ENC 
>>>>>>> or
>>>>>>> MASK_EXTR
>>>>>>> +can be used, because they appear as is in specifications, 
>>>>>>> manuals,
>>>>>>> and
>>>>>>> +algorithm descriptions."
>>>>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>>>>>
>>>>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of the
>>>>>> same
>>>>>> name could imo be introduced without issues in, say, Arm code. The
>>>>>> above
>>>>>> would then needlessly suppress findings there, aiui.
>>>>>>
>>>>>> MASK_EXTR() otoh is a global macro which ise used for various
>>>>>> purposes.
>>>>>> Excluding checking there is imo going too far, too.
>>>>>
>>>>> I should have thought about it; I can simply enforce the deviation 
>>>>> to
>>>>> additionally match
>>>>> only a specific file for each of the macros.
>>>>
>>>> That'll work for INSTR_ENC(), but not for MASK_EXTR().
>>>>
>>>
>>> Why? What I'm deviating is reports due to octal constants used in
>>> expressions
>>> that contain MASK_EXTR in their expansion if and only if these are
>>> located in the
>>> file svm.h.
>>> No extra octal constant will match all these constraints.
>>
>> New MASK_EXTR() uses can appear at any time, without necessarily
>> matching the justification.
>>
>> Jan
> 
> Sorry, but I don't understand what's your concern exactly. With the 
> improvements I proposed
> (hence a new patch revision) I see the following possible future 
> scenarios:
> 
> 1. an use of MASK_EXTR() in a file other than x86/hvm/svm/emulate.c 
> appears, with no
>     use of octal constants in the expansion. This won't be deviated;
> 2. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with no use 
> of octal
>     constants in the expansion. This won't be deviated;
> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with octal
>     constants in the expansion. This will be deviated;

This is what I'm concerned of: How do you know up front whether such new
uses want deviating?

Jan

> 4. an use of any other macro with an octal constant in its expansion 
> won't be deviated,
>     unless the configuration is suitably edited.
> 
> Does this address your concern?

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

[Nicola Vetrini]

On 23/10/2023 08:34, Jan Beulich wrote:
> On 20.10.2023 16:58, Nicola Vetrini wrote:
>> On 20/10/2023 15:24, Jan Beulich wrote:
>>> On 20.10.2023 12:33, Nicola Vetrini wrote:
>>>> On 20/10/2023 08:38, Jan Beulich wrote:
>>>>> On 19.10.2023 18:34, Nicola Vetrini wrote:
>>>>>> On 19/10/2023 17:57, Jan Beulich wrote:
>>>>>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>>>>>  # 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_begin="Octal constants used as arguments to macro 
>>>>>>>> INSTR_ENC
>>>>>>>> or
>>>>>>>> MASK_EXTR
>>>>>>>> +can be used, because they appear as is in specifications,
>>>>>>>> manuals,
>>>>>>>> and
>>>>>>>> +algorithm descriptions."
>>>>>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>>>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>>>>>> 
>>>>>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of 
>>>>>>> the
>>>>>>> same
>>>>>>> name could imo be introduced without issues in, say, Arm code. 
>>>>>>> The
>>>>>>> above
>>>>>>> would then needlessly suppress findings there, aiui.
>>>>>>> 
>>>>>>> MASK_EXTR() otoh is a global macro which ise used for various
>>>>>>> purposes.
>>>>>>> Excluding checking there is imo going too far, too.
>>>>>> 
>>>>>> I should have thought about it; I can simply enforce the deviation
>>>>>> to
>>>>>> additionally match
>>>>>> only a specific file for each of the macros.
>>>>> 
>>>>> That'll work for INSTR_ENC(), but not for MASK_EXTR().
>>>>> 
>>>> 
>>>> Why? What I'm deviating is reports due to octal constants used in
>>>> expressions
>>>> that contain MASK_EXTR in their expansion if and only if these are
>>>> located in the
>>>> file svm.h.
>>>> No extra octal constant will match all these constraints.
>>> 
>>> New MASK_EXTR() uses can appear at any time, without necessarily
>>> matching the justification.
>>> 
>>> Jan
>> 
>> Sorry, but I don't understand what's your concern exactly. With the
>> improvements I proposed
>> (hence a new patch revision) I see the following possible future
>> scenarios:
>> 
>> 1. an use of MASK_EXTR() in a file other than x86/hvm/svm/emulate.c
>> appears, with no
>>     use of octal constants in the expansion. This won't be deviated;
>> 2. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with no use
>> of octal
>>     constants in the expansion. This won't be deviated;
>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with octal
>>     constants in the expansion. This will be deviated;
> 
> This is what I'm concerned of: How do you know up front whether such 
> new
> uses want deviating?
> 
> Jan
> 

I understand you concern now. I can argue that all the macros in that 
table have indeed
an octal constant in their definition (0 is explicitly allowed by 
MISRA).
This is also specified in the comment above the INSTR_ENC macro 
definition, therefore any
new addition should have an octal the second argument to INSTR_ENC.

>> 4. an use of any other macro with an octal constant in its expansion
>> won't be deviated,
>>     unless the configuration is suitably edited.
>> 
>> Does this address your concern?

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

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

[Jan Beulich]

On 23.10.2023 10:03, Nicola Vetrini wrote:
> On 23/10/2023 08:34, Jan Beulich wrote:
>> On 20.10.2023 16:58, Nicola Vetrini wrote:
>>> On 20/10/2023 15:24, Jan Beulich wrote:
>>>> On 20.10.2023 12:33, Nicola Vetrini wrote:
>>>>> On 20/10/2023 08:38, Jan Beulich wrote:
>>>>>> On 19.10.2023 18:34, Nicola Vetrini wrote:
>>>>>>> On 19/10/2023 17:57, Jan Beulich wrote:
>>>>>>>> On 19.10.2023 13:04, Nicola Vetrini wrote:
>>>>>>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>>>>>>> @@ -85,10 +85,10 @@ conform to the directive."
>>>>>>>>>  # 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_begin="Octal constants used as arguments to macro 
>>>>>>>>> INSTR_ENC
>>>>>>>>> or
>>>>>>>>> MASK_EXTR
>>>>>>>>> +can be used, because they appear as is in specifications,
>>>>>>>>> manuals,
>>>>>>>>> and
>>>>>>>>> +algorithm descriptions."
>>>>>>>>> +-config=MC3R1.R7.1,reports+={safe,
>>>>>>>>> "any_area(any_loc(any_exp(macro(^(INSTR_ENC|MASK_EXTR)$))))"}
>>>>>>>>
>>>>>>>> INSTR_ENC() is a local macro in x86'es AMD SVM code. A macro of 
>>>>>>>> the
>>>>>>>> same
>>>>>>>> name could imo be introduced without issues in, say, Arm code. 
>>>>>>>> The
>>>>>>>> above
>>>>>>>> would then needlessly suppress findings there, aiui.
>>>>>>>>
>>>>>>>> MASK_EXTR() otoh is a global macro which ise used for various
>>>>>>>> purposes.
>>>>>>>> Excluding checking there is imo going too far, too.
>>>>>>>
>>>>>>> I should have thought about it; I can simply enforce the deviation
>>>>>>> to
>>>>>>> additionally match
>>>>>>> only a specific file for each of the macros.
>>>>>>
>>>>>> That'll work for INSTR_ENC(), but not for MASK_EXTR().
>>>>>>
>>>>>
>>>>> Why? What I'm deviating is reports due to octal constants used in
>>>>> expressions
>>>>> that contain MASK_EXTR in their expansion if and only if these are
>>>>> located in the
>>>>> file svm.h.
>>>>> No extra octal constant will match all these constraints.
>>>>
>>>> New MASK_EXTR() uses can appear at any time, without necessarily
>>>> matching the justification.
>>>>
>>>> Jan
>>>
>>> Sorry, but I don't understand what's your concern exactly. With the
>>> improvements I proposed
>>> (hence a new patch revision) I see the following possible future
>>> scenarios:
>>>
>>> 1. an use of MASK_EXTR() in a file other than x86/hvm/svm/emulate.c
>>> appears, with no
>>>     use of octal constants in the expansion. This won't be deviated;
>>> 2. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with no use
>>> of octal
>>>     constants in the expansion. This won't be deviated;
>>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with octal
>>>     constants in the expansion. This will be deviated;
>>
>> This is what I'm concerned of: How do you know up front whether such 
>> new
>> uses want deviating?
> 
> I understand you concern now. I can argue that all the macros in that 
> table have indeed
> an octal constant in their definition (0 is explicitly allowed by 
> MISRA).
> This is also specified in the comment above the INSTR_ENC macro 
> definition, therefore any
> new addition should have an octal the second argument to INSTR_ENC.

Right, and I previously indicated I agree as far as INSTR_ENC() goes.
What we appear to continue to disagree about is MASK_EXTR().

Jan

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

[Nicola Vetrini]

>>>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with 
>>>> octal
>>>>     constants in the expansion. This will be deviated;
>>> 
>>> This is what I'm concerned of: How do you know up front whether such
>>> new
>>> uses want deviating?
>> 
>> I understand you concern now. I can argue that all the macros in that
>> table have indeed
>> an octal constant in their definition (0 is explicitly allowed by
>> MISRA).
>> This is also specified in the comment above the INSTR_ENC macro
>> definition, therefore any
>> new addition should have an octal the second argument to INSTR_ENC.
> 
> Right, and I previously indicated I agree as far as INSTR_ENC() goes.
> What we appear to continue to disagree about is MASK_EXTR().
> 

Yeah, sorry. What about

if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* octal-ok */
      (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok */
      (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok */
             return emul_len;

It does not really fit in the SAF framework, because the deviation is 
still done with a
configuration, but at least it gives some clear indication on how to 
introduce an octal
constant in this file.

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

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

[Nicola Vetrini]

On 23/10/2023 10:44, Nicola Vetrini wrote:
>>>>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with 
>>>>> octal
>>>>>     constants in the expansion. This will be deviated;
>>>> 
>>>> This is what I'm concerned of: How do you know up front whether such
>>>> new
>>>> uses want deviating?
>>> 
>>> I understand you concern now. I can argue that all the macros in that
>>> table have indeed
>>> an octal constant in their definition (0 is explicitly allowed by
>>> MISRA).
>>> This is also specified in the comment above the INSTR_ENC macro
>>> definition, therefore any
>>> new addition should have an octal the second argument to INSTR_ENC.
>> 
>> Right, and I previously indicated I agree as far as INSTR_ENC() goes.
>> What we appear to continue to disagree about is MASK_EXTR().
>> 
> 
> Yeah, sorry. What about
> 
> if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* octal-ok */
>      (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok */
>      (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok */
>             return emul_len;
> 
> It does not really fit in the SAF framework, because the deviation is
> still done with a
> configuration, but at least it gives some clear indication on how to
> introduce an octal
> constant in this file.

What I mean is that matching /* octal-ok */ is an additional constraint 
for the
deviation to be applied in that file.

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

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

[Jan Beulich]

On 23.10.2023 10:44, Nicola Vetrini wrote:
> 
>>>>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with 
>>>>> octal
>>>>>     constants in the expansion. This will be deviated;
>>>>
>>>> This is what I'm concerned of: How do you know up front whether such
>>>> new
>>>> uses want deviating?
>>>
>>> I understand you concern now. I can argue that all the macros in that
>>> table have indeed
>>> an octal constant in their definition (0 is explicitly allowed by
>>> MISRA).
>>> This is also specified in the comment above the INSTR_ENC macro
>>> definition, therefore any
>>> new addition should have an octal the second argument to INSTR_ENC.
>>
>> Right, and I previously indicated I agree as far as INSTR_ENC() goes.
>> What we appear to continue to disagree about is MASK_EXTR().
>>
> 
> Yeah, sorry. What about
> 
> if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* octal-ok */
>       (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok */
>       (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok */
>              return emul_len;
> 
> It does not really fit in the SAF framework, because the deviation is 
> still done with a
> configuration, but at least it gives some clear indication on how to 
> introduce an octal
> constant in this file.

Well, I don't mind the comment, but is the config change then going to
also match (part of) the comment, i.e. key off of not just MASK_EXTR()?

Jan

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

[Nicola Vetrini]

On 23/10/2023 10:47, Jan Beulich wrote:
> On 23.10.2023 10:44, Nicola Vetrini wrote:
>> 
>>>>>> 3. an use of MASK_EXTR() in x86/hvm/svm/emulate.c appears, with
>>>>>> octal
>>>>>>     constants in the expansion. This will be deviated;
>>>>> 
>>>>> This is what I'm concerned of: How do you know up front whether 
>>>>> such
>>>>> new
>>>>> uses want deviating?
>>>> 
>>>> I understand you concern now. I can argue that all the macros in 
>>>> that
>>>> table have indeed
>>>> an octal constant in their definition (0 is explicitly allowed by
>>>> MISRA).
>>>> This is also specified in the comment above the INSTR_ENC macro
>>>> definition, therefore any
>>>> new addition should have an octal the second argument to INSTR_ENC.
>>> 
>>> Right, and I previously indicated I agree as far as INSTR_ENC() goes.
>>> What we appear to continue to disagree about is MASK_EXTR().
>>> 
>> 
>> Yeah, sorry. What about
>> 
>> if ( modrm_mod       == MASK_EXTR(instr_modrm, 0300) && /* octal-ok */
>>       (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok 
>> */
>>       (modrm_rm  & 7) == MASK_EXTR(instr_modrm, 0007) )  /* octal-ok 
>> */
>>              return emul_len;
>> 
>> It does not really fit in the SAF framework, because the deviation is
>> still done with a
>> configuration, but at least it gives some clear indication on how to
>> introduce an octal
>> constant in this file.
> 
> Well, I don't mind the comment, but is the config change then going to
> also match (part of) the comment, i.e. key off of not just MASK_EXTR()?
> 
> Jan

Yes, I added that to my reply.

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