Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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..81473fb4 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)
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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..81473fb4 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.
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
> On 31 Oct 2023, at 15:36, Julien Grall wrote: > > > > On 31/10/2023 15:32, Luca Fancellu wrote: >>> On 31 Oct 2023, at 15:27, Julien Grall wrote: >>> >>> Hi, >>> >>> On 31/10/2023 15:12, Luca Fancellu wrote: > On 31 Oct 2023, at 15:10, Nicola Vetrini > wrote: > > On 2023-10-31 15:13, Luca Fancellu wrote: >>> On 31 Oct 2023, at 13:27, 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..81473fb4 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. >> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ >> So that the suppression engine do what it should (currently it doesn’t >> suppress the same line, but we could do something about it) and the >> developer >> has a way to understand what is the violation here without going to the >> justification database. > > I guess. It could overflow the 80-char limit in > xen/arch/x86/hvm/svm/svm.h, though. Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars >>> >>> In the past I expressed concerned with this kind of the rule because it is >>> not entirely clear how an automatic formatter will be able to check it. >>> >>> Can you clarify whether clang-format would be able to handle your proposed >>> rule? >> So, yesterday Bertrand pointed out a StackOverflow thread for this issue and >> if we use ReflowComments: false we should >> be able to let the line as it is (not tested). > > Wouldn't that prevent reflow for all the comments? If so, I don't think this > is we want. Instead, we want to allow reflow for any comments but the one > done at the end of the line. Ok well, I was optimistic, in reality with the option as false, it would anyway reflow the line leaving the comment untouched. E.g. from this: if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe octal-ok */ (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe octal-ok */ (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe octal-ok */ return emul_len; To this: if ( modrm_mod == MASK_EXTR(instr_modrm, 0300) && /* SAF-2-safe octal-ok */ (modrm_reg & 7) == MASK_EXTR(instr_modrm, 0070) && /* SAF-2-safe octal-ok */ (modrm_rm & 7) == MASK_EXTR(instr_modrm, 0007) ) /* SAF-2-safe
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
On 31/10/2023 15:32, Luca Fancellu wrote: On 31 Oct 2023, at 15:27, Julien Grall wrote: Hi, On 31/10/2023 15:12, Luca Fancellu wrote: On 31 Oct 2023, at 15:10, Nicola Vetrini wrote: On 2023-10-31 15:13, Luca Fancellu wrote: On 31 Oct 2023, at 13:27, 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..81473fb4 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. Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer has a way to understand what is the violation here without going to the justification database. I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though. Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars In the past I expressed concerned with this kind of the rule because it is not entirely clear how an automatic formatter will be able to check it. Can you clarify whether clang-format would be able to handle your proposed rule? So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if we use ReflowComments: false we should be able to let the line as it is (not tested). Wouldn't that prevent reflow for all the comments? If so, I don't think this is we want. Instead, we want to allow reflow for any comments but the one done at the end of the line. Cheers, -- Julien Grall
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
> On 31 Oct 2023, at 15:27, Julien Grall wrote: > > Hi, > > On 31/10/2023 15:12, Luca Fancellu wrote: >>> On 31 Oct 2023, at 15:10, Nicola Vetrini wrote: >>> >>> On 2023-10-31 15:13, Luca Fancellu wrote: > On 31 Oct 2023, at 13:27, 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..81473fb4 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. Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer has a way to understand what is the violation here without going to the justification database. >>> >>> I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, >>> though. >> Yeah, but we could rule out something in code_style to allow only this kind >> of trailing comments to exceed the 80 chars > > In the past I expressed concerned with this kind of the rule because it is > not entirely clear how an automatic formatter will be able to check it. > > Can you clarify whether clang-format would be able to handle your proposed > rule? So, yesterday Bertrand pointed out a StackOverflow thread for this issue and if we use ReflowComments: false we should be able to let the line as it is (not tested). https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments > > Cheers, > > -- > Julien Grall
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
Hi, On 31/10/2023 15:12, Luca Fancellu wrote: On 31 Oct 2023, at 15:10, Nicola Vetrini wrote: On 2023-10-31 15:13, Luca Fancellu wrote: On 31 Oct 2023, at 13:27, 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..81473fb4 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. Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer has a way to understand what is the violation here without going to the justification database. I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though. Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars In the past I expressed concerned with this kind of the rule because it is not entirely clear how an automatic formatter will be able to check it. Can you clarify whether clang-format would be able to handle your proposed rule? Cheers, -- Julien Grall
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
> On 31 Oct 2023, at 15:10, Nicola Vetrini wrote: > > On 2023-10-31 15:13, Luca Fancellu wrote: >>> On 31 Oct 2023, at 13:27, 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..81473fb4 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. >> Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ >> So that the suppression engine do what it should (currently it doesn’t >> suppress the same line, but we could do something about it) and the developer >> has a way to understand what is the violation here without going to the >> justification database. > > I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, > though. Yeah, but we could rule out something in code_style to allow only this kind of trailing comments to exceed the 80 chars > > -- > Nicola Vetrini, BSc > Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
On 2023-10-31 15:13, Luca Fancellu wrote: On 31 Oct 2023, at 13:27, 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..81473fb4 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. Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer has a way to understand what is the violation here without going to the justification database. I guess. It could overflow the 80-char limit in xen/arch/x86/hvm/svm/svm.h, though. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
> On 31 Oct 2023, at 13:27, 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..81473fb4 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. Would it be ok to have both, for example: /* SAF-XX-safe octal-ok */ So that the suppression engine do what it should (currently it doesn’t suppress the same line, but we could do something about it) and the developer has a way to understand what is the violation here without going to the justification database.
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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..81473fb4 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
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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..81473fb4 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.
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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..81473fb4 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
Re: [XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
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 Reviewed-by: Stefano Stabellini
[XEN PATCH][for-4.19 v5] xen: Add deviations for MISRA C:2012 Rule 7.1
As specified in rules.rst, these constants can be used
in the code.
Signed-off-by: Nicola Vetrini
---
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..81473fb4 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_INT3INSTR_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
