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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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:

/* 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?