> On 6 Oct 2023, at 02:02, Stefano Stabellini <sstabell...@kernel.org> wrote: > > On Thu, 5 Oct 2023, Luca Fancellu wrote: >>> On 5 Oct 2023, at 00:32, Stefano Stabellini <sstabell...@kernel.org> wrote: >>> >>> On Wed, 4 Oct 2023, Luca Fancellu wrote: >>>>> On 4 Oct 2023, at 11:29, Nicola Vetrini <nicola.vetr...@bugseng.com> >>>>> wrote: >>>>> On 04/10/2023 12:06, Luca Fancellu wrote: >>>>>> Hi Nicola, >>>>>>> On 4 Oct 2023, at 10:56, andrew.coop...@citrix.com 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.vetr...@bugseng.com> >>>>>>>> "SAF" discussion aside that can be resolved elsewhere: >>>>>>>> Reviewed-by: Stefano Stabellini <sstabell...@kernel.org> >>>>>>> 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.
