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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > wrote:
> > > > 
> > > > On Thu, 5 Oct 2023, Luca Fancellu wrote:
> > > > > > On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > > wrote:
> > > > > > 
> > > > > > On Wed, 4 Oct 2023, Luca Fancellu wrote:
> > > > > > > > On 4 Oct 2023, at 11:29, Nicola Vetrini
> > > > > > > > <nicola.vetrini@xxxxxxxxxxx> wrote:
> > > > > > > > On 04/10/2023 12:06, Luca Fancellu wrote:
> > > > > > > > > Hi Nicola,
> > > > > > > > > > On 4 Oct 2023, at 10:56, andrew.cooper3@xxxxxxxxxx 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@xxxxxxxxxxx>
> > > > > > > > > > > "SAF" discussion aside that can be resolved elsewhere:
> > > > > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > > > > > > 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