| Message ID | 338d8e574db943a86c7605e4c6d9a299d45f067d.1696347345.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN] xen: Add SAF deviations for MISRA C:2012 Rule 7.1 | expand |
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
> 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 >
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
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
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 >
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
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
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.
> 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
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.
> 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.
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)
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.
> 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.
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> ?
> 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.
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?
>>> >>> 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
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,
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
> >>> /* 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
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; } }
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,
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.
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,
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,
>>> >>> 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
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,
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.
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
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; }
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(-)