| Message ID | a9c52c943380f2c35f0a6ccab8215c03e87c99dc.1697712857.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN,for-4.19,v2] xen: Add SAF deviations for MISRA C:2012 Rule 7.1 | expand |
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
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
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
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
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
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?
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?
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?
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
>>>> 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.
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.
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
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.
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; }
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(-) -- 2.34.1