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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?

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.