.../eclair_analysis/ECLAIR/deviations.ecl | 7 ++-- docs/misra/deviations.rst | 7 ++++ docs/misra/safe.json | 8 ++++ xen/arch/x86/hvm/svm/emulate.c | 6 +-- xen/arch/x86/hvm/svm/svm.h | 38 +++++++++---------- xen/common/inflate.c | 4 +- 6 files changed, 42 insertions(+), 28 deletions(-)
As specified in rules.rst, these constants can be used
in the code.
Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
---
Changes in v2:
- replace some SAF deviations with configurations
Changes in v3:
- refine configurations and justifications
Changes in v4:
- updated deviation record comment.
Changes in v5:
- use octal-ok instead of keying the deviation to the file.
---
Indentation on svm.h has been modified to fit the whole line within
80 characters
---
.../eclair_analysis/ECLAIR/deviations.ecl | 7 ++--
docs/misra/deviations.rst | 7 ++++
docs/misra/safe.json | 8 ++++
xen/arch/x86/hvm/svm/emulate.c | 6 +--
xen/arch/x86/hvm/svm/svm.h | 38 +++++++++----------
xen/common/inflate.c | 4 +-
6 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..fabbf9d66330 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -85,10 +85,9 @@ conform to the directive."
# Series 7.
#
--doc_begin="Usage of the following constants is safe, since they are given as-is
-in the inflate algorithm specification and there is therefore no risk of them
-being interpreted as decimal constants."
--config=MC3R1.R7.1,literals={safe, "^0(007|37|070|213|236|300|321|330|331|332|333|334|335|337|371)$"}
+-doc_begin="It is safe to use certain octal constants the way they are defined
+in specifications, manuals, and algorithm descriptions."
+-config=MC3R1.R7.1,reports+={safe, "any_area(any_loc(any_exp(text(^.*octal-ok.*$))))"}
-doc_end
-doc_begin="Violations in files that maintainers have asked to not modify in the
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..8aaaa1473fb4 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules:
- __emulate_2op and __emulate_2op_nobyte
- read_debugreg and write_debugreg
+ * - R7.1
+ - It is safe to use certain octal constants the way they are defined
+ in specifications, manuals, and algorithm descriptions. Such places
+ are marked safe with a /\* octal-ok \*/ in-code comment, or with a SAF
+ comment (see safe.json).
+ - Tagged as `safe` for ECLAIR.
+
* - R7.2
- Violations caused by __HYPERVISOR_VIRT_START are related to the
particular use of it done in xen_mk_ulong.
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7d4..7ea47344ffcc 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -20,6 +20,14 @@
},
{
"id": "SAF-2-safe",
+ "analyser": {
+ "eclair": "MC3R1.R7.1"
+ },
+ "name": "Rule 7.1: constants defined in specifications, manuals, and algorithm descriptions",
+ "text": "It is safe to use certain octal constants the way they are defined in specifications, manuals, and algorithm descriptions."
+ },
+ {
+ "id": "SAF-3-safe",
"analyser": {},
"name": "Sentinel",
"text": "Next ID to be used"
diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index aa2c61c433b3..93ac1d3435f9 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) && /* octal-ok */
+ (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* octal-ok */
+ (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* octal-ok */
return emul_len;
}
diff --git a/xen/arch/x86/hvm/svm/svm.h b/xen/arch/x86/hvm/svm/svm.h
index d2a781fc3fb5..8dbf37ff4961 100644
--- a/xen/arch/x86/hvm/svm/svm.h
+++ b/xen/arch/x86/hvm/svm/svm.h
@@ -53,25 +53,25 @@ static inline void svm_invlpga(unsigned long linear, uint32_t asid)
*/
#define INSTR_ENC(opc, modrm) (((opc) << 8) | (modrm))
-#define INSTR_PAUSE INSTR_ENC(X86EMUL_OPC_F3(0, 0x90), 0)
-#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)
-#define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321)
-#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330)
-#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331)
-#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332)
-#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333)
-#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334)
-#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335)
-#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337)
-#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)
-#define INSTR_WRMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x30), 0)
-#define INSTR_RDTSC INSTR_ENC(X86EMUL_OPC(0x0f, 0x31), 0)
-#define INSTR_RDMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x32), 0)
-#define INSTR_CPUID INSTR_ENC(X86EMUL_OPC(0x0f, 0xa2), 0)
+#define INSTR_PAUSE INSTR_ENC(X86EMUL_OPC_F3(0, 0x90), 0)
+#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)
+#define INSTR_XSETBV INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0321) /* octal-ok */
+#define INSTR_VMRUN INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0330) /* octal-ok */
+#define INSTR_VMCALL INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0331) /* octal-ok */
+#define INSTR_VMLOAD INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0332) /* octal-ok */
+#define INSTR_VMSAVE INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0333) /* octal-ok */
+#define INSTR_STGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0334) /* octal-ok */
+#define INSTR_CLGI INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0335) /* octal-ok */
+#define INSTR_INVLPGA INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0337) /* octal-ok */
+#define INSTR_RDTSCP INSTR_ENC(X86EMUL_OPC(0x0f, 0x01), 0371) /* octal-ok */
+#define INSTR_INVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x08), 0)
+#define INSTR_WBINVD INSTR_ENC(X86EMUL_OPC(0x0f, 0x09), 0)
+#define INSTR_WRMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x30), 0)
+#define INSTR_RDTSC INSTR_ENC(X86EMUL_OPC(0x0f, 0x31), 0)
+#define INSTR_RDMSR INSTR_ENC(X86EMUL_OPC(0x0f, 0x32), 0)
+#define INSTR_CPUID INSTR_ENC(X86EMUL_OPC(0x0f, 0xa2), 0)
unsigned int svm_get_insn_len(struct vcpu *v, unsigned int instr_enc);
unsigned int svm_get_task_switch_insn_len(void);
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.1Hi Nicola, On 27/10/2023 16:11, Nicola Vetrini wrote: > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > index 8511a189253b..8aaaa1473fb4 100644 > --- a/docs/misra/deviations.rst > +++ b/docs/misra/deviations.rst > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules: > - __emulate_2op and __emulate_2op_nobyte > - read_debugreg and write_debugreg > > + * - R7.1 > + - It is safe to use certain octal constants the way they are defined > + in specifications, manuals, and algorithm descriptions. Such places > + are marked safe with a /\* octal-ok \*/ in-code comment, or with a SAF > + comment (see safe.json). Reading this, it is unclear to me why we have two ways to deviate the rule r7.1. And more importantely, how would the developper decide which one to use? Cheers, -- Julien Grall
On Mon, 30 Oct 2023, Julien Grall wrote: > Hi Nicola, > > On 27/10/2023 16:11, Nicola Vetrini wrote: > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > > index 8511a189253b..8aaaa1473fb4 100644 > > --- a/docs/misra/deviations.rst > > +++ b/docs/misra/deviations.rst > > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules: > > - __emulate_2op and __emulate_2op_nobyte > > - read_debugreg and write_debugreg > > + * - R7.1 > > + - It is safe to use certain octal constants the way they are defined > > + in specifications, manuals, and algorithm descriptions. Such places > > + are marked safe with a /\* octal-ok \*/ in-code comment, or with a > > SAF > > + comment (see safe.json). > > Reading this, it is unclear to me why we have two ways to deviate the rule > r7.1. And more importantely, how would the developper decide which one to use? I agree with you on this and we were discussing this topic just this morning in the FUSA community call. I think we need a way to do this with the SAF framework: if (some code with violation) /* SAF-xx-safe */ This doesn't work today unfortunately. It can only be done this way: /* SAF-xx-safe */ if (some code with violation) Which is not always desirable. octal-ok is just an ad-hoc solution for one specific violation but we need a generic way to do this. Luca is investigating possible ways to support the previous format in SAF. I think we should take this patch for now and harmonize it once SAF is improved.
Hi Stefano, On 30/10/2023 22:49, Stefano Stabellini wrote: > On Mon, 30 Oct 2023, Julien Grall wrote: >> Hi Nicola, >> >> On 27/10/2023 16:11, Nicola Vetrini wrote: >>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >>> index 8511a189253b..8aaaa1473fb4 100644 >>> --- a/docs/misra/deviations.rst >>> +++ b/docs/misra/deviations.rst >>> @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules: >>> - __emulate_2op and __emulate_2op_nobyte >>> - read_debugreg and write_debugreg >>> + * - R7.1 >>> + - It is safe to use certain octal constants the way they are defined >>> + in specifications, manuals, and algorithm descriptions. Such places >>> + are marked safe with a /\* octal-ok \*/ in-code comment, or with a >>> SAF >>> + comment (see safe.json). >> >> Reading this, it is unclear to me why we have two ways to deviate the rule >> r7.1. And more importantely, how would the developper decide which one to use? > > I agree with you on this and we were discussing this topic just this > morning in the FUSA community call. I think we need a way to do this > with the SAF framework: > > if (some code with violation) /* SAF-xx-safe */ > > This doesn't work today unfortunately. It can only be done this way: > > /* SAF-xx-safe */ > if (some code with violation) > > Which is not always desirable. octal-ok is just an ad-hoc solution for > one specific violation but we need a generic way to do this. Luca is > investigating possible ways to support the previous format in SAF. Why can't we use octal-ok everywhere for now? My point here is to make simple for the developper to know what to use. > > I think we should take this patch for now and harmonize it once SAF is > improved. The description of the deviation needs some improvement. To give an example, with the current wording, one could they can use octal-ok everywhere. But above, you are implying that SAF-xx-safe should be preferred. I would still strongly prefer if we use octal-ok everywhere because this is simple to remember. But if the other are happy to have both SAF-XX and octal-ok, then the description needs to be completely unambiguous and the patch should contain some explanation why we have two different ways to deviate. Cheers, -- Julien Grall
On Tue, 30 Oct 2023, Julien Grall wrote: > Hi Stefano, > > On 30/10/2023 22:49, Stefano Stabellini wrote: > > On Mon, 30 Oct 2023, Julien Grall wrote: > > > Hi Nicola, > > > > > > On 27/10/2023 16:11, Nicola Vetrini wrote: > > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst > > > > index 8511a189253b..8aaaa1473fb4 100644 > > > > --- a/docs/misra/deviations.rst > > > > +++ b/docs/misra/deviations.rst > > > > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules: > > > > - __emulate_2op and __emulate_2op_nobyte > > > > - read_debugreg and write_debugreg > > > > + * - R7.1 > > > > + - It is safe to use certain octal constants the way they are > > > > defined > > > > + in specifications, manuals, and algorithm descriptions. Such > > > > places > > > > + are marked safe with a /\* octal-ok \*/ in-code comment, or with > > > > a > > > > SAF > > > > + comment (see safe.json). > > > > > > Reading this, it is unclear to me why we have two ways to deviate the rule > > > r7.1. And more importantely, how would the developper decide which one to > > > use? > > > > I agree with you on this and we were discussing this topic just this > > morning in the FUSA community call. I think we need a way to do this > > with the SAF framework: > > > > if (some code with violation) /* SAF-xx-safe */ > > > > This doesn't work today unfortunately. It can only be done this way: > > > > /* SAF-xx-safe */ > > if (some code with violation) > > > > Which is not always desirable. octal-ok is just an ad-hoc solution for > > one specific violation but we need a generic way to do this. Luca is > > investigating possible ways to support the previous format in SAF. > > Why can't we use octal-ok everywhere for now? I think this is a good option for now, yes > My point here is to make simple for the developper to know what to use. > > > > > I think we should take this patch for now and harmonize it once SAF is > > improved. > > The description of the deviation needs some improvement. +1 > To give an example, > with the current wording, one could they can use octal-ok everywhere. But > above, you are implying that SAF-xx-safe should be > preferred. > > I would still strongly prefer if we use octal-ok everywhere because this is > simple to remember. But if the other are happy to have both SAF-XX and > octal-ok, then the description needs to be completely unambiguous and the > patch should contain some explanation why we have two different ways to > deviate. I think we could say "octal-ok" only and not mention SAF. As you can see from the other messages we still have work to do on SAF to be able to use it the way we would like to use it.
Hi Julien, Stefano On 2023-10-31 22:41, Stefano Stabellini wrote: > On Tue, 30 Oct 2023, Julien Grall wrote: >> Hi Stefano, >> >> On 30/10/2023 22:49, Stefano Stabellini wrote: >> > On Mon, 30 Oct 2023, Julien Grall wrote: >> > > Hi Nicola, >> > > >> > > On 27/10/2023 16:11, Nicola Vetrini wrote: >> > > > diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst >> > > > index 8511a189253b..8aaaa1473fb4 100644 >> > > > --- a/docs/misra/deviations.rst >> > > > +++ b/docs/misra/deviations.rst >> > > > @@ -90,6 +90,13 @@ Deviations related to MISRA C:2012 Rules: >> > > > - __emulate_2op and __emulate_2op_nobyte >> > > > - read_debugreg and write_debugreg >> > > > + * - R7.1 >> > > > + - It is safe to use certain octal constants the way they are >> > > > defined >> > > > + in specifications, manuals, and algorithm descriptions. Such >> > > > places >> > > > + are marked safe with a /\* octal-ok \*/ in-code comment, or with >> > > > a >> > > > SAF >> > > > + comment (see safe.json). >> > > >> > > Reading this, it is unclear to me why we have two ways to deviate the rule >> > > r7.1. And more importantely, how would the developper decide which one to >> > > use? >> > >> > I agree with you on this and we were discussing this topic just this >> > morning in the FUSA community call. I think we need a way to do this >> > with the SAF framework: >> > >> > if (some code with violation) /* SAF-xx-safe */ >> > >> > This doesn't work today unfortunately. It can only be done this way: >> > >> > /* SAF-xx-safe */ >> > if (some code with violation) >> > >> > Which is not always desirable. octal-ok is just an ad-hoc solution for >> > one specific violation but we need a generic way to do this. Luca is >> > investigating possible ways to support the previous format in SAF. >> >> Why can't we use octal-ok everywhere for now? > > I think this is a good option for now, yes > > >> My point here is to make simple for the developper to know what to >> use. >> >> > >> > I think we should take this patch for now and harmonize it once SAF is >> > improved. >> >> The description of the deviation needs some improvement. > > +1 > > >> To give an example, >> with the current wording, one could they can use octal-ok everywhere. >> But >> above, you are implying that SAF-xx-safe should be >> preferred. >> >> I would still strongly prefer if we use octal-ok everywhere because >> this is >> simple to remember. But if the other are happy to have both SAF-XX and >> octal-ok, then the description needs to be completely unambiguous and >> the >> patch should contain some explanation why we have two different ways >> to >> deviate. > > I think we could say "octal-ok" only and not mention SAF. As you can > see > from the other messages we still have work to do on SAF to be able to > use it the way we would like to use it. Thanks for the feedback; I'll revise the patch to use and mention only octal-ok. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
On Fri, 27 Oct 2023, Nicola Vetrini wrote: > As specified in rules.rst, these constants can be used > in the code. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
© 2016 - 2024 Red Hat, Inc.